feat: Add method to fetch allowed actions on a tile, also misc changes#57
Conversation
📝 WalkthroughWalkthroughIntroduces a world-tile permission system: new bitflags, permission resolution methods, tracking of a world-lock tile index, conversion of lock access UIDs from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Map as WorldTileMap
participant Tile as Tile/Lock
participant Resolver as PermissionResolver
Client->>Map: get_tile_permission(x,y,user_id)
Map->>Map: fetch tile at (x,y)
alt tile has TileType::Lock (direct)
Map->>Resolver: check direct lock (owner_uid, access_uids, flags)
Resolver-->>Map: permission (FULL/NONE/BUILD/BREAK)
else tile has parent
Map->>Map: fetch parent tile
Map->>Resolver: resolve parent lock (flags, access_uids)
Resolver-->>Map: permission (FULL/BUILD/BREAK/NONE)
else standard tile
Map->>Map: check world_lock_index
alt world_lock exists
Map->>Tile: read world-lock TileType::Lock
Map->>Resolver: resolve world-lock (owner/access/public)
Resolver-->>Map: permission
else
Resolver-->>Map: default permission (FULL)
end
end
Map-->>Client: WorldTilePermission
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bot/core.rs`:
- Around line 2291-2293: has_access() currently only checks
TileType::Lock.access_uids which denies access to bots that own the tile/world;
update the TileType::Lock branch in has_access() to also return true when the
locking tile's owner UID equals bot_uid (e.g., tile.owner_uid == bot_uid) or
when the bot is the world owner (compare bot_uid to world.owner_uid), so owned
locks do not block collect() or find_path(); modify the condition that currently
uses access_uids.contains(&bot_uid) to include these ownership checks.
In `@src/world/mod_impl.rs`:
- Around line 349-350: TileType::Lock is constructed with access_count and
access_uids but access_count is left as the original wire count while
access_uids is filtered to only positive IDs; make them consistent by deriving
access_count from the filtered access_uids (e.g. access_count =
access_uids.len() as u32) instead of using the raw wire value, and apply the
same change in the other construction site that mirrors this logic (the second
block that handles access_count/access_uids).
- Line 111: The field world_lock_index on the WorldTileMap is only set in
WorldTileMap::parse() and can become stale when tiles are changed; update_tile()
and any bot tile-update handlers must update this cache so get_tile_permission()
never reads a stale index. Modify World::update_tile (and the tile mutation
handlers) to either recompute world_lock_index by scanning for the lock tile
after a mutation, or invalidate it (set to None) and ensure
WorldTileMap::parse()/accessors lazily recompute it when needed; keep references
to world_lock_index, WorldTileMap::parse(), World::update_tile(), and
get_tile_permission() to locate the fixes. Ensure any code paths that
convert/place/remove locks update or clear world_lock_index immediately after
the mutation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ecc4270-837d-4aa1-b744-9937bb1762f0
📒 Files selected for processing (3)
src/bot/core.rssrc/world/constants.rssrc/world/mod_impl.rs
| if let TileType::Lock { access_uids, .. } = &tile.tile_type { | ||
| if access_uids.contains(&(bot_uid as i32)) { | ||
| if access_uids.contains(&bot_uid) { | ||
| return true; |
There was a problem hiding this comment.
Owners still fail has_access().
This only checks access_uids, so a bot in a world it owns but has not explicitly added to the ACL still gets false. collect() and find_path() both use this flag, so owned locks remain path-blocking.
Suggested fix
- if let TileType::Lock { access_uids, .. } = &tile.tile_type {
- if access_uids.contains(&bot_uid) {
+ if let TileType::Lock { owner_uid, access_uids, .. } = &tile.tile_type {
+ if *owner_uid == bot_uid || access_uids.contains(&bot_uid) {
return true;
}
}📝 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.
| if let TileType::Lock { access_uids, .. } = &tile.tile_type { | |
| if access_uids.contains(&(bot_uid as i32)) { | |
| if access_uids.contains(&bot_uid) { | |
| return true; | |
| if let TileType::Lock { owner_uid, access_uids, .. } = &tile.tile_type { | |
| if *owner_uid == bot_uid || access_uids.contains(&bot_uid) { | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bot/core.rs` around lines 2291 - 2293, has_access() currently only checks
TileType::Lock.access_uids which denies access to bots that own the tile/world;
update the TileType::Lock branch in has_access() to also return true when the
locking tile's owner UID equals bot_uid (e.g., tile.owner_uid == bot_uid) or
when the bot is the world owner (compare bot_uid to world.owner_uid), so owned
locks do not block collect() or find_path(); modify the condition that currently
uses access_uids.contains(&bot_uid) to include these ownership checks.
| pub width: u32, | ||
| pub height: u32, | ||
| pub tiles: Vec<Tile>, | ||
| pub world_lock_index: Option<usize>, |
There was a problem hiding this comment.
Keep world_lock_index synchronized after tile mutations.
This cache is only populated during WorldTileMap::parse(), but tiles are later replaced by World::update_tile() and the bot’s tile-update handlers. After a world lock is placed, removed, or converted, get_tile_permission() can keep using a stale index or never see the new lock.
Also applies to: 120-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/world/mod_impl.rs` at line 111, The field world_lock_index on the
WorldTileMap is only set in WorldTileMap::parse() and can become stale when
tiles are changed; update_tile() and any bot tile-update handlers must update
this cache so get_tile_permission() never reads a stale index. Modify
World::update_tile (and the tile mutation handlers) to either recompute
world_lock_index by scanning for the lock tile after a mutation, or invalidate
it (set to None) and ensure WorldTileMap::parse()/accessors lazily recompute it
when needed; keep references to world_lock_index, WorldTileMap::parse(),
World::update_tile(), and get_tile_permission() to locate the fixes. Ensure any
code paths that convert/place/remove locks update or clear world_lock_index
immediately after the mutation.
There was a problem hiding this comment.
@badewen, understood! When you do get around to addressing it, the simplest approach would be to invalidate the cache in World::update_tile() by setting self.tile_map.world_lock_index = None after the tile is replaced — and then lazily recompute it on first access in get_tile_permission(). That way you don't need to do a full scan on every tile update, only when the world-lock index is actually queried. Feel free to reach out if you'd like help drafting the fix.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/world/mod_impl.rs (1)
49-59:⚠️ Potential issue | 🟠 Major
world_lock_indexcan still go stale here.This only refreshes the cache when the incoming tile is a world lock. If the cached world lock is removed/replaced, or
get_tile_mut()fails after Line 55,world_lock_indexkeeps pointing at stale data andget_tile_permission()will resolve permissions against the wrong tile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/world/mod_impl.rs` around lines 49 - 59, The code sets self.tile_map.world_lock_index prematurely and never clears it when a world-lock tile is removed or when get_tile_mut() fails; parse the Tile first but do NOT mutate world_lock_index before confirming the target tile exists and is updated: call self.get_tile_mut(x,y) and fail early if None, then assign *target_tile = tile, and after the assignment update world_lock_index—set Some(idx) only if the stored tile.type is Lock and fg not in NON_WORLDLOCK_TILE_IDS, otherwise if self.tile_map.world_lock_index == Some(idx) set it to None so the cache is cleared when a lock is replaced/removed (also ensure you don't change world_lock_index on parse failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/world/mod_impl.rs`:
- Around line 166-173: The index calculation does unchecked u32 math and can
wrap before casting to usize; change get_tile_parent to perform bounds-safe
indexing by converting operands to usize and using checked_mul/checked_add
(e.g., let x = x as usize; let y = y as usize; let idx =
y.checked_mul(self.width as usize).and_then(|v| v.checked_add(x)).and_then(|i|
if i < self.tiles.len() { Some(i) } else { None }) ), returning None on
overflow/out-of-bounds, then proceed to check TileFlags::HAS_PARENT and use
tile.parent_block as usize via safe indexing (self.tiles.get(parent_idx)). Apply
the same checked-index pattern to the other new tile helper that computes y *
self.width + x (the method referenced in the review).
- Around line 183-185: The pattern matches are attempting to move non-Copy enums
out of borrowed structs; change the match arms to match on references (e.g., use
&tile.tile_type, &world_lock.tile_type, &lock.tile_type) and adjust comparisons
to dereference any borrowed fields (for example compare user_id to *owner_uid
when owner_uid is a &Uid) so you don't move values out of &Tile; update the
three occurrences around the TileType::Lock matches (the block that returns
WorldTilePermission::FULL_ACCESS / NONE and the similar matches at the other
noted locations) accordingly.
---
Duplicate comments:
In `@src/world/mod_impl.rs`:
- Around line 49-59: The code sets self.tile_map.world_lock_index prematurely
and never clears it when a world-lock tile is removed or when get_tile_mut()
fails; parse the Tile first but do NOT mutate world_lock_index before confirming
the target tile exists and is updated: call self.get_tile_mut(x,y) and fail
early if None, then assign *target_tile = tile, and after the assignment update
world_lock_index—set Some(idx) only if the stored tile.type is Lock and fg not
in NON_WORLDLOCK_TILE_IDS, otherwise if self.tile_map.world_lock_index ==
Some(idx) set it to None so the cache is cleared when a lock is replaced/removed
(also ensure you don't change world_lock_index on parse failure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| pub fn get_tile_parent(&self, x: u32, y: u32) -> Option<&Tile> { | ||
| let idx = (y * self.width + x) as usize; | ||
| if idx >= self.tiles.len() { return None; } | ||
|
|
||
| let tile = self.tiles.get(idx).unwrap(); | ||
| if !tile.flags.contains(TileFlags::HAS_PARENT) { return None; } | ||
|
|
||
| self.tiles.get(tile.parent_block as usize) |
There was a problem hiding this comment.
Use checked index math in the new tile helpers.
Both methods compute y * self.width + x in u32. For oversized coordinates that can wrap before the cast to usize, so an invalid query may read a different tile instead of returning None / WorldTilePermission::NONE. Reusing the checked indexing pattern from World::get_tile() would avoid that edge case.
Also applies to: 177-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/world/mod_impl.rs` around lines 166 - 173, The index calculation does
unchecked u32 math and can wrap before casting to usize; change get_tile_parent
to perform bounds-safe indexing by converting operands to usize and using
checked_mul/checked_add (e.g., let x = x as usize; let y = y as usize; let idx =
y.checked_mul(self.width as usize).and_then(|v| v.checked_add(x)).and_then(|i|
if i < self.tiles.len() { Some(i) } else { None }) ), returning None on
overflow/out-of-bounds, then proceed to check TileFlags::HAS_PARENT and use
tile.parent_block as usize via safe indexing (self.tiles.get(parent_idx)). Apply
the same checked-index pattern to the other new tile helper that computes y *
self.width + x (the method referenced in the review).
| if let TileType::Lock { owner_uid, .. } = tile.tile_type { | ||
| return if user_id == owner_uid { WorldTilePermission::FULL_ACCESS } | ||
| else { WorldTilePermission::NONE }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust, does if let Enum::Variant { .. } = borrowed_struct.fieldcompile whenborrowed_struct: &Structandfield is a non-Copy enum? What is the correct way to pattern-match that field by reference?
💡 Result:
No, if let Enum::Variant { .. } = borrowed_struct.field does not compile when borrowed_struct: &Struct and field is a non-Copy enum. This attempts to move the non-Copy enum value out of the borrowed struct field, which is not allowed ("cannot move out of borrowed content"). The correct way to pattern-match the field by reference is to match on a reference to the field: if let Enum::Variant { .. } = &borrowed_struct.field { ... } This works because the pattern automatically dereferences the & reference (via reference patterns) and matches the enum value without moving it. Inside the if let block, you have an immutable borrow of the field limited to that scope, avoiding partial borrow issues in most cases. For struct-like variants, use { .. } to ignore fields or bind specific ones like { field1, .. }. For match, use: match &borrowed_struct.field { Enum::Variant { .. } => { ... }, _ => { ... }, } Modern Rust (1.26+) allows matching &enum_value directly without ref in subpatterns for many cases due to match ergonomics.
Citations:
- 1: https://stackoverflow.com/questions/36590549/what-is-the-syntax-to-match-on-a-reference-to-an-enum
- 2: https://doc.rust-lang.org/1.52.1/reference/patterns.html
- 3: https://doc.rust-lang.org/1.59.0/reference/patterns.html
- 4: https://stackoverflow.com/questions/30365055/cannot-move-out-of-borrowed-content-when-matching-an-enum
- 5: https://doc.rust-lang.org/1.76.0/reference/patterns.html
- 6: https://stackoverflow.com/questions/33925232/how-to-match-over-self-in-an-enum
- 7: https://stackoverflow.com/questions/28843931/get-an-enum-field-from-a-struct-cannot-move-out-of-borrowed-content
- 8: https://stackoverflow.com/questions/78738240/what-is-the-syntax-for-if-let-with-struct-like-enum-value
- 9: https://stackoverflow.com/questions/50775023/why-do-i-get-an-error-when-pattern-matching-a-struct-like-enum-variant-with-fiel
🏁 Script executed:
# Find and examine the file structure
find . -name "mod_impl.rs" -path "*/world/*" | head -5Repository: CLOEI/Mori
Length of output: 77
🏁 Script executed:
# Look at the specific lines mentioned in the review
sed -n '180,220p' src/world/mod_impl.rs | cat -nRepository: CLOEI/Mori
Length of output: 2134
🏁 Script executed:
# Find the TileType definition to check derives
rg "enum TileType|pub enum TileType" -A 15Repository: CLOEI/Mori
Length of output: 1481
🏁 Script executed:
# Check the function signature and context around line 183
sed -n '170,230p' src/world/mod_impl.rs | cat -nRepository: CLOEI/Mori
Length of output: 3303
🏁 Script executed:
# Find the complete TileType enum definition with derives
rg "enum TileType" -B 5 -A 30 src/world/mod_impl.rsRepository: CLOEI/Mori
Length of output: 769
🏁 Script executed:
# Check if the code actually compiles by looking at Cargo.toml and trying to understand the project structure
head -20 Cargo.tomlRepository: CLOEI/Mori
Length of output: 590
🏁 Script executed:
# Verify what `tile` is by looking at the exact types in the function
sed -n '175,195p' src/world/mod_impl.rs | cat -nRepository: CLOEI/Mori
Length of output: 1197
Match TileType by reference in these pattern matches.
tile, world_lock, and lock are all borrowed references (&Tile). Matching directly on .tile_type attempts to move a non-Copy enum out of borrowed content, which will not compile. Match on &tile.tile_type, &world_lock.tile_type, and &lock.tile_type instead, and dereference copied fields where needed.
Suggested fix
- if let TileType::Lock { owner_uid, .. } = tile.tile_type {
- return if user_id == owner_uid { WorldTilePermission::FULL_ACCESS }
+ if let TileType::Lock { owner_uid, .. } = &tile.tile_type {
+ return if user_id == *owner_uid { WorldTilePermission::FULL_ACCESS }
else { WorldTilePermission::NONE };
}
@@
- let TileType::Lock { owner_uid, ref access_uids, .. } = world_lock.tile_type else {
+ let TileType::Lock { owner_uid, access_uids, .. } = &world_lock.tile_type else {
eprintln!("[world] world_lock tile type is not TileType::Lock. {:?}", world_lock.tile_type);
return WorldTilePermission::NONE;
};
- if user_id == owner_uid {
+ if user_id == *owner_uid {
return WorldTilePermission::FULL_ACCESS;
}
@@
- let TileType::Lock { settings, owner_uid, ref access_uids, .. } = lock.tile_type else {
+ let TileType::Lock { settings, owner_uid, access_uids, .. } = &lock.tile_type else {
eprintln!("[world] parent_tile type is not TileType::Lock. {:?}", lock.tile_type);
return WorldTilePermission::NONE;
};
- if user_id == owner_uid {
+ if user_id == *owner_uid {
return WorldTilePermission::FULL_ACCESS;
}
- let flags = TileLockFlags::from_bits_retain(settings);
+ let flags = TileLockFlags::from_bits_retain(*settings);Also applies to: 192-195, 212-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/world/mod_impl.rs` around lines 183 - 185, The pattern matches are
attempting to move non-Copy enums out of borrowed structs; change the match arms
to match on references (e.g., use &tile.tile_type, &world_lock.tile_type,
&lock.tile_type) and adjust comparisons to dereference any borrowed fields (for
example compare user_id to *owner_uid when owner_uid is a &Uid) so you don't
move values out of &Tile; update the three occurrences around the TileType::Lock
matches (the block that returns WorldTilePermission::FULL_ACCESS / NONE and the
similar matches at the other noted locations) accordingly.
|
Thanks for the hard work! Really appreciate it! |
2 participants
The next pathfinder will depend on this method (which should be finished soon)
Kept forgetting to switch branch ;-;
Summary by CodeRabbit
Bug Fixes
New Features