Repository navigation
fix(cache): preserve full varp values on varbit changes - #2132
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: varbit change handling now preserves full parent varp values in the cache. It matches the summary and objectives. It is short and specific, so it passes. Description check The description states the problem and the fix. It says varbit events now read the full parent varp from the client, and varp-only updates keep the event value. This relates directly to the changeset,… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description states the problem and the fix. It says varbit events now read the full parent varp from the client, and varp-only updates keep the event value. This relates directly to the changeset, so it passes.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
chsami
left a comment
chsami
left a comment
There was a problem hiding this comment.
Thanks for this fix. Storing only the bitfield value as the whole varp was a real bug, and refreshing the parent value means scripts reading a varp after a varbit change now see the correct number.
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/api/playerstate/Rs2PlayerStateCache.java:105 - CI is red because of the new
client.getVarpValue(...)call. Run 40708893640 fails:client:runUnitTestsinClientThreadGuardrailTest > rs2AndQueryApiMustNotCallInferredListMethodsOffClientThreadwith one new violation:Rs2PlayerStateCache#onVarbitChanged(VarbitChanged) -> Client#getVarpValue(int). The guardrail keeps client API calls in the Rs2/Query API from running off the client thread, which is what causes random wrong values and crashes for players. The PR cannot merge while this test fails. Smallest acceptable direction, pick one:- Do not read the client here: for varbit events, call
varps.remove(event.getVarpId())so the nextgetVarpValuerefetches the full value through the existing path. This drops the new call entirely and still fixes the bug (adjust the tests to match). - Or, since
onVarbitChangedis an event handler that already runs on the client thread, keep the call and regenerate the baseline with-Dmicrobot.guardrail.regenerate-baseline=true, then commit the baseline diff so the exception is visible in review.
- Do not read the client here: for varbit events, call
Push an update or reply here and the maintainer bot reviews it again on a later run.
… the client The varbit event handler is guarded against client API calls. Removing the cached parent varp lets the next read refetch the full value through the existing client-thread path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2jMX5mBiWgqWdWSSPcBDW
chsami
left a comment
chsami
left a comment
There was a problem hiding this comment.
Applied the requested direction: varbit events now drop the cached parent varp instead of reading the client, so the next read refetches the full value on the client thread. Guardrail and cache tests pass. Thanks for the fix.
VarbitChanged supplies the changed bitfield value even when it also identifies the parent varp. Copying it into both caches could replace a full value such as 384 with 1, leaving scripts with incorrect player state.
Read the full parent varp from the client for varbit events, preserving unrelated bits and retaining the event value for varp-only updates.