Treat an approval and a server request as one waiting state - #128
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughServer requests and approvals now share a “waiting on you” activity state. The frontend updates rendering, notifications, focus handling, resolution cleanup, sub-agent indicators, documentation, and regression tests. ChangesWaiting-on-user activity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant WebSocketClient
participant Notification
participant ThreadUI
Server->>WebSocketClient: send server_request_received
WebSocketClient->>ThreadUI: mark thread activity-waiting
WebSocketClient->>Notification: create waiting_request notification
Notification->>ThreadUI: focus waiting request on click
ThreadUI->>Server: send user answer
ThreadUI->>Notification: close waiting notification
ThreadUI->>ThreadUI: clear waiting activity
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/giskard-server/static/app.js (1)
2226-2231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrphaned comment above
releaseWaitingNotificationClaim.Lines 2226-2228 describe the connect-bootstrap replay handler (
handleThreadActivityBootstrap), not claim release. As it stands, two unrelated doc blocks stack on one function.♻️ Move or drop the stale block
-// Cross-thread activity the server replayed because we were not connected when it happened. Paints -// the badge exactly like a live event; notification is gated separately, because a reconnect is not -// news — see `maybeNotifyWaitingRequest`. // Undo a notification claim when the notification did not actually reach the user. Both records // must be released together: leaving the session-scoped one set would silence every later replay of // an approval the user was never shown. function releaseWaitingNotificationClaim(notificationKey) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/giskard-server/static/app.js` around lines 2226 - 2231, Remove or relocate the stale cross-thread activity comment so it documents handleThreadActivityBootstrap rather than releaseWaitingNotificationClaim. Keep the claim-release comment directly above releaseWaitingNotificationClaim and ensure the two unrelated documentation blocks no longer stack on that function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/giskard-server/static/app.js`:
- Around line 2325-2330: Update maybeNotifyWaitingRequest to derive the
notification headline from activity.kind, using input-specific wording for
requestUserInput and approval wording for approval requests while preserving the
existing sub-agent distinction. Update the corresponding assertion in
crates/giskard-server/tests/ui.rs to expect the new requestUserInput headline.
In `@tests/e2e/tests/approvals.spec.ts`:
- Line 58: Scope the activity-waiting assertion in the approval test to the
specific thread created and answered, using its retained identifier or row
locator rather than the page-wide .thread.activity-waiting locator. Assert that
only that thread’s row has no .activity-waiting element.
---
Nitpick comments:
In `@crates/giskard-server/static/app.js`:
- Around line 2226-2231: Remove or relocate the stale cross-thread activity
comment so it documents handleThreadActivityBootstrap rather than
releaseWaitingNotificationClaim. Keep the claim-release comment directly above
releaseWaitingNotificationClaim and ensure the two unrelated documentation
blocks no longer stack on that function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b310614e-52d3-4c79-904f-51f6e62d06cd
📒 Files selected for processing (10)
crates/giskard-server/src/registry.rscrates/giskard-server/src/routes.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/tests/approvals.spec.tstests/e2e/tests/server-requests.spec.tstests/e2e/tests/subagent-approvals.spec.ts
4efc06f to
d6efa37
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/giskard-server/static/app.js (2)
3060-3096: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
pending_server_requestsreplay never notifies, unlikepending_approval.On reconnect,
snap.pending_approvalis routed throughhandleIncomingApprovalRequest→notifyIncomingApproval→maybeNotifyWaitingRequest, so a still-pending approval re-arms the desktop notification. Thepending_server_requestsloop only callssetActiveThreadActivity(...)andrenderServerRequest(request)— it never callsmaybeNotifyWaitingRequest. A user who reconnects while a server request is outstanding gets the sidebar glyph but no notification, breaking the parity this PR is meant to establish between the two waiting kinds.🐛 Proposed fix
for (const request of (snap.pending_server_requests || [])) { - setActiveThreadActivity("server_request_received", true, "Waiting for your input", { - server_request_id:request && request.id ? String(request.id) : null + const serverRequestId = request && request.id ? String(request.id) : null; + setActiveThreadActivity("server_request_received", true, "Waiting for your input", { + server_request_id:serverRequestId }); renderServerRequest(request); + maybeNotifyWaitingRequest(String(snap.thread_id || state.threadId || ""), { + kind:"server_request_received", + active_turn:true, + approval_id:null, + server_request_id:serverRequestId, + summary:"Waiting for your input", + source:"live_turn_snapshot_pending_server_request", + unread:false + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/giskard-server/static/app.js` around lines 3060 - 3096, Update the pending_server_requests replay loop in renderLiveTurnSnapshot to invoke maybeNotifyWaitingRequest for each still-pending server request, matching the notification path used by pending_approval via handleIncomingApprovalRequest. Preserve the existing activity update and renderServerRequest behavior while ensuring reconnects re-arm the desktop notification.
284-293: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGeneralize the notification diagnostics bucket
maybeNotifyWaitingRequest()andhandleNotificationClick()still emit approval-prefixed diagnostics forserver_request_received, whilebrowserDiagnosticsSnapshot()groups them underlast_approval_decision/recent_approval_decisionsviaisApprovalNotificationDecision(). That keeps the two waiting modes indistinguishable in the diagnostics summary. If this snapshot is meant to cover both approval and server-request waits, the event naming or predicate needs to be widened.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/giskard-server/static/app.js` around lines 284 - 293, Generalize the notification diagnostics classification used by maybeNotifyWaitingRequest(), handleNotificationClick(), and browserDiagnosticsSnapshot() so server_request_received events are grouped alongside approval waits. Update isApprovalNotificationDecision() and the related last_approval_decision/recent_approval_decisions fields or event naming to use a shared notification/waiting-request category while preserving existing approval and server-request diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/giskard-server/static/app.js`:
- Around line 3060-3096: Update the pending_server_requests replay loop in
renderLiveTurnSnapshot to invoke maybeNotifyWaitingRequest for each
still-pending server request, matching the notification path used by
pending_approval via handleIncomingApprovalRequest. Preserve the existing
activity update and renderServerRequest behavior while ensuring reconnects
re-arm the desktop notification.
- Around line 284-293: Generalize the notification diagnostics classification
used by maybeNotifyWaitingRequest(), handleNotificationClick(), and
browserDiagnosticsSnapshot() so server_request_received events are grouped
alongside approval waits. Update isApprovalNotificationDecision() and the
related last_approval_decision/recent_approval_decisions fields or event naming
to use a shared notification/waiting-request category while preserving existing
approval and server-request diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 63d0f853-849e-432c-adee-6d27558e486e
📒 Files selected for processing (10)
crates/giskard-server/src/registry.rscrates/giskard-server/src/routes.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/tests/approvals.spec.tstests/e2e/tests/server-requests.spec.tstests/e2e/tests/subagent-approvals.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/e2e/tests/approvals.spec.ts
- crates/giskard-server/src/registry.rs
- crates/giskard-server/static/app.css
- tests/e2e/tests/server-requests.spec.ts
- docs/subagents.md
- crates/giskard-server/src/routes.rs
- specs/giskard-specification.md
- tests/e2e/tests/subagent-approvals.spec.ts
- crates/giskard-server/tests/ui.rs
d6efa37 to
3b24625
Compare
|
Addressed the review on Orphaned comment — valid, fixed. Inserting Diagnostics bucket — valid, and worse than reported. This wasn't only a naming inconsistency: the predicate matched
Fixed the predicate and renamed the bucket to match what it holds:
The premise is that a still-pending server request re-arms no notification on reconnect. It does, just not from the loop the comment points at.
Verified rather than reasoned. Reconnecting with an outstanding request and the tab unfocused (so the visible-and-focused suppression doesn't mask the result): Adding the suggested call would fire a second time for the same request and be swallowed by the dedup key — dead code that reads like it does something. Worth noting the symmetry, since it explains the shape rather than just excusing it: Happy to reconsider if there's a reconnect path that skips the accumulated replay — I didn't find one. Verification: 483 Rust tests with every target compiling, Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/giskard-server/static/app.js (2)
360-391: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiagnostics filter has gaps for some renamed reason strings.
isWaitingNotificationDiagnosticonly matcheswaiting_notify_received,waiting_notify_suppressed_*,waiting_notify_constructor_failed,waiting_notify_created, andbrowser_notification_*withkind:"waiting_request". Several reasons emitted elsewhere in this file (waiting_notification_clicked,waiting_notify_skipped_invalid_call,approval_notification_closed/approval_notification_close_failed) don't match any of these patterns, so they're silently excluded frombrowserDiagnosticsSnapshot()'slast_waiting_notification/recent_waiting_notificationsand thus fromrenderBrowserDiagnosticsPanel. This undermines the panel's usefulness for debugging why a notification did or didn't fire — see the consolidated comment for all affected sites and a combined fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/giskard-server/static/app.js` around lines 360 - 391, Update isWaitingNotificationDiagnostic so it also recognizes the emitted waiting-notification reasons waiting_notification_clicked, waiting_notify_skipped_invalid_call, approval_notification_closed, and approval_notification_close_failed, ensuring these entries appear in browserDiagnosticsSnapshot() and renderBrowserDiagnosticsPanel diagnostics.
2438-2468: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale
approval_notification_*diagnostic reason/key breaks the waiting-diagnostics filter and the id column.
closeWaitingNotificationstill recordsapproval_notification_closed/approval_notification_close_failedwith anapproval_idkey. This doesn't matchisWaitingNotificationDiagnostic'swaiting_notify_*patterns (excluded from the waiting panel), and sincerenderBrowserDiagnosticsPanel's generic event list readsdetail.request_id(Line 481), notapproval_id, these entries also lose their id inrecentBrowserEvents. Consolidated fix below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/giskard-server/static/app.js` around lines 2438 - 2468, The diagnostics emitted by closeWaitingNotification use obsolete approval_notification_* reasons and approval_id metadata. Update both success and failure recordNotificationDiagnostic calls in closeWaitingNotification to use the established waiting_notify_* reason/key conventions expected by isWaitingNotificationDiagnostic and renderBrowserDiagnosticsPanel, preserving the existing close behavior and failure error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/giskard-server/static/app.js`:
- Around line 284-293: Update the diagnostic classification used by
isWaitingNotificationDiagnostic so it recognizes the
waiting_notification_clicked reason emitted by handleNotificationClick, while
preserving recognition of the existing waiting_notify_* reasons and ensuring the
event appears in recent_waiting_notifications and last_waiting_notification.
- Around line 2245-2250: Rename the diagnostic reason emitted by
maybeNotifyWaitingRequest from waiting_notify_skipped_invalid_call to use the
established suppressed_ prefix, and update any corresponding diagnostics filter
references consistently.
---
Outside diff comments:
In `@crates/giskard-server/static/app.js`:
- Around line 360-391: Update isWaitingNotificationDiagnostic so it also
recognizes the emitted waiting-notification reasons
waiting_notification_clicked, waiting_notify_skipped_invalid_call,
approval_notification_closed, and approval_notification_close_failed, ensuring
these entries appear in browserDiagnosticsSnapshot() and
renderBrowserDiagnosticsPanel diagnostics.
- Around line 2438-2468: The diagnostics emitted by closeWaitingNotification use
obsolete approval_notification_* reasons and approval_id metadata. Update both
success and failure recordNotificationDiagnostic calls in
closeWaitingNotification to use the established waiting_notify_* reason/key
conventions expected by isWaitingNotificationDiagnostic and
renderBrowserDiagnosticsPanel, preserving the existing close behavior and
failure error details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b083afb4-01da-4752-a86a-fa074fd39475
📒 Files selected for processing (10)
crates/giskard-server/src/registry.rscrates/giskard-server/src/routes.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/tests/approvals.spec.tstests/e2e/tests/server-requests.spec.tstests/e2e/tests/subagent-approvals.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/tests/approvals.spec.ts
- crates/giskard-server/src/registry.rs
- crates/giskard-server/static/app.css
- tests/e2e/tests/server-requests.spec.ts
- docs/subagents.md
- tests/e2e/tests/subagent-approvals.spec.ts
- specs/giskard-specification.md
- crates/giskard-server/tests/ui.rs
0100360 to
077ffb0
Compare
|
Both findings valid — fixed in The consistency pass on Enumerating every emitted reason rather than fixing the two that were reported:
So four reasons were missing from the panel, not the two reported. The The filter is now one prefix test rather than a list: return reason.startsWith("waiting_notify_") ||
(reason.startsWith("browser_notification_") && detail.kind === "waiting_request");That's the actual fix. An enumeration silently omits whatever reason is added next, which is precisely how these fell out — adding four more entries to the list would have left the same trap in place. Added a Skipped: renaming Verification: 483 Rust tests with every target compiling, Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/giskard-server/static/app.js`:
- Around line 3005-3021: The server_request_received branch in handleEvent must
skip replayed or already-answered requests before setting activity or calling
maybeNotifyWaitingRequest. Mirror the approval path’s answered-request guard,
using the request state consumed by renderServerRequest/resolveServerRequest,
then only run setActiveThreadActivity, renderServerRequest, and
maybeNotifyWaitingRequest for unresolved requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 106d58ab-13d3-488d-91c6-b33e2ed49e73
📒 Files selected for processing (13)
crates/giskard-server/src/registry.rscrates/giskard-server/src/routes.rscrates/giskard-server/static/app.csscrates/giskard-server/static/app.jscrates/giskard-server/static/index.htmlcrates/giskard-server/static/sw.jscrates/giskard-server/tests/ui.rsdocs/subagents.mdspecs/giskard-specification.mdtests/e2e/tests/approvals.spec.tstests/e2e/tests/helpers.tstests/e2e/tests/server-requests.spec.tstests/e2e/tests/subagent-approvals.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/e2e/tests/approvals.spec.ts
- crates/giskard-server/static/sw.js
- crates/giskard-server/static/app.css
- tests/e2e/tests/helpers.ts
- crates/giskard-server/src/registry.rs
- crates/giskard-server/static/index.html
- docs/subagents.md
- crates/giskard-server/src/routes.rs
- tests/e2e/tests/server-requests.spec.ts
- specs/giskard-specification.md
- tests/e2e/tests/subagent-approvals.spec.ts
- crates/giskard-server/tests/ui.rs
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
077ffb0 to
2ead1fc
Compare
|
Valid, and a real bug I introduced in this PR. Fixed in Adding Reproduced before fixing — answered a request, reloaded with the tab unfocused so the visible-and-focused suppression couldn't mask it: A backgrounded tab was being pinged about work the user had already done. The branch now checks Covered by a new e2e test, and confirmed to fail without the guard: removing it, rebuilding, and re-running fails exactly that test while the other four still pass. One thing worth recording, since it bit me twice now: my first version of that test filtered notifications by request id alone and failed for the wrong reason. Earlier tests in the file leave outstanding requests behind, all reusing the same scripted id, and the connect bootstrap from #126 replays them — so the assertion was counting another test's leftovers. Now scoped by thread id as well. The shared, never-completing replay state makes any page-wide or id-only assertion quietly order-dependent. Verification: 483 Rust tests with every target compiling, Generated by Claude Code |
2 participants
Tier 1 of the approval/server-request unification. Presentation layer only — no protocol change.
The problem
An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking:
3active_turn→1!o— indistinguishable from merely busySo a sub-agent blocked on a question was exactly as invisible as the case fixed in #125 — on the path right next to it.
Codex itself already blurs the split: MCP tool approvals arrive as
requestUserInputserver requests and get promoted to approval cards by a marker (mapping.rs). The distinction is a protocol artifact, not something worth surfacing.The change
Activity ranking, the sidebar glyph, the sub-agent monitor and notifications key off "waits on the user" rather than "is an approval".
Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form. That part was never duplicated and shouldn't be forced together.
Names follow behaviour. State that now holds both kinds loses its "approval" prefix (
activity-waiting,waitingNotificationKey,notifiedRequests,waitingRequestRowById, …);pendingApprovalsandansweredApprovalskeep theirs, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. That accounts for most of the diff — it's a rename, and deliberately not left half-done, since names that lie about what they hold are how this divergence went unnoticed.A second gap, found by writing the test
The waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts
ApprovalResolved; a server request's resolved event comes from the harness on its own schedule and may never come — so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send.Also: the activity summary was the raw JSON-RPC method name, so notification bodies read
item/tool/requestUserInput request. Now "Waiting for your input".Verification
!withactivity-waitingand notactivity-running, then drops back to plain running once answered.fmt --checkandclippy --all-targets --all-features -D warningsclean.The
ui.rssource assertions earned their keep here: they failed nine times through the rename, each on something real, including a genuine bug where the diagnostic payload'sapproval_id→request_idrename left one reader still asking forapproval_id, which would have keyed the notification untrack onundefined.Known flake, not from this change
One of three full runs failed in
thread-selection.spec.ts. I have the mechanism now:The turn finishes before the browser subscribes, so the reply can only arrive via
history_page; the WebSocket then resets and the reconnect loop fails auth for ~7s, so the transcript never receives it and the 10s assertion times out. Same signature appeared during the #126 work, before server requests existed — it's a WS reconnect/auth issue in the replay setup, not this feature. Not fixed here; worth its own change.Follow-up
Tier 2 is the remaining half: the server-side plumbing and wire shape. Since #127 landed, both
pending_approvalandpending_server_requestsare verifiably redundant withaccumulated(I confirmed by emptying each and watching the reload tests still pass), andpending_approvalbeingOptionwas never a real cardinality limit — multiple simultaneous approvals work fine end to end. So that step is now mostly deletion plus merging the twoanswered_*lists, rather than reconciling two divergent shapes.🤖 Generated with Claude Code
https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY
Generated by Claude Code
Summary by CodeRabbit