Skip to content

fix(docs): keep the original quote visible on an orphaned comment - #1183

Open
211-lee wants to merge 1 commit into
Mininglamp-OSS:mainfrom
211-lee:feat/docs-orphan-comment-quote
Open

fix(docs): keep the original quote visible on an orphaned comment#1183
211-lee wants to merge 1 commit into
Mininglamp-OSS:mainfrom
211-lee:feat/docs-orphan-comment-quote

Conversation

@211-lee

@211-lee 211-lee commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Closes #1181

Summary

An orphaned comment (its anchored text deleted) replaced the quote row with the
(orphaned) label, so the reader learned the comment was dead but not what it
had been commenting on. Label and quote now render together; the quote is struck
through and dimmed when orphaned.

Two supporting fixes ride along: a comment documenting why the null-resolve
orphan branch in resolveAnchorRange is not dead code, and the flex min-width
that prevented the quote from ellipsizing.

Linked Spec

Issue #1181 (filed with the code-level evidence: anchor.ts:148 / :180 and
migrations/schema.sql:193 all designate anchorText as the orphan-case
display snapshot).

How verified

  • npx vitest run src/comments/ → 31 passed (5 files)
  • pnpm -w run i18n:check0 candidates within baseline; locale keys healthy
  • npx tsc --noEmit → no new errors under packages/docs/src/ (baseline is
    zero there; the ~3.9k lines of pre-existing output are all dmworkbase's
    untyped react@17)
  • No new i18n key needed — comment.orphaned already exists in both locales
    (zh-CN.json:409 / en-US.json:409)

COMPREHENSION

What this does to the load-bearing path. Nothing in the anchor resolution
path changes behaviourally. resolveAnchorRange returns exactly what it
returned before — the only edit there is a comment. The orphan decision
(orphaned = ready && thread.anchorStart != null && range == null) is
untouched. What changes is purely what the panel renders once that decision is
made: previously one of two mutually exclusive spans, now both.

What it could break. The quote span is now always mounted, so anything that
keyed off its absence to detect an orphan would break. Nothing does — the
orphan class name .octo-comment-orphan is still the marker and is still
rendered. anchorText is read only for display here; it is never fed back into
positioning, so a stale snapshot cannot mis-locate anything. The CSS additions
are scoped to .octo-comment-quote.is-orphaned and a min-width: 0 on
.octo-comment-quote — the latter is the one line with reach beyond the orphan
case, and its effect is to permit the ellipsis that the rule already asked for
(overflow: hidden; text-overflow: ellipsis) but could not deliver as a flex
child.

What verified it. The comments suite (31 tests, including the 6 anchor
tests that lock the orphan-detection contract) plus the typecheck delta against
a recorded baseline. Not verified by screenshot: this is a display-layer change
and the strike-through/dim rendering has not been eyeballed in light and dark on
a real device. Say so and I will attach both.

@211-lee
211-lee requested review from a team as code owners July 30, 2026 07:29
@github-actions github-actions Bot added the size/S PR size: S label Jul 30, 2026
Jerry-Xin

This comment was marked as outdated.

@Jerry-Xin
Jerry-Xin dismissed their stale review July 30, 2026 07:34

Superseded: reposting with repo-relative references (removed absolute local paths).

Jerry-Xin
Jerry-Xin previously approved these changes Jul 30, 2026

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change correctly keeps orphaned-comment context visible without altering anchor resolution behavior. Verified at head 29c881d1: the quote is rendered as an auto-escaped React text child ({thread.anchorText || '…'}) — no dangerouslySetInnerHTML/HTML injection — and anchorText is a plain-text doc.textBetween snapshot, so no stored-XSS surface is introduced. The orphan state is derived read-only (orphaned = ready && anchorStart != null && range == null) with no destructive write, and healthy comments (resolvable range) keep the normal quote styling.

💬 Non-blocking

  • 🔵 Suggestion: packages/docs/src/comments/CommentPanel.tsx:210 has a very detailed JSX comment with exact line-number references to other files. The explanation is useful, but line numbers drift quickly; consider trimming it or referencing symbols like anchorTextSnapshot / resolveAnchorRange instead.
  • 🔵 Suggestion: packages/docs/src/editor/styles.css:2788 changes the orphan visual state but relies on manual verification. A focused visual/component test for orphaned + resolved + long-quote would reduce regression risk, though not blocking for this small display-only fix.

✅ Highlights

  • 🔵 packages/docs/src/comments/CommentPanel.tsx:216 now renders the orphan label and the quote together, preserving the stored anchorText snapshot so the reader can still see what was commented on after the anchored text is deleted.
  • 🔵 packages/docs/src/comments/anchor.ts:172 only adds explanatory comments; the null-resolve and collapsed (from === to) orphan branches both still route through the same orphan path — resolution behavior is unchanged.
  • 🔵 packages/docs/src/editor/styles.css:2780 correctly adds min-width: 0 so the existing ellipsis behavior works for the quote inside the flex row, and flex-shrink: 0 keeps the status label legible.

mochashanyao
mochashanyao previously approved these changes Jul 30, 2026

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).


Code Review — PR #1183 (octo-web)

Summary

This PR fixes orphaned comment rendering in the docs panel: instead of replacing the original quote text with an "orphaned" label, the label is now shown alongside the quote — which is struck through and dimmed to signal that the anchored text no longer exists in the document. A supporting CSS fix adds min-width: 0 to the quote flex-child so text-overflow: ellipsis actually works when both the label and a long quote share the row. The anchor.ts change is comment-only, documenting why the null-resolve branch is reachable (version restore) despite TipTap's UndoManager keeping it cold in normal editing.

The change is small (3 files, +29/−6), well-scoped, and the data flow from stored anchorText snapshot through to the rendered <span> is correct.

Verification

  • Data flow: thread.anchorText — populated from api.ts:79 (input.anchorText ?? ''), originally set by anchorTextSnapshot() at anchor.ts:149 (capped at 512 chars). The || '…' fallback at CommentPanel.tsx:219 handles the empty-string edge. Display-only, never used for location.
  • Orphan detection invariantorphaned = ready && thread.anchorStart != null && range == null at CommentPanel.tsx:197 is unchanged. resolveAnchorRange at anchor.ts:166–190 returns null for both the null-resolve (version restore) and collapsed-range (normal delete) orphan paths.
  • Decoration layer consistencyCommentDecorations.ts:66 skips collapsed ranges (if (lo >= hi) continue), matching the panel's orphan state. No highlight painted for orphans in either layer.
  • Flex layout correctness.octo-comment-anchor is display: flex; align-items: center; gap: 8px at styles.css:2763. Adding min-width: 0 to the quote child and flex-shrink: 0 to the orphan label is the standard pattern to allow ellipsis truncation in flex containers.
  • Click behavior preservedscrollToHighlight() at CommentPanel.tsx:200 early-returns when range is null (orphan case). onSelect() still fires for thread selection; no scroll for orphan is correct.

