Skip to content

fix(studio): replace ScrollTable with KUI table, fix scrolling#391

Open
nv-odrulea wants to merge 4 commits into
mainfrom
od/files-picker-scrolling
Open

fix(studio): replace ScrollTable with KUI table, fix scrolling#391
nv-odrulea wants to merge 4 commits into
mainfrom
od/files-picker-scrolling

Conversation

@nv-odrulea

@nv-odrulea nv-odrulea commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

ScrollTable wasn't the best fit for the files list in the Dataset file picker modal. One reason is that it didn't bring sticky table headers with it, leaving us to either use DataView (overkill) or roll our own. So we roll our own basic implementation of KUI table here, which is a fairly simple use case and appropriate.

As a bonus, the styling looks better since the rows do not have padding beyond their separators. We also gain sticky table headers.

To fix sticky table header bottom row from scrolling out of view, we needed to apply a known CSS hack involving box-shadow:

border-bottom on a table element (tr, th, thead) is laid out by the browser's table algorithm, not the element's own paint layer — sticky doesn't take it with the element. box-shadow bypasses the table layout model and is always painted with the sticky element. This is standard practice; MDN and CSS Working Group guidance both recommend box-shadow for sticky table header separators.

Screenshot 2026-06-22 at 12 52 07 PM

Summary by CodeRabbit

  • New Features

    • Improved file selection in the upload list, supporting both single-select (radio) and multi-select (checkbox) modes with clearer per-row controls.
  • Style

    • Redesigned the file upload modal layout for a simpler, cleaner presentation.
    • Enhanced the file table presentation with sticky headers and clearer visual separators to improve browsing.

@nv-odrulea nv-odrulea requested review from a team as code owners June 22, 2026 19:52
@github-actions github-actions Bot added the fix label Jun 22, 2026
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 963cb37c-32a7-4112-9946-b7aacf645ed0

📥 Commits

Reviewing files that changed from the base of the PR and between 70f1ff9 and e686d0f.

📒 Files selected for processing (1)
  • web/packages/common/src/components/UploadModal/SimpleFilesTable.test.tsx

📝 Walkthrough

Walkthrough

SimpleFilesTable drops ScrollTable in favor of rendering DataView.Root primitives directly, with a new fileRows memo and makeColumns callback for conditional checkbox/radio controls. UploadModal removes the Stack scroll wrapper, switching to a fixed-width overflow-hidden layout. A .sticky-table-header CSS utility is added using box-shadow for a sticky thead border.

Changes

UploadModal table and layout refactor

Layer / File(s) Summary
SimpleFilesTable: ScrollTable → DataView.Root with fileRows and makeColumns
web/packages/common/src/components/UploadModal/SimpleFilesTable.tsx, web/packages/common/src/components/UploadModal/SimpleFilesTable.test.tsx
Removes ScrollTable and its TableColumnDefinition/TableRowDefinition model. Introduces FileRow type and fileRows useMemo with isDisabled property derived from invalidFileMode and allowed-extension rules. Adds makeColumns callback for DataView.Root that renders Checkbox (multi-select) or RadioGroupItem/RadioGroupInput (single-select) selection controls. Wraps table in RadioGroupRoot and dispatches TOGGLE_FILE_SELECTION on radio value changes. Test adds getBoundingClientRect spy to force virtualizer to render all rows in test environment.
Modal layout simplification and sticky header CSS
web/packages/common/src/components/UploadModal/index.tsx, web/packages/studio/src/index.css
Removes Stack import and replaces nested Stack/scroll layout with fixed-width w-[560px] overflow-hidden ModalContent. UploadPickerBody renders directly under ModalMain without asChild. Adds .sticky-table-header CSS utility using box-shadow: 0 2px 0 var(--border-color-base) for cross-browser sticky thead borders.

Possibly related PRs

  • NVIDIA-NeMo/nemo-platform#378: Both PRs modify SimpleFilesTable's handling of invalidFileMode="disable" by changing how disabled/unacceptable files are detected/rendered.

