fix: delay anchor line scrolling until layout settles

[JohnMcLear]

Summary

• keep reapplying #L... line scrolling for a short settle window after pad load
• avoid landing above the intended line when late content like images changes layout
• add regression coverage for delayed content expansion after anchor scrolling

Closes #5700

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Delay anchor line scrolling until layout settles

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Refactor anchor line scrolling to reapply focus until layout settles
• Extract line number parsing and focus logic into separate functions
• Add 10-second settle window with 250ms polling intervals
• Replace animation with direct scrollTop for better reliability
• Add regression tests for delayed content expansion scenarios

Diagram

flowchart LR
  A["Hash URL with #L anchor"] -->|getHashedLineNumber| B["Extract line number"]
  B -->|focusOnLine| C["Start settle loop"]
  C -->|250ms interval| D["getCurrentTargetOffset"]
  D -->|focusOnHashedLine| E["Scroll and set selection"]
  E -->|until 10s timeout| F["Layout stabilized"]

Loading

File Changes

1. src/static/js/pad_editor.ts 🐞 Bug fix +69/-42

Refactor anchor scrolling with delayed settle mechanism

• Extract getHashedLineNumber() to parse and validate hash-based line numbers
• Create focusOnHashedLine() helper to handle scrolling and selection logic
• Refactor focusOnLine() to implement delayed settling mechanism with 250ms polling
• Replace jQuery animate with direct scrollTop() call for more reliable scrolling
• Add 10-second maximum settle duration to prevent indefinite polling

src/static/js/pad_editor.ts

---

2. src/tests/frontend-new/specs/anchor_scroll.spec.ts 🧪 Tests +50/-0

Add Playwright test for delayed anchor scroll

• Add new Playwright-based test for anchor scroll regression
• Test that #L scroll reapplies when earlier content changes height
• Verify target line stays in viewport after dynamic content expansion
• Use polling to check scroll position and viewport offset stability

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

---

3. src/tests/frontend/specs/scrollTo.js 🧪 Tests +14/-0

Add test for scroll reapplication on layout change

• Add regression test for scroll reapplication after content height changes
• Verify initial scroll position is maintained when earlier content expands
• Test that top offset increases appropriately when layout changes occur

src/tests/frontend/specs/scrollTo.js

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

1. Immediate focusUntilStable() scroll 📎 Requirement gap ≡ Correctness

Description

The new #L... behavior scrolls immediately on load via focusUntilStable() instead of deferring
until the pad layout has fully settled. This can still cause an initial incorrect viewport position
before late-loading content (for example images) finishes affecting layout.

Code

src/static/js/pad_editor.ts[R271-283]

+  const focusUntilStable = () => {
+    if (Date.now() - startTime >= maxSettleDuration) {
+      window.clearInterval(intervalId);
+      return;
  }
-  }
+    const currentOffsetTop = getCurrentTargetOffset();
+    if (currentOffsetTop == null) return;
+
+    focusOnHashedLine(ace, lineNumberInt);
+  };
+
+  focusUntilStable();
+  intervalId = window.setInterval(focusUntilStable, settleInterval);

Evidence

PR Compliance ID 1 requires anchor/line scrolling to occur only after content (including async
elements) has fully loaded and the final layout is stable. The new code calls focusUntilStable()
immediately (before any stability detection) and performs focusOnHashedLine() on each run, which
means scrolling happens prior to confirmed layout stabilization.

Defer anchor/line scrolling until pad content has fully loaded
src/static/js/pad_editor.ts[271-283]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The implementation scrolls immediately and then re-applies scrolling for a fixed settle window, but it does not actually *defer* scrolling until the layout has stabilized.
## Issue Context
Compliance requires that `#L...` navigation scroll occurs only after async content (e.g., images/plugins) has finished loading and the resulting layout is stable, to prevent landing on the wrong line/region.
## Fix Focus Areas
- src/static/js/pad_editor.ts[271-283]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Anchor scroll steals selection ☑ 🐞 Bug ≡ Correctness

Description

When loading a pad with a #L… hash, focusOnLine() re-runs focusOnHashedLine() every 250ms for up to
10s, and each run resets scroll position and Ace selection. This can prevent users from
scrolling/clicking away from the anchored line because their scroll/selection is continually
overwritten until the timer expires.

Code

src/static/js/pad_editor.ts[R266-283]

+  const maxSettleDuration = 10000;
+  const settleInterval = 250;
+  const startTime = Date.now();
+  let intervalId = null;
+
+  const focusUntilStable = () => {
+    if (Date.now() - startTime >= maxSettleDuration) {
+      window.clearInterval(intervalId);
+      return;
  }
-  }
+    const currentOffsetTop = getCurrentTargetOffset();
+    if (currentOffsetTop == null) return;
+
+    focusOnHashedLine(ace, lineNumberInt);
+  };
+
+  focusUntilStable();
+  intervalId = window.setInterval(focusUntilStable, settleInterval);

Evidence

padeditor.init() invokes exports.focusOnLine(self.ace) on load; focusOnLine() then starts a
setInterval for 10 seconds that calls focusOnHashedLine() every 250ms. focusOnHashedLine() sets
scrollTop and calls ace.ace_setSelection(), so each interval tick can override the user's current
scroll position and selection.

