feat(logs): number the lines of the log, the source and the generated code #485

Merged

allan merged 1 commits from feat/logs-line-numbers into release/7.23 2026-11-06 12:37:08 +00:00

Conversation 3 Commits 1 Files Changed 4

+530 -148

hermes commented 2026-11-06 10:46:20 +00:00

Collaborator

Copy link

Copy source

What the modal shows now

The SAS Logs modal (SAS Log, SAS Source Code, SAS Generated Code) renders one row per line, and every row carries its line number. A line that SAS or the editor wrapped hangs under the text column rather than under the number, and the numbers are position: sticky so a long log line scrolled sideways runs under them instead of through them. The numbers are user-select: none, so copying the log still yields the log.

The jumping links

The error, warning and note links used to scroll to an offset computed as measureLineHeight() * issue.line, where the height came from tempDiv.clientHeight. Two things were wrong with that:

• clientHeight rounds to a whole pixel, so a fractional line height is truncated. The error is (rounded - real) * line, which grows the further down the log you click - the symptom being that the first link looks right and the later ones do not.
• The wrapper's padding: 10px was never counted.

Measured in a browser against the real component and the real styles.scss (a 168-line log, 16.25px monospace so that a clientHeight of 19 hides a real advance of 18.5px), the old offsets were line 43: 817 vs 806.5 (-10.5px), line 84: 1596 vs 1565 (-31px), line 127: 2413 vs 2360.5 (-52.5px). At whole-pixel metrics the same formula was out by a constant 11px - the padding plus the rounding - enough to put the highlight over the previous line.

The link now scrolls to the row element itself, so there is nothing to drift. Every link lands its line at the top of the log, less the wrapper's 1px border, and flashes the right row.

Answers to the review

1. logHtml write-only - removed, together with the interface field. analyseLog now only builds the three line windows, and the two specs that inspected the marked log call markIssueLines(LOG, parseLog(LOG)) directly.

2. One row and one [innerHTML] per line is slower - measured, and bounded. Against a 20,000-line SAS 9 log:

lines	whole log as one innerHTML (browser floor)	same rows built by hand	rendering every line in Angular	open time now (window of 400)
1,000	-	-	356ms	279ms
5,000	-	-	1.2s	193ms
10,000	-	-	3.6s	-
20,000	42ms	138ms	5.3s	164ms

So an all-lines render was 5.3s at 20,000 lines, and the cost is ~0.25ms per row of Angular binding plus sanitizer. Each pane now renders FIRST_RENDERED_LINES (400) and shows Showing 400 of 20000 lines [Show more] underneath; Show more adds MORE_RENDERED_LINES (2,000) at a time (580ms per click in the same harness). Jumping to a line beyond the window renders through to that line first and calls detectChanges() so the row exists before it is scrolled to - line 12,000 of a 20,000-line log took 2.4s for the 11,600 rows in between, and lands clamped to the bottom because only 11 rows follow it, which is the correct outcome rather than a miss.

Virtual scroll would keep that flat, but it needs the viewport API for jump-to-line and variable row heights on the wrapped panes; the window is the smaller change that removes the freeze, and the numbers above are what a follow-up can be judged against.

3. endLine below line - clamped: const endLine = Math.max(issue.endLine ?? issue.line, issue.line), used for the flash range and for the render-through. Spec: treats an issue that ends before it starts as one line; putting the old expression back fails it.

4. border and scroll padding - logWrapper.clientTop is now subtracted. Spec: keeps the border out of the scroll offset sets wrapper.style.border = '6px solid' and asserts scrollTop is the row offset minus 6; dropping the subtraction fails it. Verified in the browser too: the real wrapper's 1px border means the row lands at 1px, which is the top of the scrollport.

Verification

• ng test (whole client suite): TOTAL: 881 SUCCESS, 34 of them in sas-logs-modal.component.spec.ts.
• Mutation checks, each caught by exactly one spec and restored: the arithmetic scroll (scrollTop = issue.line * 20), dropping clientTop, dropping the endLine clamp, and dropping the render-through on a jump.
• tsc -p tsconfig.app.json --noEmit and tsc -p tsconfig.spec.json --noEmit: exit 0. prettier --check "src/**/*.{ts,html}": clean.
• Browser run of the real component with the real stylesheet (throwaway esbuild harness, not committed): numbers 1..N aligned to their text (measured top offset < 1px); source and generated tabs numbered; sticky numbers with a horizontally scrolled long line; the 20,000-line log numbers, "Show more" and jump above; and a log whose markup is wrapped in a <pre> keeps every line's text and still navigates.

No Cypress spec covers this modal, so the rendering assertions come from the browser run rather than from CI. A spec for it would need request history inside the modal, which is a separate piece of work.

## What the modal shows now The SAS Logs modal (SAS Log, SAS Source Code, SAS Generated Code) renders one row per line, and every row carries its line number. A line that SAS or the editor wrapped hangs under the text column rather than under the number, and the numbers are `position: sticky` so a long log line scrolled sideways runs under them instead of through them. The numbers are `user-select: none`, so copying the log still yields the log. ## The jumping links The error, warning and note links used to scroll to an offset computed as `measureLineHeight() * issue.line`, where the height came from `tempDiv.clientHeight`. Two things were wrong with that: - `clientHeight` rounds to a whole pixel, so a fractional line height is truncated. The error is `(rounded - real) * line`, which grows the further down the log you click - the symptom being that the first link looks right and the later ones do not. - The wrapper's `padding: 10px` was never counted. Measured in a browser against the real component and the real `styles.scss` (a 168-line log, 16.25px monospace so that a `clientHeight` of 19 hides a real advance of 18.5px), the old offsets were line 43: 817 vs 806.5 (-10.5px), line 84: 1596 vs 1565 (-31px), line 127: 2413 vs 2360.5 (-52.5px). At whole-pixel metrics the same formula was out by a constant 11px - the padding plus the rounding - enough to put the highlight over the previous line. The link now scrolls to the row element itself, so there is nothing to drift. Every link lands its line at the top of the log, less the wrapper's 1px border, and flashes the right row. ## Answers to the review 1. **`logHtml` write-only** - removed, together with the interface field. `analyseLog` now only builds the three line windows, and the two specs that inspected the marked log call `markIssueLines(LOG, parseLog(LOG))` directly. 2. **One row and one `[innerHTML]` per line is slower** - measured, and bounded. Against a 20,000-line SAS 9 log: | lines | whole log as one `innerHTML` (browser floor) | same rows built by hand | rendering every line in Angular | open time now (window of 400) | |---|---|---|---|---| | 1,000 | - | - | 356ms | 279ms | | 5,000 | - | - | 1.2s | 193ms | | 10,000 | - | - | 3.6s | - | | 20,000 | 42ms | 138ms | 5.3s | 164ms | So an all-lines render was 5.3s at 20,000 lines, and the cost is ~0.25ms per row of Angular binding plus sanitizer. Each pane now renders `FIRST_RENDERED_LINES` (400) and shows `Showing 400 of 20000 lines [Show more]` underneath; Show more adds `MORE_RENDERED_LINES` (2,000) at a time (580ms per click in the same harness). Jumping to a line beyond the window renders through to that line first and calls `detectChanges()` so the row exists before it is scrolled to - line 12,000 of a 20,000-line log took 2.4s for the 11,600 rows in between, and lands clamped to the bottom because only 11 rows follow it, which is the correct outcome rather than a miss. Virtual scroll would keep that flat, but it needs the viewport API for jump-to-line and variable row heights on the wrapped panes; the window is the smaller change that removes the freeze, and the numbers above are what a follow-up can be judged against. 3. **`endLine` below `line`** - clamped: `const endLine = Math.max(issue.endLine ?? issue.line, issue.line)`, used for the flash range and for the render-through. Spec: `treats an issue that ends before it starts as one line`; putting the old expression back fails it. 4. **border and scroll padding** - `logWrapper.clientTop` is now subtracted. Spec: `keeps the border out of the scroll offset` sets `wrapper.style.border = '6px solid'` and asserts `scrollTop` is the row offset minus 6; dropping the subtraction fails it. Verified in the browser too: the real wrapper's 1px border means the row lands at 1px, which is the top of the scrollport. ## Verification - `ng test` (whole client suite): `TOTAL: 881 SUCCESS`, 34 of them in `sas-logs-modal.component.spec.ts`. - Mutation checks, each caught by exactly one spec and restored: the arithmetic scroll (`scrollTop = issue.line * 20`), dropping `clientTop`, dropping the `endLine` clamp, and dropping the render-through on a jump. - `tsc -p tsconfig.app.json --noEmit` and `tsc -p tsconfig.spec.json --noEmit`: exit 0. `prettier --check "src/**/*.{ts,html}"`: clean. - Browser run of the real component with the real stylesheet (throwaway esbuild harness, not committed): numbers 1..N aligned to their text (measured top offset < 1px); source and generated tabs numbered; sticky numbers with a horizontally scrolled long line; the 20,000-line log numbers, "Show more" and jump above; and a log whose markup is wrapped in a `<pre>` keeps every line's text and still navigates. No Cypress spec covers this modal, so the rendering assertions come from the browser run rather than from CI. A spec for it would need request history inside the modal, which is a separate piece of work.

hermes force-pushed feat/logs-line-numbers from 92a031caaa to 2e3f0ac9b4 2026-11-06 11:14:23 +00:00 Compare

hermes commented 2026-11-06 11:14:48 +00:00

Author

Collaborator

Copy link

Copy source

All four findings are fixed on 2e3f0ac9 (force-pushed with lease; body updated with the measurements).

1. logHtml write-only. Dropped the field and the assignment - analyseLog now only builds the three line windows (logWindow, sourceWindow, generatedWindow). The two specs that inspected the marked log called analyseLog and read request.logHtml; they now call markIssueLines(LOG, parseLog(LOG)) directly, which is what they were really testing.

2. One row and one [innerHTML] per line. You were right, and it was worse than "noticeably slower": measured in the harness, rendering every line took 356ms at 1,000 lines, 1.2s at 5,000, 3.6s at 10,000 and 5.3s at 20,000 (against 42ms for the whole log as a single innerHTML and 138ms to build the same 20,000 rows by hand - so ~0.25ms per row of Angular binding plus sanitizer). Each pane now renders a window: FIRST_RENDERED_LINES 400 with a Showing 400 of 20000 lines [Show more] line underneath, Show more adding 2,000 at a time (580ms per click in the harness), and open time is flat at 165-280ms for 1,000 / 5,000 / 20,000 lines. A jump past the window renders through to its target line and calls detectChanges() so the row exists before it is scrolled to: line 12,000 of a 20,000-line log cost 2.4s for the 11,600 rows in between and landed correctly, clamped to the bottom because only 11 rows follow it. I did not go to virtual scroll: it needs the viewport API for jump-to-line and the wrapped panes have variable row heights, and the window removes the freeze for a fraction of the change - the numbers in the body are the baseline for doing it later.

3. endLine below line. Clamped: const endLine = Math.max(issue.endLine ?? issue.line, issue.line), used for both the flash range and the render-through. New spec treats an issue that ends before it starts as one line; putting issue.endLine back fails it.

4. Border / scroll padding. logWrapper.clientTop is now subtracted. New spec keeps the border out of the scroll offset puts a 6px border on the wrapper and asserts scrollTop is the row offset minus 6; dropping the subtraction fails it. Confirmed in the browser as well: the real wrapper's 1px border means the row lands at 1px, the top of the scrollport, instead of scrolling the border away.

Suite is TOTAL: 881 SUCCESS (34 in this spec), both tsc projects exit 0, prettier clean. Mutation checks for the four behaviours (arithmetic scroll, clientTop, the clamp, the render-through) each fail exactly one spec and pass again restored.

All four findings are fixed on `2e3f0ac9` (force-pushed with lease; body updated with the measurements). **1. `logHtml` write-only.** Dropped the field and the assignment - `analyseLog` now only builds the three line windows (`logWindow`, `sourceWindow`, `generatedWindow`). The two specs that inspected the marked log called `analyseLog` and read `request.logHtml`; they now call `markIssueLines(LOG, parseLog(LOG))` directly, which is what they were really testing. **2. One row and one `[innerHTML]` per line.** You were right, and it was worse than "noticeably slower": measured in the harness, rendering every line took 356ms at 1,000 lines, 1.2s at 5,000, 3.6s at 10,000 and 5.3s at 20,000 (against 42ms for the whole log as a single `innerHTML` and 138ms to build the same 20,000 rows by hand - so ~0.25ms per row of Angular binding plus sanitizer). Each pane now renders a window: `FIRST_RENDERED_LINES` 400 with a `Showing 400 of 20000 lines [Show more]` line underneath, Show more adding 2,000 at a time (580ms per click in the harness), and open time is flat at 165-280ms for 1,000 / 5,000 / 20,000 lines. A jump past the window renders through to its target line and calls `detectChanges()` so the row exists before it is scrolled to: line 12,000 of a 20,000-line log cost 2.4s for the 11,600 rows in between and landed correctly, clamped to the bottom because only 11 rows follow it. I did not go to virtual scroll: it needs the viewport API for jump-to-line and the wrapped panes have variable row heights, and the window removes the freeze for a fraction of the change - the numbers in the body are the baseline for doing it later. **3. `endLine` below `line`.** Clamped: `const endLine = Math.max(issue.endLine ?? issue.line, issue.line)`, used for both the flash range and the render-through. New spec `treats an issue that ends before it starts as one line`; putting `issue.endLine` back fails it. **4. Border / scroll padding.** `logWrapper.clientTop` is now subtracted. New spec `keeps the border out of the scroll offset` puts a 6px border on the wrapper and asserts `scrollTop` is the row offset minus 6; dropping the subtraction fails it. Confirmed in the browser as well: the real wrapper's 1px border means the row lands at 1px, the top of the scrollport, instead of scrolling the border away. Suite is `TOTAL: 881 SUCCESS` (34 in this spec), both `tsc` projects exit 0, prettier clean. Mutation checks for the four behaviours (arithmetic scroll, `clientTop`, the clamp, the render-through) each fail exactly one spec and pass again restored.

hermes added 1 commit 2026-11-06 11:25:51 +00:00

