Nav editor#1630
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a local-first, admin-editable navigation system: new Zustand navigation store and hooks, drag-and-drop editor and item components, signed server API for navigation config updates with validation, SystemConfig extensions (communityId, adminIdentityPublicKeys), client-side fallback for missing server space data, plus related docs and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (Browser)
participant Editor as NavigationEditor
participant Store as Navigation Store
participant SpaceSvc as Space Service
participant API as /api/navigation/config
participant DB as Database
Admin->>Editor: Enter edit mode and modify items
Editor->>Store: create/update/delete/reorder (local state)
Admin->>Editor: Commit
Editor->>Store: commitNavigationChanges()
Store->>SpaceSvc: Register new spaces (signed requests)
SpaceSvc->>DB: Create space records
SpaceSvc-->>Store: Return spaceIds
Store->>API: PUT /api/navigation/config (signed payload)
API->>DB: Verify admin, validate items, update navigation_config
DB-->>API: Success
API-->>Store: Confirm update
Store->>Editor: Update remoteNavigation, clear uncommitted changes
Editor->>Admin: Show success, exit edit mode
sequenceDiagram
participant User as Browser
participant Page as page.tsx (server)
participant Client as NavPageSpace (client)
participant LocalStore as Local Store
participant DB as Database
User->>Page: Request /[navSlug]/[[...tabName]]
Page->>DB: Fetch spaceData for navSlug
alt spaceData found
Page-->>Client: spaceData (server-rendered)
Client->>Client: Render from server data
else spaceData missing
Page-->>Client: null + adminIdentityPublicKeys
Client->>LocalStore: Query localNavigation & local spaces
alt Local data available
LocalStore-->>Client: Construct spaceData from local
Client->>User: Render local-first space
else
Client-->>User: Render 404
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 8
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/Sidebar.tsx (1)
63-81:navEditModeis not destructured, causing incorrect conditional logic.On line 79,
navEditModeis referenced but it's not included in the destructuring on line 64. This meansnavEditModewill beundefined, and the conditioneditMode && !navEditModewill effectively beeditMode && true, which defeats the purpose of the navigation edit mode check.🔎 Proposed fix
- const { editMode, setEditMode, sidebarEditable, portalRef } = + const { editMode, setEditMode, navEditMode, sidebarEditable, portalRef } = useSidebarContext();
🧹 Nitpick comments (10)
src/common/data/stores/app/index.tsx (1)
70-78: Consider clearing navigation state on logout.The
logoutfunction resets account, homebase, space, checkpoints, and chat stores, but doesn't reset navigation state. If users make local navigation edits, those could persist after logout and potentially be visible to another user on the same device.🔎 Proposed fix
logout: () => { get().account.reset(); get().homebase.clearHomebase(); get().homebase.clearHomebaseTabOrder(); get().space.clear(); get().checkpoints.clearCheckpoints(); get().chat.clearMessages(); + get().navigation.resetNavigationChanges(); localStorage.removeItem(LOCAL_STORAGE_LOCATION); },src/common/components/organisms/NavigationManagement.tsx (3)
54-56: Simplify the nested ternary logic for readability.The current logic is hard to follow with nested ternaries. Consider simplifying:
🔎 Proposed refactor
- // Use local navigation items if available, otherwise fall back to props - // Only use localNavigation if it has items (not empty array from initial state) - const items = localNavigation.length > 0 || navigationItems.length === 0 - ? localNavigation.length > 0 ? localNavigation : navigationItems - : navigationItems; + // Use local navigation items if available, otherwise fall back to props + const items = localNavigation.length > 0 ? localNavigation : navigationItems;The simplified version achieves the same result: use
localNavigationwhen it has items, otherwise usenavigationItems.
58-63: Add cleanup for the debounced function.The debounced function should be cancelled on unmount to prevent potential memory leaks or state updates on unmounted components.
🔎 Proposed fix using useMemo with cleanup
+import { useEffect, useMemo } from "react"; + - const debouncedUpdateOrder = useCallback( - debounce((newOrder: NavigationItem[]) => { + const debouncedUpdateOrder = useMemo( + () => debounce((newOrder: NavigationItem[]) => { updateNavigationOrder(newOrder); }, 300), [updateNavigationOrder] ); + + useEffect(() => { + return () => { + debouncedUpdateOrder.cancel(); + }; + }, [debouncedUpdateOrder]);
107-115: Consider removing the 404-as-success fallback before production.Treating 404 errors as success is useful for local testing, but this should be removed or feature-flagged before production deployment to avoid masking real API issues.
🔎 Suggestion
Consider wrapping this logic in an environment check:
} catch (error: any) { console.error("Failed to commit navigation changes:", error); - // For local testing, still show success if it's just a 404 (API not implemented) - if (error?.response?.status === 404) { + // TODO: Remove this fallback before production + if (process.env.NODE_ENV === 'development' && error?.response?.status === 404) { toast.success("Navigation changes staged locally (API endpoints not yet implemented)"); onCommit?.(); } else { toast.error("Failed to commit navigation changes"); }src/common/components/organisms/Navigation.tsx (2)
559-566: Click handler relies on DOM inspection which can be fragile.Using
target.closest('button[aria-label="Delete item"]')or checkingtagName === 'INPUT'is fragile if the DOM structure changes. Consider usingevent.stopPropagation()directly on the delete button and EditableText wrapper instead.
613-630:hasUncommittedChanges()is called during render.Calling a function that reads state during render (in the JSX condition) could cause unnecessary re-renders if the function implementation changes. Consider storing the result in a variable or using
useMemo.🔎 Proposed fix
+ // Memoize uncommitted changes check + const showCommitButtons = React.useMemo( + () => hasUncommittedChanges(), + [localNavigation, hasUncommittedChanges] + ); {/* Commit/Cancel buttons */} - {hasUncommittedChanges() && !shrunk && ( + {showCommitButtons && !shrunk && ( <div className="flex gap-2 mt-4 pt-4 border-t">src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (2)
96-107: Consider using a more descriptivespaceNameinstead of rawnavSlug.Currently
spaceNameis set tonavSlugdirectly. For better UX, consider using the navigation item's label if available:🔎 Proposed fix
return { spaceId: localNavItem.spaceId, - spaceName: navSlug, + spaceName: localNavItem.label || navSlug, spaceType: SPACE_TYPES.NAV_PAGE,
110-122: 404 UI is basic but functional.For a client component, this is an appropriate fallback. Consider adding a link back to home for better UX, or using
useRouterto enable programmatic navigation.src/pages/api/navigation/config.ts (1)
25-38: Type guard could be more thorough.The type guard validates the shape but doesn't check if
navigationConfig.itemsarray elements have the required structure when present. Consider adding validation for item structure.🔎 Proposed fix
function isUpdateNavigationConfigRequest( thing: unknown, ): thing is UpdateNavigationConfigRequest { + const isValidNavigationItem = (item: unknown): boolean => { + if (typeof item !== 'object' || item === null) return false; + const i = item as Record<string, unknown>; + return typeof i.id === 'string' && + typeof i.label === 'string' && + typeof i.href === 'string'; + }; + return ( isSignable(thing) && typeof thing["communityId"] === "string" && typeof thing["publicKey"] === "string" && typeof thing["timestamp"] === "string" && thing["navigationConfig"] !== undefined && typeof thing["navigationConfig"] === "object" && (thing["navigationConfig"]["items"] === undefined || - isArray(thing["navigationConfig"]["items"])) + (isArray(thing["navigationConfig"]["items"]) && + thing["navigationConfig"]["items"].every(isValidNavigationItem))) ); }src/common/data/stores/app/navigation/navigationStore.ts (1)
252-263: TODO for space deletion left unimplemented.Deleting navigation items with associated spaces doesn't clean up the space data. This could lead to orphaned space records in the database over time.
Would you like me to open an issue to track implementing the space deletion endpoint?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
scripts/seed.ts(2 hunks)src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx(4 hunks)src/app/[navSlug]/[[...tabName]]/page.tsx(1 hunks)src/common/components/atoms/editable-text.tsx(1 hunks)src/common/components/organisms/Navigation.tsx(6 hunks)src/common/components/organisms/NavigationManagement.tsx(1 hunks)src/common/components/organisms/Sidebar.tsx(3 hunks)src/common/data/stores/app/index.tsx(3 hunks)src/common/data/stores/app/navigation/navigationStore.ts(1 hunks)src/pages/api/navigation/config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/pages/api/navigation/config.ts (4)
src/config/systemConfig.ts (1)
NavigationConfig(155-160)src/common/lib/signedFiles.ts (3)
Signable(20-23)isSignable(29-41)validateSignable(43-52)src/common/data/api/requestHandler.ts (2)
NounspaceResponse(9-16)requestHandler(31-51)src/common/data/database/supabase/clients/server.ts (1)
createSupabaseServerClient(7-7)
src/common/data/stores/app/index.tsx (1)
src/common/data/stores/app/navigation/navigationStore.ts (2)
NavigationStore(51-51)createNavigationStoreFunc(58-292)
src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (2)
src/common/types/spaceData.ts (2)
NavPageSpacePageData(77-83)SPACE_TYPES(7-13)src/common/lib/hooks/useCurrentSpaceIdentityPublicKey.ts (1)
useCurrentSpaceIdentityPublicKey(3-5)
src/common/data/stores/app/navigation/navigationStore.ts (6)
src/common/types/spaceData.ts (1)
SPACE_TYPES(7-13)src/config/systemConfig.ts (2)
NavigationItem(167-175)NavigationConfig(155-160)src/common/data/stores/createStore.ts (2)
StoreSet(51-51)StoreGet(52-52)src/common/data/stores/app/index.tsx (1)
AppStore(44-57)src/common/lib/signedFiles.ts (1)
signSignable(54-62)src/pages/api/space/registry/index.ts (1)
RegisterNewSpaceResponse(114-114)
🔇 Additional comments (9)
src/common/components/atoms/editable-text.tsx (1)
42-56: LGTM!The event propagation handling is correctly implemented. Stopping propagation on both the input click and the display div double-click prevents unintended navigation when editing inline text within navigation items.
src/app/[navSlug]/[[...tabName]]/page.tsx (1)
30-43: LGTM with a minor observation.The client-side fallback pattern is implemented correctly. The dynamic import of
loadSystemConfigand the defensive default foradminIdentityPublicKeysare appropriate.Consider moving the dynamic import to the top of the file as a static import if
loadSystemConfigis always needed, which would improve code predictability and potentially bundle optimization. However, this is a minor point and the current approach works.src/common/data/stores/app/index.tsx (1)
37-41:partializedNavigationStoreis imported but not used in persistence.The
partializedNavigationStoreis imported on line 40, but it's not included in thepartializefunction (lines 109-115). This means navigation state won't persist to localStorage across sessions.If navigation changes should persist locally (for the local-first editing flow), add it to the partialize function. If navigation state is intentionally ephemeral, remove the unused import.
🔎 Proposed fix (if persistence is needed)
partialize: (state: AppStore) => ({ account: partializedAccountStore(state), homebase: partializedHomebaseStore(state), space: partializedSpaceStore(state), checkpoints: partializedCheckpointStore(state), chat: partializedChatStore(state), + navigation: partializedNavigationStore(state), }),Also applies to: 109-115
src/common/components/organisms/NavigationManagement.tsx (1)
147-180: LGTM!The Reorder.Group implementation is correct. Keys are properly set using
item.id, and the drag-and-drop reordering integrates well with the debounced update callback.src/common/components/organisms/Navigation.tsx (1)
634-655: JSX structure mismatch:<ul>tag opened but items conditionally rendered.Lines 634-655 map navigation items inside a
<ul>that starts at line 634, but the closing</ul>is at line 698, after the conditional edit button and otherNavButton/NavItemcomponents. This is correct structurally, but ensure the edit button (lines 688-697) should be inside the<ul>as it usesNavButtonwhich renders<li>.Also applies to: 656-698
src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (1)
69-108: Fallback logic looks correct for uncommitted navigation items.The fallback mechanism properly checks local stores when server data is unavailable, enabling preview of uncommitted navigation items. The dependency array is comprehensive.
src/pages/api/navigation/config.ts (1)
87-96: Admin key check is correct and secure.The authorization properly validates that the signer's public key is in the admin list before allowing updates.
src/common/data/stores/app/navigation/navigationStore.ts (2)
72-104: Good separation of concerns for navigation item creation.The implementation correctly handles the optional space creation alongside navigation item creation, maintaining consistency between the navigation and space stores.
142-159:hasUncommittedChangescomparison is correct but could be simplified.The logic correctly compares length, order, and key properties. Consider using
isEqualfrom lodash for a more maintainable comparison, though the current explicit approach has the benefit of being clear about which fields matter.
| const debouncedUpdateOrder = React.useCallback( | ||
| debounce((newOrder: NavigationItem[]) => { | ||
| updateNavigationOrder(newOrder); | ||
| }, 300), | ||
| [updateNavigationOrder] | ||
| ); |
There was a problem hiding this comment.
Missing cleanup for debounced function.
The debounce function creates a timer that should be cancelled on unmount to prevent memory leaks and stale updates. Additionally, the dependency array is missing from React.useCallback.
🔎 Proposed fix
- // Debounced reorder handler
- const debouncedUpdateOrder = React.useCallback(
- debounce((newOrder: NavigationItem[]) => {
- updateNavigationOrder(newOrder);
- }, 300),
- [updateNavigationOrder]
- );
+ // Debounced reorder handler
+ const debouncedUpdateOrder = React.useMemo(
+ () => debounce((newOrder: NavigationItem[]) => {
+ updateNavigationOrder(newOrder);
+ }, 300),
+ [updateNavigationOrder]
+ );
+
+ // Cleanup debounce on unmount
+ React.useEffect(() => {
+ return () => {
+ debouncedUpdateOrder.cancel();
+ };
+ }, [debouncedUpdateOrder]);📝 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 debouncedUpdateOrder = React.useCallback( | |
| debounce((newOrder: NavigationItem[]) => { | |
| updateNavigationOrder(newOrder); | |
| }, 300), | |
| [updateNavigationOrder] | |
| ); | |
| // Debounced reorder handler | |
| const debouncedUpdateOrder = React.useMemo( | |
| () => debounce((newOrder: NavigationItem[]) => { | |
| updateNavigationOrder(newOrder); | |
| }, 300), | |
| [updateNavigationOrder] | |
| ); | |
| // Cleanup debounce on unmount | |
| React.useEffect(() => { | |
| return () => { | |
| debouncedUpdateOrder.cancel(); | |
| }; | |
| }, [debouncedUpdateOrder]); |
🤖 Prompt for AI Agents
In src/common/components/organisms/Navigation.tsx around lines 159-164, the
debouncedUpdateOrder created via debounce needs proper cleanup and stable
dependencies: move the debounced function into a ref (e.g. const debouncedRef =
useRef(debounce(...))) or create it inside useCallback but ensure debounce is
created only once, then add a useEffect that on unmount calls
debouncedRef.current.cancel() (or the debounce library's cancel/clear method) to
avoid timers leaking; also ensure the useCallback/useRef setup includes
updateNavigationOrder in its dependency handling (either rebuild the debounced
function when updateNavigationOrder changes or use a ref to the latest
updateNavigationOrder) so the dependency array is correct and linter-safe.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/common/data/stores/app/navigation/navigationStore.ts:
- Around line 265-274: The hasUncommittedChanges check currently uses strict !==
for localItem.icon vs remoteItem.icon which will falsely flag differences when
icon is an object; update the comparison to perform a deep-equality check (e.g.,
use a utility like lodash/isEqual or compare JSON.stringify(localItem.icon) ===
JSON.stringify(remoteItem.icon)) and replace the icon inequality test inside the
state.localNavigation.some callback accordingly, keeping all other field
comparisons the same and ensuring null/undefined icons are handled consistently.
- Around line 420-434: The catch block in navigationStore.ts currently logs full
error.response.headers and serializes the entire error object which can expose
sensitive info; update the logging in that catch to omit or redact sensitive
header fields (e.g., Authorization, Cookie, Set-Cookie) and stop
JSON.stringify-ing the whole error — instead log only non-sensitive fields such
as error.response.status, error.response.statusText, and a truncated or
sanitized error.message (or a safe subset of error.response.data), or implement
a small sanitizer that removes header keys before logging; keep rethrowing the
original error unchanged.
In @tests/navigation/errorHandling.test.ts:
- Around line 9-60: Remove the local duplicate type and functions from the test
and instead export NavigationError, normalizeNavigationError, and
getUserFriendlyMessage from the production module (either add these exports to
useNavigation.ts or move them into a new errorHandling.ts and export them), then
update the test to import those exact symbols (NavigationError,
normalizeNavigationError, getUserFriendlyMessage) and use them; ensure the
exported implementations match the current logic (including axios.isAxiosError
checks and retryable logic) so tests exercise the real code rather than a local
copy.
🧹 Nitpick comments (23)
src/common/utils/tabUtils.ts (1)
36-58: Consider case-insensitive duplicate checking for consistency.The
generateUniqueNamefunction uses exact string matching (existingNames.includes(uniqueName)on line 53), whileisDuplicateTabNameuses case-insensitive matching (line 32). This inconsistency could allowgenerateUniqueNameto create names that differ only in case (e.g., "Tab" and "tab"), whichisDuplicateTabNamewould flag as duplicates.Consider normalizing the comparison for consistency:
♻️ Proposed fix to align case-sensitivity
export function generateUniqueName( baseName: string, existingNames: string[], maxIterations: number = 100 ): string | null { const trimmedBase = baseName.trim(); let uniqueName = trimmedBase; let iter = 1; + const normalizedExisting = existingNames.map(name => name.toLowerCase()); - while (existingNames.includes(uniqueName) && iter <= maxIterations) { + while (normalizedExisting.includes(uniqueName.toLowerCase()) && iter <= maxIterations) { uniqueName = `${trimmedBase} - ${iter++}`; } return iter > maxIterations ? null : uniqueName; }src/common/components/organisms/TabBar.tsx (1)
16-16: LGTM! Consider adding user feedback for edge case.The refactor to use
generateUniqueNamecorrectly extracts the deduplication logic into a reusable utility. The null return case is properly handled by the caller at line 169.As an optional enhancement, consider adding user feedback if
generateUniqueNamereturns null (when max iterations is exceeded), since the current implementation silently fails:Optional: Add user feedback
const uniqueName = generateUniqueTabName(sanitizedName); if (!uniqueName || uniqueName === tabName) { + if (!uniqueName) { + toast.error("Unable to generate a unique tab name. Please try a different name."); + } return; }Also applies to: 205-205
BRANCH_REVIEW.md (1)
1-409: Consider removing this review document before merging.This appears to be an internal code review/analysis artifact rather than permanent project documentation. Review notes documenting issues and recommendations are valuable during PR review but typically should not be committed to the repository.
If this analysis should persist, consider:
- Moving actionable items to GitHub issues
- Converting stable recommendations to ADRs (Architecture Decision Records) or contributing guidelines
- Adding a
docs/reviews/directory if review artifacts are intentionally retainedREFACTOR_REVIEW.md (1)
1-287: Same concern: review artifact should not be committed.Like
BRANCH_REVIEW.md, this is a review document that would be better suited as PR comments, issues, or ADRs rather than committed markdown files. Consider removing before merge or relocating to appropriate documentation structure.src/common/data/stores/app/space/spaceStore.ts (1)
691-740: Robust error handling for space tab loading.Good defensive programming with:
- HTML 404 detection in both the catch block and after successful fetch (covers different axios configurations)
- Separate try-catch blocks for JSON parsing and decryption with specific error messages
- Graceful fallback to local data when remote tab doesn't exist
The duplicate HTML detection logic (lines 704-711 and 718-721) could be extracted to a helper function for DRY, but the current approach is acceptable.
Optional: Extract HTML detection helper
// Helper at module level const isHtml404Response = (text: string): boolean => { const trimmed = text.trim(); return trimmed.startsWith("<!DOCTYPE") || trimmed.startsWith("<html"); };src/common/utils/navUtils.ts (1)
49-58: Misleading comment about protocol-relative URLs.The comment on line 55 mentions "protocol-relative URLs" but:
- Protocol-relative URLs (
//example.com) start with//and wouldn't pass the path regex on line 52- The actual intent appears to be preventing malformed paths like
/foo//barConsider updating the comment for clarity:
Suggested comment fix
- // Prevent double slashes (except at start for protocol-relative URLs) + // Prevent double slashes in path segments (e.g., "/foo//bar") if (trimmedHref.includes("//") && !trimmedHref.startsWith("//")) {src/common/components/atoms/editable-text.tsx (2)
35-52: Validation flow works but UX could be improved.The validation logic is correct - it resets text and stays in edit mode on error. However:
User feedback:
console.warn(line 46) won't be visible to users. Consider surfacing the validation error in the UI (tooltip, toast, or inline message).Edit mode behavior: On validation failure, the text resets to
initialTextbut editing continues. This might confuse users who don't understand why their input was rejected.Consider showing the validation error to users:
Optional: Add user-facing feedback
+import { toast } from "sonner"; // or your toast library if (validationError) { // Reset text to original and stay in edit mode settext(initialText); - console.warn("Invalid input:", validationError); + toast.error(validationError); return; // Don't exit edit mode, don't call updateMethod }
75-85: Double-click works but lacks keyboard accessibility.The double-click to edit works correctly with proper event handling. However, there's no keyboard-accessible way to enter edit mode. Consider adding:
tabIndex={0}to make the element focusableonKeyDownhandler to enter edit mode on Enter/SpaceOptional: Add keyboard accessibility
<div - className="select-none min-w-0" + className="select-none min-w-0 cursor-pointer" + tabIndex={0} + role="button" + aria-label={`Edit ${text}`} onDoubleClick={(e) => { e.preventDefault(); e.stopPropagation(); setisEditing(true); }} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + setisEditing(true); + } + }} >src/common/components/organisms/navigation/ErrorBoundary.tsx (1)
61-67: Consider accepting a reset callback prop for more robust recovery.The
handleResetmethod clears error state, but if the underlying error is deterministic (e.g., bad navigation data), re-rendering children will immediately re-throw. Consider adding an optionalonResetprop that allows parent components to perform additional cleanup (e.g., resetting navigation state to a known good value) before re-rendering.💡 Suggested enhancement
interface ErrorBoundaryProps { children: ReactNode; fallback?: ReactNode; onError?: (error: Error, errorInfo: ErrorInfo) => void; + onReset?: () => void; } // ... handleReset = () => { + this.props.onReset?.(); this.setState({ hasError: false, error: null, errorInfo: null, }); };tests/navigation/errorHandling.test.ts (1)
100-111: Consider using Vitest's mocking utilities for cleaner axios mocking.The manual override of
axios.isAxiosErrorworks but is fragile. Vitest provides cleaner mocking utilities.💡 Cleaner approach with vi.spyOn
import { vi } from 'vitest'; it('should normalize network errors from axios', () => { const axiosError = Object.assign(new Error('Request failed'), { isAxiosError: true, response: { status: 500, data: { error: { message: 'Internal server error' } }, }, }); vi.spyOn(axios, 'isAxiosError').mockImplementation((err) => err === axiosError); const normalized = normalizeNavigationError(axiosError); expect(normalized.type).toBe('NETWORK'); expect(normalized.retryable).toBe(true); vi.restoreAllMocks(); });src/common/components/organisms/navigation/useNavigation.ts (1)
119-125: Unused destructured values:deleteNavigationItemandrenameNavigationItem.These are destructured from the store but never used within this hook. If they're intended for external use, they should be included in the return value. Otherwise, remove them to avoid confusion.
♻️ Remove unused destructuring
const { loadNavigation, localNavigation, hasUncommittedChanges, createNavigationItem, - deleteNavigationItem, - renameNavigationItem, updateNavigationOrder, commitNavigationChanges, resetNavigationChanges, } = useAppStore((state) => ({ loadNavigation: state.navigation.loadNavigation, localNavigation: state.navigation.localNavigation, hasUncommittedChanges: state.navigation.hasUncommittedChanges, createNavigationItem: state.navigation.createNavigationItem, - deleteNavigationItem: state.navigation.deleteNavigationItem, - renameNavigationItem: state.navigation.renameNavigationItem, updateNavigationOrder: state.navigation.updateNavigationOrder, commitNavigationChanges: state.navigation.commitNavigationChanges, resetNavigationChanges: state.navigation.resetNavigationChanges, }));src/common/components/organisms/navigation/NavigationEditor.tsx (2)
207-212: Remove emptyonKeyDownhandler.This handler checks a condition but does nothing - navigation is already handled by the
Linkcomponent. Empty handlers can be confusing and suggest incomplete implementation.♻️ Remove the empty handler
- onKeyDown={(e) => { - // Allow keyboard navigation but prevent during drag - if (e.key === "Enter" && !e.currentTarget.closest('[data-dragging="true"]')) { - // Navigation handled by Link component - } - }}
233-254: Extract duplicate delete logic into a single handler.The click and keydown handlers duplicate the same
onDelete+ toast logic.♻️ Deduplicate delete handlers
+ const handleDeleteItem = (e: React.SyntheticEvent) => { + e.preventDefault(); + e.stopPropagation(); + onDelete(item.id); + toast.success("Navigation item deleted"); + }; + <button - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - onDelete(item.id); - toast.success("Navigation item deleted"); - }} + onClick={handleDeleteItem} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - e.stopPropagation(); - onDelete(item.id); - toast.success("Navigation item deleted"); + handleDeleteItem(e); } }}src/common/components/organisms/Navigation.tsx (3)
266-270: Consider combining store selectors to reduce subscriptions.This creates a second
useAppStoresubscription in addition to the one inuseNavigationhook (which also destructuresdeleteNavigationItemandrenameNavigationItembut doesn't use them). Consider either:
- Having
useNavigationreturn these handlers, or- Combining this with the existing
useAppStorecall at lines 70-76Multiple subscriptions can cause unnecessary re-renders.
♻️ Option: Return handlers from useNavigation
In
useNavigation.ts, include these in the return:return { // ... existing returns deleteNavigationItem, renameNavigationItem, };Then in
Navigation.tsx:const { localNavigation, hasUncommittedChanges, navItemsToDisplay, debouncedUpdateOrder, handleCommit, handleCancel, handleCreateItem, + deleteNavigationItem, + renameNavigationItem, } = useNavigation(systemConfig, setNavEditMode); - // Get store functions for editor - const { deleteNavigationItem, renameNavigationItem } = useAppStore((state) => ({ - deleteNavigationItem: state.navigation.deleteNavigationItem, - renameNavigationItem: state.navigation.renameNavigationItem, - }));
370-370: Simplify redundant conditional.
usernameis alreadystring | undefined(line 227), sousername ? username : undefinedis equivalent to justusername.- username={username ? username : undefined} + username={username}
467-481: Extract inline icon component to avoid recreation on each render.The inline arrow function at line 470-474 creates a new component on every render, which can cause unnecessary re-renders of
NavigationButton.♻️ Extract to a stable component reference
+const EditNavIcon = () => ( + <div className="w-6 h-6 flex items-center justify-center"> + <FaPencil className="w-4 h-4 text-gray-800 dark:text-white" /> + </div> +); // In the component: {isNavigationEditable && !mobile && ( <NavigationButton label={navEditMode ? "Exit Edit" : "Edit Nav"} - Icon={navEditMode ? LogoutIcon : () => ( - <div className="w-6 h-6 flex items-center justify-center"> - <FaPencil className="w-4 h-4 text-gray-800 dark:text-white" /> - </div> - )} + Icon={navEditMode ? LogoutIcon : EditNavIcon} onClick={() => { setNavEditMode(!navEditMode); }}src/common/components/organisms/navigation/NavigationItem.tsx (1)
84-102: Redundante.preventDefault()call on line 96.The
e.preventDefault()on line 93 already prevents the default behavior for both Enter and Space keys. The nested call on line 96 is unnecessary.Suggested simplification
const handleKeyDown = useCallback( (e: React.KeyboardEvent<HTMLAnchorElement>) => { if (disable) { e.preventDefault(); return; } // Enter or Space key activates the link if (e.key === "Enter" || e.key === " ") { e.preventDefault(); - if (e.key === " ") { - // Prevent page scroll on Space - e.preventDefault(); - } handleClick(e as unknown as React.MouseEvent<HTMLAnchorElement>); } }, [disable, handleClick] );src/pages/api/navigation/config.ts (2)
80-81: Remove debug logging before production.Line 81 logs navigation items including IDs, labels, and hrefs. This should be removed or guarded behind a debug flag to avoid log pollution in production.
Suggested fix
- // Debug: log what we received - console.log("Received navigation items:", updateRequest.navigationConfig.items?.map((item: NavigationItem) => ({ id: item.id, label: item.label, href: item.href }))); + // Debug logging - enable only in development + if (process.env.NODE_ENV === 'development') { + console.log("Received navigation items:", updateRequest.navigationConfig.items?.map((item: NavigationItem) => ({ id: item.id, label: item.label, href: item.href }))); + }
103-112: Potential information disclosure in error message.The 403 error response includes the public key and community ID. While the public key is not secret, including it in error messages could aid reconnaissance. Consider a more generic message.
Suggested fix
if (!isArray(adminKeys) || !adminKeys.includes(updateRequest.publicKey)) { res.status(403).json({ result: "error", error: { - message: `Identity ${updateRequest.publicKey} is not an admin for community ${updateRequest.communityId}`, + message: "You do not have permission to update navigation for this community.", }, }); return; }CRITIQUE_VERIFICATION.md (1)
1-214: Consider whether this verification document should be committed.This document is useful for tracking that review feedback was addressed, but it may be more appropriate as a PR comment or wiki page rather than a committed file. If kept, consider:
- Moving to a
/docsor/notesdirectory- Adding a note that it's internal documentation
- Removing it after merge if it's only for review purposes
The document does provide valuable context about the error handling architecture, so keeping a refined version could aid future maintainers.
src/common/data/stores/app/navigation/navigationStore.ts (3)
117-122: Fallback href usesDate.now()which could collide in rapid succession.If the generated href validation fails (noted as "shouldn't happen"), the fallback
/item-${Date.now()}could theoretically collide if called multiple times within the same millisecond. Consider using the already-generatednewId(UUID) for guaranteed uniqueness.Suggested fix
+ const newId = uuidv4(); + // Auto-generate href from label, ensuring uniqueness trimmedHref = generateUniqueHrefFromLabel(uniqueLabel, state.localNavigation); // Validate generated href (should always be valid, but check anyway) const hrefError = validateNavItemHref(trimmedHref); if (hrefError) { // This shouldn't happen with our generator, but handle it gracefully - trimmedHref = `/item-${Date.now()}`; + trimmedHref = `/item-${newId}`; } } - const newId = uuidv4(); const spaceId = uuidv4();
137-163: Two separatesetcalls create non-atomic state updates.The space creation (lines 137-159) and navigation item addition (lines 161-163) are in separate
setcalls. If an error occurs between them, state could be inconsistent (space exists without nav item). Consider combining into a singlesetcall.Suggested fix
- set((draft) => { - // Add to space store's localSpaces so PublicSpace can find it - // Initialize with default "Home" tab so the space can be viewed immediately - draft.space.localSpaces[spaceId] = { - id: spaceId, - updatedAt: timestamp, - tabs: { - Home: { - ...INITIAL_SPACE_CONFIG_EMPTY, - theme: { - ...INITIAL_SPACE_CONFIG_EMPTY.theme, - id: `${spaceId}-Home-theme`, - name: `${spaceId}-Home-theme`, - }, - isPrivate: false, - timestamp: timestamp, - }, - }, - order: ["Home"], // Initial default tab - changedNames: {}, - deletedTabs: [], - }; - }, "createNavigationItem-addLocalSpace"); - - set((draft) => { - draft.navigation.localNavigation.push(cloneDeep(newItem)); - }, "createNavigationItem"); + set((draft) => { + // Add to space store's localSpaces so PublicSpace can find it + // Initialize with default "Home" tab so the space can be viewed immediately + draft.space.localSpaces[spaceId] = { + id: spaceId, + updatedAt: timestamp, + tabs: { + Home: { + ...INITIAL_SPACE_CONFIG_EMPTY, + theme: { + ...INITIAL_SPACE_CONFIG_EMPTY.theme, + id: `${spaceId}-Home-theme`, + name: `${spaceId}-Home-theme`, + }, + isPrivate: false, + timestamp: timestamp, + }, + }, + order: ["Home"], + changedNames: {}, + deletedTabs: [], + }; + // Add navigation item in the same transaction + draft.navigation.localNavigation.push(cloneDeep(newItem)); + }, "createNavigationItem");
391-397: Remove debug logging or guard with environment check.Similar to the API endpoint, this debug logging should be removed or guarded for production.
Suggested fix
- // Debug: log what we're about to send - console.log("Committing navigation config:", { - items: itemsToCommit.map(item => ({ id: item.id, label: item.label, href: item.href })), - logoTooltip: navigationConfigToCommit.logoTooltip, - showMusicPlayer: navigationConfigToCommit.showMusicPlayer, - showSocials: navigationConfigToCommit.showSocials, - }); + // Debug logging - enable only in development + if (process.env.NODE_ENV === 'development') { + console.log("Committing navigation config:", { + items: itemsToCommit.map(item => ({ id: item.id, label: item.label, href: item.href })), + logoTooltip: navigationConfigToCommit.logoTooltip, + showMusicPlayer: navigationConfigToCommit.showMusicPlayer, + showSocials: navigationConfigToCommit.showSocials, + }); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
BRANCH_REVIEW.mdCRITIQUE_VERIFICATION.mdNAVIGATION_REFACTOR_PLAN.mdREFACTOR_REVIEW.mddocs/GETTING_STARTED.mddocs/SYSTEMS/CONFIGURATION/ARCHITECTURE_OVERVIEW.mdsrc/common/components/atoms/editable-text.tsxsrc/common/components/atoms/reorderable-tab.tsxsrc/common/components/organisms/Navigation.tsxsrc/common/components/organisms/NavigationManagement.tsxsrc/common/components/organisms/Sidebar.tsxsrc/common/components/organisms/TabBar.tsxsrc/common/components/organisms/navigation/ErrorBoundary.tsxsrc/common/components/organisms/navigation/NavigationEditor.tsxsrc/common/components/organisms/navigation/NavigationItem.tsxsrc/common/components/organisms/navigation/constants.tssrc/common/components/organisms/navigation/useNavigation.tssrc/common/data/stores/app/index.tsxsrc/common/data/stores/app/navigation/navigationStore.tssrc/common/data/stores/app/space/spaceStore.tssrc/common/utils/layoutFormatUtils.tssrc/common/utils/navUtils.tssrc/common/utils/tabUtils.tssrc/config/loaders/runtimeLoader.tssrc/pages/api/navigation/config.tstests/navigation/errorHandling.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/common/components/organisms/NavigationManagement.tsx
- src/common/components/organisms/Sidebar.tsx
- src/common/data/stores/app/index.tsx
🧰 Additional context used
🧬 Code graph analysis (11)
src/common/components/organisms/navigation/ErrorBoundary.tsx (1)
src/common/components/atoms/button.tsx (1)
Button(81-81)
src/common/components/organisms/TabBar.tsx (1)
src/common/utils/tabUtils.ts (1)
generateUniqueName(44-58)
src/common/data/stores/app/space/spaceStore.ts (4)
src/common/data/database/supabase/clients/api.ts (2)
set(14-16)get(11-13)src/common/lib/signedFiles.ts (1)
SignedFile(16-18)src/pages/api/space/registry/[spaceId]/index.ts (1)
UpdateTabOrderRequest(30-30)src/pages/api/space/homebase/tabOrder.ts (1)
UpdateTabOrderRequest(15-15)
src/common/components/organisms/navigation/useNavigation.ts (2)
src/common/components/organisms/navigation/NavigationItem.tsx (1)
NavigationItem(139-153)src/common/components/organisms/navigation/constants.ts (1)
NAVIGATION_REORDER_DEBOUNCE_MS(14-14)
src/pages/api/navigation/config.ts (7)
src/config/systemConfig.ts (1)
NavigationConfig(155-160)src/common/lib/signedFiles.ts (3)
Signable(20-23)isSignable(29-41)validateSignable(43-52)src/common/data/api/requestHandler.ts (2)
NounspaceResponse(9-16)requestHandler(31-51)src/common/data/database/supabase/clients/server.ts (1)
createSupabaseServerClient(7-7)src/common/utils/navUtils.ts (2)
validateNavItemLabel(19-35)validateNavItemHref(42-72)src/common/types/spaceData.ts (1)
SPACE_TYPES(7-13)src/supabase/database.d.ts (1)
Json(3-9)
src/common/utils/navUtils.ts (3)
src/common/components/organisms/navigation/constants.ts (2)
NAVIGATION_MAX_LABEL_LENGTH(23-23)NAVIGATION_MAX_UNIQUE_NAME_ITERATIONS(32-32)src/common/components/organisms/navigation/NavigationItem.tsx (1)
NavigationItem(139-153)src/common/utils/tabUtils.ts (2)
isDuplicateTabName(25-34)generateUniqueName(44-58)
src/common/components/organisms/navigation/NavigationItem.tsx (4)
src/common/lib/hooks/useUIColors.ts (1)
useUIColors(14-28)src/common/components/atoms/badge.tsx (1)
Badge(40-40)src/fidgets/ui/Links.tsx (1)
Link(27-32)src/common/lib/utils/mergeClasses.ts (1)
mergeClasses(4-6)
src/common/components/atoms/reorderable-tab.tsx (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
renameTab(375-497)
src/common/components/organisms/Navigation.tsx (5)
src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(55-57)src/common/lib/hooks/useCurrentSpaceIdentityPublicKey.ts (1)
useCurrentSpaceIdentityPublicKey(3-5)src/common/components/organisms/navigation/useNavigation.ts (1)
useNavigation(101-267)src/common/components/organisms/navigation/NavigationEditor.tsx (1)
NavigationEditor(365-365)src/common/components/organisms/navigation/NavigationItem.tsx (1)
NavigationButton(224-236)
src/common/components/atoms/editable-text.tsx (1)
src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/common/utils/layoutFormatUtils.ts (1)
src/common/fidgets/index.d.ts (3)
LayoutFidgetDetails(114-122)LayoutFidgetConfig(110-112)FidgetInstanceData(94-101)
🪛 LanguageTool
NAVIGATION_REFACTOR_PLAN.md
[style] ~6-~6: To elevate your writing, try using a synonym here.
Context: ... Issues: Too large, mixed concerns, hard to test/maintain ## Recommended Split ...
(HARD_TO)
🔇 Additional comments (33)
src/config/loaders/runtimeLoader.ts (1)
17-17: Verify environment variable is set correctly in all environments.The environment variable has been renamed from
SUPABASE_SERVICE_ROLE_KEYtoSUPABASE_SERVICE_KEY. Ensure this variable is properly configured in all deployment environments and that the fallback toNEXT_PUBLIC_SUPABASE_ANON_KEYis intentional for your use case.docs/GETTING_STARTED.md (1)
72-72: LGTM!The environment variable name has been correctly updated in the example configuration, aligning with the changes in the architecture documentation and runtime loader.
src/common/components/organisms/navigation/constants.ts (1)
1-32: LGTM! Well-documented constants file.Good practice to centralize magic numbers with clear documentation. The values are reasonable: 300ms debounce balances responsiveness with API efficiency, 50-char label limit is appropriate for navigation UI, and 100 iterations provides a safe upper bound.
src/common/data/stores/app/space/spaceStore.ts (3)
260-291: Good defensive initialization for new spaces.The initialization of
remoteSpaces[spaceId]before updating tabs handles the case where a new space hasn't been synced yet. The guard on line 281 correctly prevents deleting non-existent entries.One minor observation: the
tryblock starting at line 260 has inconsistent indentation - it appears to be missing a leading space compared to surrounding code.
607-617: Initialization ensures safe batch operations.Pre-initializing
remoteSpaces[spaceId]before the batch commit prevents undefined access errors. While this could theoretically be combined with the final syncset()call at line 651 for atomicity, the current approach is clearer and the initialization is idempotent.
827-838: Consistent 404 handling for tab order.Good addition that gracefully handles spaces without a committed tab order file, allowing local order to be used as fallback. This complements the similar handling in
loadSpaceTab.src/common/utils/navUtils.ts (5)
19-35: Label validation looks good with appropriate character allowlist.The regex
[^a-zA-Z0-9-_ $]correctly allows alphanumeric, hyphens, underscores, spaces, and$(for token symbols). Length validation uses the centralized constant.
82-90: Good code reuse with tab utilities.Clean delegation to
isDuplicateTabNamewith appropriate data transformation.
99-108: Case-sensitive href comparison is correct.URLs and paths should be compared case-sensitively, unlike labels. Implementation is straightforward and correct.
118-154: Well-implemented slug generation with appropriate fallbacks.The slugification logic correctly:
- Lowercases and replaces non-alphanumeric characters with hyphens
- Trims leading/trailing hyphens
- Falls back to "item" for empty slugs
- Uses timestamp fallback if max iterations exceeded (edge case safety)
164-179: Consistent code reuse with appropriate fallback.Good delegation to
generateUniqueNamewith a timestamp-based fallback matching the pattern ingenerateUniqueHrefFromLabel.src/common/utils/layoutFormatUtils.ts (4)
7-11: Good undefined-safe implementation.The function correctly handles undefined input by returning
false, maintaining backward compatibility while allowing callers to pass potentially undefined layout details.
19-33: Clean early return with sensible default.Returning
'grid'for undefined layout details is a reasonable fallback that maintains consistent behavior.
41-56: Appropriate fallback for undefined layouts.Returning all fidget keys when layout is undefined ensures fidgets are still displayed, just without a specific mobile ordering.
103-118: Consistent defensive handling with safe defaults.Both the undefined check (line 107-109) and the fallback for missing
layoutConfig(line 117) ensure callers always receive a valid object structure.src/common/components/atoms/editable-text.tsx (3)
12-24: Clean prop interface with optional validation.Good design allowing callers to provide custom validation while defaulting to tab name validation. The
maxLengthdefault of 22 should be extracted to a constant if it has semantic meaning.
28-33: Correct synchronization pattern.The effect properly syncs with
initialTextchanges only when not editing, preventing loss of user input during edits. Dependencies are correctly specified.
54-73: Good event handling on input.The
stopPropagationon click (lines 68-71) correctly prevents click events from bubbling to parent navigation elements, avoiding accidental navigation while editing.src/common/components/atoms/reorderable-tab.tsx (1)
85-91: LGTM!Good defensive check. This ensures
EditableTextonly renders whenrenameTabis actually provided, preventing potential undefined callback issues and aligning edit mode behavior with callback availability.src/common/components/organisms/navigation/ErrorBoundary.tsx (1)
22-112: Well-structured error boundary implementation.The error boundary correctly follows React patterns with proper state management, development-only error details, and a clean fallback UI. The TODO for error logging service integration is a good reminder for production readiness.
NAVIGATION_REFACTOR_PLAN.md (1)
1-307: Comprehensive refactoring plan.This is a well-structured design document that clearly outlines the decomposition strategy, migration phases, and expected benefits. The phased approach (hooks → sub-components → main split) with "quick wins first" prioritization is pragmatic.
One minor note: line number references (e.g., "Extract from: Lines 544-733") will become stale as the codebase evolves. Consider removing or updating these references as the refactor progresses.
src/common/components/organisms/navigation/useNavigation.ts (3)
137-143: LGTM - Well-designed initial load guard.The ref-based pattern correctly prevents re-loading after commits while still loading on initial mount when the store is empty. The comment clearly explains the intent.
152-164: LGTM - Proper debounce with cleanup.The debounced reorder handler is correctly memoized and cleaned up on unmount, preventing memory leaks and stale updates.
258-266: LGTM - Clean return interface.The return value properly exposes navigation state and handlers. The
hasUncommittedChangesis correctly evaluated at render time (since it's a store selector function).src/common/components/organisms/navigation/NavigationEditor.tsx (1)
302-358: LGTM - System items display.Showing system navigation items (Notifications, Search, My Space, Login/Logout) as disabled during edit mode provides good visual context while preventing accidental interaction. The conditional rendering based on
isLoggedInandisInitializingis correct.src/common/components/organisms/Navigation.tsx (1)
355-388: LGTM - Good error boundary integration.The
NavigationErrorBoundarycorrectly wrapsNavigationEditorto isolate editing-related errors. TheonErrorcallback with production logging placeholder is a good practice.src/common/components/organisms/navigation/NavigationItem.tsx (3)
55-79: LGTM! Well-structured client-side navigation with proper modifier key handling.The click handler correctly distinguishes between internal navigation (using
router.pushwithstartTransition) and external links. The modifier key detection (metaKey,ctrlKey, etc.) properly allows users to open links in new tabs.
139-153: Memo comparator omitsIconand function props, which may cause stale closures.The custom comparator ignores
Icon,onClick, andonNavigate. If these props change (e.g.,onClickcaptures different closure variables), the component won't re-render. WhileIconis typically stable, callback changes could lead to stale behavior.Consider whether function props need comparison or if they're guaranteed stable via
useCallbackin parent components.
224-236: NavigationButton memo comparator also omitsonClick, similar concern.Same potential issue as NavigationItem—if
onClickchanges identity and captures different state, the memoized component won't update.src/pages/api/navigation/config.ts (3)
176-224: Space deletion errors are logged but don't fail the request—verify this is intentional.When deleting spaces for removed navigation items, storage and registration deletion errors are caught and logged with
console.warnbut the request continues. This is a deliberate design choice (as noted in the comment), but could leave orphaned data if deletions repeatedly fail.Consider adding monitoring/alerting for these warnings or implementing a cleanup job for orphaned spaces.
233-235: JSON round-trip for type conversion is correct but adds overhead.The
JSON.parse(JSON.stringify(...))pattern ensures JSON compatibility but incurs serialization cost. For navigation configs (typically small), this is acceptable.
266-279: Good defensive check for race conditions.Verifying that rows were actually updated catches edge cases where the config was deleted or unpublished between fetch and update. The 409 status code is appropriate for this conflict scenario.
src/common/data/stores/app/navigation/navigationStore.ts (1)
75-82: LGTM! Clean initialization with proper filtering and deep cloning.The
loadNavigationfunction correctly filters out hardcoded items and usescloneDeepto prevent mutation of the source data.
| return state.localNavigation.some((localItem, index) => { | ||
| const remoteItem = state.remoteNavigation[index]; | ||
| return !remoteItem || | ||
| localItem.id !== remoteItem.id || | ||
| localItem.label !== remoteItem.label || | ||
| localItem.href !== remoteItem.href || | ||
| localItem.icon !== remoteItem.icon || | ||
| localItem.spaceId !== remoteItem.spaceId; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
hasUncommittedChanges comparison may fail for object-type icon property.
Line 271 uses strict equality (!==) for icon comparison. If icon is an object, reference comparison will fail even when values are equal. Consider deep comparison or JSON stringification for the icon field.
Suggested fix
return state.localNavigation.some((localItem, index) => {
const remoteItem = state.remoteNavigation[index];
return !remoteItem ||
localItem.id !== remoteItem.id ||
localItem.label !== remoteItem.label ||
localItem.href !== remoteItem.href ||
- localItem.icon !== remoteItem.icon ||
+ JSON.stringify(localItem.icon) !== JSON.stringify(remoteItem.icon) ||
localItem.spaceId !== remoteItem.spaceId;
});🤖 Prompt for AI Agents
In @src/common/data/stores/app/navigation/navigationStore.ts around lines 265 -
274, The hasUncommittedChanges check currently uses strict !== for
localItem.icon vs remoteItem.icon which will falsely flag differences when icon
is an object; update the comparison to perform a deep-equality check (e.g., use
a utility like lodash/isEqual or compare JSON.stringify(localItem.icon) ===
JSON.stringify(remoteItem.icon)) and replace the icon inequality test inside the
state.localNavigation.some callback accordingly, keeping all other field
comparisons the same and ensuring null/undefined icons are handled consistently.
| } catch (error: any) { | ||
| console.error("Failed to commit navigation changes:", error); | ||
| // Log full error response for debugging | ||
| if (error?.response) { | ||
| console.error("API error response:", { | ||
| status: error.response.status, | ||
| statusText: error.response.statusText, | ||
| data: error.response.data, | ||
| headers: error.response.headers, | ||
| }); | ||
| } else if (error?.message) { | ||
| console.error("Error message:", error.message); | ||
| } | ||
| console.error("Full error object:", JSON.stringify(error, null, 2)); | ||
| throw error; |
There was a problem hiding this comment.
Error logging exposes potentially sensitive data including headers.
Line 428 logs response headers which could include authentication tokens or other sensitive information. Consider limiting logged fields.
Suggested fix
} catch (error: any) {
console.error("Failed to commit navigation changes:", error);
// Log full error response for debugging
if (error?.response) {
console.error("API error response:", {
status: error.response.status,
statusText: error.response.statusText,
data: error.response.data,
- headers: error.response.headers,
});
} else if (error?.message) {
console.error("Error message:", error.message);
}
- console.error("Full error object:", JSON.stringify(error, null, 2));
throw error;
}🤖 Prompt for AI Agents
In @src/common/data/stores/app/navigation/navigationStore.ts around lines 420 -
434, The catch block in navigationStore.ts currently logs full
error.response.headers and serializes the entire error object which can expose
sensitive info; update the logging in that catch to omit or redact sensitive
header fields (e.g., Authorization, Cookie, Set-Cookie) and stop
JSON.stringify-ing the whole error — instead log only non-sensitive fields such
as error.response.status, error.response.statusText, and a truncated or
sanitized error.message (or a safe subset of error.response.data), or implement
a small sanitizer that removes header keys before logging; keep rethrowing the
original error unchanged.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @docs/README.md:
- Around line 36-42: "Customization" is under the wrong heading; move the
"Customization" list item so it is nested with the "Themes" section. Locate the
"Themes" heading and its list (the "Theme System" bullet) and reattach the
"Customization" bullet (currently listed under the "Navigation" heading)
directly beneath the "Themes" section; ensure the "Navigation" section only
contains its intended bullets (e.g., "Navigation System") and remove the
orphaned "Customization" entry from "Navigation".
In @docs/SYSTEMS/NAVIGATION/OVERVIEW.md:
- Around line 148-154: Update the documentation to match the code: in the
NavigationConfig declaration (logoTooltip) mark href as optional (e.g.,
logoTooltip?: { text: string; href?: string }) or otherwise note that href is
optional to match src/config/systemConfig.ts; ensure the docs refer to
logoTooltip (or LogoTooltipConfig if used) and show href?: string instead of
href: string.
In @src/common/components/organisms/Navigation.tsx:
- Around line 298-333: iconFor currently creates new functional components
(ReactIconWrapper/CustomIconWrapper) on each call causing remounts; change
iconFor to return stable component references by caching wrapper components per
key: create a useRef<Map<string, React.FC>> or a useMemo-generated lookup object
keyed by icon key that stores and reuses wrapper components for iconPack entries
and URL icons, and for URL icons use a single reusable ImgIcon component factory
that reads src via props or returns a memoized wrapper for that src; update
iconFor to check the cache first and only create/store a new wrapper if one
doesn't exist (still fall back to HomeIcon), keeping the existing symbol names
iconFor, iconPack, ReactIconWrapper/CustomIconWrapper and HomeIcon for clarity.
In @src/common/components/organisms/navigation/NavigationItem.tsx:
- Around line 92-102: Remove the duplicate e.preventDefault() and stop casting
the keyboard event to a MouseEvent; instead make handleClick accept a union
event type (e.g., React.MouseEvent<HTMLAnchorElement> |
React.KeyboardEvent<HTMLAnchorElement>) and update its internals to guard any
mouse-only properties (use type guards or checks like 'button' in event and
optional chaining for metaKey/ctrlKey) before accessing them, then call
handleClick(e) directly from the onKeyDown handler when Enter or Space is
pressed.
In @src/common/data/stores/app/navigation/navigationStore.ts:
- Line 596: The current console.error call uses JSON.stringify(error, null, 2)
which can throw on circular structures (common with Axios errors); replace it
with a safe serialization approach: either use Node's util.inspect(error, {
depth: null }) or wrap JSON.stringify in try/catch and fall back to
error.toString() or logging specific fields (error.message, error.stack,
error.response?.data) so the logging line that references error (the
console.error("Full error object:", JSON.stringify(error, null, 2)) statement)
never throws and always outputs useful error details.
- Around line 195-209: deleteNavigationItem currently removes the entry from
navigation.localNavigation but doesn't remove its associated local space,
causing buildup; modify deleteNavigationItem so it first finds the item (using
get().navigation.localNavigation.find) to obtain item.spaceId, then inside the
set(draft => { ... }, "deleteNavigationItem") block delete the corresponding
entry from draft.space.localSpaces (e.g., if (item?.spaceId) delete
draft.space.localSpaces[item.spaceId]) before or after filtering
draft.navigation.localNavigation; ensure you guard for missing item/spaceId to
avoid exceptions and keep the existing debug logs.
- Around line 337-345: The icon field is being compared by reference in
hasUncommittedChanges (state.localNavigation / localItem.icon vs
state.remoteNavigation / remoteItem.icon), which yields false positives for
equivalent icon objects; replace the reference check localItem.icon !==
remoteItem.icon with a deep equality check (e.g., use a stable deep-equal
utility like lodash/isEqual or a project deepEqual helper) and guard for
null/undefined so two absent icons are considered equal, keeping the rest of the
property comparisons unchanged.
- Around line 156-178: The two separate set calls in createNavigationItem risk
leaving draft.space.localSpaces[spaceId] orphaned if the subsequent set adding
the navigation item fails; combine the updates into a single set call so both
the new local space and its corresponding navigation item are added atomically:
inside the same set(draft => { ... }, "...") update both
draft.space.localSpaces[spaceId] (with id, updatedAt, tabs, order, changedNames,
deletedTabs) and the draft.navigation/whatever structure where the navigation
item is created (the code that was in the second set call), ensuring both
mutations occur together to avoid inconsistent state.
- Around line 565-568: The axiosBackend.put call in navigationStore that sends
signedRequest should validate the returned response body before treating the
update as successful: capture the result of await
axiosBackend.put("/api/navigation/config", signedRequest), check response.data
for an error (e.g., response.data?.error?.message) or an explicit success flag,
and if an error is present throw or handle it the same way as the pattern used
at the other call (line ~484) so the store does not update state on 2xx error
payloads; update the console log block around navigationStore to use the
validated response and only proceed when the payload indicates success.
In @src/pages/api/navigation/config.ts:
- Around line 31-44: The type guard isUpdateNavigationConfigRequest currently
permits navigationConfig.items to be undefined while the NavigationConfig
interface demands items: NavigationItem[]; either tighten the guard to require
navigationConfig.items be an array (remove the allowance for undefined in the
check of thing["navigationConfig"]["items"]) so only payloads with items pass,
or if undefined is acceptable make NavigationConfig.items optional (items?:
NavigationItem[]) and update the UpdateNavigationConfigRequest type accordingly;
locate and change the check inside isUpdateNavigationConfigRequest or update the
NavigationConfig/UpdateNavigationConfigRequest type definitions to keep them
consistent.
- Around line 276-284: The multi-step operation that calls storage deletions and
space-registration deletions before updating the community config (the supabase
.from("community_configs").update call that sets navigation_config and
updated_at) can leave the system inconsistent if the final update fails; modify
the flow to either wrap all DB changes and related cleanup in a single
transaction (use Supabase/Postgres transaction via rpc/BEGIN...COMMIT or
server-side function) or perform the config update first (the .update/select on
community_configs using updateRequest.communityId and is_published=true) and
confirm it succeeded before performing storage deletions and space registration
removals, or implement soft-delete flags for cleanup steps so failures are
reversible — update code paths that call storage deletion and space registration
deletion to run only after the update returns no updateError and updatedRows are
present, or move deletions into a transactional stored procedure if possible.
🧹 Nitpick comments (17)
src/app/page.tsx (1)
15-21: Simplify the href normalization logic.The ternary on lines 17-19 is correct but slightly verbose. The
startsWith('/')check keeps the href unchanged when true, which means you're just ensuring a leading slash exists.♻️ Suggested simplification
if (firstNavItem?.href) { - // Remove leading slash if present and redirect - const href = firstNavItem.href.startsWith('/') - ? firstNavItem.href - : `/${firstNavItem.href}`; + // Ensure leading slash and redirect + const href = firstNavItem.href.startsWith('/') + ? firstNavItem.href + : `/${firstNavItem.href}`; redirect(href); }The logic is correct; the comment says "Remove leading slash" but the code actually ensures one exists. Consider updating the comment to match the behavior.
src/common/components/organisms/navigation/errorHandling.ts (2)
19-28: String-based error classification is fragile.The error type detection using
includes('Invalid'),includes('duplicate'),includes('admin'), andincludes('permission')could produce false positives if these strings appear in unrelated error messages (e.g., "Invalid token" being classified as VALIDATION when it might be an auth error).Consider using more specific patterns, error codes, or a dedicated error class hierarchy for more reliable classification.
♻️ Suggested approach
// Option 1: Use error codes or custom error classes class ValidationError extends Error { readonly code = 'VALIDATION_ERROR'; } // Option 2: More specific string patterns const VALIDATION_PATTERNS = [ /^Invalid\s+(href|label|name)/i, /duplicate\s+(href|name)/i, ]; const PERMISSION_PATTERNS = [ /not\s+an?\s+admin/i, /permission\s+denied/i, ];
30-38: Consider extending retryable error detection.The current logic only marks 5xx errors as retryable. Consider also handling:
- Timeout errors (
error.code === 'ECONNABORTED'orerror.code === 'ETIMEDOUT')- Rate limiting (429 status)
- Network connectivity issues (
error.code === 'ERR_NETWORK')♻️ Extended retryable detection
if (axios.isAxiosError(error)) { - const isRetryable = error.response?.status !== undefined && error.response.status >= 500; + const status = error.response?.status; + const isServerError = status !== undefined && status >= 500; + const isRateLimited = status === 429; + const isNetworkError = error.code === 'ERR_NETWORK' || error.code === 'ECONNABORTED'; + const isRetryable = isServerError || isRateLimited || isNetworkError; return { type: 'NETWORK', message: error.response?.data?.error?.message || error.message || 'Network error occurred', retryable: isRetryable, }; }src/common/components/atoms/editable-text.tsx (1)
25-26: Consider using camelCase for state setters.The state setter names
setisEditingandsettextdeviate from the conventional camelCase pattern (setIsEditing,setText). While functional, consistent naming improves readability.♻️ Suggested fix
- const [isEditing, setisEditing] = useState(false); - const [text, settext] = useState(initialText); + const [isEditing, setIsEditing] = useState(false); + const [text, setText] = useState(initialText);Then update all references to use the new names.
src/config/loaders/registry.ts (1)
129-135: Consider documenting or adjusting the cleanup threshold.The magic number
10for the cache size threshold before triggering cleanup is reasonable but arbitrary. A few observations:
- Cleanup only runs on writes, so a read-heavy workload could accumulate expired entries until the next write
- The threshold of 10 is low—consider whether this matches expected community count in production
This is fine for now given the short TTL (60s) and the fact that expired entries will be individually purged on read miss.
💡 Optional: Extract threshold to a named constant
const SYSTEM_CONFIG_CACHE_TTL_MS = 60_000; +const SYSTEM_CONFIG_CACHE_CLEANUP_THRESHOLD = 10; // ... function writeSystemConfigCache(communityId: string, value: SystemConfig | null) { - if (SYSTEM_CONFIG_CACHE.size > 10) { + if (SYSTEM_CONFIG_CACHE.size > SYSTEM_CONFIG_CACHE_CLEANUP_THRESHOLD) { cleanupExpiredCacheEntries(); }src/common/components/organisms/navigation/useNavigation.ts (1)
65-66: Unused store selectorsdeleteNavigationItemandrenameNavigationItem.These selectors are destructured from the store but never used within this hook. They should be removed to avoid confusion.
♻️ Suggested fix
const { loadNavigation, localNavigation, hasUncommittedChanges, createNavigationItem, - deleteNavigationItem, - renameNavigationItem, updateNavigationOrder, commitNavigationChanges, resetNavigationChanges, } = useAppStore((state) => ({ loadNavigation: state.navigation.loadNavigation, localNavigation: state.navigation.localNavigation, hasUncommittedChanges: state.navigation.hasUncommittedChanges, createNavigationItem: state.navigation.createNavigationItem, - deleteNavigationItem: state.navigation.deleteNavigationItem, - renameNavigationItem: state.navigation.renameNavigationItem, updateNavigationOrder: state.navigation.updateNavigationOrder, commitNavigationChanges: state.navigation.commitNavigationChanges, resetNavigationChanges: state.navigation.resetNavigationChanges, }));src/common/components/organisms/navigation/NavigationEditor.tsx (4)
103-111: Icon button refs are never cleaned up.The
iconButtonRefs.currentobject accumulates refs for each item but never removes them when items are deleted. This could lead to memory leaks over time if many items are created and deleted.♻️ Suggested cleanup approach
Consider cleaning up refs when items are removed, or use a WeakMap-like pattern. Alternatively, since refs are lightweight and items are typically few, this may be acceptable for this use case.
+ // Clean up stale refs when editableItems changes + useEffect(() => { + const currentIds = new Set(editableItems.map(item => item.id)); + Object.keys(iconButtonRefs.current).forEach(id => { + if (!currentIds.has(id)) { + delete iconButtonRefs.current[id]; + } + }); + }, [editableItems]);
151-166: Error re-throwing after toast could cause double error display.When
onRenamethrows, the code shows a toast error (line 160) and then re-throws (lines 163-165). If an error boundary catches this, it may show another error UI, resulting in duplicate error feedback to the user.♻️ Consider not re-throwing after showing toast
toast.error(errorMessage); - - // Re-throw for error boundaries - if (error instanceof Error) { - throw error; - } + // Error already handled with toast - no need to re-throwOr, if error boundary handling is needed, don't show the toast here and let the boundary handle it.
178-179: Orphaned JSDoc comment block.Lines 169-178 contain an orphaned JSDoc comment that describes an effect for URL updates, but no such effect exists in the code. This appears to be leftover documentation from removed code.
♻️ Remove orphaned comment
- /** - * Updates URL when the currently viewed navigation item's href changes - * - * This effect watches for changes in localNavigation and updates the browser URL - * if the current pathname matches an item that has been renamed. This ensures - * the URL stays in sync with navigation item changes without requiring a page reload. - * - * Uses a ref to track the previous pathname to avoid unnecessary updates and - * prevent race conditions where the effect might run before the store has fully updated. - */ /** * Handles renaming a navigation item
371-401: Delete button shows success toast before deletion completes.The delete button calls
onDelete(item.id)and immediately showstoast.success("Navigation item deleted"). IfonDeleteis asynchronous or throws, the success toast would be misleading.♻️ Suggested fix
Move the toast to after the delete operation, or handle it in the parent component that owns the delete logic.
onClick={(e) => { e.preventDefault(); e.stopPropagation(); console.log('[NavigationEditor] Deleting navigation item:', { itemId: item.id, label: item.label, href: item.href }); onDelete(item.id); - toast.success("Navigation item deleted"); }}Note: The toast is also shown in
Navigation.tsx'shandleDeleteItem, so removing it here would be appropriate to avoid duplication.src/common/components/organisms/Navigation.tsx (2)
500-500: Redundant ternary forusernameprop.Line 500 has
username={username ? username : undefined}which is equivalent to justusername={username}sinceusernameis already typed asstring | undefined(from line 276).♻️ Simplify
- username={username ? username : undefined} + username={username}
597-612: Edit Nav button creates inline Icon component.Line 601-605 creates an inline arrow function component for the icon, which will cause React to treat it as a new component on each render. This could cause unnecessary re-renders of the
NavigationButton.♻️ Extract to a named component
+const EditNavIcon: React.FC = () => ( + <div className="w-6 h-6 flex items-center justify-center"> + <FaPencil className="w-4 h-4 text-gray-800 dark:text-white" /> + </div> +); // In the render: {isNavigationEditable && !mobile && ( <NavigationButton label={navEditMode ? "Exit Edit" : "Edit Nav"} - Icon={navEditMode ? LogoutIcon : () => ( - <div className="w-6 h-6 flex items-center justify-center"> - <FaPencil className="w-4 h-4 text-gray-800 dark:text-white" /> - </div> - )} + Icon={navEditMode ? LogoutIcon : EditNavIcon}src/pages/api/navigation/config.ts (1)
82-92: Sensitive data logged in production.The debug logging on lines 83-92 logs navigation items including
spaceIdand potentially other data. While this is helpful for debugging, ensure these logs are appropriate for production environments or consider gating them behind a development check.♻️ Gate debug logging
- // Debug: log what we received - console.log('[API] Received navigation items:', { - communityId: updateRequest.communityId, - itemCount: updateRequest.navigationConfig.items?.length || 0, - items: updateRequest.navigationConfig.items?.map((item: NavigationItem) => ({ - id: item.id, - label: item.label, - href: item.href, - spaceId: item.spaceId - })) - }); + if (process.env.NODE_ENV === 'development') { + console.log('[API] Received navigation items:', { + communityId: updateRequest.communityId, + itemCount: updateRequest.navigationConfig.items?.length || 0, + items: updateRequest.navigationConfig.items?.map((item: NavigationItem) => ({ + id: item.id, + label: item.label, + href: item.href, + spaceId: item.spaceId + })) + }); + }src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (1)
72-126: Memoization dependency array includesprovidedTabNamebut only affects local branch.The
spaceDatauseMemo includesprovidedTabNamein its dependency array, butprovidedTabNameis only used inside theif (localNavItem && localNavItem.spaceId)branch to computeactiveTab. When server data is used (lines 112-114), the memoization will unnecessarily recompute even though the result is the sameserverSpaceDatareference.This is a minor inefficiency but acceptable given the complexity of refactoring. Consider extracting the local computation to avoid recomputing when only
providedTabNamechanges and server data is in use.src/common/data/stores/app/navigation/navigationStore.ts (3)
445-494: No retry logic for transient failures during space registration.The space registration loop fails the entire commit on the first error. For transient network issues, this could be frustrating for users who have made many changes. Consider adding retry logic with exponential backoff for network errors, while still failing fast on validation errors.
#!/bin/bash # Check if there's a retry utility already available in the codebase rg -n "retry" --type ts -l | head -10
453-453: Redundant non-null assertion after explicit check.Line 453 uses
registrationResponse.value!.spaceIdbut line 449 already checks!registrationResponse.value?.spaceId. After the check, TypeScript should knowvalueis defined. The!is unnecessary and could mask future type issues.♻️ Suggested fix
-const actualSpaceId = registrationResponse.value!.spaceId; +const actualSpaceId = registrationResponse.value.spaceId;
501-526: Multiple individualsetcalls in a loop may cause excessive re-renders.The loop at lines 501-526 calls
setfor each successful registration. With many new items, this triggers multiple state updates. Consider batching these updates into a singlesetcall.♻️ Suggested fix: Batch state updates
-for (const registration of successfulRegistrations) { - set((draft) => { +set((draft) => { + for (const registration of successfulRegistrations) { const itemIndex = draft.navigation.localNavigation.findIndex( (item) => item.id === registration.itemId ); if (itemIndex !== -1) { draft.navigation.localNavigation[itemIndex].spaceId = registration.actualSpaceId; } // Ensure the spaceId is set correctly in localSpaces if (draft.space.localSpaces[registration.spaceId]) { if (registration.spaceId !== registration.actualSpaceId) { // Move the localSpaces entry to the actual spaceId (shouldn't happen, but handle it) console.log('[navigationStore] Moving localSpace entry:', { from: registration.spaceId, to: registration.actualSpaceId }); draft.space.localSpaces[registration.actualSpaceId] = draft.space.localSpaces[registration.spaceId]; draft.space.localSpaces[registration.actualSpaceId].id = registration.actualSpaceId; delete draft.space.localSpaces[registration.spaceId]; } else { // Ensure the spaceId is set correctly draft.space.localSpaces[registration.actualSpaceId].id = registration.actualSpaceId; } } - }, "commitNavigationChanges-updateSpaceId"); -} + } +}, "commitNavigationChanges-updateSpaceIds");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
docs/DOCUMENTATION_OVERVIEW.mddocs/README.mddocs/SYSTEMS/CONFIGURATION/ARCHITECTURE_OVERVIEW.mddocs/SYSTEMS/NAVIGATION/OVERVIEW.mdscripts/seed.tssrc/app/[navSlug]/[[...tabName]]/NavPageSpace.tsxsrc/app/page.tsxsrc/common/components/atoms/editable-text.tsxsrc/common/components/atoms/reorderable-tab.tsxsrc/common/components/organisms/Navigation.tsxsrc/common/components/organisms/navigation/NavigationEditor.tsxsrc/common/components/organisms/navigation/NavigationItem.tsxsrc/common/components/organisms/navigation/errorHandling.tssrc/common/components/organisms/navigation/useNavigation.tssrc/common/data/stores/app/navigation/navigationStore.tssrc/common/lib/theme/ThemeSettingsEditor.tsxsrc/config/index.tssrc/config/loaders/registry.tssrc/config/systemConfig.tssrc/pages/api/navigation/config.tstests/navigation/errorHandling.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/navigation/errorHandling.test.ts
- docs/SYSTEMS/CONFIGURATION/ARCHITECTURE_OVERVIEW.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T21:13:36.931Z
Learnt from: willyogo
Repo: blankdotspace/space-system PR: 1641
File: src/fidgets/farcaster/components/CastRow.tsx:247-250
Timestamp: 2025-12-31T21:13:36.931Z
Learning: In src/fidgets/farcaster/components/CastRow.tsx (and similar UI components), hardcoded semi-transparent colors like `rgba(128, 128, 128, 0.2)` for borders and backgrounds are acceptable because they adapt to any background color, eliminating the need for theme tokens in ui_config for these specific use cases.
Applied to files:
src/common/components/organisms/Navigation.tsx
🧬 Code graph analysis (10)
src/common/lib/theme/ThemeSettingsEditor.tsx (1)
src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(55-57)
src/common/components/atoms/reorderable-tab.tsx (1)
src/common/data/stores/app/homebase/homebaseTabsStore.ts (1)
renameTab(375-497)
src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (4)
src/common/types/spaceData.ts (2)
NavPageSpacePageData(77-83)SPACE_TYPES(7-13)src/common/lib/hooks/useCurrentSpaceIdentityPublicKey.ts (1)
useCurrentSpaceIdentityPublicKey(3-5)src/common/data/stores/app/index.tsx (1)
useAppStore(174-174)src/app/(spaces)/PublicSpace.tsx (1)
PublicSpace(33-463)
src/pages/api/navigation/config.ts (7)
src/config/systemConfig.ts (1)
NavigationConfig(144-149)src/common/lib/signedFiles.ts (3)
Signable(20-23)isSignable(29-41)validateSignable(43-52)src/common/data/api/requestHandler.ts (2)
NounspaceResponse(9-16)requestHandler(31-51)src/common/data/database/supabase/clients/server.ts (1)
createSupabaseServerClient(7-7)src/common/utils/navUtils.ts (2)
validateNavItemLabel(19-35)validateNavItemHref(42-72)src/common/types/spaceData.ts (1)
SPACE_TYPES(7-13)src/supabase/database.d.ts (1)
Json(3-9)
src/common/components/organisms/Navigation.tsx (10)
src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(55-57)src/common/lib/hooks/useCurrentSpaceIdentityPublicKey.ts (1)
useCurrentSpaceIdentityPublicKey(3-5)src/common/lib/theme/index.d.ts (1)
UserTheme(7-23)src/common/components/organisms/navigation/useNavigation.ts (1)
useNavigation(41-318)src/common/data/stores/app/index.tsx (1)
useAppStore(174-174)src/config/systemConfig.ts (1)
NavigationItem(156-164)src/common/components/organisms/navigation/ErrorBoundary.tsx (1)
NavigationErrorBoundary(22-113)src/common/components/organisms/navigation/NavigationEditor.tsx (1)
NavigationEditor(494-494)src/common/lib/utils/analyticsUtils.ts (1)
trackAnalyticsEvent(5-12)src/common/components/atoms/button.tsx (1)
Button(81-81)
src/common/components/organisms/navigation/useNavigation.ts (5)
src/common/components/organisms/navigation/NavigationItem.tsx (1)
NavigationItem(146-160)src/config/systemConfig.ts (2)
NavigationItem(156-164)SystemConfig(27-37)src/common/components/organisms/navigation/constants.ts (1)
NAVIGATION_REORDER_DEBOUNCE_MS(14-14)src/config/loaders/registry.ts (1)
DEFAULT_COMMUNITY_ID(6-6)src/common/components/organisms/navigation/errorHandling.ts (2)
normalizeNavigationError(18-48)getUserFriendlyMessage(56-69)
src/common/components/atoms/editable-text.tsx (1)
src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/common/data/stores/app/navigation/navigationStore.ts (2)
src/common/types/spaceData.ts (1)
SPACE_TYPES(7-13)src/common/utils/navUtils.ts (6)
validateNavItemLabel(19-35)isDuplicateNavLabel(82-90)generateUniqueNavLabel(164-179)validateNavItemHref(42-72)isDuplicateNavHref(99-108)generateUniqueHrefFromLabel(118-154)
src/config/loaders/registry.ts (1)
src/config/systemConfig.ts (1)
SystemConfig(27-37)
src/common/components/organisms/navigation/NavigationEditor.tsx (5)
src/common/components/organisms/navigation/NavigationItem.tsx (1)
NavigationItem(146-160)src/common/lib/utils/mergeClasses.ts (1)
mergeClasses(4-6)src/common/utils/navUtils.ts (1)
validateNavItemLabel(19-35)src/common/components/organisms/navigation/constants.ts (1)
NAVIGATION_MAX_LABEL_LENGTH(23-23)src/common/lib/utils/analyticsUtils.ts (1)
trackAnalyticsEvent(5-12)
🔇 Additional comments (33)
src/common/components/atoms/reorderable-tab.tsx (1)
76-82: LGTM! Good defensive check for optionalrenameTabprop.Adding
&& renameTabto the condition ensuresEditableTextis only rendered when the callback is actually provided, preventing potential runtime errors when the component is used without rename functionality.src/common/lib/theme/ThemeSettingsEditor.tsx (3)
31-32: LGTM!The imports are correctly added and both are used in the component.
434-452: LGTM!The conditional rendering correctly handles the nav edit mode case:
- Theme changes are saved locally (staged) without committing.
- User returns to navigation edit mode where they can commit or cancel all changes together.
This aligns well with the PR objective of staging space edits locally during nav edit mode.
189-191: Verify that this component is always rendered withinSidebarContext.Provider.
useSidebarContext()directly returnsuseContext(SidebarContext)without a default fallback. IfThemeSettingsEditoris ever rendered outside the sidebar context provider,setEditModewill be undefined and calling it at line 443 will throw a runtime error.Consider adding a guard or ensuring the context provides safe defaults.
#!/bin/bash # Check where ThemeSettingsEditor is rendered and if it's within SidebarContext rg -n "ThemeSettingsEditor" --type=tsx --type=ts -C 3src/app/page.tsx (1)
23-26: LGTM on the fallback redirect logic.The fallback to
/homewhen no navigation items exist is a sensible default. Thereturn nullafterredirect()is fine for TypeScript inference even though it's technically unreachable.docs/DOCUMENTATION_OVERVIEW.md (1)
31-32: LGTM!The navigation system documentation entries are consistent with the existing documentation structure and properly reference the new
SYSTEMS/NAVIGATION/OVERVIEW.mdfile introduced by this PR.Also applies to: 74-74
src/common/components/organisms/navigation/errorHandling.ts (1)
56-68: LGTM on getUserFriendlyMessage.The function exhaustively handles all discriminated union cases, and TypeScript will catch any missing cases. The messaging is appropriate for user display.
src/common/components/atoms/editable-text.tsx (3)
28-33: LGTM on the sync effect.The effect correctly synchronizes the internal
textstate withinitialTextonly when not editing, preventing external updates from interrupting user input.
35-52: LGTM on validation and edit completion logic.The validation flow is well-structured:
- Trims input before validation
- Falls back to
validateTabNamewhen no custom validator is provided- Resets text on validation failure without exiting edit mode
- Only calls
updateMethodon successful validation
68-76: Good event propagation handling for edit mode.The
onClickandonDoubleClickhandlers withstopPropagation()andpreventDefault()correctly prevent navigation and text selection interference while the input is active.scripts/seed.ts (1)
20-20: Environment variable rename is complete and consistent across the codebase.The rename from
SUPABASE_SERVICE_ROLE_KEYtoSUPABASE_SERVICE_KEYhas been properly applied in all relevant locations:
- Source code (
src/common/data/database/supabase/clients/server.ts)- Tests (
tests/setup.ts)- Scripts (
scripts/seed.ts- lines 20, 64, 70)- Environment template (
.env.example)- Documentation (
docs/GETTING_STARTED.md,docs/SYSTEMS/CONFIGURATION/ARCHITECTURE_OVERVIEW.md)No occurrences of the old variable name remain.
src/config/index.ts (1)
72-76: LGTM! Log level change is appropriate.Downgrading from
console.errortoconsole.warnis correct for these fallback paths. Since the system gracefully recovers by usingDEFAULT_COMMUNITY_ID, these are expected scenarios rather than true errors. This reduces noise in error monitoring while still maintaining visibility.Also applies to: 88-94
src/config/systemConfig.ts (2)
35-37: LGTM! AddingcommunityIdto SystemConfig.This field is necessary for the navigation store to correctly scope API calls to the appropriate community. The value is populated during config loading in
transformRowToSystemConfig.
160-160: Acceptable trade-off: Widening icon type from union to string.Changing from a constrained union type to a generic
stringreduces type safety but is necessary to support dynamic react-icons names (e.g.,'FaHouse','FaRss') and custom icon URLs. The comment adequately documents the expected values.Consider adding runtime validation in the icon rendering component to handle invalid icon names gracefully.
docs/SYSTEMS/NAVIGATION/OVERVIEW.md (1)
1-302: Well-structured documentation covering the navigation system.The documentation provides a clear overview of the navigation editor architecture including data flow, components, API contracts, and security considerations. The local-first pattern explanation and memory management notes are particularly helpful for maintainers.
src/config/loaders/registry.ts (3)
83-95: LGTM! Signature change to includecommunityId.The updated signature correctly propagates
communityIdinto the returnedSystemConfig, aligning with the interface definition insystemConfig.ts. This enables downstream code (navigation store, API calls) to access the community context.
97-108: Cache cleanup implementation looks good.The cleanup function correctly iterates and removes expired entries. The O(n) iteration is acceptable given the small cache size (capped implicitly by the trigger threshold).
251-252: LGTM! Call site correctly passescommunityId.The call to
transformRowToSystemConfig(data, communityId)properly supplies the community ID that was used for the database query, ensuring consistency.src/common/components/organisms/navigation/NavigationItem.tsx (2)
146-160: Memo comparator omitsIconandonClick/onNavigateprops.The custom memo comparator doesn't check
Icon,onClick, oronNavigateprops. If these change (e.g., different icon component or callback identity), the component won't re-render. This could lead to stale behavior. Consider if these omissions are intentional for performance.
231-243: Memo comparator omitsonClickprop.Similar to
NavigationItem, theNavigationButtonmemo comparator doesn't includeonClick. If the callback identity changes, the button won't re-render with the new handler.src/common/components/organisms/navigation/useNavigation.ts (2)
126-145: Potential race condition in cancel navigation effect.If the user triggers multiple cancel operations quickly, or if the navigation takes longer than expected, the
pendingCancelTargetRefcould be overwritten. Additionally, if the target URL changes during navigation (e.g., due to redirects), the conditionpathname === pendingCancelTargetRef.currentmight never match, leaving the cancel operation incomplete.Consider adding a timeout fallback or tracking navigation state more robustly.
308-317: This approach is correct and does not create stale values.The
hasUncommittedChanges()function reads current state viaget().navigationon every call, returning a fresh boolean at the moment the hook renders. Since the selector includeslocalNavigation(line 62), the component re-renders wheneverlocalNavigationchanges—which is the primary condition affecting the result ofhasUncommittedChanges(). Each render calls the function with up-to-date state, so the value is not stale.src/common/components/organisms/navigation/NavigationEditor.tsx (2)
94-99: Large icon pack merged on every render's memoization.The
iconPackmemo has an empty dependency array, which is correct for stability. However, merging three large icon libraries (FaIcons,BsIcons,GiIcons) creates a substantial object. This is fine since it's memoized, but be aware this increases bundle size significantly.
239-241:router.replace(newUrl)with potentially nullnewUrl.The variable
newUrlis typed asstring | null(line 223), but TypeScript should catch this since theif (newUrl)check on line 234 guards the usage. However, thestartTransitioncallback capturesnewUrlin a closure, and TypeScript's type narrowing doesn't persist into the callback. This should work at runtime but could be cleaner.src/common/components/organisms/Navigation.tsx (1)
485-519: NavigationEditor error boundaryonErrorhas a TODO for production logging.The error boundary's
onErrorcallback has a TODO comment for integrating with an error logging service. This is fine for now, but ensure this is tracked for follow-up.src/pages/api/navigation/config.ts (2)
191-249: Space deletion failures are silently logged but don't affect response.When deleting spaces for removed navigation items, failures are logged with
console.warnbut the operation continues. This is intentional (as noted in the comment on line 245-246), but there's no way for the client to know if cleanup partially failed. Consider returning warnings in the response or implementing a cleanup job.
269-271: JSON serialization round-trip for type compatibility.The
JSON.parse(JSON.stringify(...))pattern on line 271 is used to ensure JSON compatibility with Supabase'sJsontype. While this works, it has performance overhead and could lose certain JavaScript values (likeundefinedbecomingnull). This is acceptable here since navigation config should be JSON-safe.src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (4)
88-92: Potential edge case: emptyorderarray fallback.When
localSpace.orderis truthy but empty ([]), the conditionlocalSpace.order && localSpace.order.length > 0correctly falls back to"Home". However, this hardcoded fallback should align with the store's initial tab name.Based on navigationStore.ts (line 174), the initial default tab is also
"Home", so this is consistent.
147-160: 404 UI implementation is appropriate for client components.The comment correctly notes that
notFound()cannot be used in client components. The inline 404 UI is a reasonable fallback. Consider extracting this to a reusable component if similar patterns exist elsewhere.
136-136: Tab name encoding is correctly applied.Using
encodeURIComponent(tabName)for the URL path ensures special characters in tab names are properly escaped.
96-107: Local space data construction is intentional for uncommitted changes.When a local navigation item is found, space data is reconstructed from local stores with
spaceOwnerFid: undefinedandconfig: INITIAL_SPACE_CONFIG_EMPTY. This behavior is by design—the code prioritizes local (uncommitted) changes over remote data to ensure users see their edits. For existing server-side items being edited locally, this reconstruction is the expected behavior. If these defaults are problematic for the edit flow, consider preservingspaceOwnerFidandconfigfrom the original server data when available.src/common/data/stores/app/navigation/navigationStore.ts (2)
1-19: Imports are well-organized and appropriate.The imports cover necessary utilities, types, and external libraries. Using named imports from lodash (
cloneDeep) is good for tree-shaking.
84-94: This concern is not applicable. The code already protects against overwriting local changes through dual guards inuseNavigation.ts(line 84): the guard checks both!hasLoadedNavigationRef.currentandlocalNavigation.length === 0, ensuringloadNavigationruns only once on initial mount when the store is empty. The function cannot overwrite uncommitted edits.Likely an incorrect or invalid review comment.
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| if (e.key === " ") { | ||
| // Prevent page scroll on Space | ||
| e.preventDefault(); | ||
| } | ||
| handleClick(e as unknown as React.MouseEvent<HTMLAnchorElement>); | ||
| } | ||
| }, | ||
| [disable, handleClick] | ||
| ); |
There was a problem hiding this comment.
Redundant e.preventDefault() call and unsafe type cast.
Line 96 calls e.preventDefault() again after it was already called on line 93. Additionally, the type cast on line 98 (as unknown as React.MouseEvent) is unsafe and could cause issues since keyboard events don't have properties like button, metaKey, etc. that handleClick checks.
🛠️ Suggested fix
const handleKeyDown = useCallback(
(e: React.KeyboardEvent<HTMLAnchorElement>) => {
if (disable) {
e.preventDefault();
return;
}
// Enter or Space key activates the link
if (e.key === "Enter" || e.key === " ") {
e.preventDefault();
- if (e.key === " ") {
- // Prevent page scroll on Space
- e.preventDefault();
- }
- handleClick(e as unknown as React.MouseEvent<HTMLAnchorElement>);
+ if (!openInNewTab && href && href.startsWith("/")) {
+ router.push(href);
+ React.startTransition(() => {
+ onClick?.();
+ onNavigate?.();
+ });
+ } else {
+ onClick?.();
+ onNavigate?.();
+ }
}
},
- [disable, handleClick]
+ [disable, href, openInNewTab, router, onClick, onNavigate]
);🤖 Prompt for AI Agents
In @src/common/components/organisms/navigation/NavigationItem.tsx around lines
92 - 102, Remove the duplicate e.preventDefault() and stop casting the keyboard
event to a MouseEvent; instead make handleClick accept a union event type (e.g.,
React.MouseEvent<HTMLAnchorElement> | React.KeyboardEvent<HTMLAnchorElement>)
and update its internals to guard any mouse-only properties (use type guards or
checks like 'button' in event and optional chaining for metaKey/ctrlKey) before
accessing them, then call handleClick(e) directly from the onKeyDown handler
when Enter or Space is pressed.
| return state.localNavigation.some((localItem, index) => { | ||
| const remoteItem = state.remoteNavigation[index]; | ||
| return !remoteItem || | ||
| localItem.id !== remoteItem.id || | ||
| localItem.label !== remoteItem.label || | ||
| localItem.href !== remoteItem.href || | ||
| localItem.icon !== remoteItem.icon || | ||
| localItem.spaceId !== remoteItem.spaceId; | ||
| }); |
There was a problem hiding this comment.
Icon comparison uses reference equality, which may cause false positives.
The hasUncommittedChanges function compares localItem.icon !== remoteItem.icon. If icon is an object (e.g., { type: 'react-icon', name: 'FaHome' }), this reference comparison will always return true even when the values are identical, causing phantom "uncommitted changes" detection.
🔧 Suggested fix: Use deep comparison for icon
+import { isEqual } from "lodash";
hasUncommittedChanges: () => {
const state = get().navigation;
// Compare arrays directly - check length, order, and content
if (state.localNavigation.length !== state.remoteNavigation.length) {
return true;
}
// Check if order changed or any item properties changed
return state.localNavigation.some((localItem, index) => {
const remoteItem = state.remoteNavigation[index];
return !remoteItem ||
localItem.id !== remoteItem.id ||
localItem.label !== remoteItem.label ||
localItem.href !== remoteItem.href ||
- localItem.icon !== remoteItem.icon ||
+ !isEqual(localItem.icon, remoteItem.icon) ||
localItem.spaceId !== remoteItem.spaceId;
});
},🤖 Prompt for AI Agents
In @src/common/data/stores/app/navigation/navigationStore.ts around lines 337 -
345, The icon field is being compared by reference in hasUncommittedChanges
(state.localNavigation / localItem.icon vs state.remoteNavigation /
remoteItem.icon), which yields false positives for equivalent icon objects;
replace the reference check localItem.icon !== remoteItem.icon with a deep
equality check (e.g., use a stable deep-equal utility like lodash/isEqual or a
project deepEqual helper) and guard for null/undefined so two absent icons are
considered equal, keeping the rest of the property comparisons unchanged.
|
|
||
| // Update navigation config in database | ||
| console.log('[navigationStore] Sending navigation config update request'); | ||
| await axiosBackend.put("/api/navigation/config", signedRequest); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the navigationStore.ts file directly
echo "=== Content at lines 565-568 ==="
sed -n '560,575p' src/common/data/stores/app/navigation/navigationStore.ts
echo -e "\n=== Search for axiosBackend definition and imports ==="
rg -n "import.*axiosBackend|export.*axiosBackend" src/common/data/stores/app/navigation/navigationStore.ts
echo -e "\n=== axiosBackend usage and context ==="
rg -n "axiosBackend" --type ts -B 2 -A 5 | head -80Repository: blankdotspace/space-system
Length of output: 6033
🏁 Script executed:
#!/bin/bash
# Find and read the axiosBackend configuration file
echo "=== Searching for axiosBackend definition ==="
find . -path ./node_modules -prune -o -type f -name "backend.ts" -o -name "backend.js" | grep -E "data/api"
echo -e "\n=== Reading the backend API configuration ==="
cat -n src/common/data/api/backend.ts 2>/dev/null || cat -n src/common/data/api/backend.js
echo -e "\n=== Context around the axiosBackend.put call in navigationStore.ts ==="
sed -n '555,580p' src/common/data/stores/app/navigation/navigationStore.tsRepository: blankdotspace/space-system
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Find the function containing the axiosBackend.put call
echo "=== Finding function context around line 573 ==="
sed -n '540,600p' src/common/data/stores/app/navigation/navigationStore.ts | cat -n
echo -e "\n=== Check for try-catch around this block ==="
sed -n '520,595p' src/common/data/stores/app/navigation/navigationStore.ts | cat -nRepository: blankdotspace/space-system
Length of output: 7004
🏁 Script executed:
#!/bin/bash
# Check if there are other axiosBackend.put calls that validate response
echo "=== Other axiosBackend.put calls to see response validation patterns ==="
rg -n "axiosBackend\.put" --type ts -A 3 -B 1
echo -e "\n=== Check if API endpoints validate responses elsewhere ==="
rg -n "validateStatus|response\.data|error.*response" src/common/data/stores/app/navigation/navigationStore.tsRepository: blankdotspace/space-system
Length of output: 1216
Add response data validation for navigation config PUT request.
The axiosBackend.put call does not validate the response data after a successful HTTP response. While the call is wrapped in a try-catch that handles HTTP errors, if the backend returns a 2xx status with an error payload in the response body, the update will be treated as successful and state will be updated. This is inconsistent with the pattern used elsewhere in the codebase (e.g., line 484) where response.data?.error?.message is validated. Consider validating the response to ensure the update actually succeeded on the server.
🤖 Prompt for AI Agents
In @src/common/data/stores/app/navigation/navigationStore.ts around lines 565 -
568, The axiosBackend.put call in navigationStore that sends signedRequest
should validate the returned response body before treating the update as
successful: capture the result of await
axiosBackend.put("/api/navigation/config", signedRequest), check response.data
for an error (e.g., response.data?.error?.message) or an explicit success flag,
and if an error is present throw or handle it the same way as the pattern used
at the other call (line ~484) so the store does not update state on 2xx error
payloads; update the console log block around navigationStore to use the
validated response and only proceed when the payload indicates success.
| } else if (error?.message) { | ||
| console.error("Error message:", error.message); | ||
| } | ||
| console.error("Full error object:", JSON.stringify(error, null, 2)); |
There was a problem hiding this comment.
Logging full error object with JSON.stringify may fail on circular references.
JSON.stringify(error, null, 2) will throw if the error object contains circular references, which is common with Axios errors. This could mask the original error.
🔧 Suggested fix: Use safe serialization
-console.error("Full error object:", JSON.stringify(error, null, 2));
+try {
+ console.error("Full error object:", JSON.stringify(error, Object.getOwnPropertyNames(error), 2));
+} catch {
+ console.error("Full error object (could not stringify):", error);
+}📝 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.
| console.error("Full error object:", JSON.stringify(error, null, 2)); | |
| try { | |
| console.error("Full error object:", JSON.stringify(error, Object.getOwnPropertyNames(error), 2)); | |
| } catch { | |
| console.error("Full error object (could not stringify):", error); | |
| } |
🤖 Prompt for AI Agents
In @src/common/data/stores/app/navigation/navigationStore.ts at line 596, The
current console.error call uses JSON.stringify(error, null, 2) which can throw
on circular structures (common with Axios errors); replace it with a safe
serialization approach: either use Node's util.inspect(error, { depth: null })
or wrap JSON.stringify in try/catch and fall back to error.toString() or logging
specific fields (error.message, error.stack, error.response?.data) so the
logging line that references error (the console.error("Full error object:",
JSON.stringify(error, null, 2)) statement) never throws and always outputs
useful error details.
Add visual feedback when committing navigation changes by showing a spinning spinner icon and "Committing..." text while the commit operation is in progress. Changes: - Add isCommitting state to useNavigation hook - Set loading state during commit operation (in finally block) - Pass isCommitting prop to NavigationEditor component - Update commit button to show FaSpinner with "Committing..." when loading - Disable button while committing to prevent multiple submissions - Add disabled styles for visual feedback This provides clear feedback to users that their commit is being processed and prevents accidental duplicate commits.
- Fix React hooks error in NavPageSpace: move all hooks before conditional returns to comply with Rules of Hooks - Add comprehensive logging throughout navigation codebase for debugging: - useNavigation: commit, cancel, create, reorder operations - navigationStore: load, create, delete, rename, updateOrder, commit operations - NavigationEditor: rename, delete operations - API endpoint: request validation, config fetch, update operations - Fix navigation cancel 404 flash: use usePathname to wait for navigation completion before resetting localNavigation, keeping uncommitted items available during navigation transition - Extract error handling to dedicated module (errorHandling.ts) for reuse
- Add useEffect to automatically expand sidebar when navEditMode becomes true - Hide sidebar toggle button during navigation edit mode (cleaner UX) - Prevent sidebar collapse while editing navigation to ensure all controls are visible
- Separate cancel button from commit button condition so it's always visible - Allow users to exit edit mode even when no changes have been made - Commit button only appears when there are uncommitted changes - Cancel button takes full width when commit button is hidden for better UX
- Integrate IconSelector component for navigation items (same system as mobile editor) - Update NavigationItem icon type to support react-icons names and custom URLs - Update iconFor function to handle react-icons names and custom icon URLs - Fix icon sizing in edit mode (w-5 h-5) to match standard nav visual size - Add back button to space customization when in nav edit mode - Space changes are staged locally when navigating back from space edit in nav edit mode - Changes are not committed until nav commits to maintain data consistency
…hanges - Add confirmation dialog before canceling navigation edits when changes exist - Skip confirmation dialog when no uncommitted changes (direct exit) - Show full-width 'Exit Edit Mode' button when no changes to save - Extract cancel handlers (handleCancelClick, handleConfirmedCancel) for cleaner code - Flatten conditional rendering for better readability - Reset confirmation state when exiting edit mode
- Add communityId to SystemConfig interface and pass through from database - Update useNavigation to use systemConfig.communityId with DEFAULT_COMMUNITY_ID fallback - Update React.memo comparisons in NavigationItem to use communityId instead of community?.type - Add cache cleanup to prevent memory leaks in SYSTEM_CONFIG_CACHE This fixes the 404 error when committing navigation changes (communityId vs community.type mismatch) and improves memory management by cleaning up expired cache entries.
- Update NavigationItem to highlight parent nav item when on tabs (e.g., /home/Settings highlights /home) - Change default community ID fallback from console.error to console.warn - Match highlighting logic between NavigationItem and NavigationEditor
- Add handleDeleteItem wrapper that checks if current pathname matches deleted item - If on deleted item, find closest nav item (previous, then next, then first) - Navigate to closest item after deletion to prevent 404 errors - Import NavigationItem type from systemConfig
- Add tab config timestamp checking in hasUncommittedChanges to detect space customizations - Always show X (cancel) and Save buttons in navigation edit mode - Refactor navigation config API to ensure atomic database updates - Compare local vs remote tab timestamps to detect uncommitted space changes
… cache - Add MAX_ICON_CACHE constant (50) to cap cache size - Evict oldest entries when cache exceeds max size using Map.keys().next().value - Clear icon cache on navigation/route changes via useEffect watching pathname - Prevents unbounded memory growth from icon wrapper cache
- Fix space tab 400 errors: Check for local tabs before fetching from storage to prevent errors for uncommitted navigation item tabs - Prevent config warning spam: Add domain warning cache to only warn once per domain per session - Add GlobalErrorHandler: Filter out known browser extension errors (PluginFactory, installHook) to reduce console noise - Remove planning/review documentation files for production readiness
- Guard all console.log statements with NODE_ENV checks - Debug logs now only appear in development mode - Production builds will not include debug logging - Preserves console.error and console.warn for error tracking
- Enhance type guard to validate each navigation item structure - Validates that items array elements have required string fields (id, label, href) - Prevents runtime errors from malformed navigation items - Add timestamp freshness validation to prevent replay attacks - Rejects timestamps older than 15 minutes - Allows up to 5 minutes clock skew tolerance - Validates timestamp format before processing
- Replace broad tab existence check with precise change detection - Only commit spaces with actual uncommitted changes (renamed tabs, deleted tabs, new tabs, or modified tabs) - Prevents unnecessary commits for spaces that just have tabs but no modifications - Mirrors the logic used in hasUncommittedChanges for consistency - Prevent render loop in Navigation component sidebar expansion - Use ref to track forced expansion per edit session - Only force sidebar expansion once when entering edit mode - Clear ref when exiting edit mode to prepare for next session - Prevents infinite loops if external code flips shrunk back to true
- Remove merge conflict markers left over from rebase - Add missing useUIColors import - Export NavIconBadge from NavigationItem.tsx for use in Navigation.tsx - Fix type names to match exported interfaces (NavigationItemProps, NavigationButtonProps) - Consolidate duplicate imports
- Remove unused 'ui' variable from systemConfig destructuring - Remove duplicate local NavItem component definition (using imported NavItemComponent instead) - Remove duplicate local NavButton component definition (using imported NavigationButton instead) - These were leftover from rebase when components were moved to separate files
- Remove NavigationItemProps, NavigationButtonProps, and NavIconBadge from imports - These were only needed for the duplicate local component definitions that were removed
There was a problem hiding this comment.
Actionable comments posted: 6
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/Navigation.tsx (1)
405-457: Components defined inside render cause remounts.
NavItemis defined inside theNavigationcomponent, meaning it's recreated on every render. This causes React to treat it as a new component type, unmounting and remounting the DOM elements. Move this outside the component or memoize it.🔧 Suggested approach
Move
NavItemoutside theNavigationcomponent:// Define outside Navigation component const NavItem: React.FC<NavigationItemProps & { shrunk: boolean; navTextStyle: React.CSSProperties; pathname: string | null; router: ReturnType<typeof useRouter>; onNavigate?: () => void; }> = React.memo(({ ... }) => { // implementation });Or use the already-imported
NavItemComponentfrom./navigation/NavigationItemwhich appears to be the extracted version of this pattern.
🤖 Fix all issues with AI agents
In `@src/common/components/organisms/Navigation.tsx`:
- Around line 387-390: The useEffect that clears iconWrapperCacheRef.current on
every pathname change is too aggressive; remove the entire useEffect block (the
one referencing useEffect(() => { iconWrapperCacheRef.current.clear(); },
[pathname])) so icons are not remounted on every navigation and rely on the
existing LRU eviction logic (MAX_ICON_CACHE and the eviction code around the
icon cache) to bound memory; if you prefer targeted cleanup instead, replace the
effect with logic that only removes stale entries from iconWrapperCacheRef based
on age rather than clearing the whole map.
- Around line 708-718: The NavItemComponent is constructing
href={`/s/${username}`} without guarding for a missing username (in
Navigation.tsx), which can produce /s/undefined; update the render logic around
NavItemComponent so when username is falsy you either disable the link (pass
href={undefined} or a disabled prop) or conditionally render a non-link variant,
and ensure trackAnalyticsEvent(AnalyticsEvent.CLICK_MY_SPACE) is only called
when a valid username exists; target the NavItemComponent invocation and the
username variable to implement this guard.
In `@src/common/components/organisms/navigation/NavigationEditor.tsx`:
- Around line 153-168: The catch block in NavigationEditor (around the icon
update logic referencing itemId and iconName) currently re-throws the caught
Error after logging and showing toast.error, which defeats the local graceful
handling; remove the "throw error" path so the handler only logs the error and
shows the toast (and optionally return/exit the function after the toast) to
prevent propagation to the error boundary while preserving the console.error and
toast.error behavior.
- Around line 470-479: The NavItemComponent is building an href with a possibly
undefined username which can produce "/s/undefined"; update the NavigationEditor
rendering logic so you only pass a valid href when username exists (e.g.,
conditionally build href using username or fallback to a safe route/empty
string) or render the NavItemComponent without the href and/or with disable=true
when username is missing; locate the NavItemComponent usage in
NavigationEditor.tsx (the block that sets href={`/s/${username}`}) and change it
to guard the href creation against undefined username and ensure
analytics/onClick behavior remains correct.
In `@src/common/components/organisms/NavigationManagement.tsx`:
- Around line 105-113: In NavigationManagement.tsx update the catch block in the
commit function to surface real error details instead of masking 404s: log
error.response?.status and error.response?.data (or error.message) in the
console and include them in the toast/error message, only treat 404 as a
local-only fallback behind a clear dev flag or by checking NODE_ENV, and ensure
onCommit is called only on true success or an explicit dev-fallback path;
reference the existing catch(error: any) block, onCommit(), and
toast.success/toast.error calls when making these changes.
- Around line 58-63: The debouncedUpdateOrder created with useCallback needs
cleanup to avoid stale invocations on unmount; replace useCallback with useMemo
to create debouncedUpdateOrder = useMemo(() => debounce((newOrder)=>
updateNavigationOrder(newOrder), 300), [updateNavigationOrder]) and add an
effect that returns a cleanup which calls debouncedUpdateOrder.cancel() (or
cancels the debounced function) to run on unmount so pending debounced calls are
cleared.
♻️ Duplicate comments (5)
docs/README.md (1)
36-42: Documentation structure issue remains: "Customization" entry is still misplaced.This issue was previously identified but has not been addressed. The "Customization" link (line 41) belongs under the "Themes" section but now appears under the "Navigation" heading. Please move it to be listed under "Themes" (after line 37) to maintain proper documentation organization.
📝 Suggested fix
### Themes - [Theme System](SYSTEMS/THEMES/OVERVIEW.md) - Theme architecture +- [Customization](SYSTEMS/THEMES/CUSTOMIZATION.md) - Theme customization ### Navigation - [Navigation System](SYSTEMS/NAVIGATION/OVERVIEW.md) - Navigation editor and management -- [Customization](SYSTEMS/THEMES/CUSTOMIZATION.md) - Theme customizationsrc/common/components/organisms/navigation/NavigationItem.tsx (1)
91-102: Redundante.preventDefault()and unsafe type cast remain.Line 96 calls
e.preventDefault()redundantly (already called on line 93). The type cast on line 98 (as unknown as React.MouseEvent) is unsafe because keyboard events lack mouse-specific properties likebuttonthathandleClickchecks on line 62.🔧 Suggested fix
const handleKeyDown = useCallback( (e: React.KeyboardEvent<HTMLAnchorElement>) => { if (disable) { e.preventDefault(); return; } // Enter or Space key activates the link if (e.key === "Enter" || e.key === " ") { e.preventDefault(); - if (e.key === " ") { - // Prevent page scroll on Space - e.preventDefault(); - } - handleClick(e as unknown as React.MouseEvent<HTMLAnchorElement>); + // Navigate directly instead of casting to MouseEvent + if (!openInNewTab && href && href.startsWith("/")) { + router.push(href); + React.startTransition(() => { + onClick?.(); + onNavigate?.(); + }); + } else { + onClick?.(); + onNavigate?.(); + } } }, - [disable, handleClick] + [disable, href, openInNewTab, router, onClick, onNavigate] );src/common/data/stores/app/navigation/navigationStore.ts (3)
369-377: Icon comparison uses reference equality, causing false positives.Line 375 compares
localItem.icon !== remoteItem.iconusing reference equality. Ificonis an object (e.g.,{ type: 'react-icon', name: 'FaHome' }), this will always returntrueeven for identical values, causing phantom "uncommitted changes" detection.🔧 Use deep comparison for icon
+import { isEqual } from "lodash"; // In hasUncommittedChanges: return !remoteItem || localItem.id !== remoteItem.id || localItem.label !== remoteItem.label || localItem.href !== remoteItem.href || - localItem.icon !== remoteItem.icon || + !isEqual(localItem.icon, remoteItem.icon) || localItem.spaceId !== remoteItem.spaceId;
807-815: Navigation config PUT request lacks response validation.The
axiosBackend.putcall at line 812 doesn't validate the response data. If the backend returns a 2xx status with an error payload in the body (e.g.,{ result: "error", error: { message: "..." } }), the update will be treated as successful and state will be updated incorrectly.🔧 Add response validation
-await axiosBackend.put("/api/navigation/config", signedRequest); +const response = await axiosBackend.put("/api/navigation/config", signedRequest); +if (response.data?.result === "error") { + throw new Error(response.data.error?.message || "Navigation config update failed"); +}
831-845: Error logging exposes sensitive data and may fail on circular references.Two issues:
- Line 839 logs
error.response.headerswhich could include authentication tokens- Line 844 uses
JSON.stringify(error, null, 2)which throws on circular references (common in Axios errors)🔧 Safer error logging
if (error?.response) { console.error("API error response:", { status: error.response.status, statusText: error.response.statusText, data: error.response.data, - headers: error.response.headers, }); } else if (error?.message) { console.error("Error message:", error.message); } - console.error("Full error object:", JSON.stringify(error, null, 2)); + try { + console.error("Error details:", { + name: error?.name, + message: error?.message, + stack: error?.stack, + }); + } catch { + console.error("Error (could not serialize):", error); + }
🧹 Nitpick comments (14)
src/common/components/organisms/navigation/ErrorBoundary.tsx (2)
6-10: Consider exposing reset capability to custom fallbacks.When a custom
fallbackis provided (lines 72-74), the reset functionality is not accessible, meaning the custom fallback cannot recover from the error state without a full page refresh.Consider using a render prop pattern or passing reset as a prop to the fallback:
♻️ Optional: Render prop pattern for custom fallback
interface ErrorBoundaryProps { children: ReactNode; - fallback?: ReactNode; + fallback?: ReactNode | ((props: { error: Error; reset: () => void }) => ReactNode); onError?: (error: Error, errorInfo: ErrorInfo) => void; }Then in render:
// Use custom fallback if provided if (this.props.fallback) { + if (typeof this.props.fallback === 'function') { + return this.props.fallback({ error: this.state.error!, reset: this.handleReset }); + } return this.props.fallback; }Also applies to: 69-74
57-58: Reminder: Address TODO for error logging integration.The TODO comment indicates plans to integrate with an error logging service (Sentry, LogRocket, etc.). This would be valuable for capturing navigation errors in production.
Would you like me to help implement the error logging integration or open an issue to track this task?
src/common/components/organisms/Sidebar.tsx (1)
76-81: Logic is correct; consider translating comment to English.The conditional logic correctly shows the sidebar during navigation edit mode while hiding it during regular page edit mode. However, the comment on lines 76-77 appears to be in Portuguese ("Retornando às classes originais..."). For codebase consistency, consider translating to English.
♻️ Suggested comment translation
- // Retornando às classes originais para preservar funcionalidade completa - // Isso mantém o comportamento de expansão/contração da barra lateral // Hide sidebar only for page edit mode, not navigation edit modeThe English comment on line 78-79 already captures the intent, so lines 76-77 could simply be removed.
tests/navigation/errorHandling.test.ts (1)
34-59: Consider usingvi.spyOnfor more idiomatic mocking.The manual override of
axios.isAxiosErrorworks but could be cleaner using vitest's built-in mocking:🔧 Suggested improvement
+import { describe, it, expect, vi } from 'vitest'; import axios from 'axios'; // ... it('should normalize network errors from axios', () => { const axiosError = Object.assign(new Error('Request failed'), { isAxiosError: true, response: { status: 500, // ... }, }) as axios.AxiosError; - const originalIsAxiosError = axios.isAxiosError; - (axios as any).isAxiosError = (error: any) => error === axiosError || originalIsAxiosError(error); + const spy = vi.spyOn(axios, 'isAxiosError').mockImplementation( + (error: any) => error === axiosError + ); try { const normalized = normalizeNavigationError(axiosError); expect(normalized.type).toBe('NETWORK'); expect(normalized.retryable).toBe(true); } finally { - (axios as any).isAxiosError = originalIsAxiosError; + spy.mockRestore(); } });src/common/data/stores/app/space/spaceStore.ts (1)
739-767: Duplicate HTML detection logic.The HTML error page detection (
textContent.trim().startsWith("<!DOCTYPE")) appears twice: once in the catch block (lines 743-751) and again after the try block (lines 759-767). The second check on lines 759-767 handles cases where the request succeeds but returns an HTML error page (some servers return 200 with HTML error content).Consider extracting this to a helper function to reduce duplication:
🔧 Suggested helper
const isHtmlErrorPage = (content: string): boolean => { const trimmed = content.trim(); return trimmed.startsWith("<!DOCTYPE") || trimmed.startsWith("<html"); };src/app/page.tsx (1)
10-24: Simplified redirect logic looks good.The redirect now uses the first configured navigation item's href, with a sensible
/homefallback. This aligns with the PR's goal of using navigation config as the source of truth.Minor simplification possible:
🔧 Simplified href handling
if (firstNavItem?.href) { - // Remove leading slash if present and redirect - const href = firstNavItem.href.startsWith('/') - ? firstNavItem.href - : `/${firstNavItem.href}`; + // Ensure leading slash and redirect + const href = firstNavItem.href.startsWith('/') + ? firstNavItem.href + : `/${firstNavItem.href}`; redirect(href); }Actually, the current code is correct. The comment says "Remove leading slash" but the code actually ensures one exists. Consider updating the comment:
- // Remove leading slash if present and redirect + // Ensure href has leading slash for redirectsrc/common/components/atoms/editable-text.tsx (1)
72-76: Double-clickpreventDefaultmay interfere with text selection.The
preventDefault()on double-click prevents the browser's native double-click-to-select-word behavior. While this prevents navigation, users may expect double-clicking to select text in the input. Consider removinge.preventDefault()if text selection is desired.Optional: Allow native text selection
onDoubleClick={(e) => { // Prevent navigation when double-clicking on input (select all text) e.stopPropagation(); - e.preventDefault(); }}src/common/components/organisms/navigation/useNavigation.ts (1)
60-70: Unused store selectors:deleteNavigationItemandrenameNavigationItem.Lines 65-66 destructure
deleteNavigationItemandrenameNavigationItemfrom the store, but these are never used within this hook. This creates unnecessary re-renders when these functions change.🧹 Remove unused selectors
const { loadNavigation, localNavigation, hasUncommittedChanges, createNavigationItem, - deleteNavigationItem, - renameNavigationItem, updateNavigationOrder, commitNavigationChanges, resetNavigationChanges, } = useAppStore((state) => ({ loadNavigation: state.navigation.loadNavigation, localNavigation: state.navigation.localNavigation, hasUncommittedChanges: state.navigation.hasUncommittedChanges, createNavigationItem: state.navigation.createNavigationItem, - deleteNavigationItem: state.navigation.deleteNavigationItem, - renameNavigationItem: state.navigation.renameNavigationItem, updateNavigationOrder: state.navigation.updateNavigationOrder, commitNavigationChanges: state.navigation.commitNavigationChanges, resetNavigationChanges: state.navigation.resetNavigationChanges, }));src/common/providers/GlobalErrorHandler.tsx (1)
34-48: Consider defensive handling fortoString()calls.The
arg?.toStringcheck at line 40 could throw on certain exotic objects (e.g., revoked Proxies). While rare, wrapping this in a try-catch would make the error handler more robust.🛠️ Suggested improvement
const isBrowserExtensionError = (args: any[]): boolean => { const errorString = args .map((arg) => { - if (typeof arg === "string") return arg; - if (arg?.message) return arg.message; - if (arg?.stack) return arg.stack; - if (arg?.toString) return arg.toString(); - return String(arg); + try { + if (typeof arg === "string") return arg; + if (arg?.message) return arg.message; + if (arg?.stack) return arg.stack; + if (arg?.toString) return arg.toString(); + return String(arg); + } catch { + return ""; + } }) .join(" ");src/common/components/organisms/NavigationManagement.tsx (1)
54-56: Simplify the items selection logic.The nested ternary is hard to follow. Consider restructuring for clarity.
♻️ Suggested refactor
- // Use local navigation items if available, otherwise fall back to props - // Only use localNavigation if it has items (not empty array from initial state) - const items = localNavigation.length > 0 || navigationItems.length === 0 - ? localNavigation.length > 0 ? localNavigation : navigationItems - : navigationItems; + // Use local navigation items if available, otherwise fall back to props + const items = useMemo(() => { + if (localNavigation.length > 0) { + return localNavigation; + } + return navigationItems; + }, [localNavigation, navigationItems]);src/common/components/organisms/navigation/NavigationEditor.tsx (2)
103-111:iconButtonRefsgrows unboundedly.The refs map accumulates entries for every item but is never cleaned up when items are deleted. Over time with many item additions/deletions, this could leak memory.
🧹 Consider cleanup on item removal
+ // Clean up refs for deleted items + useEffect(() => { + const currentIds = new Set(items.map(item => item.id)); + Object.keys(iconButtonRefs.current).forEach(id => { + if (!currentIds.has(id)) { + delete iconButtonRefs.current[id]; + } + }); + }, [items]);
382-416: Duplicate delete logic between onClick and onKeyDown.The delete handler logic is duplicated. Extract to a shared function.
♻️ Suggested refactor
+ const handleDeleteClick = useCallback((item: NavigationItem, e: React.SyntheticEvent) => { + e.preventDefault(); + e.stopPropagation(); + if (process.env.NODE_ENV === 'development') { + console.log('[NavigationEditor] Deleting navigation item:', { + itemId: item.id, + label: item.label, + href: item.href + }); + } + onDelete(item.id); + toast.success("Navigation item deleted"); + }, [onDelete]); // Then use in both handlers: <button - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - // ... duplicated logic - }} - onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - e.stopPropagation(); - // ... duplicated logic - } - }} + onClick={(e) => handleDeleteClick(item, e)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + handleDeleteClick(item, e); + } + }}src/common/components/organisms/Navigation.tsx (2)
313-324:createUrlIconWrappercreates new component on each call.Despite being wrapped in
useCallback, this function returns a newReact.memocomponent instance each time it's called. This defeats the memoization and causes remounts. The caching iniconFormitigates this, but the function itself should return stable references.♻️ Consider pre-creating or lazily caching the wrapper
The current pattern works because
iconForcaches the result, butcreateUrlIconWrapperis misleadingly named as a factory when it should ideally be integrated into the caching logic directly. TheuseCallbackon it provides no benefit since each call creates a new component anyway.- // Reusable component factory for URL icons - const createUrlIconWrapper = useCallback((src: string): React.FC => { - const CustomIconWrapper = React.memo(() => ( + // Factory for URL icon components (caching handled by iconFor) + const createUrlIconWrapper = (src: string): React.FC => { + const CustomIconWrapper: React.FC = () => ( <img src={src} alt="icon" className="w-5 h-5 rounded object-contain" aria-hidden="true" /> - )); + ); CustomIconWrapper.displayName = `CustomIcon(${src})`; return CustomIconWrapper; - }, []); + };The
React.memowrapper is unnecessary sinceiconForalready caches the component.
641-641: Redundant conditional expression.
username ? username : undefinedis equivalent to justusernamesinceusernameis alreadystring | undefined.- username={username ? username : undefined} + username={username}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
docs/DOCUMENTATION_OVERVIEW.mddocs/GETTING_STARTED.mddocs/README.mddocs/SYSTEMS/CONFIGURATION/ARCHITECTURE_OVERVIEW.mddocs/SYSTEMS/NAVIGATION/OVERVIEW.mdscripts/seed.tssrc/app/[navSlug]/[[...tabName]]/NavPageSpace.tsxsrc/app/[navSlug]/[[...tabName]]/page.tsxsrc/app/page.tsxsrc/common/components/atoms/editable-text.tsxsrc/common/components/atoms/reorderable-tab.tsxsrc/common/components/organisms/Navigation.tsxsrc/common/components/organisms/NavigationManagement.tsxsrc/common/components/organisms/Sidebar.tsxsrc/common/components/organisms/TabBar.tsxsrc/common/components/organisms/navigation/ErrorBoundary.tsxsrc/common/components/organisms/navigation/NavigationEditor.tsxsrc/common/components/organisms/navigation/NavigationItem.tsxsrc/common/components/organisms/navigation/constants.tssrc/common/components/organisms/navigation/errorHandling.tssrc/common/components/organisms/navigation/useNavigation.tssrc/common/data/stores/app/index.tsxsrc/common/data/stores/app/navigation/navigationStore.tssrc/common/data/stores/app/space/spaceStore.tssrc/common/lib/theme/ThemeSettingsEditor.tsxsrc/common/providers/GlobalErrorHandler.tsxsrc/common/providers/index.tsxsrc/common/utils/layoutFormatUtils.tssrc/common/utils/navUtils.tssrc/common/utils/tabUtils.tssrc/config/index.tssrc/config/loaders/registry.tssrc/config/systemConfig.tssrc/pages/api/navigation/config.tstests/navigation/errorHandling.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/common/utils/tabUtils.ts
- src/config/systemConfig.ts
- src/app/[navSlug]/[[...tabName]]/page.tsx
- src/config/index.ts
- docs/GETTING_STARTED.md
- src/common/components/organisms/navigation/errorHandling.ts
- src/common/lib/theme/ThemeSettingsEditor.tsx
- src/common/utils/layoutFormatUtils.ts
- docs/SYSTEMS/NAVIGATION/OVERVIEW.md
- src/common/data/stores/app/index.tsx
- src/common/utils/navUtils.ts
- src/common/components/atoms/reorderable-tab.tsx
- docs/DOCUMENTATION_OVERVIEW.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-29T18:25:31.969Z
Learnt from: j-paterson
Repo: blankdotspace/space-system PR: 1625
File: src/common/lib/utils/tokenGates.ts:12-20
Timestamp: 2025-12-29T18:25:31.969Z
Learning: Gate tokens in src/common/lib/utils/tokenGates.ts are currently fetched redundantly from 3 client components (CreateCast.tsx, BackgroundGenerator.tsx, NogsGateButton.tsx) with no caching, and should be refactored to use a cached API route with a shared client hook to reduce redundant server calls.
Applied to files:
src/common/components/organisms/Navigation.tsx
📚 Learning: 2025-12-31T21:13:36.931Z
Learnt from: willyogo
Repo: blankdotspace/space-system PR: 1641
File: src/fidgets/farcaster/components/CastRow.tsx:247-250
Timestamp: 2025-12-31T21:13:36.931Z
Learning: In src/fidgets/farcaster/components/CastRow.tsx (and similar UI components), hardcoded semi-transparent colors like `rgba(128, 128, 128, 0.2)` for borders and backgrounds are acceptable because they adapt to any background color, eliminating the need for theme tokens in ui_config for these specific use cases.
Applied to files:
src/common/components/organisms/Navigation.tsx
🧬 Code graph analysis (11)
src/common/components/atoms/editable-text.tsx (1)
src/common/utils/tabUtils.ts (1)
validateTabName(11-16)
src/common/data/stores/app/space/spaceStore.ts (3)
src/common/lib/signedFiles.ts (1)
SignedFile(16-18)src/pages/api/space/registry/[spaceId]/index.ts (1)
UpdateTabOrderRequest(30-30)src/pages/api/space/homebase/tabOrder.ts (1)
UpdateTabOrderRequest(15-15)
src/common/components/organisms/TabBar.tsx (1)
src/common/utils/tabUtils.ts (1)
generateUniqueName(44-58)
tests/navigation/errorHandling.test.ts (1)
src/common/components/organisms/navigation/errorHandling.ts (3)
normalizeNavigationError(18-48)NavigationError(6-10)getUserFriendlyMessage(56-69)
src/common/providers/index.tsx (1)
src/common/providers/GlobalErrorHandler.tsx (1)
GlobalErrorHandler(13-78)
src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (3)
src/common/types/spaceData.ts (2)
NavPageSpacePageData(77-83)SPACE_TYPES(7-13)src/common/lib/hooks/useCurrentSpaceIdentityPublicKey.ts (1)
useCurrentSpaceIdentityPublicKey(3-5)src/app/(spaces)/PublicSpace.tsx (1)
PublicSpace(33-463)
src/config/loaders/registry.ts (1)
src/config/systemConfig.ts (1)
SystemConfig(27-37)
src/common/components/organisms/navigation/NavigationItem.tsx (5)
src/config/index.ts (1)
SystemConfig(106-106)src/config/systemConfig.ts (2)
SystemConfig(27-37)NavigationItem(156-164)src/common/lib/hooks/useUIColors.ts (1)
useUIColors(15-49)src/common/components/atoms/badge.tsx (1)
Badge(40-40)src/common/lib/utils/mergeClasses.ts (1)
mergeClasses(4-6)
src/common/components/organisms/Navigation.tsx (9)
src/common/components/organisms/Sidebar.tsx (1)
useSidebarContext(55-57)src/common/lib/hooks/useCurrentSpaceIdentityPublicKey.ts (1)
useCurrentSpaceIdentityPublicKey(3-5)src/common/lib/hooks/useUIColors.ts (1)
useUIColors(15-49)src/common/components/organisms/navigation/NavigationItem.tsx (2)
NavigationItemProps(27-38)NavigationItem(146-160)src/common/data/stores/app/index.tsx (1)
useAppStore(174-174)src/config/systemConfig.ts (1)
NavigationItem(156-164)src/common/components/organisms/navigation/ErrorBoundary.tsx (1)
NavigationErrorBoundary(22-113)src/common/components/organisms/navigation/NavigationEditor.tsx (1)
NavigationEditor(508-508)src/common/lib/utils/analyticsUtils.ts (1)
trackAnalyticsEvent(5-12)
src/pages/api/navigation/config.ts (7)
src/config/systemConfig.ts (1)
NavigationConfig(144-149)src/common/lib/signedFiles.ts (3)
Signable(20-23)isSignable(29-41)validateSignable(43-52)src/common/data/api/requestHandler.ts (2)
NounspaceResponse(9-16)requestHandler(31-51)src/common/data/database/supabase/clients/server.ts (1)
createSupabaseServerClient(7-7)src/common/utils/navUtils.ts (2)
validateNavItemLabel(19-35)validateNavItemHref(42-72)src/supabase/database.d.ts (1)
Json(3-9)src/common/types/spaceData.ts (1)
SPACE_TYPES(7-13)
src/common/data/stores/app/navigation/navigationStore.ts (7)
src/common/types/spaceData.ts (1)
SPACE_TYPES(7-13)src/config/systemConfig.ts (2)
NavigationItem(156-164)NavigationConfig(144-149)src/common/data/stores/createStore.ts (2)
StoreSet(51-51)StoreGet(52-52)src/common/data/stores/app/index.tsx (1)
AppStore(43-56)src/common/utils/navUtils.ts (5)
isDuplicateNavLabel(82-90)generateUniqueNavLabel(164-179)validateNavItemHref(42-72)isDuplicateNavHref(99-108)generateUniqueHrefFromLabel(118-154)src/common/lib/signedFiles.ts (1)
signSignable(54-62)src/pages/api/space/registry/index.ts (1)
RegisterNewSpaceResponse(114-114)
🔇 Additional comments (39)
src/common/components/organisms/TabBar.tsx (2)
16-16: LGTM!Clean import consolidation - both
validateTabNameandgenerateUniqueNameare appropriately imported from the same utility module.
198-206: Good refactor to centralized utility.The delegation to
generateUniqueNamecorrectly centralizes the deduplication logic. The null return (when max iterations exceeded) is properly handled by the caller at line 169.scripts/seed.ts (1)
20-20: LGTM - Environment variable rename is consistently applied.The rename from
SUPABASE_SERVICE_ROLE_KEYtoSUPABASE_SERVICE_KEYis consistently updated across the comment (line 20), usage (line 64), and error message (line 70) in this file.Also applies to: 64-64, 70-70
src/common/components/organisms/navigation/ErrorBoundary.tsx (1)
35-59: Error handling lifecycle is correctly implemented.The separation between
getDerivedStateFromError(for synchronous state update during render) andcomponentDidCatch(for side effects like logging and storingerrorInfo) follows React's recommended error boundary pattern.src/common/components/organisms/navigation/constants.ts (1)
1-33: Well-documented constants with sensible values.The constants are appropriately named, reasonably valued, and well-documented. The inline JSDoc comments clearly explain the purpose and rationale for each constant, which aids maintainability.
src/common/components/organisms/Sidebar.tsx (1)
37-48: Memoization is correctly implemented.The
useMemodependencies correctly include botheditModeandnavEditMode, ensuring the context value updates when either edit mode changes. The state setters (setEditMode, etc.) are stable and don't need to be in the dependency array.src/config/loaders/registry.ts (3)
83-95: Correctly propagates communityId to SystemConfig.The
transformRowToSystemConfigfunction now correctly accepts and includescommunityIdin the returnedSystemConfig, aligning with the updated interface definition.
97-140: Cache cleanup strategy is reasonable.The dual approach—lazy deletion on read (line 121) plus batch cleanup on write when cache exceeds 10 entries—effectively prevents unbounded memory growth while minimizing cleanup overhead. The 60-second TTL combined with read-time eviction ensures stale entries don't persist long.
188-258: Database loading with proper validation and caching.The
loadCommunityConfigFromDatabasefunction correctly:
- Checks cache before querying
- Validates required fields and throws descriptive errors for data integrity issues
- Distinguishes between "not found" (returns null) and transient errors (logs and returns null)
- Caches successful results
The updated call to
transformRowToSystemConfig(data, communityId)on line 252 correctly passes thecommunityIdparameter.tests/navigation/errorHandling.test.ts (2)
1-7: LGTM! Previous concern addressed.The test file now correctly imports
NavigationError,normalizeNavigationError, andgetUserFriendlyMessagefrom the production module, ensuring tests validate the actual implementation rather than a local copy.
103-151: Good test coverage for user-friendly messages.The tests comprehensively cover all
NavigationErrorvariants including the retryable vs non-retryable network error distinction.src/common/components/organisms/navigation/NavigationItem.tsx (2)
146-160: Memo comparison omitsonClickandonNavigate- verify these are stable.The custom comparison function doesn't include
onClick,onNavigate, orIconin the equality check. If these callbacks change without other props changing, the component won't re-render and will use stale closures.This is fine if callers always wrap these in
useCallback, but could cause subtle bugs otherwise.
111-134: Good accessibility implementation.The component properly handles:
aria-current="page"for active statearia-labelwhen shrunktabIndex={-1}when disabledrole="none"on<li>for proper menu semanticssrc/common/data/stores/app/space/spaceStore.ts (4)
260-291: Good defensive initialization of remoteSpaces.The initialization guard at lines 267-275 prevents errors when committing tabs for newly created spaces that don't yet have a remoteSpaces entry. The additional check on line 281 prevents attempting to delete non-existent tabs.
607-617: Proactive initialization prevents commit failures for new spaces.Pre-initializing
remoteSpaces[spaceId]before the batch operations ensures thatcommitSpaceTabToDatabaseand other operations have a valid remote space entry to work with, even for newly created spaces.
680-702: Smart optimization for uncommitted tabs.Skipping storage fetch when a tab exists locally but not remotely prevents unnecessary 400/404 errors for new uncommitted tabs. This improves UX during navigation editing.
873-884: Graceful handling of missing tabOrder file.This correctly handles new spaces that haven't committed their tab order yet, falling back to local order without throwing errors.
src/app/[navSlug]/[[...tabName]]/NavPageSpace.tsx (2)
72-126: Local-first resolution logic looks correct.The data resolution properly:
- Handles rename transitions via
hrefMappings- Searches both current and mapped hrefs
- Falls back to server data when local data unavailable
- Returns
nullfor proper 404 handlingOne consideration: if
localNavItem.spaceIdexists butspaceLocalSpaces[localNavItem.spaceId]doesn't, the code falls through to server data. This seems intentional but could potentially cause a brief flash if local nav exists but space hasn't been initialized yet.
129-160: Correct hook ordering with 404 handling.The code properly calls all hooks before the conditional return, satisfying React's Rules of Hooks. The comment on line 129 documents this intentional pattern clearly.
src/app/page.tsx (1)
7-26: Clean root redirect implementation.The simplified redirect logic correctly:
- Loads system config for navigation items
- Redirects to the first nav item's href
- Falls back to
/homeif no items configuredThis is a good simplification from the previous implementation that loaded tab order from storage.
src/common/components/atoms/editable-text.tsx (2)
28-33: LGTM! Good state synchronization pattern.The
useEffectcorrectly syncs internal state with props only when not actively editing, preventing loss of in-progress edits.
35-52: LGTM! Validation flow is well-structured.The validation logic correctly:
- Trims input before validation
- Falls back to
validateTabNamewhen no custom validator is provided- Resets to original value on validation failure while keeping edit mode active
src/common/data/stores/app/navigation/navigationStore.ts (3)
159-185: LGTM! Atomic state update combining space and navigation item creation.The single
setcall atomically updates bothlocalSpacesandlocalNavigation, preventing inconsistent state if one operation fails. This addresses a previous review concern.
210-226: LGTM! Proper cleanup of associated local space on deletion.The deletion logic now correctly removes both the navigation item and its associated local space in a single atomic operation, preventing memory leaks.
616-629: LGTM! Space registration failures now abort the entire commit.This change properly fails the commit when space registration fails, preventing navigation items from pointing to non-existent spaces. This addresses a critical previous review concern about data consistency.
src/common/components/organisms/navigation/useNavigation.ts (3)
130-151: LGTM! Safe cancel navigation pattern prevents 404 flashes.The
pendingCancelTargetRefpattern correctly defers the navigation reset until the route change completes, preventing 404 errors when canceling from a locally-created (uncommitted) navigation item.
116-121: LGTM! Proper cleanup of debounced function.The cleanup effect correctly cancels pending debounced calls on unmount, preventing memory leaks and state updates on unmounted components.
336-337: Remove this comment—the current implementation is correct.
hasUncommittedChangesis retrieved from the store as a function() => booleanand intentionally evaluated at line 336 to return a boolean value. This matches the hook's return type interface (hasUncommittedChanges: boolean) and consumer expectations. Consumers receive a boolean snapshot of the state, not a function. If reactive updates are needed, that would require consumers to subscribe directly to the store or the hook to return an observable, but the current design is intentional.Likely an incorrect or invalid review comment.
src/pages/api/navigation/config.ts (5)
32-69: LGTM! Type guard now validates navigation item structure.The strengthened type guard correctly validates that each item has required
id,label, andhreffields as strings, preventing runtime errors from malformed items.
81-103: LGTM! Timestamp freshness validation prevents replay attacks.The
isTimestampFreshfunction correctly:
- Rejects timestamps older than 15 minutes
- Allows 5 minutes of clock skew for future timestamps
- Validates timestamp format before comparison
309-354: LGTM! Row-level update verification prevents silent failures.The update now uses
.select()to verify rows were updated and returns a 409 error if no rows matched. This handles race conditions where the config was unpublished between fetch and update.
375-424: Space cleanup is best-effort after successful config update.The cleanup logic correctly:
- Only runs after the config update succeeds
- Logs warnings but doesn't fail the request if cleanup fails
- Handles both storage files and space registrations
This is appropriate since the primary operation (config update) succeeded and cleanup failures can be addressed separately.
159-170: Debug logging is appropriately gated to development.The detailed logging of navigation items (including
spaceId) is now properly gated behindprocess.env.NODE_ENV === 'development', addressing the previous concern about production logging.docs/SYSTEMS/CONFIGURATION/ARCHITECTURE_OVERVIEW.md (3)
193-195: LGTM! SystemConfig documentation updated with new fields.The documentation correctly reflects the addition of
adminIdentityPublicKeysfor admin authorization andcommunityIdfor API operations.
210-212: Helpful clarification distinguishingcommunityIdfromcommunity.type.This note prevents confusion between the database identifier (
communityId) and the semantic community descriptor (community.type).
529-532: LGTM! Navigation editor files properly documented.The related files section now includes all new navigation-related components: NavigationEditor, useNavigation hook, navigationStore, and the API endpoint.
src/common/providers/index.tsx (1)
43-68: LGTM!The
GlobalErrorHandleris correctly placed at the top level outside the provider chain since it only sets up console overrides and doesn't require any React context. The fragment wrapper is appropriate for adding this sibling component.src/common/providers/GlobalErrorHandler.tsx (1)
71-74: LGTM!Cleanup correctly restores the original console handlers on unmount, preventing memory leaks and ensuring proper handler restoration when the component is removed.
src/common/components/organisms/Navigation.tsx (1)
131-149: LGTM - Addresses past review feedback.The
forcedExpansionRefpattern correctly prevents render loops when forcing sidebar expansion during edit mode, and properly clears the ref when exiting edit mode.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Clear icon cache on navigation/route changes to prevent memory growth | ||
| useEffect(() => { | ||
| iconWrapperCacheRef.current.clear(); | ||
| }, [pathname]); |
There was a problem hiding this comment.
Icon cache cleared on every navigation.
Clearing the entire cache on every pathname change is overly aggressive. This defeats the purpose of caching and causes icons to remount on every navigation. Consider only clearing stale entries or removing this effect entirely since the LRU eviction at lines 356-376 already handles unbounded growth.
🔧 Suggested fix - remove aggressive clearing
- // Clear icon cache on navigation/route changes to prevent memory growth
- useEffect(() => {
- iconWrapperCacheRef.current.clear();
- }, [pathname]);The LRU-style eviction (MAX_ICON_CACHE = 50) already prevents unbounded growth. Clearing on every navigation causes unnecessary component remounts.
📝 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.
| // Clear icon cache on navigation/route changes to prevent memory growth | |
| useEffect(() => { | |
| iconWrapperCacheRef.current.clear(); | |
| }, [pathname]); |
🤖 Prompt for AI Agents
In `@src/common/components/organisms/Navigation.tsx` around lines 387 - 390, The
useEffect that clears iconWrapperCacheRef.current on every pathname change is
too aggressive; remove the entire useEffect block (the one referencing
useEffect(() => { iconWrapperCacheRef.current.clear(); }, [pathname])) so icons
are not remounted on every navigation and rely on the existing LRU eviction
logic (MAX_ICON_CACHE and the eviction code around the icon cache) to bound
memory; if you prefer targeted cleanup instead, replace the effect with logic
that only removes stale entries from iconWrapperCacheRef based on age rather
than clearing the whole map.
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error | ||
| ? error.message | ||
| : "Failed to update icon"; | ||
| console.error('[NavigationEditor] Icon update failed:', { | ||
| itemId, | ||
| iconName, | ||
| error: errorMessage | ||
| }); | ||
| toast.error(errorMessage); | ||
|
|
||
| // Re-throw for error boundaries | ||
| if (error instanceof Error) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Re-throwing after catch defeats error handling purpose.
Lines 165-167 re-throw the error after showing a toast, which will propagate to the error boundary and potentially crash the UI. If you want the error boundary to catch it, don't catch it here. If you want to show a toast and continue, don't re-throw.
🔧 Suggested fix - remove re-throw to allow graceful handling
} catch (error: unknown) {
const errorMessage = error instanceof Error
? error.message
: "Failed to update icon";
console.error('[NavigationEditor] Icon update failed:', {
itemId,
iconName,
error: errorMessage
});
toast.error(errorMessage);
-
- // Re-throw for error boundaries
- if (error instanceof Error) {
- throw error;
- }
}🤖 Prompt for AI Agents
In `@src/common/components/organisms/navigation/NavigationEditor.tsx` around lines
153 - 168, The catch block in NavigationEditor (around the icon update logic
referencing itemId and iconName) currently re-throws the caught Error after
logging and showing toast.error, which defeats the local graceful handling;
remove the "throw error" path so the handler only logs the error and shows the
toast (and optionally return/exit the function after the toast) to prevent
propagation to the error boundary while preserving the console.error and
toast.error behavior.
| {isLoggedIn && ( | ||
| <NavItemComponent | ||
| label="My Space" | ||
| Icon={CurrentUserImage} | ||
| href={`/s/${username}`} | ||
| onClick={() => trackAnalyticsEvent(AnalyticsEvent.CLICK_MY_SPACE)} | ||
| disable={true} | ||
| shrunk={isShrunk} | ||
| systemConfig={systemConfig} | ||
| /> |
There was a problem hiding this comment.
Potential /s/undefined URL when username is missing.
username is an optional prop, but line 474 uses it directly in the href. If username is undefined, this creates an invalid URL /s/undefined.
🔧 Suggested fix
{isLoggedIn && (
<NavItemComponent
label="My Space"
Icon={CurrentUserImage}
- href={`/s/${username}`}
+ href={username ? `/s/${username}` : "#"}
onClick={() => trackAnalyticsEvent(AnalyticsEvent.CLICK_MY_SPACE)}
- disable={true}
+ disable={!username}
shrunk={isShrunk}
systemConfig={systemConfig}
/>
)}📝 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.
| {isLoggedIn && ( | |
| <NavItemComponent | |
| label="My Space" | |
| Icon={CurrentUserImage} | |
| href={`/s/${username}`} | |
| onClick={() => trackAnalyticsEvent(AnalyticsEvent.CLICK_MY_SPACE)} | |
| disable={true} | |
| shrunk={isShrunk} | |
| systemConfig={systemConfig} | |
| /> | |
| {isLoggedIn && ( | |
| <NavItemComponent | |
| label="My Space" | |
| Icon={CurrentUserImage} | |
| href={username ? `/s/${username}` : "#"} | |
| onClick={() => trackAnalyticsEvent(AnalyticsEvent.CLICK_MY_SPACE)} | |
| disable={!username} | |
| shrunk={isShrunk} | |
| systemConfig={systemConfig} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
In `@src/common/components/organisms/navigation/NavigationEditor.tsx` around lines
470 - 479, The NavItemComponent is building an href with a possibly undefined
username which can produce "/s/undefined"; update the NavigationEditor rendering
logic so you only pass a valid href when username exists (e.g., conditionally
build href using username or fallback to a safe route/empty string) or render
the NavItemComponent without the href and/or with disable=true when username is
missing; locate the NavItemComponent usage in NavigationEditor.tsx (the block
that sets href={`/s/${username}`}) and change it to guard the href creation
against undefined username and ensure analytics/onClick behavior remains
correct.
| const debouncedUpdateOrder = useCallback( | ||
| debounce((newOrder: NavigationItem[]) => { | ||
| updateNavigationOrder(newOrder); | ||
| }, 300), | ||
| [updateNavigationOrder] | ||
| ); |
There was a problem hiding this comment.
Missing cleanup for debounced function.
The debounced function should be cancelled on unmount to prevent stale updates. Use useMemo instead of useCallback and add cleanup.
🔧 Proposed fix
- const debouncedUpdateOrder = useCallback(
- debounce((newOrder: NavigationItem[]) => {
+ const debouncedUpdateOrder = useMemo(
+ () => debounce((newOrder: NavigationItem[]) => {
updateNavigationOrder(newOrder);
}, 300),
[updateNavigationOrder]
);
+
+ // Cleanup debounce on unmount
+ useEffect(() => {
+ return () => {
+ debouncedUpdateOrder.cancel();
+ };
+ }, [debouncedUpdateOrder]);📝 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 debouncedUpdateOrder = useCallback( | |
| debounce((newOrder: NavigationItem[]) => { | |
| updateNavigationOrder(newOrder); | |
| }, 300), | |
| [updateNavigationOrder] | |
| ); | |
| const debouncedUpdateOrder = useMemo( | |
| () => debounce((newOrder: NavigationItem[]) => { | |
| updateNavigationOrder(newOrder); | |
| }, 300), | |
| [updateNavigationOrder] | |
| ); | |
| // Cleanup debounce on unmount | |
| useEffect(() => { | |
| return () => { | |
| debouncedUpdateOrder.cancel(); | |
| }; | |
| }, [debouncedUpdateOrder]); |
🤖 Prompt for AI Agents
In `@src/common/components/organisms/NavigationManagement.tsx` around lines 58 -
63, The debouncedUpdateOrder created with useCallback needs cleanup to avoid
stale invocations on unmount; replace useCallback with useMemo to create
debouncedUpdateOrder = useMemo(() => debounce((newOrder)=>
updateNavigationOrder(newOrder), 300), [updateNavigationOrder]) and add an
effect that returns a cleanup which calls debouncedUpdateOrder.cancel() (or
cancels the debounced function) to run on unmount so pending debounced calls are
cleared.
| } catch (error: any) { | ||
| console.error("Failed to commit navigation changes:", error); | ||
| // For local testing, still show success if it's just a 404 (API not implemented) | ||
| if (error?.response?.status === 404) { | ||
| toast.success("Navigation changes staged locally (API endpoints not yet implemented)"); | ||
| onCommit?.(); | ||
| } else { | ||
| toast.error("Failed to commit navigation changes"); | ||
| } |
There was a problem hiding this comment.
Masking 404 errors as success may hide real issues.
The PR comments report a 400 Bad Request causing commit failures. Treating 404 as a success (lines 108-110) while showing a generic error for other statuses may obscure the actual problem. The commit failure mentioned is a 400, not 404, so this code path wouldn't help with that issue.
Consider logging the actual error status and message to help diagnose failures:
🐛 Suggested improvement
} catch (error: any) {
- console.error("Failed to commit navigation changes:", error);
+ console.error("Failed to commit navigation changes:", {
+ status: error?.response?.status,
+ message: error?.message,
+ data: error?.response?.data,
+ });
// For local testing, still show success if it's just a 404 (API not implemented)
if (error?.response?.status === 404) {
toast.success("Navigation changes staged locally (API endpoints not yet implemented)");
onCommit?.();
} else {
- toast.error("Failed to commit navigation changes");
+ const errorMessage = error?.response?.data?.message || error?.message || "Failed to commit navigation changes";
+ toast.error(errorMessage);
}📝 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.
| } catch (error: any) { | |
| console.error("Failed to commit navigation changes:", error); | |
| // For local testing, still show success if it's just a 404 (API not implemented) | |
| if (error?.response?.status === 404) { | |
| toast.success("Navigation changes staged locally (API endpoints not yet implemented)"); | |
| onCommit?.(); | |
| } else { | |
| toast.error("Failed to commit navigation changes"); | |
| } | |
| } catch (error: any) { | |
| console.error("Failed to commit navigation changes:", { | |
| status: error?.response?.status, | |
| message: error?.message, | |
| data: error?.response?.data, | |
| }); | |
| // For local testing, still show success if it's just a 404 (API not implemented) | |
| if (error?.response?.status === 404) { | |
| toast.success("Navigation changes staged locally (API endpoints not yet implemented)"); | |
| onCommit?.(); | |
| } else { | |
| const errorMessage = error?.response?.data?.message || error?.message || "Failed to commit navigation changes"; | |
| toast.error(errorMessage); | |
| } |
🤖 Prompt for AI Agents
In `@src/common/components/organisms/NavigationManagement.tsx` around lines 105 -
113, In NavigationManagement.tsx update the catch block in the commit function
to surface real error details instead of masking 404s: log
error.response?.status and error.response?.data (or error.message) in the
console and include them in the toast/error message, only treat 404 as a
local-only fallback behind a clear dev flag or by checking NODE_ENV, and ensure
onCommit is called only on true success or an explicit dev-fallback path;
reference the existing catch(error: any) block, onCommit(), and
toast.success/toast.error calls when making these changes.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.