fix: quick wins — monthly recurrence, blur, SSR fix

[vayungodara]

Summary

• handleMiss double-submit guard in PactCard (prevents rapid-click duplicate DB writes)
• Monthly recurrence option added to pact creation (UI button + DB constraint updated)
• Overlay blur — dashboard blurs behind modal when opened
• SSR fix — guard createPortal with typeof document check
• useStreakFreeze export alias for DailySummaryCard compatibility

DB changes applied

• pacts CHECK constraint updated to allow 'monthly' recurrence
• 15 abandoned focus sessions cleaned up
• Stale streak reset for user 419f05e6

Test plan

• Build passes
• 104/104 unit tests pass
• Open New Pact modal — overlay blurs dashboard
• Toggle recurring → Monthly button appears
• Rapid-click "I missed it" on a pact — only one update fires
• No SSR errors on page load

🤖 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 22, 2026 11:03am

[vayungodara]

@claude review

[claude]

Claude finished @vayungodara's task in 3m 0s —— View job

---

PR #27 Code Review

• Gather context
• Read changed files
• Review: PactCard.js (handleMiss guard)
• Review: CreatePactModal.js (monthly recurrence)
• Review: Modal components (overlay blur, SSR fix)
• Review: lib/streaks-advanced.js
• Review: lib/pactTemplates.js + tests
• Review: useModalScrollLock.js
• Post final review

---

Overall this is a solid PR with good attention to UX polish and defensive coding. A few real bugs need fixing before merge.

Bugs

🔴 Monthly recurrence causes wrong toast text (PactCard.js:124, PactCard.js:163)

Both handleComplete and handleMiss use a ternary that doesn't account for 'monthly':

// Line 124 (handleComplete) and 163 (handleMiss)
pact.recurrence_type === 'daily' ? 'day' : pact.recurrence_type === 'weekly' ? 'week' : 'weekday'

When recurrence_type === 'monthly', this falls through to 'weekday', so the user sees "It'll reset for the next weekday" instead of "month". Fix this →

---

🔴 No SQL migration file for monthly constraint (mentioned in PR description only)

The DB constraint in supabase/checkpoint8_complete.sql:23 and supabase/new_features_schema.sql:9 only allows ('daily', 'weekly', 'weekdays'):

CHECK (recurrence_type IS NULL OR recurrence_type IN ('daily', 'weekly', 'weekdays'))

The PR says "DB changes applied" but there's no migration file. A fresh Supabase setup or a new dev environment would reject 'monthly' at the DB level, silently failing with a constraint violation. Add a migration file (e.g., supabase/add_monthly_recurrence.sql) that drops and recreates the constraint. Fix this →

---

🔴 mark_overdue_pacts.sql doesn't handle monthly recurrence (supabase/mark_overdue_pacts.sql:34-46, 70-81)

The stored procedure's CASE recurrence_type block falls through to else for monthly pacts — which adds 1 day. Monthly pacts would cycle daily instead of monthly. The else branch needs a when 'monthly' case:

when 'monthly' then
  next_dl := next_dl + interval '1 month';

This affects both the completed/missed block and the active-overdue block. Fix this →

---

Minor Issues

🟡 useStreakFreeze naming (lib/streaks-advanced.js:75)

export const useStreakFreeze = applyStreakFreeze;

The use prefix conventionally signals a React hook. applyStreakFreeze contains no React hooks, so ESLint's react-hooks/rules-of-hooks may warn about calling it conditionally or outside a component. The alias works today but is risky if the rules-of-hooks lint rule is ever enabled. Consider renaming the export to applyStreakFreeze and updating the one call site in DailySummaryCard.js.

🟡 SSR fix applied inconsistently across modals

CreatePactModal.js:183 has the typeof document !== 'undefined' guard, but CreateGroupModal.js:101, CreateTaskModal.js:96, and JoinGroupModal.js:103 use {isOpen && createPortal(...)}. These are safe today because isOpen starts as false and is set only in effects (which don't run during SSR). But the inconsistency could confuse future contributors. One pattern or the other — pick one.

🟡 useModalScrollLock cleanup is unconditional (lib/useModalScrollLock.js:9-12)

return () => {
  document.body.style.overflow = '';
  document.documentElement.removeAttribute('data-modal-open');
};

The cleanup always removes data-modal-open and overflow: hidden, even when the effect fired with isOpen=false (meaning it set nothing). If two modals are ever open at the same time, closing one would re-enable body scroll while the other stays open. Not a bug with the current UI, but fragile. A common fix is a ref-count or moving the cleanup inside the if (isOpen) branch.

---

Positives

• handleMiss guard — correct use of isCompletingRef shared across both handleComplete and handleMiss to prevent concurrent double-submits. Clean approach.
• Accessibility — role="dialog", aria-modal, aria-labelledby, aria-pressed on recurrence buttons, Escape key, and focus management are all implemented correctly in CreatePactModal.
• resetFormState extraction — eliminates the 3× duplication noted in the old code, and useCallback prevents unnecessary re-renders.
• Test coverage — 238-line test file for streaks-advanced, proper Supabase mock setup, realistic edge cases (cooldown, pool cap, milestone boundary). Solid foundation.
• useModalScrollLock hook — clean extraction, shared correctly across all modal components.

---

Summary: 3 bugs to fix before merge (monthly toast text, missing migration file, missing monthly case in stored procedure). The rest are minor consistency/style issues that can be addressed in follow-up.

[vayungodara]

@claude review

[claude]

Claude finished @vayungodara's task in 1m 42s —— View job

---

PR #27 Code Review

• Read changed files
• Check if previous bugs were fixed
• Review new components (CommandPalette, Skeleton, sounds.js)
• Review SQL/DB changes
• Review test coverage
• Post final review

---

Status: 3 bugs from previous review remain open

The previous Claude review flagged 3 red bugs. None of them have been fixed in the current branch.

---

Bugs (still open)

🔴 Monthly recurrence toast shows wrong period (components/PactCard.js:159, 213)

Both handleComplete and handleMiss still use the same two-arm ternary:

pact.recurrence_type === 'daily' ? 'day' : pact.recurrence_type === 'weekly' ? 'week' : 'weekday'

monthly falls through to 'weekday' → user sees "It'll reset for the next weekday". Needs a third arm:

pact.recurrence_type === 'daily' ? 'day'
  : pact.recurrence_type === 'weekly' ? 'week'
  : pact.recurrence_type === 'monthly' ? 'month'
  : 'weekday'

Fix this →

---

🔴 No SQL migration for 'monthly' constraint (supabase/checkpoint8_complete.sql:23, supabase/new_features_schema.sql:9)

Both files still only allow ('daily', 'weekly', 'weekdays'):

CHECK (recurrence_type IS NULL OR recurrence_type IN ('daily', 'weekly', 'weekdays'))

streak_freeze_redesign.sql was added as a migration pattern — same pattern needed for monthly. A fresh Supabase setup will reject monthly pacts at the DB level, silently returning a constraint violation error. Fix this →

---

🔴 mark_overdue_pacts.sql silently cycles monthly pacts daily (supabase/mark_overdue_pacts.sql:34-46, 70-81)

Both CASE blocks still have no when 'monthly' arm. Monthly pacts fall through to else → next_dl + interval '1 day', so an overdue monthly pact gets reset every day instead of every month. Fix this →

---

New code review

CommandPalette.js — good overall. Two small notes:

• ARIA wiring is incomplete: the <input> has no aria-controls pointing to the <ul>, and the <ul role="listbox"> is disconnected from the input. Screen readers won't associate the input with the results. Add aria-controls="cmd-palette-list" on the input and id="cmd-palette-list" on the <ul>.
• Silent failure when onCreatePact not passed (line 89): ⌘N is listed as a shortcut in the command list, but if onCreatePact is undefined the command silently does nothing. Either omit that command when the prop is absent, or ensure every CommandPalette usage site passes it.

sounds.js — solid. One edge case: browsers sometimes auto-suspend AudioContext after inactivity. Calling osc.start() on a suspended context throws. A ctx.state === 'suspended' && ctx.resume() guard before scheduling nodes would prevent the silent failure. Not blocking.

useStreakFreeze naming (lib/streaks-advanced.js:83): still present. DailySummaryCard.js:170 calls it inside an async handler — fine today, but the use prefix will cause ESLint react-hooks/rules-of-hooks to warn if the rule is ever enabled. Consider renaming to applyStreakFreeze at the call site too.

useModalScrollLock cleanup (lib/useModalScrollLock.js:9-12): still unconditional — closing one modal clears data-modal-open even if another is open. Low risk given current UI (modals don't stack), but fragile.

---

Positives

• streak_freeze_redesign.sql is a clean migration pattern — same pattern just needs applying to the monthly constraint.
• CommandPalette keyboard navigation (arrow keys, Enter, Escape) and scroll-into-view are implemented correctly.
• sounds.js is SSR-safe and uses no external files — good dependency hygiene.
• isCompletingRef guard across both handleComplete and handleMiss is correct and blocks concurrent calls regardless of which button is clicked.

---

Summary: 3 pre-existing bugs need fixes before merge (monthly toast, DB constraint migration, stored procedure case). The new CommandPalette ARIA wiring is a minor gap worth addressing.

[vayungodara]

@claude review

[claude]

Claude finished @vayungodara's task in 1m 25s —— View job

---

PR #27 Code Review

• Gather context
• Check if previous bugs were fixed (monthly toast, DB migration, stored proc)
• Review open minor issues from last review
• Post final review

---

All 3 red bugs from previous reviews are fixed ✅

Bug	Status
Monthly toast shows "weekday" fallback (PactCard.js)	✅ Fixed — monthly arm added at lines 159 & 213
No SQL migration for 'monthly' constraint	✅ Fixed — supabase/add_monthly_recurrence.sql added
mark_overdue_pacts.sql cycles monthly pacts daily	✅ Fixed — when 'monthly' added to both CASE blocks (lines 44, 82)

---

Remaining minor issues (carried from previous reviews)

🟡 CommandPalette ARIA wiring incomplete (components/CommandPalette.js:154, 169)

The input has no aria-controls pointing to the list, and the <ul role="listbox"> has no matching id. Screen readers can't associate them:

// input needs:
aria-controls="cmd-palette-list"

// ul needs:
id="cmd-palette-list"

The "Create New Pact" command (line 89) also silently does nothing when onCreatePact is undefined — consider either filtering it out when the prop is absent, or always passing it at every usage site. Fix this →

🟡 useStreakFreeze naming (lib/streaks-advanced.js:83)

The alias export const useStreakFreeze = applyStreakFreeze still exists. The use prefix flags it as a hook under react-hooks/rules-of-hooks. Low risk now, but renaming the export and the call in DailySummaryCard.js:170 to applyStreakFreeze removes the ambiguity entirely.

🟡 useModalScrollLock cleanup is unconditional (lib/useModalScrollLock.js:9-12)

Cleanup always removes data-modal-open and overflow: hidden, even when isOpen was false when the effect ran. Harmless with the current UI (no stacked modals), but worth fixing if that ever changes.

🟡 sounds.js — no AudioContext.resume() guard

Browsers suspend AudioContext after inactivity. Calling osc.start() on a suspended context throws a silent error. Adding if (ctx.state === 'suspended') await ctx.resume() before scheduling nodes in each function would prevent this edge case.

---

Positives

• Clean migration pattern — add_monthly_recurrence.sql follows the same structure as streak_freeze_redesign.sql. Consistent.
• mark_overdue_pacts.sql — inner loop correctly advances by full months until the deadline is in the future, same as other recurrence types.
• CommandPalette keyboard nav — arrow keys, Enter, Escape, and scrollIntoView are all correct.
• sounds.js SSR safety — typeof window !== 'undefined' guard in getAudioContext is correct.

---

Ready to merge. The 3 blocking bugs are all resolved. The remaining items are minor polish that can be addressed in a follow-up.