Add parcel return feature

[Jakubk15]

• feat: add COLLECTED parcel status and guard ParcelSendTask against re-delivery
• feat: add CollectedParcel domain and collected_parcels repository
• fix: compare return-window expiry temporally, not lexicographically
• feat: add conditional collect/return status flips and returnable query to ParcelRepository
• feat: add return window, fees, attribute-check flags and return notices to config
• feat: add config-driven item equivalence for parcel returns
• feat: add multiset return-content validator
• feat: keep collected parcels for the return window instead of deleting them
• feat: add parcel return service and ParcelReturnEvent
• fix: keep post-commit return failures away from the refund path
• feat: purge collected parcels after the return window expires
• feat: add return button, return list GUI and deposit GUI (Parcel return logic #69)

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive parcel return system, including return validation, configurable item equivalence checks, database persistence for collected parcels, a periodic purge task for expired returns, and dedicated return GUIs. The code review highlights critical thread-safety issues with asynchronous Vault economy calls, potential NullPointerExceptions due to missing null/air checks in item validation, a potential ConcurrentModificationException when modifying item enchantments, and a performance concern regarding full table scans for expired parcels.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Thread Safety Issue: Asynchronous Vault Economy Call

Calling Vault's withdrawPlayer on an asynchronous thread is highly unsafe. Most economy providers (such as Essentials, CraftConomy, etc.) are not thread-safe and expect all economy transactions to be executed on the main server thread. Running this asynchronously can lead to race conditions, database deadlocks, or ConcurrentModificationExceptions.

Please wrap this economy withdrawal in a synchronous scheduler task to ensure it runs on the main thread.

[gemini-code-assist]

Thread Safety Issue: Asynchronous Vault Economy Call

Similar to the withdrawal logic, calling depositPlayer on an asynchronous thread is unsafe because Vault economy providers are generally not thread-safe.

Please ensure that the refund operation is scheduled to run on the main server thread using this.scheduler.run(...).

[gemini-code-assist]

Defensive Programming: Missing Null Checks

If either expected or actual is null, calling expected.getType() will throw a NullPointerException. Adding a defensive null check at the beginning of the method prevents potential crashes.

    @Override
    public boolean test(ItemStack expected, ItemStack actual) {
        if (expected == null || actual == null) {
            return expected == actual;
        }
        if (expected.getType() != actual.getType()) {
            return false;
        }

[gemini-code-assist]

Defensive Programming: Missing Null/Air Checks on List Elements

If the expected or deposited lists contain null or empty/air items, calling item.getAmount() will throw a NullPointerException. Adding a defensive check to skip null or air items in both loops ensures robustness.

    public boolean matches(List<ItemStack> deposited, List<ItemStack> expected) {
        List<ItemStack> samples = new ArrayList<>();
        List<Integer> expectedTotals = new ArrayList<>();
        List<Integer> depositedTotals = new ArrayList<>();

        for (ItemStack item : expected) {
            if (item == null || item.getType().isAir()) {
                continue;
            }
            int index = this.indexOf(samples, item);
            if (index < 0) {
                samples.add(item);
                expectedTotals.add(item.getAmount());
                depositedTotals.add(0);
                continue;
            }
            expectedTotals.set(index, expectedTotals.get(index) + item.getAmount());
        }

        for (ItemStack item : deposited) {
            if (item == null || item.getType().isAir()) {
                continue;
            }
            int index = this.indexOf(samples, item);
            if (index < 0) {
                return false;
            }
            depositedTotals.set(index, depositedTotals.get(index) + item.getAmount());
        }

        return expectedTotals.equals(depositedTotals);
    }

[gemini-code-assist]

Potential ConcurrentModificationException

Iterating over meta.getEnchants().keySet() and modifying the underlying ItemMeta via meta.removeEnchant inside the loop can throw a ConcurrentModificationException if the map returned by getEnchants() is a live view or if the implementation modifies the collection being iterated.

Copying the key set to a temporary list before iterating is a safer and more robust approach.

Suggested change

	if (!this.checks.checkEnchantments) {
	meta.getEnchants().keySet().forEach(meta::removeEnchant);
	}
	if (!this.checks.checkEnchantments) {
	new ArrayList<>(meta.getEnchants().keySet()).forEach(meta::removeEnchant);
	}

[gemini-code-assist]

Performance: Full Table Scan and In-Memory Filtering

Loading the entire collected_parcels table into memory and filtering it in Java is an anti-pattern that can degrade performance as the database grows.

While different fractional renderings of ISO-8601 strings can cause sub-second lexicographical misordering, a slight sub-second inaccuracy at the 7-day boundary is completely negligible for a purge task running every 30 minutes. Using a database-level WHERE clause with a range operator is much more efficient and scalable. Alternatively, storing the timestamp as epoch milliseconds (long) would allow perfect temporal filtering directly in SQL.