Skip to content

Centralize stall placement validation and fix undoSell bug#167

Merged
candour merged 2 commits into
mainfrom
fix-undosell-validation-11469130489472164888
May 20, 2026
Merged

Centralize stall placement validation and fix undoSell bug#167
candour merged 2 commits into
mainfrom
fix-undosell-validation-11469130489472164888

Conversation

@candour

@candour candour commented May 20, 2026

Copy link
Copy Markdown
Owner

I have fixed a bug in the undoSell mechanic where players could restore a sold stall even if it blocked the main path or left a Tray Return Uncle without any walkable neighbors.

To solve this, I centralized the stall placement validation logic into a new private method validateStallPlacement within MainViewModel.kt. This method now handles all placement-related checks:

  1. Proximity to enemies (cannot build on or immediately in front of moving enemies).
  2. Path existence (cannot block the only path to the goal).
  3. Tray Return Uncle rule (every Uncle must have at least one adjacent walkable tile).
  4. Enemy re-pathing (all current enemies must still be able to find a path to the goal).

Both onCellClick (for new stalls) and undoSell (for restoring sold stalls) now call this centralized validation. I also added a comprehensive unit test suite UndoPathBlockTest.kt to cover these scenarios and updated existing tests to ensure they provide valid board configurations. Finally, I documented the fix in fixes.md.


PR created automatically by Jules for task 11469130489472164888 started by @candour

Summary by CodeRabbit

  • Bug Fixes

    • Prevented restoring or placing stalls that would block the main enemy path or violate neighbor/walkability rules, ensuring consistent game flow and gold handling.
  • Tests

    • Added tests covering undo/restore scenarios that would block paths or violate special-tile neighbor constraints.
  • Documentation

    • Added a bugfix entry summarizing the undo/placement validation improvement.

Review Change Stack

Centralized stall placement validation logic into `validateStallPlacement` in `MainViewModel.kt`. This ensures that all rules—including enemy proximity, path blocking, and the 'Tray Return Uncle' neighbor rule—are consistently enforced when placing a new stall or using the 'undo sell' feature. Previously, `undoSell` lacked these checks, which could lead to illegal game states.

Changes:
- Added `validateStallPlacement` helper in `MainViewModel.kt`.
- Updated `onCellClick` and `undoSell` to use the helper.
- Added `UndoPathBlockTest.kt` to verify pathing and Uncle rule enforcement during undo.
- Updated `UndoUncleTeleportTest.kt` to comply with stricter placement rules.
- Logged fix as FIX-014 in `fixes.md`.

Co-authored-by: candour <4670475+candour@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a7cd9166-72a3-45e4-bb90-080454c16be5

📥 Commits

Reviewing files that changed from the base of the PR and between b7ea40d and 54f3ec1.

📒 Files selected for processing (2)
  • app/src/test/java/com/messark/hawker/UndoPathBlockTest.kt
  • fixes.md
✅ Files skipped from review due to trivial changes (1)
  • fixes.md

📝 Walkthrough

Walkthrough

MainViewModel now centralizes stall placement validation via a reusable validateStallPlacement(...) helper that enforces enemy proximity, path feasibility, and Tray Return Uncle constraints. Both onCellClick stall placement and undoSell stall restoration now use this helper instead of duplicating logic, with comprehensive new tests and refined existing tests verifying the validation behavior.

Changes

Stall Placement Validation Centralization

