Skip to content

feat(data-modeling): add lineage graph loading skeleton - #108509

Open
sakce wants to merge 6 commits into
masterfrom
posthog/better-lineage-loading-state
Open

sakce wants to merge 6 commits into
masterfrom
posthog/better-lineage-loading-state

Conversation

@sakce

@sakce sakce commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Problem

People see a small spinner in a large canvas while a lineage graph loads its data or calculates its layout.

Changes

  • The Models graph now shows two neutral skeleton nodes when no model is in focus.
  • Focused lineage views show upstream, current, and downstream nodes.
  • The loading graph uses the normal ELK layout and respects the graph direction.
  • Loading nodes pulse softly and stop the animation when reduced motion is enabled.
  • Focused views keep the known node name and type. Anonymous nodes contain only skeleton bars.
  • Node detail, metric lineage, and SQL editor lineage now use the shared loading graph.
  • Narrow metric panels keep a table-shaped skeleton because their resolved state shows a table.

Before

Centered spinner

After

Models graph

Anonymous lineage loading graph

Focused graph

Focused lineage loading graph

Dark mode

Dark lineage loading graph

How did you test this code?

  • Ran the full frontend TypeScript check.
  • Ran Oxlint and Oxfmt on all changed files.
  • Rendered the new Storybook states in headless Chromium at wide and narrow widths.
  • Verified two anonymous nodes, three focused nodes, edge counts, dark mode, focused labels, and no browser errors.
  • Verified the pulse animation stops when reduced motion is enabled.
  • Added visual stories for anonymous, focused, and dark loading states. These catch a return to the spinner-only state.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

None. This change only affects the loading presentation.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: PostHog Desktop, gpt-5.6-sol

  • The open PR search found no other PR for the lineage loading state.
  • The implementation used /writing-ui-components, /writing-tests, /writing-user-facing-copy, /writing-code-comments, and /writing-pr-descriptions.
  • The loading state uses the real graph layout instead of a separate static illustration.
  • All Storybook and screenshot data comes from existing public fixtures.

Created with PostHog Desktop

Replace the centered lineage spinner with a three-node ELK graph. Keep known node names visible in focused lineage views.

Generated-By: PostHog Desktop
Task-Id: d30b63b9-ca68-4fde-bebe-e6cdbd9740a4
@sakce sakce self-assigned this Sep 29, 2026
@trunk-io

trunk-io Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

✨ Submitted to Merge by Jovan Sakovic (@sakce). It will be added to the merge queue once all branch protection rules pass. See more details here.

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

⚠️ Complexity (TypeScript) — 4 functions above the limit (max 42)

Cyclomatic complexity above the limit in changed typescript files (10 for production files, 15 for test files). Warn only: worth simplifying when you next touch these functions.

Function Location Complexity Limit
LineageNode products/data_modeling/frontend/lineage/LineageNode.tsx:194 42 10
QueryInfo frontend/src/scenes/data-warehouse/editor/output-pane-tabs/QueryInfo.tsx:38 40 10
LineageGraphContent products/data_modeling/frontend/lineage/LineageGraph.tsx:59 17 10
MetricLineagePanel products/data_catalog/frontend/MetricLineagePanel.tsx:74 12 10
✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Duplication (TypeScript) — 1 new duplicated block (worst 74 tokens)

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

First copy Second copy Lines Tokens
products/data_modeling/frontend/lineage/LineageNode.tsx:224 products/data_modeling/frontend/lineage/LineageNode.tsx:305 12 74
⚠️ Bundle size — 🔺 +9.1 KiB (+0.0%)

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 68.97 MiB · 🔺 +9.1 KiB (+0.0%)

File Size Δ vs base
exporter/src/exporter/scenes/ExporterNotebookScene.js 3.69 MiB 🔺 +3.4 KiB (+0.1%)
render-query/src/render-query/render-query.js 20.18 MiB 🔺 +2.4 KiB (+0.0%)
posthog-app/_parent/products/signals/frontend/inbox/InboxScene.js 491.0 KiB 🔺 +1.8 KiB (+0.4%)

Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report

✅ Eager graph — within budget

How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.

