Refactor history_cell for clarity, intent, and safety #7079
+4,262
−2,431
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Reorganized history cells into dedicated modules, moving factory constructors alongside implementations so mod.rs mostly re-exports shared helpers. - Documented every cell and method with clear intent: where it appears, what it shows, how it renders (styling, wrapping, padding), plus # Output examples to convey expected layout without reading code. - Added comprehensive characterization tests and aligned insta snapshots per cell, using realistic data and wrapped/unwrapped scenarios to protect rendering behavior during future changes. - Ran fmt and scoped clippy fixes; cargo test -p codex-tui passes with updated snapshots.
| /// `Vec<Line<'static>>` representation to make it easier to display in a | ||
| /// scrollable list. | ||
| pub(crate) trait HistoryCell: std::fmt::Debug + Send + Sync + Any { | ||
| /// Render this cell into a set of display lines for the on-screen history panel. |
Collaborator
There was a problem hiding this comment.
can we avoid adding the term "panel"?
| mod web_search; | ||
|
|
||
| pub(crate) use agent::AgentMessageCell; | ||
| #[expect(unused_imports)] |
Collaborator
There was a problem hiding this comment.
Why all the unused imports?
Collaborator
Author
There was a problem hiding this comment.
The factory methods return these (fn new_foo() -> XxxCell) so they need to be exported, but they're not actually used anywhere as the types. This seems like a weird edge case in the way that clippy detects used imports. I've marked these as expected rather than allow so that if this bug is fixed we can just remove the attribute.
Comment on lines
+85
to
+86
| /// The width is the available area in the history list. Implementations should wrap or | ||
| /// truncate as needed so callers can drop the result directly into a `Paragraph`. |
Collaborator
There was a problem hiding this comment.
What does this mean?
| .unwrap_or(0) | ||
| } | ||
|
|
||
| /// Render this cell into a set of lines suitable for transcript export. |
Collaborator
There was a problem hiding this comment.
"export" is a weird word to use here
Comment on lines
+112
to
+113
| /// The default implementation matches `display_lines`, but cells can opt in to returning | ||
| /// additional context or omit styling-only padding when exporting. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// The default implementation matches `display_lines`, but cells can opt in to returning | |
| /// additional context or omit styling-only padding when exporting. | |
| /// The default implementation matches `display_lines`, but cells can opt in to returning | |
| /// a different rendering for transcript mode. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a prepatory refactor, with added characterization tests and documentation of the history cells code.
It prepares the code so that we can start more easily and safely making changes to the way these cells are rendered, and catch any regressions in the snapshots, tests etc.