fix(profile): persist pending social links on save by rebelchris · Pull Request #6663 · dailydotdev/apps

rebelchris · 2026-09-15T14:12:54Z

Issue: https://linear.app/dailydev/issue/ENG-2011/feedback-ux-issue-link-to-github-in-profile-details-does-not-save-in

Summary:

Flush pending social link input before profile Save so pasted GitHub URLs are committed without requiring Add.
Never send socialLinks until the profile query has settled, and merge links added while it was in flight with the server's on arrival, so a save can no longer replace the user's existing links.
Render a loading placeholder or an explicit error in the links section instead of an empty list that reads as "no links".
Surface profile update failures through inline socialLinks errors, validation messages, or fallback toasts instead of invisible field errors.
Add focused RTL coverage plus a Playwright regression for the reported Save-without-Add flow.

Key decisions:

The committed socialLinks array remains the form source of truth; pending input is only flushed on submit.
Pending text is not committed on blur. The submit-time flush covers the ticket, while blur-commit turned abandoned text into a committed link (dirtying the form), double-toasted duplicates on Save, and needed a mousedown/mouseup handshake on Add that stayed stuck when the press was released elsewhere.
The URL field is type="text" with inputMode="url", not type="url". github.com/username fails native constraint validation, so a real submit event never fired; blur-commit had been masking this by clearing the input before the browser checked it. The settings page only got away with it because Save is a type="button" with an onClick.
Invalid pending text blocks Save and renders the existing field error UI.
Plain-string ValidationErrors are surfaced by their own message, distinguished by extensions.code, not by matching message text. Errors for fields absent from this settings page are routed to toasts, and internal errors keep the generic toast.

Also fixed, found while de-duplicating on review: useProfileForm (deprecated, but still used by registration and onboarding) ran JSON.parse on the raw error message unguarded, so it threw on exactly the plain-string ValidationErrors this PR handles in useUserInfoForm. ProfileFormHint and a null-returning parseProfileFormHint now live once, next to the mutations in graphql/users.ts, and both hooks consume them.

Closes ENG-2011

Created by Huginn 🐦‍⬛

Preview domain

https://eng-2011-feedback-ux-issue-link.preview.app.daily.dev

vercel · 2026-09-15T14:13:00Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
daily-webapp	Ready	Preview	Sep 17, 2026 10:24am UTC

rebelchris

Summary

The submit-time flush fixes the reported Save-without-Add flow, the untouched-before-init payload omission is correct against updateUserInfo (absent socialLinks falls back to the legacy merge; a present array replaces the whole set), and the error routing finally makes { github: 'github already exists' } visible. CI (shared tests, strict typecheck, lint) is green.

One blocking item: the dirty-before-init path still sends only the locally added links and replaces the user's existing links on the server (details inline). The rest is non-blocking: whether blur-commit is intended, some ref mirroring and helper duplication, and Playwright locator/duplication nits.

Verification

Read root/webapp/playwright AGENTS.md
Traced save/flush/blur/error paths and the API updateUserInfo handling of socialLinks
Consumers checked: SocialLinksInput and useUserInfoForm are only used by the settings profile page
CI inspected (all green); tests not re-run locally
Manual check on a real phone of the blur-commit behaviour (tap outside the input after pasting)

Reviewed by AI.

rebelchris · 2026-09-16T14:12:42Z

