Skip to content

Restore model and effort picks when a background send is recovered #887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kalvinnchau merged 3 commits into from
Oct 13, 2026
Merged

Restore model and effort picks when a background send is recovered #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12c6b45
fix(composer): restore model and effort picks when a background send …
matt2e Oct 12, 2026
ae59c11
refactor(composer): name the recovered picks parameter distinctly
matt2e Oct 12, 2026
0716636
fix(composer): retain recovered picks until an unmounted composer ret…
matt2e Oct 12, 2026
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
160 changes: 148 additions & 12 deletions src/features/messages/MessageComposer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,9 @@ async function mountUploadComposer(
editable?: boolean;
media?: (url: string) => string | undefined;
extensions?: MessageComposerProps["extensions"];
agents?: Agents;
/** Channel members besides the viewer; only members can be mentioned. */
members?: readonly string[];
} = {},
) {
vi.stubGlobal(
Expand Down Expand Up @@ -1021,7 +1024,14 @@ async function mountUploadComposer(
? [roster(relay, "other", other.members, other.time)]
: [metadata(relay, "other", "Random", other.time)]
: filter.kinds?.includes(39002)
? [roster(relay, "channel", [viewer.pubkey], 1700000000)]
? [
roster(
relay,
"channel",
[viewer.pubkey, ...(options.members ?? [])],
1700000000,
),
]
: filter.kinds?.includes(39000)
? [metadata(relay, "channel", "General", 1700000000)]
: [],
Expand Down Expand Up @@ -1051,17 +1061,21 @@ async function mountUploadComposer(
);
const scope = `https://relay.example.test:${viewer.pubkey}`;
const view = render(
<MessageComposer
session={owner.session}
scope={scope}
channelId="channel"
channelName="General"
{...(options.extensions ? { extensions: options.extensions } : {})}
{...(options.threadRootId ? { threadRootId: options.threadRootId } : {})}
{...(options.replyParentId
? { replyParentId: options.replyParentId }
: {})}
/>,
<AgentsContext.Provider value={options.agents}>
<MessageComposer
session={owner.session}
scope={scope}
channelId="channel"
channelName="General"
{...(options.extensions ? { extensions: options.extensions } : {})}
{...(options.threadRootId
? { threadRootId: options.threadRootId }
: {})}
{...(options.replyParentId
? { replyParentId: options.replyParentId }
: {})}
/>
</AgentsContext.Provider>,
{ reactStrictMode: true, wrapper: ToastProvider },
);
const input = () => within(view.container).getByRole("textbox");
Expand Down Expand Up @@ -1211,6 +1225,128 @@ it("restores a failed background upload into the composer with Desktop's toast",
);
});

// Remounting after the failure covers the usual leave-and-return case: the
// recovery lands with no mounted listener and must wait for the composer.
it.each(["never", "before the failure", "after the failure"])(
"restores the model and effort picks of a failed background upload (remounted: %s)",
async (remounted) => {
// Snapshots are stable, as the service's are.
const snapshot = {
status: "ready",
agents: [{ pubkey: first.pubkey, name: "Honey", type: "test/agent" }],
};
const models = {
current: "fast",
models: [
{
id: "fast",
label: "Fast",
efforts: [
{ id: "low", label: "Low" },
{ id: "high", label: "High" },
],
},
],
};
const types = [{ key: "test/agent", models: () => models }];
const agents = {
subscribe: () => () => {},
snapshot: () => snapshot,
types: () => types,
} as unknown as Agents;
const tools: readonly Contribution<ComposerTool>[] = [
{
id: "fixture",
key: "test/fixture",
pluginId: "test",
revision: "1",
title: "Fixture tools",
component: (props: ComposerToolProps) => (
<button type="button" onClick={() => props.insertMention(first)}>
First Honey
</button>
),
},
];
const extensions = {
tools: { snapshot: () => tools, subscribe: () => () => {} },
inline: { snapshot: () => [], subscribe: () => () => {} },
};
const h = await mountUploadComposer({
agents,
members: [first.pubkey],
extensions,
});
const picker = () =>
screen.findByRole("button", {
name: "Model and effort for the next run",
});
attachByPaste(h.input(), attachmentFile("tuned.txt"));
await userEvent.type(h.input(), "think hard");
await userEvent.click(screen.getByRole("button", { name: "First Honey" }));
await userEvent.click(await picker());
const slider = await screen.findByLabelText("Effort for Honey's next run");
slider.focus();
await userEvent.keyboard("{ArrowRight}");
expect(slider).toHaveAttribute("aria-valuetext", "High");
await userEvent.keyboard("{Escape}");
fireEvent.click(h.send());
await waitFor(() => expect(h.uploadCalls).toHaveLength(1));

let container = h.container;
if (remounted !== "never") h.unmount();
const remount = () => {
container = render(
<AgentsContext.Provider value={agents}>
<MessageComposer
session={h.owner.session}
scope={h.scope}
channelId="channel"
channelName="General"
extensions={extensions}
/>
</AgentsContext.Provider>,
{ wrapper: ToastProvider },
).container;
};
if (remounted === "before the failure") remount();
await act(async () => {
h.uploadCalls[0]?.result.reject(new Error("offline"));
});
if (remounted === "after the failure") remount();
const input = () => within(container).getByRole("textbox");
await waitFor(() => expect(input()).toHaveValue("think hard@Honey "));
expect(within(container).getByText("tuned.txt")).toBeVisible();
// The restored draft brings its pick back with it, not the default run.
const button = await picker();
expect(button).toHaveAttribute("data-selected");
await userEvent.hover(button);
expect(await screen.findByRole("tooltip")).toHaveTextContent(
"Next run: High",
);
await userEvent.unhover(button);
expect(h.publish).not.toHaveBeenCalled();

await userEvent.click(
screen.getByRole("button", { name: "Retry tuned.txt" }),
);
fireEvent.click(
within(container).getByRole("button", { name: "Send message" }),
);
await waitFor(() => expect(h.uploadCalls).toHaveLength(2));
await act(async () => {
h.uploadCalls[1]?.result.resolve(uploadDescriptor("tuned.txt"));
});
await waitFor(() => expect(h.publish).toHaveBeenCalledTimes(1));
expect(h.publish.mock.calls[0]?.[0].tags).toEqual(
expect.arrayContaining([
["p", first.pubkey],
["effort", first.pubkey, "high"],
]),
);
},
);

it.each(["cleanup", "restore"] as const)(
"retains a failed attachment send until %s storage recovers",
async (phase) => {
Expand Down
125 changes: 73 additions & 52 deletions src/features/messages/MessageComposer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,19 +116,21 @@ type AcceptedDraft = {
// Failed post-acceptance cleanup survives composer remounts within this session.
// This is recovery evidence only, not another persistent draft inventory.
const acceptedDrafts = new WeakMap<RelaySession, Map<string, AcceptedDraft>>();
type RecoveredSend = { draft: MentionDraft; preparationError?: unknown };
type RecoveredSend = {
draft: MentionDraft;
picks?: MentionPicks;
preparationError?: unknown;
};
const recoveryListeners = new WeakMap<
RelaySession,
Map<string, Set<(recovered: RecoveredSend) => void>>
>();
// Only undelivered preparation failures need replay. The saved draft and
// attachment store remain the owners of the actual recovered content.
const missedPreparation = new WeakMap<
RelaySession,
Map<string, RecoveredSend>
>();
function takeMissedPreparation(session: RelaySession, key: string) {
const byKey = missedPreparation.get(session);
// A recovery that no mounted composer consumed replays on remount. Only the
// unpersisted parts need it: the pick snapshot and a preparation failure. The
// saved draft and attachment store remain the owners of the recovered content.
const missedRecovery = new WeakMap<RelaySession, Map<string, RecoveredSend>>();
function takeMissedRecovery(session: RelaySession, key: string) {
const byKey = missedRecovery.get(session);
const recovered = byKey?.get(key);
byKey?.delete(key);
return recovered;
Expand All @@ -138,16 +140,14 @@ function notifyRecovered(
key: string,
recovered: RecoveredSend,
) {
const listeners = recoveryListeners.get(session)?.get(key);
if (recovered.preparationError !== undefined) {
let byKey = missedPreparation.get(session);
if (!byKey) {
byKey = new Map();
missedPreparation.set(session, byKey);
}
byKey.set(key, recovered);
let byKey = missedRecovery.get(session);
if (!byKey) {
byKey = new Map();
missedRecovery.set(session, byKey);
}
for (const listener of listeners ?? []) listener(recovered);
byKey.set(key, recovered);
for (const listener of recoveryListeners.get(session)?.get(key) ?? [])
listener(recovered);
}
function errorForRecovery(reason: unknown) {
return reason instanceof Error
Expand Down Expand Up @@ -605,55 +605,73 @@ function Composer({
setConflict(false);
setStorageFailed(false);
}
const reconcileDraft = useEffectEvent((restored?: MentionDraft) => {
if (submission || editing.target || writing.current) return;
if (accepted) {
// The recovered revision can precede attachment adoption. The explicit
// recovery notification below reconciles the mounted owner afterward.
if (recoveryFor(session).get(recoveryKey) === accepted) return;
// Picks belong to the recovered revision alone. They return whenever the
// composer holds it unedited, never to a later edit that keeps a conflict.
const reconcileDraft = useEffectEvent(
(restored?: MentionDraft, recoveredPicks?: MentionPicks) => {
if (submission || editing.target || writing.current) return;
if (accepted) {
// The recovered revision can precede attachment adoption. The explicit
// recovery notification below reconciles the mounted owner afterward.
if (recoveryFor(session).get(recoveryKey) === accepted) return;
const current = viewRevision(scope, draftKey);
if (current !== undefined) {
setAccepted(undefined);
loadSaved(current);
if (restored && current === JSON.stringify(restored))
setPicks(recoveredPicks ?? {});
}
return;
}
const current = viewRevision(scope, draftKey);
if (current !== undefined) {
setAccepted(undefined);
loadSaved(current);
if (current === undefined) return;
const recovering = !!restored && current === JSON.stringify(restored);
if (current === revision.current) {
// The storage listener usually loads the recovered revision before
// its notification arrives.
if (recovering && !dirty.current) setPicks(recoveredPicks ?? {});
return;
}
return;
}
const current = viewRevision(scope, draftKey);
if (current === undefined || current === revision.current) return;
// An explicit successful recovery may replace only the cleared follow-up,
// never a meaningful unsaved edit. Ordinary external revisions keep their
// conflict behavior.
if (
restored &&
current === JSON.stringify(restored) &&
!valueRef.current.text.trim() &&
!valueRef.current.recipients.length &&
!composerMarkdown(valueRef.current).trim() &&
!sendAttempt.current
)
loadSaved(current);
else if (dirty.current || sendAttempt.current) setConflict(true);
else loadSaved(current);
});
// An explicit successful recovery may replace only the cleared follow-up,
// never a meaningful unsaved edit. Ordinary external revisions keep their
// conflict behavior.
if (
recovering &&
!valueRef.current.text.trim() &&
!valueRef.current.recipients.length &&
!composerMarkdown(valueRef.current).trim() &&
!sendAttempt.current
)
loadSaved(current);
else if (dirty.current || sendAttempt.current) {
setConflict(true);
return;
} else loadSaved(current);
if (recovering) setPicks(recoveredPicks ?? {});
},
);
const editingDraft = !!editing.target;
const deferredRecovery = useRef<RecoveredSend | undefined>(undefined);
useEffect(() => {
if (submission) return;
const stop = subscribeView(scope, () => reconcileDraft());
// Reconcile changes while message-edit mode or mounting paused this listener.
reconcileDraft(deferredRecovery.current?.draft);
reconcileDraft(
deferredRecovery.current?.draft,
deferredRecovery.current?.picks,
);
if (!editingDraft && deferredRecovery.current) {
takeMissedPreparation(session, recoveryKey);
takeMissedRecovery(session, recoveryKey);
setError(errorForRecovery(deferredRecovery.current.preparationError));
deferredRecovery.current = undefined;
}
return stop;
}, [scope, submission, editingDraft, session, recoveryKey]);
const receiveRecovery = useEffectEvent((recovered: RecoveredSend) => {
if (editing.target) deferredRecovery.current = recovered;
reconcileDraft(recovered.draft);
reconcileDraft(recovered.draft, recovered.picks);
if (!editing.target) {
takeMissedPreparation(session, recoveryKey);
takeMissedRecovery(session, recoveryKey);
setError(errorForRecovery(recovered.preparationError));
}
});
Expand All @@ -664,11 +682,11 @@ function Composer({
// Recovery may have completed between unmount and this subscription. Replay
// only if its saved caption still owns the draft; never attach an old error
// to a replacement written while the composer was away.
const missed = missedPreparation.get(session)?.get(recoveryKey);
const missed = missedRecovery.get(session)?.get(recoveryKey);
if (missed) {
if (viewRevision(scope, draftKey) === JSON.stringify(missed.draft))
receiveRecovery(missed);
else takeMissedPreparation(session, recoveryKey);
else takeMissedRecovery(session, recoveryKey);
}
return stop;
}, [session, recoveryKey, scope, draftKey]);
Expand Down Expand Up @@ -1257,8 +1275,11 @@ function Composer({
return false;
if (!target.adopt(files)) return false;
recoveryFor(session).delete(recoveryKey);
// The whole captured pick set returns with the whole draft; the
// recipient filter above applies again on the next send.
notifyRecovered(session, recoveryKey, {
draft: captured,
picks: capturedPicks,

@kalvinnchau kalvinnchau Oct 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [P2] Retain recovered picks until an unmounted composer returns

If a user selects a model/effort, starts an attachment send, leaves the conversation, and the upload fails before they return, this picks snapshot reaches no mounted listener. notifyRecovered retains missed recovery only when preparationError is present, but ordinary upload failures and cancellation recover without it. On remount, the caption and attachment return while picks initialize empty, so retry falls back to defaults.

Moving the added test’s remount until after upload rejection reproduces the missing selection; its current remount-before-rejection ordering passes. Retain the pick snapshot with the unconsumed recovery record, replay only against the matching saved revision, and retire it on consumption/replacement. Add the unmount → reject → remount regression case.

preparationError,
});
return true;
Expand Down
Loading