fix: sanitize peer-controlled display text#447
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughIntroduces a ChangesPeer-controlled name sanitization
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit fcd38d9 on Jul 13, 2026 1:11pm UTC. |
Pip adversarial review — changes requestedBlocking findings:
Suggestion:
Checks run/read:
No out-of-scope follow-up filed; the unsanitized surfaces above are part of this issue. |
8d84bc0 to
b61bdeb
Compare
|
Addressed Pip review findings in b61bdeb:
Current head checks are green and the PR is marked ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
whitenoise-macTests/PeerDisplayTextTests.swift (1)
21-236: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNew test struct isn't part of the single serialized suite.
This file introduces
struct PeerDisplayTextTestswithout@Suite(.serialized), and it isn't merged into the existingwhitenoise_macTestssuite. Per the repo's Swift Testing guideline, this can run in parallel with (and independently of) the serialized suite, and the runner's-only-testing:filter won't reliably target its functions either. This split also forces duplicated helpers (isolated,timelineMessage,groupSystemEvent) between this file andwhitenoise_macTests.swift.Consider folding these test functions into
whitenoise_macTests.swift's existing@Suite(.serialized) struct whitenoise_macTests, or adding@Suite(.serialized)here if a separate file is intentional.As per coding guidelines: "Unit tests in
whitenoise-macTests/should use Swift Testing in a single@Suite(.serialized)struct, and because the runner's per-test-only-testing:filter does not reliably match individual functions in that serialized suite, prefer running the whole suite."🤖 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 `@whitenoise-macTests/PeerDisplayTextTests.swift` around lines 21 - 236, The new PeerDisplayTextTests suite is outside the repo’s single serialized Swift Testing suite, which can break test ordering and filtering. Move these tests into the existing whitenoise_macTests `@Suite`(.serialized) struct, or add `@Suite`(.serialized) to PeerDisplayTextTests if it must remain separate, and consolidate shared helpers like isolated, timelineMessage, and groupSystemEvent to avoid duplication.Source: Coding guidelines
🧹 Nitpick comments (1)
whitenoise-mac/Models/MessengerModels.swift (1)
2075-2094: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double sanitization of
displayName.
initalready storesself.displayName = PeerDisplayText.sanitize(displayName), buttitlere-sanitizes the already-sanitized value (PeerDisplayText.sanitize(displayName)). Harmless (idempotent) but confusing — the stored property is already clean.♻️ Proposed simplification
var title: String { - PeerDisplayText.sanitize(displayName) ?? DisplayText.short(accountIdHex) + displayName ?? DisplayText.short(accountIdHex) }🤖 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 `@whitenoise-mac/Models/MessengerModels.swift` around lines 2075 - 2094, The title logic is redundantly sanitizing `displayName` even though `init(...)` already assigns a sanitized value to `self.displayName`. Update `title` in `MessengerModels` to use the stored `displayName` property directly, falling back to `DisplayText.short(accountIdHex)` when it is nil, and keep the sanitization only in the initializer.
🤖 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.
Outside diff comments:
In `@whitenoise-macTests/PeerDisplayTextTests.swift`:
- Around line 21-236: The new PeerDisplayTextTests suite is outside the repo’s
single serialized Swift Testing suite, which can break test ordering and
filtering. Move these tests into the existing whitenoise_macTests
`@Suite`(.serialized) struct, or add `@Suite`(.serialized) to PeerDisplayTextTests
if it must remain separate, and consolidate shared helpers like isolated,
timelineMessage, and groupSystemEvent to avoid duplication.
---
Nitpick comments:
In `@whitenoise-mac/Models/MessengerModels.swift`:
- Around line 2075-2094: The title logic is redundantly sanitizing `displayName`
even though `init(...)` already assigns a sanitized value to `self.displayName`.
Update `title` in `MessengerModels` to use the stored `displayName` property
directly, falling back to `DisplayText.short(accountIdHex)` when it is nil, and
keep the sanitization only in the initializer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b3cfc3b-71a3-44e3-a362-9d6903846020
📒 Files selected for processing (10)
whitenoise-mac/Core/MarmotMapping.swiftwhitenoise-mac/Core/PeerDisplayText.swiftwhitenoise-mac/Core/WorkspaceState+NewChat.swiftwhitenoise-mac/Core/WorkspaceState+Notifications.swiftwhitenoise-mac/Core/WorkspaceState.swiftwhitenoise-mac/Models/MessengerModels.swiftwhitenoise-mac/Views/GroupViews.swiftwhitenoise-macTests/PeerDisplayTextTests.swiftwhitenoise-macTests/PureValueTests.swiftwhitenoise-macTests/whitenoise_macTests.swift
2135be9 to
239cd24
Compare
04af3dd to
fcd38d9
Compare
2 participants
Fixes #387
Summary
PeerDisplayTextto strip Unicode format controls from peer-controlled display strings before rendering.Verification
git diff --check— passedbash -n scripts/ci/macos-sanity-checks.sh— passedgit grep -n -E '^(<<<<<<<|=======|>>>>>>>)' -- .— passedscripts/ci/macos-sanity-checks.sh— not runnable on this Linux host:xcodebuild: command not foundxcrun swift-format,xcodebuild test/build/analyze) — not runnable on this Linux host;xcodebuild,xcrun,swift,swift-format, andplutilare missingSensitive paths
none
Summary by CodeRabbit
New Features
Bug Fixes
Tests