Refine web navigation chrome#59
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds Global Notifications and Global Threads screens that aggregate data across online projects, registers them as root stack routes, refactors ProjectSidebar to a primary-nav mode with a SidebarMode atom, simplifies mobile tabs to three entries, adds TopBar route buttons, adjusts AuthMenu local-mode rendering, and fixes stale-closure behavior in LibraryScreen. ChangesGlobal Notifications & Threads with Navigation Restructuring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/app/(main)/(tabs)/(threads)/threads.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. app/app/(main)/(tabs)/library/index.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. app/app/(main)/global-notifications.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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
🧹 Nitpick comments (2)
app/components/TopBar.tsx (1)
40-40: 💤 Low valueConsider using theme tokens instead of hardcoded colors.
The icon colors are hardcoded (
#fafafaand#a1a1aa) while the rest of the component uses theme-based className props (e.g.,text-foreground,text-muted-foreground). For consistency and maintainability, consider extracting these colors from the theme system or using NativeWind's color utilities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/TopBar.tsx` at line 40, The Icon component in TopBar uses hardcoded colors ("`#fafafa`" and "`#a1a1aa`"); change this to use the app theme tokens or NativeWind utilities instead—e.g., derive the color from the theme (useTheme or your theme object) or map the active boolean to theme class names (like "text-foreground" / "text-muted-foreground") and pass that value to Icon (or wrap Icon in a styled View) so colors come from the shared theme instead of hardcoded hex values; update the Icon usage in TopBar to reference those theme tokens or className-driven colors.app/components/MobileTabBar.tsx (1)
60-60: 💤 Low valueConsider dynamic icon color for active state consistency.
The icon uses a hardcoded
#a1a1aa(zinc-400) color that does not change based on active state, while the text below dynamically switches betweentext-foregroundandtext-muted-foreground. For better visual consistency, the icon color could also reflect the active state.♻️ Proposed refactor for dynamic icon color
- <Icon size={20} color="`#a1a1aa`" /> + <Icon size={20} className={active ? "text-foreground" : "text-muted-foreground"} />Note: This assumes lucide-react-native supports className for color. If not, use conditional color prop:
- <Icon size={20} color="`#a1a1aa`" /> + <Icon size={20} color={active ? "`#09090b`" : "`#a1a1aa`"} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/MobileTabBar.tsx` at line 60, Replace the hardcoded Icon color with the same active-state logic used for the label so the icon reflects selection; instead of <Icon size={20} color="`#a1a1aa`" />, derive the color from the tab's active flag (e.g., isActive or active) and pass that value to the Icon via its color prop (or className if lucide-react-native supports it), using the same tokens/values that drive the label classes (text-foreground when active, text-muted-foreground when inactive) so both icon and text stay visually consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/app/`(main)/(tabs)/library/index.tsx:
- Around line 79-104: The refresh function can race: a previous request for a
different endpoint may resolve after a later request and overwrite state; fix
refresh (the async function named refresh which calls listProjectLibrary) by
recording the currentEndpoint at start (already assigned to currentEndpoint) and
before committing results call setDocuments and setError only if
endpointRef.current === currentEndpoint (or otherwise track a monotonic
requestId/nonce stored in a ref and only apply results when it matches); ensure
this check is applied both in the success path (after setDocuments) and in the
catch path before calling setError so stale responses are ignored when
endpointKey/endpointRef changed.
In `@app/app/`(main)/global-notifications.tsx:
- Around line 158-165: The empty-state branch currently shows "All caught up"
whenever rows.length === 0 even if fetches failed; update the render condition
to distinguish a genuine empty inbox from a failure by introducing or using an
error flag (e.g., hasFetchError or fetchError) and change the check to show the
success empty state only when rows.length === 0 && !loading && !hasFetchError;
otherwise render the failure banner or an error-specific message. Locate the
conditional that renders the Card (the JSX using rows and loading) and adjust
the logic to consult the error flag (or set one where project fetches are
handled) so failed requests do not display the "All caught up" message.
- Around line 65-110: Make refresh last-write-wins by introducing a local
request id checked before committing results: in the refresh function generate
an incrementing id (e.g., refreshId / latestRefreshIdRef) when starting, capture
it in the async closure, and only call setRows and setErrors if the captured id
=== latestRefreshIdRef.current; also add a top-level try/catch around the whole
refresh body (wrapping the getToken call and Promise.allSettled) so refresh-wide
failures (like getToken throwing) push a descriptive error into setErrors
instead of silently falling through, and ensure setLoading(false) still runs in
finally. Update references to onlineProjectsRef, refresh, getToken, setRows,
setErrors, and onlineProjectKey accordingly.
In `@app/app/`(main)/global-threads.tsx:
- Around line 128-135: The current empty-state Card in global-threads.tsx shows
"No threads" even when all project fetches failed; update the render logic
around rows, errors and loading so that when errors.length > 0 && rows.length
=== 0 you render an error-only empty state (e.g., a Card with an error
title/message) instead of the success "No threads" message. Locate the JSX block
using rows, loading and the Card/Text elements and add a branch that checks
errors.length > 0 && rows.length === 0 first, rendering the error UI; keep the
existing loading check for the "Loading threads..." text and fall back to the
original "No threads" when there are no errors and rows is empty.
- Around line 52-94: The refresh function is vulnerable to races and loses
refresh-wide errors; make it last-write-wins by generating a unique requestId at
the start of refresh (e.g., const reqId = ++refreshIdRef.current), capture it in
the async flow and before committing any state (setRows, setErrors, setLoading)
verify reqId === refreshIdRef.current so only the latest refresh writes state,
and also catch errors from getToken (and other top-level failures) to push a
refresh-wide error message into setErrors instead of leaving the UI empty;
update the surrounding hooks to use a refreshIdRef (useRef<number>) and keep
onlineProjectsRef, getToken, setRows, setErrors, and setLoading checks in place,
and ensure the useEffect that calls refresh still depends on onlineProjectKey
and refresh.
- Around line 138-143: Update the Pressable onPress in global-threads.tsx so the
tapped row's thread id is passed to the threads view and/or a selected-thread
atom is set: change the router.navigate(buildViewHref("/threads", { project:
row.projectPath })) call inside the rows.map(…) Pressable to include the thread
id (e.g., threadId: row.thread.thread.id) in the route params and/or call the
app's selected-thread Jotai setter (e.g.,
setSelectedThreadAtom(row.thread.thread.id)) before navigating; ensure the
threads.tsx reader consumes that threadId or selected-thread atom to render the
tapped thread.
In `@app/components/ProjectSidebar.tsx`:
- Around line 610-621: The component initializes sidebarMode to "dashboard" for
routeTab but the usePrePaintEffect sets sidebarMode to "views" when routeTab is
"inbox" or "threads", causing a flash; update the initialization logic (where
sidebarMode is initially set) to include routeTab === "inbox" and routeTab ===
"threads" in the same condition used by usePrePaintEffect (or alternatively
change usePrePaintEffect to match the existing initializer), so that the initial
value of sidebarMode and the effect are consistent; specifically adjust the
initializer that reads routeTab and showPrimaryNav and the setSidebarMode usage
so both include "inbox" and "threads".
- Around line 560-564: ProjectSidebar currently uses local useState for
sidebarMode which conflicts with the repo guideline to manage client/UI state
via Jotai and also initializes differently than usePrePaintEffect causing a
mount flip; replace the local state with a Jotai-backed atom (or derive from
existing atoms in app/stores/ui) so sidebarMode and setSidebarMode use the atom
instead of useState, and change the initial/derived value logic to match the
pre-paint effect condition (treat routeTab "project" | "topology" | "library" |
"inbox" | "threads" as "views" when showPrimaryNav is true) to avoid the extra
mode flip on mount (update references to sidebarMode and setSidebarMode in
ProjectSidebar accordingly).
---
Nitpick comments:
In `@app/components/MobileTabBar.tsx`:
- Line 60: Replace the hardcoded Icon color with the same active-state logic
used for the label so the icon reflects selection; instead of <Icon size={20}
color="`#a1a1aa`" />, derive the color from the tab's active flag (e.g., isActive
or active) and pass that value to the Icon via its color prop (or className if
lucide-react-native supports it), using the same tokens/values that drive the
label classes (text-foreground when active, text-muted-foreground when inactive)
so both icon and text stay visually consistent.
In `@app/components/TopBar.tsx`:
- Line 40: The Icon component in TopBar uses hardcoded colors ("`#fafafa`" and
"`#a1a1aa`"); change this to use the app theme tokens or NativeWind utilities
instead—e.g., derive the color from the theme (useTheme or your theme object) or
map the active boolean to theme class names (like "text-foreground" /
"text-muted-foreground") and pass that value to Icon (or wrap Icon in a styled
View) so colors come from the shared theme instead of hardcoded hex values;
update the Icon usage in TopBar to reference those theme tokens or
className-driven colors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 664a9784-d17b-46e9-9236-6aa6f92a5afa
📒 Files selected for processing (10)
app/app/(main)/(tabs)/_layout.tsxapp/app/(main)/(tabs)/library/index.tsxapp/app/(main)/_layout.tsxapp/app/(main)/global-notifications.tsxapp/app/(main)/global-threads.tsxapp/components/AppShell.tsxapp/components/AuthMenu.tsxapp/components/MobileTabBar.tsxapp/components/ProjectSidebar.tsxapp/components/TopBar.tsx
This was referenced Jun 7, 2026
This was referenced Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Improvements
Bug Fixes