chore: prod-code hardening (auth, convex split, cover letter UI) - #8
chore: prod-code hardening (auth, convex split, cover letter UI)#8Aditya190803 wants to merge 6 commits into
Conversation
…, docx export - Gate Gemini-backed routes with getAuthenticatedUser and checkRateLimit - Add shared cover-letter tone/length options for API validation - Extend route tests with 401 coverage
Split recent history, preferences, and result panels; page ~700→~500 LOC.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthrough
ChangesConvex Domain Modules, User-Scoped Access, API Auth/Rate-Limit, Cover-Letter UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
- Require userId in getById and delete mutations; reject cross-user access - Return full doc from saveAnalysis instead of partial shape - Use by_lookup index prefix + filter for getAnalysis/getCoverLetter - Fix searchHistory pagination with numeric cursor and over-fetch - Remove redundant avgScore from userStats
- Update convex-server wrappers to pass userId to getById/delete helpers - Normalize all function path references to convexFunctions map - Update history, resumes, and user-data routes for new signatures - Drop avgScore from user stats contract - Fix import spacing in analyze route - Update route and convex-server tests for ownership checks
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/dashboard/cover-letter/page.tsx (1)
49-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-validate persisted
tone/lengthbefore hydrating state.These casts only change the TypeScript type. A stale or tampered local/server draft can still contain invalid strings, and
restoreDraft()will push them into state unchecked even thoughsrc/lib/cover-letter-options.tsalready exposes runtime guards for this contract. Normalize those fields before callingsetTone/setLength.Also applies to: 126-133, 210-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/dashboard/cover-letter/page.tsx` around lines 49 - 56, The persisted draft handling in restoreDraft() is only using TypeScript casts, so invalid tone/length values can still slip into state from stale or tampered storage. Re-validate the parsed draft fields before calling setTone and setLength, using the existing runtime guards from src/lib/cover-letter-options.ts to normalize or ignore bad values. Apply the same validation wherever the draft is read back for hydration so the CoverLetter state only receives supported CoverLetterTone and CoverLetterLength values.
🧹 Nitpick comments (6)
test/app/api/match-score/route.test.ts (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a 429 regression case for the new rate-limit guard.
Lines 19-20 stub
checkRateLimitas allowed for every test, so this suite still never exercises the new short-circuit branch. A small 429 case here would catch regressions where the handler stops honoring rate-limit denials.Suggested test
it('returns 401 when not authenticated', async () => { vi.mocked(getAuthenticatedUser).mockResolvedValue(null); const req = new NextRequest('http://localhost', { method: 'POST', body: JSON.stringify({ resumeText: 'R', jobDescription: 'J' }), }); const res = await POST(req); expect(res.status).toBe(401); }); + + it('returns 429 when rate limited', async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, remaining: 0, resetIn: 1000 }); + const req = new NextRequest('http://localhost', { + method: 'POST', + body: JSON.stringify({ resumeText: 'R', jobDescription: 'J' }), + }); + const res = await POST(req); + expect(res.status).toBe(429); + });Also applies to: 23-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/app/api/match-score/route.test.ts` around lines 17 - 20, The match-score route tests always mock checkRateLimit as allowed, so the new rate-limit short-circuit in the route handler is never exercised. Add a dedicated 429 regression test in route.test.ts for the match-score handler that overrides the default checkRateLimit mock to return denied, then assert the handler responds with 429 and does not continue into the normal success path. Use the existing getAuthenticatedUser, checkRateLimit, and the route handler invocation in this suite to locate the setup and keep the new case isolated from the default beforeEach behavior.convex/tailoredResumes.ts (1)
100-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse both fields in
by_userId_builderSlug.convex/tailoredResumes.ts:102-103binds onlyuserIdin the composite index and filtersbuilderSlugafterward, so this still scans every resume for that user. BindbuilderSluginwithIndex(...)to keep the lookup narrow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/tailoredResumes.ts` around lines 100 - 103, The lookup in tailoredResumes is only using the by_userId_builderSlug index for userId and then filtering builderSlug afterward, which makes the query broader than intended. Update the query in the docs retrieval path to bind both userId and builderSlug directly in withIndex(...) on by_userId_builderSlug, using the existing tailoredResumes query chain, so the index fully narrows the result set.test/app/api/export-cover-letter-docx/route.test.ts (1)
12-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new 429 branch.
checkRateLimitis mocked, but no test verifies the route returnsRATE_LIMITEDwhen it denies the request.Proposed test
it('returns 401 when not authenticated', async () => { vi.mocked(getAuthenticatedUser).mockResolvedValue(null); const req = new NextRequest('http://localhost', { method: 'POST', body: JSON.stringify({ coverLetter: 'Hello' }), }); const res = await POST(req); expect(res.status).toBe(401); }); + + it('returns 429 when rate limited', async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, remaining: 0, resetIn: 1000 }); + const req = new NextRequest('http://localhost', { + method: 'POST', + body: JSON.stringify({ coverLetter: 'Hello' }), + }); + const res = await POST(req); + expect(res.status).toBe(429); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/app/api/export-cover-letter-docx/route.test.ts` around lines 12 - 36, The POST route tests for export-cover-letter-docx are missing coverage for the rate-limit failure path. Add a test in the existing describe('/api/export-cover-letter-docx') suite that mocks checkRateLimit to return allowed: false and asserts POST returns 429 with the RATE_LIMITED response, alongside the existing getAuthenticatedUser and checkRateLimit mocks.test/app/api/resumes/route.test.ts (2)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the scoped delete call too.
This only verifies the lookup receives
userId; a regression in thedeleteResume(resumeId, userId)route call would still pass.Proposed fix
-import { getResumeById } from '`@/lib/convex-server`'; +import { deleteResume, getResumeById } from '`@/lib/convex-server`'; ... expect(res.status).toBe(200); expect(data.success).toBe(true); expect(getResumeById).toHaveBeenCalledWith('res1', 'u1'); + expect(deleteResume).toHaveBeenCalledWith('res1', 'u1');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/app/api/resumes/route.test.ts` at line 61, The test for the resume route currently only verifies the scoped lookup via getResumeById, so it can miss a regression in the delete path. Update the route test to also assert that deleteResume is called with both the resumeId and the authenticated userId, using the existing resume route handler and the deleteResume mock so the delete action is covered explicitly.
64-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the cross-user test with scoped lookup behavior.
With
getResumeById(resumeId, userId), another user’s resume should come back asnull, so this mismatched-owner mock no longer reflects the real data-access contract.Proposed fix
- vi.mocked(getResumeById).mockResolvedValue({ _id: 'res1', userId: 'u2' } as never); + vi.mocked(getResumeById).mockResolvedValue(null); ... - expect(res.status).toBe(403); - expect(data.code).toBe('FORBIDDEN'); + expect(res.status).toBe(404); + expect(data.code).toBe('RESUME_NOT_FOUND');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/app/api/resumes/route.test.ts` around lines 64 - 75, Update the DELETE resume test to match the scoped lookup contract used by getResumeById(resumeId, userId): when a resume belongs to another user, the mocked lookup should return null rather than a mismatched record. Adjust the cross-user case in route.test.ts so it verifies the forbidden/not-found behavior through the scoped access path in DELETE without relying on an owner-mismatch object.test/app/api/extract-skills/route.test.ts (1)
16-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for rate-limit failures.
This route now has a new 429 path, but the tests only exercise auth, success, and validation.
Proposed test
it('returns 401 when not authenticated', async () => { vi.mocked(getAuthenticatedUser).mockResolvedValue(null); const req = new NextRequest('http://localhost', { method: 'POST', body: JSON.stringify({ resumeText: 'I know JS' }), }); const res = await POST(req); expect(res.status).toBe(401); }); + + it('returns 429 when rate limited', async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, remaining: 0, resetIn: 1000 }); + const req = new NextRequest('http://localhost', { + method: 'POST', + body: JSON.stringify({ resumeText: 'I know JS' }), + }); + const res = await POST(req); + expect(res.status).toBe(429); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/app/api/extract-skills/route.test.ts` around lines 16 - 60, Add a test for the new 429 rate-limit path in the /api/extract-skills route. Extend the existing POST test coverage in route.test.ts by mocking checkRateLimit to return allowed: false and asserting that POST responds with 429 and the expected rate-limit message. Use the same setup patterns already used in the describe('/api/extract-skills') block and reference POST, checkRateLimit, and getAuthenticatedUser so the new test fits the current mocking style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convex/analyses.ts`:
- Around line 4-107: Public Convex functions in saveAnalysis, getAnalysis,
getAnalysisById, deleteAnalysis, and getUserAnalyses currently trust userId from
args, which allows impersonation. Update these handlers to derive the caller
identity from ctx.auth.getUserIdentity() and use that user identifier for all
reads/writes/deletes, or convert them to internal/server-only functions if
client access is not required. Keep the existing query/mutation structure in
convex/analyses.ts, but remove any authorization decisions based solely on
args.userId.
In `@convex/lib/parseMatchScore.ts`:
- Around line 2-17: parseMatchScore currently only attempts JSON.parse on the
raw result, so markdown-fenced JSON responses like ```json ... ``` fall through
to the legacy % regex and return null. Update parseMatchScore to normalize
fenced JSON before parsing, matching the handling in analysis-normalizer, then
keep the existing fallback to the % pattern if parsing still fails; ensure the
parsed object’s matchScore is returned when present.
In `@convex/searchHistory.ts`:
- Around line 21-37: Pagination in searchHistory is dropping older records
because the queries on analyses, coverLetters, and tailoredResumes use
take(limit * OVER_FETCH) before applying the cursor filter in memory. Update the
searchHistory flow to use a real query-time cursor, or separate per-table
cursors with a stable tie-breaker, so pagination does not stop once a table
exceeds the fixed over-fetch window. Make sure the cursor logic in searchHistory
handles _creationTime ties deterministically and is applied at query time rather
than after fetching.
In `@src/app/api/analyze/route.ts`:
- Around line 87-91: The cover-letter option validators are using property
membership checks that can accept inherited keys, so invalid values may pass the
400 guard and later break access in src/lib/gemini.ts. Update the validation
helpers used by analyze/route.ts, specifically isValidCoverLetterTone and
isValidCoverLetterLength, to use own-property checks such as Object.hasOwn on
TONE_OPTIONS and LENGTH_OPTIONS or switch to a Set-based lookup so only real
option keys are accepted.
In `@src/app/api/extract-skills/route.ts`:
- Around line 20-37: The skills cache key is still shared across users, which
can cause cross-user cache collisions in the extract-skills route. Update the
cache key construction in the route handler to include the authenticated userId
along with the resume and job description hashes, so each user gets an isolated
cache entry; use the existing request flow around checkRateLimit and createHash
to locate the cacheKey logic.
In `@src/app/api/history/`[id]/route.ts:
- Around line 36-41: The user-scoped lookup flow in history/[id]/route.ts
changed the unauthorized response from 403 to 404 because getAnalysisById and
getCoverLetterById now return null on ownership mismatch, so the later
item.userId !== userId check is unreachable. Update the route handlers that use
getAnalysisById/getCoverLetterById to either treat missing ownership as
NOT_FOUND and remove the dead FORBIDDEN branch, or change the Convex lookup
helpers if FORBIDDEN is the intended contract, and make the tests match the
chosen behavior.
In `@src/app/api/match-score/route.ts`:
- Around line 20-37: The match-score cache key is still shared across users even
though the route already has userId, which can cause cross-user cache
collisions. Update the cache key construction in the match-score route to
include userId alongside the resumeText and jobDescription hashes, and make sure
the same user-scoped key is used anywhere this cached result is read or written.
- Around line 39-47: The cached response path in match-score route currently
returns only result and cached, while the fresh path also derives and returns
score, so make the cache-hit branch in the route handler mirror the same
response shape. Update the logic around matchCache.get(cacheKey) and
NextResponse.json so cached responses include score as well, using the same
score extraction/serialization used after analyzeResume.
In `@src/components/dashboard/cover-letter/CoverLetterPreferencesPanel.tsx`:
- Around line 34-55: The tone/length option buttons in
CoverLetterPreferencesPanel are only indicating selection visually; update the
button groups in the tone and length selectors to expose their active state to
assistive tech by adding aria-pressed based on the current selection. Use the
existing button rendering in CoverLetterPreferencesPanel and the selected state
variables (tone and the corresponding length state) so screen readers can
announce which option is currently active.
In `@src/lib/cover-letter-options.ts`:
- Around line 12-15: The length labels in LENGTH_DESCRIPTIONS are out of sync
with the generator settings used by GeminiConfig in gemini.ts. Update the
CoverLetterLength copy for standard and detailed so it matches the actual
configured word counts exposed by the generator, keeping the UI options
consistent with the backend output. Locate the mismatch in
cover-letter-options.ts and align those descriptions with the values defined for
the same length keys in gemini.ts.
- Around line 34-39: The runtime guards in isValidCoverLetterTone and
isValidCoverLetterLength are too permissive because using the in operator
accepts inherited properties like toString and __proto__. Update these
validators to check only own keys of TONE_OPTIONS and LENGTH_OPTIONS so
/api/analyze cannot narrow invalid request values to CoverLetterTone or
CoverLetterLength.
---
Outside diff comments:
In `@src/app/dashboard/cover-letter/page.tsx`:
- Around line 49-56: The persisted draft handling in restoreDraft() is only
using TypeScript casts, so invalid tone/length values can still slip into state
from stale or tampered storage. Re-validate the parsed draft fields before
calling setTone and setLength, using the existing runtime guards from
src/lib/cover-letter-options.ts to normalize or ignore bad values. Apply the
same validation wherever the draft is read back for hydration so the CoverLetter
state only receives supported CoverLetterTone and CoverLetterLength values.
---
Nitpick comments:
In `@convex/tailoredResumes.ts`:
- Around line 100-103: The lookup in tailoredResumes is only using the
by_userId_builderSlug index for userId and then filtering builderSlug afterward,
which makes the query broader than intended. Update the query in the docs
retrieval path to bind both userId and builderSlug directly in withIndex(...) on
by_userId_builderSlug, using the existing tailoredResumes query chain, so the
index fully narrows the result set.
In `@test/app/api/export-cover-letter-docx/route.test.ts`:
- Around line 12-36: The POST route tests for export-cover-letter-docx are
missing coverage for the rate-limit failure path. Add a test in the existing
describe('/api/export-cover-letter-docx') suite that mocks checkRateLimit to
return allowed: false and asserts POST returns 429 with the RATE_LIMITED
response, alongside the existing getAuthenticatedUser and checkRateLimit mocks.
In `@test/app/api/extract-skills/route.test.ts`:
- Around line 16-60: Add a test for the new 429 rate-limit path in the
/api/extract-skills route. Extend the existing POST test coverage in
route.test.ts by mocking checkRateLimit to return allowed: false and asserting
that POST responds with 429 and the expected rate-limit message. Use the same
setup patterns already used in the describe('/api/extract-skills') block and
reference POST, checkRateLimit, and getAuthenticatedUser so the new test fits
the current mocking style.
In `@test/app/api/match-score/route.test.ts`:
- Around line 17-20: The match-score route tests always mock checkRateLimit as
allowed, so the new rate-limit short-circuit in the route handler is never
exercised. Add a dedicated 429 regression test in route.test.ts for the
match-score handler that overrides the default checkRateLimit mock to return
denied, then assert the handler responds with 429 and does not continue into the
normal success path. Use the existing getAuthenticatedUser, checkRateLimit, and
the route handler invocation in this suite to locate the setup and keep the new
case isolated from the default beforeEach behavior.
In `@test/app/api/resumes/route.test.ts`:
- Line 61: The test for the resume route currently only verifies the scoped
lookup via getResumeById, so it can miss a regression in the delete path. Update
the route test to also assert that deleteResume is called with both the resumeId
and the authenticated userId, using the existing resume route handler and the
deleteResume mock so the delete action is covered explicitly.
- Around line 64-75: Update the DELETE resume test to match the scoped lookup
contract used by getResumeById(resumeId, userId): when a resume belongs to
another user, the mocked lookup should return null rather than a mismatched
record. Adjust the cross-user case in route.test.ts so it verifies the
forbidden/not-found behavior through the scoped access path in DELETE without
relying on an owner-mismatch object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9212923-5b82-40ed-9fa1-8c8a11637c2b
📒 Files selected for processing (33)
convex/analyses.tsconvex/coverLetters.tsconvex/functions.tsconvex/lib/parseMatchScore.tsconvex/resumes.tsconvex/searchHistory.tsconvex/tailoredResumes.tsconvex/userStats.tslint_results.txtsrc/app/api/analyze/route.tssrc/app/api/export-cover-letter-docx/route.tssrc/app/api/extract-skills/route.tssrc/app/api/history/[id]/route.tssrc/app/api/match-score/route.tssrc/app/api/resumes/route.tssrc/app/api/user-data/route.tssrc/app/dashboard/cover-letter/page.tsxsrc/components/dashboard/cover-letter/CoverLetterPreferencesPanel.tsxsrc/components/dashboard/cover-letter/CoverLetterRecentHistory.tsxsrc/components/dashboard/cover-letter/CoverLetterResultPanel.tsxsrc/components/dashboard/cover-letter/types.tssrc/lib/contracts/api.tssrc/lib/convex-server.tssrc/lib/cover-letter-options.tstest/app/api/contracts.test.tstest/app/api/export-cover-letter-docx/route.test.tstest/app/api/extract-skills/route.test.tstest/app/api/history/[id]/route.test.tstest/app/api/match-score/route.test.tstest/app/api/resumes/route.test.tstest/app/api/user-data/route.test.tstest/lib/analysis-normalizer.test.tstest/lib/convex-server.test.ts
💤 Files with no reviewable changes (3)
- src/lib/contracts/api.ts
- lint_results.txt
- test/app/api/contracts.test.ts
- cover-letter-options: use Object.hasOwn() instead of in for validation - cover-letter-options: align LENGTH_DESCRIPTIONS with actual word counts - page.tsx: validate tone/length from localStorage/server drafts before setting state - parseMatchScore: strip markdown fences before JSON.parse - match-score/route: add userId to cache key, include score in cached response - extract-skills/route: add userId to cache key - history/[id]/route: remove dead 403 branches (scoped lookups enforce ownership) - resumes/route: remove dead 403 branch (getResumeById enforces ownership) - CoverLetterPreferencesPanel: add aria-pressed for accessibility - tailoredResumes: bind userId in withIndex, keep builderSlug filter (queryGeneric types limit chaining) - Add 429 rate-limit tests for match-score, extract-skills, export-cover-letter-docx - Fix resume/history route tests for scoped lookup behavior
- Make all functions:* handlers internal-only; server client uses CONVEX_DEPLOY_KEY - Scope /api/analyze in-memory cache by userId - searchHistory: apply cursor filter at query time per table - tailoredResumes: bind userId+builderSlug on composite index (runtime chain) - Add parseMatchScore unit tests; document CONVEX_DEPLOY_KEY in .env.example
Summary
Implements the prod-code review follow-ups: secure AI/export routes, split Convex and cover-letter UI, shared options, and tests.
Changes
Security (blockers)
/api/match-scoreand/api/extract-skills:getAuthenticatedUser+checkRateLimit/api/export-cover-letter-docx: auth + rate limitStructure
resumes,analyses,coverLetters,tailoredResumes,userStats,searchHistory);functions.tsre-exports for stablefunctions:*pathsCoverLetterRecentHistory,CoverLetterPreferencesPanel,CoverLetterResultPanel(~700 → ~500 LOC)src/lib/cover-letter-options.ts: single source for tone/length UI + API validationTests & cleanup
analysis-normalizerunit testsconvex-servertests (getUserStats,getAnalysis)userIdtrust note onconvex-serverlint_results.txtVerification (mirrors CI
quality)bun run lint✓bun run typecheck✓bun run test:coverage✓ (134 tests, coverage thresholds met)Summary by CodeRabbit