fix(notifications): make action buttons work and keep IDs stable across restarts - #138
Open
renatoaraujoc wants to merge 1 commit into
Open
fix(notifications): make action buttons work and keep IDs stable across restarts#138renatoaraujoc wants to merge 1 commit into
renatoaraujoc wants to merge 1 commit into
Conversation
…ss restarts Fixes sameerasw#136 and sameerasw#137. Plain action buttons (sameerasw#136) WebSocketMessageHandler read the optional "text" field with optString(), which returns "" when the field is absent. performNotificationAction() branched on `replyText != null`, so an absent text was still treated as an inline reply: every plain action button went down the reply path, failed the remoteInputs check and returned false. Any client that omits "text" for non-reply actions could never invoke a button. The guard now tests for a non-empty string, and the handler normalizes an absent field to null. BleTransportBridge also dropped the optional reply text entirely, so inline replies never worked over BLE. It now forwards it, treating empty as "no reply" for consistency with the WebSocket path. Actions after a process restart (sameerasw#137) activeNotifications was only populated from onNotificationPosted, so after a process restart nothing already in the shade was actionable — clients got "not found" for notifications that were still visible. onListenerConnected() now re-registers what is currently posted. Re-registering alone is not enough: generated IDs embed postTime, and during a session the first-seen ID is preserved across updates via keyToId. Once that in-memory map is gone, regenerating from the current postTime produces a different ID than the one the client holds, so its requests would still miss. The key -> id mapping is therefore persisted (sbn.key is stable across updates and restarts) and consulted when re-registering, so IDs a client obtained before the restart keep resolving. Stale entries are pruned against the currently active notifications on each listener connect. Verified on a Galaxy Z Fold7 (Android 16) against the macOS client: invoking a plain button now reaches pendingIntent.send() and the app-side effect happens, and an ID held by the client before a force-stop is restored identically afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Fixes #136 and #137.
Both issues live on the notification action path, and neither is specific to a particular client or transport — anything speaking the protocol hits them.
Plain action buttons never fire (#136)
WebSocketMessageHandlerreads the optionaltextfield withoptString(), which returns""(not null) when the field is absent. That empty string reachedperformNotificationAction(), whose branch wasif (replyText != null), so an absent text still selected the inline reply path: plain buttons failed theremoteInputscheck and returned false. Theelsebranch that callspendingIntent.send()was unreachable for anything arriving over the WebSocket.The guard now tests for a non-empty string, and the handler normalizes an absent field to
null.BleTransportBridgehad a related gap: it parsedidandactionNamebut droppedparts[2], so inline replies never worked over BLE at all. It now forwards the reply text, treating empty as "no reply" so both transports behave the same.Actions and dismissals break after a process restart (#137)
activeNotificationswas only populated fromonNotificationPosted, so once the process restarted, nothing already in the shade was actionable — clients gotnot foundfor notifications that were still on screen.onListenerConnected()now re-registers what is currently posted.Re-registering by itself isn't sufficient, which is worth spelling out since it's not obvious: generated IDs embed
postTime, and within a session the first-seen ID is preserved across updates viakeyToId. Once that in-memory map is gone, regenerating from the currentpostTimeyields a different ID than the one the client is holding, so its requests would still miss. I confirmed this on device before adding the second half of the fix.So the
key -> idmapping is persisted (sbn.keyis stable across updates and restarts) and consulted when re-registering, which makes IDs obtained before a restart keep resolving. Stale entries are pruned against the currently active notifications on each listener connect, so the store doesn't grow unbounded.Verification
Tested on a Galaxy Z Fold7 (Android 16) against the macOS client, on a build from this branch:
pendingIntent.send()— logcat showsInvoked action '<name>'instead ofdoes not support reply, and the app-side effect actually happens (media notification action changed the track; "Mark as read" on a mail notification cleared it).force-stopis restored identically afterwards, instead of being regenerated with a newerpostTime.I left the ID format alone on purpose. Deriving IDs from
sbn.keyinstead of embeddingpostTimewould make this inherently stable and drop the need for persistence, but that's protocol-visible and felt like your call rather than something to slip into a bug fix.