Tab bar fix #1489
Tab bar fix #1489
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMove tab/space UI to a store-driven flow using Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as TabBar / SpacePage
participant Store
participant API
Note over UI,Store: User triggers tab operation (create / rename / delete / switch)
User->>UI: click create/rename/delete/switch
UI->>Store: dispatch action (createTab / renameSpaceTab / deleteTab / setCurrentTabName)
Store->>Store: withOptimisticUpdate → updateFn (apply optimistic/local change)
Store->>UI: optimistic state rendered
Store->>API: commitFn (persist change)
alt commit success
API-->>Store: success
Store->>UI: committed state rendered
else commit failure
API-->>Store: error
Store->>Store: rollbackFn (restore previous state)
Store->>UI: rolled-back state + error surfaced
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/(spaces)/PublicSpace.tsx (1)
248-265: Don’t read stalelocalSpaceswhen loading post-registration tabsInside the registration effect,
tabOrderis pulled from thelocalSpacessnapshot that was captured when the effect first ran. Because you mutate the store vialoadSpaceTabOrder,loadEditableSpaces, andloadSpaceTab, thelocalSpacesreference inside this closure never sees those updates, sotabOrderresolves to[]. That means the “load remaining tabs” loop is a no-op, and additional tabs for a freshly registered space never hydrate locally.Re-fetch the latest state from the store before iterating. For example:
- const tabOrder = localSpaces[newSpaceId]?.order || []; + const latestLocalSpaces = get().space.localSpaces; + const tabOrder = latestLocalSpaces[newSpaceId]?.order || []; for (const tabName of tabOrder) { if (tabName !== spacePageData.defaultTab) { await loadSpaceTab(newSpaceId, tabName); } }This ensures you’re working with the up-to-date tab order populated by the prior async calls.
src/common/components/organisms/TabBar.tsx (1)
153-191: Restore URL when an optimistic rename fails
debouncedRenameTabswitches the URL to the tentativeuniqueNamebefore awaitingrenameTab. IfrenameTabthrows (network error, validation, etc.),withOptimisticUpdaterolls the store back to the old tab name, but the router stays on the new path (router.replaceisn’t reversed). The user ends up on a URL that no longer matches the active tab, and navigation/bookmarking breaks until they manually click away.Wrap the
await renamePromisein a try/catch and, on failure, swap the router back to the original tab:- await renamePromise; - commitTab(uniqueName); - commitTabOrder(); + try { + await renamePromise; + commitTab(uniqueName); + commitTabOrder(); + } catch (error) { + updateUrl(tabName); + setCurrentTabName(tabName); + throw error; + }This keeps the URL in sync with the rolled-back store state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/(spaces)/PublicSpace.tsx(11 hunks)src/app/(spaces)/homebase/PrivateSpace.tsx(5 hunks)src/common/components/organisms/TabBar.tsx(6 hunks)src/common/data/stores/app/homebase/homebaseTabsStore.ts(6 hunks)src/common/data/stores/app/space/spaceStore.ts(7 hunks)src/common/utils/tabUtils.ts(1 hunks)supabase/migrations/20250929120000_add_channel_spaces.sql.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/common/data/stores/app/space/spaceStore.ts (2)
src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/common/utils/tabUtils.ts (3)
validateTabName(11-16)isDuplicateTabName(25-34)withOptimisticUpdate(46-74)
src/app/(spaces)/homebase/PrivateSpace.tsx (1)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)
src/common/utils/tabUtils.ts (3)
validateTabName(11-16)isDuplicateTabName(25-34)withOptimisticUpdate(46-74)src/pages/api/space/homebase/tabs/index.ts (1)
UnsignedManageHomebaseTabsRequest(24-29)src/common/lib/signedFiles.ts (1)
signSignable(54-62)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
src/common/components/organisms/TabBar.tsx (4)
src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/common/data/stores/app/currentSpace/index.ts (1)
setCurrentTabName(51-55)src/common/data/stores/app/homebase/homebaseTabsStore.ts (3)
createTab(229-309)deleteTab(310-347)renameTab(348-449)src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/app/(spaces)/PublicSpace.tsx (5)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-78)
🔇 Additional comments (1)
supabase/migrations/20250929120000_add_channel_spaces.sql.sql (1)
2-18: Well-designed migration using partial unique index pattern.The migration correctly implements channel space support:
- Column addition (lines 2–3) is safely guarded with
IF NOT EXISTSand nullable by default.- Constraint update (lines 6–12) idempotently replaces the old validation with one that includes the new
'channel'spaceType; existing data remains valid.- Partial unique index (lines 15–18) is the correct pattern here: it allows multiple NULL values (for non-channel spaces) while enforcing uniqueness only on non-NULL
channelIdvalues. The comment clearly explains this design choice.All operations are properly guarded to ensure idempotency and backward compatibility.
Verify this migration in context of the broader PR changes.
Since the PR summary mentions refactoring tab/space management and introducing optimistic updates, please ensure:
- The migration is tested alongside the tab/space management code changes (e.g., creation, rename, delete operations for channel spaces).
- Any application code that inserts into
spaceRegistrationscorrectly populates or leaveschannelIdNULL as appropriate.- The optimistic update patterns in the application layer handle the case where an operation might roll back after the index constraint is checked.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
325-346: Fix the dependency array for saveConfig.Line 345 includes
config.fidgetInstanceDatumsin the dependency array, butconfigis a derived object created at lines 182-187. This will causesaveConfigto be recreated on every render where the config object identity changes, even if the underlying data hasn't changed. This can lead to performance issues and unnecessary re-renders of child components.Apply this diff to fix the dependency:
const saveConfig = useCallback( async (spaceConfig: SpaceConfigSaveDetails) => { if (isNil(currentSpaceId)) { throw new Error("Cannot save config until space is registered"); } const saveableConfig = { ...spaceConfig, fidgetInstanceDatums: spaceConfig.fidgetInstanceDatums ? mapValues(spaceConfig.fidgetInstanceDatums, (datum) => ({ ...datum, config: { settings: datum.config.settings, editable: datum.config.editable, }, })) : undefined, isPrivate: false, }; return saveLocalSpaceTab(currentSpaceId, currentTabName, saveableConfig); }, - [currentSpaceId, currentTabName, saveLocalSpaceTab, config.fidgetInstanceDatums] + [currentSpaceId, currentTabName, saveLocalSpaceTab] );
🧹 Nitpick comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
156-169: Consider adding cleanup for async operations.The effect contains async operations without cleanup, which could cause issues if the component unmounts or dependencies change while the promises are still pending. This could lead to state updates on unmounted components or stale data being loaded.
Apply this diff to add proper cleanup:
useEffect(() => { if (!currentSpaceId || !currentTabName) return; + let cancelled = false; + const loadSpace = async () => { try { await loadSpaceTabOrder(currentSpaceId); + if (cancelled) return; await loadSpaceTab(currentSpaceId, currentTabName); } catch (error) { console.error("Error loading space:", error); } }; loadSpace(); + + return () => { + cancelled = true; + }; }, [currentSpaceId, currentTabName, loadSpaceTabOrder, loadSpaceTab]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(spaces)/PublicSpace.tsx(11 hunks)src/app/(spaces)/homebase/PrivateSpace.tsx(5 hunks)src/common/components/organisms/TabBar.tsx(6 hunks)src/common/data/stores/app/space/spaceStore.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/common/components/organisms/TabBar.tsx (3)
src/common/data/stores/app/currentSpace/index.ts (1)
setCurrentTabName(51-55)src/common/data/stores/app/homebase/homebaseTabsStore.ts (3)
createTab(229-309)deleteTab(310-347)renameTab(348-449)src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/common/data/stores/app/space/spaceStore.ts (2)
src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/common/utils/tabUtils.ts (3)
validateTabName(11-16)isDuplicateTabName(25-34)withOptimisticUpdate(46-74)
src/app/(spaces)/homebase/PrivateSpace.tsx (1)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)
src/app/(spaces)/PublicSpace.tsx (5)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-78)
🔇 Additional comments (10)
src/app/(spaces)/homebase/PrivateSpace.tsx (2)
154-170: LGTM!The TabBar memoization correctly includes
currentTabNameandtabNamein the dependency array, ensuring proper re-renders when the active tab changes. ThedefaultTab="Feed"prop andcurrentTab={currentTabName || tabName}provide appropriate fallback behavior.
92-102: Verify the intended behavior of Feed config loading.The review comment correctly identifies that the effect will re-run when
homebaseConfigchanges. However, the underlying cause differs from the stated reasoning: the functions fromuseAppStoreare stable (bound to the store instance), but the effect logic itself has a conditional flow issue.Specifically:
- When the component first renders with
tabName="Feed"and!homebaseConfig, the effect callsloadFeedConfig()- When
homebaseConfigbecomes defined asynchronously, the effect re-runs (due to the dependency)- At that point, the condition
tabName === "Feed" && !homebaseConfigbecomes false, soloadTab(tabName)executes insteadWhether this is intentional or a bug depends on your Feed loading flow. If
loadFeedConfig()should be the final action without a subsequentloadTab()call, then splitting the effects as suggested would prevent this behavior. However, if callingloadTab()after config loads is correct, the current code may be fine.Manually verify that the effect behavior matches your intended data-loading sequence for the Feed tab.
src/app/(spaces)/PublicSpace.tsx (1)
96-102: LGTM!The effect properly sets the current space and tab name when the space data changes, with appropriate dependencies to trigger re-runs when
spacePageData.spaceId,providedTabName, orspacePageData.defaultTabchange.src/common/components/organisms/TabBar.tsx (3)
67-71: LGTM!The
switchTabTofunction provides a clean, centralized way to handle tab navigation by updating both the store state and the URL. TheshouldSaveparameter provides flexibility for different use cases.
186-209: LGTM!The tab name generation logic is well-designed:
generateNewTabNamestarts counting fromtabList.length + 1and has a reasonable fallback usingDate.now()generateUniqueTabNamevalidates the input, handles duplicates with numeric suffixes, and has a safety limit to prevent infinite loops
82-110: Error handling needed for tab switching and order commit; verify ifcommitTabOrder()is redundant.The create tab flow correctly handles errors from
createTab(line 105), but ifswitchTabTo(line 100) orcommitTabOrder(line 103) fail, there's no error handling.Regarding the potential redundancy of
commitTabOrder(): A TODO comment in PrivateSpace indicates that component has not yet been updated to use optimistic updates like PublicSpace. This suggests that in at least some code paths,createTabmay only perform local state updates without persisting to the backend, makingcommitTabOrder()necessary rather than redundant. Verify whether this applies to your use case before removing the call.src/common/data/stores/app/space/spaceStore.ts (4)
239-276: LGTM!The refactored
saveLocalSpaceTabcorrectly:
- Removes the rename logic (now in a separate
renameSpaceTabfunction)- Handles both new and existing tabs
- Updates timestamps appropriately
- Uses proper deep cloning and merging
364-386: Verify currentTabName update logic during rename.Lines 382-384 update
draft.currentSpace.currentTabNamewhen renaming a tab, but only if the current tab matches the old tab name. This logic is correct. However, the same update appears in the rollback at lines 415-419, which is also correct.The optimistic update flow looks solid:
- Updates tab config, changedNames mapping, order, and timestamps
- Conditionally updates currentTabName only if renaming the current tab
- Rollback restores all state
359-395: LGTM!The network resolution logic (lines 359-361) properly falls back through multiple sources:
- Provided
networkparameter- Local space's network property
- Remote space's network property
The
commitFn(lines 387-395) correctly handles both sync and async return values fromcommitSpaceTabToDatabase, awaiting only if a promise is returned.
1328-1348: LGTM!The enhanced
loadEditableSpacescorrectly populates missing metadata inlocalSpaces:
- Only updates entries that don't exist or are missing contract metadata
- Preserves existing tabs, order, and changedNames
- Adds contractAddress, network, fid, proposalId, and channelId from remote data
This ensures the local cache has complete metadata for all editable spaces.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
418-430: Critical: Remove invalid await on config.Line 420 attempts to
await config, butconfigis a plain object (defined at lines 185-193), not a Promise. This will causeresolvedConfigto simply be the config object itself, which works but is misleading and incorrect usage ofawait.Apply this diff:
renameTab={async (oldName, newName) => { if (currentSpaceId) { - const resolvedConfig = await config; return renameSpaceTab( currentSpaceId, oldName, newName, - resolvedConfig, + config, isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, ); } return undefined; }}
🧹 Nitpick comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
185-193: Complex conditional logic for config resolution.The config logic has nested ternaries that determine whether to return a loaded tab config, a default config, or undefined (to trigger Suspense). While logically correct, this could benefit from clarifying comments explaining each branch condition.
Consider adding inline comments to clarify the branches:
+ // Config resolution: + // 1. If tab exists in store, use it with computed isEditable + // 2. If viewing unregistered space (no currentSpaceId), use default config + // 3. Otherwise, return undefined to trigger Suspense during load const config = currentTabName ? ( currentConfig?.tabs?.[currentTabName] ? {
331-352: Unnecessary dependency in saveConfig callback.Line 351 includes
config?.fidgetInstanceDatumsin the dependency array, but the callback only referencescurrentSpaceId,currentTabName, andsaveLocalSpaceTab. ThespaceConfigparameter comes from the function arguments, not the outerconfigvariable.Apply this diff to remove the unnecessary dependency:
- [currentSpaceId, currentTabName, saveLocalSpaceTab, config?.fidgetInstanceDatums] + [currentSpaceId, currentTabName, saveLocalSpaceTab]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/(spaces)/PublicSpace.tsx(11 hunks)src/app/(spaces)/SpacePage.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/(spaces)/SpacePage.tsx (1)
src/app/(spaces)/Space.tsx (1)
Space(112-360)
src/app/(spaces)/PublicSpace.tsx (6)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfigSaveDetails(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-76)
🔇 Additional comments (4)
src/app/(spaces)/SpacePage.tsx (1)
62-74: LGTM! Fragment wrapper removed.Removing the unnecessary React fragment wrapper simplifies the component structure without affecting functionality.
src/app/(spaces)/PublicSpace.tsx (3)
95-102: LGTM! Store synchronization effect.Properly synchronizes route params with store state, ensuring currentSpaceId and currentTabName reflect the latest space data.
146-169: LGTM! Data loading effects with error handling.The two effects properly handle:
- Loading editable spaces when the user signs in
- Loading space data when IDs are set
Both include appropriate error handling and correct dependencies.
467-477: LGTM! Direct SpacePage rendering.Returns SpacePage directly with the necessary props, consistent with the removal of the fragment wrapper in SpacePage.tsx.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
104-109: Remove unnecessary dependencies from useCallback; consider if getConfig wrapper is needed at all.The
getCurrentSpaceConfig()already readscurrentSpaceIdandcurrentTabNamefresh from the store at call time viaget(), making these dependencies redundant. Additionally, the callback wrapper serves no purpose since it's called immediately on line 109 rather than being passed as a memoized dependency to child components.Consider calling
getCurrentSpaceConfig()directly:- // Get the current config using the store's getter - const getConfig = useCallback(() => { - return getCurrentSpaceConfig(); - }, [getCurrentSpaceConfig, currentSpaceId, currentTabName]); - - const currentConfig = getConfig(); + // Get the current config using the store's getter + const currentConfig = getCurrentSpaceConfig();Based on past review comments.
418-430: Critical: Remove invalid await on config.Line 420 attempts to
await config, butconfigis a plain object (or undefined), not a Promise. This will causeresolvedConfigto simply be the config object itself, which works but is misleading and incorrect usage ofawait.Apply this diff:
renameTab={async (oldName, newName) => { if (currentSpaceId) { - const resolvedConfig = await config; return renameSpaceTab( currentSpaceId, oldName, newName, - resolvedConfig, + config, isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, ); } return undefined; }}Based on past review comments.
🧹 Nitpick comments (4)
src/app/(spaces)/PublicSpace.tsx (4)
14-14: Remove unused imports.The imports
debounce(line 14) andwithOptimisticUpdate(line 28) are not used anywhere in the visible code.Apply this diff:
-import { indexOf, isNil, mapValues, noop, debounce } from "lodash"; +import { indexOf, isNil, mapValues, noop } from "lodash";-import { withOptimisticUpdate } from "@/common/utils/tabUtils";Also applies to: 28-28
172-179: Remove unnecessary dependency from isEditable useMemo.The dependency
isSignedIntoFarcasteron line 179 is not used in theuseMemocomputation (lines 173-178), making it unnecessary and causing redundant recomputations.Apply this diff:
const isEditable = useMemo(() => { const result = spacePageData.isEditable( currentUserFid || undefined, wallets.map((w) => ({ address: w.address as Address })) ); return result; - }, [spacePageData, currentUserFid, wallets, isSignedIntoFarcaster]); + }, [spacePageData, currentUserFid, wallets]);
185-193: Consider extracting config logic to useMemo for clarity.The config determination logic is complex with nested ternaries. While correct, wrapping it in a
useMemowould improve readability and make the dependencies explicit.- const config = currentTabName ? ( - currentConfig?.tabs?.[currentTabName] ? { - ...currentConfig.tabs[currentTabName], - isEditable, - } : !currentSpaceId ? { - ...spacePageData.config, - isEditable, - } : undefined - ) : undefined; + const config = useMemo(() => { + if (!currentTabName) return undefined; + + if (currentConfig?.tabs?.[currentTabName]) { + return { + ...currentConfig.tabs[currentTabName], + isEditable, + }; + } + + if (!currentSpaceId) { + return { + ...spacePageData.config, + isEditable, + }; + } + + return undefined; + }, [currentTabName, currentConfig, currentSpaceId, spacePageData.config, isEditable]);
331-352: Remove incorrect dependency from saveConfig callback.Line 351 includes
config?.fidgetInstanceDatumsin the dependency array, but this value is never used in thesaveConfigfunction. The function only uses the passed-inspaceConfigparameter. This unnecessary dependency causes redundant recreations of the callback.Apply this diff:
const saveConfig = useCallback( async (spaceConfig: SpaceConfigSaveDetails) => { if (isNil(currentSpaceId) || isNil(currentTabName)) { throw new Error("Cannot save config until space and tab are initialized"); } const saveableConfig = { ...spaceConfig, fidgetInstanceDatums: spaceConfig.fidgetInstanceDatums ? mapValues(spaceConfig.fidgetInstanceDatums, (datum) => ({ ...datum, config: { settings: datum.config.settings, editable: datum.config.editable, }, })) : undefined, isPrivate: false, }; return saveLocalSpaceTab(currentSpaceId, currentTabName, saveableConfig); }, - [currentSpaceId, currentTabName, saveLocalSpaceTab, config?.fidgetInstanceDatums] + [currentSpaceId, currentTabName, saveLocalSpaceTab] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/(spaces)/PublicSpace.tsx(11 hunks)src/app/home/[tabname]/page.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (5)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfigSaveDetails(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-76)
🔇 Additional comments (9)
src/app/home/[tabname]/page.tsx (2)
83-83: LGTM: Default tab alignment.The
defaultTabprop correctly aligns with the static tab configuration and provides a consistent fallback for the home page.
88-88: LGTM: No-op rename callback for static tabs.The no-op
renameTabimplementation is appropriate since home page tabs are static and not renameable.src/app/(spaces)/PublicSpace.tsx (7)
96-102: LGTM: Effect synchronizes store state with space data.The effect correctly initializes
currentSpaceIdandcurrentTabNamefrom the space page data with proper dependencies.
147-153: LGTM: Effect loads editable spaces when user authenticates.The effect correctly triggers space loading when the user signs in, with proper error handling.
156-169: LGTM: Effect loads space data when IDs are set.The effect correctly loads space tab order and tab data when both
currentSpaceIdandcurrentTabNameare available, with proper error handling.
196-329: LGTM: Space registration effect with proper orchestration.The effect correctly handles space registration for different space types (token, proposal, profile, channel) with proper early returns, error handling, and post-registration data loading.
354-358: LGTM: commitConfig callback.The callback correctly commits the space tab with proper dependencies.
360-377: LGTM: resetConfig callback.The callback correctly resets the config to either the default or remote config with proper dependencies.
467-477: LGTM: SpacePage rendering.The component correctly returns
SpacePagewith all required props including the computedconfig.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
104-109: Remove useCallback wrapper or fix dependencies (regression from previous review).This was previously flagged and marked as addressed, but the issue remains. The callback includes
currentSpaceIdandcurrentTabNamein dependencies, yetgetCurrentSpaceConfig()reads these values fresh from the store viaget(), making them redundant. More critically, the callback is called immediately on line 109 rather than being passed as a dependency, so the memoization serves no purpose.Either remove the wrapper entirely:
- const getConfig = useCallback(() => { - return getCurrentSpaceConfig(); - }, [getCurrentSpaceConfig, currentSpaceId, currentTabName]); - - const currentConfig = getConfig(); + const currentConfig = getCurrentSpaceConfig();Or if memoization is needed for other reasons, reduce the deps:
- }, [getCurrentSpaceConfig, currentSpaceId, currentTabName]); + }, [getCurrentSpaceConfig]);
416-428: Critical: Remove invalid await on config (still unfixed from previous review).Line 418 attempts to
await config, butconfig(defined at lines 185-191) is a plain object or undefined, not a Promise. Awaiting it is incorrect and misleading, though it won't cause a runtime error. This critical issue was previously flagged but remains unfixed.Apply this diff:
renameTab={async (oldName, newName) => { if (currentSpaceId) { - const resolvedConfig = await config; return renameSpaceTab( currentSpaceId, oldName, newName, - resolvedConfig, + config, isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, ); } return undefined; }}
🧹 Nitpick comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
329-350: Remove unnecessary dependency from saveConfig.The dependency
config?.fidgetInstanceDatumson line 349 is not referenced in the callback body. The callback usesspaceConfig.fidgetInstanceDatumsfrom the parameter instead. This unnecessary dependency causes the callback to be recreated wheneverconfig.fidgetInstanceDatumschanges, which is inefficient.Apply this diff:
- }, [currentSpaceId, currentTabName, saveLocalSpaceTab, config?.fidgetInstanceDatums]); + }, [currentSpaceId, currentTabName, saveLocalSpaceTab]);
181-191: Consider extracting config logic for clarity.The nested ternary on lines 185-191 determines which config to use based on multiple conditions. While functionally correct, extracting this into a helper function or using if-else statements would improve readability.
Example refactor:
const config = useMemo(() => { // Tab loaded in store if (currentTabName && currentConfig?.tabs?.[currentTabName]) { return { ...currentConfig.tabs[currentTabName], isEditable, }; } // Viewing someone else's space (no currentSpaceId) if (!currentSpaceId) { return { ...spacePageData.config, isEditable, }; } // Loading state - trigger Suspense return undefined; }, [currentTabName, currentConfig, currentSpaceId, spacePageData.config, isEditable]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/(spaces)/PublicSpace.tsx(11 hunks)src/app/home/[tabname]/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/home/[tabname]/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (5)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfigSaveDetails(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-76)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
416-428: Critical: Remove invalid await on config.Line 418 attempts to
await config, butconfigis a plain object (defined at lines 185-191), not a Promise. This will causeresolvedConfigto simply be the config object itself, which works but is misleading and incorrect usage ofawait.Note: This issue was flagged in a previous review and marked as "Addressed in commit 849154b", but the fix is not present in the current code.
Apply this diff:
renameTab={async (oldName, newName) => { if (currentSpaceId) { - const resolvedConfig = await config; return renameSpaceTab( currentSpaceId, oldName, newName, - resolvedConfig, + config, isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, ); } return undefined; }}
104-109: Remove unnecessary useCallback wrapper.The
getConfigcallback is invoked immediately on line 109 rather than being passed as a memoized dependency, making the wrapper pointless. Additionally,getCurrentSpaceConfig()already readscurrentSpaceIdandcurrentTabNamefrom the store at call-time, so including them in the dependency array forces unnecessary recalculations.Apply this diff to remove the wrapper:
- // Get the current config using the store's getter - const getConfig = useCallback(() => { - return getCurrentSpaceConfig(); - }, [getCurrentSpaceConfig, currentSpaceId, currentTabName]); - - const currentConfig = getConfig(); + // Get the current config using the store's getter + const currentConfig = getCurrentSpaceConfig();
🧹 Nitpick comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
185-191: Memoize config to prevent unnecessary re-renders.The
configobject is created on every render because it's not memoized. This creates a new object reference each time, potentially causing unnecessary re-renders of child components that receiveconfigas a prop.Apply this diff to memoize the config:
+ const config = useMemo(() => { + const baseConfig = currentTabName && currentConfig?.tabs?.[currentTabName] + ? currentConfig.tabs[currentTabName] + : spacePageData.config; + + return { + ...baseConfig, + isEditable, + }; + }, [currentTabName, currentConfig, spacePageData.config, isEditable]); - const config = currentTabName && currentConfig?.tabs?.[currentTabName] ? { - ...currentConfig.tabs[currentTabName], - isEditable, - } : { - ...spacePageData.config, - isEditable, - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(spaces)/PublicSpace.tsx(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (6)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfigSaveDetails(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-76)
🔇 Additional comments (3)
src/app/(spaces)/PublicSpace.tsx (3)
95-102: LGTM!The effect correctly synchronizes
spacePageDataandprovidedTabNamewith the store-backed state.
146-153: LGTM!The effect correctly loads editable spaces when the user signs in, with proper error handling.
155-169: LGTM!The effect correctly loads space data when IDs are set, with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/common/components/organisms/TabBar.tsx (1)
174-182: Fix URL encoding in switchTabTo call.Line 178 calls
switchTabTo(encodeURIComponent(uniqueName), false), butswitchTabToexpects a tab name, not a URL-encoded string. ThegetSpacePageUrlfunction insideswitchTabToshould handle URL encoding if needed. Passing an already-encoded name could result in double-encoding or incorrect tab state.Apply this diff:
// Fire off rename (optimistic update happens immediately and handles order update) const renamePromise = renameTab(tabName, uniqueName); // Update current tab name in store and URL - switchTabTo(encodeURIComponent(uniqueName), false); + switchTabTo(uniqueName, false); // Wait for rename to complete, then commit await renamePromise;
🧹 Nitpick comments (3)
src/app/home/[tabname]/page.tsx (1)
69-73: Remove unused function.The
switchTabTofunction is defined but never called in this file. Since the TabBar component now handles tab switching internally via its ownswitchTabToimplementation (usinghistory.replaceState), this local function appears to be dead code.Apply this diff to remove the unused function:
- function switchTabTo(newTabName: string) { - // Update the store immediately for better responsiveness - setCurrentTabName(newTabName); - router.push(`/home/${newTabName}`); - } - const tabBar = (src/common/components/organisms/TabBar.tsx (2)
67-71: Remove unused parameter.The
shouldSaveparameter is declared but never used in the function body. Either implement the intended behavior or remove the parameter.Apply this diff to remove the unused parameter:
- function switchTabTo(tabName: string, shouldSave: boolean = true) { + function switchTabTo(tabName: string) { setCurrentTabName(tabName); const url = getSpacePageUrl(tabName); window.history.replaceState(null, '', url);Then update all call sites to remove the second argument:
- switchTabTo(cleanTabName, true); + switchTabTo(cleanTabName);- switchTabTo(cleanTabName, false); + switchTabTo(cleanTabName);- switchTabTo(finalTabName, false); + switchTabTo(finalTabName);- switchTabTo(nextTab, true); + switchTabTo(nextTab);- switchTabTo(encodeURIComponent(uniqueName), false); + switchTabTo(encodeURIComponent(uniqueName));- switchTabTo(tabName, true); + switchTabTo(tabName);
82-116: Consider implementing rollback for failed tab creation.The TODO comment at line 112 indicates that rollback logic for failed tab creation is not yet implemented. If tab creation fails after the optimistic UI update, the user will see the new tab in the UI but it won't exist on the server.
Do you want me to generate a rollback implementation that removes the tab from the UI if creation fails?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/home/[tabname]/page.tsx(4 hunks)src/common/components/organisms/TabBar.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/common/components/organisms/TabBar.tsx (3)
src/common/data/stores/app/currentSpace/index.ts (1)
setCurrentTabName(51-55)src/common/data/stores/app/homebase/homebaseTabsStore.ts (3)
createTab(229-309)deleteTab(310-347)renameTab(348-449)src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/app/home/[tabname]/page.tsx (3)
src/common/data/stores/app/currentSpace/index.ts (1)
setCurrentTabName(51-55)src/app/(spaces)/SpacePage.tsx (2)
SpacePageArgs(5-15)SpacePage(17-76)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/components/organisms/TabBar.tsx (1)
266-284: Don’t allow the default tab to be draggableDefault tab is rendered in the list but
valuesexcludes it. Withdraggable={inEditMode}, it can become a Reorder.Item and desync the list.- draggable={inEditMode} + draggable={inEditMode && isEditableTab(tabName, defaultTab)}Optionally ensure the Tab component does not render a Reorder.Item for non-draggable items.
♻️ Duplicate comments (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
409-411: LGTM: active tab follows renameUpdating
draft.currentSpace.currentTabNameduring optimistic rename fixes the prior selection loss.
🧹 Nitpick comments (7)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (3)
25-25: Replace Moment with native Date to reduce bundle sizeUse Date.parse and new Date().toISOString() instead of moment. Same behavior, fewer deps.
Based on learnings
-import moment from "moment"; +// import moment from "moment"; // not needed; use native Date- if (existingTab?.config?.timestamp && - remoteConfig.timestamp && - moment(existingTab.config.timestamp).isAfter(moment(remoteConfig.timestamp))) { + if ( + existingTab?.config?.timestamp && + remoteConfig.timestamp && + Date.parse(existingTab.config.timestamp) > Date.parse(remoteConfig.timestamp) + ) {- // Update timestamp to mark this as a local change - localCopy.timestamp = moment().toISOString(); + // Update timestamp to mark this as a local change + localCopy.timestamp = new Date().toISOString();Also applies to: 493-496, 571-572
470-477: Avoid stale tab config by cache-busting the GETAdd a
?t=param like you do for tab ordering to defeat CDN/browser caches.- const { data } = await axios.get<Blob>(publicUrl, { + const t = Date.now(); + const urlWithParam = `${publicUrl}?t=${t}`; + const { data } = await axios.get<Blob>(urlWithParam, { responseType: "blob", headers: { "Cache-Control": "no-cache", Pragma: "no-cache", Expires: "0", }, });
334-345: Optional: keep currentTabName valid when deleting current tabStore method doesn’t adjust
currentTabNameif the active tab is deleted (UI handles it in TabBar). Consider a defensive update here to avoid stale state when deleteTab is called from other flows.// Commit the updated tab order to the database await get().homebase.commitTabOrderingToDatabase(); + + // Optional guard: if the deleted tab was active, switch to a safe fallback + set((draft) => { + if (draft.currentSpace.currentTabName === tabName) { + const next = draft.homebase.tabOrdering.local[0] ?? HOMEBASE_DEFAULT_TAB; + draft.currentSpace.currentTabName = next; + } + }, "deleteHomebaseTab:ensureCurrentTab");src/common/components/organisms/TabBar.tsx (2)
68-72: Remove unusedshouldSaveparameter from switchTabToParam isn’t used. Simplify signature and calls.
- function switchTabTo(tabName: string, shouldSave: boolean = true) { + const switchTabTo = React.useCallback((tabName: string) => { setCurrentTabName(tabName); const url = getSpacePageUrl(tabName); window.history.replaceState(null, '', url); - } + }, [getSpacePageUrl, setCurrentTabName]);- switchTabTo(cleanTabName, true); + switchTabTo(cleanTabName);- switchTabTo(cleanTabName, false); + switchTabTo(cleanTabName);- if (finalTabName !== cleanTabName) { - switchTabTo(finalTabName, false); - } + if (finalTabName !== cleanTabName) { + switchTabTo(finalTabName); + }- if (shouldSwitch && nextTab) { - switchTabTo(nextTab, true); - } + if (shouldSwitch && nextTab) { + switchTabTo(nextTab); + }- switchTabTo(tabName, true); + switchTabTo(tabName);Also applies to: 91-93, 96-97, 104-107, 139-142, 226-227
259-266: Verify Reorder values vs rendered items
values={tabList}while rendering[HOMEBASE_DEFAULT_TAB, ...tabList]for homebase. If Tab renders a Reorder.Item for the default tab, this mismatch can break ordering. Either ensure the default tab is not a Reorder.Item (non-draggable) or include it in values and filter it before persisting.If needed:
- values={tabList} + values={inHomebase ? [HOMEBASE_DEFAULT_TAB, ...tabList] : tabList} onReorder={debouncedReorder}And filter before commit:
- debounce((newOrder) => { - updateTabOrder(newOrder); + debounce((newOrder) => { + const filtered = newOrder.filter((n: string) => isEditableTab(n, defaultTab)); + updateTabOrder(filtered); commitTabOrder(); }, 300)Also applies to: 266-268, 238-243
src/app/(spaces)/homebase/PrivateSpace.tsx (1)
176-205: Optional: allow Suspense instead of default config fallbackReturning a default config prevents SpacePage’s Suspense from showing a loading state and can cause a brief “flash” of default layout. Consider returning
undefineduntil the active config is available.docs/TAB_MANAGEMENT_SYSTEM.md (1)
40-44: Specify language for fenced code block (markdownlint MD040)Add a language (e.g.,
text) to the ASCII diagram block.-``` +```text URL Params → Store Sync → Tab Loading → Config Resolution → UI Render ↓ ↓ ↓ ↓ ↓ tabName → currentTabName → loadTab → getConfig → SpacePage</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a6cb4396a94c1070c50d475b528ab01844fea74e and 8278d61ac5254d4ad44eeea35d24a617bb6f2223. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `docs/TAB_MANAGEMENT_SYSTEM.md` (1 hunks) * `src/app/(spaces)/homebase/PrivateSpace.tsx` (9 hunks) * `src/common/components/organisms/TabBar.tsx` (8 hunks) * `src/common/data/stores/app/homebase/homebaseTabsStore.ts` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)</summary><blockquote> <details> <summary>src/common/utils/tabUtils.ts (3)</summary> * `validateTabName` (11-16) * `isDuplicateTabName` (25-34) * `withOptimisticUpdate` (46-74) </details> <details> <summary>src/pages/api/space/homebase/tabs/index.ts (1)</summary> * `UnsignedManageHomebaseTabsRequest` (24-29) </details> <details> <summary>src/common/lib/signedFiles.ts (1)</summary> * `signSignable` (54-62) </details> <details> <summary>src/app/(spaces)/Space.tsx (1)</summary> * `SpaceConfig` (37-49) </details> </blockquote></details> <details> <summary>src/common/components/organisms/TabBar.tsx (3)</summary><blockquote> <details> <summary>src/common/data/stores/app/currentSpace/index.ts (1)</summary> * `setCurrentTabName` (51-55) </details> <details> <summary>src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)</summary> * `createTab` (232-312) * `deleteTab` (313-350) * `renameTab` (351-456) * `HOMEBASE_DEFAULT_TAB` (35-35) </details> <details> <summary>src/common/utils/tabUtils.ts (1)</summary> * `validateTabName` (11-16) </details> </blockquote></details> <details> <summary>src/app/(spaces)/homebase/PrivateSpace.tsx (4)</summary><blockquote> <details> <summary>src/common/data/stores/app/currentSpace/index.ts (3)</summary> * `setCurrentSpaceId` (43-47) * `HOMEBASE_ID` (25-25) * `setCurrentTabName` (51-55) </details> <details> <summary>src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)</summary> * `HOMEBASE_DEFAULT_TAB` (35-35) </details> <details> <summary>src/app/(spaces)/SpacePage.tsx (2)</summary> * `SpacePageArgs` (5-15) * `SpacePage` (17-76) </details> <details> <summary>src/constants/initialSpaceConfig.ts (1)</summary> * `INITIAL_SPACE_CONFIG_EMPTY` (8-20) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/TAB_MANAGEMENT_SYSTEM.md</summary> [grammar] ~9-~9: There might be a mistake here. Context: ...able of Contents - [System Architecture](#system-architecture) - [Store Comparison](#store-comparison) - [... (QB_NEW_EN) --- [grammar] ~10-~10: There might be a mistake here. Context: ...system-architecture) - [Store Comparison](#store-comparison) - [State Management Patterns](#state-manage... (QB_NEW_EN) --- [grammar] ~11-~11: There might be a mistake here. Context: ...comparison) - [State Management Patterns](#state-management-patterns) - [Tab Operations](#tab-operations) - [Opti... (QB_NEW_EN) --- [grammar] ~12-~12: There might be a mistake here. Context: ...e-management-patterns) - [Tab Operations](#tab-operations) - [Optimistic Updates](#optimistic-updates)... (QB_NEW_EN) --- [grammar] ~13-~13: There might be a mistake here. Context: ...](#tab-operations) - [Optimistic Updates](#optimistic-updates) - [Component Integration](#component-integr... (QB_NEW_EN) --- [grammar] ~14-~14: There might be a mistake here. Context: ...mistic-updates) - [Component Integration](#component-integration) - [Performance Considerations](#performance... (QB_NEW_EN) --- [grammar] ~15-~15: There might be a mistake here. Context: ...tegration) - [Performance Considerations](#performance-considerations) - [Security Model](#security-model) - [Migr... (QB_NEW_EN) --- [grammar] ~16-~16: There might be a mistake here. Context: ...rmance-considerations) - [Security Model](#security-model) - [Migration Guide](#migration-guide) ## S... (QB_NEW_EN) --- [grammar] ~34-~34: There might be a mistake here. Context: ...tils.ts` - Shared utilities and patterns - Validation functions - Optimistic upd... (QB_NEW_EN) --- [grammar] ~35-~35: There might be a mistake here. Context: ...s and patterns - Validation functions - Optimistic update helpers ### State Flo... (QB_NEW_EN) --- [grammar] ~70-~70: There might be a mistake here. Context: ...rder } } ``` **Key Characteristics**: - ✅ Encrypted storage (Supabase Private) -... (QB_NEW_EN) --- [grammar] ~71-~71: There might be a mistake here. Context: ...- ✅ Encrypted storage (Supabase Private) - ✅ Single user only - ✅ Simple local/remo... (QB_NEW_EN) --- [grammar] ~72-~72: There might be a mistake here. Context: ... (Supabase Private) - ✅ Single user only - ✅ Simple local/remote state pattern - ✅ ... (QB_NEW_EN) --- [grammar] ~73-~73: There might be a mistake here. Context: ...ly - ✅ Simple local/remote state pattern - ✅ Direct API endpoints - ✅ Timestamp-bas... (QB_NEW_EN) --- [grammar] ~74-~74: There might be a mistake here. Context: ...e state pattern - ✅ Direct API endpoints - ✅ Timestamp-based conflict resolution #... (QB_NEW_EN) --- [grammar] ~101-~101: There might be a mistake here. Context: ...r fields } ``` **Key Characteristics**: - ❌ Unencrypted storage (Supabase Public) ... (QB_NEW_EN) --- [grammar] ~102-~102: There might be a mistake here. Context: ... ❌ Unencrypted storage (Supabase Public) - ✅ Multi-user support - ✅ Complex state m... (QB_NEW_EN) --- [grammar] ~103-~103: There might be a mistake here. Context: ...(Supabase Public) - ✅ Multi-user support - ✅ Complex state management - ✅ Registry ... (QB_NEW_EN) --- [grammar] ~104-~104: There might be a mistake here. Context: ...ser support - ✅ Complex state management - ✅ Registry API system - ✅ Advanced confl... (QB_NEW_EN) --- [grammar] ~105-~105: There might be a mistake here. Context: ...state management - ✅ Registry API system - ✅ Advanced conflict resolution ## State... (QB_NEW_EN) --- [grammar] ~448-~448: There might be a mistake here. Context: ... All data encrypted with user's root key - **Storage**: Supabase Private Storage (enc... (QB_NEW_EN) --- [grammar] ~449-~449: There might be a mistake here. Context: ...base Private Storage (encrypted at rest) - **Access**: Single user only - **Authentic... (QB_NEW_EN) --- [grammar] ~450-~450: There might be a mistake here. Context: ... at rest) - **Access**: Single user only - **Authentication**: Cryptographic signatur... (QB_NEW_EN) --- [grammar] ~455-~455: There might be a mistake here. Context: ...es - **Encryption**: None (public data) - **Storage**: Supabase Public Storage - **A... (QB_NEW_EN) --- [grammar] ~456-~456: There might be a mistake here. Context: ...) - **Storage**: Supabase Public Storage - **Access**: Multi-user, public - **Authent... (QB_NEW_EN) --- [grammar] ~457-~457: There might be a mistake here. Context: ...Storage - **Access**: Multi-user, public - **Authentication**: API-based permissions ... (QB_NEW_EN) --- [grammar] ~563-~563: There might be a mistake here. Context: ...window.history.replaceState` instead of `router.push` 4. **Flash of Default Config**: Return `undef... (QB_NEW_EN) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/TAB_MANAGEMENT_SYSTEM.md</summary> 40-40: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)</summary><blockquote> `153-153`: **Backend route verification complete—no issues found.** The route file `src/pages/api/space/homebase/tabOrder.ts` exists and is properly configured. It exports a default handler with POST support via `requestHandler({ post: updateTabOrder })`, which will correctly handle the frontend call to `/api/space/homebase/tabOrder`. No 404 risk. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| interface TabBarProps { | ||
| inHome?: boolean; | ||
| inHomebase: boolean; | ||
| inEditMode: boolean; | ||
| currentTab: string; | ||
| tabList: string[]; | ||
| defaultTab: string; | ||
| updateTabOrder: (newOrder: string[]) => void; | ||
| commitTabOrder: () => void; | ||
| switchTabTo: (tabName: string, shouldSave?: boolean) => void; | ||
| deleteTab: (tabName: string) => void; | ||
| createTab: (tabName: string) => Promise<{ tabName: string } | undefined>; | ||
| renameTab: (tabName: string, newName: string) => void; | ||
| renameTab: (tabName: string, newName: string) => Promise<void> | void; | ||
| commitTab: (tabName: string) => void; | ||
| getSpacePageUrl: (tabName: string) => string; | ||
| isTokenPage?: boolean; | ||
| contractAddress?: Address; | ||
| isEditable?: boolean; | ||
| } |
There was a problem hiding this comment.
Fix TabBar prop types to match usage
deleteTab is awaited; commitTab may be async. Update types.
- deleteTab: (tabName: string) => void;
+ deleteTab: (tabName: string) => Promise<void>;
createTab: (tabName: string) => Promise<{ tabName: string } | undefined>;
- renameTab: (tabName: string, newName: string) => Promise<void> | void;
- commitTab: (tabName: string) => void;
+ renameTab: (tabName: string, newName: string) => Promise<void> | void;
+ commitTab: (tabName: string) => Promise<void> | void;📝 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.
| interface TabBarProps { | |
| inHome?: boolean; | |
| inHomebase: boolean; | |
| inEditMode: boolean; | |
| currentTab: string; | |
| tabList: string[]; | |
| defaultTab: string; | |
| updateTabOrder: (newOrder: string[]) => void; | |
| commitTabOrder: () => void; | |
| switchTabTo: (tabName: string, shouldSave?: boolean) => void; | |
| deleteTab: (tabName: string) => void; | |
| createTab: (tabName: string) => Promise<{ tabName: string } | undefined>; | |
| renameTab: (tabName: string, newName: string) => void; | |
| renameTab: (tabName: string, newName: string) => Promise<void> | void; | |
| commitTab: (tabName: string) => void; | |
| getSpacePageUrl: (tabName: string) => string; | |
| isTokenPage?: boolean; | |
| contractAddress?: Address; | |
| isEditable?: boolean; | |
| } | |
| interface TabBarProps { | |
| inHomebase: boolean; | |
| inEditMode: boolean; | |
| currentTab: string; | |
| tabList: string[]; | |
| defaultTab: string; | |
| updateTabOrder: (newOrder: string[]) => void; | |
| commitTabOrder: () => void; | |
| deleteTab: (tabName: string) => Promise<void>; | |
| createTab: (tabName: string) => Promise<{ tabName: string } | undefined>; | |
| renameTab: (tabName: string, newName: string) => Promise<void> | void; | |
| commitTab: (tabName: string) => Promise<void> | void; | |
| getSpacePageUrl: (tabName: string) => string; | |
| isTokenPage?: boolean; | |
| contractAddress?: Address; | |
| isEditable?: boolean; | |
| } |
🤖 Prompt for AI Agents
In src/common/components/organisms/TabBar.tsx around lines 19 to 35, the
TabBarProps types don't match usage: deleteTab is awaited and commitTab may be
async. Change deleteTab to return a Promise (e.g., Promise<void>) and make
commitTab async by returning a Promise (e.g., Promise<void>) as well (or union
Promise<void> | void if still used synchronously elsewhere) so the prop
signatures reflect their actual asynchronous usage.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
338-370: Guard against deleting the default tabDefensive check in the store prevents accidental/indirect deletion even if UI guards fail.
async deleteTab(tabName) { // console.log('Deleting tab:', { tabName }); const publicKey = get().account.currentSpaceIdentityPublicKey; if (!publicKey) return; + if (tabName === HOMEBASE_DEFAULT_TAB) { + console.warn("Cannot delete the default tab."); + return; + }
♻️ Duplicate comments (1)
src/common/components/organisms/TabBar.tsx (1)
18-34: Align TabBar prop types with async usagedeleteTab and commitTab are awaited/async. Update types to avoid confusion. This mirrors a prior comment.
interface TabBarProps { inHomebase: boolean; inEditMode: boolean; currentTab: string; tabList: string[]; defaultTab: string; updateTabOrder: (newOrder: string[]) => void; commitTabOrder: () => void; - deleteTab: (tabName: string) => void; + deleteTab: (tabName: string) => Promise<void>; createTab: (tabName: string) => Promise<{ tabName: string } | undefined>; - renameTab: (tabName: string, newName: string) => Promise<void> | void; - commitTab: (tabName: string) => void; + renameTab: (tabName: string, newName: string) => Promise<void> | void; + commitTab: (tabName: string) => Promise<void> | void; getSpacePageUrl: (tabName: string) => string; isTokenPage?: boolean; contractAddress?: Address; isEditable?: boolean; }
🧹 Nitpick comments (8)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
148-176: Pin default tab at commit-time to prevent driftUsers can reorder the default away from index 0; this persists to remote. Re-pin before commit so the invariant holds consistently.
- const localCopy = cloneDeep( - get().homebase.tabOrdering.local.filter((name, i, arr) => - arr.indexOf(name) === i, - ), - ); + const deduped = get().homebase.tabOrdering.local.filter((name, i, arr) => arr.indexOf(name) === i); + let localCopy = cloneDeep(deduped); + if (!localCopy.includes(HOMEBASE_DEFAULT_TAB)) { + localCopy.unshift(HOMEBASE_DEFAULT_TAB); + } else if (localCopy[0] !== HOMEBASE_DEFAULT_TAB) { + localCopy = [HOMEBASE_DEFAULT_TAB, ...localCopy.filter((n) => n !== HOMEBASE_DEFAULT_TAB)]; + }
595-597: Prefer native Date for ISO timestamps (drop moment dependency here)Simplify and avoid extra dependency for trivial ISO stamps.
- localCopy.timestamp = moment().toISOString(); + localCopy.timestamp = new Date().toISOString();src/common/data/stores/app/homebase/homebaseStore.ts (1)
73-79: Preserve remote snapshot when local winsKeep remoteHomebaseConfig updated even if you return the local config, so reset operations have the latest remote.
- if (currentHomebase && - currentHomebase.timestamp && - spaceConfig && - (!spaceConfig.timestamp || moment(currentHomebase.timestamp).isAfter(moment(spaceConfig.timestamp)))) { - return cloneDeep(currentHomebase); - } + if ( + currentHomebase && + currentHomebase.timestamp && + spaceConfig && + (!spaceConfig.timestamp || + moment(currentHomebase.timestamp).isAfter(moment(spaceConfig.timestamp))) + ) { + set((draft) => { + draft.homebase.remoteHomebaseConfig = cloneDeep(spaceConfig); + }, "loadHomebase-remote-only"); + return cloneDeep(currentHomebase); + }src/app/(spaces)/homebase/PrivateSpace.tsx (2)
143-149: Type the save handler parameterUse SpaceConfigSaveDetails for better type-safety.
- const saveConfigHandler = async (configToSave) => { + const saveConfigHandler = async (configToSave: SpaceConfigSaveDetails) => {Add to imports:
-import SpacePage, { SpacePageArgs } from "@/app/(spaces)/Space"; +import SpacePage, { SpacePageArgs, SpaceConfigSaveDetails } from "@/app/(spaces)/Space";
102-111: Skip redundant load for default tabloadHomebaseTab no-ops for the default; just load the feed config.
- if (!isNil(currentTabName)) { - if (currentTabName === HOMEBASE_DEFAULT_TAB && !homebaseConfig) { - loadFeedConfig().then(() => loadTab(currentTabName)); - } else { - loadTab(currentTabName); - } - } + if (!isNil(currentTabName)) { + if (currentTabName === HOMEBASE_DEFAULT_TAB) { + if (!homebaseConfig) void loadFeedConfig(); + } else { + loadTab(currentTabName); + } + }src/common/components/organisms/TabBar.tsx (3)
236-242: Keep default tab pinned at index 0 on reorderPrevents “Feed” drifting via drag-and-drop.
- const debouncedReorder = React.useCallback( - debounce((newOrder) => { - updateTabOrder(newOrder); - commitTabOrder(); - }, 300), - [updateTabOrder, commitTabOrder] - ); + const debouncedReorder = React.useCallback( + debounce((newOrder: string[]) => { + const normalized = [defaultTab, ...newOrder.filter((n) => n !== defaultTab)]; + updateTabOrder(normalized); + commitTabOrder(); + }, 300), + [updateTabOrder, commitTabOrder, defaultTab] + );
266-281: Disable dragging for the default tabUI affordance to match the pinned-first invariant.
- draggable={inEditMode} + draggable={inEditMode && isEditableTab(tabName, defaultTab)}
97-114: Rollback active tab on create failuresIf create fails after optimistic switch, revert to previous tab to avoid a stuck URL/selection.
- } catch (error) { - console.error("Error creating tab:", error); - toast.error("Failed to create tab. Please try again."); - // TODO: Rollback optimistic changes if needed - } + } catch (error) { + console.error("Error creating tab:", error); + toast.error("Failed to create tab. Please try again."); + // Roll back optimistic switch + switchTabTo(currentTab); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(spaces)/homebase/PrivateSpace.tsx(8 hunks)src/common/components/organisms/TabBar.tsx(7 hunks)src/common/data/stores/app/homebase/homebaseStore.ts(1 hunks)src/common/data/stores/app/homebase/homebaseTabsStore.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)
src/common/utils/tabUtils.ts (3)
validateTabName(11-16)isDuplicateTabName(25-34)withOptimisticUpdate(46-74)src/pages/api/space/homebase/tabs/index.ts (1)
UnsignedManageHomebaseTabsRequest(24-29)src/common/lib/signedFiles.ts (1)
signSignable(54-62)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
src/common/components/organisms/TabBar.tsx (3)
src/common/data/stores/app/currentSpace/index.ts (1)
setCurrentTabName(51-55)src/common/data/stores/app/homebase/homebaseTabsStore.ts (3)
createTab(257-337)deleteTab(338-375)renameTab(376-481)src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/app/(spaces)/homebase/PrivateSpace.tsx (3)
src/common/data/stores/app/currentSpace/index.ts (3)
setCurrentSpaceId(43-47)HOMEBASE_ID(25-25)setCurrentTabName(51-55)src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
loadTabNames(176-256)HOMEBASE_DEFAULT_TAB(35-35)src/app/(spaces)/SpacePage.tsx (2)
SpacePageArgs(5-15)SpacePage(17-76)
| const sanitizedNewName = newName.trim(); | ||
| if (!sanitizedNewName || sanitizedNewName === tabName) { | ||
| return; | ||
| } | ||
|
|
||
| // Validate tab name using shared utility | ||
| const validationError = validateTabName(sanitizedNewName); | ||
| if (validationError) { | ||
| const error = new Error(validationError); | ||
| (error as any).status = 400; | ||
| throw error; | ||
| } | ||
|
|
||
| const req: UnsignedManageHomebaseTabsRequest = { | ||
| publicKey, | ||
| type: "rename", | ||
| tabName, | ||
| newName, | ||
| }; | ||
| const signedReq = await signSignable( | ||
| req, | ||
| get().account.getCurrentIdentity()!.rootKeys.privateKey, | ||
| // Check for duplicates - should not happen since UI ensures uniqueness | ||
| // but log a warning just in case | ||
| const existingTabs = get().homebase.tabOrdering.local; | ||
| if (isDuplicateTabName(sanitizedNewName, existingTabs, tabName)) { | ||
| console.warn(`Tab name "${sanitizedNewName}" already exists - UI should have ensured uniqueness`); | ||
| return; // Exit gracefully instead of throwing | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Disallow renaming the default tab
Renaming the canonical default breaks assumptions across ordering and routing. Block it early.
const sanitizedNewName = newName.trim();
if (!sanitizedNewName || sanitizedNewName === tabName) {
return;
}
+ if (tabName === HOMEBASE_DEFAULT_TAB) {
+ console.warn("Cannot rename the default tab.");
+ return;
+ }📝 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 sanitizedNewName = newName.trim(); | |
| if (!sanitizedNewName || sanitizedNewName === tabName) { | |
| return; | |
| } | |
| // Validate tab name using shared utility | |
| const validationError = validateTabName(sanitizedNewName); | |
| if (validationError) { | |
| const error = new Error(validationError); | |
| (error as any).status = 400; | |
| throw error; | |
| } | |
| const req: UnsignedManageHomebaseTabsRequest = { | |
| publicKey, | |
| type: "rename", | |
| tabName, | |
| newName, | |
| }; | |
| const signedReq = await signSignable( | |
| req, | |
| get().account.getCurrentIdentity()!.rootKeys.privateKey, | |
| // Check for duplicates - should not happen since UI ensures uniqueness | |
| // but log a warning just in case | |
| const existingTabs = get().homebase.tabOrdering.local; | |
| if (isDuplicateTabName(sanitizedNewName, existingTabs, tabName)) { | |
| console.warn(`Tab name "${sanitizedNewName}" already exists - UI should have ensured uniqueness`); | |
| return; // Exit gracefully instead of throwing | |
| } | |
| const sanitizedNewName = newName.trim(); | |
| if (!sanitizedNewName || sanitizedNewName === tabName) { | |
| return; | |
| } | |
| if (tabName === HOMEBASE_DEFAULT_TAB) { | |
| console.warn("Cannot rename the default tab."); | |
| return; | |
| } | |
| // Validate tab name using shared utility | |
| const validationError = validateTabName(sanitizedNewName); | |
| if (validationError) { | |
| const error = new Error(validationError); | |
| (error as any).status = 400; | |
| throw error; | |
| } | |
| // Check for duplicates - should not happen since UI ensures uniqueness | |
| // but log a warning just in case | |
| const existingTabs = get().homebase.tabOrdering.local; | |
| if (isDuplicateTabName(sanitizedNewName, existingTabs, tabName)) { | |
| console.warn(`Tab name "${sanitizedNewName}" already exists - UI should have ensured uniqueness`); | |
| return; // Exit gracefully instead of throwing | |
| } |
🤖 Prompt for AI Agents
In src/common/data/stores/app/homebase/homebaseTabsStore.ts around lines 380 to
399, add an early guard that prevents renaming the canonical default tab: detect
when the current tabName matches the app's DEFAULT_TAB constant (import it if
not present) and immediately throw a 400 error (new Error with status = 400)
with a clear message like "Renaming the default tab is not allowed" instead of
proceeding to validation or duplicate checks; ensure the check occurs before
trimming/validating the new name so the default tab cannot be changed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (3)
198-240: Keep state consistent when remote tabs are removed; also ensure default placeholder
- If a tab is removed remotely and was active, currentTabName remains pointing to a deleted tab.
- Also ensure tabs entry for the default exists when enforcing it to be first.
Apply within this set block:
currentTabNames.forEach(tabName => { if (!validTabNames.includes(tabName)) { delete draft.homebase.tabs[tabName]; } }); @@ draft.homebase.tabOrdering.local = draft.homebase.tabOrdering.local.filter( tabName => validTabNames.includes(tabName) ); + // If current selection no longer exists, switch to a safe default + if (!draft.homebase.tabOrdering.local.includes(draft.currentSpace.currentTabName)) { + draft.currentSpace.currentTabName = + draft.homebase.tabOrdering.local[0] ?? HOMEBASE_DEFAULT_TAB; + } @@ if (!draft.homebase.tabOrdering.local.includes(HOMEBASE_DEFAULT_TAB)) { draft.homebase.tabOrdering.local.unshift(HOMEBASE_DEFAULT_TAB); } else { // If default tab exists but isn't first, move it to the front const defaultTabIndex = draft.homebase.tabOrdering.local.indexOf(HOMEBASE_DEFAULT_TAB); if (defaultTabIndex > 0) { draft.homebase.tabOrdering.local.splice(defaultTabIndex, 1); draft.homebase.tabOrdering.local.unshift(HOMEBASE_DEFAULT_TAB); } } + // Ensure a placeholder exists for default so the order never references a missing tab + if (!draft.homebase.tabs[HOMEBASE_DEFAULT_TAB]) { + draft.homebase.tabs[HOMEBASE_DEFAULT_TAB] = {}; + }
359-366: Handle active-tab deletion (and consider disallowing default deletion)If you delete the active tab, selection becomes invalid. Switch to default or the first tab.
Apply within this set block:
draft.homebase.tabOrdering.local = [...draft.homebase.tabOrdering.local] .filter(name => name !== tabName); + // If we deleted the active tab, switch to a safe fallback + if (draft.currentSpace.currentTabName === tabName) { + const next = draft.homebase.tabOrdering.local[0] ?? HOMEBASE_DEFAULT_TAB; + draft.currentSpace.currentTabName = next; + }Additionally, add an early guard near the start of deleteTab to block deleting the default:
if (tabName === HOMEBASE_DEFAULT_TAB) { console.warn("Cannot delete the default tab."); return; }
262-267: Create-tab duplicate check should be case-insensitiveCurrent check is key-existence only. You can create “feed” alongside “Feed”. Align with rename’s duplicate semantics and treat it as “open existing”.
Apply:
- // Check if tab already exists - if (get().homebase.tabs[tabName]) { + // Check if tab already exists (case-insensitive) and treat as existing + const existingNames = Object.keys(get().homebase.tabs); + const canonical = existingNames.find((n) => n.toLowerCase() === tabName.toLowerCase()); + if (canonical) { // console.log('Tab already exists:', { tabName }); // If tab exists but doesn't have remote state, load it - if (!get().homebase.tabs[tabName]?.remoteConfig) { - await get().homebase.loadHomebaseTab(tabName); + if (!get().homebase.tabs[canonical]?.remoteConfig) { + await get().homebase.loadHomebaseTab(canonical); } // Add it back to the tab order if it's not already there set((draft) => { - if (!draft.homebase.tabOrdering.local.includes(tabName)) { - draft.homebase.tabOrdering.local.push(tabName); + if (!draft.homebase.tabOrdering.local.includes(canonical)) { + draft.homebase.tabOrdering.local.push(canonical); } }, "addExistingTabToOrder"); return get().homebase.commitTabOrderingToDatabase(); }Also applies to: 269-283
♻️ Duplicate comments (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
380-400: Block renaming the default tab; it breaks the default-in-first-order invariantYour ordering logic always re-inserts HOMEBASE_DEFAULT_TAB = "Feed". Renaming the default will desync ordering vs tabs.
Apply an early guard:
const sanitizedNewName = newName.trim(); if (!sanitizedNewName || sanitizedNewName === tabName) { return; } + if (tabName === HOMEBASE_DEFAULT_TAB) { + const error = new Error("Renaming the default tab is not allowed"); + (error as any).status = 400; + throw error; + }
🧹 Nitpick comments (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
25-25: Optional: avoid Moment for simple ISO timestampsNative Date suffices (smaller bundle, fewer deps). Consider replacing Moment usage here.
Based on learnings
Suggested changes:
- Replace comparison:
- new Date(existingTab.config.timestamp!).getTime() > new Date(remoteConfig.timestamp!).getTime()
- Replace timestamp generation:
- new Date().toISOString()
- Remove
import moment from "moment";Also applies to: 529-535, 614-616
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)
src/common/utils/tabUtils.ts (3)
validateTabName(11-16)isDuplicateTabName(25-34)withOptimisticUpdate(46-74)src/pages/api/space/homebase/tabs/index.ts (1)
UnsignedManageHomebaseTabsRequest(24-29)src/common/lib/signedFiles.ts (1)
signSignable(54-62)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
🔇 Additional comments (2)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
434-436: Good fix: active tab follows renameUpdating currentSpace.currentTabName ensures UI stays consistent during rename.
166-166: API route change verified as valid and safeThe endpoint
/api/space/homebase/tabOrderis properly implemented atsrc/pages/api/space/homebase/tabOrder.tswith a POST handler that validates and processes signed file uploads. The client call at line 166 in homebaseTabsStore.ts correctly invokes this endpoint, and no lingering references to old paths exist in the codebase.
| // Ensure default tab is always first in the loaded order | ||
| const orderedTabs = clone(tabOrder); | ||
| if (!orderedTabs.includes(HOMEBASE_DEFAULT_TAB)) { | ||
| orderedTabs.unshift(HOMEBASE_DEFAULT_TAB); | ||
| } else { | ||
| // If default tab exists but isn't first, move it to the front | ||
| const defaultTabIndex = orderedTabs.indexOf(HOMEBASE_DEFAULT_TAB); | ||
| if (defaultTabIndex > 0) { | ||
| orderedTabs.splice(defaultTabIndex, 1); | ||
| orderedTabs.unshift(HOMEBASE_DEFAULT_TAB); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid order referencing a non-existent default tab
You force-insert the default into ordering but don’t ensure a tabs entry exists. This can break lookups and lazy loads. Create a placeholder when adding the default.
Apply inside this set block:
const orderedTabs = clone(tabOrder);
if (!orderedTabs.includes(HOMEBASE_DEFAULT_TAB)) {
orderedTabs.unshift(HOMEBASE_DEFAULT_TAB);
} else {
// If default tab exists but isn't first, move it to the front
const defaultTabIndex = orderedTabs.indexOf(HOMEBASE_DEFAULT_TAB);
if (defaultTabIndex > 0) {
orderedTabs.splice(defaultTabIndex, 1);
orderedTabs.unshift(HOMEBASE_DEFAULT_TAB);
}
}
+ // Ensure a placeholder exists for default so order never points to a missing tab
+ if (!draft.homebase.tabs[HOMEBASE_DEFAULT_TAB]) {
+ draft.homebase.tabs[HOMEBASE_DEFAULT_TAB] = {};
+ }Optionally trigger a lazy load after the set:
// After set(...)
void get().homebase.loadHomebaseTab(HOMEBASE_DEFAULT_TAB);🤖 Prompt for AI Agents
In src/common/data/stores/app/homebase/homebaseTabsStore.ts around lines 124 to
136, when you force-insert HOMEBASE_DEFAULT_TAB into orderedTabs you must also
ensure a corresponding tabs entry exists to avoid lookup/lazy-load failures; if
the default tab is added and no tabs[HOMEBASE_DEFAULT_TAB] exists, create a
minimal placeholder tab object in the store before calling set so subsequent
lookups succeed, and optionally trigger the lazy loader after set by calling
get().homebase.loadHomebaseTab(HOMEBASE_DEFAULT_TAB).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/app/(spaces)/PublicSpace.tsx (2)
7-7: Clean up unused imports
TabBarSkeleton,debounce, andwithOptimisticUpdatearen’t used here.-import TabBarSkeleton from "@/common/components/organisms/TabBarSkeleton"; +// import removed -import { indexOf, isNil, mapValues, noop, debounce } from "lodash"; +import { indexOf, isNil, mapValues, noop } from "lodash"; -import { withOptimisticUpdate } from "@/common/utils/tabUtils"; +// import removedAlso applies to: 14-14, 28-28
417-426: Critical: Remove invalid await on config (duplicate of prior review)
configis a plain object, not a Promise.await configis misleading and unnecessary.- const resolvedConfig = await config; - return renameSpaceTab( - currentSpaceId, - oldName, - newName, - resolvedConfig, - isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, - ); + return renameSpaceTab( + currentSpaceId, + oldName, + newName, + config, + isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, + );src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
124-136: Ensure order doesn’t reference a missing default tab (placeholder entry)You force-insert HOMEBASE_DEFAULT_TAB into ordering but don’t ensure a tabs entry exists, which can break lookups/lazy loads when only ordering is fetched first.
set((draft) => { // Ensure default tab is always first in the loaded order const orderedTabs = clone(tabOrder); if (!orderedTabs.includes(HOMEBASE_DEFAULT_TAB)) { orderedTabs.unshift(HOMEBASE_DEFAULT_TAB); } else { // If default tab exists but isn't first, move it to the front const defaultTabIndex = orderedTabs.indexOf(HOMEBASE_DEFAULT_TAB); if (defaultTabIndex > 0) { orderedTabs.splice(defaultTabIndex, 1); orderedTabs.unshift(HOMEBASE_DEFAULT_TAB); } } + // Ensure a placeholder exists for default so order never points to a missing tab + if (!draft.homebase.tabs[HOMEBASE_DEFAULT_TAB]) { + draft.homebase.tabs[HOMEBASE_DEFAULT_TAB] = {}; + } draft.homebase.tabOrdering = { local: orderedTabs, remote: clone(tabOrder), }; }, `loadHomebaseTabOrdering`);
380-399: Disallow renaming the default tabRenaming the canonical default breaks assumptions across ordering/routing.
const sanitizedNewName = newName.trim(); if (!sanitizedNewName || sanitizedNewName === tabName) { return; } + if (tabName === HOMEBASE_DEFAULT_TAB) { + const err: any = new Error("Renaming the default tab is not allowed"); + err.status = 400; + throw err; + }
🧹 Nitpick comments (3)
src/app/(spaces)/PublicSpace.tsx (1)
104-109: Remove unnecessary useCallback wrapper for getConfigThe callback is called immediately and adds unrelated deps. Use the getter directly.
- const getConfig = useCallback(() => { - return getCurrentSpaceConfig(); - }, [getCurrentSpaceConfig, currentSpaceId, currentTabName]); - - const currentConfig = getConfig(); + const currentConfig = getCurrentSpaceConfig();src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
521-548: Optional: avoid Moment for simple timestamp comparisonsMoment is heavy and in maintenance. Native Date or a lighter lib (e.g., date-fns) could suffice for ISO comparisons here.
Example with Date:
const isLocalNewer = existingTab?.config?.timestamp && (!remoteConfig.timestamp || new Date(existingTab.config.timestamp).getTime() > new Date(remoteConfig.timestamp).getTime());Based on learnings.
src/app/(spaces)/homebase/PrivateSpace.tsx (1)
3-3: Remove unused imports
useTransitionanduseRouteraren’t used.-import React, { useEffect, useMemo, useCallback, lazy, useTransition } from "react"; +import React, { useEffect, useMemo, useCallback, lazy } from "react"; ... -import { useRouter } from "next/navigation"; +// import removedAlso applies to: 9-9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/(spaces)/PublicSpace.tsx(12 hunks)src/app/(spaces)/homebase/PrivateSpace.tsx(8 hunks)src/common/data/stores/app/homebase/homebaseTabsStore.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (4)
src/common/utils/tabUtils.ts (3)
validateTabName(11-16)isDuplicateTabName(25-34)withOptimisticUpdate(46-74)src/pages/api/space/homebase/tabs/index.ts (2)
UnsignedManageHomebaseTabsRequest(24-29)ManageHomebaseTabsResponse(21-22)src/common/lib/signedFiles.ts (1)
signSignable(54-62)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
src/app/(spaces)/homebase/PrivateSpace.tsx (3)
src/common/data/stores/app/currentSpace/index.ts (3)
setCurrentSpaceId(43-47)HOMEBASE_ID(25-25)setCurrentTabName(51-55)src/common/data/stores/app/homebase/homebaseTabsStore.ts (2)
loadTabNames(176-256)HOMEBASE_DEFAULT_TAB(35-35)src/app/(spaces)/SpacePage.tsx (2)
SpacePageArgs(5-15)SpacePage(17-76)
src/app/(spaces)/PublicSpace.tsx (6)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfigSaveDetails(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-76)
🪛 Biome (2.1.2)
src/app/(spaces)/PublicSpace.tsx
[error] 291-291: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/app/(spaces)/PublicSpace.tsx
Outdated
| const freshLocalSpaces = useAppStore((state) => state.space.localSpaces); | ||
| const tabOrder = freshLocalSpaces[newSpaceId]?.order || []; | ||
| for (const tabName of tabOrder) { |
There was a problem hiding this comment.
Fix Rules of Hooks violation: don’t call store hook inside async effect
useAppStore(...) is used inside an async function within useEffect. Hooks must run unconditionally at the top level. Use the store’s getState() instead.
Based on static analysis hints.
- // Load remaining tabs - get fresh reference after loading operations
- const freshLocalSpaces = useAppStore((state) => state.space.localSpaces);
- const tabOrder = freshLocalSpaces[newSpaceId]?.order || [];
+ // Load remaining tabs - read from store without using the hook
+ const tabOrder =
+ useAppStore.getState().space.localSpaces[newSpaceId]?.order || [];📝 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 freshLocalSpaces = useAppStore((state) => state.space.localSpaces); | |
| const tabOrder = freshLocalSpaces[newSpaceId]?.order || []; | |
| for (const tabName of tabOrder) { | |
| // Load remaining tabs - read from store without using the hook | |
| const tabOrder = | |
| useAppStore.getState().space.localSpaces[newSpaceId]?.order || []; | |
| for (const tabName of tabOrder) { |
🧰 Tools
🪛 Biome (2.1.2)
[error] 291-291: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/app/(spaces)/PublicSpace.tsx around lines 291 to 293, the code calls
useAppStore(...) inside an async effect which violates the Rules of Hooks;
replace that in-effect hook call with the store's synchronous getter: use the
store's getState() to read localSpaces (e.g. const freshLocalSpaces =
useAppStore.getState().space.localSpaces) before or inside the effect without
invoking the hook, then compute tabOrder from
freshLocalSpaces[newSpaceId]?.order || [] and iterate over it; ensure no Hook
calls occur conditionally or inside async functions.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
350-366: Critical: guard against undefined remote tab in resetConfig.
remoteSpaces[currentSpaceId].tabs[currentTabName]can be undefined; spreading it throws. Add a safe fallback.} else { - const remoteConfig = remoteSpaces[currentSpaceId].tabs[currentTabName]; - configToSave = { - ...remoteConfig, - }; + const remoteConfig = + remoteSpaces[currentSpaceId]?.tabs?.[currentTabName]; + configToSave = remoteConfig + ? { ...remoteConfig } + : { ...spacePageData.config, isPrivate: false }; } - saveLocalSpaceTab(currentSpaceId, currentTabName, configToSave); + saveLocalSpaceTab(currentSpaceId, currentTabName, configToSave);
♻️ Duplicate comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
26-26: Remove unused import (duplicate).
withOptimisticUpdateis not used.-import { withOptimisticUpdate } from "@/common/utils/tabUtils";
407-416: Critical: remove invalid await on config (duplicate).
configis a plain object;await configis misleading and unnecessary. Passconfigdirectly.renameTab={async (oldName, newName) => { if (currentSpaceId) { - const resolvedConfig = await config; return renameSpaceTab( currentSpaceId, oldName, newName, - resolvedConfig, + config, isTokenSpace(spacePageData) ? spacePageData.tokenData?.network as EtherScanChainName : undefined, ); } return undefined; }}
🧹 Nitpick comments (8)
src/app/(spaces)/PublicSpace.tsx (8)
102-108: Drop unnecessary useCallback wrapper for getConfig.It’s called immediately and doesn’t use
currentSpaceId/currentTabName; read from the store directly.- // Get the current config using the store's getter - const getConfig = useCallback(() => { - return getCurrentSpaceConfig(); - }, [getCurrentSpaceConfig, currentSpaceId, currentTabName]); - - const currentConfig = getConfig(); + // Read current config once per render + const currentConfig = getCurrentSpaceConfig();
340-341: Trim saveConfig deps to avoid unnecessary re-creations.
config?.fidgetInstanceDatumsisn’t used in the callback body.- [currentSpaceId, currentTabName, saveLocalSpaceTab, config?.fidgetInstanceDatums] + [currentSpaceId, currentTabName, saveLocalSpaceTab]
169-177: Remove unused dep in isEditable memo.
isSignedIntoFarcasteris not referenced in the memo body.- }, [spacePageData, currentUserFid, wallets, isSignedIntoFarcaster]); + }, [spacePageData, currentUserFid, wallets]);
179-189: Align comment with behavior or implement Suspense path.Comment says “return undefined to trigger Suspense,” but code always falls back to
spacePageData.config. Either update the comment or returnundefinedwhen a user space is loading to leverageSpacePage’s Suspense.- // - Otherwise, return undefined to trigger Suspense while loading - const config = currentTabName && currentConfig?.tabs?.[currentTabName] ? { - ...currentConfig.tabs[currentTabName], - isEditable, - } : { - ...spacePageData.config, - isEditable, - }; + // - Otherwise, fall back to default config (or return undefined to trigger Suspense) + const config = + currentTabName && currentConfig?.tabs?.[currentTabName] + ? { ...currentConfig.tabs[currentTabName], isEditable } + : isNil(currentSpaceId) + ? { ...spacePageData.config, isEditable } + : undefined;
93-101: Avoid redundant store writes when props don’t change.No-op if values are already equal; reduces unnecessary store updates/renders.
useEffect(() => { const newSpaceId = spacePageData.spaceId ?? null; const newTabName = providedTabName || spacePageData.defaultTab; - - setCurrentSpaceId(newSpaceId); - setCurrentTabName(newTabName); - }, [spacePageData.spaceId, providedTabName, spacePageData.defaultTab, setCurrentSpaceId, setCurrentTabName]); + if (currentSpaceId !== newSpaceId) setCurrentSpaceId(newSpaceId); + if (currentTabName !== newTabName) setCurrentTabName(newTabName); + }, [ + spacePageData.spaceId, + providedTabName, + spacePageData.defaultTab, + currentSpaceId, + currentTabName, + setCurrentSpaceId, + setCurrentTabName, + ]);
153-167: Optional: guard against stale async loads.If IDs change quickly, earlier loads may complete late. Add a simple staleness check.
useEffect(() => { if (!currentSpaceId || !currentTabName) return; - const loadSpace = async () => { + const spaceId = currentSpaceId; + const tabName = currentTabName; + let cancelled = false; + const loadSpace = async () => { try { - await loadSpaceTabOrder(currentSpaceId); - await loadSpaceTab(currentSpaceId, currentTabName); + await loadSpaceTabOrder(spaceId); + if (cancelled) return; + await loadSpaceTab(spaceId, tabName); } catch (error) { console.error("Error loading space:", error); } }; - loadSpace(); + loadSpace(); + return () => { + cancelled = true; + }; }, [currentSpaceId, currentTabName, loadSpaceTabOrder, loadSpaceTab]);
64-89: Don’t select unused fields from the store.
clearLocalSpaces,getCurrentSpaceId, andgetCurrentTabNamearen’t used; removing them avoids unnecessary re-renders when they change.- } = useAppStore((state) => ({ - clearLocalSpaces: state.clearLocalSpaces, - getCurrentSpaceId: state.currentSpace.getCurrentSpaceId, + } = useAppStore((state) => ({ setCurrentSpaceId: state.currentSpace.setCurrentSpaceId, - getCurrentTabName: state.currentSpace.getCurrentTabName, setCurrentTabName: state.currentSpace.setCurrentTabName,
420-427: Consistency: cast network like other handlers.Minor consistency with other TabBar handlers using
as EtherScanChainName.- isTokenSpace(spacePageData) ? spacePageData.tokenData?.network : undefined + isTokenSpace(spacePageData) + ? (spacePageData.tokenData?.network as EtherScanChainName) + : undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(spaces)/PublicSpace.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (6)
src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfigSaveDetails(51-55)src/common/types/spaceData.ts (1)
isTokenSpace(85-87)src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(50-52)src/constants/etherscanChainIds.ts (1)
EtherScanChainName(1-1)src/app/(spaces)/SpacePage.tsx (1)
SpacePage(17-76)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores