refactor(rum): use resolveRumDomainKey from spacecat-shared-rum-api-client#2648
refactor(rum): use resolveRumDomainKey from spacecat-shared-rum-api-client#2648Kanishkavijay39 wants to merge 3 commits into
Conversation
…lient Remove the inline 4-candidate waterfall and timeout loop from rum-config-service.js. The shared helper resolveRumDomainKey owns that logic now (spacecat-shared PR #1662). updateRumConfig becomes a thin wrapper that only handles the save semantics. Bumps @adobe/spacecat-shared-rum-api-client 2.40.13 → 2.43.1. Closes SITES-46321
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Hey @Kanishkavijay39,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - one resilience concern to address before merge.
Changes: Replaces inline 4-candidate waterfall + timeout logic with a shared resolveRumDomainKey helper from spacecat-shared-rum-api-client, simplifying the service to own only save semantics (4 files).
Must fix before merge
- [Important] Missing error boundary around
resolveRumDomainKey-src/support/rum-config-service.js:41(details inline)
Non-blocking (1): minor issues and suggestions
- suggestion: Add a test case for
resolveRumDomainKeyrejection to document whether crash-propagation or graceful degradation is the intended contract -test/support/rum-config-service.test.js
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 59s | Cost: $3.50 | Commit: c13a2f8dcbcffe256714f59062bb9fc6bdc6f4e7
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| const { hasDomainKey } = await resolveRumDomainKey(site, context); |
There was a problem hiding this comment.
issue (blocking): The old code wrapped the waterfall in a try/catch that guaranteed updateRumConfig always resolved (returning false on any failure). The new bare await resolveRumDomainKey(site, context) has no error boundary - if the helper throws for an unexpected reason, the exception propagates unhandled.
Note: the shared helper does handle expected errors (timeouts, lookup failures, malformed URLs) internally, so the risk is limited to unexpected internal errors. But the fix is cheap insurance that preserves the function's implicit contract of "always resolves to a boolean."
Suggested fix:
let hasDomainKey = false;
try {
({ hasDomainKey } = await resolveRumDomainKey(site, context));
} catch (e) {
context.log.warn(`[rum-config-service] resolveRumDomainKey failed: ${e.message}`);
}Preserve the implicit contract that updateRumConfig always resolves to a boolean. The shared helper handles expected failures (timeouts, lookup errors, malformed URLs) internally, but an unexpected throw would propagate unhandled without this guard.
|
This PR will trigger a patch release when merged. |
There was a problem hiding this comment.
Hey @Kanishkavijay39,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - prior concern resolved, clean refactor ready to ship.
Changes: Replaces inline 4-candidate waterfall + timeout logic with a shared resolveRumDomainKey helper from spacecat-shared-rum-api-client, simplifying the service to own only save semantics (4 files).
Previously flagged, now resolved
- Missing error boundary around
resolveRumDomainKey- now wrapped in try/catch with warn-level log and graceful fallback tohasDomainKey = false, plus a test covering the rejection path.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 44s | Cost: $2.58 | Commit: 3a42a1a34b9db40ff91348c7aa996df270babea4
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Summary
rum-config-service.jswith the sharedresolveRumDomainKeyhelper from@adobe/spacecat-shared-rum-api-client(added in spacecat-shared#1662)updateRumConfigis now a thin wrapper that only owns the save semantics@adobe/spacecat-shared-rum-api-client2.40.13→2.43.1Closes SITES-46321
Test plan
npx mocha test/support/rum-config-service.test.js— 4 tests passnpm test— full suite green