Layer / File(s) Summary
Centralized placement validation helper
app/src/main/java/com/messark/hawker/MainViewModel.kt
New validateStallPlacement(...) function computes and returns a blocked coordinate set only when placement is allowed: no enemy is adjacent to or targeting the tile, Tray Return Uncle neighbor rule is satisfied, and pathfinding remains feasible for both the main path and enemy repathing.
Integration into stall placement and undo flows
app/src/main/java/com/messark/hawker/MainViewModel.kt
onCellClick stall placement and undoSell stall restoration both call validateStallPlacement(...), recalculate enemy paths using the returned blocked set, update gold/state, and clear lastSoldStall on successful validation.
Comprehensive test coverage for validation enforcement
app/src/test/java/com/messark/hawker/UndoPathBlockTest.kt, app/src/test/java/com/messark/hawker/UndoUncleTeleportTest.kt
UndoPathBlockTest (new) verifies that undoSell() rejects stall restoration when it would block the main path or violate Tray Return Uncle constraints, using a StandardTestDispatcher for deterministic coroutines. UndoUncleTeleportTest (updated) adds extra floor tiles to the game board and replaces non-null assertions with explicit assertNotNull calls.
Bugfix documentation
fixes.md
FIX-014 entry documents the undoSell validation fix that centralizes pathing and Tray Return Uncle rule enforcement into validateStallPlacement.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • candour/towerpower#145: Modifies Tray Return Uncle placement validation and related tests; overlaps with this PR's centralization of that rule.
  • candour/towerpower#70: Earlier changes to Tray Return Uncle "last empty space" adjacency rules that this PR consolidates into validateStallPlacement.
  • candour/towerpower#117: Adjusts which neighbor tile types are considered valid for the uncle rule; related to the same placement constraints centralized here.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: centralizing stall placement validation and fixing the undoSell bug.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-undosell-validation-11469130489472164888

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2c4ca824-1daa-4060-adec-7e5e42dc4840

📥 Commits

Reviewing files that changed from the base of the PR and between 6d96c81 and b7ea40d.

📒 Files selected for processing (4)
  • app/src/main/java/com/messark/hawker/MainViewModel.kt
  • app/src/test/java/com/messark/hawker/UndoPathBlockTest.kt
  • app/src/test/java/com/messark/hawker/UndoUncleTeleportTest.kt
  • fixes.md

Comment thread app/src/test/java/com/messark/hawker/UndoPathBlockTest.kt
Comment thread fixes.md Outdated
@github-actions

Copy link
Copy Markdown

Build Successful! 🚀

Download APK

Note: This link will be removed when the PR is closed.

@candour

candour commented May 20, 2026

Copy link
Copy Markdown
Owner Author

[MAJOR] ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Second test does not isolate the Uncle-rule failure path.
Line 104 and Line 105 use start/end coordinates that are not present in hexes, so undoSell() can be rejected by path validation even if the Tray Return Uncle check regresses. This can produce a false-positive test pass.

Suggested test setup adjustment ...

[MINOR] ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Fix log date is in the future for a resolved item.
Line 47 records FIX-014 as Resolved on 2026-07-10, which is a future date relative to current project timeline. Please use the actual resolution date (or mark as planned) to keep release notes auditable.

@candour
candour enabled auto-merge (squash) May 20, 2026 06:19
@google-labs-jules

Copy link
Copy Markdown
Contributor

[MAJOR] ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Second test does not isolate the Uncle-rule failure path.
Line 104 and Line 105 use start/end coordinates that are not present in hexes, so undoSell() can be rejected by path validation even if the Tray Return Uncle check regresses. This can produce a false-positive test pass.

Suggested test setup adjustment ...

[MINOR] ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Fix log date is in the future for a resolved item.
Line 47 records FIX-014 as Resolved on 2026-07-10, which is a future date relative to current project timeline. Please use the actual resolution date (or mark as planned) to keep release notes auditable.

Thank you for the feedback. I have updated the undoSell should not be allowed if it violates Tray Return Uncle rule test in UndoPathBlockTest.kt to include the start and end positions in the hexes map, ensuring that the test correctly isolates the Tray Return Uncle rule violation. I also updated fixes.md to use the correct resolution date.

- Updated `UndoPathBlockTest.kt` to include start and end positions in the hex map for the Tray Return Uncle rule test, ensuring it isolates the intended failure path.
- Corrected the resolution date for FIX-014 in `fixes.md` to the actual date (2026-05-20).

Co-authored-by: candour <4670475+candour@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Build Successful! 🚀

Download APK

Note: This link will be removed when the PR is closed.

@candour
candour merged commit d592660 into main May 20, 2026
3 checks passed
@candour
candour deleted the fix-undosell-validation-11469130489472164888 branch May 20, 2026 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant