Fix Panel C target reachability and compact power-up tray re-anchoring on fullscreen lifecycle changes - #76
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…screen lifecycle Agent-Logs-Url: https://github.com/TeacherEvan/MathMasterHTML/sessions/8970ed66-eade-42c9-a1ec-f2e3e6fe5730 Co-authored-by: TeacherEvan <189960447+TeacherEvan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/TeacherEvan/MathMasterHTML/sessions/8970ed66-eade-42c9-a1ec-f2e3e6fe5730 Co-authored-by: TeacherEvan <189960447+TeacherEvan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix bug with falling characters in Panel C viewport
Fix Panel C target reachability and compact power-up tray re-anchoring on fullscreen lifecycle changes
Apr 18, 2026
TeacherEvan
requested changes
Apr 18, 2026
TeacherEvan
left a comment
TeacherEvan
left a comment
Owner
There was a problem hiding this comment.
Is this the best you can do? @copilot
Contributor
Author
For the requested minimal/surgical scope, yes— |
TeacherEvan
approved these changes
Apr 19, 2026
Contributor
Reviewer's GuideRestricts the Panel C 'stuck at bottom' cleanup heuristic to desktop-only and wires the compact power-up tray layout to fullscreen and shared resize lifecycle events, with new regression tests ensuring mobile targets reach the catch zone and the compact tray re-anchors after fullscreen changes. Sequence diagram for power-up tray layout sync on fullscreen and resize eventssequenceDiagram
participant Window
participant Document
participant SharedResizeObserver
participant WormPowerupsUI
rect rgb(230,230,250)
Window->>WormPowerupsUI: init()
WormPowerupsUI->>WormPowerupsUI: _displayLayoutHandler = syncDisplayLayout
Window->>WormPowerupsUI: addEventListener resize
Window->>WormPowerupsUI: addEventListener orientationchange
Document->>WormPowerupsUI: addEventListener fullscreenchange
Window->>Window: __ensureSharedResizeObserver()
Window-->>WormPowerupsUI: sharedResizeObserver
WormPowerupsUI->>SharedResizeObserver: subscribe(_displayLayoutHandler, immediate true, source worm-powerups-ui)
SharedResizeObserver-->>WormPowerupsUI: initial callback (immediate)
WormPowerupsUI->>WormPowerupsUI: syncDisplayLayout()
end
rect rgb(220,255,220)
Window->>WormPowerupsUI: resize event
WormPowerupsUI->>WormPowerupsUI: _displayLayoutHandler()
WormPowerupsUI->>WormPowerupsUI: syncDisplayLayout()
end
rect rgb(220,245,255)
Document->>WormPowerupsUI: fullscreenchange event
WormPowerupsUI->>WormPowerupsUI: _displayLayoutHandler()
WormPowerupsUI->>WormPowerupsUI: syncDisplayLayout()
end
rect rgb(255,245,220)
SharedResizeObserver->>WormPowerupsUI: resize notification
WormPowerupsUI->>WormPowerupsUI: _displayLayoutHandler()
WormPowerupsUI->>WormPowerupsUI: syncDisplayLayout()
end
Flow diagram for Panel C target cleanup and stuck at bottom heuristicflowchart TD
A[Start per symbol] --> B[Read symbolObj from activeFallingSymbols]
B --> C{symbolObj.y > containerHeight + 50?}
C -- Yes --> D[Mark as off screen and remove]
C -- No --> E{!state.isMobileMode?}
E -- No --> H[Skip stuck at bottom cleanup for mobile]
E -- Yes --> F{symbolObj.y > containerHeight - 100?}
F -- No --> H
F -- Yes --> G{activeFallingSymbols.length > 30?}
G -- Yes --> I[Mark as stuck at bottom and remove]
G -- No --> H
H[Keep symbol active] --> J[Check next symbol]
D --> J
I --> J
J --> K[End per symbol]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
fullscreenchangelistener and shared resize observer subscription inworm-powerups.ui.jsare only registered and never removed; consider mirroring whatever teardown/unsubscribe logic exists forresize/orientationchangeto avoid leaks when the UI is destroyed or reinitialized. - The compact layout test’s
COMPACT_TRAY_MAX_TOP = 80is a fairly strict magic value that may become brittle with small layout tweaks; it may be more robust to derive this bound from the actual control panel or safe-area geometry instead of a fixed pixel threshold.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `fullscreenchange` listener and shared resize observer subscription in `worm-powerups.ui.js` are only registered and never removed; consider mirroring whatever teardown/unsubscribe logic exists for `resize`/`orientationchange` to avoid leaks when the UI is destroyed or reinitialized.
- The compact layout test’s `COMPACT_TRAY_MAX_TOP = 80` is a fairly strict magic value that may become brittle with small layout tweaks; it may be more robust to derive this bound from the actual control panel or safe-area geometry instead of a fixed pixel threshold.
## Individual Comments
### Comment 1
<location path="src/scripts/worm-powerups.ui.js" line_range="103-106" />
<code_context>
this._displayLayoutHandler = () => this.syncDisplayLayout();
window.addEventListener("resize", this._displayLayoutHandler);
window.addEventListener("orientationchange", this._displayLayoutHandler);
+ document.addEventListener("fullscreenchange", this._displayLayoutHandler);
+ const sharedResizeObserver =
+ window.__ensureSharedResizeObserver?.() || window.SharedResizeObserver;
+ sharedResizeObserver?.subscribe?.(this._displayLayoutHandler, {
+ immediate: true,
+ source: "worm-powerups-ui",
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider how the new fullscreen listener and resize observer subscription are cleaned up to avoid leaks.
These are long-lived subscriptions. Please ensure the UI’s teardown/dispose path removes the `document` `fullscreenchange` listener and unsubscribes from the shared resize observer (if it returns an unsubscribe handle); otherwise repeated teardown/recreate cycles may accumulate handlers.
</issue_to_address>
### Comment 2
<location path="tests/powerups.spec.js" line_range="9" />
<code_context>
waitForGameplayInputReady,
} from "./utils/onboarding-runtime.js";
+const COMPACT_TRAY_MAX_TOP = 80;
+
async function pressPowerUp(page, type) {
</code_context>
<issue_to_address>
**suggestion:** Avoid hard-coding a magic top threshold for the compact tray to reduce layout brittleness.
Using a fixed `COMPACT_TRAY_MAX_TOP = 80` couples this test to the current pixel layout and makes it brittle to spacing/typography changes. Please derive this bound from the actual compact controls layout (e.g. from the controls’ bounding rect plus a margin), or at minimum add a clear comment explaining why `80` is valid and when it should be updated, so layout tweaks don’t cause spurious test failures.
```suggestion
/**
* Upper bound (in CSS px) for the compact power-up tray’s top position.
*
* This value is derived from the current compact layout:
* - ~56px: tray height + internal padding
* - ~16px: top HUD spacing
* - ~8px: safety margin so minor typography/spacing tweaks don’t break the test
*
* If the compact controls are visibly moved lower in the viewport (e.g. layout
* redesign, font/spacing changes), update this threshold to be slightly larger
* than the tray’s new expected top position. The intent is to assert that the
* tray remains “near the top” of the screen, not to lock the exact pixel value.
*/
const COMPACT_TRAY_MAX_TOP = 80;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+103
to
+106
| document.addEventListener("fullscreenchange", this._displayLayoutHandler); | ||
| const sharedResizeObserver = | ||
| window.__ensureSharedResizeObserver?.() || window.SharedResizeObserver; | ||
| sharedResizeObserver?.subscribe?.(this._displayLayoutHandler, { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Consider how the new fullscreen listener and resize observer subscription are cleaned up to avoid leaks.
These are long-lived subscriptions. Please ensure the UI’s teardown/dispose path removes the document fullscreenchange listener and unsubscribes from the shared resize observer (if it returns an unsubscribe handle); otherwise repeated teardown/recreate cycles may accumulate handlers.
| waitForGameplayInputReady, | ||
| } from "./utils/onboarding-runtime.js"; | ||
|
|
||
| const COMPACT_TRAY_MAX_TOP = 80; |
Contributor
There was a problem hiding this comment.
suggestion: Avoid hard-coding a magic top threshold for the compact tray to reduce layout brittleness.
Using a fixed COMPACT_TRAY_MAX_TOP = 80 couples this test to the current pixel layout and makes it brittle to spacing/typography changes. Please derive this bound from the actual compact controls layout (e.g. from the controls’ bounding rect plus a margin), or at minimum add a clear comment explaining why 80 is valid and when it should be updated, so layout tweaks don’t cause spurious test failures.
Suggested change
| const COMPACT_TRAY_MAX_TOP = 80; | |
| /** | |
| * Upper bound (in CSS px) for the compact power-up tray’s top position. | |
| * | |
| * This value is derived from the current compact layout: | |
| * - ~56px: tray height + internal padding | |
| * - ~16px: top HUD spacing | |
| * - ~8px: safety margin so minor typography/spacing tweaks don’t break the test | |
| * | |
| * If the compact controls are visibly moved lower in the viewport (e.g. layout | |
| * redesign, font/spacing changes), update this threshold to be slightly larger | |
| * than the tray’s new expected top position. The intent is to assert that the | |
| * tray remains “near the top” of the screen, not to lock the exact pixel value. | |
| */ | |
| const COMPACT_TRAY_MAX_TOP = 80; |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes two gameplay regressions impacting mobile/compact play by (1) preventing premature cleanup of Panel C falling symbols in mobile mode and (2) ensuring the power-up tray re-anchors correctly across fullscreen/resize lifecycle events.
Changes:
- Gate the “stuck at bottom” cleanup heuristic in symbol rain so it does not run in mobile mode.
- Hook power-up tray layout syncing into fullscreen lifecycle + shared resize observer notifications.
- Add Playwright regression assertions for Panel C catch-zone reachability and compact tray re-anchoring after fullscreen lifecycle changes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/symbol-rain.mobile.spec.js | Adds a contract asserting at least one live Panel C symbol reaches a lower “catch zone” band in a WebView-like mobile runtime. |
| tests/powerups.spec.js | Adds a compact-layout regression test asserting the power-up tray re-anchors after a fullscreen lifecycle signal. |
| src/scripts/worm-powerups.ui.js | Adds fullscreen + shared resize observer hooks to keep tray layout synced during lifecycle changes. |
| src/scripts/symbol-rain.animation.js | Prevents the “stuck at bottom” cleanup from removing active symbols in mobile mode. |
Comment on lines
101
to
+109
| window.addEventListener("resize", this._displayLayoutHandler); | ||
| window.addEventListener("orientationchange", this._displayLayoutHandler); | ||
| document.addEventListener("fullscreenchange", this._displayLayoutHandler); | ||
| const sharedResizeObserver = | ||
| window.__ensureSharedResizeObserver?.() || window.SharedResizeObserver; | ||
| sharedResizeObserver?.subscribe?.(this._displayLayoutHandler, { | ||
| immediate: true, | ||
| source: "worm-powerups-ui", | ||
| }); |
There was a problem hiding this comment.
_bindUIEventHandlers now subscribes to the shared resize observer hub but still binds direct resize/orientationchange/fullscreenchange listeners. Because the hub already forwards those events, syncDisplayLayout() will be invoked multiple times per lifecycle event (and the hub subscription can’t be cleaned up because the unsubscribe function isn’t stored). Consider using either the hub (preferred) or the direct listeners as a fallback, and store the unsubscribe callback so it can be invoked if the system is ever torn down/reinitialized.
Suggested change
| window.addEventListener("resize", this._displayLayoutHandler); | |
| window.addEventListener("orientationchange", this._displayLayoutHandler); | |
| document.addEventListener("fullscreenchange", this._displayLayoutHandler); | |
| const sharedResizeObserver = | |
| window.__ensureSharedResizeObserver?.() || window.SharedResizeObserver; | |
| sharedResizeObserver?.subscribe?.(this._displayLayoutHandler, { | |
| immediate: true, | |
| source: "worm-powerups-ui", | |
| }); | |
| } | |
| if (!this._displayLayoutUnsubscribe) { | |
| const sharedResizeObserver = | |
| window.__ensureSharedResizeObserver?.() || window.SharedResizeObserver; | |
| if (sharedResizeObserver?.subscribe) { | |
| const unsubscribe = sharedResizeObserver.subscribe( | |
| this._displayLayoutHandler, | |
| { | |
| immediate: true, | |
| source: "worm-powerups-ui", | |
| }, | |
| ); | |
| this._displayLayoutUnsubscribe = | |
| typeof unsubscribe === "function" ? unsubscribe : null; | |
| } else { | |
| window.addEventListener("resize", this._displayLayoutHandler); | |
| window.addEventListener("orientationchange", this._displayLayoutHandler); | |
| document.addEventListener("fullscreenchange", this._displayLayoutHandler); | |
| } |
This branch was successfully deployed
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
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.
This PR addresses two high-impact gameplay regressions: (1) Panel C falling targets could be culled before entering the player’s visible catch zone, and (2) the power-up tray could render in the wrong position until a fullscreen/back lifecycle event forced reflow.
Panel C target visibility (Slice 1)
Power-up tray positioning lifecycle bug (Slice 2)
worm-powerups.ui.jsnow listens to:fullscreenchangeimmediate: true)Regression coverage
Summary by Sourcery
Ensure mobile symbol rain targets remain reachable in Panel C and keep the compact power-up tray correctly anchored during fullscreen and layout changes.
Bug Fixes:
Enhancements:
Tests: