WIP: small chat 3#29131
WIP: small chat 3#29131chrisnojima-zoom wants to merge 11 commits intonojima/HOTPOT-next-670-clean-2from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the chat thread performance/refactor work by stabilizing message list rendering/recycling and improving profiling/regression guidance across native + desktop.
Changes:
- Move more row recycling/type decisions into convo-store derived metadata and simplify native list subscriptions/callbacks.
- Add/adjust profiling instrumentation and documentation (including a manual regression checklist) for thread performance work.
- Minor UI/styling and desktop behavior fixes (avatar stacking, edit-jump effect logic, waypoint profiling).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
shared/stores/convostate.tsx |
Derive row recycle subtypes from render type + message state (pending/reply/reactions) and use that in per-row metadata refresh. |
shared/chat/conversation/list-area/index.native.tsx |
Reduce store subscription fan-out via a shallow selector, stabilize getItemType, and avoid unconditional ordinal reversing work. |
shared/chat/conversation/list-area/index.desktop.tsx |
Fix edit-jump effect condition and add a PerfProfiler around waypoint chunk rendering. |
shared/chat/conversation/messages/reactions-rows.tsx |
Attempt to memoize reaction emoji order computation (currently introduces a correctness risk; see PR comment). |
shared/chat/conversation/messages/wrapper/wrapper.tsx |
Adjust mobile avatar stacking (zIndex) for consistent overlay behavior. |
shared/perf/PERF-TESTING.md |
Update profiler wrapper locations and add a chat thread regression checklist. |
plans/chat-refactor.md |
Mark completed workstreams and record decisions/verification steps for the perf refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -672,26 +672,45 @@ func (s *store) Add(ctx context.Context, convID chat1.ConversationID, | |||
| msgs []chat1.MessageUnboxed, | |||
| ) (err error) { | |||
| defer s.Trace(ctx, &err, "Add")() | |||
There was a problem hiding this comment.
Fixed two deadlocks in the chat search indexer that caused thread loading to break permanently after switching users.
Bug 1: Indexer.Clear held idx.Lock() while performing bulk LevelDB deletes (one per indexed message in a conversation). All GetThreadNonblock calls go through
Indexer.Suspend, which also needs idx.Lock(), so they blocked for the entire duration of the delete loop — potentially many seconds.
Bug 2: store.Add held the store's write mutex (s.Lock()) while calling ChatHelper.GetMessages to fetch superseded messages for EDIT and ATTACHMENTUPLOADED message types.
That call goes through ConvSource.GetMessages → ConversationLockTab, which has no timeout on the lock acquisition path when no deadlock cycle is detected. If any other
goroutine held the conversation lock at that moment, store.Add would block indefinitely while holding s.Lock(). Since Indexer.Clear calls store.ClearMemory (which also
needs s.Lock()), it too blocked indefinitely, keeping idx.Lock() held and freezing all thread loading.
The repro: send a message via CLI as a different user, then switch users. The background indexer queues work from the previous session; when it processes EDIT messages it
triggers the store.Add → conv lock path while Indexer.Clear is running for the new session, creating the deadlock.
Fixes:
- Removed the unnecessary idx.Lock() from Indexer.Clear — the store has its own concurrency controls and the indexer mutex guards unrelated fields (started, suspendCount).
- Moved the superseded-message fetch in store.Add to before s.Lock() is acquired, so the mutex is only held for pure in-memory index mutations.
cc: @zoom-ua
No description provided.