fix(admin): Helm Bridge soft failures, incident feed, baseball auth#786
Closed
njrini99-code wants to merge 2 commits into
Closed
fix(admin): Helm Bridge soft failures, incident feed, baseball auth#786njrini99-code wants to merge 2 commits into
njrini99-code wants to merge 2 commits into
Conversation
…sh baseball auth
Log { success: false } server-action returns to admin_events via observeActionSoftFailure
in withBaseballAction, withLiftingAction, and withAdminObserved so toast failures surface
in Bridge. Unify Overview, triage, and Errors tab on a shared 24h incident feed with
consistent Sentry windowing. Fix baseball login triple-loading cascade with auth/nav
caching and a branded bootstrap loader. Add Ben+Leah issue board workflow, Work log PR
timeline tab, and session-activity hardening.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
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
|
WalkthroughThis PR adds a GitHub PR work log admin page, a Ben+Leah GitHub issue tracker with workflow updates, a shared incident feed for admin metrics, soft-failure observability for server actions, and baseball auth/session caching and heartbeat changes. ChangesAdmin Work Log
Estimated code review effort: 4 (Complex) | ~60 minutes Ben + Leah Issue Tracker
Estimated code review effort: 4 (Complex) | ~55 minutes Incident Feed Unification
Estimated code review effort: 4 (Complex) | ~50 minutes Server Action Observability
Estimated code review effort: 3 (Moderate) | ~30 minutes Baseball Auth, Navigation, and Session State
Estimated code review effort: 4 (Complex) | ~45 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 |
…ing redirect Replace phantom /admin/audit link with /admin/auth, centralize command-header shortcuts in admin-nav, and send completed players to /baseball/player/today instead of an extra /baseball/dashboard dispatcher hop. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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. |
PR Summary by Qodofix(admin): Helm Bridge soft failures, incident feed, baseball auth
AI Description
Diagram
High-Level Assessment
Files changed (47)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
97 rules 1. Arbitrary text-[11px] font size
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| </option> | ||
| ))} | ||
| </NativeSelect> | ||
| {error ? <p className="max-w-[12rem] text-[11px] text-fw-danger">{error}</p> : null} |
There was a problem hiding this comment.
1. Arbitrary text-[11px] font size 📘 Rule violation ⚙ Maintainability
BenLeahIssueWorkflowSelect uses the Tailwind arbitrary font-size class text-[11px] outside src/components/ui. This violates the typography token requirement and can lead to inconsistent text sizing across the app.
Agent Prompt
## Issue description
`src/app/admin/ben-leah/BenLeahIssueWorkflowSelect.tsx` uses an arbitrary Tailwind pixel font size class (`text-[11px]`). The compliance checklist requires using the approved typography token classes outside `src/components/ui`.
## Issue Context
The error message text should use one of: `text-display`, `text-h1`, `text-h2`, `text-h3`, `text-body-lg`, `text-body`, `text-body-sm`, `text-caption`, `text-eyebrow`.
## Fix Focus Areas
- src/app/admin/ben-leah/BenLeahIssueWorkflowSelect.tsx[67-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+166
to
+177
| <Link | ||
| href={filters.sport ? '/admin/errors?window=168' : '/admin/errors?window=168'} | ||
| className="text-accent-700 underline" | ||
| > | ||
| Open 7-day view | ||
| </Link> | ||
| {filters.sport ? ( | ||
| <> | ||
| {' '} | ||
| or{' '} | ||
| <Link href="/admin/errors?window=168" className="text-accent-700 underline"> | ||
| drop the sport filter |
There was a problem hiding this comment.
2. 7-day link drops sport 🐞 Bug ≡ Correctness
In ErrorsPage, the “Open 7-day view” link does not preserve the current sport filter (both branches use the same href), so users can’t open a 7‑day view scoped to the selected sport and the following “drop the sport filter” link becomes redundant.
Agent Prompt
## Issue description
The “Open 7-day view” link on `/admin/errors` should preserve the current filters (especially `sport`) while changing only the `window` to 168h. Right now it always navigates to `/admin/errors?window=168` regardless of whether a sport filter is active.
## Issue Context
This hint renders only when the current window is empty but a wider 7-day window has unresolved/untagged rows. The copy implies two distinct actions: open a 7-day view *with* the sport filter, or explicitly drop the sport filter to include legacy untagged rows.
## Fix Focus Areas
- src/app/admin/errors/page.tsx[146-181]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+82
| export async function setBenLeahIssueWorkflow( | ||
| issueNumber: number, | ||
| currentLabels: string[], | ||
| selection: BenLeahWorkflowSelection, | ||
| ): Promise<void> { | ||
| const token = githubIssuesToken(); | ||
| if (!token) { | ||
| throw new Error('GitHub issue token is not configured. Set GITHUB_ISSUES_TOKEN or GITHUB_TOKEN.'); | ||
| } | ||
|
|
||
| await ensureBenLeahGitHubLabels(); | ||
|
|
||
| const { owner, repo } = githubIssuesRepo(); | ||
| const labels = applyWorkflowSelection(currentLabels, selection); | ||
|
|
||
| const res = await fetch(`https://api.github.com/repos/${owner}/${repo}/issues/${issueNumber}`, { | ||
| method: 'PATCH', | ||
| headers: { | ||
| ...githubIssuesHeaders(token), | ||
| 'content-type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ labels }), | ||
| }); |
There was a problem hiding this comment.
3. Workflow overwrites labels 🐞 Bug ☼ Reliability
Ben+Leah workflow updates PATCH the issue’s full label list based on currentLabels provided by the client, which can drop labels added/removed concurrently in GitHub or via other sessions when the dropdown is used.
Agent Prompt
## Issue description
`setBenLeahIssueWorkflow()` constructs a complete `labels` array from `currentLabels` passed in by the client and PATCHes it to GitHub. This can overwrite concurrent label changes (race/stale UI) and is also unnecessarily trustful of client-provided label state.
## Issue Context
- The client passes `issue.labels` from the rendered snapshot.
- The server action forwards those labels directly.
- The GitHub PATCH updates the issue with the computed label array, overwriting the server’s current labels.
## Fix Focus Areas
- src/app/admin/ben-leah/BenLeahIssueWorkflowSelect.tsx[26-45]
- src/app/admin/ben-leah/actions.ts[73-112]
- src/lib/admin/github-issues-workflow.ts[60-88]
## Suggested implementation direction
- On the server, re-fetch the issue’s current labels from GitHub immediately before applying the workflow change, then apply `applyWorkflowSelection()` to that fresh label set.
- Alternatively, use GitHub’s labels APIs to add/remove just the workflow labels (avoid full overwrite).
- Consider dropping `currentLabels` from the server action signature entirely to prevent accidental misuse.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+75
to
+82
| const res = await fetch(`https://api.github.com/repos/${owner}/${repo}/issues/${issueNumber}`, { | ||
| method: 'PATCH', | ||
| headers: { | ||
| ...githubIssuesHeaders(token), | ||
| 'content-type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ labels }), | ||
| }); |
| }; | ||
|
|
||
| function stripHtmlComments(text: string): string { | ||
| return text.replace(/<!--[\s\S]*?-->/g, ''); |
|
|
||
| if (!shouldPersistAdminTables()) { | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, context.action ?? ''); | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, enriched.action ?? ''); |
|
|
||
| if (!shouldPersistAdminTables()) { | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, context.action ?? ''); | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, enriched.action ?? ''); |
|
|
||
| if (!shouldPersistAdminTables()) { | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, context.action ?? ''); | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, enriched.action ?? ''); |
|
|
||
| if (!shouldPersistAdminTables()) { | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, context.action ?? ''); | ||
| console.error(`[ServerErrorLogger] (${severity}, not persisted off-prod)`, message, enriched.action ?? ''); |
| await writeAdminTables(message, normalizedError, enriched, severity); | ||
| } catch { | ||
| console.error('[ServerErrorLogger] Failed to persist trace:', message, context); | ||
| console.error('[ServerErrorLogger] Failed to persist trace:', message, enriched); |
| await writeAdminTables(message, normalizedError, enriched, severity); | ||
| } catch { | ||
| console.error('[ServerErrorLogger] Failed to persist trace:', message, context); | ||
| console.error('[ServerErrorLogger] Failed to persist trace:', message, enriched); |
| return Promise.resolve(cachedContext); | ||
| } | ||
|
|
||
| if (userId === null) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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/admin/ben-leah/BenLeahIssueWorkflowSelect.tsx`:
- Line 67: The error message paragraph in BenLeahIssueWorkflowSelect uses an
arbitrary Tailwind font size outside the UI components area. Update the
conditional error render in BenLeahIssueWorkflowSelect to replace the
text-[11px] class with the appropriate canonical text token from the design
system, keeping the existing max-width and danger color styling intact.
In `@src/app/admin/errors/page.tsx`:
- Around line 166-171: The “Open 7-day view” link in the admin errors page is
using a dead ternary and always navigates to the same URL, which drops the
active sport filter. Update the Link in the page component so the href built for
the 7-day view preserves filters.sport when it is present, and leave the
separate “drop the sport filter” link as the only place that removes it. Use the
existing filters logic in the admin errors page to construct the correct query
string.
In `@src/app/admin/page.tsx`:
- Line 342: The Command Center overview is fetching the incident feed twice in
the same render path, once through TriagePanel via fetchTriageQueue and again
through BannerAndKpis via fetchOverviewSnapshot. Hoist a single
fetchIncidentFeed result in src/app/admin/page.tsx (or reuse the existing
prefetched.sentry support in fetchIncidentFeed) and thread the shared Sentry
payload into both panels so both use the same 24h feed instead of issuing
duplicate reads.
In `@src/hooks/use-baseball-nav-context.ts`:
- Around line 64-71: The baseball nav cache is keyed only by userId, so team
switches can reuse stale nav state until TTL expires. Update
use-baseball-nav-context’s caching logic in hasWarmNavCache and the related
cache setters/resetters to include activeTeamId in the cache key, or explicitly
invalidate the cache whenever active_baseball_team changes. Make sure the cache
aligns with getBaseballNavContext, which derives capabilities and programType
from the active team.
In `@src/lib/admin/__tests__/ben-leah-issues.test.ts`:
- Around line 30-70: Add test coverage in deriveBenLeahTrackStatus for the
closed-issue fallback when productionReadyAt is null so it returns fixed, and
add a case that exercises latestProductionReadyAt with multiple READY production
deployments listed out of order. Use the existing issue() helper and
deriveBenLeahTrackStatus assertions in ben-leah-issues.test.ts to cover the
default fixed path and to verify the tracker logic does not depend on array
order, matching the behavior in ben-leah-issue-tracker.ts.
In `@src/lib/admin/ben-leah-issue-tracker.ts`:
- Around line 121-133: The latestProductionReadyAt helper is order-dependent
because it uses deployments.find(), so it can return an older READY production
deployment instead of the newest one. Update latestProductionReadyAt to scan all
eligible VercelDeployment entries and choose the deployment with the greatest
ready timestamp, keeping the existing readyAt/commitSha return shape. Make sure
the selection logic is independent of the array order produced by
fetchVercelDeployments().
In `@src/lib/admin/ben-leah-issues.ts`:
- Around line 141-147: The board fetch path in ben-leah-issues should not
trigger label bootstrapping on every read. Remove the call to
ensureBenLeahGitHubLabels from the board-loading flow in ben-leah-issues and
keep label creation confined to the write paths handled by github-feedback and
github-issues-workflow. If labels still need initialization, move that
responsibility to the label-writing actions rather than the read-only fetch
path.
In `@src/lib/admin/data/incident-feed.ts`:
- Around line 92-94: The `incidentFeed` data fetch is swallowing Supabase
`admin_events` query failures by destructuring only `data` and returning an
empty array, which makes errors look like a valid empty feed. Update the
`incident-feed.ts` fetch path to preserve the query result’s `error` and either
throw on failure or return an explicit app-events status flag, then make sure
callers like `overview.ts` can distinguish failed app-events queries from truly
empty results.
In `@src/lib/admin/github-issues-workflow.ts`:
- Around line 60-88: The label update flow is using a stale client snapshot, so
concurrent GitHub label changes can be overwritten. Update
setBenLeahIssueWorkflow() to fetch the issue’s current labels server-side
immediately before calling the PATCH request, then pass those live labels into
applyWorkflowSelection() instead of the issue.labels value from
BenLeahIssueWorkflowSelect. Keep the existing GitHub API request and error
handling in github-issues-workflow.ts, but ensure the workflow selection is
merged against fresh labels right before writing.
- Around line 28-38: The GitHub API fetches in ensureBenLeahGitHubLabels and
setBenLeahIssueWorkflow can hang indefinitely because they do not use an
AbortSignal. Update both fetch calls in github-issues-workflow.ts to include a
timeout-backed signal, ideally via a shared helper or
AbortSignal.timeout(10_000), so the admin workflow fails fast if api.github.com
stalls.
In `@src/lib/admin/github-pr-timeline.ts`:
- Around line 92-106: The GitHub issue search query in github-pr-timeline’s
author filter is being built with literal plus signs, which causes the author
constraint to be serialized incorrectly. Update the logic in the
authorQuery/searchParams construction so the q value uses space-separated
qualifiers like author:foo author:bar instead of + separators, and keep the rest
of the GitHub search request in github-pr-timeline unchanged.
In `@src/lib/admin/observe-action-result.ts`:
- Around line 25-59: Soft-failure messages are being persisted verbatim into
admin events, so redact or normalize user-derived content before saving. Update
extractActionSoftFailure and the observeActionResult persistence path so
failure.message and the `[${context.action}] ...` wrapper only store approved
user-facing text or a sanitized version with identifiers removed. Use the
existing symbols extractActionSoftFailure and observeActionResult to keep the
redaction centralized for all callers that pass through these action result
envelopes.
In `@src/lib/admin/observed-action.ts`:
- Around line 77-100: The success path in withAdminObserved is doing unnecessary
work on every call by resolving the observed user and building extra context
before confirming there is a soft failure to record. Update the flow around fn,
observeActionSoftFailure, and resolveObservedUser so you first perform the cheap
extractActionSoftFailure check on result and only then call
resolveObservedUser() and opts.contextFrom?.(args) when there is an actual soft
failure to log. Keep the existing context fields and handled flag, but gate the
expensive auth/network round-trip behind that condition.
In `@src/lib/admin/pr-body-parser.ts`:
- Around line 160-178: In pr-body-parser.ts, the parsing logic in the
summary/partnerSummary handling can produce duplicate Problem and Fix text when
only Summary is present. Update the return flow around splitPartnerSummary,
problem, and fix so fix does not fall back to summary when it would equal
problem; instead, leave it null or emit a distinct placeholder that TimelineCard
in WorkTimeline.tsx can display without duplicating the Problem panel. Ensure
the behavior still prefers partnerParts.fix and timelineNote, and only uses
summary as a fallback when it produces a meaningfully different Fix value.
In `@src/lib/baseball/__tests__/with-baseball-action-observability.test.ts`:
- Around line 158-183: Add an integration test in withBaseballAction that
exercises the expected/auth-noise soft-failure path, not just the unexpected
error branch. Use withBaseballAction and the existing Helm Bridge mock
assertions to verify that a soft failure like "Not authenticated" is logged via
logServerError with severity "warning" and skipSentry set to true, while still
returning { success: false, error: ... } and preserving the same action metadata
shape checked in the current test.
In `@src/lib/lifting/with-lifting-action.ts`:
- Around line 181-192: Add a dedicated observability test for
with-lifting-action.ts to cover the new observeActionSoftFailure wiring,
mirroring the existing with-baseball-action-observability.test.ts pattern.
Create a with-lifting-action-observability.test.ts that exercises the
withLiftingAction wrapper’s soft-failure path and asserts
observeActionSoftFailure is called with the expected action,
featureArea/feature, source, sport, userId/userEmail, teamId, handled flag, and
tags. Use the unique symbols withLiftingAction and observeActionSoftFailure to
locate the wrapper and keep the test aligned with the baseball cohort’s
coverage.
- Around line 181-192: The soft-failure event in withLiftingAction is persisting
the wrong sport grouping key, so it is being stored as shared while only the
Sentry tag says lifting. Update the observeActionSoftFailure call in
withLiftingAction to set the top-level sport field to lifting so
admin_events.sport matches the incident-feed grouping used by
server-error-logger, incident-feed, and triage. Keep the existing tags.sport
metadata if needed, but make sure the persisted sport value and the
lifting-specific grouping are aligned.
In `@src/lib/server-error-logger.ts`:
- Around line 151-172: The issue is that inferSport() and enrichTraceContext()
classify the same error using different probes, which can produce conflicting
sport and feature values. Update server-error-logger’s inferSport() and
enrichTraceContext() so classification happens once from a single shared
probe/result, then use that same classification to populate both sport and
feature. Keep the logic centered around classifyTraceSurface() and
enrichTraceContext() to ensure admin_events.sport and admin_events.feature
always stay aligned.
🪄 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: c3b673bd-2613-4c6d-b993-9279622e7fbe
📒 Files selected for processing (48)
.env.examplesrc/app/admin/_components/AdminShell.tsxsrc/app/admin/_components/__tests__/admin-nav.test.tssrc/app/admin/_components/admin-nav.tssrc/app/admin/ben-leah/BenLeahIssueBoard.tsxsrc/app/admin/ben-leah/BenLeahIssueTable.tsxsrc/app/admin/ben-leah/BenLeahIssueWorkflowSelect.tsxsrc/app/admin/ben-leah/actions.tssrc/app/admin/ben-leah/page.tsxsrc/app/admin/errors/page.tsxsrc/app/admin/page.tsxsrc/app/admin/work/WorkTimeline.tsxsrc/app/admin/work/page.tsxsrc/app/baseball/(dashboard)/BaseballFairwayShell.tsxsrc/app/baseball/(dashboard)/layout.tsxsrc/app/golf/actions/__tests__/coverage-contract.observability.test.tssrc/components/auth/baseball-sign-in-form.tsxsrc/components/baseball/BaseballDashboardBootstrap.tsxsrc/components/baseball/BaseballShellLayout.tsxsrc/components/providers/SessionActivityProvider.tsxsrc/hooks/__tests__/use-baseball-auth.test.tsxsrc/hooks/use-baseball-auth.tssrc/hooks/use-baseball-nav-context.tssrc/lib/__tests__/server-error-logger-bridge.test.tssrc/lib/admin/__tests__/ben-leah-issues.test.tssrc/lib/admin/__tests__/observe-action-result.test.tssrc/lib/admin/__tests__/pr-body-parser.test.tssrc/lib/admin/ben-leah-issue-tracker.tssrc/lib/admin/ben-leah-issues.tssrc/lib/admin/data/__tests__/incident-feed.test.tssrc/lib/admin/data/errors.tssrc/lib/admin/data/incident-feed.tssrc/lib/admin/data/overview.tssrc/lib/admin/data/triage.tssrc/lib/admin/github-feedback.tssrc/lib/admin/github-issues-config.tssrc/lib/admin/github-issues-workflow.tssrc/lib/admin/github-pr-timeline.tssrc/lib/admin/observe-action-result.tssrc/lib/admin/observed-action.tssrc/lib/admin/pr-body-parser.tssrc/lib/auth/session-activity.tssrc/lib/auth/session-idle-shared.tssrc/lib/baseball/__tests__/with-baseball-action-observability.test.tssrc/lib/baseball/with-baseball-action.tssrc/lib/lifting/with-lifting-action.tssrc/lib/server-error-logger.tssrc/test/lib/auth/session-idle-shared.test.ts
| </option> | ||
| ))} | ||
| </NativeSelect> | ||
| {error ? <p className="max-w-[12rem] text-[11px] text-fw-danger">{error}</p> : null} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Arbitrary text-[11px] outside src/components/ui.
Path instructions require the canonical text scale (text-caption, text-body-sm, etc.) instead of arbitrary pixel sizes for files outside src/components/ui.
🎨 Use a canonical text token
- {error ? <p className="max-w-[12rem] text-[11px] text-fw-danger">{error}</p> : null}
+ {error ? <p className="max-w-[12rem] text-caption text-fw-danger">{error}</p> : null}As per path instructions, "Do not use arbitrary text-[Npx] sizes outside src/components/ui; use the canonical text scale tokens instead."
📝 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.
Suggested change
| {error ? <p className="max-w-[12rem] text-[11px] text-fw-danger">{error}</p> : null} | |
| {error ? <p className="max-w-[12rem] text-caption text-fw-danger">{error}</p> : 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/app/admin/ben-leah/BenLeahIssueWorkflowSelect.tsx` at line 67, The error
message paragraph in BenLeahIssueWorkflowSelect uses an arbitrary Tailwind font
size outside the UI components area. Update the conditional error render in
BenLeahIssueWorkflowSelect to replace the text-[11px] class with the appropriate
canonical text token from the design system, keeping the existing max-width and
danger color styling intact.
Source: Path instructions
Comment on lines
+166
to
+171
| <Link | ||
| href={filters.sport ? '/admin/errors?window=168' : '/admin/errors?window=168'} | ||
| className="text-accent-700 underline" | ||
| > | ||
| Open 7-day view | ||
| </Link> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dead ternary drops the active sport filter on "Open 7-day view." src/app/admin/errors/page.tsx:167 — both branches of filters.sport ? '/admin/errors?window=168' : '/admin/errors?window=168' are byte-identical, so the conditional is a no-op and the sport filter is discarded whenever it's set. Since the separate link at line 176 already exists to "drop the sport filter," the primary "Open 7-day view" link should preserve it.
🐛 Preserve sport on the primary link
<Link
- href={filters.sport ? '/admin/errors?window=168' : '/admin/errors?window=168'}
+ href={filters.sport ? `/admin/errors?window=168&sport=${filters.sport}` : '/admin/errors?window=168'}
className="text-accent-700 underline"
>
Open 7-day view
</Link>📝 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.
Suggested change
| <Link | |
| href={filters.sport ? '/admin/errors?window=168' : '/admin/errors?window=168'} | |
| className="text-accent-700 underline" | |
| > | |
| Open 7-day view | |
| </Link> | |
| <Link | |
| href={filters.sport ? `/admin/errors?window=168&sport=${filters.sport}` : '/admin/errors?window=168'} | |
| className="text-accent-700 underline" | |
| > | |
| Open 7-day view | |
| </Link> |
🤖 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/admin/errors/page.tsx` around lines 166 - 171, The “Open 7-day view”
link in the admin errors page is using a dead ternary and always navigates to
the same URL, which drops the active sport filter. Update the Link in the page
component so the href built for the 7-day view preserves filters.sport when it
is present, and leave the separate “drop the sport filter” link as the only
place that removes it. Use the existing filters logic in the admin errors page
to construct the correct query string.
|
|
||
| async function TriagePanel() { | ||
| const { items, sentry } = await fetchTriageQueue(); | ||
| const { items, sentry, counts } = await fetchTriageQueue(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
Overview page fetches the incident feed twice per render. TriagePanel calls fetchTriageQueue() (src/app/admin/page.tsx:342) which runs fetchIncidentFeed, and BannerAndKpis (line 85) runs fetchOverviewSnapshot which runs fetchIncidentFeed again — so every Command Center load hits the Sentry read API and the admin_events query twice with identical 24h filters. fetchIncidentFeed already exposes a prefetched: { sentry } param precisely to dedupe this.
This is a reasonable trade for independent per-panel streaming/degradation, but given Sentry API rate limits it's worth hoisting one feed fetch and threading it (or at least the Sentry result) into both panels.
🤖 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/admin/page.tsx` at line 342, The Command Center overview is fetching
the incident feed twice in the same render path, once through TriagePanel via
fetchTriageQueue and again through BannerAndKpis via fetchOverviewSnapshot.
Hoist a single fetchIncidentFeed result in src/app/admin/page.tsx (or reuse the
existing prefetched.sentry support in fetchIncidentFeed) and thread the shared
Sentry payload into both panels so both use the same 24h feed instead of issuing
duplicate reads.
Comment on lines
+64
to
+71
| function hasWarmNavCache(userId: string | null): boolean { | ||
| return ( | ||
| userId !== null && | ||
| userId === cacheUserId && | ||
| cachedContext !== null && | ||
| Date.now() - cacheResolvedAt < NAV_CACHE_TTL_MS | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for team-switch invalidation of the nav cache
rg -nP 'clearNavContextCache|setActiveTeam|switchTeam|activeTeamId' src/lib/baseball src/hooks src/stores -C3Repository: njrini99-code/helmv3
Length of output: 19600
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== use-baseball-nav-context.ts =="
cat -n src/hooks/use-baseball-nav-context.ts | sed -n '1,220p'
echo
echo "== clearNavContextCache references =="
rg -n "clearNavContextCache" src -C 2
echo
echo "== active team / switcher references =="
rg -n "activeTeamId|setActiveTeam|switchTeam|team switch|switch team|select team|current team" src/app src/components src/hooks src/lib/baseball src/stores -C 2Repository: njrini99-code/helmv3
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the hook around the cache and effect logic with line numbers.
sed -n '1,220p' src/hooks/use-baseball-nav-context.ts | nl -ba
echo
# Find the active team resolver to see what actually changes the context.
sed -n '1,220p' src/lib/baseball/active-context.ts | nl -ba
echo
# Locate any team-switch UI or action that might clear the nav cache.
rg -n "clearNavContextCache|activeTeamId|setActiveTeam|switchTeam|teamId" src/app src/components src/hooks src/lib/baseball src/stores -C 2Repository: njrini99-code/helmv3
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/hooks/use-baseball-nav-context.ts | sed -n '1,220p'Repository: njrini99-code/helmv3
Length of output: 6607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== useBaseballNavContext usages =="
rg -n "useBaseballNavContext" src -C 2
echo
echo "== baseball active-team switch / cookie refs =="
rg -n "active_baseball_team|setActiveBaseball|switchTeam|team switch|fellBackFromStale|clearNavContextCache" src/app/baseball src/components/baseball src/lib/baseball src/hooks -C 2
echo
echo "== baseball team switcher / active-context action files =="
fd -a 'team-switcher*' src/app/baseball src/components/baseball src/lib/baseball src/hooks 2>/dev/null || true
fd -a '*active*team*' src/app/baseball src/components/baseball src/lib/baseball src/hooks 2>/dev/null || trueRepository: njrini99-code/helmv3
Length of output: 12298
Key the baseball nav cache by active team. src/hooks/use-baseball-nav-context.ts:64-71 caches by userId only, and src/hooks/use-baseball-nav-context.ts:122-151 never invalidates on team changes. getBaseballNavContext() reads activeTeamId when resolving capabilities and programType (src/lib/baseball/nav-context.ts:118-127), so a multi-team coach can keep the previous team's gated nav until the 5s TTL expires. Add activeTeamId to the cache key or clear the cache when the team switcher updates active_baseball_team.
🤖 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/hooks/use-baseball-nav-context.ts` around lines 64 - 71, The baseball nav
cache is keyed only by userId, so team switches can reuse stale nav state until
TTL expires. Update use-baseball-nav-context’s caching logic in hasWarmNavCache
and the related cache setters/resetters to include activeTeamId in the cache
key, or explicitly invalidate the cache whenever active_baseball_team changes.
Make sure the cache aligns with getBaseballNavContext, which derives
capabilities and programType from the active team.
Comment on lines
+30
to
+70
| describe('deriveBenLeahTrackStatus', () => { | ||
| it('marks open issues without workflow labels as open', () => { | ||
| expect(deriveBenLeahTrackStatus(issue(), null)).toBe('open'); | ||
| }); | ||
|
|
||
| it('marks status:in-progress as in progress while open', () => { | ||
| expect( | ||
| deriveBenLeahTrackStatus(issue({ labels: ['ben-leah', 'status:in-progress'] }), null), | ||
| ).toBe('in_progress'); | ||
| }); | ||
|
|
||
| it('marks status:wontfix regardless of state', () => { | ||
| expect( | ||
| deriveBenLeahTrackStatus(issue({ state: 'closed', labels: ['status:wontfix'] }), null), | ||
| ).toBe('wont_fix'); | ||
| }); | ||
|
|
||
| it('marks closed issues shipped when production deploy is newer than close', () => { | ||
| const closed = issue({ | ||
| state: 'closed', | ||
| closed_at: '2026-07-04T12:00:00.000Z', | ||
| }); | ||
| const prodReady = Date.parse('2026-07-04T13:00:00.000Z'); | ||
| expect(deriveBenLeahTrackStatus(closed, prodReady)).toBe('in_production'); | ||
| }); | ||
|
|
||
| it('marks closed issues pending deploy when production is older than close', () => { | ||
| const closed = issue({ | ||
| state: 'closed', | ||
| closed_at: '2026-07-04T14:00:00.000Z', | ||
| }); | ||
| const prodReady = Date.parse('2026-07-04T12:00:00.000Z'); | ||
| expect(deriveBenLeahTrackStatus(closed, prodReady)).toBe('fixed_pending_deploy'); | ||
| }); | ||
|
|
||
| it('honors explicit status:in-production label', () => { | ||
| expect( | ||
| deriveBenLeahTrackStatus(issue({ labels: ['status:in-production'] }), null), | ||
| ).toBe('in_production'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for the fixed fallback and multi-deployment ordering.
No test covers a closed issue with productionReadyAt === null (defaults to 'fixed') — the common case when Vercel isn't configured. Also no test for latestProductionReadyAt with multiple READY production deployments out of order, which would have caught the .find() ordering issue flagged in ben-leah-issue-tracker.ts.
✅ Suggested additional cases
it('honors explicit status:in-production label', () => {
expect(
deriveBenLeahTrackStatus(issue({ labels: ['status:in-production'] }), null),
).toBe('in_production');
});
+
+ it('falls back to fixed when closed with no production deploy data', () => {
+ const closed = issue({ state: 'closed', closed_at: '2026-07-04T14:00:00.000Z' });
+ expect(deriveBenLeahTrackStatus(closed, null)).toBe('fixed');
+ });
});Also applies to: 92-125
🤖 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/admin/__tests__/ben-leah-issues.test.ts` around lines 30 - 70, Add
test coverage in deriveBenLeahTrackStatus for the closed-issue fallback when
productionReadyAt is null so it returns fixed, and add a case that exercises
latestProductionReadyAt with multiple READY production deployments listed out of
order. Use the existing issue() helper and deriveBenLeahTrackStatus assertions
in ben-leah-issues.test.ts to cover the default fixed path and to verify the
tracker logic does not depend on array order, matching the behavior in
ben-leah-issue-tracker.ts.
Comment on lines
77
to
+100
| try { | ||
| return await fn(...args); | ||
| const result = await fn(...args); | ||
| const observedUser = await resolveObservedUser(); | ||
| let extraContext: ObservedActionContext = {}; | ||
| try { | ||
| extraContext = opts.contextFrom?.(args) ?? {}; | ||
| } catch { | ||
| extraContext = {}; | ||
| } | ||
| observeActionSoftFailure(result, { | ||
| action: name, | ||
| source: 'server_action', | ||
| feature: opts.feature ?? null, | ||
| featureArea: opts.featureArea ?? opts.feature ?? null, | ||
| sport: opts.sport, | ||
| userId: observedUser.userId, | ||
| userEmail: observedUser.userEmail, | ||
| roundId: extraContext.roundId ?? null, | ||
| playerId: extraContext.playerId ?? null, | ||
| teamId: extraContext.teamId ?? null, | ||
| route: extraContext.route ?? null, | ||
| handled: true, | ||
| }); | ||
| return result; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Unconditional auth round-trip added to every successful call — gate it behind an actual soft-failure detection.
resolveObservedUser() (a fresh supabase.auth.getUser() network call) and opts.contextFrom?.(args) now run on every successful invocation of any withAdminObserved-wrapped action, before checking whether result is even a soft failure. Previously this path was a plain return await fn(...args). Since observeActionSoftFailure internally bails out immediately via extractActionSoftFailure(result) for the vast majority of normal successes (line 79 of observe-action-result.ts), this work is thrown away most of the time while still paying the auth-server latency on the hot path for every admin action.
Hoist the cheap synchronous extractActionSoftFailure check before doing the expensive/async identity resolution:
⚡ Proposed fix — only resolve identity when there's actually something to log
+import { extractActionSoftFailure, observeActionSoftFailure } from '`@/lib/admin/observe-action-result`';
-import { observeActionSoftFailure } from '`@/lib/admin/observe-action-result`';
...
const result = await fn(...args);
- const observedUser = await resolveObservedUser();
- let extraContext: ObservedActionContext = {};
- try {
- extraContext = opts.contextFrom?.(args) ?? {};
- } catch {
- extraContext = {};
- }
- observeActionSoftFailure(result, {
- action: name,
- source: 'server_action',
- feature: opts.feature ?? null,
- featureArea: opts.featureArea ?? opts.feature ?? null,
- sport: opts.sport,
- userId: observedUser.userId,
- userEmail: observedUser.userEmail,
- roundId: extraContext.roundId ?? null,
- playerId: extraContext.playerId ?? null,
- teamId: extraContext.teamId ?? null,
- route: extraContext.route ?? null,
- handled: true,
- });
+ if (extractActionSoftFailure(result)) {
+ const observedUser = await resolveObservedUser();
+ let extraContext: ObservedActionContext = {};
+ try {
+ extraContext = opts.contextFrom?.(args) ?? {};
+ } catch {
+ extraContext = {};
+ }
+ observeActionSoftFailure(result, {
+ action: name,
+ source: 'server_action',
+ feature: opts.feature ?? null,
+ featureArea: opts.featureArea ?? opts.feature ?? null,
+ sport: opts.sport,
+ userId: observedUser.userId,
+ userEmail: observedUser.userEmail,
+ roundId: extraContext.roundId ?? null,
+ playerId: extraContext.playerId ?? null,
+ teamId: extraContext.teamId ?? null,
+ route: extraContext.route ?? null,
+ handled: true,
+ });
+ }
return result;📝 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.
Suggested change
| try { | |
| return await fn(...args); | |
| const result = await fn(...args); | |
| const observedUser = await resolveObservedUser(); | |
| let extraContext: ObservedActionContext = {}; | |
| try { | |
| extraContext = opts.contextFrom?.(args) ?? {}; | |
| } catch { | |
| extraContext = {}; | |
| } | |
| observeActionSoftFailure(result, { | |
| action: name, | |
| source: 'server_action', | |
| feature: opts.feature ?? null, | |
| featureArea: opts.featureArea ?? opts.feature ?? null, | |
| sport: opts.sport, | |
| userId: observedUser.userId, | |
| userEmail: observedUser.userEmail, | |
| roundId: extraContext.roundId ?? null, | |
| playerId: extraContext.playerId ?? null, | |
| teamId: extraContext.teamId ?? null, | |
| route: extraContext.route ?? null, | |
| handled: true, | |
| }); | |
| return result; | |
| import { extractActionSoftFailure, observeActionSoftFailure } from '`@/lib/admin/observe-action-result`'; | |
| ... | |
| try { | |
| const result = await fn(...args); | |
| if (extractActionSoftFailure(result)) { | |
| const observedUser = await resolveObservedUser(); | |
| let extraContext: ObservedActionContext = {}; | |
| try { | |
| extraContext = opts.contextFrom?.(args) ?? {}; | |
| } catch { | |
| extraContext = {}; | |
| } | |
| observeActionSoftFailure(result, { | |
| action: name, | |
| source: 'server_action', | |
| feature: opts.feature ?? null, | |
| featureArea: opts.featureArea ?? opts.feature ?? null, | |
| sport: opts.sport, | |
| userId: observedUser.userId, | |
| userEmail: observedUser.userEmail, | |
| roundId: extraContext.roundId ?? null, | |
| playerId: extraContext.playerId ?? null, | |
| teamId: extraContext.teamId ?? null, | |
| route: extraContext.route ?? null, | |
| handled: true, | |
| }); | |
| } | |
| return result; |
🤖 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/admin/observed-action.ts` around lines 77 - 100, The success path in
withAdminObserved is doing unnecessary work on every call by resolving the
observed user and building extra context before confirming there is a soft
failure to record. Update the flow around fn, observeActionSoftFailure, and
resolveObservedUser so you first perform the cheap extractActionSoftFailure
check on result and only then call resolveObservedUser() and
opts.contextFrom?.(args) when there is an actual soft failure to log. Keep the
existing context fields and handled flag, but gate the expensive auth/network
round-trip behind that condition.
Comment on lines
+160
to
+178
| const partnerParts = partnerSummary ? splitPartnerSummary(partnerSummary) : { problem: null, fix: null }; | ||
|
|
||
| const problem = partnerParts.problem ?? summary ?? null; | ||
|
|
||
| const fix = | ||
| partnerParts.fix ?? | ||
| timelineNote ?? | ||
| (partnerParts.problem && summary ? summary : null); | ||
|
|
||
| return { | ||
| summary, | ||
| partnerSummary, | ||
| problem, | ||
| fix, | ||
| area, | ||
| timelineNote, | ||
| changeTypes, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Problem/Fix collapse to duplicate text when only "Summary" is filled.
When a PR has a ## Summary but no ## Partner-readable summary and no ## Git Activity Timeline note, partnerParts is {problem: null, fix: null}, so:
problem = summary(line 162)fix = null ?? null ?? (null && summary ? ... : null) = null(lines 164-167, sincepartnerParts.problemisnull, the final fallback branch is never taken here)
fix ends up null, and WorkTimeline.tsx's TimelineCard (lines 53-57) then falls back to entry.parsed.summary for the "Fix / outcome" card — the exact same text already shown in "Problem". Given the PR copy explicitly nudges authors to fill the partner-readable summary (page.tsx: "no AI summaries"), this will be the common case until authors adopt the new template, so the timeline will regularly show two identical panels per card.
Consider either not falling back to summary for fix when it equals problem, or emitting a distinct placeholder in that case.
🩹 Proposed fix
- const problem = partnerParts.problem ?? summary ?? null;
-
- const fix =
- partnerParts.fix ??
- timelineNote ??
- (partnerParts.problem && summary ? summary : null);
+ const problem = partnerParts.problem ?? summary ?? null;
+
+ const fix =
+ partnerParts.fix ??
+ timelineNote ??
+ (partnerParts.problem && summary && summary !== problem ? summary : 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/admin/pr-body-parser.ts` around lines 160 - 178, In
pr-body-parser.ts, the parsing logic in the summary/partnerSummary handling can
produce duplicate Problem and Fix text when only Summary is present. Update the
return flow around splitPartnerSummary, problem, and fix so fix does not fall
back to summary when it would equal problem; instead, leave it null or emit a
distinct placeholder that TimelineCard in WorkTimeline.tsx can display without
duplicating the Problem panel. Ensure the behavior still prefers
partnerParts.fix and timelineNote, and only uses summary as a fallback when it
produces a meaningfully different Fix value.
Comment on lines
+158
to
+183
|
|
||
| it('logs { success: false } soft failures to Helm Bridge without throwing', async () => { | ||
| const action = withBaseballAction( | ||
| 'uploadBaseballDocument', | ||
| { featureArea: 'baseball-documents', feature: 'baseball_documents' }, | ||
| async () => ({ success: false as const, error: 'Storage bucket unavailable' }), | ||
| ); | ||
|
|
||
| const result = await action(); | ||
| expect(result).toEqual({ success: false, error: 'Storage bucket unavailable' }); | ||
| expect(mocks.logServerException).not.toHaveBeenCalled(); | ||
| expect(mocks.logServerError).toHaveBeenCalledTimes(1); | ||
|
|
||
| const softCall = mocks.logServerError.mock.calls[0] as | ||
| | [string, Record<string, unknown> | undefined, 'warning' | 'error' | 'critical'] | ||
| | undefined; | ||
| expect(softCall?.[0]).toBe('Storage bucket unavailable'); | ||
| expect(softCall?.[2]).toBe('error'); | ||
| expect(softCall?.[1]).toMatchObject({ | ||
| action: 'uploadBaseballDocument', | ||
| sport: 'baseball', | ||
| feature: 'baseball_documents', | ||
| source: 'server_action', | ||
| handled: true, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a companion test for the expected/auth-noise soft-failure path through the wrapper.
This test only covers the "unexpected" branch ('Storage bucket unavailable' → severity: 'error'). The whole point of isExpectedSoftFailureMessage/skipSentry is to suppress noisy-but-expected messages (e.g. 'Not authenticated') at 'warning' with skipSentry: true — that mapping is only unit-tested in isolation in observe-action-result.test.ts, not through the actual withBaseballAction integration path exercised here.
✅ Suggested additional test
+ it('logs expected soft failures as warnings with skipSentry through the wrapper', async () => {
+ const action = withBaseballAction(
+ 'loadBaseballRoster',
+ { featureArea: 'baseball-roster', feature: 'baseball_roster' },
+ async () => ({ success: false as const, error: 'Not authenticated' }),
+ );
+
+ await action();
+ const softCall = mocks.logServerError.mock.calls[0] as
+ | [string, Record<string, unknown> | undefined, 'warning' | 'error' | 'critical']
+ | undefined;
+ expect(softCall?.[2]).toBe('warning');
+ expect(softCall?.[1]).toMatchObject({ skipSentry: true });
+ });📝 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.
Suggested change
| it('logs { success: false } soft failures to Helm Bridge without throwing', async () => { | |
| const action = withBaseballAction( | |
| 'uploadBaseballDocument', | |
| { featureArea: 'baseball-documents', feature: 'baseball_documents' }, | |
| async () => ({ success: false as const, error: 'Storage bucket unavailable' }), | |
| ); | |
| const result = await action(); | |
| expect(result).toEqual({ success: false, error: 'Storage bucket unavailable' }); | |
| expect(mocks.logServerException).not.toHaveBeenCalled(); | |
| expect(mocks.logServerError).toHaveBeenCalledTimes(1); | |
| const softCall = mocks.logServerError.mock.calls[0] as | |
| | [string, Record<string, unknown> | undefined, 'warning' | 'error' | 'critical'] | |
| | undefined; | |
| expect(softCall?.[0]).toBe('Storage bucket unavailable'); | |
| expect(softCall?.[2]).toBe('error'); | |
| expect(softCall?.[1]).toMatchObject({ | |
| action: 'uploadBaseballDocument', | |
| sport: 'baseball', | |
| feature: 'baseball_documents', | |
| source: 'server_action', | |
| handled: true, | |
| }); | |
| }); | |
| it('logs { success: false } soft failures to Helm Bridge without throwing', async () => { | |
| const action = withBaseballAction( | |
| 'uploadBaseballDocument', | |
| { featureArea: 'baseball-documents', feature: 'baseball_documents' }, | |
| async () => ({ success: false as const, error: 'Storage bucket unavailable' }), | |
| ); | |
| const result = await action(); | |
| expect(result).toEqual({ success: false, error: 'Storage bucket unavailable' }); | |
| expect(mocks.logServerException).not.toHaveBeenCalled(); | |
| expect(mocks.logServerError).toHaveBeenCalledTimes(1); | |
| const softCall = mocks.logServerError.mock.calls[0] as | |
| | [string, Record<string, unknown> | undefined, 'warning' | 'error' | 'critical'] | |
| | undefined; | |
| expect(softCall?.[0]).toBe('Storage bucket unavailable'); | |
| expect(softCall?.[2]).toBe('error'); | |
| expect(softCall?.[1]).toMatchObject({ | |
| action: 'uploadBaseballDocument', | |
| sport: 'baseball', | |
| feature: 'baseball_documents', | |
| source: 'server_action', | |
| handled: true, | |
| }); | |
| }); | |
| it('logs expected soft failures as warnings with skipSentry through the wrapper', async () => { | |
| const action = withBaseballAction( | |
| 'loadBaseballRoster', | |
| { featureArea: 'baseball-roster', feature: 'baseball_roster' }, | |
| async () => ({ success: false as const, error: 'Not authenticated' }), | |
| ); | |
| await action(); | |
| const softCall = mocks.logServerError.mock.calls[0] as | |
| | [string, Record<string, unknown> | undefined, 'warning' | 'error' | 'critical'] | |
| | undefined; | |
| expect(softCall?.[2]).toBe('warning'); | |
| expect(softCall?.[1]).toMatchObject({ skipSentry: true }); | |
| }); |
🤖 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/__tests__/with-baseball-action-observability.test.ts` around
lines 158 - 183, Add an integration test in withBaseballAction that exercises
the expected/auth-noise soft-failure path, not just the unexpected error branch.
Use withBaseballAction and the existing Helm Bridge mock assertions to verify
that a soft failure like "Not authenticated" is logged via logServerError with
severity "warning" and skipSentry set to true, while still returning { success:
false, error: ... } and preserving the same action metadata shape checked in the
current test.
Comment on lines
+181
to
+192
| observeActionSoftFailure(result, { | ||
| action: name, | ||
| featureArea, | ||
| feature: featureArea.replaceAll('-', '_'), | ||
| source: 'server_action', | ||
| sport: 'shared', | ||
| userId: user.id, | ||
| userEmail: user.email ?? null, | ||
| teamId: orgId, | ||
| handled: true, | ||
| tags: { sport: 'lifting', feature: featureArea, lifting_org: orgId }, | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No lifting-specific integration test for the new observeActionSoftFailure wiring.
src/lib/baseball/with-baseball-action.ts got a dedicated observability test (with-baseball-action-observability.test.ts); this cohort's file list has no equivalent for with-lifting-action.ts. Given the pattern is now established, replicating it here is cheap and closes the gap for this wrapper's soft-failure path.
Want me to draft a with-lifting-action-observability.test.ts mirroring the baseball one?
🤖 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/lifting/with-lifting-action.ts` around lines 181 - 192, Add a
dedicated observability test for with-lifting-action.ts to cover the new
observeActionSoftFailure wiring, mirroring the existing
with-baseball-action-observability.test.ts pattern. Create a
with-lifting-action-observability.test.ts that exercises the withLiftingAction
wrapper’s soft-failure path and asserts observeActionSoftFailure is called with
the expected action, featureArea/feature, source, sport, userId/userEmail,
teamId, handled flag, and tags. Use the unique symbols withLiftingAction and
observeActionSoftFailure to locate the wrapper and keep the test aligned with
the baseball cohort’s coverage.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how sport is used for incident-feed grouping/filtering vs tags.sport.
rg -nP "sport" src/lib/admin/data/incident-feed.ts src/lib/server-error-logger.ts -C2Repository: njrini99-code/helmv3
Length of output: 4871
🏁 Script executed:
#!/bin/bash
sed -n '150,240p' src/lib/lifting/with-lifting-action.ts
printf '\n--- server-error-logger ---\n'
sed -n '1,280p' src/lib/server-error-logger.ts
printf '\n--- incident-feed ---\n'
sed -n '1,220p' src/lib/admin/data/incident-feed.tsRepository: njrini99-code/helmv3
Length of output: 17283
🏁 Script executed:
#!/bin/bash
rg -n "function mergeTriage|mergeTriage\(|type AppTriageEventRow|sport" src/lib/admin/data/triage.ts src/lib/admin/incident-grouping.ts src/lib/admin/data -C3Repository: njrini99-code/helmv3
Length of output: 42853
Keep the persisted sport key aligned with incident-feed grouping src/lib/lifting/with-lifting-action.ts:181-192
admin_events.sport is what the feed filters and buckets on (src/lib/server-error-logger.ts:106,163-169,218; src/lib/admin/data/incident-feed.ts:87; src/lib/admin/data/triage.ts:192,210). tags.sport only survives as Sentry metadata (src/lib/server-error-logger.ts:241). If these soft failures are meant to show up as lifting, the top-level sport value needs to match the grouping key; otherwise the event lands in shared while still carrying a lifting tag.
🤖 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/lifting/with-lifting-action.ts` around lines 181 - 192, The
soft-failure event in withLiftingAction is persisting the wrong sport grouping
key, so it is being stored as shared while only the Sentry tag says lifting.
Update the observeActionSoftFailure call in withLiftingAction to set the
top-level sport field to lifting so admin_events.sport matches the incident-feed
grouping used by server-error-logger, incident-feed, and triage. Keep the
existing tags.sport metadata if needed, but make sure the persisted sport value
and the lifting-specific grouping are aligned.
Comment on lines
+151
to
+172
| /** Infer sport when legacy emitters omit context.sport (most baseball actions pre-withBaseballAction). */ | ||
| function inferSport(message: string, context: RoundErrorContext): NonNullable<RoundErrorContext['sport']> | null { | ||
| if (context.sport) return context.sport; | ||
| const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''}`; | ||
| const classified = classifyTraceSurface(probe, context.action ?? null); | ||
| if (classified.sport) return classified.sport; | ||
| if (/baseball/i.test(message)) return 'baseball'; | ||
| if (/\bgolf\b|coachhelm/i.test(message)) return 'golf'; | ||
| return null; | ||
| } | ||
|
|
||
| function enrichTraceContext(message: string, context: RoundErrorContext): RoundErrorContext { | ||
| const sport = inferSport(message, context); | ||
| if (!sport) return context; | ||
| const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''} ${message}`; | ||
| const classified = classifyTraceSurface(probe, context.action ?? null); | ||
| return { | ||
| ...context, | ||
| sport, | ||
| feature: context.feature ?? classified.feature ?? context.featureArea ?? null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
sport and feature can be derived from conflicting classifications.
inferSport() classifies using probe = route+url+action only (Line 154-156), falling back to message regex if classifyTraceSurface finds nothing. enrichTraceContext() then classifies again with a different probe that also includes message (Line 165-166) purely to pick feature. classifyTraceSurface prioritizes baseball over golf internally, so if route/url signal one sport but message text signals the other, the two calls can disagree: the final row gets sport from the first classification and feature from the second, i.e. a baseball_* feature key attached to a golf sport row (or vice versa). This directly feeds admin_events.sport/admin_events.feature, which the new unified incident feed uses for Overview KPIs/triage grouping — a mismatched pair misroutes incidents.
🐛 Proposed fix — classify once, derive feature from the same result as sport
-/** Infer sport when legacy emitters omit context.sport (most baseball actions pre-withBaseballAction). */
-function inferSport(message: string, context: RoundErrorContext): NonNullable<RoundErrorContext['sport']> | null {
- if (context.sport) return context.sport;
- const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''}`;
- const classified = classifyTraceSurface(probe, context.action ?? null);
- if (classified.sport) return classified.sport;
- if (/baseball/i.test(message)) return 'baseball';
- if (/\bgolf\b|coachhelm/i.test(message)) return 'golf';
- return null;
-}
-
function enrichTraceContext(message: string, context: RoundErrorContext): RoundErrorContext {
- const sport = inferSport(message, context);
+ // Classify once, over the same probe (route/url/action + message), so
+ // `sport` and `feature` are always derived from the same call — never
+ // from two different classifications that can disagree on sport.
+ const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''} ${message}`;
+ const classified = classifyTraceSurface(probe, context.action ?? null);
+ const sport =
+ context.sport ??
+ classified.sport ??
+ (/baseball/i.test(message) ? 'baseball' : /\bgolf\b|coachhelm/i.test(message) ? 'golf' : null);
if (!sport) return context;
- const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''} ${message}`;
- const classified = classifyTraceSurface(probe, context.action ?? null);
return {
...context,
sport,
- feature: context.feature ?? classified.feature ?? context.featureArea ?? null,
+ feature: context.feature ?? (classified.sport === sport ? classified.feature : null) ?? context.featureArea ?? null,
};
}📝 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.
Suggested change
| /** Infer sport when legacy emitters omit context.sport (most baseball actions pre-withBaseballAction). */ | |
| function inferSport(message: string, context: RoundErrorContext): NonNullable<RoundErrorContext['sport']> | null { | |
| if (context.sport) return context.sport; | |
| const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''}`; | |
| const classified = classifyTraceSurface(probe, context.action ?? null); | |
| if (classified.sport) return classified.sport; | |
| if (/baseball/i.test(message)) return 'baseball'; | |
| if (/\bgolf\b|coachhelm/i.test(message)) return 'golf'; | |
| return null; | |
| } | |
| function enrichTraceContext(message: string, context: RoundErrorContext): RoundErrorContext { | |
| const sport = inferSport(message, context); | |
| if (!sport) return context; | |
| const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''} ${message}`; | |
| const classified = classifyTraceSurface(probe, context.action ?? null); | |
| return { | |
| ...context, | |
| sport, | |
| feature: context.feature ?? classified.feature ?? context.featureArea ?? null, | |
| }; | |
| } | |
| function enrichTraceContext(message: string, context: RoundErrorContext): RoundErrorContext { | |
| // Classify once, over the same probe (route/url/action + message), so | |
| // `sport` and `feature` are always derived from the same call — never | |
| // from two different classifications that can disagree on sport. | |
| const probe = `${context.route ?? ''} ${context.url ?? ''} ${context.action ?? ''} ${message}`; | |
| const classified = classifyTraceSurface(probe, context.action ?? null); | |
| const sport = | |
| context.sport ?? | |
| classified.sport ?? | |
| (/baseball/i.test(message) ? 'baseball' : /\bgolf\b|coachhelm/i.test(message) ? 'golf' : null); | |
| if (!sport) return context; | |
| return { | |
| ...context, | |
| sport, | |
| feature: context.feature ?? (classified.sport === sport ? classified.feature : null) ?? context.featureArea ?? 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/server-error-logger.ts` around lines 151 - 172, The issue is that
inferSport() and enrichTraceContext() classify the same error using different
probes, which can produce conflicting sport and feature values. Update
server-error-logger’s inferSport() and enrichTraceContext() so classification
happens once from a single shared probe/result, then use that same
classification to populate both sport and feature. Keep the logic centered
around classifyTraceSurface() and enrichTraceContext() to ensure
admin_events.sport and admin_events.feature always stay aligned.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/actions/onboarding.ts`:
- Around line 710-713: The onboarding action in `onboarding.ts` revalidates
`/baseball` and `/baseball/player` but does not explicitly cover the new
redirect target `/baseball/player/today`. Update the `revalidatePath` calls in
the onboarding flow so the redirected page is also invalidated, using the
relevant path in the same action that returns `redirectTo:
'/baseball/player/today'`. Keep the existing `revalidatePath` usage in
`onboarding` and add the missing target so the server cache for the destination
stays fresh.
🪄 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: 07f825dc-d7bd-44dc-9050-bdc9de8862f4
📒 Files selected for processing (5)
src/app/admin/_components/__tests__/admin-nav.test.tssrc/app/admin/_components/admin-nav.tssrc/app/admin/page.tsxsrc/app/baseball/actions/__tests__/complete-player-onboarding.test.tssrc/app/baseball/actions/onboarding.ts
Comment on lines
710
to
+713
| revalidatePath('/baseball'); | ||
| revalidatePath('/baseball/player'); | ||
|
|
||
| return { success: true, redirectTo: '/baseball/dashboard' }; | ||
| return { success: true, redirectTo: '/baseball/player/today' }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
revalidatePath doesn't cover the new redirect target.
The redirect target changed to /baseball/player/today, but the revalidation calls at Line 710-711 only cover /baseball and /baseball/player (default 'page' type). Per Next.js docs: "This will not invalidate pages beneath the specific page." So /baseball/player/today itself isn't explicitly invalidated. Downstream router.refresh() in the caller currently masks this for the client that submitted onboarding, but the server-side Data Cache for that route stays stale until its own next natural revalidation.
💚 Proposed fix
revalidatePath('/baseball');
revalidatePath('/baseball/player');
+ revalidatePath('/baseball/player/today');
return { success: true, redirectTo: '/baseball/player/today' };📝 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.
Suggested change
| revalidatePath('/baseball'); | |
| revalidatePath('/baseball/player'); | |
| return { success: true, redirectTo: '/baseball/dashboard' }; | |
| return { success: true, redirectTo: '/baseball/player/today' }; | |
| revalidatePath('/baseball'); | |
| revalidatePath('/baseball/player'); | |
| revalidatePath('/baseball/player/today'); | |
| return { success: true, redirectTo: '/baseball/player/today' }; |
🤖 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/onboarding.ts` around lines 710 - 713, The
onboarding action in `onboarding.ts` revalidates `/baseball` and
`/baseball/player` but does not explicitly cover the new redirect target
`/baseball/player/today`. Update the `revalidatePath` calls in the onboarding
flow so the redirected page is also invalidated, using the relevant path in the
same action that returns `redirectTo: '/baseball/player/today'`. Keep the
existing `revalidatePath` usage in `onboarding` and add the missing target so
the server cache for the destination stays fresh.
njrini99-code
added a commit
that referenced
this pull request
Jul 4, 2026
Unified Fairway shell routing and Helm Bridge observability on one branch. Supersedes #786.
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{ success: false }/{ ok: false }/{ data: null, error }server-action returns now write toadmin_eventsviaobserveActionSoftFailure, wired intowithBaseballAction,withLiftingAction, andwithAdminObserved. Fixes the gap where toast failures during Baseball click-throughs produced zero Bridge rows.incident-feed.ts(24h window, consistent SentrylastSeenfiltering). Legacy emitters getsport: 'baseball'inference inserver-error-logger./admin/audit(404); shortcuts now use registered tabs (/admin/auth, etc.) viaADMIN_COMMAND_SHORTCUTS. Player onboarding completion lands on/baseball/player/todaydirectly instead of bouncing through/baseball/dashboard.BaseballDashboardBootstrap,router.replaceon sign-in)./admin/work), session-activity hardening.Test plan
npm run typecheck/admin/auth(not 404)/admin/errorswithsport: baseball/admin/ben-leah+/admin/workload as expected