Repository navigation
feat(today): Today AI insight chip (#221) - #317
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: PR #317
Scope: PR #317 — feat(today): Today AI insight chip (#221)
Recommendation: APPROVE
Summary
PR #317 adds a contextual AI "nudge" chip to the Today screen Astra card on both web and mobile, free-riding the existing GET /api/habits/summary response by adding an insight field. The change is well-scoped: both platforms are updated identically, i18n keys land in both locales in the same commit, tests cover the three key render conditions (pro+insight present, no insight, free user), and token usage follows DESIGN.md semantic-token rules. No Critical or High findings survived adversarial challenge.
Findings
Critical
None.
High
None.
Medium
None. One candidate — SummaryResponse.insight typed as string vs query.data?.insight ?? null — was reviewed adversarially and refuted. The ?? null guard handles query.data being undefined during TanStack Query's loading lifecycle, not a nullable field in the API response. The interface correctly models the API shape.
Low / Info
None posted (Signal gate: Low/Info suppressed on PR reviews per rubric).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — web and mobile hooks and components updated with identical logic; platform adapters (BFF fetch vs apiClient, CSS vs StyleSheet) are intentional differences |
| i18n-syncer | IN SYNC — summary.insightLabel present in both en.json and pt-BR.json; both files have 1703 keys after the change |
| contract-aligner | N/A — packages/shared types not changed (SummaryResponse is a local interface, not a shared Zod schema); alignment verified via paired API PR orbit-api#256 |
| security-reviewer | N/A — no orbit-api code in this diff |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI environment; PR body reports green |
| Type check | N/A — CI environment; PR body reports green |
| Tests | N/A — CI environment; PR body reports 1643 web / 625 mobile / 1013 shared passing |
| Build (api) | N/A — no orbit-api changes in this diff |
Deferred
- Dimension 5 (No-workaround): N/A — no client-side reshaping; insight returned directly from existing summary endpoint.
- Dimension 7 (No console.log): N/A — no console.log in any changed file.
- Dimension 12 (Security): N/A — no auth, data exposure, or injection surface; read-only AI text rendered via JSX.
- Dimension 13 (Backend hard rules): N/A — no orbit-api code changed.
- Backward-compat guard: SummaryResponse is a local interface, not exported from packages/shared/src/types/. Adding insight: string is purely additive — old mobile clients receive undefined for the field, fall through the ?? null guard, and render no chip. No old-client-break finding.
What's good
- Clean free-ride on the existing cached, gated summary call — no new endpoint, query key, or AI round-trip.
- Token usage is correct: rgba(var(--primary-rgb),0.16) and inset ring on web; tintFromPrimary(tokens, 0.16/0.32) on mobile — no hardcoded violet rgba, no raw slate references.
- Render condition (isSummaryText && insight) is precise: chip only appears for Pro + AI-enabled + no error + summary present + insight non-null.
- Three-test suite on both platforms asserts behavior, not implementation details.
- Parity is genuine — logically identical across both platforms.
Recommendation
Clean merge. No blocking issues. The PR does exactly what it says, respects the design system, and ships full parity with tests on both platforms.
🤖 Generated with Claude Code
There was a problem hiding this comment.
PR 317 follow-on review (covers Merge branch main into issue-221 added after prior approval). The merge pulled in #316 and #313 — neither touches the insight chip surface. Feature diff unchanged, prior APPROVE stands. No Critical, no High. All 13 rubric dimensions PASS or N/A (see prior review for detail). SonarCloud green, 0 new issues. Clean merge. Generated with Claude Code
|



What
Adds a small contextual AI insight ("nudge") chip to the Today screen's Astra card, on both web and mobile. It rides the existing daily-summary call:
useSummary(web + mobile) now returnsinsightalongsidesummaryfrom the sameGET /api/habits/summaryresponse.TodayAISummary(web + mobile) renders the insight as a token-accurate pill at the top of the card body — only in the pro + AI-summary-enabled + summary state. Navy-violet semantic tokens (primary tint, hairline ring, radius 999,--primary-softtext); no new color/radius, single accent.summary.insightLabelin bothen.json("Nudge") andpt-BR.json("Toque"), used as the chip's accessibility label prefix. The insight text itself is backend-localized.Why
Issue #221 — a timely contextual nudge on Today without opening Astra. No new endpoint, query key, or AI call: the chip free-rides the already-gated, already-cached daily-summary generation (gated by inheritance via
CanUseDailySummary+AiSummaryEnabled).Parity & i18n
Mirrored hook + component edits across apps/web and apps/mobile; the new label key added to both locales in the same change. "Orbit"/"Astra" stay literal.
Tests
today-ai-summarycomponent tests (web RTL + mobile react-test-renderer): chip renders when an insight is present, is absent when there is none, and is hidden for free users.use-summaryweb hook test assertsinsightis parsed/returned.Closes #221
Paired API PR: thomasluizon/orbit-api#256
🤖 Generated with Claude Code