docs(adr): unified athlete settings and registrations hub#402
Open
theianjones wants to merge 1 commit intomainfrom
Open
docs(adr): unified athlete settings and registrations hub#402theianjones wants to merge 1 commit intomainfrom
theianjones wants to merge 1 commit intomainfrom
Conversation
Adds ADR-0011 proposing a /compete/athlete/settings/* shell with shared panels (profile, security, sessions, teams) and a dedicated /compete/athlete/competitions page, so compete athletes stop being bounced to /_protected/settings/* to manage their account. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughAdded Architecture Decision Record (ADR-0011) proposing a unified athlete settings and registrations hub, including routing restructuring, new layout components, shared UI panels, navigation changes with a dropdown menu, and optional form refactoring—without implementation code. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
docs/adr/0011-unified-athlete-settings-and-registrations-hub.md (5)
104-104: Add language specification to fenced code block.The fenced code block on line 104 should specify a language for better rendering and accessibility. Since this is a route tree structure, consider using
textortxt:-``` +```text /compete/athlete/ (existing: profile view)As per static analysis hint from markdownlint-cli2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` at line 104, The fenced code block containing the route tree line "/compete/athlete/ (existing: profile view)" is missing a language tag; update the opening fence to include a language (e.g., change ``` to ```text or ```txt) so the block is rendered/accessibility-friendly and satisfies markdownlint-cli2 checks.
216-216: Provide clearer guidance on server function implementation.The "(or extend existing
getAthleteProfileDataFnoutput)" alternative is vague. The implementation team should have clearer direction on whether to:
- Create a dedicated
getAthleteRegistrationsFnthat returns only registration data with status discriminators, OR- Extend
getAthleteProfileDataFnto return enhanced registration dataConsider specifying which approach is preferred and why, or note that this decision can be deferred to implementation with specific criteria for choosing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` at line 216, The ADR currently gives a vague alternative; pick and state a preferred approach: either add a dedicated server function getAthleteRegistrationsFn that returns only registration records with explicit status discriminators, or extend getAthleteProfileDataFn to include an enhanced registrations field; update the ADR to declare the preferred choice and, if deferring the decision, provide clear selection criteria (e.g., use getAthleteRegistrationsFn when registrations are used independently by multiple clients or require different caching/permissions, otherwise extend getAthleteProfileDataFn when registrations are always fetched with full profile) and include examples of the expected output shape (status discriminators) and where to implement (function names getAthleteRegistrationsFn / getAthleteProfileDataFn) so implementers know which function to change and what payload to return.
111-116: Clarify route tree notation for nested paths.The route tree notation is confusing. Line 112 shows
└── /settingsas a child of/compete/athlete/settings(line 111), which would imply the full path is/compete/athlete/settings/settings.If the parent route is
/compete/athlete/settings, the children should be shown as:
└── /(index, redirects to./profile)└── /profile└── /security└── /sessions└── /teamsOr use full paths without nesting for clarity.
📝 Proposed fix for route tree notation
/compete/athlete/settings (NEW: shell with CompeteAthleteSettingsSidebar) - └── /settings (NEW index: redirect to ./settings/profile) - └── /settings/profile (NEW: account identity — name, avatar, email) - └── /settings/security (NEW: passkeys) - └── /settings/sessions (NEW: active sessions) - └── /settings/teams (NEW: memberships — reuses getUserTeamsFn) + └── / (NEW index: redirect to ./profile) + └── /profile (NEW: account identity — name, avatar, email) + └── /security (NEW: passkeys) + └── /sessions (NEW: active sessions) + └── /teams (NEW: memberships — reuses getUserTeamsFn)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` around lines 111 - 116, The route tree under /compete/athlete/settings is misleading because children are shown as full names (e.g., "└── /settings") which reads as /compete/athlete/settings/settings; update the notation so children are relative to the parent: list the index as "└── /" (with a note that it redirects to ./profile) and the rest as "└── /profile", "└── /security", "└── /sessions", "└── /teams" — or alternatively replace all entries with full absolute paths (e.g., /compete/athlete/settings/profile) for clarity; apply this change to the route tree block in the ADR (document containing /compete/athlete/settings, CompeteAthleteSettingsSidebar, and the child entries) so the intent is unambiguous.
228-228: Clarify panel mutation pattern.The guidance states "panels consume via props — no direct
useServerFninside panels," but the existing profile settings component (see context snippet 1) usesuseServerFn(updateUserProfileFn)directly for mutations.Please clarify whether:
- Panels should receive both data AND mutation callbacks as props (fully controlled), OR
- Panels can use
useServerFnfor mutations but must receive initial data via props (current pattern)Line 132 says "loaders stay on the route" which suggests the restriction is only about data loading, but line 228's wording is broader. This distinction is important for implementation consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` at line 228, Clarify the panel mutation pattern by stating that route loaders must supply data and panels should be fully controlled: routes pass both initial data and mutation callbacks as props (do not call useServerFn inside panels); update the ADR wording around "Route loaders return shaped data; panels consume via props" and replace the broader sentence on line 228 with an explicit rule that panels receive mutation handlers (e.g., the callback that currently wraps updateUserProfileFn) from the route, and remove the implication that useServerFn may be used inside panel components so the profile settings example (which currently calls useServerFn(updateUserProfileFn)) is brought into compliance by moving that hook to the route and passing its handler into the panel.
168-168: Consider potential user confusion between "Profile" and "Athlete Profile".The sidebar will include both:
- "Profile" →
/compete/athlete/settings/profile(account identity: name, avatar)- "Athlete Profile" →
/compete/athlete/edit(competition fields: benchmarks, PRs, affiliate)Users may find it confusing to have two items with "Profile" in the name serving different purposes. Consider:
- Renaming "Athlete Profile" to "Competition Profile" or "Athletic Details"
- Adding descriptions/icons to differentiate them
- User testing the navigation terminology
This is a UX consideration that may warrant attention during implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` at line 168, The sidebar currently shows two confusing entries "Profile" and "Athlete Profile" (links: /compete/athlete/settings/profile and /compete/athlete/edit); update the label for the second item to a clearer name (e.g., "Competition Profile" or "Athletic Details") and/or add a short descriptor/icon to distinguish purposes, then update any references in docs/ADR and UI text/translation keys that mention "Athlete Profile" (search for the literal "Athlete Profile" and the route /compete/athlete/edit) so the sidebar, routes, and user-facing strings are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md`:
- Around line 200-208: The ADR currently labels the "Edit Form Split" as
optional but the verification checklist still requires removing
firstName/lastName/avatar from /compete/athlete/edit, causing ambiguity; fix by
either (A) making the split mandatory (remove the "optional" qualifier in the
section header and keep the checklist item), or (B) keeping the section optional
and moving the checklist item about `/compete/athlete/edit` inputs into a new
"Optional verification" subsection, or (C) explicitly mark checklist entries as
"Mandatory" or "Optional" and label the item about removing
firstName/lastName/avatar accordingly; update references to the routes
`/compete/athlete/edit` and `/compete/athlete/settings/profile` and the server
functions updateUserProfileFn/updateAthleteExtendedProfileFn so the intent is
unambiguous.
- Line 164: Update the redirect in src/routes/compete/athlete/settings/index.tsx
to use the relative path './profile' (relative to /compete/athlete/settings) and
confirm the route tree entry that previously showed './settings/profile' (line
referenced as route tree entry) is corrected to './profile' so both the
redirection code and the route tree notation match the intended route structure.
---
Nitpick comments:
In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md`:
- Line 104: The fenced code block containing the route tree line
"/compete/athlete/ (existing: profile view)" is missing a
language tag; update the opening fence to include a language (e.g., change ```
to ```text or ```txt) so the block is rendered/accessibility-friendly and
satisfies markdownlint-cli2 checks.
- Line 216: The ADR currently gives a vague alternative; pick and state a
preferred approach: either add a dedicated server function
getAthleteRegistrationsFn that returns only registration records with explicit
status discriminators, or extend getAthleteProfileDataFn to include an enhanced
registrations field; update the ADR to declare the preferred choice and, if
deferring the decision, provide clear selection criteria (e.g., use
getAthleteRegistrationsFn when registrations are used independently by multiple
clients or require different caching/permissions, otherwise extend
getAthleteProfileDataFn when registrations are always fetched with full profile)
and include examples of the expected output shape (status discriminators) and
where to implement (function names getAthleteRegistrationsFn /
getAthleteProfileDataFn) so implementers know which function to change and what
payload to return.
- Around line 111-116: The route tree under /compete/athlete/settings is
misleading because children are shown as full names (e.g., "└── /settings")
which reads as /compete/athlete/settings/settings; update the notation so
children are relative to the parent: list the index as "└── /" (with a note that
it redirects to ./profile) and the rest as "└── /profile", "└── /security", "└──
/sessions", "└── /teams" — or alternatively replace all entries with full
absolute paths (e.g., /compete/athlete/settings/profile) for clarity; apply this
change to the route tree block in the ADR (document containing
/compete/athlete/settings, CompeteAthleteSettingsSidebar, and the child entries)
so the intent is unambiguous.
- Line 228: Clarify the panel mutation pattern by stating that route loaders
must supply data and panels should be fully controlled: routes pass both initial
data and mutation callbacks as props (do not call useServerFn inside panels);
update the ADR wording around "Route loaders return shaped data; panels consume
via props" and replace the broader sentence on line 228 with an explicit rule
that panels receive mutation handlers (e.g., the callback that currently wraps
updateUserProfileFn) from the route, and remove the implication that useServerFn
may be used inside panel components so the profile settings example (which
currently calls useServerFn(updateUserProfileFn)) is brought into compliance by
moving that hook to the route and passing its handler into the panel.
- Line 168: The sidebar currently shows two confusing entries "Profile" and
"Athlete Profile" (links: /compete/athlete/settings/profile and
/compete/athlete/edit); update the label for the second item to a clearer name
(e.g., "Competition Profile" or "Athletic Details") and/or add a short
descriptor/icon to distinguish purposes, then update any references in docs/ADR
and UI text/translation keys that mention "Athlete Profile" (search for the
literal "Athlete Profile" and the route /compete/athlete/edit) so the sidebar,
routes, and user-facing strings are consistent.
🪄 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: CHILL
Plan: Pro
Run ID: 5cb0b73f-67db-4081-8146-26a85b93b919
📒 Files selected for processing (1)
docs/adr/0011-unified-athlete-settings-and-registrations-hub.md
| } | ||
| ``` | ||
|
|
||
| **`src/routes/compete/athlete/settings/index.tsx`** — redirect to `./profile`. |
Contributor
There was a problem hiding this comment.
Verify redirect path matches corrected route structure.
This line correctly states the redirect should be to ./profile, but it conflicts with line 112 in the route tree which shows ./settings/profile. After fixing the route tree notation issue, ensure this remains ./profile (relative to /compete/athlete/settings).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` at line 164,
Update the redirect in src/routes/compete/athlete/settings/index.tsx to use the
relative path './profile' (relative to /compete/athlete/settings) and confirm
the route tree entry that previously showed './settings/profile' (line
referenced as route tree entry) is corrected to './profile' so both the
redirection code and the route tree notation match the intended route structure.
Comment on lines
+200
to
+208
| ### Edit Form Split (optional in this ADR, recommended) | ||
|
|
||
| Move the account-identity inputs (`firstName`, `lastName`, avatar URL) *out* of `/compete/athlete/edit` and into the new profile panel at `/compete/athlete/settings/profile`. The athlete-competition form at `/compete/athlete/edit` then scopes down to: gender, DOB, affiliate, height/weight, benchmarks/PRs, social, cover image. | ||
|
|
||
| Both forms submit to their existing server functions: | ||
|
|
||
| - `updateUserProfileFn({ firstName, lastName, avatar })` | ||
| - `updateAthleteExtendedProfileFn({ ...AthleteProfileFormValues })` | ||
|
|
Contributor
There was a problem hiding this comment.
Resolve inconsistency between optional feature and verification requirements.
The section header marks the edit form split as "optional in this ADR, recommended," but the verification checklist (line 255) includes it as a requirement: "Athlete edit page at /compete/athlete/edit no longer contains firstName, lastName, or avatar inputs."
Either:
- Make the edit form split mandatory (remove "optional" qualifier), OR
- Move line 255 to a separate "Optional verification" section, OR
- Clarify that the verification checklist includes both mandatory and optional items with clear markers
This ambiguity could lead to confusion during implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/0011-unified-athlete-settings-and-registrations-hub.md` around lines
200 - 208, The ADR currently labels the "Edit Form Split" as optional but the
verification checklist still requires removing firstName/lastName/avatar from
/compete/athlete/edit, causing ambiguity; fix by either (A) making the split
mandatory (remove the "optional" qualifier in the section header and keep the
checklist item), or (B) keeping the section optional and moving the checklist
item about `/compete/athlete/edit` inputs into a new "Optional verification"
subsection, or (C) explicitly mark checklist entries as "Mandatory" or
"Optional" and label the item about removing firstName/lastName/avatar
accordingly; update references to the routes `/compete/athlete/edit` and
`/compete/athlete/settings/profile` and the server functions
updateUserProfileFn/updateAthleteExtendedProfileFn so the intent is unambiguous.
🚀 Preview DeployedURL: https://wodsmith-app-pr-402.zacjones93.workers.dev
This comment is automatically updated on each push to this PR. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/adr/0011-unified-athlete-settings-and-registrations-hub.md">
<violation number="1" location="docs/adr/0011-unified-athlete-settings-and-registrations-hub.md:112">
P2: The nested route tree uses `settings/...` child paths under `/compete/athlete/settings`, which documents an incorrect URL shape (`.../settings/settings/...`).</violation>
<violation number="2" location="docs/adr/0011-unified-athlete-settings-and-registrations-hub.md:200">
P3: The edit form split is marked as "optional" here, but the verification checklist (line 255) includes it as an unconditional requirement. Either remove the "optional" qualifier to make it mandatory, or mark the corresponding verification item as optional to keep the ADR internally consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /compete/athlete/sponsors (existing) | ||
| /compete/athlete/competitions (NEW: my registrations, filterable) | ||
| /compete/athlete/settings (NEW: shell with CompeteAthleteSettingsSidebar) | ||
| └── /settings (NEW index: redirect to ./settings/profile) |
Contributor
There was a problem hiding this comment.
P2: The nested route tree uses settings/... child paths under /compete/athlete/settings, which documents an incorrect URL shape (.../settings/settings/...).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/adr/0011-unified-athlete-settings-and-registrations-hub.md, line 112:
<comment>The nested route tree uses `settings/...` child paths under `/compete/athlete/settings`, which documents an incorrect URL shape (`.../settings/settings/...`).</comment>
<file context>
@@ -0,0 +1,264 @@
+/compete/athlete/sponsors (existing)
+/compete/athlete/competitions (NEW: my registrations, filterable)
+/compete/athlete/settings (NEW: shell with CompeteAthleteSettingsSidebar)
+ └── /settings (NEW index: redirect to ./settings/profile)
+ └── /settings/profile (NEW: account identity — name, avatar, email)
+ └── /settings/security (NEW: passkeys)
</file context>
|
|
||
| **`src/components/compete-mobile-nav.tsx`** — mirror the same entries. | ||
|
|
||
| ### Edit Form Split (optional in this ADR, recommended) |
Contributor
There was a problem hiding this comment.
P3: The edit form split is marked as "optional" here, but the verification checklist (line 255) includes it as an unconditional requirement. Either remove the "optional" qualifier to make it mandatory, or mark the corresponding verification item as optional to keep the ADR internally consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/adr/0011-unified-athlete-settings-and-registrations-hub.md, line 200:
<comment>The edit form split is marked as "optional" here, but the verification checklist (line 255) includes it as an unconditional requirement. Either remove the "optional" qualifier to make it mandatory, or mark the corresponding verification item as optional to keep the ADR internally consistent.</comment>
<file context>
@@ -0,0 +1,264 @@
+
+**`src/components/compete-mobile-nav.tsx`** — mirror the same entries.
+
+### Edit Form Split (optional in this ADR, recommended)
+
+Move the account-identity inputs (`firstName`, `lastName`, avatar URL) *out* of `/compete/athlete/edit` and into the new profile panel at `/compete/athlete/settings/profile`. The athlete-competition form at `/compete/athlete/edit` then scopes down to: gender, DOB, affiliate, height/weight, benchmarks/PRs, social, cover image.
</file context>
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.
Summary
/compete/athlete/settings/*shell so compete athletes stop bouncing to/_protected/settings/*./compete/athlete/competitionsregistrations list (filterable upcoming/past) replacing the buriedCompetitiveHistorycard./compete/athlete/editso account identity (name/avatar) lives in the settings profile panel and the edit page keeps only athlete-competition fields (benchmarks, PRs, affiliate)./_protected/settings/*is preserved for programming / team-admin users.Status: proposed — no code changes in this PR, review the decision before implementation.
Test plan
docs/adr/0011-unified-athlete-settings-and-registrations-hub.md/settings/*retirement, no password rework)🤖 Generated with Claude Code
Summary by cubic
Proposes ADR-0011 to unify athlete account management inside the compete shell and add a dedicated registrations page, so athletes stop bouncing to
/_protected/settings/*.New Features
\compete\athlete\settings\*shell that reuses shared panels (Profile, Security, Sessions, Teams) — single backend, two shells.\compete\athlete\competitionspage listing registrations with upcoming/past filters.\compete\athlete\edit: identity (name/avatar) moves to\compete\athlete\settings\profile; edit page keeps athlete-competition fields.\_protected\settings\*for programming/team-admin users.Migration
docs/adr/0011-unified-athlete-settings-and-registrations-hub.md; implementation will follow in separate PRs.Written for commit 0daac98. Summary will update on new commits.
Summary by CodeRabbit