Repository navigation
fix(chat): persist the viewer's EditChat grant - #1526
Merged
Merged
Conversation
ViewerState.permissions.canEdit is mapped from the proto into the domain model but never reached the database. ChatEntityMapper wrote mute and version only, and rebuilt ViewerState without permissions, so the grant came back false on every read. Screens take viewer state from the DB-backed observeMetadata rather than the fetch that produced it, so the server's answer was already gone by the time anything read it. The same function collapsed a chat to a null ViewerState when it held no mute at version 0, which discarded the grant for any chat nobody had muted. Adds can_edit to chat_metadata (schema 37), carries it through the mapper, the DAO and ChatMetadataDataSource, and counts it in the emptiness test. Migration36To37 zeroes viewer_state_version. The column arrives as 0 for every existing row, and updateViewerStateIfNewer only writes when the incoming version beats the stored one; a chat whose viewer state had not otherwise changed would report the version it already reported, lose the gate, and hold the grant at 0 until something unrelated moved it. Nothing is lost by re-arming, because every column that gate protects is server-owned and re-sent by the next fetch.
The server bumps ViewerState.version only when the viewer's state changes, so a chat nobody has ever muted stays at version zero indefinitely. The gate on updateViewerStateIfNewer is a strict >, which made 0 > 0 false and rejected every payload such a chat ever sends. That was invisible while the gate only guarded mute columns, because the rejected payload said nothing new about them. It stopped being invisible once canEdit rode the same gate: the grant arrives on those zero-versioned payloads too, so it could never be stored. On a test account the server returns canEdit=true for a group at version zero and the edit affordance stayed hidden on it, while a group at version 7 showed it. A stored zero means nothing has ever been applied to the row, so anything the server sends beats it. Ordering is lost for that first write, but two payloads at version zero describe the same unchanged state; the strict gate resumes as soon as a non-zero version lands. Rows that predate this all sit at zero already, because the 36-to-37 migration rearmed them, so they pick the grant up on the next fetch without another migration.
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.
ViewerState.permissions.canEditarrived in #1524 and is mapped from the proto correctly, but it never reached the database. Anything gated on it seesfalse, always.ChatEntityMapperwrotemuteandversiononly, and rebuiltViewerStatewithoutpermissions, so the grant came back at itsfalsedefault on every read. That would be survivable if screens read the fetch that produced it, but they don't —ChatViewModelobservesobserveMetadata, which is DB-backed. The server's answer was already gone by the time anything looked.The same function had a second problem:
A chat with no mute at version 0 collapsed to a null
ViewerStateoutright, discarding the grant for exactly the common case — a group nobody has muted. Adding a column without fixing this would not have been enough.The change
can_editonchat_metadata(schema 37), carried throughChatEntityMapper,ChatMetadataDaoandChatMetadataDataSource, and counted in the emptiness test above.clearViewerStatedrops it with the rest.updateViewerStateIfNewertakescanEditas a required parameter rather than a defaulted one. A default would deny silently on any caller that forgot, which is the failure this PR exists to fix.Why the migration re-arms the version
Migration36To37zeroesviewer_state_version. The column arrives as 0 for every existing row, and the gate only writes when the incoming version beats the stored one. A chat whose viewer state hasn't otherwise changed reports the version it already reported, loses the gate, and holds the grant at 0 until something unrelated moves it — so without the re-arm the fix would appear to work on new chats and fail on every chat that predates the upgrade.Nothing is lost by re-arming: every column that gate protects is server-owned and re-sent by the next fetch. The cost is a window between migration and that fetch in which an out-of-order stream event could apply; it closes at the first feed load, before the stream opens.
I considered relaxing the gate to
>=instead, which would also let an equal-version fetch fill the column. I left it alone — the>is deliberate and documented on the DAO, and it governs mute as well as this.Notes
Found while building the group-chat Edit UI (#1525), where the overflow item is gated on this field and so never rendered. The two PRs touch disjoint files and can land in either order; #1525 stays inert until this one merges.