fix: Fix some conduit bugs #1148

2025-09-27T21:11:34Z

Rover656 commented

2025-09-27 21:11:34 +00:00

(Migrated from github.com)

Description

Fixes:

Crash when connecting ME conduit to Dense ME conduit.
Breaking conduits occasionally failing

TODO

Test all changes thoroughly - esp. in survival
Remove some unused functionality in conduit API

Breaking Changes

A handful of API breaks, probably.

Checklist

My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you).
I have made corresponding changes to the documentation.
My changes are ready for review from a contributor.

Summary by CodeRabbit

New Features
- Unified, more consistent rules for conduit connection, replacement and upgrades across conduit types.
- Server‑authorised conduit/facade removal flows with client visual feedback and particle effects.
Bug Fixes
- Reliable item drops for conduit and facade removal respecting creative/instabuild.
- Correct light updates after facade removal.
- Prevents invalid connections between incompatible conduits.
Chores
- Network protocol version bumped to support new interactions.

# Description Fixes: * Crash when connecting ME conduit to Dense ME conduit. * Breaking conduits occasionally failing  # TODO - [x] Test all changes thoroughly - esp. in survival - [x] Remove some unused functionality in conduit API # Breaking Changes A handful of API breaks, probably.  # Checklist - [x] My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you). - [x] I have made corresponding changes to the documentation. - [x] My changes are ready for review from a contributor.   ## Summary by CodeRabbit - New Features - Unified, more consistent rules for conduit connection, replacement and upgrades across conduit types. - Server‑authorised conduit/facade removal flows with client visual feedback and particle effects. - Bug Fixes - Reliable item drops for conduit and facade removal respecting creative/instabuild. - Correct light updates after facade removal. - Prevents invalid connections between incompatible conduits. - Chores - Network protocol version bumped to support new interactions.

coderabbitai[bot] commented

2025-09-27 21:11:41 +00:00

(Migrated from github.com)

Walkthrough

Refactors conduit APIs to use strongly typed conduit parameters, adds ConduitUtility for type-safe comparisons, updates bundle and block-entity logic for compatibility and consumer-based drops, introduces client-to-server packets and server handlers for breaking conduits/removing facades, and updates integration/drop handling and some GUI checks.

Changes

Cohort / File(s)	Summary of changes
Core API & utilities `enderio-conduits/src/main/java/com/enderio/conduits/api/Conduit.java`, `enderio-conduits/src/main/java/com/enderio/conduits/api/ConduitUtility.java`, `enderio-conduits/src/main/java/com/enderio/conduits/api/bundle/ConduitBundle.java`	Replace Holder-based bundle/replace APIs with typed variants: add `canReplaceConduit(TConduit)`, change `canConnectToConduit(Holder<...>)` → `canConnectToConduit(TConduit)`, remove legacy `canBeInSameBundle`/`canBeReplacedBy`; add `ConduitUtility` helpers; rename/extend bundle API for compatibility and consumer-based removal.
Conduit implementations `enderio-conduits-modded/.../appeng/MEConduit.java`, `enderio-conduits-modded/.../mekanism/chemical/ChemicalConduit.java`, `enderio-conduits/.../type/energy/EnergyConduit.java`, `enderio-conduits/.../type/fluid/FluidConduit.java`	Update signatures to typed conduit parameters; consolidate replacement logic into `canReplaceConduit(...)` using direct compareTo; simplify connectivity checks (e.g., MEConduit now compares color directly).
Bundle & block behaviour `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java`	Use `ConduitUtility.canConnectConduits(...)` for compatibility checks, add `getCompatibleConduit(...)`, change `removeConduit` to accept `Consumer<ItemStack>` for server-side drop handling, split client/server break handling and route removal via network packets, add model/consistency hooks and unify drop logic.
Networking & server handlers `enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SBreakConduitPacket.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SRemoveConduitFacadePacket.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitNetwork.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java`	Add two C2S packet records with codecs, register them (protocol bump to "2"), and add server handlers that validate interactions, fire block-break events, handle cancellations, drop items, clear facades, and remove conduits.
Integration & minor UI `enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/item/ItemConduit.java`	Update Ultimine handler to spawn ItemEntity via helper and use new removal consumer; replace `hasConduitByType` calls with `hasConduitOfType` in GUI logic.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Player
  participant Client
  participant Server
  participant BundleBE as ConduitBundleBlockEntity
  participant World

  Player->>Client: Attempt break (conduit or facade)
  alt Remove facade
    Client->>Server: C2SRemoveConduitFacadePacket(pos)
    Server->>World: Validate & fire BlockBreakEvent
    alt Event not cancelled
      Server->>BundleBE: clear facade, drop facade via consumer
      Server->>World: update lighting, destroy bundle if empty
    else Event cancelled
      Server-->>Client: Resync block state
    end
  else Break conduit
    Client->>Server: C2SBreakConduitPacket(pos, conduit)
    Server->>World: Validate & fire BlockBreakEvent
    alt Event not cancelled
      Server->>BundleBE: removeConduit(conduit, droppedItemConsumer)
      Note right of BundleBE: droppedItemConsumer spawns ItemEntity (unless instabuild)
      Server->>World: destroy bundle if empty
    else Event cancelled
      Server-->>Client: Resync block state
    end
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Team-EnderIO/EnderIO#1126 — Adds/changes networking and server payload handling; overlaps with new C2S packet registrations and handlers.
Team-EnderIO/EnderIO#1097 — Changes FTB Ultimine integration and conduit break/drop flows; aligns with updated drop/removal logic.
Team-EnderIO/EnderIO#1066 — Larger conduit API refactor touching typed conduit parameters and connectivity semantics; strongly related to these API changes.

Suggested labels

Area-Conduits, Area-Backend, Type-Enhancement, MC-1.21.1

Poem

I nibble nibbles on typed wires bright,
Packets hop home through the moonlit night.
Bundles tidy, facades softly fall,
A rabbit claps for every small call.
Hooray for drops that land just right — 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The description follows the template headings but leaves the Breaking Changes section vague (“A handful of API breaks, probably.”) without listing specific removed or altered methods and retains a TODO section that should have been removed once completed. It does provide a brief summary of fixes, but it omits detailed testing instructions and precise API breaking-change details.	Please enumerate each API method added, removed, or changed under Breaking Changes, remove the completed TODO section if this is not a draft, and include a clear list of test scenarios or validation steps for reviewers.
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.13% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title Check	❓ Inconclusive	The title “fix: Fix some conduit bugs” is overly generic and does not clearly describe the primary change or scope of this extensive API and behavioural overhaul across multiple conduit types and network handlers. It uses non-descriptive terms that do not convey the specific issues addressed or the key modules impacted.	Please update the title to summarise the most significant change—such as the introduction of generic conduit utilities and streamlined replacement/connect logic—and reference the specific subsystem or bug being fixed.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/conduit-issues

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1b7af28ce and bd7d21a019.

📒 Files selected for processing (2)

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (3 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (22 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-09-28T18:33:28.286Z

Learnt from: Rover656
PR: Team-EnderIO/EnderIO#1148
File: enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java:116-190
Timestamp: 2025-09-28T18:33:28.286Z
Learning: The canInteractWithBlock(pos, 1.0) method in Minecraft ServerPlayer does not incorrectly square the distance parameter as I initially suggested. The user Rover656 corrected this misunderstanding, indicating that using 1.0 as the distance parameter may be appropriate for conduit breaking functionality.

Applied to files:

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java

🧬 Code graph analysis (2)

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (1)

enderio-conduits/src/main/java/com/enderio/conduits/api/ConduitUtility.java (1)

ConduitUtility (7-29)

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (2)

enderio-conduits/src/main/java/com/enderio/conduits/client/particle/ConduitBreakParticle.java (1)

ConduitBreakParticle (21-147)

enderio-conduits/src/main/java/com/enderio/conduits/client/model/conduit/facades/FacadeUtil.java (1)

FacadeUtil (8-20)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Run Tests
GitHub Check: Analyze (java-kotlin)

🔇 Additional comments (8)

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (2)

18-19: LGTM! Imports align with network-based conduit operations.

The new imports support the client-server split approach for conduit breaking and facade removal operations.

Also applies to: 63-63, 66-66

264-344: LGTM! Well-implemented client-server split for conduit breaking.

The refactored method properly separates client-side visual feedback from server-side authoritative operations:

Simple cases (empty/single conduit bundles) are handled immediately on both sides

Complex cases delegate to network packets, preventing unauthorised client-side mutations

Visual feedback is preserved via particle effects on the client

Ray hit casting now correctly validates the hit type before casting (addressing the previous review concern)

The approach ensures server authority over conduit state while maintaining responsive client-side feedback.

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (6)

9-9: LGTM! Imports support the new compatibility and drop handling approaches.

The ConduitUtility and Consumer imports align with the refactored conduit compatibility checking and consumer-based drop handling.

Also applies to: 49-49

404-429: Improved conduit compatibility API with centralised logic.

The refactored methods provide clearer semantics:

hasCompatibleConduit() uses ConduitUtility.canConnectConduits() for connection compatibility

hasConduitOfType() checks for exact type matches

getCompatibleConduit() enables finding compatible neighbour conduits

Centralising compatibility logic in ConduitUtility improves consistency across the codebase.

448-448: Excellent consistent migration to centralised compatibility checks.

The systematic replacement of ad-hoc compatibility logic with ConduitUtility methods creates a unified approach:

canConduitReplace() for upgrade scenarios

canConnectConduits() for connection compatibility

Consistent usage across connection, addition, and replacement flows

This improves maintainability by centralising conduit compatibility rules.

Also applies to: 458-460, 483-483, 541-541, 596-597, 834-852, 881-894, 923-930, 947-947

1506-1533: Good defensive synchronisation for cancelled conduit operations.

The ensureModelsAreCorrect() method handles edge cases where server-side operations (like conduit removal) are cancelled, ensuring client-side visual state remains consistent. This is particularly important with the new network-based conduit breaking approach.

300-304: Supporting changes look good.

The mechanical updates properly adopt the new APIs:

Wrench removal uses new consumer-based drop handling

dropItem() method uses cleaner getCenter() approach

Consistent use of hasConduitStrict() for precise conduit checking

Appropriate model synchronisation calls added

Comments provide useful context about future cleanup plans.

Also applies to: 708-710, 715-715, 1301-1301, 1182-1182, 1503-1503, 1025-1025, 1297-1297, 1722-1722

606-606: Creative mode handling verified. All removeConduit(...) calls are guarded by player.getAbilities().instabuild checks, so drops are correctly suppressed in creative mode.

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.}

## Walkthrough Refactors conduit APIs to use strongly typed conduit parameters, adds ConduitUtility for type-safe comparisons, updates bundle and block-entity logic for compatibility and consumer-based drops, introduces client-to-server packets and server handlers for breaking conduits/removing facades, and updates integration/drop handling and some GUI checks. ## Changes | Cohort / File(s) | Summary of changes | | --- | --- | | **Core API & utilities** `enderio-conduits/src/main/java/com/enderio/conduits/api/Conduit.java`, `enderio-conduits/src/main/java/com/enderio/conduits/api/ConduitUtility.java`, `enderio-conduits/src/main/java/com/enderio/conduits/api/bundle/ConduitBundle.java` | Replace Holder-based bundle/replace APIs with typed variants: add `canReplaceConduit(TConduit)`, change `canConnectToConduit(Holder<...>)` → `canConnectToConduit(TConduit)`, remove legacy `canBeInSameBundle`/`canBeReplacedBy`; add `ConduitUtility` helpers; rename/extend bundle API for compatibility and consumer-based removal. | | **Conduit implementations** `enderio-conduits-modded/.../appeng/MEConduit.java`, `enderio-conduits-modded/.../mekanism/chemical/ChemicalConduit.java`, `enderio-conduits/.../type/energy/EnergyConduit.java`, `enderio-conduits/.../type/fluid/FluidConduit.java` | Update signatures to typed conduit parameters; consolidate replacement logic into `canReplaceConduit(...)` using direct compareTo; simplify connectivity checks (e.g., MEConduit now compares color directly). | | **Bundle & block behaviour** `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java` | Use `ConduitUtility.canConnectConduits(...)` for compatibility checks, add `getCompatibleConduit(...)`, change `removeConduit` to accept `Consumer<ItemStack>` for server-side drop handling, split client/server break handling and route removal via network packets, add model/consistency hooks and unify drop logic. | | **Networking & server handlers** `enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SBreakConduitPacket.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SRemoveConduitFacadePacket.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitNetwork.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java` | Add two C2S packet records with codecs, register them (protocol bump to "2"), and add server handlers that validate interactions, fire block-break events, handle cancellations, drop items, clear facades, and remove conduits. | | **Integration & minor UI** `enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java`, `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/item/ItemConduit.java` | Update Ultimine handler to spawn ItemEntity via helper and use new removal consumer; replace `hasConduitByType` calls with `hasConduitOfType` in GUI logic. | ## Sequence Diagram(s) ```mermaid sequenceDiagram autonumber actor Player participant Client participant Server participant BundleBE as ConduitBundleBlockEntity participant World Player->>Client: Attempt break (conduit or facade) alt Remove facade Client->>Server: C2SRemoveConduitFacadePacket(pos) Server->>World: Validate & fire BlockBreakEvent alt Event not cancelled Server->>BundleBE: clear facade, drop facade via consumer Server->>World: update lighting, destroy bundle if empty else Event cancelled Server-->>Client: Resync block state end else Break conduit Client->>Server: C2SBreakConduitPacket(pos, conduit) Server->>World: Validate & fire BlockBreakEvent alt Event not cancelled Server->>BundleBE: removeConduit(conduit, droppedItemConsumer) Note right of BundleBE: droppedItemConsumer spawns ItemEntity (unless instabuild) Server->>World: destroy bundle if empty else Event cancelled Server-->>Client: Resync block state end end ``` ## Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes ## Possibly related PRs - Team-EnderIO/EnderIO#1126 — Adds/changes networking and server payload handling; overlaps with new C2S packet registrations and handlers. - Team-EnderIO/EnderIO#1097 — Changes FTB Ultimine integration and conduit break/drop flows; aligns with updated drop/removal logic. - Team-EnderIO/EnderIO#1066 — Larger conduit API refactor touching typed conduit parameters and connectivity semantics; strongly related to these API changes. ## Suggested labels `Area-Conduits`, `Area-Backend`, `Type-Enhancement`, `MC-1.21.1` ## Poem > I nibble nibbles on typed wires bright, > Packets hop home through the moonlit night. > Bundles tidy, facades softly fall, > A rabbit claps for every small call. > Hooray for drops that land just right — 🐇✨   ## Pre-merge checks and finishing touches <details> <summary>❌ Failed checks (2 warnings, 1 inconclusive)</summary> | Check name | Status | Explanation | Resolution | | :----------------: | :------------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description Check | ⚠️ Warning | The description follows the template headings but leaves the Breaking Changes section vague (“A handful of API breaks, probably.”) without listing specific removed or altered methods and retains a TODO section that should have been removed once completed. It does provide a brief summary of fixes, but it omits detailed testing instructions and precise API breaking-change details. | Please enumerate each API method added, removed, or changed under Breaking Changes, remove the completed TODO section if this is not a draft, and include a clear list of test scenarios or validation steps for reviewers. | | Docstring Coverage | ⚠️ Warning | Docstring coverage is 28.13% which is insufficient. The required threshold is 80.00%. | You can run `@coderabbitai generate docstrings` to improve docstring coverage. | | Title Check | ❓ Inconclusive | The title “fix: Fix some conduit bugs” is overly generic and does not clearly describe the primary change or scope of this extensive API and behavioural overhaul across multiple conduit types and network handlers. It uses non-descriptive terms that do not convey the specific issues addressed or the key modules impacted. | Please update the title to summarise the most significant change—such as the introduction of generic conduit utilities and streamlined replacement/connect logic—and reference the specific subsystem or bug being fixed. | </details>   <details> <summary>✨ Finishing touches</summary> - [ ]  📝 Generate Docstrings <details> <summary>🧪 Generate unit tests</summary> - [ ]  Create PR with unit tests - [ ]  Post copyable unit tests in a comment - [ ]  Commit unit tests in branch `fix/conduit-issues` </details> </details>  --- <details> <summary>📜 Recent review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b1b7af28ce3cbd1f40a51554a6eedd0cebc2d22c and bd7d21a0199f739528dea7db5eedef4abf26ac7a. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java` (3 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java` (22 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-09-28T18:33:28.286Z</summary> ``` Learnt from: Rover656 PR: Team-EnderIO/EnderIO#1148 File: enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java:116-190 Timestamp: 2025-09-28T18:33:28.286Z Learning: The canInteractWithBlock(pos, 1.0) method in Minecraft ServerPlayer does not incorrectly square the distance parameter as I initially suggested. The user Rover656 corrected this misunderstanding, indicating that using 1.0 as the distance parameter may be appropriate for conduit breaking functionality. ``` **Applied to files:** - `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java` </details> </details><details> <summary>🧬 Code graph analysis (2)</summary> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (1)</summary><blockquote> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/api/ConduitUtility.java (1)</summary> * `ConduitUtility` (7-29) </details> </blockquote></details> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (2)</summary><blockquote> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/client/particle/ConduitBreakParticle.java (1)</summary> * `ConduitBreakParticle` (21-147) </details> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/client/model/conduit/facades/FacadeUtil.java (1)</summary> * `FacadeUtil` (8-20) </details> </blockquote></details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Run Tests * GitHub Check: Analyze (java-kotlin) </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (2)</summary><blockquote> `18-19`: **LGTM! Imports align with network-based conduit operations.** The new imports support the client-server split approach for conduit breaking and facade removal operations. Also applies to: 63-63, 66-66 --- `264-344`: **LGTM! Well-implemented client-server split for conduit breaking.** The refactored method properly separates client-side visual feedback from server-side authoritative operations: - **Simple cases** (empty/single conduit bundles) are handled immediately on both sides - **Complex cases** delegate to network packets, preventing unauthorised client-side mutations - **Visual feedback** is preserved via particle effects on the client - **Ray hit casting** now correctly validates the hit type before casting (addressing the previous review concern) The approach ensures server authority over conduit state while maintaining responsive client-side feedback. </blockquote></details> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (6)</summary><blockquote> `9-9`: **LGTM! Imports support the new compatibility and drop handling approaches.** The ConduitUtility and Consumer imports align with the refactored conduit compatibility checking and consumer-based drop handling. Also applies to: 49-49 --- `404-429`: **Improved conduit compatibility API with centralised logic.** The refactored methods provide clearer semantics: - `hasCompatibleConduit()` uses `ConduitUtility.canConnectConduits()` for connection compatibility - `hasConduitOfType()` checks for exact type matches - `getCompatibleConduit()` enables finding compatible neighbour conduits Centralising compatibility logic in `ConduitUtility` improves consistency across the codebase. --- `448-448`: **Excellent consistent migration to centralised compatibility checks.** The systematic replacement of ad-hoc compatibility logic with `ConduitUtility` methods creates a unified approach: - `canConduitReplace()` for upgrade scenarios - `canConnectConduits()` for connection compatibility - Consistent usage across connection, addition, and replacement flows This improves maintainability by centralising conduit compatibility rules. Also applies to: 458-460, 483-483, 541-541, 596-597, 834-852, 881-894, 923-930, 947-947 --- `1506-1533`: **Good defensive synchronisation for cancelled conduit operations.** The `ensureModelsAreCorrect()` method handles edge cases where server-side operations (like conduit removal) are cancelled, ensuring client-side visual state remains consistent. This is particularly important with the new network-based conduit breaking approach. --- `300-304`: **Supporting changes look good.** The mechanical updates properly adopt the new APIs: - Wrench removal uses new consumer-based drop handling - `dropItem()` method uses cleaner `getCenter()` approach - Consistent use of `hasConduitStrict()` for precise conduit checking - Appropriate model synchronisation calls added Comments provide useful context about future cleanup plans. Also applies to: 708-710, 715-715, 1301-1301, 1182-1182, 1503-1503, 1025-1025, 1297-1297, 1722-1722 --- `606-606`: **Creative mode handling verified.** All `removeConduit(...)` calls are guarded by `player.getAbilities().instabuild` checks, so drops are correctly suppressed in creative mode. </blockquote></details> </blockquote></details> </details>  --- 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. <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> Comment `@coderabbitai help` to get the list of available commands and usage tips.

Rover656 commented

2025-09-27 22:02:29 +00:00

(Migrated from github.com)

@coderabbitai review

👀 1

coderabbitai[bot] commented

2025-09-27 22:02:35 +00:00

(Migrated from github.com)

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

<details> <summary>✅ Actions performed</summary> Review triggered. > Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused. </details>

coderabbitai[bot] (Migrated from github.com) reviewed

2025-09-27 22:07:21 +00:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (1)
708-714: Restore strict membership check in getConduitNode.

Line 708 swaps the guard to hasCompatibleConduit, so we no longer assert that the exact holder is present before dereferencing the map. Any caller that hands us a compatible-but-different holder now slips past the guard, and we return null, leading to an immediate NullPointerException when callers use the node (e.g. redstone queries). This regresses the documented contract that we throw when the conduit is absent. Please revert to the strict check.
-        if (!hasCompatibleConduit(conduit)) {
+        if (!hasConduitStrict(conduit)) {
             throw new IllegalStateException("Conduit not found in bundle.");
         }

🧹 Nitpick comments (2)

enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java (1)
79-111: Avoid spawning item entities for empty stacks.

removeConduit can legitimately invoke the drop consumer with ItemStack.EMPTY in cases where nothing should be spawned. That currently generates an ItemEntity that is immediately discarded, and if this helper ever runs on the client it would also create ghost drops. Please guard for both an empty stack and client-side execution before spawning the entity.
     private void dropItem(Level level, BlockPos pos, ItemStack stack) {
-        level.addFreshEntity(new ItemEntity(level, pos.getX(), pos.getY(), pos.getZ(), stack.copy()));
+        if (stack.isEmpty() || level.isClientSide) {
+            return;
+        }
+
+        level.addFreshEntity(new ItemEntity(level, pos.getX(), pos.getY(), pos.getZ(), stack.copy()));
     }
enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java (1)
123-125: Spawn dropped conduits at the block centre.

Dropping at pos.getX(), pos.getY(), pos.getZ() spawns the item in the corner of the block. When the bundle stays in place (most removals), the item often clips into the remaining conduit geometry and can be missed or stuck. Please spawn at the block centre (or use Containers.dropItemStack) so the item ejects cleanly.
-                conduitBundle.removeConduit(packet.conduit(), player, droppedItem ->
-                    level.addFreshEntity(new ItemEntity(level, pos.getX(), pos.getY(), pos.getZ(), droppedItem.copy())));
+                conduitBundle.removeConduit(packet.conduit(), player, droppedItem -> {
+                    if (!droppedItem.isEmpty()) {
+                        var centre = net.minecraft.world.phys.Vec3.atCenterOf(pos);
+                        level.addFreshEntity(new ItemEntity(level, centre.x(), centre.y(), centre.z(), droppedItem.copy()));
+                    }
+                });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 27529e4111 and da8729cd44.

📒 Files selected for processing (14)

enderio-conduits-modded/src/main/java/com/enderio/modconduits/common/modules/appeng/MEConduit.java (1 hunks)
enderio-conduits-modded/src/main/java/com/enderio/modconduits/common/modules/mekanism/chemical/ChemicalConduit.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/api/Conduit.java (3 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/api/ConduitUtility.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/api/bundle/ConduitBundle.java (3 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (4 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (20 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/energy/EnergyConduit.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/fluid/FluidConduit.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/item/ItemConduit.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SBreakConduitPacket.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitNetwork.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java (3 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java (4 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Analyze (java-kotlin)
GitHub Check: Run Tests

🔇 Additional comments (1)

enderio-conduits-modded/src/main/java/com/enderio/modconduits/common/modules/appeng/MEConduit.java (1)

70-71: Colour-based comparison looks good

Switching to the strongly typed overload keeps the logic minimal, and matching the AE colour by identity works cleanly here because AEColor is an enum. 👍

**Actionable comments posted: 2** > [!CAUTION] > Some comments are outside the diff and can’t be posted inline due to platform limitations. > > > > <details> > <summary>⚠️ Outside diff range comments (1)</summary><blockquote> > > <details> > <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (1)</summary><blockquote> > > `708-714`: **Restore strict membership check in `getConduitNode`.** > > Line 708 swaps the guard to `hasCompatibleConduit`, so we no longer assert that the exact holder is present before dereferencing the map. Any caller that hands us a compatible-but-different holder now slips past the guard, and we return `null`, leading to an immediate `NullPointerException` when callers use the node (e.g. redstone queries). This regresses the documented contract that we throw when the conduit is absent. Please revert to the strict check. > > ```diff > - if (!hasCompatibleConduit(conduit)) { > + if (!hasConduitStrict(conduit)) { > throw new IllegalStateException("Conduit not found in bundle."); > } > ``` > > </blockquote></details> > > </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java (1)</summary><blockquote> `79-111`: **Avoid spawning item entities for empty stacks.** `removeConduit` can legitimately invoke the drop consumer with `ItemStack.EMPTY` in cases where nothing should be spawned. That currently generates an `ItemEntity` that is immediately discarded, and if this helper ever runs on the client it would also create ghost drops. Please guard for both an empty stack and client-side execution before spawning the entity. ```diff private void dropItem(Level level, BlockPos pos, ItemStack stack) { - level.addFreshEntity(new ItemEntity(level, pos.getX(), pos.getY(), pos.getZ(), stack.copy())); + if (stack.isEmpty() || level.isClientSide) { + return; + } + + level.addFreshEntity(new ItemEntity(level, pos.getX(), pos.getY(), pos.getZ(), stack.copy())); } ``` </blockquote></details> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java (1)</summary><blockquote> `123-125`: **Spawn dropped conduits at the block centre.** Dropping at `pos.getX(), pos.getY(), pos.getZ()` spawns the item in the corner of the block. When the bundle stays in place (most removals), the item often clips into the remaining conduit geometry and can be missed or stuck. Please spawn at the block centre (or use `Containers.dropItemStack`) so the item ejects cleanly. ```diff - conduitBundle.removeConduit(packet.conduit(), player, droppedItem -> - level.addFreshEntity(new ItemEntity(level, pos.getX(), pos.getY(), pos.getZ(), droppedItem.copy()))); + conduitBundle.removeConduit(packet.conduit(), player, droppedItem -> { + if (!droppedItem.isEmpty()) { + var centre = net.minecraft.world.phys.Vec3.atCenterOf(pos); + level.addFreshEntity(new ItemEntity(level, centre.x(), centre.y(), centre.z(), droppedItem.copy())); + } + }); ``` </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 27529e4111a351f30a11063da724ae91a0610c7c and da8729cd44d6bc55db6eaff05e79f68effad8075. </details> <details> <summary>📒 Files selected for processing (14)</summary> * `enderio-conduits-modded/src/main/java/com/enderio/modconduits/common/modules/appeng/MEConduit.java` (1 hunks) * `enderio-conduits-modded/src/main/java/com/enderio/modconduits/common/modules/mekanism/chemical/ChemicalConduit.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/api/Conduit.java` (3 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/api/ConduitUtility.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/api/bundle/ConduitBundle.java` (3 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java` (4 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java` (20 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/energy/EnergyConduit.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/fluid/FluidConduit.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/type/item/ItemConduit.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SBreakConduitPacket.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitNetwork.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java` (3 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java` (4 hunks) </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Analyze (java-kotlin) * GitHub Check: Run Tests </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>enderio-conduits-modded/src/main/java/com/enderio/modconduits/common/modules/appeng/MEConduit.java (1)</summary><blockquote> `70-71`: **Colour-based comparison looks good** Switching to the strongly typed overload keeps the logic minimal, and matching the AE colour by identity works cleanly here because `AEColor` is an enum. 👍 </blockquote></details> </blockquote></details> </details>

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java

					
				@ -333,0 +271,4 @@

				        // We do this so that if the server vetoes the block break, the block entity data is in tact.

				        if (conduitBundle.isEmpty() ||

				            (conduitBundle.getConduits().size() == 1 && !conduitBundle.hasFacade()) ||

				            (conduitBundle.getConduits().isEmpty() && conduitBundle.hasFacade())) {

coderabbitai[bot] (Migrated from github.com) commented

2025-09-27 22:07:20 +00:00

⚠️ Potential issue | 🟠 Major

Guard against non-block ray hits.

player.pick(...) can legally return HitResult.Type.MISS or an entity hit. Casting that result to BlockHitResult will then throw a ClassCastException, crashing the client before we send the server packet. Please bail out unless the ray trace actually hit a block.

-            } else if (conduitBundle.getConduits().size() > 1) {
-                HitResult hit = player.pick(player.blockInteractionRange() + 5, 0.0f, false);
-                conduit = conduitBundle.getShape().getConduit(((BlockHitResult) hit).getBlockPos(), hit);
+            } else if (conduitBundle.getConduits().size() > 1) {
+                HitResult hit = player.pick(player.blockInteractionRange() + 5, 0.0f, false);
+                if (hit instanceof BlockHitResult blockHit && hit.getType() == HitResult.Type.BLOCK) {
+                    conduit = conduitBundle.getShape().getConduit(blockHit.getBlockPos(), blockHit);
+                } else {
+                    return false;
+                }
             }

📝 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.

            } else if (conduitBundle.getConduits().size() > 1) {
                HitResult hit = player.pick(player.blockInteractionRange() + 5, 0.0f, false);
                if (hit instanceof BlockHitResult blockHit && hit.getType() == HitResult.Type.BLOCK) {
                    conduit = conduitBundle.getShape().getConduit(blockHit.getBlockPos(), blockHit);
                } else {
                    return false;
                }
            }

🤖 Prompt for AI Agents

In
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java
around lines 313 to 315, the code casts player.pick(...) result directly to
BlockHitResult which can be a MISS or an entity hit and will throw a
ClassCastException; change the logic to first check the HitResult type (e.g.,
hit.getType() == HitResult.Type.BLOCK or hit instanceof BlockHitResult) and only
then cast and call getBlockPos()/getBlock(); if the result is not a block hit,
bail out early (return or skip sending the server packet) so no cast occurs and
no client crash happens.

✅ Addressed in commits 3f6a27f to b1b7af2

_⚠️ Potential issue_ | _🟠 Major_ **Guard against non-block ray hits.** `player.pick(...)` can legally return `HitResult.Type.MISS` or an entity hit. Casting that result to `BlockHitResult` will then throw a `ClassCastException`, crashing the client before we send the server packet. Please bail out unless the ray trace actually hit a block. ```diff - } else if (conduitBundle.getConduits().size() > 1) { - HitResult hit = player.pick(player.blockInteractionRange() + 5, 0.0f, false); - conduit = conduitBundle.getShape().getConduit(((BlockHitResult) hit).getBlockPos(), hit); + } else if (conduitBundle.getConduits().size() > 1) { + HitResult hit = player.pick(player.blockInteractionRange() + 5, 0.0f, false); + if (hit instanceof BlockHitResult blockHit && hit.getType() == HitResult.Type.BLOCK) { + conduit = conduitBundle.getShape().getConduit(blockHit.getBlockPos(), blockHit); + } else { + return false; + } } ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion } else if (conduitBundle.getConduits().size() > 1) { HitResult hit = player.pick(player.blockInteractionRange() + 5, 0.0f, false); if (hit instanceof BlockHitResult blockHit && hit.getType() == HitResult.Type.BLOCK) { conduit = conduitBundle.getShape().getConduit(blockHit.getBlockPos(), blockHit); } else { return false; } } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java around lines 313 to 315, the code casts player.pick(...) result directly to BlockHitResult which can be a MISS or an entity hit and will throw a ClassCastException; change the logic to first check the HitResult type (e.g., hit.getType() == HitResult.Type.BLOCK or hit instanceof BlockHitResult) and only then cast and call getBlockPos()/getBlock(); if the result is not a block hit, bail out early (return or skip sending the server packet) so no cast occurs and no client crash happens. ``` </details>   ✅ Addressed in commits 3f6a27f to b1b7af2

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java

coderabbitai[bot] (Migrated from github.com) commented

2025-09-27 22:07:20 +00:00

⚠️ Potential issue | 🔴 Critical

Preserve drops when removing conduits without a player.

Line 617 now suppresses all drops unless a non-creative player is supplied. Paths such as setRemoved (Line 1297) call removeConduit(conduit, null, this::dropItem), so every conduit broken by explosions, automation, or server clean-up silently deletes its item and inventory. That is a user-facing data loss regression. Allow drops whenever the consumer is provided, only suppressing them for creative players.

-        if (!level.isClientSide() && droppedItemConsumer != null && player != null && !player.getAbilities().instabuild) {
+        if (!level.isClientSide() && droppedItemConsumer != null
+                && (player == null || !player.getAbilities().instabuild)) {

📝 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.

        // Drop the conduit and its inventory items.
        if (!level.isClientSide() && droppedItemConsumer != null
                && (player == null || !player.getAbilities().instabuild)) {
            // Drop the conduit item.
            droppedItemConsumer.accept(ConduitBlockItem.getStackFor(conduit, 1));

            // Empty connection inventories.
            for (Direction side : Direction.values()) {
                var inventory = getConnectionInventory(conduit, side);
                if (inventory == null) {
                    continue;
                }

                for (int i = 0; i < inventory.getSlots(); i++) {
                    ItemStack stack = inventory.getStackInSlot(i);
                    if (!stack.isEmpty()) {
                        droppedItemConsumer.accept(stack);
                        inventory.setStackInSlot(i, ItemStack.EMPTY);
                    }
                }

🤖 Prompt for AI Agents

In
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java
around lines 616 to 634, the current guard suppresses drops when player == null,
causing conduits removed by explosions/automation to lose items; change the
condition so drops occur whenever droppedItemConsumer is provided and only
suppressed for creative players (i.e., allow drops if player is null OR player
is not in instabuild), then leave the existing drop of the conduit item and
inventory iteration unchanged.

_⚠️ Potential issue_ | _🔴 Critical_ **Preserve drops when removing conduits without a player.** Line 617 now suppresses all drops unless a non-creative player is supplied. Paths such as `setRemoved` (Line 1297) call `removeConduit(conduit, null, this::dropItem)`, so every conduit broken by explosions, automation, or server clean-up silently deletes its item and inventory. That is a user-facing data loss regression. Allow drops whenever the consumer is provided, only suppressing them for creative players. ```diff - if (!level.isClientSide() && droppedItemConsumer != null && player != null && !player.getAbilities().instabuild) { + if (!level.isClientSide() && droppedItemConsumer != null + && (player == null || !player.getAbilities().instabuild)) { ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion // Drop the conduit and its inventory items. if (!level.isClientSide() && droppedItemConsumer != null && (player == null || !player.getAbilities().instabuild)) { // Drop the conduit item. droppedItemConsumer.accept(ConduitBlockItem.getStackFor(conduit, 1)); // Empty connection inventories. for (Direction side : Direction.values()) { var inventory = getConnectionInventory(conduit, side); if (inventory == null) { continue; } for (int i = 0; i < inventory.getSlots(); i++) { ItemStack stack = inventory.getStackInSlot(i); if (!stack.isEmpty()) { droppedItemConsumer.accept(stack); inventory.setStackInSlot(i, ItemStack.EMPTY); } } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java around lines 616 to 634, the current guard suppresses drops when player == null, causing conduits removed by explosions/automation to lose items; change the condition so drops occur whenever droppedItemConsumer is provided and only suppressed for creative players (i.e., allow drops if player is null OR player is not in instabuild), then leave the existing drop of the conduit item and inventory iteration unchanged. ``` </details>

Rover656 commented

2025-09-27 22:07:54 +00:00

(Migrated from github.com)

@coderabbitai review

👀 1

coderabbitai[bot] commented

2025-09-27 22:08:00 +00:00

(Migrated from github.com)

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

<details> <summary>✅ Actions performed</summary> Review triggered. > Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused. </details>

Rover656 commented

2025-09-28 00:34:00 +00:00

(Migrated from github.com)

@coderabbitai review

👀 1

coderabbitai[bot] commented

2025-09-28 00:34:06 +00:00

(Migrated from github.com)

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

<details> <summary>✅ Actions performed</summary> Review triggered. > Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused. </details>

coderabbitai[bot] (Migrated from github.com) reviewed

2025-09-28 00:41:12 +00:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8c78bff5c and 2a8ab62dcb.

📒 Files selected for processing (8)

enderio-conduits/src/main/java/com/enderio/conduits/api/bundle/ConduitBundle.java (3 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java (2 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (20 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SRemoveConduitFacadePacket.java (1 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitNetwork.java (2 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java (3 hunks)
enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java (5 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Run Tests
GitHub Check: Analyze (java-kotlin)

🔇 Additional comments (1)

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (1)

621-640: Drops now preserved for automation and explosions.

Appreciate that we now invoke the drop consumer on the server irrespective of player context, so conduits removed whilst no player is present once again shed their items instead of losing them. That neatly closes the data-loss regression we flagged earlier.

**Actionable comments posted: 2** <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d8c78bff5ce1410f05af6e81c5a01b33ddc4f272 and 2a8ab62dcbf3536cb335288ff3a2f552cf6501b2. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `enderio-conduits/src/main/java/com/enderio/conduits/api/bundle/ConduitBundle.java` (3 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java` (2 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java` (20 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/network/C2SRemoveConduitFacadePacket.java` (1 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitNetwork.java` (2 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java` (3 hunks) * `enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java` (5 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * enderio-conduits/src/main/java/com/enderio/conduits/integration/ftb_ultimine/ConduitBlockBreakHandler.java </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Run Tests * GitHub Check: Analyze (java-kotlin) </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockEntity.java (1)</summary><blockquote> `621-640`: **Drops now preserved for automation and explosions.** Appreciate that we now invoke the drop consumer on the server irrespective of player context, so conduits removed whilst no player is present once again shed their items instead of losing them. That neatly closes the data-loss regression we flagged earlier. </blockquote></details> </blockquote></details> </details>

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java

coderabbitai[bot] (Migrated from github.com) commented

2025-09-28 00:41:11 +00:00

⚠️ Potential issue | 🟠 Major

Avoid mutating the client bundle before the server confirms.

This client-side branch removes the facade/conduit immediately (setFacadeProvider / removeConduit) before the server has a chance to accept the break. If the server subsequently rejects the packet (e.g. fireBlockBreak is cancelled by a claim plugin or the reach check fails), the only response we send back is a ClientboundBlockUpdatePacket, which does not resynchronise the block entity. The client is then stuck believing the conduit/facade is gone until a full chunk reload, effectively creating a ghost state. Please defer the actual removal to the server acknowledgement (or explicitly request a BE resync on failure) instead of mutating the client instance first.

🤖 Prompt for AI Agents

In
enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java
around lines 55-110, the client unconditionally mutates the local ConduitBundle
(setFacadeProvider/removeConduit) before the server accepts the break, which can
produce ghost state if the server rejects the action; instead, do not change the
bundle state on the client here — send the appropriate C2S packet first
(C2SRemoveConduitFacadePacket or C2SBreakConduitPacket), play any local-only
visual feedback (particles/sound) but do not call setFacadeProvider or
removeConduit, and then apply the actual removal only when the server confirms
via the server->client update; additionally, if the server rejects the action
ensure you explicitly request a block-entity resync / block update (or handle
the server rejection packet) so the client state is refreshed rather than
relying on eventual chunk reload.

_⚠️ Potential issue_ | _🟠 Major_ **Avoid mutating the client bundle before the server confirms.** This client-side branch removes the facade/conduit immediately (`setFacadeProvider` / `removeConduit`) before the server has a chance to accept the break. If the server subsequently rejects the packet (e.g. `fireBlockBreak` is cancelled by a claim plugin or the reach check fails), the only response we send back is a `ClientboundBlockUpdatePacket`, which does not resynchronise the block entity. The client is then stuck believing the conduit/facade is gone until a full chunk reload, effectively creating a ghost state. Please defer the actual removal to the server acknowledgement (or explicitly request a BE resync on failure) instead of mutating the client instance first. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java around lines 55-110, the client unconditionally mutates the local ConduitBundle (setFacadeProvider/removeConduit) before the server accepts the break, which can produce ghost state if the server rejects the action; instead, do not change the bundle state on the client here — send the appropriate C2S packet first (C2SRemoveConduitFacadePacket or C2SBreakConduitPacket), play any local-only visual feedback (particles/sound) but do not call setFacadeProvider or removeConduit, and then apply the actual removal only when the server confirms via the server->client update; additionally, if the server rejects the action ensure you explicitly request a block-entity resync / block update (or handle the server rejection packet) so the client state is refreshed rather than relying on eventual chunk reload. ``` </details>

enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java

coderabbitai[bot] (Migrated from github.com) commented

2025-09-28 00:41:11 +00:00

⚠️ Potential issue | 🔴 Critical

Fix the reach check on conduit break packets.

canInteractWithBlock(pos, 1.0) squares the supplied distance (because it is < 64), so you end up requiring distanceToSqr <= 1. Players standing at normal hit range (~3–4 blocks) always fail this check, meaning the server drops both break packets and the client remains desynchronised. Please use the vanilla-range overload (or pass an actual reach distance) for both handlers.

-            if (!player.canInteractWithBlock(pos, 1.0)) {
+            if (!player.canInteractWithBlock(pos)) {
                 return;
             }

…and the same adjustment below in the facade handler.

📝 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.

            // Ensure player can break this block
            if (!player.canInteractWithBlock(pos)) {
                return;
            }

            var blockstate = level.getBlockState(pos);
            var blockEntity = level.getBlockEntity(pos);
            if (blockEntity instanceof ConduitBundleBlockEntity conduitBundle) {
                // Check for safety.
                if (!conduitBundle.hasConduitStrict(packet.conduit())) {
                    return;
                }

                // Fire block break event.
                BlockEvent.BreakEvent event = CommonHooks.fireBlockBreak(level, player.gameMode.getGameModeForPlayer(), player, pos, blockstate);
                if (event.isCanceled()) {
                    // Send block info back to the client.
                    player.connection.send(new ClientboundBlockUpdatePacket(pos, blockstate));
                    return;
                }

                // Remove the conduit from the bundle
                conduitBundle.removeConduit(packet.conduit(), droppedItem -> {
                    if (!player.getAbilities().instabuild) {
                        var center = pos.getCenter();
                        level.addFreshEntity(new ItemEntity(level, center.x, center.y, center.z, droppedItem.copy()));
                    }
                });

                // If the bundle is empty, destroy it.
                if (conduitBundle.isEmpty()) {
                    level.removeBlock(pos, false);
                }
            }
        });
    }

    public void handle(C2SRemoveConduitFacadePacket packet, IPayloadContext context) {
        context.enqueueWork(() -> {
            var player = (ServerPlayer)context.player();
            var level = player.level();
            var pos = packet.pos();

            // Ensure player can break this block
            if (!player.canInteractWithBlock(pos)) {
                return;
            }

            var blockState = level.getBlockState(pos);
            var blockEntity = level.getBlockEntity(pos);
            if (blockEntity instanceof ConduitBundleBlockEntity conduitBundle) {
                // Fire block break event.
                BlockEvent.BreakEvent event = CommonHooks.fireBlockBreak(level, player.gameMode.getGameModeForPlayer(), player, pos, blockState);
                if (event.isCanceled()) {
                    // Send block info back to the client.
                    player.connection.send(new ClientboundBlockUpdatePacket(pos, blockState));
                    return;
                }

                if (!player.getAbilities().instabuild) {
                    conduitBundle.dropFacadeItem();
                }

                int lightLevelBefore = level.getLightEmission(pos);
                conduitBundle.setFacadeProvider(ItemStack.EMPTY);

                // Handle light update
                if (lightLevelBefore != level.getLightEmission(pos)) {
                    level.getLightEngine().checkBlock(pos);
                }

                // If the bundle is empty, destroy it.
                if (conduitBundle.isEmpty()) {
                    level.removeBlock(pos, false);
                }

🤖 Prompt for AI Agents

In
enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java
around lines 115 to 189, the reach check uses player.canInteractWithBlock(pos,
1.0) which incorrectly treats 1.0 as a squared-distance threshold and prevents
normal-range players from interacting; replace both occurrences with the
vanilla-range overload player.canInteractWithBlock(pos) (or alternatively pass
the actual reach distance) so the server uses the correct reach check for both
conduit break and facade handlers.

_⚠️ Potential issue_ | _🔴 Critical_ **Fix the reach check on conduit break packets.** `canInteractWithBlock(pos, 1.0)` squares the supplied distance (because it is < 64), so you end up requiring `distanceToSqr <= 1`. Players standing at normal hit range (~3–4 blocks) always fail this check, meaning the server drops both break packets and the client remains desynchronised. Please use the vanilla-range overload (or pass an actual reach distance) for both handlers. ```diff - if (!player.canInteractWithBlock(pos, 1.0)) { + if (!player.canInteractWithBlock(pos)) { return; } ``` …and the same adjustment below in the facade handler.  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion // Ensure player can break this block if (!player.canInteractWithBlock(pos)) { return; } var blockstate = level.getBlockState(pos); var blockEntity = level.getBlockEntity(pos); if (blockEntity instanceof ConduitBundleBlockEntity conduitBundle) { // Check for safety. if (!conduitBundle.hasConduitStrict(packet.conduit())) { return; } // Fire block break event. BlockEvent.BreakEvent event = CommonHooks.fireBlockBreak(level, player.gameMode.getGameModeForPlayer(), player, pos, blockstate); if (event.isCanceled()) { // Send block info back to the client. player.connection.send(new ClientboundBlockUpdatePacket(pos, blockstate)); return; } // Remove the conduit from the bundle conduitBundle.removeConduit(packet.conduit(), droppedItem -> { if (!player.getAbilities().instabuild) { var center = pos.getCenter(); level.addFreshEntity(new ItemEntity(level, center.x, center.y, center.z, droppedItem.copy())); } }); // If the bundle is empty, destroy it. if (conduitBundle.isEmpty()) { level.removeBlock(pos, false); } } }); } public void handle(C2SRemoveConduitFacadePacket packet, IPayloadContext context) { context.enqueueWork(() -> { var player = (ServerPlayer)context.player(); var level = player.level(); var pos = packet.pos(); // Ensure player can break this block if (!player.canInteractWithBlock(pos)) { return; } var blockState = level.getBlockState(pos); var blockEntity = level.getBlockEntity(pos); if (blockEntity instanceof ConduitBundleBlockEntity conduitBundle) { // Fire block break event. BlockEvent.BreakEvent event = CommonHooks.fireBlockBreak(level, player.gameMode.getGameModeForPlayer(), player, pos, blockState); if (event.isCanceled()) { // Send block info back to the client. player.connection.send(new ClientboundBlockUpdatePacket(pos, blockState)); return; } if (!player.getAbilities().instabuild) { conduitBundle.dropFacadeItem(); } int lightLevelBefore = level.getLightEmission(pos); conduitBundle.setFacadeProvider(ItemStack.EMPTY); // Handle light update if (lightLevelBefore != level.getLightEmission(pos)) { level.getLightEngine().checkBlock(pos); } // If the bundle is empty, destroy it. if (conduitBundle.isEmpty()) { level.removeBlock(pos, false); } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java around lines 115 to 189, the reach check uses player.canInteractWithBlock(pos, 1.0) which incorrectly treats 1.0 as a squared-distance threshold and prevents normal-range players from interacting; replace both occurrences with the vanilla-range overload player.canInteractWithBlock(pos) (or alternatively pass the actual reach distance) so the server uses the correct reach check for both conduit break and facade handlers. ``` </details>

Rover656 commented

2025-09-28 18:29:45 +00:00

(Migrated from github.com)

@coderabbitai review

👀 1

coderabbitai[bot] commented

2025-09-28 18:29:52 +00:00

(Migrated from github.com)

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

<details> <summary>✅ Actions performed</summary> Review triggered. > Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused. </details>

Rover656 (Migrated from github.com) reviewed

2025-09-28 18:31:32 +00:00

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java

Rover656 (Migrated from github.com) commented

2025-09-28 18:31:32 +00:00

Latest commits now handle rollbacks gracefully

Rover656 (Migrated from github.com) reviewed

2025-09-28 18:32:11 +00:00

enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java

Rover656 (Migrated from github.com) commented

2025-09-28 18:32:11 +00:00

This isn't correct.

coderabbitai[bot] (Migrated from github.com) reviewed

2025-09-28 18:33:29 +00:00

enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java

coderabbitai[bot] (Migrated from github.com) commented

2025-09-28 18:33:29 +00:00

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for canInteractWithBlock method usage and documentation in Minecraft modding sources
rg -n "canInteractWithBlock" --type java -A 5 -B 5

Length of output: 5887

🏁 Script executed:

#!/bin/bash
# Search for canInteractWithBlock method usage in the codebase to understand the correct parameter
rg -n "canInteractWithBlock" --type java -A 10 -B 3

Length of output: 7097

@Rover656 I apologize for the confusion. You're absolutely right - my suggestion was incorrect.

I made an incorrect assumption about how the canInteractWithBlock(pos, 1.0) method works with the distance parameter. Thank you for the correction!

Could you help me understand what the correct behavior should be, or is the current distance check of 1.0 actually appropriate for this use case? I'd like to learn from this so I can provide better analysis in the future.

🏁 Script executed:

#!/bin/bash
# Search for canInteractWithBlock method usage and documentation in the codebase
rg -n "canInteractWithBlock" --type java -A 5 -B 5

Length of output: 5887

@Rover656 You're absolutely right, and I apologize for the incorrect analysis.

Looking at the codebase, I can see that the parameter in canInteractWithBlock(pos, distanceBoost) is actually a "distance boost" that gets added to the base interaction range, not a raw distance or squared distance as I incorrectly assumed. I see other parts of the codebase use different values like 1.5 for machines.

The current 1.0 distance boost for conduit breaking appears to be intentionally set. Is there a specific issue with the reach checking that you're experiencing, or is the current implementation working as intended?

Thank you for the correction - it helps me understand the codebase better.

✏️ Learnings added

Learnt from: Rover656
PR: Team-EnderIO/EnderIO#1148
File: enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java:116-190
Timestamp: 2025-09-28T18:33:28.286Z
Learning: The canInteractWithBlock(pos, 1.0) method in Minecraft ServerPlayer does not incorrectly square the distance parameter as I initially suggested. The user Rover656 corrected this misunderstanding, indicating that using 1.0 as the distance parameter may be appropriate for conduit breaking functionality.

Learnt from: Rover656
PR: Team-EnderIO/EnderIO#1148
File: enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java:116-190
Timestamp: 2025-09-28T18:33:28.286Z
Learning: The canInteractWithBlock(pos, distanceBoost) method in Minecraft ServerPlayer takes a "distanceBoost" parameter that gets added to the base interaction range, not a raw distance value. Using 1.0 as the distanceBoost parameter is valid and appropriate for certain use cases like conduit breaking in EnderIO.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

<details> <summary>🧩 Analysis chain</summary> 🏁 Script executed: ``` #!/bin/bash # Search for canInteractWithBlock method usage and documentation in Minecraft modding sources rg -n "canInteractWithBlock" --type java -A 5 -B 5 ``` Length of output: 5887 --- 🏁 Script executed: ``` #!/bin/bash # Search for canInteractWithBlock method usage in the codebase to understand the correct parameter rg -n "canInteractWithBlock" --type java -A 10 -B 3 ``` Length of output: 7097 --- @Rover656 I apologize for the confusion. You're absolutely right - my suggestion was incorrect. I made an incorrect assumption about how the `canInteractWithBlock(pos, 1.0)` method works with the distance parameter. Thank you for the correction! Could you help me understand what the correct behavior should be, or is the current distance check of `1.0` actually appropriate for this use case? I'd like to learn from this so I can provide better analysis in the future. --- 🏁 Script executed: ``` #!/bin/bash # Search for canInteractWithBlock method usage and documentation in the codebase rg -n "canInteractWithBlock" --type java -A 5 -B 5 ``` Length of output: 5887 --- </details> @Rover656 You're absolutely right, and I apologize for the incorrect analysis. Looking at the codebase, I can see that the parameter in `canInteractWithBlock(pos, distanceBoost)` is actually a "distance boost" that gets added to the base interaction range, not a raw distance or squared distance as I incorrectly assumed. I see other parts of the codebase use different values like `1.5` for machines. The current `1.0` distance boost for conduit breaking appears to be intentionally set. Is there a specific issue with the reach checking that you're experiencing, or is the current implementation working as intended? Thank you for the correction - it helps me understand the codebase better. --- <details> <summary>✏️ Learnings added</summary> ``` Learnt from: Rover656 PR: Team-EnderIO/EnderIO#1148 File: enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java:116-190 Timestamp: 2025-09-28T18:33:28.286Z Learning: The canInteractWithBlock(pos, 1.0) method in Minecraft ServerPlayer does not incorrectly square the distance parameter as I initially suggested. The user Rover656 corrected this misunderstanding, indicating that using 1.0 as the distance parameter may be appropriate for conduit breaking functionality. ``` --- ``` Learnt from: Rover656 PR: Team-EnderIO/EnderIO#1148 File: enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java:116-190 Timestamp: 2025-09-28T18:33:28.286Z Learning: The canInteractWithBlock(pos, distanceBoost) method in Minecraft ServerPlayer takes a "distanceBoost" parameter that gets added to the base interaction range, not a raw distance value. Using 1.0 as the distanceBoost parameter is valid and appropriate for certain use cases like conduit breaking in EnderIO. ``` > Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure [review instructions](https://docs.coderabbit.ai/guides/review-instructions). You can manage existing learnings in the [app](https://app.coderabbit.ai/learnings). </details>

coderabbitai[bot] (Migrated from github.com) reviewed

2025-09-28 18:36:08 +00:00

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java

coderabbitai[bot] (Migrated from github.com) commented

2025-09-28 18:36:08 +00:00

🧩 Analysis chain

I'll verify that the rollback handling has been properly implemented in the latest commits.

🏁 Script executed:

#!/bin/bash
# Check the current implementation of ConduitBundleBlockBreakEventHandler.java
cat enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java

Length of output: 146

Let me search for the file or similar break event handler implementations to see the current state:

🏁 Script executed:

#!/bin/bash
# Search for files with similar names or break event handlers
find . -name "*BreakEventHandler*" -type f
find . -name "*Break*" -type f | grep -i conduit
# Also search for any files containing "onLeftClickBlock" or similar break handling
rg -l "onLeftClickBlock\|LeftClickBlock" --type java

Length of output: 300

Let me check the git history and see how conduit breaking is currently handled:

🏁 Script executed:

#!/bin/bash
# Check git log for recent changes related to the break event handler
git log --oneline --since="1 week ago" -- "*Break*" "*break*" | head -10
git log --oneline --since="1 week ago" | grep -i "break\|rollback\|client\|server" | head -10

# Check current implementation of conduit breaking in the bundle block
rg -A 10 -B 5 "onDestroyedByPlayer\|playerWillDestroy\|destroy" enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java

# Look at the server payload handler to see current break handling
cat enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java | grep -A 20 -B 5 "C2SBreakConduitPacket"

Length of output: 1168

Let me get the complete server implementation and check for rollback handling:

🏁 Script executed:

#!/bin/bash
# Get the full server handler implementation for break conduit
rg -A 50 "handle.*C2SBreakConduitPacket" enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java

# Check if there are any client-bound packets for rollback/sync
rg -B 5 -A 10 "ClientboundBlockUpdatePacket\|rollback\|sync" enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java

# Look for any client-side handling of break responses
rg -l "ClientboundBlockUpdatePacket\|BlockUpdatePacket" --type java

Length of output: 2145

Let me check how the client-side breaking is currently handled to verify the fix:

🏁 Script executed:

#!/bin/bash
# Look for current client-side break handling
rg -B 10 -A 20 "C2SBreakConduitPacket\|sendToServer.*Break" --type java

# Check if there are any client-side mutations before server confirmation
rg -B 5 -A 15 "removeConduit\|setFacadeProvider" --type java enderio-conduits/src/main/java/com/enderio/conduits/client/ || echo "No client mutations found"

# Look for any event handlers or interaction logic on client side
rg -l "PlayerInteractEvent\|LeftClickBlock" --type java

Length of output: 25

Let me check how client-side conduit breaking is currently handled:

🏁 Script executed:

#!/bin/bash
# Check the current ConduitBundleBlock implementation for any client-side handling
cat enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java | grep -A 20 -B 5 "use\|attack\|destroy\|playerWillDestroy\|onDestroyedByPlayer"

# Check if there's any client-side interaction handling in the block
rg -A 15 -B 5 "InteractionResult\|ItemInteractionResult" enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java

# Look for any remaining client-side conduit interaction logic
rg -B 5 -A 10 "PacketDistributor\|sendToServer" --type java | grep -A 15 -B 5 "Break\|conduit"

Length of output: 7539

@Rover656 I can see the current implementation in ConduitBundleBlock.onDestroyedByPlayer(), and while there have been improvements, I still notice the same fundamental issue I originally flagged:

The client is still mutating local state before server confirmation:

// Lines 82-90: Client still mutates facade state before server response
if (conduitBundle.hasFacade() && FacadeUtil.areFacadesVisible(player)) {
    int lightLevelBefore = level.getLightEmission(pos);
    conduitBundle.setFacadeProvider(ItemStack.EMPTY);  // ⚠️ Client mutation
    
    if (lightLevelBefore != level.getLightEmission(pos)) {
        level.getLightEngine().checkBlock(pos);
    }
    
    PacketDistributor.sendToServer(new C2SRemoveConduitFacadePacket(pos));
}

// Lines 105-106: Client still removes conduit before server response  
conduitBundle.removeConduit(conduit, droppedItem -> {});  // ⚠️ Client mutation
PacketDistributor.sendToServer(new C2SBreakConduitPacket(pos));

While the server now properly sends level.sendBlockUpdated(pos, blockstate, blockstate, Block.UPDATE_ALL) when the break event is canceled, this only syncs the block state, not the block entity data. The client's conduit bundle entity will still have the wrong facade/conduit state until a full chunk reload or explicit block entity resync.

For true rollback safety, the client should:

Only play visual effects (particles, sounds) immediately
Send the packet to server
Wait for server confirmation before mutating setFacadeProvider or removeConduit
Handle any server rejection by not changing state at all

The current approach still leaves a window for ghost state if the server rejects the action.

<details> <summary>🧩 Analysis chain</summary> I'll verify that the rollback handling has been properly implemented in the latest commits. --- 🏁 Script executed: ``` #!/bin/bash # Check the current implementation of ConduitBundleBlockBreakEventHandler.java cat enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java ``` Length of output: 146 --- Let me search for the file or similar break event handler implementations to see the current state: --- 🏁 Script executed: ``` #!/bin/bash # Search for files with similar names or break event handlers find . -name "*BreakEventHandler*" -type f find . -name "*Break*" -type f | grep -i conduit # Also search for any files containing "onLeftClickBlock" or similar break handling rg -l "onLeftClickBlock\|LeftClickBlock" --type java ``` Length of output: 300 --- Let me check the git history and see how conduit breaking is currently handled: --- 🏁 Script executed: ``` #!/bin/bash # Check git log for recent changes related to the break event handler git log --oneline --since="1 week ago" -- "*Break*" "*break*" | head -10 git log --oneline --since="1 week ago" | grep -i "break\|rollback\|client\|server" | head -10 # Check current implementation of conduit breaking in the bundle block rg -A 10 -B 5 "onDestroyedByPlayer\|playerWillDestroy\|destroy" enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java # Look at the server payload handler to see current break handling cat enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java | grep -A 20 -B 5 "C2SBreakConduitPacket" ``` Length of output: 1168 --- Let me get the complete server implementation and check for rollback handling: --- 🏁 Script executed: ``` #!/bin/bash # Get the full server handler implementation for break conduit rg -A 50 "handle.*C2SBreakConduitPacket" enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java # Check if there are any client-bound packets for rollback/sync rg -B 5 -A 10 "ClientboundBlockUpdatePacket\|rollback\|sync" enderio-conduits/src/main/java/com/enderio/conduits/common/network/ConduitServerPayloadHandler.java # Look for any client-side handling of break responses rg -l "ClientboundBlockUpdatePacket\|BlockUpdatePacket" --type java ``` Length of output: 2145 --- Let me check how the client-side breaking is currently handled to verify the fix: --- 🏁 Script executed: ``` #!/bin/bash # Look for current client-side break handling rg -B 10 -A 20 "C2SBreakConduitPacket\|sendToServer.*Break" --type java # Check if there are any client-side mutations before server confirmation rg -B 5 -A 15 "removeConduit\|setFacadeProvider" --type java enderio-conduits/src/main/java/com/enderio/conduits/client/ || echo "No client mutations found" # Look for any event handlers or interaction logic on client side rg -l "PlayerInteractEvent\|LeftClickBlock" --type java ``` Length of output: 25 --- Let me check how client-side conduit breaking is currently handled: --- 🏁 Script executed: ``` #!/bin/bash # Check the current ConduitBundleBlock implementation for any client-side handling cat enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java | grep -A 20 -B 5 "use\|attack\|destroy\|playerWillDestroy\|onDestroyedByPlayer" # Check if there's any client-side interaction handling in the block rg -A 15 -B 5 "InteractionResult\|ItemInteractionResult" enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlock.java # Look for any remaining client-side conduit interaction logic rg -B 5 -A 10 "PacketDistributor\|sendToServer" --type java | grep -A 15 -B 5 "Break\|conduit" ``` Length of output: 7539 --- </details> @Rover656 I can see the current implementation in `ConduitBundleBlock.onDestroyedByPlayer()`, and while there have been improvements, I still notice the same fundamental issue I originally flagged: **The client is still mutating local state before server confirmation:** ```java // Lines 82-90: Client still mutates facade state before server response if (conduitBundle.hasFacade() && FacadeUtil.areFacadesVisible(player)) { int lightLevelBefore = level.getLightEmission(pos); conduitBundle.setFacadeProvider(ItemStack.EMPTY); // ⚠️ Client mutation if (lightLevelBefore != level.getLightEmission(pos)) { level.getLightEngine().checkBlock(pos); } PacketDistributor.sendToServer(new C2SRemoveConduitFacadePacket(pos)); } // Lines 105-106: Client still removes conduit before server response conduitBundle.removeConduit(conduit, droppedItem -> {}); // ⚠️ Client mutation PacketDistributor.sendToServer(new C2SBreakConduitPacket(pos)); ``` While the server now properly sends `level.sendBlockUpdated(pos, blockstate, blockstate, Block.UPDATE_ALL)` when the break event is canceled, this only syncs the block state, not the block entity data. The client's conduit bundle entity will still have the wrong facade/conduit state until a full chunk reload or explicit block entity resync. **For true rollback safety, the client should:** 1. Only play visual effects (particles, sounds) immediately 2. Send the packet to server 3. Wait for server confirmation before mutating `setFacadeProvider` or `removeConduit` 4. Handle any server rejection by not changing state at all The current approach still leaves a window for ghost state if the server rejects the action.

Rover656 (Migrated from github.com) reviewed

2025-09-28 18:39:17 +00:00

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java

Rover656 (Migrated from github.com) commented

2025-09-28 18:39:17 +00:00

sendBlockUpdated also syncs block entity data - quit while you're ahead :)

coderabbitai[bot] (Migrated from github.com) reviewed

2025-09-28 18:40:01 +00:00

enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java

coderabbitai[bot] (Migrated from github.com) commented

2025-09-28 18:40:01 +00:00

@Rover656, you're absolutely right! I stand corrected. Thank you for the clarification - sendBlockUpdated() does indeed synchronize block entity data along with the block state, not just the block state as I mistakenly assumed.

The current implementation where the server calls level.sendBlockUpdated(pos, blockstate, blockstate, Block.UPDATE_ALL) when the break event is canceled will properly resync the block entity data back to the client, preventing any ghost state issues.

I appreciate the correction - the rollback handling is indeed properly implemented!

✏️ Learnings added

Learnt from: Rover656
PR: Team-EnderIO/EnderIO#1148
File: enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java:0-0
Timestamp: 2025-09-28T18:39:56.273Z
Learning: Level#sendBlockUpdated() in Minecraft/NeoForge also synchronizes block entity data to clients, not just block state. When a server-side action is canceled and sendBlockUpdated is called, it will resync both the block state and the block entity's data to prevent ghost state on the client.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

@Rover656, you're absolutely right! I stand corrected. Thank you for the clarification - `sendBlockUpdated()` does indeed synchronize block entity data along with the block state, not just the block state as I mistakenly assumed. The current implementation where the server calls `level.sendBlockUpdated(pos, blockstate, blockstate, Block.UPDATE_ALL)` when the break event is canceled will properly resync the block entity data back to the client, preventing any ghost state issues. I appreciate the correction - the rollback handling is indeed properly implemented! --- <details> <summary>✏️ Learnings added</summary> ``` Learnt from: Rover656 PR: Team-EnderIO/EnderIO#1148 File: enderio-conduits/src/main/java/com/enderio/conduits/common/conduit/bundle/ConduitBundleBlockBreakEventHandler.java:0-0 Timestamp: 2025-09-28T18:39:56.273Z Learning: Level#sendBlockUpdated() in Minecraft/NeoForge also synchronizes block entity data to clients, not just block state. When a server-side action is canceled and sendBlockUpdated is called, it will resync both the block state and the block entity's data to prevent ghost state on the client. ``` > Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure [review instructions](https://docs.coderabbit.ai/guides/review-instructions). You can manage existing learnings in the [app](https://app.coderabbit.ai/learnings). </details> ---  If you found this review helpful, would you consider giving us a shout-out on [X](https://twitter.com/intent/tweet?related=CodeRabbitAI&text=Avoid%20mutating%20the%20client%20before%20server%20ACK%20%E2%80%94%20it%20creates%20ghost%20conduit%2Ffacade%20state%3B%20defer%20state%20changes%20or%20request%20a%20block-entity%20resync%20on%20failure.%20Smart%20review%20by%20%40coderabbitai.%20Try%20it!%20%F0%9F%94%A7%E2%9A%A0%EF%B8%8F&url=https%3A%2F%2Fgithub.com%2FTeam-EnderIO%2FEnderIO%2Fpull%2F1148%23discussion_r2384406576)? Thank you for using CodeRabbit!

Sign in to join this conversation.

No reviewers