fix(openworlds): portrait gallery empty-state fallback (Closes #379) - #390
Closed
100yenadmin wants to merge 1 commit into
Closed
fix(openworlds): portrait gallery empty-state fallback (Closes #379)#390100yenadmin wants to merge 1 commit into
100yenadmin wants to merge 1 commit into
Conversation
Closes the Critical regression Loop-10 caught: PR #369 added 5 races to RACES (dwarf, halfling, gnome, dragonborn, half-orc) without adding matching PORTRAIT_GALLERY entries. The race-filter at screen-create.jsx:548 returns [] for those races, so the portrait grid renders 0 buttons — the wizard dead-ends for any player who picks one of those 5 lineages. Fix per #379 AC3 (empty-state fallback) — the curated 24+ per race path (AC1) is the better long-term answer but requires portrait curation work tracked separately as #378. What this lands --------------- When the lineage filter yields [], the gallery render block: 1. Surfaces an a11y-friendly banner (role="status", aria-live="polite") explaining the gap by lineage name + pointing at #378 / #379 as the tracking issues. So a player picking dwarf sees: "No canon portrait curated for Dwarf yet — pick any face below for now, or generate a unique one with 'A face of your own' further down. The full per-race catalogue lands as #378 finishes ingest (tracked: #379)." 2. Falls back to showing the FULL living gallery (livingAll = PORTRAIT_GALLERY.filter(p => p.alive !== false)) so the player can still pick a portrait, generate a unique face, or proceed to the next step. The wizard never dead-ends. 3. Preserves the original gallery-index → scope mapping (the existing PORTRAIT_GALLERY.indexOf(p) remap continues to work for both the filtered and fallback paths), so hero.portrait / portraitScope / bindHero spec all stay correct. Why this approach ----------------- - ZERO new state: no useState, no toggle, no controlled component. The fallback is purely derived from the filter result. - ZERO behavior change for the 7 races with portraits (the 6 pre-PR-#369 races + aasimar from PR #389): when lineageMatches has entries, usingFallback is false and toRender = lineageMatches exactly as before. - ZERO regression risk to selection state: a portrait the player picked while their race was "dwarf" (e.g. Astarion, idx 2) stays pickable + remains the chosen portrait if they change race to a lineage that doesn't include him; the engine takes the slug, not a race-match assertion. - Honest signposting: the banner names the tracking issues so the player (or anyone reading the markup) can see WHY the gallery is showing the full set instead of a curated lineage subset. Acceptance criteria (per #379) ------------------------------ - [x] AC3 (fallback): empty-state affordance renders when filter yields [] - [x] Banner names the lineage that has no portraits ("No canon portrait curated for <RaceName> yet") - [x] CTA paths exposed: "pick any face below" + "generate a unique one" (both already exist; the banner just signposts them) - [x] References #378 (the curated catalogue work) so the relationship between this fallback and the long-term fix is visible to the player - [x] a11y: role="status" + aria-live="polite" so screen readers announce the banner when the race change triggers the fallback Out of scope ------------ - AC1 (curated 24+ per race) — that's #378, requires content curation from the 2,077-portrait local pool, separate PR - BYO drag-drop PNG (#376) — separate PR - Subrace handling (#377) — separate PR Smoke test ---------- Manually verified the brace + paren balance of the file (558/558, 361/361). The change is purely a wrap-into-IIFE-with-conditional; no other code paths are affected. When the gate from PR #386 (gallery_per_race) sees this PR, the underlying PORTRAIT_GALLERY data is unchanged (still 0 entries for the 5 races) so the gate would still FAIL on those races — that's correct, because #386 measures data drift (the symptom that needs content curation in #378), while this PR closes the user-facing deadend (the symptom that breaks the wizard today). Both gates fire on the right shape of regression. Refs ---- - Closes #379 (Critical: portrait gallery empty for 5 of 11 races) - Pairs with PR #389 (#375 aasimar lineage) - Tracks toward #378 (curated 24+ per race — the proper fix) - Builds on Loop-9 + Loop-10 verification trail
📝 WalkthroughWalkthroughThe Face step of character creation now handles lineages with no curated living portraits by filtering the gallery by hero race, detecting empty results, falling back to the full living gallery with an explanatory banner, and preserving selection consistency by mapping rendered entries to original ChangesPortrait Gallery Filtering with Fallback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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)
viewer/openworlds/screen-create.jsx (1)
578-578: 💤 Low valueIndex mapping via
indexOfworks correctly.The
PORTRAIT_GALLERY.indexOf(p)approach correctly maps filtered portraits back to their original indices becausefilter()preserves object references. This ensures thatpickGallery(i)receives the correct index regardless of whether the fallback is active.Note: This relies on reference equality, so it would break if
PORTRAIT_GALLERYentries were ever cloned or reconstructed. For the current 12-portrait gallery this is fine, but worth keeping in mind if the gallery becomes dynamically loaded.Alternative approach (optional)
If portraits were ever loaded dynamically or cloned, consider adding an explicit index field to each portrait object:
const PORTRAIT_GALLERY = [ { slug: "aubree", name: "Aubree", race: "human", alive: true, index: 0 }, { slug: "shadowheart", name: "Shadowheart", race: "half", alive: true, index: 1 }, // ... ];Then use
p.indexinstead ofPORTRAIT_GALLERY.indexOf(p). However, this adds maintenance burden and isn't needed for the current static gallery.🤖 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 `@viewer/openworlds/screen-create.jsx` at line 578, When mapping filtered portraits back to their original index, guard against PORTRAIT_GALLERY.indexOf(p) returning -1; in the code around the variable i and the call to pickGallery, resolve the index defensively by first using PORTRAIT_GALLERY.indexOf(p) and if it equals -1, fallback to PORTRAIT_GALLERY.findIndex(x => x.slug === p.slug) or use p.index when portraits are augmented with an explicit index field; then pass the resolved index to pickGallery and optionally log a warning if resolution fails to aid debugging.
🤖 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 `@viewer/openworlds/screen-create.jsx`:
- Line 570: The style entry using color: "var(--ink-2)" is likely a typo; update
the color value in the banner/text style to use an existing design token (e.g.,
replace "var(--ink-2)" with a consistent token such as "var(--ink-700)" or the
token used elsewhere in this file) so it matches the pattern used by other color
variables; locate the color property in the style object where color:
"var(--ink-2)" appears and swap it for the correct --ink-* token (for example
--ink-700) to restore proper rendering.
---
Nitpick comments:
In `@viewer/openworlds/screen-create.jsx`:
- Line 578: When mapping filtered portraits back to their original index, guard
against PORTRAIT_GALLERY.indexOf(p) returning -1; in the code around the
variable i and the call to pickGallery, resolve the index defensively by first
using PORTRAIT_GALLERY.indexOf(p) and if it equals -1, fallback to
PORTRAIT_GALLERY.findIndex(x => x.slug === p.slug) or use p.index when portraits
are augmented with an explicit index field; then pass the resolved index to
pickGallery and optionally log a warning if resolution fails to aid debugging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d1b72cf-cd02-499f-b371-fd6a2663f244
📒 Files selected for processing (1)
viewer/openworlds/screen-create.jsx
| border: "1px dashed rgba(140,100,60,0.35)", | ||
| borderRadius: 6, | ||
| fontSize: 13, | ||
| color: "var(--ink-2)", |
There was a problem hiding this comment.
Likely typo in color variable.
The color value var(--ink-2) is inconsistent with the color variable pattern used elsewhere in this file (--ink-600, --ink-700, --ink-800, --ink-900). If --ink-2 is undefined, the banner text may not render with the intended color, degrading visibility and accessibility.
🎨 Proposed fix
- color: "var(--ink-2)",
+ color: "var(--ink-800)",📝 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.
Suggested change
| color: "var(--ink-2)", | |
| color: "var(--ink-800)", |
🤖 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 `@viewer/openworlds/screen-create.jsx` at line 570, The style entry using
color: "var(--ink-2)" is likely a typo; update the color value in the
banner/text style to use an existing design token (e.g., replace "var(--ink-2)"
with a consistent token such as "var(--ink-700)" or the token used elsewhere in
this file) so it matches the pattern used by other color variables; locate the
color property in the style object where color: "var(--ink-2)" appears and swap
it for the correct --ink-* token (for example --ink-700) to restore proper
rendering.
Member
Author
100yenadmin
pushed a commit
that referenced
this pull request
Jun 5, 2026
…, #315 AC5) The character creator offered no subrace choice. This adds subrace as an OPTIONAL second-tier lineage choice and threads it through the ability preview, the review/summary surfaces, the portrait filter, and the startProviderSession seam. Additive, render-only (viewer JSX, no build step). Validated by transforming screen-create.jsx through viewer/openworlds/vendor/babel-standalone-7.29.0.min.js. Changes (viewer/openworlds/screen-create.jsx): - RACES: optional `subraces` map on elf (high/wood), dwarf (mountain/hill), halfling (lightfoot/stout), gnome (forest/rock). Each subrace declares name/body and a `bonus` DELTA that stacks on the base race. Drow DECISION documented: drow stays a STANDALONE top-level race (its own RACES entry + Minthara gallery face), NOT folded into elf.subraces — matches the already-shipped data fork. (AC1) - Helpers: subracesForRace / hasSubraces / mergeBonus (additive, non-mutating) / effectiveRaceBonus (base + chosen subrace) / subraceLineageName. - StepRace: new SubracePicker renders brass/sketch radio chips, gated on the race having a non-empty subraces map; a synthesized "Standard" (subrace: null) option is first and is the default so base-race-only/homebrew heroes are never blocked. Switching race clears a stale subrace. (AC2) - Ability stacking: StepAbilities preview, StepReview summary, and the live right-rail summary all read effectiveRaceBonus(hero) — Mountain Dwarf shows STR +2 / CON +2. No surface left reading the bare base-race bonus. (AC3) - bindHero: spec.subrace serialized (empty string when none) across the startProviderSession seam. Engine-side wiring out of scope. (AC4) - Initial hero state gains `subrace: null`. - portraitChoicesForRace(race, subrace): ANDs in subrace when the gallery carries subrace-tagged faces, else degrades to the race-level pool (never an empty grid — the curated faces are race-only today). Preserves the existing #379/#390 race-level fallback. (#315 AC5) Test guard (viewer/tests/test_openworlds_static.py): - test_openworlds_create_subrace_handling_is_wired: parses RACES per-race, asserts the four subrace maps + bonus deltas, the drow-standalone decision, the SubracePicker/radiogroup/Standard-default UI, the single effectiveRaceBonus stacking path, the race-switch reset, the bindHero spec.subrace, and the subrace-aware portrait filter. Findings skipped: none — all four AC of #377 implemented.
100yenadmin
added a commit
that referenced
this pull request
Jun 5, 2026
…, #315 AC5) (#669) The character creator offered no subrace choice. This adds subrace as an OPTIONAL second-tier lineage choice and threads it through the ability preview, the review/summary surfaces, the portrait filter, and the startProviderSession seam. Additive, render-only (viewer JSX, no build step). Validated by transforming screen-create.jsx through viewer/openworlds/vendor/babel-standalone-7.29.0.min.js. Changes (viewer/openworlds/screen-create.jsx): - RACES: optional `subraces` map on elf (high/wood), dwarf (mountain/hill), halfling (lightfoot/stout), gnome (forest/rock). Each subrace declares name/body and a `bonus` DELTA that stacks on the base race. Drow DECISION documented: drow stays a STANDALONE top-level race (its own RACES entry + Minthara gallery face), NOT folded into elf.subraces — matches the already-shipped data fork. (AC1) - Helpers: subracesForRace / hasSubraces / mergeBonus (additive, non-mutating) / effectiveRaceBonus (base + chosen subrace) / subraceLineageName. - StepRace: new SubracePicker renders brass/sketch radio chips, gated on the race having a non-empty subraces map; a synthesized "Standard" (subrace: null) option is first and is the default so base-race-only/homebrew heroes are never blocked. Switching race clears a stale subrace. (AC2) - Ability stacking: StepAbilities preview, StepReview summary, and the live right-rail summary all read effectiveRaceBonus(hero) — Mountain Dwarf shows STR +2 / CON +2. No surface left reading the bare base-race bonus. (AC3) - bindHero: spec.subrace serialized (empty string when none) across the startProviderSession seam. Engine-side wiring out of scope. (AC4) - Initial hero state gains `subrace: null`. - portraitChoicesForRace(race, subrace): ANDs in subrace when the gallery carries subrace-tagged faces, else degrades to the race-level pool (never an empty grid — the curated faces are race-only today). Preserves the existing #379/#390 race-level fallback. (#315 AC5) Test guard (viewer/tests/test_openworlds_static.py): - test_openworlds_create_subrace_handling_is_wired: parses RACES per-race, asserts the four subrace maps + bonus deltas, the drow-standalone decision, the SubracePicker/radiogroup/Standard-default UI, the single effectiveRaceBonus stacking path, the race-switch reset, the bindHero spec.subrace, and the subrace-aware portrait filter. Findings skipped: none — all four AC of #377 implemented. Co-authored-by: Eva <arncalso@gmail.com>
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.
TL;DR
Closes the Critical regression Loop-10 caught: PR #369 added 5 races to
RACES(dwarf / halfling / gnome / dragonborn / half-orc) without adding matchingPORTRAIT_GALLERYentries. The race-filter atscreen-create.jsx:548returns[]for those races, so the portrait grid renders 0 buttons — the wizard dead-ends for any player who picks one of those 5 lineages.This PR delivers #379 AC3 (empty-state fallback). The curated 24+ per race path (AC1) is tracked separately as #378.
Closes
What this lands
When the lineage filter yields
[], the gallery render block:Surfaces an a11y-friendly banner (
role="status",aria-live="polite") explaining the gap by lineage name. A player picking dwarf sees:Falls back to showing the full living gallery (
livingAll = PORTRAIT_GALLERY.filter(p => p.alive !== false)) so the player can still pick a portrait, generate a unique face, or proceed. The wizard never dead-ends.Preserves the original gallery-index → scope mapping — the existing
PORTRAIT_GALLERY.indexOf(p)remap continues to work for both filtered and fallback paths, sohero.portrait/portraitScope/bindHerospec all stay correct.Why this approach
useState, no toggle. Fallback is purely derived from the filter result.lineageMatches.length > 0,usingFallback === falseandtoRender === lineageMatchesexactly as before. 6 of the 11 races (7 with PR fix(openworlds): add aasimar lineage to RACES (Closes #375) #389's aasimar) see the original render path unchanged.Acceptance criteria (per #379)
[]role="status"+aria-live="polite"so screen readers announce the banner on race changeOut of scope
Interaction with PR #386 (gallery_per_race gate)
The
gallery_per_racegate measuresPORTRAIT_GALLERYdata drift. After this PR:Both gates fire on the right shape of regression. The gate stays useful as the gate for #378's eventual completion.
Diff stat
viewer/openworlds/screen-create.jsx— 1 file, +46/-15 linesCollision audit
viewer/openworlds/screen-create.jsxlast touched onmainby PR fix(openworlds): create — #277 #281 #315 #369. The gallery render block (lines ~548-568 pre-PR) hasn't been touched since.fix(dm,viewer): cold-open delivers a real 2nd-person opening— DM/cold-open territory, zero overlap.fix/375-aasimar-lineageand touches a DIFFERENT block (RACES at line ~866). No conflict with this PR.pickGallery,genMode,hero.portrait,portraitScope,Img) continue to resolve as before.DO NOT MERGE yet
Per owner direction. Ready for review.
Summary by CodeRabbit
Release Notes