refactor(studio): decompose the ten oversized panels - #237
Merged
Conversation
MailPanel was 306 lines of component, 290 of them logic. The read + poll, the clear/send-test actions, and the selection/filter/tab state move to `useMailCapture` — the same split `useDataBrowser` and `useFileBrowser` already use — and the pure selection helpers to `mail-selection.ts`, where they are testable without a renderer. The panel is now markup and copy: 274 lines, and the no-giant-component suppression is deleted rather than annotated. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SchemaEditorOverlay was 323 lines, 207 of them markup. Two pieces come out: - `SchemaEditorModeBar` — a closed set of four choices whose only state is which is active. Inline they were four near-identical twenty-line blocks differing only in label, testid, and handler. - `SchemaEditorResult` — the one part that renders the OUTCOME rather than collecting input; the forms above it all read and write draft state, this reads only `result`. Suppression deleted. 43 schema tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MetricsPanel was 356 lines. Two surfaces come out, plus the pieces three files now share: - `MetricsOverviewStats` — the single-shard readout, pure presentation of one snapshot. - `MetricsAggregateView` — the cross-shard rollup: a different question, a different data shape, and only present after a fan-out. - `stat-card.tsx` and `metrics-format.ts` — `StatCard` plus the duration and hit-rate formatters, so the three surfaces cannot drift on how a value reads. Suppression deleted. 79 reports tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ormatting HealthPanel was 346 lines, now 282. - `HealthDigest` — the two-column "what is going wrong right now" grid (functions by error rate, most recent errors). Both cards render data the panel already fetched and read none of its fan-out, poll, or shard state. - `slo-format.ts` — the level type, its badge tones, the warn/crit thresholds, and the rate renderer, so the panel and the digest cannot disagree on what a breach looks like. Suppression deleted. 79 reports tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GlobalDataBrowser was 362 lines, now 281. - `GlobalDataPage` — everything that is a function of ONE loaded page: the drill-down chips, the grid, the pager. The browser keeps discovery, the poll, and the facet state. - `GlobalTablesEmptyState` — the no-global-tables case, mostly to get eighteen lines of inline SVG out from between two conditionals. - `global-row-format.ts` — row identity and chip values, shared by both and testable without a renderer. `removeFilter` now takes an index instead of a MouseEvent it read `data-index` back out of — the child already has the index, so round-tripping it through a DOM attribute was the parent reaching into the child's markup. Suppression deleted. 171 data tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LogsPanel was 413 lines, now 291. Four pieces, each a closed set of controls or one reading of the window: - `LogsViewBar` — the only chrome common to all three views (view switch, shard, live error); the filter bars below it swap per view, this does not. - `LogsErrorFilters` / `LogsRequestFilters` — per-view controls that previously sat adjacent guarded by opposite conditions. `LevelToggle` moved in with the filters that render it. - `LogsSummary` — the aggregate reading, which is the ALTERNATIVE to the entry list rather than part of it. Its `LogSummary`/`SummaryBucket` types moved with it. Suppression deleted. 80 logs tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OrganizationDetail was 430 lines, now 203. 312 of those lines were markup: four sections of near-identical shape (a SectionCard, a button that opens a dialog, a table) plus the dialogs. - `organization-sections.tsx` — Members, Teams, TeamMembers, Roles. One module because they share that shape and props vocabulary; four files would have meant four identical import headers for no gain. - `organization-primitives.tsx` — `SectionCard`, `ManagedTable`, `Column`, and `DialogState`: the shell and vocabulary the page owns as a whole. The sections no longer touch the client. Each states intent — `onRemoveMember`, `onRemoveTeamMember`, `onConfirmDeleteTeam`, `onConfirmDeleteRole`, `onDialog` — and the page keeps the call, the busy flag, and the error handling in one place. The conditional sections take their guard as an early return rather than returning a JSX expression. Suppression deleted. 34 auth tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SchemaViewer was 489 lines — 471 of logic behind 18 of markup — now 231. `useSchemaExplorer` owns both schema planes: the shard's tables with their lazily probed columns and indexes, the `.global()` (D1) tables with theirs, the graph's column probe, and the view / shard / filter selection. The two planes live in one hook on purpose: the graph probe needs both table lists, so separating them would only mean threading one into the other. Its return type is named rather than inferred, so the hook's surface is a readable contract instead of twenty-odd inferred fields. Suppression deleted. 930 studio tests green. Refs #230. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DataBrowser was the ninth of ten components over react-doctor's 300-line limit. Two extractions, no behaviour change: - `useDataViewPreferences` — pins, masking, transpose, and inspection state, which were four independent `useState` clusters interleaved with the fetch logic. - `DataBrowserPage` — everything rendered once a page has loaded. It takes the two MODELS rather than their fields; the naive extraction wanted ~55 loose props, which moves the wiring without reducing it. The toolbar's `CONTROL_BTN` moves with it. Note that `grid-features` has a near-identical copy WITHOUT the `aria-pressed:*` variants — importing that one instead would have silently dropped the pressed styling from the view and mask toggles, so the fuller class travels with the toolbar that needs it. Drops the `no-giant-component` suppression; react-doctor reports nothing for this file. 171 data tests unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SqlEditorPanel was the last component over react-doctor's 300-line limit, at 714. It is now 193, split along the seams that were already there — three hooks and four components, no behaviour change: - `useSqlEditorTabs` — which tabs are open, which is active, each one's ephemeral output, and the right-click menu's bulk closes with their unsaved-work confirm. - `useSqlLibrary` — the persisted PRIVATE query list, the run history, and the sidebar's search. Reaches the editor through two callbacks, so it never needs the tab model. - `useSqlEditorSurface` — completion, caret tracking, the keyboard map, gutter/overlay scroll sync, and diagnostic reveal: everything about the one textarea and where its caret is. - `SqlTabStrip`, `SqlEditorPane`, `SqlResultsPane`, and `SqlResultTable`. The strip and the two panes take their models whole rather than 18-plus loose props each: every field is rendered in exactly one of them, so a flattened signature would restate the hook's shape without decoupling anything. `onSelect` stays a separate prop because switching tabs also dismisses the completion popover — the panel's concern, not the strip's. Verified the extraction is structural: the set of `data-testid`s the panel renders is byte-identical before and after. 930 studio tests unchanged. Closes the last `no-giant-component` suppression in the package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grouping the editor's three scroll-synced refs into one `refs` prop made the React Compiler bail out of the whole pane — it treats a ref reaching the JSX through a member access (`refs.overlay`) as a render-time ref read, so the component lost its automatic memoization. react-doctor reported it as `react-hooks-js/refs`. Passing them as three props restores the optimization. Comments on both the hook's return type and the pane's props record why they are flat, since grouping is otherwise the convention in this refactor. Also formats the two panels from the earlier commits that Prettier had not been run over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis PR decomposes Studio panels into focused hooks and components, extracts shared UI and formatting primitives, centralizes state contracts, adds global-data and mail helpers, improves autocomplete and row-count rendering, expands unit-test registration, and documents intentionally bundled dependencies. ChangesBundled dependency lint suppressions
Organization management
Data browser
Logs and mail
Reports
Schema
SQL editor
Testing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Thank you for following the naming conventions! 🙏 |
Contributor
|
Thank you for confirming the Contributor License Agreement! 🙏 |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mpose-panels # Conflicts: # packages/auth/src/create-auth.ts
Contributor
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Merging this PR will regress 1 benchmark
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
Both jobs have been failing for every PR since #203, in packages that PR touched. Two independent causes. **`import/no-extraneous-dependencies` (8 sites).** `@lunora/search-core` is an internal, unpublished package that packem inlines into `server`, `do`, and `sql-store`, so it is a devDependency on purpose — moving it to `dependencies` would declare a runtime dep on something never published. The rule can't see that, and #203 added *value* imports (type-only imports don't trip it). Annotated each site with the same disable-plus-rationale comment `@lunora/dispatch` already uses in `queue`/`workflow`, which is the established precedent for this exact arrangement. **API snapshot drift (4 entries).** The committed snapshot claimed `SearchLanguage` and `SearchStrategy` were "Re-exported from `@lunora/search-core` — signature tracked at its source", but search-core is unpublished and has no snapshot, so the signature was tracked nowhere. The generator resolves through the built `dist`, where packem has inlined search-core into server's own output — so with a build present it emits the real signature, which is what CI produces and what the gate should hold. Regenerated: `server` now records both signatures, and `lunora` attributes them to `@lunora/server`, making its "tracked at its source" claim true. Both verified against a clean `origin/alpha` checkout, so neither is specific to a feature branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both passed types, ESLint, react-doctor, and all 930 tests, so each now ships with a guard test verified to fail against the bug. **The health panel's error badge was capped at 5.** The extraction switched it from `recentErrors.length` to `topErrors.length`, but `topErrors` is `recentErrors.slice(0, RECENT_ERROR_LIMIT)` — the *display* cap for the list below it. A shard with 137 errors in its buffer read `137`; it now read `5`. The list was always capped; the badge was the only thing reporting the volume, which is exactly the signal that matters during an incident. `HealthDigest` takes an explicit `errorCount` so the two numbers can't be confused again, and the dangling `recentErrors` variable is gone. The existing test asserted `"2"` — under the cap, so it passed either way; the new one uses eight errors. **Pinned columns lost their storage key.** The move renamed `lunora-studio-pinned-columns` to `lunora-studio-data-pinned-columns`. Nothing motivated it — no collision, no migration — and every operator's pins would have silently vanished on upgrade, with the old key orphaned in localStorage forever. Restored, with a comment saying the name is load-bearing and a test that spells the key out literally rather than importing it, so a rename fails instead of following along. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review pass over the extracted modules. Every docblock defending a decomposition choice was checked against the code, and the ones that overstated their case are now either correct or gone — an inflated justification is worse than none, because the next reader either trusts it and reasons wrongly or checks it and stops trusting the rest. Claims corrected: - `metrics-format` said three surfaces share it; one does. The rollup's hit rate arrives pre-reduced to a fraction, so it genuinely can't share `hitRate`'s hits/misses inputs. Also renamed `formatMs`/`formatDuration` to `formatLatency`/`formatElapsed`: two functions taking `ms`, named three characters apart, whose output differs by orders of magnitude. - `stat-card` said three surfaces render it; two do. Dropped the count. - `logs-error-filters` justified the component by a `useCallback` the file never had. - `global-tables-empty-state` called a 12-line SVG eighteen lines. `SqlTabStrip` claimed it received only fields it renders. It received all 22, including the panel's write path into tab state (`patchActiveTab`, `patchActiveOutput`, `unlinkQuery`) and the active tab's run output — the god-object failure mode in a component that otherwise avoided it. Now takes a named `SqlTabStripModel` (a 16-field `Pick`), so the claim holds by construction rather than by assertion. Comments the mechanical moves broke: - `useSchemaExplorer`'s docblock was orphaned above two hoisted constants, so it documented `EMPTY_COLUMNS` while the hook had none. - `data-browser` kept the `PINNED_COLUMNS_KEY` docblock after the constant left. - The mask-preview comment was truncated mid-clause. - `sql-editor-pane` argued the refs are "grouped because they are only ever passed together" 25 lines above the comment explaining why they are NOT grouped — an instruction to reintroduce the React Compiler bail-out. - `logs-panel` kept the seven-severities docblock after `LOG_LEVELS` moved. - `organization-primitives`' module header sat below the first declaration. - `slo-format`'s plural threshold doc attached to only the first constant. Also: - One `ResultTab` instead of two identical exported copies, now declared in `sql-tabs` with the tab model it belongs to. Exported `LogsView` and used it in the bar instead of an inlined duplicate of the union. - Deleted the dead re-export of `LogSummary`/`SummaryBucket` (nothing imports them from `logs-panel`), the `MAX_TABS` pass-through that gave one constant two import paths, and four exported-but-unused names. - Dropped `SchemaViewer`'s `prefer-useReducer` suppression: the six `useState`s it justified are in `useSchemaExplorer` now and the component has none. - Moved the `teamsEnabled`/`rolesEnabled` gates back to the composition site. Behaviour was identical either way, but with early returns inside the sections you cannot see from the page that three of its five are conditional. `OrganizationTeamMembers` now takes a non-null `selectedTeam`, since the page gates on it. 934 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four modules were carved out with docblocks saying they exist so the logic is "unit-testable without a renderer", and none got a test. 24 cases across `slo-format`, `metrics-format`, `global-row-format`, and `mail-selection`, registered in the `unit` project so they run under node with no DOM. The cases are the boundaries these functions exist to get right: the μs/ms/s switch either side of 1ms and 1000ms, floor-not-round on elapsed time, NULL vs empty-string vs a real value in a filter chip, `_id` winning over positional row keys, HTML-body-before-text link extraction and where a URL must stop, and newest-visible fallback for the mail selection. One real divergence surfaced while writing them: `ratePercent` answers a zero denominator with an em-dash, while `rateLevel` answered the same edge "ok" only as a side effect of `NaN >= x` being false — two different answers to one question, inside the module extracted to unify it. `rateLevel` now says so explicitly, and distinguishes the two zero-denominator cases: `0/0` is no traffic and reads "ok", while `errors/0` with errors present is `Infinity` and still breaches, because a missing denominator is not a reason to call errors healthy. That second half preserves the original behaviour — my first attempt at the guard used `Number.isFinite` and silently turned those into "ok", which the test caught. 958 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The PR created `features/reports/stat-card.tsx` while `fanout-panel.tsx`, five
files away in the same directory, kept its own — a card created to be shared
with a duplicate as its neighbour. `fanout-panel`'s props are a strict subset
(`{label, testId, value}` against `{chart?, footer?, label, testId?, value}`)
and the markup differed only by a `justify-between` and a flex wrapper that
have no effect without a chart or footer, so it swaps cleanly.
Moved to `src/components/`, the studio's primitives home, because three
features render this anatomy and none of them owns it. Its props are `readonly`
now, matching the interfaces around them.
Two things deliberately NOT unified, recorded so the next reader doesn't have
to re-derive them:
- **`home-panel`'s `StatCard` is a different component** that happens to share
the noun — it carries a delta, a trend line, and a unit, and calls `useT()`.
Folding it in would be a redesign, not a de-duplication. Noted in the shared
card's docblock.
- **The three remaining `formatMs` variants are three different formats**, not
three copies: `fanout-panel` renders `1234 ms`, `home-panel` `24ms`/`1.2s`,
`function-stats` `9.5ms`/`1.23s`, and `metrics-format.formatLatency`
`400μs`/`1.00s`. Unifying them changes what operators read on four screens,
which is a product call and not something to slip into a refactor that claims
no behaviour change.
958 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three boundaries were drawn by "how many props would it otherwise be" rather than by whether the group is a thing. Redrawn: **`useDataViewPreferences` was two hooks.** Its fourteen fields were read by two consumers with *perfectly disjoint* sets — the page used nine, the browser the other five, zero overlap. That is the tell. Split into `useDataViewPreferences` (pins, masking, transpose — how the grid renders what it has) and `useRowInspection` (which overlay is open). An open detail drawer was never a view preference: it is transient and does not survive a table switch, which is why the old docblock's "these are all view preferences over the same table" was the one claim I could not make true by editing it. `onInspect` is now its own prop on the page, the only inspection field the page reads. **`DataBrowserTableView` took the table model spread into eight props** — in the file that had just argued props should be grouped. Those eight fields already have a name, `DataBrowserTableModel`: one measured view of one table, always produced together, none meaningful alone. Now one `tableModel` prop. Both changes shrink what each side has to know without a god object: the page no longer receives inspection state it does not render, and the grid no longer receives eight fields it immediately recombines. 958 tests pass. `DataBrowser` is 298 lines — under the 300 limit but with little headroom, worth knowing before the next edit to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The multi-recipient case put a quoted address list on the same line as the word "password", which the scanner's generic rule reads as a credential assignment. Hoisting the list clears it without a suppression — the assertions are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
packages/studio/src/features/auth/organization-sections.tsx (1)
82-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: hoist the per-row id.
Each
rowActionscallback callsformatCell(row["id"])two to three times. A singleconst id = formatCell(row["id"]);at the top of the callback body reads better and keeps testid/handler ids provably identical.Also applies to: 167-204, 329-354
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/auth/organization-sections.tsx` around lines 82 - 108, In each rowActions callback, including the sections around the member actions and the other referenced ranges, hoist formatCell(row["id"]) into a local id constant at the start of the callback. Reuse id for all data-testid values and action handlers so each row consistently uses the same formatted identifier.packages/studio/src/features/auth/organization-primitives.tsx (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
readonlyprop bags here too.
organization-sections.tsxmarks every propreadonly; these two primitives don't.readonly columns: readonly Column[]/readonly rows: readonly Row[]would also make the non-mutating contract explicit.Also applies to: 53-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/auth/organization-primitives.tsx` at line 24, Update the prop types for SectionCard and the other organization primitive at the referenced declaration to mark the prop bag fields readonly, including columns/rows as readonly arrays where applicable. Preserve the existing render behavior while making the non-mutating contract consistent with organization-sections.tsx.packages/studio/src/features/data/data-browser-page.tsx (1)
83-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueToolbar strings are only half localized.
t()wraps the live indicator, mask toggle, and "Generate rows", while "Table", "JSON", "Add row", and bothConfirmButtonlabels stay hardcoded English in the same bar. SinceuseTis already in scope here, wrapping the rest is cheap and makes the extraction a good place to close the gap (EmptyStatetitle/description at Lines 276-291 have the same issue).🌐 Example for the confirm labels
- <ConfirmButton confirmLabel={`Delete ${total.toString()} matching?`} onConfirm={onBulkDelete} testId="db-bulk-delete"> - {`Delete ${total.toString()} matching`} + <ConfirmButton confirmLabel={t("Delete {total} matching?", { total: total.toString() })} onConfirm={onBulkDelete} testId="db-bulk-delete"> + {t("Delete {total} matching", { total: total.toString() })} </ConfirmButton>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/data/data-browser-page.tsx` around lines 83 - 122, Complete localization in the data browser toolbar by passing the Table, JSON, Add row, and both ConfirmButton labels/children through the existing useT translator. Also update the EmptyState title and description in the same component to use t(), preserving the current text and interpolation values.packages/studio/src/features/schema/schema-editor-result.tsx (2)
23-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn
ReactElement | nullinstead of empty fragments. Both no-op branches (lines 27 and 52) exist only to satisfy theReactElementannotation;nullis the idiomatic "render nothing" for React components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/schema-editor-result.tsx` around lines 23 - 28, Update the component return type to ReactElement | null and replace both no-op empty-fragment returns in the schema editor result component with null, preserving the existing rendering behavior for non-empty results.
61-65: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDiagnostic strings as keys collide on duplicates. Codegen can emit the same message twice; key on
${index}-${diagnostic}to keep the list stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/schema-editor-result.tsx` around lines 61 - 65, Update the diagnostic list mapping in the schema editor result so each li key combines the diagnostic’s array index with its message, using the `${index}-${diagnostic}` pattern. Preserve the existing diagnostic rendering and styling.packages/studio/src/features/schema/schema-editor-overlay.tsx (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDependency direction: the overlay's state type now comes from its presentational child.
Modedescribes the overlay's workflow, not the button strip; hosting it inlib/schema-edit.ts(alongsideSchemaEditResult) and having the mode bar import it keeps the parent → child direction one-way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/schema-editor-overlay.tsx` around lines 13 - 14, Move the Mode type definition from schema-editor-mode-bar into lib/schema-edit.ts alongside SchemaEditResult, then update SchemaEditorOverlay and SchemaEditorModeBar imports to reference that shared definition. Preserve the existing Mode API and maintain a one-way parent-to-child dependency direction.packages/studio/src/features/schema/hooks/use-schema-explorer.tsx (3)
190-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
toggleGlobalisn't memoized whiletoggleis. It's re-created each render and returned as part of the explorer contract; wrap it inuseCallback([client, globalColumns, globalExpanded])for consistency withtoggleand to keep the returned actions stable for future memoized consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/hooks/use-schema-explorer.tsx` around lines 190 - 212, Memoize the toggleGlobal callback with useCallback using client, globalColumns, and globalExpanded as dependencies, while preserving its existing expansion and loading behavior. Ensure the explorer contract returns this stable callback alongside toggle.
249-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Promise.allSettledaround a single promise reads oddly. BothallSettled([...])wrappers hold exactly one promise;.catch/await ... .thenor a smallsettle()helper would express "don't reject the batch" more directly and drop the[0]indexing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/hooks/use-schema-explorer.tsx` around lines 249 - 261, Replace the single-promise Promise.allSettled wrappers in the described/globalPages loading flow with direct error handling, such as catch-based fallbacks or a shared settle helper. Preserve the current non-rejecting behavior and fallback values, while removing the status check and [0] indexing from the page result handling.
306-320: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo in-flight guard on the graph probe.
shardColumns[shardKey]is only populated afterprobeSchemaresolves, so any re-render that re-evaluates this effect while the probe is pending (view toggled graph → list → graph,globalTablesarriving, another shard's entry landing inshardColumns) fires a second batcheddescribeTablesplus onereadGlobalTablePageper global table. A ref of probed/in-flight shard keys avoids the duplicate round trips.♻️ Sketch
+ const probing = useRef<Set<string>>(new Set()); + useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler -- lazy data-load gated on view + shard, not an event handler - if (view !== "graph" || tables === null || (globalTables === null && globalError === null) || shardColumns[shardKey] !== undefined) { + if (view !== "graph" || tables === null || (globalTables === null && globalError === null) || shardColumns[shardKey] !== undefined || probing.current.has(shardKey)) { return; } + probing.current.add(shardKey); fireAndForget(
refreshwould also need to clear the entry for the re-listed shard so a re-seed re-probes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/hooks/use-schema-explorer.tsx` around lines 306 - 320, Update the graph-loading effect around probeSchema to track probed and in-flight shard keys with a ref, skipping keys already being probed or completed before starting another request. Ensure refresh clears the relevant shard key from this tracking state so re-seeding that shard can probe again.packages/studio/src/features/schema/schema-viewer.tsx (1)
306-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCache-key format is now duplicated across files. The
${shardKey}:${table}convention lives in bothuse-schema-explorer.tsx(lines 159, 175, 183) and here; exporting acolumnCacheKey(shardKey, table)helper from the hook module keeps the two sides from drifting silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/schema/schema-viewer.tsx` around lines 306 - 307, Centralize cache-key construction by exporting a columnCacheKey(shardKey, table) helper from use-schema-explorer.tsx, then replace the inline `${shardKey}:${table.name}` construction in schema-viewer.tsx with that helper. Update the existing cache-key creation sites in the hook module to reuse the same helper and preserve the current key format.packages/studio/src/features/sql/hooks/use-sql-editor-tabs.tsx (1)
114-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
TabMenufor the state type.The inline object type restates the
TabMenuinterface declared just above, so the two can drift.♻️ Proposed tweak
- const [tabMenu, setTabMenu] = useState<{ id: string; x: number; y: number } | null>(null); + const [tabMenu, setTabMenu] = useState<TabMenu | null>(null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/sql/hooks/use-sql-editor-tabs.tsx` around lines 114 - 116, Update the tabMenu state declaration in the useSqlEditorTabs hook to use the existing TabMenu type instead of repeating the inline object shape, while preserving the current nullable state and behavior.packages/studio/src/features/sql/sql-editor-pane.tsx (1)
55-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
EditorHandlersinstead of restating it.
use-sql-editor-surface.tsxalready exportsEditorHandlers; the inline copy can drift and drops thereadonlymodifiers.♻️ Proposed refactor
- readonly handlers: { - onBlur: () => void; - onChange: (event: React.ChangeEvent<HTMLTextAreaElement>) => void; - onKeyDown: (event: React.KeyboardEvent<HTMLTextAreaElement>) => void; - onScroll: (event: React.UIEvent<HTMLTextAreaElement>) => void; - onSelect: (event: React.SyntheticEvent<HTMLTextAreaElement>) => void; - }; + readonly handlers: EditorHandlers;Add to the imports:
import type { EditorHandlers } from "./hooks/use-sql-editor-surface";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/sql/sql-editor-pane.tsx` around lines 55 - 61, Replace the inline handlers type in the relevant component props with the exported EditorHandlers type from use-sql-editor-surface.tsx, importing it as a type. Remove the duplicated handler definitions so the component preserves the shared readonly contract.packages/studio/src/features/sql/sql-tab-strip.tsx (1)
92-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add Escape/focus handling to the context menu.
The
role="menu"container has no Escape-to-close and never moves focus into itself, so keyboard users must tab through the rest of the page to reach the bulk-close items. Focusing the first item on open and closing on Escape would match the ARIA menu pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/sql/sql-tab-strip.tsx` around lines 92 - 97, Update the SQL tab context-menu implementation around the role="menu" container and its bulk-close items to move focus to the first menu item when opened and close the menu when Escape is pressed. Preserve the existing menu actions and styling while adding the required keyboard and focus handling.packages/studio/src/features/logs/logs-error-filters.tsx (1)
58-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten
timeRangeprop to theTimeRangeunion instead ofstring.The
<select>only ever offers"all" | "5m" | "15m" | "1h"(lines 103-106), matching theTimeRangetype already exported fromlogs-panel.tsx. Typing the prop as plainstringhere drops that guarantee at the component boundary, so a future caller could pass an out-of-range value and the select would silently render unselected.♻️ Proposed fix
+import type { TimeRange } from "./logs-panel"; + const LogsErrorFilters = ({ ... }: { ... - readonly timeRange: string; + readonly timeRange: TimeRange; }): ReactElement => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/logs/logs-error-filters.tsx` around lines 58 - 70, Update the timeRange prop in the logs error filters component to use the existing TimeRange union exported by logs-panel.tsx instead of string. Import and apply TimeRange at the component boundary while preserving the current select options and behavior.packages/studio/src/features/logs/mail-panel.tsx (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
PreviewTabtype — use the one exported byuseMailCapture.
use-mail-capture.tsxnowexport type { MailCapture, PreviewTab };specifically so consumers share one definition, but this file still declares its own identical localPreviewTabalias instead of importing it. Harmless today since the literals match, but it can silently drift if the hook's tab set changes.♻️ Proposed fix to reuse the hook's exported type
-import { useMailCapture } from "./hooks/use-mail-capture"; +import { useMailCapture } from "./hooks/use-mail-capture"; +import type { PreviewTab } from "./hooks/use-mail-capture"; import { recipientText } from "./mail-selection";-type PreviewTab = "headers" | "html" | "text";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/logs/mail-panel.tsx` at line 21, Remove the local PreviewTab alias and import the exported PreviewTab type from useMailCapture, updating the mail-panel.tsx type usage to reference that shared definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/studio/src/features/auth/organization-primitives.tsx`:
- Around line 72-87: Update the rows.map callback to use the row index as a
fallback when formatCell(row["id"]) returns an empty value, ensuring both the
TableRow key and data-testid remain unique for rows without an id while
preserving existing identifiers for rows with ids.
In `@packages/studio/src/features/auth/organization-sections.tsx`:
- Around line 14-16: Update the documentation comment in
organization-sections.tsx to replace the stale AdminTable reference with the
current ManagedTable name, without changing the surrounding description.
In `@packages/studio/src/features/logs/mail-selection.ts`:
- Around line 49-58: Update matchingMail to include recipientText(entry.cc) in
the searchable text alongside the subject and to recipients, so messages
matching a cc-only address are returned while preserving the existing
blank-filter behavior.
In `@packages/studio/src/features/schema/schema-editor-result.tsx`:
- Around line 38-49: Update the needs-migration branch in the schema editor
result component to render result.message, falling back to the existing
hardcoded migration warning when no message is provided. Preserve the current
alert styling and behavior, including the Open Migrations action.
In `@packages/studio/src/features/sql/hooks/use-sql-editor-surface.tsx`:
- Around line 80-84: Update onPickSuggestion to commit the clicked suggestion
using the index passed into the handler, rather than relying on
commitAutocomplete reading state.active immediately after moveAutocomplete.
Preserve the existing keyboard behavior and ensure the mouse path inserts the
row identified by index.
In `@packages/studio/src/features/sql/sql-editor-pane.tsx`:
- Around line 81-83: Add an inline ESLint suppression to the gutter line-number
map in the SQL editor pane, immediately alongside the positional key in the
Array.from callback. Use the specified react_x/no-array-index-key rule and
rationale, while leaving the line-number rendering unchanged.
In `@packages/studio/src/features/sql/sql-result-table.tsx`:
- Around line 9-11: Update the zero-column rendering in the SQL result table to
use the existing useT localization path for the “0 rows” message, matching the
equivalent behavior in sql-results-pane.tsx. Ensure the rowCount > 0 case does
not render an empty paragraph by applying the established localized fallback or
conditional rendering behavior.
---
Nitpick comments:
In `@packages/studio/src/features/auth/organization-primitives.tsx`:
- Line 24: Update the prop types for SectionCard and the other organization
primitive at the referenced declaration to mark the prop bag fields readonly,
including columns/rows as readonly arrays where applicable. Preserve the
existing render behavior while making the non-mutating contract consistent with
organization-sections.tsx.
In `@packages/studio/src/features/auth/organization-sections.tsx`:
- Around line 82-108: In each rowActions callback, including the sections around
the member actions and the other referenced ranges, hoist formatCell(row["id"])
into a local id constant at the start of the callback. Reuse id for all
data-testid values and action handlers so each row consistently uses the same
formatted identifier.
In `@packages/studio/src/features/data/data-browser-page.tsx`:
- Around line 83-122: Complete localization in the data browser toolbar by
passing the Table, JSON, Add row, and both ConfirmButton labels/children through
the existing useT translator. Also update the EmptyState title and description
in the same component to use t(), preserving the current text and interpolation
values.
In `@packages/studio/src/features/logs/logs-error-filters.tsx`:
- Around line 58-70: Update the timeRange prop in the logs error filters
component to use the existing TimeRange union exported by logs-panel.tsx instead
of string. Import and apply TimeRange at the component boundary while preserving
the current select options and behavior.
In `@packages/studio/src/features/logs/mail-panel.tsx`:
- Line 21: Remove the local PreviewTab alias and import the exported PreviewTab
type from useMailCapture, updating the mail-panel.tsx type usage to reference
that shared definition.
In `@packages/studio/src/features/schema/hooks/use-schema-explorer.tsx`:
- Around line 190-212: Memoize the toggleGlobal callback with useCallback using
client, globalColumns, and globalExpanded as dependencies, while preserving its
existing expansion and loading behavior. Ensure the explorer contract returns
this stable callback alongside toggle.
- Around line 249-261: Replace the single-promise Promise.allSettled wrappers in
the described/globalPages loading flow with direct error handling, such as
catch-based fallbacks or a shared settle helper. Preserve the current
non-rejecting behavior and fallback values, while removing the status check and
[0] indexing from the page result handling.
- Around line 306-320: Update the graph-loading effect around probeSchema to
track probed and in-flight shard keys with a ref, skipping keys already being
probed or completed before starting another request. Ensure refresh clears the
relevant shard key from this tracking state so re-seeding that shard can probe
again.
In `@packages/studio/src/features/schema/schema-editor-overlay.tsx`:
- Around line 13-14: Move the Mode type definition from schema-editor-mode-bar
into lib/schema-edit.ts alongside SchemaEditResult, then update
SchemaEditorOverlay and SchemaEditorModeBar imports to reference that shared
definition. Preserve the existing Mode API and maintain a one-way
parent-to-child dependency direction.
In `@packages/studio/src/features/schema/schema-editor-result.tsx`:
- Around line 23-28: Update the component return type to ReactElement | null and
replace both no-op empty-fragment returns in the schema editor result component
with null, preserving the existing rendering behavior for non-empty results.
- Around line 61-65: Update the diagnostic list mapping in the schema editor
result so each li key combines the diagnostic’s array index with its message,
using the `${index}-${diagnostic}` pattern. Preserve the existing diagnostic
rendering and styling.
In `@packages/studio/src/features/schema/schema-viewer.tsx`:
- Around line 306-307: Centralize cache-key construction by exporting a
columnCacheKey(shardKey, table) helper from use-schema-explorer.tsx, then
replace the inline `${shardKey}:${table.name}` construction in schema-viewer.tsx
with that helper. Update the existing cache-key creation sites in the hook
module to reuse the same helper and preserve the current key format.
In `@packages/studio/src/features/sql/hooks/use-sql-editor-tabs.tsx`:
- Around line 114-116: Update the tabMenu state declaration in the
useSqlEditorTabs hook to use the existing TabMenu type instead of repeating the
inline object shape, while preserving the current nullable state and behavior.
In `@packages/studio/src/features/sql/sql-editor-pane.tsx`:
- Around line 55-61: Replace the inline handlers type in the relevant component
props with the exported EditorHandlers type from use-sql-editor-surface.tsx,
importing it as a type. Remove the duplicated handler definitions so the
component preserves the shared readonly contract.
In `@packages/studio/src/features/sql/sql-tab-strip.tsx`:
- Around line 92-97: Update the SQL tab context-menu implementation around the
role="menu" container and its bulk-close items to move focus to the first menu
item when opened and close the menu when Escape is pressed. Preserve the
existing menu actions and styling while adding the required keyboard and focus
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88b21c89-2a23-4e8a-bc4a-a3c9f80f97b9
⛔ Files ignored due to path filters (9)
api-snapshots/lunora.api.mdis excluded by none and included by noneapi-snapshots/server.api.mdis excluded by none and included by nonepackages/studio/__tests__/features/data/global-row-format.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/features/data/use-data-view-preferences.test.tsxis excluded by!**/__tests__/**and included bypackages/**packages/studio/__tests__/features/logs/mail-selection.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/features/reports/health-panel.test.tsxis excluded by!**/__tests__/**and included bypackages/**packages/studio/__tests__/features/reports/metrics-format.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/features/reports/slo-format.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**plans/README.mdis excluded by none and included by none
📒 Files selected for processing (52)
packages/do/src/ctx-db-backfill.tspackages/do/src/ctx-db-companions.tspackages/do/src/ctx-db-migrations.tspackages/do/src/ctx-db.tspackages/server/src/schema.tspackages/sql-store/src/ctx-db-search.tspackages/sql-store/src/ctx-db.tspackages/sql-store/src/search-layout.tspackages/studio/src/components/stat-card.tsxpackages/studio/src/features/auth/organization-detail.tsxpackages/studio/src/features/auth/organization-primitives.tsxpackages/studio/src/features/auth/organization-sections.tsxpackages/studio/src/features/data/data-browser-grid.tsxpackages/studio/src/features/data/data-browser-page.tsxpackages/studio/src/features/data/data-browser.tsxpackages/studio/src/features/data/global-data-browser.tsxpackages/studio/src/features/data/global-data-page.tsxpackages/studio/src/features/data/global-row-format.tspackages/studio/src/features/data/global-tables-empty-state.tsxpackages/studio/src/features/data/hooks/use-data-view-preferences.tsxpackages/studio/src/features/data/hooks/use-row-inspection.tsxpackages/studio/src/features/logs/hooks/use-mail-capture.tsxpackages/studio/src/features/logs/logs-error-filters.tsxpackages/studio/src/features/logs/logs-panel.tsxpackages/studio/src/features/logs/logs-request-filters.tsxpackages/studio/src/features/logs/logs-summary.tsxpackages/studio/src/features/logs/logs-view-bar.tsxpackages/studio/src/features/logs/mail-panel.tsxpackages/studio/src/features/logs/mail-selection.tspackages/studio/src/features/reports/fanout-panel.tsxpackages/studio/src/features/reports/health-digest.tsxpackages/studio/src/features/reports/health-panel.tsxpackages/studio/src/features/reports/metrics-aggregate-view.tsxpackages/studio/src/features/reports/metrics-format.tspackages/studio/src/features/reports/metrics-overview-stats.tsxpackages/studio/src/features/reports/metrics-panel.tsxpackages/studio/src/features/reports/slo-format.tspackages/studio/src/features/schema/hooks/use-schema-explorer.tsxpackages/studio/src/features/schema/schema-editor-mode-bar.tsxpackages/studio/src/features/schema/schema-editor-overlay.tsxpackages/studio/src/features/schema/schema-editor-result.tsxpackages/studio/src/features/schema/schema-viewer.tsxpackages/studio/src/features/sql/hooks/use-sql-editor-surface.tsxpackages/studio/src/features/sql/hooks/use-sql-editor-tabs.tsxpackages/studio/src/features/sql/hooks/use-sql-library.tsxpackages/studio/src/features/sql/sql-editor-pane.tsxpackages/studio/src/features/sql/sql-editor-panel.tsxpackages/studio/src/features/sql/sql-result-table.tsxpackages/studio/src/features/sql/sql-results-pane.tsxpackages/studio/src/features/sql/sql-tab-strip.tsxpackages/studio/src/features/sql/sql-tabs.tspackages/studio/vitest.config.ts
Six of seven were valid. One is a real user-facing bug: **Clicking an autocomplete suggestion inserted the highlighted one.** `onPickSuggestion` moved the highlight to the clicked index and then committed — but `move` schedules a state update, so `commit` still read the pre-move `active`. Clicking the third suggestion while the first was highlighted inserted the first. The autocomplete hook now exposes `commitAt(index)` and the mouse path uses it directly; the keyboard path is unchanged. Pinned by a test verified to fail against the old code (it inserted `messages.id` for a click on `messages.body`). Pre-existing on alpha, in code this PR moved. The rest: - **`matchingMail` skipped `cc`.** The detail pane renders cc as a recipient, so filtering by a cc-only address hid a message the operator can see is there. `bcc` stays excluded deliberately — it is never rendered, so a match on it would be unexplainable. Both cases covered. - **`schema-editor-result` discarded `result.message`** in the `needs-migration` branch. The only current producer sets identical text, so nothing is visibly wrong today, but a host-supplied message would never reach the operator. Falls back to the literal when empty. - **`SqlResultTable`'s zero-column branch bypassed `useT`** and rendered an empty paragraph when a columnless result still reported rows. Now localized and reports the count, matching the results pane. - **`ManagedTable` keyed rows on `formatCell(row["id"])`,** which renders a missing id as `""` — every such row would share a key and a test id. Falls back to the index. - **Stale `AdminTable` in a comment** → `ManagedTable`. Not applied: the suggestion to add a `react-x/no-array-index-key` disable to the editor gutter. I checked by rewriting the loop as `.map` — the rule IS active for that file and fires on `.map`, but the gutter uses `Array.from(…, mapper)`, which it does not match. The suppression would be a disable for a rule that never fires. 960 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/studio/src/features/logs/mail-selection.ts (1)
18-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim trailing sentence punctuation from extracted links.
firstLinkcan return values such ashttps://example.com.;selectedLinkthen passes that value directly to copy/open actions. Strip terminal punctuation before returning while preserving punctuation inside the URL.Suggested fix
- return match?.[0]; + return match?.[0].replace(/[.,!?;:]+$/, "");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/studio/src/features/logs/mail-selection.ts` around lines 18 - 28, Update firstLink to remove sentence-ending punctuation from the extracted match before returning it, while preserving punctuation occurring within the URL. Keep the existing undefined behavior and LINK_PATTERN matching flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/studio/src/features/logs/mail-selection.ts`:
- Around line 18-28: Update firstLink to remove sentence-ending punctuation from
the extracted match before returning it, while preserving punctuation occurring
within the URL. Keep the existing undefined behavior and LINK_PATTERN matching
flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a945f2fe-c070-487b-8722-5c7e5a5124de
⛔ Files ignored due to path filters (2)
packages/studio/__tests__/features/logs/mail-selection.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/studio/__tests__/features/sql/sql-editor-panel.test.tsxis excluded by!**/__tests__/**and included bypackages/**
📒 Files selected for processing (7)
packages/studio/src/features/auth/organization-primitives.tsxpackages/studio/src/features/auth/organization-sections.tsxpackages/studio/src/features/logs/mail-selection.tspackages/studio/src/features/schema/schema-editor-result.tsxpackages/studio/src/features/sql/hooks/use-sql-editor-surface.tsxpackages/studio/src/features/sql/sql-autocomplete-ui.tsxpackages/studio/src/features/sql/sql-result-table.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/studio/src/features/auth/organization-sections.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #230.
packages/studiocarried ten components over react-doctor's 300-lineno-giant-componentlimit, each with a suppression comment saying thedecomposition was deferred. This does the decomposition and deletes all ten
suppressions. No behaviour changes.
SqlEditorPanelDataBrowserSchemaViewerOrganizationDetailLogsPanelGlobalDataBrowserMetricsPanelHealthPanelSchemaEditorOverlayMailPanelApproach
Each panel's seam was measured before cutting — the split follows the
logic/JSX ratio the component already had, rather than a uniform recipe:
useStategroups wereinterleaved with a fetch or run path, so it was hard to see where the data
flow ended and the view state began:
useDataViewPreferences,useSchemaExplorer,useMailCapture,useSqlEditorTabs,useSqlLibrary,useSqlEditorSurface.DataBrowserPage,SqlTabStrip,SqlEditorPane,SqlResultsPane,GlobalDataPage, the logs toolbars, the four organization sections, and themetrics/health readouts.
render:
global-row-format,metrics-format,slo-format,mail-selection.Where a naive extraction wanted 50-plus loose props, the new component takes
the models instead —
DataBrowserPagetakesbrowserandpreferences,SqlTabStriptakes the tabs model. Flattening those would have moved thewiring without reducing it, and every field is rendered in exactly one place.
Two things worth a reviewer's eye
grid-featureshas a secondCONTROL_BTNwithout thearia-pressed:*variants. Importing that one when the data-browser toolbar moved out would
have silently dropped the pressed styling from the view and mask toggles, so
the fuller class travels with the toolbar. The duplication is pre-existing.
them made the React Compiler bail out of the whole pane (it reads
refs.overlayin JSX as a render-time ref access), costing the component itsautomatic memoization. Comments at both ends record why.
Verification
react-doctor: 0no-giant-componentfindings, and the package's totalis unchanged from before this branch (5 errors / 51 warnings, 63/100 — all
pre-existing under react-doctor 0.9.1).
dropped by a broken parse.
data-testids the panel renders isbyte-identical before and after — the extraction is structural.
lint:types, ESLint--max-warnings=0, Prettier,build:packages, andapi:check(39 snapshots) all clean.Plus one guard test added along the way:
sql-schema.test.tsxpins theuseSqlSchemamemo's referential stability, which a mid-refactor deletion hadbroken (Escape stopped dismissing the autocomplete).
Review round
Two adversarial review passes over the branch found two real behaviour
changes in a PR claiming none. Both had passed types, ESLint, react-doctor,
and all 930 tests, so each is now fixed and pinned by a test verified to fail
against the bug:
from
recentErrors.lengthtotopErrors.length— the latter beingslice(0, RECENT_ERROR_LIMIT), the display cap for the list below. A shardwith 137 errors in its buffer read
5. The list was always capped; the badgewas the only thing reporting volume, which is the signal that matters during
an incident. The existing test asserted
"2"— under the cap, so it passedeither way.
lunora-studio-pinned-columnstolunora-studio-data-pinned-columnswith nocollision or migration to motivate it, silently discarding every operator's
pins on upgrade. The new test spells the key out literally rather than
importing it, so a rename fails instead of following along.
The rest of the round tightened what the decomposition claims about itself:
code. Six overstated their case (a three-consumer claim with one consumer, a
three-surface claim with two, an eighteen-field claim over 22, a
useCallbackthe file never had, a twelve-line SVG called eighteen, and twosections documented as "explaining" a disabled feature while returning an
empty fragment). All corrected. Seven more were orphaned or truncated by the
mechanical moves — including one in
sql-editor-panethat argued for theref grouping the comment 25 lines below explains we removed, i.e. an
instruction to reintroduce the React Compiler bail-out.
SqlTabStripwas receiving the panel's whole write path into tab stateplus the active tab's run output — 22 fields for the 16 it renders. Now takes
a named 16-field
Pick, so the claim holds by construction.useDataViewPreferenceswas two hooks. Its two consumers used perfectlydisjoint field sets (9 and 5, zero overlap). Split into view preferences and
row inspection; an open detail drawer was never a view preference, which is
why that docblock was the one claim I couldn't make true by editing it.
DataBrowserTableViewtook the table model spread into eight props — inthe file that had just argued props should be grouped. Now one
tableModel.StatCard, incomponents/.fanout-panelkept a duplicate fivefiles from the one this PR created to be shared; its props were a strict
subset.
home-panel's same-named component is genuinely different (delta,trend, unit) and stays.
ResultTabinstead of two exported copies,LogsViewexportedinstead of re-inlined, and the dead exports deleted (a re-export nothing
imports, a
MAX_TABSpass-through giving one constant two import paths, fourexported-but-unused names, and
SchemaViewer's now-deadprefer-useReducersuppression).
teamsEnabled/rolesEnabledearly returns inside the sections were behaviour-identical butmeant you couldn't see from the page that three of its five sections are
conditional.
shipped with none. Writing them surfaced a real divergence inside the module
extracted to unify it:
ratePercentanswers a zero denominator with anem-dash while
rateLevelanswered "ok" only as a side effect ofNaN >= xbeing false. Now explicit, and distinguishing
0/0(no traffic → ok) fromerrors/0(Infinity → still breaches).Test count 930 → 958.
Deliberately not changed, so it isn't lost: the three remaining
formatMsvariants render genuinely different strings (
1234 ms,24ms/1.2s,9.5ms/1.23s,400μs/1.00s). Unifying them changes what operators read onfour screens — a product call, not a de-duplication, and not something to slip
into a refactor claiming no behaviour change.
DataBrowseralso sits at 298lines: under the limit, but with little headroom.
Two structural suggestions from the review are real and not done here, because
each is its own refactor with its own review — the same argument this PR makes
about why the suppressions existed: collapsing
useSchemaExplorer's twoidentical table planes into one
useTablePlaneused twice (13 of its 26 fieldsand a whole duplicated
toggleare written out twice), and extractinglogs-panel's purefilterLogs/summarizeLogsso their tests can drop therender harness.
🤖 Generated with Claude Code
Summary by CodeRabbit