Feat/i18n and ssh naming#6
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an idempotent shell-completion installer: exports Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Change "Generate new ed25519 key" to "Generate new SSH key" in both en and ko locale messages. The actual keygen still uses ed25519 internally.
- Add installCompletion() that appends eval line to ~/.zshrc or ~/.bashrc - Uses marker comment to avoid duplicate installation - Called automatically during ghem init - Add i18n messages for completion install status
Jamkris
force-pushed
the
feat/i18n-and-ssh-naming
branch
from
e345969 to
7e2ded7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/completion.ts (1)
3-7:⚠️ Potential issue | 🟡 Minor
detectShell()silently falls back to bash for unsupported shells.Fish, PowerShell, and other shell users will see a success message but get non-functional completion. Consider either:
- Returning
nullfor unsupported shells and skipping installation- Warning users that only bash/zsh are supported
Possible fix to skip unsupported shells
-function detectShell(): string { +function detectShell(): string | null { const shell = process.env.SHELL ?? ''; if (shell.includes('zsh')) return 'zsh'; - return 'bash'; + if (shell.includes('bash')) return 'bash'; + return null; }Then update
installCompletion()to handle null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/completion.ts` around lines 3 - 7, detectShell() currently returns 'bash' for any unknown SHELL which silently gives non-functional completions for shells like fish or PowerShell; change detectShell() to return null for unsupported shells instead of defaulting to 'bash', and update installCompletion() to check for a null return from detectShell() and skip installation while printing a clear warning that only bash/zsh are supported (or provide instructions/exit code). Use the functions detectShell and installCompletion to locate where to implement the null return and the early-skip/warning behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/completion.ts`:
- Around line 127-129: Wrap the appendFileSync call in
src/commands/completion.ts in a try/catch so a filesystem permission error
doesn't throw out of setupCompletion() in init.ts after config is written; catch
the error from appendFileSync, log the error (include the thrown error object)
and return a non-success result (e.g., { installed: false, shell, rcFile } or
similar) instead of rethrowing so init flow can handle the failure gracefully.
Ensure you reference the appendFileSync invocation and adjust the caller
behavior in setupCompletion() if it expects a boolean/installed flag so failures
are propagated without crashing.
- Around line 110-112: Replace the inline CommonJS require calls for built-ins
by adding top-level ESM imports for existsSync, readFileSync, appendFileSync
from 'node:fs', join from 'node:path', and homedir from 'node:os' and remove the
three inline require lines (the ones that declare
existsSync/readFileSync/appendFileSync, join, and homedir) in completion.ts;
update any usages to rely on those imported bindings so the file uses standard
ESM imports instead of runtime require().
---
Outside diff comments:
In `@src/commands/completion.ts`:
- Around line 3-7: detectShell() currently returns 'bash' for any unknown SHELL
which silently gives non-functional completions for shells like fish or
PowerShell; change detectShell() to return null for unsupported shells instead
of defaulting to 'bash', and update installCompletion() to check for a null
return from detectShell() and skip installation while printing a clear warning
that only bash/zsh are supported (or provide instructions/exit code). Use the
functions detectShell and installCompletion to locate where to implement the
null return and the early-skip/warning behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5e9208a-fc6d-4de8-a9e7-cb85271ad2a8
📒 Files selected for processing (5)
src/commands/completion.tssrc/commands/init.tssrc/i18n/locales/en.tssrc/i18n/locales/ko.tssrc/i18n/types.ts
- Replace inline require() with top-level ESM imports - Wrap appendFileSync in try/catch to prevent init abort on permission errors
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/commands/completion.ts (2)
110-112:⚠️ Potential issue | 🔴 CriticalReplace inline
require()with top-level ESM imports.Line 110-112 uses CommonJS
require()inside a TypeScript ESM module, which is a runtime hazard in Node ESM (requiremay be undefined).Suggested fix
import type { Command } from 'commander'; +import { appendFileSync, existsSync, readFileSync } from 'node:fs'; +import { homedir } from 'node:os'; +import { join } from 'node:path'; @@ export function installCompletion(): { installed: boolean; shell: string; rcFile: string } { - const { existsSync, readFileSync, appendFileSync } = require('node:fs') as typeof import('node:fs'); - const { join } = require('node:path') as typeof import('node:path'); - const { homedir } = require('node:os') as typeof import('node:os'); - const shell = detectShell();#!/bin/bash set -euo pipefail echo "== package.json module type ==" fd -HI '^package\.json$' -x sh -c 'echo "--- $1"; rg -n "\"type\"\\s*:\\s*\"module\"" "$1" || true' sh {} echo "== tsconfig module settings ==" fd -HI '^tsconfig.*\.json$' -x sh -c 'echo "--- $1"; rg -n "\"module\"|\"moduleResolution\"|\"target\"" "$1" || true' sh {} echo "== inline require usage in completion command ==" rg -n "require\\('node:(fs|path|os)'\\)" src/commands/completion.tsAs per coding guidelines:
**/*.ts: TypeScript ESM project targeting Node.js 20+ and built-ins should be imported withnode:modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/completion.ts` around lines 110 - 112, The inline CommonJS requires for node built-ins (existsSync, readFileSync, appendFileSync, join, homedir) inside src/commands/completion.ts should be converted to top-level ESM imports; remove the require(...) expressions and add equivalent imports at the top of the module like: import { existsSync, readFileSync, appendFileSync } from 'node:fs' and import { join } from 'node:path' and import { homedir } from 'node:os' so the code uses proper ESM semantics and avoids runtime undefined require in Node ESM.
120-129:⚠️ Potential issue | 🟠 MajorGuard filesystem reads/writes to prevent partial init failure.
readFileSync/appendFileSynccan throw (permissions, I/O errors). That exception currently bubbles and can abortinitafter config has already been written.Suggested fix
-export function installCompletion(): { installed: boolean; shell: string; rcFile: string } { +export function installCompletion(): + | { status: 'installed'; shell: string; rcFile: string } + | { status: 'already-installed'; shell: string; rcFile: string } + | { status: 'failed'; shell: string; rcFile: string; error: Error } { @@ - if (existsSync(rcFile)) { - const content = readFileSync(rcFile, 'utf-8'); - if (content.includes(COMPLETION_MARKER)) { - return { installed: false, shell, rcFile }; - } - } - - const line = `\n${COMPLETION_MARKER}\neval "$(ghem completion --shell ${shell})"\n`; - appendFileSync(rcFile, line, 'utf-8'); - return { installed: true, shell, rcFile }; + try { + if (existsSync(rcFile)) { + const content = readFileSync(rcFile, 'utf-8'); + if (content.includes(COMPLETION_MARKER)) { + return { status: 'already-installed', shell, rcFile }; + } + } + + const line = `\n${COMPLETION_MARKER}\neval "$(ghem completion --shell ${shell})"\n`; + appendFileSync(rcFile, line, 'utf-8'); + return { status: 'installed', shell, rcFile }; + } catch (error) { + return { + status: 'failed', + shell, + rcFile, + error: error instanceof Error ? error : new Error(String(error)), + }; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/completion.ts` around lines 120 - 129, The completion rc-file read/write is unguarded (uses existsSync/readFileSync/appendFileSync) and can throw, causing init to fail after config is written; wrap the filesystem operations that check and append COMPLETION_MARKER (the existsSync/readFileSync/appendFileSync sequence around rcFile) in a try/catch, and on error avoid throwing—log or surface a non-fatal warning and return a safe result (e.g., { installed: false, shell, rcFile }) so init continues without a partial-failure; ensure the catch references the same rcFile and COMPLETION_MARKER so the behavior is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/commands/completion.ts`:
- Around line 110-112: The inline CommonJS requires for node built-ins
(existsSync, readFileSync, appendFileSync, join, homedir) inside
src/commands/completion.ts should be converted to top-level ESM imports; remove
the require(...) expressions and add equivalent imports at the top of the module
like: import { existsSync, readFileSync, appendFileSync } from 'node:fs' and
import { join } from 'node:path' and import { homedir } from 'node:os' so the
code uses proper ESM semantics and avoids runtime undefined require in Node ESM.
- Around line 120-129: The completion rc-file read/write is unguarded (uses
existsSync/readFileSync/appendFileSync) and can throw, causing init to fail
after config is written; wrap the filesystem operations that check and append
COMPLETION_MARKER (the existsSync/readFileSync/appendFileSync sequence around
rcFile) in a try/catch, and on error avoid throwing—log or surface a non-fatal
warning and return a safe result (e.g., { installed: false, shell, rcFile }) so
init continues without a partial-failure; ensure the catch references the same
rcFile and COMPLETION_MARKER so the behavior is explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aaf8f0ae-104c-437f-a171-fdb156b73228
📒 Files selected for processing (5)
src/commands/completion.tssrc/commands/init.tssrc/i18n/locales/en.tssrc/i18n/locales/ko.tssrc/i18n/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/completion.ts`:
- Around line 112-131: The installCompletion function currently conflates
"already installed" with I/O failures and leaves readFileSync uncaught; update
installCompletion so that when the rc file already contains COMPLETION_MARKER it
returns { installed: true, shell, rcFile } (idempotent success), wrap the
readFileSync call in a try/catch and convert any read errors into a PersonaError
(include the original error message), and likewise convert failures from
appendFileSync into a PersonaError instead of returning { installed: false };
reference the installCompletion function, COMPLETION_MARKER symbol, and use
PersonaError so init.ts/logger can correctly distinguish and display real I/O
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 987925cf-a431-49fc-939c-c61687e4402e
📒 Files selected for processing (1)
src/commands/completion.ts
- Return status union type (installed/already_installed/failed) instead of boolean to differentiate I/O errors from idempotent skips - Wrap readFileSync in same try/catch to handle read permission errors - Add completionFailed i18n message with manual install hint
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/completion.ts`:
- Around line 117-134: installCompletion currently treats any non-'zsh' shell as
'bash' via detectShell and unconditionally writes to rcFile; change this so that
detectShell (or installCompletion) returns a distinct unsupported/ambiguous
value for shells other than 'bash' or 'zsh' and have installCompletion return an
explicit status like 'unsupported_shell' or 'ambiguous_shell' (include the
detected shell string and null/undefined rcFile) instead of defaulting to
~/.bashrc; do not append or modify any file for unsupported shells (use
COMPLETION_MARKER, installCompletion, detectShell, and InstallResult to
implement the new status) and ensure the caller uses that status to tell the
user to run `ghem completion --shell <bash|zsh>` explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90d698da-0dde-47ae-a501-58b195425243
📒 Files selected for processing (5)
src/commands/completion.tssrc/commands/init.tssrc/i18n/locales/en.tssrc/i18n/locales/ko.tssrc/i18n/types.ts
- detectShell() now returns null for unrecognized shells (fish, nushell, etc.) - installCompletion() returns 'unsupported' status instead of silently writing to ~/.bashrc for wrong shell - Add completionUnsupported i18n message guiding manual setup
This was referenced Apr 9, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Localization