feat: Serenity sentiment-overview endpoint | LLMO-6300 by calvarezg · Pull Request #2837 · adobe/spacecat-api-service

calvarezg · 2026-07-16T09:04:55Z

Summary

Adds GET /v2/orgs/:spaceCatId/brands/:brandId/serenity/brand-presence/sentiment-overview, sourcing per-week brand sentiment (positive/neutral/negative) from the Semrush SENTIMENT element and emitting the legacy { weeklyTrends } contract so the existing brand-presence sentiment chart consumes it drop-in. Direct parallel of the LLMO-6179 URL Inspector cutover — same auth, plumbing, and conventions; values differ (Semrush metrics vs Postgres), shape is identical.

Changes

definitions/sentiment-overview.js — buildSentimentOverviewPayload (mirrors cited-domains' verified filter shape) + transformSentimentOverviewResponse (rolls the element's daily bar rows into ISO weeks, computes percentages the legacy way, reuses SENTIMENT_COLORS).
elements-service.js — getSentimentOverview (single upstream call, like getCitedDomains).
controllers/elements.js — listSentimentOverview: authorizeOrg, required + validated dates, 366-day span cap, region → project_id resolution.
Route + required-capabilities (brand:read) + FACS llmo/can_view classification.
OpenAPI: path spec + SerenityBrandPresenceSentimentOverview schema + serenity contract-test fixture covering the new endpoint end-to-end.

Test plan

npm run type-check, npm run lint, npm run docs:lint + docs:build all clean.
Route suite (3117), FACS suite (31), elements unit (218), serenity OpenAPI contract (20, incl. the new endpoint) all pass.
Transform verified against a live dev probe of the SENTIMENT element (real workspace/data) — percentages sum to 100, shape matches the legacy contract.
Unit tests for the transform are deferred (c8 ignore), matching the sibling POC element endpoints (cited-domains/owned-urls/domain-urls/weeks); no mock-backed serenity IT added (siblings have none for element reads).

Notes

The element's three sentiment legends are overlapping sets (a mixed-sentiment prompt counts in each), so promptsWithSentiment can exceed totalPrompts; percentages are unaffected (internal ratios). Documented in the transform + schema.
UI wiring (project-elmo-ui) is a separate follow-up, parallel to how LLMO-6179 wired the UI after the backend landed.

Related Issues

https://jira.corp.adobe.com/browse/LLMO-6300

Add GET /v2/orgs/:spaceCatId/brands/:brandId/serenity/brand-presence/sentiment-overview, sourcing per-week brand sentiment from the Semrush SENTIMENT element and emitting the legacy { weeklyTrends } contract so the existing chart consumes it drop-in. Direct parallel of the LLMO-6179 url-inspector cutover. - definition (buildSentimentOverviewPayload + transformSentimentOverviewResponse): mirrors cited-domains' verified filter shape; rolls the element's daily bar rows into ISO weeks - getSentimentOverview service method (single call, like cited-domains) - listSentimentOverview controller handler: authorizeOrg, required+validated dates, 366-day span cap, region->project_id resolution - route + required-capabilities (brand:read) + FACS llmo/can_view classification - OpenAPI path + SerenityBrandPresenceSentimentOverview schema - serenity OpenAPI contract fixture covering the new endpoint Transform verified against a live dev probe of the SENTIMENT element. Note: the element's three sentiment legends are overlapping sets (a mixed-sentiment prompt counts in each), so promptsWithSentiment can exceed totalPrompts; percentages are unaffected (internal ratios). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

codecov · 2026-07-16T09:10:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

MysticatBot

Hey @calvarezg,

⚠ 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 - two data-integrity edge cases in the transform logic need one-line defensive fixes.
Complexity: HIGH - large diff; API surface + FACS/ReBAC wiring.
Changes: Adds a Serenity brand-presence sentiment-overview endpoint sourcing per-week sentiment from the Semrush SENTIMENT element, emitting the legacy weeklyTrends contract (13 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI specs). The bot review is a complement to, not a replacement for, a human read here.
Note: CI build is currently pending - confirm it passes before merge.

Must fix before merge

[Important] dateToIsoWeek can produce NaN-WNaN phantom entry on malformed upstream bar values - src/support/elements/definitions/sentiment-overview.js:164 (details inline)
[Important] neutralPct can go negative when Math.round pushes positive + negative above 100 - src/support/elements/definitions/sentiment-overview.js:187 (details inline)

Non-blocking (6): minor issues and suggestions

