fix(web-ui): update settings sidebar active state at scroll bottom#506
fix(web-ui): update settings sidebar active state at scroll bottom#506VernSG wants to merge 3 commits into
Conversation
Greptile SummaryThis PR fixes a scroll-sync bug in the Settings dialog where the sidebar's active state could stay locked on "Appearance" even when the "Project" section (the last one) was fully visible at the bottom of the scroll container. The root cause is that the last section marker may never cross the 40 px threshold used by the scroll loop when the container can no longer scroll far enough.
Confidence Score: 4/5Safe to merge — the change is a small, well-contained addition to the scroll handler with no risk to existing scroll behavior. The implementation is correct and the regression test exercises the exact scenario described. The only thing worth a second look is the hardcoded No files require special attention; the test file's structural assumption that
|
| Filename | Overview |
|---|---|
| web-ui/src/components/runtime-settings-dialog.tsx | Added bottom-of-scroll detection in handleBodyScroll that activates the final settings section when scrollTop reaches maxScrollTop - 2; logic is correct and handles edge cases (empty headings, non-scrollable containers). |
| web-ui/src/components/runtime-settings-dialog.test.tsx | New setScrollMetric helper and regression test that mocks jsdom scroll metrics and verifies "Project" becomes active at scroll bottom; test correctly exercises the fix but has a minor structural assumption about the scroll container's DOM depth. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[scroll event fires] --> B{isScrollingProgrammatically?}
B -- yes --> Z[return early]
B -- no --> C[query all data-settings-section headings]
C --> D[compute maxScrollTop = scrollHeight - clientHeight]
D --> E[current = 'general']
E --> F{for each heading}
F --> G{rect.top - bodyRect.top <= 40?}
G -- yes --> H[current = heading section id]
G -- no --> F
H --> F
F -- done --> I{maxScrollTop > 0 AND scrollTop >= maxScrollTop - 2?}
I -- yes --> J[current = last heading's section id]
I -- no --> K[setActiveSection current]
J --> K
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
web-ui/src/components/runtime-settings-dialog.tsx:614
The `2`-pixel fudge factor is a magic number. Browsers can return fractional `scrollTop` values (e.g. `599.7`) due to sub-pixel rendering, so a tolerance is necessary, but an unnamed literal makes the intent opaque. A named constant or an inline comment would make it easier for the next reader to understand why `2` was chosen.
```suggestion
// Allow a 2-pixel tolerance for sub-pixel scrollTop values reported by some browsers.
if (maxScrollTop > 0 && body.scrollTop >= maxScrollTop - 2) {
```
Reviews (1): Last reviewed commit: "fix(web-ui): update settings sidebar act..." | Re-trigger Greptile
When scrolling the Settings dialog to the bottom, the sidebar active state could remain stuck on
Appearanceeven though theProjectsection was visible.This happened because the active section is calculated from section markers crossing a top threshold. Near the bottom of the scroll container, the final section marker may not cross that threshold before scrolling reaches the end, so the sidebar keeps highlighting the previous section.
This fix detects when the Settings body has reached the bottom and marks the final settings section as active. This keeps the sidebar navigation state aligned with the visible content.
Changes:
Testing:
npm --prefix web-ui test -- runtime-settings-dialog.test.tsxnpm --prefix web-ui run typecheck