feat(notebooks): size sql/python cells to the output they return - #73358
Conversation
Growing every result to one tall default meant a cell returning a single number got as much room as a hundred-row table. Size the cell to what came back instead: rows, printed lines, and figures each contribute, clamped between a compact floor and a cap so a large result stays readable without swallowing the notebook. Still grows only, and now re-sizes on each new run rather than only the first one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🤖 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 | no change | ████████░░ 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-JGF7ZYC3.js |
| 484.2 KiB | dist/toolbar/chunk-chunk-YMFODIKP.js |
| 133.6 KiB | dist/toolbar/chunk-chunk-YSVTUFEA.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.0 KiB | dist/toolbar/toolbar-app-Y3ZKEAFE.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-ALQOU6WO.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-VMBXRLPQ.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 — 🔺 +8.1 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1352.23 MiB · 🔺 +8.1 KiB (+0.0%)
|
👋 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. |
🦔 Hogbox preview · ✅ ready▶ Open the preview
commit |
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/scenes/notebooks/Nodes/notebookNodeOutputHeight.ts:47-49
**Empty dataframes bypass sizing**
When an execution returns columns but no rows, the dataframe table still renders its header, controls, and surrounding chrome, but this branch returns `null`. The node therefore retains its short existing height and the empty-result table remains clipped.
### Issue 2 of 2
frontend/src/scenes/notebooks/Nodes/NotebookNodeSQLV2.tsx:180-182
**Reruns override manual heights**
When a user manually sizes a V2 cell below the calculated target and reruns it, the new run ID makes this effect grow the cell again. Because V2 resize handlers persist no marker distinguishing manual and automatic heights, the completed run overwrites the user's selected size.
Reviews (1): Last reviewed commit: "Merge branch 'master' into feat/revamped..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pure frontend UI sizing logic for notebook cells, well outside risky territory, with a dedicated pure-function test suite; the two bot-raised P1 concerns were discussed and explicitly accepted as reasonable trade-offs by author and reviewer, not ignored.
- Author wrote 100% of the modified lines and has 12 merged PRs in these paths (familiarity STRONG).
- 👍 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 | ✓ | 119L, 3F substantive, 153L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (153L, 4F, single-area, feat) |
| stamphog 2.0.0b3 | .stamphog/policy.yml @ ec2002f · reviewed head 28a17ae |
1 participant
Problem
Follow-up to #72512. That PR made V2 cells grow once results land, which fixed the clipping, but it grew every cell to the same 300px. A query that returns a single number now gets as much vertical room as a hundred-row table, and the notebook turns into a lot of empty boxes.
Changes
Size the cell to the output it actually got back instead of one fixed default. A new pure helper,
outputHeightForShape, adds up what's there - rows in the returned page, printed stdout/stderr lines, and whether there's a figure - and clamps the total between a compact floor (160px) and a cap (460px). So a scalar lands around 176px, five rows around 290px, and anything past ~13 rows stops at the cap and scrolls.Two smaller behavior changes come with it:
VIZ_MIN_HEIGHT(350), since charts need room regardless of row count.Still grow-only: a cell never shrinks, so a deliberate manual resize and a reload of an already-sized cell are both left alone.
Note
Existing notebooks keep whatever height they were saved with. The new sizing only applies from the next run onward.
How did you test this code?
Added
notebookNodeOutputHeight.test.ts(6 cases) against the pure helper. The regression it catches is the one this PR exists for: a one-row result must come out shorter than a five-row result, and shorter than the old flat default - plus the clamps hold, so a 1000-line traceback or a 10k-row page can't produce an absurd height. No existing test covered sizing at all. I kept it at the pure-function rung rather than rendering the node, since all the arithmetic lives in the helper.Ran locally: the new file plus the full
src/scenes/notebooksJest suite (all green),hogli ci:preflight(clean), oxlint + oxfmt on the touched files. Typecheck ran but the worktree only had a partial install, so unrelated workspace packages reported missing modules; nothing was reported inscenes/notebooks.I (Claude) did not run this in a browser. The pixel constants are reasoned from the rendered chrome (table header, pagination bar, dataframe-name footer) rather than measured, so they're worth an eyeball in dev before this goes out.
Automatic notifications
Docs update
No user-facing docs cover cell sizing.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
I directed this; Claude Code (Opus 4.8) wrote it in a separate worktree. Skills invoked:
/writing-tests.The first idea was to keep the single threshold and just add a second, larger one past 5 rows. We went with a continuous function instead - a step still gets the scalar case wrong unless you special-case it, and once you're special-casing you may as well compute from the shape. Rejected shrinking the cell when a re-run returns less: it would fight manual resizes, and grow-only matches what the previous PR established.
The switch from "first output" to "per run id" wasn't in the original ask, but row-count sizing is useless without it - a cell that already had a result would never re-size no matter how much data the next run returned.