Sidebar performance improvements#95
Conversation
WalkthroughThis PR consolidates workspace configuration fetching by creating a unified Changes
Sequence DiagramsequenceDiagram
participant Sidebar as Sidebar Component
participant Router as Next Router
participant NewAPI as /api/workspace/[id]/<br/>settings/general/config
participant OldAPIs as /api/workspace/<br/>[id]/(multiple)
rect rgb(240, 248, 255)
Note over Sidebar,OldAPIs: OLD BEHAVIOR (Before PR)
Sidebar->>OldAPIs: fetch guides config
OldAPIs-->>Sidebar: guides
Sidebar->>OldAPIs: fetch allies config
OldAPIs-->>Sidebar: allies
Sidebar->>OldAPIs: fetch sessions config
OldAPIs-->>Sidebar: sessions
Sidebar->>OldAPIs: fetch notices config
OldAPIs-->>Sidebar: notices
Sidebar->>OldAPIs: fetch leaderboard config
OldAPIs-->>Sidebar: leaderboard
Sidebar->>OldAPIs: fetch policies config
OldAPIs-->>Sidebar: policies
Sidebar->>Sidebar: Render with flags
end
rect rgb(240, 255, 240)
Note over Sidebar,NewAPI: NEW BEHAVIOR (After PR)
Sidebar->>Router: Compute workspace groupId
Sidebar->>NewAPI: fetch all configs in parallel<br/>(guides, allies, sessions,<br/>notices, leaderboard, policies)
par Parallel Config Fetches
NewAPI->>NewAPI: getConfig(guides)
NewAPI->>NewAPI: getConfig(allies)
NewAPI->>NewAPI: getConfig(sessions)
NewAPI->>NewAPI: getConfig(notices)
NewAPI->>NewAPI: getConfig(leaderboard)
NewAPI->>NewAPI: getConfig(policies)
end
NewAPI-->>Sidebar: { guides, allies, sessions,<br/>notices, leaderboard, policies }
Sidebar->>Sidebar: Render with consolidated flags
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pages/api/activity/session.ts (1)
72-76: The[id]placeholder issue affects database storage here.The
picturefield is set togetThumbnail(userid)and then stored in the database via upsert (line 76). This will persist the literal[id]placeholder in the user's picture field.components/sidebar.tsx (1)
333-341: Dead code:.replace("[id]", ...)is no longer needed.Since
page.hrefnow contains the actual workspace ID (e.g.,/workspace/123/wall), the.replace("[id]", workspace.groupId.toString())calls are redundant and will have no effect.-router.asPath === page.href.replace("[id]", workspace.groupId.toString()) +router.asPath === page.hrefThis applies to both line 333 and line 341.
pages/api/@me.ts (1)
23-27: Type definition does not match returned data structure.The
Datatype definesgroupthumbnailandgroupname(lowercase), but the actual return at lines 89-93 usesgroupThumbnailandgroupName(camelCase). This mismatch will cause TypeScript to not catch property access errors in consuming code.workspaces?: { groupId: number - groupthumbnail: string - groupname: string + groupThumbnail: string + groupName: string }[]
🧹 Nitpick comments (6)
pages/workspace/[id]/index.tsx (1)
264-269: Beta badge rendering implemented correctly.The conditional rendering of the beta badge is well-implemented with appropriate styling. The badge will only display when
widgetConfig.betais true.Minor: Consider consistent indentation.
Lines 265-269 use tabs while the rest of the file uses spaces. Consider using consistent indentation throughout.
- </div> + </div>pages/api/workspace/[id]/staff/search/[username].ts (1)
32-37: TheasyncandPromise.allare now redundant.Since
getThumbnailis now synchronous (returns a string directly), theasynccallback andPromise.allwrapper are unnecessary overhead. Consider simplifying:-const infoUsers = await Promise.all(users.map(async (user: any) => { - return { - username: user.username, - thumbnail: getThumbnail(user.userid) - } -})) +const infoUsers = users.map((user: any) => ({ + username: user.username, + thumbnail: getThumbnail(user.userid) +}))pages/workspace/[id]/alliances/manage/[aid].tsx (1)
75-83: Theasynccallback is now redundant.Since
getThumbnailis synchronous, theasynckeyword in the map callback serves no purpose. The same applies to lines 102-111 and 130-140.-const infoUsers: any = await Promise.all( - users.map(async (user: any) => { - return { - ...user, - userid: Number(user.userid), - thumbnail: getThumbnail(user.userid), - }; - }) -); +const infoUsers: any = users.map((user: any) => ({ + ...user, + userid: Number(user.userid), + thumbnail: getThumbnail(user.userid), +}));pages/workspace/[id]/alliances/index.tsx (1)
71-79: Redundant async wrapper can be simplified.Same as other files - the
asyncandPromise.allare unnecessary for synchronousgetThumbnail.pages/api/workspace/[id]/settings/general/configuration.ts (1)
20-44: Add error handling for robustness.The endpoint lacks try-catch error handling. If
getConfigorparseIntfails, the response will be a 500 without useful context.async function handler( req: NextApiRequest, res: NextApiResponse<Data> ) { if (req.method !== "GET") return res.status(405).json({ success: false, error: "Method not allowed" }); if(!req.session.userid) return res.status(401).json({ success: false, error: "Not logged in" }); + const groupId = parseInt(req.query.id as string); + if (isNaN(groupId)) return res.status(400).json({ success: false, error: "Invalid workspace ID" }); + + try { const configuration = await Promise.all([ - getConfig("guides", parseInt(req.query.id as string)), - getConfig("allies", parseInt(req.query.id as string)), - getConfig("sessions", parseInt(req.query.id as string)), - getConfig("notices", parseInt(req.query.id as string)), - getConfig("leaderboard", parseInt(req.query.id as string)), - getConfig("policies", parseInt(req.query.id as string)), + getConfig("guides", groupId), + getConfig("allies", groupId), + getConfig("sessions", groupId), + getConfig("notices", groupId), + getConfig("leaderboard", groupId), + getConfig("policies", groupId), ]) const keys = ["guides", "allies", "sessions", "notices", "leaderboard", "policies"]; return res.status(200).json({ success: true, value: configuration.reduce((acc, curr, index) => { acc[keys[index]] = curr; return acc; }, {} as Record<string, any>) }) + } catch (error) { + console.error("Error fetching configuration:", error); + return res.status(500).json({ success: false, error: "Failed to fetch configuration" }); + } }pages/api/@me.ts (1)
34-41: Consider cleanup for hot-reload scenarios.In development with hot module replacement, this interval can accumulate across reloads. Consider adding a guard or cleanup mechanism:
+let cleanupInterval: NodeJS.Timeout | null = null; +if (!cleanupInterval) { -setInterval(() => { + cleanupInterval = setInterval(() => { const now = Date.now(); for (const [key, value] of userCache.entries()) { if (now - value.timestamp > CACHE_DURATION) { userCache.delete(key); } } }, 60000); +}Alternatively, for serverless deployments, this pattern may not be necessary since the in-memory cache lifetime is limited anyway.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
components/sidebar.tsx(2 hunks)package.json(1 hunks)pages/api/@me.ts(3 hunks)pages/api/activity/session.ts(1 hunks)pages/api/auth/checkOwner.ts(0 hunks)pages/api/auth/login.ts(1 hunks)pages/api/setupworkspace.ts(1 hunks)pages/api/workspace/[id].ts(2 hunks)pages/api/workspace/[id]/index.ts(0 hunks)pages/api/workspace/[id]/profile/[uid].ts(1 hunks)pages/api/workspace/[id]/settings/general/configuration.ts(1 hunks)pages/api/workspace/[id]/staff/search/[username].ts(1 hunks)pages/workspace/[id]/alliances/index.tsx(2 hunks)pages/workspace/[id]/alliances/manage/[aid].tsx(3 hunks)pages/workspace/[id]/index.tsx(3 hunks)pages/workspace/[id]/profile/[uid].tsx(1 hunks)pages/workspace/[id]/views.tsx(1 hunks)utils/permissionsManager.ts(1 hunks)utils/userinfoEngine.ts(1 hunks)
💤 Files with no reviewable changes (2)
- pages/api/auth/checkOwner.ts
- pages/api/workspace/[id]/index.ts
🧰 Additional context used
🧬 Code graph analysis (13)
pages/api/setupworkspace.ts (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/workspace/[id]/profile/[uid].tsx (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
utils/permissionsManager.ts (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/auth/login.ts (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/workspace/[id]/staff/search/[username].ts (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/activity/session.ts (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/workspace/[id].ts (1)
utils/configEngine.ts (1)
getConfig(7-24)
pages/workspace/[id]/alliances/manage/[aid].tsx (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/workspace/[id]/settings/general/configuration.ts (2)
utils/configEngine.ts (1)
getConfig(7-24)lib/withSession.ts (1)
withSessionRoute(41-47)
pages/workspace/[id]/alliances/index.tsx (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/workspace/[id]/views.tsx (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/workspace/[id]/profile/[uid].ts (1)
utils/userinfoEngine.ts (1)
getThumbnail(19-21)
pages/api/@me.ts (3)
utils/userinfoEngine.ts (3)
getUsername(8-17)getDisplayName(23-32)getThumbnail(19-21)utils/database.ts (2)
user(14-14)role(14-14)types/index.d.ts (1)
User(13-18)
🔇 Additional comments (12)
package.json (1)
3-3: LGTM! Version bump looks appropriate.The version increment from 2.1.6beta7 to 2.1.6beta10 aligns with the beta release cadence for this PR's changes.
pages/workspace/[id]/index.tsx (2)
44-44: LGTM! Beta flag properly added to interface.The optional
beta?: booleanfield is correctly typed and will support the new beta badge feature.
87-101: Beta badge implementation looks good.The policies and compliance widgets are appropriately marked as beta, and the descriptions have been cleaned up (removed "(BETA)" suffix). The implementation aligns with the PR objectives.
pages/api/setupworkspace.ts (1)
297-303: LGTM with the same caveat about thumbnail URL.The change aligns with the synchronous
getThumbnailrefactor. The returned URL contains a literal[id]placeholder which should be resolved on the client side or by the routing layer.pages/workspace/[id]/alliances/manage/[aid].tsx (1)
102-111: Consider simplifying the nested async pattern.The
infoRepsmapping still usesawait getUsername()which requires async, but you could separate the sync and async operations for clarity. However, the current implementation is functional.pages/workspace/[id]/alliances/index.tsx (1)
89-107: LGTM - async is still needed here.The
getUsernamecall on line 96 is async, soPromise.allandasyncare correctly retained in this mapping.pages/api/workspace/[id].ts (2)
61-108: Good optimization with parallel data fetching.The
Promise.allbatch fetch significantly improves performance by parallelizing multiple database and API calls. This aligns well with the PR's performance improvement goals.
134-151: LGTM - Settings response structure is clean.The response properly aggregates all fetched config values with sensible defaults using optional chaining and nullish coalescing.
components/sidebar.tsx (1)
128-163: Good refactor to dynamic workspace URLs.The hardcoded
[id]placeholders are replaced with actualworkspace.groupIdvalues, making the URLs directly usable without runtime replacement.pages/workspace/[id]/views.tsx (1)
320-320: No action needed. ThegetThumbnailfunction correctly returns/api/workspace/[id]/avatar/${userId}. The avatar API route atpages/api/workspace/[id]/avatar/[userid]/index.tsonly uses the[userid]parameter and ignores the[id]placeholder, which is the intended design. The literal[id]string in the URL is not problematic; Next.js routes it to the correct handler, which successfully serves avatars based on the userId. This pattern is used consistently throughout the codebase and functions as designed.Likely an incorrect or invalid review comment.
pages/api/@me.ts (2)
61-68: LGTM!Good use of
Promise.allto parallelize independent async operations, improving response latency.
83-95: Good error handling for external API calls.The per-role error handling with
.catch()fallbacks prevents a single failed group lookup from failing the entire request. The parallel fetching is well-structured.
| picture: getThumbnail(userId), | ||
| username: await getUsername(userId), | ||
| registered: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Redundant getUsername call; reuse the existing variable.
username was already fetched at line 66 and is still in scope. Calling getUsername(userId) again is wasteful.
data: {
picture: getThumbnail(userId),
- username: await getUsername(userId),
+ username,
registered: 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.
| picture: getThumbnail(userId), | |
| username: await getUsername(userId), | |
| registered: true | |
| picture: getThumbnail(userId), | |
| username, | |
| registered: true |
🤖 Prompt for AI Agents
In pages/api/@me.ts around lines 109 to 111, the code calls await
getUsername(userId) again even though username was already fetched at line 66
and remains in scope; replace that redundant call with the existing username
variable (ensure the variable name is correct and in scope) to avoid an
unnecessary async call and duplicate work.
| ).length; | ||
|
|
||
| const avatar = await getThumbnail(user.userid); | ||
| const avatar = getThumbnail(user.userid); |
There was a problem hiding this comment.
The [id] placeholder issue affects the API response.
The avatar field (line 243) will contain the literal [id] placeholder. However, this route has access to the actual workspace ID via workspaceGroupId (line 17), so the URL could be corrected here.
Consider replacing the placeholder:
- const avatar = getThumbnail(user.userid);
+ const avatar = `/api/workspace/${workspaceGroupId}/avatar/${user.userid}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const avatar = getThumbnail(user.userid); | |
| const avatar = `/api/workspace/${workspaceGroupId}/avatar/${user.userid}`; |
🤖 Prompt for AI Agents
In pages/api/workspace/[id]/profile/[uid].ts around line 199,
getThumbnail(user.userid) is producing an avatar URL that still contains the
literal `[id]` placeholder; use the actual workspace ID available as
workspaceGroupId (declared around line 17) when building the thumbnail URL.
Replace the call to getThumbnail so it receives or constructs a URL that
substitutes workspaceGroupId for `[id]` (e.g., pass workspaceGroupId into
getThumbnail or build the corrected path before calling) so the returned avatar
contains the real workspace ID instead of the placeholder.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.