member flow hardening#60
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
Pull request overview
Hardens signed-in listing and chat flows by standardizing inline mutation handling, improving route-level loading/error boundaries, and restoring Turbopack/Next Yak build compatibility while tightening Playwright production execution.
Changes:
- Introduces
useInlineMutationand extracts typed controllers for listing write + chat flows to unify pending/success/failure behavior. - Adds route
loading.tsx/error.tsxboundaries for listing create/edit and chat routes. - Fixes Turbopack/Next Yak regression by moving the shared Yak theme entrypoint to a
.jsmodule; updates e2e/docs/Playwright prod server behavior accordingly.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/listing.ts | Adds typed shapes for listing write inputs/results and field errors. |
| src/types/chat.ts | Tightens chat message types and adds normalized ChatThreadView / action results. |
| src/styles/theme.yak.js | Moves Yak theme entrypoint to JS for Turbopack/Next Yak compatibility. |
| src/hooks/useInlineMutation.ts | Adds shared inline mutation hook for consistent pending/result state. |
| src/data/demo/threads.ts | Updates demo thread/message fixtures to match stricter chat message typing. |
| src/components/RouteBoundaryState/RouteBoundaryState.tsx | Adds shared loading/error boundary UI component. |
| src/components/ListingWrite/listingWriteController.ts | Extracts listing write validation/draft building/submit/delete logic. |
| src/components/ListingWrite/ListingWrite.tsx | Refactors listing write UI to use controllers + inline mutations + disabled state handling. |
| src/components/ChatWindow/chatWindowController.ts | Extracts chat thread/message load/send/read operations into typed controller helpers. |
| src/components/ChatWindow/ChatWindow.tsx | Refactors chat UI to use controller + inline mutation flow; improves pending/failure behavior. |
| src/components/ChatPageClient/ChatPageClient.tsx | Switches selected thread prop to normalized ChatThreadView. |
| src/components/ButtonToDialog/ButtonToDialog.tsx | Adds disabled/pending support to prevent duplicate submits and improve UX feedback. |
| src/app/actions.ts | Hardens listing create/update/delete actions to return InlineActionResult<T> and adds path revalidation. |
| src/app/(forms)/profile/listings/new/[type]/loading.tsx | Adds loading boundary for new listing flow. |
| src/app/(forms)/profile/listings/new/[type]/error.tsx | Adds error boundary with retry for new listing flow. |
| src/app/(forms)/profile/listings/[slug]/loading.tsx | Adds loading boundary for edit listing flow. |
| src/app/(forms)/profile/listings/[slug]/error.tsx | Adds error boundary with retry for edit listing flow. |
| src/app/(core)/(interact)/(stretched)/chats/[[...threadId]]/page.tsx | Normalizes selected thread into ChatThreadView. |
| src/app/(core)/(interact)/(stretched)/chats/[[...threadId]]/loading.tsx | Adds chat route loading boundary. |
| src/app/(core)/(interact)/(stretched)/chats/[[...threadId]]/error.tsx | Adds chat route error boundary with retry. |
| playwright.prod.config.ts | Forces prod Playwright to start its own next start server. |
| messages/pt-BR.json | Adds Actions.tryAgain translation. |
| messages/fr.json | Adds Actions.tryAgain translation. |
| messages/es.json | Adds Actions.tryAgain translation. |
| messages/en.json | Adds Actions.tryAgain translation. |
| messages/de.json | Adds Actions.tryAgain translation. |
| e2e/listings.spec.ts | Extends listing edit e2e coverage for pending/disabled/aria-busy behavior. |
| e2e/helpers.ts | Adds helpers to delay/fail server-action and chat send requests for deterministic e2e assertions. |
| e2e/chat.spec.ts | Adds chat send pending/success/failure coverage (disable composer, preserve draft, show inline feedback). |
| docs/supabase-local-first.md | Documents Playwright expectations for local seeded Supabase + anon key. |
| README.md | Adds clearer local Playwright + Supabase seeded stack setup and explains prod server behavior. |
Comments suppressed due to low confidence (1)
src/styles/theme.yak.js:126
- Renaming the theme entrypoint to JS fixes Turbopack evaluation, but this module is no longer typechecked (JS files aren’t included in
tsconfig.json), sothemewill effectively beanyin TS consumers. To avoid losing type safety across the codebase, consider adding asrc/styles/theme.yak.d.ts(or a typed TS wrapper that doesn’t get evaluated by next-yak) so imports of@/styles/theme.yakretain a concrete type shape.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (createError || !newThread?.id) { | ||
| return { | ||
| success: false, | ||
| error: createError?.message ?? "Unable to create chat thread.", | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| error: null, | ||
| data: { | ||
| threadId: newThread.id, |
There was a problem hiding this comment.
ensureChatThread uses upsert(..., { ignoreDuplicates: true }) and then treats a missing newThread.id as a hard failure. If another request creates the same thread between the preceding select and this upsert, PostgREST can ignore the duplicate without returning a row (no error, but data is null), causing an unnecessary "Unable to create chat thread" error. Consider removing ignoreDuplicates, or if the upsert returns no row, perform a follow-up select('id') for the conflict keys to fetch the existing thread id.
| if (createError || !newThread?.id) { | |
| return { | |
| success: false, | |
| error: createError?.message ?? "Unable to create chat thread.", | |
| }; | |
| } | |
| return { | |
| success: true, | |
| error: null, | |
| data: { | |
| threadId: newThread.id, | |
| if (createError) { | |
| return { | |
| success: false, | |
| error: createError.message, | |
| }; | |
| } | |
| let threadId = newThread?.id ?? null; | |
| if (!threadId) { | |
| const { data: existingThread, error: existingThreadError } = await supabase | |
| .from("chat_threads") | |
| .select("id") | |
| .match({ | |
| listing_id: listing.id, | |
| initiator_id: userId, | |
| owner_id: listing.owner_id, | |
| }) | |
| .maybeSingle(); | |
| if (existingThreadError || !existingThread?.id) { | |
| return { | |
| success: false, | |
| error: | |
| existingThreadError?.message ?? "Unable to create chat thread.", | |
| }; | |
| } | |
| threadId = existingThread.id; | |
| } | |
| return { | |
| success: true, | |
| error: null, | |
| data: { | |
| threadId, |
| export function getThreadMessages( | ||
| existingThread?: { | ||
| chat_messages?: ChatMessageRecord[] | null; | ||
| chat_messages_with_senders?: ChatMessageRecord[] | null; | ||
| } | null | ||
| ) { | ||
| return ( | ||
| existingThread?.chat_messages_with_senders ?? | ||
| existingThread?.chat_messages ?? | ||
| [] | ||
| ); |
There was a problem hiding this comment.
ChatThreadView introduces a normalized messages array, but getThreadMessages only reads chat_messages_with_senders / chat_messages and ignores a precomputed messages field. This leaves the new ChatThreadView.messages unused and makes the "normalized" shape easy to misuse. Consider updating getThreadMessages to prefer existingThread.messages when present (and/or narrowing the input type to reflect the normalized shape).
| if (listingType === "residential") { | ||
| if (name !== profile?.first_name) { | ||
| const validation = validateName(name); | ||
|
|
||
| if (!validation.isValid) { | ||
| errors.name = validation.error; | ||
| } else { | ||
| validatedName = validation.value ?? ""; | ||
| } | ||
| } |
There was a problem hiding this comment.
validateListingWriteForm uses validateName for residential listings, but residential name is actually the user's first name. This is weaker than the server-side updateFirstNameAction validation (which uses validateFirstName) and can lead to submit-time failures that only appear as a generic/global error. Aligning client validation with validateFirstName (and translating its error code, similar to translateFirstNameFieldError in src/app/actions.ts) would keep errors inline and consistent with the server.
| return { | ||
| success: false, | ||
| error: String(result.error), |
There was a problem hiding this comment.
When updateFirstNameAction fails, submitListingWrite returns the error as a global InlineActionResult.error. In the previous flow, first-name update failures were surfaced as a field error on the name input, which is more actionable. Consider returning structured field-level errors for this case (or handling the updateFirstNameAction call in the component so it can set errors.name) so the first-name field is highlighted instead of only showing a generic form message.
| return { | |
| success: false, | |
| error: String(result.error), | |
| const nameError = String(result.error); | |
| const errors: ListingWriteFieldErrors = { | |
| name: nameError, | |
| }; | |
| return { | |
| success: false, | |
| error: nameError, | |
| data: { | |
| errors, | |
| } as ListingSubmitResult, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const run = useCallback( | ||
| async ( | ||
| mutation: () => Promise<InlineActionResult<T>>, | ||
| options: InlineMutationOptions = {} | ||
| ) => { | ||
| if (isPending) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
useInlineMutation uses React state (isPending) as the in-flight guard. Because state updates are async, two rapid invocations in the same tick (e.g. double-click submit / send) can both see isPending === false and run concurrently, potentially creating duplicate listings/messages. Consider tracking pending with a useRef that is set synchronously at the start of run (and cleared in finally), and use that ref for the early-return guard (state can still be used for rendering).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setErrors({}); | ||
| deleteMutation.reset(); | ||
|
|
||
| const validation = validateListingWriteForm({ | ||
| coordinates, | ||
| listingType, | ||
| name, | ||
| profile, | ||
| t, | ||
| }); | ||
|
|
||
| if (Object.keys(validation.errors).length > 0) { | ||
| setErrors(validation.errors); | ||
| return; |
There was a problem hiding this comment.
handleSubmit clears errors and resets deleteMutation, but it doesn’t reset submitMutation before returning on client-side validation errors. If a previous submit attempt set submitMutation.result.error, that stale error will keep taking precedence in feedbackMessage even when the current attempt fails validation. Reset submitMutation (or clear its error) at the start of handleSubmit and/or immediately before returning due to validation errors so the UI reflects the current attempt.
| const { data: newThread, error: createError } = await supabase | ||
| .from("chat_threads") | ||
| .upsert( | ||
| { | ||
| listing_id: listing.id, | ||
| initiator_id: userId, | ||
| owner_id: listing.owner_id, | ||
| }, | ||
| { | ||
| onConflict: "listing_id,initiator_id,owner_id", | ||
| } | ||
| ) | ||
| .select("id") | ||
| .maybeSingle(); |
There was a problem hiding this comment.
ensureChatThread uses .upsert(..., { onConflict: ... }) without ignoreDuplicates: true. Because chat_threads has an INSERT policy but no UPDATE policy, a conflict can cause the upsert to attempt an UPDATE and fail under RLS in race conditions (two clients creating the same thread). Use ignoreDuplicates: true (and keep the existing fallback select when no row is returned) to avoid UPDATE-on-conflict and make thread creation resilient.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ( | ||
| select count(*) >= 2 | ||
| from public.listings as owner_listings | ||
| where owner_listings.owner_id = chat_threads.owner_id | ||
| and owner_listings.type in ('community', 'business') | ||
| ) as owner_has_multiple_non_residential_listings, |
There was a problem hiding this comment.
owner_has_multiple_non_residential_listings currently uses a correlated count(*) >= 2, which will scan all matching listings for each thread row. For large owners this is avoidable; consider rewriting this as an EXISTS against the second matching row (e.g., SELECT 1 ... LIMIT 1 OFFSET 1) so Postgres can stop early and use an index on (owner_id, type) more effectively.
| where chat_messages.thread_id = chat_threads.id | ||
| order by chat_messages.created_at desc | ||
| limit 1 |
There was a problem hiding this comment.
The lateral join picks the latest message by order by chat_messages.created_at desc only. If two messages share the same created_at value, the chosen row is non-deterministic, which can make thread previews flicker. Consider adding a stable tie-breaker (for example ordering by created_at desc, id desc).
| const { data: unreadMessages, error: unreadMessagesError } = | ||
| previewThreadIds.length > 0 | ||
| ? await supabase | ||
| .from("chat_messages") | ||
| .select("thread_id") | ||
| .in("thread_id", previewThreadIds) | ||
| .neq("sender_id", user.id) | ||
| .is("read_at", null) | ||
| : { data: [], error: null }; |
There was a problem hiding this comment.
The unread-thread detection query loads every unread chat_messages row for the thread list and then dedupes in JS. If a user has many unread messages, this can become a large payload. Consider switching to a server-side distinct thread_id result (e.g., a view/RPC that returns unread thread IDs for the current user), or otherwise aggregating on the database side so only unique thread IDs are transferred.
|
|
||
| if ( | ||
| request.method() === "POST" && | ||
| request.headers()["next-action"] !== undefined | ||
| ) { | ||
| await page.waitForTimeout(delayMs); | ||
| } | ||
|
|
There was a problem hiding this comment.
delayServerActionRequests installs a page.route("**/*") handler, which intercepts every request (including assets) for the whole test. This adds overhead and can interfere with other route stubs in the same test. Consider scoping the route to only the pages under test (e.g. the relevant pathname patterns) and/or using route.fallback() for non-matching requests so only server-action POSTs pay the interception cost.
| if ( | |
| request.method() === "POST" && | |
| request.headers()["next-action"] !== undefined | |
| ) { | |
| await page.waitForTimeout(delayMs); | |
| } | |
| const isServerActionRequest = | |
| request.method() === "POST" && | |
| request.headers()["next-action"] !== undefined; | |
| if (!isServerActionRequest) { | |
| await route.fallback(); | |
| return; | |
| } | |
| await page.waitForTimeout(delayMs); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 36 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| where owner_listings.owner_id = chat_threads.owner_id | ||
| and owner_listings.type in ('community', 'business') | ||
| offset 1 | ||
| limit 1 | ||
| ) as owner_has_multiple_non_residential_listings, |
There was a problem hiding this comment.
In Postgres, LIMIT must come before OFFSET (i.e. LIMIT 1 OFFSET 1). As written (OFFSET 1 then LIMIT 1) this subquery is likely to fail to parse, which would break the migration and the chat_threads_with_participants view creation.
Suggested fix: reorder to limit 1 offset 1, or switch back to a count(*) >= 2 pattern if you prefer clarity over the micro-optimisation.
| create or replace view public.chat_threads_with_participants | ||
| with (security_invoker = on) as | ||
| select | ||
| chat_threads.id, | ||
| chat_threads.created_at, |
There was a problem hiding this comment.
This PR adds two migrations that both create or replace view public.chat_threads_with_participants (this one, and 20260424091500_optimize...). Since they’re introduced together, it’s better to squash into a single migration so the schema history doesn’t contain an intermediate view definition (and to keep the associated index creation in the final migration).
Summary
What Changed
ListingWriteandChatWindow, plus a shareduseInlineMutationhookInlineActionResult<T>with one pending/message pathloading.tsxanderror.tsxboundaries for listing create/edit and chat routessrc/styles/theme.yak.tstosrc/styles/theme.yak.jsso Next Yak can resolve the shared theme during Turbopack buildsnext startserver instead of reusing an existing local dev serverWhy
The next roadmap slice was to harden the member flows with the biggest real-user impact: listing write/edit and chat. While validating that work, Turbopack builds were failing because Next Yak was trying to evaluate the shared
theme.yak.tsfile directly in Node and choking on the TypeScript extension. The local prod Playwright path also turned out to be too easy to run against a hosted.env.localor a straynext devserver, which produced misleading failures.Impact
Validation
npm run typechecknpm run checknpm run buildnpm run test:e2e:prodwas not rerun end-to-end in this checkout because the current.env.localpoints at hosted Supabase and I stopped short of pulling the full local Supabase image set in this session