fix(webapp): show sessions with no live run as Idle instead of Active - #4570
fix(webapp): show sessions with no live run as Idle instead of Active#4570D-K-P wants to merge 4 commits into
Conversation
Session status was derived only from closedAt/expiresAt, so an open session whose run had already finished stayed Active forever and its duration ticked up from createdAt without end. Status is now derived from the current run's liveness: a session with no live run reads Idle, and its duration freezes at the run's completion instead of counting up. Active is reserved for sessions with a run actually executing. Applies to the sessions list and the session detail page.
The tag filter matches the session's own top-level tags, not triggerConfig.tags.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
📜 Recent review details⏰ Context from checks skipped due to timeout. (19)
🧰 Additional context used📓 Path-based instructions (6)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (13)📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-06-13T19:53:13.759ZApplied to files:
📚 Learning: 2026-06-17T17:13:49.929ZApplied to files:
📚 Learning: 2026-06-23T13:04:21.413ZApplied to files:
📚 Learning: 2026-05-01T15:45:08.099ZApplied to files:
📚 Learning: 2026-05-12T21:04:05.815ZApplied to files:
📚 Learning: 2026-06-25T18:21:51.905ZApplied to files:
📚 Learning: 2026-07-03T17:10:21.498ZApplied to files:
📚 Learning: 2026-06-04T18:16:35.386ZApplied to files:
📚 Learning: 2026-06-09T17:58:04.699ZApplied to files:
🔇 Additional comments (1)
WalkthroughSession status derivation now distinguishes closed, expired, active, and idle sessions using session lifecycle fields and current-run state. Session list results include current run completion timestamps. The route and session components support the 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| vi.mock("~/db.server", async () => { | ||
| const { Prisma } = await import("@trigger.dev/database"); | ||
| const lazyProxy = (holder: { client: any }, label: string) => | ||
| new Proxy( | ||
| {}, | ||
| { | ||
| get(_t, prop) { | ||
| if (!holder.client) throw new Error(`${label} not set for this test`); | ||
| const value = holder.client[prop]; | ||
| if (value !== null && typeof value === "object") { | ||
| return new Proxy(value, { get: (_d, method) => holder.client[prop][method] }); | ||
| } | ||
| return value; | ||
| }, | ||
| } | ||
| ); | ||
| return { | ||
| prisma: lazyProxy(primaryHolder, "primaryHolder.client"), | ||
| $replica: lazyProxy(replicaHolder, "replicaHolder.client"), | ||
| runOpsNewPrismaClient: undefined, | ||
| runOpsNewReplicaClient: undefined, | ||
| runOpsLegacyPrisma: undefined, | ||
| runOpsLegacyReplica: undefined, | ||
| sqlDatabaseSchema: Prisma.sql([`public`]), | ||
| }; | ||
| }); | ||
|
|
||
| // Orthogonal peripherals. | ||
| const STUB_ENV = { | ||
| id: "env_stub", | ||
| type: "DEVELOPMENT" as const, | ||
| slug: "dev", | ||
| organizationId: "org_stub", | ||
| projectId: "proj_stub", | ||
| userId: undefined, | ||
| branchName: null, | ||
| git: null, | ||
| }; | ||
|
|
||
| vi.mock("~/models/runtimeEnvironment.server", () => ({ | ||
| findDisplayableEnvironment: async () => STUB_ENV, | ||
| })); | ||
|
|
||
| vi.mock("~/v3/models/workerDeployment.server", () => ({ | ||
| findCurrentWorkerFromEnvironment: async () => null, | ||
| })); | ||
|
|
||
| // The session list comes from ClickHouse via SessionsRepository — orthogonal to the run read. | ||
| // The stub returns controlled session rows whose currentRunId points at runs we seed for real. | ||
| const sessionListHolder = vi.hoisted(() => ({ sessions: [] as any[] })); | ||
| vi.mock("~/services/sessionsRepository/sessionsRepository.server", () => ({ | ||
| LEGACY_PLAYGROUND_TAG: "__playground__", | ||
| SessionsRepository: class { | ||
| constructor(_deps: any) {} | ||
| async listSessions() { | ||
| return { | ||
| sessions: sessionListHolder.sessions, | ||
| pagination: { nextCursor: null, previousCursor: null }, | ||
| }; | ||
| } | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🟡 New test relies on module mocking, which the repository forbids
The added integration test stubs out the database module, the environment lookup, the worker lookup and the sessions repository with vi.mock (apps/webapp/test/sessionListPresenterStatus.test.ts:22-83), which contradicts the repository's testing rule to never mock anything and use testcontainers instead.
Impact: The test violates the project's mandatory testing convention.
Rule reference
AGENTS.md, "Testing": "We use vitest exclusively. Never mock anything - use testcontainers instead." The test does use a real Postgres container for runs, but replaces ~/db.server, ~/models/runtimeEnvironment.server, ~/v3/models/workerDeployment.server and ~/services/sessionsRepository/sessionsRepository.server with mocks.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This follows an existing merged pattern: apps/webapp/test/presentersSessionBatchReplicaLag.guard.test.ts drives the same presenter against a real Postgres (testcontainer) while stubbing only orthogonal peripherals (the ~/db.server proxies, displayable-env, worker probe, and the ClickHouse-backed SessionsRepository, which only supplies ordered ids). The system under test (the presenter's run read + status derivation) and its Postgres data are real; only the ClickHouse index and control-plane lookups are stubbed to keep this to one container. Leaving open for a maintainer's call on the pattern.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address review on the sessions status change: - Keep the "Close session" action on the detail page for Idle sessions; they are open, only Closed and Expired are terminal. - Rename the status helper input from currentRunId to hasCurrentRun, since the detail page passes a run friendlyId, not the session's currentRunId. - Restore the Active tooltip copy so it stays accurate now that the Active filter also returns open, idle sessions. - Align the sessions docs example so the listed tag matches a top-level tag set at start time.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ing-duration # Conflicts: # apps/webapp/vitest.config.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
| export function deriveSessionStatus(input: DeriveSessionStatusInput): SessionDisplayStatus { | ||
| if (input.closedAt != null) { | ||
| return "CLOSED"; | ||
| } | ||
|
|
||
| if (input.expiresAt != null && input.expiresAt.getTime() < input.now) { | ||
| return "EXPIRED"; | ||
| } | ||
|
|
||
| const hasLiveRun = | ||
| input.hasCurrentRun && | ||
| input.currentRunStatus !== undefined && | ||
| !isFinalRunStatus(input.currentRunStatus); | ||
|
|
||
| return hasLiveRun ? "ACTIVE" : "IDLE"; | ||
| } |
There was a problem hiding this comment.
🔍 Run detail page still derives session status the old way, so it can disagree with the new Idle label
The run/span page renders a session badge from run.session.status, which apps/webapp/app/presenters/v3/SpanPresenter.server.ts:373-379 still derives from closedAt/expiresAt only. After this PR, an open session whose current run has terminated reads IDLE on the sessions list and the session detail page, but still reads ACTIVE on the run's span panel. Same for the agent activity chart legend (apps/webapp/app/presenters/v3/AgentDetailPresenter.server.ts:34). Not strictly wrong (those surfaces are also fed by the three-value model), but the PR's stated goal of list/detail agreement leaves this third surface behind.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good flag, splitting it in two:
- Agent activity chart (AgentDetailPresenter): this is a ClickHouse aggregation over closedAt/expiresAt, i.e. the same aggregation class as the status filter. Idle is display-only and intentionally not represented in aggregation/filter surfaces, so this one deliberately stays ACTIVE/CLOSED/EXPIRED.
- Run/span panel session badge (SpanPresenter:373): agreed this is a display badge like the list and detail page, so for full consistency it should reuse deriveSessionStatus. It needs the session's current-run status available there (an extra field to load). That is beyond this ticket's list+detail scope, so leaving this open for the maintainer to decide whether to fold in here or track as a follow-up.
1 participant
Summary
An abandoned chat session (never closed, its run long since finished) showed as Active on the Sessions list with a duration that counted up from creation forever. Session status now reflects whether a run is actually live: a session with no running run reads Idle, and its duration freezes at when the run finished. Active is kept for sessions with a run genuinely executing. Closed and Expired are unchanged. The same derivation now backs the session detail page, so the list and detail no longer disagree.
Fix
Status was derived only from closedAt/expiresAt and never looked at the current run. A small shared helper now folds in the current run's status (via the run pointer the list already loads), so it can tell open-but-idle from live. The status filter is untouched: Idle is display only, so there is no ClickHouse or migration change, and filtering by Active still returns open sessions.
Also corrects the sessions.list docs, where the tag filter was described as matching triggerConfig.tags. It actually matches the session's own top-level tags.