fix(synology-chat): respect SYNOLOGY_RATE_LIMIT=0 by using ?? instead of ||#39220
fix(synology-chat): respect SYNOLOGY_RATE_LIMIT=0 by using ?? instead of ||#39220Nipurn123 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR attempts to fix a bug where Confidence Score: 1/5
Last reviewed commit: 4bfde34 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| accountOverride.rateLimitPerMinute ?? | ||
| channelCfg.rateLimitPerMinute ?? | ||
| (envRateLimit ? parseInt(envRateLimit, 10) || 30 : 30), | ||
| (envRateLimit ? parseInt(envRateLimit, 10) ?? 30 : 30), |
There was a problem hiding this comment.
?? doesn't guard against NaN from parseInt
parseInt never returns null or undefined — it returns either a number or NaN. The nullish coalescing operator (??) only falls back for null/undefined, so if SYNOLOGY_RATE_LIMIT is set to a non-numeric value (e.g. "abc"), parseInt("abc", 10) returns NaN, and NaN ?? 30 evaluates to NaN — silently propagating an invalid rate limit. The previous || operator would have correctly fallen back to 30 in this case (NaN || 30 → 30).
A correct fix needs to handle both 0 and invalid strings:
| (envRateLimit ? parseInt(envRateLimit, 10) ?? 30 : 30), | |
| (envRateLimit ? (Number.isNaN(parseInt(envRateLimit, 10)) ? 30 : parseInt(envRateLimit, 10)) : 30), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/synology-chat/src/accounts.ts
Line: 83
Comment:
`??` doesn't guard against `NaN` from `parseInt`
`parseInt` never returns `null` or `undefined` — it returns either a number or `NaN`. The nullish coalescing operator (`??`) only falls back for `null`/`undefined`, so if `SYNOLOGY_RATE_LIMIT` is set to a non-numeric value (e.g. `"abc"`), `parseInt("abc", 10)` returns `NaN`, and `NaN ?? 30` evaluates to `NaN` — silently propagating an invalid rate limit. The previous `||` operator *would* have correctly fallen back to `30` in this case (`NaN || 30` → `30`).
A correct fix needs to handle both `0` and invalid strings:
```suggestion
(envRateLimit ? (Number.isNaN(parseInt(envRateLimit, 10)) ? 30 : parseInt(envRateLimit, 10)) : 30),
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bfde340e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| accountOverride.rateLimitPerMinute ?? | ||
| channelCfg.rateLimitPerMinute ?? | ||
| (envRateLimit ? parseInt(envRateLimit, 10) || 30 : 30), | ||
| (envRateLimit ? parseInt(envRateLimit, 10) ?? 30 : 30), |
There was a problem hiding this comment.
Using ?? with parseInt here breaks the defaulting behavior for malformed non-empty env values (for example SYNOLOGY_RATE_LIMIT=abc), because parseInt returns NaN and NaN ?? 30 stays NaN. That propagates rateLimitPerMinute as NaN, and getRateLimiter in extensions/synology-chat/src/webhook-handler.ts:22-24 will recreate the limiter on every request (NaN !== NaN), effectively disabling rate limiting instead of falling back to 30.
Useful? React with 👍 / 👎.
1 participant
Issue
Fixes #39191
Problem
In
extensions/synology-chat/src/accounts.ts:83, the rate limit parsing used||instead of??:When
SYNOLOGY_RATE_LIMIT=0,parseInt("0", 10)returns0, and0 || 30evaluates to30, silently ignoring the user′s intent.Solution
Changed
||to??(nullish coalescing) so that0is respected:Now
SYNOLOGY_RATE_LIMIT=0correctly producesrateLimitPerMinute: 0.Testing
Verified the fix:
SYNOLOGY_RATE_LIMIT=0→rateLimitPerMinute: 0✓SYNOLOGY_RATE_LIMIT=5→rateLimitPerMinute: 5✓SYNOLOGY_RATE_LIMITunset →rateLimitPerMinute: 30✓