feat(group-chat): global auto-scroll toggle (#1120)#1131
Conversation
Adds a persisted groupChatAutoScroll setting (default true) wired through the settings store, metadata, and a Display-tab toggle. GroupChatMessages now gates its auto-scroll-to-bottom effect on the setting, so users can keep their scroll position while reading earlier messages. Covered by store + component tests.
… reset (#1120) Codex review fixes: add setGroupChatAutoScroll to getSettingsActions(), add a searchableSettings Display entry so the toggle is deep-linkable, and reset groupChatAutoScroll in the test resetStore() to remove order-dependent state leak. Skipped main SETTINGS_DEFAULTS by design: renderer-only display prefs (chatRawTextMode, markdownEditMode, bionifyReadingMode, userMessageAlignment) are all absent there too; the renderer + metadata defaults cover it.
Greptile SummaryThis PR adds a global setting for group chat auto-scroll. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(group-chat): reset initial-scroll tr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a global ChangesGroup chat auto-scroll setting
Sequence Diagram(s)sequenceDiagram
participant User
participant DisplayTab
participant settingsStore
participant GroupChatPanel
participant GroupChatMessages
participant chatContainer
User->>DisplayTab: toggle Auto-scroll to newest message
DisplayTab->>settingsStore: setGroupChatAutoScroll(value)
settingsStore->>settingsStore: persist groupChatAutoScroll
GroupChatPanel->>GroupChatMessages: pass chatId
GroupChatMessages->>settingsStore: read groupChatAutoScroll
alt initial chat load
GroupChatMessages->>chatContainer: scroll to bottom
else new message arrives and setting enabled
GroupChatMessages->>chatContainer: scroll to bottom
else new message arrives and setting disabled
GroupChatMessages->>chatContainer: keep current scrollTop
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40e082987f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Auto-scroll on new messages | ||
| // Auto-scroll on new messages (unless disabled via the global setting) | ||
| useEffect(() => { | ||
| if (!groupChatAutoScroll) return; |
There was a problem hiding this comment.
When groupChatAutoScroll is off, this early return also skips the scroll that used to happen when a populated group chat is first mounted or when handleOpenGroupChat replaces the messages after switching chats. In that scenario the scroll container keeps its previous scrollTop (or 0 on first mount), so users who disabled auto-scroll to read older incoming messages can open another existing chat and land at the top or an arbitrary old offset instead of the newest message. Consider still scrolling on mount/chat-change and only suppressing scrolls for subsequent appends.
Useful? React with 👍 / 👎.
| if (allSettings['groupChatAutoScroll'] !== undefined) | ||
| patch.groupChatAutoScroll = allSettings['groupChatAutoScroll'] as boolean; |
There was a problem hiding this comment.
When a user disables this setting and then resets it externally, for example maestro-cli settings reset groupChatAutoScroll, the key is removed from maestro-settings.json; because this new key was not added to src/main/stores/defaults.ts, the settings:getAll snapshot has no value for it, and this guard leaves the existing false value in the Zustand store since setState(patch) merges rather than resets. The documented default is true, so reset or deletion does not take effect until the user explicitly sets it back to true or restarts into the renderer default.
Useful? React with 👍 / 👎.
| // Auto-scroll on new messages | ||
| // Auto-scroll on new messages (unless disabled via the global setting) | ||
| useEffect(() => { | ||
| if (!groupChatAutoScroll) return; |
There was a problem hiding this comment.
This new setting only gates the desktop GroupChatMessages component. The web/mobile group chat path still auto-scrolls unconditionally in src/web/mobile/GroupChatPanel.tsx when chatState.messages.length changes, and the web settings snapshot currently hardcodes autoScroll: true, so users viewing group chats through the remote/mobile interface will still be snapped to the bottom despite turning off the global group-chat auto-scroll setting in Display. If this is intended as a global group chat preference, thread groupChatAutoScroll through the web settings payload and consume it in the mobile group chat panel too.
Useful? React with 👍 / 👎.
…ly new-message auto-scroll (#1120) Review fix: the first scroll-to-bottom (initial mount / first non-empty messages load) always runs so a chat opens at its latest message even when auto-scroll is off; only subsequent new-message scrolls respect the setting. Effect now depends on [messages] with a render-time setting ref, so toggling no longer yanks the reader's scroll position. Adds two tests. Declined the reset-fallback (sibling chatRawTextMode behaves identically); deferred mobile to a follow-up.
|
@greptile re-review please |
#1120) Round-3 review fix: GroupChatMessages is reused (not keyed) across group chats, so hasAutoScrolledRef persisted and switching chats skipped the initial scroll-to-newest. Thread a chatId prop and reset the ref when it changes so each newly opened chat lands at its latest message even with auto-scroll off. +2 tests.
|
@greptile re-review please |
1 participant
Summary
Closes #1120. Adds a global on/off setting to disable auto-scroll in group chats so users can read earlier messages without the view snapping to the bottom on every new message.
Changes
groupChatAutoScrollsetting (defaulttrue, preserving current behavior), wired through the settings store the same way as other global booleans: state + action interfaces, default, setter,window.maestro.settingspersistence, load/hydration patch,useSettingstype,getSettingsActions(), andsettingsMetadata.GroupChatMessagesnow gates its scroll-to-bottom effect on the setting.searchableSettingsentry so it is discoverable in settings search.Verification
npm run lint(tsc x3): cleanvitest runsettings store plus a newGroupChatMessagesrender test proving the effect scrolls when enabled and does NOT when disabled: 157 tests passeslint+prettier: cleancodex exec(read-only); review findings (test reset leak, action/search registration) addressed.Summary by CodeRabbit