fix: Interactive book and improvements#517
fix: Interactive book and improvements#517spc-28 wants to merge 4 commits intoCircuitVerse:masterfrom
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:
WalkthroughUpdates Android toolchain versions (compileSdk → 36, minSdk → 24, targetSdk → 36), Gradle wrapper and plugin versions, and pubspec dependency changes (replaces flutter_markdown with flutter_markdown_plus). Adds a MathJax block syntax and multiple markdown builder/syntax adjustments, tightens HTML/iframe parsing, introduces lazy-loading HTML interaction widgets with WebView height management and retry UI, removes CustomImageBuilder.onTapImage, adds two l10n keys with generated translations, adds devtools_options.yaml and .claude settings, and registers quill_native_bridge_windows for Windows. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 8
🧹 Nitpick comments (3)
pubspec.yaml (1)
54-54: Clarify the shared_preferences version change.The version was changed from
^2.5.4(allowing updates) to exact2.5.3(pinned, and a lower version). Was this intentional? If pinning is needed for compatibility, consider adding a comment explaining why.lib/ui/views/ib/syntaxes/ib_mathjax_block_syntax.dart (2)
14-15: Consider removing the case-insensitive\Begin{check.LaTeX/MathJax commands are case-sensitive, so
\Begin{would not be valid LaTeX. This check appears unnecessary unless there's specific content in the Interactive Book that uses this variant.🔧 Suggested simplification
- if (currentLine.trimLeft().startsWith(r'$\begin{') || - currentLine.trimLeft().startsWith(r'$\Begin{')) { + if (currentLine.trimLeft().startsWith(r'$\begin{')) {
24-35: Verify graceful handling of unclosed MathJax blocks.If the content has an unclosed
\begin{...}block (missing}$terminator), the parser will consume all remaining lines. This may be acceptable behavior, but consider whether a warning or truncation limit would be beneficial for debugging malformed content.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
android/app/build.gradleandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/settings.gradledevtools_options.yamll10n.yamllib/services/ib_engine_service.dartlib/ui/views/ib/builders/ib_chapter_contents_builder.dartlib/ui/views/ib/builders/ib_headings_builder.dartlib/ui/views/ib/builders/ib_highlight_builder.dartlib/ui/views/ib/builders/ib_interaction_builder.dartlib/ui/views/ib/builders/ib_mathjax_builder.dartlib/ui/views/ib/builders/ib_pop_quiz_builder.dartlib/ui/views/ib/builders/ib_subscript_builder.dartlib/ui/views/ib/builders/ib_superscript_builder.dartlib/ui/views/ib/builders/ib_webview_builder.dartlib/ui/views/ib/components/ib_pop_quiz.dartlib/ui/views/ib/ib_page_view.dartlib/ui/views/ib/syntaxes/ib_embed_syntax.dartlib/ui/views/ib/syntaxes/ib_filter_syntax.dartlib/ui/views/ib/syntaxes/ib_highlight_syntax.dartlib/ui/views/ib/syntaxes/ib_inline_html_syntax.dartlib/ui/views/ib/syntaxes/ib_liquid_syntax.dartlib/ui/views/ib/syntaxes/ib_mathjax_block_syntax.dartlib/ui/views/ib/syntaxes/ib_mathjax_syntax.dartlib/ui/views/ib/syntaxes/ib_md_tag_syntax.dartlib/viewmodels/ib/ib_page_viewmodel.dartpubspec.yamlwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugins.cmake
💤 Files with no reviewable changes (1)
- l10n.yaml
| id "com.android.application" version "8.9.1" apply false | ||
| id "org.jetbrains.kotlin.android" version "2.1.0" apply false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Flutter Kotlin 2.1.0 compatibility 2026
💡 Result:
Flutter doesn’t have a single “Kotlin compatibility” switch—what matters is whether your Android toolchain (AGP/Gradle/R8/D8) supports the Kotlin version you want.
Kotlin 2.1.0 (and Kotlin 2.1.x) on Android in 2026
- Kotlin 2.1 requires Android Gradle Plugin (AGP) ≥ 8.6 (and the matching bundled R8/D8). If your Flutter Android project uses AGP < 8.6, Kotlin 2.1 is not supported for that build setup. [1]
- Kotlin 2.1.0 was released Nov 27, 2024. [2]
- Kotlin 2.1.20 (2.1 line) is documented as compatible with Gradle 7.6.3 through 8.11 (and typically newer with possible warnings). [3]
Important 2026 caveat: AGP 9.0+ changes Kotlin handling
- AGP 9.0 enables “built-in Kotlin” by default and has a runtime dependency on Kotlin Gradle Plugin (KGP) 2.2.10, auto-upgrading lower KGP versions. So if you move to AGP 9.0, you generally won’t be “on Kotlin 2.1” anymore—you’ll effectively be on (at least) KGP 2.2.10. [4]
Practical takeaway
- If you specifically need Kotlin 2.1.x, target AGP 8.6–8.9 (not AGP 9.0+), and use a compatible Gradle version per Kotlin/AGP requirements. [1][3][4]
Sources:
[1] Android Developers — AGP/D8/R8 requirements for Kotlin versions (Kotlin 2.1 ⇒ AGP 8.6)
[2] Kotlin docs — Kotlin 2.1.0 release notes (release date)
[3] Kotlin docs — Kotlin 2.1.20 Gradle compatibility range
[4] Android Developers — AGP 9.0.0 notes (built-in Kotlin, depends on KGP 2.2.10)
🌐 Web query:
AGP 8.9.1 Flutter compatibility
💡 Result:
- Yes—Flutter can work with AGP 8.9.1, provided your Android project is using the newer Flutter/Gradle setup (the Flutter repo’s own “legacy_project” CI fixture was updated to AGP 8.9.1, along with Gradle 8.11.1 and Kotlin 2.1.0). (chromium.googlesource.com)
- Version pairing you must meet: AGP 8.9.x requires Gradle 8.11.1+ (per Google’s AGP↔Gradle compatibility table). (developer.android.com)
- If you hit “Plugin
com.android.applicationversion8.9.1not found” in a Flutter project: Flutter has a closed/solved issue where a newly-created project on Flutter 3.38.1 tried to use AGP 8.9.1 but Gradle couldn’t resolve it from configured repositories—this typically points to environment/network/proxy/firewall/AV interception, not Flutter incompatibility. (github.com) - If you’re upgrading specifically because dependencies demand it: some AndroidX artifacts (e.g., androidx.core 1.17.0) can require AGP 8.9.1+ (often seen in Flutter/FlutterFlow builds). (github.com)
Citations:
- 1: https://chromium.googlesource.com/external/github.com/flutter/packages/%2B/HEAD/.ci/legacy_project/README.md?utm_source=openai
- 2: https://developer.android.com/studio/releases/gradle-plugin?utm_source=openai
- 3: [Android] Build failed: Plugin 'com.android.application' version '8.9.1' not found after upgrading to Flutter 3.38.1 flutter/flutter#178903
- 4: Unable to Build AAB in FlutterFlow due to outdated Android Gradle Plugin (AGP) FlutterFlow/flutterflow-issues#6609
🌐 Web query:
Kotlin K2 compiler Flutter Android support
💡 Result:
Flutter’s Android build pipeline doesn’t “support K2” as a separate switch—it supports whatever Kotlin Gradle Plugin (KGP) your Android/Gradle setup uses. Since Kotlin 2.0+ enables the K2 compiler by default, using KGP 2.x effectively means you’re building with K2. [1], [2]
What this means in practice for Flutter Android:
- Flutter apps (regular
flutter createprojects): you can generally move to Kotlin 2.x/K2 as long as your AGP/Gradle/Kotlin versions are mutually compatible and your plugins don’t pin older Kotlin tooling. (Most “incompatible Kotlin metadata” errors are version mismatches across dependencies.) - Flutter modules / AAR builds (
flutter create -t module+flutter build aar): Flutter’s generated.androidtemplate has historically pinned older Kotlin/AGP versions, and there’s an open Flutter issue requesting official template support for Kotlin 1.9/2.0+ because this breaks real-world module/AAR workflows. [3] - AGP 9+ note: Flutter has a published migration note that AGP 9 changes Kotlin handling (“built-in Kotlin” becomes default) and requires manual migration steps; this is adjacent to (and can compound) Kotlin 2/K2 upgrades. [4]
Bottom line: Flutter can build Android with Kotlin 2.x (K2), but “officially smooth” support depends on the specific Flutter template/tooling path you use—and Flutter module templates are the most likely to hit blockers today. [3]
Sources:
[1] Kotlin K2 migration guide (Kotlin 2.0 enables K2 by default): https://kotlinlang.org/docs/k2-compiler-migration-guide.html
[2] Kotlin 2.0 compatibility guide (K2-related Gradle property removal and defaults): https://kotlinlang.org/docs/compatibility-guide-20.html
[3] Flutter issue: “Support for Kotlin 1.9 and 2.0 in flutter module template” (open): flutter/flutter#160922
[4] Flutter breaking change doc: migrate to AGP 9 (built-in Kotlin default, manual migration): https://docs.flutter.dev/release/breaking-changes/migrate-to-agp-9
Verify Gradle version and Flutter plugin compatibility with Kotlin 2.1.0 and K2 compiler.
The Kotlin 2.1.0 and AGP 8.9.1 pairing is compatible (Flutter's own CI uses this combination). However, ensure:
- Gradle is updated to 8.11.1+ (required by AGP 8.9.x)
- All Flutter plugins support the K2 compiler enabled by default in Kotlin 2.0+
- If building Flutter modules or AARs, test thoroughly as compatibility there is less established
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/ui/views/ib/ib_page_view.dart (1)
447-456:⚠️ Potential issue | 🟡 MinorHardcoded strings bypass localization.
Lines 448 (
'Failed to load page') and 455 ('Retry') introduce untranslated UI text. These should use localization keys for consistency with the rest of the app.Suggested fix using localization
Text( - 'Failed to load page', + AppLocalizations.of(context)!.ib_page_failed_to_load, style: Theme.of(context).textTheme.titleMedium, ), const SizedBox(height: 8), TextButton( onPressed: () => _model.fetchPageData(id: widget.chapter.id), - child: const Text('Retry'), + child: Text(AppLocalizations.of(context)!.common_retry), ),Note: You'll need to add these keys to the ARB localization files.
lib/services/ib_engine_service.dart (1)
171-177:⚠️ Potential issue | 🟡 MinorTOC/chapter labels can still become blank for whitespace-first
<li>nodes.Line 176 and Line 214 rely on first-node text;
trim()can return'', which bypasses??fallback. This can still emit empty labels.Suggested fix
+ String _firstNonEmptyNodeText(Element li) { + return li.nodes + .map((n) => (n.text ?? '').trim()) + .firstWhere((t) => t.isNotEmpty, orElse: () => li.text.trim()); + } + List<IbTocItem> _parseToc(Element element, {bool num = true}) { @@ - final nestedOl = li.getElementsByTagName('ol'); + Element? nestedOl; + for (final child in li.children) { + if (child.localName == 'ol') { + nestedOl = child; + break; + } + } if (nestedOl.isNotEmpty) { toc.add( IbTocItem( leading: '$eff_index.', - content: li.firstChild?.text?.trim() ?? li.text, - items: _parseToc(nestedOl.first, num: !num), + content: _firstNonEmptyNodeText(li), + items: _parseToc(nestedOl!, num: !num), ), ); } else { - toc.add(IbTocItem(leading: '$eff_index.', content: li.text)); + toc.add(IbTocItem(leading: '$eff_index.', content: _firstNonEmptyNodeText(li))); } @@ - content: root - ? (li.nodes.isNotEmpty - ? li.nodes[0].text?.trim() ?? li.text.trim() - : li.text.trim()) - : li.text.trim(), + content: root ? _firstNonEmptyNodeText(li) : li.text.trim(),Also applies to: 212-216
🧹 Nitpick comments (2)
lib/ui/views/ib/builders/ib_mathjax_builder.dart (1)
72-99: Consider adding width constraints to block math builder for consistency.The inline builder uses
LayoutBuilderto handle width constraints, butIbMathjaxBlockBuilderrelies solely onSingleChildScrollView. While this works, the block builder could behave unexpectedly in constrained parent layouts.Optional: Add LayoutBuilder for width constraint consistency
`@override` Widget visitElementAfterWithContext( BuildContext context, md.Element element, TextStyle? preferredStyle, TextStyle? parentStyle, ) { var content = _cleanTex(element.textContent.trim()); - return Padding( - padding: const EdgeInsets.symmetric(vertical: 8.0), - child: SingleChildScrollView( - scrollDirection: Axis.horizontal, - child: _buildMathTex( - content, - mathStyle: MathStyle.display, - textStyle: _safeMathStyle(context, preferredStyle), + return LayoutBuilder( + builder: (context, constraints) { + return Padding( + padding: const EdgeInsets.symmetric(vertical: 8.0), + child: SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: _buildMathTex( + content, + mathStyle: MathStyle.display, + textStyle: _safeMathStyle(context, preferredStyle), + ), + ), + ); + }, + ); - ), - ), - ); }lib/ui/views/ib/ib_page_view.dart (1)
520-533: Double-tap navigation may conflict with content interaction.The
onDoubleTapDowngesture could interfere with user interactions like double-tapping to select text, zoom images, or interact with embedded content. Consider whether this gesture should be applied more selectively or provide visual feedback.Consider alternatives:
- Apply double-tap only to non-interactive areas (e.g., margins)
- Add visual feedback (edge highlight) to indicate the gesture zones
- Use swipe gestures instead, which are more conventional for page navigation
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/services/ib_engine_service.dartlib/ui/views/ib/builders/ib_mathjax_builder.dartlib/ui/views/ib/builders/ib_webview_builder.dartlib/ui/views/ib/ib_page_view.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/views/ib/builders/ib_webview_builder.dart
| result = result.replaceAll( | ||
| 'display: none', | ||
| 'display: inherit', | ||
| ); |
There was a problem hiding this comment.
Avoid global display: none rewrites in interaction HTML.
Line 349 changes every hidden element to display: inherit, which can break interaction behavior beyond navigation arrows. Scope this to only the intended nav selector(s).
Safer direction
- result = result.replaceAll(
- 'display: none',
- 'display: inherit',
- );
+ // Avoid global display rewrites; patch only known navigation selectors.
+ result = result.replaceAllMapped(
+ RegExp(r'(#(?:prev|next|left|right)[^{}]*\{[^}]*display:\s*)none',
+ caseSensitive: false),
+ (m) => '${m[1]}inherit',
+ );📝 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.
| result = result.replaceAll( | |
| 'display: none', | |
| 'display: inherit', | |
| ); | |
| // Avoid global display rewrites; patch only known navigation selectors. | |
| result = result.replaceAllMapped( | |
| RegExp(r'(#(?:prev|next|left|right)[^{}]*\{[^}]*display:\s*)none', | |
| caseSensitive: false), | |
| (m) => '${m[1]}inherit', | |
| ); |
There was a problem hiding this comment.
Here review is considering button or other elements as HTML, but actually they are Flutter widgets, nothing to do with the HTML interaction
There was a problem hiding this comment.
@spc-28, thanks for the clarification — you're right that the navigation arrows are Flutter FAB widgets and have no connection to the HTML string returned by getHtmlInteraction. That part of my concern was misplaced, and I appreciate the correction.
That said, the concern about the global display: none → display: inherit replacement still applies to the WebView's HTML content itself (the circuit simulation/interaction HTML loaded from the Interactive Book). If the fetched interaction HTML contains any elements that are intentionally hidden (e.g., overlays, loading panels, error states, or conditional UI within the circuit simulator), this blanket replacement would force them all visible regardless of intent.
Could you confirm whether the replacement was added specifically to fix a known hidden element in the interaction HTML (e.g., #too_small or a specific simulator panel)? If so, a more targeted selector — similar to what's already done for #too_small just below this line — would be safer:
result = result.replaceAll(
RegExp(r'#too_small\s*\{[^}]*display:\s*block[^}]*\}'),
'#too_small { display: none !important; }',
);Applying the same targeted approach for the specific element(s) you intended to unhide would avoid any unintended side effects on other hidden components within the circuit simulation.
✏️ Learnings added
Learnt from: spc-28
Repo: CircuitVerse/mobile-app PR: 517
File: lib/services/ib_engine_service.dart:349-352
Timestamp: 2026-03-10T08:02:09.672Z
Learning: In CircuitVerse mobile-app, the navigation arrows (prev/next chapter) in the Interactive Book are Flutter FAB widgets, not HTML elements. They are rendered as Flutter overlays over the page content, not inside the WebView HTML returned by `getHtmlInteraction`. Do not conflate Flutter UI elements with HTML elements inside the WebView interaction content when reviewing `lib/services/ib_engine_service.dart`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SantamRC
Repo: CircuitVerse/mobile-app PR: 514
File: lib/ui/views/new_ib/components/new_ib_chapter_page.dart:680-767
Timestamp: 2026-03-08T14:31:18.647Z
Learning: In the CircuitVerse mobile-app repository, the comments section in `lib/ui/views/new_ib/components/new_ib_chapter_page.dart` (PR `#514`) uses hardcoded mock comments intentionally as a PoC placeholder. Real Disqus API integration for comments is planned for a future PR. Avoid flagging the mock data in `_buildCommentsList` and the no-op submit handler as issues.
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/ui/views/ib/ib_page_view.dart (1)
109-124:⚠️ Potential issue | 🟠 MajorReturn after launching an Interactive Book URL.
The
elsebranch at Lines 113-115 opens the IB URL, then falls through to the genericcanLaunchUrlStringblock and opens the same link again. That can trigger duplicate browser launches.Suggested fix
if (href.startsWith(EnvironmentConfig.IB_BASE_URL)) { final pageUrl = _model.pageData?.pageUrl; if (pageUrl != null && href.startsWith(pageUrl) && href.contains('#')) { return _scrollToWidget(href.split('#').last); } else { launchURL(href); + return; } }
♻️ Duplicate comments (2)
lib/services/ib_engine_service.dart (2)
171-177:⚠️ Potential issue | 🟠 MajorStill avoid first-node assumptions when extracting TOC titles.
Line 176 and Line 214 still read
firstChild/nodes[0]directly. If the first node is whitespace or a comment, the entry label becomes empty or falls back to the full nested text, so TOC/chapter items render with the wrong title.Suggested direction
- final nestedOl = li.getElementsByTagName('ol'); - if (nestedOl.isNotEmpty) { + Element? nestedOl; + for (final child in li.children) { + if (child.localName == 'ol') { + nestedOl = child; + break; + } + } + final title = li.nodes + .map((n) => n.text?.trim() ?? '') + .firstWhere((t) => t.isNotEmpty, orElse: () => li.text.trim()); + if (nestedOl != null) { toc.add( IbTocItem( leading: '$eff_index.', - content: li.firstChild?.text?.trim() ?? li.text, - items: _parseToc(nestedOl.first, num: !num), + content: title, + items: _parseToc(nestedOl, num: !num), ), ); } else { - toc.add(IbTocItem(leading: '$eff_index.', content: li.text)); + toc.add(IbTocItem(leading: '$eff_index.', content: title)); }+ final title = li.nodes + .map((n) => n.text?.trim() ?? '') + .firstWhere((t) => t.isNotEmpty, orElse: () => li.text.trim()); toc.add( IbTocItem( leading: '$eff_index.', - content: root - ? (li.nodes.isNotEmpty - ? li.nodes[0].text?.trim() ?? li.text.trim() - : li.text.trim()) - : li.text.trim(), + content: root ? title : li.text.trim(), items: sublist.isNotEmpty ? sublist : null, ), );Also applies to: 212-216
349-352:⚠️ Potential issue | 🟠 MajorDon’t flip every
display: nonetoinherit.This restores far more than the nav arrows. Hidden responsive fallbacks, overlays, and interaction state can all become visible, which is likely to break existing IB widgets. Scope the patch to the known navigation selectors instead.
Safer direction
- result = result.replaceAll( - 'display: none', - 'display: inherit', - ); + result = result.replaceAllMapped( + RegExp(r'(#(?:prev|next|left|right)[^{}]*\{[^}]*display:\s*)none', + caseSensitive: false), + (m) => '${m[1]}inherit', + );
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21c59534-f93c-44ee-8e42-317c51034507
📒 Files selected for processing (12)
.claude/settings.jsonlib/gen_l10n/app_localizations.dartlib/gen_l10n/app_localizations_ar.dartlib/gen_l10n/app_localizations_en.dartlib/gen_l10n/app_localizations_hi.dartlib/l10n/app_ar.arblib/l10n/app_en.arblib/l10n/app_hi.arblib/services/ib_engine_service.dartlib/ui/views/ib/builders/ib_interaction_builder.dartlib/ui/views/ib/ib_page_view.dartlib/ui/views/ib/syntaxes/ib_embed_syntax.dart
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ui/views/ib/syntaxes/ib_embed_syntax.dart
spc-28
force-pushed
the
interactive-book#388
branch
from
7f0e741 to
25639e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ui/views/ib/ib_page_view.dart (1)
180-188:⚠️ Potential issue | 🟠 MajorEnabling
IbMathjaxBlockSyntaxhere will break single-line$$...$$blocks.
lib/ui/views/ib/syntaxes/ib_mathjax_block_syntax.dart, Lines 27-54, does not handle a line that both opens and closes with$$. For$$x$$, it capturesx$$as content and then keeps consuming following lines until another closing fence or EOF, which will swallow the rest of the markdown once this syntax is registered here.💡 Guard the single-line case before the multiline loop
final trimmed = currentLine.trim(); if (trimmed.startsWith(r'$$') && trimmed.endsWith(r'$$') && trimmed.length > 4) { parser.advance(); return md.Element.text( 'mathjax_block', trimmed.substring(2, trimmed.length - 2).trim(), ); }
♻️ Duplicate comments (3)
lib/services/ib_engine_service.dart (2)
171-177:⚠️ Potential issue | 🟠 MajorUse the first non-empty node text here, not
??ontrim().At Line 176 and Lines 213-215, a whitespace-only first node yields
'', so the??fallback never runs. On pretty-printed HTML that leaves these TOC labels blank instead of using the actual<li>text. Please switch to “first non-empty text node, elseli.text.trim()” in both parsers.Suggested fix
- final nestedOl = li.getElementsByTagName('ol'); + final nestedOl = li.getElementsByTagName('ol'); + final title = li.nodes + .map((n) => n.text?.trim() ?? '') + .firstWhere((t) => t.isNotEmpty, orElse: () => li.text.trim()); if (nestedOl.isNotEmpty) { toc.add( IbTocItem( leading: '$eff_index.', - content: li.firstChild?.text?.trim() ?? li.text, + content: title, items: _parseToc(nestedOl.first, num: !num), ), ); } else { - toc.add(IbTocItem(leading: '$eff_index.', content: li.text)); + toc.add(IbTocItem(leading: '$eff_index.', content: title)); }+ final title = li.nodes + .map((n) => n.text?.trim() ?? '') + .firstWhere((t) => t.isNotEmpty, orElse: () => li.text.trim()); toc.add( IbTocItem( leading: '$eff_index.', - content: root - ? (li.nodes.isNotEmpty - ? li.nodes[0].text?.trim() ?? li.text.trim() - : li.text.trim()) - : li.text.trim(), + content: root ? title : li.text.trim(), items: sublist.isNotEmpty ? sublist : null, ), );Also applies to: 212-216
349-352:⚠️ Potential issue | 🟠 MajorScope this visibility patch to the intended WebView element.
This runs against the combined
module.js+ interaction HTML payload, so everydisplay: nonein the WebView document gets rewritten—not just the one hidden element you meant to fix. That can unhide unrelated simulator states or mutate matching CSS/JS literals in the embedded script. Please replace only the specific selector/rule you actually need to override.Based on learnings: In CircuitVerse mobile-app, the navigation arrows are Flutter FAB widgets, so this concern is only about the WebView HTML returned by
getHtmlInteraction.lib/ui/views/ib/builders/ib_interaction_builder.dart (1)
53-75:⚠️ Potential issue | 🟠 MajorAwait WebView setup before calling
loadHtmlString().The call order looks correct now, but
setJavaScriptMode,setNavigationDelegate, andloadHtmlStringare async inwebview_flutter. Cascading them still lets the HTML load start before the delegate is guaranteed to be attached, so the firstonPageFinishedcan be missed and the interaction height never updates from its placeholder value.
🧹 Nitpick comments (1)
lib/ui/views/ib/syntaxes/ib_embed_syntax.dart (1)
18-23: Extract the closing-tag RegExp to a static field for efficiency.A new
RegExpinstance is created on every loop iteration, which is wasteful. Extract it to a static field similar to_singleLinePattern.♻️ Proposed fix
+ static final _closeTagPattern = + RegExp(r'</iframe>', caseSensitive: false); + `@override` md.Node parse(md.BlockParser parser) { final firstLine = parser.current.content; ... while (!parser.isDone) { final line = parser.current.content; buffer.write('\n$line'); parser.advance(); - if (RegExp(r'</iframe>', caseSensitive: false).hasMatch(line)) break; + if (_closeTagPattern.hasMatch(line)) break; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db96fb91-cc25-41ab-af33-1068479be57e
📒 Files selected for processing (13)
.claude/settings.jsonREADME.mdlib/gen_l10n/app_localizations.dartlib/gen_l10n/app_localizations_ar.dartlib/gen_l10n/app_localizations_en.dartlib/gen_l10n/app_localizations_hi.dartlib/l10n/app_ar.arblib/l10n/app_en.arblib/l10n/app_hi.arblib/services/ib_engine_service.dartlib/ui/views/ib/builders/ib_interaction_builder.dartlib/ui/views/ib/ib_page_view.dartlib/ui/views/ib/syntaxes/ib_embed_syntax.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- .claude/settings.json
- lib/gen_l10n/app_localizations_hi.dart
- lib/l10n/app_hi.arb
- lib/gen_l10n/app_localizations_ar.dart
- lib/gen_l10n/app_localizations.dart
- lib/l10n/app_en.arb
| Text( | ||
| 'Tap to load interaction', | ||
| style: TextStyle( | ||
| color: Theme.of(context).colorScheme.primary, | ||
| fontWeight: FontWeight.w500, | ||
| ), |
There was a problem hiding this comment.
Localize the new interaction CTA.
Tap to load interaction is now user-visible on IB pages with interactions, but it is hardcoded in English. Please add an ARB key and read it from AppLocalizations so non-English locales do not regress to mixed-language UI.
spc-28
force-pushed
the
interactive-book#388
branch
from
25639e1 to
b023f90
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/ui/views/ib/builders/ib_interaction_builder.dart (1)
103-104:⚠️ Potential issue | 🟡 MinorLocalize user-visible strings.
'Tap to load interaction'(line 104),'Error Loading Interaction'(line 127), and'Retry'(line 130) are hardcoded in English. Add ARB keys and useAppLocalizationsfor consistency with the rest of the app.Suggested fix
Text( - 'Tap to load interaction', + AppLocalizations.of(context)!.ib_interaction_tap_to_load, style: TextStyle(-const Text('Error Loading Interaction'), +Text(AppLocalizations.of(context)!.ib_interaction_load_error), TextButton( onPressed: _loadInteraction, - child: const Text('Retry'), + child: Text(AppLocalizations.of(context)!.ib_page_retry), ),
🧹 Nitpick comments (2)
lib/services/ib_engine_service.dart (1)
258-286: Greedy regex may misbehave with nested or multiple same-type tags.The
dotAllregexes are non-nested and greedy. For content like<code>a</code> text <code>b</code>, the first match consumes up to the last</code>, producing`a</code> text <code>b`instead of`a` text `b`.Consider using non-greedy quantifiers:
Suggested fix
- RegExp(r'<code>(.*?)</code>', dotAll: true), + RegExp(r'<code>(.*?)</code>'),The
.*?is already non-greedy, butdotAll: trueallows.to match newlines. If you need newline support, ensure the content doesn't have multiple same-type tags spanning lines, or use a proper HTML parser for complex cases.lib/ui/views/ib/ib_page_view.dart (1)
521-534: Consider adding feedback for double-tap navigation.The double-tap gesture for page navigation is a nice UX improvement. However, without visual or haptic feedback, users may not realize when a tap was recognized versus when no navigation occurred (e.g., tapping left on the first page).
Optional: Add haptic feedback
onDoubleTapDown: (details) { final screenWidth = MediaQuery.of(context).size.width; final tapX = details.globalPosition.dx; if (tapX < screenWidth / 2) { if (widget.chapter.prev != null) { + HapticFeedback.lightImpact(); widget.setPage(widget.chapter.prev); } } else { if (widget.chapter.next != null) { + HapticFeedback.lightImpact(); widget.setPage(widget.chapter.next); } } },Requires
import 'package:flutter/services.dart';
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6adeb84a-d68f-4355-8d04-981a81f34251
📒 Files selected for processing (13)
.claude/settings.jsonREADME.mdlib/gen_l10n/app_localizations.dartlib/gen_l10n/app_localizations_ar.dartlib/gen_l10n/app_localizations_en.dartlib/gen_l10n/app_localizations_hi.dartlib/l10n/app_ar.arblib/l10n/app_en.arblib/l10n/app_hi.arblib/services/ib_engine_service.dartlib/ui/views/ib/builders/ib_interaction_builder.dartlib/ui/views/ib/ib_page_view.dartlib/ui/views/ib/syntaxes/ib_embed_syntax.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/l10n/app_ar.arb
- lib/gen_l10n/app_localizations_en.dart
- .claude/settings.json
spc-28
force-pushed
the
interactive-book#388
branch
from
b023f90 to
ab80102
Compare
1 participant
Reference #457
Describe the changes you have made in this PR -
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
Chores
Platform
Documentation