Merged
Conversation
Update tsconfig lib to ES2022 for error.cause support and fix invalid IconButton props (size/variant) to match Apsara component types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add theme registry for config-driven theme selection and implement paper theme with chapter nav, serif content, reading progress ruler, and prev/next navigation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restructure paper layout with sticky navbar (arrows + breadcrumb + search), paper-style content area with shadow/borders, neutral background, and full-height reading progress ruler. Remove PrevNextNav component, add className prop to Search. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Navbar and ReadingProgress use position fixed. Layout content is the scroll container with scrollbar at browser edge. Neutral background on sidebar and content area. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaces simple heading list with Apsara-style ruler TOC featuring tick marks, scroll marker with percentage, heading labels on hover, and connecting lines. Monospace font, fixed positioning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add heading margins/sizes, paragraph spacing, list indentation, and blockquote styling for proper content readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Paper Layout supports classNames prop for overrides - ChapterNav handles nested folders recursively for API endpoints - Truncate sidebar text with ellipsis - Hidden Search in API layout for Cmd+K support - API layout removes right padding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new "paper" theme (Layout, Page, ChapterNav, ReadingProgress + CSS), introduces a theme registry and dynamic theme selection, wires Search className through, updates a few IconButton props, and removes next-env declarations from the chronicle package. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/chronicle/src/components/api/field-row.tsx (1)
66-71:⚠️ Potential issue | 🟡 MinorIcon-only
IconButtons are missingaria-labelBoth
IconButtonelements wrap purely visual icons (PlusIcon,TrashIcon) with no accessible name. Screen readers will announce them as unlabelled buttons.key-value-editor.tsxalready establishes the correct pattern (aria-label={\Delete ${entry.key || 'entry'}`}`); the same treatment is needed here.♿ Proposed fix
- <IconButton size={1} onClick={() => { + <IconButton size={1} aria-label={`Add item to ${field.name}`} onClick={() => { const newItem = itemChildren ? {} : '' onChange?.(field.name, [...items, newItem]) }}> <PlusIcon width={14} height={14} /> </IconButton>- <IconButton size={1} onClick={() => { + <IconButton size={1} aria-label={`Remove item ${i + 1} from ${field.name}`} onClick={() => { const updated = items.filter((_, j) => j !== i) onChange?.(field.name, updated) }}> <TrashIcon width={14} height={14} /> </IconButton>Also applies to: 108-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/components/api/field-row.tsx` around lines 66 - 71, The IconButton elements wrapping purely visual icons (PlusIcon and TrashIcon) lack accessible names; update the PlusIcon button in the add handler and the TrashIcon button (the other occurrence around the remove handler) to include descriptive aria-labels (e.g., for the add button use aria-label={`Add ${field.name || 'entry'}`} and for the delete button use aria-label={`Delete ${entry.key || 'entry'}`} or similar), so that IconButton, PlusIcon, TrashIcon and surrounding handlers (onChange, itemChildren, field.name, items, entry) mirror the accessible pattern used in key-value-editor.tsx.
🧹 Nitpick comments (11)
packages/chronicle/src/app/apis/[[...slug]]/layout.tsx (1)
4-5: Dynamic theming and hidden Search wiring look correct.
getTheme(config.theme?.name)is a safe, fallback-guarded lookup, and a Server Component rendering a Client Component (Search) is valid in the App Router.One minor point:
Searchmounts withallowEmpty: trueanddelayMs: 100, souseDocsSearchfires an initial fetch to/api/searchon every API page load even though the button isdisplay: none. This is an extra network round-trip that's never surfaced to the user until Cmd+K is pressed. If load reduction on API pages matters, you could defer the fetch until the dialog opens (e.g., by not rendering<Search>at all and instead only attaching thekeydownlistener directly in a lightweight client component).Also applies to: 10-10, 21-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/app/apis/`[[...slug]]/layout.tsx around lines 4 - 5, The Search component mounts on every API page and immediately triggers useDocsSearch due to allowEmpty: true and delayMs: 100, causing an unnecessary initial fetch; to fix, stop rendering <Search> on server pages by replacing its immediate render with a lightweight client-only keyboard-listener component (e.g., a small client component that attaches the Cmd+K / keydown handler and only mounts/returns the full Search component when the dialog is opened), or change the search initialization so useDocsSearch is only invoked on open; update references around Search, useDocsSearch, allowEmpty and delayMs to ensure the dialog-triggering client component instantiates Search lazily.packages/chronicle/src/themes/paper/Layout.module.css (1)
9-15:padding-right: 260pxhardcodes the sidebar width — consider a CSS custom property.Both
.sidebar { width: 260px }and.content { padding-right: 260px }share the same magic number. If the sidebar width ever changes, both must be updated manually.♻️ Suggested refactor using a CSS custom property
+:root { + --paper-sidebar-width: 260px; +} + .sidebar { - width: 260px; + width: var(--paper-sidebar-width); padding: var(--rs-space-7) var(--rs-space-5); ... } .content { flex: 1; overflow-y: auto; background: var(--rs-color-background-neutral-primary); - padding-right: 260px; + padding-right: var(--paper-sidebar-width); }Also applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/Layout.module.css` around lines 9 - 15, The hard-coded 260px "magic number" is duplicated between .sidebar and .content; introduce a CSS custom property (e.g., --sidebar-width) at the top of Layout.module.css and use it for .sidebar { width: var(--sidebar-width, 260px); } and .content { padding-right: var(--sidebar-width, 260px); } so changing the sidebar width requires updating only the custom property; update any other occurrences (lines noted around .sidebar and .content) to reference the same variable.packages/chronicle/src/themes/paper/ChapterNav.module.css (1)
34-36: Redundantlist-style: noneon.item.
.chapterItems(the parent<ul>) already setslist-style: none, so the declaration on.item(<li>) has no additional effect.♻️ Suggested cleanup
-.item { - list-style: none; -} +.item {}Or simply remove the
.itemrule if it has no other declarations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ChapterNav.module.css` around lines 34 - 36, The .item rule in ChapterNav.module.css is redundant because the parent .chapterItems already sets list-style: none; remove the list-style: none declaration from the .item selector (or delete the entire .item rule if it contains no other declarations) so styling is not duplicated; update any references to .item in the component only if you remove the selector entirely to ensure no dependent styles are lost.packages/chronicle/src/themes/paper/Page.module.css (1)
73-78:margin-top: 72pxon.articleis a magic number tied to the fixed navbar height.The fixed navbar is
height: 40px+padding: var(--rs-space-3) 0. If either value changes,72pxwill need a manual update. Consider deriving it from a CSS custom property or usingpadding-topwith a CSS variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/Page.module.css` around lines 73 - 78, Replace the hard-coded "margin-top: 72px" on the .article rule with a value derived from the navbar's size using a CSS custom property (e.g., --navbar-height) or a calc() that references --rs-space-3 and the fixed navbar height, so the spacing updates automatically when the navbar's height or padding changes; update the .article selector to use that CSS variable (or calc expression) and ensure the fixed navbar sets/exports the same --navbar-height variable so both elements stay in sync.packages/chronicle/src/themes/paper/Page.tsx (2)
42-48: Consider memoizing derived navigation data.
flattenTree,findIndex, andfindBreadcrumbrun on every render. While tree sizes in documentation sites are typically small, wrapping these inuseMemokeyed on[tree, pathname, page.slug]would prevent redundant computation during unrelated re-renders (e.g., from ReadingProgress state changes propagating up).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/Page.tsx` around lines 42 - 48, Derived navigation values in Page (pages, currentIndex, prev, next, crumbs) are recomputed on every render; wrap the computations that call flattenTree and findBreadcrumb in a useMemo so they only recompute when inputs change. Specifically, memoize pages = flattenTree(tree.children) and derive currentIndex, prev, next from pages and pathname inside a single useMemo keyed on [tree, pathname, page.slug], and memoize crumbs = findBreadcrumb(tree.children, page.slug) (or include it in the same memo) to avoid redundant work during unrelated re-renders; update the Page component to import/use React.useMemo and replace the direct calls to flattenTree/findBreadcrumb accordingly.
55-72: Disabled arrows use<span>— not focusable, but acceptable for disabled state.The disabled prev/next arrows render as
<span>elements, which aren't keyboard-focusable. Since they represent disabled navigation, this is a reasonable trade-off. If full ARIA compliance is desired later, consider using<button disabled>oraria-disabledlinks instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/Page.tsx` around lines 55 - 72, The disabled arrow rendering currently uses non-semantic <span> elements (in the prev/next conditional blocks around NextLink and ChevronLeftIcon/ChevronRightIcon), which harms accessibility; replace those <span className={styles.arrowDisabled}> elements with proper disabled controls — either a <button disabled className={styles.arrowDisabled} aria-label="Previous" or "Next"> wrapping the Chevron*Icon, or keep an anchor but add aria-disabled="true" and tabIndex={-1} to the non-link state so screenreaders and keyboard users get correct semantics; update the prev/next conditional branches that reference prev, next, NextLink, styles.arrowDisabled, ChevronLeftIcon and ChevronRightIcon accordingly.packages/chronicle/src/themes/paper/ChapterNav.tsx (1)
23-31: Mutable counter inside render's.map()callback.
chapterIndexis mutated inside.map(), which is a side-effect during rendering. While it works here because the variable is local and re-initialized every render, it's an anti-pattern in React's functional rendering model. A cleaner approach would be to pre-compute the folder count or usereduce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ChapterNav.tsx` around lines 23 - 31, The code mutates chapterIndex inside the render loop (tree.children.map) in ChapterNav; remove this side-effect by precomputing folder indices before returning JSX: iterate tree.children once (e.g., with Array.prototype.reduce or a for loop) to build a list or map of folder items to their sequential chapter numbers, then in the render map use that precomputed index (referencing chapterIndex logic, tree.children, and the folder branch) instead of incrementing a variable during mapping. Ensure the new logic preserves skipping 'separator' items and only counts items with type === 'folder' and children.packages/chronicle/src/themes/paper/ReadingProgress.module.css (1)
70-80: Inconsistent font-family declarations.Line 73 hardcodes
'SF Mono', 'Fira Code', monospacewhile.headingLabel(line 62) uses the design-system variablevar(--rs-font-mono). Consider using the CSS variable here as well for consistency.Proposed fix
.headingLink { color: var(--rs-color-foreground-base-primary); text-align: right; - font-family: 'SF Mono', 'Fira Code', monospace; + font-family: var(--rs-font-mono); font-size: var(--rs-font-size-mini);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.module.css` around lines 70 - 80, Replace the hardcoded font stack in the .headingLink rule with the design-system variable used by .headingLabel: use var(--rs-font-mono) for font-family instead of "'SF Mono', 'Fira Code', monospace" so both .headingLink and .headingLabel consistently reference the same mono font variable.packages/chronicle/src/themes/paper/ReadingProgress.tsx (3)
119-163: Direct DOM manipulation inhandleScrollbypasses React's rendering model — acceptable for performance but worth documenting.The imperative
classListandstyle.topmanipulation in the scroll handler avoids React re-renders on every scroll event, which is a valid performance optimization. Consider adding a brief comment (e.g.,// Imperative updates to avoid re-render on scroll) to signal intent to future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx` around lines 119 - 163, handleScroll currently performs imperative DOM updates (style.top, classList, querySelector) to avoid React re-renders on every scroll; add a concise inline comment above the function (or at the top of handleScroll) explaining this is intentional for performance (e.g., "// Imperative updates to avoid re-render on scroll") and reference the rationale and the related refs (ARTICLE_SELECTOR, containerRef, scrollMarkerRef, scrollPosRef, snapToTick) so future maintainers understand this is deliberate rather than a bug.
248-257: Connecting line width can reach 0px for deeply nested headings.
(3 - h.level) * 4 + 12yields0pxatdepth=6. If TOC items at depth 5+ are possible, the connecting line disappears. If depth > 6, the width goes negative (though browsers clamp to 0).Consider a
Math.maxguard:- width: `${(3 - h.level) * 4 + 12}px`, + width: `${Math.max(4, (3 - h.level) * 4 + 12)}px`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx` around lines 248 - 257, The connecting line width calculation for headings in the headings.map render can produce 0 or negative widths for deep levels; update the inline style on the element with className styles.connectingLine (the block that maps headings and uses h.level) to clamp the width using Math.max, e.g. Math.max((3 - h.level) * 4 + 12, 4) and append "px" so the width never falls below a sensible minimum.
165-180: Hardcoded 250ms initialization delay is fragile.The
setTimeout(…, 250)assumes the article DOM is ready within 250ms. If content loads slower (e.g., lazy images, code blocks, or server component streaming), heading positions will be calculated against incomplete layout. Theresizelistener partially mitigates this, but doesn't cover content reflows.Consider using a
ResizeObserveron the article element instead, which would reliably trigger recalculation when the article dimensions settle.Sketch using ResizeObserver
useEffect(() => { - const init = setTimeout(() => { - recalcHeadings() - handleScroll() - setReady(true) - }, 250) + // Initial calc + recalcHeadings() + handleScroll() + setReady(true) + + // Re-calc when article layout changes + const article = document.querySelector(ARTICLE_SELECTOR) + let ro: ResizeObserver | undefined + if (article) { + ro = new ResizeObserver(() => { + recalcHeadings() + handleScroll() + }) + ro.observe(article) + } window.addEventListener('resize', recalcHeadings) window.addEventListener('scroll', handleScroll, { passive: true }) return () => { - clearTimeout(init) + ro?.disconnect() window.removeEventListener('resize', recalcHeadings) window.removeEventListener('scroll', handleScroll) } }, [recalcHeadings, handleScroll])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx` around lines 165 - 180, Replace the fragile setTimeout-based init with a ResizeObserver watching the article element so heading positions are recalculated when layout changes: find the element used to derive headings (e.g., via articleRef or document.querySelector('article')) and create a ResizeObserver that calls recalcHeadings() and handleScroll() (optionally debounced and then calls setReady(true) once dimensions have stabilized), attach the observer instead of relying on the 250ms timer, and on cleanup disconnect the observer; keep the existing window scroll listener but remove the hardcoded setTimeout and clearTimeout usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chronicle/next-env.d.ts`:
- Line 3: This Next.js-generated file next-env.d.ts is tracked but flips imports
between .next/dev/types and .next/types causing git noise; add an entry for
next-env.d.ts to the package's .gitignore so the auto-generated file is not
committed (keep next-env.d.ts in tsconfig.json include as required).
In `@packages/chronicle/src/themes/paper/ChapterNav.tsx`:
- Line 46: Top-level <li> elements are being rendered directly inside <nav> by
the ChapterItem usage in ChapterNav (the line returning ChapterItem for each
tree.children item); wrap the mapped output in a proper list container or ensure
ChapterItem only emits <li> when wrapped by a <ul>/<ol>. Concretely, modify
ChapterNav so the tree.children.map(...) output is enclosed in a <ul> (and
adjust any top-level chapter wrappers to become <li> with nested <ul> for
children) or alter ChapterItem to render a non-<li> element for top-level
non-folder items and only render <li> when inside a list, so all <li> elements
are descendants of a <ul> or <ol>.
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 119-122: The CSS selectors .content h5 and .content h6 in
Page.module.css only set margins and rely on browser defaults for font-size; add
explicit font-size declarations for both selectors to match the paper theme's
typographic scale (choose sizes consistent with existing h1–h4 definitions in
this file) so h5 and h6 no longer inherit inconsistent browser defaults; update
the .content h5, .content h6 rule to include the chosen font-size values and
adjust line-height if needed for visual consistency.
- Around line 7-18: The fixed .navbar is starting at the viewport edge and
overlapping the 260px sidebar; update the .navbar rules in Page.module.css (the
.navbar selector) to offset it from the left by the sidebar width and constrain
its width accordingly (e.g., set left: 260px and adjust width to calc(100% -
260px) and/or max-width to calc(1024px - 260px)); if a CSS variable for the
sidebar width exists (e.g., --sidebar-width), use that instead of hardcoding
260px.
In `@packages/chronicle/src/themes/paper/ReadingProgress.module.css`:
- Around line 30-36: The two CSS classes tickLineBefore and tickLineAfter in
ReadingProgress.module.css are identical, so the scroll progress ticks never
visually change; update tickLineBefore to use the "read"/accent color (e.g., the
cobalt/accent token used elsewhere) instead of
var(--rs-color-background-neutral-tertiary) so the component in
ReadingProgress.tsx (which toggles these classes) shows read vs unread ticks;
locate the tickLineBefore and tickLineAfter selectors in
ReadingProgress.module.css and change tickLineBefore's background-color to the
appropriate accent variable (matching the scroll marker color) while leaving
tickLineAfter as the neutral color.
---
Outside diff comments:
In `@packages/chronicle/src/components/api/field-row.tsx`:
- Around line 66-71: The IconButton elements wrapping purely visual icons
(PlusIcon and TrashIcon) lack accessible names; update the PlusIcon button in
the add handler and the TrashIcon button (the other occurrence around the remove
handler) to include descriptive aria-labels (e.g., for the add button use
aria-label={`Add ${field.name || 'entry'}`} and for the delete button use
aria-label={`Delete ${entry.key || 'entry'}`} or similar), so that IconButton,
PlusIcon, TrashIcon and surrounding handlers (onChange, itemChildren,
field.name, items, entry) mirror the accessible pattern used in
key-value-editor.tsx.
---
Nitpick comments:
In `@packages/chronicle/src/app/apis/`[[...slug]]/layout.tsx:
- Around line 4-5: The Search component mounts on every API page and immediately
triggers useDocsSearch due to allowEmpty: true and delayMs: 100, causing an
unnecessary initial fetch; to fix, stop rendering <Search> on server pages by
replacing its immediate render with a lightweight client-only keyboard-listener
component (e.g., a small client component that attaches the Cmd+K / keydown
handler and only mounts/returns the full Search component when the dialog is
opened), or change the search initialization so useDocsSearch is only invoked on
open; update references around Search, useDocsSearch, allowEmpty and delayMs to
ensure the dialog-triggering client component instantiates Search lazily.
In `@packages/chronicle/src/themes/paper/ChapterNav.module.css`:
- Around line 34-36: The .item rule in ChapterNav.module.css is redundant
because the parent .chapterItems already sets list-style: none; remove the
list-style: none declaration from the .item selector (or delete the entire .item
rule if it contains no other declarations) so styling is not duplicated; update
any references to .item in the component only if you remove the selector
entirely to ensure no dependent styles are lost.
In `@packages/chronicle/src/themes/paper/ChapterNav.tsx`:
- Around line 23-31: The code mutates chapterIndex inside the render loop
(tree.children.map) in ChapterNav; remove this side-effect by precomputing
folder indices before returning JSX: iterate tree.children once (e.g., with
Array.prototype.reduce or a for loop) to build a list or map of folder items to
their sequential chapter numbers, then in the render map use that precomputed
index (referencing chapterIndex logic, tree.children, and the folder branch)
instead of incrementing a variable during mapping. Ensure the new logic
preserves skipping 'separator' items and only counts items with type ===
'folder' and children.
In `@packages/chronicle/src/themes/paper/Layout.module.css`:
- Around line 9-15: The hard-coded 260px "magic number" is duplicated between
.sidebar and .content; introduce a CSS custom property (e.g., --sidebar-width)
at the top of Layout.module.css and use it for .sidebar { width:
var(--sidebar-width, 260px); } and .content { padding-right:
var(--sidebar-width, 260px); } so changing the sidebar width requires updating
only the custom property; update any other occurrences (lines noted around
.sidebar and .content) to reference the same variable.
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 73-78: Replace the hard-coded "margin-top: 72px" on the .article
rule with a value derived from the navbar's size using a CSS custom property
(e.g., --navbar-height) or a calc() that references --rs-space-3 and the fixed
navbar height, so the spacing updates automatically when the navbar's height or
padding changes; update the .article selector to use that CSS variable (or calc
expression) and ensure the fixed navbar sets/exports the same --navbar-height
variable so both elements stay in sync.
In `@packages/chronicle/src/themes/paper/Page.tsx`:
- Around line 42-48: Derived navigation values in Page (pages, currentIndex,
prev, next, crumbs) are recomputed on every render; wrap the computations that
call flattenTree and findBreadcrumb in a useMemo so they only recompute when
inputs change. Specifically, memoize pages = flattenTree(tree.children) and
derive currentIndex, prev, next from pages and pathname inside a single useMemo
keyed on [tree, pathname, page.slug], and memoize crumbs =
findBreadcrumb(tree.children, page.slug) (or include it in the same memo) to
avoid redundant work during unrelated re-renders; update the Page component to
import/use React.useMemo and replace the direct calls to
flattenTree/findBreadcrumb accordingly.
- Around line 55-72: The disabled arrow rendering currently uses non-semantic
<span> elements (in the prev/next conditional blocks around NextLink and
ChevronLeftIcon/ChevronRightIcon), which harms accessibility; replace those
<span className={styles.arrowDisabled}> elements with proper disabled controls —
either a <button disabled className={styles.arrowDisabled} aria-label="Previous"
or "Next"> wrapping the Chevron*Icon, or keep an anchor but add
aria-disabled="true" and tabIndex={-1} to the non-link state so screenreaders
and keyboard users get correct semantics; update the prev/next conditional
branches that reference prev, next, NextLink, styles.arrowDisabled,
ChevronLeftIcon and ChevronRightIcon accordingly.
In `@packages/chronicle/src/themes/paper/ReadingProgress.module.css`:
- Around line 70-80: Replace the hardcoded font stack in the .headingLink rule
with the design-system variable used by .headingLabel: use var(--rs-font-mono)
for font-family instead of "'SF Mono', 'Fira Code', monospace" so both
.headingLink and .headingLabel consistently reference the same mono font
variable.
In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx`:
- Around line 119-163: handleScroll currently performs imperative DOM updates
(style.top, classList, querySelector) to avoid React re-renders on every scroll;
add a concise inline comment above the function (or at the top of handleScroll)
explaining this is intentional for performance (e.g., "// Imperative updates to
avoid re-render on scroll") and reference the rationale and the related refs
(ARTICLE_SELECTOR, containerRef, scrollMarkerRef, scrollPosRef, snapToTick) so
future maintainers understand this is deliberate rather than a bug.
- Around line 248-257: The connecting line width calculation for headings in the
headings.map render can produce 0 or negative widths for deep levels; update the
inline style on the element with className styles.connectingLine (the block that
maps headings and uses h.level) to clamp the width using Math.max, e.g.
Math.max((3 - h.level) * 4 + 12, 4) and append "px" so the width never falls
below a sensible minimum.
- Around line 165-180: Replace the fragile setTimeout-based init with a
ResizeObserver watching the article element so heading positions are
recalculated when layout changes: find the element used to derive headings
(e.g., via articleRef or document.querySelector('article')) and create a
ResizeObserver that calls recalcHeadings() and handleScroll() (optionally
debounced and then calls setReady(true) once dimensions have stabilized), attach
the observer instead of relying on the 250ms timer, and on cleanup disconnect
the observer; keep the existing window scroll listener but remove the hardcoded
setTimeout and clearTimeout usage.
- Add next-env.d.ts to .gitignore and remove from tracking
- Fix invalid HTML: wrap ChapterNav items in <ul>, precompute chapter indices
- Use CSS variables for sidebar width and navbar height (no magic numbers)
- Add explicit font-size for h5/h6 in prose styles
- Use semantic accent token for ReadingProgress tick/marker colors
- Use var(--rs-font-mono) consistently in ReadingProgress
- Replace setTimeout init with ResizeObserver for reliable layout detection
- Add Math.max guard for connecting line width at deep heading levels
- Add useMemo for derived navigation data in Page
- Use <button disabled> with aria-label for disabled prev/next arrows
- Remove redundant .item { list-style: none } in ChapterNav
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/chronicle/src/themes/paper/ReadingProgress.tsx (2)
233-240: Optional: simplify thezIndexexpression.
10 * (h.level < 4 ? 1 : 0)is more clearly expressed as a direct ternary.♻️ Proposed simplification
- zIndex: 10 * (h.level < 4 ? 1 : 0), + zIndex: h.level < 4 ? 10 : 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx` around lines 233 - 240, The inline style for styles.headingLabel uses an unnecessary multiplication for zIndex; replace the expression 10 * (h.level < 4 ? 1 : 0) with a direct ternary returning 10 or 0 (e.g., h.level < 4 ? 10 : 0) inside the style object in ReadingProgress.tsx where the heading label div is rendered (referencing h and the inline style block).
233-240: Optional: simplify thezIndexexpression.
10 * (h.level < 4 ? 1 : 0)is needlessly roundabout.♻️ Proposed simplification
- zIndex: 10 * (h.level < 4 ? 1 : 0), + zIndex: h.level < 4 ? 10 : 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx` around lines 233 - 240, The zIndex expression in ReadingProgress.tsx is overly indirect: replace 10 * (h.level < 4 ? 1 : 0) with a clearer expression such as h.level < 4 ? 10 : 0 (or Number(h.level < 4) * 10) in the inline style on the element using styles.headingLabel so the intent (assign zIndex 10 when h.level < 4, otherwise 0) is explicit and easier to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chronicle/src/themes/paper/Layout.module.css`:
- Around line 1-4: The CSS rule .layout violates stylelint's
declaration-empty-line-before by placing the regular declaration min-height
immediately after the custom property --paper-sidebar-width; fix it in
Layout.module.css by inserting a single blank line between the custom property
declaration (--paper-sidebar-width: 260px;) and the following regular
declaration (min-height: 100vh;) so that there is an empty line separating the
custom property from min-height.
- Around line 1-4: In the .layout rule, add a blank line between the custom
property --paper-sidebar-width and the normal declaration min-height so the
declaration-empty-line-before Stylelint rule is satisfied; update the .layout
block containing --paper-sidebar-width and min-height to insert an empty line
immediately before the min-height declaration.
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 1-8: In the .main rule in Page.module.css, add a single blank line
between the last custom property (--paper-navbar-total) and the first standard
declaration (flex) to satisfy the Stylelint declaration-empty-line-before rule;
update the .main block so the custom properties remain grouped together, then a
blank line, then the rest of declarations (flex, max-width, margin) in the same
rule.
- Around line 1-8: The .main rule in Page.module.css places several custom
properties (--paper-*) followed immediately by normal declarations (flex,
max-width, margin) which violates the stylelint declaration-empty-line-before
rule; open the .main selector and insert a single blank line between the last
custom property (--paper-navbar-total) and the next declaration (flex), matching
the fix used in Layout.module.css so custom-property blocks are separated from
other declarations.
In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx`:
- Around line 31-58: The backward pass in resolveOverlaps wrongly reads mutated
neighbor positions which lets multiple headings collapse to 0; fix by replacing
the backward loop with a single-pass clamp that uses a running "nextPos"
initialized to maxPosition and for each heading (iterate i from end to start)
set its yPosition to Math.min(resolved[i].yPosition, nextPos) then update
nextPos = Math.max(0, resolved[i].yPosition - TICK_HEIGHT) so the allowed slot
is propagated backward without reading already-clamped neighbors; keep
references to resolveOverlaps, resolved, maxPosition, and TICK_HEIGHT when
making the change.
- Around line 31-58: The backward pass in resolveOverlaps can leave multiple
headings at the same yPosition when there are more headings than tick slots; fix
this by turning the clamp into a clamp-and-shift: inside resolveOverlaps's
backward loop where you compute maxAllowed and detect resolved[i].yPosition >
maxAllowed, set resolved[i].yPosition = Math.max(0, maxAllowed) and then walk j
from i-1 down to 0; for each earlier resolved[j], if resolved[j].yPosition >
resolved[j+1].yPosition - TICK_HEIGHT, set resolved[j].yPosition = Math.max(0,
resolved[j+1].yPosition - TICK_HEIGHT) so earlier items are pulled along and no
two items end up with the same yPosition. Ensure you update the objects in the
resolved array (e.g., resolved[i] = { ...resolved[i], yPosition: newPos }) when
modifying positions.
---
Duplicate comments:
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 10-21: The fixed .navbar is missing a left offset for the sidebar;
update the .navbar rule to set left: var(--paper-sidebar-width) and adjust its
width (e.g., width or max-width to calc(100% - var(--paper-sidebar-width))) so
it no longer sits under the sidebar defined by --paper-sidebar-width in
Layout.module.css; ensure the change is applied in Page.module.css where the
.navbar selector is defined.
- Around line 10-21: The .navbar rule uses position: fixed with width: 100% but
no left offset, so it starts at viewport left and covers the sidebar; update the
.navbar CSS to account for the sidebar by adding a left offset and adjusting
width (e.g., set left to the sidebar width variable or calc and change width to
calc(100% - sidebarWidthVar)) so the fixed navbar clears the 260px sidebar;
modify the .navbar selector in Page.module.css accordingly.
---
Nitpick comments:
In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx`:
- Around line 233-240: The inline style for styles.headingLabel uses an
unnecessary multiplication for zIndex; replace the expression 10 * (h.level < 4
? 1 : 0) with a direct ternary returning 10 or 0 (e.g., h.level < 4 ? 10 : 0)
inside the style object in ReadingProgress.tsx where the heading label div is
rendered (referencing h and the inline style block).
- Around line 233-240: The zIndex expression in ReadingProgress.tsx is overly
indirect: replace 10 * (h.level < 4 ? 1 : 0) with a clearer expression such as
h.level < 4 ? 10 : 0 (or Number(h.level < 4) * 10) in the inline style on the
element using styles.headingLabel so the intent (assign zIndex 10 when h.level <
4, otherwise 0) is explicit and easier to read.
- Fix resolveOverlaps backward pass to propagate clamp-and-shift, preventing multiple headings from collapsing to same position - Add blank line after CSS custom properties (stylelint) - Simplify zIndex ternary in ReadingProgress Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/chronicle/src/themes/paper/Page.module.css (1)
32-52:.arrowDisabledduplicates.arrow's flex setup — consider CSS composition.Both
.arrowand.arrowDisabledindependently declaredisplay: flex; align-items: center. In a CSS-modules context, extract the shared layout into a composable utility or merge them with a modifier class:♻️ Suggested refactor
+.arrowBase { + display: flex; + align-items: center; +} + .arrow { - display: flex; - align-items: center; color: var(--rs-color-foreground-base-primary); text-decoration: none; } .arrowDisabled { - display: flex; - align-items: center; color: var(--rs-color-foreground-base-tertiary); opacity: 0.4; cursor: default; border: none; background: none; padding: 0; }Then in
Page.tsx, compose the classes:// For active arrow: className={`${styles.arrowBase} ${styles.arrow}`} // For disabled arrow: className={`${styles.arrowBase} ${styles.arrowDisabled}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/Page.module.css` around lines 32 - 52, Extract the shared layout rules (display: flex; align-items: center) from .arrow and .arrowDisabled into a new CSS class (e.g., .arrowBase) in Page.module.css, leave color/hover/disabled-specific rules in .arrow and .arrowDisabled, then update Page.tsx to compose classes so active arrows use styles.arrowBase + styles.arrow and disabled ones use styles.arrowBase + styles.arrowDisabled (ensure you reference the existing .arrow and .arrowDisabled class names when composing).packages/chronicle/src/themes/paper/ReadingProgress.tsx (1)
191-192: Windowresizelistener callsrecalcHeadingswithout debouncing.
recalcHeadingsperforms severalgetBoundingClientRect()calls (each a forced reflow) and multiplesetStatedispatches. Theresizeevent fires on every pixel change during a drag-resize. On pages with many TOC items or on lower-end devices this can cause noticeable jank.Consider debouncing via
requestAnimationFrameor a shortsetTimeout:♻️ Suggested debounce wrapper (rAF-based)
+ const rafRef = useRef<number | null>(null) + const debouncedRecalc = useCallback(() => { + if (rafRef.current !== null) cancelAnimationFrame(rafRef.current) + rafRef.current = requestAnimationFrame(() => { + recalcHeadings() + }) + }, [recalcHeadings]) window.addEventListener('resize', debouncedRecalc) ... return () => { ... - window.removeEventListener('resize', recalcHeadings) + if (rafRef.current !== null) cancelAnimationFrame(rafRef.current) + window.removeEventListener('resize', debouncedRecalc) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx` around lines 191 - 192, The resize listener currently calls recalcHeadings directly causing layout thrash; replace the direct window.addEventListener('resize', recalcHeadings) with a debounced wrapper (e.g., an rAF-based or short setTimeout debounce) that batches calls to recalcHeadings, ensure you keep a stable reference to the debounced function so the cleanup removes the exact same listener, and update the effect/unmount logic to remove this debounced resize handler instead of the raw recalcHeadings; target the window.addEventListener('resize', ...) site and the recalcHeadings function for the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx`:
- Around line 97-98: The hasScroll calculation underestimates scrollability by
not accounting for the top navbar height; update the logic in ReadingProgress
(where hasScroll is computed and setIsScrollable is called) to compare
articleBox.height against (window.innerHeight - NAV_HEIGHT) so pages whose
content exceeds the visible area beneath the navbar are treated as scrollable;
ensure you reference the NAV_HEIGHT constant and keep the rest of the
setIsScrollable flow unchanged.
- Around line 234-239: The clickable tick divs rendered in ticks.map (the
element using className styles.tickClickable and the click handler
scrollToTick(y)) are not keyboard/AT accessible; update that element to be an
accessible button by adding role="button", tabIndex={0}, a descriptive
aria-label (e.g. indicating the tick position or percent), and an onKeyDown
handler that listens for Enter/Space and calls scrollToTick(y) (preserve the
existing onClick). Ensure the handlers use the same scrollToTick function and
that aria-label text is meaningful for screen readers.
---
Duplicate comments:
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 11-22: The .navbar CSS rule is missing a left offset so the fixed
navbar overlaps the 260px sidebar; update the .navbar declaration to include a
left offset (e.g., left: var(--paper-sidebar-width) or left: 260px) and adjust
its width (e.g., width: calc(100% - var(--paper-sidebar-width)) or calc(100% -
260px)) so it lines up to the right of the sidebar; modify the .navbar rule
(class name .navbar) to use the chosen CSS variable (or hard value) for left and
recalc width accordingly.
In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx`:
- Around line 31-68: The review note shows an accidental duplicate comment
marker; remove the stray "[duplicate_comment]" token from the review text (or
any duplicated approval tag) so the PR comment is not duplicated, and ensure the
code change message only contains the intended approval
"[approve_code_changes]"; this is a non-code cleanup tied to the resolveOverlaps
change and its inner propagation loop (the j-based backward propagation that
updates resolved[i] and resolved[j]).
---
Nitpick comments:
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 32-52: Extract the shared layout rules (display: flex;
align-items: center) from .arrow and .arrowDisabled into a new CSS class (e.g.,
.arrowBase) in Page.module.css, leave color/hover/disabled-specific rules in
.arrow and .arrowDisabled, then update Page.tsx to compose classes so active
arrows use styles.arrowBase + styles.arrow and disabled ones use
styles.arrowBase + styles.arrowDisabled (ensure you reference the existing
.arrow and .arrowDisabled class names when composing).
In `@packages/chronicle/src/themes/paper/ReadingProgress.tsx`:
- Around line 191-192: The resize listener currently calls recalcHeadings
directly causing layout thrash; replace the direct
window.addEventListener('resize', recalcHeadings) with a debounced wrapper
(e.g., an rAF-based or short setTimeout debounce) that batches calls to
recalcHeadings, ensure you keep a stable reference to the debounced function so
the cleanup removes the exact same listener, and update the effect/unmount logic
to remove this debounced resize handler instead of the raw recalcHeadings;
target the window.addEventListener('resize', ...) site and the recalcHeadings
function for the change.
rohilsurana
approved these changes
Feb 20, 2026
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
2 participants
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
config.theme.nameTest plan
theme: { name: paper }inchronicle.yamland run dev serverchronicle build— no TS errors🤖 Generated with Claude Code