fix(chat): scope the send-button spring to the button - #741
Merged
Conversation
Sending sometimes left the sent text in the field, so the next message had to be erased before it could be written. The send-button spring was on the whole composer row rather than on the button. Tapping Send flips `showsSubmit` true to false on the same update that sets `draft` to empty, so the field's text update ran as an animated one against its backing text view, where it can be coalesced away. The binding reads empty either way, and an unchanged binding never pushes again, so a missed update is permanent. Whether it is missed depends on the field's pending-edit state at the tap, which is why it only happened sometimes. Scoping the spring to the button keeps the field out of that transaction. The button still pops in and out: the `.animation(_:value:)` now sits on the `Group` wrapping the conditional, which is what supplies its transition's transaction. `ComposerModel` is untouched. `clear()` already emptied the draft synchronously before the send was dispatched, and its tests already covered that.
Three UI tests around the send path: a run of plain sends, a reply, and a send made while the field is mid-edit. Each reads the field back after the send and fails if the sent text is still there. The assertion reports which side of the binding is stale. The send button is drawn from the same emptiness the field is, so a button that has gone away while the text remains means the draft emptied and the text view never caught up, rather than the send never firing. Sends are dispatched without waiting for the button to settle, and delivery is not awaited between them: the gap between the last keystroke and the tap is the window the failure was thought to live in, and waiting anywhere in it settles it. The tests pass against the pre-fix ConversationBottomBar, so they do not reproduce the reported failure and do not stand as evidence for the fix in ec986a5. A control that types without sending does fail, so the assertion reads the field rather than passing vacuously.
bmc08gt
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The send-button spring was applied to the whole composer row rather than to the button that needed it. Only the button has a
.transition, and a transition takes its animation from the transaction its container supplies — so the modifier belongs on a wrapper around the conditional, not on the row that also holds the text field.Scoping it there keeps the field out of that transaction. The button still pops in and out: the
.animation(_:value:)now sits on theGroupwrapping the conditional.This started as a candidate explanation for the composer sometimes keeping the sent text after a send. Tapping Send flips
showsSubmiton the same update that emptiesdraft, so with the row-wide spring the field's text update ran as an animated one against its backing text view, where it can be coalesced away; the binding reads empty either way, and an unchanged binding never pushes again, which would make a missed update permanent rather than late. That mechanism is plausible but unconfirmed, and this change should not be read as fixing the reported behaviour.ComposerModelis untouched.clear()already emptied the draft synchronously before the send was dispatched,ComposerModelTestsalready covered it, and nothing writes the draft back:draftDidChangeonly broadcasts typing state, and there is no draft persistence.The UI tests are regression cover for the send path, which had none. They send a run of messages, a reply, and a message typed mid-edit, reading the field back after each send. The assertion names which side of the binding is stale — the send button is drawn from the same emptiness the field is, so a button that has gone away while the text remains means the draft emptied and the text view never caught up. They pass against the composer as it stood before this change, so they do not stand as evidence for it.