fix(android): attach GPS location on recordings - #12179
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Permission prompt exceeds capture timeout
- Moved the while-in-use permission prompt outside the 3s totalTimeout so the system dialog can complete before GPS/upload starts, and added a regression test for a grant slower than that budget.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 138043e. Configure here.
| // Prompt at record start so a skipped onboarding page still gets a fix | ||
| // when the activity is visible. deniedForever is not re-prompted. | ||
| permission = await _requestPermission(); | ||
| } |
There was a problem hiding this comment.
Permission prompt exceeds capture timeout
High Severity
_requestPermission now runs inside the existing 3s totalTimeout, and currentPositionTimeout grew to 2s without raising that budget. On the new denied path the system dialog almost always outlives 3s, so captureAndUpload returns early while the dialog is still up. Callers then proceed to mic permission / recording start, racing finalize and risking stacked permission requests.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 138043e. Configure here.
![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: 138043e6ac
ℹ️ 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".
| } on TimeoutException { | ||
| // A recent OS fix is preferable to losing the conversation location when | ||
| // a fresh GPS fix is slow indoors. | ||
| position = await _getLastKnownPosition(); |
There was a problem hiding this comment.
When the OS has a non-null fix from before the user moved, this unconditional last-known-first path uploads that old coordinate as the recording's location and never invokes _getCurrentPosition; the foreground refresher added in this commit uses the same ordering, so the app can repeatedly attach a stale location. Check position.timestamp against a bounded freshness window and request a current fix when it is expired, with a regression test for an old cached position; the rewritten fresh-first test currently provides no such guard or external basis for accepting every cached fix.
AGENTS.md reference: AGENTS.md:L126-L126
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented at d125f9e. lastKnownMaxAge is 5m with _isFresh. Stale last-known falls through to a current fix, then stale last-known if current fails. Tests: refreshes a stale last-known position before upload, uses a stale last-known when the current fix fails. Same window in foreground.dart.
| if (permission == LocationPermission.denied) { | ||
| // Prompt at record start so a skipped onboarding page still gets a fix | ||
| // when the activity is visible. deniedForever is not re-prompted. | ||
| permission = await _requestPermission(); |
There was a problem hiding this comment.
When a user takes more than the three-second totalTimeout to answer this newly added system prompt, Future.timeout returns false without cancelling _captureAndUpload; the recording callers then proceed while this operation can resume later and update Redis after capture has started or a short device conversation has already finalized. Keep the interactive permission request outside the bounded location-acquisition timeout, or fence the timed-out operation so it cannot complete late.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented. Permission request is unbounded; totalTimeout applies only to GPS/upload. Late _captureAndUpload completion is fenced with timedOut / isCancelled so a timed-out run does not upload. Same test as the timeout-vs-prompt case.
| if (permission == LocationPermission.denied) { | ||
| // Prompt at record start so a skipped onboarding page still gets a fix | ||
| // when the activity is visible. deniedForever is not re-prompted. | ||
| permission = await _requestPermission(); |
There was a problem hiding this comment.
For the first-run/skipped-onboarding case targeted here, HomePage checks permission first and skips ForegroundUtil.startForegroundTask() while it is denied, then immediately calls streamDeviceRecording, which can grant permission on this line. Nothing retries foreground-service startup after that grant, so these users receive the one-off upload but no periodic location refresh until the page or app is recreated, leaving long-running/background device capture on the stale initial snapshot. Start the Android foreground task after a successful newly requested grant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented. ConversationLocationCapture(onNewlyGranted: _startAndroidLocationForegroundTask) in capture_controller.dart. Hook fires on a new while-in-use/always grant only; tests assert it does not fire on deniedForever or an already-granted check.
| } catch (e) { | ||
| Logger.log('Fresh conversation location fix failed: $e'); | ||
| return false; |
There was a problem hiding this comment.
When no cached position exists and the fresh fix throws or times out, this new catch converts the failure into a silent fail-open return and recording continues without geolocation; the local log does not expose how often the Android fix remains broken in production. Record this through the shared fallback telemetry surface with an exhausted outcome (and instrument the equivalent foreground-task branch) rather than relying only on Logger.log.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining. docs/agents/fallback-telemetry.md is Python/Swift/Rust only — there is no Dart record_fallback surface. Adding a fake Dart telemetry path would not satisfy the contract. Exhausted GPS still fail-opens with a local log; recording continues.
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks @undivisible — solid root-cause analysis, and the permission posture here is exactly right (while-in-use only, no ACCESS_BACKGROUND_LOCATION, documented in the manifest).
The diagnosis holds up against the code, and the earlier "prompt inside the 3s timeout" concern is fixed at this head with a regression test. Two CI blockers are attributable to this PR's own changes, so requesting changes for those:
1. Dart Analyze & Tests — analyzer ratchet: unnecessary_null_comparison (2 new occurrences, baseline 0)
app/lib/services/capture/conversation_location_capture.dart:111—if (position == null) return false;is unreachable: if the current fix fails, the catch at 106–109 already returns; otherwisepositionis non-null. Please remove.app/lib/utils/audio/foreground.dart:44—if (locationData == null)is likewise dead: both branches either assign or return in their catch blocks. Please remove.
flutter test never ran — the ratchet gate precedes it. These should just be deleted rather than baselined (they're new dead code, not pre-existing debt).
2. Hygiene — failure-class-protocol: commit 440c2f8824 (fix(android): …) is missing its Failure-Class: FC-<slug> | new | none trailer. Your latest commit already carries Failure-Class: none; the first commit needs the same treatment — reword it (rebase) with either none or a real class from scripts/pr-preflight --suggest.
Two non-blocking observations, both verified in the code, worth addressing or consciously accepting:
- Stale last-known uploads:
conversation_location_capture.dart:98-110will upload any non-null cached position without a freshness check, so a fix from before the user moved can become the recording's location. A bounded freshness window onposition.timestamp(falling back to the current fix when stale) plus a regression test would make the attach trustworthy. - Foreground refresh gap:
app/lib/pages/home/page.dart:483-489starts the foreground location service only when permission was already granted at page load, so a fresh grant from the new record-start prompt leaves the periodic refresh unstarted until the page is recreated. A retry after a newly granted permission would close that.
On the Codex P1 suggestion to use record_fallback: not applicable here — the fallback-telemetry contract (docs/agents/fallback-telemetry.md) covers Python/Swift/Rust and there's no Dart surface for it today.
Left for human maintainer review: the new runtime while-in-use prompt at record start is a UX/product decision worth explicit sign-off.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
added
needs-maintainer-review
0d33165 to
d125f9e
Compare
|
@Git-on-my-level both CI blockers are addressed at d125f9e: dead Non-blocking: 5m last-known freshness + FGS start after a newly granted while-in-use are in at the same SHA. Codex P1 ( Please re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d125f9eceb
ℹ️ 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".
| if (permission == LocationPermission.denied) { | ||
| // Prompt at record start so a skipped onboarding page still gets a fix | ||
| // when the activity is visible. deniedForever is not re-prompted. | ||
| permission = await _requestPermission(); |
There was a problem hiding this comment.
When the home page opens with no connected device and location permission is denied, HomePage still unconditionally calls streamDeviceRecording(device: connectedDevice) (page.dart:498-502), and that method calls captureAndUpload() even when device is null (capture_controller.dart:1670-1681). This new request therefore shows the sensitive system location prompt on ordinary home-page entry on both Android and iOS, rather than at the claimed recording start; users can deny it without context and may later be unable to attach location when they do record. Gate permission prompting on an actual recording/device path, while allowing noninteractive callers to keep the former check-only behavior.
Useful? React with 👍 / 👎.
|
Re-reviewed at d125f9e — thanks @undivisible, both blockers from my earlier review are resolved and verified at this head:
On the two red checks ( One product item stays open for a human maintainer (label kept): by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Dismissed as resolved at d125f9e: dead Position? null checks removed (late final + catch fallback, analyzer green) and Failure-Class: none trailers present on both fix commits.
Git-on-my-level
added
the
positive-signal
Prefer last-known and medium-accuracy fixes, and request while-in-use at record start, so Android conversation JSON gets coordinates without ACCESS_BACKGROUND_LOCATION. Failure-Class: none
Add unit coverage for while-in-use prompting at record start and last-known-first upload so Android GPS attach does not require a device.
The while-in-use prompt at record start lived inside the 3s totalTimeout, so the system dialog almost always lost and callers moved on to microphone permission while it was still showing. Keep the interactive grant unbounded, then apply totalTimeout only to the GPS fix and upload. Failure-Class: none
Address review: freshness window, permission-timeout fence, FGS after a newly granted while-in-use, and drop dead null checks the analyzer ratchet flagged.
Recording is the tap; location is metadata. Stop awaiting captureAndUpload() on the record-start critical path so the OS location dialog cannot hold RecordingState.initialising or stack in front of the microphone prompt. Failure-Class: none
d125f9e to
a4f8f1b
Compare
|
@Git-on-my-level product call on the remaining item (your re-review at #12179 (comment)): do not block. Recording is the tap; location is metadata. We will not hold At
Please drop by AI on behalf of Max — if you need Max's attention urgently, please @undivisible and escalate with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4f8f1b583
ℹ️ 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".
| if (position == null) return false; | ||
| if (isCancelled()) return false; | ||
|
|
||
| return _upload( |
There was a problem hiding this comment.
When location acquisition completes within three seconds but the PATCH is slow or queued, Future.timeout returns false while this _upload future continues running because Dart timeouts do not cancel their source future. Fresh evidence despite the earlier thread resolution is that isCancelled() is checked only before _upload starts; makeApiCall permits writes to run for up to 300 seconds with a retry, and backend/routers/users.py::set_user_geolocation caches requests in arrival order without rejecting older timestamps. An earlier recording can therefore update Redis after a later recording has started or finalized; keep upload completion outside the acquisition timeout or sequence/cancel late uploads by capture identity.
Useful? React with 👍 / 👎.
|
Reviewed at
One item remains before the label comes off, and it's the same placement question in new clothes — the unresolved bot finding from 09:22, still live at this head: Minor, non-blocking: the 11:23 bot note on a slow PATCH outliving the 3s budget is real but low-impact — the by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Confidence 4/5 (diff slightly over 300 lines, everything else checks out): clear multi-part root cause for missing Android GPS (permission never requested, timeout too short, no fallback on error), tests added, CI green. Verified not already fixed on main.
|
@Git-on-my-level leftover from #12179 (comment) is implemented at Homepage / The short-conversation miss (location PATCH landing after a brief recording ends) remains the accepted trade-off. This PR already merged at by AI on behalf of Max — if you need Max's attention urgently, please @undivisible and escalate with |
4 participants
Report
Android conversation/recording JSON always had
geolocation: null/location=nil. The reporter suspected a missing always-on /ACCESS_BACKGROUND_LOCATIONgrant. They also left the app in the foreground, turned on the recorder, and still got nil.Root cause
Not missing always-on. The attach path already exists (
ConversationLocationCapture→PATCH /v1/users/geolocation→ Redis snapshot on finalize) and is awaited on phone-mic and device record start.What actually dropped Android coordinates:
denied, so it uploaded nothing even with the activity visible.PRIORITY_HIGH_ACCURACY. A cold GPS fix routinely exceeds 1s.getLastKnownPosition()is often null on Android until this app has received a fix, so both legs failed in the foreground.Background/always-on is not required for a visible recorder. Android already declares
FOREGROUND_SERVICE_LOCATIONand only asks for while-in-use.ACCESS_BACKGROUND_LOCATIONis still not requested (Play Store prominent-disclosure). iOS "Always" remains iOS-only for BGTask windows.Fix
denied(notdeniedForever).TimeoutException.ACCESS_BACKGROUND_LOCATIONis intentionally not used.Test plan
flutter test test/unit/conversation_location_capture_test.dart(no device)deniedForeverdeniedgeolocationon the conversation JSONFailure class (fixes)
Failure-Class: none
Note
Medium Risk
Changes permission prompts and location acquisition at recording start and in the foreground location service; scope is limited to geolocation attach, not new background permissions.
Overview
Fixes null conversation geolocation on Android when recording in the foreground with only while-in-use location—not missing
ACCESS_BACKGROUND_LOCATION.ConversationLocationCapturenow requests while-in-use permission at record start when status isdenied(notdeniedForever). Capture tries last-known first (with Android LocationManager fallback), then a medium-accuracy current fix with a 2s budget; any current-fix failure is handled without blocking recording. Docs in code and AndroidManifest state that background location is intentionally not requested.The foreground-service location refresh uses the same last-known → medium-accuracy pattern, with errors reported to the main isolate.
Unit tests cover permission prompting, last-known-first behavior, and failure paths.
Reviewed by Cursor Bugbot for commit 138043e. Configure here.