feat: MDX component styling and UI improvements#16
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lugin Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Mermaid support and new MDX components (code, details/summary, paragraph), a remark plugin to normalize unused directives, styling updates, sidebar scroll persistence and header API buttons, search UI tweaks, and refactors page tree ordering and app layout wiring. Changes
Sequence DiagramsequenceDiagram
participant Comp as Mermaid Component
participant Effect as useEffect
participant Merlib as mermaid (dynamic import)
participant Renderer as mermaid.render
participant DOM as DOM (innerHTML)
Comp->>Effect: mount or chart prop change
Effect->>Merlib: import('mermaid')
Merlib-->>Effect: mermaid module
Effect->>Merlib: mermaid.initialize({ startOnLoad:false, theme:... })
Effect->>Renderer: mermaid.render(id, chart)
Renderer-->>Effect: svgString
Effect->>Comp: setState(svgString)
Comp->>DOM: render div with dangerouslySetInnerHTML(svgString)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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: 3
🧹 Nitpick comments (5)
packages/chronicle/src/lib/source.ts (1)
45-46: Local function shadows exportedsortByOrderand mutates input arrays.The local
sortByOrdershadows the exported function at line 10, which can cause confusion during maintenance. Additionally, the local version mutates the input array in place (.sort()), while the exported version creates a copy ([...items].sort()). This inconsistency could lead to subtle bugs if the code is refactored.🔧 Suggested fix: rename local function and avoid mutation
- const sortByOrder = (items: PageTreeItem[]) => - items.sort((a, b) => (a.order ?? Number.MAX_SAFE_INTEGER) - (b.order ?? Number.MAX_SAFE_INTEGER)) + const sortTreeItems = (items: PageTreeItem[]) => + [...items].sort((a, b) => (a.order ?? Number.MAX_SAFE_INTEGER) - (b.order ?? Number.MAX_SAFE_INTEGER))Then update usages on lines 48, 52, and 62 to use
sortTreeItems.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/lib/source.ts` around lines 45 - 46, The local helper currently declared as sortByOrder shadows the exported sortByOrder and mutates inputs; rename the local function to sortTreeItems and implement it to return a new sorted array (e.g., use [...items].sort(...)) rather than calling .sort() in-place, then update all local calls that used the old name (the three usages referenced near the surrounding code) to call sortTreeItems instead to preserve non-mutating behavior and avoid the export/name collision.packages/chronicle/src/themes/default/Layout.module.css (1)
22-24: Consider header height when calculating sticky sidebar height.When the sidebar becomes sticky at
top: 0, theheight: 100vhworks correctly if the header scrolls out of view. However, if the header is ever made sticky in the future, the sidebar would extend past the viewport bottom by the header's height.For robustness, consider using
max-height: 100vhinstead, which prevents overflow while allowing the sidebar to shrink if needed:♻️ Optional defensive adjustment
.sidebar { width: 260px; padding: var(--rs-space-5); border-right: 1px solid var(--rs-color-border-base-primary); overflow-y: auto; position: sticky; top: 0; - height: 100vh; + max-height: 100vh; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/default/Layout.module.css` around lines 22 - 24, The sticky sidebar currently sets height: 100vh which can overflow if a header becomes sticky; in the Layout.module.css rule that contains "position: sticky; top: 0; height: 100vh" replace the fixed height with a defensive max-height (e.g. max-height: 100vh or preferably max-height: calc(100vh - var(--header-height, 0px))) so the sidebar cannot extend past the viewport and can account for a sticky header in the future.packages/chronicle/src/themes/paper/Page.module.css (1)
162-164: Inline code will inherit serif font in paper theme.With
font-family: inherit, inline<code>elements will display in Georgia/serif (from line 97) rather than monospace. This may be intentional for the paper aesthetic, but verify this matches the desired design.🤖 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 162 - 164, The .content code rule sets font-family: inherit so inline <code> inherits the serif paper font (from the earlier body/font rules) rather than showing monospace; change the .content code selector to explicitly use a monospace stack (e.g., system monospace or your theme’s mono variable) so inline code renders in monospace (or, if the paper aesthetic intentionally uses serif, confirm and add a comment). Update the .content code rule (and optionally introduce/use a CSS variable like --font-mono) to ensure consistent monospace rendering across browsers.packages/chronicle/src/components/mdx/mermaid.tsx (2)
11-11: Unusedrefvariable.The
refis created but never used since the SVG is injected viadangerouslySetInnerHTML. Consider removing it.🧹 Proposed cleanup
export function Mermaid({ chart }: MermaidProps) { - const ref = useRef<HTMLDivElement>(null) const [svg, setSvg] = useState<string>('') ... return ( <div - ref={ref} className={styles.mermaid}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/components/mdx/mermaid.tsx` at line 11, Remove the unused ref created by useRef in the Mermaid component: delete the const ref = useRef<HTMLDivElement>(null) declaration (and any unused import of useRef) since the SVG is injected via dangerouslySetInnerHTML and the ref is never referenced; if you intended to manipulate the DOM later, instead attach the ref to the rendered div and use it in functions like renderMermaid (or update the render logic in the mermaid component) so the symbol names to look for are useRef, ref, and the Mermaid component in mermaid.tsx.
17-19: Movemermaid.initializeoutside the render function.Calling
mermaid.initializeon every render/re-render is unnecessary and could cause configuration conflicts. Initialize once at the module level or use a ref to track initialization.♻️ Proposed refactor
+let mermaidInitialized = false + export function Mermaid({ chart }: MermaidProps) { const ref = useRef<HTMLDivElement>(null) const [svg, setSvg] = useState<string>('') useEffect(() => { let cancelled = false async function render() { const { default: mermaid } = await import('mermaid') - mermaid.initialize({ startOnLoad: false, theme: 'default' }) + if (!mermaidInitialized) { + mermaid.initialize({ startOnLoad: false, theme: 'default' }) + mermaidInitialized = true + } const { svg: rendered } = await mermaid.render(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/components/mdx/mermaid.tsx` around lines 17 - 19, The code currently calls mermaid.initialize inside the async function render which causes re-initialization on every render; move the initialization to module-level (perform import and call mermaid.initialize once when the module loads) or add an initialization guard (e.g., module-scoped boolean or React ref) so render only imports/uses mermaid without calling mermaid.initialize repeatedly—update references in the render function and the import/default mermaid usage accordingly to ensure initialization happens once.
🤖 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/lib/remark-unused-directives.ts`:
- Around line 19-24: The current transformation uses node.name directly so if
node.name is undefined you end up with ":undefined"; in the block inside
remark-unused-directives where you check if (!hasAttributes && !hasChildren)
(and you assign const name = node.name) add a guard or fallback for node.name
(e.g. skip the conversion when node.name is undefined or use a safe fallback
like an explicit placeholder string) before you delete keys and set
node.type/node.value, ensuring node.value never becomes ":undefined".
In `@packages/chronicle/src/themes/default/Layout.tsx`:
- Around line 39-45: The current map in config.api renders a Button inside
NextLink causing nested interactive elements (see the JSX using NextLink,
Button, and api.basePath); change it to avoid an <a> containing a <button> by
either: 1) using Button's asChild prop to render NextLink inside the Button
(e.g., <Button asChild><NextLink href={api.basePath}>...</NextLink></Button>),
or 2) render NextLink styled as a button (remove Button and apply the Button
styles to NextLink), or 3) replace with a dedicated LinkButton component if
available—apply one of these fixes where config.api is mapped to ensure the
clickable element is a single interactive element.
In `@packages/chronicle/src/themes/default/Page.module.css`:
- Around line 37-39: The .content code rule is forcing inherited body fonts onto
all code and will override inline code (.inlineCode / MdxCode); remove or change
the selector so inline code keeps a monospace font. Update the selector .content
code to either be removed entirely or tightened to target block/code-in-pre only
(e.g., .content pre code) or set an explicit monospace font-family (e.g.,
ui-monospace, SFMono-Regular, Menlo, Monaco, monospace) so that MdxCode's
inlineCode remains monospace.
---
Nitpick comments:
In `@packages/chronicle/src/components/mdx/mermaid.tsx`:
- Line 11: Remove the unused ref created by useRef in the Mermaid component:
delete the const ref = useRef<HTMLDivElement>(null) declaration (and any unused
import of useRef) since the SVG is injected via dangerouslySetInnerHTML and the
ref is never referenced; if you intended to manipulate the DOM later, instead
attach the ref to the rendered div and use it in functions like renderMermaid
(or update the render logic in the mermaid component) so the symbol names to
look for are useRef, ref, and the Mermaid component in mermaid.tsx.
- Around line 17-19: The code currently calls mermaid.initialize inside the
async function render which causes re-initialization on every render; move the
initialization to module-level (perform import and call mermaid.initialize once
when the module loads) or add an initialization guard (e.g., module-scoped
boolean or React ref) so render only imports/uses mermaid without calling
mermaid.initialize repeatedly—update references in the render function and the
import/default mermaid usage accordingly to ensure initialization happens once.
In `@packages/chronicle/src/lib/source.ts`:
- Around line 45-46: The local helper currently declared as sortByOrder shadows
the exported sortByOrder and mutates inputs; rename the local function to
sortTreeItems and implement it to return a new sorted array (e.g., use
[...items].sort(...)) rather than calling .sort() in-place, then update all
local calls that used the old name (the three usages referenced near the
surrounding code) to call sortTreeItems instead to preserve non-mutating
behavior and avoid the export/name collision.
In `@packages/chronicle/src/themes/default/Layout.module.css`:
- Around line 22-24: The sticky sidebar currently sets height: 100vh which can
overflow if a header becomes sticky; in the Layout.module.css rule that contains
"position: sticky; top: 0; height: 100vh" replace the fixed height with a
defensive max-height (e.g. max-height: 100vh or preferably max-height:
calc(100vh - var(--header-height, 0px))) so the sidebar cannot extend past the
viewport and can account for a sticky header in the future.
In `@packages/chronicle/src/themes/paper/Page.module.css`:
- Around line 162-164: The .content code rule sets font-family: inherit so
inline <code> inherits the serif paper font (from the earlier body/font rules)
rather than showing monospace; change the .content code selector to explicitly
use a monospace stack (e.g., system monospace or your theme’s mono variable) so
inline code renders in monospace (or, if the paper aesthetic intentionally uses
serif, confirm and add a comment). Update the .content code rule (and optionally
introduce/use a CSS variable like --font-mono) to ensure consistent monospace
rendering across browsers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1664c81-76b4-41ef-b6c2-b03359c7e29a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
packages/chronicle/package.jsonpackages/chronicle/source.config.tspackages/chronicle/src/components/common/callout.module.csspackages/chronicle/src/components/mdx/code.module.csspackages/chronicle/src/components/mdx/code.tsxpackages/chronicle/src/components/mdx/details.module.csspackages/chronicle/src/components/mdx/details.tsxpackages/chronicle/src/components/mdx/index.tsxpackages/chronicle/src/components/mdx/mermaid.module.csspackages/chronicle/src/components/mdx/mermaid.tsxpackages/chronicle/src/components/mdx/paragraph.module.csspackages/chronicle/src/components/mdx/paragraph.tsxpackages/chronicle/src/lib/remark-unused-directives.tspackages/chronicle/src/lib/source.tspackages/chronicle/src/themes/default/Layout.module.csspackages/chronicle/src/themes/default/Layout.tsxpackages/chronicle/src/themes/default/Page.module.csspackages/chronicle/src/themes/paper/Page.module.css
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move Layout to layout.tsx and render only Page in page.tsx. Sidebar scroll preserved via module-level variable targeting Apsara Sidebar.Main scrollable element. Remove duplicate border-right and overflow-y from sidebar CSS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/chronicle/src/themes/default/Layout.module.css (1)
17-22: Sticky sidebar implementation looks correct.The
position: sticky; top: 0; height: 100vh;approach is appropriate for the sticky sidebar objective. However, note thatheight: 100vhincludes the full viewport height. If the header is outside this sidebar's container, the sidebar might extend beyond the visible area by the header's height.Consider using
height: calc(100vh - <header-height>)if you observe the sidebar bottom being cut off, or verify that the parent flex layout already constrains this correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/default/Layout.module.css` around lines 17 - 22, The .sidebar rule in Layout.module.css uses height: 100vh which can overlap with a header; update the .sidebar declaration (in Layout.module.css) to either remove the fixed height so the parent flex container controls sizing, or replace height: 100vh with height: calc(100vh - <header-height>) (substituting the actual header height or CSS variable) so the sticky sidebar respects the header space; ensure this change is applied to the .sidebar selector and verify the parent layout (flex/grid) still constrains the sidebar as intended.packages/chronicle/src/themes/default/Layout.tsx (1)
31-33: Fragile selector targeting internal library structure.The selector
[class*="main"]depends on the internal class naming of the APSARASidebarcomponent. If the library changes its internal class names in a future version, this will silently break scroll persistence.Consider checking if
@raystack/apsaraprovides a ref forwarding mechanism or documented way to access the scroll container, or add a comment documenting this dependency on internal structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/themes/default/Layout.tsx` around lines 31 - 33, The getScrollEl function uses a fragile selector '[class*="main"]' against sidebarRef which relies on APSARA internals; replace this by using any official ref-forwarding or API from `@raystack/apsara` to obtain the scroll container (e.g., pass a ref prop into Sidebar or call a provided method) and only fall back to a selector as a last resort, or alternatively add a clear comment above getScrollEl documenting the dependency on APSARA's internal class names and why the selector is required; update references to sidebarRef and getScrollEl accordingly so future maintainers know to switch to the library API if available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/chronicle/src/themes/default/Layout.module.css`:
- Around line 17-22: The .sidebar rule in Layout.module.css uses height: 100vh
which can overlap with a header; update the .sidebar declaration (in
Layout.module.css) to either remove the fixed height so the parent flex
container controls sizing, or replace height: 100vh with height: calc(100vh -
<header-height>) (substituting the actual header height or CSS variable) so the
sticky sidebar respects the header space; ensure this change is applied to the
.sidebar selector and verify the parent layout (flex/grid) still constrains the
sidebar as intended.
In `@packages/chronicle/src/themes/default/Layout.tsx`:
- Around line 31-33: The getScrollEl function uses a fragile selector
'[class*="main"]' against sidebarRef which relies on APSARA internals; replace
this by using any official ref-forwarding or API from `@raystack/apsara` to obtain
the scroll container (e.g., pass a ref prop into Sidebar or call a provided
method) and only fall back to a selector as a last resort, or alternatively add
a clear comment above getScrollEl documenting the dependency on APSARA's
internal class names and why the selector is required; update references to
sidebarRef and getScrollEl accordingly so future maintainers know to switch to
the library API if available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef42392e-3f15-40a4-bfd3-3d5b6eb9e1b0
📒 Files selected for processing (6)
packages/chronicle/src/app/[[...slug]]/layout.tsxpackages/chronicle/src/app/[[...slug]]/page.tsxpackages/chronicle/src/components/ui/search.module.csspackages/chronicle/src/components/ui/search.tsxpackages/chronicle/src/themes/default/Layout.module.csspackages/chronicle/src/themes/default/Layout.tsx
💤 Files with no reviewable changes (1)
- packages/chronicle/src/components/ui/search.module.css
- Guard against undefined node.name in remark-unused-directives - Use asChild on Button for API links (avoid nested interactive elements) - Remove .content code font-family: inherit (preserves monospace) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Render div instead of p when children contain block elements like summary, details, etc. to avoid invalid HTML nesting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@packages/chronicle/src/components/mdx/paragraph.tsx`:
- Around line 6-9: hasBlockChild currently only recognizes intrinsic elements
(typeof child.type === 'string') and thus misses MDX-mapped wrappers like
MdxDetails and MdxSummary; update hasBlockChild to also treat those components
as block children by checking for function/element component types and matching
them against the known block wrappers (e.g., MdxDetails and MdxSummary) either
by direct reference (child.type === MdxDetails || child.type === MdxSummary) or
by name/displayName (child.type.name or child.type.displayName equals
"MdxDetails"/"MdxSummary"); ensure you still preserve the existing
BLOCK_ELEMENTS check for string types and add the new component checks inside
the Children.toArray(...).some(...) predicate so paragraphs won't be wrapped
around these block components.
In `@packages/chronicle/src/themes/default/Layout.module.css`:
- Around line 19-21: Replace the forcing height on the sidebar: in the CSS rule
for .sidebar remove or replace height: 100vh with max-height: 100vh and add
align-self: flex-start so the .body flex row isn't stretched; move any scrolling
behavior into the sidebar's inner container (e.g., .sidebarInner or
.sidebarContent) by giving that inner element overflow: auto and an appropriate
max-height so it owns scrolling instead of the outer .sidebar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a48101a-1aed-4e17-b3ea-a06b4820741f
📒 Files selected for processing (6)
packages/chronicle/src/components/mdx/paragraph.tsxpackages/chronicle/src/lib/remark-unused-directives.tspackages/chronicle/src/themes/default/Layout.module.csspackages/chronicle/src/themes/default/Layout.tsxpackages/chronicle/src/themes/default/Page.module.csspackages/chronicle/src/themes/paper/Page.module.css
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/chronicle/src/lib/remark-unused-directives.ts
- packages/chronicle/src/themes/paper/Page.module.css
- packages/chronicle/src/themes/default/Page.module.css
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rohilsurana
approved these changes
Mar 11, 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
<details>,<summary>,<p>, inline<code>:5173) by reverting unused text directivesremarkMdxMermaidTest plan
<details>/<summary>styling on configurations page🤖 Generated with Claude Code