|
20917
|
SlackFileEditViewGoHistoryWindowHelp•APP ()DOCKER₴ SlackFileEditViewGoHistoryWindowHelp•APP ()DOCKER₴81DEV (docker)₴2APP (-zsh)-zsh+ScrmService->syncOpportunity('374720564');ScrmService->matchByName('Robot');end diffFixed 4 of 5666 files in 146.870 seconds, 60.00 MB memory usedWhat's next:Try Docker Debug for seamless, persistentdebugging tools in any containerimage →Learn moreat [URL_WITH_CREDENTIALS] ~/jiminny/app (JY-20725-handle-HS-search-rate-lirdocker exec -it docker_lamp_1 ./vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.plPHP CS Fixer 3.87.1 Alexander by Fabien Potencier, Dariusz Ruminskicontributors.PHP runtime: 8.3.30Running analysis on 7 cores with 10 files per process.Parallel runner is an experimental feature and may be unstable, use it at your own risk.Loadedconfig default from".php-cs-fixer.dist.php".5666/5666 [100%Fixed 0 of 5666 files in 66.457 seconds, 60.00 MB memory usedWhat's next:Try Docker Debug for seamless, persistent debugging tools in any containeror image →Learn more at https://docs.docker.com/go/debug-cli/lukas@Lukas-Kovaliks-MacBook-Pro-Jiminny ~/jiminny/app (JY-20725-handle-HS-search-rate-lir•• 0ED→HomeDMsActivityFilesLaterMoreJiminny ...# contusion-clinic# curiosity_lab# engineering# general# jiminny-bg# platform-tickets# product_launches# random# releases# sofia-office# support# thank-yous# the_people_of jimi...^ Direct messagesP. Aneliya Angelova% Galya Dimitrova I. Petko Kashinski&. Stefka StoyanovaVasil VasilevCo Nikolay IvanovAneliya Angelova, ...Stoyan Tanev&o VesE. Lukas Kovalik y... O::: AppsS Jira CloudToast(abl100% <78• Mon 11 May 19:32:07Describe what you are looking for# releases8 226 0• MessagesC FilesBookmarks+9353b609 - JYToday~bee402b7- JY-20395 Stop fetching ALLheaders (including file) for email importd6a0fe5a - JY-20395 lintdaa0373e - JY-20395 lint fixesaf59d609 - JY-20395 Removed no longer usedmethodsShow morejiminny/app| Added by GitHubCircleCl APP 5:11 PMDeployment Successful!Project: appWhen:05/11/202614:11:35Tag:View JobCircleCl APP 5:35 PMDeployment Successful!Project: appWhen:05/11/202614:35:58Tag:View JobMessage #releases+..•...
|
Firefox
|
SQLite Web: db.sqlite — Personal
|
sqlite.screenpipe.lakylak.xyz
|
20917
|
|
20918
|
New Tab
New Tab
Screenpipe — Archive
Screenpipe — New Tab
New Tab
Screenpipe — Archive
Screenpipe — Archive
All docs · AFFiNE
All docs · AFFiNE
DXP4800PLUS-B5F8
DXP4800PLUS-B5F8
New Tab...
|
Firefox
|
SQLite Web: archive.db — Personal
|
db.screenpipe.lakylak.xyz/frames/content/
|
20918
|
|
20919
|
VIeWWinaowScreenpipe sync script failing after rec VIeWWinaowScreenpipe sync script failing after recent migrations0, Chat°= Cowork+ New chat5 Projectso0 Arutacts₴ CustomizeBulgarian citizenship application proces:Dawarich location tracking projectKecentsHubspot Badkequest headers debuggin.Screenpipe sunc script tailing after receiMonthly expense trackingExporting transaction data from Notion t® How much have I spent for groc...Code dift reviewHubSpot rate limit imolementation strateScreenpipe retention policy code locatioViewing retention policy in screenpipeclean snot x video recoraing terminauiHubSpot rate limit handling with executeUntitledgScreen pipe. Is there ability.SMB mount access inconsistency betwesWhat is the best switch I can..Last swimming outing with DaniDefinition of incarceratedClick to collapse *8Wouldn't it be easier to implement a retention policy that the ScreenPipe, forsome reason, doesn't support and always remove all your data from the originalScreenPipe SQLite database as a part of the script that moves the data to theNAS? Let's say I would like to move data from yesterday and also remove olderdataTrom the NAs, like, let's sav. more than one week. Can vou write such ascript, or, first of all, is it a better solution? I can still update the NAS once inthree montns, and then can create a sevarare darabase out al wavs nave onethree-month database for ScreenPipe.Architected retention-based sync strategy with reinstall detection handling >Yes, this is genuinely a better fit for your use case. Let me explain why, then write the script.Why this approach winsThe composite-key design solves a problem you'd only hit if Mac and NAS both heldoverladping data forever. But the workflow vou actually want — Mac holds a rolling 7-davwindow, NAS holds everything historical - means the two databases never overlap in time.Mac always has only recent data. NAS has everything older. So:• Mac source IDs stay monotonic forever because you only ever DELETE old rows(DELETE preserves sqlite_sequence ). Source's id=50000 today, id=50001 next, no• NAS uses source's exact schema - clone it via . schema and execute on NAS, preservingINTEGER PRIMARY KEY AUTOINCREMENT. RealPKs. real INSERT OR IGNORE no sDecialhandling.• Queries stay simple - SELECT * FROM elements WHERE frame_id = X works withoutcomposite joins.• Performance is optimal - INTEGER PK = ROWID alias, single B-tree traversal.The one edge case: reinstallIf you reinstall screenpipe, source's machine_id changes and ids restart at 1. NAS still has ids1..N from the previous era. New INSERT OR IGNORE would silently skip new data as"duplicates."Solution: the sync script detects the machine_id change and rotates the NAS archive -renamesarchive.dh to archive-<eras-<date>.dh and startstrech. Bofh eras stavqueryable; current archive is always clean. This matches your quarterly-rotation idea anyway:rotations happen automatically on reinstall, plu v u can trigger one manually every 3 monthsfor size reasons.Write a message…Opus 4.7 Adaptive vlK Lukas. ProClaude is Al and can make mistakes. Please double-check responses.New Tab@ Screenpipe — Archive& All docs - AFFINEam) DXP4800PLUS-B5F8New Tab(* Screenpipe - Archive@ SQLite Web: archive.db x(* SQLite Web: db.sqlite4 New Tal, 50 lal100% C28• Mon 11 May 19:32:07ipe.lakylak.xyz/frames/content/sqlite-web 0.7.2archive.db frames 96749 rows, showing page 1eletlielllselements fts (v)elements_fts_configelements_fts_dataelements_fts_idxframesframes_fts (v)frames_fts_configtrames tis datalframes_fts_idxmeetingsOCr. Textui_eventsui_events_fts (v)ui_events_fts_configui_events_fts_dataui_events_fts_idxViceo cnunksToggle helper tablesstructureContentQuervExportvideo_chunk_id offset_index timestampname8037 1522026-04-14T06:23:20.487451+00:00Users/lukas/.screenpipe/data/data/2026-04-14/1776app name window name Tocused browser url NULLSlack2026-04-14T06:23:20.994555÷00:00 /Users/Slacklukas/.screenpipe/data/2026-04-14/17762026-04-14T06:23:23.730853+00:00/Users/lukas/.screenpipeldataldata/2026-04-14/17762026-04-14T06:23:24.310310+00:00/Users/data/datal2026-04-14/177612026-04-14T06:23:25.864393+00:00 /Users/lukas/.screenpipe/data/data/2026-04-14/1776NULL2026-04-14T06:23:25.848448+00:00 /Users/lukas/.screenpipe/dataldata/2026-04-14/1776Slack2026-04-14T06:23:27.230179÷00:00/Users/lukas/.screenpipe/data/2026-04-14/1776NULL2026-04-14T06:23:30.287923+00:00/Users/lukas/.screenoioeldatoldata/2026-04-14/1776NULLaleam9229970ndananlearellNULUlukas/.screenpipe/data/datal2026-04-14/17762026-04-14706:23:56.387802÷00:00 /Users/NULLlukas/.screenpipe/data/data/2026-04-14/1776Jiminny Inc-Slack(Channel) -Jiminny Inc -SlackNULLNULLNULLIToast (DM) -Jiminny Inc -SlackNULLNULLNULLINULINULLNULLNULLNULLNULLNULLNULLNULLdevicemonitomonitomonitomonitomonitomonitomonitcmonitomonitomonitoOAA5 mananal...
|
Firefox
|
SQLite Web: db.sqlite — Personal
|
sqlite.screenpipe.lakylak.xyz
|
20919
|
|
20920
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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...
|
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
|
20920
|
|
20921
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented...
|
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
|
20921
|
|
20922
|
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
Pull requests · jiminny/app
Pull requests · 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
Awaiting approval
Awaiting approval
Code
Code
Open
LakyLak
LakyLak
wants to merge 4 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 (4)
Commits
(
4
)
Checks (2)
Checks
(
2
)
Files changed (12)
Files changed
(
12
)
Open
JY-20725 add HS rate limit handling on activities rematching #12066 LakyLak wants to merge 4 commits into master from JY-20725-handle-HS-search-rate-limit Copy head branch name to clipboard
JY-20725 add HS rate limit handling on activities rematching
JY-20725 add HS rate limit handling on activities rematching
#
12066
LakyLak
LakyLak
wants to merge 4 commits into
master
master
from
JY-20725-handle-HS-search-rate-limit...
|
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
|
20922
|
|
20923
|
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...
|
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
|
20923
|
|
20924
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20924
|
|
20925
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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....
|
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
|
20925
|
|
20926
|
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
Pull requests · jiminny/app
Pull requests · 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
Awaiting approval
Awaiting approval
Code
Code
Open
LakyLak
LakyLak
wants to merge 4 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 (4)
Commits
(
4
)
Checks (2)
Checks
(
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
|
20926
|
|
20927
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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
12 / 12 checks OK
0f3e438
0f3e438
@sonarqubecloud
Show options
sonarqubecloud Bot commented 16 minutes ago
sonarqubecloud
sonarqubecloud
Bot
commented
16 minutes ago
16 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...
|
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
|
20927
|
|
20928
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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
(...
|
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
|
20928
|
|
20929
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20929
|
|
20930
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20930
|
|
20931
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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
12 / 12 checks OK
0f3e438
0f3e438
@sonarqubecloud
Show options
sonarqubecloud Bot commented 17 minutes ago
sonarqubecloud
sonarqubecloud
Bot
commented
17 minutes ago
17 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...
|
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
|
20931
|
|
20932
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20932
|
|
20933
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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
12 / 12 checks OK
0f3e438
0f3e438
@sonarqubecloud
Show options
sonarqubecloud Bot commented 18 minutes ago
sonarqubecloud
sonarqubecloud
Bot
commented
18 minutes ago
18 minutes ago
Quality Gate Passed Quality Gate 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
|
20933
|
|
20934
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20934
|
|
20935
|
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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20935
|
|
20936
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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
,...
|
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
|
20936
|
|
20937
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20937
|
|
20938
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20938
|
|
20939
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 hours ago
•
edited...
|
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
|
20939
|
|
20940
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20940
|
|
20941
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20941
|
|
20942
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
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
|
20942
|
|
20943
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20943
|
|
20944
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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()...
|
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
|
20944
|
|
20945
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20945
|
|
20946
|
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
Pull requests · jiminny/app
Pull requests · 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...
|
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
|
20946
|
|
20947
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20947
|
|
20948
|
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
Pull requests · jiminny/app
Pull requests · 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
Awaiting approval
Awaiting approval
Code
Code
Open
LakyLak
LakyLak
wants to merge 4 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)...
|
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
|
20948
|
|
20949
|
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
Pull requests · jiminny/app
Pull requests · 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...
|
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
|
20949
|
|
20950
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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
,...
|
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
|
20950
|
|
20951
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20951
|
|
20952
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 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
|
20952
|
|
20953
|
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
Pull requests · jiminny/app
Pull requests · 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
Awaiting approval
Awaiting approval
Code
Code
Open
LakyLak
LakyLak
wants to merge 4 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 (4)
Commits
(
4
)
Checks (2)
Checks
(
2
)
Files changed (12)
Files changed
(
12
)
Open
JY-20725 add HS rate limit handling on activities rematching #12066 LakyLak wants to merge 4 commits into master from JY-20725-handle-HS-search-rate-limit Copy head branch name to clipboard
JY-20725 add HS rate limit handling on activities rematching
JY-20725 add HS rate limit handling on activities rematching
#
12066
LakyLak
LakyLak
wants to merge 4 commits into...
|
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
|
20953
|
|
20954
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
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
|
20954
|
|
20955
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours 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...
|
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
|
20955
|
|
20956
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20956
|
|
20957
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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 2 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
2 hours ago
2 hours ago
Quality Gate Passed Quality Gate passed
Quality Gate Passed
Quality Gate 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
|
20957
|
|
20958
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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....
|
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
|
20958
|
|
20959
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20959
|
|
20960
|
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
Pull requests · jiminny/app
Pull requests · 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
Awaiting approval
Awaiting approval
Code
Code
Open
LakyLak
LakyLak
wants to merge 4 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 (4)
Commits
(
4
)
Checks (2)
Checks
(
2
)
Files changed (12)
Files changed
(
12
)
Open
JY-20725 add HS rate limit handling on activities rematching #12066 LakyLak wants to merge 4 commits into master from JY-20725-handle-HS-search-rate-limit Copy head branch name to clipboard
JY-20725 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
|
20960
|
|
20961
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 hours 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
|
20961
|
|
20962
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20962
|
|
20963
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20963
|
|
20964
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes...
|
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
|
20964
|
|
20965
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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()...
|
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
|
20965
|
|
20966
|
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 4 hours ago
LakyLak
LakyLak
commented
4 hours ago
4 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
4 hours ago
4 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 3 hours ago
sonarqubecloud
sonarqubecloud
Bot
commented
3 hours ago
3 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 3 hours ago
LakyLak
LakyLak
commented
3 hours ago
3 hours ago
@claude
@claude
Add or remove reactions
react with eyes
👀
1
@claude
Show options
claude Bot commented 3 hours ago •
claude
claude
Bot
commented
3 hours ago
3 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...
|
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
|
20966
|