feat: outline the current note's headings in the sidebar - #895
Conversation
Add a collapsible "Outline" section to the note context sidebar listing the current note's headings. It updates live as you edit, jumps to a heading on click, and highlights the section you're currently reading as you scroll. Scoped to single notes (not the daily stream); reuses parseNote's headings and the editor's revealHeading; apps/desktop only, no core or Rust changes.
WalkthroughAdds a live Markdown-derived heading outline to the desktop note sidebar, with active-heading tracking, click-to-reveal navigation, per-note subscription state, editor-handle support, and tests for storage, rendering, scrolling, and mocks. ChangesNote outline navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NotePane
participant OutlineStore
participant OutlineSection
participant NoteEditor
NotePane->>OutlineStore: publishOutlineFromMarkdown(path, markdown)
OutlineStore-->>OutlineSection: notify outline subscriber
OutlineSection->>OutlineStore: read current headings
OutlineSection->>NoteEditor: revealHeading(selected heading text)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 (3)
apps/desktop/src/components/context-sidebar/use-active-heading.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ACTIVE_LINE_OFFSETis duplicated as a magic number in the test.This constant isn't exported, so
use-active-heading.test.tshardcodesconst LINE = 12with a comment stating it must be "kept in sync" manually — a silent-drift risk if the offset ever changes.♻️ Proposed fix
-const ACTIVE_LINE_OFFSET = 12 +export const ACTIVE_LINE_OFFSET = 12Then in the test, import and reuse it instead of the hardcoded
12.🤖 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 `@apps/desktop/src/components/context-sidebar/use-active-heading.ts` at line 7, Export ACTIVE_LINE_OFFSET from use-active-heading.ts and update use-active-heading.test.ts to import and reuse it instead of defining the duplicated LINE = 12 value, removing the manual sync comment.apps/desktop/src/components/note-pane.tsx (2)
234-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the outline-publish wiring into a small hook.
NotePaneComponentis already a large multi-concern component; this new block (seed publish effect + onChange wrapper) is self-contained business logic that could live in its own hook (e.g.usePublishNoteOutline(path, editorSeed, onEditorChange)).As per coding guidelines, "Move large mutation handlers, parsing, persistence, and business logic into helpers or hooks instead of embedding them in components."
🤖 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 `@apps/desktop/src/components/note-pane.tsx` around lines 234 - 261, Extract the outline publishing logic from NotePaneComponent into a dedicated hook, such as usePublishNoteOutline, accepting path, editorSeed, and the document editor-change callback. Move the onEditorChange wrapper and publish/cleanup effect into that hook, preserving their existing dependencies and clearOutline cleanup behavior, then use the hook from NotePaneComponent.Source: Path instructions
247-254: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvery keystroke now runs a second full markdown parse for the outline.
onDocumentEditorChange(markdown)almost certainly already parsesmarkdownfor persistence/rename-tracking;publishOutlineFromMarkdownruns an independentparseNoteover the same string on every keystroke viaonEditorChange. For large notes this doubles parse cost on the hot typing path.♻️ Possible mitigation
Debounce/throttle the outline publish (e.g. via
requestIdleCallbackor a shortsetTimeout), or, ifdocument.onEditorChangealready exposes a parsed AST/headings list, reuse it instead of re-parsing:- const onEditorChange = useCallback( - (markdown: string) => { - onDocumentEditorChange(markdown) - publishOutlineFromMarkdown(path, markdown) - }, - [onDocumentEditorChange, path], - ) + const onEditorChange = useCallback( + (markdown: string) => { + onDocumentEditorChange(markdown) + scheduleOutlinePublish(path, markdown) // debounced wrapper + }, + [onDocumentEditorChange, path], + )🤖 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 `@apps/desktop/src/components/note-pane.tsx` around lines 247 - 254, Update the onEditorChange callback in note-pane.tsx to avoid synchronously running publishOutlineFromMarkdown on every keystroke alongside onDocumentEditorChange. Debounce or defer outline publishing using the component’s existing lifecycle/cleanup patterns, or reuse parsed headings/AST data if onDocumentEditorChange exposes it, while preserving outline updates and preventing stale scheduled work.
🤖 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 `@apps/desktop/src/components/context-sidebar/use-active-heading.ts`:
- Around line 41-46: Update the reset logic in the active-heading hook to key
only on the note path, removing headingCount from paneKey so heading edits
within the same note do not reset active. Preserve the path-change reset
behavior, and add or adjust coverage for a headingCount-only change with an
unchanged path.
- Line 52: Update the selector construction in the active-heading lookup to
escape path before interpolating it into the querySelector selector, using
CSS.escape(path) or an equivalent ref/registry lookup. Preserve the existing
aria-label matching behavior for paths without special characters.
In `@apps/desktop/src/components/note-pane.tsx`:
- Around line 256-261: Guard the outline-publishing effect around
publishOutlineFromMarkdown so it does not process protected/conflicted notes
whose editorSeed contains raw conflict markers. Reuse the same document
protection/conflict state used by the document.protected early return and
detectConflictMarkers logic, while preserving clearOutline cleanup and normal
outline behavior for non-conflicted notes.
---
Nitpick comments:
In `@apps/desktop/src/components/context-sidebar/use-active-heading.ts`:
- Line 7: Export ACTIVE_LINE_OFFSET from use-active-heading.ts and update
use-active-heading.test.ts to import and reuse it instead of defining the
duplicated LINE = 12 value, removing the manual sync comment.
In `@apps/desktop/src/components/note-pane.tsx`:
- Around line 234-261: Extract the outline publishing logic from
NotePaneComponent into a dedicated hook, such as usePublishNoteOutline,
accepting path, editorSeed, and the document editor-change callback. Move the
onEditorChange wrapper and publish/cleanup effect into that hook, preserving
their existing dependencies and clearOutline cleanup behavior, then use the hook
from NotePaneComponent.
- Around line 247-254: Update the onEditorChange callback in note-pane.tsx to
avoid synchronously running publishOutlineFromMarkdown on every keystroke
alongside onDocumentEditorChange. Debounce or defer outline publishing using the
component’s existing lifecycle/cleanup patterns, or reuse parsed headings/AST
data if onDocumentEditorChange exposes it, while preserving outline updates and
preventing stale scheduled work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1941556-3792-4fb3-a3d5-41d021ab663d
📒 Files selected for processing (16)
apps/desktop/src/components/context-sidebar/note-context-sidebar.tsxapps/desktop/src/components/context-sidebar/outline-section.test.tsxapps/desktop/src/components/context-sidebar/outline-section.tsxapps/desktop/src/components/context-sidebar/use-active-heading.test.tsapps/desktop/src/components/context-sidebar/use-active-heading.tsapps/desktop/src/components/note-pane.tsxapps/desktop/src/components/route-content.test.tsxapps/desktop/src/editor/note-editor.tsxapps/desktop/src/editor/note-outline-store.test.tsapps/desktop/src/editor/note-outline-store.tsapps/desktop/src/editor/reveal-heading.test.tsxapps/desktop/src/editor/use-note-document.test.tsxapps/desktop/src/editor/use-template-slash-items.test.tsapps/desktop/src/lib/attach-files.test.tsapps/desktop/src/mobile/mobile-screen.test.tsxapps/desktop/src/mobile/screens/tasks.test.tsx
| if (headingCount === 0) { | ||
| return | ||
| } | ||
| const pane = window.document.querySelector(`[aria-label="Editing ${path}"]`) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,140p' apps/desktop/src/components/context-sidebar/use-active-heading.tsRepository: team-reflect/reflect-open
Length of output: 4042
🌐 Web query:
MDN CSS.escape attribute selector quoted value quote escaping querySelector WebView2 WKWebView support
💡 Result:
The CSS.escape static method is the standard, recommended approach for escaping arbitrary strings for use within CSS selectors, including attribute selectors [1][2][3]. When using querySelector with attribute selectors, you should use CSS.escape on the attribute value to ensure it is treated as a valid CSS identifier or string, preventing syntax errors caused by special characters [2][4][5]. For an attribute selector such as [attr=value], the proper implementation is: document.querySelector([data-id="${CSS.escape(id)}"]); Note that CSS.escape handles the necessary escaping of characters—such as quotes, spaces, and other symbols—that would otherwise break the selector syntax [1][2][3]. You generally do not need to add extra quotes manually inside the template literal when using CSS.escape, as it effectively prepares the string to be safely concatenated into the selector [1][6]. Regarding platform support: CSS.escape is widely supported in modern environments [7][8]. - WebView2: Supported (consistent with Chromium support) [7][8]. - WKWebView: Supported (consistent with Safari support, which added it in version 10.1) [7][8]. Since both modern WebView2 and WKWebView are built on engines that support this API (Chromium and WebKit, respectively), it is safe to use in these environments without a polyfill [9][7][8]. If you need to support very old versions of these WebViews, you may need to include a polyfill, such as the one available from the Mathias Bynens CSS.escape repository [3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Element/querySelector
- 3: https://googlechrome.github.io/samples/css-escape/
- 4: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/require-css-escape.md
- 5: https://www.stefanjudis.com/today-i-learned/how-to-escape-css-selectors-in-javascript/
- 6: https://stackoverflow.com/questions/13987979/how-to-properly-escape-attribute-values-in-css-js-attribute-selector-attr-value
- 7: https://caniuse.com/wf-css-escape
- 8: https://web-platform-dx.github.io/web-features-explorer/features/css-escape/
- 9: https://caniwebview.com/features/web-feature-css-escape/
Escape path before building the selector. querySelector will throw on quotes and other selector metacharacters in path; use CSS.escape(path) here, or switch to a ref/registry lookup.
🛡️ Proposed fix
- const pane = window.document.querySelector(`[aria-label="Editing ${path}"]`)
+ const pane = window.document.querySelector(`[aria-label="Editing ${CSS.escape(path)}"]`)📝 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.
| const pane = window.document.querySelector(`[aria-label="Editing ${path}"]`) | |
| const pane = window.document.querySelector(`[aria-label="Editing ${CSS.escape(path)}"]`) |
🤖 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 `@apps/desktop/src/components/context-sidebar/use-active-heading.ts` at line
52, Update the selector construction in the active-heading lookup to escape path
before interpolating it into the querySelector selector, using CSS.escape(path)
or an equivalent ref/registry lookup. Preserve the existing aria-label matching
behavior for paths without special characters.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/components/context-sidebar/use-active-heading.ts (1)
55-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEscape
pathbefore building the selector.
querySelectorwill throw on quotes and other selector metacharacters inpath; useCSS.escape(path)here.🛡️ Proposed fix
- const pane = window.document.querySelector(`[aria-label="Editing ${path}"]`) + const pane = window.document.querySelector(`[aria-label="Editing ${CSS.escape(path)}"]`)🤖 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 `@apps/desktop/src/components/context-sidebar/use-active-heading.ts` at line 55, Update the selector construction in the active-heading logic to pass path through CSS.escape before interpolating it into the aria-label querySelector selector. Preserve the existing label format and query behavior for ordinary paths while ensuring paths containing quotes or selector metacharacters cannot break the selector.
🤖 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.
Duplicate comments:
In `@apps/desktop/src/components/context-sidebar/use-active-heading.ts`:
- Line 55: Update the selector construction in the active-heading logic to pass
path through CSS.escape before interpolating it into the aria-label
querySelector selector. Preserve the existing label format and query behavior
for ordinary paths while ensuring paths containing quotes or selector
metacharacters cannot break the selector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 673488a5-534b-4693-bcd5-05c5796da5bd
📒 Files selected for processing (3)
apps/desktop/src/components/context-sidebar/use-active-heading.test.tsapps/desktop/src/components/context-sidebar/use-active-heading.tsapps/desktop/src/components/note-pane.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/components/context-sidebar/use-active-heading.test.ts
- apps/desktop/src/components/note-pane.tsx
There was a problem hiding this comment.
Pull request overview
Adds an always-present, collapsible Outline section to the note context sidebar in apps/desktop, powered by live heading extraction from the editor and a scroll-spy hook to track the active section.
Changes:
- Introduces a per-note-path outline store and wires
NotePaneto publish/clear headings as the editor loads and edits. - Adds an
OutlineSectionsidebar component and auseActiveHeadingscroll-spy hook to highlight and keep the active row in view. - Exposes
NoteEditorHandle.revealHeading(delegating to Meowdown) so clicking an outline row can navigate within the editor; updates/introduces tests accordingly.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/desktop/src/mobile/screens/tasks.test.tsx | Updates NoteEditor mock to include revealHeading. |
| apps/desktop/src/mobile/mobile-screen.test.tsx | Updates NoteEditor mock to include revealHeading. |
| apps/desktop/src/lib/attach-files.test.ts | Updates NoteEditor mock to include revealHeading. |
| apps/desktop/src/editor/use-template-slash-items.test.ts | Updates NoteEditor mock to include revealHeading. |
| apps/desktop/src/editor/use-note-document.test.tsx | Updates NoteEditor mock to include revealHeading. |
| apps/desktop/src/editor/reveal-heading.test.tsx | Adds a unit test ensuring NoteEditorHandle.revealHeading delegates to Meowdown. |
| apps/desktop/src/editor/note-outline-store.ts | Adds module-level outline store keyed by note path (get/subscribe/publish/clear). |
| apps/desktop/src/editor/note-outline-store.test.ts | Tests outline store behavior and markdown->headings parsing. |
| apps/desktop/src/editor/note-editor.tsx | Adds revealHeading to NoteEditorHandle and delegates to the inner editor handle. |
| apps/desktop/src/components/route-content.test.tsx | Updates NoteEditor mock to include revealHeading. |
| apps/desktop/src/components/note-pane.tsx | Publishes outline headings on load and on editor changes; clears on unmount. |
| apps/desktop/src/components/context-sidebar/use-active-heading.ts | Adds scroll-spy hook to compute active heading index based on DOM positions. |
| apps/desktop/src/components/context-sidebar/use-active-heading.test.ts | Tests scroll-spy logic and update triggers (scroll/DOM paint/mutations). |
| apps/desktop/src/components/context-sidebar/outline-section.tsx | Adds the Outline sidebar section UI using the store + scroll-spy + editor navigation. |
| apps/desktop/src/components/context-sidebar/outline-section.test.tsx | Tests outline rendering, placeholder state, aria-current, and click navigation. |
| apps/desktop/src/components/context-sidebar/note-context-sidebar.tsx | Inserts OutlineSection into the note context sidebar layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [previousPath, setPreviousPath] = useState(path) | ||
| if (previousPath !== path) { | ||
| setPreviousPath(path) | ||
| setActive(0) | ||
| } |
| const pane = window.document.querySelector(`[aria-label="Editing ${path}"]`) | ||
| if (pane === null) { | ||
| return | ||
| } |
| useEffect(() => { | ||
| // Protected notes (sync conflicts land here) render raw, conflict-marked | ||
| // content instead of the editor, so parsing `editorSeed` would outline | ||
| // headings from both conflict sides rather than the note's real structure. | ||
| if (document.protected) { | ||
| return | ||
| } | ||
| publishOutlineFromMarkdown(path, editorSeed) | ||
| return () => { | ||
| clearOutline(path) | ||
| } | ||
| }, [path, editorSeed, document.protected]) |
| const onEditorChange = useCallback( | ||
| (markdown: string) => { | ||
| onDocumentEditorChange(markdown) | ||
| publishOutlineFromMarkdown(path, markdown) | ||
| }, |
| const onSelect = useCallback( | ||
| (text: string) => { | ||
| noteEditorHandleFor(path)?.revealHeading(text) | ||
| }, | ||
| [path], |
Closes #870.
Adds a collapsible Outline section to the note context sidebar — the intra-note navigation aid proposed in #870.
What it does
Scope / alignment with the discussion
parseNote(...).headingsand the editor's existingrevealHeading;apps/desktoponly, nopackages/coreor Rust changes.How it works
editor-handle-registry) carries the parsed headings from the note pane to the sidebar viauseSyncExternalStore.Checks
pnpm typecheckandpnpm lintclean.revealHeadingpassthrough, the scroll-spy hook, and the outline section (colocated*.test.ts(x)).cargo testis not applicable.Notes
Summary by CodeRabbit