Skip to content

Gate draft image ownership checks by local attachments - #32098

Merged
AmethystLiang merged 1 commit into
mainfrom
perf-draft-image-demand-gate
Oct 22, 2026
Merged

AmethystLiang merged 1 commit into
mainfrom
perf-draft-image-demand-gate

Conversation

@AmethystLiang

@AmethystLiang AmethystLiang commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​211 $\color{#cf222e}{\Huge{\mathbf{−}}}$​146 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​65
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​127 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​121

Problem

Restoring a draft could repeatedly resolve image ownership during unrelated status updates, even when no local stored attachment needed checking.

Solution

Only resolve local ownership when a structured draft includes a relevant local attachment-store image. Preserve retries when the draft returns to local ownership and ignore stale checks from a previous host.

ELI5

Draft images now check whether they still exist only when that check is needed. This avoids extra background work while still preventing missing local images from being sent.

What Changed

  • Gate local draft-owner resolution on structured drafts containing local attachment-store paths.
  • Keep ownership checks disabled for ordinary paths, paste paths, SSH attachments, terminal scopes, and drafts without images.
  • Add coverage for unrelated status publications, host changes during pending checks, and mounted/unmounted rechecks.
  • Extract the attachment hook test probe into a reusable test file.

Why

This reduces unnecessary work caused by unrelated app status updates while retaining correct missing-image handling for local restored attachments.

Linked Issue

If you do not have one and are an outside contributors, your PR wiil be ignored. Refs is not sufficient. Link an actual issue

Fixes #

Visual Proof

N/A — this changes background ownership-check scheduling and restored attachment validation without a visual UI change.

Testing

  • Added/updated tests in use-native-chat-composer-attachments.test.tsx covering demand-gated checks, host changes, and rechecks.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No known issues in security, cross-platform support (Linux, Windows, macOS), Remote SSH, mobile, backwards compatibility, or performance. The change reduces unnecessary ownership-resolution work.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d00e2ef7-c18f-4ba0-8eb5-ffd978096f3d
📥 Commits

Reviewing files that changed from the base of the PR and between e710e89 and fe31ebe.

📒 Files selected for processing: 3
  • src/renderer/src/components/native-chat/native-chat-composer-attachments-test-probe.tsx
  • src/renderer/src/components/native-chat/native-chat-composer-draft-image-check.ts
  • src/renderer/src/components/native-chat/use-native-chat-composer-attachments.test.tsx

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

📝 Walkthrough

The attachment hook now grants local-store authority only when its active checks include an eligible unconnected agent-session attachment-store image, while retaining the existing scope and owner conditions. Tests use a shared React probe and cover validation across attachment types, host changes, unrelated status updates, and upload sweeps with the composer mounted or unmounted.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to fe31e

The ownership gate preserves restored-image checks and retries while avoiding checks when there is no eligible pending image. No identified issue blocks merging after normal checks.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description explains the problem, solution, scope, testing changes, and lack of visual impact. It does not provide the required linked issue, and it does not report lint, typecheck, full test, or … Add a valid issue link after "Fixes #". Report the results of pnpm lint, pnpm typecheck, pnpm test, and pnpm build, or state that CI will cover them. Complete the self-review checklist when applicable.
Docstring Coverage Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: gating draft image ownership checks by local attachments.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, solution, scope, testing changes, and lack of visual impact. It does not provide the required linked issue, and it does not report lint, typecheck, full test, or build results or confirm CI coverage.

Resolution

Add a valid issue link after "Fixes #". Report the results of pnpm lint, pnpm typecheck, pnpm test, and pnpm build, or state that CI will cover them. Complete the self-review checklist when applicable.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

@AmethystLiang
AmethystLiang merged commit e5df152 into main Oct 22, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant