Skip to content

Comments v1 - #441

Merged
cscheid merged 5 commits into
mainfrom
comments-v1
Jul 30, 2026
Merged

Comments v1#441
cscheid merged 5 commits into
mainfrom
comments-v1

Conversation

@vezwork

@vezwork vezwork commented Jul 30, 2026

Copy link
Copy Markdown
Member

This PR adds a default block render component for q2-preview and revealjs formats that

  • shows comment threads and allows you to add comments and resolve them.
  • adds a toggle to the bottom bar that expands, collapses, and hides comment threads.
Kapture.2026-07-30.at.13.51.44.mp4

Notes

  • I didn't try to get commenting on list table entries working, disabled that
  • commenting doesn't work on figures for some reason (it writes, but it causes broken syntax) so I've disabled commenting on figures.
  • comment bubbles try to position themselves at the top right of their associated block, but, especially when in expanded mode, bubbles can get large and its impossible for them to stay in that position without overlapping with other bubbles. To address this problem I've added force layout that ensures bubbles don't overlap. When you hover over the right half of a block, it highlights the associated comment bubble and moves the bubble to the top right of the block, pushing other bubbles around to make that happen.
  • If you hover over the left half of a block, it doesn't cause any comment bubble interactions. I added this so people have an easy place to put their mouse while scrolling without causing bubbles to move around a bunch.

@posit-snyk-bot

posit-snyk-bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Comment on lines -15 to -19
#root {
width: 100%;
height: 100vh;
overflow: auto;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was causing there to be multiple scroll bars on the preview. It is necessary for revealjs tho, so that format adds this style programmatically now.

Comment on lines +1503 to +1509
// The replacer strips every pool-index-carrying key: `s` (node
// SourceInfo), `a` (AttrSourceInfo), and `targetS` (Link/Image
// URL+title source refs — missing it caused InvalidSourceInfoRef
// on any subtree containing a link).
const commitSubtreeEdit = (destinationSourceInfoJson: string, modifiedBlock: BlockNode) => {
const stripped = JSON.parse(JSON.stringify(modifiedBlock, (key, value) =>
key === 's' || key === 'a' ? undefined : value,
key === 's' || key === 'a' || key === 'targetS' ? undefined : value,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't know what is going on here, but I believe it fixed a bug where I could not edit or comment on anything that had a link in it.

@vezwork
vezwork requested a review from cscheid July 30, 2026 17:59
@cscheid cscheid mentioned this pull request Jul 30, 2026

@cscheid cscheid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from #442 (which we'll solve separately anyway), this looks great!

@cscheid
cscheid merged commit a6fc44b into main Jul 30, 2026
8 checks passed
@cscheid
cscheid deleted the comments-v1 branch July 30, 2026 18:18
cscheid added a commit that referenced this pull request Jul 30, 2026
…(bd-ddaqjb91) (#443)

* fix(preview): strip captionS and citationIdS sidecars in commitSubtreeEdit (#442)

Extract the commitSubtreeEdit stripping replacer into a shared
stripSourceInfoFields util and extend it to remove the two remaining
bare-pool-index sidecar keys: captionS (Table/Figure captions) and
citationIdS (Cite citation objects). Subtrees cloned from the
untransformed AST carry pool indices that apply_node_edit cannot
resolve (the replacement doc has no pool), so any surviving sidecar
fails the whole edit with InvalidSourceInfoRef — the same failure
PR #441 fixed for targetS on links.

The test file now imports the real util instead of re-declaring a
copy, and gains coverage for targetS (Link/Image), captionS (Figure),
and citationIdS (Cite). Table-internal sidecars (headS/footS/bodiesS/
rowsS/cellsS/bodyS) are left as-is: their pool-carrying leaves are
s/a keys the recursive replacer already removes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* plan: diagnosis + fix plan for bd-ddaqjb91 (CommentBlock crash in s0 tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(preview): CommentBlock tolerates malformed ResolvedSource; honest s0 stub (bd-ddaqjb91)

The Comments v1 merge (a6fc44b) made CommentBlock the registry's Block
wrapper, so every block render now consults resolveSource and passes
resolved.sourceNode to sameCommentableKind without checking it exists.
The s0-list-item-surfaces harness's resolveSource stub predates the
current ResolvedSource shape (no sourceNode, invalid 'Reachable' class,
bogus sourceIndex prop), so 18/23 tests crashed with
"TypeError: Cannot read properties of undefined (reading 't')".

Two fixes, TDD'd:
- CommentBlock treats a resolved entry without sourceNode like an
  unresolvable block (passthrough render), in both the chrome gate and
  resolveCommittable. New defensive test proves passthrough for the
  malformed shape and that the guard is not over-broad.
- The s0 stub now satisfies ResolvedSource for real (sourceNode: node,
  reachabilityClass 'Descendable', typed sourceEntry). s0: 23/23 pass.

Production was likely unaffected: the real resolveSource in
PreviewRoot.tsx always supplies sourceNode.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(ts): type-check test files in preview-renderer + preview-runtime (bd-ddaqjb91)

The s0 breakage was invisible because nothing type-checks test files:
the package tsconfigs exclude *.test.* and vitest transforms with
esbuild. Close the gap:

- tsconfig.tests.json in both packages (extends base, noEmit;
  noUnusedLocals/noUnusedParameters off — lint-grade, not drift
  detection), exposed as `npm run typecheck:tests`.
- cargo xtask verify step 11 runs both typechecks before the suites.
- Fix the pre-existing type errors the new gate surfaced:
  - preview-renderer (6 test files): typed vi.fn generics, Mock import,
    missing setLocalAst props, a useContext type typo, tuple-typed pool
    fixtures, two documented narrow casts.
  - preview-runtime: MockSyncClient's interface was missing
    applyEditorOperations (the impl had it); typed the handler mocks;
    cast once at the _setClientForTesting test seam.

Reintroducing the stale s0 stub now fails the typecheck
(TS2322: '"Reachable"' is not assignable to 'ReachabilityClass').

Also updates the plan doc checkboxes
(claude-notes/plans/2026-07-30-commentblock-defensive-resolvesource.md).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

3 participants