Skip to content

loading animation fixes#423

Merged
sudo-tee merged 1 commit into
sudo-tee:mainfrom
phanen:fix/loading-animation-attach-busy-spinner
Jun 30, 2026
Merged

loading animation fixes#423
sudo-tee merged 1 commit into
sudo-tee:mainfrom
phanen:fix/loading-animation-attach-busy-spinner

Conversation

@phanen

@phanen phanen commented Jun 23, 2026

Copy link
Copy Markdown
Contributor
  • fix(animation): don't drive the spinner from local HTTP activity
  • fix(animation): hydrate spinner state via sync on (re)attach

@phanen phanen marked this pull request as draft June 23, 2026 16:31
@sudo-tee

Copy link
Copy Markdown
Owner

Thanks for the PR,

This looks like a nice improvement. I will do some testing throughout the day

@phanen

phanen commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

there‘s still edge case spinner won‘t disappear and cannot stably reproduce

@sudo-tee

Copy link
Copy Markdown
Owner

No stress, I tried it a little bit and for the most part it works pretty well. But I'll wait on merging it.

@phanen phanen force-pushed the fix/loading-animation-attach-busy-spinner branch 4 times, most recently from 4539d7f to 6d5a61a Compare June 30, 2026 02:12
Problem:

1. Spinner flashed on every UI open. The gate was jobs.is_running(),
   which every HTTP request bumps (including pure queries), so
   toggle on issued 3-4 queries and the spinner blinked.
2. Spinner never appeared after attach or toggle off-then-on. SSE
   cannot replay past events, and toggle off wiped the only local
   copy of status_data.
3. Spinner could stay on forever after the model finished:
   a. sync's GET response, stale by the time it landed, overwrote
      the cache with a busy snapshot and started the spinner
      against an already-idle server (which emits no follow-up).
   b. SSE idle before set_active was skipped (active_session
      guard), so replay read the stale busy.

Solution:
- Gate the spinner on a new `_should_animate()`: status_data is
  non-idle AND tagged with the active session's sessionID.
- Per-session `last_status_map` (always a table) holds the cache.
  SSE writes unconditionally; sync merges only missing entries
  (SSE wins).
- New `OpencodeApiClient:list_session_status` + `M.sync_from_server()`
  hydrate on setup and replay to status_data.
- `M.refresh()` is the single entry point that reconciles the
  running state with status_data and active_session.

Tests:
- `does not regress to busy when sync returns a stale snapshot
  after SSE idle` covers race 3a.
- `replays the active session from the cache (handles
  sync-before_set_active)` covers race 3b.
- `merges the response into the cache (only fills missing
  entries)` asserts SSE-preserved entries survive sync.
- `does not leave a stale spinner running when the model finishes
  during a hide` covers the toggle off / model finishes / toggle
  on scenario.
@phanen phanen force-pushed the fix/loading-animation-attach-busy-spinner branch from 6d5a61a to aadc790 Compare June 30, 2026 03:03
@phanen

phanen commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

there‘s still edge case spinner won‘t disappear and cannot stably reproduce

I can be reproduced when spinner should stop during opencode window is hidden, so previous commit forgot to clean up potential stale status in teardown()

M._animation.last_status_map = {}
M._animation.status_data = nil
M._animation.status_session_id = nil

@phanen phanen marked this pull request as ready for review June 30, 2026 03:25
@phanen

phanen commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Another case may happened when SSE connection is broken unexpectedly. In this case not only animation, the whole render will stop working, maybe we can retry SSE connect on failure and backoff... this may be done in another PR.

@sudo-tee

Copy link
Copy Markdown
Owner

You are right, we can address this in another PR.

I will do one last test today, and it should be good to go

@jensenojs

Copy link
Copy Markdown
Collaborator

@sudo-tee maybe test with this pr hh #437

@sudo-tee

Copy link
Copy Markdown
Owner

Did some tests and it seem pretty stable

Thanks for the PR, it's a great improvement

@sudo-tee sudo-tee merged commit d461f70 into sudo-tee:main Jun 30, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants