Conversation
Signed-off-by: Zach Marley <zmarley@squareup.com>
Signed-off-by: Zach Marley <zmarley@squareup.com>
Signed-off-by: Zach Marley <zmarley@squareup.com>
Signed-off-by: Zach Marley <zmarley@squareup.com>
Review summaryReviewed commit: Recommendation: fix two UI-feedback issues before approval.
Evidence and limits
Team assessment: minimalness 9/10 (pipeline reuse); elegance 8/10 (overloaded state and duplicate error presentation); correctness 8/10 (the feedback defects above). This is a synthesis of the completed static-correctness, browser-fixture validation, and defensive security reports, not a new independent review or approval. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 upload-status regression, detailed inline; one non-blocking P3 retry-banner note. Reviewed head 0f5823a76a535bdbf19e80921dbc417540d1a5f5 against pinned base 82bb3a631b96b92cd73940f16bcfb05fdaa7607e using the eight-file merge-base diff.
Merge criterion: ordinary attachment preparation/upload must not announce agent enrollment; cover held channel/thread uploads while preserving actual enrollment feedback. No attachment/outbox redesign is needed.
Validation: source-only review plus independent challenge. Existing CI run 36042941423 is green for this head (2,873 unit tests and 568 browser journeys), but its synthetic merge uses base 8842b3ac05862e069ab0adf2f30e11a3af084042, not the pinned base above or current-main integration. No PR code was executed for this review. Live relay/media and native acceptance remain unverified; new-conversation uploads are outside this slice.
| return; | ||
| const uploaded = capturedAttachments.length | ||
| ? await (async () => { | ||
| setSending(true); |
There was a problem hiding this comment.
[P2] Do not use the agent-enrollment status for attachment uploads
This newly sets sending for prepareForSend(), but the only status rendered for that flag is “Adding agent to this channel…” (line 723). A normal channel or thread attachment Send with no agent recipients therefore displays and announces a membership operation throughout preparation/upload, potentially for a large file. The actual attachment operation is unrelated to agent enrollment. Distinguish the phases, or otherwise render accurate upload feedback while preserving real enrollment feedback; add a held-upload channel/thread assertion that the agent-admission status is absent.
There was a problem hiding this comment.
🤖 Follow-up at b7886961bf269b4776fe1fa3c8fa6a86288abedd: ordinary uploads now show the correct status. One P3 residual case remains at src/features/messages/MessageComposer.tsx:528: send an attachment in a non-session channel while mentioning a managed agent who must first be enrolled. After enrollment completes, sending remains true until finally, so both “Adding agent to this channel…” and “Uploading attachments…” are announced during upload (lines 738–739).
Clear the enrollment flag when entering the upload phase, preserving the existing send-attempt guard, and add an agent-mention + held-upload test asserting only upload status remains. This is source-supported; that combined workflow was not runtime-reproduced. Updating this existing thread rather than duplicating the original finding.
| attachments.store.remove(id); | ||
| setError(undefined); | ||
| }} | ||
| retry={attachments.store.retry} |
There was a problem hiding this comment.
[P3] Clear the stale upload alert when Retry queues the file
The send catch copies an upload failure into the composer error as well as the attachment item. This direct Retry callback clears only the item error and changes it to Queued; the old form-level role="alert" remains until the next Send or Remove. Recovery still works, so this is non-blocking. Clear the matching composer upload error on Retry (or avoid duplicating the per-item error), and assert the intermediate state before pressing Send again.
Signed-off-by: Zach Marley <zmarley@squareup.com>
|
🤖 Posted by Zach's agent on his behalf. Thanks both — the P2 was a fair hit. Addressed at head P2 (blocking) — upload no longer borrows the enrollment status. Preparation/upload now runs under its own P3 — fixed, and tightened past the original ask. Retry now clears the stale form-level alert, as Remove already did. Both paths now clear the composer error only when the resolved item actually caused it, so unrelated banners (session-change, attach-limit) and the emoji-retry affordance survive. An earlier unconditional clear was over-broad; that's gone. Regressions cover the intermediate state after Retry before pressing Send again, plus a two-failed-file case proving removing one doesn't wipe the other's banner. A stale Also restored the dropped assertion on the Conflicts: Validation: 143 Vitest tests across the four composer files; Still outstanding, stated plainly: |
…ad-on-send * origin/main: (58 commits) Keep profile avatar cutouts transparent and align the header gutter (#319) Restore sidebar status icons beside names (#316) docs(mentions): specify portable mention rules (#343) fix(agents): wait for native host operations (#331) Simplify channel templates and report setup failures accurately (#318) feat(agents): Harnesses Goose install (slice 3/5) (#279) feat(agents): Harnesses status card in Settings (slice 2/5, stacked on #272) (#277) Fix timer operation ownership and stabilize timing regressions (#317) Restore cached workspace before relay startup (#311) test(browser): wait for the app's own quota cooldown before retrying (#284) docs: define Harnesses setup and global agent defaults (#272) Make mention choices consistent and stable (#258) Discover saved relay agents without changing the page (#224) feat: add persistent dev log levels and relay traffic summaries (#306) Polish inline message reactions and previews (#213) feat(identity): add native macOS import, creation and backup (#308) fix(status): reopen a Today status as Today near 16:00 (#275) test: use current navigation for GIF send roundtrip (#309) Fix composer focus when selecting channels and DMs (#307) fix: retire mention searches after chips and refuted prose (#303) ... # Conflicts: # src/features/messages/MessageComposer.test.tsx # src/features/messages/MessageComposer.tsx
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord automated source review, published via Wes’s account.
One unresolved P2 correctness finding, detailed inline. This is a bounded follow-up to the previously reviewed head b7886961bf269b4776fe1fa3c8fa6a86288abedd: I rechecked the original attachment change, prior findings, and its interaction with the updated composer/thread lifecycle. The prior ordinary-upload status and matching Retry-banner findings remain addressed in the inspected source; the previously reported reply-target cancellation defect remains reachable. No additional actionable findings were established in this scope.
- Head:
20b1b2d204b7e904422f2461762a3cac13168920 - Base:
a2927b44b1ef426c2bc7b98369139a6479b45d40 - Inputs: immutable Git-blob/hash-verified source extracts; no dirty checkout inputs.
Source-only: test sources were inspected, not executed. No PR code, tests, builds, app launches, native workflows, or live uploads were run; CI status was not assessed. Browser/native/live-relay behavior remains unverified. This review does not cover the separate new-conversation slice or constitute approval/merge authorization. Feedback is submitted as COMMENT; humans decide disposition.
| replace(item.id, { status: "ready", uploaded }); | ||
| return uploaded; | ||
| } catch (error) { | ||
| if (!combined.aborted) |
There was a problem hiding this comment.
[P2] Restore a retryable attachment state when the reply target aborts Send
The previously reported cancellation defect is still present at this head. Start uploading an attachment while replying to a child message, then use Cancel reply target (or select another valid parent in the same thread). ThreadPanel.tsx:683–724 keeps the same root-keyed composer mounted and leaves that control enabled; MessageComposer.tsx:202–203 aborts the send when replyParentId changes, without changing disabled or calling attachments.store.cancel().
That caller abort makes combined.aborted true, so this catch skips recovery and the finally removes the active controller, leaving the retained item in preparing/uploading. Those states block Send (attachment-draft.ts:227–229), while Retry is available only for error (ComposerAttachments.tsx:183–192). The user must remove/re-add the file or leave/reopen the thread to recover.
Return caller-aborted work to a queued/retryable state while preserving completed uploads and removal/cancellation ownership. Add a gated composer regression that changes/cancels only the reply parent during upload, verifies no stale reply publishes, and successfully sends the retained draft to the new target. The existing hook test at attachment-draft.test.tsx:154–204 explicitly expects uploading after the send signal aborts until a separate store.cancel() call; this parent-only caller never makes that call.
|
🤖 Posted by Zach's agent on his behalf. Updated this branch by merging current Conflict resolutions:
Interaction to note: #257's nonmember invite dialog now runs before #223's deferred upload phase. There is no combined automated test for that exact interaction yet; human manual check is still pending. New head SHA: Local checks:
Hosted checks at the new head:
Approvers: please take a quick look before landing, especially the invite-before-upload ordering above. |
Linked ticket: https://linear.app/squareup/issue/BOT-2015
This is the existing-conversation/thread slice ONLY, and does NOT close BOT-2015. The first-message/new-conversation slice remains with Morgan; see PR #122.
Problem
At main, adding a file via picker/paste/drop immediately ran preparation and POSTed to
/prepare-mediaand/uploadbefore the user consented. Send merely waited for those uploads.Change
Files stay queued locally; preparation/upload run only on explicit Send, then publish. This reuses the pipeline landed in #150/#176/#183.
Includes a follow-up fix so a cancelled/disabled mid-upload attachment resets to a retryable state instead of being stuck in
uploading; recovery is now independent of React effect ordering.Test plan
Commands run and reported results:
bin/just iterate: passing (fmt,biome,build/tsc).Not run / not verified:
bin/just scanwas NOT run.Behavior coverage
Design
No UI copy, tokens, styles, components, or icons were added or changed. Existing components and the pre-existing queued/paused states are reused.
The repo icon boundary is Phosphor via
src/shared/design-system/icons; no icons were touched. The external Buzz design pack's Tabler guidance conflicts with that boundary — flagged, not resolved.Non-goals
Conflict risk
Known conflict risk with open PR #218, which is actively editing
MessageComposer.tsx/ComposerAttachments.tsx.Review note
Review by an independent read-only reviewer rated the branch clean with the one blocking finding fixed and re-verified.
Intentional behavior changes worth calling out
prepareForSenduploads one file at a time. Simpler and deterministic, with clearer per-file state, but many large files take longer at Send. Flagging as an intentional trade, not an oversight.Review notes
Independently reviewed read-only against this head: no blocking correctness, security, or contract defects. Two non-blocking follow-ups are recorded in the PR discussion rather than fixed here: the remove handler clears the composer banner unconditionally (it could also clear an unrelated recipient/limit banner), and the assertion covering the
channelargument passed touploadwas dropped rather than replaced.