Conversation
…d, heatmap, teams)
… settings, and teams
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a typed API client and types, a client WebSocket hook and env var, a DashboardShell integrating WS events, many dashboard pages and UI components (friends, leaderboard, stats, teams, settings), a LayoutShell layout refactor, and supporting hooks/components for real-time dashboard functionality. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser (DashboardShell)
participant WS as WebSocket Server
participant Sidebar as Sidebar component
participant API as REST API
Browser->>WS: CONNECT ws://.../?token=auth_token
WS-->>Browser: AUTH_SUCCESS / CONNECTED
Browser->>Browser: start HEARTBEAT interval
WS-->>Browser: POKE { fromUser, message }
Browser->>Browser: handlers.POKE → show toast
Browser->>Sidebar: set isConnected = true
WS-->>Browser: ACHIEVEMENT { achievement }
Browser->>Browser: handlers.ACHIEVEMENT → show toast / UI update
Browser->>API: friendRequests.count()
API-->>Browser: { count: N }
Browser->>Sidebar: render badge with count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25d9ce30dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+104
to
+107
| const disconnect = useCallback(() => { | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| attemptRef.current = RECONNECT.maxAttempts; | ||
| if (wsRef.current) { |
There was a problem hiding this comment.
disconnect() always sets attemptRef to RECONNECT.maxAttempts, and this function is used as the cleanup for the [enabled, connect, disconnect] effect. On dashboard load, enabled typically flips from false to true after auth resolves, so cleanup runs first and exhausts retries before the first authenticated connect; if that initial socket closes before onopen (e.g., brief backend/network hiccup), onclose sees retries already exhausted and never reconnects, leaving real-time features offline until a manual refresh.
Useful? React with 👍 / 👎.
Comment on lines
35
to
37
| useEffect(() => { | ||
| if (isAuthenticated) fetchStats(); | ||
| }, [isAuthenticated, fetchStats]); |
There was a problem hiding this comment.
loading is initialized to true, but this effect only calls fetchStats() when isAuthenticated is true. For signed-out users (or expired sessions), loading is never set to false, so visiting /dashboard/stats directly leaves the page in an infinite skeleton state instead of showing a sign-in/empty state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive dashboard overhaul for DevRadar, introducing a modern, feature-rich dashboard experience with real-time WebSocket connectivity, friend management, leaderboards, team collaboration features, and detailed statistics tracking.
Changes:
- Added WebSocket integration with custom
useWebSockethook for real-time features (pokes, achievements, friend status) - Implemented complete dashboard UI with sidebar navigation, stats visualization, contribution heatmap, and network activity monitoring
- Added comprehensive friend management system with search, follow/unfollow, and friend requests
- Implemented team management features including creation, member management, invitations, and role-based access control
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/lib/hooks/use-websocket.ts | New WebSocket hook with auto-reconnect, heartbeat, and message handling |
| apps/web/src/lib/api/types.ts | Comprehensive type definitions for users, teams, stats, leaderboards, and WebSocket messages |
| apps/web/src/lib/api/index.ts | API client functions for all dashboard features |
| apps/web/src/lib/auth/api.ts | Updated to handle empty API responses |
| apps/web/src/lib/auth/auth-context.tsx | Added privacyMode field to User interface |
| apps/web/src/components/layout/layout-shell.tsx | Smart layout wrapper that conditionally renders header/footer |
| apps/web/src/components/layout/header.tsx | Added Settings menu item and refactored mobile menu overflow handling |
| apps/web/src/components/dashboard/sidebar.tsx | Collapsible sidebar with navigation, friend request badge, and connection status |
| apps/web/src/components/dashboard/user-search.tsx | User search component with debouncing and friend request sending |
| apps/web/src/components/dashboard/friend-card.tsx | Friend and request card components with status indicators |
| apps/web/src/components/dashboard/contribution-heatmap.tsx | GitHub-style contribution heatmap visualization |
| apps/web/src/components/dashboard/network-activity-card.tsx | Real-time network activity monitoring with auto-refresh |
| apps/web/src/components/dashboard/leaderboard-table.tsx | Leaderboard table with pagination and ranking |
| apps/web/src/components/dashboard/create-team-modal.tsx | Modal for creating teams with slug generation |
| apps/web/src/components/dashboard/invite-member-modal.tsx | Modal for inviting team members |
| apps/web/src/app/dashboard/dashboard-shell.tsx | Dashboard layout shell with WebSocket integration |
| apps/web/src/app/dashboard/page.tsx | Completely redesigned dashboard overview page |
| apps/web/src/app/dashboard/stats/page.tsx | Detailed stats page with streaks, heatmap, and achievements |
| apps/web/src/app/dashboard/friends/page.tsx | Friends management page with tabs for following/followers/requests |
| apps/web/src/app/dashboard/leaderboard/page.tsx | Leaderboard page with multiple ranking categories |
| apps/web/src/app/dashboard/teams/page.tsx | Teams listing page with tier gate for free users |
| apps/web/src/app/dashboard/teams/[id]/page.tsx | Team detail page with member management and invitations |
| apps/web/src/app/dashboard/settings/page.tsx | Settings page with profile, privacy, theme, and notifications |
| apps/web/.env.example | Added NEXT_PUBLIC_WS_URL environment variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
45
to
85
| const { weeks, monthLabels } = useMemo(() => { | ||
| const today = new Date(); | ||
| const cells: Array<{ date: string; level: number; seconds: number; day: number }> = []; | ||
|
|
||
| for (let i = 364; i >= 0; i--) { | ||
| const d = new Date(today); | ||
| d.setDate(d.getDate() - i); | ||
| const dateStr = formatDate(d); | ||
| const seconds = data.get(dateStr) ?? 0; | ||
| cells.push({ | ||
| date: dateStr, | ||
| level: getLevel(seconds), | ||
| seconds, | ||
| day: d.getDay(), | ||
| }); | ||
| } | ||
|
|
||
| const offset = cells[0]?.day ?? 0; | ||
| const padded = Array.from({ length: offset }, () => null); | ||
| const allCells = [...padded, ...cells]; | ||
|
|
||
| const weeksList: Array<Array<(typeof allCells)[number]>> = []; | ||
| for (let i = 0; i < allCells.length; i += 7) { | ||
| weeksList.push(allCells.slice(i, i + 7)); | ||
| } | ||
|
|
||
| const labels: Array<{ label: string; col: number }> = []; | ||
| let lastMonth = -1; | ||
| weeksList.forEach((week, wi) => { | ||
| const firstReal = week.find((c) => c !== null); | ||
| if (firstReal) { | ||
| const month = new Date(firstReal.date).getMonth(); | ||
| if (month !== lastMonth) { | ||
| labels.push({ label: MONTHS[month], col: wi }); | ||
| lastMonth = month; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return { weeks: weeksList, monthLabels: labels }; | ||
| }, [data]); |
There was a problem hiding this comment.
The ContributionHeatmap useMemo depends on 'data', which is a Map object. Since Maps are compared by reference, the memoization will break on every render even if the Map content is identical. This will cause the expensive computation to run on every render. Consider using a stable data structure or a custom comparison function, or ensure the Map reference is stable from the parent component.
| alt={member.displayName || member.username} | ||
| width={28} | ||
| height={28} | ||
| className="shrink-0" |
There was a problem hiding this comment.
The Image component is missing the 'rounded-full' class that is present in other avatar images throughout the codebase (e.g., line 125 in the same file, line 274 in dashboard/page.tsx). This creates visual inconsistency. Add the 'rounded-full' class to maintain consistent avatar styling.
Suggested change
| className="shrink-0" | |
| className="shrink-0 rounded-full" |
Comment on lines
40
to
44
| const handleInvite = async () => { | ||
| if (!email.includes('@')) { | ||
| toast.error('Enter a valid email address'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The email validation only checks if the string contains an '@' symbol, which is insufficient. This allows invalid emails like '@', 'test@', '@example.com', or 'test@@example.com'. Consider using a proper email regex pattern or a validation library to ensure the email format is valid before sending the invitation.
Comment on lines
+114
to
+117
| useEffect(() => { | ||
| if (enabled) connect(); | ||
| return () => disconnect(); | ||
| }, [enabled, connect, disconnect]); |
There was a problem hiding this comment.
The cleanup function in this useEffect has a dependency on connect and disconnect, which are useCallback dependencies that change on every render due to getToken. This creates a memory leak where the disconnect function in the cleanup may not properly clear the timeout because it references a stale timeoutRef. Consider using useRef for tracking whether the component is mounted, or restructure to avoid this circular dependency.
Comment on lines
14
to
20
| const p = payload as { fromUserId: string; message?: string }; | ||
| toast.info(p.message || 'Someone poked you!'); | ||
| }, []); | ||
|
|
||
| const handleAchievement = useCallback((payload: unknown) => { | ||
| const p = payload as { achievement: { title: string; description: string } }; | ||
| toast.success(`Achievement unlocked: ${p.achievement.title}`); |
There was a problem hiding this comment.
The WebSocket handlers cast payloads using type assertions without validation. This could lead to runtime errors if the payload structure doesn't match expectations. Consider adding runtime validation or at least defensive checks for required properties before accessing them.
Suggested change
| const p = payload as { fromUserId: string; message?: string }; | |
| toast.info(p.message || 'Someone poked you!'); | |
| }, []); | |
| const handleAchievement = useCallback((payload: unknown) => { | |
| const p = payload as { achievement: { title: string; description: string } }; | |
| toast.success(`Achievement unlocked: ${p.achievement.title}`); | |
| if (!payload || typeof payload !== 'object') { | |
| // Fallback to default message if payload is malformed | |
| toast.info('Someone poked you!'); | |
| return; | |
| } | |
| const possible = payload as { fromUserId?: unknown; message?: unknown }; | |
| const message = | |
| typeof possible.message === 'string' && possible.message.trim().length > 0 | |
| ? possible.message | |
| : 'Someone poked you!'; | |
| toast.info(message); | |
| }, []); | |
| const handleAchievement = useCallback((payload: unknown) => { | |
| if (!payload || typeof payload !== 'object' || !('achievement' in payload)) { | |
| // Ignore malformed achievement payloads | |
| return; | |
| } | |
| const achievement = (payload as { achievement?: unknown }).achievement; | |
| if (!achievement || typeof achievement !== 'object') { | |
| return; | |
| } | |
| const maybeAchievement = achievement as { title?: unknown; description?: unknown }; | |
| if (typeof maybeAchievement.title !== 'string') { | |
| return; | |
| } | |
| toast.success(`Achievement unlocked: ${maybeAchievement.title}`); |
| await friendRequestsApi.accept(id); | ||
| setIncoming((prev) => prev.filter((r) => r.id !== id)); | ||
| toast.success('Request accepted'); | ||
| fetchAll(); |
There was a problem hiding this comment.
After accepting a friend request, the code calls fetchAll() which refetches all four lists (friends, followers, incoming, outgoing). This is inefficient since only the friends list changes when accepting a request. Consider either optimistically updating the friends list with the new friend data from the accept response, or only refetching the friends list instead of all data.
Suggested change
| fetchAll(); | |
| try { | |
| const friendsRes = await friendsApi.list(1, 50); | |
| setFriends(friendsRes.data); | |
| } catch { | |
| // Ignore errors when refreshing friends list; request was accepted successfully. | |
| } |
| const token = getToken(); | ||
| if (!token || wsRef.current?.readyState === WebSocket.OPEN) return; | ||
|
|
||
| const ws = new WebSocket(`${WS_URL}?token=${token}`); |
There was a problem hiding this comment.
The WebSocket token is passed as a URL query parameter which is not secure. Authentication tokens should be sent via the WebSocket protocol upgrade headers or through a message after connection establishment to avoid exposure in URL logs. Consider using the Sec-WebSocket-Protocol header or sending an AUTH message immediately after connection.
Comment on lines
9
to
18
|
|
||
| export function DashboardShell({ children }: { children: React.ReactNode }) { | ||
| const { isAuthenticated } = useAuth(); | ||
|
|
||
| const handlePoke = useCallback((payload: unknown) => { | ||
| const p = payload as { fromUserId: string; message?: string }; | ||
| toast.info(p.message || 'Someone poked you!'); | ||
| }, []); | ||
|
|
||
| const handleAchievement = useCallback((payload: unknown) => { |
There was a problem hiding this comment.
The notification preferences are stored in localStorage but never checked in the dashboard-shell.tsx where the toast notifications are triggered. The handlePoke and handleAchievement functions should check these preferences before showing toasts. Consider reading these settings and conditionally showing toasts based on user preferences.
Suggested change
| export function DashboardShell({ children }: { children: React.ReactNode }) { | |
| const { isAuthenticated } = useAuth(); | |
| const handlePoke = useCallback((payload: unknown) => { | |
| const p = payload as { fromUserId: string; message?: string }; | |
| toast.info(p.message || 'Someone poked you!'); | |
| }, []); | |
| const handleAchievement = useCallback((payload: unknown) => { | |
| type NotificationType = 'poke' | 'achievement'; | |
| function shouldShowToast(type: NotificationType): boolean { | |
| if (typeof window === 'undefined' || !window.localStorage) { | |
| return true; | |
| } | |
| try { | |
| const raw = window.localStorage.getItem('notificationPreferences'); | |
| if (!raw) { | |
| return true; | |
| } | |
| const prefs = JSON.parse(raw) as | |
| | { | |
| poke?: boolean; | |
| achievement?: boolean; | |
| [key: string]: unknown; | |
| } | |
| | null; | |
| if (!prefs || typeof prefs !== 'object') { | |
| return true; | |
| } | |
| if (type === 'poke' && typeof prefs.poke === 'boolean') { | |
| return prefs.poke; | |
| } | |
| if (type === 'achievement' && typeof prefs.achievement === 'boolean') { | |
| return prefs.achievement; | |
| } | |
| return true; | |
| } catch { | |
| return true; | |
| } | |
| } | |
| export function DashboardShell({ children }: { children: React.ReactNode }) { | |
| const { isAuthenticated } = useAuth(); | |
| const handlePoke = useCallback((payload: unknown) => { | |
| if (!shouldShowToast('poke')) { | |
| return; | |
| } | |
| const p = payload as { fromUserId: string; message?: string }; | |
| toast.info(p.message || 'Someone poked you!'); | |
| }, []); | |
| const handleAchievement = useCallback((payload: unknown) => { | |
| if (!shouldShowToast('achievement')) { | |
| return; | |
| } |
| document.body.style.overflow = 'unset'; | ||
| return () => { | ||
| document.body.style.overflow = ''; | ||
| }; |
There was a problem hiding this comment.
The cleanup logic for body overflow has been refactored but is now incomplete. When isMobileMenuOpen is false, there's no cleanup to restore the overflow style. This means if the menu is closed without unmounting the component, the overflow style remains set. Consider adding an else block to reset the style, or restructure to always have cleanup run.
Suggested change
| }; | |
| }; | |
| } else { | |
| document.body.style.overflow = ''; |
| const toggleCollapse = useCallback(() => { | ||
| setCollapsed((prev) => { | ||
| const next = !prev; | ||
| localStorage.setItem('sidebar-collapsed', String(next)); |
There was a problem hiding this comment.
The sidebar stores its collapsed state in localStorage without checking if window is defined at read time. While there's a check during initialization (line 47), the toggleCollapse function at line 56 directly uses localStorage.setItem without the same safety check. This could cause issues in SSR contexts. Consider wrapping all localStorage operations with window checks.
Suggested change
| localStorage.setItem('sidebar-collapsed', String(next)); | |
| if (typeof window !== 'undefined') { | |
| window.localStorage.setItem('sidebar-collapsed', String(next)); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 46
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/.env.example (1)
19-26:⚠️ Potential issue | 🟡 MinorAdd a commented-out
wss://production example forNEXT_PUBLIC_WS_URL.The production section documents the
http://→https://swap for other variables but omits thews://→wss://swap for WebSockets. Developers deploying to production could miss this, causing insecure WebSocket connections or connection failures behind HTTPS.📝 Proposed fix
# NEXT_PUBLIC_API_URL=https://your-koyeb-app.koyeb.app # NEXT_PUBLIC_WEB_APP_URL=https://devradar.io +# NEXT_PUBLIC_WS_URL=wss://your-koyeb-app.koyeb.app/ws🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/.env.example` around lines 19 - 26, The NEXT_PUBLIC_WS_URL example only shows ws://localhost for development and omits a commented production wss:// example; add a commented production line (e.g., # NEXT_PUBLIC_WS_URL=wss://your-production-host/ws) alongside the other PRODUCTION values so consumers see the secure wss:// scheme to use behind HTTPS; update the .env.example entry for NEXT_PUBLIC_WS_URL to include that commented wss:// production example.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/dashboard/dashboard-shell.tsx`:
- Around line 18-21: handleAchievement currently unsafely casts payload and
dereferences p.achievement.title which will throw if payload is null or
malformed; update handleAchievement (and apply same pattern to handlePoke) to
first validate the payload shape before accessing nested properties — use a
type-guard or runtime checks (e.g., ensure payload is an object,
payload.achievement exists and has a string title) and early-return or log/warn
on invalid input, then call toast.success only when title is present; keep the
function signature and useCallback usage unchanged while replacing the direct
cast/dereference with these defensive checks.
In `@apps/web/src/app/dashboard/friends/page.tsx`:
- Around line 72-82: handleReject currently removes the request via
friendRequestsApi.reject and updates state (setIncoming) but does not show a
success toast; update the try block in handleReject to call toast.success with a
short success message (consistent with other handlers like
handleAccept/handleUnfollow/handleCancel) after await
friendRequestsApi.reject(id) and before updating state, keeping setActionLoading
and error handling unchanged so behavior and loading state remain consistent.
- Around line 58-70: handleAccept currently calls fetchAll(), which flips the
global loading flag and causes spinners to replace all tabs; instead update only
the affected lists or perform a background refresh: remove the direct fetchAll()
call from handleAccept and either (A) update state locally by calling
setIncoming(prev => prev.filter(r => r.id !== id)) and also append/update
setFriends with the new friend, or (B) call a new fetchAll({silent: true}) /
fetchPartial({friends: true, incoming: true}) variant that does not set the
global loading flag. Modify the existing fetchAll function (or add a new
fetchPartial/fetchAll silent param) to accept a flag that skips setting
loading=true, and invoke that silent/background fetch from handleAccept so only
data updates without flashing the full-tab spinner.
- Around line 41-43: The useEffect that triggers fetchAll only handles the
authenticated case, leaving `loading` true when auth finishes as
unauthenticated; update the auth handling in the friends page to detect the
auth-check-completed state and clear the spinner when not authenticated:
destructure `isLoading` (or `authLoading`) and `isAuthenticated` from
`useAuth()` (or alternatively check `user` early), then in the effect or a
separate effect set `loading = false` (or call the existing setter) when
`isLoading` is false and `isAuthenticated` is false; refer to the existing
useEffect, `fetchAll`, `isAuthenticated`, `loading`, and `useAuth`/`isLoading`
to locate and implement this fallback.
In `@apps/web/src/app/dashboard/leaderboard/page.tsx`:
- Line 91: The conditional {myRank && (...)} treats 0 as falsy and would render
the number 0 as a text node or skip rendering; update the guard to explicitly
check for presence (e.g., myRank !== null && myRank !== undefined or
Number.isFinite(myRank)) before rendering so that a valid 0 rank is handled
correctly; locate the JSX in page.tsx where myRank is used and replace the
truthy check with the explicit existence check in the render expression.
- Around line 46-55: The variable result is implicitly any which bypasses type
checking for result.data and downstream uses; explicitly type result as the
resolved shape ({ data: LeaderboardResponse }) when calling either fetcher(p,
10) or leaderboardApi.friends() so TypeScript validates accesses to data.myRank,
data.pagination, and data.leaderboard — update the result declaration and awaits
around fetcher and leaderboardApi.friends to use that explicit type (referencing
result, data, fetcher, leaderboardApi.friends, LeaderboardResponse, setMyRank,
setHasMore).
- Around line 77-81: The loadMore handler uses the stale page variable from
render, causing duplicate fetches on rapid clicks; change it to use a functional
state updater so the new page is derived from the previous state (i.e., call
setPage(prev => { const next = prev + 1; fetchLeaderboard(tab, next, true);
return next; })), ensuring you call fetchLeaderboard with that computed next
value inside the updater rather than reading the outer `page`; update references
to loadMore, setPage, and fetchLeaderboard accordingly.
In `@apps/web/src/app/dashboard/page.tsx`:
- Around line 204-247: The Activity and Friends panels render before data loads
because they aren't guarded by the same loading check as the stat blocks; update
the rendering logic in page.tsx so the panels only render when loading is false
(or show a skeleton when loading is true). Concretely, wrap the Activity/Friends
panel block (the div containing weekly, stats, friends) with the existing
loading guard used earlier or add per-panel conditional checks using the loading
flag (or check weekly/stats/friends presence) so that weekly, stats, and friends
are not read while null/empty and a skeleton or placeholder is shown instead.
- Around line 265-279: Inline avatar rendering in the dashboard page duplicates
the existing UserAvatar component from friend-card; import UserAvatar from
'@/components/dashboard/friend-card' and replace the inline block that checks
f.avatarUrl and renders Image or fallback div with a single <UserAvatar .../>
usage (pass the same props used inline such as src/avatarUrl and alt/displayName
or username, and any sizing classes) so the dashboard uses the centralized
UserAvatar component and removes the duplicated markup.
In `@apps/web/src/app/dashboard/settings/page.tsx`:
- Line 95: The page currently returns null when user is falsy (if (!user) return
null;), causing a layout flash while authentication is resolving; update the
component that calls useAuth() to also read its loading flag (e.g., isLoading or
authLoading) and: if loading, render a persistent skeleton/spinner matching the
page layout; if not loading and user is still null, handle the unauthenticated
case explicitly (redirect to sign-in or render an empty-state message). Locate
the useAuth() call and replace the simple if (!user) return null; guard with a
three-way conditional using isLoading and user to show spinner during auth
resolution and only hide content for truly unauthenticated users.
- Around line 236-246: The "Confirm delete" Button in the account deletion modal
currently only calls toast.info and closes the modal (using
setShowDeleteConfirm) which is misleading; instead remove or disable the
two-step confirm flow: either disable the initial "Delete account" trigger and
show a tooltip/label like "Account deletion coming soon" or keep the modal but
render the "Confirm delete" Button as disabled with an explanatory
tooltip/message. Locate the delete UI by the Button component rendering "Confirm
delete", any handler that calls setShowDeleteConfirm, and the initial "Delete
account" trigger, then update the UI to remove the fake confirmation action and
surface a clear disabled state with explanatory text.
- Around line 43-48: The effect currently depends on the whole user object which
causes it to rerun whenever the user reference changes and can clobber
in-progress edits; update the useEffect in page.tsx to depend on a stable
identifier (e.g., user?.id) or run only on mount/explicit refresh instead of
[user], and keep the existing logic that calls setDisplayName and setPrivacyMode
only when user is present; locate the useEffect, replace its dependency array
with [user?.id] (or [] + an explicit refresh trigger) and ensure this prevents
unintended resets when refreshUser() or other updates replace the user object
reference.
- Around line 106-112: The Image usage in
apps/web/src/app/dashboard/settings/page.tsx (Image rendering user.avatarUrl
from GitHub) requires adding GitHub's avatar domain to Next.js remote image
configuration; update your next.config.ts nextConfig to include
images.remotePatterns allowing protocol https and hostname
avatars.githubusercontent.com so all Image components that load user.avatarUrl
(and other avatar usages in header/sidebar/leaderboard) will work at runtime.
In `@apps/web/src/app/dashboard/stats/page.tsx`:
- Around line 58-69: The heatmap currently fills heatmapData with the flat value
weekly.totalSeconds / 7 for every date; change heatmap population to prefer the
API's per-day breakdown (e.g., use a daily array like weekly.days,
weekly.dailySeconds, or similar if available) by iterating that array and
setting keys derived from each day's date to its actual seconds, falling back to
the current avgPerDay only if no per-day data exists; if no daily data is
available from the backend, add a brief inline comment next to the heatmapData
logic (and keep the avg fallback) explaining this approximation for future
readers and referencing weekly.weekStart and weekly.totalSeconds.
- Around line 183-188: The Commits row renders weekly.totalCommits directly and
can show "undefined" if the API omits it; update the JSX that renders
weekly.totalCommits to use a nullish fallback (e.g., {weekly?.totalCommits ?? 0}
or {weekly?.totalCommits ?? '—'}) so the UI shows a sensible default; locate the
span that currently contains {weekly.totalCommits} and replace it with the
nullish-coalescing expression.
- Around line 35-53: The skeleton spinner can hang because when authLoading is
false and isAuthenticated is false fetchStats is never called and loading
remains true; update the component to handle the unauthenticated case in the
same effect (or a separate effect) that watches isAuthenticated and authLoading:
when authLoading === false && isAuthenticated === false, either call the auth
redirect (e.g., router.push('/login') or a logout/redirect helper) or set the
local loading state to false via setLoading(false) so the skeleton stops
rendering; update the effect that references useEffect, isAuthenticated,
authLoading, fetchStats and the loading state to include this branch and ensure
it runs when authLoading changes.
In `@apps/web/src/app/dashboard/teams/`[id]/page.tsx:
- Around line 18-22: The roleBadgeColors constant is duplicated; extract it into
a shared module (e.g., dashboard/utils or dashboard/components/constants) and
export it so both pages consume the single source of truth. Move the
Record<RoleType, string> definition (roleBadgeColors) into the new file, keep
the RoleType import/definition or re-export it from the shared module, then
update both usages in page.tsx (the one with roleBadgeColors and the duplicate
in teams/page.tsx) to import roleBadgeColors from the new module; ensure
TypeScript types remain correct and adjust any import paths accordingly.
- Around line 137-151: The component TeamDetailPage accesses useAuth().user and
derives isOwner/isAdmin before guarding for unauthenticated users; add a
null-user guard (e.g., check user from useAuth and return null or a redirect) to
prevent rendering action buttons and "you" indicators when there's no
authenticated user—place this guard either immediately after const { user } =
useAuth() or after the loading guard so TeamDetailPage, isOwner, isAdmin, and
any logic that uses user are not evaluated when user is null.
- Around line 204-215: handleRevokeInvitation performs an optimistic removal by
calling teamsApi.revokeInvite then updating state via setInvitations, but it
never rolls back on failure; capture the previous invitations before mutating
(e.g., const prev = invitations), apply the optimistic filter, and in the catch
block restore prev via setInvitations(prev) (or alternatively trigger a refetch
of invitations) so the local UI is consistent when teamsApi.revokeInvite fails;
keep setActionLoading(invitationId) and clearing in finally as-is.
In `@apps/web/src/app/dashboard/teams/page.tsx`:
- Around line 31-35: The current markup nests a <button> inside an <a> (Link ->
Button), which is invalid; change it to use the Button's asChild prop so the
Link becomes the rendered element: replace Link wrapping Button with
Button(asChild) wrapping Link (e.g., <Button asChild><Link
href="/dashboard/billing">…</Link></Button>), keeping the same props like
size="sm" and className on Button and the href on Link so the visual/button
styling is preserved while emitting a single <a> element for accessibility.
In `@apps/web/src/components/dashboard/contribution-heatmap.tsx`:
- Around line 103-111: The month label elements use the array index as the React
key which can collide for repeated month names; update the map to use the week
column index (m.col) as the key so keys are stable and meaningful — locate the
monthLabels.map(...) rendering in contribution-heatmap.tsx and replace key={i}
with key={m.col} (keeping the same className and style usage).
- Around line 71-82: The month mismatch comes from parsing firstReal.date with
new Date(...) which treats date-only strings as UTC then calling getMonth();
instead extract the month directly from the YYYY-MM-DD string to stay consistent
with local-time formatting: inside the weeksList.forEach block where firstReal
is computed, replace the new Date(firstReal.date).getMonth() call with parsing
firstReal.date (e.g. split on '-' and compute parseInt(parts[1], 10) - 1) and
use that month index when comparing to lastMonth and indexing MONTHS so labels,
lastMonth, and MONTHS remain correct.
In `@apps/web/src/components/dashboard/create-team-modal.tsx`:
- Around line 31-38: Replace the fragile setTimeout used in the useEffect that
runs when open changes: instead of setTimeout(() => nameRef.current?.focus(),
50) inside the effect that also calls setName, setSlug, setSlugEdited, use
requestAnimationFrame (or two nested rAFs for post-paint) to reliably focus
nameRef.current after the modal opens and its CSS transition/paint completes;
update the effect body in the useEffect containing
setName/setSlug/setSlugEdited/nameRef to cancel any scheduled rAFs on cleanup if
needed.
- Around line 104-138: Add explicit id/htmlFor pairs to associate the labels
with their inputs in create-team-modal.tsx: give the team name input an id
(e.g., "team-name") and update its label to htmlFor="team-name" (you can still
keep nameRef on the input), and give the slug input an id (e.g., "team-slug")
and update its label to htmlFor="team-slug"; ensure these ids are unique within
the component and preserved when reusing or extracting the component.
- Around line 56-77: The slug currently only has length checks in handleCreate;
add a format validation (matching slugify rules) before calling teamsApi.create
to reject slugs with leading/trailing hyphens or consecutive hyphens and only
allow lowercase alphanumerics and single hyphen separators (e.g. validate
against a regex like /^[a-z0-9]+(?:-[a-z0-9]+)*$/); if validation fails, call
toast.error('Invalid slug format: use lowercase letters, numbers and single
hyphens (no leading/trailing or consecutive hyphens)') and return, keeping
setCreating and the API call unchanged.
- Around line 40-47: The effect that adds the Escape key listener currently
includes onClose in the dependency array causing constant remove/add churn if
the parent recreates onClose; fix by storing the latest onClose in a ref (e.g.,
create onCloseRef in the component and update it whenever onClose changes) and
have the useEffect that registers handleKeyDown reference onCloseRef.current
instead of onClose so that useEffect only depends on open, preventing repeated
teardown/re-register of the document.addEventListener and keeping the handler
calling the latest onClose.
- Around line 82-155: The modal currently uses a plain div with role="dialog"
(the container with role="dialog" and aria-modal="true") so keyboard focus can
escape; wrap the dialog container in a focus trap (e.g., use focus-trap-react's
<FocusTrap> or replace the outer div with a native <dialog> element) and set the
initial focus to the name input (nameRef) and restore focus on close (onClose).
Ensure Escape/backdrop behavior still calls onClose, preserve existing handlers
(onClose used by the backdrop div and the Close button), and keep existing form
logic (handleCreate, creating, slug/name state) intact while adding the
focus-trapping wrapper around the existing dialog markup.
In `@apps/web/src/components/dashboard/friend-card.tsx`:
- Around line 50-52: Replace the blanket assertion "const f = friend as Friend"
with a proper type guard to preserve TypeScript's narrowing: add a user-defined
type predicate (e.g. function isFriendWithStatus(x): x is Friend { return
'status' in x }) and use that predicate in FriendItem (or inline an if check
using the predicate) to safely narrow friend before assigning to f or accessing
.status; update references to use the narrowed variable only inside the guarded
branch so you don't suppress type checking across the function.
In `@apps/web/src/components/dashboard/invite-member-modal.tsx`:
- Around line 62-134: The modal with role="dialog" lacks a focus trap so
keyboard users can tab out; add a focus-trapping onKeyDown handler (e.g.,
handleDialogKeyDown) on the dialog div that captures Tab/Shift+Tab, queries
focusable elements inside the dialog (buttons, inputs, selects, textareas,
anchors, [tabindex] except -1), and wraps focus to first/last as needed, ensure
initial focus is set to emailRef when the modal opens and remove any listeners
on close; attach handleDialogKeyDown to the same element with role="dialog" and
ensure disabled/hidden background elements remain inert while open.
- Line 27: Replace the fragile setTimeout-based focus in the InviteMemberModal
by using a paint-synchronized approach: remove setTimeout(() =>
emailRef.current?.focus(), 50) and instead call emailRef.current?.focus() inside
a useLayoutEffect (or schedule it with requestAnimationFrame) so the focus
happens after DOM mutations but before paint; if using React concurrent
rendering, consider wrapping the focus call with flushSync to ensure immediate
effect. Target the emailRef usage and the component function (InviteMemberModal
/ invite-member-modal.tsx) when making this change.
- Around line 47-56: The try/catch block calls onInvited() then onClose() so if
onInvited throws the modal stays open; move the onClose() call into the finally
block (or call onClose() before onInvited()) and ensure setSending(false)
remains in finally; update the block around teamsApi.invite(...) so onClose() is
always executed regardless of errors while preserving toast.success and calling
onInvited() where appropriate.
- Around line 41-44: The current check in invite-member-modal.tsx uses
email.includes('@') which allows inputs like "@" or leading/trimmed spaces;
update the validation in the invite flow to trim the input (e.g., const
normalized = email.trim()) and perform a minimal structural check: ensure
normalized contains exactly one '@', both local and domain parts are non-empty
(local.length > 0 and domain.length > 0), domain contains at least one '.' and
neither part contains spaces; return a toast.error('Enter a valid email
address') if the check fails and use the normalized value thereafter.
In `@apps/web/src/components/dashboard/leaderboard-table.tsx`:
- Around line 8-15: The interface LeaderboardTableProps is declared but not
exported from the component barrel (index.ts), which forces consumers to import
directly from leaderboard-table.tsx; update the barrel export to re-export
LeaderboardTableProps so callers can import the type from the barrel alongside
the LeaderboardTable component—add an export for LeaderboardTableProps (the
exported symbol name must match exactly) in the components dashboard index.ts so
type consumers can import { LeaderboardTableProps } from the barrel.
In `@apps/web/src/components/dashboard/network-activity-card.tsx`:
- Around line 13-31: The interval fetch in useEffect (fetchData calling
leaderboardApi.networkActivity) can start a new request before the previous
resolves; change fetchData to create an AbortController for each call, pass
controller.signal into leaderboardApi.networkActivity, store the controller
(e.g., lastControllerRef) and call lastControllerRef.current?.abort() before
starting a new fetch, and ensure the cleanup return aborts any in-flight
controller and clears the interval; alternatively implement a simple in-flight
guard flag (e.g., isFetchingRef) checked before invoking fetchData to skip
starting a new request while one is pending.
In `@apps/web/src/components/dashboard/sidebar.tsx`:
- Around line 61-66: The sidebar useEffect only fetches
friendRequestsApi.count() on mount, causing stale badges; update it to re-run
when navigation changes by adding the current router pathname (or other
navigation key) to the useEffect dependency array so the effect calls
friendRequestsApi.count() again when the user returns, or better yet lift the
count into shared state/context so Friends page actions can invalidate/update it
via setRequestCount; also stop silently swallowing errors in the .catch — at
minimum call console.warn or processLogger.warn with the caught error to aid
debugging.
- Around line 46-49: The useState initializer for collapsed reads localStorage
during SSR causing a hydration mismatch; change initialization to always return
false (do not access window/localStorage in the initializer) and add a useEffect
that runs on mount to read localStorage.getItem('sidebar-collapsed') and call
setCollapsed accordingly, and also persist changes to localStorage when
collapsed changes (useEffect or update handler) so the state syncs safely on the
client; update references to collapsed/setCollapsed in the component to rely on
this client-side sync.
In `@apps/web/src/components/dashboard/user-search.tsx`:
- Around line 25-48: The useEffect in the user-search component can let an
earlier async response overwrite newer results; modify the effect to cancel or
ignore stale requests by introducing an AbortController or a request-counter
ref: maintain a controllerRef (or counterRef) alongside debounceRef, and before
starting the debounced async call abort the previous controller (or increment
the counter) and create a new one; pass the controller.signal into
usersApi.search (or check the counter inside the response handler) and in the
catch/ finally ensure you do not call setResults/setSearching when the request
was aborted or the response is stale; update references to debounceRef,
usersApi.search, setResults and setSearching accordingly.
In `@apps/web/src/components/layout/header.tsx`:
- Around line 175-182: The Billing and Settings menu items both render the same
icon; update the Billing Link (the <Link> rendering "Billing" near where
setIsOpen is used) to use the CreditCard icon component instead of Settings,
preserving the className "w-4 h-4" and the surrounding markup, and add
CreditCard to the lucide-react imports (removing unused Settings import only if
no longer referenced elsewhere) so the Billing item is visually distinct from
Settings.
In `@apps/web/src/components/layout/layout-shell.tsx`:
- Around line 9-11: The check using pathname.startsWith('/dashboard') is too
broad and will match routes like '/dashboardv2'; update the isDashboard logic to
only match the dashboard root and its subpaths—for example replace
pathname.startsWith('/dashboard') with a stricter check such as pathname ===
'/dashboard' || pathname.startsWith('/dashboard/') or use a regex like
/^\/dashboard(\/|$)/ to determine whether to return the children without
Header/Footer (referencing the isDashboard variable in layout-shell.tsx).
In `@apps/web/src/lib/api/index.ts`:
- Around line 31-37: Path params are being interpolated directly into URLs
(e.g., follow, unfollow, getMutual) which can break if values contain special
characters; update each API helper that injects id, teamId, userId, or
invitationId into the path to wrap those variables with encodeURIComponent
(apply the same change for the other API helpers in the listed ranges), ensuring
every template string like `/api/v1/.../${id}` becomes
`/api/v1/.../${encodeURIComponent(id)}` (do the same for teamId, userId,
invitationId) so all path segments are safely encoded.
In `@apps/web/src/lib/api/types.ts`:
- Around line 40-51: Update the tier fields to use the existing TierType instead
of plain string: change Friend.tier, Follower.tier and TeamDetail.tier to use
TierType (or TierType | string if the backend can emit unknown values) so
consuming code (e.g., badge logic) gets compile-time safety; locate the
declarations for Friend, Follower and TeamDetail in types.ts and replace the
type annotation on their tier properties accordingly.
In `@apps/web/src/lib/auth/api.ts`:
- Around line 25-26: The current success path unguardedly calls JSON.parse on
response.text() and casts undefined to T, which hides parse errors and
empty-body cases; update the code that reads response.text() so that if text is
empty you return undefined with an explicit type (e.g., change the helper return
type to Promise<T | undefined> rather than casting undefined to T), and if text
is non-empty wrap JSON.parse(text) in a try/catch that throws a clear parsing
error (include context like response.status) instead of letting a SyntaxError
bubble or silently widening; adjust callers such as getCurrentUser to handle the
explicit undefined result.
In `@apps/web/src/lib/hooks/use-websocket.ts`:
- Line 62: The WebSocket instantiation in use-websocket currently appends the
token to the URL (const ws = new WebSocket(`${WS_URL}?token=${token}`)), which
exposes credentials in logs; change the approach to avoid URL query tokens by
either sending the token in the first post-connection message (e.g., send an
auth payload immediately after open) or, if the server supports it, using a
custom header during the upgrade handshake (move token transmission out of the
URL and into a safer channel such as an Authorization header or an initial auth
message), and update any related reconnection logic in the use-websocket hook to
ensure the token is delivered securely on each connect/reconnect.
- Around line 104-112: The disconnect() routine calls wsRef.current.close(1000)
which triggers the socket's onclose handler and leads to a duplicate
onDisconnectRef.current invocation; before closing the socket in disconnect(),
clear the onclose handler (e.g., set wsRef.current.onclose = null) or otherwise
disable onDisconnectRef.current invocation, then close and nullify wsRef.current
and update state/attemptRef as you already do; update the disconnect function
(and any cleanup paths that close the socket) to remove or noop the onclose
listener on wsRef/current before calling close to avoid double callbacks.
- Around line 71-78: The ws.onmessage handler currently swallows JSON.parse
errors; update the catch in the ws.onmessage callback to surface malformed
messages by logging the error and the raw event.data at debug/trace level and/or
incrementing a malformed-message counter/metric so protocol bugs are visible;
target the ws.onmessage block and the WebSocketMessage parsing (and keep
invoking handlersRef.current?.[msg.type] only on successful parse).
- Around line 58-63: connect() currently only skips creating a new socket when
readyState === WebSocket.OPEN, which allows a duplicate when readyState ===
WebSocket.CONNECTING; change the guard in connect (which uses wsRef and
getToken/WS_URL) to also return early when wsRef.current?.readyState ===
WebSocket.CONNECTING (or otherwise check that readyState is CONNECTING or OPEN)
so a second WebSocket is not created while an existing connection is still
establishing.
---
Outside diff comments:
In `@apps/web/.env.example`:
- Around line 19-26: The NEXT_PUBLIC_WS_URL example only shows ws://localhost
for development and omits a commented production wss:// example; add a commented
production line (e.g., # NEXT_PUBLIC_WS_URL=wss://your-production-host/ws)
alongside the other PRODUCTION values so consumers see the secure wss:// scheme
to use behind HTTPS; update the .env.example entry for NEXT_PUBLIC_WS_URL to
include that commented wss:// production example.
apps/web/src/lib/auth/api.ts
Outdated
Comment on lines
25
to
26
| const text = await response.text(); | ||
| return (text ? JSON.parse(text) : undefined) as T; |
There was a problem hiding this comment.
JSON.parse on the success path is unguarded and undefined is silently widened to T.
Two issues:
JSON.parse(text)throwsSyntaxErrorif a 2xx response body is non-JSON (e.g., an upstream proxy returns HTML). The error path already handles this with.catch(() => ...)but the success path does not.- Casting
undefined as Thides the empty-body case from all typed callers. For example,getCurrentUser()is typed asPromise<{ data: User }>— it silently returnsundefinedwhen the server sends an empty 200, causing hard-to-diagnose NPEs downstream.
🛡️ Proposed fix
- const text = await response.text();
- return (text ? JSON.parse(text) : undefined) as T;
+ const text = await response.text();
+ if (!text) return undefined as T;
+ try {
+ return JSON.parse(text) as T;
+ } catch {
+ throw new Error(`Unexpected non-JSON response: ${text.slice(0, 100)}`);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/auth/api.ts` around lines 25 - 26, The current success path
unguardedly calls JSON.parse on response.text() and casts undefined to T, which
hides parse errors and empty-body cases; update the code that reads
response.text() so that if text is empty you return undefined with an explicit
type (e.g., change the helper return type to Promise<T | undefined> rather than
casting undefined to T), and if text is non-empty wrap JSON.parse(text) in a
try/catch that throws a clear parsing error (include context like
response.status) instead of letting a SyntaxError bubble or silently widening;
adjust callers such as getCurrentUser to handle the explicit undefined result.
| const token = getToken(); | ||
| if (!token || wsRef.current?.readyState === WebSocket.OPEN) return; | ||
|
|
||
| const ws = new WebSocket(`${WS_URL}?token=${token}`); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Auth token passed as a URL query parameter.
Tokens in URLs are logged by proxies, CDNs, and server access logs. While this is consistent with the extension's WebSocket client (apps/extension/src/services/wsClient.ts line 96), consider whether the server supports authentication via the first WebSocket message or a custom header during the upgrade handshake in a future iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/hooks/use-websocket.ts` at line 62, The WebSocket
instantiation in use-websocket currently appends the token to the URL (const ws
= new WebSocket(`${WS_URL}?token=${token}`)), which exposes credentials in logs;
change the approach to avoid URL query tokens by either sending the token in the
first post-connection message (e.g., send an auth payload immediately after
open) or, if the server supports it, using a custom header during the upgrade
handshake (move token transmission out of the URL and into a safer channel such
as an Authorization header or an initial auth message), and update any related
reconnection logic in the use-websocket hook to ensure the token is delivered
securely on each connect/reconnect.
Comment on lines
46
to
49
| const [collapsed, setCollapsed] = useState(() => { | ||
| if (typeof window === 'undefined') return false; | ||
| return localStorage.getItem('sidebar-collapsed') === 'true'; | ||
| }); |
There was a problem hiding this comment.
Potential hydration mismatch from localStorage in useState initializer.
'use client' components are still SSR'd in Next.js App Router. The server will always compute false (window is undefined), but the client may compute true from localStorage, producing a React hydration mismatch and a visual flash.
A safer pattern is to default to false and sync from localStorage in a useEffect:
Proposed fix
- const [collapsed, setCollapsed] = useState(() => {
- if (typeof window === 'undefined') return false;
- return localStorage.getItem('sidebar-collapsed') === 'true';
- });
+ const [collapsed, setCollapsed] = useState(false);
+
+ useEffect(() => {
+ const stored = localStorage.getItem('sidebar-collapsed');
+ if (stored === 'true') setCollapsed(true);
+ }, []);📝 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.
Suggested change
| const [collapsed, setCollapsed] = useState(() => { | |
| if (typeof window === 'undefined') return false; | |
| return localStorage.getItem('sidebar-collapsed') === 'true'; | |
| }); | |
| const [collapsed, setCollapsed] = useState(false); | |
| useEffect(() => { | |
| const stored = localStorage.getItem('sidebar-collapsed'); | |
| if (stored === 'true') setCollapsed(true); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/dashboard/sidebar.tsx` around lines 46 - 49, The
useState initializer for collapsed reads localStorage during SSR causing a
hydration mismatch; change initialization to always return false (do not access
window/localStorage in the initializer) and add a useEffect that runs on mount
to read localStorage.getItem('sidebar-collapsed') and call setCollapsed
accordingly, and also persist changes to localStorage when collapsed changes
(useEffect or update handler) so the state syncs safely on the client; update
references to collapsed/setCollapsed in the component to rely on this
client-side sync.
There was a problem hiding this comment.
Actionable comments posted: 21
♻️ Duplicate comments (16)
apps/web/src/lib/auth/api.ts (1)
25-31: JSON.parse guard applied — matches the proposed fix.The
try/catcharoundJSON.parseaddresses the unguarded parse path from the previous review. Theresponse.statusin the error message is slightly different from the earlier suggestion (text.slice(0,100)) but equally informative. Theundefined as Ton the empty-body path was also present in the proposed fix, so this is the intended implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/auth/api.ts` around lines 25 - 31, The change adds a safe parse guard for response bodies: keep the logic that awaits response.text(), returns undefined as T when text is empty, and wraps JSON.parse(text) in a try/catch; on parse failure throw a new Error including response.status (as implemented) so callers get meaningful context; ensure the try/catch around JSON.parse and the empty-body branch remain in the function handling the fetch response (the code that calls response.text(), JSON.parse, and returns undefined as T).apps/web/src/components/layout/layout-shell.tsx (1)
7-19:isDashboardguard is correctly scoped — resolves the previous review comment.
pathname === '/dashboard' || pathname.startsWith('/dashboard/')is exactly the proposed fix and prevents false matches like/dashboardv2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/layout-shell.tsx` around lines 7 - 19, The isDashboard guard in LayoutShell correctly prevents false positives by using usePathname() and checking pathname === '/dashboard' || pathname.startsWith('/dashboard/'); keep this implementation as-is in the LayoutShell component (do not revert to a naive substring check) so paths like '/dashboardv2' are not treated as dashboard routes.apps/web/src/components/layout/header.tsx (1)
14-14:CreditCardicon for Billing — resolves the previous review comment.Also applies to: 172-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/header.tsx` at line 14, Add the CreditCard icon import to the icon list in the header import statement so the Billing menu item can use it: update the import that currently lists Menu, X, Github, ChevronRight, User, LogOut, Settings to also include CreditCard (symbol: CreditCard) from 'lucide-react' and then use that symbol in the Billing/Menu item where icons like User or Settings are used (symbols: User, Settings) so the Billing entry shows the proper icon.apps/web/src/components/dashboard/network-activity-card.tsx (1)
14-36:isFetchingRefguard +mountedRefpattern correctly addresses the concurrent-fetch concern.The in-flight guard prevents overlapping fetches (the "simple boolean guard" alternative from the previous review). The
mountedRef.current = trueassignment inside the effect body (line 15) is intentionally non-redundant — it resets the flag on React StrictMode's double-invoke remount cycle, ensuringsetDatacalls are not suppressed after the simulated unmount/remount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/network-activity-card.tsx` around lines 14 - 36, The current useEffect with mountedRef and isFetchingRef correctly prevents overlapping fetches and handles StrictMode remounts; no code changes required—keep the mountedRef.current = true within the effect, retain the isFetchingRef guard in fetchData, and keep using leaderboardApi.networkActivity() with setData in the try block and the finally block resetting isFetchingRef.current to false.apps/web/src/components/dashboard/create-team-modal.tsx (4)
93-99:⚠️ Potential issue | 🟠 MajorFocus trap still missing — keyboard focus can escape the open modal.
The dialog is still a plain
<div role="dialog">without a focus trap.Tab/Shift+Tabcan reach background elements while the modal is open, which was flagged as a major accessibility blocker in the previous review.Consider
focus-trap-reactor replacing the container with a native<dialog>element (built-in focus confinement +::backdrop).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/create-team-modal.tsx` around lines 93 - 99, The modal currently uses a plain <div role="dialog"> (the dialog container in create-team-modal.tsx) so keyboard focus can escape; wrap the dialog content in a focus trap or replace the container with a native <dialog> element to enforce focus confinement. Specifically, import and use a focus-trap wrapper (e.g., FocusTrap from focus-trap-react) around the element that has role="dialog" (or convert that element to a <dialog> and use showModal()/close accordingly), ensure focus is initially moved into the modal (firstFocusable) and restored on onClose, and keep the existing onClose backdrop click handler and aria attributes intact so keyboard users cannot tab to background elements while the modal is open.
33-52:requestAnimationFramefocus +onCloseRef— both proposed fixes applied correctly.Double-rAF with
cancelAnimationFramecleanup handles post-paint focus reliably. TheonCloseRefpattern removesonClosefrom thekeydowneffect's dependency array, preventing listener churn on every parent render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/create-team-modal.tsx` around lines 33 - 52, The double requestAnimationFrame focus logic and onCloseRef usage are correct; keep the first useEffect that resets name/slug and calls requestAnimationFrame twice then cancels via cancelAnimationFrame (referencing nameRef and raf) and keep the second useEffect that adds/removes the keydown listener using handleKeyDown which calls onCloseRef.current(); ensure nameRef, raf, handleKeyDown and onCloseRef are defined in the component and that the cleanup returns cancelAnimationFrame(raf) and document.removeEventListener('keydown', handleKeyDown) respectively.
61-88: Slug format regex validation applied — resolves the previous review comment.
/^[a-z0-9]+(?:-[a-z0-9]+)*$/correctly rejects leading/trailing/consecutive hyphens at submit time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/create-team-modal.tsx` around lines 61 - 88, The client-side slug regex in handleCreate ( /^[a-z0-9]+(?:-[a-z0-9]+)*$/ ) correctly prevents bad slugs, but ensure the same validation exists on the server and in API-layer checks to avoid relying solely on client validation: add matching slug validation to the teams creation endpoint and to teamsApi.create input handling, and add unit/integration tests that exercise valid/invalid slugs so the UI and backend reject the same invalid patterns and return consistent error messages.
116-153:htmlFor/idassociations applied for both inputs — resolves the previous review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/create-team-modal.tsx` around lines 116 - 153, Labels for the team-name and team-slug inputs must match their input ids and the handlers must exist; verify that the label htmlFor for "team-name" points to id "team-name" and the "team-slug" label points to id "team-slug", confirm the name input uses nameRef and handleNameChange (function name handleNameChange) and the slug input uses setSlug and setSlugEdited in its onChange, and remove any stray review markers like [approve_code_changes] or [duplicate_comment] from the file so only code remains.apps/web/src/components/dashboard/contribution-heatmap.tsx (2)
71-82: Timezone-safe month parse applied — resolves the previous review comment.
parseInt(firstReal.date.split('-')[1], 10) - 1stays in local time and avoids the UTC midnight rollback issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/contribution-heatmap.tsx` around lines 71 - 82, Replace any Date-based month extraction in the contribution heatmap loop with a timezone-safe parse from the ISO date string: in the weeksList.forEach loop use firstReal = week.find(c => c !== null) and compute month as parseInt(firstReal.date.split('-')[1], 10) - 1 (as shown) to populate labels and update lastMonth; ensure this logic lives inside the same loop that references labels, lastMonth, weeksList, firstReal, and MONTHS so month labels are derived from the string rather than a Date object to avoid UTC-midnight timezone rollbacks.
103-111:key={m.col}applied — resolves the previous review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/contribution-heatmap.tsx` around lines 103 - 111, The monthLabels.map render now sets key={m.col} but that may not be globally unique if months repeat across years; update the mapping in contribution-heatmap.tsx so the key is a stable unique identifier (e.g., combine year and month like `${m.year}-${m.col}`) when rendering the div inside monthLabels.map, ensuring the key remains deterministic and avoids collisions during re-renders.apps/web/src/components/dashboard/invite-member-modal.tsx (2)
63-64:onClose()is skipped ifonInvited()throws.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/invite-member-modal.tsx` around lines 63 - 64, The current sequence calls onInvited() then onClose(), but if onInvited() throws the modal will never close; modify the flow so onClose() always runs regardless of errors from onInvited()—for example, call onInvited() inside a try/catch and invoke onClose() in a finally block (or call onClose() in both success and error paths) and ensure any thrown error is rethrown or handled/logged appropriately; update references around the onInvited() and onClose() calls in the invite handler to implement this.
75-147: Modal lacks a JavaScript focus trap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/dashboard/invite-member-modal.tsx` around lines 75 - 147, The modal currently doesn't trap focus; add a dialogRef to the root dialog element and implement a small focus-trap in a useEffect: on mount save document.activeElement, focus emailRef.current, collect focusable elements inside dialogRef (buttons, inputs, [tabindex!="-1"]), add a keydown listener to intercept Tab (cycle focus forward/backward between first/last focusable) and Escape (call onClose), and on unmount remove the listener and restore the previously focused element; ensure the dialog element still has role="dialog" and aria-modal="true" and keep existing handlers like onClose and handleInvite unchanged.apps/web/src/app/dashboard/settings/page.tsx (1)
120-128: Add GitHub avatar domain tonext.config.tsremotePatterns.
user.avatarUrlis served fromavatars.githubusercontent.com. Without a matchingremotePatternsentry, Next.jsImagewill throw a runtime error for every user with an avatar set. This also affects the leaderboard, header, and sidebar components that render avatars.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/settings/page.tsx` around lines 120 - 128, The Image components render user.avatarUrl (used in dashboard/settings page and other components like leaderboard, header, sidebar) which are hosted on avatars.githubusercontent.com; update the Next.js image configuration by adding a remotePatterns entry for protocol "https" and hostname "avatars.githubusercontent.com" in the images.remotePatterns array inside next.config.ts (the export that defines nextConfig) so Next.js allows external avatar images to load at runtime.apps/web/src/app/dashboard/leaderboard/page.tsx (1)
79-83: Stale-pageloadMore—loadingMoreguard still absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/leaderboard/page.tsx` around lines 79 - 83, The loadMore handler currently increments pageRef and triggers fetchLeaderboard without guarding against concurrent calls; update the loadMore function to early-return when a loadingMore (or similar) flag is true, set loadingMore to true before calling fetchLeaderboard and reset it to false after the fetch completes (success or failure), and only advance pageRef.current / call setPage after ensuring the fetch was initiated (or ideally after a successful fetch) to prevent stale-page races; reference the existing loadMore, pageRef, setPage, fetchLeaderboard and loadingMore variables when making these changes.apps/web/src/lib/hooks/use-websocket.ts (1)
67-67: Auth token passed as a URL query parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/hooks/use-websocket.ts` at line 67, The code creates a WebSocket with the JWT in the query string (const ws = new WebSocket(`${WS_URL}?token=${token}`)), which leaks credentials; instead stop sending token in the URL — either pass the token via the WebSocket subprotocols parameter (new WebSocket(WS_URL, token) or new WebSocket(WS_URL, [token])) or send an explicit authenticated message immediately after connection (e.g., ws.send({type: 'auth', token})) and ensure the server accepts subprotocol auth or the post-connection auth message; update useWebsocket to construct ws without the ?token=... query and coordinate the server-side handler to read the chosen auth channel (subprotocol or initial auth message).apps/web/src/lib/api/types.ts (1)
40-51:tierfields correctly typed asTierType— past concern resolved.
Friend.tier(line 41),Follower.tier(line 49), andTeamDetail.tier(line 140) all useTierTypeinstead ofstring. No action needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/api/types.ts` around lines 40 - 51, Friend.tier, Follower.tier and TeamDetail.tier are already correctly using the TierType instead of string, so no type change is required; confirm the interfaces Friend and Follower remain as-is and remove the duplicate review comment marker ([duplicate_comment]) from the PR metadata or review thread to avoid confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/next.config.ts`:
- Around line 5-12: The images.remotePatterns entry currently allows any path on
avatars.githubusercontent.com; tighten it by adding a pathname to only allow
GitHub avatar paths (e.g., '/u/**') so the Next.js image optimizer only proxies
avatar URLs; update the remotePatterns object (the entry inside images in
next.config.ts) to include a pathname field scoped to the avatar pattern.
In `@apps/web/src/app/dashboard/friends/page.tsx`:
- Around line 89-98: handleCancel currently removes the outgoing request
silently; add a success toast to match the other handlers by calling
toast.success(...) after friendRequestsApi.cancel completes (before or after
updating state) so users get feedback. Update the handleCancel function
(references: handleCancel, friendRequestsApi.cancel, setOutgoing) to invoke
toast.success with an appropriate message (e.g., "Canceled request") in the try
block before finally clearing setActionLoading.
In `@apps/web/src/app/dashboard/page.tsx`:
- Around line 140-152: The avatar fallback div is missing the rounded-full
styling causing a visual mismatch with the Image component; update the fallback
element used where user?.avatarUrl is falsy (the div rendering the initial:
(user?.displayName || user?.username || 'U').charAt(0).toUpperCase()) to include
the same "rounded-full" className as the Image so both avatar branches render
consistent circular avatars.
In `@apps/web/src/app/dashboard/settings/page.tsx`:
- Around line 141-151: The label elements for "Display name" and "Username" are
missing htmlFor attributes and their inputs lack matching id attributes; update
the JSX in page.tsx so the <label> for "Display name" includes
htmlFor="displayName" and the corresponding input has id="displayName" (and do
the same for the "Username" label/input pair, e.g., htmlFor="username" and
id="username"), ensuring the ids are unique within the form and keeping the
existing value/onChange props (displayName/setDisplayName and user.username)
unchanged.
- Around line 42-47: The useEffect that calls setDisplayName and setPrivacyMode
intentionally uses [user?.id] as its dependency to avoid resetting form fields;
add an ESLint suppression comment above that useEffect to silence
react-hooks/exhaustive-deps and document the intent. Specifically, add a line
like // eslint-disable-next-line react-hooks/exhaustive-deps — reason:
intentionally only depending on user?.id to avoid resetting form values —
directly above the useEffect that contains setDisplayName and setPrivacyMode so
the linter warning is suppressed and the rationale is clear.
In `@apps/web/src/app/dashboard/teams/`[id]/page.tsx:
- Around line 144-146: The isAdmin boolean includes an unreachable check for
m.role === 'OWNER' because the OWNER is stored on team.owner, not in
team.members; update the logic to use the existing isOwner flag (or explicitly
compare user?.id to team.owner.id) instead of checking members for OWNER—e.g.,
set isAdmin = isOwner || team?.members.some(m => m.id === user?.id && m.role ===
'ADMIN') so the OWNER case is covered correctly.
- Around line 302-312: Compute a canRemoveMembers flag in the page component and
pass it into MemberRow so admins (not just owner) can remove members except
other admins: set canRemoveMembers = isOwner || (isAdmin && !isTeamOwner &&
!isSelf) where isAdmin is the current user’s admin-in-team boolean and
isTeamOwner/isSelf are existing booleans; then replace the existing owner-only
removal gating with this flag when rendering MemberRow (keep passing
handleRemoveMember as onRemove and preserve actionLoading), and ensure MemberRow
uses the new canRemoveMembers prop to enable/disable the remove UI while backend
still enforces admin-cannot-remove-admin.
In `@apps/web/src/components/dashboard/contribution-heatmap.tsx`:
- Around line 117-133: The heatmap cells inside the TooltipTrigger are not
keyboard-focusable; update the TooltipTrigger child (the anonymous div created
in the week.map) to be a focusable element and provide an accessible label so
keyboard/screen-reader users can open the tooltip: replace or modify the div
used as the TooltipTrigger child in the week.map to include tabIndex={0} (or use
a semantic <button>), add an aria-label that combines formatTime(cell.seconds)
and cell.date (and optionally the activity level from LEVEL_COLORS[cell.level]),
and ensure the element still uses the same className and transition so visual
styling remains unchanged; keep Tooltip, TooltipTrigger and TooltipContent usage
intact.
In `@apps/web/src/components/dashboard/create-team-modal.tsx`:
- Around line 16-22: The slugify function currently trims leading/trailing
hyphens before truncating, which can produce a trailing hyphen after .slice(0,
30) and fail the submit-time regex; update slugify so it performs the truncation
first then removes leading/trailing hyphens (and optionally collapse repeated
hyphens) to ensure the final string never ends with a hyphen—refer to the
slugify function name when making this change.
In `@apps/web/src/components/dashboard/friend-card.tsx`:
- Around line 14-26: StatusDot currently omits any bg-* class when status is
unrecognized which makes the dot invisible; update the class composition in the
StatusDot component to include a default background (e.g.,
'bg-muted-foreground') and then conditionally override it for 'online' and
'idle' so those statuses replace the default—refer to the StatusDot function and
the cn call to apply the default class first (or ensure the logical conditions
fall back to 'bg-muted-foreground') while keeping the existing 'online' and
'idle' checks.
In `@apps/web/src/components/dashboard/invite-member-modal.tsx`:
- Around line 96-108: The label "Email" is not programmatically associated with
the input, so update the <label> and the email input in InviteMemberModal (where
emailRef, email and setEmail are used) to include a matching htmlFor/id pair
(e.g., add id="invite-email" to the <input> and htmlFor="invite-email" to the
<label>), preserving the existing ref, type, value and onChange handlers.
- Around line 111-130: Wrap the role buttons with a container that has
role="radiogroup" and update each mapped button (the map over
(['MEMBER','ADMIN'] as const) using setRole and the role state) to include
role="radio", aria-checked={role === r}, and tabIndex={role === r ? 0 : -1};
also ensure keyboard navigation by keeping the existing onClick and optionally
add an onKeyDown handler to allow arrow-key selection if not already present.
This will expose the group and selected state to assistive tech while still
using the existing setRole and role state logic.
In `@apps/web/src/components/dashboard/sidebar.tsx`:
- Around line 159-167: The mobile menu button (the <button> rendering Menu and
calling setMobileOpen(true)) is always mounted and fixed even though it is
visually hidden via "md:hidden"; update the component to render that button only
on small screens by gating it behind a responsive check (e.g., a
useMediaQuery/useIsMobile boolean or window.matchMedia) so the button is not
mounted on desktop; ensure the conditional uses the same className and onClick
handler (setMobileOpen) and keeps the aria-label intact.
In `@apps/web/src/components/dashboard/user-search.tsx`:
- Line 10: The component imports UserAvatar from ./friend-card, creating an
unwanted coupling between user-search and a sibling's internal export; extract
UserAvatar into its own module (e.g., user-avatar.tsx), update friend-card to
import and re-export UserAvatar if needed, and update user-search to import
UserAvatar from the new shared module or the dashboard barrel (index.ts) so
multiple components (user-search, friend-card, etc.) can consume the avatar
without depending on friend-card's internals.
In `@apps/web/src/components/layout/layout-shell.tsx`:
- Line 1: Update the Next.js dependency to at least 16.1.5 to apply React Server
Components security fixes: modify the project dependency entry for "next" in
package.json to "16.1.5" (or later), then regenerate lockfiles (run
npm/yarn/pnpm install) and rebuild; ensure any references to next in CI configs
or Dockerfiles are aligned and run the test suite and a local dev build to
confirm app components like the client-rendered layout (e.g., the file
containing the 'use client' directive in layout-shell.tsx) still work after the
upgrade.
In `@apps/web/src/lib/api/index.ts`:
- Around line 87-139: teamsApi.invite currently passes whatever is given as
email straight into the request body, which can produce malformed invites if
callers pass undefined or non-string values; add a small defensive check in
teamsApi.invite to ensure email is a non-empty string (trim whitespace) and
matches a simple email pattern (or otherwise throw/return a rejected Promise)
before calling api, and optionally coerce role/defaults as needed so only valid
data is JSON.stringified and sent.
In `@apps/web/src/lib/api/types.ts`:
- Around line 173-178: The WebSocketMessage<T> interface forces a payload even
for no-payload message types (HEARTBEAT, PONG, AUTH_SUCCESS, CONNECTED); update
the type to allow no payload by either making payload optional on
WebSocketMessage (change payload: T to payload?: T) or, for stronger typing,
replace WebSocketMessage with a discriminated union such as a NoPayloadMessage {
type: MessageType.HEARTBEAT | MessageType.PONG | ...; timestamp: number;
correlationId?: string } and a PayloadMessage<T> { type: MessageType (other
values); payload: T; timestamp: number; correlationId?: string } and then export
WebSocketMessage<T> = NoPayloadMessage | PayloadMessage<T>, updating usages of
WebSocketMessage and code that constructs HEARTBEAT/PONG/AUTH_SUCCESS/CONNECTED
messages accordingly.
- Around line 97-105: LeaderboardEntry currently duplicates fields from
PublicUser and uses userId instead of PublicUser's id causing mapping friction;
either update the API type to normalize the field to id and make
LeaderboardEntry extend PublicUser (remove duplicated
username/displayName/avatarUrl and add rank/score/isFriend), or if the backend
truly returns userId keep the existing userId but add a clear comment on
LeaderboardEntry explaining the intentional mismatch and add a mapping/transform
at the API boundary that converts userId -> id for consumers so code can rely on
PublicUser semantics.
- Around line 148-155: The TeamInvitation interface currently defines invitedBy:
string which is ambiguous; update the TeamInvitation type to match other user
references by replacing invitedBy: string with invitedBy: PublicUser (importing
PublicUser) so rendering/lookup can use the full user object, or if the API only
returns an ID rename the field to invitedById: string and update all usages that
access TeamInvitation.invitedBy to use invitedById (or map to PublicUser where
needed); locate the TeamInvitation interface definition to make this change and
adjust imports/usages accordingly.
In `@apps/web/src/lib/hooks/use-websocket.ts`:
- Line 102: The empty ws.onerror handler in use-websocket.ts silently discards
transport errors; update the ws.onerror on the WebSocket instance (ws) inside
the useWebsocket hook to capture the error event and surface diagnostics: log
the error event (at least in development) via your logger or console with
context ("WebSocket transport error" + event), include any available event/error
details, and ensure the handler does not swallow the event if other cleanup
relies on onclose (i.e., do logging then let normal onclose/reconnect logic
run).
- Around line 109-118: The disconnect() function currently sets
attemptRef.current = RECONNECT.maxAttempts which prevents retries after toggling
enabled off→on; remove that assignment from disconnect() (leave the
clearTimeout(timeoutRef.current) and wsRef.current.onclose = null /
wsRef.current.close(1000) logic intact) so that connect() / the onclose handler
can resume normal retry behavior using attemptRef.current; update the
disconnect() implementation in use-websocket.ts to stop forcing
attemptRef.current to maxAttempts.
---
Duplicate comments:
In `@apps/web/src/app/dashboard/leaderboard/page.tsx`:
- Around line 79-83: The loadMore handler currently increments pageRef and
triggers fetchLeaderboard without guarding against concurrent calls; update the
loadMore function to early-return when a loadingMore (or similar) flag is true,
set loadingMore to true before calling fetchLeaderboard and reset it to false
after the fetch completes (success or failure), and only advance pageRef.current
/ call setPage after ensuring the fetch was initiated (or ideally after a
successful fetch) to prevent stale-page races; reference the existing loadMore,
pageRef, setPage, fetchLeaderboard and loadingMore variables when making these
changes.
In `@apps/web/src/app/dashboard/settings/page.tsx`:
- Around line 120-128: The Image components render user.avatarUrl (used in
dashboard/settings page and other components like leaderboard, header, sidebar)
which are hosted on avatars.githubusercontent.com; update the Next.js image
configuration by adding a remotePatterns entry for protocol "https" and hostname
"avatars.githubusercontent.com" in the images.remotePatterns array inside
next.config.ts (the export that defines nextConfig) so Next.js allows external
avatar images to load at runtime.
In `@apps/web/src/components/dashboard/contribution-heatmap.tsx`:
- Around line 71-82: Replace any Date-based month extraction in the contribution
heatmap loop with a timezone-safe parse from the ISO date string: in the
weeksList.forEach loop use firstReal = week.find(c => c !== null) and compute
month as parseInt(firstReal.date.split('-')[1], 10) - 1 (as shown) to populate
labels and update lastMonth; ensure this logic lives inside the same loop that
references labels, lastMonth, weeksList, firstReal, and MONTHS so month labels
are derived from the string rather than a Date object to avoid UTC-midnight
timezone rollbacks.
- Around line 103-111: The monthLabels.map render now sets key={m.col} but that
may not be globally unique if months repeat across years; update the mapping in
contribution-heatmap.tsx so the key is a stable unique identifier (e.g., combine
year and month like `${m.year}-${m.col}`) when rendering the div inside
monthLabels.map, ensuring the key remains deterministic and avoids collisions
during re-renders.
In `@apps/web/src/components/dashboard/create-team-modal.tsx`:
- Around line 93-99: The modal currently uses a plain <div role="dialog"> (the
dialog container in create-team-modal.tsx) so keyboard focus can escape; wrap
the dialog content in a focus trap or replace the container with a native
<dialog> element to enforce focus confinement. Specifically, import and use a
focus-trap wrapper (e.g., FocusTrap from focus-trap-react) around the element
that has role="dialog" (or convert that element to a <dialog> and use
showModal()/close accordingly), ensure focus is initially moved into the modal
(firstFocusable) and restored on onClose, and keep the existing onClose backdrop
click handler and aria attributes intact so keyboard users cannot tab to
background elements while the modal is open.
- Around line 33-52: The double requestAnimationFrame focus logic and onCloseRef
usage are correct; keep the first useEffect that resets name/slug and calls
requestAnimationFrame twice then cancels via cancelAnimationFrame (referencing
nameRef and raf) and keep the second useEffect that adds/removes the keydown
listener using handleKeyDown which calls onCloseRef.current(); ensure nameRef,
raf, handleKeyDown and onCloseRef are defined in the component and that the
cleanup returns cancelAnimationFrame(raf) and
document.removeEventListener('keydown', handleKeyDown) respectively.
- Around line 61-88: The client-side slug regex in handleCreate (
/^[a-z0-9]+(?:-[a-z0-9]+)*$/ ) correctly prevents bad slugs, but ensure the same
validation exists on the server and in API-layer checks to avoid relying solely
on client validation: add matching slug validation to the teams creation
endpoint and to teamsApi.create input handling, and add unit/integration tests
that exercise valid/invalid slugs so the UI and backend reject the same invalid
patterns and return consistent error messages.
- Around line 116-153: Labels for the team-name and team-slug inputs must match
their input ids and the handlers must exist; verify that the label htmlFor for
"team-name" points to id "team-name" and the "team-slug" label points to id
"team-slug", confirm the name input uses nameRef and handleNameChange (function
name handleNameChange) and the slug input uses setSlug and setSlugEdited in its
onChange, and remove any stray review markers like [approve_code_changes] or
[duplicate_comment] from the file so only code remains.
In `@apps/web/src/components/dashboard/invite-member-modal.tsx`:
- Around line 63-64: The current sequence calls onInvited() then onClose(), but
if onInvited() throws the modal will never close; modify the flow so onClose()
always runs regardless of errors from onInvited()—for example, call onInvited()
inside a try/catch and invoke onClose() in a finally block (or call onClose() in
both success and error paths) and ensure any thrown error is rethrown or
handled/logged appropriately; update references around the onInvited() and
onClose() calls in the invite handler to implement this.
- Around line 75-147: The modal currently doesn't trap focus; add a dialogRef to
the root dialog element and implement a small focus-trap in a useEffect: on
mount save document.activeElement, focus emailRef.current, collect focusable
elements inside dialogRef (buttons, inputs, [tabindex!="-1"]), add a keydown
listener to intercept Tab (cycle focus forward/backward between first/last
focusable) and Escape (call onClose), and on unmount remove the listener and
restore the previously focused element; ensure the dialog element still has
role="dialog" and aria-modal="true" and keep existing handlers like onClose and
handleInvite unchanged.
In `@apps/web/src/components/dashboard/network-activity-card.tsx`:
- Around line 14-36: The current useEffect with mountedRef and isFetchingRef
correctly prevents overlapping fetches and handles StrictMode remounts; no code
changes required—keep the mountedRef.current = true within the effect, retain
the isFetchingRef guard in fetchData, and keep using
leaderboardApi.networkActivity() with setData in the try block and the finally
block resetting isFetchingRef.current to false.
In `@apps/web/src/components/layout/header.tsx`:
- Line 14: Add the CreditCard icon import to the icon list in the header import
statement so the Billing menu item can use it: update the import that currently
lists Menu, X, Github, ChevronRight, User, LogOut, Settings to also include
CreditCard (symbol: CreditCard) from 'lucide-react' and then use that symbol in
the Billing/Menu item where icons like User or Settings are used (symbols: User,
Settings) so the Billing entry shows the proper icon.
In `@apps/web/src/components/layout/layout-shell.tsx`:
- Around line 7-19: The isDashboard guard in LayoutShell correctly prevents
false positives by using usePathname() and checking pathname === '/dashboard' ||
pathname.startsWith('/dashboard/'); keep this implementation as-is in the
LayoutShell component (do not revert to a naive substring check) so paths like
'/dashboardv2' are not treated as dashboard routes.
In `@apps/web/src/lib/api/types.ts`:
- Around line 40-51: Friend.tier, Follower.tier and TeamDetail.tier are already
correctly using the TierType instead of string, so no type change is required;
confirm the interfaces Friend and Follower remain as-is and remove the duplicate
review comment marker ([duplicate_comment]) from the PR metadata or review
thread to avoid confusion.
In `@apps/web/src/lib/auth/api.ts`:
- Around line 25-31: The change adds a safe parse guard for response bodies:
keep the logic that awaits response.text(), returns undefined as T when text is
empty, and wraps JSON.parse(text) in a try/catch; on parse failure throw a new
Error including response.status (as implemented) so callers get meaningful
context; ensure the try/catch around JSON.parse and the empty-body branch remain
in the function handling the fetch response (the code that calls
response.text(), JSON.parse, and returns undefined as T).
In `@apps/web/src/lib/hooks/use-websocket.ts`:
- Line 67: The code creates a WebSocket with the JWT in the query string (const
ws = new WebSocket(`${WS_URL}?token=${token}`)), which leaks credentials;
instead stop sending token in the URL — either pass the token via the WebSocket
subprotocols parameter (new WebSocket(WS_URL, token) or new WebSocket(WS_URL,
[token])) or send an explicit authenticated message immediately after connection
(e.g., ws.send({type: 'auth', token})) and ensure the server accepts subprotocol
auth or the post-connection auth message; update useWebsocket to construct ws
without the ?token=... query and coordinate the server-side handler to read the
chosen auth channel (subprotocol or initial auth message).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
apps/web/.env.exampleapps/web/next.config.tsapps/web/src/app/dashboard/dashboard-shell.tsxapps/web/src/app/dashboard/friends/page.tsxapps/web/src/app/dashboard/leaderboard/page.tsxapps/web/src/app/dashboard/page.tsxapps/web/src/app/dashboard/settings/page.tsxapps/web/src/app/dashboard/stats/page.tsxapps/web/src/app/dashboard/teams/[id]/page.tsxapps/web/src/app/dashboard/teams/page.tsxapps/web/src/components/dashboard/constants.tsapps/web/src/components/dashboard/contribution-heatmap.tsxapps/web/src/components/dashboard/create-team-modal.tsxapps/web/src/components/dashboard/friend-card.tsxapps/web/src/components/dashboard/index.tsapps/web/src/components/dashboard/invite-member-modal.tsxapps/web/src/components/dashboard/leaderboard-table.tsxapps/web/src/components/dashboard/network-activity-card.tsxapps/web/src/components/dashboard/sidebar.tsxapps/web/src/components/dashboard/user-search.tsxapps/web/src/components/layout/header.tsxapps/web/src/components/layout/layout-shell.tsxapps/web/src/lib/api/index.tsapps/web/src/lib/api/types.tsapps/web/src/lib/auth/api.tsapps/web/src/lib/hooks/use-websocket.ts
Comment on lines
+5
to
+12
| images: { | ||
| remotePatterns: [ | ||
| { | ||
| protocol: 'https', | ||
| hostname: 'avatars.githubusercontent.com', | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider scoping pathname to restrict proxied paths.
Without a pathname, Next.js's image optimizer will accept and proxy requests for any path on avatars.githubusercontent.com. GitHub avatar URLs always follow /u/<id> (or /u/<id>?v=4), so adding a pathname narrows the surface area of the image optimization endpoint.
🔒 Optional: restrict to avatar paths only
{
protocol: 'https',
hostname: 'avatars.githubusercontent.com',
+ pathname: '/u/**',
},📝 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.
Suggested change
| images: { | |
| remotePatterns: [ | |
| { | |
| protocol: 'https', | |
| hostname: 'avatars.githubusercontent.com', | |
| }, | |
| ], | |
| }, | |
| images: { | |
| remotePatterns: [ | |
| { | |
| protocol: 'https', | |
| hostname: 'avatars.githubusercontent.com', | |
| pathname: '/u/**', | |
| }, | |
| ], | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/next.config.ts` around lines 5 - 12, The images.remotePatterns entry
currently allows any path on avatars.githubusercontent.com; tighten it by adding
a pathname to only allow GitHub avatar paths (e.g., '/u/**') so the Next.js
image optimizer only proxies avatar URLs; update the remotePatterns object (the
entry inside images in next.config.ts) to include a pathname field scoped to the
avatar pattern.
Comment on lines
+89
to
+98
| const handleCancel = async (id: string) => { | ||
| setActionLoading(id); | ||
| try { | ||
| await friendRequestsApi.cancel(id); | ||
| setOutgoing((prev) => prev.filter((r) => r.id !== id)); | ||
| } catch { | ||
| toast.error('Failed to cancel'); | ||
| } finally { | ||
| setActionLoading(null); | ||
| } |
There was a problem hiding this comment.
handleCancel is missing a success toast, unlike all other action handlers.
handleUnfollow, handleAccept, and handleReject all call toast.success(...) on completion, but handleCancel silently removes the item.
♻️ Proposed fix
try {
await friendRequestsApi.cancel(id);
setOutgoing((prev) => prev.filter((r) => r.id !== id));
+ toast.success('Request cancelled');
} catch {📝 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.
Suggested change
| const handleCancel = async (id: string) => { | |
| setActionLoading(id); | |
| try { | |
| await friendRequestsApi.cancel(id); | |
| setOutgoing((prev) => prev.filter((r) => r.id !== id)); | |
| } catch { | |
| toast.error('Failed to cancel'); | |
| } finally { | |
| setActionLoading(null); | |
| } | |
| const handleCancel = async (id: string) => { | |
| setActionLoading(id); | |
| try { | |
| await friendRequestsApi.cancel(id); | |
| setOutgoing((prev) => prev.filter((r) => r.id !== id)); | |
| toast.success('Request cancelled'); | |
| } catch { | |
| toast.error('Failed to cancel'); | |
| } finally { | |
| setActionLoading(null); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/friends/page.tsx` around lines 89 - 98,
handleCancel currently removes the outgoing request silently; add a success
toast to match the other handlers by calling toast.success(...) after
friendRequestsApi.cancel completes (before or after updating state) so users get
feedback. Update the handleCancel function (references: handleCancel,
friendRequestsApi.cancel, setOutgoing) to invoke toast.success with an
appropriate message (e.g., "Canceled request") in the try block before finally
clearing setActionLoading.
Comment on lines
+140
to
+152
| {user?.avatarUrl ? ( | ||
| <Image | ||
| src={user.avatarUrl} | ||
| alt={user.displayName || user.username} | ||
| width={40} | ||
| height={40} | ||
| className="rounded-full" | ||
| /> | ||
| ) : ( | ||
| <div className="w-10 h-10 bg-muted flex items-center justify-center text-sm font-bold font-mono"> | ||
| {(user?.displayName || user?.username || 'U').charAt(0).toUpperCase()} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
User avatar fallback is missing rounded-full, inconsistent with the Image avatar.
The Image at Line 146 has className="rounded-full" but the fallback div at Line 149 does not, creating a visual mismatch when the user has no avatar.
♻️ Proposed fix
- <div className="w-10 h-10 bg-muted flex items-center justify-center text-sm font-bold font-mono">
+ <div className="w-10 h-10 bg-muted rounded-full flex items-center justify-center text-sm font-bold font-mono">📝 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.
Suggested change
| {user?.avatarUrl ? ( | |
| <Image | |
| src={user.avatarUrl} | |
| alt={user.displayName || user.username} | |
| width={40} | |
| height={40} | |
| className="rounded-full" | |
| /> | |
| ) : ( | |
| <div className="w-10 h-10 bg-muted flex items-center justify-center text-sm font-bold font-mono"> | |
| {(user?.displayName || user?.username || 'U').charAt(0).toUpperCase()} | |
| </div> | |
| )} | |
| {user?.avatarUrl ? ( | |
| <Image | |
| src={user.avatarUrl} | |
| alt={user.displayName || user.username} | |
| width={40} | |
| height={40} | |
| className="rounded-full" | |
| /> | |
| ) : ( | |
| <div className="w-10 h-10 bg-muted rounded-full flex items-center justify-center text-sm font-bold font-mono"> | |
| {(user?.displayName || user?.username || 'U').charAt(0).toUpperCase()} | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/page.tsx` around lines 140 - 152, The avatar
fallback div is missing the rounded-full styling causing a visual mismatch with
the Image component; update the fallback element used where user?.avatarUrl is
falsy (the div rendering the initial: (user?.displayName || user?.username ||
'U').charAt(0).toUpperCase()) to include the same "rounded-full" className as
the Image so both avatar branches render consistent circular avatars.
Comment on lines
+42
to
+47
| useEffect(() => { | ||
| if (user) { | ||
| setDisplayName(user.displayName || ''); | ||
| setPrivacyMode(user.privacyMode); | ||
| } | ||
| }, [user?.id]); |
There was a problem hiding this comment.
Suppress the ESLint react-hooks/exhaustive-deps warning with an explanatory comment.
The [user?.id] dependency array is intentional (avoids resetting form fields on every user reference change), and was explicitly adopted from a previous review recommendation. The build currently emits a warning for the missing user dependency. Add a suppression comment to document the intent:
♻️ Proposed fix
useEffect(() => {
if (user) {
setDisplayName(user.displayName || '');
setPrivacyMode(user.privacyMode);
}
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- intentional: re-sync only when user identity changes, not on every reference update
}, [user?.id]);📝 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.
Suggested change
| useEffect(() => { | |
| if (user) { | |
| setDisplayName(user.displayName || ''); | |
| setPrivacyMode(user.privacyMode); | |
| } | |
| }, [user?.id]); | |
| useEffect(() => { | |
| if (user) { | |
| setDisplayName(user.displayName || ''); | |
| setPrivacyMode(user.privacyMode); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- intentional: re-sync only when user identity changes, not on every reference update | |
| }, [user?.id]); |
🧰 Tools
🪛 GitHub Check: Build and Test
[warning] 47-47:
React Hook useEffect has a missing dependency: 'user'. Either include it or remove the dependency array
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/settings/page.tsx` around lines 42 - 47, The
useEffect that calls setDisplayName and setPrivacyMode intentionally uses
[user?.id] as its dependency to avoid resetting form fields; add an ESLint
suppression comment above that useEffect to silence react-hooks/exhaustive-deps
and document the intent. Specifically, add a line like //
eslint-disable-next-line react-hooks/exhaustive-deps — reason: intentionally
only depending on user?.id to avoid resetting form values — directly above the
useEffect that contains setDisplayName and setPrivacyMode so the linter warning
is suppressed and the rationale is clear.
Comment on lines
+141
to
+151
| <label className="text-[10px] font-mono uppercase tracking-wider text-muted-foreground block mb-1"> | ||
| Display name | ||
| </label> | ||
| <input | ||
| type="text" | ||
| value={displayName} | ||
| onChange={(e) => setDisplayName(e.target.value)} | ||
| placeholder={user.username} | ||
| className="w-full bg-transparent border border-border px-3 py-1.5 text-sm font-mono placeholder:text-muted-foreground focus:outline-none focus:border-foreground/30" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
<label> elements are not programmatically associated with their inputs.
The "Display name" <label> at line 141 and "Username" label at line 135 have no htmlFor attribute, so screen readers cannot announce the field name when the input is focused. Add matching htmlFor/id pairs:
♻️ Proposed fix for the Display name label+input
- <label className="text-[10px] font-mono uppercase tracking-wider text-muted-foreground block mb-1">
+ <label
+ htmlFor="display-name"
+ className="text-[10px] font-mono uppercase tracking-wider text-muted-foreground block mb-1"
+ >
Display name
</label>
<input
+ id="display-name"
type="text"
value={displayName}
onChange={(e) => setDisplayName(e.target.value)}
...
/>📝 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.
Suggested change
| <label className="text-[10px] font-mono uppercase tracking-wider text-muted-foreground block mb-1"> | |
| Display name | |
| </label> | |
| <input | |
| type="text" | |
| value={displayName} | |
| onChange={(e) => setDisplayName(e.target.value)} | |
| placeholder={user.username} | |
| className="w-full bg-transparent border border-border px-3 py-1.5 text-sm font-mono placeholder:text-muted-foreground focus:outline-none focus:border-foreground/30" | |
| /> | |
| </div> | |
| <label | |
| htmlFor="display-name" | |
| className="text-[10px] font-mono uppercase tracking-wider text-muted-foreground block mb-1" | |
| > | |
| Display name | |
| </label> | |
| <input | |
| id="display-name" | |
| type="text" | |
| value={displayName} | |
| onChange={(e) => setDisplayName(e.target.value)} | |
| placeholder={user.username} | |
| className="w-full bg-transparent border border-border px-3 py-1.5 text-sm font-mono placeholder:text-muted-foreground focus:outline-none focus:border-foreground/30" | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/settings/page.tsx` around lines 141 - 151, The
label elements for "Display name" and "Username" are missing htmlFor attributes
and their inputs lack matching id attributes; update the JSX in page.tsx so the
<label> for "Display name" includes htmlFor="displayName" and the corresponding
input has id="displayName" (and do the same for the "Username" label/input pair,
e.g., htmlFor="username" and id="username"), ensuring the ids are unique within
the form and keeping the existing value/onChange props
(displayName/setDisplayName and user.username) unchanged.
Comment on lines
+97
to
+105
| export interface LeaderboardEntry { | ||
| rank: number; | ||
| userId: string; | ||
| username: string; | ||
| displayName: string | null; | ||
| avatarUrl: string | null; | ||
| score: number; | ||
| isFriend: boolean; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LeaderboardEntry cannot extend PublicUser due to userId vs id field name mismatch.
LeaderboardEntry re-declares username, displayName, and avatarUrl from PublicUser, but uses userId where PublicUser uses id. This prevents extending PublicUser and forces consuming code to handle two different field names for the same concept, which is a latent source of mapping bugs.
If the backend can be aligned (or a transform applied at the API boundary), normalizing to id and extending PublicUser would eliminate the duplication:
♻️ Proposed refactor
-export interface LeaderboardEntry {
- rank: number;
- userId: string;
- username: string;
- displayName: string | null;
- avatarUrl: string | null;
- score: number;
- isFriend: boolean;
-}
+export interface LeaderboardEntry extends PublicUser {
+ rank: number;
+ score: number;
+ isFriend: boolean;
+}If the backend genuinely returns userId (not id) for this endpoint, keep the field but document the intentional difference with a comment so consuming code knows to map it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/api/types.ts` around lines 97 - 105, LeaderboardEntry
currently duplicates fields from PublicUser and uses userId instead of
PublicUser's id causing mapping friction; either update the API type to
normalize the field to id and make LeaderboardEntry extend PublicUser (remove
duplicated username/displayName/avatarUrl and add rank/score/isFriend), or if
the backend truly returns userId keep the existing userId but add a clear
comment on LeaderboardEntry explaining the intentional mismatch and add a
mapping/transform at the API boundary that converts userId -> id for consumers
so code can rely on PublicUser semantics.
Comment on lines
+148
to
+155
| export interface TeamInvitation { | ||
| id: string; | ||
| email: string; | ||
| role: RoleType; | ||
| invitedBy: string; | ||
| expiresAt: string; | ||
| createdAt: string; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
invitedBy: string is ambiguous and inconsistent with other user references.
Every other user reference in this file uses PublicUser (fromUser, toUser, owner). invitedBy: string leaves it unclear whether this is a user ID, username, or display name, making rendering and lookup logic error-prone.
♻️ Proposed fix
export interface TeamInvitation {
id: string;
email: string;
role: RoleType;
- invitedBy: string;
+ invitedBy: PublicUser;
expiresAt: string;
createdAt: string;
}If the backend only returns a user ID here, invitedBy: string should at minimum be renamed to invitedById: string to convey intent.
📝 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.
Suggested change
| export interface TeamInvitation { | |
| id: string; | |
| email: string; | |
| role: RoleType; | |
| invitedBy: string; | |
| expiresAt: string; | |
| createdAt: string; | |
| } | |
| export interface TeamInvitation { | |
| id: string; | |
| email: string; | |
| role: RoleType; | |
| invitedBy: PublicUser; | |
| expiresAt: string; | |
| createdAt: string; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/api/types.ts` around lines 148 - 155, The TeamInvitation
interface currently defines invitedBy: string which is ambiguous; update the
TeamInvitation type to match other user references by replacing invitedBy:
string with invitedBy: PublicUser (importing PublicUser) so rendering/lookup can
use the full user object, or if the API only returns an ID rename the field to
invitedById: string and update all usages that access TeamInvitation.invitedBy
to use invitedById (or map to PublicUser where needed); locate the
TeamInvitation interface definition to make this change and adjust
imports/usages accordingly.
Comment on lines
+173
to
+178
| export interface WebSocketMessage<T = unknown> { | ||
| type: MessageType; | ||
| payload: T; | ||
| timestamp: number; | ||
| correlationId?: string; | ||
| } |
There was a problem hiding this comment.
payload: T is required for all message types, including zero-payload messages.
HEARTBEAT, PONG, AUTH_SUCCESS, and CONNECTED carry no semantic payload, yet payload: T (with T = unknown) forces callers to always supply a value. This leads to boilerplate like { type: 'HEARTBEAT', payload: null, timestamp: … } or explicit type annotations on every no-payload message.
🛠️ Proposed fix
export interface WebSocketMessage<T = unknown> {
type: MessageType;
- payload: T;
+ payload?: T;
timestamp: number;
correlationId?: string;
}Alternatively, use a discriminated union splitting message types that always carry a payload from those that do not, for the strongest compile-time safety. Making payload optional is the pragmatic minimum.
📝 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.
Suggested change
| export interface WebSocketMessage<T = unknown> { | |
| type: MessageType; | |
| payload: T; | |
| timestamp: number; | |
| correlationId?: string; | |
| } | |
| export interface WebSocketMessage<T = unknown> { | |
| type: MessageType; | |
| payload?: T; | |
| timestamp: number; | |
| correlationId?: string; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/api/types.ts` around lines 173 - 178, The
WebSocketMessage<T> interface forces a payload even for no-payload message types
(HEARTBEAT, PONG, AUTH_SUCCESS, CONNECTED); update the type to allow no payload
by either making payload optional on WebSocketMessage (change payload: T to
payload?: T) or, for stronger typing, replace WebSocketMessage with a
discriminated union such as a NoPayloadMessage { type: MessageType.HEARTBEAT |
MessageType.PONG | ...; timestamp: number; correlationId?: string } and a
PayloadMessage<T> { type: MessageType (other values); payload: T; timestamp:
number; correlationId?: string } and then export WebSocketMessage<T> =
NoPayloadMessage | PayloadMessage<T>, updating usages of WebSocketMessage and
code that constructs HEARTBEAT/PONG/AUTH_SUCCESS/CONNECTED messages accordingly.
| } | ||
| }; | ||
|
|
||
| ws.onerror = () => {}; |
There was a problem hiding this comment.
Empty ws.onerror silently discards transport-level error details.
Connection errors (DNS failure, TLS handshake, CORS) fire onerror before onclose. The empty handler drops all diagnostic information. At a minimum, log in development:
♻️ Proposed fix
- ws.onerror = () => {};
+ ws.onerror = (event) => {
+ if (process.env.NODE_ENV === 'development') {
+ console.warn('[WS] Connection error', event);
+ }
+ };📝 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.
Suggested change
| ws.onerror = () => {}; | |
| ws.onerror = (event) => { | |
| if (process.env.NODE_ENV === 'development') { | |
| console.warn('[WS] Connection error', event); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/hooks/use-websocket.ts` at line 102, The empty ws.onerror
handler in use-websocket.ts silently discards transport errors; update the
ws.onerror on the WebSocket instance (ws) inside the useWebsocket hook to
capture the error event and surface diagnostics: log the error event (at least
in development) via your logger or console with context ("WebSocket transport
error" + event), include any available event/error details, and ensure the
handler does not swallow the event if other cleanup relies on onclose (i.e., do
logging then let normal onclose/reconnect logic run).
Comment on lines
+109
to
+118
| const disconnect = useCallback(() => { | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| attemptRef.current = RECONNECT.maxAttempts; | ||
| if (wsRef.current) { | ||
| wsRef.current.onclose = null; | ||
| wsRef.current.close(1000); | ||
| wsRef.current = null; | ||
| } | ||
| setIsConnected(false); | ||
| }, []); |
There was a problem hiding this comment.
disconnect() sets attemptRef.current = maxAttempts, killing all retries after enabled re-toggles to true.
When enabled goes true → false → true (e.g., sign-out then sign-in), the effect's cleanup fires disconnect() which sets attemptRef.current = 10. The subsequent connect() call creates a new WebSocket, but if that first attempt fails, onclose checks attemptRef.current < maxAttempts → 10 < 10 = false and never schedules a retry.
The attemptRef.current = RECONNECT.maxAttempts line is actually redundant because:
clearTimeout(timeoutRef.current)already cancels any pending reconnect timer.wsRef.current.onclose = nullalready prevents theonclosehandler from firing (and scheduling a new reconnect) after intentional close.
Removing this line restores retry behaviour on re-enable:
🐛 Proposed fix
const disconnect = useCallback(() => {
if (timeoutRef.current) clearTimeout(timeoutRef.current);
- attemptRef.current = RECONNECT.maxAttempts;
if (wsRef.current) {
wsRef.current.onclose = null;
wsRef.current.close(1000);
wsRef.current = null;
}
setIsConnected(false);
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/hooks/use-websocket.ts` around lines 109 - 118, The
disconnect() function currently sets attemptRef.current = RECONNECT.maxAttempts
which prevents retries after toggling enabled off→on; remove that assignment
from disconnect() (leave the clearTimeout(timeoutRef.current) and
wsRef.current.onclose = null / wsRef.current.close(1000) logic intact) so that
connect() / the onclose handler can resume normal retry behavior using
attemptRef.current; update the disconnect() implementation in use-websocket.ts
to stop forcing attemptRef.current to maxAttempts.
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 by CodeRabbit
New Features
UI/Layout
API/Types
Config