feat: add NeoForge storage adapters#369
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces complete NeoForge adapter implementations for three resource types: energy, fluids, and items. Each resource type receives a key wrapper class (e.g., Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java (1)
34-62: 💤 Low valueConsider reusing the transaction-checking pattern from
NeoForgeEnergyStorage.
NeoForgeEnergyStorageuses anopenTransaction()helper that checks for an existing transaction and opens a nested transaction if one is already open. Here,Transaction.openRoot()is always called, which could break atomicity if these methods are ever invoked within an existing NeoForge transaction context (the new root would commit independently).For consistency and future-proofing, consider extracting the same helper pattern:
private static Transaction openTransaction() { TransactionContext current = Transaction.getCurrentOpenedTransaction(); return current == null ? Transaction.openRoot() : Transaction.open(current); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java` around lines 34 - 62, The insert/extract methods call Transaction.openRoot() directly which can break atomicity when invoked inside an existing transaction; add a private static Transaction openTransaction() helper (as used in NeoForgeEnergyStorage) that checks Transaction.getCurrentOpenedTransaction() and returns Transaction.openRoot() or Transaction.open(current) accordingly, then replace all Transaction.openRoot() calls in insert(IItemKey...), extract(IItemKey...) with openTransaction() so these methods join existing transactions instead of always starting a new root.neoforge/src/main/java/com/logistics/neoforge/energy/NeoForgeEnergyStorage.java (1)
144-153: 💤 Low valueConsider validating commit result to detect unexpected shortfalls.
If the capacity calculation bug above is fixed, this should be safe. However, if for any reason the actual
storage.insertorstorage.extractreturns less thandelta, the discrepancy is silently ignored. Consider logging or asserting that the returned value matches expectations for debugging purposes in development builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@neoforge/src/main/java/com/logistics/neoforge/energy/NeoForgeEnergyStorage.java` around lines 144 - 153, The onRootCommit method applies pendingDelta to storage but ignores the returned amount from storage.insert and storage.extract, so shortfalls are silently dropped; update onRootCommit to capture the returned long from storage.insert(delta, false) and storage.extract(-delta, false) and then compare it to the expected delta (pendingDelta/original calculation) and in development builds either assert equality or log an error showing expected vs actual (use a descriptive message including pendingDelta, delta, and the returned amount) so unexpected shortfalls are detected for debugging; keep the existing behavior in production but ensure the debug/assert path triggers when values differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@neoforge/src/main/java/com/logistics/neoforge/energy/NeoForgeEnergyStorage.java`:
- Around line 114-122: The insert method is not accounting for already-staged
inserts (pendingDelta) and can over-stage; fix NeoForgeEnergyStorage.insert by
computing the actual insertable amount as the simulated storage.insert(amount,
true) minus the current pendingDelta (use getAmountAsLong() or pendingDelta to
reference staged amount), clamp that to >=0, then add that computed inserted
long to pendingDelta and return clampToInt(inserted); keep calling
updateSnapshots(transaction) first and preserve types and clampToInt usage so
commit sees the correct staged total.
---
Nitpick comments:
In
`@neoforge/src/main/java/com/logistics/neoforge/energy/NeoForgeEnergyStorage.java`:
- Around line 144-153: The onRootCommit method applies pendingDelta to storage
but ignores the returned amount from storage.insert and storage.extract, so
shortfalls are silently dropped; update onRootCommit to capture the returned
long from storage.insert(delta, false) and storage.extract(-delta, false) and
then compare it to the expected delta (pendingDelta/original calculation) and in
development builds either assert equality or log an error showing expected vs
actual (use a descriptive message including pendingDelta, delta, and the
returned amount) so unexpected shortfalls are detected for debugging; keep the
existing behavior in production but ensure the debug/assert path triggers when
values differ.
In
`@neoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java`:
- Around line 34-62: The insert/extract methods call Transaction.openRoot()
directly which can break atomicity when invoked inside an existing transaction;
add a private static Transaction openTransaction() helper (as used in
NeoForgeEnergyStorage) that checks Transaction.getCurrentOpenedTransaction() and
returns Transaction.openRoot() or Transaction.open(current) accordingly, then
replace all Transaction.openRoot() calls in insert(IItemKey...),
extract(IItemKey...) with openTransaction() so these methods join existing
transactions instead of always starting a new root.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f08bf6d-9775-4332-af38-f37a81019a05
📒 Files selected for processing (7)
.gitignoreneoforge/src/main/java/com/logistics/neoforge/energy/NeoForgeEnergyCapabilityLookup.javaneoforge/src/main/java/com/logistics/neoforge/energy/NeoForgeEnergyStorage.javaneoforge/src/main/java/com/logistics/neoforge/fluids/NeoForgeFluidKey.javaneoforge/src/main/java/com/logistics/neoforge/fluids/NeoForgeFluidStorage.javaneoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemKey.javaneoforge/src/main/java/com/logistics/neoforge/storage/NeoForgeItemStorage.java
…to pr/neoforge-storage-adapters
1 participant
Summary
Part 1 of the NeoForge runtime wiring stack.
Adds NeoForge equivalents for the Fabric storage adapters so common code can interact with NeoForge item, fluid, and energy APIs through the existing loader-agnostic abstractions.
Changes
NeoForgeItemStorageandNeoForgeItemKey.NeoForgeFluidStorageandNeoForgeFluidKey.NeoForgeEnergyStorage.NeoForgeEnergyCapabilityLookupto NeoForge energy capabilities.Notes
This PR intentionally does not register capabilities yet. It only adds the adapter layer needed by the next PR in the stack.
Validation
Validated on the final stack:
./gradlew :neoforge:build./gradlew :fabric:build./gradlew spotlessCheck./gradlew :neoforge:runServerreachedDonewithout recipe parse errors