refactor(studio): consolidate four delete modals into generic BulkDeleteModal#467
Open
aray12 wants to merge 12 commits into
Open
refactor(studio): consolidate four delete modals into generic BulkDeleteModal#467aray12 wants to merge 12 commits into
aray12 wants to merge 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
💤 Files with no reviewable changes (4)
📝 Walkthrough>BulkDeleteModal: success or throwsBulkDeleteModal->>Consumer: onClose() |
![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
Contributor
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsx (1)
11-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract props into an interface and add an explicit return type.
This exported component still uses an inline object shape and inferred JSX return, which breaks the TypeScript rules used under
web/**/*.tsx.Suggested refactor
+interface HistorySessionButtonProps { + active: boolean; + onSelect: () => void; + session: ClaudeCodeHistorySession; +} + export const HistorySessionButton = ({ active, onSelect, session, -}: { - active: boolean; - onSelect: () => void; - session: ClaudeCodeHistorySession; -}) => ( +}: HistorySessionButtonProps): React.JSX.Element => (As per coding guidelines, "Prefer
interfaceovertypefor object shapes and contracts in TypeScript" and "Use explicit return types for public APIs and complex functions in TypeScript".🤖 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` around lines 11 - 19, Extract the props from HistorySessionButton into a named interface instead of the inline object type, then update the exported component signature to use that interface and add an explicit JSX return type. Keep the refactor scoped to HistorySessionButton so the public API is clear and the component satisfies the TypeScript rules under web/**/*.tsx.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/api/common/useMutateMany.ts`:
- Around line 33-35: The error aggregation in useMutateMany assumes every
rejection has a message property, which breaks when a failure reason is null, a
string, or a plain object. Update the failedItems handling in useMutateMany to
safely normalize each failure.reason into a readable string before joining, and
preserve the existing action/items summary in the thrown Error. Use the
failure.reason and failedItems mapping logic as the place to add a non-Error
guard.
In
`@web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.test.tsx`:
- Around line 303-305: The pending-state assertion in BulkDeleteModal should
verify both actions, not just the Cancel button. Update the test around the
dialog assertions to also check the primary Delete button in the same state
after the mutation starts, using the existing dialog and button queries in
BulkDeleteModal/index.test.tsx so the test fails if Delete remains enabled
during loading.
---
Nitpick comments:
In
`@web/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsx`:
- Around line 11-19: Extract the props from HistorySessionButton into a named
interface instead of the inline object type, then update the exported component
signature to use that interface and add an explicit JSX return type. Keep the
refactor scoped to HistorySessionButton so the public API is clear and the
component satisfies the TypeScript rules under web/**/*.tsx.
🪄 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: 130adc4c-0503-4c70-b723-3d8e2feed404
📒 Files selected for processing (7)
web/packages/studio/src/api/common/useMutateMany.tsweb/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.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/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsxweb/packages/studio/src/routes/agents/ClaudeCodeChatRoute/historyPanel/HistorySessionButton.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/packages/studio/src/components/dataViews/DataDesignerJobsDataView/index.tsx
- web/packages/studio/src/components/dataViews/SafeSynthesizerJobsDataView/index.tsx
- web/packages/studio/src/routes/FilesetListRoute/DatasetBulkDeleteModal/index.tsx
- web/packages/studio/src/components/filesets/FilesetFileExplorer/BulkDeleteModal/index.tsx
…eteModal Signed-off-by: Alex Ray <alray@nvidia.com>
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>
…ew bulk delete Validates all selected jobs have workspace and name before starting any deletions, rather than silently filtering out invalid rows and reporting success on a partial batch. Signed-off-by: Alex Ray <alray@nvidia.com>
…aView bulk delete Same up-front validation as the DataDesigner fix: aborts the entire batch with an error if any selected job is missing workspace or name, rather than silently skipping invalid rows. Signed-off-by: Alex Ray <alray@nvidia.com>
…eleteModal Replace the cloneElement override that dropped any existing onClick with one that calls the original handler first, then opens the modal. Removes the React.ReactElement<any> cast by typing via isValidElement<TriggerProps>. Signed-off-by: Alex Ray <alray@nvidia.com>
…t skip - Compose slotTrigger's existing onClick with openTrigger instead of replacing it; removes React.ReactElement<any> cast. - Throw an error if any selected dataset lacks workspace or name rather than silently skipping and reporting success on a partial batch. Signed-off-by: Alex Ray <alray@nvidia.com>
Add an optional `action` param (default "create") so callers can pass "delete" and get a correctly-phrased error instead of "Failed to create N items". Update DatasetBulkDeleteModal to pass action: 'delete'. Signed-off-by: Alex Ray <alray@nvidia.com>
The visible label is line-clamped to 2 lines, so sessions with similar prefixes become hard to distinguish. Include the first prompt alongside the timestamp in the title attribute so hovering reveals the full text. Signed-off-by: Alex Ray <alray@nvidia.com>
…BulkDeleteModal Add a test that holds the delete mutation in-flight and asserts the Cancel button is disabled, exercising the isPending path that was dropped during the consolidation refactor. Signed-off-by: Alex Ray <alray@nvidia.com>
failure.reason.message crashes when a promise rejects with a non-Error value. Normalize via instanceof check with String() fallback. Signed-off-by: Alex Ray <alray@nvidia.com>
The delete button's accessible name becomes "Delete Loading..." when the spinner mounts, so use /delete/i to match both states. Adds the missing assertion for the submit button alongside the existing Cancel check. Signed-off-by: Alex Ray <alray@nvidia.com>
… return type Extract inline props into a named interface and annotate the component with an explicit React.JSX.Element return type per TypeScript guidelines. Signed-off-by: Alex Ray <alray@nvidia.com>
aray12
force-pushed
the
astd-32-merge-near-rebased
branch
from
bd62185 to
dca7ad2
Compare
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