Skip to content

fix: bind research menus to their own station - #7

Merged
XxFran10xX merged 1 commit into
mainfrom
fix/research-per-station-menus
Sep 26, 2026
Merged

XxFran10xX merged 1 commit into
mainfrom
fix/research-per-station-menus

Conversation

@XxFran10xX

@XxFran10xX XxFran10xX commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

A player who owned more than one research station could act on the wrong one (reported by Dref: "Research station/data needs to be separated per station so a user can have multiple stations at once").

Station data was already stored per lectern; the problem was the menu session. Opening the scrap confirmation fires an inventory close for the station menu, which cleared the player's open-station entry. Clicks then fell back to the first station the player owned, so Scrap → Yes on one station could delete another station's project, and No reopened the wrong station.

Each station menu and scrap confirmation now carries its lectern location in a StationMenuHolder, and clicks resolve the station from the menu that is open. The per-player open-station map and its first-owned-station fallback are removed. Breaking or scrapping a station only closes that station's own menu.

Validation: mvn clean verify passes (5 tests). The new MultipleStationsTest fails on the old code (the other station is scrapped) and passes with the fix. The jar loaded cleanly on TFMCDev01 with its full config and no Research warnings. No in-game two-station playtest yet.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Research menus now stay associated with the station they were opened for, preventing actions in one station’s menu from affecting another.
    • Scrap confirmations apply only to the selected station, and the corresponding menu closes when that station is scrapped.
    • Menu actions no longer fall back to another owned station when the selected station can’t be found or accessed.

A player who owned more than one research station could act on the wrong
one. Opening the scrap confirmation fired an inventory close for the
station menu, which cleared the player's open-station entry; clicks then
fell back to the first station the player owned, so confirming a scrap
could delete a different station's project.

Each station and scrap-confirm menu now carries its lectern location in
an InventoryHolder, and clicks resolve the station from the menu that is
open. The per-player open-station map and its first-owned-station
fallback are removed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1cacb5f4-6aed-4a62-a2b7-0ff7235b87c8

📥 Commits

Reviewing files that changed from the base of the PR and between 0f8c47e and ac1bd9a.

📒 Files selected for processing (5)
  • src/main/java/net/tfminecraft/research/manager/InventoryManager.java
  • src/main/java/net/tfminecraft/research/manager/ResearchManager.java
  • src/main/java/net/tfminecraft/research/manager/StationMenuHolder.java
  • src/test/java/net/tfminecraft/research/manager/MultipleStationsTest.java
  • src/test/java/net/tfminecraft/research/manager/ResearchRefundTest.java

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Research menu inventories now identify their station through a StationMenuHolder. ResearchManager resolves menu actions from the open inventory, checks station ownership, and closes a menu during scrapping only when it identifies the station being scrapped.

Changes

Station Menu Ownership

Layer / File(s) Summary
Attach station locations to menus
src/main/java/net/tfminecraft/research/manager/StationMenuHolder.java, src/main/java/net/tfminecraft/research/manager/InventoryManager.java
StationMenuHolder stores a station location and inventory. Main and scrap-confirm inventories use a holder associated with their station.
Resolve menu actions and scrap targets
src/main/java/net/tfminecraft/research/manager/ResearchManager.java, src/test/java/net/tfminecraft/research/manager/MultipleStationsTest.java, src/test/java/net/tfminecraft/research/manager/ResearchRefundTest.java
ResearchManager resolves stations from menu holders and checks ownership. Scrapping closes a menu only when its holder identifies the station. Tests cover scrapping one of two owned stations and use a holder in refund-test setup.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: ryanbarlow97

Merge Risk: ⚪ Minimal · up to ac1bd

No actionable issue remains before merge based on the supplied evidence. An in-game two-station check has not been reported.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ac1bd

The menu now stays bound to its station, and ownership is checked before menu actions run. This reduces the risk of scrapping the wrong station. No new cross-player menu access is evident, though live two-station behavior and block-break protection remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For menu-driven actions, the independently reachable target is limited to an existing station owned by the clicking player and identified by that menu. The previous fallback could instead select another station owned by the same player.

Trust Boundaries and Controls

  • observed — The player-controlled inventory-click path cancels clicks in research menus, then requires a holder-derived station that exists and has the clicking player’s UUID before dispatching station actions.
  • observed — Block breaking is a separate route to station deletion. Its handler has no local breaker-ownership check; the evidence does not establish whether server-side protection authorizes the break. This route predates the changed menu binding and is not established as a PR-introduced exposure.

Resilience and Maintainability Implications

  • inferred — After a station is removed, a repeated click using its old menu cannot resolve that station from the in-memory station list. Menu closure also leaves an unrelated station menu open rather than closing it as part of another station’s deletion.

Hardening Proposals

  • proposed — Verify and document the server policy that authorizes block breaks before station deletion, or enforce that authorization locally if no such policy exists. This is a separate, unverified pre-existing path, not a finding introduced by the menu change.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.70% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 5 files. 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 clearly and concisely describes the main change: binding research menus to their corresponding station.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

A rabbit checks the menu’s place,
And finds the station by its trace.
The right one leaves, the first stays near,
Its holder keeps the target clear.
Soft paws approve the tested way,
Then hop back through the fields to play.

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

@XxFran10xX
XxFran10xX merged commit a364dc3 into main Sep 26, 2026
2 checks passed
@XxFran10xX
XxFran10xX deleted the fix/research-per-station-menus branch September 26, 2026 19:38
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