Christophe Vila
a828055c30
Discovered while verifying the sync design with the user: these two
exported methods are dead in production (only FullSync's *Core calls
are actually used), kept alive only by tests, and their doc comments
still reference the background sync loop removed in 4d2cbe4.
150 lines
8.0 KiB
Markdown
150 lines
8.0 KiB
Markdown
# Improve Synchronization UX — Design
|
|
|
|
**Source idea:** `docs/IDEAS.md` — "improve synchronization: make it modal, split downloads of
|
|
activities and workouts (as workouts are currently downloaded from their ID in related
|
|
activities), add progress bar (which requires to know in advance how many activities or
|
|
workouts will have to be downloaded)."
|
|
|
|
## Goal
|
|
|
|
Replace the Profile page's single inline "syncing: N/M activities" text line with a blocking
|
|
modal that shows real progress through sync, broken into named phases the user can actually
|
|
follow: discovering new activities, fetching activity details, then fetching workouts.
|
|
|
|
## Cleanup folded into this work: dead `Backfill`/`IncrementalSync` wrappers
|
|
|
|
`Service.Backfill(ctx)` and `Service.IncrementalSync(ctx)` are exported methods that each record
|
|
their own `SyncRun`, wrapping `backfillCore`/`incrementalSyncCore`. Their doc comments say
|
|
they're used "by the periodic background loop" -- but that loop was removed in an earlier
|
|
session (`4d2cbe4 refactor: remove automatic background incremental sync`), and grepping
|
|
`internal/api/` and `cmd/` confirms nothing in production calls them anymore. `FullSync` (the
|
|
only production caller of the core logic) calls `backfillCore`/`incrementalSyncCore` directly.
|
|
Only `internal/sync/service_test.go` still calls `Backfill`/`IncrementalSync`, which is the only
|
|
reason they're not already flagged as unused by the compiler.
|
|
|
|
Since this plan already restructures `service.go`'s progress model, remove these two dead
|
|
exported methods (and their now-inaccurate doc comments) as an early task, rewriting the tests
|
|
that called them to exercise the same behavior through `FullSync` or the `*Core` functions
|
|
directly (same package, so unexported functions are still directly testable) -- before any of
|
|
the progress-model changes below, so later tasks aren't touching code that's about to be
|
|
deleted.
|
|
|
|
## Current state (for reference)
|
|
|
|
- `sync.Service.Progress()` returns a flat `{Done, Total}`, written only by
|
|
`FillPendingDetails` — `Backfill`/`IncrementalSync` (the "discover which activities exist"
|
|
phase) report no progress at all today.
|
|
- `FillPendingDetails` fetches, per activity, in one pass: `GetActivitySplits`,
|
|
`GetActivityDetails`, and (if the activity has a `workout_id`) `GetWorkoutByID` — then writes
|
|
samples, laps (with workout-derived target pace/HR bands baked in), activity details, and
|
|
marks splits fetched.
|
|
- `GET /api/sync/status` returns `{in_progress, detail_fill_progress: {Done, Total},
|
|
activities_pending_details, last_run?}`. The frontend (`GarminConnection.tsx`) polls this and
|
|
renders an inline `<p>` line, both while syncing and once idle (a static "last sync: ..."
|
|
summary).
|
|
- `sync_runs` stores one row per `Backfill`/`IncrementalSync`/`FullSync` call
|
|
(`kind`/`started_at`/`finished_at`/`activities_fetched`/`status`/`error_message`).
|
|
`GET /api/sync/runs` exists but nothing in the frontend calls it.
|
|
|
|
## Scope
|
|
|
|
**In scope:** "Sync now" (`FullSync`) gets the new modal and phase-aware progress. The static
|
|
idle "last sync" summary on the Profile page is removed — the modal becomes the only place
|
|
sync status/results are shown; closing it means nothing is shown again until the next sync.
|
|
|
|
**Out of scope:** "Reset all" keeps its current behavior unchanged (confirm dialog, fire, poll
|
|
via a blocking `while` loop, reload the page) — no modal, no progress UI. The "modern error
|
|
displays" idea (ephemeral banners, backend-down handling) from the same backlog section is a
|
|
separate, later feature — this one only makes sync's own errors visible via the modal's final
|
|
state, reusing the `sync_runs.error_message` field that's already fetched but never rendered
|
|
today.
|
|
|
|
## Backend design
|
|
|
|
### Phase-aware progress
|
|
|
|
```go
|
|
type Progress struct {
|
|
Phase string // "idle" | "discovering" | "activities" | "workouts"
|
|
Done int
|
|
Total int
|
|
}
|
|
```
|
|
|
|
`FullSync` drives the phase transitions:
|
|
|
|
1. **`discovering`** — set (Done=0, Total=0, indeterminate) while `Backfill`/`IncrementalSync`
|
|
run. These still report no count; the modal shows a spinner, not a bar, during this phase.
|
|
2. **`activities`** — `FillPendingDetails` is split into two sequential passes. The first pass
|
|
fetches `GetActivitySplits`+`GetActivityDetails`+samples for every activity missing details
|
|
(`ActivitiesMissingDetails`/`CountActivitiesMissingDetails`, unchanged queries), writing laps
|
|
*without* workout-target alignment yet. `Total` is the pending-details count taken once at
|
|
the start of this pass; `Done` advances per activity.
|
|
3. **`workouts`** — a new second pass. `workout_id` is decoded from Garmin's activity summary
|
|
and stored on the `activities` row at upsert time, well before any detail fetch — so
|
|
"activities needing a workout fetched" is an independently queryable condition
|
|
(`workout_id IS NOT NULL AND workout_raw_json IS NULL`), needing no live Garmin call to
|
|
count. Two new store methods, `ActivitiesMissingWorkout`/`CountActivitiesMissingWorkout`
|
|
(mirroring the naming of the existing `ActivitiesMissingDetails`/
|
|
`CountActivitiesMissingDetails`), list/count these. For each: fetch `GetWorkoutByID`,
|
|
re-derive lap target bands from the just-stored lap data (`alignWorkoutTargets`), update
|
|
the laps and set `workout_raw_json`. This condition also picks up any older activity that
|
|
has a `workout_id` but never got its workout aligned in some prior run (e.g. a past
|
|
transient failure) — not just ones touched in this run's `activities` pass — a small
|
|
latent-bug fix as a side effect.
|
|
4. **`idle`** — reset to `{Phase: "idle", Done: 0, Total: 0}` once `FullSync` returns, same
|
|
`defer`-based reset as today.
|
|
|
|
Restructuring `fillActivityDetails` to decouple lap-writing from workout-target alignment
|
|
means an activity with a workout gets its laps written twice (once plain in the `activities`
|
|
pass, once updated with target bands in the `workouts` pass) — this is a second local SQL
|
|
delete+insert, not a second Garmin API call, so it costs nothing against rate limits.
|
|
|
|
### API
|
|
|
|
`GET /api/sync/status` changes shape:
|
|
|
|
```json
|
|
{
|
|
"in_progress": true,
|
|
"progress": { "phase": "workouts", "done": 3, "total": 8 },
|
|
"activities_pending_details": 0,
|
|
"last_run": { "...": "SyncRun, unchanged fields" }
|
|
}
|
|
```
|
|
|
|
This replaces `detail_fill_progress`/`DetailFillProgress` outright (no other consumer exists).
|
|
`SyncRun.Kind`'s frontend type also gains its missing `"full"` value (backend has always been
|
|
able to report it; the frontend type was just never updated to match).
|
|
|
|
## Frontend design
|
|
|
|
`GarminConnection.tsx` keeps its connect/MFA/Reset-all logic and buttons untouched, but loses
|
|
its `syncStatus` polling, `syncProgressLabel` helper, and all inline sync-progress/last-sync
|
|
JSX.
|
|
|
|
A new `SyncModal.tsx`:
|
|
|
|
- Renders as a blocking overlay (page behind it non-interactive) the moment `api.syncRun()`
|
|
resolves successfully.
|
|
- Polls `api.syncStatus()` every 1500ms while open (same cadence as today's polling).
|
|
- Renders by `progress.phase`:
|
|
- `discovering` → spinner + "Discovering activities…"
|
|
- `activities` → progress bar + "Activities: {done}/{total}"
|
|
- `workouts` → progress bar + "Workouts: {done}/{total}"
|
|
- Once `in_progress` becomes `false`: shows `last_run.Status`, `ActivitiesFetched`, and
|
|
`ErrorMessage` (if the run errored) plus the existing "N more pending — sync again" nudge if
|
|
`activities_pending_details > 0`, and a **Close** button. No auto-close, no timeout — the
|
|
user decides when to dismiss it (this is the one place sync errors are visible, so nothing
|
|
should hide it automatically).
|
|
|
|
## Testing
|
|
|
|
- `internal/sync`: new/updated tests for the two-phase `FillPendingDetails` split (an activity
|
|
with a workout gets its laps written in both passes, target bands only present after the
|
|
`workouts` pass; an activity without a workout is untouched by the `workouts` pass), and for
|
|
phase-aware `Progress()` transitions.
|
|
- `internal/api`: `sync_test.go` updated for the new `/api/sync/status` JSON shape.
|
|
- Frontend: no test suite exists (per CLAUDE.md) — manual smoke test against `seedsample` data,
|
|
watching the modal move through all four phases.
|