fix: hittest pending entities update#297
Open
draedful wants to merge 4 commits into
Open
Conversation
…ntities The old approach zeroed usableRectTracker to signal instability, but HitBox.update() skips re-registration when coordinates haven't changed, causing waitUsableRectUpdate to never resolve for unchanged block positions. New approach: explicit $pendingEntitiesUpdate signal set by markPendingUpdate(), cleared by processQueue. usableRectTracker is never cleared on setEntities. markPendingUpdate() no longer eagerly schedules processQueue — doing so caused it to fire in the same rAF frame (before MEDIUM/component renders), clearing $pendingEntitiesUpdate before new block hitboxes were queued. Instead, processQueue fires naturally when hitbox updates arrive from component renders. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Reorder unsubRect/unsubPending declarations before cleanup closure to resolve use-before-define warnings. Suppress callback-return warning with inline eslint comment. Auto-fix formatting in test file. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Reviewer's GuideIntroduces an explicit pending-entities signal in HitTest and wires Graph and the scheduler around it so waitUsableRectUpdate correctly waits for entity replacement without clearing usableRectTracker, plus adds targeted unit/integration tests and design docs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
debounce.flush, callingfn(...args)beforecurrentRemoveScheduler()means any re-scheduling done insidefncan be unintentionally canceled by the stalecurrentRemoveScheduler; consider clearing state and callingcurrentRemoveSchedulerfirst, then invokingfn, or otherwise guarding against canceling a newly scheduled run. waitUsableRectUpdatenow subscribes to both$usableRectand$pendingEntitiesUpdateand only unsubscribes whenisUnstablebecomes false; if a caller never invokes the returned cleanup and the hit test instance is long-lived, this can lead to lingering subscribers, so you may want a more centralized teardown path (e.g., tying these subscriptions into theclear/destroy lifecycle).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `debounce.flush`, calling `fn(...args)` before `currentRemoveScheduler()` means any re-scheduling done inside `fn` can be unintentionally canceled by the stale `currentRemoveScheduler`; consider clearing state and calling `currentRemoveScheduler` first, then invoking `fn`, or otherwise guarding against canceling a newly scheduled run.
- `waitUsableRectUpdate` now subscribes to both `$usableRect` and `$pendingEntitiesUpdate` and only unsubscribes when `isUnstable` becomes false; if a caller never invokes the returned cleanup and the hit test instance is long-lived, this can lead to lingering subscribers, so you may want a more centralized teardown path (e.g., tying these subscriptions into the `clear`/destroy lifecycle).
## Individual Comments
### Comment 1
<location path="src/services/HitTest.ts" line_range="97-100" />
<code_context>
+Replace with:
+```typescript
+ if (hasZeroUsableRect && !this.hasGraphElements()) {
+ return hasProcessingQueue || this.$pendingEntitiesUpdate.value;
+ }
+```
</code_context>
<issue_to_address>
**issue:** Handling `setEntities` -> empty graph may leave `isUnstable` stuck true with no path to clear.
With the new `pendingEntitiesUpdate` flag, `isUnstable` returns `hasProcessingQueue || this.$pendingEntitiesUpdate.value` when `hasZeroUsableRect && !this.hasGraphElements()`. If `setEntities` is called with an empty blocks list, `markPendingUpdate()` sets the flag, but there may be no hitbox updates to run `processQueue`, so the flag might never be cleared and `waitUsableRectUpdate` would never resolve for an empty graph. Consider explicitly clearing `pendingEntitiesUpdate` when transitioning to an empty graph, or treating `!hasGraphElements` as stable even if the flag is true, so `waitUsableRectUpdate` remains usable for empty results.
</issue_to_address>
### Comment 2
<location path="src/services/HitTest.ts" line_range="235" />
<code_context>
+
+Current code (around lines 215–225):
+```typescript
+ if (this.isUnstable) {
+ const removeListener = this.$usableRect.subscribe(() => {
+ if (!this.isUnstable) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a single stability tick signal and a local pending flag variable to simplify waitUsableRectUpdate subscriptions and the isUnstable logic.
You can keep the new behavior but move the coordination logic out of `waitUsableRectUpdate`, so it only has a single subscription again, and simplify the `isUnstable` getter a bit.
### 1. Centralize the “stability-related changes” into a single signal
Instead of dual subscriptions + manual cleanup in `waitUsableRectUpdate`, introduce a simple “tick” signal that bumps whenever either `$usableRect` or `$pendingEntitiesUpdate` changes. Then `waitUsableRectUpdate` can subscribe once and keep its previous structure.
```ts
// New field
private readonly $stabilityTick = signal(0);
constructor(protected graph: Graph) {
super();
// Any change that could affect stability increments the tick
this.$usableRect.subscribe(() => {
this.$stabilityTick.value++;
});
this.$pendingEntitiesUpdate.subscribe(() => {
this.$stabilityTick.value++;
});
}
```
Then `waitUsableRectUpdate` becomes:
```ts
public waitUsableRectUpdate(callback: (rect: TRect) => void): () => void {
// For empty graphs, immediately call callback with current usableRect
if (!this.hasGraphElements()) {
callback(this.$usableRect.value);
return noop;
}
if (this.isUnstable) {
const unsubscribe = this.$stabilityTick.subscribe(() => {
if (!this.isUnstable) {
unsubscribe();
callback(this.$usableRect.value);
}
});
return unsubscribe;
}
callback(this.$usableRect.value);
return noop;
}
```
Behavior stays the same: we still wake up when either rect changes or the pending flag flips, but the complexity is now in one small place (the constructor wiring), and `waitUsableRectUpdate` regains a single, easy-to-read subscription.
### 2. Reduce duplication in `isUnstable`
You can make the logic a bit easier to parse by hoisting the pending flag into a local:
```ts
public get isUnstable() {
const hasProcessingQueue = this.processQueue.isScheduled() || this.queue.size > 0;
const hasZeroUsableRect =
this.$usableRect.value.height === 0 &&
this.$usableRect.value.width === 0 &&
this.$usableRect.value.x === 0 &&
this.$usableRect.value.y === 0;
const hasPendingEntitiesUpdate = this.$pendingEntitiesUpdate.value;
// If graph has no elements, it's stable even with zero usableRect
if (hasZeroUsableRect && !this.hasGraphElements()) {
return hasProcessingQueue || hasPendingEntitiesUpdate;
}
return hasProcessingQueue || hasZeroUsableRect || hasPendingEntitiesUpdate;
}
```
Same semantics, but the duplicated `this.$pendingEntitiesUpdate.value` access and condition are clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -213,15 +233,25 @@ export class HitTest extends Emitter { | |||
| } | |||
|
|
|||
| if (this.isUnstable) { | |||
There was a problem hiding this comment.
issue (complexity): Consider introducing a single stability tick signal and a local pending flag variable to simplify waitUsableRectUpdate subscriptions and the isUnstable logic.
You can keep the new behavior but move the coordination logic out of waitUsableRectUpdate, so it only has a single subscription again, and simplify the isUnstable getter a bit.
1. Centralize the “stability-related changes” into a single signal
Instead of dual subscriptions + manual cleanup in waitUsableRectUpdate, introduce a simple “tick” signal that bumps whenever either $usableRect or $pendingEntitiesUpdate changes. Then waitUsableRectUpdate can subscribe once and keep its previous structure.
// New field
private readonly $stabilityTick = signal(0);
constructor(protected graph: Graph) {
super();
// Any change that could affect stability increments the tick
this.$usableRect.subscribe(() => {
this.$stabilityTick.value++;
});
this.$pendingEntitiesUpdate.subscribe(() => {
this.$stabilityTick.value++;
});
}Then waitUsableRectUpdate becomes:
public waitUsableRectUpdate(callback: (rect: TRect) => void): () => void {
// For empty graphs, immediately call callback with current usableRect
if (!this.hasGraphElements()) {
callback(this.$usableRect.value);
return noop;
}
if (this.isUnstable) {
const unsubscribe = this.$stabilityTick.subscribe(() => {
if (!this.isUnstable) {
unsubscribe();
callback(this.$usableRect.value);
}
});
return unsubscribe;
}
callback(this.$usableRect.value);
return noop;
}Behavior stays the same: we still wake up when either rect changes or the pending flag flips, but the complexity is now in one small place (the constructor wiring), and waitUsableRectUpdate regains a single, easy-to-read subscription.
2. Reduce duplication in isUnstable
You can make the logic a bit easier to parse by hoisting the pending flag into a local:
public get isUnstable() {
const hasProcessingQueue = this.processQueue.isScheduled() || this.queue.size > 0;
const hasZeroUsableRect =
this.$usableRect.value.height === 0 &&
this.$usableRect.value.width === 0 &&
this.$usableRect.value.x === 0 &&
this.$usableRect.value.y === 0;
const hasPendingEntitiesUpdate = this.$pendingEntitiesUpdate.value;
// If graph has no elements, it's stable even with zero usableRect
if (hasZeroUsableRect && !this.hasGraphElements()) {
return hasProcessingQueue || hasPendingEntitiesUpdate;
}
return hasProcessingQueue || hasZeroUsableRect || hasPendingEntitiesUpdate;
}Same semantics, but the duplicated this.$pendingEntitiesUpdate.value access and condition are clearer.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Preview is ready. |
Prevents a newly-scheduled debounced run from being accidentally canceled when fn() triggers a re-schedule during flush execution. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
draedful
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce an explicit pending-entities flag in hit testing to make usable-rect updates robust during entity replacement and adjust scheduling to avoid stale or hanging updates.
Bug Fixes:
Enhancements:
Documentation:
Tests: