General: Adopt the shared authenticated axios instance in phase components - #2068
Conversation
prompt-shared-state 1.2.0 exports createAuthenticatedAxiosInstance and the shared Patch interface, which the phase components need in order to drop their local axios configs.
Replace the six per-phase axios configs with createAuthenticatedAxiosInstance from @tumaet/prompt-shared-state and drop the five dead local copies of the Patch interface, which is now exported by the shared package. The inlined try/catch logging in the network layers stays untouched: the shared factory only installs the request interceptor, so removing those would drop the error logging entirely.
The local coreServerConfig duplicated the axiosInstance singleton that @tumaet/prompt-shared-state already binds to env.CORE_HOST. The team allocation component already calls the same core endpoint through that singleton, so the local config is removed in favor of it.
|
Warning Review limit reached
Next review available in: 17 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mathildeshagl
left a comment
mathildeshagl
left a comment
There was a problem hiding this comment.
Approving. Good refactor, and the interceptor-equivalence section in the description is the reason this was quick to review.
I checked the claims against the published @tumaet/prompt-shared-state@1.2.0 tarball rather than taking them on trust, and they all hold:
- The factory does
parseURL(baseHost || '')internally, so passingenv.X_HOSTstraight in needs no local|| ''. if (token)is equivalent to the oldgetItem() && getItem() !== '', and thetypeof localStorage !== 'undefined'guard is strictly safer.envreally is a module-load snapshot (normalizeEnv(window.env)evaluated at import), so resolvingparseURLper request instead of once at module load gives the same value. That was the claim most worth checking and it is correct.
The refactor is also complete rather than partial: all seven duplicated setups under clients/ are accounted for, six replaced and one deleted. And Patch is declared in five files and imported by none, so deleting it needed no import rewiring, as you say.
One equivalence the description doesn't cover, worth knowing rather than changing: baseURL moves from an instance default to a request interceptor, so a per-request baseURL override would now be overwritten instead of taking precedence. No call site does that today (the only baseURL occurrences in clients/ are the ones this PR deletes), so it is latent rather than a bug. Absolute URLs passed as url are unaffected either way, since axios ignores baseURL for those.
Cosmetic, take it or leave it: const x = createAuthenticatedAxiosInstance(...) followed by export { x } could be a single export const. It does preserve the shape the files already had, which is a fair reason to keep it.
Agreed on leaving the try/catch wrappers alone until #1985 unblocks.
…instance # Conflicts: # clients/package.json # clients/yarn.lock
2 participants
Summary
Removes the duplicated authenticated axios setup from the phase components and replaces it with the
createAuthenticatedAxiosInstancefactory that@tumaet/prompt-shared-state1.2.0 exports.Details
@tumaet/prompt-shared-stateand@tumaet/prompt-ui-componentsto1.2.0inclients/package.jsonand regeneratedclients/yarn.lock.axios.createplus request interceptor in six phase configs withcreateAuthenticatedAxiosInstance(env.<PHASE>_HOST): assessment, certificate, example, interview,self team allocation, team allocation.
Patchinterface (assessment, example, interview, selfteam allocation, team allocation). Nothing imported them, so no import rewiring was needed; the
interface is now exported by
@tumaet/prompt-shared-statefor future consumers.clients/self_team_allocation_component/.../network/coreServerConfig.tsand pointed itssingle consumer at the shared
axiosInstancesingleton instead of the factory. That singleton isalready bound to
env.CORE_HOST, and the team allocation component calls the very same coreendpoint through it, so reusing it removes a whole file rather than replacing one duplicate with
another. It also brings the sanitized response error logging that the singleton carries.
Interceptor equivalence was verified against the shipped 1.2.0 build before deleting the local code:
localStorage.getItem('jwt_token') && ... !== ''and the factory'sif (token)behave identically, since the empty string is falsy. The factory adds a
typeof localStorage !== 'undefined'guard, which is strictly safer.config.headers.Authorizationandconfig.headers['Authorization']are the same assignment.parseURL(host)once at module load; the factory resolves it perrequest inside the interceptor. Same value either way, because
envis a snapshot normalized atmodule load.
No behavioral regressions found. The only behavior change is the additive response error logging on
the one self team allocation core query, which comes from switching to the shared singleton.
Out of scope: the inlined
try { } catch (err) { console.error(err); throw err }wrappers in thenetwork layers stay untouched.
createAuthenticatedAxiosInstanceinstalls only the requestinterceptor and
logNetworkErroris not publicly exported, so removing them would silently drop allnetwork error logging in the phase components. That cleanup is #1985 and remains blocked.
The dependency bump in the first commit is intentionally identical to the one on the branches for
#1987 and #1988, so those PRs are unblocked by the same version and the three branches merge cleanly
instead of conflicting on
clients/package.jsonandclients/yarn.lock.Reason / Link to issue
Closes #1979
How to Test
cd clients && yarn install --immutablemake lintcd clients/<component> && npx tsc --noEmit -p tsconfig.json && yarn buildfor each of the sixtouched components.
allocation phase. Requests must still carry the
Authorization: Bearerheader and hit thecorrect per-phase host.
PR Checklist