MultiBot: improve bridge synchronization, strategy controls and persistent favorites - #49
Merged
Wishmaster117 merged 4 commits intoAug 7, 2026
Conversation
…stent favorites ## Summary This PR consolidates the current MultiBot addon-side work for the gradual migration from legacy chat-driven bot control toward the MultiBot Bridge architecture. The main goals are to improve bridge synchronization, make strategy controls more reliable, and keep bot/favorite UI state consistent across connection changes and reloads. ## Main changes ### Bridge-backed bot state - Improves synchronization between the addon roster and bridge-provided bot state. - Keeps existing bot buttons updated when bridge roster information changes. - Uses bridge `classId` and level information to refresh: - class icon; - localized class tooltip; - bot level; - runtime button metadata. - Preserves the existing addon behavior for functionality that has not yet been migrated away from chat. ### Strategy controls - Improves strategy state synchronization and refresh behavior. - Keeps strategy UI state consistent after bridge commands and acknowledgements. - Validated with multiple classes and strategy operations, including: - loot; - gather; - tank assist; - cure. ### Favorites - Favorites stored in SavedVariables can now be represented in the Favorites roster even when the bot is offline. - Offline favorite buttons remain clickable and can still be used to connect the bot. - Favorite buttons are reused when the corresponding bot becomes available instead of requiring a UI reload. - Fixes the case where connecting several favorite bots successively could leave the second bot without its class bar until `/reload`. ### Favorite metadata synchronization - Bot details received from the bridge are persisted through the existing global bot store. - Normalizes bridge gender metadata to the format already expected by the addon store: - `Male` -> `[M]` - `Female` -> `[F]` - Reuses the existing `profile.bots` SavedVariables structure instead of introducing another persistence table. - Requests bot details when required to populate metadata for newly added favorites. - Adds a bounded/rearmable roster refresh after connecting an offline favorite so successive bot connections are detected without an addon reload. ## Compatibility Target environment: - World of Warcraft 3.3.5a - AzerothCore WotLK - mod-playerbots - mod-multibot-bridge No modern WoW Lua APIs are introduced. ## Validation Validated with: - addon reloads; - reconnects; - offline favorites; - successive connection of multiple favorite bots; - class icon/tooltip refresh; - persisted bot metadata; - strategy enable/disable operations; - multiple bots; - zero Lua errors during the final in-game tests. The latest Favorites synchronization runtime test confirmed: - offline favorite buttons: OK; - successive bot connections: OK; - second bot bar without `/reload`: OK; - class icon synchronization: OK; - class tooltip synchronization: OK; - Lua errors: none. ## Scope This PR intentionally does not attempt to remove every remaining chat-based Playerbots command. The migration is incremental: legacy chat mechanisms are kept where a validated bridge replacement is not available yet.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds favorite synchronization, class-based auto-group transactions, framed bridge state transfers, and validated bridge strategy mutations. Engine actions use bridge execution when available and retain chat fallbacks. ChangesFavorites and Auto-Group
Bridge State and Strategy Transactions
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MultiBotUnitsRootUI
participant MultiBot
participant Bridge
MultiBotUnitsRootUI->>MultiBot: UpdateFavoritesIndex()
MultiBot->>MultiBot: EnsureFavoriteButtons()
MultiBot->>Bridge: request favorite roster data
Bridge->>MultiBot: roster response
MultiBot->>MultiBotUnitsRootUI: update favorite button metadata
sequenceDiagram
participant MultiBotEngine
participant MultiBotComm
participant Bridge
MultiBotEngine->>MultiBotComm: RunStrategyCommand(scope, target, stateScope, changes)
MultiBotComm->>Bridge: send strategy mutation
Bridge->>MultiBotComm: STRATEGY_ACK or protocol error
MultiBotComm->>MultiBotEngine: invoke mutation callback
MultiBotEngine->>MultiBotComm: request state refresh
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3591f232db
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
Core/MultiBot.lua (2)
1510-1622: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: remove the duplicated time and name helpers.
GetAddClassAutoGroupNowandNormalizeAddClassAutoGroupNameduplicateGetFavoriteRosterRefreshNowandNormalizeFavoriteRosterRefreshNameat lines 1338-1356. The bodies are identical. Extract one shared local pair and reuse it in both blocks.🤖 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 `@Core/MultiBot.lua` around lines 1510 - 1622, Remove the duplicate helper definitions GetAddClassAutoGroupNow and NormalizeAddClassAutoGroupName, and extract a single shared local implementation from the equivalent GetFavoriteRosterRefreshNow and NormalizeFavoriteRosterRefreshName helpers. Update both the favorite roster refresh flow and the add-class auto-group flow to call the shared helpers without changing their behavior.
1645-1652: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
GetNumRaidMembersfor raid detection.
IsInRaidis not part of the WoW 3.3.5a API, so the type guard makesinRaidfalse in that client. UseGetNumRaidMembers()and setinRaidfrom that count before applying the party-count condition.🤖 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 `@Core/MultiBot.lua` around lines 1645 - 1652, Update the raid detection in the surrounding conversion logic to use GetNumRaidMembers() rather than IsInRaid, setting inRaid from the returned raid-member count before evaluating the partyCount >= 4 condition. Preserve the existing ConvertToRaid call and delay behavior.Source: Coding guidelines
Core/MultiBotComm.lua (3)
314-337: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a longer or progress-based deadline for global state transactions.
scheduleStateTimeoutapplies the same fixedSTATE_TIMEOUT_SECONDS = 5.0to a single-bot request and to a globalSTATESrequest. A global request can carry up toSTATE_MAX_BOTS = 128bots, and each bot needsSTATE_BEGIN, oneSTATE_ITEMper strategy, andSTATE_END. Addon message throughput is throttled by the client, so a large roster can exceed 5 seconds. When the timer fires,clearStateRequestdrops the request and all later frames are discarded, so the roster silently keeps stale states.Two options: use a larger timeout for global requests, or refresh the deadline whenever a frame for that token arrives.
♻️ Sketch: separate the global deadline
-local function scheduleStateTimeout(token) +local function scheduleStateTimeout(token, isGlobal) if not (MultiBot and type(MultiBot.TimerAfter) == "function") then return end - MultiBot.TimerAfter(STATE_TIMEOUT_SECONDS, function() + local timeout = isGlobal and STATES_TIMEOUT_SECONDS or STATE_TIMEOUT_SECONDS + MultiBot.TimerAfter(timeout, function()🤖 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 `@Core/MultiBotComm.lua` around lines 314 - 337, Update scheduleStateTimeout and its caller beginStateRequest so global state requests use a longer deadline than single-bot requests, while preserving STATE_TIMEOUT_SECONDS for single-bot requests. Ensure the timeout selection is based on the isGlobal flag and the resulting timer still clears the request and records STATE_TIMEOUT~token when the applicable deadline expires.
4239-4255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider handling the
STATErequest type in the sameERRbranch.The branch resolves a pending strategy mutation when
requestType == "STRATEGY". A protocol error for a framed state request leaves the entry instate.stateRequestsuntilscheduleStateTimeoutfires 5 seconds later. CallingclearStateRequestforrequestType == "STATE"would free the slot againstSTATE_MAX_ACTIVEimmediately.This branch also uses the lenient
urlDecodeField, while the framed handlers useurlDecodeFieldStrict. Using the strict decoder here keeps the parsing rules uniform.🤖 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 `@Core/MultiBotComm.lua` around lines 4239 - 4255, Update the ERR parsing branch around requestType to use urlDecodeFieldStrict for the request type and reason, and handle requestType == "STATE" by calling clearStateRequest with the parsed token and protocol-error reason. Preserve the existing STRATEGY finishStrategyMutationCommand behavior and error fields.
666-736: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Comm.RunStrategyCommandreturns a token string, not a boolean. Confirm the contract with all callers.The function returns
falsefor every rejection and a token string on success.Core/MultiBotEngine.luahandles this withtoken ~= false and token ~= nil, which is correct. Confirm no other caller assumes a boolean.Also consider recording the outbound token in
state.lastError-independent diagnostics is not needed; the current failure paths returnfalsewithout a reason, so the caller cannot distinguish "not connected" from "too many active commands". Settingstate.lastErroron the rejection paths, asComm.RequestStatedoes withSTATE_TOO_MANY_REQUESTS, would make the failures observable.🤖 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 `@Core/MultiBotComm.lua` around lines 666 - 736, Audit all callers of Comm.RunStrategyCommand and preserve its token-or-false contract, updating any caller that treats the result as a boolean; retain the existing token handling in MultiBotEngine.lua. Add distinct state.lastError diagnostics for each rejection path, including disconnected/incapable state, invalid inputs, missing changes, and active-command limits, following the pattern used by Comm.RequestState.
🤖 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 `@Core/MultiBot.lua`:
- Around line 2046-2048: Update the bracketed-value pass-through condition in
BuildBridgeDetailStoreValue to reject any value containing a comma, allowing
only bracketed strings without commas to return unchanged. Preserve the existing
handling for non-bracketed and comma-free values so cached metadata fields
remain correctly aligned when GetFavoriteCachedMetadata splits them.
- Around line 1461-1507: Update UpdateBridgeUnitButton so an unknown or missing
incoming class does not overwrite an existing resolved button.class; retain the
prior class instead. When preserving that known class, also skip the
unknown-class presentation update so the existing icon and metadata remain
unchanged, while still applying valid incoming class values normally.
- Around line 2034-2051: Update NormalizeBridgeDetailStoreGender to explicitly
map the bridge’s numeric gender convention, using 0 for male and 1 for female as
documented by the add-class command comment. Preserve the existing textual
mappings, bracketed-value handling, and "[?]" fallback.
In `@Core/MultiBotComm.lua`:
- Around line 1505-1506: Update Comm.MarkDisconnected to drain all entries in
state.strategyMutationCommands through finishStrategyMutationCommand with a
terminal result before clearing the table, so registered callbacks and waiting
callers are completed. Also reset stateFramingCapable and clear stateRequests,
stateActive, and stateLatestByBot alongside the existing capability reset,
preventing framed state requests until the next capability announcement.
- Around line 4101-4119: Update the STRATEGY_ACK parsing flow to decode field 8
with empty values allowed, so successful acknowledgements with reason "" pass
validation while timeout reasons such as "TIMEOUT" remain supported. Change the
allowEmpty argument in urlDecodeFieldStrict and preserve the existing validation
and STRATEGY_ACK_INVALID handling for genuinely missing or invalid fields.
- Around line 1843-1856: Update Comm.ApplyStateAbortPayload to decode fields[2]
as the targeted bot name and abort only that bot’s transaction for the token,
rather than calling abortStateRequest in a way that clears every matching
transaction. Preserve fields[1] as the token and fields[3] as the reason, and
leave other bot transactions for the same request active.
In `@Core/MultiBotEngine.lua`:
- Around line 500-523: Update _mbRunBridgeStrategyMutation to handle
acknowledgement failures such as no_match, failed, timeout, and error instead of
only reacting to result.matched > 0. Reuse the established FORMATION_ACK failure
behavior to notify the user or restore the affected UI state, while preserving
successful state-refresh handling for matched mutations and ensuring callers do
not leave optimistic button or toggle state applied after rejection.
---
Nitpick comments:
In `@Core/MultiBot.lua`:
- Around line 1510-1622: Remove the duplicate helper definitions
GetAddClassAutoGroupNow and NormalizeAddClassAutoGroupName, and extract a single
shared local implementation from the equivalent GetFavoriteRosterRefreshNow and
NormalizeFavoriteRosterRefreshName helpers. Update both the favorite roster
refresh flow and the add-class auto-group flow to call the shared helpers
without changing their behavior.
- Around line 1645-1652: Update the raid detection in the surrounding conversion
logic to use GetNumRaidMembers() rather than IsInRaid, setting inRaid from the
returned raid-member count before evaluating the partyCount >= 4 condition.
Preserve the existing ConvertToRaid call and delay behavior.
In `@Core/MultiBotComm.lua`:
- Around line 314-337: Update scheduleStateTimeout and its caller
beginStateRequest so global state requests use a longer deadline than single-bot
requests, while preserving STATE_TIMEOUT_SECONDS for single-bot requests. Ensure
the timeout selection is based on the isGlobal flag and the resulting timer
still clears the request and records STATE_TIMEOUT~token when the applicable
deadline expires.
- Around line 4239-4255: Update the ERR parsing branch around requestType to use
urlDecodeFieldStrict for the request type and reason, and handle requestType ==
"STATE" by calling clearStateRequest with the parsed token and protocol-error
reason. Preserve the existing STRATEGY finishStrategyMutationCommand behavior
and error fields.
- Around line 666-736: Audit all callers of Comm.RunStrategyCommand and preserve
its token-or-false contract, updating any caller that treats the result as a
boolean; retain the existing token handling in MultiBotEngine.lua. Add distinct
state.lastError diagnostics for each rejection path, including
disconnected/incapable state, invalid inputs, missing changes, and
active-command limits, following the pattern used by Comm.RequestState.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8de43895-ce94-4db2-929b-a105193e709a
📒 Files selected for processing (4)
Core/MultiBot.luaCore/MultiBotComm.luaCore/MultiBotEngine.luaUI/MultiBotUnitsRootUI.lua
Wishmaster117
deleted the
MultiBot-improve-bridge-synchronization,-strategy-controls-and-persistent-favorites
branch
August 7, 2026 19:36
This was referenced Aug 8, 2026
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.
Summary
This PR consolidates the current MultiBot addon-side work for the gradual migration from legacy chat-driven bot control toward the MultiBot Bridge architecture.
The main goals are to improve bridge synchronization, make strategy controls more reliable, and keep bot/favorite UI state consistent across connection changes and reloads.
Main changes
Bridge-backed bot state
classIdand level information to refresh:Strategy controls
Favorites
/reload.Favorite metadata synchronization
Male->[M]Female->[F]profile.botsSavedVariables structure instead of introducing another persistence table.Compatibility
Target environment:
No modern WoW Lua APIs are introduced.
Validation
Validated with:
The latest Favorites synchronization runtime test confirmed:
/reload: OK;Scope
This PR intentionally does not attempt to remove every remaining chat-based Playerbots command.
The migration is incremental: legacy chat mechanisms are kept where a validated bridge replacement is not available yet.
Summary by CodeRabbit
New Features
Bug Fixes