Integrate mobile mcp as microservice #17
Conversation
- Fixed E2E test hangs by racing navigation + API calls with Promise.all() - Replaced 3-test suite with single deterministic run-validation test - Added data-testid attributes to run-events and discovered-screens - Updated frontend development + debugging skills with E2E patterns - Integrated spec-kit as submodule for spec-driven development - Added Appium startup script for automated mobile testing Test result: ✅ 1 passed (5.7s) Resolves: BUG-015, BUG-014 validation
Replaces direct Appium integration with a dedicated microservice for mobile device automation. This improves reliability and provides a typed API for mobile operations. Co-authored-by: niranjankurambhatti <niranjankurambhatti@gmail.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
backend/mobile/session-repo.ts
Outdated
| const whereClause = conditions.join(" AND "); | ||
|
|
||
| const result = await db.queryRow<{ | ||
| device_id: string; | ||
| name: string; | ||
| platform: "android" | "ios"; | ||
| device_type: "real" | "emulator" | "simulator"; | ||
| version: string; | ||
| screen_width: number | null; | ||
| screen_height: number | null; | ||
| orientation: "portrait" | "landscape" | null; | ||
| }>` | ||
| SELECT device_id, name, platform, device_type, version, | ||
| screen_width, screen_height, orientation | ||
| FROM device_info | ||
| WHERE ${whereClause} | ||
| ORDER BY last_seen_at DESC | ||
| LIMIT 1 |
There was a problem hiding this comment.
The allocation lookup builds a whereClause containing placeholders like $1 and collects the values in a local params array, but the array is never passed to db.queryRow. When createSession supplies a platform, device type, or provider, Postgres receives a query that references $1/$2 without any bound values and fails with a parameter-mismatch error before any device is returned. The query needs to be constructed with tagged template literals (e.g. composing SQL`` fragments) or otherwise supply the collected parameters to db.queryRow`.
Useful? React with 👍 / 👎.
| /** | ||
| * Create a new device session. | ||
| */ | ||
| export const createSession = api( | ||
| { expose: true, method: "POST", path: "/mobile/sessions/create" }, | ||
| async (req: CreateSessionRequest): Promise<CreateSessionResponse> => { | ||
| logger.info("creating device session", { allocation: req.allocation }); | ||
|
|
||
| const sessionRepo = getDeviceSessionRepository(); | ||
|
|
||
| // For now, find first available device matching criteria | ||
| // In future, implement smart allocation with AWS Device Farm integration | ||
| const device = await sessionRepo.findAvailableDevice(req.allocation); | ||
|
|
||
| if (!device) { | ||
| throw new Error(`No available device matching criteria: ${JSON.stringify(req.allocation)}`); | ||
| } | ||
|
|
||
| const session = await sessionRepo.createSession(device.id, { | ||
| allocation: req.allocation, |
There was a problem hiding this comment.
Device selection filters on available = TRUE, but createSession only inserts a row in device_sessions and never updates device_info.available when a device is allocated (nor when a session closes). As a result every call to createSession can return the same physical device even while an earlier session is active, because the availability flag is never cleared. The allocation path should mark the chosen device unavailable and restore it when the session ends to avoid concurrent sessions contending for the same hardware.
Useful? React with 👍 / 👎.
- Add mobile MCP client and session management - Implement mobile session repository with database persistence - Create mobile service with WebDriver session lifecycle - Add mobile session adapter for agent integration - Update agent state and device setup nodes for mobile support - Add database migration for mobile sessions table - Update environment config for mobile MCP settings - Add comprehensive mobile integration tests - Update package.json with mobile service dependencies - Create detailed implementation documentation and status tracking
| screenWidth: result.screen_width || undefined, | ||
| screenHeight: result.screen_height || undefined, | ||
| orientation: result.orientation || undefined, | ||
| }; |
There was a problem hiding this comment.
Bug: Unreachable Code: A Ghost in the Machine
The findAvailableDevice method contains unreachable dead code after an early return statement. Lines 284-297 will never execute because the function returns at line 282 within the for await loop. This dead code appears to be a duplicate/leftover implementation that contradicts the working loop-based logic above it.
| retryable: isRetryable, | ||
| humanReadableFailureSummary: errorMessage, | ||
| mobileSessionId: null, | ||
| mobileDeviceId: null, |
There was a problem hiding this comment.
Bug: Device Identity Lost During Failures
In the failure path, mobileDeviceId is set to null but should preserve the device name from deviceConfiguration.deviceName like in the success path. This loses device identification information when failures occur, making debugging harder since the device ID won't be tracked in failure outputs.
|
|
||
| # Expected Run Metrics (for deterministic testing) | ||
| EXPECTED_UNIQUE_SCREENS_DISCOVERED=1 | ||
| ENABLE_MOBILE_MCP=true |
There was a problem hiding this comment.
Bug: Experimental Feature Flag Enabled By Default
The feature flag ENABLE_MOBILE_MCP=true is enabled by default in the committed .env file. According to the PR description, the mobile-mcp integration is intended to replace Appium integration, but the implementation shows it's still experimental with pending agent adapter migration and end-to-end testing. Enabling this by default could break existing functionality since the agent integration is incomplete, and the .env file header explicitly states it's committed to git, affecting all developers.
- Updated dependencies for mobile-mcp and modelcontextprotocol/sdk to specific versions. - Added mobile session management with new session.adapter.ts for lifecycle coordination. - Enhanced agent state to track mobile session and device IDs. - Implemented device session repository for managing active sessions and logging operations. - Created comprehensive mobile service with 25+ REST API endpoints for device management. - Added database migrations for mobile session tracking. - Introduced feature flag `ENABLE_MOBILE_MCP` for conditional integration. - Completed integration tests and ensured all backend tests are passing. This commit establishes a robust mobile device automation service, replacing direct Appium integration and improving reliability.
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
| AND (${request.provider !== undefined} = FALSE OR provider = ${request.provider || null}) | ||
| ORDER BY last_seen_at DESC | ||
| LIMIT 1 | ||
| `; |
There was a problem hiding this comment.
Bug: Boolean String Injection Breaks SQL Queries
The WHERE clause injects boolean values into SQL template literals, converting them to string literals "true" or "false" rather than SQL boolean expressions. This produces invalid SQL syntax like "false" = FALSE when optional fields (deviceType, provider, version) are undefined. The logic should evaluate these conditionals outside the template literal or use proper SQL conditional expressions to correctly skip optional filters when not provided.
Integrate
mobile-mcpas a new Encore microservice to replace direct Appium integration and resolve persistent device connection issues.The existing Appium integration was causing reliability issues, such as shell stalls (BUG-011) and agent stalls on privacy consent dialogs (BUG-015).
mobile-mcpoffers a more robust and battle-tested solution for device I/O, providing better error handling, cross-platform abstraction, and a foundation for improved observability and future scalability. This PR establishes the newbackend/mobileservice with 25+ typed APIs, session management, and an audit log to address these problems.Note
Adds comprehensive Mobile-MCP integration documentation (spec, plan, tasks, test plan, decisions, summaries) and updates handoff notes.
spec.md,plan.md, andanalysis.mddefining requirements, phases, and rationale.tasks.md,PHASE_2_OPERATIONS_TASKS.md, andTASK_CREATION_TEMPLATE.mdwith detailed task breakdowns and exit criteria.EXECUTIVE_SUMMARY.md,COMPLETION_SUMMARY.md,ALIGNMENT_CHECKPOINT.md, andrecommendation.mdsummarizing progress, alignment, and next steps.bugs.md(fixed/known issues) andtest-plan.md(unit/integration/E2E, benchmarks).decision.mdcapturing incremental integration approach.specs/003-coding-agent-optimization/BACKEND_HANDOFF_DEPRECATED.mdwith a new handoff entry describing the Mobile-MCP microservice work and pending integration steps.Written by Cursor Bugbot for commit 7525bab. This will update automatically on new commits. Configure here.