Perf/UX: Virtualised tables, per-tab view state, JSON/BLOB viewers, and safer VS Code integration #5
Conversation
…ble rendering - Added virtualization support for large tables to improve performance and responsiveness. - Introduced caching mechanisms for row counts, table info, and foreign keys to reduce redundant database queries. - Enhanced table rendering logic to handle pagination and display row counts dynamically. - Updated UI elements to reflect the new data structure and improve user experience. - Refactored table row and cell rendering functions for better maintainability and performance.
- Implemented new context menu items for viewing and copying blob data in Base64 and Hex formats. - Enhanced context menu visibility logic based on blob presence in cells. - Added functions to handle blob viewing, copying as Base64, and copying as Hex. - Created a blob viewer dialog for displaying images and hex dumps of blob data. - Introduced CSS styles for blob viewer dialog and blob cell rendering. - Updated event handling to support blob download requests and responses. - Implemented logic to manage blob data in memory for efficient access and display.
WalkthroughAdds client-side large-table virtualization, per-tab persistent view state, JSON/Blob viewers with context‑menu actions and downloads, keyboard shortcuts for refresh/hard‑reload, delegated resizing with persisted sizes, frontend state APIs, and backend in‑memory caches with invalidation and blob download handling. Changes touch webview UI, CSS, state modules, resizing, context menu, and the extension backend. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Webview
participant StateModule as State
participant ViewerDialog as Viewer
participant Extension as ExtensionHost
participant FS as FileSystem
User->>Webview: Right-click cell -> request view or copy
Webview->>StateModule: captureCurrentDataViewState()
Webview->>Webview: detect JSON/blob, build menu
User->>Webview: select "View Blob" / "View JSON"
Webview->>ViewerDialog: showBlobViewerDialog / showJsonViewerDialog
ViewerDialog->>Extension: postMessage(downloadBlob, meta)
Extension->>FS: validate & read bytes
FS-->>Extension: return bytes or error
Extension->>Webview: postMessage(blobData or error)
ViewerDialog->>User: render preview or trigger download/save
sequenceDiagram
participant User
participant TableRenderer as Table
participant VirtualEngine as Virtual
participant DOM
User->>Table: Open large table
Table->>Virtual: decide virtualization (threshold)
Virtual->>Table: enable virtual mode, compute metrics
Virtual->>DOM: render spacers + visible row subset
DOM->>User: display partial rows
User->>DOM: scroll
DOM->>Virtual: scroll event -> compute visible range
Virtual->>DOM: virtualRender() update visible rows
Virtual->>Table: persist scroll/rowHeights via setTabViewState
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to review closely:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
media/css/30-components/confirm-dialog.css (1)
62-107: Duplicate CSS rule blocks should be removed.Lines 62-107 duplicate the rules already defined at lines 3-61 (
.confirm-dialog-overlay,.confirm-dialog-title,.confirm-dialog-buttons,.confirm-dialog-btn,.confirm-dialog-btn-cancel,.confirm-dialog-btn-confirm). This appears to be a copy/paste artefact.Remove the duplicate block to reduce file size and avoid maintenance confusion:
-/* ===== CONFIRM DIALOG ===== */ -.confirm-dialog-overlay { - position: fixed; - top: 0; - left: 0; - width: 100%; - height: 100%; - background: rgba(0, 0, 0, 0.5); - display: flex; - align-items: center; - justify-content: center; - z-index: 10000; -} - -.confirm-dialog-title { - font-size: 18px; - font-weight: 600; - margin: 0 0 20px; - padding-bottom: 12px; - border-bottom: 1px solid var(--vscode-widget-border); -} -.confirm-dialog-buttons { - display: flex; - gap: 10px; - justify-content: flex-end; -} -.confirm-dialog-btn { - padding: 6px 14px; - border: none; - border-radius: 2px; - cursor: pointer; - font-size: 13px; -} -.confirm-dialog-btn-cancel { - background: var(--vscode-button-secondaryBackground); - color: var(--vscode-button-secondaryForeground); -} -.confirm-dialog-btn-cancel:hover { - background: var(--vscode-button-secondaryHoverBackground); -} -.confirm-dialog-btn-confirm { - background: var(--vscode-inputValidation-errorBackground); - color: var(--vscode-errorForeground); -} -.confirm-dialog-btn-confirm:hover { - background: var(--vscode-inputValidation-errorBorder); -}media/context-menu.js (1)
88-91: Check Unicode character in FK menu item icon.Line 89 contains
<span class="icon">�</span>which appears to be a replacement character (U+FFFD) rather than an intended icon. This may display incorrectly. Consider using an emoji like 🔗 or a proper icon.<div class="context-menu-item context-menu-item-fk" data-action="navigate-foreign-key" style="display: none;"> - <span class="icon">�</span> + <span class="icon">🔗</span> <span>Query Referenced Row</span> </div>
🧹 Nitpick comments (11)
media/query-editor-enhanced.js (2)
566-584: Duplicate debug messages for helper functions.The debug output for
window.refreshQueryEditor()appears twice: once at lines 570-571 and again at line 582. Consider removing one to avoid redundant console output.if (window.debug) { window.debug.debug("QueryEditor", "Global helper functions loaded"); window.debug.debug( "QueryEditor", " - window.refreshQueryEditor() - Refresh the editor if unresponsive" ); window.debug.debug( "QueryEditor", " - window.focusQueryEditor() - Focus the editor" ); window.debug.debug( "QueryEditor", " - window.getQueryEditorStatus() - Get editor status" ); - window.debug.debug( - "QueryEditor", - " - window.refreshQueryEditor() - Manual refresh helper" - ); }
614-624: Duplicate debug message for emergency function.Similar to above,
window.emergencyFixEditor()is documented twice (lines 618 and 622). Consider consolidating.if (window.debug) { window.debug.debug("QueryEditor", "Emergency functions loaded"); window.debug.debug( "QueryEditor", " - window.emergencyFixEditor() - Nuclear editor restoration" ); - window.debug.debug( - "QueryEditor", - " - window.emergencyFixEditor() - Emergency recovery helper" - ); }src/extension.ts (2)
37-39: Consider extracting duplicated helper functions.The
isDatabaseFilehelper is defined identically in three places within this file (lines 37-39, 86-88, and 183). Consider extracting it to a module-level utility function to reduce duplication and improve maintainability.+const DATABASE_EXTENSIONS = ['.db', '.sqlite', '.sqlite3']; + +function isDatabaseFile(uri: vscode.Uri): boolean { + return DATABASE_EXTENSIONS.some(ext => uri.fsPath.toLowerCase().endsWith(ext)); +} + +function isDatabasePath(p: string): boolean { + return DATABASE_EXTENSIONS.some(ext => p.toLowerCase().endsWith(ext)); +} + export function activate(context: vscode.ExtensionContext) {Also applies to: 86-88, 183-183
94-96: Type assertion on tab input is pragmatic but fragile.The
anycast onactiveTab?.input(lines 95-96 and 190-191) is necessary because VS Code's Tab API doesn't expose a strongly-typed input property. This is acceptable, but consider adding a brief comment explaining why the cast is needed for future maintainers.Also applies to: 190-192
src/databaseEditorProvider.ts (1)
571-583: Background fetch could send stale updates if user switches tables quickly.The async row count fetch (and similarly tableInfo at lines 608-617) posts a message to the webview without checking if the user has switched to a different table. If the user navigates away before the count resolves, the UI could receive an update for the wrong table.
Consider adding a check before posting the message, or having the webview validate the
tableNamematches the active table before applying updates.media/main.js (1)
372-383: Inconsistent cache storage patterns may cause lookup failures.The result cache lookup checks three different patterns:
window._resultCache(Map),state.tableCache(Map), andstate.tableCache[key](object). This suggests the cache storage evolved over time and could lead to data being stored in one format but not found because a different pattern is checked.Consider consolidating to a single cache storage pattern for consistency.
media/state.js (2)
100-173: Consider extractingpruneToLastKeysto avoid duplication.The
pruneToLastKeyshelper is defined inline here and again ininitializeState(lines 427-442). Extract it to a module-level utility to reduce code duplication and ensure consistent behaviour.+// Module-level utility for pruning large objects +function pruneToLastKeys(obj, maxKeys) { + try { + const keys = Object.keys(obj || {}); + if (keys.length <= maxKeys) { + return obj || {}; + } + const keep = keys.slice(-maxKeys); + const next = {}; + keep.forEach((k) => { + next[k] = obj[k]; + }); + return next; + } catch (_) { + return obj || {}; + } +} + function setTabViewState(tabKey, patch, options = {}) { // ... existing validation ... - // Prevent persisted state from ballooning (large objects make vscode.setState slow). - const pruneToLastKeys = (obj, maxKeys) => { - // ... inline definition ... - }; + // Use module-level pruneToLastKeys
210-225: Timer reference should be cleared on module unload to prevent leaks.The
persistTimeris a module-level variable that could potentially retain references if the webview is disposed but JavaScript context isn't fully cleaned up. Consider clearing it inresetStateas well.function resetState() { + // Clear any pending persistence timer + if (persistTimer) { + clearTimeout(persistTimer); + persistTimer = null; + } currentState = { // ... rest of reset logicmedia/table.js (2)
65-81: Stash size limit may be too small for power users with many tabs.The
maxEntries = 25limit for table data stash could cause data loss if users have many tabs open. Consider increasing this or implementing LRU eviction based on access time rather than insertion order.- const maxEntries = 25; + // Allow more entries for users with many open tabs + const maxEntries = 50;
353-371: Virtualisation stash cleanup should be more aggressive.The virtualisation stash stores potentially large page data. Unlike the table data stash, there's no size limit here. Consider adding similar eviction logic to prevent unbounded memory growth.
if (virtualize) { try { const stash = getVirtualizationStash(); + // Prevent unbounded growth + const maxEntries = 25; + while (stash.size > maxEntries) { + const firstKey = stash.keys().next().value; + if (firstKey === undefined) break; + stash.delete(firstKey); + } stash.set(tableId, { // ... payload });media/events.js (1)
1307-1328: Consider usingCSS.escapepolyfill for broader compatibility.The
CSS.escapefallback only escapes double quotes, which may not handle all special characters in table names (e.g., brackets, backslashes).// CSS.escape isn't supported everywhere; fallback to a cheap escape. const safeKey = - typeof CSS !== "undefined" && CSS.escape ? CSS.escape(tableKey) : tableKey.replace(/"/g, '\\"'); + typeof CSS !== "undefined" && CSS.escape + ? CSS.escape(tableKey) + : tableKey.replace(/[!"#$%&'()*+,./:;<=>?@[\\\]^`{|}~]/g, '\\$&');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(2 hunks)media/context-menu.js(18 hunks)media/css/30-components/confirm-dialog.css(1 hunks)media/css/30-components/tables.css(4 hunks)media/debug-ui.js(2 hunks)media/dom.js(2 hunks)media/events.js(28 hunks)media/main.js(11 hunks)media/notifications.js(1 hunks)media/query-editor-enhanced.js(2 hunks)media/resizing.js(4 hunks)media/state.js(7 hunks)media/table.js(16 hunks)src/databaseEditorProvider.ts(11 hunks)src/extension.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/extension.ts (1)
src/databaseEditorProvider.ts (1)
DatabaseEditorProvider(7-1073)
media/debug-ui.js (1)
media/events.js (1)
isCtrlOrCmd(147-147)
media/dom.js (2)
media/state.js (1)
currentState(8-37)media/table-tabs.js (1)
currentState(691-691)
media/state.js (5)
media/events.js (10)
currentState(405-405)currentState(493-494)currentState(1747-1747)currentState(2246-2246)currentState(2395-2395)state(239-239)state(271-271)state(621-621)vs(668-671)vs(2696-2696)media/table.js (7)
currentState(1887-1888)vs(863-905)vs(951-951)vs(1277-1277)vs(1327-1327)vs(1570-1570)vs(1628-1628)media/dom.js (1)
currentState(194-195)media/enhanced-diagram.js (1)
currentState(325-326)media/main.js (12)
state(58-58)state(187-190)state(264-267)state(377-377)state(469-472)state(513-514)state(532-533)state(552-553)state(639-639)vs(228-231)vs(303-306)vs(324-327)
🪛 LanguageTool
CHANGELOG.md
[style] ~25-~25: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ers - Cell rendering is truncated for very large values while preserving originals for e...
(EN_WEAK_ADJECTIVE)
🔇 Additional comments (52)
media/dom.js (1)
237-258: LGTM! Proper guard for VS Code API availability.The defensive check
window.vscode && typeof window.vscode.postMessage === "function"correctly prevents runtime errors when the webview context lacks the VS Code API. This aligns with the PR's security/robustness goals.media/debug-ui.js (1)
142-168: LGTM! Robust keyboard shortcut handling.The reworked handler properly:
- Uses capture phase to intercept events early
- Checks multiple key representations (
e.codeande.key) for cross-browser/layout compatibility- Excludes Alt to prevent shortcut conflicts
- Follows the same
isCtrlOrCmdpattern used inmedia/events.jsmedia/css/30-components/confirm-dialog.css (1)
186-226: LGTM! Well-structured viewer dialog styles.The new JSON and blob viewer dialog styles appropriately:
- Constrain dimensions for responsiveness (
max-width,max-height,85vh)- Style blob-specific elements (image preview, hex dump, notes)
- Use VS Code CSS variables for theme consistency
CHANGELOG.md (1)
3-35: LGTM! Comprehensive changelog entries.The [Unreleased] section thoroughly documents all major changes in this PR with clear categorisation and helpful descriptions. The keyboard shortcut documentation and feature descriptions align well with the implementation.
media/css/30-components/tables.css (3)
101-154: LGTM! Well-structured blob and virtualization styles.The blob cell styling provides a clean layout for thumbnails and labels, and the virtual spacer/loading row styles correctly hide visual elements for placeholder rows during virtualized rendering.
167-221: LGTM! Layout adjustments support horizontal resizing and focused search.Switching
.table-scroll-containertodisplay: blockcorrectly prevents flex child stretching that interferes with horizontal overflow. The focused search mode styles provide a clean UX by temporarily hiding pagination controls.
43-53: Thewidth: max-contentandtable-layout: fixedcombination is intentional and documented.The inline comment at line 44 explicitly states the purpose: "Allow tables to grow wider than the container so column resizing can expand horizontally." This design allows the table to expand beyond its container while maintaining fixed column widths determined by the first row. The combination works correctly for the intended resizing use case.
media/context-menu.js (8)
48-69: LGTM! Well-structured context menu items for JSON/blob actions.The new menu items are appropriately hidden by default and include proper separators for visual grouping. The implementation follows the existing patterns in the file.
139-169: LGTM! Clean refactor for cell-specific context menu handling.The new
showContextMenuForCellfunction properly encapsulates the cell validation and schema table check, making it reusable for both delegated and per-cell event handlers.
1008-1057: LGTM! Appropriate size limits for blob operations.The copy/download size limits (5MB base64, 1MB hex, 20MB download) are reasonable to prevent UI hangs and message-passing issues. The error messages clearly inform users when limits are exceeded.
1259-1386: LGTM! Proper resource management in blob viewer dialog.The implementation correctly:
- Creates and revokes object URLs to prevent memory leaks
- Removes the Escape key handler on dialog close
- Falls back to browser download when VS Code API is unavailable
1388-1438: LGTM! Safe JSON formatting with syntax highlighting.The
escapeHtmlForInnerHtmlcall before regex tokenisation prevents XSS. The token regex correctly identifies JSON structural elements for colour-coding whilst preserving escaped content.
2341-2398: LGTM! Virtualization-aware foreign key highlighting.The implementation correctly searches in-memory data when virtualisation is enabled, scrolls to render the target row, and falls back to DOM traversal for non-virtualised tables. This ensures FK navigation works regardless of rendering mode.
2268-2302: LGTM! Consistent VS Code API guard for FK navigation.The postMessage call is properly guarded with
window.vscode && typeof window.vscode.postMessage === "function", consistent with the PR's security improvements across all webview scripts.
2527-2546: Confirm thatwindow.showContextMenusignature change is properly integrated.The public API
window.showContextMenunow points toshowContextMenuForCell(e, cell)instead of the coordinate-basedshowContextMenuAt(x, y). Verification shows media/events.js (the sole external caller) already uses the new signaturewindow.showContextMenu(e, cell)correctly. No external code calling the old coordinate-based signature was found.media/notifications.js (1)
163-170: LGTM!The new keyboard shortcut documentation entries are well-structured and follow the existing pattern. The shortcuts align with the new refresh and hard reload functionality introduced in this PR.
media/resizing.js (3)
25-62: Good use of event delegation for performance.The delegated mousedown listener pattern is a solid performance improvement over attaching individual listeners to every cell and row. The
data-resize-delegatedattribute guard prevents duplicate listener attachment on re-initialisation.
130-211: LGTM!The delegated resize helper functions are well-structured and consistent. They properly prevent default behaviour, manage resize state, and provide appropriate visual feedback.
336-357: Row height persistence is correctly implemented for the pagination model used.The row heights are persisted using
data-row-index, which represents the absolute row position in the dataset—not a viewport-relative index. TheglobalIndexis computed asstartIndex + localIndex, wherestartIndexis based on pagination position(currentPage - 1) * pageSize. When users navigate between pages, rows retain their absolute positions and thus their persisted heights apply correctly.Additionally, when rows are inserted or deleted,
data-row-indexvalues are properly maintained through increments and decrements to preserve height associations with the correct rows.src/databaseEditorProvider.ts (3)
391-475: LGTM!The
handleDownloadBlobRequestimplementation has robust input validation, usespath.basenameto prevent path traversal, and requires user confirmation viashowSaveDialog. Error handling is comprehensive with appropriate feedback to the webview.
136-149: LGTM!The
reloadDatabasehandler correctly closes the cached connection before re-opening, ensuring a true "hard reload" from disk. Thefinallyblock ensures the webview receives thedatabaseReloadedmessage regardless of success or failure.
232-232: LGTM!The CSP update to include
blob:inimg-srcis necessary for the new image BLOB thumbnail feature and represents a minimal, appropriate relaxation of the security policy.media/main.js (3)
18-26: Deferred global assignment pattern is appropriate.Using
setTimeout(..., 0)ensures the global function assignments occur after all scripts have been loaded and parsed, preventing race conditions during module initialisation.
435-458: Good optimisation: lazy schema loading.Deferring schema fetches until the Schema tab is active reduces unnecessary backend requests and improves initial load performance. The defensive fallback chain (DOM → state → catch fallback) handles edge cases during initialisation.
179-181: LGTM!Consistent use of
captureCurrentDataViewState()at the start of tab operations ensures the current view state (scroll position, search term, etc.) is preserved before navigating away. This provides a good user experience when switching between tabs.Also applies to: 261-263, 321-323
media/state.js (6)
39-58: LGTM - Well-structured default tab view state.The default tab view state function provides sensible defaults with proper fallback for
pageSize. The inclusion ofupdatedAttimestamp is useful for debugging and potential conflict resolution.
60-78: LGTM - Robust normalisation handles legacy and malformed data.The function correctly handles arrays, strings (legacy format), objects, and null/undefined values, filtering out invalid entries with
filter(Boolean).
183-208: LGTM - Proper exclusion of non-serializable data from persistence.The function correctly excludes
tableCache(which containsMapobjects) and filters out result tabs that depend on in-memory data. The fallback logic foractiveTable/selectedTableis sound.
232-270: LGTM - Flexible persistence options with proper guards.The enhanced
updateStatefunction withpersistStateoption provides good flexibility. The defensive checks forwindow.vscodeavailability prevent errors in non-VS Code contexts.
405-456: Verify that pruning on load doesn't discard recently-used sizing data.The pruning logic uses
slice(-maxKeys)which keeps the last N keys based on insertion order. If a user has heavily customised columns early in the object, those customisations could be lost. Consider whether pruning based onupdatedAttimestamps or usage frequency would be more appropriate.This is likely acceptable given the generous limits (2000 columns, 500 rows), but verify this matches user expectations for persisted state behaviour.
593-598: LGTM - Proper module and window exports for cross-module access.The new per-tab view state helpers are correctly exported both via
module.exportsandwindowglobals, maintaining consistency with existing patterns.Also applies to: 609-613
media/table.js (11)
14-28: LGTM - Sensible virtualisation thresholds and rendering limits.The configuration values are well-documented and conservative. The 300-row/12000-cell threshold provides a good balance between performance and simplicity. The 64KB BLOB thumbnail limit prevents UI jank from large images.
30-47: LGTM - Efficient HTML escaping implementation.The
escapeHtmlFastfunction correctly escapes all necessary HTML special characters. The switch-case approach is performant for this use case.
83-106: LGTM - Comprehensive BLOB normalisation handles various input formats.The function correctly handles
Uint8Array,ArrayBuffer, numeric arrays, and Node.js Buffer serialisation formats. This ensures robust BLOB handling across different data sources.
108-172: LGTM - Thorough image MIME detection with proper magic byte checks.The implementation correctly identifies PNG, JPEG, GIF, WebP, BMP, and SVG formats using their magic bytes/signatures. The SVG detection with UTF-8 fallback is a nice touch.
216-237: LGTM - Virtualisation decision logic is sound.The function correctly excludes schema tables, respects explicit
virtualizeoptions, and uses both row count and cell count thresholds for the decision.
654-691: LGTM - Robust cell rendering with proper truncation and BLOB handling.The cell rendering correctly handles NULL, BLOB, and text values with appropriate truncation for display performance. The
data-original-truncatedattribute enables proper editing workflow.
818-944: Well-implemented virtualisation initialisation with proper state management.The
initializeVirtualTablefunction correctly:
- Guards against double-initialisation
- Retrieves and clears stashed data
- Attaches throttled scroll listeners
- Measures actual row heights for accurate virtualisation
- Uses double RAF for reliable timing
1088-1171: LGTM - Efficient virtual rendering with spacer-based scrolling.The
virtualRenderfunction correctly:
- Handles empty state
- Uses binary search for row lookup
- Calculates overscan for smooth scrolling
- Uses spacer rows for scroll height preservation
- Synchronises pinned columns, widths, and heights after render
1173-1191: Binary search implementation is correct.The
findRowAtOffsetfunction correctly implements binary search on prefix sums. The bounds handling and edge cases are properly addressed.
1521-1540: LGTM - Pinned column state is correctly persisted per-tab.The pin state is saved using column names (not indices), which is more robust across schema changes. The error handling with try-catch prevents UI disruption.
191-199: Stack overflow concern is not applicable here.The
uint8ToBase64function is only called when processing image thumbnails that are explicitly limited to 64KB (BLOB_THUMB_MAX_BYTES). With this constraint, the function processes at most 8 chunks of 8192 bytes each, making the stack overflow risk negligible. TheString.fromCharCode.apply()call receives at most 8192 arguments, far below the practical limits of modern JavaScript engines (typically 65536+ arguments). No changes needed.media/events.js (10)
52-54: Good use of capture phase for keyboard shortcuts.Using capture phase ensures shortcuts work even when focused components (e.g., Monaco editor) stop event bubbling. The
removeEventListenerbeforeaddEventListenerprevents duplicate handlers on reinitialisation.
146-235: Comprehensive keyboard shortcut handling with proper guards.The shortcuts are well-documented and handle edge cases:
- Ctrl/Cmd + Enter: execute query
- Ctrl/Cmd + K: clear query
- Ctrl/Cmd + F: focus search with multi-panel fallback
- "/": alternative search focus
- Ctrl/Cmd + Shift + R: refresh view
- Ctrl/Cmd + Alt + R: hard reload
The input element check for "/" prevents interference when typing.
237-268: LGTM - Refresh function correctly handles both database info and active table.The function properly checks for result tabs before attempting refresh and uses the per-tab view state for pagination.
891-968: LGTM - Async row count handling updates UI progressively.The
handleTableRowCountfunction correctly updates all relevant UI elements when async row counts arrive:
- Total rows attribute
- Records info stat
- Visible rows label
- Pagination controls with re-attached listeners
1079-1112: LGTM - View state capture correctly gathers all relevant UI state.The function captures page, pageSize, searchTerm, and scroll positions. Using
persistState: 'none'avoids redundant persistence during tab switches.
1114-1305: Robust view state restoration with retry mechanism.The
applyTabViewStateToWrapperfunction handles:
- Pinned columns (by name for robustness)
- Column widths with colgroup sync
- Row heights
- Search term with filter application
- Scroll position with retry mechanism
The retry mechanism (up to 6 attempts, 50ms apart) handles cases where layout hasn't completed. The
data-viewstate-restoring-scrollattribute prevents scroll events during restoration.
1829-1844: Good practice: Normalise wrapper element before initialisation.The wrapper normalisation ensures events are attached to the correct element regardless of whether a container, wrapper, or child element is passed.
1919-1955: LGTM - Scroll position persistence is properly throttled.The 400ms debounce prevents excessive state updates during scrolling. The
data-viewstate-restoring-scrollcheck prevents feedback loops during restoration.
2032-2124: Well-implemented delegated event handling for cell editing.Using event delegation instead of per-cell listeners significantly reduces memory overhead and handles dynamically-added rows automatically. The handler correctly:
- Checks for editable cells
- Handles dblclick, keydown (Enter/F2), and focusout
- Manages save/cancel buttons
2693-2771: Comprehensive virtualisation support in delta updates.The
handleTableDataDeltafunction correctly handles virtualised tables by:
- Modifying in-memory
pageDatainstead of DOM- Converting global indices to local indices
- Applying deletes from bottom to top
- Applying inserts from top to bottom
- Updating metadata and triggering virtual refresh
This ensures delta updates work correctly for both virtualised and non-virtualised tables.
…e reload confirmation dialog
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/databaseEditorProvider.ts (2)
172-216: Hard reload should invalidate caches too (currently it only closes connections).
If the user triggers “hard reload DB”, they’ll expect fresh counts/metadata. Right now${db}:${table}caches survive the reload and can serve stale data.case 'reloadDatabase': // Hard reload: close cached connections and re-open from disk. + this.invalidateCachesForDatabase(document.uri.fsPath); this.closeConnection(document.uri.fsPath); void this.handleDatabaseInfoRequest(webviewPanel, document.uri.fsPath, e.key) .catch(() => { // errors are already surfaced via handleDatabaseInfoRequest })
593-663:foreignKeysCachenever “sticks” for FK-less tables.
Because you key offforeignKeys.length === 0, tables with no foreign keys will re-query every time. PreferMap.has(key)to distinguish “uncached” from “cached empty”.- const metaKey = `${databasePath}:${tableName}`; - let foreignKeys: any[] = this.foreignKeysCache.get(metaKey) || []; - if (foreignKeys.length === 0) { + const metaKey = `${databasePath}:${tableName}`; + let foreignKeys: any[]; + if (this.foreignKeysCache.has(metaKey)) { + foreignKeys = this.foreignKeysCache.get(metaKey) || []; + } else { try { foreignKeys = await dbService.getForeignKeys(tableName); this.foreignKeysCache.set(metaKey, foreignKeys); } catch (err) { this.debugWarn('getForeignKeys', `Failed to fetch foreign keys for ${tableName}:`, err); foreignKeys = []; + this.foreignKeysCache.set(metaKey, foreignKeys); } }
🧹 Nitpick comments (3)
media/events.js (1)
150-239: Global shortcuts should ignore non-Monaco text inputs to avoid surprising actions.
Because this runs in capture phase,Ctrl/Cmd+Enter, refresh, etc. can trigger even when the user is editing a cell input / search field. Consider early-returning whene.targetis aninput/textarea/[contenteditable](except Monaco).src/databaseEditorProvider.ts (1)
435-519: Blob download: consider size limits and avoid returning full save path to the webview.
Decoding base64 into aBuffercan spike memory for very large blobs; also, the UI likely only needs{ bytes, mime }(returningtarget.fsPathis avoidable).src/extension.ts (1)
36-75: Nice move tovscode.openWithfor the custom editor; consider de-duplicatingisDatabaseFile.
You defineisDatabaseFile(...)in multiple commands—worth extracting to a small local helper to keep behaviours consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
media/events.js(28 hunks)src/databaseEditorProvider.ts(14 hunks)src/extension.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
media/events.js (3)
media/state.js (2)
tab(92-92)options(233-237)media/table-tabs.js (4)
state(979-979)state(1136-1136)state(1198-1198)state(1232-1232)media/debug-ui.js (1)
isCtrlOrCmd(146-146)
🔇 Additional comments (5)
media/events.js (2)
37-59: Capture-mode keyboard handler is a good call, but ensure it doesn’t fire twice across hot reloads.
You’re guarding initialisation, and removing/re-adding the keydown handler withcapture=trueis sensible for Monaco focus. Just double-check no other module attacheshandleGlobalKeyboardseparately (capture vs bubble can lead to “double shortcut” symptoms).
2153-2453: Virtualisation-first initialisation + scroll/search persistence looks solid.
Normalising wrapper inputs, initialising virtualisation early, and throttling scroll persistence should pay off for large tables. Delegated cell editing listeners are also a big win for DOM listener count.src/databaseEditorProvider.ts (1)
27-69: Cache invalidation helpers are the right direction.
The table/database-scoped invalidation methods make follow-up changes (updates/deletes/external changes) much safer and easier to reason about.src/extension.ts (2)
190-238: Active DB path resolution for WAL checkpoint is clearer and more robust now.
The helper-based path discovery reads well and should behave better across different tab/editor focus states.
77-162: Replaceanycast with properinstanceofchecks for tab input types.The current code uses duck-typing with
anycast to accessinput.uri. Per VS Code API documentation, only some TabInput types (TabInputText, TabInputCustom, TabInputNotebook, TabInputTextDiff) expose auriproperty; others (TabInputWebview, TabInputTerminal, internal UI tabs) do not.Replace the unsafe pattern:
const input: any = activeTab?.input; const inputUri: vscode.Uri | undefined = input && input.uri && typeof input.uri.fsPath === 'string' ? input.uri : undefined;With explicit instanceof checks:
const inputUri: vscode.Uri | undefined = activeTab?.input instanceof vscode.TabInputText ? activeTab.input.uri : activeTab?.input instanceof vscode.TabInputCustom ? activeTab.input.uri : undefined;This pattern appears twice in the file. Note: Your minimum VS Code version (^1.101.0) supports these TabInput types.
… and improve snapshot handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
media/events.js (2)
1614-1643: Scroll restoration retry logic is defensive but complex.The retry mechanism (up to 6 attempts with 50ms delay) handles async layout timing, and the 2-pixel tolerance (line 1623) accounts for sub-pixel rounding. Disabling smooth scrolling during restore (lines 1606-1610) and the restoration flag (line 1612) prevent recursive persistence. Whilst this works correctly, if timing issues arise in future, consider using a single
IntersectionObserverorResizeObservercallback instead of polling.
1159-1167: Cache management is consistent but lacks cleanup.The
window._tableMetaCache(Map) andwindow._resultCache(Map) are created lazily and updated correctly throughout the code. Foreign-key detection (lines 1855-1862) efficiently iterates cached metadata rather than re-parsing state. However, neither cache is ever pruned—if the user connects to a large database or runs many queries, these Maps will grow unbounded until the webview is destroyed.Consider adding a periodic cleanup or LRU eviction policy for
_resultCacheto cap memory usage, especially for long-lived webview sessions.Also applies to: 1194-1203, 1307-1321, 1827-1896, 1943-1946
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
media/events.js(28 hunks)
🔇 Additional comments (6)
media/events.js (6)
52-58: Keyboard handling improvements look solid.The capture-phase registration (line 54) ensures shortcuts work even when Monaco or other components stop bubbling, and the key detection logic (lines 151-153) using both
e.codeande.key.toLowerCase()is robust for international keyboard layouts. The new shortcuts (Ctrl+Shift+R refresh, Ctrl+Alt+R hard reload, / for search) include appropriate guards to prevent conflicts with text inputs (lines 206-210) and other modifier combinations (lines 224, 230).Also applies to: 150-239
421-499: Fallback reload snapshot security is well-designed.The snapshot deliberately excludes sensitive data:
encryptionKeyis omitted from the whitelist (lines 426-440) and explicitly deleted during normalisation (lines 476-482), whilst the query editor snapshot (lines 446-465) captures only view state (cursor position, scroll) without the actual query text. Additional defensive sanitisation at restore time (lines 555-561) ensures older snapshots can't leak secrets. This matches the user-facing message at line 312.Also applies to: 501-584
2204-2263: Search input focus/blur handling is correct.The deferred blur check (lines 2213-2224) using
setTimeout(..., 0)allows focus to move to the next element before collapsing the expanded search state, and theactiveElementguard (lines 2218-2221) prevents premature collapse when focus is still within the search input. Persisting the search term to view state (lines 2232-2239) and keeping focus after clearing (line 2251) provide a polished UX.
3036-3114: Virtualized table delta logic is correct.The index arithmetic handles deletes (bottom-to-top, lines 3048-3055), inserts (top-to-bottom with clamping, lines 3057-3074), and updates (lines 3076-3086) correctly. Deletes preserve indices by working backwards, inserts allow extension up to
pageSize(line 3065), and the clamp at line 3072 ensures end-of-page inserts work. Updating wrapper metadata (lines 3088-3108) and callingrefreshVirtualTable(lines 3110-3113) keeps the UI in sync.
104-112: VS Code API guards are consistent and safe.All
window.vscode.postMessagecalls are now protected withwindow.vscode && typeof window.vscode.postMessage === "function"guards. The fallback reload path (lines 283-308) gracefully degrades when the API is unavailable, and the debug logging insaveCellEdit(lines 2598-2603) andtryInitialConnection(lines 2782-2785) aids troubleshooting without throwing exceptions.Also applies to: 246-252, 278-280, 770-775, 802-807, 997-1024, 2588-2603, 2776-2780
2375-2467: Critical: Delegated event listeners are attached repeatedly.The guard check at line 2376 reads the flag but doesn't return early, so every call to
initializeTableEventsattaches fresh delegated listeners on the wrapper. After a few delta updates or tab switches, a single click will fire multiple handlers, leading to duplicate edits, save attempts, or other erratic behaviour.Apply this diff to add the missing early return:
// Delegated cell editing listeners (avoid attaching listeners to every cell). if (tableWrapper.getAttribute("data-cell-events-delegated") !== "true") { tableWrapper.setAttribute("data-cell-events-delegated", "true"); + } else { + // Listeners already attached; skip delegation setup. + return; + } tableWrapper.addEventListener("dblclick", (e) => {Likely an incorrect or invalid review comment.
1 participant
Perf/UX: Virtualised tables, per-tab view state, JSON/BLOB viewers, and safer VS Code integration
Summary
What's Included
Performance
Per-tab persistence
UX features
Backend reliability & integration
reloadDatabase) and supports BLOB downloads viashowSaveDialog(downloadBlob) (src/databaseEditorProvider.ts).vscode.openWith. Encrypted connect flow better targets the active DB editor and reconnects the webview (src/extension.ts).Security / robustness
window.vscode.postMessageusage across scripts to avoid runtime errors if the host API isn't available.img-src blob:for image previews (src/databaseEditorProvider.ts and relevant webview code).Changelog
Notes / Behaviour Changes
…until async counts return for large tables; UI will update viatableRowCountwhen ready.Files Touched (high level)
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.