Conversation
Combine the eight reviewed audit branches, retain their history, reconcile shared CI configuration and terminal test injection, and preserve v2.4.106 pane creation fixes.
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. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
React Doctor found 3 new issues in 2 files · 3 warnings · score 81 / 100 (Needs work) · 13 fixed · vs 3 warnings
Reviewed by React Doctor for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,29 @@ | |||
| import Editor, { loader } from '@monaco-editor/react'; | |||
There was a problem hiding this comment.
React Doctor · react-doctor/prefer-dynamic-import (warning)
"@monaco-editor/react" ships extra code to your users up front & slows page load. Load it on demand with React.lazy() or next/dynamic.
Fix → Load it only when needed: const Component = dynamic(() => import('library'), { ssr: false }) from next/dynamic, or React.lazy().
| @@ -0,0 +1,29 @@ | |||
| import Editor, { loader } from '@monaco-editor/react'; | |||
| import * as monaco from 'monaco-editor'; | |||
There was a problem hiding this comment.
React Doctor · react-doctor/prefer-dynamic-import (warning)
"monaco-editor" ships extra code to your users up front & slows page load. Load it on demand with React.lazy() or next/dynamic.
Fix → Load it only when needed: const Component = dynamic(() => import('library'), { ssr: false }) from next/dynamic, or React.lazy().
| const promptsResponse = await API.sessions.getPrompts(activeSession.id); | ||
| if (promptsResponse.success && promptsResponse.data?.length > 0) { | ||
| return promptsResponse.data.map((p: PromptMarker) => p.prompt_text).filter(Boolean).join('\n\n'); | ||
| return promptsResponse.data.map((p: { prompt_text: string }) => p.prompt_text).filter(Boolean).join('\n\n'); |
There was a problem hiding this comment.
React Doctor · react-doctor/js-flatmap-filter (warning)
This loops over your list twice because .map().filter(Boolean) makes two passes, so use .flatMap() to change & drop items in one pass
Fix → Use .flatMap(item => condition ? [value] : []) to change and drop items in one pass, instead of building a throwaway array in between
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6dea4531
ℹ️ 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".
| launchState.agentSessionId = agentSessionId; | ||
| launchState.hasClaudeSessionId = true; |
There was a problem hiding this comment.
When a fresh command exits before Claude creates its session—for example because the executable is missing or the arguments are rejected—these fields can still be persisted by initializeTerminal; the retained panel then has a generated agentSessionId and hasClaudeSessionId: true. The next sessions:continue therefore treats that nonexistent ID as resumeId in runSessionClaude and launches with --resume instead of retrying fresh, repeatedly preventing recovery. Mark the ID resumable only after successful startup, or clear this state on launch failure.
Useful? React with 👍 / 👎.
| const executable = config?.claudeExecutablePath | ||
| ?? (project?.wsl_enabled ? 'claude' : await this.getCliExecutablePath()); |
There was a problem hiding this comment.
For a WSL-enabled project on Windows, setting the application-wide Custom Claude executable through the file picker normally stores a Windows host path, but this expression selects that path before checking wsl_enabled. TerminalPanelManager then passes it to WSL's bash -lc, where the unconverted C:\... path is not executable, so every session-level Claude launch for that project fails. Give the WSL branch precedence so it resolves claude inside the distro, as the adjacent comment requires.
Useful? React with 👍 / 👎.
1 participant
The audit fixes were developed in parallel, and their shared CI configuration and terminal test infrastructure need to work together. This PR combines all eight source PRs into one reviewable tree against main
99791ac(v2.4.106):Integration preserves every source change and regression test. It combines the maintained browser test lists, keeps the shared remote-font removal, and retains both the contrast and packaged-editor CI gates. The new terminal lifecycle methods use the injected panel manager introduced by #637, and their tests use that same explicit fake. The Windows launch assertion from #647 is retained. Upstream #643 remains intact: archived display names can be reused without sharing worktrees, concurrent creation reserves names through persistence, and failures reach both desktop and remote views.
The existing dependency pins, frozen lockfile, pnpm 10.19 build allowlist, and bundled editor/analytics boundaries remain in effect. The reviewed cross-platform attribution inventory is preserved with the audited lockfile changes. No additional dependency is introduced by integration.
Local validation on the final tree (
1dc1b86), including current main:Broader validation on the combined audit changes before the current-main refresh also passed all 252 frontend unit tests and browser-computed contrast for every maintained gated theme. The full main-process run had 1,001 passing tests, including all affected lifecycle, persistence, queue, and MCP cases; fifteen existing Unix-socket tests hit a local
EPERMrestriction, independently reproduced with a plain Node listener, and one existing DNS fallback test timed out locally. The full platform matrix remains enabled in CI.CI passed on exact head
2b6dea4531100419088353b5568af21649f85297: all 10 jobs in Code Quality and React Doctor succeeded. This includes lint, typecheck, full main/frontend unit suites, production frontend and worker build, browser-computed contrast, sandboxed Electron preload, the packaged offline Electron editor, all maintained browser journeys, Windows/macOS main-process tests, and all six wrapper platform/runtime combinations. All 13 reported check runs are green. The source PRs remain available for focused review.