mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Route TUI /feedback submission through the app server (#16184)
The TUI’s `/feedback` flow was still uploading directly through the local feedback crate, which bypassed app-server behavior such as auth-derived feedback tags like chatgpt_user_id and made TUI feedback handling diverge from other clients. It also meant that remove TUI sessions failed to upload the correct feedback logs and session details. Testing: Manually tested `/feedback` flow and confirmed that it didn't regress.
This commit is contained in:
committed by
GitHub
Unverified
parent
03b2465591
commit
f396454097
+316
-7
@@ -3,6 +3,7 @@ use crate::app_command::AppCommand;
|
||||
use crate::app_command::AppCommandView;
|
||||
use crate::app_event::AppEvent;
|
||||
use crate::app_event::ExitMode;
|
||||
use crate::app_event::FeedbackCategory;
|
||||
use crate::app_event::RealtimeAudioDeviceKind;
|
||||
#[cfg(target_os = "windows")]
|
||||
use crate::app_event::WindowsSandboxEnableMode;
|
||||
@@ -55,6 +56,8 @@ use codex_app_server_client::TypedRequestError;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::CodexErrorInfo as AppServerCodexErrorInfo;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_app_server_protocol::FeedbackUploadParams;
|
||||
use codex_app_server_protocol::FeedbackUploadResponse;
|
||||
use codex_app_server_protocol::ListMcpServerStatusParams;
|
||||
use codex_app_server_protocol::ListMcpServerStatusResponse;
|
||||
use codex_app_server_protocol::McpServerStatus;
|
||||
@@ -502,6 +505,15 @@ enum ThreadBufferedEvent {
|
||||
Notification(ServerNotification),
|
||||
Request(ServerRequest),
|
||||
HistoryEntryResponse(GetHistoryEntryResponseEvent),
|
||||
FeedbackSubmission(FeedbackThreadEvent),
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
struct FeedbackThreadEvent {
|
||||
category: FeedbackCategory,
|
||||
include_logs: bool,
|
||||
feedback_audience: FeedbackAudience,
|
||||
result: Result<String, String>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
@@ -523,6 +535,7 @@ impl ThreadEventStore {
|
||||
ThreadBufferedEvent::Request(_)
|
||||
| ThreadBufferedEvent::Notification(ServerNotification::HookStarted(_))
|
||||
| ThreadBufferedEvent::Notification(ServerNotification::HookCompleted(_))
|
||||
| ThreadBufferedEvent::FeedbackSubmission(_)
|
||||
)
|
||||
}
|
||||
|
||||
@@ -627,7 +640,8 @@ impl ThreadEventStore {
|
||||
.pending_interactive_replay
|
||||
.should_replay_snapshot_request(request),
|
||||
ThreadBufferedEvent::Notification(_)
|
||||
| ThreadBufferedEvent::HistoryEntryResponse(_) => true,
|
||||
| ThreadBufferedEvent::HistoryEntryResponse(_)
|
||||
| ThreadBufferedEvent::FeedbackSubmission(_) => true,
|
||||
})
|
||||
.cloned()
|
||||
.collect(),
|
||||
@@ -1054,7 +1068,6 @@ impl App {
|
||||
model_catalog: self.model_catalog.clone(),
|
||||
feedback: self.feedback.clone(),
|
||||
is_first_run: false,
|
||||
feedback_audience: self.feedback_audience,
|
||||
status_account_display: self.chat_widget.status_account_display().cloned(),
|
||||
initial_plan_type: self.chat_widget.current_plan_type(),
|
||||
model: Some(self.chat_widget.current_model().to_string()),
|
||||
@@ -1942,6 +1955,124 @@ impl App {
|
||||
});
|
||||
}
|
||||
|
||||
fn submit_feedback(
|
||||
&mut self,
|
||||
app_server: &AppServerSession,
|
||||
category: FeedbackCategory,
|
||||
reason: Option<String>,
|
||||
include_logs: bool,
|
||||
) {
|
||||
let request_handle = app_server.request_handle();
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
let origin_thread_id = self.chat_widget.thread_id();
|
||||
let rollout_path = if include_logs {
|
||||
self.chat_widget.rollout_path()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let params = build_feedback_upload_params(
|
||||
origin_thread_id,
|
||||
rollout_path,
|
||||
category,
|
||||
reason,
|
||||
include_logs,
|
||||
);
|
||||
tokio::spawn(async move {
|
||||
let result = fetch_feedback_upload(request_handle, params)
|
||||
.await
|
||||
.map(|response| response.thread_id)
|
||||
.map_err(|err| err.to_string());
|
||||
app_event_tx.send(AppEvent::FeedbackSubmitted {
|
||||
origin_thread_id,
|
||||
category,
|
||||
include_logs,
|
||||
result,
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
fn handle_feedback_thread_event(&mut self, event: FeedbackThreadEvent) {
|
||||
match event.result {
|
||||
Ok(thread_id) => {
|
||||
self.chat_widget
|
||||
.add_to_history(crate::bottom_pane::feedback_success_cell(
|
||||
event.category,
|
||||
event.include_logs,
|
||||
&thread_id,
|
||||
event.feedback_audience,
|
||||
))
|
||||
}
|
||||
Err(err) => self
|
||||
.chat_widget
|
||||
.add_to_history(history_cell::new_error_event(format!(
|
||||
"Failed to upload feedback: {err}"
|
||||
))),
|
||||
}
|
||||
}
|
||||
|
||||
async fn enqueue_thread_feedback_event(
|
||||
&mut self,
|
||||
thread_id: ThreadId,
|
||||
event: FeedbackThreadEvent,
|
||||
) {
|
||||
let (sender, store) = {
|
||||
let channel = self.ensure_thread_channel(thread_id);
|
||||
(channel.sender.clone(), Arc::clone(&channel.store))
|
||||
};
|
||||
|
||||
let should_send = {
|
||||
let mut guard = store.lock().await;
|
||||
guard
|
||||
.buffer
|
||||
.push_back(ThreadBufferedEvent::FeedbackSubmission(event.clone()));
|
||||
if guard.buffer.len() > guard.capacity
|
||||
&& let Some(removed) = guard.buffer.pop_front()
|
||||
&& let ThreadBufferedEvent::Request(request) = &removed
|
||||
{
|
||||
guard
|
||||
.pending_interactive_replay
|
||||
.note_evicted_server_request(request);
|
||||
}
|
||||
guard.active
|
||||
};
|
||||
|
||||
if should_send {
|
||||
match sender.try_send(ThreadBufferedEvent::FeedbackSubmission(event)) {
|
||||
Ok(()) => {}
|
||||
Err(TrySendError::Full(event)) => {
|
||||
tokio::spawn(async move {
|
||||
if let Err(err) = sender.send(event).await {
|
||||
tracing::warn!("thread {thread_id} event channel closed: {err}");
|
||||
}
|
||||
});
|
||||
}
|
||||
Err(TrySendError::Closed(_)) => {
|
||||
tracing::warn!("thread {thread_id} event channel closed");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn handle_feedback_submitted(
|
||||
&mut self,
|
||||
origin_thread_id: Option<ThreadId>,
|
||||
category: FeedbackCategory,
|
||||
include_logs: bool,
|
||||
result: Result<String, String>,
|
||||
) {
|
||||
let event = FeedbackThreadEvent {
|
||||
category,
|
||||
include_logs,
|
||||
feedback_audience: self.feedback_audience,
|
||||
result,
|
||||
};
|
||||
if let Some(thread_id) = origin_thread_id {
|
||||
self.enqueue_thread_feedback_event(thread_id, event).await;
|
||||
} else {
|
||||
self.handle_feedback_thread_event(event);
|
||||
}
|
||||
}
|
||||
|
||||
/// Process the completed MCP inventory fetch: clear the loading spinner, then
|
||||
/// render either the full tool/resource listing or an error into chat history.
|
||||
///
|
||||
@@ -2536,6 +2667,9 @@ impl App {
|
||||
self.enqueue_thread_history_entry_response(thread_id, event)
|
||||
.await?;
|
||||
}
|
||||
ThreadBufferedEvent::FeedbackSubmission(event) => {
|
||||
self.enqueue_thread_feedback_event(thread_id, event).await;
|
||||
}
|
||||
}
|
||||
}
|
||||
self.chat_widget
|
||||
@@ -3457,7 +3591,6 @@ impl App {
|
||||
model_catalog: model_catalog.clone(),
|
||||
feedback: feedback.clone(),
|
||||
is_first_run,
|
||||
feedback_audience,
|
||||
status_account_display: status_account_display.clone(),
|
||||
initial_plan_type,
|
||||
model: Some(model.clone()),
|
||||
@@ -3492,7 +3625,6 @@ impl App {
|
||||
model_catalog: model_catalog.clone(),
|
||||
feedback: feedback.clone(),
|
||||
is_first_run,
|
||||
feedback_audience,
|
||||
status_account_display: status_account_display.clone(),
|
||||
initial_plan_type,
|
||||
model: config.model.clone(),
|
||||
@@ -3532,7 +3664,6 @@ impl App {
|
||||
model_catalog: model_catalog.clone(),
|
||||
feedback: feedback.clone(),
|
||||
is_first_run,
|
||||
feedback_audience,
|
||||
status_account_display: status_account_display.clone(),
|
||||
initial_plan_type,
|
||||
model: config.model.clone(),
|
||||
@@ -4293,6 +4424,22 @@ impl App {
|
||||
AppEvent::OpenFeedbackConsent { category } => {
|
||||
self.chat_widget.open_feedback_consent(category);
|
||||
}
|
||||
AppEvent::SubmitFeedback {
|
||||
category,
|
||||
reason,
|
||||
include_logs,
|
||||
} => {
|
||||
self.submit_feedback(app_server, category, reason, include_logs);
|
||||
}
|
||||
AppEvent::FeedbackSubmitted {
|
||||
origin_thread_id,
|
||||
category,
|
||||
include_logs,
|
||||
result,
|
||||
} => {
|
||||
self.handle_feedback_submitted(origin_thread_id, category, include_logs, result)
|
||||
.await;
|
||||
}
|
||||
AppEvent::LaunchExternalEditor => {
|
||||
if self.chat_widget.external_editor_state() == ExternalEditorState::Active {
|
||||
self.launch_external_editor(tui).await;
|
||||
@@ -5384,6 +5531,9 @@ impl App {
|
||||
ThreadBufferedEvent::HistoryEntryResponse(event) => {
|
||||
self.chat_widget.handle_history_entry_response(event);
|
||||
}
|
||||
ThreadBufferedEvent::FeedbackSubmission(event) => {
|
||||
self.handle_feedback_thread_event(event);
|
||||
}
|
||||
}
|
||||
if needs_refresh {
|
||||
self.refresh_status_line();
|
||||
@@ -5401,6 +5551,9 @@ impl App {
|
||||
ThreadBufferedEvent::HistoryEntryResponse(event) => {
|
||||
self.chat_widget.handle_history_entry_response(event)
|
||||
}
|
||||
ThreadBufferedEvent::FeedbackSubmission(event) => {
|
||||
self.handle_feedback_thread_event(event);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5869,6 +6022,38 @@ async fn fetch_plugin_uninstall(
|
||||
.wrap_err("plugin/uninstall failed in TUI")
|
||||
}
|
||||
|
||||
fn build_feedback_upload_params(
|
||||
origin_thread_id: Option<ThreadId>,
|
||||
rollout_path: Option<PathBuf>,
|
||||
category: FeedbackCategory,
|
||||
reason: Option<String>,
|
||||
include_logs: bool,
|
||||
) -> FeedbackUploadParams {
|
||||
let extra_log_files = if include_logs {
|
||||
rollout_path.map(|rollout_path| vec![rollout_path])
|
||||
} else {
|
||||
None
|
||||
};
|
||||
FeedbackUploadParams {
|
||||
classification: crate::bottom_pane::feedback_classification(category).to_string(),
|
||||
reason,
|
||||
thread_id: origin_thread_id.map(|thread_id| thread_id.to_string()),
|
||||
include_logs,
|
||||
extra_log_files,
|
||||
}
|
||||
}
|
||||
|
||||
async fn fetch_feedback_upload(
|
||||
request_handle: AppServerRequestHandle,
|
||||
params: FeedbackUploadParams,
|
||||
) -> Result<FeedbackUploadResponse> {
|
||||
let request_id = RequestId::String(format!("feedback-upload-{}", Uuid::new_v4()));
|
||||
request_handle
|
||||
.request_typed(ClientRequest::FeedbackUpload { request_id, params })
|
||||
.await
|
||||
.wrap_err("feedback/upload failed in TUI")
|
||||
}
|
||||
|
||||
/// Convert flat `McpServerStatus` responses into the per-server maps used by the
|
||||
/// in-process MCP subsystem (tools keyed as `mcp__{server}__{tool}`, plus
|
||||
/// per-server resource/template/auth maps). Test-only because the TUI
|
||||
@@ -6266,7 +6451,6 @@ mod tests {
|
||||
model_catalog: app.model_catalog.clone(),
|
||||
feedback: codex_feedback::CodexFeedback::new(),
|
||||
is_first_run: false,
|
||||
feedback_audience: app.feedback_audience,
|
||||
status_account_display: None,
|
||||
initial_plan_type: None,
|
||||
model: Some(model),
|
||||
@@ -9100,6 +9284,132 @@ guardian_approval = true
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_feedback_upload_params_includes_thread_id_and_rollout_path() {
|
||||
let thread_id = ThreadId::new();
|
||||
let rollout_path = PathBuf::from("/tmp/rollout.jsonl");
|
||||
|
||||
let params = build_feedback_upload_params(
|
||||
Some(thread_id),
|
||||
Some(rollout_path.clone()),
|
||||
FeedbackCategory::SafetyCheck,
|
||||
Some("needs follow-up".to_string()),
|
||||
/*include_logs*/ true,
|
||||
);
|
||||
|
||||
assert_eq!(params.classification, "safety_check");
|
||||
assert_eq!(params.reason, Some("needs follow-up".to_string()));
|
||||
assert_eq!(params.thread_id, Some(thread_id.to_string()));
|
||||
assert_eq!(params.include_logs, true);
|
||||
assert_eq!(params.extra_log_files, Some(vec![rollout_path]));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_feedback_upload_params_omits_rollout_path_without_logs() {
|
||||
let params = build_feedback_upload_params(
|
||||
/*origin_thread_id*/ None,
|
||||
Some(PathBuf::from("/tmp/rollout.jsonl")),
|
||||
FeedbackCategory::GoodResult,
|
||||
/*reason*/ None,
|
||||
/*include_logs*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(params.classification, "good_result");
|
||||
assert_eq!(params.reason, None);
|
||||
assert_eq!(params.thread_id, None);
|
||||
assert_eq!(params.include_logs, false);
|
||||
assert_eq!(params.extra_log_files, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn feedback_submission_without_thread_emits_error_history_cell() {
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
|
||||
app.handle_feedback_submitted(
|
||||
/*origin_thread_id*/ None,
|
||||
FeedbackCategory::Bug,
|
||||
/*include_logs*/ true,
|
||||
Err("boom".to_string()),
|
||||
)
|
||||
.await;
|
||||
|
||||
let cell = match app_event_rx.try_recv() {
|
||||
Ok(AppEvent::InsertHistoryCell(cell)) => cell,
|
||||
other => panic!("expected feedback error history cell, saw {other:?}"),
|
||||
};
|
||||
assert_eq!(
|
||||
lines_to_single_string(&cell.display_lines(/*width*/ 120)),
|
||||
"■ Failed to upload feedback: boom"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn feedback_submission_for_inactive_thread_replays_into_origin_thread() {
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
let origin_thread_id = ThreadId::new();
|
||||
let active_thread_id = ThreadId::new();
|
||||
let origin_session = test_thread_session(origin_thread_id, PathBuf::from("/tmp/origin"));
|
||||
let active_session = test_thread_session(active_thread_id, PathBuf::from("/tmp/active"));
|
||||
app.thread_event_channels.insert(
|
||||
origin_thread_id,
|
||||
ThreadEventChannel::new_with_session(
|
||||
THREAD_EVENT_CHANNEL_CAPACITY,
|
||||
origin_session.clone(),
|
||||
Vec::new(),
|
||||
),
|
||||
);
|
||||
app.thread_event_channels.insert(
|
||||
active_thread_id,
|
||||
ThreadEventChannel::new_with_session(
|
||||
THREAD_EVENT_CHANNEL_CAPACITY,
|
||||
active_session.clone(),
|
||||
Vec::new(),
|
||||
),
|
||||
);
|
||||
app.activate_thread_channel(active_thread_id).await;
|
||||
app.chat_widget.handle_thread_session(active_session);
|
||||
while app_event_rx.try_recv().is_ok() {}
|
||||
|
||||
app.handle_feedback_submitted(
|
||||
Some(origin_thread_id),
|
||||
FeedbackCategory::Bug,
|
||||
/*include_logs*/ true,
|
||||
Ok("uploaded-thread".to_string()),
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_matches!(
|
||||
app_event_rx.try_recv(),
|
||||
Err(tokio::sync::mpsc::error::TryRecvError::Empty)
|
||||
);
|
||||
|
||||
let snapshot = {
|
||||
let channel = app
|
||||
.thread_event_channels
|
||||
.get(&origin_thread_id)
|
||||
.expect("origin thread channel should exist");
|
||||
let store = channel.store.lock().await;
|
||||
assert!(matches!(
|
||||
store.buffer.back(),
|
||||
Some(ThreadBufferedEvent::FeedbackSubmission(_))
|
||||
));
|
||||
store.snapshot()
|
||||
};
|
||||
|
||||
app.replay_thread_snapshot(snapshot, /*resume_restored_queue*/ false);
|
||||
|
||||
let mut rendered_cells = Vec::new();
|
||||
while let Ok(event) = app_event_rx.try_recv() {
|
||||
if let AppEvent::InsertHistoryCell(cell) = event {
|
||||
rendered_cells.push(lines_to_single_string(&cell.display_lines(/*width*/ 120)));
|
||||
}
|
||||
}
|
||||
assert!(rendered_cells.iter().any(|cell| {
|
||||
cell.contains("• Feedback uploaded. Please open an issue using the following URL:")
|
||||
&& cell.contains("uploaded-thread")
|
||||
}));
|
||||
}
|
||||
|
||||
fn next_user_turn_op(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver<Op>) -> Op {
|
||||
let mut seen = Vec::new();
|
||||
while let Ok(op) = op_rx.try_recv() {
|
||||
@@ -10005,7 +10315,6 @@ guardian_approval = true
|
||||
model_catalog: app.model_catalog.clone(),
|
||||
feedback: app.feedback.clone(),
|
||||
is_first_run: false,
|
||||
feedback_audience: app.feedback_audience,
|
||||
status_account_display: app.chat_widget.status_account_display().cloned(),
|
||||
initial_plan_type: app.chat_widget.current_plan_type(),
|
||||
model: Some(app.chat_widget.current_model().to_string()),
|
||||
|
||||
@@ -518,6 +518,21 @@ pub(crate) enum AppEvent {
|
||||
category: FeedbackCategory,
|
||||
},
|
||||
|
||||
/// Submit feedback for the current thread via the app-server feedback RPC.
|
||||
SubmitFeedback {
|
||||
category: FeedbackCategory,
|
||||
reason: Option<String>,
|
||||
include_logs: bool,
|
||||
},
|
||||
|
||||
/// Result of a feedback upload request initiated by the TUI.
|
||||
FeedbackSubmitted {
|
||||
origin_thread_id: Option<ThreadId>,
|
||||
category: FeedbackCategory,
|
||||
include_logs: bool,
|
||||
result: Result<String, String>,
|
||||
},
|
||||
|
||||
/// Launch the external editor after a normal draw has completed.
|
||||
LaunchExternalEditor,
|
||||
|
||||
|
||||
@@ -1,6 +1,3 @@
|
||||
use std::cell::RefCell;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_feedback::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME;
|
||||
use codex_feedback::feedback_diagnostics::FeedbackDiagnostics;
|
||||
use crossterm::event::KeyCode;
|
||||
@@ -15,13 +12,13 @@ use ratatui::widgets::Clear;
|
||||
use ratatui::widgets::Paragraph;
|
||||
use ratatui::widgets::StatefulWidgetRef;
|
||||
use ratatui::widgets::Widget;
|
||||
use std::cell::RefCell;
|
||||
|
||||
use crate::app_event::AppEvent;
|
||||
use crate::app_event::FeedbackCategory;
|
||||
use crate::app_event_sender::AppEventSender;
|
||||
use crate::history_cell;
|
||||
use crate::render::renderable::Renderable;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
|
||||
use super::CancellationEvent;
|
||||
use super::bottom_pane_view::BottomPaneView;
|
||||
@@ -44,15 +41,12 @@ pub(crate) enum FeedbackAudience {
|
||||
External,
|
||||
}
|
||||
|
||||
/// Minimal input overlay to collect an optional feedback note, then upload
|
||||
/// both logs and rollout with classification + metadata.
|
||||
/// Minimal input overlay to collect an optional feedback note, then submit it
|
||||
/// through the app-server-managed feedback flow.
|
||||
pub(crate) struct FeedbackNoteView {
|
||||
category: FeedbackCategory,
|
||||
snapshot: codex_feedback::FeedbackSnapshot,
|
||||
rollout_path: Option<PathBuf>,
|
||||
app_event_tx: AppEventSender,
|
||||
include_logs: bool,
|
||||
feedback_audience: FeedbackAudience,
|
||||
|
||||
// UI state
|
||||
textarea: TextArea,
|
||||
@@ -63,19 +57,13 @@ pub(crate) struct FeedbackNoteView {
|
||||
impl FeedbackNoteView {
|
||||
pub(crate) fn new(
|
||||
category: FeedbackCategory,
|
||||
snapshot: codex_feedback::FeedbackSnapshot,
|
||||
rollout_path: Option<PathBuf>,
|
||||
app_event_tx: AppEventSender,
|
||||
include_logs: bool,
|
||||
feedback_audience: FeedbackAudience,
|
||||
) -> Self {
|
||||
Self {
|
||||
category,
|
||||
snapshot,
|
||||
rollout_path,
|
||||
app_event_tx,
|
||||
include_logs,
|
||||
feedback_audience,
|
||||
textarea: TextArea::new(),
|
||||
textarea_state: RefCell::new(TextAreaState::default()),
|
||||
complete: false,
|
||||
@@ -84,90 +72,12 @@ impl FeedbackNoteView {
|
||||
|
||||
fn submit(&mut self) {
|
||||
let note = self.textarea.text().trim().to_string();
|
||||
let reason_opt = if note.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(note.as_str())
|
||||
};
|
||||
let attachment_paths = if self.include_logs {
|
||||
self.rollout_path.iter().cloned().collect::<Vec<_>>()
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
let classification = feedback_classification(self.category);
|
||||
|
||||
let mut thread_id = self.snapshot.thread_id.clone();
|
||||
|
||||
let result = self.snapshot.upload_feedback(
|
||||
classification,
|
||||
reason_opt,
|
||||
self.include_logs,
|
||||
&attachment_paths,
|
||||
Some(SessionSource::Cli),
|
||||
/*logs_override*/ None,
|
||||
);
|
||||
|
||||
match result {
|
||||
Ok(()) => {
|
||||
let prefix = if self.include_logs {
|
||||
"• Feedback uploaded."
|
||||
} else {
|
||||
"• Feedback recorded (no logs)."
|
||||
};
|
||||
let issue_url =
|
||||
issue_url_for_category(self.category, &thread_id, self.feedback_audience);
|
||||
let mut lines = vec![Line::from(match issue_url.as_ref() {
|
||||
Some(_) if self.feedback_audience == FeedbackAudience::OpenAiEmployee => {
|
||||
format!("{prefix} Please report this in #codex-feedback:")
|
||||
}
|
||||
Some(_) => format!("{prefix} Please open an issue using the following URL:"),
|
||||
None => format!("{prefix} Thanks for the feedback!"),
|
||||
})];
|
||||
match issue_url {
|
||||
Some(url) if self.feedback_audience == FeedbackAudience::OpenAiEmployee => {
|
||||
lines.extend([
|
||||
"".into(),
|
||||
Line::from(vec![" ".into(), url.cyan().underlined()]),
|
||||
"".into(),
|
||||
Line::from(" Share this and add some info about your problem:"),
|
||||
Line::from(vec![
|
||||
" ".into(),
|
||||
format!("https://go/codex-feedback/{thread_id}").bold(),
|
||||
]),
|
||||
]);
|
||||
}
|
||||
Some(url) => {
|
||||
lines.extend([
|
||||
"".into(),
|
||||
Line::from(vec![" ".into(), url.cyan().underlined()]),
|
||||
"".into(),
|
||||
Line::from(vec![
|
||||
" Or mention your thread ID ".into(),
|
||||
std::mem::take(&mut thread_id).bold(),
|
||||
" in an existing issue.".into(),
|
||||
]),
|
||||
]);
|
||||
}
|
||||
None => {
|
||||
lines.extend([
|
||||
"".into(),
|
||||
Line::from(vec![
|
||||
" Thread ID: ".into(),
|
||||
std::mem::take(&mut thread_id).bold(),
|
||||
]),
|
||||
]);
|
||||
}
|
||||
}
|
||||
self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new(
|
||||
history_cell::PlainHistoryCell::new(lines),
|
||||
)));
|
||||
}
|
||||
Err(e) => {
|
||||
self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new(
|
||||
history_cell::new_error_event(format!("Failed to upload feedback: {e}")),
|
||||
)));
|
||||
}
|
||||
}
|
||||
let reason = if note.is_empty() { None } else { Some(note) };
|
||||
self.app_event_tx.send(AppEvent::SubmitFeedback {
|
||||
category: self.category,
|
||||
reason,
|
||||
include_logs: self.include_logs,
|
||||
});
|
||||
self.complete = true;
|
||||
}
|
||||
}
|
||||
@@ -382,7 +292,7 @@ fn feedback_title_and_placeholder(category: FeedbackCategory) -> (String, String
|
||||
}
|
||||
}
|
||||
|
||||
fn feedback_classification(category: FeedbackCategory) -> &'static str {
|
||||
pub(crate) fn feedback_classification(category: FeedbackCategory) -> &'static str {
|
||||
match category {
|
||||
FeedbackCategory::BadResult => "bad_result",
|
||||
FeedbackCategory::GoodResult => "good_result",
|
||||
@@ -392,6 +302,60 @@ fn feedback_classification(category: FeedbackCategory) -> &'static str {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn feedback_success_cell(
|
||||
category: FeedbackCategory,
|
||||
include_logs: bool,
|
||||
thread_id: &str,
|
||||
feedback_audience: FeedbackAudience,
|
||||
) -> history_cell::PlainHistoryCell {
|
||||
let prefix = if include_logs {
|
||||
"• Feedback uploaded."
|
||||
} else {
|
||||
"• Feedback recorded (no logs)."
|
||||
};
|
||||
let issue_url = issue_url_for_category(category, thread_id, feedback_audience);
|
||||
let mut lines = vec![Line::from(match issue_url.as_ref() {
|
||||
Some(_) if feedback_audience == FeedbackAudience::OpenAiEmployee => {
|
||||
format!("{prefix} Please report this in #codex-feedback:")
|
||||
}
|
||||
Some(_) => format!("{prefix} Please open an issue using the following URL:"),
|
||||
None => format!("{prefix} Thanks for the feedback!"),
|
||||
})];
|
||||
match issue_url {
|
||||
Some(url) if feedback_audience == FeedbackAudience::OpenAiEmployee => {
|
||||
lines.extend([
|
||||
"".into(),
|
||||
Line::from(vec![" ".into(), url.cyan().underlined()]),
|
||||
"".into(),
|
||||
Line::from(" Share this and add some info about your problem:"),
|
||||
Line::from(vec![
|
||||
" ".into(),
|
||||
format!("https://go/codex-feedback/{thread_id}").bold(),
|
||||
]),
|
||||
]);
|
||||
}
|
||||
Some(url) => {
|
||||
lines.extend([
|
||||
"".into(),
|
||||
Line::from(vec![" ".into(), url.cyan().underlined()]),
|
||||
"".into(),
|
||||
Line::from(vec![
|
||||
" Or mention your thread ID ".into(),
|
||||
thread_id.to_string().bold(),
|
||||
" in an existing issue.".into(),
|
||||
]),
|
||||
]);
|
||||
}
|
||||
None => {
|
||||
lines.extend([
|
||||
"".into(),
|
||||
Line::from(vec![" Thread ID: ".into(), thread_id.to_string().bold()]),
|
||||
]);
|
||||
}
|
||||
}
|
||||
history_cell::PlainHistoryCell::new(lines)
|
||||
}
|
||||
|
||||
fn issue_url_for_category(
|
||||
category: FeedbackCategory,
|
||||
thread_id: &str,
|
||||
@@ -625,18 +589,25 @@ mod tests {
|
||||
lines.join("\n")
|
||||
}
|
||||
|
||||
fn render_cell(cell: &impl history_cell::HistoryCell, width: u16) -> String {
|
||||
cell.display_lines(width)
|
||||
.into_iter()
|
||||
.map(|line| {
|
||||
line.spans
|
||||
.into_iter()
|
||||
.map(|span| span.content.into_owned())
|
||||
.collect::<String>()
|
||||
.trim_end()
|
||||
.to_string()
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
fn make_view(category: FeedbackCategory) -> FeedbackNoteView {
|
||||
let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let snapshot = codex_feedback::CodexFeedback::new().snapshot(/*session_id*/ None);
|
||||
FeedbackNoteView::new(
|
||||
category,
|
||||
snapshot,
|
||||
/*rollout_path*/ None,
|
||||
tx,
|
||||
/*include_logs*/ true,
|
||||
FeedbackAudience::External,
|
||||
)
|
||||
FeedbackNoteView::new(category, tx, /*include_logs*/ true)
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -678,33 +649,56 @@ mod tests {
|
||||
fn feedback_view_with_connectivity_diagnostics() {
|
||||
let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let diagnostics = FeedbackDiagnostics::new(vec![
|
||||
FeedbackDiagnostic {
|
||||
headline: "Proxy environment variables are set and may affect connectivity."
|
||||
.to_string(),
|
||||
details: vec!["HTTP_PROXY = http://proxy.example.com:8080".to_string()],
|
||||
},
|
||||
FeedbackDiagnostic {
|
||||
headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(),
|
||||
details: vec!["OPENAI_BASE_URL = https://example.com/v1".to_string()],
|
||||
},
|
||||
]);
|
||||
let snapshot = codex_feedback::CodexFeedback::new()
|
||||
.snapshot(/*session_id*/ None)
|
||||
.with_feedback_diagnostics(diagnostics);
|
||||
let view = FeedbackNoteView::new(
|
||||
FeedbackCategory::Bug,
|
||||
snapshot,
|
||||
/*rollout_path*/ None,
|
||||
tx,
|
||||
/*include_logs*/ false,
|
||||
FeedbackAudience::External,
|
||||
);
|
||||
let view = FeedbackNoteView::new(FeedbackCategory::Bug, tx, /*include_logs*/ false);
|
||||
let rendered = render(&view, /*width*/ 60);
|
||||
|
||||
insta::assert_snapshot!("feedback_view_with_connectivity_diagnostics", rendered);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn submit_feedback_emits_submit_event_with_trimmed_note() {
|
||||
let (tx_raw, mut rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let mut view = FeedbackNoteView::new(FeedbackCategory::Bug, tx, /*include_logs*/ true);
|
||||
view.textarea.insert_str(" something broke ");
|
||||
|
||||
view.submit();
|
||||
|
||||
let event = rx.try_recv().expect("submit feedback event");
|
||||
assert!(matches!(
|
||||
event,
|
||||
AppEvent::SubmitFeedback {
|
||||
category: FeedbackCategory::Bug,
|
||||
reason: Some(reason),
|
||||
include_logs: true,
|
||||
} if reason == "something broke"
|
||||
));
|
||||
assert_eq!(view.is_complete(), true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn submit_feedback_omits_empty_note() {
|
||||
let (tx_raw, mut rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let mut view = FeedbackNoteView::new(
|
||||
FeedbackCategory::GoodResult,
|
||||
tx,
|
||||
/*include_logs*/ false,
|
||||
);
|
||||
|
||||
view.submit();
|
||||
|
||||
let event = rx.try_recv().expect("submit feedback event");
|
||||
assert!(matches!(
|
||||
event,
|
||||
AppEvent::SubmitFeedback {
|
||||
category: FeedbackCategory::GoodResult,
|
||||
reason: None,
|
||||
include_logs: false,
|
||||
}
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn should_show_feedback_connectivity_details_only_for_non_good_result_with_diagnostics() {
|
||||
let diagnostics = FeedbackDiagnostics::new(vec![FeedbackDiagnostic {
|
||||
@@ -774,4 +768,76 @@ mod tests {
|
||||
let expected_external_url = "https://github.com/openai/codex/issues/new?template=3-cli.yml&steps=Uploaded%20thread:%20t";
|
||||
assert_eq!(bug_url_non_employee.as_deref(), Some(expected_external_url));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn feedback_success_cell_matches_external_bug_copy() {
|
||||
let rendered = render_cell(
|
||||
&feedback_success_cell(
|
||||
FeedbackCategory::Bug,
|
||||
/*include_logs*/ true,
|
||||
"thread-1",
|
||||
FeedbackAudience::External,
|
||||
),
|
||||
/*width*/ 120,
|
||||
);
|
||||
assert_eq!(
|
||||
rendered,
|
||||
"• Feedback uploaded. Please open an issue using the following URL:\n\n https://github.com/openai/codex/issues/new?template=3-cli.yml&steps=Uploaded%20thread:%20thread-1\n\n Or mention your thread ID thread-1 in an existing issue."
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn feedback_success_cell_matches_employee_bug_copy() {
|
||||
let rendered = render_cell(
|
||||
&feedback_success_cell(
|
||||
FeedbackCategory::Bug,
|
||||
/*include_logs*/ true,
|
||||
"thread-2",
|
||||
FeedbackAudience::OpenAiEmployee,
|
||||
),
|
||||
/*width*/ 120,
|
||||
);
|
||||
assert_eq!(
|
||||
rendered,
|
||||
"• Feedback uploaded. Please report this in #codex-feedback:\n\n http://go/codex-feedback-internal\n\n Share this and add some info about your problem:\n https://go/codex-feedback/thread-2"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn feedback_success_cell_matches_good_result_copy() {
|
||||
let rendered = render_cell(
|
||||
&feedback_success_cell(
|
||||
FeedbackCategory::GoodResult,
|
||||
/*include_logs*/ false,
|
||||
"thread-3",
|
||||
FeedbackAudience::External,
|
||||
),
|
||||
/*width*/ 120,
|
||||
);
|
||||
assert_eq!(
|
||||
rendered,
|
||||
"• Feedback recorded (no logs). Thanks for the feedback!\n\n Thread ID: thread-3"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn feedback_success_cell_uses_issue_links_for_remaining_categories() {
|
||||
for category in [
|
||||
FeedbackCategory::BadResult,
|
||||
FeedbackCategory::SafetyCheck,
|
||||
FeedbackCategory::Other,
|
||||
] {
|
||||
let rendered = render_cell(
|
||||
&feedback_success_cell(
|
||||
category,
|
||||
/*include_logs*/ false,
|
||||
"thread-4",
|
||||
FeedbackAudience::External,
|
||||
),
|
||||
/*width*/ 120,
|
||||
);
|
||||
assert!(rendered.contains("Please open an issue using the following URL:"));
|
||||
assert!(rendered.contains("thread-4"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -93,8 +93,10 @@ pub(crate) use list_selection_view::popup_content_width;
|
||||
pub(crate) use list_selection_view::side_by_side_layout_widths;
|
||||
mod feedback_view;
|
||||
pub(crate) use feedback_view::FeedbackAudience;
|
||||
pub(crate) use feedback_view::feedback_classification;
|
||||
pub(crate) use feedback_view::feedback_disabled_params;
|
||||
pub(crate) use feedback_view::feedback_selection_params;
|
||||
pub(crate) use feedback_view::feedback_success_cell;
|
||||
pub(crate) use feedback_view::feedback_upload_consent_params;
|
||||
pub(crate) use skills_toggle_view::SkillsToggleItem;
|
||||
pub(crate) use skills_toggle_view::SkillsToggleView;
|
||||
|
||||
@@ -301,7 +301,6 @@ use crate::bottom_pane::ColumnWidthMode;
|
||||
use crate::bottom_pane::DOUBLE_PRESS_QUIT_SHORTCUT_ENABLED;
|
||||
use crate::bottom_pane::ExperimentalFeatureItem;
|
||||
use crate::bottom_pane::ExperimentalFeaturesView;
|
||||
use crate::bottom_pane::FeedbackAudience;
|
||||
use crate::bottom_pane::InputResult;
|
||||
use crate::bottom_pane::LocalImageAttachment;
|
||||
use crate::bottom_pane::McpServerElicitationFormRequest;
|
||||
@@ -553,7 +552,6 @@ pub(crate) struct ChatWidgetInit {
|
||||
pub(crate) model_catalog: Arc<ModelCatalog>,
|
||||
pub(crate) feedback: codex_feedback::CodexFeedback,
|
||||
pub(crate) is_first_run: bool,
|
||||
pub(crate) feedback_audience: FeedbackAudience,
|
||||
pub(crate) status_account_display: Option<StatusAccountDisplay>,
|
||||
pub(crate) initial_plan_type: Option<PlanType>,
|
||||
pub(crate) model: Option<String>,
|
||||
@@ -915,7 +913,6 @@ pub(crate) struct ChatWidget {
|
||||
last_rendered_width: std::cell::Cell<Option<usize>>,
|
||||
// Feedback sink for /feedback
|
||||
feedback: codex_feedback::CodexFeedback,
|
||||
feedback_audience: FeedbackAudience,
|
||||
// Current session rollout path (if known)
|
||||
current_rollout_path: Option<PathBuf>,
|
||||
// Current working directory (if known)
|
||||
@@ -2115,28 +2112,18 @@ impl ChatWidget {
|
||||
category: crate::app_event::FeedbackCategory,
|
||||
include_logs: bool,
|
||||
) {
|
||||
let snapshot = self.feedback.snapshot(self.thread_id);
|
||||
self.show_feedback_note(category, include_logs, snapshot);
|
||||
self.show_feedback_note(category, include_logs);
|
||||
}
|
||||
|
||||
fn show_feedback_note(
|
||||
&mut self,
|
||||
category: crate::app_event::FeedbackCategory,
|
||||
include_logs: bool,
|
||||
snapshot: codex_feedback::FeedbackSnapshot,
|
||||
) {
|
||||
let rollout = if include_logs {
|
||||
self.current_rollout_path.clone()
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let view = crate::bottom_pane::FeedbackNoteView::new(
|
||||
category,
|
||||
snapshot,
|
||||
rollout,
|
||||
self.app_event_tx.clone(),
|
||||
include_logs,
|
||||
self.feedback_audience,
|
||||
);
|
||||
self.bottom_pane.show_view(Box::new(view));
|
||||
self.request_redraw();
|
||||
@@ -4649,7 +4636,6 @@ impl ChatWidget {
|
||||
model_catalog,
|
||||
feedback,
|
||||
is_first_run,
|
||||
feedback_audience,
|
||||
status_account_display,
|
||||
initial_plan_type,
|
||||
model,
|
||||
@@ -4788,7 +4774,6 @@ impl ChatWidget {
|
||||
turn_runtime_metrics: RuntimeMetricsSummary::default(),
|
||||
last_rendered_width: std::cell::Cell::new(None),
|
||||
feedback,
|
||||
feedback_audience,
|
||||
current_rollout_path: None,
|
||||
current_cwd,
|
||||
session_network_proxy: None,
|
||||
|
||||
@@ -10,7 +10,6 @@ use crate::app_event::ExitMode;
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
use crate::app_event::RealtimeAudioDeviceKind;
|
||||
use crate::app_event_sender::AppEventSender;
|
||||
use crate::bottom_pane::FeedbackAudience;
|
||||
use crate::bottom_pane::LocalImageAttachment;
|
||||
use crate::bottom_pane::MentionBinding;
|
||||
use crate::chatwidget::realtime::RealtimeConversationPhase;
|
||||
@@ -2009,7 +2008,6 @@ async fn helpers_are_available_and_do_not_panic() {
|
||||
model_catalog: test_model_catalog(&cfg),
|
||||
feedback: codex_feedback::CodexFeedback::new(),
|
||||
is_first_run: true,
|
||||
feedback_audience: FeedbackAudience::External,
|
||||
status_account_display: None,
|
||||
initial_plan_type: None,
|
||||
model: Some(resolved_model),
|
||||
@@ -2183,7 +2181,6 @@ async fn make_chatwidget_manual(
|
||||
turn_runtime_metrics: RuntimeMetricsSummary::default(),
|
||||
last_rendered_width: std::cell::Cell::new(None),
|
||||
feedback: codex_feedback::CodexFeedback::new(),
|
||||
feedback_audience: FeedbackAudience::External,
|
||||
current_rollout_path: None,
|
||||
current_cwd: None,
|
||||
session_network_proxy: None,
|
||||
@@ -6952,7 +6949,6 @@ async fn collaboration_modes_defaults_to_code_on_startup() {
|
||||
model_catalog: test_model_catalog(&cfg),
|
||||
feedback: codex_feedback::CodexFeedback::new(),
|
||||
is_first_run: true,
|
||||
feedback_audience: FeedbackAudience::External,
|
||||
status_account_display: None,
|
||||
initial_plan_type: None,
|
||||
model: Some(resolved_model.clone()),
|
||||
@@ -6997,7 +6993,6 @@ async fn experimental_mode_plan_is_ignored_on_startup() {
|
||||
model_catalog: test_model_catalog(&cfg),
|
||||
feedback: codex_feedback::CodexFeedback::new(),
|
||||
is_first_run: true,
|
||||
feedback_audience: FeedbackAudience::External,
|
||||
status_account_display: None,
|
||||
initial_plan_type: None,
|
||||
model: Some(resolved_model.clone()),
|
||||
|
||||
Reference in New Issue
Block a user