feat: PR-5 add CLI interactive components#5
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 a runnable CLI entrypoint, interactive prompt flows, a TTY message browser with raw-mode key handling, extended global CLI flags, updated argument parsing and tests, refined Discord rate-limit bucket propagation, pagination accumulation fixes, and a schema limit change. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Router
participant Prompts as Prompt System
participant Terminal as Terminal/TTY
participant Discord as Discord API
participant Browser as Message Browser
User->>CLI: invoke CLI (interactive / search / --help / --version)
CLI->>Prompts: promptForToken / promptForSearchParams
Prompts->>Terminal: render prompts
Terminal->>Prompts: user input / cancel
Prompts->>CLI: return SearchParams
CLI->>Discord: discordFetch (paginated)
Discord->>Discord: response + X-RateLimit-Bucket -> remapped key
Discord->>CLI: return messages (pages)
CLI->>Browser: browseMessages(messages)
Browser->>Terminal: render preview / json
Terminal->>Browser: key events (j/k/v/q, arrows)
Browser->>Browser: update view/state
Browser->>Terminal: re-render or exit
Browser->>User: cleanup & resolve
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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.
1 issue found across 3 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/cli/keys.ts">
<violation number="1" location="src/cli/keys.ts:18">
P2: Restore stdin state before exiting on Ctrl+C so the terminal isn’t left in raw mode.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
mynameistito
changed the title
|
@CodeRabbit, please do a deep anaylsis on this PR and tell me any issues that need to be resolved even nit picky ones. |
|
@greptile @cubic-dev-ai |
@mynameistito I have started the AI code review. It will take a few minutes to complete. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the full PR details and file contents for a thorough analysis. 🏁 Script executed: Length of output: 58004
🔴 Bugs / Correctness Issues1.
|
| # | File | Severity | Issue |
|---|---|---|---|
| 1 | args-types.ts |
🔴 Bug | Duplicate discriminator values crash ParsedArgsSchema |
| 2 | discord/client.ts |
🔴 Bug | handle202 retries don't handle non-202/non-OK responses |
| 3 | args-parse.ts |
🔴 Bug | Flags without values silently no-op |
| 4 | args-parse.ts |
🔴 Bug | parseInt NaN not validated for --slop/--offset/--limit |
| 5 | keys.ts |
🔴 Bug | Ctrl+C exits without calling cleanup() → raw mode left on |
| 6 | types.ts |
🟡 Design | AppState is a duplicate of Config |
| 7 | discord/client.ts |
🟡 Design | Module-level rate limit singleton unsafe for concurrent use |
| 8 | paths.ts |
🟡 Design | OUTPUT_DIR defined but never created by ensureAppDir |
| 9 | args-help.ts |
🟡 Design | --guild / --client-id global flags not in help text |
| 10 | discord/search.ts |
🟡 Design | totalResults stale on snowflake partition boundaries |
| 11 | prompts.ts |
🔵 Nit | handleCancel type predicate always false |
| 12 | keys.ts |
🔵 Nit | stdin.setEncoding not restored on cleanup |
| 13 | browser.ts |
🔵 Nit | Empty messages array resolves silently |
| 14 | prompts.ts |
🔵 Nit | "none" sentinel in authorType select |
| 15 | prompts.ts |
🔵 Nit | No empty-check validation in promptForToken |
🧠 Learnings used
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:43.140Z
Learning: Applies to src/handlers/**/handlers/**/*.ts : Use clack/prompts with handleCancel wrapper for user interactions
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T14:56:15.999Z
Learning: Applies to src/handlers/**/*.ts : Use clack/prompts with handleCancel wrapper for user interactions in handlers
There was a problem hiding this comment.
5 issues found across 14 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/discord/client.ts">
<violation number="1" location="src/discord/client.ts:150">
P1: The 202-retry loop doesn't handle error responses (429, 4xx, 5xx). If a retry gets a 429 or server error, it will be fed to `parseResponse` and fail with a misleading `ValidationError` instead of the correct error type. Mirror the status handling from `discordFetch` (check for 429, non-OK) before calling `parseResponse`.</violation>
</file>
<file name="src/config.ts">
<violation number="1" location="src/config.ts:67">
P1: Settings are written with the bot token but without enforcing restrictive file permissions, which can expose credentials on multi-user systems.</violation>
</file>
<file name="src/cli/browser.ts">
<violation number="1" location="src/cli/browser.ts:102">
P1: Handle non-TTY mode before starting the interactive browser; otherwise this promise can hang forever when key input is unavailable.</violation>
</file>
<file name="src/discord/search.ts">
<violation number="1" location="src/discord/search.ts:130">
P1: Non-zero `offset` causes pagination to stop after the first page because `totalResults` is never initialized.</violation>
</file>
<file name="src/cli/args-parse.ts">
<violation number="1" location="src/cli/args-parse.ts:258">
P2: `preset run-all --export json` will incorrectly treat `json` (or `--output-dir` values) as preset names because the loop doesn’t skip flag values. Skip the next arg when consuming output flags to avoid corrupting `names`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR wires up a working CLI entrypoint (help/version), adds Confidence Score: 4/5Safe to merge after addressing the non-TTY hang in One P1 issue:
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["process.argv"] --> B["parseArgs()"]
B --> C{command?}
C -- "--help / no subcommand + --help" --> D["printHelp()"]
C -- "--version" --> E["printVersion()"]
C -- "interactive (no subcommand)" --> F["[stub] exit 1"]
C -- "search --guild ..." --> G["[stub] exit 1"]
C -- "preset / settings" --> H["[stub] exit 1"]
subgraph "Interactive flow (future)"
I["promptForToken()"] --> J["promptForSearchParams()"]
J --> K["searchAllMessages()"]
K --> L["browseMessages()"]
L --> M{stdin TTY?}
M -- "yes" --> N["createKeyListener()\nj/k/v/q navigation"]
M -- "no" --> O["⚠️ Promise never resolves"]
end
Reviews (2): Last reviewed commit: "fix: correct NOTE admonition syntax, tra..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
github-actions
Bot
added
documentation
There was a problem hiding this comment.
4 issues found across 43 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:8">
P1: The configured CLI entrypoint is currently a no-op, so the built `discord-search` command does not run the application.</violation>
</file>
<file name="src/discord/search.ts">
<violation number="1" location="src/discord/search.ts:168">
P1: Updating `totalResults` on every page breaks snowflake partitioning. When a subsequent partition adds a `max_id` filter, Discord returns a smaller `total_results` for that filtered scope. Overwriting `state.totalResults` with this smaller value causes the `allMessages.length >= totalResults` check to fire immediately, terminating pagination early and losing messages beyond the first partition.
Only capture `totalResults` from the initial unfiltered response, or use a monotonically non-decreasing update to prevent the total from shrinking across partitions.</violation>
</file>
<file name="src/discord/client.ts">
<violation number="1" location="src/discord/client.ts:334">
P1: Stale `bucketKey` after Discord remaps the rate-limit bucket. `updateRateLimitState` deletes the old key from `rateLimitBuckets` and stores state under a new `discord:*` key, but this `const` never updates. On 429 retries (and inside `handle202`), `getBucketState` creates a fresh state instead of using the remapped one, silently losing rate-limit tracking and risking additional 429s.</violation>
</file>
<file name="src/cli/keys.ts">
<violation number="1" location="src/cli/keys.ts:34">
P2: Cleanup does not restore stdin encoding when the original encoding was null, so this listener can leave stdin permanently in UTF-8 mode.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Add @clack/prompts wrappers for user input, interactive message browser with keyboard navigation, and TTY key listener for live view toggle.
- Fix Ctrl+C exiting without restoring terminal state (keys.ts) - Restore stdin encoding on cleanup (keys.ts) - Document --guild and --client-id in global help options (args-help.ts) - Update totalResults on every page to fix stale progress on partitions (search.ts) - Fix handleCancel misleading type predicate (prompts.ts) - Replace "none" sentinel with confirm-first flow for author type (prompts.ts) - Add non-empty validation to token prompt (prompts.ts) - Show feedback when browse has no messages (browser.ts) - Add doc comment for shared rate limit state (client.ts)
mynameistito
force-pushed
the
init/05-cli-interactive
branch
from
f8d91e2 to
98e622d
Compare
github-actions
Bot
removed
documentation
… stdin cleanup - Replace no-op index.ts with working CLI entrypoint that parses args and dispatches commands - Only capture totalResults from first unfiltered response to prevent early pagination termination - Return remapped bucket key from updateRateLimitState so callers track the active key across retries - Use strict null check for stdin encoding restore to avoid leaving stdin in UTF-8 mode
There was a problem hiding this comment.
1 issue found across 4 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:30">
P2: Handle `--help` for interactive/no-command mode before returning the unimplemented interactive error, otherwise `discord-search --help` fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (parsed.command === "interactive") { | ||
| process.stderr.write( | ||
| "Interactive mode is not yet implemented. Use 'discord-search search --help' for CLI usage.\n" | ||
| ); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
P2: Handle --help for interactive/no-command mode before returning the unimplemented interactive error, otherwise discord-search --help fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index.ts, line 30:
<comment>Handle `--help` for interactive/no-command mode before returning the unimplemented interactive error, otherwise `discord-search --help` fails.</comment>
<file context>
@@ -1,8 +1,55 @@
+ process.exit(0);
+ }
+
+ if (parsed.command === "interactive") {
+ process.stderr.write(
+ "Interactive mode is not yet implemented. Use 'discord-search search --help' for CLI usage.\n"
</file context>
| if (parsed.command === "interactive") { | |
| process.stderr.write( | |
| "Interactive mode is not yet implemented. Use 'discord-search search --help' for CLI usage.\n" | |
| ); | |
| process.exit(1); | |
| } | |
| if (parsed.command === "interactive") { | |
| if (parsed.help) { | |
| process.stdout.write(`${HELP_TEXT}\n`); | |
| process.exit(0); | |
| } | |
| process.stderr.write( | |
| "Interactive mode is not yet implemented. Use 'discord-search search --help' for CLI usage.\n" | |
| ); | |
| process.exit(1); | |
| } |
![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
🤖 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/cli/prompts.ts`:
- Around line 138-158: The prompt validators for limitInput and offsetInput
currently only check for NaN; update the validate callbacks (the inline validate
passed to text(...) for limitInput and offsetInput) to parse the trimmed value
to an integer and enforce the same bounds used by SearchParamsSchema: check that
limit ≤ MAX_PAGE_SIZE and offset ≤ MAX_OFFSET (and that both are ≥ 0), returning
clear error messages like "Must be between 0 and MAX_PAGE_SIZE" or "Must be
between 0 and MAX_OFFSET" when out of range; keep allowing empty input (to mean
default/all) and still call handleCancel(limitInput)/handleCancel(offsetInput)
as before.
In `@src/index.ts`:
- Around line 17-53: The run() function currently handles known parsed.command
values but will silently return if ParsedArgs gains new command types; add an
exhaustive check after the existing if blocks that handles any unknown
parsed.command (use a default branch or an assertUnreachable-style helper) to
log an error and exit non-zero or throw; reference parsed.command and run() and,
if you add a helper, name it assertUnreachable(value: never) to make TypeScript
catch unhandled command variants at compile time.
🪄 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: febe38f9-78cf-41ee-850f-f08e0f7c4e90
📒 Files selected for processing (7)
src/cli/args-help.tssrc/cli/browser.tssrc/cli/keys.tssrc/cli/prompts.tssrc/discord/client.tssrc/discord/search.tssrc/index.ts
- Fix global help behavior: --help without subcommand now shows help instead of falling into interactive mode - Update parseSearchCommand to short-circuit to help when global.help is true regardless of --guild presence - Add parser tests for --help and search --help scenarios - Unify --limit contract: changed from page-size limit to total fetch cap - Update README with correct usage commands and implementation status
github-actions
Bot
added
documentation
There was a problem hiding this comment.
2 issues found across 5 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="README.md">
<violation number="1" location="README.md:70">
P3: Fix the NOTE admonition syntax by closing the bracket so it renders correctly.</violation>
</file>
<file name="src/discord/search.ts">
<violation number="1" location="src/discord/search.ts:164">
P1: `fetchPage` now compares results against `MAX_PAGE_SIZE` even when the API request used a smaller `params.limit`, causing premature `break` in pagination.</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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 70: Replace the malformed alert token "[!NOTE>" with the correct
"[!NOTE]" in the README so the note block renders properly; locate occurrences
of the alert start marker (the string "[!NOTE>") and change the trailing ">" to
a closing bracket "]" (i.e., update "[!NOTE>" -> "[!NOTE]") wherever it appears.
- Line 72: Remove the trailing whitespace on the blank line in README.md (the
line the reviewer flagged) by deleting the extra space character so the line
contains no trailing space; simply save the file with that trailing space
removed.
🪄 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: 4a67d5fc-b29a-4830-8361-dbea7d79535b
📒 Files selected for processing (5)
README.md__tests__/index.test.tssrc/cli/args-parse.tssrc/discord/schemas.tssrc/discord/search.ts
…ion pageSize mismatch - README.md: Fix [!NOTE> -> [!NOTE] so admonition renders correctly - README.md: Remove trailing whitespace on blank line - search.ts: Use actual request limit instead of MAX_PAGE_SIZE in fetchPage to prevent premature pagination break when params.limit < MAX_PAGE_SIZE Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/discord/search.ts (1)
168-175:⚠️ Potential issue | 🟠 MajorAvoid hard-terminating pagination on
total_results; it is not reliable for large searches.The code correctly avoids overwriting
state.totalResultsper partition, but it is still used as a stop condition at lines 207 and 322. Discord's API documentation states thattotal_results"may not be accurate" and "may return slightly fewer results than the limit specified." For result sets exceeding 10,000,total_resultscan be capped or scoped, particularly in dynamic guilds or with old messages. This directly breaks snowflake pagination logic, which requires resilience beyond a single unreliable count.Terminate on
maxMessages(if specified), empty page, ormaxIdnon-advancement instead. UsetotalResultsfor progress display only, not pagination control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/discord/search.ts` around lines 168 - 175, The pagination must not hard-terminate based on state.totalResults (which is unreliable); update the loop/stop logic so that state.totalResults and data.total_results are only used for progress display, and pagination termination relies on: hitting maxMessages (if provided), receiving an empty page (no messages returned), or detecting no advancement of the pagination cursor (max_id / maxId not changing between iterations). Remove or disable any checks that use state.totalResults as a stop condition (references near the existing checks that currently check state.totalResults at the later stop points) and ensure the loop instead checks message array length, compares previous and current max_id, and enforces maxMessages for final termination.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/discord/search.ts`:
- Around line 168-175: The pagination must not hard-terminate based on
state.totalResults (which is unreliable); update the loop/stop logic so that
state.totalResults and data.total_results are only used for progress display,
and pagination termination relies on: hitting maxMessages (if provided),
receiving an empty page (no messages returned), or detecting no advancement of
the pagination cursor (max_id / maxId not changing between iterations). Remove
or disable any checks that use state.totalResults as a stop condition
(references near the existing checks that currently check state.totalResults at
the later stop points) and ensure the loop instead checks message array length,
compares previous and current max_id, and enforces maxMessages for final
termination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3f7668b-7a01-4164-a3f6-8bc1b5990149
📒 Files selected for processing (2)
README.mdsrc/discord/search.ts
|
@greptile |
|
@coderabbitai any more issues that need to be resolved, even really nit picky ones. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me do a thorough read of all the changed files to find any additional issues. 🏁 Script executed: Length of output: 77488 Let me also verify the 🏁 Script executed: Length of output: 1366
🔴 New Bugs16.
|
| # | File | Severity | Issue |
|---|---|---|---|
| 16 | browser.ts |
🔴 Bug | ANSI_CLEAR_SCREEN without ANSI_CURSOR_HOME → garbled rendering |
| 17 | index.ts |
🔴 Bug | --version with subcommand triggers guild error instead of printing version |
| 18 | utils.ts/prompts.ts |
🟡 Design | parseCommaSeparated duplicated with divergent return types |
| 19 | index.ts |
🟡 Design | printVersion() hardcodes "0.0.0" instead of reading from package.json |
| 20 | index.ts |
🟡 Design | run() missing exhaustiveness check for unhandled commands |
| 21 | prompts.ts |
🟡 Design | parseCommaSeparated(",,,") → [] is truthy; sets empty-array filter |
| 22 | prompts.ts |
🟡 Design | authorType uses select (one) but schema supports an array |
| 23 | schemas.ts |
🔵 Nit | .max(20) on snowflakeSchema is redundant after regex |
| 24 | browser.ts |
🔵 Nit | Embed title, field name/value not truncated — can break terminal layout |
| 25 | browser.ts |
🔵 Nit | browseMessages Promise never rejects — hangs if stdout fails |
| 26 | browser.ts |
🔵 Nit | Escape key undocumented in nav hint |
| 27 | search.ts |
🔵 Nit | Array.prototype.push.apply — prefer push(...pageMessages) |
| 28 | README.md |
🔵 Nit | References .env.example that doesn't appear to exist |
| 29 | browser.ts |
🔵 Nit | msg.timestamp shown as raw ISO string; hard to read |
🧠 Learnings used
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:43.140Z
Learning: Applies to src/handlers/**/handlers/**/*.ts : Use clack/prompts with handleCancel wrapper for user interactions
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/discord/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:36.708Z
Learning: Applies to src/discord/**/*.ts : Do not skip Zod validation of API responses
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T03:48:58.569Z
Learning: Applies to src/handlers/**/*.ts : Coordinate discord API → collate → export → file pipeline in handlers
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T14:56:15.999Z
Learning: Applies to src/handlers/**/*.ts : Use clack/prompts with handleCancel wrapper for user interactions in handlers
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/discord/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:36.708Z
Learning: Applies to src/discord/**/*.ts : Validate every API response with Zod before processing
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:43.140Z
Learning: Applies to src/handlers/**/handlers/**/*.ts : Don't call discord directly from CLI handlers
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/handlers/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:43.140Z
Learning: Applies to src/handlers/**/handlers/**/export.ts : Coordinate discord API → collate → export → file pipeline
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/discord/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:36.708Z
Learning: Applies to src/discord/**/*.ts : Extract magic numbers as named constants (MAX_OFFSET=9975, MAX_RETRIES=5, BASE_DELAY_MS)
Learnt from: CR
Repo: mynameistito/discord-search PR: 0
File: src/discord/AGENTS.md:0-0
Timestamp: 2026-03-26T03:41:36.708Z
Learning: Applies to src/discord/**/search.ts : Use offset pagination with 0-9975 range (25 results per page, max 400 pages)
…emas, and search - Add non-TTY guard to prevent Promise hangs in browseMessages - Fix ANSI cursor positioning by adding ANSI_CURSOR_HOME - Fix --version flag to work with subcommands (search, preset, settings) - Consolidate parseCommaSeparated in utils.ts, remove duplicate from prompts - Add VERSION constant for version display - Fix empty array truthy bug by adding length checks - Change authorType to multiselect to match schema array support - Remove redundant .max(20) validation from snowflakeSchema - Truncate embed titles (80 chars) and field names/values (40/100 chars) - Add comprehensive error handling with reject to browseMessages Promise - Document Esc key in navigation hints - Use spread syntax for array operations - Format timestamps with 24-hour locale format
There was a problem hiding this comment.
2 issues found across 7 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/cli/browser.ts">
<violation number="1" location="src/cli/browser.ts:168">
P2: `render()` references `cleanup` before it is initialized, which can throw a `ReferenceError` on initial render failures.</violation>
</file>
<file name="src/cli/args-parse.ts">
<violation number="1" location="src/cli/args-parse.ts:330">
P2: The `search --version` path constructs an invalid `SearchArgs` object, causing parse-time failure instead of printing the version.</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: 6
♻️ Duplicate comments (1)
src/cli/prompts.ts (1)
132-151:⚠️ Potential issue | 🟡 MinorNumeric prompt validation remains too permissive (Line 132-151 / 190-198).
Using
Number.parseInt+ NaN-only checks accepts partial numerics (e.g.,10abc) and defers failures to schema validation instead of giving immediate prompt feedback.♻️ Proposed fix
+import { MAX_OFFSET } from "@/discord/schemas.ts"; @@ const limitInput = await text({ @@ validate: (v) => { - if (v?.trim() && Number.isNaN(Number.parseInt(v.trim(), 10))) { - return "Must be a number"; + const s = v?.trim(); + if (!s) return; + if (!/^\d+$/.test(s)) { + return "Must be a whole number"; + } + const n = Number(s); + if (n < 1) { + return "Must be at least 1"; } }, }); @@ const offsetInput = await text({ @@ validate: (v) => { - if (v?.trim() && Number.isNaN(Number.parseInt(v.trim(), 10))) { - return "Must be a number"; + const s = v?.trim(); + if (!s) return; + if (!/^\d+$/.test(s)) { + return "Must be a whole number"; + } + const n = Number(s); + if (n > MAX_OFFSET) { + return `Must be <= ${MAX_OFFSET}`; } }, });#!/bin/bash # Verify permissive parseInt-based validation in prompts rg -nP 'Number\.parseInt\(v\.trim\(\), 10\)' src/cli/prompts.ts -C3 # Verify offset schema bound for mismatch with prompt-time validation strictness rg -nP 'offset:\s*z\.number\(\)\.int\(\)\.min\(0\)\.max\(MAX_OFFSET\)' src/discord/schemas.ts -C1Also applies to: 190-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/prompts.ts` around lines 132 - 151, The prompt validators for limitInput and offsetInput use Number.parseInt(...) with only a NaN check, which permits inputs like "10abc"; update the validate functions used in text({...}) for limitInput and offsetInput to enforce strictly numeric input (e.g., check trimmed value matches /^\d+$/ or use Number(v.trim()) and Number.isInteger with no extraneous chars) and return "Must be a number" when invalid so the prompt rejects malformed values immediately; ensure this stricter validation aligns with the offset schema (offset: z.number().int().min(0).max(MAX_OFFSET)) and apply the same change to the duplicate validator later in the file.
🤖 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/cli/args-parse.ts`:
- Around line 493-504: The code is inventing fake actions like "preset/list" and
"settings/show" to carry --version; instead create a dedicated version variant
(or a top-level short-circuit) in parseArgs so --version never mutates other
action values. Concretely, replace the branches that return parseWithError with
fake actions when global.version is true by returning a ParsedArgs for a
dedicated command such as command: "version" (e.g., via
parseWithError<ParsedArgs>({ command: "version", action: "show", help:
global.help, version: true, token: global.token }, "version")), or alternatively
add a single early check in parseArgs (if global.version) that returns this
dedicated version ParsedArgs before any command-specific mapping; update any
other similar blocks (including the other occurrence around settings/show) to
use the same dedicated variant.
- Around line 329-338: The returned SearchArgs for the global.version branch is
missing required fields and fails schema validation; update the object passed to
parseWithError in the global.version check to include the required fields (e.g.,
add params: [] and json: false) so the shape satisfies
ParsedArgsSchema/SearchArgs validation and allows the version handler in
index.ts to run; reference the parseWithError call, the SearchArgs type, and the
global.version condition when making this change.
In `@src/cli/browser.ts`:
- Around line 104-111: handleRenderError currently calls reject(err) then
cleanup(), which lets exceptions from cleanup escape; change it so cleanup is
invoked inside a try/catch (e.g., try { cleanup(); } catch (cleanupErr) { /*
swallow or log via console.error/processLogger */ }) and then call reject(err)
(or call reject in a finally block) so that any exception thrown by cleanup does
not propagate; update the handleRenderError function to swallow or log cleanup
errors and ensure reject(err) is always invoked without rethrowing cleanup
exceptions.
- Around line 160-169: render() is called before the cleanup variable is
initialized, so if renderMessageView throws, handleRenderError(err, reject,
cleanup) will reference cleanup in TDZ and throw a ReferenceError that hides the
original error; fix by declaring and initializing cleanup to a no-op before the
initial render call (e.g. let cleanup: () => void = () => {};), then later
assign the real cleanup function where currently defined, so render,
renderMessageView, handleRenderError and state/messages logic stay the same but
cleanup is safe to reference on errors.
In `@src/discord/search.ts`:
- Line 200: The spread operator call state.allMessages.push(...pageMessages)
causes large-batch performance issues; replace it with an explicit loop that
iterates pageMessages and pushes each item into state.allMessages (e.g., for
(const msg of pageMessages) state.allMessages.push(msg)) so accumulation happens
one element at a time; update the code in the function/method containing
state.allMessages and pageMessages to use this for-loop pattern instead of the
spread push.
In `@src/index.ts`:
- Around line 22-25: The code currently calls process.exit() after printing
output (e.g., when parsed.version is truthy and printVersion() is called);
change these exits to set process.exitCode to the desired status (0 or other)
and then return from the function so the process can flush I/O naturally. Locate
the branches using parsed.version, printVersion(), and other places that call
process.exit() (the blocks around those conditions) and replace each direct
process.exit(...) call with assignment to process.exitCode and a plain return;
ensure the same exit code value is preserved.
---
Duplicate comments:
In `@src/cli/prompts.ts`:
- Around line 132-151: The prompt validators for limitInput and offsetInput use
Number.parseInt(...) with only a NaN check, which permits inputs like "10abc";
update the validate functions used in text({...}) for limitInput and offsetInput
to enforce strictly numeric input (e.g., check trimmed value matches /^\d+$/ or
use Number(v.trim()) and Number.isInteger with no extraneous chars) and return
"Must be a number" when invalid so the prompt rejects malformed values
immediately; ensure this stricter validation aligns with the offset schema
(offset: z.number().int().min(0).max(MAX_OFFSET)) and apply the same change to
the duplicate validator later in the file.
🪄 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: da21e05e-9060-4644-a46f-a481fafda602
📒 Files selected for processing (7)
src/cli/args-parse.tssrc/cli/browser.tssrc/cli/prompts.tssrc/cli/utils.tssrc/discord/schemas.tssrc/discord/search.tssrc/index.ts
There was a problem hiding this comment.
1 issue found across 5 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/cli/args-parse.ts">
<violation number="1" location="src/cli/args-parse.ts:336">
P2: `params: {}` does not satisfy SearchParamsSchema (missing required `guildId`), so `search --version` will fail validation and exit instead of showing the version.</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: 4
🤖 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/cli/browser.ts`:
- Around line 168-200: The initial render failure can still lead to
createKeyListener being installed; change the logic so the code only sets up the
key listener if the initial render succeeded. In practice, add a local boolean
(e.g., renderSucceeded) or make render() return a success flag and in the outer
flow check that flag before assigning cleanup = createKeyListener(...). Update
the try/catch around render() (and the inner catch that calls handleRenderError)
to set renderSucceeded = false on error and skip calling createKeyListener,
referencing the existing render, handleRenderError, createKeyListener, cleanup,
resolve, and reject symbols.
In `@src/cli/prompts.ts`:
- Around line 34-42: Update the validate callback on the text(...) call that
assigns guildId to not only check for non-empty but also enforce Discord
snowflake format: trim the input and ensure it matches /^\d{17,19}$/, returning
"Guild ID must be a 17–19 digit Discord snowflake" when it fails; otherwise
return undefined/true to indicate success. Target the validate function passed
to text({ message: "Guild (Server) ID:", initialValue: defaultGuildId, validate:
... }) and replace the current only-empty check with the combined trim + regex
check.
- Around line 15-20: Change handleCancel from returning void to an assertion
function so TypeScript can narrow types without casts: make handleCancel generic
with an assertion signature (using asserts value is T) and keep the existing
logic that calls isCancel and cancel/process.exit; update callsites (e.g., where
token/password is awaited) to rely on the assertion instead of casting.
Reference symbols: handleCancel, isCancel, cancel. Ensure the function remains
exported and behavior unchanged except for its type signature.
In `@src/index.ts`:
- Line 7: Replace the hardcoded const VERSION = "0.0.0" with a dynamic value so
it cannot drift from package.json: either import or require the package.json
version (e.g., pull version from package.json into the VERSION constant in
src/index.ts) or read process.env.npm_package_version at runtime, or support a
build-time injected token like __VERSION__ and fall back to package.json; update
the symbol VERSION accordingly so the app always uses the package.json-provided
version.
🪄 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: ab486f62-6cf8-4746-b096-cb0ec08186ff
📒 Files selected for processing (5)
src/cli/args-parse.tssrc/cli/browser.tssrc/cli/prompts.tssrc/discord/search.tssrc/index.ts
- args-parse: search --version returns HelpArgs to avoid Zod validation failure on missing guildId - browser: only install key listener if initial render succeeded - prompts: enforce snowflake format (17-20 digits) on guildId validation - prompts: convert handleCancel to assertion function for proper type narrowing - index: import VERSION from package.json instead of hardcoded value Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai any more issues that need to be resolved, please do a deep analysis |
- args-parse: reject --all + named presets in preset run-all - args-parse: validate snowflake format for settings set guild/client-id - index: add exhaustiveness check for unhandled commands - utils: extract shared INTEGER_REGEX, import in args-parse and prompts - schemas: export SNOWFLAKE_REGEX, import in prompts and args-parse - browser: rename misleading safeWrite to write - keys: document stdin encoding restoration limitation Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
Stacked PR Chain
This is PR 5 of 7 — merge in order.
main← PR1 ← PR2 ← PR3 ← PR4 ← PR5 ← PR6 ← PR7Test plan
bun run typecheckpassesbun run checkpasses lintingSummary by cubic
Adds interactive CLI components for Discord message search using
@clack/prompts, a keyboard-driven message browser, and safer terminal cleanup. The CLI now routes help/version correctly (including subcommands), unifies--limitto cap total results, and tightens parser validation for presets and settings.New Features
--helpand--version(works with subcommands, version read frompackage.json); adds--guildand--client-id; unimplemented subcommands print clear errors (exit 1).Bug Fixes
--helpwithout a subcommand shows global help;search --helpshort-circuits;search --versionprints version without requiring--guild.--limit< max;--limitcaps total fetched; only capture total results from the first page; track remapped bucket keys across retries.preset run-allwhen mixing--allwith names; validate snowflakes forsettings set guild/client-id; shareINTEGER_REGEXand exportSNOWFLAKE_REGEX; add exhaustiveness check for unknown commands; TTY/browser rendering fixes with 24-hour timestamps and truncated embed titles/fields.Written for commit ce9b52e. Summary will update on new commits.