Skip to content

fix(pad): URL view-option params lost to padeditor.init race (#7840)#7843

Merged
SamTV12345 merged 2 commits into
developfrom
fix/url-view-option-race-7840
May 25, 2026
Merged

fix(pad): URL view-option params lost to padeditor.init race (#7840)#7843
SamTV12345 merged 2 commits into
developfrom
fix/url-view-option-race-7840

Conversation

@JohnMcLear
Copy link
Copy Markdown
Member

Summary

Closes #7840. ?showLineNumbers=false (and ?useMonospaceFont=true) on an iframe-embedded pad get re-applied to the server default a few hundred ms after pad boot. Same race that affected ?rtl=false before #7464 — generalises that fix to the neighbouring URL view-toggles.

Why the iframe reporter sees it and direct users don't

The race always fires, but it only becomes observable when the value padeditor.init's deferred setViewOptions(initialViewOptions) call re-applies disagrees with the URL param. initialViewOptions is built from cookie pref ?? server default:

  • Direct-browser users typically have a prefs cookie from a prior in-pad toggle that matches what they're asking for via URL → no observable race.
  • Cross-context iframe embeds with no cookie → server default showLineNumbers:true fights the URL false → URL preference visibly lost.

What the race looks like

  1. _afterHandshakegetParams() sets settings.LineNumbersDisabled = true.
  2. _afterHandshake calls padeditor.init(view).then(postAceInit) — async; ace iframes still loading.
  3. Sync tail of _afterHandshake hits the URL-param block: changeViewOption('showLineNumbers', false) → queues setProperty('showslinenumbers', false) in Ace2Editor.actionsPendingInit (loaded=false).
  4. ace.init resolves → loaded=true → queue flushes → URL-driven false applied.
  5. padeditor.init resumes past its own await and runs self.setViewOptions(initialViewOptions)initialViewOptions.showLineNumbers === true (server default) → setProperty('showslinenumbers', true) runs against loaded=true and immediately re-shows the gutter.

#7464 spotted this for RTL and moved the override into postAceInit. The neighbouring blocks for showLineNumbers / noColors / useMonospaceFontGlobal at the same synchronous-tail site never got the same treatment. This PR generalises that fix.

Test plan

  • New spec `src/tests/frontend-new/specs/url_view_options.spec.ts`:
    • `?showLineNumbers=false` → outer body has `.line-numbers-hidden` on first paint
    • `?showLineNumbers=true` no-regression
    • `?useMonospaceFont=true` → `#viewfontmenu` value is `RobotoMono` on first paint
  • Verified the three race cases fail on `develop` before the fix.
  • Verified `rtl_url_param.spec.ts`, `timeslider_line_numbers.spec.ts`, `pad_settings.spec.ts`, `embed_value.spec.ts`, `hide_menu_right.spec.ts` still pass (26/26 green together).

Not in scope

  • The `?noColors=true` URL path turns out to be unaffected for the inner-body `authorColors` class (the override at `pad_editor.ts:268-270` always wins). The colorscheck checkbox UI doesn't reflect `settings.noColors` in either the racy or fixed state — that's a pre-existing cosmetic mismatch independent of this race and worth its own follow-up.

🤖 Generated with Claude Code

@qodo-code-review
Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Fix URL view-option params lost to padeditor.init race

🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Move URL view-option overrides into postAceInit to prevent race condition
  - Fixes showLineNumbers, noColors, useMonospaceFont params being clobbered
• Generalizes fix from #7464 (RTL race) to all three neighboring view toggles
• Add comprehensive test suite for URL view-option parameters on initial load
Diagram
flowchart LR
  A["_afterHandshake sync tail<br/>queues URL overrides"] -->|"race condition"| B["padeditor.init resolves<br/>calls setViewOptions"]
  B -->|"clobbers URL values"| C["Server defaults applied<br/>URL params lost"]
  D["postAceInit<br/>after padeditor.init"] -->|"fix: moved here"| E["URL overrides applied<br/>after initialViewOptions"]
  E -->|"no race"| F["URL params preserved"]

Loading

File Changes

1. src/static/js/pad.ts 🐞 Bug fix +17/-18

Move URL view-option overrides to postAceInit

• Moved LineNumbersDisabled, noColors, and useMonospaceFontGlobal URL-param overrides from
 _afterHandshake sync tail into postAceInit function
• Added detailed comment explaining the race condition and why these overrides must run after
 padeditor.init resolves
• Removed duplicate override logic from the synchronous tail section (lines 807-829)

src/static/js/pad.ts


2. src/tests/frontend-new/specs/url_view_options.spec.ts 🧪 Tests +49/-0

Add URL view-option parameter race condition tests

• New test file covering URL-param view options on initial pad load
• Tests ?showLineNumbers=false hides line-number gutter on first paint
• Tests ?showLineNumbers=true keeps gutter visible (no regression)
• Tests ?useMonospaceFont=true applies monospace font on first paint
• Clears cookies in beforeEach to simulate cross-context iframe embeds without prior preferences

src/tests/frontend-new/specs/url_view_options.spec.ts


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 25, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Qodo Logo

JohnMcLear and others added 2 commits May 25, 2026 14:39
`?showLineNumbers=false` and `?useMonospaceFont=true` were being
silently clobbered shortly after pad load. Same race that affected
`?rtl=false` before #7464:

  1. _afterHandshake → getParams() sets settings.LineNumbersDisabled
     (or useMonospaceFontGlobal, noColors).
  2. _afterHandshake calls padeditor.init(view).then(postAceInit) —
     async; ace iframes still loading.
  3. Sync tail of _afterHandshake hits the URL-param overrides and
     calls changeViewOption('showLineNumbers', false) etc. These
     queue setProperty('showslinenumbers', false) in Ace2Editor's
     actionsPendingInit queue (loaded=false).
  4. ace.init resolves → loaded=true → queue flushes → URL-driven
     value applied.
  5. padeditor.init resumes past its own await and calls
     setViewOptions(initialViewOptions) — initialViewOptions is
     built from clientVars.initialOptions.view (server defaults
     ∨ cookie), which does NOT carry the URL preference. The
     resulting setProperty('showslinenumbers', true) runs against
     loaded=true ace and immediately re-shows the gutter.

#7464 noticed this race for RTL and moved the override into
postAceInit. The neighbouring blocks for showLineNumbers / noColors
/ useMonospaceFontGlobal were left at the synchronous-tail site —
generalise the same fix to all three.

Direct-browser users typically had a `prefs` cookie with
showLineNumbers=false from a prior in-pad toggle, so the
initialViewOptions value happened to match the URL param and the
race was unobservable. Cross-context iframe embeds (the reporter's
configuration) start with no cookie, so the server default true
fights the URL false and the race becomes visible.

Adds src/tests/frontend-new/specs/url_view_options.spec.ts covering
the showLineNumbers=false / =true / useMonospaceFont=true cases on
initial-load navigation (the path where the race actually fires).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes CI ts-check: parameters page/padId/param implicitly had `any`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JohnMcLear JohnMcLear force-pushed the fix/url-view-option-race-7840 branch from e54cfba to 769c08f Compare May 25, 2026 13:39
@JohnMcLear JohnMcLear requested a review from SamTV12345 May 25, 2026 14:00
@SamTV12345
Copy link
Copy Markdown
Member

Makes sense that it is now firing correctly.

@SamTV12345 SamTV12345 merged commit bfaf442 into develop May 25, 2026
32 checks passed
@SamTV12345 SamTV12345 deleted the fix/url-view-option-race-7840 branch May 25, 2026 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iframe showLineNumbers=false ignored

2 participants