feat: lineage refactor #1212
feat: lineage refactor #1212
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| window.addEventListener( | ||
| "message", | ||
| ( | ||
| event: MessageEvent<{ command: string; args: Record<string, unknown> }>, |
Check warning
Code scanning / CodeQL
Missing origin verification in `postMessage` handler Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 5 months ago
To fix the problem, we need to verify the origin of the incoming messages in the postMessage handler. This involves checking the event.origin property against a list of trusted origins before processing the message.
- Identify the trusted origins for your application.
- Modify the
postMessagehandler to include a check for theevent.origin. - If the origin is trusted, proceed with the existing logic; otherwise, ignore the message or log a warning.
| @@ -92,2 +92,3 @@ | ||
|
|
||
| const trustedOrigins = ['https://www.example.com', 'https://another-trusted-origin.com']; | ||
| window.addEventListener( | ||
| @@ -97,2 +98,6 @@ | ||
| ) => { | ||
| if (!trustedOrigins.includes(event.origin)) { | ||
| panelLogger.warn(`Untrusted origin: ${event.origin}`); | ||
| return; | ||
| } | ||
| panelLogger.log("lineage:message -> ", JSON.stringify(event.data)); |
| @@ -173,14 +173,6 @@ export class LineagePanel implements WebviewViewProvider, Disposable { | |||
| return; | |||
| } | |||
|
|
|||
| if (command === "openURL") { | |||
There was a problem hiding this comment.
this is handled via altimateWebviewProvider
| @@ -188,17 +188,6 @@ export class QueryResultPanel extends AltimateWebviewProvider { | |||
| ); | |||
| } | |||
|
|
|||
| private async checkIfWebviewReady() { | |||
There was a problem hiding this comment.
moved to altimateWebviewProvider
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/webview_provider/newLineagePanel.ts (2)
594-596: Proper error handling for cancellationThe code now checks for cancellation using
this.cancellationTokenSource!.token, which is a good practice. However, the use of the non-null assertion operator (!) might be risky ifcancellationTokenSourcecould be undefined.Consider adding a null check before using
cancellationTokenSourceto avoid potential runtime errors:if (this.cancellationTokenSource?.token) { // Use the token } else { // Handle the case where cancellationTokenSource is undefined }
913-915: Improved error handling in getMissingLineageMessageThe code now uses optional chaining (
?.) when callingthrowDiagnosticsErrorIfAvailable(), which is safer than the previous implementation. However, there's still room for improvement.Consider using nullish coalescing to provide a default error message:
try { this.queryManifestService.getProject()?.throwDiagnosticsErrorIfAvailable(); } catch (err) { return { message: (err as Error).message ?? "Unknown error occurred", type: "error" }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/webview_provider/newLineagePanel.ts (24 hunks)
🧰 Additional context used
🔇 Additional comments (9)
src/webview_provider/newLineagePanel.ts (9)
74-117: Improved class structure and dependency injectionThe
NewLineagePanelclass has been refactored to extendAltimateWebviewProviderand implementLineagePanelView. The constructor now accepts more dependencies, improving modularity and testability. The addition of a configuration change listener is a good practice for keeping the panel up-to-date with user settings.
165-171: Enhanced command handling with sync request supportThe
handleCommandmethod signature has been updated to include an optionalsyncRequestId. This allows for better tracking and correlation of asynchronous operations, improving the overall reliability of the command handling system.
181-181: Consistent use of syncRequestId in response messagesThe
syncRequestIdis now consistently included in the response messages sent back to the webview. This improvement ensures that the frontend can accurately match responses to their corresponding requests, enhancing the reliability of the communication between the extension and the webview.Also applies to: 190-190, 199-199, 208-208, 218-218, 247-247, 252-252
327-327: Proper delegation to parent classThe
handleCommandmethod now correctly delegates to the parent class implementation usingsuper.handleCommand(message)for unsupported commands. This ensures that any common command handling logic in the parent class is not bypassed.
576-579: Improved usage of event objectThe code now correctly uses
event.eventwhen callingDBTProject.getNonEphemeralParents, ensuring that the correct data is passed to the method. This change aligns with the updated structure of theeventobject.Also applies to: 582-583
619-621: Efficient bulk compilation of SQLThe code now uses
project.getBulkCompiledSqlto compile multiple SQL models at once, which is likely more efficient than compiling them individually. This is a good optimization for performance.
786-788: Improved null handling in createTable methodThe code now properly checks if
_nodeis undefined before attempting to use it, preventing potential runtime errors. This is a good defensive programming practice.
975-978: New method for checking if V2 is enabledThe addition of the
isV2Enabledmethod is a good practice for encapsulating the configuration check. This makes it easier to update the condition in the future if needed.
Line range hint
980-992: Improved renderWebviewView methodThe
renderWebviewViewmethod now checks if V2 is enabled and renders the appropriate HTML. This is a good way to handle feature flags and gradually roll out new features.To ensure that the V2 rendering is working as expected, we should verify that the configuration is being read correctly. Run the following script to check the configuration:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/webview_provider/newLineagePanel.ts (2)
Line range hint
149-311: Improved command handling with suggestion for further refactoringThe updates to the
handleCommandmethod, including the addition ofsyncRequestIdfor better response tracking, are good improvements. However, the method is handling many different commands, which could make it harder to maintain as the number of commands grows.Consider refactoring this method to use a command pattern or a map of command handlers to improve maintainability and make it easier to add new commands in the future.
Line range hint
524-740: Consider refactoringgetConnectedColumnsfor improved maintainabilityThe
getConnectedColumnsmethod is quite long and complex, handling multiple responsibilities. Consider breaking it down into smaller, more focused methods to improve readability and maintainability. This would make the code easier to understand, test, and modify in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/webview_provider/newLineagePanel.ts (23 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/webview_provider/newLineagePanel.ts (2)
73-101: Improved class structure and dependency injectionThe refactoring of
NewLineagePanelto extendAltimateWebviewProviderand the addition of new dependencies in the constructor enhance the modularity and maintainability of the code. This change aligns with best practices for dependency injection and separation of concerns.
959-963: Simplified webview renderingThe new
renderWebviewViewmethod effectively leverages the superclass implementation, promoting code reuse and maintaining a clear separation of concerns. This change simplifies the rendering process and makes the code more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/webview_provider/newLineagePanel.ts (3)
84-101: LGTM! Improved dependency injection. Consider using parameter properties for conciseness.The updated constructor properly injects the required services, which is excellent for managing dependencies and improving testability.
For improved readability and conciseness, consider using TypeScript's parameter properties. This can reduce boilerplate code. For example:
constructor( protected readonly dbtProjectContainer: DBTProjectContainer, private readonly altimate: AltimateRequest, protected readonly telemetry: TelemetryService, private readonly terminal: DBTTerminal, protected readonly eventEmitterService: SharedStateService, protected readonly queryManifestService: QueryManifestService, protected readonly usersService: UsersService ) { super( dbtProjectContainer, altimate, telemetry, eventEmitterService, terminal, queryManifestService, usersService ); }This change would eliminate the need for separate property declarations.
149-155: LGTM! Improved command handling with sync request tracking. Consider adding a default error handler.The changes to the
handleCommandmethod are good:
- The addition of
syncRequestIdimproves request-response tracking.- The call to
super.handleCommand(message)ensures proper inheritance behavior.Consider adding a default error handler for unhandled commands before calling the super method. This could improve error reporting and debugging. For example:
if (!handled) { console.warn(`Unhandled command: ${command}`); // Optionally, send an error response to the webview this._panel?.webview.postMessage({ command: "response", args: { id, syncRequestId, error: "Unhandled command", status: false }, }); } super.handleCommand(message);This addition would help catch and report any commands that aren't explicitly handled in this class or its superclass.
Also applies to: 165-165, 174-174, 183-183, 192-192, 202-202, 231-231, 236-236, 277-277, 298-298, 311-311
897-899: LGTM! Improved null safety. Consider enhancing error handling.The changes in
getMissingLineageMessageare good:
- The use of
queryManifestServiceis consistent with the overall refactoring.- The null-safe call operator
?.prevents potential null reference errors.Consider enhancing the error handling to provide more specific error messages. For example:
try { const project = this.queryManifestService.getProject(); if (!project) { return { message: "No active dbt project found", type: "error" }; } project.throwDiagnosticsErrorIfAvailable(); } catch (err) { return { message: `Error in dbt project: ${(err as Error).message}`, type: "error" }; }This change would provide more context in the error message, making it easier for users to understand and troubleshoot issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/webview_provider/newLineagePanel.ts (24 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/webview_provider/newLineagePanel.ts (3)
32-35: LGTM! Improved modularity and service-oriented architecture.The changes to the import statements and class declaration show a shift towards a more modular and service-oriented architecture. This is a positive change as it:
- Improves maintainability by separating concerns.
- Enhances testability by allowing easier mocking of dependencies.
- Promotes code reuse through the use of shared services.
Also applies to: 73-76
Line range hint
1-963: Overall, great improvements to modularity and maintainability!The refactoring of
NewLineagePanelrepresents a significant improvement to the codebase:
- The shift towards a service-oriented architecture enhances modularity and testability.
- Improved error handling and null checks increase robustness.
- Consistent use of
queryManifestServicecentralizes data retrieval logic.While there are some areas for potential further improvement (as noted in previous comments), these changes lay a solid foundation for easier maintenance and future enhancements.
959-963: LGTM! Simplified rendering. Verify webview script execution.The simplification of
renderWebviewViewis good:
- It reduces code duplication by using the superclass's
getHtmlmethod.- It makes the code more maintainable and consistent with the base class implementation.
Please verify that removing the
webview.options = { enableScripts: true }setting doesn't affect the functionality of the webview. If scripts are required in the webview, ensure that this setting is properly handled in the superclass or add it back if necessary.Run the following script to check if
enableScriptsis set in the superclass:If the script doesn't find any results, consider adding the
enableScriptsoption back to this method.✅ Verification successful
Verified:
enableScriptsis enabled in the superclass.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if enableScripts is set in AltimateWebviewProvider # Test: Search for enableScripts in AltimateWebviewProvider rg -A 5 'enableScripts' src/webview_provider/altimateWebviewProvider.tsLength of output: 253
| const event = this.queryManifestService.getEventByCurrentProject(); | ||
| if (!event?.event) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider abstracting repeated null checks into a helper method.
The use of queryManifestService to retrieve project and event data is a good practice. However, the repeated null checks for event and project in multiple methods introduce code duplication.
Consider creating a helper method to perform these checks, which would reduce code duplication and improve maintainability. For example:
private getEventAndProject(): { event: ManifestCacheProjectAddedEvent['event'], project: DBTProject } | undefined {
const event = this.queryManifestService.getEventByCurrentProject();
if (!event?.event) {
return undefined;
}
const project = this.queryManifestService.getProject();
if (!project) {
return undefined;
}
return { event: event.event, project };
}You can then use this helper method in place of the repeated checks throughout the class:
const eventAndProject = this.getEventAndProject();
if (!eventAndProject) {
return;
}
const { event, project } = eventAndProject;This refactoring would make the code more DRY and easier to maintain.
Also applies to: 382-382, 412-413, 416-416, 541-542, 545-545
| const event = this.queryManifestService.getEventByCurrentProject(); | ||
| if (!event?.event) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider further simplification of table-related methods.
The changes to use queryManifestService in getConnectedTables and createTable are consistent with the overall refactoring. However, these methods still contain complex logic that could potentially be simplified.
Consider the following improvements:
-
Extract the logic for creating different types of tables (source, metric, exposure, etc.) into separate methods or a factory class. This would make the
createTablemethod more readable and easier to maintain. -
Use early returns in
getConnectedTablesto reduce nesting and improve readability. For example:
private getConnectedTables(key: keyof GraphMetaMap, table: string): Table[] | undefined {
const event = this.queryManifestService.getEventByCurrentProject();
if (!event?.event) return;
const { graphMetaMap } = event.event;
const dependencyNodes = graphMetaMap[key];
const node = dependencyNodes.get(table);
if (!node) return;
const tables = new Map<string, Table>();
node.nodes.forEach(({ url, key }) => {
const _node = this.createTable(event.event!, url, key);
if (_node && !tables.has(_node.table)) {
tables.set(_node.table, _node);
}
});
return Array.from(tables.values()).sort((a, b) => a.table.localeCompare(b.table));
}These changes would improve the overall structure and readability of the code.
Also applies to: 762-762, 770-772
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
webview_panels/src/modules/lineage/LineageView.tsx (1)
38-38: Remove debugging log statementsThere are log statements with the message "thisisiit" that appear to be left over from debugging. To maintain clean and readable code, consider removing or updating these log statements.
Apply this diff to clean up the log statements:
- panelLogger.log("thisisiit", "render"); - panelLogger.log("thisisiit", "ApiHelper.get", url, data); - panelLogger.log("thisisiit", renderNode);Also applies to: 48-48, 122-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- webview_panels/src/modules/lineage/LineageView.tsx (1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
webview_panels/src/modules/lineage/LineageView.tsx
[warning] 98-98: Missing origin verification in
postMessagehandler
Postmessage handler has no origin check.
| } | ||
|
|
||
| // const lineageType = renderNode.details ? "sql" : "dynamic"; | ||
| const lineageType = "dynamic" as "dynamic" | "sql"; |
There was a problem hiding this comment.
Determine lineage type dynamically instead of hardcoding
Currently, lineageType is hardcoded to "dynamic". To ensure the component behaves correctly in all scenarios, determine the lineage type based on the available data.
Apply this diff to set lineageType dynamically:
- const lineageType = "dynamic" as "dynamic" | "sql";
+ const lineageType = renderNode.details ? "sql" : "dynamic";This change uses the presence of renderNode.details to decide the lineage type.
Committable suggestion was skipped due to low confidence.
| if (!event.origin.startsWith("vscode-webview://")) { | ||
| panelLogger.debug("invalid message ", event); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve origin verification in message event handler
The current origin check using event.origin.startsWith("vscode-webview://") may not be reliable, as event.origin can be an empty string in VSCode webviews. Consider verifying the message source by checking event.source or implementing a more robust message validation mechanism.
Apply this diff to enhance security:
- if (!event.origin.startsWith("vscode-webview://")) {
+ if (event.source !== window.parent) {
panelLogger.debug("invalid message ", event);
return;
}Ensure that event.source correctly identifies messages from the expected source.
📝 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 (!event.origin.startsWith("vscode-webview://")) { | |
| panelLogger.debug("invalid message ", event); | |
| return; | |
| } | |
| if (event.source !== window.parent) { | |
| panelLogger.debug("invalid message ", event); | |
| return; | |
| } |
| // @ts-expect-error TODO: add type generic for executeRequestInSync | ||
| ApiHelper.get = async (url: string, data?: Record<string, unknown>) => { | ||
| panelLogger.log("thisisiit", "ApiHelper.get", url, data); | ||
| switch (url) { | ||
| case "init": { | ||
| const event = new CustomEvent("renderStartNode", { | ||
| detail: _data, | ||
| }); | ||
| document.dispatchEvent(event); | ||
| return; | ||
| } | ||
| case "upstreamTables": | ||
| case "downstreamTables": | ||
| case "getExposureDetails": | ||
| case "getColumns": | ||
| case "getConnectedColumns": | ||
| case "sendFeedback": | ||
| case "getLineageSettings": | ||
| case "persistLineageSettings": | ||
| case "openFile": | ||
| case "openChat": | ||
| case "showInfoNotification": | ||
| case "previewFeature": | ||
| case "telemetryEvents": | ||
| return executeRequestInSync(url, { args: { params: data ?? {} } }); | ||
| case "columnLineage": | ||
| return executeRequestInSync(url, { args: data }); | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Avoid overwriting imported module methods directly
Overwriting ApiHelper.get directly can lead to unexpected behavior and makes the code harder to maintain and test. Consider using dependency injection or a wrapper function to achieve the desired behavior without modifying the imported module.
Apply this diff to refactor the code:
- // @ts-expect-error TODO: add type generic for executeRequestInSync
- ApiHelper.get = async (url: string, data?: Record<string, unknown>) => {
- // Implementation
- };
+ // Consider creating a custom API helper or passing necessary functions as propsCommittable suggestion was skipped due to low confidence.
| useEffect(() => { | ||
| const commandMap = { | ||
| render, | ||
| columnLineage: (data: { event: CllEvents }) => { | ||
| columnLineage(data); | ||
| }, | ||
| }; | ||
|
|
||
| window.addEventListener( | ||
| "message", | ||
| ( | ||
| event: MessageEvent<{ command: string; args: Record<string, unknown> }>, | ||
| ) => { | ||
| if (!event.origin.startsWith("vscode-webview://")) { | ||
| panelLogger.debug("invalid message ", event); | ||
| return; | ||
| } | ||
|
|
||
| panelLogger.log("lineage:message -> ", JSON.stringify(event.data)); | ||
| const { command, args } = event.data; | ||
|
|
||
| if (command in commandMap) { | ||
| ( | ||
| commandMap[command as keyof typeof commandMap] as ( | ||
| args: Record<string, unknown>, | ||
| ) => void | ||
| )(args); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| panelLogger.info("lineage:onload"); | ||
| document.documentElement.classList.add(styles.lineageBody); | ||
| executeRequestInAsync("init", {}); | ||
| }, []); |
There was a problem hiding this comment.
Add cleanup for event listeners in useEffect
The useEffect hook adds an event listener to the window but does not remove it upon component unmounting, which can lead to memory leaks or unwanted behavior. Return a cleanup function to remove the event listener when the component unmounts.
Apply this diff to ensure proper cleanup:
useEffect(() => {
const commandMap = {
render,
columnLineage: (data: { event: CllEvents }) => {
columnLineage(data);
},
};
+ const messageHandler = (
+ event: MessageEvent<{ command: string; args: Record<string, unknown> }>,
+ ) => {
+ // Existing event handler code
+ };
+
- window.addEventListener(
- "message",
- (
- event: MessageEvent<{ command: string; args: Record<string, unknown> }>,
- ) => {
- // Existing event handler code
- },
- );
+ window.addEventListener("message", messageHandler);
panelLogger.info("lineage:onload");
document.documentElement.classList.add(styles.lineageBody);
executeRequestInAsync("init", {});
+ return () => {
+ window.removeEventListener("message", messageHandler);
+ };
}, []);📝 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.
| useEffect(() => { | |
| const commandMap = { | |
| render, | |
| columnLineage: (data: { event: CllEvents }) => { | |
| columnLineage(data); | |
| }, | |
| }; | |
| window.addEventListener( | |
| "message", | |
| ( | |
| event: MessageEvent<{ command: string; args: Record<string, unknown> }>, | |
| ) => { | |
| if (!event.origin.startsWith("vscode-webview://")) { | |
| panelLogger.debug("invalid message ", event); | |
| return; | |
| } | |
| panelLogger.log("lineage:message -> ", JSON.stringify(event.data)); | |
| const { command, args } = event.data; | |
| if (command in commandMap) { | |
| ( | |
| commandMap[command as keyof typeof commandMap] as ( | |
| args: Record<string, unknown>, | |
| ) => void | |
| )(args); | |
| } | |
| }, | |
| ); | |
| panelLogger.info("lineage:onload"); | |
| document.documentElement.classList.add(styles.lineageBody); | |
| executeRequestInAsync("init", {}); | |
| }, []); | |
| useEffect(() => { | |
| const commandMap = { | |
| render, | |
| columnLineage: (data: { event: CllEvents }) => { | |
| columnLineage(data); | |
| }, | |
| }; | |
| const messageHandler = ( | |
| event: MessageEvent<{ command: string; args: Record<string, unknown> }>, | |
| ) => { | |
| if (!event.origin.startsWith("vscode-webview://")) { | |
| panelLogger.debug("invalid message ", event); | |
| return; | |
| } | |
| panelLogger.log("lineage:message -> ", JSON.stringify(event.data)); | |
| const { command, args } = event.data; | |
| if (command in commandMap) { | |
| ( | |
| commandMap[command as keyof typeof commandMap] as ( | |
| args: Record<string, unknown>, | |
| ) => void | |
| )(args); | |
| } | |
| }; | |
| window.addEventListener("message", messageHandler); | |
| panelLogger.info("lineage:onload"); | |
| document.documentElement.classList.add(styles.lineageBody); | |
| executeRequestInAsync("init", {}); | |
| return () => { | |
| window.removeEventListener("message", messageHandler); | |
| }; | |
| }, []); |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 98-98: Missing origin verification in
postMessagehandler
Postmessage handler has no origin check.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- webview_panels/src/modules/lineage/LineageView.tsx (1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
webview_panels/src/modules/lineage/LineageView.tsx
[warning] 96-96: Missing origin verification in
postMessagehandler
Postmessage handler has no origin check.
| // @ts-expect-error TODO: add type generic for executeRequestInSync | ||
| ApiHelper.get = async (url: string, data?: Record<string, unknown>) => { |
There was a problem hiding this comment.
Resolve TypeScript errors instead of suppressing them
The @ts-expect-error directive suppresses a TypeScript error. It's better to properly type executeRequestInSync to ensure type safety and prevent potential runtime issues.
Update the type definitions to include generic types for executeRequestInSync, eliminating the need for the @ts-expect-error comment.
| ( | ||
| event: MessageEvent<{ command: string; args: Record<string, unknown> }>, | ||
| ) => { | ||
| panelLogger.log("lineage:message -> ", JSON.stringify(event.data)); |
There was a problem hiding this comment.
Sanitize user-provided data before logging
Logging user-provided data directly can introduce security risks like log injection. Consider sanitizing or validating event.data before logging.
Update the logging statement to ensure safe logging practices:
- panelLogger.log("lineage:message -> ", JSON.stringify(event.data));
+ const sanitizedData = sanitizeEventData(event.data);
+ panelLogger.log("lineage:message -> ", JSON.stringify(sanitizedData));Implement sanitizeEventData to properly sanitize the data before logging.
Committable suggestion was skipped due to low confidence.
| // @ts-expect-error TODO: add type generic for executeRequestInSync | ||
| ApiHelper.get = async (url: string, data?: Record<string, unknown>) => { | ||
| switch (url) { | ||
| case "upstreamTables": | ||
| case "downstreamTables": | ||
| case "getExposureDetails": | ||
| case "getColumns": | ||
| case "getConnectedColumns": | ||
| case "sendFeedback": | ||
| case "getLineageSettings": | ||
| case "persistLineageSettings": | ||
| case "init": | ||
| case "openFile": | ||
| case "openChat": | ||
| case "showInfoNotification": | ||
| case "previewFeature": | ||
| case "telemetryEvents": | ||
| return executeRequestInSync(url, { args: { params: data ?? {} } }); | ||
| case "columnLineage": | ||
| return executeRequestInSync(url, { args: data }); | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid overwriting imported module methods directly
Overwriting ApiHelper.get can lead to unexpected behavior and make the code harder to maintain and test. Consider using a custom API helper or dependency injection instead of modifying the imported module.
Create a custom API helper instance:
- // @ts-expect-error TODO: add type generic for executeRequestInSync
- ApiHelper.get = async (url: string, data?: Record<string, unknown>) => {
- // Implementation
- };
+ const customApiHelper = {
+ get: async (url: string, data?: Record<string, unknown>) => {
+ // Implementation
+ },
+ };Then use customApiHelper.get instead of ApiHelper.get in your component.
📝 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.
| // @ts-expect-error TODO: add type generic for executeRequestInSync | |
| ApiHelper.get = async (url: string, data?: Record<string, unknown>) => { | |
| switch (url) { | |
| case "upstreamTables": | |
| case "downstreamTables": | |
| case "getExposureDetails": | |
| case "getColumns": | |
| case "getConnectedColumns": | |
| case "sendFeedback": | |
| case "getLineageSettings": | |
| case "persistLineageSettings": | |
| case "init": | |
| case "openFile": | |
| case "openChat": | |
| case "showInfoNotification": | |
| case "previewFeature": | |
| case "telemetryEvents": | |
| return executeRequestInSync(url, { args: { params: data ?? {} } }); | |
| case "columnLineage": | |
| return executeRequestInSync(url, { args: data }); | |
| default: | |
| break; | |
| } | |
| }; | |
| const customApiHelper = { | |
| get: async (url: string, data?: Record<string, unknown>) => { | |
| switch (url) { | |
| case "upstreamTables": | |
| case "downstreamTables": | |
| case "getExposureDetails": | |
| case "getColumns": | |
| case "getConnectedColumns": | |
| case "sendFeedback": | |
| case "getLineageSettings": | |
| case "persistLineageSettings": | |
| case "init": | |
| case "openFile": | |
| case "openChat": | |
| case "showInfoNotification": | |
| case "previewFeature": | |
| case "telemetryEvents": | |
| return executeRequestInSync(url, { args: { params: data ?? {} } }); | |
| case "columnLineage": | |
| return executeRequestInSync(url, { args: data }); | |
| default: | |
| break; | |
| } | |
| }, | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/webview_provider/altimateWebviewProvider.ts (3)
640-640: Add validation for viewPathWhile the implementation works, it would be safer to validate the viewPath format before using it as a class name.
Consider sanitizing the viewPath:
-<body class="${this.viewPath.replace(/\//g, "")}"> +<body class="${this.viewPath.replace(/[^a-zA-Z0-9-_]/g, "")}">This ensures only alphanumeric characters, hyphens, and underscores are used in class names, preventing potential CSS injection or invalid class names.
643-643: Enhance modal accessibilityThe modal div should include ARIA attributes for better accessibility.
Consider adding ARIA attributes:
-<div id="modal"></div> +<div id="modal" role="dialog" aria-modal="true"></div>
647-647: Improve type safety for window object extensionThe lineageGif variable is added to the window object without proper TypeScript type definitions.
Consider adding type definitions:
// Add to a separate .d.ts file or at the top of this file declare global { interface Window { viewPath: string; spinnerUrl: string; lineageGif: string; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/webview_provider/altimateWebviewProvider.ts (3 hunks)
🔇 Additional comments (1)
src/webview_provider/altimateWebviewProvider.ts (1)
526-535: Optimize webview ready check implementationThe current implementation has potential issues:
- Uses
setIntervalwhich is less efficient than a recursivesetTimeoutapproach- No timeout mechanism to prevent infinite polling
The previous review comment suggesting improvements to this implementation is still valid. Please refer to the existing comment for the recommended implementation using
setTimeoutwith a timeout mechanism.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
webview_panels/src/lib/altimate/altimate-components.js (2)
1-1: Consider improving import naming for better maintainability.The extensive use of single-letter aliases (
o,t,n, etc.) in imports makes the code harder to maintain and understand. Consider using more descriptive names that reflect the actual component or utility being imported.Example of a more maintainable import:
- import { A as o, B as t, m as n, p as i, o as r, n as m, r as C, t as T, C as l, E as c, k as p, i as g, h as v, D as L, I as u, x as h, l as B, L as M, P as d, F as A, J as b, H as k, q as y, y as F, z as P, w as x, T as D, G as I, v as S } from "./main.js"; + import { + ApiHelperComponent as ApiHelper, + BadgeComponent as Badge, + CLLComponent as CLL, + // ... other imports with meaningful names + } from "./main.js";
1-32: Update README.md with component changes.As mentioned in the PR checklist, the README.md hasn't been updated. Given the significant component renaming and restructuring, documentation should be updated to reflect these changes.
Would you like me to help create a documentation section that covers:
- The new component naming scheme
- Any breaking changes in the exports
- Migration guide for existing code
webview_panels/src/lib/altimate/altimate-components.d.ts (1)
353-356: Consider documenting the status types for onStatusUpdateThe Props_9 interface changes look good, adding proper type safety for status updates and follow-up requests. Consider documenting the possible values for the status
typestring to help implementers.Add JSDoc comments to document the possible status types:
/** Props for the Chatbot component */ interface Props_9 extends ProChatProps<any> { /** Callback for handling chat requests * @param messages The chat messages to process * @param sessionId The current session identifier * @param onStatusUpdate Callback for status updates * @param onStatusUpdate.type The type of status update (e.g., 'processing', 'error', 'complete') * @param onStatusUpdate.message A human-readable status message */ onRequest: (messages: ChatMessage[], sessionId: string, onStatusUpdate: (info: { type: string; message: string; }) => void) => any; /** Optional callback for handling follow-up requests * @param sessionId The current session identifier * @returns Promise resolving to an array of follow-up suggestions or undefined */ onFollowupRequest?: (sessionId: string) => Promise<string[] | undefined>; }Also applies to: 358-358
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- webview_panels/src/lib/altimate/altimate-components.d.ts (2 hunks)
- webview_panels/src/lib/altimate/altimate-components.js (1 hunks)
🔇 Additional comments (3)
webview_panels/src/lib/altimate/altimate-components.js (1)
3-32: Verify the impact of renamed exports on dependent code.The extensive renaming of exports (e.g.,
ChatbottoChatTriggerLink,CllEventstoChatbot) could potentially break existing code that imports these components.Let's verify the usage of these renamed exports:
webview_panels/src/lib/altimate/altimate-components.d.ts (2)
22-22: LGTM: Well-structured Chatbot component declarationThe updated Chatbot component declaration with the optional
onFollowupRequestparameter follows TypeScript best practices.
24-26: Verify the integration of ChatTriggerLink with lineage componentsThe ChatTriggerLink component declaration looks good. Since this is part of the lineage refactoring, let's verify its integration.
| import { A as o, B as t, m as n, p as i, o as r, n as m, r as C, t as T, C as l, E as c, k as p, i as g, h as v, D as L, I as u, x as h, l as B, L as M, P as d, F as A, J as b, H as k, q as y, y as F, z as P, w as x, T as D, G as I, v as S } from "./main.js"; | ||
| import "reactstrap"; | ||
| export { | ||
| o as ApiHelper, | ||
| t as Badge, | ||
| n as CLL, | ||
| i as Chatbot, | ||
| r as CllEvents, | ||
| m as CoachForm, | ||
| C as CoachFormButton, | ||
| i as ChatTriggerLink, | ||
| r as Chatbot, | ||
| m as CllEvents, | ||
| C as CoachForm, | ||
| T as CoachFormButton, | ||
| l as CodeBlock, | ||
| T as ContentCategory, | ||
| c as ConversationGroupProvider, | ||
| p as ConversationInputForm, | ||
| c as ContentCategory, | ||
| p as ConversationGroupProvider, | ||
| g as ConversationInputForm, | ||
| v as ConversationSources, | ||
| g as DbtDocs, | ||
| L as DbtDocs, | ||
| u as IconButton, | ||
| L as Learnings, | ||
| h as Learnings, | ||
| B as Lineage, | ||
| M as LoadingButton, | ||
| d as PersonalizationScope, | ||
| h as TaskLabels, | ||
| A as TeamMateActionType, | ||
| b as TeamMateAvailability, | ||
| A as TaskLabels, | ||
| b as TeamMateActionType, | ||
| k as TeamMateAvailability, | ||
| y as TeamMateProvider, | ||
| F as TeamMates, | ||
| P as TeamMatesConfig, | ||
| k as TeammateActions, | ||
| x as Tooltip, | ||
| D as learningSchema, | ||
| I as useTeamMateContext | ||
| x as TeammateActions, | ||
| D as Tooltip, | ||
| I as learningSchema, | ||
| S as useTeamMateContext |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider grouping related exports for better organization.
The exports appear to be alphabetically ordered but could benefit from logical grouping based on functionality (e.g., Chat-related, Team-related, UI components).
Example organization:
// Chat & Communication
export { ChatTriggerLink, Chatbot, CllEvents };
// Coaching & Learning
export { CoachForm, CoachFormButton, DbtDocs, Learnings, learningSchema };
// Team Management
export {
TeamMateActionType,
TeamMateAvailability,
TeamMateProvider,
TeamMates,
TeamMatesConfig,
TeammateActions,
useTeamMateContext
};
// UI Components
export { Badge, CodeBlock, IconButton, LoadingButton, Tooltip };
// Utilities
export { ApiHelper, ConversationGroupProvider };
Overview
Problem
Extract lineage component for better maintainability
Solution
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
dbt.enableLineageV2as true in settings.jsonChecklist
README.mdupdated and added information about my changeSummary by CodeRabbit
Release Notes
New Features
LineageDemocomponent for rendering a GIF in the icon set..modal-contentCSS class for improved styling in the webview.DemoButtoncomponent for displaying a demo modal with a close button.LineageViewcomponent for managing lineage data display and interactions.Improvements
NewLineagePanelandSQLLineagePanel.QueryResultPanelduring query execution.Chatbotcomponent for better follow-up request handling.Refactor
NewLineagePanelandSQLLineagePanelclasses for better modularity and service integration.Bug Fixes