Skip to content

Conversation

chrisbobbe
Copy link
Collaborator

This PR aims to add all the data we'll need to show channel folders in the inbox, i.e.:

@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Oct 2, 2025
@chrisbobbe
Copy link
Collaborator Author

(Fixed tools/check build_runner.)

Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

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

Thanks @chrisbobbe. LGTM, moving over to Greg's review.

@rajveermalviya rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Oct 6, 2025
@rajveermalviya rajveermalviya requested a review from gnprice October 6, 2025 14:39
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Just small comments; otherwise all looks good.

Comment on lines 57 to +59
final UnreadMessagesSnapshot unreadMsgs;

final List<ChannelFolder>? channelFolders;
Copy link
Member

Choose a reason for hiding this comment

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

nit: docs have these in opposite order


final UnreadMessagesSnapshot unreadMsgs;

final List<ChannelFolder>? channelFolders;
Copy link
Member

Choose a reason for hiding this comment

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

TODO(server-11), right?


final UnreadMessagesSnapshot unreadMsgs;

final List<ChannelFolder>? channelFolders;
Copy link
Member

Choose a reason for hiding this comment

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

nit: commit-message prefixes, similar to #1869 (comment) ; the current revision has

63b5b83 initial_snapshot: Add channelFolders
36e7c96 model: Add ZulipStream.folderId
3da70bc api: Add channel_folder event
3ff64be store: Add channelFolders
84604e0 channel: Add ChannelStore.compareChannelFolders

and I think the first three should all say "api:".

Comment on lines +653 to +654
// TODO(server-11) delete TODO but keep optional, for channels not in folders
int? folderId;
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to not having a TODO, right? That seems simpler 🙂

Comment on lines 652 to 655
ChannelPostPolicy? channelPostPolicy; // TODO(server-10) remove
// TODO(server-11) delete TODO but keep optional, for channels not in folders
int? folderId;
// final bool isAnnouncementOnly; // deprecated for `channelPostPolicy`; ignore
Copy link
Member

Choose a reason for hiding this comment

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

nit: this stanza is all various kinds of permission settings; let's pull this field out separately, earlier.

(We already have this class's fields in a different order from the docs in order to be more logical, in particular grouping description with renderedDescription.)

Comment on lines +537 to +538
channelFolders[newChannelFolder.id] = newChannelFolder;
case ChannelFolderUpdateEvent():
Copy link
Member

Choose a reason for hiding this comment

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

nit: bit easier to read if each case is its own new stanza (since they're each nontrivial):

Suggested change
channelFolders[newChannelFolder.id] = newChannelFolder;
case ChannelFolderUpdateEvent():
channelFolders[newChannelFolder.id] = newChannelFolder;
case ChannelFolderUpdateEvent():

Comment on lines +73 to +75
final orderA = a.order;
final orderB = b.order;
return switch ((orderA, orderB)) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: since the switch already means these are consumed just once, can inline them for I think a bit more clarity:

Suggested change
final orderA = a.order;
final orderB = b.order;
return switch ((orderA, orderB)) {
return switch ((a.order, b.order)) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants