Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — This PR adds message feedback persistence (thumbs up/down) across the full stack — new Key changes
Summary ｜ 18 files ｜ 9 commits ｜ base: Message feedback database schema
The table uses the standard
Data access layer
Run API feedback endpoints
The routes use
Manage API conversation feedback endpoint
Playground UI feedback wiring
CI: stable preview URL comments
The script uses an HTML comment marker (
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Urgency: medium — This PR bundles two unrelated features (CI preview URL comments + message feedback persistence). The feedback feature is well-structured overall but has one data-integrity issue that should be addressed before merge.
PR scope mismatch: The title says "ci: surface stable preview URLs in PRs" but ~85% of the diff (15 of 18 files, ~5900 of ~6000 added lines) implements message feedback persistence — a new message_feedback table, DAL functions, API routes on both manage and run domains, integration tests, and a playground UI hook. Consider splitting this into two PRs or at minimum updating the title and description to reflect the actual content.
| const conversation = await getConversation(runDbClient)({ | ||
| scopes: { tenantId, projectId }, | ||
| conversationId, | ||
| }); | ||
| if (!conversation) { | ||
| throw createApiError({ code: 'not_found', message: 'Conversation not found' }); | ||
| } | ||
|
|
||
| const message = await getMessageById(runDbClient)({ | ||
| scopes: { tenantId, projectId }, | ||
| messageId, | ||
| }); | ||
| if (!message) { | ||
| throw createApiError({ code: 'not_found', message: 'Message not found' }); | ||
| } |
There was a problem hiding this comment.
getMessageById filters only by (scopes, messageId) — it does not verify the message belongs to conversationId. If a caller provides a valid conversationId and a messageId from a different conversation in the same tenant/project, the feedback row is saved with the mismatched conversationId.
Either:
- After fetching the message, assert
message.conversationId === conversationId, or - Use a query that includes
conversationIdin the WHERE clause.
The FK to conversations won't catch this because the conversationId in the insert payload is the one from the URL path, not from the message row.
| import { createProtectedRoute, inheritedRunApiKeyAuth } from '@inkeep/agents-core/middleware'; | ||
| import runDbClient from '../../../data/db/runDbClient'; | ||
|
|
||
| type AppVariables = { |
There was a problem hiding this comment.
The AppVariables type is redeclared here, but it's already exported from ../../types. Use the shared type to avoid drift.
| type AppVariables = { | |
| import type { AppVariables } from '../../../types'; |
| type AppVariables = { | |
| import type { AppVariables } from '../../../types'; |
| ALTER TABLE "message_feedback" ADD CONSTRAINT "message_feedback_conversation_fk" FOREIGN KEY ("tenant_id","project_id","conversation_id") REFERENCES "public"."conversations"("tenant_id","project_id","id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "message_feedback" ADD CONSTRAINT "message_feedback_message_fk" FOREIGN KEY ("tenant_id","project_id","message_id") REFERENCES "public"."messages"("tenant_id","project_id","id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| CREATE INDEX "message_feedback_conversation_idx" ON "message_feedback" USING btree ("tenant_id","project_id","conversation_id");--> statement-breakpoint | ||
| CREATE INDEX "message_feedback_message_idx" ON "message_feedback" USING btree ("tenant_id","project_id","message_id"); No newline at end of file |
There was a problem hiding this comment.
Missing trailing newline — git diff flagged \ No newline at end of file. Minor, but worth fixing for clean diffs.
| messageId, | ||
| type: body.type, | ||
| reasons: body.reasons ?? null, | ||
| userId: executionContext.metadata?.endUserId ?? null, |
There was a problem hiding this comment.
The userId field on the feedback row is populated from executionContext.metadata?.endUserId, but the unique constraint is (tenantId, projectId, messageId) — not scoped by user. This means if two different end-users submit feedback on the same message, the second overwrites the first. If that's the intended design (one canonical feedback per message), that's fine, but worth a conscious acknowledgment since the userId column exists and suggests per-user tracking was considered.
| async onFeedback(feedback) { | ||
| try { | ||
| const response = await fetch( | ||
| `${PUBLIC_INKEEP_AGENTS_API_URL}/run/v1/conversations/${conversationId}/messages/${feedback.messageId}/feedback`, |
There was a problem hiding this comment.
The conversationId here comes from the component prop (the playground session ID). If the widget SDK's feedback.messageId ever refers to a message in a different conversation context (e.g. after a conversation reset without re-mounting), this URL would be wrong. Consider adding a defensive check or using the conversation ID from the message context if available.
vnv-varun
force-pushed
the
varun/preview-stable-urls
branch
from
d6fe781 to
77c9096
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a clean, low-risk CI enhancement that surfaces stable preview URLs (pr-<n>-*.preview.inkeep.com) as a sticky PR comment after successful smoke checks.
✅ What's Good
Well-structured implementation:
- Uses established patterns from existing preview scripts (
common.sh,require_env_vars) - Proper error handling with
set -euo pipefail - Safe JSON encoding with
jq -Rsfor arbitrary content - Connection and max-time timeouts on curl calls
- Idempotent upsert via HTML comment marker (
<!-- preview-environments:stable-urls -->)
Correct workflow integration:
issues: writepermission is appropriate (PR comments use the Issues API)- Step condition correctly gates on
steps.smoke.outcome == 'success' - All required env vars are passed from workflow outputs
Security:
- Uses
${{ github.token }}which is repo-scoped - URL variables come from controlled workflow outputs, not user input
💭 Consider (2)
Minor hygiene improvements noted as inline comments:
- 💭 Consider:
.github/scripts/preview/comment-preview-urls.sh:19— Add cleanup trap for temp file - 💭 Consider:
.github/scripts/preview/comment-preview-urls.sh:54— Pagination limited to 100 comments
Neither affects correctness or security for typical PRs.
⚠️ Note on Prior Review
The pullfrog bot review mentions "message feedback persistence" features that are not present in the current PR diff. The current PR contains only CI/preview URL changes (2 files, 96 additions). The prior review appears to be from a different version of the branch or a different PR entirely — those findings are not applicable to this changeset.
✅ APPROVE
Summary: This is a well-implemented CI enhancement that follows existing patterns and improves developer experience by making stable preview URLs more discoverable. No blocking issues — ship it! 🚀
Note: Unable to submit formal approval due to GitHub App permissions. This review recommends approval.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
5 | 0 | 0 | 0 | 2 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 5 | 0 | 0 | 0 | 2 | 0 | 0 |
Note: 3 of the 5 devops findings were positive observations (INFO level), not issues.
| COMMENT_MARKER="<!-- preview-environments:stable-urls -->" | ||
| COMMENTS_ENDPOINT="${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" | ||
| API_HEALTH_URL="${API_URL%/}/health" | ||
| COMMENT_BODY_FILE="$(mktemp)" |
There was a problem hiding this comment.
💭 Consider: Add cleanup trap for temp file
Issue: The temporary file created with mktemp is not explicitly cleaned up on exit.
Why: While GitHub Actions runners are ephemeral and the file will be deleted when the runner terminates, explicit cleanup is a best practice and improves local testability of the script.
Fix: Add a trap after this line:
trap 'rm -f "${COMMENT_BODY_FILE}"' EXITRefs:
- provision-railway.sh — uses similar temp file patterns
| curl --connect-timeout 10 --max-time 30 -fsS \ | ||
| -H "Authorization: Bearer ${GITHUB_TOKEN}" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| "${COMMENTS_ENDPOINT}?per_page=100" |
There was a problem hiding this comment.
💭 Consider: Pagination limited to 100 comments
Issue: The GitHub API comments endpoint is paginated and the query fetches only per_page=100. If a PR has more than 100 comments, the existing preview URLs comment might not be found, leading to duplicate comments instead of an upsert.
Why: While uncommon, PRs with extensive discussion or automated bot activity could exceed this limit. The jq query correctly selects the last matching comment, but if the marker comment isn't in the first 100, it won't be found.
Fix: This is a known edge case that's acceptable for most PRs. If you want complete coverage, you'd need to iterate through pages. Alternatively, document this as a known limitation since busy PRs with 100+ comments are rare.
Refs:
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
Ito Test Report ✅18 test cases ran. 18 passed. All 18 test cases passed with zero failures, confirming the preview URL publishing flow is functioning as designed end-to-end: sticky marker comments are created/updated idempotently, stable UI/API/API-health links are consistently published (including /health as expected 204), raw deployment URLs stay tucked in collapsed details, and behavior remains stable across reruns, rapid back/forward navigation, concurrent rerun/comment churn, and mobile viewing. Key safeguards also held in negative/adversarial scenarios (invalid PR input, smoke-fail and closed-event gating, forged marker protection, malformed URL sanitization, and safe 403 handling without partial writes), while one notable monitored risk was validated that markers pushed beyond the newest 100 comments can be missed by first-page queries, creating potential duplicate-comment risk if pagination is not accounted for. ✅ Passed (18)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
Summary
Surface the stable
pr-<n>-*.preview.inkeep.compreview URLs directly in the PR conversation so reviewers can find the right links without digging through Vercel bot comments.Changes
Publish stable preview URLsstep after successful preview smoke checks.github/scripts/preview/comment-preview-urls.shto create or update a single sticky GitHub Actions commentWhy
The preview workflow already computes and aliases stable preview URLs, but they were easy to miss because the default Vercel comment is more prominent and uses deployment-specific hashes. This change makes the stable aliases the reviewer-facing entry point without exposing Railway infrastructure URLs.
Test Plan
bash -n .github/scripts/preview/*.sh.github/workflows/preview-environments.ymlwithyaml.safe_loadgit diff --checkNotes
This intentionally surfaces the stable Vercel alias URLs, not Railway service endpoints.