-
Notifications
You must be signed in to change notification settings - Fork 0
feat(table): expand cell viewer + row multiline toggle; improved resize + width restore #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…table resizing and overflow handling
…nd/collapse functionality
WalkthroughAdds a cell-viewer dialog and context-menu actions for cell/row expansion, enables multiline row rendering with persisted heights, introduces pointer-event-based resizing with auto-scroll, and exposes table layout/multiline/overflow helper APIs on window; updates CSS and changelog. Changes
Sequence DiagramssequenceDiagram
participant User
participant ContextMenu as Context Menu
participant Table
participant Dialog as Cell Viewer Dialog
User->>ContextMenu: Right-click cell
ContextMenu->>ContextMenu: showContextMenuAt()
ContextMenu->>Dialog: Select "Expand Cell"
Dialog->>Table: Read cell value & metadata
Dialog->>Dialog: showCellViewerDialog(opts)
Dialog->>User: Render modal with full content + copy
User->>Dialog: Copy / Close
sequenceDiagram
participant User
participant ContextMenu as Context Menu
participant Table
participant Events as Event Handlers
participant Resizing as Resize Engine
User->>ContextMenu: Right-click row
ContextMenu->>ContextMenu: showContextMenuAt()
ContextMenu->>Table: Select "Expand Row"
Table->>Table: toggleRowMultiline()
Table->>Events: updateRowMultilineForRow(row)
Events->>Table: Recalculate heights & classes
Events->>Resizing: scheduleCellOverflowIndicators()
Resizing->>Table: Update overflow visuals
Table->>User: Row expands/collapses with multiline content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
media/resizing.js (1)
187-207:handleElis undefined — this will cause a ReferenceError.Line 194 references
handleEl, which does not exist instartColumnResize. This appears to be a copy-paste error fromstartColumnResizeFromHandle. Additionally, line 187 already callscaptureResizePointer(e, target), so line 194 is both erroneous and redundant.🐛 Proposed fix
if (header) { - captureResizePointer(e, handleEl); currentResizeTarget = /** `@type` {HTMLElement} */ (header); startWidth = header.offsetWidth;
🤖 Fix all issues with AI agents
In `@media/context-menu.js`:
- Around line 312-351: The expand-cell visibility logic only checks horizontal
overflow via cellContent.scrollWidth > cellContent.clientWidth, so update the
hasOverflow computation (used when deciding shouldShow for expandItem) to also
detect vertical clipping by checking cellContent.scrollHeight >
cellContent.clientHeight (apply the same small tolerance like +2 if desired);
adjust the hasOverflow expression in the block that reads
getCellRawValue(currentCell) and computes shouldShow so vertical overflow
triggers showing the expand action.
In `@media/table.js`:
- Around line 289-314: The updateCellOverflowIndicators currently only removes
the "cell-overflow" class; modify it to detect overflow for each td.data-cell
(e.g., compare scrollWidth>clientWidth or scrollHeight>clientHeight on the
HTMLElement) and add the "cell-overflow" class when overflow is detected and
remove it when not; keep using the same selector and guard for instanceof
HTMLElement, and leave scheduleCellOverflowIndicators and its
__overflowIndicatorRaf logic as-is to throttle updates; also ensure a
corresponding CSS rule for .cell-overflow exists elsewhere so the visual
indicator is applied.
🧹 Nitpick comments (2)
media/table.js (2)
14-17: Setting render limits toInfinityremoves truncation safeguards.While this enables full cell content display, it could impact rendering performance on tables with very large text values (e.g., JSON blobs, long descriptions). Consider whether a high but finite limit (e.g., 50,000 characters) might provide a reasonable balance.
302-314: Minor: Consider usingnullinstead of0for cleared RAF ID.Setting
__overflowIndicatorRafto0works due to falsiness, butnullorundefinedwould be more semantically clear for "no pending frame".♻️ Optional refinement
tableAny.__overflowIndicatorRaf = requestAnimationFrame(() => { - tableAny.__overflowIndicatorRaf = 0; + tableAny.__overflowIndicatorRaf = null; updateCellOverflowIndicators(table); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.mdmedia/context-menu.jsmedia/css/30-components/confirm-dialog.cssmedia/css/30-components/tables.cssmedia/events.jsmedia/resizing.jsmedia/table.js
🧰 Additional context used
📓 Path-based instructions (4)
CHANGELOG.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update
CHANGELOG.mdwith each release documenting breaking changes and migration instructions
Files:
CHANGELOG.md
{src/**/*.ts,media/**/*.js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
{src/**/*.ts,media/**/*.js}: Use parameterized queries and input validation to prevent SQL injection in database operations
All database operations must be local with no network requests or telemetry
Files:
media/resizing.jsmedia/table.jsmedia/events.jsmedia/context-menu.js
media/css/**/*.css
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
media/css/**/*.css: Use modular CSS inmedia/css/with organized sections and component-based structure in subfolders like30-components/
Always add new styles to the correct file and preserve import order in CSS
Use VS Code theme variables (--vscode-*) for all colors and backgrounds in CSS
Files:
media/css/30-components/confirm-dialog.cssmedia/css/30-components/tables.css
{src/databaseService.ts,media/table.js,media/main.js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use LIMIT/OFFSET for pagination, virtual scrolling for large tables, and clean up event listeners/resources
Files:
media/table.js
🧠 Learnings (2)
📚 Learning: 2025-12-13T21:32:29.999Z
Learnt from: CR
Repo: Bowlerr/sqlite-intelliview-vscode PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-13T21:32:29.999Z
Learning: Applies to CHANGELOG.md : Update `CHANGELOG.md` with each release documenting breaking changes and migration instructions
Applied to files:
CHANGELOG.md
📚 Learning: 2025-12-13T21:32:29.999Z
Learnt from: CR
Repo: Bowlerr/sqlite-intelliview-vscode PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-13T21:32:29.999Z
Learning: Applies to {src/databaseService.ts,media/table.js,media/main.js} : Use LIMIT/OFFSET for pagination, virtual scrolling for large tables, and clean up event listeners/resources
Applied to files:
media/resizing.jsmedia/table.js
🧬 Code graph analysis (2)
media/table.js (1)
media/resizing.js (6)
headers(502-502)width(419-419)col(349-349)rect(64-64)rect(161-161)rect(605-605)
media/context-menu.js (1)
media/events.js (4)
cellContent(2658-2658)cellContent(2796-2796)isNull(2660-2662)rowIndex(2732-2732)
🔇 Additional comments (27)
media/css/30-components/confirm-dialog.css (1)
195-204: Cell viewer styling aligns with the existing dialog patterns.Sizing and scrolling constraints look consistent with the JSON/Blob viewers.
CHANGELOG.md (1)
3-14: Changelog entry reads well for the new UX features.Clear summary of what’s added and changed.
media/css/30-components/tables.css (2)
103-118: Multiline cell styling looks solid.The line-height variables and scrollable content block should make expanded rows predictable.
43-60: Column resizing is properly handled. The resize logic inmedia/resizing.jsexplicitly setsmaxWidthto "none" whenever a column width is updated, so the default 220px constraint won't cap user-initiated expansions.media/context-menu.js (5)
48-55: New context menu actions are integrated cleanly.They slot neatly into the existing menu structure.
461-466: Action routing for the new items is clear.
519-554: Expand-cell handler is straightforward.
556-662: Row height calculation and persistence look good.
1724-1803: No external callers found—export is not functionally required.Verification found no external usage of
showCellViewerDialoganywhere in the codebase; it is only called internally at line 548. However,scrollToTargetRow(the comparison function mentioned) is similarly internal-only but is exported onwindowat line 2826. If consistency with this pattern is desired, export could be added, but it is not currently necessary.media/events.js (4)
1603-1616: Width restore now clears max-width and recomputes layout — nice.
1638-1640: Row-height restore now refreshes multiline state.
2274-2286: Initial table setup now schedules overflow and multiline updates.
2816-2822: Overflow indicators are refreshed after edits.media/resizing.js (6)
15-20: LGTM! Pointer Events feature detection and state variables.The feature detection pattern using
"PointerEvent" in windowis correct and the state variables for tracking the active pointer capture are well-structured.
22-53: LGTM! Pointer capture and release functions.Good defensive coding with type checks and try-catch blocks to handle potential errors during pointer capture/release operations. The cleanup in
releaseResizePointerproperly resets both state variables.
55-85: LGTM! Auto-scroll during resize.The edge-based auto-scroll logic is well-implemented. Adjusting
startXto compensate for the scroll delta ensures the resize calculation remains accurate relative to the cursor position.
100-147: LGTM! Conditional event listener setup for Pointer Events.The branching for pointer vs mouse events is correct, and adding
pointercancelas a cleanup event is good practice for handling interrupted pointer interactions.
335-380: LGTM! Resize handling with auto-scroll and dynamic UI updates.The integration of
maybeAutoScrollForResizeduring column resize and calls toupdateTableWidthFromColsandupdateRowMultilineForRowensure the UI stays synchronised with the resize operation. The type check forMouseEvent || PointerEventat line 336 is appropriate.
387-480: LGTM! Cleanup and post-resize hooks inendResize.The
releaseResizePointer()call ensures pointer capture is released, and scheduling cell overflow indicators after column resize is appropriate for refreshing visual state. Based on learnings, event listeners are properly managed with the one-time attachment pattern.media/table.js (8)
31-39: LGTM! Well-structured layout constants.The derived constants (
DEFAULT_ROW_HEIGHT,MULTILINE_ROW_HEIGHT) are correctly computed from the base values, making them easy to adjust if needed.
230-263: LGTM! Table width calculation from column widths.The function correctly falls back to
getBoundingClientRect()when the<col>element's style width is unavailable, and applies the computed total to the table.
265-278: LGTM! Row multiline state calculation.The line count derivation from height and padding is correct, and the CSS variable updates enable flexible styling.
534-543: LGTM! Initial table width and column width setup.Using
DEFAULT_COLUMN_WIDTHfor both the table's initial width calculation and individual<col>elements ensures consistency.
998-998: LGTM! Virtual table uses the standardised row height constant.Using
DEFAULT_ROW_HEIGHTensures consistency between the initial table rendering and virtualisation metrics.
1331-1331: LGTM! Overflow indicators scheduled after virtual render.Scheduling the update via RAF after rendering ensures the DOM has settled before evaluating overflow state.
1408-1409: LGTM! Table width and row multiline sync during virtualisation.Calling
updateTableWidthFromColsafter syncing column widths andupdateRowMultilineForRowwhen applying row heights ensures the UI remains consistent. Based on learnings, virtual scrolling is properly used for large tables.Also applies to: 1432-1432
2302-2315: LGTM! Window exports for new utilities and constants.Exposing the new functions and constants globally enables cross-module access from
resizing.jsandevents.jsas indicated in the PR objectives.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@media/context-menu.js`:
- Around line 1752-1831: The keydown handler handleEscape remains attached when
the dialog is closed via the close button or overlay click; update the close
function inside showCellViewerDialog to also call
document.removeEventListener("keydown", handleEscape) so the listener is removed
on any close path (and keep the existing removal inside handleEscape for the
Escape path). Reference showCellViewerDialog, the close function, and
handleEscape when making the change.
In `@media/resizing.js`:
- Around line 189-195: The code calls captureResizePointer(e, handleEl) but
handleEl can be undefined; ensure a valid handle element is resolved before
calling captureResizePointer by deriving it from the event/DOM (for example:
const resolvedHandle = typeof handleEl !== "undefined" ? handleEl :
target.closest('.resize-handle') || target) and pass resolvedHandle into
captureResizePointer; leave the currentResizeTarget assignment
(currentResizeTarget = header) intact so resizing uses the header while the
resolved handle is used for pointer capture.
In `@media/table.js`:
- Around line 14-18: The constants CELL_RENDER_LIMIT and CELL_ORIGINAL_LIMIT in
media/table.js should not be Number.POSITIVE_INFINITY; change them to a high but
finite cap (e.g., a tuned value like 100000 characters or a configurable
setting) to prevent multi‑MB cell payloads from freezing the webview, and ensure
the full content is still accessible via the cell viewer/editor; update any
related logic that assumes Infinity (e.g., truncation checks) to use the new
finite limits and make the cap configurable if needed.
PR title
feat(table): expand cell viewer + row multiline toggle; improved resize + width restore
Summary
This PR improves table UX for long text and resizing:
What changed
✨ Cell viewer dialog (context menu)
New context menu action: Expand Cell (shown only when useful: truncated/long/contains newlines/overflow).
Opens a Cell Viewer dialog:
Cell Viewer — <column>).Files:
media/context-menu.jsmedia/css/30-components/confirm-dialog.css↕ Row expand/collapse (context menu + persisted state)
New context menu action: Expand Row / Collapse Row.
Toggling:
row-multilinestyling and updates CSS variables for visible lines.setTabViewState.Files:
media/context-menu.jsmedia/table.jsmedia/events.jsmedia/css/30-components/tables.css📐 Table layout defaults (multiline previews + width derived from cols)
cell-contentblock (non-blob)..data-tabledefault width moved frommax-content→100%, while table width is still explicitly managed from col widths via JS.Files:
media/css/30-components/tables.cssmedia/table.js🖱️ Resizing UX improvements (pointer events + edge auto-scroll)
Resizing now uses Pointer Events when available:
pointercancel.Adds auto-scroll near scroll container edges while resizing columns.
After column resize:
<col>width + clears maxWidth constraints.Files:
media/resizing.js🧰 View state restore improvements
When restoring:
maxWidth: none, updates pinned positions, recalculates table width.Files:
media/events.jsmedia/table.jsNotable implementation details
No default truncation:
CELL_RENDER_LIMITandCELL_ORIGINAL_LIMITset toInfinityto avoid truncating visible values by default.Added shared helpers on
windowfor cross-module coordination:updateTableWidthFromColsscheduleCellOverflowIndicatorsupdateRowMultilineForRowupdateRowMultilineForTableChangelog
Added an
[Unreleased]section documenting:Risk & rollback
media/resizing.js,media/table.js,media/context-menu.js, and the CSS changes for table/cell layout.Summary by CodeRabbit
New Features
Changes
Style
✏️ Tip: You can customize this high-level summary in your review settings.