feat: Added warning message in response pane while prepared statement is on #36407
Conversation
github-actions
bot
added
IDE Navigation
|
/build-deploy-preview skip-test=true |
WalkthroughThe pull request introduces a new warning mechanism in the application related to prepared statements. It adds a constant for the warning message and modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10935591147. |
…o action-redesign/prepared-statement-warning
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10935666183. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx (1)
213-215: Include dependencies in youruseCallbackhookTo ensure that
navigateToSettingsupdates correctly when dependencies change, it's advisable to includedispatchin the dependency array of youruseCallbackhook. This practice aligns with React's guidelines for hooks and prevents potential stale closures.You can modify your code as follows:
const navigateToSettings = useCallback(() => { dispatch(setQueryPaneConfigSelectedTabIndex(EDITOR_TABS.SETTINGS)); - }, []); + }, [dispatch]);app/client/src/ce/constants/messages.ts (2)
2508-2508: Remember to maintain consistent punctuation in messagesIn the
MESSAGEstring, consider adding a period at the end for consistency with other messages that use complete sentences.Apply this change:
-"Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again" +"Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again."Consistency in punctuation enhances readability and maintains a professional tone throughout the application.
2506-2510: Consider internationalization support for the new messagesTo support localization, ensure that the new messages are compatible with the application's internationalization framework.
If the application uses a localization library, consider adding these messages to the appropriate locale files. This will make it easier to support multiple languages and reach a broader audience.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/constants/messages.ts (1 hunks)
- app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx (6 hunks)
Additional comments not posted (7)
app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsx (6)
1-1: Excellent use of React hooksYou've correctly imported
useCallbackanduseStatefrom React. This will help manage your component's state and memoize functions efficiently.
16-21: Components imported successfully from "@appsmith/ads"It's great to see that you've imported
Callout,Flex,SegmentedControl, andCalloutLinkPropsproperly. These components will enhance your UI and provide better user interaction.
40-45: Necessary imports for action dispatching and messagesWell done importing
setQueryPaneConfigSelectedTabIndex,EDITOR_TABS,createMessage, andPREPARED_STATEMENT_WARNING. These imports are essential for navigation and displaying messages to the user.
165-165: InitializingshowPreparedStatementWarningcorrectlyGood job initializing
showPreparedStatementWarningtofalse. This sets the default state appropriately before any conditions are applied.
217-223: ConfiguringpreparedStatementCalloutLinksappropriatelyYou've set up
preparedStatementCalloutLinkscorrectly with the necessaryonClickhandler and link text. This will provide users with a helpful navigation option directly from the warning message.
235-239: Conditional rendering of the warning messageExcellent implementation of the conditional rendering for the
Calloutcomponent. Displaying the warning only whenshowPreparedStatementWarningistrueensures users are informed at the appropriate times without cluttering the UI.app/client/src/ce/constants/messages.ts (1)
2506-2510: Verify the usage ofPREPARED_STATEMENT_WARNINGin the codebasePlease ensure that the new constant
PREPARED_STATEMENT_WARNINGis correctly utilized wherever necessary in the application, and that any components or views that display this message handle it appropriately.Run the following script to check where
PREPARED_STATEMENT_WARNINGis used:This will help verify that the constant is being used as intended and highlight any places where it might be missing.
Verification successful
Class, let's take a closer look at how our new constant is being used in the QueryResponseTab component. We'll use a special tool to peek into the file and see the surrounding code. Here's what we're going to do:
Now, let's run this script and see what we can learn from it!
Alright, class! We've done our homework and examined how our new
PREPARED_STATEMENT_WARNINGconstant is being used. I'm pleased to report that everything looks ship-shape! Let's review our findings:
PREPARED_STATEMENT_WARNINGis correctly implemented and usedGreat job, developers! The new constant is being used exactly as intended in the QueryResponseTab component. Here's what we observed:
- The constant is properly imported and utilized.
- Both the MESSAGE and LINK properties are being accessed correctly.
- The warning is displayed in a Callout component, which is perfect for this type of alert.
- There's a condition in place to show the warning only when necessary.
Keep up the good work! This implementation shows attention to detail and follows best practices. No further changes are needed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of PREPARED_STATEMENT_WARNING in the codebase # Test: Find all occurrences of PREPARED_STATEMENT_WARNING # Expect: Usages in the relevant components or views rg --type ts --no-heading "PREPARED_STATEMENT_WARNING"Length of output: 477
Script:
#!/bin/bash # Description: View the context of PREPARED_STATEMENT_WARNING usage in QueryResponseTab.tsx # Test: Show the lines where PREPARED_STATEMENT_WARNING is used, with 5 lines of context before and after # Expect: To see how the constant is being implemented in the component rg --type ts -C 5 "PREPARED_STATEMENT_WARNING" app/client/src/pages/Editor/QueryEditor/QueryResponseTab.tsxLength of output: 1075
| const { pluginSpecifiedTemplates } = | ||
| currentActionConfig.actionConfiguration; | ||
|
|
||
| if (error && pluginSpecifiedTemplates[0].value === true) { | ||
| showPreparedStatementWarning = true; | ||
| } |
There was a problem hiding this comment.
Ensure pluginSpecifiedTemplates is defined before accessing
To prevent potential runtime errors, it's important to check if pluginSpecifiedTemplates is defined and has elements before accessing pluginSpecifiedTemplates[0].value. This validation will make your code more robust and error-resistant.
Consider applying this change:
const { pluginSpecifiedTemplates } =
currentActionConfig.actionConfiguration;
- if (error && pluginSpecifiedTemplates[0].value === true) {
+ if (
+ error &&
+ pluginSpecifiedTemplates &&
+ pluginSpecifiedTemplates.length > 0 &&
+ pluginSpecifiedTemplates[0].value === true
+ ) {
showPreparedStatementWarning = true;
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { pluginSpecifiedTemplates } = | |
| currentActionConfig.actionConfiguration; | |
| if (error && pluginSpecifiedTemplates[0].value === true) { | |
| showPreparedStatementWarning = true; | |
| } | |
| const { pluginSpecifiedTemplates } = | |
| currentActionConfig.actionConfiguration; | |
| if ( | |
| error && | |
| pluginSpecifiedTemplates && | |
| pluginSpecifiedTemplates.length > 0 && | |
| pluginSpecifiedTemplates[0].value === true | |
| ) { | |
| showPreparedStatementWarning = true; | |
| } |
| export const PREPARED_STATEMENT_WARNING = { | ||
| MESSAGE: () => | ||
| "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again", | ||
| LINK: () => "Open settings", | ||
| }; |
There was a problem hiding this comment.
Well done on introducing the PREPARED_STATEMENT_WARNING constant
You've defined a new constant to display a warning message when prepared statements are enabled and an error occurs. This aligns perfectly with the PR objectives and enhances user awareness during error scenarios.
However, let's consider a small improvement to keep our code consistent and maintainable.
To maintain consistency with other message constants in this file, it would be better to define MESSAGE and LINK as constant strings rather than arrow functions, unless dynamic computation is required. Many existing constants use direct string assignments.
Here's how you might refactor the code:
-export const PREPARED_STATEMENT_WARNING = {
- MESSAGE: () =>
- "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again",
- LINK: () => "Open settings",
+export const PREPARED_STATEMENT_WARNING_MESSAGE =
+ "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again.";
+export const PREPARED_STATEMENT_WARNING_LINK = "Open settings";This change makes the constants consistent with the rest of the file and simplifies their usage.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const PREPARED_STATEMENT_WARNING = { | |
| MESSAGE: () => | |
| "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again", | |
| LINK: () => "Open settings", | |
| }; | |
| export const PREPARED_STATEMENT_WARNING_MESSAGE = | |
| "Prepared Statements are currently enabled, which may be causing the query error. Turn them off and try running the query again."; | |
| export const PREPARED_STATEMENT_WARNING_LINK = "Open settings"; |
|
Deploy-Preview-URL: https://ce-36407.dp.appsmith.com |
1 similar comment
|
Deploy-Preview-URL: https://ce-36407.dp.appsmith.com |
…o action-redesign/prepared-statement-warning
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10937465273. |
|
Deploy-Preview-URL: https://ce-36407.dp.appsmith.com |
…o action-redesign/prepared-statement-warning
albinAppsmith
deleted the
action-redesign/prepared-statement-warning
branch
…36493) ## Description Query instance was breaking because of missing prepared statement obj in the action configuration. PR caused this issue: #36407 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!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="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11007670447> > Commit: 420801e > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11007670447&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 24 Sep 2024 07:04:23 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Enhanced logic for displaying warnings related to prepared statements, improving user experience by streamlining checks. - **Bug Fixes** - Resolved issues with warning display for prepared statements, ensuring accurate notifications based on user actions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… is on (appsmithorg#36407) ## Description Added warning message in response pane to show that use prepared statement is turned on. This will be showing up only if there is an error. Fixes appsmithorg#36326 ## Automation /ok-to-test tags="@tag.Sanity, @tag.Datasource" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10987691166> > Commit: debc9f3 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10987691166&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.Datasource` > Spec: > <hr>Mon, 23 Sep 2024 05:25:23 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a warning message for users regarding prepared statements that may cause query errors. - Added a mechanism to navigate users to the settings page for resolving issues related to prepared statements. - Enhanced the Query Response Tab with contextual warnings and improved user guidance. - **Bug Fixes** - Improved error handling related to prepared statements in the query response. - **Tests** - Added comprehensive unit tests for the Query Response Tab to ensure correct behavior of the prepared statement warning under various conditions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ppsmithorg#36493) ## Description Query instance was breaking because of missing prepared statement obj in the action configuration. PR caused this issue: appsmithorg#36407 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!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="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11007670447> > Commit: 420801e > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11007670447&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 24 Sep 2024 07:04:23 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Enhanced logic for displaying warnings related to prepared statements, improving user experience by streamlining checks. - **Bug Fixes** - Resolved issues with warning display for prepared statements, ensuring accurate notifications based on user actions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Added warning message in response pane to show that use prepared statement is turned on. This will be showing up only if there is an error.
Fixes #36326
Automation
/ok-to-test tags="@tag.Sanity, @tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10987691166
Commit: debc9f3
Cypress dashboard.
Tags:
@tag.Sanity, @tag.DatasourceSpec:
Mon, 23 Sep 2024 05:25:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests