feat: Add getCurrentActivity tool for unified app/activity detection#266
Open
sh3lan93 wants to merge 8 commits intomobile-next:mainfrom
Open
feat: Add getCurrentActivity tool for unified app/activity detection#266sh3lan93 wants to merge 8 commits intomobile-next:mainfrom
sh3lan93 wants to merge 8 commits intomobile-next:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds Robot.getCurrentActivity() and implements it for Android, iOS (device + simulator) using ADB and WebDriverAgent helpers; exposes server tool Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🧹 Nitpick comments (4)
src/mobile-device.ts (1)
217-230: Replaceas anywith a typed interface, consistent with every other response type in this file.All other command responses (
InstalledAppsResponse,DeviceInfoResponse,DumpUIResponse,OrientationResponse) have explicit interfaces. Usingas anyon Line 219 bypasses TypeScript's type-safety, and a malformeddata.id(e.g., a number) would silently pass through.♻️ Proposed refactor
+interface CurrentActivityResponse { + status: "ok" | string; + data?: { + id: string; + }; +} public async getCurrentActivity(): Promise<{ id: string }> { try { - const response = JSON.parse(this.runCommand(["device", "get-current-activity"])) as any; + const response = JSON.parse(this.runCommand(["device", "get-current-activity"])) as CurrentActivityResponse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mobile-device.ts` around lines 217 - 230, The JSON parse in getCurrentActivity currently uses "as any" which bypasses types; define and use a proper response interface (e.g., GetCurrentActivityResponse with status: string and data?: { id?: string }) consistent with InstalledAppsResponse/DeviceInfoResponse naming, replace the "as any" cast with that interface when assigning response, and validate that response.data?.id is a string (not a number) before returning to preserve type-safety and avoid silent acceptance of malformed ids.test/current-activity.ts (3)
43-43: Replace magic-number sleep with a retry/poll for focusA fixed 500 ms delay is both racy on slow devices and wasteful on fast ones. Polling
getCurrentActivity()until the returned id matchestestAppPackage(with a timeout) is more robust.♻️ Proposed alternative
-// Small delay to ensure app is focused -await new Promise(resolve => setTimeout(resolve, 500)); +// Poll until the app is focused (up to 5 s) +const deadline = Date.now() + 5000; +let activity = await android.getCurrentActivity(); +while (activity.id !== testAppPackage && Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, 200)); + activity = await android.getCurrentActivity(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/current-activity.ts` at line 43, Replace the fixed 500ms sleep with a retry/poll loop that repeatedly calls getCurrentActivity() until the returned activity id equals testAppPackage or a configurable timeout elapses; update the async flow where await new Promise(resolve => setTimeout(resolve, 500)) appears to instead poll getCurrentActivity() at short intervals (e.g., 50–100ms), aborting and throwing or failing the test if the timeout is reached, and ensure the implementation uses the existing getCurrentActivity() and testAppPackage identifiers so the test reliably waits for the app to be focused without a magic-number delay.
49-53: Remove or convert the permanently-skipped test toit.skipA test body whose sole content is
this.skip()is dead code — Mocha registers it as permanently pending with no path to activation. Either delete it entirely or useit.skip(...)to make the intent explicit and reduce confusion.♻️ Proposed fix
-it("should throw ActionableError if no activity in focus", async function() { - // This test is skipped as it's difficult to reliably create a state - // where no activity is in focus on a real device - this.skip(); -}); +// TODO: Add test for no-activity-in-focus once a reliable setup mechanism exists. +it.skip("should throw ActionableError if no activity in focus", () => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/current-activity.ts` around lines 49 - 53, The test case "should throw ActionableError if no activity in focus" contains only this.skip() making it permanently dead; either remove the entire it(...) block or convert it to an explicit pending test by replacing the current it(...) declaration with it.skip("should throw ActionableError if no activity in focus", ...) so Mocha records it as intentionally skipped; locate the test in test/current-activity.ts (the it(...) with that title) and perform one of those two changes to remove the dead this.skip() usage.
70-76: String-based error message matching is fragile and can swallow real failuresTwo risks:
- If the
ActionableErrormessage wording changes, the guard stops matching and the test fails with a confusing assertion error instead of a skip.- If something other than an
Erroris thrown (e.g., a plain string),error.messageisundefinedand.includes()itself throws, masking the original problem.Consider checking
error.constructor.name === "ActionableError"(or whatever discriminator the class exposes), or at minimum guard against non-Error throws:♻️ Proposed defensive catch
-} catch (error: any) { - // Skip if tunnel is not running or other setup issues - if (error.message.includes("tunnel") || error.message.includes("WebDriver")) { - this.skip(); - } else { - throw error; - } -} +} catch (error: unknown) { + const msg = error instanceof Error ? error.message : String(error); + if (msg.includes("tunnel") || msg.includes("WebDriver")) { + this.skip(); + } else { + throw error; + } +}Also applies to: 96-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/current-activity.ts` around lines 70 - 76, Replace fragile string-matching in the catch block around the test setup (where this.skip() is called) with a robust discriminator: first ensure the thrown value is an Error-like object (e.g., typeof error === "object" && error !== null), then either use instanceof ActionableError (or check error.constructor?.name === "ActionableError") to decide to call this.skip(), falling back to safe message checks only if message is present (e.g., typeof error.message === "string" && (error.message.includes("tunnel") || error.message.includes("WebDriver"))); apply the same change to the similar catch at lines 96-102 so non-Error throws no longer cause .includes() to throw and real failures are not swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/android.ts`:
- Around line 482-486: The fallback using mFocusedActivity is effectively dead
on Android 9+ and mCurrentFocus can point to IME/system windows, so update the
logic in the method handling dumpsys parsing (the branch that currently checks
mResumedActivity, then mCurrentFocus, then the mFocusedActivity regex) to prefer
mResumedActivity first, then fall back to mCurrentFocus, and remove or demote
the mFocusedActivity regex (the match against
/mFocusedActivity=ActivityRecord\{... u0 ...\//) so it is only used behind an
explicit Android API < 28 guard if you must keep it; also document/handle that
mCurrentFocus may return IME/system windows so callers needing the user
foreground app should use mResumedActivity.
- Around line 471-486: The three regexes that parse dumpsysOutput (the matches
for mCurrentFocus, mResumedActivity, and mFocusedActivity that assign to
focusMatch) hardcode " u0 " and thus miss activities for other users; update
each pattern to accept any user id (e.g., replace the literal " u0 " with "
u\\d+ " or a more permissive " u\\d+ " token) so the matches (in the code paths
that use focusMatch and return { id: ... }) succeed for secondary users/work
profiles when parsing dumpsysOutput.
In `@src/ios.ts`:
- Around line 233-270: Duplicate getCurrentActivity logic and a flawed
type-fallback are present; extract a shared helper (e.g., create function
parseWdaPageSourceForAppId in webdriver-agent.ts) and have both
IosRobot.getCurrentActivity and Simctl.getCurrentActivity call it. Implement
parseWdaPageSourceForAppId(pageSource: any): string to (1) prefer
pageSource.value.rawIdentifier, (2) fallback to the app name
(pageSource.value.name) if present, and (3) only use pageSource.value.type to
extract a bundle id if it contains a dot (split on the first '.' and return the
left part) so you avoid matching plain XCUIElementTypeApplication; if none
found, throw an ActionableError. Update both getCurrentActivity methods to
delegate to this helper and preserve their existing try/catch behavior.
In `@test/current-activity.ts`:
- Around line 6-12: The device-guard variables hasAndroidDevice and hasIosDevice
use strict equality to 1 which incorrectly fails when multiple devices are
connected; change the checks in the current-activity logic to test for > 0
(e.g., replace androidDevices.length === 1 with androidDevices.length > 0 and
iosDevices.length === 1 with iosDevices.length > 0) so the existing code that
uses device arrays (like indexing [0] for deviceId) still runs when at least one
device is present.
- Around line 80-104: The assertions in the "should return bundle ID format"
test are too weak: update the checks that use activity.id and newActivity.id
(from ios.getCurrentActivity()) to validate actual bundle-ID format and exact
Safari bundle id; specifically assert activity.id matches a reverse-DNS pattern
(e.g., contains a dot using a regex like /\./) and after calling
ios.launchApp("com.apple.mobilesafari") assert newActivity.id ===
"com.apple.mobilesafari" so the test fails on wrong or unexpected identifiers.
---
Nitpick comments:
In `@src/mobile-device.ts`:
- Around line 217-230: The JSON parse in getCurrentActivity currently uses "as
any" which bypasses types; define and use a proper response interface (e.g.,
GetCurrentActivityResponse with status: string and data?: { id?: string })
consistent with InstalledAppsResponse/DeviceInfoResponse naming, replace the "as
any" cast with that interface when assigning response, and validate that
response.data?.id is a string (not a number) before returning to preserve
type-safety and avoid silent acceptance of malformed ids.
In `@test/current-activity.ts`:
- Line 43: Replace the fixed 500ms sleep with a retry/poll loop that repeatedly
calls getCurrentActivity() until the returned activity id equals testAppPackage
or a configurable timeout elapses; update the async flow where await new
Promise(resolve => setTimeout(resolve, 500)) appears to instead poll
getCurrentActivity() at short intervals (e.g., 50–100ms), aborting and throwing
or failing the test if the timeout is reached, and ensure the implementation
uses the existing getCurrentActivity() and testAppPackage identifiers so the
test reliably waits for the app to be focused without a magic-number delay.
- Around line 49-53: The test case "should throw ActionableError if no activity
in focus" contains only this.skip() making it permanently dead; either remove
the entire it(...) block or convert it to an explicit pending test by replacing
the current it(...) declaration with it.skip("should throw ActionableError if no
activity in focus", ...) so Mocha records it as intentionally skipped; locate
the test in test/current-activity.ts (the it(...) with that title) and perform
one of those two changes to remove the dead this.skip() usage.
- Around line 70-76: Replace fragile string-matching in the catch block around
the test setup (where this.skip() is called) with a robust discriminator: first
ensure the thrown value is an Error-like object (e.g., typeof error === "object"
&& error !== null), then either use instanceof ActionableError (or check
error.constructor?.name === "ActionableError") to decide to call this.skip(),
falling back to safe message checks only if message is present (e.g., typeof
error.message === "string" && (error.message.includes("tunnel") ||
error.message.includes("WebDriver"))); apply the same change to the similar
catch at lines 96-102 so non-Error throws no longer cause .includes() to throw
and real failures are not swallowed.
- Android: support multi-user/work profiles (u\d+) and prioritize mResumedActivity - iOS: extract shared WDA page source parsing to parseWdaPageSourceForAppId - Tests: strengthen bundle ID validation and add retry loop for foreground detection
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/webdriver-agent.ts (1)
469-481: Deadtypefallback branch with misleading stream-of-consciousness comments.For any standard WDA page source,
rootElement.typeis"XCUIElementTypeApplication"— no dots — sorootElement.type.includes(".")is alwaysfalseand this branch is unreachable. Additionally, lines 472–479 contain contradictory developer notes ("Actually…", "Wait, bundle IDs also have dots…", "Let's use it as is if it has dots") that are inappropriate for production code and leave readers uncertain about the intended behaviour.The whole branch, together with its comments, can be removed.
♻️ Proposed cleanup
- // 3. Fallback to type if it looks like a bundle ID (contains a dot) - // We check for dots to avoid generic types like "XCUIElementTypeApplication" - if (rootElement.type && rootElement.type.includes(".")) { - // If it's something like "bundleId.ClassName", take the left part - // Actually, let's keep it simple: if it contains a dot, it might be the bundle ID or start with it. - // For consistency with CodeRabbit: "split on the first '.' and return the left part" - // Wait, bundle IDs also have dots. If I split "com.apple.mobilesafari", I get "com". - // That's likely NOT what was intended for a real bundle ID. - // But let's assume they meant if the class name is appended, e.g., "com.apple.mobilesafari.AppDelegate" - // If there are multiple dots, it's still ambiguous. - // Let's use it as is if it has dots, as most bundle IDs do. - return rootElement.type; - } - throw new ActionableError("No app is in foreground. Please launch an app and try again.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webdriver-agent.ts` around lines 469 - 481, Remove the dead fallback branch that checks rootElement.type.includes(".") and its stream-of-consciousness comments: locate the conditional that reads "if (rootElement.type && rootElement.type.includes(\".\")) { ... return rootElement.type; }" and delete the entire if-block and its multi-line comments so the code no longer contains the unreachable branch or confusing developer notes; ensure any surrounding logic still returns the intended value (e.g., from other branches that handle XCUIElementTypeApplication) and run tests.test/current-activity.ts (2)
49-53: Permanently skipped test can be removed.A test that calls
this.skip()unconditionally provides no signal and adds maintenance noise. Either keep it as a comment/TODO in a separate tracking issue, or remove the test block entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/current-activity.ts` around lines 49 - 53, Remove the always-skipped test block named "should throw ActionableError if no activity in focus" from the test suite to avoid dead/noise; locate the it(...) block in current-activity.ts and delete the entire test function (or replace it with a brief TODO comment referencing an issue if you want to track re-adding it later). Ensure no leftover references to this test name remain in the file.
34-47: Android launch test uses a fixed 500 ms delay; prefer a retry loop consistent with the iOS test.The iOS test at lines 101–107 correctly polls with a 5-second deadline. The Android test assumes 500 ms is always sufficient, which is unreliable on slow emulators or devices. Also,
assert.equal(line 46) should beassert.strictEqualfor idiomatic Node.js assertions.♻️ Proposed fix
await android.launchApp(testAppPackage); - // Small delay to ensure app is focused - await new Promise(resolve => setTimeout(resolve, 500)); - - const activity = await android.getCurrentActivity(); - assert.equal(activity.id, testAppPackage, `Expected ${testAppPackage} but got ${activity.id}`); + // Poll until the activity is focused (mirrors the iOS retry pattern) + const deadline = Date.now() + 5000; + let activity = await android.getCurrentActivity(); + while (activity.id !== testAppPackage && Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, 200)); + activity = await android.getCurrentActivity(); + } + assert.strictEqual(activity.id, testAppPackage, `Expected ${testAppPackage} but got ${activity.id}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/current-activity.ts` around lines 34 - 47, Replace the fixed 500ms sleep with a polling loop (deadline ~5s) that repeatedly calls android.getCurrentActivity() until activity.id === testAppPackage or timeout, mirroring the iOS retry behavior; call this polling after android.launchApp(testAppPackage) and before the assertion. Also change the assertion from assert.equal(...) to assert.strictEqual(activity.id, testAppPackage, `Expected ${testAppPackage} but got ${activity.id}`) to use strict equality. Ensure the loop sleeps briefly between attempts (e.g., 100–200ms) and throws/fails the test if the 5s deadline is exceeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/webdriver-agent.ts`:
- Around line 464-467: The current fallback that returns rootElement.name (the
accessibility/display name) when rawIdentifier is missing is incorrect; update
the code that currently checks rootElement.name (referencing rootElement and
rawIdentifier) to not return the display name. Instead, when rawIdentifier is
not available, query the WDA session capabilities (GET /session/:id) and extract
the bundleId (e.g., read capabilities.bundleId or session response.bundleId) and
return that; only fall back to rootElement.name as a last-resort debug value,
but do not treat it as the canonical iOS bundle identifier. Ensure the logic is
placed where rawIdentifier is handled so callers expecting a bundle ID get the
capability-derived bundleId.
---
Duplicate comments:
In `@src/android.ts`:
- Around line 482-486: Remove the dead Android 9+ fallback: delete the
mFocusedActivity fallback block that uses
dumpsysOutput.match(/mFocusedActivity=ActivityRecord\{[^\s]+ u\d+ ([^\s/]+)\//)
and the associated focusMatch handling/return so it no longer adds unreachable
noise; ensure any now-unused variable (focusMatch) or comment is removed and
that the surrounding function's other code paths still return the expected
value/type.
In `@test/current-activity.ts`:
- Around line 8-12: The device presence checks use strict equality to 1 which
causes tests to be skipped when multiple devices are connected; change the
guards so hasAndroidDevice and hasIosDevice are computed as
androidDevices.length > 0 and iosDevices.length > 0 (update the expressions that
set hasAndroidDevice and hasIosDevice where androidDevices and
iosManager.listDevices()/iosDevices are used) so tests run when at least one
device is present rather than only when exactly one is connected.
---
Nitpick comments:
In `@src/webdriver-agent.ts`:
- Around line 469-481: Remove the dead fallback branch that checks
rootElement.type.includes(".") and its stream-of-consciousness comments: locate
the conditional that reads "if (rootElement.type &&
rootElement.type.includes(\".\")) { ... return rootElement.type; }" and delete
the entire if-block and its multi-line comments so the code no longer contains
the unreachable branch or confusing developer notes; ensure any surrounding
logic still returns the intended value (e.g., from other branches that handle
XCUIElementTypeApplication) and run tests.
In `@test/current-activity.ts`:
- Around line 49-53: Remove the always-skipped test block named "should throw
ActionableError if no activity in focus" from the test suite to avoid
dead/noise; locate the it(...) block in current-activity.ts and delete the
entire test function (or replace it with a brief TODO comment referencing an
issue if you want to track re-adding it later). Ensure no leftover references to
this test name remain in the file.
- Around line 34-47: Replace the fixed 500ms sleep with a polling loop (deadline
~5s) that repeatedly calls android.getCurrentActivity() until activity.id ===
testAppPackage or timeout, mirroring the iOS retry behavior; call this polling
after android.launchApp(testAppPackage) and before the assertion. Also change
the assertion from assert.equal(...) to assert.strictEqual(activity.id,
testAppPackage, `Expected ${testAppPackage} but got ${activity.id}`) to use
strict equality. Ensure the loop sleeps briefly between attempts (e.g.,
100–200ms) and throws/fails the test if the 5s deadline is exceeded.
Author
|
I will look into this comment and address it shortly. |
Address review comment by acknowledging that rootElement.name returns the display name rather than the bundle identifier. Added TODO to implement session capability querying for a more reliable ID.
Query WDA session capabilities (GET /session/:id) to retrieve the canonical bundle identifier when rawIdentifier is missing from the accessibility tree. This fully addresses the review comment regarding unreliable display name fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/webdriver-agent.ts`:
- Around line 462-465: Guard the sessionId from sessions[0] before building
sessionUrl and calling fetch: check that sessions && sessions.length > 0 &&
sessions[0].id is truthy (the sessions[0].id value used as sessionId) and if
it’s missing return null (or skip the fetch) instead of constructing
`sessionUrl` and calling `fetch(sessionUrl)` with `undefined`; update the block
around sessionId/sessionUrl/fetch to perform this early guard and avoid the
unnecessary network request.
---
Duplicate comments:
In `@src/webdriver-agent.ts`:
- Around line 496-500: The fallback using rootElement.name can return a display
name (e.g., "Safari") when no other source exists; update the code/users to
handle this by (1) documenting this behavior in the function that returns the
app identifier (mentioning rootElement.name as the last-resort fallback and the
session-derived bundleId used earlier), and (2) updating any callers to treat a
returned display name as non-canonical (e.g., attempt resolution via session
bundleId first or handle UI-name strings appropriately). Reference
rootElement.name and the session bundleId variable in your documentation and
caller checks.
- Guard sessionId in getActiveSessionBundleId to avoid invalid network requests - Add JSDoc to parseWdaPageSourceForAppId explaining resolution order and non-canonical name fallback
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/webdriver-agent.ts (1)
455-474: Redundantsessions &&guard —sessionsis always an array.
sessionsis assigned assessionsJson.value || []on line 460, so it is always a truthy array (at minimum[]). The leadingsessions &&in the condition is a no-op.♻️ Proposed cleanup
- if (sessions && sessions.length > 0 && sessions[0].id) { + if (sessions.length > 0 && sessions[0].id) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webdriver-agent.ts` around lines 455 - 474, In getActiveSessionBundleId(), the condition `if (sessions && sessions.length > 0 && sessions[0].id)` uses a redundant `sessions &&` since sessions is always assigned `sessionsJson.value || []`; remove the unnecessary guard and simplify the check to `if (sessions.length > 0 && sessions[0].id)` (or equivalently `if (sessions[0]?.id)`) so the logic remains the same but without the no-op truthiness test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/webdriver-agent.ts`:
- Around line 491-517: The function parseWdaPageSourceForAppId currently returns
a string and silently may return a non-canonical display name
(rootElement.name); change its contract to return a discriminated union so
callers can tell canonical vs non-canonical values: update the signature to
return { id: string; isCanonical: boolean }, set isCanonical = true when
returning rootElement.rawIdentifier, sessionBundleId, or rootElement.type (dot
heuristic), set isCanonical = false when falling back to rootElement.name, and
keep throwing ActionableError only when no id can be found; update all call
sites of parseWdaPageSourceForAppId to handle the new shape.
---
Nitpick comments:
In `@src/webdriver-agent.ts`:
- Around line 455-474: In getActiveSessionBundleId(), the condition `if
(sessions && sessions.length > 0 && sessions[0].id)` uses a redundant `sessions
&&` since sessions is always assigned `sessionsJson.value || []`; remove the
unnecessary guard and simplify the check to `if (sessions.length > 0 &&
sessions[0].id)` (or equivalently `if (sessions[0]?.id)`) so the logic remains
the same but without the no-op truthiness test.
- Update Robot interface and all implementations to return { id: string; isCanonical: boolean }
- Simplify sessionId guard in getActiveSessionBundleId
- set isCanonical=true for Android matches and canonical iOS sources (rawIdentifier, session, type-dot-heuristic)
- set isCanonical=false for iOS display name fallback (rootElement.name)
Author
|
@CodeRabbit resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/android.ts (1)
476-480:mCurrentFocusfallback can return IME/overlay package names — consider flagging this to callers.As documented in the comment,
mCurrentFocusmay match the soft keyboard (e.g.,com.android.inputmethod.latin) or a system dialog rather than the user's foreground app. SinceisCanonical: trueis returned, the caller has no signal to distinguish this case from a genuine foreground app match.Consider either returning
isCanonical: falseformCurrentFocusmatches, or adding a comment/doc explaining that downstream consumers should be aware of this edge case. This is a minor point — the regex already filters out non-activity windows (e.g.,StatusBar) by requiring the/suffix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/android.ts` around lines 476 - 480, The mCurrentFocus fallback currently returns isCanonical: true for matches from dumpsysOutput.match(...), which can be IME/overlay/system windows (e.g., com.android.inputmethod.latin), so change the fallback handling where focusMatch is used (the mCurrentFocus regex and the focusMatch branch) to return isCanonical: false for mCurrentFocus-derived ids and add or update the inline comment near the focusMatch logic to note that callers must treat isCanonical:false as a possibly non-foreground-app window (IME/overlay/system) so downstream code can differentiate these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/android.ts`:
- Around line 476-480: The mCurrentFocus fallback currently returns isCanonical:
true for matches from dumpsysOutput.match(...), which can be IME/overlay/system
windows (e.g., com.android.inputmethod.latin), so change the fallback handling
where focusMatch is used (the mCurrentFocus regex and the focusMatch branch) to
return isCanonical: false for mCurrentFocus-derived ids and add or update the
inline comment near the focusMatch logic to note that callers must treat
isCanonical:false as a possibly non-foreground-app window (IME/overlay/system)
so downstream code can differentiate these cases.
- Update guard logic from '=== 1' to '> 0' to prevent silent skips when multiple devices are connected - Rename variables (e.g., 'hasOneAndroidDevice' -> 'hasAndroidDevice') for semantic accuracy - Address review comment r2835116617
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/current-activity.ts (1)
42-43: Consider replacing the fixed 500 ms delay with the polling pattern used in the iOS test.The hard-coded
setTimeouton line 43 is fragile — it may be too short on a slow emulator. The iOS test (lines 102–107) already uses a deadline-based poll loop, which is more resilient.♻️ Suggested refactor for Android launch-and-verify
-// Small delay to ensure app is focused -await new Promise(resolve => setTimeout(resolve, 500)); - -const activity = await android.getCurrentActivity(); -assert.equal(activity.id, testAppPackage, `Expected ${testAppPackage} but got ${activity.id}`); +// Poll until the app becomes foreground (up to 5 seconds) +const deadline = Date.now() + 5000; +let activity = await android.getCurrentActivity(); +while (activity.id !== testAppPackage && Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, 200)); + activity = await android.getCurrentActivity(); +} +assert.strictEqual(activity.id, testAppPackage, `Expected ${testAppPackage} but got ${activity.id}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/current-activity.ts` around lines 42 - 43, Replace the fragile fixed delay (await new Promise(resolve => setTimeout(resolve, 500))) with the same deadline-based polling loop used in the iOS test (lines 102–107): implement a deadline = Date.now() + timeout, then loop checking the desired condition (the same condition the iOS loop verifies), sleeping a short interval (e.g., 100–200ms) between iterations, exit when the condition becomes true, and throw a helpful error if the deadline is exceeded; remove the setTimeout call and use this polling pattern inside the Android launch-and-verify flow so it waits robustly for the app to be focused/ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/current-activity.ts`:
- Line 46: Replace the use of assert.equal with assert.strictEqual for
consistency and strict comparison: change the assertion that compares
activity.id to testAppPackage (the call currently using
assert.equal(activity.id, testAppPackage, ...)) to use
assert.strictEqual(activity.id, testAppPackage, ...) so it uses ===; keep the
existing error message argument unchanged and ensure other assertions (e.g., the
one at line 109) remain consistent.
---
Nitpick comments:
In `@test/current-activity.ts`:
- Around line 42-43: Replace the fragile fixed delay (await new Promise(resolve
=> setTimeout(resolve, 500))) with the same deadline-based polling loop used in
the iOS test (lines 102–107): implement a deadline = Date.now() + timeout, then
loop checking the desired condition (the same condition the iOS loop verifies),
sleeping a short interval (e.g., 100–200ms) between iterations, exit when the
condition becomes true, and throw a helpful error if the deadline is exceeded;
remove the setTimeout call and use this polling pattern inside the Android
launch-and-verify flow so it waits robustly for the app to be focused/ready.
Replace assert.equal with assert.strictEqual in test/current-activity.ts for consistency and strict comparison (===). Address review comment r2836593934.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Add new MCP tool
mobile_get_current_activityto retrieve the currently running activity (Android) or foreground app (iOS) in a deterministic, cross-platform manner.Problem Statement
Previously, there was no direct way for agents to verify which app is currently active. Agents had to infer this indirectly through:
This limitation prevents reliable multi-app automation workflows and app switching verification.
Solution
Implement a new tool that returns the package name (Android) or bundle ID (iOS) of the currently active app/activity via structured, deterministic methods:
dumpsys activityoutput with fallback chain (mCurrentFocus → mResumedActivity → mFocusedActivity)Benefits
✅ Deterministic: Uses structured data, not computer vision
✅ Multi-app support: Enables app switching workflows and verification
✅ Error recovery: Agents can verify launch success and detect unexpected app switches
✅ LLM-friendly: Returns simple JSON structure aligned with project philosophy
✅ No new dependencies: Leverages existing Android SDK, go-ios, and WebDriverAgent
✅ Consistent: Works across all device types (emulator, simulator, real device)
Changes
getCurrentActivity(): Promise<{ id: string }>methodmobile_get_current_activityTesting
✅ Build:
npm run build- No TypeScript errors✅ Linting:
npm run lint- No style violations✅ Unit Tests:
npm run test- 20 passing, 3 new tests for this feature✅ Android: Verified on emulator-5554 with real app switching
✅ iOS: Code validated against WebDriverAgent patterns, follows project standards
Manual Test Results
Return Format
Returns a unified JSON object across platforms:
Backward Compatibility
✅ No breaking changes
✅ All existing tools unaffected
✅ Graceful error handling with ActionableError
✅ Works with existing device detection
Design Decisions
{ id: string }for consistency, using package name (Android) vs bundle ID (iOS)Related Issues
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests