-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Demote non-actionable report_error! calls to log::warn! in terminal model #13911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,14 +10,14 @@ use itertools::Itertools; | |
| use lazy_static::lazy_static; | ||
| use parking_lot::Mutex; | ||
| use rangemap::{RangeInclusiveMap, StepLite}; | ||
| use warp_errors::report_error; | ||
| use warpui::elements::SecretRange; | ||
| use warpui::EntityId; | ||
|
|
||
| use super::grid::grid_handler::GridHandler; | ||
| use super::grid::{Dimensions as _, RespectDisplayedOutput}; | ||
| use super::terminal_model::RangeInModel; | ||
| use crate::ai::blocklist::TextLocation; | ||
| use crate::safe_warn; | ||
| use crate::terminal::model::find::RegexDFAs; | ||
| use crate::terminal::model::index::Point; | ||
|
|
||
|
|
@@ -377,8 +377,10 @@ pub fn set_user_and_enterprise_secret_regexes<'a>( | |
| let dfas = match RegexDFAs::new_many(&all_secrets, false, true) { | ||
| Ok(dfas) => dfas, | ||
| Err(err) => { | ||
| report_error!(anyhow::Error::new(err) | ||
| .context("Failed to construct new RegexDFA with combined secrets")); | ||
| safe_warn!( | ||
| safe: ("Failed to construct new RegexDFA with combined secrets"), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| full: ("Failed to construct new RegexDFA with combined secrets: {err:#}") | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
@@ -392,8 +394,10 @@ pub fn set_user_and_enterprise_secret_regexes<'a>( | |
| }, | ||
| }, | ||
| Err(err) => { | ||
| report_error!(anyhow::Error::new(err) | ||
| .context("Failed to construct new Regex with combined secrets")); | ||
| safe_warn!( | ||
| safe: ("Failed to construct new Regex with combined secrets"), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning as the RegexDFA case above: a failure to build the combined secret regex is external config, gracefully handled (old regexes retained), so |
||
| full: ("Failed to construct new Regex with combined secrets: {err:#}") | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -528,9 +528,9 @@ impl Sessions { | |
| if let Some(in_band_command_output_tx) = self.in_band_command_output_tx_map.get(&session_id) | ||
| { | ||
| if let Err(e) = in_band_command_output_tx.try_send(event) { | ||
| report_error!(anyhow::Error::new(e).context( | ||
| "Failed to send ExecutedExecutorCommandEvent to InBandCommandExecutor" | ||
| )); | ||
| log::warn!( | ||
| "Failed to send ExecutedExecutorCommandEvent to InBandCommandExecutor: {e:#}" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -740,7 +740,7 @@ impl SessionInfo { | |
| } | ||
| } | ||
| Err(e) => { | ||
| warp_errors::report_error!(e); | ||
| log::warn!("Failed to get local hostname when determining session type: {e:#}"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| BootstrapSessionType::Local | ||
| } | ||
| } | ||
|
|
@@ -1560,7 +1560,7 @@ impl Session { | |
| ) | ||
| } | ||
| CommandExitStatus::Failure => { | ||
| report_error!("Failed to parse history file from file"); | ||
| log::warn!("Failed to parse history file from file"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| None | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ use async_trait::async_trait; | |
| use parking_lot::{Mutex, MutexGuard}; | ||
| use warp_completer::completer::{CommandExitStatus, CommandOutput}; | ||
| use warp_core::command::ExitCode; | ||
| use warp_errors::report_error; | ||
| use warp_terminal::model::Point; | ||
| use warp_util::on_cancel::OnCancelFutureExt; | ||
| use warpui::r#async::block_on; | ||
|
|
@@ -226,9 +225,9 @@ impl InBandCommandExecutor { | |
| } | ||
| }; | ||
| if let Err(error) = output_tx.try_send(command_output) { | ||
| report_error!(anyhow::Error::new(error).context( | ||
| "Error occurred when sending generator command output" | ||
| )); | ||
| log::warn!( | ||
| "Error occurred when sending generator command output: {error}" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,7 @@ use super::session::{BootstrapSessionType, InBandCommandOutputReceiver, SessionI | |
| use super::{Secret, SecretHandle}; | ||
| use crate::ai::ambient_agents::AmbientAgentTaskId; | ||
| use crate::ai::blocklist::SerializedBlockListItem; | ||
| use crate::safe_warn; | ||
| use crate::terminal::available_shells::AvailableShell; | ||
| use crate::terminal::block_filter::BlockFilterQuery; | ||
| use crate::terminal::block_list_element::GridType; | ||
|
|
@@ -3016,7 +3017,7 @@ impl ansi::Handler for TerminalModel { | |
| // Not being able to read the value should not cause a full-app crash. Instead, | ||
| // bootstrapping should fail in the same way that it would if the DCS message | ||
| // were otherwise corrupted. | ||
| report_error!("Received bootstrap message with no pending session info."); | ||
| log::warn!("Received bootstrap message with no pending session info."); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| return; | ||
| } | ||
| }; | ||
|
|
@@ -3162,9 +3163,9 @@ impl ansi::Handler for TerminalModel { | |
| uname: data.uname, | ||
| })) | ||
| } | ||
| None => report_error!( | ||
| "Received invalid shell name in init_subshell", | ||
| extra: { "shell" => %data.shell } | ||
| None => safe_warn!( | ||
| safe: ("Received invalid shell name in init_subshell"), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| full: ("Received invalid shell name in init_subshell: {}", data.shell) | ||
| ), | ||
| } | ||
| } | ||
|
|
@@ -3186,9 +3187,12 @@ impl ansi::Handler for TerminalModel { | |
| )) | ||
| } | ||
| None => { | ||
| report_error!( | ||
| "Received invalid shell name in SourcedRCFileForWarpValue", | ||
| extra: { "shell" => %data.shell } | ||
| safe_warn!( | ||
| safe: ("Received invalid shell name in SourcedRCFileForWarpValue"), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the |
||
| full: ( | ||
| "Received invalid shell name in SourcedRCFileForWarpValue: {}", | ||
| data.shell | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ use std::sync::mpsc::SyncSender; | |
| use std::sync::Arc; | ||
|
|
||
| use parking_lot::FairMutex; | ||
| use warp_errors::report_error; | ||
| use warpui::{AppContext, ModelHandle, SingletonEntity}; | ||
|
|
||
| use crate::persistence::{ModelEvent, StartedCommandMetadata}; | ||
|
|
@@ -69,7 +68,7 @@ pub fn update_command_history( | |
| .spawn(async move { | ||
| // Sending over a sync sender can block the current thread, so we do this async. | ||
| if let Err(e) = sender_clone.send(insert_command_event) { | ||
| report_error!(anyhow::Error::new(e).context("Error sending ModelEvent")); | ||
| log::warn!("Error sending ModelEvent: {e:#}"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| } | ||
| }) | ||
| .detach(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,8 +5,6 @@ use std::sync::Arc; | |
| use async_channel::{Receiver, Sender}; | ||
| use parking_lot::FairMutex; | ||
| use thiserror::Error; | ||
| #[cfg(feature = "local_fs")] | ||
| use warp_errors::report_error; | ||
| use warp_util::path::ShellFamily; | ||
| use warpui::r#async::block_on; | ||
| use warpui::{Entity, ModelContext, ModelHandle, SingletonEntity}; | ||
|
|
@@ -429,7 +427,7 @@ impl<T: EventLoopSender> PtyController<T> { | |
| self.source_bootstrap_script(path, shell_type, ctx); | ||
| } else { | ||
| self.write_terminating_bootstrap_bytes(ctx); | ||
| report_error!("Could not convert bootstrap script file path to str"); | ||
| log::warn!("Could not convert bootstrap script file path to str"); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Demotion |
||
| } | ||
|
|
||
| self.bootstrap_file = Some(file); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Demotion
report_error!→safe_warn!. Per logging-and-error-reporting,report_error!is reserved for actionable failures where our code is wrong.fs::remove_filefailing to clean up a kitty temp file is an external filesystem error we can't fix, and it's best-effort/handled (the siblingshm_unlinkpath just below already logs the same class atwarn!) — "non-ideal but expected" =warn!. Usedsafe_warn!because warn breadcrumbs are uploaded to Sentry and the temp path is user-controlled, so the path only appears in the dogfood-onlyfull:arm.