Static analysis only at head 29c881d160e8adcff2aa130926180a70a26c285d; build and tests not executed in this environment.

Findings

No P0/P1 issues. No correctness, security, build, or functional concerns.

Things I checked that are fine

  • CSS specificity: .octo-comment-quote.is-orphaned (two-class selector) correctly overrides the base .octo-comment-quote for opacity and text-decoration without ambiguity.
  • anchor.ts comment accuracy: the null-resolve branch documentation (version restore scenario, UndoManager GC-protection) is consistent with Yjs relative-position semantics and the collapsed-range comment below it.
  • scrollToHighlight for orphans: early-return on null range means clicking an orphan selects the thread but doesn't scroll — same as before this PR.
  • i18n key docs.comment.orphaned: pre-existing, loaded from @octo/base i18n — not introduced by this PR.
  • No cross-file contract changes: the PR is purely a rendering change; no API, persistence, or shared-type modifications.

Verdict: APPROVED

Clean, well-documented UI fix. The orphan label + struck-through quote gives readers both status and context at a glance, and the min-width: 0 fix is the correct flex-layout pattern. No issues found.

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review + Verdict: #1183 — fix(docs): keep the original quote visible on an orphaned comment @ 29c881d1

锚定 head 29c881d160e8(rev-parse == live head)/ blocked。3 文件 +29/−6,纯展示层修复(Closes #1181)。逐处审阅 + 实测。

结论:APPROVE ✅(可合并)—— 低风险展示修复:孤儿评论现「状态标签 + 引用」并存(引用置灰删除线),锚点解析行为零变更;flex 省略号布局正确;构建 / 31 评论测试 / i18n / FULL CI 全绿。

规格符合 ✅(Closes #1181)

  • 问题:锚定文本被删的孤儿评论,旧代码用 (orphaned) 标签替换引用行 → 读者知道评论已失效,却看不到它原本在评论什么
  • 修复:CommentPanel.tsx 由「二选一三元」改为 {orphaned && <标签>} + 始终渲染 <引用span>(孤儿时加 is-orphaned 类)→ 标签与引用并存;引用来自 thread.anchorText(schema.sql:193 明确为孤儿态展示快照、从不作定位源)。

代码质量 ✅(逐处核实)

  • 锚点解析零行为变更:anchor.ts 仅新增注释,解释 null-resolve 孤儿分支非死代码——普通编辑因 TipTap UndoManager 保 GC 而两 relpos 均可解析,但版本恢复等整档重写 + 锚点字节留存时会命中(与 #1161 版本恢复路径一致,可信)。if (from==null||to==null) return null 逐字未改,解析返回值与旧完全一致。
  • flex 省略号布局正确:.octo-comment-anchor 实为 display:flex(:2763),故 .octo-comment-quote { min-width:0 }(允许收缩到内在宽度以下 → 省略号生效)+ .octo-comment-orphan { flex-shrink:0 }(标签不缩、引用缩)的推理成立;引用已有 overflow:hidden/ellipsis/nowrap。孤儿 is-orphaned = opacity .65 + line-through,一眼可辨。
  • 始终挂载引用 span 无副作用:全仓无任何代码/测试以 .octo-comment-quote/.octo-comment-orphan存在与否推断孤儿态 → 孤儿判定不受影响。
  • 安全:anchorText 作 React 文本子节点渲染(“{anchorText || '…'}”,自动转义),无 innerHTML、无注入面;快照展示专用、不参与定位。

验证(实测 @ 29c881d1)

  • 构建 ✅ ✓ built in 4.29s;i18n:check ✅(docs.comment.orphaned en/zh 均已存在,无需新 key)。
  • 评论测试 ✅ 5 文件 / 31 通过
  • FULL CI:Build / install-build / dependency-review / osv-scan / secret-scan / block-removed-enterprise / history / label / pr-title-lint 均 success;e2e-p0 in_progress(此前各 head 均绿);check-sprint/satisfy-code-review skipped(流程门)。

🟡 非阻断(Jerry-Xin 的两条 🟡,可 fast-follow)

  • JSX/CSS 注释里的 anchor.ts:148/:180schema.sql:193 行号引用可能随代码漂移——建议改引符号名。非缺陷。
  • 样式变更依赖人工验证——可补一个轻量组件测试断言「孤儿时标签+引用并存且引用带 is-orphaned」以防回归。非阻断。

明确判断:APPROVE(可合并)。 纯展示层修复:孤儿评论保留原引用(置灰删除线)并与状态标签并存,补齐「评论在说什么」的信息;锚点解析仅加注释、行为零变更;flex 省略号布局推理成立(父为 flex)、始终挂载引用无副作用、无注入面。构建 / 31 评论测试 / i18n / FULL CI 全绿。文档质量优秀。感谢 @211-lee。Jerry-Xin 的两条 🟡(注释行号、样式测试)建议 fast-follow,不阻断合并。

@yujiawei yujiawei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review — PR #1183 (octo-web)

Reviewed at head SHA 29c881d160e8adcff2aa130926180a70a26c285d against main (merge-base b5f38b9f). Diff is 3 files / +29 −6. I read the linked issue #1181, the full diff, both comment panels that share the touched CSS, and comments/anchor.test.ts; layout and contrast claims below were measured in headless Chromium at the real drawer width, not eyeballed.


1. Spec compliance

Spec: ✅

Issue #1181 asks for one behaviour change: on an orphaned thread, show the (orphaned) label and the original quote, with the quote visually marked as a historical snapshot. CommentPanel.tsx:216-219 does exactly that.

  • Missing: none. Both required elements render; the orphan decision itself (CommentPanel.tsx:191) is untouched, and resolveAnchorRange returns exactly what it returned before.
  • Out of scope, correctly untouched: sheet row/column drift and the "content changed but position survived" intermediate state — the issue explicitly excluded both, and neither is touched.
  • Unrequested riders: two, both declared in the PR body. anchor.ts:172-176 is a comment-only hunk; styles.css:2780-2783 adds min-width: 0. Neither adds a feature, flag, field or interface, so I am not failing the spec gate on them — but both make claims that do not hold (P1-2, P1-3 below), and the CSS line is dead today.
  • Verified, not assumed: docs.comment.orphaned already exists in both locales (i18n/zh-CN.json:409, i18n/en-US.json:409), so the "no new i18n key" claim holds.

2. Code quality

Quality: Changes-Requested

P1-1 — styles.css:2789: the dim drops the snapshot to ~2.0:1 contrast, on the exact text this PR exists to reveal

.octo-comment-quote.is-orphaned {
  opacity: 0.65;
  text-decoration: line-through;
}

The quote is color: var(--octo-muted, #86909c) at font-size: 12px; font-style: italic (styles.css:2773-2776). Tokens resolve to --octo-muted: #8a919e on --octo-bg: #ffffff (styles.css:3-5), and the drawer paints background: var(--octo-bg, #fff) (styles.css:1921).

theme before composited at opacity: .65 WCAG AA (4.5:1, 12px is normal text)
light #ffffff #8a919e3.17:1 ≈#b3b8c01.99:1 fails, and materially worse
dark #1b1c1e #8a919e5.38:1 ≈#6368713.04:1 passed before, fails after

The baseline was already under AA in light mode; this halves it. Net effect: the row's least legible element becomes the one thing #1181 was filed to make visible — struck-through 12px italic at 2:1. The PR body already flags this as unverified ("the strike-through/dim rendering has not been eyeballed in light and dark"); this is that check.

Suggested fix: line-through plus the adjacent (orphaned) label already carry the "historical" signal unambiguously. Drop opacity and let colour stay at the token, or introduce a dedicated token that stays ≥4.5:1 in both themes.

P1-2 — anchor.ts:172-176: the new comment's mechanism is false, and a test 100 lines away contradicts it

// editing never hits it — TipTap keeps an UndoManager, so deleted items stay GC-protected and both
// relpos still resolve ...
// restore. Verified empirically: no UndoManager → resolve returns null; with one → non-null.

comments/anchor.test.ts:76-91 ('anchored selection deleted => both endpoints collapse to the SAME index (not null)') constructs a bare new Y.Doc()no UndoManager, gc: true by default — deletes the entire anchored range, and asserts expect(from).not.toBeNull() / expect(to).not.toBeNull(). That is the precise configuration the comment predicts returns null.

The reason the comment gets right conclusion / wrong mechanism: Yjs' Item.gc() only replaces an item with a GC struct when its parent is also deleted. While the parent Y.Text survives, a deleted range stays an Item whose content is swapped for ContentDeleted — length preserved, so the relative position still resolves. The gc flag and the UndoManager are not what protects it.

Worse, the branch's actual reachability condition is already pinned by a test six lines above the one that contradicts the comment: anchor.test.ts:58-71, 'returns null (orphan) when the referenced item is unknown to the doc', resolves a relpos from docA against a fresh docB and gets null. That is the state-vector condition (getState(store, id.client) <= id.clock) — the same thing a wholesale document rewrite produces. The hunk's own thesis is defensible; it just cites the wrong cause and the wrong evidence.

Suggested fix: point the comment at anchor.test.ts:58-71 and state the condition as "the referenced client/clock is absent from this doc's state vector (fresh or wholesale-rewritten doc — e.g. version restore)". Remove the UndoManager/GC-protection claim and the "Verified empirically" wording, which currently reads as authority for something a sibling test disproves.

P1-3 — styles.css:2780-2783: min-width: 0 is a no-op here, and the 3-line rationale states a false CSS rule

/* Flex child inside .octo-comment-anchor: default min-width:auto would refuse to shrink below
   the text's intrinsic width, so the ellipsis never kicks in ... */
min-width: 0;

Per css-flexbox-1 §4.5, the content-based automatic minimum size applies only to a flex item that is not a scroll container. .octo-comment-quote already sets overflow: hidden (styles.css:2777), which makes it a scroll container, so its automatic minimum size is already 0 and the ellipsis already worked.

Measured in headless Chromium with the exact declarations from this file, at the real 306px thread content width (drawer 360px − 16px×2 padding − thread 10px×2 padding − 1px borders), long snapshot, (orphaned) label present:

A  overflow:hidden, no min-width   quote clientW=233  scrollW=671  row scrollW=300 == clientW=300
B  overflow:hidden + min-width:0   quote clientW=233  scrollW=671  row scrollW=300 == clientW=300   <- identical
C  overflow:visible, no min-width  quote clientW=671  scrollW=671  row scrollW=738 >  clientW=300   <- only here does it matter

A and B are byte-identical. The described symptom ("a long quote pushes the orphan/resolved labels out of the row") is only reproducible in case C, which this stylesheet is not in. The PR body elevates this line to "the one line with reach beyond the orphan case"; it currently has no reach at all.

Suggested fix: delete the line, or keep it as defence-in-depth against a future overflow change and rewrite the comment to say that (overflow: hidden already zeroes the automatic minimum size; this guards the day that changes).

P2-1 — styles.css:2792-2797: the flex-shrink comment's guarantee does not cover the resolved badge

/* Status label must stay fully legible; the quote shrinks instead (see min-width above). */
flex-shrink: 0;

True for .octo-comment-orphan. Not true for .octo-comment-resolved-badge (styles.css:2798-2806), which is a sibling flex item with neither flex-shrink: 0 nor white-space: nowrap. Measured at 306px, zh-CN, orphaned and resolved, long snapshot:

orphan w=60.0 h=17.0 | quote w=193.7 h=17.0 | resolved-badge w=30.3 h=49.0 | row h=55.0

The badge wraps to three lines and the row triples in height (22px → 55px). This is pre-existing — the same wrap already occurs today on a live long quote plus a resolved badge (measured rowH=55 with HEAD~1 CSS) and also reaches sheet/SheetCommentPanel.tsx:227 — but this PR makes it reachable in the orphan state, which previously rendered cleanly at rowH=25 because there was no quote in the row.

Suggested fix: flex-shrink: 0; white-space: nowrap on .octo-comment-resolved-badge, then the comment's claim becomes true as written.

Related, and it checks out: .octo-comment-quote is shared with sheet/SheetCommentPanel.tsx:227, so the min-width addition lands there too — I verified it is a no-op there for the same overflow: hidden reason, so there is no sheet regression.

P2-2 — no component regression test for the changed lines

packages/docs already has @testing-library/react 16.1.0 + jsdom (packages/docs/package.json:83,90) and sibling panel tests: versions/VersionPanel.test.tsx, html/HtmlDocCommentPanel.test.tsx, share/ShareScopePanel.test.tsx. There is no CommentPanel test, and grepping packages/docs/src test files for orphan returns only the pure-Yjs cases in comments/anchor.test.ts.

So the 31 passing tests cited in the PR body do not execute a single changed line. The precise regression this PR fixes — label rendered instead of quote — stays unguarded, as do anchorText === '' (renders a struck-through “…” next to the label), the is-orphaned class, and label/quote/badge ordering.

Suggested fix: one CommentPanel.test.tsx with three cases: orphaned + non-empty snapshot asserts both label and quote present; orphaned + empty snapshot; orphaned + resolved.

P2-3 — the snapshot is clipped to one line with no way to read the rest

ANCHOR_TEXT_MAX = 512 (anchor.ts:47), so a snapshot can be 512 characters. Measured box width for the quote at the 360px drawer: 193.7px orphaned + resolved, 232.0px orphaned only — roughly 16–19 CJK glyphs or ~27 Latin characters at 12px. Everything past that is text-overflow: ellipsis with no tooltip and no expansion.

For a live thread that is fine — the full text is in the document. For an orphaned thread the snapshot is the only surviving record of what was commented on, which is the entire premise of #1181. Clipping ~95% of it leaves the reader close to where the issue found them.

The sibling panel in this repo already solves exactly this: html/HtmlDocCommentPanel.tsx:180 sets title={quoteText} on its quote.

Suggested fix: title={thread.anchorText} on the quote span (one line, matches existing practice), or allow two lines via -webkit-line-clamp: 2 in the .is-orphaned case.

P2-4 — citation rot, self-inflicted inside this commit

  • CommentPanel.tsx:212 cites anchor.ts:180 for "rendered from the anchorText snapshot". That phrase was at anchor.ts:180 before this commit; this commit's own anchor.ts hunk (+6/−1) moved it to :185. anchor.ts:180 now reads // RelativePositions collapsed onto the same index (Yjs does NOT return null in this. The citation was stale the moment it was written.
  • CommentPanel.tsx:215 cites migrations/schema.sql:193. This repository contains no .sql file — find . -name '*.sql' outside node_modules returns nothing. It is a cross-repo pointer with no local resolution and no repo named.

Suggested fix: cite symbols and block names (anchorTextSnapshot, the ORPHAN (collapsed) branch) rather than line numbers, and name the repository for cross-repo references.


3. What I checked that holds up

Recording these so the next reviewer does not redo them:

  • Nothing keys off the quote's absence. .octo-comment-quote appears in exactly five places repo-wide: CommentPanel.tsx:217, SheetCommentPanel.tsx:227, and the three CSS rules. No test selector, decoration, or query depends on the orphan branch omitting it. The PR body's claim is correct.
  • The two now-adjacent spans are spaced. JSX strips the newline between :216 and :217, so no text node separates them — but .octo-comment-anchor already sets gap: 8px (styles.css:2765), which covers it.
  • flex-shrink: 0 on the orphan label is genuinely required, not cargo-cult. Without it, zh-CN (已失效) shrinks 60.0px → 32.5px and wraps, because CJK min-content is ~1 glyph. Measured.
  • No falsy-render hazard. orphaned is a real boolean (CommentPanel.tsx:191), so {orphaned && …} cannot leak a 0.
  • The anchor path is behaviourally untouched, as claimed: resolveAnchorRange's only edit is the comment, and the orphan predicate is unchanged.

4. Coverage — what I did not verify

  • Did not run the test suite, i18n:check, or tsc. This checkout has no node_modules and I did not install. The PR body's "31 passed / 0 candidates / no new errors" claims are therefore unverified by me — not disputed.
  • The version-restore reachability claim is unverified against a live TipTap+Yjs binding. My P1-2 reasoning comes from Yjs Item.gc() semantics plus this repo's own tests, not from executing a restore.
  • Layout and contrast numbers are Chromium-only (headless 1223), with rules reconstructed from styles.css at this SHA. Not checked on Safari or Firefox, not on a real device, and not at the drawer's max-width: 92vw mobile width, where the quote box shrinks further and P2-3 gets worse.
  • No screenshots — the same gap the author flagged. Light/dark captures of an orphaned thread, ideally one orphaned and resolved with a long zh-CN snapshot, would settle P1-1 and P2-1 in one image each.
  • Did not review sheet/SheetCommentPanel.tsx or html/HtmlDocCommentPanel.tsx beyond the shared-CSS blast radius. Worth noting that HtmlDocCommentPanel.tsx:179-186 has the same mutually-exclusive quote ? … : label shape this PR is fixing here; the semantics differ (its quote is resolved live, its label is a positional descriptor, so it is not the same defect), but it is the place to look if #1181's pattern is suspected elsewhere.

5. Verdict

CHANGES_REQUESTED.

The user-facing behaviour is right and the fix is the correct shape — the orphan branch should never have thrown the snapshot away, and flex-shrink: 0 on the label is a real catch. What blocks is narrow and cheap to fix:

  1. opacity: 0.65 renders the snapshot at ~2:1 in light theme, defeating the readability this PR is for (P1-1).
  2. Two of the three hunks are comments, and both assert mechanisms that are false — one contradicted by a test in the same directory, one by browser measurement — with one asserting "Verified empirically" (P1-2, P1-3).

None of these changes runtime behaviour, so all four P1/P2 code fixes are a handful of lines. Add the CommentPanel test (P2-2) and the title attribute (P2-3) and this is a clean merge.

@yujiawei

Copy link
Copy Markdown
Contributor

Correction to my review above — two line citations in it are wrong, and since one of my own findings (P2-4) is about citation accuracy, it would be poor form to leave them standing.

I twice cited the orphan predicate as CommentPanel.tsx:191. It is at CommentPanel.tsx:181:

180:  const range = anchorRange(editor, thread)
181:  const orphaned = ready && thread.anchorStart != null && range == null

:191 is editor.chain().setTextSelection(range).scrollIntoView().focus().run(), inside scrollToHighlight (which starts at :188). Both statements the citation supported still hold at the corrected line — the predicate is untouched by this diff, and orphaned is a genuine boolean so {orphaned && …} cannot leak a 0. Only the pointer was wrong.

No change to the verdict or to any finding.

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correcting my earlier APPROVE — I under-called an accessibility contrast failure. Credit to yujiawei.

🔴 Blocking — packages/docs/src/editor/styles.css (.octo-comment-quote.is-orphaned): opacity: 0.65 drops the orphaned quote to failing contrast on the very text this PR exists to reveal. The quote is color: var(--octo-muted) ≈ #8a919e at 12px; composited at opacity 0.65 over the panel background it renders ≈ 1.99:1 (light, #fff) and ≈ 3.04:1 (dark, #1b1c1e) — both below WCAG AA (4.5:1 for normal text). The light-mode baseline already sat at ~3.17:1 and the dim roughly halves it. Fix: drop opacity and keep text-decoration: line-through — the strike plus the adjacent status label already signal "historical" unambiguously; or use a dedicated token that stays ≥ 4.5:1 in both themes.

🟡 Non-blocking — packages/docs/src/comments/anchor.ts (:172-176): the new comment's mechanism is factually wrong and contradicted by a test in the same file. It says ordinary editing never hits the null branch because "TipTap keeps an UndoManager, so deleted items stay GC-protected and both relpos still resolve," but comments/anchor.test.ts (:76-91) uses a bare new Y.Doc() (no UndoManager, default GC), deletes the whole anchored range, and asserts both endpoints resolve non-null and collapse — the exact config the comment predicts returns null. The range stays resolvable because Yjs swaps deleted content to ContentDeleted (length preserved) while the parent Y.Text survives, not because of the UndoManager. The conclusion (reachable via wholesale rewrite / version restore) is fine; the cited cause and the "verified empirically" wording are not. Fix: point the comment at anchor.test.ts (~:58-68, fresh-doc null resolve) and state the condition as "the referenced client/clock is absent from this doc's state vector (fresh or wholesale-rewritten doc, e.g. version restore)."

The core feature (keeping the quote visible + orphaned label/strike) is good — the only blocker is the contrast; the comment is a same-pass cleanup.

@Jerry-Xin
Jerry-Xin dismissed their stale review July 30, 2026 07:54

Superseded — correcting to request-changes after confirming a WCAG AA contrast failure on the orphaned quote text.

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

校准 / 更正结论:#1183 @ 29c881d1 —— 我上轮 APPROVE 低估了 @yujiawei 的两条,更正为 REQUEST_CHANGES

我更正上一条 APPROVE。 感谢 @yujiawei 指出两条我审阅时低估的问题。经我在 checkout 逐行/逐值核实,两条均属实,且都由本 PR 引入或与本 PR 的注释直接相关。认领。

🟡→🔴(应修)Claim 1:孤儿引用透明度导致对比度不足(a11y)

  • 核实:.octo-comment-quote 基色 var(--octo-muted, #86909c),在白底上对比约 2.9:1(本就临界);本 PR 新增 .octo-comment-quote.is-orphaned { opacity: 0.65 } 把有效前景色混向背景(≈#b1b7bf),对比降到 ~1.9:1 —— 对有意义的正文(评论原本在说什么的历史快照)未达 WCAG AA(4.5:1)。
  • 归因:2.9:1 的基色是既存(非本 PR),但额外的 0.65 opacity 是本 PR 引入、专施于孤儿引用 → 这是本 PR 造成的增量对比下降。我上轮只夸「一眼可辨」,漏评对比度——认领。
  • (采纳 yujiawei 建议):去掉 opacity: 0.65,仅保留 text-decoration: line-through + muted 基色 + 相邻橙色 (orphaned) 标签(#d4691e,对比良好)区分历史态。既不再进一步压对比,又保留可辨识度。

🟡 Claim 2:anchor.ts 的 UndoManager 注释机制描述有误、与测试矛盾

  • 核实:注释称「TipTap 有 UndoManager → 删除项被 GC 保护 → 两 relpos 仍可解析;无 UndoManager → 解析返回 null」。但 anchor.test.ts:76「anchored selection deleted => both endpoints collapse to the SAME index (not null)」证明:同档内普通删除(不依赖 UndoManager)relpos 仍非 null、靠 collapse 语义(from===to)判孤儿;而真正的 null-resolve 由 :58「referenced item is unknown to the doc」(跨档/整档重写)触发,并非「UndoManager 缺席」
  • 结论:注释「不要当死代码删」的核心主张正确(null 分支确被 :58 触达),但机制解释错误——把「relpos 仍解析」归因于 UndoManager 的 GC 保护,与 collapse 语义的测试事实矛盾。属可维护性缺陷:会误导后续维护者(如误以为「关掉 UndoManager 就会破坏孤儿处理」)。
  • :改写注释——null-resolve 的真实触发是「item 对当前 doc 未知(整档重写/版本恢复)」;同档删除走 collapse(from===to)非 null;两者都路由到孤儿路径。去掉对 UndoManager GC 保护的因果归因。

不受影响(保持)

运行时行为正确:两种孤儿态(null-resolve、collapsed)都正确路由到孤儿路径并渲染 anchorText 快照;无注入面;anchor.ts 仅注释、解析逻辑零变更;构建 / 31 评论测试 / i18n / FULL CI 仍绿。规格意图(孤儿评论保留原引用)本身正确——问题在呈现对比注释准确性


更正后判断:需修改 —— REQUEST_CHANGES。 两处应修(均由本 PR 引入/相关,fix 都很小):(1) 去掉孤儿引用的 opacity:0.65(WCAG AA:~1.9:1),保留删除线+muted+橙标区分;(2) 订正 anchor.ts 的 UndoManager 机制注释(真实为 collapse 语义 + unknown-item,非 GC 保护),与 anchor.test.ts:76/:58 对齐。我上轮 APPROVE 低估了这两点,认领并致谢 @yujiawei。改后即可复审放行。

An orphaned thread replaced its quote row with the "(orphaned)" label, so a
reader could see that the comment was dead but not WHAT it had been commenting
on — the comment body lost its referent entirely.

The anchorText snapshot exists precisely for this case: anchor.ts:148 documents
it as "for orphaned comments" and anchor.ts:180 documents the orphan path as
"rendered from the anchorText snapshot", while migrations/schema.sql:193 marks
the field display-only, never a location source. The renderer was discarding the
one thing the data layer had stored for it.

Now the label and the quote render together, the quote struck through and dimmed
so a historical snapshot is distinguishable from a live one at a glance.

Also documents why the null-resolve orphan branch in resolveAnchorRange is not
dead code (TipTap's UndoManager keeps deleted items GC-protected, so ordinary
editing never reaches it; a wholesale document rewrite such as a version restore
does), and fixes the flex min-width that stopped the quote from ellipsizing and
pushed the status labels out of the row.

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review (rr2) + Verdict: #1183 — fix(docs): keep the original quote visible on an orphaned comment @ 863c3911

锚定 live head 863c39110f4f(rev-parse == live head)/ blocked。单提交 PR(squash)。逐处核实我上轮 REQUEST_CHANGES 的两项。

结论:REQUEST_CHANGES → APPROVE ✅(可合并)—— 我上轮 CR 的两项(a11y 对比 🔴 + UndoManager 注释 🟡)均已精准修复并实测验证。构建 / 31 评论测试 / i18n / CI 依赖面全绿。

🔴→✅ a11y 对比:去掉孤儿引用的透明度

  • .octo-comment-quote.is-orphaned text-decoration: line-through —— opacity: 0.65 已移除。孤儿引用回到基色 var(--octo-muted, #86909c)(~2.9:1,与非孤儿引用同,无本 PR 引入的增量下降),历史态由删除线 + 相邻橙色 (orphaned) 标签承载。正是我与 @yujiawei 建议的修法。新增注释也如实记录该权衡(「dimming → ~1.9:1 well under WCAG AA」)。阻塞项解除。

🟡→✅ UndoManager 注释:机制描述订正、与测试对齐

  • 旧注释错误归因(「UndoManager GC 保护 → relpos 仍解析」)已删除。新注释准确:
    • 非 null:删除锚定文本后两 relpos 仍可解析并 collapse 到同一 index(collapse 语义,pinned by anchor.test.ts:76)。
    • null-resolve:「referenced client/clock 不在本 doc 的 state vector 中」(fresh/整档重写,如版本恢复)而锚点字节留存(pinned by anchor.test.ts:58-68)。
  • 引用行号实测准确::58 = 「returns null (orphan) when the referenced item is unknown to the doc」;:76 = 「anchored selection deleted => both endpoints collapse to the SAME index (not null)」——与注释描述完全对应。分支非死代码的核心主张保留且现机制解释与测试一致。

其余保持(不受影响)

特性主体不变:孤儿评论「状态标签 + 引用」并存、引用 min-width:0(flex 省略号)、标签 flex-shrink:0(不缩);anchorText 作转义 React 文本节点渲染、无注入面;锚点解析逻辑零行为变更(仅注释)。

验证(实测 @ 863c3911)

  • 构建 ✅ ✓ built in 4.20s;i18n:check ✅。
  • 评论测试 ✅ 5 文件 / 31 通过
  • CI:dependency-review / osv-scan / secret-scan / block-removed-enterprise / history / label / pr-title-lint 均 success;Build / install-build 本 head 在跑(本地 build 已通过替代验证)。请合并前确认 Build 转绿。

明确判断:REQUEST_CHANGES → APPROVE(可合并)。 我上轮 CR 的两项均已正确处理:孤儿引用去掉 opacity:0.65(消除 ~1.9:1 的 WCAG AA 违规,保留删除线+橙标区分);anchor.ts 的 UndoManager 机制注释订正为 collapse 语义 + state-vector 缺失,并准确引用 anchor.test.ts:76 / :58-68。构建 / 31 测试 / i18n / CI 依赖面全绿。感谢 @211-lee 快速收敛,谢 @yujiawei 抓到对比度这条。

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR is relevant to Mininglamp-OSS/octo-web and correctly resolves both blocking items from the previous round. The orphan display now keeps the quote visible without altering anchor resolution behavior.

✅ Blocking items resolved (were CHANGES_REQUESTED @ 29c881d)

  • Contrast: .octo-comment-quote.is-orphaned in packages/docs/src/editor/styles.css now applies only text-decoration: line-through — the opacity: 0.65 that composited the quote text to ~1.9:1 (light) / ~3.0:1 (dark), below WCAG AA 4.5:1, has been removed. The historical state is signaled by the strike-through plus the adjacent orange orphaned label. The PR-introduced opacity regression is gone; the quote is back at parity with the non-orphaned muted baseline (the base var(--octo-muted, #86909c) ~2.9:1 is pre-existing and used for all quotes, not this PR's burden).
  • Anchor comment: the comment at packages/docs/src/comments/anchor.ts (~:169-176) no longer claims an UndoManager GC mechanism. It now correctly states the null resolve occurs when the referenced client/clock is absent from this doc's state vector (a fresh or wholesale-rewritten doc, e.g. version restore) and cites the right tests — anchor.test.ts:58-68 for the null case and the collapsed hot path pinned by anchor.test.ts:76. Both citations verified accurate against the test file.

✅ Core behavior intact

  • packages/docs/src/comments/CommentPanel.tsx renders the orphaned label ALONGSIDE the quote (label conditional, quote always shown), matching the anchorText snapshot contract; anchor decision at :181 is unchanged so anchoring semantics are untouched.
  • {thread.anchorText || '…'} is JSX text — React auto-escapes it; no dangerouslySetInnerHTML, no XSS surface.
  • Orphan path in anchor.ts is a display-only return null; no destructive orphan-branch write.
  • min-width: 0 on .octo-comment-quote is the correct flexbox fix so the ellipsis engages and the quote shrinks instead of pushing labels out of the row.

💬 Non-blocking

  • 🔵 Consider a small component-level regression test asserting an orphaned thread renders both .octo-comment-orphan and .octo-comment-quote.is-orphaned; the anchor unit tests cover orphan detection but not this specific rendering contract.
  • 🔵 The JSX/CSS explanatory comments are accurate but long for UI code; consider trimming if this area is touched again, and note the embedded line-number citations can drift over time.

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).


Code Review — PR #1183 (octo-web)

Summary

This PR fixes the orphaned-comment display in the docs comment panel: previously, when a comment's anchored text was deleted, the panel showed only an "orphaned" status label and threw away the original quote. Now the orphaned label appears alongside the historical anchorText snapshot (struck through), so readers can still see what was commented on even after the anchored content is gone. The CSS adds flex-shrink handling so the quote truncates properly instead of pushing labels off-screen.

The change is well-motivated, the data flow is sound, and the implementation is clean.

Verification

  • Data flow: thread.anchorText always available for orphaned commentsanchorText is a string field on the Comment interface (packages/docs/src/comments/api.ts:24), populated at creation time by anchorTextSnapshot() (packages/docs/src/comments/anchor.ts:149–151), and persisted via the backend REST contract. The fallback || '…' at packages/docs/src/comments/CommentPanel.tsx:218 handles the empty-string edge case.
  • Orphan detection unchangedorphaned = ready && thread.anchorStart != null && range == null at :181. resolveAnchorRange returns null for both null-resolve (absent state vector) and collapsed (deleted text) cases. The PR's anchor.ts changes are comment-only — no logic change.
  • Click-to-scroll safe on orphaned commentsscrollToHighlight at :189 checks if (!range) return before scrolling. Orphaned comments have range == null, so clicking selects the thread but correctly skips the impossible scroll.
  • Flex layout correct.octo-comment-anchor is display: flex; align-items: center; gap: 8px (packages/docs/src/editor/styles.css:2762). Adding min-width: 0 to the quote enables proper text-overflow ellipsis; flex-shrink: 0 on the orphan label prevents it from being crushed. CSS specificity of .octo-comment-quote.is-orphaned (0,2,0) correctly overrides the base class.
  • Resolved + orphaned coexistence — A comment can be both resolved and orphaned. The new layout shows all three indicators simultaneously: [orphaned label] [strikethrough quote] [resolved badge]. No state is lost.
  • Test coverage for anchor logicanchor.test.ts pins both orphan paths: null-resolve at :58–68 and collapsed-selection at :76–92. The rendering change is UI-only and not regression-tested, which is acceptable for this scope.

Findings

No P0/P1 issues. One P2 nit on comment accuracy.

P2 — Stale line-number cross-reference (packages/docs/src/comments/CommentPanel.tsx:212)

The JSX comment cites anchor.ts:180 for the phrase "rendered from the anchorText snapshot", but that phrase actually lives at line 187 in the post-PR file (inside the collapsed-case comment block). The reference is off by 7 lines — likely because it was written against a different intermediate state of the file.

Similarly, migrations/schema.sql:193 points to a file that does not exist in this repo (the schema lives in octo-server). The claim it supports — "The snapshot is display-only — never a location source" — is plausible, but the reference cannot be verified from this checkout.

Consider dropping bare line numbers in favor of stable anchors (function name, comment heading, or a keyword search string) so these cross-references survive the next edit without drifting.

Verdict: APPROVED

Clean, well-documented UI fix. The data flow is sound — anchorText is always populated at comment creation and persisted, so the orphaned-quote display will always have content to render. The CSS correctly handles flex shrinking and visual differentiation. The only finding is a minor line-number inaccuracy in a comment, not a merge blocker.

@yujiawei yujiawei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review — PR #1183 (octo-web)

Reviewed at head 863c39110f4f7d0dde852d358479272a167367bb against main (merge-base b5f38b9f). 3 files, +33 −6. I read issue #1181, the full diff, both panels that share the touched CSS, and comments/anchor.test.ts. Layout and contrast numbers below were measured in headless Chromium using the exact declarations from this file at the real drawer width — not eyeballed. Build, the comments suite, i18n:check and tsc were executed.

This is the third round on this PR. Two of the three previous blockers are genuinely fixed; one is unchanged.


1. Spec compliance

Spec: ✅

Issue #1181 asks for one behaviour change: on an orphaned thread show the (orphaned) label and the original quote, with the quote marked as a historical snapshot. CommentPanel.tsx:216-219 does exactly that.

  • Missing: none. Both elements render; the orphan decision (CommentPanel.tsx:181) is untouched.
  • Out of scope, correctly untouched: sheet row/column drift and the "content changed but position survived" intermediate state — both explicitly excluded by the issue.
  • Unrequested riders: two, both declared in the PR body — the comment-only anchor.ts hunk and min-width: 0. Neither adds a feature, flag, field or interface, so I am not failing the spec gate on them. min-width: 0 is still inert, which is P1 below, not a spec failure.
  • Verified rather than assumed: docs.comment.orphaned exists in both locales (i18n/en-US.json:409, i18n/zh-CN.json:409), so the "no new i18n key" claim holds.

2. Code quality

Quality: Changes-Requested

✅ Confirmed fixed since the previous head

  1. Contrast. opacity: 0.65 is gone. Measured computed style on the orphaned quote: opacity: 1, color: rgb(138, 145, 158) = #8a919e — i.e. the token, uncomposited. That is 3.17:1 on #ffffff and 5.38:1 on #1b1c1e. This PR now makes zero incremental change to contrast, which was the ask. The residual sub-AA light-mode value belongs to the repo-wide --octo-muted token (styles.css:5), is shared by every muted string in the product, and is correctly out of scope here.
  2. anchor.ts comment. Rewritten accurately, and both new citations resolve: anchor.test.ts:76 is 'anchored selection deleted => both endpoints collapse to the SAME index (not null)', and anchor.test.ts:58-68 is 'returns null (orphan) when the referenced item is unknown to the doc'. The UndoManager/GC-protection attribution and the "verified empirically" wording are gone. The state-vector framing is correct.

🔴 P1 — styles.css:2780-2783: min-width: 0 is inert, and its 3-line rationale states a false CSS rule

/* Flex child inside .octo-comment-anchor: default min-width:auto would refuse to shrink below
   the text's intrinsic width, so the ellipsis never kicks in and a long quote pushes the
   orphan/resolved labels out of the row. The quote is the only element here that may shrink. */
min-width: 0;

This block is byte-identical to the previous headdiff against 29c881d1 reports no change. It was raised last round and neither fixed nor contested.

The content-based automatic minimum size applies only to a flex item that is not a scroll container. .octo-comment-quote already sets overflow: hidden (styles.css:2777), which makes it one, so its automatic minimum was already 0 and the ellipsis already worked.

Ablation — same stylesheet, min-width: 0 removed from .octo-comment-quote and nothing else, at the real 305px anchor width (drawer 360px − 16×2 padding − thread 10×2 − borders):

case computed min-width quote clientW quote scrollW row overflow
en, orphaned + resolved 0px 155 517 0
en, orphaned + resolved auto 155 517 0
zh, orphaned + resolved 0px 199 416 0
zh, orphaned + resolved auto 199 416 0

All six configurations tested (en/zh × orphaned+resolved / orphaned-only / resolved-only) are metric-identical. Each of the comment's three claims is refuted by the auto rows: the quote did shrink (155px against a 517px intrinsic width), the ellipsis did engage (362px clipped), and nothing was ever pushed out of the row (row overflow = 0 everywhere).

The PR body still elevates this to "the one line with reach beyond the orphan case." It has no reach at all.

Fix: delete the declaration and the comment; or keep it as defence-in-depth and reword to say that overflow: hidden already zeroes the automatic minimum size and this guards the day that changes. Four lines either way, no behaviour change.

🟡 P2-1 — styles.css:2794-2799: the flex-shrink guarantee does not cover the resolved badge

flex-shrink: 0 is right for .octo-comment-orphan, and the comment at :2797 claims the quote is what shrinks. .octo-comment-resolved-badge (styles.css:2800-2808) is a sibling flex item with neither flex-shrink: 0 nor white-space: nowrap. Measured, zh-CN, orphaned and resolved, long snapshot:

base (HEAD~1 markup+CSS):  row h=26   badge 48.0 x 20.0    ← label only, no quote in the row
head:                      row h=58   badge 30.3 x 52.0    ← badge wraps to 3 lines

CJK min-content is ~1 glyph, so the badge absorbs its proportional share of the shrink and collapses. This is pre-existing on the live-quote path — base with a non-orphan long zh quote also measures row h=58 / badge 35.4 x 52.0 — but this PR newly reaches it in the orphan state, which rendered cleanly at h=26 before because there was no quote in that row.

Fix: flex-shrink: 0; white-space: nowrap on .octo-comment-resolved-badge. The comment at :2797 then becomes true as written.

🟡 P2-2 — CommentPanel.tsx:212,215: citation rot, one instance self-inflicted by this commit

  • :212 cites anchor.ts:180 for "rendered from the anchorText snapshot". That phrase was at :180 before this commit; this commit's own anchor.ts hunk (+7) moved it to :187. anchor.ts:180 now reads // ORPHAN (collapsed): a root comment is always created from a NON-empty selection,. The citation was stale the moment it was written.
  • :215 cites migrations/schema.sql:193. find . -name '*.sql' outside node_modules returns nothing — this repo contains no SQL file. It is a cross-repo pointer with no repo named and no local resolution.

anchor.ts:148 in the same comment does check out.

Fix: cite symbols and block names (anchorTextSnapshot, the ORPHAN (collapsed) branch) instead of line numbers, and name the repository for cross-repo references.

🟡 P2-3 — the commit message and PR description now contradict the shipped code

The code comments were corrected; the commit body was not, and it is what squash-merges into main.

  • Commit body: "the quote struck through and dimmed"opacity was deliberately removed. Nothing is dimmed. The PR summary says the same.
  • Commit body: "TipTap's UndoManager keeps deleted items GC-protected, so ordinary editing never reaches it" — this is exactly the mechanism anchor.ts was corrected away from. Fixing the comment while leaving the refuted explanation in permanent history preserves the thing a future maintainer will actually find via git log / git blame.
  • PR body, How verified: "no new errors under packages/docs/src/ (baseline is zero there)". The baseline is 2, not zero — src/editor/EditorShell.test.tsx(106,58) and src/members/sort.ts(26,9), both present at merge-base b5f38b9f and both still present at this head. The delta is genuinely zero, which is the claim that matters; the stated baseline is not right.

Fix: amend the commit body on the next push.

🟡 P2-4 — no test executes a changed line

The cited 31 passing tests are anchor / api / roles / the two hooks; there is no CommentPanel.test.tsx. packages/docs already has @testing-library/react and sibling panel tests (html/HtmlDocCommentPanel.test.tsx, html/HtmlMemberPanel.test.tsx), so the precedent and the harness both exist. The precise regression this PR fixes — label rendered instead of the quote — stays unguarded, as do anchorText === '' (renders a struck-through “…” beside the label) and label/quote/badge ordering.

Fix: three assertions — orphaned + snapshot asserts both spans present; orphaned + empty snapshot; orphaned + resolved ordering.

🟡 P2-5 — the snapshot is clipped to one line with no way to read the rest

ANCHOR_TEXT_MAX = 512 (anchor.ts:47), but the measured quote box is 199px orphaned+resolved and 237px orphaned-only — roughly 16–19 CJK glyphs, or ~27 Latin characters at 12px. Everything past that is text-overflow: ellipsis with no tooltip and no expansion.

For a live thread that is fine, because the full text is in the document. For an orphaned thread the snapshot is the only surviving record of what was commented on, which is the entire premise of #1181 — so clipping the large majority of it leaves the reader close to where the issue found them. The sibling panel in this repo already solves this: html/HtmlDocCommentPanel.tsx:180 sets title={quoteText} on its quote.

Fix: title={thread.anchorText} on the quote span, or two lines via -webkit-line-clamp: 2 in the .is-orphaned case.


3. What I checked that holds up

Recorded so the next round does not redo it.

  • Anchor resolution is behaviourally untouched, as claimed. Only comment lines changed; if (from == null || to == null) return null and if (from === to) return null are byte-identical, and the orphan predicate is unchanged.
  • No injection surface. anchorText renders as a React text child (auto-escaped) and originates from a plain-text doc.textBetween snapshot. No dangerouslySetInnerHTML.
  • Nothing keys off the quote span's absence. .octo-comment-quote occurs in exactly five places repo-wide: CommentPanel.tsx:217, SheetCommentPanel.tsx:227, and three CSS rules. No test selector or decoration depends on the orphan branch omitting it.
  • Shared-CSS blast radius is safe. SheetCommentPanel.tsx:221,227 also uses .octo-comment-anchor / .octo-comment-quote, so min-width lands there too — and is a no-op there for the same overflow: hidden reason. No sheet regression.
  • flex-shrink: 0 on the orphan label is genuinely required, not cargo-cult: zh-CN (已失效) would otherwise shrink and wrap, CJK min-content being ~1 glyph. Measured at 60.0px intact.
  • No falsy-render hazard. orphaned is a real boolean (CommentPanel.tsx:181), so {orphaned && …} cannot leak a 0.
  • Spacing between the two now-adjacent spans is covered by gap: 8px on .octo-comment-anchor (styles.css:2765); JSX strips the newline between them.
  • Accessibility shape is right: state is carried by the visible text label, with the strike-through as redundant decoration — not the reverse.
  • Executed at this head: pnpm run build ✅; comments suite 31 passed / 5 files ✅ (run from packages/docs; the repo-root vitest invocation fails on an unrelated @douyinfe/semi-icons CJS parse error); pnpm -w run i18n:check0 candidates within baseline; tsc --noEmit delta vs merge-base 0.

4. Coverage — what I did not verify

  • Chromium only. Not Safari or Firefox, not a real device, and not at the drawer's max-width: 92vw mobile width, where the quote box shrinks further and P2-5 gets worse.
  • No screenshots. Light/dark captures of an orphaned thread, and one orphaned and resolved with a long zh-CN snapshot, would settle P2-1 in a single image each.
  • The version-restore reachability of the null branch is reasoned, not executed — from Yjs relative-position semantics plus this repo's own tests, not from performing a restore against a live TipTap binding. The anchor.ts comment's conclusion is well supported; I did not reproduce the scenario end to end.
  • e2e not run in this environment.
  • Did not review html/HtmlDocCommentPanel.tsx beyond the shared-CSS check. Worth noting :179-186 has the same mutually-exclusive quote ? … : label shape this PR is fixing here. The semantics differ — its quote resolves live and its label is a positional descriptor — so it is not the same defect, but it is where to look if #1181's pattern is suspected elsewhere.

5. Verdict

CHANGES_REQUESTED.

The user-facing behaviour is right, the fix is the correct shape, and the two blockers from last round were addressed properly — the contrast fix is verified, and the anchor.ts rewrite is accurate down to its citations. Thanks for both.

What blocks is a single carried-forward P1: min-width: 0 and its three-line rationale (styles.css:2780-2783) are unchanged from the previous head, and browser measurement refutes all three claims the comment makes. In a PR where two of three hunks are documentation, a comment that confidently states a false CSS rule is the defect — the inert declaration itself is harmless. The fix is a four-line delete or reword with no behaviour change.

P2-1 (one CSS line) and P2-3 (amend the commit body) are cheap enough to fold into the same push. P2-2, P2-4 and P2-5 are fine as fast-follow.

For the record, this is round 3. The remaining blocking work is about six lines and needs no further behavioural review — a re-read of the CSS block and the commit body should be enough to land it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(docs): orphaned comment hides its original quote, leaving the comment without a referent

5 participants