Repository navigation
fix(mobile): parse pasted pairing links - #5596
DominicVonk wants to merge 7 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughPairing URL construction and parsing now normalize inputs that lack a scheme. The connection form parses host and pairing-code fields on blur and before submission, then builds the pairing URL from the parsed values. ChangesPairing input handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The pairing input normalization is mergeable after normal checks. Supported tokens are preserved, and blur and submission consistently use parsed fields. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A pasted hosted pairing link containing a schemeless IP backend can now select unencrypted HTTP where the previous submission path selected HTTPS. This can expose pairing and session credentials on untrusted networks. Explicitly specified schemes and the existing pairing checks remain intact. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description thoroughly explains the problem, implementation, verification steps, test results, and UI evidence. It does not provide the required scope-and-approval information or explain why this focused fix qualifies for an exemption. Resolution Add a Scope and approval section with a triaged issue or maintainer approval comment. If no prior issue or discussion exists, explain why this focused, obvious bug fix qualifies for the exemption. Organize the existing problem and change details under the template headings if needed.
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Effect service conventions review: one finding. The new ConnectionActivationStore implementations map their failures onto unrelated ConnectionPersistenceError operation labels (register-connection for activation writes, list-targets for listDisabled), so the structured operation context no longer identifies the failing operation. Everything else in the diff (namespace imports, Context.Service tag placement in platform/persistence.ts, the new setEnabled registry operation and atom command, and the test-only service instances in the harnesses) follows the conventions.
Posted via Macroscope — Effect Service Conventions
86f8ade to
f7d7de8
Compare
There was a problem hiding this comment.
One convention issue found: in apps/mobile/src/connection/storage.ts the new activation-specific ConnectionPersistenceError operation literals are swapped with the existing target/registration ones, so the structured operation attribute no longer identifies the operation that actually failed. The web store (apps/web/src/connection/storage.ts) maps these correctly and can be used as the reference.
Posted via Macroscope — Effect Service Conventions
f7d7de8 to
efe9f1a
Compare
There was a problem hiding this comment.
One convention finding on the new shared connectivity helper. Everything else (activation store definition, platform implementations, error operation literals, new registry command) follows the service conventions.
Posted via Macroscope — Effect Service Conventions
efe9f1a to
c0ef226
Compare
3d5c7e5 to
a7c44d9
Compare
ApprovabilityVerdict: Approved 055f151 Small, self-contained bug fix adding URL parsing for pasted pairing links on mobile. Changes include helper functions and comprehensive unit tests. The open review comments reference either unrelated files or scenarios covered by tests. You can customize Macroscope's approvability policy. Learn more. |
a7c44d9 to
397415a
Compare
ae9a81e to
8cf47b4
Compare
8cf47b4 to
d96ef7a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 59e70a4. Configure here.
Dismissing prior approval to re-evaluate 055f151
|
Note This comment is posted by Julius' dot Closing for missing UI verification. The parser tests are useful, but this also rewrites the visible Host and Pairing Code fields on blur and submit. Please add before/after screenshots and a short recording of pasting a complete link, leaving Host, and connecting with a dummy token, then request reconsideration. |
|
@juliusmarminge Added the requested UI verification to the PR description for the unchanged head commit
Verified in T3 Code Dev on an iOS 27 simulator. Pairing tests (14), mobile typecheck, and targeted lint pass. Only a dummy token appears in the evidence; no successful authentication is claimed. Android was not exercised. Could you reconsider reopening this PR now that the missing evidence is supplied? |
|
Reopened. Please resolve conflcits |
|
@juliusmarminge Merged current main and resolved the two conflicts in Rebuilt the matching iOS native client and replaced the PR’s UI evidence with fresh before/after screenshots and a 36-second recording for this merge commit. The recording shows native paste → leave Host → extracted token → Add Environment → expected invalid-dummy-credential error. All 14 pairing tests and mobile typecheck pass. Targeted lint has zero errors and the same four warnings as main. Android was not exercised. |

Mobile treated the Host and Pairing Code fields independently, so pasting a complete pairing URL could leave the token embedded in the host field. Schemeless local URLs also behaved differently from desktop.
This change gives mobile the same paste behavior as desktop:
Related independent PRs
These PRs all target
mainand can be merged in any order:Verification
vp test run apps/mobile/src/features/connection/pairing.test.tsvp run --filter @t3tools/mobile typecheckvp lintfor the three changed filesfallow audit --base upstream/mainUI verification after merging main (2026-10-01)
Verified merge commit
f92dc1762496a852aec21dcee03098a1b9263d04againstmainat5cc99e1c23980d7995a13c47f969b47cb68ed1be. The conflicts were resolved by keeping main’s current form components, connection cleanup/development automation, and Effect error API, then applying this PR’s parsing on blur and submit. The diff against main still contains only the original three files.Captured in T3 Code Dev on an iOS 27 simulator with a matching rebuilt native client and a disposable local backend. Before uses main’s three affected files; after and recording use the pushed merge commit’s files.
http://127.0.0.1:15596/#token=DUMMYTOKEN01into Host using the native Paste menu.http://127.0.0.1:15596and Pairing Code becomesDUMMYTOKEN01.36-second recording: paste → leave Host → Add Environment → dummy-token rejection.
paste-blur-connect.mp4
Result screenshot · GitHub-hosted evidence
Re-ran the focused pairing tests (14 passed), mobile typecheck (passed), and targeted lint (zero errors; the same four existing warnings also reproduced on main). The native-client ensure/build passed. No evidence files were committed. Android and successful authentication were not exercised; the requested connection attempt deliberately uses an invalid dummy token.
Merge resolution, UI verification, and evidence update: GPT-6.1-Sol via the Codex harness in T3 Code.
Built and reviewed with GPT-5 Codex via Codex CLI.
Fork validation PR
Note
Low Risk
Connection-form parsing and URL normalization only; behavior is covered by unit tests with no auth or data-model changes.
Overview
Aligns mobile Add Environment pairing input with desktop by parsing full pairing URLs pasted into the Host field instead of treating host and code independently.
pairing.tsaddsnormalizePairingUrlInput(schemeless IPs → HTTP, hostnames → HTTPS,//→ HTTPS) andparsePairingFields, which splits a pasted URL’s token from the host when present.buildPairingUrlandparsePairingUrluse that normalization for direct, schemeless, protocol-relative, and hosted pairing links.ConnectionsNewRouteScreenruns field normalization on host blur and before connect so submit builds the URL from cleaned host/code values.Tests in
pairing.test.tscover the new parsing and URL-building cases.Reviewed by Cursor Bugbot for commit 9bdff53. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix pairing link parsing to extract embedded tokens from the host field on mobile
parsePairingFieldsin pairing.ts to detect when a full pairing URL is pasted into the host field and extract the token into the code field.normalizePairingUrlInputto infer schemes for schemeless and protocol-relative inputs (HTTPS for hostnames, HTTP for IPs).parsePairingUrlandbuildPairingUrlto use the new normalization, so protocol-relative and schemeless hosts are handled correctly.parsePairingFieldson blur and at submit so the host and code fields are normalized before the pairing URL is built.Macroscope summarized 9bdff53.