fix: guard unsafe .first access in realtime listeners and query results (#782) - #783
Open
dolliecoder wants to merge 2 commits into
Open
fix: guard unsafe .first access in realtime listeners and query results (#782)#783dolliecoder wants to merge 2 commits into
dolliecoder wants to merge 2 commits into
Conversation
…ealtime listeners and query results
Contributor
|
🎉 Welcome @dolliecoder!
We appreciate your contribution! 🚀 |
📝 WalkthroughWalkthroughAdded defensive guards across several controllers to avoid unsafe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/controllers/explore_story_controller.dart (1)
629-649:⚠️ Potential issue | 🟠 MajorOnly decrement story likes when a like document was actually removed.
When Line 640 path is hit (no like doc found), the code still decrements likes at Line 648. This can corrupt like counts in concurrent/unlike-twice scenarios.
Suggested fix
- if (userLikeDocuments.isNotEmpty) { + bool removedLike = false; + if (userLikeDocuments.isNotEmpty) { try { await databases.deleteDocument( databaseId: storyDatabaseId, collectionId: likeCollectionId, documentId: userLikeDocuments.first.$id, ); + removedLike = true; } on AppwriteException catch (e) { log('Failed to Unlike i.e delete Like Document: ${e.message}'); } } else { log('No like document found to delete'); } - try { - await databases.updateDocument( - databaseId: storyDatabaseId, - collectionId: storyCollectionId, - documentId: story.storyId, - data: {"likes": story.likesCount.value - 1}, - ); - } on AppwriteException catch (e) { - log("Failed to reduce one story like: ${e.message}"); - } + if (removedLike) { + try { + await databases.updateDocument( + databaseId: storyDatabaseId, + collectionId: storyCollectionId, + documentId: story.storyId, + data: {"likes": story.likesCount.value - 1}, + ); + } on AppwriteException catch (e) { + log("Failed to reduce one story like: ${e.message}"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/controllers/explore_story_controller.dart` around lines 629 - 649, The code currently always decrements likes via databases.updateDocument even when no like document was found; modify the flow so that the decrement only happens when a like document was actually removed: check userLikeDocuments.isNotEmpty and perform databases.deleteDocument, and only if that deletion completes successfully (no AppwriteException) call databases.updateDocument to decrement the story.likesCount for story.storyId; alternatively set a boolean (e.g., deletedLike) to true upon successful deletion and wrap the updateDocument call in an if (deletedLike) block to avoid decrementing on the "No like document found" or deletion failure paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/controllers/single_room_controller.dart`:
- Around line 248-250: Change getParticipantDocId's return type from
Future<String> to Future<String?> so it can legally return null when
participantDocsRef.documents.isEmpty; then update all call sites in
makeModerator, removeModerator, makeSpeaker, makeListener, and
kickOutParticipant to handle a nullable result by checking if participantDocId
== null and returning/aborting early (e.g., if (participantDocId == null)
return;) before using participantDocId.
In `@lib/controllers/upcomming_rooms_controller.dart`:
- Around line 119-124: The anonymous .then callback currently throws a generic
Exception when value.documents.isEmpty which escapes since only
AppwriteException is caught; instead, remove the throw and return a safe empty
value (e.g., null or an empty Map/Object matching the expected document type)
from the callback where value.documents.first is returned so callers can handle
an empty result; update the enclosing function's return type if necessary and
adjust callers to handle the null/empty response.
---
Outside diff comments:
In `@lib/controllers/explore_story_controller.dart`:
- Around line 629-649: The code currently always decrements likes via
databases.updateDocument even when no like document was found; modify the flow
so that the decrement only happens when a like document was actually removed:
check userLikeDocuments.isNotEmpty and perform databases.deleteDocument, and
only if that deletion completes successfully (no AppwriteException) call
databases.updateDocument to decrement the story.likesCount for story.storyId;
alternatively set a boolean (e.g., deletedLike) to true upon successful deletion
and wrap the updateDocument call in an if (deletedLike) block to avoid
decrementing on the "No like document found" or deletion failure paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/controllers/explore_story_controller.dartlib/controllers/friend_calling_controller.dartlib/controllers/live_chapter_controller.dartlib/controllers/pair_chat_controller.dartlib/controllers/room_chat_controller.dartlib/controllers/single_room_controller.dartlib/controllers/upcomming_rooms_controller.dart
…ntDocId safely (fixes AOSSIE-Org#782)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/controllers/single_room_controller.dart (1)
296-297: Consider extracting repeated nullable lookup guard into a helper.The null handling is correct, but this pattern is duplicated across five methods; extracting it will reduce drift risk.
Refactor sketch
+ Future<void> _withParticipantDocId( + Participant participant, + Future<void> Function(String participantDocId) action, + ) async { + final participantDocId = await getParticipantDocId(participant); + if (participantDocId == null) return; + await action(participantDocId); + } Future<void> makeModerator(Participant participant) async { - final participantDocId = await getParticipantDocId(participant); - if (participantDocId == null) return; - await updateParticipantDoc(participantDocId, { + await _withParticipantDocId(participant, (participantDocId) async { + await updateParticipantDoc(participantDocId, { "isSpeaker": true, "hasRequestedToBeSpeaker": false, "isModerator": true, - }); + }); + }); }Also applies to: 306-307, 341-342, 350-351, 359-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/controllers/single_room_controller.dart` around lines 296 - 297, Multiple methods in single_room_controller.dart repeatedly perform the nullable lookup pattern: calling getParticipantDocId(participant) and returning when null; extract this into a small helper (e.g., requireParticipantDocId or withParticipantDocId) that encapsulates the lookup and null-guard so callers pass the participant and either receive a non-null docId or execute a callback only when docId != null; update the five occurrences that call getParticipantDocId (the lines around the calls to getParticipantDocId in the methods that currently do "final participantDocId = await getParticipantDocId(participant); if (participantDocId == null) return;") to use the new helper to eliminate duplicated nullable-guard logic while preserving the original early-return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/controllers/single_room_controller.dart`:
- Around line 296-297: Multiple methods in single_room_controller.dart
repeatedly perform the nullable lookup pattern: calling
getParticipantDocId(participant) and returning when null; extract this into a
small helper (e.g., requireParticipantDocId or withParticipantDocId) that
encapsulates the lookup and null-guard so callers pass the participant and
either receive a non-null docId or execute a callback only when docId != null;
update the five occurrences that call getParticipantDocId (the lines around the
calls to getParticipantDocId in the methods that currently do "final
participantDocId = await getParticipantDocId(participant); if (participantDocId
== null) return;") to use the new helper to eliminate duplicated nullable-guard
logic while preserving the original early-return behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/controllers/single_room_controller.dartlib/controllers/upcomming_rooms_controller.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/controllers/upcomming_rooms_controller.dart
Author
|
@M4dhav please review the pr once |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
This PR prevents runtime crashes caused by unsafe .first access on potentially empty lists in realtime listeners and Appwrite query results.
Several controllers were directly accessing:
data.events.first
documents.first
Without checking if the list was empty. If the realtime event list or query result returned an empty list, this would throw:
Changes made
Added if (data.events.isEmpty) return; before accessing data.events.first
Added isEmpty checks before accessing documents.first
Added safe guards for query result handling to prevent crashes when no documents are returned
These changes ensure the app fails safely instead of crashing in edge cases such as empty realtime events or concurrent deletions.
Fixes #782
Type of change
Bug fix (non-breaking change which fixes an issue)
Checklist
My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
I have checked my code and corrected any misspellings
Summary by CodeRabbit