fix(container): handle depot and batch page state - #3995
Conversation
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConst-correctness is added to ChangesContainer Management and Depot Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/lua/functions/core/game/batch_update_functions.cppsrc/lua/functions/core/game/batch_update_functions.cpp:10:10: fatal error: 'lua/functions/core/game/batch_update_functions.hpp' file not found ... [truncated 1251 characters] ... "-internal-isystem" "/usr/local/include" "-internal-isystem" 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request marks Player::isNearDepotBox() as a const method, updates the depot chest closing logic to check if the player is near a depot box, and adjusts the container batch update logic to dynamically correct out-of-bounds indices. Additionally, it fixes the Lua garbage collection registration for BatchUpdate to use luaSharedPtrGarbageCollection. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Follow-up fix to the recent container batching work, addressing edge cases in container window lifecycle and Lua userdata ownership. It ensures depot-related container windows don’t remain open after losing depot access, and hardens batched container refresh behavior when the container size changes.
Changes:
- Switch
BatchUpdate.delete()to the typed shared-userdata shared_ptr finalizer (luaSharedPtrGarbageCollection), aligning with the shared userdata ownership contract. - Make
Player::isNearDepotBox()constand use it to close depot chest windows when the player is no longer near a depot tile. - Clamp/adjust the container “first index” during batched container sends to avoid resending an out-of-range index after removals shrink the container.
Tests: Not run (review only).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/lua/functions/core/game/batch_update_functions.cpp |
Aligns BatchUpdate.delete() with typed shared-userdata shared_ptr GC behavior. |
src/creatures/players/player.hpp |
Makes isNearDepotBox() const to support calls from const contexts. |
src/creatures/players/player.cpp |
Fixes batched container resend index when size shrinks; closes depot chest containers when player moves away from depot access. |
dudantas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/lua-api/lua_api.d.lua`:
- Around line 104-105: The LuaDoc for BatchUpdate.delete currently declares
"---@return any" but the binding uses
Lua::luaSharedPtrGarbageCollection<BatchUpdate> which returns no Lua values
(effectively nil); update the documentation for the function BatchUpdate.delete
to "---@return nil" to match other delete methods (e.g., Condition.delete,
NetworkMessage.delete) and the actual binding behavior.
In `@docs/lua-api/lua_api.json`:
- Around line 172-175: The docs entry for BatchUpdate.delete incorrectly
declares "return": "any"; update the JSON so the return matches the actual
implementation (luaSharedPtrGarbageCollection<BatchUpdate> via
lua_functions_loader.hpp) which returns no Lua values—change the "return" field
to your project’s no-return convention (e.g., "nil" or explicit "void"/"none"
used elsewhere) for the "delete" function in the JSON to accurately reflect
runtime behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6f66267-a6f8-4d03-92ca-61f1a49a929b
📒 Files selected for processing (3)
docs/lua-api/lua_api.d.luadocs/lua-api/lua_api.jsondocs/lua-api/lua_api.md
✅ Files skipped from review due to trivial changes (1)
- docs/lua-api/lua_api.md
|
Merges 4 upstream commits from main: - fix(container) opentibiabr#3995: page-index bounds clamp in sendBatchUpdateContainer, isNearDepotBox() → const, shouldCloseContainer depot proximity fix - fix(teleport) opentibiabr#3818: anti-recursive teleport guard in InternalGame namespace - feat: release workflow + MyAAC client 1501→1511 - chore: release metadata 3.5.0→3.6.0 SOLID boundary preserved: - Page-index fix ported to PlayerStashComponent::sendBatchUpdateContainer (not re-inlined in player.cpp) - isNearDepotBox() const propagated through PlayerStashComponent - Teleport guard added to Game::internalTeleport facade; actual teleport logic stays in MovementService::internalTeleport - 3 regression tests added (isNearDepotBox const, sendBatchUpdate null guard) Tests: 579/579 passing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This is a small functional follow-up to the container batching work.
It fixes edge cases exposed by batched container refreshes:
firstIndexafter removals changed the container size;BatchUpdate:delete()now uses the typed shared userdata finalizer, matching the Lua userdata ownership contract.Related context:
This PR does not add new performance behavior or change container ownership.
Summary by CodeRabbit