feat(tui): persistent preferences #9512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
anthonyshew
changed the title
anthonyshew
commented
Dec 17, 2024
anthonyshew
commented
Dec 17, 2024
| @@ -730,6 +761,7 @@ fn cleanup<B: Backend + io::Write>( | |||
| )?; | |||
| let tasks_started = app.tasks_by_status.tasks_started(); | |||
| app.persist_tasks(tasks_started)?; | |||
| app.preferences.flush_to_disk().ok(); | |||
There was a problem hiding this comment.
@chris-olszewski, this was smart. Kudos.
anthonyshew
commented
Dec 17, 2024
crates/turborepo-ui/src/tui/mod.rs
Outdated
| @@ -36,4 +37,6 @@ pub enum Error { | |||
| Stdin { name: String, e: std::io::Error }, | |||
| #[error(transparent)] | |||
| Io(#[from] std::io::Error), | |||
| #[error("Unable to persist preferences")] | |||
There was a problem hiding this comment.
Would it make sense to do:
Suggested change
| #[error("Unable to persist preferences")] | |
| #[error("Unable to persist preferences. Please file a bug report.")] |
chris-olszewski
approved these changes
Dec 17, 2024
| @@ -262,8 +262,9 @@ impl Run { | |||
|
|
|||
| let (sender, receiver) = TuiSender::new(); | |||
| let color_config = self.color_config; | |||
| let repo_root = self.repo_root.clone(); | |||
There was a problem hiding this comment.
We can just borrow here instead of cloning
Suggested change
| let repo_root = self.repo_root.clone(); | |
| let repo_root = &self.repo_root; |
| @@ -126,6 +131,18 @@ impl<W> App<W> { | |||
| }) | |||
| } | |||
|
|
|||
| fn update_sidebar_toggle(&mut self) { | |||
There was a problem hiding this comment.
I feel like toggle implies an update, up to you if this is better name 🤷
Suggested change
| fn update_sidebar_toggle(&mut self) { | |
| fn toggle_sidebar(&mut self) { |
Comment on lines
+44
to
+47
| pub fn set_active_task(&mut self, value: Option<String>) -> Result<(), Error> { | ||
| self.config.active_task = value; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
No need to lie to the caller that this could fail. We can propagate this change up to callers that now no longer need to handle the error case.
Suggested change
| pub fn set_active_task(&mut self, value: Option<String>) -> Result<(), Error> { | |
| self.config.active_task = value; | |
| Ok(()) | |
| } | |
| pub fn set_active_task(&mut self, value: Option<String>) { | |
| self.config.active_task = value; | |
| } |
Comment on lines
+121
to
+137
| loader | ||
| .file_path | ||
| .ensure_dir() | ||
| .expect("Failed to create directory"); | ||
|
|
||
| let preferences = Preferences { | ||
| active_task: Some("web#dev".to_owned()), | ||
| is_task_list_visible: Some(false), | ||
| }; | ||
|
|
||
| loader | ||
| .file_path | ||
| .create_with_contents( | ||
| serde_json::to_string_pretty(&preferences) | ||
| .expect("Failed to serialize preferences"), | ||
| ) | ||
| .expect("Failed to create file"); |
There was a problem hiding this comment.
I think it would be fine to use the methods instead of creating the file manually
Suggested change
| loader | |
| .file_path | |
| .ensure_dir() | |
| .expect("Failed to create directory"); | |
| let preferences = Preferences { | |
| active_task: Some("web#dev".to_owned()), | |
| is_task_list_visible: Some(false), | |
| }; | |
| loader | |
| .file_path | |
| .create_with_contents( | |
| serde_json::to_string_pretty(&preferences) | |
| .expect("Failed to serialize preferences"), | |
| ) | |
| .expect("Failed to create file"); | |
| loader.set_is_task_list_visible(Some(false)); | |
| loader.set_active_task(Some("web#dev".to_owned())); | |
| loader.flush_to_disk().expect("failed to create file); |
Comment on lines
+151
to
+167
| loader | ||
| .file_path | ||
| .ensure_dir() | ||
| .expect("Failed to create directory"); | ||
|
|
||
| let preferences = Preferences { | ||
| active_task: Some("web#dev".to_owned()), | ||
| is_task_list_visible: Some(false), | ||
| }; | ||
|
|
||
| loader | ||
| .file_path | ||
| .create_with_contents( | ||
| serde_json::to_string_pretty(&preferences) | ||
| .expect("Failed to serialize preferences"), | ||
| ) | ||
| .expect("Failed to create file"); |
| self.preferences.set_active_task( | ||
| self.is_task_selection_pinned | ||
| .then(|| active_task.to_owned()), | ||
| )?; |
There was a problem hiding this comment.
This isn't doing FS anymore so it shouldn't have a failure
Suggested change
| )?; | |
| ); |
Comment on lines
+139
to
+144
| fn update_task_selection_pinned_state(&mut self) -> Result<(), Error> { | ||
| // Preferences assume a pinned state when there is an active task. | ||
| // This `None` creates "un-pinned-ness" on the next TUI startup. | ||
| self.preferences.set_active_task(None)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I find this name a little confusing. Does clear_pinned_task express the behavior better?
Suggested change
| fn update_task_selection_pinned_state(&mut self) -> Result<(), Error> { | |
| // Preferences assume a pinned state when there is an active task. | |
| // This `None` creates "un-pinned-ness" on the next TUI startup. | |
| self.preferences.set_active_task(None)?; | |
| Ok(()) | |
| } | |
| fn update_task_selection_pinned_state(&mut self) { | |
| // Preferences assume a pinned state when there is an active task. | |
| // This `None` creates "un-pinned-ness" on the next TUI startup. | |
| self.preferences.set_active_task(None); | |
| } |
This was referenced Dec 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Users have expressed that they find themselves repeatedly having to select out the task that they want to use. This ends up being annoying when invoking
turbo, shutting down, invokingturbo, shutting down...You end up having to reselect the task you care about every re-invoke.To improve on this, we're going to hold some state in
.turbo/preferences/tui.jsonso that users pick up where they left off from their previous TUI invocation. State we'll keep around is:Testing Instructions
Wrote tests but you should also try it out by hand. You should be able to run
turboinvocations, pick out tasks, and see the state persist across invocations. You also should be able to delete the.turbo/preferences/tui.jsonfile and it'll feel like the first invocation (no preferences).Small note
One thing that I noticed while hand-testing is that this implementation tolerates extra fields fine, but doesn't tolerate the wrong types for a field. It will crash for in the latter scenario.
Personally, I think this is fine. You'd have to purposefully go and edit the file to get the wrong types into there.