feat(ui): add shared tooltip component

[Candice00711]

Summary

Add a shared @octo/ui Tooltip primitive based on the approved component preview, then migrate verified Semi/WK tooltip consumers without changing their business actions.

This is a stacked PR and depends on #1413 (Tag refresh) and #1439 (Badge/Dot). Please review and merge those dependencies first.

Related Issue

None.

Changes

• Add a business-agnostic Tooltip API that keeps Semi positioning details internal, supports the approved content variants, automatic viewport adjustment, optional 300 ms entry delay, and immediate dismissal.
• Add Storybook design variants, technical auto-flip coverage, component tests, exports, tokens, and the small Button text variant required by Tooltip compositions.
• Migrate verified Tooltip usages in Base, Contacts, MCP, and Summary while preserving clicks, permissions, hidden capability gates, truncation behavior, and existing placement.
• Remove only the migrated legacy tooltip CSS; native tooltips, dead entries, SpaceList, VoiceSettingsPanel, and the disputed Summary-list create tooltip are intentionally left unchanged.

Architecture / Module Boundary

• Affected module(s): @octo/ui, Base, Contacts, MCP, Summary
• New or changed user-visible entry point: no
• Shared layer touched: ui, Components, Messages
• If shared code changed, impact scope: shared Tooltip rendering plus the explicitly migrated consumers; routing, requests, permissions, caching, and business actions are unchanged
• Duplicate entry point checked: yes

Testing

• pnpm --filter @octo/ui test — 5 files, 28 tests passed
• pnpm --filter @octo/ui typecheck
• Base focused tests — 4 files, 33 tests passed
• pnpm --filter @octo/contacts test — 9 files, 38 tests passed
• MCP focused tests — 3 files, 20 tests passed
• Summary Tooltip/list focused tests — 5 files, 38 tests passed
• pnpm --filter @octo/web build
• Storybook and reachable real-page cases manually verified in light and dark themes

Known baseline: SummaryCard.test.tsx has the same 12 unrelated failures on the parent Badge/Dot worktree; this PR does not change that failure set.

Checklist

• I have read CONTRIBUTING.md
• PR description is in English
• Added tests for my changes
• Updated documentation (Storybook documents the public component contract)
• Confirmed the change does not affect application UI copy
• Confirmed the change follows module ownership and does not add duplicate user-visible entry points
• Described impact scope when shared components, messages, bridge, or services are changed
• Followed commit message conventions (Conventional Commits)

[github-actions]

Dependency Changes Detected

This PR modifies dependency files. Please review whether these changes are intentional.

Changed files:

• apps/extension/package.json
• apps/web/package.json
• packages/dmworkbase/package.json
• packages/dmworkcontacts/package.json
• packages/dmworkmcp/package.json
• packages/dmworksummary/package.json
• packages/octo-ui/package.json
• pnpm-lock.yaml

Maintainer checklist:

• Confirm dependency changes are intentional
• Review package delta if lockfile changed

[Jerry-Xin]

Shared Tooltip primitive is well-built (portal-based, a11y-complete, tokens/exports/CI scoped), but the overflow-tooltip consumer migration introduces a stale ResizeObserver regression that breaks truncation tracking after the first state toggle.

🔴 Blocking

• 🔴 Stale resize observation in the migrated truncation tooltips. Each of these components measures a node with ResizeObserver set up once (effect dep = content only), then conditionally switches its rendered root between the bare element and <Tooltip>. That switch changes the element type at the component root, so React unmounts the measured node and mounts a new one — while the observer stays attached to the now-detached node. From that point on, container-only resizes no longer update isTruncated; eligibility only recovers on a window resize or a content change. Affected:

  • packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:24-60
  • packages/dmworkcontacts/src/Contacts/index.tsx:35-63
  • packages/dmworksummary/src/components/OverflowTooltip.tsx:17-46

  This is a behavior regression versus the old trigger="custom" implementation, which re-checked truncation on every mouseenter. Fix options: keep the rendered hierarchy stable (always render the wrapper and gate via isDisabled while keeping the measured node mounted), or re-observe whenever the measured node identity changes (callback ref). Also add a regression test that drives ResizeObserver after the wrapper mounts — the current OverflowTooltip.test.tsx only dispatches window.resize (which always reads the live ref), so it passes while the real resize path is broken. I confirmed the failure mode by inspection and reproduced the test-suite gap locally.

💬 Non-blocking notes

• isDelayed sets mouseEnterDelay=300 with mouseLeaveDelay=0, which trips Semi's dev-console warning ("mouseLeaveDelay cannot be less than mouseEnterDelay") on every update of a delayed tooltip. Functionally harmless here (the foundation clears the show/hide timer on both paths, so the overlay still hides), but worth a code comment or a mouseLeaveDelay that satisfies the invariant to keep dev consoles clean.
• Migration drops showArrow and explicit position="top" in a few spots (e.g. PdfRenderer.tsx sidebar toggle). That matches the arrow-less design defaults, just confirming it is intentional.

✅ Highlights / verified

• Rendering model is a portal to document.body (Semi default getPopupContainer, z-index 1060) — no inline rendering, so tooltips in scroll/overflow containers (conversation rows, TooltipCell grid cells, NavRail) are not clipped, and tooltips inside modals stack above the modal (default Modal z-index 1000). Verified ClawInfoModal specifically.
• Accessibility is inherited correctly from Semi: role="tooltip", auto aria-describedby on the trigger, focus shows the tooltip, Escape dismisses. Delay semantics (300 ms enter / 0 ms leave) match the described behavior.
• New Button (incl. the text variant) is a fully isolated octo-ui component with BEM-scoped selectors and namespaced keyframes — no cascade/specificity impact on existing buttons.
• tokens.css, package exports, and the ci.yml additions are appropriately scoped. No leftover legacy Semi tooltip imports in migrated files (remaining ones are the wrapper itself and intentionally-unmigrated SpaceList/ThreadList).
• Stacked dependency portions are byte-identical to their approved heads: Tag matches #1413 (ca108432) and Badge/Dot match #1439 (9c541d61). Per the author, #1413 and #1439 should be merged first.
• Tests run locally: @octo/ui 28/28 pass; Contacts 38/38; base Badge/Dot migration set 31/31; OverflowTooltip 7/7. The broader dmworksummary suite shows 97 failures, but these are identical on the dependency baseline (verified via a worktree at 9c541d61) — pre-existing and not introduced by this PR.

[yujiawei]

Code Review — PR #1478 (octo-web)

Reviewed at head SHA ecd72ef4a46be33504c51775b77a607e84c1cba9.

Scope note. This is a stacked PR whose base is main, so gh pr diff shows 127 files. Its own incremental work is git diff 9c541d614..HEAD (commits 40445bd66 + ecd72ef4a): 35 files, +877/-203. Everything below is scoped to that incremental diff; content inherited from #1413 (Tag) and #1439 (Badge/Dot) is out of scope here.

---

1. Spec compliance

Spec: ❌

No linked issue, so the diff is judged against the scope stated in the PR description.

• Missing work: none found. The Tooltip primitive, tokens, exports, stories, tests, the Button text variant, and the four consumer migrations are all present.
• Extra work: ecd72ef4a re-orders the pre* lifecycle scripts in apps/web/package.json. I verified the resulting scripts map is key-for-key and value-for-value identical to the parent commit, so this is behaviour-neutral — but it is unrelated churn that the "Changes" section does not mention.
• Deviation: the description states the migration preserves "truncation behavior". It does not. The three overflow-tooltip consumers were rewritten from measure-on-every-hover to measure-once-into-cached-state + ResizeObserver, and the observer wiring has a defect (see P1-1). This is a different mechanism with a different failure mode, and it is not called out as a behavioural change.

Claims I did verify as accurate:

• SummaryListPage create tooltip is indeed left on Semi (ecd72ef4a reverts it) ✔
• pnpm --filter @octo/ui test → 5 files / 28 tests pass ✔; @octo/ui typecheck passes ✔
• pnpm --filter @octo/contacts test → 9 files / 38 tests pass ✔
• "this PR does not change that failure set" for dmworksummary ✔ — the failing set is byte-identical at the merge-base 6e52f7e45 and at the parent 9c541d61. Note the description understates the baseline: it is 9 files / 50 tests, not "SummaryCard.test.tsx … 12 failures".

---

2. Code quality

Quality: Changes-Requested

P0 — CI Build is red, and it is caused by this PR

packages/dmworkmcp/src/components/__tests__/McpDetailModal.inlineDelete.test.tsx fails at module load:

TypeError: Cannot set properties of null (setting 'fillStyle')
 ❯ node_modules/.pnpm/lottie-web@5.13.0/.../lottie.js:1119:21

Root cause chain:

1. packages/dmworkmcp/src/components/McpDetailModal.tsx:11 — import { resolveOwner } from "./McpCard";
2. packages/dmworkmcp/src/components/McpCard.tsx:2 — this PR changes it to import { Tooltip } from "@octo/ui";
3. packages/octo-ui/src/components/Tooltip/index.tsx:2 — import { Tooltip as SemiTooltip } from "@douyinfe/semi-ui";
4. The test mocks Semi at line 22 specifically to keep the real barrel out of jsdom:

vi.mock("@douyinfe/semi-ui", () => ({
  Toast: { success: vi.fn(), error: vi.fn() },
  Spin: () => null,
}));

That mock only covers the copy resolved from packages/dmworkmcp. pnpm gives @octo/ui a different physical @douyinfe/semi-ui (different peer-dependency hash):

packages/dmworkmcp/node_modules/@douyinfe/semi-ui -> .../@douyinfe+semi-ui@2.93.0_..._81d28888c1701e8dbffbbbf11506916c/...
packages/octo-ui/node_modules/@douyinfe/semi-ui  -> .../@douyinfe+semi-ui@2.93.0_..._52db265bf2d87afaa22c90606adbced0/...

Different module id ⇒ the mock does not intercept it ⇒ the real Semi barrel loads ⇒ @douyinfe/semi-foundation pulls lottie-web, which calls canvas.getContext('2d') at module scope and gets null under jsdom.

Reproduced locally at this SHA:

• fails at HEAD;
• passes when McpCard.tsx alone is reverted to the merge-base version;
• passes again at HEAD after adding vi.mock("@octo/ui", () => ({ Tooltip: ({ children }) => children })) to that test file.

That is exactly the stub already added to three sibling test files in this PR (McpOfficialPublisher.test.tsx, contacts onlineBadgeSync.test.tsx, visibilityHeal.test.tsx) — this one was missed. It slipped through because the description's testing line is "MCP focused tests — 3 files, 20 tests passed", while the CI gate runs the whole package: pnpm --filter @dmwork/mcp test.

Please also treat this as systemic, not a one-file typo: any existing test that mocks @douyinfe/semi-ui to keep Semi out of jsdom will silently start loading the real barrel the moment its component tree reaches an @octo/ui component. A shared setup file that stubs @octo/ui, or deduping Semi to one physical copy, is more durable than adding one vi.mock per file as migrations land.

Full local run at this SHA, for the record:

Suite	Result
@octo/ui test / typecheck	5 files, 28 tests pass / pass
@octo/base (full)	357 files, 3197 tests pass
@octo/contacts (full)	9 files, 38 tests pass
@dmwork/skillmarket (full)	13 files, 128 tests pass
@dmwork/mcp (full)	1 file fails (above)
@dmwork/summary (full)	9 files / 50 tests fail — identical at merge-base, pre-existing

---

P1-1 — ResizeObserver is left observing a detached node after the truncation flip

• packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:24-41 (deps [content])
• packages/dmworkcontacts/src/Contacts/index.tsx:38-55 (deps [text])
• packages/dmworksummary/src/components/OverflowTooltip.tsx:17-35 (deps [children, title])

All three follow the same shape:

// TooltipCell.tsx:52-61
if (!isTruncated || !hasContent) {
  return cellContent;
}
return <Tooltip content={content}>{cellContent}</Tooltip>;

When isTruncated flips, the element type at the component's root changes from "div" to Tooltip. React does not reconcile across a type change — it unmounts the measured <div> and mounts a brand-new one inside the Semi overlay wrapper. The effect does not list isTruncated in its deps, so it never re-runs: resizeObserver keeps observing the detached node and never fires again. Only the window resize listener survives, because it dereferences ref.current at call time.

Failure scenario: an excel-preview column, a contacts row, or a summary title changes width because a splitter/panel is dragged or a sibling re-flows — no window resize event — so isTruncated is stale. The tooltip then either shows on a cell that no longer overflows, or is missing on one that now does. The previous implementations re-measured scrollWidth > clientWidth inside handleMouseEnter, so they could not go stale.

Fix: add isTruncated to the dependency array, or attach the observer from a ref callback instead of an effect.

Related: in OverflowTooltip.tsx:35 the deps are [children, title], and children is a fresh object identity on every render — so that effect tears down and rebuilds its ResizeObserver on every render. It accidentally papers over the bug above, at a cost.

Why the tests do not catch it: jsdom has no ResizeObserver, so typeof ResizeObserver === "undefined" short-circuits and none of these tests ever construct one. And the rewritten OverflowTooltip.test.tsx:22 now fires a window resize inside mockOverflow, which exercises only the one code path that still works.

---

P1-2 — The MCP "+N" tag tooltip loses its light surface and drops below AA contrast

• packages/dmworkmcp/src/components/McpCard.tsx:178-192 drops className="wk-mcp-tooltip-light"
• packages/dmworkmcp/src/index.css deletes the whole .wk-mcp-tooltip-light block

That tooltip's content is a cluster of accent chips (McpCard.tsx:180-186), styled by packages/dmworkmcp/src/index.css:813-816:

.wk-mcp-tag--accent {
  background: var(--wk-accent-tint-06, rgba(127, 59, 245, 0.06));
  color: var(--wk-color-accent);
}

--wk-color-accent → --wk-purple-500 → #7F3BF5 (packages/dmworkbase/src/theme/semantic.css:25, primitive.css:20). The new primitive forces background-color: var(--octo-ui-tooltip-bg) = #1C1C23 (packages/octo-ui/src/styles/tokens.css:53).

#7F3BF5 on #1C1C23 ≈ 3.1:1 — below the 4.5:1 AA threshold for this 12px chip text. The 6%-opacity chip fill is effectively invisible on the dark surface, so the "mini pill cloud" that the (now-deleted) comment describes no longer reads as pills at all.

There is also no way for a consumer to fix this locally: the primitive exposes no surface variant, and .octo-ui-tooltip.octo-ui-tooltip (packages/octo-ui/src/components/Tooltip/index.css:1) is deliberately doubled to outrank consumer classes. If a single dark surface is the intended design, the MCP chips need their own on-dark treatment as part of this migration.

---

P1-3 — Button variant="text" is unreadable on the surface it was added for

packages/octo-ui/src/components/Button/index.css:91-101:

.octo-ui-button--text { background: transparent; color: var(--wk-color-accent); }
.octo-ui-button--text:hover:not(:disabled) { color: var(--wk-color-accent-hover); }

The description says this variant is "required by Tooltip compositions", and the stories place it inside the tooltip (Tooltip.stories.tsx:128-137, 165-176). On the fixed #1C1C23 tooltip surface:

• rest #7F3BF5 ≈ 3.1:1
• hover --wk-color-accent-hover → --wk-purple-600 → #6A2FD0 ≈ 2.4:1

So hover makes the action less legible — the opposite of the intended affordance — because the accent pair is designed to darken against light surfaces. Suggest a dedicated on-dark token pair (e.g. --octo-ui-tooltip-action-color / -hover) or a .octo-ui-tooltip .octo-ui-button--text scope, rather than reusing the light-surface accent.

---

P2 — non-blocking

1. Duplicated layout default. packages/octo-ui/src/components/Tooltip/index.tsx:45 and :101-102 both compute content.layout ?? (content.title ? "vertical" : "horizontal"). Two copies of one rule will drift; extract it.
2. Blank guard is shallow. hasNodeContent (index.tsx:25-29) rejects only null, undefined, false and blank strings. true, [], <></>, and <span>{""}</span> all pass it and mount an overlay that renders nothing — precisely the "stray empty dark bubble" that the removed trigger="custom" comments in TooltipCell.tsx and OverflowTooltip.tsx were written to prevent.
3. Hand-maintained ambient types. packages/octo-ui/src/index.d.ts:12,29-35,52 must be edited in lockstep with src/index.ts on every new component. Emitting .d.ts from tsup would remove a standing drift risk.
4. Keyboard reachability. The primitive hardcodes Semi's default hover trigger and exposes no trigger prop, so tooltip content is mouse-only. Most migrated call sites still carry an aria-label (e.g. McpCard.tsx:189), but TooltipCell.tsx:43-50 has no accessible name at all.
5. No getPopupContainer / zIndex escape hatch. ClawInfoModal.tsx:348 renders a tooltip inside a modal; Semi portals the overlay to document.body, so stacking and clipping there depend entirely on global z-index. Worth an explicit visual check in that modal.
6. Partial migration. Semi tooltips remain in ThreadList, ConnectionStatus, JsonlRenderer, ExcelRenderer, AIMessageCard, ChatSummaryStarButton, SummaryDetailPage, SkillCard, SkillDetailModal, plus the intentionally deferred SpaceList / VoiceSettingsPanel / SummaryListPage. Reasonable for an increment, but the app now ships two tooltip surfaces — a follow-up ticket would keep that from becoming permanent.
7. Merge order. #1478, #1439 and #1413 all target main, so merging this one first would land the other two PRs' content along with it. The description already asks for the dependencies to go first; that ordering needs to be enforced at merge time, not just documented.

---

3. Overall verdict

CHANGES_REQUESTED

Spec ❌ (undisclosed change to truncation mechanism) AND Quality Changes-Requested (P0 CI break + three P1s). The P0 alone is blocking: Build is red on this head SHA and the cause is reproducible and PR-attributable.

The primitive itself is well-shaped — Semi placement names stay internal, the content-config API is clean, and the disciplined "leave the disputed consumers alone" call is the right one. The problems are at the seams: the new cross-package import breaks an existing Semi mock, the truncation rewrite has a real invalidation bug, and two removed/added styles put purple text on a dark surface.

4. Suggested next steps

1. Add the @octo/ui stub to McpDetailModal.inlineDelete.test.tsx, and run the full pnpm --filter @dmwork/mcp test (not focused files) before pushing. Consider a shared setup stub so the next migration does not repeat this.
2. Add isTruncated to the effect deps in the three overflow components (or move the observer to a ref callback), and add one test that flips truncation via the observer path rather than window resize.
3. Give the tooltip surface its own on-dark tokens for chips and text actions; verify the MCP "+N" tooltip and the tooltip action buttons in both themes.
4. Optional: fold the duplicated layout-default expression into one helper, and generate index.d.ts from tsup.

5. Additional observations

• dmworksummary's full suite is red on main today (9 files / 50 tests). It is not this PR's doing, but CI only runs one file from that package, so the breakage is invisible to the gate and will keep growing. Worth a separate ticket.
• Advisory coverage: two independent advisory passes were run alongside this review. One independently reached the same conclusion on the detached-observer defect in all three overflow components (P1-1) and on the blank-content guard (P2-2). The other flagged TooltipCell.tsx for "computing isTruncated but never using it" — that is incorrect; TooltipCell.tsx:53 does consume it, and the claim is rejected. Its other two findings landed in AiBadge and Subscribers, which belong to the parent PRs and are out of scope here. Areas that none of the passes could verify: real-browser rendering (Semi's autoAdjustOverflow flip behaviour, the interaction between the wrapper padding and Semi's own .semi-tooltip-content padding, and overlay stacking inside modals), and Storybook visual output — those still need a human pass in both themes.

[mochashanyao]

[Octo-Q · automated review]

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

---

Code Review — PR #1478 (octo-web)

Reviewer: Octo-Q (automated review)

Summary

Adds a business-agnostic Tooltip primitive to @octo/ui (Semi-based, fixed dark surface, no arrow/automated review, 320px wrapping, optional 300ms entry delay) and migrates verified Semi/WK tooltip consumers in Base, Contacts, MCP, and Summary, plus a small Button text variant, tokens, exports, stories, and tests. This is a stacked PR: I reviewed the tooltip-only scope 9c541d614..ecd72ef4a (35 files, +877/−203) on top of the #1439 head, not the 127-file diff GitHub shows against main.

The shared component is well-designed and the migrations preserve their gating contracts (truncation-only display, empty-content suppression). No P0/P1 issues; two P2 items and one confirmation nit below.

Verification

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

• ✅ Semi delay semantics — verified against semi-ui 2.93.0 docs and semi-foundation/lib/es/tooltip/foundation.js: mouseEnterDelay/mouseLeaveDelay are milliseconds; the docs' "mouseLeaveDelay ≥ mouseEnterDelay" note is not enforced in code, so 300/0 yields delayed-show + instant-dismiss as designed.
• ✅ Placement mapping — SEMI_PLACEMENT covers all 12 TooltipPlacement values (satisfies Record<TooltipPlacement, string>); bottom-start → bottomLeft matches the old position="bottomLeft" in ExpertMarketListPage.
• ✅ Truncation gating preserved — TooltipCell.tsx:53, Contacts/index.tsx:63, and OverflowTooltip.tsx:46 all conditionally mount the tooltip only when measured overflow (plus non-empty content), replacing the old controlled-visible pattern without losing the "no empty bubble" invariant.
• ✅ No orphaned CSS/props — removed .wk-fold-session-tooltip bubble CSS and wk-mcp-tooltip-light block have zero remaining references; no leftover Semi-only props (position/trigger/visible/showArrow) on migrated call sites (remaining grep hits are Modal/Dropdown/Popover).
• ✅ Manifests — dmworkcontacts gains @octo/ui: workspace:* matching pnpm-lock.yaml; octo-ui files gains Tooltip/types.ts consistent with the source-backed types pattern; apps/web/package.json change is a pure reorder of the same seven pre* scripts (verified 1:1).
• ✅ Stack hygiene — last commit ecd72ef4a reverts the SummaryListPage.tsx hunk from the first commit (net diff empty), consistent with "disputed Summary-list create tooltip intentionally left unchanged".

Findings

No P0/P1 issues; two P2 items and one nit below.

P2 — ResizeObserver keeps watching a detached node after the truncation flip (packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:32)

The effect registers a ResizeObserver on ref.current once (deps [content]), but the render switches between the bare cellContent div and <Tooltip>{cellContent}</Tooltip> based on isTruncated (:53). The conditional wrap changes the element type at the root, so React remounts the measured div inside the Semi trigger while the effect stays mounted — the observer now watches a detached node. After the first truncation flip, container-only resizes (e.g. toggling the PDF sidebar in FilePreviewPanel) no longer re-evaluate truncation: cells can keep a tooltip after fitting or miss one after becoming truncated, until a window resize or content change. The pre-PR hover-time measurement was always fresh. Diff-scope: new (this PR introduced the effect+conditional-wrap pattern); user-visible but presentational-only — hence P2, not P1. Same pattern in packages/dmworkcontacts/src/Contacts/index.tsx:46 and packages/dmworksummary/src/components/OverflowTooltip.tsx:24. Fix: add isTruncated to the effect deps or use a callback ref so the observer rebinds after the wrap toggles.

P2 — Tooltip line-height sourced from the spacing scale (packages/octo-ui/src/styles/tokens.css:58)

--octo-ui-tooltip-line-height: var(--wk-sp-5) resolves line-height from the spacing scale (20px) while the sibling Button uses --wk-leading-normal (1.5) for the same 14px text. Values happen to be close today, but the namespaces are independent, so a spacing-scale change would silently distort tooltip typography. Use --wk-leading-normal or a dedicated token.

Nit — MCP tooltips: 100ms delay and light surface dropped (packages/dmworkmcp/src/pages/ExpertMarketListPage.tsx:485)

This tooltip and the +N chip tooltip in packages/dmworkmcp/src/components/McpCard.tsx:185 drop mouseEnterDelay={100} (now instant under the shared 0ms default), and the deleted wk-mcp-tooltip-light stylesheet moves that content from the deliberate white floating card to the fixed dark surface. Consistent with the shared design contract and the PR states these consumers were verified; flagging only so both transitions are consciously signed off.

Things I checked that are fine

• Empty/blank content ("", " ", null, false) returns children unwrapped via hasNodeContent; config objects gated by "body" in content + !isValidElement; covered by Tooltip.test.tsx.
• @octo/base does not import @octo/ui, and the Contacts test vi.doMock("@octo/ui", …) mirrors the pre-existing semi-ui mock pattern (only Tooltip is consumed in the contacts graph).
• McpOfficialPublisher.test.tsx and both Contacts tests mock the new @octo/ui import; OverflowTooltip tests rewritten to assert mount-only-on-overflow.
• Dark theme: tooltip intentionally keeps the fixed #1c1c23 surface in both themes (matches Storybook contract "Fixed dark surface"); dark item separators in Conversation/index.css still harmonize.
• No i18n copy changes; all migrated contents keep their t(...) sources.

Verdict: APPROVED

The shared Tooltip contract is sound, the migrations preserve truncation/empty-content gating, and no change makes a working path unavailable or produces wrong data. The two P2s are non-blocking improvements; the RO-rebinding one is worth a fast follow since it regresses the always-fresh hover measurement in resizable panels.

---

附录 — Octo 专项段（供终审参考）

数据流回溯（每个被消费数据 → 上游来源 → 是否真流到消费点）

1. SEMI_PLACEMENT[placement] ← placement prop（默认 "top"）。map 覆盖全部 12 个 TooltipPlacement 值（satisfies Record<TooltipPlacement, string> 编译期校验）→ 不存在 undefined position。
2. Tooltip content ← 各调用方：t() i18n 字符串（ClawInfoModal:343、PdfRenderer:303、Messages/File:931/987、ChannelSearchResults:127、SummaryCard:168）、join 名单串（SummaryCreatePage:1346/1402）、participants.map JSX（Conversation:2353）、tag 簇 JSX（McpCard）。全部经 renderContent 流入 Semi content；空/空白串被 hasNodeContent 短路为裸 children（有测试锁定）。
3. isTruncated ← ref 实测 scrollWidth > clientWidth（mount/content 变化/window resize/RO 触发）→ 消费点为条件包裹（TooltipCell:53 / Contacts:63 / OverflowTooltip:46）。⚠️ 唯一断点即 F1：翻转后 RO 观察已 detach 的旧节点（仅容器级 resize 场景失效，window resize 仍走 ref.current 自愈）。
4. participants ← Conversation 既有 state，数据源未变，仅外层容器从 CSS-hover 气泡换成 Semi 触发器。
5. tooltip tokens ← tokens.css :root（dark 块无 override = 双主题固定深色面，与 Storybook 契约一致）→ Tooltip/index.css 经 var() 消费，链路完整。
6. mouseEnterDelay={isDelayed ? 300 : 0} ← 对照 semi-ui 2.93.0 官方文档 + foundation 源码（lib/es/tooltip/foundation.js 48-54 / 175-182 行）确认单位为毫秒、无 leave≥enter 强制钳制 → 300ms/0ms 与设计意图一致，不是单位 bug。

盲点 checklist（C1–C6）

• C1 双路径 parity：hit → clear。show/hide 两腿独立计时（foundation 源码证实，无 latch）；CSS 删除↔新增成对核验（wk-fold-session-tooltip 气泡类与 wk-mcp-tooltip-light 全块删除，全仓 grep 零孤儿引用，保留的 item/content 类仍被 Conversation:2353-2362 消费）；wrap/unwrap 双路径（disabled/blank → 裸 children）有测试覆盖。
• C2 control-flow ordering / 复用：hit → clear。共享 Tooltip 10 处复用，全部只传受支持 props（TS 类型强制；grep 确认迁移点无残留 position/trigger/visible/showArrow，残留命中均属 Modal/Dropdown/Popover）。非规范 content 形态试穿：0 会渲染、""/" "/null/false 不包裹。
• C3 授权边界：N/A — 纯展示层，无 endpoint/tool/凭证改动。
• C4 授权生命周期/容器级联：N/A — 同上。
• C5 build 通过 ≠ 运行期正确：本环境未装 node_modules，未跑 build/test（诚实声明 static-only）。运行期推演：新消费方 dmworkcontacts 走 workspace:* + exports development/import 条件解析，与 #1413/#1439 已验证链路同构；apps/web 七个 pre* 脚本逐一比对为纯排序无增删；files 数组新增 types.ts 与既有 source-backed 声明模式一致。
• C6 治理/策略文档自洽：N/A — 非治理/文档类 PR。

跨轮 blocker 复检（R6）

N/A — 本 PR 首轮审查，无上一轮 blocker。

额外发现（非 finding）

• 末位 commit ecd72ef4a（chore: avoid stale main conflicts）= apps/web/package.json 脚本重排 + SummaryListPage.tsx 净零回退，均已核实无语义变化。
• 审查基线说明：GitHub 对 main 显示 127 文件（含未合并的依赖 PR #1413/#1439），本审查按 stacked 真实范围 9c541d614..ecd72ef4a（35 文件）执行。
• 环境备注：共享 worktree 的 pr-base ref 在审查中途被并发任务移动过，已改用显式 SHA 复核全部 diff，结论不受影响。

[Octo-Q] verdict: APPROVE — 无 P0/P1：共享 Tooltip 契约完备、placement/delay 语义经 Semi 源码级验证、各迁移点截断/空内容门控保持；仅 2 个 P2（RO 翻转后观察 detach 节点、行高取 spacing token）+ 1 个设计确认 nit，均不阻塞，建议终审维持 APPROVE 并可附 P2 建议。

[yujiawei]

Review addendum — one more P2, plus a scoping note for the P0 fix

P2 — tooltip line-height is sourced from the spacing scale

packages/octo-ui/src/styles/tokens.css:58

--octo-ui-tooltip-line-height: var(--wk-sp-5);

--wk-sp-5 is 20px (packages/dmworkbase/src/theme/semantic.css:244), while the tooltip text is --wk-text-size-md = 14px (semantic.css:206) and the sibling Button uses --wk-leading-normal = 1.5 (semantic.css:222) for the same size. So the two are not just conceptually different namespaces — they resolve to different values today (20px vs 21px), and a future spacing-scale tweak would silently move tooltip typography.

In fairness this follows an existing convention in the same file (--octo-ui-tag-line-height: var(--wk-sp-5), line 6), so it is internally consistent — but if that convention is going to spread across the primitive set, --wk-leading-normal or a dedicated typography token is the safer anchor.

Scoping note for the P0 fix

When sweeping for the Semi-mock hazard described in the review, note the exposure is wider than dmworkmcp: @octo/base now imports @octo/ui at 12 call sites (Messages/File/index.tsx:9, Components/Conversation/index.tsx:83, Components/ClawInfoModal/ClawInfoModal.tsx:2, Components/FilePreviewPanel/renderers/TooltipCell.tsx:2, renderers/PdfRenderer.tsx:19, Components/ConversationList/index.tsx:18, Components/SidebarTabBar/index.tsx:2, Components/IconListItem/index.tsx:1, ui/profileDetail/ProfileOnlineStatus.tsx:2, Components/Conversation/FoldSessionCard/index.tsx:3, and others).

The full @octo/base suite is green today (357 files / 3197 tests, run locally at this head SHA), so nothing is broken there right now — but every one of those call sites is a place where a future test that mocks @douyinfe/semi-ui will quietly pull in the real Semi barrel. That is the argument for fixing this once in shared test setup rather than per file.

[yujiawei]

Code Review — PR #1478 (octo-web)

Reviewed at head SHA 45805705e1bcd6a42e3ca9fad34c8f0f5f3d6f44 (round 2).

Scope. Stacked PR based on #1439. Its own incremental work is git diff 9c541d614..45805705e — 36 files, +961/−207. The round-2 delta is the single commit 45805705e ("fix(ui): resolve tooltip review blockers"): 10 files, +98/−18. Content inherited from #1413 (Tag) and #1439 (Badge/Dot) is out of scope.

---

1. Status of the round-1 blockers

Round-1 finding	Status at this head
P0 — @octo/ui import broke the Semi mock in McpDetailModal.inlineDelete.test.tsx	✅ Fixed. pnpm --filter @dmwork/mcp test now passes 17 files / 183 tests locally, and CI Build is green.
P1 — ResizeObserver left observing a detached node after the truncation flip	✅ Fixed structurally — the rendered hierarchy no longer changes type — but delivered via a mechanism that breaks something else (see P0-1).
P1 — MCP "+N" tag chips below AA on the dark tooltip surface	✅ Fixed. #b79afa on #1c1c23 ≈ 7.4:1; on the new 18 % tinted chip fill ≈ 5.3:1. Both clear AA.
P1 — Button variant="text" unreadable on the dark surface, hover made it worse	✅ Fixed. Rest ≈ 7.4:1, hover (white) ≈ 16.9:1 — hover now increases contrast. Specificity of .octo-ui-tooltip .octo-ui-button--text (0,2,0) correctly outranks the base rule.
P2 — tooltip line-height sourced from the spacing scale	◐ Changed to a literal 1.42857143 (still 20 px at 14 px). Decoupled from the spacing scale as asked, but see P2-2.

Two of the four blockers are cleanly resolved. The third introduced a new, larger defect.

---

2. Spec compliance

Spec: ❌

• Missing work: none. Every item in the "Changes" list is present.
• Extra work: apps/web/package.json still carries the pre* lifecycle-script re-ordering. I re-verified it is a pure key re-order — parsing both revisions as JSON, the whole file compares equal, so it is behaviour-neutral. Still unrelated churn the description does not mention.
• Deviation (blocking): the description states the migration preserves "truncation behavior". At this head it does not, and the gap is now wider than in round 1: the isDisabled gate that all three overflow consumers rely on has no effect at runtime (P0-1). Non-truncated cells and blank titles will show a tooltip on hover, which is the opposite of the stated contract.

---

3. Code quality

Quality: Changes-Requested

P0-1 — visible={false} does not disable a Semi tooltip; isDisabled is now a no-op

packages/octo-ui/src/components/Tooltip/index.tsx:97,121

const isInactive = isDisabled || !hasContent;
...
  visible={isInactive ? false : undefined}

Round 1's if (isDisabled || !hasContent) return children; was removed in favour of always mounting SemiTooltip and passing visible={false} when inactive. That is not what visible does in semi-ui 2.93.0:

1. visible is not in Tooltip.defaultProps (@douyinfe/semi-ui/tooltip/index.tsx:160-186) — it is not a controlled-mode switch.
2. componentDidMount → foundation.init() → _bindEvent() → _generateEvent('hover') binds mouseEnter/focus → delayShow() unconditionally (@douyinfe/semi-foundation/tooltip/foundation.ts:88-108, 203-250).
3. show() never reads props.visible (foundation.ts:311-378). It inserts the portal and toggles internal state.
4. componentDidUpdate only acts when prevProps.visible !== this.props.visible (index.tsx:569-574). While inactive, that prop is constant, so nothing ever clamps the overlay shut.

Reproduced at runtime against the real Semi component in the repo's jsdom harness (jsdom cannot evaluate :hover, so Element.prototype.matches was stubbed for that selector only; a control case in the same harness confirms the enabled path behaves normally):

• <Tooltip content="…" isDisabled> → hovering renders <div class="semi-tooltip-content">SHOULD-NEVER-APPEAR</div>.
• <Tooltip content=""> → hovering renders <div class="octo-ui-tooltip semi-tooltip-wrapper semi-tooltip-wrapper-show"><div class="semi-tooltip-content"></div></div> — an empty dark bubble.

This is the exact failure mode the pre-migration code documented in two places, both deleted by this PR:

9c541d614:packages/dmworkbase/.../TooltipCell.tsx:37-40

// NOTE: 这里刻意使用 trigger="custom" 而非 trigger="hover"。
// hover 模式下 semi 会用内部状态挂载浮层，绕过受控的 visible，
// 当内容为空时会出现一个空的深色气泡（"乌云"）。

9c541d614:packages/dmworksummary/src/components/OverflowTooltip.tsx:17-23

// NOTE: we intentionally use trigger="custom" instead of trigger="hover".
// With trigger="hover", semi binds its own mouseenter/focus handlers that mount
// the overlay from internal state and bypass the controlled `visible` prop.

Affected call sites — all three now rely on a gate that does nothing:

• packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:53 — isDisabled={!isTruncated || !hasContent}. Every non-truncated spreadsheet cell shows a tooltip; empty cells show an empty bubble.
• packages/dmworkcontacts/src/Contacts/index.tsx:63 — isDisabled={!isTruncated}. Every contact row shows a tooltip regardless of truncation.
• packages/dmworksummary/src/components/OverflowTooltip.tsx:46 — isDisabled={!isTruncated || !title}. title === undefined yields an empty bubble.

Note on one tempting fix: switching to trigger={isInactive ? "custom" : "hover"} does not work either. Semi binds trigger events once, inside foundation.init() from componentDidMount; there is no rebind path on prop change. I verified this at runtime — a Semi Tooltip mounted with trigger="custom" and later flipped to "hover" never responds to hover again. Any trigger-based approach requires a remount, which is precisely what the round-2 change was trying to avoid.

Recommended shape:

1. Restore the early return in the primitive: if (isInactive) return children;. Suppression by not mounting is the only reliable option with this Semi version.
2. Fix the round-1 ResizeObserver staleness the way it was originally suggested — a callback ref that re-observes when the measured node identity changes — rather than by pinning the hierarchy. That fixes the stale observer without touching the mount contract:

const observerRef = useRef<ResizeObserver | null>(null);
const setNode = useCallback((node: HTMLDivElement | null) => {
  observerRef.current?.disconnect();
  ref.current = node;
  if (!node || typeof ResizeObserver === "undefined") return;
  observerRef.current = new ResizeObserver(checkTruncation);
  observerRef.current.observe(node);
  checkTruncation();
}, [checkTruncation]);

3. Add one test that hovers a disabled tooltip against the real Semi component (not a mock) and asserts no overlay — see P1-2.

---

P1-1 — Always mounting a Semi Tooltip per row/cell has real cost in virtualized views

packages/dmworkbase/.../TooltipCell.tsx:53, packages/dmworksummary/src/components/OverflowTooltip.tsx:46

Before this PR, TooltipCell and the summary OverflowTooltip returned a bare element when not truncated — zero Semi instances for ordinary cells. Now every rendered cell constructs a TooltipFoundation and, via _bindEvent() → _bindResizeEvent() → registerResizeHandler, adds its own throttled window.resize listener (@douyinfe/semi-ui/tooltip/index.tsx:387-398). TooltipCell is used for both header and body cells in ExcelRenderer.tsx:166,175 and JsonlRenderer.tsx:192,204, so a wide virtualized viewport now carries hundreds of foundations and listeners that previously did not exist, and they churn on every scroll-driven remount.

Semi also rewrites every trigger child (index.tsx:808-830): tabIndex: props.tabIndex || 0 plus aria-describedby and data-popupid. So each spreadsheet cell and contact row becomes a keyboard tab stop and advertises a described-by target that, when inactive, describes nothing. That is a new keyboard-navigation and screen-reader change, not just a perf one.

(For packages/dmworkcontacts/src/Contacts/index.tsx the always-mounted Tooltip is pre-existing, so only the tabIndex behaviour there is unchanged from before. The window.addEventListener("resize", …) inside the consumers is also pre-existing — the new cost is the per-cell Semi instance.)

Restoring the early return in the primitive removes this item along with P0-1.

---

P1-2 — The round-2 tests assert against their own mocks, so they cannot catch P0-1

packages/octo-ui/src/components/Tooltip/Tooltip.test.tsx:112-128

-  it("does not mount an overlay for disabled or blank content", () => {
-    expect(disabled).toBe("<span>Disabled</span>");
-    expect(blank).toBe("<span>Blank</span>");
+  it("keeps the trigger hierarchy stable while disabling blank content", () => {
+    expect(disabled).toContain('data-visible="false"');
+    expect(blank).toContain('data-visible="false"');

The assertion that actually protected the behaviour was deleted and replaced with one that proves only that false was forwarded to a hand-written mock (Tooltip.test.tsx:14-45). The suite also uses renderToStaticMarkup, which cannot dispatch a hover at all.

packages/dmworksummary/src/components/OverflowTooltip.test.tsx:6-13 has the same shape: the @octo/ui mock is authored to hide content when isDisabled is true — behaviour the real component does not have — so every "does not show tooltip" assertion in that file is validating the mock.

The new "keeps observing the live element after truncation toggles" case (OverflowTooltip.test.tsx:107-121) is a genuine regression test for the observer bug and is good to have. Please add its counterpart for suppression, exercising real Semi.

---

P2 — non-blocking

1. isDelayed still trips Semi's dev warning. index.tsx:116-117 sets mouseEnterDelay=300 with mouseLeaveDelay=0; componentDidUpdate warns 'mouseLeaveDelay' cannot be less than 'mouseEnterDelay' on every update of a delayed tooltip. Carried over from round 1, still unaddressed.
2. --octo-ui-tooltip-line-height: 1.42857143 (tokens.css:60) is a magic constant. It resolves to the same 20 px as before, but the sibling Button uses --wk-leading-normal (1.5). A named typography token would age better than a nine-digit literal.
3. .wk-mcp-tag-overflow still sets max-width: 320px (packages/dmworkmcp/src/index.css:844) while the wrapper caps at --octo-ui-tooltip-max-width: 320px plus var(--wk-sp-2) var(--wk-sp-3) padding. The inner box can therefore hit the wrapper's content box early and wrap sooner than intended. Worth an eyeball in a real browser.
4. apps/web/package.json churn (above) — behaviour-neutral, but unrelated to this PR.
5. Merge order. #1478, #1439 and #1413 all target main; merging this one first would land the other two along with it. The dependency order in the description needs to be enforced at merge time.

---

4. Overall verdict

CHANGES_REQUESTED

Spec ❌ (the stated "preserves truncation behavior" contract is not met) AND Quality Changes-Requested (P0-1 plus two P1s).

The direction of travel is good — the CI break is genuinely fixed, both contrast findings are fixed correctly and verifiably, and the observer-staleness diagnosis was understood. The problem is the chosen remedy: visible={false} was adopted as a suppression mechanism without checking what Semi does with it, and the tests that would have caught that were rewritten to match the assumption instead of the library. The two deleted NOTE: comments in TooltipCell.tsx and OverflowTooltip.tsx were describing this exact trap.

5. Suggested next steps

1. Restore if (isInactive) return children; in packages/octo-ui/src/components/Tooltip/index.tsx.
2. Re-fix the ResizeObserver staleness with a callback ref in all three consumers, keeping the conditional wrapper.
3. Add a hover test against the real Semi Tooltip (a jsdom suite that stubs matches(":hover") works) covering: disabled → no overlay; blank content → no overlay; enabled + truncated → overlay.
4. Optional: revisit the mouseLeaveDelay/mouseEnterDelay warning and the literal line-height.

6. Additional observations

• Local test results at this head: @octo/ui 5 files / 28 tests pass and typecheck passes; @dmwork/mcp 17 files / 183 tests pass; @octo/base 357 files / 3197 tests pass; @octo/contacts 9 files / 38 tests pass. @dmwork/summary is red at 9 files / 50 tests, unchanged from the parent — the failing files are SummaryCard.test.tsx, SummaryEditor.test.tsx, ChatSelectorModal.test.tsx and siblings; OverflowTooltip.test.tsx is not among them. That package's suite is red on main today and CI only gates one file from it, so the breakage stays invisible and keeps growing. Still worth a separate ticket.
• Advisory coverage: two independent second-opinion passes ran alongside this review; both completed. Both independently reached the same conclusion on P0-1 from the semi-ui source, and both flagged the mock-only tests (P1-2) and the always-mount cost (P1-1). One of them proposed trigger={isInactive ? "custom" : "hover"} as the fix; I rejected that after verifying at runtime that Semi does not rebind trigger events after mount. One rated the per-consumer window.resize listener as newly introduced; that listener is pre-existing at 9c541d614, so it is downgraded here. Neither pass, and not this review, could verify: real-browser rendering (Semi's autoAdjustOverflow flip behaviour, wrapper padding vs .semi-tooltip-content padding, overlay stacking inside ClawInfoModal), Storybook visual output in light and dark themes, or the practical scroll performance of the excel preview under the new per-cell Tooltip load. Those still need a human pass.

[Jerry-Xin]

Code Review — PR #1478 (round 2)

Reviewed at head 45805705e1bcd6a42e3ca9fad34c8f0f5f3d6f44. Incremental scope 9c541d614..45805705e (36 files, +961/−207); round-2 delta is the single commit 45805705e (10 files, +98/−18). Content inherited from #1413/#1439 out of scope.

Verdict: REQUEST_CHANGES

The round-1 ResizeObserver remount defect is structurally fixed and the test-environment break is fixed — but the replacement isDisabled gate that all three overflow consumers now rely on is inert at runtime: visible={false} does not stop a semi-ui 2.93.0 tooltip from showing on hover under the default hover trigger. Net effect: every non-truncated cell/row shows a tooltip on hover, and blank content shows an empty dark bubble — regressing the truncation-only contract the PR states it preserves.

🔴 Blocking

🔴 B1 — isDisabled / blank-content suppression does nothing at runtime

packages/octo-ui/src/components/Tooltip/index.tsx:97,121. Round 1's early return (if (isDisabled || !hasContent) return children;) was replaced with an always-mounted SemiTooltip plus visible={isInactive ? false : undefined}. Byte-verified against the semi-ui 2.93.0 installed in this repo:

• trigger defaults to 'hover' (@douyinfe/semi-ui/lib/es/tooltip/index.js:725); the primitive passes no trigger prop.
• foundation.init() → _bindEvent() binds mouseEnter → delayShow() unconditionally (@douyinfe/semi-foundation/lib/es/tooltip/foundation.js:223-229). Only trigger="custom" skips binding — the foundation comment says controlled visible was designed for that mode ("show/hide completely depend on props.visible", foundation.js:384-386).
• delayShow() → show(), and show() never reads props.visible (foundation.js:47-58, 59-123) — it reads internal state and inserts the portal.
• componentDidUpdate clamps only when the prop value changes (prevProps.visible !== this.props.visible, tooltip/index.js:594-598). While inactive, visible={false} is constant from mount, so nothing ever clamps the overlay shut; _shouldShow() at init only acts when the prop is truthy (foundation.js:402-409).

Consequences at this head — all three consumers gate through the inert prop:

• packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:53 — isDisabled={!isTruncated || !hasContent}: every non-truncated spreadsheet cell shows a tooltip on hover; empty cells mount an empty dark bubble.
• packages/dmworkcontacts/src/Contacts/index.tsx:63 — isDisabled={!isTruncated}: every contact row shows a tooltip regardless of truncation.
• packages/dmworksummary/src/components/OverflowTooltip.tsx:46 — isDisabled={!isTruncated || !title}: title === undefined yields an empty bubble.

The empty-bubble case is exactly the failure mode the pre-migration trigger="custom" comments documented — both deleted by this PR. The new unit tests mask the defect: Tooltip.test.tsx asserts data-visible="false" on a mocked Semi (static markup only), and OverflowTooltip.test.tsx mocks @octo/ui itself — neither exercises the real component's hover path.

This independently confirms yujiwei's round-2 P0-1 (review 4979881490, incl. a runtime repro); my finding is based on a byte-level read of the installed semi-ui source.

Fix direction: with this Semi version, suppression by not mounting is the only reliable mechanism. Either restore the primitive's early return and fix the observer staleness with a callback ref that re-observes when the measured node identity changes (hierarchy may change — the observer follows the live node), or keep the stable hierarchy but gate so no hover binding exists while inactive. Add one test that hovers a disabled/blank tooltip against the real Semi component and asserts no overlay mounts. Note: flipping trigger between custom and hover after mount does not work — Semi binds trigger events once in componentDidMount, there is no rebind path.

Round-1 items — status at this head

• ✅ ResizeObserver stranded on a detached node (my round-1 blocker; yujiwei P1-1; mochashanyao P2): structurally fixed — the rendered root type no longer changes with the truncation toggle (always <Tooltip isDisabled=…>), so the measured node cannot be remounted by the toggle and the observer stays attached to the live node. The new OverflowTooltip.test.tsx is a genuine regression test: it installs a MockResizeObserver global (so jsdom's missing RO no longer short-circuits), then drives triggerResize(el) — the observer callback, not window.resize — on the post-toggle live element and asserts isTruncated updates in both directions; the old code strands the observer on the detached node and fails it. (The behavior is still wrong at runtime via B1, but the observer defect itself is gone.)
• ✅ Test-environment break (yujiwei round-1 P0): fixed — vi.mock("@octo/ui") stub added to McpDetailModal.inlineDelete.test.tsx (same pattern as the three sibling files). pnpm --filter @dmwork/mcp test at this head: 17 files / 183 tests pass; CI Build is green on this head. Note: the underlying dual-path resolution persists — node_modules/.pnpm still holds three physical copies of @douyinfe/semi-ui@2.93.0 at this head, so any existing test that mocks @douyinfe/semi-ui will silently load the real barrel once its import graph reaches @octo/ui (12 @octo/base call sites, per yujiwei's addendum). Per-file stubs fix today's break; a shared setup stub or Semi dedupe remains the durable fix — non-blocking here since nothing currently fails.
• ✅ MCP "+N" chips below AA on the dark surface (yujiwei P1-2): fixed — new .octo-ui-tooltip .wk-mcp-tag-overflow .wk-mcp-tag--accent rule recolors chips to --octo-ui-tooltip-action-color (--wk-purple-300 = #b79afa) with an 18% tinted fill; ≈ 7.3:1 on #1c1c23, clears AA.
• ✅ Button variant="text" unreadable on the dark surface (yujiwei P1-3): fixed — .octo-ui-tooltip .octo-ui-button--text scoped override, rest #b79afa (≈ 7.3:1), hover white (≈ 16.9:1); hover now increases contrast.
• ✅ Line-height from spacing scale (yujiwei P2): --octo-ui-tooltip-line-height is now the unitless 1.42857143 — decoupled from the spacing scale.

💬 Non-blocking

1. Always-mounted Semi instances in virtualized grids (yujiwei round-2 P1-1): every rendered cell now constructs a TooltipFoundation and binds trigger events at mount; TooltipCell is used for all header/body cells in ExcelRenderer.tsx:166,175 and JsonlRenderer.tsx:192,204, so wide viewports carry hundreds of Semi instances that did not exist pre-PR. A callback-ref / conditional-mount fix for B1 would also restore the zero-instance baseline.
2. packages/dmworksummary/src/components/OverflowTooltip.tsx:34 — effect deps [children, title]; children has fresh identity every render, so the ResizeObserver is torn down and rebuilt on each render. Harmless now (stable node) but wasteful; a callback ref resolves it.
3. Blank guard stays shallow (hasNodeContent passes true, [], <></>); this matters again once B1 restores unmount-based suppression.
4. apps/web/package.json pre* script re-order churn remains — behaviour-neutral (both revisions parse to identical key/value maps) but undisclosed in the description.

Verification performed at this head

• Tests (local): @octo/ui 5 files / 28 pass; @dmwork/mcp full 17 files / 183 pass (incl. the round-1 failing file); @octo/contacts 9 files / 38 pass; dmworksummary OverflowTooltip 1 file / 8 pass (incl. the new RO regression test); dmworkbase FilePreviewPanel slice 3 files / 47 pass; base Badge slice 3 files / 19 pass.
• Stacked deps: #1413 head (ca108432df43) and #1439 head (9c541d614) are exact ancestors of this head — no drift; both still need to merge first.
• Round-1 verified-good items unchanged: portal to document.body, z-index 1060 > modal 1000, no leftover Semi imports at the 12 migrated consumers, a11y attributes inherited from Semi.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).

---

Code Review — PR #1478 (octo-web)

Reviewer: Octo-Q (automated review)
Reviewed at head 45805705e1bcd6a42e3ca9fad34c8f0f5f3d6f44 — round 2, after fix commit 45805705e ("fix(ui): resolve tooltip review blockers").

Scope note. Stacked PR: base main shows 127 files, but this PR's own incremental work is 9c541d614..HEAD (36 files, +961/-207). Everything below 9c541d614 is byte-identical by commit identity to the approved stacked dependencies (#1413 Tag @ ca108432, #1439 Badge/Dot @ 9c541d61) and is out of scope. This round's delta is ecd72ef4a..45805705e (10 files, +98/-18), concentrated exactly on the round-1 blocker sites.

Summary

The PR adds a shared portal-based Tooltip primitive to @octo/ui (Semi-backed, tokens/exports/CSS scoped) and migrates 12 consumers. Round 1 requested changes for one blocker: the overflow-tooltip consumers conditionally swapped their root between a bare element and <Tooltip>, so React remounted the measured node while the ResizeObserver stayed attached to the detached one.

The fix commit resolves that staleness the right way — the wrapper is now always mounted, so the measured node's identity is stable — but its replacement gating mechanism (visible={isInactive ? false : undefined}) does not actually disable the tooltip under Semi's real hover semantics. The result is a new user-visible regression on the exact surfaces the blocker fix touched: hovering or focusing a non-truncated cell now shows a tooltip, and empty-content cells show an empty dark bubble. One P1 below.

Verification

• ✅ Static review of all 36 files in the incremental diff at head 45805705e; this-round delta inspected line-by-line.
• ✅ Pinned-dependency source trace: read @douyinfe/semi-ui@2.93.0 / @douyinfe/semi-foundation@2.93.0 (the exact versions locked in pnpm-lock.yaml) — visible prop lifecycle (componentDidUpdate is the only post-mount consumer), hover/focus event binding (unconditional), show() (never reads props.visible), render() cloneElement/ref chaining, renderPortal() className merge.
• ✅ Empirical probe (jsdom + vitest, isolated environment, exact pinned semi-ui 2.93.0): mounting <Tooltip visible={false}> and firing mouseEnter inserts the portal content anyway (shows=true); the false → undefined transition works; the uncontrolled sanity check passes. Probe removed afterwards.
• ✅ R6 diff-scope re-verification of the round-1 blocker against both ecd72ef4a and 45805705e.
• Static analysis only at head 45805705e; build and repo test suites were not executed in this environment (the probe exercised the pinned dependency, not PR code).

Findings

One P1 blocker, one P2, one Nit.

P1 — visible={false} does not suppress hover/focus: "disabled" tooltips still show (packages/octo-ui/src/components/Tooltip/index.tsx:121)

The fix gates inactive tooltips with visible={isInactive ? false : undefined}. In semi-ui 2.93.0 this does not disable anything: hover/focus handlers are bound unconditionally at mount (semi-foundation/tooltip/foundation.ts _generateEvent('hover') → mouseEnter → delayShow()), and show() never consults props.visible — the prop is only read at init and on change in componentDidUpdate. Empirically confirmed against the pinned build: <Tooltip visible={false}> + mouseEnter renders the portal content.

Consequence in all three overflow consumers — packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:53, packages/dmworkcontacts/src/Contacts/index.tsx:63, packages/dmworksummary/src/components/OverflowTooltip.tsx:46: hovering/focusing a non-truncated cell now shows a tooltip (redundant bubble over fully visible text), and cells with empty content (hasContent=false / !title) show an empty dark bubble. Notably, the migration deleted the original TooltipCell NOTE comment that documented exactly this Semi behavior — "hover 模式下 semi 会用内部状态挂载浮层，绕过受控的 visible，当内容为空时会出现一个空的深色气泡" — and its trigger="custom" countermeasure; the fix re-introduced the bypassed-controlled-visible pattern under default trigger="hover".

Diff-scope: new — introduced by fix commit 45805705e (at ecd72ef4a, inactive cells rendered bare children with no wrapper, so no bubble). It makes a previously working path worse in production (every hover in the file-preview grid, contacts list, summary overflow text). Fix direction: do not gate via visible={false}. Either (a) restore conditional wrapping (bare children when inactive) and fix the stale observer with a callback ref that re-observes when the measured node's identity changes, or (b) keep the stable wrapper and make activation fully controlled (trigger="custom" + consumer-driven visible state). Note a dynamic trigger prop swap will not work — Semi binds trigger events only at mount. Add an integration test using the real Semi Tooltip asserting hover-when-inactive inserts no portal.

P2 — Regression tests mock away the layer where the defect lives (packages/dmworksummary/src/components/OverflowTooltip.test.tsx:6)

The new MockResizeObserver test is a good regression test for the round-1 blocker, but the @octo/ui Tooltip stub re-implements isDisabled semantics itself, and packages/octo-ui/src/components/Tooltip/Tooltip.test.tsx:112 only asserts that the visible prop is forwarded to a mocked Semi. The suite is green while the real Semi shows through visible={false} on hover — the changed high-risk contract (inactive gating) is untested on its production path. Add at least one test that mounts the real Semi Tooltip (deep-import @douyinfe/semi-ui/lib/es/tooltip bypasses the unrelated tiptap resolution issue) and asserts no portal appears when hovering an inactive tooltip.

Nit — Delayed-tooltip dev warning persists (packages/octo-ui/src/components/Tooltip/index.tsx:117)

mouseEnterDelay={300} with mouseLeaveDelay={0} trips Semi's dev-console warning ("mouseLeaveDelay cannot be less than mouseEnterDelay") on every update of a delayed tooltip (semi-ui tooltip/index.tsx componentDidUpdate). Functionally harmless (both paths clear the delay timer), but worth a compliant leave delay or a code comment. Round-1 carryover, unaddressed.

Things I checked that are fine

• ✅ Round-1 blocker (stale ResizeObserver): genuinely fixed — wrapper always mounted, measured-node identity stable (Semi renders children in place via cloneElement; span-wrapping only triggers for disabled/non-element children, which none of the three consumers hit), the effect's observer keeps watching the live node, and Semi's callback-ref chain preserves the consumer ref.
• ✅ The other 9 migrated sites pass no isDisabled → visible={undefined} → uncontrolled, behavior unchanged from round 1.
• ✅ Portal className merge: renderPortal() applies the caller className to the portal wrapper, so .octo-ui-tooltip .wk-mcp-tag-overflow .wk-mcp-tag--accent (new rule in packages/dmworkmcp/src/index.css:848) styles the portaled tag cluster correctly; target classes exist (McpCard.tsx:180-182).
• ✅ apps/web/package.json change is a pure reordering of the seven pre* scripts (no add/remove); pnpm-lock.yaml/package.json additions match the new @octo/ui dependency in dmworkcontacts; exports/index.d.ts/components.css wiring consistent; Button text variant BEM-scoped and tested.
• ✅ No leftover semi-ui Tooltip imports outside the wrapper itself and the intentionally-unmigrated SpaceList/ThreadList.
• ✅ tokens.css: --octo-ui-tooltip-line-height moved from a spacing token to unitless 1.42857143 (round-1 P2 resolved); dark-fixed surface contract unchanged.

Verdict: CHANGES_REQUESTED

The round-1 blocker is fixed correctly (stable hierarchy, live-node observation), but the replacement gating mechanism is ineffective under Semi's real hover semantics, shipping a new user-visible regression (unwanted tooltips on non-truncated cells, empty bubbles on empty content) on three production surfaces. Replacing the visible={false} gate with one of the two approaches above plus one real-Semi integration test is sufficient to clear this; the P2 and Nit are non-blocking.

---

附录 — Octo 专项段（供终审参考）

数据流回溯（每个被消费数据 → 上游来源 → 是否真流到消费点）

1. visible prop ← isInactive ← isDisabled（三个 overflow 消费方传 !isTruncated [|| !hasContent/!title]）|| !hasContent。消费点本应是 Semi 的 show 门控，但 Semi 只在 componentDidUpdate/_shouldShow 读取该 prop，hover show 路径不读 → 数据未流到真正的 show 门控（P1；jsdom 实测证实 hover 穿透）。
2. isTruncated ← RO/window-resize 回调经 ref.current.scrollWidth>clientWidth。ref 经 Semi cloneElement callback-ref 链保留（render() 源码证实），且节点身份现已稳定 → 测量数据真的流到消费点（round-1 blocker 已修）。
3. content（TooltipCell=单元格内容 / OverflowTooltip=title / Contacts=text）→ hasNodeContent 短路空值 → isInactive。空内容路径：visible={false} 但 hover 仍 show → 空气泡（P1 的一部分）。
4. tokens（--octo-ui-tooltip-*）← tokens.css :root（dark 块无 override = 双主题固定深色面）→ Tooltip/index.css var() 消费，链路完整；line-height 已改无单位数值。
5. McpCard overflow tag 簇 JSX → portal → 新 CSS 选择器经 renderPortal className 合并生效（源码证实 propClassName 并入 portal wrapper）。
6. Conversation participants ← 既有 state，数据源未变，仅 CSS-hover 气泡换成 isDelayed（300ms/0ms 语义与原 CSS transition-delay 0.3s/0s 对齐）。

盲点 checklist（C1–C6）

• C1 双路径 parity：HIT → 产出 P1。active↔inactive 门控不对称：hide 腿有效（componentDidUpdate visible 变化 → delayHide），show-suppress 腿无效（hover 事件无条件绑定、show() 不读 prop）。show/hide 计时独立无 latch（foundation 源码复核）；CSS 删除↔新增成对核验（wk-mcp-tooltip-light 块删除 ↔ .octo-ui-tooltip .wk-mcp-tag-overflow 规则新增，无孤儿引用）。
• C2 control-flow ordering / 复用：HIT → clear（附 P1 关联说明）。共享 Tooltip 12 处复用，仅 3 处 overflow 传 isDisabled（grep 全仓证实）；未使用动态 trigger 切换，且已核实 Semi 只在 mount 绑定 trigger 事件（防止后续错误修法）。非规范 content 试穿：0 会渲染；""/空白/null/false → inactive —— 但 inactive 门控本身失效（P1）。
• C3 授权边界：N/A — 纯展示层，无 endpoint/tool/凭证改动。
• C4 授权生命周期/容器级联：N/A — 同上。
• C5 build 通过 ≠ 运行期正确：HIT。本环境未跑 build/仓库测试（诚实声明）；对锁定版本 semi-ui 2.93.0 做了运行期推演 + jsdom 实测探针，证明单测全绿（mock 掉了出问题的层）而运行期行为破坏 —— 正是 C5 要防的形态。
• C6 治理/策略文档自洽：N/A — 非治理/文档类 PR。

跨轮 blocker 复检（R6）

上轮（round 1，Jerry-Xin 的 CHANGES_REQUESTED @ 04:34Z）blocker 逐条复核：

• 🔴 stale ResizeObserver（TooltipCell / Contacts / OverflowTooltip 翻转后观察已 detach 节点）：已修。证据：wrapper 常驻（index.tsx:121 + 三消费方改为恒包裹 + isDisabled），节点身份稳定，RO 持续观察活节点；新增回归测试覆盖翻转路径（但受 P2 的 mock 局限）。
• 非阻塞 note（mouseEnterDelay/mouseLeaveDelay dev warning）：未修，仍存（Nit，不阻塞）。
• 非阻塞 note（showArrow/position 移除为设计意图）：round 1 已确认 intentional。
• ⚠️ 本轮 fix 引入新 P1（上无下有新）→ 整体 verdict 仍 CHANGES_REQUESTED。这是该 PR 第 2 轮：若下一轮修复仍在"包裹 vs 裸渲染 vs visible 门控"之间来回切换，建议对 inactive 路径做一次整体设计重审，避免逐轮打地鼠。

额外发现（非 finding）

• fix commit 顺手修复了 McpDetailModal.inlineDelete.test.tsx（补 @octo/ui mock）——该测试在 round-1 迁移 commit（40445bd66）时即被打破（同样缺 mock 的 McpOfficialPublisher.test.tsx 在 commit 1 内已修）；HEAD 净状态已修复，记录在案。
• actions 内容槽（含新 action-color token / text-button 配色）目前仅 Storybook stories 消费，无生产调用方 —— 前瞻契约，无碍。
• 审查基线说明：本审查按 stacked 真实范围 9c541d614..45805705e 执行；对锁定依赖 semi-ui 2.93.0 的源码阅读与 jsdom 探针在隔离环境完成（未安装任何依赖、未改动 PR checkout），探针文件用后即删。

[Octo-Q] verdict: REQUEST_CHANGES — 存在 1 个 P1（visible={false} 门控被 Semi hover 语义穿透，三个 overflow 面出现多余/空气泡，系本轮 fix commit 新引入，使原本工作的路径变糟，符合 R1 阻塞定义）+ 1 个 P2（回归测试 mock 掉出问题层）+ 1 个 Nit；round-1 的 RO blocker 本身已正确修复。建议终审维持 CHANGES_REQUESTED，修复方向见 P1 段。

[yujiawei]

Correction to my review — there are two valid fixes, not one

In the P0-1 section I wrote that "suppression by not mounting is the only reliable option with this Semi version." That is stronger than my evidence supported, and I want to correct it before it narrows your options.

A statically set trigger="custom" with consumer-driven visible state also works. I verified both halves against the pinned semi-ui 2.93.0 in a jsdom harness:

• <SemiTooltip trigger="custom" visible={false}> + mouseEnter → no overlay. Hover is genuinely suppressed, because _generateEvent('custom') binds no mouseEnter/focus handler at all.
• Same tooltip re-rendered with visible={true} → overlay appears. So the controlled path still opens on demand.

That is precisely what TooltipCell, the contacts OverflowTooltip, and the summary OverflowTooltip did before this PR, and it is why their now-deleted NOTE: comments specified trigger="custom".

So both of these are sound:

• (a) Restore if (isInactive) return children; in the primitive, and fix the observer staleness with a callback ref that re-observes when the measured node's identity changes.
• (b) Keep the always-mounted wrapper, but expose trigger on the primitive and set it to "custom" for inactive tooltips, driving visibility from consumer state.

What still does not work is swapping trigger after mount — Semi binds trigger events once, in foundation.init() from componentDidMount, with no rebind path. I confirmed that separately: a tooltip mounted as trigger="custom" and later flipped to "hover" never responds to hover again. So under (b) the trigger has to be fixed for the lifetime of the instance, which in practice means driving visibility entirely from consumer state rather than from Semi's hover.

One tradeoff worth weighing if you pick (b): it keeps a Semi TooltipFoundation — and its own throttled window.resize listener, plus the injected tabIndex={0} / aria-describedby on the trigger — mounted for every rendered cell. For TooltipCell that is a change from the pre-PR behaviour, which had a zero-instance fast path for non-truncated cells, and it is used for both header and body cells in ExcelRenderer and JsonlRenderer. The contacts and summary consumers already always-mounted before this PR, so (b) is closer to status quo for those two. That is the P1-1 consideration in my review, not a blocker on its own — but it is the reason I'd lean toward (a) for TooltipCell specifically.

The verdict is unchanged: CHANGES_REQUESTED, on P0-1 as stated.

[Jerry-Xin]

✅ APPROVE (round 3). The round-2 blocker B1 (inert visible suppression) is genuinely fixed, verified byte-level against semi-ui 2.93.0 vendor source and with real (un-mocked) interaction tests.

B1 fix verification

The fix restores suppression by NOT mounting the Semi overlay at all instead of relying on props.visible:

• packages/octo-ui/src/components/Tooltip/index.tsx:99 — if (isInactive) return children; (isInactive = isDisabled || !hasContent); the dead visible={isInactive ? false : undefined} binding is removed.
• All three overflow consumers gate the wrapper itself: packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:61 (if (!isTruncated || !hasContent) return cellContent;), packages/dmworkcontacts/src/Contacts/index.tsx:76, packages/dmworksummary/src/components/OverflowTooltip.tsx:58.

This is the correct mechanism: in semi-ui 2.93.0, controlled visible is only consumed with trigger="custom" (semi-foundation tooltip/foundation.ts: "show/hide completely depend on props.visible" applies to the custom-trigger path only; the hover trigger's show() never reads props.visible, and componentDidUpdate clamps only on value change — a constant false never fires it). Not mounting the component cannot be bypassed by any trigger path. Empty-content cells can no longer produce empty dark bubbles (third B1 symptom) — blank guard at the shared layer plus hasContent guards in consumers.

Integration test (would have caught B1)

packages/octo-ui/src/components/Tooltip/Tooltip.integration.test.tsx renders REAL Semi (no vi.mock), drives actual mouseEnter interactions, and asserts .semi-tooltip-content stays absent for isDisabled and blank content while appearing for enabled content. Against the round-2 shape this fails by construction: hover would still invoke show() despite the constant visible={false}.

ResizeObserver fix coexistence (round-2 item preserved)

Conditional mounting reintroduces a remount of the measured node when truncation toggles, so the observer lifecycle is now bound to the node via callback refs (setCellRef / setTextRef / setContainerRef): React detaches the old observer and re-observes the live node on every mount/unmount. The rewritten regression test asserts the observer is attached to the post-toggle live node. Negative control: running the new test against the round-2 component code fails with expected +0 to be 1 (observer stranded on the detached node) — the test genuinely guards the defect. Also verified semi-ui wraps children only when disabled/loading (wrapWhenSpecial/isSpecial), so plain div/span triggers are cloned without an extra wrapper element — no layout feedback on the measured node.

Tests at this head (run locally, deps installed)

• octo-ui Tooltip: 2 files / 7 passed (incl. 2 real-Semi integration tests)
• dmworksummary OverflowTooltip: 8/8
• dmworkbase FilePreviewPanel slice: 3 files / 47 passed
• dmworkcontacts: 9 files / 38 passed
• dmworkmcp full: 17 files / 183 passed (round-2 test-env fix intact)

Round-2 verified items: no regression

Portal rendering and z-index config untouched (the only index.tsx changes are the early return and the removed visible prop); contrast/line-height fixes unchanged; deps #1413/#1439 remain exact ancestors — merge order still applies.

Non-blocking (carried)

• Blank guard is trim()-based; zero-width characters (e.g. U+200B) still count as content — minor.
• Per-cell Semi foundation instances in virtualized Excel/Jsonl grids are now bounded by visible truncated cells only (inactive cells mount no Semi at all), a material reduction vs round 2; instances are still created/destroyed as truncated cells scroll in/out — inherent to the pattern.
• window.resize listener plus ResizeObserver per overflow item is redundant (observer suffices) — packages/dmworkcontacts/src/Contacts/index.tsx:57, packages/dmworksummary/src/components/OverflowTooltip.tsx:42.
• apps/web pre* script additions are intentional build-order guards.

[yujiawei]

Code Review — PR #1478 (octo-web)

Reviewed at head SHA f03f17b2e534295128262f5bb387e2b1e0082ff3 (round 3).

Scope. Stacked PR based on #1439. Its own incremental work is git diff 9c541d61..f03f17b2 — 37 files, +1094/−216. The round-3 delta is the single commit f03f17b2 ("fix(ui): restore tooltip suppression behavior"): 7 files, +189/−65. Content inherited from #1413 (Tag) and #1439 (Badge/Dot) is out of scope; I re-confirmed both dependency heads (ca108432, 9c541d61) are still exact ancestors of this head, so there is no drift.

---

1. Status of the previous blockers

Blocker	Status at this head
R2 — visible={false} does not disable a Semi tooltip, so isDisabled was a no-op (tooltips on non-truncated cells, empty dark bubbles on blank content)	✅ Fixed. packages/octo-ui/src/components/Tooltip/index.tsx:99 restores if (isInactive) return children; and the visible prop is gone from the SemiTooltip call (index.tsx:113-124). Verified at runtime against real Semi, see §2.
R2 — a Semi TooltipFoundation mounted for every rendered cell/row	✅ Fixed. Conditional mount restored in all three consumers: TooltipCell.tsx:64, dmworkcontacts/src/Contacts/index.tsx:76, dmworksummary/src/components/OverflowTooltip.tsx:58. Contacts is now better than its pre-PR baseline, which always mounted.
R2 — the round-2 tests asserted against their own mocks, so they could not catch the defect	✅ Fixed. packages/octo-ui/src/components/Tooltip/Tooltip.integration.test.tsx (new, 84 lines) mounts the real Semi component under jsdom and covers disabled / blank / enabled hover. Tooltip.test.tsx:109-122 also reverts to the meaningful expect(disabled).toBe("<span>Disabled</span>") assertion.
R1 — ResizeObserver left observing a detached node after the truncation flip	✅ Fixed the recommended way — callback refs that disconnect and re-observe on node-identity change: TooltipCell.tsx:31-41, Contacts/index.tsx:47-57, OverflowTooltip.tsx:23-33. Verified at runtime, see §2.
R1 — @octo/ui import broke the Semi mock in McpDetailModal.inlineDelete.test.tsx	✅ Still fixed: @dmwork/mcp 17 files / 183 tests pass.

Both round-1 and round-2 blockers are now resolved, and — importantly — resolved without re-introducing each other. That was the trap this PR bounced between for two rounds.

---

2. Runtime verification

Static reading is what got the last two rounds wrong, so I exercised the production code paths against the real, pinned semi-ui@2.93.0 in the repo's own jsdom harnesses (Element.prototype.matches stubbed for ":hover" only; probe files deleted afterwards, working tree left clean).

TooltipCell (dmworkbase), real Semi:

• non-truncated cell + mouseEnter → no .semi-tooltip-content. ✅
• whitespace-only content (" ") + mouseEnter → no overlay, no empty bubble. ✅
• truncated cell + mouseEnter → overlay with the exact cell text; then un-truncating the live (remounted) node hides it again, with exactly one live ResizeObserver attached throughout. ✅

That third case is the decisive one: it proves the observer follows the measured node across the wrap/unwrap remount, which is what round 1 broke and what round 2 "fixed" by a mechanism that broke something larger.

OverflowTooltip (dmworksummary), real @octo/ui + real Semi (its own suite mocks @octo/ui, so I re-ran it unmocked):

• not truncated → no overlay; truncated → overlay with the exact title; observer follows the live node and un-truncating hides it. ✅
• truncated but title undefined → no overlay. ✅

The contacts OverflowTooltip (Contacts/index.tsx:35-77) is structurally identical to the summary one; it is not separately module-exported, so I did not probe it in isolation.

I also confirmed from the installed dependency why the callback-ref approach works at all: Semi rebuilds its trigger element with cloneElement and explicitly chains the child's ref — @douyinfe/semi-ui/lib/es/tooltip/index.js:650-655, const ref = getRef(children); if (typeof ref === 'function') { ref(node); }. So setCellRef is still invoked when the cell is wrapped.

---

3. Spec compliance

Spec: ✅

• Missing work: none. Every item in the "Changes" list is present, and the three round-2 fix directions were all followed.
• Extra work: apps/web/package.json still carries the pre* lifecycle-script re-ordering. I re-verified it independently — parsing both revisions as JSON, the whole file compares equal and only key order differs, so it is behaviour-neutral. It remains undisclosed churn, but it adds and removes nothing.
• Deviation: none. The description's "preserving ... truncation behavior" contract is the one that failed in round 2; at this head it holds, and §2 is the evidence.

---

4. Code quality

Quality: Approved

No P0 or P1 findings. Everything below is P2 or a nit and none of it should hold up the merge.

P2-1 (new) — the ResizeObserver is rebuilt on every re-render while a cell is in the wrapped state

packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:31-41

const setCellRef = useCallback((node: HTMLDivElement | null) => {
  resizeObserverRef.current?.disconnect();
  ...
  resizeObserverRef.current = new ResizeObserver(checkTruncation);
  resizeObserverRef.current.observe(node);
}, [checkTruncation]);

setCellRef itself is stable, so in the unwrapped state there is no churn. But once the cell is wrapped, Semi builds a fresh inline ref arrow on every render (@douyinfe/semi-ui/lib/es/tooltip/index.js:656) and chains through to ours, so React detaches and re-attaches setCellRef each render — i.e. disconnect() + new ResizeObserver() + observe() every time.

Measured in the harness: mount + first wrap → 3 observer constructions; 3 further content re-renders → 5 more (8 total). No leak — exactly one live observer at all times, and no render loop. Just avoidable allocation on the hot path, where TooltipCell renders for both header and body cells (ExcelRenderer.tsx:166,175, JsonlRenderer.tsx:192,204).

Cheap fix: keep an observedNodeRef and early-return from the callback when node === observedNodeRef.current.

P2-2 (new) — truncation is no longer re-measured at hover time

All three consumers previously measured scrollWidth > clientWidth live inside onMouseEnter:

• 9c541d61:packages/dmworkbase/.../TooltipCell.tsx:41-46
• 9c541d61:packages/dmworksummary/src/components/OverflowTooltip.tsx:24-29
• 9c541d61:packages/dmworkcontacts/src/Contacts/index.tsx (onEnter)

At this head the only inputs are the ResizeObserver, the window.resize listener, and the content-change effect. ResizeObserver watches the element's box, not its scrollWidth, so an overflow change that does not resize the node will not refresh isTruncated — most realistically a webfont swapping in after first paint, or nested content changing intrinsic width inside a fixed-width container. Consequence: a genuinely truncated cell can stay tooltip-less until the next resize.

Low likelihood, and a one-line belt-and-braces fix exists (onMouseEnter={checkTruncation} on the measured node). Flagging it because it is a real, deliberate narrowing of the trigger surface relative to the code being replaced. A second-opinion pass reached this same conclusion independently.

P2-3 (carryover, third round) — isDelayed still trips Semi's dev warning

packages/octo-ui/src/components/Tooltip/index.tsx:118-119 sets mouseEnterDelay={300} with mouseLeaveDelay={0}. componentDidUpdate warns unconditionally on that combination — @douyinfe/semi-ui/lib/es/tooltip/index.js:593, warning(this.props.mouseLeaveDelay < this.props.mouseEnterDelay, ...). Console noise only, but it has now survived three rounds; either give it a compliant leave delay or leave a comment saying the warning is knowingly accepted.

P2-4 (carryover) — --octo-ui-tooltip-line-height: 1.42857143

packages/octo-ui/src/styles/tokens.css:60. Decoupled from the spacing scale as asked, but a nine-digit literal where the sibling Button uses --wk-leading-normal. A named typography token would age better.

P2-5 (carryover) — redundant inner cap on the MCP tag cluster

packages/dmworkmcp/src/index.css:844 sets max-width: 320px on .wk-mcp-tag-overflow, while the wrapper is box-sizing: border-box with max-width: var(--octo-ui-tooltip-max-width) (also 320px) plus var(--wk-sp-2) var(--wk-sp-3) padding (packages/octo-ui/src/components/Tooltip/index.css:2-5). The wrapper's content box is therefore always the binding constraint and the inner cap never applies. Harmless, but dead.

P2-6 (carryover) — apps/web/package.json script re-order

Behaviour-neutral (verified, §3), but unrelated to this PR and unmentioned in the description.

Nits

1. Tooltip.integration.test.tsx:20-22 installs the canvas getContext stub in beforeAll, but afterEach runs vi.restoreAllMocks(), which restores it too — so only the first test in the file has it. It passes today because the stub is only needed while the module is being imported, but the pairing is accidental rather than intentional.
2. Tooltip.integration.test.tsx:5 imports act from react-dom/test-utils, which React 18.3 deprecates and React 19 removes. act is exported from react itself in 18.3.
3. The primitive's isDisabled prop now has zero production callers — a repo-wide grep finds it only in tests and stories, since all three overflow consumers gate by conditional rendering. Fine to keep as part of the primitive's contract; noting it so nobody assumes it is load-bearing.
4. packages/dmworkmcp/src/components/McpCard.tsx dropped mouseEnterDelay={100} in the migration (no isDelayed → 0 ms). Small unannounced UX change; probably intended, worth a line in the description.

---

5. Overall verdict

APPROVE

Spec ✅ and Quality Approved. The round-2 regression is genuinely gone on the production path — not just in a test that agrees with the implementation — and the round-1 observer defect is fixed by the mechanism that was actually recommended, with a real-Semi test now guarding both. What remains is a handful of P2s, three of which are carryovers that were never blocking.

Worth saying plainly: this converged in three rounds because each round's fix was checked against the library's real behaviour instead of its documented prop names. The two deleted NOTE: comments in the pre-migration TooltipCell and OverflowTooltip were describing exactly the trap round 2 fell into — if the new primitive ever grows a "keep it mounted but suppressed" mode, that history is worth re-reading first.

6. Suggested follow-ups (none blocking)

1. Skip the disconnect/observe cycle when the ref callback receives the same node (P2-1).
2. Add onMouseEnter={checkTruncation} on the measured node as a cheap freshness guarantee (P2-2).
3. Settle the mouseEnterDelay/mouseLeaveDelay warning one way or the other (P2-3).
4. Separate ticket: the @dmwork/summary suite is red on main today (see §7), which is why an unrelated breakage there would be invisible.

7. Verification performed at this head

• Installed the pinned dependency set (pnpm install --frozen-lockfile; lockfile already up to date).
• @octo/ui: 6 files / 30 tests pass (including the new real-Semi integration file); typecheck clean.
• @dmwork/mcp: 17 files / 183 tests pass.
• @octo/contacts: 9 files / 38 tests pass.
• @octo/base FilePreviewPanel slice: 3 files / 47 tests pass.
• @dmwork/summary OverflowTooltip.test.tsx: 8/8 pass.
• @dmwork/summary full suite: 9 files / 50 tests red — byte-for-byte the same counts at base 9c541d61 (I checked out the base commit and re-ran it). Pre-existing, not caused by this PR. OverflowTooltip.test.tsx is not among the failures.
• Runtime probes described in §2, all removed afterwards; working tree left clean.
• CI at this head: Build, install-build, e2e-p0 and the sanity/scan jobs are all green.
• Stacked dependencies #1413 and #1439 are both still open and both target main, so the stated merge order has to be enforced manually at merge time — merging this PR first would land all three.

8. Coverage — what this review could not verify

• Real-browser rendering: Semi's autoAdjustOverflow flip near viewport edges, wrapper padding versus .semi-tooltip-content padding, and overlay stacking inside ClawInfoModal. jsdom cannot answer any of these.
• Storybook visual output in light and dark themes.
• Actual scroll performance of the excel/jsonl preview under the restored conditional-mount path (it should now be at or below the pre-PR baseline, but that is reasoning, not measurement).
• The contacts OverflowTooltip was reviewed statically and by structural equivalence, not probed in isolation.
• Two independent second-opinion passes ran alongside this review; both completed, and one of them was limited to static reading because it could not execute anything in its sandbox — its "no P0/P1" result is therefore unvalidated by execution and I have not leaned on it. Between them they surfaced one finding I had also reached independently (P2-2, the loss of hover-time measurement) and three I checked and rejected: that the conditional wrapping might silently drop the callback ref (refuted — Semi chains it, tooltip/index.js:650-655, and the runtime probes confirm the observer follows the live node); that tone="online" is not a valid Dot tone (it is — packages/octo-ui/src/components/Dot/types.ts:10 — and those files belong to #1439, not this PR); and that removing position="top" from Messages/File/index.tsx and PdfRenderer.tsx changes placement (it does not — the primitive defaults to placement="top", Tooltip/index.tsx:83). One of those passes also recommended returning to an always-mounted tooltip with a disabling prop; that is precisely the round-2 approach that Semi ignores, so it is rejected on the record rather than left open.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).

---

Code Review — PR #1478 (octo-web)

Reviewer: Octo-Q (automated review)
Head f03f17b2e534295128262f5bb387e2b1e0082ff3 · Base main (merge-base 6e52f7e4) · 129 files, +3784/−599

Summary

Despite the "shared tooltip component" title, this PR lands the entire @octo/ui design-system package — Button, Tag, Badge, Dot, Tooltip plus token/theme CSS, a tsup build, generator support, CI steps — and migrates ~40 call sites across dmworkbase, dmworkcontacts, dmworkmcp, and dmworksummary off Semi UI Tag/Tooltip/Badge and assorted hand-rolled styles. The architecture is sound: one aggregated styles.css, a :where() (specificity-0) override contract that lets consumer classes re-theme components, and an exports map that feeds source to Vite dev and dist to production builds. I found one P1 robustness issue in the new Tooltip wrapper (type-gated and unreachable from current call sites — reviewer discretion), plus five P2 items.

Verification

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

• ✅ Diff scope — local git diff pr-base...HEAD matches GitHub's PR diff exactly (129 files, +3784/−599).
• ✅ Token wiring — every --wk-* variable referenced by octo-ui CSS (incl. --wk-color-online, --wk-text-size-badge, --wk-brand-gradient, --wk-purple-300) exists in packages/dmworkbase/src/theme/.
• ✅ Build wiring — traced every entrypoint: web src/index.tsx and extension sidepanel import @octo/ui/styles.css; options/offscreen entrypoints render no migrated components; development exports condition resolves source in Vite dev while prod builds consume dist, backed by pre* scripts and turbo dependsOn.
• ✅ Migration parity — each replaced call site checked against its pre-PR contract (gating conditions, 99+ overflow, placement/delay/size/color mappings).
• ✅ Semi semantics — verified against DouyinFE/semi-design source that mouseEnterDelay is milliseconds (passed directly to setTimeout) and confirmed the componentDidUpdate warning polarity.

Findings

One P1 and five P2 items below.

P1 — Non-renderable object content crashes Tooltip rendering (packages/octo-ui/src/components/Tooltip/index.tsx:25)

hasNodeContent() returns true for any value that is not null/undefined/false/string — including plain objects — and renderContent() (:42) returns such an object straight to React, which throws "Objects are not valid as a React child". Reachability: TooltipContent = ReactNode | TooltipContentConfig rejects a bare {} at compile time and no current call site passes one, so today this only fires when TypeScript is bypassed; as a brand-new shared library, the safe contract should not depend on that. Harden hasNodeContent to return false for plain non-element objects (or sanitize in renderContent), plus a unit test.

P2 — isDelayed pairing trips a Semi console warning on every update (packages/octo-ui/src/components/Tooltip/index.tsx:118)

mouseEnterDelay={isDelayed ? 300 : 0} with hardcoded mouseLeaveDelay={0} satisfies semi-ui's mouseLeaveDelay < mouseEnterDelay check, and semi-foundation's warning(flag, msg) fires console.warn whenever the flag is true (not NODE_ENV-gated). Every re-render of the delayed tooltip (currently the folded AI-session participants tooltip, packages/dmworkbase/src/Components/Conversation/index.tsx:2351) logs "mouseLeaveDelay cannot be less than mouseEnterDelay…". Hiding itself still works (delayHide clears the enter timer), but the recurring warning is noisy and misleading. Pick a leave delay that satisfies the invariant or control visibility manually.

P2 — Overflow tooltips no longer re-check truncation on hover (packages/dmworksummary/src/components/OverflowTooltip.tsx:18)

The old code re-evaluated scrollWidth > clientWidth on every mouseenter; the rewrite relies solely on ResizeObserver + window resize + children/title changes. Scroll-width changes that don't resize the element's own box (late font-metric changes, content loading inside the cell) leave isTruncated stale, so a truncated value can show no tooltip. Same pattern in packages/dmworkbase/src/Components/FilePreviewPanel/renderers/TooltipCell.tsx:25 and packages/dmworkcontacts/src/Contacts/index.tsx:40. Keep a cheap mouseenter re-check, and ideally dedupe these three copies into the shared package this PR introduces.

P2 — Join-approval CTA silently shrinks from 44px/16px to 36px/14px (apps/web/src/Components/JoinApprovalResult/index.tsx:37)

The removed CSS (commented "替代 !important") deliberately forced height: 44px / font-size: 16px on this public-page action; octo-ui size="md" resolves to min-height: calc(--wk-sp-5 + --wk-sp-4) = 36px and --wk-text-size-md = 14px (packages/octo-ui/src/components/Button/index.css). If 44px was a design requirement this is a visual regression; if the smaller button is the intended unification, confirm explicitly, or add an lg size.

P2 — Tag close button accessible name enforced only at the type level (packages/octo-ui/src/components/Tag/index.tsx:62)

The discriminated union already requires closeAriaLabel when closable is true (all current call sites pass it), so this is narrower than it first appears — but untyped/runtime callers can still omit it and produce an unnamed close button. A dev-time fallback label or warning would close the gap for future consumers.

P2 — Icon-only Button has no accessible-name guard (packages/octo-ui/src/components/Button/index.tsx:23)

With iconOnly the text content is suppressed and nothing warns if aria-label/aria-labelledby is missing. No production call site uses iconOnly yet; add a dev-mode warning before the pattern spreads.

Data-flow backtrace

• Unread counts (totalUnread / followUnread / recentUnread): gating conditions unchanged (> 0, mute split intact); Badge's default overflowCount=99 reproduces the old 99+ cap; locked by new tests in ConversationList/__tests__/layout.test.tsx and SidebarTabBar.test.tsx.
• Online dots: isOnline / needShowOnlineStatus / getOnlineTip gating unchanged; consumer classes override --octo-ui-dot-size/--octo-ui-dot-color safely because component variants sit in :where() (specificity 0).
• Tooltip content: all migrated call sites pass i18n strings or JSX; disabled/blank suppression preserved and covered by Tooltip.integration.test.tsx, which exercises the real Semi Tooltip (no mock).
• Styles at runtime: @octo/ui/styles.css loads after base tokens in web + sidepanel; options/offscreen pages render no migrated components, so the missing import there is harmless.
• Build artifacts: dist/index.js + dist/styles.css are guaranteed before consumption by pre* scripts and turbo dependsOn; Storybook aliases to source.

Blind-spot checklist

• C1 dual-path parity — clear: ResizeObserver connect/disconnect is symmetric in all three truncation components; Tag close handles stopPropagation + onClose; every deleted CSS class (wk-conv-compact-icon--reddot, hc-dot*, wk-fold-session-tag, wk-mcp-tooltip-light, dark-mode .semi-badge-count override) has no remaining consumer. Leftover ai-badge* class strings in AiBadge are dead but asserted by tests.
• C2 control-flow ordering — clear: the conditional Tooltip wrapper flips tree shape when truncation changes; ref-callback teardown/setup handles the remount; :where() overrides are order-independent.
• C3 authorization boundary — N/A: no auth/permission/tool-surface changes.
• C4 container/member cascade — N/A: no authorization-state changes.
• C5 build ≠ runtime — hit, cleared by tracing every entrypoint's resolution path (dev development condition vs prod dist, WXT/Vite/Storybook), not just trusting turbo config.
• C6 governance/docs — N/A: no governance or security docs touched.

Cross-round blocker re-check

First formal round for this PR/issue — no prior blockers to re-verify. The branch carries fix commits from the earlier stacked iteration ("resolve tooltip review blockers", "restore tooltip suppression behavior"); those fixes are inside this diff and were reviewed as such — suppression behavior is locked by the real-Semi integration test.

Things I checked that are fine

• mouseEnterDelay=300 is genuinely 300ms (semi passes the value straight to setTimeout) — matches the "停留满 0.3s 才出现" intent.
• The component generator auto-wires src/index.ts exports and the components.css @import (scripts/gen-component.mjs:254-264) — no manual CSS step is missing.
• OctoUIProvider is an intentional no-op; components need no context.
• AiBadge unification to the xs AITag (17 size="small" call sites) is intentional per commit history and covered by web tests.
• No new runtime dependencies — lockfile adds only tsup/postcss/postcss-import/vitest as dev tooling.
• forwardRef declarations in the hand-written index.d.ts match the actual component implementations.

Verdict: CHANGES_REQUESTED

One P1 (non-renderable content crashing the shared Tooltip) gates the merge under the high-recall policy, though it is compile-time-gated and unreachable from current call sites — if the final reviewer accepts the type gate as sufficient mitigation and downgrades it to P2, nothing else in this PR blocks: the five P2s are non-blocking hardening/parity items. The P2s worth addressing regardless are the Semi console-warning pairing, the lost hover-time truncation re-check, and confirming the join-approval CTA sizing.

[Octo-Q] verdict: REQUEST_CHANGES — 1 P1 (Tooltip object-content crash; type-gated, no reachable caller today — final reviewer may downgrade) + 5 P2; zero fabricated blockers, all findings carry path/line evidence.

[yujiawei]

Addendum — adjudicating a follow-up automated pass (verdict unchanged: Approve)

A follow-up automated review landed after my approval, reporting one P1 and five P2s at this same head f03f17b2. I worked through all six rather than let them sit. My verdict does not change, but two items are worth putting on the record and one belongs on a different PR.

The reported P1 — non-renderable object content — is a real mechanism, but not reachable. Downgraded to P2.

The mechanism is genuine and I reproduced it. hasNodeContent (packages/octo-ui/src/components/Tooltip/index.tsx:25-29) returns true for any non-null/undefined/false non-string, and renderContent (:42-43) hands anything that isn't a TooltipContentConfig straight back to React. Rendering the primitive with a mocked Semi:

content	result
{ foo: "bar" }	throws — Objects are not valid as a React child (found: object with keys {foo})
new Date(0)	throws — Objects are not valid as a React child (found: [object Date])
{ body: "hello" }	renders correctly (valid config)
[] / <></>	renders an empty bubble
0	renders 0 — correct

So the Date case is worth adding to the report, and the empty-bubble behaviour for []/<></> is the shallow blank-guard that has been noted as non-blocking before.

Reachability is the part I disagree on. I swept every @octo/ui Tooltip content= in this diff. They receive i18n strings, .join("、") strings, typed string props (text, title, typeLabel, label), or JSX. The one path carrying arbitrary untrusted data — spreadsheet and JSONL cell values — is explicitly stringified upstream before it reaches TooltipCell:

• packages/dmworkbase/src/Components/FilePreviewPanel/renderers/ExcelRenderer.tsx:143-145
• packages/dmworkbase/src/Components/FilePreviewPanel/renderers/JsonlRenderer.tsx:80-82

const renderCellContent = (value: unknown): string => {
  if (value === null || value === undefined || value === "") return "-";
  if (typeof value === "object") return JSON.stringify(value);

Both are declared => string and both collapse objects with JSON.stringify. Header cells pass col.title, a string. With that path closed and TooltipContent rejecting a bare object at compile time, there is no caller today that can trigger this without deliberately bypassing TypeScript.

That makes it a hardening item on a new shared primitive, not a merge gate — worth doing (guard hasNodeContent against plain non-element objects, plus a unit test), but it does not block. I'd rather say that plainly than hold a merge on a defect with no reachable caller.

Two of the P2s duplicate findings already in my review

The Semi mouseEnterDelay/mouseLeaveDelay console warning and the lost hover-time truncation re-check are P2-3 and P2-2 above. Three independent passes have now converged on the hover-time re-check, which is a good signal that it is worth the one-line fix even though it is unlikely to bite.

The join-approval CTA finding is real — but it belongs to #1413, not here

I confirmed the numbers. The removed CSS forced height: 44px; font-size: 16px on that public-page action, and size="md" resolves to min-height: calc(var(--wk-sp-5) + var(--wk-sp-4)) = 36px with --wk-text-size-md = 14px (packages/octo-ui/src/components/Button/index.css:6,11; packages/dmworkbase/src/theme/semantic.css:206,243,244). So it is a genuine 44→36px / 16→14px shrink on a public page, and someone should confirm it is intended.

It is not in this PR's incremental scope, though. git log puts it in commit 0aa468db ("feat(ui): add octo button and tag components"), which belongs to #1413. The same is true of the Tag close-button and icon-only Button accessible-name items. Those three should be raised on #1413 where they can actually be fixed by their author.

This does matter for merge order: all three PRs target main, so if #1478 merges first it lands #1413 and #1439 with it — and these findings along with them. That is the merge-order caveat from §7 of my review, now with a concrete reason to care about it.

Note on the differing scope

The follow-up pass reviewed 129 files against main; I reviewed the 37-file incremental range 9c541d61..f03f17b2. Both framings are defensible — the difference is exactly why three of its six findings land outside what this PR's author changed. Given the stack lands together, the union is the right thing to look at before merge; the ownership of each fix is still per-PR.

[yujiawei]

Code Review — PR #1478 (octo-web)

Reviewed at head SHA 980d8604e04dc5a6a2c1a598234a5e0ae1eb731c (round 4).

Scope. Stacked PR based on #1439. Its own incremental work is git diff 9c541d61..980d8604 — 37 files, +1114/−216. The round-4 delta is the single commit 980d8604 ("fix(ui): guard invalid tooltip content"): 2 files, +20/−0. I re-confirmed both dependency heads (ca108432 for #1413, 9c541d61 for #1439) are still exact ancestors of this head, so the stack has not drifted; content inherited from them is out of scope here.

---

1. Status of the previous blockers

Blocker	Status at this head
R1 — @octo/ui import pulled the real Semi barrel into McpDetailModal.inlineDelete.test.tsx, breaking CI Build	✅ Fixed and still fixed — @dmwork/mcp 17 files / 183 tests pass locally
R1 — ResizeObserver left observing a detached node after the truncation flip	✅ Fixed via callback refs; unchanged since f03f17b2
R2 — visible={false} did not disable a Semi tooltip, so isDisabled was inert	✅ Fixed by if (isInactive) return children; (Tooltip/index.tsx:108); unchanged since f03f17b2
R3 — non-renderable object content crashes the shared Tooltip (type-gated)	✅ Fixed by this round's commit — see §2

The consumer files (TooltipCell.tsx, Contacts/index.tsx, OverflowTooltip.tsx) are byte-identical to the head I verified last round; the only change is inside the shared primitive.

---

2. Verification of the round-4 fix

The commit adds an object-shape guard to hasNodeContent (packages/octo-ui/src/components/Tooltip/index.tsx:25-38):

const REACT_PORTAL_TYPE = Symbol.for("react.portal");

function isReactPortal(content: object) {
  return "$$typeof" in content && content.$$typeof === REACT_PORTAL_TYPE;
}

function hasNodeContent(content: TooltipContent) {
  if (content === null || content === undefined || content === false) return false;
  if (typeof content === "object" && !Array.isArray(content)) {
    return isValidElement(content) || isReactPortal(content);
  }
  return typeof content !== "string" || content.trim().length > 0;
}

The explicit portal branch is necessary and correct: React's isValidElement matches only Symbol.for("react.element"), so a portal would otherwise have been misclassified as non-content and silently suppressed. The TooltipContentConfig path is unaffected because isContentConfig is evaluated first (:96) and its fields are checked individually.

I exercised the primitive directly rather than reading it (probe file removed afterwards; working tree left clean):

content	result at this head
{}	suppressed — renders bare children ✅
{ foo: 1 }	suppressed ✅
new Date(0)	suppressed ✅ (the case that was missing from the original report)
{ body: { foo: 1 } } (config, invalid body)	suppressed ✅
<b>x</b>	renders ✅
[<span/>]	renders ✅
0	renders 0 ✅
createPortal(...)	passes the guard as content ✅

Tests / build at this head (deps installed, run locally):

• pnpm --filter @octo/ui test → 6 files / 31 tests pass (incl. the real-Semi integration tests)
• pnpm --filter @octo/ui typecheck → clean
• pnpm --filter @octo/contacts test → 9 files / 38 tests pass
• pnpm --filter @dmwork/mcp test → 17 files / 183 tests pass
• @octo/base FilePreviewPanel slice → 3 files / 47 tests pass
• @dmwork/summary OverflowTooltip → 8/8 pass
• pnpm --filter @octo/web build → exit 0

---

3. Spec compliance

Spec: ✅

• Missing work: none. Every item in the "Changes" list is present.
• Extra work: apps/web/package.json still carries the pre* lifecycle-script re-ordering. Re-verified at this head by parsing both revisions as JSON — the documents compare deep-equal and only the key order differs, so it is behaviour-neutral. Unrelated churn the description does not mention, but not a functional deviation.
• Deviation: the description's "preserving truncation behavior" is now accurate in outcome. The mechanism did change (measure-on-every-hover → ResizeObserver + callback refs), which leaves one narrow residual gap noted below; worth a line in the description, not a blocker.

---

4. Code quality

Quality: Approved — no P0/P1. The items below are non-blocking.

P2-1 — the object guard is shallow: nested invalid values still throw

The guard only inspects the top-level value, so two shapes in the same reachability class as the one just fixed still crash. Verified empirically at this head:

• content={[{ foo: 1 }]} — arrays are excluded from the object branch (index.tsx:34), so an array carrying a plain object falls through to renderContent and React throws "Objects are not valid as a React child".
• content={{ body: "ok", title: { foo: 1 } }} — hasContent uses .some(hasNodeContent) (:97-104), so a valid body makes the whole config active and the invalid title is rendered at :66. Same for shortcut (:78) and actions (:81).

If the type gate was judged insufficient for a bare {}, it is equally insufficient here. Suggest sanitizing inside renderContent (drop non-renderable field values) rather than widening hasNodeContent, which keeps the fix in one place.

P2-2 — non-array iterables are now silently suppressed

Under React 18's typings, ReactNode includes Iterable<ReactNode>, so a Set/Map/generator is a type-legal content. Before this commit React would render it; now typeof === "object" && !Array.isArray classifies it as non-content and the tooltip does not mount at all (probe: new Set(["a"]) → bare children). No current call site is affected, and the failure mode is benign (missing tooltip, not a crash) — flagging it because it is a behaviour narrowing introduced by a hardening commit. Symbol.iterator in content would close it.

P2-3 — blank-ish content still yields an empty bubble

[], <></> and true all pass hasNodeContent and mount an empty dark bubble. Previously noted as non-blocking; unchanged.

P2-4 — carried over, still open (previously reported, not regressions)

• mouseEnterDelay={isDelayed ? 300 : 0} with a hardcoded mouseLeaveDelay={0} trips Semi's non-NODE_ENV-gated mouseLeaveDelay cannot be less than mouseEnterDelay warning on every update of a delayed tooltip (index.tsx:127-128). Functionally harmless; noisy in dev consoles.
• The overflow consumers no longer re-check truncation on mouseenter. ResizeObserver covers box changes but not scroll-width changes that leave the box identical (late font metrics, content loading in-cell), so isTruncated can go stale. A cheap onMouseEnter re-measure would close it, and the three near-identical copies (TooltipCell.tsx, Contacts/index.tsx, OverflowTooltip.tsx) are a good candidate to dedupe into the shared package this PR introduces.

Out of scope — reported against the wrong PR

The join-approval CTA sizing change (apps/web/src/Components/JoinApprovalResult/index.tsx) is not in this PR's incremental diff (9c541d61..980d8604) — it is inherited from the Button/Tag work in the stacked dependencies. It should be raised on #1413 / #1439, where it can still be acted on.

---

5. Overall verdict

APPROVE.

Every blocker raised across four rounds is resolved, and — the trap this PR kept falling into — resolved without re-introducing one another. I verified that at runtime, not by reading. The residual items are hardening against inputs that TypeScript already rejects, plus two carried-over polish notes.

---

6. Process note — stacked-PR merge order

This is round 4 of review on this PR, so flagging the structural risk rather than only the code:

980d8604's base is main, but #1413 (ca108432) and #1439 (9c541d61) are both still open and unmerged. Their heads are exact ancestors of this head, so merging #1478 as-is lands Tag + Badge/Dot + Tooltip in a single 129-file, +3804/−599 commit and makes the two parent PRs — and their independent reviews — moot after the fact.

Two implications worth a maintainer decision before merge:

1. Merge order should be #1413 → #1439 → #1478. Merging this one first bypasses the parents' own review gates.
2. Findings raised against the inherited surface (e.g. the join-approval CTA sizing) have no PR left to be fixed on once #1478 merges. Either resolve them on the parents first, or accept them explicitly here.

This is a merge-sequencing question for the maintainer, not a defect in the diff, and it does not gate this approval.

[Jerry-Xin]

Re-review (round 4) at 980d860 — new commit fix(ui): guard invalid tooltip content on top of the approved round-3 head f03f17b. Verdict: APPROVE.

✅ Blocking item resolved

• 🔴 → Fixed — invalid tooltip content guard. hasNodeContent() now rejects non-renderable plain objects: packages/octo-ui/src/components/Tooltip/index.tsx:34-36 adds an object branch requiring isValidElement(content) || isReactPortal(content) (portal check at lines 25-29 via Symbol.for("react.portal")). Rejected shapes: null/undefined/false, blank strings, and any plain object that is neither a ReactElement nor a portal (this is the crash input — pre-guard, content={{}} passed hasNodeContent, mounted SemiTooltip, and React threw "Objects are not valid as a React child"). Accepted shapes verified intact: non-empty strings, numbers, ReactElement, fragments, portals, arrays of nodes. Rejection renders children bare via the existing non-mounting suppression (if (isInactive) return children;, index.tsx:113) — no wrapper, no overlay. The guard also covers TooltipContentConfig fields since config mode runs hasNodeContent over title/body/shortcut/actions (index.tsx:104-110). New regression test does not render plain objects passed by untyped consumers (Tooltip.test.tsx:125-135) asserts the rejection path; acceptance of valid shapes remains covered by the pre-existing suite cases.

🟡 Carried (non-blocking, unchanged on this head)

• Delay warnings: still live. mouseEnterDelay={isDelayed ? 300 : 0} with mouseLeaveDelay={0} (index.tsx:118-119) is unchanged since round 1. Verified against the pinned semi-ui 2.93.0: componentDidUpdate warns 'mouseLeaveDelay' cannot be less than 'mouseEnterDelay' whenever leave < enter (semi-ui tooltip/index.js:593; Semi's warning() is an ungated console.warn), so the single isDelayed consumer (packages/dmworkbase/src/Components/Conversation/index.tsx:2351) logs on every update. Cosmetic console noise only; not a functional defect.
• Hover-time re-detection: factually — the pre-migration trigger="custom" consumers DID re-check truncation inside handleMouseEnter (el.scrollWidth > el.clientWidth before setVisible(true)); that hover recheck was removed in the migration. The round-2 mechanism replaces it with continuous detection: callback-ref observer rebinding, ResizeObserver on the measured element, content-dep effect, and window-resize listener (all three consumers verified). The only residual gap vs the old behavior is a scrollWidth change with unchanged element box and unchanged content (e.g. a late-loading font), which won't re-detect until the next size/content change. Worst case is a stale tooltip decision, not a crash or wrong data. Non-blocking.

💬 Minor observations (non-blocking)

• The guard does not recurse: an array containing a plain object (content={[{}]}) or a config whose fields mix one valid value with one plain-object value would still reach React and throw. Both require TS-level type violations (TooltipContent is ReactNode | TooltipContentConfig) and no current consumer passes config objects or untyped content — all three truncation consumers pass strings/elements (TooltipCell.tsx:66, Contacts/index.tsx:76, OverflowTooltip.tsx:58). Noted for future hardening, e.g. filtering invalid fields in renderContent.

🔬 Validation

• octo-ui Tooltip suite: 2 files / 8 tests passed, including the real-Semi integration test (Tooltip.integration.test.tsx) against semi-ui 2.93.0.
• Consumer regression: dmworksummary OverflowTooltip.test.tsx 8/8 (incl. MockResizeObserver cases), dmworkcontacts Contacts tests 3/3.
• Round-3 items intact on this head: non-mounting suppression, integration test, ResizeObserver stranded-node protection (callback ref + disconnect in all three consumers).
• Stacked deps #1413 (ca10843) and #1439 (9c541d6) verified as exact ancestors of this head.

✅ Highlights

• Minimal, surgical +20-line fix that directly targets the reported crash without disturbing the round-3 suppression mechanism.
• Portal support in the validity check shows care (portals are objects but legitimate renderable content).

[mochashanyao]

[Octo-Q · automated review]

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

---

Code Review — PR #1478 (octo-web)

Reviewer: Octo-Q (automated review)

Summary

Introduces the shared design-system package @octo/ui (Tooltip, Tag, Badge, Dot, Button + token CSS layer, tsup build, stories/tests) and migrates ~40 consumer components across @octo/base, @dmwork/summary, @dmwork/mcp, @octo/contacts, and the web/extension apps from Semi UI Tooltip/Tag/Badge and ad-hoc CSS dots/badges onto the shared components. Also rewires build (turbo deps + pre* hooks), CI (octo-ui tests + targeted migration tests), and Storybook (aliases + provider).

Verification

Static analysis only at head 980d8604e04dc5a6a2c1a598234a5e0ae1eb731c (merge-base 6e52f7e4); build and tests not executed in this environment. All 129 changed files reviewed via local diff; every --wk-* token referenced by octo-ui CSS was grep-verified against packages/dmworkbase/src/theme/primitive.css / semantic.css (all defined).

Findings

No P0/P1 issues. Two nits.

Nit — Class-name typo in new bot-admin tag (packages/dmworkbase/src/Components/Subscribers/list.tsx:443)

New class wk-subscrierlist-bot-admin-tag is missing a 'b' ("subscrier" vs "subscriber"). It matches the declaration at packages/dmworkbase/src/Components/Subscribers/list.css:152 so styles apply, but worth renaming while the class is brand new.

Nit — Orphaned ai-badge classes after stylesheet deletion (packages/dmworkbase/src/Components/AiBadge/index.tsx:14)

AiBadge still emits ai-badge / ai-badge-default, but their stylesheet was deleted in this PR — the classes are dead hooks kept alive only by a test assertion. Consider dropping them (and the assertion) or documenting them as compatibility hooks.

Data-flow tracing

• Unread counts: totalUnread / followUnread / recentUnread flow into <Badge count>; legacy >99 → "99+" semantics preserved by Badge's default overflowCount={99} (count > overflowCount is identical to the old > 99 test) and pinned by new tests (SidebarTabBar.test.tsx, ConversationList/__tests__/layout.test.tsx). Muted-state override now sets --octo-ui-badge-bg/color at class specificity, which beats the :where()-scoped defaults.
• Online status: channelInfo.online / process_status → Dot tone mapping verified per consumer (NavRail/Avatar online, ProfileOnlineStatus success|neutral, ClawInfoModal success|warning|neutral, ClawHealthCheckItem error→danger). Consumer CSS overrides (--octo-ui-dot-size/color) match the old hard-coded px values (10px/9px/6px).
• Tooltip content: all migrated consumers pass string labels or JSX. The new hasNodeContent guard returns children untouched for empty/blank/disabled content, replacing the old trigger="custom" + controlled-visible hacks in TooltipCell, OverflowTooltip, and Contacts; those now conditionally mount the Tooltip and track truncation via ResizeObserver + resize listener. Observer disconnect verified on both the callback-ref null path and effect cleanup.
• Tag tones: getStatusColor re-typed to TagTone with grey→gray corrected; every variant×tone combo used by consumers has a matching .octo-ui-tag--{variant}.octo-ui-tag--{tone} rule plus tokens (light: gray/red/amber/green/blue/cyan/purple/orange/pink; solid/pastel/ai complete).
• prefixIcon → icon prop rename applied at all SummaryDetailPage sites; position → placement mapped via SEMI_PLACEMENT (bottomLeft → bottom-start etc.), default "top" matches all removed position="top" usages.

Blind-spot checklist (C1–C6)

• C1 dual-path parity: clear — Tooltip mount/unmount cleans up the ResizeObserver on both paths; gen-component.mjs only appends exports (no remove command exists; N/A).
• C2 control-flow ordering / reuse: clear — Tooltip guards are pure and single-site; Badge/Dot/Tag renders are side-effect-free; no reordered reuse.
• C3 authorization boundary: N/A — presentational components only.
• C4 authorization lifecycle: N/A.
• C5 build ≠ runtime: statically verified — every entry importing @octo/ui/styles.css (web, extension sidepanel, Storybook preview) has a pre* hook or turbo dep building dist first; exports.development → src/index.ts matches the existing workspace convention (@octo/base ships src/index.tsx via main); committed src/index.d.ts matches runtime exports 1:1. Storybook surfaces not runtime-verified (no build executed here).
• C6 governance/docs consistency: N/A — no governance docs touched.

Cross-round blocker re-check (R6)

N/A — first review round for this PR on this issue.

Things I checked that are fine

• Removed .semi-badge-count dark-mode override in App.css: no remaining Semi Badge usage anywhere (grep).
• WKButton → Button in McpMarketListPage.tsx: WKButton is a plain ButtonHTMLAttributes wrapper with an identical variant set; props used (variant/icon/onClick/data-testid) all supported — no behavior lost.
• Positioning: .wk-conv-compact-icon is position: relative (anchors the new absolute unread dot); OnlineStatusBadge empty state keeps 9px/2px-border/box-sizing parity.
• Lockfile additions are tsup + build-tooling deps only (dev); no new runtime dependencies; workspace globs already cover packages/octo-ui.
• CI: new pnpm --filter @octo/ui test and targeted migration tests reuse the established pnpm --filter … exec vitest run pattern; test mocks for @octo/ui added wherever consumers are rendered under test.
• AiBadge/AITag/wk-ai-collab-tag visual changes are deliberate unification (new spec documented in AITag stories); replaced CSS removed everywhere.
• Tests: Tooltip unit (mocked Semi) + integration (real Semi in jsdom) cover the invalid-content guard; migration tests pin the new classes and the 99+ cap.

Verdict: APPROVED

No P0/P1 findings; two optional nits.

[Octo-Q] verdict: APPROVE — the design-system migration preserves all runtime contracts (unread counts, online dots, tooltip content guards, tag tones) with tests pinning the behavior; only two cosmetic nits.