Conversation
- clean up to use the new typed arena client/socket interfaces - move file arounds for more readability - replace broken draw/code/typing implementations with read-only, concept-preserving stubs - keep typed boundaries intact while deferring action/event behavior
📝 WalkthroughWalkthroughThis pull request refactors the arena feature's component architecture from prop-drilling to a data-driven pattern using generic props. It relocates ArenaActive, introduces EventBridge for event management, enhances useArenaSocket with enabled flags and robust error handling, and updates Draw/Code/Typing arena components to receive data through a unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
|
@greptile review |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/routes/arena/-lobby.tsx
Line: 82:93
Comment:
**Always toasts success**
`handleCopyLink` shows `toast.success("Link copied to clipboard!")` even when `navigator.clipboard.writeText(...)` fails (it toasts error then immediately toasts success). This is user-visible and will mislead users on browsers/contexts where clipboard write is denied.
How can I resolve this? If you propose a fix, please make it concise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/routes/arena/-lobby.tsx (1)
82-93:⚠️ Potential issue | 🟡 MinorBug: success toast fires even when clipboard write fails.
When
navigator.clipboard.writeTextrejects,successisfalse, the error toast fires at Line 89, but execution continues to Line 92, which unconditionally shows the success toast.🐛 Proposed fix
const handleCopyLink = async () => { const success = await navigator.clipboard .writeText(window.location.href) .then(() => true) .catch(() => false) if (!success) { toast.error("Failed to copy link to clipboard") + return } toast.success("Link copied to clipboard!") }src/routes/arena/$arenaId.tsx (1)
93-93: 🧹 Nitpick | 🔵 TrivialNon-null assertion is unnecessary —
ArenaEndedProps.endedAtalready acceptsundefined.Since
endedAtis typed asnumber | undefinedinArenaEndedProps, you can passarena.endedAtdirectly without the!assertion.Suggested fix
- endedAt={arena.endedAt!} + endedAt={arena.endedAt}
🤖 Fix all issues with AI agents
In `@src/routes/arena/-active.tsx`:
- Around line 184-232: Replace the mutable let arenaContent with a const by
moving the switch into a small resolver (either an IIFE or a helper function
like resolveArenaContent) that returns the JSX for DrawArena, CodeArena or
TypingArena and assign that result to const arenaContent; keep using the
existing props (user._id, data, participants, isSpectator, prompt, sendAction,
and bridgeRef.current as EventBridge<...>) when constructing each component. In
the switch default branch keep the exhaustive never check (const _exhaustive:
never = type) but ensure the resolver returns a concrete fallback (e.g., null)
or throws after logging so TypeScript understands arenaContent is always
assigned. This removes the let and fixes the unassigned-default problem.
In `@src/routes/arena/-code/arena.tsx`:
- Around line 24-32: Extract the duplicated participant-sorting logic into a new
custom hook named useSortedParticipants(participants, userId) that returns the
memoized sorted array; replace the inline useMemo in both DrawArena and
CodeArena (the sortedParticipants definition) with calls to
useSortedParticipants(participants, userId) so the current user is always first
while preserving memoization and the same dependency list.
- Around line 38-56: Extract the repeated "Problem" header block into a small
reusable component (e.g., PromptHeader) that accepts the prompt string (prop
name: prompt) and renders the same structure currently duplicated; then replace
both inline blocks in the empty-state branch and the non-empty-state branch with
<PromptHeader prompt={prompt} /> (or equivalent) to remove duplication while
keeping existing classNames and layout; ensure the new component is defined in
the same module (or imported) and used in place of the duplicate JSX so
sortedParticipants logic and prompt variable usage remain unchanged.
In `@src/routes/arena/-code/pane.tsx`:
- Around line 128-132: The rendered cursor display in pane.tsx incorrectly
treats CursorPos.x as line and CursorPos.y as column; locate the render block
that checks {!isEditable && cursor && ...} and fix by mapping spatial CursorPos
to text coordinates (either introduce a dedicated TextCursorPos with
{line,column} and convert CursorPos → TextCursorPos, or swap the fields when
rendering so line = Math.round(cursor.y) and column = Math.round(cursor.x));
update any helper or prop types you add and document the mapping so future
maintainers know CursorPos is spatial while the code pane uses line/column.
In `@src/routes/arena/-draw/arena.tsx`:
- Around line 39-53: Extract the duplicated header JSX (the block that displays
"Challenge" and the prompt using prompt) into a shared component (e.g.,
PromptHeader) and replace both occurrences in this file and in CodeArena with
that component; specifically locate the duplicated JSX around the early return
when sortedParticipants.length === 0 and the identical header in the main
render, move the markup into PromptHeader (accepting prompt as a prop) and
import/use it in place of the inline markup to avoid duplication.
- Around line 109-118: The FocusOverlay is currently given all entries from
cursorsByParticipant which includes the focused participant's own cursor; update
the cursors prop construction in the FocusOverlay invocation to exclude the
focused participant (i.e., filter out entries where participantId ===
focusedParticipant.id) similar to how paneCursors is created; keep the same
mapping to { participantId, ...cursor } so the shape stays identical and still
use userId to determine whether to pass an empty array when
focusedParticipant.id === userId.
In `@src/routes/arena/-draw/canvas.tsx`:
- Around line 7-25: DrawCanvasProps declares ownerId, onElementsChange, and
onCursorMove but the DrawCanvas function no longer destructures or uses them;
either remove these fields from the DrawCanvasProps interface or mark them as
intentionally unused by renaming to _ownerId, _onElementsChange, and
_onCursorMove (or prefix with underscore in the parameter destructure) so the
contract matches implementation and linter warnings go away; update the
DrawCanvas function signature/props destructuring (and any references inside
DrawCanvas) to match the chosen approach (remove unused props or rename them)
while keeping ownerUsername, isEditable, elements, cursors, and onFocus
unchanged.
In `@src/routes/arena/-event-bridge.ts`:
- Around line 6-27: The createEventBridge function currently allows silent
replacement of an existing subscriber and clears the entire queue even if the
subscriber throws during the flush; update sub, emit, and unsub to be safer:
either enforce the single-listener policy by throwing if listener is already set
in sub (or explicitly document behavior) and, when flushing queued events in
sub, drain the queue one-by-one calling the listener and stop on the first error
so that unprocessed events remain in q (do not zero q unconditionally), leaving
unsub to still clear listener and queue when explicitly called; reference
createEventBridge, sub, emit, unsub, listener, and q when making these changes.
In `@src/routes/arena/-hooks/use-arena-socket.ts`:
- Around line 73-81: The options object passed to the websocket hook is
recreated every render so onOpen/onMessage can capture stale
userId/username/config; memoize the options (or memoize the handlers) using
useMemo/useCallback so that options is recreated only when its dependencies
change: include userId, username, config, onArenaEvent, onGameOver, enabled (and
any functions like sendJsonMessage or setState if not stable) in the dependency
list; ensure the onOpen handler inside options sends the init ClientMsg<T> using
the current values and onMessage uses the current onArenaEvent/onGameOver by
referencing the memoized handlers instead of inline closures.
In `@src/routes/arena/-props.tsx`:
- Around line 1-18: This file only exports types (ArenaComponentProps, ArenaType
import aliases, and EventBridge usage) and contains no JSX, so rename the module
from -props.tsx to -props.ts and update any imports that reference "./-props" in
the codebase; ensure the exported types (ArenaComponentProps and referenced
types like ArenaData, ArenaEvent, ClientAction, Participant, and EventBridge)
remain unchanged and that your build/tsconfig allows .ts imports.
In `@src/routes/arena/-typing/arena.tsx`:
- Line 21: The useMemo around previewText is unnecessary for a cheap string
slice; replace the memoized value with a plain constant by computing previewText
directly from prompt (e.g., const previewText = prompt.slice(0, 320)) and remove
the useMemo import/usage so the component no longer creates a memo for this
trivial operation.
|
@greptile re-review |
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: shared/arena-protocol.ts
Line: 90:106
Comment:
**Type alias uses ActionMap**
`ClientAction<T>` is defined as `ActionMap[T]` before `ActionMap` is declared. TypeScript generally requires referenced type aliases to be declared before use, so this can break typechecking in `shared/arena-protocol.ts` after this reordering.
```suggestion
export type ActionMap = {
draw: CursorUpdate | CanvasUpdate
code: CursorUpdate | CodeUpdate | CodeRun
typing: TypingProgressUpdate
}
export type ClientAction<T extends ArenaType> = ActionMap[T]
export type DrawAction = ActionMap["draw"]
export type CodeAction = ActionMap["code"]
export type TypingAction = ActionMap["typing"]
```
How can I resolve this? If you propose a fix, please make it concise. |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
@greptile review |
| export type EventBridge<T> = { | ||
| emit: (event: T) => void | ||
| sub: (listener: (event: T) => void) => void |
There was a problem hiding this comment.
Single subscriber overwrites
sub() overwrites the previous listener without notifying/unsubscribing it, and unsub() clears all queued events. If multiple components subscribe (or a component re-subscribes), earlier subscribers will silently stop receiving events and queued events can be dropped unexpectedly.
This is especially easy to hit if each arena pane/child uses the bridge. Consider supporting multiple listeners (Set) or making sub() return an unsubscribe function scoped to that listener, without clearing global queue on unrelated unsubs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/arena/-event-bridge.ts
Line: 1:3
Comment:
**Single subscriber overwrites**
`sub()` overwrites the previous `listener` without notifying/unsubscribing it, and `unsub()` clears *all* queued events. If multiple components subscribe (or a component re-subscribes), earlier subscribers will silently stop receiving events and queued events can be dropped unexpectedly.
This is especially easy to hit if each arena pane/child uses the bridge. Consider supporting multiple listeners (Set) or making `sub()` return an unsubscribe function scoped to that listener, without clearing global queue on unrelated unsubs.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
only one component subscribes at a time - this is intended behavior, unsub will run in cleanup function - sub in the useEffect of component mount
|
@rgtile |
![rgtile[bot]](https://avatars.githubusercontent.com/u/116030715?s=60&v=4){kind=small}
| export type TestResult = { | ||
| passed: boolean | ||
| name: string | ||
| name?: string | ||
| output?: string | ||
| time?: number | ||
| } |
There was a problem hiding this comment.
TestResult type diverges from protocol's RunResult
TestResult adds name?: string and makes output optional, but CodeData.testResults in arena-protocol.ts is typed as Record<string, RunResult[]> where RunResult has no name field. In code/arena.tsx the RunResult[] values are passed directly to CodePane as testResults, which TypeScript accepts only because TestResult is a looser supertype. When the real code arena is wired up, callers building TestResult objects will get no compile error if they omit output (expecting it to be required as in RunResult), or incorrectly populate name (absent from RunResult). Consider removing the local type and importing RunResult from ~/shared/arena-protocol instead.
| export type TestResult = { | |
| passed: boolean | |
| name: string | |
| name?: string | |
| output?: string | |
| time?: number | |
| } | |
| export type { RunResult as TestResult } from "~/shared/arena-protocol" |
Or simply replace TestResult references in this file with the imported RunResult.
| sub: (fn: (event: T) => void) => { | ||
| listener = fn | ||
| q.forEach(fn) | ||
| q.length = 0 | ||
| }, |
There was a problem hiding this comment.
sub silently replaces an existing listener
Calling sub a second time (e.g. component remount in React Strict Mode, or a bug where two consumers try to subscribe) silently discards the previous listener without any warning. The first subscriber stops receiving events with no error. Consider guarding or at minimum asserting:
| sub: (fn: (event: T) => void) => { | |
| listener = fn | |
| q.forEach(fn) | |
| q.length = 0 | |
| }, | |
| sub: (fn: (event: T) => void) => { | |
| if (listener) { | |
| console.warn("EventBridge: replacing existing listener. Only one subscriber is supported.") | |
| } | |
| listener = fn | |
| q.forEach(fn) | |
| q.length = 0 | |
| }, |
| const leave = useCallback(() => { | ||
| sendJsonMessage({ type: "leave" } satisfies ClientMsg<T>) | ||
| }, [sendJsonMessage]) |
There was a problem hiding this comment.
leave is not guarded by enabled or socketUrl
sendAction checks if (!enabled || !socketUrl) return before sending, but leave calls sendJsonMessage unconditionally. If leave() is called when the hook is disabled or the host env-var is absent, react-use-websocket will silently no-op, which is harmless — but the inconsistency could be surprising when debugging. For symmetry:
| const leave = useCallback(() => { | |
| sendJsonMessage({ type: "leave" } satisfies ClientMsg<T>) | |
| }, [sendJsonMessage]) | |
| const leave = useCallback(() => { | |
| if (!enabled || !socketUrl) return | |
| sendJsonMessage({ type: "leave" } satisfies ClientMsg<T>) | |
| }, [enabled, sendJsonMessage, socketUrl]) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactoring
Greptile Overview
Greptile Summary
This PR updates the arena client/server websocket protocol to use a typed
ClientMsgenvelope (including{type:"action"}), refactors the Durable Object websocket handler to switch onmsg.type, and restructures the arena route components (Active/Lobby/Ended) for clearer separation.On the UI side, draw/code/typing modes are moved onto a unified
ArenaComponentPropsshape and most mode UIs are intentionally converted to read-only/WIP stubs while keeping typed boundaries (arena data, actions, and events) intact viauseArenaSocketand a new event bridge.Confidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as ArenaActive participant Hook as useArenaSocket participant WS as WebSocket participant DO as ArenaWSS (Durable Object) UI->>Hook: useArenaSocket({arenaId,userId,username,config,enabled}) Hook->>WS: connect(socketUrl) if enabled && socketUrl!=null WS-->>Hook: onOpen Hook->>WS: send {type:"init", userId, username, config} WS->>DO: ClientMsg(init) DO-->>WS: ServerMsg(state) WS-->>Hook: onMessage(state) Hook-->>UI: participants/data/timeRemaining UI->>Hook: sendAction(action) Hook->>WS: send {type:"action", action} WS->>DO: ClientMsg(action) DO->>DO: handleAction(msg.action) DO-->>WS: ServerMsg(mechanic|tick|arena_over|error) WS-->>Hook: onMessage(...) Hook-->>UI: onArenaEvent / state updates UI->>Hook: leave() Hook->>WS: send {type:"leave"} WS->>DO: ClientMsg(leave) DO-->>WS: close(1000) WS-->>Hook: onClose