Add support for tapping elements via id/label in addition to x/y coords #149
Conversation
commit: |
WalkthroughThe version in Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/tools/ui-testing/tap.ts (1)
202-209: Extract executeAxeCommand to a shared utility to eliminate duplication across all UI testing tools.This function is duplicated identically across nine UI testing tools (tap, type_text, touch, long_press, key_press, gesture, describe_ui, swipe, button, and key_sequence). Move this shared logic to
src/utils/axe-helpers.tsalongside the existing AXe helper functions to reduce maintenance burden and ensure consistency.The comment referencing "inlined from src/tools/axe/index.ts" appears outdated—this file does not exist. The actual shared utilities are already in
axe-helpers.ts, making this the appropriate location for the executeAxeCommand function as well.
🧹 Nitpick comments (2)
src/mcp/tools/ui-testing/tap.ts (1)
189-191: Consider adding a brief comment explaining the type assertion.The
as unknown as z.ZodType<TapParams>cast is necessary becausesuperRefinereturnsZodEffectsrather thanZodObject, which is incompatible with the generic constraint. A short inline comment would help future maintainers understand this isn't an unsafe cast.🔎 Suggested documentation
handler: createSessionAwareTool<TapParams>({ + // Cast needed: superRefine returns ZodEffects, not ZodObject internalSchema: tapSchema as unknown as z.ZodType<TapParams>,src/mcp/tools/ui-testing/__tests__/tap.test.ts (1)
677-729: Consider adding tests for empty string and no-target scenarios.The validation logic handles these cases, but explicit handler-level tests would provide additional confidence:
- Empty string
idorlabel(rejected by.min(1))- No target provided (neither coordinates nor id/label)
🔎 Example additional tests
it('should return validation error when no target is provided', async () => { sessionStore.setDefaults({ simulatorId: '12345678-1234-1234-1234-123456789012' }); const result = await tapPlugin.handler({}); expect(result.isError).toBe(true); const message = result.content[0].text; expect(message).toContain('Provide x/y coordinates or an element id/label'); }); it('should return validation error for empty id', async () => { sessionStore.setDefaults({ simulatorId: '12345678-1234-1234-1234-123456789012' }); const result = await tapPlugin.handler({ id: '' }); expect(result.isError).toBe(true); const message = result.content[0].text; expect(message).toContain('Id must be non-empty'); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.axe-version(1 hunks)src/mcp/tools/device/__tests__/build_device.test.ts(0 hunks)src/mcp/tools/simulator/__tests__/install_app_sim.test.ts(0 hunks)src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts(0 hunks)src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts(0 hunks)src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/gesture.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/key_press.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/long_press.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/screenshot.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/swipe.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/tap.test.ts(6 hunks)src/mcp/tools/ui-testing/__tests__/touch.test.ts(0 hunks)src/mcp/tools/ui-testing/__tests__/type_text.test.ts(0 hunks)src/mcp/tools/ui-testing/tap.ts(4 hunks)src/utils/__tests__/session-aware-tool-factory.test.ts(1 hunks)src/utils/typed-tool-factory.ts(1 hunks)
💤 Files with no reviewable changes (13)
- src/mcp/tools/ui-testing/tests/touch.test.ts
- src/mcp/tools/ui-testing/tests/long_press.test.ts
- src/mcp/tools/ui-testing/tests/key_press.test.ts
- src/mcp/tools/simulator/tests/launch_app_sim.test.ts
- src/mcp/tools/simulator/tests/launch_app_logs_sim.test.ts
- src/mcp/tools/ui-testing/tests/gesture.test.ts
- src/mcp/tools/simulator/tests/stop_app_sim.test.ts
- src/mcp/tools/ui-testing/tests/key_sequence.test.ts
- src/mcp/tools/ui-testing/tests/screenshot.test.ts
- src/mcp/tools/ui-testing/tests/swipe.test.ts
- src/mcp/tools/ui-testing/tests/type_text.test.ts
- src/mcp/tools/simulator/tests/install_app_sim.test.ts
- src/mcp/tools/device/tests/build_device.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.ts: Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Files:
src/utils/__tests__/session-aware-tool-factory.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Use .ts extensions for all internal relative imports (e.g., import { tool } from './tool.ts')
Use .ts extensions in re-exports for internal files (e.g., export { default } from '../shared/tool.ts')
Use .js extension only for external package entry points (e.g., import ... from '@scope/pkg/file.js')
Never import internal project files using .js extensions
Files:
src/utils/__tests__/session-aware-tool-factory.test.tssrc/utils/typed-tool-factory.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/tap.ts
🧠 Learnings (1)
📚 Learning: 2025-09-21T19:27:22.795Z
Learnt from: CR
Repo: cameroncooke/XcodeBuildMCP PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-21T19:27:22.795Z
Learning: Applies to **/*.{test,spec}.ts : In tests, import the core ...Logic function from the tool and use createMockExecutor/createMockFileSystemExecutor to simulate outcomes
Applied to files:
src/mcp/tools/ui-testing/__tests__/tap.test.ts
🧬 Code graph analysis (2)
src/mcp/tools/ui-testing/__tests__/tap.test.ts (3)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/ui-testing/tap.ts (1)
tapLogic(101-179)src/utils/session-store.ts (1)
sessionStore(48-48)
src/mcp/tools/ui-testing/tap.ts (4)
src/utils/responses/index.ts (1)
createErrorResponse(7-7)src/utils/errors.ts (1)
createErrorResponse(111-122)src/utils/logging/index.ts (1)
log(5-5)src/utils/typed-tool-factory.ts (1)
getSessionAwareToolSchemaShape(89-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
.axe-version (1)
1-1: LGTM!Version bump to 1.2.0 aligns with the new tap targeting features (id/label support).
src/utils/typed-tool-factory.ts (1)
193-195: LGTM!The removal of the tip from Zod validation errors is appropriate. The tip remains in requirement-based errors (lines 157-159, 171-173) where it's more contextually relevant—when session defaults are missing rather than when parameters have invalid types/values.
src/utils/__tests__/session-aware-tool-factory.test.ts (1)
121-130: LGTM!Test description and assertions correctly updated to reflect the removal of the tip from Zod validation errors.
src/mcp/tools/ui-testing/tap.ts (2)
35-73: Well-structured validation logic.The
superRefinevalidation correctly handles:
- Mutual exclusivity between
idandlabelwhen coordinates aren't provided- Requiring both
xandywhen either is provided- Ensuring at least one target type is specified
Note: when coordinates are provided alongside both
idandlabel, validation passes and coordinates take precedence—this matches the documented behaviour and test expectations.
118-136: LGTM!Clear precedence logic: coordinates take priority over element targeting. The defensive fallback error (lines 131-136) is good practice, even though it should be unreachable after schema validation.
src/mcp/tools/ui-testing/__tests__/tap.test.ts (4)
66-79: Good schema validation coverage.Tests correctly validate the new
idandlabelfields, including combinations with coordinates. The assertions cover the expected valid input combinations.
188-319: Comprehensive command generation tests.Tests cover all targeting modes (coordinates, id, label) and verify the precedence behaviour where coordinates take priority. The command structure assertions are precise and match the implementation.
620-674: LGTM!Tests correctly verify that element-based taps (by id or label) return success messages without the
describe_uiwarning, as the warning is only relevant for coordinate-based taps.
691-729: LGTM!Validation error tests cover the new scenarios: missing paired coordinate and mutual exclusivity between
idandlabel. Error messages match thesuperRefinevalidation logic.
This pull request primarily updates the Tap Plugin to support targeting elements by accessibility id or label in addition to coordinates, and improves validation and test coverage for these new features. It also removes redundant validation tip messages from test assertions across multiple plugins and updates the AXe tool version.
Tap Plugin enhancements:
id) or label (label) in the Tap Plugin, including updated schema validation, command generation logic, and success responses for these new targeting methods. Tests were added to verify correct behavior and command construction for all cases. [1] [2] [3]idandlabel, and improved test assertions for these cases. [1] [2] [3] [4] [5] [6]General test cleanup:
Documentation and schema:
Tooling:
.axe-versionto use AXe tool version 1.2.0.Note
Adds Tap plugin element targeting via id/label with improved validation and tests, removes session tip from error messages, and updates AXe to 1.2.0.
src/mcp/tools/ui-testing/tap.ts):id/labelalongside optionalx/y; enforce mutual exclusivity and paired coords; improved error messages.createSessionAwareTool(typed-tool-factory): drop appended "session defaults" tip from Zod validation error details; adjust tests..axe-versionfrom1.1.1to1.2.0.Written by Cursor Bugbot for commit f0f6391. This will update automatically on new commits. Configure here.