mirror of
https://github.com/openai/codex.git
synced 2026-09-14 11:57:03 +00:00
fix(tui): trim CRLF in highlighted spans, enforce diff size guardrails
Two fixes from code review:
1. `trim_end_matches('\n')` left a stray `\r` on CRLF inputs, which
propagated into rendered code blocks and diffs. Now strips both
`\r` and `\n`.
2. Unified diff highlighting called `highlight_code_to_styled_spans`
per line, bypassing the global size guardrails (max bytes/lines).
On large patches this triggered thousands of parser initializations.
Added a pre-check on aggregate patch size that skips highlighting
when limits are exceeded.
This commit is contained in:
@@ -16,6 +16,7 @@ use unicode_width::UnicodeWidthChar;
|
||||
|
||||
use crate::exec_command::relativize_to_home;
|
||||
use crate::render::Insets;
|
||||
use crate::render::highlight::exceeds_highlight_limits;
|
||||
use crate::render::highlight::highlight_code_to_styled_spans;
|
||||
use crate::render::line_utils::prefix_lines;
|
||||
use crate::render::renderable::ColumnRenderable;
|
||||
@@ -266,10 +267,19 @@ fn render_change(
|
||||
FileChange::Update { unified_diff, .. } => {
|
||||
if let Ok(patch) = diffy::Patch::from_str(unified_diff) {
|
||||
let mut max_line_number = 0;
|
||||
let mut total_diff_bytes: usize = 0;
|
||||
let mut total_diff_lines: usize = 0;
|
||||
for h in patch.hunks() {
|
||||
let mut old_ln = h.old_range().start();
|
||||
let mut new_ln = h.new_range().start();
|
||||
for l in h.lines() {
|
||||
let text = match l {
|
||||
diffy::Line::Insert(t)
|
||||
| diffy::Line::Delete(t)
|
||||
| diffy::Line::Context(t) => t,
|
||||
};
|
||||
total_diff_bytes += text.len();
|
||||
total_diff_lines += 1;
|
||||
match l {
|
||||
diffy::Line::Insert(_) => {
|
||||
max_line_number = max_line_number.max(new_ln);
|
||||
@@ -287,6 +297,16 @@ fn render_change(
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Skip per-line syntax highlighting when the patch is too
|
||||
// large — avoids thousands of parser initializations that
|
||||
// would stall rendering on big diffs.
|
||||
let diff_lang = if exceeds_highlight_limits(total_diff_bytes, total_diff_lines) {
|
||||
None
|
||||
} else {
|
||||
lang
|
||||
};
|
||||
|
||||
let line_number_width = line_number_width(max_line_number);
|
||||
let mut is_first_hunk = true;
|
||||
for h in patch.hunks() {
|
||||
@@ -302,7 +322,7 @@ fn render_change(
|
||||
for l in h.lines() {
|
||||
// Per-line highlighting for unified diffs.
|
||||
let highlight_line = |s: &str| -> Option<Vec<RtSpan<'static>>> {
|
||||
let l = lang?;
|
||||
let l = diff_lang?;
|
||||
let spans = highlight_code_to_styled_spans(s, l)?;
|
||||
spans.into_iter().next()
|
||||
};
|
||||
@@ -1040,4 +1060,59 @@ mod tests {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn large_update_diff_skips_highlighting() {
|
||||
// Build a patch large enough to exceed MAX_HIGHLIGHT_LINES (10_000).
|
||||
// Without the pre-check this would attempt 10k+ parser initializations.
|
||||
let line_count = 10_500;
|
||||
let original: String = (0..line_count).map(|i| format!("line {i}\n")).collect();
|
||||
let modified: String = (0..line_count)
|
||||
.map(|i| {
|
||||
if i % 2 == 0 {
|
||||
format!("line {i} changed\n")
|
||||
} else {
|
||||
format!("line {i}\n")
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
let patch = diffy::create_patch(&original, &modified).to_string();
|
||||
|
||||
let mut changes: HashMap<PathBuf, FileChange> = HashMap::new();
|
||||
changes.insert(
|
||||
PathBuf::from("huge.rs"),
|
||||
FileChange::Update {
|
||||
unified_diff: patch,
|
||||
move_path: None,
|
||||
},
|
||||
);
|
||||
|
||||
// Should complete quickly (no per-line parser init). If guardrails
|
||||
// are bypassed this would be extremely slow.
|
||||
let lines = create_diff_summary(&changes, &PathBuf::from("/"), 80);
|
||||
|
||||
// The diff rendered without timing out — the guardrails prevented
|
||||
// thousands of per-line parser initializations. Verify we actually
|
||||
// got output (the patch is non-empty).
|
||||
assert!(
|
||||
lines.len() > 100,
|
||||
"expected many output lines from large diff, got {}",
|
||||
lines.len(),
|
||||
);
|
||||
|
||||
// No span should contain an RGB foreground color (syntax themes
|
||||
// produce RGB; plain diff styles only use named Color variants).
|
||||
for line in &lines {
|
||||
for span in &line.spans {
|
||||
if let Some(ratatui::style::Color::Rgb(..)) = span.style.fg {
|
||||
panic!(
|
||||
"large diff should not have syntax-highlighted spans, \
|
||||
got RGB color in style {:?} for {:?}",
|
||||
span.style,
|
||||
span.content,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -220,6 +220,15 @@ const MAX_HIGHLIGHT_BYTES: usize = 512 * 1024;
|
||||
/// Skip highlighting for inputs with more than 10,000 lines.
|
||||
const MAX_HIGHLIGHT_LINES: usize = 10_000;
|
||||
|
||||
/// Check whether an input exceeds the safe highlighting limits.
|
||||
///
|
||||
/// Callers that highlight content in a loop (e.g. per diff-line) should
|
||||
/// pre-check the aggregate size with this function and skip highlighting
|
||||
/// entirely when it returns `true`.
|
||||
pub(crate) fn exceeds_highlight_limits(total_bytes: usize, total_lines: usize) -> bool {
|
||||
total_bytes > MAX_HIGHLIGHT_BYTES || total_lines > MAX_HIGHLIGHT_LINES
|
||||
}
|
||||
|
||||
// -- Core highlighting --------------------------------------------------------
|
||||
|
||||
/// Parse `code` using syntect for `lang` and return per-line styled spans.
|
||||
@@ -247,8 +256,9 @@ fn highlight_to_line_spans(code: &str, lang: &str) -> Option<Vec<Vec<Span<'stati
|
||||
let ranges = h.highlight_line(line, syntax_set()).ok()?;
|
||||
let mut spans: Vec<Span<'static>> = Vec::new();
|
||||
for (style, text) in ranges {
|
||||
// Strip trailing newline since we handle line breaks ourselves.
|
||||
let text = text.trim_end_matches('\n');
|
||||
// Strip trailing line endings (LF and CR) since we handle line
|
||||
// breaks ourselves. CRLF inputs would otherwise leave a stray \r.
|
||||
let text = text.trim_end_matches(['\n', '\r']);
|
||||
if text.is_empty() {
|
||||
continue;
|
||||
}
|
||||
@@ -377,6 +387,23 @@ mod tests {
|
||||
assert_eq!(reconstructed(&lines), script);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn highlight_crlf_strips_carriage_return() {
|
||||
// Windows-style \r\n line endings must not leave a trailing \r in
|
||||
// span text — that would propagate into rendered code blocks.
|
||||
let code = "fn main() {\r\n println!(\"hi\");\r\n}\r\n";
|
||||
let lines = highlight_code_to_lines(code, "rust");
|
||||
for (i, line) in lines.iter().enumerate() {
|
||||
for span in &line.spans {
|
||||
assert!(
|
||||
!span.content.contains('\r'),
|
||||
"line {i} span {:?} contains \\r",
|
||||
span.content,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[allow(clippy::disallowed_methods)]
|
||||
fn style_conversion_correctness() {
|
||||
|
||||
Reference in New Issue
Block a user