feat: remove wsl specific npm dependencies #237
Conversation
WalkthroughReplaces local HTML export/write logic with URL-driven handling via a new UrlHandler (with custom filename, inferred extensions and unique temp paths), adds a WSLHandler for WSL-specific open behavior, updates test runner to delegate opening, bumps package to 1.51.0 and removes two dependencies; tightens one validation warning. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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
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-remove-wsl-dependencies
branch
from
34273c1 to
66b65b1
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/liquidTestRunner.js (1)
2-2: Remove unusedaxiosimport.The pipeline failure indicates
axiosis no longer used in this file after the refactoring.🧹 Proposed fix
const yaml = require("yaml"); -const axios = require("axios"); const path = require("path");lib/utils/urlHandler.js (1)
94-98: Extension regex may fail for unquoted filenames.The regex requires a trailing quote
['"]\, butContent-Dispositionheaders can have unquoted filenames (e.g.,filename=file.ext). This is inconsistent with#identifyFilenamewhich handles both cases.🐛 Proposed fix
#identifyFileExtension(string) { if (!string) return null; - const match = string.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['"]/i); + const match = string.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['";\s]?/i); return match ? match[1] : null; }
🤖 Fix all issues with AI agents
In @lib/utils/wslHandler.js:
- Around line 19-23: The open method on the WslHandler class currently
interpolates filePath into a shell string and is vulnerable to command
injection; change it to call the child_process execFile (or spawn) API instead
of exec so the filePath is passed as a safe argument; update the import to
require or destructure execFile from child_process, replace exec(`wsl-open
${filePath}`) in the static open method with an execFile("wsl-open", [filePath],
...) call (preserving error handling), and keep the existing
this.#setupWslOpen() invocation.
- Line 22: The exec invocation that runs `wsl-open ${filePath}` currently has no
callback so failures are swallowed; update the exec call in wslHandler.js to
include an error callback that handles and surfaces errors (e.g., log or
return/throw a descriptive error) when `err` is non-null, reference the existing
`filePath` variable and the `exec` call to locate the spot, and ensure any
spawned-process stdout/stderr are considered if relevant for debugging.
🧹 Nitpick comments (2)
lib/utils/urlHandler.js (2)
58-59: Moverequire("os")to the top of the file with other imports.Inline requires are less idiomatic and can impact readability/maintainability.
♻️ Proposed fix
At the top of the file:
const open = require("open"); const path = require("path"); const axios = require("axios"); const { consola } = require("consola"); const errorUtils = require("./errorUtils"); const fs = require("fs"); +const os = require("os"); const { WSLHandler } = require("./wslHandler");Then in
#downloadFile:- const tempDir = path.resolve(require("os").tmpdir(), "silverfin"); + const tempDir = path.resolve(os.tmpdir(), "silverfin");
70-84: Consider adding an upper bound to the uniqueness counter.While practically unlikely, the loop could run excessively if many files with the same base name exist. A reasonable limit (e.g., 1000) with a fallback to UUID would be defensive.
💡 Suggested improvement
#getUniqueFilePath(directory, filename) { const ext = path.extname(filename); const nameWithoutExt = path.basename(filename, ext); let filePath = path.resolve(directory, filename); let counter = 1; + const maxAttempts = 1000; // If file exists, append (1), (2), etc. until we find a unique name - while (fs.existsSync(filePath)) { + while (fs.existsSync(filePath) && counter <= maxAttempts) { const uniqueFilename = `${nameWithoutExt} (${counter})${ext}`; filePath = path.resolve(directory, uniqueFilename); counter++; } + + if (counter > maxAttempts) { + // Fallback to timestamp-based name + filePath = path.resolve(directory, `${nameWithoutExt}_${Date.now()}${ext}`); + } return filePath; }
📜 Review details
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/liquidTestRunner.jslib/utils/urlHandler.jslib/utils/wslHandler.jspackage.json
🧰 Additional context used
🪛 GitHub Actions: Testing and Linting
lib/liquidTestRunner.js
[error] 2-2: 'axios' is assigned a value but never used. Allowed unused vars must match /^_/u no-unused-vars
🔇 Additional comments (4)
package.json (1)
3-3: LGTM!Version bump and dependency cleanup align with the PR objectives. Removing
command-existsandis-wslin favor of built-in WSL detection is a good approach to reduce external dependencies.CHANGELOG.md (1)
5-8: LGTM!The changelog entry accurately describes the changes and appropriately notes no expected user impact.
lib/liquidTestRunner.js (1)
382-388: LGTM!Clean refactoring that delegates HTML file opening to
UrlHandler, centralizing the file handling logic as intended.lib/utils/urlHandler.js (1)
33-39: LGTM!Clean integration with
WSLHandlerfor cross-platform file opening. The WSL detection and delegation pattern is straightforward.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/utils/urlHandler.js:
- Around line 33-39: The WSL path is not awaited causing async inconsistency:
update the private method #openLocalFile to await WSLHandler.open(filePath) just
like the non-WSL branch awaits open(filePath), and modify WSLHandler.open to be
async and return the Promise from the underlying exec(...) call (so callers can
await it); keep WSLHandler.isWSL and the non-WSL open(...) usage unchanged.
🧹 Nitpick comments (3)
lib/liquidTestRunner.js (1)
400-411: Minor: Consider awaiting async operations in the loop.The
forEachwith an async callback on line 407 doesn't await the promises returned bygetHTMLrenders. While this works as fire-and-forget for independent file opens, usingPromise.allwithmapwould ensure proper error propagation and completion tracking.This appears to be pre-existing behavior, so it can be addressed separately if desired.
♻️ Optional refactor
async function handleHTMLfiles(testName = "", testRun, renderMode) { if (testName) { // Only one test - getHTMLrenders(renderMode, testName, testRun, true); + await getHTMLrenders(renderMode, testName, testRun, true); } else { // All tests const testNames = Object.keys(testRun.tests); - testNames.forEach(async (testName) => { - getHTMLrenders(renderMode, testName, testRun, true); - }); + await Promise.all( + testNames.map((testName) => getHTMLrenders(renderMode, testName, testRun, true)) + ); } }lib/utils/urlHandler.js (2)
86-92: Consider handling malformed URI encoding.
decodeURIComponentcan throw aURIErrorif the filename contains invalid percent-encoded sequences. Consider wrapping in try-catch to avoid unhandled exceptions.♻️ Defensive handling
#identifyFilename(string) { if (!string) return null; // Match full filename from content-disposition header // Handles: filename="file.ext", filename*=UTF-8''file.ext, filename=file.ext const match = string.match(/filename[*]?=['"]?(?:UTF-8'')?([^'";\s]+)['";\s]?/i); - return match ? decodeURIComponent(match[1]) : null; + if (!match) return null; + try { + return decodeURIComponent(match[1]); + } catch { + return match[1]; // Return raw value if decoding fails + } }
94-98: Regex inconsistency with#identifyFilename.The
#identifyFileExtensionregex requires quotes around the filename (['"]/iat the end), but#identifyFilenamehandles unquoted filenames likefilename=file.ext. This could result in successfully extracting a filename but failing to extract its extension.The fallback to
"html"on line 47 mitigates this, but consider aligning the regex patterns for consistency:♻️ Suggested fix
#identifyFileExtension(string) { if (!string) return null; - const match = string.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['"]/i); + const match = string.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['";\s]?/i); return match ? match[1] : null; }
📜 Review details
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/liquidTestRunner.jslib/utils/urlHandler.jslib/utils/wslHandler.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/wslHandler.js
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
lib/utils/urlHandler.js (1)
lib/utils/wslHandler.js (2)
require(1-1)require(3-3)
🔇 Additional comments (6)
package.json (1)
3-3: LGTM!Version bump to 1.51.0 is appropriate for this feature release that removes external WSL-related dependencies in favor of built-in detection.
lib/liquidTestRunner.js (2)
10-10: LGTM!Clean import of the new UrlHandler utility that centralizes URL-based HTML handling with WSL support.
380-386: LGTM!The refactored
getHTMLfunction properly delegates HTML handling toUrlHandler, with meaningful filenames that combine test name and HTML mode for easy identification.lib/utils/urlHandler.js (3)
7-18: LGTM!Clean constructor with proper URL validation and optional
customFilenameparameter for flexible file naming.
70-84: LGTM!Clean implementation for generating unique file paths with familiar
(1),(2)suffix pattern, consistent with OS conventions for duplicate files.
41-68: Moverequire("os")to top-level imports for consistency.The inline
require("os")on line 58 is inconsistent with the file's import style. Addconst os = require("os");to the top-level imports and useos.tmpdir()instead.Likely an incorrect or invalid review comment.
AgustinSilverfin
force-pushed
the
agustin-remove-wsl-dependencies
branch
2 times, most recently
from
ea219ba to
e6cf8af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/urlHandler.js (1)
86-98: Fix Content-Disposition header parsing inconsistencies.The regex patterns have several issues:
Line 96 (#identifyFileExtension) requires a closing quote
['"]/ibut valid Content-Disposition headers can be unquoted (e.g.,attachment; filename=document.pdf). This will fail to extract extensions from unquoted filenames.Line 90-91 (#identifyFilename) calls
decodeURIComponenton all matches, but should only decode RFC 5987 encoded values (those withfilename*=UTF-8''). Applying it to non-encoded filenames could corrupt them if they contain percent signs.Inconsistency: #identifyFilename handles both quoted and unquoted formats (closing quote optional), while #identifyFileExtension requires quotes. Both should handle the same formats consistently.
Additionally, no tests exist for these methods. Add test coverage for standard Content-Disposition formats:
attachment; filename=file.pdf(unquoted)attachment; filename="file.pdf"(quoted)attachment; filename*=UTF-8''file%20name.pdf(RFC 5987)
🧹 Nitpick comments (1)
lib/utils/urlHandler.js (1)
58-59: Consider implementing temp file cleanup.The dedicated temp directory is good for isolation, but downloaded files accumulate indefinitely. While OS temp cleaners may eventually remove these, consider implementing periodic cleanup or removal after successful opens to prevent disk space issues over time.
💡 Optional: Add cleanup after file open
One approach is to clean up the file after successfully opening it:
async openFile() { try { const filePath = await this.#downloadFile(); await this.#openLocalFile(filePath); + // Optional: Clean up temp file after opening + // Note: May need delay to ensure file is opened before deletion + setTimeout(() => { + try { + fs.unlinkSync(filePath); + } catch (err) { + // Ignore cleanup errors + } + }, 5000); } catch (error) { consola.error(`Failed to open URL: ${this.url}`, error); } }Alternatively, implement a cleanup utility that removes old files from the temp directory periodically.
📜 Review details
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 (6)
CHANGELOG.mdlib/exportFileInstanceGenerator.jslib/liquidTestRunner.jslib/utils/urlHandler.jslib/utils/wslHandler.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- lib/utils/wslHandler.js
🧰 Additional context used
🧬 Code graph analysis (1)
CHANGELOG.md (1)
tests/lib/cli/changelogReader.spec.js (1)
mockChangelog(110-126)
🔇 Additional comments (7)
CHANGELOG.md (1)
5-8: LGTM! Clear and informative changelog entry.The version entry accurately describes the changes and correctly sets user expectations regarding the lack of visible impact.
lib/liquidTestRunner.js (1)
380-386: Excellent simplification of HTML handling logic.The rewrite successfully delegates file management to
UrlHandler, eliminating local disk writes and OS-specific logic. The centralized approach improves maintainability and reduces code duplication.lib/utils/urlHandler.js (4)
13-19: Good enhancement to support custom filenames.The optional
customFilenameparameter provides flexibility while maintaining backward compatibility with the defaultnullvalue triggering automatic filename detection.
46-56: Well-designed filename resolution logic.The cascading fallback strategy (custom → inferred → timestamp) ensures a valid filename is always generated, while the "html" default extension aligns with the primary use case.
70-84: Solid implementation of unique file path generation.The counter-based approach correctly handles filename collisions. While there's a theoretical race condition between the existence check and file write, it's negligible in practice for this use case involving temporary files with unique base names.
7-7: WSLHandler integration is correctly implemented.The imported
WSLHandlerfromlib/utils/wslHandler.jsproperly implements:
static isWSL(): Detects WSL environment by checking/proc/versionfor "microsoft" or "wsl"static async open(filePath): Opens files asynchronously using thewsl-opencommand with user-prompted installation fallbackThe implementation in
urlHandler.js(lines 7, 33-39) correctly uses these methods for WSL-specific file opening behavior.lib/exportFileInstanceGenerator.js (1)
89-91: The length check improves the logging condition, but the array type assumption cannot be verified from the codebase.The addition of
response.validation_errors.length > 0appropriately prevents logging empty arrays. However, no type definitions or API documentation in the repository confirm thatvalidation_errorsis always an array type when present. The single codebase usage offers insufficient evidence to guarantee this assumption is safe across all API responses.
|
@michieldegezelle btw, I have tested the changes ( |
AgustinSilverfin
force-pushed
the
agustin-remove-wsl-dependencies
branch
from
e6cf8af to
1e53e93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/liquidTestRunner.js (1)
400-411: Async callbacks in forEach are not awaited.The
forEachwith an async callback (line 407) does not await the promises, andgetHTMLrenderson line 403 is also not awaited. While this likely works in practice (files will still open), any errors won't propagate properly.♻️ Suggested fix using for...of loop
async function handleHTMLfiles(testName = "", testRun, renderMode) { if (testName) { // Only one test - getHTMLrenders(renderMode, testName, testRun, true); + await getHTMLrenders(renderMode, testName, testRun, true); } else { // All tests const testNames = Object.keys(testRun.tests); - testNames.forEach(async (testName) => { - getHTMLrenders(renderMode, testName, testRun, true); - }); + for (const testName of testNames) { + await getHTMLrenders(renderMode, testName, testRun, true); + } } }lib/utils/urlHandler.js (2)
41-68: Error handling silently swallows the exception without returning or re-throwing.When
errorUtils.errorHandler(error)is called on line 66, the function returnsundefinedimplicitly. The caller (openFile) will then attempt to call#openLocalFile(undefined), potentially causing a secondary error.♻️ Suggested fix: re-throw after handling
} catch (error) { errorUtils.errorHandler(error); + throw error; }
94-98: Regex inconsistency with #identifyFilename.The regex requires a closing quote (
['"]/i) whereas#identifyFilenamemakes it optional (['";\s]?). This means unquoted filenames likefilename=test.htmlmay returnnullfor the extension but successfully parse the filename.♻️ Suggested fix for consistency
#identifyFileExtension(string) { if (!string) return null; - const match = string.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['"]/i); + const match = string.match(/filename[*]?=['"]?(?:[^'"]*\.)?([^.'";\s]+)['";\s]?/i); return match ? match[1] : null; }
📜 Review details
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 (6)
CHANGELOG.mdlib/exportFileInstanceGenerator.jslib/liquidTestRunner.jslib/utils/urlHandler.jslib/utils/wslHandler.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/exportFileInstanceGenerator.js
- lib/utils/wslHandler.js
🔇 Additional comments (9)
package.json (1)
3-3: LGTM!Version bump to 1.51.0 aligns with the changelog entry and reflects the removal of WSL-specific npm dependencies.
CHANGELOG.md (1)
5-9: LGTM!The changelog entry clearly communicates the scope of changes and sets appropriate expectations for users regarding the absence of visible impact.
lib/liquidTestRunner.js (2)
10-10: LGTM!Import for UrlHandler correctly added to support the new URL-based HTML handling.
380-386: LGTM!Clean refactor that centralizes file handling through UrlHandler. The filename format
${testName}_${htmlMode}provides clear identification of the generated files.lib/utils/urlHandler.js (5)
7-7: LGTM!WSLHandler import correctly added to support cross-platform file opening.
13-18: LGTM!Constructor properly validates the required
urlparameter and accepts an optionalcustomFilenamefor flexible file naming.
33-39: LGTM!Clean abstraction for cross-platform file opening, properly delegating to WSLHandler when running in WSL environment.
70-84: LGTM!Standard pattern for generating unique filenames. The TOCTOU window between
existsSyncandwriteFileSyncis acceptable for temp file generation in this context.
86-92: LGTM!The regex handles common Content-Disposition formats including UTF-8 encoded filenames. The
decodeURIComponentcall properly handles URL-encoded characters.
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
No issues when running tests or generating export files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/lib/utils/urlHandler.test.js (6)
20-62: Constructor coverage is solid; consider looseningcustomFilenamedefault expectations.The
customFilenamedefault expectation is hardcoded tonull(Line 27). If the implementation choosesundefined(common in JS), this becomes a noisy failure without behavior impact.Possible tweak
- expect(handler.customFilename).toBeNull(); + expect(handler.customFilename).toBeNull(); // keep if this is a deliberate public contract + // or, if not a contract: + // expect(handler.customFilename ?? null).toBeNull();
64-97:process.exit+ throwing inerrorHandlercan create misleading failure modes across tests.Because
errorUtils.errorHandleris mocked toprocess.exit(1)and thenthrow new Error("Process exited"),openFile()may hit its own catch and logconsola.errorwith"Process exited"(depending on implementation), which can cause unrelated tests to fail/flake when additional assertions are added later.More targeted errorHandler mock
- errorUtils.errorHandler.mockImplementation(() => { - process.exit(1); - throw new Error("Process exited"); - }); + errorUtils.errorHandler.mockImplementation((err) => { + // emulate current behavior without introducing a new synthetic error type + process.exit(1); + throw err; + });
98-157: Strengthen “open was called” assertions by linking to the actual written file path.Several tests assert
openis called withexpect.stringContaining("document.pdf")etc. A more robust check is: “open was called with the same path we wrote to”, which avoids coupling to naming rules.Proposed diff (assert open uses the written path)
- expect(open).toHaveBeenCalledWith(expect.stringContaining("document.pdf")); + const writtenPath = fs.writeFileSync.mock.calls[0][0]; + expect(open).toHaveBeenCalledWith(writtenPath);
249-297: Unique-name tests are good, but they’re implementation-coupled to the “ (n)” naming convention.If the naming convention changes (e.g.,
-1,_1, etc.), these tests will fail even if behavior remains correct (“don’t overwrite existing”). If that convention is intentional/public, keep as-is; otherwise consider asserting “not equal to original name and ends with extension”.
409-425: Long-filename test may be brittle if UrlHandler sanitizes/truncates filenames.The current assertion requires the full 200-char base name to be present in the output path (Line 423). If the implementation later adds filesystem-safe truncation, this becomes a false failure.
Example more future-proof assertion
- expect(fs.writeFileSync).toHaveBeenCalledWith(expect.stringContaining(longFilename), expect.any(Buffer)); + const writtenPath = fs.writeFileSync.mock.calls[0][0]; + expect(writtenPath).toMatch(/\.pdf$/); + expect(path.basename(writtenPath)).toMatch(/^a+/);
1-18: Make module mocks explicit to avoid export-shape brittleness.Right now you rely on Jest's automock inference for
consola,../../../lib/utils/wslHandler, and../../../lib/utils/errorUtils. The automock approach works for immediate needs but creates brittleness: if a test later accesses a method that doesn't match the real exports, it fails silently. SinceWSLHandleris a class with static methods anderrorUtilsexports specific functions, explicit mocks make the contract clearer and catch mismatches earlier.Proposed diff (explicit mock factories)
-jest.mock("consola"); -jest.mock("../../../lib/utils/wslHandler"); -jest.mock("../../../lib/utils/errorUtils"); +jest.mock("consola", () => ({ + consola: { + error: jest.fn(), + warn: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + success: jest.fn(), + }, +})); + +jest.mock("../../../lib/utils/wslHandler", () => ({ + WSLHandler: { + isWSL: jest.fn(), + open: jest.fn(), + }, +})); + +jest.mock("../../../lib/utils/errorUtils", () => ({ + errorHandler: jest.fn(), +}));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/lib/utils/urlHandler.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
tests/lib/utils/urlHandler.test.js (1)
lib/utils/urlHandler.js (1)
UrlHandler(11-51)
🔇 Additional comments (1)
tests/lib/utils/urlHandler.test.js (1)
159-247: Nice Content-Disposition parsing coverage (quoted/unquoted, filename, extra params, multiple extensions).*This suite exercises the common and tricky header formats well (including
filename*decoding). Good signal for regression prevention.
Description
Testing Instructions
Nothing should change. You can test
silverfin run-testwith--html-inputand--html-previewflags and files should be opened in the browser as usual. Alsosilverfin generate-export-fileshould open the generated file as well.Author Checklist
Reviewer Checklist