Conversation
Contributor
Reviewer's GuideRefactors menu and navigation JavaScript to store richer state objects, introduces an interval-based scrollIntoView mechanism that retries briefly until active links exist to improve reliability/performance, cleans up event handler registration/disposal, and makes minor code-style fixes in C# and anchor scrolling logic. Sequence diagram for the new interval-based menu scrollIntoView behaviorsequenceDiagram
participant BlazorMenu as BlazorMenuComponent
participant MenuJs as Menu_razor_js
participant Data as DataModule
participant DOM as Document
participant Timer as IntervalTimer
BlazorMenu->>MenuJs: init(id)
MenuJs->>DOM: querySelector(#id)
DOM-->>MenuJs: element
MenuJs->>Data: set(id, menu)
MenuJs->>MenuJs: scrollElementToView(menu)
rect rgb(230,230,250)
MenuJs->>Timer: setInterval(callback, 100)
loop up to 3 attempts
Timer->>DOM: querySelectorAll(.nav-link.active)
alt active links found
DOM-->>Timer: links
Timer->>MenuJs: callback(links)
MenuJs->>Timer: clearInterval(handler)
MenuJs->>DOM: link.scrollIntoView(smooth, center)
else no active links
DOM-->>Timer: []
Timer->>Timer: increment count
end
end
end
BlazorMenu->>MenuJs: update(id)
MenuJs->>Data: get(id)
Data-->>MenuJs: menu
MenuJs->>DOM: update collapses
MenuJs->>MenuJs: scrollElementToView(menu)
BlazorMenu->>MenuJs: scrollToView(id)
MenuJs->>Data: get(id)
Data-->>MenuJs: menu
MenuJs->>MenuJs: scrollElementToView(menu)
BlazorMenu->>MenuJs: dispose(id)
MenuJs->>Data: get(id)
Data-->>MenuJs: menu
MenuJs->>Data: remove(id)
alt menu.handler exists
MenuJs->>Timer: clearInterval(menu.handler)
MenuJs->>MenuJs: menu.handler = null
end
MenuJs->>DOM: remove collapse event listeners
Sequence diagram for NavMenu state storage and event handler cleanupsequenceDiagram
participant Layout as ComponentLayout
participant NavMenuJs as NavMenu_razor_js
participant Data as DataModule
participant EventHandler as EventHandlerModule
participant DOM as Document
Layout->>NavMenuJs: init(id)
NavMenuJs->>DOM: querySelector(.navbar-toggler)
DOM-->>NavMenuJs: navbar
NavMenuJs->>DOM: querySelector(.sidebar-content)
DOM-->>NavMenuJs: menu
NavMenuJs->>Data: set(id, { navbar, menu })
NavMenuJs->>EventHandler: on(navbar, click, handler)
NavMenuJs->>EventHandler: on(menu, click, .nav-link, handler)
actor User
User->>navbar: click
EventHandler-->>navmenu: delegated click
navmenu->>menu: toggle(show)
User->>menu: click .nav-link
EventHandler-->>navlink: delegated click
navlink->>navlink: read href
alt href != #
navlink->>menu: remove(show)
end
Layout->>NavMenuJs: dispose(id)
NavMenuJs->>Data: get(id)
Data-->>NavMenuJs: { navbar, menu }
NavMenuJs->>Data: remove(id)
NavMenuJs->>EventHandler: off(navbar, click)
NavMenuJs->>EventHandler: off(menu, click, .nav-link)
Class diagram for Menu and NavMenu JavaScript state objectsclassDiagram
class MenuModule {
+init(id)
+update(id)
+scrollToView(id)
+dispose(id)
-scrollElementToView(menu)
}
class MenuState {
+element
+collapses
+handler
}
class NavMenuModule {
+init(id)
+dispose(id)
}
class NavMenuState {
+navbar
+menu
}
class DataModule {
+set(id, value)
+get(id)
+remove(id)
}
MenuModule --> MenuState : stores
NavMenuModule --> NavMenuState : stores
DataModule --> MenuState : values
DataModule --> NavMenuState : values
class ComponentLayoutModule {
+scrollToAnchor()
}
class DOM
MenuModule ..> DataModule : uses
NavMenuModule ..> DataModule : uses
NavMenuModule ..> NavMenuState : destructures
ComponentLayoutModule ..> DOM : queries elements
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
scrollElementToView, the retry interval is only cleared when an active link is found; if no.nav-link.activeappears within the retry window the interval will run indefinitely, so consider clearing the interval once the retry limit is reached or before assigning a new handler. scrollElementToViewnow assignsmenu.handlerwithout clearing any existing interval first; ifinit/update/scrollToVieware called multiple times, this can leave orphaned intervals running, so ensure any existing handler is cleared before creating a new one.- In
NavMenu.dispose,datais used without a null/undefined check; adding a guard whenData.get(id)returns nothing will prevent runtime errors on double-dispose or mismatched IDs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `scrollElementToView`, the retry interval is only cleared when an active link is found; if no `.nav-link.active` appears within the retry window the interval will run indefinitely, so consider clearing the interval once the retry limit is reached or before assigning a new handler.
- `scrollElementToView` now assigns `menu.handler` without clearing any existing interval first; if `init/update/scrollToView` are called multiple times, this can leave orphaned intervals running, so ensure any existing handler is cleared before creating a new one.
- In `NavMenu.dispose`, `data` is used without a null/undefined check; adding a guard when `Data.get(id)` returns nothing will prevent runtime errors on double-dispose or mismatched IDs.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Menu/Menu.razor.js:110-119` </location>
<code_context>
- link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
- }
+ let count = 0;
+ menu.handler = setInterval(() => {
+ if (count < 3) {
+ var links = [...element.querySelectorAll('.nav-link.active')]
+ if (links.length > 0) {
+ clearInterval(menu.handler);
+ menu.handler = null;
+
+ var link = links.pop()
+ link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
+ }
+ }
+ count++;
+ }, 100);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Interval is never cleared when no active links are found, causing a runaway timer.
Because the interval is only cleared when `.nav-link.active` exists, if none ever appear the timer runs indefinitely and `count` grows unbounded. Add a guard to clear the interval once `count` reaches the max number of attempts (e.g., `count >= 3`) to prevent a perpetual timer and wasted work.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/Menu/Menu.razor.js:107-110` </location>
<code_context>
}
-const scrollElementToView = element => {
+const scrollElementToView = menu => {
+ const { element } = menu;
const scroll = element.hasAttribute('data-bb-scroll-view')
if (scroll) {
- var links = [...element.querySelectorAll('.nav-link.active')]
- if (links.length > 0) {
- var link = links.pop()
- link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
- }
+ let count = 0;
+ menu.handler = setInterval(() => {
+ if (count < 3) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Repeated calls to `scrollElementToView` can create multiple active intervals on the same menu instance.
If `scrollElementToView(menu)` is invoked again before a previous interval completes, the new call overwrites `menu.handler` but the earlier intervals keep running via their closures. Consider clearing any existing interval before starting a new one (e.g. `if (menu.handler) clearInterval(menu.handler);`) so only a single timer runs per menu instance.
```suggestion
const scroll = element.hasAttribute('data-bb-scroll-view')
if (scroll) {
let count = 0;
if (menu.handler) {
clearInterval(menu.handler);
menu.handler = null;
}
menu.handler = setInterval(() => {
```
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/Menu/Menu.razor.js:100` </location>
<code_context>
})
}
-const scrollElementToView = element => {
+const scrollElementToView = menu => {
+ const { element } = menu;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the scroll retry logic into an element-based helper with its own timer management instead of storing timer state on the menu object.
You can keep the new “wait for `.nav-link.active`” behavior while reducing coupling and side effects by:
* Making `scrollElementToView` operate on the element again.
* Managing the retry state and timer locally (or in a small helper) instead of mutating `menu`.
* Centralizing timer cleanup in one place.
One concrete option:
```js
// module-scope map to track timers per menu element
const scrollTimers = new Map();
const clearScrollTimer = (element) => {
const handle = scrollTimers.get(element);
if (handle) {
clearTimeout(handle);
scrollTimers.delete(element);
}
};
const scrollElementToView = (element) => {
if (!element.hasAttribute('data-bb-scroll-view')) {
return;
}
clearScrollTimer(element); // avoid multiple concurrent timers
let count = 0;
const tryScroll = () => {
const links = [...element.querySelectorAll('.nav-link.active')];
if (links.length > 0) {
scrollTimers.delete(element);
const link = links[links.length - 1];
link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
return;
}
if (++count < 3) {
const handle = setTimeout(tryScroll, 100);
scrollTimers.set(element, handle);
} else {
scrollTimers.delete(element);
}
};
const handle = setTimeout(tryScroll, 100);
scrollTimers.set(element, handle);
};
```
Call sites stay simple and decoupled from timer state:
```js
// init/update/scrollToView
scrollElementToView(menu.element);
```
And `dispose` only needs to clear the timer for this element (no `menu.handler` property):
```js
export function dispose(id) {
const menu = Data.get(id);
Data.remove(id);
clearScrollTimer(menu.element);
menu.collapses.forEach(el => {
const target = getTargetElement(el);
const collapse = bootstrap.Collapse.getInstance(target);
if (collapse !== null) {
menu.disposeCollapse(collapse, el);
}
});
}
```
This preserves the retry behavior and cleanup while:
* Removing the `handler` field from `menu`.
* Keeping `scrollElementToView` element-focused and side-effect-limited.
* Localizing timer logic and cleanup to a small helper, which is easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+110
to
+119
| menu.handler = setInterval(() => { | ||
| if (count < 3) { | ||
| var links = [...element.querySelectorAll('.nav-link.active')] | ||
| if (links.length > 0) { | ||
| clearInterval(menu.handler); | ||
| menu.handler = null; | ||
|
|
||
| var link = links.pop() | ||
| link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' }); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Interval is never cleared when no active links are found, causing a runaway timer.
Because the interval is only cleared when .nav-link.active exists, if none ever appear the timer runs indefinitely and count grows unbounded. Add a guard to clear the interval once count reaches the max number of attempts (e.g., count >= 3) to prevent a perpetual timer and wasted work.
| }) | ||
| } | ||
|
|
||
| const scrollElementToView = element => { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring the scroll retry logic into an element-based helper with its own timer management instead of storing timer state on the menu object.
You can keep the new “wait for .nav-link.active” behavior while reducing coupling and side effects by:
- Making
scrollElementToViewoperate on the element again. - Managing the retry state and timer locally (or in a small helper) instead of mutating
menu. - Centralizing timer cleanup in one place.
One concrete option:
// module-scope map to track timers per menu element
const scrollTimers = new Map();
const clearScrollTimer = (element) => {
const handle = scrollTimers.get(element);
if (handle) {
clearTimeout(handle);
scrollTimers.delete(element);
}
};
const scrollElementToView = (element) => {
if (!element.hasAttribute('data-bb-scroll-view')) {
return;
}
clearScrollTimer(element); // avoid multiple concurrent timers
let count = 0;
const tryScroll = () => {
const links = [...element.querySelectorAll('.nav-link.active')];
if (links.length > 0) {
scrollTimers.delete(element);
const link = links[links.length - 1];
link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
return;
}
if (++count < 3) {
const handle = setTimeout(tryScroll, 100);
scrollTimers.set(element, handle);
} else {
scrollTimers.delete(element);
}
};
const handle = setTimeout(tryScroll, 100);
scrollTimers.set(element, handle);
};Call sites stay simple and decoupled from timer state:
// init/update/scrollToView
scrollElementToView(menu.element);And dispose only needs to clear the timer for this element (no menu.handler property):
export function dispose(id) {
const menu = Data.get(id);
Data.remove(id);
clearScrollTimer(menu.element);
menu.collapses.forEach(el => {
const target = getTargetElement(el);
const collapse = bootstrap.Collapse.getInstance(target);
if (collapse !== null) {
menu.disposeCollapse(collapse, el);
}
});
}This preserves the retry behavior and cleanup while:
- Removing the
handlerfield frommenu. - Keeping
scrollElementToViewelement-focused and side-effect-limited. - Localizing timer logic and cleanup to a small helper, which is easier to reason about.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7423 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 32798 32798
Branches 4551 4551
=========================================
Hits 32798 32798
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the scrollIntoView performance for the Menu component by implementing a polling mechanism to wait for active links to be rendered before attempting to scroll them into view. The implementation addresses timing issues where the scroll operation was attempted before elements were fully available in the DOM.
Key changes:
- Implemented a retry mechanism with setInterval to poll for active menu links (up to 3 attempts at 100ms intervals)
- Added proper cleanup of interval handlers in the dispose method
- Refactored code for better maintainability with destructuring and improved variable scoping
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Menu/Menu.razor.js | Added polling mechanism to wait for active links before scrolling; added interval cleanup in dispose method |
| src/BootstrapBlazor/Components/Menu/Menu.razor.cs | Changed string literals from double quotes to single quotes for consistency |
| src/BootstrapBlazor.Server/Components/Layout/NavMenu.razor.js | Refactored to use destructuring and direct variable references for cleaner code |
| src/BootstrapBlazor.Server/Components/Layout/ComponentLayout.razor.js | Reordered clearTimeout call relative to scrollIntoView operation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+110
to
+122
| menu.handler = setInterval(() => { | ||
| if (count < 3) { | ||
| var links = [...element.querySelectorAll('.nav-link.active')] | ||
| if (links.length > 0) { | ||
| clearInterval(menu.handler); | ||
| menu.handler = null; | ||
|
|
||
| var link = links.pop() | ||
| link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' }); | ||
| } | ||
| } | ||
| count++; | ||
| }, 100); |
There was a problem hiding this comment.
If scrollElementToView is called multiple times before the active links are found (e.g., during rapid updates), multiple intervals will be created and stored in menu.handler, but only the last one will be tracked. Previous intervals will become orphaned and continue running indefinitely. Consider clearing any existing handler before creating a new one.
Suggested change
| menu.handler = setInterval(() => { | |
| if (count < 3) { | |
| var links = [...element.querySelectorAll('.nav-link.active')] | |
| if (links.length > 0) { | |
| clearInterval(menu.handler); | |
| menu.handler = null; | |
| var link = links.pop() | |
| link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' }); | |
| } | |
| } | |
| count++; | |
| }, 100); | |
| // Clear any existing interval before creating a new one to avoid orphaned handlers | |
| if (menu.handler) { | |
| clearInterval(menu.handler); | |
| menu.handler = null; | |
| } | |
| const handler = setInterval(() => { | |
| if (count < 3) { | |
| var links = [...element.querySelectorAll('.nav-link.active')] | |
| if (links.length > 0) { | |
| clearInterval(handler); | |
| if (menu.handler === handler) { | |
| menu.handler = null; | |
| } | |
| var link = links.pop() | |
| link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' }); | |
| } | |
| } else { | |
| // Stop retrying after the maximum number of attempts | |
| clearInterval(handler); | |
| if (menu.handler === handler) { | |
| menu.handler = null; | |
| } | |
| } | |
| count++; | |
| }, 100); | |
| menu.handler = handler; |
|
|
||
| var link = links.pop() | ||
| link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' }); | ||
| } |
There was a problem hiding this comment.
The interval handler is not cleared when the maximum number of attempts (count >= 3) is reached. This causes the interval to continue running indefinitely, wasting resources. Add cleanup logic to clear the interval and set handler to null when count reaches 3.
Suggested change
| } | |
| } | |
| } else { | |
| clearInterval(menu.handler); | |
| menu.handler = null; |
Comment on lines
8
to
+9
| const handler = setTimeout(() => { | ||
| el.scrollIntoView({ behavior: 'smooth', block: 'start' }) | ||
| clearTimeout(handler) | ||
| el.scrollIntoView({ behavior: 'smooth', block: 'start' }) |
There was a problem hiding this comment.
The clearTimeout is called before the scrollIntoView operation. If there's an error during scrollIntoView, the timeout would have already been cleared, potentially masking the issue. Consider moving the clearTimeout call after the scrollIntoView operation completes.
Suggested change
| clearTimeout(handler) | |
| el.scrollIntoView({ behavior: 'smooth', block: 'start' }) | |
| try { | |
| el.scrollIntoView({ behavior: 'smooth', block: 'start' }) | |
| } finally { | |
| clearTimeout(handler) | |
| } |
This was referenced Dec 29, 2025
This was referenced Jan 7, 2026
This was referenced Jan 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7422
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve menu and layout scrolling behavior and cleanup related JavaScript data and event handling.
Enhancements: