Replace O(N) blockList based lookups with O(1) blockIndex property

[Ashutoshx7]

Description

This PR optimizes the performance of block lookups by replacing O(N) blockList.indexOf(this) calls with O(1) this.blockIndex property access.

Key Changes:

1. Index Caching: Added blockIndex property to the Block class (initialized to -1). This property is updated whenever a block is added to blockList via makeNewBlock.
   • Note: makeNewBlock is the central point for block creation, covering new blocks, loaded projects, and pasted stacks.
2. Conversion: Replaced ~33 instances of blockList.indexOf(...) across js/block.js, js/blocks.js, js/piemenus.js, and js/activity.js with direct blockIndex access.
3. Safety: blockIndex defaults to -1, preserving the behavior of indexOf (returning -1) for blocks not in the list.

Performance Impact

• Before: Operations like artwork generation, event handling, and finding block IDs required scanning the entire blockList (O(N)). For projects with thousands of blocks, this caused measurable overhead.
• After: These lookups are now constant time (O(1)), significantly improving responsiveness in large projects.

Verification

• Unit Tests: npm test passed (110 suites, 3108 tests).
• Browser Test: Manually verified that:
  • The application loads successfully.
  • Blocks can be dragged from the palette and placed on the canvas.
  • No console errors related to blockIndex or undefined were observed during interaction.

[github-actions]

✅ All Jest tests passed! This PR is ready to merge.

[kartikktripathi]

While the idea of caching blockIndex to avoid repeated blockList.indexOf() scans is good, testing revealed a correctness issue.

Running structural operations (example: deleting random blocks via sendStackToTrash) caused a runtime exception because the cached blockIndex was used in places that previously relied on a fresh lookup. The original indexOf guaranteed a current valid position in blockList, but the cached value can refer to a block in a context where that assumption no longer holds.

This means the change alters behavioural guarantees rather than only improving performance, and certain operations can crash even though the index invariant itself appears intact.

The optimisation approach is valid, but the implementation needs additional safeguards or revalidation of indices before being safely merged.
Thanks!

[github-actions]

✅ All Jest tests passed! This PR is ready to merge.

[Ashutoshx7]

While the idea of caching blockIndex to avoid repeated blockList.indexOf() scans is good, testing revealed a correctness issue.

Running structural operations (example: deleting random blocks via sendStackToTrash) caused a runtime exception because the cached blockIndex was used in places that previously relied on a fresh lookup. The original indexOf guaranteed a current valid position in blockList, but the cached value can refer to a block in a context where that assumption no longer holds.

This means the change alters behavioural guarantees rather than only improving performance, and certain operations can crash even though the index invariant itself appears intact.

The optimisation approach is valid, but the implementation needs additional safeguards or revalidation of indices before being safely merged. Thanks!

thanx for the review
i did the changes and verified locally
lmk your thoughts
and also there were merge conflicts ( resolved)
@walterbender lmk your thoughts on this

[kartikktripathi]

The caching idea is good for performance, but replacing indexOf with property access removes a crucial safety check. If a block is removed or the project clears, structural operations (like sendStackToTrash) crash because they use stale indices. Please add a getter to revalidate the cached index before it's used (blockList[cachedIndex] === block).

[Ashutoshx7]

The caching idea is good for performance, but replacing indexOf with property access removes a crucial safety check. If a block is removed or the project clears, structural operations (like sendStackToTrash) crash because they use stale indices. Please add a getter to revalidate the cached index before it's used (blockList[cachedIndex] === block).

blockIndex is assigned at creation time and blocks are appended to blockList. There is no reordering of blockList, and deletion paths do not shift indices in a way that leaves live blocks in inconsistent positions.

refreence: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice

[kartikktripathi]

I think the confusion might be around this part that even if we don’t explicitly reorder the array, Array.splice() itself shifts the indices of elements after the removed item. So if blockList is modified via splice during deletion operations, cached indices after that point can become stale unless they’re updated or revalidated. That’s the case I was worried about.