feat: add JSONC support for preset files#21
Conversation
…iscriminator error Multiple preset and settings schemas share the same command literal value, which z.discriminatedUnion does not allow.
…time support Adds src/fs.ts with fileExists, readTextFile, readJsonFile, and writeTextFile helpers backed by node:fs/promises, replacing all Bun.file/Bun.write usage.
Bump to 0.1.2-beta.4
github-actions
Bot
added
documentation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (67)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR establishes the complete foundational structure and implementation for a Discord message search CLI application. It adds project configuration (package.json, TypeScript, Biome, build tools), GitHub workflows and templates, comprehensive documentation, CLI argument parsing and handlers, Discord API integration with rate-limit handling, data collation and multi-format export utilities, preset management, and supporting configuration/filesystem utilities. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const outputDir = await text({ | ||
| message: "Output directory:", | ||
| initialValue: defaultDir, | ||
| }); |
There was a problem hiding this comment.
🟡 Medium handlers/export.ts:67
When the user clears the default directory value and submits an empty or whitespace-only string, outputDir.trim() returns "", causing paths like ${dir}/data.json to resolve to /data.json and attempt writing to the filesystem root. The text prompt lacks a validate function to prevent empty input, unlike the similar pattern elsewhere in the codebase.
| const outputDir = await text({ | |
| message: "Output directory:", | |
| initialValue: defaultDir, | |
| }); | |
| const outputDir = await text({ | |
| message: "Output directory:", | |
| initialValue: defaultDir, | |
| validate: (v) => { | |
| if (!v?.trim()) return "Output directory is required"; | |
| }, | |
| }); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/handlers/export.ts around lines 67-70:
When the user clears the default directory value and submits an empty or whitespace-only string, `outputDir.trim()` returns `""`, causing paths like `${dir}/data.json` to resolve to `/data.json` and attempt writing to the filesystem root. The `text` prompt lacks a `validate` function to prevent empty input, unlike the similar pattern elsewhere in the codebase.
Evidence trail:
src/handlers/export.ts:67-72 (text prompt without validate, trim to dir variable), src/handlers/export.ts:78-89 (mkdir and path construction with `${dir}/`), src/cli/prompts.ts:45-48 (example of validate function checking `!v?.trim()`), src/handlers/presets.ts:100-103 (another validate example for required field)
| const parsePresetRunAllAction = ( | ||
| remaining: string[], | ||
| global: GlobalFlags | ||
| ): PresetRunAllArgs => { | ||
| const restArgs = remaining.slice(2); | ||
| const names: string[] = []; | ||
| let all = false; | ||
| const flags = parseOutputFlags(restArgs); | ||
|
|
||
| for (const arg of restArgs) { | ||
| if (arg === "--all") { | ||
| all = true; | ||
| } else if (!arg.startsWith("-")) { | ||
| names.push(arg); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 High cli/args-parse.ts:259
In parsePresetRunAllAction, flag values for --export and --output-dir are incorrectly captured as preset names. The function calls parseOutputFlags(restArgs) which reads those values, but they remain in restArgs. When iterating to collect names, any non-flag argument is added, so preset run-all --export json myPreset produces names: ["json", "myPreset"] instead of ["myPreset"]. Consider filtering out consumed flag values before collecting names, or iterate only over arguments not consumed by parseOutputFlags.
- const restArgs = remaining.slice(2);
+ const restArgs = remaining.slice(2).filter(arg => arg !== "--export" && arg !== "--output-dir" && arg !== "--json");🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/cli/args-parse.ts around lines 259-274:
In `parsePresetRunAllAction`, flag values for `--export` and `--output-dir` are incorrectly captured as preset names. The function calls `parseOutputFlags(restArgs)` which reads those values, but they remain in `restArgs`. When iterating to collect `names`, any non-flag argument is added, so `preset run-all --export json myPreset` produces `names: ["json", "myPreset"]` instead of `["myPreset"]`. Consider filtering out consumed flag values before collecting names, or iterate only over arguments not consumed by `parseOutputFlags`.
Evidence trail:
src/cli/args-parse.ts lines 150-172: `parseOutputFlags` function reads flag values but does NOT modify the input array.
src/cli/args-parse.ts lines 259-282: `parsePresetRunAllAction` calls `parseOutputFlags(restArgs)` then iterates over the same unmodified `restArgs`, pushing any arg that doesn't start with `-` to `names`.
Line 271-272: The condition `!arg.startsWith("-")` would match flag values like "json" (from `--export json`) since they don't start with a hyphen.
| const data = result.value; | ||
| const pageSize = params.limit ? Math.min(params.limit, 25) : 25; | ||
|
|
||
| if (offset === 0 && !maxId) { |
There was a problem hiding this comment.
🔴 Critical discord/search.ts:130
When params.offset is non-zero, state.totalResults is never initialized because the condition on line 130 (offset === 0 && !maxId) fails. This leaves totalResults at 0, causing state.allMessages.length >= state.totalResults on line 160 to immediately evaluate true and return "done", truncating pagination to just the first page.
- if (offset === 0 && !maxId) {
+ if ((offset === 0 || offset === config.startOffset) && !maxId) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/discord/search.ts around line 130:
When `params.offset` is non-zero, `state.totalResults` is never initialized because the condition on line 130 (`offset === 0 && !maxId`) fails. This leaves `totalResults` at 0, causing `state.allMessages.length >= state.totalResults` on line 160 to immediately evaluate true and return `"done"`, truncating pagination to just the first page.
Evidence trail:
src/discord/search.ts lines 130-135 (condition that guards totalResults initialization), line 160 (comparison that checks completion), line 229 (state initialization with totalResults: 0), line 221 (startOffset from params.offset), lines 186-188 (fetchPartition using startOffset as initialOffset when maxId is undefined)
| const buildStateFromConfig = ( | ||
| configResult: ConfigResult, | ||
| cliToken?: string | ||
| ): AppState => ({ | ||
| token: configResult.isOk() ? configResult.value.token : (cliToken ?? ""), |
There was a problem hiding this comment.
🟠 High src/index.ts:176
The cliToken parameter is silently ignored when configResult.isOk() is true, so the --token CLI flag has no effect when a config file exists. Compare with runInteractive at line 86, which uses resolveToken(cliArgs.token, configResult) to properly prioritize CLI arguments over config file values.
token: configResult.isOk() ? configResult.value.token : (cliToken ?? ""),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/index.ts around lines 176-180:
The `cliToken` parameter is silently ignored when `configResult.isOk()` is true, so the `--token` CLI flag has no effect when a config file exists. Compare with `runInteractive` at line 86, which uses `resolveToken(cliArgs.token, configResult)` to properly prioritize CLI arguments over config file values.
Evidence trail:
src/index.ts lines 176-184: `buildStateFromConfig` uses `configResult.isOk() ? configResult.value.token : (cliToken ?? "")` which ignores cliToken when config exists.
src/index.ts line 86: `runInteractive` uses `resolveToken(cliArgs.token, configResult)` which properly prioritizes CLI token.
src/handlers/settings.ts lines 105-110, 138-143: `resolveToken` and `resolveTokenNonInteractive` both check `if (cliToken) { return cliToken; }` first, correctly prioritizing CLI over config.
src/index.ts lines 128, 145, 157: Other non-interactive handlers use `resolveTokenNonInteractive` which properly handles CLI token priority.
| const presetsResult = await loadPresets(); | ||
| const presets = presetsResult.isOk() ? presetsResult.value : []; |
There was a problem hiding this comment.
🟠 High src/presets.ts:49
When loadPresets() returns an Err, the code silently falls back to an empty array on line 50 and then overwrites the presets file with only the new preset. If loading fails due to a transient I/O error or permission issue, all existing presets are lost. Consider propagating the error instead of proceeding with an empty array.
+ const presetsResult = await loadPresets();
+ if (!presetsResult.isOk()) {
+ return presetsResult;
+ }
+ const presets = presetsResult.value;Also found in 1 other location(s)
src/handlers/export.ts:26
If an unrecognized
formatvalue is passed (e.g.,"none"or a typo), the function silently creates an empty directory and returns success without exporting any files or indicating an error. Callers have no way to know their format was invalid.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/presets.ts around lines 49-50:
When `loadPresets()` returns an `Err`, the code silently falls back to an empty array on line 50 and then overwrites the presets file with only the new preset. If loading fails due to a transient I/O error or permission issue, all existing presets are lost. Consider propagating the error instead of proceeding with an empty array.
Evidence trail:
src/presets.ts lines 22-39 (loadPresets function - returns Ok([]) for missing file, Err for actual errors), line 50 (falls back to empty array on error: `presetsResult.isOk() ? presetsResult.value : []`), line 59 (overwrites file: `await writeTextFile(file, JSON.stringify(presets, null, 2))`). Commit: REVIEWED_COMMIT
Also found in 1 other location(s):
- src/handlers/export.ts:26 -- If an unrecognized `format` value is passed (e.g., `"none"` or a typo), the function silently creates an empty directory and returns success without exporting any files or indicating an error. Callers have no way to know their format was invalid.
| "$schema": "./node_modules/@biomejs/biome/configuration_schema.json", | ||
| "extends": ["ultracite/biome/core"], | ||
| "files": { | ||
| "includes": ["!output"] |
There was a problem hiding this comment.
🟡 Medium biome.jsonc:5
The files.includes array contains only a negation pattern ["!output"], so Biome matches no files — all linting and formatting is silently disabled. Biome requires at least one positive glob (e.g., "**") for negation patterns to work. Consider adding "**" before the negation, or move the exclusion to files.ignores instead.
- "includes": ["!output"]
+ "includes": ["**", "!output"]🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file biome.jsonc around line 5:
The `files.includes` array contains only a negation pattern `["!output"]`, so Biome matches no files — all linting and formatting is silently disabled. Biome requires at least one positive glob (e.g., `"**"`) for negation patterns to work. Consider adding `"**"` before the negation, or move the exclusion to `files.ignores` instead.
| return bodyResult; | ||
| } | ||
|
|
||
| const retryAfter = bodyResult.value.retry_after; |
There was a problem hiding this comment.
🟠 High discord/client.ts:99
handle429 extracts retry_after from Discord's 429 response without validation. If the field is missing or not a number, sleep(undefined * 1000) becomes sleep(NaN), which resolves immediately and burns through all retries in a tight loop. Consider adding a fallback default like handle202 does with || 2.
- const retryAfter = bodyResult.value.retry_after;
+ const retryAfter = bodyResult.value.retry_after || 2;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/discord/client.ts around line 99:
`handle429` extracts `retry_after` from Discord's 429 response without validation. If the field is missing or not a number, `sleep(undefined * 1000)` becomes `sleep(NaN)`, which resolves immediately and burns through all retries in a tight loop. Consider adding a fallback default like `handle202` does with `|| 2`.
Evidence trail:
src/discord/client.ts lines 99-109 (REVIEWED_COMMIT): `handle429` extracts `retry_after` directly without validation: `const retryAfter = bodyResult.value.retry_after;` and uses it as `await sleep(retryAfter * 1000);`
src/discord/client.ts line 136 (REVIEWED_COMMIT): `handle202` uses fallback: `const retryAfter = parsed.success ? parsed.data.retry_after || 2 : 2;`
src/discord/client.ts lines 25-26 (REVIEWED_COMMIT): sleep function implementation uses setTimeout which coerces NaN to 0, causing immediate resolution.
| if (retryResponse.status !== 202) { | ||
| return await parseResponse(retryResponse, schema); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium discord/client.ts:150
When a retry response in the 202 polling loop has status !== 202, parseResponse is called directly without checking for 429 or error statuses first. If Discord returns a 429 or 4xx/5xx during polling, the error response body is validated against the success schema, producing a misleading ValidationError instead of the proper DiscordApiError or RateLimitExhaustedError. Consider routing non-202 statuses through the same error handling as the initial request (429 handling, error response handling, then success parsing).
- if (retryResponse.status !== 202) {
- return await parseResponse(retryResponse, schema);
- }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/discord/client.ts around lines 150-153:
When a retry response in the 202 polling loop has `status !== 202`, `parseResponse` is called directly without checking for 429 or error statuses first. If Discord returns a 429 or 4xx/5xx during polling, the error response body is validated against the success schema, producing a misleading `ValidationError` instead of the proper `DiscordApiError` or `RateLimitExhaustedError`. Consider routing non-202 statuses through the same error handling as the initial request (429 handling, error response handling, then success parsing).
Evidence trail:
src/discord/client.ts lines 136-156 (handle202 function with polling loop, line 150 showing direct parseResponse call for non-202), src/discord/client.ts lines 217-248 (discordFetch function showing proper error handling flow: 429 → handle429, !response.ok → handleErrorResponse, then parseResponse), src/discord/client.ts lines 80-114 (handle429 function), src/discord/client.ts lines 161-177 (handleErrorResponse function), src/discord/client.ts lines 180-210 (parseResponse function)
| const wasRaw = process.stdin.isRaw; | ||
| process.stdin.setRawMode(true); | ||
| process.stdin.resume(); | ||
| process.stdin.setEncoding("utf8"); | ||
|
|
||
| const handler = (data: string) => { | ||
| // Ctrl+C — exit | ||
| if (data === "\x03") { | ||
| process.exit(0); | ||
| } | ||
| onKey(data); | ||
| }; | ||
|
|
||
| process.stdin.on("data", handler); | ||
|
|
||
| return () => { | ||
| process.stdin.removeListener("data", handler); | ||
| if (!wasRaw) { | ||
| process.stdin.setRawMode(false); | ||
| } | ||
| process.stdin.pause(); |
There was a problem hiding this comment.
🟢 Low cli/keys.ts:10
The cleanup function unconditionally calls process.stdin.pause(), even if stdin was already resumed before createKeyListener was called. This leaves stdin paused after cleanup, breaking other code that depends on stdin remaining active. Consider saving the initial paused state with const wasPaused = process.stdin.isPaused() and only calling pause() during cleanup if wasPaused was true.
+ const wasPaused = process.stdin.isPaused();
process.stdin.setRawMode(true);
process.stdin.resume();
process.stdin.setEncoding("utf8");
@@ -27,3 +28,3 @@
- process.stdin.pause();
+ if (wasPaused) {
+ process.stdin.pause();
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/cli/keys.ts around lines 10-30:
The cleanup function unconditionally calls `process.stdin.pause()`, even if stdin was already resumed before `createKeyListener` was called. This leaves stdin paused after cleanup, breaking other code that depends on stdin remaining active. Consider saving the initial paused state with `const wasPaused = process.stdin.isPaused()` and only calling `pause()` during cleanup if `wasPaused` was true.
Evidence trail:
src/cli/keys.ts (lines 1-31 at REVIEWED_COMMIT): Line 9 saves `wasRaw = process.stdin.isRaw`, line 11 calls `process.stdin.resume()`, cleanup function (lines 24-30) conditionally restores raw mode with `if (!wasRaw)` but unconditionally calls `process.stdin.pause()` without checking if stdin was already active before the function was called.
1 participant
Summary
.discord-search-presets.jsoncas a supported preset file (parsed viajsonc-parser, allowing comments).discord-search-presets.jsonwhen no.jsoncfile exists.jsonfor new installsTest plan
.discord-search-presets.jsoncwith comments — verify presets load correctly.jsoncpresent — verify it writes back to.jsonc.discord-search-presets.json.jsonpresent — verify behaviour unchangedSummary by cubic
Adds support for
.discord-search-presets.jsoncso preset files can include comments.New Features
.discord-search-presets.jsoncusingjsonc-parser(comment support)..discord-search-presets.jsonwhen no.jsoncfile exists..json.Migration
.discord-search-presets.jsonto.discord-search-presets.jsonc.Written for commit b8b8aef. Summary will update on new commits.
Note
Add JSONC support for preset files with fallback to JSON format
.discord-search-presets.jsonc) usingjsonc-parser, with fallback to.discord-search-presets.jsonif no JSONC file existsresolvePresetsFilefunction in src/presets.ts📊 Macroscope summarized b8b8aef. 51 files reviewed, 20 issues evaluated, 5 issues filtered, 9 comments posted
🗂️ Filtered Issues
src/cli/browser.ts — 0 comments posted, 1 evaluated, 1 filtered
msg.content.lengthon line 26 will throw aTypeErrorifmsg.contentisnullorundefined. The fallback|| "(no text content)"on line 28 only executes after the length check passes. Discord messages containing only embeds or attachments can have null/undefined content. The fix should check for content existence first:const content = !msg.content ? "(no text content)" : msg.content.length > 100 ? \${msg.content.slice(0, 100)}...` : msg.content;` [ Failed validation ]src/cli/prompts.ts — 0 comments posted, 1 evaluated, 1 filtered
","or", , "), the function returns an empty array[]instead ofundefined. Since empty arrays are truthy in JavaScript, the caller's checkif (authorIdList)passes, causingparams.authorId = []to be set. This may cause the API to interpret an empty array as "match no authors" rather than "no author filter", potentially returning zero results when the user intended no filter. [ Failed validation ]src/handlers/export.ts — 1 comment posted, 3 evaluated, 1 filtered
formatvalue is passed (e.g.,"none"or a typo), the function silently creates an empty directory and returns success without exporting any files or indicating an error. Callers have no way to know their format was invalid. [ Cross-file consolidated ]src/handlers/search.ts — 0 comments posted, 1 evaluated, 1 filtered
messages.length === 0, line 221 outputs{"totalMessages":0,"messages":[]}with amessagesarray. However, when messages exist, line 229 outputsJSON.stringify(collated)which, based ondisplaySummary's usage ofcollateResults, returns an object with fields liketotalMessages,totalEmbeds,byAuthor,embedsByType, etc. — but nomessagesarray. JSON consumers expecting a consistent schema will break when switching between empty and non-empty results. [ Failed validation ]src/handlers/settings.ts — 0 comments posted, 4 evaluated, 1 filtered
password(), an empty input is stored as""without any trim or fallback toundefined. Compare with lines 51 and 61 which use(input as string).trim() || undefinedforclientIdanddefaultGuildId. An emptystate.tokencan cause issues downstream when the token is expected to be a valid string or undefined. [ Cross-file consolidated ]