feat(frontend): per-tab persisted table state + returnUrl round-trip#114
feat(frontend): per-tab persisted table state + returnUrl round-trip#114
Conversation
# Conflicts: # frontend/src/app/pages/accounts/accounts-upsert.component.ts
|
@gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive system for persisting and restoring table states (filters, sorting, and pagination) across the application's list and upsert views. It adds several utility helpers and services, such as TableStatePersistence and ReturnUrlHelper, to manage state synchronization and navigation with return URLs. The feedback highlights opportunities to reduce code duplication by centralizing shared logic in services or base classes, improving the performance of the local storage garbage collection, and ensuring standard form usability by using proper button types for submission.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #114 +/- ##
=======================================
Coverage 86.80% 86.80%
=======================================
Files 86 86
Lines 7153 7153
=======================================
Hits 6209 6209
Misses 717 717
Partials 227 227
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive table state persistence and restoration system across various list components, including Accounts, Categories, and Transactions. It introduces utility helpers for managing return URLs and synchronizing table configurations like filters and sorting with LocalStorage and query parameters. Review feedback identifies a logic error in filter merging where URL parameters are incorrectly overwritten by persisted state, a memory leak in the Transactions table component due to an unmanaged subscription, and a performance concern regarding the frequency of garbage collection in the state persistence helper.
…ubmit on upsert forms
|
Addressed bot review comments in b7a0476:
|
|
@gemini review |
…tilDestroyed on subscriptions
|
Round 2 in a4f74dc:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust mechanism for persisting and restoring table states—including filters, sorting, and pagination—across navigations using localStorage and URL query parameters. It adds several utility helpers and a session service to manage this state. The review feedback highlights missing property bindings in the transactions table and an incorrect merge order for filters that prevents URL parameters from overriding persisted state. Additionally, it recommends standardizing button types in upsert forms to use native submit behavior and moving navigation logic from component constructors to the ngOnInit lifecycle hook to follow Angular best practices.
|
Round 3 triage:
No code changes. |
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a robust system for persisting table states (filters, sorting, and pagination) and managing return URLs across various list and upsert pages. It introduces several utility helpers and a session service to handle state storage in localStorage and sessionStorage. The review feedback identifies several valid improvement opportunities: refactoring duplicated state restoration logic into a shared helper to reduce maintenance overhead, moving navigation logic out of constructors to avoid Angular lifecycle issues, and addressing potential ExpressionChangedAfterItHasBeenCheckedError risks when programmatically filtering tables. Additionally, the garbage collection logic in the persistence helper could be improved to handle full storage scenarios more effectively.
| if (route.snapshot.queryParamMap.get('restore') === '1') { | ||
| const stored = TableStatePersistence.read(this.stateKey, this.tabSession.id); | ||
| if (stored) { | ||
| if (stored.filters) this.filters = { ...this.filters, ...(stored.filters as { [s: string]: FilterMetadata }) }; | ||
| if (stored.sort && stored.sort.length > 0) this.multiSortMeta = stored.sort; | ||
| if (stored.global) this.initialGlobalFilter = stored.global; | ||
| const tagIds = stored.extra?.['tagIds']; | ||
| if (Array.isArray(tagIds)) this.selectedTagIds = tagIds as number[]; | ||
| } | ||
| TableStatePersistence.clear(this.stateKey, this.tabSession.id); | ||
| this.router.navigate([], { relativeTo: route, queryParams: { restore: null }, queryParamsHandling: 'merge', replaceUrl: true }); | ||
| } | ||
|
|
||
| const queryState = TableQueryStateHelper.decode(route.snapshot.queryParams); | ||
| if (queryState.filters) { | ||
| this.filters = { ...this.filters, ...(queryState.filters as { [s: string]: FilterMetadata }) }; | ||
| } | ||
| if (queryState.sort && queryState.sort.length > 0) { | ||
| this.multiSortMeta = queryState.sort; | ||
| } | ||
| if (queryState.global) { | ||
| this.initialGlobalFilter = queryState.global; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for restoring table state from localStorage and query parameters is duplicated across multiple list components (Accounts, Categories, Currencies, etc.). This increases maintenance overhead and the risk of inconsistent behavior. Consider refactoring this into a shared helper method or a base class that handles state restoration consistently across all list pages.
| if (Array.isArray(tagIds)) this.selectedTagIds = tagIds as number[]; | ||
| } | ||
| TableStatePersistence.clear(this.stateKey, this.tabSession.id); | ||
| this.router.navigate([], { relativeTo: route, queryParams: { restore: null }, queryParamsHandling: 'merge', replaceUrl: true }); |
There was a problem hiding this comment.
Performing navigation via this.router.navigate inside a component constructor is generally discouraged in Angular. The component is not yet fully initialized, which can lead to race conditions or unexpected behavior with the router's lifecycle. It is recommended to move this logic to ngOnInit.
| ngAfterViewInit() { | ||
| if (this.initialGlobalFilter && this.table) { | ||
| if (this.filter?.nativeElement) { | ||
| this.filter.nativeElement.value = this.initialGlobalFilter; | ||
| } | ||
| this.table.filterGlobal(this.initialGlobalFilter, 'contains'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling this.table.filterGlobal inside ngAfterViewInit can trigger an ExpressionChangedAfterItHasBeenCheckedError because it modifies the table's state after it has been checked by Angular's change detection. Wrapping the call in setTimeout or using a microtask is a safer approach to ensure it runs in a subsequent cycle.
ngAfterViewInit() {
if (this.initialGlobalFilter && this.table) {
setTimeout(() => {
if (this.filter?.nativeElement) {
this.filter.nativeElement.value = this.initialGlobalFilter;
}
this.table.filterGlobal(this.initialGlobalFilter, 'contains');
});
}
}| private static gc(): void { | ||
| const now = Date.now(); | ||
| if (now - this.lastGcAt < GC_MIN_INTERVAL_MS) return; | ||
| this.lastGcAt = now; | ||
| const toDrop: string[] = []; | ||
| try { | ||
| for (let i = 0; i < localStorage.length; i++) { | ||
| const k = localStorage.key(i); | ||
| if (!k || !k.startsWith(KEY_PREFIX)) continue; | ||
| const raw = localStorage.getItem(k); | ||
| if (!raw) continue; | ||
| try { | ||
| const entry = JSON.parse(raw) as StoredEntry; | ||
| if (typeof entry.expiresAt !== 'number' || entry.expiresAt < now) { | ||
| toDrop.push(k); | ||
| } | ||
| } catch { | ||
| toDrop.push(k); | ||
| } | ||
| } | ||
| } catch { | ||
| return; | ||
| } | ||
| for (const k of toDrop) this.removeItem(k); | ||
| } |
There was a problem hiding this comment.
The garbage collection process iterates over all keys in localStorage. While throttled, this can still be a performance concern if the origin has many items in storage. Additionally, gc() is only called after a successful write. If localStorage is full, setItem will throw and gc() will be skipped. Consider calling gc() before attempting to write to help free up space.
| if (stored.rows != null) this.initialRows = stored.rows; | ||
| } | ||
| TableStatePersistence.clear(this.stateKey, this.tabSession.id); | ||
| this.router.navigate([], { relativeTo: this.routeSnapshot, queryParams: { restore: null }, queryParamsHandling: 'merge', replaceUrl: true }); |
There was a problem hiding this comment.
Performing navigation via this.router.navigate inside a component constructor is generally discouraged in Angular. It is recommended to move this logic to ngOnInit to ensure the component is properly initialized and to avoid potential issues with the router's lifecycle.
1 participant
Summary
localStoragewith a 24 h TTL.returnUrlquery param and go back to it after save/cancel/delete instead of a hardcoded route.returnUrlcarries a transient?restore=1marker; the list applies the stored state once, clears the entry, and strips the flag — so opening the same page from the menu always gives a fresh view.?filters=...&sort=...(optional JSON query string) still works.sessionStorageUUID so two tabs don't clobber each other.type="button"on all action buttons inside<form (ngSubmit)>to prevent double-fire.Touched areas
frontend/src/app/shared/helpers/return-url.helper.ts— build/safe/withRestoreFlag.frontend/src/app/shared/helpers/table-query-state.helper.ts— URL ↔ state encode/decode (handles menu-modeFilterMetadata[], JSON validation).frontend/src/app/shared/helpers/table-state-persistence.helper.ts— localStorage r/w with TTL + GC.frontend/src/app/shared/services/tab-session.service.ts— per-tab UUID.transactions-table— read on?restore=1, write on change.navigateAfterSave()usesreturnUrl + ?restore=1, falls back to existing hardcoded route when absent.Test plan
/accounts— apply tag filter → edit → save → filter restores./accounts→ menu click back → fresh view (no filters)./accounts/liabilitiesroute-data preselect still applied./transactions/depositslazy table — filter/sort/page round-trip./transactions?title=rent&ignoreDateFilter=truestill works.?restore=1= fresh view.npx ng buildpasses.🤖 Generated with Claude Code