fix: sync docstore drive selections#972
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Review: Fix/support-19 Drive PickerSummary: This PR fixes Google Drive token refresh logic in The fix addresses a real user-facing bug (SUPPORT-19) where re-authentication fails due to a missing OAuth config scenario. The changes are minimal and targeted, which is appropriate for a support fix. Critical IssuesNone identified. No security regressions, no multi-tenancy bypasses, and no new unprotected routes were introduced. Major Concerns1. Duplicated fallback logic - The same The original failure mode is:
However, looking at the server implementation ( Consider simplifying to a single catch-only approach, or verifying whether the server ever returns HTTP 200 with // Suggested simplified version:
const handleRefreshAccessToken = useCallback(async () => {
if (!credentialId) return
try {
setIsRefreshing(true)
try {
await oauth2Api.refresh(credentialId)
} catch (error) {
const errorMessage = error.response?.data?.message || error.message || ''
if (errorMessage.includes('Missing required OAuth configuration')) {
await credentialsApi.refreshAccessToken({ credentialId })
} else {
throw error
}
}
getCredentialDataApi.request(credentialId)
setIsTokenExpired(false)
setIsReauthRequired(false)
showSnackbar('Successfully refreshed access token', 'success')
} catch (error) {
// ... existing error handling
} finally {
setIsRefreshing(false)
}
}, [credentialId, getCredentialDataApi, showSnackbar])2. Silent string-matching for control flow - The fallback decision is driven by string matching on error messages (
Minor Issues and Suggestions3.
4. Missing
5. No tests for the new fallback path The new branching logic in Positive Observations
TheAnswer-Specific Checklist
Next Steps
Overall this is a reasonable targeted fix for a production support issue. The major concern is about code clarity and a potentially unreachable branch, not correctness — the fallback via the Review generated by Claude Code PR Review Agent |
|
Scoped this PR to the intended fix only. Removed unrelated AGENTS/opencode changes so the diff now only contains GoogleDrivePicker refresh fallback logic. |
PR Review: Fix/support 19 Drive PickerSummary: This PR adds a fallback mechanism in the Critical Issues1. Silent swallowing of unexpected errors inside the inner try/catch Location: The inner // Current flow (inner catch re-throws an error it just created):
} else if (oauth2Failed) {
throw new Error(oauth2Message || 'Error refreshing access token')
// ^^ caught by the inner catch immediately below
}
} catch (error) {
// ... re-throws the above error
if (!isMissingOauthConfig) { throw error }
}The outer 2. The Location: This is a pre-existing issue that this PR now makes more prominent. The // Line 314 — no organizationId filter, no auth check
const credential = await credentialRepository.findOneBy({ id: credentialId })Any authenticated user who knows a Major Concerns3. Inconsistent response shape detection is fragile Location: const oauth2Message = response?.data?.message || ''
const oauth2Failed = response?.data?.success === false
const isMissingOauthConfig = oauth2Message.includes('Missing required OAuth configuration')The server returns Consider: defining a machine-readable error code (e.g. 4. Success path does not await the fallback call result Location: Both fallback calls to 5. The fallback call also duplicates logic across two branches Location: Lines 320-333 The Minor Issues and Suggestions6. Missing Location: Line 336 getCredentialDataApi.request(credentialId)This is a fire-and-forget call that was present before this PR and continues unchanged. It is not awaited. This is likely intentional (the hook handles state updates asynchronously), but worth documenting. 7. Error message extraction in catch could surface confusing messages Location: Lines 326-327 const errorMessage = error.response?.data?.message || error.message || ''If 8. No test coverage for the new fallback logic The fallback mechanism involves multiple conditional branches. There are no unit tests added for this component. Given the fragility of the string-matching approach, tests covering:
...would significantly reduce the risk of regressions here. Positive Observations
Next Steps
Overall: the fix solves a real user-facing issue (Drive Picker broken when OAuth config is incomplete) but the implementation would benefit from the refactoring above before merging. |
5d5274e to
23cd1cd
Compare
PR Review: fix: add Google Drive OAuth refresh fallbackReviewer: Claude Code (automated review) SummaryThis PR adds a fallback path inside Critical Issues1. Silent double-invocation of Location: The outer The inner // Current: outer catch re-intercepts oauth2Api.refresh errors
} catch (error) {
const errorMessage = error.response?.data?.message || error.message || ''
const isMissingOauthConfig = errorMessage.includes('Missing required OAuth configuration')
if (!isMissingOauthConfig) {
throw error
}
await credentialsApi.refreshAccessToken({ credentialId })
}The intent was to catch the case where Major Concerns2. String-matching on error messages is brittle Location: const isMissingOauthConfig = oauth2Message.includes('Missing required OAuth configuration')
// ... and ...
const isMissingOauthConfig = errorMessage.includes('Missing required OAuth configuration')The fallback decision depends on matching a specific English-language string from the server response. This couples the frontend to the exact wording of a backend error message. If that message ever changes (e.g. a future refactor of A more robust approach would be for the server to return a machine-readable error code (e.g. 3. No user feedback when the fallback path is taken Location: When the fallback to Minor Issues & Suggestions4. Duplicated Location: The string-check 5. Location: const oauth2Failed = response?.data?.success === falseThe 6. No test coverage for the new fallback logic The PR description mentions validation via
These paths are non-trivial (they involve conditional branching and two different API calls) and would benefit from Jest/React Testing Library tests on Positive Observations
Checklist
Next Steps
The fix is directionally correct and should unblock the Drive Picker in affected environments. Addressing Issue #1 and Issue #6 before merge would make the change more robust and maintainable. |
PR Review: fix: add Google Drive OAuth refresh fallbackReviewer: Claude Code (automated review) SummaryThis PR delivers three targeted fixes to the Google Drive integration:
Changes 2 and 3 are solid improvements. Change 1 works correctly but has maintainability concerns worth addressing before merge. Critical IssuesNone. No security regressions, no new unprotected routes, no multi-tenancy bypasses. Major Concerns1. String-matching drives critical control flow without a constant or test Location: const isMissingOauthConfig = oauth2Message.includes('Missing required OAuth configuration')
// ...
const isMissingOauthConfig = errorMessage.includes('Missing required OAuth configuration')The fallback decision depends on matching a substring of a server-defined English error message. The authoritative source lives in message: 'Missing required OAuth configuration: clientId, clientSecret, or refresh_token'If this message ever changes (a wording fix, an upstream Flowise merge, or a refactor adding new fields to the message), the client fallback silently stops working. There is no test to catch this regression. At a minimum, extract the matched substring to a named constant shared between the two checks. Ideally, the server should also return a machine-readable 2. The inner Location: try {
const response = await oauth2Api.refresh(credentialId)
const oauth2Message = response?.data?.message || ''
const oauth2Failed = response?.data?.success === false // <-- always false here
const isMissingOauthConfig = oauth2Message.includes('Missing required OAuth configuration')
if (oauth2Failed && isMissingOauthConfig) { // <-- dead code
await credentialsApi.refreshAccessToken({ credentialId })
} else if (oauth2Failed) {
throw new Error(oauth2Message || 'Error refreshing access token')
}
} catch (error) { ... }The server endpoint ( This is not a correctness problem because the fallback via the Minor Issues and Suggestions3. Silent JSON parse failure in Location: try {
rawItems = JSON.parse(trimmed)
} catch (error) {
rawItems = [] // silently returns no files
}If a user passes a malformed JSON string that looks like an array (starts with 4. Location: getCredentialDataApi.request(credentialId) // not awaitedThis was present before this PR and is intentional (the hook manages async state). However, if this request fails after a successful fallback refresh, the token will not be updated in the component state and the user may see a stale expired-token UI even though the refresh succeeded. This is a pre-existing issue but is worth a follow-up ticket. 5. No test coverage for the new fallback branches The PR description lists 6. Missing Location: The Positive Observations
TheAnswer-Specific Checklist
Next Steps
The DocStoreInputHandler and GoogleDrive.ts changes are ready to merge as-is. The GoogleDrivePicker refresh logic works correctly via the Review generated by Claude Code PR Review Agent |
Summary
Why
Validation