src/static/js/pad_editor.ts[39-55]
src/static/js/pad_editor.ts[213-285]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`exports.focusOnLine()` keeps forcing scroll + `ace_setSelection()` for the full settle window (10s) even after the target offset has stabilized, and even if the user interacts. This can make anchor links feel "stuck" on the target line.
## Issue Context
The intended behavior is to reapply the anchor scroll while late content changes layout. The current implementation does not detect stability or user intent; it simply runs until `maxSettleDuration` expires.
## Fix Focus Areas
- src/static/js/pad_editor.ts[255-285]
## Suggested fix approach
- Track the target line’s computed offset each tick (ideally the same offset basis used for scrolling).
- Clear the interval once the offset has remained within a small threshold for N consecutive ticks (e.g., 2–4).
- Additionally, cancel the interval on explicit user interaction events (wheel/touchmove/keydown/mousedown) so the code stops fighting the user.
- Ensure the existing regression tests still pass (late content insertion should still be corrected before the interval stops).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. maxSettleDuration undocumented logic ☑ 📘 Rule violation ⚙ Maintainability

Description

The settle-window re-scrolling logic (timers/intervals) is non-obvious and is introduced without
explanatory comments about intent and tradeoffs. This increases maintenance risk and makes it harder
to reason about why these constants exist and when the interval should stop.

Code

src/static/js/pad_editor.ts[R266-280]

+  const maxSettleDuration = 10000;
+  const settleInterval = 250;
+  const startTime = Date.now();
+  let intervalId = null;
+
+  const focusUntilStable = () => {
+    if (Date.now() - startTime >= maxSettleDuration) {
+      window.clearInterval(intervalId);
+      return;
  }
-  }
+    const currentOffsetTop = getCurrentTargetOffset();
+    if (currentOffsetTop == null) return;
+
+    focusOnHashedLine(ace, lineNumberInt);
+  };

Evidence

PR Compliance ID 9 requires comments for complex/non-obvious logic. The added code introduces a
timed re-scroll loop (maxSettleDuration, settleInterval, setInterval) with implicit behavior
(always runs up to 10s unless timed out) but no comment explaining rationale or expected
stabilization conditions.

src/static/js/pad_editor.ts[266-280]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new settle-window interval logic is non-obvious and lacks comments explaining intent, stop conditions, and why the chosen constants are appropriate.
## Issue Context
Future changes to editor layout or plugin content could break this behavior, and reviewers/maintainers need clear documentation of the approach.
## Fix Focus Areas
- src/static/js/pad_editor.ts[266-280]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. FF scrolling rationale lost 🐞 Bug ⚙ Maintainability

Description

The code replaced the previously Firefox-targeted scroll operation with a direct scrollTop() call,
but did not preserve any explanation or add browser coverage to justify that the Firefox-specific
workaround is no longer needed. This makes it unclear whether anchor scrolling remains correct in
Firefox and increases the risk of reintroducing an old browser-specific bug.

Code

src/static/js/pad_editor.ts[R231-234]

+  const $outerdocHTML = $aceOuter.contents().find('#outerdocbody').parent();
+  $outerdoc.css({top: `${offsetTop}px`}); // Chrome
+  $outerdocHTML.scrollTop(offsetTop);
+  const node = line[0];

Evidence

focusOnHashedLine() now sets the scroll container position via $outerdocHTML.scrollTop(offsetTop)
and has no remaining Firefox-specific handling or comments indicating why the prior Firefox-specific
behavior is safe to remove.

src/static/js/pad_editor.ts[220-234]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A previously documented Firefox-specific scroll behavior was changed to a generic `scrollTop()` implementation, but no explanation or Firefox coverage was added. This makes future maintainers unable to tell if Firefox support was intentionally dropped, fixed elsewhere, or accidentally regressed.
## Issue Context
Anchor scrolling happens inside nested iframes, which historically has had browser-specific differences. The old implementation explicitly called out Firefox.
## Fix Focus Areas
- src/static/js/pad_editor.ts[220-234]
- src/tests/frontend-new/specs/anchor_scroll.spec.ts[1-50]
## Suggested fix approach
- Either (a) restore a Firefox-safe path (if still needed) or (b) add a short comment explaining why `scrollTop()` is now sufficient.
- Add/ensure test coverage that runs this spec on Firefox in CI (or add a targeted test/config) so the behavior is continuously validated.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-free-for-open-source-projects]

1. Immediate focusuntilstable() scroll 📎 Requirement gap ≡ Correctness

The new #L... behavior scrolls immediately on load via focusUntilStable() instead of deferring
until the pad layout has fully settled. This can still cause an initial incorrect viewport position
before late-loading content (for example images) finishes affecting layout.

Agent Prompt

## Issue description
The implementation scrolls immediately and then re-applies scrolling for a fixed settle window, but it does not actually *defer* scrolling until the layout has stabilized.

## Issue Context
Compliance requires that `#L...` navigation scroll occurs only after async content (e.g., images/plugins) has finished loading and the resulting layout is stable, to prevent landing on the wrong line/region.

## Fix Focus Areas
- src/static/js/pad_editor.ts[271-283]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[SamTV12345]

I must say this code is quite a mess. I like the new style with better function naming 😊.

[SamTV12345]

Just fyi. Substr should not be used as it is deprecated. You can use substring which should work in your case identically. You only need to pay attention because the second argument does not anymore mean length but end position. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr

[SamTV12345]

Is this only for chrome? Is it still relevant?