feat: add mobilewright inspect command with browser-based element inspector#213
Open
marcomaes wants to merge 1 commit into
Open
feat: add mobilewright inspect command with browser-based element inspector#213marcomaes wants to merge 1 commit into
marcomaes wants to merge 1 commit into
Conversation
WalkthroughAdds a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
packages/inspector/src/lib/device-manager.test.ts (1)
135-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test assert that the first device was actually closed.
Right now it only proves that the second device became active. The test still passes if
await this.#activeDevice.close()disappears, so it does not cover the behavior named in the test.Suggested change
test('closes previous device before connecting new one', async () => { + let firstClosed = false + const first = fakeDevice({ close: async () => { firstClosed = true } }) const second = fakeDevice() const dm = new DeviceManager({ ios: { devices: async () => [], launch: async (opts: { deviceId: string }) => - (opts.deviceId === 'sim-2' ? second : fakeDevice()) as unknown as Device, + (opts.deviceId === 'sim-2' ? second : first) as unknown as Device, }, android: { devices: async () => [], launch: async () => { throw new Error() } }, }) await dm.select('sim-1', 'ios') await dm.select('sim-2', 'ios') + expect(firstClosed).toBe(true) expect(dm.device).toBe(second) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/src/lib/device-manager.test.ts` around lines 135 - 148, Update the DeviceManager test so it verifies the previous device is actually closed, not just that the new one becomes active. In the "closes previous device before connecting new one" test in DeviceManager.select, make the first fake device a tracked mock/spied Device with a close method assertion, then after the second select assert that the first device’s close was called while still checking the active device is the second one. Use the existing fakeDevice helper and the DeviceManager.device/select flow to locate the right spot.packages/inspector/src/routes.test.ts (1)
138-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a wrong-platform regression case for
POST /api/devices/:id/select.The suite checks invalid platforms and unknown ids, but not the case where the id exists on one platform and the request specifies the other. A dedicated test here would lock in the 404 behavior once the route matches on both fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/src/routes.test.ts` around lines 138 - 157, Add a regression test in routes.test.ts for POST /api/devices/:id/select that covers an existing device id requested with the wrong platform, and assert it returns 404. Use the existing post helper and mirror the current select route tests so the case is verified alongside the platform validation and unknown id checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/css/app.css`:
- Line 412: The font-family declaration is using quote styles that violate
stylelint’s font-family-name-quotes rule. Update the font-family in the
stylesheet to use the quote style expected by the linter for the named fonts in
this declaration, keeping the monospace fallback unchanged, and adjust the
app.css rule containing the SF Mono / Menlo / Monaco stack accordingly.
In `@packages/inspector/public/index.html`:
- Around line 11-28: The header selects `#device-select` and
`#auto-refresh-interval` are missing accessible names, so update the markup in the
header to associate each select with a clear label or an
aria-label/aria-labelledby using the existing device-picker and
auto-refresh-controls structure. Make sure the device selector and refresh
interval control each have distinct, descriptive names that assistive
technologies can announce.
In `@packages/inspector/public/js/app.js`:
- Around line 202-206: The visibility toggle in updateRowVisibility() updates
the button glyph, title, and aria-pressed, but leaves aria-label stale, so
screen readers announce the wrong action after toggles. Update the .vis-btn
handling in updateRowVisibility() to also set aria-label to match the current
hidden state, keeping it in sync with the existing textContent and title values.
- Around line 377-383: Clear stale inspect state when `#fetchDevices`() receives
no active device. In the app.js device refresh flow, detect when
this.#state.activeId becomes null after fetching /api/devices, and explicitly
reset the current inspect data so the old screenshot and locator list do not
remain visible. Update the `#fetchDevices` method and any related state/render
helpers in the inspector app to clear the selected device/inspect view before
calling `#renderDevicePicker`().
In `@packages/inspector/src/lib/device-manager.ts`:
- Around line 68-73: The `DeviceManager` is clearing `#activeDevice` and
`#activeDeviceInfo` even when `close()` rejects, which can leave a live session
orphaned while allowing `select()` to start another one. Update the cleanup
logic in both the previous-device block and `DeviceManager.close()` so the state
is only nulled after a successful `await this.#activeDevice.close()`, and
preserve the existing active-device references when `close()` fails so callers
can retry or handle the error.
In `@packages/inspector/src/lib/logger.ts`:
- Around line 1-9: The new TypeScript files in this package violate the enforced
semicolon style, and the logger.ts snippet shows the same formatting pattern.
Run the formatter/linter fix on logger.ts and the other new TS files so the
exported logger methods and related code conform to `@stylistic/semi` and CI
passes.
In `@packages/inspector/src/routes/devices.ts`:
- Around line 28-33: The precheck in the devices route only matches on id, but
listDevices() returns platform-specific entries, so update the existence check
in the route handler around deviceManager.listDevices() / deviceManager.select()
to require both id and platform before proceeding. If no entry matches both
fields, return the same 404 response instead of calling deviceManager.select(),
so the wrong platform for an existing device id does not fall through to a 500.
In `@packages/inspector/src/routes/inspect.ts`:
- Around line 35-41: The iOS scaling logic in inspect() is guessing logical size
from screenshot width bands, which breaks iPad overlays when it falls back to
scale 1. Update the platform handling in inspect.ts to use Device.screenSize()
for iOS logical dimensions instead of the current 300-520pt heuristic, so
screen.width/height stay aligned with the view tree and box highlights. Keep the
existing Android path unchanged and make the scale/screen size calculation
consistent with the inspect route’s returned metadata.
- Around line 29-30: The inspect retry flow in
attemptWithRetry/getScreenshotTree is leaving timed-out device work running in
the background, so the next retry can overlap on the same shared device. Update
the inspect route logic around attemptWithRetry, withTimeout, screenshot(),
viewTree(), and endInspect() so a timed-out attempt is actually canceled or
prevented from continuing before retrying, and ensure the manager lock is not
cleared until all in-flight driver calls from that attempt have fully settled.
In `@packages/inspector/tsconfig.json`:
- Around line 7-11: The inspector tsconfig currently includes all of src, which
causes test files like the *.test.ts specs to be compiled into the build and
then published from dist. Update the package’s TypeScript config so the build
only picks up production sources for the inspector package, excluding test-only
files while preserving the existing references to protocol and
mobilewright-core. Use the tsconfig include/exclude setup in this package to
narrow what tsc -b emits, and verify the generated dist no longer contains the
test artifacts.
In `@packages/mobilewright/src/cli.ts`:
- Around line 311-331: The new inspector action in cli.ts is missing required
semicolons and will fail the repo’s `@stylistic/semi` lint rule. Update the
statements in the inspector startup/shutdown flow around start, open, console
output, shutdown, and the SIGINT/SIGTERM handlers in the cli action so they
follow the existing semicolon style used elsewhere in the file.
- Around line 321-331: The `mobilewright inspect` flow in `cli.ts` currently
awaits `open(inspector.url)` directly, so a launch failure can abort startup
before the `SIGINT`/`SIGTERM` handlers in `shutdown()` are registered. Wrap the
`open()` call in `try/catch` inside the `start`/`inspector` setup path, log or
ignore the failure as appropriate, and continue so the server stays running and
controllable even when no browser/app can be launched.
---
Nitpick comments:
In `@packages/inspector/src/lib/device-manager.test.ts`:
- Around line 135-148: Update the DeviceManager test so it verifies the previous
device is actually closed, not just that the new one becomes active. In the
"closes previous device before connecting new one" test in DeviceManager.select,
make the first fake device a tracked mock/spied Device with a close method
assertion, then after the second select assert that the first device’s close was
called while still checking the active device is the second one. Use the
existing fakeDevice helper and the DeviceManager.device/select flow to locate
the right spot.
In `@packages/inspector/src/routes.test.ts`:
- Around line 138-157: Add a regression test in routes.test.ts for POST
/api/devices/:id/select that covers an existing device id requested with the
wrong platform, and assert it returns 404. Use the existing post helper and
mirror the current select route tests so the case is verified alongside the
platform validation and unknown id checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fbad740-3ab8-4243-8667-51917a1bc9bb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
marcomaes
force-pushed
the
feat/inspector
branch
from
3882020 to
9ec743d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/inspector/src/routes.test.ts (1)
16-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the test HTTP helper.
If a route hangs, this promise waits until the whole Playwright test timeout. A small request timeout makes failures faster and easier to diagnose.
Suggested fix
}); req.on('error', reject); + req.setTimeout(5000, () => { + req.destroy(new Error(`${method} ${parsed.pathname} timed out`)); + }); if (payload) req.write(payload);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/src/routes.test.ts` around lines 16 - 39, The test HTTP helper in request should enforce a short timeout so hung routes fail quickly instead of waiting for the full Playwright timeout. Update the request() implementation to set a timeout on the http.request instance and reject/abort with a clear error when it expires, keeping the existing resolve/reject behavior intact for normal responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 281-283: The user visibility override storage in the inspector app
is keyed only by locatorKey, so duplicate rows share the same persisted state
and all get toggled together. Update the override handling in app.js, especially
the `#userOverrides` map and the refresh/application logic around the element
list, to persist only when a locator key is unique or to add a stable
per-element disambiguator for duplicates. Ensure the code that records and
reapplies overrides distinguishes duplicate elements instead of treating every
row with the same locatorKey as one entry.
- Around line 420-438: The `#connectDevice`() flow leaves the screenshot pane
stuck in the loading state on failure because the catch path only updates the
status bar. In the `#connectDevice`(device) method, clear or reset the
`#screenshotPane` placeholder when the fetch/select request fails, alongside the
existing error status update, so the pane no longer shows “Connecting...” after
a failed connection.
- Around line 480-490: Guard the auto-refresh loop in `#applyAutoRefresh` so a new
tick cannot start while a previous async refresh is still running. Add an
in-flight flag around the setInterval callback that wraps `#fetchDevices` and
refresh, skip the body when a refresh is already active, and clear the flag in a
finally block so overlapping ticks cannot race or leave stale UI state.
In `@packages/inspector/src/index.ts`:
- Around line 47-53: Bind the inspector server to loopback only in the startup
logic around server.listen and the assignedPort/url setup in index.ts. Update
the listen call so it explicitly binds to the localhost loopback interface
rather than all interfaces, and keep the returned URL consistent with that
binding. Use the existing server and assignedPort flow to ensure the inspector
remains reachable only via localhost.
---
Nitpick comments:
In `@packages/inspector/src/routes.test.ts`:
- Around line 16-39: The test HTTP helper in request should enforce a short
timeout so hung routes fail quickly instead of waiting for the full Playwright
timeout. Update the request() implementation to set a timeout on the
http.request instance and reject/abort with a clear error when it expires,
keeping the existing resolve/reject behavior intact for normal responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: febfb87a-bb16-4497-977b-1910ee7762c1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (5)
- packages/mobilewright/tsconfig.json
- README.md
- packages/mobilewright/package.json
- tsconfig.json
- packages/inspector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/inspector/package.json
- packages/inspector/src/lib/logger.ts
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/public/index.html
- packages/inspector/src/lib/device-manager.test.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/src/lib/device-manager.ts
- packages/inspector/public/css/app.css
marcomaes
force-pushed
the
feat/inspector
branch
2 times, most recently
from
6401c95 to
a2bc11c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 258-263: The row keyboard shortcut currently reacts to bubbled
Enter/Space events from the child visibility button, causing accidental row
selection. Update the keydown handler on the row to only trigger when the event
target is the row itself, and keep the existing `#onClickCb`(el.index) behavior
for direct row keyboard interaction. Use the row.addEventListener('keydown',
...) handler and the el.index callback path to locate and adjust the guard.
In `@packages/inspector/src/lib/device-manager.ts`:
- Around line 73-98: The `DeviceManager.select()` flow can still finish
`launcher.launch(...)` after `close()` has started, which may leave a newly
assigned active device after shutdown. Add a closing/closed guard in
`DeviceManager` and check it in `select()` before and after
`launcher.launch(...)`; if shutdown has begun, abort or immediately close the
launched device and avoid setting `#activeDevice`/`#activeDeviceInfo`. Also
update `close()` to mark the manager as closing/closed and ensure any in-flight
`select()` uses that state to skip cleanup-safe assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a134331-59a2-4deb-9679-6f183b566f66
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (3)
- tsconfig.json
- README.md
- packages/inspector/public/index.html
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/inspector/tsconfig.json
- packages/inspector/package.json
- packages/mobilewright/tsconfig.json
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/logger.ts
- packages/mobilewright/src/cli.ts
- packages/mobilewright/package.json
- packages/inspector/src/routes/inspect.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/public/css/app.css
- packages/inspector/src/lib/locator-derivation.test.ts
marcomaes
force-pushed
the
feat/inspector
branch
2 times, most recently
from
0dbf0ba to
40826bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 441-444: The failed-connect path in app.js leaves the device
picker out of sync because the select element still shows the attempted device
while `#state.activeId` remains unchanged. In the connect flow around the
catch/finally handling, restore the picker selection back to the current active
device when the POST fails, using the existing state managed by `#state.activeId`
and the picker update logic so the UI matches the real device and reselecting
the same option can trigger change again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af4db7f9-4482-4587-a414-51b6abe7e206
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (3)
- packages/inspector/public/index.html
- README.md
- packages/inspector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/inspector/package.json
- packages/mobilewright/src/cli.ts
- packages/inspector/public/css/app.css
- tsconfig.json
- packages/inspector/src/lib/logger.ts
- packages/mobilewright/package.json
- packages/mobilewright/tsconfig.json
- packages/inspector/src/routes/inspect.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/lib/device-manager.ts
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/locator-derivation.test.ts
marcomaes
force-pushed
the
feat/inspector
branch
2 times, most recently
from
98f884b to
bfece5d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/inspector/src/index.ts (1)
56-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn the same loopback address that the server binds to.
Line 56 listens only on
127.0.0.1, but Line 60 still returnshttp://localhost:.... On hosts wherelocalhostresolves to::1first, the CLI can open a URL the server is not actually listening on.Suggested fix
- const url = `http://localhost:${assignedPort}`; + const url = `http://127.0.0.1:${assignedPort}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/src/index.ts` around lines 56 - 60, The URL returned from the inspector startup should use the same loopback address that server.listen binds to, instead of hardcoding localhost. Update the logic in the startup path around server.listen and the assignedPort/url निर्माण so the returned URL uses 127.0.0.1 consistently, preventing mismatches when localhost resolves to ::1 first.packages/inspector/public/js/app.js (2)
475-483: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIgnore saved overrides once a locator key becomes duplicated.
#toggleHidden()now skips writing new duplicate keys, but Lines 479-480 still read any older override for that key. If a locator was unique when the user hid it and becomes duplicated on a later refresh, every duplicate row will inherit that stale override again.Suggested fix
`#computeHiddenIndices`() { + const dupes = buildDuplicateSet(this.#state.elements) return new Set( this.#state.elements .filter(el => { const key = el.locator ? locatorKey(el.locator) : null - const override = key ? this.#userOverrides.get(key) : undefined + const override = key && !dupes.has(key) ? this.#userOverrides.get(key) : undefined if (override === 'visible') return false if (override === 'hidden') return true return el.locator?.kind === 'testId' && DEFAULT_HIDDEN_TESTIDS.has(el.locator.value) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/public/js/app.js` around lines 475 - 483, The hidden-state lookup in `#computeHiddenIndices` is still applying stale user overrides for locator keys that are now duplicated. Update the locatorKey-based override handling in this method so it only honors overrides for keys that are still unique in the current `#state.elements` set, and ignore any saved override when the same key appears more than once. Keep the existing DEFAULT_HIDDEN_TESTIDS fallback and the override checks aligned with `#toggleHidden`’s duplicate-key behavior.
456-462: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLook up the toggled element by
el.index, not by array offset.The rest of this file treats
indexas the element identifier, but Line 457 readsthis.#state.elements[index]as if it were the current array position. If those ever diverge, the toggle will compute the locator key/dup state from the wrong element and the override will not survive the next refresh.Suggested fix
`#toggleHidden`(index) { - const el = this.#state.elements[index] + const el = this.#state.elements.find(el => el.index === index) const key = el?.locator ? locatorKey(el.locator) : null const nowHidden = this.#state.hiddenIndices.has(index) // Only persist overrides for unique locator keys — shared keys would affect all duplicates. const isDupe = key ? buildDuplicateSet(this.#state.elements).has(key) : false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/public/js/app.js` around lines 456 - 462, In `#toggleHidden`, the element should be resolved by its stable identifier rather than treating index as an array offset. Update the lookup around this method to find the matching element by el.index in this.#state.elements before computing locatorKey, nowHidden, and the duplicate-set check, so the toggle operates on the same element identity used elsewhere in the file and survives refreshes correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/inspector/public/js/app.js`:
- Around line 475-483: The hidden-state lookup in `#computeHiddenIndices` is still
applying stale user overrides for locator keys that are now duplicated. Update
the locatorKey-based override handling in this method so it only honors
overrides for keys that are still unique in the current `#state.elements` set, and
ignore any saved override when the same key appears more than once. Keep the
existing DEFAULT_HIDDEN_TESTIDS fallback and the override checks aligned with
`#toggleHidden`’s duplicate-key behavior.
- Around line 456-462: In `#toggleHidden`, the element should be resolved by its
stable identifier rather than treating index as an array offset. Update the
lookup around this method to find the matching element by el.index in
this.#state.elements before computing locatorKey, nowHidden, and the
duplicate-set check, so the toggle operates on the same element identity used
elsewhere in the file and survives refreshes correctly.
In `@packages/inspector/src/index.ts`:
- Around line 56-60: The URL returned from the inspector startup should use the
same loopback address that server.listen binds to, instead of hardcoding
localhost. Update the logic in the startup path around server.listen and the
assignedPort/url निर्माण so the returned URL uses 127.0.0.1 consistently,
preventing mismatches when localhost resolves to ::1 first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aac6d8a6-eb81-4c72-93c6-e7935f26a093
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (5)
- packages/inspector/tsconfig.json
- tsconfig.json
- README.md
- packages/inspector/package.json
- packages/mobilewright/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/inspector/src/lib/logger.ts
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/public/index.html
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/mobilewright/package.json
- packages/inspector/src/routes/inspect.ts
- packages/inspector/src/lib/device-manager.ts
- packages/inspector/public/css/app.css
marcomaes
force-pushed
the
feat/inspector
branch
2 times, most recently
from
3ef9869 to
c3473b6
Compare
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 282-284: The visibility override state in InspectorApp is
currently shared across devices because `#userOverrides` is keyed only by locator
and reused by refresh/prune logic in the inspect/update flow. Update the
override storage and lookup in InspectorApp (including the map definition and
the code paths that read/write it, such as the refresh pruning and toggle
handlers) so overrides are scoped by activeId, or clear `#userOverrides` whenever
the selected device changes. Make sure the pruning logic only retains entries
for the current device’s activeId and locator combination.
- Around line 12-24: The locator snippet builder in locatorLabel() currently
produces invalid single-quoted JavaScript when locator values contain line
breaks or Unicode separators because escQ() only escapes backslashes and
apostrophes. Update escQ() to also escape \r, \n, \u2028, and \u2029 so all
callers in locatorLabel() (testId, role, label, text) generate valid inspector
snippets.
In `@packages/inspector/src/index.ts`:
- Around line 63-69: The close() shutdown sequence in inspector/src/index.ts
needs to stop accepting new connections before forcefully dropping existing
ones. Update the close() function so server.close() is called and awaited before
server.closeAllConnections(), keeping the existing deviceManager.close()
handling intact and preserving the same close() symbol for easy location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da1b73f7-df35-4640-8e17-308d5dc10ca5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (5)
- packages/inspector/tsconfig.json
- tsconfig.json
- packages/mobilewright/tsconfig.json
- README.md
- packages/inspector/public/index.html
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/mobilewright/package.json
- packages/inspector/package.json
- packages/inspector/src/lib/logger.ts
- packages/inspector/public/css/app.css
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/device-manager.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/routes/inspect.ts
marcomaes
force-pushed
the
feat/inspector
branch
from
c3473b6 to
4925db4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/inspector/src/routes.test.ts (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the production error middleware in
buildApp().
start()installs a JSON 500 handler, but this test app does not. A thrown route error here will exercise Express's default HTML error page instead of the response contract the real server ships.Proposed fix
function buildApp(dm: DeviceManager) { const app = express(); app.use(express.json()); app.get('/health', (_req, res) => res.json({ ok: true })); app.use('/api/devices', createDevicesRouter(dm)); app.use('/api', createInspectRouter(dm)); + app.use((err: Error, _req: express.Request, res: express.Response, _next: express.NextFunction) => { + res.status(500).json({ error: err.message }); + }); return app; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/src/routes.test.ts` around lines 49 - 55, Mirror the production error handling in buildApp() so this test app matches the real server’s response contract. The app currently sets up routes in buildApp() but never installs the JSON 500 error middleware that start() uses, so thrown errors fall back to Express’s default HTML handler. Add the same final error-handling middleware after createDevicesRouter() and createInspectRouter() in buildApp(), using the existing production middleware pattern so route failures return JSON here too.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 18-19: The regex literals in the app.js string replacement chain
use raw U+2028/U+2029 characters, which breaks parsing. Update the replacement
expressions in the inspector app bootstrap to use escaped Unicode code points
instead, keeping the existing replace calls but changing the pattern literals so
the file can load correctly.
---
Nitpick comments:
In `@packages/inspector/src/routes.test.ts`:
- Around line 49-55: Mirror the production error handling in buildApp() so this
test app matches the real server’s response contract. The app currently sets up
routes in buildApp() but never installs the JSON 500 error middleware that
start() uses, so thrown errors fall back to Express’s default HTML handler. Add
the same final error-handling middleware after createDevicesRouter() and
createInspectRouter() in buildApp(), using the existing production middleware
pattern so route failures return JSON here too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d62d4cb-88f1-443f-af6f-0beb7e5b897c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (3)
- packages/inspector/tsconfig.json
- packages/mobilewright/package.json
- README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/inspector/package.json
- packages/inspector/src/lib/locator-derivation.ts
- packages/mobilewright/tsconfig.json
- packages/mobilewright/src/cli.ts
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/logger.ts
- packages/inspector/src/routes/inspect.ts
- packages/inspector/public/index.html
- packages/inspector/public/css/app.css
- tsconfig.json
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/src/lib/device-manager.ts
marcomaes
force-pushed
the
feat/inspector
branch
from
4925db4 to
4d9c880
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 309-315: The device selection flow can start multiple overlapping
calls to `#connectDevice`(), allowing later selections to race with earlier POSTs
and overwrite `#state.activeId` incorrectly. Update the device connect path in
`#connectDevice` and the change handler on `#deviceSelect` to serialize or ignore
stale in-flight connections so only the latest selection can complete and
trigger refresh logic. Use the existing `#connectDevice`, `#state.activeId`, and
device change listener as the key spots to guard against out-of-order responses.
In `@packages/inspector/src/index.ts`:
- Around line 63-71: The close() shutdown flow in index.ts keeps the HTTP server
accepting requests while deviceManager.close() is draining, which allows
/api/inspect and /api/devices/:id/select to race teardown. Update the close()
sequence so server.close() (and server.closeAllConnections()) runs before
waiting on deviceManager.close(), using the close() helper and
deviceManager.close() symbols to keep the shutdown order deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 582608f5-912c-4999-b7b0-55be4ff4a731
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (2)
- README.md
- packages/inspector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/inspector/package.json
- tsconfig.json
- packages/mobilewright/tsconfig.json
- packages/inspector/src/routes/devices.ts
- packages/mobilewright/package.json
- packages/mobilewright/src/cli.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/public/index.html
- packages/inspector/public/css/app.css
- packages/inspector/src/routes/inspect.ts
- packages/inspector/src/lib/device-manager.ts
- packages/inspector/src/lib/logger.ts
- packages/inspector/src/lib/locator-derivation.test.ts
marcomaes
force-pushed
the
feat/inspector
branch
from
4d9c880 to
ed79260
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 311-316: Disable the device picker while a connect request is in
flight so the UI can’t drift from the actual active device. Update the change
handling around `#deviceSelect` and `#connectDevice` to set the select disabled
before starting the request and re-enable it when the request finishes, while
still honoring `#connectInFlight`. Ensure the picker state stays aligned with
`#state.activeId` after a successful connect so a second selection cannot be
silently dropped and leave the wrong device shown.
- Around line 333-366: The refresh flow in the inspector UI does not clear stale
data when /api/inspect returns 409, so the previous screenshot and element list
can remain visible after a disconnect. Update the fetch handling in the app’s
inspect refresh method to detect the 409 “no device connected” response and
reset the last snapshot state, including elements, hidden indices, highlights,
and screenshot display, before setting the disconnected status. Use the existing
state and rendering symbols in this path such as this.#state,
this.#screenshotPane, this.#elementsPane, and this.#setStatus so the UI no
longer looks current after a disconnect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cba47de2-a798-48e0-b60f-da621a0e4d15
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (3)
- packages/inspector/tsconfig.json
- README.md
- packages/inspector/public/index.html
🚧 Files skipped from review as they are similar to previous changes (13)
- tsconfig.json
- packages/mobilewright/tsconfig.json
- packages/mobilewright/package.json
- packages/inspector/src/lib/logger.ts
- packages/inspector/src/routes/inspect.ts
- packages/inspector/src/routes/devices.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/public/css/app.css
- packages/inspector/package.json
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/lib/device-manager.ts
- packages/inspector/src/lib/locator-derivation.test.ts
marcomaes
force-pushed
the
feat/inspector
branch
from
ed79260 to
ceb1f8c
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/inspector/public/js/app.js (2)
291-293: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLock the whole auto-refresh tick, not just
refresh().
#refreshInFlightonly serializes/api/inspect. Overlapping intervals can still issue concurrent/api/devicesrequests, and an older response can rewind#state.activeIdafter a newer selection has already won.Suggested fix
`#autoRefreshTimer` = null + `#autoRefreshTickInFlight` = false `#refreshInFlight` = false `#connectInFlight` = false @@ `#applyAutoRefresh`() { clearInterval(this.#autoRefreshTimer) this.#autoRefreshTimer = null + this.#autoRefreshTickInFlight = false this.#autoRefreshInterval.disabled = !this.#autoRefreshToggle.checked this.#consecutiveErrors = 0 // reset when user manually reconfigures auto-refresh if (this.#autoRefreshToggle.checked) { const ms = Number(this.#autoRefreshInterval.value) this.#autoRefreshTimer = setInterval(async () => { - await this.#fetchDevices().catch(() => {}) - await this.refresh() + if (this.#autoRefreshTickInFlight) return + this.#autoRefreshTickInFlight = true + try { + await this.#fetchDevices().catch(() => {}) + await this.refresh() + } finally { + this.#autoRefreshTickInFlight = false + } }, ms) } }Also applies to: 516-526
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/public/js/app.js` around lines 291 - 293, The auto-refresh flow in the inspector app needs a single lock around the entire tick, not only around `refresh()`. Update the `#autoRefreshTimer`-driven logic and the related `refresh()`/devices fetch path so overlapping intervals cannot start concurrent `/api/devices` requests or let an older response overwrite `#state.activeId`. Use a whole-tick guard in the `app.js` auto-refresh code to skip a new tick while the previous one is still running, and keep the state update tied to the latest successful refresh in the existing `#refreshInFlight`-related flow.
406-409: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear visibility overrides when
/api/devicesswitches the active device.
#userOverridesis only reset in#connectDevice(). If another inspector tab/process changes the shared active device,#fetchDevices()updates#state.activeIdbut reuses the previous device's locator overrides on the next refresh.Suggested fix
async `#fetchDevices`() { const res = await fetch('/api/devices') if (!res.ok) throw new Error(`Device list failed: ${res.status}`) const data = await res.json() const prevActiveId = this.#state.activeId this.#state.devices = data.devices ?? [] this.#state.activeId = data.activeId ?? null + if (prevActiveId !== this.#state.activeId) { + this.#userOverrides.clear() + } this.#renderDevicePicker() if (prevActiveId && !this.#state.activeId) {Also applies to: 461-463
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/public/js/app.js` around lines 406 - 409, The device refresh path in `#fetchDevices`() and the active-device switch path around `#connectDevice`() leave stale `#userOverrides` in place when `#state.activeId` changes externally. Update the state change handling so that when the active device returned by /api/devices differs from the previous activeId, the inspector clears `#userOverrides` before the next render/refresh, using the existing `#state.activeId`, `#fetchDevices`(), and `#renderDevicePicker`() flow to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 420-441: Render a placeholder option in `#renderDevicePicker` when
`#state.activeId` is null so the first real device is not auto-selected; this is
causing the picker to appear connected and suppressing change events for
retrying the same device. Update the device-select rendering logic to add a
disabled/placeholder option for the no-active-device state before appending the
entries, and ensure the selected value only maps to a real device when
`#state.activeId` is set.
---
Duplicate comments:
In `@packages/inspector/public/js/app.js`:
- Around line 291-293: The auto-refresh flow in the inspector app needs a single
lock around the entire tick, not only around `refresh()`. Update the
`#autoRefreshTimer`-driven logic and the related `refresh()`/devices fetch path
so overlapping intervals cannot start concurrent `/api/devices` requests or let
an older response overwrite `#state.activeId`. Use a whole-tick guard in the
`app.js` auto-refresh code to skip a new tick while the previous one is still
running, and keep the state update tied to the latest successful refresh in the
existing `#refreshInFlight`-related flow.
- Around line 406-409: The device refresh path in `#fetchDevices`() and the
active-device switch path around `#connectDevice`() leave stale `#userOverrides` in
place when `#state.activeId` changes externally. Update the state change handling
so that when the active device returned by /api/devices differs from the
previous activeId, the inspector clears `#userOverrides` before the next
render/refresh, using the existing `#state.activeId`, `#fetchDevices`(), and
`#renderDevicePicker`() flow to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3b9dcca-1936-44bc-b729-b8c5a47c1555
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (2)
- README.md
- packages/mobilewright/package.json
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/inspector/tsconfig.json
- packages/mobilewright/tsconfig.json
- packages/inspector/src/routes/devices.ts
- packages/mobilewright/src/cli.ts
- tsconfig.json
- packages/inspector/src/lib/logger.ts
- packages/inspector/public/css/app.css
- packages/inspector/src/routes/inspect.ts
- packages/inspector/public/index.html
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/package.json
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/src/lib/device-manager.ts
marcomaes
force-pushed
the
feat/inspector
branch
from
ceb1f8c to
1eeaa40
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 389-396: Stop auto-selecting the first device in `#loadDevices`. The
current branch in `#loadDevices`() immediately calls
`#connectDevice`(this.#state.devices[0]) when `#state.activeId` is unset, which
bypasses the new “Select a device” state and picks an arbitrary target. Update
the initial-load flow so `#fetchDevices`() can leave activeId as null, and only
call `#connectDevice` or refresh() when there is an explicit activeId; otherwise
let the UI stay unconnected until the user chooses a device.
- Around line 333-336: The refresh flow in app.js leaves the UI stuck in
“Loading…” when `/api/inspect` returns 503 because the early return skips
resetting state. Update `refresh()` so the 503 branch clears the loading
indicator before returning, using the same loading-state handling already used
elsewhere in the function (and keep the existing `fetch('/api/inspect')` and
status checks intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 704c890d-f97a-4940-b300-eec34e35ec5f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (2)
- packages/mobilewright/package.json
- README.md
🚧 Files skipped from review as they are similar to previous changes (14)
- tsconfig.json
- packages/inspector/package.json
- packages/inspector/src/lib/logger.ts
- packages/inspector/tsconfig.json
- packages/inspector/src/routes/devices.ts
- packages/mobilewright/tsconfig.json
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/routes/inspect.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/public/css/app.css
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/public/index.html
- packages/inspector/src/lib/device-manager.ts
marcomaes
force-pushed
the
feat/inspector
branch
from
1eeaa40 to
0814512
Compare
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Author
|
@coderabbitai approve |
✅ Action performedComments resolved. Approval is disabled; enable |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/inspector/public/js/app.js (2)
530-533: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe interval still allows overlapping device polls.
This guard only blocks the
refresh()part. A second tick can still enter while the first#fetchDevices()is awaiting, so device list andactiveIdupdates can race and briefly desync the picker/state.Suggested fix
`#autoRefreshTimer` = null + `#autoRefreshTickInFlight` = false `#refreshInFlight` = false @@ const ms = Number(this.#autoRefreshInterval.value) this.#autoRefreshTimer = setInterval(async () => { - if (this.#refreshInFlight) return - await this.#fetchDevices().catch(() => {}) - await this.refresh() + if (this.#autoRefreshTickInFlight) return + this.#autoRefreshTickInFlight = true + try { + await this.#fetchDevices().catch(() => {}) + await this.refresh() + } finally { + this.#autoRefreshTickInFlight = false + } }, ms) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/public/js/app.js` around lines 530 - 533, The auto-refresh loop in app.js still allows overlapping device polls because the `#refreshInFlight` guard is only checked before `#fetchDevices`() starts, so a second setInterval tick can enter while the first fetch is still awaiting. Update the auto-refresh flow around `#autoRefreshTimer`, `#fetchDevices`(), and refresh() so the in-flight guard covers the entire poll sequence (including the fetchDevices await), and ensure the flag is set/reset consistently to prevent concurrent device list and activeId updates.Source: Linters/SAST tools
333-335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReintroduces the 503 loading-state bug.
refresh()already swaps the pane into a loading placeholder before this fetch. This early return clears only the status text, so a skipped inspect can leave the UI stuck on “Loading screenshot…” when the screenshot was hidden.Suggested fix
const res = await fetch('/api/inspect') - if (res.status === 503) { this.#setStatus('', ''); return } // another inspect in flight, skip this tick + if (res.status === 503) { + this.#setStatus('Inspect already in progress') + if (this.#screenshotPane.isScreenshotHidden) { + this.#screenshotPane.showPlaceholder('Inspect already in progress', 'Retry in a moment') + } + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/public/js/app.js` around lines 333 - 335, The early return in refresh() after fetch('/api/inspect') is clearing only the status text, which can leave the pane stuck in the loading placeholder. Update the 503 handling in refresh() so it fully restores the prior UI state instead of just calling `#setStatus`('', ''), using the same state-reset path that runs when inspection is skipped or cancelled. Keep the fix localized to refresh() and its loading/state transition logic so the screenshot pane does not remain on “Loading screenshot…”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/inspector/public/js/app.js`:
- Around line 530-533: The auto-refresh loop in app.js still allows overlapping
device polls because the `#refreshInFlight` guard is only checked before
`#fetchDevices`() starts, so a second setInterval tick can enter while the first
fetch is still awaiting. Update the auto-refresh flow around `#autoRefreshTimer`,
`#fetchDevices`(), and refresh() so the in-flight guard covers the entire poll
sequence (including the fetchDevices await), and ensure the flag is set/reset
consistently to prevent concurrent device list and activeId updates.
- Around line 333-335: The early return in refresh() after fetch('/api/inspect')
is clearing only the status text, which can leave the pane stuck in the loading
placeholder. Update the 503 handling in refresh() so it fully restores the prior
UI state instead of just calling `#setStatus`('', ''), using the same state-reset
path that runs when inspection is skipped or cancelled. Keep the fix localized
to refresh() and its loading/state transition logic so the screenshot pane does
not remain on “Loading screenshot…”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a9f66cb-be07-462e-9618-aed42083af32
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (2)
- README.md
- packages/inspector/public/index.html
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/mobilewright/tsconfig.json
- tsconfig.json
- packages/inspector/package.json
- packages/inspector/src/lib/logger.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/tsconfig.json
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/src/lib/locator-derivation.ts
- packages/mobilewright/package.json
- packages/inspector/src/routes/inspect.ts
- packages/inspector/public/css/app.css
- packages/inspector/src/lib/device-manager.ts
- packages/inspector/src/routes/devices.ts
…pector Adds a new `@mobilewright/inspector` workspace package and a `mobilewright inspect` CLI subcommand that opens a browser-based element inspector for connected devices. The inspector shows a live screenshot on the left and an annotated element list on the right. Each element is annotated with the best mobilewright locator (testId > role > label > text), mirroring the priority order of @mobilewright/core's query engine. Clicking a locator highlights its bounding box on the screenshot; clicking a box selects the matching row. Duplicate locators get a "dup" badge. Architecture: - packages/inspector: standalone Express server, plain HTML/CSS/JS frontend (no build step), locator derivation pure function, DeviceManager with injected launchers (DIP) - packages/mobilewright: inspect CLI command injects ios/android launchers into start() to avoid a circular dependency; open is an explicit dependency here Hardening and review fixes: - Logs to stderr so the URL line on stdout is uncluttered - Port validated as integer in [0, 65535] - Node 18 compatible server close (no Symbol.asyncDispose) - 4-arg Express error handler returns JSON on unexpected throws - escQ escapes backslashes in addition to single quotes - open() wrapped in try/catch so headless environments print URL and continue - device.screenSize() replaces PNG IHDR heuristic for correct iPad support - 404 check in select route matches by id AND platform - Test files excluded from tsconfig (not shipped in dist) - Stale screenshot cleared when active device disconnects - aria-label on device picker and interval selects; vis-btn aria-label kept in sync - Passes ESLint @stylistic/semi throughout - 83 tests (@playwright/test): locator derivation, DeviceManager, HTTP routes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
marcomaes
force-pushed
the
feat/inspector
branch
from
0814512 to
e83d721
Compare
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/inspector/src/routes.test.ts (1)
49-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
start()smoke test instead of duplicating the server assembly here.
buildApp()/startServer()copy the bootstrap logic frompackages/inspector/src/index.ts, so these tests can still pass if the real startup path drifts. A narrow integration test aroundstart()would cover the exported URL/binding/shutdown contract and keep this wiring from diverging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/inspector/src/routes.test.ts` around lines 49 - 69, The test setup in buildApp() and startServer() duplicates the server bootstrap from the real startup path, so replace this with a narrow smoke test for start() instead. Use the exported start() entrypoint from packages/inspector/src/index.ts and assert the URL/binding/shutdown contract rather than assembling Express and listen logic directly in routes.test.ts. Keep the existing router coverage focused on behavior, but remove the copied bootstrap wiring so the test stays aligned with the production startup flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/inspector/public/js/app.js`:
- Around line 337-346: The 409 handling in app.js is rebuilding the picker from
stale cached devices, so refresh the device list before calling
`#renderDevicePicker`. Update the device-disconnected branch to fetch the latest
devices or otherwise sync this.#state.devices first, then rebuild the picker so
removed devices disappear and new ones can appear immediately. Use the existing
state and rendering flow around this.#state.devices and `#renderDevicePicker` to
keep the UI consistent.
---
Nitpick comments:
In `@packages/inspector/src/routes.test.ts`:
- Around line 49-69: The test setup in buildApp() and startServer() duplicates
the server bootstrap from the real startup path, so replace this with a narrow
smoke test for start() instead. Use the exported start() entrypoint from
packages/inspector/src/index.ts and assert the URL/binding/shutdown contract
rather than assembling Express and listen logic directly in routes.test.ts. Keep
the existing router coverage focused on behavior, but remove the copied
bootstrap wiring so the test stays aligned with the production startup flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60521e32-ab33-4b39-b008-e4929b16d88d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
README.mdpackages/inspector/package.jsonpackages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/index.tspackages/inspector/src/lib/device-manager.test.tspackages/inspector/src/lib/device-manager.tspackages/inspector/src/lib/locator-derivation.test.tspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/lib/logger.tspackages/inspector/src/routes.test.tspackages/inspector/src/routes/devices.tspackages/inspector/src/routes/inspect.tspackages/inspector/tsconfig.jsonpackages/mobilewright/package.jsonpackages/mobilewright/src/cli.tspackages/mobilewright/tsconfig.jsontsconfig.json
✅ Files skipped from review due to trivial changes (3)
- README.md
- packages/mobilewright/tsconfig.json
- packages/inspector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/inspector/public/index.html
- packages/inspector/package.json
- tsconfig.json
- packages/inspector/src/lib/logger.ts
- packages/mobilewright/src/cli.ts
- packages/inspector/src/routes/inspect.ts
- packages/inspector/public/css/app.css
- packages/mobilewright/package.json
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/src/lib/device-manager.test.ts
- packages/inspector/src/routes/devices.ts
- packages/inspector/src/lib/locator-derivation.test.ts
- packages/inspector/src/lib/device-manager.ts
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved. Approval is disabled; enable |
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
1 participant
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.
Summary
@mobilewright/inspectoras a new workspace package: an Express server with a plain HTML/CSS/JS frontend (no build step) that shows a live device screenshot alongside an annotated element list@mobilewright/core's query engine priority order exactlymobilewright inspectCLI subcommand that starts the server and opens the browser automaticallyArchitecture
packages/inspector: standalone package, no dependency onmobilewright(avoids circular dep). Launchers (ios,android) are injected viastart({ ios, android, port? })from the CLIpackages/mobilewright:inspectcommand imports@mobilewright/inspectorand injects the real launchersTest plan
npx playwright test --config=tests/mobilewright.config.ts packages/inspector— 83 tests (locator derivation, DeviceManager, HTTP routes including 503)mobilewright inspectopens browser, lists connected devices, screenshot + elements load on device selectmobilewright inspect --port 5000binds to correct port🤖 Generated with Claude Code