feat: new command to generate export file instances #221
Conversation
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
Couple of questions about adding returns in certain conditions, but apart from that, all good! Very nice addition!
AgustinSilverfin
force-pushed
the
agustin-generate-export-file
branch
from
6f95fb4 to
ebee28c
Compare
|
Warning Rate limit exceeded@AgustinSilverfin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
WalkthroughAdds a new CLI command Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. 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 |
AgustinSilverfin
force-pushed
the
agustin-generate-export-file
branch
2 times, most recently
from
ca842af to
f603cb1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @bin/cli.js:
- Around line 718-729: The CLI command for "generate-export-file" calls
ExportFileInstanceGenerator.generateAndOpenFile() but doesn't await its
asynchronous completion; update the command handler so the .action callback
awaits the async method (e.g., change the call to await
generator.generateAndOpenFile()) and ensure the action is declared async
(already is) and propagate or catch errors (handle rejections) so the process
does not exit before the operation completes and any errors are surfaced.
In @CHANGELOG.md:
- Line 7: In the 1.50.0 changelog sentence describing `silverfin
generate-export-file`, fix the grammar by changing both instances of "etc" to
"etc." (ensure the period appears before the closing parenthesis where
applicable, e.g., "CSV, etc.)") and correct the typo "runnig" to "running",
making sure the sentence ends with a final period.
In @lib/exportFileInstanceGenerator.js:
- Around line 52-71: The polling loop in #generateInstance (the while using
this.#MAX_ATTEMPTS) can fall through and return undefined when all attempts stay
"pending"; add a clear failure return after the loop: after the while block log
an error with consola.error (including this.#details(exportFileInstanceId)) and
return false (or throw if you prefer), ensuring callers like
#logValidationErrors and #openUrl receive a defined failure value instead of
undefined.
- Around line 88-94: The private method #openUrl currently calls
UrlHandler.openFile() without awaiting it, which can let the process exit before
the async file open/download completes; change the call in #openUrl to await new
UrlHandler(response.content_url).openFile() and mark #openUrl as async if it
isn't, and update the callers (notably generateAndOpenFile()) to await
this.#openUrl(...) so the async flow is properly propagated.
In @lib/utils/urlHandler.js:
- Line 35: Remove the leftover debug statement by deleting the
console.log(response) call in lib/utils/urlHandler.js (the stray
console.log(response) that prints the full Axios response object); if needed,
replace it with an appropriate logger.debug or a more specific log (e.g.,
logging response.status or response.data) consistent with existing logging in
the module, but do not leave console.log(response) in production code.
- Around line 31-44: In #downloadFile, fix binary corruption and potential
TypeError by requesting the response as binary and guarding the header: call
axios.get(this.url, { responseType: "arraybuffer" }) (or "stream" if preferred),
pass (response.headers["content-disposition"] || "") into #identifyExtension so
it never receives undefined, and write the file as binary (e.g.,
fs.writeFileSync(tempFilePath, Buffer.from(response.data))) instead of relying
on default text handling; also remove the stray console.log(response).
🧹 Nitpick comments (4)
bin/cli.js (1)
722-725: Consider validating firm ID and using consistent option shorthands.
- Unlike other commands, this doesn't call
cliUtils.checkDefaultFirm()to validate the firm option.- The
-pshorthand is used for--periodhere but for--partnerin all other commands, which may confuse users.♻️ Suggested improvements
.command("generate-export-file") .description("Create a new export file instance from an export file template (e.g. (i)XBRL)") .option("-f, --firm <firm-id>", "Specify the firm to be used", firmIdDefault) .requiredOption("-c, --company <company-id>", "Specify the company to be used") - .requiredOption("-p, --period <period-id>", "Specify the period to be used") + .requiredOption("--period <period-id>", "Specify the period to be used") .requiredOption("-e, --export-file <export-file-id>", "Specify the export file template to be used") .action(async (options) => { + cliUtils.checkDefaultFirm(options.firm, firmIdDefault); const generator = new ExportFileInstanceGenerator(options.firm, options.company, options.period, options.exportFile); await generator.generateAndOpenFile(); });lib/utils/urlHandler.js (2)
46-49: Remove unnecessaryasyncand improve robustness.This method performs no async operations and should be synchronous. The regex also won't handle all content-disposition formats (e.g.,
filename*=UTF-8''...encoding).♻️ Proposed fix
- async #identifyExtension(url) { - const match = url.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['"]/i); + #identifyExtension(contentDisposition) { + if (!contentDisposition) return null; + const match = contentDisposition.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['"]/i); return match ? match[1] : null; }
36-36: Consider including a meaningful filename.Currently the file is saved as just a timestamp (e.g.,
1704672000000.xml). Consider extracting the original filename from the content-disposition header to provide a more meaningful name for users.lib/exportFileInstanceGenerator.js (1)
28-36: Consider short-circuiting on failure.If
#generateInstance()returnsfalseorundefined, the code still proceeds to call#logValidationErrorsand#openUrl, which will just log an error. Consider returning early on failure for cleaner flow.♻️ Suggested improvement
async generateAndOpenFile() { try { const response = await this.#generateInstance(); + if (!response) { + return; + } this.#logValidationErrors(response); - this.#openUrl(response); + await this.#openUrl(response); } catch (error) { errorUtils.errorHandler(error); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/cli/cliUpdater.jslib/exportFileInstanceGenerator.jslib/utils/urlHandler.jspackage.json
🧰 Additional context used
🧬 Code graph analysis (2)
lib/utils/urlHandler.js (1)
lib/exportFileInstanceGenerator.js (3)
require(2-2)require(4-4)errorUtils(3-3)
lib/api/sfApi.js (2)
lib/liquidTestRunner.js (1)
response(209-209)index.js (15)
response(151-151)response(187-187)response(220-220)response(351-351)response(386-386)response(418-418)response(548-548)response(589-589)response(628-628)response(679-679)response(753-753)response(788-788)response(821-821)response(887-887)response(1045-1045)
🪛 GitHub Actions: CLI version check
package.json
[error] 1-1: Version check failed: current branch version (1.50.0) must be greater than main branch version (1.49.0).
🪛 LanguageTool
CHANGELOG.md
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...on of export files (XBRLs, iXBRLs, CSV, etc) with the CLI. This could be used as pa...
(ETC_PERIOD)
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...ault application (browser, text editor, etc). See more details on how to use it by ...
(ETC_PERIOD)
[grammar] ~7-~7: Ensure spelling is correct
Context: ...). See more details on how to use it by runnig silverfin generate-export-file --help...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (5)
lib/cli/cliUpdater.js (1)
7-8: LGTM! Minor formatting adjustment adds spacing after imports, which improves readability.package.json (1)
3-3: Version bump is correct, but pipeline failure needs investigation.The version change from 1.49.0 → 1.50.0 is appropriate for a new feature and satisfies the semantic versioning constraint. However, the GitHub Actions pipeline reports: "Version check failed: current branch version (1.50.0) must be greater than main branch version (1.49.0)."
Since 1.50.0 is greater than 1.49.0, this appears to be either a pipeline configuration issue or a timing issue with the main branch version cache. Verify that the pipeline passes after merge or check the version-check workflow configuration.
CHANGELOG.md (1)
9-15: Changelog formatting looks good. The adjustments to the 1.49.0 and 1.48.0 entries maintain consistency with existing entries and improve clarity.lib/api/sfApi.js (1)
644-666: LGTM!Both
createExportFileInstanceandgetExportFileInstancefollow the established patterns in this file for API calls, error handling, and response processing. The implementation is consistent with similar functions likereadExportFileById.lib/exportFileInstanceGenerator.js (1)
10-12: LGTM on polling configuration.The exponential-like backoff (1s initial, +1s per attempt, max 5s) with 25 max attempts provides a reasonable ~75-125 second total wait time for export file generation.
AgustinSilverfin
force-pushed
the
agustin-generate-export-file
branch
from
f603cb1 to
9a59736
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @bin/cli.js:
- Around line 718-729: The action handler invokes
ExportFileInstanceGenerator.generateAndOpenFile() without awaiting its async
result; update the action to await the call (e.g., await
generator.generateAndOpenFile()) so the CLI waits for completion, and wrap the
await in a try/catch to log errors and set a non-zero exit code if the async
operation fails—look for the program.command("generate-export-file") block and
the ExportFileInstanceGenerator.generateAndOpenFile reference to apply the
change.
In @CHANGELOG.md:
- Line 7: Update the changelog sentence describing the new CLI command
`silverfin generate-export-file`: correct the two occurrences of "etc" to "etc."
and fix the typo "runnig" to "running" so the paragraph reads with proper
punctuation and spelling.
📜 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 (7)
CHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/cli/cliUpdater.jslib/exportFileInstanceGenerator.jslib/utils/urlHandler.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/utils/urlHandler.js
- lib/exportFileInstanceGenerator.js
- lib/cli/cliUpdater.js
🧰 Additional context used
🪛 GitHub Actions: CLI version check
CHANGELOG.md
[error] 1-1: Changelog entry for version 1.50.0 is missing. The CI step that verifies the changelog failed with exit code 1.
🪛 LanguageTool
CHANGELOG.md
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...on of export files (XBRLs, iXBRLs, CSV, etc) with the CLI. This could be used as pa...
(ETC_PERIOD)
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...ault application (browser, text editor, etc). See more details on how to use it by ...
(ETC_PERIOD)
[grammar] ~7-~7: Ensure spelling is correct
Context: ...). See more details on how to use it by runnig silverfin generate-export-file --help...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (7)
package.json (1)
3-3: LGTM!The version bump to 1.50.0 is appropriate for the new export file generation feature.
lib/api/sfApi.js (3)
644-654: LGTM!The
createExportFileInstancefunction follows the established patterns in this module, with consistent error handling and response structure.
656-666: LGTM!The
getExportFileInstancefunction is correctly implemented and follows the module's conventions.
717-718: LGTM!The new functions are properly exported and integrated into the module's public API.
bin/cli.js (1)
6-6: LGTM!The import statement correctly brings in the
ExportFileInstanceGeneratorclass.CHANGELOG.md (2)
5-5: Verify the release date.The changelog entry is dated "07/01/2026", but the PR was created on 2025-09-10. Please confirm this date is correct.
5-8: Remove this comment — the changelog entry is correctly formatted.The actual CHANGELOG.md entry is
## [1.50.0] (07/01/2026)with square brackets, which matches the CI validation requirement. The snippet shown in your review is incorrect; it displays the version without brackets. No CI failure exists for this entry.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @bin/cli.js:
- Around line 726-729: The CLI action is calling the async method
generateAndOpenFile() on ExportFileInstanceGenerator without awaiting it, which
can let the process exit before completion; update the .action handler to await
generator.generateAndOpenFile() (e.g., await generator.generateAndOpenFile();)
and optionally wrap it in a try/catch to propagate or log errors from the async
call so failures aren’t lost.
📜 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 (7)
CHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/cli/cliUpdater.jslib/exportFileInstanceGenerator.jslib/utils/urlHandler.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/cli/cliUpdater.js
- lib/exportFileInstanceGenerator.js
- lib/utils/urlHandler.js
- package.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...on of export files (XBRLs, iXBRLs, CSV, etc) with the CLI. This could be used as pa...
(ETC_PERIOD)
[style] ~7-~7: In American English, abbreviations like “etc.” require a period.
Context: ...ault application (browser, text editor, etc). See more details on how to use it by ...
(ETC_PERIOD)
[grammar] ~7-~7: Ensure spelling is correct
Context: ...). See more details on how to use it by runnig silverfin generate-export-file --help...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (5)
lib/api/sfApi.js (3)
644-654: LGTM!The function follows the established pattern in this file: creates a firm instance, makes a POST request to the correct endpoint, handles success/error cases, and returns the appropriate data structure.
656-666: LGTM!The function correctly follows the established pattern: creates a firm instance, makes a GET request to fetch the specific export file instance, handles success/error cases, and returns the data structure.
717-718: LGTM!The new functions are correctly added to the module exports, maintaining consistency with the existing export structure.
bin/cli.js (1)
6-6: LGTM!The import statement correctly brings in the
ExportFileInstanceGeneratorclass using destructuring, consistent with the file's import patterns.CHANGELOG.md (1)
7-7: Fix grammar and spelling issues (already flagged).The changelog description needs corrections as previously noted:
- "etc" should be "etc." (appears twice)
- "runnig" should be "running"
Likely an incorrect or invalid review comment.
AgustinSilverfin
force-pushed
the
agustin-generate-export-file
branch
from
9a59736 to
d4ab1df
Compare
AgustinSilverfin
force-pushed
the
agustin-generate-export-file
branch
from
d4ab1df to
10eef46
Compare
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
Two nitpick questions left, but approving already since these are not blocking/wrong
Fixes # (link to the corresponding issue if applicable)
Description
Include a summary of the changes made
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist