Cleanup HTTP client headers and testing code#55
Conversation
📝 WalkthroughWalkthroughThis PR consolidates HTTP request handling across the codebase by introducing type-safe request configuration, refactoring the core HTTP client to merge headers centrally, and migrating all API clients (OSM, TDEI, Workspaces) to use the new standardized types and header management patterns. ChangesHTTP Client Request Handling Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
services/http.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. services/osm.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. services/tdei.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/osm.ts`:
- Around line 521-535: The _send method can reuse a stale Authorization header
because `#setAuthHeader` only adds a header when auth exists but never removes an
old one; update the code so that before calling `#setAuthHeader` (or inside
`#setAuthHeader`) you explicitly remove any existing Authorization header when
there is no current token — for example, clear this.headers['authorization'] (or
call a new clearAuthHeader helper) if `#tdeiClient.tryRefreshAuth` indicates auth
is not present, then only set the header when a fresh token is available; make
this change in the _send flow (and optionally in `#setAuthHeader`) to ensure stale
bearer values are not sent.
In `@services/tdei.ts`:
- Around line 429-440: refreshToken() can clear this.#auth but headers may still
contain a stale Authorization value when _send() forwards the request; before
calling super._send(url, method, body, config) explicitly sync the Authorization
header to current this.#auth (set it when present, remove/clear it when auth is
empty) so no stale bearer is sent, and apply the same header-sync logic at the
other send path referenced (the second _send call site).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea4a8759-b770-4d35-8ac8-23510a92af8f
📒 Files selected for processing (4)
services/http.tsservices/osm.tsservices/tdei.tsservices/workspaces.ts
| override async _send( | ||
| url: string, | ||
| method: string, | ||
| body?: HttpBody, | ||
| config?: FetchConfig, | ||
| ): Promise<Response> { | ||
| try { | ||
| await this.#tdeiClient.tryRefreshAuth(); | ||
| this.#setAuthHeader(); | ||
|
|
||
| const requestOptions = { | ||
| credentials: 'include' | ||
| } | ||
| const requestOptions: FetchConfig = { | ||
| credentials: 'include', | ||
| }; | ||
|
|
||
| return await super._send(url, method, body, { ...requestOptions, ...config }); |
There was a problem hiding this comment.
Clear stale Authorization header before sending requests.
On Line 529, _send reuses #setAuthHeader(), but that helper only writes when auth is complete and never clears an old token. After auth is cleared, this can keep sending a stale bearer value.
Suggested fix
`#setAuthHeader`() {
- if (this.#tdeiClient.auth.complete) {
- this._requestHeaders.Authorization = 'Bearer ' + this.#tdeiClient.auth.accessToken;
- }
+ this._requestHeaders.Authorization = this.#tdeiClient.auth.complete
+ ? `Bearer ${this.#tdeiClient.auth.accessToken}`
+ : '';
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/osm.ts` around lines 521 - 535, The _send method can reuse a stale
Authorization header because `#setAuthHeader` only adds a header when auth exists
but never removes an old one; update the code so that before calling
`#setAuthHeader` (or inside `#setAuthHeader`) you explicitly remove any existing
Authorization header when there is no current token — for example, clear
this.headers['authorization'] (or call a new clearAuthHeader helper) if
`#tdeiClient.tryRefreshAuth` indicates auth is not present, then only set the
header when a fresh token is available; make this change in the _send flow (and
optionally in `#setAuthHeader`) to ensure stale bearer values are not sent.
| override async _send( | ||
| url: string, | ||
| method: string, | ||
| body?: HttpBody, | ||
| config?: FetchConfig, | ||
| ): Promise<Response> { | ||
| try { | ||
| if (this.#auth.needsRefresh) { | ||
| await this.refreshToken(); | ||
| } | ||
|
|
||
| return await super._send(url, method, body, config); |
There was a problem hiding this comment.
Resync Authorization header on every send and clear it when auth is empty.
refreshToken() can clear auth on Line 195, but requests at Line 440/Line 536 may still carry a stale bearer because the header helpers never clear old values. Please make header sync explicit before dispatch.
Suggested fix
`#setAuthHeader`() {
- if (this.#auth.complete) {
- this._requestHeaders.Authorization = 'Bearer ' + this.#auth.accessToken;
- }
+ this._requestHeaders.Authorization = this.#auth.complete
+ ? `Bearer ${this.#auth.accessToken}`
+ : '';
}
override async _send(
url: string,
method: string,
body?: HttpBody,
config?: FetchConfig,
): Promise<Response> {
try {
if (this.#auth.needsRefresh) {
await this.refreshToken();
}
+ this.#setAuthHeader();
return await super._send(url, method, body, config);
}
catch (e: unknown) {
...
}
}
`#setAuthHeader`() {
- if (this.#auth.complete) {
- this._requestHeaders.Authorization = 'Bearer ' + this.#auth.accessToken;
- }
+ this._requestHeaders.Authorization = this.#auth.complete
+ ? `Bearer ${this.#auth.accessToken}`
+ : '';
}Also applies to: 526-537
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/tdei.ts` around lines 429 - 440, refreshToken() can clear this.#auth
but headers may still contain a stale Authorization value when _send() forwards
the request; before calling super._send(url, method, body, config) explicitly
sync the Authorization header to current this.#auth (set it when present,
remove/clear it when auth is empty) so no stale bearer is sent, and apply the
same header-sync logic at the other send path referenced (the second _send call
site).
1 participant
This cleans up the abstraction for HTTP request header configuration in the various API clients by merging headers instead of overwriting them.
This also cleans up some old HTTP testing code which resolves some potential auth token refresh issues in TDEI calls.
Summary
This PR refactors HTTP client header handling across the frontend services to merge headers instead of overwriting them, and removes legacy testing code. The changes introduce type-safe configurations for HTTP requests and centralize header management in the base HTTP client.
Changes by File
services/http.ts
FetchConfig(based onRequestInitwith optional typed headers) andHttpBody(union ofBodyInit | object)BaseHttpClient._sendpipeline to merge default headers with per-request headers fromFetchConfigContent-Typeheader to allow browser defaults, and JSON stringification is determined by both resolvedContent-Typeand runtime body type_sendTesthelper method, simplifying the test code cleanup_get,_post,_put,_patch,_delete) to use the newFetchConfigtypeservices/osm.ts
_sendoverride to use typedHttpBodyandFetchConfigparametersAcceptheader fromtext/plainto*/*provisionUserto pass JSON objects directly withContent-Type: application/jsoninstead of manually stringifying_requestHeadersacross multiple methods (workspace/element/node/way retrieval, changeset operations, notes search, export)_sendoverride now callstryRefreshAuth()before sending and re-applies auth headers via#setAuthHeader(), then mergescredentials: 'include'with provided configservices/tdei.ts
_sendoverride signatures in bothTdeiClientandTdeiUserClientto use typedHttpBodyandFetchConfigparameters_sendTestcalls with_getfor dataset downloads (downloadOswDataset,downloadPathwaysDataset)Authorizationheader injection from upload methodsAccept/Authorizationheader handlingservices/workspaces.ts
WorkspacesClient._sendmethod signature to use typedHttpBodyandFetchConfigparameters instead of loose typesKey Improvements
_sendTesttest helper reduces code duplication