fix(feedback): address review findings in the controller and controls

Resolve the note-erasure race the review found: a control that rendered before the first list read held no item and passed note=undefined, so switching a rating silently dropped the stored note. The controller now owns note resolution and toggle-vs-retract, deciding from the committed item inside the serialized mutation, and clearNote expresses deletion. Also from review: - serialize the reconnect re-read behind queued mutations (resync), so a list reply cannot resurrect a version a newer mutation replaced - re-check disposal after the seeding read, so an unloaded fiber never reaches the wire - drop Object.freeze on Maps, which does not prevent set/delete - declare the @deepseek-ai/dsh-client-connection dependency it imports - surface a failed list load in the controls - drop the /client value exports that had no consumer - state the per-turn render scope in the README and subsystem pages - align the package version with the root
2026-08-12 01:12:41 +08:00
parent 63e6ea1b79
commit 47f254a252
19 changed files with 373 additions and 74 deletions
--- 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
          </button>
        </span>
      )}
+      {failure === null && loadFailed && (
+        <span className={css.failure} role="status">{t('error.load')}</span>
+      )}
      {failure !== null && <span className={css.failure} role="status">{failure}</span>}
    </>
  )
--- 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<MessageId, MessageFeedbackItem> = 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<MessageId, MessageFeedbackItem> = 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<FeedbackView> {
  /**
   * 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<FeedbackActionResult> {
@@ -129,12 +143,29 @@ export class FeedbackController implements HostObservable<FeedbackView> {
    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<FeedbackActionResult> {
+    // 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<FeedbackView> {
  ): Promise<FeedbackActionResult> {
    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<FeedbackActionResult> {
+    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<FeedbackActionResult> {
+    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<FeedbackView> {
    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<FeedbackActionResult> {
+    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<FeedbackActionResult> {
+    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<FeedbackView> {
      }
      const items = new Map<MessageId, MessageFeedbackItem>()
      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<FeedbackView> {
   * 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<FeedbackActionResult>): Promise<FeedbackActionResult> {
+  private mutate(
+    operation: () => Promise<FeedbackActionResult>,
+    options: { readonly seed?: boolean } = {},
+  ): Promise<FeedbackActionResult> {
    const guarded = async (): Promise<FeedbackActionResult> => {
-      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<FeedbackView> {
    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. */
--- 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),
        }
      },
--- 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<string, string>

@@ -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<FeedbackKey, string>
--- 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<FeedbackActionResult>
+  /**
+   * 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<FeedbackActionResult>
+  /**
+   * Drop the note while keeping the rating.
+   * @param messageId - target assistant message.
+   */
+  clearNote: (messageId: MessageId) => Promise<FeedbackActionResult>
  /**
   * Remove this Session's feedback for one message.
   * @param messageId - target assistant message.