Skip to content

ref(settings): Switch OwnerInput to useMutation with fetchMutation#111087

Closed
JoshuaKGoldberg wants to merge 4 commits intomasterfrom
JoshuaKGoldberg/ref/owner-input-use-mutation
Closed

ref(settings): Switch OwnerInput to useMutation with fetchMutation#111087
JoshuaKGoldberg wants to merge 4 commits intomasterfrom
JoshuaKGoldberg/ref/owner-input-use-mutation

Conversation

@JoshuaKGoldberg
Copy link
Copy Markdown
Member

@JoshuaKGoldberg JoshuaKGoldberg commented Mar 19, 2026

Switch OwnerInput from manual new Client() + api.requestPromise to useMutation/fetchMutation, and derive hasChanges as a computed variable instead of independent boolean state.

Follow-up cleanup from #110200 based on review feedback. The useMutation hook replaces the manual .then()/.catch() chain with declarative onSuccess/onError callbacks. hasChanges is now derived by comparing the current text against mutation.data?.raw (the last successfully saved value), falling back to initialText.

Fixes ENG-7011

Made with Cursor

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 19, 2026
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the JoshuaKGoldberg/ref/owner-input-use-mutation branch from 78371b1 to fa75cda Compare March 25, 2026 16:54
Replace manual Client + requestPromise with useMutation/fetchMutation
hook pattern. Derive hasChanges from mutation state instead of tracking
it as independent boolean state, comparing current text against the
last successfully saved value from mutation.data.

Refs LINEAR-ENG-7011
Co-Authored-By: Claude Sonnet 4 <noreply@example.com>

Made-with: Cursor
@linear-code
Copy link
Copy Markdown

linear-code bot commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

},
});

const hasChanges = (text ?? initialText) !== (mutation.data?.raw ?? initialText);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derived hasChanges breaks retry after failed save

Medium Severity

In TanStack Query v5, mutation.data is reset to undefined when mutate() is called again. This means if a user previously saved successfully, then edits text back to match initialText, and attempts to save again, mutation.data?.raw ?? initialText falls back to initialText — which equals the current text — so hasChanges becomes false. If that save fails, the user is stuck: the Save button is disabled and they cannot retry. The old code unconditionally set hasChanges = false only on success and true on any edit, avoiding this issue entirely.

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TkDodo I would normally solve this by setting a piece of state like "hasChanges" again, which is the opposite of #110200 (comment). Do you have advice?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasChanges is only used to disable the button if the text is “the same”. Our UX guidelines usually don’t want us to disable buttons at all, our form system also always allows submission.

What you can do is:

  • use our new form system here (useScrapsForm) instead of local state with text. I know it’s just one field but it’s still a form.
  • compare against initialText only. This is the data that comes from the API response, so the flow is like this:
    • query delivers initialText
    • user changes it
    • submits, makes a mutation
    • mutation invalidates the query or updates the cache
      • one problem I see is that doesn’t happen in all instances. Like here, we don’t implement onSave so the query never gets updated
    • the parent component that has the query re-renders
    • this component receives a new initialText, that now has the latest value you want to compare against

});
},
onError: (caught: RequestError) => {
setError(caught.responseJSON as InputError);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this? why not just use the error state returned from the mutation:

const { error } = useMutation(...)

that state re-sets automatically if the mutation is fired again.

return request;
const handleUpdateOwnership = () => {
setError(null);
mutation.mutate();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we’d probably want to pass the text here so that it’s available as variables in the mutation. feels a bit better than closing over text

},
});

const hasChanges = (text ?? initialText) !== (mutation.data?.raw ?? initialText);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasChanges is only used to disable the button if the text is “the same”. Our UX guidelines usually don’t want us to disable buttons at all, our form system also always allows submission.

What you can do is:

  • use our new form system here (useScrapsForm) instead of local state with text. I know it’s just one field but it’s still a form.
  • compare against initialText only. This is the data that comes from the API response, so the flow is like this:
    • query delivers initialText
    • user changes it
    • submits, makes a mutation
    • mutation invalidates the query or updates the cache
      • one problem I see is that doesn’t happen in all instances. Like here, we don’t implement onSave so the query never gets updated
    • the parent component that has the query re-renders
    • this component receives a new initialText, that now has the latest value you want to compare against

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft April 8, 2026 13:39
@JoshuaKGoldberg
Copy link
Copy Markdown
Member Author

👋 @TkDodo I've got a bunch of logs work on my plate and don't have the time to continue this. The changes are here if you/yours want them. Thanks for the advice, it was really helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants