Fix #3543 TOC sidebar scrollability for long articles#310
Fix #3543 TOC sidebar scrollability for long articles#310anchalsahani wants to merge 5 commits intokeploy:mainfrom
Conversation
Signed-off-by: unknown <sahanianchal7@gmail.com> Signed-off-by: Anchal Sahani <sahanianchal7@gmail.com>
anchalsahani
force-pushed
the
fix/toc-scrollbar-3543
branch
from
f723b63 to
779e51c
Compare
|
hi @amaan-bhati I noticed an existing PR addressing the TOC scrollbar behavior. This PR extends that work by:
Happy to rebase or align this with the existing PR if you’d prefer a single consolidated fix. |
…en avatars Signed-off-by: Anchal Sahani <sahanianchal7@gmail.com>
Signed-off-by: Anchal Sahani <sahanianchal7@gmail.com>
Signed-off-by: Anchal Sahani <sahanianchal7@gmail.com>
|
Hi @Achanandhi-M I’ve fixed the build issues and pushed the updates. I’ve fixed the build issues and pushed the updates. |
Signed-off-by: Anchal Sahani <sahanianchal7@gmail.com>
|
Hi @anchalsahani However, the issue mentioned in this PR has already been fixed in the repository. Because of that, we won’t be able to proceed with merging this one. Feel free to look into other open issues in the repository and pick one that hasn’t been addressed yet. We’d be happy to review another contribution from you. Thanks again for your effort! |
|
Thanks for the update @dhananjay6561 I appreciate the review and will look into other open issues in the repository and contribute there. Thanks! |
|
Hey @anchalsahani 👋 — thanks for putting this PR together, we appreciate the effort! We've gone ahead and requested a Copilot review on this. Here's some context from the reviewer:
Once you've had a chance to go through the comments, please address the feedback and resolve the threads — and we'll get this across the line. Feel free to ask if anything's unclear. Happy coding! 💙 |
There was a problem hiding this comment.
Pull request overview
Fixes TOC sidebar behavior for long articles by constraining TOC height and enabling immediate scrolling, while also adding scroll-spy highlighting and TOC auto-scroll-to-active behavior.
Changes:
- Make the desktop TOC container scrollable immediately via
max-h-[80vh] overflow-y-autoand sticky positioning. - Add active-section tracking using
IntersectionObserver, plus auto-scrolling the TOC to keep the active item visible. - Update tweet/testimonial avatar handling and refresh several tweet avatar URLs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| services/Tweets.tsx | Updates stored tweet avatar URLs. |
| components/tweets.tsx | Adds client-side tweet card rendering tweaks, avatar proxying logic, and fallback handling. |
| components/testimonials.tsx | Refactors testimonial cards to use next/image, adds avatar fallback/proxy logic, and adjusts markup. |
| components/TableContents.tsx | Implements bounded-height scrollable TOC, scroll-spy active highlighting, and TOC auto-scroll. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fallbackAvatar = | ||
| "https://abs.twimg.com/sticky/default_profile_images/default_profile_400x400.png"; | ||
|
|
||
| const Tweets = ({ avatar, name, id, post, content }) => { | ||
| const { basePath } = useRouter(); | ||
| const isExternal = typeof avatar === "string" && /^https?:\/\//i.test(avatar); | ||
| const proxiedAvatar = isExternal | ||
| ? `${basePath}/api/proxy-image?url=${encodeURIComponent(avatar)}` | ||
| : avatar; | ||
|
|
||
| let imgSrc = avatar || fallbackAvatar; | ||
|
|
||
| const isExternal = /^https?:\/\//i.test(imgSrc); | ||
| const isTwitterCDN = | ||
| imgSrc.includes("pbs.twimg.com") || imgSrc.includes("abs.twimg.com"); | ||
|
|
||
| if (isExternal && !isTwitterCDN) { | ||
| imgSrc = `${basePath}/api/proxy-image?url=${encodeURIComponent(imgSrc)}`; | ||
| } |
There was a problem hiding this comment.
fallbackAvatar points to abs.twimg.com, but the repo’s next.config.js only allowlists pbs.twimg.com for next/image and the proxy route allowlist also excludes abs.twimg.com (see pages/api/proxy-image.ts). As written, any missing/failed avatar will switch to a URL that Next Image will reject at runtime. Use a local fallback (e.g., a file in /public) or update the image/proxy allowlists and (if keeping unoptimized) CSP accordingly.
| src={src} | ||
| alt={`${name} avatar`} | ||
| width={48} | ||
| height={48} | ||
| className="rounded-full" | ||
| unoptimized | ||
| onError={() => setSrc(fallbackAvatar)} | ||
| loading="lazy" |
There was a problem hiding this comment.
unoptimized forces the browser to request https://pbs.twimg.com/... directly, but the site-wide CSP in next.config.js currently sets img-src to 'self' (plus a couple domains) and does not include pbs.twimg.com/abs.twimg.com. This will likely block avatar images in production. Either remove unoptimized (so images are served from /_next/image on the same origin) or always proxy Twitter CDN avatars through /api/proxy-image.
| const fallbackAvatar = | ||
| "https://abs.twimg.com/sticky/default_profile_images/default_profile_400x400.png"; | ||
|
|
||
| const firstRow = Tweets.slice(0, Tweets.length / 2); | ||
| const secondRow = Tweets.slice(Tweets.length / 2); | ||
|
|
||
| const ReviewCard = ({ | ||
| avatar, | ||
| name, | ||
| id, | ||
| content, | ||
| post, | ||
| }: { | ||
| avatar: string; | ||
| name: string; | ||
| post: string; | ||
| id: string; | ||
| content: string; | ||
| }) => { | ||
| const ReviewCard = ({ avatar, name, id, content, post }) => { | ||
| const { basePath } = useRouter(); | ||
| const isExternal = typeof avatar === "string" && /^https?:\/\//i.test(avatar); | ||
| const proxiedAvatar = isExternal ? `${basePath}/api/proxy-image?url=${encodeURIComponent(avatar)}` : avatar; | ||
|
|
||
| // Validate post link | ||
| if (!post || !post.startsWith("https://")) return null; | ||
|
|
||
| let imgSrc = avatar || fallbackAvatar; | ||
|
|
||
| const isExternal = /^https?:\/\//i.test(imgSrc); | ||
| const isTwitterCDN = | ||
| imgSrc.includes("pbs.twimg.com") || imgSrc.includes("abs.twimg.com"); | ||
|
|
||
| // Proxy only non-twitter external images | ||
| if (isExternal && !isTwitterCDN) { | ||
| imgSrc = `${basePath}/api/proxy-image?url=${encodeURIComponent(imgSrc)}`; | ||
| } |
There was a problem hiding this comment.
fallbackAvatar uses abs.twimg.com, but next.config.js only includes pbs.twimg.com in images.domains, so the initial render (when avatar is falsy) or the onError fallback will throw “hostname not configured” in next/image. Prefer a local fallback image in /public or add abs.twimg.com to the Next Image + proxy allowlists if you want to keep it remote.
| const ReviewCard = ({ avatar, name, id, content, post }) => { | ||
| const { basePath } = useRouter(); |
There was a problem hiding this comment.
This file is .tsx, but the ReviewCard prop types were removed. Reintroducing the explicit prop typing (or using a typed Tweet interface shared with services/Tweets.tsx) will prevent accidental shape mismatches and improve refactor safety.
| useEffect(() => { | ||
| if (!tocContainerRef.current) return; | ||
|
|
||
| function resizeHandler() { | ||
| setIsList(container.clientHeight > window.innerHeight * 0.8); | ||
| } | ||
| const resizeHandler = () => { | ||
| setIsList( | ||
| tocContainerRef.current!.clientHeight > | ||
| window.innerHeight * 0.8 | ||
| ); | ||
| }; | ||
|
|
||
| resizeHandler() | ||
| window.addEventListener("resize", resizeHandler) | ||
| resizeHandler(); | ||
| window.addEventListener("resize", resizeHandler); | ||
| return () => window.removeEventListener("resize", resizeHandler); | ||
| }, [setIsList]); |
There was a problem hiding this comment.
The “auto-switch list mode” effect only depends on setIsList, so it runs before headings are populated (they’re set asynchronously in post-body.tsx) and won’t recompute when the TOC grows/shrinks unless the window is resized. Include headings in the dependency list and/or recompute after render (or use a ResizeObserver) so isList reflects the actual TOC height on initial load.
| <button | ||
| onClick={() => setIsDropdownOpen((p) => !p)} | ||
| className="w-full px-4 py-2 bg-white border rounded-md flex justify-between" | ||
| > | ||
| <span className="font-semibold">Table of Contents</span> | ||
| <span>{isDropdownOpen ? "▲" : "▼"}</span> | ||
| </button> |
There was a problem hiding this comment.
The mobile TOC toggle button removed the previous aria-expanded / aria-controls attributes, which is an accessibility regression for screen readers. Please restore those ARIA attributes (and ensure the controlled element has a stable id).
| }, | ||
| { | ||
| avatar: | ||
| "https://pbs.twimg.com/profile_images/1422864637532332033/mC1Nx0vj_400x400.jpg", | ||
| "https://pbs.twimg.com/profile_images/2006390396847628288/cSHW1_MM_400x400.jpg", | ||
| name: "matsuu@充電期間", | ||
| id: "matsuu", | ||
| post: "https://x.com/matsuu/status/1747448928575099236?s=20", |
There was a problem hiding this comment.
This PR is titled/described as a TOC scrollability fix, but it also changes tweet avatar URLs and substantially refactors tweet/testimonial components. These changes look unrelated to issue #3543 and make the PR harder to review/rollback; consider splitting the tweet/testimonial updates into a separate PR (or updating the PR description to cover them).
Fixes #3543
🐞 Fix: TOC sidebar scrollability for long articles
Summary
This PR fixes an issue where the Table of Contents (TOC) sidebar was not scrollable on initial page load for long articles and only became scrollable after scrolling the main content.
As a result, TOC items below the viewport were inaccessible on first render, leading to inconsistent and confusing navigation.
Root cause
This caused the TOC to behave differently before and after scrolling the page.
What this PR changes
Additional improvements
IntersectionObserverfor efficient scroll-spy behaviorImplementation details
max-heightrelative to the viewportoverflow-y: autoIntersectionObserverscrollIntoView({ block: "nearest" })Expected outcome
Screenshots / Videos
Screen.Recording.2026-01-29.134545.mp4
Checklist