Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .claude/rules/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,22 @@ layer per capability," never "a seed per thing":
granting Accessibility once, taking two consecutive beta updates,
and confirming the summon hotkey still registers with no new
permission prompt.
- **The real browser-tab approval notification** (goal 0132 slice A,
`shared/browserNotify.ts` + `app/useBrowserNotify.ts`) — the
should-notify decision (`shouldNotifyBrowserTab`) is unit-tested
across its full input range, and the Settings opt-in control's
presence and its default/granted/denied states are e2e-proven
(`remote-access.spec.ts`). What stays manual: an actual OS
notification banner appearing, and clicking it landing on the Review
decision, both require a real granted browser permission and a real
OS compositor — same OS-bound class as the dock bounce and
apply-notify's banner above. Verify by running a server-mode
instance reached from a real browser tab, enabling notifications in
Settings > Remote access, parking a guardrail approval from another
tab/device, switching away from the paired tab, and confirming a
system notification titled "Approval needed" appears; click it and
confirm the tab both regains focus and lands on the Review queue
showing that item.

From the UX point of view the seed layer stays privileged — it's the
one a human can SEE working — but correctness under change belongs to
Expand Down
74 changes: 74 additions & 0 deletions frontend/e2e/remote-access.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,80 @@ test('Remote access section discloses reachability and pairs a device on demand'
await expect(code).toHaveText(/^[A-Z0-9]{8}$/)
})

// docs/goals/0132-remote-access.md SLICE A: the "Notify me on this
// device" opt-in control. The worker server is a real mill-server
// binary (Server build tag), so the control renders here the same as
// it would over a real Tailscale connection; only the browser's own
// Notification permission is a per-test variable. Confirmed live: this
// harness's headless Chromium reports `Notification.permission` as
// 'denied' unconditionally -- neither leaving it unset nor calling
// `context.grantPermissions(['notifications'])` produces 'default' or
// 'granted' (no notification-display surface exists headless for a
// grant to attach to). Every state below is therefore reached via an
// `addInitScript` stub of `Notification.permission`/`requestPermission`
// (testing.md's documented escape hatch for a state no user primitive
// can reach in this harness), never `dispatchEvent` or a DOM mutation
// on the app's own elements.
test.describe('browser notification opt-in control', () => {
test('default permission shows the enable button and its caption, never granted/denied text', async ({ page }) => {
await page.addInitScript(() => {
Object.defineProperty(window.Notification, 'permission', { value: 'default', configurable: true })
})
await page.goto('/')
await page.getByRole('link', { name: 'Settings' }).click()
const control = page.getByTestId('browser-notify-control')
await control.scrollIntoViewIfNeeded()
await expect(page.getByTestId('browser-notify-enable')).toBeVisible()
await expect(control).toContainText("this tab isn't in view")
await expect(page.getByTestId('browser-notify-granted')).toHaveCount(0)
await expect(page.getByTestId('browser-notify-denied')).toHaveCount(0)
})

test('a granted permission renders the granted state with no button', async ({ page }) => {
await page.addInitScript(() => {
Object.defineProperty(window.Notification, 'permission', { value: 'granted', configurable: true })
})
await page.goto('/')
await page.getByRole('link', { name: 'Settings' }).click()
const control = page.getByTestId('browser-notify-control')
await control.scrollIntoViewIfNeeded()
await expect(page.getByTestId('browser-notify-granted')).toBeVisible()
await expect(page.getByTestId('browser-notify-enable')).toHaveCount(0)
})

test('a denied permission states it plainly and offers no retry button', async ({ page }) => {
await page.addInitScript(() => {
Object.defineProperty(window.Notification, 'permission', { value: 'denied', configurable: true })
})
await page.goto('/')
await page.getByRole('link', { name: 'Settings' }).click()
const control = page.getByTestId('browser-notify-control')
await control.scrollIntoViewIfNeeded()
await expect(page.getByTestId('browser-notify-denied')).toBeVisible()
await expect(page.getByTestId('browser-notify-denied')).toContainText('browser')
await expect(page.getByTestId('browser-notify-enable')).toHaveCount(0)
})

test('clicking enable calls requestPermission and its result flips the control to granted', async ({ page }) => {
// No headless UI exists to click Allow on the real permission
// prompt requestPermission() would otherwise show, so the prompt
// itself is stubbed to resolve 'granted' -- the click and the
// resulting UI transition are both real, only the browser-native
// prompt in between is faked.
await page.addInitScript(() => {
Object.defineProperty(window.Notification, 'permission', { value: 'default', configurable: true })
window.Notification.requestPermission = () => Promise.resolve('granted')
})
await page.goto('/')
await page.getByRole('link', { name: 'Settings' }).click()
const control = page.getByTestId('browser-notify-control')
await control.scrollIntoViewIfNeeded()
await page.getByTestId('browser-notify-enable').click()
await expect(page.getByTestId('browser-notify-granted')).toBeVisible()
await expect(page.getByTestId('browser-notify-enable')).toHaveCount(0)
})
})

// This spec's assertions read the GLOBAL paired-devices list
// (testing.md's shared-pool-vs-dedicated triage), so each test below
// seeds its own uniquely-labeled device and revokes it before
Expand Down
6 changes: 5 additions & 1 deletion frontend/src/app/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { applyDensity } from "../shared/density";
import { pageIconFor, pageLabelFor } from './pageMeta'
import { useMillNavigate } from './useMillNavigate'
import { useKeymapDispatch } from './useKeymapDispatch'
import { useBrowserNotify } from './useBrowserNotify'
import styles from "./App.module.css";
import { newLocalID } from '../shared/localId'

Expand Down Expand Up @@ -265,6 +266,8 @@ function App() {

useMillNavigate(setView);

const notifyBrowserTab = useBrowserNotify(buildInfo?.Server === true);

useEffect(() => {
return Events.On('hotkey-activity', (evt) => {
pushActivity({
Expand Down Expand Up @@ -363,14 +366,15 @@ function App() {
if (notifiedIds.current.has(item.key)) continue;
notifiedIds.current.add(item.key);
void SettingsService.NotifyPendingApproval(item.id, item.description, item.kind, document.hasFocus()).catch(() => {});
if (item.kind === 'guardrail') notifyBrowserTab({ dedupeKey: item.id, title: t('browserNotificationTitle'), body: item.description, onClick: () => setView({ kind: 'review' }) });
}
});
};
refresh();
const offGuardrail = Events.On('guardrail-pending-changed', refresh);
const offMCP = Events.On('mcp-write-approval', refresh);
return () => { offGuardrail(); offMCP(); };
}, [t]);
}, [t, notifyBrowserTab, setView]);

return (
<div className="app-shell" data-sidebar-open={sidebarOpen} data-view={view.kind}>
Expand Down
36 changes: 36 additions & 0 deletions frontend/src/app/browserNotifyPredicate.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { describe, expect, it } from 'vitest'
import { shouldNotifyBrowserTab } from './browserNotifyPredicate'

describe('shouldNotifyBrowserTab', () => {
it('notifies when server mode, unfocused, and unseen', () => {
expect(shouldNotifyBrowserTab({ isServerMode: true, hasFocus: false, alreadyNotified: false })).toBe(true)
})

it('never notifies in desktop mode, even unfocused and unseen', () => {
expect(shouldNotifyBrowserTab({ isServerMode: false, hasFocus: false, alreadyNotified: false })).toBe(false)
})

it('never notifies while the tab is focused', () => {
expect(shouldNotifyBrowserTab({ isServerMode: true, hasFocus: true, alreadyNotified: false })).toBe(false)
})

it('never notifies twice for the same pending id', () => {
expect(shouldNotifyBrowserTab({ isServerMode: true, hasFocus: false, alreadyNotified: true })).toBe(false)
})

it('never notifies when desktop mode AND focused', () => {
expect(shouldNotifyBrowserTab({ isServerMode: false, hasFocus: true, alreadyNotified: false })).toBe(false)
})

it('never notifies when desktop mode AND already notified', () => {
expect(shouldNotifyBrowserTab({ isServerMode: false, hasFocus: false, alreadyNotified: true })).toBe(false)
})

it('never notifies when focused AND already notified', () => {
expect(shouldNotifyBrowserTab({ isServerMode: true, hasFocus: true, alreadyNotified: true })).toBe(false)
})

it('never notifies when all three disqualifying conditions hold', () => {
expect(shouldNotifyBrowserTab({ isServerMode: false, hasFocus: true, alreadyNotified: true })).toBe(false)
})
})
18 changes: 18 additions & 0 deletions frontend/src/app/browserNotifyPredicate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
export interface ShouldNotifyBrowserTabInput {
isServerMode: boolean
hasFocus: boolean
alreadyNotified: boolean
}

// docs/goals/0132-remote-access.md SLICE A: a parked approval raises a
// Notifications-API banner on this tab only when all three hold at
// once -- server mode (desktop already has its own native banner and
// must never double-fire alongside it), the tab not focused (nothing
// to interrupt if the user is already looking at it), and this
// pending id not already notified (once per park, never repeated on
// re-render or reconnect). Pulled into its own dependency-free module
// so it can be unit-tested without pulling the DOM-touching
// Notification API through it.
export function shouldNotifyBrowserTab(input: ShouldNotifyBrowserTabInput): boolean {
return input.isServerMode && !input.hasFocus && !input.alreadyNotified
}
35 changes: 35 additions & 0 deletions frontend/src/app/useBrowserNotify.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { useCallback, useRef } from 'react'
import { getNotificationPermission, raiseNotification } from '../shared/browserNotify'
import { shouldNotifyBrowserTab } from './browserNotifyPredicate'

export interface BrowserNotifyRequest {
// Identifies the thing being notified about (a run id, a pending
// approval id, ...) -- never notified twice for the same key.
dedupeKey: string
title: string
body: string
// What clicking the raised notification does: focus this tab (handled
// by raiseNotification itself) and land on whatever the caller
// considers "the thing that needs attention" -- an in-app setView
// call, not a URL, since the tab is already alive.
onClick: () => void
}

// docs/goals/0132-remote-access.md SLICE A: the one browser-tab
// notification seam every consumer goes through, never a bespoke
// notifier per event type. A new event that wants this (a finished
// run, an agent action while away) is a new call site with its own
// dedupeKey/title/body/onClick -- never a change here. The parked-
// approval call in App.tsx is this seam's first consumer, not its only
// intended one.
export function useBrowserNotify(isServerMode: boolean) {
const notifiedKeys = useRef<Set<string>>(new Set())

return useCallback((req: BrowserNotifyRequest) => {
const alreadyNotified = notifiedKeys.current.has(req.dedupeKey)
if (!shouldNotifyBrowserTab({ isServerMode, hasFocus: document.hasFocus(), alreadyNotified })) return
if (getNotificationPermission() !== 'granted') return
notifiedKeys.current.add(req.dedupeKey)
raiseNotification(req.title, req.body, req.onClick)
}, [isServerMode])
}
1 change: 1 addition & 0 deletions frontend/src/locales/en/app.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"reviewPendingAriaLabel": "{{count}} pending in Review",
"pendingApprovalDescription": "{{workflowLabel}}: {{step}} needs approval",
"pendingApprovalStepFallback": "a step",
"browserNotificationTitle": "Approval needed",
"openTabs": "Open tabs",
"search": {
"noMatchesTitle": "No matches",
Expand Down
9 changes: 8 additions & 1 deletion frontend/src/locales/en/views.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,14 @@
"renameAriaLabel": "Rename {{label}}",
"renameInputAriaLabel": "Device name",
"renameSaveAriaLabel": "Save name",
"renameCancelAriaLabel": "Cancel rename"
"renameCancelAriaLabel": "Cancel rename",
"notify": {
"enable": "Notify me on this device",
"caption": "Alerts you here when a decision needs your action and this tab isn't in view.",
"granted": "Notifications are on for this device.",
"denied": "Notifications are blocked for this device. Turn them on again in your browser's site settings.",
"unsupported": "Notifications aren't available over this connection. It needs to be secure (https)."
}
},
"contract": {
"description": "The full step catalog, every data schema, and this app's version in one file — hand it to an agent that can't reach Mill directly.",
Expand Down
37 changes: 37 additions & 0 deletions frontend/src/shared/browserNotify.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
export type BrowserNotifyPermission = 'unsupported' | 'default' | 'granted' | 'denied'

// The Notifications API is a secure-context-only browser feature --
// unavailable over a plain http connection (a Tailscale-reached
// server-mode instance today has no TLS). window.isSecureContext
// already covers this (true for https and for loopback origins, false
// for a remote http origin), so every caller goes through this door
// rather than touching `Notification` directly -- same pattern as
// clipboardWrite.ts for navigator.clipboard.
export function getNotificationPermission(): BrowserNotifyPermission {
if (typeof window === 'undefined' || !window.isSecureContext || !('Notification' in window)) {
return 'unsupported'
}
return Notification.permission
}

// Must only be called from a user gesture (a click handler) -- browsers
// silently resolve to 'denied' when requestPermission fires any other
// way.
export async function requestNotificationPermission(): Promise<BrowserNotifyPermission> {
if (getNotificationPermission() === 'unsupported') return 'unsupported'
return Notification.requestPermission()
}

// Raises the OS notification. No-ops when permission isn't actually
// granted, so a caller can't accidentally trigger a browser permission
// prompt by calling this directly. Clicking it focuses this tab and
// hands off to onClick before closing the notification.
export function raiseNotification(title: string, body: string, onClick: () => void): void {
if (getNotificationPermission() !== 'granted') return
const notification = new Notification(title, { body })
notification.onclick = () => {
window.focus()
onClick()
notification.close()
}
}
48 changes: 47 additions & 1 deletion frontend/src/views/RemoteAccessSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { useTranslation } from 'react-i18next'
import { ActionList, Button, IconButton, Stack, Text, TextInput } from '@primer/react'
import { Blankslate } from '@primer/react/experimental'
import { CheckIcon, DeviceMobileIcon, PencilIcon, PlusIcon, XIcon } from '@primer/octicons-react'
import { RemoteAuthService } from '../shared/bindings'
import { RemoteAuthService, SettingsService } from '../shared/bindings'
import type { DeviceInfo, PairingCodeInfo } from '../shared/bindings'
import { ConfirmDialog } from '../shared/ConfirmDialog'
import { formatUpdated } from '../shared/inventorySort'
import { getNotificationPermission, requestNotificationPermission } from '../shared/browserNotify'
import type { BrowserNotifyPermission } from '../shared/browserNotify'
import listStyles from '../shared/ListCard.module.css'
import monoStyles from '../shared/monoText.module.css'

Expand All @@ -25,6 +27,20 @@ function RemoteAccessSection() {
const [revoking, setRevoking] = useState<DeviceInfo | null>(null)
const [renamingId, setRenamingId] = useState<string | null>(null)
const [renameDraft, setRenameDraft] = useState('')
// docs/goals/0132 SLICE A: this control has no reason to render on a
// desktop build -- the desktop path already gets a native banner with
// no opt-in, and browserNotifyPredicate.shouldNotifyBrowserTab never
// fires there anyway.
const [isServerMode, setIsServerMode] = useState(false)
const [notifyPermission, setNotifyPermission] = useState<BrowserNotifyPermission>(() => getNotificationPermission())

useEffect(() => {
SettingsService.GetBuildInfo().then((info) => setIsServerMode(info?.Server === true)).catch(() => {})
}, [])

const enableNotify = () => {
requestNotificationPermission().then(setNotifyPermission).catch(() => {})
}

const refresh = () => {
RemoteAuthService.ListDevices()
Expand Down Expand Up @@ -90,6 +106,36 @@ function RemoteAccessSection() {
{t('settings.remoteAccess.macAlwaysAllowed')}
</Text>

{isServerMode && (
<Stack direction="vertical" gap="condensed" style={{ marginTop: 'var(--base-size-16)' }} data-testid="browser-notify-control">
{notifyPermission === 'unsupported' && (
<Text as="p" size="small" className={listStyles.muted} data-testid="browser-notify-unsupported">
{t('settings.remoteAccess.notify.unsupported')}
</Text>
)}
{notifyPermission === 'default' && (
<>
<Button size="small" onClick={enableNotify} data-testid="browser-notify-enable">
{t('settings.remoteAccess.notify.enable')}
</Button>
<Text as="p" size="small" className={listStyles.muted}>
{t('settings.remoteAccess.notify.caption')}
</Text>
</>
)}
{notifyPermission === 'granted' && (
<Text as="p" size="small" data-testid="browser-notify-granted">
{t('settings.remoteAccess.notify.granted')}
</Text>
)}
{notifyPermission === 'denied' && (
<Text as="p" size="small" className={listStyles.error} data-testid="browser-notify-denied">
{t('settings.remoteAccess.notify.denied')}
</Text>
)}
</Stack>
)}

<Button
size="small"
leadingVisual={PlusIcon}
Expand Down
8 changes: 8 additions & 0 deletions userdocs/llms-full.txt
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,14 @@ from? Stop it, delete its saved device list, and start it again — it
writes a fresh code to its log, same as an instance that's never been
paired.

On a phone or another computer's browser tab, turn on "Notify me on
this device" in Settings > Remote access to get a notification when a
decision needs your action. Your browser asks for permission the first
time. Notifications only appear while that tab isn't in view, and
clicking one brings you straight to the item waiting for you. If
notifications are blocked, turn them back on in your browser's site
settings — Mill can't ask again automatically.

## Backups

Mill snapshots your workflow history and settings automatically —
Expand Down
8 changes: 8 additions & 0 deletions userdocs/reference/settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ from? Stop it, delete its saved device list, and start it again — it
writes a fresh code to its log, same as an instance that's never been
paired.

On a phone or another computer's browser tab, turn on "Notify me on
this device" in Settings > Remote access to get a notification when a
decision needs your action. Your browser asks for permission the first
time. Notifications only appear while that tab isn't in view, and
clicking one brings you straight to the item waiting for you. If
notifications are blocked, turn them back on in your browser's site
settings — Mill can't ask again automatically.

## Backups

Mill snapshots your workflow history and settings automatically —
Expand Down
Loading