feat: add persisted feedback-provided markers for message feedback me…#1059
feat: add persisted feedback-provided markers for message feedback me…#1059
Conversation
🦋 Changeset detectedLatest commit: a25ea6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughAdds persisted feedback metadata (provided, providedAt, feedbackId), new feedback helpers and AgentFeedbackHandle, agent.markFeedbackProvided to persist provided state to memory, server schema updates, tests, and docs demonstrating client/server flows for marking feedback provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Agent
participant Memory
participant FeedbackURL as ExternalFeedbackEndpoint
User->>Client: click "submit feedback"
Client->>FeedbackURL: POST feedback payload
FeedbackURL-->>Client: 200 OK
Client->>Agent: call result.feedback.markFeedbackProvided(feedbackId?)
Agent->>Memory: load conversation & message by ids
Agent->>Memory: update message.metadata.feedback { provided, providedAt, feedbackId }
Memory-->>Agent: confirm persisted metadata
Agent-->>Client: resolve updated AgentFeedbackMetadata / handle updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying voltagent with
|
| Latest commit: |
a25ea6a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bd162f7e.voltagent.pages.dev |
| Branch Preview URL: | https://feat-feedback-status-persist.voltagent.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/core/src/agent/feedback.ts`:
- Around line 110-121: The code always overwrites providedAt by calling
resolveFeedbackProvidedAt(input.providedAt); instead, preserve the existing
timestamp unless the caller explicitly supplies one: compute providedAt by
checking if input.providedAt is defined (use
resolveFeedbackProvidedAt(input.providedAt)), otherwise reuse
existingFeedback.providedAt (fall back to resolveFeedbackProvidedAt(undefined)
only if there is no existing value). Update the assignment that builds
updatedFeedback (and any uses in markFeedbackProvided) to use this conditional
logic so repeated calls don't clobber the original providedAt.
In `@website/docs/agents/overview.md`:
- Around line 155-163: The example uses an undeclared variable `feedbackId` in
the `result.feedback.markFeedbackProvided({ feedbackId })` call; update the
snippet to either declare `feedbackId` or add an inline comment explaining its
origin (for example: `// feedbackId returned from feedback ingestion response`)
so readers know where to get it; modify the
`result.feedback.markFeedbackProvided` example to include that comment or a
brief note that `feedbackId` comes from the feedback ingestion API response.
In `@website/observability/feedback.md`:
- Around line 106-112: The code examples reference undeclared variables
(feedbackId, agentId, userId, conversationId) which will cause ReferenceErrors;
update the examples around result.feedback.markFeedbackProvided and
submitFeedback to show where these values come from (e.g., add a short preamble
or inline comment indicating they are provided by your app/context) and ensure
the sample shows placeholder declarations for feedbackId, agentId, userId, and
conversationId so readers can copy the snippets without errors.
🧹 Nitpick comments (2)
website/observability/feedback.md (1)
268-276:JSON.stringifyused in client-side documentation example.The coding guidelines state to use
safeStringifyfrom@voltagent/internalinstead ofJSON.stringify. However, this is a client-side browserfetchexample in a Markdown doc where@voltagent/internalmay not be available. If this example is intended for server-side usage, consider usingsafeStringify. If it's genuinely client-side, a brief note clarifying thatsafeStringifyis preferred for server-side code would help maintain consistency with project conventions.As per coding guidelines: "Never use JSON.stringify; use the
safeStringifyfunction instead, imported from@voltagent/internal"packages/core/src/agent/feedback.spec.ts (1)
14-41: Consider adding a test case forfeedbackId-only detection.
isFeedbackProvidedhas three detection paths (provided,providedAt,feedbackId), but only two are tested here. ThefeedbackId-only case is missing:💡 Suggested additional test case
expect( isFeedbackProvided({ traceId: "trace-1", key: "satisfaction", url: "https://example.com/fb", providedAt: "2026-02-12T00:00:00.000Z", }), ).toBe(true); + + expect( + isFeedbackProvided({ + traceId: "trace-1", + key: "satisfaction", + url: "https://example.com/fb", + feedbackId: "fb-123", + }), + ).toBe(true); });
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/agent/feedback.ts`:
- Around line 154-158: The code currently calls .trim() on feedback.traceId,
feedback.key, and feedback.url which can throw if any are undefined; update the
extraction to guard like the tokenId logic: use typeof checks (e.g., typeof
feedback.traceId === "string" ? feedback.traceId.trim() : "") or default to ""
for feedbackTraceId, feedbackKey, and feedbackUrl (or validate and throw a clear
error before trimming) so trimming is only done on strings and runtime
TypeErrors are avoided.
🧹 Nitpick comments (2)
packages/core/src/agent/feedback.ts (1)
249-258: ConsiderObject.assigninstead of manual field-by-field copy.All enumerable properties of
updatedare copied ontofeedbackHandleone by one.Object.assign(feedbackHandle, updated)achieves the same result more concisely and avoids needing to update this block when new fields are added toAgentFeedbackMetadata.♻️ Proposed simplification
if (updated) { - feedbackHandle.traceId = updated.traceId; - feedbackHandle.key = updated.key; - feedbackHandle.url = updated.url; - feedbackHandle.tokenId = updated.tokenId; - feedbackHandle.expiresAt = updated.expiresAt; - feedbackHandle.feedbackConfig = updated.feedbackConfig; - feedbackHandle.provided = updated.provided; - feedbackHandle.providedAt = updated.providedAt; - feedbackHandle.feedbackId = updated.feedbackId; + Object.assign(feedbackHandle, updated); }website/observability/feedback.md (1)
275-311: Documentation examples useJSON.stringify; consider usingsafeStringifyfor consistency.The
submitFeedback(line 278) andmarkFeedbackProvided(line 309) examples useJSON.stringify. While this.mdfile isn't directly subject to the**/*.tsguideline, users will copy these snippets into their TypeScript code. Consider usingsafeStringifyfrom@voltagent/internalin the examples to promote the project's convention and avoid users introducingJSON.stringifyinto their code. As per coding guidelines, "Never use JSON.stringify; use thesafeStringifyfunction instead, imported from@voltagent/internal".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/src/agent/feedback.ts">
<violation number="1" location="packages/core/src/agent/feedback.ts:156">
P2: Defaulting missing feedback identifiers to "" can cause false matches: if feedback lacks tokenId/traceId/key/url, the empty-string comparison will match any message missing those fields. Use undefined (or non-empty checks) so matching only occurs when identifiers are present.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const feedbackTraceId = typeof feedback.traceId === "string" ? feedback.traceId.trim() : ""; | ||
| const feedbackKey = typeof feedback.key === "string" ? feedback.key.trim() : ""; | ||
| const feedbackUrl = typeof feedback.url === "string" ? feedback.url.trim() : ""; |
There was a problem hiding this comment.
P2: Defaulting missing feedback identifiers to "" can cause false matches: if feedback lacks tokenId/traceId/key/url, the empty-string comparison will match any message missing those fields. Use undefined (or non-empty checks) so matching only occurs when identifiers are present.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/agent/feedback.ts, line 156:
<comment>Defaulting missing feedback identifiers to "" can cause false matches: if feedback lacks tokenId/traceId/key/url, the empty-string comparison will match any message missing those fields. Use undefined (or non-empty checks) so matching only occurs when identifiers are present.</comment>
<file context>
@@ -153,9 +153,9 @@ export function findFeedbackMessageId(
- const feedbackTraceId = feedback.traceId.trim();
- const feedbackKey = feedback.key.trim();
- const feedbackUrl = feedback.url.trim();
+ const feedbackTraceId = typeof feedback.traceId === "string" ? feedback.traceId.trim() : "";
+ const feedbackKey = typeof feedback.key === "string" ? feedback.key.trim() : "";
+ const feedbackUrl = typeof feedback.url === "string" ? feedback.url.trim() : "";
</file context>
| const feedbackTraceId = typeof feedback.traceId === "string" ? feedback.traceId.trim() : ""; | |
| const feedbackKey = typeof feedback.key === "string" ? feedback.key.trim() : ""; | |
| const feedbackUrl = typeof feedback.url === "string" ? feedback.url.trim() : ""; | |
| const feedbackTraceId = | |
| typeof feedback.traceId === "string" && feedback.traceId.trim().length > 0 | |
| ? feedback.traceId.trim() | |
| : undefined; | |
| const feedbackKey = | |
| typeof feedback.key === "string" && feedback.key.trim().length > 0 | |
| ? feedback.key.trim() | |
| : undefined; | |
| const feedbackUrl = | |
| typeof feedback.url === "string" && feedback.url.trim().length > 0 | |
| ? feedback.url.trim() | |
| : undefined; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/src/agent/feedback.ts`:
- Around line 190-196: The current equality check using messageTraceId ===
feedbackTraceId && messageKey === feedbackKey && messageUrl === feedbackUrl can
false-positive when all three feedback fallbacks are empty strings; add a
short-circuit guard before this check that skips matching if feedbackTraceId,
feedbackKey, and feedbackUrl are all empty/blank (or falsy), e.g., require at
least one non-empty feedback field before returning message.id so you don't
accidentally match messages with missing/empty feedback fields.
🧹 Nitpick comments (2)
packages/core/src/agent/feedback.ts (1)
249-258: Fragile manual field-copy — considerObject.assignto stay in sync withAgentFeedbackMetadata.If
AgentFeedbackMetadatagains new fields, this manual copy will silently drop them from the local handle state.Object.assign(feedbackHandle, updated)would be future-proof and shorter.♻️ Suggested fix
if (updated) { - feedbackHandle.traceId = updated.traceId; - feedbackHandle.key = updated.key; - feedbackHandle.url = updated.url; - feedbackHandle.tokenId = updated.tokenId; - feedbackHandle.expiresAt = updated.expiresAt; - feedbackHandle.feedbackConfig = updated.feedbackConfig; - feedbackHandle.provided = updated.provided; - feedbackHandle.providedAt = updated.providedAt; - feedbackHandle.feedbackId = updated.feedbackId; + Object.assign(feedbackHandle, updated); }packages/core/src/agent/feedback.spec.ts (1)
15-41: Consider adding a case forfeedbackId-only detection.
isFeedbackProvidedhas three independent marker checks (provided,providedAt,feedbackId), but only the first two are exercised here. Adding a case with onlyfeedbackIdset would close the coverage gap.♻️ Suggested addition
expect( isFeedbackProvided({ traceId: "trace-1", key: "satisfaction", url: "https://example.com/fb", providedAt: "2026-02-12T00:00:00.000Z", }), ).toBe(true); + + expect( + isFeedbackProvided({ + traceId: "trace-1", + key: "satisfaction", + url: "https://example.com/fb", + feedbackId: "fb-1", + }), + ).toBe(true); });
| if ( | ||
| messageTraceId === feedbackTraceId && | ||
| messageKey === feedbackKey && | ||
| messageUrl === feedbackUrl | ||
| ) { | ||
| return message.id; | ||
| } |
There was a problem hiding this comment.
Empty-string fallback match can produce false positives.
When feedbackTraceId, feedbackKey, and feedbackUrl all resolve to "" (e.g., malformed input), the triple equality on Line 190-194 will match any message whose feedback fields are also empty or missing. This silently returns the wrong message ID.
Consider short-circuiting when all three fallback fields are empty:
♻️ Suggested guard
+ if (!feedbackTraceId && !feedbackKey && !feedbackUrl) {
+ return undefined;
+ }
+
for (let index = messages.length - 1; index >= 0; index--) {🤖 Prompt for AI Agents
In `@packages/core/src/agent/feedback.ts` around lines 190 - 196, The current
equality check using messageTraceId === feedbackTraceId && messageKey ===
feedbackKey && messageUrl === feedbackUrl can false-positive when all three
feedback fallbacks are empty strings; add a short-circuit guard before this
check that skips matching if feedbackTraceId, feedbackKey, and feedbackUrl are
all empty/blank (or falsy), e.g., require at least one non-empty feedback field
before returning message.id so you don't accidentally match messages with
missing/empty feedback fields.
1 participant
…tadata
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
What is the new behavior?
fixes (issue)
Notes for reviewers
Summary by cubic
Adds persisted “feedback provided” markers to assistant message metadata and returns a feedback handle from generate/stream results so feedback UIs stay hidden after reloads. Updates server schema, exported types, docs, and tests.
New Features
Migration
Written for commit a25ea6a. Summary will update on new commits.
Summary by CodeRabbit
New Features
API / Types
Documentation
Tests