Skip to content

Commit 332b06e

Browse files
authored
fix(desktop): ask-user dialog + migrate to pi-ask-user (prune connector, add tests) (#49)
* fix(desktop): show ask-user dialog immediately, notify only as reminder On macOS the makit window reports itself backgrounded whenever it isn't frontmost, so SrvRequestHandler diverted every srv.request to a transient system notification instead of the in-app dialog — the popup flashed and vanished with no way to answer. Add a reminderDelay mode: when set (desktop), requests always present the dialog immediately regardless of window focus, and only fire a system notification if still unanswered after the delay (2 min). Timers are cancelled on answer/dispose. Mobile behavior (background→notification) is unchanged (reminderDelay null). * refactor(server): drop makit-pi connector + pi-ask filter for pi-ask-user pi-ask-user degrades to ctx.ui.select/input under rpc, which the PiAdapter interceptor (Path B) already transports to the app. That makes makit's own AskUserQuestion connector tool (Path A) redundant, and the @mammothb/pi-ask exclusion a no-op now that it's uninstalled. Remove: - connectors/makit-pi.ts (AskUserQuestion/askUserQuestion shadow tool) - connectors/makit-piano.ts (piano connector skeleton) - src/pi-agent-dir.ts + the buildFilteredAgentDir(['@mammothb/pi-ask']) filter and PI_CODING_AGENT_DIR wiring Keep the loopback bridge + extensionPaths (still used by test/e2e-server.ts: StubAdapter reverse-RPC and the fake-model provider) and the askUser wiring (Path B). No functional change to the ask_user flow. * test(server): cover ask_user flow (e2e + integration + regression) - e2e (test/ask-user-e2e.test.ts): real pi + pi-ask-user through PiAdapter with a hermetic keyless fake model; proves ask_user round-trips with no connector/bridge. Skips when pi/pi-ask-user are absent (e.g. cloud VM). - integration (pi.test.ts): select/input/editor/confirm interceptor round-trips assert the exact extension_ui_response written to pi stdin. - regression (pi.test.ts): cancelled→{cancelled}, no-askUser→cancel (pi never hangs after connector removal), detection keyed on method not tool name. * fix: dismiss answered request dialogs
1 parent e5cd71a commit 332b06e

13 files changed

Lines changed: 541 additions & 304 deletions

File tree

app/lib/desktop/desktop_app.dart

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,9 @@ class _DesktopAppState extends ConsumerState<_DesktopApp> {
233233
themeMode: ThemeMode.system,
234234
builder: (context, child) => SrvRequestHandler(
235235
navigatorKey: _desktopNavKey,
236+
// Desktop is the control surface: always show the in-app dialog, and
237+
// only fall back to a system notification if it goes unanswered.
238+
reminderDelay: const Duration(minutes: 2),
236239
child: child ?? const SizedBox(),
237240
),
238241
home: DesktopChatShell(onOpenSettings: _openSettings),

app/lib/ui/widgets/srv_request_handler.dart

Lines changed: 113 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,23 @@ import '../../store/store.dart';
2020
import '../../transport/protocol.dart';
2121

2222
class SrvRequestHandler extends ConsumerStatefulWidget {
23-
const SrvRequestHandler({super.key, required this.child, this.navigatorKey});
23+
const SrvRequestHandler({
24+
super.key,
25+
required this.child,
26+
this.navigatorKey,
27+
this.reminderDelay,
28+
});
2429
final Widget child;
2530
final GlobalKey<NavigatorState>? navigatorKey;
2631

32+
/// Desktop mode. When set, requests ALWAYS present the in-app dialog
33+
/// immediately (never diverted to a notification based on foreground
34+
/// state — the desktop window is the control surface). If the request is
35+
/// still unanswered after this delay, a system notification is fired as a
36+
/// reminder. When null (mobile), backgrounded requests are diverted to an
37+
/// actionable notification instead of the invisible dialog.
38+
final Duration? reminderDelay;
39+
2740
@override
2841
ConsumerState<SrvRequestHandler> createState() => _SrvRequestHandlerState();
2942
}
@@ -42,6 +55,16 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
4255
final Map<String, Envelope> _pendingBackground = {};
4356
static const _kMaxPendingBackground = 50;
4457

58+
/// Desktop reminder timers, keyed by request id. Fires a system notification
59+
/// when a dialog has been open (unanswered) for [SrvRequestHandler.reminderDelay];
60+
/// cancelled when the request is answered or the widget disposes.
61+
final Map<String, Timer> _reminderTimers = {};
62+
63+
/// Dialog contexts keyed by request id, so an answer from a notification can
64+
/// remove that request's exact route without disturbing another open dialog.
65+
final Map<String, BuildContext> _activeDialogContexts = {};
66+
final Map<String, Object> _activeDialogTokens = {};
67+
4568
/// Salted so request-notification ids can't collide with the status
4669
/// notifications keyed on `sessionId.hashCode`.
4770
int _notificationId(String requestId) =>
@@ -92,12 +115,86 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
92115
final controller = ref.read(connectionControllerProvider.notifier);
93116
_sub = controller.srvRequests.listen(_dispatch);
94117
_respondedSub?.cancel();
95-
_respondedSub = controller.responded.listen(_pendingBackground.remove);
118+
_respondedSub = controller.responded.listen(_onResponded);
119+
}
120+
121+
void _onResponded(String id) {
122+
_pendingBackground.remove(id);
123+
_reminderTimers.remove(id)?.cancel();
124+
_activeDialogTokens.remove(id);
125+
final dialogContext = _activeDialogContexts.remove(id);
126+
if (dialogContext == null || !dialogContext.mounted) return;
127+
final route = ModalRoute.of(dialogContext);
128+
if (route != null && route.isActive) {
129+
Navigator.of(dialogContext).removeRoute(route);
130+
}
131+
}
132+
133+
Future<T?> _showTrackedDialog<T>({
134+
required String requestId,
135+
required BuildContext context,
136+
required WidgetBuilder builder,
137+
bool barrierDismissible = true,
138+
}) async {
139+
final dialogToken = Object();
140+
final result = await showDialog<T>(
141+
context: context,
142+
barrierDismissible: barrierDismissible,
143+
builder: (dctx) {
144+
_activeDialogContexts[requestId] = dctx;
145+
_activeDialogTokens[requestId] = dialogToken;
146+
return builder(dctx);
147+
},
148+
);
149+
if (identical(_activeDialogTokens[requestId], dialogToken)) {
150+
_activeDialogTokens.remove(requestId);
151+
_activeDialogContexts.remove(requestId);
152+
}
153+
return result;
154+
}
155+
156+
/// Fire a system notification once a still-unanswered request has been
157+
/// on-screen for [SrvRequestHandler.reminderDelay] (desktop only).
158+
void _scheduleReminder(Envelope env, String kind) {
159+
final sessionId = env.body['sessionId'] as String? ?? '';
160+
_reminderTimers.remove(env.id)?.cancel();
161+
_reminderTimers[env.id] = Timer(widget.reminderDelay!, () async {
162+
_reminderTimers.remove(env.id);
163+
if (!mounted) return;
164+
final notif = notificationForRequest(
165+
kind: kind,
166+
body: env.body,
167+
label: _labelFor(sessionId),
168+
);
169+
if (notif == null) return;
170+
await ref
171+
.read(notificationServiceProvider)
172+
.show(
173+
id: _notificationId(env.id),
174+
title: notif.title,
175+
body: notif.body,
176+
category: notif.category,
177+
payload: encodeRequestPayload(
178+
sessionId: sessionId,
179+
requestId: env.id,
180+
kind: kind,
181+
),
182+
);
183+
});
96184
}
97185

98186
Future<void> _dispatch(Envelope env) async {
99187
final kind = env.body['kind'] as String? ?? 'unknown';
100188

189+
// Desktop: always present the dialog now; if it goes unanswered for
190+
// `reminderDelay`, nudge with a system notification (the tap routes back
191+
// through respondTo, and the in-app dialog stays answerable meanwhile).
192+
if (widget.reminderDelay != null) {
193+
_scheduleReminder(env, kind);
194+
await _presentDialog(env);
195+
return;
196+
}
197+
101198
// Backgrounded: if this request kind has an actionable-notification
102199
// affordance, fire the notification and skip the (invisible) dialog. The
103200
// user resolves it from the lock screen; the tap routes back through
@@ -207,7 +304,8 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
207304
String requestId,
208305
List<Map<String, dynamic>> questions,
209306
) async {
210-
final result = await showDialog<Map<String, dynamic>?>(
307+
final result = await _showTrackedDialog<Map<String, dynamic>?>(
308+
requestId: requestId,
211309
context: ctx,
212310
barrierDismissible: false,
213311
builder: (dctx) => _AskWizard(questions: questions),
@@ -243,7 +341,8 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
243341
String requestId,
244342
Map<String, dynamic> body,
245343
) async {
246-
final approved = await showDialog<bool>(
344+
final approved = await _showTrackedDialog<bool>(
345+
requestId: requestId,
247346
context: ctx,
248347
barrierDismissible: false,
249348
builder: (dctx) => AlertDialog(
@@ -298,7 +397,8 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
298397
text: body['prefill']?.toString() ?? '',
299398
);
300399
final multiline = body['multiline'] == true;
301-
final value = await showDialog<String?>(
400+
final value = await _showTrackedDialog<String?>(
401+
requestId: requestId,
302402
context: ctx,
303403
barrierDismissible: false,
304404
builder: (dctx) => AlertDialog(
@@ -333,7 +433,8 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
333433

334434
Future<void> _showGeneric(BuildContext ctx, Envelope env) async {
335435
final controller = TextEditingController();
336-
final text = await showDialog<String?>(
436+
final text = await _showTrackedDialog<String?>(
437+
requestId: env.id,
337438
context: ctx,
338439
builder: (dctx) => AlertDialog(
339440
title: Text(env.body['title']?.toString() ?? 'Server request'),
@@ -395,6 +496,12 @@ class _SrvRequestHandlerState extends ConsumerState<SrvRequestHandler>
395496
WidgetsBinding.instance.removeObserver(this);
396497
_sub?.cancel();
397498
_respondedSub?.cancel();
499+
for (final t in _reminderTimers.values) {
500+
t.cancel();
501+
}
502+
_reminderTimers.clear();
503+
_activeDialogContexts.clear();
504+
_activeDialogTokens.clear();
398505
super.dispose();
399506
}
400507

app/test/srv_request_handler_notify_test.dart

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ void main() {
104104
WidgetTester tester, {
105105
_RecordingNotificationService? notifications,
106106
GlobalKey<NavigatorState>? navigatorKey,
107+
Duration? reminderDelay,
107108
}) async {
108109
final transport = _EmittingTransport();
109110
final service = notifications ?? _RecordingNotificationService();
@@ -126,6 +127,7 @@ void main() {
126127
navigatorKey: key,
127128
home: SrvRequestHandler(
128129
navigatorKey: key,
130+
reminderDelay: reminderDelay,
129131
child: const Scaffold(body: SizedBox()),
130132
),
131133
),
@@ -404,4 +406,112 @@ void main() {
404406
expect(otherId, isNot(firstId));
405407
},
406408
);
409+
410+
testWidgets(
411+
'desktop mode shows the dialog immediately even when backgrounded',
412+
(tester) async {
413+
final (transport, notifications, _) = await pumpHandler(
414+
tester,
415+
reminderDelay: const Duration(minutes: 2),
416+
);
417+
// Not frontmost (as a macOS window often is while the agent runs).
418+
tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.paused);
419+
await tester.pump();
420+
421+
transport.emit(
422+
Envelope(
423+
t: MsgType.srvRequest,
424+
id: 'req-desk-q',
425+
body: {
426+
'kind': 'askUserQuestion',
427+
'question': 'Deploy to prod?',
428+
'options': [
429+
{'label': 'Yes'},
430+
{'label': 'No'},
431+
],
432+
'sessionId': 's1',
433+
},
434+
),
435+
);
436+
await tester.pump();
437+
await tester.pump();
438+
439+
// No diversion to a notification while backgrounded — the dialog route
440+
// is pushed instead. Settle its entrance animation to confirm it shows.
441+
expect(notifications.shown, isEmpty);
442+
tester.binding.handleAppLifecycleStateChanged(AppLifecycleState.resumed);
443+
await tester.pumpAndSettle();
444+
expect(find.byType(AlertDialog), findsOneWidget);
445+
expect(find.text('Deploy to prod?'), findsOneWidget);
446+
expect(notifications.shown, isEmpty);
447+
},
448+
);
449+
450+
testWidgets(
451+
'desktop mode fires a reminder notification only if left unanswered',
452+
(tester) async {
453+
final (transport, notifications, _) = await pumpHandler(
454+
tester,
455+
reminderDelay: const Duration(minutes: 2),
456+
);
457+
458+
transport.emit(
459+
Envelope(
460+
t: MsgType.srvRequest,
461+
id: 'req-remind',
462+
body: {
463+
'kind': 'confirmAction',
464+
'action': 'rm -rf build/',
465+
'sessionId': 's1',
466+
},
467+
),
468+
);
469+
await tester.pump();
470+
await tester.pump();
471+
expect(find.text('Approve'), findsOneWidget);
472+
expect(notifications.shown, isEmpty);
473+
474+
// Past the reminder delay while still unanswered → notification fires.
475+
await tester.pump(const Duration(minutes: 2));
476+
await tester.pump();
477+
expect(notifications.shown, hasLength(1));
478+
expect(notifications.shown.single.category, kConfirmCategoryId);
479+
},
480+
);
481+
482+
testWidgets('desktop dialog closes and reminder is cancelled once answered', (
483+
tester,
484+
) async {
485+
final (transport, notifications, controller) = await pumpHandler(
486+
tester,
487+
reminderDelay: const Duration(minutes: 2),
488+
);
489+
490+
transport.emit(
491+
Envelope(
492+
t: MsgType.srvRequest,
493+
id: 'req-answered-fast',
494+
body: {
495+
'kind': 'confirmAction',
496+
'action': 'rm -rf build/',
497+
'sessionId': 's1',
498+
},
499+
),
500+
);
501+
await tester.pump();
502+
await tester.pump();
503+
504+
// Answered before the delay elapses.
505+
controller.respondTo('req-answered-fast', {
506+
'kind': 'confirmAction',
507+
'approved': true,
508+
});
509+
await tester.pumpAndSettle();
510+
expect(find.byType(AlertDialog), findsNothing);
511+
512+
// No reminder should fire after the delay.
513+
await tester.pump(const Duration(minutes: 2));
514+
await tester.pump();
515+
expect(notifications.shown, isEmpty);
516+
});
407517
}

docs/CONNECTORS.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,8 @@ Adding a new variant:
6363

6464
## Writing a connector
6565

66-
The working example is [`server/connectors/makit-pi.ts`](../server/connectors/makit-pi.ts).
67-
A drop-in template lives at [`server/connectors/makit-piano.ts`](../server/connectors/makit-piano.ts).
66+
No built-in connector is currently checked in. Use the minimum example below
67+
as the starting point for a new `server/connectors/<your-agent>.ts` file.
6868

6969
A connector is a TypeScript file with a `default export` function that
7070
receives the agent's extension API. The function registers tools that:
@@ -124,7 +124,7 @@ Connectors are **auto-discovered** at server startup. Anything matching
124124
You'll see this in the server log:
125125

126126
```
127-
[makit] loading 2 connector(s): makit-pi.ts, makit-piano.ts
127+
[makit] loading 1 connector(s): my-agent.ts
128128
```
129129

130130
## Environment contract

docs/NOTIFICATIONS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ Requires a paired iPhone with notification permission granted.
2626
1. Pair the phone with a running `makit` server over Tailscale.
2727
2. Grant notification permission during onboarding (or Settings → Notifications).
2828
3. Open a session, then **background** the app (home button / swipe up).
29-
4. On the desktop, trigger a `confirmAction` (e.g. `piano_confirm` in
30-
`server/connectors/makit-piano.ts`, or any agent tool that needs approval).
29+
4. On the desktop, trigger a `confirmAction` from any installed agent extension
30+
or tool that needs approval (for Pi, this is a `ctx.ui.confirm` request).
3131
5. Within a few seconds the phone shows a notification with **Approve** and
3232
**Deny**.
3333
6. Tap **Approve** on the lock screen. The agent continues **without** opening

0 commit comments

Comments
 (0)