-
Notifications
You must be signed in to change notification settings - Fork 107
Fix/file tree selection parents #276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When all childs of a folder are selected in the tree, the selected path was only simplifiyng one level. But if the reduction was also making its own parent "fully selected" it was not correctly reporting only the parent as selected
WalkthroughAdds a FileTree test suite and related test setup, updates FileTree selection consolidation and parent/child handling, changes NodeButton rendering from Changes
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
fbc4a3a to
8a88d2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/client/components/file-tree.tsx (1)
196-237: Consider adding a maximum iteration limit as a safeguard for deeply nested hierarchies.The consolidation algorithm has O(depth × fileList.length × selectedItems) complexity. While the break statement limits iterations to tree depth (typically manageable), adding a max iterations check would prevent any edge cases with unusual hierarchies from causing performance issues.
let changed = true; let iterations = 0; const MAX_ITERATIONS = 100; // Safeguard for pathological cases while (changed && iterations < MAX_ITERATIONS) { iterations++; changed = false; // ... rest of algorithm }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
app/client/components/__test__/file-tree.test.tsxapp/client/components/file-tree.tsxapp/client/components/volume-file-browser.tsxapp/test/setup-client.tspackage.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting with
bunx biome check --write ., format only withbunx biome format --write ., or lint withbunx biome lint .
Files:
app/client/components/__test__/file-tree.test.tsxapp/client/components/file-tree.tsxpackage.jsonapp/test/setup-client.tsapp/client/components/volume-file-browser.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use tabs (not spaces) for indentation with a line width of 120 characters
Use double quotes for strings
Do not auto-organize imports - imports organization is disabled in Biome
All imports must include file extensions when targeting Node/Bun, as the project uses"type": "module"
Files:
app/client/components/__test__/file-tree.test.tsxapp/client/components/file-tree.tsxapp/test/setup-client.tsapp/client/components/volume-file-browser.tsx
app/client/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
app/client/**/*.{ts,tsx}: Client uses TanStack Query for server state management
Client uses Radix UI primitives with custom Tailwind styling
Use Server-Sent Events hook (use-server-events.ts) for real-time updates in the client
Files:
app/client/components/__test__/file-tree.test.tsxapp/client/components/file-tree.tsxapp/client/components/volume-file-browser.tsx
🧠 Learnings (7)
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Use `bun run test` to run all tests, or `bunx dotenv-cli -e .env.test -- bun test --preload ./app/test/setup.ts` for specific test files
Applied to files:
package.json
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Use `bun run gen:api-client` to generate TypeScript API client from OpenAPI spec
Applied to files:
package.json
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Use `bun run build` for production builds
Applied to files:
package.json
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Applies to **/*.{ts,tsx,js,jsx,json} : Use Biome for code formatting and linting with `bunx biome check --write .`, format only with `bunx biome format --write .`, or lint with `bunx biome lint .`
Applied to files:
package.json
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Use `bun run tsc` for type checking and generating React Router types
Applied to files:
package.json
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Applies to app/drizzle/**/*.ts : Never create migration files manually. Always use the provided command to generate migrations
Applied to files:
package.json
📚 Learning: 2025-12-28T17:31:39.171Z
Learnt from: CR
Repo: nicotsx/zerobyte PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T17:31:39.171Z
Learning: Applies to app/client/**/*.{ts,tsx} : Client uses Radix UI primitives with custom Tailwind styling
Applied to files:
app/client/components/volume-file-browser.tsx
🧬 Code graph analysis (1)
app/client/components/__test__/file-tree.test.tsx (1)
app/client/components/file-tree.tsx (2)
FileEntry(19-25)FileTree(45-338)
🪛 GitHub Actions: Checks
app/client/components/__test__/file-tree.test.tsx
[warning] 36-36: Forbidden non-null assertion. (noNonNullAssertion)
[warning] 59-59: Forbidden non-null assertion. (noNonNullAssertion)
[warning] 90-90: Forbidden non-null assertion. (noNonNullAssertion)
[warning] 130-130: Forbidden non-null assertion. (noNonNullAssertion)
[warning] 161-161: Forbidden non-null assertion. (noNonNullAssertion)
🔇 Additional comments (8)
app/client/components/volume-file-browser.tsx (1)
60-60: LGTM! Consistent styling update.The change from
min-h-[200px]tomin-h-50is semantically equivalent in Tailwind (50 × 4px = 200px) and consistently applied across all three placeholder states.Also applies to: 68-68, 76-76
app/test/setup-client.ts (1)
1-4: LGTM! Clean test setup for client tests.The setup correctly imports shared test configuration and registers Happy DOM globals for client-side testing. The module-level
GlobalRegistrator.register()call ensures the DOM environment is available before tests execute.package.json (1)
20-22: LGTM! Well-structured test script organization.The split between
test:serverandtest:clientwith appropriate preload files is clean and maintainable. Sequential execution ensures server tests pass before running client tests.app/client/components/__test__/file-tree.test.tsx (1)
1-167: Excellent test coverage for selection logic!The test suite comprehensively validates the FileTree selection consolidation behaviors including parent simplification, child deselection, recursive consolidation, and deep path handling. The test scenarios align well with the updated selection logic in
file-tree.tsx.app/client/components/file-tree.tsx (4)
119-127: LGTM! Performance optimization for collapsed state sync.The addition of
hasChangestracking prevents unnecessary state updates when the collapsed folder set hasn't changed. This is a clean optimization that avoids triggering re-renders.
154-157: LGTM! Correct iterator for descendant removal.The change from iterating over
fileListto iterating overnewSelectionis correct—you only need to check paths that are already in the selection set to find descendants to remove.
187-188: LGTM! Enhanced sibling restoration logic.The additional conditions correctly prevent re-adding the unchecked path itself and any ancestors of the unchecked path when expanding a selected parent to its siblings.
493-503: Add keyboard accessibility to interactive tree node elements.The
NodeButtoncomponent lacks keyboard support for folder expansion and file/folder selection. The biome-ignore comments acknowledge accessibility violations, but they're not actually resolved. While the nested Checkbox (a Radix UI primitive) is keyboard accessible and doesn't cause nesting issues, the parentNodeButtondiv itself isn't keyboard navigable—there are no keyboard event handlers, noroleattribute, and notabIndex.Consider either:
- Adding keyboard handlers (
onKeyDownfor Enter/Space to toggle folders, arrow keys for navigation) and appropriate ARIA attributes- Using an actual
<button>element instead of adiv- Using a Radix primitive like
@radix-ui/react-primitiveor rendering with the Button component'sasChildpattern (as demonstrated inui/button.tsx)Disabling linting rules doesn't replace the missing keyboard interaction logic.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.