Repository navigation
fix(app): restore green Flutter tests on main (#21302 #21340 #21341 #21343 follow-ups) - #21425
Merged
Git-on-my-level merged 2 commits intoOct 13, 2026
Merged
Conversation
…sedHardware#21340 BasedHardware#21341 BasedHardware#21343 follow-ups) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Git-on-my-level
added
the
ci:full
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b3da84871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // TextPainter's sample glyphs can be shorter than the actual app name and | ||
| // localized category/rating lines. Keep a line-height of vertical slack so | ||
| // fallback fonts and platform font metrics do not clip the final line. | ||
| final double targetItemHeight = max(85.0, 16 + max(52.0, rowText + 32) + 1); |
There was a problem hiding this comment.
This changes shipped category-card dimensions, but the reviewed commit explicitly records UI-Evidence: none and reports only widget tests. The app playbook requires agent-flutter verification after every Flutter UI edit, so run the affected large-text category screen through that check before landing; otherwise the hard-coded 32-point allowance has not been checked against the runtime font and platform metrics that motivated the fix.
AGENTS.md reference: app/AGENTS.md:L148-L148
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 13, 2026
Merged
Git-on-my-level
added a commit
that referenced
this pull request
Oct 14, 2026
…21430) ## Summary Closes the ways the app can show a pendant as Disconnected while native BLE holds a live, ready link. In that state, a pendant that is visibly connected in iOS Settings (blue light) never streams audio until the app is killed. **Root cause (observed on a real iPhone, prod 1.0.563+1377).** The app opened at 22:42:30 and sent one `connect` request at 22:42:34. Dart's launch connect gave up at its 60s deadline. The pendant arrived at 22:43:54, 80s after the request, and native finished service discovery at 22:43:56. After that the app process sent zero GATT reads, writes, or notify subscriptions until it was restarted. The only traffic was the native 10s RSSI poll, which runs only while `peripheral.state == .connected`. This is the pre-#21302 path: the timeout tore the connection down and disposed its transport. That dispose unregistered the peripheral from `BleBridge`, but `NativeBleTransport.disconnect()` returned early because Dart's mirror already said disconnected, so it never sent `unmanageDevice`. Native kept connecting and delivered device-ready to nobody. #21302 (merged 2026-10-13 14:45 UTC) keeps the transport on timeout. A new provider-level test reproduces the report: it fails on `ce3826d2fd^` and passes on main. This PR removes the footguns that made that path possible and still leave sibling paths open on main: 1. **Owned bridge registration.** `BleBridge.registerPeripheral` now returns a disposer that unregisters only while the caller still owns the peripheral. Before, unregistration was by id alone, so a retired transport disposed after its replacement registered would silence the replacement's device-ready. 2. **Disconnect releases native based on intent, not the mirror.** `NativeBleTransport.disconnect()` now calls `unmanageDevice` unless this transport already released native. Before, it skipped the call whenever Dart's mirrored state said disconnected, even though native keeps connecting after a Dart-side timeout. A device-ready already in flight after release is ignored, so it cannot re-adopt the link behind an explicit disconnect. 3. **`DeviceService.disconnectDevice` disposes the transport it drops**, matching `_teardownConnection`. Before, the dropped transport stayed registered and could swallow the next device-ready: tap Disconnect while native is reconnecting, native reconnects, and the ready lands on an orphan whose connection no longer listens. 4. **Foreground reconcile.** A new `NativeLinkReconciler`, bound in `main.dart`, runs on app resume. If the provider shows the paired device disconnected but native reports its peripheral connected, it re-checks that the same id is still paired and still disconnected after the async probe. It then calls `DeviceService.ensureConnection(id, force: true)` for exactly that id and publishes the result through `DeviceProvider.onDeviceConnectionStateChanged`, the entry point native events use. That covers both a missing transport and a `DeviceService` connection the provider never heard about. Native answers `manageDevice` for a connected peripheral by replaying device-ready (iOS `replayReady` / `hydrateReady`, Android `adoptConnectedDevice`), so this does not touch the radio and never starts a physical connect. It covers any future lost-ready path, not one cause. Also: - If `unmanageDevice` fails, the transport stays unreleased, so the release can be retried and native readies are still adopted. - Every transport's `_updateState` ignores events after `dispose()` instead of throwing on a closed controller. - `DeviceProvider._handleDeviceConnected` admits each physical connect once, keyed on `(connection, connectionEpoch)` and cleared whenever the mirror disconnects. Overlapping connected events (a native connect still initializing plus a resume reconcile) no longer run capture start, battery listeners or WAL attach twice. Line-Count-Exception: app/lib/providers/device_provider.dart | 1488 -> 1576 | the per-connect admission guard and the readoption pairing constraint (armed admission, gated persists and setup continuation) live in the provider's own connect entry point; the reconcile logic itself is split into native_link_reconciler.dart UI-Evidence: none -- no visible UI change; main.dart only binds the device provider app-lifecycle listener ## Verification - `flutter test test/providers/device_provider_late_ready_test.dart`: 14 tests. Each invariant was mutation-checked: reverting any one of the ten fixes (owner-scoped bridge release, intent-based disconnect, the ready guard after release, dispose in `disconnectDevice`, the reconcile native probe, the held-link republish, the reconcile id re-check, retryable failed release, the per-connect admission guard, and the admission reset on any mirror disconnect) fails exactly its test. The late-ready provider test also fails on `ce3826d2fd^`. - Independent review: codex `gpt-6.1-sol` via agentctl (read-only brief). Five rounds via `agentctl continue` on one session, each re-reviewing the delta. Rounds 1–4 found eight P2 defects in earlier revisions, all fixed and pinned by tests above: the reconcile didn't republish a link `DeviceService` already held; the probed id could differ from the connected id; a failed release was recorded as released; synthetic publishes across awaits (double admission, overriding a real disconnect, publishing a re-paired device); overlapping admissions for one physical connect; and an admission latch surviving provider-only disconnects. Round 5: "Nothing blocks merge in this delta." - `bash app/test.sh` (full hermetic suite + spine) with Flutter 3.44.5 on macOS: spine 138/138; suite 5,500 passed, 21 skipped, 4 failed. The four failures (`apps_category_section_large_text_test` ×2, `ai_app_generator_large_text_test`, `settings_search_header_stability_test`) fail identically on unmodified `origin/main` and are fixed by the open #21425. That PR also fixes `device_service_connection_failure_test`, which this PR leaves alone to avoid a conflict. - Changed-file `flutter analyze`: no new issues (two pre-existing infos). Not verified on device: the resume reconcile has not been exercised on a physical phone. The incident evidence (bluetoothd/runningboard logs) comes from the reporting iPhone. ## Product invariants affected - INV-DATA-1 — production data-plane routing is unchanged; only the device provider's construction in `main.dart` changes. ## Failure class Failure-Class: FC-superseded-connection-mutates-live-session 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/21430?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="View guided diff" src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a){kind=link}
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.
Summary
Restore green Flutter tests on main for follow-ups to #21302, #21340, #21341, and #21343.
Per-test root cause and fix
app/test/services/device_service_connection_failure_test.dart— The native reconnect contract was already preserved byDeviceService; the test emitted a broadcast stream event and asserted state in the same turn, before the asynchronous listener ran. Verdict: test timing was wrong. Await one event-loop turn after emitting the event. This does not change connection or reconnect behavior.app/test/widgets/apps_category_section_large_text_test.dart— The measurement usedTextPainter('Ag')with the card styles but omitted the inheritedDefaultTextStyle, which supplies line metrics used by the actualTextwidgets. The card renders a 60 pt icon, threemaxLines: 1/ellipsis labels, 2 pt gaps, and 8 pt vertical padding on both sides; there is no explicit strut or line-height override. Verdict: product layout was wrong. Measure single lines with the inherited style, scaler, direction, one-line/ellipsis settings, and the card's actual body/footnote/caption styles. Derive row height asmax(85, 16 + max(60, textStack) + 1), with no extra slack. The test also corrects its expected label count (the heading plus three cards all sayProductivity) and pins rendered row targets and fit at all four scales.+32slack)The omitted inherited line metrics account for 18, 19, 23, and 25 pt of stack height respectively. The corrected targets include the actual 16 pt padding and 1 pt clearance. iPhone 17 Pro simulator (iOS 26.5, 393x852 logical viewport, 3x) screenshots at 1.0x were captured and opened: the section is 389 pt tall before and 333 pt after; labels stay single-line and ellipsized. The 85 pt default target returns to the original baseline.
app/test/widgets/settings/ai_app_generator_large_text_test.dart— The English localization includes an ellipsis (Try something like…), while the test expected the text without it. Verdict: test expectation was wrong. Match the localized label, verify the horizontal suggestion list, and retain the loaded fallback prompt assertion. The product strip already scales with text size. Test passes across all four configured scales.app/test/widgets/settings/settings_search_header_stability_test.dart— The harness did not provideUsageProvider, whichSettingsDrawerreads while building. The header was never mounted, so the key finder failed before measuring layout. Verdict: test harness was incomplete. Supply the provider; the existing 48 pt header and alignment behavior pass.Verification
bash test.sh --reporter expanded: 5,493 passed, 21 existing skips, no failures.bash scripts/analyze_ratchet.sh: passed.Failure-Class: none
UI-Evidence: none -- Opened iOS simulator before/after captures are local at
/private/tmp/main-red-tests/before/category-1-0.pngand/private/tmp/main-red-tests/after/category-1-0.png; automated widget assertions cover the 1.0x through 1.5x targets.🤖 Generated with Claude Code