Drop pending_server_requests from the reconnect snapshot - #131
Conversation
📝 WalkthroughWalkthroughChangesThe reconnect snapshot no longer carries a separate Server-request replay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReplayHarness
participant LiveBufferStore
participant LiveTurnSnapshot
participant Browser
ReplayHarness->>LiveBufferStore: emit ServerRequestReceived
ReplayHarness->>LiveBufferStore: emit Error
LiveBufferStore->>LiveTurnSnapshot: accumulate lifecycle events
LiveTurnSnapshot->>Browser: replay snapshot
Browser->>Browser: derive outstandingServerRequests
Browser->>Browser: show waiting state and Continue action
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 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/src/bin/giskard-server-replay.rs`:
- Around line 488-497: Update the AgentEvent::Error send path guarded by
raise_server_request_then_error to capture the broadcast send failure instead of
discarding it. When sending fails, emit a warn! containing thread_id, turn_id,
action, and the send error, while preserving the existing successful send
behavior.
In `@crates/giskard-server/static/app.js`:
- Around line 3189-3198: Update received-event handling to remove the request ID
from closed state before storing the latest payload, so a later
server_request_received reopens it. Apply the corresponding closed-marker reset
in the live-buffer test helper, and update the server-request tests to replay
events sequentially, clear closure state on later receives, and add a received →
resolved → received regression case in
crates/giskard-server/tests/server_requests.rs:645-657; also make the matching
changes in crates/giskard-server/static/app.js:3189-3198 and
crates/giskard-server/src/live_buffer.rs:480-491.
🪄 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: 383a9dba-4a34-4995-9451-51a4d3a8a3d6
📒 Files selected for processing (9)
crates/giskard-proto/src/lib.rscrates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/src/live_buffer.rscrates/giskard-server/static/app.jscrates/giskard-server/tests/server_requests.rscrates/giskard-server/tests/ui.rsspecs/giskard-specification.mdtests/e2e/tests/helpers.tstests/e2e/tests/server-requests.spec.ts
6c6c7de to
264bb88
Compare
|
Addressed the two CodeRabbit findings in 🟠 Finding 2 (reopen after resolution) — fixedThis was a real regression I introduced. The server's Fixed on both ends:
Pinned by a new regression test 🟠 Finding 1 (log the send failure) — skipped, with reasonThe replay binary uses Verification
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 3193-3201: Update the request aggregation logic around the
received/resolved handling to track first-seen IDs independently of the `latest`
map, so reopening a resolved request updates its payload without appending its
ID to `order` again. Preserve the original first-seen ordering and ensure the
final map renders one card using the newest payload. Add a focused regression
test covering received → resolved → received and asserting a single card with
the latest request data.
In `@crates/giskard-server/tests/ui.rs`:
- Around line 290-293: Strengthen the UI regression coverage around
outstandingServerRequests so it executes the browser behavior rather than only
checking JavaScript source text. Verify that replayed answered server requests
render as resolved and non-actionable, while unanswered requests remain
actionable, and retain the source assertion only if still useful.
🪄 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: cdde4bc8-4608-489b-b095-8c713326ccae
📒 Files selected for processing (9)
crates/giskard-proto/src/lib.rscrates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/src/live_buffer.rscrates/giskard-server/static/app.jscrates/giskard-server/tests/server_requests.rscrates/giskard-server/tests/ui.rsspecs/giskard-specification.mdtests/e2e/tests/helpers.tstests/e2e/tests/server-requests.spec.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/e2e/tests/server-requests.spec.ts
- tests/e2e/tests/helpers.ts
- crates/giskard-server/src/bin/giskard-server-replay.rs
- crates/giskard-server/tests/server_requests.rs
- specs/giskard-specification.md
- crates/giskard-server/src/live_buffer.rs
- crates/giskard-proto/src/lib.rs
264bb88 to
88831ae
Compare
|
Addressed the two new CodeRabbit findings on the reopen fix in 🟠 Finding 3 (duplicate card on reopen) — fixedMy reopen fix had a follow-on bug CodeRabbit caught: Fixed by splitting first-seen tracking from the latest-payload map, as suggested: a The existing 🟡 Finding 4 (assert replay behavior, not source text) — already covered, skippedThe
These execute the real browser behavior, so the invariant is covered. Kept the Verification
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/giskard-server/src/live_buffer.rs (1)
692-693: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse ULID request IDs in the new fixtures.
pending,resolved, andsrvare newly introduced identifiers but are not ULIDs. Use fixed valid ULID literals or a fixture generator.Also applies to: 745-745
🤖 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/src/live_buffer.rs` around lines 692 - 693, Update the new request-ID fixtures near the pending/resolved definitions and the srv fixture to use fixed valid ULID literals or the existing fixture generator, replacing the non-ULID values while preserving each fixture’s distinct identity.Source: Coding guidelines
🤖 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/src/live_buffer.rs`:
- Around line 488-492: Update the WireAgentEvent::ServerRequestResolved branch
to mark the request closed without removing its ID from seen or order,
preserving first-seen ordering when it reopens. Extend the relevant live-buffer
test to cover received(A), received(B), resolved(A), received(A) and verify the
order remains [A, B] while closed state toggles.
---
Outside diff comments:
In `@crates/giskard-server/src/live_buffer.rs`:
- Around line 692-693: Update the new request-ID fixtures near the
pending/resolved definitions and the srv fixture to use fixed valid ULID
literals or the existing fixture generator, replacing the non-ULID values while
preserving each fixture’s distinct identity.
🪄 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: d966de3e-1639-4daf-99e9-3d45da51297b
📒 Files selected for processing (9)
crates/giskard-proto/src/lib.rscrates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/src/live_buffer.rscrates/giskard-server/static/app.jscrates/giskard-server/tests/server_requests.rscrates/giskard-server/tests/ui.rsspecs/giskard-specification.mdtests/e2e/tests/helpers.tstests/e2e/tests/server-requests.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/tests/helpers.ts
- crates/giskard-server/tests/server_requests.rs
- crates/giskard-server/src/bin/giskard-server-replay.rs
- crates/giskard-proto/src/lib.rs
- tests/e2e/tests/server-requests.spec.ts
- specs/giskard-specification.md
88831ae to
3c3c7c5
Compare
|
Addressed the new CodeRabbit finding (#5, 🟡 Finding 5 (preserve first-seen order on reopen) — fixedValid catch — this was a real client/server-helper divergence. My server-side test helper Fixed by making the server helper match the client: Pinned by a new test Verification
|
`LiveTurnSnapshot::pending_server_requests: Vec<ServerRequest>` was a precomputed outstanding set that duplicated a derivation already available on both ends: the outstanding server requests are exactly the rows in `accumulated` whose latest event is a `server_request_received` and which are neither in `answered_server_requests` (the user answered) nor closed by a `server_request_resolved` later in `accumulated` (the harness closed it). This is the symmetric slice to dropping `pending_approval`: the snapshot now carries no "pending" list for either kind, and the client derives both from `accumulated` plus the answered set while replaying. - `LiveTurnSnapshot` loses `pending_server_requests`. The client renders answered requests resolved from `answered_server_requests` and treats the rest as actionable, all from `accumulated`. - The server's `pending_attention` (the SB5 connect bootstrap) derives the outstanding server requests the same way, via a `pending_server_requests` helper that now takes the resolved set, mirroring `pending_approvals`. - The client's `renderLiveTurnSnapshot` re-asserts the outstanding server requests *after* the `accumulated` replay, mirroring the approval path (SR10b): a later `error` overwrites the thread's activity and clears the active turn, so re-asserting last gives the waiting state the last word. A turn blocked on a server request that then errored still reads as waiting on the user rather than "errored, idle". `answered_server_requests` is kept: it is the only record of a user's answer before/without the harness's own resolved event, so dropping it would replay an answered request as actionable and re-answering routes a stale id to the harness (SR6). The larger refactor's SR11 relocates that onto the row; this slice does not. The SR11b ordering (re-assert after replay, for server requests) is pinned by a new Playwright test that reloads a turn blocked on a server request that then errored, asserting the sidebar still reads `activity-waiting` with a `!` glyph and the request stays answerable. Confirmed load-bearing by moving the re-assert loop before the replay: it fails exactly as described, passes in order.
3c3c7c5 to
073ff28
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)
3142-3147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve valid falsy request IDs.
Line 3144 treats
request.id === 0as missing, whileoutstandingServerRequestsexplicitly accepts zero-valued IDs. This leaves the request rendered withserver_request_id: null.Suggested fix
- server_request_id:request && request.id ? String(request.id) : null + server_request_id: + request && request.id !== undefined && request.id !== null + ? String(request.id) + : null🤖 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 3142 - 3147, Update the server_request_id construction in the outstandingServerRequests loop to preserve valid falsy IDs such as 0; only use null when request or request.id is actually absent, while keeping existing string conversion for present IDs.
3142-3147: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRegenerate screenshots for this visible replay change.
This changes which server-request cards and waiting states appear during replay. Run
tests/e2e/screenshots.shand commit updateddocs/screenshots/ide-desktop.pnganddocs/screenshots/ide-mobile.png.As per coding guidelines, frontend changes under
crates/giskard-server/static/**that affect visible UI must regenerate the documentation screenshots in the same change.🤖 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 3142 - 3147, Regenerate the documentation screenshots after the visible replay change in the outstandingServerRequests replay flow by running tests/e2e/screenshots.sh, then commit the updated ide-desktop.png and ide-mobile.png artifacts.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (1)
crates/giskard-server/static/app.js (1)
3174-3200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve first-seen order when reopening requests.
pending.delete(id)followed by a laterpending.set(id, request)moves a reopened request to the end. ForA → B → resolve(A) → receive(A), the result becomesB, Ainstead of the originalA, B, breaking the client/server ordering contract.Track first-seen IDs separately from the latest payload map, and use a closed set or ordered ID list so resolution hides a request without forgetting its original position.
Suggested fix
const pending = new Map(); + const seen = new Set(); + const order = []; for (const ev of (snap.accumulated || [])) { ... if (!id) continue; + if (!seen.has(id)) { + seen.add(id); + order.push(id); + } pending.set(id, request); ... - return Array.from(pending.values()) - .filter(request => request && !answered.has(String(request.id))); + return order + .map(id => pending.get(id)) + .filter(request => request && !answered.has(String(request.id)));🤖 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 3174 - 3200, Update outstandingServerRequests so resolving a request hides it without discarding its original order: keep the latest request payload separately from a first-seen ordered ID collection, and track resolved IDs or active state rather than deleting the ID’s ordering record. On later server_request_received events, refresh the payload while preserving the original position, so reopening A after B still yields A, B.
🤖 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 3142-3147: Update the server_request_id construction in the
outstandingServerRequests loop to preserve valid falsy IDs such as 0; only use
null when request or request.id is actually absent, while keeping existing
string conversion for present IDs.
- Around line 3142-3147: Regenerate the documentation screenshots after the
visible replay change in the outstandingServerRequests replay flow by running
tests/e2e/screenshots.sh, then commit the updated ide-desktop.png and
ide-mobile.png artifacts.
---
Duplicate comments:
In `@crates/giskard-server/static/app.js`:
- Around line 3174-3200: Update outstandingServerRequests so resolving a request
hides it without discarding its original order: keep the latest request payload
separately from a first-seen ordered ID collection, and track resolved IDs or
active state rather than deleting the ID’s ordering record. On later
server_request_received events, refresh the payload while preserving the
original position, so reopening A after B still yields A, B.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c29dc5b-335b-42a0-bfbd-39b2b8d6f706
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/giskard-proto/src/lib.rscrates/giskard-server/Cargo.tomlcrates/giskard-server/src/bin/giskard-server-replay.rscrates/giskard-server/src/live_buffer.rscrates/giskard-server/static/app.jscrates/giskard-server/tests/server_requests.rscrates/giskard-server/tests/ui.rsspecs/giskard-specification.mdtests/e2e/tests/helpers.tstests/e2e/tests/server-requests.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/giskard-server/tests/ui.rs
- crates/giskard-server/tests/server_requests.rs
- crates/giskard-server/src/bin/giskard-server-replay.rs
- crates/giskard-proto/src/lib.rs
- specs/giskard-specification.md
- crates/giskard-server/src/live_buffer.rs
|
Addressed the three new CodeRabbit findings on 🟡 #1 (outside diff,
|
| Check | Result |
|---|---|
cargo fmt / cargo clippy -D warnings |
clean |
cargo test --workspace |
all green |
tests/e2e/run.sh tests/server-requests.spec.ts |
6/6 |
| CI (rustfmt / clippy / test / playwright / cargo-deny / CodeRabbit) | all pass |
1 participant
The symmetric slice to #130 (which dropped
pending_approval). After both,LiveTurnSnapshotcarries no precomputed "pending" list for either kind; the browser derives what's still outstanding fromaccumulatedplus the two answered sets.What
LiveTurnSnapshot::pending_server_requests: Vec<ServerRequest>was a precomputed outstanding set that duplicated a derivation already available on both ends: the outstanding server requests are exactly the rows inaccumulatedwhose latest event is aserver_request_receivedand which are neither inanswered_server_requests(the user answered) nor closed by aserver_request_resolvedlater inaccumulated(the harness closed it).LiveTurnSnapshotlosespending_server_requests. The client renders answered requests resolved fromanswered_server_requestsand treats the rest as actionable, all fromaccumulated.pending_attention(the SB5 connect bootstrap) derives the outstanding server requests the same way, via apending_server_requests(events, resolved)helper that now takes the resolved set, mirroringpending_approvals.renderLiveTurnSnapshotre-asserts the outstanding server requests after theaccumulatedreplay, mirroring the approval path (SR10b): a latererroroverwrites the thread's activity and clears the active turn, so re-asserting last gives the waiting state the last word. A turn blocked on a server request that then errored still reads as waiting on the user rather than "errored, idle".answered_server_requestsis kept: it is the only record of a user's answer before/without the harness's own resolved event, so dropping it would replay an answered request as actionable and re-answering routes a stale id to the harness (SR6). The larger refactor's SR11 relocates that onto the row; this slice does not.Test
The SR11b ordering (re-assert after replay, for server requests) is pinned by a new Playwright test that reloads a turn blocked on a server request that then errored, asserting the sidebar still reads
activity-waitingwith a!glyph and the request stays answerable. Confirmed load-bearing by moving the re-assert loop before the replay: it fails exactly as the approvals-side test did, passes in order.Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacetests/e2e/run.sh(full Playwright suite)The spec (
specs/giskard-specification.md) is updated in the same change: bumped to 1.62 with aChangelog (1.61 → 1.62), drop pending_server_requestsentry (SR11a/SR11b), and theLiveTurnSnapshotshape line rewritten to describe the unified derivation for both kinds.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation