feat: documentation lineage propagation #1498
Conversation
WalkthroughThis pull request introduces several significant changes across multiple files, primarily focusing on enhancing data lineage functionality. Key updates include the addition of a new Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
Hey @AdiGajbhiye, here is an example of how you can ask me to improve this pull request: @Sweep Add unit tests for the `LineageService.getConnectedTables()` method to verify: 📖 For more information on how to use Sweep, please read our documentation. |
![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 470a5f0 in 1 minute and 39 seconds
More details
- Looked at
1708lines of code in9files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/services/dbtLineageService.ts:12
- Draft comment:
Avoid using abbreviations like 'env'. Use 'environment variables' for clarity. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ObVZ9kZhp8hJOifz
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.
| import { | ||
| CancellationToken, | ||
| CancellationTokenSource, | ||
| env, |
There was a problem hiding this comment.
Avoid using abbreviations like 'env'. Use 'environment variables' for clarity.
| env, | |
| environmentVariables, |
| @@ -913,6 +985,60 @@ export class DocsEditViewPanel implements WebviewViewProvider { | |||
| ); | |||
| } | |||
|
|
|||
| private async handleSyncRequestFromWebview( | |||
There was a problem hiding this comment.
This function duplicates existing functionality in altimateWebviewProvider.ts. Consider refactoring to use the existing function.
- function
handleSyncRequestFromWebview(altimateWebviewProvider.ts)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
webview_panels/src/modules/documentationEditor/components/docGenerator/DocGeneratorInput.tsx (1)
148-148: Consider enhancing error handling and user feedbackWhile the DocumentationPropagationButton integration looks good, consider these improvements:
- Add error boundaries to handle potential failures gracefully
- Include loading states for better user feedback
- Consider adding user confirmation before propagation to prevent accidental updates
Example implementation:
<ErrorBoundary fallback={<ErrorMessage />}> <DocumentationPropagationButton type={type} name={entity.name} onBeforePropagation={() => confirm('Are you sure you want to propagate this documentation?')} onError={(error) => handleError(error)} /> </ErrorBoundary>src/services/dbtTestService.ts (2)
259-267: Add error handling and documentation forgetTestsForCurrentModel.The method could be improved in several ways:
- Add JSDoc documentation explaining the method's purpose and return type.
- Add error handling for invalid file paths.
- Consider supporting other dbt model file extensions besides .sql.
Here's a suggested improvement:
+ /** + * Retrieves tests for the currently active model in the editor. + * @returns {Promise<TestMetaData[] | undefined>} Array of test metadata for the current model, or undefined if no model is found. + * @throws {Error} If the current file is not a valid dbt model. + */ public async getTestsForCurrentModel() { const eventResult = this.queryManifestService.getEventByCurrentProject(); if (!eventResult?.event || !eventResult?.currentDocument) { return undefined; } const project = this.queryManifestService.getProject(); if (!project) { return undefined; } const { currentDocument } = eventResult; + const supportedExtensions = ['.sql', '.py', '.yml']; // Add other supported extensions + const fileExt = path.extname(currentDocument.uri.fsPath); + if (!supportedExtensions.includes(fileExt)) { + throw new Error(`Invalid dbt model file type: ${fileExt}`); + } const modelName = path.basename(currentDocument.uri.fsPath, fileExt); return this.getTestsForModel(modelName); }
Line range hint
284-324: Improve readability and type safety of test metadata processing.The implementation could be enhanced with better type safety and readability:
- Extract the "tests" constant to avoid magic strings.
- Add explicit type for the filter callback.
- Break down the complex map-filter chain for better readability.
Here's a suggested improvement:
+ const GRAPH_META_TESTS_KEY = 'tests'; + type TestMetaDataWithKey = TestMetaData & { key: string }; + this.dbtTerminal.debug( "dbtTests", "getting tests by modelName:", false, modelName, ); const _node = nodeMetaMap.lookupByBaseName(modelName); if (!_node) { this.dbtTerminal.debug("no node for tableName:", modelName); return; } const key = _node.uniqueId; - return (graphMetaMap["tests"].get(key)?.nodes || []) + return (graphMetaMap[GRAPH_META_TESTS_KEY].get(key)?.nodes || []) .map((n) => { const testKey = n.label.split(".")[0]; const testData = testMetaMap.get(testKey); if (!testData) { return null; } // For singular tests, attached_node will be undefined if (!testData.attached_node) { return { ...testData, key: testKey }; } // dbt sends tests (ex: relationships) to both source and connected models // do not send the test which has different model in attached_node if (testData.attached_node !== key) { return null; } const { depends_on: { macros }, test_metadata, } = testData; const macroFilepath = this.getMacroFilePath( macros, projectName, macroMetaMap, test_metadata?.name, ); return { ...testData, path: macroFilepath || testData.path, key: testKey, }; - }) - .filter((t) => Boolean(t)); + }) + .filter((t): t is TestMetaDataWithKey => Boolean(t));src/altimate.ts (1)
Line range hint
743-748: Consider adding input validation for event_type.The
getColumnLevelLineagemethod accepts the request without validating theevent_type. Consider adding validation or using an enum to restrict the possible values.+export enum DBTColumnLineageEventType { + DOCUMENTATION = "documentation", + // Add other valid event types +} export interface DBTColumnLineageRequest { model_dialect: string; targets: { uniqueId: string; column_name: string }[]; model_info: ModelInfo[]; upstream_expansion: boolean; upstream_models: string[]; selected_column: { model_node?: ModelNode; column: string }; session_id: string; show_indirect_edges: boolean; - event_type: string; + event_type: DBTColumnLineageEventType; }src/webview_provider/newLineagePanel.ts (2)
Line range hint
118-122: Add Error Handling for 'getUpstreamTables' MethodThe call to
this.dbtLineageService.getUpstreamTables(params)lacks error handling. If an exception occurs within this method, it could lead to unhandled exceptions and potentially crash the extension. Consider wrapping this call in atry-catchblock to handle possible errors gracefully.Apply this diff to add error handling:
+ try { const body = this.dbtLineageService.getUpstreamTables(params); this._panel?.webview.postMessage({ command: "response", args: { id, syncRequestId, body, status: true }, }); + } catch (error) { + window.showErrorMessage( + extendErrorWithSupportLinks( + "Unable to get upstream tables: " + (error as Error).message, + ), + ); + this._panel?.webview.postMessage({ + command: "response", + args: { id, syncRequestId, error, status: false }, + }); + this.telemetry.sendTelemetryError("getUpstreamTablesError", { + params, + }); + }
Line range hint
127-131: Add Error Handling for 'getDownstreamTables' MethodSimilar to the previous issue, the call to
this.dbtLineageService.getDownstreamTables(params)does not include error handling. If an exception occurs, it could result in unhandled exceptions and potentially crash the extension. It's advisable to wrap this call in atry-catchblock to manage errors effectively.Apply this diff to add error handling:
+ try { const body = this.dbtLineageService.getDownstreamTables(params); this._panel?.webview.postMessage({ command: "response", args: { id, syncRequestId, body, status: true }, }); + } catch (error) { + window.showErrorMessage( + extendErrorWithSupportLinks( + "Unable to get downstream tables: " + (error as Error).message, + ), + ); + this._panel?.webview.postMessage({ + command: "response", + args: { id, syncRequestId, error, status: false }, + }); + this.telemetry.sendTelemetryError("getDownstreamTablesError", { + params, + }); + }src/services/dbtLineageService.ts (1)
483-485: Enhance error handling for undefined error messagesThe error handling assumes that
erroris an instance ofErrorwith amessageproperty. Iferroris of a different type, this could result in an undefined message being displayed. Consider checking iferrorhas amessageproperty or using a default message.Apply this diff to improve error handling:
window.showErrorMessage( extendErrorWithSupportLinks( - "Could not generate column level lineage: " + - (error as Error).message, + "Could not generate column level lineage: " + + (error instanceof Error && error.message ? error.message : String(error)), ), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
webview_panels/src/assets/icons/propagate.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
src/altimate.ts(1 hunks)src/services/dbtLineageService.ts(1 hunks)src/services/dbtTestService.ts(2 hunks)src/webview_provider/docsEditPanel.ts(7 hunks)src/webview_provider/newLineagePanel.ts(8 hunks)webview_panels/src/assets/icons/index.tsx(1 hunks)webview_panels/src/modules/documentationEditor/components/docGenerator/DocGeneratorInput.tsx(5 hunks)webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx(1 hunks)webview_panels/src/modules/documentationEditor/components/documentationPropagation/styles.module.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- webview_panels/src/modules/documentationEditor/components/documentationPropagation/styles.module.scss
🧰 Additional context used
🪛 Biome
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx
[error] 171-171: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (9)
webview_panels/src/modules/documentationEditor/components/docGenerator/DocGeneratorInput.tsx (3)
26-26: LGTM: Clean import addition
The import follows React conventions and aligns with the feature's purpose.
126-126: Minor formatting changes
These changes only affect the closing parentheses alignment.
Also applies to: 136-136
26-26: Verify documentation propagation integration
Let's verify the complete integration of the documentation propagation feature.
Also applies to: 148-148
✅ Verification successful
Based on the shell script output, I can see the complete implementation of the documentation propagation feature. Let me analyze the key findings:
-
The
DocumentationPropagationButtoncomponent is properly integrated:- It's imported and used in
DocGeneratorInput.tsx - Has proper event handling with
propagateDocumentationfunction - Uses proper state management through props (
typeandname) - Has proper UI elements with button and icon
- It's imported and used in
-
The propagation event handling is properly implemented:
- Uses proper event handling with
onClick - Has proper event emission to backend via
documentation_propagationevent type
- Uses proper event handling with
-
The component is properly typed and follows the codebase patterns:
- Uses TypeScript
- Follows the component structure pattern
- Has proper prop types
Documentation propagation integration is properly implemented
The feature is correctly integrated with proper event handling, state management and type definitions. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper integration of documentation propagation feature
# Test 1: Check for proper type definitions
echo "Checking for DocumentationPropagationButton type definitions..."
rg -t typescript "interface.*DocumentationPropagationButton"
# Test 2: Verify state management integration
echo "Checking for documentation propagation state management..."
rg -t typescript "useDocumentationContext.*propagation"
# Test 3: Look for event handlers
echo "Checking for propagation event handlers..."
rg -t typescript "onPropagate|handlePropagate"
Length of output: 633
Script:
#!/bin/bash
# Let's try without typescript file type flag and search for the component and its integration
# Check for DocumentationPropagationButton component definition and usage
echo "Checking DocumentationPropagationButton component..."
rg "DocumentationPropagationButton"
# Check for propagation related state and handlers
echo "Checking propagation related code..."
rg "propagation|propagate" -i
# Look for the actual component file
echo "Finding component file..."
fd "DocumentationPropagation"
Length of output: 278290
webview_panels/src/assets/icons/index.tsx (1)
30-30: LGTM! Verify SVG file existence.
The new icon export follows the established pattern and naming convention.
Let's verify that the SVG file exists:
✅ Verification successful
SVG file exists and follows the established pattern ✅
The propagate.svg file is present in the expected location at webview_panels/src/assets/icons/propagate.svg, confirming that the icon export is properly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the propagate.svg file exists in the icons directory
fd --type f "propagate.svg" webview_panels/src/assets/icons/
Length of output: 106
src/altimate.ts (2)
Line range hint 743-748: Implementation looks good!
The integration of the new event_type property is clean and doesn't impact the existing error handling or request processing logic.
66-66: Verify the impact of adding required property.
The addition of event_type as a required property in DBTColumnLineageRequest interface could be a breaking change. Existing code using this interface will need to be updated to provide this property.
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (1)
1-270: Overall implementation is solid and aligns with the requirements
The DocumentationPropagationButton component is well-structured and integrates seamlessly with the existing documentation editor. The use of React hooks (useState, useEffect, useRef) is appropriate, and asynchronous operations are handled correctly with executeRequestInAsync and executeRequestInSync. The component effectively manages state and provides a good user experience by allowing users to propagate documentation to downstream models.
🧰 Tools
🪛 Biome
[error] 171-171: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
src/services/dbtLineageService.ts (1)
466-466: Typo in property name 'confindence'
There's a typo in the return object: confindence should be confidence. This could lead to issues when accessing this property elsewhere in the code.
src/webview_provider/docsEditPanel.ts (1)
1023-1040: Utility method 'sendResponseToWebview' is well-implemented
The sendResponseToWebview method provides a clear and consistent way to send responses back to the webview. The implementation is clean and follows best practices.
| public async getTestsForModel(modelName: string) { | ||
| const eventResult = this.queryManifestService.getEventByCurrentProject(); | ||
| if (!eventResult?.event || !eventResult?.currentDocument) { | ||
| return undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary check and add documentation for getTestsForModel.
The method has a few issues that should be addressed:
- Missing JSDoc documentation.
- Unnecessary check for
currentDocumentsince this method doesn't use it. - Missing return type annotation.
Here's the suggested improvement:
+ /**
+ * Retrieves tests associated with a specific dbt model.
+ * @param modelName - The name of the dbt model without file extension
+ * @returns {Promise<TestMetaData[] | undefined>} Array of test metadata for the model, or undefined if no model is found
+ */
- public async getTestsForModel(modelName: string) {
+ public async getTestsForModel(modelName: string): Promise<TestMetaData[] | undefined> {
const eventResult = this.queryManifestService.getEventByCurrentProject();
- if (!eventResult?.event || !eventResult?.currentDocument) {
+ if (!eventResult?.event) {
return undefined;
}📝 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.
| public async getTestsForModel(modelName: string) { | |
| const eventResult = this.queryManifestService.getEventByCurrentProject(); | |
| if (!eventResult?.event || !eventResult?.currentDocument) { | |
| return undefined; | |
| } | |
| /** | |
| * Retrieves tests associated with a specific dbt model. | |
| * @param modelName - The name of the dbt model without file extension | |
| * @returns {Promise<TestMetaData[] | undefined>} Array of test metadata for the model, or undefined if no model is found | |
| */ | |
| public async getTestsForModel(modelName: string): Promise<TestMetaData[] | undefined> { | |
| const eventResult = this.queryManifestService.getEventByCurrentProject(); | |
| if (!eventResult?.event) { | |
| return undefined; | |
| } | |
| const setAllColumnsValue = (value: boolean) => { | ||
| setSelectedColumns( | ||
| allColumns.reduce( | ||
| (acc, curr) => ({ | ||
| ...acc, | ||
| [curr.model + "/" + curr.column]: value, | ||
| }), | ||
| {}, | ||
| ), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor setAllColumnsValue to improve performance
Using the spread operator (...acc) on accumulators within a reduce function can lead to O(n²) time complexity due to the creation of a new object at each iteration. This can cause performance issues with larger datasets.
Apply the following refactor to enhance performance:
- setSelectedColumns(
- allColumns.reduce(
- (acc, curr) => ({
- ...acc,
- [curr.model + "/" + curr.column]: value,
- }),
- {},
- ),
- );
+ const newSelectedColumns: Record<string, boolean> = {};
+ for (const curr of allColumns) {
+ newSelectedColumns[curr.model + "/" + curr.column] = value;
+ }
+ setSelectedColumns(newSelectedColumns);📝 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 setAllColumnsValue = (value: boolean) => { | |
| setSelectedColumns( | |
| allColumns.reduce( | |
| (acc, curr) => ({ | |
| ...acc, | |
| [curr.model + "/" + curr.column]: value, | |
| }), | |
| {}, | |
| ), | |
| ); | |
| }; | |
| const setAllColumnsValue = (value: boolean) => { | |
| const newSelectedColumns: Record<string, boolean> = {}; | |
| for (const curr of allColumns) { | |
| newSelectedColumns[curr.model + "/" + curr.column] = value; | |
| } | |
| setSelectedColumns(newSelectedColumns); | |
| }; |
🧰 Tools
🪛 Biome
[error] 171-171: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
| const url = window.activeTextEditor!.document.uri.path; | ||
| const node = this.dbtLineageService.createTable(event.event, url, key); |
There was a problem hiding this comment.
Prevent Potential Null Reference Error with 'window.activeTextEditor'
The use of window.activeTextEditor! assumes that there is always an active text editor. However, window.activeTextEditor can be undefined, which may lead to a runtime error if no editor is open. To prevent this, add a null check before accessing window.activeTextEditor.
Apply this diff to include a null check:
+ if (!window.activeTextEditor) {
+ return {
+ aiEnabled,
+ missingLineageMessage: this.getMissingLineageMessage(),
+ };
+ }
const url = window.activeTextEditor.document.uri.path;
const node = this.dbtLineageService.createTable(event.event, url, key);
return { node, aiEnabled };Committable suggestion skipped: line range outside the PR's diff.
| return g.get(key)?.nodes.length || 0; | ||
| } | ||
|
|
||
| private cancellationTokenSource: CancellationTokenSource | undefined; |
There was a problem hiding this comment.
Ensure 'cancellationTokenSource' is initialized before use
In the getConnectedColumns method, this.cancellationTokenSource is asserted as non-null using the ! operator. If cancellationTokenSource is undefined, this will cause a runtime error. Ensure that cancellationTokenSource is properly initialized before using it, or add necessary null checks.
Apply this diff to handle the potential undefined value:
await project.getNodesWithDBColumns(
event,
modelsToFetch,
- this.cancellationTokenSource!.token,
+ this.cancellationTokenSource?.token || new CancellationTokenSource().token,
);Alternatively, ensure that this.cancellationTokenSource is always initialized before calling getConnectedColumns.
Also applies to: 318-318
| if (this.cancellationTokenSource?.token.isCancellationRequested) { | ||
| return { column_lineage: [] }; | ||
| } | ||
| const sessionId = `${env.sessionId}-${selectedColumn.table}-${selectedColumn.name}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize 'sessionId' components to handle special characters
When constructing sessionId with selectedColumn.table and selectedColumn.name, special characters in these values may lead to issues. To prevent potential errors or security vulnerabilities, encode these components.
Apply this diff to encode the variables:
- const sessionId = `${env.sessionId}-${selectedColumn.table}-${selectedColumn.name}`;
+ const sessionId = `${env.sessionId}-${encodeURIComponent(selectedColumn.table)}-${encodeURIComponent(selectedColumn.name)}`;📝 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 sessionId = `${env.sessionId}-${selectedColumn.table}-${selectedColumn.name}`; | |
| const sessionId = `${env.sessionId}-${encodeURIComponent(selectedColumn.table)}-${encodeURIComponent(selectedColumn.name)}`; |
| nodeType: string; | ||
| materialization?: string; | ||
| description?: string; | ||
| tests: any[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a specific type for the 'tests' property
The 'tests' property in the Table type is currently typed as any[]. Using any reduces type safety and can lead to runtime errors. Consider defining a specific interface or type for tests to enhance code reliability.
Apply this diff to define a specific type for tests:
- tests: any[];
+ tests: TestMetadata[];Ensure that TestMetadata is defined appropriately to reflect the structure of test objects.
Committable suggestion skipped: line range outside the PR's diff.
| if (!t[0].startsWith("model")) { | ||
| return; |
There was a problem hiding this comment.
Add null check for 't[0]' before using 'startsWith'
Ensure that t[0] is defined before calling t[0].startsWith("model") to prevent potential runtime errors if t[0] is undefined.
Apply this diff to fix the issue:
- if (!t[0].startsWith("model")) {
+ if (!t[0] || !t[0].startsWith("model")) {📝 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.
| if (!t[0].startsWith("model")) { | |
| return; | |
| if (!t[0] || !t[0].startsWith("model")) { | |
| return; |
| const _tables = targets | ||
| .map( | ||
| (t) => | ||
| this.dbtLineageService.getDownstreamTables({ table: t[0] }) | ||
| ?.tables, | ||
| ) | ||
| .filter((t) => Boolean(t)) | ||
| .flat() as Table[]; | ||
| const tables = _tables.map((t) => t?.table); |
There was a problem hiding this comment.
Ensure asynchronous functions are properly awaited
The method getDownstreamTables might be asynchronous. If it returns a Promise, you need to await its result. Currently, the code does not await the result, which could lead to unexpected behavior or runtime errors.
Apply this diff to fix the issue:
- const _tables = targets
- .map(
- (t) =>
- this.dbtLineageService.getDownstreamTables({ table: t[0] })
- ?.tables,
- )
- .filter((t) => Boolean(t))
- .flat() as Table[];
+ const _tablesNested = await Promise.all(
+ targets.map(async (t) => {
+ const result = await this.dbtLineageService.getDownstreamTables({ table: t[0] });
+ return result?.tables;
+ }),
+ );
+ const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[];📝 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 _tables = targets | |
| .map( | |
| (t) => | |
| this.dbtLineageService.getDownstreamTables({ table: t[0] }) | |
| ?.tables, | |
| ) | |
| .filter((t) => Boolean(t)) | |
| .flat() as Table[]; | |
| const tables = _tables.map((t) => t?.table); | |
| const _tablesNested = await Promise.all( | |
| targets.map(async (t) => { | |
| const result = await this.dbtLineageService.getDownstreamTables({ table: t[0] }); | |
| return result?.tables; | |
| }), | |
| ); | |
| const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[]; | |
| const tables = _tables.map((t) => t?.table); |
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/services/dbtLineageService.ts (3)
73-111: Consider enhancing token disposalWhile the implementation handles cancellation states well, consider adding a try-finally block to ensure token disposal in all cases, including unexpected errors.
async handleColumnLineage( { event }: { event: CllEvents }, onCancel: () => void, ) { + try { if (event === CllEvents.START) { window.withProgress( { title: "Retrieving column level lineage", location: ProgressLocation.Notification, cancellable: true, }, async (_, token) => { await new Promise<void>((resolve) => { this.cancellationTokenSource = new DerivedCancellationTokenSource( token, ); this.cllProgressResolve = resolve; token.onCancellationRequested(() => { onCancel(); }); }); }, ); return; } // ... rest of the implementation + } finally { + if (event !== CllEvents.START) { + this.cancellationTokenSource?.dispose(); + } + } }
262-491: Consider decomposing this large methodThe
getConnectedColumnsmethod is quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused methods:
- Model preparation
- SQL compilation
- API request handling
- Error handling
Example refactor for the model preparation part:
+ private prepareModels( + currAnd1HopTables: string[], + targets: [string, string][], + upstreamExpansion: boolean, + event: ManifestCacheProjectAddedEvent + ) { + const modelInfos: ModelInfo[] = []; + let upstream_models: string[] = []; + let auxiliaryTables: string[] = []; + let sqlTables: string[] = []; + + currAnd1HopTables = Array.from(new Set(currAnd1HopTables)); + const currTables = new Set(targets.map((t) => t[0])); + + if (upstreamExpansion) { + const hop1Tables = currAnd1HopTables.filter((t) => !currTables.has(t)); + upstream_models = [...hop1Tables]; + sqlTables = [...hop1Tables]; + auxiliaryTables = DBTProject.getNonEphemeralParents(event, hop1Tables); + } else { + auxiliaryTables = DBTProject.getNonEphemeralParents( + event, + Array.from(currTables), + ); + sqlTables = Array.from(currTables); + } + + return { + modelInfos, + upstream_models, + auxiliaryTables, + sqlTables, + currAnd1HopTables: Array.from(new Set(currAnd1HopTables)) + }; + }
374-384: Enhance error message with actionable informationThe error message for materialization failure could be more helpful by including steps to resolve the issue.
if (relationsWithoutColumns.length !== 0) { window.showErrorMessage( extendErrorWithSupportLinks( - "Failed to fetch columns for " + - relationsWithoutColumns.join(", ") + - ". Probably the dbt models are not yet materialized.", + `Failed to fetch columns for ${relationsWithoutColumns.join(", ")}. ` + + "These models need to be materialized first. Run 'dbt run' to materialize the models." ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/services/dbtLineageService.ts(1 hunks)
🔇 Additional comments (1)
src/services/dbtLineageService.ts (1)
53-60: Well-implemented token management!
The DerivedCancellationTokenSource class effectively manages linked cancellation tokens, ensuring proper cleanup and cancellation propagation.
| @@ -535,7 +543,7 @@ export class DocsEditViewPanel implements WebviewViewProvider { | |||
| return undefined; | |||
| } | |||
|
|
|||
| const { command, syncRequestId, args } = message; | |||
| const { command, syncRequestId, ...params } = message; | |||
There was a problem hiding this comment.
This might break other commands handled here. Make sure all the commands in this handler are tested from webview panel
| } | ||
| } | ||
|
|
||
| private sendResponseToWebview({ |
There was a problem hiding this comment.
this is also available in altimateWebview provider
| @@ -168,7 +124,7 @@ export class NewLineagePanel | |||
| } | |||
|
|
|||
| if (command === "downstreamTables") { | |||
| const body = await this.getDownstreamTables(params); | |||
| const body = this.dbtLineageService.getDownstreamTables(params); | |||
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 84cb9ef in 51 seconds
More details
- Looked at
498lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/webview_provider/docsEditPanel.ts:744
- Draft comment:
ThesaveDocumentationfunction is duplicated. Consider removing the duplicate to avoid redundancy and potential maintenance issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be incorrect as there is no evidence of duplication in the provided code. The function was refactored into a separate method, which is a common practice to improve code organization and readability.
I might be missing some context from other parts of the codebase that are not included in the diff. However, based on the provided information, there is no duplication.
The task is to review the diff provided, and based on that, there is no duplication of thesaveDocumentationfunction.
The comment about thesaveDocumentationfunction being duplicated is incorrect based on the provided diff. It should be removed.
Workflow ID: wflow_E5k4DkzGROblD48L
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.
| @@ -913,6 +741,288 @@ export class DocsEditViewPanel implements WebviewViewProvider { | |||
| ); | |||
| } | |||
|
|
|||
| private async saveDocumentation(message: any, syncRequestId: string) { | |||
There was a problem hiding this comment.
Please provide a return type for the saveDocumentation function to improve code readability and maintainability.
| private async saveDocumentation(message: any, syncRequestId: string) { | |
| private async saveDocumentation(message: any, syncRequestId: string): Promise<void> { |
| } | ||
| } | ||
|
|
||
| private async handleSyncRequestFromWebview( |
There was a problem hiding this comment.
Please provide a return type for the handleSyncRequestFromWebview function to improve code readability and maintainability.
| private async handleSyncRequestFromWebview( | |
| private async handleSyncRequestFromWebview(syncRequestId: string | undefined, callback: () => any, command: string, showErrorNotification?: boolean): Promise<void> { |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| // check if file exists, if not create an empty file | ||
| if (!existsSync(patchPath)) { | ||
| writeFileSync(patchPath, ""); |
Check failure
Code scanning / CodeQL
Potential file system race condition High
| } | ||
| // Force reload from manifest after manifest refresh | ||
| this.loadedFromManifest = false; | ||
| writeFileSync(patchPath, stringify(parsedDocFile, { lineWidth: 0 })); |
Check failure
Code scanning / CodeQL
Potential file system race condition High
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/webview_provider/docsEditPanel.ts(7 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
src/webview_provider/docsEditPanel.ts
[failure] 792-792: Potential file system race condition
The file may have changed since it was checked.
[failure] 924-924: Potential file system race condition
The file may have changed since it was checked.
🪛 Biome
src/webview_provider/docsEditPanel.ts
[error] 761-764: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 771-773: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
src/webview_provider/docsEditPanel.ts (3)
23-23: LGTM: New imports and constructor changes
The new imports and constructor dependency injection are well-structured and align with the new lineage functionality being added.
Also applies to: 35-35, 53-58, 121-121
660-668: LGTM: Column lineage base handler
The handler is well-implemented with proper error handling through the callback.
972-1024: LGTM: Well-structured utility methods
The new utility methods handleSyncRequestFromWebview and sendResponseToWebview are well-implemented with:
- Proper error handling
- Type safety
- Consistent response format
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 42eb8bc in 49 seconds
More details
- Looked at
75lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:11
- Draft comment:
Remove unused importpanelLoggerto clean up the code. - Reason this comment was not posted:
Confidence changes required:10%
The code inDocumentationPropagation.tsxhas a redundant import statement forpanelLoggerwhich is not used anywhere in the file. Removing unused imports is a good practice to keep the code clean and efficient.
2. src/webview_provider/docsEditPanel.ts:741
- Draft comment:
Consider adding error handling withinwindow.withProgressto manage any errors during the bulk save operation. - Reason this comment was not posted:
Confidence changes required:50%
ThesaveDocumentationBulkfunction indocsEditPanel.tsuseswindow.withProgressto show progress while saving documentation. However, it does not handle any errors that might occur during the save operation. Adding error handling would improve robustness.
3. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:157
- Draft comment:
Consider using??instead of||for nullish coalescing to handlenullorundefinedvalues more accurately. - Reason this comment was not posted:
Confidence changes required:30%
ThepatchPathassignment inDocumentationPropagation.tsxuses a fallback mechanism with||. In TypeScript, using??(nullish coalescing) is preferred for handlingnullorundefinedvalues, as it avoids false positives with falsy values like0or''.
4. src/webview_provider/docsEditPanel.ts:734
- Draft comment:
Consider using early returns in thesaveDocumentationBulkfunction to reduce nesting and improve readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The 'saveDocumentationBulk' function is a new addition, and the comment is about improving readability by using early returns. The function currently uses a loop to iterate over 'message.models' and calls 'saveDocumentation' for each item. There is no obvious nesting that could be reduced by early returns, as the function is straightforward.
I might be missing some subtlety in how early returns could be applied here, but the function seems simple enough that early returns wouldn't significantly improve readability.
Given the simplicity of the function, early returns might not provide a noticeable improvement in readability. The function's current structure is clear and direct.
The comment about using early returns in the 'saveDocumentationBulk' function is not strongly justified given the function's simplicity. The comment should be deleted.
5. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:149
- Draft comment:
Use semantic versioning (semver) for version comparisons instead of direct array index comparisons for better accuracy and readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be misplaced as there is no evidence of version comparison or array index usage for versioning in the provided code. The code is focused on propagating documentation and handling related data structures.
I might be missing some context about where versioning could be relevant, but based on the provided code, there is no indication of version handling that would require semantic versioning.
Given the code provided, there is no indication of version handling, so the comment about semantic versioning seems irrelevant.
The comment about using semantic versioning is not relevant to the changes made in the diff and should be deleted.
Workflow ID: wflow_LZV4eGj5tmoQrvl0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (4)
41-76: Consider consolidating state managementThe component uses multiple separate useState hooks that could be consolidated into a single state object for better maintainability.
Consider refactoring the state management like this:
- const [allColumns, setAllColumns] = useState<DocsItem[]>([]); - const [currColumns, setCurrColumns] = useState<DocsItem[]>([]); - const [isLoading, setIsLoading] = useState(false); - const [tableMetadata, setTableMetadata] = useState<TableMetadata[]>([]); - const [testsMetadata, setTestsMetadata] = useState<Record<string, unknown>>({}); - const [selectedColumns, setSelectedColumns] = useState<Record<string, boolean>>({}); + const [state, setState] = useState({ + allColumns: [] as DocsItem[], + currColumns: [] as DocsItem[], + isLoading: false, + tableMetadata: [] as TableMetadata[], + testsMetadata: {} as Record<string, unknown>, + selectedColumns: {} as Record<string, boolean> + });
77-86: Extract magic number into a named constantThe iteration limit of 3 should be extracted into a named constant for better maintainability.
+const MAX_DOWNSTREAM_LEVELS = 3; const loadMoreDownstreamModels = async () => { executeRequestInAsync("columnLineageBase", { event: "start" }); setIsLoading(true); let i = 0; const iAllColumns = [...allColumns]; let iCurrColumns = currColumns; - while (i++ < 3) { + while (i++ < MAX_DOWNSTREAM_LEVELS) {
99-100: Remove commented codeRemove the commented line as it's not providing any value and could cause confusion.
if (item.type === "indirect") continue; - // if (item.viewsType !== "Unchanged") continue;
156-158: Use nullish coalescing consistentlyThe eslint comment suggests nullish coalescing is preferred, but logical OR is used instead.
- // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - patchPath: node?.patchPath || defaultPatchPath, + patchPath: node?.patchPath ?? defaultPatchPath,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/webview_provider/docsEditPanel.ts(6 hunks)webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
src/webview_provider/docsEditPanel.ts
[failure] 810-810: Potential file system race condition
The file may have changed since it was checked.
[failure] 942-942: Potential file system race condition
The file may have changed since it was checked.
🪛 Biome
src/webview_provider/docsEditPanel.ts
[error] 779-782: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 789-791: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx
[error] 170-170: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (6)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (3)
13-39: LGTM: Well-structured interfaces
The interfaces are well-defined with appropriate types and clear structure.
166-176: ** Performance optimization needed in setAllColumnsValue**
This comment is skipped as it's already covered in past review comments and the static analysis hints correctly identified the performance issue with spread operator in reducers.
🧰 Tools
🪛 Biome
[error] 170-170: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
178-269: LGTM: Well-structured UI implementation
The UI implementation is well-organized with proper conditional rendering and state management for buttons and form elements.
src/webview_provider/docsEditPanel.ts (3)
23-23: LGTM: Clean dependency injection and import organization
The new imports and constructor injection of DbtLineageService are well-organized and follow good practices.
Also applies to: 35-35, 53-58, 121-121
660-668: LGTM: Clean implementation of columnLineageBase handler
The handler properly delegates to the lineage service and handles cancellation.
990-1042: LGTM: Well-implemented utility methods
The new utility methods handleSyncRequestFromWebview and sendResponseToWebview are well-structured with proper error handling and type safety.
| ); | ||
|
|
||
| if (existingColumn) { | ||
| // ignore tests, data_tests from existing column, as it will be recreated in `getTestDataByColumn` | ||
| const { tests, data_tests, ...rest } = existingColumn.toJSON(); | ||
| this.setOrDeleteInParsedDocument( | ||
| existingColumn, | ||
| "description", | ||
| column.description?.trim(), | ||
| ); | ||
| this.setOrDeleteInParsedDocument( | ||
| existingColumn, | ||
| "data_type", | ||
| (rest.data_type || column.type)?.toLowerCase(), | ||
| ); | ||
| const allTests = this.getTestDataByColumn( | ||
| message, | ||
| column.name, | ||
| project, | ||
| existingColumn.toJSON(), | ||
| ); | ||
| this.setOrDeleteInParsedDocument( | ||
| existingColumn, | ||
| "tests", | ||
| allTests?.tests, | ||
| ); | ||
| this.setOrDeleteInParsedDocument( | ||
| existingColumn, | ||
| "data_tests", | ||
| allTests?.data_tests, | ||
| ); | ||
| } else { | ||
| const name = getColumnNameByCase( | ||
| column.name, | ||
| projectByFilePath.getAdapterType(), | ||
| ); | ||
| model.addIn(["columns"], { | ||
| name, | ||
| description: column.description?.trim() || undefined, | ||
| data_type: column.type?.toLowerCase(), | ||
| ...this.getTestDataByColumn(message, column.name, project), | ||
| ...(isQuotedIdentifier( | ||
| column.name, | ||
| projectByFilePath.getAdapterType(), | ||
| ) | ||
| ? { quote: true } | ||
| : undefined), | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| // Force reload from manifest after manifest refresh | ||
| this.loadedFromManifest = false; | ||
| writeFileSync(patchPath, stringify(parsedDocFile, { lineWidth: 0 })); | ||
| this.documentation = ( | ||
| await this.docGenService.getDocumentationForCurrentActiveFile() | ||
| ).documentation; | ||
| const tests = await this.dbtTestService.getTestsForCurrentModel(); | ||
| if (syncRequestId) { | ||
| this._panel!.webview.postMessage({ | ||
| command: "response", | ||
| args: { | ||
| syncRequestId, | ||
| body: { | ||
| saved: true, | ||
| tests, | ||
| }, | ||
| status: true, | ||
| }, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| this.transmitError(); | ||
| this.telemetry.sendTelemetryError( | ||
| TelemetryEvents["DocumentationEditor/SaveError"], | ||
| error, | ||
| ); | ||
| window.showErrorMessage( | ||
| `Could not save documentation to ${patchPath}: ${error}`, | ||
| ); | ||
| this.terminal.error( | ||
| "saveDocumentationError", | ||
| `Could not save documentation to ${patchPath}`, | ||
| error, | ||
| false, | ||
| ); | ||
| if (syncRequestId) { | ||
| this._panel!.webview.postMessage({ | ||
| command: "response", | ||
| args: { | ||
| syncRequestId, | ||
| body: { | ||
| saved: false, | ||
| }, | ||
| status: true, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address file system race conditions and improve code organization
The saveDocumentation method has several issues:
-
Potential file system race conditions when checking existence and writing files
-
Switch statement declarations that could leak
-
Method is quite long and could be broken down
-
Fix file system race conditions:
+ import { writeFileSync, readFileSync, renameSync } from 'fs';
+ import { tmpdir } from 'os';
+ import { randomBytes } from 'crypto';
private async saveDocumentation(message: any, syncRequestId: string) {
let patchPath = message.patchPath;
try {
// ... existing code ...
- if (!existsSync(patchPath)) {
- writeFileSync(patchPath, "");
+ // Create a temporary file
+ const tmpPath = path.join(tmpdir(), randomBytes(16).toString('hex'));
+ if (!existsSync(patchPath)) {
+ writeFileSync(tmpPath, "");
+ renameSync(tmpPath, patchPath);
}
// ... existing code ...
- writeFileSync(patchPath, stringify(parsedDocFile, { lineWidth: 0 }));
+ writeFileSync(tmpPath, stringify(parsedDocFile, { lineWidth: 0 }));
+ renameSync(tmpPath, patchPath);- Fix switch statement declarations:
switch (message.dialogType) {
case "Existing file": {
+ let openDialog;
openDialog = await window.showOpenDialog({
filters: { Yaml: ["yml"] },
canSelectMany: false,
});
if (openDialog === undefined || openDialog.length === 0) {
return;
}
patchPath = openDialog[0].fsPath;
break;
}
case "New file": {
+ let saveDialog;
saveDialog = await window.showSaveDialog({
filters: { Yaml: ["yml"] },
});
if (!saveDialog) {
return;
}
patchPath = saveDialog.fsPath;
break;
}
}- Consider extracting the following methods to improve readability:
handleFileSelection(dialogType: string): Promise<string>updateModelDocumentation(model: YAMLMap, message: any)updateColumnDocumentation(column: YAMLMap, message: any)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 810-810: Potential file system race condition
The file may have changed since it was checked.
[failure] 942-942: Potential file system race condition
The file may have changed since it was checked.
🪛 Biome
[error] 779-782: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 789-791: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| case "getDownstreamColumns": { | ||
| const targets = params.targets as [string, string][]; | ||
| const _tables = targets | ||
| .map( | ||
| (t) => | ||
| this.dbtLineageService.getDownstreamTables({ table: t[0] }) | ||
| ?.tables, | ||
| ) | ||
| .filter((t) => Boolean(t)) | ||
| .flat() as Table[]; | ||
| const tables = _tables.map((t) => t?.table); | ||
| if (tables.length === 0) { | ||
| this.handleSyncRequestFromWebview( | ||
| syncRequestId, | ||
| () => ({}), | ||
| command, | ||
| ); | ||
| return; | ||
| } | ||
| const selectedColumn = { | ||
| table: params.model as string, | ||
| name: params.column as string, | ||
| }; | ||
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | ||
| const columns = await this.dbtLineageService.getConnectedColumns({ | ||
| targets, | ||
| currAnd1HopTables, | ||
| selectedColumn, | ||
| upstreamExpansion: false, | ||
| showIndirectEdges: false, | ||
| eventType: "documentation_propagation", | ||
| }); | ||
| const testsResult = await Promise.all( | ||
| targets.map(async (t) => { | ||
| if (!t[0].startsWith("model")) { | ||
| return; | ||
| } | ||
| const splits = t[0].split("."); | ||
| const modelName = splits[splits.length - 1]; | ||
| return await this.dbtTestService.getTestsForModel(modelName); | ||
| }), | ||
| ); | ||
| const tests: Record<string, unknown> = {}; | ||
| targets.forEach((t, i) => { | ||
| tests[t[0]] = testsResult[i]; | ||
| }); | ||
| this.handleSyncRequestFromWebview( | ||
| syncRequestId, | ||
| () => { | ||
| return { ...columns, tables: _tables, tests }; | ||
| }, | ||
| command, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix async operations and add error handling in getDownstreamColumns
The current implementation has several issues that need to be addressed:
- Async operations aren't properly awaited
- Missing null check before using startsWith
- No error handling for the entire operation
Apply this diff to fix the issues:
case "getDownstreamColumns": {
+ try {
const targets = params.targets as [string, string][];
- const _tables = targets
- .map(
- (t) =>
- this.dbtLineageService.getDownstreamTables({ table: t[0] })
- ?.tables,
- )
- .filter((t) => Boolean(t))
- .flat() as Table[];
+ const _tablesNested = await Promise.all(
+ targets.map(async (t) => {
+ const result = await this.dbtLineageService.getDownstreamTables({ table: t[0] });
+ return result?.tables;
+ })
+ );
+ const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[];
const tables = _tables.map((t) => t?.table);
if (tables.length === 0) {
this.handleSyncRequestFromWebview(
syncRequestId,
() => ({}),
command,
);
return;
}
const selectedColumn = {
table: params.model as string,
name: params.column as string,
};
const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])];
const columns = await this.dbtLineageService.getConnectedColumns({
targets,
currAnd1HopTables,
selectedColumn,
upstreamExpansion: false,
showIndirectEdges: false,
eventType: "documentation_propagation",
});
const testsResult = await Promise.all(
targets.map(async (t) => {
- if (!t[0].startsWith("model")) {
+ if (!t[0] || !t[0].startsWith("model")) {
return;
}
const splits = t[0].split(".");
const modelName = splits[splits.length - 1];
return await this.dbtTestService.getTestsForModel(modelName);
}),
);
const tests: Record<string, unknown> = {};
targets.forEach((t, i) => {
tests[t[0]] = testsResult[i];
});
this.handleSyncRequestFromWebview(
syncRequestId,
() => {
return { ...columns, tables: _tables, tests };
},
command,
);
+ } catch (error) {
+ this.handleSyncRequestFromWebview(
+ syncRequestId,
+ () => {
+ throw error;
+ },
+ command,
+ true
+ );
+ }
break;
}📝 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.
| case "getDownstreamColumns": { | |
| const targets = params.targets as [string, string][]; | |
| const _tables = targets | |
| .map( | |
| (t) => | |
| this.dbtLineageService.getDownstreamTables({ table: t[0] }) | |
| ?.tables, | |
| ) | |
| .filter((t) => Boolean(t)) | |
| .flat() as Table[]; | |
| const tables = _tables.map((t) => t?.table); | |
| if (tables.length === 0) { | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => ({}), | |
| command, | |
| ); | |
| return; | |
| } | |
| const selectedColumn = { | |
| table: params.model as string, | |
| name: params.column as string, | |
| }; | |
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | |
| const columns = await this.dbtLineageService.getConnectedColumns({ | |
| targets, | |
| currAnd1HopTables, | |
| selectedColumn, | |
| upstreamExpansion: false, | |
| showIndirectEdges: false, | |
| eventType: "documentation_propagation", | |
| }); | |
| const testsResult = await Promise.all( | |
| targets.map(async (t) => { | |
| if (!t[0].startsWith("model")) { | |
| return; | |
| } | |
| const splits = t[0].split("."); | |
| const modelName = splits[splits.length - 1]; | |
| return await this.dbtTestService.getTestsForModel(modelName); | |
| }), | |
| ); | |
| const tests: Record<string, unknown> = {}; | |
| targets.forEach((t, i) => { | |
| tests[t[0]] = testsResult[i]; | |
| }); | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => { | |
| return { ...columns, tables: _tables, tests }; | |
| }, | |
| command, | |
| ); | |
| break; | |
| } | |
| case "getDownstreamColumns": { | |
| try { | |
| const targets = params.targets as [string, string][]; | |
| const _tablesNested = await Promise.all( | |
| targets.map(async (t) => { | |
| const result = await this.dbtLineageService.getDownstreamTables({ table: t[0] }); | |
| return result?.tables; | |
| }) | |
| ); | |
| const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[]; | |
| const tables = _tables.map((t) => t?.table); | |
| if (tables.length === 0) { | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => ({}), | |
| command, | |
| ); | |
| return; | |
| } | |
| const selectedColumn = { | |
| table: params.model as string, | |
| name: params.column as string, | |
| }; | |
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | |
| const columns = await this.dbtLineageService.getConnectedColumns({ | |
| targets, | |
| currAnd1HopTables, | |
| selectedColumn, | |
| upstreamExpansion: false, | |
| showIndirectEdges: false, | |
| eventType: "documentation_propagation", | |
| }); | |
| const testsResult = await Promise.all( | |
| targets.map(async (t) => { | |
| if (!t[0] || !t[0].startsWith("model")) { | |
| return; | |
| } | |
| const splits = t[0].split("."); | |
| const modelName = splits[splits.length - 1]; | |
| return await this.dbtTestService.getTestsForModel(modelName); | |
| }), | |
| ); | |
| const tests: Record<string, unknown> = {}; | |
| targets.forEach((t, i) => { | |
| tests[t[0]] = testsResult[i]; | |
| }); | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => { | |
| return { ...columns, tables: _tables, tests }; | |
| }, | |
| command, | |
| ); | |
| } catch (error) { | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => { | |
| throw error; | |
| }, | |
| command, | |
| true | |
| ); | |
| } | |
| break; | |
| } |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 773d0b9 in 30 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_yyFUkAfUp7X5fHtE
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.
| if (item.type === "indirect") continue; | ||
| if (item.viewsType !== "Unchanged") continue; | ||
| if ( | ||
| iCurrColumns.find( |
There was a problem hiding this comment.
Consider optimizing the search for columns in iCurrColumns by using a Set or Map for faster lookup instead of using find in a loop. This will improve performance, especially with larger datasets.
| iCurrColumns = []; | ||
| break; | ||
| } | ||
| const newColumns: DocsItem[] = []; |
There was a problem hiding this comment.
Consider using early returns in loadMoreDownstreamModels to reduce nesting and improve readability. This applies to other parts of the function as well.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (2)
41-70: Consider using useReducer for better state managementThe component currently manages multiple related state variables. Consider refactoring to use
useReducerto centralize state management and make state updates more predictable.Example implementation:
interface State { allColumns: DocsItem[]; currColumns: DocsItem[]; isLoading: boolean; tableMetadata: TableMetadata[]; testsMetadata: Record<string, unknown>; selectedColumns: Record<string, boolean>; } type Action = | { type: 'SET_ALL_COLUMNS', payload: DocsItem[] } | { type: 'SET_CURR_COLUMNS', payload: DocsItem[] } | { type: 'SET_LOADING', payload: boolean } // ... other actions const reducer = (state: State, action: Action): State => { switch (action.type) { case 'SET_ALL_COLUMNS': return { ...state, allColumns: action.payload }; // ... other cases } };
186-271: Enhance accessibility and loading state feedbackConsider the following improvements:
- Add aria-labels to buttons and inputs
- Show loading spinner during operations
- Add error messages for failed operations
Example improvements:
<Button color="primary" outline onClick={loadMoreDownstreamModels} disabled={isLoading} + aria-label="Load more downstream models" > - Load 3 more downstream levels + {isLoading ? 'Loading...' : 'Load 3 more downstream levels'} </Button>src/webview_provider/docsEditPanel.ts (1)
762-762: Add return type for better type safetyThe function should explicitly declare its return type.
- private async saveDocumentation(message: any, syncRequestId: string) { + private async saveDocumentation(message: any, syncRequestId: string): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/webview_provider/docsEditPanel.ts(6 hunks)webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
src/webview_provider/docsEditPanel.ts
[failure] 810-810: Potential file system race condition
The file may have changed since it was checked.
[failure] 942-942: Potential file system race condition
The file may have changed since it was checked.
🪛 Biome
src/webview_provider/docsEditPanel.ts
[error] 779-782: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 789-791: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx
[error] 174-174: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (6)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (3)
13-39: LGTM! Well-structured interfaces
The interfaces are well-defined with appropriate types and clear structure.
140-168: Add validation and error handling to propagateDocumentation
The function should validate inputs and handle potential errors during the bulk save operation.
Consider adding:
- Input validation before processing
- Try-catch block around the save operation
- User feedback for success/failure cases
170-180: Skip: Performance issue already flagged
This issue was already identified in a previous review comment and the static analysis hints.
🧰 Tools
🪛 Biome
[error] 174-174: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
src/webview_provider/docsEditPanel.ts (3)
23-23: LGTM: Clean dependency injection and imports
The new imports and constructor parameter for DbtLineageService are well-organized and follow dependency injection patterns.
Also applies to: 35-35, 53-58, 121-121
660-668: LGTM: Clean implementation of column lineage base handler
The handler properly integrates with the lineage service and includes appropriate event handling.
990-1042: LGTM: Well-structured utility methods
The new utility methods are well-implemented with:
- Proper error handling including PythonException
- Consistent response formatting
- Clear separation of concerns
| const loadMoreDownstreamModels = async () => { | ||
| executeRequestInAsync("columnLineageBase", { event: "start" }); | ||
| setIsLoading(true); | ||
| let i = 0; | ||
| const iAllColumns = [...allColumns]; | ||
| let iCurrColumns = currColumns; | ||
| while (i++ < 3) { | ||
| if (iCurrColumns.length === 0) { | ||
| break; | ||
| } | ||
| const result = (await executeRequestInSync("getDownstreamColumns", { | ||
| targets: iCurrColumns.map((c) => [c.model, c.column]), | ||
| model: currentDocsData?.uniqueId, | ||
| column: name, | ||
| })) as DownstreamColumns; | ||
| if (!result.column_lineage) { | ||
| break; | ||
| } | ||
| setTableMetadata((prev) => [...prev, ...result.tables]); | ||
| setTestsMetadata((prev) => ({ ...prev, ...result.tests })); | ||
| if (result.column_lineage.length === 0) { | ||
| iCurrColumns = []; | ||
| break; | ||
| } | ||
| const newColumns: DocsItem[] = []; | ||
| for (const item of result.column_lineage) { | ||
| if (item.type === "indirect") continue; | ||
| if (item.viewsType !== "Unchanged") continue; | ||
| if ( | ||
| iCurrColumns.find( | ||
| (c) => c.model === item.target[0] && c.column === item.target[1], | ||
| ) | ||
| ) { | ||
| newColumns.push({ | ||
| model: item.source[0], | ||
| column: item.source[1], | ||
| description: | ||
| result.tables.find((t) => t.table === item.source[0])?.columns[ | ||
| item.source[1] | ||
| ]?.description ?? "", | ||
| }); | ||
| } | ||
| } | ||
| iCurrColumns = newColumns; | ||
| iAllColumns.push(...newColumns); | ||
| } | ||
| executeRequestInAsync("columnLineageBase", { event: "end" }); | ||
| const finalAllColumns: DocsItem[] = []; | ||
| for (const c of iAllColumns) { | ||
| if ( | ||
| finalAllColumns.find( | ||
| (_c) => _c.model === c.model && _c.column === c.column, | ||
| ) | ||
| ) { | ||
| continue; | ||
| } | ||
| finalAllColumns.push(c); | ||
| } | ||
| setIsLoading(false); | ||
| setAllColumns(finalAllColumns); | ||
| setCurrColumns(iCurrColumns); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and simplify loadMoreDownstreamModels function
The function could benefit from:
- Proper error handling for failed requests
- Breaking down complex logic into smaller functions
- Adding loading state error handling
Consider refactoring like this:
const loadMoreDownstreamModels = async () => {
try {
executeRequestInAsync("columnLineageBase", { event: "start" });
setIsLoading(true);
const newColumns = await loadNextThreeLevels();
const finalColumns = removeDuplicateColumns(newColumns);
setAllColumns(finalColumns);
setCurrColumns(newColumns.currColumns);
} catch (error) {
console.error('Failed to load downstream models:', error);
// Handle error appropriately
} finally {
setIsLoading(false);
executeRequestInAsync("columnLineageBase", { event: "end" });
}
};| case "getDownstreamColumns": { | ||
| const targets = params.targets as [string, string][]; | ||
| const testsResult = await Promise.all( | ||
| targets.map(async (t) => { | ||
| if (!t[0].startsWith("model")) { | ||
| return; | ||
| } | ||
| const splits = t[0].split("."); | ||
| const modelName = splits[splits.length - 1]; | ||
| return await this.dbtTestService.getTestsForModel(modelName); | ||
| }), | ||
| ); | ||
| const tests: Record<string, unknown> = {}; | ||
| targets.forEach((t, i) => { | ||
| tests[t[0]] = testsResult[i]; | ||
| }); | ||
| const _tables = targets | ||
| .map( | ||
| (t) => | ||
| this.dbtLineageService.getDownstreamTables({ table: t[0] }) | ||
| ?.tables, | ||
| ) | ||
| .filter((t) => Boolean(t)) | ||
| .flat() as Table[]; | ||
| const tables = _tables.map((t) => t?.table); | ||
| if (tables.length === 0) { | ||
| this.handleSyncRequestFromWebview( | ||
| syncRequestId, | ||
| () => ({ column_lineage: [], tables: [], tests }), | ||
| command, | ||
| ); | ||
| return; | ||
| } | ||
| const selectedColumn = { | ||
| table: params.model as string, | ||
| name: params.column as string, | ||
| }; | ||
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | ||
| const columns = await this.dbtLineageService.getConnectedColumns({ | ||
| targets, | ||
| currAnd1HopTables, | ||
| selectedColumn, | ||
| upstreamExpansion: false, | ||
| showIndirectEdges: false, | ||
| eventType: "documentation_propagation", | ||
| }); | ||
| this.handleSyncRequestFromWebview( | ||
| syncRequestId, | ||
| () => { | ||
| return { ...columns, tables: _tables, tests }; | ||
| }, | ||
| command, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix async operations and add safety checks
The getDownstreamColumns handler needs several improvements:
- Async operations aren't properly awaited
- Missing null check for array access
- The code could be more maintainable with intermediate variables
Apply this diff to fix the issues:
case "getDownstreamColumns": {
const targets = params.targets as [string, string][];
const testsResult = await Promise.all(
targets.map(async (t) => {
- if (!t[0].startsWith("model")) {
+ if (!t[0] || !t[0].startsWith("model")) {
return;
}
const splits = t[0].split(".");
const modelName = splits[splits.length - 1];
return await this.dbtTestService.getTestsForModel(modelName);
}),
);
const tests: Record<string, unknown> = {};
targets.forEach((t, i) => {
tests[t[0]] = testsResult[i];
});
- const _tables = targets
- .map(
- (t) =>
- this.dbtLineageService.getDownstreamTables({ table: t[0] })
- ?.tables,
- )
- .filter((t) => Boolean(t))
- .flat() as Table[];
+ const _tablesNested = await Promise.all(
+ targets.map(async (t) => {
+ const result = await this.dbtLineageService.getDownstreamTables({ table: t[0] });
+ return result?.tables;
+ })
+ );
+ const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[];📝 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.
| case "getDownstreamColumns": { | |
| const targets = params.targets as [string, string][]; | |
| const testsResult = await Promise.all( | |
| targets.map(async (t) => { | |
| if (!t[0].startsWith("model")) { | |
| return; | |
| } | |
| const splits = t[0].split("."); | |
| const modelName = splits[splits.length - 1]; | |
| return await this.dbtTestService.getTestsForModel(modelName); | |
| }), | |
| ); | |
| const tests: Record<string, unknown> = {}; | |
| targets.forEach((t, i) => { | |
| tests[t[0]] = testsResult[i]; | |
| }); | |
| const _tables = targets | |
| .map( | |
| (t) => | |
| this.dbtLineageService.getDownstreamTables({ table: t[0] }) | |
| ?.tables, | |
| ) | |
| .filter((t) => Boolean(t)) | |
| .flat() as Table[]; | |
| const tables = _tables.map((t) => t?.table); | |
| if (tables.length === 0) { | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => ({ column_lineage: [], tables: [], tests }), | |
| command, | |
| ); | |
| return; | |
| } | |
| const selectedColumn = { | |
| table: params.model as string, | |
| name: params.column as string, | |
| }; | |
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | |
| const columns = await this.dbtLineageService.getConnectedColumns({ | |
| targets, | |
| currAnd1HopTables, | |
| selectedColumn, | |
| upstreamExpansion: false, | |
| showIndirectEdges: false, | |
| eventType: "documentation_propagation", | |
| }); | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => { | |
| return { ...columns, tables: _tables, tests }; | |
| }, | |
| command, | |
| ); | |
| break; | |
| } | |
| case "getDownstreamColumns": { | |
| const targets = params.targets as [string, string][]; | |
| const testsResult = await Promise.all( | |
| targets.map(async (t) => { | |
| if (!t[0] || !t[0].startsWith("model")) { | |
| return; | |
| } | |
| const splits = t[0].split("."); | |
| const modelName = splits[splits.length - 1]; | |
| return await this.dbtTestService.getTestsForModel(modelName); | |
| }), | |
| ); | |
| const tests: Record<string, unknown> = {}; | |
| targets.forEach((t, i) => { | |
| tests[t[0]] = testsResult[i]; | |
| }); | |
| const _tablesNested = await Promise.all( | |
| targets.map(async (t) => { | |
| const result = await this.dbtLineageService.getDownstreamTables({ table: t[0] }); | |
| return result?.tables; | |
| }) | |
| ); | |
| const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[]; | |
| const tables = _tables.map((t) => t?.table); | |
| if (tables.length === 0) { | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => ({ column_lineage: [], tables: [], tests }), | |
| command, | |
| ); | |
| return; | |
| } | |
| const selectedColumn = { | |
| table: params.model as string, | |
| name: params.column as string, | |
| }; | |
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | |
| const columns = await this.dbtLineageService.getConnectedColumns({ | |
| targets, | |
| currAnd1HopTables, | |
| selectedColumn, | |
| upstreamExpansion: false, | |
| showIndirectEdges: false, | |
| eventType: "documentation_propagation", | |
| }); | |
| this.handleSyncRequestFromWebview( | |
| syncRequestId, | |
| () => { | |
| return { ...columns, tables: _tables, tests }; | |
| }, | |
| command, | |
| ); | |
| break; | |
| } |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b944892 in 30 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:104
- Draft comment:
Ensure that filtering out 'Transformation' types aligns with the intended logic. Double-check if this change from 'Unchanged' to 'Transformation' is correct. - Reason this comment was not posted:
Comment did not seem useful.
2. webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx:101
- Draft comment:
Use early returns inloadMoreDownstreamModelsto reduce nesting and improve readability. This applies to the entire function. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_sNJr33vS0qvawQeH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (2)
125-134: Optimize duplicate checking infinalAllColumnsconstructionIn the loop constructing
finalAllColumns, using.findinside a loop can lead to O(n²) time complexity due to repeated linear searches. To improve performance, consider using aSetor aMapto track duplicates more efficiently.Apply this diff to enhance performance:
- const finalAllColumns: DocsItem[] = []; - for (const c of iAllColumns) { - if ( - finalAllColumns.find( - (_c) => _c.model === c.model && _c.column === c.column, - ) - ) { - continue; - } - finalAllColumns.push(c); - } + const seenKeys = new Set<string>(); + const finalAllColumns: DocsItem[] = []; + for (const c of iAllColumns) { + const key = c.model + "/" + c.column; + if (seenKeys.has(key)) { + continue; + } + seenKeys.add(key); + finalAllColumns.push(c); + }
263-265: Optimize selection count calculationCalculating the number of selected columns using
Object.values(selectedColumns).filter((v) => Boolean(v)).lengthon every render can be inefficient for large datasets. Consider maintaining a separate count state or using a memoized selector to improve performance.Here's how you might implement a selection count state:
+ const [selectedCount, setSelectedCount] = useState(0); + useEffect(() => { + setSelectedCount( + Object.values(selectedColumns).filter((v) => Boolean(v)).length + ); + }, [selectedColumns]); <Button color="primary" disabled={ - Object.values(selectedColumns).filter((v) => Boolean(v)).length === 0 + selectedCount === 0 } onClick={() => propagateDocumentation()} > Propagate documentation to selected models </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx
[error] 174-174: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (1)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx (1)
226-231: Avoid using spread operator when updating state in setSelectedColumns
Using the spread operator in state updates can be inefficient for large objects. Since selectedColumns is an object, consider using functional updates or alternative methods to update state without unnecessarily copying the entire object.
However, in this context, the impact might be minimal. If performance becomes a concern with large datasets, refactoring might be needed.
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| patchPath: node?.patchPath || defaultPatchPath, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use nullish coalescing operator instead of logical OR
The use of || can lead to unintended behavior when node?.patchPath is a falsy value like an empty string. Replace node?.patchPath || defaultPatchPath with node?.patchPath ?? defaultPatchPath to ensure that defaultPatchPath is used only when node?.patchPath is null or undefined.
Apply this diff to follow best practices:
- // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
- patchPath: node?.patchPath || defaultPatchPath,
+ patchPath: node?.patchPath ?? defaultPatchPath,Also, remove the ESLint disable comment since it is no longer necessary.
📝 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.
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| patchPath: node?.patchPath || defaultPatchPath, | |
| patchPath: node?.patchPath ?? defaultPatchPath, |
| const propagateDocumentation = async () => { | ||
| const defaultPackageName = tableMetadata.filter((t) => t.packageName)[0] | ||
| ?.packageName; | ||
| const defaultPatchPath = defaultPackageName | ||
| ? defaultPackageName + "://models/schema.yml" | ||
| : ""; | ||
|
|
||
| const req = []; | ||
|
|
||
| for (const item of allColumns) { | ||
| const key = item.model + "/" + item.column; | ||
| if (!selectedColumns[key]) continue; | ||
| const splits = item.model.split("."); | ||
| const modelName = splits[splits.length - 1]; | ||
| const node = tableMetadata.find((t) => t.table === item.model); | ||
| req.push({ | ||
| name: modelName, | ||
| description: node?.description, | ||
| columns: [{ name: item.column, description: currColumnDescription }], | ||
| dialogType: "Existing file", | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| patchPath: node?.patchPath || defaultPatchPath, | ||
| filePath: node?.url, | ||
| updatedTests: testsMetadata[item.model], | ||
| }); | ||
| } | ||
|
|
||
| await executeRequestInSync("saveDocumentationBulk", { models: req }); | ||
| }; |
There was a problem hiding this comment.
Add error handling in propagateDocumentation function
The function propagateDocumentation lacks error handling for the asynchronous operation. If executeRequestInSync("saveDocumentationBulk", { models: req }) fails, it could lead to unhandled promise rejections and impact user experience. Implement error handling to manage potential failures gracefully.
Apply this diff to include error handling:
- await executeRequestInSync("saveDocumentationBulk", { models: req });
+ try {
+ await executeRequestInSync("saveDocumentationBulk", { models: req });
+ } catch (error) {
+ console.error('Failed to propagate documentation:', error);
+ // Optionally, provide user feedback or retry logic here
+ }📝 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 propagateDocumentation = async () => { | |
| const defaultPackageName = tableMetadata.filter((t) => t.packageName)[0] | |
| ?.packageName; | |
| const defaultPatchPath = defaultPackageName | |
| ? defaultPackageName + "://models/schema.yml" | |
| : ""; | |
| const req = []; | |
| for (const item of allColumns) { | |
| const key = item.model + "/" + item.column; | |
| if (!selectedColumns[key]) continue; | |
| const splits = item.model.split("."); | |
| const modelName = splits[splits.length - 1]; | |
| const node = tableMetadata.find((t) => t.table === item.model); | |
| req.push({ | |
| name: modelName, | |
| description: node?.description, | |
| columns: [{ name: item.column, description: currColumnDescription }], | |
| dialogType: "Existing file", | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| patchPath: node?.patchPath || defaultPatchPath, | |
| filePath: node?.url, | |
| updatedTests: testsMetadata[item.model], | |
| }); | |
| } | |
| await executeRequestInSync("saveDocumentationBulk", { models: req }); | |
| }; | |
| const propagateDocumentation = async () => { | |
| const defaultPackageName = tableMetadata.filter((t) => t.packageName)[0] | |
| ?.packageName; | |
| const defaultPatchPath = defaultPackageName | |
| ? defaultPackageName + "://models/schema.yml" | |
| : ""; | |
| const req = []; | |
| for (const item of allColumns) { | |
| const key = item.model + "/" + item.column; | |
| if (!selectedColumns[key]) continue; | |
| const splits = item.model.split("."); | |
| const modelName = splits[splits.length - 1]; | |
| const node = tableMetadata.find((t) => t.table === item.model); | |
| req.push({ | |
| name: modelName, | |
| description: node?.description, | |
| columns: [{ name: item.column, description: currColumnDescription }], | |
| dialogType: "Existing file", | |
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
| patchPath: node?.patchPath || defaultPatchPath, | |
| filePath: node?.url, | |
| updatedTests: testsMetadata[item.model], | |
| }); | |
| } | |
| try { | |
| await executeRequestInSync("saveDocumentationBulk", { models: req }); | |
| } catch (error) { | |
| console.error('Failed to propagate documentation:', error); | |
| // Optionally, provide user feedback or retry logic here | |
| } | |
| }; |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on cc9f90a in 1 minute and 1 seconds
More details
- Looked at
48lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/webview_provider/docsEditPanel.ts:688
- Draft comment:
The methodgetUpstreamTablesshould begetDownstreamTablesto correctly fetch downstream tables.
this.dbtLineageService.getDownstreamTables({ table: t[0] })
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The change fromgetDownstreamTablestogetUpstreamTablesseems intentional, especially sinceupstreamExpansionwas also set totrue. This suggests the code is now intended to handle upstream tables. The comment does not provide strong evidence that the change is incorrect, and the context supports the change.
I might be missing some specific business logic that requires downstream tables instead of upstream. However, the change in the code suggests a clear intention to switch to upstream tables.
The change in the code, including settingupstreamExpansiontotrue, strongly indicates that the intention is to work with upstream tables. Without additional context suggesting otherwise, the comment seems incorrect.
The comment should be deleted as the change togetUpstreamTablesappears intentional and aligns with other changes in the code.
2. src/webview_provider/docsEditPanel.ts:685
- Draft comment:
Use semantic versioning (semver) for version comparisons instead of direct array index comparisons. This ensures more accurate and reliable version checks. This issue is also present in other parts of the code. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_89ZEWB7PBOdO5j0Q
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.
| if (item.viewsType === "Transformation") continue; | ||
| if ( | ||
| iCurrColumns.find( | ||
| (c) => c.model === item.source[0] && c.column === item.source[1], |
There was a problem hiding this comment.
The condition should check against item.target instead of item.source to correctly identify current columns.
| (c) => c.model === item.source[0] && c.column === item.source[1], | |
| (c) => c.model === item.target[0] && c.column === item.target[1], |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/webview_provider/docsEditPanel.ts(6 hunks)webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
src/webview_provider/docsEditPanel.ts
[error] 779-782: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 789-791: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
webview_panels/src/modules/documentationEditor/components/documentationPropagation/DocumentationPropagation.tsx
[error] 174-174: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (3)
src/webview_provider/docsEditPanel.ts (3)
23-23: LGTM: Clean dependency injection and error handling setup.
The new imports and constructor changes follow good practices for dependency injection and error handling.
Also applies to: 35-35, 53-58, 121-121
660-668: LGTM: Clean implementation of column lineage base handler.
The handler properly delegates to the lineage service and handles cancellation.
990-1042: LGTM: Well-structured utility methods with proper error handling.
The new utility methods are well-implemented with:
- Proper error handling for different error types
- Clear separation of concerns
- Type-safe message handling
| if (!existsSync(patchPath)) { | ||
| writeFileSync(patchPath, ""); | ||
| } |
There was a problem hiding this comment.
Address potential file system race conditions.
The current implementation has potential race conditions when checking file existence and writing files.
Apply this diff to fix the issues:
+ import { writeFileSync, readFileSync, renameSync } from 'fs';
+ import { tmpdir } from 'os';
+ import { randomBytes } from 'crypto';
- if (!existsSync(patchPath)) {
- writeFileSync(patchPath, "");
- }
+ const tmpPath = path.join(tmpdir(), randomBytes(16).toString('hex'));
+ writeFileSync(tmpPath, "");
+ renameSync(tmpPath, patchPath);
- writeFileSync(patchPath, stringify(parsedDocFile, { lineWidth: 0 }));
+ const tmpOutputPath = path.join(tmpdir(), randomBytes(16).toString('hex'));
+ writeFileSync(tmpOutputPath, stringify(parsedDocFile, { lineWidth: 0 }));
+ renameSync(tmpOutputPath, patchPath);Also applies to: 941-942
| switch (message.dialogType) { | ||
| case "Existing file": | ||
| const openDialog = await window.showOpenDialog({ | ||
| filters: { Yaml: ["yml"] }, | ||
| canSelectMany: false, | ||
| }); | ||
| if (openDialog === undefined || openDialog.length === 0) { | ||
| return; | ||
| } | ||
| patchPath = openDialog[0].fsPath; | ||
| break; | ||
| case "New file": | ||
| const saveDialog = await window.showSaveDialog({ | ||
| filters: { Yaml: ["yml"] }, | ||
| }); | ||
| if (!saveDialog) { | ||
| return; | ||
| } | ||
| this.telemetry.sendTelemetryEvent( | ||
| TelemetryEvents["DocumentationEditor/SaveNewFilePathSelect"], | ||
| ); | ||
| patchPath = saveDialog.fsPath; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix switch statement variable declarations.
The switch statement has variable declarations that could leak between cases.
Apply this diff to fix the issue:
switch (message.dialogType) {
case "Existing file": {
+ let openDialog;
openDialog = await window.showOpenDialog({
filters: { Yaml: ["yml"] },
canSelectMany: false,
});
if (openDialog === undefined || openDialog.length === 0) {
return;
}
patchPath = openDialog[0].fsPath;
break;
}
case "New file": {
+ let saveDialog;
saveDialog = await window.showSaveDialog({
filters: { Yaml: ["yml"] },
});
if (!saveDialog) {
return;
}
patchPath = saveDialog.fsPath;
break;
}
}📝 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.
| switch (message.dialogType) { | |
| case "Existing file": | |
| const openDialog = await window.showOpenDialog({ | |
| filters: { Yaml: ["yml"] }, | |
| canSelectMany: false, | |
| }); | |
| if (openDialog === undefined || openDialog.length === 0) { | |
| return; | |
| } | |
| patchPath = openDialog[0].fsPath; | |
| break; | |
| case "New file": | |
| const saveDialog = await window.showSaveDialog({ | |
| filters: { Yaml: ["yml"] }, | |
| }); | |
| if (!saveDialog) { | |
| return; | |
| } | |
| this.telemetry.sendTelemetryEvent( | |
| TelemetryEvents["DocumentationEditor/SaveNewFilePathSelect"], | |
| ); | |
| patchPath = saveDialog.fsPath; | |
| break; | |
| } | |
| switch (message.dialogType) { | |
| case "Existing file": { | |
| let openDialog; | |
| openDialog = await window.showOpenDialog({ | |
| filters: { Yaml: ["yml"] }, | |
| canSelectMany: false, | |
| }); | |
| if (openDialog === undefined || openDialog.length === 0) { | |
| return; | |
| } | |
| patchPath = openDialog[0].fsPath; | |
| break; | |
| } | |
| case "New file": { | |
| let saveDialog; | |
| saveDialog = await window.showSaveDialog({ | |
| filters: { Yaml: ["yml"] }, | |
| }); | |
| if (!saveDialog) { | |
| return; | |
| } | |
| this.telemetry.sendTelemetryEvent( | |
| TelemetryEvents["DocumentationEditor/SaveNewFilePathSelect"], | |
| ); | |
| patchPath = saveDialog.fsPath; | |
| break; | |
| } | |
| } |
🧰 Tools
🪛 Biome
[error] 779-782: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 789-791: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| case "getDownstreamColumns": { | ||
| const targets = params.targets as [string, string][]; | ||
| const testsResult = await Promise.all( | ||
| targets.map(async (t) => { | ||
| if (!t[0].startsWith("model")) { | ||
| return; | ||
| } | ||
| const splits = t[0].split("."); | ||
| const modelName = splits[splits.length - 1]; | ||
| return await this.dbtTestService.getTestsForModel(modelName); | ||
| }), | ||
| ); | ||
| const tests: Record<string, unknown> = {}; | ||
| targets.forEach((t, i) => { | ||
| tests[t[0]] = testsResult[i]; | ||
| }); | ||
| const _tables = targets | ||
| .map( | ||
| (t) => | ||
| this.dbtLineageService.getUpstreamTables({ table: t[0] }) | ||
| ?.tables, | ||
| ) | ||
| .filter((t) => Boolean(t)) | ||
| .flat() as Table[]; | ||
| const tables = _tables.map((t) => t?.table); | ||
| if (tables.length === 0) { | ||
| this.handleSyncRequestFromWebview( | ||
| syncRequestId, | ||
| () => ({ column_lineage: [], tables: [], tests }), | ||
| command, | ||
| ); | ||
| return; | ||
| } | ||
| const selectedColumn = { | ||
| table: params.model as string, | ||
| name: params.column as string, | ||
| }; | ||
| const currAnd1HopTables = [...tables, ...targets.map((t) => t[0])]; | ||
| const columns = await this.dbtLineageService.getConnectedColumns({ | ||
| targets, | ||
| currAnd1HopTables, | ||
| selectedColumn, | ||
| upstreamExpansion: true, | ||
| showIndirectEdges: false, | ||
| eventType: "documentation_propagation", | ||
| }); | ||
| this.handleSyncRequestFromWebview( | ||
| syncRequestId, | ||
| () => { | ||
| return { ...columns, tables: _tables, tests }; | ||
| }, | ||
| command, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix async operations and add safety checks.
The getDownstreamColumns handler needs several improvements:
- Missing null check before using startsWith
- Async operations in getDownstreamTables aren't properly awaited
- No error handling for the entire operation
Apply this diff to fix the issues:
case "getDownstreamColumns": {
try {
const targets = params.targets as [string, string][];
const testsResult = await Promise.all(
targets.map(async (t) => {
- if (!t[0].startsWith("model")) {
+ if (!t[0] || !t[0].startsWith("model")) {
return;
}
const splits = t[0].split(".");
const modelName = splits[splits.length - 1];
return await this.dbtTestService.getTestsForModel(modelName);
}),
);
const tests: Record<string, unknown> = {};
targets.forEach((t, i) => {
tests[t[0]] = testsResult[i];
});
- const _tables = targets
- .map(
- (t) =>
- this.dbtLineageService.getUpstreamTables({ table: t[0] })
- ?.tables,
- )
- .filter((t) => Boolean(t))
- .flat() as Table[];
+ const _tablesNested = await Promise.all(
+ targets.map(async (t) => {
+ const result = await this.dbtLineageService.getUpstreamTables({ table: t[0] });
+ return result?.tables;
+ })
+ );
+ const _tables = _tablesNested.filter((t) => Boolean(t)).flat() as Table[];
// ... rest of the code ...
} catch (error) {
this.handleSyncRequestFromWebview(
syncRequestId,
() => {
throw error;
},
command,
true
);
}
}Committable suggestion skipped: line range outside the PR's diff.
| for (const c of iAllColumns) { | ||
| if ( | ||
| finalAllColumns.find( | ||
| (_c) => _c.model === c.model && _c.column === c.column, | ||
| ) | ||
| ) { | ||
| continue; | ||
| } | ||
| finalAllColumns.push(c); | ||
| } | ||
| setIsLoading(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate removal to improve performance
Using find within a loop to check for duplicates results in O(n²) time complexity. This can negatively impact performance with larger datasets.
Apply the following refactor to enhance performance:
- const finalAllColumns: DocsItem[] = [];
- for (const c of iAllColumns) {
- if (
- finalAllColumns.find(
- (_c) => _c.model === c.model && _c.column === c.column,
- )
- ) {
- continue;
- }
- finalAllColumns.push(c);
- }
+ const seenKeys = new Set<string>();
+ const finalAllColumns: DocsItem[] = [];
+ for (const c of iAllColumns) {
+ const key = `${c.model}/${c.column}`;
+ if (!seenKeys.has(key)) {
+ seenKeys.add(key);
+ finalAllColumns.push(c);
+ }
+ }📝 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.
| for (const c of iAllColumns) { | |
| if ( | |
| finalAllColumns.find( | |
| (_c) => _c.model === c.model && _c.column === c.column, | |
| ) | |
| ) { | |
| continue; | |
| } | |
| finalAllColumns.push(c); | |
| } | |
| setIsLoading(false); | |
| const seenKeys = new Set<string>(); | |
| const finalAllColumns: DocsItem[] = []; | |
| for (const c of iAllColumns) { | |
| const key = `${c.model}/${c.column}`; | |
| if (!seenKeys.has(key)) { | |
| seenKeys.add(key); | |
| finalAllColumns.push(c); | |
| } | |
| } | |
| setIsLoading(false); |
Overview
https://www.loom.com/share/b289e2b70e674eb39ff34341520d9ee5
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 changeImportant
Introduces documentation lineage propagation with
DbtLineageServiceand UI updates for column-level lineage handling.event_typetoDBTColumnLineageRequestinaltimate.ts.DbtLineageServiceto handle column-level lineage and documentation propagation.DocumentationPropagationButtoninDocGeneratorInput.tsxfor UI interaction.DbtLineageServiceclass for managing lineage operations, includinggetConnectedColumns()andhandleColumnLineage().DbtTestServiceto support fetching tests for models and columns.DocumentationPropagationButtoninDocGeneratorInput.tsxfor propagating documentation.styles.module.scssfor UI consistency.PropagateIcontoicons/index.tsxfor UI representation.NewLineagePanelandDocsEditViewPanelto integrate withDbtLineageService.This description was created by
for cc9f90a. It will automatically update as commits are pushed.
Summary by CodeRabbit
Release Notes
New Features
DbtLineageServicefor managing and retrieving column-level lineage information.DocumentationPropagationButtonfor propagating documentation across database columns.DbtTestServicefor retrieving tests based on the current model context.DocsEditViewPanelwith new commands for managing column lineage.event_typeto improve column lineage request handling.PropagateIconfor improved visual representation.Improvements
DocsEditViewPanel.DbtLineageService.DbtLineageService.Styles