Conversation
* modernize * refactor: migrate to biome and standardize code style across project * chore: update biome lint command and add fix script alias * style: standardize quotes to single quotes and fix formatting * refactor: replace forEach loops with for...of and remove unnecessary async keywords * refactor: replace forEach loops with for...of and remove unnecessary async declarations * ci: update workflow commands and disable coverage in ci-ts pipeline * temporarily disable docker
* activate testing in ci * ci: remove frozen lockfile in CI workflow
) * feat: implement container pooling system for device emulation * fix: address CodeRabbit review comments - Fix YAML linting issues in GitHub workflow files - Remove trailing spaces in ci-ts.yaml - Add missing newlines at end of YAML files - Fix typo: settingsNavnavLeftButton → settingsNavLeftButton - Organize imports in containerPool.ts (external packages first) - Remove unused model variable in cleanup() method - Apply automated code formatting improvements * fix: address additional CodeRabbit review comments - Fix missing parentheses in tests/minapp/index.ts toString() call - Add error handling to gRPC exchange promise chain - Remove async modifier from globalsetup.ts (no await needed) - Add race condition protection for pool initialization - Document DEV_CERT_PRIVATE_KEY as development-only test key * fix: resolve remaining CI issues and CodeRabbit comments - Fix trailing spaces in .github/dependabot.yml - Add missing await to async operations in tests/pullImageKillOld.ts - Wrap async calls in IIFE to prevent CI hanging on cleanup This should resolve the hanging CI build and address the remaining CodeRabbit linting issues. * fix: handle transport errors to fail fast instead of timing out - Add error tracking in transport layer via proxy interceptor - Create errors.ts with APDU status codes and error handling utilities - Propagate critical transport errors in waitUntilScreenIs, waitUntilScreenIsNot, and waitForText - Update getEvents to check for critical transport errors before making requests - Add comprehensive tests to verify fast-fail behavior Previously, when errors like 0x6984 (INVALID_DATA) or 0x6E00 (CLA_NOT_SUPPORTED) occurred, Zemu would continue waiting for timeouts instead of failing immediately. Now these errors are detected and cause immediate failure with clear error messages. Fixes issue where transport errors were absorbed causing unnecessary timeout delays. * fix: update error handling test to expect correct error code The test was expecting 0x6984 (INVALID_DATA) but invalid CLA (0xFF) actually triggers 0x6E00 (CLA_NOT_SUPPORTED) which is the correct behavior. * fix: resolve linter issues - Remove unused imports in test files - Fix import organization in Zemu.ts - Fix formatting (newlines at end of files) - Apply Biome linter rules All tests passing after linter fixes.
Changed default pool configuration from 1 to 2 containers per device type (except nanos which was already 2). This provides better parallelism for test suites that run multiple tests concurrently.
WalkthroughThis update introduces significant changes to project configuration, code style, testing, and container management. The project migrates from Yarn/Jest/ESLint/Prettier to pnpm/Vitest/Biome, adds container pooling for emulator management, improves error handling, and updates GitHub Actions workflows. Several configuration and dotfiles are added, removed, or replaced. New tests and documentation are included, and all code adopts a unified style. Changes
Sequence Diagram(s)Emulator Start with Container PoolingsequenceDiagram
participant TestSuite
participant Zemu
participant ContainerPool
participant EmuContainer
TestSuite->>Zemu: start(options)
alt Pooling enabled and not disabled in options
Zemu->>ContainerPool: initializePool(config)
Zemu->>ContainerPool: acquire(model, elfPath, libElfs)
alt Container acquired
ContainerPool-->>Zemu: pooledContainer
Zemu->>Zemu: finalizeStart()
else Pool unavailable or acquire failed
Zemu->>Zemu: startWithNewContainer()
end
else Pooling disabled
Zemu->>Zemu: startWithNewContainer()
end
Zemu-->>TestSuite: Emulator ready
Emulator Close with Container PoolingsequenceDiagram
participant TestSuite
participant Zemu
participant ContainerPool
participant EmuContainer
TestSuite->>Zemu: close()
alt Using pooled container
Zemu->>ContainerPool: release(pooledContainer)
alt Release succeeds
ContainerPool-->>Zemu: released
else Release fails
Zemu->>EmuContainer: stop()
end
else Not using pool
Zemu->>EmuContainer: stop()
end
Zemu-->>TestSuite: Emulator closed
Transport Error PropagationsequenceDiagram
participant TestSuite
participant Zemu
participant LedgerTransport
TestSuite->>Zemu: getTransport().send() or .exchange()
Zemu->>LedgerTransport: send()/exchange()
alt Critical transport error occurs
LedgerTransport-->>Zemu: error with status code
Zemu->>Zemu: Cache lastTransportError
Zemu-->>TestSuite: Error thrown immediately
else Success
LedgerTransport-->>Zemu: response
Zemu-->>TestSuite: response
end
Estimated code review effort4 (~90 minutes) ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
src/buttons.ts (1)
52-53: Avoidconsole.login library code – surface via errors or explicit return
Blindly logging unsupportedmodel / buttonKindmixes concerns and is easy to miss in CI. Either:- console.log(`Unsupported ButtonKind: ${model}, ${buttonKind}`) + // Consider bubbling the issue up instead of silent console noise + // e.g. throw new Error(`Unsupported ButtonKind: ${model}/${buttonKind}`)or return a discriminated-union “not-found” result.
.github/dependabot.yml (1)
24-24: File lacks trailing newline – YAML-lint fails
Add a single\nat EOF to satisfy linters.- NPM_TOKEN: ${{ secrets.NPM_TOKEN_PUBLISH_AUTO }} + NPM_TOKEN: ${{ secrets.NPM_TOKEN_PUBLISH_AUTO }} +.github/workflows/publish.yml (1)
24-24: YAML-lint: missing newline at EOF
Same as dependabot file – add trailing newline..mise.toml (1)
1-3: Consider pinning pnpm version for reproducibility.While Node.js 22 is appropriate, using
pnpm = "latest"could introduce variability across environments. Consider pinning to a specific version likepnpm = "8"for better reproducibility.-pnpm = "latest" +pnpm = "8"biome.json (2)
83-83: Consider adding more test framework globals if needed.The current globals list covers the main Vitest/Jest functions. You may want to add other common test utilities like
beforeAll,afterAll,vi(for Vitest mocking) if they're used in the codebase.- "globals": ["test", "describe", "it", "expect", "beforeEach", "afterEach", "beforeAll", "afterAll", "vi"] + "globals": ["test", "describe", "it", "expect", "beforeEach", "afterEach", "beforeAll", "afterAll", "vi", "Mock", "MockedFunction"]
91-102: Consider adding more specific file exclusions.The current exclusions are good, but you might want to add other common directories that should be excluded from linting:
"!**/node_modules/**", "!**/dist/**", "!**/.github/**", "!**/coverage/**", "!**/build/**", "!**/.next/**", "!**/.turbo/**", + "!**/tmp/**", + "!**/temp/**", + "!**/.cache/**", "!**/*.d.ts"tests/error-handling-simple.test.ts (1)
19-46: Consider enabling or removing the skipped test.The skipped test appears to be for understanding current behavior. If this test is still needed for validation, consider enabling it. If it was only for exploration during development, consider removing it to keep the test suite clean.
- test.skip('First understand current timeout behavior', async () => { + test('Should handle invalid APDU command errors', async () => {Or remove entirely if no longer needed.
tests/error-handling-fixed.test.ts (1)
39-39: Consider using more idiomatic Vitest assertion.While
expect.fail()works, consider using a more specific assertion that might be more idiomatic in Vitest:- expect.fail('Expected transport.send to throw an error') + throw new Error('Expected transport.send to throw an error')Or use
expect().rejects.toThrow()if restructuring the test to use async assertions.src/containerPool.ts (1)
254-290: Container recreation approach is inefficientThe current implementation stops and recreates containers to load new ELF files. As noted in the comment, implementing dynamic ELF loading via Speculos API would be more efficient.
Consider implementing dynamic ELF loading in a future iteration to avoid the overhead of container recreation. This would significantly improve pool performance, especially under high load.
src/Zemu.ts (2)
448-494: Consider handling all transport methods in the proxyThe proxy only intercepts
sendandexchangemethods. Other transport methods likesetScrambleKeyorclosemight also need error tracking.return new Proxy(originalTransport, { get(target, prop, receiver) { + // List of methods that perform communication + const communicationMethods = ['send', 'exchange', 'setScrambleKey']; + + if (communicationMethods.includes(prop as string)) { + return async function (...args: any[]) { + try { + self.lastTransportError = null + const result = await (target as any)[prop](...args) + return result + } catch (error) { + self.lastTransportError = error as Error + if (isCriticalTransportError(error)) { + const statusCode = (error as any).statusCode + self.log(`Critical transport error detected: ${getAPDUStatusMessage(statusCode)}`) + } + throw error + } + } + } + if (prop === 'send') { // existing send implementation... } if (prop === 'exchange') { // existing exchange implementation... } // For all other properties/methods, return as-is return Reflect.get(target, prop, receiver) }, }) as Transport
411-438: Complex error handling in close methodThe error handling has multiple nested try-catch blocks. Consider simplifying the logic for better maintainability.
async close(): Promise<void> { - try { - this.stopGRPCServer() - - if (this.usingPool && this.pooledContainer && Zemu.containerPool) { - this.log('Returning container to pool') - await Zemu.containerPool.release(this.pooledContainer) - this.usingPool = false - this.pooledContainer = null - } else { - this.log('Stopping container') - await this.emuContainer.stop() - } - } catch (error) { - this.log(`Error during close: ${error}`) - // If pool return fails, try to stop container directly - if (this.usingPool) { - try { - await this.emuContainer.stop() - } catch (stopError) { - this.log(`Failed to stop container after pool release failure: ${stopError}`) - } - this.usingPool = false - this.pooledContainer = null - } - throw error - } + this.stopGRPCServer() + + const shouldUsePool = this.usingPool && this.pooledContainer && Zemu.containerPool + + try { + if (shouldUsePool) { + this.log('Returning container to pool') + await Zemu.containerPool.release(this.pooledContainer!) + } else { + this.log('Stopping container') + await this.emuContainer.stop() + } + } catch (error) { + this.log(`Error during close: ${error}`) + + // Fallback: try to stop container directly if pool release failed + if (shouldUsePool) { + try { + await this.emuContainer.stop() + } catch (stopError) { + this.log(`Failed to stop container after pool release failure: ${stopError}`) + } + } + throw error + } finally { + // Always reset pool-related state + this.usingPool = false + this.pooledContainer = null + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
.eslintrc.json(0 hunks).github/dependabot.yml(1 hunks).github/workflows/ci-ts.yaml(1 hunks).github/workflows/main.yml(0 hunks).github/workflows/publish.yml(1 hunks).gitignore(1 hunks).mise.toml(1 hunks).nvmrc(1 hunks).prettierignore(0 hunks).prettierrc(0 hunks).vscode/extensions.json(1 hunks).vscode/settings.json(1 hunks).yarnrc.yml(0 hunks)CLAUDE.md(1 hunks)biome.json(1 hunks)jest.config.js(0 hunks)package.json(2 hunks)src/Zemu.ts(9 hunks)src/actions.ts(1 hunks)src/buttons.ts(1 hunks)src/buttons_flex.ts(4 hunks)src/buttons_stax.ts(4 hunks)src/constants.ts(1 hunks)src/containerPool.ts(1 hunks)src/emulator.ts(1 hunks)src/errors.ts(1 hunks)src/grpc/index.ts(2 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/zondax.ts(3 hunks)tests/basic.s.test.ts(2 hunks)tests/basic.x.test.ts(2 hunks)tests/error-handling-fixed.test.ts(1 hunks)tests/error-handling-simple.test.ts(1 hunks)tests/error-handling.test.ts(1 hunks)tests/globalsetup.ts(1 hunks)tests/minapp/index.ts(2 hunks)tests/pullImageKillOld.ts(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- .prettierignore
- .yarnrc.yml
- .prettierrc
- .github/workflows/main.yml
- .eslintrc.json
- jest.config.js
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/index.ts (1)
src/Zemu.ts (1)
Zemu(69-1142)
src/constants.ts (2)
src/index.ts (3)
DEFAULT_START_OPTIONS(20-20)IStartOptions(21-21)ButtonKind(21-21)src/types.ts (2)
IStartOptions(44-65)IDeviceWindow(16-21)
src/buttons.ts (1)
src/types.ts (2)
IButton(73-78)TModel(42-42)
src/buttons_flex.ts (2)
src/buttons_stax.ts (17)
quitAppButton(27-32)swipeContinueButton(34-39)navRightButton(41-46)navLeftButton(48-53)settingsNavRightButton(56-56)settingsNavLeftButton(58-63)settingsQuitButton(65-70)rejectButton(72-77)toggleOption1(80-85)approveTapButton(87-92)prevPageButton(94-99)approveHoldButton(101-106)toggleOption2(109-114)confirmYesButton(123-128)confirmNoButton(130-135)showQRButton(137-142)closeQRButton(144-149)src/types.ts (1)
IButton(73-78)
src/buttons_stax.ts (3)
src/buttons_flex.ts (17)
quitAppButton(27-32)swipeContinueButton(34-39)navRightButton(41-46)navLeftButton(48-53)settingsNavRightButton(56-56)settingsNavLeftButton(58-63)settingsQuitButton(65-70)rejectButton(72-77)toggleOption1(80-85)approveTapButton(87-92)prevPageButton(94-99)approveHoldButton(101-106)toggleOption2(109-114)confirmYesButton(116-121)confirmNoButton(123-128)showQRButton(130-135)closeQRButton(137-142)src/types.ts (1)
IButton(73-78)src/index.ts (1)
ButtonKind(21-21)
src/containerPool.ts (3)
src/emulator.ts (1)
EmuContainer(27-250)src/types.ts (2)
TModel(42-42)IStartOptions(44-65)src/constants.ts (4)
DEFAULT_HOST(26-26)DEFAULT_EMU_IMG(18-18)BASE_NAME(27-27)DEFAULT_START_DELAY(24-24)
🪛 GitHub Check: CodeQL
.github/workflows/publish.yml
[warning] 19-24: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
.github/workflows/ci-ts.yaml
[warning] 17-27: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
[warning] 28-47: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 YAMLlint (1.37.1)
.github/workflows/publish.yml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.27.2)
src/emulator.ts
23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-with-docker
🔇 Additional comments (56)
src/index.ts (1)
16-22: Excellent style consistency improvements.The conversion from double quotes to single quotes in import/export statements aligns perfectly with the project's migration to Biome tooling and improves code consistency.
src/actions.ts (1)
16-66: Clean formatting improvements throughout the file.The consistent conversion to single quotes and removal of trailing semicolons enhances code uniformity without affecting functionality.
src/buttons_flex.ts (2)
16-16: Consistent formatting improvements across button definitions.The standardization of object literal formatting and import quotes improves code consistency throughout the file.
Also applies to: 25-25, 32-32, 39-39, 46-46, 53-53, 63-63, 70-70, 77-77, 85-85, 92-92, 99-99, 106-106, 114-114, 121-121, 128-128, 135-135, 142-142, 170-170
56-58: Excellent fix for the button naming typo.Correcting
settingsNavnavLeftButtontosettingsNavLeftButtonfixes a clear naming error that could have caused confusion. The fix is properly applied both in the declaration and the TouchElements map.Also applies to: 152-152
src/buttons_stax.ts (2)
16-16: Consistent formatting improvements align with project standards.The style changes match the formatting applied across the codebase and improve consistency.
Also applies to: 25-25, 32-32, 39-39, 46-46, 53-53, 63-63, 70-70, 77-77, 85-85, 92-92, 99-99, 106-106, 114-114, 121-121, 128-128, 135-135, 142-142, 149-149, 177-177
56-58: Good catch on the button naming typo.The correction from
settingsNavnavLeftButtontosettingsNavLeftButtonfixes the same naming error found in the flex buttons, maintaining consistency across device types.Also applies to: 159-159
src/zondax.ts (1)
16-17: Final piece of the consistent formatting puzzle.The style changes in this file complete the consistent formatting migration across the entire codebase, maintaining all functional behavior while improving code uniformity.
Also applies to: 20-20, 32-32, 34-35, 40-40, 42-43, 48-48, 50-51, 63-63
src/buttons.ts (1)
17-19: Imports re-ordered only – no issues spotted
Purely stylistic, the new single-quote imports compile and tree-shake the same way.tests/minapp/index.ts (1)
24-34:Object.hasOwnlimits runtime support – confirm minimal Node version
Object.hasOwnis only available from Node v16.9.0+. The repo now targets Node 22, so that’s fine, but please ensure README / engines field reflect this bump to avoid surprises for downstream users still on LTS 14/16.tsconfig.json (1)
3-3: ES2022 target – verify consumer compatibility
Bumping to ES2022 is fine for Node ≥ 16; however, if the package is consumed in browser or older runtimes, document the requirement or publish transpiled code..nvmrc (1)
1-1: LGTM! Node.js 22 version specification is appropriate.The .nvmrc file correctly specifies Node.js version 22, aligning with the project's modernization efforts and consistency across tooling configurations.
.gitignore (1)
176-177: LGTM! Appropriate addition for test artifact management.Adding
tests/snapshots-tmp/to .gitignore is a good practice to prevent temporary snapshot files from being committed to version control..vscode/extensions.json (1)
1-3: LGTM! VS Code extension recommendation supports tooling migration.The biomejs.biome extension recommendation aligns perfectly with the project's migration from ESLint/Prettier to Biome toolchain.
tests/pullImageKillOld.ts (1)
1-6: LGTM! Improved asynchronous handling with modern patterns.The refactoring to use async/await within an IIFE ensures proper sequential execution of container operations. The single quote formatting aligns with the project's updated code style standards.
src/constants.ts (1)
16-86: LGTM! Consistent code style formatting aligned with Biome toolchain.The changes are purely stylistic (single quotes, consistent trailing commas) with no functional modifications. All constants, values, and logic remain unchanged, ensuring backward compatibility while aligning with the project's migration to Biome for code formatting.
tests/globalsetup.ts (3)
1-1: LGTM: Import style aligns with Biome formatting.The change from double quotes to single quotes is consistent with the project's migration to Biome for code formatting.
10-12: Verify module export compatibility with Vitest.The export was changed from an async function to a synchronous one. Ensure this is compatible with Vitest's global setup expectations.
3-8: No async behavior incatchExit– safe to be synchronous
We verified thatcatchExitis only ever called intests/globalsetup.tsand contains no asynchronous operations (it merely registers a SIGINT listener). It’s never awaited elsewhere, and Vitest supports synchronous global setup hooks. Removingasynchere won’t break anything.vitest.config.ts (1)
1-16: LGTM: Well-configured Vitest setup for container-based testing.The configuration is appropriate for this project:
- 60-second timeout accommodates container operations
- Node environment is correct for this testing context
- Test file patterns and excludes are properly configured
- The alias setup will improve import ergonomics
- Global APIs enable convenient test syntax
.vscode/settings.json (3)
2-13: LGTM: Proper Biome formatter configuration.The settings correctly configure Biome as the default formatter with appropriate code actions for TypeScript and JavaScript files. Format on save and import organization will maintain code consistency.
14-14: LGTM: Practical spell check customization.Adding "zemu" to the spell checker dictionary prevents false positives for the project name.
15-35: Optional workspace theming configuration.The extensive color customizations and Peacock configuration are optional aesthetic choices that don't impact functionality. They provide a consistent visual theme for the workspace.
src/types.ts (3)
17-41: LGTM: Consistent formatting improvements.The removal of semicolons from interface properties aligns with the project's migration to Biome formatting standards.
42-42: LGTM: Quote style consistency.The change from double quotes to single quotes in the
TModeltype alias is consistent with the Biome formatting configuration.
85-132: LGTM: Explicit enum values improve clarity and stability.The explicit numeric value assignments to enum members are an improvement over implicit auto-incremented values. This makes the enum values deterministic and prevents potential issues if enum members are reordered in the future.
.mise.toml (2)
5-7: LGTM: Appropriate environment configuration.Setting
NODE_ENV = "development"ensures consistent pnpm behavior across environments.
9-31: LGTM: Well-structured task definitions.The tasks provide useful development shortcuts:
- Individual tasks for common operations (install, build, test, check)
- Composite
devtask for complete setup workflow- Clear descriptions for each task
This configuration supports the CI/CD pipeline and local development workflow effectively.
tests/basic.x.test.ts (1)
1-89: LGTM: Clean migration to new toolchainThe stylistic changes consistently migrate the test file to the new Vitest/Biome toolchain with proper formatting (single quotes, semicolon removal, modern Node.js imports). All test logic remains functionally identical.
tests/basic.s.test.ts (1)
1-168: LGTM: Consistent stylistic migrationThe file successfully migrates to the new toolchain with consistent formatting changes while preserving all test functionality. The changes align perfectly with the project-wide migration standards.
tests/error-handling.test.ts (3)
15-15: Good practice: Disabling container pooling for error testingCorrectly disables container pooling to ensure isolated error behavior testing without interference from pooled containers.
78-90: Robust error propagation testingExcellent test design that verifies both timing (fast-fail under 1 second) and error message content (no timeout indication). This validates the enhanced error handling mentioned in the PR objectives.
111-119: Comprehensive error handling for getEventsGood defensive testing that handles both success and error scenarios for getEvents when the transport is in an error state. This covers edge cases effectively.
src/grpc/index.ts (2)
1-4: LGTM: Clean migration to modern importsProper migration to single quotes and modern Node.js import syntax (
node:path).
37-45: Excellent error propagation enhancementThe addition of
.catch()error handling ensures transport failures are properly propagated to gRPC clients instead of leaving them hanging. This directly supports the "Fast-Fail Transport Error Handling" feature mentioned in the PR objectives.package.json (4)
26-36: LGTM: Comprehensive toolchain migrationThe script migration from Yarn to pnpm and Jest to Vitest is comprehensive and consistent. The new Biome-based linting and formatting scripts provide good coverage for code quality maintenance.
55-67: LGTM: Clean migration to modern dev toolchainThe migration from ESLint/Prettier/Jest to Biome/Vitest is well-executed with appropriate version selections. TypeScript upgrade to 5.8.3 brings modern language features and improvements.
42-52: Ensure compatibility of upgraded dependencies in package.json
npm auditshows zero vulnerabilities, but major version bumps can introduce breaking changes. Please manually verify that all upgrades work together:• File to review:
package.json(lines 42–52)
• Key dependencies to check:
–@grpc/grpc-js&@grpc/proto-loader
–axios&axios-retry
–dockerode
–fs-extra
– Others with major bumps (e.g.,pngjs,randomstring)
• Action items:
– Review each library’s changelog for breaking changes
– Run full test suite (unit, integration, e2e)
– Perform smoke tests on gRPC endpoints and HTTP calls
21-24: Confirm Node.js engine aligns with your deployment targetsRequiring Node.js >=22.0.0 matches the current Active LTS (22.x entered LTS in October 2024 and is supported until April 2027). However, Node.js 20.x remains in maintenance support until April 2027.
- If you only target the latest LTS, keeping
"node": ">=22.0.0"is appropriate.- If you need to support Node.js 20.x environments, consider adjusting the engines field to
"node": ">=20.0.0".biome.json (1)
1-104: Well-structured Biome configuration with comprehensive rules.The configuration effectively replaces ESLint and Prettier with sensible defaults. The linter rules provide good coverage across complexity, correctness, security, and style categories, while the formatter settings are appropriate for modern TypeScript/JavaScript development.
tests/error-handling-simple.test.ts (2)
48-85: Well-structured timeout test with proper assertions.The test correctly validates timeout behavior by:
- Creating a modified snapshot that won't match
- Measuring elapsed time accurately
- Asserting reasonable timeout bounds (1900-2500ms for 2s timeout)
- Verifying error message content
The test design effectively validates the waitUntilScreenIs timeout functionality.
49-50: Consistent environment variable usage.Good practice to set the environment variable at both the instance level (line 15) and globally (line 50) to ensure container pooling is properly disabled for isolation.
src/emulator.ts (4)
17-18: Good modernization to Node.js built-in module imports.Using
node:pathandnode:streamimports follows modern Node.js best practices and makes it explicit that these are built-in modules.
64-69: Improved iteration with for...of loop.Converting from
forEachtofor...ofis a good improvement that aligns with the Biome linter rules and provides better performance characteristics.
21-23: Development certificate key is appropriately documented.The static analysis tool flagged this as a potential API key, but the extensive comments clearly indicate this is a well-known development certificate key specifically for emulator testing. The documentation explicitly states it's not for production use, which addresses security concerns.
146-149: Clean refactoring of library argument construction.The conversion from
forEachtofor...ofwith destructuring improves readability while maintaining the same functionality for building library arguments..github/workflows/ci-ts.yaml (3)
1-48: Well-structured CI workflow with modern tooling.The workflow effectively migrates from yarn to pnpm and integrates with external reusable workflows. The job dependencies and Docker test setup are appropriate for the project's needs.
21-22: Appropriate test configuration for Docker-dependent tests.Disabling tests in the ts-checks job and running them separately in the Docker job is the right approach since the tests require Docker containers to run properly.
46-47: Good environment setup for Docker tests.Setting
DISPLAY=:99is appropriate for headless testing environments and aligns with the emulator's display requirements.tests/error-handling-fixed.test.ts (4)
1-155: Comprehensive error handling test coverage.This test suite provides excellent coverage of the enhanced error handling functionality, validating fast-fail behavior, transport error propagation, and error handling across different methods. The timing assertions and error type checking are particularly valuable for ensuring the performance improvements work as expected.
35-51: Well-designed fast-fail validation test.The test correctly validates that transport errors occur quickly (< 1 second) rather than timing out, which is crucial for the performance improvements. The error assertions are appropriate for validating the expected behavior.
101-108: Excellent conditional error handling validation.The test intelligently handles both fast-fail transport errors and regular timeouts, validating timing and error messages appropriately for each case. This provides robust coverage of the error handling improvements.
115-154: Thorough getEvents error handling test.The test properly validates that getEvents handles transport errors appropriately, whether by throwing an error with the expected properties or succeeding if the device recovered. The flexible assertions account for different valid behaviors.
CLAUDE.md (1)
1-116: Well-structured documentation for AI code assistantsThe documentation provides comprehensive guidance covering project overview, development commands, architecture, testing patterns, and release process. This will be valuable for both human developers and AI assistants working with the codebase.
src/errors.ts (1)
72-99: Comprehensive APDU status message mappingThe function provides clear, human-readable messages for all defined APDU status codes, which will improve error diagnostics.
src/Zemu.ts (2)
440-494: Transport proxy implementation is innovativeThe proxy pattern for intercepting transport errors is a clever solution that enables fast-fail behavior without modifying the underlying transport library. This will significantly improve test feedback speed.
552-614: Excellent fast-fail implementation for transport errorsThe integration of transport error checking in wait methods prevents unnecessary timeouts and provides immediate feedback. This is a significant improvement for developer experience.
Address security warnings by adding explicit GITHUB_TOKEN permissions: - ci-ts.yaml: contents: read (minimal permissions for CI) - publish.yml: contents: read, packages: write (for npm publishing) This follows GitHub security best practices to limit token permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/publish.yml (2)
23-23: Pin the reusable workflow to an immutable commit SHA
Referencing a reusable workflow via a mutable branch (@main) leaves you open to supply-chain drift or a malicious force-push. Pin it to a specific commit SHA instead.- uses: zondax/_workflows/.github/workflows/_publish-npm.yaml@main + # Pin to a vetted commit for reproducibility & security + uses: zondax/_workflows/.github/workflows/_publish-npm.yaml@<commit-sha>
17-19: Addid-token: writeto enable provenance / OIDC
If the reusable workflow signs the NPM publication (GitHub’s provenance feature) or later adopts OIDC, the token must haveid-token: write. Add it now to avoid hidden runtime failures.permissions: contents: read # required by actions/checkout packages: write # needed if publishing to GH Packages + id-token: write # required for OIDC / provenance
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
28-28: YAMLLint: ensure trailing newline
File is missing a terminating newline, tripping linters (new-line-at-end-of-file). Add a blank line after the last line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
.github/workflows/ci-ts.yaml(1 hunks).github/workflows/publish.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-ts.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/publish.yml
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
- Add missing newline at EOF in .github/workflows/publish.yml - Replace console.log with proper error handling in buttons.ts - Improve error handling in Zemu.ts close method with try-catch-finally - Add setScrambleKey support to transport proxy for comprehensive error tracking - Remove debugging console.log statements from tests - Remove skipped test that was only for exploration - Replace console.warn with process.stderr.write for cleaner logging
- Update test title to reference 0x6E00 (CLA_NOT_SUPPORTED) instead of 0x6984 - Update comment before transport.send to reference CLA_NOT_SUPPORTED - Update comment on thrown error to reference CLA_NOT_SUPPORTED This aligns test documentation with the actual error code (0x6E00) that the test assertion validates, removing confusion about which error is being tested.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/publish.yml (2)
22-23: Pin reusable workflow to a commit SHA to prevent supply-chain driftReferencing
@mainmeans every run pulls whatever is at HEAD of that branch, introducing nondeterminism and security risk.- uses: zondax/_workflows/.github/workflows/_publish-npm.yaml@main + # pin to a vetted commit for reproducibility / security + uses: zondax/_workflows/.github/workflows/_publish-npm.yaml@<commit-sha>Replace
<commit-sha>with the latest trusted commit ID of the workflow repository.
17-19:id-token: writepermission missing – breaks provenance / OIDCThe reusable workflow publishes to npm; most publish actions now request an OIDC token for provenance.
Add the minimalid-token: writescope to avoid runtime failures and satisfy CodeQL.permissions: contents: read packages: write + id-token: write # required for OIDC-based npm provenancesrc/Zemu.ts (1)
262-270: Clear pool initialization promise after completion.The
poolInitPromiseis set but never cleared, which could cause issues if initialization fails and needs to be retried.private async tryStartWithPool(): Promise<void> { try { // Initialize pool if not already done if (!Zemu.poolInitialized && Zemu.containerPool === null) { if (!Zemu.poolInitPromise) { Zemu.poolInitPromise = Zemu.initializePool() } - await Zemu.poolInitPromise + try { + await Zemu.poolInitPromise + } finally { + Zemu.poolInitPromise = null + } }
🧹 Nitpick comments (3)
tests/error-handling-simple.test.ts (2)
20-21: Remove redundant environment variable setting.Setting
process.env.ZEMU_CONTAINER_POOLING = 'false'is redundant since you're already disabling pooling via thedisablePooloption. Additionally, modifying environment variables in tests can affect other tests running concurrently.- // Disable pooling globally for this test - process.env.ZEMU_CONTAINER_POOLING = 'false' -
46-47: Consider more robust timing assertions.The timing assertions may be flaky in CI environments where execution can be slower. Consider using a wider tolerance or focusing on the timeout behavior rather than precise timing.
- expect(elapsedTime).toBeGreaterThan(1900) - expect(elapsedTime).toBeLessThan(2500) + expect(elapsedTime).toBeGreaterThan(1800) // Allow for slower CI environments + expect(elapsedTime).toBeLessThan(3000)src/Zemu.ts (1)
451-513: Consider simplifying the transport proxy implementation.The transport proxy is quite complex with nested functions and repeated error handling logic. Consider extracting the common error handling logic to reduce duplication.
+ private wrapTransportMethod<T extends (...args: any[]) => Promise<any>>( + method: T, + methodName: string + ): T { + const self = this + return (async (...args: Parameters<T>) => { + try { + self.lastTransportError = null + const result = await method.apply(this.transport, args) + return result + } catch (error) { + self.lastTransportError = error as Error + if (isCriticalTransportError(error)) { + const statusCode = (error as any).statusCode + self.log(`Critical transport error in ${methodName}: ${getAPDUStatusMessage(statusCode)}`) + } + throw error + } + }) as T + } getTransport(): Transport { if (this.transport == null) throw new Error('Transport is not loaded.') const self = this const originalTransport = this.transport return new Proxy(originalTransport, { get(target, prop, receiver) { if (prop === 'send') { - return async function (cla: number, ins: number, p1: number, p2: number, data?: Buffer, statusList?: number[]) { - // ... existing logic - } + return self.wrapTransportMethod(target.send.bind(target), 'send') } if (prop === 'exchange') { - return async function (apdu: Buffer) { - // ... existing logic - } + return self.wrapTransportMethod(target.exchange.bind(target), 'exchange') } if (prop === 'setScrambleKey' && typeof target[prop] === 'function') { - return async function (key: string) { - // ... existing logic - } + return self.wrapTransportMethod((target as any).setScrambleKey.bind(target), 'setScrambleKey') } return Reflect.get(target, prop, receiver) }, }) as Transport }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (5)
.github/workflows/publish.yml(1 hunks)src/Zemu.ts(9 hunks)src/buttons.ts(1 hunks)src/emulator.ts(1 hunks)tests/error-handling-simple.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/buttons.ts
- src/emulator.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/error-handling-simple.test.ts (2)
src/types.ts (1)
IStartOptions(44-65)src/Zemu.ts (2)
Zemu(69-1162)snapshot(552-566)
🔇 Additional comments (5)
tests/error-handling-simple.test.ts (1)
28-36: Good snapshot modification logic.The approach of modifying a single byte to ensure the snapshot will never match is elegant and reliable for forcing a timeout condition.
src/Zemu.ts (4)
582-589: Excellent fast-fail error handling implementation.The implementation of immediate error detection and propagation in
waitUntilScreenIsis exactly what was needed to address the hanging timeout issue mentioned in the PR objectives. This will provide much better user experience with clear error messages.
1017-1025: Consistent error handling across waiting methods.Good consistency in applying the same fast-fail error handling pattern across
waitForText,waitUntilScreenIs, andwaitUntilScreenIsNotmethods.
417-440: Robust container cleanup with fallback logic.The enhanced
closemethod with pool return and fallback to direct container stop provides good resilience against pool failures while ensuring containers are always cleaned up properly.
975-978: Critical error propagation in getEvents is safeI reviewed every
getEvents()call across the codebase (including tests and all usage sites insrc/Zemu.ts) and found no consumer that assumes it will always return an array without handling exceptions.
- Tests already wrap
getEvents()intry/catchwhen verifying non-critical vs. critical errors.- Internal methods (e.g.,
waitForScreenChanges,click,fingerTouch) intentionally let critical errors bubble up.Throwing on critical transport errors won’t break existing code paths that expect arrays—merging as is.
- Update tests/error-handling-simple.test.ts to use disablePool: true - Update tests/error-handling.test.ts to use disablePool: true - Update tests/error-handling-fixed.test.ts to use disablePool: true The property name now matches the IStartOptions interface definition which defines disablePool?: boolean, not containerPooling.
- Create dummyButton constant to avoid calling getTouchElement for nanos/nanox/nanosp - Add device type checks before calling getTouchElement in Zemu.ts navigation - Fix TouchNavigation usage to check isTouchDevice before instantiation - Non-touch devices now use ActionKind.RightClick with dummy button for navigation - Remove hardcoded getTouchElement call in scheduleToNavElement function This resolves 'Unsupported ButtonKind: nanos, 1' and 'Unsupported ButtonKind: nanos, 2' errors when using ButtonKind.QuitAppButton and ButtonKind.SwipeContinueButton with Nano devices.
- Add cleanupStaleContainers() method to ContainerPool that runs before initialization - Use Docker API to find and remove stale zemu containers automatically - Enhanced global test setup with comprehensive process exit handlers - Added handlers for SIGINT, SIGTERM, beforeExit, uncaughtException, and unhandledRejection - Prevents port allocation conflicts by cleaning up leftover containers from previous runs - No more manual container cleanup required - the library manages this automatically This resolves port conflicts like 'Bind for 0.0.0.0:15000 failed: port is already allocated' that occurred when containers weren't properly cleaned up from interrupted test runs.
- Keep secure numeric-only patterns (rejecting unsafe CodeRabbit suggestion v*.*.*) - Add explicit patterns for common prerelease formats: - v1.2.3-rc1, v1.2.3-rc2 (release candidates) - v1.2.3-beta.1, v1.2.3-beta.2 (beta releases) - v1.2.3-alpha.1, v1.2.3-alpha.2 (alpha releases) - Maintain backwards compatibility with existing patterns - General v[0-9]+.[0-9]+.[0-9]+-* pattern still covers other formats This ensures -rc1 support while preventing dangerous patterns like v*.*.* that could match malicious strings like vMALICIOUS.CODE.INJECTION-hack.
- Use concise patterns with descriptive comments - Main releases: v1, v1.2, v1.2.3 - Prereleases: v1.2.3-rc1, v1.2.3-beta.1, etc. - Remove redundant explicit patterns - general v[0-9]+.[0-9]+.[0-9]+-* covers all prerelease formats - Maintain security by requiring numeric versions only
|
Also we need to update the readme |
| console.warn(`Cleaned up ${containers.length} stale containers`) | ||
| } catch (error) { | ||
| console.warn('Failed to cleanup stale containers:', error) | ||
| // Don't throw - we want initialization to continue even if cleanup fails |
There was a problem hiding this comment.
Do we? if cleanup failed, it is for a reason, and it may fail the following tests because of it. Why would we fail to cleanup?
- Add BUSY (0x9001) for device busy state - Add EXECUTION_ERROR (0x6400) for execution failures - Add EMPTY_BUFFER (0x6982) for empty buffer conditions - Add OUTPUT_BUFFER_TOO_SMALL (0x6983) for buffer size issues - Add TX_NOT_INITIALIZED (0x6987) for uninitialized transactions - Add BAD_KEY_HANDLE (0x6A80) for invalid key handles - Add SIGN_VERIFY_ERROR (0x6F01) for signature verification failures - Update critical transport errors list with new error codes - Add human-readable messages for all new error codes
…xport - Export dummyButton from buttons.ts module - Remove duplicate definitions from Zemu.ts and actions.ts - Update imports to use the shared dummyButton export - Reduces code duplication and improves maintainability
- Change permissions from packages:write to id-token:write - Required for npm provenance and OIDC authentication - Addresses security recommendation from PR review
- Update CI badge to point to new workflow (ci-ts.yaml) - Add support for Stax and Flex devices in overview - Update features list with container pooling and TypeScript support - Add Vitest to supported test frameworks - Include installation instructions with pnpm support - Add quick start code example - Highlight enhanced error handling capabilities
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/publish.yml (2)
25-25: Reusable workflow reference is still floating onmain
Previous review already highlighted supply-chain drift risk. Pin the reusable workflow to a commit SHA instead of a mutable branch:- uses: zondax/_workflows/.github/workflows/_publish-npm.yaml@main + # pin for reproducibility / security + uses: zondax/_workflows/.github/workflows/_publish-npm.yaml@<commit-sha>
8-13: Release tag patterns still use regex – workflow will never trigger
push.tagsexpects shell-style glob patterns, not regex. Patterns likev[0-9]+will only match the literal string, so no publish runs will start.- - "v[0-9]+" - - "v[0-9]+.[0-9]+" - - "v[0-9]+.[0-9]+.[0-9]+" - - "v[0-9]+.[0-9]+.[0-9]+-*" +# semantic-version tags (e.g. v1, v1.2, v1.2.3, v1.2.3-beta.1) + - "v*" + - "v*.*" + - "v*.*.*" + - "v*.*.*-*"Without this change, the publish job will never run for new releases.
🧹 Nitpick comments (3)
README.md (2)
30-35: Replace informal “thru” with “through”Minor wording polish for formal documentation.
- - Navigate thru screens and take screenshots of them + - Navigate through screens and take screenshots of them
40-46: Include Yarn for parity & keep flag styles consistentMany consumers still rely on Yarn; adding it prevents friction and keeps the three major package managers covered.
Also use the long--save-dev/--devflag consistently for clarity.```bash npm install --save-dev @zondax/zemu +# or +yarn add --dev @zondax/zemu # or pnpm add --save-dev @zondax/zemu</blockquote></details> <details> <summary>.github/workflows/publish.yml (1)</summary><blockquote> `10-10`: **Remove stray trailing spaces to appease YAML linters** Line 10 (`- "v[0-9]+.[0-9]+" `) contains two trailing spaces flagged by YAMLlint. ```diff - - "v[0-9]+.[0-9]+" + - "v[0-9]+.[0-9]+"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
.github/workflows/publish.yml(1 hunks)README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~35-~35: The word ‘thru’ is informal. Consider replacing it with “through”.
Context: ...rol (buttons, reset, etc.) - Navigate thru screens and take screenshots of them - ...
(THRU)
🪛 YAMLlint (1.37.1)
.github/workflows/publish.yml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-with-docker
- GitHub Check: test-with-docker
🔇 Additional comments (3)
README.md (2)
9-9: CI badge switch looks correctThe workflow path and badge link resolve properly. Nice touch keeping the badge copy-paste friendly.
50-65: Clarify quick-start placeholders & async context
APP_PATHis undefined; readers may be confused where it comes from.- Top-level
awaitonly works in ESM modules or an async IIFE; call that out to avoid copy-paste errors.import Zemu from '@zondax/zemu' -const sim = new Zemu(APP_PATH) +// Path to your compiled Ledger app (e.g. '../app/bin/app.elf') +const APP_PATH = '/absolute/or/relative/path/to/app' +const sim = new Zemu(APP_PATH) -await sim.start({ model: 'nanos' }) +// wrap in an async function if not using ESM +await sim.start({ model: 'nanos' })Optionally add a short note after the snippet:
Tip: If you are using CommonJS, wrap the snippet in an
async function main() { … } main();..github/workflows/publish.yml (1)
19-21: 👍 Good addition of a minimalpermissionsblock
The top-level permissions tighten the token scope and satisfy CodeQL.
|
any chance we can merge this, release as rc1 and check it with our apps? |
chcmedeiros
left a comment
chcmedeiros
left a comment
There was a problem hiding this comment.
LGTM. Lets try this on the apps
6 participants
🚀 Zemu v0.50.0 Release
This release introduces significant performance improvements and enhanced error handling capabilities to the Zemu testing framework.
✨ Major Features
1. Container Pooling for Dramatic Performance Improvements (#549, #551)
Problem Solved: Each test container startup previously took 3-5 seconds, significantly slowing down test suites.
Solution: Pre-initialize a pool of Docker containers that are reused across test runs.
Performance Impact:
Implementation Details:
ContainerPoolclass manages container lifecycle2. Fast-Fail Transport Error Handling (#551)
Problem Solved: Transport errors (like
0x6984or0x6E00) would cause tests to hang until timeout instead of failing immediately.Solution: Implement intelligent error detection and propagation throughout the transport layer.
Impact:
🔧 Configuration
Container Pool Settings
The container pool uses the following default configuration:
To disable pooling entirely:
# Environment variable ZEMU_DISABLE_POOL=truePer-Instance Configuration
📋 Complete Changelog
Added
Container Pooling System (
src/containerPool.ts)Enhanced Error Handling (
src/errors.ts)Transport Error Interception
getTransport()Changed
src/Zemu.ts)waitUntilScreenIs(),waitUntilScreenIsNot(), andwaitForText()with error detectiongetEvents()to propagate transport errorsFixed
Infrastructure
🧪 Testing
Extensive test coverage added:
tests/error-handling.test.ts- Core error handling validationtests/error-handling-fixed.test.ts- Transport error propagation tests📊 Performance Benchmarks
Test suite execution times (example with 20 tests):
🔄 Migration Guide
This release is fully backward compatible. To adopt:
ZEMU_DISABLE_POOL=trueor usecontainerPooling: false🏗️ Technical Debt Addressed
🔗 Related PRs
📝 Notes
This release represents a major step forward in Zemu's performance and reliability, making it even more suitable for large-scale Ledger application testing.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style
Documentation
Tests