[codex] show local freebuff model availability#542
Conversation
Greptile SummaryThis PR replaces the static Confidence Score: 5/5Safe to merge; all findings are P2 style/simplification suggestions with no correctness or reliability impact. The logic is correct, tests cover open/closed boundaries and timezone label formatting, and the component integration is straightforward. Remaining comments are purely about code clarity and the unreachable fallback branch, none of which affect runtime behavior. common/src/constants/freebuff-models.ts — minor simplification opportunities in the interface, exported constant, and loop.
|
| Filename | Overview |
|---|---|
| common/src/constants/freebuff-models.ts | Adds helpers to compute next open/close UTC instants and format them in the user's local timezone; logic is correct but has redundant minutes field, stale exported constant, and a loop that can be simplified. |
| cli/src/components/freebuff-model-selector.tsx | Swaps the static FREEBUFF_DEPLOYMENT_HOURS_LABEL for the new dynamic getFreebuffDeploymentAvailabilityLabel call wrapped in useMemo; also contains several minor formatting cleanups with no logic issues. |
| common/src/tests/freebuff-models.test.ts | New test file covering label formatting for open/closed states, same-day vs next-day weekday display, and ET/PT boundary checks; coverage is appropriate and assertions are correct. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: common/src/constants/freebuff-models.ts
Line: 26-34
Comment:
**Redundant `minutes` field alongside `minute` in `ZonedDateParts`**
The interface stores both `minute` (raw 0–59) and `minutes` (`hour * 60 + minute`) as similarly-named fields. `minutes` is only consumed in `isFreebuffDeploymentHours`; everywhere else the code uses `hour` and `minute` separately. Dropping `minutes` from the interface and inlining the arithmetic in `isFreebuffDeploymentHours` removes the ambiguity and one confusingly similar name:
```suggestion
interface ZonedDateParts {
year: number
month: number
day: number
weekday: string
hour: number
minute: number
}
```
Then in `isFreebuffDeploymentHours`:
```ts
return eastern.hour * 60 + eastern.minute >= 9 * 60 && pacific.hour * 60 + pacific.minute < 17 * 60
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: common/src/constants/freebuff-models.ts
Line: 20
Comment:
**Exported constant is now stale and no longer drives the displayed label**
`FREEBUFF_DEPLOYMENT_HOURS_LABEL` (`'9am ET-5pm PT'`) is still exported but the component now calls `getFreebuffDeploymentAvailabilityLabel` instead. Anyone importing the constant from another call site would still see the static string while the UI shows a dynamic local-time label. If this constant is no longer intended for external use, removing or deprecating it avoids silent drift; if it still needs to remain for other callers, a doc comment clarifying that it is not the UI-facing label would help.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: common/src/constants/freebuff-models.ts
Line: 176-197
Comment:
**`getNextFreebuffDeploymentStart` loop is harder to follow than necessary**
The 8-iteration loop skipping weekends and checking whether the 9 am ET candidate is still in the future can be replaced with direct arithmetic: if the current ET day is already past 9 am (or is a weekend), advance by the number of days to the next weekday, then construct the UTC instant directly. The current fallback after the loop (`offset = 8`) is also unreachable — in any 8-day window there will always be at least one weekday whose 9 am ET instant is in the future. Simplifying to at-most-a-few lines of offset arithmetic would make the intent clearer.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "show local freebuff model availability" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| interface ZonedDateParts { | ||
| year: number | ||
| month: number | ||
| day: number | ||
| weekday: string | ||
| hour: number | ||
| minute: number | ||
| minutes: number | ||
| } |
There was a problem hiding this comment.
minutes field alongside minute in ZonedDateParts
The interface stores both minute (raw 0–59) and minutes (hour * 60 + minute) as similarly-named fields. minutes is only consumed in isFreebuffDeploymentHours; everywhere else the code uses hour and minute separately. Dropping minutes from the interface and inlining the arithmetic in isFreebuffDeploymentHours removes the ambiguity and one confusingly similar name:
| interface ZonedDateParts { | |
| year: number | |
| month: number | |
| day: number | |
| weekday: string | |
| hour: number | |
| minute: number | |
| minutes: number | |
| } | |
| interface ZonedDateParts { | |
| year: number | |
| month: number | |
| day: number | |
| weekday: string | |
| hour: number | |
| minute: number | |
| } |
Then in isFreebuffDeploymentHours:
return eastern.hour * 60 + eastern.minute >= 9 * 60 && pacific.hour * 60 + pacific.minute < 17 * 60Prompt To Fix With AI
This is a comment left during a code review.
Path: common/src/constants/freebuff-models.ts
Line: 26-34
Comment:
**Redundant `minutes` field alongside `minute` in `ZonedDateParts`**
The interface stores both `minute` (raw 0–59) and `minutes` (`hour * 60 + minute`) as similarly-named fields. `minutes` is only consumed in `isFreebuffDeploymentHours`; everywhere else the code uses `hour` and `minute` separately. Dropping `minutes` from the interface and inlining the arithmetic in `isFreebuffDeploymentHours` removes the ambiguity and one confusingly similar name:
```suggestion
interface ZonedDateParts {
year: number
month: number
day: number
weekday: string
hour: number
minute: number
}
```
Then in `isFreebuffDeploymentHours`:
```ts
return eastern.hour * 60 + eastern.minute >= 9 * 60 && pacific.hour * 60 + pacific.minute < 17 * 60
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in c703c2ac8. I removed minutes from ZonedDateParts and now inline the hour * 60 + minute arithmetic directly in isFreebuffDeploymentHours() so the type no longer carries the duplicate field.
| @@ -20,6 +20,23 @@ export interface FreebuffModelOption { | |||
| export const FREEBUFF_DEPLOYMENT_HOURS_LABEL = '9am ET-5pm PT' | |||
There was a problem hiding this comment.
FREEBUFF_DEPLOYMENT_HOURS_LABEL ('9am ET-5pm PT') is still exported but the component now calls getFreebuffDeploymentAvailabilityLabel instead. Anyone importing the constant from another call site would still see the static string while the UI shows a dynamic local-time label. If this constant is no longer intended for external use, removing or deprecating it avoids silent drift; if it still needs to remain for other callers, a doc comment clarifying that it is not the UI-facing label would help.
Prompt To Fix With AI
This is a comment left during a code review.
Path: common/src/constants/freebuff-models.ts
Line: 20
Comment:
**Exported constant is now stale and no longer drives the displayed label**
`FREEBUFF_DEPLOYMENT_HOURS_LABEL` (`'9am ET-5pm PT'`) is still exported but the component now calls `getFreebuffDeploymentAvailabilityLabel` instead. Anyone importing the constant from another call site would still see the static string while the UI shows a dynamic local-time label. If this constant is no longer intended for external use, removing or deprecating it avoids silent drift; if it still needs to remain for other callers, a doc comment clarifying that it is not the UI-facing label would help.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Addressed in c703c2ac8. I kept the constant because web/src/server/free-session/public-api.ts and web/src/llm-api/fireworks.ts still need a timezone-agnostic server-side message, and added a doc comment clarifying that the CLI should use getFreebuffDeploymentAvailabilityLabel() instead.
| function getNextFreebuffDeploymentStart(now: Date): Date { | ||
| const easternNow = getZonedParts(now, FREEBUFF_EASTERN_TIMEZONE) | ||
|
|
||
| for (let offset = 0; offset < 8; offset++) { | ||
| const day = addDaysToYmd( | ||
| easternNow.year, | ||
| easternNow.month, | ||
| easternNow.day, | ||
| offset, | ||
| ) | ||
| if (isWeekend(day)) continue | ||
| const candidate = getUtcForZonedTime(day, FREEBUFF_EASTERN_TIMEZONE, 9, 0) | ||
| if (candidate.getTime() > now.getTime()) return candidate | ||
| } | ||
|
|
||
| return getUtcForZonedTime( | ||
| addDaysToYmd(easternNow.year, easternNow.month, easternNow.day, 8), | ||
| FREEBUFF_EASTERN_TIMEZONE, | ||
| 9, | ||
| 0, | ||
| ) | ||
| } |
There was a problem hiding this comment.
getNextFreebuffDeploymentStart loop is harder to follow than necessary
The 8-iteration loop skipping weekends and checking whether the 9 am ET candidate is still in the future can be replaced with direct arithmetic: if the current ET day is already past 9 am (or is a weekend), advance by the number of days to the next weekday, then construct the UTC instant directly. The current fallback after the loop (offset = 8) is also unreachable — in any 8-day window there will always be at least one weekday whose 9 am ET instant is in the future. Simplifying to at-most-a-few lines of offset arithmetic would make the intent clearer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: common/src/constants/freebuff-models.ts
Line: 176-197
Comment:
**`getNextFreebuffDeploymentStart` loop is harder to follow than necessary**
The 8-iteration loop skipping weekends and checking whether the 9 am ET candidate is still in the future can be replaced with direct arithmetic: if the current ET day is already past 9 am (or is a weekend), advance by the number of days to the next weekday, then construct the UTC instant directly. The current fallback after the loop (`offset = 8`) is also unreachable — in any 8-day window there will always be at least one weekday whose 9 am ET instant is in the future. Simplifying to at-most-a-few lines of offset arithmetic would make the intent clearer.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed in c703c2ac8. I replaced the loop with direct weekday-based offset arithmetic, so getNextFreebuffDeploymentStart() now computes the next ET 9am opening without the fallback scan.
|
Addressed the three simplification suggestions in Changes made:
Re-ran the targeted test plus |
2 participants
Summary
Validation
bun test src/__tests__/freebuff-models.test.tsincommon/bun run typecheckincommon/bun run typecheckincli/