fix: page down/up scrolls by viewport height, not line count

[JohnMcLear]

Summary

• Page down/up counted logical lines in the viewport and scrolled by that count
• When long wrapped lines filled the viewport, only 1-2 logical lines were visible, so page down barely moved
• Now scrolls by actual viewport height in pixels (minus 40px overlap for context)
• Cursor moves to the first/last visible line after scrolling

Test plan

• Type check passes
• Manual: create 3+ very long wrapped lines, press Page Down from the top, verify it scrolls through the content

Fixes #4562

🤖 Generated with Claude Code

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

Review Summary by Qodo

Fix Page Down/Up scrolling with long wrapped lines

🐞 Bug fix

Walkthroughs

Description

• Page Down/Up now scrolls by viewport height in pixels instead of line count
• Fixes issue where long wrapped lines consumed entire viewport, making scrolling ineffective
• Maintains 40px overlap for context preservation between page transitions
• Cursor repositions to first/last visible line after scrolling

Diagram

flowchart LR
  A["Page Down/Up keypress"] --> B["Calculate viewport height in pixels"]
  B --> C["Scroll by viewport height minus 40px overlap"]
  C --> D["Reposition cursor to new visible area"]
  D --> E["Update selection and display"]

Loading

File Changes

1. src/static/js/ace2_inner.ts 🐞 Bug fix +34/-38

Replace line-count scrolling with viewport height scrolling

• Replaced line-count-based scrolling with pixel-height-based scrolling
• Removed complex caret position calculation logic that relied on DOM traversal
• Simplified cursor repositioning to use visible line range boundaries
• Changed scheduler timeout from 200ms to 0ms for immediate cursor updates

src/static/js/ace2_inner.ts

---

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

Code Review by Qodo

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

1. No regression test for #4562 ☑ 📘 Rule violation ☼ Reliability

Description

This PR changes PageUp/PageDown scrolling/caret behavior but does not add an automated regression
test for the long wrapped-line scenario that triggered the bug. Without a targeted test, the issue
can reappear unnoticed in future refactors.

Code

src/static/js/ace2_inner.ts[R2862-2897]

+          if ((isPageDown && padShortcutEnabled.pageDown) ||
+              (isPageUp && padShortcutEnabled.pageUp)) {
+            // Scroll by actual viewport height in pixels, not by line count.
+            // This fixes the case where very long wrapped lines consume the
+            // entire viewport, making line-count-based scrolling useless.
+            const viewportHeight = outerWin.document.documentElement.clientHeight;
+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
       }
-            if (isPageDown && padShortcutEnabled.pageDown) {
-              rep.selStart[0] += numberOfLinesInViewport;
-              rep.selEnd[0] += numberOfLinesInViewport;
-            }
+            // Move cursor into the new visible area
+            scheduler.setTimeout(() => {
+              const linesCount = rep.lines.length();
+              const newVisibleRange = scroll.getVisibleLineRange(rep);
-            // clamp to valid line range
-            rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
-            rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
-            updateBrowserSelectionFromRep();
-            // get the current caret selection, can't use rep. here because that only gives
-            // us the start position not the current
-            const myselection = targetDoc.getSelection();
-            // get the carets selection offset in px IE 214
-            let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
-                myselection.focusNode.offsetTop;
-
-            // sometimes the first selection is -1 which causes problems
-            // (Especially with ep_page_view)
-            // so use focusNode.offsetTop value.
-            if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
-            // set the scrollY offset of the viewport on the document
-            scroll.setScrollY(caretOffsetTop);
-          }, 200);
+              if (isPageDown) {
+                // Place cursor at the first line of the new viewport
+                rep.selStart[0] = newVisibleRange[0];
+                rep.selEnd[0] = newVisibleRange[0];
+              } else {
+                // Place cursor at the last line of the new viewport
+                rep.selEnd[0] = newVisibleRange[1];
+                rep.selStart[0] = newVisibleRange[1];
+              }
+
+              // clamp to valid line range
+              rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
+              rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
+              updateBrowserSelectionFromRep();
+            }, 0);

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test that would fail without the fix.
The PR implements the PageUp/PageDown behavior change in src/static/js/ace2_inner.ts, but the
existing PageUp/PageDown Playwright test only covers many short lines and is not updated to cover
consecutive very long wrapped lines (the #4562 reproduction).

src/static/js/ace2_inner.ts[2854-2897]
src/tests/frontend-new/specs/page_up_down.spec.ts[12-49]
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
This bug fix changes PageUp/PageDown behavior, but the change set does not include a regression test for the reported failure mode (consecutive very long wrapped lines at the top, and the "small edit changes wrapping" variant).
## Issue Context
There is an existing Playwright test for PageUp/PageDown, but it only uses many short lines and does not reproduce #4562 conditions (few logical lines that wrap into many visual lines).
## Fix Focus Areas
- src/tests/frontend-new/specs/page_up_down.spec.ts[1-123]
- src/static/js/ace2_inner.ts[2854-2897]

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

---

2. Bad viewport height access ☑ 🐞 Bug ≡ Correctness

Description

The PageUp/PageDown handler reads outerWin.document.documentElement.clientHeight, but outerWin
is an iframe element (accessed via getElementsByName('ace_outer')[0]) and does not have a
.document property. This will throw at runtime on PageUp/PageDown, breaking paging and cursor
movement.

Code

src/static/js/ace2_inner.ts[2867]

+            const viewportHeight = outerWin.document.documentElement.clientHeight;

Evidence

outerWin is used as an iframe element and its document is accessed via
outerWin.contentWindow.document / outerDoc; there are no other references to outerWin.document
in the repo, so this new access is incorrect and will be undefined at runtime.

src/static/js/ace2_inner.ts[61-70]
src/static/js/ace2_inner.ts[2862-2871]
src/static/js/ace2_inner.ts[3459-3467]

Agent prompt

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

## Issue description
`outerWin.document` is invalid because `outerWin` is the `ace_outer` iframe element. The PageUp/PageDown handler should derive viewport height from the iframe document (`outerDoc`) or reuse the existing `getInnerHeight()` helper.
### Issue Context
`outerWin` is obtained via `document.getElementsByName('ace_outer')[0]` and the codebase consistently uses `outerWin.contentWindow.document` (aliased as `outerDoc`) to access the iframe document.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2862-2871]
- src/static/js/ace2_inner.ts[61-70]
- src/static/js/ace2_inner.ts[3459-3467]
### Suggested change
Replace:
- `const viewportHeight = outerWin.document.documentElement.clientHeight;`
With one of:
- `const viewportHeight = outerDoc.documentElement.clientHeight;` (consistent with existing code), or
- `const viewportHeight = getInnerHeight();` (already handles hidden iframe cases).

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

