Error sweep: APNs token pruning, honest severities, unauth redirects, baseball RLS-denial parity#798
Conversation
…auth redirects, baseball RLS-denial parity From the three-source error triage (Vercel runtime + admin_events + Sentry), whose entire surface reduced to three root causes: - APNs 410 Unregistered / BadDeviceToken responses now prune the device_tokens row in the shared send path (every push caller benefits) — ends the retry spam running since Jun 11 (Fixes JAVASCRIPT-NEXTJS-BQ) - Expected empty-state soft failures (engine_no_recent_rounds, engine_session_expired) carry typed codes and log at info/warning with skipSentry across BOTH consumers (observeActionSoftFailure and post-round-trigger); unrecognized codes still alert at error severity (Fixes JAVASCRIPT-NEXTJS-BP) - BaseballUnauthorizedError no longer leaks as a raw 500: shared redirectOnUnauthorized helper (golf layout idiom, returnTo preserved) applied across all 11 affected pages; independent sweep confirms zero remaining instances of the bug class (Fixes JAVASCRIPT-NEXTJS-BN) Baseball error-tracking parity with golf: - RLS-denial capture wired centrally into withBaseballAction (all 60 action files covered for free) + explicit sites where actions swallow PG errors into result envelopes; lifting logSetResult rethrow corrected so the wrapper fallback cannot double-capture - 3 route-group error boundaries added; dead admin_client_errors realtime subscription removed - Audit verdict recorded: baseball is AHEAD of golf on action instrumentation (60/60 wrapped vs golf CRM cluster 9 files/~55 actions uninstrumented — follow-up) Every fix adversarially verified (repairs re-verified; one load-bearing test proven by revert-experiment). Gates: tsc 0, eslint 0, unit 478 files / 4860 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
WalkthroughThe pull request adds targeted unauthorized redirects and route error boundaries, RLS-denial capture across Supabase actions, stable engine-failure telemetry classification, removal of an inactive realtime subscription, push-token pruning events, and regression tests. ChangesUnauthorized routing and dashboard errors
RLS-denial observability
Engine failure observability
Push-token pruning telemetry
Repository change-file filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR addresses three production error sources: dead APNs tokens accumulating retries, routine engine outcomes (no recent rounds, background session expiry) logged as errors, and
Confidence Score: 4/5Safe to merge. All three error sources are correctly addressed, the RLS double-fire regression is pinned by a dedicated test, and the redirect helper is well-covered. Two pages use inline guard logic instead of the new helper, and several announcement/game action callbacks omit user attribution from RLS capture calls — both are minor gaps with no user-facing impact. The core changes are architecturally clean and well-tested (4,860 passing, regression test for double-fire, severity-classification tests for both consumers). The two non-blocking findings — inconsistent use of the new helper in scout-packets/preview pages, and missing userId in four maybeCaptureRlsDenial calls inside withBaseballAction callbacks — reduce future debuggability but don't affect correctness or user data. src/app/baseball/actions/announcements.ts and src/app/baseball/actions/games.ts (missing userId in RLS capture calls); src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx and the scout-packet preview page (inline guard pattern vs. the new redirectOnUnauthorized helper). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[withBaseballAction catch block] --> B{instanceof BaseballUnauthorizedError?}
B -- yes --> C[rethrow to Server Component page]
C --> D{redirectOnUnauthorized or inline try-catch}
D -- is unauthorized --> E[redirect /baseball/login?returnTo=]
D -- is other error --> F[rethrow to error.tsx]
B -- no --> G[maybeCaptureRlsDenial table=featureArea verb=rpc]
G --> H{isRlsDenial code=42501?}
H -- yes --> I[logServerEvent RLS denial]
H -- no --> J[no-op]
G --> K[logServerException normalized error]
K --> L[rethrow sanitized Error]
subgraph Action body override
M[Action body detects RLS] --> N[maybeCaptureRlsDenial precise table]
N --> O[throw new BaseballActionError]
O --> B
end
subgraph Severity classification
P[triggerPlayerInsightsAfterRound result] --> Q{code?}
Q -- engine_no_recent_rounds --> R[logServerEvent info + skipSentry]
Q -- engine_session_expired --> S[logServerError warning + skipSentry]
Q -- unrecognized --> T[logServerError error no skipSentry]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[withBaseballAction catch block] --> B{instanceof BaseballUnauthorizedError?}
B -- yes --> C[rethrow to Server Component page]
C --> D{redirectOnUnauthorized or inline try-catch}
D -- is unauthorized --> E[redirect /baseball/login?returnTo=]
D -- is other error --> F[rethrow to error.tsx]
B -- no --> G[maybeCaptureRlsDenial table=featureArea verb=rpc]
G --> H{isRlsDenial code=42501?}
H -- yes --> I[logServerEvent RLS denial]
H -- no --> J[no-op]
G --> K[logServerException normalized error]
K --> L[rethrow sanitized Error]
subgraph Action body override
M[Action body detects RLS] --> N[maybeCaptureRlsDenial precise table]
N --> O[throw new BaseballActionError]
O --> B
end
subgraph Severity classification
P[triggerPlayerInsightsAfterRound result] --> Q{code?}
Q -- engine_no_recent_rounds --> R[logServerEvent info + skipSentry]
Q -- engine_session_expired --> S[logServerError warning + skipSentry]
Q -- unrecognized --> T[logServerError error no skipSentry]
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/app/baseball/actions/announcements.ts:155-163
These three `withBaseballAction` callbacks (`createAnnouncementAction`, `acknowledgeAnnouncementImpl`, `deleteAnnouncementAction`) all receive `ctx` and have `ctx.user.id` in scope, but omit `userId` from `maybeCaptureRlsDenial`. The same oversight appears in `saveFullBoxScoreAction` in `games.ts`. Without user attribution the RLS denial events in the admin feed can't be correlated to a specific coach or session, making it harder to distinguish a user-specific policy misconfiguration from a schema-wide grant issue. Compare `watchlist.ts` `addToWatchlist` and `lifting-v11.ts` `logSetResult`, which both pass `userId: ctx.user.id`.
```suggestion
if (annError || !announcement) {
maybeCaptureRlsDenial(annError, {
table: 'baseball_announcements',
verb: 'insert',
action: 'createAnnouncement',
feature: 'baseball_announcements',
sport: 'baseball',
userId: ctx.user.id,
});
return { success: false, error: 'Failed to create announcement' };
```
### Issue 2 of 2
src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx:47-55
`scout-packets/page.tsx` and `scout-packet/preview/page.tsx` (the two pages not refactored in this PR) use inline try/catch while the other 9 pages fixed here all use the `redirectOnUnauthorized` helper. The inline code is functionally correct today, but if the login redirect target or the unauthorized predicate ever changes, these two pages are silent drift points. `redirectOnUnauthorized` was specifically designed to avoid this — using it here would be a one-liner swap.
```suggestion
const roster = await redirectOnUnauthorized(
() => getScoutPacketRoster(),
(error) => error instanceof BaseballUnauthorizedError,
'/baseball/dashboard/scout-packets',
);
```
Reviews (1): Last reviewed commit: "Error sweep: prune dead APNs tokens, hon..." | Re-trigger Greptile |
PR Summary by QodoError sweep: APNs token pruning, soft-failure severities, unauth redirects, RLS parity
AI Description
Diagram
High-Level Assessment
Files changed (44)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
98 rules 1. Misplaced games.ts server action
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/coachhelm/v2/post-round-trigger.ts (1)
60-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sanitizeFailureReasonstill parses the message string forFailureCodeeven though a stablecodeis now on the same result.
classifyEngineFailureSeverity(lines 71-78) correctly uses the newcodefield, butsanitizeFailureReason(lines 80-89), called right below at line 153 with the samereasonstring, still relies on brittle substring matching (lower.includes('no completed rounds'),lower.includes('session') && lower.includes('expired')) to derive'engine_no_recent_rounds'/'engine_session_expired'for thecoachhelm_failure_reasonDB column. Sinceresult.codealready carries these exact literal values, a future edit to the user-facing error message (the very drift this PR'scodefield was introduced to survive, per the doc comments onEXPECTED_SOFT_FAILURE_CODES) would silently desync the DB-persisted failure reason from the telemetry classification.♻️ Proposed fix
-function sanitizeFailureReason(reason: string): FailureCode { +function sanitizeFailureReason(reason: string, code?: string | null): FailureCode { + if (code === 'engine_session_expired' || code === 'engine_no_recent_rounds') return code; const lower = reason.toLowerCase(); if (lower.includes('timeout') || lower.includes('timed out')) return 'engine_timeout'; if (lower.includes('session') && lower.includes('expired')) return 'engine_session_expired'; if (lower.includes('membership')) return 'engine_membership_missing'; if (lower.includes('disabled')) return 'engine_disabled'; if (lower.includes('no completed rounds')) return 'engine_no_recent_rounds'; if (lower.includes('generator failure') || lower.includes('tier-1')) return 'engine_generator_failure'; return 'engine_error'; }And thread
result.codethrough at the call site (line ~153):- coachhelm_failure_reason: sanitizeFailureReason(reason), + coachhelm_failure_reason: sanitizeFailureReason(reason, result.code ?? null),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/coachhelm/v2/post-round-trigger.ts` around lines 60 - 89, Update sanitizeFailureReason to accept and prioritize the stable result.code, mapping recognized codes directly to the corresponding FailureCode instead of parsing user-facing message text. At its call site in postRoundTrigger, pass result.code alongside reason, while retaining message-based fallback only when code is null or unrecognized.src/app/golf/actions/insights.ts (1)
3837-3860: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReserve
engine_session_expiredfor real session/auth failures
src/app/golf/actions/insights.ts:3838-3859drops the thrown error and returnscode: 'engine_session_expired'for everyanalyzePlayerfailure. That code is treated as an expected soft failure insrc/lib/admin/observe-action-result.ts:28-45andsrc/lib/coachhelm/v2/post-round-trigger.ts:71-84, so engine bugs and timeouts get downgraded to handled noise and skip Sentry. Bind the error here and only emit that code when the failure is actually session-related; otherwise return the raw failure without the code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/golf/actions/insights.ts` around lines 3837 - 3860, Update the catch block around coachHelmIntelligence.analyzePlayer to bind the thrown error and classify it before returning. Emit code 'engine_session_expired' only for genuine session/auth failures; for all other errors, preserve and return the original failure details without that code so observability can report engine bugs and timeouts. Use the analyzePlayer catch path and existing error/session classification utilities or conventions to implement the distinction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/app/baseball/`(dashboard)/dashboard/decision-room/__tests__/page.test.tsx:
- Around line 43-54: Extract the duplicated FakeBaseballUnauthorizedError class
into a shared test helper module, preserving its status, default message, and
name. Update each affected page test to import the helper and have the
vi.mock('`@/lib/baseball/with-baseball-action`', ...) factory reference the
imported class, removing the local vi.hoisted boilerplate.
- Around line 27-31: Replace the inline Supabase mock in the decision-room page
test with the shared createFakeSupabase fixture, configuring auth.getUser as
needed while preserving the existing mock behavior. In the successful render
test, replace the weak toBeTruthy assertion with a specific assertion on the
rendered output or the props passed to the returned element.
In
`@src/app/baseball/`(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx:
- Around line 37-57: Replace the local BaseballUnauthorizedError catch and
manual login redirect in the page’s getScoutPacketPreview handling with the
shared redirectOnUnauthorized helper, matching its usage elsewhere in the
baseball flow while preserving propagation of unrelated errors.
In `@src/app/baseball/`(dashboard)/dashboard/scout-packets/page.tsx:
- Around line 39-55: Replace the local BaseballUnauthorizedError catch and
redirect logic around getScoutPacketRoster with the shared
redirectOnUnauthorized guard, preserving propagation of non-unauthorized errors
and using the centralized predicate and login URL.
In
`@src/app/baseball/`(dashboard)/dashboard/settings/audit/__tests__/page.test.tsx:
- Around line 30-34: The test uses an ad-hoc Supabase server mock instead of the
shared test client. Update the test setup around the module-level mock to use
the canonical Supabase test client from src/test/, preserving the existing
getUser behavior through that shared client and removing the duplicated
vi.mock('`@/lib/supabase/server`', ...) implementation.
In
`@src/app/baseball/`(dashboard)/dashboard/settings/program/__tests__/page.test.tsx:
- Around line 27-31: The test currently defines a local Supabase server mock
instead of using the shared test client. Replace the
vi.mock('`@/lib/supabase/server`', ...) setup in page.test.tsx with the shared
Supabase test client from src/test/, and configure its auth.getUser behavior
through that client.
In `@src/app/baseball/actions/__tests__/lifting-v11-rls-denial.test.ts`:
- Around line 156-167: Strengthen the non-RLS error test for logSetResult by
asserting the arguments passed to logServerException, not only its call count.
Verify the logged exception or message preserves the original duplicate-key
error details, including code 23505 and its message, while still confirming no
RLS event is emitted.
In `@src/app/baseball/actions/announcements.ts`:
- Around line 156-162: Update all four maybeCaptureRlsDenial calls in
createAnnouncementAction, getAnnouncementsWithMetaImpl,
acknowledgeAnnouncementImpl, and deleteAnnouncementAction to include the
relevant userId: use ctx.user.id for action handlers, user.id in
acknowledgeAnnouncementImpl, and pass the existing _userId parameter through
getAnnouncementsWithMetaImpl. Rename that parameter if needed to reflect its
use.
In `@src/app/baseball/actions/games.ts`:
- Around line 933-942: Update saveFullBoxScoreAction’s error handling to pass
_ctx.user.id as the userId field in the maybeCaptureRlsDenial call, matching the
attribution pattern used by announcements.ts; retain the existing table, verb,
action, and feature metadata.
In `@src/app/baseball/actions/lifting-v11.ts`:
- Around line 2032-2048: In logSetResult, preserve diagnostics for non-RLS
database failures by logging the original error's full Postgrest metadata (code,
message, details, and hint) through the existing server-error logging mechanism
before throwing the sanitized BaseballActionError. Keep maybeCaptureRlsDenial
and the generic rethrow unchanged for RLS handling, and mirror sendMessage's
logging pattern in messages.ts.
In `@src/lib/baseball/redirect-on-unauthorized.ts`:
- Around line 33-41: Validate the decoded `returnTo` value in the login page
before navigating with `router.push()`: allow only internal paths beginning with
`/baseball/`, and fall back to `/baseball/dashboard/command-center` for missing,
malformed, or external values. Update the redirect flow associated with
`redirectOnUnauthorized` to use this allowlisted destination.
---
Outside diff comments:
In `@src/app/golf/actions/insights.ts`:
- Around line 3837-3860: Update the catch block around
coachHelmIntelligence.analyzePlayer to bind the thrown error and classify it
before returning. Emit code 'engine_session_expired' only for genuine
session/auth failures; for all other errors, preserve and return the original
failure details without that code so observability can report engine bugs and
timeouts. Use the analyzePlayer catch path and existing error/session
classification utilities or conventions to implement the distinction.
In `@src/lib/coachhelm/v2/post-round-trigger.ts`:
- Around line 60-89: Update sanitizeFailureReason to accept and prioritize the
stable result.code, mapping recognized codes directly to the corresponding
FailureCode instead of parsing user-facing message text. At its call site in
postRoundTrigger, pass result.code alongside reason, while retaining
message-based fallback only when code is null or unrecognized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26ac96a8-ae7f-4f8f-aed2-bed1d012c047
📒 Files selected for processing (44)
src/app/actions/messages.tssrc/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/decision-room/page.tsxsrc/app/baseball/(dashboard)/dashboard/operations/error.tsxsrc/app/baseball/(dashboard)/dashboard/performance/builder/error.tsxsrc/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsxsrc/app/baseball/(dashboard)/dashboard/scout-packets/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/scout-packets/page.tsxsrc/app/baseball/(dashboard)/dashboard/scouting/error.tsxsrc/app/baseball/(dashboard)/dashboard/settings/audit/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/audit/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/imports/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/imports/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/integrations/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/integrations/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/permissions/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/permissions/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/program/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/program/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/roles/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/roles/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/season/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/season/page.tsxsrc/app/baseball/(dashboard)/dashboard/settings/teams/__tests__/page.test.tsxsrc/app/baseball/(dashboard)/dashboard/settings/teams/page.tsxsrc/app/baseball/actions/__tests__/lifting-v11-rls-denial.test.tssrc/app/baseball/actions/announcements.tssrc/app/baseball/actions/games.tssrc/app/baseball/actions/lifting-v11.tssrc/app/baseball/actions/watchlist.tssrc/app/golf/actions/insights.tssrc/hooks/useAdminRealtime.tssrc/lib/admin/__tests__/observe-action-result.test.tssrc/lib/admin/observe-action-result.tssrc/lib/baseball/__tests__/redirect-on-unauthorized.test.tssrc/lib/baseball/__tests__/with-baseball-action-observability.test.tssrc/lib/baseball/redirect-on-unauthorized.tssrc/lib/baseball/with-baseball-action.tssrc/lib/coachhelm/v2/post-round-trigger.tssrc/lib/coachhelm/v2/trigger-insights-bridge.tssrc/lib/notifications/push.tssrc/test/coachhelm/v2/post-round-trigger.test.tssrc/test/lib/notifications/push.test.ts
| vi.mock('@/lib/supabase/server', () => ({ | ||
| createClient: vi.fn(async () => ({ | ||
| auth: { getUser: mocks.getUser }, | ||
| })), | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for an existing shared Supabase test client convention.
fd . src/test -e ts -e tsx
rg -n "createClient" src/test -A3 -B3
rg -n "supabase" src/test -ilRepository: njrini99-code/helmv3
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/test/fixtures/fake-supabase.ts ---'
cat -n src/test/fixtures/fake-supabase.ts | sed -n '1,220p'
echo
echo '--- src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx ---'
cat -n 'src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx' | sed -n '1,220p'
echo
echo '--- nearby shared-test-client usage in baseball dashboard tests ---'
rg -n "fake-supabase|createFakeSupabase|createClient: vi\.fn\(\s*async|toBeTruthy\(\)" src/app/baseball src/test -g '*.test.ts' -g '*.test.tsx' -A2 -B2Repository: njrini99-code/helmv3
Length of output: 50377
Use the shared Supabase fixture here
- Replace the inline
createClientmock atsrc/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx:27-31withcreateFakeSupabasefromsrc/test/fixtures/fake-supabase.ts:1-20; this page test only needsauth.getUser(), and the shared fixture is the repo convention. - Tighten the success case at
src/app/baseball/(dashboard)/dashboard/decision-room/__tests__/page.test.tsx:84-88;toBeTruthy()on the returned element is too weak—assert the rendered output or passed props instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(dashboard)/dashboard/decision-room/__tests__/page.test.tsx
around lines 27 - 31, Replace the inline Supabase mock in the decision-room page
test with the shared createFakeSupabase fixture, configuring auth.getUser as
needed while preserving the existing mock behavior. In the successful render
test, replace the weak toBeTruthy assertion with a specific assertion on the
rendered output or the props passed to the returned element.
Source: Path instructions
| const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({ | ||
| FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error { | ||
| readonly status = 401; | ||
| constructor(message = 'You must be signed in.') { | ||
| super(message); | ||
| this.name = 'BaseballUnauthorizedError'; | ||
| } | ||
| }, | ||
| })); | ||
| vi.mock('@/lib/baseball/with-baseball-action', () => ({ | ||
| BaseballUnauthorizedError: FakeBaseballUnauthorizedError, | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the duplicated FakeBaseballUnauthorizedError boilerplate into a shared test helper.
This exact ~12-line class + vi.mock('@/lib/baseball/with-baseball-action', ...) block is copy-pasted verbatim across every page test in this cohort (imports, integrations, permissions, roles, season, teams). Since vi.mock factories can safely reference values imported from a separate module (regular ES imports are resolved before module evaluation, unlike local const which hits the hoisting TDZ), this can be centralized.
♻️ Proposed shared helper
// src/test/baseball/fake-unauthorized-error.ts
export class FakeBaseballUnauthorizedError extends Error {
readonly status = 401;
constructor(message = 'You must be signed in.') {
super(message);
this.name = 'BaseballUnauthorizedError';
}
}+import { FakeBaseballUnauthorizedError } from '`@/test/baseball/fake-unauthorized-error`';
-const { FakeBaseballUnauthorizedError } = vi.hoisted(() => ({
- FakeBaseballUnauthorizedError: class FakeBaseballUnauthorizedError extends Error {
- readonly status = 401;
- constructor(message = 'You must be signed in.') {
- super(message);
- this.name = 'BaseballUnauthorizedError';
- }
- },
-}));
vi.mock('`@/lib/baseball/with-baseball-action`', () => ({
BaseballUnauthorizedError: FakeBaseballUnauthorizedError,
}));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(dashboard)/dashboard/decision-room/__tests__/page.test.tsx
around lines 43 - 54, Extract the duplicated FakeBaseballUnauthorizedError class
into a shared test helper module, preserving its status, default message, and
name. Update each affected page test to import the helper and have the
vi.mock('`@/lib/baseball/with-baseball-action`', ...) factory reference the
imported class, removing the local vi.hoisted boilerplate.
| // | ||
| // getScoutPacketPreview independently re-resolves auth (withBaseballAction), | ||
| // so a session that expires in the narrow window between the context check | ||
| // above and this call throws BaseballUnauthorizedError here. Left uncaught, | ||
| // that raw-throws through this Server Component's render straight to | ||
| // error.tsx and the error tracker (Sentry/Vercel) instead of the honest | ||
| // "please sign in again" redirect — the same class of bug fixed on the | ||
| // baseball announcements page. Redirect on that specific case; any other | ||
| // thrown error (e.g. a real capability failure for a signed-in coach) is a | ||
| // genuine failure and should keep propagating to error.tsx. | ||
| let model: Awaited<ReturnType<typeof getScoutPacketPreview>>; | ||
| try { | ||
| model = await getScoutPacketPreview(id); | ||
| } catch (error) { | ||
| if (error instanceof BaseballUnauthorizedError) { | ||
| redirect( | ||
| `/baseball/login?returnTo=${encodeURIComponent(`/baseball/dashboard/players/${id}/scout-packet/preview`)}`, | ||
| ); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target page and shared helper.
git ls-files 'src/app/baseball/**/preview/page.tsx' 'src/lib/baseball/**' | sed -n '1,200p'
printf '\n--- helper references ---\n'
rg -n "redirectOnUnauthorized|BaseballUnauthorizedError|getScoutPacketPreview|withBaseballAction" src/app/baseball src/lib/baseball -g '!**/node_modules/**' | sed -n '1,240p'Repository: njrini99-code/helmv3
Length of output: 32977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the target page and the helper implementation/call sites.
sed -n '1,220p' src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx
printf '\n--- helper ---\n'
sed -n '1,220p' src/lib/baseball/redirect-on-unauthorized.ts
printf '\n--- similar pages ---\n'
rg -n "redirectOnUnauthorized\(" src/app/baseball/(dashboard)/dashboard -g 'page.tsx' -g '!**/node_modules/**' | sed -n '1,240p'Repository: njrini99-code/helmv3
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target file ---'
sed -n '1,220p' 'src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx'
echo
echo '--- helper file candidates ---'
git ls-files 'src/lib/baseball/*redirect*' 'src/lib/baseball/**/redirect*' | sed -n '1,120p'
echo
echo '--- helper definition/calls ---'
rg -n "redirectOnUnauthorized|BaseballUnauthorizedError" src/lib/baseball src/app/baseball -g '!**/node_modules/**' | sed -n '1,240p'Repository: njrini99-code/helmv3
Length of output: 31817
Use redirectOnUnauthorized here. src/app/baseball/(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx:37-57 duplicates the same BaseballUnauthorizedError → login redirect path already used elsewhere in this baseball flow; switch to the shared helper to keep redirect behavior consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/baseball/`(dashboard)/dashboard/players/[id]/scout-packet/preview/page.tsx
around lines 37 - 57, Replace the local BaseballUnauthorizedError catch and
manual login redirect in the page’s getScoutPacketPreview handling with the
shared redirectOnUnauthorized helper, matching its usage elsewhere in the
baseball flow while preserving propagation of unrelated errors.
| // getScoutPacketRoster independently re-resolves auth (withBaseballAction), | ||
| // so a session that expires in the narrow window between the checks above | ||
| // and this call throws BaseballUnauthorizedError here. Left uncaught, that | ||
| // raw-throws through this Server Component's render straight to error.tsx | ||
| // and the error tracker (Sentry/Vercel) instead of the honest "please sign | ||
| // in again" redirect — the same class of bug fixed on the baseball | ||
| // announcements page. Redirect on that specific case; any other thrown | ||
| // error (a real failure for a signed-in coach) keeps propagating to error.tsx. | ||
| let roster: Awaited<ReturnType<typeof getScoutPacketRoster>>; | ||
| try { | ||
| roster = await getScoutPacketRoster(); | ||
| } catch (error) { | ||
| if (error instanceof BaseballUnauthorizedError) { | ||
| redirect('/baseball/login?returnTo=/baseball/dashboard/scout-packets'); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file ==\n'
sed -n '1,220p' 'src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx'
printf '\n== helper implementation ==\n'
rg -n "redirectOnUnauthorized" src/app src/lib | sed -n '1,120p'
printf '\n== likely related baseball pages ==\n'
rg -n "BaseballUnauthorizedError|redirectOnUnauthorized|returnTo=/baseball/dashboard/scout-packets|returnTo=/baseball/dashboard" src/app/baseball src/lib | sed -n '1,200p'Repository: njrini99-code/helmv3
Length of output: 34989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' 'src/lib/baseball/redirect-on-unauthorized.ts'
printf '\n== tests ==\n'
sed -n '1,220p' 'src/lib/baseball/__tests__/redirect-on-unauthorized.test.ts'Repository: njrini99-code/helmv3
Length of output: 5260
Use redirectOnUnauthorized here. src/app/baseball/(dashboard)/dashboard/scout-packets/page.tsx:39-55 duplicates the shared unauthorized redirect guard already used on the other baseball Server Components, so the predicate and login URL stay centralized in one place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/`(dashboard)/dashboard/scout-packets/page.tsx around lines
39 - 55, Replace the local BaseballUnauthorizedError catch and redirect logic
around getScoutPacketRoster with the shared redirectOnUnauthorized guard,
preserving propagation of non-unauthorized errors and using the centralized
predicate and login URL.
| vi.mock('@/lib/supabase/server', () => ({ | ||
| createClient: vi.fn(async () => ({ | ||
| auth: { getUser: mocks.getUser }, | ||
| })), | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Inline Supabase mock instead of the shared test client.
Path instructions for test files call for mocking Supabase via the shared test client in src/test/, not an ad-hoc vi.mock('@/lib/supabase/server', ...). This works today but duplicates the mock shape and will drift from the canonical test client as the real createClient() contract evolves.
As per path instructions, "Mock Supabase via the test client in src/test/."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/baseball/`(dashboard)/dashboard/settings/audit/__tests__/page.test.tsx
around lines 30 - 34, The test uses an ad-hoc Supabase server mock instead of
the shared test client. Update the test setup around the module-level mock to
use the canonical Supabase test client from src/test/, preserving the existing
getUser behavior through that shared client and removing the duplicated
vi.mock('`@/lib/supabase/server`', ...) implementation.
Source: Path instructions
| it('a non-RLS error still surfaces as a single unexpected-failure log (no RLS event, no regression to over-suppression)', async () => { | ||
| upsertError = { code: '23505', message: 'duplicate key value violates unique constraint' }; | ||
|
|
||
| await expect(logSetResult(BASE_INPUT)).rejects.toBeInstanceOf(BaseballActionError); | ||
|
|
||
| const rlsEvents = mocks.logServerEvent.mock.calls.filter( | ||
| (call): call is [string, ...unknown[]] => | ||
| typeof call[0] === 'string' && call[0].startsWith('RLS denial:'), | ||
| ); | ||
| expect(rlsEvents).toHaveLength(0); | ||
| expect(mocks.logServerException).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Third test asserts logServerException call count but not its content — won't catch the detail-loss concern raised on lifting-v11.ts:2032-2048.
Since logSetResult now always throws a generic BaseballActionError() regardless of whether the underlying error was RLS-shaped or a genuine constraint violation, this test's assertion (toHaveBeenCalledTimes(1)) can't distinguish "logged with rich context" from "logged with a generic, uninformative message." Consider asserting on the exception object/message passed to logServerException to pin down whether the original error's code/message survives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/__tests__/lifting-v11-rls-denial.test.ts` around
lines 156 - 167, Strengthen the non-RLS error test for logSetResult by asserting
the arguments passed to logServerException, not only its call count. Verify the
logged exception or message preserves the original duplicate-key error details,
including code 23505 and its message, while still confirming no RLS event is
emitted.
| maybeCaptureRlsDenial(annError, { | ||
| table: 'baseball_announcements', | ||
| verb: 'insert', | ||
| action: 'createAnnouncement', | ||
| feature: 'baseball_announcements', | ||
| sport: 'baseball', | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
All four maybeCaptureRlsDenial calls omit userId, unlike the equivalent calls in messages.ts/lifting-v11.ts/watchlist.ts.
- Line 156-162 (
createAnnouncementAction):ctx.user.idis available (perBaseballActionContext). - Line 420-429 (
deleteAnnouncementAction): same,ctx.user.idavailable. - Line 349-355 (
acknowledgeAnnouncementImpl):user.idis resolved locally at line 300. - Line 205-214 (
getAnnouncementsWithMetaImpl): the function already takes a_userIdparam (currently unused/underscore-prefixed) that could be wired through.
Without userId, the RLS denial: ... on baseball_announcements admin events land with userId: null, making it impossible to correlate a specific coach/player's denial in the admin feed — undercutting the whole point of this telemetry.
🔧 Proposed fix (createAnnouncementAction shown; same pattern applies to the other 3 sites)
maybeCaptureRlsDenial(annError, {
table: 'baseball_announcements',
verb: 'insert',
action: 'createAnnouncement',
feature: 'baseball_announcements',
sport: 'baseball',
+ userId: ctx.user.id,
});Also applies to: 205-214, 349-355, 420-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/announcements.ts` around lines 156 - 162, Update all
four maybeCaptureRlsDenial calls in createAnnouncementAction,
getAnnouncementsWithMetaImpl, acknowledgeAnnouncementImpl, and
deleteAnnouncementAction to include the relevant userId: use ctx.user.id for
action handlers, user.id in acknowledgeAnnouncementImpl, and pass the existing
_userId parameter through getAnnouncementsWithMetaImpl. Rename that parameter if
needed to reflect its use.
| if (error) { | ||
| maybeCaptureRlsDenial(error, { | ||
| table: 'baseball_games', | ||
| verb: 'rpc', | ||
| action: 'saveFullBoxScore', | ||
| feature: 'baseball_games', | ||
| sport: 'baseball', | ||
| }); | ||
| return { success: false, error: sanitizeDbError(error, 'games') }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Same userId gap as announcements.ts — _ctx.user.id is available but not passed.
saveFullBoxScoreAction's handler signature is (_ctx, gameId, ...); the leading underscore just means the context resolution (requireActiveContext: false) is skipped, not that _ctx.user is unavailable — auth still runs first in withBaseballAction. Wire it through so RLS denial: rpc on baseball_games events are attributable.
if (error) {
maybeCaptureRlsDenial(error, {
table: 'baseball_games',
verb: 'rpc',
action: 'saveFullBoxScore',
feature: 'baseball_games',
sport: 'baseball',
+ userId: _ctx.user.id,
});
return { success: false, error: sanitizeDbError(error, 'games') };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error) { | |
| maybeCaptureRlsDenial(error, { | |
| table: 'baseball_games', | |
| verb: 'rpc', | |
| action: 'saveFullBoxScore', | |
| feature: 'baseball_games', | |
| sport: 'baseball', | |
| }); | |
| return { success: false, error: sanitizeDbError(error, 'games') }; | |
| } | |
| if (error) { | |
| maybeCaptureRlsDenial(error, { | |
| table: 'baseball_games', | |
| verb: 'rpc', | |
| action: 'saveFullBoxScore', | |
| feature: 'baseball_games', | |
| sport: 'baseball', | |
| userId: _ctx.user.id, | |
| }); | |
| return { success: false, error: sanitizeDbError(error, 'games') }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/games.ts` around lines 933 - 942, Update
saveFullBoxScoreAction’s error handling to pass _ctx.user.id as the userId field
in the maybeCaptureRlsDenial call, matching the attribution pattern used by
announcements.ts; retain the existing table, verb, action, and feature metadata.
| if (error) { | ||
| maybeCaptureRlsDenial(error, { | ||
| table: 'helm_lifting_set_results', | ||
| verb: 'insert', | ||
| action: 'logSetResult', | ||
| feature: 'baseball_lifting', | ||
| sport: 'baseball', | ||
| userId: ctx.user.id, | ||
| }); | ||
| // Rethrow a generic BaseballActionError (not the raw `error`) so the | ||
| // wrapper's catch-all `maybeCaptureRlsDenial` fallback in | ||
| // with-baseball-action.ts doesn't see an RLS-shaped error and | ||
| // double-fire a second, imprecise denial (table: 'baseball-lifting') | ||
| // on top of the precise one just captured above. Matches | ||
| // watchlist.ts's addToWatchlist pattern. | ||
| throw new BaseballActionError(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Solid double-fire fix, correctly includes userId — but non-RLS errors now lose all diagnostic detail.
The precise maybeCaptureRlsDenial call + generic BaseballActionError() rethrow is well documented and correctly avoids the wrapper's imprecise fallback (verified against with-baseball-action-observability.test.ts and lifting-v11-rls-denial.test.ts). Good.
However, for a non-RLS failure (e.g. the '23505' unique-constraint case exercised in the regression test), maybeCaptureRlsDenial no-ops and the only thing that ultimately reaches logServerException is a fresh BaseballActionError('Something went wrong. Please try again.') — the original Postgrest code/message/details/hint are discarded entirely at this call site. Contrast with messages.ts's sendMessage (lines 141-151), which calls logServerError with the full error metadata before throwing a sanitized message. logSetResult has no equivalent call, so a genuine constraint violation on an upsert becomes an untraceable generic error in Sentry/admin_events.
🩹 Suggested addition (log full detail before throwing, mirroring messages.ts)
if (error) {
maybeCaptureRlsDenial(error, {
table: 'helm_lifting_set_results',
verb: 'insert',
action: 'logSetResult',
feature: 'baseball_lifting',
sport: 'baseball',
userId: ctx.user.id,
});
+ await logServerError(`[logSetResult] Upsert failed: ${error.message}`, {
+ action: 'logSetResult',
+ metadata: { code: error.code, details: error.details, hint: error.hint, userId: ctx.user.id },
+ });
throw new BaseballActionError();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error) { | |
| maybeCaptureRlsDenial(error, { | |
| table: 'helm_lifting_set_results', | |
| verb: 'insert', | |
| action: 'logSetResult', | |
| feature: 'baseball_lifting', | |
| sport: 'baseball', | |
| userId: ctx.user.id, | |
| }); | |
| // Rethrow a generic BaseballActionError (not the raw `error`) so the | |
| // wrapper's catch-all `maybeCaptureRlsDenial` fallback in | |
| // with-baseball-action.ts doesn't see an RLS-shaped error and | |
| // double-fire a second, imprecise denial (table: 'baseball-lifting') | |
| // on top of the precise one just captured above. Matches | |
| // watchlist.ts's addToWatchlist pattern. | |
| throw new BaseballActionError(); | |
| } | |
| if (error) { | |
| maybeCaptureRlsDenial(error, { | |
| table: 'helm_lifting_set_results', | |
| verb: 'insert', | |
| action: 'logSetResult', | |
| feature: 'baseball_lifting', | |
| sport: 'baseball', | |
| userId: ctx.user.id, | |
| }); | |
| await logServerError(`[logSetResult] Upsert failed: ${error.message}`, { | |
| action: 'logSetResult', | |
| metadata: { code: error.code, details: error.details, hint: error.hint, userId: ctx.user.id }, | |
| }); | |
| // Rethrow a generic BaseballActionError (not the raw `error`) so the | |
| // wrapper's catch-all `maybeCaptureRlsDenial` fallback in | |
| // with-baseball-action.ts doesn't see an RLS-shaped error and | |
| // double-fire a second, imprecise denial (table: 'baseball-lifting') | |
| // on top of the precise one just captured above. Matches | |
| // watchlist.ts's addToWatchlist pattern. | |
| throw new BaseballActionError(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/baseball/actions/lifting-v11.ts` around lines 2032 - 2048, In
logSetResult, preserve diagnostics for non-RLS database failures by logging the
original error's full Postgrest metadata (code, message, details, and hint)
through the existing server-error logging mechanism before throwing the
sanitized BaseballActionError. Keep maybeCaptureRlsDenial and the generic
rethrow unchanged for RLS handling, and mirror sendMessage's logging pattern in
messages.ts.
| try { | ||
| return await action(); | ||
| } catch (error) { | ||
| if (isUnauthorized(error)) { | ||
| redirect(`/baseball/login?returnTo=${encodeURIComponent(loginReturnTo)}`); | ||
| } | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the login route validates returnTo is a relative internal path.
fd -t f 'page.tsx' src/app/baseball -x rg -nP 'returnTo' {} \;
rg -nP 'returnTo' src/app/baseball --glob '**/login/**' -C3Repository: njrini99-code/helmv3
Length of output: 5752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- redirect-on-unauthorized.ts --\n'
sed -n '1,120p' src/lib/baseball/redirect-on-unauthorized.ts
printf '\n-- login page --\n'
sed -n '1,140p' src/app/baseball/(auth)/login/page.tsx
printf '\n-- search for any returnTo validation helpers in baseball auth --\n'
rg -n "returnTo|isRelative|startsWith\\('/baseball'\\)|router\\.push\\(" src/app/baseball/(auth) src/lib/baseball -g '!**/*.map'Repository: njrini99-code/helmv3
Length of output: 2109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- login page --\n'
sed -n '1,160p' "src/app/baseball/(auth)/login/page.tsx"
printf '\n-- auth returnTo usage --\n'
rg -n "returnTo|router\\.push|redirect\\(" "src/app/baseball/(auth)" "src/lib/baseball"Repository: njrini99-code/helmv3
Length of output: 10904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' "src/app/baseball/(auth)/login/page.tsx"
printf '\n-- returnTo-related auth routes --\n'
rg -n "returnTo|router\\.push|redirect\\(" "src/app/baseball/(auth)" "src/lib/baseball"Repository: njrini99-code/helmv3
Length of output: 11549
Allowlist returnTo before router.push()
src/app/baseball/(auth)/login/page.tsx:83-96 reads returnTo from the query string and navigates to it directly. Restrict this to internal /baseball/... paths and fall back to /baseball/dashboard/command-center; otherwise this is an open redirect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/baseball/redirect-on-unauthorized.ts` around lines 33 - 41, Validate
the decoded `returnTo` value in the login page before navigating with
`router.push()`: allow only internal paths beginning with `/baseball/`, and fall
back to `/baseball/dashboard/command-center` for missing, malformed, or external
values. Update the redirect flow associated with `redirectOnUnauthorized` to use
this allowlisted destination.
… annotate realtime-messages revalidate exception - push.ts: replace raw fetch with SUPABASE_SERVICE_ROLE_KEY header by supabase.functions.invoke on the existing admin client; error body is duck-typed from the error context so 410 shouldDeactivateToken pruning behavior is unchanged (tests updated to mock functions.invoke). - messages.ts: nosemgrep on sendMessage matching the repo idiom — the messages UI is realtime-subscribed and revalidatePath caused a reload loop on every send (documented in-file since the original removal). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
BASE..HEAD is a two-dot tree diff, but scanners run against the PR
merge-ref checkout; a branch lagging main's deletions feeds semgrep
paths that don't exist there ('Invalid scanning root', exit 2). Filter
the diff to on-disk paths so the gate judges findings, not phantoms.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/notifications/push.ts (1)
220-244: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate prune telemetry possible under concurrent sends — comment's "exactly once" guarantee doesn't hold.
The comment at Line 228 states the prune log "fires exactly once per dead token" because of the
active: trueread-gate at Line 162. But that gate is checked at select time, not enforced atomically at update time — the.update(...)at Line 220 filters only by.eq('token', ...). Two concurrentsendPushNotificationcalls for the same user could both pass the token-select gate before either write commits, both succeed the update, and both emit the "pruned" log.Low blast radius (duplicate info-level log, no data corruption), but the fix is trivial: guard the update with
.eq('active', true)and only log if a row was actually affected.🔧 Proposed fix
// eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const { error: deactivateError } = await (supabase as any) + const { data: deactivatedRows, error: deactivateError } = await (supabase as any) .from('device_tokens') .update({ failed_count: (currentToken?.failed_count || 0) + 1, ...(shouldDeactivateToken ? { active: false } : {}), }) - .eq('token', deviceToken.token) as { error: { message: string } | null }; + .eq('token', deviceToken.token) + .eq('active', true) + .select('token') as { data: unknown[] | null; error: { message: string } | null }; // Prune confirmation: the row's `active: true` gate on the token // read above means a deactivated token is never selected again, so // this fires exactly once per dead token. Logged at info (not // error/warning) so it surfaces as a routine Sentry message / admin // feed entry, not an Error issue competing with real incidents. - if (shouldDeactivateToken && !deactivateError) { + if (shouldDeactivateToken && !deactivateError && deactivatedRows && deactivatedRows.length > 0) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/notifications/push.ts` around lines 220 - 244, Prevent duplicate prune telemetry by making deactivation atomic in the device-token update within sendPushNotification: add an active=true predicate to the existing token update, request or otherwise inspect the affected-row count, and emit the prune log only when the update actually affected a row. Update the nearby comment to describe this atomic update-based guarantee rather than relying solely on the earlier read gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/notifications/push.ts`:
- Around line 220-244: Prevent duplicate prune telemetry by making deactivation
atomic in the device-token update within sendPushNotification: add an
active=true predicate to the existing token update, request or otherwise inspect
the affected-row count, and emit the prune log only when the update actually
affected a row. Update the nearby comment to describe this atomic update-based
guarantee rather than relying solely on the earlier read gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f70c2d1a-99dd-413d-85ea-17a36ad03ad9
📒 Files selected for processing (4)
.github/scripts/changed-files.shsrc/app/actions/messages.tssrc/lib/notifications/push.tssrc/test/lib/notifications/push.test.ts
…ve task-reminder re-arm, annotate the guarded scorecard swap - push.ts: same functions.invoke-through-admin-client change as #798 (3-way merged under this branch's preference-routing rewrite) so the raw service-role header is gone on both branches and they merge clean. - messages.ts: same nosemgrep annotation as #798 (realtime-subscribed UI, intentional no-revalidate). - tasks.ts setTaskReminder: update-or-insert re-arm of the unsent queue row instead of delete-then-insert — a transient failure can no longer leave a task with no armed reminder. - golf.ts fallback swap: nosemgrep annotations — the flagged deletes are the snapshot-guarded swap (null-checked snapshot captured before any delete; every failure path restores) and its rollback helper itself. semgrep exit 0 (unpiped), tsc, eslint, 1,408 targeted tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
1 participant
Morning error-sweep directive: all three production error sources (Vercel runtime, in-app admin_events, Sentry) triaged — the entire surface reduced to three root causes, all fixed and adversarially verified (with a repair round; re-verifiers confirmed each defect closed).
The three root causes
device_tokensrow in the shared send path. Ends retry spam running since June 11. Fixes JAVASCRIPT-NEXTJS-BQ.engine_no_recent_rounds/engine_session_expired) classified info/warning + skipSentry across both consumers; unrecognized codes still alert. Fixes JAVASCRIPT-NEXTJS-BP.BaseballUnauthorizedErrorleaking as raw 500 — sharedredirectOnUnauthorizedhelper (golf idiom,returnTopreserved) across all 11 affected pages; independent sweep of 201 wrapped getters × 106 pages confirms zero remaining instances. Fixes JAVASCRIPT-NEXTJS-BN.Baseball tracking parity
withBaseballAction(60/60 action files covered) + explicit capture at PG-error swallow sites; lifting rethrow corrected (revert-experiment proved the pin test load-bearing).admin_client_errorsrealtime subscription removed.Gates: tsc 0 · eslint 0 (max-warnings 0) · unit 478 files / 4,860 passed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01J2E46URHkhCnEQSYDtZLJX