feat: PR-7 add business logic handlers and CLI entry point#7
Merged
mynameistito merged 9 commits intomainfrom Mar 29, 2026
Merged
feat: PR-7 add business logic handlers and CLI entry point#7mynameistito merged 9 commits intomainfrom
mynameistito merged 9 commits intomainfrom
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:
📝 WalkthroughWalkthroughAdds four new CLI handler modules (export, presets, search, settings), refactors the CLI entrypoint into interactive and non-interactive flows, changes AppState shape, and updates the typecheck script. All changes are implementation additions — no deletions of existing handler files. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as index.ts
participant SearchHandler as search.ts
participant SearchSvc as searchAllMessages
participant Collate as collateResults
participant Export as exportNonInteractive
User->>CLI: invoke non-interactive search
CLI->>CLI: resolve token & guild
CLI->>SearchHandler: executeNonInteractiveSearch(params, token)
SearchHandler->>SearchSvc: searchAllMessages (pages + progress)
SearchSvc-->>SearchHandler: page results
SearchHandler->>Collate: collateResults
Collate-->>SearchHandler: aggregated data
alt export requested
SearchHandler->>Export: exportNonInteractive(data, guildId, format, outputDir)
Export-->>SearchHandler: output directory
SearchHandler-->>User: print export path to stderr
else json requested
SearchHandler-->>User: write JSON to stdout
else
SearchHandler-->>User: display summary to stderr
end
sequenceDiagram
participant User
participant CLI as index.ts
participant PresetHandler as presets.ts
participant SearchSvc as searchAllMessages
participant Collate as collateResults
participant Export as exportNonInteractive
User->>CLI: interactive -> run presets
CLI->>PresetHandler: handleRunAllPresets()
PresetHandler->>User: prompt presets & format
loop for each selected preset
PresetHandler->>SearchSvc: searchAllMessages(preset)
SearchSvc-->>PresetHandler: results
PresetHandler->>Collate: collateResults
Collate-->>PresetHandler: aggregated data
PresetHandler->>User: show summary
alt export chosen
PresetHandler->>Export: exportNonInteractive(...)
Export-->>PresetHandler: export path
PresetHandler->>User: report export path
end
end
PresetHandler-->>User: completion message
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:136">
P2: Check the Result from savePreset and report failures instead of always logging success; otherwise IO errors silently succeed and users believe the preset was saved.</violation>
</file>
<file name="src/handlers/export.ts">
<violation number="1" location="src/handlers/export.ts:67">
P2: Validate the output directory prompt so empty input doesn't crash the export (mkdir("") throws).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
mynameistito
changed the title
mynameistito
changed the title
Owner
Author
|
@coderabbitai Please Review THIS PR. |
|
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai Please Review THIS PR. |
Owner
Author
|
@cubic-dev-ai @greptile |
|
✅ Actions performedFull review triggered. |
@mynameistito I have started the AI code review. It will take a few minutes to complete. |
Greptile SummaryThis PR adds the final layer of the CLI tool: four business-logic handlers (
Confidence Score: 4/5Safe to merge after fixing the hardcoded output path in One concrete P1 bug exists: src/handlers/presets.ts — Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant index.ts
participant handlers/search.ts
participant handlers/presets.ts
participant handlers/settings.ts
participant handlers/export.ts
participant discord/search.ts
participant export.ts
User->>index.ts: CLI invocation (argv)
index.ts->>index.ts: parseArgs()
alt interactive mode
index.ts->>index.ts: runInteractive()
index.ts->>handlers/settings.ts: resolveToken()
loop menu loop
index.ts->>User: select(action)
alt search / preset
index.ts->>handlers/presets.ts: resolveSearchParams()
handlers/presets.ts->>User: promptForSearchParams() / select preset
index.ts->>handlers/search.ts: executeSearch()
handlers/search.ts->>discord/search.ts: searchAllMessages()
discord/search.ts-->>handlers/search.ts: Result<Message[]>
handlers/search.ts->>User: displaySummary()
handlers/search.ts->>handlers/export.ts: handleExport()
handlers/export.ts->>export.ts: exportJson / exportMessagesCsv / …
else run-all presets
index.ts->>handlers/presets.ts: handleRunAllPresets()
loop each selected preset
handlers/presets.ts->>discord/search.ts: searchAllMessages()
handlers/presets.ts->>export.ts: exportJson / exportMessagesCsv / …
end
else settings
index.ts->>handlers/settings.ts: handleSettings()
end
end
else non-interactive mode
index.ts->>index.ts: runNonInteractive()
alt search command
index.ts->>handlers/search.ts: executeNonInteractiveSearch()
handlers/search.ts->>discord/search.ts: searchAllMessages()
handlers/search.ts->>handlers/export.ts: exportNonInteractive()
handlers/export.ts->>export.ts: exportJson / exportMessagesCsv / …
handlers/search.ts->>User: stdout (JSON or summary)
else preset command
index.ts->>handlers/presets.ts: runPresetNonInteractive() / runAllPresetsNonInteractive()
handlers/presets.ts->>handlers/search.ts: executeNonInteractiveSearch()
else settings command
index.ts->>handlers/settings.ts: showSettingsNonInteractive / setSettingNonInteractive / showInviteLinkNonInteractive
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/handlers/export.ts">
<violation number="1" location="src/handlers/export.ts:24">
P2: Reject unsupported non-interactive export formats; current logic can report a successful export while creating no files.</violation>
<violation number="2" location="src/handlers/export.ts:67">
P2: Validate the interactive output directory input before trimming; whitespace-only input currently crashes on mkdir with ENOENT.</violation>
</file>
<file name="src/handlers/search.ts">
<violation number="1" location="src/handlers/search.ts:221">
P2: Keep `--json` output schema consistent for empty and non-empty results. The empty-results branch currently emits a different object shape than `collateResults`, which can break downstream parsers.</violation>
</file>
<file name="src/handlers/presets.ts">
<violation number="1" location="src/handlers/presets.ts:53">
P2: Check the delete result before logging success; currently the interactive path can claim a preset was deleted when deletion failed.</violation>
<violation number="2" location="src/handlers/presets.ts:100">
P2: Validate preset names against path characters/traversal patterns before saving; current validation allows unsafe file path segments.</violation>
<violation number="3" location="src/handlers/presets.ts:107">
P2: Handle `savePreset` errors in the interactive flow; otherwise users can see a false “Saved preset” message.</violation>
</file>
<file name="src/handlers/settings.ts">
<violation number="1" location="src/handlers/settings.ts:126">
P2: Saving a prompted token here can wipe previously saved clientId/default guild values. Persist only the token key instead of building AppState with undefined fields.</violation>
</file>
<file name="src/index.ts">
<violation number="1" location="src/index.ts:1">
P1: The entrypoint declares Node as the runtime, but it calls Bun APIs; running it via shebang will crash with `Bun is not defined`.</violation>
<violation number="2" location="src/index.ts:264">
P2: Help is checked too late: parsing runs first, so `search --help` can error on required arg validation instead of printing help.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Add search execution with live view, preset management (CRUD and bulk run), settings management, export orchestration, and the main entry point that wires interactive and non-interactive CLI flows.
mynameistito
force-pushed
the
init/07-handlers-entry
branch
from
a8cdc86 to
0a15270
Compare
…OMMAND_HELP Co-authored-by: Amp <amp@ampcode.com>
Co-authored-by: Amp <amp@ampcode.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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/handlers/export.ts`:
- Around line 81-96: The export code in handleExport collects promises in
exports and awaits Promise.all(exports) but doesn't check for failures, so the
spinner may report success even if some exports failed; wrap the await in a
try/catch (or use Promise.allSettled and inspect results) and on any failed
export call spinner.fail with a descriptive message and propagate or return an
error status instead of spinner.succeed; update the code around Promise handling
in src/handlers/export.ts (the exports array and the await Promise.all(exports)
block) to detect rejections from
exportJson/exportMessagesCsv/exportEmbedsCsv/exportFieldsCsv and handle them
accordingly.
- Around line 47-57: Interactive export format values in handleExport do not
match the strings expected by exportNonInteractive (handleExport uses
"messages","embeds","fields" while exportNonInteractive expects
"csv-messages","csv-embeds","csv-fields"); update the select options in
handleExport to use the "csv-*" values (e.g.,
"csv-messages","csv-embeds","csv-fields") so they match exportNonInteractive,
and then adjust any conditional checks in handleExport that branch on those
values (references: handleExport, exportNonInteractive) to use the aligned
strings.
- Around line 24-40: The export promises are awaited without checking their
Result<void, ExportError> outcomes, so failures are swallowed; change the
exports array to Promise<Result<void, ExportError>>[], await them into a results
array (const results = await Promise.all(exports)), then iterate results and
detect any error Result (e.g., Result.isErr(res) or res.isErr() / checking for
an 'error'/'err' property depending on your Result implementation) and either
throw or return the first ExportError (or aggregate them) so the caller sees the
failure; update the handler that calls exportJson, exportMessagesCsv,
exportEmbedsCsv, exportFieldsCsv to perform this check before returning dir.
In `@src/handlers/presets.ts`:
- Around line 350-352: The hardcoded "./output/" path in
runAllPresetsNonInteractive should be replaced with the project's canonical
OUTPUT_DIR from paths.ts; update the code that builds dir (which currently uses
options.outputDir, preset.name and timestamp) to fall back to
`${OUTPUT_DIR}/${preset.name}-${timestamp}` when options.outputDir is not
provided, and add the import for OUTPUT_DIR from paths.ts so the function uses
the shared path constant.
- Around line 124-128: The inline progress callback passed to searchAllMessages
(used in presets.ts) currently has implicit any parameters and may not match the
expected ProgressCallback signature; fix by importing or referencing the
ProgressCallback type and annotate the callback explicitly (e.g., use (fetched:
number, total: number) => void or annotate the function as ProgressCallback) so
the parameters are strongly typed and match the expected signature, and apply
the same change for the other occurrence around the s.message call so both
callbacks conform to ProgressCallback.
- Around line 158-159: Replace the hardcoded "./output" usage by importing and
using OUTPUT_DIR from paths.ts: add "import { OUTPUT_DIR } from '@/paths.ts';"
at the top, then change the directory construction from "const dir =
`./output/${name}-${timestamp}`;" to "const dir =
`${OUTPUT_DIR}/${name}-${timestamp}`;" (keep the existing Bun.write call to
create ".gitkeep"). This ensures all file I/O uses the centralized OUTPUT_DIR
constant.
In `@src/handlers/search.ts`:
- Around line 219-224: When messages.length === 0 and options.json is true,
return the same rich JSON shape as when results exist by calling
collateResults([]) instead of writing a minimal object; modify the zero-case
branch in the handler (the block that checks messages.length === 0) to invoke
collateResults with an empty array and write its JSON output to process.stdout
(preserving the newline) so the JSON structure matches the non-empty path.
- Around line 188-196: The progress callback passed to searchAllMessages uses a
two-arg signature but must match the ProgressCallback type; update the anonymous
callback used in the call to searchAllMessages so its parameter list and usage
follow the ProgressCallback definition (e.g., accept the single progress object
or the exact tuple defined by ProgressCallback), extract fetched and total from
that parameter, and then write to process.stderr using those extracted values;
refer to the searchAllMessages invocation and the ProgressCallback type to align
the parameter names and shape exactly.
In `@src/handlers/settings.ts`:
- Around line 34-78: The function handleSettingsAction uses multiple separate if
blocks for mutually exclusive choices; refactor it to use else if chains or a
switch(action) so only one branch executes (e.g., replace the sequential
if(action === "...") checks with else if or a switch on action), keeping
existing behavior for "token", "client-id", "guild", "invite" and "save", and
preserving calls to password/text, handleCancel, state assignments, log calls,
and the early return when clientId is missing in the "invite" branch.
- Around line 138-152: Change resolveTokenNonInteractive to return a Result
instead of calling process.exit(1): update the signature to something like
resolveTokenNonInteractive(...): Result<string, Error> (or your project's
Result<E, V> shape), return Ok(cliToken) or Ok(configResult.value.token) when
present, and return Err(new Error("No token found. Provide --token or set
DISCORD_BOT_TOKEN.")) when missing; remove the process.exit and stderr.write,
and update any callers of resolveTokenNonInteractive to handle the Err case
(propagate or print/exit there). Use the existing loadConfig and configResult
checks as-is to decide which Ok value to produce.
In `@src/index.ts`:
- Around line 266-270: SUBCOMMAND_HELP is typed too narrowly causing TS7053 when
indexing with cliArgs.command (which can be 'help' or 'interactive'); update the
code so the access is type-safe by either widening SUBCOMMAND_HELP's type to
include 'help' and 'interactive' (or make it Record<string,string>) or narrow
cliArgs.command before indexing (e.g., check if cliArgs.command is one of the
SUBCOMMAND_HELP keys) and then use SUBCOMMAND_HELP[cliArgs.command]; ensure the
fallback to HELP_TEXT remains (console.log(subHelp ?? HELP_TEXT)) and reference
SUBCOMMAND_HELP, cliArgs.command, and HELP_TEXT when making the change.
- Around line 176-185: buildStateFromConfig currently returns token: "" when
configResult.isErr() and no cliToken is passed, which can cause silent failures;
change it to return token: undefined in that case (i.e., set token =
configResult.isOk() ? configResult.value.token : cliToken ?? undefined) so
missing tokens are explicit, update any dependent types if necessary, and verify
callers like handleSettingsShowCommand and handleSettingsSetCommand handle
undefined tokens appropriately.
- Around line 272-278: The current call to Bun.file(...).json() assigns an
untyped pkg and uses any; replace that with a typed approach by defining a
PackageJson interface (with at least version: string) or cast the json result to
unknown and perform type-narrowing, then assign to pkg typed as PackageJson
before using pkg.version; update the block that checks cliArgs.version to use
the validated/typed pkg (referencing cliArgs.version, Bun.file(...).json(), and
the pkg variable) so TypeScript no longer relies on any.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8940d1d8-eb02-4484-a063-cd650c3a81a3
📒 Files selected for processing (5)
src/handlers/export.tssrc/handlers/presets.tssrc/handlers/search.tssrc/handlers/settings.tssrc/index.ts
- Check export Result outcomes instead of swallowing errors - Align interactive export format values to csv-* naming - Replace hardcoded ./output/ with OUTPUT_DIR in presets - Use collateResults([]) for consistent zero-results JSON shape - Return Result from resolveTokenNonInteractive instead of process.exit - Convert handleSettingsAction to else-if chain - Make AppState.token explicitly undefined when missing - Type pkg from package.json read Co-authored-by: Amp <amp@ampcode.com>
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/index.ts`:
- Line 290: The unawaited Promise returned by main() should have rejection
handling; replace the bare call to main() with a call that catches errors from
main() (e.g., main().catch(...)) and surface the error via your logger or
console.error and exit with a non-zero code. Locate the top-level invocation
main() in src/index.ts and wrap it so uncaught errors from the async function
are logged (include the error object) and the process exits (process.exit(1)) to
avoid silent failures.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 610832b4-585a-4ec9-821b-fa5523a35cec
📒 Files selected for processing (5)
package.jsonsrc/handlers/presets.tssrc/handlers/search.tssrc/index.tssrc/types.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/index.ts (1)
304-304: 🧹 Nitpick | 🔵 TrivialAdd error handling for the top-level
main()call.The
main()function returns a Promise that isn't handled. Unhandled rejections could silently fail. Adding a.catch()ensures errors are properly surfaced to the user.♻️ Proposed fix
-main(); +main().catch((err) => { + console.error(err); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` at line 304, The top-level call main() is unhandled; wrap it to catch rejected Promises by appending a .catch handler on the main() invocation (the top-level main() call) that logs the error (e.g., console.error or your processLogger) and exits with a non-zero status (process.exit(1)) so failures are surfaced; ensure the handler prints the error stack or message for debugging.
🤖 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/handlers/presets.ts`:
- Around line 52-56: The code calls deletePreset(name) but ignores its Result;
update the handler to await deletePreset(name), inspect the returned
Result<void, PresetError> (e.g., check isOk/isErr or match on the Result), log
success with log.success only when the result is ok, and on error call log.error
(or appropriate error logger) with the PresetError details so users aren’t shown
a success message when deletePreset fails; reference the variables/methods
presetAction, name, deletePreset, and log.success/log.error when making the
change.
- Around line 107-110: The code calls savePreset(savedName, params) but ignores
its Result<void, PresetError>; change to capture the returned result (e.g.,
const result = await savePreset(savedName, params)) and check it before logging
success: if the result indicates failure (use the Result API used in the repo,
e.g., result.isErr() or result.ok === false), log or throw an appropriate error
using the PresetError info and do not call log.success; only call
log.success(`Saved preset: ${savedName}`) when the result is successful. Ensure
you reference savePreset, savedName, params, and log.success when making the
fix.
- Around line 162-172: The exports array is currently typed as
Promise<unknown>[] and its results aren’t checked; change its type to
Promise<Result<void, ExportError>>[] and push the calls to exportJson,
exportMessagesCsv, exportEmbedsCsv, exportFieldsCsv into that array, then await
Promise.all(exports) to get an array of Result<void, ExportError> values and
iterate over them to detect and handle any Err results (matching the error
handling pattern used in exportNonInteractive and handleExport) — return or
propagate the first ExportError or log/format errors consistently with those
functions instead of ignoring the results.
In `@src/index.ts`:
- Around line 59-72: The code casts state.token as string when calling
handleRunAllPresets and executeSearch which can mask missing-token bugs; either
narrow the token type before casting by validating that state.token is a
non-empty string (e.g., if (!state.token) return or throw) and pass that
validated token into handleRunAllPresets(state.token) and executeSearch(...,
state.token), or change the handleMenuAction signature to require token: string
and update callers accordingly (ensure resolveSearchParams, handleSettings and
other callers are updated to supply the non-undefined token); make sure to fail
fast with a clear error if token is absent.
---
Duplicate comments:
In `@src/index.ts`:
- Line 304: The top-level call main() is unhandled; wrap it to catch rejected
Promises by appending a .catch handler on the main() invocation (the top-level
main() call) that logs the error (e.g., console.error or your processLogger) and
exits with a non-zero status (process.exit(1)) so failures are surfaced; ensure
the handler prints the error stack or message for debugging.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7792363a-cfa6-4f2c-a321-62da5539d198
📒 Files selected for processing (6)
src/handlers/export.tssrc/handlers/presets.tssrc/handlers/search.tssrc/handlers/settings.tssrc/index.tssrc/types.ts
…ttings save failures, reject --json on preset run-all Co-authored-by: Amp <amp@ampcode.com>
…ts, handle top-level main() rejection Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:49">
P1: Using a captured `token` parameter here makes interactive searches/preset runs ignore token updates made in Settings. Read the current token from `state` when executing actions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/handlers/presets.ts (1)
55-58:⚠️ Potential issue | 🟡 MinorCheck the preset mutation result before logging success.
These branches await a mutating call and then immediately emit
log.success(...). IfdeletePreset()orsavePreset()fails, the UI still reports success. Inspect the returnedResultfirst and handle the error path instead.Also applies to: 110-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/presets.ts` around lines 55 - 58, The code currently calls deletePreset(name) and savePreset(...) and unconditionally logs success; instead, inspect the returned Result from deletePreset and savePreset (e.g., capture the promise result variable) and check whether it indicates success or an error before calling log.success; on failure call log.error (or log.warn) with the returned error details and avoid the success message. Update both the delete branch (function deletePreset) and the save branch (function savePreset) to handle the Result error path and include the error info in the log output.
🤖 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/handlers/presets.ts`:
- Around line 162-163: User-supplied preset names are used directly to build
filesystem paths (dir = `${OUTPUT_DIR}/${name}-${timestamp}`), which allows path
traversal or nested dirs; add a sanitize/validate step before composing dir:
implement a helper (e.g., sanitizePresetName) in presets.ts that rejects names
containing path separators or ".." or control characters, or normalizes them by
replacing unsafe characters with a safe char (e.g., hyphen), enforce a non-empty
and length-limited result, and use that sanitized value instead of name wherever
dir is built (apply the same fix to the other occurrence around lines 355-357).
- Around line 37-40: Split the error and empty cases when handling
loadPresets(): first check presetsResult.isErr() and surface the actual failure
(e.g., processLogger.error or throw the error returned by presetsResult.error)
instead of treating it as an empty state, then separately handle
presetsResult.value.length === 0 as the “no presets” path; apply the same change
to the second occurrence (the block around resolveSearchParams) so
corrupt/unreadable preset storage does not fall through into the new-search flow
and the real error is logged/surfaced.
In `@src/handlers/settings.ts`:
- Around line 165-191: setSettingNonInteractive currently writes empty strings
into state which then get dropped by saveStateToSettings/saveSettings (since
they only persist truthy values), causing attempts to clear keys to fail; update
setSettingNonInteractive to normalize empty string inputs to undefined (same
behavior as the interactive path) before assigning to state.token,
state.clientId, or state.defaultGuildId, or alternatively validate and reject
empty strings—make the change where setSettingNonInteractive assigns values and
ensure saveStateToSettings receives undefined to clear a setting rather than an
empty string.
---
Duplicate comments:
In `@src/handlers/presets.ts`:
- Around line 55-58: The code currently calls deletePreset(name) and
savePreset(...) and unconditionally logs success; instead, inspect the returned
Result from deletePreset and savePreset (e.g., capture the promise result
variable) and check whether it indicates success or an error before calling
log.success; on failure call log.error (or log.warn) with the returned error
details and avoid the success message. Update both the delete branch (function
deletePreset) and the save branch (function savePreset) to handle the Result
error path and include the error info in the log output.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4b71ad47-2b6c-4f74-9c05-8d288c6522cb
📒 Files selected for processing (4)
src/cli/args-parse.tssrc/cli/args-types.tssrc/handlers/presets.tssrc/handlers/settings.ts
💤 Files with no reviewable changes (1)
- src/cli/args-types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/handlers/presets.ts (1)
230-235:⚠️ Potential issue | 🟠 MajorDon't collapse preset-load failures into the empty-state path.
The condition
presetsResult.isErr() || presetsResult.value.length === 0treats load errors and empty presets identically, hiding actual failures (e.g., corrupt/unreadable preset storage) behind "No presets saved yet." This is inconsistent with the proper error handling inhandleManagePresets(lines 69-76).🐛 Proposed fix
export const handleRunAllPresets = async (token: string): Promise<void> => { const presetsResult = await loadPresets(); - if (presetsResult.isErr() || presetsResult.value.length === 0) { + if (presetsResult.isErr()) { + log.error(`Failed to load presets: ${presetsResult.error.message}`); + return; + } + if (presetsResult.value.length === 0) { log.info("No presets saved yet."); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/presets.ts` around lines 230 - 235, Split the combined condition in handleRunAllPresets so load errors are handled separately from the empty-presets case: after calling loadPresets() check presetsResult.isErr() first and log or return the actual error (use the error/value fields on the Result object, similar to how handleManagePresets handles load failures), and only if load succeeds check presetsResult.value.length === 0 and log "No presets saved yet." or proceed; reference the loadPresets function and the handleRunAllPresets handler when making the change.
🤖 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/index.ts`:
- Around line 146-149: The savePreset call in the cliArgs.savePreset branch is
ignoring the Result returned by savePreset (which returns Promise<Result<void,
PresetError>>), so the code always prints "Saved preset" even on failure; update
the cliArgs.savePreset handling to await savePreset and inspect the Result (like
resolveSearchParams does): on Ok, print the success message; on Err, write the
error message to stderr (include preset name and error details from the
PresetError) and exit non-zero or return an error status. Reference: savePreset
and the cliArgs.savePreset branch in src/index.ts and mirror the pattern used in
resolveSearchParams in presets.ts.
---
Duplicate comments:
In `@src/handlers/presets.ts`:
- Around line 230-235: Split the combined condition in handleRunAllPresets so
load errors are handled separately from the empty-presets case: after calling
loadPresets() check presetsResult.isErr() first and log or return the actual
error (use the error/value fields on the Result object, similar to how
handleManagePresets handles load failures), and only if load succeeds check
presetsResult.value.length === 0 and log "No presets saved yet." or proceed;
reference the loadPresets function and the handleRunAllPresets handler when
making the change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3921d883-20cd-46fa-95b2-02f8234a65c4
📒 Files selected for processing (3)
src/handlers/presets.tssrc/handlers/settings.tssrc/index.ts
…ndleRunAllPresets
Owner
Author
|
@greptile, is there any blockers for merging this? |
|
No blockers! The PR is ready to merge: ✅ All checks passing (status: success) There are several automated review comments from cubic-dev-ai and CodeRabbit with suggestions for improvements (error handling, input validation, etc.), but these are non-blocking recommendations that could be addressed in follow-up PRs if desired. |
Owner
Author
|
@greptile is there anything that still needs to be fixed? do a deep anaylsis. |
Critical Issues1. Unhandled exception crashes bulk preset exportLocation: In Fix: Wrap in try-catch like the search error handling above it: if (options.export) {
try {
const safeName = sanitizePresetDirName(preset.name);
const dir = options.outputDir
? `${options.outputDir}/${safeName}`
: `${OUTPUT_DIR}/${safeName}-${timestamp}`;
await exportNonInteractive(collated, preset.params.guildId, options.export, dir);
process.stderr.write(` Exported to ${dir}/\n`);
} catch (err) {
process.stderr.write(` Export failed: ${err instanceof Error ? err.message : String(err)}\n`);
}
}2. Resource leak on search errorLocation: If Fix: Use try-finally: const cleanupKeys = createKeyListener((key) => { ... });
try {
const result = await searchAllMessages(...);
// handle result...
} finally {
cleanupKeys();
}Moderate Issues3. Path sanitization too aggressiveLocation: Rejects periods ( Suggestions:
4. Code duplication in export logicLocation: The format→export mapping appears twice. Extract to shared helper. 5. No format validation in exportNonInteractiveLocation: Accepts any string for 6. Unsafe type assertionsLocation: Using Minor Issues
Recommended Test Coverage
|
Wrap exportNonInteractive in try-catch during bulk preset runs so a failed export logs the error and continues to the next preset instead of crashing. Add upfront format validation in exportNonInteractive to reject invalid format strings with a descriptive error.
4 tasks
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
Stacked PR Chain
This is PR 7 of 7 — merge in order.
main← PR1 ← PR2 ← PR3 ← PR4 ← PR5 ← PR6 ← PR7Test plan
bun run typecheckpassesbun run checkpasses lintingbun run buildproduces working binarysearchcommand executesSummary by cubic
Adds search, preset, settings, and export handlers plus the CLI entry point. Supports interactive and non-interactive flows for running searches, browsing results, exporting JSON/CSVs, and managing presets/settings.
New Features
preset runsupports--json; export dirs use sanitized preset names.--exportsupportsjson,csv-messages,csv-embeds,csv-fields, orall.interactivemenu and subcommandssearch(supports--json,--save-preset),preset(list/run/run-all/save/delete),settings(show/set/invite); progress streamed to stderr.Bug Fixes
--jsonforpreset run-all(use--export); usecollateResults([])for zero-result JSON; separate load-error handling in bulk runs to surface failures clearly.Resultfrom non-interactive token resolution; remove unsafe token casts; makeAppState.tokenexplicitly undefined when missing.OUTPUT_DIR; type thepackage.jsonversion read; fix types forAppState, progress callbacks, andSUBCOMMAND_HELP; handle top-levelmain()rejections; switch typecheck totsgo.Written for commit 90d81a3. Summary will update on new commits.