---

3. Off-by-one visible line count ☑ 🐞 Bug ≡ Correctness

Description

visibleLogicalLines is computed as visibleEnd - visibleStart even though
scroll.getVisibleLineRange() returns an inclusive end index. This undercounts visible logical
lines (and therefore the page increment) by 1 in the common case.

Code

src/static/js/ace2_inner.ts[R2874-2887]

+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;
+            for (let i = visibleStart; i <= Math.min(visibleEnd, linesCount - 1); i++) {
+              const entry = rep.lines.atIndex(i);
+              if (entry && entry.lineNode) {
+                totalPixelHeight += entry.lineNode.offsetHeight;
+              }
+            }
+            const visibleLogicalLines = visibleEnd - visibleStart;
+            // Use pixel-based count: how many logical lines fit in one viewport
+            const numberOfLinesInViewport = visibleLogicalLines > 0 && totalPixelHeight > 0
+                ? Math.max(1, Math.round(visibleLogicalLines * viewportHeight / totalPixelHeight))
+                : Math.max(1, visibleLogicalLines);

Evidence

scroll.getVisibleLineRange() returns [start, end - 1], so the second element is the last visible
line index (inclusive). Therefore, the count of visible logical lines is `visibleEnd - visibleStart
+ 1, not visibleEnd - visibleStart` as used in the new computation.

src/static/js/scroll.ts[317-330]
src/static/js/ace2_inner.ts[2866-2887]

Agent prompt

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

## Issue description
`scroll.getVisibleLineRange()` returns an inclusive end index, but `visibleLogicalLines` is calculated as an exclusive range. This causes PageUp/PageDown to move by too few logical lines.
### Issue Context
The returned range is `[start, endInclusive]` (see `return [start, end - 1]`).
### Fix
Adjust the visible logical line count to be inclusive:
- `const visibleLogicalLines = visibleEnd - visibleStart + 1;`
Also review any other math that assumes an exclusive end.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2873-2887]
- src/static/js/scroll.ts[317-330]

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

---

View more (8)

4. No regression test for #4562 ☑ 📘 Rule violation ☼ Reliability

Description

This PR changes PageUp/PageDown scrolling/caret behavior but does not add an automated regression
test for the long wrapped-line scenario that triggered the bug. Without a targeted test, the issue
can reappear unnoticed in future refactors.

Code

src/static/js/ace2_inner.ts[R2862-2897]

+          if ((isPageDown && padShortcutEnabled.pageDown) ||
+              (isPageUp && padShortcutEnabled.pageUp)) {
+            // Scroll by actual viewport height in pixels, not by line count.
+            // This fixes the case where very long wrapped lines consume the
+            // entire viewport, making line-count-based scrolling useless.
+            const viewportHeight = outerWin.document.documentElement.clientHeight;
+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
      }
-            if (isPageDown && padShortcutEnabled.pageDown) {
-              rep.selStart[0] += numberOfLinesInViewport;
-              rep.selEnd[0] += numberOfLinesInViewport;
-            }
+            // Move cursor into the new visible area
+            scheduler.setTimeout(() => {
+              const linesCount = rep.lines.length();
+              const newVisibleRange = scroll.getVisibleLineRange(rep);
-            // clamp to valid line range
-            rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
-            rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
-            updateBrowserSelectionFromRep();
-            // get the current caret selection, can't use rep. here because that only gives
-            // us the start position not the current
-            const myselection = targetDoc.getSelection();
-            // get the carets selection offset in px IE 214
-            let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
-                myselection.focusNode.offsetTop;
-
-            // sometimes the first selection is -1 which causes problems
-            // (Especially with ep_page_view)
-            // so use focusNode.offsetTop value.
-            if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
-            // set the scrollY offset of the viewport on the document
-            scroll.setScrollY(caretOffsetTop);
-          }, 200);
+              if (isPageDown) {
+                // Place cursor at the first line of the new viewport
+                rep.selStart[0] = newVisibleRange[0];
+                rep.selEnd[0] = newVisibleRange[0];
+              } else {
+                // Place cursor at the last line of the new viewport
+                rep.selEnd[0] = newVisibleRange[1];
+                rep.selStart[0] = newVisibleRange[1];
+              }
+
+              // clamp to valid line range
+              rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
+              rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
+              updateBrowserSelectionFromRep();
+            }, 0);

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test that would fail without the fix.
The PR implements the PageUp/PageDown behavior change in src/static/js/ace2_inner.ts, but the
existing PageUp/PageDown Playwright test only covers many short lines and is not updated to cover
consecutive very long wrapped lines (the #4562 reproduction).

src/static/js/ace2_inner.ts[2854-2897]
src/tests/frontend-new/specs/page_up_down.spec.ts[12-49]
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
This bug fix changes PageUp/PageDown behavior, but the change set does not include a regression test for the reported failure mode (consecutive very long wrapped lines at the top, and the "small edit changes wrapping" variant).
## Issue Context
There is an existing Playwright test for PageUp/PageDown, but it only uses many short lines and does not reproduce #4562 conditions (few logical lines that wrap into many visual lines).
## Fix Focus Areas
- src/tests/frontend-new/specs/page_up_down.spec.ts[1-123]
- src/static/js/ace2_inner.ts[2854-2897]

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

---

5. Bad viewport height access ☑ 🐞 Bug ≡ Correctness

Description

The PageUp/PageDown handler reads outerWin.document.documentElement.clientHeight, but outerWin
is an iframe element (accessed via getElementsByName('ace_outer')[0]) and does not have a
.document property. This will throw at runtime on PageUp/PageDown, breaking paging and cursor
movement.

Code

src/static/js/ace2_inner.ts[2867]

+            const viewportHeight = outerWin.document.documentElement.clientHeight;

Evidence

outerWin is used as an iframe element and its document is accessed via
outerWin.contentWindow.document / outerDoc; there are no other references to outerWin.document
in the repo, so this new access is incorrect and will be undefined at runtime.

src/static/js/ace2_inner.ts[61-70]
src/static/js/ace2_inner.ts[2862-2871]
src/static/js/ace2_inner.ts[3459-3467]

Agent prompt

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

## Issue description
`outerWin.document` is invalid because `outerWin` is the `ace_outer` iframe element. The PageUp/PageDown handler should derive viewport height from the iframe document (`outerDoc`) or reuse the existing `getInnerHeight()` helper.
### Issue Context
`outerWin` is obtained via `document.getElementsByName('ace_outer')[0]` and the codebase consistently uses `outerWin.contentWindow.document` (aliased as `outerDoc`) to access the iframe document.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2862-2871]
- src/static/js/ace2_inner.ts[61-70]
- src/static/js/ace2_inner.ts[3459-3467]
### Suggested change
Replace:
- `const viewportHeight = outerWin.document.documentElement.clientHeight;`
With one of:
- `const viewportHeight = outerDoc.documentElement.clientHeight;` (consistent with existing code), or
- `const viewportHeight = getInnerHeight();` (already handles hidden iframe cases).

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

---

6. Off-by-one visible line count 🐞 Bug ≡ Correctness

Description

visibleLogicalLines is computed as visibleEnd - visibleStart even though
scroll.getVisibleLineRange() returns an inclusive end index. This undercounts visible logical
lines (and therefore the page increment) by 1 in the common case.

Code

src/static/js/ace2_inner.ts[R2874-2887]

+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;
+            for (let i = visibleStart; i <= Math.min(visibleEnd, linesCount - 1); i++) {
+              const entry = rep.lines.atIndex(i);
+              if (entry && entry.lineNode) {
+                totalPixelHeight += entry.lineNode.offsetHeight;
+              }
+            }
+            const visibleLogicalLines = visibleEnd - visibleStart;
+            // Use pixel-based count: how many logical lines fit in one viewport
+            const numberOfLinesInViewport = visibleLogicalLines > 0 && totalPixelHeight > 0
+                ? Math.max(1, Math.round(visibleLogicalLines * viewportHeight / totalPixelHeight))
+                : Math.max(1, visibleLogicalLines);

Evidence

scroll.getVisibleLineRange() returns [start, end - 1], so the second element is the last visible
line index (inclusive). Therefore, the count of visible logical lines is `visibleEnd - visibleStart
+ 1, not visibleEnd - visibleStart` as used in the new computation.

src/static/js/scroll.ts[317-330]
src/static/js/ace2_inner.ts[2866-2887]

Agent prompt

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

## Issue description
`scroll.getVisibleLineRange()` returns an inclusive end index, but `visibleLogicalLines` is calculated as an exclusive range. This causes PageUp/PageDown to move by too few logical lines.
### Issue Context
The returned range is `[start, endInclusive]` (see `return [start, end - 1]`).
### Fix
Adjust the visible logical line count to be inclusive:
- `const visibleLogicalLines = visibleEnd - visibleStart + 1;`
Also review any other math that assumes an exclusive end.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2873-2887]
- src/static/js/scroll.ts[317-330]

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

---

7. No regression test for #4562 ☑ 📘 Rule violation ☼ Reliability

Description

This PR changes PageUp/PageDown scrolling/caret behavior but does not add an automated regression
test for the long wrapped-line scenario that triggered the bug. Without a targeted test, the issue
can reappear unnoticed in future refactors.

Code

src/static/js/ace2_inner.ts[R2862-2897]

+          if ((isPageDown && padShortcutEnabled.pageDown) ||
+              (isPageUp && padShortcutEnabled.pageUp)) {
+            // Scroll by actual viewport height in pixels, not by line count.
+            // This fixes the case where very long wrapped lines consume the
+            // entire viewport, making line-count-based scrolling useless.
+            const viewportHeight = outerWin.document.documentElement.clientHeight;
+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
       }
-            if (isPageDown && padShortcutEnabled.pageDown) {
-              rep.selStart[0] += numberOfLinesInViewport;
-              rep.selEnd[0] += numberOfLinesInViewport;
-            }
+            // Move cursor into the new visible area
+            scheduler.setTimeout(() => {
+              const linesCount = rep.lines.length();
+              const newVisibleRange = scroll.getVisibleLineRange(rep);
-            // clamp to valid line range
-            rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
-            rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
-            updateBrowserSelectionFromRep();
-            // get the current caret selection, can't use rep. here because that only gives
-            // us the start position not the current
-            const myselection = targetDoc.getSelection();
-            // get the carets selection offset in px IE 214
-            let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
-                myselection.focusNode.offsetTop;
-
-            // sometimes the first selection is -1 which causes problems
-            // (Especially with ep_page_view)
-            // so use focusNode.offsetTop value.
-            if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
-            // set the scrollY offset of the viewport on the document
-            scroll.setScrollY(caretOffsetTop);
-          }, 200);
+              if (isPageDown) {
+                // Place cursor at the first line of the new viewport
+                rep.selStart[0] = newVisibleRange[0];
+                rep.selEnd[0] = newVisibleRange[0];
+              } else {
+                // Place cursor at the last line of the new viewport
+                rep.selEnd[0] = newVisibleRange[1];
+                rep.selStart[0] = newVisibleRange[1];
+              }
+
+              // clamp to valid line range
+              rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
+              rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
+              updateBrowserSelectionFromRep();
+            }, 0);

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test that would fail without the fix.
The PR implements the PageUp/PageDown behavior change in src/static/js/ace2_inner.ts, but the
existing PageUp/PageDown Playwright test only covers many short lines and is not updated to cover
consecutive very long wrapped lines (the #4562 reproduction).

src/static/js/ace2_inner.ts[2854-2897]
src/tests/frontend-new/specs/page_up_down.spec.ts[12-49]
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
This bug fix changes PageUp/PageDown behavior, but the change set does not include a regression test for the reported failure mode (consecutive very long wrapped lines at the top, and the "small edit changes wrapping" variant).
## Issue Context
There is an existing Playwright test for PageUp/PageDown, but it only uses many short lines and does not reproduce #4562 conditions (few logical lines that wrap into many visual lines).
## Fix Focus Areas
- src/tests/frontend-new/specs/page_up_down.spec.ts[1-123]
- src/static/js/ace2_inner.ts[2854-2897]

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

---

8. Bad viewport height access ☑ 🐞 Bug ≡ Correctness

Description

The PageUp/PageDown handler reads outerWin.document.documentElement.clientHeight, but outerWin
is an iframe element (accessed via getElementsByName('ace_outer')[0]) and does not have a
.document property. This will throw at runtime on PageUp/PageDown, breaking paging and cursor
movement.

Code

src/static/js/ace2_inner.ts[2867]

+            const viewportHeight = outerWin.document.documentElement.clientHeight;

Evidence

outerWin is used as an iframe element and its document is accessed via
outerWin.contentWindow.document / outerDoc; there are no other references to outerWin.document
in the repo, so this new access is incorrect and will be undefined at runtime.

src/static/js/ace2_inner.ts[61-70]
src/static/js/ace2_inner.ts[2862-2871]
src/static/js/ace2_inner.ts[3459-3467]

Agent prompt

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

## Issue description
`outerWin.document` is invalid because `outerWin` is the `ace_outer` iframe element. The PageUp/PageDown handler should derive viewport height from the iframe document (`outerDoc`) or reuse the existing `getInnerHeight()` helper.
### Issue Context
`outerWin` is obtained via `document.getElementsByName('ace_outer')[0]` and the codebase consistently uses `outerWin.contentWindow.document` (aliased as `outerDoc`) to access the iframe document.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2862-2871]
- src/static/js/ace2_inner.ts[61-70]
- src/static/js/ace2_inner.ts[3459-3467]
### Suggested change
Replace:
- `const viewportHeight = outerWin.document.documentElement.clientHeight;`
With one of:
- `const viewportHeight = outerDoc.documentElement.clientHeight;` (consistent with existing code), or
- `const viewportHeight = getInnerHeight();` (already handles hidden iframe cases).

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

