Paths

Table of Contentst

Differential D13287

Update input type of useChangeThreadSettings
ClosedPublic
Actions

Authored by marcin on Sep 11 2024, 2:42 AM.

Tags

None

Referenced Files

	Unknown Object (File)
	Tue, Oct 22, 5:34 AM

	Unknown Object (File)
	Tue, Oct 22, 5:33 AM

	Unknown Object (File)
	Tue, Oct 22, 5:33 AM

	Unknown Object (File)
	Mon, Oct 14, 2:56 PM

	Unknown Object (File)
	Mon, Oct 14, 2:56 PM

	Unknown Object (File)
	Mon, Oct 14, 2:28 PM

	Unknown Object (File)
	Sep 26 2024, 12:13 AM

	Unknown Object (File)
	Sep 22 2024, 9:05 PM

View All 32 Files

Subscribers

Details

Reviewers

tomek
kamil

Commits

rCOMM3909f7edd16f: Update input type of useChangeThreadSettings

Summary

This differential updates input type of useChangeThreadSettings to accept union of thin and thick thread specific data.

Test Plan

test that changing various thread settings works for both thin and thick threads and results in the right notif.

Diff Detail

Repository

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Sep 11 2024, 2:42 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 11 2024, 2:42 AM

marcin added a parent revision: D13260: Implement joining DM thread.Sep 11 2024, 2:44 AM

Harbormaster completed remote builds in B31526: Diff 44031.Sep 11 2024, 2:59 AM

marcin requested review of this revision.Sep 11 2024, 2:59 AM

marcin added a child revision: D13289: Update input type of useUpdateSubscription.Sep 11 2024, 4:10 AM

tomek accepted this revision.Sep 12 2024, 12:34 AM

tomek added inline comments.

native/chat/settings/thread-settings-description.react.js
267 ↗	(On Diff #44031)	Why is it always false?

This revision is now accepted and ready to land.Sep 12 2024, 12:34 AM

kamil accepted this revision.Sep 12 2024, 1:53 AM

kamil added inline comments.

lib/actions/thread-actions.js
124–128 ↗	(On Diff #44031)	wondering, if this could be improved even more - for thick threads we're not able to modify thread members here (it's done via different spec), so we could remove the `newMemberIDs` field from `UpdateThreadRequest`
native/chat/settings/thread-settings-description.react.js
267 ↗	(On Diff #44031)	it shouldn't - it's possbile for DMs to modify description, but it's probably bug from the previous code
native/chat/settings/thread-settings-name.react.js
12–16 ↗	(On Diff #44031)	can be merged (applies to other files too)

Address review

Made significant changes to address feedback regarding adding members to thick threads.

Harbormaster completed remote builds in B31555: Diff 44072.Sep 12 2024, 4:41 AM

ashoat added inline comments.Sep 12 2024, 7:54 AM

lib/actions/thread-actions.js
126 ↗	(On Diff #44072)	Typo here

tomek accepted this revision.Sep 12 2024, 8:04 AM

tomek added inline comments.

web/modals/threads/settings/thread-settings-general-tab.react.js
23–24 ↗	(On Diff #44072)	Why are we replacing this type by `ThickThreadChanges`? Are we still supporting thin threads? The new type is narrower, so it looks safe, but is confusing.

This revision is now accepted and ready to land.Sep 12 2024, 8:04 AM

Rebase before landing

This revision was landed with ongoing or failed builds.Sep 12 2024, 10:23 AM

Closed by commit rCOMM3909f7edd16f: Update input type of useChangeThreadSettings (authored by marcin). · Explain Why

This revision was automatically updated to reflect the committed changes.

marcin added a commit: rCOMM3909f7edd16f: Update input type of useChangeThreadSettings.

Harbormaster completed remote builds in B31571: Diff 44099.Sep 12 2024, 10:24 AM

ashoat added inline comments.Sep 12 2024, 12:33 PM

web/modals/threads/settings/thread-settings-general-tab.react.js
23–24 ↗	(On Diff #44072)	This comment was not responded to

marcin added inline comments.Sep 12 2024, 12:35 PM

web/modals/threads/settings/thread-settings-general-tab.react.js
23–24 ↗	(On Diff #44072)	I am sorry for not documenting my answer here. However necessary changes were applied. `ThickThreadChanges` was renamed to `ThreadChanges` and `ThreadChanges` was renamed to `ThinThreadChanges` and used only in places where new members are added to thin thread.

ashoat added inline comments.Sep 12 2024, 12:37 PM

web/modals/threads/settings/thread-settings-general-tab.react.js
23–24 ↗	(On Diff #44072)	Thanks for explaining!

Revision Contents
Changeset List

Path

Size

lib/

actions/

thread-actions.js

35 lines

components/

base-edit-thread-avatar-provider.react.js

13 lines

hooks/

promote-sidebar.react.js

1 line

shared/

messages/

text-message-spec.js

25 lines

types/

thread-types.js

18 lines

native/

chat/

settings/

add-users-modal.react.js

1 line

color-selector-modal.react.js

35 lines

thread-settings-description.react.js

27 lines

thread-settings-name.react.js

27 lines

web/

modals/

threads/

members/

add-members-modal.react.js

1 line

settings/

thread-settings-utils.js

22 lines

Diff 44099

lib/actions/thread-actions.js

Loading...

lib/components/base-edit-thread-avatar-provider.react.js

Loading...

lib/hooks/promote-sidebar.react.js

Loading...

lib/shared/messages/text-message-spec.js

Loading...

lib/types/thread-types.js

Loading...

native/chat/settings/add-users-modal.react.js

Loading...

native/chat/settings/color-selector-modal.react.js

Loading...

native/chat/settings/thread-settings-description.react.js

Loading...

native/chat/settings/thread-settings-name.react.js

Loading...

web/modals/threads/members/add-members-modal.react.js

Loading...

web/modals/threads/settings/thread-settings-utils.js

Loading...