Fix profile space tab selection using cached channel config #1469
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a tab-config sanitizer, mount-safe async guards, and deterministic space/tab resolution; updates tab load/switch/save/commit/reset flows and TabBar wiring; removes a previously exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant R as Router
participant P as PublicSpace
participant S as SpaceStore
participant T as TabLoader
participant B as TabBar
Note over P: derive identities & sanitize
U->>R: navigate(spaceId, tab)
R-->>P: route params
P->>P: compute currentSpaceId / currentTab
P->>P: matchesSpaceData -> matchingSpace
P->>P: compute resolvedTabName & activeSpaceId
P->>P: sanitizeTabConfig(candidate) -> cached/fallback config
alt need to load tabs
P->>T: loadRemainingTabs(activeSpaceId, resolvedTabName)
T-->>P: tabs loaded
end
P-->>B: render (resolvedTabName, activeSpaceId, handlers)
U->>B: trigger action (switch/save/commit/rename)
B->>P: invoke handler
P->>P: isMounted? & matchesSpaceData?
alt valid
P->>S: perform mutation via resolveSpaceIdForActions
S-->>P: store updated
P-->>U: UI updates
else invalid
P-->>U: abort/log
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 (1 warning)
✅ Passed checks (2 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 (3)
src/app/(spaces)/PublicSpace.tsx (3)
262-291: Prevent crash: guard all decodeURIComponent() callsdecodeURIComponent(providedTabName) is invoked even when providedTabName is undefined (Lines 275, 283, 289), which throws. Decode once with a guard and reuse.
Apply this diff:
- let nextSpaceId = spacePageData.spaceId; - let nextTabName = providedTabName ? decodeURIComponent(providedTabName) : spacePageData.defaultTab; + let nextSpaceId = spacePageData.spaceId; + const decodedProvidedTab = providedTabName ? decodeURIComponent(providedTabName) : undefined; + let nextTabName = decodedProvidedTab ?? spacePageData.defaultTab; @@ - if (existingSpace) { - nextSpaceId = existingSpace.id; - nextTabName = decodeURIComponent(providedTabName); - } + if (existingSpace) { + nextSpaceId = existingSpace.id; + nextTabName = decodedProvidedTab ?? spacePageData.defaultTab; + } @@ - if (existingSpace) { - nextSpaceId = existingSpace.id; - nextTabName = decodeURIComponent(providedTabName); - } + if (existingSpace) { + nextSpaceId = existingSpace.id; + nextTabName = decodedProvidedTab ?? spacePageData.defaultTab; + } @@ - if (spacePageData.spaceId) { - nextSpaceId = spacePageData.spaceId; - nextTabName = decodeURIComponent(providedTabName); - } + if (spacePageData.spaceId) { + nextSpaceId = spacePageData.spaceId; + nextTabName = decodedProvidedTab ?? spacePageData.defaultTab; + }
227-235: Cache clear condition may wipe the matching local spaceClearing when currentSpaceId !== spacePageData.spaceId is too coarse with the new matching logic; it can purge a correctly matched local space (e.g., token/channel/profile cases).
Apply this diff to gate by matchesSpaceData:
useEffect(() => { - const currentSpaceId = getCurrentSpaceId(); - if (currentSpaceId !== spacePageData.spaceId) { + const currentSpaceId = getCurrentSpaceId(); + const candidate = currentSpaceId ? localSpaces[currentSpaceId] : undefined; + if (currentSpaceId && !matchesSpaceData(candidate)) { clearLocalSpaces(); loadedTabsRef.current = {}; initialDataLoadRef.current = false; } -}, [clearLocalSpaces, getCurrentSpaceId, spacePageData.spaceId]); +}, [clearLocalSpaces, getCurrentSpaceId, localSpaces, matchesSpaceData]);
363-364: Wrong fallback tabHardcoding "Profile" breaks non-profile spaces. Use the page’s default tab.
Apply this diff:
- const currentTabName = getCurrentTabName() ?? "Profile"; + const currentTabName = getCurrentTabName() ?? spacePageData.defaultTab;
🧹 Nitpick comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
715-723: Remove unnecessary await on plain objectconfig is a plain object; awaiting it is a no-op and misleading.
Apply this diff:
- const resolvedConfig = await config; + const resolvedConfig = config; await Promise.all([ saveLocalSpaceTab(currentSpaceId, currentTabName, resolvedConfig), commitSpaceTab( currentSpaceId, currentTabName, isTokenSpace(spacePageData) ? spacePageData.tokenData?.network : undefined ) ]);
205-224: Avoid double-writes: consolidate space synchronization effectsTwo effects both adjust currentSpaceId/currentTabName. Merge into one that:
- derives matchingSpaceId via matchesSpaceData,
- sets currentSpaceId only when it changes,
- sets currentTabName using the decoded provided tab or default.
This reduces churn and risk of loops.
Also applies to: 256-303
📜 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 (2)
src/common/types/spaceData.ts (4)
isProfileSpace(81-83)isChannelSpace(93-95)isTokenSpace(85-87)isProposalSpace(89-91)src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)
🔇 Additional comments (3)
src/app/(spaces)/PublicSpace.tsx (3)
308-349: Good guardrails in loadRemainingTabsEarly return on non-matching space and using resolvedTabName for the active space avoids loading stale/mismatched tabs.
455-471: resolveSpaceIdForActions is a solid safety netValidating current space against page context and falling back to matchingSpaceId appropriately gates mutations.
793-798: Using resolvedTabName/current space order in TabBarPassing currentTab={resolvedTabName} and tabList based on activeSpaceId ensures consistent UI with validated state.
| const requestedTabName = | ||
| currentTabNameValue || providedTabName || spacePageData.defaultTab; | ||
|
|
There was a problem hiding this comment.
Use decoded provided tab when resolving requestedTabName
Using the raw providedTabName risks mismatch with config keys and can force an unnecessary default fallback.
Apply this diff:
- const requestedTabName =
- currentTabNameValue || providedTabName || spacePageData.defaultTab;
+ const providedTabDecoded = providedTabName
+ ? decodeURIComponent(providedTabName)
+ : undefined;
+ const requestedTabName =
+ currentTabNameValue || providedTabDecoded || spacePageData.defaultTab;📝 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 requestedTabName = | |
| currentTabNameValue || providedTabName || spacePageData.defaultTab; | |
| const providedTabDecoded = providedTabName | |
| ? decodeURIComponent(providedTabName) | |
| : undefined; | |
| const requestedTabName = | |
| currentTabNameValue || providedTabDecoded || spacePageData.defaultTab; |
🤖 Prompt for AI Agents
In src/app/(spaces)/PublicSpace.tsx around lines 161 to 163, the resolved
requestedTabName uses the raw providedTabName which can be URL-encoded and
mismatch config keys; decode the providedTabName before using it (e.g. safe
decodeURIComponent) and fall back to providedTabName if decoding fails or it's
falsy, then use currentTabNameValue || decodedProvidedTabName ||
spacePageData.defaultTab so encoded names match config keys.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
171-172: Decode providedTabName before using in requestedTabName resolution.The raw
providedTabNamecan be URL-encoded and won't match config keys, forcing an unnecessary fallback todefaultTab. This issue was flagged in a previous review but remains unaddressed.Apply this diff:
+ const providedTabDecoded = providedTabName + ? decodeURIComponent(providedTabName) + : undefined; const requestedTabName = - currentTabNameValue || providedTabName || spacePageData.defaultTab; + currentTabNameValue || providedTabDecoded || spacePageData.defaultTab;
🧹 Nitpick comments (4)
src/app/(spaces)/PublicSpace.tsx (4)
15-25: Consider modular lodash imports to reduce bundle size.While the current imports work correctly, using modular imports (e.g.,
import pickBy from 'lodash/pickBy') instead of importing from the main lodash package can reduce bundle size. This is a best practice for lodash usage.Based on learnings.
Apply this diff if you want to reduce bundle size:
-import { - indexOf, - isNil, - mapValues, - noop, - debounce, - pickBy, - isUndefined, - isPlainObject, - cloneDeep, -} from "lodash"; +import indexOf from "lodash/indexOf"; +import isNil from "lodash/isNil"; +import mapValues from "lodash/mapValues"; +import noop from "lodash/noop"; +import debounce from "lodash/debounce"; +import pickBy from "lodash/pickBy"; +import isUndefined from "lodash/isUndefined"; +import isPlainObject from "lodash/isPlainObject"; +import cloneDeep from "lodash/cloneDeep";
174-205: Consider extracting the nested transformation logic.The nested
pickByandmapValuesoperations (4 levels deep) make this code harder to read and maintain. Consider extracting the tab enrichment logic into a separate helper function.Example refactor:
const enrichTabConfig = (tabInfo: typeof matchingSpace.tabs[string]) => { if (!tabInfo) return undefined; return { ...tabInfo, fidgetInstanceDatums: mapValues( tabInfo.fidgetInstanceDatums, (datum) => ({ ...datum, config: { settings: datum.config.settings, editable: datum.config.editable, data: {}, }, }), ), }; }; const currentConfig = useMemo(() => { if (!matchingSpace) return undefined; const enrichedTabs = pickBy( mapValues(matchingSpace.tabs, enrichTabConfig), (value) => !isUndefined(value), ); return { ...matchingSpace, tabs: enrichedTabs }; }, [matchingSpace]);
382-426: Add logging when matchesSpaceData guard fails.The early return at lines 384-386 silently skips loading remaining tabs when the cached space doesn't match the page context. This can make debugging difficult.
Apply this diff to add logging:
async (spaceId: string) => { if (!matchesSpaceData(localSpaces[spaceId])) { + console.warn( + "Skipping loadRemainingTabs because cached space does not match page context", + { spaceId, spacePageData } + ); return; }
857-949: Approve defensive guards; consider refactoring for complexity.The addition of
matchesSpaceDatavalidation,resolvedTabNameusage, andisMountedRefguards significantly improves robustness and prevents stale state issues.However, this function is now 92 lines long and handles multiple concerns (validation, navigation, saving, loading). Consider extracting the save/commit logic into a separate helper function to improve maintainability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(spaces)/PublicSpace.tsx(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (4)
src/common/types/spaceData.ts (4)
isProfileSpace(81-83)isChannelSpace(93-95)isTokenSpace(85-87)isProposalSpace(89-91)src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)src/common/data/stores/app/space/spaceStore.ts (1)
UpdatableSpaceConfig(81-83)
🔇 Additional comments (3)
src/app/(spaces)/PublicSpace.tsx (3)
110-116: LGTM!The isMountedRef pattern is correctly implemented to guard async operations and prevent state updates after unmount. The cleanup function properly sets the ref to false.
118-154: LGTM!The
matchesSpaceDatafunction correctly validates cached spaces against the current page context. The logic properly handles all space types (profile, channel, token, proposal) and uses appropriate type guards to ensure type-safe property access.
540-582: LGTM!The
sanitizeTabConfighelper provides strong defensive validation. It correctly checks for plain objects (preventing prototype pollution), validates required fields, logs warnings for invalid configs, and returns deep clones to prevent shared references.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
181-182: DecodeprovidedTabNamebefore resolvingrequestedTabName.The raw
providedTabNamecan be URL-encoded (e.g., "My%20Tab"), which will mismatch with config keys that expect "My Tab". This was previously flagged but remains unaddressed.Apply this diff:
+ const providedTabDecoded = providedTabName + ? decodeURIComponent(providedTabName) + : undefined; const requestedTabName = - currentTabNameValue || providedTabName || spacePageData.defaultTab; + currentTabNameValue || providedTabDecoded || spacePageData.defaultTab;
🧹 Nitpick comments (2)
src/app/(spaces)/PublicSpace.tsx (2)
283-320: Cache clearing logic is correct but complex.The effect correctly clears local spaces when switching to a different space identity. The special case (lines 301-308) that skips clearing when there's no explicit
spaceIdbut theactiveSpaceIdmatches current is a subtle optimization to avoid unnecessary cache clears during profile/channel/token space loads.Note: The logic is correct but quite intricate. Consider adding inline comments explaining each branch condition for future maintainability.
922-1019: RefactoredswitchTabTocorrectly validates space context.The function now properly:
- Validates that the current local space matches page data before switching
- Uses
resolvedTabNameto identify the current tab being saved- Guards state updates and async operations with
isMountedRef- Converts configs to saveable format using
toSavableTabConfigMinor: Remove unnecessary
awaiton synchronousconfigvalue.Line 946 uses
await config, butconfig(defined at line 661) is a memoized synchronous value, not a promise. Theawaitis harmless but misleading.Apply this diff:
- const resolvedConfig = await config; + const resolvedConfig = config;Note: The same pattern appears in
renameTabat line 1079 and should be updated there as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(spaces)/PublicSpace.tsx(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (4)
src/common/types/spaceData.ts (5)
SpacePageData(18-36)isProfileSpace(81-83)isChannelSpace(93-95)isTokenSpace(85-87)isProposalSpace(89-91)src/common/data/stores/app/space/spaceStore.ts (2)
UpdatableSpaceConfig(81-83)UpdatableDatabaseWritableSpaceSaveConfig(76-79)src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
🔇 Additional comments (13)
src/app/(spaces)/PublicSpace.tsx (13)
9-48: LGTM!The new type definitions clearly distinguish between server-side tab configurations and sanitizable configurations, providing good type safety for the config handling logic introduced in this PR.
120-126: LGTM!Standard pattern for tracking component mount state to prevent state updates after unmount. This ref is properly used throughout the component to guard async operations.
128-164: LGTM!The
matchesSpaceDatafunction provides comprehensive validation across all space types (profile, channel, token, proposal) to ensure cached spaces align with the current page context. The logic correctly handles optional properties and type guards.
217-223: LGTM!The
resolvedTabNamelogic correctly validates that the requested tab exists in the current config before using it, with a safe fallback to the default tab.
225-243: LGTM!This effect correctly synchronizes the store's
currentSpaceIdwith thematchingSpaceId, ensuring that the store always reflects a space that matches the current page context or is cleared otherwise.
245-280: LGTM!The space identity resolution logic provides stable cache keys across space types, preferring explicit IDs when available and falling back to type-specific identifiers (e.g.,
profile:fid,channel:id).
392-436: LGTM!The
loadRemainingTabsfunction now correctly validates that the space matches current page data before loading tabs, and usesresolvedTabNameto identify the current tab when appropriate.
487-542: LGTM!Mount guards are correctly placed to prevent state updates after component unmount at all async operation completion points.
550-667: LGTM!The config helper functions provide robust validation and normalization:
sanitizeTabConfigvalidates structure and clones safelytoSavableTabConfigprepares configs for database persistence by removing non-saveable fields- Proper memoization prevents unnecessary recomputation
669-684: LGTM!The
resolveSpaceIdForActionshelper centralizes the logic for obtaining a validated space ID for tab actions, ensuring consistency across all TabBar action handlers.
884-919: LGTM!The
resetConfigfunction correctly uses the new sanitization pipeline, falling back to the default config if sanitization fails, and properly converts to a saveable format before persisting.
1036-1121: LGTM!The TabBar props are correctly updated to use:
resolvedTabNameforcurrentTabto ensure the active tab reflects validated stateactiveSpaceIdfortabListto show tabs from the matching spaceresolveSpaceIdForActionsacross all action handlers for consistency- Mount guards in async handlers to prevent updates after unmount
184-215: No changes needed: clearingconfig.datainPublicSpace.tsxis intentional to omit fidget data for public views.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
181-183: Use decoded providedTabName when computing requestedTabName.This issue was previously flagged: using the raw
providedTabNamerisks mismatch with config keys if the tab name is URL-encoded. TheresolvedTabNamecomputation (lines 217-223) depends onrequestedTabName, so this affects tab resolution throughout the component.Apply this diff:
+ const providedTabDecoded = providedTabName + ? decodeURIComponent(providedTabName) + : undefined; const requestedTabName = - currentTabNameValue || providedTabName || spacePageData.defaultTab; + currentTabNameValue || providedTabDecoded || spacePageData.defaultTab;
🧹 Nitpick comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
550-612: Well-structured validation and sanitization.The
sanitizeTabConfigfunction provides thorough validation of tab configs with appropriate warnings. UsingcloneDeepensures immutability, though note that this could impact performance for very large configs (consider profiling if performance issues arise).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(spaces)/PublicSpace.tsx(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(spaces)/PublicSpace.tsx (4)
src/common/types/spaceData.ts (5)
SpacePageData(18-36)isProfileSpace(81-83)isChannelSpace(93-95)isTokenSpace(85-87)isProposalSpace(89-91)src/common/data/stores/app/space/spaceStore.ts (2)
UpdatableSpaceConfig(81-83)UpdatableDatabaseWritableSpaceSaveConfig(76-79)src/common/data/stores/app/currentSpace/index.ts (2)
setCurrentSpaceId(43-47)setCurrentTabName(51-55)src/app/(spaces)/Space.tsx (1)
SpaceConfig(37-49)
🔇 Additional comments (1)
src/app/(spaces)/PublicSpace.tsx (1)
1018-1107: Consistent use of resolved space and tab identifiers.The TabBar props correctly use
resolvedTabNameforcurrentTab,activeSpaceIdfortabList, andresolveSpaceIdForActions()for all tab actions. This ensures operations target the correct space and tab context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/common/utils/sanitizeTabConfig.ts (1)
1-1: Consider per-method lodash imports for smaller bundle size.While the current destructured import works fine, using per-method imports can reduce bundle size:
-import { cloneDeep, isArray, isPlainObject } from "lodash"; +import cloneDeep from "lodash/cloneDeep"; +import isArray from "lodash/isArray"; +import isPlainObject from "lodash/isPlainObject";Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/utils/sanitizeTabConfig.ts(1 hunks)
🔇 Additional comments (1)
src/common/utils/sanitizeTabConfig.ts (1)
1-106: LGTM! Robust validation and sanitization logic.The implementation correctly validates tab config structure, handles edge cases (null/undefined checks, type guards), and uses deep cloning to prevent mutations. The isPrivate normalization logic properly handles boolean coercion and defaults.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e532a097b88325877b26dc7e8f955a
Summary by CodeRabbit
Improvements
Bug Fixes
Tests
Chores