Skip to content

Conversation

@Cairo09
Copy link

@Cairo09 Cairo09 commented Jan 23, 2026

Fixes: #1074

Changes made with reasoning:

  1. Made visibilityPolicy nullable at deserialization events.dart (UserTopicEvent) and initial_snapshot.dart - Did the same for UserTopicItem.visibilityPolicy so maps missing/unknown value to null
    2.example_data.dart- made UserTopicVisibility needed to be made nullable so tests can construct null edge cases and normal cases. Same for store.dart
    3 .channel.dart- changed position of _warnInvalidVisibilityPolicy up to ChannelStore mixin so it can be accessed by all methods.

handleUserTopicEvent- Separated null and .none handling here so that later if logging required can be done easier
skips adding any null entry to the map
If event.visibilityPolicy is null and if there was no previous entry, it just returns (skips it)
If there was a previous entry, it removes that entry. Drop the stream map if it becomes empty

willChangeIfTopicVisibleInStream and willChangeIfTopicVisible - Added null checks that return UserTopicVisibilityEffect.none when visibilityPolicy is null since its case for .none is just return (do nothing)

  1. widgets-removed all unreachable cases with UserTopicVisibilityPolicy.unknown so widgets only handle 4 valid enum values now

Tests added:

  1. events_test.dart - Check at boundary if UserTopicEvent.visibilityPolicy maps known JSON values to the correct enum cases and unknown/missing/null values to null.
  2. initial_snapshot_test- Same check but done for the initial snapshot
  3. channel_test.dart- Checks for null visibilityPolicy-
    does not add a new entry with null for that topic
    if there was an old entry, it removes it from topicVisibility
    Checks to check that invalid visibility_policy (null) does not cause any visibility change

Passed all tests

@Cairo09 Cairo09 force-pushed the remove-unknown-from-usertopicvisibilitypolicy branch 8 times, most recently from 8ba75a6 to 02fdee4 Compare January 27, 2026 15:05
@gnprice
Copy link
Member

gnprice commented Jan 27, 2026

Thanks. This PR contains a lot of unrelated changes. You'll need to remove those before it can be reviewed. It will also need tests. These are the two basic requirements stated in our README.

@Cairo09 Please also see the AI use policy in the Zulip contributing guide. Your PR description reads like the output of an LLM, and it says a bunch of things that are either redundant or untrue. That isn't a good use of maintainers' time.

@Cairo09 Cairo09 force-pushed the remove-unknown-from-usertopicvisibilitypolicy branch 2 times, most recently from 33d69fa to 863ae86 Compare January 28, 2026 08:28
@Cairo09
Copy link
Author

Cairo09 commented Jan 28, 2026

Sorry for the trouble, and thankyou for the feedback. I’ve started on removing the unrelated changes, clean up commits and commit history, add tests, and change the PR description according to the guidelines and keep the PR as a draft for now. This is my first pr so I'll make sure to learn from this and do better going forward.

@Cairo09 Cairo09 marked this pull request as draft January 28, 2026 15:28
@gnprice
Copy link
Member

gnprice commented Jan 28, 2026

Sounds good. When you're finished with those changes, you can mark the PR as ready for review and we'll take a look again.

Previously, UserTopicVisibilityPolicy included an .unknown value to
represent unexpected server values.
This change removes that value and instead invalid or missing
visibility_policy values deserialize as null. Did the same for UserTopicItem.visibilityPolicy, so unknown/missing values in the initial snapshot user_topics data are represented as null.handleUserTopicEvent- For events with visibilityPolicy == null, removed any existing entry for that topic and dropped the per‑stream map if it becomes empty.Tests added
Fixes zulip#1074
@Cairo09 Cairo09 force-pushed the remove-unknown-from-usertopicvisibilitypolicy branch from 36abc82 to 3e56f1d Compare January 31, 2026 10:08
@Cairo09 Cairo09 marked this pull request as ready for review January 31, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Statically ensure that UserTopicVisibilityPolicy.unknown is not possible in our data models

2 participants