modernise form submit typing and polish listing edit#61
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
Note
Copilot was unable to run its full agentic suite in this review.
This PR modernizes form submit typing across the UI while polishing listing edit behavior/performance and aligning avatar editing responsibilities with the profile page.
Changes:
- Introduced shared submit-event types and updated multiple form submit handlers to use them.
- Optimized listing edit page data fetching with narrower selects and more parallel server work.
- Removed residential avatar editing from listing forms and added/updated E2E coverage around avatar surfaces and form layout.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/events.ts | Adds shared submit-event typings used across form surfaces. |
| src/components/SignUpForm/SignUpForm.tsx | Uses shared submit typing and memoizes Turnstile props to reduce churn. |
| src/components/ListingWrite/ListingWrite.tsx | Updates submit typing, removes residential avatar editing, and improves state updates + submit button layout. |
| src/components/ListingTypeChooser.tsx | Updates submit handler typing to shared submit event type. |
| src/components/ChatWindow/ChatWindow.tsx | Updates submit typing and improves read-message lookup performance. |
| src/components/ChatComposer/ChatComposer.tsx | Updates onSubmit prop type to shared submit handler. |
| src/components/ButtonToDialog/ButtonToDialog.tsx | Updates onSubmit typing and refines form attribute typing. |
| src/components/AvatarUploadView/AvatarUploadView.tsx | Adds a test id for avatar upload fieldset keyed by bucket. |
| src/app/(forms)/profile/listings/[slug]/page.tsx | Parallelizes server work, narrows selects, and adds explicit error handling. |
| e2e/profile.spec.ts | Adds assertion for profile avatar upload presence. |
| e2e/listings.spec.ts | Adds layout assertion for full-width submit and a new residential avatar-surface test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { SyntheticEvent } from "react"; | ||
|
|
||
| export type FormSubmitEvent = SyntheticEvent<HTMLFormElement, SubmitEvent>; |
There was a problem hiding this comment.
FormSubmitEvent is defined as SyntheticEvent<..., SubmitEvent>, but React's submit callbacks are typed as React.FormEvent<HTMLFormElement> (whose nativeEvent is generally Event in the type system). This makes the shared type narrower than what React actually promises, and can create friction when passing handlers into APIs expecting FormEventHandler<HTMLFormElement>. Consider basing the alias on React.FormEvent<HTMLFormElement> and (if you need submitter) augment it via intersection (e.g., add a nativeEvent refinement) or narrow at the usage site.
| import type { SyntheticEvent } from "react"; | |
| export type FormSubmitEvent = SyntheticEvent<HTMLFormElement, SubmitEvent>; | |
| import type { FormEvent } from "react"; | |
| export type FormSubmitEvent = FormEvent<HTMLFormElement>; |
| const t = await getTranslations(); | ||
| const { slug } = await params; | ||
| const supabase = await createClient(); | ||
| const [{ slug }, supabase] = await Promise.all([params, createClient()]); |
There was a problem hiding this comment.
Including params in Promise.all adds indirection without actually improving parallelism (since params is already a plain object in Next route handlers). A clearer approach is to synchronously read slug from params, and keep Promise.all for the genuinely async work (e.g., translations + queries).
| throw new Error(profileError.message); | ||
| } | ||
|
|
||
| if (listingError) { | ||
| throw new Error(listingError.message); |
There was a problem hiding this comment.
These thrown errors lose important context (which query failed and for which listing/user), making server-side debugging harder. Consider throwing an error message that includes contextual identifiers (e.g., slug) and which fetch failed, while still attaching/including the original Supabase error message.
| throw new Error(profileError.message); | |
| } | |
| if (listingError) { | |
| throw new Error(listingError.message); | |
| throw new Error( | |
| `Failed to fetch profile for user "${user.id}": ${profileError.message}` | |
| ); | |
| } | |
| if (listingError) { | |
| throw new Error( | |
| `Failed to fetch listing for slug "${slug}" and user "${user.id}": ${listingError.message}` | |
| ); |
| const formWidth = await page | ||
| .getByTestId("listing-write-form") | ||
| .evaluate((form) => form.getBoundingClientRect().width); |
There was a problem hiding this comment.
This pixel-precision width assertion is likely to be brittle across browser rendering differences (fonts, device scale factor, subpixel rounding). If the intent is to validate width=\"full\", a more stable assertion is to check a semantic signal (e.g., a class/attribute applied for full-width) or a layout relationship less sensitive to rounding (e.g., left/right edges align within a looser tolerance).
| const submitWidth = await submitButton.evaluate( | ||
| (button) => button.getBoundingClientRect().width | ||
| ); | ||
| expect(Math.abs(submitWidth - formWidth)).toBeLessThanOrEqual(2); |
There was a problem hiding this comment.
This pixel-precision width assertion is likely to be brittle across browser rendering differences (fonts, device scale factor, subpixel rounding). If the intent is to validate width=\"full\", a more stable assertion is to check a semantic signal (e.g., a class/attribute applied for full-width) or a layout relationship less sensitive to rounding (e.g., left/right edges align within a looser tolerance).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Validation
Notes