Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: notebook issues #1420

Merged
merged 8 commits into from
Sep 20, 2024
Merged

fix: notebook issues #1420

merged 8 commits into from
Sep 20, 2024

Conversation

saravmajestic
Copy link
Collaborator

@saravmajestic saravmajestic commented Sep 18, 2024

Overview

Problem

https://www.notion.so/altimate/Notebook-Testing-10341b892926807d9be9ec544b246260

Solution

Fixes added for the issues mentioned in notion doc

Screenshot/Demo

A picture is worth a thousand words. Please highlight the changes if applicable.

How to test

  • Steps to be followed to verify the solution or code changes
  • Mention if there is any settings configuration added/changed/deleted

Checklist

  • I have run this code and it appears to resolve the stated issue
  • README.md updated and added information about my change

Important

This PR enhances notebook management by adding delete and update functionalities, updating UI components, and modifying backend logic to support these operations.

  • Backend Changes:
    • Add deleteNotebook and updateNotebook methods in altimateWebviewProvider.ts to handle notebook deletion and updates.
    • Modify NotebookFileSystemProvider to emit file change events on notebook creation.
    • Update DatapilotNotebookController to manage notebook execution and context.
  • Frontend Changes:
    • Add DeleteNotebookButton.tsx for deleting notebooks with confirmation.
    • Update NotebooksList.tsx to support editing notebook names and deleting notebooks.
    • Modify Notebooks.tsx to handle refetching notebooks on certain events.
  • UI/UX:
    • Update styles in notebooklist.module.scss for better UI consistency.
    • Adjust renderer.tsx to set a fixed height for the perspective viewer.

This description was created by Ellipsis for a15b527. It will automatically update as commits are pushed.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced functionality to add, update, and delete notebooks via a more flexible interface.
    • Added a webview feature for users to delete and update notebooks directly.
    • Implemented a new button component for notebook deletion with user confirmation.
  • Enhancements

    • Improved responsiveness of the insights panel to file changes.
    • Enhanced notebook list interactivity with real-time updates and editing capabilities.
    • Streamlined visual presentation of notebook names and buttons.
  • Bug Fixes

    • Addressed potential race conditions in code lens provider registration.
  • Style

    • Updated styling for notebook name presentation to enhance readability.

@saravmajestic saravmajestic self-assigned this Sep 18, 2024
Copy link
Contributor

coderabbitai bot commented Sep 18, 2024

Walkthrough

The changes introduced in this pull request significantly enhance the notebook management system by modifying interface definitions, method signatures, and adding new functionalities. Key updates include the renaming of the NotebookRequest interface to AddNotebookRequest, the introduction of the UpdateNotebookRequest interface, and the addition of methods for deleting notebooks. The code lens provider's registration logic has been improved to prevent race conditions, while various components have been updated to support these new functionalities, including the addition of a React component for deleting notebooks.

Changes

Files Change Summary
src/altimate.ts - Renamed NotebookRequest to AddNotebookRequest and made data optional.
- Added UpdateNotebookRequest interface for updating notebooks.
- Updated addNotebook and updateNotebook methods to use new request types.
- Added deleteNotebook method.
src/code_lens_provider/index.ts - Modified initialization logic for code lens providers to register after DBT projects are initialized, preventing race conditions.
src/lib/index.d.ts - Added new properties and methods in DatapilotNotebookController and NotebookFileSystemProvider to enhance notebook management.
src/webview_provider/altimateWebviewProvider.ts - Added command cases for deleteNotebook and updateNotebook to handle notebook management from the webview.
src/webview_provider/insightsPanel.ts - Introduced an event listener for file creation to trigger notebook refetching in the InsightsPanel.
webview_panels/src/modules/notebooks/DeleteNotebookButton.tsx - Introduced a React component for deleting notebooks with confirmation prompts and callbacks.
webview_panels/src/modules/notebooks/Notebooks.tsx - Added message handling for refetching notebooks and updated props for NotebooksList.
webview_panels/src/modules/notebooks/NotebooksList.tsx - Enhanced editing functionalities for notebook names and adjusted visibility based on privacy settings.
webview_panels/src/modules/notebooks/notebooklist.module.scss - Updated styles for .notebookName to improve visual presentation.
webview_panels/src/notebook/renderer.tsx - Removed dynamic height calculation for PerspectiveViewer, replacing it with fixed height properties.

Possibly related PRs

  • chore: add debug info for query blank issue #1406: The changes in this PR involve the addition of a deleteNotebook command in the altimateWebviewProvider.ts, which is directly related to the new deleteNotebook method introduced in the main PR. Both PRs enhance the functionality for managing notebooks, specifically focusing on deletion capabilities.

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@saravmajestic saravmajestic marked this pull request as ready for review September 20, 2024 04:16
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to a15b527 in 1 minute and 38 seconds

More details
  • Looked at 3621 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. webview_panels/src/modules/notebooks/NotebooksList.tsx:48
  • Draft comment:
    Ensure that the loading state is updated and the user is informed if an error occurs during fetchNotebooks.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. webview_panels/src/modules/notebooks/NotebooksList.tsx:78
  • Draft comment:
    Consider adding error handling for the updateNotebook request to inform the user if the update fails.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is relevant to the changes made in the diff, specifically the new handleSaveNotebookName function. The function currently lacks user-facing error handling, which could be beneficial for user experience. The comment is actionable and suggests a clear improvement.
    The comment does not specify how to implement the error handling, which might be necessary for a more actionable suggestion. However, it does highlight a valid issue that could improve the code.
    While the comment could be more specific, it still points out a legitimate area for improvement that aligns with best practices for user feedback.
    The comment is relevant and suggests a valid improvement to the new code. It should be kept as it highlights a potential enhancement in error handling.
3. webview_panels/src/modules/notebooks/NotebooksList.tsx:58
  • Draft comment:
    Consider adding error handling for the openNewNotebook request to inform the user if the operation fails.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_ApVuq0iyqf4bTgbT


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.

items: ["Yes", "No"],
});
if (result === "Yes") {
await executeRequestInSync("deleteNotebook", {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling for the deleteNotebook request to inform the user if the deletion fails.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a6d0fdf and a15b527.

Files selected for processing (10)
  • src/altimate.ts (2 hunks)
  • src/code_lens_provider/index.ts (1 hunks)
  • src/lib/index.d.ts (5 hunks)
  • src/webview_provider/altimateWebviewProvider.ts (2 hunks)
  • src/webview_provider/insightsPanel.ts (4 hunks)
  • webview_panels/src/modules/notebooks/DeleteNotebookButton.tsx (1 hunks)
  • webview_panels/src/modules/notebooks/Notebooks.tsx (1 hunks)
  • webview_panels/src/modules/notebooks/NotebooksList.tsx (5 hunks)
  • webview_panels/src/modules/notebooks/notebooklist.module.scss (1 hunks)
  • webview_panels/src/notebook/renderer.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
  • webview_panels/src/modules/notebooks/notebooklist.module.scss
Additional comments not posted (29)
webview_panels/src/modules/notebooks/DeleteNotebookButton.tsx (1)

1-31: LGTM! The DeleteNotebookButton component is well-designed and aligns with the PR objectives.

The component provides a clear and interactive method for deleting notebooks, with the following strengths:

  • It prompts the user for confirmation before deleting the notebook, which helps prevent accidental deletions.
  • It uses executeRequestInSync for synchronous request execution, which is appropriate for this use case.
  • The component's prop types and naming are clear and descriptive, making it easy to understand and integrate into the parent component.
  • The afterDelete callback allows the parent component to handle any necessary updates or state changes after a successful deletion.

Overall, this component enhances the user experience and ensures that users confirm their actions before proceeding.

webview_panels/src/modules/notebooks/Notebooks.tsx (6)

1-4: LGTM!

The new imports are correctly added and necessary for the functionality introduced in this file.


8-8: LGTM!

The refetch state variable is correctly initialized with the current timestamp to trigger an initial fetch of the notebooks data.


10-22: LGTM!

The onMesssage callback function correctly handles the "refetchNotebooks" command and updates the refetch state. The use of useCallback with an empty dependency array is appropriate to memoize the callback.


24-30: LGTM!

The useEffect hook correctly sets up and cleans up the event listener for the onMesssage callback. Including the onMesssage callback in the dependency array ensures that the effect is re-run if the callback changes.


32-34: LGTM!

The refetchNotebook function correctly updates the refetch state with the current timestamp. Passing this function as a prop to the NotebooksList components allows them to trigger a refetch when needed.


36-42: LGTM!

The changes in this code segment are correctly implemented:

  • Logging the refetch action with the timestamp helps in debugging and understanding when the refetches occur.
  • The NotebooksList components are updated to accept the refetchTime and refetchNotebook props, allowing them to respond to refetch commands and update their data accordingly.
  • The refetch state and refetchNotebook function are correctly passed as props to each NotebooksList component.
webview_panels/src/modules/notebooks/NotebooksList.tsx (10)

3-13: LGTM!

The additional imports are necessary for the new functionality and there are no unused imports.


19-20: LGTM!

The additional icon imports are necessary for the new edit functionality and there are no unused imports.


25-26: LGTM!

The new props in the NotebookListProps interface are used to trigger a refresh of the notebook list and the prop names and types are appropriate.


29-34: LGTM!

The NotebooksList component correctly accepts the new props defined in the NotebookListProps interface and the prop names match the interface definition.


38-40: LGTM!

The new state variables editNotebook and newNotebookName are used to track the notebook being edited and hold the input for the new notebook name. The types for both state variables are appropriate.


Line range hint 45-55: LGTM!

The fetchNotebooks function is correctly updated to depend on refetchTime and the useEffect hook is updated to call fetchNotebooks whenever refetchTime changes. This ensures that the notebook list is refreshed when refetchTime changes.


66-102: LGTM!

The new handleSaveNotebookName function correctly updates the notebook's metadata with the new name, updates the state by mapping over the notebooks array, and resets the editNotebook and newNotebookName state variables after a successful update.


104-115: LGTM!

The new onKeyPress and handleNotebookNameChange functions correctly handle saving the notebook name on pressing "Enter", canceling the edit on pressing "Escape", and updating the newNotebookName state variable with the input value.


117-121: LGTM!

The new afterDelete function correctly filters out the deleted notebook from the notebooks array and updates the state after a notebook is deleted.


Line range hint 158-223: LGTM!

The rendering logic changes correctly:

  • Display an input field for the notebook name when a notebook is being edited.
  • Display the edit button only for private notebooks.
  • Ensure that the edit and delete functionalities are only available for private notebooks.
  • Render the Tag component only for draft notebooks.
src/webview_provider/altimateWebviewProvider.ts (2)

238-249: LGTM!

The code segment correctly implements the deleteNotebook functionality by invoking the deleteNotebook method on the altimateRequest object with the notebookId parameter. The operation is appropriately wrapped in a call to handleSyncRequestFromWebview to ensure synchronous processing with the webview.

This addition enhances the interactivity of the webview by allowing users to delete notebooks directly.


250-267: LGTM!

The code segment correctly implements the updateNotebook functionality by extracting the notebookId, name, and optional data from the params object and passing them to the updateNotebook method of the altimateRequest object. The operation is appropriately wrapped in a call to handleSyncRequestFromWebview to ensure synchronous processing with the webview.

This addition enhances the interactivity of the webview by allowing users to update notebook details directly.

src/webview_provider/insightsPanel.ts (2)

90-90: [Approved] The constructor change is syntactically correct, but more context is needed.

The addition of the NotebookFileSystemProvider dependency to the constructor follows the existing pattern correctly. However, the purpose and usage of this new dependency are not clear from this change alone.

To better understand the impact of this change, please provide more context on how the NotebookFileSystemProvider is being used within the InsightsPanel class and why it was added as a dependency.


137-151: [Approved] The new event listener enhances the functionality of the insights panel.

The addition of the onDidChangeFile event listener is a valuable enhancement to the InsightsPanel class. By monitoring file creation events in the notebook file system and triggering a refetch of the notebooks in the webview, this change ensures that the insights panel always displays the most current state of the notebooks.

The logic of the listener is sound, and the refetchNotebooks command sent to the webview is appropriate for the purpose.

Moreover, this change provides the missing context for the previous change that introduced the NotebookFileSystemProvider dependency. It's now clear that the dependency is used to subscribe to file system events and keep the insights panel in sync with the notebook files.

src/altimate.ts (5)

127-132: LGTM!

The interface changes look good. Making the data property optional provides flexibility in the request payload.


134-138: LGTM!

The new UpdateNotebookRequest interface looks good. The optional properties allow for partial updates to notebook attributes, providing flexibility in the update process.


971-975: LGTM!

The new deleteNotebook method looks good. It follows the existing patterns and aligns with the purpose of deleting notebooks by ID.


Line range hint 977-982: Verify the method signature change in the codebase.

The method logic looks good. However, ensure that all method calls to updateNotebook have been updated to match the new signature.

Run the following script to verify the method usage:

#!/bin/bash
# Description: Verify all method calls to `updateNotebook` match the new signature.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type typescript -A 5 $'updateNotebook'

964-968: Verify the method signature change in the codebase.

The method logic looks good. However, ensure that all method calls to addNotebook have been updated to match the new signature.

Run the following script to verify the method usage:

src/code_lens_provider/index.ts (1)

13-17: Constructor dependency injection is appropriate

The addition of dbtProjectContainer as a dependency is appropriate and aligns with the class's responsibilities.

src/lib/index.d.ts (2)

140-140: Addition of 'outputs' property to 'NotebookCellSchema' looks good

The optional outputs property in NotebookCellSchema enhances the schema by allowing cell outputs to be included. This change is appropriate and aligns with notebook data structures.


174-174: Verify the implications of exporting 'NotebookFileSystemProvider'

The class NotebookFileSystemProvider is now exported. Confirm that this is intentional and assess whether exposing this class aligns with the module's design. Ensure that exporting it does not unintentionally expose internal implementations.

Consider the potential impact on the public API surface and whether additional documentation is required.

return (
<div className={classes.perspectiveWrapper}>
<PerspectiveViewer
columnNames={items.columnNames}
columnTypes={items.columnTypes}
data={items.data}
styles={{ height }}
styles={{ minHeight: 100, height: 400 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a dynamic height calculation for the PerspectiveViewer.

The current implementation sets a fixed height of 400 pixels for the PerspectiveViewer, which may not be optimal for all data set sizes. If the data set is small, the viewer may appear overly spacious, and if the data set is large, the viewer may not provide enough vertical space to display all the data without scrolling.

Consider calculating the viewer's height dynamically based on the data set size, while still providing a minimum height for small data sets. Here's a suggested approach:

-styles={{ minHeight: 100, height: 400 }}
+styles={{ height: Math.max(items.data.length * 30, 100) }}

This approach calculates the height by multiplying the number of data items (items.data.length) by a fixed row height (e.g., 30 pixels) and ensures a minimum height of 100 pixels using Math.max().

Adjust the row height multiplier and minimum height as needed to achieve the desired visual presentation for your specific use case.

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.

Suggested change
styles={{ minHeight: 100, height: 400 }}
styles={{ height: Math.max(items.data.length * 30, 100) }}

),
);
// Add codelens after projects are initialized to avoid race conditions in executing notebook cells
this.dbtProjectContainer.onDBTProjectsInitialization(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure event subscription is properly disposed to prevent memory leaks

The event handler registered with onDBTProjectsInitialization is not added to the disposables array. This could lead to memory leaks if the event listener remains active after the extension is deactivated.

Apply this diff to add the event subscription to the disposables:

-    this.dbtProjectContainer.onDBTProjectsInitialization(() => {
+    const initializationDisposable = this.dbtProjectContainer.onDBTProjectsInitialization(() => {
       // existing code
     });
+    this.disposables.push(initializationDisposable);

This ensures that the event listener is properly disposed when the CodeLensProviders instance is disposed.

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.

Suggested change
this.dbtProjectContainer.onDBTProjectsInitialization(() => {
const initializationDisposable = this.dbtProjectContainer.onDBTProjectsInitialization(() => {
// existing code
});
this.disposables.push(initializationDisposable);

Comment on lines +20 to +43
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_YAML_SELECTOR,
this.sourceModelCreationCodeLensProvider,
),
);
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_SQL_SELECTOR,
this.virtualSqlCodeLensProvider,
),
);
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_YAML_SELECTOR,
this.documentationCodeLensProvider,
),
);
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_SQL_SELECTOR,
this.documentationCodeLensProvider,
),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to reduce code duplication in code lens provider registrations

The repeated code for registering code lens providers can be refactored to improve maintainability and readability.

Apply this diff to refactor the registration code:

-      this.disposables.push(
-        languages.registerCodeLensProvider(
-          DBTPowerUserExtension.DBT_YAML_SELECTOR,
-          this.sourceModelCreationCodeLensProvider,
-        ),
-      );
-      this.disposables.push(
-        languages.registerCodeLensProvider(
-          DBTPowerUserExtension.DBT_SQL_SELECTOR,
-          this.virtualSqlCodeLensProvider,
-        ),
-      );
-      this.disposables.push(
-        languages.registerCodeLensProvider(
-          DBTPowerUserExtension.DBT_YAML_SELECTOR,
-          this.documentationCodeLensProvider,
-        ),
-      );
-      this.disposables.push(
-        languages.registerCodeLensProvider(
-          DBTPowerUserExtension.DBT_SQL_SELECTOR,
-          this.documentationCodeLensProvider,
-        ),
-      );
+      const codeLensRegistrations = [
+        {
+          selector: DBTPowerUserExtension.DBT_YAML_SELECTOR,
+          provider: this.sourceModelCreationCodeLensProvider,
+        },
+        {
+          selector: DBTPowerUserExtension.DBT_SQL_SELECTOR,
+          provider: this.virtualSqlCodeLensProvider,
+        },
+        {
+          selector: DBTPowerUserExtension.DBT_YAML_SELECTOR,
+          provider: this.documentationCodeLensProvider,
+        },
+        {
+          selector: DBTPowerUserExtension.DBT_SQL_SELECTOR,
+          provider: this.documentationCodeLensProvider,
+        },
+      ];
+
+      codeLensRegistrations.forEach(({ selector, provider }) => {
+        this.disposables.push(
+          languages.registerCodeLensProvider(selector, provider),
+        );
+      });

This refactoring reduces duplication and simplifies future updates to the code lens registrations.

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.

Suggested change
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_YAML_SELECTOR,
this.sourceModelCreationCodeLensProvider,
),
);
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_SQL_SELECTOR,
this.virtualSqlCodeLensProvider,
),
);
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_YAML_SELECTOR,
this.documentationCodeLensProvider,
),
);
this.disposables.push(
languages.registerCodeLensProvider(
DBTPowerUserExtension.DBT_SQL_SELECTOR,
this.documentationCodeLensProvider,
),
);
const codeLensRegistrations = [
{
selector: DBTPowerUserExtension.DBT_YAML_SELECTOR,
provider: this.sourceModelCreationCodeLensProvider,
},
{
selector: DBTPowerUserExtension.DBT_SQL_SELECTOR,
provider: this.virtualSqlCodeLensProvider,
},
{
selector: DBTPowerUserExtension.DBT_YAML_SELECTOR,
provider: this.documentationCodeLensProvider,
},
{
selector: DBTPowerUserExtension.DBT_SQL_SELECTOR,
provider: this.documentationCodeLensProvider,
},
];
codeLensRegistrations.forEach(({ selector, provider }) => {
this.disposables.push(
languages.registerCodeLensProvider(selector, provider),
);
});

private dbtTerminal;
private altimate;
private _emitter;
readonly onDidChangeFile: Event_2<FileChangeEvent[]>;
private notebookDataMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add type annotation for 'notebookDataMap' property

The property notebookDataMap is declared without a type annotation. Providing a specific type enhances code readability and type safety.

Apply this diff to add the appropriate type annotation:

-private notebookDataMap;
+private notebookDataMap: Map<string, NotebookData>;

Replace Map<string, NotebookData> with the actual type if different.

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.

Suggested change
private notebookDataMap;
private notebookDataMap: Map<string, NotebookData>;

@@ -67,8 +67,8 @@ export declare class DatapilotNotebookController implements Disposable_2 {
private _onNotebookCellEvent;
readonly onNotebookCellChangeEvent: Event_2<NotebookCellEvent>;
private readonly disposables;
private associatedNotebooks;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add type annotation for 'associatedNotebooks' property

The property associatedNotebooks is declared without a type annotation. In TypeScript, specifying the type of class properties enhances type safety and code maintainability.

Apply this diff to add the appropriate type annotation:

-private associatedNotebooks;
+private associatedNotebooks: Set<NotebookDocument>;

Replace Set<NotebookDocument> with the actual type if different.

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.

Suggested change
private associatedNotebooks;
private associatedNotebooks: Set<NotebookDocument>;

private onNotebookOpen;
private waitForControllerAssociation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete method declaration for 'waitForControllerAssociation'

The method waitForControllerAssociation is missing parameters and a return type. Providing these details improves type safety and code comprehension.

Apply this diff to complete the method declaration:

-private waitForControllerAssociation;
+private waitForControllerAssociation(notebook: NotebookDocument): Promise<void>;

Replace NotebookDocument and Promise<void> with the appropriate types.

Committable suggestion was skipped due to low confidence.

@@ -86,14 +86,17 @@ export declare class DatapilotNotebookController implements Disposable_2 {
generateDbtDbtModelCTE(args: any): Promise<void>;
extractExposuresFromMetabase(args: any): Promise<void>;
extractExposuresFromTableau(args: any): Promise<void>;
private getUntitledFileName;
private getFileName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete method declaration for 'getFileName'

The method getFileName is declared without parameters or a return type. In TypeScript declaration files (.d.ts), methods should include parameter types and return types for accurate type checking.

Apply this diff to complete the method declaration:

-private getFileName;
+private getFileName(notebookId: string): string;

Replace notebookId: string and string with the appropriate parameter and return types.

Committable suggestion was skipped due to low confidence.

private onNotebookOpen;
private waitForControllerAssociation;
private isControllerAssociatedWithNotebook;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete method declaration for 'isControllerAssociatedWithNotebook'

The method isControllerAssociatedWithNotebook should include parameters and a return type for clarity and type accuracy.

Apply this diff to complete the method declaration:

-private isControllerAssociatedWithNotebook;
+private isControllerAssociatedWithNotebook(notebook: NotebookDocument): boolean;

Replace NotebookDocument and boolean with the correct types as needed.

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.

Suggested change
private isControllerAssociatedWithNotebook;
private isControllerAssociatedWithNotebook(notebook: NotebookDocument): boolean;

@@ -183,6 +188,7 @@ declare class NotebookFileSystemProvider implements FileSystemProvider {
stat(_uri: Uri): FileStat;
readDirectory(_uri: Uri): [string, FileType][];
createDirectory(_uri: Uri): void;
private getNotebookData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete method declaration for 'getNotebookData'

The method getNotebookData lacks parameters and a return type. Including these details is important for accurate type checking and understanding method functionality.

Apply this diff to complete the method declaration:

-private getNotebookData;
+private getNotebookData(uri: Uri): NotebookData | undefined;

Replace Uri and NotebookData | undefined with the appropriate types.

Committable suggestion was skipped due to low confidence.

createNotebook(args: OpenNotebookRequest | undefined): Promise<void>;
private sendMessageToPreloadScript;
private getRandomString;
private genUniqueId;
private updateCellId;
private onNotebookClose;
private onDidChangeSelectedNotebooks;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete method declaration for 'onDidChangeSelectedNotebooks'

The method onDidChangeSelectedNotebooks lacks parameters and a return type. Ensure that you specify these for proper type checking and clarity in method usage.

Apply this diff to complete the method declaration:

-private onDidChangeSelectedNotebooks;
+private onDidChangeSelectedNotebooks(e: NotebookControllerChangeEvent): void;

Replace NotebookControllerChangeEvent and void with the correct types if different.

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.

Suggested change
private onDidChangeSelectedNotebooks;
private onDidChangeSelectedNotebooks(e: NotebookControllerChangeEvent): void;

@mdesmet mdesmet merged commit cd4f6c0 into master Sep 20, 2024
10 checks passed
@mdesmet mdesmet deleted the fix/notebooks-v1 branch September 20, 2024 18:25
@coderabbitai coderabbitai bot mentioned this pull request Oct 24, 2024
2 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 4, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants