fix: 输入框 @mention 加浅紫胶囊背景，展示层对齐设计 token (#1153) - #1157
Conversation
…lamp-OSS#1153) - 输入框 TiptapMention 新增 renderHTML，按 isBroadcastSentinelUid 区分普通成员 mention 加 mention-user class，广播态（@所有人/@所有AI/-1/-2/-3/all）保持纯 mention class - CSS 新增 .wk-messageinput-editor .mention.mention-user 浅紫胶囊样式（对齐消息展示层 .mention-entity），含 hover 态；baseline .mention 保留透明背景供广播使用 - 新增 mentionStyle.test.tsx 锁 class 区分逻辑，覆盖普通用户/-2/-3/-1/all 四种 uid
- 将已废弃的 renderLabel 替换为 renderText，避免 @tiptap/extension-mention 内部短路导致 renderHTML 不生效（修复前普通成员 mention 始终走透明背景分支） - renderHTML 按 isBroadcastSentinelUid(uid) 区分：普通成员追加 mention-user class， 广播 sentinel（-1/-2/-3/"all"）保持纯 mention class（透明背景） - CSS 新增 .wk-messageinput-editor .mention.mention-user 胶囊样式 + :hover 态， 对齐消息展示层 .mention-entity；baseline .mention 保持透明背景用于广播态 - 新增 mentionStyle.test.tsx：4 条单测锁 class 区分（生产等价配置 renderText+renderHTML）
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review — PR #1157 (octo-web)
Summary
This PR adds a light-purple capsule background to regular member @mention nodes in the message input editor, visually aligning them with the message display layer's .mention-entity style. Broadcast mentions (@所有人, @所有AI, legacy @all) retain the existing plain-text purple style with transparent background. The implementation replaces the old renderLabel shorthand with explicit renderHTML + renderText on the Tiptap Mention extension, adding a mention-user CSS class only for non-broadcast mentions.
The change is well-scoped (3 files, +135/−3), the CSS values are an exact match with the display layer, and the new test file mirrors the production code 1:1.
Verification
- ✅ Broadcast sentinel coverage —
isBroadcastSentinelUid(atpackages/dmworkbase/src/Utils/mentionRender.ts:137) checks all four sentinel UIDs:"-1"(legacy all),"-2"(humans),"-3"(AIs),"all"(render-all). TherenderHTMLbranch atindex.tsx:577consumes this correctly. - ✅ CSS parity with display layer —
.mention.mention-userinindex.css:462uses identical values to.mention-entityinpackages/dmworkbase/src/ui/message/TextContent/index.css:43(samergba(107,61,216,0.08)background,4pxradius,2px 8pxpadding,font-weight: 500,cursor: pointer, hover0.12alpha). Visual consistency between editor and message display is preserved. - ✅ Send serialization unaffected — The send path uses
getJSON()→serializeEditorTextNodeForSend()(atindex.tsx:148,mentionSendParse.ts:228), which operates on the JSON document tree, notgetHTML(). TherenderHTML/renderTextchange only affects DOM rendering, so mention markers in sent messages are unchanged. - ✅ Paste comparison unaffected — Paste detection at
index.tsx:778,801comparesJSON.stringify(editor.getJSON())snapshots, notgetHTML(). No impact. - ✅ Test-production parity — The test's
createMentionEditor()at__tests__/mentionStyle.test.tsx:17replicates the productionrenderHTML/renderTextverbatim. All four sentinel UIDs are tested (individual cases for-2,-3, plus loop over-1/all), and a regular user mention (id: "user123") assertsmention-userclass +data-id/data-labelattributes. - ✅
renderHTMLattribute merge correct —{ ...options.HTMLAttributes, class: \mention${extraClass}` }atindex.tsx:581correctly overrides the baseclassfromHTMLAttributes: { class: "mention" }while preservingdata-type/data-id/data-label` set by the Tiptap Mention extension.
Findings
No P0/P1 issues. No correctness, security, build, or functional concerns.
Static analysis only at head 58d25333; build and tests not executed in this environment.
Things I checked that are fine
renderLabel→renderHTML+renderTextAPI migration: correct Tiptap Mention extension pattern.renderLabelis the legacy convenience; splitting into HTML/text is the canonical approach for custom markup.node.attrs.label ?? node.attrs.idfallback in bothrenderHTMLandrenderText: guards against missing label while keeping id as visible fallback. Consistent between the two render paths.String(node.attrs.id ?? ""): empty-string uid failsisBroadcastSentinelUid→ getsmention-userclass. An id-less mention node is edge-case-invalid; treating it as a regular user mention is harmless.- CSS selector specificity:
.mention.mention-user(two classes) correctly overrides.mentionbaseline without!important. Broadcast mentions match only.mentionand keepbackground-color: transparent.
Verdict: APPROVED
Clean, well-tested visual enhancement. The editor-layer mention styling now matches the display-layer capsule appearance for regular members while preserving the broadcast mention distinction. No defects found.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: The PR is relevant to octo-web and the mention rendering logic looks sound, but the CSS additions violate this repo’s explicit token rules.
🔴 Blocking
- 🔴 Critical:
packages/dmworkbase/src/Components/MessageInput/index.css:463-465adds hardcoded color, radius, and spacing values:rgba(107, 61, 216, 0.08),4px, and2px 8px.DEVELOPMENT.mdexplicitly forbids hardcoded colors/spacing/radius in component CSS. Move the mention color/background values into the theme token layer, and use existing spacing/radius tokens where possible, for examplevar(--wk-sp-0-5) var(--wk-sp-2)and an approved radius token or new theme token.
💬 Non-blocking
- 🔵 Suggestion:
packages/dmworkbase/src/Components/MessageInput/index.tsx:581currently replaces the configured class withmention${extraClass}. It is correct for today’s config, but preservingoptions.HTMLAttributes.classwould make this less fragile if the extension config later adds another class.
✅ Highlights
packages/dmworkbase/src/Components/MessageInput/index.tsx:575-587correctly switches torenderTextand keepsrenderHTMLresponsible for the broadcast/member class split.packages/dmworkbase/src/Utils/mentionRender.ts:137-143centralizes the sentinel set, so the new visual distinction stays aligned with send/paste safeguards.packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:51-107covers ordinary users and all broadcast sentinel cases.
Verification caveat: I attempted to run the focused Vitest command, but this checkout reports Command "vitest" not found for pnpm --filter @octo/base exec vitest ..., so I could not independently verify the test suite locally.
- CSS 硬编码 rgba/px/rem 全部替换为语义 token（wk-purple-alpha-08/12、wk-r-xs、wk-sp-0-5/2、wk-color-accent） - renderHTML 合并 class 时保留 options.HTMLAttributes.class，避免将来扩展 class 时被覆盖 - 测试配置同步更新，保持与生产逻辑一致
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1157 (octo-web)
Reviewed at head 58d25333af6f66d0861a3e0bfc97398b106ac7c7 against main. 3 files, +135/-3.
1. Spec compliance
Spec: ❌
Issue #1153 Expected Behavior: "输入框中的普通成员 @mention 应具有与消息展示一致的明确语义样式，例如紫色文字和浅紫背景". The PR description makes the same claim: "对齐消息展示侧 .mention-entity 的视觉样式".
-
Missing (漏建) — the capsule gets the background but not the colour.
.wk-messageinput-editor .mention.mention-user(packages/dmworkbase/src/Components/MessageInput/index.css:462-469) copiesbackground-color,border-radius,padding,font-weight,cursorandtransitionfrom.mention-entity, but notcolor. The text colour therefore still comes from the baseline rule atindex.css:453-459:.wk-messageinput-editor .mention { color: var(--wk-color-theme); ... }
--wk-color-themeis defined exactly once in the repo —packages/dmworkbase/src/App.css:4→#1C1C23(near-black). There is no other definition and no runtime override (verified by a repo-wide grep for--wk-color-themeassignments). The display side usescolor: #6B3DD8(packages/dmworkbase/src/ui/message/TextContent/index.css:43-51).Net rendered result after this PR: light-purple capsule + near-black text in the composer vs light-purple capsule + purple text in the bubble. The style jump described in #1153 is reduced, not removed. See P1-1 below.
-
Over-built (超建) — none. The diff is confined to the editor visual layer, exactly as #1153 recommends; the send payload, mention node attributes and
@[uid:label]draft grammar are untouched (extractMentionsFromEditorwalkseditor.getJSON()and is not affected by therenderLabel→renderTextswap). -
Divergence (偏离) — the broadcast branch is justified by a premise that does not match the display layer. See P1-2 below.
2. Code quality
Quality: Changes-Requested
P1-1 — Capsule text colour is never set, so the composer still does not match the bubble
packages/dmworkbase/src/Components/MessageInput/index.css:462-469
.wk-messageinput-editor .mention.mention-user {
background-color: rgba(107, 61, 216, 0.08);
border-radius: 4px;
padding: 2px 8px;
font-weight: 500;
/* no `color` */
}Evidence chain: index.css:454 color: var(--wk-color-theme) → App.css:4 --wk-color-theme: #1C1C23 → composer mention text is #1C1C23, while TextContent/index.css:44 sets #6B3DD8.
This is also a readability problem in dark mode, which is a live runtime state (packages/dmworkbase/src/App.tsx:174-183 sets body[theme-mode=dark]; DEVELOPMENT.md:174-176 documents it as the dark-mode mechanism). Neither .mention-entity nor the new .mention-user has a dark override, so in dark mode the composer renders near-black text on a dark background behind an 8%-alpha purple wash. The display side at least keeps #6B3DD8 and stays legible.
Fix: add the foreground colour alongside the background (ideally as a shared token — see P2-3), e.g.
.wk-messageinput-editor .mention.mention-user { color: #6B3DD8; /* or var(--wk-mention-entity-fg) */ }P1-2 — Broadcast mentions are excluded from the capsule based on an incorrect reading of the display layer
packages/dmworkbase/src/Components/MessageInput/index.tsx:575-583, index.css:452
The PR description and the new CSS comment both assert that broadcast mentions render as "纯紫色文字 + 透明背景" on the display side ("三级广播态设计"). The display code says otherwise:
packages/dmworkbase/src/Utils/mentionRender.ts:70-72—@所有人/@所有AIare synthesised as{ name, uid: "all" }.packages/dmworkbase/src/Messages/Text/mentionRenderState.ts:11-14—uid === "all"→className: "mention-entity", i.e. the same light-purple capsule as an ordinary member. Onlyuid === "channel"maps tomention-highlight(plain purple, no background), and"channel"is not producible from the composer (isBroadcastSentinelUidatmentionRender.ts:137-144covers-1/-2/-3/"all"only).- The comment at
mentionRender.ts:36-40states this explicitly: "Synthetic entries reuse theuid: "all"sentinel … while applying the same visual style as ordinary member mentions." - There is already a DOM-level regression test asserting it:
packages/dmworkbase/src/Messages/Text/__tests__/MarkdownContentMention.test.tsx:68— "广播 mention 与普通成员 mention 同款高亮（视觉一致）", asserting both@所有人and@张三land onspan.mention-entity.
Consequence: after this PR, typing @所有人 still shows plain text in the composer and then pops into a capsule the instant the message is sent — the exact input↔display jump #1153 was filed for, now left in place under a rationale that the code contradicts. Note #1153 says broadcast may ("可以") keep plain purple text, so this is a product call rather than a hard spec breach — but it has to be made with the correct facts.
Please pick one and make it explicit:
- Apply the capsule to broadcast mentions too (drop the branch entirely), matching
getMentionRenderState("all"); or - Keep the branch, and change the display side so broadcast maps to
mention-highlight(this would regress the GH#295 test above, so it needs its own decision); or - Keep the branch as a deliberate composer-only affordance, and correct the PR description and the
index.css:452comment so they do not claim a display-side behaviour that does not exist.
P2-1 — The new unit test asserts against a copy of the production config, so it locks nothing
packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:15-48
createMentionEditor() re-declares renderHTML / renderText inline instead of importing them from MessageInput/index.tsx. Deleting the renderHTML hook from production, flipping the isBroadcast ternary, or reintroducing renderLabel would all leave this suite green — it only tests the assertions' own fixture. The file header comment ("与生产代码 MessageInput/index.tsx 保持一致") is also not accurate: StarterKit.configure disables a different set of nodes than production (index.tsx:523-537) and suggestion is {} as any rather than createMentionSuggestion(...).
Suggestion: extract the class-selection logic into Utils/mentionRender.ts (e.g. mentionNodeClass(uid: string): string) and import it in both index.tsx and the test. That makes the class-distinction contract genuinely regression-locked at a cost of ~5 lines. Also worth asserting editor.getText() once, since renderText is the path getText() uses and it feeds the MAX_MESSAGE_LENGTH guard at index.tsx:941.
P2-2 — padding: 2px 8px on an inline node inside a fixed-line-height, scrollable contenteditable
index.css:466 vs index.css:433-444
The editor is font-size: 14px; line-height: 20px; max-height: 100px; overflow-y: auto. Vertical padding on an inline box does not grow the line box, so a ~21px capsule sits inside a 20px line: in wrapped multi-line compose the capsules of adjacent lines can visually touch or overlap, and a capsule on the first/last visible line can be clipped by the scroll container. The display side can afford the same padding because it has line-height: 22px and no height cap (TextContent/index.css:8). The 8px horizontal padding also widens the caret's hit area around an atom node.
Please confirm visually with 3–4 wrapped lines each containing a mention, at the 100px scroll cap. A line-height bump or a display: inline-block; vertical-align: baseline on the capsule may be needed.
P2-3 — Hard-coded colour/spacing values duplicated across two layers
index.css:463-467, index.css:472
DEVELOPMENT.md:127-141 ("禁止硬编码") requires semantic tokens for color / background / padding / border-radius, and DEVELOPMENT.md:176 additionally forbids hard-coded colours in dark styles. This block introduces rgba(107, 61, 216, 0.08), rgba(107, 61, 216, 0.12), 4px, 2px 8px and 500 as literals, duplicating the literals already in TextContent/index.css:43-55. The repo already carries a third, different mention purple — --wk-onboarding-mention-bg: rgba(124, 58, 237, 0.12) at packages/dmworkbase/src/theme/semantic.css:57.
Since "alignment between the two layers" is the whole point of this PR, a shared token (e.g. --wk-mention-entity-fg / -bg / -bg-hover in theme/semantic.css, consumed by both files) is what actually keeps them from drifting again; two copies of the same literal is what produced #1153 in the first place.
P2-4 — cursor: pointer promises an interaction the composer does not have
index.css:467
On the display side the capsule is genuinely clickable (mentionRenderState.ts:18-19 returns interactive: true for member uids). In the composer the Tiptap mention node is selectable: false, atom: true (@tiptap/extension-mention@3.22.2), so a click only moves the caret. Copying cursor: pointer across gives a false affordance inside a text field; cursor: text (or omitting the property) reads more honestly.
3. Verified as correct
Worth recording, because the renderLabel → renderHTML + renderText swap is the risky part of this diff and it checks out:
-
The PR's stated root cause is accurate. In
@tiptap/extension-mention@3.22.2, the node'srenderHTMLshort-circuits tothis.options.renderLabel(...)wheneverrenderLabelis defined, so a customrenderHTMLoption would never have run. DroppingrenderLabelis required, and it also removes aconsole.warn("renderLabel is deprecated…")that previously fired on every mention render. -
No DOM attributes are lost. Tiptap builds
mergedOptions.HTMLAttributes = mergeAttributes({ 'data-type': name }, this.options.HTMLAttributes, HTMLAttributes)before invoking the user hook, and that third argument is the node-attribute-derived record. So{ ...options.HTMLAttributes, class: … }still carriesdata-type,data-id,data-labelanddata-mention-suggestion-char. Executed against the installed 3.22.2 with the exact production config, the output is:<span data-type="mention" class="mention mention-user" data-id="user123" data-label="Alice" data-mention-suggestion-char="@">@Alice</span>
and feeding that HTML back through
setContent()parses to{"type":"mention","attrs":{"id":"user123","label":"Alice","mentionSuggestionChar":"@"}}, so the copy/paste and draft round-trip is intact. -
editor.getText()output is byte-identical before and after the change for every uid tested (user123,-1,-2,-3,all), so theMAX_MESSAGE_LENGTHguard, the slash-command detector (index.tsx:616) and the multi-line heuristics are unaffected. The only difference is thelabel == nulledge case, which improves from@nullto@<id>. -
The send payload cannot be affected.
extractMentionsFromEditor(index.tsx:372-403) walkseditor.getJSON()and callsserializeMentionMarker(node.attrs.id, node.attrs.label, trusted); it never touchesrenderText/renderHTML.MENTION_TRUST_MARKhandling and the broadcast trust boundary are untouched. -
CSS specificity is correct:
.wk-messageinput-editor .mention.mention-user(0,3,0) beats the baseline.wk-messageinput-editor .mention(0,2,0), sobackground-color: transparent,padding: 0 0.125remandfont-weight: 600are all overridden as intended. -
The new CSS is scoped under
.wk-messageinput-editor, so the second Tiptap Mention instance inpackages/docs/src/editor/mention.tsis not affected.
4. Overall verdict
CHANGES_REQUESTED
Spec ❌ (P1-1: the requested purple text is missing, so composer and bubble still differ) AND Quality Changes-Requested → blocked. The mechanism of the fix is sound and well chosen; what is missing is the last property of the style it claims to mirror, plus a corrected decision on the broadcast branch.
Minimum to unblock:
- Set
coloron.mention.mention-user(P1-1). - Resolve the broadcast branch against
getMentionRenderState("all")— change the behaviour or correct the stated rationale (P1-2).
Strongly recommended in the same pass: make the test exercise production code (P2-1) and move the shared colours into a token (P2-3), since drift between these two layers is the root cause of #1153.
5. Coverage / what was not verified
- The PR's own Vitest suite was not executed in this review (no
node_modulesin the review worktree). Tiptap semantics were verified with a standalone harness against the installed@tiptap/extension-mention@3.22.2instead. The claim of 103 passing tests is taken at face value. - No browser or computed-style verification. #1153 is a purely visual bug, and no before/after screenshot exists on this PR. P1-1 (rendered colour), P2-2 (line-box overlap and scroll clipping) and dark-mode legibility all need a real render to close out. A screenshot of the composer next to the sent bubble, in both themes, would settle P1-1 and P1-2 in one image and is the single most useful artefact to attach here.
- Dark mode (
body[theme-mode=dark]) was traced in CSS but not exercised. - Behaviour with
labelcontaining markup, and IME composition adjacent to a padded capsule, were not tested.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Review + Verdict: #1157 — fix: 输入框普通成员 @mention 加浅紫胶囊背景 (#1153) @ 1ba5c3d1
注意 head 已前进:请求时给的是 58d25333,但 live head 已推进到 1ba5c3d1(新增 commit「fix: 输入框 mention 样式改用设计 token」)。我锚定 live head 1ba5c3d1(rev-parse == live head)/ merge:blocked(仅缺当前 head 的通过票)复审。
结论:APPROVE ✅ —— 可合并。
✅ 已解除:@Steve / @Jerry-Xin 在 58d25333 上判的 🔴 CSS 硬编码(DEVELOPMENT.md token 规范)已在 1ba5c3d1 修复
他们复审的是旧 head——那版 index.css:463 硬编码 rgba(107,61,216,0.08) / 0.25rem / 0.125rem,违反「颜色/间距/圆角一律走 var(--wk-*)」。新 commit 全部改为设计 token(实测 grep 确认旧硬编码值零残留):
--wk-r-xs(3px)/--wk-sp-0-5(2px)/--wk-sp-2(8px)/--wk-color-accent(→--wk-purple-500)/--wk-purple-alpha-08(rgba(127,59,245,.08))/--wk-purple-alpha-12(hover)。- 逐一核实这些 token 均已在 token 层定义(
theme/semantic.css+theme/primitive.css)——符合「新色值须先进 token 层」。胶囊色由 ad-hoc107,61,216归到设计系统紫127,59,245(对齐展示层.mention-entity),正是该规范期望的做法。
规格符合 ✅(#1153)
普通成员 @mention 加浅紫胶囊、@所有人/所有AI(sentinel -1/-2/-3/"all")保持纯紫透明——由 renderHTML 按 isBroadcastSentinelUid(node.attrs.id) 分 class(普通追加 mention-user、广播保持 mention)。.mention-user 类仅作用于 .wk-messageinput-editor,零其他消费方、无附带回归。
代码质量 / 安全 ✅
- 只改展示、不动序列化:出站消息文本走
serializeMentionMarker(node.attrs.id, node.attrs.label)(直接读 node attrs),与 renderHTML/renderText 无关。renderLabel → renderText的替换是必需的(tiptap 在废弃renderLabel存在时会短路renderHTML),产出文本一致(还补了?? id兜底)。 - mention 仍是单个原子 Tiptap 节点,胶囊纯 CSS 装饰(bg/radius/padding)→ 无光标陷阱、不破坏原子删除、不改序列化。
cls基于options.HTMLAttributes?.class ?? "mention"拼接,保留调用方传入的 class(Steve 上轮 nit 已被这版写法规避)。- 无依赖爆炸半径(#1147 类):未碰 pnpm-workspace/lock/任何 package.json。
验证(实测 @ 1ba5c3d1)
- 硬编码零残留:
grep rgba\(107 / 0.25rem / 0.125rem→ NONE ✅。所有 token 定义存在。 - 构建 ✅:
pnpm --filter @octo/web build→✓ built in 3.74s。 - 测试 ✅:
MessageInput/__tests__/7 文件 / 103 测试通过,含新增mentionStyle.test.tsx5 case——普通用户含mention-user+data-id/data-label,-1/-2/-3/"all"均不含mention-user,锁定 broadcast 分流。
🟡 非阻断
mentionStyle.test.tsx用复刻的 TiptapMention 配置(而非直接导入生产 config)来防短路,与生产MessageInput/index.tsx的 renderHTML/renderText 是并行副本——两者若未来漂移,测试可能失去保护。可考虑抽出共享的 mention-render 配置工厂,让生产与测试引用同一份。
明确判断:APPROVE(可合并)。 同事在旧 head 58d25333 判的 🔴 CSS 硬编码违规,已在 live head 1ba5c3d1「改用设计 token」commit 中修复且 token 均在层内定义;规格分流(普通/广播)正确、序列化不受影响、纯展示零回归;构建通过、mention 测试 103/103 通过。感谢 @Steve、@Jerry-Xin 精确定位这条 token 规范 🔴——作者已按其修复。
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1157 (octo-web), re-review at 1ba5c3d1
The head advanced from 58d25333 to 1ba5c3d1 ("输入框 mention 样式改用设计 token") while the previous review was being written, so that review is pinned to the older commit. This is the formal verdict for the current head 1ba5c3d1538e2d781b0e74b423af4611e5861a60; it supersedes the previous one. Only the deltas are re-argued below — everything not mentioned here is unchanged and still applies.
1. Spec compliance
Spec: ❌ (was ❌ — one of the two reasons is now resolved)
Resolved. P1-1 is fixed. .wk-messageinput-editor .mention.mention-user now sets color: var(--wk-color-accent) (packages/dmworkbase/src/Components/MessageInput/index.css:462), which resolves through theme/semantic.css:25 → theme/primitive.css:20 → #7f3bf5. The composer mention is now purple text on a purple capsule, and it stays legible under body[theme-mode=dark]. The user-visible symptom in #1153 is genuinely gone.
I also verified the token migration is safe rather than a silent no-op, because that was the obvious risk in this commit:
- All six new tokens exist at
:root:--wk-color-accent(semantic.css:25),--wk-purple-alpha-08/-12(primitive.css:64,66),--wk-r-xs(semantic.css:206),--wk-sp-0-5/--wk-sp-2(semantic.css:193,196). - They reach the production bundle:
apps/web/src/index.tsx:3imports@octo/base/src/theme/tokens.css, which forwards via@import './index.css'→primitive.css+semantic.css. The@import-expansion caveat inDEVELOPMENT.md:150-160is Storybook-specific; these tokens are already relied on by shipped component CSS in this package (392 uses of--wk-sp-2, 40 of--wk-r-xs, 15 of--wk-color-accent), so an unresolved chain would have broken the UI long ago. - Values are preserved where intended:
--wk-sp-0-5= 2px and--wk-sp-2= 8px, so the2px 8pxpadding is unchanged.
Still missing (漏建). The stated goal — "对齐消息展示侧 .mention-entity 的视觉样式" — is still not met; the gap moved from absence to hue:
| property | composer (.mention.mention-user, new) |
bubble (.mention-entity, TextContent/index.css:43-51) |
|---|---|---|
color |
#7f3bf5 (--wk-color-accent) |
#6B3DD8 (hard-coded) |
background-color |
rgba(127, 59, 245, 0.08) |
rgba(107, 61, 216, 0.08) |
border-radius |
3px (--wk-r-xs) |
4px (hard-coded) |
Two different purple families now coexist. To be clear, I think this commit picked the right purple — #7f3bf5 is the token-sanctioned one and .mention-highlight already uses rgba(127, 59, 245, 1) (TextContent/index.css:37); #6B3DD8 is the outlier. The problem is that the PR only migrated one of the two layers, so it has replaced "two hard-coded copies of the same colour" with "one token and one divergent hard-coded colour", and the description still claims parity that does not exist.
Completing this is small and is the change that actually stops #1153 from recurring: point .mention-entity / .mention-entity:hover at the same tokens (TextContent/index.css:43-55). Then both layers have one source of truth, and the table above collapses to identical.
Still divergent (偏离). P1-2 is unchanged — see below.
Unrequested (超建), nit. The baseline .mention rule also changed border-radius: 0.25rem → var(--wk-r-xs) (4px → 3px) and padding: 0 0.125rem → 0 var(--wk-sp-0-5) (2px → 2px). The radius change is not required by #1153 and applies to the broadcast branch; it is harmless in practice because that branch has background-color: transparent, so no corner is visible. Worth a line in the description rather than a code change.
2. Code quality
Quality: Changes-Requested
P1-2 (unchanged, blocking) — broadcast mentions are excluded from the capsule on a premise the display layer contradicts
packages/dmworkbase/src/Components/MessageInput/index.tsx:575-585, index.css:452
Restating the evidence, since this is the one blocker left:
packages/dmworkbase/src/Utils/mentionRender.ts:70-72—@所有人/@所有AIare synthesised as{ name, uid: "all" }.packages/dmworkbase/src/Messages/Text/mentionRenderState.ts:11-14—uid === "all"→className: "mention-entity", i.e. the same capsule as an ordinary member. Onlyuid === "channel"maps tomention-highlight, and"channel"is not producible from the composer (isBroadcastSentinelUid,mentionRender.ts:137-144, covers-1/-2/-3/"all"only).mentionRender.ts:36-40says so in prose: "Synthetic entries reuse theuid: "all"sentinel … while applying the same visual style as ordinary member mentions."packages/dmworkbase/src/Messages/Text/__tests__/MarkdownContentMention.test.tsx:68is a DOM-level regression test named "广播 mention 与普通成员 mention 同款高亮（视觉一致）" asserting both@所有人and@张三land onspan.mention-entity.
So @所有人 typed in the composer still shows plain text and then pops into a capsule the moment it is sent — the same input↔display jump #1153 exists to remove, now with a rationale ("三级广播态设计", index.css:452 comment) that the display code does not implement. #1153 says broadcast may ("可以") stay plain purple, so this is a product call — it just has to be made against the real display behaviour.
Pick one and make it explicit:
- Drop the branch so broadcast also gets the capsule, matching
getMentionRenderState("all"); or - Change the display side so broadcast maps to
mention-highlight(this regresses the GH#295 test above, so it needs its own decision); or - Keep the branch as a deliberate composer-only affordance, and fix the description and the
index.css:452comment so neither claims a display-side behaviour that does not exist.
P2-1 (unchanged, and this commit demonstrates the cost) — the test asserts against a copy of the production config
packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:15-49
This commit had to apply the identical baseClass / cls edit twice — once in index.tsx:578-582 and once in the test's own duplicate at mentionStyle.test.tsx:36-40. That is the maintenance cost made concrete: the test still exercises its own fixture, so deleting renderHTML from production, flipping the isBroadcast ternary, or reintroducing renderLabel would leave the suite green.
Extract the class-selection logic into Utils/mentionRender.ts (e.g. mentionNodeClass(uid: string, baseClass: string): string) and import it in both places. ~5 lines, and the class-distinction contract becomes genuinely locked.
P2-2, P2-4 (unchanged)
padding: var(--wk-sp-0-5) var(--wk-sp-2) still computes to 2px 8px against .ProseMirror { font-size: 14px; line-height: 20px; max-height: 100px; overflow-y: auto } (index.css:433-444), so the line-box overlap / scroll-clipping check in the previous review still stands (the bubble side can afford it at line-height: 22px, TextContent/index.css:8). cursor: pointer (index.css:467) still advertises a click the composer does not handle — the Tiptap mention node is selectable: false, atom: true, so a click only moves the caret, whereas the bubble capsule really is interactive (mentionRenderState.ts:18-19).
Improvement worth noting
const baseClass = options.HTMLAttributes?.class ?? "mention" is a genuine robustness improvement over the previous hard-coded "mention" literal — the class now follows TiptapMention.configure({ HTMLAttributes }) instead of silently duplicating it.
3. Still verified as correct at this head
The renderLabel → renderHTML + renderText swap is unchanged by this commit and re-checked against the installed @tiptap/extension-mention@3.22.2: data-type, data-id, data-label and data-mention-suggestion-char all survive (Tiptap merges the node-attribute record into mergedOptions.HTMLAttributes before invoking the hook), the produced HTML round-trips back through setContent() to the same mention node, editor.getText() is byte-identical to the pre-PR output for every uid tested, and the send payload is untouched because extractMentionsFromEditor (index.tsx:372-403) walks editor.getJSON(). Dropping renderLabel also silences a deprecation console.warn that previously fired on every mention render. CSS specificity is still correct (0,3,0 beats 0,2,0).
4. Overall verdict
CHANGES_REQUESTED
Real progress — the user-visible defect in #1153 is fixed for member mentions, and the token migration is both correct and the repo-idiomatic way to do it. Two things still block:
- P1-2: resolve the broadcast branch against
getMentionRenderState("all")— change the behaviour, or correct the rationale in the description and theindex.css:452comment. - Spec 漏建: finish the alignment by migrating
.mention-entity(TextContent/index.css:43-55) onto the same tokens, so composer and bubble share one source of truth instead of two purples. If you would rather keep that out of scope, then drop the "对齐.mention-entity" claim from the description and file the follow-up.
Recommended in the same pass: P2-1 (make the test exercise production code — this commit already paid the duplication cost once).
Process note: counting the earlier reviews on this PR, this is the third review round. If the next round does not close P1-2, it is probably worth agreeing the intended three-tier visual contract for composer and bubble in the issue first, rather than iterating on the CSS.
5. Coverage / what was not verified
- Vitest was not executed here (no
node_modulesin the review worktree); Tiptap semantics were verified with a standalone harness against the installed 3.22.2. The claim of a passing suite is taken at face value. - No browser or computed-style verification. #1153 is a purely visual bug and there is still no before/after screenshot on this PR. The exact rendered purple, the
#7f3bf5vs#6B3DD8delta next to a real bubble, P2-2 (line-box overlap and scroll clipping at the 100px cap), and dark-mode appearance all need a real render. A single screenshot of the composer beside the sent bubble, in both themes, would close the spec question and P1-2 at once and is the most useful thing to attach. - Dark mode (
body[theme-mode=dark], set atApp.tsx:174-183) was traced in CSS but not exercised. - Token resolution was verified by static chain-tracing plus the argument from existing widespread usage, not by inspecting a built CSS bundle.
- IME composition adjacent to a padded capsule, and
labelvalues containing markup, were not tested.
- renderHTML 去掉 mention-user 分支，广播和普通成员统一胶囊（对齐展示层 uid==="all" → .mention-entity） - class 选择逻辑提取到 mentionNodeClass(uid, baseClass)，index.tsx 与测试共用 - .wk-messageinput-editor .mention 统一色值/背景/圆角/间距 token；cursor 改为 text（输入框内不可点击） - .wk-msg-text-content .mention-entity / .mention-fallback / .mention-highlight 全部改用 var(--wk-color-accent) 等设计 token，消除输入框/展示层紫色漂移 - 单测更新：断言所有 uid（含广播 sentinel）均渲染为 class=mention
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review: octo-web PR#1157 — fix: 输入框普通成员 @mention 加浅紫胶囊背景
Reviewer: Octo-Q (automated review)
Head SHA: b8311762df951ad348db3f0758a7d27c3a790b29
Base: main
Files changed: 5 (+147 / -17)
1. Verification Summary
| Area | Status | Evidence |
|---|---|---|
| CSS custom properties defined | ✅ | --wk-color-accent → semantic.css:25 → var(--wk-purple-500) → primitive.css:20 = #7f3bf5; --wk-purple-alpha-08 = rgba(127,59,245,0.08) at primitive.css:64; --wk-purple-alpha-12 at primitive.css:66 |
renderHTML DOMOutputSpec format |
✅ | Returns ["span", {attrs}, text] — valid Tiptap DOMOutputSpec |
renderText replaces deprecated renderLabel |
✅ | renderLabel was blocking renderHTML invocation (Tiptap deprecated path); now correctly split |
isBroadcastSentinelUid still functional |
✅ | Unchanged; still guards send-side broadcast routing in mentionRender.ts:136-143 |
| Display-side token unification | ✅ | mention-entity / mention-highlight / mention-fallback now all use --wk-color-accent + design tokens, matching input side |
| New test coverage | ✅ | 5 test cases covering normal user, -1, -2, -3, "all" UIDs |
2. Findings
F1 — P2 (maintainability): mentionNodeClass accepts uid parameter but never reads it
File: packages/dmworkbase/src/Utils/mentionRender.ts:149-152
export function mentionNodeClass(uid: string, baseClass: string = "mention"): string {
return baseClass;
}The uid parameter is accepted and passed from renderHTML (index.tsx:576-577) but the function body unconditionally returns baseClass. The JSDoc explains the rationale (broadcast and member mentions share visual treatment), but a future maintainer reading the signature will assume uid-dependent branching exists or is planned.
Diff-scope: New code (new function). Not pre-existing.
Suggestion: Either drop the uid parameter (and update the call-site in index.tsx:577 to just pass baseClass directly), or add a _uid prefix to signal it's intentionally unused (TypeScript noUnusedParameters-friendly). If future differentiation is planned, a TODO comment would be clearer.
F2 — P2 (documentation): PR body contradicts implementation — claims broadcast transparency, code unifies capsule
PR body states:
@所有人/@所有AI（broadcast sentinel uid: -1/-2/-3/"all"）保持原有纯紫色文字+透明背景，不使用胶囊样式（三级广播态设计）
Actual implementation:
mentionNodeClass()returns"mention"for ALL UIDs including broadcast sentinels- CSS applies capsule styling (
background-color: var(--wk-purple-alpha-08),border-radius,padding) to.wk-messageinput-editor .mention— which covers every mention node - Code comment in
mentionRender.ts:144-148explicitly says "广播与普通成员视觉一致（胶囊背景）" and "输入侧不一致会造成发送后跳变"
The code behavior (unified capsule) is the correct design choice — the comment explains the send-vs-display jump rationale. The PR body appears to describe an earlier iteration of the approach.
Diff-scope: New discrepancy introduced by this PR. Not pre-existing.
Suggestion: Update the PR body to match the implementation: all mentions get capsule styling in the editor, matching the display side's .mention-entity capsule.
F3 — P2 (test quality): Tests recreate production config instead of importing it
File: packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:15-45
The createMentionEditor() helper manually re-creates the TiptapMention.configure({ renderHTML, renderText }) block, copying the production logic from index.tsx:575-585. This is a hand-feed pattern: the test validates that the copied logic works, but if the production config drifts (e.g. someone changes the renderHTML in index.tsx without updating the test), the test still passes against stale logic.
Diff-scope: New test file. Not pre-existing.
Suggestion: If the renderHTML/renderText callbacks can be extracted to a shared helper (e.g. exported from mentionRender.ts alongside mentionNodeClass), the test can import and exercise the exact production callbacks. This would close the integration gap without requiring a full DOM test.
F4 — P2 (visual): Minor color shift on display side via token unification
File: packages/dmworkbase/src/ui/message/TextContent/index.css:44,51,63
| Selector | Before | After |
|---|---|---|
.mention-entity color |
#6B3DD8 |
var(--wk-color-accent) = #7f3bf5 |
.mention-highlight color |
rgba(127, 59, 245, 1) = #7F3BF5 |
var(--wk-color-accent) = #7f3bf5 |
.mention-fallback color |
#6B3DD8 |
var(--wk-color-accent) = #7f3bf5 |
.mention-entity and .mention-fallback shift from #6B3DD8 to #7f3bf5 (slightly brighter/lighter purple). .mention-highlight was already #7f3bf5 so no change there. The token unification is correct practice; the minor hue shift should be confirmed with design.
3. Data Flow Trace
| Consumer | Upstream Source | Flow Verified |
|---|---|---|
mentionNodeClass(uid, baseClass) return value → cls → span class attribute |
uid = node.attrs.id from Tiptap mention node; baseClass = options.HTMLAttributes.class ("mention") |
✅ uid flows from Tiptap node attrs; function ignores it (see F1) but returns valid class |
.wk-messageinput-editor .mention CSS → all mention spans in editor |
Tiptap renderHTML always emits class "mention" |
✅ All mentions (broadcast + normal) receive capsule styling |
Display-side .mention-entity / .mention-highlight CSS tokens |
--wk-color-accent, --wk-purple-alpha-08/12, --wk-r-xs, --wk-sp-0-5/2 |
✅ All defined in semantic.css:25 / primitive.css:20,64-66 |
renderText text serialization |
node.attrs.label ?? node.attrs.id |
✅ Defensive fallback from label to id; old renderLabel used node.attrs.label without fallback |
4. Blind Spot Checklist (R5)
| Check | Result | Evidence |
|---|---|---|
| C1 — Dual-path parity | CLEAR | Input side (.mention) and display side (.mention-entity) now share identical token values. Broadcast routing path (isBroadcastSentinelUid) unchanged — only visual rendering affected. |
| C2 — Control-flow ordering | CLEAR | renderHTML registered once in TiptapMention.configure(). No nested/duplicate registration. mentionNodeClass called once per render. |
| C3 — Auth boundary | N/A | No auth/permission changes. |
| C4 — Auth lifecycle | N/A | No auth changes. |
| C5 — Build/runtime gap | CLEAR | CSS custom properties verified in theme files (semantic.css, primitive.css). No build-only assumptions. |
| C6 — Governance/docs | N/A | No governance changes. |
5. Cross-round Blocker Review (R6)
N/A — first review pass on this PR.
6. Summary
This is a clean visual-unification PR. The core change — giving all @mentions in the editor the same capsule styling as the display side — is correctly implemented and well-rationed (prevents send-after visual jump). The renderLabel → renderHTML + renderText migration properly exits the Tiptap deprecated path. CSS token unification on the display side is a welcome cleanup.
All findings are P2 (maintainability, documentation, test quality). No P0/P1 — no path becomes broken, no user-visible incorrect data, no regression from the pre-PR state.
[Octo-Q] verdict: APPROVE + 理由: 所有 finding 均为 P2 级别（dead parameter、PR body 与实现不一致、测试 hand-feed 模式、微小色值偏移）。无 P0/P1 阻塞项——改动不破坏任何能工作的路径，不产生用户可见的错误数据，不比修复前更糟。视觉统一方案（输入框 + 展示侧共用设计 token）实现正确，CSS 变量在 theme 系统中有完整定义。建议处理 F1（去除/标记 unused uid 参数）和 F2（更新 PR body 以匹配实际实现）。
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: Re-review at head b831176. My prior blocker (hardcoded color/radius/spacing in the composer mention CSS) is fixed, and the two follow-up concerns raised on the previous head are resolved by tracing the actual message-display code. Approving.
Verified fixed
-
Design-token rule (my prior 🔴):
packages/dmworkbase/src/Components/MessageInput/index.cssmention rule now usesvar(--wk-color-accent),var(--wk-purple-alpha-08),var(--wk-r-xs),var(--wk-sp-0-5),var(--wk-sp-2), andvar(--wk-purple-alpha-12)(hover). No hardcodedrgba(107,61,216,...)/4px/2px 8pxresidue in the mention region.packages/dmworkbase/src/ui/message/TextContent/index.csswas also migrated to the same tokens (previously hardcoded#6B3DD8/rgba(107,61,216,0.08)). All referenced tokens resolve (semantic.css / primitive.css). -
Text-color / capsule consistency (composer vs message-display): composer
.wk-messageinput-editor .mentionand display.wk-msg-text-content .mention-entitynow share identical tokens —color: var(--wk-color-accent),background-color: var(--wk-purple-alpha-08), radius/padding from the same spacing tokens. Input and read side match. -
Broadcast vs normal split: the authoritative display mapping is
getMentionRenderState(uid)inpackages/dmworkbase/src/Messages/Text/mentionRenderState.ts(called fromMarkdownContent.tsx:475). It mapsuid==="all"(broadcast @所有人) tomention-entity(capsule),uid==="channel"tomention-highlight(plain, no background), any other uid tomention-entity(capsule). So on the display side, broadcast @所有人 already renders as a capsule identical to a normal member; only @频道 is plain. The composer therefore correctly unifies all mention nodes to the capsule viamentionNodeClass()returning the base class for every uid — this matches the read side and avoids a post-send visual jump. The earlier expectation that broadcast should stay plain does not match the display code foruid==="all".
Scope / safety
- Display-only: only
renderHTML/renderText(DOM rendering) changed;renderLabel→renderTextis the correct Tiptap 3 path. Send-side re-parse / sentinel serialization / mention eligibility are untouched. No public API / protocol / persisted-format change. - No dependency, lockfile, or workspace manifest changes — no build-config blast radius.
- Tests: the mention-style unit test was updated consistently with the unified-class behavior; not re-run locally in this fresh mirror (workspace deps not installed), but the change is mechanical and a peer approved this same head.
Non-blocking note
--wk-accent-tint-08/--wk-accent-tint-12exist in semantic.css with the same values as the--wk-purple-alpha-*primitives used here. Either is fine; using the same token as the display side keeps both sides literally identical, so no change required.
Superseded: re-reviewed at head b831176; blocker fixed, approving in newer review.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Re-review (rr2) + Verdict: #1157 — fix: 输入框普通成员 @mention 加浅紫胶囊背景 (#1153) @ b8311762
锚定 live head b8311762df95(rev-parse == live head)/ blocked(仅缺当前 head 票)。较我上轮 APPROVE(@ 1ba5c3d1)增量:input index.css/index.tsx、新 mentionNodeClass 助手、并把消息展示层 TextContent/index.css 一并 token 化。
结论:维持 APPROVE ✅ —— 本轮进一步统一了输入/展示两侧的 @mention 视觉,直接回应了 @yujiawei 的复审意见。
本轮变化 = 设计收敛(输入侧胶囊对齐展示层)
- 统一为胶囊:
mentionNodeClass(uid, baseClass)现对所有 uid 统一返回baseClass→ 输入框里广播(@所有人/@所有AI/legacy)与普通成员一致渲染为.mention胶囊(去掉了旧的.mention-user分支)。理由(注释):展示层getMentionRenderState中uid==="all"走.mention-entity胶囊,旧版输入把广播画成无背景纯文字 → 发送后跳变;统一后消除该不一致。 - input
.mention:--wk-color-accent+--wk-purple-alpha-08胶囊 + hover--wk-purple-alpha-12,cursor: text(编辑器内合理)。 - 展示层 token 化:
TextContent/index.css的.mention-entity/.mention-highlight/.mention-fallback由硬编码#6B3DD8/rgba(107,61,216,…)/4px/2px 8px全部改为--wk-*token(实测无硬编码残留)——把 token 规范也补到展示侧,与输入共用同一批变量(利于暗黑模式)。
一致性核验(输入能产出的 mention 全部与展示层匹配)
展示层 getMentionRenderState(本次未改):uid==="all"→胶囊、uid==="channel"→纯文字高亮、普通 uid→胶囊、无 uid→fallback。输入下拉(buildMentionDropdownItems)只产出普通成员 + @所有人/@所有AI(sentinel),不产出 channel mention。故输入统一胶囊 ↔ 展示层:普通成员✓、@all/广播✓;唯一「纯文字」的 channel 属展示层解析既有消息的概念、输入无法创建 → 无输入/展示不一致。
@yujiawei 复审各点均已闭环
- 「普通成员文字颜色未设置」→ 现
.mention统一color: var(--wk-color-accent)。✅ - 「广播提及实际是浅紫胶囊、输入侧不一致」→ 输入侧统一为胶囊、与展示层
.mention-entity对齐。✅ - 「颜色/背景统一为共享变量、利于暗黑」→ 输入 + 展示两侧现共用
--wk-*token。✅
安全/序列化(保持)
出站文本仍走 serializeMentionMarker(node.attrs…),与 renderHTML/renderText 无关;mention 仍为原子节点、纯 CSS 装饰;无依赖爆炸半径(未碰 lock/workspace/根 package.json)。
验证(实测 @ b8311762)
- 展示层 CSS 无硬编码残留(grep
#6B3DD8/107,61,216/4px/2px 8px→ NONE)✅。 - 构建 ✅
pnpm --filter @octo/web build→✓ built in 3.97s;frozen-install 干净。 - 测试 ✅ MessageInput +
mentionRenderState套件 8 文件 / 107 测试通过;更新后的mentionStyle.test断言普通成员与-1/-2/-3/all均渲染class="mention",并覆盖mentionNodeClass对所有 uid 返回 baseClass。 - 规范 CI 绿:
b8311762install-build / Build / e2e-p0 均 success。
🟡 非阻断
mentionNodeClass(uid, baseClass)现忽略uid、恒返回 baseClass —— 目前是「视觉统一」的显式接缝(prod 与 test 共用同一函数,较上轮改善)。若未来无重新分叉计划,可直接内联;若保留,值得一行注释说明它是有意的 no-op 分类器。
明确判断:维持 APPROVE(可合并)。 本轮把输入侧广播/普通成员统一为胶囊、并将展示层 CSS 一并 token 化,消除了发送后视觉跳变、闭环了 @yujiawei 的三点意见;输入可产出的所有 mention 与展示层类映射一致(channel 输入不可创建、无碰撞);构建通过、107 测试通过、规范 CI 全绿。感谢 @yujiawei 推动输入/展示一致性——现已收敛。
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1157 (octo-web)
Reviewed at head b8311762 against origin/main (merge-base). 5 files, +157/−17.
This is the fourth review round on this PR (heads 58d25333, 1ba5c3d1, 1ba5c3d1, now b8311762). Earlier rounds are not carried over — everything below was re-derived against this head. See the escalation note at the end.
1. Spec compliance
Spec: ❌
Missing — the member-vs-broadcast visual distinction #1153 explicitly asks for is not implemented
Issue #1153, Expected Behavior, second paragraph:
@所有人/@所有AI可以继续遵循现有三等级设计使用纯紫色文字；普通成员 mention 应与广播 mention 保持可识别的视觉区别。
The first clause is permissive (可以) about how broadcast is styled. The second is normative (应): member and broadcast mentions must stay visually distinguishable in the composer. #1153 also lists the absence of that distinction as part of the bug under Actual Behavior:
普通成员与
@所有人/@所有AI没有对应消息展示层的视觉等级差异。
At this head there is zero distinction in the composer:
packages/dmworkbase/src/Utils/mentionRender.ts:152-154—mentionNodeClass()returnsbaseClassfor everyuid, so every mention node getsclass="mention".packages/dmworkbase/src/Components/MessageInput/index.css:453-461— a single.mentionrule, no sibling/modifier rule.
Verified at runtime against the installed @tiptap/extension-mention@3.22.2: user123, -1, -2, -3 and all all produce byte-identical markup apart from data-id/data-label. Round 1 (58d25333) did implement this via a mention-user class; this commit removes it.
To be explicit about why this is genuinely unresolved rather than a preference: #1153's premise and the merged display layer contradict each other, and only a human can pick which wins.
packages/dmworkbase/src/Messages/Text/mentionRenderState.ts:12-14—uid === "all"→mention-entity, the same capsule as an ordinary member.packages/dmworkbase/src/Utils/mentionRender.ts:36-40— the prose says so: "Synthetic entries reuse theuid: "all"sentinel … while applying the same visual style as ordinary member mentions."packages/dmworkbase/src/Messages/Text/__tests__/MarkdownContentMention.test.tsx:68— a DOM regression test literally named "广播 mention 与普通成员 mention 同款高亮（视觉一致）" locks that behaviour (GH#295).mention-highlight(ui/message/TextContent/index.css:36-40) is the only visually distinct tier, and it is reachable only foruid === "channel"— which no mention builder produces (grep -rn '"channel"'overpackages/dmworkbase/srchits onlymentionRenderState.ts:15and its own test).
So the "现有三等级设计" #1153 refers to does not exist at the display layer for broadcast mentions. This commit is self-consistent with the display layer and inconsistent with the issue. That is a defensible engineering choice — it just is not a choice a PR can make silently, because #1153 says 应 and a test locks the opposite.
Ask: get this decided and recorded in #1153 before merge. Either (a) implement a composer-side distinction and say why the display layer keeps them identical, or (b) amend #1153 to strike the distinction requirement, citing mentionRenderState.ts:12-14 and the GH#295 test as the governing contract. Either is fine. Leaving both on the record contradicting each other is not, and it is what has kept this PR looping.
Divergence — the PR description describes a different implementation than the code
The description was not updated after the last two commits, and now asserts the opposite of what ships:
| Description claim | Code at b8311762 |
|---|---|
"@所有人 / @所有AI … 保持原有纯紫色文字+透明背景，不使用胶囊样式（三级广播态设计）" |
Broadcast mentions get the identical capsule (index.css:453-461, mentionRender.ts:152-154) |
"普通成员 span 追加 mention-user class" |
No mention-user class exists anywhere in the diff or the tree |
"根据 node.attrs.id 调用 isBroadcastSentinelUid() 判断" |
mentionNodeClass() never calls it; isBroadcastSentinelUid is imported only by the test |
| "改动文件" lists 3 files | 5 files — Utils/mentionRender.ts and ui/message/TextContent/index.css are undisclosed |
"grep -n "background-color: rgba(107, 61, 216, 0.08)" → L463 命中" |
That literal no longer exists in the repo |
| "不涉及…纯视觉修复" (Comprehension gate) | True, but it omits that the change is no longer composer-only — see P2-1 |
This is not a cosmetic nit on a 4-round PR: the description is the durable record of what was decided and why, and right now it documents the round-1 design that was deliberately reverted. Please rewrite it to match b8311762.
Over-built
None that I would hold against the change. The two files outside the composer (Utils/mentionRender.ts, ui/message/TextContent/index.css) go beyond #1153's "建议修复边界保持在编辑器视觉层", but both were explicitly requested by earlier review rounds, so they are sanctioned scope — they just need disclosing in the description (above) and calling out as a user-visible change (P2-1).
2. Code quality
Quality: Approved — no P0/P1. Everything below is advisory.
P2-1 — Token migration silently changes the rendered colour of every existing message
packages/dmworkbase/src/ui/message/TextContent/index.css:43-55, :58-60
The substitutions are not value-preserving:
| property | before | after (resolved) |
|---|---|---|
.mention-entity color |
#6B3DD8 |
var(--wk-color-accent) → --wk-purple-500 → #7f3bf5 |
.mention-entity background-color |
rgba(107, 61, 216, 0.08) |
var(--wk-purple-alpha-08) → rgba(127, 59, 245, 0.08) |
.mention-entity border-radius |
4px |
var(--wk-r-xs) → 3px |
.mention-fallback color |
#6B3DD8 |
#7f3bf5 |
.mention-highlight color |
rgba(127, 59, 245, 1) |
#7f3bf5 — no-op ✅ |
Resolution chain: theme/semantic.css:25 → theme/primitive.css:20 (--wk-purple-500: #7f3bf5), theme/primitive.css:64 (--wk-purple-alpha-08), theme/semantic.css:206 (--wk-r-xs: 3px). Tokens do reach both files in production — apps/web/src/index.tsx:3 imports @octo/base/src/theme/tokens.css, which forwards to theme/index.css → primitive.css + semantic.css. So no risk of an unresolved var().
I think #7f3bf5 is the right purple (it is the token-sanctioned one; #6B3DD8 was the outlier), and dark-mode legibility improves because #7f3bf5 is lighter and --wk-color-accent is in semantic.css's "全局固定 token（不跟主题走）" block with no dark override. The point is that a P3 composer-only UX ticket now repaints every mention in every historical message, and neither the description nor any screenshot records it. No test breaks — the mention tests assert class names only (mentionRenderState.test.ts, MarkdownContentMention.test.tsx), never computed values.
P2-2 — mentionNodeClass is an identity function, and the whole custom renderHTML is now a no-op reimplementation
packages/dmworkbase/src/Utils/mentionRender.ts:146-155, packages/dmworkbase/src/Components/MessageInput/index.tsx:575-587
mentionNodeClass(uid, baseClass) ignores uid entirely. That makes cls === baseClass === options.HTMLAttributes.class, so line 581's { ...options.HTMLAttributes, class: cls } is just { ...options.HTMLAttributes }.
I verified empirically against the installed @tiptap/extension-mention@3.22.2 (harness driving a real Editor over uids user123 / -1 / -2 / -3 / all / null):
- This PR's
renderHTMLvs the extension's default renderer: identical attribute set and identical text; the only difference is attribute serialisation order (data-typefirst vsclassfirst). Semantically equivalent. - This PR's
renderHTMLvs the pre-PRrenderLabel: byte-identicalgetHTML()for every uid tested.
So the .tsx change produces the same DOM as before the PR and the same DOM as writing nothing at all. Deleting renderHTML, renderText, mentionNodeClass, the uid/baseClass locals and the import would leave behaviour unchanged. The two real gains from touching this file are worth keeping and are achieved by simply removing renderLabel:
- It silences
console.warn("renderLabel is deprecated…"), which fired on every mention render (extension-mention/dist/index.js, thethis.options.renderLabel !== void 0branch). - It fixes a label fallback: with a mention node carrying
idbut nolabel, the oldrenderLabelrendered@null; the default (and this PR) render@user123. Confirmed at runtime.
Suggestion: drop mentionNodeClass and both hooks; keep the renderLabel deletion. If you prefer to keep an explicit hook for future divergence, at least drop the unused uid parameter so the signature stops advertising a decision it does not make. (packages/tsconfig/base.json:15 sets noUnusedParameters: false and .eslintrc.js is fully commented out, so nothing flags it automatically.)
P2-3 — The new 113-line test locks nothing about this change
packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx
Two independent problems:
- It re-implements the production config instead of importing it (
:22-40duplicatesindex.tsx:575-587verbatim). DeletingrenderHTMLfrom production, or reintroducingrenderLabel, leaves the suite green. - Its assertions pass against pre-PR code. I ran the exact assertion set from
:51-107(data-type="mention",class="mention",data-id,data-label,@label) against arenderLabel-configured editor: all five cases pass. So the file has zero regression-detection power for the.tsxchange it accompanies. :102-107is tautological —expect(mentionNodeClass("-2", "mention")).toBe("mention")asserts that an identity function is an identity function, for five inputs.
If P2-2 is taken and the hooks are deleted, this file should go with them. If the hooks stay, the test needs to import the real extension config (export the TiptapMention.configure({...}) object from index.tsx or a sibling module) so it exercises production rather than a copy.
P2-4 — padding: 2px 8px now applies to every composer mention, inside a 20px line box
packages/dmworkbase/src/Components/MessageInput/index.css:457 vs :434-444
The editor is font-size: 14px; line-height: 20px; max-height: 100px; overflow-y: auto. var(--wk-sp-0-5) var(--wk-sp-2) resolves to 2px 8px, so an inline mention's background box is roughly 2 + ~17 + 2 ≈ 21px — taller than the 20px line box. On wrapped input, capsules on adjacent lines will touch or overlap, and at the max-height: 100px cap the first/last row's capsule can clip against the scroll edge. Previously invisible because .mention had background-color: transparent; now visible, and now also on broadcast mentions. The display side absorbs the same padding at line-height: 22px (ui/message/TextContent/index.css:8). Horizontal padding also went 2px → 8px, so mentions get noticeably wider in the composer.
Worth one look in a real browser with a two-line message containing two mentions. cursor: text at :459 is a good fix for the earlier round's complaint that cursor: pointer advertised a click the composer does not handle (the Tiptap mention node is selectable: false, atom: true).
P2-5 (nit) — primitive-layer tokens used directly in component CSS
index.css:455,464, TextContent/index.css:45,54 use --wk-purple-alpha-08 / -12, defined in theme/primitive.css:64,66. theme/semantic.css:5 states "组件里只用这一层，禁止直接引用 primitive". There is plenty of precedent (CategoryEmptyState/index.css:14, CategoryHeader/index.css:32,53, WKInput/index.css:19, InputEdit/index.css:22), so this is convention drift rather than a defect — but a semantic alias (--wk-mention-bg / --wk-mention-bg-hover) would give the mention capsule one name across both layers and a hook for a future dark-mode override.
3. Re-verified as correct at this head
renderLabel→renderHTML+renderTextis behaviour-preserving. Against@tiptap/extension-mention@3.22.2,options.HTMLAttributeshanded to the hook is the merged record — the node-levelrenderHTMLdoesmergedOptions.HTMLAttributes = mergeAttributes({"data-type": this.name}, this.options.HTMLAttributes, HTMLAttributes)before calling it. Sodata-type,data-id,data-labelanddata-mention-suggestion-charall survive the spread atindex.tsx:581; I confirmed this on real output for all five uids.setContent(getHTML())round-trips to an identical JSON document, andgetText()is byte-identical to the pre-PR output.- Send payload untouched.
extractMentionsFromEditorwalkseditor.getJSON(), notgetHTML(), so serialisation is unaffected by a render-hook change. - Sentinel handling unchanged.
isBroadcastSentinelUid(mentionRender.ts:137-144) is not called on any production path in this diff, so the paste guard / send re-parse safeguards are untouched. - Tokens resolve in production (chain traced in P2-1).
- No test regressions from the CSS change — no test asserts computed styles or hex values anywhere in the tree.
4. Overall verdict
CHANGES_REQUESTED
The user-visible defect in #1153 is genuinely fixed and the token migration is the repo-idiomatic way to do it. What blocks is the record, not the CSS:
- Rewrite the PR description to match
b8311762— it currently documents the round-1 design that this commit reverted, lists 3 of 5 files, and its verification greps no longer match. - Resolve the member-vs-broadcast distinction in #1153 one way and write it down: implement it, or strike it from the issue citing
mentionRenderState.ts:12-14+ the GH#295 test as the governing contract.
Recommended in the same pass: P2-2 (delete the no-op renderHTML/mentionNodeClass, keep the renderLabel removal) and P2-3 (drop or rewire the test), since together they remove ~120 lines that assert nothing.
Escalation
This is round 4, and rounds 2–4 have each been spent re-litigating the same question — should composer broadcast mentions look like member mentions — with the issue text and the display code pointing opposite ways. Please stop iterating on CSS and settle the intended mention visual contract for composer and bubble together in #1153 first, with a maintainer's decision on the record. One screenshot (composer beside the sent bubble, light and dark, a two-line message with two mentions) would close the spec question, P2-1 and P2-4 in one shot and is the single most useful thing to attach — #1153 is a purely visual bug and no round has produced a rendered image yet.
5. Coverage / not verified
- No browser render. No screenshot on the PR after four rounds. The exact rendered purple, the
#6B3DD8→#7f3bf5delta on historical messages, P2-4's line-box overlap and scroll clipping at the 100px cap, and dark-mode appearance all still need a real render. - Vitest not executed (no
node_modulesin the review checkout). The "7 files / 103 tests passed" claim is taken at face value. Tiptap semantics were instead verified by driving a realEditoragainst the installed3.22.2in a standalone harness — that is stronger evidence than the suite for the specific claims above, but it is not the suite. - Dark mode traced in CSS only (
body[theme-mode=dark], no--wk-color-accentoverride) — not exercised. Contrast of#7f3bf5on the dark surface behind an 8%-alpha wash was not measured. - Token resolution by static chain-tracing plus the import graph, not by inspecting a built CSS bundle.
- Not exercised: IME composition adjacent to a padded capsule;
labelvalues containing markup; clipboard copy of a composer mention into a third-party target. - Sibling PRs / post-merge compile not checked beyond
mergeable: MERGEABLE.
) - 移除冗余 renderHTML/mentionNodeClass：Tiptap 默认渲染已正确输出 class/data-*，自定义 renderHTML 与默认行为等价，删除后代码更简洁，同时保留 renderText 以消除 deprecation warn - 测试改写为断言 CSS 文件中 .mention 胶囊规则真实存在（读取 CSS 源码 + regex），DOM 测试使用与生产一致的最小 TiptapMention 配置（仅 renderText），不再复制生产 render 逻辑 - 输入框 .mention padding 垂直方向由 2px 收紧为 1px，避免 20px 行高内胶囊背景溢出相邻行；vertical-align: baseline 保证基线对齐
a9258d4
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Re-review (rr3) + Verdict: #1157 — fix: 输入框 @mention 胶囊背景 (#1153) @ a9258d4e
锚定 live head a9258d4e7493(rev-parse == live head)/ blocked。较我上轮 APPROVE(@ b8311762)线性增量,4 文件 +39/−60:移除自定义 renderHTML 与 mentionNodeClass(改用 tiptap 默认渲染 + 保留 renderText)、CSS 微调 padding/vertical-align。
结论:REQUEST_CHANGES 🔴 —— 代码本身正确(见下,我实测 CI 绿 / 104 测试过),唯一阻塞项:PR 描述与实现完全相反,须更正后再合(@yujiawei 的描述失真 CR 属实,本轮更甚)。
代码质量 ✅(本轮 = 简化,行为不变)
- 删除自定义
renderHTML:tiptap 默认渲染器已用配置的HTMLAttributes.class="mention"+renderText产出@label,输出一致(class=mention + data-id/data-label + @Label)。删掉的mentionNodeClass(上轮我提的忽略 uid 的赘余助手)一并清理——回应了我上轮 🟡。 - 纯展示、序列化未动(出站仍
serializeMentionMarker(node.attrs…));无依赖爆炸半径。 - 测试更扎实:
mentionStyle.test由源码子串断言改为 CSS 规则断言 + 默认渲染器 DOM 断言(普通成员与-1/-2/-3/all均class=mention)。 - 实测:构建
✓ 3.85s;MessageInput 套件 7 文件 / 104 通过;规范 CI(install-build / Build / e2e-p0)全 success。
🔴 阻塞:PR 描述与实现相反(声明≠实现)
当前描述仍写:
- 「@所有人/@所有AI 保持纯紫色文字+透明背景,不使用胶囊(三级广播态设计)」
- 「新增
renderHTML……普通成员追加mention-userclass…….mentionbaseline 保留透明背景」
而实现已相反:所有 mention(含广播)统一为 .mention 胶囊;renderHTML / mention-user / mentionNodeClass 已删除;.mention baseline 现为胶囊(非透明)。→ 描述描述的是已删掉的旧方案、并断言了与代码相反的广播视觉,会误导 maintainer「以为在合入区分态」。这与我对 #1146「描述声明的筛选未实现」同类,按同一标准记为阻塞。
- 修(二选一):① 更新描述为「输入侧所有 @mention 统一胶囊,对齐展示层
.mention-entity;广播不再单独透明」(纯文档改,推荐);或 ② 若团队确定要区分,则改代码实现区分态(并同步展示层,见下)。
关于「成员/广播是否应视觉区分」(设计判断,附证据)
@yujiawei 主张按 #1153 区分(广播纯文字)。但展示层权威映射 getMentionRenderState(uid)(mentionRenderState.ts)对 uid==="all"(@所有人)本就返回 .mention-entity胶囊,仅 uid==="channel"(输入不可创建)才纯文字。故「广播应无背景纯文字」的前提与实际展示码不符;输入统一胶囊恰好消除发送后跳变、与展示一致,技术上自洽正确。若仍要区分,应从展示层 + #1153 一并推进,而非仅动输入侧。倾向采用「方案①更新描述」以匹配这一正确实现。
明确判断:REQUEST_CHANGES —— 仅因 PR 描述失真(声明广播透明/区分,实际统一胶囊、且相关代码已删)须更正;代码实现本身正确、简化合理、测试更强、CI 全绿、与展示层一致。 更新描述(推荐)或经产品决策改为真正区分态(需带上展示层),即可放行。成员/广播区分与否属产品决策,当前统一方案技术自洽。
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-review — head a9258d4e7493
Re-reviewed the delta b8311762..a9258d4e7493 against main (byte-verified in a local mirror). Verdict: APPROVE.
What the delta changed
packages/dmworkbase/src/Components/MessageInput/index.tsx— removed the per-uidrenderHTMLbranch and thementionNodeClassimport; the composer now uses Tiptap's default renderer, which appliesclass="mention"to every mention node.renderTextis retained for the text-serialization path.packages/dmworkbase/src/Utils/mentionRender.ts— removed the now-unusedmentionNodeClass()helper (it returnedbaseClassfor all uids anyway; the default renderer makes it redundant).packages/dmworkbase/src/Components/MessageInput/index.css— only padding tweak (var(--wk-sp-0-5)→1px) plusvertical-align: baselineon.wk-messageinput-editor .mention. Still the lavender capsule viavar(--wk-purple-alpha-08)for all mentions.- test file rewritten to assert the CSS capsule rule and that all sentinel uids render
class="mention".
Net effect on rendering is unchanged from b8311762: every composer mention (normal member and broadcast @所有人/@所有AI) renders as the same lavender capsule.
Composer ↔ display consistency (the load-bearing check)
This is the criterion for a real post-send visual jump, and it holds:
- Display side
packages/dmworkbase/src/Messages/Text/mentionRenderState.ts—getMentionRenderState("all")→mention-entity(capsule); ordinary uid →mention-entity(capsule); only"channel"→mention-highlight(plain). Unchanged by this PR (byte-verified: not in the merge-base..head delta). - Composer dropdown can only produce ordinary members and broadcast sentinels (
-1/-2/-3/all), never@channel. - Therefore every creatable mention is a capsule on both sides → no post-send styling jump.
On the differentiation request
There is a genuine tension: #1153's Expected Behavior text asks broadcast mentions to stay plain purple text and differ from ordinary members. However, the already-merged display layer does not honor that — getMentionRenderState("all") renders broadcast as the same mention-entity capsule as a normal member (see also the display-side regression test asserting broadcast and normal member share the same highlight). Making the composer render broadcast as plain to satisfy the spec text would make the composer diverge from the display layer and reintroduce a post-send jump — trading a spec-literal match for a real visual regression. Whether the display layer's unified treatment should itself change to differentiate broadcast is a product decision for #1153, not a defect in this composer-only fix. This PR correctly optimizes for composer/display consistency.
Other gates
- Token rule: fixed, no hardcoded-color residue in the touched CSS; both composer and display now share
--wk-purple-alpha-08/--wk-purple-alpha-12/--wk-r-xs. - Scope: display-only; no serialization or mention-eligibility change (
renderTextpath unchanged for send). - Tests: mention style test updated and coherent with behavior;
Build/install-build/e2e-p0CI checks pass. - No dependency hazard: the delta touches no
package.json/pnpm-lock/pnpm-workspace.
Note
The PR description still narrates an earlier mention-user-split approach (broadcast plain); the actual code at this head keeps the unified-capsule design. Recommend updating the PR body to match the shipped implementation to avoid future confusion.
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review — PR #1157 (octo-web)
Summary
This PR adds a light purple capsule background to regular member @-mentions in the message input editor, visually distinguishing them from broadcast mentions (@所有人, @所有AI) which retain the plain purple text style. The implementation replaces the old renderLabel callback with renderHTML + renderText on the Tiptap Mention extension, giving full DOM control to conditionally apply a mention-user CSS class. The editor styling is aligned pixel-for-pixel with the message display layer's .mention-entity class. A new test file validates the broadcast-vs-user class distinction. Overall, a clean, well-scoped cosmetic change with appropriate test coverage.
Verification
Static analysis only at head 58d25333; build and tests not executed in this environment.
- ✅ Broadcast sentinel classification —
isBroadcastSentinelUid()atpackages/dmworkbase/src/Utils/mentionRender.ts:137covers all four sentinels (-1,-2,-3,all). TherenderHTMLoverride atpackages/dmworkbase/src/Components/MessageInput/index.tsx:577convertsnode.attrs.idviaString(...)and routes correctly: broadcast UIDs get bare.mentionclass, real user UIDs get.mention.mention-user. - ✅ CSS specificity chain —
.mention.mention-user(specificity 0,2,0) correctly overrides the base.mention(0,1,0) forbackground-color,border-radius,padding, andfont-weight. Broadcast mentions (class="mention") never match.mention-userso they retain transparent background. - ✅ Editor↔display parity — The new
.mention.mention-uservalues (background-color: rgba(107, 61, 216, 0.08),border-radius: 4px,padding: 2px 8px,font-weight: 500,cursor: pointer,transition: background-color 0.2s, hoverrgba(107, 61, 216, 0.12)) exactly match the message display layer.mention-entityatpackages/dmworkbase/src/ui/message/TextContent/index.css:43-55. - ✅ Tiptap API contract —
renderHTMLis a valid Tiptap Mention configure option that overrides the default DOM output. Theoptions.HTMLAttributesspread preserves the baseclass: "mention", and ProseMirror'sDOMSerializeraddsdata-type,data-id,data-labelfrom the NodeSpectoDOMdefinition (confirmed by test assertions atpackages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:63-67). - ✅ Label fallback — The new
node.attrs.label ?? node.attrs.idfallback in bothrenderHTMLandrenderTextis a defensive improvement over the oldrenderLabelwhich usednode.attrs.labelwithout fallback.
Findings
No P0/P1 issues. One minor maintainability note below.
Nit — Duplicated color literals without shared CSS variable (packages/dmworkbase/src/Components/MessageInput/index.css:463)
The rgba(107, 61, 216, 0.08) / rgba(107, 61, 216, 0.12) values are duplicated from packages/dmworkbase/src/ui/message/TextContent/index.css:45,54. If the design system adjusts the mention capsule shade, both files need updating. Not a change I'd block on — this matches the existing codebase pattern where neither location uses a CSS variable — but extracting a shared --wk-mention-bg / --wk-mention-bg-hover variable would reduce future drift risk.
Things I checked that are fine
- Test file creates real Tiptap
Editorinstances with the samerenderHTML/renderTextconfig as production, exercising the true code path rather than hand-fed mock output. renderTextfallback tonode.attrs.idwhenlabelis missing prevents@undefinedin clipboard/serialization output.- The
transition: background-color 0.2sis scoped tobackground-coloronly — no unintended animation of other properties. - No distributed side effects (notification/push/counting) are affected — this is a purely cosmetic editor-layer change.
Verdict: APPROVED
Clean, well-scoped cosmetic enhancement that correctly distinguishes broadcast and user mentions at the DOM level, with matching editor↔display styling and proper test coverage. No correctness, security, or functional concerns.
dolphinsboy
changed the title
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1157 (octo-web)
Reviewed at head a9258d4e749355ea2b2158c159812e79be445242, merge-base 2d087f0b. 4 files, +136/−17.
Verified locally at this exact SHA: pnpm install --frozen-lockfile → OK; pnpm --filter @octo/web build → ✓ built in 6.97s; vitest run src/Components/MessageInput/__tests__/ → 7 files / 104 tests passed; vitest run src/Messages/Text/__tests__/ → 4 files / 24 tests passed.
The code is in good shape. The blocking problem is that the PR record no longer describes the code, and one of the four changed files silently restyles a surface the linked issue asked us to stay out of. I am not asking for the CSS or the TS to be reverted — see "What I'm asking for" at the end.
1. Spec compliance
Spec: ❌
Missing (漏建) — none
The composer capsule is implemented and now genuinely matches the bubble. .wk-messageinput-editor .mention (packages/dmworkbase/src/Components/MessageInput/index.css:453-462) and .wk-msg-text-content .mention-entity (packages/dmworkbase/src/ui/message/TextContent/index.css:43-51) resolve to the same color, background-color, border-radius and font-weight. I confirmed both rules and all six token definitions land in the same built bundle graph (apps/web/build/assets/App-*.css carries the rules; index-*.css carries --wk-color-accent / --wk-purple-alpha-08 / --wk-r-xs), so this is not a silently-unresolved var(). Token values: semantic.css:25 → primitive.css:20 = #7f3bf5; primitive.css:64,66; semantic.css:193,196,206 = 2px / 8px / 3px.
The only intentional delta is vertical padding (composer 1px, bubble var(--wk-sp-0-5) = 2px), which commit a9258d4e explains as avoiding capsule bleed inside the editor's 20px line-height. That reasoning is sound.
Over-built (超建) — TextContent/index.css is outside the issue's stated fix boundary, and it is not a pure refactor
Issue #1153 recommends keeping the fix in the editor visual layer. The PR also changes the message display layer, and that change is not value-preserving:
| property | before | after | delta |
|---|---|---|---|
.mention-entity color |
#6B3DD8 |
var(--wk-color-accent) = #7f3bf5 |
hue shift |
.mention-entity background-color |
rgba(107, 61, 216, .08) |
var(--wk-purple-alpha-08) = rgba(127, 59, 245, .08) |
hue shift |
.mention-entity border-radius |
4px |
var(--wk-r-xs) = 3px |
−1px, real visual change |
.mention-entity padding |
2px 8px |
var(--wk-sp-0-5) var(--wk-sp-2) |
no-op (2px 8px) |
.mention-highlight / .mention-fallback color |
rgba(127,59,245,1) / #6B3DD8 |
var(--wk-color-accent) |
no-op / hue shift |
This repaints every regular-member and broadcast mention in every message in history (Messages/Text/mentionRenderState.ts:12-19 routes both all and ordinary uids to .mention-entity). The PR body does not mention this file at all.
To be fair: this is a reasonable change and it was forced by the token rule in DEVELOPMENT.md:127-142 that an earlier review correctly raised — once the composer moved to --wk-purple-*, leaving the bubble on ad-hoc 107,61,216 would have produced two coexisting purple families. I also cleared the obvious risk myself: contrast on white goes 6.39:1 → 5.46:1, both above WCAG AA 4.5:1, so there is no accessibility regression, and a repo-wide search found no snapshot or style assertion pinned to the old literals. The objection is disclosure, not direction — an undisclosed restyle of the primary message surface should not ride along inside a P3 composer ticket without a word in the description or a before/after screenshot.
Divergence (偏离) — the implementation inverts the issue's stated broadcast behaviour
#1153's Expected Behavior says @所有人 / @所有AI should keep plain purple text and stay visually distinct from regular members. This PR does the opposite: the single .mention rule at index.css:453 gives every composer mention the capsule, and the code comment at index.css:452 states the unification outright.
I am ruling in favour of the code and against the issue text. The issue's premise is wrong: it inferred broadcast behaviour from .mention-highlight, but getMentionRenderState() (Messages/Text/mentionRenderState.ts:12-19) only routes uid "channel" there — uid "all" gets .mention-entity, i.e. the capsule. That is locked by an explicit regression test whose own title is "广播 mention 与普通成员 mention 同款高亮（视觉一致）" (Messages/Text/__tests__/MarkdownContentMention.test.tsx:56-81, GH#295). Keeping broadcast transparent in the composer would have reproduced, for broadcast mentions, the exact send-time style jump #1153 was filed about.
So: do not revert this. But the ticket and the code currently assert opposite acceptance criteria, and that has to be reconciled before merge rather than after — otherwise the merge record permanently disagrees with the requirement it claims to close.
The PR description materially misrepresents this head
This is what tips the spec gate. The body still describes an earlier, abandoned design:
- Claims a
renderHTMLthat adds amention-userclass for regular members — norenderHTMLand nomention-userclass exist at this head. - Claims
@所有人/@所有AI"保持原有纯紫色文字+透明背景，不使用胶囊样式" — the code gives them the capsule. - Claims
.wk-messageinput-editor .mentionkeeps a transparent baseline background — it does not. - Claims an
isBroadcastSentinelUidimport inindex.tsx— there is none (only the test file imports it). - Says "改动文件" = 3 files — it is 4;
ui/message/TextContent/index.cssis unlisted. - "验收证据" is falsified:
grep "background-color: rgba(107, 61, 216, 0.08)"no longer matches anywhere in the tree, andgit diff --name-only origin/main...HEADreturns 4 paths, not 3. - The quoted verification command is not runnable as written —
pnpm --filter @octo/base exec vitest run packages/dmworkbase/src/Components/MessageInput/__tests__/resolves relative to the package root and exits 1 with "No test files found"; the working form issrc/Components/MessageInput/__tests__/. Actual count is 104 tests, not 103.
The individual commit messages (a9258d4e especially) are accurate and well-written — only the PR body was never updated. That matters because the PR body is what survives into the squash-merge record, and because earlier approvals on this PR reasoned explicitly from claims in it that are no longer true.
2. Code quality
Quality: Approved (no P0/P1; P2s below)
No correctness, security or data-loss defect found. Specifically cleared:
- Send path unaffected. Outbound text is built by
extractMentionsFromEditorwalkingeditor.getJSON()and callingserializeMentionMarker(node.attrs.id, node.attrs.label, trusted)(MessageInput/index.tsx:371-381). It never readsgetHTML()/renderText, so mention markers, theMENTION_TRUST_MARKbroadcast-trust scheme (Utils/mentionRender.ts:150-153) and draft round-trip are all untouched. renderLabel→renderTextis correct, and is a small improvement. In@tiptap/extension-mention@3.22.2therenderLabeloption is deprecated-but-honoured:dist/index.js:145-150and:203-205short-circuitoptions.renderHTML/options.renderTextand emit aconsole.warnon every render. Removing it stops that warning spam.renderTextis a declaredMentionOptionsmember (dist/index.d.ts:47) consumed atdist/index.js:207, so this is a supported API, not a misuse — build and 128 tests confirm it.- No selector bleed.
class: "mention"has exactly one emitter (index.tsx:543); the suggestion dropdown uses.mention-list-itemand is portaled todocument.body(mentionSuggestion.tsx:68), so it cannot inherit the capsule. - No line-height regression. Padding on an inline non-replaced element does not contribute to line-box height, so the 20px editor rows (
index.css:437) do not grow. The only effect is a 1px background bleed past the line box on wrapped lines — which is exactly whata9258d4etightened 2px→1px to mitigate.
P2-1 — hardcoded 1px violates the token rule that already blocked this PR once
packages/dmworkbase/src/Components/MessageInput/index.css:457
padding: 1px var(--wk-sp-2);DEVELOPMENT.md:127-142 lists raw padding: 8px as ❌. The spacing scale starts at --wk-sp-0-5: 2px (theme/semantic.css:193), so there is no 1px token — but that is an argument for adding one (--wk-sp-px), not for a literal. Note stylelint.config.mjs only lints hex/named colours and rgb()/hsl(), so CI will not catch this.
P2-2 — the new test file mostly exercises Tiptap and a hand-copied config, not production code
packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:16-38
createMentionEditor() re-declares the mention config instead of importing the one at index.tsx:541-577. The DOM assertions (:68, :85, :96) then verify that Tiptap emits class="mention" and data-* — for a class the test itself supplied. If production drifts, these stay green.
P2-3 — the CSS assertions can be green while the UI is broken, and red while it is fine
mentionStyle.test.tsx:43-59
Regex-matching index.css off disk with /\.wk-messageinput-editor \.mention\s*{([^}]*)}/ asserts text presence, not applied style: it passes even if the token is undefined at runtime or the rule is overridden, and breaks on harmless reformatting, selector grouping, or a comment inside the block. A computed-style assertion, or a small PostCSS parse, would be worth more than both blocks combined.
P2-4 — contradictory affordance in the composer
index.css:461, 464-466 sets cursor: text but also a :hover background change. Hover feedback signals interactivity that the composer mention does not have. Harmless, but the bubble's cursor: pointer + hover pairing is the coherent one; consider dropping the composer :hover.
P2-5 — dormant dark-mode gap
theme/semantic.css:25 fixes --wk-color-accent to --wk-purple-500 with no body[theme-mode=dark] override, while the sibling --wk-text-accent switches to the lighter --wk-purple-400 (semantic.css:395). Dark mode is force-disabled today (App.tsx:827 → ThemeMode.light), so this is dormant, not live — but both mention surfaces will need a dark-mode accent when it is re-enabled.
3. Overall verdict
CHANGES_REQUESTED — Spec ❌ (over-build + divergence + a description that no longer matches the code) AND-gated with Quality: Approved.
4. What I'm asking for
Nothing here requires reworking the CSS or the TypeScript. In rough order of importance:
- Rewrite the PR body to describe this head. Drop the
renderHTML/mention-user/ "broadcast keeps transparent background" narrative, list all 4 files, and replace the "验收证据" block with commands that actually run (vitest run src/Components/MessageInput/__tests__/, 104 tests). - Disclose and justify the
TextContent/index.csschange in the body: state that regular and broadcast mentions in existing messages shift#6B3DD8→#7f3bf5and4px→3pxradius, and attach a before/after screenshot of a message bubble. If you would rather keep this PR strictly composer-scoped, split it into its own token-migration PR instead — either resolution is fine, silence is not. - Reconcile #1153. Amend its Expected Behavior to record that broadcast mentions already render as
.mention-entityin the display layer (citementionRenderState.ts:12-19and the GH#295 test), so the ticket and the shipped behaviour stop contradicting each other. Please do not reintroduce a broadcast/member split in the composer. - Optional, take or leave: P2-1 (add a
--wk-sp-pxtoken), P2-3 (assert computed style instead of regex), P2-4 (drop the composer:hover).
5. Additional notes
- Process, not code — this is the fifth head on a P3 CSS fix (
2f876709→58d25333→1ba5c3d1→b8311762→a9258d4e), with several full review cycles behind it, and the substantive design has changed twice mid-flight (mention-usersplit → unified capsule). The remaining items above are documentation and ticket hygiene, so I would suggest landing this on a body edit plus a screenshot rather than opening another full review round. If it does come back a sixth time, that is a signal to step back and agree the target design in the issue first rather than iterating on the diff. - Verification gap nobody has closed, including me: no reviewer has driven this in a real browser. Caret/selection geometry around a padded inline atom, capsule bleed on wrapped multi-line input, IME composition adjacent to a capsule, and RTL layout are all unexercised, and the PR carries no screenshot. For a change whose entire payload is visual, one before/after image of the composer and one of a message bubble would be worth more than the current test file.
packages/docsalso depends on@tiptap/extension-mention@3.22.2and has its own mention surfaces (packages/docs/src/sheet/sheetMention.ts,SheetMentionOverlay.tsx). Out of scope here and untouched — just noting it as the other place this styling question will eventually surface.
- 删除输入框 .mention :hover 态（cursor:text 不应有 hover 反馈，避免虚假交互暗示） - 新增 --wk-sp-px: 1px 设计 token，替换硬编码 1px padding - 更新 mentionStyle 测试：断言 padding 使用 var(--wk-sp-px)，且无 :hover 规则
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Re-review (rr4) + Verdict: #1157 — fix: 输入框 @mention 胶囊背景，展示层对齐 token (#1153) @ fdac5b7d
锚定 live head fdac5b7d999c(rev-parse == live head)/ blocked。较我上轮 CR(@ a9258d4e)线性增量:index.css −7(去 :hover、1px→--wk-sp-px)、semantic.css +1(新 --wk-sp-px:1px)、test −10。
结论:flip CR → APPROVE ✅ —— 我上轮唯一的阻塞项(描述与实现相反)已彻底修复:PR 描述已重写、准确,并记录了 maintainer 对规格分歧的决策。
🔴→✅ 我上轮的阻塞项(描述失真)已解除
新描述准确:
- 明确写「输入框与展示层一致,广播与普通成员均使用胶囊」——与代码(统一
.mention胶囊、无mention-user/renderHTML)一致。 - 决策说明直面 @yujiawei 的规格争议:「Issue 原文要求 @所有人/@所有AI 纯紫无胶囊,但展示层
mentionRenderState.ts:12-19对uid==="all"返回.mention-entity(同款胶囊)、GH#295 测试锁定了视觉一致,经 maintainer 确认采用统一方案」。→ 规格分歧由产品/maintainer 拍板记录在案,不再是「静默偏离」。 - 变更摘要如实列出输入侧统一胶囊、展示层 token 迁移、
--wk-sp-px新增、renderLabel→renderText(顺带修@null与 deprecation)。文件清单准确。
本轮代码 ✅
- 去掉
.mention:hover(cursor:text 不响应 hover、避免虚假交互暗示)——合理。 - 最后一处字面量
padding:1px→var(--wk-sp-px),并在 semantic.css 定义--wk-sp-px:1px。mention 及展示层.mention-entity规则现无硬编码残留(index.css 里其余1px/4px/14px均为本 PR 未触碰的既有工具栏/编辑器规则)。
关于 @yujiawei 持续的三点(现已闭环/记录)
- 「广播应视觉区分」→ 展示层权威(
getMentionRenderState("all")=胶囊)与之矛盾,maintainer 已确认统一方案,属产品决策、非代码缺陷。 - 「改了展示层、超出范围」→ 描述已说明必要性(输入侧改 token 后展示层若留旧硬编码会两版紫并存 + DEVELOPMENT.md 禁硬编码);属合理且 token 合规的连带迁移,已文档化。
- 「PR 记录未描述变更」→ 描述已完整重写,本条解除。
验证(实测 @ fdac5b7d)
- 构建 ✅
pnpm --filter @octo/web build→✓ built in 3.94s;frozen-install 干净。 - 测试 ✅ MessageInput 套件 7 文件 / 104 通过。
- 规范 CI:Build / install-build success,e2e-p0 in_progress(前序 head 均绿)。
- 纯展示、序列化未动;无依赖爆炸半径(未碰 lock/workspace/根 package.json)。
明确判断:APPROVE(可合并)。 上轮唯一阻塞(描述与实现相反)已修复——描述重写准确并记录 maintainer 对广播样式的统一决策;代码正确、token 全合规(补 --wk-sp-px、去 hover)、构建 + 104 测试通过、规范 CI 绿、与展示层一致。@yujiawei 的规格分歧属已由 maintainer 拍板的产品决策,当前统一方案技术自洽。描述更正后无保留意见。
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-review (rr) + Verdict: #1157 — fix: 输入框 @mention 胶囊背景，展示层对齐设计 token (#1153) @ fdac5b7d
Anchored to live head fdac5b7d999c (rev-parse == live head) / merge-base 2d087f0b. Linear advance from my prior APPROVE @ a9258d4e (0 behind / 1 ahead, no rebase drift). True delta since a9258d4e = 3 files, composer-side polish only.
Verdict: APPROVE ✅
The two blocking themes from the last round — (1) hardcoded 1px token-rule residue in the composer, and (2) the PR description contradicting the code — are both resolved at this head. Composer and display now render each mention type consistently (no post-send visual jump), and the description finally matches the shipped design.
True delta a9258d4e..fdac5b7d (byte-verified)
Components/MessageInput/index.css: removed the composer.mention:hoverrule +transition(cursor istext, so hover implied false interactivity);padding: 1px …→padding: var(--wk-sp-px) var(--wk-sp-2).theme/semantic.css: added--wk-sp-px: 1pxspacing token (the value that P2-1 last round asked to tokenize).__tests__/mentionStyle.test.tsx: dropped the now-removed:hoverassertion; addedpadding: var(--wk-sp-px) var(--wk-sp-2)+ no-transitionassertions to match the CSS.- The display-layer
ui/message/TextContent/index.csstoken migration was already ina9258d4e(not new here); no display TS (Messages/Text/mentionRenderState.ts), send serializer (MessageInput/mentionSendParse.ts), or composerindex.tsxchange this delta → display-only, serialization/eligibility intact.
The crux — composer vs display consistency (byte evidence, both sides)
- Display authority
Messages/Text/mentionRenderState.ts:uid==="all"(@所有人 broadcast) →.mention-entity(capsule, non-interactive); ordinary uid →.mention-entity(capsule, interactive); onlyuid==="channel"(not creatable in the composer) →.mention-highlight(plain). - Display
ui/message/TextContent/index.css:43.mention-entity:color: var(--wk-color-accent),background-color: var(--wk-purple-alpha-08),border-radius: var(--wk-r-xs). - Composer
Components/MessageInput/index.css:453.wk-messageinput-editor .mention(single rule for all mentions incl.-1/-2/-3/allsentinels): samevar(--wk-color-accent)/var(--wk-purple-alpha-08)/var(--wk-r-xs). - Result: normal member → capsule both sides; broadcast @所有人 → capsule both sides. Everything creatable in the composer maps to the same capsule on display ⇒ no post-send style jump. The only intentional deltas are vertical padding (composer
--wk-sp-px/1px vs display--wk-sp-0-5/2px, to avoid capsule bleed inside the 20px editor line-height) and cursor/hover affordance (composercursor:text, no hover; displaycursor:pointer+ hover, since display entities are clickable) — both appropriate, neither changes the resting visual.
Description now matches code ✅
Body describes the unified capsule, lists all 5 files, discloses the TextContent token migration (#6B3DD8→#7f3bf5, radius 4px→3px, WCAG 5.46:1 ≥ AA), cites the display mentionRenderState.ts uid==="all" → capsule behaviour + the #295 regression test, and the "验收证据" block now uses the runnable src/... vitest path. The stale renderHTML/mention-user/"broadcast transparent" narrative and the "3 files" miscount are gone. The description-vs-code contradiction that gated the previous round is cleared.
#1153 spec
The final design unifies (broadcast @所有人 = capsule, same as members), which is the opposite of #1153's literal Expected Behavior text. But that text was premised on a wrong reading of the display layer: getMentionRenderState("all") already returns .mention-entity (capsule), so keeping the composer broadcast plain would itself have reproduced the exact send-time jump #1153 was filed to kill. The body records this as a maintainer-confirmed unification and asks to reconcile the ticket text. Internally consistent + disclosed ⇒ acceptable; the remaining "amend #1153 Expected Behavior" is ticket hygiene, not a code blocker.
Token / tests / CI / deps
- Token rule: no hardcoded color/spacing residue in either mention CSS;
--wk-color-accent,--wk-purple-alpha-08/-12,--wk-r-xs,--wk-sp-px,--wk-sp-2,--wk-sp-0-5all defined intheme/. - CI: all required checks green (Build, install-build, e2e-p0, osv/secret/dependency scans pass);
code-reviewpending = awaiting this review. - Deps: no
package.json/pnpm-lock.yaml/pnpm-workspace.yamltouched — no #1147-style dependency blast radius. - Test delta is trivial and tracks the CSS change; suites verified green at the prior head (104 + 24) and CI Build/e2e green here.
Non-blocking notes
- 🟡 Composer + display CSS consume the primitive
--wk-purple-alpha-08directly; a future semantic "accent-subtle-background" token would be cleaner (not blocking). - 🔵
mentionStyle.test.tsxre-declares a minimal Tiptap mention config rather than importing the productionMessageInputextension setup, so it could miss future divergence in the real component (carried over from last round; non-blocking).
APPROVE: consistent composer/display rendering for both member and broadcast mentions (no post-send jump), truthful description, token rule satisfied, display-only/serialization intact, CI green.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1157 (octo-web)
Reviewed at head fdac5b7d999ce123bd6efe11bef66989819e3f1a, merge-base 2d087f0b. 5 files, +130/−17.
Verified locally at this exact SHA:
| check | result |
|---|---|
pnpm install --frozen-lockfile |
OK |
pnpm --filter @octo/web build |
✓ built in 6.94s |
vitest run src/Components/MessageInput/__tests__/ |
7 files / 104 tests passed |
vitest run src/Messages/Text/__tests__/ |
4 files / 24 tests passed |
git diff --check origin/main...HEAD |
clean |
This head is the sixth on this PR. The delta since the previously-reviewed head a9258d4e is small and is exactly the set of changes the last round asked for. Verdict flips to approve.
1. Spec compliance
Spec: ✅
Delta since a9258d4e — 3 files, +6/−12
mentionStyle.test.tsx | 10 ++++------
MessageInput/index.css | 7 +------
theme/semantic.css | 1 +
theme/semantic.css:193adds--wk-sp-px: 1px, andMessageInput/index.css:457now readspadding: var(--wk-sp-px) var(--wk-sp-2). The last raw literal in the mention rule is gone, closing the token-rule finding (DEVELOPMENT.md:127-142) that blocked round 2.- The composer
:hoverrule andtransition: background-color 0.2sare removed (index.css, −7).cursor: textno longer pairs with hover feedback that implied an affordance the composer mention does not have. - The test was updated in step with both: it now asserts the token-based padding and asserts the absence of a composer
:hoverrule.
Missing (漏建) — none
The composer capsule matches the bubble. Confirmed in the built bundle rather than in source alone, so no var() resolves to nothing:
apps/web/build/assets/App-CwA8aWY8.css
.wk-messageinput-editor .mention { color:var(--wk-color-accent); background-color:var(--wk-purple-alpha-08);
border-radius:var(--wk-r-xs); padding:var(--wk-sp-px) var(--wk-sp-2); cursor:text; vertical-align:baseline; font-weight:500 }
.wk-msg-text-content .mention-entity { color:var(--wk-color-accent); background-color:var(--wk-purple-alpha-08);
border-radius:var(--wk-r-xs); padding:var(--wk-sp-0-5) var(--wk-sp-2); cursor:pointer; font-weight:500; transition:background-color .2s }
apps/web/build/assets/index-*.css
--wk-sp-px:1px
All tokens resolve: --wk-color-accent (theme/semantic.css:25 → theme/primitive.css:20 = #7f3bf5), --wk-purple-alpha-08 / -12 (primitive.css:64,66), --wk-r-xs (semantic.css:207 = 3px), --wk-sp-px / -0-5 / -2 (semantic.css:193,194,197). --wk-sp-px is declared in the same :root block (opens at semantic.css:12) as the rest of the scale, so it ships with them.
The one intentional delta remains vertical padding — composer 1px vs bubble 2px — justified by the editor's 20px line-height. Sound.
Over-built (超建) — resolved
ui/message/TextContent/index.css is still outside the boundary #1153 recommended, but the last round's objection was disclosure, not direction, and disclosure has now happened: the PR body carries a dedicated "展示层（TextContent）—— 设计 token 迁移" section naming the file, the old literals, the new tokens, and the #6B3DD8 → #7f3bf5 hue change. That closes it. theme/semantic.css is a one-line token addition requested by review, so it is not scope creep either.
Divergence (偏离) — resolved for the merge record
The composer unifies broadcast and member mentions into one capsule, which contradicts the literal Expected Behavior text in #1153. The last round already ruled in favour of the code here, and the reasoning stands: getMentionRenderState() (Messages/Text/mentionRenderState.ts:12-19) routes uid "all" to .mention-entity — the capsule — and only uid "channel" to .mention-highlight. That is locked by a regression test titled "广播 mention 与普通成员 mention 同款高亮（视觉一致）" (Messages/Text/__tests__/MarkdownContentMention.test.tsx:56-81). #1153 inferred broadcast behaviour from the wrong rule; keeping broadcast transparent in the composer would have recreated the send-time style jump the ticket was filed about, for broadcast mentions.
What was blocking last round was that the PR record asserted the opposite of the code. It no longer does — the body now states the decision, names it option A, and records that it was confirmed with the maintainer. The merge record and the code now agree. Amending #1153's Expected Behavior is still worth doing as ticket hygiene (see §4) but is no longer load-bearing on this merge.
2. Code quality
Quality: Approved — no P0/P1. P2s below, none blocking.
Re-derived against this head, not carried over:
- Send path untouched. Outbound text is built by
extractMentionsFromEditorwalkingeditor.getJSON()and callingserializeMentionMarker(node.attrs.id, node.attrs.label, trusted)(MessageInput/index.tsx:371-381). It never readsgetHTML()orrenderText, so mention markers, theMENTION_TRUST_MARKbroadcast-trust scheme (Utils/mentionRender.ts:150-153) and draft round-trip are unaffected by anything in this diff. renderLabel→renderTextis correct. In@tiptap/extension-mention@3.22.2,renderLabelis deprecated-but-honoured:dist/index.js:145-150and:203-205short-circuitoptions.renderHTML/options.renderTextand emit aconsole.warnon every render. Removing it stops that warning.renderTextis a declaredMentionOptionsmember (dist/index.d.ts:47) consumed atdist/index.js:207. The?? node.attrs.idfallback also fixes rendering a literal@nullwhenlabelis absent.- No selector bleed — checked, not assumed.
class: "mention"has exactly one emitter (index.tsx:543) and.wk-messageinput-editorhas exactly one mount point (index.tsx:1337).MessageInput/index.csscontains exactly one.mentionrule. The suggestion dropdown uses.mention-list-itemand is portaled todocument.body, so it cannot pick up the capsule. - No line-height regression. Padding on an inline non-replaced element does not contribute to line-box height, so the 20px editor rows (
index.css:437) do not grow. The residual effect is 1px of background bleed past the line box on wrapped lines, which is what the 2px → 1px tightening mitigates. - Security surface: none. This PR is labelled security-sensitive, but that is a keyword false positive — the only match for
tokenin the diff is design token. No auth, credential, middleware, storage-format or serialization code is touched; the diff is CSS plus a two-line render-callback swap. Secret scan and OSV scan are green.
P2-1 — the CSS assertions can be green while the UI is broken, and red while it is fine
packages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx:43-59
Regex-matching index.css off disk with /\.wk-messageinput-editor \.mention\s*{([^}]*)}/ asserts text presence, not applied style. It passes even if a token were undefined at runtime or the rule were overridden downstream, and it breaks on harmless reformatting, selector grouping, or a comment inside the block. The new not.toMatch(/transition/) assertion is the sharpest example: it also fails on a rule that merely mentions transition in a comment. A computed-style assertion (getComputedStyle in jsdom with the sheet attached) or a small PostCSS parse would be worth more than the whole block. Carried from last round, unchanged, still non-blocking.
P2-2 — the DOM half of the test exercises Tiptap and a hand-copied config, not production code
mentionStyle.test.tsx:16-38
createMentionEditor() re-declares the mention config rather than importing the one at index.tsx:541-577. The assertions at :68, :85, :96 then verify that Tiptap emits class="mention" and data-* — for a class the test itself supplied. If production drifts, these stay green. Carried, unchanged.
P2-3 — --wk-sp-px sits under a header that says the opposite
packages/dmworkbase/src/theme/semantic.css:192-193
/* ── Spacing — 4px 栅格 ── */
--wk-sp-px: 1px;A 1px token is by definition off the 4px grid the comment declares. This is the right fix for the literal it replaced, but a one-clause note (e.g. "1px hairline, off-grid, for inline capsule padding") would stop the next reader from treating it as a general-purpose step and reaching for it in block layout.
P2-4 — dormant dark-mode gap
theme/semantic.css:25
--wk-color-accent is fixed to --wk-purple-500 with no body[theme-mode=dark] override, while the sibling --wk-text-accent switches to the lighter --wk-purple-400. Dark mode is force-disabled today (App.tsx:827 → ThemeMode.light), so this is dormant, not live — but both mention surfaces will need a dark-mode accent when it is re-enabled. Carried, unchanged.
Nit — the PR body still has two stale claims about this head
Not blocking, and calling this out only so the squash-merge record can be cleaned up in passing:
- It claims
hover 态 var(--wk-purple-alpha-12)on the composer mention. This head deletes that rule — the new test asserts its absence. The sentence describes the previous head. - "改动文件（4 files, +136 -17）" — it is 5 files, +130/−17.
packages/dmworkbase/src/theme/semantic.cssis missing from the list.
The distinction from last round matters: then, the body inverted an acceptance criterion (claimed broadcast mentions kept a transparent background when the code gave them a capsule), and reviewers had reasoned from that claim. These two are a leftover sentence about a removed nicety and an un-incremented file count. Nobody is misled about behaviour, so this does not warrant another round.
3. Overall verdict
APPROVED — Spec ✅ AND Quality: Approved. Every blocking item from the previous round is closed, and the two P2s that round flagged as optional (token for the 1px literal, drop the composer hover) were both taken. Build and 128 tests pass at this SHA.
4. Follow-ups (none blocking this merge)
- Amend #1153's Expected Behavior to record that broadcast mentions already render as
.mention-entityin the display layer (citementionRenderState.ts:12-19and the regression test atMarkdownContentMention.test.tsx:56-81), so the ticket stops asserting the opposite of what shipped. Please do not reintroduce a broadcast/member split in the composer. - Tidy the two stale body claims above before squash-merge.
- P2-1 / P2-2 (assert computed style, import the production config) are worth a small follow-up PR for whoever next touches mention styling.
5. For a human to verify before merge
Two gaps that static review and unit tests structurally cannot close. Neither is a reason to hold the PR another round, but both are worth two minutes of eyes-on:
- Nobody has driven this in a browser, across six heads. The entire payload is visual, and the PR carries no screenshot. Caret and selection geometry around a padded inline atom, capsule bleed on wrapped multi-line input, IME composition adjacent to a capsule, and RTL layout are all unexercised. One before/after image of the composer would be worth more than the current test file.
- The display-layer migration repaints every mention in message history.
.mention-entitymoves#6B3DD8→#7f3bf5andborder-radius: 4px→3px(--wk-r-xs), affecting regular and broadcast mentions in all existing messages, not just new ones. I cleared the mechanical risks myself — contrast on white goes 6.39:1 → 5.46:1, both above WCAG AA 4.5:1, and a repo-wide search finds no snapshot or style assertion pinned to the old literals — so there is no accessibility or test regression. What remains is a purely aesthetic call on a −1px radius change to the primary message surface, and that belongs to whoever owns the design system, not to a reviewer.
6. Process note
Six heads on a P3 CSS fix (2f876709 → 58d25333 → 1ba5c3d1 → b8311762 → a9258d4e → fdac5b7d), with the substantive design changing twice mid-flight (per-member mention-user split → unified capsule). The convergence is real and this head is clean, so this is not a criticism of the last two rounds — they were tightening, not thrashing. The lesson worth carrying forward is that rounds 1–4 were spent discovering that the ticket's stated acceptance criterion was itself wrong. On the next visual-consistency ticket, confirming the target design against the existing display-layer contract before opening the PR would collapse most of that. Also note this is the only review on this head; earlier approvals are pinned to superseded commits.
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review — PR #1157 (octo-web)
Summary
This PR applies capsule-style background to @mention nodes in the message input editor and aligns both input and display mention styles to the canonical design token system (--wk-color-accent, --wk-purple-alpha-08, --wk-r-xs, --wk-sp-*). It also migrates the tiptap mention extension from the deprecated renderLabel option to the v3 renderText option, adding a defensive label ?? id fallback. A new --wk-sp-px: 1px spacing token is introduced in semantic.css. New unit tests cover CSS contract assertions and DOM rendering for both regular and broadcast-sentinel mentions.
Verification
- ✅ Design tokens resolve — every token referenced in the changed CSS (
--wk-color-accent,--wk-purple-alpha-08,--wk-purple-alpha-12,--wk-r-xs,--wk-sp-px,--wk-sp-0-5,--wk-sp-2) is defined inpackages/dmworkbase/src/theme/primitive.cssorsemantic.css. No danglingvar()references. - ✅
renderTextAPI is correct for tiptap v3 —@tiptap/extension-mention: ^3.22.2usesrenderTextfor text serialization; the deprecatedrenderLabelis fully removed from the codebase (zero remaining references). - ✅
label ?? idfallback is consistent — same pattern already used atindex.tsx:1445and:1463for clipboard/selection text extraction paths. Mention creation paths (mentionResolve.ts,mentionSendParse.ts, draft rehydration at:916) always set bothidandlabel, so the fallback is defensive-only. - ✅
isBroadcastSentinelUidimport path valid — exported atUtils/mentionRender.ts:137, imported correctly in the test. Sentinel set (-1,-2,-3,all) matches the production definition. - ✅ Hardcoded-to-token color migration is intentional — old
#6B3DD8/rgba(107, 61, 216, …)values in TextContent were ad-hoc purples that differed from the canonical#7f3bf5/rgba(127, 59, 245, …)primitives. The PR title explicitly states design-token alignment as the goal. - ✅ No
renderLabelresiduals —grep renderLabelacross all.ts/.tsx/.jsfiles returns zero hits.
Static analysis only at head fdac5b7d999ce123bd6efe11bef66989819e3f1a; build and tests not executed in this environment.
Findings
No P0/P1 issues. One nit below.
Nit — Input vs display vertical padding asymmetry (packages/dmworkbase/src/Components/MessageInput/index.css:456)
The input editor .mention uses padding: var(--wk-sp-px) var(--wk-sp-2) (1px / 8px) while the display .mention-entity at packages/dmworkbase/src/ui/message/TextContent/index.css:46 uses padding: var(--wk-sp-0-5) var(--wk-sp-2) (2px / 8px). The CSS comment says "对齐消息展示层 .mention-entity" but the vertical padding token differs. Likely intentional to account for editor line-height differences, but worth a brief confirmation.
Things I checked that are fine
- tiptap v3 API migration:
renderLabel→renderTextis the correct v3 path; HTML output is handled by the defaultrenderHTMLwithHTMLAttributes: { class: "mention" }. - Token completeness:
--wk-sp-pxis new but properly slotted into the 4px spacing grid atsemantic.css:193, positioned correctly before--wk-sp-0-5. - CSS scoping:
.wk-messageinput-editor .mentionis scoped to the editor container;.wk-msg-text-content .mention-entityis scoped to the message display. No selector bleed. - Test coverage: The new test file covers the CSS contract (via
readFileSyncregex assertions), DOM rendering of regular and broadcast mentions, and theisBroadcastSentinelUidclassification. Tests correctly assert the absence of:hoverstate andtransitionon input mentions. - C1 (dual-path parity): Both input editor and message display surfaces updated in lockstep. Broadcast and regular mentions share the same
.mentionCSS class in the editor. - C2 (control-flow ordering): No control-flow logic changes;
renderTextis a pure serialization hook. - C3–C6: N/A — no auth, lifecycle, build, or governance changes.
Verdict: APPROVED
Clean, well-scoped design-token alignment with a correct tiptap v3 API migration. The single nit is non-blocking — the padding asymmetry is likely intentional. No correctness, security, or build-breaking issues.
[Octo-Q] verdict: APPROVE — No P0/P1 findings. CSS token migration is correct, tiptap v3 API migration is clean, all tokens resolve, data flow for label ?? id fallback is consistent with existing code paths.
## Summary - Give ordinary member mentions a visible pill state in the message input, matching the already-shipped sent-message pill metrics. - Keep broadcast mentions (`@所有人`, `@所有AI`) as text-only purple highlights, following the three-level behavior requested in #1153. - Align the input editor and display renderer so sending does not change broadcast mentions from text-only to a pill. - Strengthen mention style coverage around broadcast sentinel IDs, Tiptap `data-id` output, and display render state. ## Product Decision #1153 explicitly asks for ordinary member mentions to use purple text with a light-purple background, while `@所有人` / `@所有AI` may continue using the existing pure-purple text treatment so broadcast mentions remain visually distinct from ordinary members. This PR follows that #1153 direction instead of the #1157 unified-pill direction. The repo currently records both interpretations, so maintainers should pick one approach and close or retarget the other PR before merge. ## Scope and Behavior - Input editor: ordinary member `.mention` nodes use the pill style; broadcast sentinel IDs (`-1`, `-2`, `-3`) remain text-only. - Message display: `uid === "all"` now maps to `mention-highlight`, matching the text-only broadcast style after send. - Existing message history containing broadcast mentions will render as text-only purple highlights instead of ordinary-member pills. - Existing ordinary member mentions keep the shipped display pill metrics: 8% tint background, 4px radius, `2px 8px` padding, and `500` font weight. - Input ordinary member mentions use the same pill metrics through semantic tokens, so the fix brings the editor up to the display behavior instead of redesigning the display layer. - Input broadcast mentions explicitly use `400` font weight to match sent-message `mention-highlight` and avoid a send-time weight jump. ## Traceability - Keeps the GH#295 regression intent: broadcast mentions must stay visibly highlighted, not inert plain text. - Applies #1153's clarification that this highlight is the broadcast text-only state, while ordinary members use the pill state. - Uses semantic mention tokens for the shipped member pill color/radius so component CSS does not carry private hardcoded values. ## Verification - `corepack pnpm --dir packages/dmworkbase exec vitest run src/Components/MessageInput/__tests__/mentionStyle.test.ts` - `corepack pnpm --dir packages/dmworkbase exec vitest run src/Messages/Text/__tests__/mentionRenderState.test.ts src/Messages/Text/__tests__/MarkdownContentMention.test.tsx` - `git diff --check` - `corepack pnpm exec stylelint packages/dmworkbase/src/Components/MessageInput/index.css packages/dmworkbase/src/ui/message/TextContent/index.css packages/dmworkbase/src/theme/semantic.css --config stylelint.config.mjs --allow-empty-input` (0 errors; existing warnings remain) - `corepack pnpm --filter @octo/base test -- --maxWorkers=4` Fixes #1153
5 participants
Fixes #1153
背景
Web 端消息输入框内 @mention 普通成员只显示紫色文字，没有浅紫胶囊背景；发送后才出现胶囊，造成输入↔发送视觉跳变。
决策说明
Issue 原文要求
@所有人/@所有AI保持纯紫文字无胶囊，但展示层mentionRenderState.ts:12-19对uid === "all"返回.mention-entity（同款胶囊），GH#295 测试锁定了「广播 mention 与普通成员同款高亮（视觉一致）」。经 maintainer 确认采用统一方案：输入框与展示层一致，广播与普通成员均使用胶囊样式，消除发送跳变。变更摘要（5 files, +137 -29 vs origin/main）
输入框（MessageInput）
index.css：.wk-messageinput-editor .mention统一胶囊样式，使用设计 token：color: var(--wk-color-accent)（#7f3bf5）、background-color: var(--wk-purple-alpha-08)、border-radius: var(--wk-r-xs)（3px）、padding: var(--wk-sp-px) var(--wk-sp-2)（1px 8px，垂直 1px 防止 20px 行高内溢出）、font-weight: 500、cursor: text、vertical-align: baseline；无:hover态（cursor:text 不响应 hover，避免虚假交互暗示）index.tsx：移除废弃renderLabel（消除 deprecation warning 并修复无 label 时渲染@null的问题），改用renderText兜底（label ?? id）；Tiptap 默认渲染器已正确输出class="mention"+data-type/data-id/data-label，无自定义renderHTML展示层（TextContent）—— 设计 token 迁移
ui/message/TextContent/index.css：.mention-entity/:hover/.mention-highlight/.mention-fallback全部从硬编码#6B3DD8/rgba(107,61,216,...)/4px/2px 8px迁移到设计 token，与输入框共用同一套 token 确保视觉一致--wk-purple-500），圆角从 4px 统一为 3px（--wk-r-xs）；WCAG 对比度 5.46:1，仍高于 AA 4.5:1设计 token
theme/semantic.css：新增--wk-sp-px: 1px间距 token，供 mention 胶囊垂直 padding 使用测试
__tests__/mentionStyle.test.tsx：CSS 源码字符串断言.mention规则含正确 token、无 transparent、无:hover规则；最小 TiptapMention 配置验证普通成员 + 广播 sentinel（-1/-2/-3/all）均渲染class="mention"改动文件
packages/dmworkbase/src/Components/MessageInput/index.tsxpackages/dmworkbase/src/Components/MessageInput/index.csspackages/dmworkbase/src/Components/MessageInput/__tests__/mentionStyle.test.tsx（新增）packages/dmworkbase/src/ui/message/TextContent/index.csspackages/dmworkbase/src/theme/semantic.css验证证据
Comprehension gate
不涉及公开 API/协议/存储格式变更，纯视觉修复。回滚方式：revert 本 PR。