feat(run-global-import): scope a global import to a single site, with an optional --force#2860
Conversation
Adds a Slack command that force-enables Optimize at Edge for a single site by enqueuing a `force-optimize-at-edge-enabled-marking` message to the import worker's imports queue. The worker enables the site on the edge request-id (routing) check ALONE, deliberately skipping prerendered-content validation. Internal Adobe use only. Records the triggering Slack user as `forcedBy`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 @jindaliiita,
⚠ 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 - error logging pattern diverges from the codebase convention, losing the stack trace on failures.
Complexity: MEDIUM - medium diff, single-service.
Changes: Adds a new Slack command (force-optimize-at-edge-enable) that force-enables Optimize at Edge for a site by enqueuing a message to the imports queue, skipping prerendered-content validation (3 files).
Must fix before merge
- [Important] Error logging loses the stack trace -
src/support/slack/commands/force-optimize-at-edge-enable.js:79(details inline)
Non-blocking (3): minor issues and suggestions
- suggestion: Add a null guard on
Configuration.findLatest()result before calling.getQueues()- the catch block handles the resulting TypeError but the Slack user sees an opaque "Cannot read properties of null" message instead of something actionable -src/support/slack/commands/force-optimize-at-edge-enable.js:60 - suggestion: Add a test for
Site.findByIdreturningnull(the not-found path is only exercised via thefindByBaseURLbranch) -test/support/slack/commands/force-optimize-at-edge-enable.test.js - suggestion: Tighten the success-path assertion from
.calledWithMatch('force')to.calledWithMatch('Triggered')- the word "force" is too generic and would match error messages containing the command name -test/support/slack/commands/force-optimize-at-edge-enable.test.js:93
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 35s | Cost: $4.61 | Commit: 99b95336da0b8ab95af3771b4b1ae88ddb724362
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…e-enable - Preserve the stack trace on error: pass the full error object to log.error instead of interpolating error.message, matching the convention in add-oae-stage-domain.js. - Guard against Configuration.findLatest() returning nothing before calling .getQueues(), posting an actionable Slack message instead of letting an opaque TypeError fall through to the generic error handler. - Add coverage for Site.findById returning null (previously only exercised via findByBaseURL) and for the new Configuration.findLatest() null-guard branch. - Tighten the success-message assertion from a generic 'force' substring match to 'Triggered', avoiding a false-positive match against error messages containing the command name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @jindaliiita,
⚠ 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 - all prior findings addressed; clean implementation following established patterns.
Complexity: MEDIUM - medium diff, single-service.
Changes: Adds a new Slack command (force-optimize-at-edge-enable) that force-enables Optimize at Edge for a site by enqueuing a message to the imports queue, skipping prerendered-content validation (3 files).
Non-blocking (1): minor issues and suggestions
- suggestion: The
forcedByfield carries the Slack user ID (e.g.U123ABC) - if the downstream consumer ever needs a human-readable display name for notifications or audit UI, that will require an extra Slack API call at consumption time. Worth noting as a future consideration if incident-response clarity matters here -src/support/slack/commands/force-optimize-at-edge-enable.js:51
Previously flagged, now resolved
- Error logging now passes the full error object (stack trace preserved)
- Null guard on
Configuration.findLatest()with user-facing warning added Site.findByIdreturning null is now tested- Success-path assertion tightened from
'force'to'Triggered'
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 14s | Cost: $2.98 | Commit: 46fc54472ff10a9e20e2e0f5d4520139abfe4855
If this code review was useful, please react with 👍. Otherwise, react with 👎.
… import to one site Threads an optional siteId through triggerGlobalImportRun / sendGlobalImportRunMessage into the SQS message, and accepts it as a second argument on the `run global import` Slack command — e.g. `run global import optimize-at-edge-enabled-marking <siteId>` — so a specific site can be re-checked without scanning all data. Global imports that don't use it (e.g. stale-suggestions-cleanup) are unaffected.
…a --force flag Removes the separate force-optimize-at-edge-enable Slack command in favor of a --force flag on `run global import optimize-at-edge-enabled-marking <site>`, matching the import-worker side's merge of the two message types into one. --force requires a site and is rejected on a bulk run. sendGlobalImportRunMessage/triggerGlobalImportRun now take a single options object (siteId, force, forcedBy) instead of a lone siteId param.
There was a problem hiding this comment.
Hey @jindaliiita,
⚠ 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 - clean implementation extending the existing command; follows established patterns with solid utility-layer test coverage.
Complexity: MEDIUM - medium diff, single-service.
Changes: Extends the run-global-import Slack command with optional single-site scoping and a --force flag that skips prerender content validation, replacing the previously planned standalone command (3 files).
Non-blocking (3): minor issues and suggestions
- suggestion:
triggerGlobalImportRunhas no default for theoptionsparameter, relying onsendGlobalImportRunMessage's downstream= {}destructuring default to handleundefined. Addingoptions = {}to the signature makes the contract explicit at the call site -src/support/utils.js:475 - suggestion:
--forceis the first CLI-style flag in this repo's Slack commands (all others use purely positional args). If additional flags arise later, consider extracting flag-parsing into a shared utility rather than per-command helpers -src/support/slack/commands/run-global-import.js:34 - nit: The usage text documents that
--force"requires a site" but does not mention that--forceis only meaningful foroptimize-at-edge-enabled-marking. The command accepts--forcewith any import type and silently passes it downstream where it is ignored - an explicit "Note: only supported by optimize-at-edge-enabled-marking" or a validation guard would prevent operator confusion -src/support/slack/commands/run-global-import.js:60
Previously flagged, now resolved
- Error logging stack trace concern (previous standalone implementation) - moot after the pivot to extending
run-global-import - Null guard on Configuration.findLatest() - moot (standalone command removed)
- Site.findById null test - moot (standalone command removed)
- Success-path assertion tightened - moot (standalone command removed)
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 13m 25s | Cost: $6.66 | Commit: ec0f713131f4eade89a48c6a8264d093b1c86148
If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
🎉 This PR is included in version 1.683.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What
Extends the existing
run global importSlack command with:[site-url-or-id]argument to scope a global import to a single site instead of scanning all data.--forceflag (requires a site) telling the target handler to skip its normal gating check — used byoptimize-at-edge-enabled-markingto enable a site on the edge request-id check alone, skipping prerender content validation.Usage:
Why
The hourly
optimize-at-edge-enabled-markingjob only ran across all sites. Operators need to:Note on history: this PR originally added a standalone
force-optimize-at-edge-enableSlack command for the force-enable case. That's been folded intorun global import ... --forceinstead — avoids two near-identical commands and reuses the same site-resolution code path (the standalone command's add-then-remove is visible in this branch's commit history but nets to zero in the diff).Change
src/support/slack/commands/run-global-import.js: parses an optional site (URL or ID) and a--forceflag out of the args (in any order); resolves the site viaSite.findByBaseURL/Site.findById; rejects--forcewithout a site (it would otherwise silently no-op on a bulk run); passes{ siteId, force, forcedBy: <slack user> }through.src/support/utils.js:sendGlobalImportRunMessage/triggerGlobalImportRunnow take a trailing options object ({ siteId, force, forcedBy }) instead of a lonesiteIdparam.Cross-repo
Pairs with spacecat-import-worker PR #813, which added
siteId/force/forcedBysupport directly to theoptimize-at-edge-enabled-markinghandler (single-site runs skip the daily-dedup andMAX_SITES_PER_RUNcap;forceskips prerender validation only — the already-enabled idempotency guard still applies unconditionally) and removed the now-redundant standaloneforce-optimize-at-edge-enabled-markinghandler.Tests
sendGlobalImportRunMessage/triggerGlobalImportRuncovering siteId/force/forcedBy inclusion and omission.run-global-import.jsitself staysc8 ignore'd, consistent with its pre-existing convention (no dedicated test file before or after this PR).test/support/slack/commands/**): 852 passing.utils.test.js: 129 passing. Lint clean.🤖 Generated with Claude Code