feat: add provider scheduled active time & remove joinClaudePool#844
Merged
feat: add provider scheduled active time & remove joinClaudePool#844
Conversation
…audePool Add HH:mm-based active time windows (start/end) to providers for time-based routing control. Supports same-day and cross-day schedules with system timezone integration. Also removes the deprecated joinClaudePool column/feature. Key changes: - Core: isProviderActiveNow() with NaN fail-open defense - Schema: active_time_start/end columns, drop join_claude_pool (single migration) - Types: 7 interfaces updated across Provider/Batch/Create/Update - Proxy: schedule_inactive filter in provider selection hot path - Batch: HH:mm regex validation in patch contract - UI: schedule toggle, time pickers, cross-day hint, list badge - i18n: 5 languages (zh-CN, zh-TW, en, ja, ru) - Tests: 25 schedule tests + 10 batch patch tests
📝 WalkthroughWalkthrough本PR为提供商引入“活跃时间窗口”调度：新增数据库字段 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature allowing administrators to define scheduled active time windows for providers, enhancing routing control and operational flexibility. Concurrently, it cleans up the codebase by removing a deprecated feature, Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
enhancement
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature allowing providers to define scheduled active time windows, enhancing routing control. It also successfully removes the deprecated joinClaudePool column and associated logic. The implementation is thorough, covering database schema changes, API modifications, UI integration, and comprehensive unit tests for the new scheduling logic. The validation for time formats and consistency checks (e.g., both start and end times must be set or null) are well-handled, contributing to data integrity. Overall, the changes are well-executed and maintain a high standard of code quality.
Comment on lines
+30
to
+32
| if (startTime == null || endTime == null) { | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
The comment on line 19 states that validation ensures startTime and endTime are both set or both null. Given this, the explicit check if (startTime == null || endTime == null) on line 30 might be redundant if the validation is strictly enforced upstream. While it acts as a defensive fail-open, it could be simplified if the type system and validation guarantee that these will always be either both strings or both null. However, keeping it as a defensive measure is also acceptable.
![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 (1)
src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx (1)
801-801: 考虑移除 default export。根据编码规范，TypeScript 文件应优先使用命名导出（named exports）。该文件在第 723 行已有命名导出
ProviderForm，此处的 default export 可能是多余的。建议的修改
-export default ProviderForm;As per coding guidelines: "Prefer named exports over default exports"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/index.tsx at line 801, The file currently re-exports ProviderForm as a default at the end; remove the redundant default export line "export default ProviderForm;" so only the existing named export ProviderForm remains, and update any imports elsewhere to use a named import { ProviderForm } if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 908-911: The new filter sets details = `outside active window
${p.activeTimeStart}-${p.activeTimeEnd}` which hardcodes an English message;
replace this with an i18n key or localized message lookup instead. Update the
branch in provider-selector.ts where isProviderActiveNow(p.activeTimeStart,
p.activeTimeEnd, systemTimezone) is false to set details to an i18n key (e.g.
"provider.schedule_inactive" with params for activeTimeStart and activeTimeEnd)
or call the localization helper used across the module, and ensure the reason
remains "schedule_inactive" while passing p.activeTimeStart and p.activeTimeEnd
into the i18n formatter so UI can render it in supported languages.
- Around line 849-861: findReusable currently returns bound/attached providers
without applying the same scheduling window check; update the findReusable code
path (function findReusable) to call
isProviderActiveNow(provider.activeTimeStart, provider.activeTimeEnd,
systemTimezone) and skip providers that are not active. Ensure systemTimezone is
available to findReusable (reuse the resolved const systemTimezone or resolve it
at the start of findReusable) so bound sessions cannot be selected outside their
activeTime window.
In `@src/lib/utils/provider-schedule.ts`:
- Around line 56-59: parseHHMM currently parses loosely and accepts malformed
values like "24:00" or "9:00", breaking fail-open logic; change parseHHMM to
perform strict format and range validation: require zero-padded HH:MM (use a
regex such as ^([01]\d|2[0-3]):([0-5]\d)$), only then parse hours/minutes and
return h*60+m, otherwise return NaN so callers that check isNaN keep the
fail-open behavior; update the implementation in parseHHMM in
provider-schedule.ts accordingly.
---
Nitpick comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/index.tsx:
- Line 801: The file currently re-exports ProviderForm as a default at the end;
remove the redundant default export line "export default ProviderForm;" so only
the existing named export ProviderForm remains, and update any imports elsewhere
to use a named import { ProviderForm } if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (36)
drizzle/0077_nappy_giant_man.sqldrizzle/meta/0077_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/settings/providers/batchEdit.jsonmessages/en/settings/providers/form/sections.jsonmessages/en/settings/providers/list.jsonmessages/ja/settings/providers/batchEdit.jsonmessages/ja/settings/providers/form/sections.jsonmessages/ja/settings/providers/list.jsonmessages/ru/settings/providers/batchEdit.jsonmessages/ru/settings/providers/form/sections.jsonmessages/ru/settings/providers/list.jsonmessages/zh-CN/settings/providers/batchEdit.jsonmessages/zh-CN/settings/providers/form/sections.jsonmessages/zh-CN/settings/providers/list.jsonmessages/zh-TW/settings/providers/batchEdit.jsonmessages/zh-TW/settings/providers/form/sections.jsonmessages/zh-TW/settings/providers/list.jsonsrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/index.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/[locale]/settings/providers/_components/provider-rich-list-item.tsxsrc/app/v1/_lib/proxy/provider-selector.tssrc/drizzle/schema.tssrc/lib/provider-patch-contract.tssrc/lib/utils/provider-schedule.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/provider.tssrc/types/message.tssrc/types/provider.tstests/unit/actions/providers-patch-contract.test.tstests/unit/lib/utils/provider-schedule.test.ts
Comment on lines
+908
to
911
| } else if (!isProviderActiveNow(p.activeTimeStart, p.activeTimeEnd, systemTimezone)) { | ||
| reason = "schedule_inactive"; | ||
| details = `outside active window ${p.activeTimeStart}-${p.activeTimeEnd}`; | ||
| } else if ( |
There was a problem hiding this comment.
新增的过滤详情文本是硬编码英文。
details = \outside active window ...`` 建议改为可本地化文案或稳定的 i18n key/code。
As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/v1/_lib/proxy/provider-selector.ts` around lines 908 - 911, The new
filter sets details = `outside active window
${p.activeTimeStart}-${p.activeTimeEnd}` which hardcodes an English message;
replace this with an i18n key or localized message lookup instead. Update the
branch in provider-selector.ts where isProviderActiveNow(p.activeTimeStart,
p.activeTimeEnd, systemTimezone) is false to set details to an i18n key (e.g.
"provider.schedule_inactive" with params for activeTimeStart and activeTimeEnd)
or call the localization helper used across the module, and ensure the reason
remains "schedule_inactive" while passing p.activeTimeStart and p.activeTimeEnd
into the i18n formatter so UI can render it in supported languages.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
After thorough analysis through 6 review perspectives (Comment Accuracy, Test Coverage, Error Handling, Type Safety, Logic/Security, and Code Clarity), no significant issues were identified in this PR.
PR Size: XL
- Lines changed: 4,669 (4,649 additions, 20 deletions)
- Files changed: 36
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Proper fail-open defense with logging
- Type safety - Sound type definitions with correct nullability
- Documentation accuracy - Comments match implementation
- Test coverage - Comprehensive (35 new tests: 25 schedule + 10 batch patch)
- Code clarity - Well-organized and readable
Key Implementation Highlights
Core Logic (src/lib/utils/provider-schedule.ts):
- O(1) pure function with fail-open defense for malformed input
- Correct same-day (
start <= now < end) and cross-day (now >= start || now < end) logic - Timezone support via Intl.DateTimeFormat with cached system settings
Hot Path Integration (src/app/v1/_lib/proxy/provider-selector.ts):
- Timezone resolved once per request (line 797)
- Schedule filtering in provider candidate selection (line 860)
schedule_inactivefilter reason tracking for observability
Validation (src/lib/validation/schemas.ts + src/lib/provider-patch-contract.ts):
- HH:mm regex validation:
^([01][0-9]|2[0-3]):[0-5][0-9]$ - Cross-validation: both fields must be set/cleared together, cannot be equal
- Batch edit support with set/clear operations
Database (drizzle/0077_nappy_giant_man.sql):
- Clean migration: adds
active_time_start/active_time_endVARCHAR(5), drops deprecatedjoin_claude_pool
Tests (tests/unit/lib/utils/provider-schedule.test.ts + tests/unit/actions/providers-patch-contract.test.ts):
- 25 schedule tests: null inputs, same-day, cross-day, boundaries, timezone, malformed input defense
- 10 batch patch tests: set, clear, reject non-string, reject invalid format, combined operations
Suggested Manual Test Plan (from PR author)
- Create provider with schedule, verify proxy selector filtering
- Batch edit schedule on multiple providers
- Verify cross-day schedule (e.g., 22:00-08:00)
- Verify provider list shows schedule badge
Automated review by Claude AI
1. Add schedule check in findReusable session reuse path to prevent bound sessions from bypassing active time window filtering. Clears session binding when provider is outside schedule. 2. Strict HH:mm validation in parseHHMM using regex - values like "24:00", "9:00", "99:99" now return NaN and trigger fail-open, instead of producing silently incorrect minute values.
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| timeZone: timezone, | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| hour12: false, |
Contributor
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] Incorrect nowMinutes for 00:00-00:59 due to hour12: false (src/lib/utils/provider-schedule.ts:66)
Evidence (src/lib/utils/provider-schedule.ts:66):
hour12: false,Why this is a problem: On Node/Intl, formatting 00:00-00:59 with hour12: false can produce an hour part of "24" (e.g. UTC 2020-01-01T00:30:00Z formats as 24:30). This code turns that into nowMinutes = 1440 + minute, so same-day windows that include midnight (e.g. 00:00-08:00, 00:00-23:59) are incorrectly treated as inactive for the first hour of the day.
Suggested fix:
function getCurrentMinutesInTimezone(now: Date, timezone: string): number {
const formatter = new Intl.DateTimeFormat("en-US", {
timeZone: timezone,
hour: "2-digit",
minute: "2-digit",
hourCycle: "h23",
});
const parts = formatter.formatToParts(now);
const hourRaw = Number.parseInt(parts.find((p) => p.type === "hour")?.value ?? "0", 10);
const minute = Number.parseInt(parts.find((p) => p.type === "minute")?.value ?? "0", 10);
// Defensive: some runtimes can still emit "24" for midnight.
const hour = hourRaw == 24 ? 0 : hourRaw;
return hour * 60 + minute;
}Add a regression test:
it("treats 00:30 as within 00:00-08:00", () => {
const now = makeDate(0, 30, "UTC");
expect(isProviderActiveNow("00:00", "08:00", "UTC", now)).toBe(true);
});
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds provider scheduled active time windows and removes joinClaudePool across DB, proxy selection, UI, and i18n. A critical bug in the new schedule utility can misclassify provider activity during 00:00-00:59, breaking schedules that start at midnight.
PR Size: XL
- Lines changed: 4669
- Files changed: 36
- Split suggestion (XL): separate into (1) DB migration/schema/types, (2) proxy hot-path + schedule runtime logic, (3) UI/i18n, (4) tests.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/lib/utils/provider-schedule.ts:66-Intl.DateTimeFormat(..., { hour12: false })can emit hour"24"for 00:xx, producingnowMinutes >= 1440and breaking same-day windows that include midnight (confidence 98)
High Priority Issues (Should Fix)
- None
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
- Identified PR
#844and applied labelsize/XL(4669 lines changed across 36 files). - Posted 1 inline review comment on
src/lib/utils/provider-schedule.ts:66:- [CRITICAL] [LOGIC-BUG]
Intl.DateTimeFormat(..., { hour12: false })can emit hour"24"for 00:00–00:59, causing same-day windows that include midnight (e.g.00:00-08:00,00:00-23:59) to be treated as inactive. - Suggested fix: switch to
hourCycle: "h23"(drophour12) and defensively normalize24 -> 0, plus add a regression test for00:30within00:00-08:00.
- [CRITICAL] [LOGIC-BUG]
- Submitted the required PR review summary via
gh pr review --comment.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
joinClaudePoolcolumn/featureChanges
Core Logic
src/lib/utils/provider-schedule.ts:isProviderActiveNow()- O(1) pure function with NaN fail-open defensestart <= now < end; Cross-day:now >= start || now < endDatabase
active_time_start/active_time_endVARCHAR(5), DROPjoin_claude_poolType System
Proxy Selection (Hot Path)
schedule_inactivefilter reason in provider-selector.tsBatch Edit
isValidSetValue)UI
i18n
Tests
Test plan
bun run typecheck- 0 errorsbun run lint- cleanbun run test- 3425 passed (360 files)bun run build- production build successGreptile Summary
Adds HH:mm-based scheduled active time windows to providers for time-based routing control, with comprehensive validation and UI support. Also removes the deprecated
joinClaudePoolfeature in a single consolidated migration.Key Changes:
provider-schedule.tswith O(1) pure function, fail-open defense for malformed data, and support for same-day and cross-day schedulesCode Quality:
Confidence Score: 5/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Provider Request] --> B{Session Provider Exists?} B -->|Yes| C[Check Schedule Active] C -->|Inactive| D[Clear Session & Return Null] C -->|Active| E[Use Session Provider] B -->|No| F[Resolve System Timezone] F --> G[Filter Enabled Providers] G --> H{Check Schedule Active} H -->|Inactive| I[Skip Provider - schedule_inactive] H -->|Active| J[Format/Model Match] J --> K[Build Eligible List] K --> L[Priority Selection] M[Provider Schedule Logic] --> N{Both Times Null?} N -->|Yes| O[Always Active - Return True] N -->|No| P{Start == End?} P -->|Yes| Q[Zero Width - Return False] P -->|No| R[Parse HH:mm to Minutes] R --> S{Valid Format?} S -->|No| T[Fail Open - Return True] S -->|Yes| U{Start < End?} U -->|Yes| V[Same Day: start <= now < end] U -->|No| W[Cross Day: now >= start OR now < end]Last reviewed commit: de1bdf7