fix: doc prop issues #1513
fix: doc prop issues #1513
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
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/documentationPropagation/DocumentationPropagation.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. 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
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.
❌ Changes requested. Reviewed everything up to d8ce150 in 54 seconds
More details
- Looked at
221lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. webview_panels/src/modules/documentationEditor/components/documentationPropagation/styles.module.scss:11
- Draft comment:
The use of:nth-childhere is incorrect. It targets the first and second child of the parent element, not the intended elements within the class. Consider using a more specific selector or restructuring the HTML to apply styles correctly. - Reason this comment was not posted:
Comment was on unchanged code.
2. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:79
- Draft comment:
Please specify a return type for theloadMoreDownstreamModelsfunction to improve code readability and maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
3. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:173
- Draft comment:
Please specify a return type for thesetAllColumnsValuefunction to improve code readability and maintainability. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rrlOcOIiZfPR6Oks
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| > | ||
| Propagate documentation to selected models | ||
| </Button> | ||
| {isSaved && <div>Saved documentation successfully</div>} |
There was a problem hiding this comment.
Consider resetting isSaved to false after displaying the success message to ensure correct UI behavior if the component is reused.
![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 (7)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/styles.module.scss (1)
21-23: Consider reducing style duplication between.itemRowand.colDesc.Both classes share identical styling for their children. Consider extracting these common styles into a shared mixin or class.
Here's a suggested refactor:
+@mixin itemChildren { + :nth-child(1) { + font-weight: 600; + } + + :nth-child(2) { + line-break: anywhere; + color: var(--text-grey2-color); + } +} .itemRow { display: grid; grid-template-columns: 1fr 3fr; - :nth-child(1) { - font-weight: 600; - } - - :nth-child(2) { - line-break: anywhere; - color: var(--text-grey2-color); - } + @include itemChildren; } .colDesc { - :nth-child(1) { - font-weight: 600; - } - - :nth-child(2) { - line-break: anywhere; - color: var(--text-grey2-color); - } + @include itemChildren; }webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (6)
Line range hint
71-77: Consider splitting the useEffect for better separation of concernsThe current useEffect handles multiple state resets. Consider separating the
isSavedreset into its own effect for better maintainability and clearer intent.useEffect(() => { setAllColumns([]); setCurrColumns(startColumn); setTableMetadata([]); - setIsSaved(false); }, [currentDocsData?.uniqueId, name]); + +useEffect(() => { + setIsSaved(false); +}, [currentDocsData?.uniqueId, name]);
189-191: Consider adding user feedback for missing descriptionWhile the null check prevents propagation of empty descriptions, consider providing feedback to help users understand why the propagation button isn't visible.
if (!currColumnDescription) { - return null; + return ( + <div className="text-muted"> + Add a description to enable documentation propagation + </div> + ); }
217-222: Consider improving the empty state UIThe "no downstream lineage" message could be enhanced with visual elements and actionable guidance.
-<div className="mt-4"> - No downstream column level lineage detected to propagate the - documentation -</div> +<Stack direction="column" className="mt-4 text-center"> + <div className="mb-2"> + <EmptyStateIcon size="lg" /> + </div> + <div className="font-medium"> + No Downstream Lineage Detected + </div> + <div className="text-muted"> + This column has no downstream dependencies to propagate documentation to. + </div> +</Stack>
284-284: Enhance the success message feedbackThe success message could be improved with styling and potentially an auto-dismiss timer.
-{isSaved && <div>Saved documentation successfully</div>} +{isSaved && ( + <div className={styles.successMessage}> + <CheckIcon className="mr-2" /> + Documentation propagated successfully + </div> +)}Consider adding this CSS to your styles module:
.successMessage { display: flex; align-items: center; padding: 8px 16px; background-color: var(--success-bg); color: var(--success-text); border-radius: 4px; margin-top: 8px; }
265-273: Improve loading state handlingThe "Load 3 more downstream levels" button's disabled state could be enhanced with a loading indicator.
<Button color="primary" outline onClick={loadMoreDownstreamModels} disabled={isLoading} > - Load 3 more downstream levels + {isLoading ? ( + <> + <Spinner size="sm" className="mr-2" /> + Loading... + </> + ) : ( + 'Load 3 more downstream levels' + )} </Button>
Line range hint
144-169: Consider adding input validation for documentation propagationThe
propagateDocumentationfunction should validate the documentation content before propagation to prevent potential XSS or injection attacks.const propagateDocumentation = async () => { + // Validate documentation content + const sanitizedDescription = sanitizeHtml(currColumnDescription, { + allowedTags: ['b', 'i', 'em', 'strong', 'a'], + allowedAttributes: { + 'a': ['href'] + } + }); + const defaultPackageName = tableMetadata.filter((t) => t.packageName)[0] ?.packageName; const defaultPatchPath = defaultPackageName ? defaultPackageName + "://models/schema.yml" : ""; const req = []; // ... rest of the function columns: [{ name: item.column, - description: currColumnDescription + description: sanitizedDescription }],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx(5 hunks)webview_panels/src/modules/documentationEditor/components/documentationPropagation/styles.module.scss(1 hunks)
🔇 Additional comments (2)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/styles.module.scss (1)
12-19: LGTM! Enhanced visual hierarchy with stronger font weights.
The increased font weight and text styling improvements help distinguish between different levels of content.
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (1)
63-63: LGTM: Good addition of save status tracking
The addition of the isSaved state variable improves user feedback by tracking successful documentation propagation.
Overview
Problem
Describe the problem you are solving. Mention the ticket/issue if applicable.
Solution
Describe the implemented solution. Add external references if needed.
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my change