fix(message-parts): isolate translation overlay renders - #17122
Conversation
Signed-off-by: suyao <sy20010504@gmail.com>
Signed-off-by: suyao <sy20010504@gmail.com>
There was a problem hiding this comment.
This review was translated automatically.
Found 1 blocker: The renderer-only transient translation streaming flag is added to the cross-process, persistable shared message-part contract; this requirement has a renderer-local feature-layer implementation, and currently does not meet the prerequisite approval and independent PR requirements for shared-contract infrastructure changes. Please address the inline comments before re-reviewing.
Original Content
发现 1 个 blocker：renderer-only 的瞬态 translation streaming 标记被加入跨进程、可持久化的 shared message-part contract；该需求存在 renderer-local 的 feature-layer 实现，当前也未满足 shared-contract 基础设施变更的前置批准与独立 PR 要求。请处理 inline comment 后再复审。
| targetLanguage: string | ||
| sourceLanguage?: string | ||
| sourceBlockId?: string | ||
| isStreaming?: boolean |
There was a problem hiding this comment.
This review comment was translated automatically.
[B3] Blocker: renderer-only transient state should not extend the cross-process, persistable message-part contract. TranslationPartData is a DTO shared between Main/Renderer and written to message.data.parts, while isStreaming is only produced by the renderer's overlay projection and has no independent cross-process value after the triggering feature is removed; this also creates an opportunity for the transient flag to be persisted when projected parts are later passed into the save path. The current requirement can be fulfilled at the feature layer, for example by placing isTranslationStreaming into a message-scoped value in MessagePartsScopeContext (or using a keyed renderer-local subscription), without changing the shared contract. Per the shared-contract/infrastructure rules in AGENTS.md, if the contract still needs to be changed, a standalone issue must first compare at least two approaches that don't modify the infrastructure, document compatibility/migration/risk/verification/rollback, and obtain direction approval before implementation, then land it in a separate PR; #17116 currently does not have this approval.
Original Content
[B3] Blocker：renderer-only 瞬态状态不应扩展跨进程、可持久化的 message-part contract。 TranslationPartData 是 Main/Renderer 共用并写入 message.data.parts 的 DTO，而 isStreaming 只由 renderer 的 overlay projection 产生，触发功能删除后没有独立的跨进程价值；这也让后续把 projected parts 传入保存路径时有机会把瞬态标记落库。当前需求可在 feature layer 完成，例如把 isTranslationStreaming 放进 MessagePartsScopeContext 的 message-scoped value（或用 keyed renderer-local subscription），无需改变 shared contract。按 AGENTS.md 的 shared-contract/基础设施规则，如仍要改该 contract，需要先在 standalone issue 比较至少两种不改基础设施的方案、记录兼容/迁移/风险/验证/回滚，并在实施前取得 direction approval，再用独立 PR 落地；#17116 当前没有该批准。
There was a problem hiding this comment.
This review was translated automatically.
Found 1 additional blocker: The translation-overlay path optimized in this PR has no callers in the current production code, and the actual menu translation still goes through a different set of actions. Therefore, the existing implementation and tests do not cover the triggerable product call chain. Another reviewer has already raised an equivalent point regarding the shared contract issue, so I won't submit a duplicate.
Original Content
发现 1 个额外 blocker：PR 优化的 translation-overlay 路径在当前生产代码中没有调用方，真实菜单翻译仍走另一套 action，因此现有实现和测试没有覆盖可触发的产品调用链。另一位 reviewer 已对 shared contract 问题提出等价意见，未重复提交。
| isActiveTurnProcessing && | ||
| (topicStreamStatus === undefined ? message.status === 'pending' : topicTurnState.isStreamLive) | ||
| const isTranslationOverlayActive = useTranslationOverlayEntry(message.id) !== undefined | ||
| const isTranslationOverlayActive = messageParts.some( |
There was a problem hiding this comment.
This review comment was translated automatically.
[A1] Blocker: This check currently has no production data source. In the current PR snapshot, useTranslateMessage has no production callers other than barrel exports, comments, and tests, so TranslationOverlaySetter will not write to the overlay during actual translation. The menu actually calls actions.translateMessage provided by homeMessageListAdapter, and that path also does not generate the isStreaming flag relied upon here. The new tests manually pass in scoped parts to construct states that the production call chain would never produce, so this change cannot fix the real UI. Please first wire the menu action into the overlay flow, or alternatively optimize the current actions.translateMessage path, and cover it with an integration test that goes through the production action.
Original Content
[A1] Blocker：这个判断目前没有生产数据源。 在当前 PR 快照中，useTranslateMessage 除 barrel 导出、注释和测试外没有生产调用方，因此 TranslationOverlaySetter 不会在实际翻译时写入 overlay；菜单实际调用的是 homeMessageListAdapter 提供的 actions.translateMessage，该路径也不会生成这里依赖的 isStreaming 标记。新增测试通过手工传入 scoped parts 构造了产品调用链不会产生的状态，所以这项改动无法修复真实 UI。请先把菜单动作接到 overlay 流程，或改为优化当前 actions.translateMessage 路径，并用经过生产 action 的集成测试覆盖。
|
Thanks for the review. I re-traced the production action and agree with both blockers.
The issue was therefore a false positive from the renderer audit. I am closing this PR rather than expanding its scope into a new translation architecture change. A future optimization of the production translation action should be measured and filed independently against that real path. |
3 participants
What this PR does
Before this PR: Every mounted
MessagePartsRenderersubscribed to the complete translation-overlay map, so each streamed translation chunk rerendered unrelated visible messages.After this PR: The transient translation part carries its streaming state through the existing message-scoped parts projection, so only the translated message rerenders while translation behavior and persistence handoff remain unchanged.
Fixes #17116
Why we need it and why it was done in this way
The following tradeoffs were made: Reuse the existing structurally shared message-parts boundary and add an ephemeral
isStreamingmarker to overlay-generated translation parts. Translation-part equality includes the marker so persisted translations correctly return to non-streaming rendering.The following alternatives were considered: A keyed external-store subscription for translation overlays. It would add a second subscription mechanism even though the existing message-scoped parts projection already isolates updates by message.
Links to places where the discussion took place: #17116
Breaking changes
None.
If this PR introduces breaking changes, please describe the changes and the impact on users.
Special notes for your reviewer
Focused renderer/translation tests passed: 64 tests across
MessagePartsRenderer,useStablePartsByMessageId, anduseTranslateMessage.pnpm formatpassed. The localpnpm build:checkcompleted lint, typechecks, i18n, formatting, and docs-link checks, but its full test phase did not complete becausebetter-sqlite3prebuild access failed in the sandbox and source compilation was interrupted. Remote CI is expected to run the complete gate.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note