Skip to content

fix: repl: Always select a file when calling workspace.set() #1297

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

Merged
merged 2 commits into from
May 22, 2025
Merged

fix: repl: Always select a file when calling workspace.set() #1297

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b507464
repl: Always select a file when calling workspace.set()
cmcaine Apr 12, 2025
90167ef
Merge branch 'main' into patch-1
teemingc May 22, 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
10 changes: 3 additions & 7 deletions packages/repl/src/lib/Workspace.svelte.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,13 +425,9 @@ export class Workspace {
throw new Error('Workspace must have at least one file');
}

if (selected) {
const file = files.find((file) => is_file(file) && file.name === selected);

if (!file) {
throw new Error(`Invalid selection ${selected}`);
}
this.#select(file as File);
const matching_file = selected && files.find((file) => is_file(file) && file.name === selected);
if (matching_file) {
this.#select(matching_file as File);
} else {
this.#select(first);
}
Comment on lines +428 to 433
Copy link
Contributor

@zqianem zqianem May 18, 2025

Choose a reason for hiding this comment

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

If we trust the types (selected can't be undefined because this.#current is never nullish), this can be further simplified to:

Suggested change
const matching_file = selected && files.find((file) => is_file(file) && file.name === selected);
if (matching_file) {
this.#select(matching_file as File);
} else {
this.#select(first);
}
const file = files.find((file) => is_file(file) && file.name === selected) as File;
this.#select(file ?? first);

and the ? in this method's default argument can be removed.

Copy link
Contributor Author

@cmcaine cmcaine May 22, 2025

Choose a reason for hiding this comment

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

Can we trust that this.#current is never nullish? It is initialised like this: #current = $state.raw() as File (which is obviously a lie) and set() is called in the constructor before any obvious updates to this.#current.

I think maybe the type of this.#current should be updated to File | undefined.

Copy link
Contributor

@zqianem zqianem May 22, 2025

Choose a reason for hiding this comment

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

Ah yes, you're right, I completely missed the initialization and was only looking at assignments to #current.

Expand Down