fix(feedback): address review on dialog state, errors, and test seams

Four findings from @willem-bd on #4401:

- FeedbackDialog stays mounted across messages, so selected tags and the
  comment survived an ESC/click-outside dismiss and pre-filled the next
  thumbs-down. Reset on every close path via a wrapped onOpenChange.
- Neither the dialog's handleSubmit nor handleDialogSubmit caught a failed
  enrichment PUT, so a rejection went unhandled and the user got no signal.
  Catch in the dialog (where the rejection lands), toast, and keep the input
  for a retry.
- rate_run awaited the RunLookup port before Feedback.create validated the
  rating, contradicting its own "before any I/O happens" docstring: an
  invalid rating on an unknown run surfaced as RunNotFoundError. Validate
  first, restoring the legacy router's 400-before-404 ordering. The service
  test now uses an unknown run id so it actually pins that order.
- InMemoryFeedbackRepository moved out of test_feedback.py into
  tests/feedback_fakes.py (with FakeRunLookup) so two test modules share it
  without one importing the other; conftest states the tests-dir sys.path
  dependency explicitly, which also makes it work under
  --import-mode=importlib.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

backend/packages/harness/deerflow/domain/feedback/service.py

@@ -34,17 +34,19 @@ class FeedbackService:
         """Set the user's current rating for a run (idempotent).
 
         Backs the PUT endpoint: "my current verdict on this run is X".
-        Verifies run ownership first, then creates the aggregate (which
+        Builds the aggregate first (which validates rating and tags), then
-        validates the rating) and stores it with upsert-by-identity
+        verifies run ownership, then stores it with upsert-by-identity
         semantics -- repeated calls replace the previous rating.
 
         Raises:
+            InvalidRatingError: rating is not +1 or -1. Raised by the
+                aggregate factory before any port call, so a malformed
+                rating is reported as such even for an unknown run.
+            InvalidTagError: a tag is not a known reason slug (same
+                pre-I/O guarantee).
             RunNotFoundError: the run does not exist or does not belong
                 to the given thread (cross-thread ids are rejected).
-            InvalidRatingError: rating is not +1 or -1 (raised by the
-                aggregate factory before any I/O happens).
         """
-        await self._require_run(thread_id, run_id)
         feedback = Feedback.create(
             run_id=run_id,
             thread_id=thread_id,
@@ -53,6 +55,7 @@ class FeedbackService:
             comment=comment,
             tags=tags,
         )
+        await self._require_run(thread_id, run_id)
         return await self._repository.save(feedback)
 
     async def retract_run_rating(self, thread_id: str, run_id: str, *, user_id: str | None) -> bool:

backend/tests/conftest.py

@@ -17,6 +17,10 @@ import pytest
 # Make 'app' and 'deerflow' importable from any working directory
 sys.path.insert(0, str(Path(__file__).parent.parent))
 sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "scripts"))
+# Shared test helpers (e.g. feedback_fakes) import as plain modules. pytest's
+# default prepend import mode already puts this directory on the path, but not
+# under --import-mode=importlib, so state the dependency explicitly.
+sys.path.insert(0, str(Path(__file__).parent))
 
 # Break the circular import chain that exists in production code:
 #   deerflow.subagents.__init__

backend/tests/feedback_fakes.py

@@ -0,0 +1,48 @@
+"""Shared zero-IO doubles for the feedback context.
+
+Lives in its own module rather than inside a test file so both the port
+contract suite (``test_feedback.py``) and the service tests
+(``test_feedback_service.py``) can import it as an ordinary module, without
+relying on the tests directory happening to be on ``sys.path``.
+"""
+
+from dataclasses import replace
+
+from deerflow.domain.feedback import Feedback
+
+
+class InMemoryFeedbackRepository:
+    """Zero-IO fake keyed by aggregate identity (thread, run, user)."""
+
+    def __init__(self) -> None:
+        self._rows: dict[tuple[str, str, str | None], Feedback] = {}
+
+    async def save(self, feedback: Feedback) -> Feedback:
+        key = (feedback.thread_id, feedback.run_id, feedback.user_id)
+        existing = self._rows.get(key)
+        if existing is not None:
+            feedback = replace(feedback, feedback_id=existing.feedback_id)
+        self._rows[key] = feedback
+        return feedback
+
+    async def latest_per_run_in_thread(self, thread_id: str, *, user_id: str | None) -> dict[str, Feedback]:
+        return {fb.run_id: fb for (thread, _run, user), fb in self._rows.items() if thread == thread_id and (user_id is None or user == user_id)}
+
+    async def latest_for_runs(self, thread_id: str, run_ids: set[str], *, user_id: str | None) -> dict[str, Feedback]:
+        if not run_ids:
+            return {}
+        per_run = await self.latest_per_run_in_thread(thread_id, user_id=user_id)
+        return {run_id: fb for run_id, fb in per_run.items() if run_id in run_ids}
+
+    async def remove_for_run(self, thread_id: str, run_id: str, *, user_id: str | None) -> bool:
+        return self._rows.pop((thread_id, run_id, user_id), None) is not None
+
+
+class FakeRunLookup:
+    """RunLookup double backed by a run_id -> thread_id mapping."""
+
+    def __init__(self, runs: dict[str, str]):
+        self._runs = runs
+
+    async def thread_of(self, run_id: str) -> str | None:
+        return self._runs.get(run_id)

backend/tests/test_feedback.py

@@ -2,44 +2,17 @@
 
 Every implementation (SQL adapter, in-memory fake) runs the same suite, so
 port semantics and implementations cannot drift apart. The in-memory fake
-also serves service-level tests as a fast zero-IO double.
+lives in ``feedback_fakes`` because service-level tests reuse it as a fast
+zero-IO double.
 """
 
-from dataclasses import replace
-
 import pytest
+from feedback_fakes import InMemoryFeedbackRepository
 
 from deerflow.domain.feedback import Feedback
 from deerflow.domain.feedback.ports import FeedbackRepository
 
 
-class InMemoryFeedbackRepository:
-    """Zero-IO fake keyed by aggregate identity (thread, run, user)."""
-
-    def __init__(self) -> None:
-        self._rows: dict[tuple[str, str, str | None], Feedback] = {}
-
-    async def save(self, feedback: Feedback) -> Feedback:
-        key = (feedback.thread_id, feedback.run_id, feedback.user_id)
-        existing = self._rows.get(key)
-        if existing is not None:
-            feedback = replace(feedback, feedback_id=existing.feedback_id)
-        self._rows[key] = feedback
-        return feedback
-
-    async def latest_per_run_in_thread(self, thread_id: str, *, user_id: str | None) -> dict[str, Feedback]:
-        return {fb.run_id: fb for (thread, _run, user), fb in self._rows.items() if thread == thread_id and (user_id is None or user == user_id)}
-
-    async def latest_for_runs(self, thread_id: str, run_ids: set[str], *, user_id: str | None) -> dict[str, Feedback]:
-        if not run_ids:
-            return {}
-        per_run = await self.latest_per_run_in_thread(thread_id, user_id=user_id)
-        return {run_id: fb for run_id, fb in per_run.items() if run_id in run_ids}
-
-    async def remove_for_run(self, thread_id: str, run_id: str, *, user_id: str | None) -> bool:
-        return self._rows.pop((thread_id, run_id, user_id), None) is not None
-
-
 class FeedbackRepositoryContract:
     """Shared port-semantics suite; subclasses provide a ``repo`` fixture."""
 

backend/tests/test_feedback_service.py

@@ -5,21 +5,11 @@ is exercised end-to-end against dict-backed doubles.
 """
 
 import pytest
-from test_feedback import InMemoryFeedbackRepository
+from feedback_fakes import FakeRunLookup, InMemoryFeedbackRepository
 
 from deerflow.domain.feedback import FeedbackService, InvalidRatingError, RunNotFoundError
 
 
-class FakeRunLookup:
-    """RunLookup double backed by a run_id -> thread_id mapping."""
-
-    def __init__(self, runs: dict[str, str]):
-        self._runs = runs
-
-    async def thread_of(self, run_id: str) -> str | None:
-        return self._runs.get(run_id)
-
-
 def _service(runs: dict[str, str] | None = None) -> FeedbackService:
     return FeedbackService(
         repository=InMemoryFeedbackRepository(),
@@ -65,10 +55,13 @@ class TestRateRun:
             await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1")
 
     @pytest.mark.anyio
-    async def test_invalid_rating_propagates_before_persistence(self):
+    async def test_invalid_rating_rejected_before_run_lookup(self):
+        # "unknown-run" is absent from the lookup, so a rating validated after
+        # the RunLookup port call would surface RunNotFoundError instead.
+        # Expecting InvalidRatingError is what pins validation ahead of I/O.
         svc = _service()
         with pytest.raises(InvalidRatingError):
-            await svc.rate_run("t1", "r1", rating=0, comment=None, user_id="u1")
+            await svc.rate_run("t1", "unknown-run", rating=0, comment=None, user_id="u1")
 
 
 class TestRetractAndReads:

frontend/src/components/workspace/messages/feedback-dialog.tsx

@@ -1,6 +1,7 @@
 "use client";
 
 import { useState } from "react";
+import { toast } from "sonner";
 
 import { FEEDBACK_TAG_SLUGS, type FeedbackTagSlug } from "@/core/api/feedback";
 import { useI18n } from "@/core/i18n/hooks";
@@ -64,21 +65,34 @@ export function FeedbackDialog({
   const canSubmit =
     (selected.size > 0 || comment.trim().length > 0) && !isSubmitting;
 
+  // This dialog stays mounted across messages, so state has to be dropped on
+  // every close path — submit, ESC, and click-outside all land here. Otherwise
+  // the next thumbs-down opens pre-filled with the previous message's answers.
+  const handleOpenChange = (next: boolean) => {
+    if (!next) {
+      setSelected(new Set());
+      setComment("");
+    }
+    onOpenChange(next);
+  };
+
   const handleSubmit = async () => {
     if (!canSubmit) return;
     setIsSubmitting(true);
     try {
       await onSubmit([...selected], comment.trim());
-      onOpenChange(false);
+      handleOpenChange(false);
-      setSelected(new Set());
+    } catch {
-      setComment("");
+      // The rating itself is already stored; only this enrichment failed. Keep
+      // the dialog open with the user's input so they can retry.
+      toast.error(t.feedback.submitFailed);
     } finally {
       setIsSubmitting(false);
     }
   };
 
   return (
-    <Dialog open={open} onOpenChange={onOpenChange}>
+    <Dialog open={open} onOpenChange={handleOpenChange}>
       <DialogContent className="sm:max-w-lg">
         <DialogHeader>
           <DialogTitle>{t.feedback.dialogTitle}</DialogTitle>

frontend/src/core/i18n/locales/en-US.ts

@@ -24,6 +24,7 @@ export const enUS: Translations = {
     dialogTitle: "Share feedback",
     detailsPlaceholder: "Share more details (optional)",
     submit: "Submit",
+    submitFailed: "Could not save your feedback. Please try again.",
     tags: {
       incorrect: "Incorrect or incomplete",
       notAsExpected: "Not what I expected",

frontend/src/core/i18n/locales/types.ts

@@ -13,6 +13,7 @@ export interface Translations {
     dialogTitle: string;
     detailsPlaceholder: string;
     submit: string;
+    submitFailed: string;
     tags: {
       incorrect: string;
       notAsExpected: string;

frontend/src/core/i18n/locales/zh-CN.ts

@@ -24,6 +24,7 @@ export const zhCN: Translations = {
     dialogTitle: "分享反馈",
     detailsPlaceholder: "分享详细信息（可选）",
     submit: "提交",
+    submitFailed: "反馈保存失败，请重试。",
     tags: {
       incorrect: "不正确或不完整",
       notAsExpected: "与期望不符",