Redesign stacks as faithful per-turn request/response records - #578
Merged
Conversation
The old stacks view was a 12-layer derived summary built on the Electron
side. It counted messages, reconstructed streamed responses, and — worst of
all — dropped the provider error body on a non-2xx, so a failed call showed
only "http status: 400" with no explanation. That made it useless for the one
thing stacks are for: seeing exactly what was sent to and returned by the
model so you can reason about context management.
Replace it with one faithful record per model call, captured on the backend:
request: { api, url, model, body } // verbatim wire payload
response: { statusCode, body, error, tokenUsage } // assembled response or
// captured error body
- Capture error bodies: both OpenAI adapters now build the request with
http_status_as_error(false) and branch on is_success(), reading and storing
the provider's error body on a non-2xx instead of collapsing it into a bare
status. Shared via capture_status_or_error so every adapter behaves the same.
- MockModel gains respond_with_trace so the offline/test path exercises capture.
- Bump the stack record schema to v2; v1 records no longer parse and are
skipped on both read and compaction (clean cutover, no migration).
- Rewrite the Electron view: per call, a scannable header (status, duration,
model, HTTP status, token summary) plus collapsible Request/Response showing
pretty-printed JSON, error bodies highlighted.
Net -495 lines; tests updated to assert on the request/response shape, with a
new test covering error-body capture on HTTP 400.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughModel call stacks transition from multi-layer debugging metadata to simple per-call request/response records. The backend records only provider API request and response details, centralizes provider error parsing, the renderer shows collapsible request/response sections, and tests/assertions are updated end-to-end. ChangesStack recording refactored to per-call request/response capture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 `@desktop/electron/renderer/src/components/StacksPage.jsx`:
- Around line 134-175: Both RequestSection and ResponseSection eagerly call
stringifyJson during initial render; change them to defer serialization until
the user expands the <details>. Add local state (e.g., expanded) in
RequestSection and ResponseSection, set it via the details onToggle handler (use
event.currentTarget.open), and only call stringifyJson(request.body) /
stringifyJson(response.body) when expanded is true; remove any immediate
stringifyJson calls so large bodies are not serialized on initial render.
In `@desktop/electron/renderer/styles.css`:
- Around line 652-668: Add an accessible keyboard focus style for the disclosure
toggle by defining a visible focus state on .stack-section > summary (use :focus
and/or :focus-visible) so keyboard users see when the summary has focus; update
the selector .stack-section > summary to include a focus rule that provides a
clear contrasting outline or box-shadow and preserves existing hover/background
styles (ensure it targets the same element that had ::-webkit-details-marker
hidden so the keyboard focus is applied to the visible summary toggle).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f47959d-81d0-4056-bf13-0639ba8c31b1
📒 Files selected for processing (12)
desktop/electron/renderer/src/components/StacksPage.jsxdesktop/electron/renderer/styles.cssdesktop/electron/renderer/window.d.tssrc/agent.rssrc/lib.rssrc/model.rssrc/stack_store.rssrc/stacks.rstests/local_backend.rstests/openai_model.rstests/session.rstests/stack_store.rs
Two follow-ups on the stacks view: - Request/Response sections now start collapsed and serialize their JSON body only after the user expands them (tracked via the details onToggle handler). A session can hold hundreds of calls, each carrying the full conversation context, so pretty-printing every body on first paint was wasted work. - Add a :focus-visible style to the section summary. The disclosure marker is hidden, so keyboard users previously had no focus indicator; use the accent ring convention (inset box-shadow, since the call card clips overflow).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The stacks view was supposed to give clarity into exactly what nav sent to the LLM and exactly what came back, per turn, so you can reason about context management (what to drop, what to summarize). The old implementation didn't do that:
messagesand reconstructed streamed responses rather than storing the real payloads.http status: 400with no explanation.What changed
One faithful record per model call, captured on the backend:
http_status_as_error(false)and branch onis_success(), reading and storing the provider's error body on a non-2xx. Centralized incapture_status_or_errorso every adapter (completions, responses, codex-responses) behaves identically.MockModelgainsrespond_with_traceso the offline/test path exercises the capture seam.~/.nav/stacks.jsonlwas discarded).Net −495 lines.
Tests
tests/openai_model.rs: newa_provider_error_body_is_captured_in_the_traceasserts the HTTP 400 body lands intrace.response_payloadand the detail reaches the error message.tests/session.rs/tests/local_backend.rs/tests/stack_store.rs: rewritten to assert on therequest/responseshape instead of layers.Verification
cargo test→ 189 passedcargo clippy --all-targets→ cleanrustfmt --edition 2024appliedbiome check→ clean; renderer builds;npm run check:electron→ 46 passedSummary by CodeRabbit
New Features
Bug Fixes
Style