pan3793 commented on PR #7386:
URL: https://github.com/apache/kyuubi/pull/7386#issuecomment-4220069909
Review comments from Claude Code
```
Issues and Suggestions
Bug: Behavioral difference in error handling (Medium severity)
The current axios implementation rejects the promise on non-2xx errors but
still dispatches the auth-required event for 401s. The new fetch implementation
throws for 401 but also throws a generic Error for all
other non-OK responses.
The key difference: the old axios interceptor returns response.data for
ALL successful responses, including those with response.data.code === 503. The
new implementation returns undefined for non-JSON responses.
This is fine for the current API, but worth noting.
More importantly, the old code's error path was:
// axios: error.response exists, caller catches an AxiosError with
.response property
return Promise.reject(error)
The new code throws a plain Error('HTTP error! status: ...'). If any
caller inspects the error shape (e.g., error.response.status), this would
break. From my review of the callers, none do — but it's worth
verifying manually.
Bug: baseURL is not carried over (Medium severity)
The old axios instance was configured with baseURL: '/'. The new fetch
implementation uses relative URLs directly (e.g., fetch('api/v1/sessions')). In
a browser, relative URLs resolve against window.location,
which works if the app is served from the root. But the Vite config sets
base: '/ui/', which means in production the app is served under /ui/.
Relative fetch('api/v1/sessions') from a page at /ui/ would resolve to
/ui/api/v1/sessions, not /api/v1/sessions. With axios, baseURL: '/' ensures all
requests go to /<url> regardless of the page path.
This is likely a bug. The new request.ts should prepend '/' to ensure
absolute path resolution:
let fullUrl = url.startsWith('/') ? url : `/${url}`
Suggestion: Handle empty response body gracefully
The implementation checks content-type to decide whether to parse JSON:
const contentType = response.headers.get('content-type')
if (!contentType || !contentType.includes('application/json')) {
return undefined
}
return response.json()
If the server returns 204 No Content with a JSON content-type header (or
200 with an empty body), response.json() will throw. Consider:
const text = await response.text()
return text ? JSON.parse(text) : undefined
This is defensive but costs almost nothing.
Suggestion: RequestConfig.method should be typed more strictly
method: string // current
method: 'get' | 'post' | 'put' | 'delete' // better
Since all callers use these four methods, a union type catches typos at
compile time.
Nit: params serialization edge case
URLSearchParams.append(key, String(value)) will serialize arrays as their
string representation (e.g., "1,2,3"). The current axios implementation
serializes array params differently (repeated keys:
key=1&key=2&key=3). From the codebase, the params objects passed are
simple key-value pairs, so this shouldn't matter today — but it's a behavioral
difference to document.
Nit: Missing the response.data.code === 503 handler
The old interceptor had a (commented/empty) case for response.data.code
=== 503. It was a no-op, so dropping it is fine, but worth mentioning for
completeness.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
