feat(swift-ios): show workspace images inline - #7378
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a new feature - inline rendering of workspace images in Markdown messages. Despite being self-contained with good test coverage and proper path validation, the new user-facing capability warrants human review. You can add or adjust custom eligibility rules. Learn more. |
|
Both High-severity findings against
Re-run at the current head
The accepted on-device behaviour (Test 89, built from |
|
Second round, against Fixed in Acknowledged, deliberately not changed — Macroscope Medium, cache freshness: overwriting a workspace file at the same thread and path keeps the previously decoded bitmap until the cache evicts it. The reviewer is right about the mechanism. Invalidating it correctly needs a server-provided content version for a workspace asset, which is a wider change than this PR should make; dropping the cache instead reintroduces a signed-URL round trip and a re-download on every transcript cell recycle, which is the problem this cache was added to solve. A transcript is also a historical record of what a message referenced. Glad to follow up separately if a content version is wanted here. Re-run at
The two earlier High findings remain fixed in |
|
Third round, against Fixed in Re-run at
Standing state of the earlier findings: the two High findings are fixed ( |
|
Fourth round, against Fixed in Re-run at
Standing state: all High findings fixed ( Every parsing repair in this series degrades to prior behaviour rather than to a wrong image: a reference the parser cannot read stays alternative text. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a91412b. Configure here.
|
Fifth round, against Fixed in
Re-run at
Standing state: all High findings fixed in |
1f39d72 to
fce7740
Compare
f61d3a3 to
581e1b1
Compare
Markdown image references that stand alone on their line become their own block instead of collapsing to alternative text, and a workspace-relative image path renders inline from the existing signed workspace asset route. Resolution reuses the transcript's attachment thumbnail loader and its bounded cache, keyed by workspace file so a recycled cell never re-resolves a signed URL it has already decoded. Remote, absolute, escaping, and non-image references keep rendering as their alternative text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Unescape backslash-escaped ASCII punctuation in an image reference, so a file named out/foo(1).png resolves instead of being requested literally. Gate the loaded and failed image state on request identity, matching the sibling remote attachment thumbnail: hosted transcript cells reuse view state across recycling and path changes, so ungated state could briefly show another message's workspace image. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A quoted title may hold an unbalanced parenthesis, as in , and counting those hid the real closing delimiter, so the line fell back to alternative text. Delimiter balancing now ignores quoted text while still allowing balanced parentheses inside a file name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ignoring quoted text while balancing delimiters made an apostrophe in a file name, as in images/team's-logo.png, open a title that never closed, so the reference fell back to alternative text. A title is separated from the destination by whitespace, so only a quote in that position opens one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CommonMark wraps an awkward destination in angle brackets, as in (<out/plot).png>), and everything inside is literal. Counting those parentheses ended the destination early, so the reference fell back to alternative text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The destination reader trims whitespace before the brackets, so the delimiter scan now enters literal mode wherever the brackets actually open the destination, not only immediately after the parenthesis. Both sides also share one unescaped scan for the closing bracket, so an escaped `>` inside the brackets stays part of the file name instead of truncating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
581e1b1 to
f050829
Compare
|
This definitely needs before/after screenshots, ideally also a video of the loading in behavior (i hate layout shifts) |
added |
|
Closing as superseded by Theo’s target commit cd2b2f8. The conflict pass selected a rebase, then aborted it when the live target proved to own the same Markdown image parsing, workspace/direct resolution, bounded loading, transcript context, and focused coverage. No branch bytes changed, no force-push occurred, and the newer upstream implementation was preserved exactly. The attempted focused local target run did not execute tests because of recorded Xcode code-signing and disk-headroom failures, so no validation pass or review request is claimed; the updated description contains the full evidence and limitations. |

Observed problem and reproduction
In the SwiftUI chat transcript, Markdown image references to workspace files were rendered as alternative text instead of as images. A representative reproduction is
in a message whose thread workspace containsout/render.png.Cause
At the PR's original base, the native Markdown model had no image block and the transcript had no workspace-aware image resolver/rendering seam.
Change and integration boundary
The original contribution added standalone-image parsing, safe workspace-relative resolution through the existing signed asset route, bounded thumbnail loading, hosted-cell context, failure UI, and focused regressions.
For this conflict pass I selected a rebase onto the live target
22b22f1463b83963d851bd0574a36a20f224a28d. The first commit conflicted inMarkdownDocument.swiftandThreadDetailView.swift. Inspection showed that Theo's newer target commitcd2b2f8fef42e3762d03425e9d43f5a3255d8625independently added the same capability end to end, including broader inline/direct-image behavior and focused source tests. The rebase was aborted. Upstream remains byte-for-byte unchanged and the Saphid head remains60c3e1c56a7d0acb05d6d4fbd40d57fd8e70eadc; this PR is superseded rather than rebased into a duplicate.Non-goals and deferred work
This pass does not alter Theo's newer parser, image loader, workspace resolver, or transcript composition. It does not port the old PR's review-fix series into another contributor's implementation. No web, desktop, React Native mobile, server, contract, provider adapter, or connection behavior is changed.
Affected areas
Validation
gh pr view 7378 --repo pingdotgg/t3code ...confirmed author/head ownersaphid, head branchfeat/issue113-inline-workspace-images, maintainer edits enabled, old head60c3e1c56a7d0acb05d6d4fbd40d57fd8e70eadc.git fetch origin refs/heads/t3code/rebuild-mobile-app-swift:refs/remotes/origin/t3code/rebuild-mobile-app-swiftresolved22b22f1463b83963d851bd0574a36a20f224a28d.git rebase refs/remotes/origin/t3code/rebuild-mobile-app-swiftstopped at the original feature commit with only the two overlapping conflicts above;git rebase --abortexited 0.MarkdownImage,MarkdownImageSource, workspace/direct image loading, transcript image context, and focusedMarkdownDocumentTestsintroduced bycd2b2f8fe.xcodebuild ... test -only-testing:T3CodeTests/MarkdownDocumentTestsexited 65 before tests because the simulator code-signing subsystem reported an internal error.Launch T3CodeTestswhile disk headroom fell to 366 MiB. The exact owned process group was terminated (exit 143), the corrupt incomplete result bundle was identified byxcresulttoolexit 64, and zero tests are claimed. The private DerivedData and corrupt bundle were removed after preserving logs and receipts.Risks, limitations, and known gaps
The superseding target implementation was verified by source and focused target coverage inspection, but this pass did not obtain a successful new local test execution because of the two recorded Xcode/disk infrastructure failures. Historical old-head tests and media do not prove the live target or a hypothetical rebased head.
Known gaps: full-size current-head light/dark before-and-after proof is queued in saphid/t3code-personal#150.
No review is requested because this PR is being closed as superseded.
Evidence
The following images are historical evidence for old PR head
60c3e1c56; they are explicitly not current-target or post-rebase proof.Historical after, light appearance
Historical after, dark appearance
There is no current-head before/after or loading video in this handoff.
Owning issue and stack
Owning lane: saphid/t3code-personal#113. The PR targeted
t3code/rebuild-mobile-app-swift, which remains related to #5178. The contribution is superseded by Theo's target commitcd2b2f8fef42e3762d03425e9d43f5a3255d8625and is not stacked on #7345.Implemented and reconciled by GPT-5.6 Sol high in the Codex harness.