|
20133
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20133
|
|
20134
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab
Pipelines - jiminny/app
Pipelines - jiminny/app
New Tab
Customize sidebar
Open Google Gemini (⌃X)
Tabs from other devices
Open history (⇧⌘H)
Open bookmarks (⌘B)
Skip to content
Skip to content
Open menu...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20134
|
|
20135
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20135
|
|
20136
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20136
|
|
20137
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20137
|
|
20138
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20138
|
|
20139
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab
Pipelines - jiminny/app
Pipelines - jiminny/app
New Tab
Customize sidebar
Open Google Gemini (⌃X)
Tabs from other devices
Open history (⇧⌘H)
Open bookmarks (⌘B)
Skip to content
Skip to content
Open menu
Homepage (g then d)
jiminny
jiminny
app
app
Search or jump to…
Type
/
to search
Chat with Copilot
Open Copilot…
Create new...
All issues(g then i)
All pull requests
All repositories
You have unread notifications(g then n)
Open user navigation menu
Repository navigation
Repository navigation
Code
Code...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20139
|
|
20140
|
Ask Google Gemini
New Tab
New Tab
Jy 20820 es rein Ask Google Gemini
New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20140
|
|
20201
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20201
|
|
20202
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20202
|
|
20203
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab
Pipelines - jiminny/app
Pipelines - jiminny/app
New Tab
Customize sidebar
Open Google Gemini (⌃X)
Tabs from other devices
Open history (⇧⌘H)
Open bookmarks (⌘B)
Skip to content
Skip to content
Open menu
Homepage (g then d)
jiminny
jiminny
app
app
Search or jump to…
Type
/
to search
Chat with Copilot
Open Copilot…
Create new...
All issues(g then i)
All pull requests
All repositories
You have unread notifications(g then n)
Open user navigation menu
Repository navigation
Repository navigation
Code
Code
Pull requests (34)
Pull requests
(
34
)
Agents
Agents
Actions
Actions
Wiki
Wiki
Security and quality (4)
Security and quality
(
4
)
Insights
Insights
Settings
Settings
Important update
Important update
On April 24 we'll start using GitHub Copilot interaction data for AI model training unless you opt out.
Review this update
Review this update
and manage your preferences in your
GitHub account settings
GitHub account settings
.
Dismiss banner
JY-20725 add HS rate limit handling on activities rematching #12066 Edit title
JY-20725 add HS rate limit handling on activities rematching
#
12066
Edit title
Preview
Preview
Checks pending
Checks pending
Code
Code
Open
LakyLak
LakyLak
wants to merge 3 commits into
master
master
from
JY-20725-handle-HS-search-rate-limit
JY-20725-handle-HS-search-rate-limit
Copy head branch name to clipboard
Lines changed: 757 additions & 249 deletions
Conversation (3)
Conversation
(
3
)
Commits (3)
Commits
(
3
)
Checks (2)
Checks
(
2
)
Files changed (12)
Files changed
(
12
)
Pull Request Toolbar
Pull Request Toolbar
Collapse file tree
All commits
All commits
0 of 12 files viewed
Submit review
Submit
review
Open diff view settings
Open overview panel
Open comments panel
(
0
)
Filter files…
Filter options
File tree
File tree
app
Exceptions
RateLimitException.php
RateLimitException.php
Jobs
Crm
MatchActivityCrmData.php
MatchActivityCrmData.php
Middleware
HandleHubspotRateLimit.php
HandleHubspotRateLimit.php
Services/Crm/Hubspot
Pagination...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20203
|
|
20204
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20204
|
|
20205
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066/changes
|
20205
|
|
20206
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab
Pipelines - jiminny/app
Pipelines - jiminny/app
New Tab
Customize sidebar
Open Google Gemini (⌃X)
Tabs from other devices
Open history (⇧⌘H)
Open bookmarks (⌘B)
Skip to content
Skip to content
Open menu
Homepage (g then d)
jiminny
jiminny
app
app
Search or jump to…...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20206
|
|
20207
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20207
|
|
20208
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20208
|
|
20209
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20209
|
|
20210
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20210
|
|
20211
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20211
|
|
20212
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20212
|
|
20213
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST https://api.hubapi.com/crm/v3/objects/contact/search` resulted in a `429 Too Many Requests` response: {"status":"error","message":"You have reached your secondly limit.","errorType":"RATE_LIMIT
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app
Close tab
Pipelines - jiminny/app
Pipelines - jiminny/app
New Tab
Customize sidebar
Open Google Gemini (⌃X)...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20213
|
|
20214
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious....
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20214
|
|
20215
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20215
|
|
20216
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented now
sonarqubecloud
sonarqubecloud
Bot
commented
now
now
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
This branch has not been deployed
This branch has not been deployed
No deployments
Merge info
Merge info
Review required
Review required
At least 1 approving review is required by reviewers with write access.
All checks have passed...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20216
|
|
20217
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented now
sonarqubecloud
sonarqubecloud
Bot
commented
now...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20217
|
|
20345
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20345
|
|
20346
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20346
|
|
20347
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20347
|
|
20348
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20348
|
|
20349
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20349
|
|
20350
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20350
|
|
20351
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20351
|
|
20352
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20352
|
|
20353
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20353
|
|
20354
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20354
|
|
20355
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20355
|
|
20356
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20356
|
|
20360
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20360
|
|
20361
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 32 minutes ago
sonarqubecloud
sonarqubecloud
Bot
commented
32 minutes ago
32 minutes ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
This branch has not been deployed
This branch has not been deployed
No deployments
Merge info
Merge info
Review required
Review required
At least 1 approving review is required by reviewers with write access.
All checks have passed
All checks have passed
12 successful checks
Collapse checks
successful checks
successful checks
build_accept_deploy
build_accept_deploy
build_accept_deploy
Successful in 14m
— Workflow: build_accept_deploy
More actions
ci/circleci: build-backend
ci/circleci: build-backend
ci/circleci: build-backend
— Your tests passed on CircleCI!
More actions
ci/circleci: build-frontend
ci/circleci: build-frontend
ci/circleci: build-frontend
— Your tests passed on CircleCI!
More actions
ci/circleci: checkout-code
ci/circleci: checkout-code
ci/circleci: checkout-code
— Your tests passed on CircleCI!
More actions
ci/circleci: phpstan
ci/circleci: phpstan
ci/circleci: phpstan
— Your tests passed on CircleCI!
More actions
ci/circleci: setup
ci/circleci: setup
ci/circleci: setup
— Your tests passed on CircleCI!
More actions
ci/circleci: sonar_cloud
ci/circleci: sonar_cloud
ci/circleci: sonar_cloud
— Your tests passed on CircleCI!
More actions
ci/circleci: test
ci/circleci: test
ci/circleci: test
— Your tests passed on CircleCI!
More actions
ci/circleci: test-backend-lint
ci/circleci: test-backend-lint
ci/circleci: test-backend-lint...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20361
|
|
20379
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20379
|
|
20380
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20380
|
|
20381
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20381
|
|
20382
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20382
|
|
20386
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
2 hours ago
2 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 1 hour ago
LakyLak
LakyLak
commented
1 hour ago
1 hour ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 1 hour ago •
claude
claude
Bot
commented
1 hour ago
1 hour ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 33 minutes ago
sonarqubecloud
sonarqubecloud
Bot
commented
33 minutes ago
33 minutes ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20386
|
|
20646
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
3 hours ago
3 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 2 hours ago •
claude
claude
Bot
commented
2 hours ago
2 hours ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20646
|
|
20647
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
3 hours ago
3 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 2 hours ago •
claude
claude
Bot
commented
2 hours ago
2 hours ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
JY-20725
JY-20725
code review suggestions
code review suggestions
3 / 6 checks OK
0f3e438
0f3e438
This branch has not been deployed...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20647
|
|
20648
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
3 hours ago
3 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 2 hours ago •
claude
claude
Bot
commented
2 hours ago
2 hours ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
JY-20725
JY-20725
code review suggestions
code review suggestions
3 / 6 checks OK
0f3e438
0f3e438
This branch has not been deployed
This branch has not been deployed
No deployments
Merge info
Merge info
Review required
Review required
At least 1 approving review is required by reviewers with write access.
Some checks haven't completed yet
Some checks haven't completed yet
2 pending, 1 in progress, 1 expected, 3 successful checks
Collapse checks
Collapse 3 pending checks group
3 pending checks
Checks settings
pending checks
pending checks
ci/circleci: build-backend
ci/circleci: build-backend
ci/circleci: build-backend
Waiting for status to be reported
— CircleCI is running your tests
More actions
ci/circleci: build-frontend
ci/circleci: build-frontend
ci/circleci: build-frontend
Waiting for status to be reported
— CircleCI is running your tests
More actions
SonarCloud Code Analysis
SonarCloud Code Analysis
Expected
— Waiting for status to be reported
Required
Collapse 1 in progress check group...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20648
|
|
20649
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
3 hours ago
3 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 2 hours ago •
claude
claude
Bot
commented
2 hours ago
2 hours ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20649
|
|
20650
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
3 hours ago
3 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 2 hours ago •
claude
claude
Bot
commented
2 hours ago
2 hours ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
JY-20725
JY-20725
code review suggestions
code review suggestions
3 / 6 checks OK
0f3e438
0f3e438
This branch has not been deployed
This branch has not been deployed
No deployments
Merge info
Merge info
Review required
Review required
At least 1 approving review is required by reviewers with write access.
Some checks haven't completed yet
Some checks haven't completed yet
2 pending, 1 in progress, 1 expected, 3 successful checks...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20650
|
|
20651
|
New Tab
New Tab
Jy 20820 es reindex stream model h New Tab
New Tab
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
Jy 20820 es reindex stream model hydration by Vasil-Jiminny · Pull Request #12059 · jiminny/app
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
[JY-20725] [HubSpot] Optimise CRM rematching on delete hubspot accounts/contacts - Jira
SevenShores\Hubspot\Exceptions\BadRequest: Client error: `POST [URL_WITH_CREDENTIALS]
Show options
LakyLak commented 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
JIRA: JY-20725
JIRA:
JY-20725
JY-20725
Changes:
Changes:
Adds
HandleHubspotRateLimit
middleware (release jobs with delay)
Adds
executeRequest
wrapper in Hubspot Client. It handles 429 errors and throws
RateLimitException
Add or remove reactions
LakyLak
LakyLak
added
2
commits
3 hours ago
3 hours ago
@LakyLak
JY-20725
JY-20725
add HS rate limit handling on activities rematching
add HS rate limit handling on activities rematching
8 / 10 checks OK
44d5678
44d5678
@LakyLak
JY-20725
JY-20725
add tests
add tests
11 / 12 checks OK
cbed2c9
cbed2c9
@sonarqubecloud
Show options
sonarqubecloud Bot commented 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Failed Quality Gate failed
Quality Gate Failed
Quality Gate failed
Failed conditions
34.4% Coverage on New Code
34.4% Coverage on New Code
(required ≥ 70%)
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
Show options
Author
LakyLak commented 2 hours ago
LakyLak
LakyLak
commented
2 hours ago
2 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 2 hours ago •
claude
claude
Bot
commented
2 hours ago
2 hours ago
•
edited
edited
Claude finished
@LakyLak
@LakyLak
's task in 1m 42s
——
View job
View job
Code Review
Code Review
Completed task
Gathering context
Completed task
Reading changed files
Completed task
Analyzing code quality
Completed task
Posting review feedback
Overall this is a well-thought-out implementation. The circuit-breaker pattern using Redis NX, the jitter for thundering-herd prevention, and the log-sampling strategy are all solid. A few issues worth addressing before merge:
Issues
Issues
1. Batch API calls bypass rate-limit handling
batchReadObjects()
in
Client.php
(lines 296–319) calls the HubSpot batch APIs directly without going through
executeRequest()
. If the batch endpoints return 429s, those exceptions will not set the Redis circuit-breaker and will not be wrapped in
RateLimitException
, so the middleware won't catch them.
// batchReadObjects — no executeRequest() wrapping
$
response
=
$
batchConfig
[
'
api
'
]->
read
(
$
batchReadRequest
);
Copy
Given the search endpoints can also hit rate limits during pagination-heavy rematching, the batch calls could too. Consider wrapping the batch call similarly, or at least catching 429 from batch APIs and converting them to
RateLimitException
.
Fix this →
Fix this →
2. Circuit-breaker key is scoped to CRM config ID, not HubSpot portal
getRateLimitCacheKey()
(
Client.php:120
) returns
hubspot:ratelimit:portal:{configId}
, but
$this->config->getId()
is the CRM
configuration
record ID, not the HubSpot portal/account ID. If multiple CRM configuration records share the same HubSpot portal (e.g. re-connections), each will have its own independent circuit-breaker. A 429 received via config A will not suppress requests from config B even though they share the same quota.
If configs are always 1:1 with portals this is fine, but it's worth confirming or renaming the key to avoid misleading future readers:
// If config IDs == portal IDs, rename to make intent clear:
return
sprintf
(
'
hubspot:ratelimit:config:%d
'
,
$
this
->
config
->
getId
());
Copy
3.
isHubspotRateLimit()
and
parseRetryAfter()
are
public
but not in the interface
These two methods (
Client.php:123
,
137
) are an internal concern of the rate-limit handling, but they are
public
. Nothing outside
Client
needs to call them (the pagination service calls
isUnauthorizedException
, not these). Making them
private
or at least
protected
avoids callers depending on them in tests or subclasses. The only reason to keep them public would be if they're tested in isolation — but they currently have no direct tests at all.
4. Missing test coverage for
executeRequest()
,
isHubspotRateLimit()
,
parseRetryAfter()
SonarCloud is already flagging 34.4% coverage. The three methods above — which contain the core logic of this PR — have zero test coverage. In particular:
parseRetryAfter()
: the
Retry-After
header parsing, fallback to message keyword matching, and the default-10-second case
isHubspotRateLimit()
: each of the exception type branches
executeRequest()
: the cached circuit-breaker path (early
throw
when Redis key exists) and the
SET NX
path when a live 429 is received
These are the most impactful paths to cover to get over the 70% gate.
Minor observations
Minor observations
5. Test constants duplicated from production code
HandleHubspotRateLimitTest.php
re-declares
MAX_RETRY_DELAY = 600
,
MIN_RETRY_DELAY = 1
, and
JITTER_SECONDS = 5
(lines 20–22). If these ever change in the middleware, the tests will silently use the old values and could still pass while testing the wrong bounds. Consider testing relative to the actual delay value returned rather than hard-coding the expected range.
6. Double-clamping in
RateLimitException
The constructor default
$retryAfter = 1
(line 13) and
getRetryAfter()
calling
max($this->retryAfter, 1)
(line 21) both guard against zero/negative values. The
max()
in the getter is unreachable given the current signature default. Not a bug, but a minor inconsistency.
7.
RateLimitException
in
MatchActivityCrmData::failed()
The
failed()
hook (line 172) checks
$exception instanceof RateLimitException
, but the middleware always calls
$job->release()
without re-throwing, so
failed()
would only fire for rate limit exceptions if the
retryUntil()
window expires with a pending rate-limit release, or if somehow the middleware is not applied. The check is defensive and harmless, but worth noting in a comment since the path is non-obvious.
Add or remove reactions
@LakyLak
JY-20725
JY-20725
add more tests
add more tests
12 / 12 checks OK
02d5214
02d5214
@sonarqubecloud
Show options
sonarqubecloud Bot commented 1 hour ago
sonarqubecloud
sonarqubecloud
Bot
commented
1 hour ago
1 hour ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate passed
Issues
0 New issues
0 New issues
0 Accepted issues
0 Accepted issues
Measures
0 Security Hotspots
0 Security Hotspots
94.8% Coverage on New Code
94.8% Coverage on New Code
0.0% Duplication on New Code
0.0% Duplication on New Code
See analysis details on SonarQube Cloud
See analysis details on SonarQube Cloud
Add or remove reactions
@LakyLak
JY-20725
JY-20725
code review suggestions
code review suggestions
3 / 6 checks OK
0f3e438
0f3e438
This branch has not been deployed
This branch has not been deployed
No deployments
Merge info
Merge info
Review required
Review required...
|
Firefox
|
JY-20725 add HS rate limit handling on activities JY-20725 add HS rate limit handling on activities rematching by LakyLak · Pull Request #12066 · jiminny/app — Work...
|
github.com/jiminny/app/pull/12066
|
20651
|