-
Notifications
You must be signed in to change notification settings - Fork 8
Release/0.12.3 #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release/0.12.3 #15
Conversation
* feat: Add calculate-hash command * fix(calculate-hash): Comment fixes
WalkthroughAdds a new CLI subcommand "files calculate-hash" for hashing a file or directory, implemented in a new module. Updates package version and a dependency. Wires the new command into the CLI entrypoint to accept a localPath argument and output algo/hash JSON. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI (files calculate-hash)
participant H as filesCalculateHash
participant CRH as calculateResourceHash
participant CU as cryptoUtils.getDirHashFileContents
U->>CLI: run files calculate-hash <localPath>
CLI->>H: filesCalculateHash({ localPath })
H->>H: validate & resolve path
H->>CRH: compute resource hash
CRH-->>H: hash data
H->>CU: build root hash from contents
CU-->>H: algo:hash string
H-->>CLI: { algo, hash } JSON
CLI-->>U: print JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (8)
package.json (1)
40-40: Dependency upgrade to @super-protocol/sp-files-addon ^0.12.1 — check packaged binaryThe new command relies on
calculateResourceHashfrom this package. Since you bundle withpkg, please verify the packaged binary (dist/spctl) can runfiles calculate-hashsuccessfully. Ifsp-files-addonhas any dynamic requires,pkgmay need explicit asset hints.If a runtime include is needed, consider adding it to
pkg.assets, e.g.:"pkg": { "assets": [ "config.example.json", "node_modules/@super-protocol/uplink-nodejs/build/Release/*", "node_modules/axios/**/*" + // "node_modules/@super-protocol/sp-files-addon/**/*" ] }src/commands/filesCalculateHash.ts (6)
12-16: Nit: message says “Filename” but the command accepts files or foldersMinor wording tweak for clarity.
- if (!inputPath) { - Printer.print('Filename should be defined'); + if (!inputPath) { + Printer.print('Path should be defined');
20-30: Avoid extra I/O before hashing or gate it behind verbosityListing directory entries is only used for an informational log and performs an additional
readdirpass. Consider:
- Dropping the
readdircompletely, or- Printing a simpler “Found folder …” message without counting entries, or
- Emitting these lines only in a verbose mode (and keeping stdout clean for JSON).
- if (stat.isDirectory()) { - const files = await fs.readdir(localPath); - - Printer.print(`Found folder "${localPath}" with ${files.length} top-level entries`); - } else if (stat.isFile()) { + if (stat.isDirectory()) { + Printer.print(`Found folder "${localPath}"`); + } else if (stat.isFile()) { Printer.print(`Found file "${localPath}"`); } else { Printer.print(`Found path "${localPath}" (not a regular file or directory)`); }
31-35: Nit: remove the leading newline in the error messageKeeps stderr formatting consistent with the rest of the CLI.
- if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - Printer.error(`\nFile or folder is missing on path ${localPath}`); + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + Printer.error(`File or folder is missing on path ${localPath}`); return; }
47-54: Consider including encoding for consistency with other hash objectsOther parts of the CLI use
{ algo, encoding, hash }. If consumers expect that structure, addencoding: 'hex'(or whateverrootHash.hashuses).- const hash = { - algo: colonIndex > 0 ? raw.slice(0, colonIndex).toLowerCase() : '', - hash: colonIndex > 0 ? raw.slice(colonIndex + 1) : raw, - }; + const hash = { + algo: colonIndex > 0 ? raw.slice(0, colonIndex).toLowerCase() : '', + hash: colonIndex > 0 ? raw.slice(colonIndex + 1) : raw, + encoding: 'hex', + };
42-55: Keep stdout JSON-only; route progress logs elsewhere or add a quiet flagRight now the command prints human-readable lines (“Found …”, “Calculating hash…”) before the JSON. If the intent is machine consumption, this breaks piping (e.g., to jq). Options:
- Emit progress messages to stderr instead (keep JSON on stdout), or
- Add a
--quiet/--json-onlyflag and suppress progress logs when set.If you want a quick minimal change without new flags, send progress to stderr:
- Printer.print('Calculating hash...'); + Printer.error('Calculating hash...');And similarly for the “Found …” lines above.
55-57: Preserve original error details when wrappingRe-throwing a new Error drops the original stack/details. Use
causeto preserve context (Node.js supports it), or follow your top-level pattern withhasCustomMessage.- } catch (error) { - throw new Error(`Failed to calculate hash: ${(error as Error).message}`); - } + } catch (error) { + throw Object.assign(new Error('Failed to calculate hash'), { cause: error }); + }Alternatively, if you want the top-level handler to display a custom message and log the original error:
throw { hasCustomMessage: true, message: 'Failed to calculate hash', error };src/index.ts (1)
1598-1605: Consider a--quiet/--json-onlyoption for machine-friendly outputThe hashing routine prints informational lines before the final JSON. Adding a flag here would let callers get pure JSON on stdout.
filesCommand .command('calculate-hash') .description('Calculate the hash of a file or all files in a folder') - .argument('localPath', 'Path to the file or folder') - .action(async (localPath: string) => { - await filesCalculateHash({ localPath }); + .argument('localPath', 'Path to the file or folder') + .option('--quiet', 'Suppress non-JSON logs', false) + .action(async (localPath: string, options: any) => { + await filesCalculateHash({ localPath /*, quiet: options.quiet */ }); });If you adopt this, update
FilesCalculateHashParamsand guard prints accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)src/commands/filesCalculateHash.ts(1 hunks)src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/commands/filesCalculateHash.ts (1)
src/printer.ts (1)
error(25-27)
🔇 Additional comments (3)
package.json (1)
3-3: Version bump to 0.12.3 — LGTMRelease metadata aligns with the new CLI feature.
src/commands/filesCalculateHash.ts (1)
44-46: Confirm shape alignment between external utilities
calculateResourceHashis imported from@super-protocol/sp-files-addonandgetDirHashFileContentsfrom@super-protocol/sdk-js. Since there are no other calls togetDirHashFileContentsin this repo, please verify in their TypeScript definitions or official docs that the value returned bycalculateResourceHash(localPath)matches the input shape expected bygetDirHashFileContents({ … }). If they differ, this call could throw or produce an incorrect root hash.src/index.ts (1)
16-16: New import — LGTMCleanly wires the new command into the CLI.
Summary by CodeRabbit