Allow 'Open Workspace' to open workspace whenever possible, even if logfile does not exist #1618
Conversation
There was a problem hiding this comment.
Hi @emilyanndavis , this looks great to me. I like the new error modal. It's nice that openPath returns an error when it doesn't work.
I appreciate you trying to preserve the behavior of opening the workspace with the logfile selected, when it exists. I don't think we were very intentional about that behavior when we first designed this. I'm sure the main objective was just to open to the workspace directory.
So I'm curious what you think about dropping the showItemInFolder pathway altogether and always using openPath? Mainly for the sake of simplicity.
On the other hand if you like having the file selected then I think your solution in this PR is great!
…oved-error-handling-when-logfile-does-not-exist
There was a problem hiding this comment.
@emilyanndavis this looks good except for some syntax in HISTORY that's not passing our checks. Maybe the indentation?
And I'm not sure why all the other checks are being cancelled. I didn't think the failed HISTORY check would cause that, but maybe it does?
I had one other code-comment, but it doesn't necessarily require any changes. Thanks!
| if (workspace_dir) { | ||
| this.openWorkspaceDir(workspace_dir); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a benefit to defining both handleOpenWorkspace and openWorkspaceDir? As opposed to doing all the logic in one function? It's fairly inconsequential so if you prefer it this way feel free to leave it as-is.
There was a problem hiding this comment.
Probably not. I think this is a holdover from when handleOpenWorkspace also called shell.showItemInFolder when the logfile was valid. With that complexity gone, it does look a little silly now.
| ? ( | ||
| <ModelStatusAlert | ||
| status={status} | ||
| handleOpenWorkspace={() => this.handleOpenWorkspace(argsValues?.workspace_dir)} |
There was a problem hiding this comment.
woah I just learned something new!
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining
Yes, it was the indentation. Should be fixed now. And the other checks were cancelled because I cancelled them once I saw the HISTORY check had failed. :) |
emilyanndavis
deleted the
bugfix/1598-improved-error-handling-when-logfile-does-not-exist
branch
Description
Fixes #1598, with a solution that is more forgiving (and perhaps more user-friendly) than the one in PR #1617.
Checklist
Representative screenshots
Open workspace with valid logfile (logfile is no longer selected, for the sake of simplicity)
Open workspace with missing logfile (in this case, it was manually deleted after a successful run)
Open empty workspace (this might happen if, for example, the workspace is readonly—see #1599)
Error modal when workspace cannot be opened (for example, if it does not exist, as would be the case if the workspace path specified a not-yet-extant subdirectory of a readonly directory—see #1599)