Conversation
✅ Deploy Preview for cv-mobile-app-web ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds offline reading support for Interactive Books: new IbOfflineService caches chapter markdown; IbPageViewModel integrates offline loading, disposal tracking, saves cached content, and exposes isOfflineAvailable; ib_page_view renders an "Offline Available" banner, updates scrolling/hero tags, defers showcase init, and adds dispose logic. Adds NetworkUtils.isOnline and dependency connectivity_plus. Changes sendResetPasswordInstructions to return non-nullable Future and always return true on successful POST. Updates forgot-password UI to surface server errors inline and show a success SnackBar. Adds localization keys (forgot_password_success in en/ar/hi and ib_page_offline_available) and updates .gitignore. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/viewmodels/cv_landing_viewmodel.dart (1)
41-52:⚠️ Potential issue | 🟠 MajorMake logout awaitable before resetting navigation.
onLogout()is declared asasync void, which prevents awaiting theGoogleSignIn.signOut()call. At line 95,onLogout()is called without await, so the subsequent navigation (line 101) and success UI (lines 102-105) run before sign-out completes. If sign-out fails, the exception goes unhandled, leaving the app in an inconsistent logged-out state.Change
onLogout()to returnFuture<void>and await it at the call site:Proposed fix
- void onLogout() async { + Future<void> onLogout() async { _storage.isLoggedIn = false; _storage.currentUser = null; _storage.token = null; _currentUser = null; // Perform google signout if auth type is google.. if (_storage.authType == AuthType.GOOGLE) { await _googleSignIn.signOut(); } notifyListeners(); }- onLogout(); + await onLogout();lib/ui/views/authentication/forgot_password_view.dart (1)
5-5:⚠️ Potential issue | 🟡 MinorRemove the unused
DialogServiceimport.The import is only referenced in a commented-out line (line 126), so it's not needed in the active code.
🧹 Nitpick comments (9)
lib/l10n/app_ar.arb (1)
45-45: Inconsistent key placement across locale files.The
forgot_password_successkey is placed afterhow_to_support(line 44) in the contributors section, but inapp_en.arbit's placed in the@forgot_password_viewsection (afterforgot_password_link). Consider moving this key to the@forgot_password_viewsection (around line 97) for consistency with other locale files.lib/viewmodels/authentication/forgot_password_viewmodel.dart (2)
24-27: Hardcoded error message should use localization.The error message "Instructions couldn't be sent! Invalid Email" is hardcoded. For consistency with the rest of the app and to support internationalization, consider using a localized string or a constant from your constants file.
35-42: Fix inconsistent indentation in catch block.The
setErrorMessageForcall and its arguments have inconsistent indentation compared to the rest of the codebase.🔧 Suggested formatting fix
catch(_){ setStateFor(SEND_RESET_INSTRUCTIONS, ViewState.Error); - setErrorMessageFor( - SEND_RESET_INSTRUCTIONS, - "Something went wrong", - ); - return false; + setErrorMessageFor( + SEND_RESET_INSTRUCTIONS, + "Something went wrong", + ); + return false; }lib/ui/views/authentication/forgot_password_view.dart (2)
41-55: Remove commented-out code.This commented-out code block appears to be the old implementation that has been replaced. It should be removed before merging to keep the codebase clean.
🧹 Proposed cleanup
- // Widget _buildEmailInput() { - // return CVTextField( - // label: AppLocalizations.of(context)!.forgot_password_email, - // type: TextInputType.emailAddress, - // validator: - // (value) => - // Validators.isEmailValid(value) - // ? null - // : AppLocalizations.of( - // context, - // )!.forgot_password_email_validation_error, - // onSaved: (value) => _email = value!.trim(), - // action: TextInputAction.done, - // ); - // } -
126-126: Remove commented-out code.This commented-out line should be removed along with the unused
DialogServiceimport if it's no longer needed.lib/services/ib_offline_service.dart (3)
18-19: Replaceprint()with proper logging.Using
print()directly is not recommended in Flutter. Consider usingdebugPrint()for debug-only logging, or a structured logging service for production code. This applies to both error handlers (lines 19 and 32).🔧 Suggested fix
+import 'package:flutter/foundation.dart'; import 'dart:io'; import 'package:path_provider/path_provider.dart';catch(e){ - print("Offline save error: $e"); + debugPrint("Offline save error: $e"); }
37-40: Remove commented-out code.The commented-out
isDownloadedmethod should be removed if it's not planned for immediate use. It can always be retrieved from version control if needed later.
6-10: The "/" replacement is appropriate for the API file paths being used.Chapter IDs in this codebase are file paths from the CircuitVerse API (e.g.,
'index.md','docs/binary-representation/index.md'), so the current sanitization replacing/with_is correct. Since these paths come from a controlled, Jekyll-based API with predictable naming conventions (alphanumeric, hyphens, underscores, dots), additional sanitization for characters like:,?,*, etc. is not necessary.If the source of chapter IDs ever changes or becomes less predictable, consider using a more comprehensive sanitization approach (e.g., a whitelist of allowed characters or a slug-generation library).
lib/services/API/users_api.dart (1)
211-214: Remove commented-out code.The commented-out lines should be removed before merging. The new implementation that always returns
trueon successful POST is cleaner and sufficient since errors are handled via exceptions.🧹 Proposed cleanup
try { - // var jsonResponse = await ApiUtils.post(uri, headers: headers, body: json); - // return jsonResponse['message'] is String; await ApiUtils.post(uri, headers: headers, body: json); return true;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52bec1ae-21d7-4c61-ad22-077ef90fcccc
📒 Files selected for processing (13)
.gitignorelib/l10n/app_ar.arblib/l10n/app_en.arblib/l10n/app_hi.arblib/services/API/users_api.dartlib/services/ib_offline_service.dartlib/ui/views/authentication/forgot_password_view.dartlib/ui/views/ib/ib_page_view.dartlib/utils/network_utils.dartlib/viewmodels/authentication/forgot_password_viewmodel.dartlib/viewmodels/cv_landing_viewmodel.dartlib/viewmodels/ib/ib_page_viewmodel.dartpubspec.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/viewmodels/ib/ib_page_viewmodel.dart (1)
65-74:⚠️ Potential issue | 🟠 MajorCache the full page snapshot, not just flattened markdown.
This stores only concatenated
IbMdtext and then rebuilds anIbPageDatawithouttableOfContents,chapterOfContents, or the realpageUrl. The offline screen will render markdown, but it loses TOC-driven UI and any inlinechapter_contentsblock, and same-page link handling no longer has the original page URL context.Also applies to: 87-92
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0c2131f-f002-4c89-963e-3fb98ef8eff7
📒 Files selected for processing (4)
lib/l10n/app_en.arblib/ui/views/ib/ib_page_view.dartlib/viewmodels/ib/ib_page_viewmodel.dartpubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pubspec.yaml
- lib/l10n/app_en.arb
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
lib/ui/views/ib/ib_page_view.dart (4)
87-88: Remove commented-out code.These lines are dead code. The
ShowCaseWidget.of(context)logic has been correctly moved todidChangeDependencies; the commented remnants add noise.🧹 Proposed cleanup
_ibFloatingButtonState = IbFloatingButtonState(); - // super.initState(); - // _showCaseWidgetState = ShowCaseWidget.of(context); _landingModel = context.read<IbLandingViewModel>();
485-489: Remove the commented-out dispose block.The active
dispose()is now at lines 76-81. Leaving the old implementation commented out adds clutter and confusion.🧹 Proposed cleanup
- // `@override` - // void dispose() { - // _hideButtonController.dispose(); - // super.dispose(); - // } - `@override` Widget build(BuildContext context) {
496-507: Clean up orphaned comments.Lines 496 and 500 are remnants of the old implementation. They should be removed for clarity.
🧹 Proposed cleanup
onModelReady: (model) { _model = model; - // model.fetchPageData(id: widget.chapter.id); WidgetsBinding.instance.addPostFrameCallback((_) async{ - if (!mounted) return; - await model.fetchPageData(id: widget.chapter.id); - // Future.delayed(const Duration(milliseconds: 300), () { - if (!mounted) return; - model.showCase( - _showCaseWidgetState, - widget.showCase, - widget.globalKeysMap, - ); - }); + if (!mounted) return; + await model.fetchPageData(id: widget.chapter.id); + if (!mounted) return; + model.showCase( + _showCaseWidgetState, + widget.showCase, + widget.globalKeysMap, + ); + }); },
526-543: Minor cleanup and optimization.
- Line 526: Remove the commented-out line.
- Line 538: Add
consttoTextStylefor a minor performance optimization.- Consider using theme-aware colors for better dark-mode support (optional).
🧹 Proposed cleanup
- // children: _buildPageContent(model.pageData), children: [ - if(model.isOfflineAvailable) + if (model.isOfflineAvailable) Container( margin: const EdgeInsets.only(bottom: 10), padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 4), decoration: BoxDecoration( color: Colors.green, borderRadius: BorderRadius.circular(5), ), - child:Text( + child: Text( AppLocalizations.of(context)!.ib_page_offline_available, - style: TextStyle(color: Colors.white, fontSize: 12), + style: const TextStyle(color: Colors.white, fontSize: 12), ), ), - ..._buildPageContent(model.pageData), ],lib/viewmodels/ib/ib_page_viewmodel.dart (2)
26-27: Consider using locator for service instantiation consistency.
_ibEngineServiceuseslocator<IbEngineService>()(line 40), but_offlineServiceis instantiated directly. Using the locator pattern forIbOfflineServicewould maintain consistency and improve testability by allowing dependency injection.♻️ Suggested refactor
First, register the service in your locator setup:
locator.registerLazySingleton<IbOfflineService>(() => IbOfflineService());Then update the viewmodel:
- final IbOfflineService _offlineService = IbOfflineService(); + final IbOfflineService _offlineService = locator<IbOfflineService>();
45-54: Remove commented-out legacy code.Version control preserves the history. Keeping the old implementation as comments adds noise and reduces readability.
♻️ Suggested fix
- // Future? fetchPageData({String id = 'index.md'}) async { - // try { - // _pageData = await _ibEngineService.getPageData(id: id); - - // setStateFor(IB_FETCH_PAGE_DATA, ViewState.Success); - // } on Failure catch (f) { - // setStateFor(IB_FETCH_PAGE_DATA, ViewState.Error); - // setErrorMessageFor(IB_FETCH_PAGE_DATA, f.message); - // } - // } -
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce2fff68-3790-44c1-8d47-45e1e4245149
📒 Files selected for processing (3)
lib/ui/views/ib/ib_page_view.dartlib/viewmodels/cv_landing_viewmodel.dartlib/viewmodels/ib/ib_page_viewmodel.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/viewmodels/cv_landing_viewmodel.dart
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/viewmodels/ib/ib_page_viewmodel.dart (3)
90-100:⚠️ Potential issue | 🟡 MinorAdd
disposedcheck after loading cached content.After
await _offlineService.loadChapter(), the method continues to mutate_pageDataandisOfflineAvailablewithout checking if the viewmodel was disposed during the await.🛡️ Proposed fix
final cachedMarkdown = await _offlineService.loadChapter(id); + if (disposed) return; if (cachedMarkdown != null) {
58-60:⚠️ Potential issue | 🟡 MinorMissing early return when
disposedis true.The
disposedcheck here only skips setting theBusystate but allows the method to continue executing the fetch and all subsequent operations. Add an early return to prevent unnecessary work and potential post-disposal state mutations.🛡️ Proposed fix
- if (!disposed) { - setStateFor(IB_FETCH_PAGE_DATA, ViewState.Busy); - } + if (disposed) return; + setStateFor(IB_FETCH_PAGE_DATA, ViewState.Busy);
63-70:⚠️ Potential issue | 🟡 MinorAdd
disposedcheck after async operation before accessing state.After
await _ibEngineService.getPageData(), ifdisposedbecame true during the await, the code continues to access_pageData!.contentand mutate state. This can cause issues if the widget tree was disposed while the request was in flight.🛡️ Proposed fix
_pageData = await _ibEngineService.getPageData(id: id); + if (disposed) return; String markdown = "";
🧹 Nitpick comments (2)
lib/viewmodels/ib/ib_page_viewmodel.dart (2)
26-26: Consider using the locator pattern forIbOfflineService.The
_ibEngineServiceon line 40 useslocator<IbEngineService>()for dependency injection, but_offlineServiceis directly instantiated. This inconsistency makesIbOfflineServiceharder to mock in unit tests.♻️ Suggested refactor
Register
IbOfflineServicein the locator and inject it here:- final IbOfflineService _offlineService = IbOfflineService(); + final IbOfflineService _offlineService = locator<IbOfflineService>();Then register it in your locator setup file.
45-54: Remove commented-out dead code.The old
fetchPageDataimplementation is preserved in git history. Remove this commented block to keep the codebase clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5e1b928-7a40-45bb-99be-4fb46b6c67cc
📒 Files selected for processing (1)
lib/viewmodels/ib/ib_page_viewmodel.dart
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/viewmodels/ib/ib_page_viewmodel.dart (2)
26-26: Inconsistent dependency injection pattern.
IbOfflineServiceis instantiated directly, whereasIbEngineServiceon line 40 is obtained vialocator<IbEngineService>(). Using direct instantiation makes unit testing harder since you cannot inject a mock. Consider registeringIbOfflineServicewith the service locator for consistency.♻️ Suggested refactor
- final IbOfflineService _offlineService = IbOfflineService(); + final IbOfflineService _offlineService = locator<IbOfflineService>();You would also need to register the service in your locator setup.
45-54: Remove commented-out dead code.The old
fetchPageDataimplementation should be deleted rather than left as comments. Version control preserves the history if you ever need to reference the original.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ebc6062-4ecf-4361-a3fe-2a9bc3335cfc
📒 Files selected for processing (1)
lib/viewmodels/ib/ib_page_viewmodel.dart
1 participant
Fixes #528
Describe the changes you have made in this PR :
This PR adds offline reading support for Interactive Book chapters in the mobile app
Features implemented:
Implementation details:
Added an
IbOfflineServiceto store chapter markdown locallyUpdated
IbPageViewModelto:Updated
IbPageViewto display an Offline Available badge when cached content existsBenefits:
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
Improvements
Localization
Chores