Address PR review feedback

- Simplify iteration tracker to lazy evaluation (only compute diagnostics
  when limit is actually exceeded, not on every iteration)
- Revert unnecessary array conversions in collection-subscriber.ts
- Revert unused boolean return from loadNextItems
- Keep cursorMinValue rename (fixes ESLint shadowing warning)
- Confirmed insertedKeys was dead code (never used after being set)

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: KyleAMathews

packages/db-ivm/src/d2.ts

@@ -1,5 +1,5 @@
 import { DifferenceStreamWriter } from './graph.js'
-import { createIterationTracker } from './iteration-tracker.js'
+import { createIterationLimitChecker } from './iteration-tracker.js'
 import type {
   BinaryOperator,
   DifferenceStreamReader,
@@ -61,24 +61,24 @@ export class D2 implements ID2 {
     // Safety limit to prevent infinite loops in case of circular data flow
     // or other bugs that cause operators to perpetually produce output.
     const MAX_RUN_ITERATIONS = 100000
-    const tracker = createIterationTracker<string>(
-      MAX_RUN_ITERATIONS,
-      (state) => `operators with work = [${state}]`,
-    )
+    const checkLimit = createIterationLimitChecker(MAX_RUN_ITERATIONS)
 
     while (this.pendingWork()) {
-      const operatorsWithWork = this.#operators
-        .filter((op) => op.hasPendingWork())
-        .map((op) => op.constructor.name)
-        .sort()
-        .join(`,`)
-
-      if (tracker.trackAndCheckLimit(operatorsWithWork)) {
-        console.warn(
-          tracker.formatWarning(`D2 graph execution`, {
-            totalOperators: this.#operators.length,
-          }),
-        )
+      if (
+        checkLimit(() => {
+          // Only compute diagnostics when limit is exceeded (lazy)
+          const operatorsWithWork = this.#operators
+            .filter((op) => op.hasPendingWork())
+            .map((op) => ({ id: op.id, type: op.constructor.name }))
+          return {
+            context: `D2 graph execution`,
+            diagnostics: {
+              operatorsWithPendingWork: operatorsWithWork,
+              totalOperators: this.#operators.length,
+            },
+          }
+        })
+      ) {
         break
       }
 

packages/db-ivm/src/iteration-tracker.ts

@@ -1,141 +1,61 @@
 /**
- * Tracks state transitions during iteration loops for diagnostic purposes.
- * Used by circuit breakers to report where iterations were spent when limits are exceeded.
+ * Creates a simple iteration counter with a limit check.
+ * When the limit is exceeded, calls the provided diagnostic function to capture state.
  *
- * The tracker collects a history of state transitions, where each entry records
- * a period of time (iteration range) spent in a particular state. When the iteration
- * limit is exceeded, this history helps diagnose infinite loop causes.
+ * This design avoids per-iteration overhead - state capture only happens when needed.
  *
  * @example
  * ```ts
- * const tracker = createIterationTracker<{ operators: string }>(100000)
+ * const checkLimit = createIterationLimitChecker(100000)
  *
  * while (pendingWork()) {
- *   const state = { operators: getOperatorsWithWork().join(',') }
- *   if (tracker.trackAndCheckLimit(state)) {
- *     console.warn(tracker.formatWarning('D2 graph execution', {
- *       totalOperators: operators.length,
- *     }))
+ *   if (checkLimit(() => ({
+ *     context: 'D2 graph execution',
+ *     diagnostics: { totalOperators: operators.length }
+ *   }))) {
  *     break
  *   }
  *   step()
  * }
  * ```
  */
 
-export type StateHistoryEntry<TState> = {
-  state: TState
-  startIter: number
-  endIter: number
-}
-
-export type IterationTracker<TState> = {
-  /**
-   * Records the current state and increments the iteration counter.
-   * Returns true if the iteration limit has been exceeded.
-   */
-  trackAndCheckLimit: (state: TState) => boolean
-
-  /**
-   * Formats a warning message with iteration breakdown and diagnostic info.
-   * Call this after trackAndCheckLimit returns true.
-   */
-  formatWarning: (
-    context: string,
-    diagnosticInfo?: Record<string, unknown>,
-  ) => string
-
-  /**
-   * Returns the current iteration count.
-   */
-  getIterations: () => number
-
-  /**
-   * Returns the state history for inspection.
-   */
-  getHistory: () => Array<StateHistoryEntry<TState>>
+export type LimitExceededInfo = {
+  context: string
+  diagnostics?: Record<string, unknown>
 }
 
 /**
- * Creates an iteration tracker that monitors loop iterations and records state transitions.
+ * Creates an iteration limit checker that logs a warning when the limit is exceeded.
  *
  * @param maxIterations - The maximum number of iterations before the limit is exceeded
- * @param stateToKey - Optional function to convert state to a string key for comparison.
- *                     Defaults to JSON.stringify.
+ * @returns A function that increments the counter and returns true if limit exceeded
  */
-export function createIterationTracker<TState>(
+export function createIterationLimitChecker(
   maxIterations: number,
-  stateToKey: (state: TState) => string = (state) => JSON.stringify(state),
-): IterationTracker<TState> {
-  const history: Array<StateHistoryEntry<TState>> = []
-  let currentStateKey: string | null = null
-  let currentState: TState | null = null
-  let stateStartIter = 1
+): (getInfo: () => LimitExceededInfo) => boolean {
   let iterations = 0
 
-  function recordCurrentState(): void {
-    if (currentStateKey !== null && currentState !== null) {
-      history.push({
-        state: currentState,
-        startIter: stateStartIter,
-        endIter: iterations,
-      })
-    }
-  }
-
-  function trackAndCheckLimit(state: TState): boolean {
-    const stateKey = stateToKey(state)
-
-    if (stateKey !== currentStateKey) {
-      recordCurrentState()
-      currentStateKey = stateKey
-      currentState = state
-      stateStartIter = iterations + 1
-    }
-
+  return function checkLimit(getInfo: () => LimitExceededInfo): boolean {
     iterations++
 
     if (iterations > maxIterations) {
-      recordCurrentState()
+      // Only capture diagnostic info when we actually exceed the limit
+      const { context, diagnostics } = getInfo()
+
+      const diagnosticSection = diagnostics
+        ? `\nDiagnostic info: ${JSON.stringify(diagnostics, null, 2)}\n`
+        : `\n`
+
+      console.warn(
+        `[TanStack DB] ${context} exceeded ${maxIterations} iterations. ` +
+          `Continuing with available data.` +
+          diagnosticSection +
+          `Please report this issue at https://github.com/TanStack/db/issues`,
+      )
       return true
     }
 
     return false
   }
-
-  function formatWarning(
-    context: string,
-    diagnosticInfo?: Record<string, unknown>,
-  ): string {
-    const iterationBreakdown = history
-      .map((h) => `    ${h.startIter}-${h.endIter}: ${stateToKey(h.state)}`)
-      .join(`\n`)
-
-    const diagnosticSection = diagnosticInfo
-      ? `\nDiagnostic info: ${JSON.stringify(diagnosticInfo, null, 2)}\n`
-      : `\n`
-
-    return (
-      `[TanStack DB] ${context} exceeded ${maxIterations} iterations. ` +
-      `Continuing with available data.\n` +
-      `Iteration breakdown (where the loop spent time):\n${iterationBreakdown}` +
-      diagnosticSection +
-      `Please report this issue at https://github.com/TanStack/db/issues`
-    )
-  }
-
-  function getIterations(): number {
-    return iterations
-  }
-
-  function getHistory(): Array<StateHistoryEntry<TState>> {
-    return [...history]
-  }
-
-  return {
-    trackAndCheckLimit,
-    formatWarning,
-    getIterations,
-    getHistory,
-  }
 }