feat(lineage): redesign NodeView sidebar for clarity#1386
Conversation
Tighten the node detail sidebar so a focused model's most-relevant metadata is visible without scrolling. Highlights: - Move the resource-type tag (e.g. "table") inline with the model name in the header row; remove the standalone tags row beneath. - Render row-count inline on the "Row Count" action button as "Row Count: 99,231 rows ↑ +0%" via a new `rowCountDisplay` slot, replacing the previous separate row-count pill. New `RowCountDiffSummary` / `RowCountSummary` components in NodeTag.tsx expose the inline content for reuse by `NodeViewOss`. - Drop the Sandbox button and `SandboxDialog` injection point — the feature is being deprecated and its slot was the only reason for the secondary "Explore" header row. Removes `onSandboxClick`, `runTypeIcons.sandbox`, and the related `trackPreviewChange` call in `NodeViewOss`. - Move "Add schema diff to checklist" out of the header and into the Columns tab, next to the schema legend, via a new `headerAction` slot on `SchemaView` (diff mode only). Storybook story migrates to the new NodeView API. Real `SchemaView` + `MockLineageProvider` from the prior storybook improvements (PR #1385) are reused — this PR is intended to stack on top of #1385. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
Resolves conflicts in two NodeView files where main and this branch touched overlapping areas. Key main changes pulled in: - feat(lineage): reorder NodeView tabs to Columns → Code → Lineage (DRC-3468 — Columns is now the default landing tab) - feat(lineage): enrich node-name tooltip with formatNodeTooltip - feat(lineage): float kebab on hover, keep status icons visible - AUTO-COMMIT version bumps to 1.50.0.dev0 Conflict resolutions: - NodeView.tsx header: kept this branch's compact layout (name + ResourceTypeTag + close, no Explore row), wrapped the name Typography in main's MuiTooltip+formatNodeTooltip so hover still surfaces resourceType and materialized. The kebab/ExploreHeaderButtons block from main was intentionally dropped — this branch removes the Sandbox slot and inlines row-count on the Row Count button via the new rowCountDisplay prop, which made the secondary header row obsolete. - NodeView.stories.tsx: took this branch's structure (createStoryArgs + rowCountDisplay slot) because main's makeStory / mockRunsAggregated plumbing was built around RowCountDiffTag/RowCountTag/SandboxDialog props that the redesign removes. Ported main's MaterializationChanged story (view → table fixture) into this branch's pattern so coverage is preserved. Follow-up filed: DRC-3512 — the MaterializationChanged storybook fixture revealed the sidebar has no visible affordance for materialization changes (changeStatus="modified" shows a dot but Code tab shows identical SQL). Story-side cleanup that fell out of the merge: - Exported RowCountDiffSummary / RowCountSummary from @datarecce/ui/advanced and replaced the storybook's inline RowCountDiffDisplay / RowCountDisplay helpers with the production components. This carries forward the spirit of main's "no story-side reimplementation of tag visuals" refactor (PR #1385) but targets the new inline summaries rather than the removed pill tags. Verified post-merge: - pnpm type:check passes - pnpm lint passes - All 459 lineage tests pass, including main's new DRC-3468 default-tab assertion - Storybook (localhost:6007) renders all 7 NodeView stories, including the ported MaterializationChanged Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
…nced These inline-content variants of RowCountDiffTag / RowCountTag (no pill background, no refresh button) are the production components behind NodeView's new rowCountDisplay slot. Exporting them lets storybook (and any other consumer) embed the real row-count summary visuals instead of reimplementing them. The storybook NodeView fixtures use them in the prior merge commit; this commit closes the gap by exposing the exports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
- Merge RowCountDiffSummary into a single RowCountSummary that
discriminates RowCount vs RowCountDiff by presence of `base`.
- Format inline row counts with `toLocaleString()` + singular/plural,
matching the existing convention in NodeRunsAggregated.
- Remove the deprecated Sandbox feature: delete SandboxView /
SandboxViewOss, drop `sandbox` from RunType / RunRegistry / RUN_TYPES /
isSandboxRun, remove trackPreviewChange{,Feedback} telemetry, clean
stale doc comments in NodeViewOss / useModelColumns / QueryDiffResultView.
- Add NodeTag.test.tsx covering all RowCountSummary branches and extend
NodeView.test.tsx with a Sandbox-absence regression guard and headerAction
gating assertions (diff mode vs single-env).
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Danyel Fisher <danyel@gmail.com>
There was a problem hiding this comment.
Pull request overview
Refactors the lineage NodeView sidebar UI to reduce vertical clutter: moves resource type into the header, renders row-count info inline on the “Row Count” action button, relocates “Add schema diff to checklist” into the Columns tab, and removes the deprecated Sandbox feature (UI + run type + tracking + exports). Updates Storybook and tests to match the new NodeView API.
Changes:
- Redesign
NodeViewheader/actions: inline resource type tag, inline row-count display via newrowCountDisplayslot, and SchemaViewheaderActionslot for the checklist button. - Remove Sandbox support end-to-end (run registry/type, OSS wrapper/components, tracking functions, and exports).
- Update Storybook NodeView stories and add/adjust tests for the new layout and regression guards.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| js/src/components/run/tests/registry.test.ts | Removes sandbox expectations from app-side run registry tests. |
| js/packages/ui/src/lib/api/track.ts | Removes preview-change tracking helpers tied to Sandbox/preview-change UX. |
| js/packages/ui/src/hooks/useModelColumns.tsx | Updates docs/comments to reflect Sandbox removal from cache-sharing consumers. |
| js/packages/ui/src/components/schema/SchemaView.tsx | Adds headerAction slot and aligns legend + action in a header row. |
| js/packages/ui/src/components/run/registry.ts | Removes sandbox registry entry/type from the UI run registry. |
| js/packages/ui/src/components/run/tests/registry.test.ts | Updates registry tests to remove sandbox references. |
| js/packages/ui/src/components/query/QueryDiffResultView.tsx | Updates comment wording away from “sandbox editor” to preview-change runs. |
| js/packages/ui/src/components/query/tests/QueryDiffResultView.test.tsx | Updates test header comment to match preview-change wording. |
| js/packages/ui/src/components/lineage/SandboxViewOss.tsx | Deletes deprecated OSS Sandbox wrapper. |
| js/packages/ui/src/components/lineage/SandboxView.tsx | Deletes deprecated base Sandbox dialog component. |
| js/packages/ui/src/components/lineage/NodeViewOss.tsx | Wires rowCountDisplay using aggregated runs and removes sandbox wiring. |
| js/packages/ui/src/components/lineage/NodeView.tsx | Implements the sidebar layout changes and new slots; removes Sandbox. |
| js/packages/ui/src/components/lineage/NodeTag.tsx | Adds RowCountSummary inline renderer for embedding in buttons. |
| js/packages/ui/src/components/lineage/index.ts | Removes Sandbox exports from the lineage barrel. |
| js/packages/ui/src/components/lineage/tests/NodeView.test.tsx | Adds regression tests for Sandbox removal and schema-diff button placement. |
| js/packages/ui/src/components/lineage/tests/NodeTag.test.tsx | Adds tests for RowCountSummary formatting and diff behavior. |
| js/packages/ui/src/api/types/run.ts | Removes sandbox from RunType unions/constants and deletes type guard. |
| js/packages/ui/src/api/index.ts | Removes re-export of isSandboxRun. |
| js/packages/ui/src/advanced.ts | Exports RowCountSummary and updates inline documentation for row-count slots. |
| js/packages/storybook/stories/lineage/NodeView.stories.tsx | Migrates story args to new NodeView API (rowCountDisplay/headerAction; no sandbox). |
Code Review: PR #1386SHA Adversarial review of the NodeView sidebar redesign + Sandbox surface removal. Type-check, biome lint, vitest (3718 passed / 5 skipped), and Notes
|
gcko
left a comment
gcko
left a comment
There was a problem hiding this comment.
Code Review: PR #1386
SHA fdcc0412 · Verdict GO
Adversarial review of the NodeView sidebar redesign + Sandbox surface removal. Type-check, biome lint, vitest (3718 passed / 5 skipped), and pnpm run build all clean against fdcc0412. Inspected the public @datarecce/ui API removals (isSandboxRun, BaseSandboxView, SandboxViewOss, sandbox registry entry, trackPreviewChange{,Feedback}, "sandbox" run-type variant) and confirmed no remaining source references in this repo or in recce-cloud-infra (only stale built artifacts under js/packages/ui/dist/ and unrelated snaptravel test fixtures). Verified the consolidated RowCountSummary discriminates correctly: single-env mode reads aggregated.row_count.result (no base field) and multi-env mode reads aggregated.row_count_diff.result (with base), so the "base" in rowCount guard routes the right branch; the cast pattern matches the prior RowCountTag / RowCountDiffTag runtime contract. Tests cover all RowCountSummary branches (null/N/A, singular/plural, equal/up/down/both-null/one-null) and add a Sandbox-absence regression guard plus diff-mode vs single-env headerAction placement assertions. The headerAction slot on SchemaView is rendered alongside SchemaLegend in a space-between Stack — diff-only and only when onAddSchemaDiffClick is wired.
Notes
js/packages/ui/src/components/lineage/NodeTag.tsx:76—_RowCountByRatereturns<> Failed to load</>with a leading space; rendered afterRow Count: in the action button this surfaces a visible double space (Row Count: Failed to load).
Evidence:return <> Failed to load</>;paired withRow Count{rowCountDisplay != null && <>: {rowCountDisplay}</>}inNodeView.tsx:310/454.
Pass A.
- Drop the leading space in `<> Failed to load</>` (rendered after the button's `Row Count: ` prefix it caused a visible double space). - Make RowCountSummary tests locale-agnostic by computing expected thousands-separated strings via `.toLocaleString()` instead of hard-coding "99,231" / "1,000" / "1,200". The implementation already uses `toLocaleString()` with the runtime locale, so symmetric expectations remove the ICU/locale flakiness Copilot flagged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jared Scott <jared.scott@datarecce.io>
Resolves NodeView.tsx conflict from main's sidebar redesign (#1386): - Keep HEAD's whole-model props (isWholeModelChanged/Impacted, wholeModelImpact) alongside main's new rowCountDisplay prop. - Keep HEAD's title chip + unified tooltip but adopt main's header flex styling for the non-chip Typography and add the new ResourceTypeTag slot. Drop ExploreHeaderButtons from the header (main moved it to the action-buttons row). - Take main's IIFE-wrapped Tabs/TabPanels block verbatim (including the SchemaView headerAction prop) and drop the now-redundant top-level columnsTabIndex/codeTabIndex/lineageTabIndex const declarations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Danyel Fisher <danyel@gmail.com>
Summary
Tighten the node detail sidebar so a focused model's most-relevant metadata is visible without scrolling.
The Storybook story is migrated to the new NodeView API. It reuses the real `SchemaView` + `MockLineageProvider` infrastructure introduced in #1385.
Type of change
Test plan
User-facing changes
🤖 Generated with Claude Code