Add push notification tester to tools panel#53
Conversation
A new "Push Notification" section in the side tools panel drives `xcrun simctl push`. The target bundle ID is auto-filled from the foreground app and offers a dropdown of recently-pushed bundles (persisted in localStorage). The JSON payload editor has a caret-aware completion popup driven by an APNS JSON schema: property names, enum values, and object/array starters get suggested as you type, with descriptions surfacing in tooltips. https://claude.ai/code/session_0199UNV5FYnUgJCTkzW27X4T
📝 WalkthroughWalkthroughAdded PushNotificationTool, a new iOS simulator push notification tester with an APNS JSON editor featuring caret-aware autocomplete, localStorage-backed payload drafts and recent bundle IDs, and a send flow that base64-encodes the payload and runs xcrun simctl push on the host. Integrated into the Tools panel UI. ChangesPush Notification Tool Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant PushNotificationTool
participant JsonSchemaEditor
participant PushHandler
participant Host
participant Simulator
User->>PushNotificationTool: Open Push Notification Tool
PushNotificationTool->>PushNotificationTool: Load recent bundle IDs from localStorage
PushNotificationTool->>PushNotificationTool: Initialize payload draft from localStorage
User->>JsonSchemaEditor: Type in payload text area with Ctrl/⌃Space
JsonSchemaEditor->>JsonSchemaEditor: Analyze JSON caret position
JsonSchemaEditor->>JsonSchemaEditor: Compute JSON path under caret
JsonSchemaEditor->>JsonSchemaEditor: Query APNS schema for completions
JsonSchemaEditor->>JsonSchemaEditor: Display suggestion popup with keyboard navigation
User->>JsonSchemaEditor: Select suggestion with arrow keys and Enter
JsonSchemaEditor->>JsonSchemaEditor: Insert completion and update payload
User->>PushNotificationTool: Click Send button
PushHandler->>PushHandler: Validate payload JSON syntax
PushHandler->>PushHandler: Compact and base64-encode payload
PushHandler->>Host: Execute mktemp, write encoded payload, execute xcrun simctl push
Host->>Simulator: Deploy push notification to app
PushHandler->>PushNotificationTool: Report success/error status
PushHandler->>PushNotificationTool: Update recent bundle IDs in localStorage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 (1)
packages/serve-sim/src/client/client.tsx (1)
1172-1181:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle rejected app-detail fetches so the panel can recover.
If
fetchAppDetails()rejects here,details.loadingnever flips back tofalse, so the app card gets stuck in a permanent loading state for that foreground app. Add a.catch()branch that records an error/fallback state.Suggested fix
fetchAppDetails(execOnHost, udid, currentApp.bundleId).then((extra) => { if (cancelled) return; setDetails({ bundleId: currentApp.bundleId, isReactNative: currentApp.isReactNative, pid: currentApp.pid, loading: false, ...extra, }); - }); + }).catch((error) => { + if (cancelled) return; + setDetails({ + bundleId: currentApp.bundleId, + isReactNative: currentApp.isReactNative, + pid: currentApp.pid, + loading: false, + error: error instanceof Error ? error.message : "Failed to load app details", + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/serve-sim/src/client/client.tsx` around lines 1172 - 1181, The promise returned by fetchAppDetails(execOnHost, udid, currentApp.bundleId) needs a .catch handler so a rejected fetch doesn't leave details.loading true; add a .catch that first checks cancelled and then calls setDetails with the same baseline fields (bundleId: currentApp.bundleId, isReactNative: currentApp.isReactNative, pid: currentApp.pid) plus loading: false and an error/fallback flag or message (e.g., error: true or errorMessage) so the UI can recover; keep the existing .then path unchanged and ensure both branches respect the cancelled guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/serve-sim/src/client/client.tsx`:
- Around line 2289-2334: The outer interactive element currently rendered as a
<button> (see the map callback that creates the element using
pushStyles.comboItem and the onClick that calls
setBundleId/setBundleManuallySet/setComboOpen) must not contain the inner
"Forget" <button>; change the outer <button> to a non-interactive container
(e.g., a <div> or <li>) with role="option", tabIndex=0 and the same onClick
handler, and add an onKeyDown handler to invoke the same click behavior for
Enter/Space so keyboard users can activate it; keep the inner remove <button>
(with its e.stopPropagation()) as a true button, preserve aria-label/title and
styles (pushStyles.comboRemove), and ensure visual focus styles remain on the
new container.
- Around line 1952-1964: The popup is anchored to ctx.tokenStart so it stays
under the token start instead of the actual caret; change the computation to
base the marker on the current caret index (use ta.selectionStart or the
equivalent ctx.selection/cursor index) — e.g. replace value.slice(0,
ctx.tokenStart) with value.slice(0, caretIndex), append the marker as before,
call setPopupPos({left, top}) using that marker, and update the effect
dependencies to include the caret/selection index (or ta.selectionStart) so the
popup repositions as the cursor moves.
---
Outside diff comments:
In `@packages/serve-sim/src/client/client.tsx`:
- Around line 1172-1181: The promise returned by fetchAppDetails(execOnHost,
udid, currentApp.bundleId) needs a .catch handler so a rejected fetch doesn't
leave details.loading true; add a .catch that first checks cancelled and then
calls setDetails with the same baseline fields (bundleId: currentApp.bundleId,
isReactNative: currentApp.isReactNative, pid: currentApp.pid) plus loading:
false and an error/fallback flag or message (e.g., error: true or errorMessage)
so the UI can recover; keep the existing .then path unchanged and ensure both
branches respect the cancelled guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cea3a2a6-7030-4cc7-9779-7512b09fa845
📒 Files selected for processing (1)
packages/serve-sim/src/client/client.tsx
| const before = value.slice(0, ctx.tokenStart); | ||
| mirror.scrollTop = ta.scrollTop; | ||
| mirror.scrollLeft = ta.scrollLeft; | ||
| mirror.textContent = before; | ||
| const marker = document.createElement("span"); | ||
| marker.textContent = ""; | ||
| mirror.appendChild(marker); | ||
| const taRect = ta.getBoundingClientRect(); | ||
| const mRect = marker.getBoundingClientRect(); | ||
| const left = mRect.left - taRect.left + ta.clientLeft; | ||
| const top = mRect.top - taRect.top + ta.clientTop + mRect.height; | ||
| setPopupPos({ left, top }); | ||
| }, [open, value, ctx.tokenStart]); |
There was a problem hiding this comment.
Anchor the autocomplete popup to the caret, not the token start.
The popup position is computed from ctx.tokenStart, so once the user types past the first character it stays pinned under the opening quote / first character instead of the actual cursor. That makes the completion menu visibly drift away during normal typing.
Suggested fix
- const before = value.slice(0, ctx.tokenStart);
+ const before = value.slice(0, caret);
@@
- }, [open, value, ctx.tokenStart]);
+ }, [open, value, caret]);📝 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.
| const before = value.slice(0, ctx.tokenStart); | |
| mirror.scrollTop = ta.scrollTop; | |
| mirror.scrollLeft = ta.scrollLeft; | |
| mirror.textContent = before; | |
| const marker = document.createElement("span"); | |
| marker.textContent = ""; | |
| mirror.appendChild(marker); | |
| const taRect = ta.getBoundingClientRect(); | |
| const mRect = marker.getBoundingClientRect(); | |
| const left = mRect.left - taRect.left + ta.clientLeft; | |
| const top = mRect.top - taRect.top + ta.clientTop + mRect.height; | |
| setPopupPos({ left, top }); | |
| }, [open, value, ctx.tokenStart]); | |
| const before = value.slice(0, caret); | |
| mirror.scrollTop = ta.scrollTop; | |
| mirror.scrollLeft = ta.scrollLeft; | |
| mirror.textContent = before; | |
| const marker = document.createElement("span"); | |
| marker.textContent = ""; | |
| mirror.appendChild(marker); | |
| const taRect = ta.getBoundingClientRect(); | |
| const mRect = marker.getBoundingClientRect(); | |
| const left = mRect.left - taRect.left + ta.clientLeft; | |
| const top = mRect.top - taRect.top + ta.clientTop + mRect.height; | |
| setPopupPos({ left, top }); | |
| }, [open, value, caret]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/serve-sim/src/client/client.tsx` around lines 1952 - 1964, The popup
is anchored to ctx.tokenStart so it stays under the token start instead of the
actual caret; change the computation to base the marker on the current caret
index (use ta.selectionStart or the equivalent ctx.selection/cursor index) —
e.g. replace value.slice(0, ctx.tokenStart) with value.slice(0, caretIndex),
append the marker as before, call setPopupPos({left, top}) using that marker,
and update the effect dependencies to include the caret/selection index (or
ta.selectionStart) so the popup repositions as the cursor moves.
| {dropdown.map((item) => ( | ||
| <button | ||
| key={item.bundleId} | ||
| type="button" | ||
| role="option" | ||
| onClick={() => { | ||
| setBundleId(item.bundleId); | ||
| setBundleManuallySet(!item.isCurrent); | ||
| setComboOpen(false); | ||
| }} | ||
| style={pushStyles.comboItem} | ||
| > | ||
| {item.iconDataUrl ? ( | ||
| <img src={item.iconDataUrl} alt="" style={pushStyles.comboIcon} /> | ||
| ) : ( | ||
| <div style={{ ...pushStyles.comboIcon, background: "#2a2a2c" }} /> | ||
| )} | ||
| <div style={pushStyles.comboMeta}> | ||
| <div style={pushStyles.comboName}> | ||
| {item.displayName ?? item.bundleId} | ||
| {item.isCurrent && <span style={pushStyles.comboBadge}>foreground</span>} | ||
| </div> | ||
| <div style={pushStyles.comboBundle}>{item.bundleId}</div> | ||
| </div> | ||
| {!item.isCurrent && ( | ||
| <button | ||
| type="button" | ||
| aria-label={`Forget ${item.bundleId}`} | ||
| title="Remove from recents" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setRecents((prev) => { | ||
| const next = prev.filter((r) => r.bundleId !== item.bundleId); | ||
| savePushRecents(next); | ||
| return next; | ||
| }); | ||
| }} | ||
| style={pushStyles.comboRemove} | ||
| > | ||
| <svg width="10" height="10" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2.4" strokeLinecap="round" strokeLinejoin="round"> | ||
| <line x1="18" y1="6" x2="6" y2="18" /> | ||
| <line x1="6" y1="6" x2="18" y2="18" /> | ||
| </svg> | ||
| </button> | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is a <button> element allowed to contain another <button> in HTML, and what accessibility/focus issues does that cause?
💡 Result:
No, a <button> element is not allowed to contain another <button> in HTML. The HTML specification explicitly states that the content model for <button> is "Phrasing content, but there must be no interactive content descendant and no descendant with the tabindex attribute specified."[1][2][3] Since <button> is interactive content, nesting one inside another violates this rule and results in invalid HTML.[4][5] This nesting causes significant accessibility and focus issues: - Screen readers often fail to announce the inner button properly, producing silent or empty tab stops where the inner control's name, role, and state are not exposed.[6][5][7] - Keyboard navigation becomes unpredictable: focus may skip the inner element, get trapped, or activate the wrong control.[5][8][9] - Event handling is inconsistent across browsers, potentially triggering the parent instead of the child, or both.[7][10] Browsers may parse it but handle the accessibility tree inconsistently, making it unreliable for users relying on assistive technologies.[11] Instead, use non-interactive wrappers like <div> with CSS positioning or event delegation to achieve visual nesting without DOM nesting.[4][12]
Citations:
- 1: https://www.w3.org/TR/html-markup/button.html
- 2: https://html.spec.whatwg.org/multipage/form-elements.html
- 3: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button
- 4: https://stackoverflow.com/questions/39386497/can-i-nest-button-inside-another-button
- 5: https://rocketvalidator.com/accessibility-validation/axe/4.11/nested-interactive
- 6: https://accessibleweb.com/question-answer/why-are-nested-interactive-controls-an-accessibility-issue/
- 7: https://metricular.net/seo-issues/accessibility/medium-severity/medium-fix/nested-interactive-controls-are-not-announced-by-screen-readers-242/
- 8: https://help.dubbot.com/en/articles/10360369-interactive-controls-must-not-be-nested
- 9: https://testingbot.com/support/accessibility/web/rules/nested-interactive
- 10: https://stackoverflow.com/questions/62092211/html-validation-why-is-it-not-valid-to-put-an-interactive-element-inside-an-int
- 11: https://stackoverflow.com/questions/40874591/nvda-screen-reader-not-switching-to-focus-mode-predictably
- 12: https://piccalil.li/blog/accessible-faux-nested-interactive-controls/
Nested buttons violate HTML spec and break accessibility.
The <button> content model cannot include interactive content descendants. This causes screen readers to fail announcing the inner button and keyboard navigation to become unpredictable. Restructure as a non-button container (e.g., <div>) with the remove button as a sibling, or use event delegation to handle both interactions from a single container.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/serve-sim/src/client/client.tsx` around lines 2289 - 2334, The outer
interactive element currently rendered as a <button> (see the map callback that
creates the element using pushStyles.comboItem and the onClick that calls
setBundleId/setBundleManuallySet/setComboOpen) must not contain the inner
"Forget" <button>; change the outer <button> to a non-interactive container
(e.g., a <div> or <li>) with role="option", tabIndex=0 and the same onClick
handler, and add an onKeyDown handler to invoke the same click behavior for
Enter/Space so keyboard users can activate it; keep the inner remove <button>
(with its e.stopPropagation()) as a true button, preserve aria-label/title and
styles (pushStyles.comboRemove), and ensure visual focus styles remain on the
new container.
main was refactored to break client.tsx into per-feature components under src/client/components/ with Tailwind classes. Moved the push notification tester into push-notification-tool.tsx so it fits the new layout, swapped inline styles for Tailwind utilities, and wired it into the rebuilt ToolsPanel. Bundle ID auto-fill, recents dropdown (now using the shared fetchAppIcon cache for icons), and the schema-driven JSON autocomplete behavior are unchanged. https://claude.ai/code/session_0199UNV5FYnUgJCTkzW27X4T
oxlint flagged `currentApp?.bundleId` as a redundant dep when `currentApp` is already in the deps array — drop it from the three hooks that listed both. https://claude.ai/code/session_0199UNV5FYnUgJCTkzW27X4T
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/serve-sim/src/client/components/push-notification-tool.tsx (1)
636-650: 💤 Low valueMinor optimization opportunity in icon fetching effect.
The
iconsobject in the dependency array causes the effect to re-run each time an icon is fetched. Theif (icons[bid] !== undefined) continue;guard prevents infinite loops, but this creates unnecessary effect cycles.Consider using a ref to track fetched bundle IDs or moving the check outside the effect's dependencies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/serve-sim/src/client/components/push-notification-tool.tsx` around lines 636 - 650, The effect is re-running because `icons` is in the dependency array; replace that by tracking fetched bundle IDs in a ref so the effect doesn't depend on `icons`. Inside the effect (which depends on `comboOpen`, `currentApp`, `recents`, `udid`, and any exec function), create/use a useRef<Set<string>>() (e.g. fetchedIconsRef) to record which bundleIds have been requested; for each target bundleId check fetchedIconsRef.current.has(bid) before calling fetchAppIcon(execOnHost, udid, bid), add the bid to fetchedIconsRef.current when you start the fetch, and still call setIcons(...) with the returned url so state updates correctly; finally remove `icons` from the effect dependency array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/serve-sim/src/client/components/push-notification-tool.tsx`:
- Around line 675-677: The command string interpolates udid directly; apply
shellEscape to the udid (after trimming) when building the cmd used by
execOnHost to ensure consistent shell escaping like bundleId and b64; update the
push command construction in push-notification-tool.tsx where udid is
interpolated so it uses the trimmed-and-escaped udid (refer to shellEscape,
udid, cmd, and execOnHost to locate the change).
---
Nitpick comments:
In `@packages/serve-sim/src/client/components/push-notification-tool.tsx`:
- Around line 636-650: The effect is re-running because `icons` is in the
dependency array; replace that by tracking fetched bundle IDs in a ref so the
effect doesn't depend on `icons`. Inside the effect (which depends on
`comboOpen`, `currentApp`, `recents`, `udid`, and any exec function), create/use
a useRef<Set<string>>() (e.g. fetchedIconsRef) to record which bundleIds have
been requested; for each target bundleId check fetchedIconsRef.current.has(bid)
before calling fetchAppIcon(execOnHost, udid, bid), add the bid to
fetchedIconsRef.current when you start the fetch, and still call setIcons(...)
with the returned url so state updates correctly; finally remove `icons` from
the effect dependency array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16fba0eb-4be3-4d7e-b6f6-b99089638b74
📒 Files selected for processing (2)
packages/serve-sim/src/client/components/push-notification-tool.tsxpackages/serve-sim/src/client/components/tools-panel.tsx
| const b64 = btoa(unescape(encodeURIComponent(compact))); | ||
| const cmd = `set -e; TMP=$(mktemp -t serve-sim-push); trap 'rm -f "$TMP"' EXIT; printf %s ${shellEscape(b64)} | base64 -d > "$TMP"; xcrun simctl push ${udid} ${shellEscape(bundleId.trim())} "$TMP"`; | ||
| const res = await execOnHost(cmd); |
There was a problem hiding this comment.
Escape udid for shell command consistency.
The bundleId is properly shell-escaped, but udid is interpolated directly into the command string. While udid typically comes from trusted simulator enumeration, applying shellEscape() consistently provides defense-in-depth.
- const cmd = `set -e; TMP=$(mktemp -t serve-sim-push); trap 'rm -f "$TMP"' EXIT; printf %s ${shellEscape(b64)} | base64 -d > "$TMP"; xcrun simctl push ${udid} ${shellEscape(bundleId.trim())} "$TMP"`;
+ const cmd = `set -e; TMP=$(mktemp -t serve-sim-push); trap 'rm -f "$TMP"' EXIT; printf %s ${shellEscape(b64)} | base64 -d > "$TMP"; xcrun simctl push ${shellEscape(udid)} ${shellEscape(bundleId.trim())} "$TMP"`;📝 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.
| const b64 = btoa(unescape(encodeURIComponent(compact))); | |
| const cmd = `set -e; TMP=$(mktemp -t serve-sim-push); trap 'rm -f "$TMP"' EXIT; printf %s ${shellEscape(b64)} | base64 -d > "$TMP"; xcrun simctl push ${udid} ${shellEscape(bundleId.trim())} "$TMP"`; | |
| const res = await execOnHost(cmd); | |
| const b64 = btoa(unescape(encodeURIComponent(compact))); | |
| const cmd = `set -e; TMP=$(mktemp -t serve-sim-push); trap 'rm -f "$TMP"' EXIT; printf %s ${shellEscape(b64)} | base64 -d > "$TMP"; xcrun simctl push ${shellEscape(udid)} ${shellEscape(bundleId.trim())} "$TMP"`; | |
| const res = await execOnHost(cmd); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/serve-sim/src/client/components/push-notification-tool.tsx` around
lines 675 - 677, The command string interpolates udid directly; apply
shellEscape to the udid (after trimming) when building the cmd used by
execOnHost to ensure consistent shell escaping like bundleId and b64; update the
push command construction in push-notification-tool.tsx where udid is
interpolated so it uses the trimmed-and-escaped udid (refer to shellEscape,
udid, cmd, and execOnHost to locate the change).
Summary
Adds a new "Push Notification" tool to the simulator tools panel that allows developers to send test APNS payloads to apps running in the simulator using
xcrun simctl push.Key Changes
Refactored app details fetching: Extracted
AppDetectionTool's app details logic into a reusableuseAppDetailshook so the push tester can access the same icon and display name without duplicateplutilreads.New
PushNotificationToolcomponent: Implements a complete push notification testing interface with:JSON Schema editor (
JsonSchemaEditor): A custom textarea-based editor with:APNS schema definitions: Complete JSON Schema for Apple Push Notification Service payloads including:
Payload delivery: Uses base64 encoding through a shell mktemp shim to safely pass arbitrary JSON to
xcrun simctl pushwithout escaping concerns.UI refactoring: Extracted
ToolsPanelBodycomponent to cleanly manage the shareduseAppDetailshook and pass details to both detection and push tools.Implementation Details
localStorageunderserve-sim:push-recentswith display name and icon data for quick accessserve-sim:push-payloadso work-in-progress survives page refreshesjsonContextAt) tolerates partial/invalid JSON since users are actively typinghttps://claude.ai/code/session_0199UNV5FYnUgJCTkzW27X4T
Summary by CodeRabbit