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: Mitigating undefined bug in alreadyOpenedProject.fsPath #310

Closed

Conversation

AryamanAgrawalIronclad
Copy link
Contributor

@AryamanAgrawalIronclad AryamanAgrawalIronclad commented Jan 11, 2024

  • When using File > Open Project we were encountering an undefined value when trying to parse alreadyOpenedProject.fsPath
  • We can mitigate this by placing another check to make sure that the field fsPath exists in alreadyOpenedProject

Error:
Screenshot 2024-01-11 at 1 42 46 PM

@AryamanAgrawalIronclad AryamanAgrawalIronclad changed the title Mitigating undefined bug in alreadyOpenedProject.fsPath fix: Mitigating undefined bug in alreadyOpenedProject.fsPath Jan 11, 2024
@@ -31,7 +31,7 @@ export function useLoadProjectWithFileBrowser() {
(p) => p.project.metadata.id === project.metadata.id,
);

if (alreadyOpenedProject) {
if (alreadyOpenedProject && alreadyOpenedProject.fsPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think alreadyOpenedPath.fsPath can be undefined, if the project is a new project that hasn't yet been saved.

Perhaps we can move the check into the toast.error call instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the project hasn't been saved then a user wouldn't be able to use useLoadProjectWithFileBrowser would they? Then it seems fine to have the check here. Not sure if I'm interpreting this right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, the purpose of this if condition only wraps the toast, if I move the check within the toast.error, I'd still be dealing with the unwarranted error in loading a project
Screenshot 2024-01-12 at 1 38 43 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my specific suggestion was:

if (alreadyOpenedProject) {
  const fileName = alreadyOpenedProject.fsPath?.split('/').pop() ?? '';
  toast.error(`"${alreadyOpenedProject.project.metadata.title} [${fileName}]" shares the same ID (...`);
  return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm not sure how we get into this situation in the first place ever, so I'm inclined to suggest programming super defensively.

If alreadyOpenedProject has the same ID as the project we are about to open, we will likely run into issues where the wrong project gets updated in state. Hence why I'd worry about adding the && alreadyOpenedProject.fsPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! Thanks for clarifying! That makes sense!

@codemile
Copy link
Collaborator

This error is fixed as part of this PR, but silencing undefined error won't fix the underlying issue. The real problem is related to closing/reopening projects that are kept in localStorage.

#293

Anyway, I'm happy to merge this PR and move forward with finishing my PR. It still needs some improvements, but we might as well get rid of this undefined error.

@codemile
Copy link
Collaborator

I have resolved the feedback on my PR #293 and if approved can be merged. Thanks for the help @AryamanAgrawalIronclad but this can be closed.

@abrenneke
Copy link
Collaborator

Thanks @AryamanAgrawalIronclad I've merged the other PR which fixes the root problem!

@abrenneke abrenneke closed this Jan 16, 2024
@AryamanAgrawalIronclad
Copy link
Contributor Author

Sounds good! Thanks for finding the right solve!

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.

4 participants