From c45e9bb777cb009e15773af856f861391e430492 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Fri, 18 Sep 2026 09:11:29 -0400 Subject: [PATCH] fix(chat): fall back to the iOS edit and delete windows when the server sends none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `message_edit_window` and `message_delete_window` are message-typed, so the server can leave them unset, and it does. Android read that as no limit and kept Edit and Delete on the menu forever; iOS substitutes 15 minutes and 48 hours. The same message therefore offered different rows depending on the platform. Match iOS. `MessagePolicy.fromFlags` substitutes `FallbackEditWindow` / `FallbackDeleteWindow` for anything the flags leave unset, and `Default` — the policy in force before flags are read — is now those two windows rather than unbounded. Every upstream state collapses to the same input: flags not fetched, a failed fetch, and unset window fields all arrive as null and all get the fallback. The cost is that a message old enough can lose Edit or Delete where the server would have taken the request. That is the trade iOS already made, and the worse failure is the opposite one: an affordance the server answers `CANNOT_EDIT` or `CANNOT_DELETE`. The values are a product choice, not one the contract supplies — the proto documents what the fields mean but never what an absent field implies. Nothing enforces the match with iOS, so a test on each side asserts the literal values and names its counterpart. The constructor no longer defaults either window. Absence decides whether the fallback applies, so it is worth stating at the call site. --- .../app/messenger/internal/ChatViewModel.kt | 4 +- .../internal/ChatMessageActionReducerTest.kt | 51 ++++++++------ .../flipcash/shared/chat/MessageCapability.kt | 54 +++++++++++++-- .../shared/chat/MessageCapabilityTest.kt | 68 +++++++++++++++++-- 4 files changed, 143 insertions(+), 34 deletions(-) diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt index 50f63dab98..79765dfa8e 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt @@ -869,10 +869,10 @@ internal class ChatViewModel @Inject constructor( private fun initChatHandlers() { // Ahead of the transcript, so the first mapping already has the real windows rather than - // the defaults, which leave both edit and delete open. + // the fallbacks the default carries. userFlags.resolvedFlags .map { - MessagePolicy( + MessagePolicy.fromFlags( editWindow = it.messageEditWindow.effectiveValue, deleteWindow = it.messageDeleteWindow.effectiveValue, ) diff --git a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt index 36cb1294bb..790879ae2b 100644 --- a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt +++ b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatMessageActionReducerTest.kt @@ -27,6 +27,15 @@ class ChatMessageActionReducerTest { private val sentAt = Instant.fromEpochSeconds(1_000) + /** + * Both windows open. `MessagePolicy.Default` carries the fallback windows, and these bubbles + * are timestamped in 1970, so the default would strip Edit and Delete from every selection + * before the assertion could tell whether the reducer stored the right bubble. The window rules + * themselves are covered in `MessageCapabilityTest`; the two cases below that do exercise a + * window state it directly. + */ + private val unbounded = MessagePolicy(editWindow = null, deleteWindow = null) + private fun bubble( messageId: Long, text: String = "hello", @@ -54,7 +63,7 @@ class ChatMessageActionReducerTest { val target = bubble(1) val state = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(target), ) @@ -68,7 +77,7 @@ class ChatMessageActionReducerTest { // window that has since closed. Offering Edit anyway costs a round-trip the server answers // CANNOT_EDIT. sentAt is decades old, so any finite window here has closed. val state = reduce( - ChatViewModel.State(messagePolicy = MessagePolicy(editWindow = 5.minutes)), + ChatViewModel.State(messagePolicy = MessagePolicy(editWindow = 5.minutes, deleteWindow = null)), ChatViewModel.Event.ToggleMessageSelection(bubble(1)), ) @@ -82,7 +91,7 @@ class ChatMessageActionReducerTest { fun `selecting a bubble past its delete window drops Delete`() { // Separate from the case above so a window wired to the wrong capability cannot pass both. val state = reduce( - ChatViewModel.State(messagePolicy = MessagePolicy(deleteWindow = 5.minutes)), + ChatViewModel.State(messagePolicy = MessagePolicy(editWindow = null, deleteWindow = 5.minutes)), ChatViewModel.Event.ToggleMessageSelection(bubble(1)), ) @@ -96,7 +105,7 @@ class ChatMessageActionReducerTest { fun `long-pressing the selected bubble again clears the bar`() { val target = bubble(1) val selected = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(target), ) @@ -111,7 +120,7 @@ class ChatMessageActionReducerTest { val first = bubble(1) val second = bubble(2, text = "goodbye") val selected = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(first), ) @@ -123,7 +132,7 @@ class ChatMessageActionReducerTest { @Test fun `copying clears the bar`() { val selected = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(bubble(1)), ) @@ -138,7 +147,7 @@ class ChatMessageActionReducerTest { // focus goes because the sheet is modal — a sharp bubble behind it reads as still live. val target = bubble(1) val selected = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(target), ) @@ -153,7 +162,7 @@ class ChatMessageActionReducerTest { // Confirmed or cancelled, the sheet's close is ClearMessageSelection, which the handler // drives. Leaving confirmingDelete set would hold the whole transcript behind the backdrop. val confirming = reduce( - reduce(ChatViewModel.State(), ChatViewModel.Event.ToggleMessageSelection(bubble(1))), + reduce(ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(bubble(1))), ChatViewModel.Event.DeleteMessage(1), ) @@ -167,7 +176,7 @@ class ChatMessageActionReducerTest { fun `starting an edit takes over the composer and stashes the draft`() { val target = bubble(1) val selected = reduce( - ChatViewModel.State(chatInputState = TextFieldState("half-written")), + ChatViewModel.State(messagePolicy = unbounded, chatInputState = TextFieldState("half-written")), ChatViewModel.Event.ToggleMessageSelection(target), ) @@ -186,7 +195,7 @@ class ChatMessageActionReducerTest { @Test fun `editing a second message keeps the original draft rather than the first edit's text`() { val first = reduce( - ChatViewModel.State(chatInputState = TextFieldState("half-written")), + ChatViewModel.State(messagePolicy = unbounded, chatInputState = TextFieldState("half-written")), ChatViewModel.Event.EditMessage(1, "hello"), ) // The composer now holds the first message's body, which is not the user's draft. @@ -204,7 +213,7 @@ class ChatMessageActionReducerTest { fun `ending an edit releases the composer`() { // Confirm, cancel and back all land here; restoring the stashed draft is the handler's job. val editing = reduce( - ChatViewModel.State(chatInputState = TextFieldState("half-written")), + ChatViewModel.State(messagePolicy = unbounded, chatInputState = TextFieldState("half-written")), ChatViewModel.Event.EditMessage(1, "hello"), ) @@ -216,7 +225,7 @@ class ChatMessageActionReducerTest { @Test fun `submitting and cancelling leave the edit in place for the handler to read`() { val editing = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.EditMessage(1, "hello"), ) @@ -236,7 +245,7 @@ class ChatMessageActionReducerTest { fun `replying opens the strip and clears the selection`() { val target = bubble(1) val selected = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ToggleMessageSelection(target), ) @@ -254,7 +263,7 @@ class ChatMessageActionReducerTest { @Test fun `replying leaves the draft in the composer`() { val state = reduce( - ChatViewModel.State(chatInputState = TextFieldState("half-written")), + ChatViewModel.State(messagePolicy = unbounded, chatInputState = TextFieldState("half-written")), ChatViewModel.Event.ReplyToMessage(quote()), ) @@ -264,7 +273,7 @@ class ChatMessageActionReducerTest { @Test fun `starting an edit takes the reply strip down`() { val replying = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ReplyToMessage(quote()), ) @@ -277,7 +286,7 @@ class ChatMessageActionReducerTest { @Test fun `replying takes an edit down`() { val editing = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.EditMessage(1, "hello"), ) @@ -290,7 +299,7 @@ class ChatMessageActionReducerTest { @Test fun `cancelling the reply keeps the draft`() { val replying = reduce( - ChatViewModel.State(chatInputState = TextFieldState("half-written")), + ChatViewModel.State(messagePolicy = unbounded, chatInputState = TextFieldState("half-written")), ChatViewModel.Event.ReplyToMessage(quote()), ) @@ -308,7 +317,7 @@ class ChatMessageActionReducerTest { @Test fun `sending leaves the reply in place for the handler to read`() { val replying = reduce( - ChatViewModel.State(), + ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.ReplyToMessage(quote()), ) @@ -323,14 +332,14 @@ class ChatMessageActionReducerTest { */ @Test fun `a jump request alone sets no target`() { - val state = reduce(ChatViewModel.State(), ChatViewModel.Event.JumpToMessage(7)) + val state = reduce(ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.JumpToMessage(7)) assertNull(state.jumpTarget) } @Test fun `a resolved jump carries the target and its bound`() { - val state = reduce(ChatViewModel.State(), ChatViewModel.Event.JumpResolved(7, 240)) + val state = reduce(ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.JumpResolved(7, 240)) assertEquals(7L, state.jumpTarget) assertEquals(240, state.jumpBudget) @@ -338,7 +347,7 @@ class ChatMessageActionReducerTest { @Test fun `consuming a jump clears both`() { - val jumping = reduce(ChatViewModel.State(), ChatViewModel.Event.JumpResolved(7, 240)) + val jumping = reduce(ChatViewModel.State(messagePolicy = unbounded), ChatViewModel.Event.JumpResolved(7, 240)) val state = reduce(jumping, ChatViewModel.Event.JumpConsumed) diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/MessageCapability.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/MessageCapability.kt index 8265cd96f8..34d91cd236 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/MessageCapability.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/MessageCapability.kt @@ -4,6 +4,8 @@ import com.flipcash.services.models.chat.ChatMessage import com.flipcash.services.models.chat.MessageContent import kotlin.time.Clock import kotlin.time.Duration +import kotlin.time.Duration.Companion.hours +import kotlin.time.Duration.Companion.minutes import kotlin.time.Instant /** @@ -30,19 +32,59 @@ enum class MessageCapability { * Client-side limits on what may be done to a message. * * Both windows come from `UserFlags` (`message_edit_window`, `message_delete_window`), which sends - * them with explicit presence: an unset field is no limit rather than a zero-length one. The - * defaults are therefore both `null`, which leaves `CANNOT_EDIT` / `CANNOT_DELETE` as the - * authority for a build that has not seen the flags yet. + * them with explicit presence: an unset field is distinguishable from a zero-length one. Where the + * server sends nothing, [fromFlags] substitutes [FallbackEditWindow] / [FallbackDeleteWindow] + * rather than leaving the action open forever, so a message old enough can lose Edit or Delete + * even where the server would have taken the request. That is the accepted cost: an affordance the + * server answers `CANNOT_EDIT` / `CANNOT_DELETE` is the worse failure. + * + * Neither window has a default here. Absence is a real input — it decides whether the fallback + * applies — so it is worth stating at the call site rather than inheriting. * * @param editWindow how long after sending a message stays editable, or `null` for no limit. * @param deleteWindow how long after sending a message stays deletable, or `null` for no limit. */ data class MessagePolicy( - val editWindow: Duration? = null, - val deleteWindow: Duration? = null, + val editWindow: Duration?, + val deleteWindow: Duration?, ) { companion object { - val Default = MessagePolicy() + /** + * The window applied when the server sends no edit window. + * + * Maintained in parallel with iOS `MessagePolicy.fallbackEditWindow` + * (`FlipcashCore/Sources/FlipcashCore/Models/Conversation/MessagePolicy.swift`). The two + * must move together or the clients offer different rows for the same message; nothing + * enforces it, so changing one means changing the other in the same release. + * + * The value is a product choice, not a figure the contract supplies: `message_edit_window` + * documents what it means but never what an absent field implies. Replace it the moment the + * server does specify one. + */ + val FallbackEditWindow = 15.minutes + + /** + * The window applied when the server sends no delete window. Same parallel-maintenance duty + * and same provenance as [FallbackEditWindow]; iOS holds it as + * `MessagePolicy.fallbackDeleteWindow`. + */ + val FallbackDeleteWindow = 48.hours + + /** + * Builds the policy in force from the windows the server sent, substituting the fallbacks + * for anything it left unset. + * + * Both arguments are nullable because every upstream state collapses to the same one: + * flags not yet fetched, a fetch that failed, and flags whose window fields are unset all + * arrive as `null` and all get the fallback. There is no second path to keep in step. + */ + fun fromFlags(editWindow: Duration?, deleteWindow: Duration?) = MessagePolicy( + editWindow = editWindow ?: FallbackEditWindow, + deleteWindow = deleteWindow ?: FallbackDeleteWindow, + ) + + /** The policy in force before any flags have been read: the fallback windows. */ + val Default = fromFlags(editWindow = null, deleteWindow = null) } } diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessageCapabilityTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessageCapabilityTest.kt index 1cc790b0e4..f8e80af6d6 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessageCapabilityTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessageCapabilityTest.kt @@ -9,6 +9,7 @@ import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner import kotlin.test.assertEquals import kotlin.time.Duration.Companion.days +import kotlin.time.Duration.Companion.hours import kotlin.time.Duration.Companion.minutes import kotlin.time.Instant @@ -53,6 +54,8 @@ class MessageCapabilityTest { isFromSelf, ) + // Resolved at the instant it was sent, so the default policy's fallback windows are both open + // and this stays a statement about content rather than about age. @Test fun `own text message is copyable, editable and deletable`() { assertEquals( @@ -62,7 +65,7 @@ class MessageCapabilityTest { MessageCapability.Edit, MessageCapability.Delete, ), - resolveCapabilities(text()), + resolveCapabilities(text(), now = sentAt), ) } @@ -116,13 +119,13 @@ class MessageCapabilityTest { MessageCapability.Edit, MessageCapability.Delete, ), - resolveCapabilities(reply), + resolveCapabilities(reply, now = sentAt), ) } @Test fun `an edit window drops Edit once it lapses and leaves Delete alone`() { - val policy = MessagePolicy(editWindow = 15.minutes) + val policy = MessagePolicy(editWindow = 15.minutes, deleteWindow = null) assertEquals( setOf( @@ -142,7 +145,7 @@ class MessageCapabilityTest { @Test fun `a delete window drops Delete once it lapses and leaves Edit alone`() { - val policy = MessagePolicy(deleteWindow = 60.minutes) + val policy = MessagePolicy(editWindow = null, deleteWindow = 60.minutes) assertEquals( setOf( @@ -177,6 +180,8 @@ class MessageCapabilityTest { @Test fun `an unset window leaves its capability open`() { + val unbounded = MessagePolicy(editWindow = null, deleteWindow = null) + assertEquals( setOf( MessageCapability.Copy, @@ -184,10 +189,63 @@ class MessageCapabilityTest { MessageCapability.Edit, MessageCapability.Delete, ), - resolveCapabilities(text(), MessagePolicy.Default, now = sentAt + 365.days), + resolveCapabilities(text(), unbounded, now = sentAt + 365.days), ) } + @Test + fun `windows the server did not send fall back rather than staying open`() { + val policy = MessagePolicy.fromFlags(editWindow = null, deleteWindow = null) + + assertEquals(MessagePolicy.FallbackEditWindow, policy.editWindow) + assertEquals(MessagePolicy.FallbackDeleteWindow, policy.deleteWindow) + assertEquals(policy, MessagePolicy.Default) + + assertEquals( + setOf(MessageCapability.Copy, MessageCapability.Reply, MessageCapability.Delete), + resolveCapabilities(text(), policy, now = sentAt + 30.minutes), + ) + assertEquals( + setOf(MessageCapability.Copy, MessageCapability.Reply), + resolveCapabilities(text(), policy, now = sentAt + 365.days), + ) + } + + /** + * The fallback covers only what the server left unset, so a window it did send has to survive + * the substitution — including one longer than the fallback, which is where a `?:` on the wrong + * side of the expression would show up. + */ + @Test + fun `windows the server did send are used as sent`() { + val policy = MessagePolicy.fromFlags(editWindow = 90.minutes, deleteWindow = null) + + assertEquals(90.minutes, policy.editWindow) + assertEquals(MessagePolicy.FallbackDeleteWindow, policy.deleteWindow) + + assertEquals( + setOf( + MessageCapability.Copy, + MessageCapability.Reply, + MessageCapability.Edit, + MessageCapability.Delete, + ), + resolveCapabilities(text(), policy, now = sentAt + 60.minutes), + ) + } + + /** + * Both numbers are maintained by hand against iOS `MessagePolicy.fallbackEditWindow` / + * `fallbackDeleteWindow`. Nothing checks the two repos against each other, so this pins the + * Android side: a change here fails until someone states the new value, which is the prompt to + * go and change iOS too. + */ + @Test + fun `the fallback windows are the values iOS carries`() { + assertEquals(15.minutes, MessagePolicy.FallbackEditWindow) + assertEquals(48.hours, MessagePolicy.FallbackDeleteWindow) + } + /** * The transcript resolves once, when it is mapped; the menu re-applies the windows when it * opens. Both go through the same rule, so a set narrowed after the fact matches what the