Repository navigation
feat: sql cell improvements - #471
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughExtracts SQL cell UI into reusable components (editor, preview, results, connection selector, dependents menu, result-name editor), adds getSqlQueryPreview and tests, migrates render-to-DOM utilities, and replaces AJV JSON-schema validation with vscode-json-languageservice across editor tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SqlCellEditor
participant SqlCellConnectionSelector
participant QueryRunner as RunAPI/Server
participant SqlCellResults
participant SqlCellDependentsMenu
User->>SqlCellEditor: Edit SQL / click Run
SqlCellEditor->>SqlCellConnectionSelector: read selected connector
SqlCellEditor->>QueryRunner: submit query (sql, connector)
QueryRunner-->>SqlCellResults: returns result metadata (resultName, version)
SqlCellResults->>QueryRunner: fetchCellResultPage(pagination, sorting)
QueryRunner-->>SqlCellResults: page of rows / arrow table data
User->>SqlCellResults: expand/collapse, change sorting/pagination
User->>SqlCellDependentsMenu: hover to view dependents
SqlCellDependentsMenu->>User: render downstream list (scroll-to-cell on click)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
dmitriy-kostianetskiy
changed the title
dmitriy-kostianetskiy
marked this pull request as ready for review
March 16, 2026 11:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cells/src/components/SqlCellResults.tsx (1)
122-128: Consider usingaria-busyinstead ofaria-hiddenfor the loading overlay.The overlay indicates a loading/busy state rather than hidden content. Using
aria-busyon the parent oraria-liveregion would be more semantically accurate for accessibility.Suggested improvement
- <div - aria-hidden={!isRunning} - className={cn( + <div + role="status" + aria-label={isRunning ? "Loading results" : undefined} + className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cells/src/components/SqlCellResults.tsx` around lines 122 - 128, The overlay currently uses aria-hidden={!isRunning} which is semantically incorrect for a loading state; update the markup so the busy state is exposed with aria-busy (or an aria-live region) instead: remove aria-hidden from the overlay div and set aria-busy={isRunning} on the containing element (the parent wrapper around the overlay in SqlCellResults) or add an appropriate aria-live status for screen readers; keep the visual behavior tied to isRunning and preserve the existing className/animation logic (reference isRunning and the overlay div in SqlCellResults).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cells/src/components/CollapsibleSectionButton.tsx`:
- Around line 20-23: The toggle button is only visible on hover which hides it
from keyboard users; update the button in CollapsibleSectionButton.tsx (the
element with onClick and aria-label using isCollapsed) to include
focus/focus-visible visibility classes (e.g., add focus:opacity-50 or
focus-visible:opacity-50 and a visible focus ring) so it becomes visible when
keyboard-focused, and add an aria-expanded attribute
(aria-expanded={!isCollapsed}) to expose the expanded/collapsed state to
assistive tech; keep the existing onClick and aria-label logic but ensure focus
styles match the hover visibility.
In `@packages/cells/src/components/SqlCellResults.tsx`:
- Around line 52-61: The effect watching pagination/sorting/cellId currently
omits resultName so changes to the displayed result (e.g., rename or switching
result) won't trigger fetchCellResultPage; update the dependency array for the
useEffect that uses isFirstRender to include resultName (alongside pagination,
sorting, cellId, fetchCellResultPage) so the effect re-runs when resultName
changes, and keep the existing first-render guard intact.
In `@packages/codemirror/src/components/CodeMirrorEditor.tsx`:
- Line 213: The mount node for the editor uses className "h-auto w-full" which
breaks full-height/absolute layouts; update the div with ref editorRef in
CodeMirrorEditor (the editor mount node) to use "h-full w-full" instead of
"h-auto w-full" so the editor fills its container (preserving existing layouts
that rely on h-full).
---
Nitpick comments:
In `@packages/cells/src/components/SqlCellResults.tsx`:
- Around line 122-128: The overlay currently uses aria-hidden={!isRunning} which
is semantically incorrect for a loading state; update the markup so the busy
state is exposed with aria-busy (or an aria-live region) instead: remove
aria-hidden from the overlay div and set aria-busy={isRunning} on the containing
element (the parent wrapper around the overlay in SqlCellResults) or add an
appropriate aria-live status for screen readers; keep the visual behavior tied
to isRunning and preserve the existing className/animation logic (reference
isRunning and the overlay div in SqlCellResults).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8afc3575-2d66-4cfe-abc1-df4859960d72
📒 Files selected for processing (11)
packages/cells/__tests__/utils.sqlQueryPreview.test.tspackages/cells/src/components/CollapsibleSectionButton.tsxpackages/cells/src/components/SqlCellConnectionSelector.tsxpackages/cells/src/components/SqlCellContent.tsxpackages/cells/src/components/SqlCellDependentsMenu.tsxpackages/cells/src/components/SqlCellEditor.tsxpackages/cells/src/components/SqlCellResultNameEditor.tsxpackages/cells/src/components/SqlCellResults.tsxpackages/cells/src/components/SqlQueryPreview.tsxpackages/cells/src/utils.tspackages/codemirror/src/components/CodeMirrorEditor.tsx
Collaborator
Author
|
@copilot fix code review comments |
Contributor
|
@dmitriy-kostianetskiy I've opened a new pull request, #472, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: dmitriy-kostianetskiy <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cells/src/components/SqlCellResults.tsx (1)
49-73: Avoid reset-triggered redundant refetch after a new run.The code comment says first page is already fetched externally, but resetting pagination/sorting on
resultVersionchange can still trigger an extra fetch for page 0 when the user was on a non-default table state.Suggested patch
const isFirstRender = useRef(true); + const skipNextFetchAfterVersionReset = useRef(false); useEffect(() => { if (isFirstRender.current) { isFirstRender.current = false; return; } + if (skipNextFetchAfterVersionReset.current) { + skipNextFetchAfterVersionReset.current = false; + return; + } if (resultName) { fetchCellResultPage(cellId, pagination, sorting); } }, [pagination, sorting, cellId, fetchCellResultPage, resultName]); // Reset pagination when a new result arrives (new run) const prevResultVersion = useRef(resultVersion); useEffect(() => { if (resultVersion !== prevResultVersion.current) { prevResultVersion.current = resultVersion; + const shouldReset = pagination.pageIndex !== 0 || sorting.length > 0; + if (shouldReset) { + skipNextFetchAfterVersionReset.current = true; + } // eslint-disable-next-line react-hooks/set-state-in-effect setPagination((prev) => prev.pageIndex === 0 ? prev : {...prev, pageIndex: 0}, ); setSorting((prev) => (prev.length === 0 ? prev : [])); } - }, [resultVersion]); + }, [resultVersion, pagination.pageIndex, sorting.length]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cells/src/components/SqlCellResults.tsx` around lines 49 - 73, The reset of pagination/sorting on resultVersion change can still trigger the fetch effect and cause an unnecessary refetch; to fix this, after updating pagination/sorting in the resultVersion useEffect, mark the render guard used by the fetch effect (isFirstRender) so it skips the next effect run (e.g., set isFirstRender.current = true), or alternatively add a dedicated guard that compares prevResultVersion to avoid calling fetchCellResultPage when the reset originates from resultVersion; update the useEffect that watches [pagination, sorting, cellId, fetchCellResultPage, resultName] and/or the resultVersion effect (prevResultVersion, setPagination, setSorting, isFirstRender) to implement this guard so resetting to pageIndex 0 or clearing sorting does not trigger an extra fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cells/src/components/SqlCellResults.tsx`:
- Around line 49-73: The reset of pagination/sorting on resultVersion change can
still trigger the fetch effect and cause an unnecessary refetch; to fix this,
after updating pagination/sorting in the resultVersion useEffect, mark the
render guard used by the fetch effect (isFirstRender) so it skips the next
effect run (e.g., set isFirstRender.current = true), or alternatively add a
dedicated guard that compares prevResultVersion to avoid calling
fetchCellResultPage when the reset originates from resultVersion; update the
useEffect that watches [pagination, sorting, cellId, fetchCellResultPage,
resultName] and/or the resultVersion effect (prevResultVersion, setPagination,
setSorting, isFirstRender) to implement this guard so resetting to pageIndex 0
or clearing sorting does not trigger an extra fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc9faba1-9788-4e31-b700-692864f0601b
📒 Files selected for processing (2)
packages/cells/src/components/CollapsibleSectionButton.tsxpackages/cells/src/components/SqlCellResults.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cells/src/components/CollapsibleSectionButton.tsx
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: bdjulbic <[email protected]> Signed-off-by: Ilya Boyandin <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]> Co-authored-by: Cursor Agent <[email protected]>
Co-authored-by: Claude Sonnet 4.5 <[email protected]>
mihajlo-stojkovic
approved these changes
Mar 17, 2026
mihajlo-stojkovic
left a comment
mihajlo-stojkovic
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, nice decomposition to clean up the code 🚀
Only had one question regarding a select dropdown component in cell connection selector 😄
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/sqlrooms-cli-ui/src/serverApi.ts (2)
184-189: Consider defensive error handling for JSON parsing.If the server returns a non-JSON response on error,
res.json()will throw a less informative error. Additionally, the type cast doesn't validate thatpathexists (thoughvalidateServerPathwill catch this).🛡️ Suggested improvement
const res = await fetch(uploadUrl, {method: 'POST', body: form}); if (!res.ok) { - throw new Error(`Upload failed: ${res.statusText}`); + const text = await res.text().catch(() => ''); + throw new Error(`Upload failed: ${res.statusText}${text ? ` - ${text}` : ''}`); } - const data = (await res.json()) as {path: string}; + let data: {path?: unknown}; + try { + data = await res.json(); + } catch { + throw new Error('Upload failed: server returned invalid JSON'); + } return validateServerPath(data.path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sqlrooms-cli-ui/src/serverApi.ts` around lines 184 - 189, Wrap the response JSON parsing in a try/catch and provide a clearer error when parsing fails: after POSTing to uploadUrl and verifying res.ok, attempt to call res.json() inside a try block, and if it throws, read res.text() (or include res.status/res.statusText) and throw a new Error that includes the parsing error plus the raw response or status for debugging; also validate that the parsed object contains a string path before casting/returning it (call validateServerPath only after confirming typeof data?.path === 'string' and throw a descriptive error if missing). Use the existing symbols uploadUrl, res.json(), validateServerPath and res.status/res.statusText to locate where to change the code.
148-175: Minor: Redundant backslash in regex after normalization.After
raw.replace(/\\/g, '/')normalizes backslashes to forward slashes, the\\inSAFE_PATH_REwill never match. Consider simplifying the regex for clarity:♻️ Suggested simplification
-const SAFE_PATH_RE = /^[A-Za-z0-9_\-./:\\]+$/; +const SAFE_PATH_RE = /^[A-Za-z0-9_\-./:]+$/;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sqlrooms-cli-ui/src/serverApi.ts` around lines 148 - 175, The regex SAFE_PATH_RE contains an unnecessary backslash character class entry that will never match after validateServerPath normalizes backslashes to forward slashes (via raw.replace(/\\/g, '/')), so update SAFE_PATH_RE to remove the backslash from the character class and ensure the hyphen is either escaped or placed at an end to avoid forming a range; keep validateServerPath and the normalization call as-is and only adjust the SAFE_PATH_RE declaration (reference SAFE_PATH_RE and validateServerPath to locate the change).packages/codemirror/__tests__/validate-json-schema.test.ts (1)
248-251: Make multi-error assertions order-independent.A few tests assume stable diagnostic ordering by index; this can become flaky. Prefer asserting on a sorted message list or
arrayContaining.Also applies to: 335-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/codemirror/__tests__/validate-json-schema.test.ts` around lines 248 - 251, The test currently asserts diagnostics by fixed indexes (expect(diagnostics[0]?.message)... in the test body that checks diagnostics length and messages); change it to be order-independent by extracting messages = diagnostics.map(d => d?.message) and then asserting either expect(messages).toEqual(expect.arrayContaining(['Missing property "name".', 'Value is below the minimum of 0.'])) or sort messages and compare to a sorted expected array; update both occurrences around the test that uses diagnostics (and the similar block at lines ~335-337) so assertions don't rely on stable ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/codemirror/__tests__/vega-schema-integration.test.ts`:
- Around line 9-15: The test currently performs a live network fetch inside the
beforeAll block (see beforeAll, SCHEMA_DOWNLOAD_TIMEOUT, and schema variable)
which makes CI flaky; replace the fetch call with a pinned local fixture by
loading a checked-in copy of the Vega-Lite v5 JSON (e.g. require/import
'./fixtures/vega-lite-v5.json' or read it via fs) and assign it to schema in the
same beforeAll, removing the external network dependency and keeping the
existing timeout handling if still needed.
In `@packages/codemirror/src/components/DiagnosticTooltip.tsx`:
- Around line 12-13: The JSX in the DiagnosticTooltip component currently uses a
truthy check "{code && ...}" which omits valid numeric codes like 0; change the
condition to test for null/undefined instead (e.g., use "code != null" or
"typeof code !== 'undefined'") where the span rendering is done so that numeric
0 is rendered correctly; update the conditional around the span that displays
the code in DiagnosticTooltip to use that null/undefined check.
In `@packages/codemirror/src/utils/validate-json-schema.ts`:
- Around line 54-61: renderMessage currently mounts React roots via
renderComponentToDomElement inside the Diagnostic.renderMessage hook (calls in
renderMessage and the second call around lines 75–80) and drops the returned
destroy callback, leaking React roots; replace these usages by returning a
simple DOM Node or string instead of mounting DiagnosticTooltip with
renderComponentToDomElement. Locate the renderMessage implementations that call
renderComponentToDomElement(DiagnosticTooltip, { message, code }) and change
them to create a plain element (e.g., document.createElement('div') with
textContent or innerHTML set) or return the string, ensuring no React root is
created and no destroy callback is required.
In `@packages/kepler/src/components/KeplerImageExport.tsx`:
- Around line 57-67: The current useEffect in KeplerImageExport only checks
CUSTOM_RESOLUTION_OPTIONS.some by id, so a resolution that exists but has
available: false can still be kept; update the validation to ensure the matched
option also has available === true (e.g., find the option by id and verify
option.available) before treating it as supported, and when not supported or not
available call setExportImageSetting({ resolution: Resolution1280x720Option.id
}); keep existing dependencies (resolution, setExportImageSetting) and reference
the existing symbols CUSTOM_RESOLUTION_OPTIONS, resolution,
setExportImageSetting, Resolution1280x720Option, and useEffect when making the
change.
In `@packages/sql-editor/src/codemirror/extensions/completion.ts`:
- Around line 52-63: The current code calls
renderComponentToDomElement(FunctionDocumentation, { functions: overloads })
outside of the Completion.info callback, creating and later destroying a React
root for every suggestion and reusing a DOM node that React can't re-mount; move
the renderComponentToDomElement call into the info() function so a fresh
DOM/root is created each time the tooltip opens and destroyed on close (keep the
info signature returning { dom, destroy }), referencing
renderComponentToDomElement, FunctionDocumentation, and the info() callback to
locate and update the code.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/serverApi.ts`:
- Around line 184-189: Wrap the response JSON parsing in a try/catch and provide
a clearer error when parsing fails: after POSTing to uploadUrl and verifying
res.ok, attempt to call res.json() inside a try block, and if it throws, read
res.text() (or include res.status/res.statusText) and throw a new Error that
includes the parsing error plus the raw response or status for debugging; also
validate that the parsed object contains a string path before casting/returning
it (call validateServerPath only after confirming typeof data?.path === 'string'
and throw a descriptive error if missing). Use the existing symbols uploadUrl,
res.json(), validateServerPath and res.status/res.statusText to locate where to
change the code.
- Around line 148-175: The regex SAFE_PATH_RE contains an unnecessary backslash
character class entry that will never match after validateServerPath normalizes
backslashes to forward slashes (via raw.replace(/\\/g, '/')), so update
SAFE_PATH_RE to remove the backslash from the character class and ensure the
hyphen is either escaped or placed at an end to avoid forming a range; keep
validateServerPath and the normalization call as-is and only adjust the
SAFE_PATH_RE declaration (reference SAFE_PATH_RE and validateServerPath to
locate the change).
In `@packages/codemirror/__tests__/validate-json-schema.test.ts`:
- Around line 248-251: The test currently asserts diagnostics by fixed indexes
(expect(diagnostics[0]?.message)... in the test body that checks diagnostics
length and messages); change it to be order-independent by extracting messages =
diagnostics.map(d => d?.message) and then asserting either
expect(messages).toEqual(expect.arrayContaining(['Missing property "name".',
'Value is below the minimum of 0.'])) or sort messages and compare to a sorted
expected array; update both occurrences around the test that uses diagnostics
(and the similar block at lines ~335-337) so assertions don't rely on stable
ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19f27f6e-4dc4-4d9f-87c4-993942c331c9
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpython/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.gitignoreapps/sqlrooms-cli-ui/src/serverApi.tsapps/sqlrooms-cli-ui/src/store.tspackages/codemirror/__mocks__/@sqlrooms/utils.tspackages/codemirror/__tests__/validate-json-schema.test.tspackages/codemirror/__tests__/vega-schema-integration.test.tspackages/codemirror/package.jsonpackages/codemirror/src/components/DiagnosticTooltip.tsxpackages/codemirror/src/components/JsonCodeMirrorEditor.tsxpackages/codemirror/src/extensions/json-schema-lint.tspackages/codemirror/src/themes/base-theme.tspackages/codemirror/src/themes/tooltip-theme.tspackages/codemirror/src/utils/create-json-schema-validator.tspackages/codemirror/src/utils/validate-json-schema.tspackages/data-table/src/DataTablePaginated.tsxpackages/data-table/src/QueryDataTableActionsMenu.tsxpackages/kepler/package.jsonpackages/kepler/src/components/KeplerImageExport.tsxpackages/notebook/src/cellOperations/AddNewCellDropdown.tsxpackages/sql-editor/src/codemirror/extensions/completion.tspackages/sql-editor/src/codemirror/extensions/duck-db/duckdb-sql-extension.tspackages/sql-editor/src/codemirror/extensions/hover.tspackages/utils/src/index.tspackages/utils/src/render-component.tsxpackages/utils/src/renderComponentToString.tsxpackages/vega/src/VegaEditAction.tsxpython/sqlrooms-cli/README.md
💤 Files with no reviewable changes (2)
- packages/utils/src/renderComponentToString.tsx
- packages/data-table/src/QueryDataTableActionsMenu.tsx
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+9
to
+15
| beforeAll(async () => { | ||
| // Fetch the actual Vega-Lite schema | ||
| const response = await fetch( | ||
| 'https://vega.github.io/schema/vega-lite/v5.json', | ||
| ); | ||
| schema = await response.json(); | ||
| }, SCHEMA_DOWNLOAD_TIMEOUT); |
Contributor
There was a problem hiding this comment.
Avoid live network calls in unit tests.
This makes CI flaky and non-deterministic (offline/rate-limit/upstream schema drift). Please load a pinned local fixture instead.
💡 Suggested change
+import {readFile} from 'node:fs/promises';
import {validateJsonSchema} from '../src/utils/validate-json-schema';
import {createJsonSchemaValidator} from '../src/utils/create-json-schema-validator';
-const SCHEMA_DOWNLOAD_TIMEOUT = 30000; // 30 second timeout for schema download
-
describe('Vega-Lite schema integration', () => {
let schema: object;
beforeAll(async () => {
- // Fetch the actual Vega-Lite schema
- const response = await fetch(
- 'https://vega.github.io/schema/vega-lite/v5.json',
- );
- schema = await response.json();
- }, SCHEMA_DOWNLOAD_TIMEOUT);
+ const json = await readFile(
+ new URL('./fixtures/vega-lite-v5.json', import.meta.url),
+ 'utf8',
+ );
+ schema = JSON.parse(json);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/codemirror/__tests__/vega-schema-integration.test.ts` around lines 9
- 15, The test currently performs a live network fetch inside the beforeAll
block (see beforeAll, SCHEMA_DOWNLOAD_TIMEOUT, and schema variable) which makes
CI flaky; replace the fetch call with a pinned local fixture by loading a
checked-in copy of the Vega-Lite v5 JSON (e.g. require/import
'./fixtures/vega-lite-v5.json' or read it via fs) and assign it to schema in the
same beforeAll, removing the external network dependency and keeping the
existing timeout handling if still needed.
Comment on lines
+12
to
+13
| {code && ( | ||
| <span className="text-muted-foreground opacity-70"> ({code})</span> |
Contributor
There was a problem hiding this comment.
Don't treat diagnostic code 0 as absent.
{code && ...} on Line 12 skips valid numeric codes like 0. Check for null/undefined instead.
Suggested fix
- {code && (
+ {code !== null && code !== undefined && (
<span className="text-muted-foreground opacity-70"> ({code})</span>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {code && ( | |
| <span className="text-muted-foreground opacity-70"> ({code})</span> | |
| {code !== null && code !== undefined && ( | |
| <span className="text-muted-foreground opacity-70"> ({code})</span> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/codemirror/src/components/DiagnosticTooltip.tsx` around lines 12 -
13, The JSX in the DiagnosticTooltip component currently uses a truthy check
"{code && ...}" which omits valid numeric codes like 0; change the condition to
test for null/undefined instead (e.g., use "code != null" or "typeof code !==
'undefined'") where the span rendering is done so that numeric 0 is rendered
correctly; update the conditional around the span that displays the code in
DiagnosticTooltip to use that null/undefined check.
Comment on lines
+54
to
+61
| renderMessage: () => { | ||
| const {dom} = renderComponentToDomElement(DiagnosticTooltip, { | ||
| message, | ||
| code, | ||
| }); | ||
|
|
||
| return dom; | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CodeMirror 6 lint, does Diagnostic.renderMessageprovide any cleanup lifecycle beyond returning aNode?
💡 Result:
No. In CM6 lint, Diagnostic.renderMessage is only a render hook: it’s called to obtain a Node to display for the diagnostic message, and there’s no paired “dispose/cleanup” callback in the Diagnostic API. The node may later simply be removed from the DOM when the tooltip/panel is updated/closed. If your rendered content allocates resources that need explicit teardown (e.g., global event listeners, timers, observers), you must manage that yourself. [1], [2], [3]
Sources
- CodeMirror 6 reference (
Diagnostic.renderMessage?: (view: EditorView) => Node) https://codemirror.net/docs/ref/#lint.Diagnostic.renderMessage renderDiagnosticusesdiagnostic.renderMessage(view)inline when building the DOM https://github.com/codemirror/lint/blob/main/src/lint.ts- Panel sync removes DOM nodes with
prev.remove()(no custom cleanup hook) https://github.com/codemirror/lint/blob/main/src/lint.ts
React roots in renderMessage can't be cleaned up by the CodeMirror lint API.
Lines 54–61 and 75–80 call renderComponentToDomElement() which returns a destroy() callback, but this callback is dropped immediately. CodeMirror's Diagnostic.renderMessage hook only receives a Node and has no paired cleanup lifecycle—when the tooltip is updated or closed, CodeMirror simply removes the DOM node without invoking root cleanup. This leaves React roots in memory. Replace these with plain DOM or string rendering to avoid managing React lifecycle within a stateless API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/codemirror/src/utils/validate-json-schema.ts` around lines 54 - 61,
renderMessage currently mounts React roots via renderComponentToDomElement
inside the Diagnostic.renderMessage hook (calls in renderMessage and the second
call around lines 75–80) and drops the returned destroy callback, leaking React
roots; replace these usages by returning a simple DOM Node or string instead of
mounting DiagnosticTooltip with renderComponentToDomElement. Locate the
renderMessage implementations that call
renderComponentToDomElement(DiagnosticTooltip, { message, code }) and change
them to create a plain element (e.g., document.createElement('div') with
textContent or innerHTML set) or return the string, ensuring no React root is
created and no destroy callback is required.
Comment on lines
+57
to
+67
| useEffect(() => { | ||
| // hardcode default resolution only when incoming resolution is not a supported custom option | ||
| const isSupportedResolution = CUSTOM_RESOLUTION_OPTIONS.some( | ||
| (option) => option.id === resolution, | ||
| ); | ||
| if (!isSupportedResolution) { | ||
| setExportImageSetting({ | ||
| resolution: Resolution1280x720Option.id, | ||
| }); | ||
| } | ||
| }, [resolution, setExportImageSetting]); |
Contributor
There was a problem hiding this comment.
Fallback resolution should account for available options
The fallback validation only checks ID presence, but the select UI also enforces option.available. This can keep/set a resolution that is listed but unusable.
Proposed fix
useEffect(() => {
- // hardcode default resolution only when incoming resolution is not a supported custom option
- const isSupportedResolution = CUSTOM_RESOLUTION_OPTIONS.some(
- (option) => option.id === resolution,
+ const availableOptions = CUSTOM_RESOLUTION_OPTIONS.filter(
+ (option) => option.available,
);
- if (!isSupportedResolution) {
+ const isSupportedResolution = availableOptions.some(
+ (option) => option.id === resolution,
+ );
+ const preferredFallback =
+ availableOptions.find((option) => option.id === Resolution1280x720Option.id) ??
+ availableOptions[0];
+
+ if (!isSupportedResolution && preferredFallback) {
setExportImageSetting({
- resolution: Resolution1280x720Option.id,
+ resolution: preferredFallback.id,
});
}
}, [resolution, setExportImageSetting]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| // hardcode default resolution only when incoming resolution is not a supported custom option | |
| const isSupportedResolution = CUSTOM_RESOLUTION_OPTIONS.some( | |
| (option) => option.id === resolution, | |
| ); | |
| if (!isSupportedResolution) { | |
| setExportImageSetting({ | |
| resolution: Resolution1280x720Option.id, | |
| }); | |
| } | |
| }, [resolution, setExportImageSetting]); | |
| useEffect(() => { | |
| const availableOptions = CUSTOM_RESOLUTION_OPTIONS.filter( | |
| (option) => option.available, | |
| ); | |
| const isSupportedResolution = availableOptions.some( | |
| (option) => option.id === resolution, | |
| ); | |
| const preferredFallback = | |
| availableOptions.find((option) => option.id === Resolution1280x720Option.id) ?? | |
| availableOptions[0]; | |
| if (!isSupportedResolution && preferredFallback) { | |
| setExportImageSetting({ | |
| resolution: preferredFallback.id, | |
| }); | |
| } | |
| }, [resolution, setExportImageSetting]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kepler/src/components/KeplerImageExport.tsx` around lines 57 - 67,
The current useEffect in KeplerImageExport only checks
CUSTOM_RESOLUTION_OPTIONS.some by id, so a resolution that exists but has
available: false can still be kept; update the validation to ensure the matched
option also has available === true (e.g., find the option by id and verify
option.available) before treating it as supported, and when not supported or not
available call setExportImageSetting({ resolution: Resolution1280x720Option.id
}); keep existing dependencies (resolution, setExportImageSetting) and reference
the existing symbols CUSTOM_RESOLUTION_OPTIONS, resolution,
setExportImageSetting, Resolution1280x720Option, and useEffect when making the
change.
ilyabo
approved these changes
Mar 17, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cells/src/components/SqlCellContent.tsx (1)
124-132: Consider memoizing thestatusobject.The
statusobject is recreated on every render and passed toSqlCellRunButton. If the child component doesn't implement shallow comparison, this could cause unnecessary re-renders.♻️ Optional: Memoize status object
+ const status = useMemo( + () => + cellStatus?.type === 'sql' + ? { + state: cellStatus.status, + message: cellStatus.lastError, + resultName: cellStatus.resultView || cellStatus.resultName, + lastRunTime: cellStatus.lastRunTime, + } + : undefined, + [cellStatus], + ); - const status = - cellStatus?.type === 'sql' - ? { - state: cellStatus.status, - message: cellStatus.lastError, - resultName: cellStatus.resultView || cellStatus.resultName, - lastRunTime: cellStatus.lastRunTime, - } - : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cells/src/components/SqlCellContent.tsx` around lines 124 - 132, The status object is recreated each render and passed to SqlCellRunButton causing potential unnecessary re-renders; wrap the status construction in a useMemo inside the SqlCellContent component (i.e., memoize status based on cellStatus and any fields used such as cellStatus?.type, cellStatus?.status, cellStatus?.lastError, cellStatus?.resultView, cellStatus?.resultName, cellStatus?.lastRunTime) so the same reference is returned when values haven't changed before passing it to SqlCellRunButton.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cells/src/components/SqlCellContent.tsx`:
- Around line 124-132: The status object is recreated each render and passed to
SqlCellRunButton causing potential unnecessary re-renders; wrap the status
construction in a useMemo inside the SqlCellContent component (i.e., memoize
status based on cellStatus and any fields used such as cellStatus?.type,
cellStatus?.status, cellStatus?.lastError, cellStatus?.resultView,
cellStatus?.resultName, cellStatus?.lastRunTime) so the same reference is
returned when values haven't changed before passing it to SqlCellRunButton.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e57446e-099f-4410-8dbd-39b1681d063f
📒 Files selected for processing (3)
packages/cells/src/components/SqlCellConnectionSelector.tsxpackages/cells/src/components/SqlCellContent.tsxpackages/sql-editor/src/codemirror/extensions/completion.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cells/src/components/SqlCellConnectionSelector.tsx
- packages/sql-editor/src/codemirror/extensions/completion.ts
dmitriy-kostianetskiy
added a commit
that referenced
this pull request
Mar 20, 2026
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
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.
This pull request refactors the
SqlCellContentcomponent by extracting several sections into dedicated, reusable components, improving code organization and maintainability. It also introduces new UI components for SQL cell editing, connection selection, dependents menu, and query preview, and adds comprehensive tests for SQL query preview logic.Component extraction and UI improvements:
Extracted the SQL editor, connection selector, dependents menu, result name editor, and results display into separate components:
SqlCellEditor,SqlCellConnectionSelector,SqlCellDependentsMenu,SqlCellResultNameEditor, andSqlCellResults, respectively. This modularizes the UI and simplifiesSqlCellContent. [1] [2] [3] [4] [5] [6] [7] [8]Added a collapsible section button (
CollapsibleSectionButton) for the SQL editor, allowing users to collapse/expand the editor and see a preview of the SQL query when collapsed. [1] [2]New features and test coverage:
SqlQueryPreviewcomponent and added comprehensive unit tests for the SQL query preview logic (getSqlQueryPreview), ensuring correct handling of whitespace, multi-line queries, and formatting.Code and logic clean-up:
SqlCellContent(such as pagination, sorting, and result name validation), delegating these responsibilities to the new components. [1] [2] [3] [4]These changes collectively improve the maintainability, readability, and user experience of the SQL cell UI.
Screen.Recording.2026-03-16.at.12.09.07.mov
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores