fix(settings): update "on this page" highlight as user scrolls by fayazg · Pull Request #730 · linuxfoundation/lfx-self-serve

fayazg · 2026-05-19T02:16:32Z

Summary

Track the active "On this page" TOC item using an IntersectionObserver on the three section anchors (email-settings, password, developer-settings) instead of relying on click-only state.
Observer is set up via afterNextRender and disconnected through destroyRef.onDestroy. Guarded with isPlatformBrowser per .claude/rules/ssr-safety.md, so SSR is unaffected.
Activation band rootMargin: '-80px 0px -70% 0px' puts the highlight on the section whose top crosses roughly the upper third of the viewport, below the sticky page header.
Click-to-scroll behavior in scrollToSection() is unchanged — the click still sets activeSection immediately for instant feedback, and the observer takes over once the smooth scroll settles.

Test plan

Navigate to /settings/account
Scroll slowly down: TOC highlight advances Email Settings → Password → Developer Settings
Scroll back up: highlight reverses correctly
Click each TOC item: page smooth-scrolls to that section and the highlight lands on it
yarn build passes (browser + SSR bundles)
yarn lint:check passes

Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

Switch the IntersectionObserver to watch each section's <h2> heading sentinel instead of the full section <div>, so a long section no longer keeps the highlight active while the next section's heading is already crossing the activation band. Addresses the Copilot review on PR #729. Also add a passive scroll listener that snaps the highlight to the last section when the user is within 4px of the page bottom, since the last section is short enough that its heading may never enter the activation band. The override only fires on scrollable pages and is registered inside the existing isPlatformBrowser guard, so SSR stays unaffected. Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

coderabbitai · 2026-05-19T02:16:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 29ae4ed6-4049-4772-9c28-3885d85b6320

📥 Commits

Reviewing files that changed from the base of the PR and between aba0d80 and 990cbab.

📒 Files selected for processing (2)

apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.html
apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts

🚧 Files skipped from review as they are similar to previous changes (2)

apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts
apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.html

Walkthrough

AccountSettingsComponent defers TOC setup with afterNextRender, then creates IntersectionObservers for heading elements and a bottom "scroll-end" sentinel to update activeSection. Both observers are disconnected in destroyRef.onDestroy().

Changes

Scroll-spy Feature

Layer / File(s)	Summary
Scroll-spy initialization wiring `apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts`	Imports `afterNextRender`, adds a private `scrollSpyObserver?: IntersectionObserver`, and defers `setupScrollSpy()` to run after the next render.
Heading observers and template sentinel `apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts`, `apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.html`	Adds stable `id` attributes to Email/Password/Developer `<h2>` headings and a `data-testid="scroll-end-sentinel"` div; implements `setupScrollSpy()` that maps headings, creates an IntersectionObserver to maintain intersecting sections and set `activeSection`, adds a second observer for the scroll-end sentinel to snap to the last section, and disconnects observers on destroy.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: implementing scroll-driven updates to the TOC highlight as users scroll through the account settings page.
Description check	✅ Passed	The description is related to the changeset, providing context about the IntersectionObserver implementation, SSR safety, activation band details, and test plan.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/LFXV2-1877

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Adds scroll-spy behavior to the "On this page" TOC in the account settings page so the highlighted item updates as the user scrolls, instead of only on click.

Changes:

Added an IntersectionObserver over the three section heading sentinels (email-settings, password, developer-settings) to drive activeSection.
Initialized the observer via afterNextRender with SSR guard, and cleaned up via destroyRef.onDestroy.
Added a scroll listener that snaps the highlight to the last section when the page is scrolled to the bottom, working around the short final section never entering the activation band.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coderabbitai

🧹 Nitpick comments (1)

apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts (1)

506-506: 💤 Low value

Consider documenting the magic numbers.

The -70% bottom margin and 4 pixel bottom threshold are effective but could benefit from inline comments explaining their purpose:

-70%: Creates activation band in upper ~30% of viewport (below header)
4: Small buffer to account for sub-pixel rendering/rounding

📝 Optional: Add inline clarifications

-      { rootMargin: '-80px 0px -70% 0px', threshold: 0 }
+      { 
+        rootMargin: '-80px 0px -70% 0px', // Activation band: below header, upper ~30% of viewport
+        threshold: 0 
+      }

-      window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 4;
+      window.innerHeight + window.scrollY >= document.documentElement.scrollHeight - 4; // 4px buffer for sub-pixel rounding

Also applies to: 515-515

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts`
at line 506, Document the magic numbers used in the IntersectionObserver options
in account-settings.component.ts: add short inline comments next to the
rootMargin string '-80px 0px -70% 0px' explaining that '-80px' accounts for the
fixed header offset and '-70%' creates an activation band in the upper ~30% of
the viewport, and add a comment next to the threshold value (the 0 / or the 4px
buffer usage nearby) describing that the small pixel buffer (4) compensates for
sub-pixel rendering/rounding; locate these comments where the
IntersectionObserver is created (the options object containing rootMargin and
threshold) so future readers understand the rationale for those numbers.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts`:
- Line 506: Document the magic numbers used in the IntersectionObserver options
in account-settings.component.ts: add short inline comments next to the
rootMargin string '-80px 0px -70% 0px' explaining that '-80px' accounts for the
fixed header offset and '-70%' creates an activation band in the upper ~30% of
the viewport, and add a comment next to the threshold value (the 0 / or the 4px
buffer usage nearby) describing that the small pixel buffer (4) compensates for
sub-pixel rendering/rounding; locate these comments where the
IntersectionObserver is created (the options object containing rootMargin and
threshold) so future readers understand the rationale for those numbers.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dd705c30-7f8f-4209-a3b6-c9d010eeeb79

📥 Commits

Reviewing files that changed from the base of the PR and between 6ee4ac8 and 0eb61fb.

📒 Files selected for processing (1)

apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts

Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

fayazg · 2026-05-19T02:31:43Z

Commit 8005bbe9 — refactor(settings): use local observer ref in scroll-spy setup

Small code-quality polish addressing the self-review NIT from the PR review:

setupScrollSpy() now assigns the IntersectionObserver to a local const observer first, then stores it on this.scrollSpyObserver
The forEach loop uses observer.observe(heading) directly instead of this.scrollSpyObserver!.observe(heading) with a non-null assertion
No behavior change — cleanup via destroyRef.onDestroy is unchanged

MRashad26

Strict pass against ~/LFX/code-enforcer-agent.md. The IntersectionObserver approach is the right primitive, SSR is correctly guarded with isPlatformBrowser, and destroyRef.onDestroy cleanup is in place. Two findings worth a look — one about coupling production behavior to test selectors, one about the end-of-scroll override.

@MRashad26

Address review comments from @MRashad26: - account-settings.component.html: add id="<section>-heading" to each of the three <h2> elements so production code can query them by id - account-settings.component.ts: replace document.querySelector with document.getElementById for heading lookups; remove scroll listener and magic-pixel bottom-detection in favour of a second IntersectionObserver over a 1px sentinel div appended at the end of the content column Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

fayazg · 2026-05-19T14:57:38Z

Review Feedback Addressed

Commit: aba0d80

Changes Made

account-settings.component.html: added id="email-settings-heading", id="password-heading", and id="developer-settings-heading" to the three <h2> elements (per @MRashad26)
account-settings.component.ts: replaced document.querySelector('[data-testid="..."]') with document.getElementById(...) — consistent with the existing scrollToSection() pattern (per @MRashad26)
account-settings.component.html: appended an invisible 1px sentinel <div> at the end of the content column (per @MRashad26)
account-settings.component.ts: removed the isAtBottom() helper, onScroll callback, and window.addEventListener('scroll', ...) — replaced with a second IntersectionObserver over the sentinel that snaps to the last section when the sentinel enters the viewport. Both observers disconnect in destroyRef.onDestroy (per @MRashad26)

No Change Needed

coderabbitai's top-level nitpick about documenting the magic numbers (-80px 0px -70% 0px and 4px) is now largely moot: the 4 is gone after the sentinel refactor, and the -70% rootMargin is the only remaining value; its meaning is conveyed by the surrounding comment.

Threads Resolved

2 of 2 unresolved threads addressed in this iteration.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Address review comments from copilot-pull-request-reviewer: - account-settings.component.html: add id="scroll-end-sentinel" to the sentinel div so production code queries it by id, not data-testid - account-settings.component.ts: replace querySelector with getElementById for the sentinel lookup (same fix pattern as round 1 heading lookups) - account-settings.component.ts: guard the end-observer callback so it only snaps to the last section when the last heading's bottom has cleared the 80px header offset; prevents the TOC from pinning to "Developer Settings" on tall viewports where the sentinel is visible from initial paint without any scrolling Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

fayazg · 2026-05-19T15:08:49Z

Review Feedback Addressed (Round 2)

Commit: 990cbab

Changes Made

account-settings.component.html: added id="scroll-end-sentinel" to the sentinel <div> (kept data-testid for test selectors) (per copilot-pull-request-reviewer)
account-settings.component.ts: replaced document.querySelector('[data-testid="..."]') with document.getElementById('scroll-end-sentinel') — same fix applied in round 1 to the heading lookups (per copilot-pull-request-reviewer)
account-settings.component.ts: added a guard in the end-observer callback; the sentinel only snaps to "Developer Settings" when lastHeading.getBoundingClientRect().bottom <= 80, preventing the TOC from pinning to the last section on tall viewports where the sentinel is visible from initial paint (per copilot-pull-request-reviewer)

No Change Needed

line 505 — tie-breaking: the rootMargin: '-80px 0px -70% 0px' already sizes the activation band to fit at most one heading at a time, and sectionIds.find() (earliest-in-document) matches reading intent. Responded on the thread with rationale. Left open for reviewer to confirm.

Threads Resolved

2 of 3 new threads resolved. Thread 1 (tie-breaking concern) left open pending reviewer acknowledgment.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts:525

The end-of-page sentinel guard duplicates the same 80px header offset used in the main observer’s rootMargin. Reuse a shared headerOffsetPx constant here to keep both observers in sync if the layout/header changes.

        // to show the whole page without scrolling the sentinel is already intersecting
        // from initial paint — this guard prevents pinning the TOC to the last section
        // before the user has reached it.
        if (entry.isIntersecting && lastHeading && lastHeading.getBoundingClientRect().bottom <= 80) {
          this.activeSection.set(lastSectionId);

+        const activeId = sectionIds.find((id) => intersecting.has(id));
+        if (activeId) this.activeSection.set(activeId);
+      },
+      { rootMargin: '-80px 0px -70% 0px', threshold: 0 }
+    );


fayazg added 2 commits May 18, 2026 19:34

fix(settings): update "On this page" highlight as user scrolls

314683f

Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 19, 2026 02:16

fayazg requested a review from a team as a code owner May 19, 2026 02:16

Copilot started reviewing on behalf of fayazg May 19, 2026 02:17 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

coderabbitai Bot reviewed May 19, 2026

View reviewed changes

refactor(settings): use local observer ref in scroll-spy setup

8005bbe

Signed-off-by: Fayaz G <5818912+fayazg@users.noreply.github.com>

MRashad26 reviewed May 19, 2026

View reviewed changes

Comment thread apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts Outdated

Comment thread apps/lfx-one/src/app/modules/settings/account-settings/account-settings.component.ts Outdated

Copilot AI review requested due to automatic review settings May 19, 2026 14:56

Copilot started reviewing on behalf of fayazg May 19, 2026 14:57 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

MRashad26 approved these changes May 19, 2026

View reviewed changes

Merge branch 'main' into feat/LFXV2-1877

4f58bbf

Copilot AI review requested due to automatic review settings May 19, 2026 16:02

fayazg merged commit ae6ac71 into main May 19, 2026
11 of 12 checks passed

fayazg deleted the feat/LFXV2-1877 branch May 19, 2026 16:02

Copilot started reviewing on behalf of fayazg May 19, 2026 16:03 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(settings): update "on this page" highlight as user scrolls#730

fix(settings): update "on this page" highlight as user scrolls#730
fayazg merged 6 commits into
mainfrom
feat/LFXV2-1877

fayazg commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

fayazg commented May 19, 2026

Uh oh!

MRashad26 left a comment

Uh oh!

Uh oh!

Uh oh!

fayazg commented May 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fayazg commented May 19, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

fayazg commented May 19, 2026

Summary

Test plan

Uh oh!

coderabbitai Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

fayazg commented May 19, 2026

Uh oh!

MRashad26 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fayazg commented May 19, 2026

Review Feedback Addressed

Changes Made

No Change Needed

Threads Resolved

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fayazg commented May 19, 2026

Review Feedback Addressed (Round 2)

Changes Made

No Change Needed

Threads Resolved

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented May 19, 2026 •

edited

Loading