Proof of Concept - Interactive Book Improvement#514
Proof of Concept - Interactive Book Improvement#514SantamRC wants to merge 61 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:
WalkthroughAdds a new Interactive Book feature and related infrastructure: new UI (NewInteractiveBookView, landing/home/chapter pages, drawer and drawer components, content renderers, markdown parser, quiz and binary-simulator widgets), structured-JSON and legacy-markdown models and parsers, new NewIbApi and DisqusApi services (with Http implementations), an IbRecommendation model, viewmodels (NewIbLandingViewModel, IbPageViewModel JSON pathways and clearPageData), locator registrations for NewIbApi/DisqusApi/NewIbLandingViewModel, environment constants (IB_JSON_API_BASE_URL, DISQUS_API_KEY), routing and localization entries, devtools_options.yaml, and Android Gradle/tooling updates. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (7)
lib/ui/views/new_ib/components/quiz_widget.dart (3)
4-16: Consider making data classes immutable with const constructors.Flutter's
@immutableannotation andconstconstructors are recommended for data classes to enable compile-time optimizations and clarify intent.♻️ Suggested improvement
+import 'package:flutter/foundation.dart'; + +@immutable class QuizQuestion { final String question; final List<QuizAnswer> answers; - QuizQuestion({required this.question, required this.answers}); + const QuizQuestion({required this.question, required this.answers}); } +@immutable class QuizAnswer { final String text; final bool isCorrect; - QuizAnswer({required this.text, required this.isCorrect}); + const QuizAnswer({required this.text, required this.isCorrect}); }
114-122: Consider extracting hardcoded colors toIbTheme.The green (
0xFF5cd65c) and red (0xFFff6666) colors are hardcoded here while the rest of the widget usesIbTheme. Extracting these to theme constants would improve consistency and maintainability, especially if theme customization is needed later.
127-158: Add accessibility semantics for answer feedback.Screen readers won't announce whether a selected answer is correct or incorrect. Adding
Semanticswith appropriate labels improves accessibility for users with visual impairments.♿ Suggested accessibility improvement
return InkWell( onTap: () => _selectAnswer(questionIndex, answerIndex, isCorrect), borderRadius: BorderRadius.circular(8), + child: Semantics( + label: isSelected + ? '${answer.text}, ${isCorrect ? "correct" : "incorrect"}' + : answer.text, + button: true, child: Container( // ... existing container code ), + ), );lib/ui/views/new_ib/components/binary_simulator_widget.dart (2)
119-145: Consider adding accessibility semantics.The
InkWelllacks semantic labels for screen readers. Users relying on assistive technologies won't understand which bit position they're interacting with.♿ Suggested improvement
Widget _buildBitCell(BuildContext context, int index) { return Expanded( - child: InkWell( - onTap: () => _toggleBit(index), + child: Semantics( + label: 'Bit ${_bitValues[index]}, value ${_bits[index] ? 1 : 0}', + button: true, + child: InkWell( + onTap: () => _toggleBit(index), child: Container( // ... existing code ), + ), ), ); }
147-166: Unusedcontextparameter.The
contextparameter is passed but not used in this method. Either remove it or use it for theme colors (addressing the hardcoding issue mentioned above).♻️ If keeping hardcoded colors
- Widget _buildResultCell(BuildContext context) { + Widget _buildResultCell() {Then update the call site on line 57.
lib/viewmodels/ib/ib_page_viewmodel.dart (1)
34-43: Consider consolidating page-reset logic to one path.
fetchPageDataandclearPageDataboth clear_pageData. Reusing one helper keeps reset behavior consistent over time.lib/ui/views/new_ib/components/new_ib_content.dart (1)
51-136: Make placeholder scrollable to avoid overflow under accessibility text scaling.Line 54 uses a fixed-height
ColumninsideCenter; on smaller devices or larger font settings this can overflow.♻️ Suggested tweak
- return Center( - child: Padding( + return Center( + child: SingleChildScrollView( + child: Padding( padding: const EdgeInsets.all(32.0), child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ ... ], ), - ), + ), + ), );
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
lib/locator.dartlib/models/ib/ib_recommendation.dartlib/services/API/disqus_api.dartlib/ui/components/cv_drawer.dartlib/ui/views/new_ib/README.mdlib/ui/views/new_ib/components/binary_simulator_widget.dartlib/ui/views/new_ib/components/new_ib_chapter_page.dartlib/ui/views/new_ib/components/new_ib_content.dartlib/ui/views/new_ib/components/new_ib_drawer.dartlib/ui/views/new_ib/components/new_ib_drawer_footer.dartlib/ui/views/new_ib/components/new_ib_drawer_header.dartlib/ui/views/new_ib/components/new_ib_drawer_section.dartlib/ui/views/new_ib/components/new_ib_drawer_tile.dartlib/ui/views/new_ib/components/new_ib_expandable_chapter_tile.dartlib/ui/views/new_ib/components/new_ib_home_page.dartlib/ui/views/new_ib/components/new_ib_markdown_parser.dartlib/ui/views/new_ib/components/new_ib_simple_chapter_tile.dartlib/ui/views/new_ib/components/new_ib_sub_chapter_tile.dartlib/ui/views/new_ib/components/quiz_widget.dartlib/ui/views/new_ib/new_ib_landing_view.dartlib/utils/router.dartlib/viewmodels/ib/ib_page_viewmodel.dart
| final headers = | ||
| headerLine | ||
| .split('|') | ||
| .map((cell) => cell.trim()) | ||
| .where((cell) => cell.isNotEmpty) | ||
| .toList(); |
There was a problem hiding this comment.
Table parsing drops valid empty cells and can misalign columns.
At Lines 570 and 590, .where((cell) => cell.isNotEmpty) removes intentional empty cells, which shifts row values into wrong columns.
🛠️ Proposed fix
- final headers =
- headerLine
- .split('|')
- .map((cell) => cell.trim())
- .where((cell) => cell.isNotEmpty)
- .toList();
+ final headers = _splitTableRow(headerLine);
@@
- var cells =
- line
- .split('|')
- .map((cell) => cell.trim())
- .where((cell) => cell.isNotEmpty)
- .toList();
+ var cells = _splitTableRow(line);static List<String> _splitTableRow(String line) {
final cells = line.split('|').map((cell) => cell.trim()).toList();
// Remove only markdown edge pipes, preserve intentional empty cells.
if (cells.isNotEmpty && cells.first.isEmpty) cells.removeAt(0);
if (cells.isNotEmpty && cells.last.isEmpty) cells.removeLast();
return cells;
}Also applies to: 586-591
| ``` | ||
| new_ib/ | ||
| ├── new_ib_landing_view.dart # Main view (entry point) | ||
| ├── components/ | ||
| │ ├── new_ib_drawer.dart # Main drawer component | ||
| │ ├── new_ib_drawer_header.dart # Drawer header with gradient | ||
| │ ├── new_ib_drawer_footer.dart # Drawer footer with theme toggle | ||
| │ ├── new_ib_drawer_section.dart # Section headers in drawer | ||
| │ ├── new_ib_drawer_tile.dart # Navigation tiles (Home, Return) | ||
| │ ├── new_ib_simple_chapter_tile.dart # Simple chapter items | ||
| │ ├── new_ib_expandable_chapter_tile.dart # Expandable parent chapters | ||
| │ ├── new_ib_sub_chapter_tile.dart # Sub-chapter items | ||
| │ ├── new_ib_content.dart # Main content area router | ||
| │ └── new_ib_home_page.dart # Home page with features | ||
| └── README.md # This file | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
Line 7 triggers markdownlint MD040. Please specify a language (e.g., text) for the tree block.
Suggested fix
-```
+```text
new_ib/
├── new_ib_landing_view.dart # Main view (entry point)
...
└── README.md # This file</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
| - "Also on Interactive Book" suggestions | ||
| - Comments section with input and list | ||
| - Auto-hide buttons on scroll | ||
| - `new_ib_markdown_parser.dart`: Custom markdown parser using Text widgets |
There was a problem hiding this comment.
Use proper noun capitalization for “Markdown”.
Line 73 should use “Markdown” (capital M) for consistency and readability.
Suggested fix
-- `new_ib_markdown_parser.dart`: Custom markdown parser using Text widgets
+- `new_ib_markdown_parser.dart`: Custom Markdown parser using Text widgets📝 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.
| - `new_ib_markdown_parser.dart`: Custom markdown parser using Text widgets | |
| - `new_ib_markdown_parser.dart`: Custom Markdown parser using Text widgets |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~73-~73: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...- new_ib_markdown_parser.dart: Custom markdown parser using Text widgets ## Future En...
(MARKDOWN_NNP)
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f08f465-90b9-4981-83c8-42e38fe17073
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
android/app/build.gradleandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/settings.gradledevtools_options.yamllib/config/environment_config.dartlib/services/API/disqus_api.dart
✅ Files skipped from review due to trivial changes (1)
- android/gradle/wrapper/gradle-wrapper.properties
|
@coderabbitai trigger |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
lib/config/environment_config.dart (2)
12-15: Production builds will fail silently with localhost default.The default value
http://localhost:4000/api-mobile/pageswon't work in production. Consider either:
- Adding a production-ready default URL (similar to
IB_API_BASE_URLpattern), or- Documenting clearly that this must be set for production builds
This is acceptable for a PoC but should be addressed before production release.
34-35: Missing API key will cause silent failures.
DISQUS_API_KEYdefaults to an empty string when not set. Per theDisqusApi.fetchRecommendationsimplementation (lib/services/API/disqus_api.dart:18-67), API errors are caught and silently return an empty list, making it difficult to diagnose missing configuration.Consider adding validation or logging when the key is empty to help developers identify configuration issues during development.
lib/services/ib_engine_service.dart (1)
58-67: Consider removing or converting debug prints before release.These
print()statements are useful for development but should be removed or replaced with a proper logging framework (e.g.,loggerpackage) that can be disabled in production builds.🔧 Example using a logging approach
- print('IbEngineService: Fetching from $url'); + // Use a logger with configurable levels instead of print + // logger.d('IbEngineService: Fetching from $url'); _apiResponse = await _ibApi.fetchApiPage(id: id); - print('IbEngineService: Got ${_apiResponse?.length ?? 0} items from API'); + // logger.d('IbEngineService: Got ${_apiResponse?.length ?? 0} items from API'); } catch (err) { - print('IbEngineService: API error - $err'); + // logger.e('IbEngineService: API error - $err');lib/ui/views/new_ib/components/quiz_widget.dart (1)
62-73: Consider defensive error handling for malformed JSON.Line 68 casts each element to
Map<String, dynamic>, which will throw if the JSON structure is unexpected. Given this is parsing external API data, consider wrapping in try-catch.🛡️ Proposed defensive parsing
void _parseQuestions() { if (widget.questions is List<QuizQuestion>) { _parsedQuestions = widget.questions as List<QuizQuestion>; } else if (widget.questions is List<dynamic>) { - _parsedQuestions = - (widget.questions as List<dynamic>) - .map((q) => QuizQuestion.fromJson(q as Map<String, dynamic>)) - .toList(); + _parsedQuestions = []; + for (final q in widget.questions as List<dynamic>) { + try { + _parsedQuestions.add(QuizQuestion.fromJson(q as Map<String, dynamic>)); + } catch (e) { + // Skip malformed questions, optionally log + } + } } else { _parsedQuestions = []; } }lib/models/ib/ib_json_page_data.dart (2)
37-46: Required field casts may throw on malformed API responses.Lines 39, 40, and 46 cast directly to
Stringwithout null checks. If the API returns malformed data with missing required fields, this will throw aTypeErrorat runtime rather than a descriptive error.🛡️ Proposed null-safe parsing
factory IbJsonPageData.fromJson(Map<String, dynamic> json) { + final id = json['id']; + final title = json['title']; + final path = json['path']; + if (id == null || title == null || path == null) { + throw FormatException('Missing required fields in JSON page data'); + } return IbJsonPageData( - id: json['id'] as String, - title: json['title'] as String, + id: id as String, + title: title as String, description: json['description'] as String?, navOrder: json['nav_order']?.toString(), level: json['level'] as String?, hasChildren: json['has_children'] as bool? ?? false, parent: json['parent'] as String?, - path: json['path'] as String, + path: path as String,
88-95: Missing null check for required 'sections' field.
IbJsonContent.fromJsonassumesjson['sections']is non-null and a valid list. If the API returnscontentwithout asectionskey, this will throw.🛡️ Proposed fix
factory IbJsonContent.fromJson(Map<String, dynamic> json) { + final sections = json['sections'] as List<dynamic>?; return IbJsonContent( - sections: - (json['sections'] as List<dynamic>) - .map((e) => IbJsonSection.fromJson(e as Map<String, dynamic>)) - .toList(), + sections: sections + ?.map((e) => IbJsonSection.fromJson(e as Map<String, dynamic>)) + .toList() ?? [], ); }IMPLEMENTATION_SUMMARY.md (2)
39-39: Minor: Capitalize "Markdown" as a proper noun.📝 Suggested fix
-- Falls back to markdown for other pages +- Falls back to Markdown for other pages
211-211: Minor: Use hyphen for compound modifier.📝 Suggested fix
-### Long Term +### Long-TermJSON_API_INTEGRATION.md (2)
15-18: Minor: Add language specifier to code fence.The code fence should specify a language for proper syntax highlighting.
📝 Suggested fix
### Endpoint Format -``` +```text GET /api-mobile/pages/{path}.json ```
190-190: Minor: Capitalize "Markdown" as a proper noun.📝 Suggested fix
-- Fallback to markdown parser for non-JSON pages +- Fallback to Markdown parser for non-JSON pageslib/viewmodels/ib/ib_page_viewmodel.dart (1)
61-75: Shared state key could cause race conditions if both fetch methods are called.Both
fetchPageDataandfetchJsonPageDatause the same state keyIB_FETCH_PAGE_DATA. If both are called in sequence or concurrently, they'll overwrite each other's state. Currently this isn't an issue sincenew_ib_chapter_pageonly callsfetchPageData, but this could cause subtle bugs if usage patterns change.Consider using separate state keys for independent tracking, or document that only one should be active at a time.
🔧 Option: Use separate state keys
class IbPageViewModel extends BaseModel { // ViewState Keys final String IB_FETCH_PAGE_DATA = 'ib_fetch_page_data'; + final String IB_FETCH_JSON_PAGE_DATA = 'ib_fetch_json_page_data'; final String IB_FETCH_INTERACTION_DATA = 'ib_fetch_interaction_data';Then update
fetchJsonPageDatato useIB_FETCH_JSON_PAGE_DATA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e86a154-824f-45cd-8c1f-2211ca9f7c44
📒 Files selected for processing (11)
IMPLEMENTATION_SUMMARY.mdJSON_API_INTEGRATION.mdlib/config/environment_config.dartlib/models/ib/ib_json_page_data.dartlib/services/API/ib_api.dartlib/services/ib_engine_service.dartlib/ui/views/new_ib/components/json_content_renderer.dartlib/ui/views/new_ib/components/new_ib_chapter_page.dartlib/ui/views/new_ib/components/quiz_widget.dartlib/viewmodels/ib/ib_landing_viewmodel.dartlib/viewmodels/ib/ib_page_viewmodel.dart
| final title = data['title'] as String?; | ||
| final headers = data['headers'] as List<dynamic>?; | ||
| final rows = data['rows'] as List<dynamic>?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Flutter Table same number of children in every TableRow
💡 Result:
Table in Flutter requires every TableRow to have the exact same number of children—otherwise you’ll get the assertion error (“Every row in a table must have the same number of children.”). [1]
Fix: pad shorter rows with empty cells
Table(
children: [
TableRow(children: [
Text('A'),
Text('B'),
Text('C'),
]),
TableRow(children: [
Text('Only 1 cell'),
const SizedBox.shrink(), // filler cell
const SizedBox.shrink(), // filler cell
]),
],
);If you need “colspan/rowspan”
Flutter’s Table doesn’t support cell spanning; use another approach (e.g., DataTable, a custom Row/Column layout, or a grid package) or simulate it by inserting placeholder cells. [1]
Sources:
[1] Flutter API docs — Table.children: “Every row in a table must have the same number of children.” https://api.flutter.dev/flutter/widgets/Table/children.html
🏁 Script executed:
# First, locate and read the file to examine the code around the specified lines
fd "json_content_renderer.dart" --type f | head -1 | xargs wc -lRepository: CircuitVerse/mobile-app
Length of output: 128
🏁 Script executed:
# Read the relevant sections of the file
fd "json_content_renderer.dart" --type f | head -1 | xargs cat -n | sed -n '350,370p'Repository: CircuitVerse/mobile-app
Length of output: 818
🏁 Script executed:
# Also read the section around 420-435
fd "json_content_renderer.dart" --type f | head -1 | xargs cat -n | sed -n '410,445p'Repository: CircuitVerse/mobile-app
Length of output: 1369
Normalize API rows to match header count before creating TableRows.
headers and rows come from dynamic JSON without structural validation. Flutter Table requires every TableRow to have the exact same number of children—mismatched row lengths cause an assertion error at runtime, crashing the entire section instead of degrading gracefully.
🛠️ Suggested guard
...rows.map((row) {
final rowData = row as List<dynamic>;
+ final normalizedRow = rowData.length < headers.length
+ ? [...rowData, ...List.filled(headers.length - rowData.length, '')]
+ : rowData.take(headers.length).toList();
return TableRow(
children:
- rowData.map((cell) {
+ normalizedRow.map((cell) {| onModelReady: (model) { | ||
| // Always use the regular markdown API for now | ||
| // JSON API will be used when backend is ready | ||
| model.fetchPageData(id: widget.chapter.id); | ||
| }, |
There was a problem hiding this comment.
Hide or wire the TOC in the markdown fallback.
_buildTocItem scrolls through _headingKeys, but the fallback branch renders raw markdown with NewIbMarkdownParser.parse(...) and never populates that map. Since this widget currently always calls fetchPageData, the TOC can show up as a dead control on markdown-backed chapters.
Also applies to: 252-274
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new “New Interactive Book” experience to the mobile app, including a new navigation shell (drawer + content area), richer content rendering (structured blocks + markdown fallback), and scaffolding for JSON/structured-content APIs and Disqus-based recommendations.
Changes:
- Introduces
NewInteractiveBookViewwith a new drawer/content layout plus new UI widgets (quiz, binary simulator) and navigation affordances. - Adds structured/JSON content models + renderers and extends the IB API/service/viewmodels to expose raw + structured content.
- Adds Disqus recommendations API integration, new routing entry, new localization strings, and Android/Gradle tooling updates.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pubspec.lock | Updates locked deps + SDK constraints. |
| lib/viewmodels/ib/ib_page_viewmodel.dart | Adds JSON page data fetch/state + page data clearing. |
| lib/viewmodels/ib/ib_landing_viewmodel.dart | Avoids isolate setup / prev-next wiring when chapters fail/empty. |
| lib/utils/router.dart | Registers route for NewInteractiveBookView. |
| lib/ui/views/new_ib/new_ib_landing_view.dart | New IB entry view (Scaffold + drawer + content). |
| lib/ui/views/new_ib/components/structured_content_renderer.dart | Renders structured content blocks into Flutter widgets. |
| lib/ui/views/new_ib/components/quiz_widget.dart | New quiz widget supporting parsed + JSON-style questions. |
| lib/ui/views/new_ib/components/new_ib_sub_chapter_tile.dart | Drawer tile UI for sub-chapters. |
| lib/ui/views/new_ib/components/new_ib_simple_chapter_tile.dart | Drawer tile UI for leaf chapters. |
| lib/ui/views/new_ib/components/new_ib_markdown_parser.dart | Custom markdown parser (widgets + TOC/quiz/table handling). |
| lib/ui/views/new_ib/components/new_ib_home_page.dart | New IB home page UI and markdown content rendering. |
| lib/ui/views/new_ib/components/new_ib_expandable_chapter_tile.dart | Drawer tile UI for expandable parent chapters. |
| lib/ui/views/new_ib/components/new_ib_drawer_tile.dart | Drawer navigation tiles (Home/Return Home). |
| lib/ui/views/new_ib/components/new_ib_drawer_section.dart | Drawer section header UI. |
| lib/ui/views/new_ib/components/new_ib_drawer_header.dart | Drawer header UI. |
| lib/ui/views/new_ib/components/new_ib_drawer_footer.dart | Drawer footer UI incl. theme toggle. |
| lib/ui/views/new_ib/components/new_ib_drawer.dart | New IB drawer assembly + chapter list rendering. |
| lib/ui/views/new_ib/components/new_ib_content.dart | Routes selected chapter to home/chapter page/placeholder. |
| lib/ui/views/new_ib/components/new_ib_chapter_page.dart | Chapter page UI: TOC, content render, recs, comments, prev/next FAB. |
| lib/ui/views/new_ib/components/json_content_renderer.dart | Renderer for JSON API content sections. |
| lib/ui/views/new_ib/components/binary_simulator_widget.dart | Interactive binary-to-decimal widget. |
| lib/ui/views/new_ib/README.md | New IB module documentation. |
| lib/ui/components/cv_drawer.dart | Adds “New Interactive Book” entry to the app drawer. |
| lib/services/ib_engine_service.dart | Adds rawPageData passthrough + debug prints for API/chapter fetch. |
| lib/services/API/ib_api.dart | Adds JSON page data fetch with caching support. |
| lib/services/API/disqus_api.dart | Adds Disqus recommendations API client. |
| lib/models/ib/ib_structured_content.dart | New structured content block + inline content models. |
| lib/models/ib/ib_recommendation.dart | New recommendation model for Disqus responses. |
| lib/models/ib/ib_raw_page_data.dart | Parses structured_content into typed models. |
| lib/models/ib/ib_page_data.dart | Adds rawPageData to carry structured content forward. |
| lib/models/ib/ib_json_page_data.dart | New models for JSON page API schema. |
| lib/locator.dart | Registers DisqusApi in DI. |
| lib/l10n/app_hi.arb | Adds “New Interactive Book” translation key. |
| lib/l10n/app_en.arb | Adds “New Interactive Book” translation key. |
| lib/l10n/app_ar.arb | Adds “New Interactive Book” translation key. |
| lib/gen_l10n/app_localizations_hi.dart | Generated localization getter. |
| lib/gen_l10n/app_localizations_en.dart | Generated localization getter. |
| lib/gen_l10n/app_localizations_ar.dart | Generated localization getter. |
| lib/gen_l10n/app_localizations.dart | Generated localization interface update. |
| lib/config/environment_config.dart | Adds JSON API base URL + Disqus API key env var. |
| devtools_options.yaml | Adds DevTools options scaffold. |
| android/settings.gradle | Updates Android Gradle plugin + Kotlin plugin versions. |
| android/gradle/wrapper/gradle-wrapper.properties | Updates Gradle wrapper version. |
| android/app/build.gradle | Updates compileSdk/targetSdk/minSdk settings. |
| STRUCTURED_CONTENT_CHANGES.md | Docs for structured content implementation. |
| JSON_API_INTEGRATION.md | Docs for JSON API schema/integration. |
| IMPLEMENTATION_SUMMARY.md | Implementation overview documentation. |
| IB_API_RESTRUCTURE_SPEC.md | Spec doc for API restructure to structured content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Widget _buildCommentsList(BuildContext context) { | ||
| // Mock comments data | ||
| final comments = [ | ||
| { |
There was a problem hiding this comment.
The PR description mentions Disqus-powered comments, but this section currently uses mock comment data (and the post action is a no-op). Either integrate Disqus for real here or clearly label this UI as placeholder to avoid misleading users.
| } else if (block is IbParagraphContent) { | ||
| return _buildParagraph(context, block); | ||
| } else if (block is IbListItemBlockContent) { | ||
| return _buildListItem(context, block); | ||
| } else if (block is IbCodeBlockContent) { |
There was a problem hiding this comment.
Structured content supports IbListContent (type: 'list'), but the renderer only handles IbListItemBlockContent. If the API returns a list block, it will currently render as empty. Add a branch to render IbListContent (including ordered vs unordered and nested items).
| _jsonPageData = null; | ||
| setStateFor(IB_FETCH_PAGE_DATA, ViewState.Busy); | ||
|
|
||
| _jsonPageData = await _ibApi.fetchJsonPageData(path: path); | ||
|
|
There was a problem hiding this comment.
fetchJsonPageData updates IB_FETCH_PAGE_DATA state, which is also used by fetchPageData. This can cause state collisions between markdown and JSON fetches. Introduce a separate ViewState key for JSON (e.g., IB_FETCH_JSON_PAGE_DATA).
| Text( | ||
| model.errorMessageFor(model.IB_FETCH_PAGE_DATA) ?? | ||
| 'Unknown error', | ||
| style: TextStyle( |
There was a problem hiding this comment.
errorMessageFor(...) returns a non-nullable String, so the ?? 'Unknown error' fallback is dead code and may trigger analyzer lints. Remove the ?? fallback (or change errorMessageFor to return String? if needed).
| Widget _buildJsonContent(BuildContext context, IbJsonPageData jsonPageData) { | ||
| return Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| // Chapter Title | ||
| Text( | ||
| jsonPageData.title, | ||
| style: TextStyle( | ||
| fontSize: 32, | ||
| fontWeight: FontWeight.bold, | ||
| color: IbTheme.primaryHeadingColor(context), | ||
| ), | ||
| ), | ||
| if (jsonPageData.description != null) ...[ | ||
| const SizedBox(height: 8), | ||
| Text( | ||
| jsonPageData.description!, | ||
| style: TextStyle( | ||
| fontSize: 16, | ||
| color: IbTheme.textColor(context).withAlpha(179), | ||
| ), | ||
| ), | ||
| ], | ||
| const SizedBox(height: 8), | ||
| Divider(thickness: 2, color: IbTheme.getPrimaryColor(context)), | ||
| const SizedBox(height: 24), | ||
|
|
||
| // JSON Content | ||
| if (jsonPageData.content != null && | ||
| jsonPageData.content!.sections.isNotEmpty) | ||
| JsonContentRenderer( | ||
| sections: jsonPageData.content!.sections, | ||
| headingKeys: _headingKeys, | ||
| ), | ||
|
|
||
| // Children (for parent pages like Binary Representation index) | ||
| if (jsonPageData.children != null && jsonPageData.children!.isNotEmpty) | ||
| _buildJsonChildren(context, jsonPageData.children!), | ||
| ], | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
_buildJsonContent (and related helpers) are currently unused in this widget, which can trigger unused_element warnings under flutter_lints. Either wire the JSON rendering path now or remove/move these helpers until the JSON API is actually used.
| Widget _buildJsonContent(BuildContext context, IbJsonPageData jsonPageData) { | |
| return Column( | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| children: [ | |
| // Chapter Title | |
| Text( | |
| jsonPageData.title, | |
| style: TextStyle( | |
| fontSize: 32, | |
| fontWeight: FontWeight.bold, | |
| color: IbTheme.primaryHeadingColor(context), | |
| ), | |
| ), | |
| if (jsonPageData.description != null) ...[ | |
| const SizedBox(height: 8), | |
| Text( | |
| jsonPageData.description!, | |
| style: TextStyle( | |
| fontSize: 16, | |
| color: IbTheme.textColor(context).withAlpha(179), | |
| ), | |
| ), | |
| ], | |
| const SizedBox(height: 8), | |
| Divider(thickness: 2, color: IbTheme.getPrimaryColor(context)), | |
| const SizedBox(height: 24), | |
| // JSON Content | |
| if (jsonPageData.content != null && | |
| jsonPageData.content!.sections.isNotEmpty) | |
| JsonContentRenderer( | |
| sections: jsonPageData.content!.sections, | |
| headingKeys: _headingKeys, | |
| ), | |
| // Children (for parent pages like Binary Representation index) | |
| if (jsonPageData.children != null && jsonPageData.children!.isNotEmpty) | |
| _buildJsonChildren(context, jsonPageData.children!), | |
| ], | |
| ); | |
| } |
| class NewInteractiveBookView extends StatefulWidget { | ||
| const NewInteractiveBookView({super.key}); | ||
|
|
||
| static const String id = 'new_interactive_book_view'; | ||
|
|
||
| @override | ||
| State<NewInteractiveBookView> createState() => _NewInteractiveBookViewState(); | ||
| } |
There was a problem hiding this comment.
There are existing UI tests for the legacy Interactive Book views, but this new entry point isn’t covered. Add at least a basic widget test for routing + drawer chapter selection + loading/error states for the new IB view.
| errorBuilder: (context, error, stackTrace) { | ||
| print('Error loading image: ${recommendation.image}'); | ||
| print('Error: $error'); | ||
| return Container( | ||
| color: IbTheme.getPrimaryColor(context).withAlpha(51), |
There was a problem hiding this comment.
Avoid print in widget error handlers (and avoid logging potentially user-controlled URLs). Prefer debugPrint gated by kDebugMode or report via a logger/crash reporter; also consider providing a user-visible fallback rather than silently swallowing the tap/action.
| if (_scrollController.position.userScrollDirection == AxisDirection.down) { | ||
| if (_showFloatingButtons) { | ||
| setState(() => _showFloatingButtons = false); | ||
| } | ||
| } else if (_scrollController.position.userScrollDirection == | ||
| AxisDirection.up) { |
There was a problem hiding this comment.
userScrollDirection is a ScrollDirection (forward/reverse/idle), but it’s compared to AxisDirection, so the condition won’t behave as intended and the auto-hide FAB logic will not trigger. Compare against ScrollDirection.forward/reverse instead.
| if (_scrollController.position.userScrollDirection == AxisDirection.down) { | |
| if (_showFloatingButtons) { | |
| setState(() => _showFloatingButtons = false); | |
| } | |
| } else if (_scrollController.position.userScrollDirection == | |
| AxisDirection.up) { | |
| if (_scrollController.position.userScrollDirection == | |
| ScrollDirection.reverse) { | |
| if (_showFloatingButtons) { | |
| setState(() => _showFloatingButtons = false); | |
| } | |
| } else if (_scrollController.position.userScrollDirection == | |
| ScrollDirection.forward) { |
| onModelReady: (model) { | ||
| // Always use the regular markdown API for now | ||
| // JSON API will be used when backend is ready | ||
| model.fetchPageData(id: widget.chapter.id); |
There was a problem hiding this comment.
fetchPageData is always triggered on entry, even though _buildContent returns a “Coming Soon” placeholder for most chapters. Consider skipping the fetch for unsupported chapters (or fetching lazily only when content is actually rendered) to avoid unnecessary network work.
| model.fetchPageData(id: widget.chapter.id); | |
| if (model.pageData == null) { | |
| model.fetchPageData(id: widget.chapter.id); | |
| } |
| void _selectAnswer(int questionIndex, int answerIndex, bool isCorrect) { | ||
| setState(() { | ||
| _selectedAnswers['$questionIndex-$answerIndex'] = isCorrect; | ||
| _showExplanation[questionIndex] = true; | ||
| }); |
There was a problem hiding this comment.
Answer selection allows multiple answers per question because selections are tracked per answer key without clearing prior choices for the same question. If the intended UX is single-choice, clear previous selections for the question index before setting the new one.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
lib/ui/views/new_ib/components/new_ib_home_page.dart (2)
42-53:⚠️ Potential issue | 🟡 MinorMove the home-page copy into the ARB/l10n flow.
This screen still hardcodes its error text, hero copy, feature labels, onboarding steps, and footer, so it will stay English when the rest of the app switches locale.
Based on learnings, ARB files are the source files that get compiled into generated localization classes, so these strings should be added there instead of editing
lib/gen_l10n/*.Also applies to: 114-125, 163-165, 191-210, 305-335
214-263:⚠️ Potential issue | 🟠 MajorUse a width-aware grid here.
Forcing 2 cards per row with a fixed
childAspectRatiostill leaves too little vertical room on narrow devices and with large text, so these feature cards can clip or overflow. This gets worse once the copy is localized.♿ Suggested direction
- gridDelegate: const SliverGridDelegateWithFixedCrossAxisCount( - crossAxisCount: 2, - crossAxisSpacing: 16, - mainAxisSpacing: 16, - childAspectRatio: 1.2, - ), + gridDelegate: const SliverGridDelegateWithMaxCrossAxisExtent( + maxCrossAxisExtent: 280, + crossAxisSpacing: 16, + mainAxisSpacing: 16, + mainAxisExtent: 170, + ),lib/ui/views/new_ib/components/new_ib_chapter_page.dart (3)
103-115:⚠️ Potential issue | 🔴 CriticalCompare
userScrollDirectionagainstScrollDirection, notAxisDirection.
ScrollPosition.userScrollDirectionreturnsScrollDirection, so these checks never match and the floating buttons won't auto-hide/show on scroll.🐛 Minimal fix
- if (_scrollController.position.userScrollDirection == AxisDirection.down) { + if (_scrollController.position.userScrollDirection == + ScrollDirection.reverse) { if (_showFloatingButtons) { setState(() => _showFloatingButtons = false); } } else if (_scrollController.position.userScrollDirection == - AxisDirection.up) { + ScrollDirection.forward) { if (!_showFloatingButtons) { setState(() => _showFloatingButtons = true); } }Flutter ScrollPosition userScrollDirection return type and enum values
241-263:⚠️ Potential issue | 🟠 MajorHide or wire the TOC when you fall back to raw markdown.
The TOC is shown whenever
pageData.tableOfContentsexists, but theNewIbMarkdownParser.parse(...)fallback never populates_headingKeys, so_scrollToSectionhas nothing to target and those entries become dead controls. Either generate heading keys in the fallback renderer or suppress the TOC unless the structured renderer is active.Also applies to: 403-440
519-521:⚠️ Potential issue | 🟡 MinorWire these tap targets or remove the affordance.
Both cards render as tappable
InkWells with navigation arrows, but their handlers are empty. Users can tap chapter-content and recommendation cards and get no result.Also applies to: 735-736
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f522d888-8c61-46ab-b8d6-dd4ba6de2923
📒 Files selected for processing (4)
lib/services/API/disqus_api.dartlib/ui/views/new_ib/components/new_ib_chapter_page.dartlib/ui/views/new_ib/components/new_ib_home_page.dartlib/ui/views/new_ib/new_ib_landing_view.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ui/views/new_ib/new_ib_landing_view.dart
- lib/services/API/disqus_api.dart
| 'Failed to load content', | ||
| style: TextStyle( |
There was a problem hiding this comment.
Move the chapter-page copy and date formatting into the localization flow.
This file still hardcodes English copy across the error UI, placeholder, TOC, recommendations, and comments UI, and it formats recommendation dates as fixed YYYY-MM-DD. Please move the strings into ARB and switch the date rendering to a locale-aware format so this page follows the active locale.
Also applies to: 206-216, 416-416, 589-631, 668-673, 814-859
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
lib/models/ib/new_ib_drawer_data.dart (1)
129-138: Consider adding null-safety guards for required fields.The
fromJsonfactories use directascasts (e.g.,json['id'] as String) which will throwTypeErrorif the API returnsnullfor any required field. For a more resilient PoC, consider providing fallback values or clearer error messages.Example defensive pattern
factory NewIbSubChapter.fromJson(Map<String, dynamic> json) { return NewIbSubChapter( - id: json['id'] as String, - title: json['title'] as String, + id: json['id'] as String? ?? '', + title: json['title'] as String? ?? 'Untitled', navOrder: json['nav_order'] as String, level: json['level'] as String, path: json['path'] as String, apiUrl: json['api_url'] as String?, ); }lib/ui/views/new_ib/components/new_ib_content.dart (2)
459-537: Unused method:_buildChapterCardis defined but never called.This method is fully implemented but not invoked anywhere in the file. Either integrate it into the home/chapter flow or remove it to avoid dead code.
567-578: Consider hiding internal API URL in production.Displaying
chapter.apiUrlis useful for debugging but may expose internal infrastructure details to end users. Consider guarding this behind a debug flag or removing it before production release.Example: conditionally show in debug mode
+ if (kDebugMode) ...[ const SizedBox(height: 8), Padding( padding: const EdgeInsets.symmetric(horizontal: 32), child: Text( 'API: ${chapter.apiUrl}', style: TextStyle( fontSize: 12, color: IbTheme.textColor(context).withAlpha(128), fontFamily: 'monospace', ), textAlign: TextAlign.center, ), ), + ],lib/viewmodels/ib/new_ib_landing_viewmodel.dart (1)
11-13: Consider making ViewState keysstatic const.These keys are constant strings that don't change per instance. Making them
static constavoids creating new string instances for each viewmodel.Suggested change
- // ViewState Keys - final String NEW_IB_FETCH_DRAWER = 'new_ib_fetch_drawer'; - final String NEW_IB_FETCH_HOME = 'new_ib_fetch_home'; + // ViewState Keys + static const String NEW_IB_FETCH_DRAWER = 'new_ib_fetch_drawer'; + static const String NEW_IB_FETCH_HOME = 'new_ib_fetch_home';lib/ui/views/new_ib/components/new_ib_drawer.dart (1)
150-156: Minor:errorMessageForreturnsString, notString?.Per
BaseModel,errorMessageForreturns an empty string''when no error is set, notnull. The?? 'Unknown error'fallback will never trigger. Consider removing it or checking for empty string instead.Suggested fix
Text( - model.errorMessageFor(model.NEW_IB_FETCH_DRAWER) ?? 'Unknown error', + model.errorMessageFor(model.NEW_IB_FETCH_DRAWER).isNotEmpty + ? model.errorMessageFor(model.NEW_IB_FETCH_DRAWER) + : 'Unknown error', style: TextStyle(lib/models/ib/new_ib_home_data.dart (1)
141-159: Potential duplication withNewIbDrawerMetadata.
NewIbMetadataandNewIbDrawerMetadata(fromnew_ib_drawer_data.dart) sharecopyright,website, andrepositoryfields. Consider extracting a shared base class or using the same type if the API schemas are compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 983c85fc-56ea-4426-82c9-59a0127b1651
📒 Files selected for processing (13)
lib/locator.dartlib/models/ib/new_ib_drawer_data.dartlib/models/ib/new_ib_home_data.dartlib/services/API/new_ib_api.dartlib/ui/views/new_ib/components/new_ib_content.dartlib/ui/views/new_ib/components/new_ib_drawer.dartlib/ui/views/new_ib/components/new_ib_drawer_footer.dartlib/ui/views/new_ib/components/new_ib_drawer_tile.dartlib/ui/views/new_ib/components/new_ib_expandable_chapter_tile.dartlib/ui/views/new_ib/components/new_ib_simple_chapter_tile.dartlib/ui/views/new_ib/components/new_ib_sub_chapter_tile.dartlib/ui/views/new_ib/new_ib_landing_view.dartlib/viewmodels/ib/new_ib_landing_viewmodel.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/views/new_ib/components/new_ib_sub_chapter_tile.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/locator.dart
- lib/ui/views/new_ib/components/new_ib_drawer_tile.dart
- lib/ui/views/new_ib/components/new_ib_drawer_footer.dart
- lib/ui/views/new_ib/components/new_ib_simple_chapter_tile.dart
| return NewIbChapter( | ||
| id: json['id'] as String, | ||
| title: json['title'] as String, | ||
| navOrder: json['nav_order'] as int, | ||
| heading: json['heading'] as String?, | ||
| description: json['description'] as String?, | ||
| path: json['path'] as String, | ||
| hasChildren: json['has_children'] as bool?, | ||
| apiUrl: json['api_url'] as String, | ||
| navOrderStr: json['nav_order_str'] as String?, | ||
| level: json['level'] as String?, | ||
| children: children, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's API documentation or sample responses that clarify navOrder types
rg -n "nav_order" --type dart -C 2Repository: CircuitVerse/mobile-app
Length of output: 6676
🏁 Script executed:
# Check the actual field definitions in the model classes
cat -n lib/models/ib/new_ib_drawer_data.dart | head -150 | tail -80Repository: CircuitVerse/mobile-app
Length of output: 2738
Type inconsistency: navOrder is int in NewIbChapter but String in NewIbSubChapter.
NewIbChapter.navOrder (line 99) casts to int, while NewIbSubChapter.navOrder (line 133) casts to String. The mock test data shows the API returns nav_order as both numeric and string values (e.g., 1, 10, 'l0s000'), so the cast will fail at runtime depending on what the API returns. Standardize both to String (like other models in ib_raw_page_data.dart and ib_json_page_data.dart do via .toString()) or add proper type guards.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/ui/views/new_ib/components/new_ib_content.dart (2)
745-748: TODO: Child page navigation is not implemented.The
onTaphandler is currently a no-op. Users tapping on child tiles will see no response, which could be confusing.Would you like me to help implement the navigation logic, or should I open an issue to track this task for a future PR?
460-538:_buildChapterCardis unused and should be removed or documented.This method (lines 460-538) is defined but never called anywhere in the file. The home page uses
_buildHomeSection()to render home content, and the chapter page uses_buildChapterSection()and_buildChildTile()to render chapter sections and sub-topics. If this method is intended for future use, add a TODO comment clarifying its purpose; otherwise, remove it as dead code.lib/viewmodels/ib/new_ib_landing_viewmodel.dart (1)
11-15: Consider usingstatic constfor ViewState keys.The keys are currently instance members (
final String). Since they're constant values that don't change per instance, making themstatic constwould be more idiomatic and slightly more efficient.Suggested change
class NewIbLandingViewModel extends BaseModel { // ViewState Keys - final String NEW_IB_FETCH_DRAWER = 'new_ib_fetch_drawer'; - final String NEW_IB_FETCH_HOME = 'new_ib_fetch_home'; - final String NEW_IB_FETCH_CHAPTER_INDEX = 'new_ib_fetch_chapter_index'; + static const String NEW_IB_FETCH_DRAWER = 'new_ib_fetch_drawer'; + static const String NEW_IB_FETCH_HOME = 'new_ib_fetch_home'; + static const String NEW_IB_FETCH_CHAPTER_INDEX = 'new_ib_fetch_chapter_index';
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b22b5bfe-48fd-4354-8cca-3a66e52be314
📒 Files selected for processing (4)
lib/models/ib/new_ib_chapter_index_data.dartlib/services/API/new_ib_api.dartlib/ui/views/new_ib/components/new_ib_content.dartlib/viewmodels/ib/new_ib_landing_viewmodel.dart
| class NewIbChapterChild { | ||
| final String title; | ||
| final String path; | ||
| final String navOrder; | ||
| final String level; | ||
|
|
||
| NewIbChapterChild({ | ||
| required this.title, | ||
| required this.path, | ||
| required this.navOrder, | ||
| required this.level, | ||
| }); | ||
|
|
||
| factory NewIbChapterChild.fromJson(Map<String, dynamic> json) { | ||
| return NewIbChapterChild( | ||
| title: json['title'] as String, | ||
| path: json['path'] as String, | ||
| navOrder: json['nav_order'] as String, | ||
| level: json['level'] as String, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the API response structure to verify navOrder types
# Look for any test fixtures or mock data that might clarify expected types
fd -e json -i | xargs rg -l 'nav_order' 2>/dev/null | head -5 | xargs cat 2>/dev/null || echo "No JSON fixtures found"
# Check if there are any other usages of these models that might clarify expected types
rg -n 'navOrder' --type=dart -C2Repository: CircuitVerse/mobile-app
Length of output: 50381
🏁 Script executed:
# First, let's see the exact content around the lines mentioned
head -160 lib/models/ib/new_ib_chapter_index_data.dart | tail -40Repository: CircuitVerse/mobile-app
Length of output: 936
🏁 Script executed:
# Now let's check around line 45 for NewIbChapterIndexData
head -60 lib/models/ib/new_ib_chapter_index_data.dart | tail -30Repository: CircuitVerse/mobile-app
Length of output: 1276
🏁 Script executed:
# Check the full class definitions to see the property types
grep -A 10 "class NewIbChapterIndexData" lib/models/ib/new_ib_chapter_index_data.dart | head -20Repository: CircuitVerse/mobile-app
Length of output: 356
Type inconsistency: navOrder is String in NewIbChapterChild but int in NewIbChapterIndexData.
NewIbChapterChild.navOrder is declared as String and parsed as String (line 147), but NewIbChapterIndexData.navOrder is declared as int and parsed as int (line 45). Since both parse from the same JSON field (nav_order), which the API provides as a numeric value, the String cast in NewIbChapterChild.fromJson will fail at runtime. Change line 147 to navOrder: json['nav_order'] as int, and update the field type on line 133 to final int navOrder;.
Interactive Book Improvement
Summary
This PR introduces a completely redesigned Interactive Book feature with a modern UI, interactive learning tools, and enhanced navigation for a better mobile learning experience.
Key Features
Modern User Interface
Enhanced Content Display
Interactive Learning Tools
Improved Navigation
Community Integration
Technical Improvements
Changes
Impact
This redesign makes learning digital circuits more engaging and accessible on mobile devices, providing students with an interactive textbook experience right in the app.
🧪 Testing
Demo Video
IB.mp4
Summary by CodeRabbit
New Features
Documentation
Localization
Chores