Repository navigation
fix: navbar listener leaks, download overlap, and web fonts - #1036
Merged
Merged
Conversation
Contributor
Reviewer's GuideThe PR replaces repeated NavbarStyler construction with a shared, explicitly managed instance that owns one removable resize listener, cleans up when navbar state changes, and ignores detached elements; a Playwright regression test validates listener counts, visual geometry updates, and cleanup across sustained refreshes and navbar replacement/removal. Sequence diagram for shared navbar styler refreshsequenceDiagram
participant Refresh as UI refresh timer
participant Update as UpdateNavbarStyler
participant Styler as NavbarStyler
participant Window as window
participant Navbar as Navbar element
Refresh->>Update: UpdateNavbarStyler()
Update->>Update: querySelector(navbar)
alt NewTopBar enabled and same navbar
Update->>Styler: init()
Styler->>Navbar: applyStyles()
Styler->>Navbar: createOverlay()
Styler->>Navbar: createSpacer()
else NewTopBar enabled and navbar replaced
Update->>Styler: destroy()
Styler->>Window: removeEventListener(resize)
Update->>Styler: new NavbarStyler(navbar)
Styler->>Window: addEventListener(resize)
else NewTopBar disabled or navbar removed
Update->>Styler: destroy()
Styler->>Window: removeEventListener(resize)
end
State diagram for navbar styler lifecyclestateDiagram-v2
[*] --> Unmanaged
Unmanaged --> Managed: UpdateNavbarStyler() with navbar and NewTopBar
Managed --> Managed: UpdateNavbarStyler() with same navbar / init()
Managed --> Managed: resize / updateBlurOverlay()
Managed --> Unmanaged: navbar replaced or NewTopBar disabled / destroy()
Managed --> Unmanaged: navbar removed / destroy()
Unmanaged --> [*]
Flow diagram for safe navbar resize handlingflowchart TD
Resize[resize event] --> Connected{navbar.isConnected}
Connected -->|yes| Blur["updateBlurOverlay()"]
Connected -->|no| Skip[Skip detached navbar]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Deploying xmoj-script-dev-channel with
|
| Latest commit: |
8758133
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bd27eca5.xmoj-script-dev-channel.pages.dev |
| Branch Preview URL: | https://codex-fix-navbar-listener-le.xmoj-script-dev-channel.pages.dev |
hendragon-bot
Bot
added
the
user-script
![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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="XMOJ.user.js" line_range="2824-2827" />
<code_context>
}
}
+ destroy() {
+ window.removeEventListener('resize', this.resizeHandler);
+ this.navbar = null;
+ }
+
init() {
</code_context>
<issue_to_address>
**issue (bug_risk):** When the navbar is removed or NewTopBar is disabled, `destroy()` removes only the resize listener and clears the navbar reference; the `#navbar-spacer` element and `#blur-overlay` remain in the document. The spacer continues to reserve the old navbar height, leaving a blank gap after the navbar has disappeared.
**Triggers:** When a page removes its navbar or the user disables NewTopBar without reloading.
**Suggested fix:** Remove or reset the styler-owned `#navbar-spacer` and `#blur-overlay` during destruction, while preserving any elements that were not created by this styler.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: XMOJ.user.js:2827
![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: c1b6d4b53d
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
boomzero
changed the title
sourcery-ai
Bot
dismissed
their stale review
October 3, 2026 13:16
Sourcery withdrew this approval because it has stopped reviewing this pull request.
Contributor
|
Sourcery has withdrawn its approval of this pull request. It auto-reviews a pull request 5 times, and this push is past that limit, so the approval no longer reflects code Sourcery has read. Comment |
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/navbar-styler.test.cjs">
<violation number="1" location="tests/navbar-styler.test.cjs:184">
P2: This assertion passes even when the spacer does not grow: the initial 88px spacer plus 16px margin leaves the card around 104px, still below a 96px navbar. Assert that the card top advances by the navbar-height delta so the test catches stale spacing.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| firstCardTop: document.querySelector('.software_item').getBoundingClientRect().top | ||
| }; | ||
| }); | ||
| assert.ok(after.firstCardTop >= after.navbarBottom, 'the spacing must follow navbar height changes'); |
Contributor
There was a problem hiding this comment.
P2: This assertion passes even when the spacer does not grow: the initial 88px spacer plus 16px margin leaves the card around 104px, still below a 96px navbar. Assert that the card top advances by the navbar-height delta so the test catches stale spacing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/navbar-styler.test.cjs, line 184:
<comment>This assertion passes even when the spacer does not grow: the initial 88px spacer plus 16px margin leaves the card around 104px, still below a 96px navbar. Assert that the card top advances by the navbar-height delta so the test catches stale spacing.</comment>
<file context>
@@ -133,3 +133,56 @@ test('navbar refreshes keep one resize listener and release replaced navbars', {
+ firstCardTop: document.querySelector('.software_item').getBoundingClientRect().top
+ };
+ });
+ assert.ok(after.firstCardTop >= after.navbarBottom, 'the spacing must follow navbar height changes');
+ } finally {
+ await browser.close();
</file context>
Suggested change
| assert.ok(after.firstCardTop >= after.navbarBottom, 'the spacing must follow navbar height changes'); | |
| assert.ok(after.firstCardTop >= before.firstCardTop + (after.navbarBottom - before.navbarBottom), 'the first row must move with navbar height changes'); |
boomzero
changed the title
PythonSmall-Q
approved these changes
Oct 3, 2026
This was referenced Oct 3, 2026
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
2 participants
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.
What does this PR aim to accomplish?:
Fix the navbar resize-listener leak deferred during emergency release #1033 the downloads.php card overlap, and inconsistent navbar fonts between /web and classic pages. With NewTopBar enabled, every refresh previously registered another permanent resize listener: once per second on the contest app and every 100 ms on classic pages. Long-lived tabs accumulated callbacks and could retain navbars replaced by Vue.
Addresses the review finding on #1033.
How does this PR accomplish the above?:
Both UI refresh paths now use a shared navbar styler. The spacer uses a block-level span so legacy body > div selectors continue to target the page content container; the downloads handler previously appended its cards inside the spacer, underneath the fixed navbar. A stable navbar reuses its instance and listener while refreshing the existing styles, spacer and blur overlay. Navbar replacement, removal, or disabling NewTopBar removes the old listener, restores the original navbar styles and classes, removes styler-owned overlays, styles and spacers, and clears the retained navbar reference. Existing page-owned overlays and spacers are preserved with their original inline geometry restored. Resize events skip detached navbars. A shared, deduplicated monochrome font loader runs during early initialization and both fallback paths, so /web requests the same Source Serif 4, Playfair Display and JetBrains Mono stylesheet as classic pages instead of staying on fallback fonts.
Validation: all 18 npm tests passed using installed Chromium, including a new browser regression with 4,000 refreshes, navbar replacement, resize geometry, setting changes, navbar removal, detached-node resize guards, full teardown, style restoration and preservation of page-owned elements. A downloads regression runs the real page handler, reproduces the failure with the old spacer, and verifies that all 15 cards remain below the navbar after the fix and after its height changes. Font-loading coverage checks /web and classic initialization with and without cached Bootstrap CSS, before and after head creation, duplicate initialization, and disabled skins. Userscript syntax and git diff whitespace checks passed. No additional dependency was introduced.
The contributor guide and complete template have been read. This PR targets dev; version metadata is managed by the existing bot. The final checklist remains unchecked because item 11 requires a human to verify both the new and old/classic XMOJ UI; automated browser fixture checks do not assert that manual verification. Please verify both UIs before confirming the checklist.
By submitting this pull request, I confirm the following:
git rebase)