Feature/cell expand side sheet#115
Merged
Merged
Conversation
Hovering any table cell reveals a Maximize2 icon. Clicking it opens a slide-in panel showing the column name, type, description, full cell value with a copy button, and (when available) the row-level sources the populate agent saved alongside the data. Changes: - SideSheet.tsx: new component with backdrop, Escape/Tab trap, scroll lock, and slide-in animation. CellDetail sub-component shows column metadata, value (with inline "Copied" feedback), and sources as clickable external links. - types.ts: add optional `sources?: string[]` to DatasetRow - DataRow.tsx: `group` + Maximize2 button on hover per cell; `onCellExpand` callback added to DataRowData interface. - DatasetTable.tsx: accept and forward `onCellExpand` prop. - page.tsx: cellDetail state, row sources lookup, SideSheet render. - globals.css: `slide-in` keyframe + `.animate-slide-in` utility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lucide-react is in bun.lock but not installed in the running Docker container's node_modules. Existing components (ColumnHeader, ColumnIcon, ThemeToggle) all use inline SVGs — follow the same pattern instead of adding a runtime dependency. Replaced: Copy, Check, ExternalLink, X in SideSheet.tsx and Maximize2 in DataRow.tsx with self-contained SVG components. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Performance: onCellExpand was an inline arrow on <DatasetTable>, busting the itemData useMemo on every parent render and causing react-window to re-render all visible rows unnecessarily. Replaced with a useCallback (handleCellExpand) so the reference is stable. Simplification: cellDetail state now stores the resolved DatasetColumn object at click time instead of a columnName string. This eliminates the render-time columns.find() call and the IIFE pattern in JSX — the SideSheet children reduce to a plain conditional render. Cleanup: removed @Keyframes slide-in and .animate-slide-in from globals.css — those were added for the side-sheet variant and are unused now that the UI is a centered modal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a cell detail slide-over feature to the dataset table. It extends the Sequence Diagram(s)sequenceDiagram
participant User
participant DataRow as DataRow (cell)
participant DatasetTable as DatasetTable
participant Handler as handleCellExpand
participant PageState as Dataset Page state
participant SideSheet as SideSheet / CellDetail
User->>DataRow: click expand
DataRow->>DatasetTable: onCellExpand(columnName, value, rowId)
DatasetTable->>Handler: forward onCellExpand
Handler->>PageState: lookup column & row -> set cellDetail
PageState->>SideSheet: render SideSheet with CellDetail
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (4)
frontend/components/table/DataRow.tsx (1)
9-16: 💤 Low valueMove
import { floorWidth }above the component declaration.
IconMaximize2is declared at Line 9 between the import block and thefloorWidthimport at Line 16. While ES module imports are hoisted so this runs correctly, it tripsimport/first-style lint rules and hurts readability.♻️ Proposed reordering
import { CellValue } from "./CellValue"; +import { floorWidth } from "./utils"; function IconMaximize2() { return ( <svg width="12" height="12" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round" aria-hidden="true"> <polyline points="15 3 21 3 21 9"/><polyline points="9 21 3 21 3 15"/><line x1="21" y1="3" x2="14" y2="10"/><line x1="3" y1="21" x2="10" y2="14"/> </svg> ); } -import { floorWidth } from "./utils";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/table/DataRow.tsx` around lines 9 - 16, The IconMaximize2 component is declared before the module import of floorWidth which violates import/first lint rules; move the import { floorWidth } statement so it appears above the IconMaximize2 function declaration (i.e., place the floorWidth import into the top import block), ensuring all imports (including floorWidth) are declared before any component/function definitions like IconMaximize2.frontend/components/SideSheet.tsx (3)
183-183: 💤 Low valueConsider using the URL itself as the key if sources are unique.
Using the array index as
keyworks but isn't ideal. If sources are guaranteed to be unique URLs, using the URL itself as the key provides better React reconciliation. If sources can contain duplicates, the current approach is acceptable.♻️ Optional: Use URL as key
- {sources.map((src, i) => ( - <li key={i}> + {sources.map((src, i) => ( + <li key={src + i}>Or if URLs are guaranteed unique:
- {sources.map((src, i) => ( - <li key={i}> + {sources.map((src) => ( + <li key={src}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/SideSheet.tsx` at line 183, In SideSheet component where the list uses <li key={i}>, replace the index key with a stable unique identifier from the source (e.g., the source URL variable used in the map) by using that URL as the key (e.g., key={sourceUrl}); if duplicates are possible, use a composite key like `${sourceUrl}-${i}` to guarantee uniqueness while preferring the URL for reconciliation.
85-109: ⚡ Quick winAdd aria-labelledby to link dialog to its heading.
The dialog has
role="dialog"andaria-modal="true"but is missingaria-labelledbyto connect it to the "Cell Detail" heading. This helps screen readers announce the dialog title when it opens.♻️ Proposed accessibility improvement
<div ref={panelRef} className="relative w-full max-w-lg max-h-[80vh] bg-surface border border-border rounded-xl shadow-2xl flex flex-col" + aria-labelledby="cell-detail-title" > <div className="flex items-center justify-between px-5 py-4 border-b border-border shrink-0"> - <h2 className="text-sm font-semibold text-foreground">Cell Detail</h2> + <h2 id="cell-detail-title" className="text-sm font-semibold text-foreground">Cell Detail</h2> <button🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/SideSheet.tsx` around lines 85 - 109, Add an accessible label by giving the dialog container (the outer div with role="dialog" and aria-modal="true") an aria-labelledby that references an id on the heading; add a unique id to the <h2> with text "Cell Detail" and set aria-labelledby on the dialog to that id so screen readers announce the title (look for the dialog/container div with role="dialog" and the heading element rendered in the component).
61-64: 💤 Low valueFocus trap uses a static focusable elements list.
The
querySelectorAllon line 61 creates a snapshot of focusable elements when the modal opens. If the modal content changes dynamically (e.g., conditional rendering, expanding lists), new focusable elements won't be included in the trap.Given the current usage with static CellDetail content, this is unlikely to cause issues.
♻️ Optional: Query focusable elements dynamically
function onKey(e: KeyboardEvent) { if (e.key === "Escape") { onClose(); return; } - if (e.key !== "Tab" || focusable.length === 0) return; - const first = focusable[0]; - const last = focusable[focusable.length - 1]; + const current = panel.querySelectorAll<HTMLElement>( + 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])', + ); + if (e.key !== "Tab" || current.length === 0) return; + const first = current[0]; + const last = current[current.length - 1]; if (e.shiftKey && document.activeElement === first) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/SideSheet.tsx` around lines 61 - 64, The focus-trap currently captures a static NodeList named "focusable" in the SideSheet component when the panel opens, so dynamically added/removed controls won't be reachable; change to query focusable elements on demand by creating a helper (e.g., getFocusable(panel) or inline calls) that runs panel.querySelectorAll(...) whenever you need the first/last focusable (on open and when handling Tab/Shift+Tab), update uses of the "focusable" variable to call that helper so newly rendered interactive elements are included, and ensure the open handler still focuses the first current focusable element via that dynamic query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/components/SideSheet.tsx`:
- Around line 127-135: The handleCopy function schedules a setTimeout that may
call setCopied after the component unmounts; store the timeout ID in a ref
(e.g., copyTimeoutRef) when calling setTimeout in handleCopy and clear that
timeout in a cleanup (useEffect return) to prevent setState on an unmounted
component, or alternatively check a mountedRef flag before calling setCopied in
the timeout; update handleCopy, add the ref (copyTimeoutRef or mountedRef) and
the useEffect cleanup to clearTimeout or set mounted=false respectively, and
ensure you clear any existing timeout before starting a new one.
- Around line 185-186: The anchor is rendering href={src} (from the sources
array) without validating the protocol which can allow javascript: or data: XSS;
update the SideSheet.tsx rendering logic that creates the anchor (the code using
src from sources) to validate each URL's protocol (e.g., create a URL object and
ensure protocol is "http:" or "https:" or use a strict whitelist) and only
render the <a href=...> when valid, otherwise render a non-clickable fallback
(plain text or disabled link); also add rel="noopener noreferrer" to the anchor
when target="_blank" for safety.
In `@frontend/components/table/DataRow.tsx`:
- Around line 135-145: The expand button in DataRow.tsx is only revealed via
group-hover (className includes group-hover:opacity-100) which leaves it
invisible to keyboard users; update the button's classes to also reveal and
style it on keyboard focus by adding focus-visible:opacity-100 and appropriate
focus-visible ring classes (e.g., focus-visible:ring and
focus-visible:ring-offset) so the control becomes visible and has a clear focus
ring when tabbed to; ensure the clickable handler (onCellExpand) and aria-label
remain unchanged and that the IconMaximize2 stays inside the same button
element.
---
Nitpick comments:
In `@frontend/components/SideSheet.tsx`:
- Line 183: In SideSheet component where the list uses <li key={i}>, replace the
index key with a stable unique identifier from the source (e.g., the source URL
variable used in the map) by using that URL as the key (e.g., key={sourceUrl});
if duplicates are possible, use a composite key like `${sourceUrl}-${i}` to
guarantee uniqueness while preferring the URL for reconciliation.
- Around line 85-109: Add an accessible label by giving the dialog container
(the outer div with role="dialog" and aria-modal="true") an aria-labelledby that
references an id on the heading; add a unique id to the <h2> with text "Cell
Detail" and set aria-labelledby on the dialog to that id so screen readers
announce the title (look for the dialog/container div with role="dialog" and the
heading element rendered in the component).
- Around line 61-64: The focus-trap currently captures a static NodeList named
"focusable" in the SideSheet component when the panel opens, so dynamically
added/removed controls won't be reachable; change to query focusable elements on
demand by creating a helper (e.g., getFocusable(panel) or inline calls) that
runs panel.querySelectorAll(...) whenever you need the first/last focusable (on
open and when handling Tab/Shift+Tab), update uses of the "focusable" variable
to call that helper so newly rendered interactive elements are included, and
ensure the open handler still focuses the first current focusable element via
that dynamic query.
In `@frontend/components/table/DataRow.tsx`:
- Around line 9-16: The IconMaximize2 component is declared before the module
import of floorWidth which violates import/first lint rules; move the import {
floorWidth } statement so it appears above the IconMaximize2 function
declaration (i.e., place the floorWidth import into the top import block),
ensuring all imports (including floorWidth) are declared before any
component/function definitions like IconMaximize2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6df78d8-9323-405b-8e56-64911f1d1b8b
📒 Files selected for processing (5)
frontend/app/dataset/[id]/page.tsxfrontend/components/SideSheet.tsxfrontend/components/table/DataRow.tsxfrontend/components/table/DatasetTable.tsxfrontend/components/table/types.ts
- SideSheet: validate source URLs to http/https before rendering as <a> to prevent javascript:/data: XSS; invalid URLs fall back to plain text - SideSheet: store setTimeout ID in a ref and clear on unmount to avoid calling setCopied after the component is gone; promote handleCopy to useCallback - SideSheet: add aria-labelledby on dialog + id on heading for screen readers - SideSheet: use URL string as list key instead of array index - DataRow: move floorWidth import above IconMaximize2 declaration (import/first) - DataRow: reveal expand button on keyboard focus-visible, not only on hover Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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 PR is to add a feature for column details window, which surfaces the "sources" information for collected data. Hovering any cell in the dataset table reveals a small expand icon. Clicking it opens a centered popup showing:
Design decisions
Special thanks to the PR opened by @MabudAlam , which also included a details side sheet. This is the direction that we decided to go for before the Bigset launch.