Root Eager (shipped) Δ vs base Budget
entry (logged-out pages, app bootstrap)
src/index.tsx
1.58 MiB · 22 files 🔺 +590 B (+0.0%) █████████░ 85.8% of 1.84 MiB
logged-out boot: index + App + bootApp (preloaded by every page, including /login)
src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
3.52 MiB · 629 files 🔺 +619 B (+0.0%) █████████░ 87.4% of 4.03 MiB
authenticated shell (every logged-in page)
src/scenes/AuthenticatedShell.tsx
7.35 MiB · 2,339 files 🔺 +1.8 KiB (+0.0%) █████████░ 88.1% of 8.34 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx
Size File
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
24.6 KiB ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js
6.3 KiB ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js
4.5 KiB ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js
3.9 KiB ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js
1.4 KiB ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js
1.3 KiB src/index.tsx
1.3 KiB src/RootErrorBoundary.tsx
912 B ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
854 B src/scenes/ChunkLoadErrorBoundary.tsx
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
Size File
301.8 KiB ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
216.9 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
100.5 KiB src/lib/api.ts
88.5 KiB src/products.tsx
69.4 KiB src/lib/lemon-ui/icons/icons.tsx
40.1 KiB src/lib/utils/eventUsageLogic.ts
38.7 KiB ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js
33.9 KiB ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js
28.4 KiB src/scenes/scenes.ts
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
Size File
301.8 KiB ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
272.3 KiB src/taxonomy/core-filter-definitions-by-group.json
216.9 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
153.7 KiB ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
100.5 KiB src/lib/api.ts
98.8 KiB ../packages/quill/packages/quill/dist/index.js
93.3 KiB ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js
90.6 KiB ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js
88.5 KiB src/products.tsx

Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479

✅ Toolbar bundle — eager 2.16 MiB within budget

What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.

Metric Size Δ vs base Budget
Eager (shipped)
entry + static imports
2.16 MiB · 19 files 🔺 +590 B (+0.0%) ████░░░░░░ 37.8% of 5.72 MiB
Deferred (lazy) 2.10 MiB · 44 files no change n/a — loads on demand
Loader dist/toolbar.js 1.2 KiB no change █░░░░░░░░░ 6.0% of 19.5 KiB
Largest eagerly-shipped chunks
Size File
805.9 KiB dist/toolbar/toolbar-app-5STLTNPX.css
651.7 KiB dist/toolbar/chunk-chunk-7ZORLPSY.js
259.4 KiB dist/toolbar/chunk-chunk-2FGAEFTI.js
138.3 KiB dist/toolbar/chunk-chunk-UA3V2PCC.js
131.8 KiB dist/toolbar/chunk-chunk-FDH2IBXT.js
75.2 KiB dist/toolbar/toolbar-app-SD4VQXWA.js
69.0 KiB dist/toolbar/chunk-chunk-TSAL54PB.js
35.6 KiB dist/toolbar/chunk-chunk-63EWNJRZ.js
21.0 KiB dist/toolbar/chunk-chunk-F5GDSM5D.js
6.8 KiB dist/toolbar/chunk-chunk-DV7IWQNF.js

Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile

✅ Dist folder size — 🔺 +190.1 KiB (+0.0%)

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 947.85 MiB · 🔺 +190.1 KiB (+0.0%)

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Adds loading skeleton UI to lineage graph displays.

The PR appears safe to merge; no new actionable issue was identified.

Reviews (2) · Last reviewed commit: "chore(visual): update storybook baseline..."

Comment thread products/data_modeling/frontend/lineage/LineageGraphLoading.tsx Outdated
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Lineage loading views now display graphs with placeholder nodes instead of standalone spinners or skeletons in the updated cases. The graph can identify a focused node, applies layout and viewport settings, and exposes a screen-reader loading status. The changes also add loading-state stories and update the initial graph layout helper.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 79a0e

While lineage loads, a focused view may briefly show its previous center, and a removed animated placeholder may remain retained until its node unmounts. These are localized presentation and resource concerns, so the change is mergeable with follow-up awareness.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 79a0e

The change is primarily a noninteractive loading display. No authorization bypass or new access to data was identified, but a previously focused name can briefly remain visible when the loading focus changes.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The newly observed loading behavior is in frontend lineage views; the loading node does not expose the normal node-action callbacks.

