fix(chart): render in-app meteogram as image, not a PlatformView#41
Merged
Conversation
The in-app chart was an AndroidView PlatformView. On Flutter 3.4x that composites via Texture Layer Hybrid Composition: the native ImageView's surface is rendered into an ImageReader and handed to Impeller as an external texture. On the Vulkan backend (e.g. Adreno / Android 12), the raster thread's Image.getHardwareBuffer() JNI call can throw on a closed/expired image, and the engine turns any pending JNI exception into a fatal abort (platform_view_android_jni_impl.cc CheckException) -- flutter/flutter#175267. Crash seen on v1.2.3 (Flutter 3.44.1); still latent on main (3.44.4), as the engine fix does not cover this case. The chart is a static, non-interactive bitmap, so the PlatformView is overkill. Rasterize the SVG to PNG natively (reuse the already-present MainActivity.renderSvgToPng, previously dead code) and display it with a plain Flutter Image.memory. This removes the only TextureLayer in the app, eliminating the Impeller external-texture path entirely. Rendering is unchanged (same AndroidSVG rasterizer as the widget). - Drop NativeSvgChartView, SvgChartPlatformView, SvgChartViewFactory and the registerViewFactory call. - Add NativeSvgService.renderSvgToPng; cache PNG bytes (stable instance so MemoryImage hits the image cache; gaplessPlayback avoids flicker). - Move the renderSvg handler off the platform thread. - Real Flutter Semantics now reach the charts: they carry resource-ids (home_hourly_chart / home_weekly_chart) and a localized content-desc, replacing the native content-desc hack. e2e/docs updated to match. Verified: make analyze clean; 123 Dart tests; Kotlin tests + debug build (JDK 17); runs on emulator with both charts rendering and the a11y ids + labels surfacing. Field confirmation needs a Vulkan device (x86_64 emulator cannot reproduce it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR replaces the Android PlatformView-based SVG chart rendering with native PNG rasterization. SvgChartPlatformView and SvgChartViewFactory are removed; MainActivity dispatches renderSvg on a background thread. HomeScreen now caches and renders PNG bytes via Image.memory. Accessibility identifiers and e2e documentation are updated accordingly. ChangesSVG-to-PNG rasterization migration
Sequence Diagram(s)sequenceDiagram
participant HomeScreen
participant NativeSvgService
participant MainActivity
participant BackgroundThread
HomeScreen->>HomeScreen: generate SVG string
HomeScreen->>NativeSvgService: renderSvgToPng(svg, width, height)
NativeSvgService->>MainActivity: invoke renderSvg via MethodChannel
MainActivity->>BackgroundThread: start Thread for rasterization
BackgroundThread-->>MainActivity: PNG bytes or error
MainActivity-->>NativeSvgService: result via runOnUiThread
NativeSvgService-->>HomeScreen: Uint8List png or null
HomeScreen->>HomeScreen: cache png and render Image.memory
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (2)
e2e/specs/home_happy_path.e2e.js (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the new chart resource IDs in the assertions below.
These checks are still keyed off localized
content-desctext, so they remain brittle to emulator locale changes even though this PR introduces stablehomeHourlyChart/homeWeeklyChartids. Switching them tobyId(ids.homeHourlyChart)andbyId(ids.homeWeeklyChart)would make the spec exercise the new contract directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/specs/home_happy_path.e2e.js` around lines 20 - 21, The home happy path spec is still asserting the meteogram charts via localized content-desc text instead of the new stable resource IDs. Update the assertions in the home_happy_path.e2e.js flow to use the chart ids exposed by ids.homeHourlyChart and ids.homeWeeklyChart, keeping the checks in the same test steps but switching the selectors to byId so the spec validates the new contract directly.test/home_screen_test.dart (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one assertion that the PNG charts actually render.
This mock now makes
Image.memorysucceed, but none of the tests shown here prove that the hourly/weekly chart widgets or their new semantics actually appear. A regression back to the placeholderSizedBoxwould still pass. As per path instructions, "Ensure tests assert real behavior; WidgetStore and SharedPreferences mocks should follow the existing patterns in test/."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/home_screen_test.dart` around lines 76 - 79, The current test setup only makes Image.memory decode successfully, but it does not verify that the PNG chart widgets are actually rendered instead of falling back to a placeholder. Update the home screen widget test to add an assertion that exercises the hourly/weekly chart path and checks for the real chart widget or its semantics using the existing test patterns in home_screen_test.dart, so a regression to SizedBox would fail. Use the relevant widget entry points around the hourly/weekly chart rendering and the existing mock setup to confirm actual behavior rather than only mocking renderSvg.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/screens/home_screen.dart`:
- Around line 444-468: The async chart update path in the chart rendering method
should be guarded so stale renders cannot overwrite newer ones and `setState`
cannot run after dispose. Add a per-request generation/token check around the
full render flow in the chart commit logic, including the extra
`NativeSvgService.getCurrentTemperatureCelsius()` await, and verify both the
token and `mounted` immediately before the `setState` that updates `_chartCache`
and `_currentTemperatureCelsius`. Use the existing chart-rendering method and
`_chartCache` update block as the place to thread and validate the token.
- Around line 88-91: The chart cache invalidation logic in
_invalidateChartCaches is clearing entry.png too early, causing _buildChart to
render an empty fallback before the replacement frame is ready. Update the cache
model so stale/in-flight state is tracked separately from the existing bytes,
and keep the previous image data intact until native rasterization completes and
can atomically replace it. Make the change around _chartCache,
_invalidateChartCaches, and the chart-building path so weather, half-hour, and
theme refreshes remain gapless.
---
Nitpick comments:
In `@e2e/specs/home_happy_path.e2e.js`:
- Around line 20-21: The home happy path spec is still asserting the meteogram
charts via localized content-desc text instead of the new stable resource IDs.
Update the assertions in the home_happy_path.e2e.js flow to use the chart ids
exposed by ids.homeHourlyChart and ids.homeWeeklyChart, keeping the checks in
the same test steps but switching the selectors to byId so the spec validates
the new contract directly.
In `@test/home_screen_test.dart`:
- Around line 76-79: The current test setup only makes Image.memory decode
successfully, but it does not verify that the PNG chart widgets are actually
rendered instead of falling back to a placeholder. Update the home screen widget
test to add an assertion that exercises the hourly/weekly chart path and checks
for the real chart widget or its semantics using the existing test patterns in
home_screen_test.dart, so a regression to SizedBox would fail. Use the relevant
widget entry points around the hourly/weekly chart rendering and the existing
mock setup to confirm actual behavior rather than only mocking renderSvg.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: adc0fa52-be98-4dc5-93f2-60d1d878f65a
📒 Files selected for processing (12)
android/app/src/main/kotlin/org/bortnik/meteogram/MainActivity.ktandroid/app/src/main/kotlin/org/bortnik/meteogram/SvgChartPlatformView.ktandroid/app/src/main/kotlin/org/bortnik/meteogram/SvgChartViewFactory.kte2e/README.mde2e/a11y_ids.jse2e/specs/home_happy_path.e2e.jslib/a11y_ids.dartlib/screens/home_screen.dartlib/services/native_svg_service.dartlib/widgets/native_svg_chart_view.darttest/home_screen_test.darttest/native_svg_service_test.dart
💤 Files with no reviewable changes (3)
- android/app/src/main/kotlin/org/bortnik/meteogram/SvgChartViewFactory.kt
- android/app/src/main/kotlin/org/bortnik/meteogram/SvgChartPlatformView.kt
- lib/widgets/native_svg_chart_view.dart
Address PR review on the render-to-image change:
- Gapless refresh: _invalidateChartCaches now marks entries stale instead
of nulling the PNG, so the current chart stays on screen until its
replacement is rasterized. Previously it fell back to a blank SizedBox
on weather/half-hour/theme refreshes, defeating gaplessPlayback.
- Async safety: _generateSvgAsync is single-flight (generating guard),
claims the dirty flag up front so mid-render updates re-kick, and
re-checks mounted after the final await before setState. A slow older
render can no longer overwrite a newer one or setState after dispose.
- e2e: home happy-path locates charts by the new resource-ids
(homeHourlyChart / homeWeeklyChart) instead of localized content-desc.
- test: assert both charts render as Images with their semantics ids;
a regression to the placeholder SizedBox would now fail.
- docs: update CLAUDE.md + docs/ai/{architecture,widget}.md and drop the
stale ProGuard keep-rules for the deleted PlatformView classes.
make analyze clean; 124 Dart tests; Kotlin tests + debug build (JDK 17).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The in-app chart was an AndroidView PlatformView. On Flutter 3.4x that composites via Texture Layer Hybrid Composition: the native ImageView's surface is rendered into an ImageReader and handed to Impeller as an external texture. On the Vulkan backend (e.g. Adreno / Android 12), the raster thread's Image.getHardwareBuffer() JNI call can throw on a closed/expired image, and the engine turns any pending JNI exception into a fatal abort (platform_view_android_jni_impl.cc CheckException) -- flutter/flutter#175267. Crash seen on v1.2.3 (Flutter 3.44.1); still latent on main (3.44.4), as the engine fix does not cover this case.
The chart is a static, non-interactive bitmap, so the PlatformView is overkill. Rasterize the SVG to PNG natively (reuse the already-present MainActivity.renderSvgToPng, previously dead code) and display it with a plain Flutter Image.memory. This removes the only TextureLayer in the app, eliminating the Impeller external-texture path entirely. Rendering is unchanged (same AndroidSVG rasterizer as the widget).
Verified: make analyze clean; 123 Dart tests; Kotlin tests + debug build (JDK 17); runs on emulator with both charts rendering and the a11y ids
Summary by CodeRabbit
New Features
Bug Fixes