[web] Fixed thread name clearing issue in `ThreadSettingGeneralTab`
ClosedPublic
Actions

Authored by • abosh on May 16 2022, 4:51 PM.

Details

Reviewers

atul

Commits

rCOMMf2d68fec126d: [web] Fixed thread name clearing issue in `ThreadSettingGeneralTab`

Summary

Fixed issue where the thread name cleared when setting it to anything with the same prefix. The bug was with the firstLine function in onChangeName.

firstLine in onChangeName was introduced in D863 and D862 to make it impossible to set a thread title with newlines, this fix still ensures that.

And for good measure, this also helps resolve the issue in D3980, where the ThreadSettingsGeneralTab button doesn't stay disabled if there are no queued changes.

Related Linear issue with full context here.

Test Plan

Tested on Chrome/Safari and the issue is gone.

Before:

After:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• abosh created this revision.May 16 2022, 4:51 PM

Herald added subscribers: • adrian, tomek, ashoat. · View Herald TranscriptMay 16 2022, 4:51 PM

• abosh edited the summary of this revision. (Show Details)May 16 2022, 5:00 PM

• abosh edited the test plan for this revision. (Show Details)

• abosh added 1 blocking reviewer(s): atul.

Harbormaster returned this revision to the author for changes because remote builds failed.May 16 2022, 5:03 PM

Harbormaster failed remote builds in B9188: Diff 12779!

• abosh added inline comments.May 16 2022, 5:03 PM

web/modals/threads/settings/thread-settings-general-tab.react.js
66 ↗	(On Diff #12779)	The `firstLine` function returns the empty string when passed `undefined` as an argument, which made `onChangeName` set `queuedChanges.name` to the empty string, not `undefined` (which would signify that no change should be made). Because `queuedChanges.name` was set to `''`, a change was queued, and the thread name would "clear" (be set to the empty string) if the current thread name was equal to the updated thread name.
66 ↗	(On Diff #12779)	This fix still ensures that there are no newlines in the thread name because it passes `target.value` into `firstLine`, but if the new thread name is equal to the current thread name, it sets `queuedChanges.name` to `undefined`, unlike before when `queuedChanges.name` was set to `''`.

Harbormaster completed remote builds in B9188: Diff 12779.May 17 2022, 7:35 AM

• abosh requested review of this revision.May 17 2022, 7:35 AM

• abosh edited the summary of this revision. (Show Details)May 18 2022, 10:18 PM

Gotcha, so the issue is that when newName is the same as threadInfo.name the ternary "returns" undefined

... and undefined gets passed to the firstLine function
... and firstLine returns the empty string when undefined is passed in
... and so the value of the "Thread name` Input gets set to the empty string
... and results in the Input "clearing" for the user

So the goal was to not queue a change when the input is the same as the existing value, but the outcome was clearing the thread name when it matches the existing one. And the solution was to pull undefined "outside" of the call to firstLine

Good catch

This revision is now accepted and ready to land.May 19 2022, 6:55 AM

Closed by commit rCOMMf2d68fec126d: [web] Fixed thread name clearing issue in `ThreadSettingGeneralTab` (authored by • abosh). · Explain WhyMay 19 2022, 5:21 PM

This revision was automatically updated to reflect the committed changes.

• abosh added a commit: rCOMMf2d68fec126d: [web] Fixed thread name clearing issue in `ThreadSettingGeneralTab`.