Trust Boundaries and Controls

  • observed — The loading graph hides its handles, marks them non-connectable, passes empty callbacks, and disables selection, dragging, connecting, and user-initiated navigation.

Hardening Proposals

  • proposed — Consider invalidating the retained layout when focused metadata changes, so an old name is not displayed during an asset or access-context transition.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the problem, user-visible changes, screenshots, testing, release status, docs impact, and agent context. It omits the CodeRabbit CLI pass or skip reason and a session link, but …
✨ Finishing Touches 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (2)
products/data_modeling/frontend/lineage/LineageGraphLoading.tsx-84-84 (1)

84-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Discard a layout when the loading center changes.

If center changes while this component stays mounted, lineageGraphLogic retains the prior layout because its key uses unchanged node IDs. Line 84 displays the prior center name and type until the new ELK pass completes. Use the initial layout until the retained layout matches the current graph, or include the center identity in the logic key.

products/data_modeling/frontend/lineage/LineageGraph.tsx-158-158 (1)

158-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve the known center in the explicit loading branch.

If a caller supplies currentNodeId and its node but omits loadingCenter, this branch shows “Loading lineage...” instead of the known name and type. The layout-pending branch already resolves that node. Apply the same fallback here so both loading paths preserve the focused label.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: a5e1a89e-7c17-4e67-ad92-a4dd996a9806

📥 Commits

Reviewing files that changed from the base of the PR and between bbad9e7 and 5b7494e.

📒 Files selected for processing (8)
  • frontend/src/scenes/data-warehouse/editor/output-pane-tabs/QueryInfo.tsx
  • products/data_catalog/frontend/MetricLineagePanel.tsx
  • products/data_modeling/frontend/lineage/LineageGraph.stories.tsx
  • products/data_modeling/frontend/lineage/LineageGraph.tsx
  • products/data_modeling/frontend/lineage/LineageGraphLoading.tsx
  • products/data_modeling/frontend/lineage/LineageNode.tsx
  • products/data_modeling/frontend/lineage/lineageGraphLogic.ts
  • products/data_modeling/frontend/nodeDetail/NodeDetailLineage.tsx

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Use two neutral placeholder nodes when the graph has no focused model. Add a reduced-motion-safe pulse to all loading nodes.

Generated-By: PostHog Desktop
Task-Id: d30b63b9-ca68-4fde-bebe-e6cdbd9740a4
@trunk-io

trunk-io Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

The fallback layout was rebuilt on every render while ELK was pending, so its new identity refired the fit-view effect on unrelated updates.

Generated-By: PostHog Desktop
Task-Id: 696f17c3-2231-48d1-b752-e9947eec024b

@coderabbitai coderabbitai Bot 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.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/data_modeling/frontend/lineage/LineageGraphLoading.tsx-91-95 (1)

91-95: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick win

Complete the required Semgrep scan. The repository-wide command timed out after 120 seconds, so it did not produce a complete result. Rerun it to completion and address any findings before merge.

Source: Coding guidelines


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: e5a77708-df03-4dfa-85d3-5b0c94f9fac4

📥 Commits

Reviewing files that changed from the base of the PR and between 32d3918 and c22d972.

📒 Files selected for processing (1)
  • products/data_modeling/frontend/lineage/LineageGraphLoading.tsx

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.

The three loading stories render skeleton nodes that never resolve, so the
Storybook test runner's postVisit loader wait timed out on every run. Opt
those stories out and wait for the rendered graph nodes instead.

Generated-By: PostHog Desktop
Task-Id: 696f17c3-2231-48d1-b752-e9947eec024b

sakce commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Working through the three grouped CodeRabbit notes, since none of them opened a thread:

LineageGraphLoading.tsx:84 — discard the layout when the loading center changes. The mechanism is right: the logic key is variant-direction-nodeIds, and idPrefix comes from useId(), so a center change keeps the same key and the retained layout carries the previous name until the next ELK pass lands. But it only bites on a loading → loading transition (changing focus while the first load is still in flight); a loaded → loading transition remounts the component, so the layout starts null and initialLayout supplies the new name. The window is one ELK pass on a two- or three-node graph, inside a placeholder. Leaving it, rather than re-keying the logic in a branch whose point is the loading presentation — happy to take it as a follow-up if you'd rather not carry it.

