fix(contacts): align letter header height with virtual list estimate (#1118)

[dolphinsboy]

Fixes #1118

变更摘要

• packages/dmworkcontacts/src/Contacts/index.css: .wk-contacts-letter-header 固定 height: 24px，改用 display: flex; align-items: center 垂直居中，去掉上下 padding（保留水平 padding），消除 CSS 实际渲染高度（~29px）与虚拟列表 JS 估算值（24px）的 ~5px 偏差，导致每个字母分组首项与下一项纵向重叠。
• packages/dmworkcontacts/src/Contacts/index.tsx: 在 LETTER_HEADER_HEIGHT 常量旁加注释，防止未来 CSS/JS 高度值漂移。
• packages/dmworkcontacts/src/Contacts/__tests__/virtualListEstimateSize.test.ts: 新增单测覆盖 estimateSize 计算逻辑（showLetter=true → 68px，false → 44px）和字母分组首行 showLetter 标记。

根因

.wk-contacts-letter-header 使用 padding: var(--wk-sp-2) var(--wk-sp-4) 上下各 8px 撑开，加上 font-size:11px; line-height:normal ≈ 13px，实际渲染高度约 29px；而虚拟列表按 ITEM_HEIGHT(44) + LETTER_HEADER_HEIGHT(24) = 68px 估算，差值约 5px 导致分组首行与下一行重叠。

验收证据

• 构建: pnpm --filter @octo/web build → ok（14969 modules transformed, built in 2.98s）
• CSS lint: 0 errors（改动区域无新 warning）
• 单测: 33/33 passed（含新增 4 个）
• Playwright DOM 验收（Chromium）：
  • letter header offsetHeight = 24px，虚拟首行 height = 68px，与估算完全一致
  • 选中态边界：selected bottom = next item top，overlap = 0
  • 全列表 consecutive items max overlap = 0
  • 滚动（0 / 2760 / 5520 scrollTop）hit test 全部通过，无行跳变
  • ≤100 条普通 DOM 路径：.virtual-row count = 0，header heights = [24]，无回退

[lml2468]

Review: #1140 — fix(contacts): align letter header height with virtual list estimate (#1118)

意图:修 #1118——通讯录虚拟列表里字母分组 header 与下一行重叠。根因:虚拟列表 estimateSize 对带字母头的行按 ITEM_HEIGHT(44)+LETTER_HEADER_HEIGHT(24)=68 估算,但 .wk-contacts-letter-header 原来只有 padding: sp-2 sp-4(无固定高、实际约 ~31px)→ 实际行高 > 估算 → 定位漂移重叠。修法:把 header 固定为 height:24px(flex 居中、去纵向 padding、border-box),使实际高 == 估算常量。锚定 head f0c10b5bca8e(rev-parse 确认 checkout==live head)/ mergeable=true。3 文件 +71/−1。

结论:APPROVE ✅

Gate 1 — 规格符合度 ✅

仅调 header CSS 高度 + 加注释绑定常量 + 加测试。精准对应 #1118,无 over-build。

Gate 2 — 代码质量 ✅

• 实际高 == 估算,重叠消除:estimateSize(index.tsx:102-103)对 showLetter 行返回 ITEM_HEIGHT+LETTER_HEADER_HEIGHT=68、否则 44;渲染 {showLetter && <div.wk-contacts-letter-header/>}{renderItem},header 现固定 24px + item CSS height:44px(:153)= 68,非头行 44 —— 逐行实际高与估算完全一致,虚拟列表按估算绝对定位不再漂移。
• header 布局正确:height:24px + display:flex + align-items:center + padding:0 sp-4 + box-sizing:border-box,11px/600 文本在 24px 内 flex 居中放得下,不再靠纵向 padding 撑高。
• 常量↔CSS 绑定:LETTER_HEADER_HEIGHT=24 上方加注释「必须与 .wk-contacts-letter-header 的 height 保持一致」。
• 无逻辑/安全/数据面(纯高度对齐)。

构建 / 测试(实跑 @ f0c10b5bca8e):pnpm --filter @octo/web build ✅ 4.73s;@octo/contacts 的 virtualListEstimateSize.test 4/4 全绿(锁定 68/44 估算、showLetter 仅每组首项、常量 24/44)。

---

🟡 非阻断

• 新测试自行复刻 estimateSize/buildListRows/getLetterFromPinyin 与常量(而非从组件 import)——能锁定「预期值/分组语义」,但不真正守卫组件实现:若组件里的 estimateSize/常量将来漂移,该测试仍绿。建议改为 import 组件的真实函数/常量。
• CSS 高度与 TS 常量为手工双写(靠注释同步),将来改 CSS header 高度易与 LETTER_HEADER_HEIGHT 悄悄脱钩;可考虑共享一个常量/CSS 变量。均非阻断。

---

把字母 header 固定为与虚拟列表估算常量一致的 24px(flex 居中、去纵向 padding),使每行实际高 == 估算(头行 68 / 非头行 44),消除 #1118 的重叠;构建通过、4 测试锁定估算与分组语义。可合并 —— APPROVE(建议后续让测试 import 组件真实实现、并把 24 收敛为 CSS/TS 共享常量以防脱钩)。

[Jerry-Xin]

The PR is in scope for octo-web and the CSS/runtime estimate now align for the contacts virtual list.

💬 Non-blocking

• 🔵 Suggestion: virtualListEstimateSize.test.ts duplicates ITEM_HEIGHT, LETTER_HEADER_HEIGHT, getLetterFromPinyin, and estimateSize instead of exercising production code from index.tsx. This means future production drift could pass the test if the copied values are not updated. Consider extracting the sizing/grouping helpers into a small testable module or exporting narrowly-scoped helpers.
• 🔵 Suggestion: virtualListEstimateSize.test.ts asserts local constants only, so it does not prove CSS and JS remain synchronized. A lightweight DOM/style assertion or shared constant would make this guard stronger.

✅ Highlights

• The change in index.css fixes the reported root cause directly: the letter header now has an explicit 24px height and no vertical padding.
• The virtual-list estimate in index.tsx remains consistent with the intended 44px + 24px row sizing.
• No security, error-handling, or performance regressions found in the changed surface.

Dismissing to re-post with repository-relative paths (previous body contained local workspace paths).

[Jerry-Xin]

The PR is in scope for octo-web and the CSS box height now reconciles exactly with the virtual-list size estimate for the contacts list, fixing the reported drift (#1118).

✅ Correctness (byte-traced)

The @tanstack/react-virtual virtualizer is estimate-only (no measureElement), so the estimateSize return value must exactly match each row's real rendered box height — otherwise scroll offset / sticky-header drift accumulates (the #1118 symptom).

• packages/dmworkcontacts/src/Contacts/index.tsx:102 — estimateSize returns ITEM_HEIGHT + LETTER_HEADER_HEIGHT (= 44 + 24 = 68px) for rows with a letter header, else ITEM_HEIGHT (44px).
• packages/dmworkcontacts/src/Contacts/index.css:253 — .wk-contacts-letter-header now has explicit height: 24px + box-sizing: border-box + padding: 0 <horizontal>, no vertical padding, no border, no margin → real layout box = 24px, matching LETTER_HEADER_HEIGHT.
• packages/dmworkcontacts/src/Contacts/index.css:150 — .wk-contacts-section-item (the contact row from renderContactItem) has height: 44px, only horizontal padding/margin, no border → real layout box = 44px, matching ITEM_HEIGHT.
• Per-index sizing is correct for the mixed header/row layout: each virtual row wrapper renders an optional 24px header followed by the 44px contact row, and showLetter is set only on the first item of each letter group (index→size mapping is consistent, no off-by-one).

Root cause was the old .wk-contacts-letter-header rule using padding: var(--wk-sp-2) var(--wk-sp-4) (vertical padding) → auto/variable height ≠ the assumed 24px, so the estimate under/over-counted each header. The new fixed 24px box removes that mismatch. No regression to scroll restore (scrollToOffset) or the sticky letter-index offset.

💬 Non-blocking

• 🟡 packages/dmworkcontacts/src/Contacts/__tests__/virtualListEstimateSize.test.ts re-declares local copies of ITEM_HEIGHT, LETTER_HEADER_HEIGHT, getLetterFromPinyin, and estimateSize instead of importing the production code from index.tsx. It's a self-consistent tautology: it cannot catch real drift between the production const and either index.tsx logic or the CSS. Consider extracting the sizing/grouping helpers into a small exported module and having the test exercise those, so the guard actually fires on regression.
• 🟡 The 24/44 values still live as two independent literals — JS consts in index.tsx and CSS literals in index.css — bridged only by a comment. A shared token (CSS var consumed by JS, or a JS const injected into style) would make single-source-of-truth structural rather than convention, preventing a future #1118 recurrence.

✅ Highlights

• Directly fixes the reported root cause with a minimal, well-scoped change.
• The 44 + 24 estimate stays consistent with the intended row sizing.
• No security, error-handling, or performance regressions in the changed surface.

Note: the check-sprint check is red, but it's the "Validate Sprint field on Project Board" project-board metadata gate — not a code/build failure. Please set the Sprint field so the gate goes green before merge.

[mochashanyao]

[Octo-Q · automated review]

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

---

Code Review — PR #1140 (octo-web)

Reviewer: Octo-Q (automated review)

Summary

This PR fixes a visual misalignment in the contacts virtual list: the @tanstack/react-virtual estimateSize callback assumed letter-group headers were 24 px tall (LETTER_HEADER_HEIGHT = 24), but the CSS class .wk-contacts-letter-header had no explicit height — it relied on padding: var(--wk-sp-2) var(--wk-sp-4) plus content, which produced a rendered height that did not match the 24 px estimate. The mismatch caused virtual-list scroll positioning drift on large contact lists. The fix pins the CSS to an explicit height: 24px with box-sizing: border-box and flexbox vertical centering, adds a maintenance comment on the constant, and ships a focused unit-test file that locks the estimateSize arithmetic and the showLetter grouping logic.

Verification

• ✅ CSS–constant alignment — .wk-contacts-letter-header at packages/dmworkcontacts/src/Contacts/index.css:254 now declares height: 24px with box-sizing: border-box, matching LETTER_HEADER_HEIGHT = 24 at packages/dmworkcontacts/src/Contacts/index.tsx:55. The estimateSize callback at :101 returns ITEM_HEIGHT + LETTER_HEADER_HEIGHT = 68 for header rows and ITEM_HEIGHT = 44 otherwise, which now matches the actual rendered box.
• ✅ Both render paths covered — The .wk-contacts-letter-header class is used in the virtual-list path (VirtualContactList at :128) and the non-virtual path (renderContactListWithLetters at :~868). Both consume the same CSS class, so the explicit height applies uniformly.
• ✅ Non-virtual path unaffected — The small-list path (≤ 100 items, :~856) renders letter headers as block-level divs without virtual sizing; the explicit height is a pure visual improvement there with no contract change.
• ✅ Item height parity — .wk-contacts-section-item at index.css:188 declares height: 44px, matching ITEM_HEIGHT = 44 at index.tsx:53. Pre-existing, unchanged, still consistent.
• ✅ showLetter flag correctness — buildIndex at :~510 sets showLetter: letter !== prevLetter, marking only the first item per letter group. The test at virtualListEstimateSize.test.ts:38–56 locks this behavior with a five-item, two-group fixture.

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

Findings

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

Nit — Test file re-implements production helpers locally (packages/dmworkcontacts/src/Contacts/__tests__/virtualListEstimateSize.test.ts)

The test file duplicates ITEM_HEIGHT, LETTER_HEADER_HEIGHT, getLetterFromPinyin, and the estimateSize formula as local definitions rather than importing them from the production module. This means a future change to the production LETTER_HEADER_HEIGHT would not cause the test to fail — the test asserts its own local 24, not the production constant. The production-side comment at index.tsx:54 ("必须与 .wk-contacts-letter-header 的 height 保持一致") partially mitigates this drift risk. Not blocking — acceptable for a simple snapshot-style test, but exporting the constants from the production module and importing them in the test would make the contract machine-enforced.

Data Flow Trace

• LETTER_HEADER_HEIGHT = 24 (index.tsx:55) → consumed by estimateSize callback (index.tsx:101) → useVirtualizer uses return value to compute virtualItem.start positions and getTotalSize() → determines scroll offset math and absolute-positioned translateY values.
• .wk-contacts-letter-header { height: 24px } (index.css:254) → rendered DOM height for <div className="wk-contacts-letter-header"> in both virtual (:128) and non-virtual (:~868) paths.
• ITEM_HEIGHT = 44 (index.tsx:53) → consumed by same estimateSize → matches .wk-contacts-section-item { height: 44px } (index.css:188).
• showLetter boolean → set in buildIndex (:~510) via letter !== prevLetter → gates whether the letter header div renders for a given virtual row.

All consumption points trace back to correct upstream sources. No empty-array, undefined, or gate-shortcut risks.

Blind-spot Checklist

• C1 — Dual-path parity: Letter header CSS consumed in both virtual and non-virtual render paths. Both use same .wk-contacts-letter-header class — explicit height applies uniformly. Clear.
• C2 — Control-flow ordering: No control-flow changes. N/A.
• C3 — Authorization: No auth changes. N/A.
• C4 — Lifecycle: No lifecycle changes. N/A.
• C5 — Build/runtime: CSS-only height pin; no build config or packaging changes. Clear.
• C6 — Governance/docs: No governance changes. N/A.

Cross-round Blocker Re-review

N/A — first review of this PR.

Verdict: APPROVED

Clean, well-scoped UI fix. The CSS height now matches the virtual list estimate, resolving scroll-positioning drift on large contact lists. The added tests lock the sizing arithmetic and grouping logic. One non-blocking nit on test-file helper duplication.

[Octo-Q] verdict: APPROVE — No P0/P1 issues. The CSS-constant alignment fix is correct, both render paths are covered, and the data flow from constants to virtual list sizing to rendered DOM is consistent.

[yujiawei]

Code Review — PR #1140 (octo-web)

Reviewed at head SHA f0c10b5bca8e08f1bfa1c31d3e9abc9d862684c9 (single commit on top of 5be068f4), against main. I read the full diff plus the surrounding production code in packages/dmworkcontacts/src/Contacts/ and the CI workflow definitions.

---

1. Spec compliance

Spec: PASS

Checked against the acceptance criteria in issue #1118.

• Nothing missing. #1118 asks that the letter header, the group's first contact, and the next contact each occupy independent vertical space. The root cause it identifies is a mismatch between the rendered height of .wk-contacts-letter-header and the JS constant LETTER_HEADER_HEIGHT = 24 that estimateSize adds to ITEM_HEIGHT = 44. The diff removes the vertical padding and pins the box to height: 24px, so the rendered group-first row is now exactly 24 + 44 = 68px — the value estimateSize returns.

  packages/dmworkcontacts/src/Contacts/index.css:253-262

  .wk-contacts-letter-header {
      height: 24px;
      display: flex;
      align-items: center;
      padding: 0 var(--wk-sp-4);
      box-sizing: border-box;
      font-size: 11px;
      ...

  I verified the arithmetic holds structurally, not just numerically: the virtual row wrapper (index.tsx:145-153) is a plain block with no vertical padding/border; the header now contributes exactly 24px with no vertical margin; and .wk-contacts-section-item (index.css:151-160) is height: 44px with margin: 0 var(--wk-sp-2) — horizontal only, so it adds nothing vertically. Sum is exactly 68px.

• Nothing over-built. No new props, flags, feature toggles, or public API. The box-sizing: border-box addition is not redundant here — this repo has no global box-sizing reset (the only declarations are per-rule, at index.css:17, :82, :258), so declaring it explicitly is correct hygiene rather than scope creep.

• No deviation from the agreed approach. #1118 offered two non-binding directions: (1) make the virtualizer measure real heights, or (2) make the header CSS and the virtual list share one explicit fixed height. This PR takes direction (2), which the issue sanctions. It also honours the issue's explicit instruction not to paper over the overlap by adjusting the group's first contact row — .wk-contacts-section-item is untouched.

• Text still fits: font-size: 11px with line-height: normal gives roughly a 13px line box, flex-centred inside 24px. No clipping.

---

2. Code quality

Quality: Approved — no P0/P1. The four items below are all P2 and none should block merge.

P2-1 — The new test exercises no production code, so it cannot deliver the drift protection the PR claims

packages/dmworkcontacts/src/Contacts/__tests__/virtualListEstimateSize.test.ts:1-30 imports nothing from ../index. It re-declares the constants and re-implements the logic locally:

import { describe, expect, it } from "vitest";

const ITEM_HEIGHT = 44;
const LETTER_HEADER_HEIGHT = 24;
...
function estimateSize(index: number, rows: { showLetter: boolean }[]): number {
    return rows[index]?.showLetter ? ITEM_HEIGHT + LETTER_HEADER_HEIGHT : ITEM_HEIGHT;
}

The last case is circular — it asserts a literal declared on line 4 of the same file:

virtualListEstimateSize.test.ts:61-64

it("LETTER_HEADER_HEIGHT is 24, aligning with CSS .wk-contacts-letter-header height", () => {
    expect(LETTER_HEADER_HEIGHT).toBe(24);
    expect(ITEM_HEIGHT).toBe(44);
});

Why it matters: the PR description states the test was added to guard against future CSS/JS height drift. It cannot. Change index.tsx:55 to 28, or index.css:254 to 28px, and all four tests stay green — which is exactly the regression #1118 was.

Two further signals that this is a copy, not a mirror:

• The repo's own convention is extract-then-import. __tests__/onlineStatusGate.test.ts:2 does import { shouldShowOnlineStatus, selectOnlineStatusUids } from "../onlineStatusGate"; against a real extracted module.
• The copy has already drifted from production. Production uppercases pinyin before bucketing (index.tsx:525-526, .toUpperCase() applied to both branches), so the /[A-Z]/ test in getLetterFromPinyin never sees lowercase input. The test's buildListRows (virtualListEstimateSize.test.ts:17-26) feeds raw item.pinyin with no normalization step. Production is fine; the copy is not faithful.

Suggested fix: extract getLetterFromPinyin / the listRows builder / estimateSize into a small module (e.g. letterRows.ts) exactly as onlineStatusGate.ts was, and import them. If that refactor is out of scope for a UX fix, dropping the file is more honest than keeping a green test that guards nothing.

P2-2 — These tests do not run in CI at all

.github/workflows/ci.yml:91-101 runs only two hand-listed test steps:

      - name: Skill Market tests
        run: pnpm --filter @dmwork/skillmarket test
      - name: Unified import/export unit tests
        run: >-
          pnpm --filter @octo/docs exec vitest run
          src/editor/EditorShell.test.tsx
          ...

There is no turbo run test. @octo/contacts declares "test": "vitest run" (packages/dmworkcontacts/package.json:6) and has ten test files under src/, none of which CI ever invokes. So the PR's "33/33 passed" evidence is local-only, and even a correctly-written version of the P2-1 test would gate nothing.

Worth a follow-up PR adding pnpm --filter @octo/contacts test to the Build job. Pre-existing gap, not introduced here.

P2-3 — Make the height coupling structural rather than documentary

index.tsx:54 adds a comment as the guard:

const ITEM_HEIGHT = 44
// 必须与 .wk-contacts-letter-header 的 height 保持一致
const LETTER_HEADER_HEIGHT = 24

A comment is the weakest available guard for an invariant that just cost a production bug. Two stronger options:

• Single source of truth in JS — drive the height from the constant at both render sites (index.tsx:155 and index.tsx:849): <div className="wk-contacts-letter-header" style={{ height: LETTER_HEADER_HEIGHT }}>. Drift then becomes impossible rather than merely discouraged.

• Fail loudly instead of silently — the virtual row wrapper (index.tsx:145-153) sets only transform, no height:

  style={{
      position: 'absolute', top: 0, left: 0, width: '100%',
      transform: `translateY(${virtualItem.start}px)`,
  }}

  Because the wrapper has no height and no overflow clipping, any future content that exceeds the estimate silently reintroduces #1118. Setting height: virtualItem.size (or attaching virtualizer.measureElement per issue direction 1) makes overflow visible or corrected rather than invisible.

P2-4 — The shared class changes vertical rhythm on the non-virtual path too

index.tsx:849 — the ≤100 plain-DOM path — renders the same class:

<div className="wk-contacts-letter-header">{letter}</div>

That path never had the overlap bug (it does no height estimation), but it inherits the change: the header goes from ~29-31px to 24px, and the 8px of vertical padding that separated the letter from the item above it is gone, leaving ~5.5px of centred slack. Purely a density change, and one the issue's direction (2) implies, but it deserves a design glance since it affects a path with no defect.

One constraint if design wants the air back: do not restore it with vertical margin or padding on the header. Either would re-inflate the real row height past 68px in the virtual path and reintroduce #1118. The only safe adjustment is to bump LETTER_HEADER_HEIGHT (index.tsx:55) and the CSS height (index.css:254) together.

---

3. Overall verdict

APPROVE. The fix is minimal, targets the actual root cause, and the height arithmetic is exact rather than approximate. All four findings are P2 — the test-quality items (P2-1, P2-2) mean this change ships with less regression protection than the description suggests, but they do not make the change wrong.

---

4. Suggested follow-ups

1. Either extract the letter-row/estimate helpers into an importable module and rewrite virtualListEstimateSize.test.ts against the real symbols, or remove the file. (P2-1)
2. Separate PR: wire pnpm --filter @octo/contacts test into ci.yml so this package's ten test files actually gate. (P2-2)
3. Consider style={{ height: LETTER_HEADER_HEIGHT }} on both header render sites to collapse the CSS/JS duplication to one source. (P2-3)
4. Consider height: virtualItem.size on the virtual row wrapper so a future estimate mismatch clips visibly instead of overlapping silently. (P2-3)

---

5. Additional observations (outside the task scope)

• check-sprint is red on this PR and it is not a code defect. The failure is Linked issue has no Sprint set — issue #1118 needs a Sprint assigned on the org project board. Needs a maintainer, not a code change.
• The alphabet index bar has no manual offset arithmetic: I grepped for other consumers of ITEM_HEIGHT / LETTER_HEADER_HEIGHT and there is no scrollToIndex-style pixel math elsewhere in the package, so no third call site needs updating alongside this change.
• .wk-contacts-letter-header has exactly one CSS definition repo-wide (index.css:253) and no theme-level override, so the fixed height cannot be undone by a dark-theme or Space-theme stylesheet.

Verification coverage / what I did not check

• I did not execute the test suite or a browser locally — this checkout has no installed node_modules, and no headless browser was available. My height reasoning is static (CSS box model plus the DOM structure in VirtualContactList), corroborated by the green Build and e2e-p0 checks on this SHA.
• One of the two independent second-opinion passes I normally run alongside my own review did not complete (timed out twice, producing no output). I am recording that as absent, not as agreement — its coverage is simply missing rather than clean. The other pass completed and independently landed on the same test-quality problem described in P2-1; its one additional claim (that the /[A-Z]/ check in getLetterFromPinyin mishandles lowercase pinyin) I checked and refuted for production — index.tsx:525-526 uppercases both branches before bucketing, so lowercase never reaches that check. That claim only holds against the test file's local copy, which is why it appears in P2-1 as drift evidence rather than as a bug of its own.
• The Playwright DOM measurements in the PR description (offsetHeight = 24, overlap = 0, hit tests at three scroll offsets) are author-reported; I did not reproduce them independently.
• I did not exercise real device-pixel-ratio / browser-zoom / forced-minimum-font-size combinations. Since height is now fixed rather than content-derived, these should not reintroduce the mismatch, but that is reasoning, not measurement.