Add get-results command (read back live results/customs as JSON)#258
Closed
Benjvandam wants to merge 1 commit into
Closed
Add get-results command (read back live results/customs as JSON)#258Benjvandam wants to merge 1 commit into
Benjvandam wants to merge 1 commit into
Conversation
New get-results -u <url> [-o <file>] reads back a live company file's computed reconciliation/account results and custom data as JSON — the verify/read-back half of the live-iteration loop. Reuses extractURL and the existing getReconciliationResults/Custom (and account-template) getters. Adds lib/resultsReader.js + tests.
WalkthroughAdds a Changesget-results CLI command
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
tests/lib/resultsReader.test.js (1)
64-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for unsupported template URLs too.
The new default branch in
lib/resultsReader.jsis still untested. Since the CLI treatsnullas the command-failure contract, it is worth locking down the unsupported-template path the same way you did for unresolved accounts.🧪 Proposed test
describe("resultsReader.fetchResults", () => { beforeEach(() => { jest.clearAllMocks(); }); @@ it("returns null and logs an error when the account cannot be resolved", async () => { Utils.extractURL.mockReturnValue({ templateType: "accountTemplate", firmId: "96", companyId: "100", ledgerId: "200", accountId: "999999", }); SF.findAccountByNumber.mockResolvedValue(null); const result = await fetchResults("https://live.getsilverfin.com/f/96/100/..."); expect(result).toBeNull(); expect(consola.error).toHaveBeenCalled(); expect(SF.getAccountTemplateResults).not.toHaveBeenCalled(); }); + + it("returns null and logs an error for unsupported template URLs", async () => { + Utils.extractURL.mockReturnValue({ + templateType: "exportFile", + }); + + const result = await fetchResults("https://live.getsilverfin.com/f/96/100/..."); + + expect(result).toBeNull(); + expect(consola.error).toHaveBeenCalled(); + expect(SF.getReconciliationResults).not.toHaveBeenCalled(); + expect(SF.getAccountTemplateResults).not.toHaveBeenCalled(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/lib/resultsReader.test.js` around lines 64 - 79, The current tests cover unresolved accounts in fetchResults but do not exercise the new unsupported-template default branch in resultsReader. Add a test in resultsReader.test.js that uses Utils.extractURL to return an unsupported templateType, calls fetchResults, and asserts it returns null, logs an error via consola.error, and does not call any SF template result fetcher such as SF.getAccountTemplateResults.lib/resultsReader.js (1)
20-21: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the result/custom fetches in parallel.
These pairs of API calls are independent, so awaiting them sequentially adds one extra round-trip to every
get-resultsinvocation.♻️ Proposed refactor
case "reconciliationText": { - const customResponse = await SF.getReconciliationCustom("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, parameters.reconciliationId); - const resultsResponse = await SF.getReconciliationResults("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, parameters.reconciliationId); + const [customResponse, resultsResponse] = await Promise.all([ + SF.getReconciliationCustom("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, parameters.reconciliationId), + SF.getReconciliationResults("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, parameters.reconciliationId), + ]); return { templateType: parameters.templateType, firmId: parameters.firmId, @@ case "accountTemplate": { const account = await SF.findAccountByNumber(parameters.firmId, parameters.companyId, parameters.ledgerId, parameters.accountId); const accountId = account?.account?.id; if (!accountId) { consola.error(`Account "${parameters.accountId}" could not be resolved in this company file.`); return null; } - const customResponse = await SF.getAccountTemplateCustom("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, accountId); - const resultsResponse = await SF.getAccountTemplateResults("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, accountId); + const [customResponse, resultsResponse] = await Promise.all([ + SF.getAccountTemplateCustom("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, accountId), + SF.getAccountTemplateResults("firm", parameters.firmId, parameters.companyId, parameters.ledgerId, accountId), + ]); return { templateType: parameters.templateType, firmId: parameters.firmId,Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/resultsReader.js` around lines 20 - 21, The sequential awaits in the results reader slow down each get-results call because getReconciliationCustom and getReconciliationResults are independent. Update the logic in the resultsReader flow that fetches the reconciliation custom and results payloads to start both requests together and then await them together, so the pair runs in parallel while preserving the existing response handling for both calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/cli.js`:
- Around line 537-547: The command action in results handling can throw during
resultsReader.fetchResults or fs.writeFileSync, which currently bypasses the
existing !data guard. Wrap the body of the action in a try/catch, keep the
existing process.exitCode = 1 behavior for failures, and add a concise error
message through the command’s logger (consola) before returning. Use the action
callback in bin/cli.js and the resultsReader.fetchResults / fs.writeFileSync
flow as the main points to update.
---
Nitpick comments:
In `@lib/resultsReader.js`:
- Around line 20-21: The sequential awaits in the results reader slow down each
get-results call because getReconciliationCustom and getReconciliationResults
are independent. Update the logic in the resultsReader flow that fetches the
reconciliation custom and results payloads to start both requests together and
then await them together, so the pair runs in parallel while preserving the
existing response handling for both calls.
In `@tests/lib/resultsReader.test.js`:
- Around line 64-79: The current tests cover unresolved accounts in fetchResults
but do not exercise the new unsupported-template default branch in
resultsReader. Add a test in resultsReader.test.js that uses Utils.extractURL to
return an unsupported templateType, calls fetchResults, and asserts it returns
null, logs an error via consola.error, and does not call any SF template result
fetcher such as SF.getAccountTemplateResults.
🪄 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: CHILL
Plan: Pro
Run ID: bb664d21-801b-4510-80d3-9e4e636bbd78
📒 Files selected for processing (4)
CHANGELOG.mdbin/cli.jslib/resultsReader.jstests/lib/resultsReader.test.js
Comment on lines
+537
to
+547
| .action(async (options) => { | ||
| const data = await resultsReader.fetchResults(options.url); | ||
| if (!data) { | ||
| process.exitCode = 1; | ||
| return; | ||
| } | ||
| const json = JSON.stringify(data, null, 2); | ||
| if (options.output) { | ||
| const fs = require("fs"); | ||
| fs.writeFileSync(options.output, json); | ||
| consola.success(`Wrote results to ${options.output}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle fetch/write failures inside the command action.
Line 538 and Line 546 can both throw. Right now those failures bypass the !data path and abort the command with an uncaught error instead of a controlled non-zero exit plus a concise message.
🛠️ Proposed fix
.option("-o, --output <file>", "Write the JSON to a file instead of stdout (optional)")
.action(async (options) => {
- const data = await resultsReader.fetchResults(options.url);
- if (!data) {
- process.exitCode = 1;
- return;
- }
- const json = JSON.stringify(data, null, 2);
- if (options.output) {
- const fs = require("fs");
- fs.writeFileSync(options.output, json);
- consola.success(`Wrote results to ${options.output}`);
- } else {
- console.log(json);
- }
+ try {
+ const data = await resultsReader.fetchResults(options.url);
+ if (!data) {
+ process.exitCode = 1;
+ return;
+ }
+ const json = JSON.stringify(data, null, 2);
+ if (options.output) {
+ const fs = require("fs");
+ fs.writeFileSync(options.output, json);
+ consola.success(`Wrote results to ${options.output}`);
+ } else {
+ console.log(json);
+ }
+ } catch (error) {
+ consola.error(error?.message ?? error);
+ process.exitCode = 1;
+ }
});📝 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.
Suggested change
| .action(async (options) => { | |
| const data = await resultsReader.fetchResults(options.url); | |
| if (!data) { | |
| process.exitCode = 1; | |
| return; | |
| } | |
| const json = JSON.stringify(data, null, 2); | |
| if (options.output) { | |
| const fs = require("fs"); | |
| fs.writeFileSync(options.output, json); | |
| consola.success(`Wrote results to ${options.output}`); | |
| .action(async (options) => { | |
| try { | |
| const data = await resultsReader.fetchResults(options.url); | |
| if (!data) { | |
| process.exitCode = 1; | |
| return; | |
| } | |
| const json = JSON.stringify(data, null, 2); | |
| if (options.output) { | |
| const fs = require("fs"); | |
| fs.writeFileSync(options.output, json); | |
| consola.success(`Wrote results to ${options.output}`); | |
| } else { | |
| console.log(json); | |
| } | |
| } catch (error) { | |
| consola.error(error?.message ?? error); | |
| process.exitCode = 1; | |
| } | |
| }); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 545-545: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(options.output, json)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/cli.js` around lines 537 - 547, The command action in results handling
can throw during resultsReader.fetchResults or fs.writeFileSync, which currently
bypasses the existing !data guard. Wrap the body of the action in a try/catch,
keep the existing process.exitCode = 1 behavior for failures, and add a concise
error message through the command’s logger (consola) before returning. Use the
action callback in bin/cli.js and the resultsReader.fetchResults /
fs.writeFileSync flow as the main points to update.
Contributor
Author
|
Consolidated into #260 (one PR for all four live-bridge commands). Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 1 of the live-bridge initiative (capture → manipulate → verify → push, without Playwright).
What
New
get-results -u <url> [-o <file>]: fetches the computed results and custom data of a reconciliation or account in a live company file and prints them as JSON. This is the read-back / verify half of the iterate-on-live-data loop.How
lib/resultsReader.jsfetchResults(url)reusesextractURLand the existinggetReconciliationResults/getReconciliationCustomgetters (account variants viafindAccountByNumber+getAccountTemplateResults/Custom).bin/cli.js; JSON to stdout, or--outputto a file.Tests
tests/lib/resultsReader.test.jscovers the reconciliation, account, and unresolved-account paths.Read-only command (no writes).