Fix/inspector all locators properties#227
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThe inspector now derives and returns multiple ordered locators along with additional element metadata. Its UI includes a responsive detail pane with locator copying, properties, raw attributes, and selection integration. Swipe visibility checks use the element’s bottom edge, with updated and new tests. The device fixture now merges Playwright project-specific settings into the base configuration. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
sarang-code2
force-pushed
the
fix/inspector-all-locators-properties
branch
2 times, most recently
from
00517bf to
296eae7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/inspector/src/lib/locator-derivation.ts (1)
86-86: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
deriveLocatorsresult to avoid double invocation.
deriveLocator(node)internally callsderiveLocators(node), and thenderiveLocators(node)is called again on the same line — so the function runs twice per node. Cache the result instead.♻️ Proposed fix
for (const node of nodes) { - result.push({ node, locator: deriveLocator(node), locators: deriveLocators(node) }); + const locators = deriveLocators(node); + result.push({ node, locator: locators[0] ?? null, locators }); if (node.children?.length) walk(node.children);🤖 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/locator-derivation.ts` at line 86, In the result-building loop, cache the single deriveLocators(node) result and reuse it when deriving the primary locator and assigning locators, rather than invoking deriveLocators twice. Update the surrounding result.push logic while preserving the existing node and locator output.packages/mobilewright-core/src/locator.test.ts (1)
705-726: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor comment inaccuracy in the edge-case test.
Line 723 states "Old code used centerY (822)" but 822 is
bounds.y(the top edge), not the center. The actual centerY would be822 + 44/2 = 844. The conclusion is still correct either way (neither 822 nor 844 is> 844), but the value labeled as centerY is misleading.📝 Suggested comment fix
- // Old code used centerY (822), which is not > 844, so it returned 'down' (wrong). - // Fixed code uses bottomY (866), which is > 844, so it returns 'up' (correct). + // Old code used centerY (844 = 822 + 22), which is not > 844, so it returned 'down' (wrong). + // Fixed code uses bottomY (866 = 822 + 44), which is > 844, so it returns 'up' (correct).🤖 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/mobilewright-core/src/locator.test.ts` around lines 705 - 726, Correct the explanatory comment in the test named “swipes up when element bottom exceeds viewport even when center is within viewport” to accurately describe the element’s center as 844, derived from its top position and height. Preserve the conclusion that the center is not greater than the viewport height while the bottom edge at 866 is, and remove the inaccurate reference to centerY being 822.
🤖 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 477: Rename the keyframe identifier from slideIn to slide-in in both its
`@keyframes` definition and the animation declaration, preserving the existing
animation behavior and timing.
In `@packages/inspector/public/js/app.js`:
- Around line 215-227: Update both navigator.clipboard.writeText calls in the
copyBtn click handler and row click handler to handle rejected promises with
catch callbacks, preventing unhandled rejections. Preserve the existing button
success feedback, and add appropriate success and failure visual feedback for
the row click flow.
---
Nitpick comments:
In `@packages/inspector/src/lib/locator-derivation.ts`:
- Line 86: In the result-building loop, cache the single deriveLocators(node)
result and reuse it when deriving the primary locator and assigning locators,
rather than invoking deriveLocators twice. Update the surrounding result.push
logic while preserving the existing node and locator output.
In `@packages/mobilewright-core/src/locator.test.ts`:
- Around line 705-726: Correct the explanatory comment in the test named “swipes
up when element bottom exceeds viewport even when center is within viewport” to
accurately describe the element’s center as 844, derived from its top position
and height. Preserve the conclusion that the center is not greater than the
viewport height while the bottom edge at 866 is, and remove the inaccurate
reference to centerY being 822.
🪄 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: b4c10535-c308-407e-a5a1-490111440eb5
📒 Files selected for processing (8)
packages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/routes/inspect.tspackages/mobilewright-core/src/locator.test.tspackages/mobilewright-core/src/locator.tspackages/test/src/fixtures.ts
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/css/app.css`:
- Around line 480-483: Update the slideIn keyframes to animate flex-basis from 0
to 320px instead of width, preserving the existing opacity animation so
`#detail-pane` visibly slides in while fading.
🪄 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: 981e1528-9f88-4795-9cfd-42099d12165b
📒 Files selected for processing (5)
packages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/routes/inspect.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/inspector/src/routes/inspect.ts
- packages/inspector/public/index.html
- packages/inspector/src/lib/locator-derivation.ts
- packages/inspector/public/js/app.js
sarang-code2
force-pushed
the
fix/inspector-all-locators-properties
branch
2 times, most recently
from
83fb2f3 to
331071e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/inspector/public/js/app.js (1)
290-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedandaria-controlsto the raw attributes toggle.The toggle button controls the
contentdiv visibility but lacksaria-expandedandaria-controls, so screen readers can't announce the expanded/collapsed state or the relationship to the controlled content.♿ Proposed accessibility improvement
const toggle = document.createElement('button') toggle.className = 'detail-raw-toggle' toggle.textContent = `▶ Raw Attributes (${Object.keys(el.raw).length})` + toggle.setAttribute('aria-expanded', 'false') + toggle.setAttribute('aria-controls', 'detail-raw-content') this.#rawEl.appendChild(toggle) const content = document.createElement('div') content.className = 'detail-raw-content' + content.id = 'detail-raw-content' content.hidden = true content.textContent = JSON.stringify(el.raw, null, 2) toggle.addEventListener('click', () => { const isHidden = content.hidden content.hidden = !isHidden + toggle.setAttribute('aria-expanded', String(isHidden)) toggle.textContent = isHidden ? `▼ Raw Attributes (${Object.keys(el.raw).length})` : `▶ Raw Attributes (${Object.keys(el.raw).length})` })🤖 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 290 - 306, Update the raw attributes toggle setup in the detail rendering flow to add an `aria-expanded` state initialized to collapsed and an `aria-controls` reference targeting the `content` div. Give the controlled div a matching unique id, and update `aria-expanded` in the existing toggle click handler alongside `content.hidden` so the accessibility state stays synchronized.
🤖 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.
Nitpick comments:
In `@packages/inspector/public/js/app.js`:
- Around line 290-306: Update the raw attributes toggle setup in the detail
rendering flow to add an `aria-expanded` state initialized to collapsed and an
`aria-controls` reference targeting the `content` div. Give the controlled div a
matching unique id, and update `aria-expanded` in the existing toggle click
handler alongside `content.hidden` so the accessibility state stays
synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17b171e9-3fa9-43ea-91df-7b4eaeaf9fae
📒 Files selected for processing (6)
packages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/routes/inspect.tspackages/mobilewright-core/src/locator.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/inspector/src/routes/inspect.ts
- packages/inspector/public/css/app.css
- packages/mobilewright-core/src/locator.test.ts
- packages/inspector/src/lib/locator-derivation.ts
sarang-code2
force-pushed
the
fix/inspector-all-locators-properties
branch
from
331071e to
81e9a1d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/inspector/src/lib/locator-derivation.ts (2)
79-94: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffRecursive
walkrisks stack overflow on deeply nested view trees.
walkrecurses intonode.childrenfor each node. For very deep view hierarchies (e.g., hundreds of nested levels), this could cause a stack overflow. This is pre-existing behavior carried forward from the original implementation, so it's not a regression, but if the inspector ever encounters deeply nested native UI trees, an iterative approach with an explicit stack would be safer.Consider this a deferred improvement, not a blocker.
🤖 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/locator-derivation.ts` around lines 79 - 94, Replace the recursive walk function inside deriveElementList with an iterative traversal using an explicit stack, preserving preorder output and locator derivation for every node. Ensure child nodes are pushed in reverse order so their original traversal order remains unchanged.
50-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
deriveLocatorsmay emit duplicatetextlocators whenlabelandtextare identical.When
node.labelandnode.textare the same string (common for static text elements), the function produces both alabellocator and atextlocator with the same value. This is technically correct since they are different strategies, but the downstream UI shows multiple locator indicators, which could be confusing if the values are identical. Consider deduplicating by(kind, value)or documenting that different strategies with the same value are intentional.This is a minor observation — the current behavior is logically sound and matches the PR's goal of returning all matching strategies.
🤖 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/locator-derivation.ts` around lines 50 - 68, Preserve the current behavior in deriveLocators: label and text are distinct locator strategies, so identical values should continue producing both locators. No code change is required; document this intentional behavior if clarification is needed.
🤖 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.
Nitpick comments:
In `@packages/inspector/src/lib/locator-derivation.ts`:
- Around line 79-94: Replace the recursive walk function inside
deriveElementList with an iterative traversal using an explicit stack,
preserving preorder output and locator derivation for every node. Ensure child
nodes are pushed in reverse order so their original traversal order remains
unchanged.
- Around line 50-68: Preserve the current behavior in deriveLocators: label and
text are distinct locator strategies, so identical values should continue
producing both locators. No code change is required; document this intentional
behavior if clarification is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3f2dfff-7bfe-433f-ac29-6d43eb073bdb
📒 Files selected for processing (6)
packages/inspector/public/css/app.csspackages/inspector/public/index.htmlpackages/inspector/public/js/app.jspackages/inspector/src/lib/locator-derivation.tspackages/inspector/src/routes/inspect.tspackages/mobilewright-core/src/locator.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/inspector/public/index.html
- packages/inspector/src/routes/inspect.ts
- packages/mobilewright-core/src/locator.test.ts
- packages/inspector/public/js/app.js
- packages/inspector/public/css/app.css
sarang-code2
force-pushed
the
fix/inspector-all-locators-properties
branch
from
81e9a1d to
f07025b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…other options The device fixture only read top-level config.use, silently ignoring options set inside config.projects[].use like actionTimeout, appLaunchTimeout, installTimeout, and animations. Now finds the matching project by name from testInfo.project.name and merges its use into the top-level use before passing to connectDevice.
… bottom partially off-screen swipeDirectionToReveal used centerY (bounds.y + bounds.height / 2) to determine scroll direction. When an element's center was within the viewport but its bottom edge extended past the screen, it returned 'down' (scroll up), pushing the element further out of view. Now uses bottomY (bounds.y + bounds.height) instead: if the trailing edge extends past the screen bottom, we correctly swipe 'up' (scroll down) to bring it into view. Also adds a test for this edge case.
mobile-next#222) - deriveLocators() returns array of all matching strategies ordered by priority - deriveElementList includes both locator (first) and locators (all) per node - /api/inspect payload expanded with identifier, resourceId, placeholder, value, isEnabled, isSelected, isFocused, isChecked, and raw attributes - New DetailPane class shows all locator strategies with copy buttons, properties table, and collapsible raw attributes - Element list shows +N more badge for elements with multiple locators - Detail pane slides in from the right with responsive 3-panel layout - Detail pane hidden on refresh to avoid stale data Signed-off-by: sarang-001 <techietriumphbusiness@gmail.com>
sarang-code2
force-pushed
the
fix/inspector-all-locators-properties
branch
from
f07025b to
83e678b
Compare
1 participant
Summary
Before Fix:
After Fix:
Fixes #222 — the inspector now returns all matching locator strategies (not just the first priority match) and shows a detail panel with all element properties.
Changes
packages/inspector/src/lib/locator-derivation.tsderiveLocators()returns array of all matching locators ordered by priority (testId > role > label > text)deriveLocator()kept for backward compat — returns first match or nullderiveElementList()extended to include bothlocator(first) andlocators(all) per nodepackages/inspector/src/routes/inspect.ts/api/inspectpayload expanded with all ViewNode fields:identifier,resourceId,placeholder,value,isEnabled,isSelected,isFocused,isChecked,rawlocatorsarraypackages/inspector/public/index.html<section id="detail-pane">with header, locators section, properties section, and raw attributes sectionpackages/inspector/public/js/app.jsDetailPaneclass: locator strategies display (each with copy button), properties table, collapsible raw attributesElementsPane.#buildRowshows+N morebadge for elements with multiple locatorsInspectorwires up detail pane: shows on element click via#selectElement, hides on refresh or closepackages/inspector/public/css/app.css.locator-morebadge style for multi-locator elementsTesting
Verified with iPhone 16 Pro simulator — all 17 elements returned with 3 locator strategies each (testId, role, label). Detail pane shows all properties correctly.
Closes #222