feat: add app commands configuration in Configure panel#2433
feat: add app commands configuration in Configure panel#2433wwwillchen merged 2 commits intomainfrom
Conversation
|
@BugBot run |
Summary of ChangesHello @wwwillchen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's flexibility by enabling users to configure custom install and start commands for their apps after creation, a capability previously limited to the initial import process. It provides a dedicated UI in the Configure panel for managing these commands, backed by new IPC communication and database persistence, ensuring a robust and user-friendly experience for tailoring application execution. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to configure application commands, but it has critical security implications, including a Command Injection vulnerability due to lack of input validation and potential sensitive information leakage through logging. The review also provides suggestions for code simplification, aligning with best practices for concise and readable code, and improvements to user-facing text and corresponding tests.
| createTypedHandler(appContracts.updateAppCommands, async (_, params) => { | ||
| const { appId, installCommand, startCommand } = params; | ||
|
|
||
| return withLock(appId, async () => { | ||
| const app = await db.query.apps.findFirst({ | ||
| where: eq(apps.id, appId), | ||
| }); | ||
|
|
||
| if (!app) { | ||
| throw new Error("App not found"); | ||
| } | ||
|
|
||
| await db | ||
| .update(apps) | ||
| .set({ | ||
| installCommand: installCommand?.trim() || null, | ||
| startCommand: startCommand?.trim() || null, | ||
| }) | ||
| .where(eq(apps.id, appId)); | ||
|
|
||
| logger.info( | ||
| `Updated commands for app ${appId}: install="${installCommand}", start="${startCommand}"`, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The updateAppCommands handler allows setting arbitrary strings for installCommand and startCommand, which are later executed using child_process.spawn with shell: true (line 218) or sh -c (line 525). This introduces a Critical Command Injection vulnerability. An attacker who can influence these commands (e.g., via a malicious project or prompt injection) can achieve arbitrary code execution on the host machine.
To mitigate this, implement strict validation for the command strings and consider adding a user confirmation step before executing custom commands, especially if they were not set by the user in the current session.
There was a problem hiding this comment.
🚩 Flagged for human review: This is a valid security concern that requires architectural decision-making.
While the concern about command injection is real, implementing strict validation for shell commands is complex because:
- Legitimate use cases may require complex bash syntax (pipes, conditionals, environment variable expansion)
- A whitelist approach could severely limit functionality
- The commands are set explicitly by the user through the UI, not from external/untrusted sources
Potential mitigations to consider:
- Add a confirmation dialog when custom commands are first executed
- Display a security warning in the UI when configuring custom commands
- Consider alternative approaches like structured command configuration instead of free-form strings
- Add input sanitization while still allowing legitimate shell constructs
This needs discussion with the team to balance security and usability.
| export const UpdateAppCommandsParamsSchema = z.object({ | ||
| appId: z.number(), | ||
| installCommand: z.string().nullable(), | ||
| startCommand: z.string().nullable(), | ||
| }); |
There was a problem hiding this comment.
The UpdateAppCommandsParamsSchema lacks validation for the installCommand and startCommand fields. Since these strings are eventually executed in a shell, they should be validated against a whitelist of allowed characters or patterns to prevent command injection.
There was a problem hiding this comment.
This has been addressed by adding server-side validation in the handler itself. The handler now enforces that both commands must be provided together or both must be null, which provides stronger validation than schema-level constraints while still maintaining the flexibility to accept nullable strings.
![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: 3a893e83cf
ℹ️ 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".
🔍 Multi-Agent Code ReviewReviewed by 3 independent agents with different file orderings and focus areas. Summary
Issues to Address
🟢 Low Priority Issues (2 items)
See inline comments for details. Generated by multi-agent consensus review (3 agents, 2+ agreement threshold) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Greptile OverviewGreptile SummaryThis PR adds the ability to configure custom install and start commands for apps through the Configure panel. The implementation follows repository patterns for IPC contracts, TanStack Query integration, and E2E testing. Key Changes:
Implementation Quality:
Security Note: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ConfigurePanel
participant TanStack Query
participant IPC Client
participant Main Process
participant Database
User->>ConfigurePanel: Click "Configure Custom Commands"
ConfigurePanel->>ConfigurePanel: setIsEditing(true)
User->>ConfigurePanel: Enter install command
ConfigurePanel->>ConfigurePanel: setInstallCommand(value)
User->>ConfigurePanel: Enter start command
ConfigurePanel->>ConfigurePanel: setStartCommand(value)
ConfigurePanel->>ConfigurePanel: Validate both commands provided
User->>ConfigurePanel: Click "Save"
ConfigurePanel->>TanStack Query: mutate({ installCmd, startCmd })
TanStack Query->>IPC Client: updateAppCommands({ appId, installCommand, startCommand })
IPC Client->>Main Process: invoke("update-app-commands", params)
Main Process->>Main Process: withLock(appId)
Main Process->>Database: Check app exists
Database-->>Main Process: App found
Main Process->>Main Process: Validate both commands provided or both null
Main Process->>Database: UPDATE apps SET installCommand, startCommand
Database-->>Main Process: Success
Main Process-->>IPC Client: void
IPC Client-->>TanStack Query: Success
TanStack Query->>TanStack Query: Invalidate apps.detail query
TanStack Query->>ConfigurePanel: onSuccess callback
ConfigurePanel->>ConfigurePanel: showSuccess("App commands saved")
ConfigurePanel->>ConfigurePanel: setIsEditing(false)
ConfigurePanel->>User: Display updated commands
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| createTypedHandler(appContracts.updateAppCommands, async (_, params) => { | ||
| const { appId, installCommand, startCommand } = params; | ||
|
|
||
| return withLock(appId, async () => { | ||
| const app = await db.query.apps.findFirst({ | ||
| where: eq(apps.id, appId), | ||
| }); | ||
|
|
||
| if (!app) { | ||
| throw new Error("App not found"); | ||
| } | ||
|
|
||
| await db | ||
| .update(apps) | ||
| .set({ | ||
| installCommand: installCommand?.trim() || null, | ||
| startCommand: startCommand?.trim() || null, | ||
| }) | ||
| .where(eq(apps.id, appId)); | ||
|
|
||
| logger.info( | ||
| `Updated commands for app ${appId}: install="${installCommand}", start="${startCommand}"`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Missing server-side validation - both commands should be provided together or both should be null.
The client validates this, but the handler doesn't. If called directly (e.g., via testing or future refactoring), inconsistent state could occur where only one command is set.
| createTypedHandler(appContracts.updateAppCommands, async (_, params) => { | |
| const { appId, installCommand, startCommand } = params; | |
| return withLock(appId, async () => { | |
| const app = await db.query.apps.findFirst({ | |
| where: eq(apps.id, appId), | |
| }); | |
| if (!app) { | |
| throw new Error("App not found"); | |
| } | |
| await db | |
| .update(apps) | |
| .set({ | |
| installCommand: installCommand?.trim() || null, | |
| startCommand: startCommand?.trim() || null, | |
| }) | |
| .where(eq(apps.id, appId)); | |
| logger.info( | |
| `Updated commands for app ${appId}: install="${installCommand}", start="${startCommand}"`, | |
| ); | |
| }); | |
| createTypedHandler(appContracts.updateAppCommands, async (_, params) => { | |
| const { appId, installCommand, startCommand } = params; | |
| return withLock(appId, async () => { | |
| const app = await db.query.apps.findFirst({ | |
| where: eq(apps.id, appId), | |
| }); | |
| if (!app) { | |
| throw new Error("App not found"); | |
| } | |
| const trimmedInstall = installCommand?.trim() || null; | |
| const trimmedStart = startCommand?.trim() || null; | |
| // Both commands must be provided together, or both must be null | |
| if ((trimmedInstall === null) !== (trimmedStart === null)) { | |
| throw new Error("Both install and start commands are required when customizing"); | |
| } | |
| await db | |
| .update(apps) | |
| .set({ | |
| installCommand: trimmedInstall, | |
| startCommand: trimmedStart, | |
| }) | |
| .where(eq(apps.id, appId)); | |
| logger.info( | |
| `Updated commands for app ${appId}: install="${installCommand}", start="${startCommand}"`, | |
| ); | |
| }); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ipc/handlers/app_handlers.ts
Line: 1837:1860
Comment:
Missing server-side validation - both commands should be provided together or both should be null.
The client validates this, but the handler doesn't. If called directly (e.g., via testing or future refactoring), inconsistent state could occur where only one command is set.
```suggestion
createTypedHandler(appContracts.updateAppCommands, async (_, params) => {
const { appId, installCommand, startCommand } = params;
return withLock(appId, async () => {
const app = await db.query.apps.findFirst({
where: eq(apps.id, appId),
});
if (!app) {
throw new Error("App not found");
}
const trimmedInstall = installCommand?.trim() || null;
const trimmedStart = startCommand?.trim() || null;
// Both commands must be provided together, or both must be null
if ((trimmedInstall === null) !== (trimmedStart === null)) {
throw new Error("Both install and start commands are required when customizing");
}
await db
.update(apps)
.set({
installCommand: trimmedInstall,
startCommand: trimmedStart,
})
.where(eq(apps.id, appId));
logger.info(
`Updated commands for app ${appId}: install="${installCommand}", start="${startCommand}"`,
);
});
});
```
How can I resolve this? If you propose a fix, please make it concise.![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/ipc/handlers/app_handlers.ts">
<violation number="1" location="src/ipc/handlers/app_handlers.ts:1849">
P2: The handler allows saving only `installCommand` OR only `startCommand`, while the frontend validates that both must be set together. Add server-side validation to prevent inconsistent database state if the API is called directly:
```typescript
const trimmedInstall = installCommand?.trim() || null;
const trimmedStart = startCommand?.trim() || null;
if ((trimmedInstall === null) !== (trimmedStart === null)) {
throw new Error("Both install and start commands are required when customizing");
}
```</violation>
</file>
<file name="src/components/preview_panel/ConfigurePanel.tsx">
<violation number="1" location="src/components/preview_panel/ConfigurePanel.tsx:162">
P2: Using OR logic here is inconsistent with the validation that requires both commands to be set together. If the database somehow has only one command set, the UI will show the display view but could render `null`/`undefined` for the missing command. Change to AND logic to match the validation requirement.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
🎭 Playwright Test Results
|
| OS | Passed | Failed | Flaky | Skipped |
|---|---|---|---|---|
| 🍎 macOS | 152 | 8 | 5 | 0 |
Summary: 152 passed, 8 failed, 5 flaky
Failed Tests
🍎 macOS
problems.spec.ts > problems auto-fix - enabled- Error: expect(locator).toBeVisible() failed
problems.spec.ts > problems auto-fix - gives up after 2 attempts- Error: expect(locator).toBeVisible() failed
problems.spec.ts > problems auto-fix - complex delete-rename-write- Error: expect(locator).toBeVisible() failed
problems.spec.ts > problems - fix all- Error: expect(locator).toBeVisible() failed
problems.spec.ts > problems - select specific problems and fix- Error: expect(locator).toBeVisible() failed
refresh.spec.ts > refresh app- TimeoutError: locator.evaluate: Timeout 30000ms exceeded.
select_component.spec.ts > deselect individual component from multiple- Error: expect(locator).toMatchAriaSnapshot(expected) failed
select_component.spec.ts > upgrade app to select component- Error: expect(string).toMatchSnapshot(expected) failed
📋 Test Commands (macOS)
Copy and paste these commands to run or update snapshots for failed tests:
Show all 8 test commands
problems.spec.ts > problems auto-fix - enabled
# Run test
npm run e2e e2e-tests/problems.spec.ts -- -g "problems auto-fix - enabled"
# Update snapshots
npm run e2e e2e-tests/problems.spec.ts -- -g "problems auto-fix - enabled" --update-snapshotsproblems.spec.ts > problems auto-fix - gives up after 2 attempts
# Run test
npm run e2e e2e-tests/problems.spec.ts -- -g "problems auto-fix - gives up after 2 attempts"
# Update snapshots
npm run e2e e2e-tests/problems.spec.ts -- -g "problems auto-fix - gives up after 2 attempts" --update-snapshotsproblems.spec.ts > problems auto-fix - complex delete-rename-write
# Run test
npm run e2e e2e-tests/problems.spec.ts -- -g "problems auto-fix - complex delete-rename-write"
# Update snapshots
npm run e2e e2e-tests/problems.spec.ts -- -g "problems auto-fix - complex delete-rename-write" --update-snapshotsproblems.spec.ts > problems - fix all
# Run test
npm run e2e e2e-tests/problems.spec.ts -- -g "problems - fix all"
# Update snapshots
npm run e2e e2e-tests/problems.spec.ts -- -g "problems - fix all" --update-snapshotsproblems.spec.ts > problems - select specific problems and fix
# Run test
npm run e2e e2e-tests/problems.spec.ts -- -g "problems - select specific problems and fix"
# Update snapshots
npm run e2e e2e-tests/problems.spec.ts -- -g "problems - select specific problems and fix" --update-snapshotsrefresh.spec.ts > refresh app
# Run test
npm run e2e e2e-tests/refresh.spec.ts -- -g "refresh app"
# Update snapshots
npm run e2e e2e-tests/refresh.spec.ts -- -g "refresh app" --update-snapshotsselect_component.spec.ts > deselect individual component from multiple
# Run test
npm run e2e e2e-tests/select_component.spec.ts -- -g "deselect individual component from multiple"
# Update snapshots
npm run e2e e2e-tests/select_component.spec.ts -- -g "deselect individual component from multiple" --update-snapshotsselect_component.spec.ts > upgrade app to select component
# Run test
npm run e2e e2e-tests/select_component.spec.ts -- -g "upgrade app to select component"
# Update snapshots
npm run e2e e2e-tests/select_component.spec.ts -- -g "upgrade app to select component" --update-snapshots⚠️ Flaky Tests
🍎 macOS
problems.spec.ts > problems - manual edit (react/vite)(passed after 1 retry)select_component.spec.ts > select component(passed after 1 retry)setup.spec.ts > setup ai provider(passed after 1 retry)visual_editing.spec.ts > edit style of one selected component(passed after 2 retries)visual_editing.spec.ts > edit text of the selected component(passed after 1 retry)
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/preview_panel/ConfigurePanel.tsx">
<violation number="1" location="src/components/preview_panel/ConfigurePanel.tsx:71">
P2: Skipping command state sync while `isEditing` allows stale commands to persist when the selected app changes, so saving can overwrite the wrong app’s commands.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@BugBot run |
|
@BugBot run |
|
@BugBot run |
Add the ability to configure install and start commands in the Configure panel after an app has been created. Previously these were only configurable when first importing an app. Changes: - Add updateAppCommands IPC contract and handler - Add AppCommandsSection component in ConfigurePanel - Allow users to view, edit, and clear custom commands - Include validation requiring both commands when customizing - Add E2E tests for the new feature https://claude.ai/code/session_01RHmPUNG7Bdw9MC1DKQJX8g
- Fix useEffect to not overwrite edits during query refetch by adding !isEditing guard - Update UI text to generic "Using default install and start commands" message - Add server-side validation requiring both commands together or both null - Remove sensitive command content from logs - Remove redundant validation in handleSave (button already disabled) - Remove unnecessary state updates in handleClear - Fix hasCustomCommands to use AND logic instead of OR - Update E2E tests to match new UI text Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
wwwillchen
force-pushed
the
claude/configure-app-commands-vIVj0
branch
from
1d11231 to
3f0abe3
Compare
|
@BugBot run |
2 participants
Add the ability to configure install and start commands in the Configure panel after an app has been created. Previously these were only configurable when first importing an app.
Changes:
https://claude.ai/code/session_01RHmPUNG7Bdw9MC1DKQJX8g
Note
Medium Risk
Moderate risk because it introduces a new IPC write path that persists user-provided command strings and affects how apps will run, though guarded by client/server validation and locking.
Overview
Adds an App Commands card to
ConfigurePanelthat lets users view default vs custom install/start commands, edit them with client-side validation (both required), cancel edits, and clear back to defaults.Introduces a new IPC contract
updateAppCommandsplus a main-process handler that trims inputs, enforces both-or-neither semantics, updates theappsrecord underwithLock, and triggers UI refresh/toasts.Adds Playwright E2E coverage for configure/edit/clear flows, validation behavior, and canceling edits.
Written by Cursor Bugbot for commit 3f0abe3. This will update automatically on new commits. Configure here.
Summary by cubic
Let users configure custom install and start commands from the Configure panel after an app is created. Defaults to “pnpm install && pnpm dev” when unset, and both fields are required when customizing.
New Features
Bug Fixes
Written for commit 3f0abe3. Summary will update on new commits.