mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-10-23 06:22:43 +00:00
Merge branch 'master' into f/channel-multi-owner
This commit is contained in:
commit
5c1985026e
5 files changed
+275
-2
No files matched your search
+31
@@ -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
|
||||
* */
|
||||
|
||||
@@ -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) <id>`. 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.
|
||||
@@ -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)
|
||||
|
||||
@@ -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 = ?
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user