fix(test-adapter): use destructuring for createScope return value

[taras]

Motivation

The createScope() function returns a tuple [Scope, () => Future<void>], but accessing it via index notation (createScope()[0]) was causing runtime issues when used in the inspector project. This was discovered via a pnpm patch applied in thefrontside/inspector.

Approach

Changed from index access to destructuring assignment:

// Beforeletparent=adapter.parent
? yield*adapter.parent["@@init@@"]()
: Ok(createScope()[0]);// After letparent: Result<Scope>;if(adapter.parent){parent=yield*adapter.parent["@@init@@"]();}else{let[rootScope]=createScope();parent=Ok(rootScope);}

This properly extracts the first element of the tuple while maintaining type safety. The expanded if/else structure also improves readability.

Summary by CodeRabbit

• Refactor
  • Improved adapter initialization and teardown internals for more robust lifecycle handling; no changes to public behavior or APIs.
• Chores
  • Bumped test-adapter package version from 0.7.2 to 0.7.3.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

When no parent adapter exists, initialization now creates a root scope via createScope() and wraps it in Ok(...) to produce a Result<Scope>; the root's destroy function is captured and invoked on teardown. Also bumps package version and reorders an import.

Changes

Cohort / File(s)	Summary
Test Adapter Init test-adapter/mod.ts	Initialize no-parent case by creating a root scope with createScope() wrapped in Ok(...); capture root destroy function and extend destroy path to call task.halt() and the captured root destroy. Adjusted import ordering.
Package Manifest test-adapter/package.json	Bumped package version from 0.7.2 to 0.7.3.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Policy Compliance	❌ Error	The PR violates the Strict Package.json Metadata Policy due to non-approved keywords 'effection' and 'effectionx' in test-adapter/package.json.	Replace keywords with only approved values like ['testing'] or ['testing', 'interop'] before merging.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title correctly identifies the main change: switching from index access to destructuring when accessing the createScope() return value.
Description check	✅ Passed	The description includes both required sections: Motivation explains the runtime issue discovered via the patch, and Approach details the exact code changes from index access to destructuring with an expanded if/else structure.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/test-adapter-createscope-destructure

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@effectionx/test-adapter@176

commit: f4d0528

[cowboyd]

The main problem I have with this is that it leaks the root test adapter scope, because the destroy() is never captured and never called.

I recognize that this is a problem existing, but it would be really nice to fix it.

[taras]

Any ideas on how to solve it?

[jbolda]

Would ensure() be appropriate here? The lifecycle isn't obvious just reading the code.

[cowboyd]

We need to run destroy when the test adapter is destroyed. In reality it is a small memory leak because it only applies to the top-level test adapter, and we're halting all the child tasks created from it, but it is a loophole through which bigger leaks can get through if we do more stuff with the scope in the future:

We just need to make sure that if there is no parent adapter, then we destroy this scope as well. Something like this.

*"@@init@@"(){if(scope){returnyield*scope.operation;}scope=withResolvers<Result<Scope>>();letparent: Result<Scope>;letdestroyRoot: (()=>Future<void>)|undefined=undefined;if(adapter.parent){parent=yield*adapter.parent["@@init@@"]();}else{let[rootScope,destroy]=createScope();rootDestroy=destroy;parent=Ok(rootScope);}if(!parent.ok){scope.resolve(parent);returnyield*scope.operation;}lettask=yield*parent.value.spawn(function*(){letinit=yield*box(function*(){for(letinitializerofadapter.setup.all){yield*initializer();}});if(!init.ok){scope?.resolve(init);}else{scope?.resolve(Ok(yield*useScope()));yield*suspend();}});destroy=function*destroy(){if(destroyRoot){yield*destroyRoot();}yield*task.halt();}returnyield*scope.operation;},