Skip to content

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

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

tiliakoos wants to merge 1 commit into
pingdotgg:mainfrom
tiliakoos:fix/12271-snapshot-into-open-question

Conversation

@tiliakoos

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.
  • SnapShotCoordinator pins that question to each capture the first time it sees it, 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: 19 passed. Four 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 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.
  • 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.
  • Current main hides the composer's image strip while a question is open (a regression from the V2 merge, fixed by fix(web): attachments on an open question are visible again #15537). The screenshots and the recording below were taken with that one-line change applied locally, so the tile is visible; this PR does not include 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 c5b865a

Macroscope's review found this PR approvable — This is a focused fix to route desktop snapshots into the currently open question attachment draft, with explicit handling for question closure, animation targeting, and shared attachment limits. The change is localized, tested, and does not alter schemas, deployment behavior, product defaults, or static-analysis configuration.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

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 →

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 configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 70f4b82b-acc6-47a3-ad85-ed96999d78cf
📥 Commits

Reviewing files that changed from the base of the PR and between 6e0abd5 and c5b865a.

📒 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

Walkthrough

Snapshot captures can now target the active question attachment draft. Captures are pinned to the question open when capture begins, fall back to the thread target if that question closes, and are subject to 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 uses it to look up pending snapshot animation IDs.
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, resolves its target after capture processing, and checks the request-wide attachment limit. Tests cover routing, 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: Read and pin the open draft for a capture
  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, including question-draft targeting, fallback, and attachment limits.

Suggested labels: size:M

Suggested reviewers: bil0000, juliusmarminge

Merge Risk: ⚪ Minimal · up to c5b86

No identified issue blocks merging the snapshot-routing change after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c5b86

Normal delivery preserves question attachment gates and shared limits. However, a pending capture can survive a reload while its question association cannot, allowing recovery to attach it to a different question. Sending that attachment still requires user action.

Retained concerns

  • Low · security · inferred: Question ownership does not survive renderer recovery. If a capture remains unacknowledged after being pinned to one question, a reload discards that pin while retaining the capture. Replay can then attach it to the currently open question rather than preserving the original decision or falling back conservatively. Subsequent user submission can include private capture content in an unintended answer. Same-mount safeguards do not cover this transition.
Security review details

Security Blast Radius

  • inferred — The evidenced exposure is pending capture content belonging to the desktop user and attachment drafts in the selected environment/thread. The new recovery risk concerns which question receives that content; existing recovery could already select the current thread. The inspected path does not demonstrate privilege gain or service-wide compromise.

Security Findings and Attack Paths

  • inferred — A capture left pending through failure or interruption can survive a renderer reload. With its original pin lost, replay can select a different open question. If the user subsequently submits that answer after upload completes, the recovered image is included in the question attachment map. This is a conditional privacy path; attacker-controlled triggering and automatic transmission were not established.

Trust Boundaries and Controls

  • observed — Question registration requires attachment support, custom-answer eligibility, and a non-responding question. Draft identity includes environment, thread, request, and question identifiers. Answer submission checks a resumable request and completed uploads before sending attachments grouped by question ID.

Resilience and Maintainability Implications

  • observed — Within one mounted coordinator, draining is serialized, insertion deduplicates capture IDs within the destination draft, and acknowledgement follows verified persistence. Delivery errors leave the capture pending for retry. These controls protect normal retry handling but do not persist ownership across recovery.

Hardening Proposals

  • proposed — Preserve the original destination decision across recovery, or avoid assigning recovered captures without ownership provenance to an active question. Validate reload and retry transitions where the original question has closed or a different question is open.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: routing SnapShots taken during an open question to that question.
Description check ✅ Passed The description covers the problem, implementation, scope and maintainer approval, and focused verification. It includes test results and visual evidence, and identifies untested platforms and the loc…
Linked Issues check ✅ Passed Issue #12271 requires keyboard SnapShots taken while a provider question is open to attach to that question instead of the thread draft. ChatComposer.tsx registers the eligible question draft and ke…
Out of Scope Changes check ✅ Passed All changes in ChatComposer.tsx, SnapShotCoordinator.tsx, questionAttachments.ts, and the coordinator tests support issue #12271. The shared request attachment-limit check and animation targetin…
  • Fix all pre-merge checks with AI
✨ 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

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