fix(lineage): detach run results from focus - #1495
Conversation
Signed-off-by: Jared Scott <jared.scott@datarecce.io>
LineageViewOss no longer imports isHistogramDiffRun, isProfileDiffRun, isTopKDiffRun, isValueDiffDetailRun, or isValueDiffRun, and runResultVisibility imports Run as a type only. The factory entries in the @datarecce/ui/api mock therefore back no runtime call and imply a gate that no longer exists. Verified by deletion: the suite stays at 85/85. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Kent <iamcxa@gmail.com>
iamcxa
left a comment
There was a problem hiding this comment.
@gcko Approving. The fix is correct, and I verified it holds under mutation rather than just reading it.
What I verified
Root cause matches the fix. LineageViewOss.tsx:1196-1198 leaves selectedRunModel undefined for a non-node-bound run, and the else branch at :1258-1261 then calls setFocusedNodeId(undefined). Pre-PR, single-environment profile runs fell into exactly that branch — that is "the model detail pane empties." Adding "profile" to isNodeBoundRunResult addresses the real cause, not a symptom.
The new test genuinely protects it. Deleting case "profile": from isNodeBoundRunResult drops the run focus synchronization suite to PASS 7 / FAIL 1, failing precisely the "profile" variant. Not a vacuous test.
Making the import type-only matters more than it looks. @datarecce/ui/api aliases to packages/ui/src/api.ts — the same module runResultVisibility.ts resolved to via "../../api". Before this PR, the component test's vi.mock was intercepting those guards, so isNodeBoundRunResult could never run for real in that suite. Dropping the runtime import is what makes the new coverage meaningful.
profile_distribution is correctly excluded — it is backend-only (run.ts:42-45, no registry entry) and never reaches the single-active-run mechanism this effect operates on. row_count / row_count_diff carry node_names, not model, so excluding them is right.
Verification summary
| Check | Result |
|---|---|
runResultVisibility.test.ts |
8/8 pass |
LineageView.component.test.tsx |
85/85 pass |
pnpm type:check (root, @datarecce/ui, @datarecce/storybook) |
clean |
| Biome on changed files | clean (one pre-existing suppression warning at L22, untouched here) |
CI on head 08261082 |
8/8 pass |
Mutation: remove case "profile": |
FAIL 1 — fix is covered |
| Mutation: force continuous focus sync | FAIL 6 — the one-shot intent guard is covered |
One commit I pushed
08261082 removes five now-dead is*Run entries from the @datarecce/ui/api mock in LineageView.component.test.tsx. After this PR, LineageViewOss.tsx has zero references to those guards and runResultVisibility.ts imports Run as a type only, so the mock entries back no runtime call and imply a gate that no longer exists. Verified by deletion: the suite stays at 85/85. Revert it freely if you would rather it not ride along.
Follow-up filed, not blocking this PR
DRC-4089 captures two pre-existing issues that live in the code this PR centralizes, both explicitly out of scope here:
-
query_baseis missing from the non-node-bound list atLineageViewOss.tsx:1179, whileRunResultPaneOss.tsx:167anduseCSVExport.ts:176/250/292all treat["query", "query_base", "query_diff"]as one group. Sinceruncomes from the app-wide context andMainLayout.tsx:167-175keeps the lineage view always mounted, running a base query clears the focused node. -
The classification lives in three hand-maintained lists (
runResultVisibility.ts:8-16,:31-38, andLineageViewOss.tsx:1179) withdefault: return falseat the end. A run type added to theRununion but forgotten here is silently misclassified instead of a compile error — which is exactly how this bug happened,profilehaving been in the union all along.
Worth noting this is the third fix in this area (DRC-2779 → a738fef1, 7d5e9eb8/5360d424, now 44b3fdfc). Each added a case; none made the next omission detectable. The repo already has the pattern for closing that: d9beadaa made the badge legend fail to compile when a badge kind is unranked.
Two inline notes below — neither blocks merge.
| ); | ||
|
|
||
| it.each(["row_count", "row_count_diff"] as const)( | ||
| "keeps node navigation independent of an open %s result", |
There was a problem hiding this comment.
These two row-count cases do not actually exercise isNodeBoundRunResult. LineageViewOss.tsx:1179 short-circuits row_count / row_count_diff out of the whole effect before the helper is ever consulted.
Verified by mutation: adding case "row_count": case "row_count_diff": to the switch in runResultVisibility.ts leaves this suite at PASS 8 / FAIL 0. The real protection for that behavior is the unit case at runResultVisibility.test.ts:32-34, which does fail under the same mutation.
So the PR description's claim — "treating row-count results as node-bound makes both row-count cases fail" — holds for the unit test but not for these two. Worth rewording the description, and possibly renaming this block to say what it verifies: that the early type-exclusion list keeps navigation independent. The tests themselves are fine and worth keeping.
| "value_diff", | ||
| "value_diff_detail", | ||
| ] as const)( | ||
| "detaches an open %s result when the user focuses another node", |
There was a problem hiding this comment.
Scoping note, not a change request. NodeViewOss is stubbed to render only data-node-id, so these assertions cover "focus moved and the run was not force-closed" — symptom (a). They are a proxy for symptom (b), "the profile diff pane empties": if a regression re-broke rendering inside the real NodeViewOss / ProfileDiffResultView while leaving closeRunResult uncalled, this suite would stay green.
A real content-persistence assertion would need heavier setup than this suite does anywhere else, so I would not add it here. Flagging it so the coverage is not read as stronger than it is.
PR checklist
What type of PR is this?
Bug fix
What this PR does / why we need it:
profilerun type, which previously emptied the model detail pane.Which issue(s) this PR fixes:
Closes DRC-2625
Special notes for your reviewer:
The one-shot focus synchronization already present on
mainis retained and now explicitly covered. The tests also prove the important boundaries: restoring continuous focus synchronization makes all six model-scoped navigation cases fail, while treating row-count results as node-bound makes both row-count cases fail.Verified locally:
pnpm lintpnpm type:check(root,@datarecce/ui, and@datarecce/storybook)pnpm test(3,992 passed, 5 skipped)pnpm run buildDoes this PR introduce a user-facing change?: