fix: resolve 13 findings from March 23 triage

[vayungodara]

Summary

• Security: Open redirect removed (auth callback), timing-safe cron secret comparison (all 4 routes)
• Performance: FocusContext value memoized, N+1 notification inserts batched, orphaned session duration capped
• Design: 8 CSS quick wins — noise texture removed, gradients desaturated/varied, radii tightened, accent desaturated, backgrounds neutralized

Cross-references findings from 3 email sources:

• Morning Report A (vgodara@britams.nl)
• Morning Report B (vayun@zo.computer)
• Frontend Audit (vgodara@britams.nl)

Resolves 13 of 15 open findings from PR #28 triage. Remaining: DST streak edge case (needs integration tests), Kanban drag-and-drop (feature addition).

Changes

Category	Files	What
Security	auth/callback/route.js	Remove x-forwarded-host redirect, add // guard
Security	lib/cronAuth.js + 4 cron routes	crypto.timingSafeEqual via shared utility
Performance	lib/FocusContext.js	useMemo on context value, useCallback on formatTime
Performance	cron/check-streak-breaks/route.js	Single batch .insert() instead of N calls
Data integrity	cron/cleanup/route.js	Math.min(now, expectedEnd) for orphaned sessions
Design	globals.css	8 audit quick wins (noise, gradients, radii, accent, borders, glow, backgrounds)

Test plan

• Verify auth callback redirects to dashboard after Google OAuth
• Verify cron routes reject invalid secrets (401)
• Verify focus timer doesn't cause excessive re-renders (React DevTools Profiler)
• Visual check: landing page, dashboard, pacts — calmer gradients, no noise texture
• Dark mode: borders softer, backgrounds neutral
• npm run build passes clean

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
lockin	Ready	Preview, Comment	Mar 23, 2026 4:47pm

[vayungodara]

@claude review

[claude]

Claude finished @vayungodara's task in 2m 16s —— View job

---

PR Review — fix: resolve 13 findings from March 23 triage

• Gather context and understand request
• Read key changed files (security, performance, design)
• Review auth callback changes
• Review cron auth utility + routes
• Review FocusContext performance changes
• Review CSS/design changes in globals.css
• Review new components (CommandPalette, Skeleton)
• Review iOS app additions
• Post final review

---

Overall

Solid batch of targeted fixes. The security work is particularly clean. A few issues are worth addressing before merge — one functional bug, one undisclosed scope expansion, and some minor items.

---

Security ✅

lib/cronAuth.js — Timing-safe secret comparison is correctly implemented. The explicit length check before timingSafeEqual is both necessary (the Node API requires equal-length buffers) and documented clearly. Shared utility avoids 4 separate implementations drifting apart. No issues.

app/auth/callback/route.js — Double-layer redirect validation is thorough: the startsWith('/') + !startsWith('//') check blocks protocol-relative URLs, then the new URL(next, origin).origin !== origin cross-origin check catches edge cases. One minor note: next is validated against origin (from the request URL) but the final redirect is built with appOrigin (NEXT_PUBLIC_APP_URL). This isn't a vulnerability since next is always a relative path by the time it's used, but if these two origins differ (e.g. staging env) the redirect will hit appOrigin${next} as intended. Fine.

---

Performance ✅

lib/FocusContext.js — useMemo on the context value is the right fix. One observation: currentWorkDuration and currentBreakDuration are computed from refs (workDurationRef.current) each render, then listed as useMemo deps. Since the storage listener calls setTimeLeft(...) on settings change, the memoized value will get a fresh re-evaluation on the next state change. Acceptable — no stale WORK_DURATION exposed to consumers in practice.

check-streak-breaks/route.js — Single batch .insert() for notifications is the right fix. Building activity entries per group membership in a loop then inserting once is correct. Promise.all([notificationPromise, activityPromise]) runs them concurrently — good.

cleanup/route.js — Math.min(now.getTime(), expectedEnd.getTime()) cap is exactly right. Upsert pattern is clean.

---

Bug: Missing @keyframes shimmer in Skeleton

components/Skeleton.module.css:4,27 — Both .skeleton and .skeletonText reference animation: shimmer 1.5s ... but only @keyframes shimmerSlide is defined. The shimmer keyframe does not exist. The shimmer slide animation on the ::after pseudo-element will work, but the base element animation silently does nothing (no pulse/fade on the container). The skeletons will appear as static blocks, not the intended shimmering loaders.

Fix this →

---

New component: CommandPalette

Not mentioned in the PR description but this is a substantial new feature. The implementation is clean:

• Correct SSR guard (typeof document === 'undefined') per CLAUDE.md gotcha Add unit and integration test coverage #10.
• Proper useModalScrollLock integration.
• useMemo on filtered commands list.
• Accessible: role="listbox", aria-selected, aria-label on input.

One note: the ⌘N shortcut label on the "Create New Pact" command (CommandPalette.js:13) is display-only — no global keydown listener registers Cmd+N inside the palette to trigger the onCreatePact callback. It just looks like a shortcut hint. If Cmd+N is already wired elsewhere in KeyboardShortcutsContext this is intentional; if not, the badge is misleading.

---

send-reminders/route.js — N+1 not fully addressed

The reminder_logs inserts at lines 55-58 still happen one-at-a-time inside a for loop. Since email sending is inherently sequential (one per pact) this is probably fine in practice, but if you want consistency with the batch approach applied to the other routes, the successful reminder log rows could be collected and inserted in one batch after the loop.

---

lib/sounds.js — New file, good implementation

Web Audio API programmatic sound generation is a clean approach — no external files, no hosting concerns. isSoundEnabled() correctly falls back to true on localStorage errors. The webkitAudioContext fallback covers older Safari. No issues.

