fix(interface): navigate to adapters picker in-tab - #3075
Conversation
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.
WalkthroughTab persistence logic in TabManagerContext deduplicates tabs by savedPath and merges /sources with /sources/adapters, renaming the kept tab "Sources". createTab now reuses existing tabs for the same path. TabNavigationSync and Adapters route update titles/navigation accordingly. SourcesHome adopts a new useOpenAdaptersPicker hook replacing direct tab creation. ChangesTab deduplication and Sources/Adapters merge
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant SourcesHome
participant Picker as useOpenAdaptersPicker
participant TabManager
participant Router
SourcesHome->>Picker: openAdaptersPicker()
Picker->>TabManager: close stale /sources/adapters tabs
Picker->>Router: navigate to /sources/adapters
Router-->>TabManager: TabNavigationSync derives title "Sources"
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
🧹 Nitpick comments (1)
packages/interface/src/components/TabManager/TabManagerContext.tsx (1)
245-278: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEliminate redundant
normalizeTabscall inactiveTabIdinitializer.The
activeTabIdinitializer callsloadPersistedState()andnormalizeTabs(persisted.tabs)again, even thoughtabs(already normalized) is defined right above and accessible in this closure. Usetabsdirectly to avoid the redundant computation.♻️ Proposed refactor
const [activeTabId, setActiveTabId] = useState<string>(() => { const persisted = loadPersistedState(); if (persisted && persisted.activeTabId) { - const normalized = normalizeTabs(persisted.tabs); - const tabExists = normalized.some( + const tabExists = tabs.some( (t) => t.id === persisted.activeTabId, ); if (tabExists) return persisted.activeTabId; - if (normalized[0]) return normalized[0].id; + if (tabs[0]) return tabs[0].id; } return tabs[0].id; });🤖 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 245 - 278, The activeTabId initializer in TabManagerContext redundantly reloads persisted state and calls normalizeTabs(persisted.tabs) even though the already-initialized tabs state is available in the same closure. Update the activeTabId useState initializer to rely on tabs directly for existence checks and fallback selection, keeping the same behavior while removing the duplicate normalization work.
🤖 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/TabManager/TabManagerContext.tsx`:
- Around line 337-370: The tab creation logic inside the setTabs updater is
impure because it also calls setActiveTabId, setExplorerStates, and
setSelectionStates, which can run twice in Strict Mode and create orphaned
per-tab state. In TabManagerContext’s tab creation flow, generate the new tab ID
and newTab object first, then move the active-tab update plus the
explorer/selection state initialization outside the setTabs callback so the
updater only returns the next tabs array.
---
Nitpick comments:
In `@packages/interface/src/components/TabManager/TabManagerContext.tsx`:
- Around line 245-278: The activeTabId initializer in TabManagerContext
redundantly reloads persisted state and calls normalizeTabs(persisted.tabs) even
though the already-initialized tabs state is available in the same closure.
Update the activeTabId useState initializer to rely on tabs directly for
existence checks and fallback selection, keeping the same behavior while
removing the duplicate normalization work.
🪄 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: 7182ae86-cee3-4d1f-8b52-a2e138ba0b12
📒 Files selected for processing (5)
packages/interface/src/components/TabManager/TabManagerContext.tsxpackages/interface/src/components/TabManager/TabNavigationSync.tsxpackages/interface/src/routes/sources/Adapters.tsxpackages/interface/src/routes/sources/index.tsxpackages/interface/src/routes/sources/useOpenAdaptersPicker.ts
| 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 | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/interface/src/components/TabManager/TabManagerContext.tsx"
echo "== file info =="
wc -l "$FILE"
echo
echo "== outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== relevant slice around 337-370 =="
sed -n '300,420p' "$FILE" | cat -n
echo
echo "== search for cleanup / state syncing =="
rg -n "setExplorerStates|setSelectionStates|setActiveTabId|DEFAULT_EXPLORER_STATE|savedPath|createTab|removeTab|deleteTab|closeTab|cleanup" "$FILE"Repository: spacedriveapp/spacedrive
Length of output: 8044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/interface/src/components/TabManager/TabManagerContext.tsx"
echo "== loadPersistedState / savePersistedState / normalizeTabs =="
sed -n '120,220p' "$FILE" | cat -n
echo
echo "== state initialization and persistence =="
sed -n '240,340p' "$FILE" | cat -n
echo
echo "== any cleanup of explorer/selection state outside closeTab =="
rg -n "setExplorerStates|setSelectionStates|delete\\(|Object\\.fromEntries|loadPersistedState|savePersistedState|normalizeTabs" "$FILE"Repository: spacedriveapp/spacedrive
Length of output: 8402
🌐 Web query:
React StrictMode double-invokes state updater functions functional updater side effects randomUUID
💡 Result:
In React Strict Mode, the double-invocation of state updater functions is an intentional behavior designed to help identify impure code during development [1][2][3]. Why React invokes updater functions twice: React Strict Mode runs certain functions twice in development—including state updater functions—to ensure that your logic is idempotent (producing the same result regardless of how many times it is called) [1][4][3]. Because React may choose to re-render or pause components during concurrent rendering, updater functions must be pure and free of side effects [5][6][3]. If an updater function contains side effects, the double-invocation will surface them, helping you catch potential bugs that could otherwise lead to inconsistent state or unexpected behavior in production [4][6][7]. Impurity and side effects: Any code that modifies variables outside the function scope, performs API calls, or generates random values during the render or update phase is considered impure [5][6][8]. If your updater function performs these actions, it will likely produce incorrect results (e.g., doubling an increment or generating two different IDs) when invoked twice [6][9][10]. Handling crypto.randomUUID(): You should not call crypto.randomUUID() directly inside a state updater function or render body [8]. Because these functions are expected to be pure, calling a non-deterministic function like randomUUID() will result in different values being produced each time they are invoked [8]. To use a unique ID safely: 1. Generate the ID inside an event handler (e.g., when a user clicks a button to add an item) [5][6][11]. 2. If the ID must be generated upon component initialization, use lazy state initialization: useState(() => crypto.randomUUID()) [8]. 3. By moving the generation of the UUID outside of the pure update phase, you ensure the value remains stable and predictable [8][11]. Summary for developers: The double-invocation is not a bug; it is a test harness [7]. If your code fails when run twice, it indicates a lack of idempotency [3][7]. Ensure your state updaters are pure, move side effects to event handlers or useEffect (with proper cleanup), and avoid non-deterministic operations like randomUUID() inside your render or update logic [5][6][8][7].
Citations:
- 1: https://react.dev/reference/react/StrictMode
- 2: https://www.facebook.com/fb-answers/react-strict-mode-updater-function-called-twice-usestate-docs/
- 3: https://stackoverflow.com/questions/60305074/react-strictmode-setstate-function-in-useeffect-is-run-multiple-times-when-effe
- 4: React.StrictMode causes setState to fire twice react/react#12856
- 5: https://react.dev/reference/rules/components-and-hooks-must-be-pure
- 6: https://react.dev/learn/keeping-components-pure
- 7: https://afterbuildlabs.com/fix/react-strict-mode-double-render
- 8: https://www.eslint-react.xyz/docs/rules/purity
- 9: https://stackoverflow.com/questions/72850895/react-strictmode-causing-my-variable-to-increment-twice
- 10: https://devsolus.com/react-strictmode-causing-my-variable-to-increment-twice/
- 11: https://www.epicreact.dev/generating-unique-ids-for-client-only-data-in-react-c6jjc
Move tab creation side effects out of the setTabs updater setActiveTabId, setExplorerStates, and setSelectionStates make this callback impure; in Strict Mode it can run twice and leave per-tab state behind for a discarded UUID. Generate the tab ID first, then perform the other state updates outside setTabs.
🤖 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 337 - 370, The tab creation logic inside the setTabs updater is impure
because it also calls setActiveTabId, setExplorerStates, and setSelectionStates,
which can run twice in Strict Mode and create orphaned per-tab state. In
TabManagerContext’s tab creation flow, generate the new tab ID and newTab object
first, then move the active-tab update plus the explorer/selection state
initialization outside the setTabs callback so the updater only returns the next
tabs array.
1 participant
Summary
navigate()instead ofcreateTab(), since it is a sub-page of Sources/sources/adaptersTest plan
Note
Key Changes: The adapters picker is now treated as a sub-page within the Sources tab rather than a separate tab. New
normalizeTabs()function deduplicates old/sources/adapterstabs from persisted state and normalizes their titles. TheuseOpenAdaptersPickerhook handles cleanup of stale adapter tabs and navigation. The back button now navigates to/sourcesinstead of browser history.Written by Tembo for commit 889f605. This will update automatically on new commits.