Conversation
There was a problem hiding this comment.
Pull request overview
Implements a “Saved Prompts” feature across the SolidJS app, enabling users to create reusable prompt templates, manage them in Settings, and quickly start new sessions by sending a saved prompt (welcome screen + /prompt slash command + quick-save from input).
Changes:
- Added a
SavedPromptsProvidercontext with localStorage-backed CRUD operations. - Extended the Session page with saved-prompt cards on the welcome screen, a
/promptpicker, and an input “save as prompt” flow. - Added a new “Prompts” Settings tab with prompt add/edit/delete UI.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
app-prefixable/src/app.tsx |
Mounts SavedPromptsProvider at the app root so prompts are globally available. |
app-prefixable/src/context/saved-prompts.tsx |
Introduces saved prompt data model + localStorage persistence + CRUD/reorder API. |
app-prefixable/src/pages/session.tsx |
Adds welcome-screen prompt cards, /prompt picker command, and quick-save modal/toast. |
app-prefixable/src/pages/settings.tsx |
Adds “Prompts” tab with prompt list and add/edit/delete modal flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| id: "prompt.pick", | ||
| title: "Send Saved Prompt", | ||
| description: "Send a saved prompt in a new session", | ||
| slash: "prompt", | ||
| onSelect: () => { | ||
| setShowPromptPicker(true); | ||
| }, |
There was a problem hiding this comment.
The /prompt command doesn’t currently support the “/prompt <search> filters by title” behavior from issue #29. handleInputChange() only recognizes slash commands when the whole input matches ^/(\S*)$ (no spaces), so users can’t type a query after the command name. Consider extending the slash parsing to allow an optional search term (e.g. /prompt triage) and using it to pre-seed/filter the picker items.
app-prefixable/src/pages/session.tsx
Outdated
| if (!directory) return; | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const id = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${id}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: id, | ||
| parts: [{ type: "text", text }], | ||
| agent: providers.selectedAgent || "build", | ||
| ...(providers.selectedModel ? { model: providers.selectedModel } : {}), | ||
| }); |
There was a problem hiding this comment.
sendSavedPrompt() performs client.session.create() and client.session.promptAsync() without any error handling or the same model/provider validation enforced in sendMessage() (e.g. requiring providers.selectedModel and checking that provider is connected). This can lead to silent failures and also bypasses the explicit-model guard the rest of the chat flow relies on. Consider reusing the same validation + try/catch/error reporting approach as sendMessage() for this flow.
| if (!directory) return; | |
| const res = await client.session.create({}); | |
| if (!res.data) return; | |
| const id = res.data.id; | |
| navigate(`/${dirSlug()}/session/${id}`); | |
| await client.session.promptAsync({ | |
| sessionID: id, | |
| parts: [{ type: "text", text }], | |
| agent: providers.selectedAgent || "build", | |
| ...(providers.selectedModel ? { model: providers.selectedModel } : {}), | |
| }); | |
| if (!directory) { | |
| console.error("Cannot send saved prompt: no directory selected."); | |
| return; | |
| } | |
| if (!providers.selectedModel) { | |
| console.error( | |
| "Cannot send saved prompt: no model selected. Please select a model first.", | |
| ); | |
| return; | |
| } | |
| try { | |
| const res = await client.session.create({}); | |
| if (!res.data) { | |
| console.error( | |
| "Failed to create session for saved prompt: response contained no data.", | |
| ); | |
| return; | |
| } | |
| const id = res.data.id; | |
| navigate(`/${dirSlug()}/session/${id}`); | |
| await client.session.promptAsync({ | |
| sessionID: id, | |
| parts: [{ type: "text", text }], | |
| agent: providers.selectedAgent || "build", | |
| model: providers.selectedModel, | |
| }); | |
| } catch (error) { | |
| console.error("Error while sending saved prompt:", error); | |
| } |
app-prefixable/src/pages/session.tsx
Outdated
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | ||
| if (!found) return; | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const sid = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${sid}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: sid, | ||
| parts: [{ type: "text", text: found.text }], | ||
| agent: providers.selectedAgent || "build", | ||
| ...(providers.selectedModel ? { model: providers.selectedModel } : {}), | ||
| }); |
There was a problem hiding this comment.
The /prompt picker onSelect handler calls async SDK methods without try/catch and without the same model/provider validation used by sendMessage() (selected model required, provider connected). If client.session.create()/promptAsync() rejects, this async handler will produce an unhandled promise rejection and the user won’t get actionable feedback. Consider applying the same validation + error reporting pattern as sendMessage() before creating/sending, and catching errors inside this handler.
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | |
| if (!found) return; | |
| const res = await client.session.create({}); | |
| if (!res.data) return; | |
| const sid = res.data.id; | |
| navigate(`/${dirSlug()}/session/${sid}`); | |
| await client.session.promptAsync({ | |
| sessionID: sid, | |
| parts: [{ type: "text", text: found.text }], | |
| agent: providers.selectedAgent || "build", | |
| ...(providers.selectedModel ? { model: providers.selectedModel } : {}), | |
| }); | |
| try { | |
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | |
| if (!found) return; | |
| const res = await client.session.create({}); | |
| if (!res.data) return; | |
| const sid = res.data.id; | |
| navigate(`/${dirSlug()}/session/${sid}`); | |
| await client.session.promptAsync({ | |
| sessionID: sid, | |
| parts: [{ type: "text", text: found.text }], | |
| agent: providers.selectedAgent || "build", | |
| ...(providers.selectedModel ? { model: providers.selectedModel } : {}), | |
| }); | |
| } catch (error) { | |
| console.error("Failed to send saved prompt from picker", error); | |
| } |
app-prefixable/src/pages/session.tsx
Outdated
| <Portal> | ||
| <div | ||
| class="fixed inset-0 z-[100] flex items-center justify-center" | ||
| style={{ background: "rgba(0,0,0,0.5)" }} | ||
| onClick={(e) => { | ||
| if (e.target === e.currentTarget) setShowSavePrompt(false); | ||
| }} | ||
| role="presentation" | ||
| > | ||
| <div | ||
| class="w-full max-w-sm rounded-lg shadow-xl overflow-hidden" | ||
| style={{ | ||
| background: "var(--background-base)", | ||
| border: "1px solid var(--border-base)", | ||
| }} | ||
| > |
There was a problem hiding this comment.
The “Save Prompt” modal is missing the accessibility/keyboard handling patterns used by other dialogs in the codebase (e.g. PickerDialog / ConfirmDialog): the dialog container has no role="dialog", aria-modal, or labeling, and there’s no Escape-to-close or focus management/trap. Consider aligning this modal with the existing dialog components/patterns so it’s accessible and consistent.
| <Show when={promptDialogOpen()}> | ||
| <Portal> | ||
| <div | ||
| class="fixed inset-0 z-[100] flex items-center justify-center" | ||
| style={{ background: "rgba(0,0,0,0.5)" }} | ||
| onClick={(e) => { | ||
| if (e.target === e.currentTarget) setPromptDialogOpen(false) | ||
| }} | ||
| role="presentation" | ||
| > | ||
| <div | ||
| class="w-full max-w-md rounded-lg shadow-xl overflow-hidden" | ||
| style={{ | ||
| background: "var(--background-base)", | ||
| border: "1px solid var(--border-base)", | ||
| }} | ||
| > | ||
| <div class="px-4 py-3" style={{ "border-bottom": "1px solid var(--border-base)" }}> | ||
| <h2 class="text-base font-medium" style={{ color: "var(--text-strong)" }}> | ||
| {editingPromptId() ? "Edit Prompt" : "Add Prompt"} | ||
| </h2> | ||
| </div> |
There was a problem hiding this comment.
The prompt Add/Edit modal doesn’t include the accessibility attributes/keyboard behavior that other dialogs implement (e.g. ConfirmDialog uses role="alertdialog", aria-modal, labeledby/describe-by, Escape handling, and focus trapping). Consider adding role="dialog", aria-modal, labeling, and Escape/focus handling (or reusing an existing dialog component) to keep the Settings UI accessible.
| <button | ||
| onClick={() => openEditPromptDialog(prompt.id)} | ||
| class="p-1.5 rounded transition-colors" | ||
| style={{ color: "var(--text-weak)" }} | ||
| onMouseEnter={(e) => { | ||
| e.currentTarget.style.background = "var(--surface-inset)" | ||
| e.currentTarget.style.color = "var(--text-strong)" | ||
| }} | ||
| onMouseLeave={(e) => { | ||
| e.currentTarget.style.background = "transparent" | ||
| e.currentTarget.style.color = "var(--text-weak)" | ||
| }} | ||
| title="Edit prompt" | ||
| > | ||
| <Pencil class="w-4 h-4" /> | ||
| </button> | ||
| <button | ||
| onClick={() => setPromptToDelete(prompt.id)} | ||
| class="p-1.5 rounded transition-colors opacity-50 hover:opacity-100" | ||
| style={{ color: "var(--icon-critical-base)" }} | ||
| title="Delete prompt" | ||
| > | ||
| <Trash2 class="w-4 h-4" /> |
There was a problem hiding this comment.
The icon-only Edit/Delete buttons rely on title for description. For screen readers, these should have explicit aria-label (or visible text) so the actions are announced correctly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| export function SavedPromptsProvider(props: ParentProps) { | ||
| const initial = loadFromStorage().sort((a, b) => b.createdAt - a.createdAt) |
There was a problem hiding this comment.
SavedPromptsProvider sorts prompts by createdAt on initialization, which will override any custom order persisted by reorder() after a page reload. If reorder() is meant to be durable, load from storage without re-sorting (or persist a separate order key and only default-sort when no stored order exists).
| const initial = loadFromStorage().sort((a, b) => b.createdAt - a.createdAt) | |
| const initial = loadFromStorage() |
app-prefixable/src/pages/session.tsx
Outdated
| // Detect slash command pattern: starts with / followed by command name (no spaces) | ||
| const slashMatch = value.match(/^\/(\S*)$/); | ||
| // Detect slash command pattern: /command or /command <search term> | ||
| const slashMatch = value.match(/^\/(\S+)(?:\s+(.*))?$/); |
There was a problem hiding this comment.
The updated slash-command regex no longer matches a bare "/" input (/^\/(\S+).../ requires at least one non-space char), so the slash-command popover won’t appear when the user types just "/". If the intended UX is to show the command list immediately (as before), allow an empty command name in the match (e.g., \S*) and keep slashQuery empty in that case.
| const slashMatch = value.match(/^\/(\S+)(?:\s+(.*))?$/); | |
| const slashMatch = value.match(/^\/(\S*)(?:\s+(.*))?$/); |
app-prefixable/src/pages/session.tsx
Outdated
| async function sendSavedPrompt(text: string) { | ||
| if (!directory) return; | ||
| if (!providers.selectedModel) { | ||
| console.error("[WelcomeScreen] No model selected, cannot send saved prompt"); | ||
| return; | ||
| } | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const id = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${id}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: id, | ||
| parts: [{ type: "text", text }], | ||
| agent: providers.selectedAgent || "build", | ||
| model: providers.selectedModel, | ||
| }); |
There was a problem hiding this comment.
sendSavedPrompt performs async network calls without error handling. If client.session.create or client.session.promptAsync rejects, this will surface as an unhandled promise rejection. Wrap the body in a try/catch (or ensure the SDK never throws) and consider surfacing a user-visible error state instead of only logging.
app-prefixable/src/pages/session.tsx
Outdated
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | ||
| if (!found) return; | ||
| if (!providers.selectedModel) { | ||
| console.error("[Session] No model selected, cannot send saved prompt"); | ||
| return; | ||
| } | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const sid = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${sid}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: sid, | ||
| parts: [{ type: "text", text: found.text }], | ||
| agent: providers.selectedAgent || "build", | ||
| model: providers.selectedModel, | ||
| }); |
There was a problem hiding this comment.
This PickerDialog onSelect handler is async, but PickerDialog does not await props.onSelect(...) when invoked (it calls and then immediately closes). Any thrown/rejected error from client.session.create/promptAsync will become an unhandled promise rejection. Handle errors inside this callback (e.g., try/catch) and optionally close the dialog only after successful navigation/send if that matters.
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | |
| if (!found) return; | |
| if (!providers.selectedModel) { | |
| console.error("[Session] No model selected, cannot send saved prompt"); | |
| return; | |
| } | |
| const res = await client.session.create({}); | |
| if (!res.data) return; | |
| const sid = res.data.id; | |
| navigate(`/${dirSlug()}/session/${sid}`); | |
| await client.session.promptAsync({ | |
| sessionID: sid, | |
| parts: [{ type: "text", text: found.text }], | |
| agent: providers.selectedAgent || "build", | |
| model: providers.selectedModel, | |
| }); | |
| try { | |
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | |
| if (!found) return; | |
| if (!providers.selectedModel) { | |
| console.error("[Session] No model selected, cannot send saved prompt"); | |
| return; | |
| } | |
| const res = await client.session.create({}); | |
| if (!res.data) return; | |
| const sid = res.data.id; | |
| navigate(`/${dirSlug()}/session/${sid}`); | |
| await client.session.promptAsync({ | |
| sessionID: sid, | |
| parts: [{ type: "text", text: found.text }], | |
| agent: providers.selectedAgent || "build", | |
| model: providers.selectedModel, | |
| }); | |
| } catch (error) { | |
| console.error("[Session] Failed to send saved prompt", error); | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
app-prefixable/src/pages/session.tsx:356
- The updated slash-command regex matches any input that starts with
/even when it contains spaces (e.g./usr/bin python). This will force the slash popover to appear and change Enter/Tab behavior for messages that are not intended as slash commands. Consider only enabling slash-command mode when the command prefix matches a known command (or when there are no spaces unless the command isprompt), and otherwise treat the input as normal text.
// Detect slash command pattern: /command or /command <search term>
const slashMatch = value.match(/^\/(\S*)(?:\s+(.*))?$/);
if (slashMatch) {
const query = slashMatch[1];
const search = slashMatch[2];
// If there's a search term after the command, auto-execute matching command
if (search !== undefined) {
const cmd = baseSlashCommands.find(
(c) => c.slash?.toLowerCase() === query.toLowerCase(),
);
if (cmd?.slash === "prompt") {
setInput("");
setShowSlashPopover(false);
setSlashQuery("");
setPromptPickerFilter(search);
setShowPromptPicker(true);
return;
}
}
setSlashQuery(query);
setShowSlashPopover(true);
setSlashIndex(0);
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| export function SavedPromptsProvider(props: ParentProps) { | ||
| const stored = loadFromStorage() | ||
| const initial = stored.length > 0 ? stored : [] |
There was a problem hiding this comment.
SavedPromptsProvider doesn't ensure the loaded prompts are sorted by createdAt descending (newest first). Since storage order isn't guaranteed, prompts() can return an arbitrary order after reload. Sort the loaded array (or the initial signal value) by createdAt desc before exposing it, while still allowing reorder() to override when explicitly called.
| const initial = stored.length > 0 ? stored : [] | |
| const initial = | |
| stored.length > 0 ? [...stored].sort((a, b) => b.createdAt - a.createdAt) : [] |
| <div | ||
| class="fixed inset-0 z-[100] flex items-center justify-center" | ||
| style={{ background: "rgba(0,0,0,0.5)" }} | ||
| onClick={(e) => { | ||
| if (e.target === e.currentTarget) setPromptDialogOpen(false) | ||
| }} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape") setPromptDialogOpen(false) | ||
| }} | ||
| role="presentation" |
There was a problem hiding this comment.
The prompt add/edit modal closes on Escape via an onKeyDown handler on the backdrop, but the backdrop div isn't focusable and typically won't receive keyboard events. This likely makes Escape-to-close unreliable. Align with ConfirmDialog/PickerDialog by attaching a keydown listener to document/window while the dialog is open (and ideally implement basic focus management/trapping).
app-prefixable/src/pages/session.tsx
Outdated
| <div | ||
| class="fixed inset-0 z-[100] flex items-center justify-center" | ||
| style={{ background: "rgba(0,0,0,0.5)" }} | ||
| onClick={(e) => { | ||
| if (e.target === e.currentTarget) setShowSavePrompt(false); | ||
| }} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape") setShowSavePrompt(false); | ||
| }} | ||
| role="presentation" | ||
| > |
There was a problem hiding this comment.
The "Save as Prompt" modal relies on onKeyDown on the backdrop to close on Escape, but the backdrop isn't focusable so it may never receive key events. Use the same pattern as PickerDialog/ConfirmDialog (global keydown listener while open) so Escape works consistently, and consider adding basic focus trapping since this is a modal.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
app-prefixable/src/pages/session.tsx:356
- The updated slash-command regex matches "/ " for all commands, so typing a normal message that starts with "/" and contains spaces will keep the slash popover active and make Enter/Tab select a command instead of sending the message. Consider restoring the old "no spaces" detection for general commands and handling only
/prompt <search>as a special-case (e.g., separate regex just for/prompt), so non-command messages aren’t intercepted.
// Detect slash command pattern: /command or /command <search term>
const slashMatch = value.match(/^\/(\S*)(?:\s+(.*))?$/);
if (slashMatch) {
const query = slashMatch[1];
const search = slashMatch[2];
// If there's a search term after the command, auto-execute matching command
if (search !== undefined) {
const cmd = baseSlashCommands.find(
(c) => c.slash?.toLowerCase() === query.toLowerCase(),
);
if (cmd?.slash === "prompt") {
setInput("");
setShowSlashPopover(false);
setSlashQuery("");
setPromptPickerFilter(search);
setShowPromptPicker(true);
return;
}
}
setSlashQuery(query);
setShowSlashPopover(true);
setSlashIndex(0);
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div | ||
| role="dialog" | ||
| aria-modal="true" | ||
| class="w-full max-w-md rounded-lg shadow-xl overflow-hidden" | ||
| style={{ | ||
| background: "var(--background-base)", | ||
| border: "1px solid var(--border-base)", | ||
| }} | ||
| > | ||
| <div class="px-4 py-3" style={{ "border-bottom": "1px solid var(--border-base)" }}> | ||
| <h2 class="text-base font-medium" style={{ color: "var(--text-strong)" }}> | ||
| {props.editing ? "Edit Prompt" : "Add Prompt"} | ||
| </h2> |
There was a problem hiding this comment.
PromptDialog sets role="dialog"/aria-modal, but it doesn’t associate the dialog with a label via aria-labelledby (or aria-label). For screen readers, consider adding an id to the title element and wiring it up with aria-labelledby on the dialog container (matching the pattern used in PickerDialog).
| <div | ||
| role="dialog" | ||
| aria-modal="true" | ||
| class="w-full max-w-sm rounded-lg shadow-xl overflow-hidden" | ||
| style={{ | ||
| background: "var(--background-base)", | ||
| border: "1px solid var(--border-base)", | ||
| }} | ||
| > | ||
| <div | ||
| class="px-4 py-3" | ||
| style={{ | ||
| "border-bottom": "1px solid var(--border-base)", | ||
| }} | ||
| > | ||
| <h2 | ||
| class="text-base font-medium" | ||
| style={{ color: "var(--text-strong)" }} | ||
| > | ||
| Save as Prompt | ||
| </h2> |
There was a problem hiding this comment.
SavePromptDialog uses role="dialog"/aria-modal but doesn’t provide an accessible name (no aria-labelledby/aria-label). Consider adding an id to the “Save as Prompt” heading and referencing it from the dialog via aria-labelledby, consistent with PickerDialog’s accessibility wiring.
app-prefixable/src/pages/session.tsx
Outdated
| async function sendSavedPrompt(text: string) { | ||
| if (!directory) return; | ||
| if (!providers.selectedModel) { | ||
| console.error("[WelcomeScreen] No model selected, cannot send saved prompt"); | ||
| return; | ||
| } | ||
| try { | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const id = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${id}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: id, | ||
| parts: [{ type: "text", text }], | ||
| agent: providers.selectedAgent || "build", | ||
| model: providers.selectedModel, | ||
| }); |
There was a problem hiding this comment.
sendSavedPrompt only checks that a model is selected, but it doesn’t enforce the same provider-connected validation (or user-facing error handling) as sendMessage(). This can fail silently (console-only) when the chosen provider isn’t connected. Consider reusing the same validation + setError(...) behavior used in sendMessage() before creating the session and calling promptAsync().
app-prefixable/src/pages/session.tsx
Outdated
| if (!providers.selectedModel) { | ||
| console.error("[Session] No model selected, cannot send saved prompt"); | ||
| return; | ||
| } | ||
| try { | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const sid = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${sid}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: sid, | ||
| parts: [{ type: "text", text: found.text }], | ||
| agent: providers.selectedAgent || "build", | ||
| model: providers.selectedModel, | ||
| }); | ||
| } catch (err) { | ||
| console.error("[Session] Failed to send saved prompt:", err); | ||
| } |
There was a problem hiding this comment.
The /prompt picker send path checks for providers.selectedModel but (unlike sendMessage()) doesn’t verify the provider is connected and doesn’t surface failures via setError(...). This can lead to a confusing no-op where the dialog closes and the send fails with only a console error. Consider sharing the same validation/error reporting logic used by sendMessage() for consistency.
| <Show when={showSavePrompt()}> | ||
| <SavePromptDialog | ||
| title={savePromptTitle} | ||
| setTitle={setSavePromptTitle} | ||
| onSave={() => { | ||
| const title = savePromptTitle().trim(); | ||
| if (!title) return; | ||
| savedPrompts.add(title, input().trim()); | ||
| setShowSavePrompt(false); | ||
| setSavePromptSuccess(true); | ||
| setTimeout(() => setSavePromptSuccess(false), 2000); | ||
| }} | ||
| onClose={() => setShowSavePrompt(false)} | ||
| /> | ||
| </Show> | ||
|
|
||
| {/* Save Prompt Success Toast */} | ||
| <Show when={savePromptSuccess()}> | ||
| <div | ||
| class="fixed bottom-20 left-1/2 -translate-x-1/2 z-[100] px-4 py-2 rounded-lg shadow-lg text-sm font-medium" | ||
| style={{ | ||
| background: "var(--interactive-base)", | ||
| color: "white", | ||
| }} | ||
| > | ||
| Prompt saved | ||
| </div> | ||
| </Show> |
There was a problem hiding this comment.
The success toast uses setTimeout without cleanup. If the user navigates away/unmounts the Session quickly after saving, the timeout will still fire. Consider storing the timeout handle and clearing it in onCleanup (or using a Solid cleanup-aware timer helper) to avoid leaking timers across unmounts.
| <Show when={showSavePrompt()}> | |
| <SavePromptDialog | |
| title={savePromptTitle} | |
| setTitle={setSavePromptTitle} | |
| onSave={() => { | |
| const title = savePromptTitle().trim(); | |
| if (!title) return; | |
| savedPrompts.add(title, input().trim()); | |
| setShowSavePrompt(false); | |
| setSavePromptSuccess(true); | |
| setTimeout(() => setSavePromptSuccess(false), 2000); | |
| }} | |
| onClose={() => setShowSavePrompt(false)} | |
| /> | |
| </Show> | |
| {/* Save Prompt Success Toast */} | |
| <Show when={savePromptSuccess()}> | |
| <div | |
| class="fixed bottom-20 left-1/2 -translate-x-1/2 z-[100] px-4 py-2 rounded-lg shadow-lg text-sm font-medium" | |
| style={{ | |
| background: "var(--interactive-base)", | |
| color: "white", | |
| }} | |
| > | |
| Prompt saved | |
| </div> | |
| </Show> | |
| {(() => { | |
| let savePromptTimeout: number | undefined; | |
| onCleanup(() => { | |
| if (savePromptTimeout !== undefined) { | |
| clearTimeout(savePromptTimeout); | |
| } | |
| }); | |
| return ( | |
| <> | |
| <Show when={showSavePrompt()}> | |
| <SavePromptDialog | |
| title={savePromptTitle} | |
| setTitle={setSavePromptTitle} | |
| onSave={() => { | |
| const title = savePromptTitle().trim(); | |
| if (!title) return; | |
| savedPrompts.add(title, input().trim()); | |
| setShowSavePrompt(false); | |
| setSavePromptSuccess(true); | |
| if (savePromptTimeout !== undefined) { | |
| clearTimeout(savePromptTimeout); | |
| } | |
| savePromptTimeout = window.setTimeout( | |
| () => setSavePromptSuccess(false), | |
| 2000, | |
| ); | |
| }} | |
| onClose={() => setShowSavePrompt(false)} | |
| /> | |
| </Show> | |
| {/* Save Prompt Success Toast */} | |
| <Show when={savePromptSuccess()}> | |
| <div | |
| class="fixed bottom-20 left-1/2 -translate-x-1/2 z-[100] px-4 py-2 rounded-lg shadow-lg text-sm font-medium" | |
| style={{ | |
| background: "var(--interactive-base)", | |
| color: "white", | |
| }} | |
| > | |
| Prompt saved | |
| </div> | |
| </Show> | |
| </> | |
| ); | |
| })()} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app-prefixable/src/pages/session.tsx:361
- The updated slash-command regex (
/^\/(\S*)(?:\s+(.*))?$/) will keep the slash popover open even after the user types a space for any command (e.g./new hello), which is a behavior change from the previous^/(\S*)$and can interfere with normal message typing that happens to start with/. Consider limiting the “allow space + args” path to/promptonly (e.g., special-case/prompt\s+...), and keep the stricter regex for all other commands so the popover closes once a space is entered.
// Detect slash command pattern: /command or /command <search term>
const slashMatch = value.match(/^\/(\S*)(?:\s+(.*))?$/);
if (slashMatch) {
const query = slashMatch[1];
const search = slashMatch[2];
// If there's a search term after the command, auto-execute matching command
if (search !== undefined) {
const cmd = baseSlashCommands.find(
(c) => c.slash?.toLowerCase() === query.toLowerCase(),
);
if (cmd?.slash === "prompt") {
setInput("");
setShowSlashPopover(false);
setSlashQuery("");
setPromptPickerFilter(search);
setShowPromptPicker(true);
return;
}
}
setSlashQuery(query);
setShowSlashPopover(true);
setSlashIndex(0);
} else {
setShowSlashPopover(false);
setSlashQuery("");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app-prefixable/src/pages/session.tsx
Outdated
| onSelect={async (item) => { | ||
| const found = savedPrompts.prompts().find((p) => p.id === item.id); | ||
| if (!found) return; | ||
| if (!providers.selectedModel) { | ||
| setError("Please select a model before sending messages. Click the model button in the header."); | ||
| return; | ||
| } | ||
| if (!providers.connected.includes(providers.selectedModel.providerID)) { | ||
| setError(`Provider "${providers.selectedModel.providerID}" is not connected. Please configure it in Settings.`); | ||
| return; | ||
| } | ||
| try { | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const sid = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${sid}`); | ||
| await client.session.promptAsync({ | ||
| sessionID: sid, | ||
| parts: [{ type: "text", text: found.text }], | ||
| agent: providers.selectedAgent || "build", | ||
| model: providers.selectedModel, | ||
| }); | ||
| } catch (err) { | ||
| setError(`Failed to send saved prompt: ${err instanceof Error ? err.message : String(err)}`); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The saved-prompt send flow is duplicated (model/provider validation + client.session.create + promptAsync) in multiple new places (sendSavedPrompt in WelcomeScreen and again in the picker onSelect). This duplication increases the risk of future divergence (e.g., if the send flow changes). Consider extracting a shared helper (e.g. createSessionAndSendPrompt(text)) and reusing it from both call sites (and potentially aligning it with the existing sendMessage creation path).
| onMount(() => { | ||
| const handler = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| e.preventDefault() | ||
| props.onClose() | ||
| } | ||
| } | ||
| document.addEventListener("keydown", handler) | ||
| onCleanup(() => document.removeEventListener("keydown", handler)) | ||
| }) | ||
|
|
||
| return ( | ||
| <Portal> | ||
| <div | ||
| class="fixed inset-0 z-[100] flex items-center justify-center" | ||
| style={{ background: "rgba(0,0,0,0.5)" }} | ||
| onClick={(e) => { | ||
| if (e.target === e.currentTarget) props.onClose() | ||
| }} | ||
| role="presentation" | ||
| > | ||
| <div | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="prompt-dialog-title" | ||
| class="w-full max-w-md rounded-lg shadow-xl overflow-hidden" | ||
| style={{ |
There was a problem hiding this comment.
PromptDialog behaves like a modal but doesn’t trap focus within the dialog. This allows keyboard users to tab to elements behind the overlay, which is inconsistent with the existing ConfirmDialog/PickerDialog patterns in the codebase. Please add focus trapping (cycle focus on Tab/Shift+Tab within the dialog’s focusable elements) and ensure initial focus is set deterministically (without relying on setTimeout).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app-prefixable/src/pages/session.tsx
Outdated
| } | ||
|
|
||
| async function createSessionAndSendPrompt(text: string) { | ||
| if (!directory) return; |
There was a problem hiding this comment.
createSessionAndSendPrompt returns early when directory is falsy, but the rest of this file frequently falls back to base64Decode(params.dir) when directory isn't available yet. This can make saved prompts (welcome cards + /prompt) do nothing in cases where directory is undefined. Consider removing this guard (or deriving the directory from params only where needed) so the feature works consistently.
| if (!directory) return; |
app-prefixable/src/pages/session.tsx
Outdated
| // Detect slash command pattern: /command or /command <search term> | ||
| const slashMatch = value.match(/^\/(\S*)(?:\s+(.*))?$/); | ||
| if (slashMatch) { | ||
| const query = slashMatch[1]; | ||
| const search = slashMatch[2]; | ||
| // If there's a search term after the command, auto-execute matching command | ||
| if (search !== undefined) { | ||
| const cmd = baseSlashCommands.find( | ||
| (c) => c.slash?.toLowerCase() === query.toLowerCase(), | ||
| ); | ||
| if (cmd?.slash === "prompt") { | ||
| setInput(""); | ||
| setShowSlashPopover(false); | ||
| setSlashQuery(""); | ||
| setPromptPickerFilter(search); | ||
| setShowPromptPicker(true); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The updated slash-command regex keeps the slash popover active even after the user types a space (e.g. /settings please ...). Because the popover stays open, pressing Enter/Tab can execute a command instead of sending the typed message. Suggestion: only keep the slash popover open for the no-space form (/^\/(\S*)$/), and separately special-case /prompt <search> to open the prompt picker with a filter.
| // Detect slash command pattern: /command or /command <search term> | |
| const slashMatch = value.match(/^\/(\S*)(?:\s+(.*))?$/); | |
| if (slashMatch) { | |
| const query = slashMatch[1]; | |
| const search = slashMatch[2]; | |
| // If there's a search term after the command, auto-execute matching command | |
| if (search !== undefined) { | |
| const cmd = baseSlashCommands.find( | |
| (c) => c.slash?.toLowerCase() === query.toLowerCase(), | |
| ); | |
| if (cmd?.slash === "prompt") { | |
| setInput(""); | |
| setShowSlashPopover(false); | |
| setSlashQuery(""); | |
| setPromptPickerFilter(search); | |
| setShowPromptPicker(true); | |
| return; | |
| } | |
| } | |
| // Special-case: `/prompt <search>` opens the prompt picker with a filter | |
| const promptMatch = value.match(/^\/prompt\s+(.+)$/i); | |
| if (promptMatch) { | |
| setInput(""); | |
| setShowSlashPopover(false); | |
| setSlashQuery(""); | |
| setPromptPickerFilter(promptMatch[1]); | |
| setShowPromptPicker(true); | |
| return; | |
| } | |
| // Keep slash popover active only for the no-space form: `/command` | |
| const slashMatch = value.match(/^\/(\S*)$/); | |
| if (slashMatch) { | |
| const query = slashMatch[1]; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app-prefixable/src/pages/session.tsx
Outdated
| setInput(""); | ||
| setShowSlashPopover(false); | ||
| setSlashQuery(""); | ||
| setPromptPickerFilter(promptMatch[1]); |
There was a problem hiding this comment.
The /prompt <search> handler sets promptPickerFilter(promptMatch[1]) without trimming. Trailing/leading spaces (e.g. "/prompt foo ") will make the picker filter include whitespace and can prevent matches. Trim the captured search string before storing it as the initial filter.
| setPromptPickerFilter(promptMatch[1]); | |
| setPromptPickerFilter(promptMatch[1].trim()); |
| onSave={() => { | ||
| const title = savePromptTitle().trim(); | ||
| if (!title) return; | ||
| savedPrompts.add(title, input().trim()); | ||
| setShowSavePrompt(false); | ||
| setSavePromptSuccess(true); | ||
| clearTimeout(toastTimer.id); | ||
| toastTimer.id = setTimeout(() => setSavePromptSuccess(false), 2000); | ||
| }} |
There was a problem hiding this comment.
The quick-save flow saves input().trim() at the moment the dialog’s Save button is pressed. If the underlying input changes while the dialog is open (or is cleared programmatically), this can save an empty/incorrect prompt body. Capture the prompt body when opening the dialog (store it in its own signal) and/or validate the body is still non-empty before calling savedPrompts.add(...).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function createSessionAndSendPrompt(text: string) { | ||
| if (!providers.selectedModel) { | ||
| setError("Please select a model before sending messages. Click the model button in the header."); | ||
| return; | ||
| } | ||
| if (!providers.connected.includes(providers.selectedModel.providerID)) { | ||
| setError(`Provider "${providers.selectedModel.providerID}" is not connected. Please configure it in Settings.`); | ||
| return; | ||
| } | ||
| try { | ||
| const res = await client.session.create({}); | ||
| if (!res.data) return; | ||
| const sid = res.data.id; | ||
| navigate(`/${dirSlug()}/session/${sid}`); | ||
| await client.session.promptAsync({ |
There was a problem hiding this comment.
createSessionAndSendPrompt() diverges from the existing “create new session + send” flow used in sendMessage(): it doesn’t clear prior errors (setError(null)), doesn’t set the local sessionId before navigating, and navigates without { replace: true } (which can leave the welcome screen in history). Consider extracting/reusing the same helper logic as sendMessage() so session state + navigation behavior stays consistent.
app-prefixable/src/pages/session.tsx
Outdated
| onMount(() => { | ||
| const handler = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| e.preventDefault(); | ||
| props.onClose(); | ||
| } | ||
| }; | ||
| document.addEventListener("keydown", handler); | ||
| onCleanup(() => document.removeEventListener("keydown", handler)); | ||
| }); |
There was a problem hiding this comment.
SavePromptDialog adds an Escape key handler but doesn’t implement a focus trap (unlike ConfirmDialog and PickerDialog). Keyboard users can tab into the background UI while the modal is open, which is an accessibility issue. Suggest trapping Tab focus within the dialog and ensuring initial focus is set without relying on a setTimeout ref callback.
…p to SavePromptDialog
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Complete saved prompts feature: context provider, welcome screen cards,
/promptslash command, settings management, and quick-save from input./promptslash command with picker dialog and filtering (feat: /prompt slash command to pick and send a saved prompt #29)Closes #27, closes #28, closes #29, closes #30