Fix spurious leading spaces in reflowed wrap output (#291) - #298
Conversation
Trim leading whitespace in `render_line` so reflow no longer emits spurious spaces at the start of wrapped lines, particularly after inline code spans. Add regression tests using the issue examples and update expectations that previously relied on carry whitespace. Co-authored-by: Cursor <cursoragent@cursor.com>
Fix spurious leading spaces in reflowed wrap outputThis PR resolves issue Technical change
Tests and validation
Related work
Outcome
WalkthroughTrim leading carry whitespace on wrapped continuation lines in the inline wrapping path and validate the change with updated unit tests, a new regression/proptest module, fixture updates, integration-test adjustments and documentation. ChangesSpurious leading spaces reflow fix
Sequence Diagram(s)No sequence diagram generated. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideFixes whitespace handling in inline wrapping so wrapped continuation lines no longer start with spurious leading spaces, while preserving list-item continuation indentation, and adds focused regression, property-based, and integration tests to lock in the behavior across paragraphs, lists, CLI, and footnotes. Flow diagram for inline render_line whitespace trimmingflowchart LR
A[InlineWrapper] --> B[render_line]
B --> C[collect InlineFragment text]
C --> D{last char is space?}
D -- yes --> E[text.pop]
D -- no --> F[skip pop]
E --> G[trim_start]
F --> G[trim_start]
G --> H[to_string]
H --> I[wrapped line without leading spaces]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Introduce exact before/after fixture tests for the reported paragraph, list, and ordered-list reflow cases, plus proptest coverage that rejects spurious carry whitespace on wrapped continuation lines while preserving legitimate list-item indents. Co-authored-by: Cursor <cursoragent@cursor.com>
Include tests/wrap/mod.rs from tests/wrap.rs so paragraph, list, footnote, and link reference definition integration tests execute under make test. Repair stale fixture paths, rstest attributes, and expectations that no longer matched current wrapping behaviour. Co-authored-by: Cursor <cursoragent@cursor.com>
Reformat the step-definitions guidance string in the CLI wrap test so make check-fmt passes after the integration test suite was wired up. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. #291 Ensure that link reference definition blocks are not reflowed at all (each definition remains on its own line and is left untouched). ❌ This PR does not modify the handling of link reference definitions. The PR description states that link reference definition passthrough was already implemented in separate work (#292 / #294), and there are no code or test changes in this diff related to skipping reflow for link reference definitions. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check status of PR `#294`
gh pr view 294 --repo leynos/mdtablefix --json state,mergedAt,title,body
# Check status of PR `#292` issue
gh issue view 292 --repo leynos/mdtablefix --json state,title,body
# Check for link reference definition handling in current branch
rg -n "link.ref|link_ref|lrd|reference.def|ReferenceDefinition|LinkRef" --type rust
# Check the wrap module for any passthrough/skip logic
rg -n "passthrough|pass_through|skip|lrd\|link" --type rust -C3Length of output: 29424
No further work is required for this item. |
Pass a continuation flag into render_line so intentional leading whitespace on the first wrapped line is preserved while carry whitespace is still stripped from later lines. Tighten the list-item proptest to use prop_assume for input constraints and prop_assert for postconditions. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e978fb2df9
ℹ️ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/wrap_leading_spaces.rs`:
- Around line 203-206: The test uses prop_assume! to check postconditions
(output length, prefix match, multi-line) which improperly discards cases;
change those three calls to prop_assert! so the property fails when invariants
are violated. Specifically, replace prop_assume!(output.iter().all(|line|
line.len() <= 80)), prop_assume!(output[0].starts_with(&prefix)), and
prop_assume!(output.len() > 1) with prop_assert! equivalents in the test using
the variables output and prefix so failures are reported instead of being
filtered out.
In `@tests/wrap.rs`:
- Line 1: Remove the crate-level blanket attribute
`#![allow(unfulfilled_lint_expectations)]` and instead attach a narrowly scoped
attribute to the specific item that needs suppression, e.g. add
`#[expect(unfulfilled_lint_expectations, reason = "temporary: <explain why>")]`
directly above the test function or module that triggers the lint (replace
<explain why> with a short justification), ensuring no other items keep the
global allow.
In `@tests/wrap/links.rs`:
- Around line 57-58: Replace the escaped-line string literal in the test (the
long markdown link literal that currently uses a backslash to continue the line)
with a concat!() invocation that joins the two parts into one compile-time
string; locate the string in tests/wrap/links.rs (the link beginning with
"[`rust-multithreaded-logging-framework-for-python-design.md`](" and split it
into separate quoted segments passed to concat!), remove the trailing backslash,
and ensure the resulting concat!() call produces the exact same content and
formatting as the original literal.
🪄 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 Plus
Run ID: 163a124c-a173-4906-8d1f-1b3144726069
⛔ Files ignored due to path filters (1)
tests/snapshots/wrap_unit__inline_footnote_reference_wrap.snapis excluded by!**/*.snap
📒 Files selected for processing (27)
src/wrap/inline.rssrc/wrap/tests/inline_wrapping.rstests/data/issue_291_errors_expected.txttests/data/issue_291_errors_input.txttests/data/issue_291_execplan_expected.txttests/data/issue_291_execplan_input.txttests/data/issue_291_kani_list_expected.txttests/data/issue_291_kani_list_input.txttests/data/issue_291_nested_ordered_list_expected.txttests/data/issue_291_nested_ordered_list_input.txttests/data/issue_291_netsuke_list_expected.txttests/data/issue_291_netsuke_list_input.txttests/data/issue_291_ordered_list_expected.txttests/data/issue_291_ordered_list_input.txttests/wrap.rstests/wrap/cli.rstests/wrap/fence_behaviour.rstests/wrap/footnotes.rstests/wrap/hard_line_breaks.rstests/wrap/link_reference_definitions.rstests/wrap/links.rstests/wrap/lists.rstests/wrap/mod.rstests/wrap/paragraphs.rstests/wrap_leading_spaces.proptest-regressionstests/wrap_leading_spaces.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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of render_line in src/wrap/inline.rs
cat src/wrap/inline.rs
echo "---"
# Check current state of the proptest in tests/wrap_leading_spaces.rs
cat tests/wrap_leading_spaces.rsLength of output: 23902
Overall comment 1 —
Overall comment 2 / individual comment —
Regressions in wrapping behaviour will now cause hard test failures rather than being silently discarded. |
The wrap integration test crate no longer needs a crate-level unfulfilled_lint_expectations allow; per-item expects in common helpers already cover the shared test utilities. Co-authored-by: Cursor <cursoragent@cursor.com>
Use concat! for the long markdown link string in test_wrap_link_with_trailing_punctuation, matching project conventions and avoiding a backslash line continuation in the literal. Co-authored-by: Cursor <cursoragent@cursor.com>
Describe render_line and strip_leading_carry_whitespace in the wrapping pipeline post-processing section so continuation-line trimming behaviour is recorded alongside the other inline wrapping invariants. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 156-163: Reflow the paragraph in docs/developers-guide.md so no
lines exceed 80 columns: break the sentence describing render_line in
src/wrap/inline.rs and its strip_leading_carry_whitespace flag into multiple
wrapped lines, preserving meaning (mention that strip_leading_carry_whitespace
removes carry whitespace attached to wrapped continuation lines and is only set
after wrap_preserving_code has emitted at least one line), and ensure the note
about trailing space/hard-break is wrapped as well; after editing, run `make
fmt` to apply documentation formatting and fix table markup.
🪄 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 Plus
Run ID: 5e6fbfb0-5af0-4db4-b4c8-57f0661db0a5
📒 Files selected for processing (1)
docs/developers-guide.md
Summary
This branch fixes spurious leading spaces on wrapped continuation lines when
mdtablefix --wrapreflows Markdown paragraphs containing inline code spans.Link reference definition passthrough was already landed on
mainvia#292 / #294.
Closes #291.
Review walkthrough
src/wrap/inline.rsto see
render_linetrimming carry whitespace at the render boundary.tests/wrap/paragraphs.rsfor the ExecPlan regression case from the issue report.
src/wrap/tests/inline_wrapping.rsand the updated inline footnote snapshot for unit-level expectations.
Validation
make check-fmt: passedmake lint: passedmake test: passedcoderabbit review --agent: 0 findingsSummary by Sourcery
Fix leading-space bugs in wrapped Markdown output and extend test coverage for wrapping behaviour.
Bug Fixes:
Tests: