feat(shell): version switcher, archived banner, noindex — #61 phase 4 - #69
Open
mhenrixon wants to merge 3 commits into
Open
feat(shell): version switcher, archived banner, noindex — #61 phase 4#69mhenrixon wants to merge 3 commits into
mhenrixon wants to merge 3 commits into
Conversation
…inks ## Summary Phase 4 of #61 (step 11): DocsUI::VersionSwitcher in the topbar before the repo/social links. The ThemeSwitcher dropdown pattern (daisyUI :focus-within — works with JS off, no new Stimulus controller); renders NOTHING unless versioning_enabled?, so an unversioned topbar is byte-identical. Each entry links the same slug in the target version, falling back to that version's first page (never a 404); the in-scope version is aria-current. ## Test Coverage - version_switcher_spec: empty render when disabled (the pin), one link per version, same-slug mapping both directions, aria-current marking, missing-slug fallback, no-request render, CSS-only (no data-controller) - shell_spec: switcher present with versions, absent without ## Verification - [x] bundle exec rubocop passes - [x] bundle exec rspec passes Refs #61
## Summary Phase 4 of #61 (step 10): the "You are viewing the 1.0 docs" Callout banner, linking the same slug in the current version and falling back to the docs home when the page no longer exists there. data-md-skip keeps it out of the .md twin; absent for current-version or versionless entries. Snapshot::Entry now exposes #version (it already received it) so the banner can tell archived from current. ## Test Coverage - archived_page_spec: banner text + equivalent link, removed-slug fallback, absent on current/versionless entries, data-md-skip - snapshot_spec: Entry#version exposure ## Verification - [x] bundle exec rubocop passes - [x] bundle exec rspec passes Refs #61
## Summary Phase 4 of #61 (step 12): MetaTags#robots_meta emits "noindex, follow" when the in-scope version is noindex'd (the archived default), overridable per version with noindex: false; canonical stays self-referential. Unversioned and current-version renders are unchanged. ## Test Coverage - meta_tags_spec: archived scope → noindex,follow (canonical untouched); current scope unchanged (regression pin); noindex: false restores seo.robots ## Verification - [x] bundle exec rubocop passes - [x] bundle exec rspec passes Refs #61
Collaborator
Author
|
@CodeRabbit fullreview |
|
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThe change adds version metadata to snapshot entries, an archived-page banner, a version switcher in the top bar, and version-scoped robots metadata. Specs cover links, fallbacks, active states, rendering conditions, and canonical behavior. ChangesVersioned documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/docs_ui/version_switcher.rb`:
- Around line 38-43: Normalize the fallback selected by
VersionSwitcher#scope_version so a configuration without an explicit current
version is treated consistently as the current, non-archived version, and add
regression coverage for that configuration. In
app/components/docs_ui/version_switcher.rb lines 38-43, update fallback
selection while preserving the request-scoped version; in lines 59-64, resolve
that selected version from live registry pages. In
app/components/docs_ui/archived_page.rb lines 45-48 and 67-72, use the same
normalized selected-current-version logic so the archived banner is suppressed
and page resolution uses live registry pages.
In `@spec/docs_ui/meta_tags_spec.rb`:
- Around line 177-204: Update the archived-version example around render_tags to
configure a non-empty seo.site_url before rendering, then assert the expected
canonical link URL is present and unchanged alongside the noindex assertion.
Keep the current-version and noindex:false examples unchanged.
🪄 Autofix
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 Plus
Run ID: 79ccd8c6-b3c3-47bf-8948-b87772d788df
📒 Files selected for processing (10)
app/components/docs_ui/archived_page.rbapp/components/docs_ui/meta_tags.rbapp/components/docs_ui/shell.rbapp/components/docs_ui/version_switcher.rblib/docs_kit/snapshot/entry.rbspec/docs_kit/snapshot_spec.rbspec/docs_ui/archived_page_spec.rbspec/docs_ui/meta_tags_spec.rbspec/docs_ui/shell_spec.rbspec/docs_ui/version_switcher_spec.rb
Comment on lines
+38
to
+43
| # The version this render serves: the request scope, else the current | ||
| # version (versioning_enabled? guarantees one exists — with none marked | ||
| # current, the first configured entry is it). | ||
| def scope_version | ||
| DocsKit::Scope.version || config.current_version | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Normalize the fallback current version.
Configuration#current_version selects versions.first when no version has current: true. That selected version still has archived? == true. The switcher then resolves its pages as snapshots, and the archived banner can identify the selected current version as archived.
Either normalize the fallback version to current?, or reject configurations without one explicit current version. Add regression coverage for configured versions with no current: true.
app/components/docs_ui/version_switcher.rb#L38-L43: keep the selected current version and its archive state consistent.app/components/docs_ui/version_switcher.rb#L59-L64: resolve the selected current version from live registry pages.app/components/docs_ui/archived_page.rb#L45-L48: suppress the banner for the selected current version.app/components/docs_ui/archived_page.rb#L67-L72: resolve the selected current version from live registry pages.
📍 Affects 2 files
app/components/docs_ui/version_switcher.rb#L38-L43(this comment)app/components/docs_ui/version_switcher.rb#L59-L64app/components/docs_ui/archived_page.rb#L45-L48app/components/docs_ui/archived_page.rb#L67-L72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/docs_ui/version_switcher.rb` around lines 38 - 43, Normalize
the fallback selected by VersionSwitcher#scope_version so a configuration
without an explicit current version is treated consistently as the current,
non-archived version, and add regression coverage for that configuration. In
app/components/docs_ui/version_switcher.rb lines 38-43, update fallback
selection while preserving the request-scoped version; in lines 59-64, resolve
that selected version from live registry pages. In
app/components/docs_ui/archived_page.rb lines 45-48 and 67-72, use the same
normalized selected-current-version logic so the archived banner is suppressed
and page resolution uses live registry pages.
Comment on lines
+177
to
+204
| describe "robots under a version scope" do | ||
| it "emits noindex, follow for an archived version (canonical untouched)" do | ||
| archived = DocsKit::DocVersion.new(id: "1.0") | ||
|
|
||
| html = DocsKit::Scope.with(version: archived) { render_tags } | ||
|
|
||
| expect(html).to include('<meta name="robots" content="noindex, follow">') | ||
| expect(html).not_to include('rel="canonical"') | ||
| end | ||
|
|
||
| it "keeps today's behavior for the current version in scope (regression pin)" do | ||
| current = DocsKit::DocVersion.new(id: "1.1", current: true) | ||
|
|
||
| html = DocsKit::Scope.with(version: current) { render_tags } | ||
|
|
||
| expect(html).not_to include('name="robots"') | ||
| end | ||
|
|
||
| it "restores seo.robots for a version with noindex: false" do | ||
| DocsKit.configure { |c| c.seo.robots = "index, follow" } | ||
| opted_out = DocsKit::DocVersion.new(id: "1.0", noindex: false) | ||
|
|
||
| html = DocsKit::Scope.with(version: opted_out) { render_tags } | ||
|
|
||
| expect(html).to include('<meta name="robots" content="index, follow">') | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'render_tags|site_url|original_url|canonical' spec/docs_ui/meta_tags_spec.rbRepository: mhenrixon/docs-kit
Length of output: 11750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f -i 'meta_tags|scope|doc_version' .
printf '%s\n' '--- relevant definitions and call sites ---'
rg -n -C 12 'def canonical_url|canonical_url|class MetaTags|module MetaTags|def render_tags|site_url|original_url' .Repository: mhenrixon/docs-kit
Length of output: 29225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
spec = Path("spec/docs_ui/meta_tags_spec.rb").read_text()
impl = Path("app/components/docs_ui/meta_tags.rb").read_text()
helper = re.search(
r'def render_tags\(.*?\n end',
spec,
re.S,
)
assert helper, "render_tags helper not found"
helper_text = helper.group(0)
archived = re.search(
r'it "emits noindex, follow for an archived version.*?\n end',
spec,
re.S,
)
assert archived, "archived example not found"
archived_text = archived.group(0)
canonical = re.search(
r'def canonical_url\n(.*?)\n end',
impl,
re.S,
)
assert canonical, "canonical_url not found"
canonical_body = canonical.group(1)
print("render_tags helper configures site_url:", "site_url" in helper_text)
print("archived example configures site_url:", "site_url" in archived_text)
print("canonical_url returns configured site_url:", "return seo.site_url if seo.site_url" in canonical_body)
print("canonical_url omits URL when request? is false:", "return unless request?" in canonical_body)
assert "site_url" not in helper_text
assert "site_url" not in archived_text
assert "return seo.site_url if seo.site_url" in canonical_body
assert "return unless request?" in canonical_body
PYRepository: mhenrixon/docs-kit
Length of output: 347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
spec = Path("spec/docs_ui/meta_tags_spec.rb").read_text()
impl = Path("app/components/docs_ui/meta_tags.rb").read_text()
helper = re.search(r'def render_tags\(.*?\n end', spec, re.S)
archived = re.search(
r'it "emits noindex, follow for an archived version.*?\n end',
spec,
re.S,
)
canonical = re.search(r'def canonical_url\n(.*?)\n end', impl, re.S)
assert helper and archived and canonical
print("render_tags helper configures site_url:", "site_url" in helper.group(0))
print("archived example configures site_url:", "site_url" in archived.group(0))
print("canonical_url uses site_url:", "return seo.site_url if seo.site_url" in canonical.group(1))
print("canonical_url requires request otherwise:", "return unless request?" in canonical.group(1))
PYRepository: mhenrixon/docs-kit
Length of output: 327
Configure a canonical source in the archived-version example.
render_tags has no seo.site_url, and the isolated render has no request. canonical_url therefore returns nil, so the assertion only tests omission. Set DocsKit.configure { |c| c.seo.site_url = ... } and assert the expected canonical URL remains unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/docs_ui/meta_tags_spec.rb` around lines 177 - 204, Update the
archived-version example around render_tags to configure a non-empty
seo.site_url before rendering, then assert the expected canonical link URL is
present and unchanged alongside the noindex assertion. Keep the current-version
and noindex:false examples unchanged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 10 files
Confidence score: 2/5
- In
app/components/docs_ui/archived_page.rb, the equivalent-page lookup calls#slugunconditionally and can raise 500s for registry entries without a slug; the same area can also treat the effective current version as archived and link back to itself, creating confusing navigation — guard slug access and gate archived-banner rendering againstDocsKit.configuration.current_version. - In
app/components/docs_ui/version_switcher.rb, topbar rendering appears to rescan the full page list per configured version just to detect slug presence, which can add avoidable latency on larger archived multi-version docs — precompute/index slug lookups (or cache the mapping) before rendering. - In
spec/docs_ui/version_switcher_spec.rb,spec/docs_ui/meta_tags_spec.rb, andspec/docs_ui/shell_spec.rb, current assertions miss key behavior boundaries (trigger labeling, canonical preservation context, and the single-version threshold), so regressions could slip through even when tests pass — tighten assertions to target the exact element/behavior and add a one-version case. - In
app/components/docs_ui/shell.rb, the render-order comment now disagrees with execution order, which increases maintenance risk during follow-up refactors — update the comment to match the new switcher/topbar sequence.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/components/docs_ui/shell.rb">
<violation number="1" location="app/components/docs_ui/shell.rb:166">
P3: The added VersionSwitcher render now executes before DocsUI::TopbarLinks, so the sibling comment "render as icon-only ghost buttons BEFORE the switcher" is no longer accurate — the topbar links actually render after the switcher. Update that comment (e.g. to "after the switcher") to avoid misleading future readers about render order.</violation>
</file>
<file name="app/components/docs_ui/archived_page.rb">
<violation number="1" location="app/components/docs_ui/archived_page.rb:46">
P2: The effective current version can show an archived banner and link back to itself when no configured version is marked `current: true`; compare the entry with `DocsKit.configuration.current_version` before rendering the banner.</violation>
<violation number="2" location="app/components/docs_ui/archived_page.rb:71">
P1: Archived pages 500 when the current registry entry lacks `#slug`, because the equivalent-page lookup calls it unconditionally; guard the call and use the existing home fallback for entries that cannot be matched.</violation>
</file>
<file name="spec/docs_ui/shell_spec.rb">
<violation number="1" location="spec/docs_ui/shell_spec.rb:100">
P3: The version-switcher spec covers only the unconfigured (0 versions) and two-version cases, but the behavior's real boundary is a single configured version: `versioning_enabled?` is `versions.size > 1`, and the DocVersion/Configuration comments say "A single configured version is not worth a switcher." A site that lists exactly one version is the case most at risk of regressing the byte-identical topbar guarantee and is untested here. Consider adding an example asserting no `version-switcher` renders with `c.versions = [{ id: "1.0", current: true }]`.</violation>
</file>
<file name="spec/docs_ui/version_switcher_spec.rb">
<violation number="1" location="spec/docs_ui/version_switcher_spec.rb:81">
P3: This assertion `include("1.0")` is satisfied by the in-scope dropdown link (also labeled "1.0"), so it would still pass if the trigger button rendered no label — it doesn't test the trigger as the example name implies. Asserting the trigger carries no distinct marker to target (no class/id/testid), consider asserting on the trigger button's text via a more specific selector (e.g. the `role="button"` element / btn trigger content) or adding a data-testid to the trigger so the label is actually pinned.</violation>
</file>
<file name="spec/docs_ui/meta_tags_spec.rb">
<violation number="1" location="spec/docs_ui/meta_tags_spec.rb:184">
P3: This assertion doesn't actually verify the behavior the test/comment claims ('canonical untouched'). In the isolated render there is no `config.seo.site_url` and no request, so `canonical_url` returns nil and no canonical is emitted regardless of the noindex change — `not_to include('rel="canonical"')` passes trivially even before this feature. To pin the documented intent (that noindexing an archived page leaves the self-referential canonical in place), configure `site_url` in this example and assert the canonical IS still present alongside the noindex meta; otherwise the 'canonical untouched' property is untested.</violation>
</file>
<file name="app/components/docs_ui/version_switcher.rb">
<violation number="1" location="app/components/docs_ui/version_switcher.rb:60">
P3: Every topbar render scans the full page list for each configured version just to decide whether a candidate slug exists there (then typically keeps only `pages.first&.href`). On an archived multi-version site this means loading and enumerating each version's entire snapshot on every page request, repeated per version. It would be cheaper and more direct to resolve the target page by slug (e.g. a `from_slug`-style lookup) or to only enumerate lazily when the candidate isn't already known, rather than materializing every page and scanning it.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| config = DocsKit.configuration | ||
| slug = @entry.respond_to?(:slug) ? @entry.slug : nil | ||
| live = slug && DocsKit::LlmsText.pages(config, version: config.current_version) | ||
| .find { |page| page.slug.to_s == slug.to_s } |
There was a problem hiding this comment.
P1: Archived pages 500 when the current registry entry lacks #slug, because the equivalent-page lookup calls it unconditionally; guard the call and use the existing home fallback for entries that cannot be matched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/components/docs_ui/archived_page.rb, line 71:
<comment>Archived pages 500 when the current registry entry lacks `#slug`, because the equivalent-page lookup calls it unconditionally; guard the call and use the existing home fallback for entries that cannot be matched.</comment>
<file context>
@@ -28,16 +25,53 @@ def view_template
+ config = DocsKit.configuration
+ slug = @entry.respond_to?(:slug) ? @entry.slug : nil
+ live = slug && DocsKit::LlmsText.pages(config, version: config.current_version)
+ .find { |page| page.slug.to_s == slug.to_s }
+ live&.href || config.brand_href
+ end
</file context>
Suggested change
| .find { |page| page.slug.to_s == slug.to_s } | |
| .find { |page| page.respond_to?(:slug) && page.slug.to_s == slug.to_s } |
| # current release rendered directly) and for entries carrying no version. | ||
| def banner | ||
| version = entry_version | ||
| return unless version&.archived? |
There was a problem hiding this comment.
P2: The effective current version can show an archived banner and link back to itself when no configured version is marked current: true; compare the entry with DocsKit.configuration.current_version before rendering the banner.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/components/docs_ui/archived_page.rb, line 46:
<comment>The effective current version can show an archived banner and link back to itself when no configured version is marked `current: true`; compare the entry with `DocsKit.configuration.current_version` before rendering the banner.</comment>
<file context>
@@ -28,16 +25,53 @@ def view_template
+ # current release rendered directly) and for entries carrying no version.
+ def banner
+ version = entry_version
+ return unless version&.archived?
+
+ current = DocsKit.configuration.current_version
</file context>
Suggested change
| return unless version&.archived? | |
| return unless version&.archived? && version.id != DocsKit.configuration.current_version&.id |
| div(class: "flex-none items-center") do | ||
| # The docs-version switcher (config.versions) renders first; nothing | ||
| # unless versioning is enabled, so an unversioned topbar is unchanged. | ||
| render DocsUI::VersionSwitcher.new |
There was a problem hiding this comment.
P3: The added VersionSwitcher render now executes before DocsUI::TopbarLinks, so the sibling comment "render as icon-only ghost buttons BEFORE the switcher" is no longer accurate — the topbar links actually render after the switcher. Update that comment (e.g. to "after the switcher") to avoid misleading future readers about render order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/components/docs_ui/shell.rb, line 166:
<comment>The added VersionSwitcher render now executes before DocsUI::TopbarLinks, so the sibling comment "render as icon-only ghost buttons BEFORE the switcher" is no longer accurate — the topbar links actually render after the switcher. Update that comment (e.g. to "after the switcher") to avoid misleading future readers about render order.</comment>
<file context>
@@ -161,6 +161,9 @@ def topbar
div(class: "flex-none items-center") do
+ # The docs-version switcher (config.versions) renders first; nothing
+ # unless versioning is enabled, so an unversioned topbar is unchanged.
+ render DocsUI::VersionSwitcher.new
# Config-driven repo/social links (config.topbar_links) render as
# icon-only ghost buttons BEFORE the switcher; nothing when unset.
</file context>
| expect(html).not_to include("version-switcher") | ||
| end | ||
|
|
||
| it "renders the switcher when two or more versions are configured" do |
There was a problem hiding this comment.
P3: The version-switcher spec covers only the unconfigured (0 versions) and two-version cases, but the behavior's real boundary is a single configured version: versioning_enabled? is versions.size > 1, and the DocVersion/Configuration comments say "A single configured version is not worth a switcher." A site that lists exactly one version is the case most at risk of regressing the byte-identical topbar guarantee and is untested here. Consider adding an example asserting no version-switcher renders with c.versions = [{ id: "1.0", current: true }].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/docs_ui/shell_spec.rb, line 100:
<comment>The version-switcher spec covers only the unconfigured (0 versions) and two-version cases, but the behavior's real boundary is a single configured version: `versioning_enabled?` is `versions.size > 1`, and the DocVersion/Configuration comments say "A single configured version is not worth a switcher." A site that lists exactly one version is the case most at risk of regressing the byte-identical topbar guarantee and is untested here. Consider adding an example asserting no `version-switcher` renders with `c.versions = [{ id: "1.0", current: true }]`.</comment>
<file context>
@@ -81,6 +81,34 @@ def view_template = topbar
+ expect(html).not_to include("version-switcher")
+ end
+
+ it "renders the switcher when two or more versions are configured" do
+ DocsKit.configure do |c|
+ c.versions = [{ id: "1.1", current: true }, { id: "1.0" }]
</file context>
| configure_versions | ||
|
|
||
| DocsKit::Scope.with(version: DocsKit.configuration.version("1.0")) do | ||
| expect(switcher(path: "/1.0/docs/installation").call).to include("1.0") |
There was a problem hiding this comment.
P3: This assertion include("1.0") is satisfied by the in-scope dropdown link (also labeled "1.0"), so it would still pass if the trigger button rendered no label — it doesn't test the trigger as the example name implies. Asserting the trigger carries no distinct marker to target (no class/id/testid), consider asserting on the trigger button's text via a more specific selector (e.g. the role="button" element / btn trigger content) or adding a data-testid to the trigger so the label is actually pinned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/docs_ui/version_switcher_spec.rb, line 81:
<comment>This assertion `include("1.0")` is satisfied by the in-scope dropdown link (also labeled "1.0"), so it would still pass if the trigger button rendered no label — it doesn't test the trigger as the example name implies. Asserting the trigger carries no distinct marker to target (no class/id/testid), consider asserting on the trigger button's text via a more specific selector (e.g. the `role="button"` element / btn trigger content) or adding a data-testid to the trigger so the label is actually pinned.</comment>
<file context>
@@ -0,0 +1,110 @@
+ configure_versions
+
+ DocsKit::Scope.with(version: DocsKit.configuration.version("1.0")) do
+ expect(switcher(path: "/1.0/docs/installation").call).to include("1.0")
+ end
+ end
</file context>
| html = DocsKit::Scope.with(version: archived) { render_tags } | ||
|
|
||
| expect(html).to include('<meta name="robots" content="noindex, follow">') | ||
| expect(html).not_to include('rel="canonical"') |
There was a problem hiding this comment.
P3: This assertion doesn't actually verify the behavior the test/comment claims ('canonical untouched'). In the isolated render there is no config.seo.site_url and no request, so canonical_url returns nil and no canonical is emitted regardless of the noindex change — not_to include('rel="canonical"') passes trivially even before this feature. To pin the documented intent (that noindexing an archived page leaves the self-referential canonical in place), configure site_url in this example and assert the canonical IS still present alongside the noindex meta; otherwise the 'canonical untouched' property is untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/docs_ui/meta_tags_spec.rb, line 184:
<comment>This assertion doesn't actually verify the behavior the test/comment claims ('canonical untouched'). In the isolated render there is no `config.seo.site_url` and no request, so `canonical_url` returns nil and no canonical is emitted regardless of the noindex change — `not_to include('rel="canonical"')` passes trivially even before this feature. To pin the documented intent (that noindexing an archived page leaves the self-referential canonical in place), configure `site_url` in this example and assert the canonical IS still present alongside the noindex meta; otherwise the 'canonical untouched' property is untested.</comment>
<file context>
@@ -174,6 +174,34 @@ def image_url(path) = "https://d.example.com/assets/#{path.sub('.png', '-abc123.
+ html = DocsKit::Scope.with(version: archived) { render_tags }
+
+ expect(html).to include('<meta name="robots" content="noindex, follow">')
+ expect(html).not_to include('rel="canonical"')
+ end
+
</file context>
| # target's first page (guaranteed routable — never a 404); else the | ||
| # target-prefixed docs root (an empty snapshot is already a degraded state). | ||
| def target_href(version) | ||
| pages = DocsKit::LlmsText.pages(config, version: version) |
There was a problem hiding this comment.
P3: Every topbar render scans the full page list for each configured version just to decide whether a candidate slug exists there (then typically keeps only pages.first&.href). On an archived multi-version site this means loading and enumerating each version's entire snapshot on every page request, repeated per version. It would be cheaper and more direct to resolve the target page by slug (e.g. a from_slug-style lookup) or to only enumerate lazily when the candidate isn't already known, rather than materializing every page and scanning it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/components/docs_ui/version_switcher.rb, line 60:
<comment>Every topbar render scans the full page list for each configured version just to decide whether a candidate slug exists there (then typically keeps only `pages.first&.href`). On an archived multi-version site this means loading and enumerating each version's entire snapshot on every page request, repeated per version. It would be cheaper and more direct to resolve the target page by slug (e.g. a `from_slug`-style lookup) or to only enumerate lazily when the candidate isn't already known, rather than materializing every page and scanning it.</comment>
<file context>
@@ -0,0 +1,84 @@
+ # target's first page (guaranteed routable — never a 404); else the
+ # target-prefixed docs root (an empty snapshot is already a degraded state).
+ def target_href(version)
+ pages = DocsKit::LlmsText.pages(config, version: version)
+ candidate = candidate_href(version)
+ return candidate if candidate && pages.any? { |page| page.href == candidate }
</file context>
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.
Phase 4 of #61 — the user-visible version chrome, on top of the #67 foundation: the topbar switcher, the archived-page banner, and archived
noindex. Steps 10–12 of the issue; step 13 (DocsUI::VersionChanges) moves to the phase-5 PR (see deviations).The backwards-compat pin holds: with
c.versionsunset the switcher renders nothing, the banner never appears, androbotsis untouched — each pinned by an explicit spec.What's in
DocsUI::VersionSwitcher(step 11)ThemeSwitcherdropdown pattern copied exactly (tabindex/role=button+dropdown-content) — daisyUI's dropdown is CSS:focus-within, so it works with JavaScript off; every entry is a plain<a>, no new Stimulus controller.Shell#topbarfirst in the right-hand cluster; nothing at all unlessversioning_enabled?.Scope.path_prefix), verified against that version's enumerated pages; a slug missing there falls back to the target's first page — never a 404. The in-scope version is markedaria-current="true"; the trigger shows its label with the lucidelayersicon.DocsUI::ArchivedPagebanner (step 10)Callout(:warning)reading "You are viewing the 1.0 docs. The current version is 1.1 — read it there." linking the same slug in the current version, falling back toconfig.brand_hrefwhen the page no longer exists there.data-md-skip, so it never leaks into the.mdtwin.Snapshot::Entrynow exposes#version(it already received it for href building).MetaTags
noindex(step 12)#robots_metaemitsnoindex, followwhen the in-scope version is noindex'd (the archived default from feat(registry): version scope + snapshot content source — the #61 foundation (phases 1-3) #67'sDocVersion), per-version overridable withnoindex: false; otherwise today's opt-inseo.robotsexactly. Canonical stays self-referential.Test plan
bundle exec rspec— 964 examples, 0 failures (19 new).bundle exec rubocop— 149 files, no offenses.layers.svgverified vendored in the dogfood icon set.Deviations & judgment calls
DocsUI::VersionChanges) deferred to the phase-5 PR: the component's whole surface is theVersionDiffAPI phase 5 defines (#added/#removed/#changed/#diff_for/#compare_url); building it against a guessed API now means rework next PR. It ships with the engine + the changes controller instead./1.0/docsmatches no route — only/1.0/docs/:docexists per the phase-7 route plan). The first enumerated page is the closest guaranteed-routable equivalent; an empty snapshot falls back to the prefixed docs root (already a degraded state).config.brand_hreffor a slug with no current equivalent — the docs home is the only always-valid current-version target.aria-current="true"(semantic, styleable later) rather than a visual-only class — the spec pins the semantics, not a class name.Summary by CodeRabbit
New Features
Bug Fixes
noindex, followfor search engines.