Repository navigation
Conversation
The project page's Details section gains a Templates row that shows how many templates the project allows and opens a dialog to edit them and their defaults. MultiSelectCombobox now shows a value passed on its first render.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27692b6332
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| </DialogHeader> | ||
| <ChatProjectTemplatesFields | ||
| templates={eligibleTemplates} | ||
| templatesError={templatesQuery.error} |
There was a problem hiding this comment.
Surface cached-query refetch failures
When the templates query has cached data and its mount refetch fails, React Query exposes both data and error; this passes both values onward, but ChatProjectTemplatesFields takes its templates branch and never renders templatesError. The editor therefore lets users save against stale eligibility without telling them the refresh failed. Keep the cached controls, but render the refetch error alongside them.
AGENTS.md reference: site/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| onSubmit={(event) => { | ||
| event.preventDefault(); | ||
| if (canSave) { | ||
| onSave(chatProjectTemplatesRequest(value, eligibleTemplates)); |
There was a problem hiding this comment.
Block repeated template submissions synchronously
If the user activates Save twice before isSaving reaches this component, canSave remains true for both submit events and two update requests are issued. The instructions editor in the same parent uses an in-flight ref specifically to close this render-delay window; use the same guard or the established Formik submitting lifecycle here so a rapid repeat cannot leave a second mutation running after the dialog closes.
AGENTS.md reference: site/AGENTS.md:L57-L58
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b5c4fbb: the save goes through the panel's existing in-flight ref (runMutation), with a double-click test.
| chatProjectTemplatesRequest, | ||
| chatProjectTemplatesValues, | ||
| templatesValidationDetail, | ||
| } from "../ChatsSidebar/dialogs/ChatProjectTemplatesFields"; |
There was a problem hiding this comment.
Move the shared fields out of ChatsSidebar
ProjectTemplatesDialog now makes project-page code depend on a reusable form module hidden under the unrelated ChatsSidebar/dialogs container. Move ChatProjectTemplatesFields and its helpers to a shared AgentsPage component module, then import that module from both dialogs so feature boundaries remain coherent.
AGENTS.md reference: site/AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping it where it is for now: ProjectHeaderSection already imports ChatProjectDialog from ChatsSidebar/dialogs, and both dialogs share this module with it. Moving the project dialogs together is a separate refactor.
| const dockerTemplate: Template = { | ||
| ...MockTemplate, | ||
| id: "template-docker", | ||
| name: "docker", | ||
| display_name: "Docker", |
There was a problem hiding this comment.
Share and consistently name the template fixtures
These unprefixed entity fixtures are repeated again in ProjectDetailsPanel.test.tsx:477-488, so story and test data can drift independently. Move the common Docker and Kubernetes variants to shared Mock* fixtures, or at minimum use named mock* variants when a fixture truly must remain local.
AGENTS.md reference: site/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
These fixtures are only meaningful for these template tests (distinct IDs and active versions on top of MockTemplate), so they stay local, the same as in ChatProjectDialog's tests and stories. No shared Mock* variants exist for multiple agent templates yet.
| const templatesQuery = { | ||
| key: agentTemplates(MockChatProject.organization_id).queryKey, | ||
| data: [dockerTemplate, kubernetesTemplate], | ||
| }; |
There was a problem hiding this comment.
Wire each story query inline with an exported key
This shared pre-wired query object both hides each story's inputs and constructs full query options merely to extract their key. Export an agentTemplatesKey helper, import it here, and compose { key, data } inline in each story's parameters.queries.
AGENTS.md reference: site/AGENTS.md:L13-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b5c4fbb: each story wires its queries inline. agentTemplates(...).queryKey matches how ChatProjectDialog.stories.tsx already seeds this query, so I did not add a separate key helper.
| const handleOpenChange = (nextOpen: boolean) => { | ||
| if (!nextOpen && !formProps.isSaving) { | ||
| onOpenChange(false); | ||
| } |
There was a problem hiding this comment.
Test the dialog's close suppression behavior
This adds behavior where dismiss and Cancel close the dialog while idle but are intentionally ignored during a save, yet the added Vitest coverage only exercises saving template values. Add interaction tests that assert onOpenChange(false) for idle cancellation or dismissal and assert that it is not called while isSaving; a Saving story cannot verify these callback outcomes.
AGENTS.md reference: site/AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cancel and dismiss go through the same guard as ChatProjectDialog and ProjectInstructionsDialog, whose tests cover it ("does not close while saving"). I'd rather not duplicate that test for a third copy of the pattern.
| chatProjectTemplatesValues(project.templates), | ||
| ); | ||
| const [value, setValue] = useState(initialValue); | ||
| const templatesQuery = useQuery(agentTemplates(project.organization_id)); |
There was a problem hiding this comment.
Add a visual story for the pending query state
The new query-backed dialog initially renders a skeleton with Save disabled, but every added story seeds successful template data, so Pixel never captures this reachable loading branch. Add a story that leaves the templates query pending to cover the loading presentation.
AGENTS.md reference: site/AGENTS.md:L71-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added a Loading story in b5c4fbb (templates request left pending).
| /** The number of templates the project's chats may use. */ | ||
| readonly templateCount: number; | ||
| /** Opens the editor for the project's templates. */ | ||
| readonly onEditTemplates: () => void; |
There was a problem hiding this comment.
Remove comments that only restate the props
The comments on templateCount and onEditTemplates only paraphrase the identifiers and their types, without documenting a constraint or non-obvious behavior. Remove them so the prop contract contains only substantive documentation.
AGENTS.md reference: site/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
… templates dialog stories
…ates update per save and cover the loading state
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5c4fbbde0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <DialogHeader> | ||
| <DialogTitle>Edit templates</DialogTitle> | ||
| </DialogHeader> | ||
| <ChatProjectTemplatesFields |
There was a problem hiding this comment.
Surface nested default refetch failures
When this dialog opens with a saved default and the presets or rich-parameters query already has cached data, a failed mount refetch leaves both data and error. ChatProjectDefaultsFields at ChatProjectTemplatesFields.tsx:283 and :318 selects the data branch and never renders that error, while Save remains enabled, so users can submit stale preset or parameter defaults without knowing the refresh failed. Keep the cached controls, but render each nested query error alongside them.
AGENTS.md reference: site/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
Moves editing a chat project's templates out of the Edit project dialog and into the project page's Details section.
templates, so the name, description, and icon are untouched, and a refetch of the project while the dialog is open keeps the draft.MultiSelectComboboxnow shows avaluepassed on its first render. Before, it only picked upvalueafter a later re-render, so the editor opened with an empty templates field.Stack (merge top to bottom):