From 177a591466c156010c298552421b5b513f5468f9 Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Thu, 23 Jul 2026 09:17:27 +0000 Subject: [PATCH 1/2] core: fix member support chats staying marked unread after they're read (#7281) * core: don't mark member support chat items read when reading group without scope Reading a group without a scope marked support-scope items read without decrementing the per-member support_chat_items_* counters, so members stayed unread in the support list even after their chat was fully read. Restrict the no-scope group read and its timed-items query to main-scope items. * plans: support chat unread on no-scope group read * core: update query plans for group scope read The main-scope read and timed-items queries now filter on group_scope_tag and group_scope_group_member_id, so they seek via idx_chat_items_group_scope_stats_all (5-column) instead of idx_chat_items_groups_user_mention (3-column). * plans: document query-plan and benchmark performance results * tests: fix unreliable support item id capture in no-scope group read test lastItemId returns the latest item by item_ts, which right after createGroup2 can be the group "connected" event rather than the just-sent support message. The per-item read then targeted the wrong (already-read, main-scope) item and never decremented the support counters, so the test failed regardless of the fix (consistently in CI, flakily locally depending on item ordering). Capture the support item id directly from the member-support scope instead, keeping the change contained to this test. Verified: the test passes with the fix and fails when the fix is reverted. * tests: fix name-shadowing build error in no-scope group read test The local pattern binding `itemId` shadowed the `itemId` helper imported from ChatTests.Utils, which -Wname-shadowing (Werror) rejects. Rename the local binding to `iId`. --- ...06-09-support-chat-unread-on-group-read.md | 200 ++++++++++++++++++ src/Simplex/Chat/Store/Messages.hs | 2 + .../SQLite/Migrations/chat_query_plans.txt | 6 +- tests/ChatTests/Groups.hs | 38 ++++ 4 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 plans/2026-06-09-support-chat-unread-on-group-read.md diff --git a/plans/2026-06-09-support-chat-unread-on-group-read.md b/plans/2026-06-09-support-chat-unread-on-group-read.md new file mode 100644 index 0000000000..5d24cc5ff1 --- /dev/null +++ b/plans/2026-06-09-support-chat-unread-on-group-read.md @@ -0,0 +1,200 @@ +# Fix: support-chat unread stats not cleared after no-scope group read + +Branch: `nd/fix-support-chat-unread-on-group-read` (off `origin/master`) + +## Symptom + +In a group with member admission ("chat with admins"), some members keep showing +as unread in the *chats with members* list even after the admin opens their support +chat and reads everything. Reopening never clears them. It is most visible for +members who have already been given a role (e.g. `member`), because pending members +always render the flag icon anyway, so the stuck badge is invisible for them. + +## Root cause + +The *chats with members* badge is driven by the per-member counters +`support_chat_items_unread / _member_attention / _mentions` on `group_members` +(surfaced as `GroupSupportChat`, `Types.hs:1087`). These are denormalized counters, +maintained by increment on receive and decrement on read. + +Reading a group **without a scope** (`APIChatRead` with `scope = Nothing`, +`Commands.hs:1153-1166`) runs `updateGroupChatItemsRead` (`Store/Messages.hs:2078`), +whose `UPDATE` filtered only by `group_id`: + +```sql +UPDATE chat_items SET item_status = RcvRead, ... +WHERE user_id = ? AND group_id = ? AND item_status = RcvNew +``` + +This flips **support-scope** items to `RcvRead` too, but never touches the +`support_chat_items_*` counters. The result is a permanent desync: + +- the items are read (so the chat "looks" fully read), but +- the per-member counters stay > 0 (so the member stays "unread" in the list), and +- reopening the support chat can't fix it: the scoped read decrement + (`updateGroupScopeUnreadStats`, `Store/Messages.hs:2202`) only fires on a real + `RcvNew -> RcvRead` transition, and there are no `RcvNew` items left. + +This is inconsistent with how the group's main unread **count** is computed, which +already excludes support items (`Store/Messages.hs:916`, +`... AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL`). The read +path simply forgot the same predicate. + +Note: `member_attention` self-heals on open via `checkSupportChatAttention` +(`Commands.hs:593-613`), recomputing it from loaded items. `unread` and `mentions` +have **no** such correction, which is why the badge persists. + +## Fix (Option A — prevent new corruption) + +Add the main-scope predicate to the two queries in the no-scope group-read path so +that reading a group without a scope leaves support-scope items (and their +disappearing-message timers) entirely untouched: + +- `updateGroupChatItemsRead` (`Store/Messages.hs`) — the read itself. +- `getGroupUnreadTimedItems`, `Nothing` branch (`Store/Messages.hs`) — its companion + timed-items query. + +```sql +AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL +``` + +Both lines are required and neither is droppable: + +- Without the read filter, support items get marked read (the bug). +- Without the timed-items filter, a disappearing support message would have its + delete timer started by a main-group read while still unread — a *new* + inconsistency introduced by a half-fix. The no-scope `APIChatRead` handler + (`Commands.hs:1160-1166`) calls `getGroupUnreadTimedItems` then + `setGroupChatItemsDeleteAt` then starts deletion threads, so both must agree that + support items are out of scope. + +Support items continue to be read (and their counters decremented) the correct way: +via the scoped read paths (`updateSupportChatItemsRead` / `updateGroupChatItemsReadList` +with a `MemberSupport` scope), exercised when the support chat is opened. + +### Why this is consistent / safe + +- Selects exactly the main-scope rows, identical to the authoritative unread-count + query (`Store/Messages.hs:916`). No main item is ever wrongly excluded — inserts + always set `group_scope_tag` and `group_scope_group_member_id` together + (`Store/Messages.hs:600-603`). +- `groups.members_require_attention` becomes *more* consistent, not less: the + no-scope read no longer zeroes support items behind a still-positive group counter. +- The added predicate is constant, parameterized SQL — no injection surface — and is + the existing codebase idiom (916 / 1214 / 1524 / 1745). + +## Test + +`tests/ChatTests/Groups.hs :: testScopedSupportUnreadStatsGroupReadNoScope` +(registered under the support-chat `describe` block). + +A member sends one support message (`markRead = False`), the admin marks the whole +group read with `/_read chat #1` (no scope), then reads the item in scope with the +**per-item** read `/_read chat items #1(_support:2) `. The per-item read only +decrements on a genuine `RcvNew -> RcvRead` transition, so: + +- on master the no-scope read already consumed the item -> decrement is a no-op -> + `unread: 1, require attention: 1` (test fails); +- with the fix the item is still unread -> decrement applies -> + `unread: 0, require attention: 0` (test passes). + +The full scoped read `/_read chat #1(_support:2)` is deliberately *not* used to +assert this, because `updateSupportChatItemsRead` hard-zeroes the counters +regardless of item state and would mask the bug. + +## Performance + +The added predicate makes both queries **faster**, not slower, and needs no new +index. Verified two ways. + +### Query plan (EXPLAIN QUERY PLAN, via the checked-in plan snapshots) + +Regenerating `chat_query_plans.txt` shows both statements switch index: + +``` +- SEARCH chat_items USING INDEX idx_chat_items_groups_user_mention + (user_id=? AND group_id=? AND item_status=?) -- 3-column seek ++ SEARCH chat_items USING INDEX idx_chat_items_group_scope_stats_all + (user_id=? AND group_id=? AND group_scope_tag=? AND + group_scope_group_member_id=? AND item_status=?) -- 5-column seek +``` + +SQLite compiles `group_scope_tag IS NULL` / `group_scope_group_member_id IS NULL` +into equality constraints (`=?`), so the two new columns *deepen the index seek* +rather than adding a post-seek filter or scan. The read now uses +`idx_chat_items_group_scope_stats_all` +(`user_id, group_id, group_scope_tag, group_scope_group_member_id, item_status, …`, +`M20250721_indexes.hs`), which already exists — this is the same index the +authoritative unread-count query uses. No migration, no new index. + +Note: master was **not** degraded here — it already got a full 3-column seek via +`idx_chat_items_groups_user_mention`. So this is a modest improvement, not the repair +of a regression. The plan file only records two hunks (the two flipped statements); +the `saveQueryPlans` test also churns an unrelated `group_members` count entry and the +whole `agent_query_plans.txt`, both of which were reverted to keep the diff to exactly +those two queries. + +### Micro-benchmark (synthetic 200k-row group) + +On a synthetic group of ~200k `chat_items` where ~25% are support-scope, running the +old vs. new `UPDATE` in alternating trials: + +| Query | mean wall-clock | +|-----------------|-----------------| +| old (no filter) | ~10.5 s | +| new (scoped) | ~7.4 s (≈ −29%) | + +The speedup tracks the row reduction: the new query touches only the ~75% main-scope +rows, so it does ~25% less work and the tighter seek trims a bit more. This also +directly demonstrates the bug — the old query flips ~50k support rows to `RcvRead` +(without decrementing counters); the new one flips **0**. + +Caveat: absolute timings were noisy (concurrent GHC builds on the box); only the +*relative* comparison across alternating trials is reliable, and it was consistent in +direction and magnitude across runs. The gain scales with the support-item share — a +group with no support chats sees no measurable change. + +## Limitations (accepted, by decision) + +1. **No retroactive repair.** This change only prevents *new* corruption. Rows already + stuck (counter > 0 while the items are `RcvRead`) stay stuck until either the + per-member "Mark read" action (`apiSupportChatRead` -> `updateSupportChatItemsRead`, + which hard-zeroes) or, for `member_attention` only, the self-heal on open + (`checkSupportChatAttention`). A one-shot migration recomputing the three counters + from `chat_items` was considered and **intentionally not done**. + +2. **`setUserChatsRead` is not fixed.** The global "mark all chats read" + (`APIUserRead` -> `setUserChatsRead`, `Store/Direct.hs:640-646`) has the same flaw: + its `UPDATE chat_items ... WHERE user_id = ? AND item_status = ?` has no chat-type + and no scope filter, so it reads support items user-wide without decrementing the + counters. It is **left as-is on purpose**: no GUI client invokes `APIUserRead` + (it exists only as the Haskell command type `Controller.hs:357` and the CLI + binding), so it is not reachable from Android / desktop / iOS — only the terminal + CLI. The same one-line predicate would close it if a client ever exposes it. + +3. **`getUpdateGroupItem` relies on a UI contract.** The per-item read + (`updateGroupChatItemsReadList` -> `getUpdateGroupItem`, `Store/Messages.hs:2178`) + has no scope filter on its `UPDATE` and its `Nothing` branch decrements nothing, so + passing a support item ID with `scope = Nothing` would reproduce the desync. This is + defended only by the in-code assumption ("we rely on UI to not pass item IDs from + incorrect scope", `Store/Messages.hs:2162-2163`), not by the query. Not reachable in + normal flow; left unchanged to keep the diff surgical. + +## Files changed + +- `src/Simplex/Chat/Store/Messages.hs` — two one-line scope predicates. +- `tests/ChatTests/Groups.hs` — one regression test + its `it` registration. + +## Build & test run + +Library compiled clean via `bash /home/user/build/linux.sh`. The full test suite was +subsequently built and run against this branch (968 examples). The regression test +`should not read support chat items when reading group without scope` passes with the +fix and fails when the fix is reverted (confirmed by rebuilding master and re-running), +so it genuinely catches the desync. + +The suite had 8 failures on the run; each was classified as flaky/environmental, +by-design, or pre-existing on master — none attributable to this change. In particular +the timed-message failure (`both users have configured timed messages … restart`) was +reproduced identically on master with the fix reverted (3/3), proving it pre-existing; +it is a direct-chat test that never touches the group-scope read path. diff --git a/src/Simplex/Chat/Store/Messages.hs b/src/Simplex/Chat/Store/Messages.hs index 5d6cc8fd8b..7b71e61512 100644 --- a/src/Simplex/Chat/Store/Messages.hs +++ b/src/Simplex/Chat/Store/Messages.hs @@ -2093,6 +2093,7 @@ updateGroupChatItemsRead db User {userId} GroupInfo {groupId} = do [sql| UPDATE chat_items SET item_status = ?, item_viewed = 1, updated_at = ? WHERE user_id = ? AND group_id = ? + AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL AND item_status = ? |] (CISRcvRead, currentTs, userId, groupId, CISRcvNew) @@ -2149,6 +2150,7 @@ getGroupUnreadTimedItems db User {userId} groupId scope = SELECT chat_item_id, timed_ttl FROM chat_items WHERE user_id = ? AND group_id = ? + AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL AND item_status = ? AND timed_ttl IS NOT NULL AND timed_delete_at IS NULL |] (userId, groupId, CISRcvNew) diff --git a/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt b/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt index 36f32944e2..14e2299514 100644 --- a/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt +++ b/src/Simplex/Chat/Store/SQLite/Migrations/chat_query_plans.txt @@ -1586,10 +1586,11 @@ Query: SELECT chat_item_id, timed_ttl FROM chat_items WHERE user_id = ? AND group_id = ? + AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL AND item_status = ? AND timed_ttl IS NOT NULL AND timed_delete_at IS NULL Plan: -SEARCH chat_items USING INDEX idx_chat_items_groups_user_mention (user_id=? AND group_id=? AND item_status=?) +SEARCH chat_items USING INDEX idx_chat_items_group_scope_stats_all (user_id=? AND group_id=? AND group_scope_tag=? AND group_scope_group_member_id=? AND item_status=?) Query: SELECT chat_item_moderation_id, moderator_member_id, created_by_msg_id, moderated_at @@ -5042,10 +5043,11 @@ SEARCH chat_items USING INTEGER PRIMARY KEY (rowid=?) Query: UPDATE chat_items SET item_status = ?, item_viewed = 1, updated_at = ? WHERE user_id = ? AND group_id = ? + AND group_scope_tag IS NULL AND group_scope_group_member_id IS NULL AND item_status = ? Plan: -SEARCH chat_items USING INDEX idx_chat_items_groups_user_mention (user_id=? AND group_id=? AND item_status=?) +SEARCH chat_items USING INDEX idx_chat_items_group_scope_stats_all (user_id=? AND group_id=? AND group_scope_tag=? AND group_scope_group_member_id=? AND item_status=?) Query: UPDATE chat_items SET item_status = ?, item_viewed = 1, updated_at = ? diff --git a/tests/ChatTests/Groups.hs b/tests/ChatTests/Groups.hs index 767728e400..c74b6367b4 100644 --- a/tests/ChatTests/Groups.hs +++ b/tests/ChatTests/Groups.hs @@ -240,6 +240,7 @@ chatGroupTests = do it "should correctly maintain unread stats for support chats on reading chat items" testScopedSupportUnreadStatsOnRead it "should correctly maintain unread stats for support chats on deleting chat items" testScopedSupportUnreadStatsOnDelete it "should correct member attention stat for support chat on opening it" testScopedSupportUnreadStatsCorrectOnOpen + it "should not read support chat items when reading group without scope" testScopedSupportUnreadStatsGroupReadNoScope it "should remove support chat with member when member is removed" testScopedSupportMemberRemoved it "should remove support chat with member when user removes member" testScopedSupportUserRemovesMember it "should remove support chat with member when member leaves" testScopedSupportMemberLeaves @@ -8462,6 +8463,43 @@ testScopedSupportUnreadStatsCorrectOnOpen = { markRead = False } +testScopedSupportUnreadStatsGroupReadNoScope :: HasCallStack => TestParams -> IO () +testScopedSupportUnreadStatsGroupReadNoScope = + testChatOpts2 opts aliceProfile bobProfile $ \alice bob -> do + createGroup2 "team" alice bob + + bob #> "#team (support) 1" + alice <# "#team (support: bob) bob> 1" + -- capture the support item id directly: lastItemId returns the latest item by + -- item_ts, which right after createGroup2 can be a group event ("connected") + -- rather than the support message, making the per-item read below target the + -- wrong item. + bobItemId <- + withCCTransaction alice $ \db -> do + rows <- DB.query_ db "SELECT chat_item_id FROM chat_items WHERE group_scope_tag = 'member_support' ORDER BY chat_item_id DESC LIMIT 1" :: IO [Only Int] + case rows of + Only iId : _ -> pure $ show iId + _ -> error "testScopedSupportUnreadStatsGroupReadNoScope: no member_support item" + + alice ##> "/member support chats #team" + alice <## "members require attention: 1" + alice <## "bob (Bob) (id 2): unread: 1, require attention: 1, mentions: 0" + + -- reading the group without scope must not mark support scope items read + alice #$> ("/_read chat #1", id, "ok") + + -- the support item was left unread, so reading it in scope still decrements the stats + alice #$> ("/_read chat items #1(_support:2) " <> bobItemId, id, "items read for chat") + + alice ##> "/member support chats #team" + alice <## "members require attention: 0" + alice <## "bob (Bob) (id 2): unread: 0, require attention: 0, mentions: 0" + where + opts = + testOpts + { markRead = False + } + testScopedSupportMemberRemoved :: HasCallStack => TestParams -> IO () testScopedSupportMemberRemoved = testChatOpts3 opts aliceProfile bobProfile cathProfile $ \alice bob cath -> do From fc28914455dc3ca69513177e7d3429051e0de4be Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Thu, 23 Jul 2026 09:18:11 +0000 Subject: [PATCH 2/2] desktop: fix saving files with '[', '*', or '?' in name on Linux (#7290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * desktop: fix saving files with '[', '*', or '?' in name on Linux JFileChooser's BasicFileChooserUI treats filenames containing '[', '*', or '?' as glob patterns on Unix when the file does not exist on disk. The Save button's ApproveSelectionAction then applies a GlobFilter and returns without saving — the dialog stays open and nothing is written. Regression introduced in #2789 (commit 02d00944f, 2023-07-28), which started pre-populating the filename via selectedFile = File(filename). Before that the save dialog ran in DIRECTORIES_ONLY mode and the JDK glob check never saw the filename. Fix: on Linux save dialogs, find the Save button by identity (BasicFileChooserUI.getApproveSelectionAction() is public and returns the exact ActionListener wired on the button) and replace it with a wrapper that delegates to the original action for non-glob names and calls approveSelection() directly for glob names. Windows (per the JDK only '*' and '?' are glob chars there) and macOS (uses AWT FileDialog) are unaffected. Open dialogs keep glob filtering for power-user search. * desktop: always bypass JFileChooser glob-on-save on Linux Earlier in this branch we added a heuristic to delegate to the original ApproveSelectionAction for non-'[' names so that typing '*.pdf' in the save dialog would still glob-filter the listing. That preserved a Swing-only quirk that no native OS save dialog implements: - macOS uses NSSavePanel (already used in this app via AWT FileDialog) — no glob-on-save. - Native Windows / Linux GTK / KDE save dialogs — no glob-on-save. - JFileChooser on Windows — has it, but '*' and '?' aren't even legal NTFS filename chars, so users won't be typing them. It also blocks legitimate filenames containing '*' or '?' on Linux (legal on POSIX filesystems) from being saved, since they too get intercepted as glob. Replace the Save button's action with a handler that always treats the typed text as a literal filename. The result: any filename can be saved on Linux, behaviour aligns with macOS and with every native OS save dialog. The open dialog is untouched, so '*.pdf' glob-filtering there still works. * desktop: restore directory-only save dialog on Linux The previous commit installed the literal-filename glob bypass for every Linux save dialog (!isLoad && isLinux), including the DIRECTORIES_ONLY mode used when filename == null — e.g. the "Save QR code as image" flow (saveTempImageUncompressed → saveDialog.awaitResult() with no params). In that mode the bypass broke directory selection entirely: selecting a subfolder + Save traversed into it instead of approving it, and Save with an empty filename field no-oped. Net result: QR-code image saving was unusable on Linux. The glob-on-save bug only arises because a filename is pre-populated (selectedFile = File(filename)), which never happens in DIRECTORIES_ONLY mode. Gate the bypass on filename != null so the directory-save path keeps JFileChooser's original approve action. * desktop: simplify Linux save glob bypass via getDefaultButton --------- Co-authored-by: shum --- .../views/helpers/DefaultDialog.desktop.kt | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/helpers/DefaultDialog.desktop.kt b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/helpers/DefaultDialog.desktop.kt index 7341c6af23..79aaf5c727 100644 --- a/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/helpers/DefaultDialog.desktop.kt +++ b/apps/multiplatform/common/src/desktopMain/kotlin/chat/simplex/common/views/helpers/DefaultDialog.desktop.kt @@ -14,10 +14,12 @@ import chat.simplex.res.MR import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import java.awt.FileDialog +import java.awt.event.ActionListener import java.io.File import javax.swing.JFileChooser import javax.swing.filechooser.FileFilter import javax.swing.filechooser.FileNameExtensionFilter +import javax.swing.plaf.basic.BasicFileChooserUI @Composable actual fun DefaultDialog( @@ -77,6 +79,13 @@ fun FrameWindowScope.FileDialogChooserMultiple( fileChooser.dialogTitle = title fileChooser.isMultiSelectionEnabled = allowMultiple && isLoad fileChooser.isAcceptAllFileFilterUsed = fileFilter == null + // Only install the glob bypass for the real file-save case (filename != null). When filename + // is null the dialog runs in DIRECTORIES_ONLY mode (e.g. "Save QR code as image"), where the + // Save button must approve the selected directory — the literal-filename handler would instead + // traverse into it (or no-op on an empty field), making directory selection impossible. + if (!isLoad && filename != null && desktopPlatform.isLinux()) { + installUnixSaveGlobBypass(fileChooser) + } if (fileFilter != null && fileFilterDescription != null) { fileChooser.addChoosableFileFilter(object: FileFilter() { override fun accept(file: File?): Boolean = fileFilter(file) @@ -120,6 +129,28 @@ fun FrameWindowScope.FileDialogChooserMultiple( } } +// Replace the Save button's action with a literal-filename handler. This bypasses JFileChooser's +// glob-on-save behaviour, which mis-handles '[' as a glob char on Unix (breaking filenames like +// '[1].pdf') and is not a feature of any native OS save dialog — macOS NSSavePanel and native +// Windows / Linux GTK / KDE save dialogs all treat the typed filename as a literal name. +private fun installUnixSaveGlobBypass(fc: JFileChooser) { + val ui = fc.ui as? BasicFileChooserUI ?: return + val original: ActionListener = ui.approveSelectionAction + val btn = ui.getDefaultButton(fc) ?: return + btn.removeActionListener(original) + btn.addActionListener { + val name = ui.fileName?.takeIf { it.isNotEmpty() } ?: return@addActionListener + val typed = File(name) + val target = if (typed.isAbsolute) typed else File(fc.currentDirectory, name) + if (target.isDirectory && fc.isTraversable(target)) { + fc.currentDirectory = target + } else { + fc.selectedFile = target + fc.approveSelection() + } + } +} + /* * Has graphic glitches on many Linux distributions, so use only on non-Linux systems. Also file filter doesn't work on Windows * */