feat: unified dashboard with AI visibility + social signals (#105)#109
feat: unified dashboard with AI visibility + social signals (#105)#109
Conversation
- Add SocialSparklineVm, BrandHealthVm, CrossSignalInsightVm view-model types - Add socialSparkline + brandVisibilityScore to PortfolioProjectVm - Add brandHealth + crossSignalInsights to ProjectCommandCenterVm - Implement buildBrandHealth and buildCrossSignalInsights builders - Export SocialData input type for social monitoring integration - Add social.mention.new, social.sentiment.negative, social.spike webhook events - Update API-routes VALID_EVENTS and CLI event descriptions - Add tests for all cross-signal insight scenarios (6 tests) Fixes #105
apps/web/src/build-dashboard.ts
Outdated
| // ── Social signal helpers ──────────────────────────────────────────────────── | ||
|
|
||
| function buildSocialSparkline(social: SocialData): SocialSparklineVm { | ||
| const counts = social.dailyMentionCounts.slice(-7) |
There was a problem hiding this comment.
[Bug] dailyMentionCounts is sliced to 7 elements here, but there is no validation that the input actually contains 7 values. The interface doc says "length must be 7", but this is a silent contract — if a caller passes fewer than 7 items, the sparkline counts will be shorter, the delta will be computed from wrong endpoints, and the spike-detection average in buildCrossSignalInsights will also be wrong.
Suggested fix: validate at the boundary (either here or in the SocialData constructor/factory):
function buildSocialSparkline(social: SocialData): SocialSparklineVm {
if (social.dailyMentionCounts.length !== 7) {
throw new Error(`dailyMentionCounts must have exactly 7 elements, got ${social.dailyMentionCounts.length}`)
}
const counts = social.dailyMentionCounts
...
}Alternatively, add a Zod schema for SocialData in the contracts package so this is enforced at the API boundary.
There was a problem hiding this comment.
Addressed in commit 18f30aa — added length validation in buildSocialSparkline that throws if dailyMentionCounts.length !== 7.
apps/web/src/build-dashboard.ts
Outdated
|
|
||
| function computeDomainLinkRate(social: SocialData): number { | ||
| if (social.totalMentions === 0) return 0 | ||
| return Math.round((social.mentionsWithCanonicalLink / social.totalMentions) * 100) |
There was a problem hiding this comment.
[Bug] computeDomainLinkRate can return a value greater than 100 if mentionsWithCanonicalLink > totalMentions — a scenario that can occur with inconsistent upstream data (e.g. deduplication differences between the two counts). Since this is used as a percentage fed into scoreTone() and computeCompositeScore(), values above 100 will silently corrupt those scores.
Suggested fix:
function computeDomainLinkRate(social: SocialData): number {
if (social.totalMentions === 0) return 0
const rate = (social.mentionsWithCanonicalLink / social.totalMentions) * 100
return Math.min(100, Math.round(rate))
}There was a problem hiding this comment.
Addressed in commit 18f30aa — computeDomainLinkRate now uses Math.min(100, Math.round(rate)) to cap the result at 100%.
apps/web/src/build-dashboard.ts
Outdated
| // Insight: high AI visibility but low social discussion | ||
| const citedKeywords = evidence.filter(e => e.citationState === 'cited').map(e => e.keyword) | ||
| const totalMentions = social.totalMentions | ||
| if (citedKeywords.length > 0 && totalMentions < 10) { |
There was a problem hiding this comment.
[Style] Magic numbers 10 (here) and 20 (line 765) are the thresholds that gate two different insights, but their values are unexplained and asymmetric — it is unclear why "low social" is < 10 mentions while "good social traction" requires ≥ 20. These should be named constants with a comment explaining the rationale:
/** Minimum mentions considered "active social discussion" for cross-signal insights. */
const LOW_SOCIAL_THRESHOLD = 10
/** Minimum mentions to qualify as meaningful social traction vs AI citation gap. */
const SOCIAL_TRACTION_THRESHOLD = 20This also makes it much easier to tune these values later without hunting through the function body.
There was a problem hiding this comment.
Addressed in commit 18f30aa — extracted LOW_SOCIAL_THRESHOLD = 10 and SOCIAL_TRACTION_THRESHOLD = 20 as named constants with JSDoc comments explaining their purpose. Both thresholds now reference these constants.
apps/web/src/build-dashboard.ts
Outdated
|
|
||
| // Insight: high AI visibility but low social discussion | ||
| const citedKeywords = evidence.filter(e => e.citationState === 'cited').map(e => e.keyword) | ||
| const totalMentions = social.totalMentions |
There was a problem hiding this comment.
[Bug] totalMentions and dailyMentionCounts are independent fields on SocialData, but logically totalMentions should equal dailyMentionCounts.reduce((s, n) => s + n, 0). If a caller provides mismatched values (e.g. totalMentions: 50 but dailyMentionCounts summing to 21), the two thresholds on lines 726 and 765 can fire in contradictory ways — the high-AI/low-social insight could trigger while simultaneously the social-traction insight also fires.
Consider either:
- Deriving
totalMentionsfrom the array:const totalMentions = social.dailyMentionCounts.reduce((s, n) => s + n, 0) - Adding a runtime consistency check when consuming the data.
There was a problem hiding this comment.
Addressed in commit 18f30aa — totalMentions in buildCrossSignalInsights is now derived from social.dailyMentionCounts.reduce(...) rather than using social.totalMentions directly, ensuring the threshold comparisons are always consistent with the spike-detection logic.
| import { deliverWebhook, resolveWebhookTarget } from './webhooks.js' | ||
|
|
||
| const VALID_EVENTS: NotificationEvent[] = ['citation.lost', 'citation.gained', 'run.completed', 'run.failed'] | ||
| const VALID_EVENTS: NotificationEvent[] = [ |
There was a problem hiding this comment.
[Bug] VALID_EVENTS is a manually maintained list that must stay in sync with notificationEventSchema in the contracts package. This PR demonstrates the pattern — both were updated together — but it is a future footgun: adding a new event to the Zod schema without updating this array will silently allow webhooks to be created for the new event but the GET /notifications/events endpoint will not advertise it.
Since notificationEventSchema already exports its values, derive the list instead:
import { notificationEventSchema } from '@ainyc/canonry-contracts'
const VALID_EVENTS: NotificationEvent[] = notificationEventSchema.optionsz.enum(...).options returns the tuple of enum values at runtime, so this stays in sync automatically.
There was a problem hiding this comment.
Addressed in commit 18f30aa — VALID_EVENTS is now derived from notificationEventSchema.options instead of a manually maintained list, so it stays in sync automatically.
|
|
||
| export interface SocialWebhookPayload { | ||
| source: 'canonry' | ||
| event: 'social.mention.new' | 'social.sentiment.negative' | 'social.spike' |
There was a problem hiding this comment.
[Type Safety] The event field here re-declares the social event names as a raw string literal union instead of reusing NotificationEvent. This is redundant and will silently diverge if a social event is ever renamed in the schema.
Also, SocialWebhookPayload and WebhookPayload are currently two completely separate interfaces with overlapping shapes. Consumers that handle webhooks via a union type need to discover SocialWebhookPayload exists — it is not referenced from WebhookPayload anywhere.
Suggested fix:
export interface SocialWebhookPayload {
source: 'canonry'
// Narrow to just the social events via Extract:
event: Extract<NotificationEvent, `social.${string}`>
...
}
// Export a discriminated union for webhook consumers:
export type AnyWebhookPayload = WebhookPayload | SocialWebhookPayloadThere was a problem hiding this comment.
Addressed in commit 18f30aa — SocialWebhookPayload.event now uses Extract<NotificationEvent, social.${string}> instead of raw string literals, so it stays in sync with NotificationEvent automatically. AnyWebhookPayload = WebhookPayload | SocialWebhookPayload is also exported as a discriminated union.
| tone: MetricTone | ||
| title: string | ||
| detail: string | ||
| keyword?: string |
There was a problem hiding this comment.
[Style] keyword and platform are declared on CrossSignalInsightVm but none of the four insights.push(...) calls in buildCrossSignalInsights ever sets them. Consumers cannot rely on these fields being populated, and the dead fields add noise to the interface.
Either:
- Remove them if there is no near-term plan to populate them, or
- Open a follow-up issue and add a
// TODOcomment explaining when/how they will be used (e.g. for per-keyword or per-platform insights in a future iteration).
There was a problem hiding this comment.
Addressed in commit 18f30aa — added TODO comments on both keyword and platform fields in CrossSignalInsightVm to make the intent explicit: // TODO: populate when per-keyword/per-platform cross-signal insights are implemented.
| const insights = buildCrossSignalInsights(evidence, social) | ||
| expect(insights.find(i => i.id === 'cross_high_ai_low_social')).toBeUndefined() | ||
| expect(insights.find(i => i.id === 'cross_negative_sentiment')).toBeUndefined() | ||
| expect(insights.find(i => i.id === 'cross_social_no_ai')).toBeUndefined() |
There was a problem hiding this comment.
[Testing] The healthy-state test asserts that three specific insights are absent, but it does not assert the absence of cross_social_spike. With dailyMentionCounts: [7, 7, 7, 7, 7, 7, 8] the last day (8) is 1.14× the 6-day average (7) — well below the 2× spike threshold — so the test happens to pass today. But if the threshold is ever lowered, this test will silently start missing a regression.
Add the missing assertion:
expect(insights.find(i => i.id === 'cross_social_spike')).toBeUndefined()There was a problem hiding this comment.
Addressed in commit 18f30aa — added expect(insights.find(i => i.id === 'cross_social_spike')).toBeUndefined() to the healthy-state test.
arberx
left a comment
There was a problem hiding this comment.
🤖 Automated Review Summary
PR: feat: unified dashboard with AI visibility + social signals (#105)
Files reviewed: 8 | Inline comments: 8 | Critical issues: 0
Overview
Solid feature work — the new buildCrossSignalInsights logic is well-structured, the composite scoring approach is sensible, and the test suite covers the main code paths. The notification contract changes are clean and consistent. A few correctness and maintainability issues worth addressing before merge:
🔴 Correctness Issues
| File | Line | Issue |
|---|---|---|
build-dashboard.ts |
666 | dailyMentionCounts length is never validated — fewer than 7 items silently corrupts sparkline and spike detection |
build-dashboard.ts |
684 | computeDomainLinkRate can return > 100 if upstream data has mentionsWithCanonicalLink > totalMentions |
build-dashboard.ts |
725 | totalMentions and sum(dailyMentionCounts) are independent fields — a mismatch can cause contradictory insights to fire simultaneously |
🟡 Maintainability / Type Safety
| File | Line | Issue |
|---|---|---|
notifications.ts |
9 | VALID_EVENTS is manually synced with notificationEventSchema; derive it from schema.options instead |
notification.ts |
44 | SocialWebhookPayload.event re-declares raw string literals instead of using NotificationEvent; no exported union type for consumers |
view-models.ts |
89 | keyword and platform fields are declared but never populated by any current insight builder |
🟢 Testing
| File | Line | Issue |
|---|---|---|
build-social.test.ts |
104 | Healthy-state test is missing an assertion for cross_social_spike absence |
What looks good ✅
- Zero-guard on
computeSentimentScore(total === 0 → return 0) andcomputeDomainLinkRate(totalMentions === 0 → return 0) buildCrossSignalInsightscorrectly handlesnull | undefinedsocial data and returns early- Spike detection uses
counts.length >= 2guard, avoiding division-by-zero on theprior.lengthaverage - New notification events (
social.mention.new,social.sentiment.negative,social.spike) are consistently added across contracts, api-routes, CLI, and tests SocialDatais typed associalData?: SocialData | null— correct optionality that lets existing callers omit the field without changes- Mock data updated to include new required fields (
brandHealth: null,crossSignalInsights: [], etc.)
None of the issues are data-loss or security risks — marking as COMMENT rather than REQUEST_CHANGES.
- Validate dailyMentionCounts.length === 7 in buildSocialSparkline
- Cap computeDomainLinkRate at 100 to prevent corrupted scores
- Extract LOW_SOCIAL_THRESHOLD and SOCIAL_TRACTION_THRESHOLD constants
- Derive totalMentions from dailyMentionCounts for consistency
- Derive VALID_EVENTS from notificationEventSchema.options
- Use Extract<NotificationEvent, `social.${string}`> in SocialWebhookPayload
- Export AnyWebhookPayload discriminated union
- Add TODO comments for unpopulated keyword/platform fields on CrossSignalInsightVm
- Add missing cross_social_spike absence assertion in healthy-state test
Addresses review feedback on PR #109
Summary
Implements the foundational data layer for the unified brand health dashboard combining AI answer engine visibility scores with social platform signals.
Changes
View Models (
apps/web/src/view-models.ts)SocialSparklineVm— 7-day mention trend with delta and sentiment summaryBrandHealthVm— combined AI visibility + social reach + sentiment + domain link rate + composite scoreCrossSignalInsightVm— cross-signal observation with tone, title, and detailPortfolioProjectVmwithsocialSparklineandbrandVisibilityScorefieldsProjectCommandCenterVmwithbrandHealthandcrossSignalInsightsfieldsDashboard Builder (
apps/web/src/build-dashboard.ts)SocialDatainput type for consuming social monitoring databuildBrandHealth()— computes AI visibility score, social reach, sentiment score (% positive), domain link rate, and weighted composite scorebuildCrossSignalInsights()— generates cross-signal observations:buildPortfolioProject()to populatesocialSparklineandbrandVisibilityScorebuildProjectCommandCenter()to populatebrandHealthandcrossSignalInsightsWebhook Events
packages/contracts/src/notification.ts— Addedsocial.mention.new,social.sentiment.negative,social.spiketonotificationEventSchema; addedSocialWebhookPayloadinterfacepackages/api-routes/src/notifications.ts— Added new events toVALID_EVENTSpackages/canonry/src/commands/notify.ts— Added descriptions for new eventsTests
notify-events.test.ts— checks for all 7 events (up from 4)apps/web/test/build-social.test.ts— 6 tests covering all cross-signal insight scenarios and the healthy/no-data edge casesTesting
All new and modified tests pass (40 tests in apps/web, 66 tests in packages/canonry). Pre-existing failures in
packages/canonry(6 tests) are caused by a missing@ainyc/canonry-provider-cdpnode_modules install and are unrelated to this PR.Caveats
This PR implements the data layer (view-models + builders + webhook contracts). UI rendering components (
ScoreGauge,ToneBadgeusage for brand health section, sparkline chart component) are expected to be implemented separately as the design system evolves.Fixes #105