feat(connect-form): limit recent connections to 10

[mcasimir]

List of changes:

• derive pre-sorted favorite and recent connections lists from loaded connections
• on connect: delete the last recent if a new one was saved and 10 were already existing
• use favoriteConncections and recentConnections in the connection list
• isolate <ResizableSidebar> from <ConnectionList>

[mcasimir]

NOTE: this technically abuses a little the fact that recentConnections contains already all the recents ordered in the right way. This may change, for example, if we introduce some way to reorder or filter that list dynamically .. not really something we are planning for now, but I could easily extract a getRecentsByLastUsed(connections) and use it both here and in useMemo instead.

Let me know if you have some thoughts about it.

[mcasimir]

anything better?

[gribnoysup]

You can write the test in onConnected and use done similar to how we usually test callback behavior, right?

[mcasimir]

If I understand it right, onConnected is called before any of this happens

[gribnoysup]

Would it make sense to move the callback and call it only after everything else finished there successfuly then maybe? Otherwise you're communicating that connection happened and then do a bunch of stuff that can fail in the same hook, is it sorta expected?

[mcasimir]

Yeah we could do something Iike that. I think is intentional that one doesn't wait for the other though.

Probably better to have a Promise.all() to make it clear, but yeah if saving "recent" won't work users should still be able to connect and move on to the next screen.

I could do

try { 
   await updateRecent()
} catch (e) {
   log(e);
};

onConnectionSucceded()

@Anemy what's your feedback on this?

To be clear that test won't flake and it would quite likely fail all the time if a recent gets removed, but is not technically guaranteed to be red if the feature is broken.

[Anemy]

I see what you mean it's a bit hard to test.
I think it makes sense not to wait for a file system access or entry before moving the user past the connect screen? Probably super fast on most machines but could cause some delay potentially and as a result we'd want to add a check if the user then cancelled the connection attempt and stop the flow here making things a bit more complicated then that probably need to be.
Can't think of a cleaner way without bringing in extra logic or debugs in the flow. I'm alright with the setTimeout here as it's only in one test and will probably never fail, can always improve it later if somehow it does flake. Would adding a debug log after this flow has completed and then monitoring debug logs in the test work?

[mcasimir]

this flow has completed and then monitoring debug logs in the test work

Seems convoluted, If we have to change the code for this test maybe we could add an optional callback like onConnectionProcessComplete or something? That we could do, not really useful outside the tests but is kinda clean.

[lerouxb]

The e2e test flake will hopefully go away if you merge in main: #2899

Or it will at least help us debug what's going on if it happens again.

[Anemy]

Nice, I really like the improvements in the resizable sidebar usage and connections-store.
One comment on the sidebar width which might not be intended? Other than that looks good to go

[Anemy]

🔥