fix: ParseLiveList.getAt() causes unnecessary requests to server
#1099
Conversation
|
🚀 Thanks for opening this pull request! |
|
Warning Rate limit exceeded@kirkmorrow has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces per-call async* element streams with persistent per-element broadcast streams via getAt(index); adds on-demand single-element loader _loadElementAt(index) with isLoading/loaded guards and per-element error propagation; refines lazy-loading field-restriction logic and adds one-time _debugLoggedInit init logging. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1099 +/- ##
==========================================
+ Coverage 43.96% 44.01% +0.05%
==========================================
Files 61 61
Lines 3596 3637 +41
==========================================
+ Hits 1581 1601 +20
- Misses 2015 2036 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/dart/lib/src/utils/parse_live_list.dart (2)
549-577: Verify list index safety after async boundary.The implementation correctly re-checks bounds after the async query (lines 571-577), but the element reference captured at line 554 may become stale if the list is modified during the query. The current code correctly uses
_list[index]at line 583 for the update, but consider also verifying the objectId still matches to ensure we're updating the correct element:if (response.success && response.results != null && response.results!.isNotEmpty) { + // Verify we're still updating the same object + final currentElement = _list[index]; + if (currentElement.object.objectId != element.object.objectId) { + if (_debug) { + print('ParseLiveList: Element at index $index changed during load'); + } + return; + } // Setting the object will mark it as loaded and emit it to the stream _list[index].object = response.results!.first;
579-592: Consider handling the case when response has no results but no error.If
response.successis true butresponse.resultsis empty (object was deleted between initial query and load), the element remains in an unloaded state with no notification to listeners:if (response.success && response.results != null && response.results!.isNotEmpty) { // Setting the object will mark it as loaded and emit it to the stream _list[index].object = response.results!.first; } else if (response.error != null) { // Emit error to the element's stream so listeners can handle it element.emitError(response.error!, StackTrace.current); if (_debug) { print( 'ParseLiveList: Error loading element at index $index: ${response.error}', ); } + } else { + // Object not found (possibly deleted) - emit an error or handle gracefully + if (_debug) { + print( + 'ParseLiveList: Element at index $index not found during load', + ); + } }packages/dart/test/src/utils/parse_live_list_test.dart (3)
145-158: Test validates query behavior but doesn't call the actual_runQuery()method.This test creates a fresh QueryBuilder and checks that it doesn't have 'keys' by default, but it doesn't actually test
_runQuery()behavior. Consider adding a comment clarifying this tests the expected baseline, not the actual implementation:test('lazyLoading=false should NOT restrict fields automatically', () { - // When lazy loading is disabled, fetch all fields + // Verifies baseline: a fresh QueryBuilder has no 'keys' restriction. + // The actual _runQuery() behavior with lazyLoading=false is tested + // indirectly through the loaded flag tests above. final query = QueryBuilder<ParseObject>(ParseObject('Room'))
160-175: Test manually applieskeysToReturn()rather than testing_runQuery()directly.The test simulates what
_runQuery()does by manually callingkeysToReturn(). While this verifies the QueryBuilder API works, it doesn't test thatParseLiveList._runQuery()actually applies this restriction. Consider adding a comment to clarify:test('lazyLoading=true with preloadedColumns should restrict fields', () { - // When lazy loading with preloaded columns, the initial query should - // only fetch those columns + // Verifies that keysToReturn() sets the 'keys' limiter as expected. + // Note: This simulates _runQuery() behavior; actual integration testing + // would require mocking the network layer. final query = QueryBuilder<ParseObject>(ParseObject('Room'))
52-56: Consider adding teardown to dispose of Parse resources.If
initializeParse()creates any persistent resources (singletons, connections), consider adding atearDownAllto clean up:setUpAll(() async { await initializeParse(); }); + + // Optional: Add teardown if initializeParse() creates persistent resources + // tearDownAll(() async { + // // Cleanup if needed + // });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dart/lib/src/utils/parse_live_list.dart(7 hunks)packages/dart/test/src/utils/parse_live_list_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu
🔇 Additional comments (9)
packages/dart/lib/src/utils/parse_live_list.dart (5)
14-27: Documentation is clear and accurate.The docstring clearly explains the lazy loading behavior and the relationship between
lazyLoadingandpreloadedColumnsparameters, which aligns with the implementation.
154-173: Field restriction logic correctly fixes the N+1 query root cause.The updated condition
_lazyLoading && _preloadedColumns.isNotEmptyproperly ensures fields are only restricted when both lazy loading is enabled AND preloaded columns are specified. This prevents unnecessary per-object queries whenpreloadedColumnsis empty.
183-200: ConsistentfieldsRestrictedcalculation aligns with_runQuery()logic.The
loadedflag is correctly set based on whether fields were actually restricted, ensuring elements are marked as loaded when all fields were fetched upfront.
526-543: The broadcast stream approach correctly eliminates the N+1 query bug.Returning
element.stream(fromStreamController<T>.broadcast()) instead of using anasync*generator ensures multiple subscriptions share the same stream instance and don't trigger duplicate loads. This is the core fix for issue #787.
878-884: Error propagation implementation is correct.The
emitError()method properly checks if the stream is closed before adding errors, preventing exceptions from being thrown on disposed elements.packages/dart/test/src/utils/parse_live_list_test.dart (4)
8-51: Excellent architectural documentation in test file.The detailed documentation explaining the stream implementation, broadcast stream benefits, and testing limitations provides valuable context for future maintainers. This is especially important given that the core architecture cannot be fully verified through unit tests alone.
58-82: Test correctly validates element loaded state forlazyLoading=false.The test verifies that elements are marked as loaded when lazy loading is disabled, which prevents unnecessary
_loadElementAt()calls.
178-262: Comprehensive demonstration of async vs broadcast stream behavior.*These tests effectively illustrate the core problem (async* creating new streams) and the solution (broadcast streams allowing multiple listeners). This serves as both documentation and regression protection.
264-335: Test clearly demonstrates the N+1 query root cause and fix.The
generatorCallCountincrement pattern clearly shows how async* generators execute their body on each subscription, while broadcast streams share initialization. This test serves as excellent documentation for the fix.
There was a problem hiding this comment.
Pull request overview
This PR fixes an N+1 query bug in ParseLiveList.getAt() where multiple subscriptions to the same element triggered duplicate network requests. The solution replaces the async* generator pattern with a broadcast stream architecture, ensuring that each element maintains a single persistent stream that all subscribers share.
Key Changes:
- Refactored
getAt()from async* generator to method returning broadcast stream - Added
_loadElementAt()method for on-demand lazy loading with proper error handling - Updated lazy loading logic to only restrict fields when both
lazyLoading=trueANDpreloadedColumnsis non-empty
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| packages/dart/lib/src/utils/parse_live_list.dart | Refactored getAt() to return broadcast stream; added _loadElementAt() for on-demand loading; updated lazy loading field restriction logic; added emitError() method to ParseLiveListElement |
| packages/dart/test/src/utils/parse_live_list_test.dart | Added comprehensive documentation on broadcast stream architecture; added 8 tests for lazy loading scenarios; added 3 tests demonstrating async* vs broadcast stream behavior patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Documentation and formatting fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/dart/lib/src/utils/parse_live_list.dart (2)
523-538: Optional: treat negative indices as out-of-bounds ingetAtRight now
getAttreatsindex >= _list.lengthas out-of-bounds (returningStream.empty()), but a negativeindexwill still hit_list[index]and throw:if (index >= _list.length) { return const Stream.empty(); } final element = _list[index];If you want
getAtto be more defensive and symmetrical, you could also treat negative indices as out-of-bounds:- if (index >= _list.length) { + if (index < 0 || index >= _list.length) { // Return an empty stream for out-of-bounds indices return const Stream.empty(); }Not strictly required, but it makes the API more forgiving.
768-769: Element-level loading state and error propagation are well-structured; minor encapsulation idea
- Initializing
_isLoadinginParseLiveListElementand guarding_loadElementAt()withloaded || _isLoadingcorrectly prevents concurrent loads while still allowing retries on failure.emitError()cleanly centralizes error delivery via the per-element broadcast stream and is used consistently from_loadElementAt().If you’d like to tighten encapsulation a bit, consider exposing
bool get isLoading/set isLoading(...)onParseLiveListElementand using that fromParseLiveListinstead of touchingelement._isLoadingdirectly. This keeps the element’s internal state changeable without affecting callers, but is purely optional given the single-library scope.Also applies to: 781-782, 910-916
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dart/lib/src/utils/parse_live_list.dart(10 hunks)packages/dart/test/src/utils/parse_live_list_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dart/test/src/utils/parse_live_list_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Flutter 3.38, Ubuntu
🔇 Additional comments (2)
packages/dart/lib/src/utils/parse_live_list.dart (2)
15-28: Lazy-loading andpreloadedColumnsdocumentation matches implementationThe new constructor docs for
lazyLoading/preloadedColumnsalign with the logic in_runQuery()and_init()(restricting fields only whenlazyLoading == trueandpreloadedColumns.isNotEmpty). No behavioral issues here.Also applies to: 34-45
12-13: One-time init logging and field restriction behavior look solid
_debugLoggedInitcorrectly ensures the initialization message is printed only on the first_runQuery()call, avoiding log spam while leaving_debug-guarded error logging untouched.- Field restriction is now applied only when
lazyLoadingis enabled andpreloadedColumnsis non-empty, andfieldsRestrictedis used to mark elements as loaded vs. not loaded, which cleanly reflects whether full objects were fetched.No changes needed here.
Also applies to: 63-65, 152-182, 191-205
Tighten encapsulation on ParseLiveListElement Improve comments
mtrezza
changed the title
ParseLiveList.getAt() causes unnecessary requests to the server
mtrezza
changed the title
ParseLiveList.getAt() causes unnecessary requests to the serverParseLiveList.getAt() causes unnecessary requests to server
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dart/lib/src/utils/parse_live_list.dart(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter 3.38, macOS
🔇 Additional comments (3)
packages/dart/lib/src/utils/parse_live_list.dart (3)
10-45: Constructor, docs, and debug init align with lazy-loading behaviorThe constructor wiring of
_lazyLoadingand_preloadedColumns, the added documentation onlazyLoading/preloadedColumns, and the introduction of_debugLoggedInitfor one-time initialization logging all look consistent and help clarify behavior without changing the external API.Also applies to: 57-57, 63-65
153-183: Field restriction andloadedflag correctly mirrorlazyLoading+preloadedColumnscontract
_runQuery()only applieskeysToReturnwhenlazyLoadingis enabled andpreloadedColumnsis non-empty, and_init()uses the same condition viafieldsRestrictedto decide the initialloadedstate for each element. This keeps the runtime behavior aligned with the documented semantics (lazy loading only when fields are actually restricted).Also applies to: 192-206
783-803: Element-level_isLoadingandemitErrorintegration looks correctInitializing
_isLoadinginParseLiveListElement, exposingisLoadingvia getter/setter, and using it from_loadElementAtprovides the intended race protection, whileemitError()cleanly surfaces load failures through the element’s broadcast stream with a closed-stream guard. This matches the new lazy-loading design and avoids duplicate requests.Also applies to: 931-941
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dart/lib/src/utils/parse_live_list.dart (1)
251-307: New objects after LiveQuery reconnect are always markedloaded = false, causing unnecessary per-element loadsOn reconnect, new objects discovered in
newListare added via:for (int i = 0; i < newList.length; i++) { tasks.add(_objectAdded(newList[i], loaded: false)); }But
_runQuery()on reconnect already applies the same field-restriction logic as during_init(only restricting whenlazyLoading && _preloadedColumns.isNotEmpty). When fields are not restricted (e.g.,lazyLoading == falseorpreloadedColumnsis empty), these new elements are fully fetched, yet they are still markedloaded = false. That means subsequentgetAt(index)calls will invoke_loadElementAt(index)and perform an extralimit=1fetch per new element, partially reintroducing the N+1 pattern after reconnect.You already correctly compute
fieldsRestrictedin_init; mirroring that here would avoid unnecessary requests and keep semantics consistent across initial load and reconnect.Suggested refactor:
@@ if (event == LiveQueryClientEvent.connected) { _updateQueue.whenComplete(() async { List<Future<void>> tasks = <Future<void>>[]; final ParseResponse parseResponse = await _runQuery(); if (parseResponse.success) { final List<T> newList = parseResponse.results as List<T>? ?? <T>[]; + + // Determine if fields were actually restricted in the query, + // same logic as in _init(). + final bool fieldsRestricted = + _lazyLoading && _preloadedColumns.isNotEmpty; @@ for (int i = 0; i < newList.length; i++) { - tasks.add(_objectAdded(newList[i], loaded: false)); + // Mark as loaded when all fields were fetched; only treat as + // not loaded when fields are actually restricted. + tasks.add( + _objectAdded( + newList[i], + loaded: !fieldsRestricted, + ), + ); } }Optionally, you could extract
fieldsRestrictedinto a private getter (e.g.bool get _fieldsRestricted => _lazyLoading && _preloadedColumns.isNotEmpty;) and reuse it in both_runQuery/_initand this reconnect path to keep the logic centralized.
🧹 Nitpick comments (1)
packages/dart/lib/src/utils/parse_live_list.dart (1)
554-662: Single-flight_loadElementAtimplementation is robust; consider minor defensive improvementsThe loader now:
- Guards against out-of-range indices.
- Uses
isLoadingto prevent concurrent loads for the same element.- Verifies list bounds and element identity (via
objectId) after the async query before applying results or emitting errors.- Emits both
response.errorand thrown exceptions to the element’s stream, with debug logging, and always clearsisLoadingin afinallyblock.This is a solid, race-safe single-flight implementation.
Two minor, optional hardening ideas:
- Also guard against negative indices for future callers:
if (index < 0 || index >= _list.length) { return; }- If you want to avoid repeated “not found during load” requests when an object was deleted but no LiveQuery delete event arrives (edge case), you could mark the element as logically removed or set
loaded = trueand rely solely on LiveQuery to clear it later.These are not blockers; the current behavior is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dart/lib/src/utils/parse_live_list.dart(10 hunks)packages/dart/lib/src/utils/parse_utils.dart(0 hunks)
💤 Files with no reviewable changes (1)
- packages/dart/lib/src/utils/parse_utils.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Dart 3.10, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu
🔇 Additional comments (6)
packages/dart/lib/src/utils/parse_live_list.dart (6)
12-13: One-time debug init logging looks correctInitializing
_debugLoggedInitfromisDebugEnabled()and flipping it tofalseafter the first_runQuery()call achieves the “log once on init, but still honor_debugfor other logs” behavior described in the PR. No issues here.Also applies to: 63-65
15-28: Constructor documentation andpreloadedColumnsplumbing match runtime semanticsThe updated
createdocs clearly explain the interaction betweenlazyLoadingandpreloadedColumns, and thepreloadedColumnsparameter is correctly threaded into the private constructor. The behavior in_runQuery/_init(only restricting fields whenlazyLoading && preloadedColumns.isNotEmpty) aligns with this contract.Also applies to: 29-45
153-183: Field restriction & initialloadedstate logic is consistent with lazy-loading designRestricting keys only when
lazyLoading && _preloadedColumns.isNotEmptyand then computingfieldsRestrictedin_initto decide the initialloadedflag is a clean way to distinguish:
- “true lazy loading with preloaded columns” (elements start as not loaded), from
- “full fetch” modes (either
lazyLoading == falseor nopreloadedColumns, elements start loaded).Including order fields into
keysensures correct sorting even when restricting fields. This block looks sound.Also applies to: 192-206
524-552:getAtbroadcast stream design correctly avoids duplicate loads and per-subscriber queriesReturning the element’s broadcast
Stream<T>and lazily kicking off_loadElementAt(index)only when!element.loadedsolves the original async* problem:
- Multiple listeners share the same element stream, so they no longer trigger separate generator executions or redundant network calls.
- Out-of-bounds indices safely return an empty stream rather than throwing.
The implementation matches the intended behavior from the PR description.
792-799:ParseLiveListElementloading flags and broadcast stream setup align with new lazy-loading flowInitializing
_loadedfrom the constructor parameter andisLoading = false, plus exposing the broadcast_streamController, integrates cleanly withgetAt/_loadElementAt:
loadedis flipped totruevia theobjectsetter when a full object is assigned.isLoadingis managed solely by_loadElementAt, preventing concurrent loads.- The broadcast controller allows multiple
Streamsubscribers per element.No correctness issues spotted here.
Also applies to: 808-811
940-946:emitErrorprovides a clean error path for element streamsThe
emitErrorhelper cleanly encapsulates error propagation to listeners while respecting_streamController.isClosed. This is consistent with its use in_loadElementAtfor bothParseResponse.errorand caught exceptions.
|
The coverage gaps are primarily in:
The core functionality is verified by existing unit tests. The new code paths work correctly in production (all tests pass). Adding coverage for these paths would require substantial integration testing infrastructure that's beyond the scope of this bug fix. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kirkmorrow is this ready for merge? |
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>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/dart/lib/src/utils/parse_live_list.dart (2)
534-562: LGTM: Broadcast stream architecture correctly prevents N+1 queries.The method returns the element's persistent broadcast stream immediately, allowing multiple subscribers without triggering duplicate loads. The bounds checking prevents crashes on invalid indices.
However, the comment at line 554 mentions "loaded and isLoading flags" but the actual field is named
isLoadingwithout an underscore prefix (declared as public at line 825). This matches a past review comment about naming consistency.Consider updating the comment for consistency:
- // This will only happen once per element due to the loaded and isLoading flags + // This will only happen once per element due to the loaded and isLoading flags (element.loaded, element.isLoading)
804-825: LGTM: Loading state flag properly documented and initialized.The
isLoadingfield is now documented as requested in past reviews, and the initialization correctly defaults tofalse.One minor style note: the field is initialized both in the initializer list (line 810) and at the field declaration (line 825). While this works in Dart (field declaration initializer is used as default for the initializer list), the double initialization is redundant. Per Dart style, non-final fields are typically initialized at declaration or in the initializer list, not both.
Consider removing the redundant initialization:
ParseLiveListElement( this._object, { bool loaded = false, Map<String, dynamic>? updatedSubItems, }) : _loaded = loaded, - isLoading = false { + {The field declaration at line 825 (
bool isLoading = false;) already provides the default value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dart/lib/src/utils/parse_live_list.dart(13 hunks)packages/dart/test/src/utils/parse_live_list_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dart/test/src/utils/parse_live_list_test.dart
🔇 Additional comments (13)
packages/dart/lib/src/utils/parse_live_list.dart (13)
5-13: LGTM: Constructor correctly initializes debug logging flags.The
_debugLoggedInitflag is properly initialized and will control one-time initialization logging as described in the PR objectives.
15-28: LGTM: Documentation clearly explains lazy loading behavior.The updated documentation accurately describes when field restriction occurs (only when
lazyLoading == trueANDpreloadedColumnsis non-empty), which matches the implementation and resolves the original issue.
63-65: LGTM: Clean separation of one-time init logging from debug logging.The comment clearly explains the purpose and the implementation correctly prevents log spam while preserving error logs.
156-162: LGTM: One-time debug logging correctly implemented.The logging occurs only once during initialization and provides useful diagnostic information about lazy loading configuration.
164-183: LGTM: Field restriction logic correctly implements the fix.The dual condition check (
_lazyLoading && _preloadedColumns.isNotEmpty) ensures fields are only restricted when explicitly configured, preventing the N+1 query bug. The automatic inclusion of order fields and deduplication are good defensive practices.
193-210: LGTM: Initialization correctly sets loaded state based on field restriction.Elements are correctly marked as loaded when all fields were fetched, eliminating unnecessary lazy loads. This is a key part of the N+1 fix.
262-314: LGTM: Reconnection logic maintains consistency with initialization.The same field restriction logic is correctly applied when reconnecting, ensuring elements have the appropriate loaded state regardless of how they enter the list.
568-582: LGTM: Race protection correctly prevents concurrent loads.The dual guard (
element.loaded || element.isLoading) with thefinallyblock ensures only one load attempt occurs per element and that the loading flag is always cleared, even on errors.
584-614: LGTM: Success path correctly handles list mutations during async operations.The bounds and identity checks ensure loaded data is never assigned to the wrong element, even if the list is modified while the query is in flight.
615-632: LGTM: Error path correctly guards against misrouted errors.The identity check mirrors the success path, ensuring errors are emitted only to the element that initiated the load.
633-643: LGTM: Not-found case appropriately handled.The decision to allow retries without emitting errors is reasonable for transient issues, and the comment clearly explains the design rationale and reliance on LiveQuery for actual deletions.
644-676: LGTM: Exception handler comprehensively guards against crashes.The exception path mirrors the success and error paths with full bounds and identity checking, ensuring robust error handling even during list mutations.
954-960: LGTM: Error propagation method cleanly implemented.The
emitErrormethod provides a safe way to propagate load failures to all stream listeners, completing the broadcast-stream architecture. The closed-stream check prevents crashes.
@mtrezza I am now. I applied the Copilot nitpick fixes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kirkmorrow this can be merged? |
Yes, I'm happy now that @copilot is happy. |
# [dart-v9.4.7](dart-9.4.6...dart-9.4.7) (2025-12-05) ### Bug Fixes * `ParseLiveList.getAt()` causes unnecessary requests to server ([#1099](#1099)) ([9114d4a](9114d4a))
|
🎉 This change has been released in version dart-v9.4.7 |
3 participants
Pull Request
Issue
Closes: #787
Approach
Problem
ParseLiveList.getAt()used anasync*generator that created a new stream on every call. When multiple listeners subscribed to the same element, each subscription re-executed the function body, causing duplicate network requests. This resulted in an N+1 query problem where accessing a single element could trigger multiple identical database queries.As reported in #787, subscribing to the same element 17 times resulted in 17 separate network requests instead of 1.
Root Cause
The async* generator pattern (
Stream<T> getAt()) creates a fresh stream instance for each invocation. Every subscription to that stream re-executes the generator's body, including any database queries. This is fundamentally incompatible with the goal of sharing state across multiple listeners.Solution
Replaced the async* generator architecture with a broadcast stream pattern:
Broadcast Stream Architecture: Each
ParseLiveListElementnow maintains a singleStreamController<T>.broadcast()that persists for the element's lifetime. ThegetAt()method returns a reference to this existing stream rather than generating a new one.On-Demand Loading: Created
_loadElementAt()method that triggers data loading when an unloaded element is first accessed. This supports lazy loading while ensuring the load operation happens only once per element.Race Condition Protection: Added
_isLoadingflag to prevent concurrent load attempts when multiple subscribers access the same unloaded element simultaneously.Proper Lazy Loading Logic: Field restriction now only applies when
lazyLoading=trueANDpreloadedColumns.isNotEmpty. Empty preloadedColumns fetches all fields regardless of lazyLoading flag.Error Handling: Added
emitError()method to propagate query errors and exceptions through the element's stream to all listeners.Debug Logging: Implemented one-time initialization logging using
_debugLoggedInitflag to prevent log spam on repeated queries, while preserving error logging functionality.Key Changes
lib/src/utils/parse_live_list.dart:
getAt(): Changed from async* generator to method returningelement.stream(broadcast stream)_loadElementAt(): New method for on-demand data loading with race condition protection_runQuery(): Added one-time debug logging; field restriction logic updated_init(): CalculatesfieldsRestrictedbased on actual query behaviorParseLiveListElement: AddedStreamController<T>.broadcast(),_isLoadingflag, andemitError()method_debugLoggedInitvariable initialized alongside_debugfor one-time loggingtest/src/utils/parse_live_list_test.dart:
Backward Compatibility
No breaking changes. All existing functionality is preserved while eliminating redundant queries. The
getAt()method signature remains unchanged; only the internal implementation was modified.Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.