Skip to content

feat(ui): implement deep linking in CodeTabs via URL hash - #9159

Open
moshams272 wants to merge 8 commits into
nodejs:mainfrom
moshams272:feat/codetabs-deep-linking
Open

moshams272 wants to merge 8 commits into
nodejs:mainfrom
moshams272:feat/codetabs-deep-linking

Conversation

@moshams272

Copy link
Copy Markdown
Contributor

Description

Implement deep linking in CodeTabs component by using the active tab state via the URL hash.

In Details

  • Add optional groupId prop to generate unique anchor IDs across multiple CodeTabs instances on the same page in format: tab-{groupId}-{language}-{index}, falling back to React's useId() when groupId is not provided.
  • Read the URL hash on mount via a lazy initializer in useState() to set the correct initial tab without a double render.
  • Update the URL silently on tab click using history.replaceState to avoid polluting the browser history

Validation

After building, I checked the tabs by clicking on them, and then the URL hash changed.

Related Issues

Fixes #9140

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@moshams272
moshams272 requested a review from a team as a code owner September 13, 2026 17:24
@vercel

vercel Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
nodejs-org Ready Ready Preview Oct 1, 2026 9:07pm UTC

Request Review

@ovflowd ovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to avoid having the CodeTabs being a client-component in all cases. We could use a wrapper that uses use client specifically for when these groupIds or whatnot are passed, ands we can do the logic of what gets used on shiki I guess?

That said per the repo guidelines it is an absolute no-go using winodw APIs from within components, not to mention adding so many event listeners (1 per CodeTab) is going to be an absolute nightmare for the browser.

@AugustinMauroy

Copy link
Copy Markdown
Member
Capture d’écran 2026-09-13 à 22 40 52

I dunno how to reproduce what I have done but it's may happened

@avivkeller avivkeller left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM minus Claudio's concerns

@moshams272

Copy link
Copy Markdown
Contributor Author

We want to avoid having the CodeTabs being a client-component in all cases. We could use a wrapper that uses use client specifically for when these groupIds or whatnot are passed, ands we can do the logic of what gets used on shiki I guess?

That said per the repo guidelines it is an absolute no-go using winodw APIs from within components, not to mention adding so many event listeners (1 per CodeTab) is going to be an absolute nightmare for the browser.

Got it, I saw the use client used in BaseCodeBox first. After your comment, I find the pattern you mean used in AvatarGroup.

I think Augustin's bug happened because of the React Hydration Error in the useState.

@ovflowd

ovflowd commented Sep 14, 2026

Copy link
Copy Markdown
Member

Got it, I saw the use client used in BaseCodeBox first. After your comment, I find the pattern you mean used in AvatarGroup.

We might need to revisit why BaseCodeBox uses "use client" but still we should thrive for reducing client components as much as possible :)

@moshams272

moshams272 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor Author

We might need to revisit why BaseCodeBox uses "use client" but still we should thrive for reducing client components as much as possible :)

I reached for a fair solution. We use useMediaQuery.ts as a custom hook and access the window through it. So, could I do the same?! Or we should revisit it too ;)

It's an initial thinking for now.

@ovflowd

ovflowd commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

I reached for a fair solution. We use useMediaQuery.ts as a custom hook and access the window through it. So, could I do the same?! Or we should revisit it too ;)

In terms of should window APIs be behind safe Hooks, yes. But in terms of, should we have god knows how many Event Listeners because they're tied to one CodeBox? No.

You might want to think of a React Context Provider, with one hook that keeps an eye on Hash changes. Or you might defer it to a HoC, so that on the apps, such as Next.js and doc-kit we defer the implementation of this, for example, Next.js has a Router/Navigation API, you could inject results on the CodeBox...

I guess it's more of a thinking of "who should be responsible for listening to this" you could argue it should be the Component itself, because he's the one interested on this, that said, it's only interested on the scenarios where it should even care abou that. IMO, the Component should subscribe to the changes aka ask to the parent component or context provider "has the hash changed?" ... yet that the responsibility of actually setting up the Event Listener shouldn't be part of the Component's responsibility.... As "Window" or even a Hook that "listens to Window" is something unbeknown to the Component. The Component's "world" should always be anything that is inside said Component. That's a very common mistake some React Components do on, for example, Modals. Where they let the Modal itself be the one observing if anyone clicked outside the Modal, whereas that should never be the Modal's responsibility.

So think a bit of how to approach this, both in a way that is properly designed and that can be reused (ie: if this is a prop/callback to the Component, you defer the implementation details (be it window APIs, be it Next.js APIs...) to the consumer) and how to make it reliable and performant.

I hope my explanation helps!

@moshams272

Copy link
Copy Markdown
Contributor Author
Screencast.from.2026-09-16.02-04-21.webm

I hope I did it right :)

@AugustinMauroy

Copy link
Copy Markdown
Member

that seem cool but on safari and vercel preview I cannot having them working

@moshams272

moshams272 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor Author

that seem cool but on safari and vercel preview I cannot having them working

Yeah, I'll summarize what I did:

  • In our app:

    • Created a hash hook. This hook works on both sides (server & browser). On server or hydration, it'll do nothing that makes the CodeTabs trigger the default tab, so they will not dismatch. On browser, it will take the hash, and I just add one event listener on window, and created a set to rerender the components manually as the React new hook useSyncExternalStore gives me a callback to trigger.
    • Created a hash provider to use this hook and pass this result to ui-components.
  • In ui-components:

    • Created a hash context, why not create it in the app? If we did that, we let our library call the context from the app and we need to make it stanalone library. So who will use this feature should give us the hash through our context.
    • Added an optional groupId feature in CodeTabs. This component works on the server side. However, If you need to use the groupId, the heavy work will still be on the server, and then pass the data to a client component to manage the state and use the context and hash here for sure.

So we haven't been added groupId to any MDX files yet, that means there is no hash needed and the component will fallback to the server component, that is the same for the old one we use now before this PR.

To actually see it works, a <CodeTabs> component needs the new groupId prop. I can push a temporary test snippet to about/index.mdx -that's what I did locally, BTW- so you can test the Safari & Vercel preview. We can simply revert it right before merging.

Comment thread apps/site/hooks/useHash.ts Outdated
import { useSyncExternalStore } from 'react';

const listeners = new Set<() => void>();
let isListening = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we not use a top level let? Use React APIs such a memo() or cache()

Comment thread apps/site/hooks/useHash.ts Outdated

import { useSyncExternalStore } from 'react';

const listeners = new Set<() => void>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, listeners itself should be a top-level cache, not a top level const

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just want to make this hook as a general hook. In our senario, we don't need them as it'll contain only the provider. So I could remove them and just dispatch an event like this:

window.dispatchEvent(new Event('hashchange'));

Instead of using a function that iterates over the set listeners, this keeps the hook clean and focused only on returning hash and setHash without any top-level vars, WDUT?

Comment thread apps/site/hooks/useHash.ts Outdated
};
};

const getSnapshot = () =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use window APIs on ui-components -- use context providers that UI Components use (I guess there's a global one? Like client-context or server-context?) -- otherwise probably have that being passed? I'm unsure if UI components should have environment specific wars. Maybe instead of window.location use at least globalThis or self or have Location be passed from downstream?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you already have a Provider, have the Provider have a prop of location={Location}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and ensure doc-kit and Next.js can pass that with their Navigation/Router APIs natively, or instead of Location they pass History, idk

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I thought of it, but the location on the server will not contain the hash in the URL. This means the server can't read the initial hash to pass it to the Provider, causing an hydration mismatch error. Then we will need to handle it in the provider like this:

const [hash, setHash] = useState('');
useEffect(() => {
  setHash(location.hash);
}, [location.hash]);

and that causes a not needed rerendering. That's why I used the new hook useSyncExternalStore that lets us handle client and server natively without errors or another rendering.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't need to be in the server. But location must come from downstream, not from ui-components.

@ovflowd ovflowd Oct 1, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you don't need to use useState you can use useRef but again, you can store that / retrieve that directly within a React Provider.

@moshams272 moshams272 Oct 1, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think there is a small confusion. Both useHash and the provider actually live in apps/site (the downstream), not in ui-components :)

The ui-component only uses the Context and is completely unaware of window as you taught me before


import type { FC, PropsWithChildren } from 'react';

export const HashProvider: FC<PropsWithChildren> = ({ children }) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Have Location be passed to the HashProvider ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I thought of it, but the location on the server will not contain the hash in the URL. This means the server can't read the initial hash to pass it to the Provider, causing an hydration mismatch error. Then we will need to handle it in the provider like this:

const [hash, setHash] = useState('');
useEffect(() => {
  setHash(location.hash);
}, [location.hash]);

and that causes a not needed rerendering. That's why I used the new hook useSyncExternalStore that lets us handle client and server natively without errors or another rendering.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied your other comment

}, [rawLanguages, rawDisplayNames]);
}, [rawLanguages, rawDisplayNames, groupId]);

const Component = groupId ? CodeTabsWithHash : CodeTabs;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be decided here IMO, but on our rehype shiki plugin. The name of the component to be used (CodeTabs) is passed there, so we can do the right pass directly from there, which reduces verbosity. This also removes the need of regular CodeTabs having an optional Group ID param

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP

return {
key: `${language}-${index}`,
label,
id:

@ovflowd ovflowd Sep 30, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no (as in this is a no-go)

const [activeTab, setActiveTab] = useState(defaultValue);
const [prevHash, setPrevHash] = useState(hash);

if (hash !== prevHash) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be on the root of a Component, that's a big no-go. Use an Effect.

@moshams272

Copy link
Copy Markdown
Contributor Author

@ovflowd Ready for round 2 ;)

This branch was successfully deployed

1 active deployment
Preview — e9021060 Deployed Oct 1, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ui): add anchor linking support to CodeTabs component

4 participants