Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (1)
bin/cli.js (1)
540-567: Consider adding URL validation and more detailed error output.Two minor observations:
If
liquidTestUtils.extractURLfails to parse an invalid URL, the error may not be user-friendly. Consider validating the parsedurlDatabefore proceeding.On upload failure (line 564), consider logging the response details to help with debugging:
🔧 Optional: Improve error messaging
// Upload to Silverfin const response = await SF.updateReconciliationCustom(firmId, companyId, periodId, reconciliationId, properties); if (response && response.status >= 200 && response.status < 300) { consola.success("Text properties updated successfully"); } else { - consola.error("Failed to update text properties"); + consola.error("Failed to update text properties", response?.data || response?.statusText || ""); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli.js` around lines 540 - 567, Wrap the call to liquidTestUtils.extractURL in a try/catch and validate the returned urlData (check that firmId, companyId, periodId and reconciliationId are present) and log a clear user-facing error and exit if parsing fails or required IDs are missing; after calling SF.updateReconciliationCustom, enhance the failure branch to log detailed response information (e.g., response.status, response.data or response.error) before calling process.exit(1) so failures are easier to debug — reference extractURL, urlData (firmId, companyId, periodId, reconciliationId), findTestData, transformCustomToProperties, SF.updateReconciliationCustom and response when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/utils/textPropertyUtils.js`:
- Around line 27-37: The code currently has inconsistent behavior when mixing
simple keys ("ns.key") and nested keys ("ns.key.sub") because namespaceMap
entries are set once and later keys either get ignored or overwrite the value;
update the logic in the block that handles keyParts and namespaceMap so
conflicts are merged deterministically: whenever adding a nested key
(keyParts.length > 2) and namespaceMap already has a primitive value for
namespaceKey, convert that entry's value into an object and store the original
primitive under a reserved property (e.g., "" or "_value"); conversely, when
adding a simple key and an entry exists whose value is an object, store the
simple value under that reserved property instead of replacing the object; use
the existing symbols namespaceMap, namespaceKey, keyParts, subKey and ensure you
consistently create/merge objects rather than overwriting or ignoring entries.
---
Nitpick comments:
In `@bin/cli.js`:
- Around line 540-567: Wrap the call to liquidTestUtils.extractURL in a
try/catch and validate the returned urlData (check that firmId, companyId,
periodId and reconciliationId are present) and log a clear user-facing error and
exit if parsing fails or required IDs are missing; after calling
SF.updateReconciliationCustom, enhance the failure branch to log detailed
response information (e.g., response.status, response.data or response.error)
before calling process.exit(1) so failures are easier to debug — reference
extractURL, urlData (firmId, companyId, periodId, reconciliationId),
findTestData, transformCustomToProperties, SF.updateReconciliationCustom and
response when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eb889f5-0776-47ba-8d67-f95f23f16839
📒 Files selected for processing (5)
.gitignoreCHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/utils/textPropertyUtils.js
Comment on lines
+27
to
+37
| if (keyParts.length === 2) { | ||
| if (!namespaceMap.has(namespaceKey)) { | ||
| namespaceMap.set(namespaceKey, { namespace, key, value }); | ||
| } | ||
| } else { | ||
| if (!namespaceMap.has(namespaceKey)) { | ||
| namespaceMap.set(namespaceKey, { namespace, key, value: {} }); | ||
| } | ||
| const subKey = keyParts.slice(2).join("."); | ||
| namespaceMap.get(namespaceKey).value[subKey] = value; | ||
| } |
There was a problem hiding this comment.
Potential data inconsistency when mixing simple and nested keys for the same namespace.key.
If the input contains both a simple key (e.g., "ns.key": "value") and nested keys (e.g., "ns.key.subkey": "nested"), the order of iteration determines the result:
- Simple key first → the nested key will overwrite
.valuewith an object containingsubkey - Nested key first → the simple key is ignored (line 28 check
!namespaceMap.has)
This could lead to unexpected behavior depending on object property iteration order.
🛡️ Proposed fix to handle mixed keys consistently
if (keyParts.length === 2) {
- if (!namespaceMap.has(namespaceKey)) {
- namespaceMap.set(namespaceKey, { namespace, key, value });
- }
+ // Simple key always sets the value directly (overwrite if exists)
+ const existing = namespaceMap.get(namespaceKey);
+ if (existing && typeof existing.value === "object") {
+ consola.warn(`Key "${fullKey}" conflicts with nested keys under same namespace.key — skipping`);
+ continue;
+ }
+ namespaceMap.set(namespaceKey, { namespace, key, value });
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/textPropertyUtils.js` around lines 27 - 37, The code currently has
inconsistent behavior when mixing simple keys ("ns.key") and nested keys
("ns.key.sub") because namespaceMap entries are set once and later keys either
get ignored or overwrite the value; update the logic in the block that handles
keyParts and namespaceMap so conflicts are merged deterministically: whenever
adding a nested key (keyParts.length > 2) and namespaceMap already has a
primitive value for namespaceKey, convert that entry's value into an object and
store the original primitive under a reserved property (e.g., "" or "_value");
conversely, when adding a simple key and an entry exists whose value is an
object, store the simple value under that reserved property instead of replacing
the object; use the existing symbols namespaceMap, namespaceKey, keyParts,
subKey and ensure you consistently create/merge objects rather than overwriting
or ignoring entries.
Author
There was a problem hiding this comment.
Shouldn't happen in the YAML files in practice
There was a problem hiding this comment.
@Benjvandam, understood! Since the YAML files in practice won't mix simple keys (e.g., "ns.key": "value") and nested keys (e.g., "ns.key.subkey": "nested") for the same namespace.key, the current behavior is safe. I'll note this for future reviews.
✏️ Learnings added
Learnt from: Benjvandam
Repo: silverfin/silverfin-cli PR: 249
File: lib/utils/textPropertyUtils.js:27-37
Timestamp: 2026-03-23T15:05:12.258Z
Learning: In `lib/utils/textPropertyUtils.js` (silverfin-cli), custom property keys in Liquid Test YAML files never mix simple keys (`namespace.key`) and nested keys (`namespace.key.subkey`) for the same `namespace.key`. Therefore, the existing logic in `transformCustomToProperties` that ignores duplicate `namespaceKey` entries is intentional and safe — no conflict-resolution handling is needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Benjvandam
Repo: silverfin/silverfin-cli PR: 249
File: lib/utils/textPropertyUtils.js:31-36
Timestamp: 2026-03-23T15:03:32.071Z
Learning: In `lib/utils/textPropertyUtils.js` (silverfin-cli), custom property keys in the Liquid Test YAML files follow at most a 3-segment dot-notation format (`namespace.key.subkey`). Keys with 4 or more segments (e.g., `namespace.key.a.b`) do not occur in practice, so collapsing deeper segments with `keyParts.slice(2).join(".")` is intentional and sufficient.
Extract and push custom data from all 4 levels in liquid test YAML: company, period, reconciliation, and account. Company/period/account endpoints send one property per request. Reconciliation and account IDs are resolved via the API by handle/number. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
lib/api/sfApi.js (1)
587-633: Extract the repeated per-property POST helper.
updateCompanyCustom,updatePeriodCustom, andupdateAccountCustomall duplicate the same request/collect/error-handling loop. Pulling that into one helper will keep response handling consistent when one of these endpoints inevitably changes.♻️ Possible refactor
+async function postCustomProperties(instance, url, properties) { + const results = []; + for (const prop of properties) { + try { + const response = await instance.post(url, prop); + apiUtils.responseSuccessHandler(response); + results.push(response); + } catch (error) { + const response = await apiUtils.responseErrorHandler(error); + results.push(response); + } + } + return results; +} + async function updateCompanyCustom(firmId, companyId, properties) { const instance = AxiosFactory.createInstance("firm", firmId); - const results = []; - for (const prop of properties) { - try { - const response = await instance.post(`/companies/${companyId}/custom`, prop); - apiUtils.responseSuccessHandler(response); - results.push(response); - } catch (error) { - const response = await apiUtils.responseErrorHandler(error); - results.push(response); - } - } - return results; + return postCustomProperties(instance, `/companies/${companyId}/custom`, properties); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/api/sfApi.js` around lines 587 - 633, The three functions updateCompanyCustom, updatePeriodCustom, and updateAccountCustom duplicate the same per-property POST loop and error/success handling; extract that loop into a shared helper (e.g., postPropertiesWithCollect or postPerPropertyCustom) that accepts an Axios instance, a URL (or URL builder), and the properties array, iterates over properties, calls instance.post(url, prop), invokes apiUtils.responseSuccessHandler on success and apiUtils.responseErrorHandler on catch, collects each response into an array and returns it; then replace the bodies of updateCompanyCustom, updatePeriodCustom, and updateAccountCustom to call this helper with the appropriate URL for company, period, and account.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/cli.js`:
- Around line 541-543: The code parses the target URL into urlData via
liquidTestUtils.extractURL (producing firmId, companyId and the target
period/reconciliation/account identity), but the update-building logic (the
loops that assemble the updates array and the fiscal_year.end_date matching
logic) currently selects records by fiscal_year.end_date and enqueues every
block found; change that logic to filter using the identity fields extracted
from options.url/urlData (e.g., periodId, reconciliationId, accountId or
equivalent) so only records whose period/reconciliation/account IDs match
urlData are included when constructing updates, and ensure subsequent
update/enqueue code uses firmId/companyId from urlData for scoping rather than
broadly matching fiscal_year.end_date.
- Around line 603-609: The current guard checks the truthiness of the value
returned by SF.findAccountByNumber, but that function can return an error object
(from apiUtils.responseErrorHandler) which is truthy and will cause accessing
account.account.id to throw; update the conditional after calling
SF.findAccountByNumber to explicitly verify the resolved shape and existence of
account.account.id (e.g., ensure account && account.account &&
account.account.id) before calling SF.updateAccountCustom, and if missing log a
descriptive error (using consola.error) and return null so lookup failures and
error objects are handled cleanly.
- Around line 631-643: The loop that applies uploads in bin/cli.js (iterating
over updates and calling update.apply()) currently logs failures but never sets
a non-zero exit status; modify the end of the loop (or after the loop completes)
to set process.exitCode = 1 when any response in responses is non-2xx (i.e.,
when failed.length > 0) so CI sees failure; alternatively throw an Error after
detecting failures — update the logic around the variables update, responses,
and failed to ensure the process exits non-zero on any upload failure.
In `@lib/utils/textPropertyUtils.js`:
- Around line 60-75: The current loop (using handleDirs, testsDir, yamlFiles and
checking parsed[testName]) returns the first YAML that contains
parsed[testName], which causes nondeterministic selection when handle is
omitted; instead, collect all matching filePath/parsed pairs across handleDirs
and only return when there is exactly one unique match, otherwise disambiguate
by preferring matches in the directory that equals the provided handle (variable
handle) or throw a clear error asking the caller to specify a handle; apply the
same change to the similar logic around lines 120-125 so both code paths gather
all matches, enforce uniqueness, and only resolve when a single unambiguous
match remains.
- Around line 31-36: The code currently collapses deep dotted keys into a single
string key by using subKey = keyParts.slice(2).join("."), causing e.g.
ns.key.a.b to become { "a.b": value } instead of a nested object; update the
logic where namespaceMap is populated (look for namespaceMap, namespaceKey,
keyParts, subKey) to build real nested objects for keyParts.slice(2) by
iterating the remaining segments and creating nested plain objects until the
final segment where you assign value (instead of joining with "."); ensure
namespaceMap.get(namespaceKey).value is mutated to contain the proper nested
structure rather than a single dotted-key entry.
---
Nitpick comments:
In `@lib/api/sfApi.js`:
- Around line 587-633: The three functions updateCompanyCustom,
updatePeriodCustom, and updateAccountCustom duplicate the same per-property POST
loop and error/success handling; extract that loop into a shared helper (e.g.,
postPropertiesWithCollect or postPerPropertyCustom) that accepts an Axios
instance, a URL (or URL builder), and the properties array, iterates over
properties, calls instance.post(url, prop), invokes
apiUtils.responseSuccessHandler on success and apiUtils.responseErrorHandler on
catch, collects each response into an array and returns it; then replace the
bodies of updateCompanyCustom, updatePeriodCustom, and updateAccountCustom to
call this helper with the appropriate URL for company, period, and account.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa47adc3-f595-435c-8c10-00b968bb4ef5
📒 Files selected for processing (3)
bin/cli.jslib/api/sfApi.jslib/utils/textPropertyUtils.js
Comment on lines
+541
to
+543
| // Parse URL to extract IDs | ||
| const urlData = liquidTestUtils.extractURL(options.url); | ||
| const { firmId, companyId } = urlData; |
There was a problem hiding this comment.
Scope period/reconciliation/account updates to the URL target.
Lines 542-543 parse a specific reconciliation URL, but Line 567 switches to matching by fiscal_year.end_date and the loops below enqueue every period/reconciliation/account block found in the YAML. That turns a targeted command into a multi-record writer and can overwrite unrelated remote custom data from the same test file. Use the URL’s period/reconciliation identity to filter the update set before building updates.
Also applies to: 561-613
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/cli.js` around lines 541 - 543, The code parses the target URL into
urlData via liquidTestUtils.extractURL (producing firmId, companyId and the
target period/reconciliation/account identity), but the update-building logic
(the loops that assemble the updates array and the fiscal_year.end_date matching
logic) currently selects records by fiscal_year.end_date and enqueues every
block found; change that logic to filter using the identity fields extracted
from options.url/urlData (e.g., periodId, reconciliationId, accountId or
equivalent) so only records whose period/reconciliation/account IDs match
urlData are included when constructing updates, and ensure subsequent
update/enqueue code uses firmId/companyId from urlData for scoping rather than
broadly matching fiscal_year.end_date.
Author
There was a problem hiding this comment.
The purpose of the command is to set up a full test scenario in a company file — that requires pushing custom data to all levels and templates referenced in the test, not just the single reconciliation from the URL. The URL is used to identify the target company/firm/period, but the test YAML defines the complete data set needed for that scenario to work.
Comment on lines
+31
to
+36
| } else { | ||
| if (!namespaceMap.has(namespaceKey)) { | ||
| namespaceMap.set(namespaceKey, { namespace, key, value: {} }); | ||
| } | ||
| const subKey = keyParts.slice(2).join("."); | ||
| namespaceMap.get(namespaceKey).value[subKey] = value; |
There was a problem hiding this comment.
Build real nested objects for deep dotted keys.
Lines 35-36 collapse the remaining path into a single "a.b" key, so ns.key.a.b is serialized differently from ns.key.a. Any property deeper than three segments will be uploaded in the wrong shape.
🐛 Possible fix
} else {
if (!namespaceMap.has(namespaceKey)) {
namespaceMap.set(namespaceKey, { namespace, key, value: {} });
}
- const subKey = keyParts.slice(2).join(".");
- namespaceMap.get(namespaceKey).value[subKey] = value;
+ let current = namespaceMap.get(namespaceKey).value;
+ for (const part of keyParts.slice(2, -1)) {
+ if (typeof current[part] !== "object" || current[part] === null) {
+ current[part] = {};
+ }
+ current = current[part];
+ }
+ current[keyParts[keyParts.length - 1]] = value;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/textPropertyUtils.js` around lines 31 - 36, The code currently
collapses deep dotted keys into a single string key by using subKey =
keyParts.slice(2).join("."), causing e.g. ns.key.a.b to become { "a.b": value }
instead of a nested object; update the logic where namespaceMap is populated
(look for namespaceMap, namespaceKey, keyParts, subKey) to build real nested
objects for keyParts.slice(2) by iterating the remaining segments and creating
nested plain objects until the final segment where you assign value (instead of
joining with "."); ensure namespaceMap.get(namespaceKey).value is mutated to
contain the proper nested structure rather than a single dotted-key entry.
Author
There was a problem hiding this comment.
4 nesting levels doesn't happen in practice
There was a problem hiding this comment.
@Benjvandam, understood! Thanks for the context. I'll note that for future reviews.
✏️ Learnings added
Learnt from: Benjvandam
Repo: silverfin/silverfin-cli PR: 249
File: lib/utils/textPropertyUtils.js:31-36
Timestamp: 2026-03-23T15:03:32.071Z
Learning: In `lib/utils/textPropertyUtils.js` (silverfin-cli), custom property keys in the Liquid Test YAML files follow at most a 3-segment dot-notation format (`namespace.key.subkey`). Keys with 4 or more segments (e.g., `namespace.key.a.b`) do not occur in practice, so collapsing deeper segments with `keyParts.slice(2).join(".")` is intentional and sufficient.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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
2 participants
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.
Fixes # #242
Description
New
update-text-propertiescommand that uploads custom text properties from a Liquid Test YAML file to a reconciliation in a company file. It extracts custom data from all 4 levels: company, period, reconciliation, and account.Supports
--handlefor faster YAML file lookup and--dry-runto preview the payload without uploading.Testing Instructions
Steps:
be_market) that has reconciliation templates with liquid test YAML filesdata.company.custom) and verify those are also pushed--handleto verify it scans all template folders and finds the test automaticallyAuthor Checklist
Reviewer Checklist