feat(scheduling): TASK-299.2 bidirectional reminder sync#714
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…t turn control, context viewer)
…snapshot API, keybinding, redaction
… questions, command palette naming
…ssistant fallback, tests
…r handling, per-step diagnostics
…sage, status handling
…nitial snapshot in worker
…ers for TASK-299.2
…ons for TASK-299.2
…idempotency stripping
…mapping and docstrings
…then-transaction sync
…ner switcher into workbench
…d sync engine null row
rmusser01
force-pushed
the
feature/task-299.2-bidirectional-reminder-sync
branch
from
July 20, 2026 03:42
8e0f740 to
72fe84b
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation for new features, including the enhanced file picker, the scheduling module, and the watchlists TUI screen. Additionally, it applies extensive formatting and import cleanup across numerous test files. As there are no review comments provided, I have no feedback to offer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
PR Summary by QodoBidirectional reminder sync with tldw_server (TASK-299.2)
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
39 rules 1. entity_table used unvalidated in SQL
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| async def sync_now(self) -> None: | ||
| """Trigger a full sync for the current owner.""" | ||
| await self.sync_engine.sync_now() | ||
| async def sync_now(self, owner_id: str | None = None) -> None: |
There was a problem hiding this comment.
6. Local owner sync pollution 🐞 Bug ≡ Correctness
SchedulingService.sync_now() always runs a server-backed sync for the provided owner_id, so triggering sync while owner_id is "local" will still pull server reminders and write them into the local owner scope. This can mix server reminders into the local queue and corrupt isolation between local and server owners.
Agent Prompt
### Issue description
`SchedulingService.sync_now()` always delegates to `SyncEngine.sync_now()` regardless of whether the current owner is local or server-scoped. Because `SyncEngine._network_phase()` unconditionally pulls server reminders and applies them under the passed `owner_id`, calling sync while `owner_id == "local"` can import server reminders into the local owner scope.
### Issue Context
- `_use_server()` already defines when server operations should run (`owner_id` starts with `server:`), but `sync_now()` does not use it.
- The UI calls `service.sync_now(service.owner_id)` without checking owner/source.
### Fix Focus Areas
- tldw_chatbook/Scheduling/services/scheduling_service.py[269-276]
- tldw_chatbook/Scheduling/services/sync_engine.py[197-217]
- tldw_chatbook/UI/Screens/scheduling/schedules_workbench.py[569-595]
### Expected fix shape
- In `SchedulingService.sync_now()`: no-op (or raise a user-facing error) unless `target_owner.startswith("server:")`.
- Defense-in-depth: in `SyncEngine.sync_now()` / `_network_phase()`, reject non-`server:` owners (so callers cannot accidentally sync into local scope).
- In UI: disable/guard the Sync action when owner is local (optional but helps prevent misuse).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| else None | ||
| ) | ||
| if pending_mutation and pending_mutation.get("fields"): | ||
| if resolution == "server": |
There was a problem hiding this comment.
8. Server conflict leaves mutation 🐞 Bug ☼ Reliability
SyncEngine.resolve_conflict(..., resolution="server") updates/deletes the local record but does not clear any existing pending_mutations for that local_id, so stale mutations may remain after a server-authoritative resolution. If such a mutation still exists, the next sync can replay discarded local changes or repeatedly fail on the same stale payload.
Agent Prompt
### Issue description
In `SyncEngine.resolve_conflict()`, choosing `resolution == "server"` makes the server state authoritative but does not delete pending mutations for the affected record. If pending mutations still exist, they can be replayed after the conflict is resolved, undermining the selected resolution or creating repeated sync errors.
### Issue Context
- `ScheduledTasksDB` provides `delete_pending_mutation_for_record()`.
- `resolve_conflict()` already computes `pending_mutation`, but only uses it on the "local" branch.
### Fix Focus Areas
- tldw_chatbook/Scheduling/services/sync_engine.py[341-412]
- tldw_chatbook/Scheduling/db/scheduled_tasks_db.py[1125-1139]
### Expected fix shape
- In the `resolution == "server"` branch, delete pending mutations for `(local_id, primitive, owner_id)`.
- Consider also clearing/adjusting tombstones depending on server_state (e.g., if server says deleted, ensure no leftover delete/create mutations remain).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| pending_local_ids, | ||
| mutations, | ||
| ) = await self._network_phase(target_owner) | ||
| except ServerClientError as exc: |
There was a problem hiding this comment.
9. Sync failures reported success 🐞 Bug ◔ Observability
SyncEngine.sync_now() catches ServerClientError, records it, and returns normally instead of
surfacing failure to the caller. SchedulesWorkbench treats normal return as success and posts
SyncCompleted ("Sync completed.") even when sync actually failed.
Agent Prompt
### Issue description
`SyncEngine.sync_now()` swallows `ServerClientError` by recording a sync error and returning, which makes the operation appear successful to callers. The UI (`SchedulesWorkbench._run_sync`) posts `SyncCompleted` after `await service.sync_now(owner_id)` completes without raising, producing a misleading success notification.
### Issue Context
- Errors are persisted in `sync_errors`, but callers currently have no explicit success/failure signal.
### Fix Focus Areas
- tldw_chatbook/Scheduling/services/sync_engine.py[106-126]
- tldw_chatbook/UI/Screens/scheduling/schedules_workbench.py[584-599]
### Expected fix shape
- Change the sync API to return an explicit result (e.g., `bool success` or a small result object with `success/error/conflict_count`).
- Alternatively: re-raise after recording, and let UI show SyncFailed.
- Update `SchedulesWorkbench._run_sync` to post `SyncFailed` when sync fails (by exception or by returned status).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Test Results SummaryOverall Summary
Status: ✅ All tests passed! ✅ Unit Tests macos - latest - 3.11
✅ Unit Tests ubuntu - latest - 3.12
✅ Integration Tests 3.12
✅ Unit Tests macos - latest - 3.13
✅ Unit Tests macos - latest - 3.12
✅ Integration Tests 3.11
✅ Unit Tests windows - latest - 3.12
✅ Unit Tests ubuntu - latest - 3.13
✅ Unit Tests ubuntu - latest - 3.11
✅ Unit Tests windows - latest - 3.11
|
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.
Implements bidirectional reminder sync with tldw_server.
Summary
Verification
Closes TASK-299.2