refactor(client): split Avatar.ts and drop write-only chat-bubble fields#7
Merged
Conversation
The avatar preview created one Pixi Application — and therefore one WebGL context — per grid cell (28 cells: 14 body + 14 bubble). Browsers cap live WebGL contexts at ~16, so the excess were context-lost: their shaders failed to compile (with an empty info log) and Pixi's error logger then threw on the null program log, blanking the page behind a runtime-error overlay. Use a single shared offscreen renderer and snapshot each variant into its own static 2D canvas via renderer.extract.canvas. One context total, so the harness now scales to any number of cells. Not a product bug: the real app uses a single Pixi context per room and renders full-body avatars correctly. This only affected the preview tool. Verified in Chromium (Playwright): all 28 avatar variants render with zero shader errors and no error overlay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split the 1557-line Avatar.ts (review finding 8.1). The ~700 lines of stateless pixel-art body rendering (drawAvatarBody + its draw helpers, the AVATAR_* palette constants, the color helpers, and the body draw types) move to a focused avatarBody.ts. Avatar.ts keeps movement, the chat-bubble lifecycle, and composition (1557 -> 835 lines; avatarBody.ts is 750). Pure code move — no logic changes. Avatar.ts imports the shared palette/color helpers it needs and re-exports drawAvatarBody and the body types so existing importers (AvatarPreview, the preview harness) are unchanged. Verified behavior-preserving: the avatar preview renders pixel-for-pixel identically before and after (ImageMagick AE diff = 0 across all 28 hair-style variants), plus typecheck, lint, and the client test suite pass. Note: the review also flagged the hair-style switch as "duplicated" between the body (drawHair) and the chat-bubble portrait (drawChatBubbleAvatar). They share the style enum but are two distinct renderers (full body vs. tiny face portrait) with entirely different draw calls, so there is no mechanical de-duplication; left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the public chatBubble/chatBubbleBackground/chatBubbleAvatar/chatBubbleText fields (review finding 8.7). They were write-only aliases to the latest entry of the chatBubbles array, maintained only so tests could read "the current bubble" — never read by the avatar itself. Keeping them in sync also required a throwaway ChatBubbleView allocation in the constructor and in syncLatestChatBubbleReferences whenever the queue drained (finding 5.3). Tests now read the latest bubble via chatBubbles.at(-1), so the fields, the syncLatestChatBubbleReferences method, and both throwaway allocations are gone. Verified behavior-preserving: avatar preview renders pixel-for-pixel identically (ImageMagick AE = 0 across all 28 variants); typecheck, lint, and the client tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
danjdewhurst
changed the title
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
Two behavior-preserving cleanups of the 1557-line
Avatar.ts(review findings 8.1, 8.7, 5.3):8.1): the ~700 lines of stateless pixel-art body rendering (drawAvatarBody+ draw helpers,AVATAR_*palette, color helpers, body types) move to a focusedavatarBody.ts.Avatar.tskeeps movement, the chat-bubble lifecycle, and composition; it imports the shared palette/color helpers and re-exportsdrawAvatarBody+ types so importers (AvatarPreview, preview harness) are unchanged.8.7/5.3): the publicchatBubble*fields were write-only mirrors ofchatBubbles.at(-1), kept in sync only for tests — and kept non-null via a throwawayChatBubbleViewallocation in the constructor andsyncLatestChatBubbleReferences. Removed the fields, the method, and both allocations; tests readchatBubbles.at(-1)instead.Avatar.ts: 1557 → 812 lines · newavatarBody.ts: 750 lines.Verified behavior-preserving
compare -metric AE= 0 differing pixels across all 28 hair-style variants (body + chat bubble).bun run typecheck✅ ·bun run lint✅ ·bun test(client) ✅ 80 pass.Scoped out
The review flagged the hair-style switch as "duplicated" between
drawHair(body) anddrawChatBubbleAvatar(bubble portrait). They share the style enum but are two distinct renderers (full body vs. a tiny face portrait) with entirely different draw calls — no mechanical de-duplication, so left as-is.🤖 Generated with Claude Code