Bust cache in generated image preview - #7860
Conversation
dosubot
Bot
added
size:M
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Thank you for your contribution! The implementation successfully adds cache-busting to prevent browser caching issues with generated images. The solution is clean and includes good test coverage. I've left a few minor suggestions for improvement.
|
|
||
| // Add cache-busting parameter to prevent browser caching issues | ||
| const cacheBuster = Date.now() | ||
| imageUri = imageUri.includes("?") ? `${imageUri}&t=${cacheBuster}` : `${imageUri}?t=${cacheBuster}` |
There was a problem hiding this comment.
Consider using a more descriptive parameter name for clarity. Instead of t, perhaps cache or cb (cache-buster) would make the purpose more obvious?
| imageUri = imageUri.includes("?") ? `${imageUri}&t=${cacheBuster}` : `${imageUri}?t=${cacheBuster}` | |
| // Add cache-busting parameter to prevent browser caching issues | |
| const cacheBuster = Date.now() | |
| imageUri = imageUri.includes("?") ? `${imageUri}&cache=${cacheBuster}` : `${imageUri}?cache=${cacheBuster}` |
| const imageUri = provider?.convertToWebviewUri?.(fullImagePath) ?? vscode.Uri.file(fullImagePath).toString() | ||
| let imageUri = provider?.convertToWebviewUri?.(fullImagePath) ?? vscode.Uri.file(fullImagePath).toString() | ||
|
|
||
| // Add cache-busting parameter to prevent browser caching issues |
There was a problem hiding this comment.
Would it be helpful to add a brief comment explaining why cache-busting is needed? This could help future maintainers understand the context, something like:
| // Add cache-busting parameter to prevent browser caching issues | |
| // Add cache-busting parameter to prevent browser caching issues | |
| // This ensures the webview always displays the newly generated image instead of a cached version | |
| const cacheBuster = Date.now() |
| expect(imageData.imageUri).toMatch(/\?t=\d+$/) | ||
| expect(imageData.imagePath).toBe("/test/workspace/test-image.png") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Great test coverage! Consider adding an additional test case to verify the behavior when the URI already contains query parameters (testing the includes("?") branch). This would ensure both code paths are covered:
it("should append cache-busting parameter to URI with existing query params", async () => {
const mockWebviewUri = "https://file+.vscode-resource.vscode-cdn.net/test/workspace/test-image.png?existing=param"
// ... rest of test
expect(imageData.imageUri).toMatch(/\?existing=param&t=\d+$/)
})
hannesrudolph
added
the
Issue/PR - Triage
|
@roomote-agent fix the tests so they pass on windows as well |
|
Looking into the Windows test failures now. I'll fix the cross-platform compatibility issues. |
…mageTool - Replace backslashes with forward slashes in imagePath to ensure consistent behavior across platforms - Fixes test failure on Windows where path.join() creates OS-specific separators
…enerateImageTool" This reverts commit 80876ab.
- Update test to handle both Unix and Windows path separators - Keep the actual implementation unchanged as it correctly uses OS-specific paths - Test now checks for the appropriate path format based on process.platform
|
✅ Fixed! The Windows tests are now passing. The issue was that the test was expecting Unix-style path separators ( I've updated the test to handle both Unix and Windows path formats by checking All CI checks are green now, including the Windows platform tests. |
4 participants
Important
Adds cache-busting parameter to image URIs in
generateImageTool.tsto prevent browser caching issues, with tests verifying the change.generateImageTool.tsto prevent browser caching issues.generateImageTool.test.tsto verify cache-busting parameter is added to image URI.mockCline.sayis called with image data containing cache-busting parameter.This description was created by
for d40af13. You can customize this summary. It will automatically update as commits are pushed.