Switch wrapping to textwrap with adapter and unified prefixes#271
Switch wrapping to textwrap with adapter and unified prefixes#271
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace the bespoke incremental LineBuffer wrapper with a fragment-based wrapping pipeline: add Changes
Sequence DiagramsequenceDiagram
participant Markdown as Markdown Text
participant Classify as Block & Token Classifier
participant Fragment as Fragment Coalescer
participant Fit as textwrap::wrap_first_fit
participant Post as Post-Processor
participant Render as Line Renderer
Markdown->>Classify: Provide token spans (code, links, plain, whitespace)
Classify->>Fragment: Group tokens into InlineFragments and measure widths (unicode-width)
Fragment->>Fit: Supply fragment slice and target width
Fit->>Post: Return wrapped fragment lines
Post->>Render: Merge whitespace-only lines and rebalance atomic tails where allowed
Render->>Markdown: Emit final prefixed/continued lines
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRefactors the Markdown wrapping engine to delegate inline line-breaking to the Sequence diagram for inline wrapping delegation to textwrapsequenceDiagram
participant WT as wrap_text
participant PW as ParagraphWriter
participant WP as wrap_preserving_code
participant TW as textwrap_wrap_first_fit
WT->>PW: handle_prefix_or_paragraph_line(line, width)
alt Line has prefix
PW->>PW: wrap_with_prefix(prefix, continuation_prefix, text)
PW->>WP: wrap_preserving_code(text, available_width)
else Plain paragraph line
PW->>WP: wrap_preserving_code(text, width)
end
WP->>WP: tokenize::segment_inline(text)
WP->>WP: build_fragments(tokens) -> Vec<InlineFragment>
loop For each fragment
WP->>WP: candidate = buffer + fragment
WP->>TW: wrap_first_fit(candidate, [width])
TW-->>WP: raw_lines: Vec<Vec<InlineFragment>>
WP->>WP: merged = merge_whitespace_only_lines(raw_lines)
WP->>WP: rebalance_atomic_tails(merged)
alt merged.len() == 1
WP->>WP: buffer = candidate
else Multiple lines
WP->>WP: render_line(all_but_last, is_final_output_line=false)
WP-->>PW: push wrapped lines
WP->>WP: buffer = last_line
end
end
alt buffer not empty
WP->>WP: render_line(buffer, is_final_output_line=true)
WP-->>PW: push final wrapped line
end
PW-->>WT: emit prefixed wrapped lines
WT-->>WT: append lines to global output
Class diagram for updated wrapping and prefix handling typesclassDiagram
class ParagraphWriter {
-out : &mut Vec~String~
-width : usize
+new(out : &mut Vec~String~, width : usize) ParagraphWriter
-wrap_with_prefix(prefix : &str, continuation_prefix : &str, text : &str) void
-append_wrapped_with_prefix(line : &PrefixLine) void
+flush_paragraph(state : &mut ParagraphState) void
-push_wrapped_segment(indent : &str, segment : &str) void
+push_verbatim(state : &mut ParagraphState, line : &str) void
}
class PrefixLine {
+prefix : String
+rest : &str
+repeat_prefix : bool
}
class ParagraphState {
+buf : String
+indent : String
+has_prefix : bool
}
class InlineFragment {
+text : String
+width : usize
+new(text : String) InlineFragment
+width() usize
}
class Fragment {
<<trait>> Fragment
+width() f64
+whitespace_width() f64
+penalty_width() f64
}
class InlineWrapperModule {
<<module>> InlineWrapper
+wrap_preserving_code(text : &str, width : usize) Vec~String~
+attach_punctuation_to_previous_line(lines : &mut [String], current : &str, width : usize) bool
+determine_token_span(tokens : &[String], start : usize) (usize, usize)
+build_fragments(tokens : &[String]) Vec~InlineFragment~
+merge_whitespace_only_lines(lines : &[Vec~InlineFragment~]) Vec~Vec~InlineFragment~~
+rebalance_atomic_tails(lines : &mut [Vec~InlineFragment~]) void
+render_line(line : &[InlineFragment], is_final_output_line : bool) String
+width_as_f64(width : usize) f64
}
class TextwrapWrapFirstFit {
<<external>> textwrap_wrap_first_fit
+wrap_first_fit(fragments : &[InlineFragment], widths : &[f64]) Vec~&[InlineFragment]~
}
ParagraphWriter --> PrefixLine : uses
ParagraphWriter --> ParagraphState : uses
ParagraphWriter --> InlineWrapperModule : calls_wrap_preserving_code
InlineFragment ..|> Fragment
InlineWrapperModule --> InlineFragment : creates
InlineWrapperModule --> TextwrapWrapFirstFit : delegates_line_breaking
class TokenizeModule {
<<module>> tokenize
+segment_inline(text : &str) Vec~String~
}
InlineWrapperModule --> TokenizeModule : calls_segment_inline
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Assessment against linked issues
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 25500
No further work is required for these two items. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120e96d72f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/architecture.md (1)
387-391:⚠️ Potential issue | 🟡 MinorCorrect the flow diagram to the real wrapping path.
Rename this adapter step to the actual fragment-building/post-process helpers, and point the line-fitting step at
textwrap::wrap_algorithms::wrap_first_fit. The implementation does not expose anInlineTextwrapAdapter, and it does not calltextwrap::wraphere, so the diagram still misstates the runtime flow.Triage:
[type:docstyle]Run this read-only check to compare the diagram with the implementation:
#!/bin/bash set -euo pipefail echo "Diagram excerpt:" sed -n '383,392p' docs/architecture.md echo echo "Implementation call sites:" rg -n 'wrap_first_fit|textwrap::wrap\b|InlineTextwrapAdapter' src/wrap/inline.rs docs/architecture.mdAs per coding guidelines: "Ensure that any API or behavioural changes are reflected in the documentation in
docs/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` around lines 387 - 391, The diagram incorrectly names and points to a non-existent InlineTextwrapAdapter and textwrap::wrap; update the nodes in docs/architecture.md so node I is renamed to "fragment-building / post-process helpers" (or similar) and change the line-fitting step (node J) to reference textwrap::wrap_algorithms::wrap_first_fit instead of textwrap::wrap; ensure the arrow flow G -> I, H -> I, I -> J, and J -> K remain but with the corrected labels so the diagram matches the actual implementation (look for node labels I and J in the snippet and the identifier textwrap::wrap_algorithms::wrap_first_fit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md`:
- Around line 50-51: The doc line that instructs contributors to "Add `textwrap`
using a caret requirement" should be changed to instruct using implicit semver
(omit the leading caret) so it follows repository policy; update the sentence to
say something like "Add `textwrap` to Cargo.toml using implicit semver (no
leading `^`) compatible with Rust 1.89 and the 2024 edition" and ensure the
guidance references Cargo.toml and the `textwrap` dependency so contributors
know where to edit.
In `@src/wrap/inline/postprocess.rs`:
- Around line 52-54: Remove panicking expect/unwrap calls in the post-processing
path: replace the pop().expect(...) that sets previous_atomic (and the similar
expects in the 99-109 region) with non-panicking control flow that returns a
proper Result/Err or gracefully skips the fragment instead of panicking.
Concretely, change the previous_atomic assignment from
previous_line.pop().expect(...) to match previous_line.pop() { Some(prev) =>
prev, None => return Err(/* appropriate PostprocessError or contextual error */)
} (or an early continue if skipping is acceptable), and apply the same pattern
for each other expect/unwrap in this file so the function signature propagates
errors (e.g., Result<..., PostprocessError>) rather than aborting. Ensure error
variants include context about the missing fragment to aid debugging.
In `@src/wrap/tests.rs`:
- Around line 314-326: The test wrap_text_repeats_nested_blockquote_prefix
currently only checks line prefixes; update it to also validate the wrapped
payload content by either (a) stripping the repeated prefix string "> > " from
each line produced by wrap_text(&input, 80), concatenating the stripped lines
and asserting that this reassembled text equals the original unwrapped quote (or
equals the original after expected whitespace normalization), or (b) asserting
the exact expected wrapped lines array against the returned wrapped variable;
locate the test function wrap_text_repeats_nested_blockquote_prefix and add the
reassembly/assertion logic so the test fails if wrap_text drops or duplicates
content rather than only checking prefixes.
---
Duplicate comments:
In `@docs/architecture.md`:
- Around line 387-391: The diagram incorrectly names and points to a
non-existent InlineTextwrapAdapter and textwrap::wrap; update the nodes in
docs/architecture.md so node I is renamed to "fragment-building / post-process
helpers" (or similar) and change the line-fitting step (node J) to reference
textwrap::wrap_algorithms::wrap_first_fit instead of textwrap::wrap; ensure the
arrow flow G -> I, H -> I, I -> J, and J -> K remain but with the corrected
labels so the diagram matches the actual implementation (look for node labels I
and J in the snippet and the identifier
textwrap::wrap_algorithms::wrap_first_fit).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be0bb100-6467-4e16-8fa6-e80abdaa3e46
📒 Files selected for processing (6)
docs/architecture.mddocs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.mdsrc/wrap/inline.rssrc/wrap/inline/postprocess.rssrc/wrap/tests.rstests/wrap_unit.rs
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
…twrap and unicode-width Add a detailed execution plan document describing the refactor to replace the bespoke line-wrapping engine in the mdtablefix project with the `textwrap` and `unicode-width` crates. This living document outlines the purpose, constraints, risks, test coverage, and stepwise implementation plan. It serves to guide the maintainability-focused refactor ensuring preserved observable behavior while modernizing and simplifying the wrapping engine. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12924
No requirements are annulled. The original suggestions contained no en-GB-oxendict spelling violations (all uses of "normalize" etc. follow |
- Replaced the custom LineBuffer-based inline wrapping with a fragment-based approach leveraging textwrap::wrap_algorithms::wrap_first_fit for line fitting. - Preserved Markdown-aware token grouping and whitespace semantics by adapting fragment merging and suffix rebalancing. - Removed the now-obsolete src/wrap/line_buffer.rs module. - Updated paragraph wrapping to use the new inline wrapper with continuation prefixes. - Added active regression tests for nested blockquotes, footnote continuation alignment, and checkbox indentation. - Updated documentation reflecting the new wrapping strategy and integration with textwrap and unicode-width crates. - Added textwrap crate as a dependency and adjusted Cargo files accordingly. This improves maintainability by using a well-tested wrapping library while preserving existing formatting behaviors through a thin adaptation layer. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Added comprehensive Mermaid sequence and flow diagrams illustrating the control flow and sequence of the wrap_text function and its components. Included detailed explanations of how the wrapper decides on line classification, paragraph buffering, prefix-aware wrapping, use of unicode-width for display width measurement, and integration with the textwrap crate. These additions improve documentation clarity around the wrapping process and internal collaboration of components. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Ensure that the post-wrap fragment rebalancing respects line width constraints by checking the width before moving trailing fragments across lines. This prevents overflow lines that violate the width guarantees of `wrap_first_fit` and maintains correct wrapping behavior after post-processing. - Added width-aware checks in `rebalance_atomic_tails` to avoid creating overflow lines. - Refactored fragment classification to track fragment kinds for better whitespace and tail handling. - Moved whitespace-only line merging and tail rebalancing logic into a dedicated `postprocess` module. - Updated tests to verify no overflow after tail rebalancing, including adding a regression test for the `a four five` / width `6` case. - Updated documentation and sequence diagrams to reflect the new wrapping flow and helper module usage. This fix improves stability and correctness of inline wrapping post-processing steps. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
2389052 to
6ce5bc8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
docs/execplans/yaml-frontmatter.md (1)
99-100:⚠️ Potential issue | 🟡 MinorRun
make nixieunconditionally in the checklist and progress log.Replace the conditional wording and require
make nixieevery time so the validation flow remains deterministic and policy-compliant.Triage:
[type:docstyle]As per coding guidelines: "
**/*.md: Validate Mermaid diagrams in Markdown files by runningmake nixie."Also applies to: 264-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/yaml-frontmatter.md` around lines 99 - 100, Update the checklist and progress log entries so they require running `make nixie` unconditionally (remove conditional phrasing like "if Mermaid content changes"); specifically edit the "checklist" and "progress log" sections to state "Run `make nixie`" every time to ensure deterministic validation and compliance with the `**/*.md` Mermaid validation guideline.docs/architecture.md (1)
420-420:⚠️ Potential issue | 🟡 MinorRename the
InlineTextwrapAdapterstep to match real implementation terms.Use concrete names already present in the code path (for example,
wrap_preserving_codeand inline helpers) or explicitly label this as a conceptual stage.Triage:
[type:docstyle]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` at line 420, The diagram step labeled InlineTextwrapAdapter is misleading; update the docs to use the concrete implementation names used in the code path (e.g., rename the node to wrap_preserving_code and/or mention the inline helpers), or explicitly mark it as a conceptual stage called "inline text-wrapping (wrap_preserving_code / inline helpers)" so the documentation matches actual symbols like wrap_preserving_code and any inline helper functions.src/wrap/tests.rs (1)
342-354:⚠️ Potential issue | 🟡 MinorAssert payload integrity, not only prefix shape.
Reassemble the stripped payload and assert exact content so the test fails on dropped or duplicated text.
Patch
#[test] fn wrap_text_repeats_nested_blockquote_prefix() { @@ let wrapped = wrap_text(&input, 80); assert!(wrapped.len() > 1); assert!(wrapped.iter().all(|line| line.starts_with("> > "))); + let payload = wrapped + .iter() + .map(|line| line.strip_prefix("> > ").expect("expected nested blockquote prefix")) + .collect::<String>(); + assert_eq!( + payload, + "This nested quote contains enough text to require wrapping so that we can verify multi-level handling." + ); }As per coding guidelines: "Write unit and behavioural tests for new Rust functionality; run both before and after making any change."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wrap/tests.rs` around lines 342 - 354, The current test only asserts every wrapped line starts with the nested blockquote prefix but misses verifying the wrapped content integrity; update the test for wrap_text to strip the "> > " prefix from each line in wrapped (use the same prefix string used in the test), reassemble those stripped lines into a single string (preserving spacing/newlines the same way the original input payload would be joined) and assert that this reassembled payload exactly equals the original input payload (or the original input with leading prefix removed), so any dropped or duplicated words will cause the test to fail.docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md (1)
50-51:⚠️ Potential issue | 🟡 MinorReplace the explicit caret requirement.
State this as implicit semver versioning, not a caret requirement. The current wording conflicts with the repository policy for
Cargo.tomldependencies.📝 Proposed fix
-- Add `textwrap` using a caret requirement that is compatible with the - repository's Rust `1.89` minimum version and existing 2024 edition build. +- Add `textwrap` to `Cargo.toml` using implicit semver versioning (no leading + `^`) compatible with the repository's Rust `1.89` minimum version and + existing 2024 edition build.Triage:
[type:docstyle]As per coding guidelines: "Dependency versions in Cargo.toml should make use of implicit semver caret versioning and must not explicitly include carets."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md` around lines 50 - 51, The docs currently say "Add `textwrap` using a caret requirement..." which explicitly mentions a caret requirement; update the wording to describe implicit semver caret versioning instead (remove any mention of "caret requirement") and reference the dependency name `textwrap`, the target Rust minimum version `1.89`, and the `2024` edition/build so readers know compatibility while conforming to the Cargo.toml policy (i.e., say "use implicit semver versioning for `textwrap` compatible with Rust 1.89 and the 2024 edition" rather than "use a caret requirement").src/wrap/inline/postprocess.rs (1)
52-54:⚠️ Potential issue | 🟠 MajorRemove the production
expectcalls from post-processing.Skip the optimisation when the fragment shape is missing instead of panicking. These branches already predicate-check the shape, so any drift here should degrade gracefully rather than abort wrapping.
🛡️ Proposed fix
- let previous_atomic = previous_line - .pop() - .expect("line with an atomic tail contains that fragment"); + let Some(previous_atomic) = previous_line.pop() else { + continue; + }; pending_whitespace.push(previous_atomic); @@ - let trailing_width = lines[index] - .last() - .map(|fragment| fragment.width) - .expect("line selected for tail rebalancing contains a trailing fragment"); + let Some(trailing_width) = lines[index].last().map(|fragment| fragment.width) else { + continue; + }; if line_width(&lines[index + 1]) + trailing_width > width { continue; } - let trailing_fragment = lines[index] - .pop() - .expect("line selected for tail rebalancing contains a trailing fragment"); + let Some(trailing_fragment) = lines[index].pop() else { + continue; + }; lines[index + 1].insert(0, trailing_fragment);As per coding guidelines: ".expect() and .unwrap() are forbidden outside of tests. Errors must be propagated."
Also applies to: 99-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wrap/inline/postprocess.rs` around lines 52 - 54, The code uses expect(...) on previous_line.pop() (variable previous_atomic) and elsewhere (lines ~99-109) which causes panics; instead make these non-panicking by checking the pop result (e.g., if let Some(previous_atomic) = previous_line.pop() { ... } else { skip this optimisation/return early }) so the post-processing gracefully skips the optimisation when the atomic fragment is missing; update both the previous_atomic usage and the other expect sites in postprocess.rs to conditional handling and ensure no unwrap/expect remains so errors are propagated or degraded gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/architecture.md`:
- Line 420: The diagram step labeled InlineTextwrapAdapter is misleading; update
the docs to use the concrete implementation names used in the code path (e.g.,
rename the node to wrap_preserving_code and/or mention the inline helpers), or
explicitly mark it as a conceptual stage called "inline text-wrapping
(wrap_preserving_code / inline helpers)" so the documentation matches actual
symbols like wrap_preserving_code and any inline helper functions.
In `@docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md`:
- Around line 50-51: The docs currently say "Add `textwrap` using a caret
requirement..." which explicitly mentions a caret requirement; update the
wording to describe implicit semver caret versioning instead (remove any mention
of "caret requirement") and reference the dependency name `textwrap`, the target
Rust minimum version `1.89`, and the `2024` edition/build so readers know
compatibility while conforming to the Cargo.toml policy (i.e., say "use implicit
semver versioning for `textwrap` compatible with Rust 1.89 and the 2024 edition"
rather than "use a caret requirement").
In `@docs/execplans/yaml-frontmatter.md`:
- Around line 99-100: Update the checklist and progress log entries so they
require running `make nixie` unconditionally (remove conditional phrasing like
"if Mermaid content changes"); specifically edit the "checklist" and "progress
log" sections to state "Run `make nixie`" every time to ensure deterministic
validation and compliance with the `**/*.md` Mermaid validation guideline.
In `@src/wrap/inline/postprocess.rs`:
- Around line 52-54: The code uses expect(...) on previous_line.pop() (variable
previous_atomic) and elsewhere (lines ~99-109) which causes panics; instead make
these non-panicking by checking the pop result (e.g., if let
Some(previous_atomic) = previous_line.pop() { ... } else { skip this
optimisation/return early }) so the post-processing gracefully skips the
optimisation when the atomic fragment is missing; update both the
previous_atomic usage and the other expect sites in postprocess.rs to
conditional handling and ensure no unwrap/expect remains so errors are
propagated or degraded gracefully.
In `@src/wrap/tests.rs`:
- Around line 342-354: The current test only asserts every wrapped line starts
with the nested blockquote prefix but misses verifying the wrapped content
integrity; update the test for wrap_text to strip the "> > " prefix from each
line in wrapped (use the same prefix string used in the test), reassemble those
stripped lines into a single string (preserving spacing/newlines the same way
the original input payload would be joined) and assert that this reassembled
payload exactly equals the original input payload (or the original input with
leading prefix removed), so any dropped or duplicated words will cause the test
to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5cbad128-c227-49c0-883f-2fc54a7978d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlREADME.mddocs/architecture.mddocs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.mddocs/execplans/yaml-frontmatter.mddocs/trailing-spaces.mdsrc/wrap.rssrc/wrap/inline.rssrc/wrap/inline/postprocess.rssrc/wrap/line_buffer.rssrc/wrap/paragraph.rssrc/wrap/tests.rstests/wrap_unit.rs
💤 Files with no reviewable changes (2)
- src/wrap.rs
- src/wrap/line_buffer.rs
This comment was marked as resolved.
This comment was marked as resolved.
- Added new tests to verify nested blockquote prefix repetition in wrap_text. - Improved robustness in postprocess.rs by handling pop operations gracefully with else continue. - Updated architecture and execplan docs to reflect wrapping improvements and dependency changes with textwrap. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md`:
- Around line 183-186: Edit the sentence describing `rebalance_atomic_tails` so
it inserts a comma before "or" in the compound clause; specifically update the
line that currently reads "...must be width-aware or it can regress downstream
layout assumptions." to read "...must be width-aware, or it can regress
downstream layout assumptions." referencing the `rebalance_atomic_tails` mention
to locate the sentence.
- Around line 111-114: Edit the sentence "Risk: the architecture docs currently
describe the bespoke tokenizer flow and module relationships in detail.
Severity: low Likelihood: high Mitigation: update the docs as part of the same
change so the repository does not advertise an implementation that no longer
exists." by inserting a comma before "so" (i.e., "...update the docs as part of
the same change, so the repository...") to properly separate the two independent
clauses and improve readability.
- Around line 188-220: Every "Decision:" bullet entry in the Decision Log ends
with a "Date/Author: YYYY-MM-DD / Name" line but lacks terminal punctuation;
update each Decision entry so the "Date/Author:" line ends with a period. Locate
the Decision Log block containing the "Decision: ..." lines and the
corresponding "Date/Author:" suffixes and add a full stop to each of those lines
to ensure consistent terminal punctuation across entries.
In `@src/wrap/inline/postprocess.rs`:
- Around line 91-110: Add unit tests for rebalance_atomic_tails that exercise
its mutation logic using InlineFragment arrays: write tests that cover empty
lines, single-fragment lines, lines whose last fragment is rebalanceable and not
rebalanceable (use line_has_rebalanceable_tail), cases where the next line
starts with a single-space plain fragment
(line_starts_with_single_space_then_plain) and where it does not, and width
boundary conditions where line_width + trailing_width is equal to, less than, or
greater than the given width; assert that the trailing fragment is moved into
the next line only when all conditions are met and that other cases leave lines
unchanged. Ensure tests include fragments with varying width values to validate
the width checks and verify behavior when popping/inserting the
trailing_fragment.
- Around line 28-89: Add focused unit tests for merge_whitespace_only_lines that
cover all conditional branches: create inputs using Vec<InlineFragment> with
FragmentKind variants to exercise whitespace-only lines, single-space lines
(is_single_space_line), lines that start with atomic fragments
(line_starts_with_atomic), the inline-code-tail removal path (previous line last
fragment with FragmentKind::InlineCode), the previous-line-single-fragment path,
carry-over of pending_whitespace into the next non-empty line, and the final
pending_whitespace append behavior when the input ends with whitespace-only
lines; place tests in the same module as merge_whitespace_only_lines (or a tests
submodule) and assert the exact merged Vec<Vec<InlineFragment>> shapes and
fragment ordering for each scenario to ensure each branch behaves as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 570989e3-ed33-4140-be1f-383777928446
📒 Files selected for processing (4)
docs/architecture.mddocs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.mdsrc/wrap/inline/postprocess.rssrc/wrap/tests.rs
This comment was marked as resolved.
This comment was marked as resolved.
…pping - Delegate line fitting to `textwrap::wrap_algorithms::wrap_first_fit` for greedy, efficient wrapping - Introduce `InlineFragment` holding display width and fragment kind for correct Unicode width handling - Centralize prefix handling and line wrapping in `ParagraphWriter` with prefix-aware width calculation - Add post-processing to merge whitespace-only lines and rebalance atomic tails - Add detailed ADR explaining rationale and design decisions - Update developers and users guides with architecture and usage details - Maintain API stability, improve correctness for non-ASCII text and markdown inline elements - Add extensive tests verifying wrapping behavior, post-processing, and prefix handling This removes the fragile, bespoke LineBuffer state machine and relies on a well-tested upstream crate, improving correctness and maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/wrap/tests.rs (1)
158-164:⚠️ Potential issue | 🟡 MinorStrengthen the new prefix regressions.
Route
wrap_with_prefix_single_linethroughwrap_textwith an actual prefix, and assert exact wrapped output in the multiline cases. The current checks can still pass if the prefix path regresses, drops payload, or fails to split at all.As per coding guidelines: "Write unit and behavioural tests for new Rust functionality; run both before and after making any change."
Also applies to: 350-356, 391-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wrap/tests.rs` around lines 158 - 164, The test wrap_with_prefix_single_line currently calls wrap_preserving_code directly and doesn't exercise prefix handling; update it to call wrap_text with a real prefix (e.g., ">> ") and assert the exact output including the prefix so regressions in prefix handling or payload splitting are caught; do the same strengthening for the related tests around lines 350-356 and 391-396 by replacing direct wrap_preserving_code calls with wrap_text using an explicit prefix and asserting the exact wrapped lines (including prefix and line breaks) to validate multiline wrapping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adrs/0002-textwrap-wrapping-engine.md`:
- Around line 27-30: Update the ADR to use Oxford -ize spellings: replace
"normalises" with "normalizes", "centralised" with "centralized", and
"centralises" with "centralizes" in the text that references
src/wrap/inline/postprocess.rs and ParagraphWriter::wrap_with_prefix, and apply
the same replacements for the other occurrences noted (around the later block
mentioned as "also applies to: 46-48"); ensure all instances in this ADR use the
en-GB -ize forms consistently.
In `@docs/developers-guide.md`:
- Around line 115-124: The API reference table is missing a caption; insert a
short caption line immediately before the Markdown table (the block that lists
symbols such as FragmentKind, InlineFragment, classify_fragment,
build_fragments, wrap_preserving_code, merge_whitespace_only_lines,
rebalance_atomic_tails, ParagraphWriter, wrap_with_prefix, ParagraphState,
PrefixLine) — e.g. a one-line caption like "Table: Key types and functions" — so
the table has an explicit caption and meets the documentation guideline to
caption every table or diagram.
- Around line 101-113: The docs use British Oxford -ize convention but currently
contain "tokenised" and "normalise"; update those to "tokenized" and "normalize"
respectively so prose matches the repository's en-GB-oxendict -ize rule and
avoids mixing spellings next to the tokenize::segment_inline symbol; ensure
occurrences in the new wrapping section around references to
tokenize::segment_inline, build_fragments, InlineFragment, classify_fragment,
wrap_preserving_code, textwrap::wrap_algorithms::wrap_first_fit,
merge_whitespace_only_lines and rebalance_atomic_tails are all changed.
In `@src/wrap/tests.rs`:
- Around line 350-441: The test module has grown past the 400-line limit;
extract the prefix/continuation tests
(wrap_with_prefix_multiline_uses_continuation,
wrap_text_repeats_nested_blockquote_prefix,
wrap_with_prefix_plain_indent_both_lines,
wrap_text_preserves_prefixed_continuation_alignment) into a new test module/file
(e.g., a submodule tests::prefix or a new integration test file) and update any
use/imports so wrap_text is visible to the new module; keep the tests unchanged
aside from their module relocation so behavior and assertions remain identical.
---
Duplicate comments:
In `@src/wrap/tests.rs`:
- Around line 158-164: The test wrap_with_prefix_single_line currently calls
wrap_preserving_code directly and doesn't exercise prefix handling; update it to
call wrap_text with a real prefix (e.g., ">> ") and assert the exact output
including the prefix so regressions in prefix handling or payload splitting are
caught; do the same strengthening for the related tests around lines 350-356 and
391-396 by replacing direct wrap_preserving_code calls with wrap_text using an
explicit prefix and asserting the exact wrapped lines (including prefix and line
breaks) to validate multiline wrapping behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 263916e4-e3e3-4cec-80fa-cd2e08a88671
📒 Files selected for processing (7)
docs/adrs/0002-textwrap-wrapping-engine.mddocs/developers-guide.mddocs/users-guide.mdsrc/wrap/inline.rssrc/wrap/inline/postprocess.rssrc/wrap/paragraph.rssrc/wrap/tests.rs
This large refactor removes the old custom inline wrapping implementation and integrates the textwrap crate's `wrap_first_fit` algorithm combined with unicode-width for accurate display width calculations. Highlights: - New `InlineFragment` model carries precomputed unicode display widths and fragment kinds (Whitespace, InlineCode, Link, Plain). - Wrapping logic uses markdown-aware fragmentation to avoid splitting inline code spans and links. - Postprocessing merges whitespace-only lines and rebalances atomic tails to preserve legacy spacing semantics. - Paragraph wrapping now centralizes prefix width calculation and continuation prefixes to correctly align wrapped output. - Extensive test additions ensure behavior preservation and correctness across prefixes, code spans, links, and whitespace handling. Docs and guides updated to reflect the new wrapping architecture and pipeline stages, providing clarity on block classification, inline fragment construction, line fitting, and rendering steps. Overall this improves maintainability, correctness, and consistent unicode support for markdown wrapping. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/users-guide.md`:
- Around line 41-52: In the sentences "subsequent wrapped lines are indented to
the same visual column so the text stays aligned." (in the paragraph about
wrapped lines and prefixes) and "`mdtablefix --wrap` preserves those trailing
spaces on the final wrapped line so hard-break semantics are not lost after
reformatting." add a comma before "so" in each compound sentence so they read
"...visual column, so the text stays aligned." and "...final wrapped line, so
hard-break semantics are not lost after reformatting." This is the only change
needed: insert the comma before each "so" to separate the independent clauses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5d2236c-89ae-4595-b5e8-9705cc8c7d29
📒 Files selected for processing (11)
docs/adrs/0002-textwrap-wrapping-engine.mddocs/developers-guide.mddocs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.mddocs/users-guide.mdsrc/wrap/inline.rssrc/wrap/inline/postprocess.rssrc/wrap/inline/test_support.rssrc/wrap/inline/tests.rssrc/wrap/paragraph.rssrc/wrap/tests.rssrc/wrap/tests/prefix.rs
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…-export - Re-export FenceTracker and is_fence in src/wrap.rs for downstream use. - Refactor tokenize_markdown to improve inline markdown tokenization logic. - Extract and relocate push_newline_if_needed function within tokenize module. These changes improve fence detection utilities and streamline tokenization logic for inline markdown processing. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10537
No further work is required for either check. |
Summary
Switch the wrapping engine from a bespoke LineBuffer-driven path to a textwrap-based implementation, with a small adapter to preserve existing public entry points. This delivery consolidates display-width-aware prefix handling and introduces Markdown-aware fragment modeling that delegates line fitting to textwrap while preserving code spans, links, and trailing spaces.
Changes
textwrapto Cargo.toml (Rust 1.89+ compatibility / 2024 edition).unicode-widthremains present and used for display-width calculations.mdtablefix::wrap::wrap_text,Token,tokenize_markdown) continue to function for this first delivery.src/wrap.rs,src/wrap/paragraph.rs, andsrc/wrap/inline.rsas primary touchpoints.textwrapwhile preserving inline fragment semantics.src/wrap/inline/postprocess.rsandsrc/wrap/inline/test_support.rsfor post-processing and test helpers.src/wrap/tests.rs,tests/wrap_unit.rs,tests/wrap_cli.rs,tests/wrap_renumber.rs, and related CLI/markdown tests).textwrap+unicode-widthhandle width-sensitive breaking.docs/architecture.md, anddocs/trailing-spaces.mdto reflect architecture changes.docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md).Token,tokenize_markdown,wrap_text) preserved for this first pass.src/wrap.rs,src/wrap/paragraph.rs,src/wrap/inline.rs,src/wrap/inline/postprocess.rs,src/wrap/inline/test_support.rs,src/wrap/inline/tests.rs,src/wrap/tests.rs(updated), removal of deadsrc/wrap/line_buffer.rs.docs/execplans/replace-bespoke-wrapping-with-textwrap-and-unicode-width.md.Rationale
unicode-width.Testing plan
Acceptance criteria
src/wrap/paragraph.rsuses a single, display-width-aware helper for prefix wrapping instead of a two-step approach.tokenize_markdownremains intact and documented as out of scope for this delivery (or its broader removal is clearly approved with updated tests/docs).README.md,docs/architecture.md, anddocs/trailing-spaces.md) accurately describe the new implementation.Rollback plan
If acceptance criteria are not met or regressions appear, revert the changes to the prior wrapping path and re-evaluate in a narrower scope, then reintroduce
textwrapsupport in a controlled, incremental manner.Artifacts and notes
textwrapandunicode-widthcrates #80Summary by Sourcery
Refactor the Markdown wrapping engine to delegate inline line-breaking to the textwrap crate with unicode-width–based fragment widths while unifying display-width-aware prefix handling, and update documentation and tests to reflect and validate the new behavior.
Enhancements:
Documentation:
Tests:
Chores:
Task/Notes
textwrapandunicode-widthcrates #80