Add Bloc-like architecture layer for offline sync workflows#6
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @Harsh4114, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the package by integrating a new Bloc-like architecture designed to streamline the development of offline-first synchronization features. It provides a structured, event-driven approach to managing data consistency between local and cloud sources, offering clear abstractions and a robust framework for handling sync operations. This addition allows for more modular and testable sync flows, complementing the existing Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
The pull request introduces a new Bloc-like architecture for offline synchronization, providing a familiar mental model and a clear separation of concerns. The new components, including LocalDataSource, CloudDataSource, SyncLogStore, SyncRepository, SyncController, and OfflineSyncEngine, are well-structured and adhere to common Dart practices like immutability for data models. The README.md has been updated with a comprehensive guide and wiring example, which is very helpful for onboarding. A basic unit test is also included, validating the core add and sync functionality.
One area for improvement is the log ID generation, which could lead to potential collisions under high concurrency.
| String _logId() { | ||
| final micros = DateTime.now().microsecondsSinceEpoch; | ||
| return 'sync_log_$micros'; | ||
| } |
There was a problem hiding this comment.
The _logId() method uses DateTime.now().microsecondsSinceEpoch to generate a unique ID. While this often works, it's not guaranteed to be unique if two SyncLog entries are created within the same microsecond, which can happen in highly concurrent scenarios. Consider using a universally unique identifier (UUID) generator for more robust uniqueness guarantees.
String _logId() {
// Consider using a UUID generator for more robust uniqueness
// For example, using the 'uuid' package: Uuid().v4();
final micros = DateTime.now().microsecondsSinceEpoch;
return 'sync_log_$micros';
}There was a problem hiding this comment.
Pull request overview
Introduces a new “Bloc-like” (events → controller → states) API layer for the offline sync engine, aiming to provide a higher-level, testable abstraction over local/cloud adapters and a sync-log queue, while keeping the existing SyncManager intact.
Changes:
- Added new bloc-like contracts (
LocalDataSource,CloudDataSource,SyncLogStore) and in-memorySyncLogStoreimplementation. - Implemented bloc-like orchestration components (
SyncRepository,SyncController,OfflineSyncEngine) plus event/state/model types. - Exported the new API via the package entrypoint, documented it in
README.md, and added a focused unit test.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| test/bloc_like_architecture_test.dart | Adds a happy-path test for add + sync using the new bloc-like facade. |
| lib/src/bloc_like/states/sync_state.dart | Introduces state types for the controller/engine state stream. |
| lib/src/bloc_like/repository/sync_repository.dart | Implements local-first writes and sync-log replay to cloud. |
| lib/src/bloc_like/models/sync_log.dart | Adds SyncLog model plus operation/status enums. |
| lib/src/bloc_like/events/sync_event.dart | Defines event types for the bloc-like workflow. |
| lib/src/bloc_like/engine/offline_sync_engine.dart | Adds the facade wrapper that forwards calls to SyncController. |
| lib/src/bloc_like/controller/sync_controller.dart | Adds event handler and state stream management. |
| lib/src/bloc_like/contracts/sync_log_store.dart | Defines persistence contract for sync logs. |
| lib/src/bloc_like/contracts/local_data_source.dart | Defines local persistence contract. |
| lib/src/bloc_like/contracts/in_memory_sync_log_store.dart | Provides an in-memory SyncLogStore for onboarding/tests. |
| lib/src/bloc_like/contracts/cloud_data_source.dart | Defines cloud adapter contract. |
| lib/offline_sync_engine.dart | Exports the new bloc-like API from the package entrypoint. |
| README.md | Documents the new bloc-like architecture and wiring approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String _logId() { | ||
| final micros = DateTime.now().microsecondsSinceEpoch; | ||
| return 'sync_log_$micros'; | ||
| } |
There was a problem hiding this comment.
_logId() uses only microsecondsSinceEpoch, which can collide if multiple logs are created within the same microsecond (especially in tight loops/tests). Consider adding a per-instance counter (similar to SyncManager._generateOpId()), or using a UUID, to guarantee uniqueness.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| Future<void> add(T data) async { | ||
| final id = idResolver(data); | ||
| await local.insert(data); |
There was a problem hiding this comment.
This new public API doesn't validate the entity ID returned by idResolver(data). If it returns an empty/whitespace-only string, you'll enqueue logs with an invalid entityId. Existing public APIs (e.g., SyncManager) throw ArgumentError.value for empty identifiers; consider adding similar trim().isEmpty checks here.
| _emit(const SyncSuccess()); | ||
| } catch (e) { | ||
| _emit(SyncFailure(e.toString())); | ||
| rethrow; |
There was a problem hiding this comment.
The controller emits SyncFailure and then rethrows the exception. The README wiring example uses await engine.sync() without a try/catch and suggests state-driven handling; rethrowing will surface exceptions to callers and can crash apps if unhandled. Consider not rethrowing (state-only error reporting) or explicitly documenting that callers must handle exceptions.
| rethrow; |
| final StreamController<SyncState> _states = | ||
| StreamController<SyncState>.broadcast(); | ||
|
|
||
| SyncState _current = const SyncInitial(); | ||
|
|
||
| SyncController({required this.repository}) { | ||
| _states.add(_current); |
There was a problem hiding this comment.
_states is a broadcast stream and the initial SyncInitial is added in the constructor. Any listener that subscribes after construction will not receive that initial state (broadcast streams don’t replay). If consumers are expected to observe the initial state via states, consider emitting the current state in onListen (or using a BehaviorSubject-style controller).
| final StreamController<SyncState> _states = | |
| StreamController<SyncState>.broadcast(); | |
| SyncState _current = const SyncInitial(); | |
| SyncController({required this.repository}) { | |
| _states.add(_current); | |
| late final StreamController<SyncState> _states; | |
| SyncState _current = const SyncInitial(); | |
| SyncController({required this.repository}) { | |
| _states = StreamController<SyncState>.broadcast( | |
| onListen: () { | |
| _states.add(_current); | |
| }, | |
| ); |
|
|
||
| Future<void> _syncLogs(List<SyncLog> logs) async { | ||
| final sorted = [...logs] | ||
| ..sort((a, b) => b.timestamp.compareTo(a.timestamp)); |
There was a problem hiding this comment.
Sync is replaying logs in latest-first order. This can cause an update to run before a prior create for the same entity, which will fail for adapters where CloudDataSource.update() is not an upsert. Consider replaying in chronological order, or coalescing pending logs per entityId into a single effective operation (e.g., keep only the newest) before executing.
| ..sort((a, b) => b.timestamp.compareTo(a.timestamp)); | |
| ..sort((a, b) => a.timestamp.compareTo(b.timestamp)); |
|
@Harsh4114 I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Harsh4114 I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Harsh4114 I've opened a new pull request, #9, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@Harsh4114 I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you. |
[WIP] Update pull request to address feedback on Bloc-like architecture
Motivation
SyncManagerAPI without changing existing engine internals.Description
LocalDataSource<T>,CloudDataSource<T>, andSyncLogStoreplus anInMemorySyncLogStorefor quick onboarding underlib/src/bloc_like/contracts/.SyncLog,SyncOperationType, andSyncStatusunderlib/src/bloc_like/models/and event/state types inlib/src/bloc_like/events/andlib/src/bloc_like/states/.SyncRepository<T>(local-first writes + latest-first replay, status updates) and orchestration layersSyncController<T>andOfflineSyncEngine<T>underlib/src/bloc_like/to provide event-driven sync flows.lib/offline_sync_engine.dart), updatesREADME.mdwith a guide and wiring example, and addstest/bloc_like_architecture_test.dartvalidating an add+sync scenario.Testing
git diff --checksucceeded.dart formatanddart testbut both could not run in this environment because thedarttool is not available (format/test skipped).test/bloc_like_architecture_test.dartwhich exercisesSyncRepository→SyncController→OfflineSyncEngine(test file committed).Codex Task