feat: add more apis for kitten-lynx#2364
Conversation
🦋 Changeset detectedLatest commit: 4d3668c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a screenshot() method to KittenLynxView (CDP stream → base64 → Buffer, optional file write), test coverage for screenshots and interactions, changeset metadata, build/config changes, and dev dependency additions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/testing-library/kitten-lynx/tests/lynx.spec.ts (1)
91-96: Consider usingsetTimeoutfromnode:timers/promisesfor cleaner delays.The test already imports
setTimeoutfromnode:timers/promises(via the module, visible in KittenLynxView.ts). Usingawait setTimeout(2000)would be cleaner than wrapping in a Promise constructor.♻️ Cleaner delay syntax
+import { setTimeout } from 'node:timers/promises'; + // In the test: - await new Promise(resolve => setTimeout(resolve, 2000)); + await setTimeout(2000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts` around lines 91 - 96, Replace the Promise-wrapped delays in the test with the imported setTimeout from node:timers/promises: instead of awaiting new Promise(resolve => setTimeout(resolve, 2000)), use await setTimeout(2000); update the two occurrences in lynx.spec.ts (the delays before checking `.Logo--react`/`.Logo--lynx` and after logoParent?.tap()) and ensure the module import for setTimeout is present and not shadowed by any local variable.packages/testing-library/kitten-lynx/src/KittenLynxView.ts (1)
366-370: Consider usingawait usingfor cleaner disposal.TypeScript 5.x supports the
await usingsyntax which automatically handles async disposal. The current manual approach works but is more verbose.♻️ Optional refactor using await using
- const stream = await this._connector.sendCDPStream( + await using stream = await this._connector.sendCDPStream( this._clientId, ReadableStream.from([ { method: 'Lynx.getScreenshot', sessionId } as any, ]), ); let buffer: Buffer | undefined; - try { - for await (const msg of stream) { - if (msg.method === 'Lynx.screenshotCaptured') { - const data = (msg.params as any).data; - if (data) { - buffer = Buffer.from(data, 'base64'); - break; - } + for await (const msg of stream) { + if (msg.method === 'Lynx.screenshotCaptured') { + const data = (msg.params as any).data; + if (data) { + buffer = Buffer.from(data, 'base64'); + break; } } - } finally { - if (typeof stream[Symbol.asyncDispose] === 'function') { - await stream[Symbol.asyncDispose](); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts` around lines 366 - 370, Replace the manual disposal in the try/finally that checks stream[Symbol.asyncDispose] with TypeScript 5's await using syntax: bind the async-disposable stream with "await using" (e.g., await using const s = stream) before the block that consumes it, use s inside the block, and remove the finally block that calls stream[Symbol.asyncDispose](); ensure the code that referenced stream now uses the new bound variable (stream -> s) and that your tsconfig/target supports the "await using" feature so compilation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts`:
- Around line 339-352: The screenshot method can hang because sendCDPStream's
optional signal isn't used and the format/quality options aren't forwarded;
update the screenshot function to create an AbortController (or accept a
timeout), pass controller.signal to this._connector.sendCDPStream, and ensure
the controller is aborted on timeout or when the stream completes to avoid
indefinite blocking; also include the declared options.format and
options.quality in the CDP request payload sent to ReadableStream.from (the
Lynx.getScreenshot call) so the native side receives them, and ensure any
listeners/streams are cleaned up on abort or completion.
---
Nitpick comments:
In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts`:
- Around line 366-370: Replace the manual disposal in the try/finally that
checks stream[Symbol.asyncDispose] with TypeScript 5's await using syntax: bind
the async-disposable stream with "await using" (e.g., await using const s =
stream) before the block that consumes it, use s inside the block, and remove
the finally block that calls stream[Symbol.asyncDispose](); ensure the code that
referenced stream now uses the new bound variable (stream -> s) and that your
tsconfig/target supports the "await using" feature so compilation succeeds.
In `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts`:
- Around line 91-96: Replace the Promise-wrapped delays in the test with the
imported setTimeout from node:timers/promises: instead of awaiting new
Promise(resolve => setTimeout(resolve, 2000)), use await setTimeout(2000);
update the two occurrences in lynx.spec.ts (the delays before checking
`.Logo--react`/`.Logo--lynx` and after logoParent?.tap()) and ensure the module
import for setTimeout is present and not shadowed by any local variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b1ecf89-ecd1-4687-b9f8-3afc9798ffb8
📒 Files selected for processing (3)
.changeset/olive-readers-turn.mdpackages/testing-library/kitten-lynx/src/KittenLynxView.tspackages/testing-library/kitten-lynx/tests/lynx.spec.ts
Merging this PR will improve performance by 6.66%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | basic-performance-div-100 |
7.3 ms | 6.9 ms | +5.39% |
| ⚡ | basic-performance-nest-level-100 |
7.4 ms | 6.9 ms | +6.66% |
Comparing PupilTong:p/hw/kitten-lynx-more-apis (4d3668c) with main (4157823)
Footnotes
-
3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Web Explorer#8318 Bundle Size — 385.21KiB (0%).4d3668c(current) vs 4157823 main#8314(baseline) Bundle metrics
|
| Current #8318 |
Baseline #8314 |
|
|---|---|---|
155.59KiB |
155.59KiB |
|
35.1KiB |
35.1KiB |
|
0% |
0% |
|
8 |
8 |
|
8 |
8 |
|
238 |
238 |
|
16 |
16 |
|
2.97% |
2.97% |
|
4 |
4 |
|
0 |
0 |
Bundle size by type no changes
| Current #8318 |
Baseline #8314 |
|
|---|---|---|
254.26KiB |
254.26KiB |
|
95.85KiB |
95.85KiB |
|
35.1KiB |
35.1KiB |
Bundle analysis report Branch PupilTong:p/hw/kitten-lynx-more-... Project dashboard
Generated by RelativeCI Documentation Report issue
266567d to
4d3668c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/testing-library/kitten-lynx/src/KittenLynxView.ts (1)
339-378:⚠️ Potential issue | 🟠 MajorPrevent indefinite wait and forward declared screenshot options.
At Line 356,
sendCDPStreamis started without an abort signal, so the loop at Line 370 can block forever if noLynx.screenshotCapturedarrives. Also, Line 366 sendsparams: {}, soformat/qualityfrom the public API are silently ignored.🔧 Suggested patch
async screenshot(options?: { path?: string; format?: 'jpeg' | 'png' | 'webp'; quality?: number; + timeout?: number; }): Promise<Buffer> { const { ReadableStream } = await import('node:stream/web'); const sessionId = (this._channel as any)._sessionId; + const timeout = options?.timeout ?? 30000; + const controller = new AbortController(); + const timeoutId = globalThis.setTimeout(() => controller.abort(), timeout); @@ const stream = await this._connector.sendCDPStream( this._clientId, inputStream, + { signal: controller.signal }, ); @@ pushMessage({ method: 'Lynx.getScreenshot', - params: {}, + params: { + ...(options?.format ? { format: options.format } : {}), + ...(typeof options?.quality === 'number' + ? { quality: options.quality } + : {}), + }, sessionId, }); @@ } finally { + clearTimeout(timeoutId); closeStream(); if (typeof stream[Symbol.asyncDispose] === 'function') { await stream[Symbol.asyncDispose](); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts` around lines 339 - 378, The screenshot method starts sendCDPStream without an abort signal and ignores the caller's options; update KittenLynxView.screenshot to create an AbortController and pass its signal into this._connector.sendCDPStream so the stream can be cancelled on timeout or on error/close, and ensure you call controller.abort() (or closeStream) in the finally block to avoid indefinite waiting; also forward the provided options (format, quality) from the public screenshot signature into the pushed CDP message params instead of {} so the remote side receives format/quality, and throw a clear error if no Lynx.screenshotCaptured arrives within a reasonable timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts`:
- Around line 344-346: The code currently dereferences this._channel as any to
read _sessionId (in the screenshot() flow) which can throw if no
attachment/session exists (e.g., screenshot() called before goto()); add an
explicit precondition check similar to locator() by verifying this._channel is
attached and has a valid _sessionId before reading it: if missing, throw a clear
Error indicating the attachment/session is required (mention screenshot() and
goto() in the message). Update the access site where ReadableStream and
sessionId are read (references: ReadableStream import, this._channel, and
_sessionId) to perform this guard and early error return.
---
Duplicate comments:
In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts`:
- Around line 339-378: The screenshot method starts sendCDPStream without an
abort signal and ignores the caller's options; update KittenLynxView.screenshot
to create an AbortController and pass its signal into
this._connector.sendCDPStream so the stream can be cancelled on timeout or on
error/close, and ensure you call controller.abort() (or closeStream) in the
finally block to avoid indefinite waiting; also forward the provided options
(format, quality) from the public screenshot signature into the pushed CDP
message params instead of {} so the remote side receives format/quality, and
throw a clear error if no Lynx.screenshotCaptured arrives within a reasonable
timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8d2691f-4a5d-42fd-bf00-d229792a4be1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.changeset/olive-readers-turn.mdpackages/testing-library/kitten-lynx/lynx.config.jspackages/testing-library/kitten-lynx/package.jsonpackages/testing-library/kitten-lynx/rslib.config.tspackages/testing-library/kitten-lynx/src/KittenLynxView.tspackages/testing-library/kitten-lynx/tests/lynx.spec.tspackages/testing-library/kitten-lynx/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/testing-library/kitten-lynx/tsconfig.json
✅ Files skipped from review due to trivial changes (3)
- packages/testing-library/kitten-lynx/lynx.config.js
- packages/testing-library/kitten-lynx/package.json
- .changeset/olive-readers-turn.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/testing-library/kitten-lynx/rslib.config.ts
- packages/testing-library/kitten-lynx/tests/lynx.spec.ts
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/react@0.117.0 ### Minor Changes - feat: export `GlobalPropsProvider`, `GlobalPropsConsumer`, `useGlobalProps` and `useGlobalPropsChanged` for `__globalProps` ([#2346](#2346)) - `GlobalPropsProvider`: A Provider component that accepts `children`. It is used to provide the `lynx.__globalProps` context. - `GlobalPropsConsumer`: A Consumer component that accepts a function as a child. It is used to consume the `lynx.__globalProps` context. - `useGlobalProps`: A hook that returns the `lynx.__globalProps` object. It triggers a re-render when `lynx.__globalProps` changes. - `useGlobalPropsChanged`: A hook that accepts a callback function. The callback is invoked when `lynx.__globalProps` changes. Note: When `globalPropsMode` is not set to `'event'` (default is `'reactive'`), these APIs will be ineffective (pass-through) and will log a warning in development mode, as updates are triggered automatically by full re-render. - **BREAKING CHANGE**: ([#2319](#2319)) Change preact package from `@hongzhiyuan/preact` to `@lynx-js/internal-preact`. Upgrade preact from [f7693b72](preactjs/preact@f7693b7) to [55254ef7](preactjs/preact@55254ef), see diffs at [f7693b72...55254ef7](https://github.com/preactjs/preact/compare/f7693b72ecb4a40c66e6e47f54e2d4edc374c9f0...preactjs:preact:55254ef7021e563cc1a86fb816058964a1b6a29a?expand=1). - feat: add `globalPropsMode` option to `PluginReactLynxOptions` ([#2346](#2346)) - When configured to `"event"`, `updateGlobalProps` will only trigger a global event and skip the `runWithForce` flow. - Defaults to `"reactive"`, which means `updateGlobalProps` will trigger re-render automatically. ### Patch Changes - Add `__BACKGROUND__` guard on `onBackgroundSnapshotInstanceUpdateId` event to prevent bundling to main-thread on dev environment. ([#2332](#2332)) - refactor: extract static string in template literal ([#2334](#2334)) - fix: avoid crash when spread undefined ref ([#2333](#2333)) - Avoid registering lifecycle refs for main-thread functions (MTF) that have not received an `execId` during `renderPage()` first-screen binding. ([#2320](#2320)) ## @lynx-js/react-umd@0.117.0 ### Minor Changes - Add standalone UMD build of the ReactLynx runtime. ([#2331](#2331)) ## @lynx-js/react-rsbuild-plugin@0.13.0 ### Minor Changes - **BREAKING CHANGE**: ([#2319](#2319)) Change preact package from `@hongzhiyuan/preact` to `@lynx-js/internal-preact`. Upgrade preact from [f7693b72](preactjs/preact@f7693b7) to [55254ef7](preactjs/preact@55254ef), see diffs at [f7693b72...55254ef7](https://github.com/preactjs/preact/compare/f7693b72ecb4a40c66e6e47f54e2d4edc374c9f0...preactjs:preact:55254ef7021e563cc1a86fb816058964a1b6a29a?expand=1). - feat: add `globalPropsMode` option to `PluginReactLynxOptions` ([#2346](#2346)) - When configured to `"event"`, `updateGlobalProps` will only trigger a global event and skip the `runWithForce` flow. - Defaults to `"reactive"`, which means `updateGlobalProps` will trigger re-render automatically. ### Patch Changes - Updated dependencies \[[`f1129ea`](f1129ea), [`27f1cff`](27f1cff), [`ed566f0`](ed566f0), [`402ec2b`](402ec2b)]: - @lynx-js/react-webpack-plugin@0.8.0 - @lynx-js/react-refresh-webpack-plugin@0.3.5 - @lynx-js/react-alias-rsbuild-plugin@0.13.0 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/template-webpack-plugin@0.10.6 - @lynx-js/css-extract-webpack-plugin@0.7.0 ## @lynx-js/react-webpack-plugin@0.8.0 ### Minor Changes - feat: add `globalPropsMode` option to `PluginReactLynxOptions` ([#2346](#2346)) - When configured to `"event"`, `updateGlobalProps` will only trigger a global event and skip the `runWithForce` flow. - Defaults to `"reactive"`, which means `updateGlobalProps` will trigger re-render automatically. ### Patch Changes - Fix sourcemap misalignment when wrapping lazy bundle main-thread chunks. ([#2361](#2361)) The lazy bundle IIFE wrapper is now injected in `processAssets` at `PROCESS_ASSETS_STAGE_OPTIMIZE_SIZE + 1` by walking chunk groups instead of patching assets in `beforeEncode`. - With `experimental_isLazyBundle: true`, the wrapper is applied to lazy-bundle chunk groups. - Without lazy bundle mode, the wrapper is applied to async main-thread chunk groups generated by dynamic import. Injecting the wrapper in this stage keeps the emitted JS stable after optimization while still running before `DEV_TOOLING` sourcemap finalization, so the generated `.js` and `.js.map` stay aligned. - Set `__DEV__` and `__PROFILE__` to `true` on `NODE_ENV === 'development'`. ([#2324](#2324)) ## @lynx-js/rspeedy@0.13.6 ### Patch Changes - Rename Web Preview label to fix URL alignment ([#2355](#2355)) - Updated dependencies \[[`799fda8`](799fda8)]: - @lynx-js/cache-events-webpack-plugin@0.0.3 - @lynx-js/web-rsbuild-server-middleware@0.19.9 ## @lynx-js/lynx-bundle-rslib-config@0.2.3 ### Patch Changes - Fix snapshot not found error when dev with external bundle ([#2316](#2316)) ## @lynx-js/external-bundle-rsbuild-plugin@0.0.4 ### Patch Changes - Updated dependencies \[[`ed566f0`](ed566f0)]: - @lynx-js/externals-loading-webpack-plugin@0.0.5 ## @lynx-js/kitten-lynx-test-infra@0.1.1 ### Patch Changes - feat: support page.screenshot() ([#2364](#2364)) - feat: initial commit ([#2272](#2272)) ## @lynx-js/testing-environment@0.1.12 ### Patch Changes - Implement `__ElementAnimate` PAPI for web platform animation lifecycle ([#2329](#2329)) ## @lynx-js/web-constants@0.19.9 ### Patch Changes - Implement `__ElementAnimate` PAPI for web platform animation lifecycle ([#2329](#2329)) - Updated dependencies \[]: - @lynx-js/web-worker-rpc@0.19.9 ## @lynx-js/web-core@0.19.9 ### Patch Changes - Updated dependencies \[[`2efecc2`](2efecc2)]: - @lynx-js/web-constants@0.19.9 - @lynx-js/web-mainthread-apis@0.19.9 - @lynx-js/web-worker-runtime@0.19.9 - @lynx-js/web-worker-rpc@0.19.9 ## @lynx-js/web-core-wasm@0.0.6 ### Patch Changes - reexports essential utils & types in @lynx-js/web-elements from @lynx-js/web-core-wasm/client ([#2321](#2321)) - Updated dependencies \[]: - @lynx-js/web-worker-rpc@0.19.9 ## @lynx-js/web-mainthread-apis@0.19.9 ### Patch Changes - Updated dependencies \[[`2efecc2`](2efecc2)]: - @lynx-js/web-constants@0.19.9 ## @lynx-js/web-worker-runtime@0.19.9 ### Patch Changes - Updated dependencies \[[`2efecc2`](2efecc2)]: - @lynx-js/web-constants@0.19.9 - @lynx-js/web-mainthread-apis@0.19.9 - @lynx-js/web-worker-rpc@0.19.9 ## @lynx-js/cache-events-webpack-plugin@0.0.3 ### Patch Changes - Cache `globalThis.loadDynamicComponent` in the cache events runtime and add tests covering tt methods, performance events, and globalThis replay behavior. ([#2343](#2343)) ## @lynx-js/externals-loading-webpack-plugin@0.0.5 ### Patch Changes - Fix snapshot not found error when dev with external bundle ([#2316](#2316)) ## @lynx-js/react-refresh-webpack-plugin@0.3.5 ### Patch Changes - Fix snapshot not found error when dev with external bundle ([#2316](#2316)) ## @lynx-js/template-webpack-plugin@0.10.6 ### Patch Changes - Updated dependencies \[[`d034dae`](d034dae)]: - @lynx-js/web-core-wasm@0.0.6 ## create-rspeedy@0.13.6 ## @lynx-js/react-alias-rsbuild-plugin@0.13.0 ## upgrade-rspeedy@0.13.6 ## @lynx-js/web-core-server@0.19.9 ## @lynx-js/web-rsbuild-server-middleware@0.19.9 ## @lynx-js/web-worker-rpc@0.19.9 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Tests
Checklist