Addon-Docs: Change URL hash when TOC item is clicked, and fix TOC loading bugs #30130

Sidnioulz · 2024-12-23T23:51:01Z

Continuation of #23618.

This PR was done to help finalise a contribution from @sookmax. The original PR updated the URL when clicking on a ToC item or scrolling to a new ToC section.

What I did

I made the following changes to the original PR:

TableOfContents: The URL change on scroll was reverted, as requested by @JReinhold in the original review
TableOfContents: Missing useEffect hook dependencies were added in the ToC component
Manager API::url: Added location hash to internal navigation events so it's preserved on initial load and when globalsUpdated fires
nit: The git history was rewritten to better qualify and scope changes

As a result, the URL no longer updates when users scroll, and the page now correctly loads on the right docs subsection when loading Storybook with a location hash set.

Checklist for Contributors

Testing

Caution

The original PR did not introduce tests, and neither does this one. I'd like to be advised on how best to proceed. I've tried, without much conviction, to build a stories file but I'd need to mock the channel for this and I don't know how to.

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

Checkout the repo, compile, and run storybook:ui
Open http://localhost:6006/?path=/docs/core-tags-config--docs#inheritance in your browser
Notice how a scroll event occurs and takes you to the relevant section

Section IDs with a space need to be URL encoded. The ID generated in MDX already contains %20, so that needs to be encoded to %2520 to load properly.

Checkout as above
Open http://localhost:6006/?path=/docs/core-tags-config--docs#tag%20removal this time
Notice how you do not get scrolled to the relevant section
Open http://localhost:6006/?path=/docs/core-tags-config--docs#tag%2520removal this time
Notice that now, it loads

Note

If maintainers know where I can edit the behaviour that generates header IDs in the docs addon, I'll happily adjust it to use _ instead of %20 to help keep URL hashes cleaner.

Caution

The 🔗 links next to headings in the addon-doc also need to be URL encoded so we get the %2520 link that will work with tocbot. I feel the better solution is to adjust all links to use _ rather than further mess with that code. Please let me know how you'd prefer me to proceed.

Documentation

Note

I do not think documentation changes are relevant here.

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

name	before	after	diff	z	%
createSize	0 B	0 B	0 B	-	-
generateSize	77.8 MB	77.8 MB	0 B	1.62	0%
initSize	143 MB	143 MB	1.31 kB	1.46	0%
diffSize	65.6 MB	65.6 MB	1.31 kB	1.45	0%
buildSize	7.19 MB	7.19 MB	397 B	-1.06	0%
buildSbAddonsSize	1.85 MB	1.85 MB	0 B	-0.88	0%
buildSbCommonSize	195 kB	195 kB	0 B	-	0%
buildSbManagerSize	1.87 MB	1.87 MB	192 B	0.55	0%
buildSbPreviewSize	0 B	0 B	0 B	-	-
buildStaticSize	0 B	0 B	0 B	-	-
buildPrebuildSize	3.92 MB	3.92 MB	192 B	-0.88	0%
buildPreviewSize	3.28 MB	3.28 MB	205 B	-1.09	0%
testBuildSize	0 B	0 B	0 B	-	-
testBuildSbAddonsSize	0 B	0 B	0 B	-	-
testBuildSbCommonSize	0 B	0 B	0 B	-	-
testBuildSbManagerSize	0 B	0 B	0 B	-	-
testBuildSbPreviewSize	0 B	0 B	0 B	-	-
testBuildStaticSize	0 B	0 B	0 B	-	-
testBuildPrebuildSize	0 B	0 B	0 B	-	-
testBuildPreviewSize	0 B	0 B	0 B	-	-

name	before	after	diff	z	%
createTime	11.4s	24.9s	13.5s	1.71	🔺54.2%
generateTime	21.3s	21.6s	262ms	0.59	1.2%
initTime	15.3s	16.8s	1.4s	1.54	🔺8.6%
buildTime	8.8s	9.8s	1s	0.82	10.8%
testBuildTime	0ms	0ms	0ms	-	-
devPreviewResponsive	4.8s	4.4s	-389ms	-0.86	-8.8%
devManagerResponsive	3.6s	3.3s	-371ms	-0.94	-11.2%
devManagerHeaderVisible	562ms	557ms	-5ms	-0.66	-0.9%
devManagerIndexVisible	574ms	613ms	39ms	-0.43	6.4%
devStoryVisibleUncached	2s	1.8s	-193ms	-0.11	-10.5%
devStoryVisible	593ms	614ms	21ms	-0.49	3.4%
devAutodocsVisible	457ms	467ms	10ms	-0.72	2.1%
devMDXVisible	485ms	500ms	15ms	-0.55	3%
buildManagerHeaderVisible	637ms	510ms	-127ms	-0.87	-24.9%
buildManagerIndexVisible	730ms	583ms	-147ms	-0.97	-25.2%
buildStoryVisible	626ms	486ms	-140ms	-0.98	-28.8%
buildAutodocsVisible	453ms	399ms	-54ms	-0.86	-13.5%
buildMDXVisible	459ms	400ms	-59ms	-1.07	-14.7%

Greptile Summary

Here's my concise review of the changes focused on fixing Table of Contents URL handling and navigation:

Updates Table of Contents functionality to properly handle URL hash navigation and preserve hash fragments across state changes, focusing on consistent URL behavior.

Added hash preservation in code/core/src/manager-api/modules/url.ts for navigation events and state updates
Fixed URL hash handling in code/lib/blocks/src/components/TableOfContents.tsx by emitting proper NAVIGATE_URL events
Updated useNavigate hook in code/core/src/router/router.tsx to handle empty hash navigation correctly
Added missing cleanup for setTimeout and error handling in code/lib/blocks/src/blocks/DocsContainer.tsx

_{💡 (5/5) You can turn off certain types of comments like style here!}

Co-authored-by: Norbert de Langen <[email protected]>

…section

greptile-apps

_{4 file(s) reviewed, 4 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

code/core/src/manager-api/modules/url.ts

code/lib/blocks/src/components/TableOfContents.tsx

code/core/src/manager-api/modules/url.ts

greptile-apps · 2024-12-23T23:51:40Z

code/core/src/router/router.tsx

    if (typeof to === 'string' && to.startsWith('#')) {
-      document.location.hash = to;
+      if (to === '#') {
+        navigate(document.location.search);
+      } else {
+        document.location.hash = to;
+      }
      return undefined;


logic: this new hash handling could cause issues if document.location.search is empty - consider falling back to '/' or preserving the current path

false positive, prior PR code

nx-cloud · 2024-12-23T23:55:35Z

View your CI Pipeline Execution ↗ for commit f07cf4a.

Command	Status	Duration	Result
`nx affected -t check -c production --parallel=7`	✅ Succeeded	56s	View ↗
`nx run-many -t build -c production --parallel=3`	✅ Succeeded	2m 51s	View ↗

☁️ Nx Cloud last updated this comment at 2024-12-24 00:26:01 UTC

Sidnioulz · 2024-12-24T00:38:39Z

Note the failing unit tests on https://github.com/storybookjs/storybook/actions/runs/12474578164/job/34816789722?pr=30130.

RouterData is supposed to always contain a location object, but it appears to be null in some states. I could address this in the code by using optional chaining when initialising hash, but there might be something to fix in these unit tests' context too. WDYT?

sookmax and others added 3 commits December 23, 2024 17:40

Addon-docs: Change URL hash when ToC item is clicked and when scrolling

f3c6e56

Router: Strengthen detection of empty URL hash

22ae239

Co-authored-by: Norbert de Langen <[email protected]>

Addon-docs: Revert updating the URL hash when scrolling to a new ToC …

2e5f92c

…section

greptile-apps bot reviewed Dec 23, 2024

View reviewed changes

Sidnioulz mentioned this pull request Dec 23, 2024

Addon-Docs: Change URL hash when TOC item is clicked and when scrolling #23618

Closed

5 tasks

Sidnioulz added 2 commits December 24, 2024 01:02

Core: Preserve location hash in URL Core API

2146472

Addon-docs: URL encode anchor IDs when clicking in ToC

c9d5743

Sidnioulz force-pushed the update-23618-fix-toc-onclick branch from 3663df5 to c9d5743 Compare December 24, 2024 00:02

Addon-docs: Make Table of Content click handler safer

f07cf4a

Sidnioulz changed the title ~~Update 23618 fix toc onclick~~ Addon-Docs: Change URL hash when TOC item is clicked, and fix TOC loading bugs Dec 24, 2024

This was referenced Dec 24, 2024

[Bug]: docs-mode table-of-contents does not change URL when clicking on an anchor #26345

Open

[Bug]: hash navigation does not work #29361

Open

Sidnioulz self-assigned this Dec 24, 2024

Sidnioulz added bug core addon: docs ci:normal labels Dec 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addon-Docs: Change URL hash when TOC item is clicked, and fix TOC loading bugs #30130

Addon-Docs: Change URL hash when TOC item is clicked, and fix TOC loading bugs #30130

Sidnioulz commented Dec 23, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Dec 23, 2024

Sidnioulz Dec 24, 2024

nx-cloud bot commented Dec 23, 2024 •

edited

Loading

Sidnioulz commented Dec 24, 2024

Addon-Docs: Change URL hash when TOC item is clicked, and fix TOC loading bugs #30130

Are you sure you want to change the base?

Addon-Docs: Change URL hash when TOC item is clicked, and fix TOC loading bugs #30130

Conversation

Sidnioulz commented Dec 23, 2024 • edited Loading

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Greptile Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

greptile-apps bot Dec 23, 2024

Choose a reason for hiding this comment

Sidnioulz Dec 24, 2024

Choose a reason for hiding this comment

nx-cloud bot commented Dec 23, 2024 • edited Loading

Sidnioulz commented Dec 24, 2024

Sidnioulz commented Dec 23, 2024 •

edited

Loading

nx-cloud bot commented Dec 23, 2024 •

edited

Loading