fix(explorer): sort direction reliability + stability fixes against hangs - #3079
Open
gprot42 wants to merge 15 commits into
Open
fix(explorer): sort direction reliability + stability fixes against hangs#3079gprot42 wants to merge 15 commits into
gprot42 wants to merge 15 commits into
Conversation
Add sort_direction to directory listing queries and per-tab sortOrder state so clicking Name (or any sort field) reverses between ascending and descending order instead of staying locked at A-Z.
WalkthroughExplorer sorting now carries sort direction through tab state, menus, and directory listing queries. Core and network filesystem checks use async-safe operations, while Tauri startup, sidebar dialogs, sources navigation, and settings styling receive targeted UI and config updates. ChangesSort direction feature
Async filesystem safety
Tauri boot and dependency setup
Spaces and sources UI flow
Settings styling
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.
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 directory listing query for keyboard navigation consistency.The query input is missing
folders_first: viewSettings.foldersFirst, whileuseExplorerFiles(line 223) andColumn(line 141) both include it. WhenfoldersFirstis enabled, this hook'sfilesarray (line 63) will be in a different order than the displayed file list, causing arrow-key navigation to select the wrong file.
viewSettingsis already destructured at line 22, so the fix is a one-line addition.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 49 - 56, Add folders_first to the directory listing query in useExplorerKeyboard so keyboard navigation matches the displayed list when foldersFirst is enabled. Update the input object built from currentPath to include folders_first: viewSettings.foldersFirst, keeping it consistent with useExplorerFiles and Column, and use the already available viewSettings value in this hook.
🧹 Nitpick comments (1)
packages/interface/src/components/TabManager/TabManagerContext.tsx (1)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate duplicate
SortOrdertype definition.
SortOrderis defined identically in both this file (line 87) andsortUtils.ts(line 4). While structurally compatible, having two independent definitions risks future divergence. Consider importing from a single source — either haveTabManagerContext.tsximportSortOrderfromsortUtils.ts, or extract it to a shared types module.🤖 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/TabManager/TabManagerContext.tsx` around lines 87 - 93, Consolidate the duplicate SortOrder definition by using a single shared type source instead of declaring it again in TabManagerContext.tsx. Update the TabExplorerState-related types to import SortOrder from sortUtils.ts, or move SortOrder into a shared types module and import it from both places, keeping the existing TabExplorerState and sortUtils symbols aligned.
🤖 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 49-56: Add folders_first to the directory listing query in
useExplorerKeyboard so keyboard navigation matches the displayed list when
foldersFirst is enabled. Update the input object built from currentPath to
include folders_first: viewSettings.foldersFirst, keeping it consistent with
useExplorerFiles and Column, and use the already available viewSettings value in
this hook.
---
Nitpick comments:
In `@packages/interface/src/components/TabManager/TabManagerContext.tsx`:
- Around line 87-93: Consolidate the duplicate SortOrder definition by using a
single shared type source instead of declaring it again in
TabManagerContext.tsx. Update the TabExplorerState-related types to import
SortOrder from sortUtils.ts, or move SortOrder into a shared types module and
import it from both places, keeping the existing TabExplorerState and sortUtils
symbols aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2a58558-146f-4483-90c3-344031611a87
⛔ Files ignored due to path filters (1)
packages/ts-client/src/generated/types.tsis excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (14)
apps/cli/src/domains/file/mod.rscore/src/ops/files/query/directory_listing.rscore/tests/normalized_cache_fixtures_test.rspackages/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
added 2 commits
July 8, 2026 19:08
- Explorer: bound directory_listing to 10k results + removed unconditional client-side sortFiles for directory views (backend already sorts; avoids main-thread work + memory blowup on huge dirs) - Core: replaced sync Path::exists / metadata / canonicalize in async paths (add_location, validate, volume_for_path, local backend, etc.) with tokio equivalents or spawn_blocking - Eliminated risky block_on calls: - file_delete protocol now fully async (like file_transfer) - extension host uses block_in_place for WASM host fns - Guarded viewMode/sort auto-adjust effect in explorer context to prevent re-render/query churn from setSortBy identity changes - Bonus: non-blocking canonicalize in file transfer/delete paths These are the quick wins + high-risk items (spacedriveapp#2 block_on, spacedriveapp#3 large dirs, spacedriveapp#4 churn) plus related blocking FS fixes identified while investigating hangs. Builds on the A-Z/Z-A sort direction reliability work.
gprot42
changed the title
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/sortUtils.ts (1)
33-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
createdinpackages/interface/src/routes/explorer/sortUtils.ts.
Virtual listings can keep the"created"sort selected, butsortFilesfalls through todefault, so creation-time ordering never applies.🤖 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 33 - 80, The sortFiles fallback in sortUtils.ts does not handle the "created" sort option, so virtual listings with that selection currently fall through to the default path. Update the sortFiles switch to include a "created" case alongside "name", "modified", "size", and "type", and compare the file creation timestamp field used by File objects so the client-side fallback matches daemon sorting. Keep the existing direction and foldersFirst behavior intact when adding the new case.
🤖 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/sortUtils.ts`:
- Around line 33-80: The sortFiles fallback in sortUtils.ts does not handle the
"created" sort option, so virtual listings with that selection currently fall
through to the default path. Update the sortFiles switch to include a "created"
case alongside "name", "modified", "size", and "type", and compare the file
creation timestamp field used by File objects so the client-side fallback
matches daemon sorting. Keep the existing direction and foldersFirst behavior
intact when adding the new case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2da8269c-2c1b-40dd-ad77-a6a9bab1f4a3
📒 Files selected for processing (14)
core/src/infra/extension/host_functions.rscore/src/location/manager.rscore/src/location/mod.rscore/src/service/network/protocol/file_delete.rscore/src/service/network/protocol/file_transfer.rscore/src/volume/backend/local.rscore/src/volume/manager.rspackages/interface/src/components/TabManager/TabManagerContext.tsxpackages/interface/src/routes/explorer/ExplorerView.tsxpackages/interface/src/routes/explorer/SortMenu.tsxpackages/interface/src/routes/explorer/context.tsxpackages/interface/src/routes/explorer/hooks/useExplorerFiles.tspackages/interface/src/routes/explorer/sortUtils.tspackages/interface/src/routes/explorer/views/ListView/useTable.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/interface/src/routes/explorer/ExplorerView.tsx
- packages/interface/src/routes/explorer/context.tsx
- Add immediate dark background styles + loading hint in index.html so the transparent Tauri window no longer flashes native grey before Vite/React paint. - Add virtual pure-JS stubs in vite.config.ts for: - @spacebot/api-client (when sibling repo missing) - hast-util-to-jsx-runtime - style-to-js These were causing module resolution / CJS default export errors that left #root empty (grey screen). - Extend optimizeDeps exclude for the problematic CJS transitives. The real Tauri webview now gets a solid dark initial paint and the module graph loads without errors.
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 `@apps/tauri/vite.config.ts`:
- Around line 32-36: The `@spacebot/api-client` fallback in the Vite config is
too minimal: it only exports an empty `apiClient`, but Spacebot routes still
call methods like `listTasks`, `channelMessages`, `listWorkers`,
`listPortalConversations`, `portalHistory`, `createPortalConversation`,
`portalSend`, `workerDetail`, `cancelProcess`, `ttsGenerate`, and
`webChatSendAudio`. Update the stub generated in `vite.config.ts` so the
exported `apiClient` includes no-op implementations for the full surface used by
the Spacebot UI, or alternatively gate/disable those routes when the sibling
repo is missing. Use the existing `apiClient`, `getEventsUrl`, and
`setServerUrl` exports as the anchor points when making the fallback consistent.
🪄 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: afd6a2a7-4e85-4413-a577-3b8e1537f600
📒 Files selected for processing (2)
apps/tauri/index.htmlapps/tauri/vite.config.ts
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
added 11 commits
July 9, 2026 00:04
Add Vite aliases and stubs for Bun-hoisted packages and the optional @spacebot/api-client dependency so tauri:dev starts without resolve errors.
Opening the adapters picker via createTab left duplicate Sources/Adapters tabs in the tab bar. Navigate within the current tab, normalize persisted tabs on load, and keep the tab title as Sources while on /sources/adapters.
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.
Release builds start with visible:false. If the webview fails to boot, force-show after 3s so the app is not stuck with zero windows.
Daemon startup often exceeds the previous 3s timeout because of volume discovery and networking init before the RPC socket binds. Increase the wait so the app does not fail launch with a hidden window.
Directory listings stopped client-side sorting for performance, but ResourceChanged and ephemeral index events append files in discovery order and scramble SQL/backend order. Re-apply locale-aware sort on the capped result set, and use COLLATE NOCASE for SQL name sorts.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 39: The SpaceSettingsModal close handler is mutating dialog state
directly via dialog.state.open, which should be replaced with the same
dialogManager.setState(dialog.id, { open: false }) pattern used elsewhere in
this PR. Update the code in SpaceSettingsModal to call dialogManager.setState
for closing the modal instead of changing dialog.state.open in place, keeping
the behavior consistent with AddGroupModal and using the existing dialogManager
API.
In `@packages/interface/src/components/TabManager/TabManagerContext.tsx`:
- Around line 348-382: The `setTabs` updater in `TabManagerContext` is impure
because it generates `crypto.randomUUID()` and triggers `setActiveTabId`,
`setExplorerStates`, and `setSelectionStates` inside the updater. Move UUID
creation and the other state updates out of the `setTabs` callback, then pass
the generated id into the updater so it only returns the next tabs array. Keep
the logic in the same tab-creation flow around `tabPath`, `deriveTitleFromPath`,
`newTab`, and the explorer/selection state initialization, but make the updater
itself deterministic.
🪄 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: 41306a17-a094-430d-aa78-6f4f37b8a64b
📒 Files selected for processing (22)
apps/tauri/src-tauri/src/main.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.rspackages/interface/src/Settings/pages/AdvancedSettings.tsxpackages/interface/src/Settings/pages/ServicesSettings.tsxpackages/interface/src/components/SpacesSidebar/AddGroupModal.tsxpackages/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/components/TabManager/TabNavigationSync.tsxpackages/interface/src/routes/explorer/ExplorerView.tsxpackages/interface/src/routes/explorer/hooks/useExplorerFiles.tspackages/interface/src/routes/explorer/sortUtils.tspackages/interface/src/routes/settings/index.tsxpackages/interface/src/routes/sources/Adapters.tsxpackages/interface/src/routes/sources/index.tsxpackages/interface/src/routes/sources/useOpenAdaptersPicker.ts
✅ Files skipped from review due to trivial changes (4)
- apps/tauri/src/stubs/debug.ts
- packages/interface/src/Settings/pages/ServicesSettings.tsx
- packages/interface/src/Settings/pages/AdvancedSettings.tsx
- packages/interface/src/components/SpacesSidebar/spacePresets.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/interface/src/routes/explorer/sortUtils.ts
- core/src/ops/files/query/directory_listing.rs
| icon: selectedIcon, | ||
| color: selectedColor, | ||
| }); | ||
| dialog.state.open = false; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use dialogManager.setState instead of direct dialog.state.open mutation.
AddGroupModal.tsx in this same PR was explicitly changed from dialog.state.open = false to dialogManager.setState(dialog.id, { open: false }). This new file should follow the same pattern for consistency and to avoid potential state batching issues with direct mutation.
🔧 Proposed fix
color: selectedColor,
});
- dialog.state.open = false;
+ dialogManager.setState(dialog.id, { open: false });
});📝 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.
Suggested change
| dialog.state.open = false; | |
| dialogManager.setState(dialog.id, { open: false }); |
🤖 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 39, The SpaceSettingsModal close handler is mutating dialog state directly
via dialog.state.open, which should be replaced with the same
dialogManager.setState(dialog.id, { open: false }) pattern used elsewhere in
this PR. Update the code in SpaceSettingsModal to call dialogManager.setState
for closing the modal instead of changing dialog.state.open in place, keeping
the behavior consistent with AddGroupModal and using the existing dialogManager
API.
Comment on lines
348
to
+382
|
|
||
| // Initialize explorer state for the new tab | ||
| setExplorerStates((prev) => | ||
| new Map(prev).set(newTab.id, { ...DEFAULT_EXPLORER_STATE }), | ||
| ); | ||
| setTabs((prev) => { | ||
| const existing = prev.find((tab) => tab.savedPath === tabPath); | ||
| if (existing) { | ||
| setActiveTabId(existing.id); | ||
| return prev; | ||
| } | ||
|
|
||
| const [pathname, search = ""] = tabPath.split("?"); | ||
| const derivedTitle = | ||
| title || | ||
| deriveTitleFromPath(pathname, search ? `?${search}` : ""); | ||
|
|
||
| const newTab: Tab = { | ||
| id: crypto.randomUUID(), | ||
| title: derivedTitle, | ||
| icon: null, | ||
| isPinned: false, | ||
| lastActive: Date.now(), | ||
| savedPath: tabPath, | ||
| }; | ||
|
|
||
| setExplorerStates((explorerPrev) => | ||
| new Map(explorerPrev).set(newTab.id, { | ||
| ...DEFAULT_EXPLORER_STATE, | ||
| }), | ||
| ); | ||
|
|
||
| // Initialize empty selection state for the new tab | ||
| setSelectionStates((prev) => new Map(prev).set(newTab.id, [])); | ||
| setSelectionStates((selectionPrev) => | ||
| new Map(selectionPrev).set(newTab.id, []), | ||
| ); | ||
|
|
||
| setTabs((prev) => [...prev, newTab]); | ||
| setActiveTabId(newTab.id); | ||
| setActiveTabId(newTab.id); | ||
| return [...prev, newTab]; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move crypto.randomUUID() and state-setter calls out of the setTabs updater.
The updater function passed to setTabs contains impure side effects: crypto.randomUUID() is non-deterministic, and setActiveTabId, setExplorerStates, and setSelectionStates are called inside it. In React Strict Mode, updaters are double-invoked — the first call generates UUID1 and creates state entries for it, the second generates UUID2. React commits only the second result (UUID2), leaving orphaned explorerStates and selectionStates entries for UUID1.
Generating the UUID before the updater and passing it in makes the updater pure and eliminates the orphaned-state problem.
🔧 Proposed fix
const createTab = useCallback(
(title?: string, path?: string) => {
const tabPath = path ?? defaultNewTabPath;
+ const newTabId = crypto.randomUUID();
setTabs((prev) => {
const existing = prev.find((tab) => tab.savedPath === tabPath);
if (existing) {
setActiveTabId(existing.id);
return prev;
}
const [pathname, search = ""] = tabPath.split("?");
const derivedTitle =
title ||
deriveTitleFromPath(pathname, search ? `?${search}` : "");
const newTab: Tab = {
- id: crypto.randomUUID(),
+ id: newTabId,
title: derivedTitle,
icon: null,
isPinned: false,
lastActive: Date.now(),
savedPath: tabPath,
};
setExplorerStates((explorerPrev) =>
- new Map(explorerPrev).set(newTab.id, {
+ new Map(explorerPrev).set(newTabId, {
...DEFAULT_EXPLORER_STATE,
}),
);
setSelectionStates((selectionPrev) =>
- new Map(selectionPrev).set(newTab.id, []),
+ new Map(selectionPrev).set(newTabId, []),
);
- setActiveTabId(newTab.id);
+ setActiveTabId(newTabId);
return [...prev, newTab];
});
},
[defaultNewTabPath],
);🤖 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/TabManager/TabManagerContext.tsx` around
lines 348 - 382, The `setTabs` updater in `TabManagerContext` is impure because
it generates `crypto.randomUUID()` and triggers `setActiveTabId`,
`setExplorerStates`, and `setSelectionStates` inside the updater. Move UUID
creation and the other state updates out of the `setTabs` callback, then pass
the generated id into the updater so it only returns the next tabs array. Keep
the logic in the same tab-creation flow around `tabPath`, `deriveTitleFromPath`,
`newTab`, and the explorer/selection state initialization, but make the updater
itself deterministic.
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.
fix: stability fixes to prevent application hangs (explorer + core)
client-side sortFiles for directory views (backend already sorts; avoids
main-thread work + memory blowup on huge dirs)
(add_location, validate, volume_for_path, local backend, etc.) with
tokio equivalents or spawn_blocking
re-render/query churn from setSortBy identity changes
These are the quick wins + high-risk items (#2 block_on, #3 large dirs,
#4 churn) plus related blocking FS fixes identified while investigating
hangs.
Builds on the A-Z/Z-A sort direction reliability work.