nit: cachedOk(result) diverges from sibling listCitedDomains which uses ok(result) - clarify whether caching is intentional or align with sibling - src/controllers/elements.js:419
suggestion: OpenAPI 503 description says "PostgREST client not configured" which is misleading on a primarily Semrush-backed endpoint - consider rewording to "Upstream service unavailable" - docs/openapi/serenity-api.yaml:336
nit: defaultDateRange() is dead code in the production path (controller requires dates before calling service) - consider removing or documenting as safety net - src/support/elements/definitions/sentiment-overview.js:32
suggestion: SENTIMENT_COLORS is duplicated from the legacy controller - extract to a shared constant file to eliminate drift risk - src/support/elements/definitions/sentiment-overview.js:18
nit: region query param reflected verbatim in 404 error without length cap (inherited pattern from siblings) - src/controllers/elements.js:407
nit: category concatenated into filter payload without length validation (inherited pattern, low risk behind auth) - src/support/elements/definitions/sentiment-overview.js:79

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 16s | Cost: $7.15 | Commit: 4f438ffe34c5e1a447c18e4a775a661927200735
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

MysticatBot · 2026-07-16T09:16:54Z

+    if (!day || !SENTIMENT_BUCKETS.includes(bucket)) {
+      return;
+    }
+    const week = dateToIsoWeek(day);


issue (blocking): dateToIsoWeek produces NaN-WNaN on non-date bar strings, creating a phantom aggregation entry in the response.

The guard typeof row?.bar === 'string' allows any string value through (e.g. "N/A", "total", metadata rows from the upstream element). When bar.slice(0, 10) is not a valid ISO date, dateToIsoWeek passes it to new Date() which returns Invalid Date, ultimately producing "NaN-WNaN" as the Map key. This ghost entry reaches the response with weekNumber: 0, year: 0 and accumulated junk counts.

Fix - validate the day string matches a date pattern before calling dateToIsoWeek:

if (!day || !/^\d{4}-\d{2}-\d{2}$/.test(day) || !SENTIMENT_BUCKETS.includes(bucket)) { return; }

Or validate the week output: const week = dateToIsoWeek(day); if (!/^\d{4}-W\d{2}$/.test(week)) return;

MysticatBot · 2026-07-16T09:16:54Z

+        ? Math.round((entry.positive / promptsWithSentiment) * 100) : 0;
+      const negativePct = promptsWithSentiment > 0
+        ? Math.round((entry.negative / promptsWithSentiment) * 100) : 0;
+      const neutralPct = promptsWithSentiment > 0 ? 100 - positivePct - negativePct : 0;


issue (blocking): neutralPct can go negative when Math.round pushes positive + negative above 100.

Example: if positive is 50.5% of promptsWithSentiment it rounds to 51, and if negative is 49.5% it rounds to 50, then neutralPct = 100 - 51 - 50 = -1. The OpenAPI schema describes values as 0-100 percentages; a negative value violates the contract and will produce unexpected chart behavior in the UI.

Fix (one line) - clamp to zero:

const neutralPct = promptsWithSentiment > 0 ? Math.max(0, 100 - positivePct - negativePct) : 0;

# Conflicts: # docs/index.html

github-actions · 2026-07-16T09:31:51Z

This PR will trigger a minor release when merged.

…94141060) A non-date `bar` string (e.g. a metadata/"N/A" row) sliced to junk that dateToIsoWeek turned into a "NaN-WNaN" map key, surfacing as a phantom weeklyTrends entry (weekNumber/year 0). Validate the day against YYYY-MM-DD before dateToIsoWeek in both the data and line loops via a shared isoDayOf helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Independent Math.round of positive and negative can push their sum over 100 (e.g. 50.5->51 and 49.5->50), making the neutral remainder negative and violating the OpenAPI 0-100 percentage contract. Clamp the remainder to 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

calvarezg · 2026-07-16T09:41:40Z

Triage of MysticatBot review

Addressed (2 blocking) — pushed as separate commits:

NaN-WNaN phantom entry — Fixed (be73ee3). Added an isoDayOf guard validating YYYY-MM-DD before dateToIsoWeek in both the data and line loops, so a non-date bar can no longer create a phantom week key.
neutralPct can go negative — Fixed (6fe9d59). Clamped the remainder with Math.max(0, …) to honor the 0-100 contract.

Not addressing (6 non-blocking) — rationale for each:

