Mobile feel: instant send, prepend anchoring, O(tail) streaming renders - #1058
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesStreaming rendering and cache management
Optimistic attachments and local echoes
Timeline scrolling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two findings from Greptile on #1058, both verified against the code. The boundary scanner closed a template literal on an escaped backtick. The TypeScript rule consumes `\\.`, so `\`` stays inside the string for the tokenizer; treating it as the closer let a following newline advance the stable boundary into the middle of the literal and freeze mis-highlighted text into the immutable prefix. Backslash now skips the next character inside backtick and triple-quote states — the same class, and Python's triple-quoted rule consumes escapes too. Applied unconditionally rather than per language: Go's raw strings have no escapes, so treating a backslash as one there can only miss a closing backtick, which stalls the boundary (slower, still correct), whereas the opposite mistake in TypeScript renders wrongly. Swapping the echo's placeholder refs for host paths replaces the chip, and releasing the in-memory image at that moment left the fresh chip fetching the copy we had just uploaded — a visible flash of the generic placeholder. The image is now promoted onto the host path instead of dropped, so the handoff is seamless and the phone never re-downloads its own upload. The store is bounded to roughly one message's worth of attachments in insertion order, and a save that did not return a ref per placeholder releases rather than pairing positionally, which would attach one image's bytes to another's path. Also: CI's earlier `test-desktop (7)` failure was a flake in `FilesWorkbench.test.tsx` ("expected '-1' to be '2000'"), unrelated to this iOS-only diff — green locally, green on main at the same base, and green on re-run of the same commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
![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: 5feb072461
ℹ️ 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".
| /// Returns the position just past the last newline in `text[start...]` reached | ||
| /// with no multi-line construct open (never earlier than `start`). `start` is | ||
| /// itself such a position, so the scan begins from a clear state. | ||
| private func syntaxStableBoundary(in text: String, from start: String.Index) -> String.Index { |
There was a problem hiding this comment.
When streaming valid Go such as a raw string whose closing backtick follows a backslash, this language-agnostic scanner skips that delimiter even though Go raw strings do not treat backslashes as escapes; a subsequent raw-string opener is then mistaken for the close, allowing the following newline into the immutable prefix while the tokenizer still considers the second string open. Similarly, multiline HTML/YAML quoted strings are matched across newlines by the token rules but are not tracked here. Once later text closes these constructs, the reused prefix cannot be recolored and the incorrect full render is cached, so the completed code block remains mis-highlighted until cache eviction. Pass the language into the boundary scan and mirror that language's multiline token rules.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf64677f03
ℹ️ 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".
| // Exactly one row carries this probe. It measures how far a prepend | ||
| // pushed the reader's content down, which total content height cannot | ||
| // do while the tail is also streaming. | ||
| if entry.id == probeRowId { |
There was a problem hiding this comment.
When the previous first timeline entry is the latest assistant message, visibleTimelineRenderEntries splits it into Markdown rows whose entry.id is suffixed, while probeRowId remains the source timeline ID. This comparison never installs a geometry probe, so loading older history when that assistant is the initial tail cannot arm the prepend anchor and the visible content jumps. Match the render row through sourceEntryId and publish the source ID, selecting only one rendered block.
Useful? React with 👍 / 👎.
| func updateLocalEchoAttachments(echoId: String, attachments: [AgentChatFileRef]?) { | ||
| guard let index = localEchoMessages.firstIndex(where: { $0.id == echoId }) else { return } | ||
| guard localEchoMessages[index].attachments != attachments else { return } | ||
| localEchoMessages[index].attachments = attachments |
There was a problem hiding this comment.
When an image steer remains queued, this mutation replaces ade-pending-upload:// refs with saved host paths without changing the echo count, text, timestamp, or delivery state. Both workIncrementalLocalEchoSignature and workTimelinePresentationSignature omit attachment refs, so the scheduled rebuild can treat the echo as unchanged and retain the uploading chip—potentially until a later canonical transcript refresh. Hash the attachment fields anywhere local-echo equality controls incremental reuse or presentation replacement.
Useful? React with 👍 / 👎.
| case .css: | ||
| return SyntaxMultilineDelimiters(symmetric: [quote, apostrophe], pairs: blockComment) |
There was a problem hiding this comment.
When streamed CSS contains a selector split across lines, such as .foo,\n.bar {, the keyword regex at line 548 matches the entire selector because its character class includes \s, but this delimiter table considers the first newline stable. The prefix is therefore frozen before the whole-text token exists, leaving the completed selector partly unhighlighted. Fresh evidence beyond the earlier delimiter report is that the current per-language fix still omits this non-delimiter multiline token; the boundary logic must cover every newline-crossing rule, not only quotes and comments.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5835da379c
ℹ️ 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".
| case .json: | ||
| return SyntaxMultilineDelimiters(symmetric: [quote]) |
There was a problem hiding this comment.
When valid JSON streams as "key"\n: 1, the key regex at line 565 does not match the snapshot ending at the newline, but its (?=\s*:) lookahead matches after the colon arrives. Returning a delimiter model here declares that newline stable because the quotes are balanced, so the unhighlighted key is frozen into the reused prefix and the completed render remains incorrect in cache. Return nil for JSON or model this cross-line lookahead before reusing its prefix.
Useful? React with 👍 / 👎.
| if let image = attachment.image { | ||
| store(image, forPath: ref.path) |
There was a problem hiding this comment.
When a user sends several high-resolution images, attachment.image is the upload-sized render (up to 2400 pixels), so retaining ten decoded UIImages here can hold roughly 170–230 MB after the composer clears. Promoted entries are not released when echoes reconcile, and the new memory-warning handler does not purge this store, leaving that allocation resident until eleven more previews are inserted. Store chip-sized thumbnails or purge/release these images once the handoff finishes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swift (1)
636-646: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated snapshot-commit sequence.
Lines 636-646 repeat the same four steps used in
applyIncrementalTimelineSnapshotIfPossible(lines 791-801),rebuildTimelineSnapshot(lines 817-827), and the coalescing worker (lines 707-716): assigntimelineSnapshot, recordtimelineIncrementalCache, callrefreshTimelinePresentation, then bumptimelineLayoutPinToken. This PR adds the fourth copy.A shared private helper keeps the four paths from drifting. A future change to the pin condition or to the cache-record arguments currently needs four edits.
♻️ Suggested helper
`@MainActor` private func commitTimelineSnapshot( _ snapshot: WorkChatTimelineSnapshot, transcript: [WorkChatEnvelope], fallbackEntries: [AgentChatTranscriptEntry], artifacts: [ComputerUseArtifactSummary], localEchoMessages: [WorkLocalEchoMessage] ) { timelineSnapshot = snapshot timelineIncrementalCache.record( transcript: transcript, fallbackEntries: fallbackEntries, artifacts: artifacts, localEchoMessages: localEchoMessages ) refreshTimelinePresentation(sourceTimeline: snapshot.timeline) if isNearBottom, !timelineDragActive { timelineLayoutPinToken &+= 1 } }The call site then reduces to:
timelineRebuildGeneration += 1 - timelineSnapshot = nextSnapshot - timelineIncrementalCache.record( - transcript: transcript, - fallbackEntries: fallbackEntries, - artifacts: artifacts, - localEchoMessages: localEchoMessages - ) - refreshTimelinePresentation(sourceTimeline: nextSnapshot.timeline) - if isNearBottom, !timelineDragActive { - timelineLayoutPinToken &+= 1 - } + commitTimelineSnapshot( + nextSnapshot, + transcript: transcript, + fallbackEntries: fallbackEntries, + artifacts: artifacts, + localEchoMessages: localEchoMessages + ) return true🤖 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 `@apps/ios/ADE/Views/Work/WorkChatSessionView`+Actions.swift around lines 636 - 646, Extract the repeated snapshot-commit sequence into a private `@MainActor` helper named commitTimelineSnapshot, accepting the snapshot and the existing transcript, fallbackEntries, artifacts, and localEchoMessages values. Move assignment, cache recording, presentation refresh, and pin-token logic into the helper, then replace the sequences in the current site, applyIncrementalTimelineSnapshotIfPossible, rebuildTimelineSnapshot, and the coalescing worker with helper calls.apps/ios/ADETests/WorkMarkdownStreamingParsingTests.swift (1)
347-385: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider bounding the diagnostic benchmark so it does not dominate CI time.
testStreamingHighlightCostIsReportedreplays ~625 snapshots twice. The legacy pass re-tokenizes the whole snapshot and walksindex(offsetBy:)fromstartIndexfor every token, so its cost grows super-linearly with snapshot length. The test asserts nothing, so the runtime buys no signal on a normal CI run.Reduce
count: 200or gate the legacy pass behind an environment flag.🤖 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 `@apps/ios/ADETests/WorkMarkdownStreamingParsingTests.swift` around lines 347 - 385, Bound the diagnostic workload in testStreamingHighlightCostIsReported so it does not dominate CI runtime: reduce the fullText repetition count from 200 to a smaller representative value, or run the legacyHighlight pass only when an environment flag enables it. Preserve the benchmark’s reporting behavior when the legacy comparison is enabled.
🤖 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 `@apps/ios/ADE/Views/Work/WorkChatSessionView.swift`:
- Around line 720-725: Update the prepend-anchor restoration logic associated
with WorkChatPrependAnchor so the displacement is added to the current
scrollMetrics.offsetY rather than the anchor’s captured offsetY. Apply the same
change to both referenced restoration paths, preserving the existing
displacement calculation and anchor behavior.
---
Nitpick comments:
In `@apps/ios/ADE/Views/Work/WorkChatSessionView`+Actions.swift:
- Around line 636-646: Extract the repeated snapshot-commit sequence into a
private `@MainActor` helper named commitTimelineSnapshot, accepting the snapshot
and the existing transcript, fallbackEntries, artifacts, and localEchoMessages
values. Move assignment, cache recording, presentation refresh, and pin-token
logic into the helper, then replace the sequences in the current site,
applyIncrementalTimelineSnapshotIfPossible, rebuildTimelineSnapshot, and the
coalescing worker with helper calls.
In `@apps/ios/ADETests/WorkMarkdownStreamingParsingTests.swift`:
- Around line 347-385: Bound the diagnostic workload in
testStreamingHighlightCostIsReported so it does not dominate CI runtime: reduce
the fullText repetition count from 200 to a smaller representative value, or run
the legacyHighlight pass only when an environment flag enables it. Preserve the
benchmark’s reporting behavior when the legacy comparison is enabled.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 839842b2-5d22-46c4-b03f-9cb21a4d6b26
⛔ Files ignored due to path filters (1)
docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (15)
apps/ios/ADE/App/ADEAppDelegate.swiftapps/ios/ADE/Views/Components/ADECodeRenderingCache.swiftapps/ios/ADE/Views/Components/FilesCodeSupport.swiftapps/ios/ADE/Views/Work/WorkChatAttachmentTray.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkMarkdownParsing.swiftapps/ios/ADE/Views/Work/WorkMarkdownViews.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/WorkMarkdownStreamingParsingTests.swiftapps/ios/ADETests/WorkSessionCanonicalStateTests.swift
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef44f6131e
ℹ️ 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".
| // Corrected from the *live* offset, not the captured one. The reader can | ||
| // keep scrolling between arming and the layout pass that measures the | ||
| // displacement; correcting from an offset captured at arm time would snap | ||
| // them back to where they were when the page started loading. Only the | ||
| // inserted height needs undoing. | ||
| scrollPosition.scrollTo(y: scrollMetrics.offsetY + displacement) |
There was a problem hiding this comment.
When an older-history page lands while the user is actively dragging, the probed row displacement includes both the inserted height and the offset change caused by that drag. If the prepend height is H and the user scrolls by D, this computes displacement = H - D, then adds it to the live offset old + D, producing old + H and cancelling the user's movement. Capture the offset alongside rowY and subtract its change when deriving the inserted height so the correction preserves scrolling performed during layout.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b8dc1e44c
ℹ️ 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".
| let next = workUnrepresentedLocalEchoMessages( | ||
| localEchoMessages, | ||
| representedKeyCounts: workRepresentedEchoKeyCounts(from: transcript) | ||
| ) |
There was a problem hiding this comment.
When two identical quick sends exist but the transcript contains only one matching row, the first invocation removes the older echo, while any subsequent invocation applies that same transcript count to the already-pruned array and removes the newer echo too. This happens immediately because loadTranscript already calls reconcileLocalEchoMessages, and schedulePostSendReconciliation calls it again without another transcript update; later polls have the same problem. Track which canonical rows have already retired echoes, rather than repeatedly consuming the total represented count.
Useful? React with 👍 / 👎.
| // The probe has to already be measuring the row we are about to anchor | ||
| // on, or there is no "before" position to restore to. | ||
| scrollMetrics.probeRowId == previousFirstId, | ||
| let previousFirstRowY = scrollMetrics.probeRowY |
There was a problem hiding this comment.
If an older-history request is in flight and the user scrolls far enough that the former first row is recycled by LazyVStack, the preference observer clears probeRowId and probeRowY; when the page arrives, this guard declines to arm an anchor, so the newly inserted rows push whatever the user is currently reading. Retain the last valid anchor sample or capture an appropriate visible-row anchor when the request starts instead of requiring the original first-row probe to remain mounted until the response lands.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cb1f6aae7
ℹ️ 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".
| if character == "\\", delimiter.escapes { | ||
| pendingEscape[position] = true | ||
| continue |
There was a problem hiding this comment.
When JavaScript/TypeScript contains a backslash-apostrophe outside an apostrophe string—for example one comment containing \' and a later comment containing '—this sets pendingEscape while the delimiter count is even and skips the first apostrophe. The string regex can nevertheless start at that apostrophe because it scans independently of the comment rule and has no preceding-backslash check, so the scanner freezes the intervening newline as stable; when the later apostrophe arrives, the whole-text tokenizer spans both comments but the cached prefix cannot be recolored. Fresh evidence beyond the earlier delimiter issues is that the current per-language scanner applies escape handling even when that delimiter is not open; only treat backslashes as escapes while its count is odd.
Useful? React with 👍 / 👎.
Two findings from Greptile on #1058, both verified against the code. The boundary scanner closed a template literal on an escaped backtick. The TypeScript rule consumes `\\.`, so `\`` stays inside the string for the tokenizer; treating it as the closer let a following newline advance the stable boundary into the middle of the literal and freeze mis-highlighted text into the immutable prefix. Backslash now skips the next character inside backtick and triple-quote states — the same class, and Python's triple-quoted rule consumes escapes too. Applied unconditionally rather than per language: Go's raw strings have no escapes, so treating a backslash as one there can only miss a closing backtick, which stalls the boundary (slower, still correct), whereas the opposite mistake in TypeScript renders wrongly. Swapping the echo's placeholder refs for host paths replaces the chip, and releasing the in-memory image at that moment left the fresh chip fetching the copy we had just uploaded — a visible flash of the generic placeholder. The image is now promoted onto the host path instead of dropped, so the handoff is seamless and the phone never re-downloads its own upload. The store is bounded to roughly one message's worth of attachments in insertion order, and a save that did not return a ref per placeholder releases rather than pairing positionally, which would attach one image's bytes to another's path. Also: CI's earlier `test-desktop (7)` failure was a flake in `FilesWorkbench.test.tsx` ("expected '-1' to be '2000'"), unrelated to this iOS-only diff — green locally, green on main at the same base, and green on re-run of the same commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5cb1f6a to
8e1c01c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a32749647e
ℹ️ 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".
| VStack(alignment: .leading, spacing: 10) { | ||
| ForEach(blocks) { block in | ||
| WorkMarkdownBlockView(block: block) | ||
| WorkMarkdownBlockView(block: block, isStreamingTail: block.id == streamingTailId) |
There was a problem hiding this comment.
When the actively streaming tail parses as a Markdown table and a cell grows token by token, this passes isStreamingTail == true to WorkMarkdownBlockView, but the table branch constructs WorkMarkdownTable without forwarding that state and its cells use the default shared-cache path. Every transient cell revision therefore enters the 256-entry completed-message cache, so a sufficiently long table evicts prior transcript renders and reintroduces scroll-back reparsing and jank. Pass the streaming state into the table and route its still-growing cell revisions through the intermediate cache.
Useful? React with 👍 / 👎.
Tier 1A of the t3code competitor audit — three client-side changes to how the iOS chat feels. No protocol changes. Instant send. The user's bubble now paints on the tap frame: - Local echoes get a synchronous incremental apply instead of waiting out the 90 ms coalescing debounce that exists for host deltas. A retired generation counter keeps an in-flight coalesced rebuild from overwriting the just-painted bubble. - Image sends echo before the upload, not after. The composer's own downscaled image renders behind an uploading state under a placeholder ref, swapped for the real host path before the send so the echo's dedupe key still matches the transcript row that comes back. - `sending` releases when the host accepts the message. The four-round-trip refresh cascade (transcript, artifacts, summary, session) now runs behind the composer, chained so two quick sends cannot interleave two transcript loads. A second message can be composed immediately. Also fixes the incremental echo path dropping attachments, and gives it the echo-suppression rules the full rebuild applies, so the fast path and the full path agree. Prepend anchoring. Older history inserted above the viewport used to push whatever the reader was looking at down by the height of the inserted page. Scroll geometry is now captured before any presentation change that inserts rows above the current first row — covering both buffered reveal and host history pages — and the offset is restored by the measured height delta in a non-animated transaction. The reveal itself is no longer animated: those rows are off-screen and immediately offset-corrected, so animating them only produced a flash. Bottom-follow, the jump-to-latest pill, and the initial force-pin are untouched. Streaming render caches: - Syntax highlighting was the worst main-thread path in a long reply: the cache keyed on the full code text (new key per token) and applied attributes by walking `index(offsetBy:)` from the start for every token. It now reuses an already-highlighted stable prefix — the same shape `parseMarkdownBlocksForStreaming` uses for prose, split at the last line boundary provably outside a block comment or backtick/triple-quote string — and applies attributes with a single forward cursor. Measured over a 475-tick replay of an 11 KB block: 5.19 ms -> 0.105 ms per tick (49x). - Streaming tail revisions no longer land in the shared 256-entry inline markdown cache. One long turn used to insert hundreds of throwaway entries and evict every completed message, so scrolling back re-parsed the transcript on the main thread. Intermediate revisions render from their own small cache; the final revision is promoted. - Cache keys use the existing `workStableDigest` fingerprint instead of bridging whole strings, and every derived render cache purges on `didReceiveMemoryWarning`. Tests: streaming-equivalence property tests assert the incremental highlight equals a from-scratch highlight at every snapshot (these caught a real bug where the scanner state was stored at end-of-text rather than at the boundary, splitting inside a Python docstring), plus intermediate-exclusion, promote-on-complete, and a 40-message eviction regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six findings from the /quality dual review (four raised independently by
CodeRabbit), all verified against the code before applying.
Prepend anchoring restored by total content height, which double-counts a
streaming tail. A reader scrolled back through history is exactly when an
assistant reply is still growing at the bottom, and that growth landed in
the same `contentSize.height` delta the correction was derived from, so the
restore overshot by the tail's growth. The displacement is now measured on
the anchored row itself, via a single geometry probe that rides whichever
row leads the list (and stays pinned to the anchored row while a prepend is
in flight). Tail growth contributes nothing to it. The probe publishes its
own row id alongside the measurement — which row carries it is decided
during body evaluation, and an observer that recomputed the id later could
pair it with a different row's y.
`highlightedSegment` retained an `AttributedString.Index` across the
attribute assignment that followed it. Attribute mutation invalidates
indices, so that was undefined even though it happened to work. It now
paints a role per UTF-16 position and appends runs in one forward walk over
immutable text, needing no `AttributedString` index at all.
That rewrite was also untested: the streaming-equivalence tests compared
`highlightedAttributedString` against `highlightedSegment`, which share
their span logic, so they could not catch a change in it. Added an oracle
test against the pre-change whole-text algorithm — it immediately caught a
real defect (a token fully containing a later one lost its trailing
portion), and `SyntaxTokenRole.tint`/`.font` are no longer private so the
oracle can apply the real attributes instead of stand-ins.
HTML comments span lines exactly like `/* */`, and the boundary scanner
did not model them, so a stable prefix could freeze mis-highlighted markup
mid-comment. Added the state plus a `syntaxMatches` marker helper.
Echo suppression tested set membership, so one transcript row retired every
echo sharing its dedupe key. Sending the same text twice ("ok", "continue")
made both bubbles vanish on the first matching row — reachable now that
`sending` releases at host acceptance rather than after the refresh
cascade. Suppression counts matches and consumes one slot per echo, applied
to all three sites that had the pattern (timeline build, reconciliation,
and the incremental fast path's agreement guard).
Dropped the wall-clock assertion from the highlighter benchmark; it stays
diagnostic, with correctness pinned by the oracle test instead. Renamed
`scheduledPostSendReconciliation` to the verb form.
1311 tests, 17 failures — the same pre-existing set as clean main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…work
Moves the local-echo suppression tests out of the markdown streaming file
and into `WorkSessionCanonicalStateTests`, which already owns local-echo
dedupe coverage — the two attachment-identity cases there cover echoes with
*different* keys, and the counted-suppression cases sit directly beside
them. No new test file, so no pbxproj registration and no new sibling in an
already-large folder.
Documents the three perceived-latency mechanisms in the iOS companion doc.
One of them was already asserted there ("preserving the visible scroll
anchor as pages prepend") without an implementation behind it; that claim is
now true, and the note records why the correction is measured on the
anchored row rather than on total content height.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two findings from Greptile on #1058, both verified against the code. The boundary scanner closed a template literal on an escaped backtick. The TypeScript rule consumes `\\.`, so `\`` stays inside the string for the tokenizer; treating it as the closer let a following newline advance the stable boundary into the middle of the literal and freeze mis-highlighted text into the immutable prefix. Backslash now skips the next character inside backtick and triple-quote states — the same class, and Python's triple-quoted rule consumes escapes too. Applied unconditionally rather than per language: Go's raw strings have no escapes, so treating a backslash as one there can only miss a closing backtick, which stalls the boundary (slower, still correct), whereas the opposite mistake in TypeScript renders wrongly. Swapping the echo's placeholder refs for host paths replaces the chip, and releasing the in-memory image at that moment left the fresh chip fetching the copy we had just uploaded — a visible flash of the generic placeholder. The image is now promoted onto the host path instead of dropped, so the handoff is seamless and the phone never re-downloads its own upload. The store is bounded to roughly one message's worth of attachments in insertion order, and a save that did not return a ref per placeholder releases rather than pairing positionally, which would attach one image's bytes to another's path. Also: CI's earlier `test-desktop (7)` failure was a flake in `FilesWorkbench.test.tsx` ("expected '-1' to be '2000'"), unrelated to this iOS-only diff — green locally, green on main at the same base, and green on re-run of the same commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex was right that the stable-prefix scan cannot be language-agnostic, and chasing it turned up that my previous two attempts were both unsound. Escapes are per *delimiter*, not per language. The last commit skipped the character after any backslash, which is correct for a TypeScript template literal and wrong for a Go raw string, where `\` before the closing backtick does not escape it. Because parity is cumulative, one uncounted delimiter flips the reading of every later line — the added Go test fails on that commit: a following multi-line raw string looks closed and the boundary lands inside it. Each delimiter now carries its own `escapes` flag from a table that sits next to the token rules it mirrors. Deriving the boundary from the tokens instead was also tried and is wrong for streaming: while a block comment is still unterminated no token covers it, so the boundary advances into text that becomes a comment once the closer arrives. That is recorded in the doc comment so it is not attempted a third time. What the scan asks is deliberately weaker than "what is open here?". The tokenizer runs each rule independently over the whole text, so a `'` inside a `//` comment really does open a string match and no state machine can mirror that. Balance cannot be fooled the same way: anything unbalanced since the last boundary simply refuses the split, so a wrong guess costs a shorter prefix, never a wrong render. That also covers the multi-line quoted strings Codex flagged in HTML and YAML — every rule using `"(?:[^"\\]|\\.)*"` can cross a newline, since `[^"\\]` matches one. The scan resumes at the reused prefix rather than rescanning the block, and each segment tokenizes only itself, so the per-tick cost stays proportional to the new tail: 5.86 ms -> 0.147 ms (39.7x) on the same 475-tick replay. New coverage: Go raw string with a trailing backslash, HTML attribute and YAML value spanning lines, an apostrophe in a JS comment, and escaped delimiters in TypeScript and Python. 1311 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…refs
Three findings from Codex, all verified.
Some rules cross a newline with no delimiter to count at all: CSS matches a
selector list through `[...\s,>+~]*\s*\{`, YAML's key rule opens with `^\s*`,
and a Markdown link's `[^\]]+` spans lines. This is the third distinct way
the boundary model has been incomplete, so rather than add a fourth
construct, `multilineDelimiters(for:)` is now optional and those three
languages return nil: no prefix reuse, whole-text highlight per tick,
exactly what they did before incremental highlighting existed. Modeling
those rules would mean re-implementing each regex, and a model that is
*nearly* right is what produced the bugs. Declaring the gap costs three
languages the speedup and costs correctness nothing. Everything else —
Swift, TypeScript, JavaScript, Python, Rust, Go, Java, HTML, JSON — keeps
it, and a test pins which side each language is on.
The prepend probe compared a render-row id against a timeline-entry id. A
streaming or expanded assistant message renders as several suffixed block
rows, so when such a message led the visible list the probe never installed,
the anchor never armed, and paging silently fell back to jumping. Resolved
through `sourceEntryId`, picking that entry's first block, and published in
timeline-entry id space so the anchor comparison still matches.
`workIncrementalLocalEchoSignature` omitted attachment refs. Swapping a
pending upload for its host path changes nothing else about the echo, so the
assistant-tail fast path could treat it as unchanged and keep rendering the
uploading chip until a canonical refresh. The render and presentation
signatures already hashed refs; this one now does too.
1323 tests, same 13 pre-existing failing cases as clean main. 38.6x held.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three findings from Codex and CodeRabbit, all verified. JSON opts out of prefix reuse. Its key rule only matches once `(?=\s*:)` finds the colon, which can arrive after the newline, so a key streamed as `"key"\n: 1` would freeze unhighlighted into the prefix. That is the fourth distinct rule shape this boundary has been wrong about, so the claim is now pinned rather than re-argued: a test fingerprints the rule patterns of every language still allowed to reuse a prefix, and editing one of those rules fails with instructions to re-check it against the delimiter model. The property is about the patterns and cannot be re-derived at runtime; the only honest thing to do is make drift impossible to land silently. While measuring whether to drop prefix reuse entirely, the earlier claim that the quadratic index walk was the dominant cost turned out to be wrong. Whole-text highlighting with the role fill runs at 5.7 ms/tick against the previous 5.4 ms — no better. The whole win is prefix reuse (0.139 ms/tick, 38.7x), which is why the answer here is to bound the claim rather than abandon it. The benchmark now prints all three numbers so this is not mis-stated again. The pending-upload store held the composer's *upload* render — up to 2400px, roughly 23 MB decoded — so ten attachments could pin a quarter-gigabyte after the composer cleared. It now stores a 256px chip-sized thumbnail (about 260 KB), which is also what the host path would have produced for these 56-72pt chips, and purges on `didReceiveMemoryWarning` along with the other render caches. The prepend restore corrected from the offset captured at arm time, so a reader who kept scrolling while the page loaded was snapped back to where they started. It corrects from the live offset now; only the inserted height needs undoing. That leaves the anchor's captured `offsetY` unread, so it is gone rather than left as a field nothing consults. 1325 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the prepend correction with the other half of the same formula. CodeRabbit's finding was that correcting from the offset captured at arm time snaps a still-scrolling reader back; Codex's is that the captured offset is still needed, because the probed row moves by the inserted height *minus* whatever the reader scrolled — scrolling moves the row up the screen too. Correcting by that raw displacement cancels their movement. Adding the offset change back isolates the insertion: with an inserted height H and a user scroll D, the row moves H - D while the offset moves D, so the sum is H either way, and a pure scroll with no prepend sums to zero and correctly restores nothing. The anchor's `offsetY` returns for that purpose rather than as the thing corrections are applied to. 1325 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more findings from Codex, both verified. Echo reconciliation ran against the same transcript more than once — `loadTranscript` reconciles, then the post-send pass reconciles again — and consuming a represented count per call is not idempotent. With two identical sends and one canonical row, the first call correctly retired one echo and the second applied that same count to the already-pruned array and retired the survivor. That is the same bubble-disappears symptom the counted suppression was introduced to fix, one layer down. A key is now retired only once the transcript holds at least as many rows as there are echoes for it, which is stable under repetition and costs nothing in between: `buildWorkTimeline` filters the surplus out of the rendered timeline, and that filter is a pure function of the full echo list. The logic moved to `workLocalEchoesRetiredByTranscript` so the property can be tested directly — the new test reconciles three times against one row and asserts the survivor lives. The prepend probe cleared its recorded sample whenever no row published one. A LazyVStack recycles the probed row if the reader scrolls away while an older-page request is in flight, so the page could land with nothing to arm against and push whatever they had moved on to. The last real measurement is retained instead. 1326 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cross-referencing the anchor against t3code's shipped implementation of the same pattern (`ThreadDetailView.restore(_:in:dataSource:)`) surfaced one thing theirs does that this did not: they bound the computed offset to `[-adjustedContentInset.top, contentSize.height - bounds.height + inset]` before `setContentOffset`. A measured inserted height should never produce an out-of-range target, since the content grew by at least that much. But the retained last-probe path — added so a recycled row does not lose the anchor — can carry a measurement from before the recycle, and an unbounded restore turns a stale number into an overscroll past the end of the transcript. Bounded, the same staleness lands a little off instead. The geometry observer tracks the scrollable height again for this, and only this; the correction itself is still derived from the anchored row's own displacement, never from total content growth. 1331 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The scan honored a delimiter's escape rule wherever a backslash appeared, including where that delimiter had nothing open. In JavaScript a `\'` sitting in a comment therefore swallowed the apostrophe — but the string rule scans independently of the comment rule and has no preceding-backslash check, so it opens a match right there and runs to the next apostrophe, lines later. The newline in between was marked stable and the span froze into the prefix before its closer arrived. Escapes now apply only while that delimiter's count is odd, which is the scan's own notion of "inside one of these". The template-literal and triple-quote cases still hold — those backslashes occur inside an open string, which is exactly when the rule now fires — and Go's raw strings are unaffected either way. 1332 tests. The failing set matches the baseline apart from `SyncEnvelopeChunkAssemblerTests.testOutboundFramesStayInsideBudgetAndReassemble`, which passes in isolation (18/18) and cannot be reached from a syntax highlighter; recorded as a flake, not adopted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tail block of a streaming message can parse as a table, and that branch built `WorkMarkdownTable` without forwarding `isStreamingTail`, so its cells fell back to the shared completed-message cache. A table growing cell by cell therefore did exactly what the intermediate exclusion exists to prevent: filled the 256-entry cache with throwaway revisions and evicted the finished messages above it, putting scroll-back re-parsing back on the main thread. The state is threaded through the table into its header and body cells. The regression test streams a cell token by token and asserts a previously cached completed message survives, then that the settled cell is promoted like any other block. 1333 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
a327496 to
219a49f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 219a49f721
ℹ️ 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".
| transcript: [WorkChatEnvelope] | ||
| ) -> [WorkLocalEchoMessage] { | ||
| guard !echoes.isEmpty else { return echoes } | ||
| let representedCounts = workRepresentedEchoKeyCounts(from: transcript) |
There was a problem hiding this comment.
When a user sends the same attachment-free text after an earlier identical send has fully reconciled, representedCounts still includes the historical transcript row while echoCounts contains only the new echo. The comparison therefore retires the new echo before its own canonical row exists; the same lifetime count passed to workUnrepresentedLocalEchoMessages also hides it from the immediate timeline rebuild. Track which transcript rows have acknowledged the current echo generation rather than comparing current echoes against all historical text matches.
Useful? React with 👍 / 👎.
1 participant
Tier 1A of the 2026-08-09 t3code competitor audit: the mobile perceived-latency trio. All client-side, iOS only, no protocol changes.
Instant send
The user's bubble now paints on the tap frame.
sendingreleases when the host accepts the message. The four-round-trip refresh cascade (transcript → artifacts → summary → session) runs behind the composer, chained so two quick sends can't interleave two transcript loads. A second message can be composed immediately.Two pre-existing defects fell out of this: the incremental echo path dropped attachments, and it lacked the echo-suppression rules the full rebuild applies — the fast and slow paths disagreed.
Prepend anchoring
Older history inserted above the viewport used to push whatever the reader was looking at down by the height of the inserted page. Scroll state is captured before any presentation change that inserts rows above the current first row — covering both buffered reveal and host history pages — and restored after layout in a non-animated transaction.
The displacement is measured on the anchored row itself, not on total content height: a reader scrolled back through history is exactly when a reply is still growing at the bottom, and a total-height correction would add that tail growth and overshoot. Bottom-follow, the jump-to-latest pill, and the initial force-pin are untouched.
Streaming render caches
Syntax highlighting was the worst main-thread path in a long reply: the cache keyed on the full code text, so every streamed token was a new key and a full re-highlight. It now reuses an already-highlighted stable prefix — the same shape
parseMarkdownBlocksForStreaminguses for prose — and paints a role per position in one forward walk. 5.4 ms → 0.139 ms per tick (38.7x), measured over a 475-tick replay of an 11 KB block.Reuse is only sound while nothing in a language's rules matches across a newline except the delimiters the boundary counts. CSS, YAML, Markdown, and JSON each have a rule that does (
[...\s,>+~]*\s*\{,^\s*,[^\]]+, and a(?=\s*:)lookahead), so they opt out and highlight whole-text as before. A test fingerprints the rule patterns of every opted-in language, so editing one fails loudly instead of silently invalidating the boundary.Note for reviewers: the per-position attribute rewrite (which replaced an
index(offsetBy:)walk per token) is not where the win comes from — whole-text with that rewrite measures 5.7 ms/tick, no better than before. It was worth doing because retaining anAttributedString.Indexacross an attribute assignment is undefined, but the speedup is entirely prefix reuse. The benchmark prints all three numbers.Streaming tail revisions no longer land in the shared 256-entry inline-markdown cache. One long turn used to insert hundreds of throwaway entries and evict every completed message, so scrolling back re-parsed the transcript on the main thread. Intermediate revisions render from their own small cache; the final revision is promoted.
Cache keys use the existing
workStableDigestfingerprint instead of bridging whole strings, and every derived render cache purges ondidReceiveMemoryWarning.Preserved
ADE's stable-prefix incremental markdown parser and its scroll-follow latch (sentinel + deadband + jump-to-latest pill) are both ahead of t3's equivalents and are extended here, not replaced.
Verification
SyncRecoveryPolicy/PairingAndDpop/ PR-list failures, none in this diff's surface)./qualityran two full dual-review passes; CodeRabbit ran twice independently. Seven findings applied, one rejected as a verified false positive.Prepend anchoring: cross-reference against t3code's shipped implementation
Device verification was waived, so this mechanism is validated by comparing it point-by-point against t3code's production implementation of the same pattern (
apps/swift-ios/Features/Chat/ThreadDetailView.swift, brancht3code/rebuild-mobile-app-swift) — theirVisibleAnchorcapture/restore ships in TestFlight — and against SwiftUI's documented scroll semantics.The correction formula is algebraically the same
t3code captures
offsetFromViewportTop = attributes.frame.minY - contentOffset.yand restores toattributes.frame.minY - offsetFromViewportTop, i.e.This PR measures the anchored row's position in the scroll coordinate space — which is their
offsetFromViewportTop— and computesSame quantity, corrected from a different origin. They coincide exactly when
newOffsetY == oldOffsetY.Where it deliberately differs, and why
dataSource.applycompletion, so the reader cannot scroll in between and the two are identical. Here, capture happens when the presentation is assigned and restore on a later layout pass, so a reader scrolling during the page load would be snapped back. Correcting from the live offset preserves their scroll and undoes only the insertion.indexPathsForVisibleItemsdoes.LazyVStackcan recycle the probed row mid-request; without retention the page lands with nothing to arm against.shouldFollowBottom), and explicitly disablesmaintainsBottomAnchorbefore restoringguard !isNearBottom)isNearBottom, so when the guard passes no pin can fire — the same mutual exclusion their explicit disable buys. Deciding at restore additionally handles a reader who leaves the bottom mid-request.animatingDifferences: false,animated: falseThe one divergence the cross-reference exposed, now fixed
t3code clamps its computed offset to
[-adjustedContentInset.top, contentSize.height - bounds.height + inset]beforesetContentOffset. This PR did not clamp. A measured inserted height should never be out of range — the content grew by at least that much — but the retained last-probe path can carry a measurement from before a recycle, and unbounded that becomes an overscroll past the end of the transcript. Now bounded, so the same staleness lands slightly off instead of blank. (8e1c01c41)One known bounded gap
t3code also re-anchors when its "Load earlier" cell disappears (
loadEarlierChanged && !canLoadEarlier). Here, the equivalent header collapse is normally absorbed automatically — the correction is measured on the row's actual displacement, so it already includes any height change above it, which is strictly more general than their special case. The uncovered sliver is a header that changes with no row insertion: reaching the very top of history when the final page returns zero entries shifts content by the ~28pt loading row. Bounded, terminal, once per thread. Left as-is rather than widening the arm condition, which would add its own over-arming risk.SwiftUI semantics relied on
ScrollPosition.scrollTo(y:)sets the vertical content offset in the same coordinate spaceScrollGeometry.contentOffset.yreports, which is what makes the measured delta directly applicable. The binding is never written except during a restore, so it does not compete with theScrollViewProxy.scrollTocalls that drive bottom-follow and the jump-to-latest pill.Known gap — please read before approving
Prepend anchoring is not verified on a device, and it is the part of this PR that has been wrong most often. Pairing succeeds over LAN but the signed-out gate never flips
hasPairedHost(SyncService.swift:18313needs a keychain token read that fails on a fresh install), so a chat surface was never reachable. That looks like the #1019 keychain class recurring, and it blocks any visual iOS verification on a fresh install — worth its own lane.Across review, prepend anchoring alone produced four separate valid findings: restoring by total content height (double-counted a streaming tail), probing a render-row id against a timeline-entry id (the anchor silently never armed), correcting from a stale captured offset (snapped the reader back), and not separating the reader's own scrolling from the insertion. Each is fixed and reasoned, and none is observed. If you would rather that item land separately once a device can verify it, it is self-contained —
WorkChatPrependAnchor, the probe preference key, andarmPrependAnchorIfRowsInsertedAbove/restorePrependAnchorIfNeeded— and the rest of the PR stands without it.🤖 Generated with Claude Code
Greptile Summary
The PR improves perceived iOS chat latency and long-transcript rendering.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains established on the current HEAD.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (10): Last reviewed commit: "fix(ios): route streaming table cells th..." | Re-trigger Greptile