Refactor sidebar persistence logic for better slow device performance #2242
Conversation
🦋 Changeset detectedLatest commit: 1bb06c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
There was a problem hiding this comment.
Really impressive refactor, tested it more and all the reproducible situations that were previously potentially showing a flash of unrestored sidebar no longer exhibit that behavior 👏
Just left a tiny question/suggestion but otherwise this is looking great to me.
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
|
Marked this as ready. I thought a bit more about the |
| <sl-sidebar-state-persist data-hash={hash}> | ||
| <script is:inline> | ||
| (() => { | ||
| try { |
There was a problem hiding this comment.
Is the try/catch needed? I don't see anythink here that is likely to throw.
Also, you can do:
{
const target = ...
}instead of the function wrapper on line 32, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/block#block_scoping_rules_with_let_const_class_or_function_declaration_in_strict_mode
There was a problem hiding this comment.
The try/catch is mainly to defend against the state loading in case sessionStorage is unavailable or for some reason it got some bad value stored to it so the JSON.parse() throws.
Good point about block scoping! Hadn’t considered that before for these inline scripts. I wonder if they’re a little too implicit in what they’re doing? Ultimately both those and the IIFE are not super explicit about locking down scope, but the IIFE feels a little more obvious to me. The {} wrapping almost looks like a mistake if you’re not familiar.
(In the past I’ve only ever really used those in tests, where I wanted to repeatedly declare some variable and test it, so the scoping was much more obvious from the repetition.)
| --- | ||
| /** Unique symbol for storing a running index in `locals`. */ | ||
| const currentGroupIndexSymbol = Symbol.for('starlight-sidebar-group-index'); | ||
| const locals = Astro.locals as Record<typeof currentGroupIndexSymbol, number>; |
* main: (37 commits) [ci] format i18n(ko-KR): update `manual-setup.mdx` (withastro#2294) i18n(ko-KR): update `configuration.mdx` (withastro#2295) [ci] release (withastro#2292) Add support for SSR (withastro#1255) Add Markdoc preset and example (withastro#2249) Refactor sidebar persistence logic for better slow device performance (withastro#2242) [ci] format Add docs.ryzekit.com to showcase (withastro#2291) Update astro dependency to 4.15.3 across monorepo (withastro#2289) [ci] release (withastro#2290) Prevent Zod errors from crashing build (withastro#2288) i18n(fr): update `guides/css-and-tailwind` (withastro#2286) i18n(ko-KR): update `css-and-tailwind.mdx` (withastro#2284) Add WCAG AAA colour contrast option to theme editor (withastro#2282) [ci] release (withastro#2283) Parse `<StarlightPage />` frontmatter asynchronously (withastro#2279) Ensure unhandled directives are restored without any extra whitespace (withastro#2281) i18n(fr): update `resources/plugins` (withastro#2278) i18n(ko-KR): update `plugins.mdx` (withastro#2277) ...
* main: (22 commits) i18n(ru): update `ru/manual-setup.mdx` and `ru/reference/configuration.mdx` (withastro#2307) [ci] format i18n(ru): update some guides (withastro#2306) i18n(fr): update `manual-setup` (withastro#2299) i18n(fr): update `guides/pages` (withastro#2298) [ci] release (withastro#2304) Convert URL to file path correctly for Git virtual module (withastro#2303) i18n(fr): update `reference/configuration` (withastro#2296) i18n(fr): update `guides/authoring-content` (withastro#2297) Update `yummacss.com.png` thumbnail (withastro#2301) i18n(ko-KR): update `pages.mdx` (withastro#2293) [ci] format i18n(ko-KR): update `authoring-content.mdx` (withastro#2300) [ci] format i18n(ko-KR): update `manual-setup.mdx` (withastro#2294) i18n(ko-KR): update `configuration.mdx` (withastro#2295) [ci] release (withastro#2292) Add support for SSR (withastro#1255) Add Markdoc preset and example (withastro#2249) Refactor sidebar persistence logic for better slow device performance (withastro#2242) ...
Description
This PR refactors how the sidebar persistence logic works (yes, already 😅).
Before this PR, we restored state for sidebar groups in a single inline script at the bottom of the sidebar. This is mostly working very well, but on underpowered devices — or, perhaps more importantly, devices momentarily under some strain — this can mean a partial render of the sidebar can be shown before the sidebar is fully rendered, and state is restored only after that. This can lead to a “flash of unrestored sidebar”. For example, here are two frames from a performance trace with 20x CPU slowdown, showing first a partial sidebar with groups expanded and then the restored state with groups collapsed:
The refactor moves the restore logic into a small custom element. Every sidebar group (rendered by
<SidebarSublist>renders this custom element, which triggers the state restore for that group. This means state can be restored incrementally as the sidebar renders instead of once after the full DOM is ready. The only exception is for the scroll position restoration. SettingscrollTopforces style recalc/layout work, so that still happens only once to minimise that. That means on a slow device, scroll restoration can still result in a visible jump in theory but it is overall still much more stable.One detail I’m still looking at is how to generate an index for each sidebar group. Currently this is happening during rendering, but it might make more sense to move it to the sidebar generation logic directly.