Conversation
WalkthroughAdds two new CLI commands (run-sampler, generate-export-file), new runner and generator classes, URL download/open utility, new SF API endpoints, a CLI updater utility, changelog and version bump; wires those modules into the CLI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![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
🤖 Fix all issues with AI agents
In @lib/liquidSamplerRunner.js:
- Around line 36-43: The code assumes SF.createSamplerRun returned an object
with a data property and immediately accesses samplerResponse.data.id; add a
null-safety guard to handle falsy or malformed responses by first validating
samplerResponse and samplerResponse.data (e.g., using optional chaining or
explicit checks) before reading .id, set samplerId from samplerResponse.data?.id
?? samplerResponse.data, and if no id is found log the full samplerResponse (or
relevant error info) via consola.error and exit; update references in this block
around SF.createSamplerRun, samplerResponse, and samplerId accordingly.
- Around line 57-67: fetchSamplerStatus currently assumes SF.readSamplerRun
returns an object with a data property; add a null-safety check after awaiting
SF.readSamplerRun to verify response and response.data exist before calling
handleSamplerResponse, and if missing call errorUtils.errorHandler or log/throw
a descriptive error (including samplerId and partnerId) so you don't pass
undefined into handleSamplerResponse; update the function surrounding
SF.readSamplerRun, fetchSamplerStatus, handleSamplerResponse and
errorUtils.errorHandler accordingly to handle the absent-data branch.
🧹 Nitpick comments (2)
lib/liquidSamplerRunner.js (2)
192-192: Remove commented-out code.The commented-out exponential backoff logic is dead code. Either implement the backoff strategy or remove the comment to keep the codebase clean.
♻️ Proposed fix
waitingTime += pollingDelay; - // pollingDelay *= 1.05; if (waitingTime >= waitingLimit) {
225-231: Address TODO or track it in an issue.There's a TODO comment indicating the browser-opening logic should follow a pattern similar to the liquid test runner. Consider implementing this consistency or creating an issue to track it.
Would you like me to help open an issue to track this refactor, or would you like me to suggest an implementation that aligns with the liquid test runner pattern?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/cli.jslib/api/sfApi.jslib/liquidSamplerRunner.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/api/sfApi.js (1)
lib/liquidSamplerRunner.js (1)
samplerId(38-38)
bin/cli.js (1)
lib/liquidSamplerRunner.js (2)
templateHandles(25-25)templateHandles(80-80)
🔇 Additional comments (5)
lib/api/sfApi.js (1)
644-666: LGTM!The new
createSamplerRunandreadSamplerRunfunctions follow the established patterns in this file, with consistent error handling usingresponseSuccessHandlerandresponseErrorHandler. The partner-scoped Axios instance is correctly used for these partner API endpoints.bin/cli.js (1)
483-516: LGTM!The
run-samplercommand implementation follows existing CLI patterns. The validation logic correctly ensures at least one template type is specified when not fetching by ID, and thetemplateHandlesobject construction properly maps CLI options to the expected format used byliquidSamplerRunner.lib/liquidSamplerRunner.js (3)
1-11: LGTM!The imports are well-organized and include all necessary dependencies for the sampler functionality.
243-246: LGTM!The module exports correctly expose
runSamplerandfetchSamplerStatusfor use by the CLI and other consumers.
101-101: No action needed.The inconsistent
awaitusage is correct.SharedPart.read()is an async function (declared withstatic async read()), whileReconciliationText.read()andAccountTemplate.read()are synchronous functions (declared withoutasync). Usingawaitonly for the async method and omitting it for synchronous methods is the correct pattern.Likely an incorrect or invalid review comment.
AgustinSilverfin
force-pushed
the
agustin-bso-sampler
branch
from
56b49ae to
990bf5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/cli/cliUpdater.js (3)
64-66: Incorrect HTTP status check logic.Line 64 returns early if
response.status !== 200, but this prevents the function from returning the version when the status is 200. The logic should check for success (=== 200) and proceed, or check for failure and return early.🐛 Proposed fix
- if (response.status !== 200) { + if (response.status === 200 && response.data.version) { + return response.data.version; + } else { return; } - if (response.data.version) { - return response.data.version; - }Or simply remove the status check since axios throws on non-2xx by default:
- if (response.status !== 200) { - return; - } if (response.data.version) { return response.data.version; }
76-85: Version comparison doesn't handle equal versions or edge cases.The function has two issues:
- It doesn't return anything when versions are equal (line 84 is unreachable)
- It assumes both version strings have the same number of parts
For example, comparing "1.2.3" with "1.2" would only check the first two segments and return undefined.
♻️ Proposed fix
static #compareVersions(latestVersion, currentVersion) { const latestVersionParts = latestVersion.split(".").map(Number); const currentVersionParts = currentVersion.split(".").map(Number); + const maxLength = Math.max(latestVersionParts.length, currentVersionParts.length); - for (let i = 0; i < latestVersionParts.length; ++i) { + for (let i = 0; i < maxLength; ++i) { + const latest = latestVersionParts[i] || 0; + const current = currentVersionParts[i] || 0; - if (latestVersionParts[i] !== currentVersionParts[i]) { - return latestVersionParts[i] > currentVersionParts[i]; + if (latest !== current) { + return latest > current; } } + return false; // versions are equal }
1-7: Upgrade axios from 1.6.2 due to multiple critical security vulnerabilities.axios 1.6.2 is affected by several known CVEs:
- Prototype pollution via formDataToJSON (affects ≥0.28.0 and <1.6.4)
- Regular-expression DoS (affects ≥1.0.0 and <1.6.3)
- SSRF vulnerability in path/protocol-relative URL handling (affects ≥1.3.2 and <1.7.4)
- Data: URI DoS (affects <1.12.0)
Upgrade to at minimum 1.6.5+ for the first two issues, 1.7.4+ to address SSRF, or preferably the latest 1.12.x/1.13.x series to cover all known vulnerabilities.
chalk 4.1.2 and consola 3.2.3 are secure with no known vulnerabilities.
🤖 Fix all issues with AI agents
In @lib/cli/cliUpdater.js:
- Line 10: The UPDATE_COMMAND currently embeds a hardcoded sudo which breaks on
Windows; update static #UPDATE_COMMAND in cliUpdater.js to be cross-platform by
removing the leading "sudo" (or conditionally prepending it only on non-Windows
platforms), so the command becomes just "npm install -g ${pkg.repository.url}"
(or build it with a platform check using process.platform !== 'win32' to add
sudo on UNIX-like systems).
In @lib/exportFileInstanceGenerator.js:
- Around line 42-81: The method #generateInstance mixes returning false with
exception handling; make error signaling consistent by throwing errors instead
of returning false: replace each `return false` in #generateInstance with `throw
new Error(...)` containing context (use this.#details(exportFileInstanceId)
where available), and update the catch to call `errorUtils.errorHandler(error)`
then rethrow the error so callers like generateAndOpenFile can handle it; ensure
success paths still return the response as before.
- Around line 31-33: The early return in generateInstance() silently exits when
response is falsy; update the function (generateInstance in
exportFileInstanceGenerator.js) to log a clear warning or error before returning
(e.g., use the module's logger or console to state that generateInstance
returned no response and include any available context such as input parameters
or status) so callers/users can see why the process stopped; then return or
throw as appropriate.
In @lib/liquidSamplerRunner.js:
- Around line 233-234: The condition currently checks the wrong property
(response.result_url) before calling new
UrlHandler(response.content_url).openFile(); update the check to test
response.content_url instead so the guard matches the value actually accessed;
locate the block around the UrlHandler usage in liquidSamplerRunner.js and
replace the condition to ensure response.content_url is defined before
constructing UrlHandler.
- Line 44: The fallback assigning samplerId using "samplerResponse.data.id ||
samplerResponse.data" is unsafe; update the logic that sets samplerId (the
variable assigned from samplerResponse) to explicitly handle the two supported
shapes: if samplerResponse.data is an object with a defined non-null id (check
existence via !== undefined and !== null and ensure type is string or number)
use that id, else if samplerResponse.data is a primitive string/number use it
directly (check typeof), otherwise throw or return a clear validation error so
later API calls receive a valid samplerId.
In @lib/utils/urlHandler.js:
- Around line 31-44: The private async method #downloadFile swallows exceptions
by calling errorUtils.errorHandler(error) without rethrowing or returning,
causing callers like openFile to get undefined filePath; update #downloadFile so
after calling errorUtils.errorHandler(error) it either rethrows the error (throw
error) or returns a rejected promise (return Promise.reject(error)) so the error
propagates to openFile and upstream callers; make this change inside the catch
block of #downloadFile and keep the existing errorUtils.errorHandler call for
logging/handling.
- Around line 37-38: The code in urlHandler.js is using blocking fs.mkdirSync
and fs.writeFileSync inside an async flow; replace them with non-blocking async
calls (e.g., use fs.promises.mkdir or import from 'fs/promises' and await
mkdir(path.dirname(tempFilePath), { recursive: true }) and await
writeFile(tempFilePath, response.data)) and propagate/handle errors
appropriately so tempFilePath is created and written without blocking the event
loop; update any surrounding try/catch in the function that fetches
response.data to await these async ops.
- Line 33: The axios call currently reads the whole payload into memory using
responseType: "arraybuffer" which can OOM on large exports; change the
axios.get(this.url, { responseType: "arraybuffer" }) call to use streaming
(responseType: "stream"), then consume the returned response.stream by piping it
to a temporary file or processing it via streaming transforms (e.g.,
response.data.pipe(fs.createWriteStream(tempPath)) or using a stream parser),
ensure you handle backpressure/errors and cleanup the temp file, and update any
downstream code that expects a Buffer to instead read from the streamed file or
stream APIs.
🧹 Nitpick comments (7)
lib/utils/urlHandler.js (2)
36-36: Potential filename collision withDate.now().Using
Date.now()alone for the filename could cause collisions if multiple files are downloaded in rapid succession (within the same millisecond).♻️ Proposed improvement
Add a random component to reduce collision probability:
- const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}.${fileExtension}`); + const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}-${Math.random().toString(36).substr(2, 9)}.${fileExtension}`);
22-29: No cleanup mechanism for temporary files.Downloaded files accumulate in the temp directory without cleanup. Consider implementing automatic cleanup or documenting the cleanup strategy.
Potential approaches:
- Clean up the file after opening (though the OS might still be using it)
- Implement periodic cleanup of old files in the silverfin temp directory
- Use OS-level temp file APIs that handle cleanup automatically
- Document that users should manually clean
os.tmpdir()/silverfinperiodicallylib/exportFileInstanceGenerator.js (1)
56-67: Polling timeout and user experience considerations.Current polling configuration:
- MAX_ATTEMPTS: 25
- Initial wait: 1s, incrementing by 1s each iteration, capped at 5s
- Total max time: ~92 seconds (1+2+3+4+5×21)
Consider:
- Displaying progress to users during long waits
- Making timeout configurable for different scenarios
- Documenting expected generation times for various export types
Adding periodic progress updates would improve UX:
if (attempts % 5 === 0) { consola.info(`Still waiting for export file generation... (${attempts}/${this.#MAX_ATTEMPTS})`); }lib/liquidSamplerRunner.js (2)
93-176: Consider extracting duplicate validation logic.The config existence check and partner_id validation are duplicated across reconciliation texts, account templates, and shared parts sections. Extracting this into a helper method would improve maintainability.
Suggested refactor
+ #validateAndReadTemplate(templateType, identifier, TemplateReader) { + const configPresent = fsUtils.configExists(templateType, identifier); + + if (!configPresent) { + consola.error(`Config file for ${templateType} "${identifier}" not found`); + process.exit(1); + } + + const config = fsUtils.readConfig(templateType, identifier); + + if (!config.partner_id || !config.partner_id[this.partnerId]) { + consola.error(`Template '${identifier}' has no partner_id entry for partner ${this.partnerId}. Import the template to this partner first.`); + process.exit(1); + } + + return { + id: config.partner_id[this.partnerId], + content: TemplateReader.read(identifier) + }; + }Then simplify each loop:
for (const handle of reconciliationTexts) { const { id, content } = this.#validateAndReadTemplate('reconciliationText', handle, ReconciliationText); templates.push({ type: "Global::Partner::ReconciliationText", id: String(id), text: content.text, text_parts: content.text_parts, }); }
202-202: Remove commented-out code.The commented-out exponential backoff logic should either be implemented or removed to keep the codebase clean.
bin/cli.js (2)
512-515: Duplicate validation with LiquidSamplerRunner.This validation is also performed in
LiquidSamplerRunner.run()(lines 32-35 of liquidSamplerRunner.js). While this defensive programming provides early feedback to users, it creates maintenance overhead if validation logic needs to change.
517-521: Consider using shorthand property syntax.For consistency and conciseness, you can use ES6 shorthand property syntax.
Suggested refactor
const templateHandles = { reconciliationTexts: handles, - accountTemplates: accountTemplates, - sharedParts: sharedParts, + accountTemplates, + sharedParts, };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
CHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/cli/cliUpdater.jslib/exportFileInstanceGenerator.jslib/liquidSamplerRunner.jslib/utils/urlHandler.jspackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
lib/liquidSamplerRunner.js (1)
lib/cli/spinner.js (1)
spinner(76-76)
🔇 Additional comments (7)
CHANGELOG.md (1)
5-7: LGTM! Clear and informative changelog entry.The new version entry provides a comprehensive description of the
silverfin generate-export-filecommand with helpful usage guidance.lib/api/sfApi.js (2)
644-666: LGTM! Export file instance endpoints follow established patterns.The implementation is consistent with other API methods in this file, properly using AxiosFactory, error handling, and response utilities.
668-690: LGTM! Sampler run endpoints are well-structured.Both
createSamplerRunandreadSamplerRunfollow the same robust pattern used throughout this API module, with proper error handling and response management.lib/exportFileInstanceGenerator.js (1)
14-23: LGTM! Parameter validation in constructor.Proper validation ensures the class cannot be instantiated with missing required parameters, catching configuration errors early.
bin/cli.js (2)
6-7: LGTM!The imports for the new modules are correctly structured.
754-766: LGTM!The generate-export-file command is well-structured with proper option handling and clear action flow.
lib/liquidSamplerRunner.js (1)
111-111: No changes needed—the code is correct as written.The read methods have different signatures:
ReconciliationText.read()andAccountTemplate.read()are synchronous methods, so they should not be awaited.SharedPart.read()is async, so the await on line 169 is correct. The implementation in liquidSamplerRunner.js properly matches each method's return type.
| const { ChangelogReader } = require("./changelogReader"); | ||
|
|
||
| class CliUpdater { | ||
| static #UPDATE_COMMAND = `sudo npm install -g ${pkg.repository.url}`; |
There was a problem hiding this comment.
Cross-platform compatibility issue with sudo.
The hardcoded sudo command will fail on Windows systems where sudo is not available. Many Windows users also run npm commands without elevated privileges.
🔧 Proposed fix for cross-platform compatibility
Consider detecting the platform and adjusting the command accordingly, or simply remove sudo and let users handle permissions:
- static #UPDATE_COMMAND = `sudo npm install -g ${pkg.repository.url}`;
+ static #UPDATE_COMMAND = `npm install -g ${pkg.repository.url}`;If elevated privileges are needed, the command will fail with a clear permission error that users can resolve themselves.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static #UPDATE_COMMAND = `sudo npm install -g ${pkg.repository.url}`; | |
| static #UPDATE_COMMAND = `npm install -g ${pkg.repository.url}`; |
🤖 Prompt for AI Agents
In @lib/cli/cliUpdater.js at line 10, The UPDATE_COMMAND currently embeds a
hardcoded sudo which breaks on Windows; update static #UPDATE_COMMAND in
cliUpdater.js to be cross-platform by removing the leading "sudo" (or
conditionally prepending it only on non-Windows platforms), so the command
becomes just "npm install -g ${pkg.repository.url}" (or build it with a platform
check using process.platform !== 'win32' to add sudo on UNIX-like systems).
| if (!response) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Silent early return may confuse users.
If #generateInstance() returns falsy, the function exits silently at line 32 without informing the user why the process stopped. Users might not realize the operation failed.
Consider adding a log message:
const response = await this.#generateInstance();
if (!response) {
+ consola.error('Export file generation failed. Check previous error messages for details.');
return;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @lib/exportFileInstanceGenerator.js around lines 31 - 33, The early return in
generateInstance() silently exits when response is falsy; update the function
(generateInstance in exportFileInstanceGenerator.js) to log a clear warning or
error before returning (e.g., use the module's logger or console to state that
generateInstance returned no response and include any available context such as
input parameters or status) so callers/users can see why the process stopped;
then return or throw as appropriate.
| async #generateInstance() { | ||
| try { | ||
| const responseCreate = await SF.createExportFileInstance(this.firmId, this.companyId, this.periodId, this.exportFileId); | ||
| if (!responseCreate || !responseCreate.id) { | ||
| consola.error(`Failed to create export file instance. ${this.#details()}`); | ||
| return false; | ||
| } | ||
| const exportFileInstanceId = responseCreate.id; | ||
| consola.debug(`Export file instance created. ${this.#details(exportFileInstanceId)}`); | ||
|
|
||
| let response; | ||
| let attempts = 0; | ||
| let wait = this.#INITIAL_WAIT; | ||
|
|
||
| while (attempts < this.#MAX_ATTEMPTS) { | ||
| await new Promise((resolve) => setTimeout(resolve, wait)); | ||
|
|
||
| response = await SF.getExportFileInstance(this.firmId, this.companyId, this.periodId, exportFileInstanceId); | ||
|
|
||
| consola.debug(`Checking status of export file instance ${exportFileInstanceId}... Attempt ${attempts + 1} of ${this.#MAX_ATTEMPTS}`); | ||
|
|
||
| if (response && response.state === "pending") { | ||
| consola.debug(`Export file generation is still pending. ${this.#details(exportFileInstanceId)}`); | ||
| attempts++; | ||
| wait = Math.min(wait + 1000, this.#MAX_WAIT); | ||
| continue; | ||
| } else if (response && response.state === "created") { | ||
| consola.success(`Export file generation completed successfully. ${this.#details(exportFileInstanceId)}`); | ||
| return response; | ||
| } else { | ||
| consola.error(`Export file generation failed or encountered an unexpected state. ${this.#details(exportFileInstanceId)}`); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } catch (error) { | ||
| errorUtils.errorHandler(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent error handling with return false.
The function returns false at lines 47, 73, and 77 to indicate failure, but also uses errorUtils.errorHandler in the catch block (line 79). This creates confusion about how errors are communicated—some use return values, others throw/handle exceptions.
Consider a consistent approach:
- Option 1: Remove
return falsestatements and throw errors instead, letting the caller's catch block handle them - Option 2: Remove the try-catch wrapper and let errors propagate naturally to
generateAndOpenFile()
This would align with how other parts of the codebase handle errors through errorUtils.errorHandler.
🤖 Prompt for AI Agents
In @lib/exportFileInstanceGenerator.js around lines 42 - 81, The method
#generateInstance mixes returning false with exception handling; make error
signaling consistent by throwing errors instead of returning false: replace each
`return false` in #generateInstance with `throw new Error(...)` containing
context (use this.#details(exportFileInstanceId) where available), and update
the catch to call `errorUtils.errorHandler(error)` then rethrow the error so
callers like generateAndOpenFile can handle it; ensure success paths still
return the response as before.
|
|
||
| // Start sampler run | ||
| const samplerResponse = await SF.createSamplerRun(this.partnerId, samplerParams); | ||
| const samplerId = samplerResponse.data.id || samplerResponse.data; |
There was a problem hiding this comment.
Suspicious fallback logic for samplerId extraction.
The expression samplerResponse.data.id || samplerResponse.data will assign the entire data object to samplerId if data.id is falsy (0, null, undefined, false, empty string). This could lead to incorrect behavior when a non-string/non-number value is used as the sampler ID in subsequent API calls.
Recommended fix
If the API can return either { data: { id: "..." } } or { data: "..." }, make this explicit:
- const samplerId = samplerResponse.data.id || samplerResponse.data;
+ const samplerId = typeof samplerResponse.data === 'object'
+ ? samplerResponse.data.id
+ : samplerResponse.data;Otherwise, if id should always be present, remove the fallback and validate:
- const samplerId = samplerResponse.data.id || samplerResponse.data;
+ const samplerId = samplerResponse.data.id;
if (!samplerId) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const samplerId = samplerResponse.data.id || samplerResponse.data; | |
| const samplerId = typeof samplerResponse.data === 'object' | |
| ? samplerResponse.data.id | |
| : samplerResponse.data; |
🤖 Prompt for AI Agents
In @lib/liquidSamplerRunner.js at line 44, The fallback assigning samplerId
using "samplerResponse.data.id || samplerResponse.data" is unsafe; update the
logic that sets samplerId (the variable assigned from samplerResponse) to
explicitly handle the two supported shapes: if samplerResponse.data is an object
with a defined non-null id (check existence via !== undefined and !== null and
ensure type is string or number) use that id, else if samplerResponse.data is a
primitive string/number use it directly (check typeof), otherwise throw or
return a clear validation error so later API calls receive a valid samplerId.
lib/liquidSamplerRunner.js
Outdated
| if (response && response.result_url) { | ||
| await new UrlHandler(response.content_url).openFile(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the file to see the actual code at lines 233-234
head -240 lib/liquidSamplerRunner.js | tail -20Repository: silverfin/silverfin-cli
Length of output: 605
🏁 Script executed:
#!/bin/bash
# Search for both property names to understand their usage
rg "result_url|content_url" -n -C 2Repository: silverfin/silverfin-cli
Length of output: 949
🏁 Script executed:
#!/bin/bash
# Look for API response definitions or type definitions
rg "createSamplerRun|readSamplerRun" -A 10 -B 2Repository: silverfin/silverfin-cli
Length of output: 3768
🏁 Script executed:
#!/bin/bash
# Search for UrlHandler to understand what property it expects
rg "UrlHandler" -A 5 -B 2Repository: silverfin/silverfin-cli
Length of output: 2509
Fix property name mismatch in URL condition check.
Line 233 checks for response.result_url, but line 234 uses response.content_url. This causes the condition to pass while the actual property is undefined, leading to a failure in UrlHandler which requires a valid URL. Change line 233 to check response.content_url to match the property being accessed on line 234. This pattern is correctly implemented in exportFileInstanceGenerator.js (line 95-96).
🤖 Prompt for AI Agents
In @lib/liquidSamplerRunner.js around lines 233 - 234, The condition currently
checks the wrong property (response.result_url) before calling new
UrlHandler(response.content_url).openFile(); update the check to test
response.content_url instead so the guard matches the value actually accessed;
locate the block around the UrlHandler usage in liquidSamplerRunner.js and
replace the condition to ensure response.content_url is defined before
constructing UrlHandler.
| async #downloadFile() { | ||
| try { | ||
| const response = await axios.get(this.url, { responseType: "arraybuffer" }); | ||
| const contentDisposition = response.headers["content-disposition"]; | ||
| const fileExtension = contentDisposition ? this.#identifyExtension(contentDisposition) : ""; | ||
| const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}.${fileExtension}`); | ||
| fs.mkdirSync(path.dirname(tempFilePath), { recursive: true }); | ||
| fs.writeFileSync(tempFilePath, response.data); | ||
|
|
||
| return tempFilePath; | ||
| } catch (error) { | ||
| errorUtils.errorHandler(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error handling doesn't propagate errors correctly.
Line 42 catches errors and calls errorUtils.errorHandler(error), but doesn't rethrow or return anything. This causes openFile() at line 25 to receive undefined as filePath, which will fail when passed to open().
🐛 Proposed fix
async #downloadFile() {
try {
const response = await axios.get(this.url, { responseType: "arraybuffer" });
const contentDisposition = response.headers["content-disposition"];
const fileExtension = contentDisposition ? this.#identifyExtension(contentDisposition) : "";
const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}.${fileExtension}`);
fs.mkdirSync(path.dirname(tempFilePath), { recursive: true });
fs.writeFileSync(tempFilePath, response.data);
return tempFilePath;
} catch (error) {
errorUtils.errorHandler(error);
+ throw error; // Re-throw to propagate to openFile()
}
}🤖 Prompt for AI Agents
In @lib/utils/urlHandler.js around lines 31 - 44, The private async method
#downloadFile swallows exceptions by calling errorUtils.errorHandler(error)
without rethrowing or returning, causing callers like openFile to get undefined
filePath; update #downloadFile so after calling errorUtils.errorHandler(error)
it either rethrows the error (throw error) or returns a rejected promise (return
Promise.reject(error)) so the error propagates to openFile and upstream callers;
make this change inside the catch block of #downloadFile and keep the existing
errorUtils.errorHandler call for logging/handling.
|
|
||
| async #downloadFile() { | ||
| try { | ||
| const response = await axios.get(this.url, { responseType: "arraybuffer" }); |
There was a problem hiding this comment.
Potential memory issue with large export files.
Loading the entire file into memory as an arraybuffer could cause memory problems with large export files (e.g., large XBRL or CSV files). Consider using streaming for better memory efficiency.
♻️ Suggested approach using streaming
- const response = await axios.get(this.url, { responseType: "arraybuffer" });
+ const response = await axios.get(this.url, { responseType: "stream" });
const contentDisposition = response.headers["content-disposition"];
const fileExtension = contentDisposition ? this.#identifyExtension(contentDisposition) : "";
const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}.${fileExtension}`);
fs.mkdirSync(path.dirname(tempFilePath), { recursive: true });
- fs.writeFileSync(tempFilePath, response.data);
+ const writer = fs.createWriteStream(tempFilePath);
+ response.data.pipe(writer);
+ await new Promise((resolve, reject) => {
+ writer.on('finish', resolve);
+ writer.on('error', reject);
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @lib/utils/urlHandler.js at line 33, The axios call currently reads the whole
payload into memory using responseType: "arraybuffer" which can OOM on large
exports; change the axios.get(this.url, { responseType: "arraybuffer" }) call to
use streaming (responseType: "stream"), then consume the returned
response.stream by piping it to a temporary file or processing it via streaming
transforms (e.g., response.data.pipe(fs.createWriteStream(tempPath)) or using a
stream parser), ensure you handle backpressure/errors and cleanup the temp file,
and update any downstream code that expects a Buffer to instead read from the
streamed file or stream APIs.
| fs.mkdirSync(path.dirname(tempFilePath), { recursive: true }); | ||
| fs.writeFileSync(tempFilePath, response.data); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use async file operations instead of synchronous methods.
Using synchronous file operations (mkdirSync, writeFileSync) in an async function blocks the event loop. Replace them with their async equivalents for better performance.
♻️ Proposed refactor
- fs.mkdirSync(path.dirname(tempFilePath), { recursive: true });
- fs.writeFileSync(tempFilePath, response.data);
+ await fs.promises.mkdir(path.dirname(tempFilePath), { recursive: true });
+ await fs.promises.writeFile(tempFilePath, response.data);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs.mkdirSync(path.dirname(tempFilePath), { recursive: true }); | |
| fs.writeFileSync(tempFilePath, response.data); | |
| await fs.promises.mkdir(path.dirname(tempFilePath), { recursive: true }); | |
| await fs.promises.writeFile(tempFilePath, response.data); |
🤖 Prompt for AI Agents
In @lib/utils/urlHandler.js around lines 37 - 38, The code in urlHandler.js is
using blocking fs.mkdirSync and fs.writeFileSync inside an async flow; replace
them with non-blocking async calls (e.g., use fs.promises.mkdir or import from
'fs/promises' and await mkdir(path.dirname(tempFilePath), { recursive: true })
and await writeFile(tempFilePath, response.data)) and propagate/handle errors
appropriately so tempFilePath is created and written without blocking the event
loop; update any surrounding try/catch in the function that fetches
response.data to await these async ops.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/liquidSamplerRunner.js`:
- Around line 200-201: The polling code calls SF.readSamplerRun(this.partnerId,
samplerId) and assigns response.data to samplerRun without null-safety; add a
defensive check after the await to verify response and response.data exist
(e.g., if (!response || !response.data) { handle error/retry/continue }) before
using samplerRun, and ensure any subsequent logic that expects samplerRun (the
variable assigned from response.data) handles the missing case gracefully;
update the block around SF.readSamplerRun, response, and samplerRun to use this
guard.
🧹 Nitpick comments (2)
lib/liquidSamplerRunner.js (1)
204-204: Remove commented-out code.The commented exponential backoff
// pollingDelay *= 1.05;should be removed if not needed, or implemented if intended. Leaving dead code creates maintenance burden.♻️ Proposed fix
waitingTime += pollingDelay; - // pollingDelay *= 1.05; if (waitingTime >= waitingLimit) {bin/cli.js (1)
508-516: Consider removing duplicate validation.The template validation (lines 513-516) duplicates the validation already present in
LiquidSamplerRunner.run()(lines 33-36). While defensive, maintaining validation in two places risks divergence. Consider letting the runner handle this validation.♻️ Option: Remove CLI-side validation
- // Validate: at least one template specified - const handles = options.handle || []; - const accountTemplates = options.accountTemplate || []; - const sharedParts = options.sharedPart || []; - - if (handles.length === 0 && accountTemplates.length === 0 && sharedParts.length === 0) { - consola.error("You need to specify at least one template using -h, -at, or -s"); - process.exit(1); - } const templateHandles = { - reconciliationTexts: handles, - accountTemplates: accountTemplates, - sharedParts: sharedParts, + reconciliationTexts: options.handle || [], + accountTemplates: options.accountTemplate || [], + sharedParts: options.sharedPart || [], };
AgustinSilverfin
force-pushed
the
agustin-bso-sampler
branch
from
b171592 to
d3c099d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/liquidSamplerRunner.js`:
- Around line 188-193: The JSDoc for fetchAndWaitSamplerResult is stale: it
lists a partnerId parameter that the function does not accept; update the
docblock above fetchAndWaitSamplerResult in lib/liquidSamplerRunner.js to remove
the incorrect `@param` {number} partnerId entry and ensure the `@param` entries
match the actual signature (e.g., `@param` {string} samplerId and any other real
params) and keep the `@returns` description accurate so the comment reflects the
real function signature.
| /** | ||
| * Poll for sampler run completion | ||
| * @param {number} partnerId - The partner ID | ||
| * @param {string} samplerId - The sampler run ID | ||
| * @returns {Promise<Object>} The completed sampler run | ||
| */ |
There was a problem hiding this comment.
Fix stale JSDoc param in #fetchAndWaitSamplerResult.
The doc block lists a partnerId param that the method doesn’t accept.
✏️ Suggested doc fix
/**
* Poll for sampler run completion
- * `@param` {number} partnerId - The partner ID
* `@param` {string} samplerId - The sampler run ID
* `@returns` {Promise<Object>} The completed sampler run
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Poll for sampler run completion | |
| * @param {number} partnerId - The partner ID | |
| * @param {string} samplerId - The sampler run ID | |
| * @returns {Promise<Object>} The completed sampler run | |
| */ | |
| /** | |
| * Poll for sampler run completion | |
| * `@param` {string} samplerId - The sampler run ID | |
| * `@returns` {Promise<Object>} The completed sampler run | |
| */ |
🤖 Prompt for AI Agents
In `@lib/liquidSamplerRunner.js` around lines 188 - 193, The JSDoc for
fetchAndWaitSamplerResult is stale: it lists a partnerId parameter that the
function does not accept; update the docblock above fetchAndWaitSamplerResult in
lib/liquidSamplerRunner.js to remove the incorrect `@param` {number} partnerId
entry and ensure the `@param` entries match the actual signature (e.g., `@param`
{string} samplerId and any other real params) and keep the `@returns` description
accurate so the comment reflects the real function signature.
d3c099d to
346af6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bin/cli.js`:
- Around line 491-527: options.firmIds are strings from Commander but
LiquidSamplerRunner.run expects Array<number>; before constructing firmIds pass
options.firmIds through a numeric normalization step (map to Number or parseInt)
and filter/validate out non-numeric entries, failing with a clear consola.error
and process.exit(1) if any provided ID is not a valid number; update the firmIds
assignment in the run-sampler handler (referencing options.firmIds and the
firmIds variable) so you call new
LiquidSamplerRunner(options.partner).run(templateHandles, firmIds) with an
Array<number>.
In `@lib/liquidSamplerRunner.js`:
- Around line 117-154: ReconciliationText.read and AccountTemplate.read are
being used synchronously and can return Promises or falsy values; change the
calls in the account and reconciliation template loops to await
ReconciliationText.read(handle) and await AccountTemplate.read(name), and after
awaiting validate the result (e.g., if (!templateContent ||
!templateContent.text) { consola.error(...); process.exit(1); }) before
accessing templateContent.text or templateContent.text_parts; use the same
defensive pattern as SharedPart.read to avoid Promise deref and runtime crashes.
| // Run Liquid Sampler | ||
| program | ||
| .command("run-sampler") | ||
| .description("Run Liquid Sampler for partner templates (reconciliation texts, account detail templates, and/or shared parts)") | ||
| .requiredOption("-p, --partner <partner-id>", "Specify the partner to be used") | ||
| .option("-h, --handle <handles...>", "Specify reconciliation text handle(s) - can specify multiple") | ||
| .option("-at, --account-template <names...>", "Specify account detail template name(s) - can specify multiple") | ||
| .option("-s, --shared-part <names...>", "Specify shared part name(s) - can specify multiple") | ||
| .option("--firm-ids <firm-ids...>", "Specify firm ID(s) to run the sampler against - can specify multiple (optional)") | ||
| .option("--id <sampler-id>", "Specify an existing sampler ID to fetch results for (optional)") | ||
| .action(async (options) => { | ||
| // If an existing sampler ID is provided, fetch and display results | ||
| if (options.id) { | ||
| await new LiquidSamplerRunner(options.partner).checkStatus(options.id); | ||
| return; | ||
| } | ||
|
|
||
| // Validate: at least one template specified | ||
| const handles = options.handle || []; | ||
| const accountTemplates = options.accountTemplate || []; | ||
| const sharedParts = options.sharedPart || []; | ||
|
|
||
| if (handles.length === 0 && accountTemplates.length === 0 && sharedParts.length === 0) { | ||
| consola.error("You need to specify at least one template using -h, -at, or -s"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const templateHandles = { | ||
| reconciliationTexts: handles, | ||
| accountTemplates: accountTemplates, | ||
| sharedParts: sharedParts, | ||
| }; | ||
|
|
||
| const firmIds = options.firmIds || []; | ||
|
|
||
| await new LiquidSamplerRunner(options.partner).run(templateHandles, firmIds); | ||
| }); |
There was a problem hiding this comment.
Normalize firmIds to numbers before calling the runner.
options.firmIds from Commander are strings, but LiquidSamplerRunner expects Array<number> (and likely the API does too). Convert and validate before passing.
🛠️ Proposed fix
- const firmIds = options.firmIds || [];
+ const firmIdsRaw = options.firmIds || [];
+ const firmIds = firmIdsRaw.map((id) => Number(id));
+
+ if (firmIds.some((id) => Number.isNaN(id))) {
+ consola.error("All --firm-ids values must be valid numbers");
+ process.exit(1);
+ }
await new LiquidSamplerRunner(options.partner).run(templateHandles, firmIds);🤖 Prompt for AI Agents
In `@bin/cli.js` around lines 491 - 527, options.firmIds are strings from
Commander but LiquidSamplerRunner.run expects Array<number>; before constructing
firmIds pass options.firmIds through a numeric normalization step (map to Number
or parseInt) and filter/validate out non-numeric entries, failing with a clear
consola.error and process.exit(1) if any provided ID is not a valid number;
update the firmIds assignment in the run-sampler handler (referencing
options.firmIds and the firmIds variable) so you call new
LiquidSamplerRunner(options.partner).run(templateHandles, firmIds) with an
Array<number>.
| const templateId = config.partner_id[this.partnerId]; | ||
| const templateContent = ReconciliationText.read(handle); | ||
|
|
||
| templates.push({ | ||
| type: "Global::Partner::ReconciliationText", | ||
| id: String(templateId), | ||
| text: templateContent.text, | ||
| text_parts: templateContent.text_parts, | ||
| }); | ||
| } | ||
|
|
||
| // Process account templates | ||
| for (const name of accountTemplates) { | ||
| const templateType = "accountTemplate"; | ||
| const configPresent = fsUtils.configExists(templateType, name); | ||
|
|
||
| if (!configPresent) { | ||
| consola.error(`Config file for account template "${name}" not found`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const config = fsUtils.readConfig(templateType, name); | ||
|
|
||
| // Validate partner_id exists in config | ||
| if (!config.partner_id || !config.partner_id[this.partnerId]) { | ||
| consola.error(`Template '${name}' has no partner_id entry for partner ${this.partnerId}. Import the template to this partner first.`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const templateId = config.partner_id[this.partnerId]; | ||
| const templateContent = AccountTemplate.read(name); | ||
|
|
||
| templates.push({ | ||
| type: "Global::Partner::AccountDetailTemplate", | ||
| id: String(templateId), | ||
| text: templateContent.text, | ||
| text_parts: templateContent.text_parts, | ||
| }); |
There was a problem hiding this comment.
Await template reads (and handle falsy content) to avoid Promise deref.
ReconciliationText.read and AccountTemplate.read are called without await, while SharedPart.read is awaited. If these methods are async (as tests elsewhere suggest), templateContent.text will fail at runtime.
🛠️ Proposed fix
- const templateContent = ReconciliationText.read(handle);
+ const templateContent = await ReconciliationText.read(handle);
+ if (!templateContent) {
+ consola.error(`Reconciliation text "${handle}" wasn't found`);
+ process.exit(1);
+ }
@@
- const templateContent = AccountTemplate.read(name);
+ const templateContent = await AccountTemplate.read(name);
+ if (!templateContent) {
+ consola.error(`Account template "${name}" wasn't found`);
+ process.exit(1);
+ }🤖 Prompt for AI Agents
In `@lib/liquidSamplerRunner.js` around lines 117 - 154, ReconciliationText.read
and AccountTemplate.read are being used synchronously and can return Promises or
falsy values; change the calls in the account and reconciliation template loops
to await ReconciliationText.read(handle) and await AccountTemplate.read(name),
and after awaiting validate the result (e.g., if (!templateContent ||
!templateContent.text) { consola.error(...); process.exit(1); }) before
accessing templateContent.text or templateContent.text_parts; use the same
defensive pattern as SharedPart.read to avoid Promise deref and runtime crashes.
346af6b to
57a4508
Compare
1 participant
Fixes # (link to the corresponding issue if applicable)
Description
Include a summary of the changes made
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist