feat(simulator): migrate simulator tools to session defaults #135
Conversation
WalkthroughThis PR completes a session-aware tool migration by renaming Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/utils/simulator-utils.ts (1)
49-49: Update warning message to reference new parameter name.The warning message still suggests using
simulatorUuidparameter, but should now recommendsimulatorIdto align with the migration:- warning: `The simulatorName '${params.simulatorName}' appears to be a UUID. Consider using simulatorUuid parameter instead.`, + warning: `The simulatorName '${params.simulatorName}' appears to be a UUID. Consider using simulatorId parameter instead.`,src/mcp/tools/simulator/stop_app_sim.ts (1)
11-22: Restore UUID validation for simulatorIdDropping the
.uuid()guard means any string now reachesxcrun simctl terminate, so we’ll surface runtime failures instead of stopping bad input up front. Please reinstate the UUID check so only valid simulator identifiers pass through.Apply this diff to restore the validation:
- simulatorId: z - .string() - .optional() - .describe( + simulatorId: z + .string() + .uuid() + .optional() + .describe(src/mcp/tools/ui-testing/long_press.ts (2)
12-19: Incorrect source for error classesDependencyError, AxeError, and SystemError should come from the errors module, not responses. This will break type resolution/runtime.
Apply this fix:
-import { - createTextResponse, - createErrorResponse, - DependencyError, - AxeError, - SystemError, -} from '../../../utils/responses/index.ts'; +import { createTextResponse, createErrorResponse } from '../../../utils/responses/index.ts'; +import { DependencyError, AxeError, SystemError } from '../../../utils/errors.ts';
109-125: Migrate to session-aware wrapper and hide simulatorId from public schemaThis tool still uses
createTypedTooland exposessimulatorIdin the public schema. Migrate to the established pattern: (1) importcreateSessionAwareToolfrom../../../utils/typed-tool-factory.ts, (2) createpublicSchemaObjectby omittingsimulatorId(e.g.,longPressSchema.omit({ simulatorId: true } as const).strict()), (3) usecreateSessionAwareTool<LongPressParams>withrequirements: [{ allOf: ['simulatorId'], message: 'simulatorId is required' }]. Reference:src/mcp/tools/ui-testing/button.ts.src/mcp/tools/ui-testing/swipe.ts (2)
120-136: Switch to session-aware factoryReplace createTypedTool with the session-aware tool factory, similar to other migrated plugins, so handler can rely on session defaults and keep simulatorId out of the public surface.
21-33: Refactor swipe.ts to use session-aware tool pattern and omit simulatorId from public schemaUpdate swipe.ts to follow the established pattern used in similar tools (button.ts, gesture.ts, etc.):
- Create
publicSchemaObjectthat omitssimulatorId(line ~21)- Change
schemaexport to usepublicSchemaObject.shapeinstead ofswipeSchema.shape(line 133)- Replace
createTypedToolwithcreateSessionAwareToolin handler (lines 134–140)- Add
requirements: [{ allOf: ['simulatorId'] }]to ensure simulatorId is provided via session defaultsThis aligns swipe.ts with the session-aware pattern already implemented in button.ts, describe_ui.ts, gesture.ts, and other tools, ensuring simulatorId is derived from session defaults rather than exposed to callers.
src/mcp/tools/ui-testing/touch.ts (1)
22-31: Refactor UI-testing tools to use session-aware wrapper patternTools
touch,type_text,tap,swipe,screenshot, andlong_pressstill exposesimulatorIdin their public schemas. Adopt the established pattern fromkey_press,key_sequence,describe_ui,gesture, andbutton:
- Create
publicSchemaObjectby omittingsimulatorIdfrom the full schema- Replace
createTypedToolwithcreateSessionAwareToolin the handler- Add
requirements: [{ allOf: ['simulatorId'], message: 'simulatorId is required' }]to surface session defaultsThis ensures consistent session-default handling across all UI-testing tools.
src/mcp/tools/ui-testing/type_text.ts (1)
20-88: Migratetype_textto the session-aware wrapperEvery other simulator-facing tool in this refactor now sources
simulatorIdfrom session defaults viacreateSessionAwareTooland exposes a public schema with that field omitted. Keepingtype_textoncreateTypedToolmeans the handler still requiressimulatorIdin the request body, so any caller relying on the new session-default contract will immediately fail validation here. Please mirror the pattern used in files likesrc/mcp/tools/simulator-management/set_sim_location.ts—introduce apublicSchemaObjectthat omitssimulatorId, switch the export tocreateSessionAwareTool, and declare therequirementsso this tool consumes the shared session default like the rest of the suite.
🧹 Nitpick comments (10)
example_projects/iOS_Calculator/.gitignore (1)
1-7: Reasonable starter .gitignore for the iOS example project.The file correctly excludes xcode-build-server artifacts and local build directories. The format is clean and well-commented.
Consider including additional common iOS development artifacts for a more complete coverage. This is entirely optional and depends on your project's needs:
# xcode-build-server files buildServer.json .compile # Local build artifacts .build/ + +# Xcode-specific artifacts +.DS_Store +*.xcodeproj/xcuserdata/ +.swiftpm/These additions capture macOS metadata and Xcode user settings that often pollute example projects, though only if your project generates them.
src/mcp/tools/ui-testing/long_press.ts (2)
20-25: Unify axe helper imports with the rest of UI testing toolsOther tools import from utils/axe-helpers.ts; this file uses utils/axe/index.ts. Prefer a single helper module to avoid drift.
44-79: Minor: message clarity for coordinates and durationConsider including units explicitly in logs/messages, e.g., “duration=1.2s” rather than raw milliseconds conversion only in command args.
src/mcp/tools/ui-testing/swipe.ts (2)
84-93: Optional: include duration=0 in message textoptionsText uses a truthy check, so duration=0 won’t render. Consider explicit check to display duration=0s when provided.
-const optionsText = duration ? ` duration=${duration}s` : ''; +const optionsText = duration !== undefined ? ` duration=${duration}s` : '';
15-19: Helper import consistencyOther files use utils/axe-helpers.ts. Consider standardizing on a single helper module path.
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (1)
157-163: Test duplicationTwo tests validate bundleId when a simulatorId default is set; they overlap substantially. Consider merging to reduce noise.
Also applies to: 165-172, 174-182
src/mcp/tools/simulator-management/set_sim_appearance.ts (1)
97-101: Trim the redundant schema cast.
setSimAppearanceSchemaalready satisfiesZodType<SetSimAppearanceParams>, so theas unknown ashop just hides real typing mistakes. Dropping the cast keeps the code honest without changing behavior.src/mcp/tools/ui-testing/tap.ts (1)
23-24: Align the validation message with the new field name.Now that the parameter is
simulatorId, consider updating the error string to something like “Invalid simulatorId (UUID) format” so the copy matches the API surface.src/mcp/tools/simulator/launch_app_sim.ts (1)
118-127: Update the fallback message to say “simulatorId.”If this guard ever fires, the user now expects the terminology you introduced elsewhere (
simulatorId). A quick copy edit keeps the messaging consistent with the renamed field.src/mcp/tools/simulator/launch_app_logs_sim.ts (1)
13-17: Align log capture contract with simulatorId renameThe rest of this tool now works with
simulatorId, but the injected log-capture contract still exposes asimulatorUuidkey. To keep the rename coherent—and to avoid silently dropping the simulator identifier ifstartLogCapturehas already switched tosimulatorId—please update the DTO and invocation to usesimulatorId, or confirm that the underlying capture helper still requires the older key.Also applies to: 46-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
docs/session-aware-migration-todo.md(1 hunks)example_projects/iOS_Calculator/.gitignore(1 hunks)src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts(7 hunks)src/mcp/tools/logging/start_sim_log_cap.ts(3 hunks)src/mcp/tools/simulator-management/__tests__/erase_sims.test.ts(4 hunks)src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts(5 hunks)src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts(5 hunks)src/mcp/tools/simulator-management/__tests__/set_sim_location.test.ts(14 hunks)src/mcp/tools/simulator-management/__tests__/sim_statusbar.test.ts(9 hunks)src/mcp/tools/simulator-management/erase_sims.ts(6 hunks)src/mcp/tools/simulator-management/reset_sim_location.ts(5 hunks)src/mcp/tools/simulator-management/set_sim_appearance.ts(5 hunks)src/mcp/tools/simulator-management/set_sim_location.ts(5 hunks)src/mcp/tools/simulator-management/sim_statusbar.ts(2 hunks)src/mcp/tools/simulator/__tests__/boot_sim.test.ts(3 hunks)src/mcp/tools/simulator/__tests__/install_app_sim.test.ts(10 hunks)src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts(8 hunks)src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts(10 hunks)src/mcp/tools/simulator/__tests__/list_sims.test.ts(4 hunks)src/mcp/tools/simulator/__tests__/open_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/record_sim_video.test.ts(6 hunks)src/mcp/tools/simulator/__tests__/screenshot.test.ts(13 hunks)src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts(6 hunks)src/mcp/tools/simulator/boot_sim.ts(4 hunks)src/mcp/tools/simulator/build_run_sim.ts(9 hunks)src/mcp/tools/simulator/get_sim_app_path.ts(1 hunks)src/mcp/tools/simulator/install_app_sim.ts(4 hunks)src/mcp/tools/simulator/launch_app_logs_sim.ts(4 hunks)src/mcp/tools/simulator/launch_app_sim.ts(9 hunks)src/mcp/tools/simulator/list_sims.ts(1 hunks)src/mcp/tools/simulator/open_sim.ts(1 hunks)src/mcp/tools/simulator/record_sim_video.ts(6 hunks)src/mcp/tools/simulator/stop_app_sim.ts(8 hunks)src/mcp/tools/ui-testing/__tests__/button.test.ts(16 hunks)src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts(8 hunks)src/mcp/tools/ui-testing/__tests__/gesture.test.ts(14 hunks)src/mcp/tools/ui-testing/__tests__/key_press.test.ts(12 hunks)src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts(13 hunks)src/mcp/tools/ui-testing/__tests__/long_press.test.ts(15 hunks)src/mcp/tools/ui-testing/__tests__/screenshot.test.ts(14 hunks)src/mcp/tools/ui-testing/__tests__/swipe.test.ts(16 hunks)src/mcp/tools/ui-testing/__tests__/tap.test.ts(31 hunks)src/mcp/tools/ui-testing/__tests__/touch.test.ts(23 hunks)src/mcp/tools/ui-testing/__tests__/type_text.test.ts(14 hunks)src/mcp/tools/ui-testing/button.ts(5 hunks)src/mcp/tools/ui-testing/describe_ui.ts(6 hunks)src/mcp/tools/ui-testing/gesture.ts(6 hunks)src/mcp/tools/ui-testing/key_press.ts(5 hunks)src/mcp/tools/ui-testing/key_sequence.ts(5 hunks)src/mcp/tools/ui-testing/long_press.ts(7 hunks)src/mcp/tools/ui-testing/screenshot.ts(4 hunks)src/mcp/tools/ui-testing/swipe.ts(7 hunks)src/mcp/tools/ui-testing/tap.ts(7 hunks)src/mcp/tools/ui-testing/touch.ts(7 hunks)src/mcp/tools/ui-testing/type_text.ts(5 hunks)src/utils/simulator-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/mcp/tools/simulator/open_sim.tssrc/mcp/tools/simulator/list_sims.tssrc/mcp/tools/ui-testing/screenshot.tssrc/mcp/tools/ui-testing/__tests__/screenshot.test.tssrc/mcp/tools/simulator/__tests__/list_sims.test.tssrc/mcp/tools/simulator/__tests__/open_sim.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/ui-testing/touch.tssrc/mcp/tools/ui-testing/__tests__/touch.test.tssrc/mcp/tools/simulator/launch_app_sim.tssrc/mcp/tools/simulator-management/sim_statusbar.tssrc/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.tssrc/mcp/tools/ui-testing/__tests__/long_press.test.tssrc/mcp/tools/simulator-management/set_sim_appearance.tssrc/mcp/tools/simulator/__tests__/screenshot.test.tssrc/mcp/tools/simulator/stop_app_sim.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/simulator-management/__tests__/set_sim_location.test.tssrc/mcp/tools/ui-testing/long_press.tssrc/mcp/tools/logging/start_sim_log_cap.tssrc/mcp/tools/simulator/launch_app_logs_sim.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/simulator/get_sim_app_path.tssrc/mcp/tools/simulator/record_sim_video.tssrc/mcp/tools/ui-testing/button.tssrc/mcp/tools/ui-testing/key_sequence.tssrc/mcp/tools/ui-testing/key_press.tssrc/mcp/tools/ui-testing/tap.tssrc/mcp/tools/simulator-management/__tests__/reset_sim_location.test.tssrc/mcp/tools/simulator/boot_sim.tssrc/mcp/tools/simulator-management/__tests__/erase_sims.test.tssrc/mcp/tools/simulator/build_run_sim.tssrc/mcp/tools/simulator-management/reset_sim_location.tssrc/mcp/tools/ui-testing/swipe.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/simulator/install_app_sim.tssrc/mcp/tools/ui-testing/type_text.tssrc/mcp/tools/simulator-management/erase_sims.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/utils/simulator-utils.tssrc/mcp/tools/simulator-management/__tests__/sim_statusbar.test.tssrc/mcp/tools/ui-testing/gesture.tssrc/mcp/tools/simulator/__tests__/boot_sim.test.tssrc/mcp/tools/simulator-management/set_sim_location.tssrc/mcp/tools/logging/__tests__/start_sim_log_cap.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/ui-testing/describe_ui.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.ts
**/*.{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/mcp/tools/ui-testing/__tests__/screenshot.test.tssrc/mcp/tools/simulator/__tests__/list_sims.test.tssrc/mcp/tools/simulator/__tests__/open_sim.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/ui-testing/__tests__/touch.test.tssrc/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.tssrc/mcp/tools/ui-testing/__tests__/long_press.test.tssrc/mcp/tools/simulator/__tests__/screenshot.test.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/simulator-management/__tests__/set_sim_location.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/simulator-management/__tests__/reset_sim_location.test.tssrc/mcp/tools/simulator-management/__tests__/erase_sims.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/simulator-management/__tests__/sim_statusbar.test.tssrc/mcp/tools/simulator/__tests__/boot_sim.test.tssrc/mcp/tools/logging/__tests__/start_sim_log_cap.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.ts
🧠 Learnings (2)
📚 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__/screenshot.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/ui-testing/__tests__/type_text.test.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/simulator/launch_app_sim.tssrc/mcp/tools/simulator-management/sim_statusbar.tssrc/mcp/tools/simulator-management/set_sim_appearance.tssrc/mcp/tools/simulator/__tests__/screenshot.test.tssrc/mcp/tools/simulator/stop_app_sim.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/logging/start_sim_log_cap.tssrc/mcp/tools/simulator/launch_app_logs_sim.tsdocs/session-aware-migration-todo.mdsrc/mcp/tools/ui-testing/key_sequence.tssrc/mcp/tools/simulator/boot_sim.tssrc/mcp/tools/simulator-management/__tests__/erase_sims.test.tssrc/mcp/tools/simulator-management/reset_sim_location.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/simulator/install_app_sim.tssrc/mcp/tools/simulator/__tests__/boot_sim.test.tssrc/mcp/tools/simulator-management/set_sim_location.tssrc/mcp/tools/logging/__tests__/start_sim_log_cap.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.ts
📚 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 : Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
Applied to files:
src/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/simulator/__tests__/record_sim_video.test.tssrc/mcp/tools/ui-testing/__tests__/describe_ui.test.tssrc/mcp/tools/logging/__tests__/start_sim_log_cap.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.ts
🧬 Code graph analysis (31)
src/mcp/tools/ui-testing/screenshot.ts (1)
src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/simulator/__tests__/record_sim_video.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/ui-testing/touch.ts (1)
src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/simulator/launch_app_sim.ts (2)
src/utils/logger.ts (1)
log(142-162)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator-management/sim_statusbar.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator-management/set_sim_appearance.ts (1)
set_sim_appearanceLogic(72-88)
src/mcp/tools/simulator-management/set_sim_appearance.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/stop_app_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/ui-testing/long_press.ts (1)
src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/logging/start_sim_log_cap.ts (3)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/execution/index.ts (1)
getDefaultCommandExecutor(5-5)src/utils/command.ts (1)
getDefaultCommandExecutor(199-210)
src/mcp/tools/simulator/launch_app_logs_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/record_sim_video.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/ui-testing/button.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/key_sequence.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/key_press.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/tap.ts (2)
src/utils/logger.ts (1)
log(142-162)src/utils/logging/index.ts (1)
log(5-5)
src/mcp/tools/simulator/boot_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator-management/__tests__/erase_sims.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator-management/erase_sims.ts (1)
erase_simsLogic(26-130)
src/mcp/tools/simulator-management/reset_sim_location.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/ui-testing/swipe.ts (1)
src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/simulator/install_app_sim.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator-management/erase_sims.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/utils/simulator-utils.ts (3)
src/utils/execution/index.ts (1)
CommandExecutor(8-8)src/utils/responses/index.ts (1)
ToolResponse(15-15)src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/ui-testing/gesture.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/simulator/__tests__/boot_sim.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator/boot_sim.ts (1)
boot_simLogic(16-62)
src/mcp/tools/simulator-management/set_sim_location.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/__tests__/install_app_sim.test.ts (1)
src/utils/session-store.ts (1)
sessionStore(47-47)
src/mcp/tools/ui-testing/describe_ui.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
⏰ 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 (56)
src/mcp/tools/ui-testing/__tests__/touch.test.ts (1)
1-829: LGTM! Comprehensive and consistent renaming.The migration from
simulatorUuidtosimulatorIdhas been applied consistently across all test cases including schema validation, command generation, and handler behavior tests. The test logic and assertions remain unchanged, ensuring the test suite continues to provide the same comprehensive coverage.src/mcp/tools/ui-testing/__tests__/screenshot.test.ts (5)
1-14: LGTM! Test structure follows coding guidelines.The imports correctly use
.tsextensions, and the test properly imports the corescreenshotLogicfunction along with the appropriate mock executors.
31-50: LGTM! Schema validation tests updated consistently.All references to the parameter have been correctly updated from
simulatorUuidtosimulatorId, including test inputs and comments.
53-82: LGTM! Validation error messages properly updated.The test expectations for validation errors have been correctly updated to reference
simulatorIdin both the test descriptions and expected error messages.
85-238: LGTM! Command generation tests consistently updated.All test cases properly use
simulatorIdas the parameter name while maintaining the correct command generation expectations.
240-448: Handler behavior tests correctly verified and aligned with implementation.Verification confirms the implementation file at
src/mcp/tools/ui-testing/screenshot.tscorrectly usessimulatorIdthroughout:
- Schema definition uses
simulatorIdwith UUID validation- Function signature and all internal references use
simulatorIdconsistently- No remnants of
simulatorUuid- All test expectations match implementation behavior
The test suite comprehensively covers success paths, command execution failures, file I/O errors, cleanup edge cases, and various error types. Tests properly use
createMockExecutorandcreateMockFileSystemExecutorwithout relying on Vitest mocking APIs.docs/session-aware-migration-todo.md (1)
36-51: LGTM! Migration tracking looks complete.The documentation clearly tracks the completed migration of simulator runtime actions, management tools, and logging tools to session-aware defaults. The "(hydrate simulatorUuid)" notation effectively indicates the migration path.
src/mcp/tools/simulator/get_sim_app_path.ts (1)
250-252: LGTM! Parameter naming updated consistently.The Next Steps guidance now correctly references
simulatorIdinstead ofSIMULATOR_UUID, aligning with the session-aware migration.src/mcp/tools/ui-testing/__tests__/tap.test.ts (1)
1-816: LGTM! Test suite successfully migrated to simulatorId.All test cases have been systematically updated from
simulatorUuidtosimulatorId. The changes include:
- Schema validation test inputs
- Logic function invocations
- Error message expectations
- UUID format validation (still correctly validates UUID format)
The test coverage and assertions remain comprehensive and logically equivalent.
src/mcp/tools/ui-testing/__tests__/long_press.test.ts (1)
1-521: LGTM! Long press tests migrated consistently.All test cases systematically updated from
simulatorUuidtosimulatorId, matching the pattern in other UI testing tools. Test coverage and validation logic remain comprehensive.src/mcp/tools/ui-testing/__tests__/gesture.test.ts (2)
30-55: LGTM! Public schema validation added.Excellent addition of tests validating that the public schema:
- Accepts
presetand optional parameters- Does not expose
simulatorIdin parsed data (line 54)- Still validates input correctly
This properly tests the session-aware tool pattern where
simulatorIdis handled via session defaults.
78-420: LGTM! Gesture logic tests migrated consistently.All
gestureLogicinvocations systematically updated to usesimulatorId, maintaining comprehensive test coverage for command generation and error handling.src/utils/simulator-utils.ts (1)
28-36: LGTM! Backward-compatible parameter handling.The function now accepts both
simulatorUuidandsimulatorId, usingsimulatorUuid ?? simulatorIdto maintain backward compatibility while supporting the new naming convention.src/mcp/tools/simulator/build_run_sim.ts (5)
240-259: LGTM! UUID determination updated for simulatorId.The call to
determineSimulatorUuidnow passessimulatorIdinstead ofsimulatorUuid, and the returned UUID is stored in thesimulatorIdvariable for clarity.
262-333: LGTM! Simulator boot logic migrated consistently.All references to the simulator identifier updated to use
simulatorId:
- State checking (line 263)
- Device comparison (line 291)
- Boot command construction (line 316)
- Logging messages (line 323)
348-364: LGTM! App installation updated to use simulatorId.Install command construction (line 351) and logging (line 349) correctly reference
simulatorId.
437-453: LGTM! App launch updated to use simulatorId.Launch command construction (line 440) and logging (line 438) correctly reference
simulatorId.
474-480: LGTM! Next Steps guidance updated consistently.User-facing examples now use
simulatorIdparameter, aligning with the new API naming convention.src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (4)
8-10: LGTM! Session store cleanup added.Proper test hygiene with
beforeEachclearing the session store to ensure test isolation.
25-40: LGTM! Public schema validation comprehensive.Excellent testing of the session-aware public API:
- Validates that
bundleIdis required in public schema- Confirms simulator selectors are stripped from parsed data (lines 32, 39)
- Tests both
simulatorIdandsimulatorNamehandling
44-58: LGTM! Session default guidance tested.Good coverage of error scenarios:
- Missing simulator selector guidance (lines 44-50)
- BundleId validation when simulator defaults exist (lines 52-58)
This ensures users receive helpful error messages when session defaults are not configured.
60-170: LGTM! Logic tests migrated to simulatorId.All
stop_app_simLogicinvocations systematically updated to usesimulatorId, maintaining comprehensive coverage of success and error scenarios.src/mcp/tools/simulator/list_sims.ts (1)
187-192: LGTM! Instructional text updated to reflect simulatorId parameter naming.The "Next Steps" guidance now consistently uses
simulatorIdacross all example commands, aligning with the broader parameter renaming in this PR.src/mcp/tools/ui-testing/screenshot.ts (1)
21-51: LGTM! Complete and consistent migration from simulatorUuid to simulatorId.The schema, parameter destructuring, command arguments, and log messages have all been updated consistently. The
z.infertype automatically reflects the schema change.src/mcp/tools/simulator/__tests__/open_sim.test.ts (1)
64-73: LGTM! Test expectations updated to match the simulatorId parameter naming.All occurrences of
simulatorUuidin the expected "Next Steps" text have been correctly updated tosimulatorId, maintaining alignment with the tool's output.src/mcp/tools/simulator/__tests__/list_sims.test.ts (1)
124-127: LGTM! Test expectations consistently updated across all test cases.All four test cases now expect
simulatorIdin the "Next Steps" guidance, maintaining alignment with the updated tool output.Also applies to: 180-183, 242-245, 309-312
src/mcp/tools/simulator/open_sim.ts (1)
43-52: LGTM! Instructional text consistently updated to use simulatorId.All example commands in the "Next Steps" guidance now use
simulatorIdinstead ofsimulatorUuid, including boot_sim, start_sim_log_cap (with both variants), and launch_app_logs_sim.src/mcp/tools/simulator/__tests__/screenshot.test.ts (3)
35-51: LGTM! Schema validation tests updated to use simulatorId.The schema validation tests now correctly use
simulatorIdfor valid and invalid input scenarios, and verify that the field is required.
83-90: LGTM! All test cases consistently updated with simulatorId parameter.Every invocation of
screenshotLogicthroughout the test suite now usessimulatorIdinstead ofsimulatorUuid, ensuring comprehensive test coverage for the renamed parameter.Also applies to: 149-157, 207-213, 262-270, 315-323, 358-366, 406-414, 459-467, 492-500, 525-533, 568-576
284-296: LGTM! Error message validation updated to reflect simulatorId.The validation error message expectation correctly references
simulatorIdas the required field, aligning with the schema changes.src/mcp/tools/simulator/__tests__/record_sim_video.test.ts (3)
1-14: LGTM! Test setup adopts session-aware pattern.The addition of
sessionStoreimport andbeforeEachhook to clear session state before each test ensures a clean testing environment, aligning with the session-aware tool migration mentioned in the PR objectives.
21-31: LGTM! Validation tests properly set up simulator context via session store.The tests now establish a simulator context using
sessionStore.setDefaults({ simulatorId: VALID_UUID })before testing validation logic, ensuring the session-aware tool has the required context.Also applies to: 33-42
69-91: LGTM! All logic tests updated to use simulatorId parameter.All invocations of
record_sim_videoLogicnow usesimulatorIdinstead ofsimulatorUuid, maintaining consistency with the broader parameter renaming effort.Also applies to: 120-130, 133-144, 179-188
src/mcp/tools/simulator-management/__tests__/sim_statusbar.test.ts (2)
28-38: LGTM! Tests verify public schema correctly omits simulatorId.The schema validation tests confirm that:
- The public schema accepts
dataNetworkvaluessimulatorIdcan be provided but is not present in the parsed output- This aligns with the session-aware tool pattern where
simulatorIdis managed through session defaults
48-64: LGTM! All test cases comprehensively updated to use simulatorId.Every invocation of
sim_statusbarLogicthroughout the test suite now usessimulatorIdinstead ofsimulatorUuid, covering:
- Success scenarios (wifi, clear operations)
- Failure scenarios (command failures, exceptions)
- Command generation verification for both override and clear operations
Also applies to: 74-86, 94-111, 118-135, 142-159, 184-207, 232-247, 255-271
src/mcp/tools/ui-testing/__tests__/type_text.test.ts (3)
54-71: Schema rename to simulatorId looks goodValidation cases cover valid/invalid UUID and text; imports use .ts extensions and no Vitest mocking is used. Good coverage.
Also applies to: 73-88
112-128: Command args assertion is correctVerifies --udid is appended and preserves text argument verbatim.
300-319: Error-path coverage is solidDependencyError handling path is exercised without relying on vi.mock. Nice.
src/mcp/tools/ui-testing/long_press.ts (1)
136-167: Map keying by simulatorId is consistentgetCoordinateWarning uses simulatorId for session tracking; consistent with rename. LGTM.
src/mcp/tools/ui-testing/__tests__/swipe.test.ts (3)
63-86: Schema validation updated correctlyAll validations now target simulatorId. Inputs and failure modes look correct.
Also applies to: 88-98, 99-125
128-169: Command construction coverage is thoroughYou validate base, duration, all-option, and bundled-path cases with explicit --udid. Nice.
Also applies to: 171-214, 216-268, 270-320
323-331: Handler error messaging checks are preciseAsserting parameter names in the error text reduces regressions in validation formatting.
Also applies to: 333-346
src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts (2)
22-30: Good: simulatorId hidden from the public schemaParsing removes simulatorId, aligning with session-defaults pattern.
100-125: Command and log prefix assertions add valuable guaranteesCapturing argv and the logPrefix ensures stable UX.
src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (2)
36-48: Public schema correctly hides simulatorIdValidates bundleId-only inputs and strips simulatorId when present.
86-91: Review comment is incorrectThe code exhibits an intentional adapter pattern, not an inconsistency. Tools expose
simulatorIdas their public parameter but convert it tosimulatorUuidwhen calling internal utilities (log_capture, video_capture). The tests correctly verify this conversion occurs—assertingsimulatorUuidis the right expectation because they're testing that the tool properly adapts fromsimulatorIdtosimulatorUuid. This pattern is consistent across all tools: launch_app_logs_sim.ts, record_sim_video.ts, and start_sim_log_cap.ts all follow the same conversion logic. No changes are needed.Likely an incorrect or invalid review comment.
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (3)
25-44: Schema hides simulator selectors while allowing passthroughNice pattern: schema exposes bundleId/args and strips simulatorId/simulatorName. Matches session-defaults approach.
98-111: Next Steps hint uses simulatorId consistentlyGuidance strings reflect the selected identifier and are easy to follow.
322-388: Great: hints respect user’s selector (simulatorName)When simulatorName is used, all hints continue with simulatorName instead of switching to the resolved UDID.
src/mcp/tools/ui-testing/touch.ts (1)
68-86: Core logic and axe execution paths look correctValidation enforces at least one of down/up; --udid appended; error mapping covers Dependency/Axe/SystemError.
Also applies to: 142-202
src/mcp/tools/simulator-management/__tests__/set_sim_location.test.ts (1)
28-41: Nice public schema assertionThe literal checks here give solid coverage that the public schema strips
simulatorIdwhile still validating latitude/longitude payloads—exactly what the session-default contract relies on.src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts (1)
85-92: Great session-default coverageExercising the handler with no
simulatorIdand asserting on the guidance text is a nice safeguard that the session-aware wrapper responds correctly when the session defaults are absent.src/mcp/tools/simulator/__tests__/boot_sim.test.ts (1)
30-37: Session default guard looks goodThanks for asserting that the new handler path flags missing session defaults and calls out
simulatorIdexplicitly—great regression coverage for the migration.src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts (1)
42-48: Solid check on session-default enforcementThe explicit assertion that a missing
simulatorIdtriggers the session-default error keeps describe_ui aligned with the new contract—nice coverage.src/mcp/tools/ui-testing/__tests__/button.test.ts (1)
26-42: Nice public schema guard coverage.Love that the test now asserts simulatorId is stripped while still validating the rest of the payload—great regression net for the session-aware boundary.
src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts (1)
26-42: Great job mirroring the public schema contract.These assertions neatly mirror the session-default story and catch accidental reintroduction of simulatorId into the exposed shape—exactly the kind of coverage we need.
| handler: createSessionAwareTool<EraseSimsParams>({ | ||
| internalSchema: eraseSimsSchema as unknown as z.ZodType<EraseSimsParams>, | ||
| logicFunction: erase_simsLogic, | ||
| getExecutor: getDefaultCommandExecutor, | ||
| }), |
There was a problem hiding this comment.
Prevent session defaults from breaking the all: true pathway.
With a stored session default (sessionStore.setDefaults({ simulatorId: 'A' })), calling erase_sims.handler({ all: true }) now fails validation because the merged payload still contains the defaulted simulatorId, so your refine sees both selectors and rejects the request. This used to work before the session-aware migration and is a functional regression: the user can no longer erase all simulators without first clearing their defaults.
Ensure that providing all: true clears any session-sourced simulatorId before validation. One straightforward fix is to configure an exclusive pair so the factory drops the default when all is supplied:
+import type { SessionDefaults } from '../../../utils/session-store.ts';
…
handler: createSessionAwareTool<EraseSimsParams>({
internalSchema: eraseSimsSchema as unknown as z.ZodType<EraseSimsParams>,
logicFunction: erase_simsLogic,
getExecutor: getDefaultCommandExecutor,
+ exclusivePairs: [['simulatorId', 'all']] as unknown as (keyof SessionDefaults)[][],
}),That way a user-provided all removes the conflicting simulatorId coming from session defaults, and validation succeeds again. Adjust the cast/import to match the actual type export if needed.
🤖 Prompt for AI Agents
In src/mcp/tools/simulator-management/erase_sims.ts around lines 138-142,
session defaults can inject a simulatorId that conflicts with a user-supplied
all: true, causing validation to fail; update the createSessionAwareTool call so
that the factory treats the pair (all, simulatorId) as exclusive — i.e., when
all is present it should drop any session-sourced simulatorId before running
validation; implement this by supplying the exclusive-pair configuration/options
to createSessionAwareTool (or the appropriate cast/imported type) so the merged
payload removes simulatorId when all is provided, ensuring the all:true pathway
validates successfully.
| const startRes = await video.startSimulatorVideoCapture( | ||
| { simulatorUuid: params.simulatorUuid, fps: fpsUsed }, | ||
| { simulatorUuid: params.simulatorId, fps: fpsUsed }, | ||
| executor, | ||
| ); |
There was a problem hiding this comment.
Fix capture helper payload key
Both start/stop invocations still send { simulatorUuid: ... }. After the rename, startSimulatorVideoCapture/stopSimulatorVideoCapture expect simulatorId, so this call hands them undefined, causing AXe to run with a missing UDID and the tool to fail consistently. Please switch the payload key to simulatorId in both places.
- { simulatorUuid: params.simulatorId, fps: fpsUsed },
+ { simulatorId: params.simulatorId, fps: fpsUsed },
@@
- { simulatorUuid: params.simulatorId },
+ { simulatorId: params.simulatorId },Also applies to: 145-146
🤖 Prompt for AI Agents
In src/mcp/tools/simulator/record_sim_video.ts around lines 94-97 (and also
apply same change at lines 145-146), the payload uses the old key simulatorUuid
when calling startSimulatorVideoCapture/stopSimulatorVideoCapture; change the
payload key to simulatorId so the function receives the expected value (e.g., {
simulatorId: params.simulatorId, fps: fpsUsed } for start and { simulatorId:
params.simulatorId } for stop), leaving executor and other args unchanged.
cameroncooke
force-pushed
the
feat/session-sim-management
branch
from
683a467 to
bf78589
Compare
|
@cursor review |
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/mcp/tools/ui-testing/swipe.ts (2)
22-32: Incomplete migration:simulatorIdstill exposed in public schema.The PR objectives state that
simulatorIdshould be "hidden from public schemas" and the AI summary indicates apublicSchemaObjectshould be introduced that omitssimulatorId. However, the schema here still includessimulatorIdas a required field.Based on the PR summary and related tools mentioned (e.g.,
set_sim_location.ts,erase_sims.ts,describe_ui.ts), you should:
- Create a
publicSchemaObjectthat omitssimulatorIdusing.omit({ simulatorId: true })with strict mode- Use the full
swipeSchemaas theinternalSchemafor session-aware handling
120-136: Incomplete migration: still usingcreateTypedToolinstead ofcreateSessionAwareTool.The PR objectives and AI summary state this tool should be migrated to use
createSessionAwareToolto enable session-aware handling withsimulatorId. However, the code still usescreateTypedTool.Migrate the default export to use
createSessionAwareToolwith:
internalSchemaset toswipeSchema(withsimulatorId)- A public schema object that omits
simulatorId- Session-aware handler that injects
simulatorIdfrom session contextApply a pattern similar to other migrated tools in this PR:
+import { createSessionAwareTool } from '../../../utils/session-aware-tool-factory.ts'; + +// Public schema omits simulatorId (injected from session) +const publicSchemaObject = swipeSchema.omit({ simulatorId: true }).strict(); + export default { name: 'swipe', description: "Swipe from one point to another. Use describe_ui for precise coordinates (don't guess from screenshots). Supports configurable timing.", - schema: swipeSchema.shape, // MCP SDK compatibility - handler: createTypedTool( - swipeSchema, - (params: SwipeParams, executor: CommandExecutor) => { - return swipeLogic(params, executor, { - getAxePath, - getBundledAxeEnvironment, - createAxeNotAvailableResponse, - }); - }, - getDefaultCommandExecutor, - ), + schema: publicSchemaObject.shape, + handler: createSessionAwareTool<typeof swipeSchema, typeof publicSchemaObject>( + swipeSchema, + publicSchemaObject, + (params: SwipeParams, executor: CommandExecutor) => { + return swipeLogic(params, executor, { + getAxePath, + getBundledAxeEnvironment, + createAxeNotAvailableResponse, + }); + }, + getDefaultCommandExecutor, + ), };src/mcp/tools/ui-testing/tap.ts (1)
108-124: Incomplete migration: tap.ts still uses createTypedTool.This file has updated the parameter name from
simulatorUuidtosimulatorIdbut hasn't completed the session-aware tool migration that other UI-testing tools have adopted. Compare withbutton.ts(lines 76-93), which:
- Imports
createSessionAwareToolinstead ofcreateTypedTool- Defines
publicSchemaObject = buttonSchema.omit({ simulatorId: true }).strict()- Uses
createSessionAwareToolwithrequirements: [{ allOf: ['simulatorId'], message: '...' }]The current implementation directly exposes
tapSchema.shape(line 112), which includessimulatorIdin the public schema, inconsistent with the PR's objective to hidesimulatorIdfrom public schemas.Apply this pattern to complete the migration:
-import { createTypedTool } from '../../../utils/typed-tool-factory.ts'; +import { createSessionAwareTool } from '../../../utils/typed-tool-factory.ts';+const publicSchemaObject = tapSchema.omit({ simulatorId: true } as const).strict(); + export default { name: 'tap', description: "Tap at specific coordinates. Use describe_ui to get precise element coordinates (don't guess from screenshots). Supports optional timing delays.", - schema: tapSchema.shape, // MCP SDK compatibility - handler: createTypedTool( - tapSchema, - (params: TapParams, executor: CommandExecutor) => { - return tapLogic(params, executor, { - getAxePath, - getBundledAxeEnvironment, - createAxeNotAvailableResponse, - }); - }, - getDefaultCommandExecutor, - ), + schema: publicSchemaObject.shape, // MCP SDK compatibility + handler: createSessionAwareTool<TapParams>({ + internalSchema: tapSchema as unknown as z.ZodType<TapParams>, + logicFunction: (params: TapParams, executor: CommandExecutor) => + tapLogic(params, executor, { + getAxePath, + getBundledAxeEnvironment, + createAxeNotAvailableResponse, + }), + getExecutor: getDefaultCommandExecutor, + requirements: [{ allOf: ['simulatorId'], message: 'simulatorId is required' }], + }), };
♻️ Duplicate comments (2)
src/mcp/tools/simulator/record_sim_video.ts (1)
94-97: Critical: Fix payload object keys to use simulatorId.The payload objects still use the old key
simulatorUuidinstead ofsimulatorId. This causes the video capture functions to receiveundefinedfor the simulator identifier, breaking the functionality.Apply this diff:
- { simulatorUuid: params.simulatorId, fps: fpsUsed }, + { simulatorId: params.simulatorId, fps: fpsUsed },And at line 145:
- { simulatorUuid: params.simulatorId }, + { simulatorId: params.simulatorId },Also applies to: 145-146
src/mcp/tools/ui-testing/gesture.ts (1)
174-230: Duplicate ofexecuteAxeCommandfromkey_sequence.ts.This is the same helper function duplicated from
key_sequence.ts(lines 110-166). See the refactoring suggestion in that file's review to centralize this helper and eliminate duplication.
🧹 Nitpick comments (3)
src/mcp/tools/simulator/get_sim_app_path.ts (2)
193-193: Simplify redundant conditional.The ternary
(simulatorId ? false : useLatestOS)is redundant since this code is in theelse if (simulatorName)branch wheresimulatorIdis guaranteed to be undefined due to XOR validation (lines 125-130). The condition will always evaluate touseLatestOS.Apply this diff to simplify:
- destinationString = `platform=${platform},name=${simulatorName}${(simulatorId ? false : useLatestOS) ? ',OS=latest' : ''}`; + destinationString = `platform=${platform},name=${simulatorName}${useLatestOS ? ',OS=latest' : ''}`;
250-252: Align placeholder with parameter naming.The placeholder text uses "SIMULATOR_UUID" while the parameter is named
simulatorId. For consistency and clarity in user-facing documentation, consider updating the placeholder to "SIMULATOR_ID".Apply this diff:
-2. Boot simulator: boot_sim({ simulatorId: "SIMULATOR_UUID" }) -3. Install app: install_app_sim({ simulatorId: "SIMULATOR_UUID", appPath: "${appPath}" }) -4. Launch app: launch_app_sim({ simulatorId: "SIMULATOR_UUID", bundleId: "BUNDLE_ID" }) +2. Boot simulator: boot_sim({ simulatorId: "SIMULATOR_ID" }) +3. Install app: install_app_sim({ simulatorId: "SIMULATOR_ID", appPath: "${appPath}" }) +4. Launch app: launch_app_sim({ simulatorId: "SIMULATOR_ID", bundleId: "BUNDLE_ID" })src/mcp/tools/ui-testing/key_sequence.ts (1)
110-166: Consider extracting the duplicatedexecuteAxeCommandhelper.This function is duplicated across multiple UI testing tools (also in
gesture.tslines 174-230, and likely others). The comment indicates it was "inlined from src/tools/axe/index.ts". Consider re-centralizing this helper to reduce maintenance burden and ensure consistency across tools.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
docs/TOOLS.md(1 hunks)docs/session-aware-migration-todo.md(1 hunks)example_projects/iOS_Calculator/.gitignore(1 hunks)src/mcp/tools/logging/__tests__/start_sim_log_cap.test.ts(7 hunks)src/mcp/tools/logging/start_sim_log_cap.ts(3 hunks)src/mcp/tools/simulator-management/__tests__/erase_sims.test.ts(4 hunks)src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts(5 hunks)src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts(5 hunks)src/mcp/tools/simulator-management/__tests__/set_sim_location.test.ts(14 hunks)src/mcp/tools/simulator-management/__tests__/sim_statusbar.test.ts(9 hunks)src/mcp/tools/simulator-management/erase_sims.ts(3 hunks)src/mcp/tools/simulator-management/reset_sim_location.ts(5 hunks)src/mcp/tools/simulator-management/set_sim_appearance.ts(5 hunks)src/mcp/tools/simulator-management/set_sim_location.ts(5 hunks)src/mcp/tools/simulator-management/sim_statusbar.ts(2 hunks)src/mcp/tools/simulator/__tests__/boot_sim.test.ts(3 hunks)src/mcp/tools/simulator/__tests__/install_app_sim.test.ts(3 hunks)src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts(2 hunks)src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts(7 hunks)src/mcp/tools/simulator/__tests__/list_sims.test.ts(4 hunks)src/mcp/tools/simulator/__tests__/open_sim.test.ts(1 hunks)src/mcp/tools/simulator/__tests__/screenshot.test.ts(13 hunks)src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts(2 hunks)src/mcp/tools/simulator/boot_sim.ts(1 hunks)src/mcp/tools/simulator/build_run_sim.ts(9 hunks)src/mcp/tools/simulator/get_sim_app_path.ts(1 hunks)src/mcp/tools/simulator/install_app_sim.ts(1 hunks)src/mcp/tools/simulator/launch_app_logs_sim.ts(2 hunks)src/mcp/tools/simulator/launch_app_sim.ts(3 hunks)src/mcp/tools/simulator/list_sims.ts(1 hunks)src/mcp/tools/simulator/open_sim.ts(1 hunks)src/mcp/tools/simulator/record_sim_video.ts(1 hunks)src/mcp/tools/simulator/stop_app_sim.ts(3 hunks)src/mcp/tools/ui-testing/__tests__/button.test.ts(16 hunks)src/mcp/tools/ui-testing/__tests__/describe_ui.test.ts(8 hunks)src/mcp/tools/ui-testing/__tests__/gesture.test.ts(14 hunks)src/mcp/tools/ui-testing/__tests__/key_press.test.ts(12 hunks)src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts(13 hunks)src/mcp/tools/ui-testing/__tests__/long_press.test.ts(15 hunks)src/mcp/tools/ui-testing/__tests__/screenshot.test.ts(14 hunks)src/mcp/tools/ui-testing/__tests__/swipe.test.ts(16 hunks)src/mcp/tools/ui-testing/__tests__/tap.test.ts(31 hunks)src/mcp/tools/ui-testing/__tests__/touch.test.ts(23 hunks)src/mcp/tools/ui-testing/__tests__/type_text.test.ts(14 hunks)src/mcp/tools/ui-testing/button.ts(5 hunks)src/mcp/tools/ui-testing/describe_ui.ts(6 hunks)src/mcp/tools/ui-testing/gesture.ts(6 hunks)src/mcp/tools/ui-testing/key_press.ts(5 hunks)src/mcp/tools/ui-testing/key_sequence.ts(5 hunks)src/mcp/tools/ui-testing/long_press.ts(7 hunks)src/mcp/tools/ui-testing/screenshot.ts(4 hunks)src/mcp/tools/ui-testing/swipe.ts(7 hunks)src/mcp/tools/ui-testing/tap.ts(7 hunks)src/mcp/tools/ui-testing/touch.ts(7 hunks)src/mcp/tools/ui-testing/type_text.ts(5 hunks)src/utils/simulator-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- src/mcp/tools/ui-testing/tests/type_text.test.ts
- example_projects/iOS_Calculator/.gitignore
- src/mcp/tools/simulator/tests/list_sims.test.ts
- src/mcp/tools/logging/tests/start_sim_log_cap.test.ts
- src/utils/simulator-utils.ts
- src/mcp/tools/simulator/list_sims.ts
- src/mcp/tools/simulator/stop_app_sim.ts
- 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/describe_ui.test.ts
- src/mcp/tools/ui-testing/long_press.ts
- src/mcp/tools/simulator/open_sim.ts
- src/mcp/tools/simulator/tests/screenshot.test.ts
- src/mcp/tools/simulator/boot_sim.ts
- src/mcp/tools/simulator/tests/boot_sim.test.ts
- src/mcp/tools/simulator/install_app_sim.ts
- src/mcp/tools/ui-testing/screenshot.ts
- docs/session-aware-migration-todo.md
- src/mcp/tools/simulator/build_run_sim.ts
- src/mcp/tools/simulator/tests/open_sim.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/mcp/tools/simulator/get_sim_app_path.tssrc/mcp/tools/ui-testing/touch.tssrc/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/simulator-management/__tests__/erase_sims.test.tssrc/mcp/tools/ui-testing/swipe.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/simulator/record_sim_video.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/ui-testing/type_text.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/simulator-management/reset_sim_location.tssrc/mcp/tools/simulator-management/__tests__/set_sim_location.test.tssrc/mcp/tools/simulator-management/__tests__/reset_sim_location.test.tssrc/mcp/tools/simulator-management/sim_statusbar.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/simulator-management/set_sim_location.tssrc/mcp/tools/simulator-management/__tests__/sim_statusbar.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/screenshot.test.tssrc/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/logging/start_sim_log_cap.tssrc/mcp/tools/simulator-management/set_sim_appearance.tssrc/mcp/tools/simulator/launch_app_sim.tssrc/mcp/tools/ui-testing/key_press.tssrc/mcp/tools/ui-testing/key_sequence.tssrc/mcp/tools/ui-testing/gesture.tssrc/mcp/tools/simulator/launch_app_logs_sim.tssrc/mcp/tools/ui-testing/describe_ui.tssrc/mcp/tools/ui-testing/tap.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.tssrc/mcp/tools/simulator-management/erase_sims.tssrc/mcp/tools/ui-testing/button.ts
**/*.{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/mcp/tools/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/simulator-management/__tests__/erase_sims.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/simulator-management/__tests__/set_sim_location.test.tssrc/mcp/tools/simulator-management/__tests__/reset_sim_location.test.tssrc/mcp/tools/ui-testing/__tests__/tap.test.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/simulator-management/__tests__/sim_statusbar.test.tssrc/mcp/tools/ui-testing/__tests__/swipe.test.tssrc/mcp/tools/ui-testing/__tests__/screenshot.test.tssrc/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/ui-testing/__tests__/key_sequence.test.ts
🧠 Learnings (2)
📚 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/simulator/__tests__/launch_app_sim.test.tssrc/mcp/tools/simulator-management/__tests__/erase_sims.test.tssrc/mcp/tools/ui-testing/__tests__/key_press.test.tssrc/mcp/tools/simulator/__tests__/stop_app_sim.test.tssrc/mcp/tools/ui-testing/__tests__/gesture.test.tssrc/mcp/tools/simulator-management/reset_sim_location.tssrc/mcp/tools/simulator-management/sim_statusbar.tssrc/mcp/tools/simulator/__tests__/install_app_sim.test.tssrc/mcp/tools/simulator-management/set_sim_location.tssrc/mcp/tools/ui-testing/__tests__/screenshot.test.tssrc/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.tssrc/mcp/tools/logging/start_sim_log_cap.tssrc/mcp/tools/simulator-management/set_sim_appearance.tssrc/mcp/tools/ui-testing/key_sequence.tssrc/mcp/tools/ui-testing/__tests__/button.test.tssrc/mcp/tools/simulator-management/erase_sims.ts
📚 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 : Do not use Vitest mocking APIs in tests (no vi.mock, vi.fn, vi.spyOn, etc.)
Applied to files:
src/mcp/tools/ui-testing/__tests__/button.test.ts
🧬 Code graph analysis (20)
src/mcp/tools/ui-testing/touch.ts (2)
src/utils/logger.ts (1)
log(142-162)src/utils/logging/index.ts (1)
log(5-5)
src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (1)
src/mcp/tools/simulator/launch_app_sim.ts (1)
launch_app_simLogic(40-191)
src/mcp/tools/simulator-management/__tests__/erase_sims.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator-management/erase_sims.ts (1)
erase_simsLogic(21-79)
src/mcp/tools/ui-testing/swipe.ts (1)
src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (1)
src/mcp/tools/simulator/stop_app_sim.ts (1)
stop_app_simLogic(37-145)
src/mcp/tools/ui-testing/type_text.ts (1)
src/utils/logger.ts (1)
log(142-162)
src/mcp/tools/simulator-management/reset_sim_location.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator-management/sim_statusbar.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator/__tests__/install_app_sim.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockFileSystemExecutor(165-181)src/mcp/tools/simulator/install_app_sim.ts (1)
install_app_simLogic(24-94)
src/mcp/tools/simulator-management/set_sim_location.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts (2)
src/test-utils/mock-executors.ts (1)
createMockExecutor(27-75)src/mcp/tools/simulator-management/set_sim_appearance.ts (1)
set_sim_appearanceLogic(72-88)
src/mcp/tools/logging/start_sim_log_cap.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (1)
getDefaultCommandExecutor(199-210)
src/mcp/tools/simulator-management/set_sim_appearance.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/ui-testing/key_press.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/key_sequence.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/gesture.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/describe_ui.ts (3)
src/utils/logger.ts (1)
log(142-162)src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
src/mcp/tools/ui-testing/tap.ts (2)
src/utils/logger.ts (1)
log(142-162)src/utils/logging/index.ts (1)
log(5-5)
src/mcp/tools/simulator-management/erase_sims.ts (1)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)
src/mcp/tools/ui-testing/button.ts (2)
src/utils/typed-tool-factory.ts (1)
createSessionAwareTool(74-176)src/utils/command.ts (2)
CommandExecutor(20-20)getDefaultCommandExecutor(199-210)
⏰ 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cursor Bugbot
🔇 Additional comments (49)
src/mcp/tools/simulator/__tests__/install_app_sim.test.ts (3)
33-39: LGTM! Correct validation of session-aware schema behavior.The test properly validates that
simulatorIdis accepted as input but stripped from parsed data, which aligns with the session-aware tool migration pattern.
177-229: Excellent coverage of bundle ID extraction failure path.This test properly validates graceful degradation when bundle ID extraction fails, ensuring users still receive helpful Next Steps guidance with a placeholder.
231-283: LGTM! Proper validation of successful bundle ID extraction.The test correctly validates the happy path where bundle ID extraction succeeds, and line 282's assertion of 2 executor calls accurately reflects the implementation (install command + bundle ID extraction).
src/mcp/tools/ui-testing/__tests__/screenshot.test.ts (1)
1-449: LGTM! Clean parameter rename throughout test file.The migration from
simulatorUuidtosimulatorIdis consistent and complete across all test cases. The test logic, command generation verification, error handling, and assertions remain unchanged—this is a clean mechanical refactor. The file follows all coding guidelines: no Vitest mocking APIs, uses the mock executor pattern withcreateMockExecutor/createMockFileSystemExecutor, and imports use.tsextensions.src/mcp/tools/ui-testing/__tests__/swipe.test.ts (1)
1-563: LGTM! Test file correctly updated forsimulatorIdrename.All test cases have been consistently updated to use
simulatorIdinstead ofsimulatorUuid. The changes include:
- Schema validation tests (lines 69, 77-86, 91-97, 102-112, 117-124)
- Command generation tests with various parameter combinations (lines 145, 187, 232, 293)
- Handler behavior tests for success and error scenarios (lines 324-561)
The test coverage remains comprehensive and the updates align with the parameter rename.
src/mcp/tools/simulator/launch_app_logs_sim.ts (1)
29-33: LGTM! Public schema correctly configured.The publicSchemaObject correctly omits simulatorId from the public surface and applies strict validation, following the consistent pattern across the PR.
src/mcp/tools/ui-testing/__tests__/tap.test.ts (1)
67-67: LGTM! Test cases correctly migrated to simulatorId.All test inputs and assertions have been consistently updated from simulatorUuid to simulatorId throughout the file.
Also applies to: 168-168, 356-356, 445-445
src/mcp/tools/simulator/__tests__/stop_app_sim.test.ts (1)
26-35: LGTM! Session defaults test correctly validates schema behavior.The test correctly verifies that when simulatorId and simulatorName are provided as session defaults, they are accepted by the schema but omitted from the parsed result, ensuring the public API surface properly hides internal session parameters.
src/mcp/tools/ui-testing/__tests__/key_press.test.ts (1)
30-47: LGTM! Public schema test correctly validates session-aware behavior.The test properly verifies that the public schema accepts simulatorId as input but omits it from the parsed data object, consistent with the session-aware tool pattern.
src/mcp/tools/ui-testing/__tests__/button.test.ts (1)
26-42: LGTM! Public schema validation test correctly implemented.The test properly validates that the public schema accepts simulatorId but omits it from parsed data, and the unused
beforeEachimport has been correctly removed.src/mcp/tools/simulator-management/set_sim_location.ts (1)
118-129: LGTM! Session-aware tool migration correctly implemented.The migration to
createSessionAwareToolis properly configured with the public schema omitting simulatorId, the internal schema retaining full validation, and requirements correctly enforcing simulatorId presence.src/mcp/tools/simulator/record_sim_video.ts (1)
221-221: LGTM! Strict validation correctly added.The addition of
.strict()to the public schema object correctly enforces that no unknown keys are accepted in the public API surface.src/mcp/tools/simulator-management/__tests__/sim_statusbar.test.ts (2)
28-38: LGTM: Schema validation correctly tests simulatorId privacy.The test properly verifies that
simulatorIdis accepted as input but hidden from the parsed data object, aligning with the session-aware tool pattern.
42-271: LGTM: Test cases consistently updated to simulatorId.All test invocations, command generation tests, and assertions correctly reference
simulatorIdinstead ofsimulatorUuid. The expected command arrays and success messages properly incorporate the new parameter name.src/mcp/tools/simulator/__tests__/launch_app_logs_sim.test.ts (2)
40-46: LGTM: Schema validation correctly tests simulatorId privacy.The new test case properly validates that
simulatorIdcan be provided in input but is not exposed in the parsed data, consistent with the session-aware tool pattern.
114-143: LGTM: Args passthrough correctly tested with internal parameter mapping.The test correctly validates that
argsare passed through to log capture setup. Note that the test acceptssimulatorIdas public input (line 130) but the captured params showsimulatorUuid(line 139), indicating the logic function correctly maps public parameter names to internal implementation names.src/mcp/tools/ui-testing/__tests__/gesture.test.ts (3)
30-55: LGTM: Schema validation correctly tests simulatorId privacy.The test properly validates that
simulatorIdis accepted as input but excluded from parsed data, and that invalid presets are rejected. This aligns with the session-aware tool pattern.
59-221: LGTM: Command generation tests correctly use simulatorId.All command generation tests properly pass
simulatorIdas input and verify it appears in the--udidargument position in the generated axe commands.
224-432: LGTM: Handler behavior tests consistently updated.All handler behavior tests correctly use
simulatorIdin test inputs across success, error, and edge case scenarios.src/mcp/tools/simulator-management/__tests__/reset_sim_location.test.ts (2)
22-30: LGTM: Schema validation correctly allows empty input.The test properly validates that the public schema accepts an empty object (relying on session defaults) and that
simulatorIdis hidden from parsed data when provided.
34-125: LGTM: All test cases correctly use simulatorId.Handler behavior and command generation tests consistently use
simulatorIdin inputs and verify correct command construction withsimulatorIdin the appropriate position.src/mcp/tools/ui-testing/touch.ts (4)
24-50: LGTM: Schema and parameter handling updated to simulatorId.The schema definition and parameter destructuring correctly use
simulatorIdthroughout.
69-108: LGTM: Logging and error handling consistently use simulatorId.All logging statements and error response creation properly reference
simulatorId.
119-140: LGTM: Session tracking updated to simulatorId.The
DescribeUISessiontype andgetCoordinateWarningfunction correctly usesimulatorIdfor session tracking and warning generation.
143-201: LGTM: Command execution helper updated to simulatorId.The
executeAxeCommandhelper function correctly usessimulatorIdin its signature, command construction (--udid), and error handling (AxeErrorconstruction).src/mcp/tools/simulator-management/__tests__/set_sim_location.test.ts (3)
28-42: LGTM: Schema validation correctly tests parameter requirements.The test properly validates that both
latitudeandlongitudeare required, and thatsimulatorIdis hidden from parsed data when provided.
45-140: LGTM: Command generation tests correctly use simulatorId.All command generation tests consistently use
simulatorIdin inputs and verify correct command construction withsimulatorIdin the proper position forxcrun simctl location.
143-386: LGTM: Response processing tests comprehensive and correct.All response processing tests, including success, validation failures, command failures, and boundary value tests, consistently use
simulatorIdin inputs and expected outputs.src/mcp/tools/ui-testing/type_text.ts (2)
26-56: LGTM: Schema, parameters, and logging updated to simulatorId.The schema definition, parameter destructuring, and all logging statements correctly use
simulatorIdthroughout.
91-149: LGTM: Command execution helper updated to simulatorId.The
executeAxeCommandhelper function correctly usessimulatorIdin its signature, command construction (--udidargument), and error handling (AxeErrorconstruction), consistent with the pattern in other UI testing tools.src/mcp/tools/simulator-management/__tests__/erase_sims.test.ts (2)
6-24: LGTM: Test suite updated for single-simulator mode.The test suite name, description, and schema validation correctly reflect the narrowed scope to single-simulator erasure by UDID, with optional
shutdownFirstparameter.
27-67: LGTM: All test cases consistently use simulatorId.All test cases correctly use
simulatorIdin inputs, expected outputs, and tool hint messages. The removal of all-mode tests aligns with the PR's focus on single-simulator operations with session defaults.src/mcp/tools/simulator/__tests__/launch_app_sim.test.ts (2)
40-48: LGTM: Proper validation of public schema behavior.The test correctly verifies that
simulatorIdandsimulatorNameare accepted as inputs but stripped from the parsed data, which aligns with the session-aware tool pattern where these parameters are hidden from the public schema.
130-171: LGTM: Command sequence properly verified.The test now properly collects commands and verifies the full sequence:
get_app_containercheck followed by thelaunchcommand. This matches the implementation logic in the relevant code snippet.src/mcp/tools/simulator-management/__tests__/set_sim_appearance.test.ts (1)
22-32: LGTM: Public schema correctly omits simulatorId.The test properly validates that while
simulatorIdcan be passed as input, it's stripped from the parsed data in the public schema, consistent with the session-aware tool pattern.src/mcp/tools/ui-testing/__tests__/key_sequence.test.ts (1)
26-44: LGTM: Public schema validation is correct.The test properly verifies that
simulatorIdis accepted as input but excluded from parsed data, maintaining the session-aware tool contract.src/mcp/tools/simulator-management/reset_sim_location.ts (1)
88-102: LGTM: Correctly implements session-aware tool pattern.The migration is complete and consistent:
- Public schema properly omits
simulatorId(lines 88-90)- Session-aware wrapper correctly configured with requirements (lines 96-102)
- All internal references updated to
simulatorIdsrc/mcp/tools/simulator-management/set_sim_appearance.ts (1)
90-102: LGTM: Complete session-aware migration.This file correctly implements the session-aware tool pattern with proper public schema separation and requirements enforcement. All references consistently updated to
simulatorId.src/mcp/tools/simulator-management/sim_statusbar.ts (1)
91-104: LGTM: Session-aware pattern correctly applied.The migration is complete with proper separation of public/internal schemas and consistent use of
simulatorIdthroughout the command construction and messaging.src/mcp/tools/ui-testing/button.ts (1)
76-94: LGTM: Exemplary session-aware tool migration.This file demonstrates the complete and correct migration pattern:
- Public schema properly omits
simulatorId(line 76)- Session-aware wrapper with requirements enforcement (lines 83-93)
- All internal references consistently use
simulatorIdThis serves as a good reference for other UI-testing tools.
src/mcp/tools/ui-testing/describe_ui.ts (1)
117-126: Session-aware handler wiring LGTM.Good job swapping in
createSessionAwareToolwith the internal schema and simulatorId requirement; this lines up cleanly with the shared factory contract.src/mcp/tools/logging/start_sim_log_cap.ts (1)
35-42: Nice UUID-to-legacy bridge.Mapping
simulatorIdback tosimulatorUuidforstartLogCapturekeeps the runtime stable while exposing the new session-aware API.src/mcp/tools/simulator-management/erase_sims.ts (1)
57-68: Helpful shutdown hint.The conditional hint when the device is still booted is a great touch for guiding users toward
shutdownFirst: true.src/mcp/tools/ui-testing/key_press.ts (1)
88-98: Session default enforcement looks solid.Requiring
simulatorIdvia the session-aware wrapper keeps the tool consistent with the rest of the AXe surface.src/mcp/tools/simulator/launch_app_sim.ts (1)
204-213: Exclusive pair handling looks great.Letting explicit args clear conflicting session defaults for
simulatorId/simulatorNameavoids the old validation traps.src/mcp/tools/ui-testing/key_sequence.ts (2)
24-34: LGTM! Schema migration is correct.The migration from
simulatorUuidtosimulatorIdis complete and consistent. The UUID validation and type inference are correctly implemented.
90-107: Session-aware tool setup is correct.The public schema correctly omits
simulatorIdand the handler properly enforces its presence via requirements. The type castingas unknown as z.ZodType<KeySequenceParams>on line 97 appears heavy-handed but is likely necessary due to TypeScript's type inference limitations withZodObjectvsZodType.src/mcp/tools/ui-testing/gesture.ts (2)
25-83: LGTM! Schema migration is consistent and correct.The migration to
simulatorIdwith UUID validation is properly implemented. The schema appropriately validates all gesture parameters including optional screen dimensions, duration, delta, and delay values.
153-171: Session-aware tool setup follows the established pattern.The configuration mirrors the approach in
key_sequence.tswith proper public schema derivation, simulatorId requirement enforcement, and correct handler wiring. The type casting on line 161 is consistent with the pattern used across UI testing tools.
| **Purpose**: Tools for managing simulators from booting, opening simulators, listing simulators, stopping simulators, erasing simulator content and settings, and setting simulator environment options like location, network, statusbar and appearance. (5 tools) | ||
|
|
||
| - `erase_sims` - Erases simulator content and settings. Provide exactly one of: simulatorUdid or all=true. Optional: shutdownFirst to shut down before erasing. | ||
| - `erase_sims` - Erases simulator content and settings for a specific simulator. Requires simulatorUdid. Optional: shutdownFirst to shut down before erasing. |
There was a problem hiding this comment.
Fix documentation to use simulatorId instead of simulatorUdid.
The documentation references simulatorUdid, but this PR consistently migrates the codebase to use simulatorId. Update the description to match.
Apply this diff:
-- `erase_sims` - Erases simulator content and settings for a specific simulator. Requires simulatorUdid. Optional: shutdownFirst to shut down before erasing.
+- `erase_sims` - Erases simulator content and settings for a specific simulator. Requires simulatorId. Optional: shutdownFirst to shut down before erasing.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `erase_sims` - Erases simulator content and settings for a specific simulator. Requires simulatorUdid. Optional: shutdownFirst to shut down before erasing. | |
| - `erase_sims` - Erases simulator content and settings for a specific simulator. Requires simulatorId. Optional: shutdownFirst to shut down before erasing. |
🤖 Prompt for AI Agents
In docs/TOOLS.md around line 72, the description for `erase_sims` uses the old
parameter name `simulatorUdid`; update the text to reference `simulatorId`
instead (e.g., "Requires simulatorId. Optional: shutdownFirst to shut down
before erasing.") so the documentation matches the codebase rename.
1 participant
Note
Migrates simulator and UI-testing tools to session-aware APIs, replacing
simulatorUuidwithsimulatorId, hiding IDs from public schemas, and updating simulator-management, logging, workflows, docs, and tests.createSessionAwareTool) with requiredsimulatorIdfrom session defaults; remove it from public schemas.simulatorUuid→simulatorIdacross code and tests.erase_simssimplified to single-UDID erase with optionalshutdownFirst; updated messages/hints.set_sim_location,reset_sim_location,set_sim_appearance,sim_statusbarto session-aware schemas/handlers.start_sim_log_capmade session-aware;launch_app_logs_simpasses throughargsand uses console capture by default.boot_sim,install_app_sim,launch_app_sim,stop_app_sim,build_run_sim,get_sim_app_path,list_sims,open_sim,record_sim_videoto use/showsimulatorIdand adjust next-step guidance.button,describe_ui,gesture,key_press,key_sequenceto session-aware; keep others validated againstsimulatorIdand update messaging.erase_simsdescription)..gitignorefor build artifacts.Written by Cursor Bugbot for commit bf78589. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Documentation
Refactor
Chores