fix(tab) Show a toast when a tab config references a missing directory#9724
fix(tab) Show a toast when a tab config references a missing directory#9724doubledare704 wants to merge 5 commits into
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @vkodithala. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR preserves the tab config directory value when constructing terminal and agent panes, allowing lower-level shell startup/bootstrap handling to receive the configured cwd even when it does not exist yet. The added regression test covers the pane-template path and asserts the startup path remains available on the terminal model.
Concerns
- No blocking correctness or security concerns found in the changed lines. Local Unix startup passes the path through
WARP_INITIAL_WORKING_DIRand starts the process from home; Windows still filterslpCurrentDirectorywithis_dir()while passing the bootstrap env var, so invalid paths should not become unsafe process cwd values.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Hey @doubledare704 - Varoon from Warp here. Thanks so much for the contribution! Think there's a small product discussion needed around the right solution here, bumped the issue priority back down for now: #9130 (comment). Chatting with the team, we'll get back to ya shortly! PR is 👌 |
vkodithala
left a comment
There was a problem hiding this comment.
Hey @doubledare704 - just chatted with the team. Here's a tl;dr: #9130 (comment). We'd like to launch a persistent error toast on tab config errors like this one vs. creating missing directories in the user's cwd ourselves (the latter feels a little too invasive).
Had an agent start on an implementation - you're welcome to wrap things up and request a re-review!
I understand. I’ll do my best. If using oz-for-oss will improve the situation, I’ll close this post. |
|
Implemented the requested persistent tab config error handling and pushed the update in 3efbd0e. Literal non-parameterized missing/non-directory Validation run:
Skipped per local constraints: @vkodithala this is ready for re-review. |
3efbd0e to
5998b60
Compare
5998b60 to
d3485d4
Compare
vkodithala
left a comment
There was a problem hiding this comment.
Hey @doubledare704, thanks for the fixes and apologies for the delay here. Mind fixing these merge conflicts and I'll re-review?
Sure, go ahead. |
|
Hi @doubledare704 — a reviewer requested changes on this PR and it hasn't had activity from you in 18 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 30 days of inactivity. |
d3485d4 to
ec87948
Compare
I've rebased a branch from master. |
|
Hi @doubledare704 — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
|
Hi @doubledare704 — final reminder: a reviewer requested changes on this PR and it has been inactive for 10 days. It will be automatically closed in about 4 day(s) unless you push updates or reply. Maintainers can apply the |
|
Please review updated changes |
|
Hi @doubledare704 — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
|
Hi @doubledare704 — final reminder: a reviewer requested changes on this PR and it has been inactive for 10 days. It will be automatically closed in about 4 day(s) unless you push updates or reply. Maintainers can apply the |
Deferring this to another reviewer who has more context on the intended behavior here!
vkodithala
left a comment
There was a problem hiding this comment.
Took a peek at this implementation; it seems reasonable, the a few small things we'd probably want to clarify are:
- Are we okay with error messages only popping up after a user's edited a tab config? i.e., if we have a pre-existing broken config in the watched directory, errors are only surfaced when the user clicks "New tab config", not on startup
- Should the missing directory toast be persistent? I'd expect it to disappear after a timeout
- Would also probably want to rev on the general messaging of this error toast; it seems a little verbose
@moirahuang tagging you on this PR since you might have more context. @doubledare704, would be awesome if you could link a video demo explaining how this all works. Thanks!
moirahuang
left a comment
There was a problem hiding this comment.
A few comments:
- Instead of making a non-existent directory cause the tab config to be a parsing failure, should we have the toast appear when you try to use a tab config with a dir that doesn't exist and we fall back to the home directory? This feels more semantically correct. A non-existent directory isn't a parsing issue, but is a runtime issue
- Agreed with Varoon we should dismiss the toasts after a timeout, we can check if there's prior art around this in our existing toasts
- We should use the directory text in the tab config, even if the tab config uses ~
- Can we please update the PR title and description to be descriptive of the toast behavior?
| PaneMode::Terminal | PaneMode::Agent => PaneGroup::create_session( | ||
| // Use cwd from the template iff such path exists, otherwise None | ||
| // TODO(CORE-3187): On Windows, support WSL directory restoration. | ||
| Some(cwd).filter(|p| p.exists()), |
There was a problem hiding this comment.
Why are we removing this filter?
There was a problem hiding this comment.
Good catch. Previously, Some(cwd).filter(|p| p.exists()) turned a missing directory into None, which made create_session silently fall back to home and hide the problem. Since this PR's goal is to surface missing tab-config directories (via the validation + toast), we keep Some(cwd) so the missing path is preserved and detected.
… open A non-existent directory is a runtime issue, not a config-file parse error, so stop rejecting tab configs at load time when a literal directory is missing or isn't a folder. When opening a tab config, fall back to the home directory for any pane whose directory is missing or not a folder, open the tab as normal, and show a toast per affected pane. The toast uses the original directory text from the config (preserving `~`) and is ephemeral so it auto-dismisses after the toast timeout. Drop the now-unused validate_load_time_tab_config_directories, contains_template_placeholder, and find_invalid_tab_config_cwd helpers, and update tests accordingly.

Description
When you open a tab config whose pane references a directory that doesn't exist or isn't a folder, Warp now opens the tab (falling back to the home directory for that pane) and shows an auto-dismissing toast naming the offending directory. The toast uses the directory text as written in the config (e.g. ~), and existing configs are no longer rejected at load time for a missing directory.
Previously,
pane_groupfiltered the configured cwd withp.exists(). When the directory was missing at tab creation time, Warp silently discarded the tab config cwd and fell back to the default startup directory, making the tab config field appear ignored.This change removes that early existence check so the configured cwd is still passed into session startup. Lower-level platform-specific startup behavior remains responsible for handling invalid process cwd values. In particular, the Windows PTY path still guards
CreateProcessWwith anis_dir()check before passinglpCurrentDirectory, so this should preserve the existing Windows spawn safety while keeping the tab config intent available to shell/bootstrap handling.Open technical questions for review:
directoryalways be treated as user intent, even when the path does not exist yet, or should Warp intentionally drop missing paths before shell startup?CreateProcessWis_dir()fallback the right place to keep spawn safety, instead of filtering tab config cwd earlier inpane_group?My current read is that
pane_groupshould preserve the configured cwd, while lower-level PTY/bootstrap code should decide how each platform handles missing or invalid directories.Linked Issue
Closes #9130
ready-to-specorready-to-implement.Screenshots / Videos
Not applicable; this is startup behavior with no UI changes.
Testing
Added a regression test covering a tab config pane whose configured cwd does not exist. The test asserts the non-existent cwd is still preserved as the terminal model startup path.
Ran:
cargo test -p warp test_tab_config_preserves_nonexistent_cwdcargo test -p warp pane_group::testscargo test -p warp working_directoriescargo test -p warp tree::testsAgent Mode
Changelog Entries for Stable
CHANGELOG-BUG-FIX: Fixed tab configs silently ignoring configured directories that do not exist yet.