fix: Fall of the error toasts wall #35839
Conversation
WalkthroughThe changes involve significant enhancements to error handling mechanisms throughout the application, particularly in Redux action creators, sagas, and testing frameworks. Key updates include the integration of structured error payloads, the transition from specific method calls related to debugger functionality, and the introduction of new saga functions to manage debugger visibility. These modifications are intended to improve clarity in error management and streamline interactions within the debugging interface. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DebuggerHelper
participant Debugger
participant Redux
User->>DebuggerHelper: OpenDebugger()
DebuggerHelper->>Debugger: Check for Tabs
alt Tabs Not Present
DebuggerHelper->>Debugger: Click Debugger Icon
end
Debugger-->>User: Show Debugger Interface
User->>Redux: Perform Action
Redux-->>User: Display Result/Error
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
|
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![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, codebase verification and nitpick comments (2)
app/client/src/sagas/JSLibrarySaga.ts (1)
277-279: Consider adding user feedback for uninstall failures.While the console error provides insight for developers, consider if additional user feedback is necessary when an uninstall operation fails, as users might not be aware of the failure without a notification.
app/client/src/ce/sagas/JSActionSagas.ts (1)
276-278: Enhance error feedback for move failures.Consider if additional user feedback is necessary when a move operation fails, as users might not be aware of the failure without a notification. Structured error payloads are great for state management, but user feedback is also crucial.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (14)
- app/client/src/actions/jsActionActions.ts (3 hunks)
- app/client/src/actions/pluginActionActions.ts (3 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (2 hunks)
- app/client/src/ce/sagas/JSActionSagas.ts (3 hunks)
- app/client/src/reducers/entityReducers/datasourceReducer.ts (1 hunks)
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts (4 hunks)
- app/client/src/sagas/ActionSagas.ts (4 hunks)
- app/client/src/sagas/ApiPaneSagas.ts (3 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (5 hunks)
- app/client/src/sagas/JSLibrarySaga.ts (4 hunks)
- app/client/src/sagas/JSPaneSagas.ts (2 hunks)
- app/client/src/sagas/OneClickBindingSaga.ts (1 hunks)
- app/client/src/sagas/PartialImportExportSagas/PartialExportSagas.ts (1 hunks)
- app/client/src/sagas/QueryPaneSagas.ts (3 hunks)
Additional comments not posted (25)
app/client/src/sagas/PartialImportExportSagas/PartialExportSagas.ts (1)
85-87: Good job on structuring the error payload!The new error payload structure with a message property enhances error reporting. Ensure that the error message is consistent with the application's error handling strategy.
Consider verifying that similar error messages across the application are consistent and informative.
app/client/src/actions/jsActionActions.ts (2)
69-69: Nice enhancement with the error payload!Adding an optional error property improves error handling. Ensure that this information is effectively utilized in the application.
Consider verifying that the error information is being used effectively in the UI or logs.
98-98: Great addition with the error property!The optional error property enhances error handling. Ensure that this information is properly utilized in the application.
Consider verifying that the error information is being used effectively in the UI or logs.
app/client/src/actions/pluginActionActions.ts (2)
212-212: Well done on enhancing the error payload!The optional error property improves error handling. Ensure that this information is effectively utilized in the application.
Consider verifying that the error information is being used effectively in the UI or logs.
241-241: Great work on adding the error property!The optional error property enhances error handling. Ensure that this information is properly utilized in the application.
Consider verifying that the error information is being used effectively in the UI or logs.
app/client/src/sagas/JSLibrarySaga.ts (2)
246-254: Ensure consistency in error payload structure.The error payload structure has been enhanced to include an error object with a message. Ensure this structure is consistently used across all error actions for clarity and maintainability.
301-311: Verify error handling consistency.Ensure that the error handling for the uninstall saga aligns with the overall strategy of structured error payloads and logging. Consistency in error handling improves maintainability and user experience.
app/client/src/sagas/OneClickBindingSaga.ts (1)
377-384: Improve error handling robustness.The change from
anytounknownis a good practice for type safety. Ensure that all error handling throughout the application follows this pattern for consistency and robustness.app/client/src/ce/sagas/JSActionSagas.ts (2)
195-202: Ensure error messages are user-friendly.The error message for copying JS collections is now part of the action payload. Ensure that these messages are user-friendly and provide enough context for users to understand the issue.
382-387: Verify error handling consistency.Ensure that the error handling for saving JS object names aligns with the overall strategy of structured error payloads and logging. Consistency in error handling improves maintainability and user experience.
app/client/src/sagas/QueryPaneSagas.ts (1)
495-503: Great job on centralizing error handling!The shift to using Redux actions for error handling instead of direct Sentry logging is a commendable change. This promotes centralized error management and aligns with the PR objectives to reduce error toast messages. The
logToSentryflag is a useful addition for conditional logging. Keep up the good work!app/client/src/reducers/entityReducers/datasourceReducer.ts (1)
373-373: Well done on categorizing error actions!Switching the action type to
ReduxActionErrorTypesfor handling OAuth access token errors is a positive step. It enhances the structure and clarity of the code by clearly distinguishing error actions from regular actions. This change aligns with best practices for maintainable code. Keep it up!app/client/src/sagas/ApiPaneSagas.ts (1)
745-752: Excellent work on centralizing error handling!The transition to using Redux actions for error handling instead of direct Sentry logging is a commendable change. This promotes centralized error management and aligns with the PR objectives to reduce error toast messages. The
logToSentryflag is a useful addition for conditional logging. Keep up the great work!app/client/src/sagas/JSPaneSagas.ts (1)
373-380: Great job integrating Redux for error handling!Switching from toast notifications to Redux actions for error handling is a smart move. This change will help in managing errors more effectively and allows for better state management. Keep up the good work!
app/client/src/sagas/ActionSagas.ts (2)
710-712: Nice improvement in error handling!Encapsulating error messages within Redux actions instead of using toast notifications enhances the flexibility of error management. This change aligns with the goal of reducing user-facing error toasts. Well done!
799-804: Good job on centralizing error handling!By including error messages within the
copyActionErroraction payload, you're making error management more consistent and maintainable. This change supports the objective of reducing toast notifications. Keep it up!app/client/src/sagas/ActionExecution/PluginActionSaga.ts (3)
14-14: Great addition ofexecutePageLoadActions!This addition suggests improved handling of page load actions, potentially enhancing modularity and error management. Well done on making the code more organized and maintainable!
75-75: Nice import restructuring!Consolidating imports from
entities/Actionimproves readability and maintainability. This change makes the code cleaner and easier to navigate. Keep up the good work!
175-175: Excellent use of structured logging for errors!Replacing toast notifications with
AppsmithConsole.addErrorsenhances error traceability and aligns with best practices for error management. This change improves clarity and robustness. Great job!app/client/src/ce/constants/ReduxActionConstants.tsx (1)
381-381: Good job on categorizing error types!Moving
GET_OAUTH_ACCESS_TOKEN_ERRORtoUserAuthActionErrorTypesenhances clarity in error management.app/client/src/sagas/DatasourcesSagas.ts (5)
473-473: Ensure error payload consistency.The error payload now includes the
idanderrorproperties. Make sure this format is consistent across all similar actions.
734-738: Streamlined error handling ingetOAuthAccessTokenSaga.The error action now includes a
messagefield, which provides clearer context for the error encountered. This change is consistent with the new error handling strategy.
797-801: Consistent error payload structure.The error payload structure in
getOAuthAccessTokenSagahas been updated to include amessagefield. Ensure this structure is consistently applied across all error actions.
910-922: Improved error reporting intestDatasourceSaga.The error payload now includes a
messagefield that consolidates error information, enhancing clarity in error reporting.
2094-2094: Consistent error handling inupdateDatasourceAuthStateSaga.The error payload now includes a
messagefield, which aligns with the new error handling strategy.
| kind: "error", | ||
| }, | ||
| ); | ||
| AppsmithConsole.error({ |
There was a problem hiding this comment.
Use AppsmithConsole.addErrors here
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/JSLibrarySaga.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/JSLibrarySaga.ts
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/ErrorSagas.tsx (3 hunks)
Additional comments not posted (3)
app/client/src/sagas/ErrorSagas.tsx (3)
193-193: Enum value update is clear and consistent.The change from
LOG_ERRORtoLOG_TO_CONSOLEenhances clarity by explicitly describing the action being performed.
225-225: Switch statement update is correctly implemented.The update to handle
LOG_TO_CONSOLEensures that errors are logged to the console, aligning with the intended functionality.
205-207: Verify the impact of removing the default value forshow.The removal of the default value for
showin the destructuring ofpayloadmight affect how error messages are displayed. Ensure that this change does not introduce unintended behavior.Run the following script to verify the usage of
errorSagaand check for any dependencies on the default value ofshow:
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- app/client/src/actions/jsActionActions.ts (3 hunks)
- app/client/src/actions/pluginActionActions.ts (3 hunks)
- app/client/src/ce/sagas/JSActionSagas.ts (3 hunks)
- app/client/src/reducers/uiReducers/debuggerReducer.ts (1 hunks)
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts (3 hunks)
- app/client/src/sagas/ActionSagas.ts (4 hunks)
- app/client/src/sagas/ApiPaneSagas.ts (3 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (5 hunks)
- app/client/src/sagas/JSLibrarySaga.ts (4 hunks)
- app/client/src/sagas/JSPaneSagas.ts (2 hunks)
- app/client/src/sagas/OneClickBindingSaga.ts (1 hunks)
- app/client/src/sagas/QueryPaneSagas.ts (3 hunks)
Files skipped from review as they are similar to previous changes (10)
- app/client/src/actions/jsActionActions.ts
- app/client/src/actions/pluginActionActions.ts
- app/client/src/ce/sagas/JSActionSagas.ts
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts
- app/client/src/sagas/ActionSagas.ts
- app/client/src/sagas/ApiPaneSagas.ts
- app/client/src/sagas/DatasourcesSagas.ts
- app/client/src/sagas/JSPaneSagas.ts
- app/client/src/sagas/OneClickBindingSaga.ts
- app/client/src/sagas/QueryPaneSagas.ts
Additional comments not posted (3)
app/client/src/reducers/uiReducers/debuggerReducer.ts (1)
23-23: Good change for error visibility.Setting
hideErrorstofalseby default enhances error visibility, aligning with the goal of improving the user experience by making errors more apparent.app/client/src/sagas/JSLibrarySaga.ts (2)
62-66: Improved error reporting structure.The structured payload with
showandmessageproperties provides better context for error reporting, enhancing clarity and user feedback.
251-259: Enhanced error handling with structured payloads.Replacing toast notifications with structured error payloads improves the clarity and consistency of error reporting, aligning with the goal of reducing excessive error messages.
Also applies to: 282-294, 316-327
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/sagas/DebuggerSagas.ts (1)
821-830: Add comments to explain the logic.The function
handleHideDebuggerSagais well-structured and correctly handles the action. However, adding comments to explain the logic would improve readability and maintainability.Consider adding comments like this:
function* handleHideDebuggerSaga(action: ReduxAction<boolean>) { const { payload } = action; const currentDebuggerErrors: Record<string, Log> = yield select(getDebuggerErrors); // If there are current debugger errors and the payload indicates to hide the debugger if (objectKeys(currentDebuggerErrors).length && !payload) { // Show the debugger yield put(showDebugger()); // Set the selected tab to ERROR_TAB yield put(setDebuggerSelectedTab(DEBUGGER_TAB_KEYS.ERROR_TAB)); } }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/DebuggerSagas.ts (5 hunks)
Additional comments not posted (1)
app/client/src/sagas/DebuggerSagas.ts (1)
843-843: LGTM!The addition of the new action listener for
HIDE_DEBUGGER_ERRORSis correct and follows the existing pattern.The code changes are approved.
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/client/src/sagas/ActionExecution/PostMessageSaga.ts (2 hunks)
- app/client/src/sagas/ActionExecution/errorUtils.ts (2 hunks)
- app/client/src/sagas/ActionExecution/geolocationSaga.test.ts (2 hunks)
- app/client/src/sagas/ActionExecution/geolocationSaga.ts (5 hunks)
- app/client/src/sagas/EvaluationsSaga.ts (3 hunks)
- app/client/src/sagas/PostEvaluationSagas.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/sagas/PostEvaluationSagas.ts
Additional context used
Learnings (1)
app/client/src/sagas/EvaluationsSaga.ts (1)
Learnt from: ashit-rath PR: appsmithorg/appsmith#35086 File: app/client/src/ce/sagas/helpers.ts:35-37 Timestamp: 2024-07-22T12:21:58.545Z Learning: The `transformTriggerEvalErrors` function in `app/client/src/ce/sagas/helpers.ts` is typed as a generator function to maintain type safety because it gets overridden in another repository where `yield` statements are used.
Additional comments not posted (11)
app/client/src/sagas/ActionExecution/PostMessageSaga.ts (2)
Line range hint
7-9: LGTM!The function is correctly implemented.
The code changes are approved.
38-38: LGTM!The function is correctly implemented and the change improves user feedback by showing toast notifications for errors.
The code changes are approved.
app/client/src/sagas/ActionExecution/geolocationSaga.test.ts (2)
Line range hint
26-48: LGTM!The test is correctly implemented.
The code changes are approved.
89-89: LGTM!The test is correctly implemented and the change aligns with the new error handling mechanism.
The code changes are approved.
app/client/src/sagas/ActionExecution/errorUtils.ts (2)
Line range hint
58-79: LGTM!The function is correctly implemented and improves user feedback by showing toast notifications for errors.
The code changes are approved.
Tools
Biome
[error] 81-86: This generator function doesn't contain yield.
(lint/correctness/useYield)
81-85: LGTM!The function is correctly implemented and ensures that the user can access relevant debugging information.
The code changes are approved.
Tools
Biome
[error] 81-86: This generator function doesn't contain yield.
(lint/correctness/useYield)
app/client/src/sagas/ActionExecution/geolocationSaga.ts (4)
110-110: Verify the new error handling function.The function
showToastOnExecutionErrorreplaceslogActionExecutionError. Ensure thatshowToastOnExecutionErroris correctly handling the error messages and providing the necessary user feedback.Please confirm that
showToastOnExecutionErroris correctly implemented and tested.
125-125: Verify the new error handling function.The function
showToastOnExecutionErrorreplaceslogActionExecutionError. Ensure thatshowToastOnExecutionErroris correctly handling the error messages and providing the necessary user feedback.Please confirm that
showToastOnExecutionErroris correctly implemented and tested.
144-144: Verify the new error handling function.The function
showToastOnExecutionErrorreplaceslogActionExecutionError. Ensure thatshowToastOnExecutionErroris correctly handling the error messages and providing the necessary user feedback.Please confirm that
showToastOnExecutionErroris correctly implemented and tested.
173-173: Verify the new error handling function.The function
showToastOnExecutionErrorreplaceslogActionExecutionError. Ensure thatshowToastOnExecutionErroris correctly handling the error messages and providing the necessary user feedback.Please confirm that
showToastOnExecutionErroris correctly implemented and tested.app/client/src/sagas/EvaluationsSaga.ts (1)
353-353: Verify the new error handling function.The function
showDebuggerOnExecutionErrorreplacesdynamicTriggerErrorHandler. Ensure thatshowDebuggerOnExecutionErroris correctly handling the error messages and providing the necessary debugging information.Please confirm that
showDebuggerOnExecutionErroris correctly implemented and tested.
hetunandu
requested review from
ashit-rath
and removed request for
a team and
ApekshaBhosale
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/Widget_Error_spec.js (3 hunks)
- app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts (1 hunks)
- app/client/src/components/editorComponents/Debugger/LogItem.tsx (1 hunks)
- app/client/src/sagas/ActionExecution/errorUtils.ts (2 hunks)
- app/client/src/sagas/ActionExecution/geolocationSaga.test.ts (2 hunks)
- app/client/src/sagas/PostEvaluationSagas.ts (2 hunks)
- app/client/src/utils/AppsmithConsole.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/components/editorComponents/Debugger/LogItem.tsx
Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/Widget_Error_spec.js
- app/client/src/sagas/ActionExecution/geolocationSaga.test.ts
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Additional comments not posted (6)
app/client/src/utils/AppsmithConsole.ts (2)
31-31: Great simplification! The code changes look good.Replacing the
momentlibrary usage withDate.now().toString()simplifies the timestamp generation process and reduces the external dependency. This change is a positive improvement in terms of clarity and efficiency.
92-92: Excellent consistency! The code change is approved.Updating the
timestampproperty to call thegetTimeStampfunction aligns with the changes made to thegetTimeStampfunction itself. This ensures that timestamps are generated consistently across the logging functions, centralizing the logic for timestamp creation. Well done on maintaining consistency and readability!app/client/src/sagas/ActionExecution/errorUtils.ts (2)
8-9: Good job adding the necessary imports!The new imports
ActionTriggerKeysandgetActionTriggerFunctionNamesare required for the changes introduced later in the file. Well done!
84-86: Excellent addition of theshowDebuggerOnExecutionErrorfunction!The new function handles the display of the debugger when an error occurs by dispatching the necessary actions. It ensures that the user can access relevant debugging information. Well done!
app/client/cypress/e2e/Regression/ServerSide/OnLoadTests/JSOnLoad2_Spec.ts (1)
94-94: The code change looks good! 👍The method call to open the debugger has been updated from
debuggerHelper.ClickDebuggerIcon()todebuggerHelper.OpenDebugger(). This change suggests an improvement in the functionality of accessing the debugger.The subsequent lines of the test case remain unchanged, indicating that the overall logic and flow of the test are preserved.
app/client/src/sagas/PostEvaluationSagas.ts (1)
60-79: Great work on enhancing the error handling logic! 🎉The changes to the
showExecutionErrorsfunction (previouslydynamicTriggerErrorHandler) provide a more nuanced approach to error handling based on the application mode. In edit mode, the function now adds the error messages to the logs tab without displaying toast notifications, allowing for focused debugging during development. In view mode, the function utilizesshowToastOnExecutionErrorto display error messages, enhancing user feedback.These modifications align with the AI-generated summary and address the TODO comment about adding errors to the debugger in edit mode.
| export function* showToastOnExecutionError( | ||
| errorMessage: string, | ||
| isExecuteJSFunc = true, | ||
| showCTA = true, | ||
| ) { | ||
| //Commenting as per decision taken for the error hanlding epic to not show the trigger errors in the debugger. | ||
| // if (triggerPropertyName) { | ||
| // AppsmithConsole.addErrors([ | ||
| // { | ||
| // payload: { | ||
| // id: `${source?.id}-${triggerPropertyName}`, | ||
| // logType: LOG_TYPE.TRIGGER_EVAL_ERROR, | ||
| // text: createMessage(DEBUGGER_TRIGGER_ERROR, triggerPropertyName), | ||
| // source: { | ||
| // type: ENTITY_TYPE.WIDGET, | ||
| // id: source?.id ?? "", | ||
| // name: source?.name ?? "", | ||
| // propertyPath: triggerPropertyName, | ||
| // }, | ||
| // messages: [ | ||
| // { | ||
| // type: errorType, | ||
| // message: { name: "TriggerExecutionError", message: errorMessage }, | ||
| // }, | ||
| // ], | ||
| // }, | ||
| // }, | ||
| // ]); | ||
| // } | ||
|
|
||
| function onDebugClick() { | ||
| const appMode = getAppMode(store.getState()); | ||
| if (appMode === "PUBLISHED") return null; | ||
|
|
||
| AnalyticsUtil.logEvent("OPEN_DEBUGGER", { | ||
| source: "TOAST", | ||
| }); | ||
| store.dispatch(showDebugger(true)); | ||
| store.dispatch(setDebuggerSelectedTab(DEBUGGER_TAB_KEYS.ERROR_TAB)); | ||
| store.dispatch(setDebuggerSelectedTab(DEBUGGER_TAB_KEYS.LOGS_TAB)); | ||
| } | ||
|
|
||
| if (isExecuteJSFunc) | ||
| // This is the toast that is rendered when any unhandled error occurs in JS object. | ||
| yield call(showToast, errorMessage, { | ||
| kind: "error", | ||
| action: { | ||
| const action = showCTA | ||
| ? { | ||
| text: "debug", | ||
| effect: () => onDebugClick(), | ||
| className: "t--toast-debug-button", | ||
| }, | ||
| }); | ||
| } | ||
| : undefined; | ||
|
|
||
| // This is the toast that is rendered when any unhandled error occurs in JS object. | ||
| yield call(showToast, errorMessage, { | ||
| kind: "error", | ||
| action, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Nicely done refactoring the error handling logic!
The new function showToastOnExecutionError simplifies the error reporting process by removing unnecessary conditional logic. Great work!
Here are a couple of suggestions to further improve the code:
- Destructure the
showCTAparameter from the function arguments for better readability:
-export function* showToastOnExecutionError(
- errorMessage: string,
- showCTA = true,
-) {
+export function* showToastOnExecutionError(errorMessage: string, { showCTA = true } = {}) {- Define the
actionvariable using a ternary operator for conciseness:
- const action = showCTA
- ? {
- text: "debug",
- effect: () => onDebugClick(),
- className: "t--toast-debug-button",
- }
- : undefined;
+ const action = showCTA ? { text: "debug", effect: () => onDebugClick(), className: "t--toast-debug-button" } : undefined;Keep up the great work!
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 function* showToastOnExecutionError( | |
| errorMessage: string, | |
| isExecuteJSFunc = true, | |
| showCTA = true, | |
| ) { | |
| //Commenting as per decision taken for the error hanlding epic to not show the trigger errors in the debugger. | |
| // if (triggerPropertyName) { | |
| // AppsmithConsole.addErrors([ | |
| // { | |
| // payload: { | |
| // id: `${source?.id}-${triggerPropertyName}`, | |
| // logType: LOG_TYPE.TRIGGER_EVAL_ERROR, | |
| // text: createMessage(DEBUGGER_TRIGGER_ERROR, triggerPropertyName), | |
| // source: { | |
| // type: ENTITY_TYPE.WIDGET, | |
| // id: source?.id ?? "", | |
| // name: source?.name ?? "", | |
| // propertyPath: triggerPropertyName, | |
| // }, | |
| // messages: [ | |
| // { | |
| // type: errorType, | |
| // message: { name: "TriggerExecutionError", message: errorMessage }, | |
| // }, | |
| // ], | |
| // }, | |
| // }, | |
| // ]); | |
| // } | |
| function onDebugClick() { | |
| const appMode = getAppMode(store.getState()); | |
| if (appMode === "PUBLISHED") return null; | |
| AnalyticsUtil.logEvent("OPEN_DEBUGGER", { | |
| source: "TOAST", | |
| }); | |
| store.dispatch(showDebugger(true)); | |
| store.dispatch(setDebuggerSelectedTab(DEBUGGER_TAB_KEYS.ERROR_TAB)); | |
| store.dispatch(setDebuggerSelectedTab(DEBUGGER_TAB_KEYS.LOGS_TAB)); | |
| } | |
| if (isExecuteJSFunc) | |
| // This is the toast that is rendered when any unhandled error occurs in JS object. | |
| yield call(showToast, errorMessage, { | |
| kind: "error", | |
| action: { | |
| const action = showCTA | |
| ? { | |
| text: "debug", | |
| effect: () => onDebugClick(), | |
| className: "t--toast-debug-button", | |
| }, | |
| }); | |
| } | |
| : undefined; | |
| // This is the toast that is rendered when any unhandled error occurs in JS object. | |
| yield call(showToast, errorMessage, { | |
| kind: "error", | |
| action, | |
| }); | |
| } | |
| export function* showToastOnExecutionError(errorMessage: string, { showCTA = true } = {}) { | |
| function onDebugClick() { | |
| AnalyticsUtil.logEvent("OPEN_DEBUGGER", { | |
| source: "TOAST", | |
| }); | |
| store.dispatch(showDebugger(true)); | |
| store.dispatch(setDebuggerSelectedTab(DEBUGGER_TAB_KEYS.LOGS_TAB)); | |
| } | |
| const action = showCTA ? { text: "debug", effect: () => onDebugClick(), className: "t--toast-debug-button" } : undefined; | |
| // This is the toast that is rendered when any unhandled error occurs in JS object. | |
| yield call(showToast, errorMessage, { | |
| kind: "error", | |
| action, | |
| }); | |
| } |
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/sagas/ActionExecution/errorUtils.ts (1)
57-82: Great work on the newshowToastOnExecutionErrorfunction!The function is well-structured and follows the Single Responsibility Principle by focusing solely on displaying the toast notification. The optional "debug" action button enhances the user experience by providing a convenient way to open the debugger and view the error details.
Here's a suggestion to improve readability:
Consider destructuring the
showCTAparameter from the function arguments:-export function* showToastOnExecutionError( - errorMessage: string, - showCTA = true, -) { +export function* showToastOnExecutionError(errorMessage: string, { showCTA = true } = {}) {This change makes it clear that
showCTAis an optional parameter with a default value oftrue.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/ActionExecution/errorUtils.ts (2 hunks)
Additional comments not posted (2)
app/client/src/sagas/ActionExecution/errorUtils.ts (2)
8-9: The new imports look good!The added imports
ActionTriggerKeysandgetActionTriggerFunctionNamesfromee/workers/Evaluation/fnsare necessary for the changes made in this file.
84-86: The newshowDebuggerOnExecutionErrorfunction looks great!The function is concise and follows the Single Responsibility Principle by focusing solely on managing the debugger visibility. It ensures that the user can access relevant debugging information when an error occurs.
|
@hetunandu Can you check the failing cypress tests? |
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/src/ce/constants/ReduxActionConstants.tsx (3 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (5 hunks)
- app/client/src/sagas/ErrorSagas.tsx (4 hunks)
- app/client/src/sagas/JSLibrarySaga.ts (4 hunks)
- app/client/src/sagas/PartialImportExportSagas/PartialExportSagas.ts (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/client/src/sagas/DatasourcesSagas.ts
- app/client/src/sagas/JSLibrarySaga.ts
- app/client/src/sagas/PartialImportExportSagas/PartialExportSagas.ts
Additional comments not posted (7)
app/client/src/sagas/ErrorSagas.tsx (5)
204-206: Great job improving the clarity of the error logging destinations! 👍The changes to the
ErrorEffectTypesenum provide more specificity about where errors are being logged. RenamingLOG_ERRORtoLOG_TO_CONSOLEand adding the newLOG_TO_DEBUGGERvalue enhances the readability and maintainability of the code.
214-215: Excellent additions to theErrorActionPayloadinterface! 🎉The new optional properties
logToDebuggerandsourceEntityenhance the error handling capabilities by allowing errors to be logged to the debugger and specifying the source entity. These additions provide more flexibility and context when handling errors.
Line range hint
219-255: Fantastic updates to theerrorSagafunction! 🚀The changes made to the
errorSagafunction seamlessly integrate with the updates to theErrorEffectTypesenum andErrorActionPayloadinterface. The function now handles logging errors to the debugger based on thelogToDebuggerflag and includes thesourceEntityin theAppsmithConsole.errorcall. This enhances the error handling capabilities and provides more flexibility in logging errors.Additionally, the consideration of the
appModewhen determining whether to show an alert is a nice touch. It ensures that alerts are shown appropriately based on the application mode.Overall, these changes improve the error handling logic and make it more robust and configurable. Well done! 👏
Tools
Biome
[error] 234-234: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
296-309: Skipped reviewing unchanged code.The
embedRedirectURLfunction remains unchanged in the provided code changes. No action required.
Line range hint
260-268: Verify the impact of the changes on Cypress tests.The additional condition and logic added in the
SHOW_ALERTcase specifically handle empty or null error messages during Cypress testing. If the condition is met, the application is safely crashed with theCYPRESS_DEBUGerror code.While this can be useful for debugging purposes during Cypress tests, it's important to ensure that this behavior does not adversely affect the existing Cypress tests.
To verify the impact on Cypress tests, please run the following script:
app/client/src/ce/constants/ReduxActionConstants.tsx (2)
381-381: Good job moving the error constant to the correct location! 👍I see that you've moved the
GET_OAUTH_ACCESS_TOKEN_ERRORconstant fromUserAuthActionTypestoUserAuthActionErrorTypes. This is the right approach, as it correctly categorizes the constant as an error type. Nice work cleaning up the constants!
1337-1349: Excellent addition of error types for toast messages! 🙌I noticed that you've added several error types to the
toastMessageErrorTypesobject. This is a great way to ensure that these specific errors are displayed as toast messages to the user. By importing the error types from the respective action error type objects, you're keeping things organized and maintainable. Fantastic work!
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/ErrorSagas.tsx (4 hunks)
Additional context used
Biome
app/client/src/sagas/ErrorSagas.tsx
[error] 226-226: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Additional comments not posted (4)
app/client/src/sagas/ErrorSagas.tsx (4)
204-204: Great job improving the clarity of the enum values! 👍Renaming
LOG_ERRORtoLOG_TO_CONSOLEmakes the purpose of the enum value more evident. Adding theLOG_TO_DEBUGGERvalue enables a new logging mechanism, which can be helpful for debugging.The changes are approved.
Also applies to: 206-206
214-215: Nice work extending theErrorActionPayloadinterface! 🙌Adding the
logToDebuggerproperty enables control over logging errors to the debugger, which aligns with the newLOG_TO_DEBUGGERenum value. ThesourceEntityproperty is a great addition for providing more context about the error source.The changes are approved.
219-231: Excellent work updating theerrorSagafunction! 🎉The changes you made bring several improvements:
- Aligning the
effectsarray with the updatedErrorEffectTypesenum ensures consistency.- Including the
logToDebuggerandsourceEntityproperties in the payload destructuring allows for more granular control over error logging.- Adding the debugger logging mechanism using
AppsmithConsole.errorenhances the debugging capabilities.- Selecting the
appModefrom the Redux store and checking the conditions for showing error toasts based onshouldShowToastandappModehelps determine the appropriate behavior based on the application state.The changes are approved. Great job on these enhancements! 👏
Also applies to: 233-260
Tools
Biome
[error] 226-226: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
226-226: Skipping the suggestion to remove redundant double-negation.While the static analysis tool suggests removing the redundant double-negation, it can be used intentionally to improve code readability and explicitly convey the expected boolean value. In this case, using
!!showclearly indicates thatshowis expected to be a boolean.Feel free to keep the double-negation as is.
Tools
Biome
[error] 226-226: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/sagas/ErrorSagas.tsx (1)
Line range hint
219-266: The changes to theerrorSagafunction look great! They introduce useful new functionality and improve error handling.A few highlights:
- Logging errors to the console by default is a good practice.
- The
logToDebuggerflag andLOG_TO_DEBUGGEReffect provide flexibility in error logging.- Considering the application mode for showing error toasts helps avoid disruptions in edit mode.
One small suggestion:
Consider adding a comment to explain the purpose of the
appModeconstant and how it's used in the error toast logic. Something like:// Get the current application mode (e.g., EDIT, PUBLISHED) const appMode: APP_MODE = yield select(getAppMode);Tools
Biome
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/ErrorSagas.tsx (4 hunks)
Additional comments not posted (3)
app/client/src/sagas/ErrorSagas.tsx (3)
204-206: Great job improving the clarity of the enum values! The changes look good to me.The renaming of
LOG_ERRORtoLOG_TO_CONSOLEand the addition ofLOG_TO_DEBUGGERmake the intent of these values clearer.
214-215: The new properties in theErrorActionPayloadinterface look good!The
logToDebuggerandsourceEntityproperties will provide more flexibility and context when handling errors.
Line range hint
271-280: The additional error handling logic for Cypress testing looks good!Checking for empty or null error messages and dispatching a specific action will help with debugging and error reporting during Cypress tests.
Description
By removing the current default behaviour of toast error messages we are going to bid adieu to the wall of toast message errors that happen when a user incurs any errors during loading of apps or pages.
Exceptions that will still show toasts:
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/4942
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10696711733
Commit: f90c54c
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Git/GitImport/GitImport_spec.js
- cypress/e2e/Regression/ClientSide/Git/GitWithJSLibrary/GitwithCustomJSLibrary_spec.js
List of identified flaky tests.Wed, 04 Sep 2024 08:33:35 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor