fix: achievements XP row collision + queued completions never counted toward review floor - #403
Conversation
…completions toward review floor - shorten profileCard.totalXp/earned strings (both locales) and harden the stats row (gap + flexShrink) so long values never collide - track review-floor completions in onMutate instead of onSuccess: queued offline completions early-return before onSuccess and were never counted, silently blocking review-moment eligibility Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
The pathname !== '/' gate (added Apr 5 alongside the proper acknowledgedLevel re-trigger fix) excluded StreakCelebration, AllDoneCelebration, WelcomeBackToast, AchievementToast, and LevelUpOverlay from the Today tab — the screen where completions happen. Celebrations queued there could not play until the user navigated away, and the stalled queue also suppressed every post-celebration prompt. Web mounts celebrations on all routes; mobile now matches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
/pr-review — PR #403
Scope: 7 files, +30/-9. Two independent fixes: (1) achievements header stat-row collision (web + mobile + both locales), (2) offline-queued habit completions never counting toward the post-celebration review-moment floor (mobile).
Findings
None at Critical/High. No concretely-actionable Medium findings survived review either.
Verification performed
- Cross-platform parity: Both platforms received the collision fix — mobile (
achievements.tsx) addsgap: 12on the row +flexShrink: 1on the shrinking child (needed because RN defaultsflexShrink: 0, unlike CSS flexbox); web (achievement-xp-card.tsx) adds the equivalentgap: 12. Change #2 (use-habits.tsonMutatetiming fix) is mobile-only by design — it fixes behavior insidereview-reminder-store, which is a pre-existing mobile-only engagement feature (added in #382/#398) with no web equivalent to mirror. - i18n:
gamification.profileCard.totalXpand.earnedshortened in bothen.jsonandpt-BR.jsonin the same diff. All callsites (web card, mobile screen, mobile i18n test) pass matching interpolation variables. No MISSING_EN/MISSING_PT/orphaned-key issues. - Logic correctness of the
onMutatemove:trackCompletionnow fires inonMutateinstead ofonSuccess, so it runs before the offline queue can swallow the callback and before cancellation/response resolution.onErrorintentionally does not roll it back — the PR description explicitly frames this as an accepted tradeoff (rare overcount on a failed request vs. systematic miss on queued completions), and the store's role is an engagement-eligibility floor, not an exact counter — reasonable. A regression test (use-habits.test.ts) exercisesonMutatedirectly and assertscompletionCount/activeDaysupdate immediately. - Backward compatibility / API contract: No
packages/sharedtype or endpoint changes, no orbit-api touch — not applicable to this diff. - Code standards: No
any, noconsole.log, no narration comments, functions stay well under size caps.
Recommendation: Approve
Small, well-scoped, correctly paired across platforms, with a regression test covering the behavioral fix.
There was a problem hiding this comment.
/pr-review — PR #403
Scope of this review: incremental diff since my prior approval at 6ee90b39 (already APPROVED). One new commit was added since then: a41d4088 "fix(mobile): mount celebration overlays on the Today route", touching only apps/mobile/app/_layout.tsx (+6/-14).
What changed
Removes showSharedCelebrations = pathname !== '/' and the five conditional wrappers it gated (StreakCelebration, AllDoneCelebration, WelcomeBackToast, AchievementToast, LevelUpOverlay), and threads the now-unused showSharedCelebrations prop out of GlobalOverlays. Previously these overlays never mounted on the Today route (pathname === '/') — the screen where completions actually happen — so queued celebrations and the post-celebration prompts (milestone share, referral, #382 review moment) silently stalled until the user navigated away.
Verification performed
- No dead code left behind. Grepped the full file post-change — zero remaining references to
showSharedCelebrations; the prop and its type were removed cleanly from both the caller andGlobalOverlays's destructured/typed params.pathnameitself is still used elsewhere in the file (tab detection, back-fallback routing), so it wasn't orphaned. - Re-trigger risk is a non-issue.
GlobalOverlaysis mounted once alongside the root stack, not inside the route stack, so it never unmounts/remounts on navigation — the pathname gate only toggled which children rendered, it never protected against remount-driven re-fires. The "infinite re-trigger" bug the commit message references was independently fixed viaacknowledgedLevelstate inapps/mobile/hooks/use-gamification.ts(mirrored inapps/web/hooks/use-gamification.tsandpackages/shared/src/utils/gamification-selectors.ts), which gates on level value, not route — confirmed unaffected by this diff. - True cross-platform parity.
apps/web/app/(app)/layout.tsx(lines 282–288) renders the same five overlays unconditionally with no pathname gate. Mobile's_layout.tsxnow matches exactly — this diff closes a parity gap (mandatory per the repo's cross-platform rule) rather than opening one. - No new surface for other rubric dimensions. No comments, no
any/type changes, no i18n, nopackages/shared/DTO changes, no security-relevant code in this commit. - Cross-repo dimensions (contract-aligner, orbit-api side of the backward-compat guard): not applicable — this commit touches no contract/shared-type surface, and
orbit-apiisn't checked out in this job.
Findings
None at Critical/High. No concretely-actionable Medium findings either. (Earlier commits in this PR were already reviewed and approved at 6ee90b39 with no findings — not re-flagged here.)
Recommendation: Approve
Small, correctly scoped fix that closes a genuine cross-platform parity gap, verified to have no dead code and no interaction with the separately-fixed re-trigger guard.
…ing policy codes The pending-operation card rendered the server's English capability DisplayName/summary verbatim and raw policy reason codes (confirmation_required) as error text. Confirmation-gated capability names now localize client-side via shared key mapping with English fallback, the summary line is composed locally, known policy reasons map to friendly copy, and the success line is localized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
useBulkLogHabits never called trackCompletion, so habits completed via the Today screen's bulk action counted as zero toward review-moment eligibility. Each bulk item now counts at mutate time, matching the single-log path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #403 — fix: achievements XP row collision + queued completions never counted toward review floor
Recommendation: APPROVE
Summary
Four bundled fixes surfaced during #382 QA: (1) an achievements-header XP/count row text collision at high locale/level values, (2) offline-queued habit completions never counting toward the post-celebration review-moment engagement floor, (3) celebration overlays never mounting on the Today route, and (4) the agent (Astra) confirmation card leaking raw English capability names and raw policy-reason codes in pt-BR sessions. All four land symmetrically across apps/web + apps/mobile (or are legitimately platform-specific), i18n stays in sync, and no Critical/High issue survived adversarial verification.
Findings
- Critical: None
- High: None
- Medium: None
- Low/Info: None
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — the review-floor onMutate fix is legitimately mobile-only (Play Store in-app-review prompt has no web equivalent); the XP-row fix, pending-operation-card localization, and celebration unconditional-mount are mirrored on both platforms |
| i18n-syncer | IN SYNC — 16 new chat.pendingOp.* keys + 2 changed gamification.profileCard.* keys present in both en.json/pt-BR.json, 0 missing, 0 orphaned callsites |
| contract-aligner | N/A — diff doesn't touch packages/shared/src/types/* or endpoints.ts, and orbit-api isn't part of this diff |
| security-reviewer | N/A — orbit-api not touched |
Validation
Could not execute lint/type-check/tests in this review session (no interactive Bash approval available to this review agent). PR body self-reports mobile 930 ✓ / web 2105 ✓ / shared 1384 ✓, lint 0 errors, type-check clean — not independently re-run here.
What's good
apps/mobile/hooks/use-habits.ts: movingtrackCompletionfromonSuccesstoonMutateis the correct fix —onMutatefires exactly once per user action regardless of whether the mutation resolves immediately, gets queued offline, or the queue is flushed later outside the mutation lifecycle. Mutations haveretry: false(apps/mobile/lib/query-client.ts), so there's no double-count risk from retries. Good regression test pinning this againstonMutatedirectly.- The Astra confirmation-card fix centralizes the capability-id → i18n-key and policy-reason → i18n-key mapping in one new shared util (
packages/shared/src/utils/agent-pending-operation.ts), consumed identically by both platforms — unmapped ids/reasons fail safe (fallback to serverdisplayName/ generic error) instead of leaking raw codes. apps/mobile/app/_layout.tsx: removingshowSharedCelebrations = pathname !== '/'fixes a real footgun — it silently blocked every post-celebration prompt (including the new #382 review moment) on the one screen where completions actually happen, and now correctly matches web's existing unconditional mount.
Deferred
- Dimension 8 (DESIGN.md/AI-slop): checked — the only UI-visual changes are
gap/flexShrinkspacing hardening on an existing row; nothing new to flag against the bans. - Dimensions 13 (backend hard rules) / 14 (FEATURES.md parity): N/A —
orbit-apiuntouched; this is a bugfix/polish PR with no feature-inventory-level change. - The new
LOCALIZED_CAPABILITY_IDS/LOCALIZED_POLICY_REASONSstring literals inpackages/shared/src/utils/agent-pending-operation.tsmust match the realcapabilityId/policyReasonvalues orbit-api sends; that repo isn't checked out here so it can't be verified against source of truth. I investigated whetherhabits.delete(plural) might mismatch the real single-habit-delete capability id (pre-existing test fixtures elsewhere in this repo use singularhabit.delete), but this repo's ownpackages/shared/src/types/config.tsplan-gating keys already establishhabits.*plural as the in-repo domain-prefix convention, and the threehabits.*entries in the new set share one consistent prefix — stronger evidence than the ambiguous placeholder fixtures — so I didn't carry this forward as a finding. A mismatch would degrade gracefully (English fallback) rather than break anything; a quick pt-BR sanity check on the "Delete habit" card is a cheap, non-blocking follow-up.
Recommendation
Nothing blocking. Optional follow-up: sanity-check the new capability-id/policy-reason string literals against the real orbit-api values in a manual pt-BR run, since there's no shared-type enforcement tying these hardcoded strings to the backend contract.
🤖 Generated with Claude Code /pr-review
There was a problem hiding this comment.
/pr-review — PR #403 (incremental)
Scope of this review: diff since my prior approvals at 6ee90b39 and a41d4088. Two new commits: 363820a2 (localize agent confirmation cards + stop leaking policy codes) and 6e73d1e9 (count bulk completions toward the review floor).
Recommendation: REQUEST CHANGES — one High finding.
High
Capability-id mapping likely uses the wrong naming convention — the localization fix may be a silent no-op in production
packages/shared/src/utils/agent-pending-operation.ts:1-14 — LOCALIZED_CAPABILITY_IDS uses plural-form ids for the simple CRUD capabilities: 'habits.delete', 'goals.delete', 'tags.delete', 'notifications.delete', 'user-facts.delete', 'subscriptions.manage', 'api-keys.manage', 'account.manage', 'sync.write', 'calendar.sync.manage'.
Every pre-existing capabilityId fixture in this repo — untouched by this PR — uses the singular form for the same field:
packages/shared/src/__tests__/types.test.ts:997—agentCapabilitySchemafixture:id: 'habit.read'(note: same fixture uses pluraldomain/scope, singularid— the two are deliberately distinct conventions)packages/shared/src/__tests__/types.test.ts:1055—capabilityId: 'habit.delete'packages/shared/src/__tests__/types.test.ts:1023—relatedCapabilityIds: ['habit.read']apps/web/__tests__/components/chat/pending-operation-card.test.tsx:16,apps/web/__tests__/components/chat/message-bubble.test.tsx:148,apps/mobile/__tests__/components/chat/pending-operation-card.test.tsx:58— all default tocapabilityId: 'habit.delete'
packages/shared/CLAUDE.md states these Zod schemas are "the contract with orbit-api" and that test factories are meant to be "realistic and minimal" — i.e. the singular-form fixtures are the closest in-repo proxy for the real API convention. If orbit-api's actual capability ids are singular (habit.delete, goal.delete, tag.delete, ...), none of this PR's ~10 plural CRUD entries will ever match a real capabilityId sent by the server.
Risk: getAgentCapabilityLabelKey() returns null for every real confirmation-gated operation, and the card silently keeps falling back to pendingOperation.displayName — the server's raw English string — which is exactly the bug this commit's message says it fixes. The new tests don't catch this because they exercise the mapping against capabilityId: 'habits.bulk.write', a value the PR itself invented and added to the same set, rather than against the repo's existing singular convention.
Fix: confirm the real orbit-api capabilityId strings for these capabilities and correct LOCALIZED_CAPABILITY_IDS to match (most likely singular for the CRUD entries — habit.delete, goal.delete, tag.delete, notification.delete, user-fact.delete, subscription.manage, api-key.manage, account.manage — keeping plural only where genuinely bulk-scoped, e.g. habits.bulk.write/habits.bulk.delete). Not verifiable in this CI job — orbit-api isn't checked out here — but a one-grep check against the sibling repo before merge.
Medium (non-blocking)
New shared util has no direct unit test
packages/shared/src/utils/agent-pending-operation.ts (new file, 2 exported functions) is only exercised indirectly through the mobile/web component test suites. Consider adding packages/shared/src/__tests__/agent-pending-operation.test.ts covering both functions' mapped/unmapped branches directly, so coverage doesn't silently erode if a future component change stops exercising every branch.
What's good
6e73d1e9(bulk-log review-floor counting) is a clean, correct fix that mirrors the already-revieweduseLogHabit.onMutatepattern exactly, with a matching regression test. No issues.363820a2's architecture is otherwise sound: one shared util consumed identically by both platforms, JSDoc on both exports, noany/console.log/comment-policy violations, i18n keys added to bothen.jsonandpt-BR.jsonin the same diff with genuine translations, andresolveExecutionErrornow maps unknown policy reasons to a safe generic error instead of leaking raw server codes.- Parity and i18n verified in sync across both platforms for this diff.
Not verifiable in CI
- contract-aligner (orbit-api capability-id convention) — sibling repo not checked out in this job; this is exactly what the High finding above depends on.
- No
orbit-apifiles changed in this diff, so the backward-compat guard and security-reviewer dimensions are N/A here.
| const LOCALIZED_CAPABILITY_IDS = new Set([ | ||
| 'habits.delete', | ||
| 'habits.bulk.write', | ||
| 'habits.bulk.delete', | ||
| 'goals.delete', | ||
| 'tags.delete', | ||
| 'notifications.delete', | ||
| 'calendar.sync.manage', | ||
| 'user-facts.delete', | ||
| 'subscriptions.manage', | ||
| 'api-keys.manage', | ||
| 'sync.write', | ||
| 'account.manage', | ||
| ]) |
There was a problem hiding this comment.
High: these ids are plural (habits.delete, goals.delete, tags.delete, notifications.delete, user-facts.delete, subscriptions.manage, api-keys.manage, account.manage, sync.write, calendar.sync.manage), but every pre-existing capabilityId fixture in this repo — untouched by this PR — uses the singular form for the same field: types.test.ts:997 (id: 'habit.read'), types.test.ts:1055 (capabilityId: 'habit.delete'), and the default fixtures in both platforms' pending-operation-card.test.tsx/message-bubble.test.tsx.
If orbit-api's real capability ids are singular, getAgentCapabilityLabelKey() returns null for every one of these in production, and the card silently keeps showing the server's raw English displayName — the exact bug this commit claims to fix. Please confirm the real orbit-api ids (singular for the CRUD ones, most likely) before merge.
Covers the mapped/unmapped branches of getAgentCapabilityLabelKey and getAgentPolicyReasonKey directly (review Medium + Sonar new-code gate). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re the High finding (capability-id naming): verified against the authoritative source — the ids in public const string HabitsDelete = "habits.delete";
public const string HabitsBulkWrite = "habits.bulk.write";
public const string GoalsDelete = "goals.delete";
public const string TagsDelete = "tags.delete";
public const string NotificationsDelete = "notifications.delete";
public const string CalendarSyncManage = "calendar.sync.manage";
public const string UserFactsDelete = "user-facts.delete";
public const string SubscriptionsManage = "subscriptions.manage";
public const string ApiKeysManage = "api-keys.manage";
public const string SyncWrite = "sync.write";
public const string AccountManage = "account.manage";The singular The Medium (direct unit test for the shared util) is addressed in ce01e01, which also clears the Sonar new-code gate (the 20% figure was 2 uncovered lines out of 4 in that util — now covered directly). |
There was a problem hiding this comment.
/pr-review — PR #403 (incremental)
Scope of this review: diff since my last CHANGES_REQUESTED review (which flagged one High + one Medium finding). Since then: one new commit, ce01e018 "test(shared): direct unit coverage for agent-pending-operation util", plus a merge of main.
Prior findings — resolved
- High (capability-id naming convention) — the PR author responded with the authoritative source: orbit-api's
src/Orbit.Domain/Models/AgentContracts.cs(AgentCapabilityIds) defines these ids in the plural form used byLOCALIZED_CAPABILITY_IDS(HabitsDelete = "habits.delete",GoalsDelete = "goals.delete", etc.) — the singular-form fixtures elsewhere in this repo are stale/invented values, not the real contract. This was empirically confirmed in a live QA pass against the production API (localized "Alterações de hábitos em lote" rendered correctly). Resolved. - Medium (no direct unit test for the shared util) —
ce01e018addspackages/shared/src/__tests__/agent-pending-operation.test.ts, covering bothgetAgentCapabilityLabelKeyandgetAgentPolicyReasonKeyacross their mapped and unmapped/null/empty branches directly. Matches the util's actual behavior (verified by readingpackages/shared/src/utils/agent-pending-operation.ts). Resolved, and clears the Sonar new-code coverage gate cited in the author's follow-up comment.
New findings
None. No Critical/High/actionable-Medium issues in this incremental diff.
Not verifiable in CI
- Cross-repo confirmation of the capability-id convention rests on the author's cited orbit-api source, which isn't checked out in this job — noted as author-provided evidence, not independently re-verified here.
Recommendation: Approve
All findings from the prior review round are resolved — one via authoritative clarification, one via an added test. Nothing new to flag.
|
1 participant
Two fixes surfaced during #382 QA:
1. Achievements header stats collision (both platforms)
At high levels/locales the two stat strings ("48.409 XP no total" + "14 / 39 conquistas desbloqueadas") overflowed the row and rendered jammed together. Fixed by shortening the copy in both locales (
{total} XP/{count}/{total} conquistas— the full current/next XP line already sits directly above) and hardening the row (gap+flexShrinkon mobile,gapon web) so no value length can collide again.2. Queued completions never counted toward the review-moment floor (mobile)
useLogHabit.onSuccessearly-returns for offline-queued results beforetrackCompletion, so any completion that went through the offline queue silently never counted toward review-moment eligibility (and the queue flush bypasses the mutation callbacks entirely). Moved the floor tracking toonMutate: an engagement counter tolerates a rare overcount on a failed request, but must not systematically miss queued completions. Regression test added.Found live: a momentary network hiccup queued a completion, today's active-day never counted, and the post-celebration review sheet (#382) silently skipped an eligible level-up moment.
Validation: mobile 930 ✓ / web 2105 ✓ / shared 1384 ✓, lint 0 errors, type-check clean.
🤖 Generated with Claude Code
3. Celebration overlays never mounted on the Today route (mobile)
showSharedCelebrations = pathname !== '/'(added 2026-04-05 alongside the properacknowledgedLevelre-trigger fix) excluded StreakCelebration, AllDoneCelebration, WelcomeBackToast, AchievementToast, and LevelUpOverlay from the Today tab — the screen where completions actually happen. Celebrations queued there could not play until the user navigated away, and the stalled queue also suppressed every post-celebration prompt (milestone share, referral, and the new #382 review moment) while on Today. Web mounts celebrations on all routes; mobile now matches. The infinite re-trigger bug that gate papered over was already properly fixed byacknowledgedLevelin the same April commit.4. Agent confirmation cards showed English + raw policy codes (both platforms)
The pending-operation card rendered the server's English capability
DisplayName/summaryverbatim in pt-BR sessions, leaked raw policy reason codes (confirmation_required) as error text, and echoed the English summary as the success line. Confirmation-gated capability names (12) now localize client-side via a shared key mapping (getAgentCapabilityLabelKey, hyphen-folded ids, English fallback for unmapped capabilities), the summary is composed locally, known policy reasons map to friendly copy (getAgentPolicyReasonKey, generic error fallback — codes never render), and the success line is localized. en + pt-BR, tests on both platforms.