---

iOS App scaffolding

Clean MVVM structure. Config.swift using the Supabase anon key in source is correct (anon key is designed to be public). The lockin-ios/.claude/settings.json with its own tool allowlist is a thoughtful addition for iOS-context Claude runs. Design doc additions are welcome.

---

Summary

Area	Status	Notes
Security (open redirect + timing-safe cron)	✅ Clean	Well-implemented
Performance (FocusContext memoization)	✅ Clean	Minor ref-deps note, benign
Performance (batch notifications)	✅ Clean
Data integrity (orphaned session cap)	✅ Clean
Skeleton animation	⚠️ Bug	Missing @keyframes shimmer
CommandPalette	✅ Clean	Undisclosed in PR description; ⌘N badge may be misleading
Design tokens (globals.css)	✅ Clean
iOS scaffolding	✅ Clean

One functional fix needed: Add @keyframes shimmer to Skeleton.module.css. Everything else is green or advisory.

[vayungodara]

@claude review

[claude]

Claude finished @vayungodara's task in 2m 8s —— View job

---

PR Review — fix: resolve 13 findings from March 23 triage

• Gather context
• Read key changed files
• Review security changes
• Review performance changes
• Review new components (CommandPalette, Skeleton)
• Review design/CSS changes
• Post final review

---

Overall

Clean batch of targeted fixes. The previously identified @keyframes shimmer bug has been resolved (commit 01e2d94). No blocking issues remain.

---

Security ✅

lib/cronAuth.js — Timing-safe comparison is correctly implemented. The explicit length check before timingSafeEqual is necessary (Node requires equal-length buffers) and clearly documented. Single shared utility eliminates drift across 4 cron routes. Clean.

app/auth/callback/route.js — Double-layer redirect validation is thorough:

• startsWith('/') + !startsWith('//') blocks protocol-relative URLs
• new URL(next, origin).origin !== origin catches edge cases like URL-encoded bypasses

One observation: the redirect uses appOrigin (NEXT_PUBLIC_APP_URL || origin) while validation is against origin. Not a vulnerability since next is always a relative path by this point, but worth knowing if staging/prod origins differ — the redirect will correctly hit appOrigin${next}.

---

Bug Fixed ✅

components/Skeleton.module.css — The missing @keyframes shimmer identified in the previous review has been added (commit 01e2d94, lines 20–24). Both .skeleton and .skeletonText now have a working opacity pulse, and the ::after shimmerSlide sweep also works. The prefers-reduced-motion media query correctly disables both. ✅

---

Performance ✅

lib/FocusContext.js — useMemo on the context value is the right fix. currentWorkDuration and currentBreakDuration are read from refs (lines 275–276) and listed as useMemo deps. Since refs don't trigger re-renders, these deps only update when another state change (like setTimeLeft() in the storage listener) causes a re-render — at which point the ref values are re-read. In practice this means WORK_DURATION/BREAK_DURATION in context are always current after a settings-change re-render. No stale values exposed to consumers.

check-streak-breaks/route.js — Single batch .insert() for notifications is clean. Building activity entries in a loop then inserting once is correct. Promise.all([notificationPromise, activityPromise]) runs both concurrently. ✅

cleanup/route.js — Math.min(now.getTime(), expectedEnd.getTime()) cap is exactly right. Upsert pattern is clean. ✅

---

send-reminders/route.js — In-loop reminder_logs inserts (advisory)

The reminder_logs inserts at lines 55–62 still happen one-at-a-time inside the for loop. This is actually correct here — each log entry is gated on the individual email API response (emailResult.success), so you can't batch them upfront. Email sending is inherently sequential. No change needed.

---

CommandPalette ✅

Well-implemented new component:

• SSR guard (typeof document === 'undefined') at line 124 per CLAUDE.md gotcha Add unit and integration test coverage #10 ✅
• useModalScrollLock(open) integrated at line 25 ✅
• useMemo on filtered commands list ✅
• Accessible: role="listbox", aria-selected, aria-label on input ✅
• Arrow-key navigation wraps correctly; empty state guarded safely at line 116 ✅

One note: the ⌘N shortcut badge on "Create New Pact" (line 13) is display-only within the palette — no keydown listener fires onCreatePact from inside the palette. If ⌘N is already wired in KeyboardShortcutsContext, this badge is a correct hint pointing at the external shortcut. If not, the badge is misleading. Worth confirming.

---

lib/sounds.js ✅

Clean approach — Web Audio API with no external file dependencies. webkitAudioContext fallback covers older Safari. isSoundEnabled() falls back to true on localStorage errors. SSR-safe (typeof window !== 'undefined' guard in getAudioContext). No issues.

---

Skeleton component ✅

After the shimmer keyframes fix: .skeleton, .skeletonText, .skeletonCircle, .skeletonRow, .skeletonRowContent all look correct. Reduced-motion media query disables both animations. Clean.

---

Summary

Area	Status	Notes
Security (open redirect + timing-safe cron)	✅ Clean
Performance (FocusContext memoization)	✅ Clean	Ref-deps update pattern is fine
Performance (batch notifications)	✅ Clean
Data integrity (orphaned session cap)	✅ Clean
Skeleton @keyframes shimmer	✅ Fixed	Was bug in previous review
CommandPalette	✅ Clean	Confirm ⌘N global shortcut exists
send-reminders reminder_logs	✅ OK	In-loop is correct here (email-gated)
lib/sounds.js	✅ Clean
iOS scaffolding	✅ Clean

No blocking issues. PR is ready to merge once you've verified the auth callback flow and visual checks pass.

[vayungodara]

Replacing with a clean branch from main (no inherited commits). See new PR.