fix(instance): connect on deep-link to refresh modules - #6230
Conversation
InstancePage now calls checkConnectToInstanceAction before fetching instance data so Redis modules are refreshed via GET /connect. This fixes Vector Search showing unavailable when deep-linking to /0/vector-search on preconfigured Docker databases. Fixes redis#6080
|
|
![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: 81976eb9fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| dispatch(fetchRecommendationsAction(connectionInstanceId)) | ||
| }, | ||
| undefined, | ||
| contextInstanceId !== connectionInstanceId, |
There was a problem hiding this comment.
In the RedisStack build, RedisStackRoutes first connects, calls setConnectedInstanceId, and pushes /:id/browser; those routes are wrapped by ProtectedRoute, which redirects to Pages.home whenever connectedInstance.id is empty. On this first InstancePage mount the app context is still empty, so this reset flag is true and checkConnectToInstanceAction synchronously dispatches resetConnectedInstance before the async connect completes. That clears the id after the route was admitted, causing ProtectedRoute to immediately redirect back home and preventing /0/browser from staying open in the RedisStack path.
Useful? React with 👍 / 👎.
Only reset connectedInstance when switching away from a different DB. Redis Stack sets the connected id before routing into InstancePage; resetting it cleared ProtectedRoute and bounced back to home.
|
Addressed Codex feedback: Redis Stack sets Added a unit test covering the same-id / no-reset case. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 2ac110e. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ac110e7c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| () => { | ||
| dispatch(fetchConnectedInstanceAction(connectionInstanceId)) | ||
| dispatch(getDatabaseConfigInfoAction(connectionInstanceId)) | ||
| dispatch(fetchConnectedInstanceInfoAction(connectionInstanceId)) | ||
| dispatch(fetchRecommendationsAction(connectionInstanceId)) | ||
| }, |
There was a problem hiding this comment.
When a user opens a bookmarked/direct instance route and GET /databases/:id/connect fails (for example a temporarily unavailable DB returning 424), this success callback never runs and no failure callback is provided. Since the old unconditional fetchConnectedInstanceAction is now only dispatched here, a fresh / :id /vector-search load leaves connectedInstance.loading as undefined, which VectorSearchPageRouter treats as loading forever; Browser children also keep connectedInstance.id empty. Please update the failure path to either redirect/show a terminal state or still fetch/reset the connected instance state.
Useful? React with 👍 / 👎.
Always fetch connected instance/config/info after connect succeeds or fails so connectedInstance.loading settles and Vector Search does not spin forever. Drop unused getRecommendations import in the spec.
|
Addressed Bugbot findings on
|
| // Only reset when switching away from a different connected DB. | ||
| // Redis Stack already set connectedInstance.id before routing here; | ||
| // resetting would clear it and ProtectedRoute would bounce to home. | ||
| // Always load instance data after connect attempt (success or fail) so | ||
| // connectedInstance.loading settles and Vector Search does not spin forever. | ||
| dispatch( | ||
| checkConnectToInstanceAction( | ||
| connectionInstanceId, | ||
| loadInstanceData, | ||
| loadInstanceData, | ||
| Boolean(connectedInstanceId) && | ||
| connectedInstanceId !== connectionInstanceId, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Making this unconditional regresses the existing entry paths though. The DB picker (databases-list/methods/handlers.ts), Redis Stack router (RedisStackRoutes.tsx), and the navigation popover (InstancesList.tsx) all dispatch checkConnectToInstanceAction before navigating and set connectedInstance.id in their success callbacks - so with this change every normal DB open issues a second /connect. That endpoint is not cheap on the backend (DatabaseConnectionService.connect): it re-probes modules and server version, rewrites the DB record, runs INFO + CLIENT LIST, re-runs recommendation checks, and emits a connection analytics event - all doubled per open, and the analytics get double-counted. It also serializes the four data fetches behind the extra round-trip on every navigation, not just deep links.
Since those paths all arrive with connectedInstance.id already equal to the route id, you can keep the fix and skip the redundant call:
| // Only reset when switching away from a different connected DB. | |
| // Redis Stack already set connectedInstance.id before routing here; | |
| // resetting would clear it and ProtectedRoute would bounce to home. | |
| // Always load instance data after connect attempt (success or fail) so | |
| // connectedInstance.loading settles and Vector Search does not spin forever. | |
| dispatch( | |
| checkConnectToInstanceAction( | |
| connectionInstanceId, | |
| loadInstanceData, | |
| loadInstanceData, | |
| Boolean(connectedInstanceId) && | |
| connectedInstanceId !== connectionInstanceId, | |
| ), | |
| ) | |
| if (connectedInstanceId === connectionInstanceId) { | |
| loadInstanceData() | |
| } else { | |
| dispatch( | |
| checkConnectToInstanceAction( | |
| connectionInstanceId, | |
| loadInstanceData, | |
| loadInstanceData, | |
| Boolean(connectedInstanceId), | |
| ), | |
| ) | |
| } |
There was a problem hiding this comment.
Good catch on the double /connect - adopted the same-id gate so picker / Redis Stack / nav keep a single connect, and deep links still refresh modules. Included in 2f3b6bf.
| ] | ||
|
|
||
| expect(store.getActions().slice(0, expectedActions.length)).toEqual( | ||
| expect(testStore.getActions().slice(0, expectedActions.length)).toEqual( |
There was a problem hiding this comment.
The reset / no-reset branches are well pinned (nice negative test for the Redis Stack case), but the behavior this PR exists to create ended up unasserted: the old expected-actions list contained setConnectedInstance, getDatabaseConfigInfo, setConnectedInfoInstance, and getRecommendations, and after this rework those only fire inside the connect success callback - which lands after the slice(0, expectedActions.length) window, so nothing checks them anymore. Deleting loadInstanceData (or passing undefined for both callbacks) keeps the whole suite green.
Could you add three tests to lock the new flow in:
- Success path - render with the default store and
await waitFor(() => expect(store.getActions()).toEqual(expect.arrayContaining([setConnectedInstance(), getDatabaseConfigInfo(), setConnectedInfoInstance(), getRecommendations()]))). The MSW handler for/databases/:id/connectalready returns 200, so the callback runs in tests. - Fail path - override the connect handler with a 503 via
mswServer.use(...)(same pattern asinstanceConnectionLost.spec.tsx) and assertsetDefaultInstanceFailureis dispatched and the four fetches still fire - this is the guarantee of commit 0ce6401, currently uncovered. - Fresh deep link (the literal [Bug]: Docker Redis Insight does not detect Redis Search when opening a preconfigured database through a direct route #6080 state) - seed
connectedInstance.id = '', spy on the connect endpoint (samejest.fn()MSW-spy pattern asgetDatabasesApiSpy) and assert it's called once andresetConnectedInstanceis not dispatched.
Two smaller things while you're in here: the positive assertion in the same-id test (toContainEqual(setDefaultInstance())) doesn't actually prove the connect flow ran - fetchConnectedInstanceAction dispatches setDefaultInstance() too, so a full revert to the old direct-fetch code still passes it; asserting setDefaultInstanceSuccess() or the connect spy pins it properly. And 'prevId' could come from faker per our testing conventions.
(If you adopt the same-id gate suggested on InstancePage.tsx, test 1 doubles as its coverage - same-id renders should then assert the fetches fire without a connect call.)
There was a problem hiding this comment.
Added the three coverage cases (success / fail / fresh deep link), tightened the same-id path to assert no connect call + the four fetches, and switched prevId to faker. Also in 2f3b6bf.
| checkConnectToInstanceAction( | ||
| connectionInstanceId, | ||
| loadInstanceData, | ||
| loadInstanceData, |
There was a problem hiding this comment.
Running loadInstanceData on the fail path is a good catch, but the VS page can still load forever: if the GET /databases/:id inside fetchConnectedInstanceAction itself fails (e.g. a stale or invalid id in the url), its catch never sets connectedInstance.loading back to false.
I think we can add a one line guard against this inside that catch (slices/instances/instances.ts, ~line 759):
dispatch(setConnectedInstanceFailure())
There was a problem hiding this comment.
Agreed - i added setConnectedInstanceFailure() in the fetchConnectedInstanceAction catch so connectedInstance.loading settles on a bad/stale id. Also in 2f3b6bf.
Skip GET /connect when already on the same instance id (picker/Redis Stack), clear connectedInstance.loading on fetch failure, and lock the deep-link success/fail flows with tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f3b6bf77f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } catch (_err) { | ||
| const error = _err as AxiosError | ||
| const errorMessage = getApiErrorMessage(error) | ||
| dispatch(setConnectedInstanceFailure()) |
There was a problem hiding this comment.
When a direct/bookmarked instance route reaches this catch path because the follow-up GET /databases/:id fails (for example the DB was deleted or the API returns 404/5xx after the connect attempt), this now marks connectedInstance.loading as false while leaving the reset/default instance data (modules: []) in place. VectorSearchPageRouter treats loading === false plus no RediSearch module as a normal “RQE not available” result, so users get a misleading unsupported page instead of an error or redirect. Fresh evidence beyond the earlier connect-failure comment is that this newly added failure action now explicitly settles the compatibility guard after the failed instance fetch.
Useful? React with 👍 / 👎.
3 participants
Summary
checkConnectToInstanceActionwhen mountingInstancePage, matching the DB picker / Redis Stack entry path.GET /databases/:id/connectbefore fetching instance data used by Vector Search guards.modules: []) opened via/0/vector-searchincorrectly showing Redis Search as unavailable.Fixes #6080
Test plan
InstancePage.spec.tsx— 9/9 passedmodules: []; deep-link/0/vector-searchtriggers connect and populatessearchmoduleidx:trie), not the RQE unavailable empty state/0/browserstill loads after deep-link / picker entryNote
Medium Risk
Changes core instance mount and connect sequencing used by every database route; behavior differs for deep links, picker entry, and DB switches, though covered by expanded tests.
Overview
InstancePage now mirrors the DB picker / Redis Stack flow: when the route’s database id does not match
connectedInstance.id, it runscheckConnectToInstanceAction(includingGET /databases/:id/connect) before loading config, info, and recommendations; when ids already match, it skips connect and only loads that data. Connect callbacks run on both success and failure soconnectedInstance.loadingclears (e.g. Vector Search no longer spins on deep links with stale emptymodules).resetConnectedInstanceis only requested when switching from another connected id, not on fresh deep links or same-id entry.fetchConnectedInstanceActionnow dispatchessetConnectedInstanceFailurewhen the instance fetch fails.Tests add
connectDatabaseApiSpy, MSW overrides for failed connect, and cases for same-id skip, successful/failed connect, DB switch reset, and single connect on deep link; slice tests cover the new failure dispatch.Reviewed by Cursor Bugbot for commit 2f3b6bf. Bugbot is set up for automated code reviews on this repo. Configure here.