Fixes NPE when spam clicking right click with a chisel

[TheFifthRider]

This should be marked as nullable :(

May resolve #106

Summary by CodeRabbit

• Bug Fixes
  • Enhanced robustness of block interaction mechanics to prevent potential crashes when certain interaction parameters are unavailable.

[coderabbitai]

Walkthrough

The OnHeldInteractStop method in CollectibleBehaviorScrapeCrucible was modified to accept a nullable BlockSelection parameter. A null-check was added to prevent accessing .Position on a null blockSel, addressing a server-side NullReferenceException that caused player disconnections during chiseling.

Changes

Cohort / File(s)	Summary
Null-safety fix for block selection SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs	Method signature updated: OnHeldInteractStop parameter blockSel changed from BlockSelection to BlockSelection? (nullable). Early guard added in server logic to skip processing when blockSel or blockSel.Position is null, preventing NullReferenceException crashes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Single file with focused changes to one method
• Straightforward null-safety pattern applied
• Method signature change requires verification against base class or interface contracts
• Ensure null-check logic doesn't skip critical server-side operations unintentionally

Poem

A chisel swung, a crash did strike, 🪨
Null references caused the blight,
Now guards stand tall at method's gate,
Block selections meet their fate,
No more disconnects in sight! 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Fixes NPE when spam clicking right click with a chisel" clearly and specifically describes the main change in the changeset. The code modifications directly address this objective by making the BlockSelection parameter nullable and adding null-safety checks to the OnHeldInteractStop method, which aligns with fixing the NullReferenceException reported in issue #106. The title is concise, non-vague, and accurately represents the primary purpose of the changes without unnecessary noise.
Linked Issues Check	✅ Passed	The code changes directly address the requirements from issue #106. The PR makes the BlockSelection parameter nullable (BlockSelection?) and adds an early guard check that returns if blockSel.Position is null, which prevents the NullReferenceException that was causing server disconnects when chiseling. The implementation aligns with the implied objectives of properly annotating nullable values and adding necessary null-checks to prevent the crash on line 51 of CollectibleBehaviorScrapeCrucible.cs.
Out of Scope Changes Check	✅ Passed	All changes in this pull request are directly in scope and related to fixing the NPE issue. Only one file is modified (CollectibleBehaviorScrapeCrucible.cs) with changes limited to the OnHeldInteractStop method signature and its null-safety implementation. The modifications—making BlockSelection nullable and adding null checks—are precisely what is needed to resolve the NullReferenceException reported in issue #106, with no extraneous or unrelated modifications present.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs (2)

30-37: Consider making blockSel nullable for consistency.

OnHeldInteractStep uses a non-nullable BlockSelection parameter while OnHeldInteractStart (line 13) and OnHeldInteractStop (line 42) both accept nullable BlockSelection?. For consistency and defensive coding, consider updating this signature to match the pattern:

 public override bool OnHeldInteractStep(float secondsUsed, ItemSlot slot, EntityAgent byEntity,
-    BlockSelection blockSel,
+    BlockSelection? blockSel,
     EntitySelection entitySel, ref EnumHandling handling)
 {
+    if (blockSel is null) return false;
     byEntity.StartAnimation("knifecut");
     handling = EnumHandling.PreventDefault;
     return secondsUsed < 1.5;
 }

---

130-135: Update method signature to accept nullable BlockSelection?.

After the fix, TryGetSelectedGroundStorage is called at line 52 with a BlockSelection? argument. To avoid potential compiler warnings and maintain consistency, update the method signature:

 private static BlockEntityGroundStorage? TryGetSelectedGroundStorage(EntityPlayer entityPlayer,
-    BlockSelection blockSel)
+    BlockSelection? blockSel)
 {
+    if (blockSel?.Position is null) return null;
     var blockEntity = entityPlayer.World.BlockAccessor.GetBlockEntity(blockSel.Position);
     return blockEntity as BlockEntityGroundStorage;
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef40e45 and 5fdc9a7.

📒 Files selected for processing (1)

• SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs (2 hunks)

🔇 Additional comments (2)

SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs (2)

39-44: Good fix for the NPE issue.

Making blockSel nullable correctly aligns with the base class contract and matches the pattern already used in OnHeldInteractStart (line 13).

---

51-51: Null check correctly prevents the NPE.

The guard clause effectively addresses the server-side crash reported in issue #106 by ensuring both blockSel and its Position property are non-null before proceeding.