Add group chat snooze menu back from history#339
Open
nacklequack wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a group IM "snooze" feature: session state gains a close-action and snooze duration, ChangesGroup IM Snooze
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LLFloaterIMSession
participant LLGroupActions
participant LLIMMgr
participant LLIMProcessing
User->>LLFloaterIMSession: click snooze_groupt_btn (duration)
LLFloaterIMSession->>LLGroupActions: snoozeIM(group_id, duration) / leaveIM(group_id)
LLGroupActions->>LLIMMgr: leaveSession(session_id) with mCloseAction set
LLIMMgr->>LLIMMgr: store expiration in mSnoozedSessions
Note over LLIMProcessing: New group message arrives
LLIMProcessing->>LLIMMgr: checkSnoozeExpiration(session_id)
LLIMMgr-->>LLIMProcessing: not expired
LLIMProcessing->>LLIMMgr: restoreSnoozedSession(session_id)
LLIMMgr->>LLIMMgr: remove from mSnoozedSessions, addSession, sendStartSession
LLIMMgr-->>LLIMProcessing: session restored
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
indra/newview/llfloaterimsession.cpp (1)
288-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: naming typo and cast robustness.
boost::lexical_cast<S32>(value)will throwbad_lexical_castifgetValue()ever returns a non-numeric string; since values are hardcoded in the XML this is low risk today but fragile against future edits (e.g., adding a non-numeric flyout item value).- The control name
snooze_groupt_btn(typo for "group") is used both here (implicitly via caller) and in the XML; worth fixing for consistency, though it's an internal identifier only.Consider using
LLStringUtil::convertToS32or wrapping the cast in a try/catch for defensive robustness against malformed values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/llfloaterimsession.cpp` around lines 288 - 300, The snooze-time handling in llfloaterimsession.cpp is too fragile because llfloaterIMSession’s use of boost::lexical_cast<S32>(value) can throw if the control value ever becomes non-numeric. Update the logic in the value-handling branch to use a safer conversion path such as LLStringUtil::convertToS32 or a guarded try/catch, and keep the LLGroupActions::leaveIM/snoozeIM behavior unchanged for the existing "-1", empty, and numeric cases. Also rename the internal control identifier snooze_groupt_btn to the consistent group spelling in both the UI XML and any code that references it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indra/newview/llfloaterimsession.cpp`:
- Around line 281-302: The LLFloaterIMSession::onSnoozeGroupClicked handler
treats the “Next relog” option as leaveIM and only stores snooze state in
mSnoozedSessions, so update the snooze flow to persist a true “mute until next
login” state instead of using a transient in-memory flag. Add a restore path for
that persistent state when sessions are recreated, and keep the existing snooze
durations for the other value cases while ensuring the value="-1" branch maps to
the new persistent snooze behavior rather than leaving the IM session.
In `@indra/newview/llimprocessing.cpp`:
- Around line 1346-1356: The message handling in the block using hasSession,
checkSnoozeExpiration, and restoreSnoozedSession drops the incoming message
after a snoozed session is successfully restored because control flow exits the
hasSession branch before the later offline/is_do_not_disturb handling. Update
this logic so a successful restore continues into the normal message-processing
path instead of returning or skipping the rest of the function, and make the
same change in the duplicated code path referenced by the other affected
section.
In `@indra/newview/llimview.cpp`:
- Around line 3561-3562: The snooze handling in llimview.cpp is collapsing the
“next relog” sentinel because llmax(0, im_session->mSnoozeDuration) turns
mSnoozeDuration == -1 into an immediately expired timer. Update the snooze
calculation in the code paths that assign mSnoozedSessions for both the session
state in llimview.cpp so that the sentinel value is preserved and treated as
“snoozed until logout,” while only finite snooze durations are converted into a
timestamp-based expiry. Use the existing im_session and mSnoozedSessions logic
as the anchor points when applying the fix.
- Around line 3779-3785: The snooze entry is being removed before restoration is
confirmed, so a failed getGroupData() call leaves the session permanently
unsnoozed. Update the snoozed-session restoration flow in the logic that uses
mSnoozedSessions and gAgent.getGroupData() so the erase only happens after group
data has been successfully retrieved and the session can actually be restored.
Keep the existing false return path, but preserve the snooze state when
restoration cannot proceed.
---
Nitpick comments:
In `@indra/newview/llfloaterimsession.cpp`:
- Around line 288-300: The snooze-time handling in llfloaterimsession.cpp is too
fragile because llfloaterIMSession’s use of boost::lexical_cast<S32>(value) can
throw if the control value ever becomes non-numeric. Update the logic in the
value-handling branch to use a safer conversion path such as
LLStringUtil::convertToS32 or a guarded try/catch, and keep the
LLGroupActions::leaveIM/snoozeIM behavior unchanged for the existing "-1",
empty, and numeric cases. Also rename the internal control identifier
snooze_groupt_btn to the consistent group spelling in both the UI XML and any
code that references it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b2a332db-80d2-46e3-aba4-5a95a34fa016
📒 Files selected for processing (8)
indra/newview/llfloaterimsession.cppindra/newview/llfloaterimsession.hindra/newview/llgroupactions.cppindra/newview/llgroupactions.hindra/newview/llimprocessing.cppindra/newview/llimview.cppindra/newview/llimview.hindra/newview/skins/default/xui/en/floater_im_session.xml
Comment on lines
+281
to
+302
| void LLFloaterIMSession::onSnoozeGroupClicked(const LLUICtrl* pCtrl) | ||
| { | ||
| if (!pCtrl) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const std::string value = pCtrl->getValue().asString(); | ||
| if (value == "-1") | ||
| { | ||
| LLGroupActions::leaveIM(mSessionID); | ||
| } | ||
| else if (value.empty()) | ||
| { | ||
| LLGroupActions::snoozeIM(mSessionID, 0); | ||
| } | ||
| else | ||
| { | ||
| LLGroupActions::snoozeIM(mSessionID, boost::lexical_cast<S32>(value) * 60); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and symbols.
git ls-files | rg 'indra/newview/(llfloaterimsession.cpp|llgroupactions.*|.*im_session.xml)$'
printf '\n--- llfloaterimsession.cpp (relevant section) ---\n'
sed -n '260,330p' indra/newview/llfloaterimsession.cpp
printf '\n--- LLGroupActions definitions/usages ---\n'
rg -n "leaveIM\\(|snoozeIM\\(|close_group_im\\(" indra/newview -g '!*.o'
printf '\n--- floater_im_session.xml (relevant section) ---\n'
sed -n '320,350p' indra/newview/skins/default/xui/en/floater_im_session.xml
printf '\n--- snooze/leave behavior references ---\n'
rg -n "CLOSE_SNOOZE|CLOSE_LEAVE|snooze_duration|Next relog|snooze" indra/newviewRepository: AlchemyViewer/Alchemy
Length of output: 8096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- llgroupactions.cpp close_group_im/leaveIM/snoozeIM ---\n'
sed -n '600,665p' indra/newview/llgroupactions.cpp
printf '\n--- llimview.cpp snooze/reopen path ---\n'
sed -n '3540,3795p' indra/newview/llimview.cpp
printf '\n--- llimview.h close action / snoozed sessions declarations ---\n'
sed -n '70,95p' indra/newview/llimview.h
sed -n '535,555p' indra/newview/llimview.hRepository: AlchemyViewer/Alchemy
Length of output: 12670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- restoreSnoozedSession references ---\n'
rg -n "restoreSnoozedSession\\(" indra/newview
printf '\n--- mSnoozedSessions references ---\n'
rg -n "mSnoozedSessions|checkSnoozeExpiration|isSnoozedSession" indra/newview
printf '\n--- login/session-init hooks that might restore snoozed sessions ---\n'
rg -n "login|logged in|login.*session|session.*login|restore.*snooze|Snooze" indra/newview/ll*.cpp indra/newview/ll*.hRepository: AlchemyViewer/Alchemy
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1328,1362p' indra/newview/llimprocessing.cpp
printf '\n--- any snooze persistence/serialization hooks ---\n'
rg -n "mSnoozedSessions|snoozed_sessions|restoreSnoozedSession|checkSnoozeExpiration|isSnoozedSession|save.*snooze|load.*snooze" indra/newview/llim*.cpp indra/newview/llim*.hRepository: AlchemyViewer/Alchemy
Length of output: 3096
Next relog needs persistent snooze handling. value="-1" still calls leaveIM, while snooze state is only kept in mSnoozedSessions in memory. If this option is meant to mute the group until the next login, it needs a persistent restore path; otherwise the label is misleading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indra/newview/llfloaterimsession.cpp` around lines 281 - 302, The
LLFloaterIMSession::onSnoozeGroupClicked handler treats the “Next relog” option
as leaveIM and only stores snooze state in mSnoozedSessions, so update the
snooze flow to persist a true “mute until next login” state instead of using a
transient in-memory flag. Add a restore path for that persistent state when
sessions are recreated, and keep the existing snooze durations for the other
value cases while ensuring the value="-1" branch maps to the new persistent
snooze behavior rather than leaving the IM session.
Comment on lines
1346
to
1356
| if (!gIMMgr->hasSession(session_id)) | ||
| { | ||
| return; | ||
| if (!gAgent.isInGroup(session_id) || | ||
| !gIMMgr->checkSnoozeExpiration(session_id) || | ||
| !gIMMgr->restoreSnoozedSession(session_id)) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| else if (offline == IM_ONLINE && is_do_not_disturb) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Process the message after restoring the snoozed session.
When restoreSnoozedSession() succeeds, execution leaves the if (!hasSession) block and skips the attached else if / else, so the message that woke the session is dropped.
🐛 Proposed fix
if (!gIMMgr->hasSession(session_id))
{
if (!gAgent.isInGroup(session_id) ||
!gIMMgr->checkSnoozeExpiration(session_id) ||
!gIMMgr->restoreSnoozedSession(session_id))
{
return;
}
}
- else if (offline == IM_ONLINE && is_do_not_disturb)
+ if (offline == IM_ONLINE && is_do_not_disturb)
{Also applies to: 1389-1415
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indra/newview/llimprocessing.cpp` around lines 1346 - 1356, The message
handling in the block using hasSession, checkSnoozeExpiration, and
restoreSnoozedSession drops the incoming message after a snoozed session is
successfully restored because control flow exits the hasSession branch before
the later offline/is_do_not_disturb handling. Update this logic so a successful
restore continues into the normal message-processing path instead of returning
or skipping the rest of the function, and make the same change in the duplicated
code path referenced by the other affected section.
Comment on lines
+3561
to
+3562
| const F64 duration = llmax(0, im_session->mSnoozeDuration); | ||
| mSnoozedSessions[session_id] = LLTimer::getTotalSeconds() + duration; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the “next relog” sentinel instead of expiring it immediately.
mSnoozeDuration == -1 is the only visible sentinel for “next relog”, but llmax(0, ...) stores it as an immediately-expired snooze, so the next group message restores the chat instead of keeping it snoozed until logout.
🐛 Proposed fix
- const F64 duration = llmax(0, im_session->mSnoozeDuration);
- mSnoozedSessions[session_id] = LLTimer::getTotalSeconds() + duration;
+ const S32 duration = im_session->mSnoozeDuration;
+ mSnoozedSessions[session_id] = duration < 0
+ ? -1.0 // in-memory only: snoozed until next relog
+ : LLTimer::getTotalSeconds() + duration;- return it != mSnoozedSessions.end() && it->second <= LLTimer::getTotalSeconds();
+ return it != mSnoozedSessions.end()
+ && it->second >= 0.0
+ && it->second <= LLTimer::getTotalSeconds();Also applies to: 3760-3763
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indra/newview/llimview.cpp` around lines 3561 - 3562, The snooze handling in
llimview.cpp is collapsing the “next relog” sentinel because llmax(0,
im_session->mSnoozeDuration) turns mSnoozeDuration == -1 into an immediately
expired timer. Update the snooze calculation in the code paths that assign
mSnoozedSessions for both the session state in llimview.cpp so that the sentinel
value is preserved and treated as “snoozed until logout,” while only finite
snooze durations are converted into a timestamp-based expiry. Use the existing
im_session and mSnoozedSessions logic as the anchor points when applying the
fix.
Comment on lines
+3779
to
+3785
| mSnoozedSessions.erase(it); | ||
|
|
||
| LLGroupData group_data; | ||
| if (!gAgent.getGroupData(session_id, group_data)) | ||
| { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t consume snooze state before restoration can succeed.
If getGroupData() fails, this returns false after erasing the snooze entry, so future messages can no longer restore the snoozed session.
🐛 Proposed fix
- mSnoozedSessions.erase(it);
-
LLGroupData group_data;
if (!gAgent.getGroupData(session_id, group_data))
{
return false;
}
+
+ mSnoozedSessions.erase(it);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mSnoozedSessions.erase(it); | |
| LLGroupData group_data; | |
| if (!gAgent.getGroupData(session_id, group_data)) | |
| { | |
| return false; | |
| } | |
| LLGroupData group_data; | |
| if (!gAgent.getGroupData(session_id, group_data)) | |
| { | |
| return false; | |
| } | |
| mSnoozedSessions.erase(it); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indra/newview/llimview.cpp` around lines 3779 - 3785, The snooze entry is
being removed before restoration is confirmed, so a failed getGroupData() call
leaves the session permanently unsnoozed. Update the snoozed-session restoration
flow in the logic that uses mSnoozedSessions and gAgent.getGroupData() so the
erase only happens after group data has been successfully retrieved and the
session can actually be restored. Keep the existing false return path, but
preserve the snooze state when restoration cannot proceed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds the group chat snooze flyout/menu back to group IM sessions on
develop.This restores the small group chat toolbar menu that allows a group conversation to be snoozed for:
The implementation ports the group-chat floater/action/session pieces from the older
mainhistory into the currentdevelopIM/session code. It tracks snoozed group sessions locally, suppresses reopening while the snooze is active, and restores the group session when a message arrives after the snooze expires.Historical attribution/context:
maincommitbe67f0cd6c06by @RyeMutt: “Port Catznip's snooze group feature”.develop, including2c15baf8b5by @cinderblocks/@RyeMutt , originally from Katherine Berry, and follow-upced1e6ed9bby Rye for muted group chat session behavior.maincommitsf804d2e7f80d/821bc5b960feby @RyeMutt partially backed out/adjusted that area. This PR is scoped to the explicit group chat toolbar snooze menu.Checklist
Please ensure the following before requesting review:
Additional Notes
Local verification completed: