fix: keep paragraphs joined across indented line comments - #17
Open
sou1118 wants to merge 1 commit into
Open
Conversation
Generalize the Space-node guard from #9: when the next Space starts with a newline and the paragraph buffer already ends in a newline plus optional indent, drop the trailing indent and the duplicate newline. This prevents whitespace-only lines in the output, which Typst treats as paragraph breaks. Closes #16
📝 WalkthroughWalkthroughThe parser now joins lines separated by dropped comments without adding false paragraph breaks. It removes trailing indentation and preserves breaks from genuine blank lines. Unit and integration tests cover regular, indented, and consecutive comments. ChangesParagraph reconstruction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 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 `@src/parse.rs`:
- Around line 79-91: Update the newline handling in the joined logic of the
paragraph parser to recognize every Typst newline form using
typst_syntax::is_newline, including treating CRLF as a single sequence before
removing the prefix. Preserve the existing paragraph joining behavior while
preventing retained indentation and duplicate newlines, and add regression
coverage for each supported newline form.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f03f05bb-7ef1-48a1-8efa-3230f286ae7f
📒 Files selected for processing (2)
src/parse.rstests/integration.rs
Comment on lines
+79
to
+91
| let joined = match text.strip_prefix('\n') { | ||
| Some(rest) => { | ||
| let line_start = paragraph_buf.trim_end_matches([' ', '\t']); | ||
| if line_start.ends_with('\n') { | ||
| paragraph_buf.truncate(line_start.len()); | ||
| paragraph_buf.push_str(rest); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| None => false, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(src/parse\.rs|Cargo\.toml|Cargo\.lock)$'
printf '%s\n' '--- parse.rs outline ---'
ast-grep outline src/parse.rs --lang rust
printf '%s\n' '--- relevant source ---'
sed -n '55,105p' src/parse.rs
sed -n '240,315p' src/parse.rs
printf '%s\n' '--- typst-syntax dependency declarations ---'
rg -n -C 3 'typst-syntax|typst_syntax' Cargo.toml Cargo.lockRepository: sou1118/typdiff
Length of output: 6812
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("src/parse.rs")
s = p.read_text()
for i, line in enumerate(s.splitlines(), 1):
if "strip_prefix" in line or "trim_end_matches" in line or "paragraph_buf" in line:
print(f"{i}: {line}")
PYRepository: sou1118/typdiff
Length of output: 1629
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- parser control flow ---'
sed -n '1,145p' src/parse.rs
printf '%s\n' '--- local dependency/cache candidates ---'
find "$HOME/.cargo" -type f \( -path '*typst-syntax*lexer.rs' -o -path '*typst-syntax*lib.rs' \) -print 2>/dev/null | head -20 || true
printf '%s\n' '--- typst-syntax 0.14.2 lexer source ---'
curl -LfsS https://raw.githubusercontent.com/typst/typst/v0.14.2/crates/typst-syntax/src/lexer.rs | sed -n '1,240p'Repository: sou1118/typdiff
Length of output: 14243
🏁 Script executed:
#!/bin/bash
set -eu
curl -LfsS https://raw.githubusercontent.com/typst/typst/v0.14.2/crates/typst-syntax/src/lexer.rs \
| rg -n -C 8 'newline|CRLF|\\r|Space|Parbreak'Repository: sou1118/typdiff
Length of output: 13084
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- public exports ---'
curl -LfsS https://raw.githubusercontent.com/typst/typst/v0.14.2/crates/typst-syntax/src/lib.rs \
| rg -n -C 4 'mod lexer|pub use .*is_newline|is_newline'
printf '%s\n' '--- paragraph flush behavior ---'
sed -n '145,170p' src/parse.rs
printf '%s\n' '--- direct reconstruction probe ---'
python3 - <<'PY'
cases = {
"LF": ("First.\n ", "\n"),
"CRLF": ("First.\r\n ", "\r\n"),
"CR": ("First.\r ", "\r"),
"LS": ("First.\u2028 ", "\u2028"),
}
for name, (buf, text) in cases.items():
joined = text.startswith("\n")
result = buf + text if not joined else buf.rstrip(" \t") + text[1:]
print(name, {"joined": joined, "result_repr": repr(result)})
PYRepository: sou1118/typdiff
Length of output: 1723
Handle all Typst newline forms.
When an indented comment occurs in a CRLF or other Typst newline sequence, strip_prefix('\n') fails. The paragraph then retains the comment indentation and duplicate newline. Use typst_syntax::is_newline, handle CRLF as one sequence, and add regression tests for supported newline forms.
🤖 Prompt for 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.
In `@src/parse.rs` around lines 79 - 91, Update the newline handling in the joined
logic of the paragraph parser to recognize every Typst newline form using
typst_syntax::is_newline, including treating CRLF as a single sequence before
removing the prefix. Preserve the existing paragraph joining behavior while
preventing retained indentation and duplicate newlines, and add regression
coverage for each supported newline form.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Follow-up to #9, closes #16.
An indented line comment between two sentences still split the paragraph in the diff output: the reconstructed source kept a whitespace-only line (the comment's indent), and Typst treats such lines as paragraph breaks.
Fix
Generalize the Space-node guard from #9. The old guard only skipped a
Spacewhose text was exactly"\n"when the buffer ended with'\n'. Now, when aSpacestarts with a newline and the buffer already ends in a newline plus optional spaces/tabs (the dropped comment's indent), the indent is truncated and the duplicate newline skipped, so no blank or whitespace-only line can appear.The invariant from #9 still holds: bare blank lines are always a single
Parbreaktoken, so two Space nodes spanning a newline can only arise from a dropped comment between them — intentional paragraph breaks are unaffected.Tests
cargo fmt --checkandclippy -D warningsare clean.Summary by CodeRabbit