diff --git a/docs/module-graph.i18n.yaml b/docs/module-graph.i18n.yaml index 51cc84c8bd..4d41c66eb4 100644 --- a/docs/module-graph.i18n.yaml +++ b/docs/module-graph.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write docs/module-graph.md -module-graph.md: cb897aed6f814c46c67c6d3e21b7b92be3f2cae8 -module-graph.zh.md: c479ff4884b74774ba657e4c2cb46956d0781ad9 +module-graph.md: c76223ecf28aaa28cfe95633489f3249c3772c20 +module-graph.zh.md: 6b0da4c52db680ae8c24385490721a1094a38bc4 diff --git a/docs/module-graph.md b/docs/module-graph.md index cb897aed6f..c76223ecf2 100644 --- a/docs/module-graph.md +++ b/docs/module-graph.md @@ -1245,6 +1245,7 @@ flowchart TD pkg_client_ui_deliverables --> pkg_client_ui_slots pkg_client_ui_deliverables --> pkg_invariants pkg_client_ui_feedback --> pkg_api_remotes + pkg_client_ui_feedback --> pkg_client_connection pkg_client_ui_feedback --> pkg_client_locale pkg_client_ui_feedback --> pkg_client_runtime pkg_client_ui_feedback --> pkg_client_ui_conversation @@ -1537,7 +1538,7 @@ flowchart TD | [`client-ui-agent-preset`](../packages/client/ui-agent-preset) | `client` | [`api-remotes`](../packages/api/remotes), [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-settings`](../packages/client/ui-settings), [`client-ui-slots`](../packages/client/ui-slots), [`client-web-react`](../packages/client/web-react), [`invariants`](../packages/support/invariants) | | [`client-ui-command`](../packages/client/ui-command) | `client` | [`api-remotes`](../packages/api/remotes), [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slash`](../packages/client/ui-slash), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants) | | [`client-ui-deliverables`](../packages/client/ui-deliverables) | `client` | [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants) | -| [`client-ui-feedback`](../packages/client/ui-feedback) | `client` | [`api-remotes`](../packages/api/remotes), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`message-feedback`](../packages/feedback/message-feedback) | +| [`client-ui-feedback`](../packages/client/ui-feedback) | `client` | [`api-remotes`](../packages/api/remotes), [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`message-feedback`](../packages/feedback/message-feedback) | | [`client-ui-goal`](../packages/client/ui-goal) | `client` | [`api-remotes`](../packages/api/remotes), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`commands`](../packages/interaction/commands), [`goal`](../packages/goal/goal), [`invariants`](../packages/support/invariants) | | [`client-ui-plan`](../packages/client/ui-plan) | `client` | [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`plan-mode`](../packages/plan/plan-mode) | | [`client-ui-subagent`](../packages/client/ui-subagent) | `client` | [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slash`](../packages/client/ui-slash), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`subagent`](../packages/subagent/subagent), [`token-meter`](../packages/llm/token-meter) | diff --git a/docs/module-graph.zh.md b/docs/module-graph.zh.md index c479ff4884..6b0da4c52d 100644 --- a/docs/module-graph.zh.md +++ b/docs/module-graph.zh.md @@ -1247,6 +1247,7 @@ flowchart TD pkg_client_ui_deliverables --> pkg_client_ui_slots pkg_client_ui_deliverables --> pkg_invariants pkg_client_ui_feedback --> pkg_api_remotes + pkg_client_ui_feedback --> pkg_client_connection pkg_client_ui_feedback --> pkg_client_locale pkg_client_ui_feedback --> pkg_client_runtime pkg_client_ui_feedback --> pkg_client_ui_conversation @@ -1539,7 +1540,7 @@ flowchart TD | [`client-ui-agent-preset`](../packages/client/ui-agent-preset) | `client` | [`api-remotes`](../packages/api/remotes), [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-settings`](../packages/client/ui-settings), [`client-ui-slots`](../packages/client/ui-slots), [`client-web-react`](../packages/client/web-react), [`invariants`](../packages/support/invariants) | | [`client-ui-command`](../packages/client/ui-command) | `client` | [`api-remotes`](../packages/api/remotes), [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slash`](../packages/client/ui-slash), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants) | | [`client-ui-deliverables`](../packages/client/ui-deliverables) | `client` | [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants) | -| [`client-ui-feedback`](../packages/client/ui-feedback) | `client` | [`api-remotes`](../packages/api/remotes), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`message-feedback`](../packages/feedback/message-feedback) | +| [`client-ui-feedback`](../packages/client/ui-feedback) | `client` | [`api-remotes`](../packages/api/remotes), [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`message-feedback`](../packages/feedback/message-feedback) | | [`client-ui-goal`](../packages/client/ui-goal) | `client` | [`api-remotes`](../packages/api/remotes), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`commands`](../packages/interaction/commands), [`goal`](../packages/goal/goal), [`invariants`](../packages/support/invariants) | | [`client-ui-plan`](../packages/client/ui-plan) | `client` | [`client-connection`](../packages/client/connection), [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`plan-mode`](../packages/plan/plan-mode) | | [`client-ui-subagent`](../packages/client/ui-subagent) | `client` | [`client-locale`](../packages/client/locale), [`client-runtime`](../packages/client/runtime), [`client-ui-conversation`](../packages/client/ui-conversation), [`client-ui-primitives`](../packages/client/ui-primitives), [`client-ui-slash`](../packages/client/ui-slash), [`client-ui-slots`](../packages/client/ui-slots), [`invariants`](../packages/support/invariants), [`subagent`](../packages/subagent/subagent), [`token-meter`](../packages/llm/token-meter) | diff --git a/docs/subsystems/feedback.i18n.yaml b/docs/subsystems/feedback.i18n.yaml index 111182d7ed..1a96a2db7e 100644 --- a/docs/subsystems/feedback.i18n.yaml +++ b/docs/subsystems/feedback.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write docs/subsystems/feedback.md -feedback.md: 03a14b40968ab27b6321dc82bd12e8af57ddf5f3 -feedback.zh.md: 68da42322dad515ac185296da66401c522fac93c +feedback.md: 21398c8e0b28c1d6de223a0c1887b249084f3f26 +feedback.zh.md: a149b4c53e4cb22faeab7b4eda74e0b95e30cc53 diff --git a/docs/subsystems/feedback.md b/docs/subsystems/feedback.md index 03a14b4096..21398c8e0b 100644 --- a/docs/subsystems/feedback.md +++ b/docs/subsystems/feedback.md @@ -205,7 +205,7 @@ Plugin disposal closes mutation admission, drains accepted per-Session queue wor [`@deepseek-ai/dsh-client-ui-feedback`](../../packages/client/ui-feedback) is the browser consumer. `@deepseek-ai/dsh-api-remotes` mounts the generated `messageFeedback` contribution, so the plugin calls `ctx.remote.messageFeedback` and never touches the transport. -The controls are the `feedback` entry (order 10) of the `conversation.chat.assistant-actions` list slot, which `ui-conversation` declares and renders inside the finalized assistant message's IconActions row. Reaching that render site required one plumbing change: `AssistantMessageNode` now carries the optional `messageId` from the `assistant/message` event. The field is absent on interruption-frozen partials, and the render site skips the slot when it is absent, so only messages the Host accepts as feedback targets present controls. +The controls are the `feedback` entry (order 10) of the `conversation.chat.assistant-actions` list slot, which `ui-conversation` declares and renders inside the finalized assistant message's IconActions row. Reaching that render site required one plumbing change: `AssistantMessageNode` now carries the optional `messageId` from the `assistant/message` event. The field is absent on interruption-frozen partials, and the render site skips the slot when it is absent. The strip renders once per turn, on the closing assistant message: the Host accepts every append-origin step message as a target, but earlier steps of a multi-step turn render tool rows rather than a rateable body, so the UI exposes a narrower set than the Host contract allows. One `FeedbackController` per Session backs every message control in that Session: a single `list` read seeds the whole transcript, deferred to first hover or focus rather than fired on mount. Each mutation sends the version that controller last observed as `ifVersion`; a `version-conflict` reply carries the authoritative item, so the controller reconciles from the reply instead of refetching. Mutations serialize per Session so a queued operation compares against the committed version. A `connection/reset` refreshes only Sessions already read. diff --git a/docs/subsystems/feedback.zh.md b/docs/subsystems/feedback.zh.md index 68da42322d..a149b4c53e 100644 --- a/docs/subsystems/feedback.zh.md +++ b/docs/subsystems/feedback.zh.md @@ -205,7 +205,7 @@ Plugin disposal 会先关闭变更接纳,排空已进入各 Session 队列的 [`@deepseek-ai/dsh-client-ui-feedback`](../../packages/client/ui-feedback) 是浏览器侧消费方。`@deepseek-ai/dsh-api-remotes` 挂载生成的 `messageFeedback` 贡献,因此该插件调用 `ctx.remote.messageFeedback`,不接触传输层。 -控件是 `conversation.chat.assistant-actions` list slot 的 `feedback` 条目(order 10),该 slot 由 `ui-conversation` 声明,并渲染在已定稿助手消息的 IconActions 行内。为抵达该渲染点需要一处管道改动:`AssistantMessageNode` 现在携带来自 `assistant/message` 事件的可选 `messageId`。被中断冻结的部分输出没有该字段,渲染点在字段缺失时跳过该 slot,因此只有 Host 认可为反馈目标的消息才会出现控件。 +控件是 `conversation.chat.assistant-actions` list slot 的 `feedback` 条目(order 10),该 slot 由 `ui-conversation` 声明,并渲染在已定稿助手消息的 IconActions 行内。为抵达该渲染点需要一处管道改动:`AssistantMessageNode` 现在携带来自 `assistant/message` 事件的可选 `messageId`。被中断冻结的部分输出没有该字段,渲染点在字段缺失时跳过该 slot。该操作栏每个 Turn 渲染一次,位于收尾的助手消息上:Host 接受每条 append-origin 步骤消息作为目标,但多步骤 Turn 中较早的步骤渲染的是工具行而非可评分正文,因此 UI 暴露的范围比 Host 契约允许的更窄。 每个 Session 一个 `FeedbackController`,支撑该 Session 内所有消息的控件:一次 `list` 读取即填充整段对话,且延迟到首次 hover 或 focus 才发起,而非挂载时触发。每次变更把该 controller 最后观察到的版本作为 `ifVersion` 发送;`version-conflict` 响应携带权威条目,controller 据此对账而不重新拉取。变更按 Session 串行,排队操作与已提交版本比较。`connection/reset` 只刷新已读取过的 Session。 diff --git a/packages/client/ui-feedback/README.i18n.yaml b/packages/client/ui-feedback/README.i18n.yaml index 1f9cdea206..daa4d8d607 100644 --- a/packages/client/ui-feedback/README.i18n.yaml +++ b/packages/client/ui-feedback/README.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write packages/client/ui-feedback/README.md -README.md: 2f347a22427ce61ea1093434273ba9b4ba759852 -README.zh.md: 2aea4cd2f4c29312f2c771094b7831171bff760f +README.md: 0187fee61863292ee4e51975efe414a4e41ba066 +README.zh.md: 9d225f4632e15a2b54437cbe30d376acb509a156 diff --git a/packages/client/ui-feedback/README.md b/packages/client/ui-feedback/README.md index 2f347a2242..0187fee618 100644 --- a/packages/client/ui-feedback/README.md +++ b/packages/client/ui-feedback/README.md @@ -2,7 +2,7 @@ English | [中文](README.zh.md) -Per-message feedback plugin, browser half: a Like/Dislike pair plus an optional note, contributed as the `feedback` entry (order 10) of the `conversation.chat.assistant-actions` strip. The strip is declared by `ui-conversation` and rendered inside the finalized assistant message's IconActions row, between copy and branch, so the controls inherit that row's chrome and hover behavior. Only finalized messages reach the slot — an interruption-frozen partial carries no `messageId` and therefore no feedback controls. +Per-message feedback plugin, browser half: a Like/Dislike pair plus an optional note, contributed as the `feedback` entry (order 10) of the `conversation.chat.assistant-actions` strip. The strip is declared by `ui-conversation` and rendered inside the finalized assistant message's IconActions row, between copy and branch, so the controls inherit that row's chrome and hover behavior. Only finalized messages reach the slot — an interruption-frozen partial carries no `messageId` and therefore no feedback controls. The strip renders once per turn, on the closing assistant message that owns the turn's IconActions row: earlier steps of a multi-step turn produce tool rows rather than a rateable body, so they present no controls even though the Host would accept them as targets. One `FeedbackController` per Session backs every message control in that Session, so a single `messageFeedback.list` read seeds the whole transcript. The read is deferred to the first hover or focus rather than fired on mount, because the controls mount once per settled message in the visible history. diff --git a/packages/client/ui-feedback/README.zh.md b/packages/client/ui-feedback/README.zh.md index 2aea4cd2f4..9d225f4632 100644 --- a/packages/client/ui-feedback/README.zh.md +++ b/packages/client/ui-feedback/README.zh.md @@ -2,7 +2,7 @@ [English](README.md) | 中文 -单条消息反馈插件的浏览器侧:一对 Like/Dislike 按钮加一个可选备注,作为 `conversation.chat.assistant-actions` 条带的 `feedback` 条目(order 10)贡献。该条带由 `ui-conversation` 声明,渲染在已定稿助手消息的 IconActions 行内、复制与分支之间,因此控件沿用该行的样式与 hover 行为。只有已定稿的消息能到达这个 slot——被中断冻结的部分输出不带 `messageId`,因此也没有反馈控件。 +单条消息反馈插件的浏览器侧:一对 Like/Dislike 按钮加一个可选备注,作为 `conversation.chat.assistant-actions` 条带的 `feedback` 条目(order 10)贡献。该条带由 `ui-conversation` 声明,渲染在已定稿助手消息的 IconActions 行内、复制与分支之间,因此控件沿用该行的样式与 hover 行为。只有已定稿的消息能到达这个 slot——被中断冻结的部分输出不带 `messageId`,因此也没有反馈控件。该操作栏每个 Turn 渲染一次,位于持有该 Turn IconActions 行的收尾助手消息上:多步骤 Turn 中较早的步骤产出的是工具行而非可评分正文,因此即使 Host 会接受它们作为目标,界面上也不出现控件。 每个 Session 一个 `FeedbackController`,支撑该 Session 内所有消息的控件,因此一次 `messageFeedback.list` 读取即可填充整段对话。该读取延迟到首次 hover 或 focus 才发起,而不是在挂载时触发,因为可见历史中每条已结束的消息都会挂载一次控件。 diff --git a/packages/client/ui-feedback/package.json b/packages/client/ui-feedback/package.json index d4cf2e70eb..bd4f269f1e 100644 --- a/packages/client/ui-feedback/package.json +++ b/packages/client/ui-feedback/package.json @@ -1,7 +1,7 @@ { "name": "@deepseek-ai/dsh-client-ui-feedback", "description": "Per-message feedback controls contributed to the assistant-message action strip, backed by the messageFeedback Host Remote", - "version": "0.0.1-rc.1", + "version": "0.0.1-rc.2", "publishConfig": { "access": "restricted" }, @@ -47,6 +47,7 @@ "license": "BSD-3-Clause", "peerDependencies": { "@deepseek-ai/dsh-api-remotes": "workspace:^", + "@deepseek-ai/dsh-client-connection": "workspace:^", "@deepseek-ai/dsh-client-locale": "workspace:^", "@deepseek-ai/dsh-client-runtime": "workspace:^", "@deepseek-ai/dsh-client-ui-conversation": "workspace:^", @@ -59,6 +60,7 @@ }, "devDependencies": { "@deepseek-ai/dsh-api-remotes": "workspace:^", + "@deepseek-ai/dsh-client-connection": "workspace:^", "@deepseek-ai/dsh-client-locale": "workspace:^", "@deepseek-ai/dsh-client-runtime": "workspace:^", "@deepseek-ai/dsh-client-test-runtime": "workspace:^", @@ -67,9 +69,9 @@ "@deepseek-ai/dsh-client-ui-slots": "workspace:^", "@deepseek-ai/dsh-invariants": "workspace:^", "@deepseek-ai/dsh-message-feedback": "workspace:^", + "@deepseek-ai/cordis": "workspace:^", "@testing-library/react": "^16.1.0", "@types/react": "~18.3.1", - "@deepseek-ai/cordis": "workspace:^", "react": "^18.2.0", "react-dom": "^18.2.0" }, diff --git a/packages/client/ui-feedback/src/client/FeedbackActions.tsx b/packages/client/ui-feedback/src/client/FeedbackActions.tsx index 75bd92c171..28b0c18025 100644 --- a/packages/client/ui-feedback/src/client/FeedbackActions.tsx +++ b/packages/client/ui-feedback/src/client/FeedbackActions.tsx @@ -19,8 +19,9 @@ import css from './FeedbackActions.module.css' * shared feedback hook. * @returns the rating buttons, plus the note editor while it is open. */ -export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t }: FeedbackActionProps) { +export function FeedbackActions({ messageId, ensure, rate, toggle, clearNote, useFeedback, t }: FeedbackActionProps) { const item = useFeedback(view => view.items.get(messageId)) + const loadFailed = useFeedback(view => view.status === 'error') const rating = item?.rating const [noteOpen, setNoteOpen] = useState(false) const [draft, setDraft] = useState('') @@ -51,14 +52,12 @@ export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t const onRate = useCallback((next: MessageFeedbackRating) => { setPending(true) setFailure(null) - // Re-clicking the active rating retracts it; the note goes with it. - if (rating === next) { - setNoteOpen(false) - void clear(messageId).then(settle) - return - } - void rate(messageId, next, item?.note).then(settle) - }, [clear, item?.note, messageId, rate, rating, settle]) + // The controller decides retract-vs-replace from the committed item, so a + // click that lands before the first list read still toggles the stored + // value instead of this render's empty view. + setNoteOpen(false) + void toggle(messageId, next).then(settle) + }, [messageId, settle, toggle]) // The rating is a parameter because only the note editor's render site can // prove one is recorded; that removes an unreachable undefined guard here. @@ -66,11 +65,16 @@ export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t const trimmed = draft.trim() setPending(true) setFailure(null) - void rate(messageId, current, trimmed.length === 0 ? undefined : trimmed).then((result) => { + // An emptied editor removes the note explicitly; `rate` alone preserves a + // stored note, so it cannot express deletion. + const settled = trimmed.length === 0 + ? clearNote(messageId) + : rate(messageId, current, trimmed) + void settled.then((result) => { settle(result) if (result.ok && alive.current) setNoteOpen(false) }) - }, [draft, messageId, rate, settle]) + }, [clearNote, draft, messageId, rate, settle]) const openNote = useCallback(() => { setDraft(item?.note ?? '') @@ -140,6 +144,9 @@ export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t )} + {failure === null && loadFailed && ( + {t('error.load')} + )} {failure !== null && {failure}} ) diff --git a/packages/client/ui-feedback/src/client/controller.ts b/packages/client/ui-feedback/src/client/controller.ts index c01366e34f..d9c75061cf 100644 --- a/packages/client/ui-feedback/src/client/controller.ts +++ b/packages/client/ui-feedback/src/client/controller.ts @@ -51,7 +51,11 @@ export type FeedbackActionResult = | { ok: true } | { ok: false; error: { code: string; message: string } } -const EMPTY_ITEMS: ReadonlyMap = Object.freeze(new Map()) +// `Object.freeze` does not protect a Map: `set`/`delete` write internal slots, +// not properties. Immutability here is by discipline instead — the view type is +// ReadonlyMap and every publish hands over a freshly built Map that this class +// keeps no mutable reference to. +const EMPTY_ITEMS: ReadonlyMap = new Map() const INITIAL_VIEW: FeedbackView = Object.freeze({ status: 'cold', @@ -61,6 +65,11 @@ const INITIAL_VIEW: FeedbackView = Object.freeze({ const OK: FeedbackActionResult = Object.freeze({ ok: true }) +const DISPOSED: FeedbackActionResult = Object.freeze({ + ok: false, + error: Object.freeze({ code: 'disposed', message: 'feedback controller is disposed' }), +}) + /** Human-readable text for one business failure code. */ function describe(code: string): string { switch (code) { @@ -119,6 +128,11 @@ export class FeedbackController implements HostObservable { /** * Re-read the authoritative list, collapsing concurrent callers onto one * in-flight read. + * + * This is the unserialized read used to seed a cold controller, where no + * mutation can be in flight yet. A reconnect must use {@link resync} instead: + * an unserialized list response can otherwise arrive after a newer mutation's + * reply and overwrite the version that mutation just committed. * @returns the settled reload result. */ refresh(): Promise { @@ -129,12 +143,29 @@ export class FeedbackController implements HostObservable { return pending.finally(() => { this.loadPromise = null }) } + /** + * Re-read the list behind this Session's queued mutations, so a reconnect + * cannot resurrect a version an in-flight mutation already replaced. + * @returns the settled reload result. + */ + resync(): Promise { + // seed: false — this operation *is* the read, so pre-seeding would either + // short-circuit it (status already ready) or run it twice. + return this.mutate(() => this.refresh(), { seed: false }) + } + /** * Create or replace feedback for one message, comparing against the version * this controller last observed. + * + * The note is resolved here rather than by the caller: `mutate` awaits the + * one list read first, so this body always sees the committed item, while a + * control that rendered before that read completed would still be holding + * `undefined`. Omitting `note` therefore keeps whatever is stored; only + * {@link clearNote} removes one. * @param messageId - target assistant message. * @param rating - desired judgment. - * @param note - optional explanation; omitted leaves the note unset. + * @param note - replacement explanation; omitted keeps the stored note. * @returns the settled mutation result. */ rate( @@ -144,21 +175,38 @@ export class FeedbackController implements HostObservable { ): Promise { return this.mutate(async () => { const observed = this.view.items.get(messageId) - const result = await this.remote.put({ - sessionId: this.sessionId, - messageId, - rating, - ...(note === undefined ? {} : { note }), - ifVersion: observed?.version ?? null, - }) - if (result.ok) { - this.commit(messageId, result.value) - return OK - } - if (result.error.code === 'version-conflict') { - this.commit(messageId, result.error.current) - } - return fail(result.error.code) + return await this.putCommitted(messageId, rating, note ?? observed?.note, observed) + }) + } + + /** + * Replace one message's rating with the opposite judgment, or retract it when + * the committed rating already matches. The decision reads the committed item + * inside the serialized mutation, so a click that lands before the first list + * read still toggles against the stored value rather than the empty view a + * cold control rendered. + * @param messageId - target assistant message. + * @param rating - the judgment the human asked for. + * @returns the settled mutation result. + */ + toggle(messageId: MessageId, rating: MessageFeedbackRating): Promise { + return this.mutate(async () => { + const observed = this.view.items.get(messageId) + if (observed?.rating === rating) return await this.deleteCommitted(messageId, observed) + return await this.putCommitted(messageId, rating, observed?.note, observed) + }) + } + + /** + * Drop the note while keeping the rating. Absent feedback needs no call. + * @param messageId - target assistant message. + * @returns the settled mutation result. + */ + clearNote(messageId: MessageId): Promise { + return this.mutate(async () => { + const observed = this.view.items.get(messageId) + if (observed === undefined || observed.note === undefined) return OK + return await this.putCommitted(messageId, observed.rating, undefined, observed) }) } @@ -172,22 +220,50 @@ export class FeedbackController implements HostObservable { return this.mutate(async () => { const observed = this.view.items.get(messageId) if (observed === undefined) return OK - const result = await this.remote.delete({ - sessionId: this.sessionId, - messageId, - ifVersion: observed.version, - }) - if (result.ok) { - this.commit(messageId, null) - return OK - } - if (result.error.code === 'version-conflict') { - this.commit(messageId, result.error.current) - } - return fail(result.error.code) + return await this.deleteCommitted(messageId, observed) }) } + /** Commit one put against the observed version and reconcile a conflict. */ + private async putCommitted( + messageId: MessageId, + rating: MessageFeedbackRating, + note: string | undefined, + observed: MessageFeedbackItem | undefined, + ): Promise { + const result = await this.remote.put({ + sessionId: this.sessionId, + messageId, + rating, + ...(note === undefined ? {} : { note }), + ifVersion: observed?.version ?? null, + }) + if (result.ok) { + this.commit(messageId, result.value) + return OK + } + if (result.error.code === 'version-conflict') this.commit(messageId, result.error.current) + return fail(result.error.code) + } + + /** Commit one delete against the observed version and reconcile a conflict. */ + private async deleteCommitted( + messageId: MessageId, + observed: MessageFeedbackItem, + ): Promise { + const result = await this.remote.delete({ + sessionId: this.sessionId, + messageId, + ifVersion: observed.version, + }) + if (result.ok) { + this.commit(messageId, null) + return OK + } + if (result.error.code === 'version-conflict') this.commit(messageId, result.error.current) + return fail(result.error.code) + } + /** Drop subscribers and refuse further work when the owning fiber unloads. */ dispose(): void { this.disposed = true @@ -205,7 +281,7 @@ export class FeedbackController implements HostObservable { } const items = new Map() for (const item of result.value.items) items.set(item.messageId, item) - this.publish({ status: 'ready', items: Object.freeze(items), error: null }) + this.publish({ status: 'ready', items, error: null }) return OK } catch (error) { if (this.disposed) return OK @@ -220,11 +296,20 @@ export class FeedbackController implements HostObservable { * operations always compare against the committed version, and translate a * transport throw into the same settled shape the controls already render. */ - private mutate(operation: () => Promise): Promise { + private mutate( + operation: () => Promise, + options: { readonly seed?: boolean } = {}, + ): Promise { const guarded = async (): Promise => { - if (this.disposed) return { ok: false, error: { code: 'disposed', message: 'feedback controller is disposed' } } - const loaded = await this.ensure() - if (!loaded.ok) return loaded + if (this.disposed) return DISPOSED + if (options.seed !== false) { + const loaded = await this.ensure() + if (!loaded.ok) return loaded + // Disposal can land while the seeding read is in flight; without this + // second check the fiber would still reach the wire after unloading. + // oxlint-disable-next-line typescript/no-unnecessary-condition -- dispose() can run during the await. + if (this.disposed) return DISPOSED + } try { return await operation() } catch (error) { @@ -255,7 +340,7 @@ export class FeedbackController implements HostObservable { const items = new Map(this.view.items) if (item === null) items.delete(messageId) else items.set(messageId, item) - this.publish({ status: 'ready', items: Object.freeze(items), error: null }) + this.publish({ status: 'ready', items, error: null }) } /** Replace the view and contain subscriber failures at the observable boundary. */ diff --git a/packages/client/ui-feedback/src/client/index.ts b/packages/client/ui-feedback/src/client/index.ts index 21c5965b0b..f24f2e8759 100644 --- a/packages/client/ui-feedback/src/client/index.ts +++ b/packages/client/ui-feedback/src/client/index.ts @@ -19,8 +19,6 @@ import { FeedbackActions } from './FeedbackActions.tsx' import type { FeedbackInjected } from './slots.ts' import { en, zh } from './locales.ts' -export { FeedbackActions } from './FeedbackActions.tsx' -export { FeedbackController } from './controller.ts' export type { FeedbackActionResult, FeedbackStatus, FeedbackView, MessageFeedbackRemote, } from './controller.ts' @@ -55,7 +53,7 @@ export function apply(ctx: ClientContext): void { // stays cold until something asks for it. ctx.on('connection/reset', () => { for (const controller of controllers.values()) { - if (controller.getSnapshot().status !== 'cold') void controller.refresh() + if (controller.getSnapshot().status !== 'cold') void controller.resync() } }) @@ -71,6 +69,8 @@ export function apply(ctx: ClientContext): void { hooks: { feedback: controller }, ensure: () => controller.ensure(), rate: (messageId, rating, note) => controller.rate(messageId, rating, note), + toggle: (messageId, rating) => controller.toggle(messageId, rating), + clearNote: messageId => controller.clearNote(messageId), clear: messageId => controller.clear(messageId), } }, diff --git a/packages/client/ui-feedback/src/client/locales.ts b/packages/client/ui-feedback/src/client/locales.ts index 81aeee7eb6..b4139be596 100644 --- a/packages/client/ui-feedback/src/client/locales.ts +++ b/packages/client/ui-feedback/src/client/locales.ts @@ -12,6 +12,7 @@ export const zh = { 'note.cancel': '取消', 'note.aria': '反馈说明', 'error.conflict': '这条反馈已在别处改动,已显示最新状态', + 'error.load': '反馈状态加载失败', 'error.generic': '反馈保存失败', } satisfies Record @@ -37,5 +38,6 @@ export const en = { 'note.cancel': 'Cancel', 'note.aria': 'Feedback note', 'error.conflict': 'This feedback changed elsewhere; the latest state is shown', + 'error.load': 'Could not load feedback', 'error.generic': 'Could not save feedback', } satisfies Record diff --git a/packages/client/ui-feedback/src/client/slots.ts b/packages/client/ui-feedback/src/client/slots.ts index 4c5d519f2a..ee171b0891 100644 --- a/packages/client/ui-feedback/src/client/slots.ts +++ b/packages/client/ui-feedback/src/client/slots.ts @@ -37,6 +37,19 @@ export interface FeedbackInjected { rating: MessageFeedbackRating, note?: string, ) => Promise + /** + * Apply the requested judgment, retracting instead when the committed rating + * already matches. The controller decides from the committed item, so a click + * before the first list read still toggles the stored value. + * @param messageId - target assistant message. + * @param rating - the judgment the human asked for. + */ + toggle: (messageId: MessageId, rating: MessageFeedbackRating) => Promise + /** + * Drop the note while keeping the rating. + * @param messageId - target assistant message. + */ + clearNote: (messageId: MessageId) => Promise /** * Remove this Session's feedback for one message. * @param messageId - target assistant message. diff --git a/packages/client/ui-feedback/tests/browser-plugin.spec.tsx b/packages/client/ui-feedback/tests/browser-plugin.spec.tsx index 343c10e177..084429c9ab 100644 --- a/packages/client/ui-feedback/tests/browser-plugin.spec.tsx +++ b/packages/client/ui-feedback/tests/browser-plugin.spec.tsx @@ -145,6 +145,21 @@ describe('ui-feedback browser plugin', () => { }) }) + it('routes toggle and clearNote to the controller', async () => { + const b = await bench() + await b.fiber.await() + + const face = b.entry()!.inject!(sid('s1')) + expect(await face.toggle(MSG, 'negative')).toEqual({ ok: true }) + expect(await face.clearNote(MSG)).toEqual({ ok: true }) + + // The seeded item is positive with no note, so a negative toggle replaces it + // through put, and clearNote has nothing to drop and touches no wire. + const puts = b.calls.filter(call => call.method === 'put').map(call => call.request) + expect(puts).toHaveLength(1) + expect(puts[0]).toMatchObject({ messageId: MSG, rating: 'negative' }) + }) + it('refreshes only Sessions already read when the connection resets', async () => { const b = await bench() await b.fiber.await() diff --git a/packages/client/ui-feedback/tests/controller.spec.ts b/packages/client/ui-feedback/tests/controller.spec.ts index 2de917d30d..fa0fed162a 100644 --- a/packages/client/ui-feedback/tests/controller.spec.ts +++ b/packages/client/ui-feedback/tests/controller.spec.ts @@ -474,4 +474,142 @@ describe('FeedbackController', () => { }) expect(controller.getSnapshot().items.get(MSG)).toEqual(existing) }) + + it('preserves a stored note when a rating switch omits one', async () => { + // Regression: a control that rendered before the first list read holds no + // item, so it passes note=undefined; that must not erase the stored note. + const stored = item({ version: version('v1'), rating: 'positive', note: 'keep me' }) + const { remote, calls } = fakeRemote({ + list: () => Promise.resolve({ ok: true, value: { items: [stored] } }), + }) + const controller = new FeedbackController(remote, SESSION) + + expect(await controller.rate(MSG, 'negative')).toEqual({ ok: true }) + + const put = calls.filter(c => c.method === 'put')[0]?.request as Record + expect(put.note).toBe('keep me') + expect(put.rating).toBe('negative') + }) + + it('toggle retracts when the committed rating already matches', async () => { + const stored = item({ version: version('v1'), rating: 'positive' }) + const { remote, calls } = fakeRemote({ + list: () => Promise.resolve({ ok: true, value: { items: [stored] } }), + }) + const controller = new FeedbackController(remote, SESSION) + + expect(await controller.toggle(MSG, 'positive')).toEqual({ ok: true }) + + expect(calls.filter(c => c.method === 'delete')).toHaveLength(1) + expect(calls.filter(c => c.method === 'put')).toHaveLength(0) + expect(controller.getSnapshot().items.has(MSG)).toBe(false) + }) + + it('toggle decides from the committed item, not a cold view', async () => { + // The click lands before any list read: the cold view knows no item, yet the + // stored rating matches, so the toggle must retract rather than re-put. + const stored = item({ version: version('v1'), rating: 'positive', note: 'kept' }) + const { remote, calls } = fakeRemote({ + list: () => Promise.resolve({ ok: true, value: { items: [stored] } }), + }) + const controller = new FeedbackController(remote, SESSION) + expect(controller.getSnapshot().status).toBe('cold') + + expect(await controller.toggle(MSG, 'positive')).toEqual({ ok: true }) + + expect(calls.filter(c => c.method === 'delete')).toHaveLength(1) + }) + + it('toggle replaces the opposite rating and carries the note forward', async () => { + const stored = item({ version: version('v1'), rating: 'positive', note: 'kept' }) + const { remote, calls } = fakeRemote({ + list: () => Promise.resolve({ ok: true, value: { items: [stored] } }), + }) + const controller = new FeedbackController(remote, SESSION) + + expect(await controller.toggle(MSG, 'negative')).toEqual({ ok: true }) + + const put = calls.filter(c => c.method === 'put')[0]?.request as Record + expect(put).toMatchObject({ rating: 'negative', note: 'kept', ifVersion: version('v1') }) + }) + + it('clearNote drops the note and keeps the rating', async () => { + const stored = item({ version: version('v1'), rating: 'negative', note: 'remove me' }) + const { remote, calls } = fakeRemote({ + list: () => Promise.resolve({ ok: true, value: { items: [stored] } }), + }) + const controller = new FeedbackController(remote, SESSION) + + expect(await controller.clearNote(MSG)).toEqual({ ok: true }) + + const put = calls.filter(c => c.method === 'put')[0]?.request as Record + expect(put.rating).toBe('negative') + expect(put).not.toHaveProperty('note') + }) + + it('clearNote is a no-op when there is no note to drop', async () => { + const { remote, calls } = fakeRemote({ + list: () => Promise.resolve({ ok: true, value: { items: [item()] } }), + }) + const controller = new FeedbackController(remote, SESSION) + + expect(await controller.clearNote(MSG)).toEqual({ ok: true }) + expect(calls.filter(c => c.method === 'put')).toHaveLength(0) + }) + + it('resync serializes behind an in-flight mutation', async () => { + // Regression: an unserialized reconnect read could land after a newer put + // and resurrect the version that put had already replaced. + const order: string[] = [] + let releasePut = (): void => {} + const putGate = new Promise((r) => { releasePut = r }) + const { remote } = fakeRemote({ + list: () => { + order.push('list') + return Promise.resolve({ ok: true, value: { items: [item({ version: version('v1') })] } }) + }, + put: async () => { + order.push('put:start') + await putGate + order.push('put:end') + return { ok: true, value: item({ version: version('v9'), rating: 'negative' }) } + }, + }) + const controller = new FeedbackController(remote, SESSION) + await controller.ensure() + + const rating = controller.rate(MSG, 'negative') + const resync = controller.resync() + releasePut() + await Promise.all([rating, resync]) + + // The reconnect read runs only after the mutation settled. + expect(order.indexOf('list', 1)).toBeGreaterThan(order.indexOf('put:end')) + }) + + it('refuses a mutation disposed while its seeding read is in flight', async () => { + // Dispose only once the seeding list call has actually started, so the + // mutation is already past the admission check and must be stopped by the + // second guard that runs after ensure() resolves. + let release = (): void => {} + const gate = new Promise((r) => { release = r }) + let started = (): void => {} + const listStarted = new Promise((r) => { started = r }) + const { remote, calls } = fakeRemote({ + list: async () => { + started() + await gate + return { ok: true, value: { items: [] } } + }, + }) + const controller = new FeedbackController(remote, SESSION) + const pending = controller.rate(MSG, 'positive') + + await listStarted + controller.dispose() + release() + + expect(await pending).toMatchObject({ ok: false, error: { code: 'disposed' } }) + expect(calls.filter(c => c.method === 'put')).toHaveLength(0) + }) }) diff --git a/packages/client/ui-feedback/tests/feedback-actions.spec.tsx b/packages/client/ui-feedback/tests/feedback-actions.spec.tsx index 20eb4d0ac4..33004879f9 100644 --- a/packages/client/ui-feedback/tests/feedback-actions.spec.tsx +++ b/packages/client/ui-feedback/tests/feedback-actions.spec.tsx @@ -12,7 +12,9 @@ import { cleanup, fireEvent, render, waitFor } from '@testing-library/react' import { makeTranslate } from '@deepseek-ai/dsh-client-test-runtime' import { zh as commonZh } from '@deepseek-ai/dsh-client-locale/src/locales/zh.ts' import type { MessageId } from '@deepseek-ai/dsh-client-connection/client' -import type { MessageFeedbackItem, MessageFeedbackVersion } from '@deepseek-ai/dsh-message-feedback/types' +import type { + MessageFeedbackItem, MessageFeedbackRating, MessageFeedbackVersion, +} from '@deepseek-ai/dsh-message-feedback/types' import { FeedbackActions } from '../src/client/FeedbackActions.tsx' import type { FeedbackActionResult, FeedbackView } from '../src/client/controller.ts' import { zh } from '../src/client/locales.ts' @@ -38,20 +40,29 @@ function mount(options: { current?: MessageFeedbackItem | undefined rateResult?: FeedbackActionResult clearResult?: FeedbackActionResult + status?: FeedbackView['status'] } = {}) { const view: FeedbackView = { - status: 'ready', + status: options.status ?? 'ready', items: new Map(options.current === undefined ? [] : [[MSG, options.current]]), error: null, } const ensure = vi.fn(() => Promise.resolve({ ok: true })) - const rate = vi.fn(() => Promise.resolve(options.rateResult ?? { ok: true as const })) - const clear = vi.fn(() => Promise.resolve(options.clearResult ?? { ok: true as const })) + const rate = vi.fn((_id: MessageId, _rating: MessageFeedbackRating, _note?: string) => + Promise.resolve(options.rateResult ?? { ok: true as const })) + const clear = vi.fn((_id: MessageId) => + Promise.resolve(options.clearResult ?? { ok: true as const })) + // The controller owns retract-vs-replace, so the double stands in for it: + // matching the shown rating retracts, anything else replaces. + const toggle = vi.fn((id: MessageId, next: MessageFeedbackRating) => + (options.current?.rating === next ? clear(id) : rate(id, next))) + const clearNote = vi.fn((_id: MessageId) => + Promise.resolve(options.rateResult ?? { ok: true as const })) const useFeedback = ((select: (v: FeedbackView) => T): T => useSyncExternalStore(() => () => {}, () => select(view))) as never - const props = { messageId: MSG, ensure, rate, clear, useFeedback, t } as unknown as + const props = { messageId: MSG, ensure, rate, toggle, clearNote, clear, useFeedback, t } as unknown as Parameters[0] - return { ...render(), ensure, rate, clear } + return { ...render(), ensure, rate, clear, toggle, clearNote } } describe('FeedbackActions', () => { @@ -91,7 +102,7 @@ describe('FeedbackActions', () => { fireEvent.click(ui.getByLabelText(zh['action.like'])) - await waitFor(() => { expect(ui.rate).toHaveBeenCalledWith(MSG, 'positive', undefined) }) + await waitFor(() => { expect(ui.toggle).toHaveBeenCalledWith(MSG, 'positive') }) expect(ui.clear).not.toHaveBeenCalled() }) @@ -100,7 +111,7 @@ describe('FeedbackActions', () => { fireEvent.click(ui.getByLabelText(zh['action.dislike'])) - await waitFor(() => { expect(ui.rate).toHaveBeenCalledWith(MSG, 'negative', 'keep me') }) + await waitFor(() => { expect(ui.toggle).toHaveBeenCalledWith(MSG, 'negative') }) }) it('retracts the feedback when the active rating is clicked again', async () => { @@ -108,8 +119,9 @@ describe('FeedbackActions', () => { fireEvent.click(ui.getByLabelText(zh['action.likeActive'])) + await waitFor(() => { expect(ui.toggle).toHaveBeenCalledWith(MSG, 'positive') }) + // The double routes a matching rating to clear(), mirroring the controller. await waitFor(() => { expect(ui.clear).toHaveBeenCalledWith(MSG) }) - expect(ui.rate).not.toHaveBeenCalled() }) it('saves a typed note through the rate verb and closes the editor', async () => { @@ -130,7 +142,7 @@ describe('FeedbackActions', () => { fireEvent.change(ui.getByLabelText(zh['note.aria']), { target: { value: ' ' } }) fireEvent.click(ui.getByText(zh['note.save'])) - await waitFor(() => { expect(ui.rate).toHaveBeenCalledWith(MSG, 'positive', undefined) }) + await waitFor(() => { expect(ui.clearNote).toHaveBeenCalledWith(MSG) }) }) it('seeds the editor with the recorded note and abandons it on cancel', () => { @@ -197,6 +209,8 @@ describe('FeedbackActions', () => { messageId: MSG, ensure: vi.fn(() => Promise.resolve({ ok: true })), rate: vi.fn(() => gate), + toggle: vi.fn(() => gate), + clearNote: vi.fn(() => Promise.resolve({ ok: true })), clear: vi.fn(() => Promise.resolve({ ok: true })), useFeedback, t, @@ -214,4 +228,22 @@ describe('FeedbackActions', () => { window.removeEventListener('error', onError) expect(errors).toEqual([]) }) + + it('surfaces a failed list load next to the controls', async () => { + const ui = mount({ status: 'error' }) + + expect(ui.getByText(zh['error.load'])).toBeTruthy() + }) + + it('prefers the action failure over the load notice', async () => { + const ui = mount({ + status: 'error', + rateResult: { ok: false, error: { code: 'target-not-found', message: 'gone' } }, + }) + + fireEvent.click(ui.getByLabelText(zh['action.like'])) + + await waitFor(() => { expect(ui.getByText(zh['error.generic'])).toBeTruthy() }) + expect(ui.queryByText(zh['error.load'])).toBeNull() + }) }) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index bc51f12800..fecbc7f7c2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2204,6 +2204,9 @@ importers: '@deepseek-ai/dsh-api-remotes': specifier: workspace:^ version: link:../../api/remotes + '@deepseek-ai/dsh-client-connection': + specifier: workspace:^ + version: link:../connection '@deepseek-ai/dsh-client-locale': specifier: workspace:^ version: link:../locale