Overworld: UI refactor (HUD, Menu, EventLog, CSS) and add CI typecheck job#1389
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
📝 PR Kommentar-ZusammenfassungWird automatisch aktualisiert, sobald sich Kommentare ändern. |
Lint Fix PreviewTarget roots:
No changes would be made by running either formatting or lint auto-fixes. Duplicate codeNo significant duplicates found (per jscpd thresholds). |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Overworld scene, focusing on UI polish, CSS variable integration, and improved accessibility for the HUD and Menu components. The EventLog was updated to dynamically manage entries, and the OverworldMenu now includes a focus trap and keyboard navigation support. Feedback focuses on ensuring React state updaters remain pure in the EventLog, correcting prop types in the OverworldMenu to allow for undefined values, and refining focus management when switching between menu categories to enhance keyboard accessibility.
| useEffect(() => { | ||
| setEntries(prev => { | ||
| const next = [...prev] | ||
| const previous = previousRef.current | ||
|
|
||
| useEffect(()=>{ | ||
| if(bodyRef.current) bodyRef.current.scrollTop = bodyRef.current.scrollHeight; | ||
| }, [day, locationName]); | ||
| if (!previous || previous.day !== day) { | ||
| next.push({ | ||
| id: ++entryIdRef.current, | ||
| day, | ||
| type: 'system', | ||
| msg: t('ui:overworld.tour_active', { | ||
| date: `Day ${day}`, | ||
| defaultValue: `Day ${day}: Tour active.` | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| return ( | ||
| <div className="event-log absolute bottom-8 left-8 z-20 pointer-events-none"> | ||
| <div className="el-header"> | ||
| <span className="el-title">// {t('ui:overworld.event_log', { defaultValue: 'EVENT LOG' })}</span> | ||
| <span className="el-count">{entries.length} {t('ui:overworld.event_log_entries', { defaultValue: 'entries' })}</span> | ||
| </div> | ||
| <div className="el-body" ref={bodyRef}> | ||
| {entries.map((e,i)=>( | ||
| <div className="el-entry" key={i}> | ||
| <span className="el-day">[{String(e.day).padStart(2,'0')}]</span> | ||
| <span className={`el-msg t-${e.type}`}>> {e.msg}</span> | ||
| if (!previous || previous.locationName !== locationName) { | ||
| next.push({ | ||
| id: ++entryIdRef.current, | ||
| day, | ||
| type: 'travel', | ||
| msg: t('ui:overworld.location_secured', { | ||
| location: locationName, | ||
| defaultValue: `${locationName} secured.` | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| previousRef.current = { day, locationName } | ||
| return next.slice(-20) | ||
| }) | ||
| }, [day, locationName, t]) |
There was a problem hiding this comment.
The setEntries updater function contains side effects, specifically updating previousRef.current and incrementing entryIdRef.current. React state updater functions should be pure. If React re-invokes the updater (e.g., in Strict Mode or due to concurrent rendering), the ref will be updated prematurely, potentially leading to missing log entries or inconsistent IDs. It is safer to calculate the new entries outside of the state update and then perform a simple update.
useEffect(() => {
const previous = previousRef.current
const added = []
if (!previous || previous.day !== day) {
added.push({
id: ++entryIdRef.current,
day,
type: 'system',
msg: t('ui:overworld.tour_active', {
date: `Day ${day}`,
defaultValue: `Day ${day}: Tour active.`
})
})
}
if (!previous || previous.locationName !== locationName) {
added.push({
id: ++entryIdRef.current,
day,
type: 'travel',
msg: t('ui:overworld.location_secured', {
location: locationName,
defaultValue: `${locationName} secured.`
})
})
}
if (added.length > 0) {
setEntries(prev => [...prev, ...added].slice(-20))
}
previousRef.current = { day, locationName }
}, [day, locationName, t])
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 221e45188c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GAME_PHASES.SETTINGS, | ||
| GAME_PHASES.CREDITS, | ||
| GAME_PHASES.GAMEOVER, | ||
| GAME_PHASES.OVERWORLD, |
There was a problem hiding this comment.
Adding GAME_PHASES.OVERWORLD to SCENES_WITHOUT_HUD stops rendering HUD in that scene, but HUD is currently the only place that registers the global keyboard handlers (?/h for help, m for mute, Escape to close help). OverworldHUD does not add replacement key listeners, so in Overworld those shortcuts silently stop working for keyboard users even though the shortcuts panel still advertises them.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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 (1)
src/App.tsx (1)
103-104: 🧹 Nitpick | 🔵 TrivialUpdate stale comment listing HUD-hidden scenes.
The inline comment still reads
Hide HUD in Intro/Menu/Settings/Credits/GameOver, butSCENES_WITHOUT_HUDnow also coversOVERWORLD,TRAVEL_MINIGAME,PRE_GIG_MINIGAME, andCLINIC. Consider updating it (or replacing it with a reference toSCENES_WITHOUT_HUD) so it stays in sync with the actual set.📝 Suggested edit
- {/* Hide HUD in Intro/Menu/Settings/Credits/GameOver */} + {/* Hide HUD on scenes listed in SCENES_WITHOUT_HUD (intro/menu/settings/credits/gameover/overworld/minigames/clinic) */} {!SCENES_WITHOUT_HUD.has(currentScene) && <HUD />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 103 - 104, The inline comment above the HUD conditional is stale; update it to reflect the actual set or reference the constant instead of hardcoding scene names. Locate the conditional that uses SCENES_WITHOUT_HUD (the line with {!SCENES_WITHOUT_HUD.has(currentScene) && <HUD />}) and either replace the comment text with a short note like "Hide HUD for scenes in SCENES_WITHOUT_HUD" or remove the explicit scene list so the comment always stays in sync with the SCENES_WITHOUT_HUD constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 157-181: The new GitHub Actions job named "typecheck" is correct
and follows the same setup pattern as "ts-nocheck-guard"; no functional changes
are required—ensure the job retains the steps (actions/checkout@v6,
pnpm/action-setup@v6, actions/setup-node@v6, pnpm install --frozen-lockfile
--ignore-scripts) and the final command "pnpm run typecheck" so TypeScript
failures surface as non-zero exits; optionally, consider refactoring the
repeated setup steps into a composite action under .github/actions/ to DRY up
the repeated boilerplate across jobs.
In `@src/overworld.css`:
- Line 136: The .menu-panel rule still hardcodes two rgb(...) values in its
box-shadow; update the box-shadow to use the project's CSS color tokens instead
(e.g. replace rgb(0 255 65 / 18%) with rgb(var(--color-toxic-green-rgb)/18%) and
replace rgb(0 0 0 / 60%) with rgb(var(--color-void-black-rgb)/60%)), keeping the
same shadow offsets and opacities so only the color sources change; modify the
box-shadow declaration inside .menu-panel accordingly and ensure variable names
match the project's RGB-triplet tokens (e.g., --color-toxic-green-rgb,
--color-void-black-rgb).
- Around line 4-7: The .crt, .scan, and .noise rules still use hardcoded rgba()
colors; update those to use the project's color tokens instead: replace
rgba(0,0,0,.13) with rgb(var(--color-void-black-rgb)/0.13) in the .crt
background, replace rgba(0,255,65,.15) with
rgb(var(--color-toxic-green-rgb)/0.15) in the .scan gradient, and if the
embedded SVG data URI contains any hardcoded colors change them to reference the
same CSS token (or adjust the SVG to use currentColor and set color via
var(--color-...)-rgb) so all three selectors (.crt, .scan, .noise) use var-based
colors rather than raw rgba().
In `@src/ui/overworld/EventLog.tsx`:
- Around line 70-78: The literal "//" inside the el-title span is being rendered
(JSX doesn't treat // as a comment) in the EventLog component; either remove the
stray comment or make it explicit text if it was meant as decoration. Locate the
JSX in EventLog (the <span className='el-title'> element) and either delete the
leading // or convert it into a string expression (e.g., include it inside the
span's text content alongside the translated title) so the linter noCommentText
is resolved.
- Around line 29-66: The setEntries updater in the useEffect is impure because
it mutates entryIdRef.current and previousRef.current inside the reducer; move
those side effects out of the reducer so the updater remains pure: inside the
effect compute whether to add the "system" and/or "travel" entry using local
booleans (based on previousRef.current snapshot), build the new entries array
purely in a call to setEntries(prev => { ... return next.slice(-20) }), and
after calling setEntries update entryIdRef.current (increment by the number of
entries actually appended) and assign previousRef.current = { day, locationName
}; keep references to the unique symbols entryIdRef, previousRef, setEntries,
and the useEffect that depends on day/locationName to locate where to change.
In `@src/ui/overworld/OverworldHUD.tsx`:
- Around line 200-208: The shortcuts panel in OverworldHUD uses a <div> with
role='region' but should be a semantic landmark; change the element rendered
when showSC is true from a <div> to a <section>, remove the explicit role
attribute, keep the existing id (shortcutsPanelId), aria-label, and className
('shortcuts-panel pointer-events-auto'), and update the matching closing tag
accordingly so the component (OverworldHUD) renders a proper <section> landmark
for accessibility.
- Around line 214-244: The IIFE that builds the shortcut tuples inside
OverworldHUD's render (the "((): [string, string][] => [...])()") causes a new
array allocation on every render and is only used to satisfy typing; move the
shortcut table out of render by creating a stable value—either a module-level
constant of i18n keys and labels or a useMemo hook inside the OverworldHUD
component (e.g., shortcuts = useMemo(() => [...], [t])—so the array is not
reallocated each render and the translation function t can be listed cleanly in
deps; update the JSX to map over this new stable "shortcuts" identifier instead
of the IIFE.
- Around line 40-44: The cleanup in the effect should not overwrite the
currently rendered value; remove the unconditional assignment prev.current = to
from the teardown and only cancel the in-flight frame and mark it cancelled
(i.e., keep cancelled = true and cancelAnimationFrame(raf)), since tick already
updates prev.current = next each frame; update the effect cleanup around
raf/tick to stop the animation without forcing prev.current back to the target.
In `@src/ui/overworld/OverworldMenu.tsx`:
- Around line 274-294: The effect restores focus unconditionally on close which
can hijack user-changed focus; add a boolean ref (e.g., didFocusMenuRef) and set
it true when you actually move focus into the menu (inside the
requestAnimationFrame after focusing firstMenuButton), and only restore
previousFocusedElementRef.current when didFocusMenuRef.current is true; also
reset didFocusMenuRef.current to false when you cancel the rAF or when closing
so future opens behave correctly — update the useEffect logic that references
isMenuOpen, previousFocusedElementRef, menuRootRef, and firstMenuButton
accordingly.
- Around line 10-11: The prop types for vanFuel and vanCondition in
OverworldMenu.tsx are declared as number but callers pass number | undefined;
update the component's props to reflect this by changing vanFuel and
vanCondition to number | undefined (or mark them optional as vanFuel?: number,
vanCondition?: number) and ensure any internal uses (e.g., the isDisabled logic
inside OverworldMenu) continue to guard against undefined values rather than
assuming a definite number.
- Around line 421-451: The key for each menu item uses the localized item.label
which can collide across translations; change the GlitchButton key to use the
unique action identifier (item.action) instead. Update the key prop on the
GlitchButton inside the cat.items.map so it uses item.action (the MenuAction
union value) rather than item.label, ensuring keys align with the actions Record
and prevent React reconciliation warnings.
---
Outside diff comments:
In `@src/App.tsx`:
- Around line 103-104: The inline comment above the HUD conditional is stale;
update it to reflect the actual set or reference the constant instead of
hardcoding scene names. Locate the conditional that uses SCENES_WITHOUT_HUD (the
line with {!SCENES_WITHOUT_HUD.has(currentScene) && <HUD />}) and either replace
the comment text with a short note like "Hide HUD for scenes in
SCENES_WITHOUT_HUD" or remove the explicit scene list so the comment always
stays in sync with the SCENES_WITHOUT_HUD constant.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f3463576-acf6-4465-b0cc-6e5ea30633de
📒 Files selected for processing (10)
.github/workflows/test.ymlsrc/App.tsxsrc/overworld.csssrc/scenes/Overworld.tsxsrc/ui/overworld/EventLog.tsxsrc/ui/overworld/OverworldHUD.tsxsrc/ui/overworld/OverworldHeader.tsxsrc/ui/overworld/OverworldMenu.tsxtests/ui/App.test.jsxtests/ui/Overworld.test.jsx
| typecheck: | ||
| name: Typecheck | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Install pnpm | ||
| uses: pnpm/action-setup@v6 | ||
| with: | ||
| version: ${{ env.PNPM_VERSION }} | ||
|
|
||
| - name: Use Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '22.13.0' | ||
| cache: 'pnpm' | ||
|
|
||
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile --ignore-scripts | ||
|
|
||
| - name: Run TypeScript checks | ||
| run: pnpm run typecheck |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — typecheck job follows the same setup pattern as the other jobs.
The new typecheck job mirrors ts-nocheck-guard (checkout → pnpm setup → Node → install → run), and the pnpm run typecheck invocation matches the documented quality-gate step. CI will fail naturally on a non-zero exit from tsc, so no extra if: always() plumbing is needed.
Optional follow-up (out of scope here): the boilerplate (checkout/pnpm/Node/install) is now repeated across five jobs; extracting it into a composite action under .github/actions/ would reduce drift the next time Node/pnpm versions are bumped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 157 - 181, The new GitHub Actions
job named "typecheck" is correct and follows the same setup pattern as
"ts-nocheck-guard"; no functional changes are required—ensure the job retains
the steps (actions/checkout@v6, pnpm/action-setup@v6, actions/setup-node@v6,
pnpm install --frozen-lockfile --ignore-scripts) and the final command "pnpm run
typecheck" so TypeScript failures surface as non-zero exits; optionally,
consider refactoring the repeated setup steps into a composite action under
.github/actions/ to DRY up the repeated boilerplate across jobs.
| ({ player, band }: OverworldHUDProps) => { | ||
| const { t } = useTranslation(['ui']) | ||
| const [showSC, setShowSC] = useState(false) | ||
| const { audioState: isPlaying, handleAudioChange } = useAudioControl() |
There was a problem hiding this comment.
🔴 isPlaying is always a truthy object, making the mute toggle keyboard shortcut only ever stop music
The new OverworldHUD calls useAudioControl() with no selector argument (src/ui/overworld/OverworldHUD.tsx:53). Without a selector, useAudioControl returns the full audio state snapshot object (with fields like musicVol, sfxVol, isMuted, isPlaying, currentSongId — see src/hooks/useAudioControl.ts:40-56), not a boolean. This object is destructured as isPlaying. Since a non-null object is always truthy in JavaScript, the if (isPlaying) check on line 136 always evaluates to true, so pressing 'm' always calls handleAudioChange.stopMusic() and never handleAudioChange.resumeMusic(). Users cannot unmute/resume audio via the keyboard shortcut.
Old code used a selector that returned a boolean
The old version passed a selector:
const { audioState: isPlaying, handleAudioChange } = useAudioControl(
useCallback((state) => {
return audioState.currentSongId === 'ambient' && audioState.isPlaying === true;
}, []),
{ pollEvenWithSubscribe: true, pollMs: 1000 }
);This returned a boolean, so if (isPlaying) worked correctly.
Prompt for agents
In src/ui/overworld/OverworldHUD.tsx line 53, useAudioControl() is called without a selector, so audioState (aliased as isPlaying) is the full snapshot object, not a boolean. The keyboard handler at line 136 does `if (isPlaying)` which is always truthy for an object.
Fix: Either pass a selector to useAudioControl that extracts a boolean (e.g. `state => state.isPlaying === true`), or access the isPlaying property from the snapshot object. For example:
Option A: Add a selector:
const { audioState: isPlaying, handleAudioChange } = useAudioControl(
useCallback((state) => state?.isPlaying === true, [])
)
Option B: Use the snapshot properly:
const { audioState, handleAudioChange } = useAudioControl()
// then in the handler: if (audioState?.isPlaying) { stopMusic() } else { resumeMusic() }
Also check that the selector return type and the dependency array on isPlaying at line 151 remain correct after the fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <div className='hud-btns pointer-events-auto'> | ||
| <GlitchButton | ||
| className='!w-[30px] !h-[30px] !p-0' | ||
| variant={showSC ? 'warning' : 'primary'} | ||
| size='sm' | ||
| onClick={() => setShowSC(s => !s)} | ||
| aria-label={t('ui:overworld.shortcuts.help', { | ||
| defaultValue: 'Toggle Help' | ||
| })} | ||
| aria-pressed={showSC} | ||
| aria-controls={shortcutsPanelId} | ||
| > | ||
| ? | ||
| </GlitchButton> | ||
| </div> |
There was a problem hiding this comment.
🚩 OverworldHUD mute button removed — mute is now keyboard-only
The old OverworldHUD rendered a visible 🔇 mute toggle button. The new version removes this button entirely — mute/unmute is now only accessible via the 'm' keyboard shortcut (which is itself broken per BUG-0001). Even after BUG-0001 is fixed, the removal of the visible mute button means touch/mouse-only users have no way to toggle audio mute from the Overworld HUD. The ToggleRadio component in src/scenes/Overworld.tsx:228 may cover some audio control, but it's worth confirming that removing the dedicated mute button is intentional.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verify each finding against the current code and only fix it if needed. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scenes/Overworld.tsx (1)
104-111:⚠️ Potential issue | 🟡 Minor
BloodBankModalis now unreachable — clean up the dead hook wiring and modal render.
openBloodBankis destructured fromuseBloodBank()at line 106 but is never called anywhere in the file. TheBloodBankModalblock at lines 292–299 is still rendered, but with no call site foropenBloodBankthere is no path to setshowBloodBank = true, leaving the modal and theuseBloodBank()hook as dead code. Remove the unused hook destructuring and modal render block:Cleanup diff
- const { - showBloodBank, - openBloodBank, - closeBloodBank, - triggerDonate, - canDonate, - config: bloodBankConfig - } = useBloodBank() - @@ - {showBloodBank && ( - <BloodBankModal - onClose={closeBloodBank} - onDonate={triggerDonate} - canDonate={canDonate} - config={bloodBankConfig} - /> - )}Also remove the
BloodBankModalimport if it is no longer used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Overworld.tsx` around lines 104 - 111, Remove the dead BloodBank wiring: delete the unused openBloodBank from the useBloodBank() destructure in Overworld.tsx and remove the BloodBankModal render block (lines that create <BloodBankModal ... />) plus its import if it is no longer referenced elsewhere; keep any remaining needed symbols from useBloodBank() (e.g., triggerDonate, closeBloodBank, canDonate, showBloodBank, config/bloodBankConfig) only if they are actually used in this file. Ensure there are no lingering references to openBloodBank or BloodBankModal after cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/overworld.css`:
- Around line 15-37: The `@keyframes` named "scan" in this diff conflicts with
another "scan" keyframe in index.css; rename the keyframe to a unique identifier
(e.g., "scan-overworld") and update the .scan rule to use that new animation
name so the scanline uses the intended top-based animation; change both the
keyframe declaration (`@keyframes` scan -> `@keyframes` scan-overworld) and the
.scan animation property (animation: scan 10s... -> animation: scan-overworld
10s...) to ensure no collision with the existing transform-based "scan"
keyframes.
In `@src/ui/overworld/EventLog.tsx`:
- Around line 23-71: The entries state in EventLog.tsx currently stores
i18n-translated strings (created via t(...)) so historical log lines won't
update when the locale changes; change the EventLogEntry shape (used by entries,
setEntries and referenced in the useEffect) to store a type/kind and payload
(e.g., { id, day, type: 'system'|'travel'|'init', payload: { count? | location?
| date? } }) instead of pre-translated msg, update the useEffect that pushes new
entries to push these typed payloads (see previousRef, entryIdRef and the added
array), and then perform t(...) at render time when mapping entries to UI so
translations update dynamically when t or locale changes. Ensure initial state
also uses a typed payload rather than calling t(...) directly.
In `@src/ui/overworld/OverworldMenu.tsx`:
- Around line 270-299: The effect currently re-captures
previousFocusedElementRef.current whenever !activeCat while the menu is open, so
change it to capture the opener only on the transition from menu closed to open:
add a ref like prevIsMenuOpenRef and check that prevIsMenuOpenRef.current is
false before setting previousFocusedElementRef.current (only when isMenuOpen
becomes true and !activeCat), then set prevIsMenuOpenRef.current = isMenuOpen at
the end of the effect (or update it in a separate effect) so submenu navigation
(activeCat toggles) won’t overwrite the original opener; keep the rest of the
logic (focusing firstMenuButton, didFocusMenuRef, and focus restore) unchanged.
---
Outside diff comments:
In `@src/scenes/Overworld.tsx`:
- Around line 104-111: Remove the dead BloodBank wiring: delete the unused
openBloodBank from the useBloodBank() destructure in Overworld.tsx and remove
the BloodBankModal render block (lines that create <BloodBankModal ... />) plus
its import if it is no longer referenced elsewhere; keep any remaining needed
symbols from useBloodBank() (e.g., triggerDonate, closeBloodBank, canDonate,
showBloodBank, config/bloodBankConfig) only if they are actually used in this
file. Ensure there are no lingering references to openBloodBank or
BloodBankModal after cleanup.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0cfdcdf1-805e-409b-9bb8-df1e20d2f64f
📒 Files selected for processing (13)
public/locales/de/ui.jsonpublic/locales/en/ui.jsonsrc/components/PixiStageController.tssrc/components/overworld/OverworldMap.tsxsrc/components/stage/EffectTextureManager.tssrc/components/stage/LaneManager.tssrc/components/stage/StageLifecycleUtils.tssrc/hooks/usePreGigLogic.tssrc/index.csssrc/overworld.csssrc/scenes/Overworld.tsxsrc/ui/overworld/EventLog.tsxsrc/ui/overworld/OverworldMenu.tsx
| @keyframes scan { | ||
| 0% { | ||
| top: -4px; | ||
| } | ||
| 100% { | ||
| top: 100%; | ||
| } | ||
| } | ||
| .scan { | ||
| position: fixed; | ||
| left: 0; | ||
| width: 100%; | ||
| height: 4px; | ||
| z-index: 29; | ||
| pointer-events: none; | ||
| background: linear-gradient( | ||
| to bottom, | ||
| transparent, | ||
| rgb(var(--color-toxic-green-rgb) / 15%), | ||
| transparent | ||
| ); | ||
| animation: scan 10s linear infinite; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,8p' src/index.css
echo '---'
rg -n '@keyframes\s+scan\b|animation:\s*scan\b' src/index.css src/overworld.cssRepository: DaFum/neurotoxic-game
Length of output: 622
🏁 Script executed:
sed -n '570,585p' src/index.cssRepository: DaFum/neurotoxic-game
Length of output: 332
Use a unique keyframe name for the scanline animation.
src/index.css defines @keyframes scan at line 572 with a different transform-based animation. Since src/index.css imports src/overworld.css, the later definition overrides the earlier one, causing .scan to use the wrong animation. Rename this keyframe to avoid the collision.
🐛 Proposed fix
-@keyframes scan {
+@keyframes overworld-scan {
0% {
top: -4px;
}
100% {
top: 100%;
}
}
.scan {
position: fixed;
left: 0;
width: 100%;
height: 4px;
z-index: 29;
pointer-events: none;
background: linear-gradient(
to bottom,
transparent,
rgb(var(--color-toxic-green-rgb) / 15%),
transparent
);
- animation: scan 10s linear infinite;
+ animation: overworld-scan 10s linear infinite;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/overworld.css` around lines 15 - 37, The `@keyframes` named "scan" in this
diff conflicts with another "scan" keyframe in index.css; rename the keyframe to
a unique identifier (e.g., "scan-overworld") and update the .scan rule to use
that new animation name so the scanline uses the intended top-based animation;
change both the keyframe declaration (`@keyframes` scan -> `@keyframes`
scan-overworld) and the .scan animation property (animation: scan 10s... ->
animation: scan-overworld 10s...) to ensure no collision with the existing
transform-based "scan" keyframes.
| const [entries, setEntries] = useState<EventLogEntry[]>(() => [ | ||
| { | ||
| id: ++entryIdRef.current, | ||
| day, | ||
| type: 'system', | ||
| msg: t('ui:overworld.locations_loaded', { | ||
| count: ALL_VENUES.length, | ||
| defaultValue: `Tour initialized. ${ALL_VENUES.length} locations loaded.` | ||
| }) | ||
| } | ||
| ]) | ||
| const previousRef = useRef<{ day: number; locationName: string } | null>( | ||
| null | ||
| ) | ||
|
|
||
| return ( | ||
| <div className="event-log absolute bottom-8 left-8 z-20 pointer-events-none"> | ||
| <div className="el-header"> | ||
| <span className="el-title">// {t('ui:overworld.event_log', { defaultValue: 'EVENT LOG' })}</span> | ||
| <span className="el-count">{entries.length} {t('ui:overworld.event_log_entries', { defaultValue: 'entries' })}</span> | ||
| </div> | ||
| <div className="el-body" ref={bodyRef}> | ||
| {entries.map((e,i)=>( | ||
| <div className="el-entry" key={i}> | ||
| <span className="el-day">[{String(e.day).padStart(2,'0')}]</span> | ||
| <span className={`el-msg t-${e.type}`}>> {e.msg}</span> | ||
| useEffect(() => { | ||
| const previous = previousRef.current | ||
| const added: EventLogEntry[] = [] | ||
|
|
||
| if (!previous || previous.day !== day) { | ||
| added.push({ | ||
| id: ++entryIdRef.current, | ||
| day, | ||
| type: 'system', | ||
| msg: t('ui:overworld.tour_active', { | ||
| date: `Day ${day}`, | ||
| defaultValue: `Day ${day}: Tour active.` | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| if (!previous || previous.locationName !== locationName) { | ||
| added.push({ | ||
| id: ++entryIdRef.current, | ||
| day, | ||
| type: 'travel', | ||
| msg: t('ui:overworld.location_secured', { | ||
| location: locationName, | ||
| defaultValue: `${locationName} secured.` | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| if (added.length > 0) { | ||
| setEntries(prev => [...prev, ...added].slice(-20)) | ||
| } | ||
|
|
||
| previousRef.current = { day, locationName } | ||
| }, [day, locationName, t]) |
There was a problem hiding this comment.
Don't freeze translated log copy in component state.
The new entries state stores t(...) results as plain strings, so switching languages later leaves the existing history—including the initial "locations loaded" row—in the old locale. Persist a typed entry kind + payload and translate during render instead.
As per coding guidelines, "All player-facing text must be i18n-driven using t('ns:key') or <Trans> components in src/ui/**`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/overworld/EventLog.tsx` around lines 23 - 71, The entries state in
EventLog.tsx currently stores i18n-translated strings (created via t(...)) so
historical log lines won't update when the locale changes; change the
EventLogEntry shape (used by entries, setEntries and referenced in the
useEffect) to store a type/kind and payload (e.g., { id, day, type:
'system'|'travel'|'init', payload: { count? | location? | date? } }) instead of
pre-translated msg, update the useEffect that pushes new entries to push these
typed payloads (see previousRef, entryIdRef and the added array), and then
perform t(...) at render time when mapping entries to UI so translations update
dynamically when t or locale changes. Ensure initial state also uses a typed
payload rather than calling t(...) directly.
| useEffect(() => { | ||
| if (isMenuOpen) { | ||
| if (!activeCat) { | ||
| previousFocusedElementRef.current = | ||
| document.activeElement instanceof HTMLElement | ||
| ? document.activeElement | ||
| : null | ||
| } | ||
| const rafId = window.requestAnimationFrame(() => { | ||
| const firstMenuButton = | ||
| menuRootRef.current?.querySelector<HTMLButtonElement>( | ||
| '.menu-panel button' | ||
| ) | ||
| if (firstMenuButton) { | ||
| firstMenuButton.focus() | ||
| didFocusMenuRef.current = true | ||
| } | ||
| }) | ||
| return () => { | ||
| window.cancelAnimationFrame(rafId) | ||
| } | ||
| } | ||
| if ( | ||
| didFocusMenuRef.current && | ||
| document.activeElement !== previousFocusedElementRef.current | ||
| ) { | ||
| previousFocusedElementRef.current?.focus() | ||
| } | ||
| didFocusMenuRef.current = false | ||
| }, [activeCat, isMenuOpen]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '268,299p' src/ui/overworld/OverworldMenu.tsxRepository: DaFum/neurotoxic-game
Length of output: 1057
🏁 Script executed:
#!/bin/bash
# Find handleBack function and activeCat state management
rg -A 5 'const handleBack' src/ui/overworld/OverworldMenu.tsxRepository: DaFum/neurotoxic-game
Length of output: 239
🏁 Script executed:
#!/bin/bash
# Check how activeCat state is updated
rg 'activeCat' src/ui/overworld/OverworldMenu.tsx | head -20Repository: DaFum/neurotoxic-game
Length of output: 400
🏁 Script executed:
#!/bin/bash
# Find the menu open/close toggle
rg -B 2 -A 2 'isMenuOpen.*=' src/ui/overworld/OverworldMenu.tsx | head -30Repository: DaFum/neurotoxic-game
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Get full context of the useEffect to see the complete flow
sed -n '260,310p' src/ui/overworld/OverworldMenu.tsxRepository: DaFum/neurotoxic-game
Length of output: 1568
🏁 Script executed:
#!/bin/bash
# Check if there are other focus-related refs or logic
rg -B 3 -A 3 'previousFocusedElementRef|didFocusMenuRef' src/ui/overworld/OverworldMenu.tsxRepository: DaFum/neurotoxic-game
Length of output: 1084
🏁 Script executed:
#!/bin/bash
# Check if there's a separate handler for submenu logic or if keyboard handler interacts
sed -n '300,330p' src/ui/overworld/OverworldMenu.tsxRepository: DaFum/neurotoxic-game
Length of output: 1109
Capture the restore target only on the initial menu open.
The effect overwrites previousFocusedElementRef.current every time !activeCat while the menu is open. When the user returns from a submenu with handleBack(), activeCat becomes null and this condition fires again, replacing the stored opener with the now-focused first menu button. On final close, focus restore compares against the wrong element and fails to restore to the real opener. Capture document.activeElement only when transitioning from isMenuOpen false to true; guard against re-capture during submenu navigation by checking the state transition explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/overworld/OverworldMenu.tsx` around lines 270 - 299, The effect
currently re-captures previousFocusedElementRef.current whenever !activeCat
while the menu is open, so change it to capture the opener only on the
transition from menu closed to open: add a ref like prevIsMenuOpenRef and check
that prevIsMenuOpenRef.current is false before setting
previousFocusedElementRef.current (only when isMenuOpen becomes true and
!activeCat), then set prevIsMenuOpenRef.current = isMenuOpen at the end of the
effect (or update it in a separate effect) so submenu navigation (activeCat
toggles) won’t overwrite the original opener; keep the rest of the logic
(focusing firstMenuButton, didFocusMenuRef, and focus restore) unchanged.
DaFum
merged commit e53ef58
into
feature/neurotoxic-overworld-ui-443636132653992449
| useEffect(() => { | ||
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (!isMenuOpen) return | ||
| if (event.key === 'Escape') { | ||
| event.stopImmediatePropagation() | ||
| event.preventDefault() | ||
| if (activeCat) { | ||
| handleBack() | ||
| } else { | ||
| handleClose() | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🚩 OverworldMenu ESC handler uses stopImmediatePropagation but OverworldHUD ESC handler fires first
Both OverworldHUD (line 144) and OverworldMenu (line 326) register window keydown listeners for Escape. Since OverworldHUD renders before OverworldMenu in Overworld.tsx (line 221 vs 231), React runs its effects first, meaning the HUD's listener is registered on window before the Menu's. When Escape is pressed while the menu is open, the HUD handler fires first (closes shortcuts panel), then the Menu handler fires with stopImmediatePropagation() — but the HUD handler already ran. The stopImmediatePropagation() in OverworldMenu.tsx:327 is effectively a no-op for preventing the HUD handler. If both the shortcuts panel and menu are open, pressing Escape closes both simultaneously rather than one at a time.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Motivation
Description
typecheckjob to the GitHub Actions workflow (.github/workflows/test.yml) that runspnpm run typecheckin CI.src/scenes/Overworld.tsxnow readssettingsfor the CRT overlay, adds anopenCliniccallback, improves glitch timer handling and audio retry typing/cleanup, and localizes the radio label.OverworldMenu,OverworldHUD,OverworldHeader, andEventLogwere modernized with stronger typing, improved accessibility (focus management, keyboard trapping/Escapehandling), localized strings, animation fixes, and stateful event log entries.src/overworld.cssto adjust z-indexes, replace hardcoded colors with CSS variables, and tweak HUD visuals and animations.tests/ui/*.Testing
pnpm test/ vitest) against the updated tests; the tests completed successfully.pnpm run typecheckto verify types for the refactor; the typecheck passed.Codex Task