cachedOk vs ok (elements.js) — Intentional. Matches the newer siblings listOwnedUrls/listDomainUrls; cited-domains' plain ok is the outlier. Aggregate weekly sentiment is a good cache candidate.
503 description "PostgREST client not configured" — Accurate. The 503 genuinely fires in authorizeOrg when the PostgREST client is absent (brand→workspace resolution), before any Semrush call; matches the filter-dimensions sibling's wording.
defaultDateRange() "dead code" — Kept. Deliberate verbatim mirror of cited-domains.js (harmless safety-net default); removing it diverges from the sibling template.
SENTIMENT_COLORS duplication — Kept. support/elements must not import controllers (same reason week-utils.js was copied verbatim); the drift risk + "keep in sync" note is already documented in the file. A shared-constant extraction spanning the legacy controller is out of scope here.
region reflected in 404 without length cap (elements.js) — Inherited/low-risk. JSON body value (not a header, no injection surface), behind auth; inherited verbatim from cited-domains.
category concatenated without length validation — Inherited/low-risk. JSON payload value to Semrush (no query-language injection), behind auth; inherited from cited-domains.

Re-requested review to pick up the two fixes.

MysticatBot

Hey @calvarezg,

⚠ 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 - both prior blocking findings are addressed; no new issues found on re-review.
Complexity: HIGH - large diff; API surface + FACS/ReBAC wiring.
Changes: Adds a Serenity brand-presence sentiment-overview endpoint sourcing per-week sentiment from the Semrush SENTIMENT element, emitting the legacy weeklyTrends contract (13 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI specs). The bot review is a complement to, not a replacement for, a human read here.

Non-blocking (1): minor issues and suggestions

nit: String-based date comparison startDate > endDate relies on lexicographic ordering working for YYYY-MM-DD; a numeric comparison via Date.parse (already used a few lines below for the span check) would be more explicit - src/controllers/elements.js:588

Previously flagged, now resolved

NaN-WNaN phantom entry from non-date bar values: fixed with isoDayOf guard validating YYYY-MM-DD before dateToIsoWeek
neutralPct going negative when rounding pushes sum above 100: fixed with Math.max(0, ...) clamp

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 26s | Cost: $5.31 | Commit: 6fe9d59fc7d4b7f26178c1a4d1b16332ecf3bf97
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

jjenscodee

Issues & Risks

neutralPct clamp can break the "sums to 100" invariant.
const neutralPct = promptsWithSentiment > 0
? Math.max(0, 100 - positivePct - negativePct) : 0;
The comment and the OpenAPI schema both state the three values are "integers summing to 100." But independent rounding can produce positivePct + negativePct = 101, in which case Math.max(0, -1) = 0 and the three now sum to 101, not 100. The clamp trades one invariant (non-negative) for another (sums to 100). It's a rare edge case and the chart likely tolerates it, but either the schema/comment wording should be softened ("approximately 100 / non-negative") or the overflow should be absorbed from positive/negative. Worth a one-line decision.
defaultDateRange / DEFAULT_WINDOW_DAYS in the definition are effectively dead.
The controller mandates and validates startDate/endDate before ever calling the service, so buildSentimentOverviewPayload's default-range fallback can never trigger via the HTTP path. It's carried "for parity" and to keep the definition pure, which is defensible, but it's untested dead code today. Minor — flagging, not blocking.
Minor OpenAPI accuracy. The 503 response is documented as "PostgREST client not configured," but this endpoint reads from the Semrush element transport, not PostgREST. Looks copy-pasted from a sibling; confirm the wording is accurate for this path (or that 503 is even reachable here).

…iew) The previous Math.max(0, …) clamp fixed non-negativity but let the three values sum to 101 when rounding pushed positive+negative over 100 — breaking the "integers summing to 100" contract stated in the comment and OpenAPI schema. Absorb the 1-point rounding overflow from the larger of positive/negative so all three are non-negative AND sum to exactly 100. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…scodee review) Both reviewers flagged the rolling-window default as unreachable dead code. It is retained deliberately for parity with the sibling definitions and to keep the support layer pure; document that the controller validates dates before calling the service so the fallback is not reached via the HTTP path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The 503 is reachable and genuinely PostgREST-related (authorizeOrg returns it when the PostgREST client is absent, during brand->workspace resolution, before any Semrush call), but "not configured for this deployment" read as misleading on a Semrush-backed endpoint. Reword to state the actual condition and why. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

calvarezg · 2026-07-16T10:03:34Z

Thanks @jjenscodee — all three addressed:

sums-to-100 (59d1e53) — replaced the bare Math.max(0, …) clamp with a largest-remainder correction: the 1-point rounding overflow is absorbed from the larger of positive/negative, so the three are now non-negative and sum to exactly 100 (verified for the 50.5/49.5 overflow case → 50/0/50).
defaultDateRange (2538034) — kept for sibling parity + support-layer purity, but documented as a defensive default not reached via the date-validated HTTP path.
503 wording (0bc1666) — reworded to "PostgREST client not available (required to resolve the brand's Semrush workspace before the element call)" — the 503 is genuinely reachable during brand→workspace resolution, so it's accurate now without the misleading "not configured" phrasing.