LineageGraph.tsx:158 — preserve the known center in the explicit loading branch. Checked every call site: QueryInfo.tsx:161, NodeDetailLineage.tsx:53 and MetricLineagePanel.tsx:104 all pass loadingCenter, and ModelsLineageTab.tsx:80 is the deliberately anonymous one with no currentNodeId. No caller passes loading together with currentNodeId and no loadingCenter, so the fallback would be dead code today. Not applying it.

Semgrep scan. Ran .semgrep/rules/security/ over the lineage directory and the other four changed files: 9 rules, 24 targets, 0 findings. Nothing to address.

Separately, the three red checks were mine, not flakes. Loading, LoadingFocused and LoadingDarkMode render skeleton nodes that never resolve, so the Storybook test runner's postVisit loader wait timed out on all 3 of 3 flake-verification runs. Fixed in c209056 by opting those stories out of waitForLoadersToDisappear and waiting for .react-flow__node instead, matching the existing LemonTable loading-story pattern.

🦉 via talyn.dev

@coderabbitai coderabbitai Bot 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.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/data_modeling/frontend/lineage/LineageGraph.stories.tsx-88-88 (1)

88-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Wait for ELK layout completion before capture.

The runner waits for .react-flow__node, page readiness, stable document dimensions, and a fixed one-second delay. It does not observe React Flow node positions or the layoutComputed action. If ELK takes longer than those generic waits, the snapshot may still contain initialLayout. Expose a DOM signal for layout !== null and wait for that signal.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 3be6b878-9589-4cea-87f5-64a4b4701e1e

📥 Commits

Reviewing files that changed from the base of the PR and between c22d972 and c209056.

📒 Files selected for processing (1)
  • products/data_modeling/frontend/lineage/LineageGraph.stories.tsx

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.

sakce commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Everything I can fix from here is fixed. One gate is left and it needs a person.

Updated for head 79a0ef6: the earlier run was superseded. The actionable set and the diff percentages are byte-for-byte the same, since the animation-cancel fix only runs on unmount.

Where CI stands on 79a0ef6: 102 checks pass. Storybook flake verification is green (3/3 runs) and all four Visual regression tests - chromium shards pass. The three still-red checks are one gate and its two dependents: PostHog Visual Review / storybook (partial) is waiting on baseline review, Complete Visual Review run exits 1 because of it, and Visual regression tests pass fails only because it reads that result.

What needs approving — run a1f96514, 1476 snapshots, all 8 actionable ones from this PR:

  • 6 new — lineage-graph--loading, --loading-focused, --loading-dark-mode, each light and dark.
  • 2 changed — metric-lineage--loading light and dark. The dark diff reads 23.6% against 0.31% for light, which looks alarming but is only luminance: both baselines are the old plain skeleton block and both current images are the new loading graph. The dark block sat far from the dark canvas colour, the light one did not.

I compared each image against the "After" screenshots in the description and they match, so the delta is this PR's intended change. I am not approving my own baselines.

Worth a look before you do. In every focused capture the three nodes sit right of centre instead of fitted, and in the 975px metric panel the "Loading downstream..." node is clipped at the container edge. That is not a capture artifact: the screenshot in the PR description shows the same offset and the same clipping, so fitView is not centring the loading graph. Deciding whether to approve that or fix the fit first is yours.

🦉 via talyn.dev

sakce commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Visual review is the only thing left on this PR

