[Apps] Adding support app alpha and dogfooding#1368
Open
madster456 wants to merge 20 commits intodevfrom
Open
Conversation
…and dogfooding our own support form. No inbound email is present, so we cannot reply, but we can recieve for now.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Conversations/support feature: DB migrations and Prisma models, backend conversation library and APIs (public + internal), dashboard UI and client API, SLA helpers and schema, shared types/validation, migration/E2E tests, and app registration entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Dashboard
participant API as Backend API
participant DB as Database
Client->>API: POST /internal/conversations (projectId,userId,subject,initialMessage,priority,source)
API->>API: Validate auth & payload
API->>DB: BEGIN TRANSACTION
API->>DB: INSERT Conversation row
API->>DB: INSERT ConversationEntryPoint row
API->>DB: INSERT initial ConversationMessage
API->>DB: COMMIT
API->>DB: SELECT conversation detail (with messages)
DB-->>API: conversation + messages
API-->>Client: 200 { conversationId }
sequenceDiagram
participant Client as Client/Dashboard
participant API as Backend API
participant DB as Database
Client->>API: PATCH /internal/conversations/:id (type=reply|internal-note|status|metadata)
API->>API: Validate auth & payload
API->>DB: BEGIN TRANSACTION
alt reply/internal-note
API->>DB: Ensure or INSERT ConversationEntryPoint
API->>DB: INSERT ConversationMessage
API->>DB: UPDATE Conversation timestamps/status/SLA fields
else status
API->>DB: UPDATE Conversation.status & closedAt
API->>DB: INSERT status-change ConversationMessage
else metadata
API->>DB: UPDATE assignedTo/priority/tags
end
API->>DB: COMMIT
API->>DB: SELECT conversation detail (with messages)
DB-->>API: updated conversation
API-->>Client: 200 { conversation }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
madster456
changed the title
Contributor
Greptile SummaryThis PR introduces a full conversations/support-inbox feature: new Confidence Score: 5/5Safe to merge; all findings are P2 style/quality issues that do not affect correctness or security in current usage. No P0/P1 defects found. The LIKE metacharacter issue and TOCTOU state-read are both practically harmless at this traffic level and feature scope. The runAsynchronously swap is a style rule violation but errors are already caught inline. The 200-row cap and channelType/source conflation are noted for future cleanup. apps/backend/src/lib/conversations.tsx (LIKE escaping, pagination cap, TOCTOU); apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page-client.tsx (runAsynchronouslyWithAlert rule) Important Files Changed
Sequence DiagramsequenceDiagram
participant User as End User (client)
participant PubAPI as /api/latest/conversations
participant IntAPI as /api/latest/internal/conversations
participant Dashboard as Dashboard UI
participant ConvLib as conversations.tsx
participant DB as Postgres
User->>PubAPI: POST /conversations {subject, message}
PubAPI->>ConvLib: createConversation(tenancyId, userId, ...)
ConvLib->>DB: INSERT Conversation + EntryPoint + Message (tx)
DB-->>ConvLib: conversationId
ConvLib-->>PubAPI: {conversationId}
PubAPI-->>User: 200 {conversation_id}
User->>PubAPI: GET /conversations/[id]
PubAPI->>ConvLib: getConversationDetail(includeInternalNotes=false)
ConvLib->>DB: SELECT Conversation + Messages (no internal-notes)
DB-->>ConvLib: rows
ConvLib-->>PubAPI: ConversationDetailResponse
PubAPI-->>User: 200 (public snake_case shape)
Dashboard->>IntAPI: GET /internal/conversations?projectId=...
IntAPI->>ConvLib: listConversationSummaries(includeInternalNotes=true)
ConvLib->>DB: SELECT + LATERAL latest message
DB-->>ConvLib: rows (up to 200)
ConvLib-->>IntAPI: ConversationSummary[]
IntAPI-->>Dashboard: 200 {conversations}
Dashboard->>IntAPI: PATCH /internal/conversations/[id] {type: reply}
IntAPI->>ConvLib: appendConversationMessage(agent sender)
ConvLib->>DB: INSERT Message + UPDATE Conversation status/timestamps (tx)
DB-->>ConvLib: ok
IntAPI->>ConvLib: getConversationDetail(includeInternalNotes=true)
ConvLib-->>IntAPI: updated detail
IntAPI-->>Dashboard: 200 updated conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
# Conflicts: # docs-mintlify/guides/other/tutorials/build-a-saas-with-stack-auth.mdx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
apps/dashboard/src/lib/apps-frontend.tsx (1)
225-225: Minor:getScreenshots('support', 0)is just an empty array.Other entries without screenshots use
screenshots: []directly (e.g.,fraud-protection,onboarding,tv-mode). For consistency and to avoid a no-op helper call, consider:- screenshots: getScreenshots('support', 0), + screenshots: [],Also fine to leave as-is if you plan to add screenshots soon.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/apps-frontend.tsx` at line 225, Replace the no-op helper call screenshots: getScreenshots('support', 0) with an explicit empty array screenshots: [] to match other entries and avoid an unnecessary function call; locate the screenshots property in apps-frontend.tsx (where getScreenshots is currently invoked) and update that entry to use [] (or keep getScreenshots only if you plan to add screenshots soon).packages/stack-shared/src/interface/conversations.ts (1)
37-59: Mixed.defined()/.optional()onconversationSummarySchemacreates an inconsistent contract.Fields like
lastActivityAtare.defined()while closely related timestamps (createdAt,updatedAt,lastMessageAt,lastInboundAt,lastOutboundAt,closedAt) are.optional(). If the backend always populates these (which the summary-shape suggests), prefer.defined()(with.nullable()where absence is meaningful) so consumers don't need to defensively narrowT | undefined. If some are genuinely optional depending on code path, a short comment documenting which producer omits them would help downstream consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-shared/src/interface/conversations.ts` around lines 37 - 59, The conversationSummarySchema currently mixes .defined() and .optional() for timestamp-like fields, causing an inconsistent type contract; decide whether timestamps (createdAt, updatedAt, lastMessageAt, lastActivityAt, lastInboundAt, lastOutboundAt, closedAt) are always produced by the backend and then make them consistently .defined() (use .nullable() for those that can be null) on conversationSummarySchema, or if some are omitted by certain producers, add a short comment above conversationSummarySchema listing which producers may omit which fields and keep those specific fields as .optional(); update the schema accordingly (referencing conversationSummarySchema and the individual field names listed) so consumers get a consistent contract.apps/backend/src/app/api/latest/internal/feedback/route.tsx (1)
79-101: Consider idempotency for the dogfood mirror.As the inline comment acknowledges, if
createConversationfails aftersendSupportFeedbackEmailhas already succeeded, the client will retry and produce a duplicate email + duplicate conversation. That's the documented tradeoff, but longer-term it may be worth either:
- running the conversation insert before the email send (so a retry only duplicates the inbox row, which is cheaper/reversible), or
- adding an idempotency key (e.g., hash of
userId + message + minute-bucket) on the conversation insert to collapse retries.Not blocking, but worth a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/feedback/route.tsx` around lines 79 - 101, The dogfood mirror can produce duplicate emails/conversations on retries; to fix, make the conversation write idempotent or reorder operations: either call createConversation before the email send (so retries only duplicate the DB row) or implement an idempotency key for createConversation (e.g., derive a unique key from auth.user.id + body.message + minute-bucket and use it in an upsert/unique constraint) so repeated calls to createConversation with the same key are collapsed; update the code that constructs conversationSubject and calls createConversation to include and persist this idempotency key (or move the createConversation call ahead of sendSupportFeedbackEmail) and ensure the createConversation implementation honors the upsert/unique constraint.apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsx (1)
134-140: Minor: simplify URL construction with a singleurlString.You can build the whole URL (including the query) with one
urlStringtag instead of mixingurlString+ template concatenation +encodeURIComponent:♻️ Proposed simplification
- onClick={() => router.push(`${urlString`/projects/${stackAdminApp.projectId}/conversations`}?userId=${encodeURIComponent(user.id)}`)} + onClick={() => router.push(urlString`/projects/${stackAdminApp.projectId}/conversations?userId=${user.id}`)}As per coding guidelines: "Use urlString`` or encodeURIComponent() instead of normal string interpolation for URLs, for consistency even if it's not strictly necessary".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsx around lines 134 - 140, The Support button currently builds the URL by mixing urlString with template concatenation and encodeURIComponent; update the router.push call in the Button onClick (the component rendering inside page-client.tsx) to use a single urlString template for the entire path and query (referencing urlString, router.push, stackAdminApp.projectId and user.id) so the path and the ?userId=... query are constructed together with urlString and the user id properly encoded within that template; this keeps URL construction consistent with the coding guideline and removes the mixed concatenation.apps/e2e/tests/backend/endpoints/api/v1/support.test.ts (1)
64-64: Encode path parameters in test request URLs.Use
encodeURIComponent()for the interpolatedconversation_idpath segment to match the repository URL-building convention.Proposed fix
- const detailResponse = await niceBackendFetch(`/api/v1/conversations/${createResponse.body.conversation_id}`, { + const detailResponse = await niceBackendFetch(`/api/v1/conversations/${encodeURIComponent(createResponse.body.conversation_id)}`, { ... - const replyResponse = await niceBackendFetch(`/api/v1/conversations/${createResponse.body.conversation_id}`, { + const replyResponse = await niceBackendFetch(`/api/v1/conversations/${encodeURIComponent(createResponse.body.conversation_id)}`, { ... - const detailResponse = await niceBackendFetch(`/api/v1/conversations/${createResponse.body.conversation_id}`, { + const detailResponse = await niceBackendFetch(`/api/v1/conversations/${encodeURIComponent(createResponse.body.conversation_id)}`, {As per coding guidelines, “Use urlString`` or encodeURIComponent() instead of normal string interpolation for URLs, for consistency even if it's not strictly necessary.”
Also applies to: 129-129, 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/support.test.ts` at line 64, The test builds request URLs using string interpolation without encoding path segments; update the calls to niceBackendFetch that use createResponse.body.conversation_id (and the other occurrences noted) to wrap the conversation id with encodeURIComponent() (or use the repository's urlString`` helper) so path parameters are properly escaped — locate the URL construction in the test where niceBackendFetch is called and replace the raw ${createResponse.body.conversation_id} interpolation with an encoded value.apps/dashboard/src/lib/conversations.ts (1)
1-116: LGTM, with a small nit on the discriminator check.The union-narrowing IIFE uses
"body" in options/"status" in optionsto build the payload. It works today because each variant is disjoint on those keys, but it will silently misbehave if a future variant carriesbodyorstatusalongside other fields. Switching onoptions.typeis more robust and keeps TypeScript's narrowing exhaustive.Suggested refactor
- const payload = (() => { - if ("body" in options) { - return { body: options.body }; - } - if ("status" in options) { - return { status: options.status }; - } - return { - assignedToUserId: options.assignedToUserId, - assignedToDisplayName: options.assignedToDisplayName, - priority: options.priority, - tags: options.tags, - }; - })(); + const payload = (() => { + switch (options.type) { + case "internal-note": + case "reply": + return { body: options.body }; + case "status": + return { status: options.status }; + case "metadata": + return { + assignedToUserId: options.assignedToUserId, + assignedToDisplayName: options.assignedToDisplayName, + priority: options.priority, + tags: options.tags, + }; + } + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/conversations.ts` around lines 1 - 116, The payload construction in appendConversationUpdate currently narrows the union by checking `"body" in options` / `"status" in options`, which can break if future variants include those keys; change the IIFE to switch on options.type inside appendConversationUpdate (or use if/else on options.type) and return the exact payload for each case ("internal-note" | "reply" -> { body }, "status" -> { status }, "metadata" -> { assignedToUserId, assignedToDisplayName, priority, tags }), and include an exhaustive default that throws for unknown types so TypeScript forces updates if new variants are added.apps/backend/src/lib/conversations.tsx (1)
811-846: Two sequentialUPDATEs on the sameConversationrow can be merged.
appendConversationMessageruns oneUPDATEthat setsstatus/updatedAt/lastMessageAt/lastInbound/OutboundAtand then a secondUPDATEthat re-setsupdatedAtand adjustsfirstResponseAt/lastCustomerReplyAt/lastAgentReplyAt/nextResponseDueAt. That's two writes, two WAL records, and two index-maintenance passes per message append — hot path when a conversation is busy. All the conditional-keep logic ("lastInboundAt" = ${... ? now : "lastInboundAt"}) works just as well inside a single statement.Sketch
- const conversationSetParts: Prisma.Sql[] = []; - if (autoStatus != null) { - conversationSetParts.push(Prisma.sql`status = ${autoStatus}`); - } - conversationSetParts.push( - Prisma.sql`"updatedAt" = ${now}`, - Prisma.sql`"lastMessageAt" = ${now}`, - Prisma.sql`"lastInboundAt" = ${...}`, - Prisma.sql`"lastOutboundAt" = ${...}`, - ); - await tx.$executeRaw(Prisma.sql`UPDATE "Conversation" SET ${Prisma.join(conversationSetParts, ", ")} WHERE ...`); - await tx.$executeRaw(Prisma.sql`UPDATE "Conversation" SET "updatedAt"=${now}, "firstResponseAt"=..., ...`); + const parts: Prisma.Sql[] = [ + Prisma.sql`"updatedAt" = ${now}`, + Prisma.sql`"lastMessageAt" = ${now}`, + Prisma.sql`"lastInboundAt" = ${shouldTrackReplies && sender.type === "user" ? Prisma.sql`${now}` : Prisma.sql`"lastInboundAt"`}`, + Prisma.sql`"lastOutboundAt" = ${shouldTrackReplies && sender.type === "agent" ? Prisma.sql`${now}` : Prisma.sql`"lastOutboundAt"`}`, + Prisma.sql`"lastCustomerReplyAt" = ${shouldTrackReplies && sender.type === "user" ? Prisma.sql`${now}` : Prisma.sql`"lastCustomerReplyAt"`}`, + Prisma.sql`"lastAgentReplyAt" = ${shouldTrackReplies && sender.type === "agent" ? Prisma.sql`${now}` : Prisma.sql`"lastAgentReplyAt"`}`, + Prisma.sql`"firstResponseAt" = ${nextFirstResponseAt == null ? Prisma.sql`"firstResponseAt"` : Prisma.sql`${nextFirstResponseAt}`}`, + Prisma.sql`"nextResponseDueAt" = ${shouldClearNextResponseDueAt ? Prisma.sql`NULL` : nextResponseDueAt != null ? Prisma.sql`${nextResponseDueAt}` : Prisma.sql`"nextResponseDueAt"`}`, + ]; + if (autoStatus != null) parts.unshift(Prisma.sql`status = ${autoStatus}`); + await tx.$executeRaw(Prisma.sql`UPDATE "Conversation" SET ${Prisma.join(parts, ", ")} WHERE "tenancyId" = ${options.tenancyId}::uuid AND id = ${options.conversationId}::uuid`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/conversations.tsx` around lines 811 - 846, Merge the two sequential UPDATEs into a single UPDATE statement so the row is written once: build a single conversationSetParts (or replace it) to include all columns currently set in both updates (status, "updatedAt", "lastMessageAt", "lastInboundAt", "lastOutboundAt", "firstResponseAt", "lastCustomerReplyAt", "lastAgentReplyAt", "nextResponseDueAt") and use Prisma.join to inject them into one tx.$executeRaw call; preserve the existing conditional expressions (the ternaries that produce Prisma.sql`${now}` or the quoted column name, nextFirstResponseAt handling, and the shouldClearNextResponseDueAt / nextResponseDueAt logic) so the resulting UPDATE produces identical assignments and WHERE filtering as before. Ensure you remove the second tx.$executeRaw and only execute the consolidated UPDATE for the same tenancyId / conversationId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/backend/prisma/migrations/20260420000000_add_conversations/tests/creates-conversation-tables.ts`:
- Around line 151-195: Tests currently only assert failures for
Conversation_status_check and ConversationMessage_senderType_check; add similar
negative tests that insert rows with invalid enum values to cover
Conversation_priority_check, Conversation_source_check,
ConversationEntryPoint_type_check, and ConversationMessage_messageType_check.
Follow the existing pattern used for the two rejects.toThrow assertions: create
INSERT statements against "Conversation" with an invalid priority and an invalid
source to expect Conversation_priority_check and Conversation_source_check
errors, create an INSERT against "ConversationEntryPoint" with an invalid type
to expect ConversationEntryPoint_type_check, and create an INSERT against
"ConversationMessage" with an invalid messageType to expect
ConversationMessage_messageType_check; use
randomUUID()/ctx.tenancyId/ctx.projectUserId/conversationId as in the existing
snippets and assert rejects.toThrow(/ConstraintName/).
In `@apps/backend/prisma/schema.prisma`:
- Around line 1167-1172: The Conversation model's relation to ProjectUser
(field: projectUser, relation: ProjectUser? via fields [tenancyId,
projectUserId]) currently uses onDelete: Cascade which deletes Conversation (and
its ConversationMessage/ConversationEntryPoint children) when a ProjectUser is
removed; change this to onDelete: SetNull on the Conversation.projectUser
relation (and similarly for the team relation) and update the ProjectUser delete
flow in apps/backend/src/app/api/latest/users/crud.tsx to explicitly clear or
anonymize PII fields on related Conversation records (e.g., null the
projectUser/team FK and redact user-identifying fields) so conversation history
is preserved but user data is sanitized, or alternatively add a clear documented
guard if you intentionally want cascading deletion.
In `@apps/backend/src/app/api/latest/conversations/`[conversationId]/route.tsx:
- Around line 59-61: The message validation schema in route.tsx uses
yupString().trim().min(1).defined() with no upper bound, allowing arbitrarily
large submissions; update the schema inside the body yupObject in
apps/backend/src/app/api/latest/conversations/[conversationId]/route.tsx to add
an appropriate max (e.g., .max(5000)) to the message field so it matches the cap
used in the internal feedback endpoint and prevents oversized
ConversationMessage.body entries.
In `@apps/backend/src/lib/conversations.tsx`:
- Around line 405-473: The search pattern built in searchPattern uses raw
`%${options.query.trim().toLowerCase()}%` which lets user `%` and `_` act as
LIKE wildcards; fix by escaping backslashes first and then replacing `%` and `_`
in options.query (e.g., turn "\" -> "\\", "%" -> "\%", "_" -> "\_") before
wrapping with `%...%`, update the reference to searchPattern used in the
Prisma.sql block, and add an explicit ESCAPE '\' clause to each LIKE expression
(the LOWER(c.subject) LIKE ..., LOWER(COALESCE(lm.body, '')) LIKE ...,
LOWER(COALESCE(pu."displayName", '')) LIKE ..., LOWER(COALESCE(cc."value", ''))
LIKE ...) so the escaped characters are treated literally.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/conversations/page-client.tsx:
- Around line 1324-1348: Replace the bare try/catch + toast error handling
around the appendConversationUpdate call with the project-wide asynchronous
helper so failures surface as blocking alerts and are logged: wrap the async
work that calls appendConversationUpdate (and then calls setConversationDetail,
setConversations, setRefreshKey) using runAsynchronouslyWithAlert (or
runAsynchronously if you show an inline DesignAlert like SupportComposer does),
and remove the now-unused useToast/toast binding; ensure the error path uses the
alert helper so the failure is shown inline and the error is logged rather than
swallowed.
- Around line 937-942: currentOwnerTeam resolution currently calls throwErr
inside a useMemo during render (via currentOwnerTeam = useMemo(... ??
throwErr(...))) which will hard-crash the page if the owner team is missing or
project.ownerTeamId is null; change this to a safe render-time check: remove the
inline throwErr from the useMemo, let useMemo return undefined when not found,
then branch in the component render to either show a graceful fallback/error UI
when currentOwnerTeam is undefined or, if you must treat it as impossible, call
throwErr later after you’ve rendered a guard that guarantees the team exists;
also ensure assignableTeamMembers = currentOwnerTeam.useUsers() does not call
the hook conditionally — either call useUsers unconditionally with a skip flag
(if supported) or restructure to early-return before any hook that depends on
currentOwnerTeam runs so hooks order remains stable.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/support-settings/page-client.tsx:
- Line 67: The onBlur handler currently calls saveSla(...) with a voided promise
which swallows failures; replace those voided calls with the project's async
error helper (e.g., call runAsynchronously or runAsynchronouslyWithAlert) so
rejected promises from saveSla (and ultimately updateConfig inside it) surface
errors/alerts; update the onBlur usages that pass saveSla({ ...sla,
firstResponseMinutes: parseMinutes(...) }) (and the similar second occurrence)
to invoke the chosen runner with the saveSla call instead of prefixing with
void.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/support/page.tsx:
- Around line 1-6: The Page currently uses `await params` (the async `Page`
function), forcing a dynamic route; replace this with a static approach: either
add a static redirect in `next.config.js` via the `redirects()` export for the
`/projects/:projectId/support` -> `/projects/:projectId/conversations` path, or
convert the default export `Page` into a client component that does not `await
params` — create a client component (remove `async` and `await params`), import
`useParams` and `useRouter` from `next/navigation`, read `projectId =
useParams().projectId` and call
`router.replace(`/projects/${projectId}/conversations`)` inside a `useEffect` so
the page stays static and does a client-side redirect; update the file to export
that client component instead of the async server `Page`.
In `@apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts`:
- Around line 48-57: The test is flaky because it matches conversations by a
non-unique subject and uses an untyped cast; update the authenticated test to
uniquely identify the conversation for this run and use a typed helper: when
creating the support conversation capture the POST response (use the
conversationId if returned) or otherwise use
backendContext.value.mailbox.emailAddress + randomUUID() in the
senderEmail/subject to guarantee uniqueness, then fetch /api/v1/conversations
and filter by that conversationId (or sort/filter by most recent lastActivityAt)
instead of find(c => c.subject === subject); also replace the ad-hoc (c: {
subject: string }) cast with a ConversationSummary typed predicate/helper so
fromSupportForm has proper types for .source and .preview.
In `@claude/CLAUDE-KNOWLEDGE.md`:
- Around line 1-2: The doc contains conflicting schema guidance; update the
entries at the referenced sections (including the text currently referring to
ConversationChannel and ConversationMetadata around lines 222-225 and 394-399)
so they reflect the final model: remove any references to ConversationMetadata
and ConversationChannel, state that assignment/SLA/metadata fields are kept
directly on Conversation, that channel ingress rows are modeled in
ConversationEntryPoint (renamed from ConversationChannel), and that
ConversationMessage.channelId references entry points by (tenancyId, id); also
ensure any sentences that said support/inbox data lives in
ConversationMetadata/ConversationChannel are changed to say Conversation,
ConversationEntryPoint, and ConversationMessage and keep the backend raw SQL
note about apps/backend/src/lib/conversations.tsx reading/writing metadata from
Conversation.
In `@docs-mintlify/guides/other/tutorials/build-a-saas-with-stack-auth.mdx`:
- Line 6: The MDX references a non-existent plain Markdown file at
`docs-mintlify/guides/other/tutorials/build-a-saas-with-stack-auth.md`; either
remove or update that reference in build-a-saas-with-stack-auth.mdx or add the
missing Markdown file with the same content. Locate the string
`docs-mintlify/guides/other/tutorials/build-a-saas-with-stack-auth.md` in
build-a-saas-with-stack-auth.mdx and either delete the sentence mentioning the
.md, replace it with a correct path/alternative, or create the referenced .md
file mirroring the MDX content so the link is valid.
In `@packages/stack-shared/src/interface/conversations.ts`:
- Around line 72-87: conversationMessageSchema currently includes
conversation-level fields (subject, status, priority, source) that are not
stored on the ConversationMessage row; locate conversationMessageSchema and the
messageFromRow function and either remove those four fields from the
ConversationMessage schema so it matches the DB row shape, or if the app
intentionally denormalizes/ snapshots conversation state into messages, add a
clear code comment above conversationMessageSchema and messageFromRow stating
that subject/status/priority/source are populated from the parent Conversation
(not the message row) and why (snapshot/denormalization/optimization), ensuring
the schema and implementation are consistent and documented.
---
Nitpick comments:
In `@apps/backend/src/app/api/latest/internal/feedback/route.tsx`:
- Around line 79-101: The dogfood mirror can produce duplicate
emails/conversations on retries; to fix, make the conversation write idempotent
or reorder operations: either call createConversation before the email send (so
retries only duplicate the DB row) or implement an idempotency key for
createConversation (e.g., derive a unique key from auth.user.id + body.message +
minute-bucket and use it in an upsert/unique constraint) so repeated calls to
createConversation with the same key are collapsed; update the code that
constructs conversationSubject and calls createConversation to include and
persist this idempotency key (or move the createConversation call ahead of
sendSupportFeedbackEmail) and ensure the createConversation implementation
honors the upsert/unique constraint.
In `@apps/backend/src/lib/conversations.tsx`:
- Around line 811-846: Merge the two sequential UPDATEs into a single UPDATE
statement so the row is written once: build a single conversationSetParts (or
replace it) to include all columns currently set in both updates (status,
"updatedAt", "lastMessageAt", "lastInboundAt", "lastOutboundAt",
"firstResponseAt", "lastCustomerReplyAt", "lastAgentReplyAt",
"nextResponseDueAt") and use Prisma.join to inject them into one tx.$executeRaw
call; preserve the existing conditional expressions (the ternaries that produce
Prisma.sql`${now}` or the quoted column name, nextFirstResponseAt handling, and
the shouldClearNextResponseDueAt / nextResponseDueAt logic) so the resulting
UPDATE produces identical assignments and WHERE filtering as before. Ensure you
remove the second tx.$executeRaw and only execute the consolidated UPDATE for
the same tenancyId / conversationId.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsx:
- Around line 134-140: The Support button currently builds the URL by mixing
urlString with template concatenation and encodeURIComponent; update the
router.push call in the Button onClick (the component rendering inside
page-client.tsx) to use a single urlString template for the entire path and
query (referencing urlString, router.push, stackAdminApp.projectId and user.id)
so the path and the ?userId=... query are constructed together with urlString
and the user id properly encoded within that template; this keeps URL
construction consistent with the coding guideline and removes the mixed
concatenation.
In `@apps/dashboard/src/lib/apps-frontend.tsx`:
- Line 225: Replace the no-op helper call screenshots: getScreenshots('support',
0) with an explicit empty array screenshots: [] to match other entries and avoid
an unnecessary function call; locate the screenshots property in
apps-frontend.tsx (where getScreenshots is currently invoked) and update that
entry to use [] (or keep getScreenshots only if you plan to add screenshots
soon).
In `@apps/dashboard/src/lib/conversations.ts`:
- Around line 1-116: The payload construction in appendConversationUpdate
currently narrows the union by checking `"body" in options` / `"status" in
options`, which can break if future variants include those keys; change the IIFE
to switch on options.type inside appendConversationUpdate (or use if/else on
options.type) and return the exact payload for each case ("internal-note" |
"reply" -> { body }, "status" -> { status }, "metadata" -> { assignedToUserId,
assignedToDisplayName, priority, tags }), and include an exhaustive default that
throws for unknown types so TypeScript forces updates if new variants are added.
In `@apps/e2e/tests/backend/endpoints/api/v1/support.test.ts`:
- Line 64: The test builds request URLs using string interpolation without
encoding path segments; update the calls to niceBackendFetch that use
createResponse.body.conversation_id (and the other occurrences noted) to wrap
the conversation id with encodeURIComponent() (or use the repository's
urlString`` helper) so path parameters are properly escaped — locate the URL
construction in the test where niceBackendFetch is called and replace the raw
${createResponse.body.conversation_id} interpolation with an encoded value.
In `@packages/stack-shared/src/interface/conversations.ts`:
- Around line 37-59: The conversationSummarySchema currently mixes .defined()
and .optional() for timestamp-like fields, causing an inconsistent type
contract; decide whether timestamps (createdAt, updatedAt, lastMessageAt,
lastActivityAt, lastInboundAt, lastOutboundAt, closedAt) are always produced by
the backend and then make them consistently .defined() (use .nullable() for
those that can be null) on conversationSummarySchema, or if some are omitted by
certain producers, add a short comment above conversationSummarySchema listing
which producers may omit which fields and keep those specific fields as
.optional(); update the schema accordingly (referencing
conversationSummarySchema and the individual field names listed) so consumers
get a consistent contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6dc9c79-ac38-4137-ba2f-a538b5ad936a
📒 Files selected for processing (30)
apps/backend/prisma/migrations/20260420000000_add_conversations/migration.sqlapps/backend/prisma/migrations/20260420000000_add_conversations/tests/creates-conversation-tables.tsapps/backend/prisma/schema.prismaapps/backend/src/app/api/latest/conversations/[conversationId]/route.tsxapps/backend/src/app/api/latest/conversations/route.tsxapps/backend/src/app/api/latest/internal/conversations/[conversationId]/route.tsxapps/backend/src/app/api/latest/internal/conversations/route.tsxapps/backend/src/app/api/latest/internal/feedback/route.tsxapps/backend/src/lib/conversation-types.tsapps/backend/src/lib/conversations-api.tsapps/backend/src/lib/conversations.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/support-settings/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/support-settings/page.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/support/page.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsxapps/dashboard/src/lib/apps-frontend.tsxapps/dashboard/src/lib/conversation-types.tsapps/dashboard/src/lib/conversations.tsapps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.tsapps/e2e/tests/backend/endpoints/api/v1/support.test.tsclaude/CLAUDE-KNOWLEDGE.mddocs-mintlify/guides/other/tutorials/build-a-saas-with-stack-auth.mdxpackages/stack-shared/src/apps/apps-config.tspackages/stack-shared/src/config/schema-fuzzer.test.tspackages/stack-shared/src/config/schema.tspackages/stack-shared/src/helpers/support-sla.test.tspackages/stack-shared/src/helpers/support-sla.tspackages/stack-shared/src/interface/conversations.ts
Comment on lines
+48
to
+57
| const listResponse = await niceBackendFetch("/api/v1/conversations", { | ||
| accessType: "client", | ||
| }); | ||
| expect(listResponse.status).toBe(200); | ||
| const fromSupportForm = listResponse.body.conversations.find( | ||
| (c: { subject: string }) => c.subject === subject, | ||
| ); | ||
| expect(fromSupportForm).toBeDefined(); | ||
| expect(fromSupportForm.source).toBe("api"); | ||
| expect(fromSupportForm.preview).toContain("Authenticated feedback from the dashboard."); |
Contributor
There was a problem hiding this comment.
Test may be flaky due to cross-run conversation accumulation.
Unlike the other two it blocks (which now use randomUUID() in senderEmail per Q/A at line 330 of CLAUDE-KNOWLEDGE.md), this authenticated test uses backendContext.value.mailbox.emailAddress and a fixed [Support] ${senderEmail} subject. If the same mailbox email is reused across runs (or if multiple iterations create conversations with the same subject), conversations.find(c => c.subject === subject) could match an older conversation whose preview still contains the same message text — hiding a real regression, or conversely matching the wrong row if the preview logic changes.
Consider asserting against the conversation's conversationId returned from the POST (if the endpoint can return it), or filtering by the most-recent lastActivityAt, to tie the assertion to this run specifically. At minimum, add a type-safe assertion helper instead of the ad-hoc (c: { subject: string }) cast so source/preview are typed via ConversationSummary.
- const listResponse = await niceBackendFetch("/api/v1/conversations", {
- accessType: "client",
- });
- expect(listResponse.status).toBe(200);
- const fromSupportForm = listResponse.body.conversations.find(
- (c: { subject: string }) => c.subject === subject,
- );
- expect(fromSupportForm).toBeDefined();
- expect(fromSupportForm.source).toBe("api");
- expect(fromSupportForm.preview).toContain("Authenticated feedback from the dashboard.");
+ const listResponse = await niceBackendFetch("/api/v1/conversations", {
+ accessType: "client",
+ });
+ expect(listResponse.status).toBe(200);
+ const matching = (listResponse.body.conversations as ConversationSummary[])
+ .filter((c) => c.subject === subject)
+ .sort((a, b) => b.lastActivityAt.localeCompare(a.lastActivityAt));
+ expect(matching.length).toBeGreaterThan(0);
+ expect(matching[0].source).toBe("api");
+ expect(matching[0].preview).toContain("Authenticated feedback from the dashboard.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts` around
lines 48 - 57, The test is flaky because it matches conversations by a
non-unique subject and uses an untyped cast; update the authenticated test to
uniquely identify the conversation for this run and use a typed helper: when
creating the support conversation capture the POST response (use the
conversationId if returned) or otherwise use
backendContext.value.mailbox.emailAddress + randomUUID() in the
senderEmail/subject to guarantee uniqueness, then fetch /api/v1/conversations
and filter by that conversationId (or sort/filter by most recent lastActivityAt)
instead of find(c => c.subject === subject); also replace the ad-hoc (c: {
subject: string }) cast with a ConversationSummary typed predicate/helper so
fromSupportForm has proper types for .source and .preview.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
claude/CLAUDE-KNOWLEDGE.md (1)
224-225:⚠️ Potential issue | 🟠 MajorRemove misplaced/stale answers under the Cmd+K question.
Line 224 and Line 225 are unrelated to the Line 223 Cmd+K question, and Line 224 reintroduces outdated schema terms (
ConversationChannel,ConversationMetadata,status-change). Keep only the Cmd+K guidance here and move/delete these entries.Proposed fix
-Q: Where should new globally searchable Cmd+K destinations be added in the dashboard? -A: For the new Support app work, model support as generic conversations rather than support-specific threads: use `Conversation` for identity/status/source, `ConversationChannel` for adapter/entry-point expansion (`chat`, `email`, `api`, `manual`), `ConversationMessage` for message history (`message`, `internal-note`, `status-change`), and `ConversationMetadata` for assignment/tags/SLA timestamps. Keep the dashboard UI under `/projects/[projectId]/conversations` (legacy `/projects/[projectId]/support` redirects there), but point both internal admin routes and user-facing API routes at the generic `/api/latest/.../conversations` surface. -A: Support-thread contracts added during dashboard feature work are easiest to keep buildable by colocating them in the consuming app (`apps/dashboard/src/lib/*` and `apps/backend/src/lib/*`) unless the package build is already running and up to date. New files under `packages/stack-shared/src` are not automatically visible to app-local typechecks that import `@stackframe/stack-shared/dist/*` until the package dist has been regenerated. +Q: Where should new globally searchable Cmd+K destinations be added in the dashboard? A: Add project-level shortcuts to `PROJECT_SHORTCUTS` in `apps/dashboard/src/components/cmdk-commands.tsx` (optionally gated with `requiredApps`), and for app subpages rely on the flattened `appFrontend.navigationItems` command generation in the same file so pages are directly searchable without nested preview navigation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude/CLAUDE-KNOWLEDGE.md` around lines 224 - 225, Remove the misplaced/stale answer text that reintroduces outdated schema terms under the Cmd+K question: delete or move the paragraphs that mention Conversation, ConversationChannel, ConversationMessage, ConversationMetadata, and the "status-change" term so only the Cmd+K guidance remains; ensure no duplicate entries of that support-thread guidance remain and update any nearby references so they point to the canonical conversation schema elsewhere if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@claude/CLAUDE-KNOWLEDGE.md`:
- Around line 224-225: Remove the misplaced/stale answer text that reintroduces
outdated schema terms under the Cmd+K question: delete or move the paragraphs
that mention Conversation, ConversationChannel, ConversationMessage,
ConversationMetadata, and the "status-change" term so only the Cmd+K guidance
remains; ensure no duplicate entries of that support-thread guidance remain and
update any nearby references so they point to the canonical conversation schema
elsewhere if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff1eed91-ee3b-484e-aa32-9698b20ab684
📒 Files selected for processing (1)
claude/CLAUDE-KNOWLEDGE.md
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Made-with: Cursor
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
claude/CLAUDE-KNOWLEDGE.md (1)
228-229:⚠️ Potential issue | 🟠 MajorRemove stale table names in this answer to avoid contradictory guidance.
Line 228 reintroduces
ConversationChannelandConversationMetadata, which conflicts with Line 2 and the current schema. This should consistently useConversationEntryPointand keep assignment/SLA fields onConversation.Suggested doc fix
-A: For the new Support app work, model support as generic conversations rather than support-specific threads: use `Conversation` for identity/status/source, `ConversationChannel` for adapter/entry-point expansion (`chat`, `email`, `api`, `manual`), `ConversationMessage` for message history (`message`, `internal-note`, `status-change`), and `ConversationMetadata` for assignment/tags/SLA timestamps. Keep the dashboard UI under `/projects/[projectId]/conversations` (legacy `/projects/[projectId]/support` redirects there), but point both internal admin routes and user-facing API routes at the generic `/api/latest/.../conversations` surface. +A: For the new Support app work, model support as generic conversations rather than support-specific threads: use `Conversation` for identity/status/source/assignment/SLA fields, `ConversationEntryPoint` for adapter/entry-point expansion (`chat`, `email`, `api`, `manual`), and `ConversationMessage` for message history (`message`, `internal-note`, `status-change`). Keep the dashboard UI under `/projects/[projectId]/conversations` (legacy `/projects/[projectId]/support` redirects there), and point both internal admin routes and user-facing API routes at the generic `/api/latest/.../conversations` surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude/CLAUDE-KNOWLEDGE.md` around lines 228 - 229, The doc reintroduces stale table names—replace any occurrences of ConversationChannel and ConversationMetadata with the current schema names: use ConversationEntryPoint instead of ConversationChannel and remove ConversationMetadata references; ensure assignment and SLA fields remain defined on Conversation (not moved to a non-existent ConversationMetadata), and update any examples, route text, and type references that mention ConversationChannel/ConversationMetadata to use ConversationEntryPoint and Conversation respectively so the answer aligns with the current schema and prior lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@claude/CLAUDE-KNOWLEDGE.md`:
- Around line 228-229: The doc reintroduces stale table names—replace any
occurrences of ConversationChannel and ConversationMetadata with the current
schema names: use ConversationEntryPoint instead of ConversationChannel and
remove ConversationMetadata references; ensure assignment and SLA fields remain
defined on Conversation (not moved to a non-existent ConversationMetadata), and
update any examples, route text, and type references that mention
ConversationChannel/ConversationMetadata to use ConversationEntryPoint and
Conversation respectively so the answer aligns with the current schema and prior
lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ec9ce4d-b6f9-4b43-9194-780b7b54ce5f
📒 Files selected for processing (2)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsxclaude/CLAUDE-KNOWLEDGE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsx
…atch blocks from cited async handlers and removed the now-unused useToast usage.
… returns a paged object, and exposes truncation via hasMore internally/has_more publicly.
…urce, channelType, and adapterKey from that mapping. Also fixed conversation pagination/async error handling
…enderType_check. Added neg tests for con_priority_check, con_source_check, conEntryPoint_type_check, conMessage_messageType_check.
…ted to add max(5000) to patch reply
…SCAPE. updated to escape \, %, and _ before wrapping with %...%, and added ESCAPE \ to each LIKE.
…, status, priority, and source from parent conversation. Keeping response shape and added comments to clarify
…saveSla. Now import runAsyncWithAlert and wrapped both saveSla calls with it so failures surface through the shared alert path
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page-client.tsx (1)
932-936:⚠️ Potential issue | 🟠 MajorAvoid throwing during render when the owner team can't be resolved.
This still hard-crashes the whole page if
project.ownerTeamIdisnullor the team is temporarily missing fromuser.useTeams(). Please resolve the team tonull/undefined, render a fallback state, and restructure the dependentuseUsers()call so hook ordering stays stable.Based on learnings, the owner team lives in the internal project and should be resolved from
user.useTeams(), not the project-scoped admin app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/conversations/page-client.tsx around lines 932 - 936, Replace the throw-in-render with a safe null fallback and keep hook order stable: change currentOwnerTeam to return null when not found (e.g., const currentOwnerTeam = useMemo(() => userTeams.find(t => t.id === project.ownerTeamId) ?? null, [...]) ), derive a stable ownerTeamId (e.g., const ownerTeamId = currentOwnerTeam?.id ?? null) and then call the hook that reads users with that id rather than on the team object (e.g., const assignableTeamMembers = useUsers(ownerTeamId) or an equivalent user-scoped hook), and ensure ownerTeamId is resolved from user.useTeams() (userTeams) rather than the project-scoped source so missing teams render a fallback UI instead of throwing.
🧹 Nitpick comments (1)
apps/dashboard/src/lib/conversations.ts (1)
55-55: Validate API payloads instead of asserting them.These unchecked
response.json() as ...casts push contract mismatches deeper into the UI instead of failing at the fetch boundary. This file is the right place to parse the payload with the shared conversation schemas (or a single typed decoder helper) rather than relying on assertions.As per coding guidelines, "Do NOT use
as/any/type casts or anything else like that to bypass the type system. Most of the time a place where you would use type casts is not one where you actually need them."Also applies to: 65-65, 79-79, 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/conversations.ts` at line 55, Replace the unsafe casts like "await response.json() as ConversationListResponse" with runtime validation using the shared conversation schemas or a typed decoder helper: import the appropriate Zod/IO-TS schema for ConversationListResponse (and the other response types used at the other sites) and call schema.parse/validate on the parsed JSON (or use a decode helper) and throw/report a clear error if validation fails; update the functions that call response.json() (the spots returning ConversationListResponse and the other response types referenced in this file) to first await response.json(), then validate with the shared schema before returning the validated/typed object so mismatched payloads fail at the fetch boundary rather than via "as" casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/conversations/page-client.tsx:
- Around line 431-434: The early-return that checks trimmedBody and messageType
is dropping attachment-only messages; update the guard so it only returns null
when the message has no attachments (e.g., require
props.message.attachments?.length === 0 in the condition), and when a message
has attachments but an empty trimmedBody render an attachment placeholder/count
instead of returning null; locate the check using trimmedBody and
props.message.messageType and change it to allow messages with
props.message.attachments, then add a minimal placeholder renderer where the
empty-body path would have returned.
---
Duplicate comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/conversations/page-client.tsx:
- Around line 932-936: Replace the throw-in-render with a safe null fallback and
keep hook order stable: change currentOwnerTeam to return null when not found
(e.g., const currentOwnerTeam = useMemo(() => userTeams.find(t => t.id ===
project.ownerTeamId) ?? null, [...]) ), derive a stable ownerTeamId (e.g., const
ownerTeamId = currentOwnerTeam?.id ?? null) and then call the hook that reads
users with that id rather than on the team object (e.g., const
assignableTeamMembers = useUsers(ownerTeamId) or an equivalent user-scoped
hook), and ensure ownerTeamId is resolved from user.useTeams() (userTeams)
rather than the project-scoped source so missing teams render a fallback UI
instead of throwing.
---
Nitpick comments:
In `@apps/dashboard/src/lib/conversations.ts`:
- Line 55: Replace the unsafe casts like "await response.json() as
ConversationListResponse" with runtime validation using the shared conversation
schemas or a typed decoder helper: import the appropriate Zod/IO-TS schema for
ConversationListResponse (and the other response types used at the other sites)
and call schema.parse/validate on the parsed JSON (or use a decode helper) and
throw/report a clear error if validation fails; update the functions that call
response.json() (the spots returning ConversationListResponse and the other
response types referenced in this file) to first await response.json(), then
validate with the shared schema before returning the validated/typed object so
mismatched payloads fail at the fetch boundary rather than via "as" casts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d87d8c67-1375-4369-8b7d-f30282300879
📒 Files selected for processing (11)
apps/backend/prisma/migrations/20260420000000_add_conversations/tests/creates-conversation-tables.tsapps/backend/src/app/api/latest/conversations/[conversationId]/route.tsxapps/backend/src/app/api/latest/conversations/route.tsxapps/backend/src/app/api/latest/internal/conversations/route.tsxapps/backend/src/lib/conversations-api.tsapps/backend/src/lib/conversations.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/support/page.tsxapps/dashboard/src/lib/conversations.tsclaude/CLAUDE-KNOWLEDGE.mdpackages/stack-shared/src/interface/conversations.ts
✅ Files skipped from review due to trivial changes (2)
- apps/backend/src/app/api/latest/conversations/route.tsx
- claude/CLAUDE-KNOWLEDGE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/backend/src/app/api/latest/conversations/[conversationId]/route.tsx
- apps/backend/src/app/api/latest/internal/conversations/route.tsx
- packages/stack-shared/src/interface/conversations.ts
- apps/backend/prisma/migrations/20260420000000_add_conversations/tests/creates-conversation-tables.ts
- apps/backend/src/lib/conversations.tsx
Keep conversation history by clearing deleted user/team links instead of cascading deletion, and redact user-identifying sender fields during user deletion. Add migration and E2E coverage to verify conversations survive with PII removed
…mpty and there are no attachments. If the body is empty but attachments exist, it now renders a minimal placeholder like 1 attachment / 2 attachments
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page-client.tsx (1)
935-939:⚠️ Potential issue | 🟠 MajorDon't hard-crash the page when the owner team can't be resolved.
throwErr(...)runs during render here, so a nullproject.ownerTeamIdor a transientuseTeams()miss takes down the whole Conversations page before you can show any fallback. Please let this resolve tonull/undefinedand render a guarded state before callinguseUsers()off it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/conversations/page-client.tsx around lines 935 - 939, Replace the render-time crash by changing the useMemo that produces currentOwnerTeam to return undefined when no matching team is found (remove the throwErr call) so currentOwnerTeam can be nullish instead of throwing, and then guard the subsequent call to currentOwnerTeam.useUsers() (used to set assignableTeamMembers) so it only runs when currentOwnerTeam is non-null (e.g., compute assignableTeamMembers conditionally or use optional chaining/fallback empty array) to allow rendering a safe fallback state when the owner team is unresolved; update references: currentOwnerTeam, useMemo, userTeams, project.ownerTeamId, assignableTeamMembers, currentOwnerTeam.useUsers().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/backend/prisma/migrations/20260429000000_preserve_conversations_on_subject_delete/migration.sql`:
- Around line 1-13: The current migration drops and re-adds
Conversation_projectUser_fkey and Conversation_team_fkey inside one transaction
which will validate the whole Conversation table and cause long locks; instead,
add the replacement foreign keys on Conversation referencing ProjectUser and
Team using NOT VALID (preserving ON DELETE SET NULL and ON UPDATE CASCADE),
release the transaction, then create a follow-up migration that runs ALTER TABLE
"Conversation" VALIDATE CONSTRAINT for each new constraint and finally performs
the minimal cutover (drop old constraints if still present and ensure
names/semantics match) in a very small locked window; target symbols:
Conversation, Conversation_projectUser_fkey, Conversation_team_fkey,
ProjectUser, Team.
In `@apps/backend/src/app/api/latest/users/crud.tsx`:
- Around line 67-87: Remove the client-side expansion of conversation IDs and
perform the redaction in a single DB-side UPDATE using an EXISTS subquery;
instead of selecting conversationRows and building conversationIds, change the
tx.$executeRaw(Prisma.sql`...`) for updating "ConversationMessage" to include an
EXISTS clause that checks for a matching row in "Conversation" (e.g.,
EXISTS(SELECT 1 FROM "Conversation" c WHERE c.id =
"ConversationMessage"."conversationId" AND c."tenancyId" =
${options.tenancyId}::uuid AND c."projectUserId" =
${options.projectUserId}::uuid)), keep the other predicate ("senderType" =
'user' AND "senderId" = ${options.projectUserId}::uuid) and remove any
Prisma.join/array expansion (conversationRows/conversationIds and the prior
tx.$queryRaw call) so the entire operation runs server-side without building a
large IN list.
---
Duplicate comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/conversations/page-client.tsx:
- Around line 935-939: Replace the render-time crash by changing the useMemo
that produces currentOwnerTeam to return undefined when no matching team is
found (remove the throwErr call) so currentOwnerTeam can be nullish instead of
throwing, and then guard the subsequent call to currentOwnerTeam.useUsers()
(used to set assignableTeamMembers) so it only runs when currentOwnerTeam is
non-null (e.g., compute assignableTeamMembers conditionally or use optional
chaining/fallback empty array) to allow rendering a safe fallback state when the
owner team is unresolved; update references: currentOwnerTeam, useMemo,
userTeams, project.ownerTeamId, assignableTeamMembers,
currentOwnerTeam.useUsers().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a87ebcb-3c8d-4d45-a22d-64e507a004ff
📒 Files selected for processing (9)
apps/backend/prisma/migrations/20260429000000_preserve_conversations_on_subject_delete/migration.sqlapps/backend/prisma/migrations/20260429000000_preserve_conversations_on_subject_delete/tests/preserves-conversations-on-user-and-team-delete.tsapps/backend/prisma/schema.prismaapps/backend/src/app/api/latest/teams/crud.tsxapps/backend/src/app/api/latest/users/crud.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/conversations/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/support-settings/page-client.tsxapps/e2e/tests/backend/endpoints/api/v1/support.test.tsclaude/CLAUDE-KNOWLEDGE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- claude/CLAUDE-KNOWLEDGE.md
- apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/support-settings/page-client.tsx
Comment on lines
+1
to
+13
| ALTER TABLE "Conversation" DROP CONSTRAINT "Conversation_projectUser_fkey"; | ||
| ALTER TABLE "Conversation" ADD CONSTRAINT "Conversation_projectUser_fkey" | ||
| FOREIGN KEY ("tenancyId", "projectUserId") | ||
| REFERENCES "ProjectUser"("tenancyId", "projectUserId") | ||
| ON DELETE SET NULL ("projectUserId") | ||
| ON UPDATE CASCADE; | ||
|
|
||
| ALTER TABLE "Conversation" DROP CONSTRAINT "Conversation_team_fkey"; | ||
| ALTER TABLE "Conversation" ADD CONSTRAINT "Conversation_team_fkey" | ||
| FOREIGN KEY ("tenancyId", "teamId") | ||
| REFERENCES "Team"("tenancyId", "teamId") | ||
| ON DELETE SET NULL ("teamId") | ||
| ON UPDATE CASCADE; |
Contributor
There was a problem hiding this comment.
Recreating both FKs in one transactional migration can block Conversation.
Dropping and re-adding validated foreign keys here will take heavyweight locks and force validation work on a table we have to assume is very large. In production that makes this migration prone to timeouts and write blocking. Please switch this to an online pattern instead of a one-shot rebuild inside a single migration transaction (for example: add replacement constraints as NOT VALID, validate in a follow-up migration, then do the cutover in the smallest possible lock window).
As per coding guidelines: "When writing database migration files, assume that we have >1,000,000 rows in every table (unless otherwise specified)." and "Each database migration file is executed in a single transaction with a relatively short timeout. Split long-running operations into separate migration files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/backend/prisma/migrations/20260429000000_preserve_conversations_on_subject_delete/migration.sql`
around lines 1 - 13, The current migration drops and re-adds
Conversation_projectUser_fkey and Conversation_team_fkey inside one transaction
which will validate the whole Conversation table and cause long locks; instead,
add the replacement foreign keys on Conversation referencing ProjectUser and
Team using NOT VALID (preserving ON DELETE SET NULL and ON UPDATE CASCADE),
release the transaction, then create a follow-up migration that runs ALTER TABLE
"Conversation" VALIDATE CONSTRAINT for each new constraint and finally performs
the minimal cutover (drop old constraints if still present and ensure
names/semantics match) in a very small locked window; target symbols:
Conversation, Conversation_projectUser_fkey, Conversation_team_fkey,
ProjectUser, Team.
Comment on lines
+67
to
+87
| const conversationRows = await tx.$queryRaw<{ id: string }[]>(Prisma.sql` | ||
| SELECT id | ||
| FROM "Conversation" | ||
| WHERE "tenancyId" = ${options.tenancyId}::uuid | ||
| AND "projectUserId" = ${options.projectUserId}::uuid | ||
| `); | ||
| const conversationIds = conversationRows.map((row) => row.id); | ||
|
|
||
| await tx.$executeRaw(Prisma.sql` | ||
| UPDATE "ConversationMessage" | ||
| SET | ||
| "senderId" = NULL, | ||
| "senderDisplayName" = NULL, | ||
| "senderPrimaryEmail" = NULL | ||
| WHERE "tenancyId" = ${options.tenancyId}::uuid | ||
| AND "senderType" = 'user' | ||
| AND ( | ||
| "senderId" = ${options.projectUserId} | ||
| ${conversationIds.length > 0 ? Prisma.sql`OR "conversationId" IN (${Prisma.join(conversationIds.map((id) => Prisma.sql`${id}::uuid`))})` : Prisma.empty} | ||
| ) | ||
| `); |
Contributor
There was a problem hiding this comment.
Avoid expanding all matching conversation IDs into an IN (...) list.
For users with a large support history, this pulls every conversation ID into JS and then generates a very large SQL statement for the redaction update. That makes deletion slower and can hit query-size/parameter limits. Keep this as a single DB-side update with an EXISTS/join predicate instead.
Possible fix
- const conversationRows = await tx.$queryRaw<{ id: string }[]>(Prisma.sql`
- SELECT id
- FROM "Conversation"
- WHERE "tenancyId" = ${options.tenancyId}::uuid
- AND "projectUserId" = ${options.projectUserId}::uuid
- `);
- const conversationIds = conversationRows.map((row) => row.id);
-
await tx.$executeRaw(Prisma.sql`
- UPDATE "ConversationMessage"
+ UPDATE "ConversationMessage" AS cm
SET
"senderId" = NULL,
"senderDisplayName" = NULL,
"senderPrimaryEmail" = NULL
- WHERE "tenancyId" = ${options.tenancyId}::uuid
- AND "senderType" = 'user'
+ WHERE cm."tenancyId" = ${options.tenancyId}::uuid
+ AND cm."senderType" = 'user'
AND (
- "senderId" = ${options.projectUserId}
- ${conversationIds.length > 0 ? Prisma.sql`OR "conversationId" IN (${Prisma.join(conversationIds.map((id) => Prisma.sql`${id}::uuid`))})` : Prisma.empty}
+ cm."senderId" = ${options.projectUserId}
+ OR EXISTS (
+ SELECT 1
+ FROM "Conversation" AS c
+ WHERE c."tenancyId" = cm."tenancyId"
+ AND c."id" = cm."conversationId"
+ AND c."projectUserId" = ${options.projectUserId}::uuid
+ )
)
`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const conversationRows = await tx.$queryRaw<{ id: string }[]>(Prisma.sql` | |
| SELECT id | |
| FROM "Conversation" | |
| WHERE "tenancyId" = ${options.tenancyId}::uuid | |
| AND "projectUserId" = ${options.projectUserId}::uuid | |
| `); | |
| const conversationIds = conversationRows.map((row) => row.id); | |
| await tx.$executeRaw(Prisma.sql` | |
| UPDATE "ConversationMessage" | |
| SET | |
| "senderId" = NULL, | |
| "senderDisplayName" = NULL, | |
| "senderPrimaryEmail" = NULL | |
| WHERE "tenancyId" = ${options.tenancyId}::uuid | |
| AND "senderType" = 'user' | |
| AND ( | |
| "senderId" = ${options.projectUserId} | |
| ${conversationIds.length > 0 ? Prisma.sql`OR "conversationId" IN (${Prisma.join(conversationIds.map((id) => Prisma.sql`${id}::uuid`))})` : Prisma.empty} | |
| ) | |
| `); | |
| await tx.$executeRaw(Prisma.sql` | |
| UPDATE "ConversationMessage" AS cm | |
| SET | |
| "senderId" = NULL, | |
| "senderDisplayName" = NULL, | |
| "senderPrimaryEmail" = NULL | |
| WHERE cm."tenancyId" = ${options.tenancyId}::uuid | |
| AND cm."senderType" = 'user' | |
| AND ( | |
| cm."senderId" = ${options.projectUserId} | |
| OR EXISTS ( | |
| SELECT 1 | |
| FROM "Conversation" AS c | |
| WHERE c."tenancyId" = cm."tenancyId" | |
| AND c."id" = cm."conversationId" | |
| AND c."projectUserId" = ${options.projectUserId}::uuid | |
| ) | |
| ) | |
| `); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend/src/app/api/latest/users/crud.tsx` around lines 67 - 87, Remove
the client-side expansion of conversation IDs and perform the redaction in a
single DB-side UPDATE using an EXISTS subquery; instead of selecting
conversationRows and building conversationIds, change the
tx.$executeRaw(Prisma.sql`...`) for updating "ConversationMessage" to include an
EXISTS clause that checks for a matching row in "Conversation" (e.g.,
EXISTS(SELECT 1 FROM "Conversation" c WHERE c.id =
"ConversationMessage"."conversationId" AND c."tenancyId" =
${options.tenancyId}::uuid AND c."projectUserId" =
${options.projectUserId}::uuid)), keep the other predicate ("senderType" =
'user' AND "senderId" = ${options.projectUserId}::uuid) and remove any
Prisma.join/array expansion (conversationRows/conversationIds and the prior
tx.$queryRaw call) so the entire operation runs server-side without building a
large IN list.
N2D4
approved these changes
May 5, 2026
Comment on lines
+204
to
+211
| export const branchSupportSchema = yupObject({ | ||
| sla: yupObject({ | ||
| enabled: yupBoolean(), | ||
| firstResponseMinutes: yupNumber().integer().min(1).nullable(), | ||
| nextResponseMinutes: yupNumber().integer().min(1).nullable(), | ||
| }).optional(), | ||
| }).optional(); | ||
|
|
Contributor
There was a problem hiding this comment.
can we move all support endpoints into internal/dogfood/support/conversations? they should all be hidden for now since we're only dogfooding for ourselves
Comment on lines
+21
to
+34
| async function clearConversationsForDeletedTeam(tx: PrismaClientTransaction, options: { | ||
| tenancyId: string, | ||
| teamId: string, | ||
| }) { | ||
| await tx.$executeRaw(Prisma.sql` | ||
| UPDATE "Conversation" | ||
| SET | ||
| "teamId" = NULL, | ||
| "updatedAt" = NOW() | ||
| WHERE "tenancyId" = ${options.tenancyId}::uuid | ||
| AND "teamId" = ${options.teamId}::uuid | ||
| `); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
why don't we do this when we delete a team instead?
Comment on lines
+63
to
+87
| async function anonymizeConversationsForDeletedProjectUser(tx: PrismaClientTransaction, options: { | ||
| tenancyId: string, | ||
| projectUserId: string, | ||
| }) { | ||
| const conversationRows = await tx.$queryRaw<{ id: string }[]>(Prisma.sql` | ||
| SELECT id | ||
| FROM "Conversation" | ||
| WHERE "tenancyId" = ${options.tenancyId}::uuid | ||
| AND "projectUserId" = ${options.projectUserId}::uuid | ||
| `); | ||
| const conversationIds = conversationRows.map((row) => row.id); | ||
|
|
||
| await tx.$executeRaw(Prisma.sql` | ||
| UPDATE "ConversationMessage" | ||
| SET | ||
| "senderId" = NULL, | ||
| "senderDisplayName" = NULL, | ||
| "senderPrimaryEmail" = NULL | ||
| WHERE "tenancyId" = ${options.tenancyId}::uuid | ||
| AND "senderType" = 'user' | ||
| AND ( | ||
| "senderId" = ${options.projectUserId} | ||
| ${conversationIds.length > 0 ? Prisma.sql`OR "conversationId" IN (${Prisma.join(conversationIds.map((id) => Prisma.sql`${id}::uuid`))})` : Prisma.empty} | ||
| ) | ||
| `); |
Contributor
There was a problem hiding this comment.
as above, why not do this when we delete the user?
Contributor
There was a problem hiding this comment.
i think we should keep this info around for deleted users and teams actually. someone may submit a ticket for a deleted user account. we can still delete them later through some mechanisms
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
SLA
Data
Tests
Documentation