Post-merge cleanup: honest MSRV and sub-agent sidebar/delete hardening - #120
Merged
Conversation
Declare MSRV 1.88: locked dependencies (time 0.3.53, plist 1.10.0) and the let-chain syntax adopted by the sub-agent feature already require rustc 1.88, so rustc 1.85-1.87 failed with confusing dependency errors instead of cargo's clean unsupported-toolchain message. Collapse the nested ifs that the version-aware clippy lint now flags into let chains. Decide cascade-delete view clearing from the freshly reloaded thread list, so a descendant materialized after the last sidebar refresh still clears the active transcript, keeping the pre-request set as a fallback when the reload fails. Render rootless thread rows after the hierarchy pass so a corrupted parent cycle stays visible for repair, as documented, instead of vanishing from the sidebar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SV1mewXCgSrpfqpXSThXND
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe workspace MSRV is raised from Rust 1.85 to 1.88. Rust conditional logic is streamlined without stated behavioral changes. Thread sidebar rendering and deletion cleanup now account for malformed relationships and refreshed project state, with expanded UI and end-to-end validation. ChangesThread consistency and support updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 `@AGENTS.md`:
- Line 64: Update the MSRV value in .coderabbit.yaml from 1.85 to 1.88 so
automated Rust review guidance matches the Edition 2024 convention documented in
AGENTS.md.
In `@crates/giskard-server/static/app.js`:
- Around line 1129-1137: Handle loadThreads(pid) separately from the outer
delete failure path: catch refresh errors around the loadThreads call, clear the
active view when deletedIds contains activeThread, and report the refresh
failure through the existing user-visible HTTP/WebSocket error mechanism without
claiming deletion failed. Preserve the authoritative-list check when refresh
succeeds.
🪄 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: 355608db-e2a7-4374-9014-2d2a888d5ee1
📒 Files selected for processing (7)
AGENTS.mdCargo.tomlREADME.mdcrates/giskard-persist/src/config.rscrates/giskard-persist/src/store.rscrates/giskard-server/static/app.jscrates/giskard-server/tests/ui.rs
…ing, test it Collapse every remaining nested `if let` the MSRV-1.88 collapsible_if lint flags into let chains so `cargo clippy --workspace --all-targets --locked -- -D warnings` is clean. The earlier MSRV bump only fixed the persist sites; a build from clean surfaces more across the harness adapters and the server (registry, routes, live_buffer, ledger, highlight, linkify, the replay binary, and several integration tests). These are mechanical, behavior-preserving conversions. Scope cascade-delete view clearing to the deleted thread's project. The active view is read after the awaits, so a delete in one project must not clear an unrelated thread the user navigated to in another project meanwhile. Cover that with an end-to-end Playwright test: two fresh projects, view a thread in one, delete a thread in the other, and assert the active selection survives. It creates threads through the start API to avoid the shared composer's WebSocket-open timing, and is verified to fail without the project-scope guard. Align `.coderabbit.yaml`'s stated MSRV with the 1.88 bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SV1mewXCgSrpfqpXSThXND
This was referenced Jul 24, 2026
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
2 participants
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.
Follow-up to #119, addressing the valid CodeRabbit findings from that review.
Declare MSRV 1.88 instead of 1.85 (
Cargo.toml,AGENTS.md,README.md). Verified empirically withcargo +1.85 check: the declared 1.85 was already unbuildable before #119 because the locked dependenciestime@0.3.53andplist@1.10.0require rustc 1.88, and #119 additionally introduced let-chain syntax (stabilized in 1.88) at four sites. CodeRabbit's suggested nested-ifrewrite would therefore not have restored 1.85. With the honest declaration, rustc 1.85–1.87 now gets cargo's clean "requires rustc 1.88" error instead of confusing dependency compile failures. The version-awarecollapsible_ifclippy lint activated by the bump flags the pre-existing nestedif lets ingiskard-persist; they are collapsed into let chains, matching the style the codebase already adopted.Cascade-delete view clearing uses the refreshed thread list (
app.js). Whether the active transcript view was deleted is now decided from the authoritative sidebar reload after the DELETE, so a descendant that materialized after the last refresh still clears correctly; the pre-request descendant set is kept as a fallback for when the reload fails.Rootless thread rows stay visible (
app.js).appendThreadRowsonly rendered nodes reachable from a root, so a corrupted parent cycle silently vanished from the sidebar — contradicting the documented guarantee that malformed records remain visible for repair. A guarded second pass now renders any leftover rows without duplicates or recursion risk.Both
app.jsbehaviors are pinned by newui.rsstatic-content assertions, and the stale assertion matching the old delete code is updated.Validation:
cargo fmt/clippy --all-targets -D warningsclean, full workspace suite 461 passed / 0 failed, Playwright suite 11/11 againstgiskard-server-replay, andcargo +1.85 checkconfirmed to fail with the clean unsupported-toolchain message.🤖 Generated with Claude Code
https://claude.ai/code/session_01SV1mewXCgSrpfqpXSThXND
Generated by Claude Code
Summary by CodeRabbit