do not block on CodyLLMSiteConfiguration (configOverwrites) fetch in initial auth #5799
Conversation
|
cc @RXminuS |
|
Just having a look at another failing test where UI seems to render before it responds to clicks to account tab |
sqs
force-pushed
the
sqs/combined-auth-reqs
branch
5 times, most recently
from
0c2ed91
to
1fb9e9c
Compare
|
@valerybugakov Can you PTAL at the changes I needed to make to the |
sqs
force-pushed
the
sqs/combined-auth-reqs
branch
from
1fb9e9c
to
ac61f1f
Compare
sqs
changed the title
sqs
force-pushed
the
sqs/combined-auth-reqs
branch
4 times, most recently
from
00553ff
to
ef7f310
Compare
…initial auth To authenticate a user and start showing the UI, we actually don't need to fetch the CodyLLMSiteConfiguration (`configOverwrites`). This reduces the number of HTTP requests (albeit they were parallelized somewhat) needed before the user can be authenticated and start seeing the Cody UI. This uncovered some possible "bugs" in the `anthropic.test.ts` autocomplete tests, where they were asserting that the `model` would be set but it actually would not be in real life. In `Provider.maybeFilterOutModel` it sees that `this.configOverwrites?.provider === 'sourcegraph'` in the test, but that should not be true in real life because it’s being passed an `authStatus` that is non-dotcom. The reason the test is bad is because that method gets the auth status from `currentAuthStatus()`, which is dotcom in testing only because of a prior test's mock value.
sqs
force-pushed
the
sqs/combined-auth-reqs
branch
from
ef7f310
to
5eed466
Compare
| expect(getRequestParamsWithoutMessages(provider)).toStrictEqual({ | ||
| ...requestParams, | ||
| // The model ID is ignored by BYOK clients | ||
| model: undefined, | ||
| }) | ||
| expect(getRequestParamsWithoutMessages(provider)).toStrictEqual(requestParams) |
There was a problem hiding this comment.
In the previous version, the test used mockAuthStatus(AUTH_STATUS_FIXTURE_AUTHED) based on isDotCom parameter, which didn't have configOverwrites. This was inconsistent with provider creation arguments and implicitly made it a BYOK provider. Thanks for fixing this!
To authenticate a user and start showing the UI, we actually don't need to
fetch the CodyLLMSiteConfiguration (
configOverwrites). This reduces thenumber of HTTP requests (albeit they were parallelized somewhat) needed
before the user can be authenticated and start seeing the Cody UI.
This uncovered some possible "bugs" in the
anthropic.test.tsautocompletetests, where they were asserting that the
modelwould be set but itactually would not be in real life. In
Provider.maybeFilterOutModelitsees that
this.configOverwrites?.provider === 'sourcegraph'in the test,but that should not be true in real life because it’s being passed an
authStatusthat is non-dotcom. The reason the test is bad is because thatmethod gets the auth status from
currentAuthStatus(), which is dotcom intesting only because of a prior test's mock value.
Test plan
CI & e2e. Sign in and ensure that operation that need the site version (such as autocomplete) still function without an error.
Changelog