Fix interactive books page view #457
Conversation
WalkthroughReplaces in-page Markdown/HTML rendering with a WebView-centric rendering path: introduces a WebViewController and navigation delegate (including URL filtering and external-launch handling), injects CSS/JS (including a MutationObserver) to remove page chrome and disable comments, and moves TOC/scroll behavior to JavaScript-driven navigation. Adds WebView loading state variables (_isLoading, _currentUrl), a webView error path and retry UI, a timer-driven FAB visibility model, an IbInteractionChannel for host–WebView interactions, and lifecycle cleanup. Removes Markdown-specific builders, scroll controllers, and related rendering/dependency code. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 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: 3
🧹 Nitpick comments (3)
lib/ui/views/ib/ib_page_view.dart (3)
157-160: Multiple setTimeout calls indicate timing issues.Running
cleanPage()four times at different intervals (immediately, 50ms, 200ms, 500ms) suggests the code is working around unpredictable page load timing. This is fragile and may not work reliably across different network conditions or page complexities.Consider these alternatives:
- Use
DOMContentLoadedor framework-specific load events to trigger cleanup once- Combine with the
MutationObserver(already present at line 162) as the primary mechanism- Use
requestAnimationFramefor a single deferred cleanup after initial render
210-220: Consider adding error handling for JavaScript execution.The JavaScript-based scrolling is appropriate for WebView, but there's no error handling if
runJavaScriptfails. While the JS itself logs when an element isn't found, therunJavaScriptcall itself could fail.🔎 Optional: Add error handling
Future _scrollToWidget(String slug) async { final jsCode = ''' var element = document.getElementById('$slug'); if (element) { element.scrollIntoView({behavior: "smooth", block: "start", inline: "nearest"}); } else { console.log("Element with id $slug not found"); } '''; - await _webViewController.runJavaScript(jsCode); + try { + await _webViewController.runJavaScript(jsCode); + } catch (e) { + debugPrint('Failed to scroll to widget: $e'); + } }
443-462: Add error state handling for WebView failures.The current implementation shows a loading indicator when
model.pageData == nullor_isLoading == true, but doesn't handle WebView load failures reported throughonWebResourceError(line 172-174). Users will see an infinite loading indicator if the page fails to load.🔎 Suggested error state handling
Add an error state variable:
bool _isLoading = true; String? _currentUrl; String? _errorMessage;Update
onWebResourceError:onWebResourceError: (WebResourceError error) { debugPrint('WebView Error: ${error.description}'); if (mounted) { setState(() { _errorMessage = error.description; _isLoading = false; }); } },Update the build method to show error state:
if (_errorMessage != null) Center( child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ const Icon(Icons.error_outline, size: 48), const SizedBox(height: 16), Text('Failed to load page'), const SizedBox(height: 8), ElevatedButton( onPressed: () { setState(() { _errorMessage = null; _isLoading = true; }); _webViewController.reload(); }, child: const Text('Retry'), ), ], ), ) else if (model.pageData != null && !_isLoading) WebViewWidget(controller: _webViewController),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/ui/views/ib/ib_page_view.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T14:34:41.433Z
Learnt from: JatsuAkaYashvant
Repo: CircuitVerse/mobile-app PR: 421
File: lib/viewmodels/simulator/simulator_viewmodel.dart:244-244
Timestamp: 2025-08-12T14:34:41.433Z
Learning: In Flutter projects, `Uint8List` is re-exported by `flutter/services.dart`, so importing `dart:typed_data` directly is redundant when `flutter/services.dart` is already imported.
Applied to files:
lib/ui/views/ib/ib_page_view.dart
📚 Learning: 2025-08-12T14:34:41.433Z
Learnt from: JatsuAkaYashvant
Repo: CircuitVerse/mobile-app PR: 421
File: lib/viewmodels/simulator/simulator_viewmodel.dart:244-244
Timestamp: 2025-08-12T14:34:41.433Z
Learning: In Flutter projects, `Uint8List` is re-exported by `flutter/services.dart`, so importing `dart:typed_data` directly is redundant when `flutter/services.dart` is already imported. The Dart analyzer will confirm such imports as unnecessary.
Applied to files:
lib/ui/views/ib/ib_page_view.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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (5)
lib/ui/views/ib/ib_page_view.dart (5)
1-1: LGTM!The new imports are appropriate for the WebView-based implementation.
dart:asyncis needed for the Timer functionality, andwebview_flutteris the core dependency for rendering IB content in a WebView.Also applies to: 15-15
52-55: LGTM!State variable declarations are appropriate. The
late finalmodifier for_webViewControlleris correct since it's initialized once ininitState. Loading state and URL tracking variables properly support the WebView lifecycle.
198-208: LGTM!The auto-hide timer implementation is clean and correct. The timer is properly cancelled before creating a new one, preventing timer leaks. The 5-second duration provides good UX for the floating navigation buttons.
66-66: URL validation is in place for unrestricted JavaScript mode.The
JavaScriptMode.unrestrictedsetting is acceptable here because theonNavigationRequesthandler (lines 175-180) restricts WebView content toEnvironmentConfig.IB_BASE_URL(https://learn.circuitverse.orgby default). External URLs are opened in the system browser instead, preventing malicious script injection through user navigation.
192-196: No action required. WebViewController in webview_flutter 4.13.0 does not provide adispose()orclose()method. Resource cleanup is handled automatically by the WebViewWidget's lifecycle and garbage collection. While webview_flutter 4.13.0 has known issues on GitHub related to lifecycle and hot-restart behavior, these are not addressed by manual controller disposal (which the API does not support). If stability is a concern, consider upgrading to a newer version of webview_flutter where these issues may have been resolved.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/ui/views/ib/ib_page_view.dart (1)
78-87:_cleanPageJsconstant is still duplicated inline inonPageStarted.The cleanup function defined in
_cleanPageJs(lines 78-87) is duplicated verbatim in the inline script at lines 125-133. This partially addresses the earlier DRY concern but the duplication remains.🔎 Proposed fix to reuse the constant
Replace the inline duplicate in
onPageStartedwith the constant:await _webViewController.runJavaScript(''' (function () { // ---- Cleanup old observer if any ---- if (window.__ibMutationObserver) { try { window.__ibMutationObserver.disconnect(); } catch (e) {} window.__ibMutationObserver = null; } $_cleanPageJs // Initial cleanup passes cleanPage(); setTimeout(cleanPage, 50); setTimeout(cleanPage, 200); setTimeout(cleanPage, 500); // ---- Create observer ---- const observer = new MutationObserver(() => { cleanPage(); }); observer.observe(document.body, { childList: true, subtree: true, }); window.__ibMutationObserver = observer; // ---- Auto-disconnect after stabilization ---- setTimeout(() => { if (window.__ibMutationObserver) { window.__ibMutationObserver.disconnect(); window.__ibMutationObserver = null; } }, 2000); })(); ''');
🧹 Nitpick comments (1)
lib/ui/views/ib/ib_page_view.dart (1)
233-233: Prefer explicitFuture<void>return type.The method returns
Futurewithout a type parameter. UsingFuture<void>is more explicit and follows Dart best practices.-Future _scrollToWidget(String slug) async { +Future<void> _scrollToWidget(String slug) async {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/ui/views/ib/ib_page_view.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T14:34:41.433Z
Learnt from: JatsuAkaYashvant
Repo: CircuitVerse/mobile-app PR: 421
File: lib/viewmodels/simulator/simulator_viewmodel.dart:244-244
Timestamp: 2025-08-12T14:34:41.433Z
Learning: In Flutter projects, `Uint8List` is re-exported by `flutter/services.dart`, so importing `dart:typed_data` directly is redundant when `flutter/services.dart` is already imported.
Applied to files:
lib/ui/views/ib/ib_page_view.dart
📚 Learning: 2025-08-12T14:34:41.433Z
Learnt from: JatsuAkaYashvant
Repo: CircuitVerse/mobile-app PR: 421
File: lib/viewmodels/simulator/simulator_viewmodel.dart:244-244
Timestamp: 2025-08-12T14:34:41.433Z
Learning: In Flutter projects, `Uint8List` is re-exported by `flutter/services.dart`, so importing `dart:typed_data` directly is redundant when `flutter/services.dart` is already imported. The Dart analyzer will confirm such imports as unnecessary.
Applied to files:
lib/ui/views/ib/ib_page_view.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). (3)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (5)
lib/ui/views/ib/ib_page_view.dart (5)
1-46: LGTM!The imports are appropriate for the WebView integration, and the
enableWebViewflag provides a useful toggle for testing or fallback scenarios.
115-161: MutationObserver lifecycle is now properly managed.The implementation correctly disconnects any existing observer before creating a new one (lines 117-123) and auto-disconnects after 2 seconds of stabilization (lines 153-159). This addresses the previous memory leak concern.
213-223: LGTM!Timer-based FAB visibility with proper cancellation in
disposeis well implemented.
343-439: LGTM!The floating action button implementation with animated visibility and Showcase integration is clean. The ChangeNotifierProvider pattern for state management is appropriate.
444-459: URL loading correctly moved out of build method.The listener-based approach in
onModelReadyaddresses the previous concern about side effects in thebuildmethod. This ensuresloadRequestis only called whenpageDataactually changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/ui/views/ib/ib_page_view.dart (1)
114-160: JavaScript cleanup logic is still partially duplicated.The
cleanPagefunction inonPageStarted(lines 124-132) duplicates the logic in_cleanPageJsconstant (lines 79-86). Consider extracting the full injection script to a helper method for both callbacks.🔎 Proposed refactor to consolidate scripts
String get _fullCleanupScript => ''' (function() { // Cleanup old observer if any if (window.__ibMutationObserver) { try { window.__ibMutationObserver.disconnect(); } catch (e) {} window.__ibMutationObserver = null; } $_cleanPageJs // Initial cleanup passes cleanPage(); setTimeout(cleanPage, 50); setTimeout(cleanPage, 200); setTimeout(cleanPage, 500); // Create observer const observer = new MutationObserver(() => cleanPage()); observer.observe(document.body, { childList: true, subtree: true }); window.__ibMutationObserver = observer; // Auto-disconnect after stabilization setTimeout(() => { if (window.__ibMutationObserver) { window.__ibMutationObserver.disconnect(); window.__ibMutationObserver = null; } }, 2000); })(); ''';Then use
_fullCleanupScriptin bothonPageStartedandonPageFinished.Also applies to: 165-178
🧹 Nitpick comments (3)
lib/ui/views/ib/ib_page_view.dart (3)
94-94: Avoid usingcontextininitState.
ShowCaseWidget.of(context)is called here ininitState, but the widget tree may not be fully built yet. While this is also set indidChangeDependencies(line 207), the call here could be removed to avoid potential issues.🔎 Proposed fix
@override void initState() { _ibFloatingButtonState = IbFloatingButtonState(); super.initState(); _startHideTimer(); - _showCaseWidgetState = ShowCaseWidget.of(context); _webViewController =
520-520: Hardcoded text should use localization.The "Retry" button text is hardcoded in English. For consistency with the rest of the app, use
AppLocalizations.🔎 Proposed fix
ElevatedButton( onPressed: () { setState(() { _webViewError = null; _isLoading = true; }); if (_currentUrl != null) { _webViewController.loadRequest( Uri.parse(_currentUrl!), ); } else { _webViewController.reload(); } }, - child: const Text('Retry'), + child: Text(AppLocalizations.of(context)!.retry), ),You may need to add the
retrykey to your localization files if it doesn't exist.
506-518: Retry logic has a potential edge case.When
_currentUrlisnull, calling_webViewController.reload()may not work as expected if no URL was ever loaded. Consider falling back to the model's page URL instead.🔎 Proposed fix
onPressed: () { setState(() { _webViewError = null; _isLoading = true; }); - // If current url is available, load it, else reload - if (_currentUrl != null) { - _webViewController.loadRequest( - Uri.parse(_currentUrl!), - ); - } else { - _webViewController.reload(); - } + final urlToLoad = _currentUrl ?? model.pageData?.pageUrl; + if (urlToLoad != null) { + _webViewController.loadRequest(Uri.parse(urlToLoad)); + } },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/ui/views/ib/ib_page_view.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T14:34:41.433Z
Learnt from: JatsuAkaYashvant
Repo: CircuitVerse/mobile-app PR: 421
File: lib/viewmodels/simulator/simulator_viewmodel.dart:244-244
Timestamp: 2025-08-12T14:34:41.433Z
Learning: In Flutter projects, `Uint8List` is re-exported by `flutter/services.dart`, so importing `dart:typed_data` directly is redundant when `flutter/services.dart` is already imported.
Applied to files:
lib/ui/views/ib/ib_page_view.dart
📚 Learning: 2025-08-12T14:34:41.433Z
Learnt from: JatsuAkaYashvant
Repo: CircuitVerse/mobile-app PR: 421
File: lib/viewmodels/simulator/simulator_viewmodel.dart:244-244
Timestamp: 2025-08-12T14:34:41.433Z
Learning: In Flutter projects, `Uint8List` is re-exported by `flutter/services.dart`, so importing `dart:typed_data` directly is redundant when `flutter/services.dart` is already imported. The Dart analyzer will confirm such imports as unnecessary.
Applied to files:
lib/ui/views/ib/ib_page_view.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). (3)
- GitHub Check: Test & Analyze (ubuntu-latest)
- GitHub Check: Test & Analyze (windows-latest)
- GitHub Check: Test & Analyze (macos-latest)
🔇 Additional comments (6)
lib/ui/views/ib/ib_page_view.dart (6)
1-17: Imports are appropriate for the WebView-based implementation.The addition of
dart:asyncforTimerandwebview_flutterfor the WebView widget is correct for the new rendering approach.
46-57: State variables are well-structured for the WebView lifecycle.Good use of nullable
VoidCallback?for_modelListenerto enable proper cleanup indispose. The separation of loading, error, and URL state is clean.
211-219: Proper cleanup of listener and timer.The listener is correctly removed from the model before disposal, and the timer is canceled. This addresses the previous memory leak concern.
452-467: Model listener setup is correctly implemented.The listener is properly stored in
_modelListenerfor cleanup indispose, and the pattern for triggering page loads on model changes is well-structured.
351-447: Floating action buttons implementation is well-structured.Good use of
ChangeNotifierProvider.valuewithConsumerfor reactive visibility, and disablingonPressedwhen invisible prevents accidental taps during the fade animation.
241-251: No security vulnerability here. Theslugparameter is already sanitized byIbEngineService.getSlug()(lib/services/ib_engine_service.dart), which removes all special characters viareplaceAll(RegExp(r'[^\w\s-]+'), ''). The result contains only alphanumerics, hyphens, and underscores—characters safe for JavaScript interpolation. The proposed escaping is unnecessary.Likely an incorrect or invalid review comment.
|
@JatsuAkaYashvant , please review , the ci is failing because the flutter_webview cant be opened in tests, thats a flutter limitation. |
|
@ShashwatXD CI is critical and we can't merge this without fixing it. Is there any alternative gem which supports markdown rendering which is still maintained ? |
I did try finding a few but i thought this can make it a lot future proof, because we have a interactive webview in a few chapters, so this was the best possible, rest i will try finding a better solution and let you know. Also i have seen the simulator Page is also a web view page in this repo, that too doesnt have a test case, so i thought this could be acceptable. |
2 participants
Fixes #388
What this PR does
Earlier, IB pages were rendered using Flutter Markdown(depreciated) with native HTML transformations, which caused issues with interactive content like quizzes and embeds.
This PR switches the content rendering to WebView, ensuring correct HTML behavior while keeping the UX clean.
Key changes:
Video
Record_2026-01-01-02-20-00.mp4
Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.