decouples the google auth from sheet service class and fixed side eff… - #82
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change centralizes Google OAuth token refresh in ChangesGoogle OAuth credential lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GoogleSheetsExecutor
participant GoogleOAuthService
participant GoogleOAuthProvider
participant CredentialPersistence
GoogleSheetsExecutor->>GoogleOAuthService: getCredentials
GoogleOAuthService->>GoogleOAuthService: isTokenExpired
GoogleOAuthService->>GoogleOAuthProvider: refreshAccessToken
GoogleOAuthProvider-->>GoogleOAuthService: refreshed OAuth tokens
GoogleOAuthService->>CredentialPersistence: persist merged credentials
GoogleOAuthService-->>GoogleSheetsExecutor: return credentials
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
apps/http-backend/src/services/token-refresh.service.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/web/app/workflows/[id]/components/ConfigModal.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/nodes/src/common/google-oauth-service.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 |
There was a problem hiding this comment.
Pull request overview
This PR moves Google OAuth token expiry/refresh logic out of GoogleSheetsService into the shared GoogleOAuthService, and updates consumers to rely on the centralized refresh behavior. It also tweaks Google Sheets “clear rows” range formatting and adjusts the web UI to read the credential email from the stored credential config.
Changes:
- Removed token-expiry and token-refresh helpers from
GoogleSheetsServiceand added them toGoogleOAuthService(including auto-refresh duringgetCredentials). - Updated Google Sheets executor logic to stop refreshing tokens itself and to build clear-range strings consistently.
- Updated web UI credential display to use
cred.config.email, and updated backend token refresh job to refresh viaGoogleOAuthService.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/nodes/src/google-sheets/google-sheets.service.ts | Removes embedded token expiry/refresh helpers from the Sheets service. |
| packages/nodes/src/google-sheets/google-sheets.executor.ts | Stops per-execution token refresh and fixes clear-range construction. |
| packages/nodes/src/common/google-oauth-service.ts | Centralizes token expiry check + refresh and performs auto-refresh in getCredentials. |
| apps/web/app/workflows/[id]/components/ConfigModal.tsx | Adjusts credential email display to read from credential config. |
| apps/http-backend/src/services/token-refresh.service.ts | Switches periodic refresh job to use GoogleOAuthService.refreshAccessToken and extends token shape. |
Suppressed comments (1)
apps/http-backend/src/services/token-refresh.service.ts:41
refreshTokencallsthis.oauthService.refreshAccessToken(tokens.refresh_token)without validating that a refresh token exists. Whenrefresh_tokenis missing/empty for an expiring credential, this will throw and mark the job as failed instead of returning a clear, actionable error.
private async refreshToken(credentialId: string, tokens: OAuthTokens): Promise<RefreshResult> {
try {
// Use your existing refreshAccessToken method
const newTokens = await this.oauthService.refreshAccessToken(tokens.refresh_token);
| expiry_date: number; | ||
| scope?: string; | ||
| email?: string | ||
| } |
| async refreshAccessToken(refresh_token: string): Promise<OAuthTokens> { | ||
| try { | ||
| await this.oauth2Client.setCredentials({ refresh_token: refresh_token }) | ||
| const { credentials } = await this.oauth2Client.refreshAccessToken(); | ||
|
|
| catch (error) { | ||
| throw new Error(`Failed to clear the rows: ${error}`) | ||
| } | ||
| } | ||
|
|
||
| isTokenExpired(): boolean { | ||
| const credentials = this.auth.credentials; | ||
| if (!credentials.expiry_date) return false; | ||
|
|
||
| return Date.now() >= credentials.expiry_date - (5 * 60 * 1000); | ||
| } | ||
|
|
||
| async refreshAccessToken(): Promise<GoogleSheetsCredentials> { | ||
| try { | ||
| const { credentials } = await this.auth.refreshAccessToken(); | ||
|
|
||
| // IMPORTANT: Only include refresh_token if Google returns a new one | ||
| // Google doesn't always return a new refresh_token on every refresh | ||
| const result: GoogleSheetsCredentials = { | ||
| access_token: credentials.access_token || '', | ||
| refresh_token: '', // Will be set below if present | ||
| token_type: credentials.token_type || '', | ||
| expiry_date: credentials.expiry_date || 0 | ||
| }; | ||
|
|
||
| // Only include refresh_token if Google actually returned one | ||
| if (credentials.refresh_token) { | ||
| result.refresh_token = credentials.refresh_token; | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| catch (error) { | ||
| throw new Error(`Failed to refresh token: ${error}`) | ||
| } | ||
| } | ||
| } |
| key={cred.id} | ||
| className="px-2.5 py-1 bg-emerald-500/15 text-emerald-400 text-xs rounded-full border border-emerald-500/20 font-medium" | ||
| > | ||
| {cred.email || cred.name} | ||
| {cred.config?.email || cred.name} | ||
| </span> |
…ects
Summary by CodeRabbit
New Features
Bug Fixes