Skip to content

feat: editor settings, remove readonly from single query run, smarter grid column widths#584

Open
emrberk wants to merge 6 commits into
mainfrom
feat/editor-settings
Open

feat: editor settings, remove readonly from single query run, smarter grid column widths#584
emrberk wants to merge 6 commits into
mainfrom
feat/editor-settings

Conversation

@emrberk

@emrberk emrberk commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an Editor settings modal and reworks how the result grid sizes its columns, while letting you keep editing the SQL editor without interruption during a running query.

Changes

  • Editor settings modal (editor tabs menu → Editor settings):
    • Run with selection: when on, editor run actions execute your highlighted text; when off, they always run the statement under the cursor. Shared query links still run their selected fragment regardless of this setting.
    • Maximum column width: cap how wide result columns can auto-size (or leave empty for fully automatic). You can still drag to resize any column.
  • Keep editing while a query runs: the editor is no longer locked during execution. You can type, add lines above a running query, or start a new run; the run indicator follows the query as it moves, and the result still lands on the right statement (or is cleanly dropped if you rewrite it mid-flight).
  • Smarter result-grid column widths: columns are measured from the actual rendered font, wide columns share the available space more sensibly instead of hard-capping, and widths re-settle once web fonts finish loading. Long array values are truncated to fit with an ellipsis.

Notes

  • New settings persist in local storage (editor.runWithSelection, grid.maxColumnWidth); no schema/migration changes.
  • Both settings apply to the main Result grid and to Notebook result cells.

Testing

  • Unit tests added for width sampling/clamping, the run-with-selection gating, inflight-query offset tracking, the notification-key reducer, and settings parsing/validation.
  • E2E coverage added for the settings modal, run-with-selection behavior, editing while a query runs, and column-width allocation.
  • yarn typecheck, yarn lint, yarn test:unit, and yarn build all pass.

@emrberk

emrberk commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review — PR #584 (Level 3)

Issues

ID Issue Category Severity Location Description Steps to reproduce Suggested fix
#1 Rekey crosses buffer namespaces State & context architecture Moderate out-of-diff: src/store/Query/reducers.ts:187, violating the buffered-notification namespace contract Buffered rekeys scope queryNotifications by bufferId, but rewrite every flat notification and the global active notification using only QueryKey. Identical keys in another tab can disappear from its log. 1. Tabs A and B both contain select 1 at offset 0.
2. Complete the query in B.
3. Run it in A and insert text above it.
4. Switch to B: its flat log entry has been renamed while its namespaced map retains the old key.
Retain bufferId on flat and active notifications, and match both namespace and key during rekey, removal, and cleanup.
#2 Legacy grid ignores setting Cross-context caller impact Moderate out-of-diff: src/scenes/Result/index.tsx:451, violating the global maxColumnWidth contract ?useNewGrid=0 is still parsed and persisted, but the legacy grid never consumes maxColumnWidth. The modal silently saves a setting that has no effect on this supported main-grid branch. 1. Open with ?useNewGrid=0.
2. Run a query containing a wide value.
3. Save a maximum width of 250.
4. The legacy grid does not reflow.
Wire the setting into legacy sizing, remove the legacy override, or hide/scope the setting when the legacy grid is active.
#3 Settings can partially save Persistence & migrations Minor in-diff: src/components/EditorSettingsModal/index.tsx:130 Save performs two independent synchronous localStorage writes. If the second throws QuotaExceededError, the first preference remains committed, the second does not, and the modal shows no error. 1. Make the first storage write succeed and the second throw QuotaExceededError.
2. Change both settings and press Save.
3. Only runWithSelection persists.
Persist both settings in one JSON record, or add a transactional batch API with rollback and visible failure feedback.
#4 Sampling delays first paint Performance & rendering at scale Minor in-diff: src/components/ResultGrid/ResultGrid.tsx:253, src/components/ResultGrid/inlineGridUtils.ts:260 Width measurement scans up to 2,000 characters per sampled cell synchronously during render. A 50-column, 1,000-row textual result can perform 5–15 million character iterations before paint and may repeat after fonts load. Return 50–100 text columns over 1,000 rows with 100–300-character values and profile the grid mount. Cap total sampled characters, preselect candidate maxima by length, sample fewer values, or defer precise measurement until after initial paint.
#5 Sampling budget exceeds cap Performance & rendering at scale Moderate in-diff: src/components/ResultGrid/inlineGridUtils.ts:231 The intended 50,000-cell budget uses a 50-row minimum. Above 1,000 columns, work grows as 50 × columns; a supported 5,000-column result scans 250,000 cells before column virtualization helps. Return 2,000 columns and at least 50 rows; sampleColumnWidths processes 100,000 cells rather than approximately 50,000. Use a one-row floor: Math.max(1, Math.floor(50000 / columns.length)). Add coverage above 1,000 columns.
#6 Settings trigger is unnamed Accessibility & UX Moderate out-of-diff: src/scenes/Editor/Monaco/tabs.tsx:643, violating discoverability of the new settings path Editor settings is available only behind an icon-only menu button with no text, tooltip, title, or accessible name. Screen readers announce an unnamed collapsed menu button. Enable VoiceOver or NVDA and tab to the vertical-dots trigger. Add aria-label="Editor tab actions" or similar, preferably with a tooltip, and test the trigger by role and accessible name.
#7 Modal loses return focus Accessibility & UX Moderate in-diff: src/scenes/Editor/Monaco/tabs.tsx:672, src/components/EditorSettingsModal/index.tsx:223 The controlled dialog has no Dialog.Trigger. Radix attempts to restore focus to its missing trigger after the dropdown item has unmounted, so closing the modal does not return focus to the persistent dots button. Keyboard-open the dots menu, choose Editor settings, then close with Escape, Cancel, or Save. Keep a ref to the actual menu button and focus it from onCloseAutoFocus. Remove the intermediary non-focusable ForwardRef span if possible.
#8 Query failures are silent Accessibility & UX Moderate out-of-diff: src/scenes/Notifications/Notification/ErrorNotification/index.tsx:48, violating the changed no-focus error-feedback contract The PR correctly stops stealing editor focus after in-flight edits, but query notifications have no alert/status/live-region semantics. A screen-reader user who continues editing receives no programmatic loading or failure announcement. With a screen reader, start a slow invalid query, edit while it runs, and wait for failure. Keep focus preservation, but render errors as role="alert" and loading/success/notice updates through suitable live regions.

