Added Commandline Arguments for Chat Creation#392
Added Commandline Arguments for Chat Creation#392derDere wants to merge 14 commits intonanbingxyz:mainfrom
Conversation
Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
… creation Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
…dling Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
…ust regex parsing Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
Co-authored-by: derDere <36049068+derDere@users.noreply.github.com>
…-args Implement CLI startup arguments for auto-creating chats
📝 WalkthroughWalkthroughThis PR introduces a Terminal Startup Arguments feature that allows users to pass CLI arguments to the 5ire application to auto-create and navigate to chats with specified configurations. It includes CLI parsing, IPC communication, a React startup handler component, test coverage, and comprehensive documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Process<br/>Args
participant Main as Main Process
participant IPC as IPC Channel
participant Renderer as Renderer Process
participant React as StartupHandler<br/>Component
participant Store as Chat Store
CLI->>Main: app launch with --provider --model --prompt args
Main->>Main: parseStartupArgs(argv)
Main->>Main: enqueueStartupArgs(parsed args)
Main->>Renderer: app ready event
Renderer->>React: mount StartupHandler
React->>IPC: send startup-handler-ready
IPC->>Main: receive startup-handler-ready
Main->>IPC: send startup-new-chat with queued args
IPC->>React: onNewChat listener triggered
React->>React: normalize provider/model names
React->>Store: createChat(chatData)
Store-->>React: new chat created
React->>React: navigate to new chat
React->>React: editStage(resolved provider/model)
alt prompt provided
React->>React: emit startup-submit event
React->>Store: auto-submit message with prompt
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 7a9c592..11ab98e
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (14)
• .erb/scripts/wait-for-renderer.ts
• docs/CLI_ARGUMENTS.md
• docs/IMPLEMENTATION_SUMMARY.md
• package-lock.json
• package.json
• src/main/cli-args.ts
• src/main/main.ts
• src/main/preload.ts
• src/renderer/ChatContext.ts
• src/renderer/components/FluentApp.tsx
• src/renderer/components/StartupHandler.tsx
• src/renderer/pages/chat/index.tsx
• src/stores/useChatStore.ts
• test/main/cli-args.spec.ts
There was a problem hiding this comment.
Pull request overview
This PR implements command-line arguments for automatic chat creation on startup or when launching a second instance of 5ire. It addresses feature request issue #391 by adding a CLI argument parser, IPC communication between main and renderer processes, and a React component to handle startup events.
Key changes:
- Added CLI argument parser supporting both individual flags (
--new-chat --provider openai) and JSON format (--chat '{...}') - Integrated argument handling into main process with queuing for pending args until renderer is ready
- Created StartupHandler component to listen for IPC events and create chats with pre-configured settings
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| test/main/cli-args.spec.ts | Comprehensive test suite covering argument parsing, normalization, and edge cases |
| src/main/cli-args.ts | CLI argument parser with support for individual flags and JSON format |
| src/main/main.ts | Main process integration for handling startup args on cold start and second instance |
| src/main/preload.ts | IPC API exposed via contextBridge for startup events |
| src/renderer/components/StartupHandler.tsx | React component handling startup events and chat creation |
| src/renderer/components/FluentApp.tsx | Integration of StartupHandler component |
| src/stores/useChatStore.ts | Enhanced createChat to preserve explicit provider/model overrides and debug logging |
| src/renderer/pages/chat/index.tsx | Event bus handling for startup-submit, dependency array fixes, and model/provider tracking |
| src/renderer/ChatContext.ts | Enhanced debug logging for provider/model resolution |
| package.json | Added argparse dependency and moved @postlight/parser to devDependencies |
| docs/CLI_ARGUMENTS.md | User documentation for CLI arguments |
| docs/IMPLEMENTATION_SUMMARY.md | Technical implementation documentation |
| .erb/scripts/wait-for-renderer.ts | Utility script to wait for renderer before starting main process |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isReady = useMemo(() => { | ||
| return chatContext.isReady(); | ||
| }, [provider, model, chatContext]); | ||
| }, [chatContext, readySignature]); |
There was a problem hiding this comment.
The readySignature dependency is problematic. When provider or model changes, readySignature is recreated, causing the useMemo to re-run. However, readySignature itself is never used in the memo callback. Instead, include provider and model directly in the dependency array: [chatContext, provider, model]. This makes the intent clearer and avoids creating an intermediate variable that's not actually needed.
| }, [chatContext, readySignature]); | |
| }, [chatContext, provider, model]); |
| isUserScrollingRef.current = false; | ||
| }; | ||
| }, []); | ||
| }, [handleScroll]); |
There was a problem hiding this comment.
Including handleScroll in the dependency array causes an infinite loop. The handleScroll function is defined with useCallback earlier, but it's not shown in this diff. If handleScroll changes on every render (which is likely if it doesn't have proper dependencies), this useEffect will continuously add/remove event listeners. Verify that handleScroll is properly memoized with useCallback and has stable dependencies, or remove it from this dependency array if it's stable.
| }, [handleScroll]); | |
| }, []); |
| setKeyword, | ||
| countInput, | ||
| countOutput, | ||
| countBlobInput, |
There was a problem hiding this comment.
The dependency countBlobInput is added to the onSubmit callback's dependency array, but countBlobInput is not visible in the diff and may not have been used in the callback before. If this function wasn't previously listed as a dependency and is now being added, ensure it's actually used within the onSubmit callback. Adding unused dependencies increases re-renders unnecessarily.
| countBlobInput, |
| const modelCtx = chatContext.getModel(); | ||
| const temperature = chatContext.getTemperature(); | ||
| const maxTokens = chatContext.getMaxTokens(); | ||
| let currentChat = chatContext.getActiveChat(); |
There was a problem hiding this comment.
[nitpick] The variable currentChat is declared with let and reassigned later (lines 314 and 338), which makes the control flow harder to follow. Consider refactoring to use const declarations with more descriptive names for each stage (e.g., initialChat, createdChat, updatedChat) to make the logic clearer and prevent accidental reassignment.
| let currentChat = chatContext.getActiveChat(); | |
| const initialChat = chatContext.getActiveChat(); |
| } else { | ||
| tempStage = pick(tempStage, Object.keys(defaultTempStage)); | ||
| } | ||
| /* eslint-disable @typescript-eslint/no-unused-vars, no-shadow */ |
There was a problem hiding this comment.
The eslint-disable comment disables no-unused-vars and no-shadow for the entire IChatStore interface. Consider addressing the underlying issues rather than disabling the rules. If specific parameters or variables are intentionally unused in interface definitions, use prefixed underscores (e.g., _unused) to indicate this explicitly.
| /* eslint-disable @typescript-eslint/no-unused-vars, no-shadow */ |
| // Force UI update by setting the stage with the correct provider/model | ||
| // This ensures the ModelCtrl component displays the right values | ||
| // Use longer delay to ensure chat context fully refreshes before auto-submit | ||
| setTimeout(async () => { |
There was a problem hiding this comment.
The 1000ms timeout on line 90 (later line 111) is another magic number. This should be extracted to a named constant (e.g., STAGE_UPDATE_DELAY_MS = 1000) with documentation explaining why this delay is necessary for the stage update to take effect.
| }, | ||
| "devDependencies": { | ||
| "@postlight/parser": "^2.2.3", | ||
| "@axiomhq/js": "^1.0.0-rc.1", |
There was a problem hiding this comment.
The dependency @postlight/parser was moved from dependencies to devDependencies (line 99), but this change wasn't mentioned in the PR description. If this package is only used in development/testing, this is correct. However, verify that no production code imports this package. If it's used in the renderer process (which is bundled), it should remain in dependencies.
| - When launched with startup arguments, 5ire will: | ||
| 1. Create a new chat with the specified configuration | ||
| 2. Navigate to the newly created chat | ||
| 3. If a `--prompt` is provided, it will be set as the initial input (but not automatically sent) |
There was a problem hiding this comment.
This statement conflicts with the implementation. Looking at StartupHandler.tsx lines 104-109, when a prompt is provided, it emits a 'startup-submit' event, which triggers auto-submission via the onSubmit callback in the chat page. Either update the documentation to accurately reflect that the prompt IS auto-submitted, or remove the auto-submit functionality from the code.
| 3. If a `--prompt` is provided, it will be set as the initial input (but not automatically sent) | |
| 3. If a `--prompt` is provided, it will be set as the initial input and automatically sent |
| const availablePort = await detectPort(port); | ||
| if (availablePort !== port) { | ||
| console.log(chalk.greenBright('✓ Renderer is ready!')); | ||
| return; |
There was a problem hiding this comment.
The return statement on line 32 is unnecessary after process.exit(1). The process.exit() call immediately terminates the process, so the return will never execute. Remove this redundant return statement.
|
|
||
| // Handle startup arguments first to create chat if needed | ||
| handleStartupArgs(commandLine); | ||
|
|
||
| // Then check for deep links (can coexist with startup args) | ||
| const deepLink = commandLine.find((arg) => arg.startsWith(`${protocol}://`)); | ||
| if (deepLink) { | ||
| onDeepLink(deepLink); | ||
| } |
There was a problem hiding this comment.
[nitpick] The comment mentions that startup args and deep links "can coexist", but there's no explanation of the expected behavior when both are present. Should the chat be created first, then the deep link handled? Or should one take precedence? Document the intended interaction between these two features.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
.erb/scripts/wait-for-renderer.ts (2)
4-4: Consider using environment variable for port consistency.The port is hardcoded as
1212, butcheck-port-in-use.jsusesprocess.env.PORT || '1212'. For consistency across scripts, consider reading from the environment variable.-const port = 1212; +const port = parseInt(process.env.PORT || '1212', 10);
24-26: Remove unreachable code afterprocess.exit(1).The
returnstatement on line 25 is unreachable sinceprocess.exit(1)terminates the process.console.log(chalk.red('✗ Renderer did not start within 60 seconds.')); console.log( chalk.yellow( 'Please start the renderer manually with: npm run start:renderer', ), ); process.exit(1); - return; }test/main/cli-args.spec.ts (1)
1-194: Good test coverage!The test suite comprehensively covers the CLI argument parsing including edge cases for malformed model strings with colons.
Consider adding a test to verify the documented behavior that
--chatJSON format takes precedence over individual flags when both are provided (perdocs/CLI_ARGUMENTS.mdline 121).Example test to add:
test('should prioritize --chat JSON over individual flags', () => { const argv = [ 'node', 'app.js', '--new-chat', '--provider', 'openai', '--chat', JSON.stringify({ provider: 'anthropic', model: 'claude-3' }), ]; const result = parseStartupArgs(argv); expect(result).toEqual({ provider: 'anthropic', model: 'claude-3', }); });docs/IMPLEMENTATION_SUMMARY.md (1)
93-101: Add language specifier to fenced code block.The data flow diagram code block should have a language specifier for markdown linting compliance. Since it's a text diagram, use
textorplaintext.-``` +```text CLI Args → parseStartupArgs() → handleStartupArgs() → IPC Eventsrc/renderer/components/StartupHandler.tsx (2)
90-111: Hardcoded delays may be fragile.The nested
setTimeoutcalls with hardcoded delays (1000ms and 500ms) are timing-based workarounds that may not be sufficient on slower systems or under heavy load. Consider using a more robust approach with event-driven coordination or retry logic.If refactoring isn't feasible now, consider:
- Making the delays configurable
- Adding a comment explaining why these specific values were chosen
- Using a single combined delay or implementing a proper ready-state check
This is acceptable for the initial implementation but may cause flaky behavior in some environments.
112-116: Consider surfacing errors to the user.The error is logged via
debug()but the user receives no feedback when chat creation fails from startup arguments. For a better UX, consider showing a toast notification on failure.} catch (error) { debug('Failed to create chat from startup args:', error); // Optionally show user feedback: // showToast({ message: 'Failed to create chat from startup arguments', type: 'error' }); }src/main/preload.ts (1)
273-302: Consider extracting the duplicated type definition.The
argstype definition duplicatesStartupChatArgsfromsrc/main/cli-args.ts. While preload scripts have limitations on imports, this creates maintenance burden if the interface changes.The implementation correctly follows the existing IPC subscription pattern with proper cleanup via the returned disposer function.
Consider creating a shared types file that both modules can reference, or add a comment noting the duplication:
+ // Note: This type mirrors StartupChatArgs from cli-args.ts startup: { onNewChat( callback: (args: {src/main/main.ts (1)
73-78: Redundant exit calls.Both
app.exit(0)andprocess.exit(0)are called.app.exit()should be sufficient as it triggers Electron's graceful shutdown. Theprocess.exit(0)is redundant and may bypass cleanup.if (hasCliHelpFlag(originalArgv)) { console.log(CLI_HELP_TEXT); logging.info(CLI_HELP_TEXT); app.exit(0); - process.exit(0); }src/main/cli-args.ts (1)
25-30: Consider using debug-level logging for argv.Logging the full
argvarray atinfolevel on every invocation could be noisy in production. Consider usingdebuglevel for this diagnostic output.- logging.info( + logging.debug?.( 'parseStartupArgs received argv:', JSON.stringify(argv, null, 2), );Or if debug isn't available, gate behind a check:
+ if (process.env.NODE_ENV === 'development') { logging.info( 'parseStartupArgs received argv:', JSON.stringify(argv, null, 2), ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.erb/scripts/wait-for-renderer.ts(1 hunks)docs/CLI_ARGUMENTS.md(1 hunks)docs/IMPLEMENTATION_SUMMARY.md(1 hunks)package.json(3 hunks)src/main/cli-args.ts(1 hunks)src/main/main.ts(7 hunks)src/main/preload.ts(2 hunks)src/renderer/ChatContext.ts(2 hunks)src/renderer/components/FluentApp.tsx(2 hunks)src/renderer/components/StartupHandler.tsx(1 hunks)src/renderer/pages/chat/index.tsx(11 hunks)src/stores/useChatStore.ts(4 hunks)test/main/cli-args.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/cli-args.ts (2)
src/main/logging.ts (1)
error(65-67).erb/scripts/remove-useless.js (1)
path(3-3)
src/renderer/components/StartupHandler.tsx (2)
src/main/preload.ts (1)
unsubscribe(234-236)src/intellichat/types.ts (1)
IStage(247-258)
test/main/cli-args.spec.ts (1)
src/main/cli-args.ts (1)
parseStartupArgs(25-151)
src/main/main.ts (1)
src/main/cli-args.ts (2)
StartupChatArgs(9-16)parseStartupArgs(25-151)
.erb/scripts/wait-for-renderer.ts (1)
.erb/scripts/check-port-in-use.js (1)
port(4-4)
src/stores/useChatStore.ts (4)
src/renderer/logging.ts (1)
debug(29-31)src/intellichat/types.ts (1)
IChat(14-30)src/main/preload.ts (2)
get(62-64)get(159-161)src/utils/util.ts (1)
date2unix(6-8)
src/renderer/components/FluentApp.tsx (1)
src/renderer/components/StartupHandler.tsx (1)
StartupHandler(15-128)
🪛 markdownlint-cli2 (0.18.1)
docs/IMPLEMENTATION_SUMMARY.md
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (16)
src/renderer/components/FluentApp.tsx (1)
21-21: LGTM!The
StartupHandlercomponent is correctly imported and placed inside theRoutercontext, which is necessary for theuseNavigatehook it uses. Since it rendersnull, there's no visual impact on the UI.Also applies to: 141-141
src/renderer/ChatContext.ts (1)
44-52: LGTM!The added debug statements provide useful visibility into provider/model resolution logic, which will help troubleshoot issues with the new startup argument feature. Using the
debugmodule ensures these logs can be easily toggled on/off.src/renderer/components/StartupHandler.tsx (1)
15-127: Overall implementation looks solid.The component properly:
- Uses a ref-based guard against race conditions
- Normalizes provider/model names for case-insensitive matching
- Cleans up the IPC listener on unmount
- Signals readiness via IPC after mounting
The integration with the chat store and navigation is well-structured.
package.json (1)
125-125: The review comment is based on incorrect information about the package.json contents.The dependency at line 125 of package.json is
"cross-env": "^7.0.3", notargparse. Theargparsepackage (v2.0.1) only appears inpackage-lock.jsonas a transitive dependency ofjs-yamlandmarkdown-it, both of which are legitimate dev dependencies.argparseis not a direct dependency that was added to the project, so there is no unused dependency issue here.src/main/preload.ts (1)
41-47: LGTM - Channel types properly extended.The new
startup-handler-readyandstartup-new-chatchannels are correctly added to theChannelsunion type, following the existing pattern.src/main/main.ts (4)
54-61: LGTM - Early argv caching is essential.Caching
process.argvbefore any hooks can modify it is the correct approach for handling electronmon's argument reordering. The help flag detection using aSetwith lowercase comparison is efficient.
279-305: Startup args handling is well-structured.The queue-based approach with
flushStartupArgsandenqueueStartupArgscorrectly handles the timing between main process startup and renderer readiness. The early return guards influshStartupArgsprevent premature sends.
311-342: Good handling of second-instance with CLI args and deep links.The second-instance handler correctly prioritizes showing help dialog, then processes startup args before deep links. This allows both features to coexist as documented.
Minor: Consider adding a guard to avoid processing startup args if help was shown (line 320 returns early, so this is actually fine).
430-436: IPC listeners correctly flush pending args.Both
install-tool-listener-readyandstartup-handler-readytriggerflushStartupArgs(), ensuring args are sent regardless of which ready signal arrives first. This is a robust approach.src/main/cli-args.ts (1)
160-184: LGTM - Normalization handles Provider:model format well.The regex properly validates the format (exactly one colon with non-empty parts on both sides) and correctly preserves an explicitly provided
--providerover the extracted one.src/renderer/pages/chat/index.tsx (3)
118-121: LGTM - Ready state now reacts to provider/model changes.Using a composite signature string ensures
isReadyis recalculated when provider or model changes. The nullish coalescing with empty strings handles undefined values gracefully.
683-728: LGTM - Startup-submit event handling properly integrated.The event bus listener pattern is consistent with the retry handler. Using a local
eventBusRefand cleaning up both handlers on unmount prevents memory leaks. The debug logging provides good traceability.
656-680: Dependency array is comprehensive.The expanded dependency array correctly captures all external references used within the
onSubmitcallback, preventing stale closure bugs.src/stores/useChatStore.ts (3)
50-51: ESLint suppressions are appropriate for the interface declaration.The
@typescript-eslint/no-unused-varsandno-shadowrules can incorrectly flag interface method parameters. The scoped disable/enable pattern is clean.Also applies to: 120-120
374-389: createChat merge strategy correctly prioritizes explicit arguments.The change from using
chatdirectly to merging ontocurrentChatwith explicitundefinedchecks for provider/model allows:
- Inheriting current state when no value provided
- Overriding with any value (including empty string) when explicitly passed
This is important for the startup args flow where provider/model should be honored even if they differ from current stage.
542-543: Debug logging improvements aid troubleshooting.Adding provider/model to the fetchChat debug output helps trace where chat configuration values originate, which is useful given the new startup args flow.
| 3. If a `--prompt` is provided, it will be set as the initial input (but not automatically sent) | ||
|
|
There was a problem hiding this comment.
Documentation contradicts implementation behavior.
The documentation states the prompt "will be set as the initial input (but not automatically sent)". However, reviewing src/renderer/components/StartupHandler.tsx (lines 103-110), the implementation explicitly auto-submits the prompt via eventBus.emit('startup-submit', args.prompt).
Update the documentation to reflect the actual behavior:
-- If a `--prompt` is provided, it will be set as the initial input (but not automatically sent)
+ 3. If a `--prompt` is provided, it will be automatically submittedCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/CLI_ARGUMENTS.md around lines 111 to 112, the documentation incorrectly
says a provided --prompt is set as initial input but not automatically sent; in
reality StartupHandler.tsx (lines 103–110) emits eventBus.emit('startup-submit',
args.prompt) which auto-submits the prompt. Update the documentation text to
state that when --prompt is provided it will be automatically submitted on
startup (mentioning the startup-submit event for clarity) and remove the "(but
not automatically sent)" clause so the docs match the implementation.
| ipcMain.on('install-tool-listener-ready', () => { | ||
| if (pendingStartupArgs !== null) { | ||
| mainWindow?.webContents.send('startup-new-chat', pendingStartupArgs); | ||
| pendingStartupArgs = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
IPC channel name mismatch.
The documentation references install-tool-listener-ready but src/renderer/components/StartupHandler.tsx (line 119) sends startup-handler-ready. Update the documentation to match the implementation.
-// Send pending args when renderer is ready
-ipcMain.on('install-tool-listener-ready', () => {
+// Send pending args when startup handler is ready
+ipcMain.on('startup-handler-ready', () => {
if (pendingStartupArgs !== null) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/IMPLEMENTATION_SUMMARY.md around lines 47 to 52 the IPC channel name is
incorrect: the doc shows "install-tool-listener-ready" but the renderer actually
sends "startup-handler-ready" (see src/renderer/components/StartupHandler.tsx
line 119); update the documentation to use "startup-handler-ready" (leave the
rest of the snippet including the "startup-new-chat" send call unchanged) so the
docs match the implementation.
| // Handle --chat JSON format | ||
| const chatIndex = argv.indexOf('--chat'); | ||
| if (chatIndex !== -1) { | ||
| try { | ||
| // Find the next non-flag argument after --chat | ||
| for (let i = chatIndex + 1; i < argv.length; i += 1) { | ||
| const arg = argv[i]; | ||
| if (!arg.startsWith('--') && !arg.endsWith('.js') && arg !== '.') { | ||
| const parsed = JSON.parse(arg); | ||
| return normalizeStartupArgs(parsed); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| logging.error('Failed to parse --chat JSON argument:', error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON parsing for --chat may skip valid JSON argument.
The loop at line 42 searches for a non-flag argument after --chat, but if electronmon scrambles args such that another flag appears between --chat and its value, this could fail silently. The current fallback to manual parsing should handle this, but the try-catch returns null on parse failure rather than falling through to manual parsing.
Consider falling through to manual parsing on JSON parse failure instead of returning null:
} catch (error) {
logging.error('Failed to parse --chat JSON argument:', error);
- return null;
+ // Fall through to manual parsing
}
}
+
+ // If --chat was present but JSON parsing failed, continue with manual parsing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle --chat JSON format | |
| const chatIndex = argv.indexOf('--chat'); | |
| if (chatIndex !== -1) { | |
| try { | |
| // Find the next non-flag argument after --chat | |
| for (let i = chatIndex + 1; i < argv.length; i += 1) { | |
| const arg = argv[i]; | |
| if (!arg.startsWith('--') && !arg.endsWith('.js') && arg !== '.') { | |
| const parsed = JSON.parse(arg); | |
| return normalizeStartupArgs(parsed); | |
| } | |
| } | |
| } catch (error) { | |
| logging.error('Failed to parse --chat JSON argument:', error); | |
| return null; | |
| } | |
| } | |
| // Handle --chat JSON format | |
| const chatIndex = argv.indexOf('--chat'); | |
| if (chatIndex !== -1) { | |
| try { | |
| // Find the next non-flag argument after --chat | |
| for (let i = chatIndex + 1; i < argv.length; i += 1) { | |
| const arg = argv[i]; | |
| if (!arg.startsWith('--') && !arg.endsWith('.js') && arg !== '.') { | |
| const parsed = JSON.parse(arg); | |
| return normalizeStartupArgs(parsed); | |
| } | |
| } | |
| } catch (error) { | |
| logging.error('Failed to parse --chat JSON argument:', error); | |
| // Fall through to manual parsing | |
| } | |
| } | |
| // If --chat was present but JSON parsing failed, continue with manual parsing |
🤖 Prompt for AI Agents
In src/main/cli-args.ts around lines 37 to 53, the current handling of --chat
returns null inside the catch when JSON.parse fails, preventing the code from
falling back to the subsequent manual parsing path; change the catch to log the
parse error but do NOT return — allow execution to continue so the existing
manual parsing logic can run as a fallback; ensure the try only surrounds the
JSON.parse and parsed normalization, and after catching the error simply
break/continue out of the inner loop (or continue outer flow) so the function
falls through to manual parsing instead of exiting early.
| // Match flags to values by position | ||
| const args: StartupChatArgs = {}; | ||
|
|
||
| for (let i = 0; i < foundFlags.length && i < values.length; i += 1) { | ||
| const flag = foundFlags[i]; | ||
| const value = values[i]; | ||
|
|
||
| switch (flag) { | ||
| case '--provider': | ||
| args.provider = value; | ||
| break; | ||
| case '--model': | ||
| args.model = value; | ||
| break; | ||
| case '--system': | ||
| args.system = value; | ||
| break; | ||
| case '--summary': | ||
| args.summary = value; | ||
| break; | ||
| case '--prompt': | ||
| args.prompt = value; | ||
| break; | ||
| case '--temperature': { | ||
| const temp = parseFloat(value); | ||
| if (!Number.isNaN(temp)) { | ||
| args.temperature = temp; | ||
| } | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for electronmon references in the codebase
rg -i "electronmon" -n -B2 -A2Repository: nanbingxyz/5ire
Length of output: 2378
🏁 Script executed:
#!/bin/bash
# Search for the cli-args.ts file and examine its content
fd "cli-args.ts" --exec wc -l {}Repository: nanbingxyz/5ire
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for foundFlags and values array population
rg -n "foundFlags\s*=" src/main/cli-args.ts -A3 -B3Repository: nanbingxyz/5ire
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Find cli-args.ts file
fd "cli-args\.ts" -type fRepository: nanbingxyz/5ire
Length of output: 229
🏁 Script executed:
#!/bin/bash
# List the directory structure to understand the layout
git ls-files | grep -E "\.(ts|js)$" | head -30Repository: nanbingxyz/5ire
Length of output: 936
🏁 Script executed:
#!/bin/bash
# Search for cli-args.ts more broadly
fd "cli-args"Repository: nanbingxyz/5ire
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if the file exists in any form
find . -name "*cli-args*" 2>/dev/nullRepository: nanbingxyz/5ire
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check git status and branch
git status
git branch -aRepository: nanbingxyz/5ire
Length of output: 409
🏁 Script executed:
#!/bin/bash
# Search for StartupChatArgs type definition which is referenced in the code snippet
rg "StartupChatArgs" -nRepository: nanbingxyz/5ire
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Search for the pattern of matching flags to values
rg "foundFlags" -n -B2 -A2Repository: nanbingxyz/5ire
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Checkout the PR branch
git checkout coderabbit_392
git log --oneline -5Repository: nanbingxyz/5ire
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Now search for the files and patterns
rg "StartupChatArgs" -n
fd "cli-args"Repository: nanbingxyz/5ire
Length of output: 840
🏁 Script executed:
#!/bin/bash
# Read the cli-args.ts file to understand the implementation
cat -n src/main/cli-args.tsRepository: nanbingxyz/5ire
Length of output: 6189
🏁 Script executed:
#!/bin/bash
# Read the test file to understand how this is being tested
cat -n test/main/cli-args.spec.tsRepository: nanbingxyz/5ire
Length of output: 6860
🏁 Script executed:
#!/bin/bash
# Check for any other references to electronmon in the codebase
rg "electronmon" -n -B2 -A2Repository: nanbingxyz/5ire
Length of output: 2916
🌐 Web query:
electronmon npm package argument handling behavior
💡 Result:
Short answer: electronmon forwards CLI arguments to the Electron process unchanged (you can also supply an args array via the JS API). If you run electronmon from an npm script, use the usual double-dash (--) to separate npm/script flags from app arguments so they reach Electron via process.argv. (npmjs.com)
Citations:
🏁 Script executed:
#!/bin/bash
# Search for any GitHub issues or comments discussing electronmon argument scrambling
rg -i "scramble|scrambl" -n -B3 -A3Repository: nanbingxyz/5ire
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Check git history for this file to understand why this approach was taken
git log --oneline --all -- src/main/cli-args.tsRepository: nanbingxyz/5ire
Length of output: 508
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or issues in the repo about electronmon
rg -i "electronmon" --type md -A5 -B5Repository: nanbingxyz/5ire
Length of output: 41
electronmon does not scramble arguments; it forwards them unchanged
Based on verification, electronmon's official documentation states it "forwards CLI arguments to the Electron process unchanged." The code comment claiming "electronmon's scrambled argument order" appears to be incorrect.
However, the review's underlying concern remains valid: the positional matching strategy (collecting all flags separately, then all values, then matching by position) is fragile and lacks validation. While the current implementation works with normal argument order, it's unnecessarily complex and relies on an unverified assumption about electronmon's behavior.
Suggested improvements:
- Verify the actual need for this strategy by testing electronmon's actual argument handling
- Add validation to detect when the number of flags doesn't match values
- Consider a more direct approach of matching each flag to the immediately following value (standard CLI parsing)
- Correct the misleading comment about "scrambled" arguments
The existing debug logging (lines 110-111, 148) is helpful but insufficient—add explicit validation or error handling for mismatches.
🤖 Prompt for AI Agents
In src/main/cli-args.ts around lines 113 to 146, the comment about
"electronmon's scrambled argument order" is incorrect and the current positional
matching (collecting flags and values separately then matching by index) is
fragile; replace the comment, add validation to detect mismatched counts, and
simplify parsing by iterating through the original argv sequence so each flag is
matched to its immediate following value (the standard CLI pattern).
Specifically: remove or correct the misleading comment, check that the number of
discovered flags equals the number of values and log an explicit error/throw if
they differ, and refactor the loop to walk argv once assigning the next token as
the value for a recognized flag (with the same conversions/validation for
temperature), falling back to clear error handling when a value is missing.
Ensure existing debug logs are preserved and augmented with the validation
failure message.
| const shouldUpdateProvider = | ||
| !currentChat.provider || currentChat.provider === providerCtx.name; | ||
| const shouldUpdateModel = | ||
| !currentChat.model || currentChat.model === modelCtx.name; | ||
|
|
||
| if (shouldUpdateProvider && shouldUpdateModel) { | ||
| await updateChat({ | ||
| id: activeChatId, | ||
| provider: providerCtx.name, | ||
| model: modelCtx.name, | ||
| temperature, | ||
| summary, | ||
| }); | ||
| } else { | ||
| await updateChat({ | ||
| id: activeChatId, | ||
| temperature, | ||
| summary, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Inverted or confusing condition logic for provider/model updates.
The variable names shouldUpdateProvider and shouldUpdateModel suggest these should be true when an update is needed. However, the conditions check if the current value is empty OR equals the new value, which would be true when no update is needed (values already match).
This appears to preserve provider/model from the first message, but the naming is misleading.
Consider renaming for clarity:
- const shouldUpdateProvider =
- !currentChat.provider || currentChat.provider === providerCtx.name;
- const shouldUpdateModel =
- !currentChat.model || currentChat.model === modelCtx.name;
+ const canOverrideProvider =
+ !currentChat.provider || currentChat.provider === providerCtx.name;
+ const canOverrideModel =
+ !currentChat.model || currentChat.model === modelCtx.name;
- if (shouldUpdateProvider && shouldUpdateModel) {
+ if (canOverrideProvider && canOverrideModel) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldUpdateProvider = | |
| !currentChat.provider || currentChat.provider === providerCtx.name; | |
| const shouldUpdateModel = | |
| !currentChat.model || currentChat.model === modelCtx.name; | |
| if (shouldUpdateProvider && shouldUpdateModel) { | |
| await updateChat({ | |
| id: activeChatId, | |
| provider: providerCtx.name, | |
| model: modelCtx.name, | |
| temperature, | |
| summary, | |
| }); | |
| } else { | |
| await updateChat({ | |
| id: activeChatId, | |
| temperature, | |
| summary, | |
| }); | |
| } | |
| const canOverrideProvider = | |
| !currentChat.provider || currentChat.provider === providerCtx.name; | |
| const canOverrideModel = | |
| !currentChat.model || currentChat.model === modelCtx.name; | |
| if (canOverrideProvider && canOverrideModel) { | |
| await updateChat({ | |
| id: activeChatId, | |
| provider: providerCtx.name, | |
| model: modelCtx.name, | |
| temperature, | |
| summary, | |
| }); | |
| } else { | |
| await updateChat({ | |
| id: activeChatId, | |
| temperature, | |
| summary, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/renderer/pages/chat/index.tsx around lines 317 to 336, the booleans
shouldUpdateProvider and shouldUpdateModel are inverted/misnamed: they are true
when the current chat provider/model is empty or already equals the new value
(i.e., no update needed). Change the logic (or rename variables) so they are
true only when the stored value differs from the new value (meaning an update is
required), then call updateChat with provider and model when those flags
indicate a change; otherwise call updateChat without provider/model. Ensure
variable names match the semantics (e.g., needsProviderUpdate/needsModelUpdate)
and update the conditional that decides which fields to include in the update
accordingly.
|
过 |
This request solves the feature request (issue): #391
I added a command line argument parser that allows the user to create a new custom chat on startup or trigger the creation of a new custom chat if 5ire is already running. The following command line options were created:
I'll admit my demand for this feature was high and urgent, so this solution was quickly vibe coded within two days. My company can't wait too long for this feature, and now with this fork we can already use it. Even if this pull request fails, I hope you make use it to have a deeper look at my feature request mentioned above.
High-level PR Summary
This PR adds command-line argument support to automatically create and configure chats when launching the application. Users can now specify
--new-chatalong with options like--provider,--model,--system,--prompt,--summary, and--temperatureto create pre-configured chat sessions on startup. The implementation includes a dedicated CLI argument parser, IPC communication between main and renderer processes, comprehensive test coverage, and detailed documentation with usage examples. The feature also supports JSON format for bulk configuration and handles edge cases like provider derivation from model strings (e.g.,anthropic:claude-3-opus).⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
docs/CLI_ARGUMENTS.mddocs/IMPLEMENTATION_SUMMARY.mdsrc/main/cli-args.tstest/main/cli-args.spec.tssrc/main/preload.tssrc/main/main.tssrc/renderer/components/StartupHandler.tsxsrc/renderer/components/FluentApp.tsxsrc/renderer/pages/chat/index.tsxsrc/stores/useChatStore.tssrc/renderer/ChatContext.ts.erb/scripts/wait-for-renderer.tspackage.jsonpackage-lock.jsonSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.