feat: full-fidelity theme skins (Phosphor / Synthwave / Charm)#13
Conversation
Introduce a Skin interface that owns the structural chrome — Logo, RoomHeader, CursorCell, FrameBuffer, HUD — and route viewTitle/viewRoom through the active skin. Renderers now assemble semantic content (roomHeader, buffer lines, HUD segments) and let the skin draw it. classicSkin reproduces the current look byte-for-byte (verified via a golden render diff across title, room, room+hint, and boss+visual screens), and every theme still falls back to it — so this commit changes nothing on screen. It's the seam the phosphor/synthwave/charm skins plug into next.
phosphorSkin renders the Turn 2c mockup: one hue per act with five brightness steps, hierarchy carried by luminance + reverse-video chips rather than a second color. Reverse-chip act banner, block-figlet logo + SYS.READY bar, bright "> " objective line, line-number gutter buffer, bg-block cursor, chip HUD, and for bosses a "!! BOSS !!" chip + T-MINUS bar. Phosphor's palette dim shifts to green so the color-only screens (map, stats) stay in-hue. Registered via skinRegistry; Classic and the other themes are untouched.
synthwaveSkin renders the Turn 2a mockup: a cyan→magenta gradient block logo, ▓▒░ dither act banners, a rounded buffer box with purple line numbers, an act-color block cursor, ▸▸ objective chevrons, neon background-chip HUD, and a BOSS » chip + TIME bar. Adds a shared roundedBox helper (reused by charm next).
charmSkin renders the Turn 2b mockup: a ✧ nvim─quest ✧ wordmark, lowercase
"act i · the cursor dojo" banners, a ♥ objective line, rounded act-colored
buffer borders, spaced pastel pills for the HUD, a pink block cursor, and
encouraging copy ("(rude!)", "you've got this"). Reuses the shared roundedBox.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a ChangesTheme skin system
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TitleView as viewTitle
participant RoomView as viewRoom
participant SkinRegistry as activeSkin()
participant Skin as Active Skin
User->>TitleView: select theme (Classic/Synthwave/Charm/Phosphor)
TitleView->>SkinRegistry: activeSkin()
SkinRegistry-->>TitleView: registered Skin or classicSkin fallback
TitleView->>Skin: Logo(logoLines)
Skin-->>TitleView: styled wordmark
User->>RoomView: enter lesson/room
RoomView->>SkinRegistry: activeSkin()
SkinRegistry-->>RoomView: Skin
RoomView->>Skin: RoomHeader(roomHeader)
RoomView->>Skin: Body(act, text)
RoomView->>Skin: FrameBuffer(act, bufferLines)
RoomView->>Skin: HUD(act, segments)
Skin-->>RoomView: rendered chrome strings
RoomView-->>User: final rendered room view
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
internal/ui/room.go (1)
243-258: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSkin lookup called repeatedly per render.
activeSkin()is invoked separately forRoomHeader,Body, andFrameBuffer(and again insiderenderHUD). Caching it once (skin := activeSkin()) would avoid the repeated map lookups and slightly clean up the call sites, though the current cost is negligible given a tiny registry map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/room.go` around lines 243 - 258, The room rendering path is calling activeSkin() multiple times per render, causing repeated lookup work and noisier call sites. In the room.go rendering flow around roomHeader, cache the result once (for example in the render method that builds the room output) and reuse that same skin value for RoomHeader, Body, FrameBuffer, and the renderHUD call instead of invoking activeSkin() repeatedly.internal/ui/skin.go (3)
126-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
classicSkin.FrameBuffermutates the input slice in place.Unlike
phosphorSkin.FrameBuffer(which allocates a newoutslice), this mutateslinesdirectly. Currently harmless since callers (bufferLines()) don't reuse the slice afterward, but it's a fragile contract for a shared interface method — a future caller that retains/reuses the buffer slice would see corrupted data.♻️ Proposed fix
func (classicSkin) FrameBuffer(_ int, lines []string) string { - for i, ln := range lines { - lines[i] = " " + ln - } - return strings.Join(lines, "\n") + out := make([]string, len(lines)) + for i, ln := range lines { + out[i] = " " + ln + } + return strings.Join(out, "\n") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/skin.go` around lines 126 - 131, classicSkin.FrameBuffer currently mutates the incoming lines slice in place, which makes this shared interface method fragile compared with phosphorSkin.FrameBuffer. Update FrameBuffer to build and return a new slice with the prefixed lines instead of rewriting lines directly, so callers of bufferLines() or any future reuse of the input slice won’t see unexpected modifications.
58-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
roundedBoxcan misalign the frame when content exceedsinnerW.
padis clamped to 0 when content is wider thaninnerW-1, but the content itself is never truncated, so the right border (bs.Right) will be pushed past its column for that row, breaking the box shape. Since buffer lines can be arbitrary code text, a long line will likely trigger this whenever the terminal/box width is narrower than the content.Consider truncating (with an ellipsis) or wrapping content that exceeds
innerW-1before computingpad.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/skin.go` around lines 58 - 80, The roundedBox helper is letting overlong buffer content push the right border out of alignment because it only clamps padding and never limits the rendered line width. Update roundedBox to handle lines wider than innerW-1 by truncating with an ellipsis or wrapping before computing pad, and keep the numbering path in sync so border placement stays stable for synthwave/charm rendering.
105-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
fmt.Fprintfinstead ofWriteString(fmt.Sprintf(...)).Static analysis flags this pattern at Line 112.
♻️ Proposed fix
if h.isBoss { b.WriteString(dangerStyle.Render("⚔ BOSS: "+h.title) + "\n") b.WriteString(dimStyle.Render(h.taunt) + "\n") - b.WriteString(fmt.Sprintf("%s %d:%02d · step %d/%d\n\n", - timerBar(h.timeLeft, h.timeTotal, 30), - h.timeLeft/60, h.timeLeft%60, h.step, h.steps)) + fmt.Fprintf(&b, "%s %d:%02d · step %d/%d\n\n", + timerBar(h.timeLeft, h.timeTotal, 30), + h.timeLeft/60, h.timeLeft%60, h.step, h.steps)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/skin.go` around lines 105 - 120, The RoomHeader method in classicSkin is building strings with WriteString(fmt.Sprintf(...)), which static analysis flags as unnecessary formatting allocation. Update RoomHeader to write formatted content directly with fmt.Fprintf on the strings.Builder for each interpolated line, keeping the existing output unchanged for both the boss and non-boss branches.Source: Linters/SAST tools
internal/ui/synthwave.go (1)
87-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHUD doesn't differentiate roles beyond
segMode.
segRoledefinessegRec,segPending,segHearts,segCombo, andsegKeys(perinternal/ui/skin.go), butsynthwaveSkin.HUDonly special-casessegMode; every other role renders with the identicalswBoxBg/pal.Accentchip. This loses semantic distinction (e.g. recording vs. combo vs. hearts) that the other skins likely convey visually.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/synthwave.go` around lines 87 - 101, The HUD rendering in synthwaveSkin.HUD only distinguishes segMode, so all other segment roles are drawn identically and lose their semantic meaning. Update the switch on s.role in HUD to handle the additional segRole values from internal/ui/skin.go—segRec, segPending, segHearts, segCombo, and segKeys—by choosing distinct chip colors/styling for each, while keeping the existing fallback for unknown roles.internal/ui/charm.go (2)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHUD mode label parsed from decorated display text.
segModehandling strips"-- "/" --"markers offs.textto recover the raw mode name. This silently degrades to the unstripped (and still-uppercase) string if the upstream format ever changes, coupling this skin's rendering to another layer's string formatting convention instead of a raw value carried onsegment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/charm.go` around lines 73 - 75, The segMode branch in charm.go is parsing the HUD label from decorated display text, which tightly couples rendering to the upstream string format. Update the segment handling to use the raw mode value carried on segment instead of stripping "-- " and " --" from s.text, and keep charmPill fed with that canonical value from segMode so the label stays correct even if the display decoration changes.
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuffer/header width
72duplicated as a magic literal.Consider hoisting to a package-level const (e.g.,
charmBoxWidth = 72) to keep the two call sites in sync if the layout width ever changes.Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/charm.go` at line 40, The fixed width value used in the Charm header/buffer rendering is duplicated as a magic literal, so hoist it into a package-level const and use that shared symbol in the relevant write/spacing call sites in Charm rendering logic. Update the `spread(...)` usage in the header builder and the other width-based call site to reference the same constant so layout changes stay in sync.internal/ui/charm_test.go (1)
8-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for boss-room chrome.
Only the non-boss
RoomHeaderbranch is exercised. The boss branch (taunt line,charmTimeBar, "boss: " pill) is untested here, and it's exactly the path with the negative-filledpanic risk flagged incharm.go. Consider adding a boss-lesson case asserting on"boss: ","(rude!)", and the time string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/charm_test.go` around lines 8 - 27, Add test coverage in TestCharmSkinChrome for the boss-room path in Charm skin, since only the regular RoomHeader branch is currently verified. Exercise a boss lesson through newTestModel/openLesson and assert the boss-specific chrome rendered by activeSkin()/charmSkin, including the "boss: " pill, the taunt line like "(rude!)", and the charmTimeBar time string. Keep the existing non-boss assertions, but extend the test so the boss branch in charmSkin and its header rendering are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 84-87: Clarify the README wording around the theme picker so it
does not imply theme selection is untouched; update the copy in README.md near
the theme description to say only gameplay progress is untouched, while the
selected theme preference is still persisted. Keep the existing theme-related
description but narrow “your progress is untouched” to gameplay progress
(XP/stars/unlocks) so it matches the behavior exercised by themes_test.go and
the theme selection flow.
---
Nitpick comments:
In `@internal/ui/charm_test.go`:
- Around line 8-27: Add test coverage in TestCharmSkinChrome for the boss-room
path in Charm skin, since only the regular RoomHeader branch is currently
verified. Exercise a boss lesson through newTestModel/openLesson and assert the
boss-specific chrome rendered by activeSkin()/charmSkin, including the "boss: "
pill, the taunt line like "(rude!)", and the charmTimeBar time string. Keep the
existing non-boss assertions, but extend the test so the boss branch in
charmSkin and its header rendering are covered.
In `@internal/ui/charm.go`:
- Around line 73-75: The segMode branch in charm.go is parsing the HUD label
from decorated display text, which tightly couples rendering to the upstream
string format. Update the segment handling to use the raw mode value carried on
segment instead of stripping "-- " and " --" from s.text, and keep charmPill fed
with that canonical value from segMode so the label stays correct even if the
display decoration changes.
- Line 40: The fixed width value used in the Charm header/buffer rendering is
duplicated as a magic literal, so hoist it into a package-level const and use
that shared symbol in the relevant write/spacing call sites in Charm rendering
logic. Update the `spread(...)` usage in the header builder and the other
width-based call site to reference the same constant so layout changes stay in
sync.
In `@internal/ui/room.go`:
- Around line 243-258: The room rendering path is calling activeSkin() multiple
times per render, causing repeated lookup work and noisier call sites. In the
room.go rendering flow around roomHeader, cache the result once (for example in
the render method that builds the room output) and reuse that same skin value
for RoomHeader, Body, FrameBuffer, and the renderHUD call instead of invoking
activeSkin() repeatedly.
In `@internal/ui/skin.go`:
- Around line 126-131: classicSkin.FrameBuffer currently mutates the incoming
lines slice in place, which makes this shared interface method fragile compared
with phosphorSkin.FrameBuffer. Update FrameBuffer to build and return a new
slice with the prefixed lines instead of rewriting lines directly, so callers of
bufferLines() or any future reuse of the input slice won’t see unexpected
modifications.
- Around line 58-80: The roundedBox helper is letting overlong buffer content
push the right border out of alignment because it only clamps padding and never
limits the rendered line width. Update roundedBox to handle lines wider than
innerW-1 by truncating with an ellipsis or wrapping before computing pad, and
keep the numbering path in sync so border placement stays stable for
synthwave/charm rendering.
- Around line 105-120: The RoomHeader method in classicSkin is building strings
with WriteString(fmt.Sprintf(...)), which static analysis flags as unnecessary
formatting allocation. Update RoomHeader to write formatted content directly
with fmt.Fprintf on the strings.Builder for each interpolated line, keeping the
existing output unchanged for both the boss and non-boss branches.
In `@internal/ui/synthwave.go`:
- Around line 87-101: The HUD rendering in synthwaveSkin.HUD only distinguishes
segMode, so all other segment roles are drawn identically and lose their
semantic meaning. Update the switch on s.role in HUD to handle the additional
segRole values from internal/ui/skin.go—segRec, segPending, segHearts, segCombo,
and segKeys—by choosing distinct chip colors/styling for each, while keeping the
existing fallback for unknown roles.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0155d665-e259-4717-97cd-929ef552814e
📒 Files selected for processing (13)
README.mddocs/ARCHITECTURE.mddocs/superpowers/specs/2026-07-04-theme-skins-full-fidelity-design.mdinternal/ui/charm.gointernal/ui/charm_test.gointernal/ui/phosphor.gointernal/ui/phosphor_test.gointernal/ui/room.gointernal/ui/skin.gointernal/ui/styles.gointernal/ui/synthwave.gointernal/ui/synthwave_test.gointernal/ui/title.go
- roundedBox now grows to fit its widest line (capped at 76) so a long buffer line can never push the right border out of column (was clamped-pad only). - Extract the shared roomInnerW=72 width const (was a magic literal in three spots across synthwave/charm). - README: clarify only gameplay progress is untouched — the theme choice is saved. - Add charm boss-room chrome coverage. Skipped (with reason): WriteString→Fprintf style nits (matches the codebase's existing pattern), per-render activeSkin() caching (a trivial map lookup), and synthwave's single non-mode HUD role (intentional — one chip style for stats).
1 participant
Follow-up to #12. Turns the color-only themes into full mockup fidelity — the Turn 2 designs differ in structure, not just color, so the renderers became theme-driven via a
Skinseam.Architecture — the
Skininterface (skin.go)Renderers assemble semantic content (
roomHeader, buffer lines, HUDsegments) and hand it toactiveSkin(), which owns the layout. Themes without a registered skin fall back toclassicSkinand re-skin by palette only.Skins restyle the title logo and the room (header, buffer, HUD, cursor, objective) — the three screens the mockups actually themed. Map / welcome / stats / results stay structurally neutral (color-only).
Skins
>objective, line-number gutter,!! BOSS !!+ T-MINUS bar.▓▒░dither banners, rounded box + purple line numbers, act-color block cursor, neon chip HUD,BOSS »chip + TIME bar.✧ nvim─quest ✧wordmark, lowercaseact i · …banners,♥objective, rounded act-colored borders, spaced pastel pills, pink cursor, encouraging copy.Phasing (one commit each)
Skinrefactor +classicSkin(golden-diff proves zero visual change)Tests
Per-skin chrome tests assert the distinguishing glyphs (
│gutter,▓▒░, rounded╭, lowercase banner) appear under that theme and not under Classic;TestTitleReskinsAcrossThemeskeeps the four titles distinct. Full suite green,go vet+gofmtclean.Spec:
docs/superpowers/specs/2026-07-04-theme-skins-full-fidelity-design.mdSummary by CodeRabbit
New Features
Bug Fixes