feat: Add a React example application to the kitten-lynx test fixture…#2356
Merged
PupilTong merged 10 commits intolynx-family:mainfrom Mar 20, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 5f4ce0f The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a local React test fixture to the kitten-lynx testing library, including a development server configuration, an interactive flappy-bird-style component with physics simulation, and updated testing infrastructure to serve bundles locally and manage navigation timeouts more flexibly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/testing-library/kitten-lynx/src/KittenLynxView.ts (1)
287-287: AddDOM.enableto the Protocol interface for improved type safety.The
as anycast indicatesDOM.enableis not defined in the Protocol types. SinceDOM.enableis a standard CDP request, it should be added to the Protocol interface inCDPChannel.tsto eliminate the type assertion and maintain consistency with other CDP methods.🤖 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` at line 287, The call to channel.send('DOM.enable' as any, {}) uses a cast because 'DOM.enable' is missing from the Protocol types; update the Protocol interface in CDPChannel.ts to include the DOM.enable method signature (matching other CDP methods) so channel.send can be called with 'DOM.enable' without casting—locate the Protocol interface/type declaration in CDPChannel.ts and add an entry for "DOM.enable" with the appropriate request/response shapes so KittenLynxView.ts can call channel.send('DOM.enable', {}) with full type safety.packages/testing-library/kitten-lynx/tests/lynx.spec.ts (1)
17-17: AvoidanyfordevServerin strict TypeScript code.Line 17 loses type safety and can hide lifecycle API misuse. Use a minimal structural type instead.
♻️ Suggested typing improvement
- let devServer: any; + let devServer: { close: () => Promise<void> } | undefined;As per coding guidelines:
**/*.{ts,tsx}: Use TypeScript with the strictest mode configuration as defined in tsconfig.json.🤖 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` at line 17, Replace the loose any on the devServer variable with a strict structural type: import type { ViteDevServer } from 'vite' and declare the variable as let devServer: ViteDevServer | undefined; (or define a small interface with the lifecycle methods you call, e.g., close(): Promise<void> | void) and update usages in your setup/teardown (e.g., close()/listen/ready) to satisfy the chosen type so the compiler checks correct API usage for devServer.
🤖 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/test-fixture/tsconfig.json`:
- Line 12: The tsconfig.json's "include" entry incorrectly points to "src" which
doesn't exist; update the "include" array in tsconfig.json to reference the
actual TypeScript directory "cases/react-example" (replace "src" with
"cases/react-example") so the compiler picks up the project's TS files.
In `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts`:
- Around line 57-62: The try/catch around execSync(`adb reverse tcp:${devPort}
tcp:${devPort}`) currently only warns on failure which lets the test continue
and fail later; change the catch in tests/lynx.spec.ts to fail fast by throwing
an error (or rethrowing the caught error) with a clear message that includes
devPort and the original error (e.g., replace console.warn(...) with throw new
Error(`adb reverse failed for port ${devPort}: ${e}`)) so the test setup stops
immediately when adb reverse cannot be established.
---
Nitpick comments:
In `@packages/testing-library/kitten-lynx/src/KittenLynxView.ts`:
- Line 287: The call to channel.send('DOM.enable' as any, {}) uses a cast
because 'DOM.enable' is missing from the Protocol types; update the Protocol
interface in CDPChannel.ts to include the DOM.enable method signature (matching
other CDP methods) so channel.send can be called with 'DOM.enable' without
casting—locate the Protocol interface/type declaration in CDPChannel.ts and add
an entry for "DOM.enable" with the appropriate request/response shapes so
KittenLynxView.ts can call channel.send('DOM.enable', {}) with full type safety.
In `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts`:
- Line 17: Replace the loose any on the devServer variable with a strict
structural type: import type { ViteDevServer } from 'vite' and declare the
variable as let devServer: ViteDevServer | undefined; (or define a small
interface with the lifecycle methods you call, e.g., close(): Promise<void> |
void) and update usages in your setup/teardown (e.g., close()/listen/ready) to
satisfy the chosen type so the compiler checks correct API usage for devServer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a56ed28f-af49-495f-9390-1095dcfb4a07
⛔ Files ignored due to path filters (4)
packages/testing-library/kitten-lynx/test-fixture/cases/react-example/assets/arrow.pngis excluded by!**/*.pngpackages/testing-library/kitten-lynx/test-fixture/cases/react-example/assets/lynx-logo.pngis excluded by!**/*.pngpackages/testing-library/kitten-lynx/test-fixture/cases/react-example/assets/react-logo.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/testing-library/kitten-lynx/package.jsonpackages/testing-library/kitten-lynx/src/KittenLynxView.tspackages/testing-library/kitten-lynx/test-fixture/cases/react-example/App.csspackages/testing-library/kitten-lynx/test-fixture/cases/react-example/App.tsxpackages/testing-library/kitten-lynx/test-fixture/cases/react-example/index.tsxpackages/testing-library/kitten-lynx/test-fixture/cases/react-example/rspeedy-env.d.tspackages/testing-library/kitten-lynx/test-fixture/cases/react-example/useFlappy.tspackages/testing-library/kitten-lynx/test-fixture/lynx.config.jspackages/testing-library/kitten-lynx/test-fixture/tsconfig.jsonpackages/testing-library/kitten-lynx/tests/lynx.spec.ts
packages/testing-library/kitten-lynx/test-fixture/tsconfig.json
Outdated
Show resolved
Hide resolved
Merging this PR will degrade performance by 8.75%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#8283 Bundle Size — 385.21KiB (0%).5f4ce0f(current) vs 726f600 main#8237(baseline) Bundle metrics
|
| Current #8283 |
Baseline #8237 |
|
|---|---|---|
 Initial JS |
155.59KiB |
155.59KiB |
| Initial CSS |
35.1KiB |
35.1KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
238 |
238 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.97% |
2.97% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8283 |
Baseline #8237 |
|
|---|---|---|
| JS |
254.26KiB |
254.26KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.1KiB |
35.1KiB |
Bundle analysis report Branch PupilTong:p/hw/kitten-test-use-l... Project dashboard
Generated by RelativeCI Documentation Report issue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/testing-library/kitten-lynx/tests/lynx.spec.ts (1)
57-71:⚠️ Potential issue | 🟠 MajorFail fast when ADB reverse setup fails (don’t continue on warning).
Line 70 only warns and allows setup to continue, which usually shifts failure to navigation with less actionable diagnostics. This should stop
beforeAllimmediately.🛠️ Proposed reliability fix
let bundleUrl: string; let devPort: number; + let adbReverseApplied = false; @@ - try { + try { const client = new AdbServerClient( new AdbServerNodeTcpConnector({ port: 5037 }), ); const devices = await client.getDevices(); const connectedDevice = devices.find(d => d.state === 'device') || devices[0]; - if (connectedDevice) { - adb = await client.createAdb({ serial: connectedDevice.serial }); - await adb.reverse.addExternal(`tcp:${devPort}`, `tcp:${devPort}`); - } + if (!connectedDevice) { + throw new Error('No connected ADB device found for reverse port mapping'); + } + adb = await client.createAdb({ serial: connectedDevice.serial }); + await adb.reverse.addExternal(`tcp:${devPort}`, `tcp:${devPort}`); + adbReverseApplied = true; } catch (e) { - console.warn(`Failed to run adb reverse for port ${devPort}`, e); + throw new Error(`Failed to run adb reverse for port ${devPort}: ${String(e)}`); } @@ - if (adb) { + if (adb && adbReverseApplied) { await adb.reverse.remove(`tcp:${devPort}`); - await adb.close(); } + await adb?.close();#!/bin/bash # Verify whether adb reverse failure path still warns instead of failing. rg -n -C3 'reverse\.addExternal|Failed to run adb reverse|console\.warn|throw new Error' packages/testing-library/kitten-lynx/tests/lynx.spec.tsBased on learnings: For CI/device networking, use host-served bundles with
adb reverse ...before localhost navigation; failures should be explicit.🤖 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 57 - 71, The try/catch around the AdbServerClient setup swallows failures by calling console.warn, letting beforeAll continue; change the catch to fail fast by rethrowing or throwing a new error so the test setup stops immediately. Specifically, update the block that creates AdbServerClient, calls client.createAdb and adb.reverse.addExternal (referenced symbols: AdbServerClient, createAdb, reverse.addExternal, devPort) in the beforeAll so any exception from reverse.addExternal results in throwing an Error (including context like the devPort and original error) instead of just console.warn.
🤖 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/tests/lynx.spec.ts`:
- Around line 16-17: Replace the two `any` declarations by giving `devServer`
and `adb` explicit types and initializing to undefined; e.g., change `let
devServer: any` to `let devServer: ViteDevServer | undefined` and `let adb:
AndroidDebugBridge | undefined` (or the specific types from the libraries you
use), and add the corresponding type-only imports (for example `import type {
ViteDevServer } from 'vite'` and the adb client type from your adb library) so
the variables remain conditionally assigned but satisfy strict TypeScript mode.
---
Duplicate comments:
In `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts`:
- Around line 57-71: The try/catch around the AdbServerClient setup swallows
failures by calling console.warn, letting beforeAll continue; change the catch
to fail fast by rethrowing or throwing a new error so the test setup stops
immediately. Specifically, update the block that creates AdbServerClient, calls
client.createAdb and adb.reverse.addExternal (referenced symbols:
AdbServerClient, createAdb, reverse.addExternal, devPort) in the beforeAll so
any exception from reverse.addExternal results in throwing an Error (including
context like the devPort and original error) instead of just console.warn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5649d7e9-d14b-4f14-bc1b-5703f9b103eb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/testing-library/kitten-lynx/package.jsonpackages/testing-library/kitten-lynx/test-fixture/lynx.config.jspackages/testing-library/kitten-lynx/tests/lynx.spec.ts
✅ Files skipped from review due to trivial changes (2)
- packages/testing-library/kitten-lynx/test-fixture/lynx.config.js
- packages/testing-library/kitten-lynx/package.json
Contributor
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 `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts`:
- Around line 38-44: The filename check is wrong because config.output.filename
is a Filename object (with a bundle property) after normalization; update the
logic that computes bundlePath in the test to accept both a string filename or a
Filename object by checking for filename.bundle (and falling back to filename
when it's a string), e.g. use the bundle template from filename.bundle when
present, otherwise use filename string, then replace [name] and [platform] as
before using entryName and 'lynx'; update references to filename, name, and
bundlePath accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af8c1713-dbec-46c2-8511-729fde2cd3fc
📒 Files selected for processing (1)
packages/testing-library/kitten-lynx/tests/lynx.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/testing-library/kitten-lynx/tests/lynx.spec.ts (2)
47-52: Remove or uncomment the reverse cleanup.Line 48 has commented-out code for removing the reverse forwarding. This leaves stale port mappings on the device between test runs. Either:
- Uncomment it to properly clean up, or
- Remove the comment if cleanup is intentionally skipped (with an explanation why)
♻️ Proposed cleanup
try { - // await adb.reverse.remove(`tcp:3001`); + await adb.reverse.remove(`tcp:3001`); await adb.close(); } catch (e) { // Ignore }🤖 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 47 - 52, The try/catch in the test leaves adb.reverse.remove commented out which can leave stale port forwards; restore proper cleanup by uncommenting and awaiting adb.reverse.remove(`tcp:3001`) before calling await adb.close() in the teardown block (or explicitly remove the commented call and add a short comment explaining why reverse cleanup is intentionally skipped), referencing the adb.reverse.remove and adb.close calls so the teardown reliably removes the reverse forwarding between test runs.
13-23: Race condition: Dev server starts at module load without readiness check.The
execacall executes at module scope, meaning the dev server starts immediately when the module is imported—beforebeforeAllruns. There's no mechanism to ensure the server is ready before the test navigates to it. While the 15sgototimeout may absorb this, it's fragile and could cause intermittent failures in slower CI environments.Additionally, the server process handle is not captured, so explicit graceful shutdown isn't possible—you're relying on
cleanup: trueto terminate the process when the parent exits.♻️ Recommended refactor: Start server in beforeAll and wait for readiness
-import { execa } from 'execa'; -execa({ - env: { - ...process.env, - NODE_ENV: 'development', - }, - cwd, - stdio: 'inherit', - shell: true, - cleanup: true, -})`pnpm serve`; -// Using Rspeedy Node API resolving instead of child_process +import { execa, type ResultPromise } from 'execa'; + +let serverProcess: ResultPromise | undefined;Then in
beforeAll:beforeAll(async () => { // Start dev server serverProcess = execa({ env: { ...process.env, NODE_ENV: 'development' }, cwd, stdio: 'inherit', shell: true, })`pnpm serve`; // Wait for server to be ready (simple retry loop) const maxAttempts = 30; for (let i = 0; i < maxAttempts; i++) { try { await fetch('http://127.0.0.1:3001/'); break; } catch { if (i === maxAttempts - 1) throw new Error('Dev server failed to start'); await new Promise(r => setTimeout(r, 1000)); } } // ... rest of beforeAll });And in
afterAll:afterAll(async () => { // ... existing cleanup serverProcess?.kill(); });Based on learnings: Be mindful of device timeouts. CDP requests time out after 5000ms. Keep connection tests tolerant of emulator boot/warm-up times gracefully in test suites.
🤖 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 13 - 23, The execa call at module scope starts the dev server immediately and doesn't provide a handle or readiness check; move the execa invocation into beforeAll and assign its return to a serverProcess variable (the current execa(...)`pnpm serve` use), implement a simple retry loop (e.g., repeated fetch to the server URL with delays and a max attempt) to wait for readiness before running tests, and in afterAll ensure you gracefully shutdown by calling serverProcess.kill() (and keep removing cleanup: true from the execa options if you rely on explicit kill); also increase any short CDP/connect timeouts used in your tests to tolerate emulator warm-up where applicable.
🤖 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/tests/lynx.spec.ts`:
- Around line 38-41: The loop over devices overwrites the single adb variable so
only the last device connection is retained; update the test in lynx.spec.ts to
either enforce a single-device run (assert devices.length === 1 or pick
devices[0] and call client.createAdb once) OR change the code to track all
connections by creating an array (e.g., adbs[]) and pushing each
client.createAdb(...) result, call adb.reverse.addExternal for each, and then in
afterAll iterate adbs to close each connection; references to fix: the devices
loop, the adb variable, client.createAdb, and the afterAll teardown that
currently closes adb.
---
Nitpick comments:
In `@packages/testing-library/kitten-lynx/tests/lynx.spec.ts`:
- Around line 47-52: The try/catch in the test leaves adb.reverse.remove
commented out which can leave stale port forwards; restore proper cleanup by
uncommenting and awaiting adb.reverse.remove(`tcp:3001`) before calling await
adb.close() in the teardown block (or explicitly remove the commented call and
add a short comment explaining why reverse cleanup is intentionally skipped),
referencing the adb.reverse.remove and adb.close calls so the teardown reliably
removes the reverse forwarding between test runs.
- Around line 13-23: The execa call at module scope starts the dev server
immediately and doesn't provide a handle or readiness check; move the execa
invocation into beforeAll and assign its return to a serverProcess variable (the
current execa(...)`pnpm serve` use), implement a simple retry loop (e.g.,
repeated fetch to the server URL with delays and a max attempt) to wait for
readiness before running tests, and in afterAll ensure you gracefully shutdown
by calling serverProcess.kill() (and keep removing cleanup: true from the execa
options if you rely on explicit kill); also increase any short CDP/connect
timeouts used in your tests to tolerate emulator warm-up where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0560bb73-e354-45b1-bf7c-467bc3850d62
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/testing-library/kitten-lynx/lynx.config.jspackages/testing-library/kitten-lynx/package.jsonpackages/testing-library/kitten-lynx/tests/lynx.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/testing-library/kitten-lynx/package.json
colinaaa
reviewed
Mar 20, 2026
PupilTong
force-pushed
the
p/hw/kitten-test-use-local-case
branch
from
703a090 to
5f4ce0f
Compare
colinaaa
approved these changes
Mar 20, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… and enhance
KittenLynxViewnavigation with timeout and DOM enable.Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
Checklist