Skip to content

Code-review findings, per-line code-block comments, and a document-comments agent skill#44

Merged
kylemcd merged 10 commits into
mainfrom
fix/code-review-findings
Jul 22, 2026
Merged

Code-review findings, per-line code-block comments, and a document-comments agent skill#44
kylemcd merged 10 commits into
mainfrom
fix/code-review-findings

Conversation

@kylemcd

@kylemcd kylemcd commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Fixes the issues from a full review of the codebase (the review doc lives in the planning vault), plus two follow-on additions built on that work: a per-line code-block comment feature and a reusable agent skill (see the last two sections). Full check — oxfmt, oxlint on src and test, tsc with noUncheckedIndexedAccess, 111 vitest tests — is green; production build verified.

Data-integrity fixes (each with a regression test)

  • Forward-Delete no longer destroys a comment thread. The hidden <!--co:--> body block is now protected from atomic deletion, so a forward-Delete at the end of an anchored line removes only the swallowed newline instead of silently deleting the whole thread with no visible change.
  • Multi-line comment text round-trips. Thread text is escaped (new format/escape.ts) so a reply like ok + Shift+Enter + +1 great no longer re-parses into a bogus entry and a fake reaction; blank lines and trailing spaces are preserved too. Reaction author commas are escaped.
  • --> in the selected text or author no longer leaks the thread. The quote/author header fields break --> like body text already did, so the HTML comment can't terminate early in Reading view / GitHub / export.
  • Add-comment inside a fenced code block is refused instead of writing masked, invisible markers.
  • Delete removes every occurrence of a comment's markers, so copy-paste duplicates don't leave invisible, un-removable leftovers.
  • Adjacent comments no longer both borrow the single space between them (which produced overlapping atomic ranges with no caret position between them).

Write-path correctness

  • New editor/routing.ts centralizes edit routing: the reading margin and sidebar prefer an open editor (undo history + unsaved buffer) over a disk write that races autosave.
  • New-comment offsets are verified against the captured selection before writing (mobile modal, reading draft, reading mobile), so a doc that shifted under the composer is caught instead of mis-anchoring.
  • Reading-view Add-comment is rejected inside an embed/preview (a different file's block).
  • Card: preserve an in-progress entry edit across external updates; drop the document-level mousedown listener on destroy; tear down the reaction-picker listener when an emoji is picked. Both margins guard rAF/reposition against a destroyed view.
  • Comment-text links resolve against the editor's own file (editorInfoField); the sidebar loads a deferred (Obsidian 1.7+) leaf before use; settings changes resync the ribbon.

Refactor (removes the duplication the review flagged)

  • Pure, tested stackTops stacking algorithm shared by both margins (and it batches DOM reads before writes); shared processFileEdit fold; shared spanSelector/closestSpanId, constants, and the draft composer. cardSignature/formatRelativeTime moved to a pure module and unit-tested. Settings single-sourced; Draft folded into TextRange; dead isValidId removed.

Tooling

  • noUncheckedIndexedAccess on (and the resulting real cases fixed); test/ is now linted; redundant tsconfig flags dropped; package.json version synced.

A handful of low-value/high-churn items are consciously deferred and listed in the review doc's resolution-status section (e.g. table-quote link resolution, full CRLF-on-insert normalization, a margin base class, gating test typechecking).

Per-line code-block comments

Comments can now anchor specific lines inside a fenced code block, not just prose. The comment wraps the fence with own-line <!--c-->/<!--/c--> markers and records the block-relative line range (line:F-T) plus the exact code as the body quote:; the target lines are highlighted in Live Preview and Reading view.

  • Live Preview layout is pixel-faithful to an uncommented block. The hidden marker/body lines are block-replaced to zero height while every newline is preserved, so the surrounding blank lines and both ``` fence rows keep their natural, theme-driven height — no ghost gap above the block and no row-shift below. (This resolves a glitch where absorbing a fence-adjacent newline stopped Obsidian tagging the opening/closing fence, and it does so with no hardcoded pixel values, so it holds across themes/font sizes.)
  • CRLF-safe delete. Deleting a code comment on a CRLF file takes the whole \r\n with each own-line marker instead of leaving a stray blank line (the raw-file path — sidebar / Reading view — sees the file's real endings). Full CRLF-on-insert normalization remains the deferred item noted above.
  • Regression tests cover the rendered row structure (both fences and both blank lines survive), the document-start and body-at-EOF edge cases, the atomic caret range, and the CRLF delete round-trip.

Agent skill (skills/document-comments/)

A self-contained, generic skill teaching an agent to create, reply to, resolve, and delete these comments correctly — leading with the alphanumeric-ID rule that silently breaks a comment when violated. Ships the on-disk format reference, a dependency-free validate_comments.py, and the eval suite used to verify it (100% with-skill vs 44% no-skill baseline over the bundled evals). Transient eval run outputs (skills/*-workspace/) and the packaged .skill artifact are gitignored; the source here is the single source of truth.

🤖 Generated with Claude Code

kylemcd added 9 commits July 22, 2026 09:43
- Protect the hidden body block from atomic forward-Delete (state.ts): a
  forward-Delete at the end of an anchored line no longer silently destroys
  the whole thread; only the swallowed newline is removed.
- Escape newlines/backslashes in thread text (new format/escape.ts) so
  multi-line replies round-trip instead of splitting into bogus entries or
  reactions; preserve blank lines and trailing spaces.
- Break --> in the quote and author header fields so the body block can't
  terminate early and leak the thread into non-plugin renderers.
- Refuse add-comment inside fenced code blocks instead of writing masked,
  invisible markers.
- Delete every occurrence of a comment's markers (copy-paste duplicates no
  longer leave invisible, UI-unremovable leftovers).
- Stop adjacent comments from both borrowing the shared space (overlapping
  atomic ranges left no caret position between them).
- Escape commas in reaction author names.

Adds test/roundtrip.test.ts and regressions in decorations/edits tests.
- New editor/routing.ts centralizes editor-vs-vault edit routing: reading
  margin and sidebar now prefer an open editor (undo history + unsaved buffer)
  instead of racing its autosave with a disk write.
- New commands.processFileEdit + errorMessage fold the vault.process Result
  pattern that was copy-pasted four times.
- New ui/stack.ts stackTops() is the pure card-stacking algorithm shared by
  both margins (and batches DOM reads before writes); new ui/constants.ts
  holds CARD_GAP and the flash durations; util/css.ts gains spanSelector /
  closestSpanId used across every surface.
- Verify a new comment's frozen offsets still cover the selected text before
  writing (mobile modal, reading draft, reading mobile), so a doc that shifted
  under the composer is caught instead of mis-anchoring.
- Reject reading-view Add-comment when the selection is inside an embed/preview
  (a different file's block) via a sourcePath check.
- Card: preserve an in-progress entry edit across external updates; remove the
  document-level mousedown listener on destroy; tear down the reaction picker's
  listener when an emoji is picked.
- Both margins guard their rAF/reposition against a destroyed view.
- Resolve comment-text links against the editor's own file (editorInfoField),
  not the workspace-active file.
- Sidebar reveal loads a deferred (Obsidian 1.7+) leaf before use.
- Settings single-sourced (fixes label drift) and now resync the ribbon.
- Consolidate Draft into TextRange; drop dead isValidId.

Adds test/stack.test.ts.
- Move cardSignature/formatRelativeTime to ui/card-format.ts (pure, no obsidian
  runtime import) and test them; add processFileEdit fold tests and an
  edits malformed-input batch.
- Out-of-range entry edit/delete now errs instead of a silent no-op rewrite.
- Style: array methods over loops (parse isInside/result map, ids randomId,
  offsetOfLine reduce, sidebar find loops), inline anchorRange's anchored check
  to drop two non-null assertions, .then chain -> async/await in table
  renderedQuote, and why-comments on the detach idiom, per-document highlight
  registry, pop-out MutationObserver, and marker navigation.
- Enable noUncheckedIndexedAccess and fix the resulting real cases (regex-group
  and array-index accesses in parse, table-highlights, preview); drop the
  redundant noImplicitAny/strictNullChecks flags and the incoherent ES2018 lib
  entry (ES2020 supersets it; esbuild still targets es2018 for output).
- Lint the test/ directory too (oxlint src test); convert the 4 function
  declarations in tests to const arrows.
- Sync package.json version to the manifest (0.1.10).
- Strip a CRLF pair (not just the LF) when deleting a comment body.
Collapse the ~45-line new-comment composer duplicated in both margins into
ui/draft-composer.ts; each margin supplies only its submit/cancel behavior.
Selecting code inside a fence now creates a comment instead of refusing. The
markers wrap the whole block on their own lines (invisible, parser-visible);
the body stores the block-relative line range (line:) plus the exact code
(quote:, encoded so newlines/quotes/--> survive the header).

- format/code-anchor.ts: locate the enclosing fence, snap a selection to whole
  lines, and resolve a stored comment back to source offsets — fast path via the
  line range, fallback re-anchor by content (survives edits above), else orphan.
- Editor (Live Preview): hide the own-line markers with their newline (no blank
  lines around the block) and highlight the resolved target line, not the block.
- Reading view: highlight each target line in the rendered <pre> (works for
  plain blocks; syntax-highlighted blocks degrade gracefully — a precise
  highlight there via CSS Custom Highlight is a follow-up).
- Margin card aligns to the target line; orphaned code comments drop out of the
  inline column (still in the sidebar).
- Clean delete (markers + newlines) round-trips to the original block.

format/escape.ts gains a reversible code-quote codec. New test/code-anchor.test.ts
plus editor and edits coverage.
The own-line markers/body wrapping a code comment were hidden with inline
replaces that merge the hidden line into its neighbor. When the neighbor is a
``` fence, Live Preview stops recognizing the fence and leaves ghost vertical
space. Remove those lines as block decorations (from a StateField, which is
allowed to replace line breaks) so the fence lines are never merged into.
The own-line markers/body wrapping a code comment were hidden with a
block replace that absorbed a newline. Absorbing the newline directly
adjacent to a fence stopped Live Preview tagging that fence line — it
rendered as a full-height blank prose line (a ghost gap above/below the
block); absorbing a spacing blank line's newline shifted the block a row
toward its neighbor.

Replace only the marker/body text and keep every newline, so each hidden
line becomes a zero-height row and the real blank lines and fence rows
render at their natural, theme-driven height — a commented block now lays
out identically to an uncommented one, with no pixel math. The trailing
newline stays in the atomic range so a single arrow press still steps
over the invisible row instead of stalling on it.
computeDeleteComment's aloneOnLine check was LF-only, so on a CRLF file a
code comment's own-line markers left their \r\n behind as a stray blank
line around the block. This bites the raw-file delete path (sidebar and
Reading-view margin), which sees the file's real endings; the editor
path is LF-normalized and was unaffected.

Treat \r\n as a single line terminator — matching the body scan, which
already handled CR — so deleting a code comment round-trips cleanly on
CRLF files.
@kylemcd kylemcd changed the title Fix code-review findings: data integrity, write-path correctness, dedup fix: code-review findings: data integrity, write-path correctness, dedup Jul 22, 2026
A self-contained skill (skills/document-comments/) teaching an agent to
create, reply to, resolve, and delete the plugin's inline comments
correctly — leading with the alphanumeric-ID rule that silently breaks a
comment when violated. Ships the on-disk format reference, a
dependency-free validator (scripts/validate_comments.py), and the eval
suite used to verify it. Generic and portable — no personal or
project-specific references.

Transient eval run outputs (skills/*-workspace/) and the packaged .skill
artifact are gitignored; the source here is the single source of truth.
@kylemcd kylemcd changed the title fix: code-review findings: data integrity, write-path correctness, dedup Code-review findings, per-line code-block comments, and a document-comments agent skill Jul 22, 2026
@kylemcd
kylemcd merged commit d126cf1 into main Jul 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant