feat(coachhelm): consolidate coach CoachHelm nav + fix player-route mis-targets - #300
Closed
njrini99-code wants to merge 2 commits into
Closed
feat(coachhelm): consolidate coach CoachHelm nav + fix player-route mis-targets#300njrini99-code wants to merge 2 commits into
njrini99-code wants to merge 2 commits into
Conversation
…is-targets
The coach CoachHelm is already tabbed — every coach surface (Brief/Signals/
Players/Effectiveness/Ask) already mounts the shared CoachHelmShell tab strip,
verified by a per-route audit. So the coach side needed only the residual
scatter removed, not a re-tabbing:
- Rail: drop the standalone "Development" item from BOTH coach rails
(GolfSidebar coachSecondaryNav + FairwayDashboardShell 'Operations'). It
duplicated the CoachHelm "Players" tab (Players → /development) — re-scattering
one CoachHelm surface into Operations next to genuine team-ops. Development
plans stay reachable via CoachHelm AI → Players tab and cmd-K "Development
Plans" (unchanged). One rail entry into CoachHelm now: "CoachHelm AI".
- Bug: two coach entry points dead-ended coaches on the PLAYER-only CoachHelm
front door (/coachhelm shows coaches a "Player Dashboard Only" state):
• cmd-K "Today's Calls" → repointed to the coach Brief /intelligence (its
description "Open the CoachHelm command center" already meant the
IntelligenceCommandCenter).
• Stats page "View all in CoachHelm" link (StatsIntelligenceStrip) → made
audience-aware: coach → /intelligence, player → /coachhelm. (Same bug
class; surfaced by the consolidation review.)
The Insights route's deliberate Signals-badge suppression (signalCount=null, a
documented anti badge-vs-tile-contradiction choice) was intentionally left as-is.
Verification: 9-agent map (all 7 coach routes mount the shell with correct
active tabs, zero duplicate mastheads) + 3-lens adversarial review (all ship,
zero blockers). typecheck clean · lint 0 errors · 2510 unit tests pass.
Out of scope (noted for follow-up): the in-page CoachHelm surface-hub grid on
the Brief page is now redundant with the tab strip (owner decision); the cmd-K
Alerts/Insights/Patterns triplet could fold into one "Signals" entry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRemoves the "Development" nav item from the coach role's sidebar and dashboard shell Operations menu, consolidates five CoachHelm command palette entries into two, and makes the ChangesCoach Navigation Restructure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84ca5887dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { name: 'Tasks', href: '/golf/dashboard/tasks', icon: IconClipboardList }, | ||
| { name: 'Recruiting HQ', href: '/golf/dashboard/recruiting', icon: IconUserPlus }, | ||
| { name: 'Development', href: '/golf/dashboard/development', icon: IconTarget }, | ||
| { name: 'Qualifiers', href: '/golf/dashboard/qualifiers', icon: IconFlag },]; |
There was a problem hiding this comment.
When NEXT_PUBLIC_REDESIGN is unset or false, GolfSidebar is the active shell and /dashboard/development renders the legacy DevelopmentPlansClient without the CoachHelm Players tab; that tab exists only inside the route's isRedesignEnabled() branch. Removing this entry therefore does not consolidate a duplicate in the supported flag-off experience—it removes the only visible navigation link to Development Plans, leaving coaches dependent on Cmd+K or a direct URL. Keep this item for the legacy sidebar and remove it only from the Fairway navigation.
Useful? React with 👍 / 👎.
Follow-up to the rail consolidation in this PR. The cmd-K palette repeated a single CoachHelm tab's member routes as separate entries, which read as scatter once the sub-nav unified them: - Merge the two entries that both pointed at /intelligence — "Today's Calls" and "CoachHelm AI" (the duplicate the Today's-Calls re-target in this PR exposed) — into ONE "CoachHelm AI" Brief entry. "today's calls" / "command center" stay as keywords so the old search terms still resolve. - Fold the Alerts / Insights / Open Patterns triplet (three routes that are all the one "Signals" tab) into ONE "CoachHelm Signals" entry → /alerts, with every member's keywords (alerts/insights/patterns/triage/mining) preserved so nothing becomes unreachable. Net: 5 coach palette entries → 2, mirroring the consolidated tabs. Distinct CoachHelm surfaces (Effectiveness/Analytics, Ask/Chat, Genome Compare, Development Plans) keep their own deep-links. NOT done — the in-page CoachHelm "surface-hub grid" is ONLY in the legacy flag-OFF Brief (IntelligenceCommandCenter); the live flag-ON Brief (FairwayBrief) has no grid. In the flag-OFF page there is no sub-nav tab strip, so that grid is its ONLY cross-surface nav — removing it would strand the fallback, not consolidate it. So there is nothing redundant to remove in the live experience. typecheck clean · lint 0 errors · 2510 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This was referenced Jun 15, 2026
Owner
Author
|
Superseded by #304, which rolls this onto a single branch (feat/coachhelm-stats-roundup) with all four PRs merged clean + a merge-interaction fix. Branch |
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
1 participant
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.
Why
Companion to the player consolidation (#299). Audit finding: the coach CoachHelm is already tabbed — every coach surface (Brief / Signals / Players / Effectiveness / Ask) already mounts the shared
CoachHelmShelltab strip. A 9-agent route map confirmed all 7 coach routes mount the shell with the correct active tab and zero duplicate mastheads. So the coach side needed the residual scatter removed, not a re-tabbing.What
1. Rail — drop the duplicate "Development" item (both coach rails):
GolfSidebar.tsx(coachSecondaryNav)Development → /golf/dashboard/developmentFairwayDashboardShell.tsx(coach 'Operations')Development → /golf/dashboard/developmentIt duplicated the CoachHelm Players tab (Players →
/development), re-scattering one CoachHelm surface into Operations next to genuine team-ops. Development plans stay reachable via CoachHelm AI → Players tab and the unchanged cmd-K "Development Plans". The coach now has one rail entry into CoachHelm: CoachHelm AI.2. Bug — two coach entry points dead-ended on the PLAYER-only front door.
/golf/dashboard/coachhelmshows coaches a "Player Dashboard Only" dead-end; the coach home is/intelligence(the Brief)./intelligence.StatsIntelligenceStrip) → made audience-aware: coach →/intelligence, player →/coachhelm.3. cmd-K palette — consolidate the CoachHelm cluster (folds the two approved follow-ups in):
/intelligence("Today's Calls" + "CoachHelm AI" — the duplicate the re-target above exposed) into one "CoachHelm AI" Brief entry./alerts.Intentionally NOT changed (verified, with reasons):
signalCount = null— a documented anti badge-vs-tile-contradiction choice ("badge 13 vs tile 8"), not scatter.IntelligenceCommandCenter); the live flag-ON Brief (FairwayBrief) has no grid. The flag-OFF page has no sub-nav tab strip, so that grid is its only cross-surface nav — removing it would strand the fallback, not consolidate it. Nothing redundant to remove in the live experience.Verification
/developmentstays reachable (Players tab + cmd-K), no test/e2e asserts the removed items,IconTargetstill used,coachhelmbadge untouched.tsc --noEmitclean ·npm run lint0 errors · 2510 unit tests pass.Reviewer note
Companion to #299 (player). Edits different lines than #299 in the two shared rail files, so they merge cleanly. Not merged — review alongside the player PR.
🤖 Generated with Claude Code
Greptile Summary
This PR removes duplicate "Development" nav entries from both coach rail implementations, consolidates five scattered CoachHelm cmd-K shortcuts into two tab-aligned entries (
CoachHelm AI/CoachHelm Signals), and fixes two dead-end links that were sending coaches to the player-only/coachhelmfront door instead of the coach home at/intelligence.GolfSidebar.tsx,FairwayDashboardShell.tsx): removes the lone "Development" item that re-exposed a CoachHelm Players-tab route as a standalone Operations item, leaving one entry into CoachHelm for coaches.CommandPalette.tsx):todays-calls,alerts,insights, andpatternsare folded into two entries whose keyword lists keep every former entry discoverable; thetodays-callsdead-end (/coachhelm) is eliminated in the process.StatsIntelligenceStrip.tsx): the "View all in CoachHelm" link now sends coaches to/intelligenceand players to/coachhelm, fixing the same dead-end class that affectedtodays-calls.Confidence Score: 5/5
All four changes are safe to merge: two are pure removals of duplicate nav entries, one is a keyword-consolidation in the command palette, and one is a targeted audience-aware routing fix with a correctly-typed prop guard.
The routing fix in StatsIntelligenceStrip is guarded by the typed audience prop, the removed nav items remain reachable via CoachHelm AI tab and cmd-K, and the palette consolidation preserves every old keyword. No logic paths are left broken or unreachable.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD subgraph before [Before PR] A1[Coach: View all in CoachHelm] --> B1[dead-end at coachhelm] A2[Coach cmd-K: Todays Calls] --> B2[dead-end at coachhelm] A3[Coach cmd-K: alerts / insights / patterns] --> B3[3 scattered entries] A4[Coach rail: Development] --> B4[duplicate Players tab route] end subgraph after [After PR] C1[Coach: View all in CoachHelm] --> D1[intelligence - Coach Brief] C2[Coach cmd-K: CoachHelm AI] --> D2[intelligence - Coach Brief] C3[Coach cmd-K: CoachHelm Signals] --> D3[alerts - Signals tab] C4[Coach rail: Development removed] --> D4[CoachHelm AI Players tab] end E[Player: View all in CoachHelm] --> F[coachhelm - unchanged]Reviews (2): Last reviewed commit: "feat(coachhelm): consolidate the coach c..." | Re-trigger Greptile