Suggested reviewers

  • htolentino-nvidia
  • dmariali
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title directly describes the main change: replacing ScrollTable with KUI table and fixing scrolling behavior, which aligns with the core refactoring across SimpleFilesTable, UploadModalContent, and CSS additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch od/files-picker-scrolling

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
web/packages/common/src/components/UploadModal/SimpleFilesTable.tsx (1)

117-117: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Keep sticky-header styling with the common component.

SimpleFilesTable is in web/packages/common, but .sticky-table-header is defined in web/packages/studio/src/index.css Lines 70-74. Non-Studio consumers will miss this styling. Inline it or move it to common/shared CSS.

Proposed fix
-        <TableHead className="sticky top-0 z-[1] bg-surface-overlay border-b-0 sticky-table-header">
+        <TableHead className="sticky top-0 z-[1] bg-surface-overlay border-b-0 shadow-[0_2px_0_var(--border-color-base)]">

Then remove the Studio-only global rule:

-/* hack fix: sticky table header separator — box-shadow is the only reliable way to
-   paint a bottom border that stays with a position:sticky thead across browsers */
-.sticky-table-header {
-  box-shadow: 0 2px 0 var(--border-color-base);
-}
🤖 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 `@web/packages/common/src/components/UploadModal/SimpleFilesTable.tsx` at line
117, The TableHead element in SimpleFilesTable uses the CSS class
`.sticky-table-header` which is defined only in
web/packages/studio/src/index.css, making it unavailable to non-Studio consumers
of this common component. Move the sticky-table-header CSS rule definition from
studio's index.css to the common package's shared CSS file, or inline the styles
directly on the TableHead element in SimpleFilesTable. Then remove the original
rule from studio's index.css to avoid duplication and ensure all consumers have
access to the proper styling.
🤖 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 `@web/packages/common/src/components/UploadModal/index.tsx`:
- Around line 72-82: The ModalContent component currently uses `w-[560px]
overflow-hidden` which clips content on narrow and short screens, making footer
actions unreachable. Replace the `overflow-hidden` class with a scrollable
overflow property (such as `overflow-y-auto` or `overflow-auto`) to allow users
to scroll and access all content including the footer actions. Ensure the width
constraint is responsive or adjusted to work properly on smaller viewports while
maintaining the modal's layout integrity.

---

Nitpick comments:
In `@web/packages/common/src/components/UploadModal/SimpleFilesTable.tsx`:
- Line 117: The TableHead element in SimpleFilesTable uses the CSS class
`.sticky-table-header` which is defined only in
web/packages/studio/src/index.css, making it unavailable to non-Studio consumers
of this common component. Move the sticky-table-header CSS rule definition from
studio's index.css to the common package's shared CSS file, or inline the styles
directly on the TableHead element in SimpleFilesTable. Then remove the original
rule from studio's index.css to avoid duplication and ensure all consumers have
access to the proper styling.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7c6cef19-70e5-4fc5-b60c-8c9ba330b81a

📥 Commits

Reviewing files that changed from the base of the PR and between a7f031b and 2896573.

📒 Files selected for processing (3)
  • web/packages/common/src/components/UploadModal/SimpleFilesTable.tsx
  • web/packages/common/src/components/UploadModal/index.tsx
  • web/packages/studio/src/index.css

Comment thread web/packages/common/src/components/UploadModal/index.tsx Outdated
@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 20908/27478 76.1% 61.2%
Integration Tests 12109/26247 46.1% 19.5%

Comment thread web/packages/common/src/components/UploadModal/SimpleFilesTable.tsx Outdated
@nv-odrulea nv-odrulea force-pushed the od/files-picker-scrolling branch from 2896573 to 816b114 Compare June 23, 2026 18:42
Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
@nv-odrulea nv-odrulea force-pushed the od/files-picker-scrolling branch from 816b114 to 70f1ff9 Compare June 23, 2026 22:16
Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants