Fix Contacts vCard escaping and group orphan cleanup#38
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 36 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated vCard handling: added ORG-specific parsing/escaping to preserve semicolon-delimited organization components and improved grouped-property stripping to remove orphaned group metadata when contact fields are updated. Tests added for semicolon and grouped-property edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bigcat88
force-pushed
the
feature/p3-contacts-fixes
branch
from
e1a48c1 to
8660832
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 96.09% 96.06% -0.04%
==========================================
Files 25 25
Lines 2282 2336 +54
==========================================
+ Hits 2193 2244 +51
- Misses 89 92 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 51-53: The helper _vcard_escape_org currently splits a
semicolon-joined string which loses information for already-escaped '\;'
sequences; fix by treating ORG as structured components end-to-end: stop passing
a pre-joined string into _vcard_escape_org (the code that flattens ORG before
calling it) and change _vcard_escape_org to accept an iterable/sequence of
components (or add a new helper like _vcard_escape_org_components) that calls
_vcard_escape on each component and then joins with ";" — alternatively, if you
must accept strings, implement a split that respects backslash-escaping (i.e.,
split on unescaped ';') so literal '\;' inside a component is preserved. Ensure
callers that previously joined the components are updated to pass the component
list or the properly split string to the new helper.
- Around line 321-352: The function _strip_updated_fields incorrectly treats
X-ABRELATEDNAMES and X-ABDATE as pure group-metadata (via
_GROUP_METADATA_FIELDS), causing user data to be dropped when a group's real
properties are stripped; update the logic by removing X-ABRELATEDNAMES and
X-ABDATE from _GROUP_METADATA_FIELDS (leaving only true metadata like X-ABLABEL)
so group_real only excludes genuine metadata and orphan_groups is computed
correctly, ensuring fields like X-ABRELATEDNAMES and X-ABDATE are preserved by
_strip_updated_fields.
In `@tests/integration/test_contacts.py`:
- Around line 736-742: The assertion checking for orphan X-ABLabel lines is
case-sensitive and can miss variants like "X-ABLABEL"; update the check in the
test that calls nc_mcp.client.dav_request (uses variable body) to perform a
case-insensitive search (e.g., normalize body to lowercase or use a
case-insensitive regex) and assert that "x-ablabel" (lowercased) is not present;
keep the same failure message but ensure it reflects the normalized comparison.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee85d45d-1ea0-49db-b7fb-3e00c21c32c2
📒 Files selected for processing (2)
src/nc_mcp_server/tools/contacts.pytests/integration/test_contacts.py
bigcat88
force-pushed
the
feature/p3-contacts-fixes
branch
from
8660832 to
f620ec1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 86-99: The ORG round-trip loses information because
_extract_raw_org only re-escapes semicolons and not backslashes, so components
that end with a literal backslash become ambiguous; fix by escaping backslashes
before escaping semicolons when reconstructing the flat value (i.e., for each
component in _extract_raw_org and the inverse in
_vcard_escape_org/_parse_org_components, replace "\" with "\\" first, then
replace ";" with "\;") so trailing backslashes are preserved and the original
component boundaries remain recoverable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8554e314-c10e-49e5-8ae1-bdfcd4978aee
📒 Files selected for processing (2)
src/nc_mcp_server/tools/contacts.pytests/integration/test_contacts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_contacts.py
bigcat88
force-pushed
the
feature/p3-contacts-fixes
branch
from
f620ec1 to
908da47
Compare
…orphaned group labels
bigcat88
force-pushed
the
feature/p3-contacts-fixes
branch
from
908da47 to
9ae5b0e
Compare
2 participants
Summary
Follow-up to #37 (Contacts tools). Fixes three issues found during review:
_vcard_escape_orgpreserves;as the RFC 2426 component separator instead of escaping it. EnsuresORG:Acme Inc;Engineering;Backendround-trips correctly through read→update→write._build_vcardnow sanitizes;infull_namebefore synthesizing the required N field, instead of silently skipping N generation (violating vCard 3.0)._strip_updated_fieldsnow removesX-ABLabel/X-ABRelatedNamesmetadata lines when all real properties in their group are stripped (e.g., replacing grouped EMAIL entries).Test plan
test_create_full_name_with_semicolon_has_n_field— N field present when full_name contains;test_update_multicomponent_org_roundtrip— multi-component ORG survives read→update→writetest_update_email_strips_orphan_group_labels— X-ABLabel removed with orphaned grouptest_update_email_preserves_tel_in_same_group— mixed group keeps non-stripped fieldstest_update_tel_preserves_email_in_same_group— mirror test for TEL updates🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests