Skip to content

feat: add unsaved changes confirmation to settings page (Closes #1086) - #1154

Open
sojetunde8 wants to merge 4 commits into
LabsCrypt:mainfrom
sojetunde8:fix/issue-1086-unsaved-changes-settings
Open

feat: add unsaved changes confirmation to settings page (Closes #1086)#1154
sojetunde8 wants to merge 4 commits into
LabsCrypt:mainfrom
sojetunde8:fix/issue-1086-unsaved-changes-settings

Conversation

@sojetunde8

Copy link
Copy Markdown
Contributor

Closes #1086

Summary

  • Track dirty state by comparing initial vs current values for all editable settings fields
  • Add native beforeunload handler to warn on tab close, browser refresh, and back/forward navigation
  • Add window.confirm prompts for in-app navigation via the "Connect Wallet" button and "Disconnect Wallet" button when dirty
  • Replace the "Connect Wallet" <Link> with a <button> to intercept navigation with confirmation

Testing

  • Added 7 unit tests covering: clean start, dirty detection for each field (theme, currency, format, decimals), no-confirm when unchanged, and disconnect flow without changes

@ogazboiz ogazboiz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good instinct on #1086 and the test coverage is appreciated, but two design problems block this:

  1. settings on main persist immediately (toggleTheme writes localStorage on click), yet isDirty stays true forever after any change, so users get warned about "unsaved" changes that are already saved. either introduce a real draft-plus-save model or reset initialValuesRef after each apply.
  2. the guard only wraps the two in-page buttons. all navbar links and any other internal Link navigation bypass it entirely, so the stated goal is not met. cover app-internal navigation or scope the issue down explicitly.
  3. small ones: the Link to button swap loses link semantics, and there is a stray blank-line diff where handleDisconnect was touched.

if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0

Comment thread frontend/src/hooks/useUnsavedChangesGuard.ts Fixed
@sojetunde8

Copy link
Copy Markdown
Contributor Author

good instinct on #1086 and the test coverage is appreciated, but two design problems block this:

  1. settings on main persist immediately (toggleTheme writes localStorage on click), yet isDirty stays true forever after any change, so users get warned about "unsaved" changes that are already saved. either introduce a real draft-plus-save model or reset initialValuesRef after each apply.
  2. the guard only wraps the two in-page buttons. all navbar links and any other internal Link navigation bypass it entirely, so the stated goal is not met. cover app-internal navigation or scope the issue down explicitly.
  3. small ones: the Link to button swap loses link semantics, and there is a stray blank-line diff where handleDisconnect was touched.

if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0

Rsolved, @ogazboiz

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Frontend] settings-content.tsx does not confirm before discarding unsaved changes on navigation

4 participants