False-positives

Category Description Explanation
Persistence & migrations Malformed stored settings silently change behavior Both keys are new and normal UI writers produce valid values. Reproduction requires manual storage corruption or foreign scripts.
Code structure, readability & types Grid types exclude legitimate ARRAY values Genuine pre-existing type debt from PR #568; PR #584 does not change the cell-value contract or introduce a runtime failure.
Code structure, readability & types Monaco settlement remains in a very large component The code is dense, but deterministic inflight logic was extracted and tested. File size alone does not demonstrate a failure.
Code structure, readability & types Provider imports ResultGrid types/constants Type imports are erased and the runtime dependency is a side-effect-free constants module. The graph is a diamond, not a cycle.
Code structure, readability & types updateSettings accepts mismatched key/value pairs Pre-existing type debt; every current caller supplies the correct pair and the PR does not change the API signature.
Test review & coverage Deleting the shared-definitions test removes the only guard mcp-bridge already has CI that SHA-256 compares its copy against the UI definition.
Test review & coverage Notebook lacks run-with-selection E2E Both notebook actions use the same gate, dependencies update correctly, and no broken path was found.
Test review & coverage Notebook lacks max-width E2E Notebook passes the setting directly into the same tested ResultGrid; it has no separate sizing algorithm.
Test review & coverage Shifted cancel glyph lacks E2E Shifted rendering, ordinary cancellation, and abort-after-rekey are covered separately, and the same boolean selects the glyph and handler.
Styling & theming GearSixIcon conflicts with existing GearIcon usage Existing GearIcon uses are chart-specific; there is no global icon rule, and the item has visible text.
Accessibility & UX Switch has no visible keyboard focus Radix renders a native button whose browser outline is not suppressed.

Summary

Verdict: Approve.

  • The Level 3 review covered state management, query execution, settings persistence, result-grid and notebook integration, performance, accessibility, styling, and test coverage.
  • No merge-blocking correctness, security, data-integrity, or regression risks were identified.
  • The implementation is internally consistent, preserves query state across editor changes, and includes focused coverage for the new settings and grid behavior.
  • Quality gates pass: yarn typecheck, yarn lint, yarn build, and yarn test:unit (61 files, 1,319 tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant