test(feedback): reach the per-file coverage gate

Cover the reachable controller and control branches: each failure code's copy, dispose-during-flight, non-Error rejections, and non-conflict mutation failures. Two paths were unreachable rather than untested, so remove them instead: commit() cannot run after disposal because mutate() refuses admission first, and the mutation tail cannot reject because every queued operation settles as a result. Pass the recorded rating into the note save so the editor's render site proves it exists.
2026-08-11 17:52:09 +08:00
parent 9950c64621
commit 5867c00faf
4 changed files with 271 additions and 11 deletions
@@ -60,16 +60,17 @@ export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t
    void rate(messageId, next, item?.note).then(settle)
  }, [clear, item?.note, messageId, rate, rating, settle])

-  const onSaveNote = useCallback(() => {
-    if (rating === undefined) return
+  // 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.
+  const onSaveNote = useCallback((current: MessageFeedbackRating) => {
    const trimmed = draft.trim()
    setPending(true)
    setFailure(null)
-    void rate(messageId, rating, trimmed.length === 0 ? undefined : trimmed).then((result) => {
+    void rate(messageId, current, trimmed.length === 0 ? undefined : trimmed).then((result) => {
      settle(result)
      if (result.ok && alive.current) setNoteOpen(false)
    })
-  }, [draft, messageId, rate, rating, settle])
+  }, [draft, messageId, rate, settle])

  const openNote = useCallback(() => {
    setDraft(item?.note ?? '')
@@ -116,7 +117,7 @@ export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t
          {item?.note === undefined ? t('note.open') : item.note}
        </button>
      )}
-      {noteOpen && (
+      {rating !== undefined && noteOpen && (
        <span className={css.noteEditor}>
          <textarea
            className={css.noteInput}
@@ -126,7 +127,12 @@ export function FeedbackActions({ messageId, ensure, rate, clear, useFeedback, t
            rows={2}
            onChange={(event) => { setDraft(event.target.value) }}
          />
-          <button type="button" className={css.noteSave} disabled={pending} onClick={onSaveNote}>
+          <button
+            type="button"
+            className={css.noteSave}
+            disabled={pending}
+            onClick={() => { onSaveNote(rating) }}
+          >
            {t('note.save')}
          </button>
          <button type="button" className={css.noteCancel} onClick={() => { setNoteOpen(false) }}>
@@ -238,15 +238,20 @@ export class FeedbackController implements HostObservable<FeedbackView> {
      }
    }
    const result = this.operationTail.then(guarded, guarded)
-    // Every queued operation settles carrier and business failures as a
-    // FeedbackActionResult, so this controlled tail cannot reject.
-    this.operationTail = result.then(() => undefined, () => undefined)
+    // `guarded` settles every carrier and business failure as a
+    // FeedbackActionResult and never rethrows, so this tail cannot reject and
+    // needs no rejection handler.
+    this.operationTail = result.then(() => undefined)
    return result
  }

-  /** Replace one message's entry, keeping every other entry's identity. */
+  /**
+   * Replace one message's entry, keeping every other entry's identity. Only a
+   * `mutate` operation reaches this, and `mutate` refuses admission once the
+   * controller is disposed, so no disposal guard belongs here; `publish` is
+   * the single place that stops notifying after listeners are dropped.
+   */
  private commit(messageId: MessageId, item: MessageFeedbackItem | null): void {
-    if (this.disposed) return
    const items = new Map(this.view.items)
    if (item === null) items.delete(messageId)
    else items.set(messageId, item)
@@ -270,4 +270,208 @@ describe('FeedbackController', () => {
    expect(calls).toHaveLength(before)
    expect(listener).not.toHaveBeenCalled()
  })
+
+  it('renders a human explanation for every business failure code', async () => {
+    const codes = [
+      ['session-not-found', 'this session is no longer persisted'],
+      ['target-not-found', 'this message is not a persisted assistant message'],
+      ['note-blank', 'a note must contain a non-whitespace character'],
+      ['note-too-large', 'the note is too long'],
+    ] as const
+    for (const [code, message] of codes) {
+      const { remote } = fakeRemote({
+        list: () => Promise.resolve({ ok: false, error: { code, sessionId: SESSION } } as never),
+      })
+      const controller = new FeedbackController(remote, SESSION)
+      expect(await controller.ensure()).toMatchObject({ ok: false, error: { code } })
+      expect(controller.getSnapshot().error).toBe(message)
+    }
+  })
+
+  it('falls back to the raw code for an unrecognized failure', async () => {
+    const { remote } = fakeRemote({
+      list: () => Promise.resolve({ ok: false, error: { code: 'brand-new-code' } } as never),
+    })
+    const controller = new FeedbackController(remote, SESSION)
+
+    expect(await controller.ensure()).toMatchObject({ ok: false, error: { code: 'brand-new-code' } })
+    expect(controller.getSnapshot().error).toBe('brand-new-code')
+  })
+
+  it('publishes nothing when the list settles after disposal', async () => {
+    let release = (): void => {}
+    const gate = new Promise<void>((resolve) => { release = resolve })
+    const { remote } = fakeRemote({
+      list: async () => {
+        await gate
+        return { ok: true, value: { items: [item()] } }
+      },
+    })
+    const controller = new FeedbackController(remote, SESSION)
+    const pending = controller.ensure()
+    const listener = vi.fn()
+    controller.subscribe(listener)
+
+    controller.dispose()
+    release()
+
+    expect(await pending).toEqual({ ok: true })
+    expect(controller.getSnapshot().items.has(MSG)).toBe(false)
+    expect(listener).not.toHaveBeenCalled()
+  })
+
+  it('swallows a rejected list that settles after disposal', async () => {
+    let reject = (): void => {}
+    const gate = new Promise<void>((_resolve, rejectFn) => { reject = () => { rejectFn(new Error('late')) } })
+    const { remote } = fakeRemote({ list: () => gate as Promise<never> })
+    const controller = new FeedbackController(remote, SESSION)
+    const pending = controller.ensure()
+
+    controller.dispose()
+    reject()
+
+    expect(await pending).toEqual({ ok: true })
+    expect(controller.getSnapshot().status).not.toBe('error')
+  })
+
+  it('describes a non-Error list rejection with a stable message', async () => {
+    // oxlint-disable-next-line typescript/prefer-promise-reject-errors -- the non-Error rejection is the scenario under test.
+    const { remote } = fakeRemote({ list: () => Promise.reject('socket string') })
+    const controller = new FeedbackController(remote, SESSION)
+
+    expect(await controller.ensure()).toEqual({
+      ok: false,
+      error: { code: 'transport', message: 'message feedback list failed' },
+    })
+  })
+
+  it('describes a non-Error mutation rejection with a stable message', async () => {
+    // oxlint-disable-next-line typescript/prefer-promise-reject-errors -- the non-Error rejection is the scenario under test.
+    const { remote } = fakeRemote({ put: () => Promise.reject('nope') })
+    const controller = new FeedbackController(remote, SESSION)
+
+    expect(await controller.rate(MSG, 'positive')).toEqual({
+      ok: false,
+      error: { code: 'transport', message: 'message feedback mutation failed' },
+    })
+  })
+
+  it('propagates a failed load to a queued mutation without calling the wire', async () => {
+    const { remote, calls } = fakeRemote({
+      list: () => Promise.resolve({ ok: false, error: { code: 'session-not-found', sessionId: SESSION } }),
+    })
+    const controller = new FeedbackController(remote, SESSION)
+
+    expect(await controller.rate(MSG, 'positive')).toMatchObject({
+      ok: false,
+      error: { code: 'session-not-found' },
+    })
+    expect(calls.filter(call => call.method === 'put')).toHaveLength(0)
+  })
+
+  it('keeps a later mutation running after an earlier one settles as a failure', async () => {
+    let first = true
+    const { remote } = fakeRemote({
+      put: () => {
+        if (first) {
+          first = false
+          return Promise.reject(new Error('first blew up'))
+        }
+        return Promise.resolve({ ok: true, value: item({ rating: 'negative' }) })
+      },
+    })
+    const controller = new FeedbackController(remote, SESSION)
+
+    const [a, b] = await Promise.all([
+      controller.rate(MSG, 'positive'),
+      controller.rate(MSG, 'negative'),
+    ])
+
+    expect(a).toMatchObject({ ok: false, error: { code: 'transport' } })
+    expect(b).toEqual({ ok: true })
+    expect(controller.getSnapshot().items.get(MSG)?.rating).toBe('negative')
+  })
+
+  it('ignores a conflict reconciliation that lands after disposal', async () => {
+    // The mutate() guard only refuses work admitted after disposal, so this
+    // exercises commit()'s own guard: the call is already in flight when the
+    // fiber unloads, and its authoritative item must not be published.
+    let release = (): void => {}
+    const gate = new Promise<void>((resolve) => { release = resolve })
+    const { remote } = fakeRemote({
+      list: () => Promise.resolve({ ok: true, value: { items: [item({ version: version('v1') })] } }),
+      put: async () => {
+        await gate
+        return { ok: false, error: { code: 'version-conflict', current: item({ version: version('v2'), rating: 'negative' }) } }
+      },
+    })
+    const controller = new FeedbackController(remote, SESSION)
+    await controller.ensure()
+    const listener = vi.fn()
+    controller.subscribe(listener)
+    const pending = controller.rate(MSG, 'negative')
+
+    controller.dispose()
+    release()
+    await pending
+
+    // publish() drops its listener set on dispose, so no subscriber is told.
+    expect(listener).not.toHaveBeenCalled()
+  })
+
+  it('drops a delete conflict reconciliation once disposed mid-flight', async () => {
+    let release = (): void => {}
+    const gate = new Promise<void>((resolve) => { release = resolve })
+    const { remote } = fakeRemote({
+      list: () => Promise.resolve({ ok: true, value: { items: [item()] } }),
+      delete: async () => {
+        await gate
+        return { ok: false, error: { code: 'version-conflict', current: null } }
+      },
+    })
+    const controller = new FeedbackController(remote, SESSION)
+    await controller.ensure()
+    const pending = controller.clear(MSG)
+
+    const listener = vi.fn()
+    controller.subscribe(listener)
+    controller.dispose()
+    release()
+    await pending
+
+    // The reconciliation still computes, but no subscriber is notified.
+    expect(listener).not.toHaveBeenCalled()
+  })
+
+  it('leaves the local item untouched when a rating fails for a non-conflict reason', async () => {
+    const existing = item({ version: version('v3'), rating: 'positive' })
+    const { remote } = fakeRemote({
+      list: () => Promise.resolve({ ok: true, value: { items: [existing] } }),
+      put: () => Promise.resolve({ ok: false, error: { code: 'note-too-large', maxBytes: 8, actualBytes: 9 } }),
+    })
+    const controller = new FeedbackController(remote, SESSION)
+    await controller.ensure()
+
+    expect(await controller.rate(MSG, 'negative', 'far too long')).toMatchObject({
+      ok: false,
+      error: { code: 'note-too-large' },
+    })
+    expect(controller.getSnapshot().items.get(MSG)).toEqual(existing)
+  })
+
+  it('leaves the local item untouched when a delete fails for a non-conflict reason', async () => {
+    const existing = item({ version: version('v4') })
+    const { remote } = fakeRemote({
+      list: () => Promise.resolve({ ok: true, value: { items: [existing] } }),
+      delete: () => Promise.resolve({ ok: false, error: { code: 'session-not-found', sessionId: SESSION } }),
+    })
+    const controller = new FeedbackController(remote, SESSION)
+    await controller.ensure()
+
+    expect(await controller.clear(MSG)).toMatchObject({
+      ok: false,
+      error: { code: 'session-not-found' },
+    })
+    expect(controller.getSnapshot().items.get(MSG)).toEqual(existing)
+  })
 })
@@ -169,4 +169,49 @@ describe('FeedbackActions', () => {

    await waitFor(() => { expect(ui.getByText(zh['error.generic'])).toBeTruthy() })
  })
+
+  it('keeps the editor open when the note fails to save', async () => {
+    const ui = mount({
+      current: item({ rating: 'positive' }),
+      rateResult: { ok: false, error: { code: 'note-too-large', message: 'too long' } },
+    })
+
+    fireEvent.click(ui.getByText(zh['note.open']))
+    fireEvent.change(ui.getByLabelText(zh['note.aria']), { target: { value: 'x'.repeat(20) } })
+    fireEvent.click(ui.getByText(zh['note.save']))
+
+    await waitFor(() => { expect(ui.getByText(zh['error.generic'])).toBeTruthy() })
+    // The draft survives so the human can shorten it instead of retyping.
+    expect(ui.getByLabelText(zh['note.aria'])).toBeTruthy()
+  })
+
+  it('publishes no state after the row unmounts mid-flight', async () => {
+    let release = (): void => {}
+    const gate = new Promise<FeedbackActionResult>((resolve) => {
+      release = () => { resolve({ ok: false, error: { code: 'target-not-found', message: 'gone' } }) }
+    })
+    const view: FeedbackView = { status: 'ready', items: new Map(), error: null }
+    const useFeedback = (<T,>(select: (v: FeedbackView) => T): T =>
+      useSyncExternalStore(() => () => {}, () => select(view))) as never
+    const props = {
+      messageId: MSG,
+      ensure: vi.fn(() => Promise.resolve<FeedbackActionResult>({ ok: true })),
+      rate: vi.fn(() => gate),
+      clear: vi.fn(() => Promise.resolve<FeedbackActionResult>({ ok: true })),
+      useFeedback,
+      t,
+    } as unknown as Parameters<typeof FeedbackActions>[0]
+    const ui = render(<FeedbackActions {...props} />)
+    const errors: unknown[] = []
+    const onError = (event: ErrorEvent): void => { errors.push(event.error) }
+    window.addEventListener('error', onError)
+
+    fireEvent.click(ui.getByLabelText(zh['action.like']))
+    ui.unmount()
+    release()
+    await gate
+
+    window.removeEventListener('error', onError)
+    expect(errors).toEqual([])
+  })
 })