mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
Make transcript list spacing uniform after streaming (#46899)
## Why Adding blank lines only after multiline items gives lists uneven spacing. Completed lists in redrawable transcript views can use consistent spacing based on their rendered width. ## What changed - Keep assistant and plan lists compact while streaming in the fullscreen transcript, without holding back completed list rows. - On completion or interruption, separate all sibling items if any item spans multiple rows; keep single-line lists compact and recompute spacing on resize. - Preserve the reader's displayed content when consolidating streamed cells, and include spacing policy in the Markdown render cache key. - Retain existing list spacing for native terminal scrollback. ## Testing Add regression and snapshot coverage for streaming completion and interruption, width-dependent reflow, reader position, nested and quoted lists, item paragraphs, and preservation of links and source metadata. GitOrigin-RevId: b2230aaf76ae798ebb6bf15563f8b8dcb6f3d19d
This commit is contained in:
@@ -0,0 +1,44 @@
|
||||
//! List spacing is a layout policy, not a streaming boundary. Completed redrawable lists use
|
||||
//! uniform sibling separators; streaming lists stay compact until source-backed consolidation.
|
||||
|
||||
use crate::terminal_hyperlinks::HyperlinkLine;
|
||||
|
||||
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
|
||||
pub(crate) enum ListSpacing {
|
||||
/// Preserve the historical spacing for terminal-owned scrollback.
|
||||
#[default]
|
||||
AfterMultiline,
|
||||
/// Do not add separators between siblings while an owned transcript streams.
|
||||
Compact,
|
||||
/// Separate every sibling if any item occupies multiple rendered rows.
|
||||
Uniform,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
pub(super) struct UniformList {
|
||||
pub(super) has_item: bool,
|
||||
pub(super) multiline: bool,
|
||||
pub(super) separators: Vec<usize>,
|
||||
}
|
||||
|
||||
impl UniformList {
|
||||
pub(super) fn finish(self, lines: &mut Vec<HyperlinkLine>) {
|
||||
if self.multiline {
|
||||
return;
|
||||
}
|
||||
let mut separators = self.separators.into_iter().peekable();
|
||||
let Some(&start) = separators.peek() else {
|
||||
return;
|
||||
};
|
||||
// Only remove this list's provisional separators, preserving source and link metadata.
|
||||
let tail = lines.split_off(start);
|
||||
lines.extend(tail.into_iter().enumerate().filter_map(|(offset, line)| {
|
||||
if separators.peek() == Some(&(start + offset)) {
|
||||
separators.next();
|
||||
None
|
||||
} else {
|
||||
Some(line)
|
||||
}
|
||||
}));
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,58 @@
|
||||
//! Width-aware list policies preserve nested layout, paragraphs, and semantic link metadata.
|
||||
|
||||
use super::ListSpacing;
|
||||
use crate::markdown::render_markdown_agent_with_list_spacing;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn list_spacing_is_uniform_per_list_and_preserves_item_paragraphs() {
|
||||
let sources = [
|
||||
"1. One\n2. This item has enough words to wrap\n3. Three\n4. Four",
|
||||
"- Parent\n - Short\n - Small\n- Other\n\n> - Quote\n> - This quoted item has enough words to wrap\n> - End",
|
||||
"- First paragraph\n\n Second paragraph\n\n- Next\n\n- Last",
|
||||
"- One\n- ```text\n code\n ```\n- Three\n\n ```text\n after paragraph\n ```",
|
||||
"- https://example.com/a-very-long-path-with-many-segments\n- Short\n- End",
|
||||
];
|
||||
let mut stages = Vec::new();
|
||||
for source in sources {
|
||||
for spacing in [ListSpacing::Compact, ListSpacing::Uniform] {
|
||||
let lines = render_markdown_agent_with_list_spacing(
|
||||
source,
|
||||
Some(24),
|
||||
/*cwd*/ None,
|
||||
/*inline_visualization_context*/ None,
|
||||
spacing,
|
||||
);
|
||||
stages.push(format!(
|
||||
"{spacing:?}:\n{}",
|
||||
lines
|
||||
.iter()
|
||||
.map(|line| line.line.to_string().trim_end().to_owned())
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
));
|
||||
}
|
||||
}
|
||||
insta::assert_snapshot!(stages.join("\n\n---\n\n"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn list_spacing_preserves_links_and_source_when_removing_separators() {
|
||||
let source = "- [One](https://example.com/one)\n- [Two](https://example.com/two)\n- Three";
|
||||
let render = |spacing| {
|
||||
render_markdown_agent_with_list_spacing(
|
||||
source,
|
||||
Some(100),
|
||||
/*cwd*/ None,
|
||||
/*inline_visualization_context*/ None,
|
||||
spacing,
|
||||
)
|
||||
};
|
||||
let compact = render(ListSpacing::Compact);
|
||||
let uniform = render(ListSpacing::Uniform);
|
||||
assert_eq!(&compact, &uniform);
|
||||
assert_eq!(
|
||||
compact.iter().map(|line| &line.source).collect::<Vec<_>>(),
|
||||
uniform.iter().map(|line| &line.source).collect::<Vec<_>>()
|
||||
);
|
||||
}
|
||||
+109
@@ -0,0 +1,109 @@
|
||||
---
|
||||
source: tui/src/markdown_render/list_spacing_tests.rs
|
||||
expression: "stages.join(\"\\n\\n---\\n\\n\")"
|
||||
---
|
||||
Compact:
|
||||
1. One
|
||||
2. This item has enough
|
||||
words to wrap
|
||||
3. Three
|
||||
4. Four
|
||||
|
||||
---
|
||||
|
||||
Uniform:
|
||||
1. One
|
||||
|
||||
2. This item has enough
|
||||
words to wrap
|
||||
|
||||
3. Three
|
||||
|
||||
4. Four
|
||||
|
||||
---
|
||||
|
||||
Compact:
|
||||
- Parent
|
||||
- Short
|
||||
- Small
|
||||
- Other
|
||||
|
||||
> - Quote
|
||||
> - This quoted item has
|
||||
> enough words to wrap
|
||||
> - End
|
||||
|
||||
---
|
||||
|
||||
Uniform:
|
||||
- Parent
|
||||
- Short
|
||||
- Small
|
||||
|
||||
- Other
|
||||
|
||||
> - Quote
|
||||
>
|
||||
> - This quoted item has
|
||||
> enough words to wrap
|
||||
>
|
||||
> - End
|
||||
|
||||
---
|
||||
|
||||
Compact:
|
||||
- First paragraph
|
||||
|
||||
Second paragraph
|
||||
- Next
|
||||
- Last
|
||||
|
||||
---
|
||||
|
||||
Uniform:
|
||||
- First paragraph
|
||||
|
||||
Second paragraph
|
||||
|
||||
- Next
|
||||
|
||||
- Last
|
||||
|
||||
---
|
||||
|
||||
Compact:
|
||||
- One
|
||||
-
|
||||
code
|
||||
- Three
|
||||
|
||||
after paragraph
|
||||
|
||||
---
|
||||
|
||||
Uniform:
|
||||
- One
|
||||
|
||||
-
|
||||
code
|
||||
|
||||
- Three
|
||||
|
||||
after paragraph
|
||||
|
||||
---
|
||||
|
||||
Compact:
|
||||
- https://example.com/a-very-long-path-with-many-segments
|
||||
- Short
|
||||
- End
|
||||
|
||||
---
|
||||
|
||||
Uniform:
|
||||
- https://example.com/a-very-long-path-with-many-segments
|
||||
|
||||
- Short
|
||||
|
||||
- End
|
||||
@@ -7,6 +7,7 @@ use super::DecodedTextMerge;
|
||||
use super::Event;
|
||||
use super::FileCitations;
|
||||
use super::HyperlinkLine;
|
||||
use super::ListSpacing;
|
||||
use super::Options;
|
||||
use super::Parser;
|
||||
use super::Tag;
|
||||
@@ -40,6 +41,7 @@ pub(crate) fn render_streaming_markdown_lines_with_width_and_cwd(
|
||||
width: Option<usize>,
|
||||
cwd: Option<&Path>,
|
||||
is_hidden_link_destination: &dyn Fn(&str) -> bool,
|
||||
list_spacing: ListSpacing,
|
||||
) -> StreamingMarkdownRender {
|
||||
let mut options = Options::empty();
|
||||
options.insert(Options::ENABLE_STRIKETHROUGH);
|
||||
@@ -57,6 +59,7 @@ pub(crate) fn render_streaming_markdown_lines_with_width_and_cwd(
|
||||
mermaid_start: None,
|
||||
};
|
||||
let mut writer = Writer::new(input, parser, width, cwd, is_hidden_link_destination);
|
||||
writer.list_spacing = list_spacing;
|
||||
writer.run();
|
||||
StreamingMarkdownRender {
|
||||
lines: writer.text,
|
||||
|
||||
@@ -238,6 +238,7 @@ fn streaming_and_full_render_agree_with_label_only_links() {
|
||||
Some(width),
|
||||
/*cwd*/ None,
|
||||
&|destination| display.hide_destination(destination),
|
||||
crate::markdown_render::ListSpacing::AfterMultiline,
|
||||
);
|
||||
assert_eq!(streamed.lines, render(prefix, width, display));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user