feat(logs): number the lines of the log, the source and the generated code ... 7a5286fca8

The SAS Logs modal rendered the log, the source code and the generated code as one text block each, so no line could be referred to and the error, warning and note links had to locate a line by arithmetic. Render one row per line instead: every line carries its number, a wrapped line hangs under the text rather than under the number, and the numbers stay in view when a long log line is scrolled sideways.

The links now scroll to the row itself. The old offset was the measured line height multiplied by the line index, and two things were wrong with it: clientHeight rounds the height to a whole pixel, and the wrapper's padding was not counted. In a 168-line log at the stylesheet's own metrics the early links landed within a few pixels of their line while the later ones did not; at an 18.5px line height, which is what 16px monospace at 1.15 produces, the scroll for line 43 was 10.5px out and for line 127 was 52.5px out, so the highlight sat above the line it named and the drift grew with the line number. A row's own position cannot drift: every link lands its line at the top of the log, less the wrapper's 1px border, and flashes the right row.

The flash is a class on the rows rather than an absolutely positioned overlay, so a message SAS wrapped over several lines flashes every line it runs to.

One row per line costs about 0.25ms in Angular - a binding plus a sanitizer pass each - so rendering a whole log would freeze the modal: 20,000 lines took 5.3s to open. Each pane renders a window of 400 lines with a "Show more" line underneath, a jump renders through to the line it was given before scrolling, and opening is now flat at 165-280ms for 1,000, 5,000 and 20,000 lines.

hermes force-pushed feat/logs-line-numbers from 2e3f0ac9b4 to 7a5286fca8 2026-11-06 11:25:51 +00:00 Compare

hermes commented 2026-11-06 11:26:00 +00:00

Author

Collaborator

Copy link

Copy source

Lighthouse and the build both failed at the production build, not at the audit: src/app/shared/sas-logs-modal/sas-logs-modal.component.html:140:60 - error TS2532: Object is possibly 'undefined'. It was my @if (programLog.logWindow?.visible.length): ?. guards logWindow only, so .length still sat on a possibly-undefined string[]. tsc --noEmit and karma do not type-check templates - only the production build does - so my local gates missed it.

Fixed on 7a5286fc by narrowing in the template instead of asserting: @if (programLog.logWindow; as logWindow) and the same for the source and generated panes, so the body reads logWindow.visible / logWindow.all with no ! and no optional chain.

Verified with the CI step itself this time: ng build --configuration production completes (dist written, no errors, only the pre-existing warnings), ngc -p tsconfig.app.json --noEmit exits 0, the spec suite is 34/34 and prettier is clean. The browser harness was re-run after the template change: all three panes render their 400-line window with the "Show more" line, and a jump still lands on its row and flashes it.

Lighthouse and the build both failed at the production build, not at the audit: `src/app/shared/sas-logs-modal/sas-logs-modal.component.html:140:60 - error TS2532: Object is possibly 'undefined'`. It was my `@if (programLog.logWindow?.visible.length)`: `?.` guards `logWindow` only, so `.length` still sat on a possibly-undefined `string[]`. `tsc --noEmit` and karma do not type-check templates - only the production build does - so my local gates missed it. Fixed on `7a5286fc` by narrowing in the template instead of asserting: `@if (programLog.logWindow; as logWindow)` and the same for the source and generated panes, so the body reads `logWindow.visible` / `logWindow.all` with no `!` and no optional chain. Verified with the CI step itself this time: `ng build --configuration production` completes (dist written, no errors, only the pre-existing warnings), `ngc -p tsconfig.app.json --noEmit` exits 0, the spec suite is 34/34 and prettier is clean. The browser harness was re-run after the template change: all three panes render their 400-line window with the "Show more" line, and a jump still lands on its row and flashes it.

hermes reviewed 2026-11-06 11:45:18 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

REVIEWED ON 2026-11-06 11:45 UTC

Reviewed the full diff: the log/source/generated-code line windows with per-line numbering, the goToLogLine rewrite, the stylesheet changes and the spec.
No blocking issues.

REVIEWED ON 2026-11-06 11:45 UTC Reviewed the full diff: the log/source/generated-code line windows with per-line numbering, the goToLogLine rewrite, the stylesheet changes and the spec. No blocking issues.

allan merged commit 712f74db6d into release/7.23 2026-11-06 12:37:08 +00:00

allan deleted branch feat/logs-line-numbers 2026-11-06 12:37:08 +00:00

hermes referenced this pull request 2026-11-07 17:41:32 +00:00

feat(release): 7.23 - licence state as a banner, an 849 kB initial load, numbered logs #481

Sign in to join this conversation.

Reviewers

No Reviewers

hermes

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

allan (Allan) dcbot DylanLeong henrik hermes (hermes) mihajlo sabir sead trevor Yury

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dc/dc#485