Skip to content

refactor(ui): reuse clipboard feedback for turn footer - #4891

Merged
liugddx merged 4 commits into
apache:mainfrom
99Gaoxiaoqi:refactor/unify-turn-copy-feedback
Sep 6, 2026
Merged

refactor(ui): reuse clipboard feedback for turn footer#4891
liugddx merged 4 commits into
apache:mainfrom
99Gaoxiaoqi:refactor/unify-turn-copy-feedback

Conversation

@99Gaoxiaoqi

@99Gaoxiaoqi 99Gaoxiaoqi commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Refs #1404 (clipboard-feedback slice only; scope claim).

TurnFooterActions duplicates the clipboard state machine already owned by useClipboardCopyFeedback. Reuse the hook with the existing 1400 ms reset and { redact: false }, removing 40 net lines from the component while preserving original-text copying, pending-write deduplication, failure feedback and unmount cleanup. The markup and shared hook behavior are unchanged.

Verification

  • npm --workspace @maka/ui run test:dist: 392 passed, 0 failed. Five added tests render the actual TurnView and cover raw text, overlapping writes, success/reset timing, failure/retry, completion after unmount, cancellation of an earlier reset during another copy, and cancellation of an active reset on unmount.
  • npm run build, npm run typecheck, npm run lint, npm run format:check: passed. The existing format configuration excludes UI/Desktop sources; their surrounding style is retained.
  • npx knip --workspace apps/desktop, npx knip --workspace packages/ui, npm run check:renderer-architecture, npm run check:asf-headers, npm run check:e2e-budget and git diff --check: passed.
  • The tests also pass against the original implementation, as expected for a behavior-preserving refactor. Removing { redact: false } makes the raw-text test fail; the final source was restored and verified.
  • The two timer-cleanup tests also fail independently when the corresponding cleanup call is deliberately removed from the compiled hook; the compiled file was restored before the final build and passing 392-test run.
  • Chromium/Storybook: same temporary fixture rendering the real TurnView, with a mocked clipboard writer. Raw output, copied feedback and reset passed before and after; corresponding screenshots are byte-identical. The temporary fixture is not included.
  • Not run: full root test suite and Electron E2E. The Electron binary download did not complete in this environment.
State Before After
Idle Before idle After idle
Copied Before copied After copied

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex authored the refactor and tests, ran verification, and prepared this PR on behalf of @99Gaoxiaoqi. The commit includes Generated-by: OpenAI Codex; please retain it when squashing. Independent human review is required.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

The first checkbox is intentionally unchecked: this refactor preserves behavior, so the original implementation passes the same tests. The mutation check verifies sensitivity to behavior regressions.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Refs apache#1404. Preserve raw answer copying, pending deduplication, failure feedback and the 1400ms reset while removing the duplicate footer state machine. Add real TurnView copy-action coverage.

Generated-by: OpenAI Codex
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 6, 2026
@99Gaoxiaoqi 99Gaoxiaoqi changed the title Refactor/unify turn copy feedback refactor(ui): reuse clipboard feedback for turn footer Sep 6, 2026

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving at exact head ea4f9a16. This is a behaviour-preserving refactor and it holds up under checking. No P0–P2; one latent coupling worth a comment.

What I verified rather than took from the description

The shared hook is untouched. clipboard-feedback.ts changes only its header comment. Since useClipboardCopyFeedback also serves ToolActivity and the structured preview, that is the difference between a local refactor and a change to three call sites — worth stating plainly, because "reuse the shared hook" is exactly the shape that usually does modify it.

The hook's state machine is point-for-point what was removed. attempt guards re-entry on pendingCopyRef, calls clearResetTimer() before setting pending, settles through the unmount-guarded settle and schedules the reset at the supplied 1400 ms. That matches the deleted copyPendingRef / clearCopyResetTimer / settleCopy trio, including the case that is easy to lose in this kind of move: a second copy while a previous success is still displayed clears the outstanding timer instead of letting it fire early.

The tests discriminate the part that could silently regress. They render the real TurnView under StrictMode, copy a secret-shaped string so original-text copying is distinguishable from the hook's default redaction, walk the 1399 ms/1400 ms boundary, and assert that a clipboard write completing after unmount starts no timer at all. The description is also candid that these pass against the original implementation — correct for a refactor, and it names the mutation that does redden ({ redact: false } removed), which is the useful claim.

[P3] The empty-text guard now lives in one place instead of two

The removed copyAssistantText began with if (!props.assistantText || copyPendingRef.current) return;. The replacement passes props.assistantText ?? '', so the component itself no longer refuses an empty copy — it would write '' to the clipboard and report success.

This is not reachable today: turn-footer-actions.ts:148 builds the action with enabled: hasContent, and handleClick returns before touching the clipboard when an action is disabled. So the behaviour is unchanged in the product, which is why this is a note rather than a finding.

It is worth writing down because the guard moved from the component to its caller. If the copy action ever becomes unconditionally enabled — or another surface reuses TurnFooterActions with its own action list — an empty copy would overwrite whatever the user had on their clipboard, which is a worse failure than the no-op it replaced. A if (!props.assistantText) return; retained at the top of the copy branch would keep both guards without costing anything.

简体中文

在 exact head ea4f9a16 上批准。这是一次保持行为的重构,而且经得起核。 无 P0–P2;有一处潜在耦合值得写下来。

核过而不是采信的部分

共享 hook 没有被改动。 clipboard-feedback.ts 只改了文件头注释。由于 useClipboardCopyFeedback 同时服务 ToolActivity 与结构化预览,这就是「只改一处」与「改了三个调用点」的区别 —— 值得明说,因为「复用共享 hook」恰恰是通常会顺手改动它的那种形状。

hook 的状态机与被删掉的那套逐点对应。 attemptpendingCopyRef 挡重入,在设置 pending 之前调用 clearResetTimer(),经由带卸载守卫的 settle 落定,并按传入的 1400 ms 排定重置。这与被删的 copyPendingRef / clearCopyResetTimer / settleCopy 三件套一致,包括这类搬移里最容易丢掉的那一条:上一次成功仍在显示时再次复制,会清掉未触发的定时器,而不是让它提前触发。

测试对可能静默回退的那部分具有区分力。 它们在 StrictMode 下渲染真实的 TurnView,复制一个密钥形状的字符串以区分「原文复制」与 hook 默认的脱敏,走了 1399 ms / 1400 ms 的边界,并断言在卸载之后才完成的剪贴板写入不会排定任何定时器。描述也坦白这些测试在原实现上同样通过 —— 对重构而言这是对的,而且它点出了真正会变红的那个变异(去掉 { redact: false }),那才是有用的主张。

[P3] 空文本守卫从两处收敛到了一处

被删的 copyAssistantText 开头是 if (!props.assistantText || copyPendingRef.current) return;。替代实现传的是 props.assistantText ?? '',于是组件本身不再拒绝一次空复制 —— 它会把 '' 写进剪贴板并报告成功。

这在今天不可达:turn-footer-actions.ts:148enabled: hasContent 构造该动作,而 handleClick 在动作被禁用时会在触碰剪贴板之前返回。所以产品行为没有变化,这也是它是一条注记而非 finding 的原因。

之所以值得写下来,是因为守卫从组件搬到了它的调用方。如果那个复制动作将来变成无条件启用,或者另一个界面以自己的动作列表复用 TurnFooterActions,一次空复制就会覆盖用户剪贴板里原有的内容 —— 那比它所取代的「什么都不做」更糟。在 copy 分支顶部保留一句 if (!props.assistantText) return; 就能同时留住两道守卫,且不付出任何代价。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head ea4f9a165bb089396b323382a3afd8ec6e76107b. I found no P0-P3 issues in this refactor.

TurnFooterActions now delegates the duplicated state machine to useClipboardCopyFeedback (packages/ui/src/chat-turn.tsx:891-898). The shared implementation is unchanged apart from its comment, and the removed behavior remains intact: copy() still rejects zero-length text before writing (clipboard-feedback.ts:96-100), { redact: false } preserves the original assistant text, attempt() blocks overlapping writes and clears an earlier reset timer, and the mounted guard prevents post-unmount state/timer work (clipboard-feedback.ts:54-93). The fixed answer key is local to one hook instance, so it does not couple separate turn footers.

The added tests exercise the production TurnView footer under StrictMode and cover raw secret-shaped text, overlap suppression, the 1400 ms boundary, failure/retry, and late completion after unmount (chat-turn-answer-identity.test.tsx:412-482). Local verification passed: clean install; build:test; UI typecheck; UI 390/390 and focused 14/14 tests; full Biome lint and format; renderer architecture 101/101 plus the base-relative ratchet; E2E budget; ASF headers; changed-file Biome; and git diff --check. A fresh merge-tree against current main a5a99a6337997b1928e07f943f1d1b7ce72baab5 is clean and preserves all three PR files.

GitHub currently exposes only the successful effort-label check for this head, so hosted build/test gates have not been independently observed here. I did not run Electron or native platform validation; this three-file UI state-machine refactor was validated through the real component path instead.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Cover a second copy before feedback resets and cancellation of an active reset timer on unmount.

Generated-by: OpenAI Codex
@liugddx
liugddx merged commit c0229b0 into apache:main Sep 6, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants