feat: theme selector + accordion world map#12
Conversation
Turn 3 of the redesign brief: the flat 29-lesson map overflowed 80x24, so rework it as an accordion — cleared/locked acts collapse to one summary row (stars stay visible), only the current act expands, always fits 24 rows. j/k move, enter plays a lesson or expands a collapsed act, tab folds/unfolds. Add a theme selector built from Turn 2's directions: Classic (the original look), Synthwave, Charm, and Phosphor. A Theme lives in styles.go as a per-act palette plus base role colors; applyTheme reassigns the shared style vars in place, so every renderer re-skins for free. The choice persists in progress.json (new optional field, no version bump — old saves default to Classic) and survives a progress reset. Picker screen lives under a new title menu entry with live preview. Sync the two now-stale strings the brief flagged: the finale reads "The Archivist is catalogued" in Act IV brass (was "The Grid Core is broken"), and roman(4) returns "IV".
|
Warning Review limit reached
Next review available in: 21 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 (4)
📝 WalkthroughWalkthroughThis PR adds a theme system (Theme model, registry, applyTheme, theme picker screen, and persistence via a new Progress.Theme field) and replaces the flat world map with an accordion-style layout using openAct/mapSel state and a focusLesson helper. It also fixes finale text/palette, roman(4), and updates docs/README/tests accordingly. ChangesTheme selector and accordion map
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TitleScreen
participant ThemesScreen as updateThemes/viewThemes
participant StylesModule as applyTheme
participant Progress as progress.Save/Load
User->>TitleScreen: select "Theme"
TitleScreen->>ThemesScreen: switch to screenThemes, set themeIdx
User->>ThemesScreen: j/k (preview)
ThemesScreen->>StylesModule: applyTheme(previewName)
User->>ThemesScreen: enter (commit)
ThemesScreen->>StylesModule: applyTheme(selectedName)
ThemesScreen->>Progress: Save(prog with Theme)
ThemesScreen->>TitleScreen: switch to screenTitle
User->>ThemesScreen: esc (cancel, alternate path)
ThemesScreen->>StylesModule: applyTheme(savedTheme)
ThemesScreen->>TitleScreen: switch to screenTitle
sequenceDiagram
participant User
participant WorldMap as updateMap/viewMap
participant Rows as visibleRows
participant Focus as focusLesson/toggleAct
User->>WorldMap: j/k (move selection)
WorldMap->>Rows: recompute visible rows for openAct
User->>WorldMap: tab (fold/unfold act header)
WorldMap->>Focus: toggleAct(sel)
Focus->>Rows: update openAct, re-anchor mapSel
User->>WorldMap: enter (open lesson if unlocked)
WorldMap->>Focus: focusLesson(lessonIdx)
Focus->>Rows: set openAct + mapSel to lesson row
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 (1)
internal/ui/themes_test.go (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest relies on external cleanup discipline rather than pinning its own starting theme.
m.themeIdx = themeIndexByName(activeThemeName)assumes the globalactiveThemeNameis already"Classic", but nothing in this test enforces that — it depends on every other test in the package (e.g. instyles_test.go, which runs earlier alphabetically) properly restoring the theme afterward. If any test forgets to reset it, this test's assumptions (and its "j should live-preview a different theme" / persistence assertions) become order-dependent.Pin the starting state explicitly instead of relying on it:
🔧 Proposed fix
func TestThemePickerSelectPersistsAndCancelReverts(t *testing.T) { defer applyTheme("Classic") // don't leak a theme to other tests + applyTheme("Classic") // ensure a known starting theme regardless of run order m := newTestModel(t) m.themeIdx = themeIndexByName(activeThemeName) // start on the active (Classic) m.scr = screenThemes🤖 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/themes_test.go` around lines 9 - 14, TestThemePickerSelectPersistsAndCancelReverts depends on the shared activeThemeName being “Classic” instead of setting its own starting state. Update this test to explicitly pin the theme before creating/configuring the model (using applyTheme and then m.themeIdx/themeIndexByName as needed) so the assertions in TestThemePickerSelectPersistsAndCancelReverts do not rely on cleanup from styles_test or any other test. Keep the existing defer cleanup, but make the initial theme setup self-contained in the test.
🤖 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 `@internal/progress/progress_test.go`:
- Line 35: The test setup in progress_test uses os.WriteFile without checking
its error, so a failed write will surface later as a confusing failure. Update
the setup around the legacy file creation in the test to handle the return value
from os.WriteFile explicitly and fail the test immediately with a clear message
if it errors, keeping the check close to the legacy and progress file
initialization.
---
Nitpick comments:
In `@internal/ui/themes_test.go`:
- Around line 9-14: TestThemePickerSelectPersistsAndCancelReverts depends on the
shared activeThemeName being “Classic” instead of setting its own starting
state. Update this test to explicitly pin the theme before creating/configuring
the model (using applyTheme and then m.themeIdx/themeIndexByName as needed) so
the assertions in TestThemePickerSelectPersistsAndCancelReverts do not rely on
cleanup from styles_test or any other test. Keep the existing defer cleanup, but
make the initial theme setup self-contained in the test.
🪄 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: c720421c-5668-4617-b8ac-ded2b2b4b0c5
📒 Files selected for processing (16)
README.mddocs/ARCHITECTURE.mddocs/superpowers/specs/2026-07-04-theme-selector-and-accordion-map-design.mdinternal/progress/progress.gointernal/progress/progress_test.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/results.gointernal/ui/room.gointernal/ui/styles.gointernal/ui/styles_test.gointernal/ui/themes.gointernal/ui/themes_test.gointernal/ui/title.gointernal/ui/worldmap.gointernal/ui/worldmap_test.go
| // A v2 save written before the Theme field existed must still load, with | ||
| // Theme defaulting to "" (no version bump, no wipe). | ||
| legacy := filepath.Join(dir, "legacy.json") | ||
| os.WriteFile(legacy, []byte(`{"version":2,"xp":10,"level":1,"stars":{}}`), 0o644) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unchecked error from os.WriteFile.
golangci-lint (errcheck) flags this. If the write fails, the test proceeds against a missing/incomplete file and fails with a confusing downstream error instead of a clear message.
🛠️ Proposed fix
- os.WriteFile(legacy, []byte(`{"version":2,"xp":10,"level":1,"stars":{}}`), 0o644)
+ if err := os.WriteFile(legacy, []byte(`{"version":2,"xp":10,"level":1,"stars":{}}`), 0o644); err != nil {
+ t.Fatalf("WriteFile: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.WriteFile(legacy, []byte(`{"version":2,"xp":10,"level":1,"stars":{}}`), 0o644) | |
| if err := os.WriteFile(legacy, []byte(`{"version":2,"xp":10,"level":1,"stars":{}}`), 0o644); err != nil { | |
| t.Fatalf("WriteFile: %v", err) | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 35-35: Error return value of os.WriteFile is not checked
(errcheck)
🤖 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/progress/progress_test.go` at line 35, The test setup in
progress_test uses os.WriteFile without checking its error, so a failed write
will surface later as a confusing failure. Update the setup around the legacy
file creation in the test to handle the return value from os.WriteFile
explicitly and fail the test immediately with a clear message if it errors,
keeping the check close to the legacy and progress file initialization.
Source: Linters/SAST tools
The theme was already applied on selection (verified: the live-committed title renders byte-identically to a restart with the same theme), but the title logo was colored by the current act only — and act I is a shade of green in every theme — so switching themes looked like nothing happened. Paint the logo top-to-bottom across all four act colors so every theme renders a distinct title, and apply the theme explicitly on commit rather than relying on the j/k preview side-effect. Adds a regression test asserting the four themes produce distinct title renders.
1 participant
Implements Turn 3 of the
nvim-quest UI.dc.htmlredesign brief plus an in-game theme picker built from the brief's Turn 2 style directions.What changed
1. Accordion world map (
worldmap.go)The flat 29-lesson map overflowed 80×24. It's now an accordion:
✓ ACT I · … ★ 24/24 · cleared,· ACT IV · … locked · clear act iii) — stars stay visible so replay value shows.★★★/☆☆☆).j/kmove ·enterplays a lesson or expands a collapsed act ·tabfolds/unfolds ·esctitle.State:
mapIdx(a lesson index) is replaced byopenAct+mapSel(selection over visible rows); afocusLessonhelper positions the cursor for callers arriving from the title / results / room screens.2. Theme selector
Classic (the original look) · Synthwave · Charm · Phosphor, from a new Theme entry in the title menu with live preview. A
Themeis a per-act palette plus base role colors;applyThemereassigns the shared style vars in place, so every renderer re-skins with no call-site changes. The choice persists inprogress.json(new optional field — no version bump, so old saves load fine and default to Classic) and is preserved across a progress reset.3. Content sync
roman(4)→"IV"so an Act IV boss-clear reads "ACT IV COMPLETE".Scope / ceiling
Themes re-skin colors only (reads clearly as synthwave-neon / charm-pastel / phosphor-green). They do not rebuild each theme's structural chrome (dither banners, pill HUD, CRT reverse-video headers); Phosphor is approximated as a per-act color ramp. Full structural fidelity is a separate, larger job.
Tests
styles_test.go— theme switching + Classic fallback for unknown/empty names.worldmap_test.go— fits-24 for every act, one-act-expanded invariant, expand-collapses-previous, summary-row status text.themes_test.go— picker select persists + esc reverts the preview.progress_test.go— v2 backward compat + theme round-trip.go vet+gofmtclean.Spec:
docs/superpowers/specs/2026-07-04-theme-selector-and-accordion-map-design.mdSummary by CodeRabbit
New Features
Bug Fixes
Tab, plus smoother navigation and selection behavior.