---

9. Off-by-one visible line count 🐞 Bug ≡ Correctness

Description

visibleLogicalLines is computed as visibleEnd - visibleStart even though
scroll.getVisibleLineRange() returns an inclusive end index. This undercounts visible logical
lines (and therefore the page increment) by 1 in the common case.

Code

src/static/js/ace2_inner.ts[R2874-2887]

+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;
+            for (let i = visibleStart; i <= Math.min(visibleEnd, linesCount - 1); i++) {
+              const entry = rep.lines.atIndex(i);
+              if (entry && entry.lineNode) {
+                totalPixelHeight += entry.lineNode.offsetHeight;
+              }
+            }
+            const visibleLogicalLines = visibleEnd - visibleStart;
+            // Use pixel-based count: how many logical lines fit in one viewport
+            const numberOfLinesInViewport = visibleLogicalLines > 0 && totalPixelHeight > 0
+                ? Math.max(1, Math.round(visibleLogicalLines * viewportHeight / totalPixelHeight))
+                : Math.max(1, visibleLogicalLines);

Evidence

scroll.getVisibleLineRange() returns [start, end - 1], so the second element is the last visible
line index (inclusive). Therefore, the count of visible logical lines is `visibleEnd - visibleStart
+ 1, not visibleEnd - visibleStart` as used in the new computation.

src/static/js/scroll.ts[317-330]
src/static/js/ace2_inner.ts[2866-2887]

Agent prompt

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

## Issue description
`scroll.getVisibleLineRange()` returns an inclusive end index, but `visibleLogicalLines` is calculated as an exclusive range. This causes PageUp/PageDown to move by too few logical lines.
### Issue Context
The returned range is `[start, endInclusive]` (see `return [start, end - 1]`).
### Fix
Adjust the visible logical line count to be inclusive:
- `const visibleLogicalLines = visibleEnd - visibleStart + 1;`
Also review any other math that assumes an exclusive end.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2873-2887]
- src/static/js/scroll.ts[317-330]

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

---

10. No regression test for #4562 ☑ 📘 Rule violation ☼ Reliability

Description

This PR changes PageUp/PageDown scrolling/caret behavior but does not add an automated regression
test for the long wrapped-line scenario that triggered the bug. Without a targeted test, the issue
can reappear unnoticed in future refactors.

Code

src/static/js/ace2_inner.ts[R2862-2897]

+          if ((isPageDown && padShortcutEnabled.pageDown) ||
+              (isPageUp && padShortcutEnabled.pageUp)) {
+            // Scroll by actual viewport height in pixels, not by line count.
+            // This fixes the case where very long wrapped lines consume the
+            // entire viewport, making line-count-based scrolling useless.
+            const viewportHeight = outerWin.document.documentElement.clientHeight;
+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
        }
-            if (isPageDown && padShortcutEnabled.pageDown) {
-              rep.selStart[0] += numberOfLinesInViewport;
-              rep.selEnd[0] += numberOfLinesInViewport;
-            }
+            // Move cursor into the new visible area
+            scheduler.setTimeout(() => {
+              const linesCount = rep.lines.length();
+              const newVisibleRange = scroll.getVisibleLineRange(rep);
-            // clamp to valid line range
-            rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
-            rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
-            updateBrowserSelectionFromRep();
-            // get the current caret selection, can't use rep. here because that only gives
-            // us the start position not the current
-            const myselection = targetDoc.getSelection();
-            // get the carets selection offset in px IE 214
-            let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
-                myselection.focusNode.offsetTop;
-
-            // sometimes the first selection is -1 which causes problems
-            // (Especially with ep_page_view)
-            // so use focusNode.offsetTop value.
-            if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
-            // set the scrollY offset of the viewport on the document
-            scroll.setScrollY(caretOffsetTop);
-          }, 200);
+              if (isPageDown) {
+                // Place cursor at the first line of the new viewport
+                rep.selStart[0] = newVisibleRange[0];
+                rep.selEnd[0] = newVisibleRange[0];
+              } else {
+                // Place cursor at the last line of the new viewport
+                rep.selEnd[0] = newVisibleRange[1];
+                rep.selStart[0] = newVisibleRange[1];
+              }
+
+              // clamp to valid line range
+              rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
+              rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
+              updateBrowserSelectionFromRep();
+            }, 0);

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test that would fail without the fix.
The PR implements the PageUp/PageDown behavior change in src/static/js/ace2_inner.ts, but the
existing PageUp/PageDown Playwright test only covers many short lines and is not updated to cover
consecutive very long wrapped lines (the #4562 reproduction).

src/static/js/ace2_inner.ts[2854-2897]
src/tests/frontend-new/specs/page_up_down.spec.ts[12-49]
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
This bug fix changes PageUp/PageDown behavior, but the change set does not include a regression test for the reported failure mode (consecutive very long wrapped lines at the top, and the "small edit changes wrapping" variant).
## Issue Context
There is an existing Playwright test for PageUp/PageDown, but it only uses many short lines and does not reproduce #4562 conditions (few logical lines that wrap into many visual lines).
## Fix Focus Areas
- src/tests/frontend-new/specs/page_up_down.spec.ts[1-123]
- src/static/js/ace2_inner.ts[2854-2897]

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

---

11. Bad viewport height access ☑ 🐞 Bug ≡ Correctness

Description

The PageUp/PageDown handler reads outerWin.document.documentElement.clientHeight, but outerWin
is an iframe element (accessed via getElementsByName('ace_outer')[0]) and does not have a
.document property. This will throw at runtime on PageUp/PageDown, breaking paging and cursor
movement.

Code

src/static/js/ace2_inner.ts[2867]

+            const viewportHeight = outerWin.document.documentElement.clientHeight;

Evidence

outerWin is used as an iframe element and its document is accessed via
outerWin.contentWindow.document / outerDoc; there are no other references to outerWin.document
in the repo, so this new access is incorrect and will be undefined at runtime.

src/static/js/ace2_inner.ts[61-70]
src/static/js/ace2_inner.ts[2862-2871]
src/static/js/ace2_inner.ts[3459-3467]

Agent prompt

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

## Issue description
`outerWin.document` is invalid because `outerWin` is the `ace_outer` iframe element. The PageUp/PageDown handler should derive viewport height from the iframe document (`outerDoc`) or reuse the existing `getInnerHeight()` helper.
### Issue Context
`outerWin` is obtained via `document.getElementsByName('ace_outer')[0]` and the codebase consistently uses `outerWin.contentWindow.document` (aliased as `outerDoc`) to access the iframe document.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2862-2871]
- src/static/js/ace2_inner.ts[61-70]
- src/static/js/ace2_inner.ts[3459-3467]
### Suggested change
Replace:
- `const viewportHeight = outerWin.document.documentElement.clientHeight;`
With one of:
- `const viewportHeight = outerDoc.documentElement.clientHeight;` (consistent with existing code), or
- `const viewportHeight = getInnerHeight();` (already handles hidden iframe cases).

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

---

12. Negative scroll delta possible ☑ 🐞 Bug ☼ Reliability

Description

scrollAmount is computed as viewportHeight - 40 with no clamp, so a small/hidden viewport can
yield a negative value and invert PageUp/PageDown direction (PageUp scrolls down and PageDown
scrolls up). This can happen in the already-documented “iframe is hidden” case where height can be
0.

Code

src/static/js/ace2_inner.ts[R2868-2876]

+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
       }

Evidence

The handler directly subtracts 40px from the viewport height and then subtracts that amount for
PageUp; if the viewport height is ≤40 (including the hidden-iframe case documented in
getInnerHeight()), the computed scroll delta becomes negative and reverses scroll direction.

src/static/js/ace2_inner.ts[2867-2876]
src/static/js/ace2_inner.ts[3460-3467]

Agent prompt

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

## Issue description
`scrollAmount = viewportHeight - 40` can be negative when the viewport is small or hidden (height 0), which inverts PageUp/PageDown behavior.
### Issue Context
There is already code that anticipates hidden iframe sizing (returning 0 if no explicit px height is set). Page navigation should remain directionally correct even in these cases.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2867-2876]
- src/static/js/ace2_inner.ts[3460-3467]
### Suggested change
After computing `viewportHeight`, clamp:
- `const scrollAmount = Math.max(0, viewportHeight - 40);`
And optionally bail out if it is 0 (to avoid calling `setScrollY` with unchanged/inverted values).

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

---

13. Viewport height miscomputed 🐞 Bug ≡ Correctness

Description

The pixel-based page size uses getInnerHeight() as the viewport height, but the scroll module’s
viewport calculation subtracts editor offset/padding before determining what is visible. This
mismatch can inflate the computed viewport height and make PageUp/PageDown skip too many lines.

Code

src/static/js/ace2_inner.ts[R2870-2876]

+            // Calculate lines to skip based on viewport pixel height divided by
+            // the average rendered line height. This correctly handles long wrapped
+            // lines that consume multiple visual rows (fixes #4562).
+            const viewportHeight = getInnerHeight();
+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;

Evidence

scroll.getVisibleLineRange() is based on _getViewPortTopBottom(), which uses
doc.documentElement.clientHeight minus editor position top and page-view padding. The new code’s
viewportHeight = getInnerHeight() uses outerDoc.documentElement.clientHeight without subtracting
those extras, so the height used in the scaling calculation is not the same height used to decide
which lines are visible.

src/static/js/scroll.ts[98-112]
src/static/js/scroll.ts[114-118]
src/static/js/ace2_inner.ts[3471-3478]
src/static/js/ace2_inner.ts[2866-2876]

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 viewport height used for the PageUp/PageDown pixel math (`getInnerHeight()`) does not match the viewport height used by `scroll.getVisibleLineRange()` (which subtracts editor offset/padding). This can cause the computed page size to be larger than the actual visible area.
### Issue Context
- Visible range uses `Scroll._getViewPortTopBottom()`.
- New paging math uses `getInnerHeight()`.
### Fix
Compute `viewportHeight` using the same logic as the scroll module, e.g.:
- Add a public method on `Scroll` such as `getViewportHeight()` / `getViewportTopBottom()` and use `bottom - top`.
- (Prefer this over calling private methods.)
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2870-2887]
- src/static/js/scroll.ts[98-112]

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

---

14. Negative scroll delta possible ☑ 🐞 Bug ☼ Reliability

Description

scrollAmount is computed as viewportHeight - 40 with no clamp, so a small/hidden viewport can
yield a negative value and invert PageUp/PageDown direction (PageUp scrolls down and PageDown
scrolls up). This can happen in the already-documented “iframe is hidden” case where height can be
0.

Code

src/static/js/ace2_inner.ts[R2868-2876]

+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
      }

Evidence

The handler directly subtracts 40px from the viewport height and then subtracts that amount for
PageUp; if the viewport height is ≤40 (including the hidden-iframe case documented in
getInnerHeight()), the computed scroll delta becomes negative and reverses scroll direction.

src/static/js/ace2_inner.ts[2867-2876]
src/static/js/ace2_inner.ts[3460-3467]

Agent prompt

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

## Issue description
`scrollAmount = viewportHeight - 40` can be negative when the viewport is small or hidden (height 0), which inverts PageUp/PageDown behavior.
### Issue Context
There is already code that anticipates hidden iframe sizing (returning 0 if no explicit px height is set). Page navigation should remain directionally correct even in these cases.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2867-2876]
- src/static/js/ace2_inner.ts[3460-3467]
### Suggested change
After computing `viewportHeight`, clamp:
- `const scrollAmount = Math.max(0, viewportHeight - 40);`
And optionally bail out if it is 0 (to avoid calling `setScrollY` with unchanged/inverted values).

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

---

View more (4)

15. Viewport height miscomputed 🐞 Bug ≡ Correctness

Description

