Skip to content

Conversation

@bytedream
Copy link
Contributor

Currently, when editing or deleting a file and the edit/commit form has changes, navigating the file tree will discard all changes without any warning. This PR prevents partial reloading when the edit form has unsaved changes, which will trigger a browser native warning dialog.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 11, 2025
@silverwind
Copy link
Member

silverwind commented Dec 11, 2025

So I understand that the JS would navigate to another entry, ignoring the "dirty form" state, and by adding this condition, it makes it skip the JS navigation via navigateTreeView and instead trigger a full-page load with the dirty checking done inside beforeunload by jquery.are-you-sure.

A better solution would be to make navigateTreeView perform the dirty checking and warning itself. That way, it will work without a disruptive full-page load when the user wants to discard their changes.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 12, 2025

A better solution would be to make navigateTreeView perform the dirty checking and warning itself. That way, it will work without a disruptive full-page load when the user wants to discard their changes.

I don't think it is a must at the moment. are-you-sure.js module has many internal logic, since it is there, we should reuse that module, but not re-invent the wheel and write a new "are-you-sure" from scratch, unless you would replace the legacy "are-you-sure.js" completely


The "dirty check" (document.querySelector('.repo-view-content .form.dirty')) can be a helper function exported from the are-you-sure.js to make the code more stable and maintainable.

image

The real logic is here:

image

@silverwind
Copy link
Member

silverwind commented Dec 12, 2025

unless you would replace the legacy "are-you-sure.js" completely

Yes I think it's best to completely replace it with custom logic eventually. The plugin only works for full page navigation, while here we have a case of JS-based navigation (pushState and replaceState). It needs to support both forms of navigation.

The "dirty check" (document.querySelector('.repo-view-content .form.dirty')) can be a helper function exported from the are-you-sure.js to make the code more stable and maintainable.

Yes, this would be a good way to expose the dirty checking as a function used for JS-based navigation.

@silverwind
Copy link
Member

silverwind commented Dec 12, 2025

Oh and I still think the "unsaved warning" should eventually be replaced by a mechanism that automatically saves and restores unsaved content (based on URL) if the user decides to navigate back. E.g. like https://github.com/github/session-resume.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants