fix: accept interoperable QR URL hosts#2116
Open
RCGV1 wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughContact and channel URL handling now uses structured host, path, and fragment validation. Contact routing supports canonical and ChangesURL handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant EntryPoint
participant ContactURLHandler
participant AccessoryManager
EntryPoint->>ContactURLHandler: Validate contact URL
ContactURLHandler-->>EntryPoint: Return supported URL result
EntryPoint->>ContactURLHandler: Handle contact URL
ContactURLHandler->>AccessoryManager: Import contact
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@Meshtastic/Views/Messages/MessageText.swift`:
- Around line 197-200: Update docs/user/messages.md to document that
message-text contact links handled by ContactURLHandler, including
www.meshtastic.org links, are intercepted instead of opening in the default
browser. Keep the documentation aligned with the behavior in MessageText.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19a68736-0b65-4ed9-a2a3-6c726d16e6fc
📒 Files selected for processing (7)
Meshtastic/AppIntents/AddContactIntent.swiftMeshtastic/Helpers/ContactURLHandler.swiftMeshtastic/Helpers/MeshtasticChannelURL.swiftMeshtastic/MeshtasticApp.swiftMeshtastic/Views/Messages/MessageText.swiftMeshtasticTests/EntityTests.swiftMeshtasticTests/MeshtasticChannelURLTests.swift
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/user/messages.md`:
- Line 148: Regenerate the bundled documentation for the updated
docs/user/messages.md content by running the repository’s build-docs.sh command
with Meshtastic/Resources/docs as output, then run copy-snapshots.sh with
Meshtastic/Resources/docs/assets/screenshots and include all generated changes.
- Line 148: Update the message-bubble link documentation to replace both
references to Safari with “the system browser,” reflecting
MessageText.handleURL’s .systemAction behavior. Keep the exceptions for
Meshtastic channel and contact links opening the in-app import flow, and retain
Meshtastic documentation links as system-browser links.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 248eba44-2758-4c62-9ecb-4ab3aa9eb231
📒 Files selected for processing (2)
MeshtasticTests/EntityTests.swiftdocs/user/messages.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MeshtasticTests/MeshtasticChannelURLTests.swift (1)
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeverage parameterized testing.
Instead of using a
forloop inside the test function, you can utilize Swift Testing's built-in parameterized testing feature. This approach is more idiomatic and provides individual, granular test results for each channel count in the test runner.♻️ Proposed refactor
- `@Test` func allSupportedChannelCountsPreserveSettingsForReplaceAndAdd() throws { - for channelCount in 1 ... 8 { - let channelSet = makeChannelSet(channelCount: channelCount) - let replace = try MeshtasticChannelURL.parse(MeshtasticChannelURL.urlString(for: channelSet)) - let add = try MeshtasticChannelURL.parse(MeshtasticChannelURL.urlString(for: channelSet, addChannels: true)) - - `#expect`(replace.channelSet.settings == channelSet.settings, "\(channelCount)-channel replace settings") - `#expect`(replace.channelSet.loraConfig == channelSet.loraConfig, "\(channelCount)-channel replace LoRa config") - `#expect`(add.channelSet.settings == channelSet.settings, "\(channelCount)-channel add settings") - `#expect`(!add.channelSet.hasLoraConfig, "\(channelCount)-channel add must not retune") - } - } + `@Test`(arguments: 1 ... 8) + func allSupportedChannelCountsPreserveSettingsForReplaceAndAdd(channelCount: Int) throws { + let channelSet = makeChannelSet(channelCount: channelCount) + let replace = try MeshtasticChannelURL.parse(MeshtasticChannelURL.urlString(for: channelSet)) + let add = try MeshtasticChannelURL.parse(MeshtasticChannelURL.urlString(for: channelSet, addChannels: true)) + + `#expect`(replace.channelSet.settings == channelSet.settings, "\(channelCount)-channel replace settings") + `#expect`(replace.channelSet.loraConfig == channelSet.loraConfig, "\(channelCount)-channel replace LoRa config") + `#expect`(add.channelSet.settings == channelSet.settings, "\(channelCount)-channel add settings") + `#expect`(!add.channelSet.hasLoraConfig, "\(channelCount)-channel add must not retune") + }🤖 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 `@MeshtasticTests/MeshtasticChannelURLTests.swift` around lines 41 - 52, Refactor allSupportedChannelCountsPreserveSettingsForReplaceAndAdd to use Swift Testing parameterized testing with channel counts 1 through 8 instead of looping inside the test. Keep the existing replace/add parsing and expectations unchanged so each channel count produces an individual test result.
🤖 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.
Nitpick comments:
In `@MeshtasticTests/MeshtasticChannelURLTests.swift`:
- Around line 41-52: Refactor
allSupportedChannelCountsPreserveSettingsForReplaceAndAdd to use Swift Testing
parameterized testing with channel counts 1 through 8 instead of looping inside
the test. Keep the existing replace/add parsing and expectations unchanged so
each channel count produces an individual test result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 699ee49b-0b08-4304-80d6-1fa415c52da1
📒 Files selected for processing (5)
Meshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/messages.mdMeshtastic/Resources/docs/user/messages.htmlMeshtasticTests/MeshtasticChannelURLTests.swiftdocs/user/messages.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/user/messages.md
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
Root cause
Android accepts both meshtastic.org and www.meshtastic.org for channel and contact links. Apple only accepted the bare host for channels and routed contacts with a literal meshtastic.org/v/# substring, so valid links using the alternate host were not imported.
Evidence
xcodebuild test -quiet -project Meshtastic.xcodeproj -scheme Meshtastic -only-testing:MeshtasticTests/MeshtasticChannelURLTests -only-testing:MeshtasticTests/ShareContactQRTests
No UI layout changes: this is URL routing and protocol regression coverage.
Summary by CodeRabbit
www.meshtastic.org), and tightened in-app importing to the supported URL formats only.