feat(connections): let a connection be tried before it has to be saved (#364) - #369
Conversation
#364) Connect only ever operated on a saved profile, and the editor's own actions were Create and Save. Someone with a connection string had to name it, file it in a folder and commit it to the vault before finding out whether it worked — and saving is what engages the vault, so the first thing a trial user did was write an encrypted credential to disk for a connection nobody had shown to work yet. When it then failed they were left holding a saved profile that did not connect. The editor now has a Connect of its own that runs on the fields as they stand and saves nothing. On success the connection is live and the user is asked whether to keep it, with a name already suggested from the host — "cluster0" for an Atlas cluster, the hostname for anything else — so the question is one click to answer either way. On failure the editor stays open with the cause named rather than the driver's topology dump, which is where the user can act on it. An unsaved connection travels under an id of its own so the app's dedupe-on-profile still tells two trial connections apart, and every path out of the editor adopts it — Escape included — because the connection is already open in the backend and declining to save is a decision about the profile, not the session. An untouched saved profile keeps its own identity instead, connecting on the exact string it stored, so tab rebinding and the duplicate guard go on recognising it. Whether it is untouched is decided against a snapshot taken when the editor opened, not a rebuilt URI: the structured form does not round-trip one byte for byte, so comparing rebuilt strings would call every profile modified the instant it was opened.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 131f1e9d97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…369 review) Codex found three P1 defects in the connect-before-save flow, all of them real. The editor header's own close button is a plain button, not a Radix close primitive, so it never reached the dialog's onOpenChange and never reached closeEditor either. Closing from there dropped the only reference to a session still open in the backend; opening another editor then cleared it for good. It routes through closeEditor now, like every other way out. Every connection field stayed editable behind the save offer. Editing one and then saving would have written a profile describing one server while the live connection handed over beside it described another — so every later reconnect and every restored tab would target the wrong one. The offer now takes those fields off screen: answering it closes the dialog, so nothing edited there could ever apply to the connection already open. Only the name, folder and colour remain, because naming the thing is the question being asked. persistEditorProfile also takes the tested URI and SSH config explicitly, so the profile cannot drift from the connection even if a field is ever exposed here again. The already-active guard keyed on the untouched profile rather than the saved one. Editing any field made it undefined, so the guard was skipped — and since saving still writes back onto the same profile id, which addActiveConnection dedupes on, the user would have been left on the old session with the new one leaked and the profile overwritten under it. It keys on the saved profile now, edited or not. Fixing the third exposed a fourth: the only outlet for `error` sits in the manager pane behind this modal, so nothing reported while the editor is open could reach the person it was written for. That predates this branch — a missing display name has always failed silently on save — but the new flow leans on it, so the editor has its own outlet now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bc51689a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…review) Dismissing the editor while connect_db was still in flight closed the dialog with nothing pending, so there was nothing to adopt. When the call then succeeded it set pendingSave on a dialog that was no longer rendered — an invisible handle on a live session, which the next editor silently discarded. The session stayed open and unreachable. Every attempt now carries the editor's generation, bumped whenever the editor closes. A connection that comes back to a changed generation is disconnected rather than held, and a failure that comes back to one is dropped instead of painted into a dialog nobody is looking at. Blocking dismissal while connecting was the other way to close this, and it is worse: server selection can take thirty seconds, and holding someone in a modal for that long to protect an invariant they cannot see is not a trade worth making. Walking away from an attempt is a legitimate way to cancel it, so it now cancels it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64b48e4542
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…CI) This test asked waitFor to confirm analyze_schema had been *called*, then read the option list on the very next line. Issuing the call says nothing about its promise having resolved or the state having been committed, so the read was a race — one that wins on an idle machine and loses under a loaded runner, where it failed as expected [ '_id', '__custom__' ] to include 'email' It now waits on the options themselves. Unrelated to the connection work on this branch; it is here because it blocked the branch's CI and the diagnosis was certain. Confirmed by holding the mocked resolution back 400ms: the old form fails with exactly the error above, the new one passes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4030f18495
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…annot come back from (#369 review) Four more from review, two of them about identity outliving the session that owns it. A trial connection's tabs were being persisted. toPersistedTab rewrites a tab id into profile: space whenever the connection has a profileId, and an ephemeral id is a non-empty string, so tabs opened on a never-saved connection came back after a restart as profile:ephemeral:<uuid> — which handleReconnectProfile resolves against load_connection_profiles, where it can never appear. Those tabs would have sat on "profile missing" forever. They are dropped at save time now, like export tabs, since there is genuinely nothing to reconnect them to. The prefix is defined once, in persistence.ts, beside the rule that enforces it. Leaving during a save adopted the connection under its throwaway id while the write carried on and handed the same session over again under the saved profile's id. addActiveConnection drops the second by connection id but set_connection_meta does not, so the backend ended up describing an identity the frontend had not agreed to. A save is a local encrypted write measured in milliseconds, so the honest fix is to not let the editor be dismissed mid-write rather than to unpick the result afterwards. One Escape arrives twice — Radix dismisses the layer from its document listener and useEscapeClose fires from window, both reading the same pendingSave because neither has re-rendered. Handing a connection over is not idempotent on the App side, so it happened twice: metadata broadcast, tab rebinding and refresh, all doubled. Adoption now goes through one funnel guarded by a ref, which settles synchronously where state would not. And the display name stays editable while the offer is up, so clearing it and choosing "Don't save" handed the app a nameless connection — a blank row in the sidebar that pinned and favourite lookups cannot resolve. Adoption falls back to the suggested name, then to the masked URI, and never to nothing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f1edcc888
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…its URI out of storage (#369 review + CodeQL) Two more, one from review and one from CodeQL. split_pane can carry a tab into the new pane, and that moveTabId obeys the same rule as move_tab's: a tab the backend never opened must not be named in a mirrored op, or the backend splits into a pane it cannot fill while the frontend moves the tab, and the two pane trees never agree again. Dropping the op entirely is not the fix — split_pane mints pane and split ids, and both reducers mint them from one op stream, so skipping it on a single side would put the id spaces permanently out of step. It is mirrored without the move instead. The pointless-split case needs nothing: the isNoOp gate already returns before this. This bug predates the branch — export and import tabs have always been unmirrored and have always been draggable — but making a trial connection's tabs unmirrored turns a corner case into an ordinary one, which is reason enough to fix it here. CodeQL then flagged js/clear-text-storage-of-sensitive-data, twice, and was right to. The adoption fallback added in the previous commit used maskUriPassword on the connection string, and that name flows to pinned and favourite state, which writes it to localStorage. Masking the password is not enough: user@host is still a credential and an infrastructure detail, sitting in clear text. It falls back to a literal now. That is also the better label — the branch is only reached when there is no host to name the connection after, so there was nothing recognisable in the URI to show anyway.
CodeQL: two high alerts, both mine, both fixedFlagging this in the open rather than letting it pass as a check that went green on its own. The run against CodeQL is right, and this is not just a conservative taint rule. Masking the password still leaves Fixed in The test that covers this is the existing "never hands over a nameless connection", strengthened to use a URI carrying real-looking credentials and to assert the resulting name contains neither the user nor the password. I nearly shipped a second test here that was worthless — it cleared the name before pressing Connect, so the offer's own prefill filled it back in and the fallback never ran; it passed with the bug reintroduced. Deleted rather than kept. The remaining test fails when the fallback goes back to the masked URI. The other |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da2359180e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t a review fix (#369) Reverts the split_pane mirroring change and its test. The CodeQL fix in that commit stays. Codex was right that mirroring the split without its move_tab_id does not make the trees agree: the backend's new pane is empty, so the first persisted tab opened there and closed again folds the backend's pane and not the frontend's. Panes do fold when emptied, so the scenario is real. But every remaining option is worse than leaving it: - mirrored WITH the move, the backend cannot resolve the id and splits into an empty pane anyway — identical end state; - not mirrored at all, the frontend gains a pane the backend never mints, and every pane id after it is minted against a different tree; - refused outright, both trees stay identical — but splitting an export tab into its own pane stops working, and there is a test from #97 that depends on it doing so. That last one is a real, tested, user-visible behaviour, and taking it away is a product decision rather than something to slip into a connection PR. The divergence is also not new: export and import tabs have been unmirrored since #326 and have always been draggable, and the #97 test creates exactly this state today. What this branch does change is how often it is reachable, since a tab on an unsaved connection is a far more ordinary thing to drag than an export tab. That is worth tracking on its own rather than fixing three different ways in a review loop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 204169b975
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…#369 review) The fields go off screen once the save offer is up, but they stay live for as long as connect_db takes — which can be a server-selection timeout's worth of seconds. Edit the URI in that window and the offer came back describing a host nobody had connected to: the name was suggested from the form's newer value, and adoption read its fallback name and its connection mode from there too. The offer now carries the editor exactly as it stood when Connect was pressed, and everything it says about the connection is read from that: the suggested name, the fallback name, the connection mode, and the mcp flag on the saved profile. Only the name, folder and colour — the three things still on screen, and the only ones the offer is asking about — are read live. The alternative was freezing or hiding the form for the duration of the attempt. Both change what the dialog looks like while someone is waiting on it, and nothing in this session has been able to run the app to see the result, so the fix that alters no layout is the one to make blind. One judgement worth flagging: a connection mode chosen mid-attempt is discarded with the rest. That follows the rule, but read-only is a safeguard, and there is an argument that the newest choice should win on safety grounds even though it belongs to an attempt that was never made. The test says which behaviour is deliberate so the other one cannot arrive by accident.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41887aa6bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…review) Pins and favourites are stored by connection name and resolved on the way back by ensureConnection, which looks that name up among saved profiles. An unsaved connection has no profile to resolve to, so a pin or favourite on it — or on any database or collection inside it — could only ever come back as "no saved connection". A dead shortcut the user has to work out how to clear is a poor reward for trying the product out without committing to it. Guarded in handleTogglePin and handleToggleFavorite rather than at the six menu items that build these entries: one choke point cannot be half-applied, and connection, database and collection shortcuts all fail for the same reason. Only adding is refused — removing keeps working whatever the entry points at, so a shortcut can never become impossible to clear. The reason is said out loud rather than left as a click that appears to do nothing, which meant a new string in the three catalogues. Mutation-checked both ways: a guard that always allows fails the refusal test, and one keyed on merely having a profile id fails the test that an ordinary saved connection can still be pinned — every saved connection has a profile id too, so the marker is what has to be read.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f31a2fc50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ts by connection (#369 review) Two follow-ons, each finding the half of an earlier fix that was left undone. Hiding the connection fields behind the save offer left the URI preview and the Export URI button on screen, because those sit beside the name above the hidden region. Built from the live form, they showed — and would have exported — a URI nobody connected to, directly under a banner reading "Connected". The previous commit made the *saved* URI right and left the *shown* one wrong, which is arguably worse than neither. The editor is now reset to the tested configuration on success, so the preview, the export and the save all describe the same connection. Name, folder and colour stay live: they are the three things still on screen, and the only ones the offer is asking about. The shortcut guard then rediscovered its connection by name, and names are not identities here — duplicate profile names are supported, and a trial connection's name is editable right up to the moment it is adopted. A trial connection sharing a name with a saved one would find the saved one first, allow the pin, and store a shortcut that resolves to the wrong server. The id of the connection the shortcut is being made from is passed through instead, from all six places that build one. Both mutation-checked: reset the editor from the live form rather than the snapshot and the preview test fails; rediscover eligibility by name and the duplicate-name test fails.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c4948566b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e's name (#369 review) The previous commit stopped shortcuts being made FOR an unsaved connection. This stops an unsaved connection answering shortcuts made for someone else. Every stored pin, favourite and quick-connect target is a name, and an unsaved connection's name is editable right up to the moment it is adopted — so it can hold a saved profile's name. Sidebar's ensureConnection returns the first active connection matching a name before it consults saved profiles, and App's handleQuickConnect accepts a name match too, so a shortcut the user made for their saved server would have run against the one they were only trying out. Both now skip ephemeral sessions when resolving a name; matching by profile id is untouched, since that is an identity rather than a label. Escape was also still reaching the editor while the export or new-folder dialog was on top of it. Those are Radix layers that dismiss themselves, but this listener is on window, so the same keypress arrived here too. That was harmless while Escape only closed a dialog. It is not harmless now that the editor's Escape answers the save offer: dismissing an export preview would have silently chosen "Don't save" and closed the manager underneath it. Both listeners stand down while a nested layer is open. Testing the quick-connect half needed a seam: the App suite mocks both the Sidebar and the ConnectionManager, so the mocks gained a button that adopts a trial connection and one that quick-connects a profile — the two real paths this bug needs to meet. All three fixes are mutation-checked, each failing only its own test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c866ab2eb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ery state (#369 review) Two more places where a name was doing an identity's job. Saved queries, the default query and query history are all keyed on the connection's display name, and an unsaved connection can be given a saved profile's. Opening a collection therefore ran the saved profile's default query against the trial server, and the trial's own history wrote back into the saved profile's. The key is now the display name for an ordinary connection and the ephemeral id for an unsaved one, so the two cannot meet. A trial session's entries are orphaned when it ends, which is the right outcome for a connection the user declined to keep. DocumentViewer takes that key as a prop rather than deriving it, since `connectionName` is also what it displays. The prop is optional and defaults to the display name — the value these stores were keyed on before it existed — so a caller with no opinion gets the old behaviour rather than a silently different one. Making it required was the first attempt and it churned thirty test fixtures for no benefit. The shortcut guard from two commits ago was also only half a guard. It refused additions, on the reasoning that removals must keep working or a shortcut could become unclearable. But shortcut keys hold only a name, so a saved pin looks "already pinned" from a trial row sharing that name, `wasPinned` came back true, and the guard stood aside while `togglePinItem` deleted the saved profile's shortcut — under a menu item helpfully labelled "Unpin". The reasoning was wrong because a shortcut can never have been created from a trial row in the first place, so there is none there to clear. Both directions are refused now.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…#369 CodeQL) generateUUID fell back to Math.random when crypto.randomUUID was unavailable. That was unremarkable while these ids only named saved profiles, but the previous commit made an ephemeral id the namespace a trial session's saved queries and history live under, and CodeQL raised js/insecure-randomness on the path. The impact is nil — nothing authorises on these ids, they never leave the machine, and guessing one grants no access. But weak randomness handed a new job is worth two lines to fix rather than an argument, so the randomness now comes from crypto.getRandomValues where randomUUID is missing. The last resort, for an environment with no crypto at all, is a timestamp and a counter. That is still unique within a session, which is the only property these ids actually need, and unlike the old fallback it does not present itself as random. Uniqueness now has a test of its own: two trial connections in a row must not share an id, since addActiveConnection dedupes on it and a collision would mean the second connection silently never arrives.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a377e30516
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nd favourite state (#369 review) Three more sites where a display name was standing in for an identity. The command palette still loaded saved queries by name, so searching it with a trial tab open listed a saved profile's queries as actions bound to that tab — and selecting one ran it against the trial server. It loads from the tab's own namespace now. The scope match beside it stays on the display name, because that is what the user typed into the palette. AI conversations were scoped by display name too, so a trial session listed and wrote into a saved profile's history, and `foreignChat` compared the same name and judged those conversations local — meaning a query generated for the saved server could be run against the trial one. AIChatPanel takes the identity as its own prop. What is sent to the agent as context stays the real connection name: that is prose for a model, not a key. Query favourites are refused on an unsaved session. The query lives under the ephemeral key while a favourite is keyed on the display name, so the two could never meet — the favourite was born pointing at nothing and reported the query gone the moment it was followed. The palette test needed two corrections before it was worth anything. It first passed against the mutation because opening the tab loads its default query through the OTHER call site, which was already fixed, so the assertion was satisfied by calls the palette never made; the call list is cleared before the palette opens now. It then found no palette calls at all, because the dynamic loader only runs once at least two characters have been typed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6d91fd72a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tton (#369 review) The generation guard stopped an abandoned attempt from being adopted, but its `finally` still cleared `connecting` unconditionally. So an attempt dismissed while a second was already running would re-enable Connect underneath the live one, and a third click would then put two requests in flight under a single generation — both passing the guard, the later to land overwriting the earlier pendingSave and leaving that connection open with nothing pointing at it. Only the attempt that is still current may release the button now. That alone would have left the opposite trap: an abandoned attempt no longer clears the flag, so a fresh editor opened while one was in flight would inherit a Connect button disabled forever. Opening the editor resets it, which is where the rest of the per-attempt state is already reset. Both halves are load-bearing and the same test catches either being removed — without the guard the button unlocks under a live attempt, without the reset the second attempt can never be started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b48634e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… recheck the profile before saving (#369 review) Three more, two of them places the earlier isolation work did not reach. Query history was still written by display name at all four callers, so merely opening a collection or running a query on an unsaved connection appended to a saved profile's history — while reads, defaults and manually saved queries were already isolated. Half an isolation is arguably worse than none, since the state looks separated until something writes. The shell has its own AI panel, and it kept the display name after the editor's was fixed. A trial shell therefore listed and wrote a saved profile's shell conversations and judged them local, so a command generated for that server could be run against the trial one. The already-active guard also only ran before connecting. The offer can sit on screen indefinitely, and this connection is not announced until it is answered, so another window can connect the same profile in the meantime. Saving then overwrote the profile and handed over a second live id that App drops as a duplicate while still publishing its metadata — the visible session pointing at the old server under a profile now describing the new one, and this connection unreachable. It rechecks before persisting. Not covered by a test: MongoShell passing the identity down to its AI panel. Reaching `list_chats` there needs the Radix dropdown to open, which that suite has no mock for, and adding one file-wide would change 54 existing tests. Both ends of the value are tested — App computing it, AIChatPanel consuming it — but the one-line pass-through between them is verified by reading, not by a failing test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7385899c87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ion the save cannot claim (#369 review) Two more. The favourite guard decided "unsaved" by testing the query store key for the ephemeral prefix. That key is the display NAME for an ordinary connection, and display names are unrestricted — so a saved connection called "ephemeral:something" was locked out of its own favourites. It now takes an explicit flag, computed from the profile id, which is the only thing that actually knows. Every other ephemeral check in the tree already reads a profile id; this was the one site inferring it from a shape. The already-active recheck also ran before an asynchronous save, so another window could take the profile during the write. There is now a second check after it. That second check does NOT close the race, and the comment in the code says so. By the time it runs the profile has already been overwritten; only the backend could hold a profile for the whole operation. What it prevents is the worse half — handing over a connection that addActiveConnection drops as a duplicate, leaving this session live, unreachable and invisible. The connection is released instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a08ba1662
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…to see a claim (#369 review) The check added in the previous commit could not do its job. A handler suspended on an await resumes inside the render that started it, so `activeConnections` there is the array as it was when the click happened — even though React has since re-rendered with another window's connection in it. A post-await check against the prop therefore cannot see the one thing it exists to catch. It was a guard that looked like protection and was not, which is worse than not having added it. Both post-await checks read a ref synchronised to the latest prop now. The untouched-profile branch had no recheck at all: another window can claim the profile while `connect_db` is pending, and handing the id over then gives App a row it drops as a duplicate while set_connection_meta still publishes it. It rechecks and releases the session. The previous test could not have caught this — it changed `activeConnections` before the click, so the handler's own render already had the new array. The two added here re-render DURING the await, which is what actually happens, and both fail against the prop read. None of this closes the race itself; that is #371 and needs the backend to hold the profile.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d29038c11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dentity (#369 review) Scoping AI chats on the ephemeral id left the two rename paths behind: they still retargeted by display name, so a trial connection's conversations stayed on the old namespace after a rename and its tabs read them as foreign. Worse, if that display name matched a saved profile, the rename moved THAT profile's chats instead. This is a regression the chat-scope commit introduced rather than a site it merely failed to reach. Before it, chats were written and retargeted under the same key — wrong for isolation, but consistent. Isolating the write and not the retarget is what broke rename. Both paths use the same key helper the panel does now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa66bd5f6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…New Connection (#369 review) The offer decided a name was an untouched placeholder by comparing it to BLANK_CONN.name. A saved profile literally called "New Connection" therefore had its name replaced with the suggested hostname the moment any other setting was edited and Connect succeeded — and Save then wrote that rename, without the user ever touching the field. "New Connection" is a placeholder only where the app puts it, which is a blank editor. Anywhere else it is a name somebody chose. The check is scoped to editMode 'new' now. An empty name still takes the suggestion in any mode, since the alternative is handing the app a nameless connection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c942548381
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…es, not after (#369 review) Dismissal invalidated an in-flight connect through a passive effect watching showEditDialog. Passive effects run after the commit, while a connect_db promise resolves on a microtask — so a connection landing in that window read the old generation, passed the abandonment check, and was stored in pendingSave on an editor that had already gone. The next editor then cleared that handle without disconnecting it, which is the exact leak the generation guard was added to prevent. closeEditor advances it synchronously now. The effect stays for the close paths that do not go through there; advancing twice is harmless, since only equality matters. Not covered by a test, and I tried. Reproducing the window needs the promise continuation to run before React flushes the effect, and in jsdom it does not: fireEvent wraps in act() which flushes, and dispatching the event raw and draining microtasks by hand still lets the effect win. A test was written, watched to pass with the fix reverted, and deleted rather than kept as coverage it does not provide. The reasoning stands on its own — a passive effect cannot be relied on to run before a microtask — but it is reasoning, not a failing test.
3 participants
Closes #364.
The problem
handleConnectClickoperated onselectedProfile, so Connect was only ever available for a profile that already existed in the list, and the editor's own actions were Create and Save. A newcomer with a connection string had to name it, choose a folder and commit it to the vault before finding out whether it worked.Saving is what engages the vault, so the first thing a trial user did was write an encrypted credential to disk for a connection nobody had shown to work yet — and when it failed, they were left holding a saved profile that did not connect.
What changed
The editor has a Connect of its own that runs on the fields as they stand and saves nothing.
cluster0forcluster0.ab12c.mongodb.net, the hostname for anything else. A name the user typed is left alone.The failure banner is deliberately separate from the Test Connection panel: a failed connect never ran the four-stage checklist, and borrowing it would have been a fiction. Both now share one
describeConnectionErrorhelper so the classification lives in one place.The two things worth reviewing
An unsaved connection needs an identity. It travels under an
ephemeral:<uuid>, unique per connection rather than one shared sentinel —addActiveConnectiondedupes onprofileId, so a shared one would silently drop a second trial connection as a duplicate of the first.It must not be possible to strand one. The connection is open in the backend before the save question is answered, so every path out of the editor adopts it — Escape included. Declining to save is a decision about the profile, not the session.
closeEditoris the single funnel, and the Escape route has its own test.An untouched saved profile is not treated as anonymous: it connects under its own id, on the exact string it stored, so tab rebinding and the duplicate guard go on recognising it. Whether it is untouched is decided against a snapshot taken when the editor opened, not a rebuilt URI — the structured form does not round-trip one byte for byte (
mongodb://mockcomes back asmongodb://mock:27017/?directConnection=true), so comparing rebuilt strings would call every profile modified the instant it was opened. Edited fields do get the offer, since Save in edit mode updates that same profile rather than filing a second copy.Testing
12 new tests. Each was mutation-checked — the fix was broken seven ways and every mutation was caught by its intended test, and only by it:
closeEditorstops adoptingtsc --noEmitclean,npm run i18n:checkgreen, and the full frontend suite is back to its known baseline (the same 5 environment-dependent failures inExportView,StatsCards,imageTypesandshellDoc.bundlethat pass in CI).One thing the i18n coverage scanner caught in review of my own diff:
hint: 'hintKey' in info ? …reads as untranslated UI copy, because a string literal sits directly afterhint:. Pulled into a namedhintKeyconst, with a comment saying why.Not verified in the app
The machine has been locked for this whole session, so none of this has been exercised in a running MQLens — the Tauri runtime is unavailable and the frontend alone stops at "Loading…". Everything above is tests, types and reading. The footer now carries three buttons where it carried two, and that is exactly the kind of thing tests do not check.