feat: black-box E2E tests (Appium + UiAutomator2) + accessibility instrumentation#23
Merged
Conversation
…trumentation Add an end-to-end UI test harness that drives the installed APK on an Android emulator, plus the Semantics instrumentation that makes the app both testable and accessible (the two are the same work — UiAutomator2 reads the a11y tree). App (lib/): - a11y_ids.dart: stable Semantics identifiers (surface to Android as resource-id), the locale-independent locator contract shared with the tests. - home_screen.dart: wrap ~20 interactive elements via a MergeSemantics helper so each is a single labeled, clickable node; enlarge the theme button and location selector to a 48dp tap target (ADA). - charts: a hybrid-composition PlatformView cannot be reached by a Flutter Semantics wrapper, so set the localized contentDescription on the native ImageView (SvgChartPlatformView.kt) via a new a11yLabel creation param. - 3 new l10n keys (clearSearch, hourly/weeklyChartLabel); regenerated across all locales (English fallback until translated). Tests (e2e/, no Flutter dependency): - WebdriverIO + Appium UiAutomator2, consuming a prebuilt APK by path. - home_happy_path.e2e.js: launch + location/theme sheet navigation. - accessibility.e2e.js: black-box ADA — every control labeled + >=48dp (inline attribution links size-exempt per WCAG 2.5.8). Contrast deferred. - uiautomator2 driver pinned to 4.2.9 (last Appium-2.x-compatible). CI: - .github/workflows/e2e.yml: Flutter build stage -> Flutter-free Node/Appium test stage on a KVM x86_64 emulator (PR + manual dispatch). Verified locally: make analyze clean, Dart + Kotlin tests pass, both specs green on an API 35 x86_64 emulator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 29 minutes and 33 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (39)
📝 WalkthroughWalkthroughAdds a shared accessibility identifier contract ( ChangesAccessibility IDs, native chart labeling, UI wiring, and E2E harness
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Flutter as Flutter build
participant Emulator as Android x86_64 Emulator
participant RunCI as run-ci.sh
participant Appium as Appium Server
participant WDIO as WebdriverIO
participant App as Meteograph APK
GHA->>Flutter: flutter build apk --debug --target-platform android-x64
Flutter-->>GHA: debug APK artifact
GHA->>Emulator: launch AVD (cached snapshot)
GHA->>RunCI: bash e2e/run-ci.sh
RunCI->>Appium: npx appium (background)
RunCI->>Appium: poll /status until ready
RunCI->>Emulator: adb emu geo fix (inject GPS)
RunCI->>WDIO: npm test
WDIO->>Appium: connect (Appium 2, UiAutomator2)
Appium->>Emulator: install & launch App
WDIO->>App: find elements by content-desc / resource-id
App-->>WDIO: element attributes (label, bounds)
WDIO-->>GHA: test results + screenshots + artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The app fetches FORECAST_DAYS=7 (WEEKLY_FORECAST_HOURS = 7*24), so the "weekly" chart shows 7 days. Correct the accessibility label and the stale "14-day" references that had crept into comments across the app. The hourly label is unchanged (still "48-hour weather forecast chart"). - weeklyChartLabel: "14-day" -> "7-day weather forecast chart" (regenerated across all locales). - Fix stale 14-day comments: home_screen.dart, native_svg_service.dart, SvgChartGenerator.kt, AndroidManifest.xml, MeteogramWidgetProvider.kt, meteogram_widget_weekly.xml. - Update the e2e weekly-chart matcher to "7-day". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
android-emulator-runner mangles multi-line inline `script:` blocks — the appium-wait for-loop was split, so `sh -c` saw `for … do` with no `done` and CI failed with "Syntax error: end of file unexpected". Move the start-appium / wait / npm-test logic into e2e/run-ci.sh and invoke it as a single line. Validated locally end-to-end against the emulator (both specs green, exit 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
actions/setup-java@v4 and actions/setup-node@v4 run on Node 20, which GitHub is deprecating (JS actions forced to Node 24 on 2026-06-16). Their v5 releases run on node24 — confirmed via each action's action.yml `using:` field. No input changes needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clearSearch, hourlyChartLabel and weeklyChartLabel were added only to the English template (app_en.arb), so gen-l10n fell back to English in every other locale. Add real translations to all 35 app_*.arb files and regenerate. Numbers (48 / 7) are kept as Western digits for consistency across languages. The major languages are high-confidence; rarer ones (jv, ka, pa, bn, ta, mk, be, sq, is, …) are best-effort and worth a native-speaker spot-check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w CI On a cold CI emulator there's no cached weather, so the home (success) screen only renders after location resolves (a ~15s fallback timeout) plus the fetch and first render. The specs clicked elements without waiting, so on the slower CI runner the success UI wasn't present yet and clicks failed with "element wasn't found". - Wait for each element (waitForDisplayed, 60s) before interacting, in both specs' tests and before-hooks. - run-ci.sh: `adb emu geo fix` so the app's location resolves immediately instead of waiting out the 15s fallback, making the success screen appear fast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a wdio afterTest hook that, on a failed test, saves a screenshot and the UiAutomator2 page source to e2e/artifacts/, and upload that (plus appium.log) as the `e2e-diagnostics` artifact. Makes CI failures diagnosable — e.g. shows whether the app was stuck on the loading spinner vs an error screen. Verified locally: a forced failure produced a PNG + XML. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Screenshots were previously captured only on failure. Make them unconditional: - afterCommand hook screenshots after each user action (tap / back / text entry), numbered in sequence — a shot per step of the flow. - afterTest screenshots after every test (pass or fail), not just failures. - page source is still captured on failure only (bulky, debug-only). - the upload step now runs `if: always()` as the `e2e-artifacts` artifact (previously failure-only `e2e-diagnostics`). Verified locally on a green run: per-step + per-test screenshots were produced (28 PNGs for the current 8 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The theme-picker options now carry Semantics(selected:) reflecting the active ThemeMode — surfaced to UiAutomator2 as the `selected` attribute and announced by screen readers (a genuine a11y improvement for the single-select list). A new happy-path test taps Dark then Light and asserts the selected option follows; the afterCommand hook also screenshots each switch for visual proof. Verified locally: selected=true/false tracks correctly (dark->true, light->false and vice-versa); 4 happy-path + 5 accessibility specs green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/widgets/native_svg_chart_view.dart (1)
73-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
didUpdateWidgetdoesn't detecta11yLabelchanges.The method checks for changes to
svgString,width, andheight, but nota11yLabel. If the label changes at runtime, the nativecontentDescriptionwon't be updated because the native side only readsa11yLabelininit{}(SvgChartPlatformView.kt lines 74-80) andrenderSvgdoesn't accept it.In practice, labels rarely change (typically only on locale changes, which trigger app restarts), but the public API accepts a parameter that won't fully work.
🔧 Proposed fix
If you want runtime label updates to work, add
a11yLabelto thedidUpdateWidgetchange detection and introduce a new method channel call to updatecontentDescriptionon the native side:`@override` void didUpdateWidget(NativeSvgChartView oldWidget) { super.didUpdateWidget(oldWidget); // Re-render if SVG or dimensions changed if (oldWidget.svgString != widget.svgString || oldWidget.width != widget.width || - oldWidget.height != widget.height) { + oldWidget.height != widget.height || + oldWidget.a11yLabel != widget.a11yLabel) { _lastRenderedSvg = widget.svgString; if (_viewId != null && _channel != null) { _channel!.invokeMethod('renderSvg', { 'svg': widget.svgString, 'width': widget.width.round(), 'height': widget.height.round(), + 'a11yLabel': widget.a11yLabel, }); } // If view not ready yet, it will render the latest SVG when created } }Then update the Kotlin side's
renderSvghandler to apply the label.🤖 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 `@lib/widgets/native_svg_chart_view.dart` around lines 73 - 90, The didUpdateWidget method in NativeSvgChartView only checks for changes to svgString, width, and height but ignores changes to a11yLabel. To fix this, add a11yLabel to the change detection condition (include oldWidget.a11yLabel != widget.a11yLabel in the if statement), and add the a11yLabel parameter to the arguments map passed to the renderSvg method channel call. Additionally, update the native Kotlin side's renderSvg handler to accept and apply the a11yLabel parameter to set the contentDescription on the native view.
🧹 Nitpick comments (1)
e2e/a11y_ids.js (1)
1-26: ⚖️ Poor tradeoffConsider automating the Dart ↔ JS contract sync.
The manual two-file mirror is documented but error-prone. If the project expands accessibility coverage, consider generating
a11y_ids.jsfromlib/a11y_ids.dartvia a build script (e.g., a Dart script that parses the source and emits JSON or JS).🤖 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/a11y_ids.js` around lines 1 - 26, The accessibility ID mapping is currently maintained manually across two files (lib/a11y_ids.dart and e2e/a11y_ids.js), creating synchronization risk as the project expands. Create a build script (such as a Dart script) that parses the accessibility ID definitions from lib/a11y_ids.dart, extracts the key-value pairs, and automatically generates the e2e/a11y_ids.js file in the correct JavaScript module.exports format. Integrate this script into the build process so the JS file is regenerated whenever the Dart file changes, eliminating the need for manual two-file synchronization and reducing the chance of inconsistency.
🤖 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 @.github/workflows/e2e.yml:
- Line 24: Replace all tag-pinned GitHub Actions references in the e2e.yml file
with full commit SHAs to satisfy supply-chain security requirements. For each
`uses:` entry that currently uses `@v*` tags (such as `actions/checkout@v6`),
replace the tag with the corresponding full commit SHA of that action version.
This needs to be applied to all affected actions/checkout references and any
other action references using tag-based pinning throughout the workflow file.
In `@e2e/README.md`:
- Line 49: Remove the stray code fence marker (the triple backticks "```") at
line 49 in e2e/README.md. This empty unlabeled fence at the end of the file
violates markdownlint rules and can cause rendering issues. Simply delete this
line entirely to fix the markdown validation error.
In `@e2e/run-ci.sh`:
- Around line 14-17: The Appium readiness check loop (lines 14-17) attempts to
curl the /status endpoint 60 times with 1-second intervals, but if all attempts
fail, the script continues to execute npm test (line 23), which runs against a
dead server and produces misleading failures. After the readiness check loop
completes, add a validation check to ensure the curl command succeeded at least
once. If the loop completed without a successful connection, exit the script
with an appropriate error message before the npm test command executes, so the
script fails fast rather than producing confusing test failures against an
unavailable Appium server.
- Line 8: The cd command on line 8 of the run-ci.sh script does not have
explicit error handling, which means if the directory change fails, subsequent
commands will execute from an incorrect working directory. Add error handling to
the cd command (such as using || exit or || { statement; }) to ensure the script
fails immediately if the directory change fails, preventing execution of
commands from the wrong location.
In `@lib/a11y_ids.dart`:
- Line 1: The file lib/a11y_ids.dart is missing the required BSL 1.1 license
header. Add the BSL 1.1 license header at the very beginning of the file before
the existing documentation comment starting with "/// Stable accessibility
identifiers". The license header should be formatted as a Dart block comment
containing the appropriate copyright and license text.
In `@lib/l10n/app_ar.arb`:
- Around line 3-5: All non-English locale files have the three keys
(clearSearch, hourlyChartLabel, weeklyChartLabel) positioned at the top of the
file immediately after @@locale, but they must be moved to match the English
template structure. In lib/l10n/app_ar.arb (lines 3-5), lib/l10n/app_be.arb
(lines 3-5), lib/l10n/app_bg.arb (lines 3-5), lib/l10n/app_bn.arb (lines 3-5),
lib/l10n/app_bs.arb (lines 3-5), and lib/l10n/app_vi.arb (lines 3-5), remove
these three keys from their current position after @@locale and relocate them to
appear after the searchCityHint key and before the theme key in each respective
file, maintaining consistency with the English template ordering.
---
Outside diff comments:
In `@lib/widgets/native_svg_chart_view.dart`:
- Around line 73-90: The didUpdateWidget method in NativeSvgChartView only
checks for changes to svgString, width, and height but ignores changes to
a11yLabel. To fix this, add a11yLabel to the change detection condition (include
oldWidget.a11yLabel != widget.a11yLabel in the if statement), and add the
a11yLabel parameter to the arguments map passed to the renderSvg method channel
call. Additionally, update the native Kotlin side's renderSvg handler to accept
and apply the a11yLabel parameter to set the contentDescription on the native
view.
---
Nitpick comments:
In `@e2e/a11y_ids.js`:
- Around line 1-26: The accessibility ID mapping is currently maintained
manually across two files (lib/a11y_ids.dart and e2e/a11y_ids.js), creating
synchronization risk as the project expands. Create a build script (such as a
Dart script) that parses the accessibility ID definitions from
lib/a11y_ids.dart, extracts the key-value pairs, and automatically generates the
e2e/a11y_ids.js file in the correct JavaScript module.exports format. Integrate
this script into the build process so the JS file is regenerated whenever the
Dart file changes, eliminating the need for manual two-file synchronization and
reducing the chance of inconsistency.
🪄 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: faff3723-e7f4-483d-ada5-ab5d73ae0774
⛔ Files ignored due to path filters (38)
e2e/package-lock.jsonis excluded by!**/package-lock.jsonlib/l10n/app_localizations.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_ar.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_be.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_bg.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_bn.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_bs.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_cs.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_da.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_de.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_el.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_en.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_es.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_fi.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_fr.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_hi.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_hr.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_is.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_it.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_ja.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_jv.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_ka.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_ko.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_mk.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_nl.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_no.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_pa.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_pl.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_pt.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_ro.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_sk.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_sq.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_sv.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_ta.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_tr.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_uk.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_vi.dartis excluded by!lib/l10n/app_localizations*.dartlib/l10n/app_localizations_zh.dartis excluded by!lib/l10n/app_localizations*.dart
📒 Files selected for processing (54)
.github/workflows/e2e.yml.gitignoreandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/kotlin/org/bortnik/meteogram/MeteogramWidgetProvider.ktandroid/app/src/main/kotlin/org/bortnik/meteogram/SvgChartGenerator.ktandroid/app/src/main/kotlin/org/bortnik/meteogram/SvgChartPlatformView.ktandroid/app/src/main/res/layout/meteogram_widget_weekly.xmle2e/README.mde2e/a11y_ids.jse2e/package.jsone2e/run-ci.she2e/specs/accessibility.e2e.jse2e/specs/home_happy_path.e2e.jse2e/wdio.conf.jslib/a11y_ids.dartlib/l10n/app_ar.arblib/l10n/app_be.arblib/l10n/app_bg.arblib/l10n/app_bn.arblib/l10n/app_bs.arblib/l10n/app_cs.arblib/l10n/app_da.arblib/l10n/app_de.arblib/l10n/app_el.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fi.arblib/l10n/app_fr.arblib/l10n/app_hi.arblib/l10n/app_hr.arblib/l10n/app_is.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_jv.arblib/l10n/app_ka.arblib/l10n/app_ko.arblib/l10n/app_mk.arblib/l10n/app_nl.arblib/l10n/app_no.arblib/l10n/app_pa.arblib/l10n/app_pl.arblib/l10n/app_pt.arblib/l10n/app_ro.arblib/l10n/app_sk.arblib/l10n/app_sq.arblib/l10n/app_sv.arblib/l10n/app_ta.arblib/l10n/app_tr.arblib/l10n/app_uk.arblib/l10n/app_vi.arblib/l10n/app_zh.arblib/screens/home_screen.dartlib/services/native_svg_service.dartlib/widgets/native_svg_chart_view.dart
| runs-on: ubuntu-latest | ||
| timeout-minutes: 45 | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the workflow file
fd -t f "e2e.yml" .github/workflows/Repository: timbortnik/widget
Length of output: 87
🏁 Script executed:
# Check the content of the workflow file to see the uses entries
cat -n .github/workflows/e2e.yml | head -120Repository: timbortnik/widget
Length of output: 4478
Pin GitHub Actions to full commit SHAs.
These uses: entries are tag-pinned (@v*) instead of commit-pinned. That weakens supply-chain guarantees and violates strict action-pinning policy.
Also applies to lines 29, 34, 49, 69, 79, 90, 104.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 24-27: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/e2e.yml at line 24, Replace all tag-pinned GitHub Actions
references in the e2e.yml file with full commit SHAs to satisfy supply-chain
security requirements. For each `uses:` entry that currently uses `@v*` tags
(such as `actions/checkout@v6`), replace the tag with the corresponding full
commit SHA of that action version. This needs to be applied to all affected
actions/checkout references and any other action references using tag-based
pinning throughout the workflow file.
Source: Linters/SAST tools
| @@ -0,0 +1,45 @@ | |||
| /// Stable accessibility identifiers for interactive UI elements. | |||
There was a problem hiding this comment.
Add the required BSL 1.1 license header.
As per coding guidelines, all Dart source files in lib/**/*.dart must include a BSL 1.1 license header.
🤖 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 `@lib/a11y_ids.dart` at line 1, The file lib/a11y_ids.dart is missing the
required BSL 1.1 license header. Add the BSL 1.1 license header at the very
beginning of the file before the existing documentation comment starting with
"/// Stable accessibility identifiers". The license header should be formatted
as a Dart block comment containing the appropriate copyright and license text.
Source: Coding guidelines
The error screen showed a generic "Failed to fetch weather data" — the HTTP status / network error was only logged then dropped. Thread the reason through all layers: - WeatherFetcher records it (e.g. "HTTP 429", "UnknownHostException: ...") in a lastFetchError field, cleared on success. - MainActivity returns it as the PlatformException message on FETCH_FAILED. - NativeSvgService.fetchWeather keeps it in lastFetchError. - _loadWeather throws it, so the on-screen _error shows the real cause. Verified on-device (offline): the screen now reads "Exception: UnknownHostException: Unable to resolve host ...". Also makes the CI E2E failures diagnosable — a rate-limited fetch shows "HTTP 429" instead of a generic message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Group the two spec files into a single wdio worker so they share one Appium session = one app launch = one Open-Meteo fetch per job, instead of an independent cold fetch per spec. The flaky read-timeouts have been hitting one session's fetch while the other succeeds; with a single fetch the job only needs that one request to land. Verified locally: 9 specs green in one worker (~38s, one session). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@android/app/src/main/kotlin/org/bortnik/meteogram/WeatherFetcher.kt`:
- Around line 33-34: The `lastFetchError` property at lines 33-34 is shared
mutable state that can be overwritten by concurrent fetch requests, causing one
request's error to incorrectly overwrite another's before being reported. Remove
this global `@Volatile` property entirely and refactor the fetch mechanism to
make errors request-scoped: modify the fetch operation to return or pass the
error through a callback mechanism (such as a Result type or callback parameter)
instead of storing it as shared state. Update all locations where
`lastFetchError` is currently assigned (lines 198-199, 210, and 214-215) to
instead return or communicate errors through the new request-scoped mechanism,
and update any location that reads `lastFetchError` (such as at line 105 in
MainActivity) to obtain the error from the fetch operation's result or callback
instead of accessing the shared property.
🪄 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: 83b58377-b3c4-4dd2-97b6-4d10de8b8f3b
📒 Files selected for processing (6)
android/app/src/main/kotlin/org/bortnik/meteogram/MainActivity.ktandroid/app/src/main/kotlin/org/bortnik/meteogram/WeatherFetcher.kte2e/specs/home_happy_path.e2e.jse2e/wdio.conf.jslib/screens/home_screen.dartlib/services/native_svg_service.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/wdio.conf.js
- lib/screens/home_screen.dart
…ipt, docs) - WeatherFetcher: replace the global @volatile lastFetchError with a per-call FetchResult(success, error); fetchFromApi now returns (JSONObject?, String?). Concurrent in-app + background-worker/alarm fetches can no longer clobber each other's error reason. MainActivity reads the returned fetchResult.error. - l10n: move clearSearch / hourlyChartLabel / weeklyChartLabel to the template position (after searchCityHint, before theme) in all 35 non-English locales, matching app_en.arb. Order-only change; values untouched. - e2e/run-ci.sh: fail fast (dump appium.log, kill server, exit 1) if Appium never reports ready, instead of running npm test against a dead server; guard the working-dir change with `cd ... || exit 1`. - e2e/README.md: remove stray trailing code fence. 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.
Add an end-to-end UI test harness that drives the installed APK on an Android emulator, plus the Semantics instrumentation that makes the app both testable and accessible (the two are the same work — UiAutomator2 reads the a11y tree).
App (lib/):
Tests (e2e/, no Flutter dependency):
CI:
Verified locally: make analyze clean, Dart + Kotlin tests pass, both specs green on an API 35 x86_64 emulator.
Summary by CodeRabbit