Fix/auth bugs in dev and prod env #182
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Changed the appEnvironment constant from AppEnvironment.demo to AppEnvironment.development in the main.dart file. - This modification allows the application to run in development mode, which typically includes more verbose logging and debugging features.
- Add a new section for upcoming releases in the CHANGELOG.md file
Creates the foundational `AppInitializer` service and its related `InitializationResult` sealed classes. This establishes a new, isolated "locked box" for orchestrating the entire application startup sequence. The `AppInitializer` class will centralize all startup logic, eliminating race conditions and making the process more robust, testable, and maintainable. This commit only creates the file structure and class shells; the implementation of the initialization logic will follow in subsequent steps.
Implements the main `initializeApp` method within the new `AppInitializer` service. This method now contains the entire sequential startup flow: 1. Fetches `RemoteConfig`. 2. Checks for maintenance mode and forced updates. 3. Fetches the initial user. 4. If a user exists, fetches `UserAppSettings` and `UserContentPreferences` in parallel for improved performance. 5. Handles demo-specific data initialization. 6. Returns a single, definitive `InitializationResult` (`Success` or `Failure`). This centralizes the critical startup logic, eliminates race conditions, and adds extensive logging for maintainability, creating a robust "glass box" for app initialization.
Implements the main initializeApp method within the new AppInitializer service. This method now contains the entire sequential startup flow: Fetches RemoteConfig. Checks for maintenance mode and forced updates. Fetches the initial user. If a user exists, fetches UserAppSettings and UserContentPreferences in parallel for improved performance. Handles demo-specific data initialization.
… initialization - Introduce AppInitializer service to handle initializations in a centralized manner - Remove individual initialization steps from bootstrap function - Update logger statements for clarity and consistency - Add packageInfoService and logger dependencies where necessary
Refactors the `App` widget to accept a single `InitializationResult` object in its constructor. This change significantly simplifies the widget's API and removes the long list of individual initial data parameters (`initialUser`, `initialRemoteConfig`, etc.). The `App` widget now passes this result directly to the `AppBloc`, which has become the sole consumer of the pre-loaded initialization data. This aligns with the new "locked box" architecture, where `AppInitializer` handles all startup logic and `AppBloc` manages the resulting state.
This is a major refactoring of the `AppBloc`. - The `AppBloc` constructor is completely rewritten. It no longer accepts a long list of repositories and initial data. Instead, it takes a single `InitializationResult` object from the `AppInitializer`. - The initial state of the BLoC is now determined synchronously from this result, eliminating all asynchronous logic and potential race conditions from the constructor. - The `_onAppStarted` event handler is now a no-op, as all its logic has been moved to the `AppInitializer`. - The `_onAppUserChanged` handler is significantly improved. It now correctly detects role changes (anonymous to authenticated) and delegates the complex data migration and re-fetching logic to the `AppInitializer.handleUserTransition` method. - All direct repository dependencies have been removed from the BLoC. It now accesses repositories when needed via `context.read<T>()` using the `navigatorKey`. This change makes the `AppBloc` a much simpler and more robust pure state manager, solidifying the new "locked box" architecture.
Removes all now-unused repository and service parameters from the `App` and `_AppView` widgets. The `App` widget's responsibility is now strictly limited to providing the repositories that are needed by the `GoRouter` and feature BLoCs. All initialization-related data is now passed exclusively through the `InitializationResult` object to the `AppBloc`.
Removes the redundant `environment` property from `AppState`. The environment is a constant determined at bootstrap and does not need to be part of the dynamic application state.
Removes dead code from `AppBloc` by deleting the now-obsolete `_onAppPeriodicConfigFetchRequested` and `_onAppVersionCheckRequested` event handlers. Critically, this change also corrects the logic in `_onAppUserChanged` to properly handle user state transitions. It now correctly identifies a logout (`newUser == null`) versus a login or role change (`newUser != null`), ensuring the data migration and re-fetching flow is triggered only when a user is present.
This commit addresses several issues in `AppBloc` that arose from the recent refactoring: - **Restores Event Handlers:** Re-registers the `AppUserFeedDecoratorShown` and `AppUserContentPreferencesChanged` event handlers, which were incorrectly removed. - **Fixes Nullability Errors:** Corrects null-safety errors in `_onAppUserChanged` and `_onAppPeriodicConfigFetchRequested` to ensure the logic for user transitions and periodic updates is robust. - **Removes Dead Code:** Deletes the unnecessary import of `package:bloc/bloc.dart`. The `AppBloc` is now correctly wired to handle all its responsibilities within the new architecture.
This commit addresses several issues in `AppBloc` that arose from the recent refactoring: - **Restores Event Handlers:** Re-registers the `AppUserFeedDecoratorShown` and `AppUserContentPreferencesChanged` event handlers, which were incorrectly removed. - **Fixes Nullability Errors:** Corrects null-safety errors in `_onAppUserChanged` and `_onAppPeriodicConfigFetchRequested` to ensure the logic for user transitions and periodic updates is robust. - **Removes Dead Code:** Deletes the unnecessary import of `package:bloc/bloc.dart`. The `AppBloc` is now correctly wired to handle all its responsibilities within the new architecture.
Removes the now-obsolete `AppVersionCheckRequested` event, as this logic is handled by the `AppInitializer`. Also simplifies the `AppStarted` event by removing the unused `initialUser` parameter, aligning it with the new initialization flow where the `AppBloc` receives all initial data via its constructor.
Removes the initial `HeadlinesFeedFetchRequested` event dispatch from the `initState` method of `HeadlinesFeedPage`. This responsibility is being moved directly into the `HeadlinesFeedBloc`, which will now listen to the `AppBloc` to determine the correct moment to fetch data. This change eliminates a potential race condition and further decouples the UI from business logic orchestration.
This is a critical refactoring that moves the initial data fetch logic for the headlines feed from the UI layer (`HeadlinesFeedPage`) into the `HeadlinesFeedBloc` itself. The BLoC now listens to the `AppBloc`'s state stream. It waits for the `AppBloc` to signal a stable, "running" status (`authenticated` or `anonymous`) before dispatching its own `HeadlinesFeedRefreshRequested` event. This change fixes a major race condition where the feed could attempt to load before its dependencies (like remote config or user settings) were available, leading to indefinite loading spinners. The feed's lifecycle is now correctly orchestrated by the application's global state.
Adds a more detailed comment to the `_onAuthenticationVerifyCodeRequested` handler in `AuthenticationBloc`. This comment explicitly states that upon successful code verification, the global `AppBloc` is responsible for handling the entire post-login sequence (data fetching, navigation, etc.) via its listener on the `authStateChanges` stream. This reinforces the new, centralized lifecycle management architecture.
Adds a more detailed comment to the `_onAuthenticationVerifyCodeRequested` handler in `AuthenticationBloc`. This comment explicitly states that upon successful code verification, the global `AppBloc` is responsible for handling the entire post-login sequence (data fetching, navigation, etc.) via its listener on the `authStateChanges` stream. This reinforces the new, centralized lifecycle management architecture.
Fixes a bug where the "Create an Account" call-to-action button in the feed decorator was pointing to a malformed URL (`/authentication//account-linking`). The `ctaUrl` is now correctly set to `Routes.accountLinking`, ensuring that tapping the button navigates the user directly to the correct account linking page.
- Robust and race-condition-free implementation - Fixes indefinite loading issues - Improves data migration processes - Enhances authentication flows
Overhauls the "Clean & Modern Architecture" section of the README to more accurately and impressively describe the application's structure. - Replaces low-level details with high-level architectural pillars. - Highlights the Multi-Layered Architecture (Data, Repository, BLoC). - Emphasizes the robust `AppInitializer` for race-condition-free startup. - Better articulates the use of BLoC, GoRouter, and Dependency Injection.
- Add new entry for upcoming release - Document major refactor of app startup and authentication lifecycle - Highlight fixes for indefinite loading, data migration, and authentication flows
Updates the "Clean & Modern Architecture" section to describe the layers in terms of their function (e.g., "Data Layer (handling raw data retrieval)") rather than using project-specific internal class names like `DataClient`. This makes the description more professional and universally understandable.
Fixes a build-time error in `HeadlinesFeedBloc` where calls to `darkTheme` and `lightTheme` were missing the required `appTextScaleFactor` and `appFontWeight` arguments. The bloc now correctly passes these values from the `AppBloc`'s state, ensuring that the theme used for generating the `AdThemeStyle` is correctly configured.
Fixes several build-time errors in `AppInitializer`: - Removes the redundant `abstract` keyword from the `sealed class InitializationResult`. - Explicitly sets the generic type for `Future.wait` to `<dynamic>` to resolve type inference issues when destructuring results. - Removes the unused `_userRepository` field. - Corrects a call to `AppLocalizationsX.of(context)` to correctly retrieve localization strings.
Fixes several build-time errors in `AppInitializer`: - Removes the redundant `HttpExceptionX` extension, as this functionality is already provided by the `ui_kit` package. - Removes the unused `_userRepository` field and its constructor initialization. - Adds explicit type casts to the results of `Future.wait` to resolve `dynamic` type assignment errors when destructuring.
Removes the `DataRepository<User>` dependency from the `AppInitializer` service. This repository is only used for updating the user model, a responsibility that correctly resides within the `AppBloc`, not during the initial app startup sequence.
Fixes a build-time error in `app.dart` where the call to `createRouter` was missing several required repository arguments. This change temporarily passes the required repositories from the `BuildContext` to the `createRouter` function. This is an intermediate step to resolve the immediate build error. A subsequent refactoring of the router and its nested BLoC providers will eliminate the need to pass these repositories as arguments, allowing them to be read from the context directly where needed.
Refactors `createRouter` to no longer accept repository instances as direct arguments. This is a crucial step in finalizing the new dependency injection architecture. - The function signature for `createRouter` is now significantly simpler. - BLoCs created within the router (e.g., `SettingsBloc`, `HeadlinesFeedBloc`, `EntityDetailsBloc`) now correctly fetch their dependencies from the `BuildContext` using `context.read<T>()`. This change ensures that repositories are provided once at the top of the widget tree and consumed where needed, adhering to a clean DI pattern.
Modified the GoRouter configuration for the `feedFilter` route. - The route builder now expects a `HeadlineFilter` object to be passed in `state.extra`. - This object is extracted and passed to the `HeadlinesFilterPage` constructor. - This change completes the decoupling of the filter page from the feed page's context, resolving the `BlocProvider.of()` error by providing dependencies explicitly.
Passes the `HeadlinesFeedBloc` instance from the `HeadlinesFeedPage` to the `HeadlinesFilterPage` via `GoRouter`'s `extra` parameter. This is the first step in fixing the `ProviderNotFoundException` that occurs when the filter page tries to access the bloc, as it exists in a separate navigation route.
Updates the `feedFilter` route to accept a map in its `extra` parameter. This map contains both the `initialFilter` and the `HeadlinesFeedBloc` instance. The route now wraps `HeadlinesFilterPage` in a `BlocProvider.value`, making the `HeadlinesFeedBloc` directly available to the filter page. This is a key step in resolving the `ProviderNotFoundException` by ensuring the filter page has access to the bloc it needs to update, regardless of its position in the navigation stack.
Refactors the `HeadlinesFilterPage` to correctly handle asynchronous dialog interactions and to properly access the `HeadlinesFeedBloc`. - The `_saveAndApplyFilter` method now `await`s the result of `showDialog`, ensuring the page is only popped *after* the save dialog is dismissed. This fixes a bug where the page would close prematurely. - The `_applyFilter` method now correctly reads the `HeadlinesFeedBloc` from the context provided by the router's `BlocProvider.value`, eliminating the `ProviderNotFoundException`. - All related methods (`_applyAndExit`, `_showApplyOptionsDialog`) have been updated to use the new synchronous navigation flow, removing unnecessary `BuildContext` passing.
Updates the `HeadlinesFeedBloc` constructor to accept an optional `UserContentPreferences` object. This object is used to "seed" the bloc's initial state with the user's saved filters. This change is the first step in fixing a state synchronization bug where saved filters would not appear on the feed page after navigating away and back. By providing the initial state directly, we eliminate the dependency on a reactive stream update for the initial data load.
Updates the router to read the `userContentPreferences` from the `AppBloc` at the time of `HeadlinesFeedBloc` creation. This initial state is then passed directly into the bloc's constructor. This change ensures that the `HeadlinesFeedBloc` is immediately aware of the user's saved filters, fixing a bug where the filters would disappear upon navigating back to the feed page.
Introduces a new `ConfirmationDialog` widget in the shared widgets directory. This generic dialog can be used across the application to prompt users for confirmation before performing a destructive or important action. It returns a boolean value (`true` for confirm, `false` for cancel) via `Navigator.pop`, allowing callers to `await` the user's choice.
Adds new localization strings in English and Arabic for the title, content, and confirm button of the new reusable confirmation dialog.
Implements a confirmation dialog on the Saved Filters page before deleting a filter, improving user experience. This change also fixes a UI bug where a red screen would flicker upon deletion. By `await`ing the result of the new asynchronous dialog, we ensure the `PopupMenuButton`'s route is fully closed before the `AppBloc` state changes and the list rebuilds, thus preventing the "unsafe ancestor" error.
Refactors the `HeadlinesFeedPage` to render the empty state message within the `CustomScrollView` using a `SliverFillRemaining` widget. This ensures that the `FeedSliverAppBar` and `SavedFiltersBar` remain visible and interactive even when a filter returns no results. This provides a more consistent user experience and allows users to easily modify or clear their filters without being forced into a UI dead end.
Refactors the `AppBloc` and `AppState` to correctly handle user logout. Previously, on logout, the `AppBloc`'s state retained the old user object. This caused a bug where signing back in with the same user ID would be ignored by the `_onAppUserChanged` handler, as it appeared no state change had occurred. This left the user stuck on the authentication screen. This fix introduces a `clearUser` flag to `AppState.copyWith`, ensuring that on logout, the `user`, `settings`, and `userContentPreferences` are explicitly set to `null`. This guarantees a clean state and allows the app to correctly process a subsequent login by the same user.
…rvice Extends the `ContentLimitationService` to enforce limits on saving new filters. This change adds a `saveFilter` case to the `ContentAction` enum and implements the corresponding logic within the `checkAction` method. It reads the user's role and the number of currently saved filters, comparing it against the role-specific limit defined in the remote configuration (`guestSavedFiltersLimit`, `authenticatedSavedFiltersLimit`, `premiumSavedFiltersLimit`). This centralizes the business rule for filter limits, ensuring it can be applied consistently across the app.
Integrates the `ContentLimitationService` into the `HeadlinesFilterPage` to enforce limits on saving new filters. When a user attempts to "Apply & Save" a filter, the app now checks if they have reached their limit. If the limit is met, a `ContentLimitationBottomSheet` is displayed to inform the user, and the save process is halted. This provides immediate, clear feedback and aligns the filter-saving feature with the app's overall content limitation strategy.
Fixes a critical bug in the demo environment where authenticating as a new user (anonymous or via email) would fail. Previously, when a new user was created, the `AppInitializer` would immediately try to fetch their settings and preferences. In the demo environment, this data did not exist yet, causing a `NotFoundException` that would halt the login process and leave the UI unresponsive. This fix modifies `AppInitializer.handleUserTransition` to explicitly call `DemoDataInitializerService` for new users in the demo environment. This ensures that the necessary user data is created on-the-fly *before* any attempt is made to read it, resolving the race condition and allowing the authentication flow to complete successfully.
- Change 'headlineSearchEnterQuery' to 'headlineSearchInitialHeadline' - Change 'headlineSearchEnterQuerySubheadline' to 'headlineSearchInitialSubheadline'
- Add 'radius' parameter to UserAvatar constructor - Update CircleAvatar to use customizable radius - Set default radius value to AppSpacing.lg
- Add sign-in/sign-out buttons to AppBar for anonymous and authenticated users - Replace custom avatar implementation with shared UserAvatar widget - Style user role display as a "capsule" for better visual appeal - Remove separate sign-in and sign-out button implementations - Update localized strings for guest user messages
Member
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and substantial refactoring of the application's startup and authentication lifecycle. The introduction of the AppInitializationPage and AppInitializer service as a "gatekeeper" is a fantastic architectural improvement that effectively eliminates race conditions and makes the startup process much more robust and predictable. The AppBloc is now significantly cleaner and adheres better to the Single Responsibility Principle. The improved environment configuration using --dart-define and the fixes to UI race conditions (like in the filter page) are also great additions. I've found a couple of minor issues, but overall, this is a very high-quality contribution that greatly improves the maintainability and stability of the app.
…FeedPageState - Move super.build(context) to the beginning of the build method - Remove redundant super.build calls elsewhere in the method - Add comments explaining the importance of calling super.build first
Member
Author
|
/gemini summary |
Summary of ChangesThis pull request significantly refactors the application's core startup and authentication mechanisms to enhance robustness and predictability. By introducing a dedicated initialization service and streamlining the Highlights
Changelog
Activity
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
IN DEVELOPMENT
Description
This pull request significantly refactors the application's core startup and authentication mechanisms to enhance robustness and predictability. By introducing a dedicated initialization service and streamlining the AppBloc architecture, the app's lifecycle, from launch to user login/logout and data loading, is now more stable and less prone to race conditions. This results in a more reliable and consistent user experience across all environments, with improved error handling and a cleaner separation of concerns within the codebase.
Type of Change