Add typed browser operations, tree, and drag-drop support - #22
Merged
Conversation
messagesgoel-blip
added
track:browser
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @messagesgoel-blip, you have reached your weekly rate limit of 2500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds typed file-operation events and enablement, transfer intent types and creators, a lazy-loading folder tree + destination picker, ChangesFile Operations, Transfer, Folder Trees, and Drag-Drop System
Sequence DiagramsequenceDiagram
participant User
participant BrowserShell
participant isBrowserActionEnabled
participant createBrowserFileOperationEvent
participant onFileOperation
participant resolveBrowserTransferOperation
participant createBrowserTransferIntent
User->>BrowserShell: trigger toolbar/context action
BrowserShell->>isBrowserActionEnabled: check(action, selectedItems, item?)
isBrowserActionEnabled-->>BrowserShell: result
alt enabled
BrowserShell->>createBrowserFileOperationEvent: construct event
BrowserShell->>onFileOperation: emit(event)
end
User->>BrowserShell: drag item (modifiers)
BrowserShell->>resolveBrowserTransferOperation: read modifiers
BrowserShell->>createBrowserTransferIntent: build intent from source/drop
BrowserShell->>onFileOperation: emit transfer intent (via onTransferIntent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
codeant-ai
Bot
added
the
size:XXL
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+209
to
+227
| const [lastSelection, setLastSelection] = useState<BrowserFolderSelectionEvent | null>(null); | ||
|
|
||
| return ( | ||
| <div data-testid="browser-destination-picker" style={styles.picker}> | ||
| <div style={styles.pickerHeader}>{title}</div> | ||
| <BrowserFolderTree | ||
| {...treeProps} | ||
| onSelectedFolderChange={(event) => { | ||
| setLastSelection(event); | ||
| treeProps.onSelectedFolderChange?.(event); | ||
| }} | ||
| /> | ||
| <button | ||
| disabled={!lastSelection} | ||
| onClick={() => { | ||
| if (lastSelection) onConfirmDestination?.(lastSelection); | ||
| }} | ||
| style={styles.confirmButton} | ||
| type="button" |
There was a problem hiding this comment.
Suggestion: The destination picker only enables confirmation after onSelectedFolderChange fires, so preselected folders (via selectedFolderId/defaultSelectedFolderId) cannot be confirmed until the user re-clicks a node. Initialize and keep confirmation state in sync with the tree's selected folder props (or derive confirmation from current selection) to avoid blocking valid preselected destinations. [logic error]
Severity Level: Major ⚠️
- ⚠️ Preselected tree destinations cannot be confirmed without extra click.
- ⚠️ Affects consumers using controlled or default folder selection props.Steps of Reproduction ✅
1. A host application uses the public export `BrowserDestinationPicker` from
`packages/chonky/src/index.ts:11` and passes `defaultSelectedFolderId` or
`selectedFolderId` as allowed by `BrowserFolderTreeProps` in
`packages/chonky/src/types/browser-tree.types.ts:36-44`.
2. Inside `BrowserDestinationPicker` (`BrowserFolderTree.tsx:202-209`), `lastSelection`
state is initialized to `null` at line 209 and is only updated inside the
`onSelectedFolderChange` wrapper passed to `BrowserFolderTree` at `214-219`.
3. On mount, `BrowserFolderTree` uses `defaultSelectedFolderId`/`selectedFolderId` only to
initialize/control `internalSelected` at `62-69` and does not emit an initial
`onSelectedFolderChange` event, so `lastSelection` remains `null` for a preselected folder
until the user manually clicks a node.
4. The confirm button in `BrowserDestinationPicker` is disabled while `!lastSelection` at
`222` and only invokes `onConfirmDestination` when `lastSelection` is non-null at
`221-225`, so users cannot immediately confirm a valid preselected destination; they must
re-click the folder, demonstrating the logic/UX bug described.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/chonky/src/components/tree/BrowserFolderTree.tsx
**Line:** 209:227
**Comment:**
*Logic Error: The destination picker only enables confirmation after `onSelectedFolderChange` fires, so preselected folders (via `selectedFolderId`/`defaultSelectedFolderId`) cannot be confirmed until the user re-clicks a node. Initialize and keep confirmation state in sync with the tree's selected folder props (or derive confirmation from current selection) to avoid blocking valid preselected destinations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+364
to
+369
| const handleDragStart = useCallback((event: DragStartEvent) => { | ||
| const data = event.active.data.current; | ||
| if (!isBrowserDndSourceData(data)) return; | ||
| modifierStateRef.current = {}; | ||
| setActiveTransferSource(data.source); | ||
| }, []); |
There was a problem hiding this comment.
Suggestion: Modifier state is reset to an empty object at drag start and only updated by future keyup/keydown events, so starting a drag while Ctrl/Cmd/Shift is already held produces the wrong transfer operation (for example, copy intent becomes move). Initialize modifier state from the drag start event's activator keyboard/mouse event so the first drop uses correct modifiers. [logic error]
Severity Level: Critical 🚨
- ❌ Ctrl/Cmd-drag copy gestures are interpreted as move.
- ❌ Hosts may delete sources when user expected copy.
- ⚠️ Transfer intents misrepresent user modifier state.Steps of Reproduction ✅
1. `BrowserShell` enables transfer intents when a host supplies `onTransferIntent` (line
239 computes `dndEnabled = Boolean(onTransferIntent)`), and item draggability is then
enabled in `BrowserShellItem` (`draggableDisabled = !dndEnabled || !canDrag(item)` at
lines 822–823).
2. When a drag starts, `handleDragStart` (lines 364–369) reads the active data, verifies
it is `BrowserDndSourceData`, then unconditionally resets `modifierStateRef.current = {}`
and stores the active transfer source; it does not inspect any modifier keys from the drag
start/activator event.
3. The only place modifier state is subsequently updated is in the `useEffect` at lines
469–480, which listens to global `window` `keydown`/`keyup` and replaces
`modifierStateRef.current` using `modifierStateFromKeyboardEvent`. If a user begins a drag
with Ctrl/Cmd/Shift already held (no new keydown after drag start), this effect never sees
a `keydown` for that key, so `modifierStateRef.current` remains the empty object `{}`
throughout the drag.
4. On drop, `handleDragEnd` (lines 376–389) calls `createBrowserTransferIntent` with
`modifiers: modifierStateRef.current`. `createBrowserTransferIntent` in
`packages/chonky/src/util/browser-transfer.ts` (lines 9–16, 18–43) computes the operation
via `resolveBrowserTransferOperation`, which maps `{}` to the default `'move'` operation;
thus a Ctrl/Cmd-drag that a host expects to mean "copy" is reported as a `'move'` intent,
and downstream file operations will perform a move rather than a copy.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/chonky/src/components/browser-shell/BrowserShell.tsx
**Line:** 364:369
**Comment:**
*Logic Error: Modifier state is reset to an empty object at drag start and only updated by future keyup/keydown events, so starting a drag while Ctrl/Cmd/Shift is already held produces the wrong transfer operation (for example, copy intent becomes move). Initialize modifier state from the drag start event's activator keyboard/mouse event so the first drop uses correct modifiers.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+758
to
+761
| const { isOver, setNodeRef } = useDroppable({ | ||
| id: currentFolder ? toDndId('browser-listing', currentFolder.id) : 'browser-listing:empty', | ||
| disabled: !enabled || !currentFolder || currentFolder.flags?.disabled === true, | ||
| data: dropData, |
There was a problem hiding this comment.
Suggestion: The listing drop target is forcibly disabled when currentFolder is missing, but folderChain is optional and defaults to empty, so hosts enabling transfer intents without a folder chain can never drop into the listing area. Remove the !currentFolder disable condition and allow listing drops with an undefined folder target. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Listing background never becomes a drop target without folderChain.
- ⚠️ Valid flat/root views lose drop-to-listing support.
- ⚠️ Hosts must add synthetic folderChain to enable drops.Steps of Reproduction ✅
1. `BrowserShellProps` in `packages/chonky/src/types/browser-shell.types.ts` define
`folderChain?: readonly BrowserFolderChainItem[];` (line 63), so hosts are allowed to omit
`folderChain` when using `BrowserShell`.
2. In `BrowserShell` (lines 203–244 of `BrowserShell.tsx`), `folderChain` defaults to
`[]`, and `currentFolder` is computed as `folderChain[folderChain.length - 1]` (line 243),
which yields `undefined` when `folderChain` is empty or omitted.
3. The listing drop target is rendered via `<BrowserListingDropTarget
currentFolder={currentFolder} enabled={dndEnabled} … />` at lines 549–555, and
`BrowserListingDropTarget`'s `useDroppable` call (lines 758–761) sets `disabled: !enabled
|| !currentFolder || currentFolder.flags?.disabled === true`. When a host enables transfer
intents with `onTransferIntent` but supplies no `folderChain`, `enabled` is true but
`currentFolder` is `undefined`, so the droppable is forcibly disabled.
4. As a result, the listing container's `data-dnd-droppable` attribute (set at lines
764–778) remains `"false"` even though `onTransferIntent` is provided and
`BrowserTransferTarget.kind: 'listing'` in `dropData` (lines 745–753) supports an
undefined `folderId`; hosts using `BrowserShell` in a flat or root view with no folder
chain cannot drop into the listing background, only onto individual folder items.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/chonky/src/components/browser-shell/BrowserShell.tsx
**Line:** 758:761
**Comment:**
*Incorrect Condition Logic: The listing drop target is forcibly disabled when `currentFolder` is missing, but `folderChain` is optional and defaults to empty, so hosts enabling transfer intents without a folder chain can never drop into the listing area. Remove the `!currentFolder` disable condition and allow listing drops with an undefined folder target.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| <div | ||
| ref={setNodeRef} | ||
| {...(draggableDisabled ? {} : attributes)} | ||
| {...(draggableDisabled ? {} : listeners)} |
There was a problem hiding this comment.
Suggestion: The drag listeners from useDraggable() are spread onto the item, but the component then defines its own onKeyDown, which overrides the listener-provided keyboard handler. This breaks KeyboardSensor activation, so keyboard-driven drag-and-drop will not work. Merge the two handlers (call the DnD listener handler inside your custom key handler) instead of overriding it. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Keyboard-based drag-and-drop on items never starts.
- ⚠️ `onTransferIntent` never fires for keyboard-only drags.
- ⚠️ Accessibility for keyboard users is significantly reduced.Steps of Reproduction ✅
1. In `packages/chonky/src/components/browser-shell/BrowserShell.tsx`, `BrowserShell`
configures `DndContext` with a `KeyboardSensor` in the sensors array (lines 231–234), and
`BrowserShellItem` wires `useDraggable()` onto each item (lines 849–859) to support
drag-and-drop.
2. `useDraggable()` returns `listeners` which include the library's `onKeyDown` handler;
these are spread onto the item element at lines 878–880: `<div ref={setNodeRef}
{...attributes} {...listeners} …>`.
3. Immediately after spreading `listeners`, `BrowserShellItem` defines its own `onKeyDown`
prop at lines 893–902 that handles Enter/Space to open/toggle selection: this custom
`onKeyDown` prop overwrites the `onKeyDown` field from `listeners` because in React the
last prop wins when spreading objects.
4. When a host renders `<BrowserShell items={browserItemFixtures}
folderChain={browserFolderChainFixtures} onTransferIntent={fn} />` (pattern used in
`packages/chonky/test/browser-shell.test.tsx` around lines 136–158 to enable DnD
affordances), pointer-based dragging works (pointer listeners are still attached), but
keyboard-based drag attempts on a focused item (handled by `KeyboardSensor` via
`listeners.onKeyDown`) never reach `DndContext` because the listener has been overridden;
as a result, keyboard-driven drag-and-drop cannot start and no transfer intents are
produced for keyboard interactions.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/chonky/src/components/browser-shell/BrowserShell.tsx
**Line:** 880:902
**Comment:**
*Api Mismatch: The drag listeners from `useDraggable()` are spread onto the item, but the component then defines its own `onKeyDown`, which overrides the listener-provided keyboard handler. This breaks `KeyboardSensor` activation, so keyboard-driven drag-and-drop will not work. Merge the two handlers (call the DnD listener handler inside your custom key handler) instead of overriding it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
packages/chonky/src/util/browser-transfer.ts (1)
9-16: 💤 Low valueConsider documenting modifier precedence.
When both Ctrl/Meta and Shift are pressed simultaneously, Ctrl/Meta takes precedence and the operation resolves to
copy. This behavior is correct but could benefit from a brief inline comment explaining the precedence order.🤖 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 `@packages/chonky/src/util/browser-transfer.ts` around lines 9 - 16, The function resolveBrowserTransferOperation currently treats ctrlKey/metaKey as higher precedence than shiftKey (so when both are pressed it returns 'copy'); add a brief inline comment above the if checks in resolveBrowserTransferOperation explaining the modifier precedence (Ctrl/Meta overrides Shift) and why (e.g. copy vs move), referencing the modifiers.ctrlKey, modifiers.metaKey, and modifiers.shiftKey checks and the defaultOperation fallback so future readers understand the decision.packages/chonky/src/components/browser-shell/BrowserShell.tsx (1)
469-480: 💤 Low valueConsider using
globalThisfor environment detection.The SSR check
typeof window === 'undefined'works correctly, but modern practice preferstypeof globalThis.window === 'undefined'or accessingglobalThisdirectly. This aligns with SonarCloud's suggestion and improves consistency across environments.♻️ Optional modernization
- if (!activeTransferSource || typeof window === 'undefined') return undefined; + if (!activeTransferSource || typeof globalThis.window === 'undefined') return undefined; const updateModifierState = (event: globalThis.KeyboardEvent) => { modifierStateRef.current = modifierStateFromKeyboardEvent(event); }; - window.addEventListener('keydown', updateModifierState); - window.addEventListener('keyup', updateModifierState); + globalThis.window.addEventListener('keydown', updateModifierState); + globalThis.window.addEventListener('keyup', updateModifierState); return () => { - window.removeEventListener('keydown', updateModifierState); - window.removeEventListener('keyup', updateModifierState); + globalThis.window.removeEventListener('keydown', updateModifierState); + globalThis.window.removeEventListener('keyup', updateModifierState); };🤖 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 `@packages/chonky/src/components/browser-shell/BrowserShell.tsx` around lines 469 - 480, The SSR environment check in the useEffect currently uses typeof window === 'undefined'; update it to use globalThis for consistency (e.g., typeof globalThis.window === 'undefined' or checking if globalThis.window is falsy) so the effect early-returns in non-browser runtimes; modify the effect that depends on activeTransferSource and uses modifierStateRef, modifierStateFromKeyboardEvent and the local updateModifierState handler to guard with the globalThis.window check before adding/removing window event listeners.Source: Linters/SAST tools
packages/chonky/test/browser-transfer.test.ts (1)
17-50: 💤 Low valueConsider using
.at(-1)for cleaner array access.The test correctly validates transfer intent creation. The traditional array indexing can be simplified with the modern
.at()method:♻️ Modern syntax alternative
- const destination = browserFolderChainFixtures[browserFolderChainFixtures.length - 1]; + const destination = browserFolderChainFixtures.at(-1)!;🤖 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 `@packages/chonky/test/browser-transfer.test.ts` around lines 17 - 50, Replace the manual last-element access for browserFolderChainFixtures with the modern .at(-1) accessor: change the assignment to destination from browserFolderChainFixtures[browserFolderChainFixtures.length - 1] to browserFolderChainFixtures.at(-1) (keep any non-null assertion if needed) in the test that creates the intent (the it block creating intent and variable destination); ensure your TS/JS target supports Array.prototype.at or add a polyfill if required.Source: Linters/SAST tools
packages/chonky/test/browser-shell.test.tsx (1)
403-417: ⚡ Quick winPrefer
.datasetovergetAttribute()for data attributes.The test correctly validates DnD affordances, but accessing data attributes via
.datasetis cleaner and more idiomatic:♻️ Proposed refactor using .dataset
- expect(screen.getByText('hero-photo.jpg').closest('[data-testid="browser-item"]')?.getAttribute('data-dnd-draggable')).toBe('false'); - expect(screen.getByLabelText('Files').getAttribute('data-dnd-droppable')).toBe('false'); + expect(screen.getByText('hero-photo.jpg').closest('[data-testid="browser-item"]')?.dataset.dndDraggable).toBe('false'); + expect(screen.getByLabelText('Files').dataset.dndDroppable).toBe('false'); rerender( <BrowserShell folderChain={browserFolderChainFixtures} items={browserItemFixtures} onTransferIntent={vi.fn()} /> ); - expect(screen.getByText('hero-photo.jpg').closest('[data-testid="browser-item"]')?.getAttribute('data-dnd-draggable')).toBe('true'); - expect(screen.getByText('Brand assets').closest('[data-testid="browser-item"]')?.getAttribute('data-dnd-droppable')).toBe('true'); - expect(screen.getByLabelText('Files').getAttribute('data-dnd-droppable')).toBe('true'); + expect(screen.getByText('hero-photo.jpg').closest('[data-testid="browser-item"]')?.dataset.dndDraggable).toBe('true'); + expect(screen.getByText('Brand assets').closest('[data-testid="browser-item"]')?.dataset.dndDraggable).toBe('true'); + expect(screen.getByLabelText('Files').dataset.dndDroppable).toBe('true');🤖 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 `@packages/chonky/test/browser-shell.test.tsx` around lines 403 - 417, Update the test assertions in the BrowserShell test to read HTML data attributes via the element.dataset API instead of getAttribute: locate the item using screen.getByText('hero-photo.jpg').closest('[data-testid="browser-item"]') and use .dataset.dndDraggable, and for droppable checks use .dataset.dndDroppable (same for screen.getByText('Brand assets') and screen.getByLabelText('Files')); keep the expected string values ('true'/'false') and the existing rerender/fixture flow intact.Source: Linters/SAST tools
packages/chonky/test/browser-folder-tree.test.tsx (1)
64-73: 💤 Low valueConsider asserting with toBeTruthy() to avoid non-null assertion.
The test correctly validates disabled folder behavior. However, the pattern of using
.toBeDefined()followed by the non-null assertion operator!can be improved for better type safety:♻️ Suggested pattern improvement
- expect(disabledFolderButton).toBeDefined(); - fireEvent.click(disabledFolderButton!); + expect(disabledFolderButton).toBeTruthy(); + if (!disabledFolderButton) throw new Error('Expected disabled button'); + fireEvent.click(disabledFolderButton);Alternatively, combine the assertion with a truthy check:
const disabledFolderButton = within(treeNode) .getAllByRole('button') .find((button) => button.hasAttribute('disabled')); - expect(disabledFolderButton).toBeDefined(); - fireEvent.click(disabledFolderButton!); + expect(disabledFolderButton).toBeTruthy(); + fireEvent.click(disabledFolderButton as HTMLButtonElement);🤖 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 `@packages/chonky/test/browser-folder-tree.test.tsx` around lines 64 - 73, Replace the two-step existence check and non-null assertion with a single truthy assertion: assert that the found button is truthy (expect(disabledFolderButton).toBeTruthy()) and then call fireEvent.click(disabledFolderButton) without the `!`; update the assertions around the variable `disabledFolderButton` (from the test in browser-folder-tree.test.tsx that defines `treeNode`, `disabledFolderButton`) and keep the final expectation on `onSelectedFolderChange` unchanged.packages/chonky/src/components/tree/BrowserFolderTree.tsx (1)
171-171: 💤 Low valueOptional: Extract nested ternary for clarity.
The nested ternary is functional but could be extracted into a helper for slightly better readability:
const getExpanderIcon = (expandable: boolean, expanded: boolean) => { if (!expandable) return '•'; return expanded ? '▾' : '▸'; };However, the current form is concise and understandable, so this is a minor style preference.
🤖 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 `@packages/chonky/src/components/tree/BrowserFolderTree.tsx` at line 171, Extract the nested ternary used for the expander icon in BrowserFolderTree (currently "{expandable ? (expanded ? '▾' : '▸') : '•'}") into a small helper like getExpanderIcon(expandable, expanded) to improve readability; implement the helper (e.g., return '•' when not expandable, otherwise return expanded ? '▾' : '▸') and replace the inline ternary with a call to getExpanderIcon where the icon is rendered.
🤖 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.
Nitpick comments:
In `@packages/chonky/src/components/browser-shell/BrowserShell.tsx`:
- Around line 469-480: The SSR environment check in the useEffect currently uses
typeof window === 'undefined'; update it to use globalThis for consistency
(e.g., typeof globalThis.window === 'undefined' or checking if globalThis.window
is falsy) so the effect early-returns in non-browser runtimes; modify the effect
that depends on activeTransferSource and uses modifierStateRef,
modifierStateFromKeyboardEvent and the local updateModifierState handler to
guard with the globalThis.window check before adding/removing window event
listeners.
In `@packages/chonky/src/components/tree/BrowserFolderTree.tsx`:
- Line 171: Extract the nested ternary used for the expander icon in
BrowserFolderTree (currently "{expandable ? (expanded ? '▾' : '▸') : '•'}") into
a small helper like getExpanderIcon(expandable, expanded) to improve
readability; implement the helper (e.g., return '•' when not expandable,
otherwise return expanded ? '▾' : '▸') and replace the inline ternary with a
call to getExpanderIcon where the icon is rendered.
In `@packages/chonky/src/util/browser-transfer.ts`:
- Around line 9-16: The function resolveBrowserTransferOperation currently
treats ctrlKey/metaKey as higher precedence than shiftKey (so when both are
pressed it returns 'copy'); add a brief inline comment above the if checks in
resolveBrowserTransferOperation explaining the modifier precedence (Ctrl/Meta
overrides Shift) and why (e.g. copy vs move), referencing the modifiers.ctrlKey,
modifiers.metaKey, and modifiers.shiftKey checks and the defaultOperation
fallback so future readers understand the decision.
In `@packages/chonky/test/browser-folder-tree.test.tsx`:
- Around line 64-73: Replace the two-step existence check and non-null assertion
with a single truthy assertion: assert that the found button is truthy
(expect(disabledFolderButton).toBeTruthy()) and then call
fireEvent.click(disabledFolderButton) without the `!`; update the assertions
around the variable `disabledFolderButton` (from the test in
browser-folder-tree.test.tsx that defines `treeNode`, `disabledFolderButton`)
and keep the final expectation on `onSelectedFolderChange` unchanged.
In `@packages/chonky/test/browser-shell.test.tsx`:
- Around line 403-417: Update the test assertions in the BrowserShell test to
read HTML data attributes via the element.dataset API instead of getAttribute:
locate the item using
screen.getByText('hero-photo.jpg').closest('[data-testid="browser-item"]') and
use .dataset.dndDraggable, and for droppable checks use .dataset.dndDroppable
(same for screen.getByText('Brand assets') and screen.getByLabelText('Files'));
keep the expected string values ('true'/'false') and the existing
rerender/fixture flow intact.
In `@packages/chonky/test/browser-transfer.test.ts`:
- Around line 17-50: Replace the manual last-element access for
browserFolderChainFixtures with the modern .at(-1) accessor: change the
assignment to destination from
browserFolderChainFixtures[browserFolderChainFixtures.length - 1] to
browserFolderChainFixtures.at(-1) (keep any non-null assertion if needed) in the
test that creates the intent (the it block creating intent and variable
destination); ensure your TS/JS target supports Array.prototype.at or add a
polyfill if required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 962abdb8-c5f4-40b4-a4fa-60878a46573d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonpackages/chonky/package.jsonpackages/chonky/src/browser.tspackages/chonky/src/components/browser-shell/BrowserShell.tsxpackages/chonky/src/components/tree/BrowserFolderTree.tsxpackages/chonky/src/fixtures/browser-items.tspackages/chonky/src/fixtures/browser-tree.tspackages/chonky/src/index.tspackages/chonky/src/types/browser-item.types.tspackages/chonky/src/types/browser-operation.types.tspackages/chonky/src/types/browser-shell.types.tspackages/chonky/src/types/browser-transfer.types.tspackages/chonky/src/types/browser-tree.types.tspackages/chonky/src/util/browser-file-operations.tspackages/chonky/src/util/browser-transfer.tspackages/chonky/test/browser-folder-tree.test.tsxpackages/chonky/test/browser-shell.test.tsxpackages/chonky/test/browser-transfer.test.ts
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
messagesgoel-blip
changed the title
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/chonky/src/components/browser-shell/BrowserShell.tsx`:
- Around line 470-479: Seed modifierStateRef before registering the keyboard
listeners so a drag started while modifiers are already held resolves correctly:
in the block guarded by activeTransferSource and globalThis.window, set
modifierStateRef.current by sampling the current modifier state (e.g. call
modifierStateFromKeyboardEvent with a KeyboardEvent constructed from current
window/document modifier states or by using Element.getModifierState) before
calling browserWindow.addEventListener; keep the existing updateModifierState
handler and removeEventListener cleanup unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 907b9318-8874-4336-8b0a-85d5a15ce122
📒 Files selected for processing (6)
packages/chonky/src/components/browser-shell/BrowserShell.tsxpackages/chonky/src/components/tree/BrowserFolderTree.tsxpackages/chonky/src/util/browser-transfer.tspackages/chonky/test/browser-folder-tree.test.tsxpackages/chonky/test/browser-shell.test.tsxpackages/chonky/test/browser-transfer.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/chonky/test/browser-transfer.test.ts
- packages/chonky/test/browser-folder-tree.test.tsx
- packages/chonky/src/util/browser-transfer.ts
- packages/chonky/test/browser-shell.test.tsx
- packages/chonky/src/components/tree/BrowserFolderTree.tsx
Comment on lines
+470
to
+479
| if (!activeTransferSource || typeof globalThis.window === 'undefined') return undefined; | ||
| const browserWindow = globalThis.window; | ||
| const updateModifierState = (event: globalThis.KeyboardEvent) => { | ||
| modifierStateRef.current = modifierStateFromKeyboardEvent(event); | ||
| }; | ||
| browserWindow.addEventListener('keydown', updateModifierState); | ||
| browserWindow.addEventListener('keyup', updateModifierState); | ||
| return () => { | ||
| browserWindow.removeEventListener('keydown', updateModifierState); | ||
| browserWindow.removeEventListener('keyup', updateModifierState); |
There was a problem hiding this comment.
Seed the drag modifier state before these listeners start.
Line 470 only begins sampling modifiers after the drag is active, so a user who starts dragging while already holding Ctrl/Cmd/Alt will still emit the default transfer operation unless they press or release a key mid-drag. That makes copy-vs-move resolution incorrect on a common path.
Suggested direction
+const lastKnownModifierStateRef = useRef<BrowserTransferModifierState>({});
+
+useEffect(() => {
+ if (typeof globalThis.window === 'undefined') return undefined;
+ const browserWindow = globalThis.window;
+ const updateModifierState = (event: globalThis.KeyboardEvent) => {
+ const next = modifierStateFromKeyboardEvent(event);
+ lastKnownModifierStateRef.current = next;
+ if (activeTransferSource) {
+ modifierStateRef.current = next;
+ }
+ };
+ browserWindow.addEventListener('keydown', updateModifierState);
+ browserWindow.addEventListener('keyup', updateModifierState);
+ return () => {
+ browserWindow.removeEventListener('keydown', updateModifierState);
+ browserWindow.removeEventListener('keyup', updateModifierState);
+ };
+}, [activeTransferSource]);
+
const handleDragStart = useCallback((event: DragStartEvent) => {
const data = event.active.data.current;
if (!isBrowserDndSourceData(data)) return;
- modifierStateRef.current = {};
+ modifierStateRef.current = lastKnownModifierStateRef.current;
setActiveTransferSource(data.source);
}, []);🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 470-470: Compare with undefined directly instead of using typeof.
🤖 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 `@packages/chonky/src/components/browser-shell/BrowserShell.tsx` around lines
470 - 479, Seed modifierStateRef before registering the keyboard listeners so a
drag started while modifiers are already held resolves correctly: in the block
guarded by activeTransferSource and globalThis.window, set
modifierStateRef.current by sampling the current modifier state (e.g. call
modifierStateFromKeyboardEvent with a KeyboardEvent constructed from current
window/document modifier states or by using Element.getModifierState) before
calling browserWindow.addEventListener; keep the existing updateModifierState
handler and removeEventListener cleanup unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow readme for more information |
|
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
User description
Closes #7
Closes #8
Closes #10
Part of #1
Summary
Verification
@coderabbitai review
CodeAnt-AI Description
Add typed file actions, folder tree browsing, and drag-and-drop transfer support
What Changed
Impact
✅ Clearer file action handling✅ Easier folder destination picking✅ Fewer invalid drag-and-drop moves💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Tests
Chores