Conversation
|
Warning Review limit reached
Next review available in: 7 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 (1)
WalkthroughAdds explicit explorer sort-order state and query propagation across backend and UI, introduces an Add Storage top-bar action, and updates Tauri resolution, stub modules, and transparent-window CSS handling. ChangesExplorer sort direction flow
Tauri build and runtime compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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/routes/explorer/ExplorerView.tsx`:
- Around line 97-102: The storage-added callback is still typed too loosely,
letting invalid values through to navigation. Update the callback type in
useAddStorageDialog and AddStorageModal so the parameter is SdPath instead of
any, and keep the ExplorerView handleAddStorage flow aligned with that contract
when calling navigateToPath.
🪄 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: b91ce94d-2b05-4b37-a630-4490927df0f0
📒 Files selected for processing (1)
packages/interface/src/routes/explorer/ExplorerView.tsx
| const handleAddStorage = useCallback(() => { | ||
| useAddStorageDialog((sdPath) => { | ||
| navigateToPath(sdPath); | ||
| }); | ||
| }, [navigateToPath]); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the dialog hook and SdPath definition/usages
git ls-files 'packages/interface/src/routes/explorer/**' 'packages/**' | rg 'AddStorageModal\.tsx|ExplorerView\.tsx|SdPath'
rg -n "useAddStorageDialog|SdPath|any" packages/interface/src/routes/explorer packages -g '*.ts' -g '*.tsx'Repository: spacedriveapp/spacedrive
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '350,390p' packages/interface/src/routes/explorer/components/AddStorageModal.tsxRepository: spacedriveapp/spacedrive
Length of output: 1056
Type onStorageAdded as SdPath in packages/interface/src/routes/explorer/components/AddStorageModal.tsx
useAddStorageDialog still accepts (sdPath: any), which bypasses the SdPath contract and lets invalid values reach navigateToPath. Replace any with SdPath.
🤖 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 97 -
102, The storage-added callback is still typed too loosely, letting invalid
values through to navigation. Update the callback type in useAddStorageDialog
and AddStorageModal so the parameter is SdPath instead of any, and keep the
ExplorerView handleAddStorage flow aligned with that contract when calling
navigateToPath.
Source: Coding guidelines
Cloud storage was only reachable from Overview. Expose the same Add Storage entry point in the explorer so users can connect S3, Google Drive, and other providers while browsing files.
2801014 to
f32267a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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)
49-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
folders_firstto the keyboard navigation directory query.Every other directory listing query in this PR includes
folders_first: viewSettings.foldersFirst, but this query omits it. Whenfolders_firstis enabled in view settings, the server returns files in a different order than what the user sees, causing arrow-key navigation, typeahead, and select-all to operate on a misaligned file list.🐛 Proposed fix
input: currentPath ? { path: currentPath, limit: null, include_hidden: false, 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 49 - 56, The directory query in useExplorerKeyboard is missing folders_first, so keyboard navigation can get out of sync with the displayed order. Update the query built from currentPath to include folders_first using the existing viewSettings.foldersFirst value, matching the other directory listing queries in this hook so arrow-key navigation, typeahead, and select-all operate on the same ordering.
🧹 Nitpick comments (2)
packages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an explicit interface for
SpaceSettingsDialogprops.As per coding guidelines, "Use explicit TypeScript interfaces for component props instead of implicit types." The inline type
{ id: number; space: Space }should be a named interface.♻️ Proposed refactor
+interface SpaceSettingsDialogProps { + id: number; + space: Space; +} + -function SpaceSettingsDialog(props: { 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, `SpaceSettingsDialog` is using an inline props type instead of a named interface. Introduce an explicit TypeScript interface for the component props and update `SpaceSettingsDialog` to use that interface rather than `{ id: number; space: Space }`. Keep the interface close to the component so it’s easy to find and reuse if needed.Source: Coding guidelines
core/src/ops/files/query/directory_listing.rs (1)
231-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate direction-resolution logic across SQL and in-memory sort paths.
The same
self.input.sort_direction.clone().unwrap_or_else(|| Self::default_sort_direction(&self.input.sort_by))pattern is repeated inquery_indexed_directory_implandsort_files. Extracting a small helper avoids the two paths silently diverging later.♻️ Suggested refactor
+ fn effective_sort_direction(&self) -> SortDirection { + self.input + .sort_direction + .clone() + .unwrap_or_else(|| Self::default_sort_direction(&self.input.sort_by)) + } + fn default_sort_direction(sort_by: &DirectorySortBy) -> SortDirection {Then replace both call sites with
self.effective_sort_direction().Also applies to: 820-837
🤖 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 `@core/src/ops/files/query/directory_listing.rs` around lines 231 - 239, Duplicate sort-direction resolution is repeated in both query_indexed_directory_impl and sort_files, which risks the SQL and in-memory paths diverging. Extract the shared logic into a helper on the directory listing type, such as effective_sort_direction, using the existing self.input.sort_direction and Self::default_sort_direction(&self.input.sort_by) behavior. Then replace both call sites with the new helper and keep the SQL dir_sql mapping and file sorting path consuming the same resolved SortDirection.
🤖 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 63-76: The color picker buttons in SpaceSettingsModal are missing
accessible labels, so screen readers cannot identify each swatch. Update the
button rendered inside PRESET_COLORS.map in SpaceSettingsModal to include a
meaningful aria-label for each color (for example, based on the color value or a
friendly name) while keeping the existing onClick, selectedColor, and styling
behavior unchanged.
In `@packages/interface/src/components/SpacesSidebar/SpaceSwitcher.tsx`:
- Around line 20-28: The `openDialogAfterMenuCloses` flow in `SpaceSwitcher`
schedules a `requestAnimationFrame` that can still fire after the component
unmounts, leading to stale dialog opens; store the returned raf id in a
`useRef`, cancel it in a cleanup effect on unmount, and ensure the callback only
calls `openDialog` if the component is still mounted.
In `@packages/interface/src/routes/explorer/sortUtils.ts`:
- Around line 6-11: The default sort fallback does not account for the
MediaSortBy key datetaken, so media views get the wrong direction and no
effective fallback sort. Update defaultSortOrder() in sortUtils to return "desc"
for datetaken alongside modified and size, and ensure the fallback sorter that
consumes SortBy also handles datetaken using the same media date field logic as
the other media sort keys.
---
Outside diff comments:
In `@packages/interface/src/routes/explorer/hooks/useExplorerKeyboard.ts`:
- Around line 49-56: The directory query in useExplorerKeyboard is missing
folders_first, so keyboard navigation can get out of sync with the displayed
order. Update the query built from currentPath to include folders_first using
the existing viewSettings.foldersFirst value, matching the other directory
listing queries in this hook so arrow-key navigation, typeahead, and select-all
operate on the same ordering.
---
Nitpick comments:
In `@core/src/ops/files/query/directory_listing.rs`:
- Around line 231-239: Duplicate sort-direction resolution is repeated in both
query_indexed_directory_impl and sort_files, which risks the SQL and in-memory
paths diverging. Extract the shared logic into a helper on the directory listing
type, such as effective_sort_direction, using the existing
self.input.sort_direction and Self::default_sort_direction(&self.input.sort_by)
behavior. Then replace both call sites with the new helper and keep the SQL
dir_sql mapping and file sorting path consuming the same resolved SortDirection.
In `@packages/interface/src/components/SpacesSidebar/SpaceSettingsModal.tsx`:
- Line 19: `SpaceSettingsDialog` is using an inline props type instead of a
named interface. Introduce an explicit TypeScript interface for the component
props and update `SpaceSettingsDialog` to use that interface rather than `{ id:
number; space: Space }`. Keep the interface close to the component so it’s easy
to find and reuse if needed.
🪄 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: 200078b9-f5ff-4ba1-8a51-3a7b3357c7f3
⛔ 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 (2)
- apps/tauri/src/stubs/debug.ts
- apps/tauri/src/stubs/spacebot-api-client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/interface/src/routes/explorer/ExplorerView.tsx
| {PRESET_COLORS.map((color) => ( | ||
| <button | ||
| key={color} | ||
| type="button" | ||
| onClick={() => setSelectedColor(color)} | ||
| className={clsx( | ||
| 'h-8 w-8 rounded-full border-2 transition-all', | ||
| selectedColor === color | ||
| ? 'scale-110 border-white' | ||
| : 'border-transparent', | ||
| )} | ||
| style={{ backgroundColor: color }} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add aria-labels to color picker buttons for screen reader accessibility.
The color buttons have no text content or aria-label, so screen readers announce them as generic "button" elements with no identifying context.
♿ Proposed fix
<button
key={color}
type="button"
+ aria-label={`Color ${color}`}
onClick={() => setSelectedColor(color)}
className={clsx(
'h-8 w-8 rounded-full border-2 transition-all',
selectedColor === color
? 'scale-110 border-white'
: 'border-transparent',
)}
style={{ backgroundColor: color }}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {PRESET_COLORS.map((color) => ( | |
| <button | |
| key={color} | |
| type="button" | |
| onClick={() => setSelectedColor(color)} | |
| className={clsx( | |
| 'h-8 w-8 rounded-full border-2 transition-all', | |
| selectedColor === color | |
| ? 'scale-110 border-white' | |
| : 'border-transparent', | |
| )} | |
| style={{ backgroundColor: color }} | |
| /> | |
| ))} | |
| {PRESET_COLORS.map((color) => ( | |
| <button | |
| key={color} | |
| type="button" | |
| aria-label={`Color ${color}`} | |
| onClick={() => setSelectedColor(color)} | |
| className={clsx( | |
| 'h-8 w-8 rounded-full border-2 transition-all', | |
| selectedColor === color | |
| ? 'scale-110 border-white' | |
| : 'border-transparent', | |
| )} | |
| style={{ backgroundColor: color }} | |
| /> | |
| ))} |
🤖 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 63 - 76, The color picker buttons in SpaceSettingsModal are missing
accessible labels, so screen readers cannot identify each swatch. Update the
button rendered inside PRESET_COLORS.map in SpaceSettingsModal to include a
meaningful aria-label for each color (for example, based on the color value or a
friendly name) while keeping the existing onClick, selectedColor, and styling
behavior unchanged.
| const [menuOpen, setMenuOpen] = useState(false); | ||
|
|
||
| const openDialogAfterMenuCloses = (openDialog: () => void) => { | ||
| setMenuOpen(false); | ||
| // Let the dropdown fully unmount before opening a modal overlay. | ||
| requestAnimationFrame(() => { | ||
| openDialog(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel pending requestAnimationFrame on component unmount to avoid stale dialog opens.
If the component unmounts between setMenuOpen(false) and the requestAnimationFrame callback (e.g., space switch causes re-render/remount), the callback still fires and attempts to open a dialog, which can cause a stale state update or unexpected dialog appearance.
🔒 Proposed fix
const [menuOpen, setMenuOpen] = useState(false);
+ useEffect(() => {
+ return () => cancelAnimationFrame(rafId);
+ }, []);
+
+ let rafId = 0;
+
const openDialogAfterMenuCloses = (openDialog: () => void) => {
setMenuOpen(false);
// Let the dropdown fully unmount before opening a modal overlay.
- requestAnimationFrame(() => {
+ rafId = requestAnimationFrame(() => {
openDialog();
});
};Alternatively, store the rafId in a useRef for cleaner handling:
+ const rafRef = useRef<number>(0);
+
+ useEffect(() => {
+ return () => {
+ if (rafRef.current) cancelAnimationFrame(rafRef.current);
+ };
+ }, []);
+
const openDialogAfterMenuCloses = (openDialog: () => void) => {
setMenuOpen(false);
// Let the dropdown fully unmount before opening a modal overlay.
- requestAnimationFrame(() => {
+ rafRef.current = requestAnimationFrame(() => {
openDialog();
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [menuOpen, setMenuOpen] = useState(false); | |
| const openDialogAfterMenuCloses = (openDialog: () => void) => { | |
| setMenuOpen(false); | |
| // Let the dropdown fully unmount before opening a modal overlay. | |
| requestAnimationFrame(() => { | |
| openDialog(); | |
| }); | |
| }; | |
| const [menuOpen, setMenuOpen] = useState(false); | |
| useEffect(() => { | |
| return () => cancelAnimationFrame(rafId); | |
| }, []); | |
| let rafId = 0; | |
| const openDialogAfterMenuCloses = (openDialog: () => void) => { | |
| setMenuOpen(false); | |
| // Let the dropdown fully unmount before opening a modal overlay. | |
| rafId = requestAnimationFrame(() => { | |
| openDialog(); | |
| }); | |
| }; |
🤖 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 20 - 28, The `openDialogAfterMenuCloses` flow in `SpaceSwitcher` schedules
a `requestAnimationFrame` that can still fire after the component unmounts,
leading to stale dialog opens; store the returned raf id in a `useRef`, cancel
it in a cleanup effect on unmount, and ensure the callback only calls
`openDialog` if the component is still mounted.
| export function defaultSortOrder(sortBy: SortBy): SortOrder { | ||
| if (sortBy === "modified" || sortBy === "size") { | ||
| return "desc"; | ||
| } | ||
| return "asc"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What fields does @sd/ts-client's generated MediaSortBy TypeScript type include?
💡 Result:
The MediaSortBy TypeScript type (or enum), exported by the @sd/ts-client package used in the Spacedrive project, defines the available fields for sorting media files within the application. Based on the project's codebase, the MediaSortBy enum is used to control sorting behavior in the explorer. While specific fields may evolve with version updates, they typically include: - Name - Size - DateCreated - DateModified - DateAccessed - DateTaken These fields are commonly used in the Spacedrive explorer to order media items, such as images and videos, when utilizing the Media View or other indexed file views [1][2][3]. Please note that as an auto-generated client, the exact structure is maintained within the generated code of @sd/ts-client and may correspond to internal sorting keys like name, sizeInBytes, dateModified, dateIndexed, dateCreated, and object.mediaData.epochTime (for Date Taken) [1][3].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sortUtils outline ==\n'
ast-grep outline packages/interface/src/routes/explorer/sortUtils.ts --view expanded || true
printf '\n== relevant files ==\n'
git ls-files 'packages/interface/src/routes/explorer/*' 'packages/ts-client/*' | sed -n '1,120p'
printf '\n== search for SortBy / datetaken ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'datetaken|type SortBy|interface SortBy|enum SortBy|MediaSortBy|DirectorySortBy|defaultSortOrder|sortFiles' \
packages/interface packages/ts-client | sed -n '1,240p'Repository: spacedriveapp/spacedrive
Length of output: 1971
🌐 Web query:
site:github.com/spacedriveapp/spacedrive "sortUtils.ts" datetaken MediaSortBy SortBy
💡 Result:
In the Spacedrive codebase, "datetaken" is a specific sort value associated with MediaSortBy, which is used to organize media files within the file explorer interface [1][2]. The codebase defines sorting types as follows: - MediaSortBy: A type that includes "datetaken" as a valid sorting option, specifically intended for media-focused views [1][2]. - SortBy: A union type defined as SortBy = DirectorySortBy | MediaSortBy, which centralizes the sorting options used across the explorer's various views and components [3]. Technical usage context: - The explorer components, such as MediaView, often set the default sort order to "datetaken" when the user switches to media-specific views [4][5]. - Logic in the codebase handles the transitions between these sort types. For instance, code may automatically update the sort preference from "modified" to "datetaken" when switching into a media view, and vice versa when exiting [1]. - While "sortUtils.ts" is not explicitly detailed in the provided search highlights as a singular file, the sorting logic is consistently managed via the Explorer context and hooks that utilize the MediaSortBy and DirectorySortBy types imported from the @sd/ts-client package [1][6][3].
Citations:
Handle datetaken in the sort fallback.
MediaSortBy includes datetaken, but defaultSortOrder() still falls back to "asc" for it, and the fallback sorter doesn’t handle it either. That leaves media views with the wrong default direction and a no-op sort for this key.
🤖 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/sortUtils.ts` around lines 6 - 11, The
default sort fallback does not account for the MediaSortBy key datetaken, so
media views get the wrong direction and no effective fallback sort. Update
defaultSortOrder() in sortUtils to return "desc" for datetaken alongside
modified and size, and ensure the fallback sorter that consumes SortBy also
handles datetaken using the same media date field logic as the other media sort
keys.
|
Motivation: Chapters:
|
This branch has not been deployed
2 participants
Summary
Test plan