The pixel-based page size uses getInnerHeight() as the viewport height, but the scroll module’s
viewport calculation subtracts editor offset/padding before determining what is visible. This
mismatch can inflate the computed viewport height and make PageUp/PageDown skip too many lines.

Code

src/static/js/ace2_inner.ts[R2870-2876]

+            // Calculate lines to skip based on viewport pixel height divided by
+            // the average rendered line height. This correctly handles long wrapped
+            // lines that consume multiple visual rows (fixes #4562).
+            const viewportHeight = getInnerHeight();
+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;

Evidence

scroll.getVisibleLineRange() is based on _getViewPortTopBottom(), which uses
doc.documentElement.clientHeight minus editor position top and page-view padding. The new code’s
viewportHeight = getInnerHeight() uses outerDoc.documentElement.clientHeight without subtracting
those extras, so the height used in the scaling calculation is not the same height used to decide
which lines are visible.

src/static/js/scroll.ts[98-112]
src/static/js/scroll.ts[114-118]
src/static/js/ace2_inner.ts[3471-3478]
src/static/js/ace2_inner.ts[2866-2876]

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 viewport height used for the PageUp/PageDown pixel math (`getInnerHeight()`) does not match the viewport height used by `scroll.getVisibleLineRange()` (which subtracts editor offset/padding). This can cause the computed page size to be larger than the actual visible area.
### Issue Context
- Visible range uses `Scroll._getViewPortTopBottom()`.
- New paging math uses `getInnerHeight()`.
### Fix
Compute `viewportHeight` using the same logic as the scroll module, e.g.:
- Add a public method on `Scroll` such as `getViewportHeight()` / `getViewportTopBottom()` and use `bottom - top`.
- (Prefer this over calling private methods.)
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2870-2887]
- src/static/js/scroll.ts[98-112]

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

---

16. Negative scroll delta possible ☑ 🐞 Bug ☼ Reliability

Description

scrollAmount is computed as viewportHeight - 40 with no clamp, so a small/hidden viewport can
yield a negative value and invert PageUp/PageDown direction (PageUp scrolls down and PageDown
scrolls up). This can happen in the already-documented “iframe is hidden” case where height can be
0.

Code

src/static/js/ace2_inner.ts[R2868-2876]

+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
       }

Evidence

The handler directly subtracts 40px from the viewport height and then subtracts that amount for
PageUp; if the viewport height is ≤40 (including the hidden-iframe case documented in
getInnerHeight()), the computed scroll delta becomes negative and reverses scroll direction.

src/static/js/ace2_inner.ts[2867-2876]
src/static/js/ace2_inner.ts[3460-3467]

Agent prompt

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

## Issue description
`scrollAmount = viewportHeight - 40` can be negative when the viewport is small or hidden (height 0), which inverts PageUp/PageDown behavior.
### Issue Context
There is already code that anticipates hidden iframe sizing (returning 0 if no explicit px height is set). Page navigation should remain directionally correct even in these cases.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2867-2876]
- src/static/js/ace2_inner.ts[3460-3467]
### Suggested change
After computing `viewportHeight`, clamp:
- `const scrollAmount = Math.max(0, viewportHeight - 40);`
And optionally bail out if it is 0 (to avoid calling `setScrollY` with unchanged/inverted values).

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

---

17. Viewport height miscomputed 🐞 Bug ≡ Correctness

Description

The pixel-based page size uses getInnerHeight() as the viewport height, but the scroll module’s
viewport calculation subtracts editor offset/padding before determining what is visible. This
mismatch can inflate the computed viewport height and make PageUp/PageDown skip too many lines.

Code

src/static/js/ace2_inner.ts[R2870-2876]

+            // Calculate lines to skip based on viewport pixel height divided by
+            // the average rendered line height. This correctly handles long wrapped
+            // lines that consume multiple visual rows (fixes #4562).
+            const viewportHeight = getInnerHeight();
+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;

Evidence

scroll.getVisibleLineRange() is based on _getViewPortTopBottom(), which uses
doc.documentElement.clientHeight minus editor position top and page-view padding. The new code’s
viewportHeight = getInnerHeight() uses outerDoc.documentElement.clientHeight without subtracting
those extras, so the height used in the scaling calculation is not the same height used to decide
which lines are visible.

src/static/js/scroll.ts[98-112]
src/static/js/scroll.ts[114-118]
src/static/js/ace2_inner.ts[3471-3478]
src/static/js/ace2_inner.ts[2866-2876]

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 viewport height used for the PageUp/PageDown pixel math (`getInnerHeight()`) does not match the viewport height used by `scroll.getVisibleLineRange()` (which subtracts editor offset/padding). This can cause the computed page size to be larger than the actual visible area.
### Issue Context
- Visible range uses `Scroll._getViewPortTopBottom()`.
- New paging math uses `getInnerHeight()`.
### Fix
Compute `viewportHeight` using the same logic as the scroll module, e.g.:
- Add a public method on `Scroll` such as `getViewportHeight()` / `getViewportTopBottom()` and use `bottom - top`.
- (Prefer this over calling private methods.)
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2870-2887]
- src/static/js/scroll.ts[98-112]

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

---

18. Negative scroll delta possible ☑ 🐞 Bug ☼ Reliability

Description

scroll...

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

Review Summary by Qodo

