refactor(studio): consolidate four delete modals into generic BulkDeleteModal#467
Open
aray12 wants to merge 2 commits into
Open
refactor(studio): consolidate four delete modals into generic BulkDeleteModal#467aray12 wants to merge 2 commits into
aray12 wants to merge 2 commits into
Conversation
…eteModal Signed-off-by: Alex Ray <alray@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughBulk delete flows now use a shared modal component, experiment sort handling is simplified, and Claude Code history titles are no longer generated from helper logic. ChangesBulk delete modal migration
Experiment sort handling
Claude Code history labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsx (1)
14-17: 📐 Maintainability & Code Quality | 🟡 MinorRestore pending-state coverage.
EvaluationJobBulkDeleteModalstill disables Cancel/Delete and showsDeleting...viaisPending, but the test file no longer exercises that state.🤖 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/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsx` around lines 14 - 17, The BulkDeleteModal tests no longer cover the pending deletion state exposed by useDatasetFilesDelete. Update the test suite in BulkDeleteModal/index.test.tsx to mock and assert the isPending path for EvaluationJobBulkDeleteModal, verifying that Cancel/Delete are disabled and the label changes to “Deleting...” when useDatasetFilesDelete returns isPending=true. Use the existing useDatasetFilesDelete mock and the EvaluationJobBulkDeleteModal render path to restore this coverage.
🧹 Nitpick comments (1)
web/packages/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
import typeforListExperimentsSort.It's referenced only in a type position (the L122 assertion).
As per coding guidelines: "Use
import typefor type-only imports in TypeScript." (Skip if it shares a value-import statement.)🤖 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/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.ts` at line 15, The import of ListExperimentsSort in useExperimentGroupExperiments is type-only, so update the existing import statement to use import type for it rather than a value import. Locate the usage in useExperimentGroupExperiments and keep it as a type import since it is only referenced in the L122 assertion and does not need a runtime import.Source: Coding guidelines
🤖 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/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx`:
- Around line 63-75: The bulk delete flow in handleDeleteJobs currently filters
out jobs missing workspace, which can make the operation appear successful while
leaving selected rows undeleted. Update the DataDesignerJobsDataView deletion
logic to validate every selected DataDesignerJob before calling
deleteJobMutation.mutateAsync, and if any job lacks a workspace (or name), throw
an error instead of skipping it so the failure is surfaced to the user.
In
`@web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx`:
- Around line 72-84: The bulk delete flow in handleDeleteJobs currently filters
incomplete rows but still relies on non-null assertions, and Promise.all will
reject on the first failure while other deletions continue. Update
SafeSynthesizerJobsDataView’s handleDeleteJobs to use a proper type guard that
narrows jobs with both workspace and name, validate the full selection up front
and abort the batch if any row is incomplete, and adjust the deletion loop so
all valid jobs are handled with clear per-job error reporting without masking
missing identifiers.
In
`@web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsx`:
- Around line 45-47: The BulkDeleteModal trigger wiring is overwriting any
existing click behavior and still relies on an `any` cast. Update the `trigger`
logic in `BulkDeleteModal` to properly type the `isValidElement` guard for
`slotTrigger`, then compose the cloned element’s existing `onClick` with
`openTrigger` instead of replacing it, so both handlers run in order without
using `any`.
In
`@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsx`:
- Line 23: The HistorySessionButton tooltip is currently showing only the
timestamp, which hides the session prompt and makes similar sessions hard to
distinguish. Update the title prop in HistorySessionButton so it includes the
full prompt again along with the timestamp, using the session data already
available in this component, while keeping the visible label unchanged.
In
`@web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx`:
- Line 34: The delete modal is reusing a create-oriented error message from
useMutateMany, so delete failures can surface as “Failed to create...”. Update
DatasetBulkDeleteModal to use a delete-specific error aggregator or extend
useMutateMany to accept an action label, and make sure the error text shown from
deleteDatasets is phrased for deletes rather than creates.
- Around line 36-44: The handleDelete flow in DatasetBulkDeleteModal currently
filters out datasets missing workspace or name and still calls
onConfirmSuccess(), which can report success after silently skipping selections.
Update handleDelete to validate the full datasets array up front, and if any
FilesetOutput is missing workspace or name, fail the operation instead of
proceeding; only call deleteDatasets and onConfirmSuccess() when every selected
dataset is valid. Use the existing handleDelete, deleteDatasets, and
onConfirmSuccess symbols to locate the change.
- Around line 49-51: The DatasetBulkDeleteModal trigger currently overwrites any
existing onClick on slotTrigger when cloning it in the trigger setup, so update
the cloneElement usage to compose the existing click handler with openTrigger
instead of replacing it, and remove the unnecessary React.ReactElement<any> cast
by using a properly typed element in this path.
---
Outside diff comments:
In
`@web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsx`:
- Around line 14-17: The BulkDeleteModal tests no longer cover the pending
deletion state exposed by useDatasetFilesDelete. Update the test suite in
BulkDeleteModal/index.test.tsx to mock and assert the isPending path for
EvaluationJobBulkDeleteModal, verifying that Cancel/Delete are disabled and the
label changes to “Deleting...” when useDatasetFilesDelete returns
isPending=true. Use the existing useDatasetFilesDelete mock and the
EvaluationJobBulkDeleteModal render path to restore this coverage.
---
Nitpick comments:
In
`@web/packages/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.ts`:
- Line 15: The import of ListExperimentsSort in useExperimentGroupExperiments is
type-only, so update the existing import statement to use import type for it
rather than a value import. Locate the usage in useExperimentGroupExperiments
and keep it as a type import since it is only referenced in the L122 assertion
and does not need a runtime import.
🪄 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: 218ee35b-5c93-4b87-9658-5613d05a6475
📒 Files selected for processing (19)
web/packages/studio/src/components/BulkDeleteModal/index.tsxweb/packages/studio/src/components/DatasetsTable/index.test.tsxweb/packages/studio/src/components/dataViews/DataDesignerJobsDataView/DeleteJobModal.tsxweb/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.tsweb/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.test.tsxweb/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.tsxweb/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsxweb/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsxweb/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsxweb/packages/studio/src/components/filesets/FilesetFileExplorer/extractFilePathsFromDirectory.tsweb/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.test.tsxweb/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsxweb/packages/studio/src/routes/FilesetListRoute/index.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeHistoryPanel.test.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.test.tsweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.ts
💤 Files with no reviewable changes (6)
- web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/DeleteJobModal.tsx
- web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.tsx
- web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.test.ts
- web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/DeleteJobModal.test.tsx
- web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/ClaudeCodeHistoryPanel.test.tsx
- web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/helpers.ts
Comment on lines
+63
to
+75
| const handleDeleteJobs = async (jobsToDelete: DataDesignerJob[]) => { | ||
| const valid = jobsToDelete.filter((job) => job.workspace && job.name); | ||
| await Promise.all( | ||
| valid.map(async (job) => { | ||
| try { | ||
| await deleteJobMutation.mutateAsync({ workspace: job.workspace!, name: job.name }); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Failed to delete job "${job.name}": ${error instanceof Error ? error.message : 'Unknown error'}` | ||
| ); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant slice with line numbers.
ast-grep outline web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx --view expanded || true
echo '--- FILE SLICE ---'
sed -n '1,220p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx | cat -n
echo '--- TYPE DEFINITIONS / USAGES ---'
rg -n "type DataDesignerJob|interface DataDesignerJob|DataDesignerJob" web/packages/studio/src/components/dataViews -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 12622
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant slice with line numbers.
ast-grep outline web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx --view expanded || true
echo '--- FILE SLICE ---'
sed -n '1,220p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx | cat -n
echo '--- TYPE DEFINITIONS / USAGES ---'
rg -n "type DataDesignerJob|interface DataDesignerJob|DataDesignerJob" web/packages/studio/src/components/dataViews -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 12622
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant slice with line numbers.
ast-grep outline web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx --view expanded || true
echo '--- FILE SLICE ---'
sed -n '1,220p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx | cat -n
echo '--- TYPE DEFINITIONS / USAGES ---'
rg -n "type DataDesignerJob|interface DataDesignerJob|DataDesignerJob" web/packages/studio/src/components/dataViews -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 12622
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- REMAINDER OF TARGET FILE ---'
sed -n '220,360p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx | cat -n
echo '--- BULK DELETE MODAL API ---'
ast-grep outline web/packages/studio/src/components/BulkDeleteModal --view expanded || true
echo '--- BULK DELETE MODAL USAGE/IMPLEMENTATION ---'
rg -n "export const BulkDeleteModal|function BulkDeleteModal|const BulkDeleteModal|onDelete|handleDelete" web/packages/studio/src/components/BulkDeleteModal -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 4163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- REMAINDER OF TARGET FILE ---'
sed -n '220,360p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx | cat -n
echo '--- BULK DELETE MODAL API ---'
ast-grep outline web/packages/studio/src/components/BulkDeleteModal --view expanded || true
echo '--- BULK DELETE MODAL USAGE/IMPLEMENTATION ---'
rg -n "export const BulkDeleteModal|function BulkDeleteModal|const BulkDeleteModal|onDelete|handleDelete" web/packages/studio/src/components/BulkDeleteModal -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 4163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- BULK DELETE MODAL IMPLEMENTATION ---'
sed -n '1,140p' web/packages/studio/src/components/BulkDeleteModal/index.tsx | cat -n
echo '--- CREATEJOB SCHEMA FIELDS ---'
rg -n "export type CreateJob|interface CreateJob|type CreateJob" web/packages/studio -S
echo '--- JOB ROW SELECTION SOURCE ---'
rg -n "selectedRows|rowSelection" web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx -n -A6 -B6Repository: NVIDIA-NeMo/nemo-platform
Length of output: 2489
🏁 Script executed:
Inspect the bulk delete modal implementation and the CreateJob schema fields to judge whether invalid rows can be selected and how delete failures are handled.Repository: NVIDIA-NeMo/nemo-platform
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- BULK DELETE MODAL IMPLEMENTATION ---'
sed -n '1,140p' web/packages/studio/src/components/BulkDeleteModal/index.tsx | cat -n
echo '--- CREATEJOB SCHEMA FIELDS ---'
rg -n "export type CreateJob|interface CreateJob|type CreateJob" web/packages/studio -S
echo '--- JOB ROW SELECTION SOURCE ---'
rg -n "selectedRows|rowSelection" web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx -n -A6 -B6Repository: NVIDIA-NeMo/nemo-platform
Length of output: 2489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the generated schema/type for CreateJob and inspect whether workspace/name are optional.
rg -n "CreateJob" web/packages/studio web/packages -g '*.ts' -g '*.tsx' -S | head -n 80
echo '--- schema file candidates ---'
fd -a "schema.ts" web/packages/studio web/packages | head -n 20
fd -a "data-designer" web/packages/studio web/packages | head -n 20Repository: NVIDIA-NeMo/nemo-platform
Length of output: 11916
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the generated schema/type for CreateJob and inspect whether workspace/name are optional.
rg -n "CreateJob" web/packages/studio web/packages -g '*.ts' -g '*.tsx' -S | head -n 80
echo '--- schema file candidates ---'
fd -a "schema.ts" web/packages/studio web/packages | head -n 20
fd -a "data-designer" web/packages/studio web/packages | head -n 20Repository: NVIDIA-NeMo/nemo-platform
Length of output: 11616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the generated data-designer schema and inspect CreateJob fields.
fd -a "data-designer" web/packages/sdk web/packages/studio | head -n 50
echo '--- schema matches ---'
rg -n "export (type|interface) CreateJob|CreateJobRequest|workspace\\??:|name\\??:" web/packages/sdk web/packages/studio -g '*.ts' -g '*.tsx' -S | head -n 120Repository: NVIDIA-NeMo/nemo-platform
Length of output: 10472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the generated data-designer schema and inspect CreateJob fields.
fd -a "data-designer" web/packages/sdk web/packages/studio | head -n 50
echo '--- schema matches ---'
rg -n "export (type|interface) CreateJob|CreateJobRequest|workspace\\??:|name\\??:" web/packages/sdk web/packages/studio -g '*.ts' -g '*.tsx' -S | head -n 120Repository: NVIDIA-NeMo/nemo-platform
Length of output: 9409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the generated data-designer schema and inspect CreateJob fields.
fd -a "data-designer" web/packages/sdk web/packages/studio | head -n 50
echo '--- schema matches ---'
rg -n "export (type|interface) CreateJob|CreateJobRequest|workspace\\??:|name\\??:" web/packages/sdk web/packages/studio -g '*.ts' -g '*.tsx' -S | head -n 120Repository: NVIDIA-NeMo/nemo-platform
Length of output: 9051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- GENERATED DATA-DESIGNER FILES ---'
fd -a "data-designer" web/packages/sdk | head -n 80
echo '--- CREATEJOB / DELETE MUTATION TYPES ---'
rg -n "CreateJob|DeleteCreateJob|ListCreateJobs|workspace: string|name: string|workspace\\?:|name\\?:" web/packages/sdk/generated web/packages/sdk/orval web/packages/studio/src -g '*.ts' -g '*.tsx' -S | head -n 200
echo '--- TARGET FILE CONTEXT ---'
sed -n '1,120p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx | cat -nRepository: NVIDIA-NeMo/nemo-platform
Length of output: 27572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- dataDesigner API ---'
sed -n '1,320p' web/packages/studio/src/api/dataDesigner.ts | cat -n
echo '--- data-designer mocks ---'
find web/packages/studio/src/mocks/data-designer -maxdepth 2 -type f | sort | xargs -r -n 1 sh -c 'echo "FILE: $1"; sed -n "1,220p" "$1" | cat -n' shRepository: NVIDIA-NeMo/nemo-platform
Length of output: 13221
Reject rows missing workspace instead of skipping them
web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx:63-75 The current filter drops selected jobs without a workspace, so bulk delete can report success while leaving some selected rows untouched. Surface that as an error.
🤖 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/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx`
around lines 63 - 75, The bulk delete flow in handleDeleteJobs currently filters
out jobs missing workspace, which can make the operation appear successful while
leaving selected rows undeleted. Update the DataDesignerJobsDataView deletion
logic to validate every selected DataDesignerJob before calling
deleteJobMutation.mutateAsync, and if any job lacks a workspace (or name), throw
an error instead of skipping it so the failure is surfaced to the user.
Source: Coding guidelines
Comment on lines
+72
to
+84
| const handleDeleteJobs = async (jobsToDelete: SafeSynthesizerJob[]) => { | ||
| const valid = jobsToDelete.filter((job) => job.workspace && job.name); | ||
| await Promise.all( | ||
| valid.map(async (job) => { | ||
| try { | ||
| await deleteJobMutation.mutateAsync({ workspace: job.workspace!, name: job.name }); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Failed to delete job "${job.name}": ${error instanceof Error ? error.message : 'Unknown error'}` | ||
| ); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby definitions
FILE="web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx"
wc -l "$FILE"
sed -n '1,220p' "$FILE"
# Find the SafeSynthesizerJob type and delete mutation definition/usages
rg -n "type SafeSynthesizerJob|interface SafeSynthesizerJob|SafeSynthesizerJob" web/packages/studio/src -g '!**/node_modules/**'
rg -n "deleteJobMutation|mutateAsync\\(" web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView web/packages/studio/src -g '!**/node_modules/**'
# Look for similar bulk delete patterns in nearby data views
rg -n "Promise\\.allSettled|Promise\\.all\\(" web/packages/studio/src/components/dataViews -g '!**/node_modules/**'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 46198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file's structure first
ast-grep outline web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx --view expanded
# Read only the relevant slice around the handler if the file is large
sed -n '60,110p' web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx
# Find the exact job type definition and any guards that populate the selection
rg -n "SafeSynthesizerJob|workspace && name|rowSelection|selected.*jobs|jobsToDelete" web/packages/studio/src/components/dataViews web/packages/studio/src -g '!**/node_modules/**'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether Promise.all is used with fire-and-forget mutations anywhere else in the package
rg -n "Promise\\.all\\(|Promise\\.allSettled\\(" web/packages/studio/src -g '!**/node_modules/**'
# Inspect the delete mutation hook if present
rg -n "use.*Delete.*Job|delete.*Job.*mutation|mutationFn" web/packages/studio/src -g '!**/node_modules/**'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 9663
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx"
sed -n '340,428p' "$FILE"
# Inspect the DataDesigner counterpart for comparison of intended semantics
sed -n '55,80p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx
# Inspect the bulk delete modal API to see whether it can surface per-item failures
ast-grep outline web/packages/studio/src/components/BulkDeleteModal --view expandedRepository: NVIDIA-NeMo/nemo-platform
Length of output: 4054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the bulk delete modal implementation if small enough
wc -l web/packages/studio/src/components/BulkDeleteModal/index.tsx
sed -n '1,260p' web/packages/studio/src/components/BulkDeleteModal/index.tsx
# Find its prop contract and error handling
rg -n "BulkDeleteModal" web/packages/studio/src/components/BulkDeleteModal web/packages/studio/src/components -g '!**/node_modules/**'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 8421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx"
sed -n '360,428p' "$FILE"
echo '--- DataDesigner compare ---'
sed -n '60,85p' web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx
echo '--- BulkDeleteModal outline ---'
ast-grep outline web/packages/studio/src/components/BulkDeleteModal --view expandedRepository: NVIDIA-NeMo/nemo-platform
Length of output: 3644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the SafeSynthesizerJob schema for field optionality
SCHEMA="web/packages/sdk/generated/safe-synthesizer/schema.ts"
wc -l "$SCHEMA"
rg -n "export type SafeSynthesizerJob|export interface SafeSynthesizerJob|workspace|name" "$SCHEMA" -n -A4 -B4
# Inspect the selectedRows type/flow if it can include incomplete rows
rg -n "selectedRows|rowSelection|renderBulkActions|SafeSynthesizerJobWithId|jobs =" web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx -A4 -B4
# Inspect whether the BulkDeleteModal caller can receive anything other than the displayed rows
rg -n "setDeleteJobs\\(selectedRows\\)|items={deleteJobs}" web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx -A2 -B2Repository: NVIDIA-NeMo/nemo-platform
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the generated schema file exactly
git ls-files | rg '^web/packages/.*/generated/safe-synthesizer/schema\.ts$|^web/packages/.*/safe-synthesizer/schema\.ts$'
# Read the schema if found
SCHEMA="$(git ls-files | rg '^web/packages/.*/generated/safe-synthesizer/schema\.ts$|^web/packages/.*/safe-synthesizer/schema\.ts$' | head -n1)"
if [ -n "$SCHEMA" ]; then
wc -l "$SCHEMA"
rg -n "export (type|interface) SafeSynthesizerJob|workspace|name" "$SCHEMA" -A3 -B3
fiRepository: NVIDIA-NeMo/nemo-platform
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the generated SafeSynthesizerJob schema
SCHEMA="$(git ls-files | rg '^web/packages/.*/generated/safe-synthesizer/schema\.ts$|^web/packages/.*/safe-synthesizer/schema\.ts$' | head -n1)"
echo "$SCHEMA"
wc -l "$SCHEMA"
sed -n '1,260p' "$SCHEMA" | rg -n "SafeSynthesizerJob|workspace|name|id|status" -A3 -B3Repository: NVIDIA-NeMo/nemo-platform
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the generated SafeSynthesizerJob schema
SCHEMA="$(git ls-files | rg '^web/packages/.*/generated/safe-synthesizer/schema\.ts$|^web/packages/.*/safe-synthesizer/schema\.ts$' | head -n1)"
echo "$SCHEMA"
wc -l "$SCHEMA"
sed -n '1,260p' "$SCHEMA" | rg -n "SafeSynthesizerJob|workspace|name|id|status" -A3 -B3Repository: NVIDIA-NeMo/nemo-platform
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the generated safe-synthesizer schema file path
git ls-files | rg 'safe[-_]synthesizer/.+schema\.ts$|safeSynthesizer/.+schema\.ts$|safe-synthesizer/schema'Repository: NVIDIA-NeMo/nemo-platform
Length of output: 163
Guard bulk deletes up front. filter((job) => job.workspace && job.name) doesn’t narrow the type, so job.workspace! still masks missing identifiers. Use a type guard and fail the batch if any selected row is incomplete; Promise.all also stops on the first rejection while other deletes keep running.
🤖 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/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx`
around lines 72 - 84, The bulk delete flow in handleDeleteJobs currently filters
incomplete rows but still relies on non-null assertions, and Promise.all will
reject on the first failure while other deletions continue. Update
SafeSynthesizerJobsDataView’s handleDeleteJobs to use a proper type guard that
narrows jobs with both workspace and name, validate the full selection up front
and abort the batch if any row is incomplete, and adjust the deletion loop so
all valid jobs are handled with clear per-job error reporting without masking
missing identifiers.
Source: Coding guidelines
Comment on lines
+45
to
+47
| const trigger = isValidElement(slotTrigger) ? ( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| cloneElement(slotTrigger as React.ReactElement<any>, { onClick: openTrigger }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsxRepository: NVIDIA-NeMo/nemo-platform
Length of output: 2544
🏁 Script executed:
set -euo pipefail
FILE='web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsx'
echo '--- outline ---'
ast-grep outline "$FILE" --view expanded || true
echo '--- relevant excerpt ---'
sed -n '1,220p' "$FILE"
echo '--- search slotTrigger usage/type ---'
rg -n "slotTrigger|BulkDeleteModal" web/packages/studio/src/components/filesets/FilesetFileExplorer -S
echo '--- search valid element typing patterns ---'
rg -n "isValidElement<|cloneElement\\(" web/packages/studio/src -SRepository: NVIDIA-NeMo/nemo-platform
Length of output: 6897
🏁 Script executed:
set -euo pipefail
echo '--- BulkActionsBar excerpt ---'
sed -n '1,140p' web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkActionsBar.tsx
echo '--- related modal excerpt ---'
sed -n '1,120p' web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx
echo '--- tests around custom trigger behavior ---'
sed -n '240,320p' web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsxRepository: NVIDIA-NeMo/nemo-platform
Length of output: 8074
🏁 Script executed:
set -euo pipefail
echo '--- BulkActionsBar excerpt ---'
sed -n '1,140p' web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkActionsBar.tsx
echo '--- related modal excerpt ---'
sed -n '1,120p' web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx
echo '--- tests around custom trigger behavior ---'
sed -n '240,320p' web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsxRepository: NVIDIA-NeMo/nemo-platform
Length of output: 8074
Compose the custom trigger handler and remove any. cloneElement(..., { onClick }) replaces any existing slotTrigger click logic; type the guard and call the original onClick before openTrigger.
🤖 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/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsx`
around lines 45 - 47, The BulkDeleteModal trigger wiring is overwriting any
existing click behavior and still relies on an `any` cast. Update the `trigger`
logic in `BulkDeleteModal` to properly type the `isValidElement` guard for
`slotTrigger`, then compose the cloned element’s existing `onClick` with
`openTrigger` instead of replacing it, so both handlers run in order without
using `any`.
Source: Coding guidelines
| <button | ||
| type="button" | ||
| aria-current={active ? 'page' : undefined} | ||
| title={new Date(session.mtime * 1000).toLocaleString()} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the full prompt in the tooltip.
line-clamp-2 truncates the visible label, and title now shows only the timestamp. Long sessions with the same prefix become hard to distinguish without opening them. Include the prompt in title again.
Suggested change
- title={new Date(session.mtime * 1000).toLocaleString()}
+ title={[
+ session.first_prompt || 'Claude Code session',
+ new Date(session.mtime * 1000).toLocaleString(),
+ ].join('\n')}Also applies to: 41-42
🤖 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/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsx`
at line 23, The HistorySessionButton tooltip is currently showing only the
timestamp, which hides the session prompt and makes similar sessions hard to
distinguish. Update the title prop in HistorySessionButton so it includes the
full prompt again along with the timestamp, using the session data already
available in this component, while keeping the visible label unchanged.
| }, | ||
| }); | ||
| const { mutateAsync: deleteDatasets, isPending } = useMutateMany(deleteDataset); | ||
| const { mutateAsync: deleteDatasets } = useMutateMany(deleteDataset); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t surface create-specific errors for deletes.
useMutateMany currently throws Failed to create...; this modal displays that message inline for delete failures. Use a delete-specific aggregator or make useMutateMany accept an action label.
🤖 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/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx`
at line 34, The delete modal is reusing a create-oriented error message from
useMutateMany, so delete failures can surface as “Failed to create...”. Update
DatasetBulkDeleteModal to use a delete-specific error aggregator or extend
useMutateMany to accept an action label, and make sure the error text shown from
deleteDatasets is phrased for deletes rather than creates.
Comment on lines
+36
to
+44
| const handleDelete = async (datasets: FilesetOutput[]) => { | ||
| const datasetsToDelete = datasets.filter( | ||
| (dataset): dataset is FilesetOutput & { workspace: string; name: string } => | ||
| !!(dataset.workspace && dataset.name) | ||
| ); | ||
| await deleteDatasets( | ||
| datasetsToDelete.map((dataset) => ({ workspace: dataset.workspace, name: dataset.name })) | ||
| ); | ||
| onConfirmSuccess(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail instead of silently skipping invalid datasets.
If any selected dataset lacks workspace or name, it is dropped and onConfirmSuccess() still runs, so the UI reports success without deleting that selection.
Proposed fix
const handleDelete = async (datasets: FilesetOutput[]) => {
const datasetsToDelete = datasets.filter(
(dataset): dataset is FilesetOutput & { workspace: string; name: string } =>
!!(dataset.workspace && dataset.name)
);
+ if (datasetsToDelete.length !== datasets.length) {
+ throw new Error('Cannot delete datasets without workspace and name.');
+ }
+
await deleteDatasets(
datasetsToDelete.map((dataset) => ({ workspace: dataset.workspace, name: dataset.name }))
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleDelete = async (datasets: FilesetOutput[]) => { | |
| const datasetsToDelete = datasets.filter( | |
| (dataset): dataset is FilesetOutput & { workspace: string; name: string } => | |
| !!(dataset.workspace && dataset.name) | |
| ); | |
| await deleteDatasets( | |
| datasetsToDelete.map((dataset) => ({ workspace: dataset.workspace, name: dataset.name })) | |
| ); | |
| onConfirmSuccess(); | |
| const handleDelete = async (datasets: FilesetOutput[]) => { | |
| const datasetsToDelete = datasets.filter( | |
| (dataset): dataset is FilesetOutput & { workspace: string; name: string } => | |
| !!(dataset.workspace && dataset.name) | |
| ); | |
| if (datasetsToDelete.length !== datasets.length) { | |
| throw new Error('Cannot delete datasets without workspace and name.'); | |
| } | |
| await deleteDatasets( | |
| datasetsToDelete.map((dataset) => ({ workspace: dataset.workspace, name: dataset.name })) | |
| ); | |
| onConfirmSuccess(); |
🤖 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/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx`
around lines 36 - 44, The handleDelete flow in DatasetBulkDeleteModal currently
filters out datasets missing workspace or name and still calls
onConfirmSuccess(), which can report success after silently skipping selections.
Update handleDelete to validate the full datasets array up front, and if any
FilesetOutput is missing workspace or name, fail the operation instead of
proceeding; only call deleteDatasets and onConfirmSuccess() when every selected
dataset is valid. Use the existing handleDelete, deleteDatasets, and
onConfirmSuccess symbols to locate the change.
Comment on lines
+49
to
+51
| const trigger = isValidElement(slotTrigger) ? ( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| cloneElement(slotTrigger as React.ReactElement<any>, { onClick: openTrigger }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx'
printf '\n--- file ---\n'
cat -n web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx | sed -n '1,220p'
printf '\n--- search slotTrigger / openTrigger / onClick ---\n'
rg -n "slotTrigger|openTrigger|onClick" web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsxRepository: NVIDIA-NeMo/nemo-platform
Length of output: 3664
Preserve the custom trigger click handler. cloneElement(..., { onClick: openTrigger }) drops any existing slotTrigger handler; compose it instead and remove the any cast.
🤖 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/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx`
around lines 49 - 51, The DatasetBulkDeleteModal trigger currently overwrites
any existing onClick on slotTrigger when cloning it in the trigger setup, so
update the cloneElement usage to compose the existing click handler with
openTrigger instead of replacing it, and remove the unnecessary
React.ReactElement<any> cast by using a properly typed element in this path.
Source: Coding guidelines
Contributor
|
Three fixes: - Remove stale ListExperimentsSort import (not in regenerated SDK since the spec defines sort as free-form string); use ListExperimentsParams['sort'] cast instead so the cast derives from whatever the SDK generates - Add aria-label="Delete selected datasets" to the bulk delete trigger in DatasetsTable so the existing test selector still finds it - Fix loading-state test in DatasetBulkDeleteModal: reuse pre-click button reference instead of re-querying, since LoadingButton appends a spinner with aria-label="Loading..." which changes the accessible name Signed-off-by: Alex Ray <alray@nvidia.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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility
Improvements