feat: add public share function for chats - #2227
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds DB-backed chat sharing: create/copy/delete public share links, public read-only retrieval, and fork shared snapshots into new chats; includes API, migration, Drizzle relations, frontend hooks, components, and a public share page. ChangesChat Sharing Feature
Sequence Diagram(s)sequenceDiagram
participant Owner as Chat Owner
participant OwnerUI as Playground App
participant API as Backend API
participant ShareDB as Database
participant PublicUI as Shared Page
participant PublicClient as Public Viewer
Owner->>OwnerUI: Click share icon
OwnerUI->>API: POST /chats/{id}/share
API->>ShareDB: Insert chat_share snapshot
ShareDB-->>API: share id
API-->>OwnerUI: share url
OwnerUI->>OwnerUI: Copy link
PublicClient->>PublicUI: Visit shared link
PublicUI->>API: GET /public/chats/share/{shareId}
API->>ShareDB: Query chat_share (not deleted)
ShareDB-->>API: messages jsonb + metadata
API-->>PublicUI: { id, messages[] }
PublicUI->>PublicUI: Render read-only view
PublicClient->>PublicUI: Click fork button
PublicUI->>API: POST /share/{shareId}/fork
API->>ShareDB: Read & parse messages
API->>API: Create new chat + insert messages
API-->>PublicUI: new chat id
PublicUI->>PublicClient: Navigate to new chat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/playground/src/app/share/[shareId]/page.tsx (1)
55-64: 💤 Low value
notFound()swallows 5xx and network errors as 404 — consider distinguishing.A flaky upstream (
5xx, timeouts) currently renders the same "not found" page as a missing share, which masks real outages from monitoring and from the user. For non-404 responses, throwing (so Next's error boundary kicks in) is more accurate.- if (!response.ok) { - notFound(); - } + if (response.status === 404) { + notFound(); + } + if (!response.ok) { + throw new Error(`Failed to load shared chat (${response.status})`); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/playground/src/app/share/`[shareId]/page.tsx around lines 55 - 64, The current fetch of `${config.apiBackendUrl}/public/chats/share/${shareId}` treats any non-ok response the same by calling notFound(), which hides 5xx and network errors; change the logic so only a 404 calls notFound(), and for other non-OK statuses or fetch/network failures re-throw an Error (or let the exception bubble) so Next's error boundary handles real outages—update the fetch/response handling around the fetch invocation and the response.ok check (and any surrounding try/catch) to distinguish response.status === 404 vs other statuses and to propagate errors for 5xx/timeouts instead of calling notFound().apps/api/src/routes/chats.ts (1)
706-706: 💤 Low valueSurface a clean 4xx instead of a 500 if a stored snapshot fails schema validation.
sharedMessageSnapshotSchema.parse(share.messages)will throw a rawZodErroron legacy/malformed snapshots, which Hono's global handler turns into a 500. Either usesafeParseand return a 400/410 with a clear message, or relax the parser to be tolerant (z.array(...).catch([])) so older snapshots remain forkable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/chats.ts` at line 706, sharedMessageSnapshotSchema.parse(share.messages) can throw a ZodError and surface as a 500; replace this with a safeParse check on share.messages (using sharedMessageSnapshotSchema.safeParse) and handle the failure by returning a clear 4xx response (e.g., 400 for malformed or 410 for stale/legacy snapshot) with an explanatory message, leaving the success branch to assign messages from parseResult.data; alternatively, if you prefer backward compatibility, relax sharedMessageSnapshotSchema to tolerate missing/legacy shapes (e.g., make the array schema catch([])) so parse no longer throws—apply the change where messages is assigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/chats.ts`:
- Around line 661-738: The forkSharedChat handler currently inserts the new chat
(db.insert into tables.chat) and then inserts messages in a separate statement,
which can leave an orphan chat if the message insert fails; wrap both inserts in
a single database transaction (use the DB client's transaction API) so the chat
and its messages are committed atomically: begin a transaction, perform the
insert into tables.chat to get newChat, insert the related messages into
tables.message (using newChat.id), then commit; if any step fails roll back the
transaction and surface the error (so the chatCount check and FREE_LIMIT_REACHED
logic remain correct).
In `@apps/playground/src/app/share/`[shareId]/page.tsx:
- Around line 99-155: The toUiMessage function uses untyped casts—replace parts:
any[] with parts: UIMessage["parts"], change parsedTools =
JSON.parse(message.tools) as any[] to parse and validate into the exact part
union type (e.g., const parsedTools = JSON.parse(message.tools) as unknown and
then assert/validate it is UIMessage["parts"] or an array of the discriminated
union before pushing), and remove the final "as UIMessage" cast by returning the
object using TypeScript's "satisfies UIMessage" to preserve exact types; update
references in toUiMessage (parts, parsedTools, and the return value) accordingly
and keep the same runtime shape checks for images/tools parsing to avoid unsafe
any casts.
In `@apps/playground/src/components/playground/fork-chat-button.tsx`:
- Around line 46-58: The effect in the component can re-fire after back/refresh
because didAutoForkRef resets on remount; before calling fork() (the mutation)
remove the fork=1 query param from the URL so subsequent mounts won't see
searchParams.get("fork") === "1". Update the import from next/navigation to
include usePathname, derive the current pathname via usePathname(), construct a
new URL/searchParams without the fork key (or push the pathname alone) and
update the history (replace or push) to strip fork=1, then set
didAutoForkRef.current = true and call fork(); keep checks in the useEffect
(isLoading, user, didAutoForkRef) as-is to avoid double-triggering.
---
Nitpick comments:
In `@apps/api/src/routes/chats.ts`:
- Line 706: sharedMessageSnapshotSchema.parse(share.messages) can throw a
ZodError and surface as a 500; replace this with a safeParse check on
share.messages (using sharedMessageSnapshotSchema.safeParse) and handle the
failure by returning a clear 4xx response (e.g., 400 for malformed or 410 for
stale/legacy snapshot) with an explanatory message, leaving the success branch
to assign messages from parseResult.data; alternatively, if you prefer backward
compatibility, relax sharedMessageSnapshotSchema to tolerate missing/legacy
shapes (e.g., make the array schema catch([])) so parse no longer throws—apply
the change where messages is assigned.
In `@apps/playground/src/app/share/`[shareId]/page.tsx:
- Around line 55-64: The current fetch of
`${config.apiBackendUrl}/public/chats/share/${shareId}` treats any non-ok
response the same by calling notFound(), which hides 5xx and network errors;
change the logic so only a 404 calls notFound(), and for other non-OK statuses
or fetch/network failures re-throw an Error (or let the exception bubble) so
Next's error boundary handles real outages—update the fetch/response handling
around the fetch invocation and the response.ok check (and any surrounding
try/catch) to distinguish response.status === 404 vs other statuses and to
propagate errors for 5xx/timeouts instead of calling notFound().
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b53ade1-59ea-4183-9d4c-a29ad14366f6
⛔ Files ignored due to path filters (1)
apps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (15)
apps/api/src/index.tsapps/api/src/routes/chats.tsapps/api/src/routes/public-chat-shares.tsapps/playground/src/app/share/[shareId]/page.tsxapps/playground/src/components/playground/chat-header.tsxapps/playground/src/components/playground/chat-page-client.tsxapps/playground/src/components/playground/chat-ui.tsxapps/playground/src/components/playground/fork-chat-button.tsxapps/playground/src/components/playground/share-chat-dialog.tsxapps/playground/src/hooks/useChats.tspackages/db/migrations/1778305250_broad_hammerhead.sqlpackages/db/migrations/meta/1778305250_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/relations.tspackages/db/src/schema.ts
Summary by CodeRabbit
New Features