Open
Conversation
dosubot
Bot
added
the
size:L
WalkthroughAdds a reusable top navigation include ( Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 6/10 reviews remaining, refill in 18 minutes and 23 seconds. Comment |
dosubot
Bot
added
area/o11ytsdb
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review ✅ ApprovedIntegrates site navigation into the TSDB learn pages to ensure consistent documentation flow. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
![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 the current code and only fix it if needed.
Inline comments:
In `@site/tsdb-engine/learn/index.html`:
- Around line 144-155: The header block (header with classes "topbar panel", the
a.brand element and nav.topnav links) is duplicated across learn pages — extract
this markup into a single reusable partial (e.g., _topbar.html or a
project-standard include) and replace the duplicated block in each page with the
include invocation; ensure the partial contains the brand anchor and nav items
exactly as in the extracted block and update any relative URLs or attributes if
needed so all pages render the same shared topbar.
In `@site/tsdb-engine/learn/shared.css`:
- Around line 122-142: The .topnav a ruleset lacks a keyboard focus style; add a
.topnav a:focus-visible selector to make focus visible and consistent across
browsers by mirroring the hover/current visual cues (set color to `#fff` and
border-color to var(--xp-accent)) and add an accessible focus ring (e.g., a
subtle box-shadow or outline that respects the existing border-radius) while
keeping outline: none only if you provide the visible ring; update the CSS
adjacent to the existing .topnav a and .topnav a:hover rules so keyboard users
see clear focus on .topnav a via :focus-visible.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 04a3f4fc-6639-4745-932c-492dc1d5b3a6
📒 Files selected for processing (8)
site/tsdb-engine/learn/alp/index.htmlsite/tsdb-engine/learn/chunk-stats/index.htmlsite/tsdb-engine/learn/delta-of-delta/index.htmlsite/tsdb-engine/learn/index.htmlsite/tsdb-engine/learn/query-engine/index.htmlsite/tsdb-engine/learn/shared.csssite/tsdb-engine/learn/string-interning/index.htmlsite/tsdb-engine/learn/xor-delta/index.html
strawgate
force-pushed
the
codex/site-polish-followup
branch
from
55ff240 to
6a724a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site/tsdb-engine/learn/_topbar.html`:
- Around line 1-12: The header block containing the duplicated navigation (the
<header> element with class "topbar panel" and the <nav> element with class
"topnav") is duplicated across pages; extract that markup into a single shared
include/partial and replace the duplicated sections with a reference to that
include so all pages reuse the same source of truth for the topbar; update the
files that currently contain the <header class="topbar panel"> / <nav
class="topnav"> markup to import the new shared partial (ensuring aria-label and
link content are preserved) so future edits to the topbar are made in one place.
In `@site/tsdb-engine/vite.config.ts`:
- Around line 5-14: The injected topbar HTML (learnTopbar) contains hardcoded
anchors and bypasses the Vite config base, so update the plugin's
transformIndexHtml (the o11ykit-learn-topbar block) to compute the runtime base
(const base = process.env.BASE_PATH ?? "/o11ykit/tsdb-engine/"), normalize
trailing slash, then replace occurrences in learnTopbar (or a placeholder token)
that point to "/o11ykit/tsdb-engine/" with that computed base before returning
the HTML; use the learnTopbar variable and perform a simple string replace or
templating step so all href/src anchors in the injected topbar follow the
configured base.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5abf7cc6-6bfd-4fb1-97bb-91feb840c06c
📒 Files selected for processing (10)
site/tsdb-engine/learn/_topbar.htmlsite/tsdb-engine/learn/alp/index.htmlsite/tsdb-engine/learn/chunk-stats/index.htmlsite/tsdb-engine/learn/delta-of-delta/index.htmlsite/tsdb-engine/learn/index.htmlsite/tsdb-engine/learn/query-engine/index.htmlsite/tsdb-engine/learn/shared.csssite/tsdb-engine/learn/string-interning/index.htmlsite/tsdb-engine/learn/xor-delta/index.htmlsite/tsdb-engine/vite.config.ts
Comment on lines
+1
to
+12
| <header class="topbar panel"> | ||
| <a class="brand" href="/o11ykit/"><img class="brand-mark" src="/o11ykit/logo.svg" width="20" height="20" alt="" aria-hidden="true" />o11ykit</a> | ||
| <nav class="topnav" aria-label="Primary"> | ||
| <a href="/o11ykit/otlpkit-docs/">OtlpKit</a> | ||
| <a href="/o11ykit/tsdb-engine/" aria-current="page">TSDB Engine</a> | ||
| <a href="/o11ykit/tracesdb-engine/">TracesDB</a> | ||
| <a href="/o11ykit/logsdb-engine/">LogsDB</a> | ||
| <a href="/o11ykit/octo11y/">Octo11y</a> | ||
| <a href="/o11ykit/benchkit/">Benchkit</a> | ||
| <a href="https://github.com/strawgate/o11ykit" target="_blank" rel="noreferrer">GitHub</a> | ||
| </nav> | ||
| </header> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate duplicated topbar markup to prevent drift.
Line [1]-Line [12] duplicates the global nav already present in site/tsdb-engine/index.html (Line [34]-Line [45]). Reusing this shared include there too will avoid link-label/style drift across TSDB pages.
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/tsdb-engine/learn/_topbar.html` around lines 1 - 12, The header block
containing the duplicated navigation (the <header> element with class "topbar
panel" and the <nav> element with class "topnav") is duplicated across pages;
extract that markup into a single shared include/partial and replace the
duplicated sections with a reference to that include so all pages reuse the same
source of truth for the topbar; update the files that currently contain the
<header class="topbar panel"> / <nav class="topnav"> markup to import the new
shared partial (ensuring aria-label and link content are preserved) so future
edits to the topbar are made in one place.
Comment on lines
+5
to
+14
| const learnTopbar = readFileSync(resolve(__dirname, "learn/_topbar.html"), "utf8"); | ||
|
|
||
| export default defineConfig({ | ||
| base: process.env.BASE_PATH ?? "/o11ykit/tsdb-engine/", | ||
| plugins: [ | ||
| { | ||
| name: "o11ykit-learn-topbar", | ||
| transformIndexHtml(html) { | ||
| return html.replaceAll("<!-- @include learn-topbar -->", learnTopbar); | ||
| }, |
There was a problem hiding this comment.
Topbar injection bypasses BASE_PATH, which can break nav links on non-default deploy roots.
Line [8] makes base configurable, but Line [13] injects _topbar.html with hardcoded /o11ykit/... anchors. If BASE_PATH changes, topbar links can route to the wrong host path.
Proposed fix (parameterize topbar root during injection)
-import { readFileSync } from "node:fs";
+import { readFileSync } from "node:fs";
import { resolve } from "node:path";
import { defineConfig } from "vite";
-const learnTopbar = readFileSync(resolve(__dirname, "learn/_topbar.html"), "utf8");
+const basePath = process.env.BASE_PATH ?? "/o11ykit/tsdb-engine/";
+const normalizedBase = basePath.endsWith("/") ? basePath : `${basePath}/`;
+const o11ykitRoot = new URL("../", `https://example.invalid${normalizedBase}`).pathname;
+const learnTopbarTemplate = readFileSync(resolve(__dirname, "learn/_topbar.html"), "utf8");
export default defineConfig({
- base: process.env.BASE_PATH ?? "/o11ykit/tsdb-engine/",
+ base: basePath,
plugins: [
{
name: "o11ykit-learn-topbar",
transformIndexHtml(html) {
- return html.replaceAll("<!-- `@include` learn-topbar -->", learnTopbar);
+ const learnTopbar = learnTopbarTemplate.replaceAll("__O11YKIT_ROOT__", o11ykitRoot);
+ return html.replaceAll("<!-- `@include` learn-topbar -->", learnTopbar);
},
},
],-<a class="brand" href="/o11ykit/"><img class="brand-mark" src="/o11ykit/logo.svg" width="20" height="20" alt="" aria-hidden="true" />o11ykit</a>
+<a class="brand" href="__O11YKIT_ROOT__"><img class="brand-mark" src="__O11YKIT_ROOT__logo.svg" width="20" height="20" alt="" aria-hidden="true" />o11ykit</a>
...
-<a href="/o11ykit/otlpkit-docs/">OtlpKit</a>
+<a href="__O11YKIT_ROOT__otlpkit-docs/">OtlpKit</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const learnTopbar = readFileSync(resolve(__dirname, "learn/_topbar.html"), "utf8"); | |
| export default defineConfig({ | |
| base: process.env.BASE_PATH ?? "/o11ykit/tsdb-engine/", | |
| plugins: [ | |
| { | |
| name: "o11ykit-learn-topbar", | |
| transformIndexHtml(html) { | |
| return html.replaceAll("<!-- @include learn-topbar -->", learnTopbar); | |
| }, | |
| const learnTopbarTemplate = readFileSync(resolve(__dirname, "learn/_topbar.html"), "utf8"); | |
| const basePath = process.env.BASE_PATH ?? "/o11ykit/tsdb-engine/"; | |
| const normalizedBase = basePath.endsWith("/") ? basePath : `${basePath}/`; | |
| const o11ykitRoot = new URL("../", `https://example.invalid${normalizedBase}`).pathname; | |
| export default defineConfig({ | |
| base: basePath, | |
| plugins: [ | |
| { | |
| name: "o11ykit-learn-topbar", | |
| transformIndexHtml(html) { | |
| const learnTopbar = learnTopbarTemplate.replaceAll("__O11YKIT_ROOT__", o11ykitRoot); | |
| return html.replaceAll("<!-- `@include` learn-topbar -->", learnTopbar); | |
| }, | |
| }, | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/tsdb-engine/vite.config.ts` around lines 5 - 14, The injected topbar
HTML (learnTopbar) contains hardcoded anchors and bypasses the Vite config base,
so update the plugin's transformIndexHtml (the o11ykit-learn-topbar block) to
compute the runtime base (const base = process.env.BASE_PATH ??
"/o11ykit/tsdb-engine/"), normalize trailing slash, then replace occurrences in
learnTopbar (or a placeholder token) that point to "/o11ykit/tsdb-engine/" with
that computed base before returning the HTML; use the learnTopbar variable and
perform a simple string replace or templating step so all href/src anchors in
the injected topbar follow the configured base.
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
Validation