Skip to content

fix(ui): show real last-active time for inactive sessions, not TUI load time#1560

Open
borng wants to merge 1 commit into
asheshgoplani:mainfrom
borng:fix/tui-last-activity-error-sessions
Open

fix(ui): show real last-active time for inactive sessions, not TUI load time#1560
borng wants to merge 1 commit into
asheshgoplani:mainfrom
borng:fix/tui-last-activity-error-sessions

Conversation

@borng

@borng borng commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The TUI session detail pane's "⏱ last active" line shows the most recent TUI load time for sessions that aren't actively running (error / idle / stopped), instead of when the session was actually last used. Reloading the TUI makes the timestamp jump to the new load time. The web UI is correct because it serves the persisted last_accessed.

Root cause

renderPreviewPane reads Instance.GetLastActivityTime(), which returns the tmux tracker's lastChangeTime (internal/tmux/tmux.go:3737). That field is seeded with time.Now() when the tracker is lazily created on TUI load (ensureStateTrackerLocked, internal/tmux/tmux.go:1255). Unlike its sibling LastObservedActivity(), GetLastActivityTime() does not check realActivityConfirmed. For a session that never confirms real activity (no spinner / busy indicator — i.e. a static error/idle/stopped pane), the value stays at that load-time seed, and the intended CreatedAt fallback in Instance.GetLastActivityTime() is dead code because the seed is never zero.

Fix

GetLastActivityTime() is left untouched because it also feeds the OpenCode session-rotation activity windows (internal/session/instance.go :1915, :4579) and its "recent-activity" semantics must not change.

Instead, add a display-only Instance.DisplayLastActivityTime():

  1. confirmed tmux activity (LastObservedActivity(), which correctly guards on realActivityConfirmed), else
  2. persisted LastAccessedAt (matches what the web serves), else
  3. CreatedAt (renders as "unknown" when also zero — an honest blank, never a fabricated time).

The detail pane now calls DisplayLastActivityTime(). Net: +82 / −2 across 3 files, no behavior change to any existing path.

Works retroactively

This is a pure display-path change reading already-persisted data — no migration/backfill. The buggy value was in-memory tracker state, never persisted, so every existing session picks up the corrected display immediately. Verified against a real DB of 195 error sessions: 174 have a real last_accessed (now match the web exactly); the rest fall back to created-date or "unknown".

Tests

  • TestDisplayLastActivityTime_NoConfirmedActivity_UsesLastAccessed — regression pin (no confirmed activity ⇒ LastAccessedAt, and asserts the result is not ~now).
  • TestDisplayLastActivityTime_NoAccess_FallsBackToCreated
  • TestDisplayLastActivityTime_AllZero_ReturnsZeroNotLoadTime

go build ./..., go vet ./internal/session/ ./internal/ui/, the session package suite, and the mandated TestPersistence_ gate all pass (the sole TestPersistence_TmuxDiesWithoutUserScope failure is pre-existing/environmental — it needs a real fake-login tmux server and fails identically on unmodified main).

Notes for reviewers

  • Design choice: DisplayLastActivityTime prefers confirmed activity over LastAccessedAt. For the reported case (error sessions) these are identical. They diverge only for a non-running session that showed a spinner earlier in the same TUI session — where "last active" is arguably more correct than "last accessed". Happy to drop step 1 and strictly mirror the web (LastAccessedAt → CreatedAt) if you prefer.
  • The session-list row badge (pickBadgeTime) uses a separate formula and already guards via LastObservedActivity, so it never leaked load-time and is unchanged; it may now differ slightly from the detail-pane value for some sessions.
  • Eval harness: the display derivation is structurally expressible in a pure Go unit test (added at the session layer), so no tests/eval/ case was added. Flagging in case you want one for the render path.

Summary by CodeRabbit

  • Bug Fixes

    • Improved the “last active” time shown in session views so it now prefers confirmed activity, then last accessed time, and finally the creation time.
    • Prevented recently loaded sessions from appearing more recently active than they actually are.
    • Aligned the preview pane’s activity timestamp with the web experience for sessions without confirmed activity.
  • Tests

    • Added coverage for activity-time fallback behavior across missing activity, missing access time, and all-timestamp-zero cases.

…ad time

The session detail pane's "last active" line read GetLastActivityTime(),
which returns the tmux tracker's lastChangeTime. That field is seeded with
time.Now() when the tracker is lazily created on TUI load, and — unlike its
guarded sibling LastObservedActivity() — GetLastActivityTime() ignores
realActivityConfirmed. For sessions that never confirm real activity
(error/idle/stopped panes), the value stays at the load-time seed, so the
detail pane showed the most recent TUI load instead of when the session was
actually last used. Reloading the TUI reseeded it, making the time jump.
The web UI was unaffected because it serves the persisted last_accessed.

GetLastActivityTime() also feeds OpenCode session-rotation windows, so its
semantics must not change. Add a display-only DisplayLastActivityTime() that
consults confirmed activity, then falls back to the persisted LastAccessedAt
(matching the web), then CreatedAt. Point the detail pane at it.

Tests:
- TestDisplayLastActivityTime_NoConfirmedActivity_UsesLastAccessed (regression)
- TestDisplayLastActivityTime_NoAccess_FallsBackToCreated
- TestDisplayLastActivityTime_AllZero_ReturnsZeroNotLoadTime

Created by gregb and his home-grown crew of builders 🦜 🤖
@borng borng requested a review from asheshgoplani as a code owner July 3, 2026 07:21
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bebd2a63-8579-403a-883d-fe7dedf271ab

📥 Commits

Reviewing files that changed from the base of the PR and between f70f19e and 88edc2d.

📒 Files selected for processing (3)
  • internal/session/instance.go
  • internal/session/instance_display_activity_test.go
  • internal/ui/home.go

📝 Walkthrough

Walkthrough

Adds a new Instance.DisplayLastActivityTime() method that returns confirmed activity time, falling back to LastAccessedAt then CreatedAt, with accompanying unit tests. The UI preview pane is updated to use this new method instead of GetLastActivityTime().

Changes

Display Last Activity Time

Layer / File(s) Summary
DisplayLastActivityTime helper and tests
internal/session/instance.go, internal/session/instance_display_activity_test.go
Adds Instance.DisplayLastActivityTime() returning confirmed activity via LastObservedActivity(), falling back to LastAccessedAt, then CreatedAt; adds tests for no-confirmed-activity, missing-access, and all-zero scenarios.
UI preview pane wiring
internal/ui/home.go
Preview pane's activity time now uses selected.DisplayLastActivityTime() instead of selected.GetLastActivityTime().

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • asheshgoplani/agent-deck#1273: Adds Instance.LastObservedActivity() gated by confirmed activity, which the new DisplayLastActivityTime() directly consumes.

Suggested reviewers: asheshgoplani

Poem

A rabbit checks the session's clock,
No load-time leak, no phantom tock.
Confirmed, then accessed, then born it seeks,
The preview pane now truly speaks.
Hop, hop, correct at last! 🐇🕐

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Remote_parity ⚠️ Warning UI preview now uses DisplayLastActivityTime only for local sessions; RemoteSession rows take a separate path and no remote-parity test/skip was added. Add a RemoteSession regression test or an explicit t.Skip documenting why the timestamp change is local-only and not applicable to remote rows.
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and accurately summarizes the UI time-display fix for inactive sessions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Test_coverage_per_surface ✅ Passed PASS: The only changed surface is TUI preview rendering; helper and render-path logic are unit-tested, and web/CLI/remote code paths were unchanged.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@borng

borng commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Let me know if you need any updates... my patch has been working great the past week

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.

1 participant