feat(feed): remove new feed from home dropdown#4542
Conversation
This comment has been minimized.
This comment has been minimized.
c3ea059 to
22489a7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Requesting changes before approval. The feature direction is fine, but this PR still needs one behavior fix and two debt cleanups to meet the bar: handle live invalidation of a selected subscribed list, remove the unrelated optimization skip tag, and remove the now-stale subscribed-list video-refs scaffolding/API if there is no remaining production caller.
| return; | ||
| } | ||
|
|
||
| if (!state.isSubscribedListSelected) { |
There was a problem hiding this comment.
This does not handle the live case where the currently selected subscribed list disappears. Startup restore already falls back when a saved list:<id> no longer exists, but this path just reloads state.source unconditionally. If the user is on a list source and unsubscribes from that list, Home stays pinned to a stale VideoFeedSource.subscribedList, reloads empty data, and keeps the old label. Please mirror the startup fallback here: detect that the selected list is gone, switch to a valid source, and rewrite the persisted selection.
There was a problem hiding this comment.
Fixed in 0decd8f: live path in _onCuratedListsChanged now detects when the selected subscribed list is gone from the new set and falls back to VideoFeedSource.forYou() (matching the _restoreSource fallback), updates SharedPreferences so the change survives restart, and reloads. Added a blocTest covering the unsubscribe transition.
| @@ -65,6 +65,9 @@ void main() { | |||
| when( | |||
| () => mockCuratedListRepository.getSubscribedListVideoRefs(), | |||
There was a problem hiding this comment.
This stub looks stale now that plain Following explicitly passes videoRefs: const {} and the production path no longer calls getSubscribedListVideoRefs(). If there is no remaining production caller after this PR, please remove the orphaned repository API and the leftover test scaffolding for it instead of carrying dead transitional surface area forward.
There was a problem hiding this comment.
Fixed in 0decd8f: grep confirmed zero production callers of getSubscribedListVideoRefs in mobile/lib. Deleted the method from CuratedListRepository, its dedicated test group in curated_list_repository_test.dart, and the stale stub from the bloc test setUp. Note: the videoRefs: param on VideosRepository.getHomeFeedVideos is now also dead surface (only called with const {}) — happy to remove it in a follow-up if you want, but it wasn't named here so I left it.
| // ABOUTME: Widget tests for KeyManagementScreen public key and export capability UI | ||
| // ABOUTME: Verifies public key copy plus Keycast local-vs-remote signing states | ||
|
|
||
| @Tags(['skip_very_good_optimization']) |
There was a problem hiding this comment.
This tag addition is unrelated to the Home dropdown change and should be removed from this PR. I verified the optimized suite passes without it, so it is not load-bearing and it adds avoidable test debt.
There was a problem hiding this comment.
Fixed in 0decd8f: removed the @Tags(['skip_very_good_optimization']) line; key_management_screen_test.dart passes locally without it.
Addresses three review threads on PR #4542: 1. video_feed_bloc: when curated lists change and the currently selected subscribed list is no longer in the subscribed set, fall back to forYou (mirroring the startup _restoreSource fallback) instead of reloading a now-stale list source. Persist the new mode to prefs so the fallback survives restart. Adds a blocTest covering the transition. 2. curated_list_repository: remove getSubscribedListVideoRefs() and its tests; the bloc no longer composes following with subscribed-list refs, so the API is dead production surface. Also strips the stale stub from the bloc test setUp. 3. key_management_screen_test: remove the unrelated skip_very_good_optimization tag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dbd0ed8 to
0decd8f
Compare
|
@NotThatKindOfDrLiz all three threads addressed in 0decd8f. Ready for another look. |
This comment has been minimized.
This comment has been minimized.
Addresses three review threads on PR #4542: 1. video_feed_bloc: when curated lists change and the currently selected subscribed list is no longer in the subscribed set, fall back to forYou (mirroring the startup _restoreSource fallback) instead of reloading a now-stale list source. Persist the new mode to prefs so the fallback survives restart. Adds a blocTest covering the transition. 2. curated_list_repository: remove getSubscribedListVideoRefs() and its tests; the bloc no longer composes following with subscribed-list refs, so the API is dead production surface. Also strips the stale stub from the bloc test setUp. 3. key_management_screen_test: remove the unrelated skip_very_good_optimization tag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0decd8f to
64cda5f
Compare
|
Took over this PR branch and pushed Cleanup in this pass:
Local verification on the rebased branch:
Watching GitHub checks now and will fix anything that comes back red before approving. |
Mobile PR PreviewPreview refreshed for Last refresh:
|
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Final pass on 64cda5f: rebased onto current main, removed the redundant following-feed lint suppression, removed the new empty-state localization debt instead of whitelisting it, and kept the branch green through local verification plus full GitHub checks. Ready to merge.
Closes #4543
Summary
Verification