feat(ios): tap-to-expand link previews in chat transcript#101198
Conversation
openclaw-barnacle
Bot
added
scripts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868f354ded
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .task(id: self.url) { | ||
| guard self.result == nil else { return } | ||
| self.result = await chatLinkPreviewStore.get(self.url) |
There was a problem hiding this comment.
When a message row keeps the same SwiftUI identity but its first URL changes after expansion, this guard leaves the old result in place because it only checks for nil. That can happen when cached or optimistic transcript rows are later replaced/reconciled with live history while preserving the message ID, so the card can show metadata or a failure for the previous URL while the domain and tap target are for the new URL. Key the state by URL or clear/ignore in-flight results when url changes.
Useful? React with 👍 / 👎.
|
|
||
| guard | ||
| let response, | ||
| let html = String(bytes: response.data, encoding: .utf8) |
There was a problem hiding this comment.
For any accepted text/html response served with a non-UTF-8 charset, such as text/html; charset=iso-8859-1 or Windows-1252, this hard-coded UTF-8 decode returns nil as soon as the page contains non-UTF-8 bytes, so a valid preview fails before parsing metadata. URLResponse exposes the server-provided text encoding (textEncodingName), so capture it with the response and decode using that charset with a UTF-8 fallback.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 6, 2026, 5:57 PM ET / 21:57 UTC. Summary PR surface: Other +999. Total +999 across 5 files. Reproducibility: yes. from source for the review findings: the PR head counts image-only metadata as loaded, keeps preview result state across URL changes, and decodes accepted HTML only as UTF-8. I did not run a native Apple UI reproduction in this read-only review. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land the Apple link-preview slice only after the parser/UI, charset, and URL-keyed state issues are fixed and a redacted iOS or macOS app run proves the tap-to-expand path without ambient fetches. Do we have a high-confidence way to reproduce the issue? Yes from source for the review findings: the PR head counts image-only metadata as loaded, keeps preview result state across URL changes, and decodes accepted HTML only as UTF-8. I did not run a native Apple UI reproduction in this read-only review. Is this the best way to solve the issue? No: the feature direction matches the merged Android sibling, but the current patch is not the best merge path until it requires visible metadata or renders images, resets state by URL, honors response charset, and includes real native proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e80e8a2b6730. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Other +999. Total +999 across 5 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Merged via squash.
|
1 participant
Related: #100699
What Problem This Solves
Bare URLs in the iOS/macOS chat transcript render as plain tappable links with no way to see what's behind them before leaving the app. Android gained tap-to-expand previews in #100898; this brings the same experience to the Apple clients.
Why This Change Was Made
Mirrors the Android product contract exactly: no ambient fetches, ever — a collapsed domain chip appears under messages containing a safe web link, metadata is fetched only on tap, and the expanded text card (title + description) opens the URL on tap like the link itself.
LinkPresentation/LPLinkMetadataProviderwas deliberately not used: it fetches ambiently and can't honor our policy. The customURLSessionfetcher enforces the same constraints as Android: http/https only; non-public hosts rejected (loopback, RFC1918, link-local, CGNAT, ULA, localhost/*.local); manual redirect approval per hop (max 3, scheme + host policy re-checked); ephemeral configuration with no cookies or credential storage; 6s total deadline; 512KB body cap; text/html only. Metadata is sanitized plain text (entities decoded, control chars stripped, capped); results including failures live in a bounded in-memory LRU only. Card is text-only, matching Android.Rebinding disclosure:
URLSessionhas no DNS hook, so DNS-name validation uses task transaction metrics after the response: absent metrics or any non-public remote address discards the result (fail closed). A rebound host can receive the initial request before the result is discarded — narrower protection than Android'sDns-level pinning; literal-IP and localhost cases are still rejected before any request. Documented at the site.User Impact
iOS and macOS users can peek at a link's title and description before opening it, with zero background network traffic from message content and failed lookups showing a plain unavailable state.
Evidence
swift test --package-path apps/shared/OpenClawKit— full suite green (542 Swift Testing + 21 XCTest); focused link-preview suites 15 tests / 6 suites: extraction (code exclusion, scheme filtering), OG parsing (fallbacks, entity handling, caps), host policy (IPv4/IPv6 literals incl. CGNAT/ULA/link-local, localhost, *.local), redirect approval, deadline, body cap, fetch-on-tap-only, cache-hit behavior via URLProtocol stubs.xcodebuild build-for-testinggreen;swiftformat --lintclean; new source registered in the iOS filelist.corepack pnpm native:i18n:syncrun for the new strings.corepack pnpm native:i18n:check, the same command CI runs) passes on this exact tree, so the inventory is precisely what the scanner generates. Whether the scanner should extract these particular string constructs is an i18n-tooling scan-pattern question, not staleness in this PR.