fix: url inspector channel filtering check for semrush dashboards - #2940
fix: url inspector channel filtering check for semrush dashboards#2940vivesing wants to merge 5 commits into
Conversation
|
This PR will trigger a patch release when merged. |
There was a problem hiding this comment.
Hey @vivesing,
⚠ 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 - missing unit test for the new shared utility.
Complexity: MEDIUM - small diff; API surface signal.
Changes: Extracts channel normalization into a shared normalizeChannel utility to fix case/format-insensitive filtering between UI snake_case params and Semrush Title Case responses (5 files).
Must fix before merge
- [Important] No unit test for
normalizeChannel-src/support/elements/constants.js:76(details inline)
Non-blocking (2): minor issues and suggestions
- nit: Comment changed to "Channel filter" but this block is specifically the "Owned" filter (hardcoded
!== 'owned'check) - the old wording was more accurate -src/support/elements/definitions/owned-urls.js:134 - suggestion: Align the normalized-channel variable name across
cited-domains.js(wanted) anddomain-urls.js(channel) since they are described as mirroring each other -src/support/elements/definitions/domain-urls.js:131
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 21s | Cost: $2.90 | Commit: 5aa7efae2631802019af76191590777651a1bc1f
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Multi-persona review — PR #2940Recommendation: Approve. No critical, major, security, or DB issues. CI is green, including
|
calvarezg
left a comment
There was a problem hiding this comment.
Approving — multi-persona review found no blocking issues. One required follow-up: add a focused unit test for normalizeChannel (waived for urgency, not dropped). See review comment.
There was a problem hiding this comment.
Hey @vivesing,
⚠ 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 - whitespace-run gap in normalizeChannel creates a latent correctness risk.
Complexity: MEDIUM - small diff; API surface signal.
Changes: Extracts channel normalization into a shared normalizeChannel utility to fix case/format-insensitive filtering between UI snake_case params and Semrush Title Case responses (6 files).
Note: CI checks are currently pending - confirm green before merge.
Must fix before merge
- [Important] normalizeChannel does not collapse whitespace runs -
src/support/elements/constants.js:78(details inline)
Non-blocking (2): minor issues and suggestions
- nit: Comment changed to "Channel filter" but this block filters domain_type, not a channel param - revert to "Owned filter" or similar -
src/support/elements/definitions/owned-urls.js:135 - suggestion: Align normalized-channel variable name across
cited-domains.js(wanted) anddomain-urls.js(channel) for grep-ability -src/support/elements/definitions/domain-urls.js:131
Previously flagged, now resolved
- Unit test for normalizeChannel added (test/support/elements/constants.test.js with 9 cases covering null, undefined, empty, whitespace, snake_case, Title Case, hyphenated, repeated separators, single-word)
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 3s | Cost: $5.22 | Commit: 2ae53f63ef44ea958ebb0fe876a04a92d1051cb1
If this code review was useful, please react with 👍. Otherwise, react with 👎.
There was a problem hiding this comment.
Hey @vivesing,
⚠ 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 blocking findings resolved, no new issues.
Complexity: HIGH - medium diff; API surface risk signal.
Changes: Extracts channel normalization into a shared normalizeChannel utility to fix format-insensitive filtering between UI snake_case params and Semrush Title Case responses (6 files).
Note: CI checks are currently pending - confirm green before merge.
Non-blocking (4): minor issues and suggestions
- nit: Comment says "Channel filter" but this block filters domain_type to 'owned', not a channel param - revert to "Owned filter" or "domain_type filter" -
src/support/elements/definitions/owned-urls.js:134 - suggestion: Align the normalized-channel variable name across
cited-domains.js(wanted) anddomain-urls.js(channel) for grep-ability -src/support/elements/definitions/domain-urls.js:131 - nit: Consider adding a non-string numeric input test (e.g.,
normalizeChannel(42)) to document the type-guard contract -test/support/elements/constants.test.js - nit: Consider adding a mixed-separator test (e.g.,
'foo_- bar') to document the regex collapsing across separator classes -test/support/elements/constants.test.js
Previously flagged, now resolved
- Unit tests for normalizeChannel added (10 cases covering null, undefined, empty, whitespace, snake_case, Title Case, hyphenated, repeated separators, single-word, double-space)
- Whitespace-run gap fixed (regex now
/[\s_-]+/ginstead of/[_-]+/g, with test for double-space input)
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 49s | Cost: $2.94 | Commit: 5a7598ceef6196951d5c234689b93e7a620c8428
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Summary of Changes —
feat/channel-filter-srProblem
The URL Inspector
channel(content-type) query parameter was inconsistently handled across three Serenity brand-presence endpoints:owned-urlsnever read/forwardedchannelat all.cited-domainsanddomain-urlsdid filter bychannel, but only via.trim().toLowerCase()— so a value likebenchmark_competitorsnever matched the Semrush Elements API's"Benchmark Competitors"(space-separated, Title Case), silently dropping results.Changes
src/support/elements/constants.jsnormalizeChannel(value)helper: trims, lowercases, and collapses_/-to spaces, sobenchmark_competitorsandBenchmark Competitorsboth canonicalize tobenchmark competitors.src/support/elements/definitions/cited-domains.jspaginateDomainsnow usesnormalizeChannelfor thechannelvscontentTypecomparison instead of plain lowercase, fixing thebenchmark_competitorsmismatch.src/support/elements/definitions/domain-urls.jstransformDomainUrlsResponsenow usesnormalizeChannelfor the samechannelvscontentTypecomparison. Same fix as above.src/support/elements/definitions/owned-urls.jstransformOwnedUrlsResponsenow comparesdomain_typevianormalizeChannel(row.domain_type) !== 'owned'(format-insensitive) butalways filters to
owned— this endpoint intentionallel` filter, by design (unlike its two siblings).src/controllers/elements.jslistOwnedUrlsexplicitly does not readchannel/selectedChannelfrom the query, with an inline comment clarifying this is deliberate,so a caller-supplied
channelon this endpoint has no effNet effect
owned-urlsdomain_type = Ownedrowscited-domainschannel, now correctly matching all 5 values includingbenchmark_competitorsdomain-urlschannel, same fix as `citedVerification
npx mocha test/support/elements/definitions/cited-domains.test.js test/controllers/elements.test.js— 175 passing