Conversation
leo-aa88
left a comment
leo-aa88
left a comment
There was a problem hiding this comment.
REQUEST CHANGES
The reported bug is real and the fix does make it disappear. But the patch doesn't distinguish "the project identity changed" from "we're reloading for the same project," and that distinction is exactly what this codebase already has a working pattern for, ten lines away, in the sibling component the linked issue itself cites (DataArea.tsx, which uses useEffect(() => setSpec(null), [projectID]) separately from its [projectID, reloadKey] fetch effect). The result: the fix trades a flicker that fires on project switch (rare) for a flicker that fires on every edit (constant). See inline comments.
VERDICT
This is an incomplete-fix problem, not a broken-fix problem: setHealth(null) is the right primitive, applied at the wrong granularity. Split the reset effect from the load effect ([projectID] vs [projectID, reloadKey]), add a test asserting facets persist across a same-project reloadKey bump, and this is mergeable.
| // Clear prior state at the start of every load so a project switch never | ||
| // renders the previous project's health while the new fetch is in flight, | ||
| // and a recovered fetch never leaves a stale error latched. | ||
| setHealth(null); |
There was a problem hiding this comment.
BLOCKING. This line is gated on the effect's dependency array, [projectID, reloadKey] (unchanged by this PR, further down in the hook). That means the reset fires on every effect run, including a reloadKey-only bump with projectID unchanged. Per this file's own top comment, reloadKey bumps "after each edit." So every edit in the visual editor now blanks the health panel (returns null, since !health && !error) until the refetch resolves, then pops it back in.
Before this PR, only error was cleared on every load; health persisted across a reloadKey-only reload, so a same-project refresh kept showing the last-good facets the whole time — no flicker. This PR removes that behavior as an unlisted side effect of fixing the project-switch case.
The codebase already has the correct decomposition for this, in DataArea.tsx (the component #206 itself cites as precedent):
useEffect(() => { setSpec(null); }, [projectID]); // reset only on identity change
useEffect(() => { /* fetch */ }, [projectID, reloadKey]); // fetch on identity OR reloadTwo effects, two dependency arrays, because "reset on switch" and "reload on edit" are different invariants. Folding both into one effect here means there's no way to reset on switch without also resetting on every same-project reload.
Concrete failure: project A open, panel shows 3 green facets. Operator renames a field (bumps reloadKey, projectID unchanged). Panel goes blank for the duration of the refetch, then facets reappear. Repeats on every edit — more visible than the bug being fixed, since edits are constant and project switches are occasional.
Fix: split into useEffect(() => { setHealth(null); setError(null); }, [projectID]) plus the existing fetch effect, mirroring DataArea.
| // project switch) must render live health again, not the stale error. | ||
| // Clear prior state at the start of every load so a project switch never | ||
| // renders the previous project's health while the new fetch is in flight, | ||
| // and a recovered fetch never leaves a stale error latched. |
There was a problem hiding this comment.
This comment describes project-switch semantics ("so a project switch never renders the previous project's health") but the code beneath it fires on every load, including same-project reloadKey reloads. A maintainer reading this has no reason to suspect the reload-after-edit flicker described in the sibling comment on line 27. Once that's split into its own [projectID] effect, this comment becomes accurate again.
| facets: [{ name: "Project A health", status: "ok", summary: "A is healthy" }], | ||
| }, | ||
| }), | ||
| } as Response; |
There was a problem hiding this comment.
Missing test: this regression test (and the existing error-recovery test below, which starts from an error state with no facets rendered) only cover project switches. Nothing renders healthy facets, bumps only reloadKey, and asserts the facets stay visible while the refetch is in flight. That's precisely the case the fix silently regresses (see the line-27 comment in HealthPanel.tsx), and it's why CI is green despite the regression.
2 participants
Closes #206
Summary
projectIDorreloadKeystarts a new health loadValidation
npm.cmd test -- src/areas/HealthPanel.test.tsx— 1 failed, 3 passednpm.cmd test -- src/areas/HealthPanel.test.tsx— 4/4 passednpm.cmd run typecheck— passednpm.cmd run build— passedgit diff --check— passedThe change is intentionally scoped to
HealthPaneland its regression test.