Skip to content

Encode trailing space as _x20_ in XML ids to keep round trip - #3225

Open
Silvertree2010 wants to merge 1 commit into
storytold:mainfrom
Silvertree2010:fix/h842-xml-id-object-name-round-trip-dr
Open

Silvertree2010 wants to merge 1 commit into
storytold:mainfrom
Silvertree2010:fix/h842-xml-id-object-name-round-trip-dr

Conversation

@Silvertree2010

@Silvertree2010 Silvertree2010 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What and why

Found while testing crates/doc/src/xml_id.rs:80 (strip_unique), triggered by from_object_name at :32 (no existing issue): XML-ID object-name round trip drops text when a name ends with a space after 20+ digits.

Cause: from_object_name turned a trailing space into a bare _. For a name like "Scan 12345678901234567890 " the id ended in _<20+ digits>_, which strip_unique treats as a uniqueness suffix and removes. to_object_name then returned "Scan".

Fix: from_object_name now writes a trailing space as _x20_, so the id never ends in a bare _<digits>_. to_object_name already decodes _xHH_ escapes, so it needs no change.

How it was tested

  • The regression asserts were added to the existing round-trip test in xml_id::tests. They check that the id ends in _x20_ and that the name round-trips. Without the fix the first assertion fails.
  • Regression test xml_id::tests::round_trip_names_that_cannot_start_an_xml_name: fails on main without the fix and passes with it (cargo test -p vectorcraft-doc --lib xml_id).
  • cargo xtask ci passes.

Checklist

  • cargo xtask ci passes (fmt, clippy, tests, assets, brands, layers, wasm).
  • No panics in shipped code: no unwrap(), expect(), panic!, unreachable!, todo! or
    unimplemented! outside tests, and no unchecked indexing on data from files, parameters or
    messages. Errors are returned as Result and shown to the user
    (why and how).
  • Clean-room: nothing copied from Adobe products or GPL/AGPL code (AGENTS.md).
  • Every new asset has a row in ASSETS.md.

Not changed

strip_unique and to_object_name are unchanged, so ids like S_0000…_ still decode to "S". Interior spaces still encode as _.

🤖 Generated with AI by ApolloOffice, see the Co-Authored-By trailers for the models

Summary by CodeRabbit

  • Bug Fixes
    • Fixed conversion of names ending with a space so they are encoded consistently.
    • Prevented trailing spaces followed by 20 digits from being misinterpreted as uniqueness suffixes.
    • Improved round-trip handling so these names can be converted and restored without losing or altering their original content.

Found while testing crates/doc/src/xml_id.rs:80 (strip_unique), triggered by from_object_name at :32

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 47335810-5db1-4f41-ba85-f2b3faa3520d
📥 Commits

Reviewing files that changed from the base of the PR and between ad05669 and 8197a7e.

📒 Files selected for processing: 1
  • crates/doc/src/xml_id.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

from_object_name now encodes a final space as _x20_. A round-trip test checks a name that ends with a space after 20 digits.

Changes: XML ID encoding

Layer / File(s) Summary
Encode final spaces and verify round trips
crates/doc/src/xml_id.rs
from_object_name encodes a final space as _x20_. The test checks that the ID ends in _x20_ and decodes to the original name.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: messhias

Merge Risk: ⚪ Minimal · up to 8197a

The trailing-space encoding change is ready to merge with no identified actionable risk.

Architecture risk: 🟡 Medium · up to 8197a

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in crates/doc/src/xml_id.rs: from_object_name detects a final space and emits _x20_; other spaces continue to use bare underscores.
  • observed — Modified behavior in crates/doc/src/xml_id.rs: Added a round-trip test ensuring a name ending with a space after 20 digits produces an ID ending in _x20_ and decodes back unchanged.

Reliability and maintainability

  • inferred — Risk-relevant change factors for crates: blast_radius_1; direct_dependents_1
Pre-merge checks: Passed 5
Check name Status Explanation
Description check Passed The description includes the required What and why, How it was tested, and Checklist sections. It clearly explains the defect, cause, fix, regression coverage, and test commands. Two checklist items r…
Title check Passed The title clearly describes the main change: encoding a trailing space as x20 in XML IDs to preserve round trips. It is concise and specific.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes the required What and why, How it was tested, and Checklist sections. It clearly explains the defect, cause, fix, regression coverage, and test commands. Two checklist items remain unchecked, but the description is otherwise complete and directly related to the change.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the full diff at 8197a7e. A trailing space in an object name is now written as x20 instead of a bare _, so a name ending in digits can no longer read back as a uniqueness suffix; the round trip is tested with the 20-digit name. FreeBSD build + test green at this head.

@Messhias Messhias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encode trailing space as x20 in XML ids to keep round trip. Checklist cargo xtask ci is ticked.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants