Sheet bugs fixes and code refactor#183
Conversation
- Implement `runFirestoreOperation` for robust error handling across all Firestore calls in `sheet_providers`. - Refine sheet list stream processing and subscription logic for better clarity. - Adjust `onTap` behavior in `SheetListItemWidget` to conditionally pop the context only when in compact mode. - Simplify empty state check in `SheetListWidget`.
WalkthroughCentralized Firestore error handling by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/sheet/providers/sheet_providers.dart (1)
79-171: Wrapping write ops withrunFirestoreOperationis a solid consistency improvement.
One exception:updateSheetShareInfouses raw'sharedBy'/'message'keys—prefer constants (e.g.,FirestoreFieldConstants.*) to avoid schema drift/typos and keep fields consistent across the app.
🧹 Nitpick comments (2)
lib/features/sheet/providers/sheet_providers.dart (2)
60-60: onDispose cancellation is the right place for cleanup.
One minor note:StreamSubscription.cancel()returns aFuture; if you’re intentionally not awaiting, considerunawaited(_subscription?.cancel());for clarity.
68-76:addSheet: early-return on null user is safe, but consider surfacing a failure.
Silently returning can hide UI/logic bugs (e.g., button appears to work but doesn’t). If your UX expects feedback, consider returning abool/Result or throwing a domain error thatrunFirestoreOperationcan render.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/sheet/providers/sheet_providers.dart(4 hunks)lib/features/sheet/widgets/sheet_list_item_widget.dart(3 hunks)lib/features/sheet/widgets/sheet_list_widget.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). (8)
- GitHub Check: ios
- GitHub Check: linux
- GitHub Check: format
- GitHub Check: android
- GitHub Check: Test zoe_native plugin
- GitHub Check: windows
- GitHub Check: test
- GitHub Check: macos
🔇 Additional comments (5)
lib/features/sheet/widgets/sheet_list_widget.dart (1)
35-35: Early-return for empty state is clean and equivalent.
No behavioral change; improves readability.lib/features/sheet/widgets/sheet_list_item_widget.dart (3)
14-18: MakingisCompactoptional w/ default is good API ergonomics.
Just ensure no callsites expected it to be required (should be caught at compile time).
31-34: Potential nav fragility:pop()thenpush()in the same callback.
Depending on route teardown timing, using the samecontextafterpop()can be flaky. Consider using a “replace” style nav for compact mode, or schedule thepush()after the pop (e.g., microtask), or navigate from a parent/root context.
90-90: Tightening_buildContentSectiontoSheetModelis a clear win.
Improves safety and readability.lib/features/sheet/providers/sheet_providers.dart (1)
54-58: Double-checkonErrorpath: throwing insiderunFirestoreOperationmay crash the stream.
IfrunFirestoreOperationrethrows (or returns aFuturehere and errors become unhandled), the provider may get into noisy crash/rebuild loops. Consider handling/logging without rethrowing from the listener, or ensure the wrapper definitively absorbs + reports errors safely in this context.
| _subscription = query.snapshots().listen( | ||
| (snapshot) { | ||
| state = snapshot.docs | ||
| .map((doc) => SheetModel.fromJson(doc.data())) | ||
| .toList(); | ||
| }, |
There was a problem hiding this comment.
High-risk: snapshot mapping may drop Firestore doc ID.
If SheetModel.fromJson(doc.data()) doesn’t reliably populate id (common if id isn’t stored as a field), sheet.id can become empty/wrong and break navigation/updates. Prefer injecting doc.id into the model mapping (or using a dedicated fromFirestore that takes DocumentSnapshot).
🤖 Prompt for AI Agents
In lib/features/sheet/providers/sheet_providers.dart around lines 48 to 53, the
snapshot mapping uses SheetModel.fromJson(doc.data()) which may drop the
Firestore document ID; update the mapping to ensure the doc.id is preserved by
either (a) passing a merged map into the constructor (e.g., final data =
{...doc.data(), 'id': doc.id} then call SheetModel.fromJson(data)), or (b) add a
SheetModel.fromFirestore(DocumentSnapshot doc) that sets id = doc.id and use
that here; adjust the state assignment to use the chosen approach so every
SheetModel includes the Firestore document ID.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/sheet/providers/sheet_providers.dart (1)
6-60: Stream mapping needs exception handling, and theonErrorhandler returns an unawaited future.The
listencallback can throw ifSheetModel.fromJsonencounters invalid data (e.g., malformed color, type mismatch). StreamonErrorwon't catch exceptions from the callback—they'll propagate and potentially crash the app. Additionally,runFirestoreOperationreturns aFuturethat is never awaited in theonErrorhandler, which can trigger lint warnings.The
doc.data()already contains theidfield (stored bytoJson()at line 125 ofsheet_model.dart), so no manualdoc.idinjection is needed.Wrap the mapping in a try-catch and mark the future as unawaited:
_subscription = query.snapshots().listen( - (snapshot) { - state = snapshot.docs - .map((doc) => SheetModel.fromJson(doc.data())) - .toList(); - }, - onError: (error, stackTrace) => runFirestoreOperation( - ref, - () => Error.throwWithStackTrace(error, stackTrace), - ), + (snapshot) { + try { + state = snapshot.docs + .map((doc) => SheetModel.fromJson(doc.data())) + .toList(); + } catch (error, stackTrace) { + unawaited(runFirestoreOperation( + ref, + () => Error.throwWithStackTrace(error, stackTrace), + )); + } + }, + onError: (error, stackTrace) { + unawaited(runFirestoreOperation( + ref, + () => Error.throwWithStackTrace(error, stackTrace), + )); + }, );Ensure
unawaitedis imported fromdart:async.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/sheet/providers/sheet_providers.dart(4 hunks)lib/features/sheet/widgets/sheet_list_item_widget.dart(3 hunks)lib/features/sheet/widgets/sheet_list_widget.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). (7)
- GitHub Check: ios
- GitHub Check: linux
- GitHub Check: android
- GitHub Check: Test zoe_native plugin
- GitHub Check: windows
- GitHub Check: test
- GitHub Check: macos
🔇 Additional comments (4)
lib/features/sheet/widgets/sheet_list_widget.dart (1)
35-35: Empty-state early return is fine (no behavior change).lib/features/sheet/widgets/sheet_list_item_widget.dart (2)
14-18: Constructor default forisCompactimproves call-site ergonomics.
90-90: Typing_buildContentSectionasSheetModelis a solid cleanup (dropsdynamic).lib/features/sheet/providers/sheet_providers.dart (1)
68-171: Centralizing Firestore writes viarunFirestoreOperationis a net win; verify the silentuserId == nullpath is intended.
Wrapping all CRUD calls should make failures more consistent. The only behavior change to double-check isaddSheet: it now returns early whenuserIdis null (Line 70). If callers expect an error state/UI feedback, consider surfacing a handled error (via the same error handler) rather than a no-op.
| onTap: () { | ||
| if (context.canPop()) context.pop(); | ||
| if (context.canPop() && isCompact) context.pop(); | ||
| context.push(AppRoutes.sheet.route.replaceAll(':sheetId', sheet.id)); | ||
| }, |
There was a problem hiding this comment.
Avoid using context for navigation after pop() (possible deactivated context).
onTap: () {
- if (context.canPop() && isCompact) context.pop();
- context.push(AppRoutes.sheet.route.replaceAll(':sheetId', sheet.id));
+ final router = GoRouter.of(context);
+ if (router.canPop() && isCompact) router.pop();
+ router.push(AppRoutes.sheet.route.replaceAll(':sheetId', sheet.id));
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onTap: () { | |
| if (context.canPop()) context.pop(); | |
| if (context.canPop() && isCompact) context.pop(); | |
| context.push(AppRoutes.sheet.route.replaceAll(':sheetId', sheet.id)); | |
| }, | |
| onTap: () { | |
| final router = GoRouter.of(context); | |
| if (router.canPop() && isCompact) router.pop(); | |
| router.push(AppRoutes.sheet.route.replaceAll(':sheetId', sheet.id)); | |
| }, |
🤖 Prompt for AI Agents
In lib/features/sheet/widgets/sheet_list_item_widget.dart around lines 31 to 34,
the code calls context.pop() and then immediately uses the same context to push
a new route which can fail because the context may be deactivated after pop; fix
by computing the target route string first, perform the pop if needed, and defer
the push to after the pop (e.g., schedule the push with Future.microtask or
WidgetsBinding.instance.addPostFrameCallback) so you don’t use the popped
context synchronously.
2 participants
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.