feat: bonfire animation, guide modal, navbar polish, responsive fixes#28
Merged
feat: bonfire animation, guide modal, navbar polish, responsive fixes#28
Conversation
- Three.js teardrop flame system (5 flames, progressive unlock via intensity thresholds) - Break-aware bonfire: short break dims to embers (0.08), long break near-dormant (0.04), camera pulls back smoothly - Slow float/pulse during breaks (5.5s/8s cycles vs 3.5s focus) - COOLING label for long break, RESTING for short break - GuideModal: right sidebar (desktop) + bottom sheet (mobile) with 6 accordion sections - Navbar: z-index click fix, Guide ? button, theme toggle moved into SettingsPanel - WCAG AA contrast fix for --text-muted in dark mode (#7E7E72) - Favicon shows minutes only (no seconds) - All emojis removed from UI, document titles, activity feed - Room name input shows placeholder instead of pre-filled generated name - Start Room button: pure-JS ripple + hover lift + press scale - Presence leave debounce 15s -> 300s + re-track on tab focus (no false "left" messages on tab switch) - Responsive: bonfire height clamp(140px, 26vh, 260px), tighter header/main padding - Dead files removed: KeyboardShortcutsModal, PreferencesMenu Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughRebrand from "PomodoroJam" to "Bonfire" and related URL/defaults; add a client Login page (OAuth/OTP); introduce a Three.js bonfire visualization and supporting hook; refactor SessionProvider to use BonfireScene and GuideModal; add HomeClient; multiple UI, styling, and utility updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as HomeClient
participant API as /api/session
participant Supabase
participant Session as /session/[id]
User->>Client: Click "Start Room"
Client->>Client: Validate inputs & build payload
Client->>API: POST /api/session { title, display_name?, isPublic }
API->>Supabase: Insert session record
Supabase-->>API: session { id, host_name }
API-->>Client: { sessionId }
Client->>Client: Store metadata in localStorage
Client->>Session: navigate /session/{id}
Session->>Supabase: Subscribe to realtime presence/channel
Session->>Session: Mount BonfireScene with useBonfireState props
Session-->>User: Render bonfire visualization + timer UI
sequenceDiagram
participant Timer as Timer Loop
participant useBonfireState
participant Visibility as Document Visibility
participant BonfireScene
Timer->>useBonfireState: send status, mode, focusCount, participantCount
useBonfireState->>useBonfireState: compute baseIntensity & bonuses
Note over useBonfireState: focusCount increments
useBonfireState->>useBonfireState: trigger 2s isSurging + intensityBoost
Note over useBonfireState: participant join/leave
useBonfireState->>useBonfireState: apply transient intensity adjustments
Visibility->>useBonfireState: visibilitychange (hidden/visible)
useBonfireState->>useBonfireState: accumulate tabHiddenMs, apply accountabilityDecay
Visibility->>useBonfireState: visible -> trigger return surge
useBonfireState->>BonfireScene: provide targetIntensity, isSurging, flameLabel
BonfireScene->>BonfireScene: animate flames, embers, glow per intensity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
app/explore/page.tsx (1)
155-155:⚠️ Potential issue | 🟡 MinorStale tomato emoji in empty state
PR summary states emojis were removed from UI, but the Explore empty state still shows
🍅(PomodoroJam-era). Swap it for a Lucide icon (e.g.,Flame) to match the Bonfire rebrand:🔧 Proposed fix
- <div className="text-5xl mb-4">🍅</div> + <Flame className="w-12 h-12 mb-4" style={{ color: 'var(--accent)' }} />As per coding guidelines: "Use Lucide React for icon components throughout the application".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/explore/page.tsx` at line 155, Replace the stale tomato emoji in the Explore empty state with the Lucide Flame icon: remove the literal "🍅" in the div inside app/explore/page.tsx and import Flame from 'lucide-react', then render <Flame /> with the same styling classes (e.g., text-5xl mb-4 or equivalent) so the visual size and spacing remain consistent; ensure you add the Flame import at the top of the file and remove the emoji literal.package.json (1)
15-30:⚠️ Potential issue | 🟡 MinorUpgrade
@react-three/fiberto 9.6.0 for React 19 compatibility.The
three(0.184.0) and@react-three/drei(10.7.7) versions are current and correct for the BonfireScene implementation. However,@react-three/fiberis outdated: the codebase specifies 8.18.0, but version 9.6.0 is the latest and includes explicit React 19 support. While React 18 (your current target) works with 8.x, upgrade to 9.6.0 now to ensure forward compatibility and avoid friction later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 15 - 30, Update the `@react-three/fiber` dependency in package.json from "8.18.0" to "9.6.0" to ensure React 19 compatibility; modify the version string for the "@react-three/fiber" entry, run your package manager (npm/yarn/pnpm) to install and then run the test/build to verify no breaking changes affect BonfireScene or imports that reference `@react-three/fiber`.hooks/useSession.ts (2)
57-84:⚠️ Potential issue | 🟡 MinorPresence re-track effect ignores
avatarUrlchanges.The existing effect at lines 57-65 re-tracks on
[isHost, username]but notavatarUrl. If the user updates their avatar mid-session (profile edit, new upload), the presence payload broadcast to peers keeps the staleavatar_urluntil the next host/username transition — which for most users is "never". The new visibility-change effect usesavatarUrlRef.currentso it would push a fresh avatar on tab focus, but that shouldn't be the only path.♻️ Suggested fix
- useEffect(() => { - if (!channelRef.current) return - channelRef.current.track({ - username: usernameRef.current ?? null, - avatar_url: avatarUrlRef.current ?? null, - is_host: isHost, - joined_at: joinedAtRef.current, - }) - }, [isHost, username]) + useEffect(() => { + if (!channelRef.current) return + channelRef.current.track({ + username: usernameRef.current ?? null, + avatar_url: avatarUrlRef.current ?? null, + is_host: isHost, + joined_at: joinedAtRef.current, + }) + }, [isHost, username, avatarUrl])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useSession.ts` around lines 57 - 84, The presence re-track effect uses avatarUrlRef.current but its dependency array omits avatarUrl, so avatar changes don’t trigger re-track; update the effect that calls channelRef.current.track(...) (the first useEffect referencing channelRef, usernameRef, avatarUrlRef, isHost, joinedAtRef) to include avatarUrl in its dependency array (e.g. [isHost, username, avatarUrl]) so a new avatar triggers re-tracking and broadcasts the updated avatar_url to peers.
180-203:⚠️ Potential issue | 🟠 Major5-minute leave grace creates ghost participants on real disconnects.
Jumping from 15s → 300s fixes the "false-leave on tab switch" symptom, but it also means every actual disconnect (user closes tab, network drop, browser crash, laptop sleep > heartbeat) leaves the participant rendered in the UI for up to 5 minutes to everyone else, including being counted toward "who's focusing" indicators, presence avatars, and activity feed re-joins. In a 2–4 person room this is very visible.
Two hardening options worth considering on top of the debounce bump:
- Explicitly untrack on tab close —
pagehide/beforeunloadfires even whenvisibilitychange→hiddenis throttled, and lets real departures short-circuit the 300s window:useEffect(() => { const onPageHide = () => { channelRef.current?.untrack() } window.addEventListener('pagehide', onPageHide) return () => window.removeEventListener('pagehide', onPageHide) }, [])- Adaptive grace — keep 300s only while
document.visibilityState === 'hidden'at the time of the leave event; collapse to ~30s otherwise (tab is visible, so this is almost certainly a real disconnect, not throttling).Also flagging the downstream effect: any consumer of
onParticipantLeave(toasts, activity messages, analytics) will now fire up to 5 minutes after the event — please sanity-check those consumers don't render stale timestamps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useSession.ts` around lines 180 - 203, The 5-minute unconditional grace in the channel.on('presence', { event: 'leave' }, ...) handler causes real disconnects to remain visible too long; change the logic in that handler to (1) short-circuit by calling channelRef.current?.untrack() on pagehide/beforeunload (add a useEffect that registers window.addEventListener('pagehide', onPageHide) and removes it on cleanup) so real tab closes immediately untrack, and (2) make the pendingLeaveTimers adaptive: when handling a leave, check document.visibilityState and use a short timeout (e.g., ~30s) if visibilityState === 'visible' and the long 300s only when visibilityState === 'hidden'; ensure you still clear existing timers from pendingLeaveTimers.current, delete the map entry when firing, call leaveCallbacksRef.current.forEach, and call setParticipants to remove the user, and verify any consumers of onParticipantLeave handle delayed callbacks correctly.app/api/og/route.tsx (1)
67-72:⚠️ Potential issue | 🟡 MinorTomato SVG still rendered under the Bonfire wordmark.
The wordmark is now
Bonfirebut the adjacent SVG is still a stem-and-fruit tomato (red circle + green stem). On a dark OG card next to "Bonfire" / "Bonfire" this reads as an unrelated icon. Likely a follow-up to swap for a flame glyph, but flagging so it doesn't ship as a branding mismatch in socials.Also applies to: 122-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/og/route.tsx` around lines 67 - 72, The SVG used next to the "Bonfire" wordmark is a tomato (rect + circle + ellipse + path) and should be replaced with a flame glyph to match branding; update the <svg> element(s) (the instances containing the rect, circle, path, ellipse elements around the wordmark — seen at the current block and the similar block around lines 122-135) to use a flame-shaped path/stroke or new flame SVG asset, keep appropriate sizing/viewBox and fill colors (e.g., orange/yellow gradients), and add an accessible <title> or aria-label like "Bonfire logo" so the OG card shows the flame instead of the tomato.components/session/SharePanel.tsx (2)
61-73: 🛠️ Refactor suggestion | 🟠 MajorNative-share title drifts from
lib/share.ts— pick one.
SharePanel.handleNativeShareuses'Bonfire Room'/'Bonfire: <name>', whilelib/share.ts::nativeShare(line 57) uses'Bonfire Session'/'Bonfire: <name>'. Same OS share sheet, two different titles depending on which path the caller takes. Worse,SharePanelhas re-implementednativeShareinline instead of calling the shared helper — so they will keep drifting.Consider delegating to the shared helper so title/text stay in lockstep:
♻️ Proposed refactor
-import { Link2, Check, Share2 } from 'lucide-react' -import { cn } from '@/lib/utils' +import { Link2, Check, Share2 } from 'lucide-react' +import { nativeShare } from '@/lib/share' @@ - const handleNativeShare = async () => { - if (typeof navigator !== 'undefined' && navigator.share) { - try { - await navigator.share({ - title: sessionName ? `Bonfire: ${sessionName}` : 'Bonfire Room', - text: 'Join my focus room!', - url, - }) - } catch { - // user cancelled or error - } - } - } + const handleNativeShare = async () => { + await nativeShare(sessionId, sessionName) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/session/SharePanel.tsx` around lines 61 - 73, handleNativeShare in SharePanel.tsx re-implements native sharing with a different title, causing drift; replace the inline implementation by importing and delegating to the shared helper lib/share.ts::nativeShare to keep titles/text consistent. Update SharePanel to import nativeShare and call nativeShare(sessionName, url) (or the helper's expected args), remove the duplicated try/catch block, and let nativeShare handle navigator.share checks and error handling so both paths use the same title/text.
13-20:⚠️ Potential issue | 🟠 MajorFix fallback order inconsistency between
getSessionUrlandformatSessionUrl.The two functions don't actually share the same fallback logic as claimed:
formatSessionUrl(lib/session.ts):NEXT_PUBLIC_APP_URL || (window ? window.location.origin : fallback)— env var wins on servergetSessionUrl(SharePanel.tsx):appUrl || (window ? window.location.origin : NEXT_PUBLIC_APP_URL || fallback)— window.location wins on clientIn client-side contexts (where
windowexists), both will usewindow.location.origin, which defeatsNEXT_PUBLIC_APP_URLeven when explicitly configured for preview/proxy scenarios. This is particularly problematic forgetSessionUrlsince it's used by the clipboard copy and social share flows — dev/preview URLs can leak into copied links and Twitter shares.Recommend
getSessionUrlmirrorformatSessionUrl's order (env var first) to centralize the behavior, or move both to a single shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/session/SharePanel.tsx` around lines 13 - 20, getSessionUrl currently prefers window.location.origin over NEXT_PUBLIC_APP_URL which causes client-side links to leak dev/preview URLs; update getSessionUrl to mirror formatSessionUrl's fallback order (use process.env.NEXT_PUBLIC_APP_URL first, then window.location.origin, then the hardcoded fallback) or consolidate both functions into a single shared utility so the same precedence is applied for clipboard copy and social share flows; adjust the logic inside getSessionUrl (and/or replace calls to it) accordingly, referencing getSessionUrl and formatSessionUrl to ensure consistent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/session/route.ts`:
- Line 17: The display_name Zod schema currently allows all-space strings;
update the schema for display_name to preprocess by trimming and converting
empty results to null (or undefined) and then validate a min length, e.g. use
z.preprocess((v) => typeof v === "string" ? v.trim() || null : v,
z.string().min(1).max(40).nullable().optional()), and apply the same
preprocessing/validation pattern to the other affected fields at the same schema
locations (the fields referenced around lines 45-47) so client-side trimming
cannot bypass server-side min-length enforcement.
In `@app/login/page.tsx`:
- Line 89: The line rendering the 📬 emoji in app/login/page.tsx contradicts the
rebrand rule to remove emojis; replace that div with a Lucide icon component
(e.g., Mail or Inbox) from 'lucide-react' and add the corresponding import (for
example import { Mail } from 'lucide-react'), then use the Mail (or Inbox)
component in the JSX with appropriate sizing/utility classes (e.g., h-10 w-10 or
text-4xl equivalent) so styling matches the original layout.
In `@components/home/HomeClient.tsx`:
- Around line 180-183: The code unsafely asserts user.user_metadata?.full_name
and avatar_url to string; instead validate types before using them: replace
casts like user.user_metadata?.full_name as string ?? 'User' and avatar_url
casts with a guarded expression that checks typeof user.user_metadata?.full_name
=== 'string' ? user.user_metadata.full_name : 'User' (and similar for
avatar_url), or use a safe converter only after checking the type; update
references in HomeClient (the user object usages) to use these guarded values so
downstream consumers receive a real string fallback instead of a misleading type
assertion.
- Around line 97-101: Replace the unsafe (document as any) cast with a proper
typed override: add an inline intersection type when calling
startViewTransition, e.g. cast document to Document & { startViewTransition?:
(cb: () => void | Promise<void>) => void } and call .startViewTransition(() =>
router.push(`/session/${id}`)); leave the existing 'in' check and fallback to
router.push(`/session/${id}`) unchanged so TypeScript remains strict-safe while
preserving behavior of document.startViewTransition and router.push.
- Around line 95-96: The code stores session and persistent values with the old
"pomodoro_" prefix (see localStorage.setItem calls around finalGuestName and id
in HomeClient.tsx); update storage keys to the new "bonfire_" prefix and add
migration logic that on app init reads any existing "pomodoro_*" keys (e.g.,
pomodoro_guest_id, pomodoro_ambient_type, pomodoro_ambient_volume,
pomodoro_nick_{id}, pomodoro_host_{id}), copies their values to the
corresponding "bonfire_*" keys, and then optionally removes the old keys to
avoid duplication; alternatively, if you intentionally keep
backward-compatibility without renaming, add a clear comment near the
localStorage usages (around finalGuestName/id and any guest/prefs handling)
stating the deliberate legacy prefix and why it remains.
In `@components/landing/LandingClient.tsx`:
- Around line 622-625: The footer in LandingClient.tsx displays "Bonfire" but
the anchor href still points to the old repo; update the anchor href in the
footer (the <a> element near the span "© {new Date().getFullYear()} Bonfire") to
the rebranded GitHub repository URL (replace
"https://github.com/MinitJain/pomodoro-jam" with
"https://github.com/MinitJain/bonfire") so the user-facing link matches the
Bonfire branding.
In `@components/session/BonfireScene.tsx`:
- Around line 556-567: The decorative WebGL scene rendered inside the div with
ref={containerRef} (wrapped around the conditional ready && <Canvas ... />) is
visual-only and should be hidden from assistive tech; add aria-hidden="true" to
that wrapper element (and optionally role="presentation" on the Canvas if
present) so screen readers ignore the unlabeled canvas while the separate timer
text remains accessible.
In `@components/session/GuideModal.tsx`:
- Around line 164-214: The dialog needs a focus trap and to restore previous
focus on close: add a ref (dialogRootRef) to the backdrop wrapper element,
capture document.activeElement on open, move focus into the currently visible
panel (the visible aside or mobile div containing GuideContent/header), and
attach a keydown handler that traps Tab/Shift+Tab inside that panel by cycling
focus among its focusable elements; on close remove the handler and restore the
saved active element; implement this logic in a useEffect inside the GuideModal
component and ensure the backdrop wrapper uses ref={dialogRootRef} so the effect
can locate the visible panel (use the aria-labelledby IDs or querySelector for
the visible role="dialog" element) and call focus() on the first tabbable
element.
In `@components/session/SessionProvider.tsx`:
- Around line 917-932: The toggle handler handleTogglePublic can leave
isTogglingPublic.current true if the async supabase update throws; wrap the
await call (or the whole try section) in a try/finally so that
isTogglingPublic.current is always reset to false in the finally block, preserve
the existing rollback behavior that calls setIsPublic(oldValue) on error, and
keep the same use of supabase.from("sessions").update({ is_public: newValue
}).eq("id", session.id) and console.error logging.
- Around line 872-903: The current handleApplySettings updates local state
(calls setSessionSettings and computes newState via reset(...)) before the
Supabase update, causing host-local divergence on DB failure; change the flow so
the DB update (supabase.from("sessions").update(...).eq("id", session.id)) is
awaited first and only after no error call setSessionSettings, compute newState
with reset(toSecs(...)), then broadcastWithCount(newState) and
broadcastShareLock(!newSettings.allowGuestShare); alternatively, if you prefer
to keep the optimistic update, capture the previous session settings and timer
state before calling setSessionSettings/reset and on DB error restore them and
avoid broadcasting.
- Around line 982-1147: roundLabel currently uses focusCount (initialized from
session.pomos_done) and incorrectly says "completed today"; update the logic
that builds roundLabel (symbol: roundLabel) to use todayCount for authenticated
users (symbol: todayCount / when userId is present) and keep "completed today",
otherwise use focusCount and change the copy to "completed in this room" so the
label reflects the correct source of the count.
- Around line 797-826: The handleSkip function currently treats any skip during
focus as a completed pomodoro by incrementing focusCountRef and setTodayCount
and passing pomos_done to enqueueSessionUpdate; change this so manual skips do
NOT count as completed pomodoros—remove the increment and setTodayCount calls
from the skippingFocus branch inside handleSkip (and stop sending pomos_done
there), and only increment focusCountRef, call setFocusCount, update
setTodayCount, and include pomos_done in enqueueSessionUpdate from the code path
that handles an actually expired/finished focus interval (the "finished" or
expiration handler that calls setMode/enqueueSessionUpdate), keeping other
behavior (nextMode calculation, broadcastActivity, setMode, broadcastWithCount)
unchanged.
In `@components/session/SettingsPanel.tsx`:
- Line 83: The active-theme detection uses theme from useTheme(), which can be
'system', causing both buttons to appear inactive; update the destructuring to
const { theme, resolvedTheme, setTheme } = useTheme() and use resolvedTheme (not
theme) when computing the aria-pressed/active state for the theme buttons (e.g.,
replace checks like theme === t or fallback logic with resolvedTheme === t,
preserving any existing dark-fallback behavior), leaving setTheme usage
unchanged.
In `@components/timer/ModeSelector.tsx`:
- Around line 48-49: The active button currently uses a hard-coded color '#fff'
in ModeSelector.tsx which fails WCAG AA; define a new CSS variable (e.g.
--on-accent) in globals.css with a color that meets 4.5:1 contrast for both
light and dark theme roots, and then replace the hard-coded color in
ModeSelector.tsx (the style object that sets background: 'var(--accent)') to use
color: 'var(--on-accent)'; ensure you set different --on-accent values under the
light/dark theme selectors if needed so contrast requirements are satisfied.
In `@components/timer/TimerDisplay.tsx`:
- Around line 23-30: TimerDisplay currently uses suppressHydrationWarning to
hide SSR/CSR mismatches; instead make it client-only or render a SSR
placeholder: update the parent to lazy-load TimerDisplay with Next's
dynamic(import) and ssr: false (providing a loading skeleton) and then remove
suppressHydrationWarning from TimerDisplay, or alternately inside TimerDisplay
implement a client-mount pattern (use a mounted state set in useEffect to render
the live timer only after mount and render a static placeholder during SSR).
Target the TimerDisplay component and its parent dynamic import/loading logic
when applying this change.
In `@hooks/useBonfireState.ts`:
- Around line 84-96: The useEffect that sets setIsSurging/setIntensityBoost
schedules a timeout via surgeTimerRef but does not clear it on unmount or when
the session resets; update the effect for focusCount (and the other effect that
uses accountabilityRef for the interval) to return a cleanup function that
clears surgeTimerRef (clearTimeout) and accountabilityRef (clearInterval) and
sets those refs to null, ensuring any pending timers (the surge timeout and the
accountability interval) are cancelled when the hook unmounts or session resets.
In `@lib/roomName.ts`:
- Line 19: Room names are now generated with spaces by the roomName function
(return `${adj} ${noun} ${num}`) but the LandingClient placeholder still shows
the old hyphenated example "e.g. focused-panda-342"; update the placeholder in
the LandingClient component (components/landing/LandingClient.tsx) to match the
new format (e.g. "e.g. focused panda 342") or, better, call roomName() to
produce a live example so the placeholder always matches the generator.
---
Outside diff comments:
In `@app/api/og/route.tsx`:
- Around line 67-72: The SVG used next to the "Bonfire" wordmark is a tomato
(rect + circle + ellipse + path) and should be replaced with a flame glyph to
match branding; update the <svg> element(s) (the instances containing the rect,
circle, path, ellipse elements around the wordmark — seen at the current block
and the similar block around lines 122-135) to use a flame-shaped path/stroke or
new flame SVG asset, keep appropriate sizing/viewBox and fill colors (e.g.,
orange/yellow gradients), and add an accessible <title> or aria-label like
"Bonfire logo" so the OG card shows the flame instead of the tomato.
In `@app/explore/page.tsx`:
- Line 155: Replace the stale tomato emoji in the Explore empty state with the
Lucide Flame icon: remove the literal "🍅" in the div inside
app/explore/page.tsx and import Flame from 'lucide-react', then render <Flame />
with the same styling classes (e.g., text-5xl mb-4 or equivalent) so the visual
size and spacing remain consistent; ensure you add the Flame import at the top
of the file and remove the emoji literal.
In `@components/session/SharePanel.tsx`:
- Around line 61-73: handleNativeShare in SharePanel.tsx re-implements native
sharing with a different title, causing drift; replace the inline implementation
by importing and delegating to the shared helper lib/share.ts::nativeShare to
keep titles/text consistent. Update SharePanel to import nativeShare and call
nativeShare(sessionName, url) (or the helper's expected args), remove the
duplicated try/catch block, and let nativeShare handle navigator.share checks
and error handling so both paths use the same title/text.
- Around line 13-20: getSessionUrl currently prefers window.location.origin over
NEXT_PUBLIC_APP_URL which causes client-side links to leak dev/preview URLs;
update getSessionUrl to mirror formatSessionUrl's fallback order (use
process.env.NEXT_PUBLIC_APP_URL first, then window.location.origin, then the
hardcoded fallback) or consolidate both functions into a single shared utility
so the same precedence is applied for clipboard copy and social share flows;
adjust the logic inside getSessionUrl (and/or replace calls to it) accordingly,
referencing getSessionUrl and formatSessionUrl to ensure consistent behavior.
In `@hooks/useSession.ts`:
- Around line 57-84: The presence re-track effect uses avatarUrlRef.current but
its dependency array omits avatarUrl, so avatar changes don’t trigger re-track;
update the effect that calls channelRef.current.track(...) (the first useEffect
referencing channelRef, usernameRef, avatarUrlRef, isHost, joinedAtRef) to
include avatarUrl in its dependency array (e.g. [isHost, username, avatarUrl])
so a new avatar triggers re-tracking and broadcasts the updated avatar_url to
peers.
- Around line 180-203: The 5-minute unconditional grace in the
channel.on('presence', { event: 'leave' }, ...) handler causes real disconnects
to remain visible too long; change the logic in that handler to (1)
short-circuit by calling channelRef.current?.untrack() on pagehide/beforeunload
(add a useEffect that registers window.addEventListener('pagehide', onPageHide)
and removes it on cleanup) so real tab closes immediately untrack, and (2) make
the pendingLeaveTimers adaptive: when handling a leave, check
document.visibilityState and use a short timeout (e.g., ~30s) if visibilityState
=== 'visible' and the long 300s only when visibilityState === 'hidden'; ensure
you still clear existing timers from pendingLeaveTimers.current, delete the map
entry when firing, call leaveCallbacksRef.current.forEach, and call
setParticipants to remove the user, and verify any consumers of
onParticipantLeave handle delayed callbacks correctly.
In `@package.json`:
- Around line 15-30: Update the `@react-three/fiber` dependency in package.json
from "8.18.0" to "9.6.0" to ensure React 19 compatibility; modify the version
string for the "@react-three/fiber" entry, run your package manager
(npm/yarn/pnpm) to install and then run the test/build to verify no breaking
changes affect BonfireScene or imports that reference `@react-three/fiber`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a112b448-b628-4883-92ef-f90edb856255
⛔ Files ignored due to path filters (6)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/apple-touch-icon.pngis excluded by!**/*.pngpublic/favicon-32.pngis excluded by!**/*.pngpublic/favicon.icois excluded by!**/*.icopublic/icon-192.pngis excluded by!**/*.pngpublic/icon-512.pngis excluded by!**/*.png
📒 Files selected for processing (43)
.github/workflows/ci.ymlCLAUDE.mdROADMAP.mdapp/api/og/route.tsxapp/api/session/route.tsapp/explore/page.tsxapp/globals.cssapp/layout.tsxapp/login/page.tsxapp/page.tsxapp/privacy/page.tsxapp/profile/[username]/page.tsxapp/session/[id]/page.tsxapp/sitemap.tsapp/terms/page.tsxcomponents/home/HomeClient.tsxcomponents/landing/LandingClient.tsxcomponents/landing/TimerPreview.tsxcomponents/session/AmbientPlayer.tsxcomponents/session/BonfireScene.tsxcomponents/session/GuideModal.tsxcomponents/session/KeyboardShortcutsModal.tsxcomponents/session/SessionProvider.tsxcomponents/session/SettingsPanel.tsxcomponents/session/SharePanel.tsxcomponents/session/StatsTab.tsxcomponents/timer/ModeSelector.tsxcomponents/timer/TimerDisplay.tsxcomponents/ui/Logo.tsxcomponents/ui/PolicyPageLayout.tsxhooks/useBonfireState.tshooks/useSession.tshooks/useTimer.tslib/ambient.tslib/favicon.tslib/roomName.tslib/session.tslib/share.tsnext.config.jspackage.jsonpublic/manifest.jsonsupabase/migrations/001_init.sqltailwind.config.ts
💤 Files with no reviewable changes (1)
- components/session/KeyboardShortcutsModal.tsx
Comment on lines
+95
to
+96
| if (finalGuestName) localStorage.setItem(`pomodoro_nick_${id}`, finalGuestName) | ||
| localStorage.setItem(`pomodoro_host_${id}`, '1') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP "pomodoro_(nick|host)_" --type=ts --type=tsxRepository: MinitJain/bonfire
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for pomodoro_nick_ and pomodoro_host_ keys across TypeScript files
rg "pomodoro_(nick|host)_" --type-list | grep -E "(typescript|tsx)"Repository: MinitJain/bonfire
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Broader search for these keys without type restrictions
rg "pomodoro_(nick|host)_" --multiline -A 2 -B 2Repository: MinitJain/bonfire
Length of output: 2623
🏁 Script executed:
#!/bin/bash
# Also check the HomeClient.tsx file specifically to see context
fd "HomeClient.tsx" --exec cat -n {} \;Repository: MinitJain/bonfire
Length of output: 21918
🏁 Script executed:
#!/bin/bash
# Search for migration-related comments or utilities
rg -i "migrat" --type tsRepository: MinitJain/bonfire
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for any "bonfire_" prefix usage to see if they renamed elsewhere
rg "bonfire_" --type tsRepository: MinitJain/bonfire
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if these keys are used anywhere else or if there's a utility that handles them
rg "localStorage\.(get|set)Item.*pomodoro_" -A 2 -B 2Repository: MinitJain/bonfire
Length of output: 5068
Address stale "pomodoro_" localStorage prefix across persistent keys
The app uses pomodoro_ prefixed keys for both session-scoped data (pomodoro_nick_*, pomodoro_host_*) and persistent cross-session data (pomodoro_guest_id, pomodoro_ambient_type, pomodoro_ambient_volume). Since the product is now "Bonfire", either:
- Add an explanatory comment if this is intentional for backward-compat
- Rename keys and implement migration logic (read both old and new prefixes during transition) to keep in-session and preference logic in lockstep
Persistent keys like pomodoro_guest_id and ambient settings make this more impactful than session-only keys—stale branding across stored user preferences should be addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/home/HomeClient.tsx` around lines 95 - 96, The code stores session
and persistent values with the old "pomodoro_" prefix (see localStorage.setItem
calls around finalGuestName and id in HomeClient.tsx); update storage keys to
the new "bonfire_" prefix and add migration logic that on app init reads any
existing "pomodoro_*" keys (e.g., pomodoro_guest_id, pomodoro_ambient_type,
pomodoro_ambient_volume, pomodoro_nick_{id}, pomodoro_host_{id}), copies their
values to the corresponding "bonfire_*" keys, and then optionally removes the
old keys to avoid duplication; alternatively, if you intentionally keep
backward-compatibility without renaming, add a clear comment near the
localStorage usages (around finalGuestName/id and any guest/prefs handling)
stating the deliberate legacy prefix and why it remains.
Comment on lines
+48
to
+49
| background: 'var(--accent)', | ||
| color: '#fff', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if --accent color values are defined and verify contrast assumptions
rg -n --type css --type scss -C2 --color=always '(--accent).*:.*#[0-9a-fA-F]{3,8}' app/ components/ || echo "No hardcoded accent colors found in CSS"
echo ""
echo "Also check globals.css for accent definitions:"
fd -e css globals.css --exec cat {}Repository: MinitJain/bonfire
Length of output: 8078
🏁 Script executed:
cat -n components/timer/ModeSelector.tsx | head -80Repository: MinitJain/bonfire
Length of output: 2345
🏁 Script executed:
python3 << 'EOF'
import re
def hex_to_rgb(hex_color):
hex_color = hex_color.lstrip('#')
return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4))
def get_luminance(rgb):
r, g, b = [x / 255.0 for x in rgb]
r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4
g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4
b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4
return 0.2126 * r + 0.7152 * g + 0.0722 * b
def get_contrast_ratio(color1, color2):
l1 = get_luminance(hex_to_rgb(color1))
l2 = get_luminance(hex_to_rgb(color2))
lighter = max(l1, l2)
darker = min(l1, l2)
return (lighter + 0.05) / (darker + 0.05)
# White text
white = '#ffffff'
# Light mode accent
light_accent = '#E8472A'
# Dark mode accent
dark_accent = '#FF5533'
# Calculate contrast
light_contrast = get_contrast_ratio(white, light_accent)
dark_contrast = get_contrast_ratio(white, dark_accent)
print(f"White (`#fff`) on Light Mode Accent (`#E8472A`): {light_contrast:.2f}:1")
print(f"White (`#fff`) on Dark Mode Accent (`#FF5533`): {dark_contrast:.2f}:1")
print(f"\nWCAG AA requirement: 4.5:1 for normal text, 3:1 for large text")
print(f"\nLight mode: {'✓ PASS' if light_contrast >= 4.5 else '✗ FAIL'} AA (normal text)")
print(f"Dark mode: {'✓ PASS' if dark_contrast >= 4.5 else '✗ FAIL'} AA (normal text)")
EOFRepository: MinitJain/bonfire
Length of output: 299
Use CSS variable for active button text color to meet WCAG AA contrast.
Hard-coded color: '#fff' on the active button fails WCAG AA contrast requirements:
- Light mode (accent
#E8472A): 3.91:1 — below 4.5:1 threshold - Dark mode (accent
#FF5533): 3.18:1 — below 4.5:1 threshold
Define a --on-accent CSS variable (or similar) in globals.css with sufficient contrast for both themes, then use it here instead of the hard-coded white.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/timer/ModeSelector.tsx` around lines 48 - 49, The active button
currently uses a hard-coded color '#fff' in ModeSelector.tsx which fails WCAG
AA; define a new CSS variable (e.g. --on-accent) in globals.css with a color
that meets 4.5:1 contrast for both light and dark theme roots, and then replace
the hard-coded color in ModeSelector.tsx (the style object that sets background:
'var(--accent)') to use color: 'var(--on-accent)'; ensure you set different
--on-accent values under the light/dark theme selectors if needed so contrast
requirements are satisfied.
Comment on lines
+23
to
+30
| suppressHydrationWarning | ||
| className={cn('flex flex-col items-center gap-2', className)} | ||
| aria-label={`Timer: ${formatted}, ${modeLabel[mode]}`} | ||
| role="timer" | ||
| aria-live="off" // intentional: timer updates every second; polite/assertive would spam screen readers | ||
| > | ||
| <span | ||
| suppressHydrationWarning |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider client-only rendering to avoid hydration warnings.
Adding suppressHydrationWarning works but is a workaround. Since the timer value is inherently client-dynamic and updates every second, consider:
- Rendering
TimerDisplayclient-side only (wrap parent in dynamic import withssr: false), or - Using a placeholder/skeleton during SSR and mounting the real timer on client.
This eliminates the root cause of the mismatch rather than suppressing the warning.
♻️ Alternative: Client-only pattern example
In the parent component that uses TimerDisplay:
'use client'
import dynamic from 'next/dynamic'
const TimerDisplay = dynamic(() => import('./TimerDisplay').then(m => ({ default: m.TimerDisplay })), {
ssr: false,
loading: () => <div className="h-20 animate-pulse bg-surface rounded" />
})Then remove suppressHydrationWarning from this component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/timer/TimerDisplay.tsx` around lines 23 - 30, TimerDisplay
currently uses suppressHydrationWarning to hide SSR/CSR mismatches; instead make
it client-only or render a SSR placeholder: update the parent to lazy-load
TimerDisplay with Next's dynamic(import) and ssr: false (providing a loading
skeleton) and then remove suppressHydrationWarning from TimerDisplay, or
alternately inside TimerDisplay implement a client-mount pattern (use a mounted
state set in useEffect to render the live timer only after mount and render a
static placeholder during SSR). Target the TimerDisplay component and its parent
dynamic import/loading logic when applying this change.
- Skip counting: only increment focusCount on status=finished, not mid-session skips - Settings apply: move setSessionSettings/setShowSettings after DB write succeeds - isTogglingPublic: wrap Supabase call in try/finally to guarantee reset - roundLabel: use todayCount for auth users, focusCount for guests - Explore empty state: replace stale tomato emoji with Lucide Flame - Login page: replace 📬 emoji with Lucide Mail icon - LandingClient: fix GitHub URL (pomodoro-jam -> bonfire) and placeholder text - BonfireScene: add aria-hidden to decorative WebGL container - HomeClient: typed document.startViewTransition override, safe user_metadata access - API route: add .trim().min(1) to display_name Zod schema - useBonfireState: store return-surge timer in ref, clear on cleanup - SettingsPanel: use resolvedTheme instead of theme for active button state Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/api/session/route.ts (2)
85-99:⚠️ Potential issue | 🟡 MinorValidate the
idquery param with Zod before querying Supabase.
GETcurrently uses the rawidfromURLSearchParams. Add a Zod schema for the query input before.eq('id', id).🛡️ Proposed validation
+const GetSessionSchema = z.object({ + id: z.string().min(1), +}) + export async function GET(request: Request) { try { const supabase = createClient() const { searchParams } = new URL(request.url) - const id = searchParams.get('id') + const parsed = GetSessionSchema.safeParse({ + id: searchParams.get('id'), + }) - if (!id) { + if (!parsed.success) { return NextResponse.json({ error: 'Session ID required' }, { status: 400 }) } + const { id } = parsed.dataAs per coding guidelines, app/api routes require “All inputs must be validated with Zod before use.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/session/route.ts` around lines 85 - 99, Add Zod validation for the incoming query param before using it in the Supabase query: in the GET handler, define a Zod schema (e.g., const QuerySchema = z.object({ id: z.string().min(1) })) and parse/validate the URL search params (use QuerySchema.parse or safeParse on an object built from searchParams) and return a 400 NextResponse when validation fails; then use the validated id value (not the raw searchParams.get result) when calling supabase.from('sessions').select('*').eq('id', id).maybeSingle(). Ensure validation happens before createClient()/supabase query to satisfy the “All inputs must be validated with Zod” rule.
7-10:⚠️ Potential issue | 🟠 MajorAdd real rate limiting to public session creation.
Input validation and RLS do not throttle room-creation spam. Use a shared limiter such as Vercel KV or a Supabase-backed counter before inserting sessions.
As per coding guidelines, app/api routes must “Check for rate limiting on any public endpoints.”
Also applies to: 20-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/session/route.ts` around lines 7 - 10, The public session creation route lacks a shared rate limiter—replace the in-memory Map check with a centralized limiter (e.g., Vercel KV or a Supabase counter) and enforce it inside the session POST handler before any DB insert; locate the POST handler / createSession logic in app/api/session/route.ts (the function handling the incoming request and calling the session insert) and add: 1) a lookup/increment in the chosen shared store keyed by requester identity (IP or user id) with an expiry window, 2) a reject path that returns an appropriate 429 response when the limit is exceeded, and 3) only proceed to Zod validation and session insertion when the limiter permits. Ensure the limiter call happens for all public requests and that errors from the shared store are handled gracefully.
♻️ Duplicate comments (6)
hooks/useBonfireState.ts (1)
89-100:⚠️ Potential issue | 🟡 MinorClear
surgeTimerRefon unmount.The completion-surge timeout still has no cleanup path, so it can fire after the hook unmounts.
🧹 Proposed cleanup
useEffect(() => { if (focusCount > prevFocusCountRef.current) { setIsSurging(true) setIntensityBoost(0.35) if (surgeTimerRef.current) clearTimeout(surgeTimerRef.current) surgeTimerRef.current = setTimeout(() => { setIsSurging(false) setIntensityBoost(0) }, 2000) } prevFocusCountRef.current = focusCount + return () => { + if (surgeTimerRef.current) { + clearTimeout(surgeTimerRef.current) + surgeTimerRef.current = null + } + } }, [focusCount])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useBonfireState.ts` around lines 89 - 100, The effect that starts the completion-surge timeout (inside the useEffect in useBonfireState that references surgeTimerRef and focusCount) needs a cleanup to clear the timeout on unmount; add and return a cleanup function from that useEffect which checks surgeTimerRef.current, calls clearTimeout on it, and sets surgeTimerRef.current to null (in addition to the existing logic that clears prior timeouts before creating a new one).components/home/HomeClient.tsx (2)
164-167: 🧹 Nitpick | 🔵 TrivialNarrow
user_metadatabefore passing it toAvatar.The menu label was fixed, but
Avatarstill receives asserted metadata values. Avoidas stringon untrusted metadata.🔒 Proposed type-safe narrowing
+ const metadataAvatarUrl = + typeof user?.user_metadata?.avatar_url === 'string' + ? user.user_metadata.avatar_url + : undefined + const metadataFullName = + typeof user?.user_metadata?.full_name === 'string' + ? user.user_metadata.full_name + : undefined + @@ <Avatar - src={user.user_metadata?.avatar_url as string | undefined} - name={(user.user_metadata?.full_name as string | undefined) ?? user.email ?? '?'} + src={metadataAvatarUrl} + name={metadataFullName ?? user.email ?? '?'} size="sm" />As per coding guidelines, TypeScript application code should run in strict mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/home/HomeClient.tsx` around lines 164 - 167, Avatar is being passed asserted values from user.user_metadata (user.user_metadata?.avatar_url as string and full_name as string) which bypasses type-safety; update HomeClient so you narrow/validate user.user_metadata before calling Avatar: read user.user_metadata into a local const, check typeof avatar_url === 'string' and typeof full_name === 'string' (or otherwise fallback to undefined or user.email), then pass those validated values to Avatar's src and name props (referencing Avatar and the user.user_metadata?.avatar_url / user.user_metadata?.full_name expressions) instead of using "as string" casts.
95-96:⚠️ Potential issue | 🟡 MinorResolve the stale
pomodoro_storage prefix.These keys persist the old brand. Either migrate to
bonfire_with backward-compatible reads, or add a clear compatibility comment if the legacy prefix is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/home/HomeClient.tsx` around lines 95 - 96, Replace the hardcoded legacy localStorage keys in HomeClient.tsx with the new bonfire_ prefix and add backward-compatible reads: when reading a name or host flag, first try localStorage.getItem(`bonfire_nick_${id}`) / `bonfire_host_${id}` and if absent fall back to `pomodoro_nick_${id}` / `pomodoro_host_${id}`; when writing (e.g., where finalGuestName is saved and where host flag is set), write to `bonfire_nick_${id}` and `bonfire_host_${id}` and optionally remove or keep the old `pomodoro_` keys for migration, or add a short compatibility comment if you intentionally keep the old keys. Ensure you update the write sites that currently call localStorage.setItem(`pomodoro_nick_${id}`, finalGuestName) and localStorage.setItem(`pomodoro_host_${id}`, '1') to use the new keys and include the fallback read logic in the same component.components/session/SessionProvider.tsx (3)
877-903:⚠️ Potential issue | 🟠 MajorDo not call
reset(...)before the settings write succeeds.
reset(toSecs(...))still mutates local timer state before the Supabase update. If the write fails, the host UI resets while the DB/watchers keep the old settings. Move the stateful reset after the successful write, or compute the DB patch from pure duration values first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/session/SessionProvider.tsx` around lines 877 - 903, In handleApplySettings, avoid calling reset(toSecs(...)) before the Supabase update because reset mutates local timer state; instead build the DB patch from newSettings.durations/rounds/flags (convert durations to seconds as needed for the DB) and perform the supabase.from("sessions").update(...) using those pure values, check for error and return false if it fails, and only after a successful update call reset(toSecs(newSettings.durations)) to produce newState, then call broadcastWithCount(newState) and broadcastShareLock(!newSettings.allowGuestShare).
797-843:⚠️ Potential issue | 🟠 MajorAdd
statustohandleSkipdependencies to fix stale closure on completion counting.The callback reads
status === "finished"on line 802 to determine whether to count a focus completion, butstatusis not in the dependency array. This creates a stale closure: if the timer naturally finishes and the user then clicks skip, the callback still sees the oldstatusvalue from when it was first created, preventing the completion from being counted. The completion counter should only increment for naturally finished sessions, but this bug causes it to skip counting them when the user skips after the timer expires.🐛 Proposed fix
}, [ mode, + status, actorName, setMode,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/session/SessionProvider.tsx` around lines 797 - 843, handleSkip closes over status to compute countCompletion but status is missing from the dependency array causing a stale-closure bug; update the handleSkip useCallback dependencies to include status so the callback sees current status when checking (in the handleSkip function where it computes countCompletion = skippingFocus && status === "finished") and rerun effects accordingly.
922-939:⚠️ Potential issue | 🟠 MajorAdd
catchblock to rollback optimistic state on promise rejection.The
finallyblock resets the guard, but if the Supabase operation throws (network errors, timeouts, auth failures), the promise rejection bypasses theif (error)check entirely, leavingisPublicin the wrong state with an unhandled rejection. State must be rolled back on both structured errors and thrown exceptions.Proposed fix
setIsPublic(newValue); try { const { error } = await supabase .from("sessions") .update({ is_public: newValue }) .eq("id", session.id); if (error) { - console.error("[handleTogglePublic] DB update failed:", error); - setIsPublic(oldValue); + throw error; } + } catch (error) { + console.error("[handleTogglePublic] DB update failed:", error); + setIsPublic(oldValue); } finally { isTogglingPublic.current = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/session/SessionProvider.tsx` around lines 922 - 939, handleTogglePublic uses optimistic update but only checks supabase's returned { error } and relies on finally to clear isTogglingPublic; add a catch block around the await supabase.from("sessions").update(...).eq("id", session.id) to handle thrown exceptions (network/auth/timeouts) so you rollback the optimistic state: in the catch(err) call setIsPublic(oldValue) and log the error (e.g., console.error("[handleTogglePublic] request failed", err)); keep the existing finally to reset isTogglingPublic.current = false so the guard still clears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/home/HomeClient.tsx`:
- Around line 44-48: handleCreate can be invoked multiple times via Enter key
presses even while a create is in-flight; add a guard to prevent duplicate
requests by introducing a creation-in-progress flag (either a useRef like
creatingRef or a piece of state e.g. isCreating) that is set true at the start
of handleCreate and checked at the top to early-return if already true, and set
back to false only on error (or left true through successful navigation). Update
all places that call handleCreate (including the Enter key handlers tied to
roomNameInputRef, signInRef and the Start button handlers referenced near the
other handlers) to check the same flag before invoking handleCreate and ensure
the Start button is disabled/aria-disabled while creating so UI and keyboard
events are consistent.
In `@package.json`:
- Line 18: The package "@types/three" is a TypeScript declaration package and
should be moved from runtime dependencies into devDependencies to avoid
increasing production install surface; update package.json by removing
"@types/three" from the "dependencies" object and adding it under
"devDependencies" with the same version spec, then run your package manager
(npm/yarn/pnpm) to update lockfile so the change is reflected in the lockfile
and CI.
---
Outside diff comments:
In `@app/api/session/route.ts`:
- Around line 85-99: Add Zod validation for the incoming query param before
using it in the Supabase query: in the GET handler, define a Zod schema (e.g.,
const QuerySchema = z.object({ id: z.string().min(1) })) and parse/validate the
URL search params (use QuerySchema.parse or safeParse on an object built from
searchParams) and return a 400 NextResponse when validation fails; then use the
validated id value (not the raw searchParams.get result) when calling
supabase.from('sessions').select('*').eq('id', id).maybeSingle(). Ensure
validation happens before createClient()/supabase query to satisfy the “All
inputs must be validated with Zod” rule.
- Around line 7-10: The public session creation route lacks a shared rate
limiter—replace the in-memory Map check with a centralized limiter (e.g., Vercel
KV or a Supabase counter) and enforce it inside the session POST handler before
any DB insert; locate the POST handler / createSession logic in
app/api/session/route.ts (the function handling the incoming request and calling
the session insert) and add: 1) a lookup/increment in the chosen shared store
keyed by requester identity (IP or user id) with an expiry window, 2) a reject
path that returns an appropriate 429 response when the limit is exceeded, and 3)
only proceed to Zod validation and session insertion when the limiter permits.
Ensure the limiter call happens for all public requests and that errors from the
shared store are handled gracefully.
---
Duplicate comments:
In `@components/home/HomeClient.tsx`:
- Around line 164-167: Avatar is being passed asserted values from
user.user_metadata (user.user_metadata?.avatar_url as string and full_name as
string) which bypasses type-safety; update HomeClient so you narrow/validate
user.user_metadata before calling Avatar: read user.user_metadata into a local
const, check typeof avatar_url === 'string' and typeof full_name === 'string'
(or otherwise fallback to undefined or user.email), then pass those validated
values to Avatar's src and name props (referencing Avatar and the
user.user_metadata?.avatar_url / user.user_metadata?.full_name expressions)
instead of using "as string" casts.
- Around line 95-96: Replace the hardcoded legacy localStorage keys in
HomeClient.tsx with the new bonfire_ prefix and add backward-compatible reads:
when reading a name or host flag, first try
localStorage.getItem(`bonfire_nick_${id}`) / `bonfire_host_${id}` and if absent
fall back to `pomodoro_nick_${id}` / `pomodoro_host_${id}`; when writing (e.g.,
where finalGuestName is saved and where host flag is set), write to
`bonfire_nick_${id}` and `bonfire_host_${id}` and optionally remove or keep the
old `pomodoro_` keys for migration, or add a short compatibility comment if you
intentionally keep the old keys. Ensure you update the write sites that
currently call localStorage.setItem(`pomodoro_nick_${id}`, finalGuestName) and
localStorage.setItem(`pomodoro_host_${id}`, '1') to use the new keys and include
the fallback read logic in the same component.
In `@components/session/SessionProvider.tsx`:
- Around line 877-903: In handleApplySettings, avoid calling reset(toSecs(...))
before the Supabase update because reset mutates local timer state; instead
build the DB patch from newSettings.durations/rounds/flags (convert durations to
seconds as needed for the DB) and perform the
supabase.from("sessions").update(...) using those pure values, check for error
and return false if it fails, and only after a successful update call
reset(toSecs(newSettings.durations)) to produce newState, then call
broadcastWithCount(newState) and
broadcastShareLock(!newSettings.allowGuestShare).
- Around line 797-843: handleSkip closes over status to compute countCompletion
but status is missing from the dependency array causing a stale-closure bug;
update the handleSkip useCallback dependencies to include status so the callback
sees current status when checking (in the handleSkip function where it computes
countCompletion = skippingFocus && status === "finished") and rerun effects
accordingly.
- Around line 922-939: handleTogglePublic uses optimistic update but only checks
supabase's returned { error } and relies on finally to clear isTogglingPublic;
add a catch block around the await
supabase.from("sessions").update(...).eq("id", session.id) to handle thrown
exceptions (network/auth/timeouts) so you rollback the optimistic state: in the
catch(err) call setIsPublic(oldValue) and log the error (e.g.,
console.error("[handleTogglePublic] request failed", err)); keep the existing
finally to reset isTogglingPublic.current = false so the guard still clears.
In `@hooks/useBonfireState.ts`:
- Around line 89-100: The effect that starts the completion-surge timeout
(inside the useEffect in useBonfireState that references surgeTimerRef and
focusCount) needs a cleanup to clear the timeout on unmount; add and return a
cleanup function from that useEffect which checks surgeTimerRef.current, calls
clearTimeout on it, and sets surgeTimerRef.current to null (in addition to the
existing logic that clears prior timeouts before creating a new one).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7e4eddb-0c54-492c-8174-344b1a017f7b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
app/api/session/route.tsapp/explore/page.tsxapp/login/page.tsxcomponents/home/HomeClient.tsxcomponents/landing/LandingClient.tsxcomponents/session/BonfireScene.tsxcomponents/session/SessionProvider.tsxcomponents/session/SettingsPanel.tsxhooks/useBonfireState.tspackage.json
- handleCreate: early-return if isCreating is true, prevents duplicate requests from rapid Enter key presses while a create is in-flight - @types/three: moved from dependencies to devDependencies (types-only package has no runtime surface, reduces production install size) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--text-muteddark mode #5A5A50 → #7E7E72 (2.75:1 → 4.67:1)clamp(140px, 26vh, 260px), tighter padding for 13" MacBookTest plan
?click and?keypress, closes on Escape/backdrop/X25,24... (minutes only, no seconds)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor
Branding