refactor(documents-panel): decompose DocumentsPanel#1099
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe monolithic ChangesDocumentsPanel Modular Refactor
CustomThemeBuilder Optional Color Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/renderer/components/DocumentsPanel/components/DocumentSelectorModal.tsx`:
- Around line 68-80: The modal container div that starts at line 68 is missing
required accessibility semantics and styling. Add role="dialog" and
aria-modal="true" attributes to the outer div container to properly declare
dialog semantics, and add the select-none class to the className string on the
same container to comply with the repository's styling guidelines for
click-primary modals.
In `@src/renderer/components/DocumentsPanel/components/DocumentSelectorTree.tsx`:
- Around line 68-79: The button element used for the onToggleFolder handler is
missing accessibility attributes required for assistive technology users. Add an
aria-label attribute to provide an accessible name describing the button's
function (for example, indicating it toggles folder expansion), and add an
aria-expanded attribute that reflects the current isExpanded state. This ensures
screen reader users understand the button's purpose and can perceive the current
expansion state of the folder.
In `@src/renderer/components/DocumentsPanel/components/LoopControls.tsx`:
- Around line 33-37: The condition for displaying the loop hint in
LoopControls.tsx only checks for exactly one document using documents.length ===
1, but it should show the hint whenever there are fewer than two documents
(including zero documents). Change the condition from documents.length === 1 to
documents.length < 2 so that users see the guidance message "You can enable
loops with two or more documents" in both cases where loop controls cannot be
enabled due to insufficient documents.
In `@src/renderer/components/DocumentsPanel/hooks/useDocumentDragReorder.ts`:
- Around line 65-66: The `handleDragLeave` function is currently a no-op, which
allows a previously computed `dropTargetIndex` to persist after the user leaves
the droppable area. This can cause `handleDragEnd` to still execute
`performDropOperation()` with a stale drop target. Update the `handleDragLeave`
callback to reset or clear the `dropTargetIndex` state so that leaving the
droppable area properly cancels any pending drop operation and prevents
unintended reordering.
In `@src/renderer/components/DocumentsPanel/hooks/useDocumentSelection.ts`:
- Line 73: The `allSelected` variable on line 73 incorrectly compares set size
against array length, which fails when selectedDocs contains filenames not
present in allDocuments. Instead of checking if selectedDocs.size equals
allDocuments.length, change the logic to verify that every document in
allDocuments is actually included in the selectedDocs set. You need to iterate
through allDocuments and check if each document is present in selectedDocs,
returning true only when all selectable documents are selected regardless of any
extra entries in selectedDocs.
In `@src/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.ts`:
- Around line 16-25: The handleRefresh function does not handle errors from
onRefresh(), leaving the refreshing state stuck on true if the promise rejects.
Wrap the await onRefresh() call in a try-catch block and ensure
setRefreshing(false) is called in all paths by placing the setTimeout that
resets the state in a finally block, so it executes regardless of whether
onRefresh() succeeds or fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c7aed4b-4931-49ce-8cf9-1020caa06db8
📒 Files selected for processing (33)
src/__tests__/renderer/components/DocumentsPanel/DocumentsPanel.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/components/DocumentList.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/components/DocumentSelectorModal.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/components/LoopControls.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentDragReorder.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentListActions.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentSelection.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/utils/documentCounts.test.tssrc/__tests__/renderer/components/DocumentsPanel/utils/documentTree.test.tssrc/renderer/components/DocumentsPanel.tsxsrc/renderer/components/DocumentsPanel/DocumentsPanel.tsxsrc/renderer/components/DocumentsPanel/components/CopyDragIndicator.tsxsrc/renderer/components/DocumentsPanel/components/DocumentList.tsxsrc/renderer/components/DocumentsPanel/components/DocumentRow.tsxsrc/renderer/components/DocumentsPanel/components/DocumentSelectorFlatList.tsxsrc/renderer/components/DocumentsPanel/components/DocumentSelectorFooter.tsxsrc/renderer/components/DocumentsPanel/components/DocumentSelectorModal.tsxsrc/renderer/components/DocumentsPanel/components/DocumentSelectorTree.tsxsrc/renderer/components/DocumentsPanel/components/LoopControls.tsxsrc/renderer/components/DocumentsPanel/components/MissingDocumentsWarning.tsxsrc/renderer/components/DocumentsPanel/components/TaskCountBadge.tsxsrc/renderer/components/DocumentsPanel/components/index.tssrc/renderer/components/DocumentsPanel/hooks/index.tssrc/renderer/components/DocumentsPanel/hooks/useDocumentDragReorder.tssrc/renderer/components/DocumentsPanel/hooks/useDocumentListActions.tssrc/renderer/components/DocumentsPanel/hooks/useDocumentSelection.tssrc/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.tssrc/renderer/components/DocumentsPanel/index.tssrc/renderer/components/DocumentsPanel/types.tssrc/renderer/components/DocumentsPanel/utils/documentCounts.tssrc/renderer/components/DocumentsPanel/utils/documentTree.tssrc/renderer/components/DocumentsPanel/utils/index.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/DocumentsPanel.tsx
Greptile SummaryThis PR decomposes a 1,293-line monolithic
Confidence Score: 4/5The refactor faithfully preserves the original behaviour including the drag deduplication guard, reset-lock rules, and modal layer registration; the timer fix is a genuine improvement. All findings are non-blocking style issues: dead code in the refresh hook, a missing useDocumentSelectorRefresh.ts (dead else branch), DocumentSelectorModal.tsx (missing select-none), DocumentSelectorFlatList.tsx / DocumentSelectorTree.tsx (duplicated CheckMark). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
BP[BatchRunnerModal] -->|props| DP[DocumentsPanel coordinator]
DP --> UDR[useDocumentDragReorder]
DP --> UDLA[useDocumentListActions]
DP --> DL[DocumentList]
DP --> LC[LoopControls]
DP --> MDW[MissingDocumentsWarning]
DP --> DSM[DocumentSelectorModal]
DP --> CDI[CopyDragIndicator]
DL --> DR[DocumentRow xN]
DSM --> UDS[useDocumentSelection]
DSM --> UDSR[useDocumentSelectorRefresh]
DSM --> DSFL[DocumentSelectorFlatList]
DSM --> DSTL[DocumentSelectorTree]
DSM --> DSF[DocumentSelectorFooter]
UDR -->|draggedId, dropTargetIndex, handlers| DL
UDLA -->|handleAddSelectedDocs| DSM
UDLA -->|handleRemove, handleToggleReset, handleDuplicate| DL
subgraph utils
DC[documentCounts.ts]
DT[documentTree.ts]
end
UDS --> DC
UDS --> DT
DR --> DC
DSTL --> DT
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
BP[BatchRunnerModal] -->|props| DP[DocumentsPanel coordinator]
DP --> UDR[useDocumentDragReorder]
DP --> UDLA[useDocumentListActions]
DP --> DL[DocumentList]
DP --> LC[LoopControls]
DP --> MDW[MissingDocumentsWarning]
DP --> DSM[DocumentSelectorModal]
DP --> CDI[CopyDragIndicator]
DL --> DR[DocumentRow xN]
DSM --> UDS[useDocumentSelection]
DSM --> UDSR[useDocumentSelectorRefresh]
DSM --> DSFL[DocumentSelectorFlatList]
DSM --> DSTL[DocumentSelectorTree]
DSM --> DSF[DocumentSelectorFooter]
UDR -->|draggedId, dropTargetIndex, handlers| DL
UDLA -->|handleAddSelectedDocs| DSM
UDLA -->|handleRemove, handleToggleReset, handleDuplicate| DL
subgraph utils
DC[documentCounts.ts]
DT[documentTree.ts]
end
UDS --> DC
UDS --> DT
DR --> DC
DSTL --> DT
Reviews (1): Last reviewed commit: "refactor(documents-panel): decompose Doc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| message = 'No changes'; | ||
| } | ||
| setRefreshMessage(message); | ||
| prevDocCountRef.current = allDocumentsLength; | ||
|
|
||
| const timer = setTimeout(() => setRefreshMessage(null), 3000); | ||
| return () => clearTimeout(timer); |
There was a problem hiding this comment.
else branch (diff === 0)
The outer guard prevDocCountRef.current !== allDocumentsLength already ensures diff is never zero, so else { message = 'No changes'; } can never execute. This dead branch was inherited from the original code (where it was equally unreachable) and can simply be removed — a no-change refresh will never enter this if block at all, so refreshMessage stays null as expected.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| <div | ||
| className="fixed inset-0 bg-black/50 flex items-center justify-center z-[10000]" | ||
| onClick={onClose} | ||
| > |
There was a problem hiding this comment.
select-none on click-driven modal root
Per the CLAUDE.md guidelines, click-driven modals (buttons, list rows, toggles) should have select-none on their root container to prevent accidental drag-selection during normal interactions. The document selector is entirely click-driven — the document list rows, checkboxes, refresh button, and footer actions are all click targets. This was also missing in the original monolithic component; extracting it into a standalone component is a good moment to add it.
| <div | |
| className="fixed inset-0 bg-black/50 flex items-center justify-center z-[10000]" | |
| onClick={onClose} | |
| > | |
| <div | |
| className="fixed inset-0 bg-black/50 flex items-center justify-center z-[10000] select-none" | |
| onClick={onClose} | |
| > |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function CheckMark() { | ||
| return ( | ||
| <svg className="w-3 h-3 text-white" viewBox="0 0 12 12" fill="none"> | ||
| <path | ||
| d="M2 6L5 9L10 3" | ||
| stroke="currentColor" | ||
| strokeWidth="2" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| /> | ||
| </svg> | ||
| ); |
There was a problem hiding this comment.
CheckMark SVG component duplicated
The same CheckMark SVG is defined verbatim in DocumentSelectorTree.tsx (line 30-38). CLAUDE.md's DRY guidance says to extract shared helpers rather than copy them. Both files are siblings in components/, so extracting this into a small shared file (e.g., components/CheckMark.tsx) or into the barrel index would remove the duplication.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks for this, @reachrazamair! This is a genuinely clean decomposition. The coordinator shell is tidy, the public import surface is preserved, and the 41 new tests plus a green full suite give real confidence that behavior was kept intact. The I went through both AI reviews (Greptile and CodeRabbit) and verified each finding against the code. Most of what they flagged is faithfully-preserved original behavior, so I would not touch those. But a few items genuinely line up with this repo's own conventions and with the maintainability goal of this PR, so I would like those addressed before merge: Please address:
Worth considering (your call):
Please skip (these preserve original behavior):
There are no merge conflicts, so once those land I am happy to approve. Thanks again for the careful work and the thorough manual QA list. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.ts (1)
39-40: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove unreachable
'No changes'branch.The outer guard at line 32 (
prevDocCountRef.current !== allDocumentsLength) ensuresdiffis always non-zero when this code executes. Thediff === 0case can never be reached, making this dead code.🧹 Proposed fix to remove dead code
if (diff > 0) { message = `Found ${diff} new document${diff === 1 ? '' : 's'}`; - } else if (diff < 0) { + } else { message = `${Math.abs(diff)} document${Math.abs(diff) === 1 ? '' : 's'} removed`; - } else { - message = 'No changes'; }🤖 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 `@src/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.ts` around lines 39 - 40, The else branch at lines 39-40 that sets message to 'No changes' is unreachable dead code because the outer guard condition at line 32 (prevDocCountRef.current !== allDocumentsLength) ensures the diff is always non-zero when this code path executes. Remove the entire else branch and the 'No changes' assignment, keeping only the necessary logic for handling the non-zero diff case. Ensure the remaining if-else structure (if any) remains logically sound.
🤖 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.
Outside diff comments:
In `@src/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.ts`:
- Around line 39-40: The else branch at lines 39-40 that sets message to 'No
changes' is unreachable dead code because the outer guard condition at line 32
(prevDocCountRef.current !== allDocumentsLength) ensures the diff is always
non-zero when this code path executes. Remove the entire else branch and the 'No
changes' assignment, keeping only the necessary logic for handling the non-zero
diff case. Ensure the remaining if-else structure (if any) remains logically
sound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 855c335d-64b1-4d63-894c-37ef8f4e8f27
📒 Files selected for processing (12)
src/__tests__/renderer/components/DocumentsPanel/components/DocumentSelectorModal.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/components/LoopControls.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentDragReorder.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentSelection.test.tsxsrc/__tests__/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.test.tsxsrc/renderer/components/DocumentsPanel/components/DocumentSelectorModal.tsxsrc/renderer/components/DocumentsPanel/components/DocumentSelectorTree.tsxsrc/renderer/components/DocumentsPanel/components/LoopControls.tsxsrc/renderer/components/DocumentsPanel/hooks/useDocumentDragReorder.tssrc/renderer/components/DocumentsPanel/hooks/useDocumentSelection.tssrc/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.tssrc/renderer/components/DocumentsPanel/types.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/tests/renderer/components/DocumentsPanel/hooks/useDocumentSelectorRefresh.test.tsx
- src/tests/renderer/components/DocumentsPanel/hooks/useDocumentSelection.test.tsx
- src/renderer/components/DocumentsPanel/hooks/useDocumentSelection.ts
- src/renderer/components/DocumentsPanel/types.ts
- src/renderer/components/DocumentsPanel/components/DocumentSelectorModal.tsx
- src/tests/renderer/components/DocumentsPanel/hooks/useDocumentDragReorder.test.tsx
- src/tests/renderer/components/DocumentsPanel/components/DocumentSelectorModal.test.tsx
- src/renderer/components/DocumentsPanel/components/DocumentSelectorTree.tsx
- src/renderer/components/DocumentsPanel/components/LoopControls.tsx
|
@CodeRabbit review |
✅ Action performedReview finished.
|
2 participants
Summary
Decomposes the monolithic
DocumentsPanel.tsxinto a focusedDocumentsPanel/module with a small coordinator shell, render components, hooks, utilities, and a barrel export.The public import surface is preserved:
import { DocumentsPanel } from './DocumentsPanel'import('./DocumentsPanel').DocTreeNodeChanges
DocumentsPanel.tsxwith a 134-line shell.DocTreeNode.pathbehavior.Tests
Added 41 focused renderer tests covering:
DocumentsPanelwiringVerification run:
npm run test -- src/__tests__/renderer/components/DocumentsPanel src/__tests__/renderer/components/BatchRunnerModal.test.tsx src/__tests__/renderer/constants/modalPriorities.test.tsnpm run test -- src/__tests__/renderer/hooks/batch src/__tests__/renderer/hooks/useBatchProcessor.test.ts src/__tests__/cli/services/batch-processor.test.ts src/__tests__/cli/services/playbooks.test.ts src/__tests__/cli/commands/run-playbook.test.ts src/__tests__/main/ipc/handlers/playbooks.test.tsnpm run format:checknpm run lintnpm run lint:eslintnpx tsc --noEmit --prettynpm run testFull suite result:
Manual QA
Summary by CodeRabbit