refactor(token-scheduler): change token scheduling from days to hours - #423
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughShared authentication helpers now validate tokens and return claims or status. ChangesToken Validation and Renewal Scheduling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RefreshScheduler
participant TokenScheduler
participant TokenValidation
participant Timer
RefreshScheduler->>TokenScheduler: create scheduler with newToken and closeUserSession
TokenScheduler->>TokenValidation: validate token and read claims
TokenValidation-->>TokenScheduler: token status and exp/iat claims
TokenScheduler->>Timer: schedule refresh using shared interval or five-minute fallback
TokenScheduler-->>RefreshScheduler: return retryability and optional job
TokenScheduler->>RefreshScheduler: invoke unauthorized for expired token
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
src/apps/main/auth/refresh-token/create-token-schedule-with-retry.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/apps/main/auth/refresh-token/create-token-schedule-with-retry.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/apps/main/token-scheduler/TokenScheduler.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 |
ad68543 to
eeeb1ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/apps/main/token-scheduler/token-scheduler.test.ts (2)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore real timers after each test.
Three tests call
vi.useFakeTimers(). TheafterEachhook only cancels schedules. Fake timers then remain active for the following tests, which usenode-scheduleandDate.now(). Addvi.useRealTimers().🧹 Proposed fix
afterEach(() => { scheduler?.cancelAll(); + vi.useRealTimers(); });🤖 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 `@src/apps/main/token-scheduler/token-scheduler.test.ts` around lines 45 - 47, Update the afterEach hook in the token scheduler tests to call vi.useRealTimers() after cancelling schedules, ensuring every test restores real timer behavior for subsequent node-schedule and Date.now() usage.
83-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test depends on undocumented library behavior for a missing
iat.The test creates a token with
noTimestamp: trueand expects the five-minute fallback. That outcome requiresauth.calculateMillisecondsUntilRefreshto return a non-positive value wheniatis absent. Confirm that behavior in@internxt/lib1.5.2. If the library changes, this test becomes misleading rather than failing for a clear reason.🤖 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 `@src/apps/main/token-scheduler/token-scheduler.test.ts` around lines 83 - 102, Update the test around TokenScheduler.schedule and createTokenWithoutIssuedAtExpiringIn to avoid relying on undocumented `@internxt/lib` behavior for missing iat; explicitly mock or stub auth.calculateMillisecondsUntilRefresh to return a non-positive value, while preserving the five-minute fallback assertions.src/backend/features/auth/validate-token.test.ts (1)
11-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the explicit
tokenargument.The tests only exercise the stored-credential path.
TokenScheduler.getTokenClaimscallsvalidateToken({ token }). Add a test that passes a token and asserts thatgetCredentialsis not called.🤖 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 `@src/backend/features/auth/validate-token.test.ts` around lines 11 - 19, Add a test alongside the existing valid-token case that calls validateToken with an explicit token, verifies the decoded claims are returned, asserts validateJwt was called with that token, and confirms getCredentials was not called.src/apps/main/token-scheduler/TokenScheduler.ts (1)
37-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe status check and the claims can read different tokens.
validateTokenAndCheckExpiration()reads the stored credentials.getTokenClaims(this.newToken)reads the constructor token. The production caller passes the stored token, so both agree today. If any future caller passes a different token, the scheduler validates one token and schedules from another. Passthis.newTokento both checks to remove that divergence.🤖 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 `@src/apps/main/token-scheduler/TokenScheduler.ts` around lines 37 - 50, Update the token validation call in the scheduler flow to pass this.newToken into validateTokenAndCheckExpiration, matching the token already supplied to getTokenClaims. Keep the existing invalid and expired status handling unchanged so both checks consistently operate on the constructor token.src/backend/features/auth/validate-token-and-check-expiration.ts (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider accepting an optional token parameter.
This helper always reads the stored credentials.
validateTokenaccepts an optionaltoken.TokenSchedulerneeds the status of the token it holds, not only the stored one. An optionaltokenparameter would make both helpers symmetric and let the caller check one specific token.🤖 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 `@src/backend/features/auth/validate-token-and-check-expiration.ts` around lines 6 - 9, Update validateTokenAndCheckExpiration to accept an optional token parameter and use it when provided, falling back to getCredentials only when omitted. Preserve the existing auth.validateTokenAndCheckExpiration call and return shape, enabling callers such as TokenScheduler to validate a specific token while retaining current behavior.src/backend/features/auth/validate-token.ts (1)
9-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the shared
Resulttype fromvalidateToken.
validateTokenAndCheckExpirationusesResult<TokenStatus, Error>, butvalidateTokenreturns an inferred{ data } | { error }union. DeclarevalidateToken()asResult<JwtPayload, Error>, importResultfromcontext/shared/domain/Result, and return{ data: decodedJwtClaims }/{ error }consistently.🤖 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 `@src/backend/features/auth/validate-token.ts` around lines 9 - 24, Update validateToken to explicitly return Result<JwtPayload, Error>, importing Result from context/shared/domain/Result. Ensure both successful validation and error paths return the shared Result shape consistently, while preserving the existing token validation and logging behavior.
🤖 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 `@src/apps/main/token-scheduler/TokenScheduler.ts`:
- Around line 36-48: Update TokenScheduler.schedule and
createTokenScheduleWithRetry so terminal TokenStatus.EXPIRED and
TokenStatus.INVALID outcomes are represented distinctly from transient
scheduling failures, allowing the caller to stop retrying immediately. Preserve
the existing unauthorized() behavior for expired tokens, ensuring
closeUserSession is invoked at most once, and keep retries only for genuinely
retryable failures.
In `@src/backend/features/auth/validate-token-and-check-expiration.ts`:
- Line 4: Update the Result import in validate-token-and-check-expiration.ts to
replace the duplicated path separator in the module path with a single
separator, without changing the imported symbol or path structure.
In `@src/backend/features/auth/validate-token.ts`:
- Around line 1-24: Run Prettier with write enabled on the added auth files,
including validateToken, and commit the resulting formatting changes without
altering behavior.
---
Nitpick comments:
In `@src/apps/main/token-scheduler/token-scheduler.test.ts`:
- Around line 45-47: Update the afterEach hook in the token scheduler tests to
call vi.useRealTimers() after cancelling schedules, ensuring every test restores
real timer behavior for subsequent node-schedule and Date.now() usage.
- Around line 83-102: Update the test around TokenScheduler.schedule and
createTokenWithoutIssuedAtExpiringIn to avoid relying on undocumented
`@internxt/lib` behavior for missing iat; explicitly mock or stub
auth.calculateMillisecondsUntilRefresh to return a non-positive value, while
preserving the five-minute fallback assertions.
In `@src/apps/main/token-scheduler/TokenScheduler.ts`:
- Around line 37-50: Update the token validation call in the scheduler flow to
pass this.newToken into validateTokenAndCheckExpiration, matching the token
already supplied to getTokenClaims. Keep the existing invalid and expired status
handling unchanged so both checks consistently operate on the constructor token.
In `@src/backend/features/auth/validate-token-and-check-expiration.ts`:
- Around line 6-9: Update validateTokenAndCheckExpiration to accept an optional
token parameter and use it when provided, falling back to getCredentials only
when omitted. Preserve the existing auth.validateTokenAndCheckExpiration call
and return shape, enabling callers such as TokenScheduler to validate a specific
token while retaining current behavior.
In `@src/backend/features/auth/validate-token.test.ts`:
- Around line 11-19: Add a test alongside the existing valid-token case that
calls validateToken with an explicit token, verifies the decoded claims are
returned, asserts validateJwt was called with that token, and confirms
getCredentials was not called.
In `@src/backend/features/auth/validate-token.ts`:
- Around line 9-24: Update validateToken to explicitly return Result<JwtPayload,
Error>, importing Result from context/shared/domain/Result. Ensure both
successful validation and error paths return the shared Result shape
consistently, while preserving the existing token validation and logging
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 947ea1b5-af60-4bb0-bc66-afffd315a4b8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/apps/main/auth/refresh-token/create-token-schedule-with-retry.tssrc/apps/main/token-scheduler/TokenScheduler.tssrc/apps/main/token-scheduler/token-scheduler.test.tssrc/backend/features/auth/validate-token-and-check-expiration.test.tssrc/backend/features/auth/validate-token-and-check-expiration.tssrc/backend/features/auth/validate-token.test.tssrc/backend/features/auth/validate-token.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
…improve validation checks
| private getTokenClaims(token?: string) { | ||
| const { data, error } = validateToken({ token }); | ||
|
|
||
| return decoded.exp ? decoded.exp * 1000 : TokenScheduler.MAX_TIME; | ||
| } catch (err) { | ||
| logger.error({ msg: '[TOKEN] Token could be not decoded' }); | ||
| if (error || !data.exp) return; | ||
|
|
||
| return TokenScheduler.MAX_TIME; | ||
| } | ||
| return { | ||
| exp: data.exp, | ||
| iat: data.iat, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Is this method really necessary? does not seem to add much value other than be a wrapper to a method
|
2 participants
What is Changed / Added
Why
Summary by CodeRabbit
Bug Fixes
Chores