Implement a new version selection dropdown in the header with custom styling and JavaScript logic.#2106
Open
Sachindu-Nethmin wants to merge 2 commits intowso2:mainfrom
Open
Implement a new version selection dropdown in the header with custom styling and JavaScript logic.#2106Sachindu-Nethmin wants to merge 2 commits intowso2:mainfrom
Sachindu-Nethmin wants to merge 2 commits intowso2:mainfrom
Conversation
…styling and JavaScript logic.
Sachindu-Nethmin
requested review from
anupama-pathirage and
chanikag
as code owners
Contributor
WalkthroughAdds a version selector dropdown to the docs site: new JS fetches version metadata and builds links, a JSON file lists versions, header template inserts the dropdown placeholder, CSS implements the dropdown styling, and MkDocs config loads the JS. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant JS as mitheme.js
participant JSON as versions.json
participant DOM
Browser->>JS: load script (extra_javascript)
JS->>JSON: fetch versions.json (activeRoot)
JSON-->>JS: return versions list & metadata
JS->>JS: sort versions (compareSemVer), build URLs (langRoot/activeRoot rules)
JS->>DOM: populate `#version-select-dropdown` and update links
Browser->>DOM: user clicks dropdown -> navigates to selected version URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
en/theme/material/assets/css/theme.css (1)
1004-1111: Keep the dropdown CSS in one place.Line 26 already imports
partials/_header.css, and this block repeats the same dropdown selectors again. Keeping both copies means every follow-up fix has to be applied twice and the cascade can drift silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@en/theme/material/assets/css/theme.css` around lines 1004 - 1111, This block duplicates dropdown styles already defined in partials/_header.css; remove the duplicated rules from en/theme/material/assets/css/theme.css (the selectors .md-header__version-select, .md-tabs__dropdown-link, .mb-tabs__dropdown, .mb-tabs__dropdown-content and related .md-tabs__item/.md-tabs__link rules) and rely on the canonical definitions in partials/_header.css (or move any unique overrides into that partial); ensure no remaining duplicate selectors remain in theme.css so future fixes are applied only in the partial.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@en/docs/assets/js/mitheme.js`:
- Around line 91-98: The code currently uses a single docSetUrl (derived from
`#page-header`[data-lang] or origin) for both the active site root and language
root which causes incorrect paths like /en/latest/latest...; change this by
introducing two separate bases (e.g., activeRoot for the current docs site —
'/en/latest/' — and langRoot for the language root — '/en/'), keep the existing
localhost override but set both bases appropriately (localhost ->
activeRoot='/en/latest/' and langRoot='/en/'), then update uses of docSetUrl:
use activeRoot when fetching versions.json and building active-site links (the
XMLHttpRequest.open call and the "Show all" link logic) and use langRoot when
composing version target links (the logic around building version URLs on the
versions page). Ensure variable names referenced are docSetUrl, activeRoot, and
langRoot so the changes are easy to find.
- Around line 111-144: The dropdown currently uses data.list.sort() which
performs lexicographic sorting and misorders semantic versions; replace that
call with a numeric semantic-version comparator (e.g., implement a compareSemVer
function that splits version strings by '.' and compares each numeric segment)
and call data.list.sort(compareSemVer) before the forEach; update the sort usage
near the existing data.list.sort() reference in mitheme.js so versions like
"4.10.0" correctly sort after "4.6.0".
In `@en/theme/material/assets/css/partials/_header.css`:
- Around line 213-229: The dropdown's closed state (.mb-tabs__dropdown
.mb-tabs__dropdown-content) only scales to 0 but remains focusable; add
visibility: hidden and pointer-events: none to that selector (or set display:
none) and then restore visibility: visible and pointer-events: auto when the
menu is opened (e.g., add rules under .mb-tabs__dropdown.open
.mb-tabs__dropdown-content) so keyboard users cannot tab into hidden links;
repeat the same change for the other similar dropdown rules referenced in the
review.
---
Nitpick comments:
In `@en/theme/material/assets/css/theme.css`:
- Around line 1004-1111: This block duplicates dropdown styles already defined
in partials/_header.css; remove the duplicated rules from
en/theme/material/assets/css/theme.css (the selectors
.md-header__version-select, .md-tabs__dropdown-link, .mb-tabs__dropdown,
.mb-tabs__dropdown-content and related .md-tabs__item/.md-tabs__link rules) and
rely on the canonical definitions in partials/_header.css (or move any unique
overrides into that partial); ensure no remaining duplicate selectors remain in
theme.css so future fixes are applied only in the partial.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0aa8e813-0d1d-43db-9f50-61d1a1caefdc
📒 Files selected for processing (6)
en/docs/assets/js/mitheme.jsen/docs/versions/assets/versions.jsonen/mkdocs.ymlen/theme/material/assets/css/partials/_header.cssen/theme/material/assets/css/theme.cssen/theme/material/partials/header.html
- Refactor `mitheme.js` to separate `docSetUrl` into `activeRoot` and `langRoot` to ensure correct URL path generation when switching versions and fetching site resources. - Implement numeric semantic version sorting (`compareSemVer`) in `mitheme.js` to accurately order the version dropdown (e.g., sorting 4.10.0 after 4.6.0). - Improve keyboard accessibility in `partials/_header.css` by adding `visibility: hidden` and `pointer-events: none` to closed dropdown menus, preventing hidden links from receiving focus. - Clean up `theme.css` by removing duplicate dropdown styles, maintaining canonical definitions only in `partials/_header.css`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
en/docs/assets/js/mitheme.js (1)
82-97:⚠️ Potential issue | 🔴 Critical
langRootderivation is still broken in production due to missingdata-langattribute.The header template does not render a
data-langattribute (seeheader.htmlline 25), sogetAttribute('data-lang')returnsnull. This causesdocSetLangto become an empty string, makinglangRootequal toorigin + '/'instead of the expectedorigin + '/en/'.The localhost override masks this in development, but in production, version URLs will be malformed (e.g.,
https://mi.docs.wso2.com/latest/...instead ofhttps://mi.docs.wso2.com/en/latest/...).🔧 Suggested fix: derive langRoot from pathname
-var pageHeader = document.getElementById('page-header'); -var docSetLang = pageHeader.getAttribute('data-lang'); - -(window.location.pathname.split('/')[1] !== docSetLang) ? - docSetLang = '' : - docSetLang = docSetLang + '/'; - -var docSetUrl = window.location.origin + '/' + docSetLang; -var langRoot = window.location.origin + '/' + docSetLang; -var activeRoot = window.location.origin + window.location.pathname.substring(0, window.location.pathname.indexOf('/', 4) + 1); +var pathParts = window.location.pathname.split('/').filter(Boolean); +var langSegment = pathParts[0] || 'en'; +var versionSegment = pathParts[1] || 'latest'; + +var langRoot = window.location.origin + '/' + langSegment + '/'; +var activeRoot = window.location.origin + '/' + langSegment + '/' + versionSegment + '/'; if (window.location.hostname === 'localhost' || window.location.hostname === '127.0.0.1') { - docSetUrl = window.location.origin + '/en/latest/'; activeRoot = window.location.origin + '/en/latest/'; langRoot = window.location.origin + '/en/'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@en/docs/assets/js/mitheme.js` around lines 82 - 97, pageHeader.getAttribute('data-lang') can be null, making docSetLang empty and producing wrong langRoot; update the logic around the docSetLang/docSetUrl/langRoot/activeRoot derivation (symbols: pageHeader, docSetLang, langRoot, docSetUrl, activeRoot) to fall back to the first pathname segment when data-lang is missing (e.g., use window.location.pathname.split('/')[1] as the language code if getAttribute('data-lang') is null/empty), normalize to an empty string or "lang/" with trailing slash as before, and then compute docSetUrl/langRoot/activeRoot from that normalized docSetLang so production URLs include the language segment.
🧹 Nitpick comments (1)
en/docs/assets/js/mitheme.js (1)
48-56:indexInParentfunction is defined but never used.This function appears to be dead code. Consider removing it to reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@en/docs/assets/js/mitheme.js` around lines 48 - 56, Remove the dead helper function indexInParent from en/docs/assets/js/mitheme.js: locate the function named indexInParent and delete its entire declaration (the function and its body) since it is defined but never referenced anywhere; confirm there are no remaining callers or exports referencing indexInParent before committing the removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@en/docs/assets/js/mitheme.js`:
- Around line 136-152: The code assumes a fixed "/en/" prefix and incorrectly
tests langRoot for "latest/"; change the pathname slicing to detect the language
segment dynamically (use currentPath.split('/') or a regex to find the first
path segment after the leading slash) and compute pathWithoutVersion by removing
the language segment and taking the remainder (instead of substring(4) and the
fragile index logic), and remove the incorrect langRoot.endsWith('latest/')
branch — instead, if you need to strip a "latest/" version token, check and trim
it on the version string coming from data.all[key].doc (or on the combined base
URL) before concatenation; ensure you still build url using docLinkType,
baseDocUrl (langRoot), data.all[key].doc and the computed pathWithoutVersion.
- Around line 169-210: The DOM-manipulation block guarded by checkVersionsPage
is dead because the template doesn't include the targeted IDs; either remove the
whole "if (checkVersionsPage) { ... }" block (including previousVersions and all
document.getElementById(...) updates for 'previous-versions',
'current-version-number', 'current-version-documentation-link',
'current-version-release-notes-link', 'pre-release-version-documentation-link')
or, if you intend runtime updates, add those exact IDs into the versions.html
Jinja2 template and ensure checkVersionsPage (the
getElementById('current-version-stable') assignment) points to an actual element
so the code executes.
---
Duplicate comments:
In `@en/docs/assets/js/mitheme.js`:
- Around line 82-97: pageHeader.getAttribute('data-lang') can be null, making
docSetLang empty and producing wrong langRoot; update the logic around the
docSetLang/docSetUrl/langRoot/activeRoot derivation (symbols: pageHeader,
docSetLang, langRoot, docSetUrl, activeRoot) to fall back to the first pathname
segment when data-lang is missing (e.g., use
window.location.pathname.split('/')[1] as the language code if
getAttribute('data-lang') is null/empty), normalize to an empty string or
"lang/" with trailing slash as before, and then compute
docSetUrl/langRoot/activeRoot from that normalized docSetLang so production URLs
include the language segment.
---
Nitpick comments:
In `@en/docs/assets/js/mitheme.js`:
- Around line 48-56: Remove the dead helper function indexInParent from
en/docs/assets/js/mitheme.js: locate the function named indexInParent and delete
its entire declaration (the function and its body) since it is defined but never
referenced anywhere; confirm there are no remaining callers or exports
referencing indexInParent before committing the removal.
🪄 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: 8c440b5a-ea9b-4c10-8a0e-744de97add3f
📒 Files selected for processing (3)
en/docs/assets/js/mitheme.jsen/theme/material/assets/css/partials/_header.cssen/theme/material/assets/css/theme.css
🚧 Files skipped from review as they are similar to previous changes (2)
- en/theme/material/assets/css/theme.css
- en/theme/material/assets/css/partials/_header.css
Comment on lines
+136
to
+152
| var currentPath = window.location.pathname; | ||
| // Find the index of '/en/' | ||
| var pathWithoutEn = currentPath.substring(4, currentPath.length); | ||
| var pathWithoutVersion = pathWithoutEn.substring(pathWithoutEn.indexOf("/"), pathWithoutEn.length); | ||
|
|
||
| if (docLinkType === 'https' || docLinkType === 'http') { | ||
| // For external links (older versions), go directly to the configured URL for that version plus the path | ||
| url = data.all[key].doc.replace(/\/$/, "") + pathWithoutVersion; | ||
| } else { | ||
| // For relative internal branches (like 'latest') | ||
| var baseDocUrl = langRoot; | ||
| if (baseDocUrl.endsWith('latest/')) { | ||
| // Trim trailing 'latest/' so we can properly append relative paths from versions.json | ||
| baseDocUrl = baseDocUrl.substring(0, baseDocUrl.length - 7); | ||
| } | ||
| url = baseDocUrl + data.all[key].doc + pathWithoutVersion; | ||
| } |
Contributor
There was a problem hiding this comment.
Hardcoded path offset and confusing langRoot.endsWith('latest/') check.
-
Line 138:
substring(4)assumes the language path is exactly/en/(4 characters). This is fragile if other language codes are used. -
Lines 147-150: The check
langRoot.endsWith('latest/')will never be true sincelangRootshould contain only the language segment (e.g.,/en/), not the version. This appears to be dead code or a logic error.
🔧 Suggested fix
var currentPath = window.location.pathname;
- // Find the index of '/en/'
- var pathWithoutEn = currentPath.substring(4, currentPath.length);
- var pathWithoutVersion = pathWithoutEn.substring(pathWithoutEn.indexOf("/"), pathWithoutEn.length);
+ // Extract path after language and version segments
+ var pathParts = currentPath.split('/').filter(Boolean);
+ // pathParts[0] = language (e.g., 'en'), pathParts[1] = version (e.g., 'latest')
+ var pathWithoutVersion = '/' + pathParts.slice(2).join('/');
if (docLinkType === 'https' || docLinkType === 'http') {
// For external links (older versions), go directly to the configured URL for that version plus the path
url = data.all[key].doc.replace(/\/$/, "") + pathWithoutVersion;
} else {
// For relative internal branches (like 'latest')
- var baseDocUrl = langRoot;
- if (baseDocUrl.endsWith('latest/')) {
- // Trim trailing 'latest/' so we can properly append relative paths from versions.json
- baseDocUrl = baseDocUrl.substring(0, baseDocUrl.length - 7);
- }
- url = baseDocUrl + data.all[key].doc + pathWithoutVersion;
+ url = langRoot + data.all[key].doc + pathWithoutVersion;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@en/docs/assets/js/mitheme.js` around lines 136 - 152, The code assumes a
fixed "/en/" prefix and incorrectly tests langRoot for "latest/"; change the
pathname slicing to detect the language segment dynamically (use
currentPath.split('/') or a regex to find the first path segment after the
leading slash) and compute pathWithoutVersion by removing the language segment
and taking the remainder (instead of substring(4) and the fragile index logic),
and remove the incorrect langRoot.endsWith('latest/') branch — instead, if you
need to strip a "latest/" version token, check and trim it on the version string
coming from data.all[key].doc (or on the combined base URL) before
concatenation; ensure you still build url using docLinkType, baseDocUrl
(langRoot), data.all[key].doc and the computed pathWithoutVersion.
Comment on lines
+169
to
+210
| if (checkVersionsPage) { | ||
| var previousVersions = []; | ||
|
|
||
| Object.keys(data.all).forEach(function(key, index){ | ||
| if ((key !== data.current) && (key !== data['pre-release'])) { | ||
| var docLinkType = data.all[key].doc.split(':')[0]; | ||
| var target = '_self'; | ||
| Object.keys(data.all).forEach(function (key, index) { | ||
| if ((key !== data.current) && (key !== data['pre-release'])) { | ||
| var docLinkType = data.all[key].doc.split(':')[0]; | ||
| var target = '_self'; | ||
|
|
||
| if ((docLinkType == 'https') || (docLinkType == 'http')) { | ||
| target = '_blank' | ||
| } | ||
| if ((docLinkType == 'https') || (docLinkType == 'http')) { | ||
| target = '_blank' | ||
| } | ||
|
|
||
| previousVersions.push('<tr>' + | ||
| '<th>' + key + '</th>' + | ||
| previousVersions.push('<tr>' + | ||
| '<th>' + key + '</th>' + | ||
| '<td>' + | ||
| '<a href="' + data.all[key].doc + '" target="' + | ||
| target + '">Documentation</a>' + | ||
| '<a href="' + data.all[key].doc + '" target="' + | ||
| target + '">Documentation</a>' + | ||
| '</td>' + | ||
| '<td>' + | ||
| '<a href="' + data.all[key].notes + '" target="' + | ||
| target + '">Release Notes</a>' + | ||
| '<a href="' + data.all[key].notes + '" target="' + | ||
| target + '">Release Notes</a>' + | ||
| '</td>' + | ||
| '</tr>'); | ||
| } | ||
| }); | ||
|
|
||
| // Past releases update | ||
| document.getElementById('previous-versions').innerHTML = | ||
| previousVersions.join(' '); | ||
|
|
||
| // Current released version update | ||
| document.getElementById('current-version-number').innerHTML = | ||
| data.current; | ||
| document.getElementById('current-version-documentation-link') | ||
| .setAttribute('href', docSetUrl + data.all[data.current].doc); | ||
| document.getElementById('current-version-release-notes-link') | ||
| .setAttribute('href', docSetUrl + data.all[data.current].notes); | ||
|
|
||
| // Pre-release version update | ||
| document.getElementById('pre-release-version-documentation-link') | ||
| .setAttribute('href', docSetUrl + 'next/'); | ||
| } | ||
|
|
||
| } else { | ||
| console.error("We reached our target server, but it returned an error"); | ||
| } | ||
| '</tr>'); | ||
| } | ||
| }); | ||
|
|
||
| // Past releases update | ||
| document.getElementById('previous-versions').innerHTML = | ||
| previousVersions.join(' '); | ||
|
|
||
| // Current released version update | ||
| document.getElementById('current-version-number').innerHTML = | ||
| data.current; | ||
| document.getElementById('current-version-documentation-link') | ||
| .setAttribute('href', langRoot + data.all[data.current].doc); | ||
| document.getElementById('current-version-release-notes-link') | ||
| .setAttribute('href', langRoot + data.all[data.current].notes); | ||
|
|
||
| // Pre-release version update | ||
| document.getElementById('pre-release-version-documentation-link') | ||
| .setAttribute('href', langRoot + 'next/'); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if any template defines the required IDs for the versions page
echo "=== Searching for required element IDs in templates ==="
rg -n 'id=["\x27](current-version-stable|current-version-number|current-version-documentation-link|current-version-release-notes-link|pre-release-version-documentation-link|previous-versions)["\x27]' --type html
echo ""
echo "=== Contents of versions.html template ==="
fd -t f 'versions.html' --exec cat {}Repository: wso2/docs-mi
Length of output: 2415
Versions page DOM manipulation code is dead—targeted elements don't exist in template.
Lines 169-210 attempt to dynamically update the versions page by manipulating elements with IDs current-version-number, current-version-documentation-link, current-version-release-notes-link, pre-release-version-documentation-link, and previous-versions. However, the versions.html template does not include these IDs anywhere. The template instead uses server-side Jinja2 rendering to generate the entire versions page statically.
Since checkVersionsPage is assigned via document.getElementById('current-version-stable') (line 109) and that element does not exist in the template, it evaluates to null and this entire block never executes. This code serves no purpose and should either be removed or the template should be updated to support dynamic manipulation if that capability is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@en/docs/assets/js/mitheme.js` around lines 169 - 210, The DOM-manipulation
block guarded by checkVersionsPage is dead because the template doesn't include
the targeted IDs; either remove the whole "if (checkVersionsPage) { ... }" block
(including previousVersions and all document.getElementById(...) updates for
'previous-versions', 'current-version-number',
'current-version-documentation-link', 'current-version-release-notes-link',
'pre-release-version-documentation-link') or, if you intend runtime updates, add
those exact IDs into the versions.html Jinja2 template and ensure
checkVersionsPage (the getElementById('current-version-stable') assignment)
points to an actual element so the code executes.
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.
Purpose
Resolves #2094.
The version dropdown in the documentation site was improperly building URLs when navigating to previous versions, leading to continuous version string appending. Additionally, clicking the previous version was incorrectly refreshing the page because the configuration still mapped it to the active environment rather than treating it as an archived release.
Goals
This fix introduces a generic approach to resolve the URL accumulation issue and updates the internal version mapping so 4.6.0 acts as the active
latestrelease.Approach
latest.Release note
Fixes URL accumulation issues in the version navigation dropdown and sets the default active documentation environment to version 4.6.0.
Documentation
N/A - This PR inherently resolves front-end theme bugs regarding the documentation portal framework itself.
Security checks
Test environment
Tested on local development environments executing mkdocs serve to verify that older version combinations successfully redirect outward, and active links reset locally without appending.
Summary by CodeRabbit
New Features
Improvements
Style
Chores