feat: use dialog instead of custom generic modal#1020
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR replaces several custom Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RobotCreate as RobotCreate.tsx
participant OptState as OptimisticState
participant LLMAPI as createLLMRobot API
participant RecordingAPI as createAndRunRecording API
participant Cache as Cache Invalidation
participant Nav as Navigation
User->>RobotCreate: Click "Create & Run"
RobotCreate->>RobotCreate: Validate name & prompt, check duplicates
RobotCreate->>OptState: addOptimisticRobot(tempRobot)
RobotCreate->>LLMAPI: createLLMRobot(provider, model, creds)
LLMAPI-->>RobotCreate: returns created robot
RobotCreate->>OptState: removeOptimisticRobot()
RobotCreate->>Cache: invalidate recordings
RobotCreate->>OptState: addOptimisticRun(tempRun)
RobotCreate->>RecordingAPI: createAndRunRecording(robotId)
RecordingAPI-->>RobotCreate: returns run
RobotCreate->>Cache: invalidate runs/recordings
RobotCreate->>Nav: navigate to run page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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📝 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/recorder/SaveRecording.tsx (1)
214-224: Dead code:modalStyleis no longer used.After migrating to
Dialog, themodalStyleconstant is unused. Consider removing it.🧹 Remove unused code
-const modalStyle = { - top: '25%', - left: '50%', - transform: 'translate(-50%, -50%)', - width: '30%', - backgroundColor: 'background.paper', - p: 4, - height: 'fit-content', - display: 'block', - padding: '20px', -};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/recorder/SaveRecording.tsx` around lines 214 - 224, Remove the dead constant modalStyle from the SaveRecording.tsx file: delete the modalStyle declaration block and ensure there are no remaining references to modalStyle in the SaveRecording component (or other functions in the file); if any reference remains, replace it with the Dialog props or styles used by the new Dialog implementation.src/components/browser/BrowserRecordingSave.tsx (1)
220-246: Inconsistency: Reset modal still usesGenericModalwhile discard usesDialog.Within the same component, the discard confirmation was migrated to
Dialogbut the reset confirmation remains onGenericModal. Consider migrating this modal as well for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/browser/BrowserRecordingSave.tsx` around lines 220 - 246, The reset confirmation still uses GenericModal (openResetModal, setOpenResetModal, performReset, modalStyle) while the discard flow was migrated to Dialog; replace GenericModal with the same Dialog-based pattern used for discard: render a Dialog controlled by openResetModal, move the Box contents into DialogTitle/DialogContent/DialogActions, wire onClose to setOpenResetModal(false), call performReset from the confirm Button and keep the same translations and styles (migrating inline sx into DialogAction Button props or sx) so behavior and handlers remain identical to the original.src/components/robot/pages/RobotCreate.tsx (2)
33-33: Dead import:GenericModalis no longer used.After migrating to
Dialog, this import is unused.🧹 Remove unused import
-import { GenericModal } from '../../ui/GenericModal';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/robot/pages/RobotCreate.tsx` at line 33, Remove the dead import of GenericModal from the top of RobotCreate (the import line referencing GenericModal) since the component now uses Dialog; delete the unused import statement and verify there are no remaining references to GenericModal in RobotCreate.tsx, then run the linter/TS compile to confirm no unused-import errors remain.
1253-1263: Dead code:modalStyleis no longer used.After migrating to
Dialog, this constant is unused.🧹 Remove unused code
-const modalStyle = { - top: '50%', - left: '50%', - transform: 'translate(-50%, -50%)', - width: '30%', - backgroundColor: 'background.paper', - p: 4, - height: 'fit-content', - display: 'block', - padding: '20px', -};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/robot/pages/RobotCreate.tsx` around lines 1253 - 1263, Remove the dead constant modalStyle which is no longer used after migrating to Dialog; locate the top-level const modalStyle declaration in RobotCreate (the object with top/left/transform/width/backgroundColor/p/height/display/padding) and delete it and any related unused imports or references so the file no longer contains this unused constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/robot/pages/RobotCreate.tsx`:
- Around line 604-608: The optimistic run can be left orphaned when the LLM run
fails to start: ensure we call invalidateRuns() whenever robot creation or
run-start fails. Specifically, add invalidateRuns() to the early failure path
around createLLMRobot / the block that checks result and result.robot (the
existing notify('error', ...) return path) and, importantly, add
invalidateRuns() where createAndRunRecording() returns without a runId (the
failure handling after the optimistic run is added) so the UI refreshes and
removes the optimistic run.
- Around line 646-651: The catch block currently calls removeOptimisticRobot,
invalidateRecordings, and notify but forgets to clean up the optimistic run
added earlier (around line 628) which leaves a stale "running" entry; update the
catch to also remove or invalidate that optimistic run by calling the same
cleanup used when adding it (e.g., call removeOptimisticRun(tempRunId) or
queryClient.invalidateQueries(['runs', robotId]) ), ensuring tempRunId (or the
run identifier created when adding the optimistic run) is in scope so the run
cache is removed/invalidated alongside removeOptimisticRobot and
invalidateRecordings.
In `@src/components/robot/RecordingsTable.tsx`:
- Around line 595-604: Wrap the header row in a proper TableHead: replace the
standalone TableRow that renders the column headers (the block rendering
TableRow with columns.map and MemoizedTableCell) by enclosing that TableRow
inside a <TableHead> element and ensure TableHead is imported at the top of the
file; keep the existing TableRow, columns mapping, and MemoizedTableCell
unchanged except for the new TableHead wrapper so accessibility and MUI
structure warnings are resolved.
---
Nitpick comments:
In `@src/components/browser/BrowserRecordingSave.tsx`:
- Around line 220-246: The reset confirmation still uses GenericModal
(openResetModal, setOpenResetModal, performReset, modalStyle) while the discard
flow was migrated to Dialog; replace GenericModal with the same Dialog-based
pattern used for discard: render a Dialog controlled by openResetModal, move the
Box contents into DialogTitle/DialogContent/DialogActions, wire onClose to
setOpenResetModal(false), call performReset from the confirm Button and keep the
same translations and styles (migrating inline sx into DialogAction Button props
or sx) so behavior and handlers remain identical to the original.
In `@src/components/recorder/SaveRecording.tsx`:
- Around line 214-224: Remove the dead constant modalStyle from the
SaveRecording.tsx file: delete the modalStyle declaration block and ensure there
are no remaining references to modalStyle in the SaveRecording component (or
other functions in the file); if any reference remains, replace it with the
Dialog props or styles used by the new Dialog implementation.
In `@src/components/robot/pages/RobotCreate.tsx`:
- Line 33: Remove the dead import of GenericModal from the top of RobotCreate
(the import line referencing GenericModal) since the component now uses Dialog;
delete the unused import statement and verify there are no remaining references
to GenericModal in RobotCreate.tsx, then run the linter/TS compile to confirm no
unused-import errors remain.
- Around line 1253-1263: Remove the dead constant modalStyle which is no longer
used after migrating to Dialog; locate the top-level const modalStyle
declaration in RobotCreate (the object with
top/left/transform/width/backgroundColor/p/height/display/padding) and delete it
and any related unused imports or references so the file no longer contains this
unused constant.
🪄 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: ca9e65b8-4db0-4c47-bf59-3c01bff40213
📒 Files selected for processing (5)
src/components/browser/BrowserRecordingSave.tsxsrc/components/recorder/SaveRecording.tsxsrc/components/robot/RecordingsTable.tsxsrc/components/robot/pages/RobotCreate.tsxsrc/components/run/ColapsibleRow.tsx
Comment on lines
+646
to
+651
| } catch (error: any) { | ||
| console.error('Error in AI robot creation:', error); | ||
| removeOptimisticRobot(tempRobotId); | ||
| invalidateRecordings(); | ||
| notify('error', error?.message || 'Failed to create and run AI robot'); | ||
| } |
There was a problem hiding this comment.
Orphaned optimistic run on error — run cache not cleaned up.
When robot creation or run execution fails, the error handler removes the optimistic robot and invalidates recordings, but it does not remove or invalidate the optimistic run that was added at line 628. This leaves a stale "running" entry in the runs cache.
🐛 Proposed fix
} catch (error: any) {
console.error('Error in AI robot creation:', error);
removeOptimisticRobot(tempRobotId);
invalidateRecordings();
+ invalidateRuns();
notify('error', error?.message || 'Failed to create and run AI robot');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/robot/pages/RobotCreate.tsx` around lines 646 - 651, The catch
block currently calls removeOptimisticRobot, invalidateRecordings, and notify
but forgets to clean up the optimistic run added earlier (around line 628) which
leaves a stale "running" entry; update the catch to also remove or invalidate
that optimistic run by calling the same cleanup used when adding it (e.g., call
removeOptimisticRun(tempRunId) or queryClient.invalidateQueries(['runs',
robotId]) ), ensuring tempRunId (or the run identifier created when adding the
optimistic run) is in scope so the run cache is removed/invalidated alongside
removeOptimisticRobot and invalidateRecordings.
Comment on lines
+595
to
+604
| <TableRow> | ||
| {columns.map((column) => ( | ||
| <MemoizedTableCell | ||
| key={column.id} | ||
| style={{ minWidth: column.minWidth }} | ||
| > | ||
| {column.label} | ||
| </MemoizedTableCell> | ||
| ))} | ||
| </TableRow> |
There was a problem hiding this comment.
Table header row should be wrapped in <TableHead>.
The header TableRow is rendered directly inside <Table> without a <TableHead> wrapper. This impacts accessibility (screen readers expect proper table structure) and may cause console warnings from MUI.
🔧 Proposed fix
<Table stickyHeader aria-label="sticky table">
+ <TableHead>
<TableRow>
{columns.map((column) => (
<MemoizedTableCell
key={column.id}
style={{ minWidth: column.minWidth }}
>
{column.label}
</MemoizedTableCell>
))}
</TableRow>
+ </TableHead>
<TableBody>You'll need to add TableHead to the imports at the top of the file.
📝 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
| <TableRow> | |
| {columns.map((column) => ( | |
| <MemoizedTableCell | |
| key={column.id} | |
| style={{ minWidth: column.minWidth }} | |
| > | |
| {column.label} | |
| </MemoizedTableCell> | |
| ))} | |
| </TableRow> | |
| <Table stickyHeader aria-label="sticky table"> | |
| <TableHead> | |
| <TableRow> | |
| {columns.map((column) => ( | |
| <MemoizedTableCell | |
| key={column.id} | |
| style={{ minWidth: column.minWidth }} | |
| > | |
| {column.label} | |
| </MemoizedTableCell> | |
| ))} | |
| </TableRow> | |
| </TableHead> | |
| <TableBody> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/robot/RecordingsTable.tsx` around lines 595 - 604, Wrap the
header row in a proper TableHead: replace the standalone TableRow that renders
the column headers (the block rendering TableRow with columns.map and
MemoizedTableCell) by enclosing that TableRow inside a <TableHead> element and
ensure TableHead is imported at the top of the file; keep the existing TableRow,
columns mapping, and MemoizedTableCell unchanged except for the new TableHead
wrapper so accessibility and MUI structure warnings are resolved.
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.
Summary by CodeRabbit
New Features
Style