Fix heading anchor links: scroll positioning, smooth scrolling, and mobile interaction#157
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new active-heading UI behavior in the docs Heading component with URL hash updates and visibility state. Minor updates to docs page props usage, base CSS smooth scrolling, Node import specifier in Vitest config, workspace display names, .gitignore entry for package-lock.json, and linter rule adjustments in biome.jsonc. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant H as Heading (clicked)
participant G as ActiveHeading Store
participant O as Other Headings
participant W as Window Location
U->>H: Click anchor
H->>H: Prevent default
H->>G: setActive(id)
G-->>H: notify(isActive=true)
G-->>O: notify(isActive=false)
H->>W: Update hash (#id)
Note over H,O: Anchor visibility toggles based on isActive
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot It's good but for some reason, pressing the pound symbol scrolls way past the header. It doesn't actually anchor around the header but about 3-4 lines below it. Also, I'd expect smooth scrolling when the user taps it. Also, I'd expect that symbol to be invisible and only be visible when tapping the header itself or the area where the pound symbol should be (on the left of the header) @copilot |
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
I've addressed all three issues you mentioned:
The anchor links now work properly on both desktop (hover to show) and mobile (tap heading area to show), with correct scroll positioning and smooth transitions. Desktop behavior: Fixed in commit 7511cac. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot its still scrolling too much. Also, now the pound appears as long as I hold the header or the pound area but once I let go, it disappears. I need it to stay visible when I let go and only disappear when I tap somewhere else like a different heading. |
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
|
@copilot we still need to scroll less (i.e. more space from the top) , because there's a menu bar and a navbar usually. also, the behavior is still not as expected. i don't want the pound to be 50% visible on mobile. i want it to be invisible, and become visible when tapping the heading. then, when tapping the pound symbol, the scroll should happen. |
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: yamcodes <2014360+yamcodes@users.noreply.github.com>
... Fixed both issues:
The anchors now behave exactly as requested - invisible by default on mobile, become visible when tapping the heading, and scroll when tapping the pound symbol. Fixed in commit 7072760. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…igation - Added state management to track active heading links. - Implemented useEffect to handle clicks outside of headings, resetting active state. - Updated anchor link behavior to prevent default and navigate to the corresponding section. Also updated vitest configuration to use Node.js import protocol and added a new rule in biome.jsonc for Node.js import protocol warnings.
- Introduced "use client" directive to enable client-side rendering for the Heading component, ensuring proper functionality in a React environment.
- Added global state to track the active heading ID and listeners for updates. - Updated useEffect to manage active state based on clicks outside headings. - Enhanced anchor click behavior to set the active heading and navigate to the corresponding section.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.gitignore (1)
7-7: OK to ignore package-lock.json; confirm no npm-only examples rely on it.If any example apps intentionally use npm, consider scoping ignore with a negation in those folders.
Example:
package-lock.json +!examples/**/package-lock.jsonapps/www/components/ui/heading.tsx (4)
28-53: Multiple document click listeners per heading; centralize to one.Each heading adds a document click handler; scale risk on long pages. Maintain one global listener tied to headingListeners size.
I can provide a small module-level refactor if you want it in this PR.
54-59: Consider replaceState to avoid history spam (optional).Setting location.hash adds history entries on repeated clicks. replaceState preserves smooth scroll without stacking entries.
- window.location.hash = `#${id}`; + history.replaceState(null, "", `#${id}`);
35-37: Initialize from current hash (optional UX).If users land on a URL with a hash, reveal that heading’s #.
- // Set initial state - setIsActive(activeHeadingId === id); + // Set initial state + setIsActive(activeHeadingId === id || (id ? window.location.hash === `#${id}` : false));
82-83: tabIndex not needed on anchor.Anchor with href is focusable by default; redundant.
- tabIndex={0}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)apps/www/app/docs/[[...slug]]/page.tsx(1 hunks)apps/www/app/styles/base.css(1 hunks)apps/www/components/ui/heading.tsx(1 hunks)apps/www/vitest.config.ts(1 hunks)arkenv.code-workspace(1 hunks)biome.jsonc(1 hunks)
🔇 Additional comments (7)
arkenv.code-workspace (1)
25-25: Display name tweaks LGTM.Also applies to: 29-29
apps/www/vitest.config.ts (1)
1-1: Switch to node: protocol aligns with lint rule.Consistent with useNodejsImportProtocol.
biome.jsonc (2)
55-59: AI summary inconsistency; rule set changes LGTM.noInferrableTypes still present (Line 54) contrary to the AI summary stating it was removed.
55-59: Enforce node: protocol across repo.
Enable "useNodejsImportProtocol" in biome.jsonc (lines 55–59), run Biome's safe autofix (e.g., pnpm -s biome check --write or npx @biomejs/biome check --write), and confirm there are no remaining bare core imports (fs, path, http, etc.) — convert them to node:fs, node:path, node:http as needed.apps/www/components/ui/heading.tsx (2)
71-71: Scroll offset check.scroll-mt-40 = 10rem (~160px). Confirm this covers banner + navbar across breakpoints and zoom.
Manual checks:
- Desktop, md/lg/xl
- Mobile Safari/Chrome with sticky headers
- With/without banner present
68-89: PR claim vs behavior.PR text says anchors “do not remain persistently visible,” but code keeps them visible until click-outside (which matches yamcodes’ request). Update PR description or behavior for consistency.
apps/www/app/docs/[[...slug]]/page.tsx (1)
63-63: Confirm source.getPage returns a stringpathused by EditOnGithub
- EditOnGithub expects a string prop
path(apps/www/components/page/edit-on-github.tsx:9–21).- page.tsx passes
page.path(apps/www/app/docs/[[...slug]]/page.tsx:63), but the generated ~/.source (docs.toFumadocsSource) wasn't present in the repo so I could not verify the page object shape. Ensure source.getPage returnspathfor all docs; otherwise pass the correct property (e.g.,page.file.path) or update EditOnGithub to accept the actual path property.
| html { | ||
| scroll-behavior: smooth; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Respect reduced‑motion preferences for smooth scroll.
Wrap smooth scrolling to avoid motion for users with prefers-reduced-motion.
- html {
- scroll-behavior: smooth;
- }
+@media (prefers-reduced-motion: no-preference) {
+ html {
+ scroll-behavior: smooth;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| html { | |
| scroll-behavior: smooth; | |
| } | |
| @media (prefers-reduced-motion: no-preference) { | |
| html { | |
| scroll-behavior: smooth; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/www/app/styles/base.css around lines 6 to 8, the rule always enables
smooth scrolling which ignores users' prefers-reduced-motion setting; update the
CSS to only apply scroll-behavior: smooth when prefers-reduced-motion:
no-preference (and ensure reduced-motion users get no smooth scrolling) by
wrapping the html { scroll-behavior: smooth; } rule inside a @media
(prefers-reduced-motion: no-preference) block so users who prefer reduced motion
do not receive smooth scrolling.
| <Component | ||
| id={id} | ||
| className={`group relative scroll-mt-40 ${className || ""}`} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mobile tap on heading does not reveal the #; add container onClick.
Current state changes only on anchor click. Per product request, tapping the heading should reveal the #.
- <Component
+ <Component
id={id}
- className={`group relative scroll-mt-40 ${className || ""}`}
+ onClick={() => setActiveHeading(id ?? null)}
+ className={`group relative scroll-mt-40 ${className || ""}`}
{...props}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Component | |
| id={id} | |
| className={`group relative scroll-mt-40 ${className || ""}`} | |
| {...props} | |
| > | |
| <Component | |
| id={id} | |
| onClick={() => setActiveHeading(id ?? null)} | |
| className={`group relative scroll-mt-40 ${className || ""}`} | |
| {...props} | |
| > |
🤖 Prompt for AI Agents
In apps/www/components/ui/heading.tsx around lines 69-73 the wrapper <Component>
has no onClick so tapping the heading on mobile does not reveal the anchor/hash;
add an onClick handler on this container that invokes the same logic used by the
anchor (either programmatically clicks the inner anchor or sets the same "show
anchor" state/function) so a tap on the heading reveals the #; ensure the
handler short-circuits if the original event target is the anchor itself to
avoid double navigation and preserve accessibility (keyboard focus/ARIA)
behavior.
| className={`select-none text-primary no-underline absolute -left-5 transition-opacity duration-200 ${ | ||
| isActive | ||
| ? "opacity-100" | ||
| : "opacity-0 hover:opacity-100 group-hover:opacity-100 focus:opacity-100" | ||
| }`} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid invisible, clickable anchor; gate pointer events until visible.
Prevents accidental scroll on mobile when the hidden anchor is tapped; still clickable on hover/active.
- className={`select-none text-primary no-underline absolute -left-5 transition-opacity duration-200 ${
- isActive
- ? "opacity-100"
- : "opacity-0 hover:opacity-100 group-hover:opacity-100 focus:opacity-100"
- }`}
+ className={`select-none text-primary no-underline absolute -left-5 transition-opacity duration-200 ${
+ isActive
+ ? "opacity-100 pointer-events-auto"
+ : "opacity-0 pointer-events-none group-hover:opacity-100 group-hover:pointer-events-auto focus:opacity-100 focus:pointer-events-auto"
+ }`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| className={`select-none text-primary no-underline absolute -left-5 transition-opacity duration-200 ${ | |
| isActive | |
| ? "opacity-100" | |
| : "opacity-0 hover:opacity-100 group-hover:opacity-100 focus:opacity-100" | |
| }`} | |
| className={`select-none text-primary no-underline absolute -left-5 transition-opacity duration-200 ${ | |
| isActive | |
| ? "opacity-100 pointer-events-auto" | |
| : "opacity-0 pointer-events-none group-hover:opacity-100 group-hover:pointer-events-auto focus:opacity-100 focus:pointer-events-auto" | |
| }`} |
🤖 Prompt for AI Agents
In apps/www/components/ui/heading.tsx around lines 76 to 80, the anchor is
invisible but still clickable which causes accidental taps; update the classes
to disable pointer events when the anchor is hidden and enable them when
visible. Specifically, add pointer-events-none to the hidden-state class branch
and pointer-events-auto to the visible/hover/focus/active branches so the
element only receives pointers when opacity makes it visible.
The heading anchor links now work properly with correct scroll positioning that accounts for the banner and navbar, smooth transitions, and appropriate mobile behavior where anchors are hidden by default and only appear when tapping the heading.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit