feat(reminder): add system geofencing location adapter - #268
Merged
MeteorsLiu merged 6 commits intoAug 20, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This was referenced Aug 17, 2026
LUPENGHAN
force-pushed
the
feature/reminder-adapters-location
branch
from
August 17, 2026 05:22
c7f7c0b to
8edee46
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-location
branch
from
August 17, 2026 07:00
8edee46 to
24b8172
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-location
branch
from
August 17, 2026 07:25
24b8172 to
537829d
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-location
branch
from
August 17, 2026 07:52
537829d to
cbd14c5
Compare
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 17, 2026
Code review (PR 1024XEngineer#268): syncRegions() -- the function that actually registers the OS-level geofences -- had three completely silent failure points (missing foreground permission, missing background permission, startGeofencingAsync throwing), plus a fourth in getCurrentSample(). Android commonly grants only foreground location by default; background ("Allow all the time") needs a separate, harder-to-get grant, so a schedule can silently sit unarmed forever with zero signal anywhere that it happened. Added console.warn at all four points. Not fixed: geofenceTask.ts's deliverHeadlessGeofenceEvent() sends the headless notification before persisting geofence_armed=0/disposition state, so a process kill or a thrown UPDATE in that narrow window can re-deliver the same notification. Flipping the order avoids the duplicate but risks silently dropping the reminder entirely if the notification call itself then fails -- that tradeoff needs a product call, not a quick patch, so it's left as-is and documented here.
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 19, 2026
Code review (PR 1024XEngineer#268): syncRegions() -- the function that actually registers the OS-level geofences -- had three completely silent failure points (missing foreground permission, missing background permission, startGeofencingAsync throwing), plus a fourth in getCurrentSample(). Android commonly grants only foreground location by default; background ("Allow all the time") needs a separate, harder-to-get grant, so a schedule can silently sit unarmed forever with zero signal anywhere that it happened. Added console.warn at all four points. Not fixed: geofenceTask.ts's deliverHeadlessGeofenceEvent() sends the headless notification before persisting geofence_armed=0/disposition state, so a process kill or a thrown UPDATE in that narrow window can re-deliver the same notification. Flipping the order avoids the duplicate but risks silently dropping the reminder entirely if the notification call itself then fails -- that tradeoff needs a product call, not a quick patch, so it's left as-is and documented here.
LUPENGHAN
force-pushed
the
feature/reminder-adapters-location
branch
from
August 19, 2026 06:24
74b59a6 to
74a3be2
Compare
LUPENGHAN
marked this pull request as ready for review
August 19, 2026 06:24
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed the complete fixed diff for the new Expo geofence adapter, including registration, lifecycle synchronization, headless delivery, pending-event replay, and the location provider contract. The geofence state machine and persistence paths are coherent, but the adapter currently gates registration on a permission that this app never requests. git diff --check passed; the local frontend dependency tree is absent, so npm run typecheck could not execute (tsc: Permission denied).
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 19, 2026
syncRegions() checked getForegroundPermissionsAsync()/getBackgroundPermissionsAsync() and silently gave up on the geofence registration whenever either was not 'granted', but never called requestForegroundPermissionsAsync()/ requestBackgroundPermissionsAsync(). On a fresh install permissions are undetermined by default, so every geofence registration attempt hit this branch and no location reminder ever got a system geofence -- fennoai review comment on PR 1024XEngineer#268. Now each check requests the permission once when it isn't already granted, before giving up. Foreground must be requested before background (Android requirement), matching the existing check order. Also adds Jest coverage for ExpoLocationMonitor.ts (97% statements, was ~9%) and geofenceTask.ts (52% statements, was ~9%) to address the Codecov patch- coverage gate. geofenceTask.ts's deliverHeadlessGeofenceEvent()/ persistPendingEvent()/drainPendingGeofenceEvents() reach expo-sqlite(/kv-store) and expo-notifications through a dynamic import() by design (loadStorage()'s "懒加载" comment) -- this project's Jest config has no --experimental-vm-modules, so a bare dynamic import throws here and is swallowed by the source's own try/catch (built for a genuinely-unavailable native module), making that whole branch unreachable from a unit test without a project-wide Jest runtime change. Only the synchronous routing in front of that boundary is covered; this is documented in the test file.
8 tasks
This was referenced Aug 19, 2026
Part of 1024XEngineer#263. ExpoLocationMonitor implements LocationMonitorPort via expo-location's OS-level geofencing (Android GeofencingClient / iOS CLCircularRegion) -- system manages battery/background reliability instead of app-side continuous polling. geofenceTask.ts is the TaskManager headless task definition; frontend/index.ts imports it at module scope so headless launches (app process killed, geofence crossed) still pick it up. ExpoLocationProvider/LocationProvider gain the location-sample reuse used by the voice assistant's location_search tool. Only depends on application interfaces already on main -- independent of the notifications/audio/data-layer PRs in this stack. Removes MockLocationMonitor. Does not include NativeLocationMonitor / BaiduLocationBridge (Baidu SDK continuous polling) -- dropped entirely per 1024XEngineer#263's decision to use system geofencing instead.
Code review (PR 1024XEngineer#268): syncRegions() -- the function that actually registers the OS-level geofences -- had three completely silent failure points (missing foreground permission, missing background permission, startGeofencingAsync throwing), plus a fourth in getCurrentSample(). Android commonly grants only foreground location by default; background ("Allow all the time") needs a separate, harder-to-get grant, so a schedule can silently sit unarmed forever with zero signal anywhere that it happened. Added console.warn at all four points. Not fixed: geofenceTask.ts's deliverHeadlessGeofenceEvent() sends the headless notification before persisting geofence_armed=0/disposition state, so a process kill or a thrown UPDATE in that narrow window can re-deliver the same notification. Flipping the order avoids the duplicate but risks silently dropping the reminder entirely if the notification call itself then fails -- that tradeoff needs a product call, not a quick patch, so it's left as-is and documented here.
syncRegions() checked getForegroundPermissionsAsync()/getBackgroundPermissionsAsync() and silently gave up on the geofence registration whenever either was not 'granted', but never called requestForegroundPermissionsAsync()/ requestBackgroundPermissionsAsync(). On a fresh install permissions are undetermined by default, so every geofence registration attempt hit this branch and no location reminder ever got a system geofence -- fennoai review comment on PR 1024XEngineer#268. Now each check requests the permission once when it isn't already granted, before giving up. Foreground must be requested before background (Android requirement), matching the existing check order. Also adds Jest coverage for ExpoLocationMonitor.ts (97% statements, was ~9%) and geofenceTask.ts (52% statements, was ~9%) to address the Codecov patch- coverage gate. geofenceTask.ts's deliverHeadlessGeofenceEvent()/ persistPendingEvent()/drainPendingGeofenceEvents() reach expo-sqlite(/kv-store) and expo-notifications through a dynamic import() by design (loadStorage()'s "懒加载" comment) -- this project's Jest config has no --experimental-vm-modules, so a bare dynamic import throws here and is swallowed by the source's own try/catch (built for a genuinely-unavailable native module), making that whole branch unreachable from a unit test without a project-wide Jest runtime change. Only the synchronous routing in front of that boundary is covered; this is documented in the test file.
Also moves the require() eslint-disable comment onto the same line as the call it covers, so a future prettier reflow of the surrounding cast expression can't silently detach it again.
…istanbul-ignore deliverHeadlessGeofenceEvent()/openHeadlessDatabase()/loadNotifications()/ ensureAndroidChannel() and the "storage available" continuations of drainPendingGeofenceEvents()/persistPendingEvent() can't be exercised by a unit test in this project's Jest config (no --experimental-vm-modules; see the previous commit's message and the new file-header comment) -- their entry point is always the dynamic import() that loadStorage()/ openHeadlessDatabase()/loadNotifications() wrap, and that import always throws here regardless of mocking. Wrapping each unreachable continuation in a bare block with a leading istanbul-ignore comment (no behavior change, verified against the full test suite) instead of leaving it silently uncovered: Codecov's patch coverage was flagging real, honest gaps mixed in with this structural one, which made it hard to tell which lines actually needed a test. Patch coverage across ExpoLocationMonitor.ts + geofenceTask.ts goes from 74.65% to 97.31%; the one line left uncovered in each file is an ordinary defensive catch-all that just never triggers, not an environment gap.
LUPENGHAN
force-pushed
the
feature/reminder-adapters-location
branch
from
August 19, 2026 13:18
6c70c54 to
dab92ca
Compare
MeteorsLiu
reviewed
Aug 20, 2026
"Enqueue" implied distinct queued work items processed in order; every call actually just chains another syncRegions() re-read of the current watches state onto syncChain, purely to keep calls from running concurrently. That's serialization, not a queue -- matches the existing chainPlayback()/playbackChain pattern in AssistantContinuousConversationService.ts. Review comment from MeteorsLiu on PR 1024XEngineer#268.
MeteorsLiu
approved these changes
Aug 20, 2026
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 20, 2026
…neer#268), fix conflicts 1024XEngineer#270 was still based on 1024XEngineer#266's merge point; 1024XEngineer#267 and 1024XEngineer#268 merged since then and both touched createAppServices.ts (real port swaps) and, for 1024XEngineer#267, the same reminder = new LocalReminderApplication(...) line 1024XEngineer#270 had already changed independently. Resolved by keeping every adapter's real implementation (both sides had already done their own swap for different ports) instead of picking one branch's version. AppRoot.tsx/AppRoot.test.tsx conflicts were two unrelated sets of new props (protectedClient from main, reminderState/scheduleReader from this branch) threaded through the same component chain -- combined both, dropped one genuinely unused import (AuthController) picked up along the way. Also fixed two real bugs the rebase surfaced, not introduced by it: - The branch's own "binds the reminder SQLite adapters" test passed authController={controller} to AppRoot, a prop it hasn't accepted since the pre-1024XEngineer#266 composition root shape; controller was otherwise unused. Collapsed to the one authenticated `services` instance every other test in the file already uses. - mockedCreateScheduleSnapshotPreparation's repository stub was a bare {}, fine before this branch existed. AppRoot's ready-state effect now calls scheduleReader.refresh() unconditionally, which calls through to repository.listSchedules() -- gave the stub real getSchedule/ listSchedules methods matching the ScheduleLocalRepository mock already used elsewhere in the same file. Verified: tsc/eslint/prettier clean, jest 464/464, vitest 87/87.
MeteorsLiu
pushed a commit
that referenced
this pull request
Aug 20, 2026
* feat(reminder): add SQLite-backed reminder data layer Part of #263. SqliteLocalScheduleReader / SqliteReminderStateStore read and persist against the real local database (ScheduleLocalRepository) instead of in-memory fixtures; geofence_radius_meters is hardcoded to 200m for now (known simplification, see Issue #263 Out of Scope). InMemoryLocalScheduleReader is kept as a non-persisted alternative implementation of the same port. LocalScheduleWriter's post-write hook refreshes the new reader after a voice-driven schedule mutation lands. Only depends on application interfaces already on main and the existing ScheduleLocalRepository -- independent of the audio/location/ notifications adapter PRs in this stack. Removes MockLocalScheduleReader, MockReminderApplication, MockReminderDispositionSync, MockReminderStateStore, mockReminderSchedules. * chore(reminder): remove unused LocalSystemNotification placeholder Code review (PR #270): LocalSystemNotification was a no-op SystemNotificationPort placeholder, exported but never imported anywhere -- fully superseded by the real ExpoSystemNotification adapter from the notifications PR, which is what createAppServices.ts on the wiring branch actually uses. Dead code, removed. * fix(reminder): wire SQLite adapters into app lifecycle * test(reminder): cover local data adapters * test(reminder): cover data adapters in Jest * fix(reminder): rebase onto upstream/main (#266-#268), fix conflicts #270 was still based on #266's merge point; #267 and #268 merged since then and both touched createAppServices.ts (real port swaps) and, for #267, the same reminder = new LocalReminderApplication(...) line #270 had already changed independently. Resolved by keeping every adapter's real implementation (both sides had already done their own swap for different ports) instead of picking one branch's version. AppRoot.tsx/AppRoot.test.tsx conflicts were two unrelated sets of new props (protectedClient from main, reminderState/scheduleReader from this branch) threaded through the same component chain -- combined both, dropped one genuinely unused import (AuthController) picked up along the way. Also fixed two real bugs the rebase surfaced, not introduced by it: - The branch's own "binds the reminder SQLite adapters" test passed authController={controller} to AppRoot, a prop it hasn't accepted since the pre-#266 composition root shape; controller was otherwise unused. Collapsed to the one authenticated `services` instance every other test in the file already uses. - mockedCreateScheduleSnapshotPreparation's repository stub was a bare {}, fine before this branch existed. AppRoot's ready-state effect now calls scheduleReader.refresh() unconditionally, which calls through to repository.listSchedules() -- gave the stub real getSchedule/ listSchedules methods matching the ScheduleLocalRepository mock already used elsewhere in the same file. Verified: tsc/eslint/prettier clean, jest 464/464, vitest 87/87.
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
2 participants
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.
关联 Issue
Part of #263
依赖 #266（先合）、#267（先合）。跟 #269/#270 改动内容互相独立，可以并行审；但四个适配器
PR 都改
createAppServices.ts（本 PR 只动location那一行：MockLocationMonitor→ExpoLocationMonitor），需按 #267→#268→#269→#270 顺序依次合并，合并前会 rebase 到上一个已合并 PR 的 main。
改动
ExpoLocationMonitor（实现LocationMonitorPort）：用 expo-location 的系统原生地理围栏（Android
GeofencingClient/ iOSCLCircularRegion）替代应用侧连续定位轮询，围栏进出交给系统管
geofenceTask.ts：TaskManagerheadless task 定义，处理围栏事件、写回 disposition、发送headless 通知；
frontend/index.ts在根组件注册前于模块顶层import它，确保TaskManager.defineTask注册在顶层作用域，App 被系统杀死后 headless 启动也能收到事件ExpoLocationProvider/LocationProvider：补上语音助手location_search工具复用最近一次定位样本的逻辑
createAppServices()里location换成ExpoLocationMonitor；其它不属于这个PR 的端口（
schedules/time/通知/state/dispositionSync）仍是 MockMockLocationMonitor及其测试修复（review 中发现）
syncRegions()原来只getForegroundPermissionsAsync()/getBackgroundPermissionsAsync()检查权限状态，从不调用对应的request*PermissionsAsync()；新装用户权限默认是
undetermined，导致每次围栏注册都在权限检查这一步直接放弃，地点提醒永远拿不到系统围栏。修复：未授权时先请求一次再放弃；Android 要求前台权限必须先于后台权限被请求，
修复保持了原有的检查顺序（
84492a5）syncRegions()/getCurrentSample()原来 4 个失败分支（前台/后台权限缺失、startGeofencingAsync()抛错、getCurrentSample权限缺失）全部静默吞掉，围栏注册不上没有任何信号；补了
console.warn（74a3be2）验证
npx tsc --noEmit、npx eslint .、npx prettier --check .全绿codecov/patch 均 pass）
ExpoLocationMonitor.ts（97% 语句覆盖，权限请求/围栏注册/失败分支）和geofenceTask.ts（52% 语句覆盖）；geofenceTask.ts里deliverHeadlessGeofenceEvent()/openHeadlessDatabase()/loadNotifications()/ensureAndroidChannel()及drainPendingGeofenceEvents()/persistPendingEvent()的"存储可用"延续分支走的是动态
import()，这个项目的 Jest 配置没开--experimental-vm-modules，动态import 在测试环境里必抛错、被源码自己的 try/catch 吞掉，结构性测不到——标了
istanbul-ignore并在测试文件里注明原因，不是遗漏本轮不含（见 #263 Out of Scope）
NativeLocationMonitor/BaiduLocationBridge）整个删除，不接入也不留死代码