# Conflicts: # docs/index.html # docs/openapi/api.yaml # docs/openapi/serenity-api.yaml # test/openapi-contract/serenity-api.test.js

Resolve conflicts with #2837 (sentiment-overview): keep both endpoints' OpenAPI path entries and contract-test fixtures side-by-side; adopt main's dynamic [fx.serviceMethod] stub and add serviceMethod to the market-tracking-trends fixture. Also drop stray docs/spec/email-service-spec.md that was not part of this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

# [1.658.0](v1.657.0...v1.658.0) (2026-07-16) ### Features * Serenity sentiment-overview endpoint | LLMO-6300 ([#2837](#2837)) ([bd8b6c9](bd8b6c9))

solaris007 · 2026-07-16T12:06:27Z

🎉 This PR is included in version 1.658.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Follow the same POC treatment as the sibling Serenity element endpoints (#2837 sentiment-overview, cited-domains, owned-urls, domain-urls): wrap the controller handler, service method, and definitions builders/transformer in `/* c8 ignore */` and drop the deferred unit + integration tests. The OpenAPI contract fixture and route test are kept, so the endpoint stays validated at the contract level. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

calvarezg self-assigned this Jul 16, 2026

calvarezg requested a review from MysticatBot July 16, 2026 09:05

calvarezg temporarily deployed to dev-branches July 16, 2026 09:15 — with GitHub Actions Inactive

MysticatBot requested changes Jul 16, 2026

View reviewed changes

MysticatBot added ai-reviewed Reviewed by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge labels Jul 16, 2026

Merge remote-tracking branch 'origin/main' into feat/sentiment-trend

a1209a3

# Conflicts: # docs/index.html

calvarezg and others added 2 commits July 16, 2026 11:40

calvarezg requested a review from MysticatBot July 16, 2026 09:41

MysticatBot approved these changes Jul 16, 2026

View reviewed changes

calvarezg temporarily deployed to dev-branches July 16, 2026 09:54 — with GitHub Actions Inactive

jjenscodee reviewed Jul 16, 2026

View reviewed changes

calvarezg and others added 3 commits July 16, 2026 12:02

jjenscodee approved these changes Jul 16, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat/sentiment-trend

58e70d5

# Conflicts: # docs/index.html # docs/openapi/api.yaml # docs/openapi/serenity-api.yaml # test/openapi-contract/serenity-api.test.js

calvarezg temporarily deployed to dev-branches July 16, 2026 10:31 — with GitHub Actions Inactive

calvarezg merged commit bd8b6c9 into main Jul 16, 2026
20 checks passed

calvarezg deleted the feat/sentiment-trend branch July 16, 2026 11:48

solaris007 pushed a commit that referenced this pull request Jul 16, 2026

chore(release): 1.658.0 [skip ci]

ba37ab5

# [1.658.0](v1.657.0...v1.658.0) (2026-07-16) ### Features * Serenity sentiment-overview endpoint | LLMO-6300 ([#2837](#2837)) ([bd8b6c9](bd8b6c9))

solaris007 added the released label Jul 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Serenity sentiment-overview endpoint | LLMO-6300#2837

feat: Serenity sentiment-overview endpoint | LLMO-6300#2837
calvarezg merged 8 commits into
mainfrom
feat/sentiment-trend

calvarezg commented Jul 16, 2026

Uh oh!

codecov Bot commented Jul 16, 2026

Uh oh!

MysticatBot left a comment •

edited

Loading

Uh oh!

MysticatBot Jul 16, 2026

Uh oh!

MysticatBot Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

calvarezg commented Jul 16, 2026

Uh oh!

MysticatBot left a comment •

edited

Loading

Uh oh!

jjenscodee left a comment

Uh oh!

calvarezg commented Jul 16, 2026

Uh oh!

Uh oh!

solaris007 commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

calvarezg commented Jul 16, 2026

Summary

Changes

Test plan

Notes

Related Issues

Uh oh!

codecov Bot commented Jul 16, 2026

Codecov Report

Uh oh!

MysticatBot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Must fix before merge

Uh oh!

MysticatBot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

MysticatBot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

calvarezg commented Jul 16, 2026

Triage of MysticatBot review

Uh oh!

MysticatBot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Previously flagged, now resolved

Uh oh!

jjenscodee left a comment

Choose a reason for hiding this comment

Uh oh!

calvarezg commented Jul 16, 2026

Uh oh!

Uh oh!

solaris007 commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MysticatBot left a comment •

edited

Loading

MysticatBot left a comment •

edited

Loading