Skip to content

fix: preserve redux cache when updating url state - #4349

Merged
StekPerepolnen merged 1 commit into
ydb-platform:mainfrom
StekPerepolnen:fix/redux-url-state-cache
Sep 12, 2026
Merged

StekPerepolnen merged 1 commit into
ydb-platform:mainfrom
StekPerepolnen:fix/redux-url-state-cache

Conversation

@StekPerepolnen

@StekPerepolnen StekPerepolnen commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Navigation currently deep-copies the entire Redux state when merging URL-backed fields, including unchanged RTK Query cache data. With large disk responses, this adds unnecessary work and invalidates references used by selectors even when mapped URL values do not change.

Use Redux Toolkit's createNextState to merge only the URL-backed changes while preserving unchanged state and API cache references. Regression tests exercise the real location middleware and reducer wrapper: navigation with no mapped changes, URL-to-state updates, state-to-URL updates, preserved runtime query parameters, and browser back/forward history.

Extracted from #4348. This PR targets main and has no dependency on compact PDisk previews.

Validation:

  • npm run typecheck passed.
  • npm run lint passed (0 errors; 219 warnings).
  • npm test -- --runInBand passed: 217 suites, 1927 tests.
  • npm run build:embedded and npm run package passed.
  • git diff --check passed.
  • Browser E2E tests were not rerun for this extraction; navigation integration is covered by the Jest tests above.

RetriggerConfidence Score: 5/5

The changed URL-state flow preserves cached Redux data references and correctly updates URL-backed fields during navigation and history traversal.

What we checked:

  • Conducted a focused Jest comparison between the previous deep-merge behavior and the updated URL-state flow to verify that untouched cache references are not replaced and that api and api.queries.getNodes maintain strict identity through mapped URL navigation and history traversal, while queryTab, metricsTab, and sort update correctly. T-Rex
  • Validated baseline behavior that merge({}, state, query) replaces untouched cache references. T-Rex
  • Head checkout test passed, confirming that identity for api and api.queries.getNodes persists through mapped URL navigation and history traversal, and that queryTab, metricsTab, and sort update as expected. T-Rex

Summary

  • URL-to-Redux state updates now use structural sharing so unrelated cached data keeps its object references.
  • Focused coverage confirms mapped navigation, URL serialization, and back/forward history updates continue to apply the expected URL-backed fields.
  • No issues requiring changes were found; this is safe to merge.

Reviews (1) · Last reviewed commit: "fix: preserve redux cache when updating ..."

Copilot AI 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.

🟢 Approval recommended

No unresolved review issues remain, and regression coverage is included.

Pull request overview

Updates URL-backed Redux state synchronization to preserve unchanged Redux and RTK Query cache references.

Changes:

  • Replaces deep cloning with createNextState.
  • Adds navigation, URL synchronization, query preservation, and history regression tests.
File summaries
File Description
src/store/state-url-mapping.ts Preserves unchanged state references during URL merges.
src/store/state-url-mapping.test.ts Tests navigation and cache-reference preservation.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@astandrik

Copy link
Copy Markdown
Contributor

Approve — reviewed c06612f7.

  • Verified URL ↔ Redux synchronization, query-parameter preservation, explicit-tab back/forward navigation, and cache refresh. Both new identity tests fail on the base implementation and pass on this head.
  • Typecheck, product-scope linters, 217 test suites / 1927 tests, 6 additional integration checks, embedded build and package build passed.
  • Completed 30 headed browser stages across Chromium, WebKit and real local-ydb. All 37 captured navigation events with populated cache preserved cached-data references.

No introduced issues found. The missing-queryTab Editor → History → Back issue reproduces on both base and head and is outside this change. Browser traces also contained Canceled messages; their cause was not investigated separately. End-to-end speedup was not measured.

@astandrik

Copy link
Copy Markdown
Contributor

Reviewed c06612f against base 38b6856.

What was checked

  • Semantics: createNextState + lodash merge into the draft keeps the previous merge({}, state, query) rules — a mapped param that is absent from the URL still does not reset state, nested plain objects are merged in place, and slices not touched by the URL keep their identity. makeReducersWithLocation now returns the same root reference when nothing mapped changed; overwriteLocationHandling / restoreUnknownParams are unaffected.
  • Freeze side effect: immer autoFreeze now deep-freezes the whole root state (in production too). Audited all root reducers — everything is createSlice except singleClusterMode / fullscreen (booleans) and header (returns fresh objects) — and searched src/ for in-place mutation of store data: none found, no Map/Set in state. Safe today.
  • Tests: both new tests fail against the base implementation (the deep clone breaks reference identity) and pass on head.
  • Micro-benchmark (node 22, synthetic 13.6 MiB / 240k-object state with an RTK Query cache): merge({}, state, query) ≈ 140 ms per LOCATION action → ≤ 0.05 ms with createNextState, state.api reference preserved.
  • Browser A/B (dev builds of base and head against the same local YDB; /viewer/json/nodes mocked to 300 nodes × (24 PDisks + 192 VDisks) so the RTK cache is large; dev store checks disabled): 39-step scenario — query tabs, Database / Diagnostics tabs, metrics tabs, Top shards mode, heatmap sort/metric, topic consumer selection, browser back/forward. URLs and active tabs identical on every step, 0 page errors, the database param never lost. Long tasks on the 21 navigation-only steps: 6877 ms on base → 0 ms on head.

Optional, non-blocking

  1. One more sentence in the comment at mergeLocationToState would help: the result is deep-frozen by immer, so a future hand-written reducer that mutates its state in place would start throwing in strict mode.
  2. The tests mock ./reducers/heatmap and ./reducers/tenant/tenant with hand-written initialState (currentMetric: 'cpu' vs the real undefined); importing the real initialState objects avoids drift. A case "pushed URL lacks a mapped param while the state holds a non-default value → state kept, URL re-synced" would pin the one lodash-merge rule this rewrite must keep.
  3. Nit: two separate imports from @reduxjs/toolkit.

Verdict: approve — the notes above are optional.

@StekPerepolnen
StekPerepolnen added this pull request to the merge queue Sep 12, 2026
Merged via the queue into ydb-platform:main with commit 26f8c81 Sep 12, 2026
16 checks passed
@StekPerepolnen
StekPerepolnen deleted the fix/redux-url-state-cache branch September 12, 2026 15:18
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.

3 participants