✨ Add file path support to SDK and client #52
Conversation
PR Review: Add File Path Support to SDK and ClientThanks for this PR @Robdel12! This is a valuable UX improvement that eliminates unnecessary file I/O for developers. I've reviewed the changes and have some feedback below. ✅ Strengths
🐛 Bugs & Issues1. Missing fs imports in client SDK (Critical)Location: The client code uses // Missing at top of file:
import { existsSync, readFileSync } from 'fs';Impact: Runtime error when using file paths with the client SDK. Fix: Add the import statement. 2. Path resolution inconsistencyLocation: The code accepts relative paths but doesn't resolve them to absolute paths before checking existence. This could lead to inconsistent behavior depending on the current working directory. // Current:
if (!existsSync(filePath)) {
throw new Error(`Screenshot file not found: ${filePath}`);
}
// Suggested:
import { resolve } from 'path';
const resolvedPath = resolve(filePath);
if (!existsSync(resolvedPath)) {
throw new Error(`Screenshot file not found: ${filePath} (resolved: ${resolvedPath})`);
}
buffer = readFileSync(resolvedPath);Impact: Error messages may be confusing, and file lookups might fail in edge cases. 🔒 Security Considerations3. Path traversal validation (Medium)While this isn't a web-facing API, consider adding basic path validation to prevent accidental issues: import { resolve, normalize } from 'path';
// Validate path doesn't contain suspicious patterns
const normalizedPath = normalize(filePath);
if (normalizedPath.includes('\0') || normalizedPath.includes('..')) {
throw new VizzlyError(
'Invalid file path: path traversal detected',
'INVALID_PATH',
{ filePath }
);
}Rationale: Defense in depth, especially if user input could flow into the file path parameter in some scenarios. 4. File type validationThe docs mention "Works with any PNG image file" but there's no validation that the file is actually a PNG: // Optional: Add PNG magic number validation
const buffer = readFileSync(resolvedPath);
const isPNG = buffer[0] === 0x89 && buffer[1] === 0x50 &&
buffer[2] === 0x4E && buffer[3] === 0x47;
if (!isPNG) {
throw new VizzlyError(
'File is not a valid PNG image',
'INVALID_IMAGE_FORMAT',
{ filePath }
);
}Impact: Could catch user errors early and provide clearer error messages. ⚡ Performance Considerations5. Synchronous file I/OLocation: Both Using
Current impact: Low (test environments typically aren't I/O constrained) 🧪 Test CoverageGood:
Suggestions:
it('should handle non-PNG files gracefully', async () => {
const textFilePath = join(testDir, 'not-an-image.txt');
writeFileSync(textFilePath, 'This is not a PNG');
// Currently this would pass - should it fail or just upload invalid data?
await vizzlyScreenshot('invalid-file', textFilePath);
});
it('should handle symbolic links to images', async () => {
const symlinkPath = join(testDir, 'symlink.png');
symlinkSync(testImagePath, symlinkPath);
await expect(vizzlyScreenshot('symlink-test', symlinkPath)).resolves.not.toThrow();
});
it('should handle permission errors gracefully', async () => {
const restrictedPath = join(testDir, 'restricted.png');
writeFileSync(restrictedPath, Buffer.from('data'));
chmodSync(restrictedPath, 0o000); // Remove all permissions
await expect(
vizzlyScreenshot('permission-test', restrictedPath)
).rejects.toThrow(/Failed to read screenshot file/);
});📝 Code Quality & Best Practices9. JSDoc completenessThe JSDoc for /**
* ...
* @throws {Error} When file path is provided but file doesn't exist
* @throws {Error} When file cannot be read due to permissions or I/O errors
* @throws {VizzlyError} When SDK operations fail
*/10. DRY: Duplicated file reading logicThe file reading logic is duplicated across:
Suggestion: Extract to a shared utility: // src/utils/file-helpers.js
import { existsSync, readFileSync } from 'fs';
import { resolve } from 'path';
import { VizzlyError } from '../errors/vizzly-error.js';
/**
* Read image buffer from file path or return buffer as-is
* @param {Buffer|string} imageBufferOrPath
* @param {string} contextName - For error messages (e.g., 'screenshot', 'compare')
* @returns {Buffer}
*/
export function resolveImageBuffer(imageBufferOrPath, contextName) {
if (Buffer.isBuffer(imageBufferOrPath)) {
return imageBufferOrPath;
}
if (typeof imageBufferOrPath !== 'string') {
throw new VizzlyError(
`Invalid image input: expected Buffer or file path string`,
'INVALID_INPUT',
{ contextName, type: typeof imageBufferOrPath }
);
}
const filePath = resolve(imageBufferOrPath);
if (!existsSync(filePath)) {
throw new VizzlyError(
`Screenshot file not found: ${imageBufferOrPath}`,
'FILE_NOT_FOUND',
{ contextName, filePath }
);
}
try {
return readFileSync(filePath);
} catch (error) {
throw new VizzlyError(
`Failed to read screenshot file: ${imageBufferOrPath} - ${error.message}`,
'FILE_READ_ERROR',
{ contextName, filePath, originalError: error.message }
);
}
}Then use it: // In SDK and client
const buffer = resolveImageBuffer(imageBuffer, 'screenshot');Benefits:
🎯 Minor Improvements
Suggestion: Use
afterEach(() => {
try {
rmSync(testDir, { recursive: true, force: true });
} catch (error) {
// Ignore cleanup errors - but maybe log them in debug mode?
}
});Consider: Log errors in CI or add a global test teardown to catch persistent file locks. 📋 SummaryMust Fix Before Merge:
Strongly Recommended:
Nice to Have:
🎉 Overall AssessmentThis is a solid PR with a clear use case and good implementation. The main blocker is the missing import statement. Once that's fixed and ideally the shared utility is extracted, this will be ready to merge. The feature provides real value by reducing boilerplate in test code, and the implementation is clean and well-tested. Recommendation: Request changes for #1 (critical), suggest #10 and #2 as high-priority improvements before merge. |
Allow SDKs to accept file paths in addition to buffers for improved DX. This is particularly useful when screenshots are already saved to disk by testing frameworks, eliminating the need to read files into buffers manually. - Modified vizzlyScreenshot() and SDK screenshot()/compare() to accept Buffer | string - Added file path detection and reading with proper error handling - Maintained full backward compatibility with existing Buffer usage - Added comprehensive test coverage (7 SDK tests, 5 client tests) - Updated documentation with examples for both approaches Resolves VIZ-58
Robdel12
force-pushed
the
robert/viz-58
branch
from
48fd845 to
3f73e35
Compare
- Extract file reading logic into shared `resolveImageBuffer()` utility - Add path resolution to convert relative paths to absolute paths - Improve error handling with consistent VizzlyError usage - Add @throws JSDoc annotations for better API documentation - Remove duplicated code from client and SDK implementations This refactoring addresses PR feedback by: - Providing clear error messages with both original and resolved paths - Ensuring consistent file lookup behavior across the codebase - Making future enhancements (like PNG validation) easier to maintain - Standardizing error types across client and SDK
|
@claude can you reevaluate this PR? |
|
Claude finished @Robdel12's task —— View job Re-evaluating PR #52 ✅Progress
Review SummaryGreat work addressing the feedback! 🎉 I've reviewed the updated PR and I'm pleased to see that all critical issues and most high-priority recommendations have been successfully addressed. ✅ Issues Addressed1. Critical: Missing fs imports ✅ FIXEDStatus: The critical issue has been partially resolved, but there's still an issue: Location: The client file now imports import { existsSync, readFileSync } from 'fs';However, these imports are no longer used in the client file since you've correctly extracted the file reading logic to the shared utility! The client now uses Action needed: Remove the unused imports on line 13: // Remove this line - no longer needed:
import { existsSync, readFileSync } from 'fs';2. Shared Utility Function ✅ EXCELLENTStatus: Implemented perfectly! You've created
This is exactly what I recommended in item #10 from my previous review! The implementation is clean, maintainable, and follows best practices. 3. Path Resolution ✅ FIXEDStatus: Resolved in the shared utility! The const filePath = resolve(imageBufferOrPath);This ensures consistent behavior regardless of the current working directory. The error messages also include both the original path and the resolved path for debugging. 4. Error Consistency ✅ FIXEDStatus: Now consistent across both SDK and client! Both the SDK and client now use the shared utility, which throws
5. Test Coverage ✅ COMPREHENSIVEStatus: Excellent test coverage added! You've added comprehensive tests:
The tests verify:
📋 Remaining Items from Previous Review✅ Addressed
|
Why
Currently, our SDKs require users to pass a
Bufferfor screenshots, which creates unnecessary friction in the developer experience. Many testing frameworks (Playwright, Cypress, Puppeteer) save screenshots directly to disk, forcing developers to:vizzlyScreenshot()This PR eliminates step 2 by allowing users to pass file paths directly to the SDK. This is especially valuable when:
What Changed
SDK Updates
vizzlyScreenshot()to acceptBuffer | string(client SDK)screenshot()andcompare()methods to acceptBuffer | stringtypeofcheckVizzlyErrorTest Coverage
tests/sdk/sdk-core.spec.jstests/unit/client-file-path.spec.jswith 5 client-specific testsDocumentation
README.mdwith both Buffer and file path examplesdocs/test-integration.mdwith framework-specific examplesdocs/api-reference.mdwith complete API documentationBackward Compatibility
✅ Fully backward compatible - all existing Buffer usage continues to work unchanged. All 584 tests pass.
Examples
Before (still supported)
After (now also supported)
Resolves VIZ-58