diff --git a/plans/2026-04-06-channel-comments.md b/plans/2026-04-06-channel-comments.md index bdd43db709..b60f478103 100644 --- a/plans/2026-04-06-channel-comments.md +++ b/plans/2026-04-06-channel-comments.md @@ -6,11 +6,11 @@ SimpleX Chat introduced channels in PR #6382 (the chat-relays MVP): groups where Channels need Telegram-style **comments**: under each channel post, a tappable affordance shows a count and opens a separate Comments view containing a flat thread of replies, with its own composer. Subscribers can comment, including quoting the parent post and quoting other comments. -This revision of the plan reflects three architectural decisions taken during plan review and replaces the earlier design that used `MSChannelMsg` / `GCSChannelMsg` / a sheet-based UI: +Three architectural choices shape the rest of the plan: -1. **Wire format.** `ExtMsgContent` and `MsgContainer` are merged into a single record `MsgContainer` whose shape matches the wire JSON 1:1. The dead `MCComment` constructor is replaced with a `parent :: Maybe MsgRef` field on the merged record. A single message can now carry both a `quote` (referencing any prior message) and a `parent` (referencing the channel post being commented on). The new `MSChannelMsg` `MsgScope` variant proposed in the previous draft is **dropped** — it is no longer needed because the parent reference now lives in the message body, not in the scope. +1. **Wire format.** `ExtMsgContent` and `MsgContainer` are merged into a single record `MsgContainer` whose shape matches the wire JSON 1:1. The dead `MCComment` constructor is replaced with a `parent :: Maybe MsgRef` field on the merged record. A single message can now carry both a `quote` (referencing any prior message) and a `parent` (referencing the channel post being commented on). The parent reference lives in the message body, so no new `MsgScope` variant is required. -2. **Local model.** Comments are NOT a `GroupChatScope` variant. A new `ChannelMsgInfo` type is added as a separate field on `ChatInfo.GroupChat` and as a separate parameter to pagination, parallel to `Maybe GroupChatScopeInfo`. This keeps the Comments view as a distinct concept rather than overloading the scope mechanism. +2. **Local model.** Comments are not a `GroupChatScope` variant. A new `ChannelMsgInfo` type is added as a separate field on `ChatInfo.GroupChat` and as a separate parameter to pagination, parallel to `Maybe GroupChatScopeInfo`. This keeps the Comments view as a distinct concept rather than overloading the scope mechanism. 3. **iOS UI.** The Comments view opens via a hidden `NavigationLink` pushed onto the existing chat navigation stack (the same trick used by `NavStackCompat` and `UserSupportChatNavLink` / `GroupReportsChatNavLink`), not as a sheet. The user can navigate forward into a comment, back to the channel, and forward into another post's comments without losing context. @@ -18,7 +18,7 @@ The plan covers Haskell core, the CLI command surface (for tests), and the iOS a ## Solution summary -- Merge `ExtMsgContent` into `MsgContainer` as a single record with optional `quote :: Maybe QuotedMsg`, `parent :: Maybe MsgRef`, and plain `forward :: Bool` fields. Remove the four `MC*` constructors and `mcExtMsgContent`. Rewrite `parseMsgContainer` and `msgContainerJSON` to read/write the three discriminator fields independently rather than as mutually-exclusive arms — the existing wire JSON shape is preserved exactly because every existing message uses at most one of these keys and `forward` is only emitted when `True` (matching the old `MCForward` serializer arm). +- Merge `ExtMsgContent` into `MsgContainer` as a single record with optional `quote :: Maybe QuotedMsg`, `parent :: Maybe MsgRef`, and plain `forward :: Bool` fields. Remove the four `MC*` constructors and `mcExtMsgContent`. Rewrite `parseMsgContainer` and `msgContainerJSON` to read/write the three discriminator fields independently rather than as mutually-exclusive arms — the existing wire JSON shape is preserved exactly because every existing message uses at most one of these keys and `forward` is only emitted when `True` (matching the existing `MCForward` serializer arm). - Add `commentsVersion :: VersionChat = VersionChat 18` and bump `currentChatVersion = VersionChat 18`. Comment-bearing messages (those whose container has `parent = Just _`) and the new `XGrpCommentsDisabled` event are gated for relay forwarding behind this version. - New SQLite + Postgres migration `M20260501_channel_comments` adding three columns to `chat_items`: - `parent_chat_item_id INTEGER REFERENCES chat_items(chat_item_id) ON DELETE CASCADE` — set on the comment row. @@ -59,11 +59,12 @@ data MsgContainer = MsgContainer -- `parent` and `quote`). quote :: Maybe QuotedMsg, parent :: Maybe MsgRef, - -- forward is a plain Bool (not Maybe) to match the old wire format - -- exactly: the old parser treated absent/false as "not a forward" and - -- the old serializer only ever emitted "forward":true. Modelling it as - -- Maybe Bool would regress round-trips of `"forward": false` and bloat - -- the serialized output with an explicit false field. + -- forward is a plain Bool (not Maybe) to match the existing wire + -- format exactly: the existing parser treats absent/false as "not a + -- forward" and the existing serializer only ever emits "forward":true. + -- Modelling it as Maybe Bool would regress round-trips of + -- `"forward": false` and bloat the serialized output with an + -- explicit false field. forward :: Bool } deriving (Eq, Show) @@ -102,9 +103,9 @@ mc = } ``` -The base `mc` value is the empty container that callers update via record syntax. Send sites that previously built an `ExtMsgContent` record and wrapped it in `MCSimple` now build `MsgContainer` directly: `mcSimple content` followed by record-update for any optional fields (`mentions = ...`, `file = ...`, `ttl = ...`, `live = ...`, `scope = ...`, `asGroup = ...`). +The base `mc` value is the empty container that callers update via record syntax. Send sites that today build an `ExtMsgContent` record and wrap it in `MCSimple` are rewritten to build `MsgContainer` directly: `mcSimple content` followed by record-update for any optional fields (`mentions = ...`, `file = ...`, `ttl = ...`, `live = ...`, `scope = ...`, `asGroup = ...`). -`MSChannelMsg` is **not** introduced — `MsgScope` stays a single-constructor type `data MsgScope = MSMember {memberId :: MemberId}`. Comments do not need a scope tag because the parent reference lives in `MsgContainer.parent`. +`MsgScope` stays a single-constructor type `data MsgScope = MSMember {memberId :: MemberId}`. Comments do not need a scope tag because the parent reference lives in `MsgContainer.parent`. **Parser rewrite** (current `parseMsgContainer` at lines ~834–852). @@ -122,7 +123,7 @@ parseMsgContainer v = do asGroup <- v .:? "asGroup" quote <- v .:? "quote" parent <- v .:? "parent" - -- forward parsing matches the old two-arm form exactly: + -- forward parsing matches the existing two-arm form exactly: -- J.Bool b -> b (modern boolean form, including `false`) -- J.Object _ -> True (legacy object form: forward-compat with group links) -- absent -> False @@ -136,7 +137,7 @@ parseMsgContainer v = do parseForward _ = fail "invalid forward field" ``` -Backward compatibility: any old client that sent only one of `quote`/`parent`/`forward` is parsed identically — the other two fields are simply `Nothing`. New clients that send both `quote` and `parent` (a comment that quotes another comment) are parsed correctly. Pre-comments clients that receive a message with both `quote` and `parent` would see only the quote (their parser tries `MCQuote` first and never reads `parent`); the relay's per-recipient version gate (see "Forward compatibility" below) prevents such messages from reaching pre-comments clients in practice. Note that no current message in the wild has a `parent` field, because `MCComment` was dead code in the previous design. +Backward compatibility: any client below `commentsVersion` that sent only one of `quote`/`parent`/`forward` is parsed identically by the new code — the other two fields are simply `Nothing`. Clients at or above `commentsVersion` that send both `quote` and `parent` (a comment that quotes another comment) are parsed correctly. A pre-`commentsVersion` client that received a message with both `quote` and `parent` would see only the quote (its parser tries `MCQuote` first and never reads `parent`); the relay's per-recipient version gate (see "Forward compatibility" below) prevents such messages from reaching pre-`commentsVersion` clients in practice. No message currently in flight has a `parent` field, because `MCComment` is a dead constructor in the existing source. **Serializer rewrite** (`msgContainerJSON` at lines ~900–909). @@ -158,10 +159,10 @@ msgContainerJSON MsgContainer {content, mentions, file, ttl, live, scope, asGrou ) ) where - -- At most one discriminator is typically present (old wire format had - -- them as mutually-exclusive arms). When a comment quotes another - -- comment, both `quote` and `parent` appear; this is new-only and - -- guarded by commentsVersion relay forwarding. + -- At most one discriminator is typically present (the existing wire + -- format treats them as mutually exclusive). When a comment quotes + -- another comment, both `quote` and `parent` appear; this combination + -- is gated by commentsVersion relay forwarding. discriminators = ["quote" .= q | Just q <- [quote]] <> ["parent" .= p | Just p <- [parent]] @@ -169,10 +170,10 @@ msgContainerJSON MsgContainer {content, mentions, file, ttl, live, scope, asGrou ``` Key wire-format invariants preserved: -- `forward` is only ever emitted as `"forward": true` (never `false`), matching the old serializer. -- The field order inside the content sub-object (file, ttl, live, mentions, scope, asGroup, content) is unchanged from the existing `msgContent` helper — the new serializer wraps the same `("file" .=? file) $ ...` fold to keep every produced `J.Object` byte-identical to the old output for any pre-existing message. +- `forward` is only ever emitted as `"forward": true` (never `false`), matching the existing serializer. +- The field order inside the content sub-object (file, ttl, live, mentions, scope, asGroup, content) is unchanged from the existing `msgContent` helper — the rewritten serializer wraps the same `("file" .=? file) $ ...` fold to keep every produced `J.Object` byte-identical to the existing output for any pre-existing message. - `nonEmptyMap` (Protocol.hs:911) is reused verbatim; empty `mentions` maps are omitted exactly as before. -- When both `quote` and `parent` are set (the new case), both fields are emitted; no old message in the wild has this shape, and the version gate prevents old recipients from seeing it. +- When both `quote` and `parent` are set (only possible at or above `commentsVersion`), both fields are emitted; no message currently in flight has this shape, and the version gate prevents pre-`commentsVersion` recipients from seeing it. **Mechanical refactor — call sites.** @@ -304,7 +305,7 @@ The cascade self-FK is safe — SQLite cascades through one level by default, an ``` Existing call sites that build `ChatRef` are extended to pass `channelMsg_ = Nothing` by default. The Comments view passes the parent `ChatItemId`. -4. **No changes** to `GroupChatScope`, `GroupChatScopeTag`, `GroupChatScopeInfo`, or `toMsgScope`. They keep their current single `MemberSupport` variant. The previous draft's `GCSChannelMsg` / `GCSIChannelMsg` / `MSChannelMsg` are NOT added. +4. **No changes** to `GroupChatScope`, `GroupChatScopeTag`, `GroupChatScopeInfo`, or `toMsgScope`. They keep their current single `MemberSupport` variant. Comments are represented through `ChannelMsgInfo` rather than as a new scope variant, so the scope ADTs are untouched. ### 6. Smart constructor for `ChannelMsgInfo` @@ -426,7 +427,7 @@ The per-parent comment cap is enforced at query time, not at insert time — see ... AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL AND parent_chat_item_id IS NULL ... ``` -Each of these sites must be audited and updated. There is no safe shortcut: the previous design relied on `group_scope_tag = 'channel_msg'` to discriminate, but the new design has no scope tag for comments and uses the dedicated column instead. +Each of these sites must be audited and updated. Comments do not have a scope tag — the dedicated `parent_chat_item_id` column is the sole discriminator — so every default-scope SELECT must add the explicit `IS NULL` predicate. **Read-side plumbing in `toGroupChatItem`** (`Store/Messages.hs:2334`) and any other chat-item SELECT that reads chat-item rows: extend the row tuple to include `parent_chat_item_id`, `comments_total`, `comments_disabled`. These three fields propagate into the `CIMeta` record as new fields with default values. Parent chat-item id is exposed via a new optional `CIMeta` field; the comment count and disabled flag are read directly into the existing meta record. @@ -493,7 +494,7 @@ allowedRole = case (scope, channelMsgInfo_) of `src/Simplex/Chat/Library/Subscriber.hs` — `newGroupContentMessage` (~line 1943). -The current code resolves a chat scope via `mkGetMessageChatScope vr user gInfo m content msgScope_`. With the rework, the parent reference is in the message container (`mc.parent :: Maybe MsgRef`), not in the scope. The receive flow becomes: +The current code resolves a chat scope via `mkGetMessageChatScope vr user gInfo m content msgScope_`. The parent reference for a comment lives in the message container (`mc.parent :: Maybe MsgRef`), not in the scope. The receive flow becomes: > **Notation note.** `mc.parent`, `mc.quote`, `mc.forward` in this section are prose shorthand for "the `parent`/`quote`/`forward` field of `mc`". `Subscriber.hs` does **not** enable `OverloadedRecordDot` and `DuplicateRecordFields` disables bare field selectors as functions, so the implementation MUST use explicit record patterns (`case mc of MsgContainer {parent = Just p, quote = q, forward = f} -> ...`) or a local `let MsgContainer {parent, quote, forward} = mc in ...` binding. Dot syntax will not compile in `Subscriber.hs`. @@ -1006,10 +1007,10 @@ This slice is the largest mechanical refactor and is independent of comments. It **Wire-format-preserving acceptance criteria for Slice 1 (ALL must hold before the slice lands):** -- `forward` is `Bool` (not `Maybe Bool`) in the record. Absent or `false` in the input JSON parses to `False`; `true` or any Object parses to `True`. The serializer emits `"forward": true` only when `True`, and emits nothing when `False`. (No `"forward": false` ever appears on the wire, matching the old `msgContainerJSON` which only ever emitted `("forward" .= True)` from the `MCForward` arm.) -- The serializer's content-field ordering (`file, ttl, live, mentions, scope, asGroup, content`) is preserved byte-for-byte — the new implementation wraps the same `("file" .=? file) $ ("ttl" .=? ttl) $ ...` fold that `msgContent` used in the old `msgContainerJSON` at Protocol.hs:908-909. +- `forward` is `Bool` (not `Maybe Bool`) in the record. Absent or `false` in the input JSON parses to `False`; `true` or any Object parses to `True`. The serializer emits `"forward": true` only when `True`, and emits nothing when `False`. (No `"forward": false` ever appears on the wire, matching the existing `msgContainerJSON` which only ever emits `("forward" .= True)` from the `MCForward` arm.) +- The serializer's content-field ordering (`file, ttl, live, mentions, scope, asGroup, content`) is preserved byte-for-byte — the rewritten implementation wraps the same `("file" .=? file) $ ("ttl" .=? ttl) $ ...` fold that `msgContent` uses today in `msgContainerJSON` at Protocol.hs:908-909. - `nonEmptyMap` (Protocol.hs:911) is reused verbatim; empty `mentions` maps are omitted exactly as before. -- Discriminator fields are emitted at the front in the same order as old case-arms: `quote` first (if present), then `parent` (if present), then `forward: true` (if true). In the old code, mutual exclusion guaranteed at most one; in the new code, at most one is present for any pre-existing message because no in-flight message has both `quote` and `parent` (MCComment was dead code). +- Discriminator fields are emitted at the front in the order: `quote` first (if present), then `parent` (if present), then `forward: true` (if true). The existing source enforces mutual exclusion via case-arms; after the refactor, at most one is present for any pre-`commentsVersion` message because no in-flight message has both `quote` and `parent` (MCComment is a dead constructor in the existing source). - Every test in `tests/JSONTests.hs` that round-trips a `MsgContainer`, `ChatMsgEvent`, or `AppMessage` passes without modification. If a test had to be modified, the rewrite is wrong and must be revisited. - Manual spot-check: take five representative in-tree sample JSON messages (a plain text, a quote, a forward-with-bool, a forward-with-object, a file send) from `tests/ProtocolTests.hs` fixtures and round-trip them through the new parser + serializer. The output bytes must be identical to the input (modulo field ordering inside a single JSON object — Haskell Aeson preserves insertion order, so the fold-based serializer produces identical ordering). - The refactor does not touch `FromJSON`/`ToJSON` instances of `MsgContent`, `QuotedMsg`, `MsgRef`, `FileInvitation`, or `MsgScope`. Only the outer `MsgContainer` instances are rewritten. @@ -1142,7 +1143,7 @@ iOS build via Xcode (out of scope for the Haskell test environment; built separa - Same gating for `XGrpCommentsDisabled` events. - The relay's existing per-recipient version check (used for `groupKnockingVersion`, `contentReportsVersion`, etc.) is the place to add the gating. - New optional `parentChatItemId` / `commentsTotal` / `commentsDisabled` fields on chat-item JSON ship with `omittedField` defaults so older remote-connection clients see chat items normally. -- The merged `MsgContainer` record's wire JSON shape is identical to the previous shape — the old parser sees the same set of optional discriminator fields. No version bump is needed for the refactor itself; only the `parent` field semantics is gated. +- The merged `MsgContainer` record's wire JSON shape is identical to the existing shape — pre-`commentsVersion` parsers see the same set of optional discriminator fields. No version bump is needed for the refactor itself; only the `parent` field semantics is gated. **Threat model.**