fix(gui-client): don't panic on existing session - #9779
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR prevents a panic when an existing session is detected during try_connect, replacing it with a warning log to gather more telemetry, and adds Debug derives for better visibility of session-related types.
- Replaced
assert!on an existing session with atracing::warn!and preserved behavior. - Added
#[derive(Debug)]toSessionandEventStreamfor richer logging.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rust/gui-client/src-tauri/src/service.rs | Removed panic assertion, added warning log and Debug derive. |
| rust/client-shared/src/lib.rs | Added Debug derives to Session and EventStream. |
Comments suppressed due to low confidence (2)
rust/gui-client/src-tauri/src/service.rs:603
- The doc comment for
try_connectwas removed; add or update documentation to reflect that it now logs a warning instead of panicking on an existing session.
fn try_connect(&mut self, api_url: &str, token: SecretString) -> Result<Session> {
rust/gui-client/src-tauri/src/service.rs:607
- Consider adding a unit or integration test to cover the new warning path when a session already exists, ensuring telemetry and behavior are correct.
tracing::warn!(session = ?self.session, "Connecting despite existing session");
| let started_at = Instant::now(); | ||
|
|
||
| assert!(self.session.is_none()); | ||
| if !self.session.is_none() { |
There was a problem hiding this comment.
[nitpick] For readability, prefer self.session.is_some() over !self.session.is_none(). This makes the intent clearer.
Suggested change
| if !self.session.is_none() { | |
| if self.session.is_some() { |
thomaseizinger
enabled auto-merge
July 3, 2025 22:52
jamilbk
approved these changes
Jul 3, 2025
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 3, 2025
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
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.
Customer hit what seems to be a rare race condition where we try to connect whilst we already have a session. I don't know which state it is in so I am replacing it with a WARN log to learn more about this in Sentry in case it gets hit again.