Everything else on c209056 is green, and the one review thread (greptile's P2 on the loading layout) is addressed in c22d972 and resolved. The three red checks are all the same gate: VR run 275b9be5 has 8 unresolved snapshots waiting on a human.

All 8 are this PR's own surfaces, nothing unrelated leaked in:

Result Snapshot
changed products-data-catalog-metric-lineage--loading (light 0.31%, dark 23.64%)
new products-data-modeling-lineage-graph--loading (light + dark)
new products-data-modeling-lineage-graph--loading-focused (light + dark)
new products-data-modeling-lineage-graph--loading-dark-mode (light + dark)

I pulled the PNGs and looked at them. The metric lineage pair is exactly the intended swap: the flat skeleton block becomes the three-node loading graph with weekly_active_accounts in the middle. snapshot-history-list shows that baseline has been stable on master since 22 Sep, so the diff is real and from here.

One thing worth your eye before you approve, because approving bakes it into the baseline: in every captured frame the loading graph is not fitted to its container. It sits up and to the right of center, and in the metric panel's ~960px container the downstream node is clipped by the right edge. That looks like the screenshot landing before the fitView effect settles rather than the real runtime appearance, since waitForSelector: '.react-flow__node' resolves as soon as the first node mounts. Related signal: the sibling products-data-modeling-lineage-graph--search-focus story is already classified noisy with 32 accepted variants, so framing-sensitive graph screenshots have a history of not settling here.

So the call is yours on two things:

  1. Whether the un-fitted framing is acceptable to baseline, or the fit should land before the capture.
  2. The actual approve/finalize. I am not going to finalize a visual baseline on your branch without you saying so.

Also minor and entirely your call: LoadingDarkMode sets globals: { theme: 'dark' } but the harness still sweeps it in both themes, so it produces a --light snapshot too and duplicates what LoadingFocused--dark already covers. Dropping it would cost two baselines and no coverage.

Once you approve the run (or tell me to), everything else is ready.

🦉 via talyn.dev

The loading node animates with an infinite pulse and always unmounts
mid-animation, which pins the detached graph subtree through Chromium's
DocumentTimeline. Attach useCancelAnimationsOnUnmount, as Spinner,
LemonSkeleton and LemonTableLoader already do.

Generated-By: PostHog Desktop
Task-Id: 696f17c3-2231-48d1-b752-e9947eec024b

sakce commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Applied in 79a0ef6. The report holds up: LemonSkeletonItem, Spinner, LemonTableLoader, WrappingLoadingSkeleton and App all attach useCancelAnimationsOnUnmount, and the skeleton bars inside the loading node inherit that protection, but the animate-pulse on the node wrapper itself did not. That wrapper is guaranteed to unmount mid-animation — the loading graph is torn down the moment ELK returns a layout or loading flips false — so it hit exactly the timeline-pins-detached-subtree case the hook's docstring describes.

Called the hook unconditionally above the state.loading early return so hook order stays stable, and put the ref on the loading wrapper.

Left the state.isRunning && 'animate-pulse' on the resolved node alone: it predates this branch and belongs in its own change rather than widening this diff.

🦉 via talyn.dev

@coderabbitai coderabbitai Bot 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.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/data_modeling/frontend/lineage/LineageNode.tsx-197-197 (1)

197-197: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Tie animation cleanup to the loading-card lifecycle.

LineageNode remains mounted while React Flow updates data.state. The hook captures ref.current only when LineageNode mounts, but the ref exists only in the loading branch. A resolved-to-loading transition can therefore capture null, and a loading-to-resolved transition removes the animated card without running the hook cleanup. The detached card can remain retained by its running animate-pulse animation.

Move the loading branch into a child component that owns useCancelAnimationsOnUnmount, or otherwise run cleanup when the loading card unmounts.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: b82bc5da-898b-4dea-96b8-a62e0cdbaf45

📥 Commits

Reviewing files that changed from the base of the PR and between c209056 and 79a0ef6.

📒 Files selected for processing (1)
  • products/data_modeling/frontend/lineage/LineageNode.tsx

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.

@posthog

posthog Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

✅ Visual changes approved by @sakce — baseline updated in 6d162ed.

View this run in PostHog

2 changed, 6 new.

Install the Visual Review Chrome extension to see visual review results at the top of your pull requests.

8 updated
Run: a1f96514-456e-4e75-8dda-30fc1909193d

Co-authored-by: sakce <49978945+sakce@users.noreply.github.com>
@sakce
sakce marked this pull request as ready for review September 29, 2026 18:40
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

🦔 Hogbox preview · ❌ build failed

The preview didn't come up for commit 6d162ed. See the build log for the failing step. It'll retry on the next push.

Previews are optional and never block merging. A failure here is often a hogland or tailnet hiccup rather than anything in your PR, so the check stays green and this comment is the status.

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested review from a team September 29, 2026 18:41

This branch had an error being deployed

1 failed deployment
preview-pr-108509 — 6d162edd Deployed Sep 29, 2026 by github-actions[bot]
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.

1 participant