diff --git a/backend/packages/harness/deerflow/domain/feedback/service.py b/backend/packages/harness/deerflow/domain/feedback/service.py index 85d33a00d..cc8cd638d 100644 --- a/backend/packages/harness/deerflow/domain/feedback/service.py +++ b/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 - validates the rating) and stores it with upsert-by-identity + Builds the aggregate first (which validates rating and tags), then + 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: diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 286f03d9c..dac546be7 100644 --- a/backend/tests/conftest.py +++ b/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__ diff --git a/backend/tests/feedback_fakes.py b/backend/tests/feedback_fakes.py new file mode 100644 index 000000000..632e9e7cb --- /dev/null +++ b/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) diff --git a/backend/tests/test_feedback.py b/backend/tests/test_feedback.py index 2c952228a..21f1a70d2 100644 --- a/backend/tests/test_feedback.py +++ b/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.""" diff --git a/backend/tests/test_feedback_service.py b/backend/tests/test_feedback_service.py index 3aab6ec63..05e5554b6 100644 --- a/backend/tests/test_feedback_service.py +++ b/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: diff --git a/frontend/src/components/workspace/messages/feedback-dialog.tsx b/frontend/src/components/workspace/messages/feedback-dialog.tsx index a5605fbe0..aae0bda07 100644 --- a/frontend/src/components/workspace/messages/feedback-dialog.tsx +++ b/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); - setSelected(new Set()); - setComment(""); + handleOpenChange(false); + } catch { + // 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 ( - + {t.feedback.dialogTitle} diff --git a/frontend/src/core/i18n/locales/en-US.ts b/frontend/src/core/i18n/locales/en-US.ts index ddbc8efa5..7bdb03e89 100644 --- a/frontend/src/core/i18n/locales/en-US.ts +++ b/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", diff --git a/frontend/src/core/i18n/locales/types.ts b/frontend/src/core/i18n/locales/types.ts index 347045b0e..68c0659fb 100644 --- a/frontend/src/core/i18n/locales/types.ts +++ b/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; diff --git a/frontend/src/core/i18n/locales/zh-CN.ts b/frontend/src/core/i18n/locales/zh-CN.ts index c9c24f54d..b6d0f5f81 100644 --- a/frontend/src/core/i18n/locales/zh-CN.ts +++ b/frontend/src/core/i18n/locales/zh-CN.ts @@ -24,6 +24,7 @@ export const zhCN: Translations = { dialogTitle: "分享反馈", detailsPlaceholder: "分享详细信息（可选）", submit: "提交", + submitFailed: "反馈保存失败，请重试。", tags: { incorrect: "不正确或不完整", notAsExpected: "与期望不符",