Archived
fix(sessions): preserve notification inbox authority
This commit is contained in:
@@ -16,6 +16,7 @@ export class MachineSwitcher extends LitElement implements KeyboardNavigableSect
|
|||||||
@property({ attribute: false }) statuses: Record<string, MachineHealth> = {};
|
@property({ attribute: false }) statuses: Record<string, MachineHealth> = {};
|
||||||
@property({ attribute: false }) activities: Record<string, Record<string, WorkspaceActivity>> = {};
|
@property({ attribute: false }) activities: Record<string, Record<string, WorkspaceActivity>> = {};
|
||||||
@property({ attribute: false }) notificationBadges: Record<string, SessionNotificationBadgeModel | undefined> = {};
|
@property({ attribute: false }) notificationBadges: Record<string, SessionNotificationBadgeModel | undefined> = {};
|
||||||
|
@property({ attribute: false }) notificationHeadingBadge?: SessionNotificationBadgeModel;
|
||||||
@property({ attribute: false }) onSelect?: (machine: Machine) => void | Promise<void>;
|
@property({ attribute: false }) onSelect?: (machine: Machine) => void | Promise<void>;
|
||||||
@property({ attribute: false }) onRemove?: (machine: Machine) => void | Promise<void>;
|
@property({ attribute: false }) onRemove?: (machine: Machine) => void | Promise<void>;
|
||||||
@property({ attribute: false }) onFocusNextSection?: () => void | Promise<void>;
|
@property({ attribute: false }) onFocusNextSection?: () => void | Promise<void>;
|
||||||
@@ -57,13 +58,14 @@ export class MachineSwitcher extends LitElement implements KeyboardNavigableSect
|
|||||||
if (selected === undefined) return null;
|
if (selected === undefined) return null;
|
||||||
const status = machineStatus(selected, this.statuses);
|
const status = machineStatus(selected, this.statuses);
|
||||||
const label = selected.name;
|
const label = selected.name;
|
||||||
|
const notificationBadge = machineSwitcherNotificationBadge(selected.id, this.notificationBadges, this.notificationHeadingBadge);
|
||||||
return html`
|
return html`
|
||||||
<div class="machine-switcher">
|
<div class="machine-switcher">
|
||||||
<button
|
<button
|
||||||
type="button"
|
type="button"
|
||||||
class="machine-switcher-button"
|
class="machine-switcher-button"
|
||||||
title=${machineTitle(selected)}
|
title=${machineTitle(selected)}
|
||||||
aria-label=${this.machineSwitcherAriaLabel(selected)}
|
aria-label=${this.machineSwitcherAriaLabel(selected, notificationBadge)}
|
||||||
aria-expanded=${String(this.open)}
|
aria-expanded=${String(this.open)}
|
||||||
@click=${(event: MouseEvent) => { this.toggleMenu(event.currentTarget); }}
|
@click=${(event: MouseEvent) => { this.toggleMenu(event.currentTarget); }}
|
||||||
@keydown=${(event: KeyboardEvent) => { this.handleSwitcherButtonKeydown(event); }}
|
@keydown=${(event: KeyboardEvent) => { this.handleSwitcherButtonKeydown(event); }}
|
||||||
@@ -74,7 +76,7 @@ export class MachineSwitcher extends LitElement implements KeyboardNavigableSect
|
|||||||
<span class="machine-switcher-label">${label}</span>
|
<span class="machine-switcher-label">${label}</span>
|
||||||
</span>
|
</span>
|
||||||
<span class=${`machine-status ${status}`}>${machineStatusLabel(status)}</span>
|
<span class=${`machine-status ${status}`}>${machineStatusLabel(status)}</span>
|
||||||
${this.notificationBadges[selected.id] === undefined ? null : html`<notification-badge .model=${this.notificationBadges[selected.id]}></notification-badge>`}
|
${notificationBadge === undefined ? null : html`<notification-badge .model=${notificationBadge}></notification-badge>`}
|
||||||
<span class="machine-chevron" aria-hidden="true">▾</span>
|
<span class="machine-chevron" aria-hidden="true">▾</span>
|
||||||
</button>
|
</button>
|
||||||
${this.open ? html`
|
${this.open ? html`
|
||||||
@@ -136,9 +138,9 @@ export class MachineSwitcher extends LitElement implements KeyboardNavigableSect
|
|||||||
return this.selected ?? this.machines.find((machine) => machine.id === "local") ?? this.machines[0];
|
return this.selected ?? this.machines.find((machine) => machine.id === "local") ?? this.machines[0];
|
||||||
}
|
}
|
||||||
|
|
||||||
private machineSwitcherAriaLabel(machine: Machine): string {
|
private machineSwitcherAriaLabel(machine: Machine, notificationBadge: SessionNotificationBadgeModel | undefined): string {
|
||||||
const notificationLabel = this.notificationBadges[machine.id]?.accessibleLabel;
|
const notificationLabel = notificationBadge?.accessibleLabel;
|
||||||
return `Machine: ${machine.name}.${notificationLabel === undefined ? "" : ` ${notificationLabel}.`} Switch machine.`;
|
return `Machine: ${machine.name}.${notificationLabel === undefined ? "" : ` Notifications across machines: ${notificationLabel}.`} Switch machine.`;
|
||||||
}
|
}
|
||||||
|
|
||||||
private switcherButton(): HTMLElement | null {
|
private switcherButton(): HTMLElement | null {
|
||||||
@@ -312,6 +314,14 @@ export class MachineSwitcher extends LitElement implements KeyboardNavigableSect
|
|||||||
`;
|
`;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export function machineSwitcherNotificationBadge(
|
||||||
|
selectedMachineId: string,
|
||||||
|
notificationBadges: Readonly<Record<string, SessionNotificationBadgeModel | undefined>>,
|
||||||
|
notificationHeadingBadge: SessionNotificationBadgeModel | undefined,
|
||||||
|
): SessionNotificationBadgeModel | undefined {
|
||||||
|
return notificationHeadingBadge ?? notificationBadges[selectedMachineId];
|
||||||
|
}
|
||||||
|
|
||||||
function machineStatus(machine: Machine, statuses: Record<string, MachineHealth>): MachineStatus {
|
function machineStatus(machine: Machine, statuses: Record<string, MachineHealth>): MachineStatus {
|
||||||
return statuses[machine.id]?.status ?? machine.status ?? "unknown";
|
return statuses[machine.id]?.status ?? machine.status ?? "unknown";
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest";
|
|||||||
import { initialAppState, type AppState } from "../appState";
|
import { initialAppState, type AppState } from "../appState";
|
||||||
import type { SessionInfo } from "../api";
|
import type { SessionInfo } from "../api";
|
||||||
import type { NavigationNotificationBadges } from "./appShell/AppNavigationPanel";
|
import type { NavigationNotificationBadges } from "./appShell/AppNavigationPanel";
|
||||||
|
import { machineSwitcherNotificationBadge } from "./MachineSwitcher";
|
||||||
import { PiWebApp } from "./PiWebApp";
|
import { PiWebApp } from "./PiWebApp";
|
||||||
|
|
||||||
const currentSession: SessionInfo = {
|
const currentSession: SessionInfo = {
|
||||||
@@ -83,6 +84,8 @@ describe("PiWebApp notification hierarchy models", () => {
|
|||||||
expect(badges.projects["project-1"]).toMatchObject({ text: "3+", severity: "error" });
|
expect(badges.projects["project-1"]).toMatchObject({ text: "3+", severity: "error" });
|
||||||
expect(badges.machines["local"]).toMatchObject({ text: "3+", severity: "error" });
|
expect(badges.machines["local"]).toMatchObject({ text: "3+", severity: "error" });
|
||||||
expect(badges.machines["remote"]).toMatchObject({ text: "5", severity: "info" });
|
expect(badges.machines["remote"]).toMatchObject({ text: "5", severity: "info" });
|
||||||
|
expect(badges.machinesHeading).toMatchObject({ text: "8+", severity: "error" });
|
||||||
|
expect(machineSwitcherNotificationBadge("local", badges.machines, badges.machinesHeading)).toBe(badges.machinesHeading);
|
||||||
expect(badges.sessionsHeading).toMatchObject({ text: "2", severity: "error" });
|
expect(badges.sessionsHeading).toMatchObject({ text: "2", severity: "error" });
|
||||||
expect(mobile).toMatchObject({ text: "8+", severity: "error" });
|
expect(mobile).toMatchObject({ text: "8+", severity: "error" });
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -116,6 +116,7 @@ export class AppNavigationPanel extends LitElement {
|
|||||||
.statuses=${this.machineStatuses}
|
.statuses=${this.machineStatuses}
|
||||||
.activities=${this.machineActivities}
|
.activities=${this.machineActivities}
|
||||||
.notificationBadges=${this.notificationBadges.machines}
|
.notificationBadges=${this.notificationBadges.machines}
|
||||||
|
.notificationHeadingBadge=${this.notificationBadges.machinesHeading}
|
||||||
.onSelect=${(machine: Machine) => this.onSelectMachine?.(machine)}
|
.onSelect=${(machine: Machine) => this.onSelectMachine?.(machine)}
|
||||||
.onRemove=${(machine: Machine) => this.onRemoveMachine?.(machine)}
|
.onRemove=${(machine: Machine) => this.onRemoveMachine?.(machine)}
|
||||||
.onFocusNextSection=${() => { this.focusNextFrom("machines"); }}
|
.onFocusNextSection=${() => { this.focusNextFrom("machines"); }}
|
||||||
|
|||||||
@@ -337,6 +337,31 @@ describe("SessionNotificationController capability and joins", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
describe("SessionNotificationController optimistic mutations", () => {
|
describe("SessionNotificationController optimistic mutations", () => {
|
||||||
|
it("does not let a delayed refresh snapshot roll back a newer dismissal response", async () => {
|
||||||
|
const initial = inboxSnapshot([entry(1)], { inboxRevision: 1, catalogRevision: 1 });
|
||||||
|
const delayedRefresh = deferred<SessionNotificationInboxSnapshot>();
|
||||||
|
const notificationInbox = vi.fn()
|
||||||
|
.mockResolvedValueOnce(initial)
|
||||||
|
.mockImplementationOnce(() => delayedRefresh.promise);
|
||||||
|
const dismissed = inboxSnapshot([], { inboxRevision: 2, catalogRevision: 2 });
|
||||||
|
const harness = createHarness(capableState(), {
|
||||||
|
notificationInbox,
|
||||||
|
dismissNotification: vi.fn(() => Promise.resolve(dismissed)),
|
||||||
|
});
|
||||||
|
|
||||||
|
harness.controller.prepareSelectedSession(session, "local");
|
||||||
|
await harness.controller.refreshSelectedSession(session, "local");
|
||||||
|
const refresh = harness.controller.refreshSelectedSession(session, "local");
|
||||||
|
await harness.controller.dismissNotification("daemon-a:1");
|
||||||
|
|
||||||
|
expect(selectedNotificationView(harness.state.selectedNotificationInbox)?.notifications).toEqual([]);
|
||||||
|
delayedRefresh.resolve(initial);
|
||||||
|
await refresh;
|
||||||
|
|
||||||
|
expect(harness.state.selectedNotificationInbox?.summary?.inboxRevision).toBe(2);
|
||||||
|
expect(selectedNotificationView(harness.state.selectedNotificationInbox)?.notifications).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
it("optimistically dismisses one card, reconciles the response, and rolls back/refetches on failure", async () => {
|
it("optimistically dismisses one card, reconciles the response, and rolls back/refetches on failure", async () => {
|
||||||
const dismiss = deferred<SessionNotificationInboxSnapshot>();
|
const dismiss = deferred<SessionNotificationInboxSnapshot>();
|
||||||
const refreshAfterFailure = deferred<SessionNotificationInboxSnapshot>();
|
const refreshAfterFailure = deferred<SessionNotificationInboxSnapshot>();
|
||||||
|
|||||||
@@ -301,7 +301,10 @@ export class SessionNotificationController {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
this.acceptedSupportByMachine.add(target.machineId);
|
this.acceptedSupportByMachine.add(target.machineId);
|
||||||
let inbox = installSelectedNotificationSnapshot(this.getState().selectedNotificationInbox, target, snapshot);
|
const current = this.getState().selectedNotificationInbox;
|
||||||
|
let inbox = current === undefined || shouldInstallSelectedSnapshot(current, target, snapshot)
|
||||||
|
? installSelectedNotificationSnapshot(current, target, snapshot)
|
||||||
|
: current;
|
||||||
const catalogEvents = [snapshotSummaryEvent(snapshot)];
|
const catalogEvents = [snapshotSummaryEvent(snapshot)];
|
||||||
for (const event of [...join.events].sort((left, right) => left.summary.inboxRevision - right.summary.inboxRevision)) {
|
for (const event of [...join.events].sort((left, right) => left.summary.inboxRevision - right.summary.inboxRevision)) {
|
||||||
const result = applySelectedNotificationEvent(inbox, target, event);
|
const result = applySelectedNotificationEvent(inbox, target, event);
|
||||||
@@ -389,10 +392,9 @@ export class SessionNotificationController {
|
|||||||
this.setState({ selectedNotificationInbox: { ...removeOverlay(current), status: "stale" } });
|
this.setState({ selectedNotificationInbox: { ...removeOverlay(current), status: "stale" } });
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const shouldInstall = current.daemonInstanceId !== snapshot.daemonInstanceId
|
const authoritative = shouldInstallSelectedSnapshot(current, target, snapshot)
|
||||||
|| current.summary === undefined
|
? installSelectedNotificationSnapshot(current, target, snapshot)
|
||||||
|| snapshot.summary.inboxRevision >= current.summary.inboxRevision;
|
: current;
|
||||||
const authoritative = shouldInstall ? installSelectedNotificationSnapshot(current, target, snapshot) : current;
|
|
||||||
this.setState({ selectedNotificationInbox: removeOverlay(authoritative) });
|
this.setState({ selectedNotificationInbox: removeOverlay(authoritative) });
|
||||||
this.applyCatalogSummary(target.machineId, snapshotSummaryEvent(snapshot));
|
this.applyCatalogSummary(target.machineId, snapshotSummaryEvent(snapshot));
|
||||||
}
|
}
|
||||||
@@ -578,6 +580,17 @@ function targetFromInbox(inbox: SelectedSessionNotificationInbox): SessionNotifi
|
|||||||
return { machineId: inbox.machineId, sessionId: inbox.sessionId, cwd: inbox.cwd };
|
return { machineId: inbox.machineId, sessionId: inbox.sessionId, cwd: inbox.cwd };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function shouldInstallSelectedSnapshot(
|
||||||
|
current: SelectedSessionNotificationInbox,
|
||||||
|
target: SessionNotificationTarget,
|
||||||
|
snapshot: SessionNotificationInboxSnapshot,
|
||||||
|
): boolean {
|
||||||
|
return !notificationTargetsEqual(current, target)
|
||||||
|
|| current.daemonInstanceId !== snapshot.daemonInstanceId
|
||||||
|
|| current.summary === undefined
|
||||||
|
|| snapshot.summary.inboxRevision >= current.summary.inboxRevision;
|
||||||
|
}
|
||||||
|
|
||||||
async function forEachWithConcurrency<T>(items: readonly T[], concurrency: number, worker: (item: T) => Promise<void>): Promise<void> {
|
async function forEachWithConcurrency<T>(items: readonly T[], concurrency: number, worker: (item: T) => Promise<void>): Promise<void> {
|
||||||
let nextIndex = 0;
|
let nextIndex = 0;
|
||||||
async function run(): Promise<void> {
|
async function run(): Promise<void> {
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
import { mkdtemp, rm, writeFile } from "node:fs/promises";
|
import { mkdtemp, rm, writeFile } from "node:fs/promises";
|
||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import { join } from "node:path";
|
import { join, resolve, sep } from "node:path";
|
||||||
import { describe, expect, it, vi } from "vitest";
|
import { describe, expect, it, vi } from "vitest";
|
||||||
import { PiSessionService, type PiAgentSession, type PiSessionRuntime } from "./piSessionService.js";
|
import { PiSessionService, type PiAgentSession, type PiSessionRuntime } from "./piSessionService.js";
|
||||||
import { SessionNotificationStore } from "./sessionNotificationStore.js";
|
import { SessionNotificationStore } from "./sessionNotificationStore.js";
|
||||||
@@ -404,8 +404,10 @@ describe("PiSessionService lifecycle, listing, and reload", () => {
|
|||||||
const hub = new CapturingSessionEventHub();
|
const hub = new CapturingSessionEventHub();
|
||||||
const store = notificationStore();
|
const store = notificationStore();
|
||||||
const branch = [{ type: "message", message: { role: "user", content: "existing" } }];
|
const branch = [{ type: "message", message: { role: "user", content: "existing" } }];
|
||||||
|
const canonicalCwd = resolve(tmpdir(), "pi-web-notification-workspace");
|
||||||
|
const rawEquivalentCwd = `${canonicalCwd}${sep}nested${sep}..`;
|
||||||
const fake = fakeRuntime("notification-session", {
|
const fake = fakeRuntime("notification-session", {
|
||||||
sessionManager: fakeSessionManager("/workspace", {
|
sessionManager: fakeSessionManager(rawEquivalentCwd, {
|
||||||
getSessionId: () => "notification-session",
|
getSessionId: () => "notification-session",
|
||||||
getBranch: () => branch,
|
getBranch: () => branch,
|
||||||
}),
|
}),
|
||||||
@@ -419,12 +421,13 @@ describe("PiSessionService lifecycle, listing, and reload", () => {
|
|||||||
heartbeatIntervalMs: 60_000,
|
heartbeatIntervalMs: 60_000,
|
||||||
});
|
});
|
||||||
|
|
||||||
await service.start("/workspace");
|
await service.start(canonicalCwd);
|
||||||
const notify = boundNotify(fake);
|
const notify = boundNotify(fake);
|
||||||
notify("duplicate", "warning");
|
notify("duplicate", "warning");
|
||||||
notify("duplicate", "error");
|
notify("duplicate", "error");
|
||||||
|
|
||||||
const snapshot = service.notificationInbox(sessionRef("notification-session"));
|
const snapshot = service.notificationInbox({ id: "notification-session", cwd: canonicalCwd });
|
||||||
|
expect(snapshot.summary.cwd).toBe(canonicalCwd);
|
||||||
expect(snapshot.notifications).toMatchObject([
|
expect(snapshot.notifications).toMatchObject([
|
||||||
{ id: "daemon-lifecycle-test:2", message: "duplicate", severity: "error" },
|
{ id: "daemon-lifecycle-test:2", message: "duplicate", severity: "error" },
|
||||||
{ id: "daemon-lifecycle-test:1", message: "duplicate", severity: "warning" },
|
{ id: "daemon-lifecycle-test:1", message: "duplicate", severity: "warning" },
|
||||||
|
|||||||
@@ -1429,10 +1429,7 @@ export class PiSessionService implements SessionRouteService {
|
|||||||
try {
|
try {
|
||||||
await session.reload(priorGeneration === undefined ? undefined : {
|
await session.reload(priorGeneration === undefined ? undefined : {
|
||||||
beforeSessionStart: () => {
|
beforeSessionStart: () => {
|
||||||
candidateGeneration = this.notificationStore.beginReplacement(priorGeneration, {
|
candidateGeneration = this.notificationStore.beginReplacement(priorGeneration, notificationIdentityForSession(session));
|
||||||
sessionId: session.sessionId,
|
|
||||||
cwd: session.sessionManager.getCwd(),
|
|
||||||
});
|
|
||||||
this.notificationGenerationBySession.set(session, candidateGeneration);
|
this.notificationGenerationBySession.set(session, candidateGeneration);
|
||||||
this.replaceSessionNotificationContext(session, candidateGeneration);
|
this.replaceSessionNotificationContext(session, candidateGeneration);
|
||||||
},
|
},
|
||||||
@@ -1629,8 +1626,7 @@ export class PiSessionService implements SessionRouteService {
|
|||||||
if (this.hasActiveWork(session)) throw new Error("Stop current session activity before reloading");
|
if (this.hasActiveWork(session)) throw new Error("Stop current session activity before reloading");
|
||||||
|
|
||||||
const priorGeneration = this.notificationGenerationBySession.get(session);
|
const priorGeneration = this.notificationGenerationBySession.get(session);
|
||||||
const sessionId = session.sessionId;
|
const { sessionId, cwd } = notificationIdentityForSession(session);
|
||||||
const cwd = session.sessionManager.getCwd();
|
|
||||||
let candidateGeneration: SessionNotificationGeneration | undefined;
|
let candidateGeneration: SessionNotificationGeneration | undefined;
|
||||||
try {
|
try {
|
||||||
await this.closeActive(
|
await this.closeActive(
|
||||||
@@ -2016,17 +2012,18 @@ export class PiSessionService implements SessionRouteService {
|
|||||||
: "external";
|
: "external";
|
||||||
|
|
||||||
if (notificationOwnership === "registered") {
|
if (notificationOwnership === "registered") {
|
||||||
|
const notificationIdentity = notificationIdentityForSession(runtime.session);
|
||||||
const existingCandidate = this.notificationStore.beginReplacementForSession(
|
const existingCandidate = this.notificationStore.beginReplacementForSession(
|
||||||
runtime.session.sessionId,
|
notificationIdentity.sessionId,
|
||||||
runtime.session.sessionManager.getCwd(),
|
notificationIdentity.cwd,
|
||||||
);
|
);
|
||||||
if (existingCandidate !== undefined) {
|
if (existingCandidate !== undefined) {
|
||||||
notificationGeneration = existingCandidate;
|
notificationGeneration = existingCandidate;
|
||||||
notificationOwnership = "replacement";
|
notificationOwnership = "replacement";
|
||||||
} else {
|
} else {
|
||||||
const registration = this.notificationStore.registerSession(
|
const registration = this.notificationStore.registerSession(
|
||||||
runtime.session.sessionId,
|
notificationIdentity.sessionId,
|
||||||
runtime.session.sessionManager.getCwd(),
|
notificationIdentity.cwd,
|
||||||
);
|
);
|
||||||
notificationGeneration = registration.generation;
|
notificationGeneration = registration.generation;
|
||||||
this.publishNotificationMutations(registration.mutations);
|
this.publishNotificationMutations(registration.mutations);
|
||||||
@@ -2042,10 +2039,7 @@ export class PiSessionService implements SessionRouteService {
|
|||||||
let candidateGeneration: SessionNotificationGeneration | undefined;
|
let candidateGeneration: SessionNotificationGeneration | undefined;
|
||||||
try {
|
try {
|
||||||
if (priorGeneration !== undefined) {
|
if (priorGeneration !== undefined) {
|
||||||
candidateGeneration = this.notificationStore.beginReplacement(priorGeneration, {
|
candidateGeneration = this.notificationStore.beginReplacement(priorGeneration, notificationIdentityForSession(session));
|
||||||
sessionId: session.sessionId,
|
|
||||||
cwd: session.sessionManager.getCwd(),
|
|
||||||
});
|
|
||||||
this.notificationGenerationBySession.set(session, candidateGeneration);
|
this.notificationGenerationBySession.set(session, candidateGeneration);
|
||||||
}
|
}
|
||||||
this.bindRuntime(active, session);
|
this.bindRuntime(active, session);
|
||||||
@@ -2512,6 +2506,13 @@ function modelToClientModel(model: PiAgentSession["model"]): ClientSessionModel
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function notificationIdentityForSession(session: PiAgentSession): { sessionId: string; cwd: string } {
|
||||||
|
return {
|
||||||
|
sessionId: session.sessionId,
|
||||||
|
cwd: canonicalizeStoredCwd(session.sessionManager.getCwd()),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
function clientSessionFromListEntry(session: PiSessionListEntry): ClientSession {
|
function clientSessionFromListEntry(session: PiSessionListEntry): ClientSession {
|
||||||
return {
|
return {
|
||||||
id: session.id,
|
id: session.id,
|
||||||
|
|||||||
Reference in New Issue
Block a user