Fix light account/backup notification banner display behavior #5898
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 178ffd38c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // If we've already read from disk, return the cached settings. | ||
| // This prevents stale disk reads from overwriting newer in-memory writes | ||
| // that may not have been persisted to disk yet. | ||
| if (readSettingsFromDisk) { | ||
| return localAccountSettings |
There was a problem hiding this comment.
The new short-circuit in readLocalAccountSettings returns the module‑level localAccountSettings whenever readSettingsFromDisk is true, but that flag is global and never reset on logout/account switch. If a user logs out and logs into a different account without restarting the app, this will reuse the previous account’s settings instead of reading the new account’s Settings.json, so notification state, reminders, and other local settings can be wrong for the new account. This regression is introduced by the unconditional cache return here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
What the AI said. It's a super-easy bug to miss, and we've made this same mistake several times in the past.
We generally avoid reading settings async, but instead just grab them from redux. With Sam's new login optimizations, the local settings should be loaded before we show any UI, so you can just rely on that.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
swansontec
left a comment
swansontec
left a comment
There was a problem hiding this comment.
Basically what the AI said.
| // If we've already read from disk, return the cached settings. | ||
| // This prevents stale disk reads from overwriting newer in-memory writes | ||
| // that may not have been persisted to disk yet. | ||
| if (readSettingsFromDisk) { | ||
| return localAccountSettings |
There was a problem hiding this comment.
What the AI said. It's a super-easy bug to miss, and we've made this same mistake several times in the past.
We generally avoid reading settings async, but instead just grab them from redux. With Sam's new login optimizations, the local settings should be loaded before we show any UI, so you can just rely on that.
"The new short-circuit in readLocalAccountSettings returns the module‑level localAccountSettings whenever readSettingsFromDisk is true, but that flag is global and never reset on logout/account switch. If a user logs out and logs into a different account without restarting the app, this will reuse the previous account's settings instead of reading the new account's Settings.json" - chatgpt-codex-connector, swansontec Adds resetLocalAccountSettingsCache() and calls it on logout to properly reset the cache between account sessions.
Jon-edge
force-pushed
the
jon/fix/light-account-banner
branch
from
9a75826 to
a6fe962
Compare
Jon-edge
force-pushed
the
jon/fix/light-account-banner
branch
from
a6fe962 to
0dfef44
Compare
Jon-edge
force-pushed
the
jon/fix/light-account-banner
branch
2 times, most recently
from
0dfef44 to
4a1aeef
Compare
"The lightAccountReminder notification is no longer properly cleaned up when a user upgrades from a light account to a full account. Previously, the call to updateNotificationInfo(account, 'lightAccountReminder', isLightAccountReminder) would set isCompleted: true and isBannerHidden: true when isLightAccountReminder became false. With that call removed... the notification is never marked complete after upgrade." - cursor Updates the effect to mark lightAccountReminder complete when user upgrades from a light account.
Jon-edge
force-pushed
the
jon/fix/light-account-banner
branch
from
4a1aeef to
1781a5c
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have:
Note
lightAccountReminderon login for light accounts; auto-complete/hide when upgrading to full accounts.notifStatewrites to avoid concurrent overwrites; remove duplicate effect; watchusernameinstead ofaccount.username.resetLocalAccountSettingsCache()on logout and prefer cached settings after first read to prevent stale disk reads overwriting newer in-memory data.Written by Cursor Bugbot for commit 1781a5c. This will update automatically on new commits. Configure here.