Conversation
The MentionPopover shared async state between @ (file scan) and / (slash commands) modes, causing race conditions: - Stale file items displayed when switching to slash command mode because items state was never cleared between mode switches - Long-running scanFilesFromRoot() could complete after switching to slash command mode, overwriting command results with files - No loading indicator shown while fetching slash commands Fix by: - Clearing items immediately when loading new data (setItems([])) - Adding cancellation flag to prevent stale async results from overwriting current data - Inlining the file scan into the unified useEffect with proper cancellation support - Showing context-appropriate loading/empty messages for each mode Signed-off-by: Douwe Osinga <douwe@squareup.com>
…isting Vite's dependency pre-bundling was inlining a second copy of React into the react-intl bundle. This happened because react-intl is hoisted to the workspace root (ui/node_modules/react-intl/), and its `import * as React from 'react'` resolved to ui/node_modules/react/ instead of ui/desktop/node_modules/react/. Two React instances at runtime causes the 'Invalid hook call' error in IntlProvider. Fix by adding resolve.alias entries in the Vite config that pin react and react-dom to the desktop app's own node_modules copies, ensuring a single React instance across all pre-bundled dependencies. Signed-off-by: Douwe Osinga <douwe@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2094047d06
ℹ️ 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".
| loadingCommands: { | ||
| id: 'mentionPopover.loadingCommands', | ||
| defaultMessage: 'Loading commands...', | ||
| }, |
There was a problem hiding this comment.
Sync i18n catalog after adding new message IDs
This commit introduces new defineMessages IDs (mentionPopover.loadingCommands here and mentionPopover.noCommandsFound below) but does not update ui/desktop/src/i18n/messages/en.json, so pnpm run i18n:check will fail because it diffs extracted messages against the committed catalog (ui/desktop/scripts/i18n-check.js), and CI runs that via pnpm run lint:check in .github/workflows/ci.yml. As written, this blocks the UI lint job and merge.
Useful? React with 👍 / 👎.
…space hoisting" This reverts commit 2094047.
jamadeo
left a comment
There was a problem hiding this comment.
out of scope here but we should do this in the backend and use ACP!
try re-creating node_modules before merging the vite config change
* origin/main: (32 commits) docs: rework homepage and add aaif migration blog post (#8356) chore(aaif): rename a bunch of repository references (#8152) fix: use OPENAI_API_KEY secret for recipe security scanner (#8358) feat: configurable extension timeouts via ACP _meta and global default (#8295) fix: hide hidden extensions in UI (#8346) refactor: skills as its own platform ext (#8244) fix baseUrl (#8347) Fix desktop slash commands (#8341) fix(cli): display platform-correct secrets path in keyring config dialog (#8328) feat(acp): add reusable ACP provider controls (#8314) fix: resolve MDX compilation error in using-goosehints.md (#8332) fix: use v1beta1 API version for Google/MaaS models on GCP Vertex AI (#8278) docs: add MCP Roots guide (#8252) rust acp client for extension methods (#8227) fix: reconsolidate split tool-call messages to follow OpenAI format (#7921) fix: clean up MCP subprocesses after abrupt parent exit (#8242) build: raise default stack reserve to 8 MB (#8234) fix(config): honour GOOSE_DISABLE_KEYRING from config.yaml at startup (#8219) feat: add configurable fast_model for declarative providers (#8194) fix(authentication): Allow connecting to Oauth servers that use protected-resource fallback instead of the WWW-authenticate header (#8148) ...
Summary
Race condition that could make / commands show the results of the @ short cut if used first