Skip to content

fix: delete multiple tables from lib pane #1705

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: delete multiple tables from lib pane #1705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac6fe72
fix: delete multiple tables from lib pane
IreneS1 Aug 7, 2025
5f2e18c
fix: delete multiple tables from lib pane
IreneS1 Aug 7, 2025
f4047d3
fix: delete multiple tables from lib pane
IreneS1 Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions client/src/components/LibraryNavigator/LibraryDataProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ class LibraryDataProvider
return this._onDidChangeTreeData.event;
}

get treeView(): TreeView<LibraryItem> {
return this._treeView;
}

constructor(
private readonly model: LibraryModel,
private readonly extensionUri: Uri,
Expand Down Expand Up @@ -170,6 +174,11 @@ class LibraryDataProvider
this._onDidChangeTreeData.fire(undefined);
}

public async deleteTables(items: LibraryItem[]): Promise<void> {
await this.model.deleteTables(items);
this._onDidChangeTreeData.fire(undefined);
}

public watch(): Disposable {
// ignore, fires for all changes...
return new Disposable(() => {});
Expand Down
18 changes: 18 additions & 0 deletions client/src/components/LibraryNavigator/LibraryModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,24 @@ class LibraryModel {
}
}

public async deleteTables(items: LibraryItem[]) {
const failures: string[] = [];

for (const item of items) {
try {
await this.libraryAdapter.deleteTable(item);
} catch {
failures.push(item.uid);
}
}

if (failures.length > 0) {
throw new Error(
l10n.t(Messages.TableDeletionError, { tableName: failures.join(", ") }),
);
}
}

public async getTableInfo(item: LibraryItem) {
if (this.libraryAdapter.getTableInfo) {
await this.libraryAdapter.setup();
Expand Down
3 changes: 3 additions & 0 deletions client/src/components/LibraryNavigator/const.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { l10n } from "vscode";

export const Messages = {
TableDeletionError: l10n.t("Unable to delete table {tableName}."),
TablesDeletionWarning: l10n.t(
"Are you sure you want to delete {count} table(s): {tableNames}?",
Copy link
Contributor

@scottdover scottdover Nov 26, 2025

Choose a reason for hiding this comment

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

It might be worth asking @skyeaussie what message to use here. We may not need the count or extra parens around the s. Maybe something like...

Are you sure you want to delete the following tables: {tableNames}?

It looks like studio uses the following two messages:

Are you sure you want to delete the selected tables?
Are you sure you want to delete the table "{tableName}"

),
ViewTableCommandTitle: l10n.t("View SAS Table"),
};

Expand Down
40 changes: 39 additions & 1 deletion client/src/components/LibraryNavigator/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
Uri,
commands,
env,
l10n,
window,
workspace,
} from "vscode";
Expand All @@ -24,6 +25,7 @@ import LibraryAdapterFactory from "./LibraryAdapterFactory";
import LibraryDataProvider from "./LibraryDataProvider";
import LibraryModel from "./LibraryModel";
import PaginatedResultSet from "./PaginatedResultSet";
import { Messages } from "./const";
import { LibraryAdapter, LibraryItem, TableData } from "./types";

class LibraryNavigator implements SubscriptionProvider {
Expand Down Expand Up @@ -66,8 +68,35 @@ class LibraryNavigator implements SubscriptionProvider {
),
commands.registerCommand("SAS.refreshLibraries", () => this.refresh()),
commands.registerCommand("SAS.deleteTable", async (item: LibraryItem) => {
const selectedItems = this.treeViewSelections(item);

if (selectedItems.length === 0) {
return;
}

try {
await this.libraryDataProvider.deleteTable(item);
if (selectedItems.length === 1) {
Copy link
Contributor

@scottdover scottdover Nov 26, 2025

Choose a reason for hiding this comment

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

Instead of having two different paths here, how about we:

  • Get rid of LibraryDataProvider.deleteTable (you'll likely need to make a small update to tests)
  • Always call LibraryDataProvider.deleteTables
  • Always show a prompt regardless if you're deleting one or more tables

await this.libraryDataProvider.deleteTable(selectedItems[0]);
} else {
const tableNames = selectedItems
.map((table) => `${table.library}.${table.name}`)
.join(", ");

const result = await window.showWarningMessage(
l10n.t(Messages.TablesDeletionWarning, {
tableNames: tableNames,
count: selectedItems.length,
}),
{ modal: true },
"Delete",
);

if (result !== "Delete") {
return;
}

await this.libraryDataProvider.deleteTables(selectedItems);
}
} catch (error) {
window.showErrorMessage(error.message);
}
Expand Down Expand Up @@ -128,6 +157,15 @@ class LibraryNavigator implements SubscriptionProvider {
this.libraryDataProvider.useAdapter(this.libraryAdapterForConnectionType());
}

private treeViewSelections(item: LibraryItem): LibraryItem[] {
const items =
Copy link
Contributor

@scottdover scottdover Nov 26, 2025

Choose a reason for hiding this comment

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

I see this was copy/pasted from ContentNavigator. Could we make this a shared function that takes a treeview, perhaps in client/src/components/utils?

Right now, I think there's a bug with this (here and in content navigator). Putting this functionality in one place will make it easier to resolve.

In case you're curious, or want to tackle this in your PR (definitely out of scope), here's the bug:

  • Create some deletable tables. I ended up running...
data work.class;
    set sashelp.class;
run;
data work.classfit;
    set sashelp.classfit;
run;
data work.air;
    set sashelp.air;
run;
data work.aarfm;
    set sashelp.aarfm;
run;
  • Select class and classfit
  • With those two selected, right click on aarfm and press delete

In this scenario, I would expect aarfm to be the item up for deletion. However, what you get instead is a prompt to delete class and classfit.

Like I said, this also happens with sas content and server files, so feel free to not resolve it in this PR and I can file a bug.

this.libraryDataProvider.treeView.selection.length > 1 || !item
? this.libraryDataProvider.treeView.selection
: [item];

return items.filter(Boolean);
Copy link
Contributor

@scottdover scottdover Nov 26, 2025

Choose a reason for hiding this comment

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

Is there ever a case where its would be falsy here? Do we need this .filter?

}

private async displayTableProperties(
item: LibraryItem,
showPropertiesTab: boolean = false,
Expand Down
83 changes: 83 additions & 0 deletions client/test/components/LibraryNavigator/LibraryDataProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,4 +270,87 @@ describe("LibraryDataProvider", async function () {

deleteTableStub.restore();
});

it("deleteTables - deletes multiple tables successfully", async () => {
Copy link
Contributor

@scottdover scottdover Nov 26, 2025

Choose a reason for hiding this comment

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

🙌

const items: LibraryItem[] = [
{
uid: "lib.table1",
id: "table1",
name: "table1",
type: "table",
readOnly: false,
library: "lib",
},
{
uid: "lib.table2",
id: "table2",
name: "table2",
type: "table",
readOnly: false,
library: "lib",
},
];

const api = dataAccessApi();
const deleteTableStub = sinon.stub(api, "deleteTable");

const provider = libraryDataProvider(api);
await provider.deleteTables(items);

expect(deleteTableStub.callCount).to.equal(2);
deleteTableStub.restore();
});

it("deleteTables - reports failures when some tables fail to delete", async () => {
const items: LibraryItem[] = [
{
uid: "lib.table1",
id: "table1",
name: "table1",
type: "table",
readOnly: false,
library: "lib",
},
{
uid: "lib.table2",
id: "table2",
name: "table2",
type: "table",
readOnly: false,
library: "lib",
},
{
uid: "lib.table3",
id: "table3",
name: "table3",
type: "table",
readOnly: false,
library: "lib",
},
];

const api = dataAccessApi();
const deleteTableStub = sinon.stub(api, "deleteTable");

// First table succeeds
deleteTableStub.onFirstCall().resolves();
// Second table fails
deleteTableStub
.onSecondCall()
.throwsException(new Error("Failed to delete"));
// Third table succeeds
deleteTableStub.onThirdCall().resolves();

const provider = libraryDataProvider(api);

try {
await provider.deleteTables(items);
expect.fail("Should have thrown an error");
} catch (error) {
expect(error.message).to.contain("lib.table2");
expect(deleteTableStub.callCount).to.equal(3);
}

deleteTableStub.restore();
});
});
Loading