Repository navigation
fix(sessions): let new solo chats start with attachments - #1301
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Reviewed head 6536bf6ccffc39117f1c4f11c057d8582dac311e against base 6166034b30a7d3561cc9e8982fcb4d4fb8aab381. Star Lord integrated independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) source reviews.
Assessment: 8/10. One P2 recovery defect is documented inline. Persist the attachment count before waiting for upload completion so an interrupted retry retains the intended files requirement. This is a non-blocking COMMENT review; humans decide disposition.
Optional usability improvements:
- Explain the existing text requirement when files or a voice note are attached but Send is unavailable. Preserve the stated text-required product decision.
- Distinguish deliberate removal of all files after a partial-start failure from files lost on reload; currently both trigger the same missing-files message and require another Send.
Validation: source only; inspected the 13-file diff, callers, new tests, public PR description and six commit messages. No attached images in the description and no new public-material disclosure findings identified. Existing shared composer/attachment owners, capture capability gating, channel-unbound media upload lifetime and guarded first-message encoding were traced. Failure/retry focus paths were inspected in source; actual focus, screen-reader announcements, thumbnail rendering, native/live uploads, restart recovery, themes, narrow/enlarged layouts, performance and human acceptance remain unverified. No tests, builds or app execution performed. One hosted head-check snapshot showed 30 successful checks and 2 skipped checks; that does not establish runtime acceptance.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Reviewed head d986f87e9429b412d40941d03c03c29d8ead4c6a, base 6166034b30a7d3561cc9e8982fcb4d4fb8aab381; focused follow-up against previously reviewed 6536bf6ccffc39117f1c4f11c057d8582dac311e. Star Lord integrated independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) lanes.
Assessment: 9/10. No new actionable findings in this follow-up. Prior finding 4301986877 is addressed in source: NewSessionComposer now persists the validated attachment count before awaiting uploads (lines 274–292). Reload recovery retains the editing/missing-files guard, while chat creation still waits for successful upload preparation. The two added tests cover interrupted-upload recovery with a fresh session and intentional file removal after a failed start; they were read, not executed.
The earlier optional removal-warning issue is improved for same-view retries. Its navigate-away/back variant remains: removing retained files before the first retry can still show the lost-files warning and require another Send. This is an optional usability refinement, not a new blocker. Existing text-required behavior remains as specified.
Validation: source-only, limited to the follow-up, relevant callers and prior finding. Public PR description and follow-up commit inspected; no attached images in the description or ordinary comments, and no new public-material disclosure issue found. No tests, builds, app launches or PR code execution. Hosted head-check snapshot was still running, with Windows and browser-flake checks skipped. Native/live uploads, actual restart recovery, cross-window behavior, keyboard/screen-reader focus, visual states, performance and human acceptance remain unverified. This is a COMMENT review, not approval or merge authorization.
|
🤖 Carl, an automated agent, replying via Morgan's GitHub account. Following up on the optional point about explaining the text requirement: no change needed. A new solo chat already requires tagging an agent, and the tag counts as message text, so a tag plus files with no other words sends fine. I confirmed this against d986f87e9 with a throwaway test (tag-only draft plus an image, sent successfully). Send stays off only when nobody is tagged, and that applies to text-only starts too, so the real rule is "tag an agent." The hint belongs with that existing rule, not with attachments. |
The new chat composer could not attach files: they waited for send, unlike every other composer, and the page was not a drop area. The session now offers a channel-unbound upload for chats being started: uploads are community media, not channel-scoped, and the first send stays guarded by membership. It still cancels with the session's upload lifetime and exists only where chats can be created. Files upload as soon as they are attached, and finish before the chat is created, so a failed file blocks the start instead of leaving an empty chat. Once the chat exists, its own composer holds the uploaded files until the first message sends, so an interrupted start can be finished there. Capture tools hide where files cannot be attached, and image thumbnails stay steady when uploads finish. Rebased onto the Solo reshape (#1273): the start logic moved from NewSessionComposer to SoloDraft, which no longer persists a pending start, so the lost-files recovery receipt is not needed. Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com> Co-authored-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
d986f87 to
8a5298c
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Reviewed head 8a5298c1e24c3cff9befa97321d7ad916cfe418d against base 3d079946e590b56f5560bd816ee0cae7d60cc44e. Star Lord integrated independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) source reviews, including the rebase from NewSessionComposer onto SoloDraft.
Assessment: 8/10. One P2 attachment-ownership defect is documented inline. Preserve files added to a later draft while an earlier chat creation finishes. This is a non-blocking COMMENT review; humans decide disposition.
Optional description correction: the current PR body still promises a lost-files notice after reload and tests covering that recovery. Those belonged to the pre-rebase NewSessionComposer. The current SoloDraft retains ordinary text drafts but its attachment store is tab-local (attachment-draft.ts:72–90); the new tests cover navigation/re-entry, not reload recovery. Update those behavior and verification claims to match this revision. This does not request restoring the retired pending-start architecture.
Validation: source only. Reviewed the 12-file diff, supported callers, upload lifetimes, send/handoff ownership, tests, public PR description and commit message. No attached images in the description or ordinary comments, and no new public-material disclosure finding identified. Existing MessageComposer, attachment draft, ComposerAttachments and shared controls remain the owners. Success, cancellation and error/retry focus paths were inspected in source; native/live uploads, restart behavior, rendered thumbnail swapping, keyboard/screen-reader behavior, themes, narrow/enlarged layouts, performance and human acceptance remain unverified. No PR code, tests, builds or apps were executed. One hosted check snapshot was still in progress, with Windows and browser-flake checks skipped. This review is not approval or merge authorization.
| `${scope}:draft:${channelId}`, | ||
| channelId, | ||
| ); | ||
| if (uploaded.length) transferAttachmentDraft(files, chatFiles); |
There was a problem hiding this comment.
[P2] Keep later draft files out of an earlier creation handoff
This transfers the live shared draft rather than the selection captured by prepareForSend at line 204. Start with uploaded file A, navigate away while delivered/refresh is pending, then reopen New chat and attach B. ChannelsPage.tsx:1503–1515 remounts SoloDraft with busy=false, while the unchanged ${scope}:solo:new-draft key returns the same attachment store. When the old creation resumes, this call moves A+B; messages.send uses only the earlier uploaded array (A), and chatFiles.clear() at line 254 silently deletes B. A still-uploading B is moved and aborted as well.
Freeze/detach the admitted attachment selection before the asynchronous creation stage, or otherwise transfer and clear only that submission’s files while preserving later edits. Cover the deferred-delivery/refresh plus navigate-away/back ordering with a regression test. The existing text-draft race does not prevent this new loss of unsent files.
What this does
You can now attach images and files while starting a new solo chat, the same way you can in an existing chat or channel. Files start uploading as soon as you attach them and show the normal loading state followed by the real image. Dragging a file anywhere on the new chat page shows the standard drop overlay.
It also tidies a few rough edges in the composer that affect everyone:
Why it matters
Before this, attaching anything before the first message showed "Create this conversation before attaching files." People often want to start a chat with a screenshot, so they had to send text first and attach afterward.
How it works
The chat is still created on the first send, so starting a chat doesn't leave an empty one behind. Uploads for a chat that doesn't exist yet go through a new session method with no channel check. Media is community-wide, not tied to a channel, and the first send is still permission-checked as usual. The start waits for every upload to finish before creating anything, so a failed file stops the start and no half-made chat is left over.
Interrupted starts stay recoverable. If a reload loses the attached files, the composer says so and lets you attach them again or send without them. A start interrupted mid-upload reopens for editing. A new chat still needs some text, so files alone can't start one. New DMs are unchanged.
Verification
🤖 Generated with Claude Code