Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjust wrap_preserving_code to keep trailing spaces when emitting the final line and streamline buffer handling by using std::mem::take. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUpdate internal wrapping and whitespace handling to preserve trailing spaces, merge code spans, and reuse allocations; extend tests and HTML/table processing to preserve per-line trailing whitespace across wrapping, table buffering, and reflow collection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Wrap as wrap_preserving_code
participant Helpers as merge/flush helpers
participant Lines as Vec<String>
Caller->>Wrap: call(tokens, width)
rect rgba(230,240,255,0.9)
note right of Wrap: iterate tokens, build `current` buffer
alt token is "`" (code span)
Wrap->>Helpers: merge_code_span(tokens, i, width)
Helpers-->>Wrap: new index, adjusted width
Wrap->>Wrap: append whole code-span (keep punctuation glued)
else token normal
Wrap->>Wrap: append token to current
end
alt overflow or explicit flush
Wrap->>Helpers: flush_trailing_whitespace / flush_current
Helpers->>Lines: push(mem::take(&mut current) [+ optional whitespace token])
Helpers-->>Wrap: current (reused capacity)
end
end
Wrap-->>Caller: return lines (preserving trailing spaces)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)tests/table/convert_html.rs (1)
🔍 Remote MCP RefSummary of additional relevant facts for reviewing PR #211
Tools/sources used
🔇 Additional comments (12)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wrap.rs (1)
383-385: Add regression tests to lock in trailing‑space behaviour.Cover:
- Final flush preserves trailing spaces.
- No‑split overflow flush does not trim.
Sample tests using
rstest:use super::wrap_preserving_code; use rstest::rstest; #[rstest] #[case("trail ", 80, &["trail "])] #[case("`code span` ", 12, &["`code span` "])] fn preserves_trailing_spaces(#[case] input: &str, #[case] width: usize, #[case] expected: &[&str]) { let out = wrap_preserving_code(input, width); assert_eq!(out, expected.iter().map(|s| s.to_string()).collect::<Vec<_>>()); } #[rstest] #[case("aaaaaaaaaaaa", 5, &["aaaaaaaaaaaa"])] // forced flush without split fn no_split_forced_flush_no_trim(#[case] input: &str, #[case] width: usize, #[case] expected: &[&str]) { let out = wrap_preserving_code(input, width); assert_eq!(out, expected.iter().map(|s| s.to_string()).collect::<Vec<_>>()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/wrap.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/wrap.rs
🔍 Remote MCP Ref
Additional context relevant to reviewing PR #211
-
PR metadata — "Preserve trailing spaces on final flush" (PR #211) by leynos; branch codex/fix-inconsistency-in-wrap_preserving_code → main; closes issue #66; testing commands:
make fmt,make lint,make test. -
Code change (high level) — src/wrap.rs: when flushing due to width the code now pushes the whole current segment by using
std::mem::take(&mut current)instead of pushing a trimmed/cloned string and callingcurrent.clear(). This preserves trailing whitespace on mid-wrap flushes; the final flush likewise stops usingtrim_end()and pushescurrentas-is. -
Motivating issue — fixes inconsistency reported in issue #66 where the final flush used
trim_end()(losing trailing spaces) while the earlier loop did not. The PR aligns final-flush behavior with the loop. -
Related changes to review for overlap/regressions — related PRs/issues (#193, #62) also touch
wrap_preserving_codeand trailing-space handling; check for merge conflicts or duplicated fixes and verify the intended behavior is consistent across all code paths. -
Suggested review checklist (concrete items)
- Confirm the exact hunk in src/wrap.rs (ownership move via
mem::take) matches the description and contains no accidental logic changes beyond whitespace handling. - Verify unit tests exist (or add) that assert preservation of trailing spaces for both mid-wrap flush and final flush; run
make test/ CI. - Run
make fmtandmake lintlocally or confirm CI passes to ensure style/lint rules are satisfied. - Review for unintended downstream effects where callers rely on trimmed output (search for other uses of
wrap_preserving_codeor expectations about trailing-space removal).
- Confirm the exact hunk in src/wrap.rs (ownership move via
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
src/wrap.rs (3)
159-161: Stop trimming on no‑split flush: correct and aligned with the objective.Pushing the moved
currentviastd::mem::takepreserves trailing whitespace and avoids clone+clear. LGTM.
176-179: Final flush preserves trailing spaces.This matches the requirement to keep Markdown hard breaks intact.
149-154: Preserve trailing spaces on the other forced‑flush or document the exception.Preserve trailing spaces in this branch to match the other forced‑flush path, or add a short comment explaining why trim_end() is intentional.
File: src/wrap.rs — lines 149–154
- if current_width > width { - lines.push(current.trim_end().to_string()); - current.clear(); - current_width = 0; - last_split = None; - } + if current_width > width { + let cap = current.capacity(); + lines.push(std::mem::take(&mut current)); + current = String::with_capacity(cap); + current_width = 0; + last_split = None; + }
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(2 hunks)src/wrap/tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/wrap/tests.rssrc/wrap.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/wrap/tests.rssrc/wrap.rs
🧬 Code graph analysis (2)
src/wrap/tests.rs (1)
src/wrap.rs (1)
wrap_preserving_code(88-195)
src/wrap.rs (1)
src/html.rs (1)
lines(181-184)
🔍 Remote MCP Ref
Summary of additional relevant facts for reviewing PR #211 (preserve trailing spaces)
-
File changes (branch codex/fix-inconsistency-in-wrap_preserving_code):
- src/wrap.rs: adds a private helper flush_trailing_whitespace(lines, current, token); replaces mid-wrap/current final flush logic to use std::mem::take(&mut current) and reinitialize current with previous capacity instead of current.clear(); final flush now pushes current as-is (no trim_end), preserving trailing spaces.
-
Tests:
- src/wrap/tests.rs: imports wrap_preserving_code directly; renames/adjusts preserves_trailing_spaces test cases to assert preserved trailing spaces (e.g., "trail " and "
code span"); adds new test no_split_forced_flush_no_trim asserting forced flush at small width does not split and preserves trailing spaces for input "aaaaaaaaaaaa".
- src/wrap/tests.rs: imports wrap_preserving_code directly; renames/adjusts preserves_trailing_spaces test cases to assert preserved trailing spaces (e.g., "trail " and "
-
Relation to main branch:
- Compared to main/src/wrap.rs behavior, the PR branch changes the final-flush behavior to match in-loop behavior (no trimming). Confirmed code targeted the same function reported in issue #66.
Review checklist items supported by these findings
- Confirm mem::take usage only moves current and does not change tokenization/width logic (verify no other logic branches altered). [source: src/wrap.rs change above]
- Ensure added tests cover both mid-wrap and final-flush scenarios (tests file shows direct cases and the new forced-flush case). Run CI/tests to validate. [source: src/wrap/tests.rs]
- Search repo for other callers that expect trimmed output; if any exist, they may require adjustment (no callers were changed in this PR). Suggested quick grep in repo for wrap_preserving_code usages before merging.
Sources:
- Updated wrap.rs (PR branch):
- Tests (PR branch):
- Current main wrap.rs for comparison:
🔇 Additional comments (2)
src/wrap/tests.rs (1)
9-9: Import the SUT explicitlyKeep the test surface tight by importing
wrap_preserving_codedirectly. LGTM.src/wrap.rs (1)
160-169: Approve changes — preserve trailing whitespace on final whitespace-only token; verify downstream trimming behaviourTests (src/wrap/tests.rs) and docs (docs/trailing-spaces.md) assert the new behaviour; wrap_preserving_code is used in src/wrap.rs. Confirm no other code assumes trimmed trailing spaces (especially HTML output).
Run the script below from the repository root and paste the output.
#!/bin/bash set -euo pipefail echo "Repo root: $(git rev-parse --show-toplevel 2>/dev/null || pwd)" echo echo "Call sites of wrap_preserving_code:" rg -nP --hidden --no-ignore -g '!target/**' -C2 '\bwrap_preserving_code\s*\(' || git ls-files | xargs -r rg -nP -C2 '\bwrap_preserving_code\s*\(' || echo "No wrap_preserving_code matches found." echo echo "Search for trim usages (.trim, .trim_end, trim_end_matches):" rg -nP --hidden --no-ignore -g '!target/**' -n -C2 '\.trim(_end(_matches)?)?\s*\(' || git ls-files | xargs -r rg -nP -C2 '\.trim(_end(_matches)?)?\s*\(' || echo "No .trim* matches found." echo echo "Show src/html.rs if present (inspect trimming for HTML output):" if [ -f src/html.rs ]; then nl -ba src/html.rs | sed -n '1,240p' else git ls-files | rg 'html\.rs' || echo "No src/html.rs found." fi
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wrap.rs (1)
112-221: Fix trailing-space inconsistency; ensure preserved trailing spaces survive HTML rendering
- Document that mid-wrap lines are intentionally trimmed in src/wrap.rs (lines 165–168 and 180–181).
- Prevent the HTML renderer from discarding preserved trailing spaces: remove or conditionalise the
.map(|l| l.trim_end())in src/html.rs (around line 183), or explicitly convert trailing spaces to<br>at render time.- Audit other
trim_end()sites that could drop authorial hard breaks: src/reflow.rs:29, src/process.rs:91–116, src/lists.rs:129, src/breaks.rs:55, src/footnotes.rs:102.
♻️ Duplicate comments (1)
src/wrap.rs (1)
103-110: Whitespace‑only trailing token flush: LGTM (precondition enforced)The debug_assert and delegation to flush_current are spot on.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(3 hunks)src/wrap/tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/wrap.rssrc/wrap/tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/wrap.rssrc/wrap/tests.rs
🧬 Code graph analysis (2)
src/wrap.rs (1)
src/html.rs (1)
lines(181-184)
src/wrap/tests.rs (1)
src/wrap.rs (1)
wrap_preserving_code(112-221)
🔍 Remote MCP Ref
Summary of additional relevant facts for reviewing PR #211 (Preserve trailing spaces on final flush)
-
Core change: wrap_preserving_code no longer trims trailing spaces on final flush; it now pushes current as-is and preserves trailing whitespace behavior that the loop already maintained. This is implemented by replacing the final trim+push with a direct push and by introducing helpers (merge_code_span, flush_current, flush_trailing_whitespace) and mem::take usage to reuse allocations.,
-
Tests added/updated:
- preserves_trailing_spaces: expanded cases to include inputs with trailing spaces and code spans (e.g., "trail ", "
code span") verifying preservation at widths that won’t force splits. - no_split_forced_flush_no_trim: new test asserting forced flush at small width does not split lines and does not trim trailing spaces for inputs like "aaaaaaaaaaaa" (w=5), "abcde" (w=3), "
codespan" (w=6). These exercise both mid-wrap and final-flush behavior.,
- preserves_trailing_spaces: expanded cases to include inputs with trailing spaces and code spans (e.g., "trail ", "
-
Memory/allocation change: use of std::mem::take(&mut current) in flush_current to reuse allocated capacity instead of trimming/clearing, reducing allocation churn while preserving contents pushed to lines. Verify this does not accidentally expose moved-from buffers.
-
No public API changes: all changes are internal/private. No exported signatures altered.
-
Related/closing references:
Notes about retrieval attempts
- Attempted to read PR page and search repository for related code/details; the retrieval tools returned rate-limit/access errors during the attempt. Useful context above was drawn from the provided PR context and previous research summary; the tool calls attempted are listed here for traceability:,
🔇 Additional comments (4)
src/wrap.rs (1)
96-101: Capacity‑preserving flush: LGTMReuse of allocation via mem::take + with_capacity is correct and addresses prior capacity thrash.
src/wrap/tests.rs (3)
117-127: Strengthen final‑flush coverage: LGTMCases exercise wide and narrow widths and confirm trailing spaces survive.
129-142: Over‑width token behaviour: LGTMAsserts no splitting or trimming for long tokens and code spans. Good guard against regressions.
9-9: Avoid wildcard import in testsImport only what’s used to keep lints quiet.
-use super::super::*; +use super::wrap_text; use super::wrap_preserving_code;Likely an incorrect or invalid review comment.
1 participant
Summary
Testing
make fmtmake lintmake testcloses #66
https://chatgpt.com/codex/tasks/task_e_68c2b687e2488322a91ef6102f515f31
Summary by Sourcery
Bug Fixes: