chore: fixed image not downloading functionality and ImageWidget cypr… #34786
Conversation
WalkthroughThe changes introduce a function to convert image URLs to base64 and update the ImageWidget component to handle image downloads using this function. Tests have been included to validate image rendering, downloading, and button visibility. These changes address the issue of images not downloading properly by ensuring that the download button provides a base64-encoded version of the image. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageComponent
participant Helper as urlToBase64 Function
User->>ImageComponent: Hover over Image
ImageComponent-->>User: Show Download Button
User->>ImageComponent: Click Download Button
ImageComponent->>Helper: Convert Image URL to Base64
Helper-->>ImageComponent: Return Base64 String
ImageComponent-->>User: Initiate Download with Base64 URL
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (5)
app/client/src/widgets/ImageWidget/helper.ts (2)
2-2: Ensure consistent return types.The function should consistently return a string. The current implementation returns an empty string for invalid URLs but could benefit from explicit typing to ensure consistency.
export const urlToBase64 = async (url: string): Promise<string> => {
5-8: Improve error message clarity.Specify the URL in the error message to improve debugging.
throw new Error(`Network response was not ok for URL: ${url}`);app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts (1)
72-74: Use consistent assertions for Cypress tests.Ensure consistency by using the same assertion method throughout the test.
cy.wrap(base64Url).then((url) => { // This is to validate the final base64 url which is used for download as href in the anchor tag agHelper.AssertAttribute(widgetLocators.imageDownloadBtn, "href", url); });app/client/src/widgets/ImageWidget/component/index.tsx (2)
7-7: Ensure consistent import structure.Maintain a consistent import structure for better readability.
import { urlToBase64 } from "../helper";
145-145: Initialize state properties directly.Initialize state properties directly in the state object for better readability.
downloadUrl: "",
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts (2 hunks)
- app/client/src/widgets/ImageWidget/component/index.tsx (5 hunks)
- app/client/src/widgets/ImageWidget/helper.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts (1)
Pattern
app/client/cypress/**/**.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait in code.
Avoid using cy.pause in code.
Use variables for locators, not strings.
Usedata-*attributes for selectors; avoid Xpaths and CSS attributes.
Avoid selectors like.btn.submitorbutton[type=submit].
Perform logins via API withLoginFromAPI.
Only interact with controlled sites/servers.
Ensure tests can run withit.onlyand are independent.
Usebefore,beforeEach,after,afterEachcorrectly; clean state before tests.
Check new specs for flakiness by running them 10 times on CI.
Use multiple assertions; don't treat Cypress as unit tests.
Use constants for strings.
Include datasource operations inbeforehooks.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/widgets/ImageWidget/component/index.tsx (5 hunks)
- app/client/src/widgets/ImageWidget/helper.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/ImageWidget/component/index.tsx
Additional context used
Learnings (1)
app/client/src/widgets/ImageWidget/helper.ts (2)
Learnt from: Shivam-z PR: appsmithorg/appsmith#34786 File: app/client/src/widgets/ImageWidget/helper.ts:14-15 Timestamp: 2024-07-09T06:18:19.736Z Learning: Handling errors directly within utility functions like `urlToBase64` is a good practice to ensure consistency and simplicity in error management. Avoid using `console.error` in production code; consider using a logging library or handling errors upstream.Learnt from: Shivam-z PR: appsmithorg/appsmith#34786 File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts:22-24 Timestamp: 2024-07-09T04:45:28.800Z Learning: Handling errors directly within utility functions like `urlToBase64` is a good practice to ensure consistency and simplicity in error management.
Additional comments not posted (6)
app/client/src/widgets/ImageWidget/helper.ts (6)
1-2: LGTM!The imports are appropriate for the functionality provided.
3-4: LGTM!The function declaration and initial null check are appropriate.
5-6: LGTM!The try block and fetch call are appropriate.
7-8: LGTM!The response validation is appropriate for handling network errors.
10-11: LGTM!The arrayBuffer conversion and content type extraction are appropriate.
12-13: LGTM!The base64 conversion and return statement are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/ImageWidget/component/index.test.tsx (1 hunks)
Additional comments not posted (3)
app/client/src/widgets/ImageWidget/component/index.test.tsx (3)
1-7: Review of imports and mock setup.The imports and the mock setup for
urlToBase64are correctly implemented. The use ofjest.mockto simulate the behavior of theurlToBase64function is appropriate for testing the component's interaction with external modules.
11-19: Review of lifecycle hooks for testing.The use of
beforeEachandafterEachto manage the DOM container for the tests is a good practice. It ensures that each test starts with a clean slate, which is crucial for avoiding side effects between tests.
31-115: Review of test cases in<ImageComponent />.
- Rendering Test: The test case for rendering the image checks if the image is rendered with the correct
srcattribute. This is a basic functionality test and is implemented correctly.- Interaction Test: The test for rendering the download button on hover and checking its functionality on click is crucial. The use of
fireEventto simulate user interactions andwaitForto handle asynchronous operations are correctly used.- Conditional Rendering Test: The test for not rendering the download button when URLs are empty is important for ensuring the component behaves correctly under edge conditions. The use of
queryByTestIdto assert the absence of the download button is appropriate.Overall, the test cases are well-structured and cover the key functionalities of the component.
|
Hi @rajatagrawal , I have added RTL unit test cases for Image Widget , could you please review this PR. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hi @rajatagrawal @ApekshaBhosale @sagar-qa007 , please review this PR else it will be auto closed in some days. |
|
@rahulbarwal Can you please help review the PR ? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@rajatagrawal @rahulbarwal please look into this PR. |
Shivam-z
dismissed stale reviews from sagar-qa007 and coderabbitai[bot]
via
9624492
Description
In this PR I have modified 3 file chages.
I have created helper.tsx file *app/client/src/widgets/ImageWidget/helper.ts in which I have defined one method urlToBase64 which will convert imageurl into base64Url.
app/client/src/widgets/ImageWidget/component/index.tsx: In this I have made one new state called downloadUrl which will takes the value from urlToBase64 method and later define the href value.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image3_Spec.ts: I have modified the downloadImage test case according to this implementation.
summary:
As we are fetching image url for base64 conversion. Therefore, this solution only works for images that are hosted on server which allows cross origin request and for images that are hosted on server which doesn't allow cross origin request (getting cors error) the functionality will works as it was working previously.
suggestion for Restiricted Images:
for images hosted on server which does not allow cross origin request, the backend should create base64 url and return to frontend.
Fixes #24632
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
ImageComponent.ImageComponent.Tests
ImageComponent, covering image rendering, download button interactions, and edge cases for image URLs.