Skip to content

fix: Stop tracking special expressions that don't refer to any variables after their values have been computed#4193

Open
jswalden wants to merge 2 commits into
bitfocus:mainfrom
jswalden:optimize-special-expression-tracking
Open

fix: Stop tracking special expressions that don't refer to any variables after their values have been computed#4193
jswalden wants to merge 2 commits into
bitfocus:mainfrom
jswalden:optimize-special-expression-tracking

Conversation

@jswalden
Copy link
Copy Markdown
Contributor

@jswalden jswalden commented May 27, 2026

This is a followup optimization after #4065.

The value of a special expression only changes if the value of a variable it depends upon changes. A special expression that refers to no variables, can never change. Therefore, there's no need to keep tracking such expression to recompute it as variables change value.

Before this change, it looks like every single feedback was being continually tracked for isInverted special expressions, for all time, and same now for all actions for storeResult as of that feature landing. This will minimize the former to exactly those feedbacks containing an isInverted that depends upon a variable, and the latter to only those actions that depend upon a variable -- the least tracking possible.

Summary by CodeRabbit

  • Refactor
    • Improved special-expression tracking and recomputation to reduce unnecessary work, speed up updates, and make lifecycle behavior more predictable.
  • Tests
    • Added focused tests validating lifecycle state transitions and timed recomputation, ensuring expression evaluation and update notifications occur reliably and only once when expected.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8c6039fe-8e84-4e3a-96c3-7b4477c026e1

📥 Commits

Reviewing files that changed from the base of the PR and between fcbc288 and 4eb51f0.

📒 Files selected for processing (2)
  • companion/lib/Controls/Entities/EntitySpecialExpressionManager.ts
  • companion/test/Controls/Entities/EntitySpecialExpressionManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • companion/test/Controls/Entities/EntitySpecialExpressionManager.test.ts
  • companion/lib/Controls/Entities/EntitySpecialExpressionManager.ts

📝 Walkthrough

Walkthrough

Manager replaces dual per-entity flags with a single referencedVariableIds: Set | null. Compute functions now return { referencedVariableIds, computedValue }. The debounced processor recomputes only when null, updates wrappers, deletes entries with no dependencies, and invalidates wrappers when variables change.

Changes

Entity recomputation state tracking

Layer / File(s) Summary
State tracking model and private field rename
companion/lib/Controls/Entities/EntitySpecialExpressionManager.ts
Replace per-entity needsProcessing / lastReferencedVariableIds with referencedVariableIds: Set\<string\> | null; rename #specialExpressions to specialExpressions and update constructor/types.
Compute function implementations
companion/lib/Controls/Entities/EntitySpecialExpressionManager.ts
Refactor isInverted and storeResult to return { referencedVariableIds, computedValue }; non-expression/undefined return referencedVariableIds: null; expression cases return computed values and dependency sets (unioning local-variable dependencies).
Entity initialization, processing, and invalidation
companion/lib/Controls/Entities/EntitySpecialExpressionManager.ts
trackEntity initializes wrappers with referencedVariableIds: null. Debounced processor uses Object.entries, recomputes only when null, updates wrappers from compute results, deletes trackers with empty dependency sets, forgetEntity removes wrappers, and onVariablesChanged invalidates by nulling when changed variables intersect previous dependencies.
Tests: referencedVariableIds lifecycle
companion/test/Controls/Entities/EntitySpecialExpressionManager.test.ts
Import updates and a new test suite verify timer-driven referencedVariableIds transitions for isInverted and storeResult wrappers, asserting execute/parse timing and exact-once update calls.

Poem

One field to hold what once was two,
Null signals "recompute" — crisp and true.
Dependencies gathered, or none to show,
The manager wakes, updates, or lets go.
🎉 Thanks — nice tidy refactor!

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main optimization: stopping unnecessary tracking of special expressions with no variable dependencies after computation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

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

Copy link
Copy Markdown
Member

@Julusian Julusian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have a test or two to verify that this works as expected.
Probably requires adding a method to be able to query the internal state of this class a little, but that is an acceptable tradeoff

@jswalden jswalden changed the title feat: Stop tracking special expressions that don't refer to any variables after their values have been computed fix: Stop tracking special expressions that don't refer to any variables after their values have been computed May 27, 2026
@jswalden jswalden force-pushed the optimize-special-expression-tracking branch from 9ffc83a to 1dda76e Compare May 28, 2026 00:41
@jswalden
Copy link
Copy Markdown
Contributor Author

Probably requires adding a method to be able to query the internal state of this class a little, but that is an acceptable tradeoff

Turns out no, indexed access can evade TypeScript access restrictions so just needed to make this.#specialExpressions private rather than a private field.

Added miscellaneous tests, didn't want to be too overly tied to the exact timer dispatch so I had to do some manual looping in poor-man's state tracking. Original patch is the same, all adjustments are in the second patch -- and aside from the rename all is in test code.

@jswalden jswalden force-pushed the optimize-special-expression-tracking branch from 1dda76e to fcbc288 Compare May 28, 2026 00:44
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e3f864cb-5e88-44b8-8660-dd3c2d2b9cd9

📥 Commits

Reviewing files that changed from the base of the PR and between 9ffc83a and 1dda76e.

📒 Files selected for processing (2)
  • companion/lib/Controls/Entities/EntitySpecialExpressionManager.ts
  • companion/test/Controls/Entities/EntitySpecialExpressionManager.test.ts

Comment thread companion/test/Controls/Entities/EntitySpecialExpressionManager.test.ts Outdated
@jswalden jswalden force-pushed the optimize-special-expression-tracking branch from fcbc288 to 4eb51f0 Compare May 28, 2026 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants