Skip to content

fix(web): SnapShots taken during a question attach to that question - #15538

Open
tiliakoos wants to merge 3 commits into
pingdotgg:mainfrom
tiliakoos:fix/12271-snapshot-into-open-question
Open

tiliakoos wants to merge 3 commits into
pingdotgg:mainfrom
tiliakoos:fix/12271-snapshot-into-open-question

Conversation

@tiliakoos

@tiliakoos tiliakoos commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

While a provider question is open, the composer stages dropped files in a per-question draft, so drag-and-drop lands beside the answer. The SnapShot coordinator still delivered shortcut captures to the thread draft, and the composer keyed the flying-capture animation to that same thread draft. A capture taken during a question therefore landed on the hidden normal composer and only appeared after the answer was sent.

Change

  • ChatComposer registers the open question's attachment draft, together with the other question drafts of the same request, while the question can take attachments (the attach button's gate) and its answer is not yet sending. The registration is a layout effect, so it changes in the same commit as the question's UI.
  • SnapShotCoordinator pins that question to each capture the first time it sees it (at the shortcut's request, or before the read for a capture a drain finds), so the animation and the delivery agree. A capture pinned to a question that has since closed goes to the thread draft, never to a newer question.
  • Delivery resolves its destination after the capture is read and compressed, immediately before the insert, so a question that closed during that work does not get its draft recreated with an image its answer can no longer carry.
  • Delivery uses addImages, because addImage refuses drafts without a thread session, which a question draft never has, and refuses a capture once the request's questions already hold the shared attachment limit, as drag-and-drop does.
  • Pending SnapShot animations in the composer are keyed by the attachment target, so the overlay lands on the tile inside the question strip.

Web and desktop share this code. Mobile has no SnapShot path and the browser build has no desktop bridge, so nothing else changes. No contract or server change.

Scope and approval

Closes #12271, accepted by a maintainer with this fix direction: #12271 (comment)

#12293 by @Gigioxx implemented the same routing and was closed only for a missing desktop recording (#12293 (comment)). This PR rebuilds that approach on current main, keeps its answers to the review findings, adds the ownership recheck before the insert that its final review left open, and supplies the recording.

Verification

  • vp test run --project unit src/components/desktop/SnapShotCoordinator.test.ts in apps/web: 20 passed. Five new cases: a capture during an open question lands in the question draft and leaves the thread draft empty; a capture stays on the question it was pinned to and never moves to a newer one; a capture whose question closes while it is being read falls back to the thread draft; a capture pinned before any question stays off a question that opens while it is being read; a capture is refused once the request's other questions hold the shared limit. With the routing reverted the first three fail, with the recheck moved back before the read the third fails, and without the limit check the fourth fails.
  • vp lint and vp fmt --check on the four touched files, and vp run --filter @t3tools/web typecheck: clean. The only lint warnings are the ones already on main.
  • On 2026-10-17 upstream/main at 70d60e03a7 was merged in. The conflicts were against perf(web): ChatComposer compiles with no manual memoization #20397 (the composer's image grouping moved into useComposerImageGroups, which now receives the attachment draft target) and perf(web): snapshot coordinator keeps the last route target in state #20466 (the coordinator keeps its route target in state, so this PR's question pins are the only ref it adds). The tests (25 passed across this file and questionAttachments.test.ts), lint (no warnings), format and the web typecheck were rerun on the merged branch. The dev desktop app run below is from 2026-10-04 and was not retaken.
  • Dev desktop app (vp run dev:desktop) on a fresh state with a throwaway project, a Claude thread with an AskUserQuestion card open, SnapShots on with the default both-Shift shortcut, the dev app in front so it captured itself:
    • Before (upstream's coordinator): the capture landed in the thread draft (checked in the persisted composer draft store); the open question showed no capture. After Submit, the capture surfaced in the normal composer, the report's exact symptom.
    • After (this branch): the capture flew into the question's strip and showed its tile within a second. Submit sent the answer with it, and Claude's reply described the captured T3 Code window.
  • When this PR was opened, main hid the composer's image strip while a question was open (a regression from the V2 merge). The screenshots and the recording below were taken with fix(web): attachments on an open question are visible again #15537's one-line change applied locally, so the tile is visible. fix(web): attachments on an open question are visible again #15537 merged on 2026-10-10, so main now shows the strip without it.
  • The shortcut was fired by posting left Shift and right Shift key events to the HID event tap, which drives the real modifier-pair detector; the app's own capture, animation, delivery and submit paths ran unchanged.
  • Not checked: Windows and Linux capture paths; they share this renderer code and only the capture bridge differs.

Before: the shortcut was pressed while the question was open, and nothing landed in the question (its strip holds only the image dropped earlier):

03-12271-before

Before, after Submit: the capture surfaces in the normal composer, which is the report's symptom:

04-12271-before-after-send

After: the capture sits in the question's strip:

05-12271-after

Recording of the shortcut capture landing in the open question and the answer being submitted with it:

12271-snapshot-into-open-question.mp4

After, Claude's reply to that answer, describing the captured window:

07-12271-after-reply

Claude Fable 5.1 via T3 Code

The coordinator delivered shortcut captures to the thread draft while the
composer staged question attachments in a per-question draft, so a capture
taken during a question stayed hidden until the answer was sent.
Copilot AI balanced review requested due to automatic review settings October 4, 2026 07:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 4, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at d621d99

Macroscope's review found this PR approvable — This is a localized, tested bug fix that routes snapshot captures into the existing active-question attachment flow while preserving normal snapshot behavior elsewhere. Two unresolved High findings concern capture/question timing races and independently block approval until addressed, although the submitted head includes fixes and regression coverage for those cases.

Notes:

  • This verdict was updated automatically after the outstanding correctness findings were resolved. Macroscope did not re-review the code.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a46d69d1-27a9-4397-907d-504b16406b64
📥 Commits

Reviewing files that changed from the base of the PR and between 70d60e0 and d621d99.

📒 Files selected for processing: 4
  • apps/web/src/components/chat/ChatComposer.tsx
  • apps/web/src/components/desktop/SnapShotCoordinator.test.ts
  • apps/web/src/components/desktop/SnapShotCoordinator.tsx
  • apps/web/src/questionAttachments.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Snapshot captures can target the active question attachment draft. Captures use the question draft pinned when first resolved, or fall back to the supplied target if that draft is no longer open. Delivery checks the request-wide attachment limit.

Changes: Question Snapshot Routing

Layer / File(s) Summary
Track active question attachment drafts
apps/web/src/questionAttachments.ts, apps/web/src/components/chat/ChatComposer.tsx
The attachment module tracks the open question draft and its request keys per scoped thread. The composer registers the active draft and resolves pending snapshot animation IDs against the active attachment target.
Resolve and deliver snapshot targets
apps/web/src/components/desktop/SnapShotCoordinator.tsx, apps/web/src/components/desktop/SnapShotCoordinator.test.ts
The coordinator pins each capture to the open question draft or its absence, resolves the target after capture processing, and checks the request-wide attachment limit. Tests cover question-draft routing, target fallback, pin cleanup, and quota rejection.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant ChatComposer
  participant questionAttachments
  participant SnapShotCoordinator
  participant DesktopSnapShotBridge
  ChatComposer->>questionAttachments: Register the active question draft
  SnapShotCoordinator->>questionAttachments: Pin the open draft or its absence
  SnapShotCoordinator->>DesktopSnapShotBridge: Read the captured snapshot
  SnapShotCoordinator->>questionAttachments: Resolve the pinned draft after processing
  SnapShotCoordinator->>SnapShotCoordinator: Add images to the resolved target
Loading

Possibly related PRs

  • pingdotgg/t3code#12293: Addresses the same snapshot routing behavior and includes question-draft targeting, fallback, and attachment-limit checks.

Suggested labels: size:M

Suggested reviewers: bil0000, juliusmarminge

Merge Risk: ⚪ Minimal · up to d621d

No actionable issue remains; the change is mergeable after normal checks.

Security architecture risk: 🔵 Low · up to d621d

Capture routing remains within the existing attachment workflow, with question eligibility checks and destination revalidation before insertion. The main limitation is recovery: after a reload, a pending capture uses the question open when it is rediscovered rather than preserving its original question destination.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed exposure is destination selection for pending desktop captures entering the selected environment/thread's question attachment workflow. Capture payloads can include window imagery and accessibility metadata. The examined path stages these attachments; provider response submission remains a separate answer transition.

Trust Boundaries and Controls

  • observed — The existing native list/read operations require the main renderer's webContents identity, and capture IDs use a bounded restricted-character schema. The renderer requires the existing desktop bridge methods before draining. These controls predate the PR and are not replaced by the question registry.

Resilience and Maintainability Implications

  • observed — Question pins are component-local, while native pending captures persist without destination identity. After reload or missed initiation events, a newly observed capture is pinned to the question then registered before reading. Thus first-observation stability is implemented, but original question ownership is not preserved across remounts. The underlying lack of durable destination metadata predates this PR.

Hardening Proposals

  • proposed — If capture-initiation destination identity is intended as a privacy guarantee across reloads, persist that provenance or require destination confirmation for recovered captures instead of treating the currently open question as authoritative.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly summarizes the primary change: SnapShots taken during an open question attach to that question.
Description check Passed The description includes the problem, implementation details, scope and approval, focused automated tests, manual desktop verification, screenshots, recording, limitations, and the model and harness u…
Linked Issues check Passed Issue #12271 requires a keyboard SnapShot taken while a provider question is open to attach to that question. ChatComposer.tsx registers the eligible question draft in a layout effect and uses it fo…
Out of Scope Changes check Passed The changes in ChatComposer.tsx, SnapShotCoordinator.tsx, questionAttachments.ts, and SnapShotCoordinator.test.ts support issue #12271. The registry, shared-limit check, animation targeting, a…
Full details: Description check

Explanation

The description includes the problem, implementation details, scope and approval, focused automated tests, manual desktop verification, screenshots, recording, limitations, and the model and harness used.

Full details: Linked Issues check

Explanation

Issue #12271 requires a keyboard SnapShot taken while a provider question is open to attach to that question. ChatComposer.tsx registers the eligible question draft in a layout effect and uses it for the animation target. SnapShotCoordinator.tsx pins each capture to the first eligible question, resolves the target before insertion, falls back to the thread draft after question closure, and uses addImages with the shared request limit. Tests cover routing, pinning, closure fallback, question-open timing, and the attachment limit.

Full details: Out of Scope Changes check

Explanation

The changes in ChatComposer.tsx, SnapShotCoordinator.tsx, questionAttachments.ts, and SnapShotCoordinator.test.ts support issue #12271. The registry, shared-limit check, animation targeting, and tests implement or verify the required SnapShot routing. No unrelated changes are evident in the whole-pull-request diff.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 11, 2026
@tiliakoos

Copy link
Copy Markdown
Contributor Author

Merged upstream/main (70d60e03a7) in a614327073, so this is mergeable again. Two files conflicted. In ChatComposer, #20397 moved the image grouping into useComposerImageGroups; it now receives attachmentDraftTarget instead of composerDraftTarget, which is this PR's one-line change there. In SnapShotCoordinator, #20466 replaced lastTargetRef with route-target state, so the question pins are the only ref this PR adds; the import block and the ref declarations were the conflicts, and the pin calls in the requested, started and drain paths merged by themselves. The diff against main is still the four files (+240 −18).

Rerun on the merged branch: SnapShotCoordinator.test.ts and questionAttachments.test.ts 24 passed; lint (no warnings), format and the web typecheck clean. The dev desktop app run in the description is from 2026-10-04 and was not retaken. #15537 has merged since, so the description's note about applying it locally is now history and reads that way.

@macroscopeapp
macroscopeapp Bot dismissed their stale review October 17, 2026 03:49

Dismissing prior approval to re-evaluate a614327

Comment thread apps/web/src/components/desktop/SnapShotCoordinator.tsx
Comment thread apps/web/src/components/desktop/SnapShotCoordinator.tsx
…uestion's commit

A capture the drain found was pinned only once it was read, so a question
opening meanwhile could claim it, and the composer registered the open
question in a passive effect, a frame after the question's UI changed.
@tiliakoos

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Creating a SnapShot while a question is being asked goes into the wrong textbox

3 participants