Skip to content

Conversation

@FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Jan 30, 2026

This PR refactors BaseSnapExecutor to follow our more recent patterns, notably using hash private for most properties and functions, but also changing a couple of naming patterns and improving types. This makes BaseSnapExecutor way more compliant with our linter.

Split off #3803 to make this easier to review.


Note

Medium Risk
Touches core snap execution/termination and teardown tracking; while mostly a refactor, it changes how termination promises and teardown state are managed and could affect edge-case lifecycle behavior.

Overview
Refactors BaseSnapExecutor to use hash-private fields/methods throughout (removing prior lint suppressions) and tightens command dispatch typing by calling #methods[method as keyof Methods] instead of any.

Updates execution/termination plumbing: snap evaluation cancellation now uses createDeferredPromise and a shared #teardownRef object is passed into withTeardown instead of this, changing the termination error string to The Snap "..." has been terminated during execution. (and updating the corresponding browser test expectation).

Written by Cursor Bugbot for commit 0082a8c. This will update automatically on new commits. Configure here.

try {
const result = await (this.methods as any)[method](...paramsAsArray);
const result = await this.#methods[method as keyof Methods](
...(paramsAsArray as any),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typing is still not great. The ideal case is to make a breaking change to always pass parameters as objects and use structs to validate them better

@FrederikBolding FrederikBolding marked this pull request as ready for review January 30, 2026 14:02
@FrederikBolding FrederikBolding requested a review from a team as a code owner January 30, 2026 14:02
@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

❌ Patch coverage is 97.26027% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.39%. Comparing base (5ab30ee) to head (0082a8c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...cution-environments/src/common/BaseSnapExecutor.ts 97.26% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3830      +/-   ##
==========================================
- Coverage   98.40%   98.39%   -0.01%     
==========================================
  Files         430      430              
  Lines       12448    12457       +9     
  Branches     1933     1932       -1     
==========================================
+ Hits        12249    12257       +8     
- Misses        199      200       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants