fix(interface): wire Space Settings and fix space switcher dropdown - #3078
fix(interface): wire Space Settings and fix space switcher dropdown#3078gprot42 wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughAdds a space settings dialog and shared presets, threads sort order through explorer state, queries, and views, and updates Tauri aliases, stubs, and overlay styling. A CLI directory listing call and a core test fixture now pass an explicit directory sort direction. ChangesSpace settings feature
Explorer sort order flow
Tauri compatibility updates
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an explicit interface for component props.
Inline type
{ id: number; space: Space }should be an explicit interface per coding guidelines. As per coding guidelines, "Use explicit TypeScript interfaces for component props instead of implicit types."♻️ Proposed refactor
-function SpaceSettingsDialog(props: { id: number; space: Space }) { +interface SpaceSettingsDialogProps { + id: number; + space: Space; +} + +function SpaceSettingsDialog(props: SpaceSettingsDialogProps) {🤖 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/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx` at line 19, The SpaceSettingsDialog component is using an inline props type instead of an explicit interface. Define a named interface for the component props (for example, alongside SpaceSettingsDialog) and update the component signature to use that interface so the props shape is explicit and consistent with the coding guidelines.Source: Coding guidelines
packages/interface/src/components/SpacesSidebar/SpaceSwitcher.tsx (1)
76-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename these dialog helpers to drop the
useprefix. They’re plain dialog factories, so the hook-like name is misleading when they’re passed through menu callbacks.createSpaceDialog/createSpaceSettingsDialogwould be clearer.🤖 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/interface/src/components/SpacesSidebar/SpaceSwitcher.tsx` around lines 76 - 92, Rename the dialog factory helpers referenced in SpaceSwitcher so they no longer use the hook-like `use` prefix; update the menu callbacks to call `createSpaceDialog` and `createSpaceSettingsDialog` instead of `useCreateSpaceDialog` and `useSpaceSettingsDialog`. Make sure the corresponding helper exports/imports and any other call sites are updated consistently so the names reflect that these are plain dialog factories, not React hooks.
🤖 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/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx`:
- Around line 30-40: The `onSubmit` handler in `SpaceSettingsModal` currently
awaits `updateSpace.mutateAsync` without surfacing failures, so rejected
`spaces.update` requests leave the dialog open with no user feedback. Wrap the
`mutateAsync` call in a try-catch inside `onSubmit`, keep `dialog.state.open =
false` only on success, and use an existing UI feedback path in this component
(toast, inline form error, or similar) to show the caught error message to the
user.
---
Nitpick comments:
In `@packages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx`:
- Line 19: The SpaceSettingsDialog component is using an inline props type
instead of an explicit interface. Define a named interface for the component
props (for example, alongside SpaceSettingsDialog) and update the component
signature to use that interface so the props shape is explicit and consistent
with the coding guidelines.
In `@packages/interface/src/components/SpacesSidebar/SpaceSwitcher.tsx`:
- Around line 76-92: Rename the dialog factory helpers referenced in
SpaceSwitcher so they no longer use the hook-like `use` prefix; update the menu
callbacks to call `createSpaceDialog` and `createSpaceSettingsDialog` instead of
`useCreateSpaceDialog` and `useSpaceSettingsDialog`. Make sure the corresponding
helper exports/imports and any other call sites are updated consistently so the
names reflect that these are plain dialog factories, not React hooks.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b7d25112-f575-49ee-a22c-87edc595fc27
📒 Files selected for processing (4)
packages/interface/src/components/SpacesSidebar/CreateSpaceModal.tsxpackages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsxpackages/interface/src/components/SpacesSidebar/SpaceSwitcher.tsxpackages/interface/src/components/SpacesSidebar/spacePresets.ts
| const onSubmit = form.handleSubmit(async (data) => { | ||
| if (!data.name?.trim()) return; | ||
|
|
||
| await updateSpace.mutateAsync({ | ||
| space_id: props.space.id, | ||
| name: data.name.trim(), | ||
| icon: selectedIcon, | ||
| color: selectedColor, | ||
| }); | ||
| dialog.state.open = false; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add error handling for the spaces.update mutation.
If mutateAsync rejects, handleSubmit catches the error but it's never surfaced to the user — the dialog stays open with no feedback. Wrap the mutation in a try-catch and display the error (toast, inline message, or form error state).
🛡️ Proposed fix
const onSubmit = form.handleSubmit(async (data) => {
if (!data.name?.trim()) return;
- await updateSpace.mutateAsync({
- space_id: props.space.id,
- name: data.name.trim(),
- icon: selectedIcon,
- color: selectedColor,
- });
- dialog.state.open = false;
+ try {
+ await updateSpace.mutateAsync({
+ space_id: props.space.id,
+ name: data.name.trim(),
+ icon: selectedIcon,
+ color: selectedColor,
+ });
+ dialog.state.open = false;
+ } catch (error) {
+ console.error('Failed to update space:', error);
+ // Surface error to user via toast, inline message, or form error state
+ }
});🤖 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/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx`
around lines 30 - 40, The `onSubmit` handler in `SpaceSettingsModal` currently
awaits `updateSpace.mutateAsync` without surfacing failures, so rejected
`spaces.update` requests leave the dialog open with no user feedback. Wrap the
`mutateAsync` call in a try-catch inside `onSubmit`, keep `dialog.state.open =
false` only on success, and use an existing UI feedback path in this component
(toast, inline form error, or similar) to show the caught error message to the
user.
ef0597f to
2801014
Compare
2801014 to
654b15b
Compare
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)
packages/interface/src/routes/explorer/hooks/useExplorerKeyboard.ts (1)
47-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
folders_firstto the directory listing query for keyboard navigation consistency.The query now includes
sort_directionbut omitsfolders_first. When the user has "folders first" enabled, the displayed files (fromuseExplorerFiles) have directories first, but this keyboard navigation query returns files without that ordering. Arrow key navigation and "select all" will operate on a different file order than what's displayed.💚 Proposed fix
sort_by: sortBy as DirectorySortBy, sort_direction: toSortDirection(sortOrder), + folders_first: viewSettings.foldersFirst, } : null!,🤖 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/interface/src/routes/explorer/hooks/useExplorerKeyboard.ts` around lines 47 - 61, The directory listing query in useExplorerKeyboard’s useNormalizedQuery input is missing the folders-first ordering flag, so keyboard navigation can diverge from the file order shown by useExplorerFiles. Update the query payload to pass the same folders-first setting used elsewhere when building the directory listing request, alongside sort_by and sort_direction, so arrow-key navigation and select-all operate on the displayed order.
🧹 Nitpick comments (2)
packages/interface/src/routes/explorer/views/KnowledgeView.tsx (1)
85-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
sortBycasting between KnowledgeView and useExplorerFiles.
useExplorerFiles.tscastssortBy as DirectorySortBybefore passing it to the directory listing query, butKnowledgeView.tsxpassessortBydirectly without a cast. IfSortBy(DirectorySortBy | MediaSortBy) includes values outsideDirectorySortBy, this is a type safety gap. Align both call sites to use the same approach.As per coding guidelines,
packages/interface/**/*.{tsx,ts}should use auto-generated types from Rust and never manually redefine or cast backend types — verify whether the cast is needed or the query input accepts the broaderSortBytype.♻️ Suggested alignment
sort_by: sortBy, +``` If the cast is needed (as in `useExplorerFiles.ts`): ```diff - sort_by: sortBy, + sort_by: sortBy as DirectorySortBy,🤖 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/interface/src/routes/explorer/views/KnowledgeView.tsx` around lines 85 - 98, Align KnowledgeView with useExplorerFiles by checking the type expected by the files.directory_listing query input and avoiding an unsafe mismatch between SortBy and DirectorySortBy. In KnowledgeView.tsx, use the same typing approach as useExplorerFiles.ts: either accept the broader query type if the Rust-generated schema supports it, or narrow sortBy through the existing DirectorySortBy-safe path before passing it to useNormalizedQuery. Keep the fix local to KnowledgeView and preserve the shared sortBy/toSortDirection flow.Source: Coding guidelines
packages/interface/src/routes/explorer/ExplorerView.tsx (1)
152-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
SortMenuto acceptViewModedirectly
ExplorerViewstill passesviewMode as anyinto bothSortMenuPanelandSortMenu.SortMenuonly branches on"media"vs everything else, so it can take the sharedViewModetype (or a shared alias) instead of suppressing type checking.🤖 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/interface/src/routes/explorer/ExplorerView.tsx` around lines 152 - 156, ExplorerView is still forcing a type cast when passing viewMode into SortMenu, which hides the real type mismatch. Update SortMenu to accept the shared ViewMode type (or a common alias) directly, and adjust its props and any internal branching so it handles the existing media vs non-media cases without requiring viewMode as any. Then remove the cast in ExplorerView and keep the SortMenuPanel/SortMenu call sites typed consistently.Source: Coding guidelines
🤖 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 `@packages/interface/src/routes/explorer/hooks/useExplorerKeyboard.ts`:
- Around line 47-61: The directory listing query in useExplorerKeyboard’s
useNormalizedQuery input is missing the folders-first ordering flag, so keyboard
navigation can diverge from the file order shown by useExplorerFiles. Update the
query payload to pass the same folders-first setting used elsewhere when
building the directory listing request, alongside sort_by and sort_direction, so
arrow-key navigation and select-all operate on the displayed order.
---
Nitpick comments:
In `@packages/interface/src/routes/explorer/ExplorerView.tsx`:
- Around line 152-156: ExplorerView is still forcing a type cast when passing
viewMode into SortMenu, which hides the real type mismatch. Update SortMenu to
accept the shared ViewMode type (or a common alias) directly, and adjust its
props and any internal branching so it handles the existing media vs non-media
cases without requiring viewMode as any. Then remove the cast in ExplorerView
and keep the SortMenuPanel/SortMenu call sites typed consistently.
In `@packages/interface/src/routes/explorer/views/KnowledgeView.tsx`:
- Around line 85-98: Align KnowledgeView with useExplorerFiles by checking the
type expected by the files.directory_listing query input and avoiding an unsafe
mismatch between SortBy and DirectorySortBy. In KnowledgeView.tsx, use the same
typing approach as useExplorerFiles.ts: either accept the broader query type if
the Rust-generated schema supports it, or narrow sortBy through the existing
DirectorySortBy-safe path before passing it to useNormalizedQuery. Keep the fix
local to KnowledgeView and preserve the shared sortBy/toSortDirection flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 712a62ed-a8d4-4a25-9249-1502f6688699
⛔ Files ignored due to path filters (1)
packages/ts-client/src/generated/types.tsis excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (22)
apps/cli/src/domains/file/mod.rsapps/tauri/src/index.cssapps/tauri/src/stubs/debug.tsapps/tauri/src/stubs/spacebot-api-client.tsapps/tauri/vite.config.tscore/src/ops/files/query/directory_listing.rscore/tests/normalized_cache_fixtures_test.rspackages/interface/src/components/SpacesSidebar/CreateSpaceModal.tsxpackages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsxpackages/interface/src/components/SpacesSidebar/SpaceSwitcher.tsxpackages/interface/src/components/SpacesSidebar/spacePresets.tspackages/interface/src/components/TabManager/TabManagerContext.tsxpackages/interface/src/routes/explorer/ExplorerView.tsxpackages/interface/src/routes/explorer/context.tsxpackages/interface/src/routes/explorer/hooks/useExplorerFiles.tspackages/interface/src/routes/explorer/hooks/useExplorerKeyboard.tspackages/interface/src/routes/explorer/sortUtils.tspackages/interface/src/routes/explorer/views/ColumnView/Column.tsxpackages/interface/src/routes/explorer/views/ColumnView/ColumnView.tsxpackages/interface/src/routes/explorer/views/KnowledgeView.tsxpackages/interface/src/routes/explorer/views/ListView/ListView.tsxpackages/interface/src/routes/explorer/views/SizeView/SizeView.tsx
✅ Files skipped from review due to trivial changes (4)
- packages/interface/src/components/SpacesSidebar/spacePresets.ts
- apps/tauri/src/stubs/debug.ts
- packages/interface/src/routes/explorer/views/SizeView/SizeView.tsx
- apps/cli/src/domains/file/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx
- packages/interface/src/components/SpacesSidebar/CreateSpaceModal.tsx
- packages/interface/src/components/SpacesSidebar/SpaceSwitcher.tsx
1 participant
Summary
The All Devices space switcher dropdown was unreadable on Tauri's transparent window. Space Settings had no handler, and opening New Space from the dropdown broke Radix focus.
Changes
!bg-sidebar-boxbackground on the dropdown menuapps/tauri/src/index.cssSpaceSettingsModal(spaces.update)Test plan