Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes introduce support for organizing text parts into subdirectories within template configurations. Deep cloning of template configs tracks changes and persists them to disk, a new recursive file scanning utility discovers nested liquid files with duplicate detection, and test coverage expands to validate nested path parsing and config-disk consistency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
tests/lib/templates/reconciliationTexts.test.js (1)
635-654: Test relies on exception handling for incomplete verification.The test correctly verifies that the config entry remains unchanged when a file is missing from disk. However, the test catches and ignores the exception from
ReconciliationText.read(handle)without verifying it was thrown for the expected reason.Consider adding an assertion to verify the exception was thrown:
💡 Suggested improvement
// read() will fail at readPartsLiquid, but the config should not have been altered + let errorThrown = false; try { ReconciliationText.read(handle); } catch (e) { // Expected to fail when reading the missing file + errorThrown = true; + expect(e.code).toBe('ENOENT'); } + expect(errorThrown).toBe(true); const configAfter = JSON.parse(fs.readFileSync(configPath, "utf-8")); expect(configAfter.text_parts.part_1).toBe("text_parts/part_1.liquid");lib/templates/accountTemplate.js (1)
211-236: Consider extracting duplicated methods to a shared utility.The
#updateConfigIfChangedand#syncTextPartsFromDiskmethods are nearly identical to those inReconciliationText. This duplication increases maintenance burden.Since both classes already use
fsUtils, consider moving these helper functions there:// In fsUtils.js function syncTextPartsInConfig(templateType, handle, templateConfig) { ... } function updateConfigIfChanged(templateType, handle, originalConfig, currentConfig) { ... }This would allow both
ReconciliationTextandAccountTemplateto share the same implementation.
Benjvandam
force-pushed
the
allow-folder-structure-in-config-text-parts
branch
2 times, most recently
from
4f4d617 to
5726497
Compare
michieldegezelle
force-pushed
the
allow-folder-structure-in-config-text-parts
branch
from
5726497 to
ac2edd3
Compare
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
Changes look good, but I discussed this with Agustin and we would slightly change the scope of this ticket (due to the stability of the auto-update).
Could you make sure that all function calls correctly look at subfolders (which is already the case). The auto update of the config file should be skipped for now
- It is not always so stable (e.g. when deleting a file)
- It only works when a command is done (it is static), so ideally this is something for a future update in the extension.
This means that we should rely on the user to update the config file manually for now (but this is acceptable).
In parallel, we should then prepare an update in the extension to:
- Make sure that subfolders are allowed
- removeDeletedParts() (templateUtils.ts) still works for subfolders
- “Open file” link (panelTemplateParts.ts) will correctly go to the file, even if it is in the subfolder
In a later stage, we can then look at the extension to automatically update the config as soon as the user saves, but let's keep that for later.
Benjvandam
force-pushed
the
allow-folder-structure-in-config-text-parts
branch
from
ac2edd3 to
a040e26
Compare
Benjvandam
force-pushed
the
allow-folder-structure-in-config-text-parts
branch
from
a040e26 to
2fe5852
Compare
|
I’ve removed the auto-update behaviour for the config files and kept the utility that maps the actual file structure. Subfolders are still supported: they’re not a problem for the CLI, but the API only receives template files and no folder structure, so the Silverfin platform doesn’t preserve it. When you re-import a template that was already imported before, you get the flat file structure back from the API, so in that case you’ll need to re-create the folder structure (and update the config) manually. |
Benjvandam
force-pushed
the
allow-folder-structure-in-config-text-parts
branch
from
2fe5852 to
c89e5d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/utils/fsUtils.js (1)
553-555: Add explicit validation for unsupportedtemplateType.Fail fast when
templateTypeis invalid instead of building a path fromundefined; this improves debuggability and prevents silent misreads.💡 Proposed refactor
function scanTextParts(templateType, handle) { + if (!FOLDERS[templateType]) { + throw new Error(`Template type should be one of ${TEMPLATE_TYPES.join(", ")}`); + } const textPartsDir = path.join(process.cwd(), FOLDERS[templateType], handle, "text_parts");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/fsUtils.js` around lines 553 - 555, The scanTextParts function builds a path using FOLDERS[templateType] without validating templateType; add an explicit validation at the start of scanTextParts to check that templateType is a supported key in FOLDERS (e.g., if (!FOLDERS.hasOwnProperty(templateType)) throw new Error(...)), and fail fast with a clear error mentioning the invalid templateType and handle so callers get a helpful message instead of constructing a path from undefined.
🤖 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/fsUtils.js`:
- Around line 574-576: The stored configPath uses OS-native separators from
path.relative which yields backslashes on Windows; update the assignment (in the
scanTextParts flow where relativePath and configPath are set) to normalize to
POSIX separators before persisting (e.g., convert backslashes to forward slashes
or use path.posix utilities) so configPath always uses '/' and remains
consistent with accountTemplate.js and reconciliationText.js expectations.
---
Nitpick comments:
In `@lib/utils/fsUtils.js`:
- Around line 553-555: The scanTextParts function builds a path using
FOLDERS[templateType] without validating templateType; add an explicit
validation at the start of scanTextParts to check that templateType is a
supported key in FOLDERS (e.g., if (!FOLDERS.hasOwnProperty(templateType)) throw
new Error(...)), and fail fast with a clear error mentioning the invalid
templateType and handle so callers get a helpful message instead of constructing
a path from undefined.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdlib/templates/accountTemplate.jslib/templates/reconciliationText.jslib/utils/fsUtils.jstests/lib/templates/reconciliationTexts.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- tests/lib/templates/reconciliationTexts.test.js
- lib/templates/accountTemplate.js
| const relativePath = path.relative(relativeTo, fullPath); | ||
| const configPath = relativePath; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js path.relative() platform-specific separators Windows POSIX
💡 Result:
path.relative(from, to)is platform-dependent because the defaultnode:pathexport uses the OS-specific implementation (Win32 on Windows, POSIX on Linux/macOS). (nodejs.org)- On Windows, Node accepts both
/and\as input separators, butpathmethods (includingpath.relative) will return paths using backslashes (\). (nodejs.org) - On POSIX, returned paths use forward slashes (
/). This corresponds topath.sepbeing\on Windows and/on POSIX. (nodejs.org) - If you need deterministic separators regardless of OS, use:
path.posix.relative(from, to)to always get/path.win32.relative(from, to)to always get\(nodejs.org)
Citations:
- 1: https://nodejs.org/api/path.html?utm_source=openai
- 2: https://nodejs.org/api/path.html?utm_source=openai
- 3: https://nodejs.org/api/path.html?utm_source=openai
- 4: https://nodejs.org/api/path.html?utm_source=openai
🏁 Script executed:
# Find and examine the scanTextParts function
cd lib/utils && cat -n fsUtils.js | sed -n '570,580p'Repository: silverfin/silverfin-cli
Length of output: 571
🏁 Script executed:
# Check how config paths are used elsewhere in the codebase
rg "relativePath|configPath" --type js -B 2 -A 2Repository: silverfin/silverfin-cli
Length of output: 32116
Normalize scanTextParts config paths to POSIX separators.
Line 574–575 currently persist OS-native separators. On Windows, path.relative() returns backslashes, storing paths like text_parts\... in config, which breaks cross-platform consistency since the codebase expects forward slashes (e.g., in accountTemplate.js, reconciliationText.js).
💡 Proposed fix
- const relativePath = path.relative(relativeTo, fullPath);
- const configPath = relativePath;
+ const relativePath = path.relative(relativeTo, fullPath);
+ const configPath = relativePath.split(path.sep).join("/");📝 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 relativePath = path.relative(relativeTo, fullPath); | |
| const configPath = relativePath; | |
| const relativePath = path.relative(relativeTo, fullPath); | |
| const configPath = relativePath.split(path.sep).join("/"); | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/fsUtils.js` around lines 574 - 576, The stored configPath uses
OS-native separators from path.relative which yields backslashes on Windows;
update the assignment (in the scanTextParts flow where relativePath and
configPath are set) to normalize to POSIX separators before persisting (e.g.,
convert backslashes to forward slashes or use path.posix utilities) so
configPath always uses '/' and remains consistent with accountTemplate.js and
reconciliationText.js expectations.
c89e5d0 to
6c40161
Compare
Benjvandam
force-pushed
the
allow-folder-structure-in-config-text-parts
branch
from
6c40161 to
cde7397
Compare
3 participants
Fixes # #240
Description
This feature allows you to create a folder structure within the text_parts of a reconciliation and account template. First it builds a map of the actual folder structure. Then based on the map, the text parts in the config are updated to to reflect the correct path. If a file was not mentioned in the config file, it will not be suddenly be included.
Testing Instructions
Steps:
Keep into account that you may have to disable the extension for testing. Because it seems that when you move files around in subfolders, the Extension automatically removes them from the config when you save. In order for it to work properly, we may need to update the Extension as well still.
Author Checklist
Reviewer Checklist