Fix PageDown/PageUp scrolling with long wrapped lines

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Fix PageDown/PageUp to scroll by viewport pixel height instead of logical line count
• Calculate lines-to-skip using actual rendered line heights for wrapped lines
• Add regression test for consecutive long wrapped lines (#4562)
• Simplify caret positioning logic and remove outdated comments

Diagram

flowchart LR
  A["PageDown/PageUp Event"] --> B["Calculate viewport height in pixels"]
  B --> C["Sum rendered line heights in viewport"]
  C --> D["Compute lines-to-skip ratio"]
  D --> E["Move cursor by calculated lines"]
  E --> F["Scroll to caret position"]
  G["Regression Test"] --> H["Create pad with long wrapped lines"]
  H --> I["Verify PageDown advances cursor correctly"]

Loading

File Changes

1. src/static/js/ace2_inner.ts 🐞 Bug fix +24/-17

Implement pixel-based line calculation for PageDown/PageUp

• Replace logical line counting with pixel-based viewport height calculation
• Iterate through visible lines to sum actual rendered heights via offsetHeight
• Calculate numberOfLinesInViewport as ratio of viewport height to average line height
• Simplify caret positioning code by removing redundant comments and consolidating logic

src/static/js/ace2_inner.ts

---

2. src/tests/frontend-new/specs/page_up_down.spec.ts 🧪 Tests +60/-0

Add regression test for long wrapped lines

• Add regression test for #4562 with consecutive long wrapped lines
• Build test pad with alternating long and short lines via DOM manipulation
• Verify PageDown advances caret position correctly on first and second presses
• Use getCaretLine() helper to track caret movement across multiple PageDown events

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

---

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

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. Off-by-one visible line count 🐞 Bug ≡ Correctness ⭐ New

Description

visibleLogicalLines is computed as visibleEnd - visibleStart even though
scroll.getVisibleLineRange() returns an inclusive end index. This undercounts visible logical
lines (and therefore the page increment) by 1 in the common case.

Code

src/static/js/ace2_inner.ts[R2874-2887]

+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;
+            for (let i = visibleStart; i <= Math.min(visibleEnd, linesCount - 1); i++) {
+              const entry = rep.lines.atIndex(i);
+              if (entry && entry.lineNode) {
+                totalPixelHeight += entry.lineNode.offsetHeight;
+              }
+            }
+            const visibleLogicalLines = visibleEnd - visibleStart;
+            // Use pixel-based count: how many logical lines fit in one viewport
+            const numberOfLinesInViewport = visibleLogicalLines > 0 && totalPixelHeight > 0
+                ? Math.max(1, Math.round(visibleLogicalLines * viewportHeight / totalPixelHeight))
+                : Math.max(1, visibleLogicalLines);

Evidence

scroll.getVisibleLineRange() returns [start, end - 1], so the second element is the last visible
line index (inclusive). Therefore, the count of visible logical lines is `visibleEnd - visibleStart
+ 1, not visibleEnd - visibleStart` as used in the new computation.

src/static/js/scroll.ts[317-330]
src/static/js/ace2_inner.ts[2866-2887]

Agent prompt

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

### Issue description
`scroll.getVisibleLineRange()` returns an inclusive end index, but `visibleLogicalLines` is calculated as an exclusive range. This causes PageUp/PageDown to move by too few logical lines.

### Issue Context
The returned range is `[start, endInclusive]` (see `return [start, end - 1]`).

### Fix
Adjust the visible logical line count to be inclusive:
- `const visibleLogicalLines = visibleEnd - visibleStart + 1;`
Also review any other math that assumes an exclusive end.

### Fix Focus Areas
- src/static/js/ace2_inner.ts[2873-2887]
- src/static/js/scroll.ts[317-330]

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

---

2. No regression test for #4562 ☑ 📘 Rule violation ☼ Reliability

Description

This PR changes PageUp/PageDown scrolling/caret behavior but does not add an automated regression
test for the long wrapped-line scenario that triggered the bug. Without a targeted test, the issue
can reappear unnoticed in future refactors.

Code

src/static/js/ace2_inner.ts[R2862-2897]

+          if ((isPageDown && padShortcutEnabled.pageDown) ||
+              (isPageUp && padShortcutEnabled.pageUp)) {
+            // Scroll by actual viewport height in pixels, not by line count.
+            // This fixes the case where very long wrapped lines consume the
+            // entire viewport, making line-count-based scrolling useless.
+            const viewportHeight = outerWin.document.documentElement.clientHeight;
+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
         }
-            if (isPageDown && padShortcutEnabled.pageDown) {
-              rep.selStart[0] += numberOfLinesInViewport;
-              rep.selEnd[0] += numberOfLinesInViewport;
-            }
+            // Move cursor into the new visible area
+            scheduler.setTimeout(() => {
+              const linesCount = rep.lines.length();
+              const newVisibleRange = scroll.getVisibleLineRange(rep);
-            // clamp to valid line range
-            rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
-            rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
-            updateBrowserSelectionFromRep();
-            // get the current caret selection, can't use rep. here because that only gives
-            // us the start position not the current
-            const myselection = targetDoc.getSelection();
-            // get the carets selection offset in px IE 214
-            let caretOffsetTop = myselection.focusNode.parentNode.offsetTop ||
-                myselection.focusNode.offsetTop;
-
-            // sometimes the first selection is -1 which causes problems
-            // (Especially with ep_page_view)
-            // so use focusNode.offsetTop value.
-            if (caretOffsetTop === -1) caretOffsetTop = myselection.focusNode.offsetTop;
-            // set the scrollY offset of the viewport on the document
-            scroll.setScrollY(caretOffsetTop);
-          }, 200);
+              if (isPageDown) {
+                // Place cursor at the first line of the new viewport
+                rep.selStart[0] = newVisibleRange[0];
+                rep.selEnd[0] = newVisibleRange[0];
+              } else {
+                // Place cursor at the last line of the new viewport
+                rep.selEnd[0] = newVisibleRange[1];
+                rep.selStart[0] = newVisibleRange[1];
+              }
+
+              // clamp to valid line range
+              rep.selStart[0] = Math.max(0, Math.min(rep.selStart[0], linesCount - 1));
+              rep.selEnd[0] = Math.max(0, Math.min(rep.selEnd[0], linesCount - 1));
+              updateBrowserSelectionFromRep();
+            }, 0);

Evidence

PR Compliance ID 3 requires bug fixes to include a regression test that would fail without the fix.
The PR implements the PageUp/PageDown behavior change in src/static/js/ace2_inner.ts, but the
existing PageUp/PageDown Playwright test only covers many short lines and is not updated to cover
consecutive very long wrapped lines (the #4562 reproduction).

src/static/js/ace2_inner.ts[2854-2897]
src/tests/frontend-new/specs/page_up_down.spec.ts[12-49]
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
This bug fix changes PageUp/PageDown behavior, but the change set does not include a regression test for the reported failure mode (consecutive very long wrapped lines at the top, and the "small edit changes wrapping" variant).
## Issue Context
There is an existing Playwright test for PageUp/PageDown, but it only uses many short lines and does not reproduce #4562 conditions (few logical lines that wrap into many visual lines).
## Fix Focus Areas
- src/tests/frontend-new/specs/page_up_down.spec.ts[1-123]
- src/static/js/ace2_inner.ts[2854-2897]

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

---

3. Bad viewport height access ☑ 🐞 Bug ≡ Correctness

Description

The PageUp/PageDown handler reads outerWin.document.documentElement.clientHeight, but outerWin
is an iframe element (accessed via getElementsByName('ace_outer')[0]) and does not have a
.document property. This will throw at runtime on PageUp/PageDown, breaking paging and cursor
movement.

Code

src/static/js/ace2_inner.ts[2867]

+            const viewportHeight = outerWin.document.documentElement.clientHeight;

Evidence

outerWin is used as an iframe element and its document is accessed via
outerWin.contentWindow.document / outerDoc; there are no other references to outerWin.document
in the repo, so this new access is incorrect and will be undefined at runtime.

src/static/js/ace2_inner.ts[61-70]
src/static/js/ace2_inner.ts[2862-2871]
src/static/js/ace2_inner.ts[3459-3467]

Agent prompt

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

## Issue description
`outerWin.document` is invalid because `outerWin` is the `ace_outer` iframe element. The PageUp/PageDown handler should derive viewport height from the iframe document (`outerDoc`) or reuse the existing `getInnerHeight()` helper.
### Issue Context
`outerWin` is obtained via `document.getElementsByName('ace_outer')[0]` and the codebase consistently uses `outerWin.contentWindow.document` (aliased as `outerDoc`) to access the iframe document.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2862-2871]
- src/static/js/ace2_inner.ts[61-70]
- src/static/js/ace2_inner.ts[3459-3467]
### Suggested change
Replace:
- `const viewportHeight = outerWin.document.documentElement.clientHeight;`
With one of:
- `const viewportHeight = outerDoc.documentElement.clientHeight;` (consistent with existing code), or
- `const viewportHeight = getInnerHeight();` (already handles hidden iframe cases).

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

