Repository navigation
Refactor tokenize_inline into tokenizer; support adjacent links/images - #258
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Enhance segment_inline by recognizing and splitting tokens before embedded link syntax [link](url) and image syntax . This change improves tokenization of inline markdown-like links by treating them as separate tokens. Added corresponding tests in src/wrap/tests.rs to verify correct segmentation of strings containing embedded links and images. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Reviewer's Guidesegment_inline now uses a custom loop to detect and split before adjacent markdown links or images, and new unit tests verify correct tokenization of these cases. Class diagram for updated segment_inline tokenization logicclassDiagram
class segment_inline {
+text: &str
+tokens: Vec<String>
+scan_while(text, i, predicate)
+collect_range(text, start, end)
+Custom loop: stops at whitespace, '`', '['
+Detects '!' followed by '[' to split before images
}
Class diagram for new test cases in tests.rsclassDiagram
class Tests {
+case("word[link](url)", &["word", "[link](url)"])
+case("word[link](url)[another](url2)", &["word", "[link](url)", "[another](url2)"])
+case("word", &["word", ""])
}
Flow diagram for new tokenization decision in segment_inlineflowchart TD
A["Start tokenization loop"] --> B["Check current character"]
B --> C{"Is whitespace, '`', or '['?"}
C -- Yes --> D["Break and collect token"]
C -- No --> E{"Is current '!' and next '['?"}
E -- Yes --> D
E -- No --> F["Advance to next character"]
F --> B
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughRefactors the tokenisation logic in Changes
Sequence DiagramsequenceDiagram
participant seg as segment_inline()
participant parse as parsing module
participant scan as scanning module
seg->>scan: scan_while(char predicate)
scan-->>seg: (idx, slice)
seg->>scan: has_odd_backslash_escape_bytes(pos)
scan-->>seg: bool (is_escaped)
alt Link/Image detected
seg->>parse: looks_like_image_start(idx, '!')
parse-->>seg: bool
seg->>parse: parse_link_or_image(text, idx)
parse->>scan: has_odd_backslash_escape_bytes(depth tracking)
scan-->>parse: bool
parse-->>seg: (link_text, end_idx)
else Regular text
seg->>scan: collect_range(start, end)
scan-->>seg: token_string
end
seg->>seg: accumulate token
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/wrap/tokenize.rs:247` </location>
<code_context>
} else {
let start = i;
- i = scan_while(text, i, |ch| !ch.is_whitespace() && ch != '`');
+ while i < text.len() {
+ let Some(current) = text[i..].chars().next() else {
+ break;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests to cover the new tokenization logic for '[', '!', and '![' cases.
The new logic in segment_inline introduces handling for '[', '!', and '![' tokens. You must add both behavioural and unit tests to src/wrap/tests.rs to verify this behaviour and ensure correctness.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
![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/tokenize.rs (1)
233-235: Extract the image detection logic to eliminate duplication.The condition for detecting
![sequences appears at both lines 233-235 and lines 254-258. Extract this into a helper predicate function to comply with DRY principles.Apply this refactor:
+fn looks_like_image_start(text: &str, idx: usize, ch: char) -> bool { + let after_bang = idx + ch.len_utf8(); + ch == '!' && after_bang <= text.len() && text[after_bang..].starts_with('[') +} + pub(super) fn segment_inline(text: &str) -> Vec<String> { let mut tokens = Vec::new(); let bytes = text.as_bytes(); @@ -230,9 +235,7 @@ continue; } - let after_bang = i + ch.len_utf8(); - let looks_like_image = - ch == '!' && after_bang <= text.len() && text[after_bang..].starts_with('['); + let looks_like_image = looks_like_image_start(text, i, ch); if ch == '[' || looks_like_image { let (tok, mut new_i) = parse_link_or_image(text, i); tokens.push(tok); @@ -252,9 +255,7 @@ break; } if current == '!' { - let after_bang = i + current.len_utf8(); - if after_bang <= text.len() && text[after_bang..].starts_with('[') { - break; + if looks_like_image_start(text, i, current) { + break; } }Also applies to: 254-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap/tests.rs(1 hunks)src/wrap/tokenize.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 / -yse / -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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/wrap/tests.rssrc/wrap/tokenize.rs
🧬 Code graph analysis (1)
src/wrap/tests.rs (1)
src/wrap/tokenize.rs (1)
segment_inline(203-266)
🔍 Remote MCP Deepwiki
Perfect! I now have enough context to provide a comprehensive summary for the PR review. Let me compile the relevant information.
Summary of Additional Context for PR Review
Purpose and Scope
The segment_inline function breaks a single line of Markdown text into a vector of strings, preserving inline code spans, links, images, and surrounding whitespace as separate tokens. This PR addresses a tokenization bug where adjacent links and images merge together instead of being preserved as separate tokens.
Related Issue and Problem Context
Issue #175 addresses how adjacent links and images are tokenized, specifically concerning trailing punctuation and their grouping during text wrapping. The current implementation fails to properly separate adjacent link/image constructs like "word[link](url)[another](url2)" or "word".
How the Fix Works
The fix modifies the non-whitespace token-scanning loop in segment_inline to add three stopping conditions:
- Stop at whitespace (existing)
- Stop at backticks
`(existing) - Stop at
[character (new) - Detect
"→["word", "[link](url)"] - Multiple adjacent links:
"word[link](url)[another](url2)"→["word", "[link](url)", "[another](url2)"] - Adjacent image:
"word"→["word", ""]
These cases directly address the tokenization bug by validating that the ![ detection prevents images from merging with preceding content.
⏰ 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 (1)
src/wrap/tests.rs (1)
39-50: Excellent test coverage for the adjacent link/image tokenization fix.The parameterized test cases comprehensively verify the tokenizer's ability to split at
[and![boundaries, directly addressing the bug described in the PR objectives. The use of rstest is appropriate and follows the coding guidelines.
- Introduced `looks_like_image_start` helper to detect Markdown images - Updated `segment_inline` to respect escaped link/image literals, avoiding parsing them - Added tests to ensure escaped links and images are preserved as literals This prevents misinterpreting escaped link/image syntax, improving tokenizer correctness. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
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/tokenize.rs (1)
1-573: File exceeds 400-line limit mandated by coding guidelines.This file contains 573 lines, significantly exceeding the 400-line maximum. Decompose the module by extracting related functions into separate submodules (e.g.,
tokenize/scanning.rsforscan_whileand byte-level utilities,tokenize/parsing.rsforparse_link_or_imageand fence handling).As per coding guidelines.
♻️ Duplicate comments (1)
src/wrap/tests.rs (1)
52-59: Test failures stem from implementation bug in tokenize.rs.Cases 3 and 4 fail because the tokeniser incorrectly splits
\sequences. The backslash escapes the exclamation mark, making\!a literal, but the subsequent[is then treated as a link start because there's no backslash immediately before it.The test logic is correct and will pass once the tokenisation bug is addressed. Refer to the review comment on src/wrap/tokenize.rs lines 249-251 for the fix.
Based on pipeline failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap/tests.rs(1 hunks)src/wrap/tokenize.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 / -yse / -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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/wrap/tokenize.rssrc/wrap/tests.rs
🧬 Code graph analysis (1)
src/wrap/tests.rs (1)
src/wrap/tokenize.rs (1)
segment_inline(219-282)
🪛 GitHub Actions: CI
src/wrap/tests.rs
[error] 58-58: Test failed: wrap::tests::segment_inline_preserves_escaped_link_literals::case_3 assertion left != right. Command 'cargo llvm-cov --workspace --summary-only --lcov --output-path lcov.info' exited with code 1.
[error] 58-58: Test failed: wrap::tests::segment_inline_preserves_escaped_link_literals::case_4 assertion left != right. Command 'cargo llvm-cov --workspace --summary-only --lcov --output-path lcov.info' exited with code 1.
⏰ 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 (2)
src/wrap/tokenize.rs (1)
140-154: Clean helper extraction for image-start detection.The
looks_like_image_startfunction correctly identifies the![pattern whilst remaining agnostic about escape sequences. The multibyte-aware offset calculation and clear documentation align with best practices.src/wrap/tests.rs (1)
39-50: Excellent test coverage for adjacent link splitting.The
segment_inline_splits_before_embedded_linkstest properly validates that consecutive links and images are split into separate tokens. The parameterised cases cover the key scenarios mentioned in the PR objectives.
Add `bracket_follows_escaped_bang` helper to detect when a `[` follows an escaped `!` (i.e. "\!["). Modify parsing logic to treat such brackets as literal text rather than image token start, fixing incorrect tokenization of escaped image syntax. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…bmodules The main tokenize.rs file was split into mod.rs (public API) and two new modules: parsing.rs (higher-level inline Markdown parsing helpers) and scanning.rs (byte-level scanning utilities). This separation improves code organization by isolating parsing logic and low-level scanning routines from the public tokenizer interface. Additionally, several functions previously defined directly in tokenize.rs were moved into these new submodules, reducing file size and making the codebase more modular and maintainable. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
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 (3)
src/wrap/tokenize/mod.rs(3 hunks)src/wrap/tokenize/parsing.rs(1 hunks)src/wrap/tokenize/scanning.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 / -yse / -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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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...
Files:
src/wrap/tokenize/scanning.rssrc/wrap/tokenize/parsing.rssrc/wrap/tokenize/mod.rs
🧬 Code graph analysis (2)
src/wrap/tokenize/parsing.rs (1)
src/wrap/tokenize/scanning.rs (3)
collect_range(42-44)has_odd_backslash_escape_bytes(52-63)scan_while(20-32)
src/wrap/tokenize/mod.rs (2)
src/wrap/tokenize/parsing.rs (4)
handle_backtick_fence(103-123)is_trailing_punctuation(96-101)looks_like_image_start(76-82)parse_link_or_image(23-66)src/wrap/tokenize/scanning.rs (4)
bracket_follows_escaped_bang(69-74)collect_range(42-44)has_odd_backslash_escape_bytes(52-63)scan_while(20-32)
🔍 Remote MCP Deepwiki
Relevant facts discovered (concise, review-focused)
-
New tokenizer split: segment_inline logic was moved into two new modules
tokenize/parsing.rsandtokenize/scanning.rsandsegment_inlinenow delegates link/image parsing toparse_link_or_imageand scanning/escaping to helpers likehas_odd_backslash_escape_bytes,bracket_follows_escaped_bang,collect_range,scan_while. (See repo docs on tokenization and wrap module organization.) -
Tests added exercise exactly the intended behavior:
- Cases expecting a split before an embedded link/image: e.g. "wordlink" → ["word", "link"]; multi-adjacent links/images also asserted.
- Cases ensuring escaped bracket/bang are preserved as single token: e.g. r"[link](url)" and r"!img". These tests are placed alongside other wrap tests and target
segment_inline/inline segmentation behavior.
-
Tokenization and wrapping are central to the pipeline:
- The wrapping pipeline uses tokenization (tokenize_markdown, Token enum) and a FenceTracker to avoid modifying fenced code;
process_stream_innerruns fence normalization / HTML conversion → core loop (table buffering + reflow) → postprocessing (headings/code-emphasis/wrap/ellipsis/footnotes). Any changes to inline segmentation can affect determine_token_span, wrap_text, and downstream postprocessing.
- The wrapping pipeline uses tokenization (tokenize_markdown, Token enum) and a FenceTracker to avoid modifying fenced code;
-
Key integration points to check when reviewing the PR:
- Fence handling: ensure new parsing/scanning helpers respect FenceTracker semantics so code fences remain unaffected. The pipeline flushes buffers on fence lines (handle_fence_line) and token-based transforms skip code tokens.
- determine_token_span / grouping rules: the wrapping logic groups related tokens (links, code spans, trailing punctuation); the new parse_link_or_image must preserve grouping widths and punctuation attachment behavior used by LineBuffer/width calculations.
- Unicode/multibyte: wrapping and table reflow use unicode-width for visual widths—segmentation must not split multibyte characters or alter width calculations.
- Escaping semantics: helpers like has_odd_backslash_escape_bytes and bracket_follows_escaped_bang must correctly detect escaped
\[and\!so escaped literals are preserved as tests expect.
-
Test & CI expectations:
- Run
cargo test(unit, integration, CLI tests) and ensure new and existing tests pass; CI enforces fmt/clippy/tests. The repo uses strict CI (format, markdown-lint, clippy as -D warnings, full tests + coverage).
- Run
Concrete review checklist (apply when reviewing code/behavior):
- Verify parse_link_or_image handles nested parentheses and returns correct slice + index for valid and malformed inputs (no off-by-one; tests for nested parentheses exist in parsing module).
- Confirm looks_like_image_start distinguishes escaped
!(i.e., not preceded by odd backslash) and that bracket_follows_escaped_bang correctly detects\→ single token (line 55) ✓\\→ splits correctly ✗ Missing\[link](url)→ single token (line 53) ✓- Multiple consecutive escaped sequences ✗ Missing
Add parametrised test cases for
\\and a multi-sequence case (e.g.\!\[two](url)or\[a]\[b]) tosegment_inline_preserves_escaped_link_literalsinsrc/wrap/tests.rs.
…s support Refactored the parse_link_or_image function into smaller helpers (parse_link_text, parse_link_url, fallback_single_char) to improve clarity and maintainability. Updated parsing logic to correctly handle nested parentheses in link URLs and added comprehensive tests for these cases. Also improved scanning.rs with additional test coverage and clarified bracket_follows_escaped_bang behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
#570 (#561) lets the wrapper break only at source whitespace, so an opener written directly before its label can no longer be split and `[a]` is a fixed point, whatever the label's characters. This records the fix. The corpus gains the issue's two reproductions as cases E2 and E3, and `tests/issue_507_bracket_label_wrap_is_idempotent.rs` runs them through the binary. A property grows prose to the wrap boundary in front of labels of letters and digits. ADR 0006 gains a 2026-09-27 addendum retiring the digit-only residue, and ADR 0010 points to it. The alphabetic shape dates from #258 (d427f7b), which split `[` from its label. The #505 tests generated only digit labels.
Summary
word[link](url)tokenize as [word,[link](url)] andwordtokenize as [word,].\\[link](url)and\\as single tokens.Changes
Tokenizer
[link](url)andas separate tokens.Parsing
Scanning
Tests
Test plan
📎 Task: https://www.terragonlabs.com/task/01f1c4bc-c82c-460f-9535-0ccc97d04906