fix: eliminate layout shifts on public wiki page load#595
Merged
NagariaHussain merged 2 commits intodevelopfrom Apr 6, 2026
Merged
fix: eliminate layout shifts on public wiki page load#595NagariaHussain merged 2 commits intodevelopfrom
NagariaHussain merged 2 commits intodevelopfrom
Conversation
Three root causes of CLS (Cumulative Layout Shift) on the Jinja-rendered wiki pages: 1. Sidebar width was entirely set by Alpine's :style binding — before Alpine loaded, the sidebar had no width, causing content to reflow. Fixed by adding w-72 as default CSS width and a blocking script that reads the persisted collapse state from localStorage (same pattern as the existing theme initialization). 2. Sidebar tree children were all visible in raw HTML, then collapsed by Alpine, then re-expanded for ancestors — two sequential shifts. Fixed by computing ancestor nodes server-side and passing them to the template, which uses x-cloak on non-ancestor children so they start hidden. 3. Content area had transition-[max-width] with max-w-[80ch]. The ch unit depends on font glyph width — when InterVar loaded via font-display:swap, the ch value changed and the transition animated the content width over 300ms. Fixed by transitioning only opacity. 4. Search shortcut kbd was empty until Alpine's x-text filled it in. Fixed by adding default text content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request adds server-side computation of an 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wiki/templates/wiki/includes/sidebar.html (1)
313-316: Remove debug code before merging.Lines 314-316 contain debug logging that should be removed:
this.$nextTick(() => { const allRouteElements = document.querySelectorAll('[data-route]'); });This queries the DOM but doesn't use the result—appears to be leftover debug code.
♻️ Proposed fix
init() { // Auto-expand parents of current page this.expandCurrentPageParents(); - - // Debug: log all route elements - this.$nextTick(() => { - const allRouteElements = document.querySelectorAll('[data-route]'); - }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` around lines 313 - 316, Remove the leftover debug DOM query: delete the this.$nextTick(...) block that declares the unused allRouteElements constant (the snippet starting with "this.$nextTick" and "const allRouteElements = document.querySelectorAll('[data-route]');"); if you intended to use the nodes, replace the block with the actual logic referencing allRouteElements inside the same this.$nextTick callback, otherwise remove the entire callback to avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 54-59: The persisted key for the sidebar collapse state is missing
an explicit name; update the Alpine store definition (Alpine.store('sidebar'))
so the isCollapsed property uses an explicit persist key by changing
isCollapsed: Alpine.$persist(false) to use .as(...) with the matching key used
by the pre-Alpine script (e.g., .as('sidebar.isCollapsed')), leaving the init()
method that removes data-sidebar-collapsed unchanged.
In `@wiki/templates/wiki/macros/sidebar_tree.html`:
- Around line 54-59: The sidebar can briefly collapse on first render because
expandCurrentPageParents() sets expandedStates in $nextTick; initialize
expandedStates synchronously during wikiSidebar.init() instead so the
server-provided expanded_nodes map is applied before Alpine evaluates x-show;
locate wikiSidebar.init(), expandCurrentPageParents(), and the expandedStates
object and ensure expandedStates is populated from the server-side
expanded_nodes (and respects node IDs used in isExpanded('{{ node.name }}'))
immediately rather than inside a $nextTick callback, removing the timing gap
that causes the flash while keeping x-cloak usage for nodes not in
expanded_nodes.
---
Nitpick comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 313-316: Remove the leftover debug DOM query: delete the
this.$nextTick(...) block that declares the unused allRouteElements constant
(the snippet starting with "this.$nextTick" and "const allRouteElements =
document.querySelectorAll('[data-route]');"); if you intended to use the nodes,
replace the block with the actual logic referencing allRouteElements inside the
same this.$nextTick callback, otherwise remove the entire callback to avoid dead
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7937a40d-44c3-4184-934d-d2afa965b3ee
📒 Files selected for processing (6)
wiki/frappe_wiki/doctype/wiki_document/wiki_document.pywiki/templates/wiki/document.htmlwiki/templates/wiki/includes/header.htmlwiki/templates/wiki/includes/sidebar.htmlwiki/templates/wiki/layout.htmlwiki/templates/wiki/macros/sidebar_tree.html
Comment on lines
54
to
+59
| <div id="{{ node.name }}-children" | ||
| class="wiki-children overflow-hidden mt-0.5 ml-[14px] pl-2 border-l border-[var(--outline-gray-2)]" | ||
| x-show="isExpanded('{{ node.name }}')" | ||
| x-collapse> | ||
| {{ render_wiki_tree(node.children, level + 1, current_route) }} | ||
| x-collapse | ||
| {% if node.name not in expanded_nodes %}x-cloak{% endif %}> | ||
| {{ render_wiki_tree(node.children, level + 1, current_route, expanded_nodes) }} |
There was a problem hiding this comment.
Potential flash for ancestor nodes on first visit.
The logic correctly uses server-computed expanded_nodes to avoid x-cloak on ancestor nodes. However, there's a timing concern:
- Ancestor nodes render visible (no
x-cloak) - Alpine initializes,
x-show="isExpanded(...)"evaluates tofalse(emptyexpandedStates) expandCurrentPageParents()runs in$nextTick, settingexpandedStates[nodeId] = true
Between steps 2 and 3, ancestor nodes might briefly collapse before re-expanding. The x-collapse directive may mitigate this by deferring the animation, and on subsequent visits expandedStates is persisted so this only affects first-time visitors.
Consider initializing expandedStates synchronously (not in $nextTick) in wikiSidebar.init() to eliminate any potential flash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wiki/templates/wiki/macros/sidebar_tree.html` around lines 54 - 59, The
sidebar can briefly collapse on first render because expandCurrentPageParents()
sets expandedStates in $nextTick; initialize expandedStates synchronously during
wikiSidebar.init() instead so the server-provided expanded_nodes map is applied
before Alpine evaluates x-show; locate wikiSidebar.init(),
expandCurrentPageParents(), and the expandedStates object and ensure
expandedStates is populated from the server-side expanded_nodes (and respects
node IDs used in isExpanded('{{ node.name }}')) immediately rather than inside a
$nextTick callback, removing the timing gap that causes the flash while keeping
x-cloak usage for nodes not in expanded_nodes.
The blocking script and Alpine store were coupled via Alpine's implicit
key format (_x_isCollapsed). Use an explicit .as('sidebar.isCollapsed')
key so the two stay in sync regardless of property renames or Alpine
internals.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
w-72) and read persisted collapse state from localStorage in a blocking<script>before render — same pattern as the existing theme initializationx-cloakon non-ancestor children so they start hidden instead of flashing visible then collapsingmax-widthfrom transition — thechunit changes when the custom font loads viafont-display: swap, causing a visible 300ms animated width shift⌘Ktext so the kbd element isn't empty before Alpine loadsCloses #591
Summary by CodeRabbit
Bug Fixes
Improvements