fix(desktop): show hover action buttons without requiring file selection#1761
Merged
Kitenite merged 6 commits intoFeb 25, 2026
Merged
Conversation
- Fix file row hover buttons (stage/unstage) only appearing after clicking to select the file first - Add discard/revert hover button on file rows with confirmation dialog - Add folder-level hover buttons for batch stage/unstage/discard - Add batch gitStageFiles/gitUnstageFiles endpoints to avoid index.lock races when staging multiple files - Reorder hover buttons to match section header pattern (discard left, stage/unstage right) - Fix invisible buttons remaining clickable via pointer-events-none - Align folder and file row right-side padding for consistent layout Fixes superset-sh#1759
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds backend batch git operations (gitStageFiles, gitUnstageFiles), exposes them as tRPC procedures (stageFiles, unstageFiles), and threads batch staging/unstaging through UI mutations and component prop propagation to enable folder- and group-level batch actions and discard confirmation dialogs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as React UI
participant tRPC as tRPC Router
participant Backend as Backend Logic
participant Git as Git
User->>UI: Click "Stage All" / "Unstage All"
UI->>tRPC: Call stageFiles/unstageFiles(worktreePath, filePaths[])
tRPC->>Backend: Invoke gitStageFiles/gitUnstageFiles
Backend->>Backend: Validate worktree & each path
Backend->>Git: Execute single git command (git add / git reset HEAD) with all paths
Git-->>Backend: Return result
Backend-->>tRPC: Return { success: boolean }
tRPC-->>UI: Response
UI->>UI: Refetch state / show toast
UI-->>User: Updated changes view
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListTree.tsx (1)
166-172:⚠️ Potential issue | 🔴 CriticalFolder-level discard currently bypasses a confirmation boundary.
handleDiscardAlldirectly loopsonDiscard(file)for every file. In the current wiring, that executes destructive mutations immediately, so one click can discard/delete an entire folder without a single confirm step.💡 Proposed direction
interface TreeNodeComponentProps { ... onDiscard?: (file: ChangedFile) => void; + onDiscardFiles?: (files: ChangedFile[]) => void; } -const handleDiscardAll = useCallback(() => { - if (!onDiscard) return; - const files = collectFilesFromNode(node); - for (const file of files) { - onDiscard(file); - } -}, [node, onDiscard]); +const handleDiscardAll = useCallback(() => { + onDiscardFiles?.(collectFilesFromNode(node)); +}, [node, onDiscardFiles]); ... - onDiscardAll={onDiscard ? handleDiscardAll : undefined} + onDiscardAll={onDiscardFiles ? handleDiscardAll : undefined}Then handle a single folder-level confirmation upstream before running the batch discard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListTree.tsx` around lines 166 - 172, handleDiscardAll currently calls onDiscard(file) in a loop causing immediate destructive actions; change it to request a single folder-level confirmation and only run the batch discard if confirmed. Specifically, update handleDiscardAll (which uses collectFilesFromNode(node) and onDiscard) to: 1) gather files via collectFilesFromNode(node), 2) open/show a single confirmation (or call a provided onRequestDiscardBatch/onConfirmDiscard callback) with summary metadata (e.g., folder name and count), and 3) upon confirmation invoke a batch discard API (either a new onDiscardBatch(files) prop or call onDiscard for each file after confirmation). Ensure the new flow references handleDiscardAll, collectFilesFromNode, and onDiscard (or the new onDiscardBatch) so the destructive loop is guarded by one confirmation.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListGrouped.tsx (1)
102-127:handleDiscardAlllacks a batch path, inconsistent with the PR's index.lock safety goal.
handleStageAll/handleUnstageAllwere refactored to prefer a single batch call to prevent index.lock races.handleDiscardAll(unchanged) still fires a per-fileonDiscardloop. Git discard operations (git checkout --for tracked files,git cleanfor untracked) also acquire the index lock, so rapidly discarding all files in a folder can race just as staging/unstaging does.Consider introducing an
onDiscardFiles?: (files: ChangedFile[]) => voidbatch prop (mirroringonStageFiles/onUnstageFiles) and applying the same fallback pattern:♻️ Proposed addition
interface FolderGroupItemProps { ... onStageFiles?: (files: ChangedFile[]) => void; onUnstageFiles?: (files: ChangedFile[]) => void; + onDiscardFiles?: (files: ChangedFile[]) => void; ... } function FolderGroupItem({ ... onStageFiles, onUnstageFiles, + onDiscardFiles, ... }: FolderGroupItemProps) { ... const handleDiscardAll = useCallback(() => { - if (!onDiscard) return; - for (const file of group.files) { - onDiscard(file); - } - }, [group.files, onDiscard]); + if (onDiscardFiles) { + onDiscardFiles(group.files); + } else if (onDiscard) { + for (const file of group.files) { + onDiscard(file); + } + } + }, [group.files, onDiscard, onDiscardFiles]);The same prop would need to be threaded through
FileListGroupedProps → FileListGrouped → FolderGroupItem, matching the pattern used foronStageFiles/onUnstageFiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListGrouped.tsx` around lines 102 - 127, Add a batch discard path to mirror the staging/unstaging change: introduce an optional prop onDiscardFiles?: (files: ChangedFile[]) => void (add it to FileListGroupedProps and thread it into FolderGroupItem), then update handleDiscardAll to call onDiscardFiles(group.files) when present and otherwise fall back to invoking onDiscard(file) per-file; keep the dependency array [group.files, onDiscard, onDiscardFiles] to preserve memoization and avoid index.lock races during bulk discards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx`:
- Line 477: The isActioning prop only checks unstageFileMutation.isPending ||
unstageFilesMutation.isPending, allowing row/folder buttons to be clickable
while section-level operations run; update the isActioning computation wherever
used (e.g., the prop set at the isActioning= line around ChangesView.tsx and the
similar block at lines ~547-552) to also include pending flags for all
section-level mutations (stageAll, unstageAll, discardAllUnstaged,
discardAllStaged) and any other row/folder mutations (stageFileMutation,
stageFilesMutation, discardFileMutation, etc.) so it becomes a combined boolean
(any of these .isPending) and pass that into the row/folder action components to
prevent overlapping git commands.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListTree.tsx`:
- Around line 166-172: handleDiscardAll currently calls onDiscard(file) in a
loop causing immediate destructive actions; change it to request a single
folder-level confirmation and only run the batch discard if confirmed.
Specifically, update handleDiscardAll (which uses collectFilesFromNode(node) and
onDiscard) to: 1) gather files via collectFilesFromNode(node), 2) open/show a
single confirmation (or call a provided onRequestDiscardBatch/onConfirmDiscard
callback) with summary metadata (e.g., folder name and count), and 3) upon
confirmation invoke a batch discard API (either a new onDiscardBatch(files) prop
or call onDiscard for each file after confirmation). Ensure the new flow
references handleDiscardAll, collectFilesFromNode, and onDiscard (or the new
onDiscardBatch) so the destructive loop is guarded by one confirmation.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListGrouped.tsx`:
- Around line 102-127: Add a batch discard path to mirror the staging/unstaging
change: introduce an optional prop onDiscardFiles?: (files: ChangedFile[]) =>
void (add it to FileListGroupedProps and thread it into FolderGroupItem), then
update handleDiscardAll to call onDiscardFiles(group.files) when present and
otherwise fall back to invoking onDiscard(file) per-file; keep the dependency
array [group.files, onDiscard, onDiscardFiles] to preserve memoization and avoid
index.lock races during bulk discards.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src/lib/trpc/routers/changes/security/git-commands.tsapps/desktop/src/lib/trpc/routers/changes/security/index.tsapps/desktop/src/lib/trpc/routers/changes/staging.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListGrouped.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileList/FileListTree.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx (2)
471-487:isActioningfor the staged section is now complete.The
onUnstageFilesbatch handler and the expandedisActioning(coveringunstageFileMutation,unstageFilesMutation,unstageAllMutation, anddiscardAllStagedMutation) correctly prevent concurrent git operations on the staged section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx` around lines 471 - 487, Staged-section concurrency handling is already implemented: ensure the onUnstageFiles handler uses unstageFilesMutation.mutate with worktreePath and filePaths (as currently done in onUnstageFiles) and keep the isActioning prop combining unstageFileMutation.isPending, unstageFilesMutation.isPending, unstageAllMutation.isPending, and discardAllStagedMutation.isPending so the staged view (component receiving worktreePath, projectId, category="staged", isExpandedView) remains disabled during any of those pending operations; no further code changes required.
546-565:isActioningfor the unstaged section is now complete.All six relevant pending flags are aggregated, correctly blocking row/folder buttons during any concurrent section-level operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx` around lines 546 - 565, The unstaged section's isActioning prop correctly aggregates all six pending flags (stageFilesMutation, stageFileMutation, stageAllMutation, discardChangesMutation, deleteUntrackedMutation, discardAllUnstagedMutation) to block UI during operations; apply the same aggregation pattern to the staged section's isActioning prop (where the component that renders category="staged" is defined) so both sections consistently check the same set of mutation.isPending flags and prevent concurrent actions.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx (2)
301-333: AlertDialog placement outside<Collapsible>is correct; minor:onDiscardAll?.()optional chain is unreachable.The dialog is only opened (via
setShowDiscardDialog(true)) from code paths that are themselves guarded byonDiscardAllbeing defined, so the?.at line 326 can never beundefined. This isn't a bug, just unnecessary defensive coding.♻️ Proposed nit
- onClick={() => { - setShowDiscardDialog(false); - onDiscardAll?.(); - }} + onClick={() => { + setShowDiscardDialog(false); + onDiscardAll(); + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx` around lines 301 - 333, The optional chaining on the discard callback is unnecessary because the dialog can only be opened when onDiscardAll is defined; in FolderRow update the AlertDialog Discard button click handler to call onDiscardAll directly (use onDiscardAll() or a non-null assertion if TypeScript strictness requires) and keep the setShowDiscardDialog(false) call as-is, ensuring you modify the onClick inside the AlertDialogFooter where setShowDiscardDialog and onDiscardAll are referenced.
132-132:hasActionis typed as a function union, notboolean.
onStageAll || onUnstageAll || onDiscardAllevaluates to the first truthy function (orundefined), givinghasActionthe type(() => void) | undefined. While this works as a JSX gate, an explicit boolean cast is clearer:♻️ Proposed nit
- const hasAction = onStageAll || onUnstageAll || onDiscardAll; + const hasAction = !!(onStageAll || onUnstageAll || onDiscardAll);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx` at line 132, The variable hasAction is currently assigned the first truthy function (onStageAll || onUnstageAll || onDiscardAll) giving it type (() => void) | undefined; change the assignment in FolderRow.tsx to produce an explicit boolean (e.g. use !!(onStageAll || onUnstageAll || onDiscardAll) or Boolean(onStageAll || onUnstageAll || onDiscardAll)) so hasAction is typed as boolean and remains usable as a JSX guard.apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx (1)
104-118:onErrorhandlers omit the failed file paths from the error message.Unlike the single-file variants (
stageFileMutation,unstageFileMutation) that includevariables.filePathin the toast, the batch mutations log a generic message. Consider acceptingvariablesin theonErrorcallback to surface the paths that failed:♻️ Proposed improvement
- const stageFilesMutation = electronTrpc.changes.stageFiles.useMutation({ - onSuccess: () => refetch(), - onError: (error) => { - console.error("Failed to stage files:", error); - toast.error(`Failed to stage files: ${error.message}`); - }, - }); + const stageFilesMutation = electronTrpc.changes.stageFiles.useMutation({ + onSuccess: () => refetch(), + onError: (error, variables) => { + console.error(`Failed to stage files ${variables.filePaths.join(", ")}:`, error); + toast.error(`Failed to stage files: ${error.message}`); + }, + }); - const unstageFilesMutation = electronTrpc.changes.unstageFiles.useMutation({ - onSuccess: () => refetch(), - onError: (error) => { - console.error("Failed to unstage files:", error); - toast.error(`Failed to unstage files: ${error.message}`); - }, - }); + const unstageFilesMutation = electronTrpc.changes.unstageFiles.useMutation({ + onSuccess: () => refetch(), + onError: (error, variables) => { + console.error(`Failed to unstage files ${variables.filePaths.join(", ")}:`, error); + toast.error(`Failed to unstage files: ${error.message}`); + }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx` around lines 104 - 118, The onError handlers for stageFilesMutation and unstageFilesMutation should include the failed file paths from the mutation variables—update the onError signature to accept (error, variables) for electronTrpc.changes.stageFiles.useMutation and electronTrpc.changes.unstageFiles.useMutation, and log/Toast the variables (e.g., variables.filePaths or variables.files) so the console.error and toast.error messages show which paths failed alongside error.message; keep the existing onSuccess refetch behavior unchanged and ensure you handle undefined variables defensively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx`:
- Around line 471-487: Staged-section concurrency handling is already
implemented: ensure the onUnstageFiles handler uses unstageFilesMutation.mutate
with worktreePath and filePaths (as currently done in onUnstageFiles) and keep
the isActioning prop combining unstageFileMutation.isPending,
unstageFilesMutation.isPending, unstageAllMutation.isPending, and
discardAllStagedMutation.isPending so the staged view (component receiving
worktreePath, projectId, category="staged", isExpandedView) remains disabled
during any of those pending operations; no further code changes required.
- Around line 546-565: The unstaged section's isActioning prop correctly
aggregates all six pending flags (stageFilesMutation, stageFileMutation,
stageAllMutation, discardChangesMutation, deleteUntrackedMutation,
discardAllUnstagedMutation) to block UI during operations; apply the same
aggregation pattern to the staged section's isActioning prop (where the
component that renders category="staged" is defined) so both sections
consistently check the same set of mutation.isPending flags and prevent
concurrent actions.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx`:
- Around line 104-118: The onError handlers for stageFilesMutation and
unstageFilesMutation should include the failed file paths from the mutation
variables—update the onError signature to accept (error, variables) for
electronTrpc.changes.stageFiles.useMutation and
electronTrpc.changes.unstageFiles.useMutation, and log/Toast the variables
(e.g., variables.filePaths or variables.files) so the console.error and
toast.error messages show which paths failed alongside error.message; keep the
existing onSuccess refetch behavior unchanged and ensure you handle undefined
variables defensively.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx`:
- Around line 301-333: The optional chaining on the discard callback is
unnecessary because the dialog can only be opened when onDiscardAll is defined;
in FolderRow update the AlertDialog Discard button click handler to call
onDiscardAll directly (use onDiscardAll() or a non-null assertion if TypeScript
strictness requires) and keep the setShowDiscardDialog(false) call as-is,
ensuring you modify the onClick inside the AlertDialogFooter where
setShowDiscardDialog and onDiscardAll are referenced.
- Line 132: The variable hasAction is currently assigned the first truthy
function (onStageAll || onUnstageAll || onDiscardAll) giving it type (() =>
void) | undefined; change the assignment in FolderRow.tsx to produce an explicit
boolean (e.g. use !!(onStageAll || onUnstageAll || onDiscardAll) or
Boolean(onStageAll || onUnstageAll || onDiscardAll)) so hasAction is typed as
boolean and remains usable as a JSX guard.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx
…scope - Add confirmation dialog to FolderRow discard action (both hover button and context menu) to prevent accidental bulk discard - Include section-level mutation states in isActioning props to disable row/folder buttons while stageAll/unstageAll/discardAll operations run
de1mat
force-pushed
the
fix/changes-view-hover-actions
branch
from
c1fc8da to
6d99db8
Compare
Contributor
Author
|
Hope this one can get in @Kitenite - it is quite annoying not being able to just hover over the files and click the + or - to stage or unstage them. Also not being able to stage a bunch of files at once, even from the menu option when you right-click on a folder ... it will stage the first file and error on all the rest. All fixed in this PR. |
Collaborator
|
Checking now @de1mat ! |
…r-actions # Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsx (1)
217-280: Indentation inconsistency between the discard and stage/unstage action blocks.Lines 219–241 (the
onDiscardbutton group) use deeper indentation than lines 242–279 (onStage/onUnstagebuttons), which appear to be siblings inside the samediv. This is a copy-paste artifact that makes the intent harder to read.♻️ Proposed fix (align indentation)
{hasAction && ( <div className="flex items-center shrink-0 opacity-0 pointer-events-none group-hover:opacity-100 group-hover:pointer-events-auto transition-opacity"> - {onDiscard && ( - <Tooltip> - <TooltipTrigger asChild> - <Button - variant="ghost" - size="icon" - className="size-5 hover:bg-accent hover:text-destructive" - onClick={(e) => { - e.stopPropagation(); - handleDiscardClick(); - }} - disabled={isActioning} - > - {isDeleteAction ? ( - <LuTrash2 className="size-3" /> - ) : ( - <LuUndo2 className="size-3" /> - )} - </Button> - </TooltipTrigger> - <TooltipContent side="bottom">{discardLabel}</TooltipContent> - </Tooltip> - )} + {onDiscard && ( + <Tooltip> + <TooltipTrigger asChild> + <Button + variant="ghost" + size="icon" + className="size-5 hover:bg-accent hover:text-destructive" + onClick={(e) => { + e.stopPropagation(); + handleDiscardClick(); + }} + disabled={isActioning} + > + {isDeleteAction ? ( + <LuTrash2 className="size-3" /> + ) : ( + <LuUndo2 className="size-3" /> + )} + </Button> + </TooltipTrigger> + <TooltipContent side="bottom">{discardLabel}</TooltipContent> + </Tooltip> + )} {onStage && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsx` around lines 217 - 280, The JSX indentation for the sibling action blocks inside the hasAction container is inconsistent: the onDiscard block (using onDiscard, Tooltip, Button, handleDiscardClick, discardLabel) is indented more deeply than the subsequent onStage and onUnstage blocks (using onStage, onUnstage, Tooltip, Button). Fix by aligning the onStage and onUnstage JSX to the same indentation level as the onDiscard block so all three conditional siblings under the div with className "flex items-center ..." have consistent indentation and visual grouping.apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx (1)
301-333: Consider usingAlertDialogCancel/AlertDialogActionfor proper accessibility semantics.The AlertDialog uses plain
Buttoncomponents instead ofAlertDialogCancel/AlertDialogAction. The Radix UI composition expects these primitives to correctly wire accessibility roles (alertdialog's action/cancel semantics). TheonOpenChangeescape-key behavior still works, but screen-reader users lose the semantic close/confirm role mapping. Consistent with the pattern inFileItem.tsx.♻️ Proposed refactor
+import { + AlertDialogAction, + AlertDialogCancel, + ... +} from "@superset/ui/alert-dialog"; ... <AlertDialogFooter className="px-4 pb-4 pt-2 flex-row justify-end gap-2"> - <Button - variant="ghost" - size="sm" - className="h-7 px-3 text-xs" - onClick={() => setShowDiscardDialog(false)} - > - Cancel - </Button> - <Button - variant="destructive" - size="sm" - className="h-7 px-3 text-xs" - onClick={() => { - setShowDiscardDialog(false); - onDiscardAll?.(); - }} - > - Discard - </Button> + <AlertDialogCancel asChild> + <Button variant="ghost" size="sm" className="h-7 px-3 text-xs"> + Cancel + </Button> + </AlertDialogCancel> + <AlertDialogAction asChild onClick={onDiscardAll}> + <Button variant="destructive" size="sm" className="h-7 px-3 text-xs"> + Discard + </Button> + </AlertDialogAction> </AlertDialogFooter>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx` around lines 301 - 333, Replace the plain Button components inside the AlertDialog in FolderRow with the Radix-aware primitives AlertDialogCancel and AlertDialogAction so accessibility semantics are preserved; keep the same props/styles (variant, size, className) and handlers: use AlertDialogCancel for the Cancel button to call setShowDiscardDialog(false) (or simply let the primitive close the dialog) and use AlertDialogAction for the Discard button to close the dialog and call onDiscardAll (ensure onDiscardAll is invoked after closing if needed). Ensure these replacements occur within the existing AlertDialogContent/Footer markup so showDiscardDialog, setShowDiscardDialog, and onDiscardAll behave the same while gaining proper action/cancel semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx`:
- Around line 320-331: The click handler calls onDiscardAll() which is typed as
(() => void) | undefined; change the handler in the Button inside FolderRow (the
onClick that currently calls setShowDiscardDialog(false); onDiscardAll();) to
guard/call safely—e.g., use optional chaining (onDiscardAll?.()) or an explicit
if (onDiscardAll) onDiscardAll()—so TypeScript knows the call is safe while
still closing the dialog via setShowDiscardDialog(false).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsx`:
- Around line 217-280: The JSX indentation for the sibling action blocks inside
the hasAction container is inconsistent: the onDiscard block (using onDiscard,
Tooltip, Button, handleDiscardClick, discardLabel) is indented more deeply than
the subsequent onStage and onUnstage blocks (using onStage, onUnstage, Tooltip,
Button). Fix by aligning the onStage and onUnstage JSX to the same indentation
level as the onDiscard block so all three conditional siblings under the div
with className "flex items-center ..." have consistent indentation and visual
grouping.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx`:
- Around line 301-333: Replace the plain Button components inside the
AlertDialog in FolderRow with the Radix-aware primitives AlertDialogCancel and
AlertDialogAction so accessibility semantics are preserved; keep the same
props/styles (variant, size, className) and handlers: use AlertDialogCancel for
the Cancel button to call setShowDiscardDialog(false) (or simply let the
primitive close the dialog) and use AlertDialogAction for the Discard button to
close the dialog and call onDiscardAll (ensure onDiscardAll is invoked after
closing if needed). Ensure these replacements occur within the existing
AlertDialogContent/Footer markup so showDiscardDialog, setShowDiscardDialog, and
onDiscardAll behave the same while gaining proper action/cancel semantics.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/changes/security/git-commands.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FileItem/FileItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/FolderRow/FolderRow.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts
Contributor
Author
|
Did you want me to sort those new issues @Kitenite ? |
Collaborator
|
No worries likely good to merge just reviewing now |
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.
Description
File row hover action buttons (stage/unstage) in the Changes View sidebar only appeared after the user clicked to select the file first. This fix ensures they show on hover regardless of selection state.
Additionally adds several UX improvements to the hover action buttons:
gitStageFiles/gitUnstageFiles) to stage/unstage multiple files in a single git command, avoiding index.lock racesRelated Issues
Fixes #1759
Type of Change
Screenshot
Testing
Summary by CodeRabbit
New Features
UI/UX Improvements
Behavior