+      return;
+    }
+
+    if (!methods.getFieldState('socialLinks').isDirty) {


Blocking: this still clears existing links in the one scenario the PR says it is guarding against.

Scenario: a user who already has links opens /settings/profile on a slow connection (or the profile query fails outright, since fullProfile then never arrives and there is no error state). Boot does not carry socialLinks, so the form starts from []. They paste a new link before the query resolves (or after it failed), which marks the field dirty. This effect then skips the reset, and getProfileUpdatePayload sends socialLinks: [newLink] because the field is touched. On the API side if (data.socialLinks) routes a present array through processSocialLinksForDualWrite, which replaces the whole set, so every pre-existing link is deleted. The new spec keeps a social link added while the profile query is in flight asserts exactly this outcome ([pendingLink] without the server's link), so it locks the data loss in.

Suggested direction: either merge on arrival (when the field is dirty, setValue to server links plus the locally added ones that are not already present, keeping shouldDirty), or treat the links section as not editable until the profile query has settled (expose the query state from the hook and render a loading/error state in SocialLinksInput instead of an empty list that reads as "no links"). Either way the spec should assert the merged result.

Reviewed by AI.

Fixed in c4b9b75. Went belt-and-braces because the merge alone left the query-failure half of the scenario open:

socialLinks is now omitted from the payload whenever hasInitializedSocialLinks is false, not just when the field is untouched. If the query never resolves or fails, the array can no longer reach processSocialLinksForDualWrite at all.

When the query does land on a dirty field, the effect merges: server links first, then the locally added ones that are not already present (compared with the new isSameSocialLinkUrl, so case and trailing slash do not create a duplicate).

The links section now renders a placeholder while loading and an explicit error when the query failed, with the input and Add disabled, instead of an empty list that reads as "no links". The hook exposes isSocialLinksLoading / isSocialLinksError for that.

The spec that locked in the data loss now asserts the merged result, plus new cases for the no-duplicate merge, the never-resolves payload omission, and both query states.

rebelchris · 2026-09-16T14:12:42Z

        placeholder="Paste a URL (e.g., github.com/username)"
        value={url}
        onChange={handleUrlChange}
+        onBlur={() => {


Non-blocking (question): is committing on blur a product decision, or a means to the Save fix? The submit-time flushPendingUrl alone covers the ticket, and blur-commit has side effects the ticket did not ask for:

Text the user typed and abandoned becomes a committed link the moment they tap or click anywhere else, which dirties the form; leaving the page then triggers the DirtyForm modal / beforeunload prompt for a change they never meant to make.

With a duplicate URL pending, clicking Save toasts twice: once from the blur commit (Save's mousedown steals focus) and again from flushPendingUrl.

The skipBlurCommitRef mousedown/mouseup handshake on the Add button exists only to work around blur-commit, and it leaks: press on Add, release outside it, and the flag stays true until the next click, so the next blur commit is silently skipped.

If blur-commit stays, please say so in the description and drop the handshake in favour of something that cannot get stuck; if not, removing it also removes the Add button special-casing.

Reviewed by AI.

Dropped it, along with the Add-button handshake. The submit-time flush covers the ticket, and all three side effects you listed go with it.

Worth recording why this mattered more than it looked: blur-commit was load-bearing for the wrong reason. The field was type="url", and github.com/username fails native constraint validation, so a real submit event never fired at all — blur-commit happened to clear the input before the browser checked it. The settings page only got away with it because Save is a type="button" with an onClick. Removing blur-commit surfaced this immediately: every submit-path test went to zero calls. The field is type="text" with inputMode="url" now, which keeps the mobile keyboard and lets the real submit path work. The spec deliberately keeps a type="submit" button so a reintroduced type="url" fails again.

rebelchris · 2026-09-16T14:12:42Z

  });

  const [url, setUrl] = useState('');
+  const pendingUrlRef = useRef('');


Non-blocking: pendingUrlRef and linksRef mirror url and links, and linksRef.current = links is written during render. The imperative handle is rebuilt whenever commitPendingUrl changes, and React flushes state between the blur (mousedown) and the click (submit), so the closures are already fresh; commitPendingUrl can read url/links directly and the two refs and the extra linksRef.current = newLinks writes can go. Same file: normalizeSocialLinkUrl re-implements withHttps from lib/links.ts and is exported without an external consumer.

Reviewed by AI.

Done — pendingUrlRef, linksRef and the render-phase write are gone; commitPendingUrl reads url and links directly. With blur-commit removed there is no intra-event state flush to reason about either.

normalizeSocialLinkUrl moved to lib/socialLink.tsx next to the other helpers and is now built on withHttps. It has a real consumer there: the new isSameSocialLinkUrl, which both the duplicate check and the merge above use.

rebelchris · 2026-09-16T14:12:42Z

+      const data = parseProfileFormHint(errorMessage);
+
+      if (!data) {
+        displayToast('Failed to update profile');


Non-blocking: the API also throws plain-string ValidationErrors for this mutation, e.g. Invalid URL when a social link fails the blocked-words check. Those are not JSON, so they collapse into the generic Failed to update profile and the user has no cue that the link is the problem. Consider surfacing a non-JSON message when it is a GraphQL validation error (or mapping it to the socialLinks field), while keeping the generic toast for raw DB errors like the character varying(39) case in the spec.

Reviewed by AI.

Fixed. ValidationError from apollo-server-errors carries extensions.code = GRAPHQL_VALIDATION_FAILED, while the raw DB errors are rethrown as-is, so the two are cleanly separable. Non-JSON messages on a validation error are now shown verbatim; everything else keeps the generic toast, and the character varying(39) spec still covers that.

I stopped short of mapping Invalid URL onto the socialLinks field. Both throw sites are social-link validation today, but matching on an API message string from the frontend is the kind of coupling that rots silently. Added ApiError.GraphqlValidationFailed to the enum, and a spec for the validation-message path.

rebelchris · 2026-09-16T14:12:43Z

+  await page.goto('/settings/profile');
+  await expect(page.getByRole('textbox', { name: 'Add link' })).toBeVisible();
+
+  const linkRow = page.locator('div', { hasText: url }).filter({


Non-blocking: page.locator('div', { hasText: url }) matches every ancestor div containing the text, so .first() resolves to the outermost one (the page container). If the test account has more than one link, the nested getByRole('button', { name: 'Remove link' }) then matches several buttons and the cleanup fails on a strict-mode violation, leaving the e2e link on the production account (tests run against production by default per packages/playwright/AGENTS.md). Anchoring on the row (e.g. getByText(url).locator('..') up to the row, or a data-testid on the row) avoids that. Similarly, once the login modal is open getByRole('button', { name: 'Log in' }) can match both the opener and the form submit.

Also getRequiredEnv, the cookie banner and login steps duplicate login.spec.ts; moving them into tests/helpers.ts keeps one copy.

Reviewed by AI.

Both fixed. The link row carries data-testid="social-link-row", so cleanup is getByTestId("social-link-row").filter({ hasText: url }) — one row, no outermost-div match and no strict-mode violation with several links on the account.

getRequiredEnv, acceptCookieBanner and login moved into tests/helpers.ts; login.spec.ts imports getRequiredEnv from there. The login submit is now scoped to the form that contains the Password field, so it cannot match the header opener.

…-profile-details-does-not

Review follow-ups on the pending-social-link fix. The profile query, not boot, is what tells the form which links the server already holds. Editing links before it lands left the form holding only the locally added ones, and saving that array replaces the whole set server-side, so every pre-existing link was deleted. socialLinks is now omitted from the payload until the query settles, links added while it is in flight are merged with the server's on arrival, and the section renders a loading or error state instead of an empty list that reads as "no links". Blur no longer commits pending text. The submit-time flush already covers the reported Save-without-Add flow, while blur-commit turned abandoned text into a committed link, double-toasted duplicates on Save, and needed a mousedown / mouseup handshake on Add that stayed stuck when the press was released elsewhere. Blur-commit was also masking native constraint validation: type="url" holding "github.com/user" is invalid, so a real submit event never fired. The field is type="text" with inputMode="url" now, which keeps the mobile keyboard. Plain-string ValidationErrors (a blocked link URL) reached the user as the generic "Failed to update profile"; they are now surfaced by their own message, with the generic toast kept for internal errors. normalizeSocialLinkUrl moves next to the other social link helpers and reuses withHttps, and the two refs mirroring url/links are gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…-profile-details-does-not

rebelchris

Conventions-only second pass (component/hook patterns, test placement, e2e, styling, imports). Query keys, flag usage, token/Tailwind classes, co-located spec placement, source-file imports and the forwardRef/displayName pattern all match the repo. Three non-blocking convention notes inline. Reviewed by AI.

rebelchris · 2026-09-17T10:03:14Z

+const isRenderedProfileField = (key: string): key is keyof UserProfile =>
+  renderedProfileFields.has(key as keyof UserProfile);
+
+const parseProfileFormHint = (message?: string): ProfileFormHint | null => {


Non-blocking (duplication): ProfileFormHint now exists twice with diverging shapes (useProfileForm.ts keeps username?/name?, this file switches to an index signature), and parseProfileFormHint re-implements the JSON.parse(firstError.message) branch that useProfileForm.ts onError still runs unguarded, so the sibling hook keeps throwing on the plain-string Invalid URL validation error this PR fixes here. Could the type and parser move to one place (e.g. next to mutateUserInfo in graphql/users.ts) and both hooks consume it? Reviewed by AI.

Sorted in 03fd147. ProfileFormHint and a null-returning parseProfileFormHint now live next to the mutations in graphql/users.ts, and useProfileForm, useUserInfoForm and SocialRegistrationForm all consume them. The single type keeps the named username?/name? keys over a Record<string, string | undefined> index signature, so SocialRegistrationForm keeps the typing it had and the API-driven keys still fit.

You were right that the sibling hook was the live half of this: useProfileForm ran JSON.parse(firstError.message) unguarded, so the plain-string Invalid URL this PR handles here threw there instead. It goes through the shared parser now and returns early when the message is not a hint. Covered by a parseProfileFormHint spec in graphql/users.spec.ts — the field-keyed hint, the plain-string validation error, the raw DB error, and the array/bare-value cases.

rebelchris · 2026-09-17T10:03:14Z

 }

+const renderedProfileFields = new Set<keyof UserProfile>([
+  'bio',


Non-blocking: renderedProfileFields hardcodes in a shared hook which fields the webapp SettingsLayout/Profile page happens to render. Adding or removing a field on that page silently changes whether its API error lands inline or in a toast, and nothing ties the two together. The form already knows its fields: key in methods.getValues() (or methods.getFieldState(key)) gives the same answer without a hand-maintained list. Reviewed by AI.

Gone. Routing is now key in methods.getValues(), so the form answers for itself and the page can add or remove a field without silently changing where its error lands.

Behaviour is unchanged for everything the API actually keys hints by: username still resolves inline, the legacy social columns and email are absent from the form values and still go to a toast. The list only ever differed from the form values by image/cover, which the API does not key hints by.

rebelchris · 2026-09-17T10:03:14Z

    enabled: !!userId,
  });

+  // Boot omits socialLinks, so until the profile query lands the form has no


Non-blocking (house style): AGENTS.md asks that the reasoning behind a fix go in the commit message rather than a comment above the code, and to match the surrounding comment density. This file gains three multi-line "why" blocks (here, above the merge in the effect, and in onError); the PR description already carries this reasoning. Reviewed by AI.

Fixed — all three are gone from useUserInfoForm. I also trimmed the two JSDoc blocks I had added in graphql/users.ts down to single-line comments, since that file otherwise carries only two // lines and they were the same mistake one file over. The reasoning is in the commit messages and the PR description.

rebelchris · 2026-09-17T10:03:14Z

+  "Run the mutating profile regression once"
+);
+
+test("persists a pasted GitHub link when saving without clicking Add", async ({


Non-blocking (e2e conventions): per packages/playwright/AGENTS.md this suite runs against live production on every main deploy with the shared CI account, and this is the first spec in the package that mutates that account. Cleanup is best-effort: if the Save or the post-save redirect fails, the finally block reloads /settings/profile and returns when the row is absent, so a partially saved link with a unique Date.now() handle survives to the next run and the account accumulates rows. Worth either scoping the cleanup to also handle a link that was saved but not redirected, or noting the trade-off in the spec header. Separately, helpers.ts was reformatted from single to double quotes in this diff; the package has no Prettier config so both styles coexist, but the churn is unrelated to the fix. Reviewed by AI.

Both addressed, and the first one was worse than best-effort — it was a gap I introduced. The links section now stays disabled until the profile query settles, so removeSocialLink could run count() against a still-loading list, see zero rows and return, leaving the link on the account. Cleanup now goes through a shared openProfileSettings that waits for the Add link input to be enabled before it trusts the list, which covers the saved-but-not-redirected case you describe. The residual risk (a hard failure mid-cleanup) is written into the spec header.

The quote churn was mine: I ran Prettier over these files without --config packages/prettier-config/index.js, and since the playwright package has no prettier key in its package.json it fell back to the double-quote default and rewrote extractRootDomain. Reformatted with the shared config; the helpers.ts diff is purely additive again.

Review follow-ups on conventions and duplication. ProfileFormHint existed twice with diverging shapes, and useProfileForm still ran JSON.parse on the raw error message, so the plain-string ValidationErrors handled in useUserInfoForm threw there instead. The type and a null-returning parser now live next to the mutations in graphql/users.ts, and both hooks plus SocialRegistrationForm consume them. renderedProfileFields hardcoded in a shared hook which fields the webapp settings page happens to render, so adding a field there silently moved its API error between inline and toast. The form already knows its own fields. Dropped the three why-comments this branch added to useUserInfoForm and trimmed the new ones in graphql/users.ts to the density around them; the reasoning is in the commit messages and the PR description. The e2e cleanup could not see a saved link while the links section was still loading, which would leak a dailydev-e2e row onto the shared account, so it waits for the section to settle first and the trade-off is written down. Also reverted the unrelated quote churn in playwright/tests/helpers.ts, from running Prettier without the package's config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rebelchris

Re-review of 03fd147 only (the shared ProfileFormHint/parseProfileFormHint move and the key in getValues() routing). Both round-2 items in useUserInfoForm.ts are verified resolved, and useProfileForm no longer throws out of onError on a plain-string message, which is a bonus fix. One small regression noted inline; nothing blocking. CircleCI for this head was still running at review time. Reviewed by AI.

rebelchris · 2026-09-17T10:36:11Z

+/**
+ * This is the only spec in the package that writes to the shared CI account,
+ * and it runs against live production. Cleanup is best effort: if Save itself
+ * fails the link was never stored, and if it stored but the redirect did not
+ * happen the row is still removed below. A hard failure mid-cleanup leaves one
+ * `dailydev-e2e-*` link behind for a maintainer to delete.
+ */


Non-blocking: this commit removes the multi-line why-comments from useUserInfoForm.ts but adds a seven-line block of the same kind here (why cleanup is best effort, what happens on each failure mode). Per AGENTS.md the reasoning belongs in the commit message / PR description, which already carry it. The two // lines inside openProfileSettings are enough; suggest dropping this block or cutting it to one line naming the fact ("Only spec that writes to the shared CI account; cleanup is best effort").

Reviewed by AI.

fix(profile): persist pending social links on save

2c5d3c0

vercel Bot deployed to Preview September 15, 2026 14:16 View deployment

rebelchris commented Sep 16, 2026

View reviewed changes

Merge branch 'main' into eng-2011-feedback-ux-issue-link-to-github-in…

2756eea

…-profile-details-does-not

vercel Bot deployed to Preview September 17, 2026 07:01 View deployment

Merge branch 'main' into eng-2011-feedback-ux-issue-link-to-github-in…

6004bf4

…-profile-details-does-not

vercel Bot deployed to Preview September 17, 2026 09:13 View deployment

vercel Bot deployed to Preview September 17, 2026 09:41 View deployment

Merge branch 'main' into eng-2011-feedback-ux-issue-link-to-github-in…

3001676

…-profile-details-does-not

vercel Bot deployed to Preview September 17, 2026 09:51 View deployment

rebelchris commented Sep 17, 2026

View reviewed changes

vercel Bot deployed to Preview September 17, 2026 10:24 View deployment

rebelchris commented Sep 17, 2026

View reviewed changes

rebelchris merged commit 3859b65 into main Sep 17, 2026
21 checks passed

rebelchris deleted the eng-2011-feedback-ux-issue-link-to-github-in-profile-details-does-not branch September 17, 2026 10:41

Uh oh!

Conversation

rebelchris commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preview domain

Uh oh!

vercel Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebelchris left a comment

Choose a reason for hiding this comment

Summary

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebelchris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebelchris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

rebelchris commented Sep 15, 2026 •

edited

Loading

vercel Bot commented Sep 15, 2026 •

edited

Loading