Removing circular dependencies of auth_controller and tab_scaffold#3443
Conversation
…thUser model to its own file, introducing an AuthLifecycle callback provider for notification registration, and using a reactive provider listener for socket reconnection
…y introducing tab_navigation leaf module for shared navigation state and updating all importing widgets, services, and tests
|
#3429 @freebeartogoodhome check this out whenever you get time |
|
Maybe it would be better to split them into two PRs. I think all of the tab_scaffold stuff looks correct. I can't comment too much on the auth stuff because it takes more thinking and my brain is done for a few days, sorry. It gets tired easily. Also I'm quite new to Flutter development so I don't know when to use |
oh no i didn't remove anything, i just shifted the user model code (with freezed) to a new file. everything works the same way I just extracted it. also all the tests passed. but if you say so, I will try to create a separate pr for that cuz I worked on the auth first so just a rollback won't work |
Got it. I missed where it went. Like I said my brain is pretty tired. Thanks for working on this. |
no problem lol, btw I found some other circular dependencies too, i will resolve them too. also i checked the li0n.dart issue, i would suggest not touching it cuz those files are automatically generated by flutter based on its config. any manual changes may risk overwrites |
…nums, classes, and typedefs into a dedicated account_pref_types leaf module, allowing account_repository to import it instead of the full preferences controller
…r and provider into ongoing_games_notifier
…Id> in PuzzleStreakResponse instead of Streak typedef, and extracting the notifier and provider into a new puzzle_streak_controller module
…ningsListProvider and puzzleOpeningNameProvider to puzzle_providers, making puzzle_opening a leaf module for type definitions
|
What is the actual gain of doing that? |
…nto chat_providers and chat data models into chat_message leaf modules
well that's what I thought before working on this issue. the benefit is on the development side rather than the users' side. circular dependencies were increasing the hot reload and restart time and the unit tests got more complex as to test a particular feature, there was extra code in it due to file having irrelevant codes for tests. the benefit now is that the hot reload and restart speeds are increased, unit tests now can be lightweight (for models and all), there is a proper clean architecture that follows unidirectional flow now, and code is more reusable (due to decoupling of models and other stuff). also I read that there is some benefit in compilation during builds as well |
|
This is a huge change so I must be cautious. I find the hot reload time to be already pretty fast. If it makes really the build (or hot reload) faster, can you benchmark it so we see the gain? Perhaps it also speeds up the build runner, if you can measure this it'd be nice. Thanks. also:
When I read this, I understand it goes beyond just "removing circular dependencies". And I'm not sure to understand exactly what it changes (both with notification service and socket pool), can you elaborate on that please? |
|
I hear you on all of that. I think those should be split into separate PRs, imo. The dependency stuff should just be on its own. |
yea i understand. it's not changing the notification service or socket pool. they are just files which were importing these circular dependencies. i just changed the imports after decoupling the circular dependencies. other than that I haven't practically benchmarked the speeds, i just read it but I will do it and check |
So i benchmarked it - hot restart (avg): build_runner/aot (avg): static analysis (flutter analyze): |
|
Ok thanks. Not a huge gain but still a gain. Gain is actually nice for the static analysis. Probably even better on a slow machine like GitHub CI ones. So I can accept that. |
|
@GalaxyPhoenix716 I saw there are changes in account preferences in your PR. I'm working on this one right now: https://github.com/lichess-org/mobile/pull/3316/changes#diff-ce2bf5ada85564dfe3cfec40a8970fa0f0d6bc1e5fc4ec27bcc821533ae770eb Which will change things here. I'm going to merge that one before so I'll let you solve the conflicts. |
There was a problem hiding this comment.
Pull request overview
This PR refactors several modules to break circular dependencies by extracting shared navigation/auth/model/provider concerns into new leaf files (notably tab_navigation.dart and auth_user.dart) and updating imports across the app and tests.
Changes:
- Introduces
tab_navigation.dartand migrates tab navigation keys/controllers/observers/providers out oftab_scaffold.dart, updating callers accordingly. - Decouples auth and notifications by moving
AuthUsertoauth_user.dartand adding anAuthLifecyclecallback provider to avoid UI/network/service imports insideAuthController. - Extracts additional controller/provider code into dedicated files (chat, puzzle streak, ongoing games, account pref types, puzzle openings), keeping core model/type files smaller.
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_provider_scope.dart | Updates import to tab_navigation.dart for rootNavRouteStackObserver. |
| test/model/challenge/challenge_service_test.dart | Updates import to tab_navigation.dart for currentNavigatorKeyProvider. |
| test/app_links_service_test.dart | Updates import to tab_navigation.dart. |
| lib/src/widgets/platform.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/watch/watch_tab_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/tournament/tournament_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/puzzle/puzzle_tab_screen.dart | Removes puzzle_opening.dart import and switches scaffold import to tab_navigation.dart. |
| lib/src/view/puzzle/puzzle_screen.dart | Removes puzzle_opening.dart import and switches scaffold import to tab_navigation.dart. |
| lib/src/view/more/more_tab_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/message/conversation_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/learn/learn_tab_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/home/home_tab_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/home/games_carousel.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/home/blog_carousel.dart | Updates import for homeTabInteraction to tab_navigation.dart. |
| lib/src/view/game/game_result_dialog.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/chat/chat_screen.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/view/broadcast/broadcast_carousel.dart | Updates import for watchTabInteraction to tab_navigation.dart. |
| lib/src/tab_scaffold.dart | Removes navigation/provider/global-key definitions and imports them from tab_navigation.dart. |
| lib/src/tab_navigation.dart | New file containing bottom-tab enum, keys/controllers, observers, and scaffold properties moved out of tab_scaffold.dart. |
| lib/src/shared_pgn_service.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/quick_actions.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/network/socket.dart | Adds a listener to react to auth changes by reconnecting the default socket client. |
| lib/src/model/study/study.dart | Switches chat dependency to chat_message.dart (core model) instead of chat.dart. |
| lib/src/model/puzzle/puzzle_streak.dart | Leaves only model/types and re-exports controller (extracted to new file). |
| lib/src/model/puzzle/puzzle_streak_controller.dart | New file with puzzle streak controller/provider logic extracted from puzzle_streak.dart. |
| lib/src/model/puzzle/puzzle_repository.dart | Adjusts streak response shape to use a simple IList<PuzzleId> instead of cyclic types. |
| lib/src/model/puzzle/puzzle_providers.dart | Moves openings-related providers here to keep puzzle_opening.dart as pure types. |
| lib/src/model/puzzle/puzzle_opening.dart | Removes openings providers; keeps only type definitions. |
| lib/src/model/notifications/notification_service.dart | Registers auth lifecycle callbacks via authLifecycleProvider to decouple from AuthController. |
| lib/src/model/message/message_service.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/model/game/playable_game.dart | Switches chat dependency to chat_message.dart (core model). |
| lib/src/model/engine/nnue_service.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
| lib/src/model/correspondence/correspondence_service.dart | Updates navigator key provider import to tab_navigation.dart. |
| lib/src/model/common/preloaded_data.dart | Switches auth model dependency to auth_user.dart. |
| lib/src/model/chat/chat.dart | Removes core message parsing/models and re-exports chat_message.dart. |
| lib/src/model/chat/chat_providers.dart | New file containing controller-dependent chat providers extracted from chat_mixin.dart. |
| lib/src/model/chat/chat_mixin.dart | Removes controller imports and exports new chat_providers.dart. |
| lib/src/model/chat/chat_message.dart | New file containing ChatMessage, ChatData, pick parsing, and spam regexes. |
| lib/src/model/challenge/challenge_service.dart | Updates navigator key provider import to tab_navigation.dart. |
| lib/src/model/auth/auth_user.dart | New file defining AuthUser, AuthLifecycle, and authLifecycleProvider. |
| lib/src/model/auth/auth_storage.dart | Switches auth model dependency to auth_user.dart. |
| lib/src/model/auth/auth_repository.dart | Switches auth model dependency to auth_user.dart. |
| lib/src/model/auth/auth_controller.dart | Removes embedded AuthUser type and replaces service calls with lifecycle callbacks. |
| lib/src/model/account/ongoing_games_notifier.dart | New file extracting ongoing-games notifier/provider out of ongoing_game.dart. |
| lib/src/model/account/ongoing_game.dart | Keeps only model and re-exports the notifier (extracted to new file). |
| lib/src/model/account/account_service.dart | Updates navigator key provider import to tab_navigation.dart. |
| lib/src/model/account/account_repository.dart | Imports extracted preference types from account_pref_types.dart. |
| lib/src/model/account/account_preferences.dart | Removes embedded types/defaults and re-exports extracted pref types. |
| lib/src/model/account/account_pref_types.dart | New file containing account preference types + defaults extracted from account_preferences.dart. |
| lib/src/app.dart | Starts using tab_navigation.dart (for moved nav globals/observers). |
| lib/src/app_links_service.dart | Swaps tab_scaffold.dart import for tab_navigation.dart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alright sure |
i'll solve these issues before solving the conflicts |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ct cycles - Moved the Freezed AccountPrefState data model, its default instance, and helper serializers to �ccount_pref_types.dart. - Updated �ccount_repository.dart to import �ccount_pref_types.dart instead of �ccount_preferences.dart to break the mutual circular dependency cycle. - Removed stale generated part files (�ccount_preferences.freezed.dart and �ccount_preferences.g.dart). - Restored @jsonvalue annotations to enums in �ccount_pref_types.dart to satisfy JSON serialization requirements and fix storage unit tests. - Removed unused imports and cleaned up trailing spaces.
|
@veloce i have resolved the merge conflicts and other issues detected by copilot. please review |
|
Looks like you're doing really well! I will go over it when I get a chance! |
thanks alot. do tell me whenever this pr gets merged |
It will be up to Veloce, I'm just a bear that tries to help out around here. Only been here a few months more than you. :) |
thanks for guiding me |
- Replaced the single-value AuthLifecycle callback registry with a global broadcast stream �uthEventsStream in �uth_user.dart. - Updated AuthController to publish AuthEvent.signIn and AuthEvent.signOut events to the broadcast stream. - Modified NotificationService to subscribe to �uthEventsStream reactively to register/unregister push tokens and cancel the subscription on dispose. - Refactored socketPoolProvider to trigger reconnects only on explicit auth events from �uthEventsStream rather than listening to all auth state mutations. - Removed legacy provider definitions and cleaned up unused imports.
|
@veloce I've updated the PR based on your feedback |
4e66dfa to
acfcf02
Compare
|
thankss @veloce First, we could make the socket pool connection lazy instead of opening a web socket immediately when the app starts. That way we only connect when the user actually opens a screen that needs it, which saves resources if they're offline or just browsing static screens. Also, for the settings screen, we could do optimistic UI updates. When someone toggles something like Zen Mode, we'd update the UI instantly rather than waiting for the server's HTTP call to finish. If the call fails, we can just flip the toggle back and show a quick warning. It would make the settings feel much faster. On the rendering side, since the clocks tick constantly, wrapping them and the chess board in a RepaintBoundary would prevent Flutter from rebuilding the entire screen layout on every single tick, which helps a lot with battery usage. Lastly, we could cache the pub-cache and build_runner directories in our GitHub Actions. It would speed up the CI pipeline build times for PRs quite a bit. |
AuthController Decoupling: Moved the AuthUser model to its own file (auth_user.dart) and created an AuthLifecycle callback provider. This allows the notification service to register its callbacks on startup and the socket pool to listen reactively, freeing AuthController from importing UI/network services.
TabScaffold Decoupling: Shifted all navigation providers, global keys, scroll controllers, and route stack observers out of tab_scaffold.dart into a new leaf file called tab_navigation.dart. This breaks the circular dependency loops with the individual tab screens.
Other minor circular dependencies resolved:-
Account & Ongoing Games Decoupling: Extracted preferences types into account_pref_types.dart and ongoing game providers/notifiers into ongoing_games_notifier.dart. This lets account_repository.dart import preferences without pulling in the full AccountPreferences notifier class, and keeps ongoing_game.dart a pure model file.
Puzzle Streak Decoupling: Extracted the notifier and provider from puzzle_streak.dart into puzzle_streak_controller.dart, freeing it from auth and sound services. Also changed the response structure to use a clean list instead of cyclic type definitions.
Puzzle Openings Decoupling: Shifted openings-related providers into puzzle_providers.dart so that puzzle_opening.dart can be a pure type-definition leaf file.
Chat & Controller Decoupling: Created chat_providers.dart for controller-dependent providers and chat_message.dart for core models, freeing both chat_mixin.dart and playable_game.dart from cyclic controller imports.
Verified everything by running tests (all passed) and analyze and formatted the code
Relevant Issue: #3429