Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors the AddTaskModal to fetch and manage tasks internally (removing tasks prop / MOCK_TASKS), extracts NewTabButton for tab creation, adds a Radix-based dropdown-menu UI, updates Dialog to accept a modal prop, and extends useTasks with a handleClearStatus API and safer progress handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MainScreen
participant AddTaskModal
participant useTaskData
participant API
participant useTasks
User->>MainScreen: Click "Add Task"
MainScreen->>AddTaskModal: open(isOpen=true)
AddTaskModal->>useTaskData: fetch tasks (mode=list, apiBaseUrl)
useTaskData->>API: GET /tasks
API-->>useTaskData: APITask[]
useTaskData->>AddTaskModal: tasks, isLoadingTasks, tasksError
User->>AddTaskModal: select task OR switch to "New"
AddTaskModal->>useTasks: handleCreateTask(taskData)
useTasks->>useTasks: attach progress listener, create via IPC/API
useTasks-->>AddTaskModal: setupStatus / setupOutput updates
User->>AddTaskModal: Click Clear
AddTaskModal->>useTasks: onClearStatus()
useTasks-->>useTasks: reset setupStatus & setupOutput
sequenceDiagram
actor User
participant WorktreeList
participant NewTabButton
participant LocalStorage
participant IPC
WorktreeList->>NewTabButton: render(currentWorkspace, selectedWorktreeId)
NewTabButton->>LocalStorage: read "newTabDefaultType"
LocalStorage-->>NewTabButton: defaultType
User->>NewTabButton: click main button or dropdown item
NewTabButton->>LocalStorage: write "newTabDefaultType" (if changed)
NewTabButton->>IPC: ipcRenderer("tab-create", {type, worktreeId})
IPC-->>NewTabButton: newTabId
NewTabButton->>WorktreeList: onTabSelect(newTabId), onReload()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
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: 4
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal.tsx (2)
49-95: API integration looks good, but consider making the API URL configurable.The new
APITaskinterface and prop changes align well with the shift to API-driven task loading. However, the hardcoded defaultapiBaseUrl = "http://localhost:3000"may cause issues in different environments.Consider:
- Loading the API base URL from environment variables or configuration
- Documenting the expected API endpoint format for future maintainers
export const AddTaskModal: React.FC<AddTaskModalProps> = ({ isOpen, onClose, openTasks, onSelectTask, onCreateTask, initialMode = "list", branches = [], worktrees = [], isCreating = false, setupStatus, setupOutput, onClearStatus, - apiBaseUrl = "http://localhost:3000", + apiBaseUrl = process.env.REACT_APP_API_URL || "http://localhost:3000", }) => {
381-390: Consider user control over auto-transition behavior.The automatic transition back to list mode after 1.5 seconds provides a smooth UX, but some users might want to review the output longer before transitioning.
Consider one of these alternatives:
- Show a manual "Done" or "Back to List" button instead of auto-transitioning
- Increase the delay to 2-3 seconds for better readability
- Add a user preference for auto-transition behavior
The current implementation with cleanup is technically correct.
packages/ui/src/components/dropdown-menu.tsx (1)
65-86: Dropdown-menu wrapper API and styling look solidThe rest of the wrappers (
Group,Item,CheckboxItem,RadioGroup,RadioItem,Label,Separator,Shortcut,Sub,SubTrigger,SubContent) are idiomatic:
- Correctly forward props to Radix primitives.
- Expose extra props like
insetandvariantviadata-*attributes for styling.- Provide consistent
data-slothooks and Tailwind classes that match the rest of the UI kit.No functional issues stand out here.
If you find yourself repeating similar Tailwind strings across multiple components, a small shared
dropdownItemBaseClassconstant could reduce duplication, but that’s purely optional.Also applies to: 88-147, 149-242
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/NewTabButton.tsx (1)
1-32: State initialization and props forNewTabButtonlook good; consider guardinglocalStorageThe lazy
useStateinitializer fordefaultTabTypecorrectly reads fromlocalStorageand falls back to"terminal", and the props match howWorktreeListpassescurrentWorkspace,selectedWorktreeId,onTabSelect, andonReload. This is a clean separation of concerns.In environments where
localStoragemight not be available (e.g., certain test runners, non-browser tooling), this could throw. If you expect to render this component in such contexts, a small defensive tweak would make it more robust:-const [defaultTabType, setDefaultTabType] = useState<"terminal" | "preview">( - () => { - // Load from localStorage or default to "terminal" - const saved = localStorage.getItem("newTabDefaultType"); - return (saved === "preview" ? "preview" : "terminal") as - | "terminal" - | "preview"; - }, -); +const [defaultTabType, setDefaultTabType] = useState<"terminal" | "preview">( + () => { + try { + const saved = + typeof window !== "undefined" + ? window.localStorage.getItem("newTabDefaultType") + : null; + return saved === "preview" ? "preview" : "terminal"; + } catch { + return "terminal"; + } + }, +);Totally optional if this component only ever runs in an Electron renderer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/src/renderer/screens/main/MainScreen.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal.tsx(10 hunks)apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/WorktreeList.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/NewTabButton.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/hooks/useTasks.ts(3 hunks)packages/ui/package.json(2 hunks)packages/ui/src/components/dialog.tsx(1 hunks)packages/ui/src/components/dropdown-menu.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/WorktreeList.tsx (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/NewTabButton.tsx (1)
NewTabButton(18-152)
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal.tsx (4)
apps/desktop/src/renderer/screens/main/components/Layout/StatusIndicator.tsx (1)
TaskStatus(3-11)packages/db/src/schema/schema.ts (1)
tasks(109-148)apps/desktop/src/renderer/screens/main/components/Sidebar/components/CreateWorktreeModal/TerminalOutput.tsx (1)
TerminalOutput(11-84)apps/desktop/src/renderer/screens/main/components/Layout/Avatar.tsx (1)
Avatar(9-48)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/NewTabButton.tsx (1)
apps/desktop/src/shared/types.ts (1)
Workspace(70-82)
🪛 GitHub Actions: CI
packages/ui/package.json
[error] 1-1: lint:check failed due to multiple errors; see individual diagnostics. (Command failed with exit code 1)
packages/ui/src/components/dropdown-menu.tsx
[warning] 6-6: Formatter would have modified formatting in DropdownMenu.tsx (prettier formatting).
🔇 Additional comments (15)
apps/desktop/src/renderer/screens/main/MainScreen.tsx (2)
105-105: LGTM! Clean integration of status clearing callback.The
handleClearStatuscallback is correctly wired fromuseTaskstoAddTaskModal, enabling external reset of setup state.Also applies to: 216-216
121-121: No issues identified with the z-index change.The reduction from
z-50toz-30is a deliberate layering strategy. The trigger element is a narrow 2px hover strip on the left edge that only renders when the sidebar is hidden. Atz-30, it sits below modals and dialogs (z-50), preventing it from interfering with critical UI elements. The sidebar overlay remains atz-40, positioned above the trigger, allowing the hover-to-reveal interaction to work correctly. This stacking order is sound and consistent with the component's purpose.apps/desktop/src/renderer/screens/main/hooks/useTasks.ts (2)
62-65: LGTM! Clean utility function for status reset.The
handleClearStatusfunction provides a simple way to reset setup state and is correctly exposed in the hook's API.Also applies to: 244-244
191-193: LGTM! Appropriate fallback for success message.The logic correctly displays the detailed setup result when available and falls back to a generic success message otherwise.
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal.tsx (3)
97-125: LGTM! Clean helper functions for data transformation.Both
formatRelativeTimeandtransformAPITaskToUITaskare well-implemented and handle null values appropriately.
564-590: LGTM! Clear loading and error state handling.The conditional rendering provides good user feedback for loading, error, empty, and filtered states.
465-467: LGTM! Clear comment explaining intentional behavior.The comment helpfully documents that the form is intentionally kept visible during creation to show progress, unlike the previous behavior.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/index.ts (1)
1-2: Barrel export forNewTabButtonlooks goodThe re-export cleanly exposes
NewTabButtonvia thecomponents/NewTabButtondirectory and matches how it’s imported inWorktreeList.apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/WorktreeList.tsx (1)
3-4: ConfirmNewTabButtonvisibility gating onselectedWorktreeId
NewTabButtonis only rendered whenselectedWorktreeIdis truthy:{selectedWorktreeId && currentWorkspace && ( <NewTabButton … /> )}Given
selectedWorktreeIdisstring | null | undefined, this means:
- No “New Tab” button will appear when there’s no selected worktree.
- Previously, the new-tab affordance may have been visible regardless of selection (depending on the old implementation).
If the UX is that you can only create new tabs for an explicitly selected worktree, this is fine. If users should always be able to create a tab for the active worktree even when
selectedWorktreeIdis unset, you might want to fall back tocurrentWorkspace.activeWorktreeIdhere.Also applies to: 76-84
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/NewTabButton.tsx (2)
34-95: IPC-based tab creation handlers are correct; ensurewindow.ipcRendererexists in all render contexts
handleAddTerminalandhandleAddPreviewboth:
- Guard on
currentWorkspaceandselectedWorktreeId.- Invoke
"tab-create"viawindow.ipcRenderer.invoke.- On success, call
onTabSelect(selectedWorktreeId, newTabId)and thenonReload.- Catch and log errors.
This is a solid flow for the desktop renderer. Just be aware that
window.ipcRendereris Electron-specific; if this component is ever rendered in Storybook, tests, or other non-Electron browser contexts, clicking the button will throw unless you stubwindow.ipcRendereror guard against its absence:if (!("ipcRenderer" in window)) { console.error("ipcRenderer not available"); return; }Whether you add such a guard or rely on test setup stubs is a project-level choice.
97-151: New Tab button + dropdown UX is clean and matches the design intentThe “Arc-style” main button plus chevron dropdown is implemented cleanly:
handleCreateDefaultrespects the stored default tab type.- The dropdown uses
DropdownMenu+DropdownMenuTrigger asChildcorrectly.onMouseDown={(e) => e.stopPropagation()}on the chevron button prevents accidental triggering of the main button while still letting the dropdown open.- Menu items correctly update the default type when selected and then create the respective tab.
No functional issues here.
packages/ui/src/components/dropdown-menu.tsx (3)
39-55: The Tailwind syntax is valid—no changes needed.Tailwind supports the parentheses shorthand for CSS variables (e.g.,
max-h-(--my-var)), which is equivalent to usingvar(...)inside square-bracket arbitrary values. The classesmax-h-(--radix-dropdown-menu-content-available-height)andorigin-(--radix-dropdown-menu-content-transform-origin)will compile correctly as-is.
1-55: Update the formatter reference and correction commandThe project uses Biome for formatting (not Prettier), configured in the root package.json. The correct command is
bun formatfrom the repository root. Please run this to normalize the file before merging to ensure it matches repository style and keeps CI green.
7-18: Confirm the intentionalmodal = falsedefault overrideThe wrapper defaults
modaltofalse, but Radix's DropdownMenu.Root v2.1.16 documents the modal prop default as true. This means your wrapper deliberately inverts the Radix default. When modal is enabled, focus is trapped to the open menu and closing restores focus to the trigger.Since this is a new wrapper component with no backward-compat constraints, this design choice is valid if intentional—you may want all dropdowns in this package to be non-modal by default. Confirm this is the desired behavior for your UI library, otherwise consider removing the default and deferring to Radix's modal behavior.
packages/ui/package.json (1)
17-18: Export and dependency additions are correctly wired—verify lint status locallyThe new export:
"./dropdown-menu": "./src/components/dropdown-menu.tsx"and dependency:
"@radix-ui/react-dropdown-menu": "^2.1.16"are correctly formatted, syntactically valid, and follow the established pattern for other Radix UI components. The component file exists at the expected path.
However, lint failures reported by CI cannot be verified in this environment. Run
bun run lint:checklocally to confirm whether failures remain and whether they're related to these changes or pre-existing issues.
| {(isCreating || setupStatus) ? ( | ||
| <div className="flex-1 flex flex-col min-h-0 overflow-hidden"> | ||
| <ScrollArea className="flex-1 min-h-0"> | ||
| <div className="px-6 pt-6 pb-4"> | ||
| {/* Progress Section - shown while creating */} | ||
| {isCreating && ( | ||
| <div className="flex-1 flex flex-col space-y-3 overflow-hidden min-h-[200px]"> | ||
| <div className="flex items-center gap-2 text-sm text-neutral-300"> | ||
| <Loader2 size={16} className="animate-spin" /> | ||
| <span>{setupStatus || "Creating worktree..."}</span> | ||
| </div> | ||
|
|
||
| {setupOutput && ( | ||
| <div className="bg-neutral-900 rounded border border-neutral-700 overflow-hidden min-h-[200px]"> | ||
| <TerminalOutput | ||
| output={setupOutput} | ||
| className="w-full h-full" | ||
| /> | ||
| {setupOutput && ( | ||
| <div className="flex-1 bg-neutral-900 rounded border border-neutral-700 overflow-hidden min-h-[400px]"> | ||
| <TerminalOutput | ||
| output={setupOutput} | ||
| className="w-full h-full" | ||
| /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Error Display - shown when creation failed */} | ||
| {!isCreating && | ||
| setupStatus && | ||
| (setupStatus.toLowerCase().includes("failed") || | ||
| setupStatus.toLowerCase().includes("error")) && ( | ||
| <div className="flex flex-col space-y-3 min-h-[200px] pt-4"> | ||
| <div className="flex items-center gap-2 text-sm text-red-400 font-medium"> | ||
| <span>{setupStatus}</span> | ||
| </div> | ||
|
|
||
| {setupOutput && ( | ||
| <div className="bg-red-500/10 rounded border border-red-500/30 p-3 overflow-auto min-h-[200px]"> | ||
| <pre className="text-red-200 text-xs font-mono whitespace-pre-wrap"> | ||
| {setupOutput} | ||
| </pre> | ||
| {/* Success Display - shown when creation succeeded */} | ||
| {!isCreating && | ||
| setupStatus && | ||
| !setupStatus.toLowerCase().includes("failed") && | ||
| !setupStatus.toLowerCase().includes("error") && | ||
| (setupStatus.toLowerCase().includes("success") || | ||
| setupStatus.toLowerCase().includes("completed")) && ( | ||
| <div className="flex-1 flex flex-col space-y-3 overflow-hidden min-h-[200px]"> | ||
| <div className="flex items-center gap-2 text-sm text-green-400 font-medium"> | ||
| <span>{setupStatus}</span> | ||
| </div> | ||
|
|
||
| {setupOutput && ( | ||
| <div className="flex-1 bg-green-500/10 rounded border border-green-500/30 p-3 overflow-auto min-h-[400px]"> | ||
| <pre className="text-green-200 text-xs font-mono whitespace-pre-wrap"> | ||
| {setupOutput} | ||
| </pre> | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Worktree creation options */} | ||
| {(branches.length > 0 || worktrees.length > 0) && ( | ||
| <div className="space-y-3 pt-4"> | ||
| {branches.length > 0 && ( | ||
| {/* Error Display - shown when creation failed */} | ||
| {!isCreating && | ||
| setupStatus && | ||
| (setupStatus.toLowerCase().includes("failed") || | ||
| setupStatus.toLowerCase().includes("error")) && ( | ||
| <div className="flex-1 flex flex-col space-y-3 overflow-hidden min-h-[200px]"> | ||
| <div className="flex items-center gap-2 text-sm text-red-400 font-medium"> | ||
| <span>{setupStatus}</span> | ||
| </div> | ||
|
|
||
| {setupOutput && ( | ||
| <div className="flex-1 bg-red-500/10 rounded border border-red-500/30 p-3 overflow-auto min-h-[400px]"> | ||
| <pre className="text-red-200 text-xs font-mono whitespace-pre-wrap"> | ||
| {setupOutput} | ||
| </pre> | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </ScrollArea> | ||
| {/* Footer for progress view */} | ||
| <div className="px-6 py-4 border-t border-neutral-800 flex items-center justify-end gap-2 shrink-0"> | ||
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| onClick={onClose} | ||
| disabled={isCreating} | ||
| > | ||
| {isCreating ? "Creating..." : "Close"} | ||
| </Button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complex nested conditionals reduce readability.
The deeply nested conditions and string matching for status detection make this code harder to maintain and test.
Refactor to computed status and extracted components:
// Add a computed status type
type CreationStatus = 'idle' | 'creating' | 'success' | 'error';
// Add a helper function before the component
function getCreationStatus(
isCreating: boolean,
setupStatus?: string
): CreationStatus {
if (isCreating) return 'creating';
if (!setupStatus) return 'idle';
const status = setupStatus.toLowerCase();
if (status.includes('success') || status.includes('completed')) {
return 'success';
}
if (status.includes('failed') || status.includes('error')) {
return 'error';
}
return 'idle';
}
// In the component:
const creationStatus = getCreationStatus(isCreating, setupStatus);
// Simplified rendering:
{creationStatus !== 'idle' ? (
<div className="flex-1 flex flex-col min-h-0 overflow-hidden">
<ScrollArea className="flex-1 min-h-0">
<div className="px-6 pt-6 pb-4">
{creationStatus === 'creating' && (
<CreatingView status={setupStatus} output={setupOutput} />
)}
{creationStatus === 'success' && (
<SuccessView status={setupStatus} output={setupOutput} />
)}
{creationStatus === 'error' && (
<ErrorView status={setupStatus} output={setupOutput} />
)}
</div>
</ScrollArea>
<ProgressFooter
isCreating={isCreating}
onClose={onClose}
/>
</div>
) : (
// Form fields
)}This approach:
- Eliminates string matching throughout the component
- Makes testing easier (test the status function separately)
- Improves readability with named status constants
- Enables component extraction for each view
| // Only close modal and select worktree if modal is still open | ||
| if (isAddTaskModalOpen) { | ||
| // Close modal and reset state | ||
| setIsAddTaskModalOpen(false); | ||
| setSetupStatus(undefined); | ||
| setSetupOutput(undefined); | ||
|
|
||
| // Switch to the new worktree if available | ||
| if (result.worktree) { | ||
| setSelectedWorktreeId(result.worktree.id); | ||
| if (result.worktree.tabs && result.worktree.tabs.length > 0) { | ||
| handleTabSelect(result.worktree.id, result.worktree.tabs[0].id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Stale closure bug with isAddTaskModalOpen state.
The isAddTaskModalOpen value is captured when the async function starts (Line 140), but by the time this code executes after await handleWorktreeCreated(), the actual modal state may have changed. React state values in closures don't update.
Scenario: If the user closes the modal while creation is in progress, the stale isAddTaskModalOpen value will still be true, causing Lines 203-213 to execute incorrectly.
Solution: Use a ref to track the latest modal state:
export function useTasks({
currentWorkspace,
setSelectedWorktreeId,
handleTabSelect,
handleWorktreeCreated,
}: UseTasksProps) {
const [isAddTaskModalOpen, setIsAddTaskModalOpen] = useState(false);
+ const isAddTaskModalOpenRef = useRef(false);
const [addTaskModalInitialMode, setAddTaskModalInitialMode] = useState<"list" | "new">("list");
// ... other state
+ // Keep ref in sync with state
+ useEffect(() => {
+ isAddTaskModalOpenRef.current = isAddTaskModalOpen;
+ }, [isAddTaskModalOpen]);
const handleOpenAddTaskModal = (mode: "list" | "new" = "list") => {
setAddTaskModalInitialMode(mode);
setIsAddTaskModalOpen(true);
+ isAddTaskModalOpenRef.current = true;
// ...
};
const handleCloseAddTaskModal = () => {
if (isCreatingWorktree) return;
setIsAddTaskModalOpen(false);
+ isAddTaskModalOpenRef.current = false;
// ...
};
// In handleCreateTask, use the ref:
- if (isAddTaskModalOpen) {
+ if (isAddTaskModalOpenRef.current) {Alternatively, consider moving the modal-close and worktree-selection logic outside this async function, triggered by status changes via useEffect.
Committable suggestion skipped: line range outside the PR's diff.
| function Dialog({ | ||
| modal = false, | ||
| ...props | ||
| }: React.ComponentProps<typeof DialogPrimitive.Root>) { | ||
| return <DialogPrimitive.Root data-slot="dialog" {...props} />; | ||
| return <DialogPrimitive.Root data-slot="dialog" modal={modal} {...props} />; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Dialog’s new modal = false default may change existing behavior
By destructuring modal = false and always passing it to DialogPrimitive.Root, any existing <Dialog> usage that didn’t specify modal will now explicitly render with modal={false} instead of Radix’s default behavior (previously you were effectively passing modal={undefined}). If Radix’s default is modal (which it is for several primitives, including dropdown menu roots (lightrun.com)), this flips all existing dialogs to non‑modal: background remains interactive, focus may no longer be trapped, and body scroll/pointer‑event handling will change.
If that global behavior change isn’t intentional, consider either:
- Dropping the default and just forwarding the prop:
-function Dialog({
- modal = false,
- ...props
-}: React.ComponentProps<typeof DialogPrimitive.Root>) {
- return <DialogPrimitive.Root data-slot="dialog" modal={modal} {...props} />;
-}
+function Dialog({
+ modal,
+ ...props
+}: React.ComponentProps<typeof DialogPrimitive.Root>) {
+ return <DialogPrimitive.Root data-slot="dialog" modal={modal} {...props} />;
+}- Or, if you do want a non‑modal default only in specific places, keep Radix’s default here and pass
modal={false}explicitly at those call sites.
🌐 Web query:
What is the documented default value and behavior of the `modal` prop on `@radix-ui/react-dialog`’s `Dialog.Root`?
💡 Result:
Default: true — Dialog.Root’s modal prop defaults to true. When modal is true the dialog is modal: focus is trapped inside, interaction with the rest of the page is blocked (overlay/inert outside), body scroll is prevented, and typical modal behaviors (e.g. Esc to close, ARIA handling via Title/Description) are applied. [1]
Sources:
[1] Radix Primitives — Dialog (API Reference: Root → modal).
Dialog's new modal = false default changes existing behavior; requires fix
Radix UI's Dialog.Root component defaults to modal={true}, where focus is trapped, page interaction is blocked, body scroll is prevented, and modal behaviors are applied. By destructuring modal = false and always passing it to DialogPrimitive.Root, all existing <Dialog> usage without an explicit modal prop will now render non-modal—background remains interactive, focus is no longer trapped, and body scroll is enabled.
Fix by either:
- Dropping the default and forwarding the prop directly (let Radix's default apply):
-function Dialog({
- modal = false,
- ...props
-}: React.ComponentProps<typeof DialogPrimitive.Root>) {
- return <DialogPrimitive.Root data-slot="dialog" modal={modal} {...props} />;
-}
+function Dialog({
+ modal,
+ ...props
-}: React.ComponentProps<typeof DialogPrimitive.Root>) {
+ return <DialogPrimitive.Root data-slot="dialog" {...props} />;
+}- Or explicitly pass
modal={false}only at call sites that require non-modal behavior.
📝 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.
| function Dialog({ | |
| modal = false, | |
| ...props | |
| }: React.ComponentProps<typeof DialogPrimitive.Root>) { | |
| return <DialogPrimitive.Root data-slot="dialog" {...props} />; | |
| return <DialogPrimitive.Root data-slot="dialog" modal={modal} {...props} />; | |
| } | |
| function Dialog({ | |
| modal, | |
| ...props | |
| }: React.ComponentProps<typeof DialogPrimitive.Root>) { | |
| return <DialogPrimitive.Root data-slot="dialog" {...props} />; | |
| } |
🤖 Prompt for AI Agents
In packages/ui/src/components/dialog.tsx around lines 7 to 12, the component
currently sets modal = false by default and always forwards modal to
DialogPrimitive.Root which overrides Radix's default modal behavior; remove the
default assignment and forward modal only when provided (or simply omit modal
from the props spread) so Radix's default (modal=true) remains in effect, or
alternatively delete the modal prop here and only pass modal={false} at explicit
call sites that require non-modal behavior.
1 participant
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes / Reliability