Skip to content

feat(client/tauri): allow users to favorite specific Resources and hide the rest#5923

Merged
ReactorScram merged 23 commits intomainfrom
feat/tauri-favorites-menu
Aug 6, 2024
Merged

feat(client/tauri): allow users to favorite specific Resources and hide the rest#5923
ReactorScram merged 23 commits intomainfrom
feat/tauri-favorites-menu

Conversation

@ReactorScram
Copy link
Copy Markdown
Contributor

@ReactorScram ReactorScram commented Jul 19, 2024

Refs #5123

Looking at a Resource when nothing is favorited
image

Looking at a favorited Resource
image

Looking at a non-favorited Resource
image

- [x] Double-check that the Resources are sorted alphabetically (or whatever) and leave a comment where they're sorted
- [x] String changes
- [x] Move "Add" and "Remove" down to a different section
- [x] Fix empty favorites menu
- [x] Merge

@ReactorScram ReactorScram self-assigned this Jul 19, 2024
@vercel
Copy link
Copy Markdown

vercel bot commented Jul 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 6, 2024 10:16pm

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 19, 2024

Terraform Cloud Plan Output

Plan: 15 to add, 23 to change, 15 to destroy.

Terraform Cloud Plan

typo in changelog

Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Comment thread rust/gui-client/src-tauri/src/client/settings.rs
Comment thread rust/gui-client/src-tauri/src/client/settings.rs
Comment thread rust/gui-client/src-tauri/src/client/gui/system_tray.rs
@ReactorScram ReactorScram marked this pull request as ready for review July 19, 2024 23:07
submenu
.separator()
.disabled("Site")
.copyable(&site.name) // Hope this is okay - The code is simpler if every enabled item sends an `Event` on click
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can change this back to the old behavior where we have some rare items that are enabled but clicking does nothing, by making enabled explicit. I just thought it's weird to have a button that does nothing, at least if it copies the text maybe it'll aid debugging or something.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 19, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 238.4 MiB (-2%) 239.7 MiB (-2%) 210 (-15%)
direct-tcp-server2client 244.0 MiB (-0%) 245.3 MiB (-0%) 624 (+96%)
relayed-tcp-client2server 245.0 MiB (-2%) 245.8 MiB (-2%) 310 (-31%)
relayed-tcp-server2client 267.6 MiB (+1%) 268.3 MiB (+1%) 625 (+3%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 500.0 MiB (-0%) 0.04ms (-15%) 44.00% (+7%)
direct-udp-server2client 500.0 MiB (+0%) 0.00ms (-40%) 20.71% (+3%)
relayed-udp-client2server 500.0 MiB (-0%) 0.04ms (+39%) 54.67% (+2%)
relayed-udp-server2client 500.0 MiB (+0%) 0.02ms (-40%) 29.15% (-10%)

Copy link
Copy Markdown
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice work!

Comment thread rust/gui-client/src-tauri/src/client/settings.rs
Comment thread rust/gui-client/src-tauri/src/client/settings.rs
fn separator(self) -> Self;
}
#[cfg(test)]
mod tests {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice tests!

Comment thread rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs Outdated
Comment thread rust/gui-client/src-tauri/src/client/gui.rs
Copy link
Copy Markdown
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

Looks good! Couple UX nits:

"Add favorite" -> "Add to favorites"
"Remove favorite" -> "Remove from favorites"

If 1 or more favorites exist, maybe we should title that section "Favorite Resources" and move others to a section below (doesn't need a title) and call the menu "Other resources"? EDIT: looks like you already added a section divider, nice.

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Jul 20, 2024

Just curious, how do they reset the menu? Is there a button for it in settings or does it reset with "Reset to defaults"?

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Jul 22, 2024

Another thought (ux) -- should "Add to favorites" be in the second submenu section at the bottom? I.e. under the other "Resource" details.

I could see it going either way, just curious your thoughts.

@ReactorScram
Copy link
Copy Markdown
Contributor Author

@jamilbk Those changes all sound good. And yeah resetting advanced settings removes all favorites. It's not intuitive but they're hooked into the settings file so it was easy to write the code that way.

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Jul 22, 2024

@jamilbk Those changes all sound good. And yeah resetting advanced settings removes all favorites. It's not intuitive but they're hooked into the settings file so it was easy to write the code that way.

Gotcha, I need to think if this should work the same way on other platforms. We can reset this menu no prob with the user signed in, unlike the other settings which either sign you out (Apple) or tell you changes won't take effect until next sign in (Android).

Might need either a "Reset favorites" button or add more logic to the above cases... probably will opt for the former for the other clients.

@ReactorScram
Copy link
Copy Markdown
Contributor Author

@jamilbk Should I keep this PR open until we figure that out?

Also, what happens in this sequence?

  • Account has Resources A, B, C
  • User favorites C
  • Account loses C

We shouldn't display an empty "Favorite Resources" section, right, we should act as if the now-deleted C is also unfavorited?

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Jul 22, 2024

Should I keep this PR open until we figure that out?

Yeah I think that makes sense. Probably need to get this feature right since most have asked for it.

We shouldn't display an empty "Favorite Resources" section, right, we should act as if the now-deleted C is also unfavorited?

Yeah, interesting edge case. Maybe it could work like this:

  • "Add to favorites" tracks resource ids in a favorites array that persists until user removes from favorites or resets favorites state
  • "Update resources" callback updates displayed resources
  • When the menu is rendered, if at least one resource_id lies in favorites, use favorites menu
  • otherwise display all resources

Base automatically changed from refactor/tauri-menu to main July 30, 2024 16:03
Comment thread rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs Outdated
Comment thread rust/gui-client/src-tauri/src/client/gui/system_tray.rs
}

#[test]
fn dns_resource_with_url() {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got rid of this by just testing the URL marker arrows (<https://example.com>) in every unit test

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 30, 2024

🐰Bencher

ReportTue, August 6, 2024 at 22:22:39 UTC
ProjectFirezone
Branchfeat/tauri-favorites-menu
Testbedgithub-actions
Click to view all benchmark results
BenchmarkThroughputThroughput Results
bits/s | (Δ%)
Throughput Lower Boundary
bits/s | (%)
direct-tcp-client2server✅ (view plot)243,341,638.48 (+0.58%)237,213,423.87 (97.48%)
direct-tcp-server2client✅ (view plot)244,920,338.89 (-1.32%)241,359,870.35 (98.55%)
direct-udp-client2server✅ (view plot)302,142,583.71 (+4.52%)271,866,563.77 (89.98%)
direct-udp-server2client✅ (view plot)402,191,907.38 (+1.53%)384,057,369.62 (95.49%)
relayed-tcp-client2server✅ (view plot)244,063,316.68 (-0.75%)239,404,508.18 (98.09%)
relayed-tcp-server2client✅ (view plot)253,623,827.81 (-1.44%)246,961,074.61 (97.37%)
relayed-udp-client2server✅ (view plot)227,491,793.93 (-0.85%)218,883,860.87 (96.22%)
relayed-udp-server2client✅ (view plot)334,813,938.43 (-0.81%)317,943,028.04 (94.96%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@ReactorScram ReactorScram changed the title feat(gui-client): allow users to favorite specific Resources and hide the rest feat(client/tauri): allow users to favorite specific Resources and hide the rest Jul 31, 2024
@thomaseizinger
Copy link
Copy Markdown
Member

@ReactorScram Please let me know when this is ready for re-review.

@ReactorScram
Copy link
Copy Markdown
Contributor Author

@thomaseizinger The Tauri Client itself should be ready, but I'm keeping it in draft to remind myself that we want to synchronize the release with Android and Apple too, so Tauri can't go out by itself

@thomaseizinger
Copy link
Copy Markdown
Member

@thomaseizinger The Tauri Client itself should be ready, but I'm keeping it in draft to remind myself that we want to synchronize the release with Android and Apple too, so Tauri can't go out by itself

Maybe we should implement feature-flags as part of the init message so we can release these things early and activate them all at once?

@ReactorScram
Copy link
Copy Markdown
Contributor Author

@thomaseizinger Sure. And we could do a CLI flag for local testing like Chromium does.

Though, it will be hard if the feature flag is needed before we reach the portal. So far there are no features like that, but there could be.

@thomaseizinger
Copy link
Copy Markdown
Member

@thomaseizinger Sure. And we could do a CLI flag for local testing like Chromium does.

Though, it will be hard if the feature flag is needed before we reach the portal. So far there are no features like that, but there could be.

The feature flag config could also just be at https://firezone.dev/flags.json, right? Doesn't have to be delivered via connlib.

@ReactorScram
Copy link
Copy Markdown
Contributor Author

Yeah I suppose. It will have to do something reasonable if we boot up without Internet. Maybe just save the flags to disk whenever they're seen, then fall back on disk if we can't reach the website. That way a flag that's seen flipped is never unflipped unless we unflip it on the server

@ReactorScram
Copy link
Copy Markdown
Contributor Author

We agreed in a meeting that we can ship this for all 3 platforms (Tauri, Android, Swift) whenever ready, so I'll merge this one first

@ReactorScram ReactorScram marked this pull request as ready for review August 6, 2024 22:11
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram ReactorScram enabled auto-merge August 6, 2024 22:13
@ReactorScram ReactorScram added this pull request to the merge queue Aug 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2024
@ReactorScram ReactorScram added this pull request to the merge queue Aug 6, 2024
Merged via the queue into main with commit 5b75e87 Aug 6, 2024
@ReactorScram ReactorScram deleted the feat/tauri-favorites-menu branch August 6, 2024 22:50
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.

3 participants