---

4. Viewport height miscomputed 🐞 Bug ≡ Correctness ⭐ New

Description

The pixel-based page size uses getInnerHeight() as the viewport height, but the scroll module’s
viewport calculation subtracts editor offset/padding before determining what is visible. This
mismatch can inflate the computed viewport height and make PageUp/PageDown skip too many lines.

Code

src/static/js/ace2_inner.ts[R2870-2876]

+            // Calculate lines to skip based on viewport pixel height divided by
+            // the average rendered line height. This correctly handles long wrapped
+            // lines that consume multiple visual rows (fixes #4562).
+            const viewportHeight = getInnerHeight();
+            const visibleStart = newVisibleLineRange[0];
+            const visibleEnd = newVisibleLineRange[1];
+            let totalPixelHeight = 0;

Evidence

scroll.getVisibleLineRange() is based on _getViewPortTopBottom(), which uses
doc.documentElement.clientHeight minus editor position top and page-view padding. The new code’s
viewportHeight = getInnerHeight() uses outerDoc.documentElement.clientHeight without subtracting
those extras, so the height used in the scaling calculation is not the same height used to decide
which lines are visible.

src/static/js/scroll.ts[98-112]
src/static/js/scroll.ts[114-118]
src/static/js/ace2_inner.ts[3471-3478]
src/static/js/ace2_inner.ts[2866-2876]

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 viewport height used for the PageUp/PageDown pixel math (`getInnerHeight()`) does not match the viewport height used by `scroll.getVisibleLineRange()` (which subtracts editor offset/padding). This can cause the computed page size to be larger than the actual visible area.

### Issue Context
- Visible range uses `Scroll._getViewPortTopBottom()`.
- New paging math uses `getInnerHeight()`.

### Fix
Compute `viewportHeight` using the same logic as the scroll module, e.g.:
- Add a public method on `Scroll` such as `getViewportHeight()` / `getViewportTopBottom()` and use `bottom - top`.
 - (Prefer this over calling private methods.)

### Fix Focus Areas
- src/static/js/ace2_inner.ts[2870-2887]
- src/static/js/scroll.ts[98-112]

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

---

5. Negative scroll delta possible ☑ 🐞 Bug ☼ Reliability

Description

scrollAmount is computed as viewportHeight - 40 with no clamp, so a small/hidden viewport can
yield a negative value and invert PageUp/PageDown direction (PageUp scrolls down and PageDown
scrolls up). This can happen in the already-documented “iframe is hidden” case where height can be
0.

Code

src/static/js/ace2_inner.ts[R2868-2876]

+            // Keep a small overlap so the user doesn't lose context
+            const scrollAmount = viewportHeight - 40;
+            const currentScrollY = scroll.getScrollY();
+
+            if (isPageDown) {
+              scroll.setScrollY(currentScrollY + scrollAmount);
+            } else {
+              scroll.setScrollY(Math.max(0, currentScrollY - scrollAmount));
         }

Evidence

The handler directly subtracts 40px from the viewport height and then subtracts that amount for
PageUp; if the viewport height is ≤40 (including the hidden-iframe case documented in
getInnerHeight()), the computed scroll delta becomes negative and reverses scroll direction.

src/static/js/ace2_inner.ts[2867-2876]
src/static/js/ace2_inner.ts[3460-3467]

Agent prompt

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

## Issue description
`scrollAmount = viewportHeight - 40` can be negative when the viewport is small or hidden (height 0), which inverts PageUp/PageDown behavior.
### Issue Context
There is already code that anticipates hidden iframe sizing (returning 0 if no explicit px height is set). Page navigation should remain directionally correct even in these cases.
### Fix Focus Areas
- src/static/js/ace2_inner.ts[2867-2876]
- src/static/js/ace2_inner.ts[3460-3467]
### Suggested change
After computing `viewportHeight`, clamp:
- `const scrollAmount = Math.max(0, viewportHeight - 40);`
And optionally bail out if it is 0 (to avoid calling `setScrollY` with unchanged/inverted values).

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

---

6. Unused topOffset local 🐞 Bug ⚙ Maintainability ⭐ New

Description

topOffset is computed from the old visible line range but never used within the PageUp/PageDown
handler. This dead local increases confusion and suggests leftover logic from the previous
implementation.

Code

src/static/js/ace2_inner.ts[R2862-2865]

+          const oldVisibleLineRange = scroll.getVisibleLineRange(rep);
+          let topOffset = rep.selStart[0] - oldVisibleLineRange[0];
+          if (topOffset < 0) topOffset = 0;
+

Evidence

Within the PageUp/PageDown key handler block, topOffset is assigned but there are no references to
it before the handler returns, so it has no effect.

src/static/js/ace2_inner.ts[2854-2910]

Agent prompt

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

### Issue description
`topOffset` is computed but unused, making the handler harder to understand.

### Fix
Remove `oldVisibleLineRange`/`topOffset` if no longer needed, or (if intended) wire it into the paging behavior.

### Fix Focus Areas
- src/static/js/ace2_inner.ts[2862-2865]

ⓘ 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

[SamTV12345]

I like more brackets but I can understand both styles. Makes it more visible what is inside the if.