Optimizing performance for traversing stacks#3343
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplace front-removing queue traversal ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
RobinTail
changed the title
|
coverage: 100.0%. remained the same — boost-stacks into master |
…ll O(n) writing nested.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed — no issues found.
The shift() → index-based transformation is correct across all five call sites. Traversal order is preserved in every case, including routing-walker.ts where unshift() → splice(idx, 0, ...) maintains depth-first semantics. The splice there still has linear cost per invocation, but it only runs when entering nested routing (not on every iteration like the old shift() did), so it's a net win.
Task list (4/4 completed)
- Read the full diff and understand the changes
- Read relevant source files for context
- Analyze correctness, performance claims, and edge cases
- Submit review
Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@express-zod-api/bench/queue-ops.bench.ts`:
- Around line 28-32: The "index approach" benchmark is unfair because it never
reads elements; update the bench("index approach", ...) loop to dereference the
array element (e.g., access q[idx]) inside the while loop so it realistically
models traversal costs like in hasCycle, fixReferences, and walkRouting;
additionally, for parity capture the element returned by shift()/pop() in their
benches (store the result to a local variable) and move any reverse() call
outside the timed closure for the pop case so the timed section measures only
consumption cost.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac2a3fff-425f-4728-bc14-1fb992b89d35
📒 Files selected for processing (7)
express-zod-api/bench/can-merge.bench.tsexpress-zod-api/bench/queue-ops.bench.tsexpress-zod-api/src/deep-checks.tsexpress-zod-api/src/diagnostics.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/routing-walker.ts
💤 Files with no reviewable changes (1)
- express-zod-api/bench/can-merge.bench.ts
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Extended the
shift()/unshift()→ index-based traversal optimization toonTemplateLiteralinzts.ts, whereshiftText()and the outer span-building loop now share anidxcursor over the spread-copiedpartsarray. The shared-state interplay between the two loops is correct —shiftText()always leavesidxat the next schema element or at the end, and theidx < parts.lengthguard for choosingcreateTemplateMiddlevscreateTemplateTailremains equivalent to the originalparts.lengthcheck. - Removed a redundant inline comment (
// Handle potential undefined values) — the?? ""is self-explanatory.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/src/zts.ts (1)
43-70: Refactor looks correct; a couple of small cleanups possible.Traced through the new index-based traversal for the expected alternating
text, schema, text, …shape (and the degenerate single-text / back-to-back-schema cases) —head, span middles/tails, and the empty-spans fallback all match the previousshift-based behavior. Two optional tidy-ups:
const parts = [...def.parts]is no longer needed; with index iteration nothing is mutated, sodef.partscan be used directly (or destructured) to drop the array clone.shiftTextno longer shifts anything — a name likeconsumeText/readTextwould better match the new mechanics.♻️ Suggested cleanup
- const parts = [...def.parts]; + const { parts } = def; let idx = 0; - const shiftText = () => { + const readText = () => { let text = ""; while (idx < parts.length) { const part = parts[idx++]; if (isSchema(part)) { idx--; break; } text += part ?? ""; } return text; }; - const head = api.f.createTemplateHead(shiftText()); + const head = api.f.createTemplateHead(readText()); const spans: ts.TemplateLiteralTypeSpan[] = []; while (idx < parts.length) { const schema = next(parts[idx++] as z.core.$ZodType); - const text = shiftText(); + const text = readText(); const textWrapper = idx < parts.length ? api.f.createTemplateMiddle : api.f.createTemplateTail; spans.push(api.f.createTemplateLiteralTypeSpan(schema, textWrapper(text))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/zts.ts` around lines 43 - 70, The clone const parts = [...def.parts] is unnecessary since we no longer mutate the array; change usages to read directly from def.parts (or destructure into a local const parts = def.parts if you prefer) and remove the array copy, and rename the helper shiftText to a clearer name like consumeText or readText to reflect that it does not shift/mutate; update the references where shiftText() is called (used to build head via api.f.createTemplateHead and when computing textWrapper/text) accordingly so parts/idx logic remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@express-zod-api/src/zts.ts`:
- Around line 43-70: The clone const parts = [...def.parts] is unnecessary since
we no longer mutate the array; change usages to read directly from def.parts (or
destructure into a local const parts = def.parts if you prefer) and remove the
array copy, and rename the helper shiftText to a clearer name like consumeText
or readText to reflect that it does not shift/mutate; update the references
where shiftText() is called (used to build head via api.f.createTemplateHead and
when computing textWrapper/text) accordingly so parts/idx logic remains the
same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcf123be-9c1a-4cb5-8cc7-79d4f941d734
📒 Files selected for processing (1)
express-zod-api/src/zts.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/src/zts.ts (1)
43-43: Optional: drop the now-redundant array copy.Since
partsis no longer mutated (noshift/unshift), the defensive spread copy is unnecessary and each call allocates a fresh array of lengthdef.parts.length. Given this PR's perf focus, you can just alias the original.♻️ Proposed tweak
- const parts = [...def.parts]; + const { parts } = def; let idx = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/zts.ts` at line 43, The local variable parts is making an unnecessary defensive copy of def.parts; replace the spread copy by aliasing the original array (use parts = def.parts) in the function that declares "const parts = [...def.parts]" so that you avoid extra allocations — update the usage sites in zts.ts to rely on the aliased parts variable instead of the copied array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@express-zod-api/src/zts.ts`:
- Line 43: The local variable parts is making an unnecessary defensive copy of
def.parts; replace the spread copy by aliasing the original array (use parts =
def.parts) in the function that declares "const parts = [...def.parts]" so that
you avoid extra allocations — update the usage sites in zts.ts to rely on the
aliased parts variable instead of the copied array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 962d4886-5e69-47da-9bcd-7a19940f4ee6
📒 Files selected for processing (1)
express-zod-api/src/zts.ts
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Removed the redundant
[...def.parts]spread copy inonTemplateLiteral— the index-based traversal doesn't mutate the array, so destructuringdefdirectly is correct and avoids an allocation per call. - Renamed
shiftText→readTextto reflect the non-mutating index cursor semantics. - Fixed the "index approach" benchmark to actually dereference the array element (
q[idx++]) instead of only incrementing the counter, making the comparison withshift()/pop()/forEachfair.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/src/zts.ts (1)
45-56: Optional: peek instead of advance-then-rewind inreadText.The
idx++followed byidx--when a schema is encountered is a slightly awkward advance-and-undo pattern. Peeking withparts[idx]before committing the increment reads more clearly and aligns with the straightidx++pattern established indeep-checks.ts(context snippet 2). Behavior is identical.♻️ Proposed refactor
- const readText = () => { - let text = ""; - while (idx < parts.length) { - const part = parts[idx++]; - if (isSchema(part)) { - idx--; - break; - } - text += part ?? ""; // Handle potential undefined values - } - return text; - }; + const readText = () => { + let text = ""; + while (idx < parts.length && !isSchema(parts[idx])) { + text += parts[idx++] ?? ""; // Handle potential undefined values + } + return text; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/zts.ts` around lines 45 - 56, The readText helper uses an advance-then-rewind pattern (idx++ then idx-- when isSchema(part) is true); change it to peek before advancing: check parts[idx] and call isSchema(parts[idx]) first, break without modifying idx if it's a schema, otherwise consume parts[idx] and increment idx after adding it to text. Update references to parts, idx, and isSchema in readText so the logic mirrors the straight idx++ consumption used elsewhere (e.g., deep-checks.ts) but avoids the unnecessary decrement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@express-zod-api/src/zts.ts`:
- Around line 45-56: The readText helper uses an advance-then-rewind pattern
(idx++ then idx-- when isSchema(part) is true); change it to peek before
advancing: check parts[idx] and call isSchema(parts[idx]) first, break without
modifying idx if it's a schema, otherwise consume parts[idx] and increment idx
after adding it to text. Update references to parts, idx, and isSchema in
readText so the logic mirrors the straight idx++ consumption used elsewhere
(e.g., deep-checks.ts) but avoids the unnecessary decrement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab094b4f-9516-4140-ac4d-e3789888f63b
📒 Files selected for processing (2)
express-zod-api/bench/queue-ops.bench.tsexpress-zod-api/src/zts.ts
✅ Files skipped from review due to trivial changes (1)
- express-zod-api/bench/queue-ops.bench.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/src/routing-walker.ts (1)
94-116: LGTM — DFS visit order preserved viasplice(idx + 1, 0, …).Using
splice(idx + 1, 0, ...children)instead of a simplepushis the correct choice here: it maintains the depth-first visit order that the previousshift()+unshift(...children)produced, which (percollectSiblingsinrouting.ts) drives the insertion order into thefamiliarMap. Swapping this forpushwould flip to BFS and change endpoint registration order — worth a brief code comment if you want to harden against future refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/routing-walker.ts` around lines 94 - 116, The DFS order is intentionally preserved by inserting children with splice(idx + 1, 0, ...processEntries(...)) instead of push; please add a brief explanatory comment next to the stack.splice(...) call (inside the for loop in routing-walker.ts) stating that using splice preserves DFS and the previous shift/unshift semantics (which affect collectSiblings and the insertion order into the familiar Map), and warn that switching to push would change traversal to BFS and alter endpoint registration order (mentioning AbstractEndpoint and ServeStatic context to make the intent clear).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@express-zod-api/src/routing-walker.ts`:
- Around line 94-116: The DFS order is intentionally preserved by inserting
children with splice(idx + 1, 0, ...processEntries(...)) instead of push; please
add a brief explanatory comment next to the stack.splice(...) call (inside the
for loop in routing-walker.ts) stating that using splice preserves DFS and the
previous shift/unshift semantics (which affect collectSiblings and the insertion
order into the familiar Map), and warn that switching to push would change
traversal to BFS and alter endpoint registration order (mentioning
AbstractEndpoint and ServeStatic context to make the intent clear).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c57a0070-aa8f-4cd3-b1e9-cf9b8089f5d8
📒 Files selected for processing (7)
express-zod-api/bench/queue-ops.bench.tsexpress-zod-api/src/deep-checks.tsexpress-zod-api/src/diagnostics.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/routing-walker.tsexpress-zod-api/src/zts.ts
✅ Files skipped from review due to trivial changes (3)
- express-zod-api/src/deep-checks.ts
- express-zod-api/src/diagnostics.ts
- express-zod-api/bench/queue-ops.bench.ts
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Added a
no-restricted-syntaxlint rule banningshift(),unshift(), andsplice()calls inperformanceConcerns, enforcing index-based iteration as the default going forward — call sites that legitimately need these operations (integration.ts,routing-walker.ts,json-schema-helpers.ts) carryeslint-disablecomments with justifications. - Refactored handler construction in
initRoutingto build thehandlersarray sequentially viapush()— CORS first (conditionally), then parsers and endpoint handler — eliminating the priorslice().concat()+unshift()pattern and removing the defensive copy ofmatchingParsers. - Simplified
readTextinonTemplateLiteralto peek atparts[idx]before advancing, removing the advance-then-rewind (idx++/idx--) pattern from the previous iteration.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
🧹 Nitpick comments (3)
eslint.config.js (1)
75-78: Rule LGTM; minor note onsplicewording.The selector correctly targets the three mutators and the
warnlevel plus per-line escape hatch (already used inrouting-walker.ts) gives an acceptable ergonomic path. One nit: the message "shifting is 2-20x slower than index-based iteration" is accurate forshift/unshift, butspliceis a broader API — the in-place insertion use inrouting-walker.tsisn't really "shifting". If you want the rule to read precisely where it fires, consider either splittingspliceinto its own entry with a tailored message (e.g., "prefer index-based iteration over array mutation") or rephrasing the shared message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 75 - 78, The rule message lumps shift/unshift and splice together but only shift/unshift are accurately described as "shifting is 2-20x slower than index-based iteration"; update the ESLint config by either (A) splitting the selector "CallExpression[callee.property.name=/^(shift|unshift|splice)$/]" into two entries—one targeting shift|unshift with the existing message and a separate selector for splice with a tailored message like "prefer index-based iteration or non-mutating methods over array mutation"—or (B) rephrase the shared message to cover all three (e.g., "avoid array mutation; prefer index-based iteration or non-mutating methods") so the message accurately reflects why the rule fires; reference the existing selector string and message field when making the change.express-zod-api/src/routing.ts (1)
92-109: Handler ordering preserved — LGTM.The new push-based assembly yields
[cors?, ...parsers, exec], matching the previousunshift-based result and preserving the issue#2706invariant that CORS runs before parsers. Closures correctly captureaccessMethods,endpoint, andmatchingParsersper-method iteration.Optional nit: the trailing colon in the inline comment on line 92 (
// issue#2706: CORS must go before parsers:) reads like a dangling sentence since the relatedif (config.cors)is on the next line. Consider promoting it to a standalone comment directly above theifblock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/routing.ts` around lines 92 - 109, Move the inline comment about issue `#2706` so it's a standalone comment immediately above the if (config.cors) block and remove the trailing colon to read as a complete sentence; this keeps the note visible and avoids the dangling-colon style while preserving behavior of handlers, config.cors, matchingParsers, and the final app[method](path, ...handlers) assembly.express-zod-api/src/routing-walker.ts (1)
94-95: Traversal semantics preserved — LGTM.Verified DFS equivalence:
- Old:
shift()+unshift(children)⇒ next iteration processes first child.- New:
stack[idx]+splice(idx + 1, 0, ...children)⇒idx++lands on the first child.So
onEndpointinvocation order, duplicate detection viavisited, and downstreamfamiliarMap population incollectSiblingsare unchanged. Performance win comes from O(1) index access on leaf iterations;spliceis still O(n) but only pays that cost on container expansion.Two small notes:
- The stack is never pruned, so peak memory is now proportional to the total number of routing entries rather than to DFS depth. Practically irrelevant for typical routings, but worth being aware of.
- The eslint-disable reason on line 113 ("acceptable due to conditional logic") is a bit opaque — something like
"splice used for in-place insertion, not front-shifting"would make the intent self-evident next time someone grep's the suppressions.Also applies to: 113-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/routing-walker.ts` around lines 94 - 95, Update the opaque eslint-disable comment near the splice usage to a clear rationale like "splice used for in-place insertion to preserve DFS traversal order; allowed instead of shifting the front" so future greps show intent (referencing stack and splice in routing-walker.ts); additionally, to address the note about peak memory, prune already-processed entries after advancing (e.g., trim or compact stack after handling a container) to keep peak size closer to DFS depth while preserving onEndpoint invocation order, visited duplicate detection, and collectSiblings/familiar Map semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eslint.config.js`:
- Around line 75-78: The rule message lumps shift/unshift and splice together
but only shift/unshift are accurately described as "shifting is 2-20x slower
than index-based iteration"; update the ESLint config by either (A) splitting
the selector "CallExpression[callee.property.name=/^(shift|unshift|splice)$/]"
into two entries—one targeting shift|unshift with the existing message and a
separate selector for splice with a tailored message like "prefer index-based
iteration or non-mutating methods over array mutation"—or (B) rephrase the
shared message to cover all three (e.g., "avoid array mutation; prefer
index-based iteration or non-mutating methods") so the message accurately
reflects why the rule fires; reference the existing selector string and message
field when making the change.
In `@express-zod-api/src/routing-walker.ts`:
- Around line 94-95: Update the opaque eslint-disable comment near the splice
usage to a clear rationale like "splice used for in-place insertion to preserve
DFS traversal order; allowed instead of shifting the front" so future greps show
intent (referencing stack and splice in routing-walker.ts); additionally, to
address the note about peak memory, prune already-processed entries after
advancing (e.g., trim or compact stack after handling a container) to keep peak
size closer to DFS depth while preserving onEndpoint invocation order, visited
duplicate detection, and collectSiblings/familiar Map semantics.
In `@express-zod-api/src/routing.ts`:
- Around line 92-109: Move the inline comment about issue `#2706` so it's a
standalone comment immediately above the if (config.cors) block and remove the
trailing colon to read as a complete sentence; this keeps the note visible and
avoids the dangling-colon style while preserving behavior of handlers,
config.cors, matchingParsers, and the final app[method](path, ...handlers)
assembly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c032c088-b21b-4556-a018-b5a5540819a4
📒 Files selected for processing (4)
eslint.config.jsexpress-zod-api/src/integration.tsexpress-zod-api/src/routing-walker.tsexpress-zod-api/src/routing.ts
✅ Files skipped from review due to trivial changes (1)
- express-zod-api/src/integration.ts
There was a problem hiding this comment.
Important
Minor convention inconsistency in the new test file.
- Narrowed the
no-restricted-syntaxlint rule to only banshift/unshift, droppingsplice— the "shifting" message no longer applies inaccurately tosplicecall sites. - Removed the now-unnecessary
eslint-disablecomment fromrouting-walker.ts. - Added
routing-walker.spec.tswith two tests asserting DFS traversal order ofwalkRouting(), protecting thesplice(idx + 1, 0, ...)invariant against accidental regression to BFS.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
🧹 Nitpick comments (1)
express-zod-api/tests/routing-walker.spec.ts (1)
5-48: Good targeted coverage for the new index-based traversal.The two cases effectively pin the depth-first / nested-before-siblings invariant that the
stack.splice(idx + 1, 0, ...)insertion inrouting-walker.tsrelies on, which protects the refactor against accidental regressions (e.g., switching back to append-at-end).One optional nit: the two tests share identical setup (
onEndpoint = vi.fn(), same config) — they could be consolidated viatest.each()(routing + expected calls) per the repo's parameterized-test guideline, but it's fine as-is for just two cases.As per coding guidelines: "Test files should use
test.each()for parameterized tests with placeholders%sfor current value and%#for index".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/tests/routing-walker.spec.ts` around lines 5 - 48, Replace the two nearly-identical tests with a single parameterized test using test.each to follow the repository guideline: create an array of cases pairing a short name, the routing object, and the expected onEndpoint.mock.calls, then call test.each(cases)("%s %#", (name, routing, expected) => { const onEndpoint = vi.fn(); walkRouting({ routing, config: { cors: false, methodLikeRouteBehavior: "path" }, onEndpoint }); expect(onEndpoint.mock.calls).toEqual(expected); }); Keep references to walkRouting, onEndpoint, and the config object so the same assertions (e.g., ["get","/v1/user/retrieve", endpoint]) are used as expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@express-zod-api/tests/routing-walker.spec.ts`:
- Around line 5-48: Replace the two nearly-identical tests with a single
parameterized test using test.each to follow the repository guideline: create an
array of cases pairing a short name, the routing object, and the expected
onEndpoint.mock.calls, then call test.each(cases)("%s %#", (name, routing,
expected) => { const onEndpoint = vi.fn(); walkRouting({ routing, config: {
cors: false, methodLikeRouteBehavior: "path" }, onEndpoint });
expect(onEndpoint.mock.calls).toEqual(expected); }); Keep references to
walkRouting, onEndpoint, and the config object so the same assertions (e.g.,
["get","/v1/user/retrieve", endpoint]) are used as expected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a07b74a5-18d9-401b-83c4-ccb9f88989b1
📒 Files selected for processing (3)
eslint.config.jsexpress-zod-api/src/routing-walker.tsexpress-zod-api/tests/routing-walker.spec.ts
✅ Files skipped from review due to trivial changes (1)
- eslint.config.js
🚧 Files skipped from review as they are similar to previous changes (1)
- express-zod-api/src/routing-walker.ts
|
No action needed — the PR author already fixed the vitest import in Task list (3/3 completed)
|
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Removed the
vitestimport fromrouting-walker.spec.ts, relying on vitest globals per project convention. - Dropped the redundant
methodLikeRouteBehavior: "path"config option from both tests (defaults to"method", irrelevant for these fixtures) and renamed thepostroute fixture torecordto avoid ambiguity with the HTTP method name.
Prior review feedback:
- Remove vitest import from
routing-walker.spec.ts— addressed
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Consolidated the two separate DFS-order tests in
routing-walker.spec.tsinto a singletest.each<Routing>parameterized test withtoMatchSnapshot()assertions, following the repo's testing convention. The hoistedonEndpointmock withafterEachcleanup correctly handles shared state across iterations.
Claude Opus ｜ 𝕏
1 participant
using index-based approach instead of
Array::shiftwhich has O(n) complexity.That gives 2-20x boost in tests for (5-100 items)
Summary by CodeRabbit
Tests
Refactor
Style