fix: show error on failure while getting distinct column values #1524
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
webview_panels/src/modules/documentationEditor/components/tests/forms/AcceptedValues.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "/webview_panels".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in "webview_panels/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces enhancements to error handling and logging across multiple components. In the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant DBTProject
participant Database
Caller->>DBTProject: getBulkSchemaFromDB(nodes, token)
DBTProject->>Database: Fetch schema
alt Successful retrieval
Database-->>DBTProject: Return schema
DBTProject-->>Caller: Return schema
else Error occurs
Database-->>DBTProject: Throw error
DBTProject->>DBTProject: Log error
DBTProject-->>Caller: Rethrow error
end
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to ee2c6cd in 1 minute and 37 seconds
More details
- Looked at
89lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/webview_provider/altimateWebviewProvider.ts:136
- Draft comment:
Consider using thestatusfield consistently in allsendResponseToWebviewcalls to indicate success or failure. This will improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a newstatusfield in thesendResponseToWebviewmethod, which is a good addition for indicating success or failure. However, thestatusfield is not used consistently in thesendResponseToWebviewmethod calls. In some places, it is set explicitly, while in others, it is not set at all. This inconsistency can lead to confusion and potential bugs.
2. src/manifest/dbtProject.ts:830
- Draft comment:
Ensure that the calling function handles the thrown error appropriately to avoid unhandled exceptions. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds athrowstatement in thecatchblock of thegetColumnValuesmethod indbtProject.ts. This is a good practice to propagate errors, but it should be ensured that the calling function handles this error appropriately to avoid unhandled exceptions.
3. webview_panels/src/modules/documentationEditor/components/tests/forms/AcceptedValues.tsx:26
- Draft comment:
Please specify a return type for thegetDistinctColumnValuesfunction to improve code readability and maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_mX37dGlrWhE8wn7o
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![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
🧹 Nitpick comments (1)
src/webview_provider/altimateWebviewProvider.ts (1)
158-158: Revisit unifying response structure.While returning different “status” values for success and failure is good, consider unifying or centralizing your response creation logic (e.g., via a helper function). This would keep the response payload structure consistent, minimize duplication, and reduce the risk of errors or inconsistent handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/manifest/dbtProject.ts(1 hunks)src/webview_provider/altimateWebviewProvider.ts(2 hunks)webview_panels/src/modules/documentationEditor/components/tests/forms/AcceptedValues.tsx(2 hunks)
🔇 Additional comments (8)
src/webview_provider/altimateWebviewProvider.ts (1)
138-138: Include success/failure indicators in API docs.
Great addition of the “status” property to indicate success. Ensure your API documentation and any relevant code comments are updated to reflect this new behavior so that users and maintainers clearly understand how to handle the “status” indicator in responses.
src/manifest/dbtProject.ts (1)
830-830: Re-throwing errors is beneficial for upstream handling.
Re-throwing the error after logging is a good practice to ensure calling functions can handle or display the error as needed. Confirm that all callers of getColumnValues either handle or bubble up this exception so errors are never silently dropped.
webview_panels/src/modules/documentationEditor/components/tests/forms/AcceptedValues.tsx (6)
7-7: Logging initialization.
Good addition of the panelLogger import. This helps maintain a consistent logging standard across the codebase.
28-32: Loading state initialization is clear.
Setting isLoading before starting the operation is excellent. This ensures the user sees immediate feedback.
34-42: Check user intent before overwriting values.
This code properly prompts the user to confirm overwriting existing values. Good user-centric approach. Ensure that any other overwriting logic in the app follows a similar pattern for consistency.
46-47: Proper error logging.
Catching the error and logging it with panelLogger.error ensures good visibility into failures. Confirm that the log messages contain meaningful contextual data (e.g., model name, column name) to facilitate easier debugging.
48-48: Ensure loading state is reset.
Using a finally block to reset “isLoading” is excellent. This prevents UI deadlocks when errors occur and is a best practice for promise-based code.
44-45: Set new distinct values on success.
The assignment to “accepted_values” is straightforward. Verify all downstream logic that consumes “accepted_values” is ready to handle these newly fetched values (e.g., sorting, uniqueness, or constraints).
✅ Verification successful
Let me analyze the code references to verify the handling of "accepted_values". Let me gather more specific information about the validation and form handling.
Let me try one more search with modified patterns to check the form validation and handling.
Based on the gathered information, I can now provide a final response:
The accepted_values handling is properly implemented with necessary validations.
The codebase shows proper handling of accepted_values through:
- Schema validation using Yup array validation
- Form submission is disabled when accepted_values array is empty
- Values are properly trimmed during save operation
- Proper type definitions and interfaces for accepted_values
- Consistent handling in both test form and display components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching the codebase for references to accepted_values usage and verifying relevant handling
rg -A 5 'accepted_values'
Length of output: 12188
Script:
#!/bin/bash
# Check form validation and handling
ast-grep --pattern 'const disableFormSubmit = useMemo(() => {
$$$
if (formType === DbtGenericTests.ACCEPTED_VALUES) {
$$$
}
$$$
}'
Length of output: 144
Script:
#!/bin/bash
# Check the schema validation
ast-grep --pattern 'accepted_values: Yup.array().of(Yup.string().required())'
Length of output: 79
Script:
#!/bin/bash
# Check the test form save handling
ast-grep --pattern 'const newValues = data.accepted_values?.map((s) => s.trim());'
Length of output: 243
Script:
#!/bin/bash
# Check form validation with a simpler pattern
ast-grep --pattern 'if (formType === DbtGenericTests.ACCEPTED_VALUES) {
$$$
}'
# Check the complete form handling in TestForm.tsx
rg -A 10 'const TestForm.*Props.*=.*=>' webview_panels/src/modules/documentationEditor/components/tests/forms/TestForm.tsx
Length of output: 938
Overview
Problem
When getting distinct column values for adding accepted_values test, if there is an error, it is not shown to user.
Solution
Show error message on failure
Screenshot/Demo
How to test
Checklist
README.mdupdated and added information about my change