github#87
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR focuses on hardening GitHub-related functionality (notably adding ETag-based caching and switching README retrieval to GitHub App installation tokens), along with a few frontend robustness/type fixes and dependency cleanup.
Changes:
- Update backend GitHub routes to use ETag-aware requests and fetch README content via GitHub App installation access tokens.
- Add defensive array-normalization in note presence and notes layout code paths to avoid crashes when inputs aren’t arrays.
- Remove the unused
@reduxjs/toolkitdependency and mark a performance audit task as completed.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/useNotePresence.ts | Adds normalization guards around presence payloads and active user state. |
| src/components/views/ActivityLogView.tsx | Adds explicit parameter typing for Chart.js callbacks to satisfy TS. |
| src/components/notes/NotesLayout.tsx | Adds safety checks around folders/notes to prevent non-array runtime errors. |
| package.json | Removes @reduxjs/toolkit dependency. |
| package-lock.json | Updates lockfile to reflect dependency removal. |
| MD/PERFORMANCE_AUDIT.md | Marks the GitHub ETags audit item as completed. |
| backend/routes/github.js | Adds ETag usage for GitHub API calls and switches README fetching to installation tokens. |
Comments suppressed due to low confidence (1)
src/components/views/ActivityLogView.tsx:415
- Typing the Chart.js tooltip callback parameter as
anyfixes the TS error but removes type safety. Prefer using Chart.js provided types (e.g.,TooltipItem<'doughnut'>) orunknown+ narrowing so future refactors don’t silently break tooltip rendering.
label: (ctx: any) => {
const v = Number(ctx.raw) || 0;
const m = Math.round(v / 60);
return `${ctx.label}: ${m} min`;
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -615,22 +624,24 @@ router.get('/readme', verifyToken, async (req, res) => { | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
getInstallationAccessToken() ultimately reads the GitHub App private key from process.env.GITHUB_APP_PRIVATE_KEY, but the repo’s env template/documentation uses GITHUB_PRIVATE_KEY. As-is, this route can crash at runtime when generating the JWT. Align the env var name (or add fallback) and update .env.example accordingly.
| const githubAppPrivateKey = | |
| process.env.GITHUB_APP_PRIVATE_KEY || process.env.GITHUB_PRIVATE_KEY; | |
| if (!githubAppPrivateKey) { | |
| console.error('GitHub App private key is not configured. Set GITHUB_APP_PRIVATE_KEY or GITHUB_PRIVATE_KEY.'); | |
| return res.status(500).json({ message: 'GitHub App is not configured' }); | |
| } |
| color: T.text3, | ||
| font: { family: 'DM Mono', size: 10 }, | ||
| callback: (v) => `${v}m`, | ||
| callback: (v: any) => `${v}m`, |
There was a problem hiding this comment.
The Y-axis tick callback is typed as any. Chart.js exposes a typed signature for tick callbacks (value is typically string | number). Using the proper type (or unknown with narrowing) will preserve type safety and avoid masking integration issues.
| callback: (v: any) => `${v}m`, | |
| callback: (v: string | number) => `${v}m`, |
|
|
||
| const now = Date.now(); | ||
| const filteredUsers = users.filter(u => { | ||
| const usersArray = (Array.isArray(users) ? users : Object.values(users || {})) as ActiveUser[]; |
There was a problem hiding this comment.
If the presence_update payload is sometimes an object (as the new normalization suggests), the earlier users.map(...) debug log will still throw before this usersArray conversion runs. Normalize to usersArray first and use usersArray for any .map()/.filter()/logging.
| const response = await fetchWithEtag( | ||
| `https://api.github.com/repos/${owner}/${repo}/readme`, | ||
| { |
There was a problem hiding this comment.
owner/repo are interpolated directly into the GitHub URL. Validate they are strings and URL-encode them (e.g., encodeURIComponent) to avoid malformed requests and to prevent cache-key confusion with reserved characters like /, ?, or spaces.
| const readmeCacheKey = `readme_${uid}_${owner}_${repo}_${installationId}`; | ||
|
|
There was a problem hiding this comment.
githubCache is an unbounded in-memory Map, and this route introduces cache entries keyed by (uid, owner, repo, installationId), which can grow without bound on a long-running server. Consider adding TTL and/or a max-size (LRU) eviction strategy.
| "@radix-ui/react-tooltip": "^1.2.8", | ||
| "@react-email/components": "^1.0.7", | ||
| "@react-email/tailwind": "^2.0.4", | ||
| "@reduxjs/toolkit": "^2.5.0", | ||
| "@tailwindcss/postcss": "^4.1.18", | ||
| "@tanstack/query-sync-storage-persister": "^5.96.1", |
There was a problem hiding this comment.
After removing @reduxjs/toolkit, it appears Redux-related deps are no longer used (no imports found in src/). If react-redux / redux-persist are truly unused, consider removing them as well to reduce install/bundle size and avoid relying on auto-installed peer deps like redux.
Description
Type of Change
Related Issues
Screenshots (if applicable)
Testing
Checklist