Fix custom report editor retaining unsaved settings#7356
Fix custom report editor retaining unsaved settings#7356tmchow wants to merge 2 commits intoactualbudget:masterfrom
Conversation
The session storage clear condition only fired when navigating from the /reports dashboard. Since the URL tracking runs inside the report component, the stored URL always pointed to the last report path, so revisiting the same report never triggered the clear. Changed the condition to clear session storage whenever the stored URL differs from the current path. This handles navigating from the dashboard, from another report, or any other page. Fixes actualbudget#7332
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
📝 WalkthroughWalkthroughSession storage clearing in CustomReportInner was changed to run only when the previous URL differs from the current pathname ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)
186-186: Consider adding a brief comment explaining the clearing condition.The condition
prevUrl !== location.pathnameencodes domain logic ("clear unsaved state when navigating from a different route") that may not be immediately obvious to future maintainers.📝 Suggested comment
+ // Clear session storage when navigating from a different route to reset unsaved changes if (prevUrl !== location.pathname) sessionStorage.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/CustomReport.tsx` at line 186, The line clearing sessionStorage when prevUrl !== location.pathname encodes domain logic that isn't obvious; add a brief inline comment above the check in CustomReport.tsx explaining that this condition clears unsaved report state when the user navigates to a different route (i.e., when prevUrl differs from location.pathname), and mention any assumptions (e.g., prevUrl holds the last route for this report) so future maintainers understand why sessionStorage.clear() is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/desktop-client/src/components/reports/reports/CustomReport.tsx`:
- Line 186: The line clearing sessionStorage when prevUrl !== location.pathname
encodes domain logic that isn't obvious; add a brief inline comment above the
check in CustomReport.tsx explaining that this condition clears unsaved report
state when the user navigates to a different route (i.e., when prevUrl differs
from location.pathname), and mention any assumptions (e.g., prevUrl holds the
last route for this report) so future maintainers understand why
sessionStorage.clear() is invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60400474-5d38-461f-b9a2-f07920940c10
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx
|
🤖 Auto-generated Release Notes Hey @tmchow! I've automatically created a release notes file based on CodeRabbit's analysis: Category: Bugfixes If you're happy with this release note, you can add it to your pull request. If not, you'll need to add your own before a maintainer can review your change. |
tmchow
changed the title
matt-fidd
force-pushed
the
master
branch
2 times, most recently
from
5c7c70d to
d262f7d
Compare
1 participant
Summary
Custom report settings (like graph type) were persisting between visits even when not saved. Changed the session storage clear condition so it fires whenever the stored URL differs from the current path, not just when coming from
/reports.Root cause
The URL tracking in
CustomReportInnerstores the current path insessionStorage.url. The clear condition only checkedif (['/reports'].includes(prevUrl)), but since this tracking code runs inside the report component (not the dashboard), the stored URL always pointed to a report path, never/reports. So the clear never fired when revisiting the same report.Changes
One-line change in
CustomReport.tsx(line 186): replaced the/reportscheck with a path comparison (prevUrl !== location.pathname). This clears session storage whenever the user navigates to the report from any different page (dashboard, another report, or external link), while preserving state during same-page interactions like switching tabs or resizing.Reproduction (from issue)
Fixes #7332
This contribution was developed with AI assistance (Claude Code).
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
locale/zh-Hans.jsonlocale/ca.jsonsrc/components/reports/reports/CustomReport.tsxView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
Unchanged
loot-core
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
api
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged