refactor: clean up codebase, remove dead code, and improve readability - #145
Conversation
- Remove dead genCommand() and editPrompt() from index.ts, extract resolveModelConfig() helper to eliminate duplicated config/model logic - Remove all commented-out code across the codebase - Remove unused imports (google, ollama, openai, anthropic, etc from ai.ts) - Remove dead getDefaultModelId() function from ai.ts - Remove dead iscancelled() function from config.ts - Remove duplicate getDetailedHardwareInfo() (identical to getHardwareInfo()) - Remove legacy evaluateGeneration() and commented-out batch eval block - Deduplicate levenshteinDistance and withRetry by exporting from eval.ts and importing in eval-improvements.ts - Simplify getDefaultModel() with data-driven env var lookup - Simplify generateTextWithModel() with spread syntax - Simplify GPU fallback in hardware.ts (identical branches consolidated) - Extract shell history logic into appendToShellHistory() helper - Fix unnecessary type assertion, redundant awaits, and unused variables - Fix inconsistent cancel handling (symbol check -> isCancel pattern) All changes verified: typecheck, lint, and build pass cleanly.
Summary of ChangesHello, 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 refactors and cleans up the codebase, resulting in a substantial reduction of 277 net lines of code across 16 files without introducing any functional changes. The primary goal was to improve code readability, remove dead or duplicated code, and eliminate commented-out sections, making the project easier to maintain and understand. These changes enhance the overall quality and efficiency of the application's core logic, AI module, configuration handling, and evaluation systems. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
WalkthroughThis PR performs extensive cleanup and refactoring: removing unused imports and functions across multiple files, consolidating model/provider configuration logic into a centralized registry-based approach, introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
The pull request primarily focuses on refactoring and simplifying the codebase by removing redundant code, consolidating logic, and improving error handling. Key changes include streamlining model configuration and command generation in src/index.ts and src/lib/ai.ts, simplifying GPU information retrieval in src/helpers/hardware.ts, and cleaning up configuration prompts in src/lib/config.ts. Additionally, utility functions like levenshteinDistance and withRetry were centralized in src/lib/eval.ts and imported where needed, and shell history appending was extracted into its own function in src/utils.ts. Review comments highlight two areas for improvement: adding warning logs in src/helpers/hardware.ts and src/lib/ai.ts when errors are caught and swallowed, to aid in debugging potential issues with external dependencies or structured output generation.
| } catch { | ||
| const result = await generateCommand(prompt, modelConf); | ||
| return { command: result, explanation: '' }; | ||
| } |
There was a problem hiding this comment.
Swallowing the error from generateObject can make it hard to debug why structured output is failing. It's beneficial to log this error to understand if there are issues with specific models or the schema, even when falling back to generateCommand.
} catch (error) {
console.warn('generateObject failed, falling back to generateCommand.', error);
const result = await generateCommand(prompt, modelConf);
return { command: result, explanation: '' };
}
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the CLI and evaluation utilities to reduce duplication and dead code, primarily by removing unused imports/helpers, extracting small utilities, and consolidating duplicated implementations.
Changes:
- Extracted shared helpers (e.g., model config resolution in CLI, shell history append) and removed commented/dead code.
- Consolidated evaluation utilities by exporting shared helpers from
eval.tsand importing them where needed. - Cleaned up unused imports/parameters and simplified control flow across CLI, config, and benchmark/eval scripts.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Removes dead code and extracts resolveModelConfig() for clearer CLI flow. |
| src/lib/ai.ts | Cleans imports, centralizes model creation via provider registry, simplifies default-model selection and generation helpers. |
| src/lib/config.ts | Removes dead cancellation helper, standardizes cancel handling, and simplifies config initialization flow. |
| src/utils.ts | Extracts shell history append helper and removes unnecessary template literals. |
| src/helpers/hardware.ts | Removes duplicated hardware helper and simplifies GPU fallback handling. |
| src/lib/eval.ts | Removes legacy/duplicated evaluation code, exports shared utilities, and simplifies scorer signatures. |
| src/lib/eval-improvements.ts | Reuses levenshteinDistance/withRetry from eval.ts and removes duplicated implementations. |
| src/lib/example.eval.ts | Removes redundant await on returned promises. |
| src/lib/basic.eval.ts | Removes unused AI imports. |
| src/lib/ci-eval.ts | Removes unused eval imports. |
| src/lib/llm-judge.eval.ts | Removes unused eval imports. |
| src/lib/enhanced-eval-example.ts | Removes unused AI imports. |
| src/commands/config.ts | Removes commented-out debug logging. |
| src/test-improved-prompt.ts | Removes unused AI imports. |
| src/bench_models.ts | Removes unused AI import and keeps benchmark path focused on structured generation. |
| bun.lock | Updates lockfile metadata (configVersion). |
Comments suppressed due to low confidence (1)
src/lib/eval-improvements.ts:311
EvalConfig.optionsstill exposesmaxConcurrency, butrunEnhancedEval()no longer reads or applies it (and parallel scoring currently uses an unboundedPromise.all). This makes the option misleading for callers; either removemaxConcurrencyfrom the public options type or implement concurrency limiting where parallelism occurs.
const { data, task, scorers, options = {} } = config;
const {
parallel = false,
saveResults = false,
outputDir = './eval-results',
continueOnError = true,
timeout = 30000,
retries = 1,
} = options;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| childProcess.stderr?.on('data', data => { | ||
| process.stderr.write(data); | ||
| }); | ||
| fs.appendFileSync(historyFilePath, entry); |
![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: c8cd7689ab
ℹ️ 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".
| } | ||
|
|
||
| // Export with the desired interface name | ||
| export { runEval as eval }; |
There was a problem hiding this comment.
Removing evaluateGeneration, EvaluationInput, and EvaluationResult turns this refactor into a breaking API change. docs/EVALUATION.md still documents the legacy interface, so any repo scripts or downstream consumers still importing the deprecated API will now fail at import time instead of getting the existing deprecation error. Keeping a thin shim here would preserve compatibility while still steering callers toward eval().
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/eval-improvements.ts (1)
529-529:⚠️ Potential issue | 🟡 MinorSame
evalidentifier issue as inllm-judge.eval.ts.This export creates the same strict mode compliance issue when consumers import it. Consider using a different export name:
🔧 Proposed fix
-export { runEnhancedEval as eval }; +export { runEnhancedEval as evaluate };Or simply export
runEnhancedEvaldirectly without aliasing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/eval-improvements.ts` at line 529, The file exports runEnhancedEval under the alias eval which can trigger strict-mode/identifier conflicts for consumers; change the export to avoid using the reserved identifier by either exporting runEnhancedEval directly (remove "export { runEnhancedEval as eval }") or pick a non-reserved alias (e.g., export { runEnhancedEval as enhancedEval }) so references use runEnhancedEval or the new alias instead of eval; update any imports that expect eval to the new name.
🧹 Nitpick comments (1)
src/index.ts (1)
67-69: Consider hoistingresolveModelConfig()outside the loop.Currently,
resolveModelConfig()is called on every loop iteration (after each refinement). This re-reads and re-validates the configuration each time, which may be unnecessary overhead if the config doesn't change during a session.If this is intentional (to support hot-reloading config changes), ignore this suggestion. Otherwise, consider moving it before the loop:
♻️ Optional refactor
let currentPrompt = prompt_parts.join(' '); let shouldContinue = true; const silent = options.silent || false; + const modelConfig = await resolveModelConfig(); while (shouldContinue) { try { - const modelConfig = await resolveModelConfig();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 67 - 69, Hoist resolveModelConfig() out of the loop by calling it once before the while (shouldContinue) loop and storing its result in a local variable (e.g., const modelConfig = await resolveModelConfig()), then use that variable inside the loop instead of re-calling resolveModelConfig() each iteration; if hot-reload behavior is required, implement an explicit reload mechanism (e.g., a reloadModelConfig() trigger) rather than implicitly re-invoking resolveModelConfig() each loop.
🤖 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/lib/llm-judge.eval.ts`:
- Around line 1-2: The import uses the reserved identifier "eval" which can
break strict-mode tooling—rename the imported symbol to a non-reserved name
(e.g., change "eval" to "runEval" in the import list alongside generateCommand,
models, ModelConfig, and createLLMJudge) and update all usages of that symbol
(for example the call currently using eval(...) to runEval(...), such as the
"Command Generation Quality Assessment" invocation) so references match the new
identifier; alternatively, change the exported name in the source module (export
from src/lib/eval.ts) and keep consumers consistent.
In `@src/utils.ts`:
- Around line 17-19: Remove the duplicate buffered stderr logging in the exec
callback: since you already stream stderr via childProcess.stderr?.on('data'),
delete the exec callback branch that does console.error(`stderr: ${stderr}`)
(the buffered stderr output) so stderr is only emitted in real time; locate the
exec callback in src/utils.ts that reads the stderr variable and remove or omit
that console.error while keeping the existing streaming listener on
childProcess.stderr.
- Around line 22-23: The call that writes shell history (appendToShellHistory /
underlying fs.appendFileSync) can throw and currently runs synchronously inside
runCommand, which may abort an already-spawned process; change
appendToShellHistory to perform the write in a non-throwing way: either use
fs.appendFile (async) or wrap fs.appendFileSync in a try/catch and swallow or
log the error (do not rethrow), ensuring runCommand continues regardless of
history-write failures; update runCommand to call the new safe
appendToShellHistory variant so a history error never interrupts command
execution.
---
Outside diff comments:
In `@src/lib/eval-improvements.ts`:
- Line 529: The file exports runEnhancedEval under the alias eval which can
trigger strict-mode/identifier conflicts for consumers; change the export to
avoid using the reserved identifier by either exporting runEnhancedEval directly
(remove "export { runEnhancedEval as eval }") or pick a non-reserved alias
(e.g., export { runEnhancedEval as enhancedEval }) so references use
runEnhancedEval or the new alias instead of eval; update any imports that expect
eval to the new name.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 67-69: Hoist resolveModelConfig() out of the loop by calling it
once before the while (shouldContinue) loop and storing its result in a local
variable (e.g., const modelConfig = await resolveModelConfig()), then use that
variable inside the loop instead of re-calling resolveModelConfig() each
iteration; if hot-reload behavior is required, implement an explicit reload
mechanism (e.g., a reloadModelConfig() trigger) rather than implicitly
re-invoking resolveModelConfig() each loop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5567fd9e-2d2d-49a3-886c-ddc22fac39eb
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
src/bench_models.tssrc/commands/config.tssrc/helpers/hardware.tssrc/index.tssrc/lib/ai.tssrc/lib/basic.eval.tssrc/lib/ci-eval.tssrc/lib/config.tssrc/lib/enhanced-eval-example.tssrc/lib/eval-improvements.tssrc/lib/eval.tssrc/lib/example.eval.tssrc/lib/llm-judge.eval.tssrc/test-improved-prompt.tssrc/utils.ts
💤 Files with no reviewable changes (1)
- src/commands/config.ts
| import { generateCommand, models, ModelConfig } from './ai'; | ||
| import { eval, createLLMJudge } from './eval'; |
There was a problem hiding this comment.
Avoid using eval as an identifier — it's reserved in strict mode.
Biome correctly flags this as an error. While this may work at runtime due to how the import is aliased, eval is a reserved identifier in JavaScript strict mode (ES5+), and using it can cause issues with static analysis tools, bundlers, and future compatibility.
Consider renaming the import to avoid shadowing the built-in:
🔧 Proposed fix
-import { eval, createLLMJudge } from './eval';
+import { eval as runEval, createLLMJudge } from './eval';Then update the usage on line 14:
await runEval('Command Generation Quality Assessment', {Alternatively, update the export in src/lib/eval.ts line 307 to use a different name, which would fix this across all consumers.
🧰 Tools
🪛 Biome (2.4.7)
[error] 2-2: Illegal use of eval as an identifier in strict mode
(parse)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/llm-judge.eval.ts` around lines 1 - 2, The import uses the reserved
identifier "eval" which can break strict-mode tooling—rename the imported symbol
to a non-reserved name (e.g., change "eval" to "runEval" in the import list
alongside generateCommand, models, ModelConfig, and createLLMJudge) and update
all usages of that symbol (for example the call currently using eval(...) to
runEval(...), such as the "Command Generation Quality Assessment" invocation) so
references match the new identifier; alternatively, change the exported name in
the source module (export from src/lib/eval.ts) and keep consumers consistent.
| if (stderr) { | ||
| console.error(`stderr: ${stderr}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify both stderr paths exist in the same function.
rg -n -C2 'exec\(command|stderr\)|childProcess\.stderr|process\.stderr\.write' src/utils.tsRepository: bernoussama/lazyshell
Length of output: 432
Remove duplicate stderr reporting path.
The function streams stderr in real-time at line 28–29 using childProcess.stderr?.on('data'), but also prints buffered stderr in the exec callback at lines 17–19. This causes stderr to be output twice for a single command execution.
Consider removing the buffered stderr output (lines 17–19) and keeping the streaming approach for better real-time feedback:
Proposed fix
- const childProcess = exec(command, (error, stdout, stderr) => {
+ const childProcess = exec(command, error => {
if (error) {
console.error(`Error: ${error.message}`);
return;
}
-
- if (stderr) {
- console.error(`stderr: ${stderr}`);
- }
});📝 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.
| if (stderr) { | |
| console.error(`stderr: ${stderr}`); | |
| } | |
| const childProcess = exec(command, error => { | |
| if (error) { | |
| console.error(`Error: ${error.message}`); | |
| return; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils.ts` around lines 17 - 19, Remove the duplicate buffered stderr
logging in the exec callback: since you already stream stderr via
childProcess.stderr?.on('data'), delete the exec callback branch that does
console.error(`stderr: ${stderr}`) (the buffered stderr output) so stderr is
only emitted in real time; locate the exec callback in src/utils.ts that reads
the stderr variable and remove or omit that console.error while keeping the
existing streaming listener on childProcess.stderr.
| appendToShellHistory(command); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unguarded sync history writes in current branch.
rg -n -C3 'appendToShellHistory|appendFileSync|try\s*\{' src/utils.tsRepository: bernoussama/lazyshell
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Get the full runCommand and appendToShellHistory functions with context
sed -n '1,60p' src/utils.ts | cat -nRepository: bernoussama/lazyshell
Length of output: 1978
Handle history write failures without interrupting command execution.
At Line 49, fs.appendFileSync(...) is unguarded and called synchronously from runCommand at Line 22. If it throws (permissions, read-only filesystem, missing home directory), the entire runCommand execution fails after the process has already been spawned, which is a poor failure mode for CLI execution.
💡 Proposed fix
function appendToShellHistory(command: string) {
const shell = process.env.SHELL?.split('/').pop() || '';
let historyFilePath: string;
let entry: string;
@@
- fs.appendFileSync(historyFilePath, entry);
+ try {
+ fs.appendFileSync(historyFilePath, entry);
+ } catch (error) {
+ // History logging should never break command execution
+ const message = error instanceof Error ? error.message : String(error);
+ process.stderr.write(`Warning: failed to append shell history (${message})\n`);
+ }
}📝 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.
| appendToShellHistory(command); | |
| function appendToShellHistory(command: string) { | |
| const shell = process.env.SHELL?.split('/').pop() || ''; | |
| let historyFilePath: string; | |
| let entry: string; | |
| // ... (initialization code for historyFilePath and entry) | |
| try { | |
| fs.appendFileSync(historyFilePath, entry); | |
| } catch (error) { | |
| // History logging should never break command execution | |
| const message = error instanceof Error ? error.message : String(error); | |
| process.stderr.write(`Warning: failed to append shell history (${message})\n`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils.ts` around lines 22 - 23, The call that writes shell history
(appendToShellHistory / underlying fs.appendFileSync) can throw and currently
runs synchronously inside runCommand, which may abort an already-spawned
process; change appendToShellHistory to perform the write in a non-throwing way:
either use fs.appendFile (async) or wrap fs.appendFileSync in a try/catch and
swallow or log the error (do not rethrow), ensuring runCommand continues
regardless of history-write failures; update runCommand to call the new safe
appendToShellHistory variant so a history error never interrupts command
execution.
Summary
Refactors and simplifies the codebase by removing dead code, eliminating duplication, cleaning up commented-out code, and improving overall readability. 277 net lines removed across 16 files with zero functional changes.
Changes
Core CLI (
src/index.ts)genCommand()function (was entirely duplicated inline in the action handler)editPrompt()function (was commented out in action handler)resolveModelConfig()helperAI Module (
src/lib/ai.ts)google,ollama,openai,anthropic,createOpenAICompatible) — the provider registry handles model creationgetDefaultModelId()function (default models already defined in the provider registry)getDefaultModel()with a data-driven env var lookup instead of an if/else chaingenerateTextWithModel()using spread syntax instead of manual conditional property assignmentProviderRegistryfrom imports (unused in this module)Config (
src/lib/config.ts)iscancelled()function (typo in name, was unimplemented, never called)typeof x === 'symbol'check with the standardisCancel()patternawaiton return valuesgetEffectiveApiKey()by removing unnecessary commentsHardware Helpers (
src/helpers/hardware.ts)getDetailedHardwareInfo()— it was byte-for-byte identical togetHardwareInfo()console.warnin catch with silent fallthroughUtils (
src/utils.ts)appendToShellHistory()helper\${msg}`→msg`)Eval System (
src/lib/eval.ts,src/lib/eval-improvements.ts)evaluateGeneration()function and its associated legacy Zod schemasbatchEvaluateGenerationsblocklevenshteinDistanceandwithRetryfromeval.tseval-improvements.tswith imports fromeval.tsConfig UI (
src/commands/config.ts)console.loglinesLint & Import Cleanup (across all files)
basic.eval.ts,ci-eval.ts,llm-judge.eval.ts,enhanced-eval-example.ts,bench_models.ts,test-improved-prompt.tsawaiton return values inexample.eval.ts_prefix conventionVerification
bun run typecheck— passes (0 errors)bun run lint— passes (0 errors, 0 warnings)bun run build— passes (bundles successfully)Summary by CodeRabbit
Release Notes
Refactor
Chores
Note: The command edit feature has been removed from the main workflow.