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. |
| @@ -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); | |||
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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