fix(profile): stop work experience skills failing to save in silence - #6645
Conversation
Saving an experience with too many skills, or one skill over the length cap, was rejected by the API and the user was told nothing: the client schema had no skills field, so nothing was caught before the request, and ProfileSkills never rendered its error, so the rejection landed nowhere. The save is a single transaction, so the whole section was lost, and the still-dirty form then offered "Save changes" on the way out, which failed just as silently until the user discarded their work. Three layers, none of which alone is enough: - The input enforces the limits it knows about. Skills are deduped on the API's slugify() identity, a paste is capped rather than dropped whole, and whatever did not fit is named in a toast. - Rejections are visible. ProfileSkills renders the error, and a zod error always raises a toast as well, since fields like companyId have nowhere to show one. ProfileSkills reads it through useFormState: useController subscribes to its exact name, so an issue on `skills.3` never reached the Controller at all. - A failed save no longer costs the user the section. The dirty-form path validates first and awaits the mutation, so DirtyFormModal stays open until it settles and the form is never reset on failure. Client validation that blocks the save says so too, otherwise the modal just closes and we are back to the original bug. useDirtyForm's onSave may now return a promise; sync callers keep the previous fire-and-forget close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Traced the three paths (input-time enforcement in ProfileSkills, onError toast + applyZodErrorsToForm, and the awaited dirty-form save) against useDirtyForm, DirtyFormModal, edit.tsx and the RHF subscription model. The fix is sound: useFormState({ name }) is non-exact so skills.N item errors reach the component, getValues() + trigger() correctly avoids the resolver stripping employmentType/grade, and the sync-vs-promise branch in DirtyFormModal keeps useUserInfoForm behaviour unchanged. The client limits mirror the server contract in daily-api#4264 (paths ['skills'] and ['skills', N]), and both shapes are covered by tests.
No blocking findings. Three non-blocking notes inline, plus one cross-cutting one:
Comment density (non-blocking). Root AGENTS.md asks that code match the surrounding comment density and that the reasoning behind a fix live in the commit message/PR description rather than above the code. This diff adds roughly ten rationale blocks (useController exact subscription, getValues() vs resolver output, sparse-array error shape, the skip-list justification, etc.) whose content already sits in the PR description. Worth trimming to the one or two that a future reader genuinely cannot recover from the code (the useFormState subscription note is the strongest candidate to keep).
Verification
- Read root/shared
AGENTS.md, PR description and companion API PR context - Traced state/error/navigation flow for submit, dirty-save success, dirty-save server rejection and dirty-save client-invalid
- Checked both
useDirtyFormconsumers - Did not run tests/typecheck locally (relying on the author's reported suites and CI)
Reviewed by AI.
| overLimit === 1 ? 'skill was' : 'skills were' | ||
| } not added.`, | ||
| ); | ||
| } else if (tooLong) { |
There was a problem hiding this comment.
Non-blocking: because this is an else if, a single paste that both overflows the 50-skill cap and contains over-length entries only reports the over-limit count; the tooLong rejections are dropped silently, which is the exact class of failure this PR is fixing. Either fold both counts into one toast, or show the tooLong toast independently of overLimit.
Reviewed by AI.
| // means introducing a form-values type and settling where the page's | ||
| // serialized `startedAt` string becomes a Date — a refactor across the | ||
| // hook, the edit page and every experience form, not this bug fix. | ||
| 'packages/shared/src/hooks/useUserExperienceForm.spec.tsx', |
There was a problem hiding this comment.
Non-blocking: adding a spec to the strict skip list means future edits to it also bypass the strict gate. Since the mismatch is confined to fixtures and setValue calls in the spec, a local form-values alias in the spec (e.g. type ExperienceFormValues = Omit<UserExperience, 'startedAt' | 'endedAt'> & { startedAt: Date; endedAt?: Date; current?: boolean; skills?: string[] } with a cast at the useForm/fixture boundary) would keep the file under strict checking without the wider refactor you describe. Fine to leave as a follow-up if you prefer, but please open the issue so the entry does not become permanent.
Reviewed by AI.
| } catch { | ||
| // The caller owns surfacing the failure; the modal closes either way so | ||
| // the user lands back on their still-unsaved form. | ||
| } finally { |
There was a problem hiding this comment.
Non-blocking question: while isSaving is true only the two buttons are disabled; the modal itself can still be dismissed via Escape/overlay through the default onRequestClose. If that happens mid-save, this finally later calls closeModal() on whatever lazy modal is current at that point (possibly a different one the user opened in the meantime), and setIsSaving fires on an unmounted component. Consider either blocking onRequestClose while saving or tracking mount state before calling closeModal() in finally. Low likelihood given the short window, so a judgment call.
Reviewed by AI.
- A paste that both overflows the cap and carries over-length entries reported only the first reason and dropped the rest of the count silently, which is the failure this PR exists to remove. One toast now names every reason and the true number left out. - DirtyFormModal could be dismissed by Escape or the overlay mid-save, so the save would later close whichever modal was current by then. Both are inert while a save is in flight. - Types the form values instead of skip-listing the spec. The form was declared as UserExperience while holding values that type does not describe, so every fixture and setValue call in the spec mismatched. UserExperienceFormValues says what the form actually holds, including dates that arrive from the page serialized and become Dates once the month/year selects write to them. The GraphQL shape stays at the mutation boundary, where the cast already lived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
ENG-1886. Pairs with daily-api#4264 (readable messages for the same limits), but neither blocks the other — this PR is safe to ship alone.
The bug
Two defects compound into what was reported.
The limit was server-only and its error was invisible. The API caps skills at 50 per experience and 100 characters each. The client schema had no
skillsfield at all, so nothing was caught before the request. On rejection the API returnsZOD_VALIDATION_ERRORwith a generic message plusextensions.issues;useUserExperienceForm.onErrorrouted those toapplyZodErrorsToFormand skipped the toast, whileProfileSkillsnever read its error. The issue landed nowhere and the user saw nothing at all.The dirty-form flow then made it destructive. The save is one transaction, so a rejected skills array means nothing is saved. The form stays dirty, and on navigation
DirtyFormModaloffers "Save changes" — which calledonSave()and closed immediately without awaiting the mutation, so a second silent failure looked like success. The next attempt reopens the modal, and "Discard" throws the whole section away.The fix
1. Prevent it client-side.
skillsadded to the client schema, mirroring the server contract, with the limits as named constants.ProfileSkillsenforces them at input time: a paste is capped rather than dropped whole and the remainder is named in a toast, over-length entries are rejected with copy, and skills are deduped case-insensitively to match the server'sslugify()identity (today "React" and "react" round-trip as two rows). Helper copy at the limit, no counter, per AGENTS.md.2. Make rejections impossible to miss.
ProfileSkillsrenders its error, and zod errors always raise a toast too — the safety net for any path with no error-rendering field (companyId,externalLocationId, …), which is why this doesn't need a field-by-field audit. The trade-off: a field that does render its error now shows both inline text and a toast.3. Stop it costing the user their edits. The dirty-form save validates first and awaits the mutation, so the modal stays open and pending until it settles. On failure the modal closes back to the still-dirty form with the error visible and navigation aborted; the form is never
reset(). Client validation that blocks the save toasts as well — otherwise the modal just closes with nothing saved and we are back to the original bug.Two things worth reviewing
useControllersubscribes withexact: true. Handling the sparse-array shape of an item-level error is not enough:setError('skills.3')never re-renders a Controller namedskillsat all. Verified both ways before fixing;ProfileSkillsreads errors throughuseFormState, whose non-exact subscription catches the array-level and item-level paths.The dirty-save path keeps
getValues(). Switching it tohandleSubmitlooks equivalent but silently dropsemploymentType,locationType,externalLocationIdandgrade— the client schema is a subset of the form, so zod strips them. It validates withtrigger()instead.useDirtyForm'sonSavemay now return a promise. The only other consumer,useUserInfoForm, stays synchronous and keeps the previous fire-and-forget close.Tests
ProfileSkills.spec.tsx— adding past the limit blocked with visible copy; an over-limit paste capped and reported; case-insensitive duplicate not added twice; array-level and item-level (skills.3) errors each renderuseUserExperienceForm.spec.tsx— aZOD_VALIDATION_ERRORon skills produces both a form error and a toast, with no reset and no navigation; non-zod errors keep existing behavior; an invalid form never reaches the mutation; a failing save resolves only once it settles, values intactDirtyFormModal.spec.tsx(new) — stays open until an async save settles, closes immediately for a sync one, closes after a rejection so the form and its error stay visibleFull suites: shared 3042/3044, webapp 691/692. Both failures (
numberFormat.spec.ts,WorldGuideSheet.spec.tsx) are the same pre-existing locale issue on unmodified files (1.234vs1,234), unrelated to this change.E2E deliberately skipped:
packages/playwrightruns against production with a shared real account, and an experience-editing spec would mutate that account's real profile.Noted, not done
useUserExperienceForm.spec.tsxis added to the strict-typecheck skip list. Its errors are one pre-existing cause — the form is typeduseForm<UserExperience>but holds values that type doesn't describe (Daterather than string dates,current,skillsasstring[]). Fixing it means introducing a form-values type and settling where the page's serializedstartedAtstring becomes aDate, across the hook, the edit page and every experience form.user_experience_skillhas no unique index on(slugify(value), experienceId), so case-variant duplicates are only prevented client-side. A durable fix needs a dedupe/backfill migration — worth a follow-up issue.🤖 Generated with Claude Code
Preview domain
https://fix-work-experience-skills-silen.preview.app.daily.dev