fix(data-modeling): speed up large DAG layout, fix table pagination - #73408
Conversation
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
|
Note 🤖 stamphog reviewed The PR description claims a bounded-height flex wrapper + allowContentScroll fix for pagination visibility, but the diff only deletes the old overflow className with no replacement — the reviewer's concern that pagination stays hidden is unaddressed despite being marked resolved, and removing the height/overflow constraint entirely could also make the table grow unbounded on large lists.
Gate mechanics and policy version
Updated in place — this replaces 1 earlier stamphog review(s) on this PR. |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
frontend/src/scenes/data-warehouse/scene/modeling/TableView.tsx:52
**Pagination remains inside scrolling content**
When a page contains enough rows to exceed the bounded height, `allowContentScroll` scrolls the entire `LemonTable__content`, including the pagination control rendered after the table, causing users to still scroll past every row to reach the page controls.
Reviews (1): Last reviewed commit: "fix(data-modeling): speed up large DAG l..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
🤖 CI report
|
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.24 MiB · 22 files | no change | ███░░░░░░░ 27.5% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.07 MiB · 3,010 files | 🔺 +31 B (+0.0%) | ████████░░ 83.1% of 9.71 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/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers 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/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 281.5 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/rrweb.js |
| 267.7 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 |
| 236.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 226.1 KiB | ../node_modules/.pnpm/posthog-js@1.407.2/node_modules/posthog-js/dist/module.js |
| 154.3 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 |
| 105.8 KiB | src/lib/api.ts |
| 94.0 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 |
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.18 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.18 MiB · 17 files | no change | ████░░░░░░ 38.1% of 5.72 MiB |
| Deferred (lazy) | 2.07 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 713.8 KiB | dist/toolbar/toolbar-app-HLTHZTTF.css |
| 545.0 KiB | dist/toolbar/chunk-chunk-6OSYGHA4.js |
| 484.2 KiB | dist/toolbar/chunk-chunk-FFAAFRZ5.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-LYC3RDNL.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-THUTYU3T.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-JD7XW4XC.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-3VBOQT7S.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +2.4 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1352.26 MiB · 🔺 +2.4 KiB (+0.0%)
7ec5c76 to
0cf561f
Compare
0cf561f to
dc1e279
Compare
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, low-risk frontend perf/layout tweak; the only reviewer concern (pagination hidden in scroll box) was addressed by removing the scroll container entirely, and greptile reacted 👍 to the fix.
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 17L, 3F substantive — within ceiling |
| tier | ✓ | T1-agent / T1a-trivial (17L, 3F, single-area, fix) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ c43608d · reviewed head dc1e279 |
1 participant
Problem
On the Data ops → Modeling tab, teams with large DAGs (hundreds of nodes) hit two problems:
Changes
Graph layout speed. The graph is laid out with ELK's
layeredalgorithm on the main thread. The dominant cost is the node-placement pass, which was set toNETWORK_SIMPLEX— it produces the tightest layout but scales super-linearly, so on a large graph it's several seconds of frozen main thread. I benchmarked the alternatives on a synthetic 800-node / ~950-edge DAG (ELK 0.10.0, the version we ship):NETWORK_SIMPLEX(before)BRANDES_KOEPF(after, large graphs)That's ~5x faster, and it isolates to the placement strategy alone — the layering strategy stays
NETWORK_SIMPLEX, so the tier structure (vertical layers) is identical.BRANDES_KOEPFjust spreads nodes a bit wider within each tier, which is fine on a pan/zoom canvas.elk.layered.nodePlacement.bk.fixedAlignment: BALANCEDwas already configured for exactly this placement.To avoid changing anything for the common small-DAG case, the switch is gated on node count (
LARGE_GRAPH_NODE_THRESHOLD = 150). Below that, both strategies finish in well under ~120 ms, so small graphs keep the tighterNETWORK_SIMPLEXplacement.Table pagination.
TableViewwrapped the wholeLemonTableinh-[calc(100vh-17rem)] overflow-y-auto.LemonTablerenders its pagination footer after the rows, so inside that fixed-height scroll box the footer sat below the rows and out of view — which is why it looked like there was no pagination. The fix is to drop the scroll box and let the table paginate natively like every other list in the app, and drop the page size from 100 to 40 so the footer is reachable with a short scroll. Net diff here is one deleted line plus the page-size constant.Note
Large graphs (>150 nodes) now use
BRANDES_KOEPFnode placement instead ofNETWORK_SIMPLEX. Same layer structure, slightly wider horizontal spread. Small graphs are unchanged.How did you test this code?
NETWORK_SIMPLEXis the super-linear term andBRANDES_KOEPFscales far better, while the two are indistinguishable in cost below the threshold.oxlintandoxfmtclean on the changed files.autolayout/TableViewto extend. The graph fix is a pure ELK option change, so I leaned on the benchmark + local render rather than a snapshot test that wouldn't catch a realistic regression.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Jovan pointed at the Modeling tab being slow on an ~800-node DAG and asked to make it work on huge DAGs. I (Claude) mapped the render path, traced the graph slowness to the ELK
nodePlacement.strategyoption (not the whole engine, and not the layering strategy — swapping layering actually made it worse), and benchmarked the fix before changing code. For the table, the "no pagination" report was a footer hidden inside a fixed-height scroll box; after trying anallowContentScrollvariant (correctly flagged in review for keeping the footer inside the scroll area) I settled on the boring fix — remove the scroll box, paginate natively, shrink the page size. Rejected moving ELK to a web worker: it keeps the tab responsive but needs a new bundler entrypoint +/staticasset (the fragile pattern that already bites Monaco in dev), and the option change already gives a 5x win with zero build changes.