fix(settings): revalidate DB resource claims when switching DB nodes - #355
Conversation
Clicking between DB nodes retargets DbSettingsProvider in place, but SWR's default 2s dedupe window suppressed revalidation on a quick revisit, so the cards (Replicas & Resources, Storage, Connection Address) kept rendering the previously cached claim. Set dedupingInterval to 0 for product-resource fetches so every node (re)selection revalidates, and guard dbSettingsDataFromExactResource against claims that do not belong to the current target so a stale cache can never back another DB's cards.
|
@cursoragent review this |
|
I reviewed PR #355 and posted a summary plus three inline comments. The same-key SWR bug is real: a quick A→B→A revisit inside the 2s dedupe window serves the cached claim and skips the network. Test 4 is the right regression. I would not merge as-is — The identity guard does not run on that path (name/namespace still match a stale-but-correct DB). It would only help if SWR returned another key’s claim, which 2.4.1 does not do without   |
ReviewThe same-key SWR dedupe bug is real, and test 4 is the right regression for it. I would not merge this as-is: the shared-hook change is broader than the settings retarget, and the identity guard is sold as fixing “another DB’s cards” even though it cannot fire on the path this PR actually tests. SWR 2.4.1 ( That is a different bug from the PR opener (“cards kept rendering the previously viewed DB”). Title can move with
Request changes1. Do not hardcode List hooks already pass SWR flags through ( Smallest fix: optional 2. The identity guard is not the card-stale fix, and it is untested. If you keep it as defense-in-depth: add a unit test that injects a mismatched claim → Tests
Please do not describe “4 tests fail before / pass after.” Keep #4; replace #1–#3 with a cheap mismatch unit test and, if you care about the report, a connection-string assertion. Nits / questions
Verdict: request changes. Test 4 is worth keeping; the production change should be scoped to the settings retarget, and the guard should either be tested as defense-in-depth or not claimed as the fix. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Only a claim that actually belongs to the target may back its cards; a | ||
| // cached claim for another DB would render that DB's spec under this pane. | ||
| const metadata = asRecord(resource.metadata); | ||
| const name = typeof metadata?.name === "string" ? metadata.name : undefined; | ||
| const namespace = | ||
| typeof metadata?.namespace === "string" ? metadata.namespace : undefined; | ||
| if ( | ||
| name !== target.name || | ||
| (namespace !== undefined && namespace !== target.namespace) | ||
| ) { | ||
| return null; |
There was a problem hiding this comment.
This guard does not run on the bug this PR tests. Same-key stale cache (postgres claim, postgres target, spec changed on the backend) matches metadata.name/namespace, so the cards still render the old spec. Only dedupingInterval: 0 (or mutate() on retarget) fixes that path.
It would only help if SWR returned another key’s claim (keepPreviousData). That option is not set here, and SWR 2.4.1 does not keep previous-key data by default. So on A→B the cache slot is already empty/undefined before this check sees A’s body.
If this stays as defense-in-depth:
- Add a unit test that feeds a mismatched claim and expects
null. Todaysettings-section-model.test.tsxonly covers the happy path; none of the new node-switch tests inject a wrong-identity payload. - Fail closed on missing namespace.
namespace !== undefined && namespace !== target.namespaceaccepts a claim with nometadata.namespace, thendbResourceToSettingsData(..., { namespaceFallback: target.namespace })fills in the target namespace sodata.workloadlooks correct. displayNamea few lines below still readsk8sGetClaimBody(dbResource.data)without this check — if the guard is meant to stop another DB’s claim from backing this pane, the title path is a hole.
Please do not comment/PR-describe this as “a stale cache can never render another DB’s spec under this pane’s cards.” That is not what the reproduction demonstrates.
| // Settings surfaces retarget this hook as the user switches resource nodes; | ||
| // SWR's default dedupe window would suppress revalidation on a quick | ||
| // revisit and keep serving the previously cached claim as card values. | ||
| { dedupingInterval: 0, refreshInterval } |
There was a problem hiding this comment.
Hardcoding dedupingInterval: 0 here disables SWR’s 2s coalescing for every caller of useBrainProductResource, not just DB settings retarget:
DbSettingsProvider(the bug)useApWorkloadSettings(1s reconcile poll for 30s after save)useApImageUpdate(2s poll while a launch pending update exists)
Re-renders do not refetch, but focus (5s throttle), reconnect, refreshInterval, remount, and key change still do. Interval 0 means those no longer share a post-flight window — Strict Mode double-fetch in dev, settings + image-versions on the same AP, poll overlapping a focus revalidate.
useK8sNamespacedList / useDbsK8sList already take SWR flags as options. This hook should too (dedupingInterval?: number, default 2000), with 0 passed only from the settings providers that retarget in place.
Even smaller: mutate() already bypasses dedupe. In DbSettingsProvider, void dbResource.mutate() when target.name/namespace changes (skip the initial mount) fixes the A→B→A revisit without touching AP image versions.
The comment also describes the wrong failure mode. The 2s FETCH TTL suppresses a same-key revisit (A→B→A), not “another node’s cached claim.” A name change is a new SWR key; without keepPreviousData (default off in 2.4.1), A→B does not keep A’s data.
| test("DB settings sections refresh card values when the database data switches", async () => { | ||
| await withTestDom(async (actAndDrain) => { | ||
| let rendered: ReturnType<typeof render> | undefined; | ||
|
|
||
| await actAndDrain(() => { | ||
| rendered = render( | ||
| <DatabaseSettingsPaneContent | ||
| data={settingsDataFromClaim(POSTGRES_CLAIM)} | ||
| editable={false} | ||
| /> | ||
| ); | ||
| }); | ||
| assert.ok(rendered, "initial render"); | ||
| const initialView = rendered; | ||
| assert.match( | ||
| initialView.container.textContent ?? "", | ||
| POSTGRES_HEADING_PATTERN | ||
| ); | ||
| assert.deepEqual(sliderValues(initialView.container), [2, 1, 2, 20]); | ||
|
|
||
| await actAndDrain(() => { | ||
| initialView.rerender( | ||
| <DatabaseSettingsPaneContent | ||
| data={settingsDataFromClaim(REDIS_CLAIM)} | ||
| editable={false} | ||
| /> | ||
| ); | ||
| }); | ||
| assert.match( | ||
| initialView.container.textContent ?? "", | ||
| REDIS_HEADING_PATTERN | ||
| ); | ||
| assert.deepEqual( | ||
| sliderValues(initialView.container), | ||
| [4, 2, 4, 8], | ||
| "card values must follow the newly selected database" | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| test("DB settings provider refreshes card values when the target switches", async () => { | ||
| await withTestDom(async (actAndDrain) => { | ||
| const { calls, override } = stubFetch((url) => { | ||
| if (url.includes("affine-redis")) { | ||
| return jsonResponse(REDIS_CLAIM); | ||
| } | ||
| if (url.includes("affine-postgresql")) { | ||
| return jsonResponse(POSTGRES_CLAIM); | ||
| } | ||
| return jsonResponse({}); | ||
| }); | ||
| let rendered: ReturnType<typeof render> | undefined; | ||
|
|
||
| try { | ||
| await actAndDrain(() => { | ||
| rendered = render( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test", | ||
| target: postgresTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| assert.ok(rendered, "initial render"); | ||
| const view = rendered; | ||
| assert.deepEqual(sliderValues(view.container), [2, 1, 2, 20]); | ||
|
|
||
| await actAndDrain(() => { | ||
| view.rerender( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test", | ||
| target: redisTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| assert.deepEqual( | ||
| sliderValues(view.container), | ||
| [4, 2, 4, 8], | ||
| "card values must follow the newly selected DB node" | ||
| ); | ||
| assert.ok( | ||
| calls.some((call) => call.url.includes("affine-redis")), | ||
| "the redis DB resource must be fetched after the switch" | ||
| ); | ||
| } finally { | ||
| restoreGlobal(override); | ||
| } | ||
| await actAndDrain(() => undefined); | ||
| }); | ||
| }); | ||
|
|
||
| test("DB settings provider revalidates when switching back after backend changes", async () => { | ||
| await withTestDom(async (actAndDrain) => { | ||
| let postgresClaim = dbClaim({ | ||
| cpuLimit: "1", | ||
| engine: "postgresql", | ||
| memoryLimit: "2Gi", | ||
| name: "affine-postgresql", | ||
| replicas: 2, | ||
| storageSize: "20Gi", | ||
| }); | ||
| const { override } = stubFetch((url) => { | ||
| if (url.includes("affine-redis")) { | ||
| return jsonResponse(REDIS_CLAIM); | ||
| } | ||
| if (url.includes("affine-postgresql")) { | ||
| return jsonResponse(postgresClaim); | ||
| } | ||
| return jsonResponse({}); | ||
| }); | ||
| let rendered: ReturnType<typeof render> | undefined; | ||
|
|
||
| try { | ||
| await actAndDrain(() => { | ||
| rendered = render( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test-2", | ||
| target: postgresTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| assert.ok(rendered, "initial render"); | ||
| const view = rendered; | ||
| assert.deepEqual(sliderValues(view.container), [2, 1, 2, 20]); | ||
|
|
||
| await actAndDrain(() => { | ||
| view.rerender( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test-2", | ||
| target: redisTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| postgresClaim = dbClaim({ | ||
| cpuLimit: "2", | ||
| engine: "postgresql", | ||
| memoryLimit: "8Gi", | ||
| name: "affine-postgresql", | ||
| replicas: 5, | ||
| storageSize: "50Gi", | ||
| }); | ||
|
|
||
| // Wait past SWR's default 2s deduping interval before revisiting. | ||
| await actAndDrain(() => undefined, 2500); | ||
|
|
||
| await actAndDrain(() => { | ||
| view.rerender( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test-2", | ||
| target: postgresTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| assert.deepEqual( | ||
| sliderValues(view.container), | ||
| [5, 2, 8, 50], | ||
| "switching back to a node must reflect its current backend state" | ||
| ); | ||
| } finally { | ||
| restoreGlobal(override); | ||
| } | ||
| await actAndDrain(() => undefined); | ||
| }); | ||
| }); | ||
|
|
||
| test("DB settings provider revalidates a quickly revisited node within the dedupe window", async () => { | ||
| await withTestDom(async (actAndDrain) => { | ||
| let postgresClaim = dbClaim({ | ||
| cpuLimit: "1", | ||
| engine: "postgresql", | ||
| memoryLimit: "2Gi", | ||
| name: "affine-postgresql", | ||
| replicas: 2, | ||
| storageSize: "20Gi", | ||
| }); | ||
| const { calls, override } = stubFetch((url) => { | ||
| if (url.includes("affine-redis")) { | ||
| return jsonResponse(REDIS_CLAIM); | ||
| } | ||
| if (url.includes("affine-postgresql")) { | ||
| return jsonResponse(postgresClaim); | ||
| } | ||
| return jsonResponse({}); | ||
| }); | ||
| let rendered: ReturnType<typeof render> | undefined; | ||
|
|
||
| try { | ||
| await actAndDrain(() => { | ||
| rendered = render( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test-3", | ||
| target: postgresTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| assert.ok(rendered, "initial render"); | ||
| const view = rendered; | ||
| assert.deepEqual(sliderValues(view.container), [2, 1, 2, 20]); | ||
|
|
||
| await actAndDrain(() => { | ||
| view.rerender( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test-3", | ||
| target: redisTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| assert.deepEqual(sliderValues(view.container), [4, 2, 4, 8]); | ||
|
|
||
| postgresClaim = dbClaim({ | ||
| cpuLimit: "2", | ||
| engine: "postgresql", | ||
| memoryLimit: "8Gi", | ||
| name: "affine-postgresql", | ||
| replicas: 5, | ||
| storageSize: "50Gi", | ||
| }); | ||
|
|
||
| // Switch back immediately: no 2s wait. The revisit must still revalidate. | ||
| const postgresFetchCountBefore = calls.filter((call) => | ||
| call.url.includes("affine-postgresql") | ||
| ).length; | ||
| await actAndDrain(() => { | ||
| view.rerender( | ||
| providerElement({ | ||
| kubeconfig: "kubeconfig-switch-test-3", | ||
| target: postgresTarget(), | ||
| }) | ||
| ); | ||
| }); | ||
| const postgresFetchCountAfter = calls.filter((call) => | ||
| call.url.includes("affine-postgresql") | ||
| ).length; | ||
| assert.ok( | ||
| postgresFetchCountAfter > postgresFetchCountBefore, | ||
| "revisiting a node must issue a fresh resource fetch" | ||
| ); | ||
| assert.deepEqual( | ||
| sliderValues(view.container), | ||
| [5, 2, 8, 50], | ||
| "a quick revisit must not serve the stale cached claim" | ||
| ); | ||
| } finally { | ||
| restoreGlobal(override); | ||
| } | ||
| await actAndDrain(() => undefined); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Only this last test locks the regression (quick A→B→A inside the 2s FETCH TTL, backend A changed, must refetch and show 5 / 2 / 8 / 50).
The three above it would pass on main without this PR:
- sections refresh — rerenders
DatabaseSettingsPaneContentwith new props. No SWR, no provider, no identity guard. Draft/slider sync was already covered by identityKey reset. - provider target switch A→B — new SWR key; default SWR already fetches B.
- revisit after 2500ms — sleeps past the default 2s dedupe window, so old code would revalidate too. Please drop the wall-clock wait (or keep it only if you are asserting the default window still works when
dedupingIntervalis not 0).
The PR text that “4 tests fail before, pass after” is not accurate. Keep this test; replace the others with a cheap dbSettingsDataFromExactResource mismatch unit test if the identity guard stays.
Address PR review: useBrainProductResource keeps SWR's default dedupe window and takes an optional dedupingInterval instead of a hardcoded 0, so the AP workload-settings and image-update pollers keep coalescing. Only DbSettingsProvider opts into 0 — it retargets in place and a quick same-key revisit inside the default window served the cached claim. The identity guard is now shared (dbClaimBodyForTarget), fails closed when the claim has no namespace, and also backs the displayName read that previously used the unguarded claim. Tests: keep the quick-revisit regression, add claim-mismatch unit coverage and a provider-level foreign-claim case; drop the cases that also pass on main.
|
Review feedback addressed in 9c19977:
Open question from the review worth a product check: AP settings retargets the same way ( |
…es (#356) Follow-up to #355. Revealing a connection string on the DB Settings pane left the DSN in useRevealedRow state for 30s while the pane retargets in place, and the row key was connection.id (private/public) — identical across every DB Service. Switching nodes within the reveal window made the next DB's connection row display (and copy) the previous DB's full DSN. Scope the settings rows' keys by workload identity so a reveal can never match another DB's row, and clear the revealed row when the identity changes (useRevealedRow now exposes clearRevealedRow) so the secret does not linger in state after the switch.
2 participants
Problem
Clicking between different database nodes retargets the right-side DB Settings panel in place (same provider instance, new target). On a quick revisit of a previously viewed node — back inside SWR's default 2s dedupe window — revalidation was suppressed entirely: cache hit, no network request, and the cards (Replicas & Resources, Storage, Connection Address) kept rendering that node's earlier claim. Reproduced by an interaction test: view A (2 replicas / 1 core / 2Gi / 20Gi) → switch to B → backend A changes to (5 / 2 / 8Gi / 50Gi) → switch back within 2s → panel still shows (2 / 1 / 2 / 20) with zero fetches issued. Switching to a node never visited before was never affected (new SWR key fetches normally).
Fix
packages/api/src/hooks/use-product-resource.ts: new optionaldedupingIntervaloption. The hook keeps SWR's default window; only callers that retarget in place opt out. The AP workload-settings (1s reconcile poll) and image-update (2s poll) callers are untouched.apps/ui/src/features/resource-settings/settings-provider-db.tsx:DbSettingsProviderpassesdedupingInterval: 0so every node (re)selection revalidates immediately.dbClaimBodyForTargetonly accepts a claim whosemetadata.nameandmetadata.namespaceboth match the current target — failing closed when either is missing — and now also backs thedisplayNameread that previously used the unguarded claim. A foreign claim leaves the pane in its loading/unavailable state instead of rendering another DB's data.Testing
apps/ui/src/features/resource-settings/db/db-settings-node-switch.test.tsx:main).dbSettingsDataFromExactResource— wrong name / wrong namespace / missing namespace / no claim / no target.bun test src/features/resource-settings/— 252 pass;packages/api— 15 pass;bun typecheck,bun checkclean.