fix: improve network log readability and copying#10062
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR reformats network log entries for display, preserves a plain-text copy snapshot, and adds a Copy all header action.
Concerns
- The new header layout uses
Flex::new(vec![...]),.gap(...), andLength, which do not match the existing Warp UI API and will not compile. - Refreshing an initially empty pane updates
latest_plain_textwithout notifying the view, so the header can remain rendered without the Copy all button after entries appear.
Found: 1 critical, 1 important, 0 suggestions
Verdict
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| left_of_overflow: Some(if self.snapshot_is_empty() { | ||
| self.render_refresh_button(app) | ||
| } else { | ||
| Flex::new(vec![self.render_copy_button(app), self.render_refresh_button(app)]) |
There was a problem hiding this comment.
🚨 [CRITICAL] This does not compile: Flex::new expects an Axis, and this codebase uses Flex::row().with_spacing(...).with_child(...); Length/.gap() are not available, so the new import is also unresolved.
| let snapshot = NetworkLogModel::as_ref(ctx).snapshot_text(); | ||
| pub fn reload_snapshot(&mut self, ctx: &mut ViewContext<Self>) { | ||
| let snapshot = NetworkLogModel::as_ref(ctx).snapshot(); | ||
| self.latest_plain_text = snapshot.plain_text; |
There was a problem hiding this comment.
latest_plain_text, notify this view; otherwise refreshing an initially empty pane after entries arrive can update the editor while leaving the header rendered without the Copy all button.
|
The review feedback is addressed in the current branch:
Local validation completed: /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR formats network log entries for readability, preserves a raw plain-text snapshot for Copy all, and adds tests for snapshot joining/copy behavior.
Concerns
- The formatted request and response display drops headers that were previously visible through the debug output, which removes important network debugging context from the pane.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| .query() | ||
| .map_or(String::new(), |query| format!("?{query}")); | ||
|
|
||
| let mut lines = vec![ |
There was a problem hiding this comment.
request.headers(), so users lose header values like content type, request IDs, and auth redaction markers that were visible in the previous debug output and are often necessary for diagnosing network issues. Include a readable header section in display_text while keeping plain_text raw.
| .query() | ||
| .map_or(String::new(), |query| format!("?{query}")); | ||
|
|
||
| [ |
There was a problem hiding this comment.
response.headers(), which removes useful debugging context such as rate-limit, content-type, cache, and request-ID headers from the pane. Add headers to the formatted response output or otherwise preserve that header content in the display snapshot.
|
Hi @princepal9120 — a reviewer requested changes on this PR and it hasn't had activity from you in 23 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 30 days of inactivity. |
Previously, format_request_for_display and format_response_for_display omitted HTTP headers from the display text, losing debugging context that was previously visible via the Debug formatter. Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
|
/oz-review Latest commit adds HTTP headers to both request and response display output, restoring the debugging context that was previously visible via the Debug formatter. Headers are now shown in the display_text for both format_request_for_display and format_response_for_display. |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates the network log pane to render request/response entries in a more readable format, keeps a raw/plain-text snapshot for copying, adds a Copy all header action, and updates snapshot tests for the new blank-line formatting.
Concerns
⚠️ [IMPORTANT] This is a user-facing UI/behavior change, but the PR description does not include a screenshot or screen recording demonstrating the updated network log rendering and Copy all action end to end. Repo review guidance requires visual evidence for user-visible changes before approval.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Summary
Flex::row().with_spacing(...).with_child(...)API and notifying the view after snapshot refresh.Fixes #9716
Testing
git diff --checkcargo test -p warp network_logging, but this machine ran out of disk while compiling dependencies.