Refactor SpigotEntityIdProvider ID supplier logic

[TWME-TW]

Refactor entity ID supplier detection logic to improve maintainability and clarity.

This pull request was written by AI. It currently fixes an issue I encountered in Spigot 1.21.10.

However, since I am not very familiar with reflection, I am not entirely sure about the contents of this PR. Therefore, please make sure to review it carefully to ensure that this PR meets the requirements.

Summary by CodeRabbit

• Refactor
  • Optimized entity ID generation logic with enhanced support across multiple server versions to improve stability and compatibility.

[coderabbitai]

Walkthrough

Refactored SpigotEntityIdProvider to improve ID-generation robustness by replacing direct reflection logic with dedicated helper methods that handle multiple entity ID storage mechanisms across different Minecraft and Paper versions.

Changes

Cohort / File(s)	Summary
ID-Generation Strategy Refactoring platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java	Replaced monolithic reflection logic with modular helper methods: resolveAtomicSupplier() for 1.14+ AtomicInteger fields, resolveLegacySupplier() for legacy int fields, and getStaticFieldOfType() for generic field location. Added version-specific branching with Paper 1.16+ path preservation via UnsafeValues.nextEntityId. Introduced Modifier import for reflection.

Sequence Diagram

sequenceDiagram
    participant Client
    participant detectIdSupplier
    participant resolveAtomicSupplier
    participant resolveLegacySupplier
    participant getStaticFieldOfType

    Client->>detectIdSupplier: Request ID supplier
    alt Paper 1.16+
        detectIdSupplier->>detectIdSupplier: Use UnsafeValues.nextEntityId
    else Minecraft 1.14+
        detectIdSupplier->>resolveAtomicSupplier: Resolve AtomicInteger field
        resolveAtomicSupplier->>getStaticFieldOfType: Locate AtomicInteger field
        getStaticFieldOfType-->>resolveAtomicSupplier: Field located
        resolveAtomicSupplier-->>detectIdSupplier: Increment supplier ready
    else Legacy versions
        detectIdSupplier->>resolveLegacySupplier: Resolve int field
        resolveLegacySupplier->>getStaticFieldOfType: Locate int field
        getStaticFieldOfType-->>resolveLegacySupplier: Field located
        resolveLegacySupplier-->>detectIdSupplier: Read/increment supplier ready
    end
    detectIdSupplier-->>Client: ID supplier

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Reflection logic complexity: Review the field-location strategy in getStaticFieldOfType() for correctness across type matching and fallback scanning
• Version-specific branching: Verify conditional logic for Paper 1.16+ detection and proper delegation between atomic and legacy paths
• Type safety: Ensure AtomicInteger and int field resolution validates types correctly before wrapping in suppliers
• Backward compatibility: Confirm all legacy version paths (field names, access patterns) remain intact

Poem

🐰 A rabbit hopped through reflection's maze,
With fields to find through many days,
Now atomic paths and legacy lanes,
Flow smoothly through version chains,
Each ID springs forth with graceful ease! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: refactoring the ID supplier logic in SpigotEntityIdProvider with improved maintainability.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

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

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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)

platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java (2)

69-76: Version‑aware supplier selection and helper split look sound

The refactored detectIdSupplier() flow (Paper fast‑path → 1.14+ atomic path → legacy path) plus resolveAtomicSupplier/resolveLegacySupplier is logically consistent and keeps behavior centralized while preserving a clear failure mode via IllegalStateException. The atomic path cleanly falls back to legacy when no suitable AtomicInteger is found.

If you want easier diagnostics on odd future versions, consider (optionally) adding a debug log when:

• resolveAtomicSupplier returns null despite a 1.14+ version, and/or
• resolveLegacySupplier throws because no legacy counter field was found.

This would make it much easier to understand reflection breakage on new Spigot/Paper releases without changing current behavior.

Also applies to: 78-112

---

153-169: getStaticFieldOfType helper improves robustness; watch first‑match fallback semantics

Centralizing the reflection into getStaticFieldOfType with:

• type checks (desiredType.isAssignableFrom(field.getType())), and
• a static‑modifier check,

is a solid improvement and should catch most NMS name changes automatically.

One thing to be aware of: the second loop returns the first static field of the requested type. If a future Entity class ever gains multiple static AtomicInteger or int fields, this might pick the wrong one. That’s probably acceptable given current NMS layouts, but if this ever becomes an issue, you could tighten the heuristic (e.g., filter by final, by name prefix, or by requiring uniqueness before accepting a match).

Also, the Javadoc above detectIdSupplier() still mentions only "entityCount"/"d"/"c" and not the newer aliases ("counter", "nextEntityId", "b"); updating that comment would better reflect the actual lookup behavior.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df6fe0f and 7ba8d1b.

📒 Files selected for processing (1)

• platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java (3 hunks)

🔇 Additional comments (1)

platforms/spigot/src/main/java/me/tofaa/entitylib/spigot/SpigotEntityIdProvider.java (1)

3-20: Imports for reflection and Bukkit/Paper APIs look correct

The added Modifier import and the Bukkit / Paper / Platform imports align with the new helper and usage; no issues here.

[Tofaa2]

LGTM!

[Tofaa2]

Thank you!

[TWME-TW]

Please make sure to review this PR carefully, because I don't really know what I'm doing.