fix/bug-snowflake-key-pair-auth-private-key-does-not-get-removed-upon… #36270
Conversation
…-removing-from-modal
WalkthroughThe changes introduced in this pull request include the addition of a test suite for the Changes
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: 0
Outside diff range and nitpick comments (1)
app/client/src/components/formControls/FilePickerControl.test.tsx (1)
57-84: Excellent test case for file selection!This test case thoroughly covers the file selection functionality by simulating a user selecting a file and verifying that the file name appears in the component. The use of
actandwaitForis appropriate for handling the asynchronous nature of file selection.One small suggestion:
Consider removing the console.log statements as they are not necessary for the test case. Apply this diff to remove them:
- console.log('File input found, uploading file...'); await userEvent.upload(fileInput, file); - console.log('File uploaded.'); ... - console.log('Waiting for file name to appear...'); expect(screen.getByText("example.txt")).toBeInTheDocument();
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/components/formControls/FilePickerControl.test.tsx (1 hunks)
- app/client/src/components/formControls/FilePickerControl.tsx (3 hunks)
Additional comments not posted (4)
app/client/src/components/formControls/FilePickerControl.test.tsx (1)
52-55: Great work on the rendering test case!This test case provides a solid foundation by ensuring that the
FilePickerControlcomponent renders correctly and the "Select" text is present in the rendered output.app/client/src/components/formControls/FilePickerControl.tsx (3)
2-2: Great job importing the necessary React hooks!The addition of
useState,useEffect, anduseCallbackhooks is a step in the right direction to enhance the component's lifecycle management and optimize performance. Keep up the good work!
50-50: Excellent work defining and exporting theRenderFilePickerfunction!The function is correctly defined with the appropriate props type, and it follows the TypeScript syntax. Exporting this function aligns with the list of alterations, indicating that it will be used in other parts of the codebase. Well done!
86-88: Fantastic work handling the case when no file is uploaded!The addition of the
elseblock within theuseEffecthook is a great improvement to the control flow. By explicitly callingprops.input?.onChange("")whenappFileToBeUploadedis not set, you ensure that the input is cleared, preventing stale data from being retained in the input state. This enhances the user experience and maintains data consistency.Moreover, the use of the optional chaining operator
?.is a good practice to safely access theonChangemethod, avoiding potential errors ifprops.inputis undefined.Keep up the excellent work in improving the component's behavior and user experience!
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10881060896. |
| @@ -48,7 +47,7 @@ type RenderFilePickerProps = FilePickerControlProps & { | |||
| onChange: (event: any) => void; | |||
| }; | |||
|
|
|||
| function RenderFilePicker(props: RenderFilePickerProps) { | |||
| export function RenderFilePicker(props: RenderFilePickerProps) { | |||
There was a problem hiding this comment.
@Jagadeesh-90 Is this export needed here? As we are already exporting this whole component at the bottom
| @@ -1,5 +1,5 @@ | |||
| import * as React from "react"; | |||
| import { useState } from "react"; | |||
| import { useState, useEffect, useCallback } from "react"; | |||
There was a problem hiding this comment.
Are these useEffect and useCallback imports needed?
| userEvent.click(screen.getByText("Select")); | ||
| userEvent.click(screen.getByText("Browse")); | ||
|
|
||
| const fileInput = document.querySelector('input[type="file"]') as HTMLInputElement; |
There was a problem hiding this comment.
Instead of using as, can we define type for fileInput?
|
Deploy-Preview-URL: https://ce-36270.dp.appsmith.com |
|
|
||
| await act(async () => { | ||
| if (fileInput) { | ||
| console.log('File input found, uploading file...'); |
There was a problem hiding this comment.
Are these console logs needed here?
| } | ||
| }); | ||
|
|
||
| await waitFor(() => { |
There was a problem hiding this comment.
Should we also add a test case for when we remove the file from file picker? Asserting that input is blank?
Issue Link
Summary by CodeRabbit
New Features
FilePickerControlcomponent for improved file selection in forms.RenderFilePickercomponent to enhance user interaction with file uploads.Bug Fixes
Tests
FilePickerControlcomponent to ensure correct rendering and functionality.