Skip to content

fix(lineage): detach run results from focus - #1495

Merged
gcko merged 2 commits into
mainfrom
feature/drc-2625-lineage-view-loses-focus-and-profile-diff-pane-empties
Aug 5, 2026
Merged

fix(lineage): detach run results from focus#1495
gcko merged 2 commits into
mainfrom
feature/drc-2625-lineage-view-loses-focus-and-profile-diff-pane-empties

Conversation

@gcko

@gcko gcko commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

PR checklist

  • Ensure you have added or ran the appropriate tests for your PR.
  • DCO signed

What type of PR is this?

Bug fix

What this PR does / why we need it:

  • Preserves the chosen detached-result behavior: after a model-scoped result opens on model A, clicking model B keeps B focused while the result for A remains open and labelled.
  • Centralizes model-scoped run classification and adds the missing single-environment profile run type, which previously emptied the model detail pane.
  • Adds mounted regression coverage for profile, profile diff, top-k diff, histogram diff, value diff, and value-diff detail results.
  • Preserves independent focus behavior for row-count and row-count-diff results.

Which issue(s) this PR fixes:

Closes DRC-2625

Special notes for your reviewer:

The one-shot focus synchronization already present on main is 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 lint
  • pnpm type:check (root, @datarecce/ui, and @datarecce/storybook)
  • pnpm test (3,992 passed, 5 skipped)
  • pnpm run build
  • Push hook: affected type checks and tests (1,685 passed, 5 skipped)

Does this PR introduce a user-facing change?:

Fixed lineage navigation so opening a model-scoped result no longer traps focus on its original model or empties the model detail pane.

Signed-off-by: Jared Scott <jared.scott@datarecce.io>
@gcko
gcko requested a review from iamcxa August 5, 2026 08:37
@gcko gcko self-assigned this Aug 5, 2026
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 iamcxa 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.

@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:

  1. query_base is missing from the non-node-bound list at LineageViewOss.tsx:1179, while RunResultPaneOss.tsx:167 and useCSVExport.ts:176/250/292 all treat ["query", "query_base", "query_diff"] as one group. Since run comes from the app-wide context and MainLayout.tsx:167-175 keeps the lineage view always mounted, running a base query clears the focused node.

  2. The classification lives in three hand-maintained lists (runResultVisibility.ts:8-16, :31-38, and LineageViewOss.tsx:1179) with default: return false at the end. A run type added to the Run union but forgotten here is silently misclassified instead of a compile error — which is exactly how this bug happened, profile having 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",

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.

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",

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.

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.

@gcko
gcko merged commit 7035813 into main Aug 5, 2026
8 checks passed
@gcko
gcko deleted the feature/drc-2625-lineage-view-loses-focus-and-profile-diff-pane-empties branch August 5, 2026 10:32
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.

2 participants