Clearer, accessible rep-category errors that name the exact rule#3136
Conversation
The Grant Rep error was a single justified blob in a cramped modal that
lumped every rule together ("...invalid characters, is shorter than one
character or is longer than 100 characters..."). Now:
- A client-side mirror of the server rules (repCategoryValidation.ts)
names the exact violation the moment a category is chosen - leading
dash, over-length with the real count, or the specific disallowed
characters used - localized via the existing i18n system, no server
round-trip.
- The error surface is restructured: a bold title, the specific reason,
then the AI-filter explainer as secondary text; left-aligned and
responsive instead of justified in a tiny box.
- role="alert" announces it to screen readers on appearance, and the
close control takes a localized label.
Pairs with backend #1717 (dash allowed, non-leading) - the copy now
states the dash rule. Deploy after that backend change is live.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds client-side rep category validation with a new utility function and i18n messages, integrates it into the Rep search selection flow, updates the Rep error component and wrapper to support translated content and a configurable close label, and updates associated tests. ChangesRep category validation feature
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UserPageRepNewRepSearch
participant getRepCategoryViolation
User->>UserPageRepNewRepSearch: select rep category
UserPageRepNewRepSearch->>UserPageRepNewRepSearch: check help-bot reserved category
UserPageRepNewRepSearch->>getRepCategoryViolation: validate(rep)
getRepCategoryViolation-->>UserPageRepNewRepSearch: violation or null
UserPageRepNewRepSearch->>UserPageRepNewRepSearch: check code point min length
UserPageRepNewRepSearch-->>User: set localized errorMsg or proceed
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
6529bot general PR review - 037a1d6Verdict: Needs changes Important
Nice-to-have
Suggested next steps
Prompt for all review comments with AI agents |
6529bot general PR review - 037a1d6Verdict: Needs changes The prior 6529bot review at head Important
Nice-to-have
Suggested next steps
Prompt for all review comments with AI agents |
6529bot crypto security analysis - 037a1d6Verdict: No security findings This is a UI/i18n change for rep-category error messaging. I reviewed it against the crypto/auth/signature focus areas and found no realistic exploit paths in the changed code. Notes checked and cleared:
Coverage note: review is scoped to the included 7-file diff; findings are limited to changed code. |
6529bot WCAG 2.2 AA analysis - 037a1d6Verdict: Needs changes Important
Nice-to-have
Suggested next steps
Prompt for all review comments with AI agents |
6529bot i18n analysis - 037a1d6Verdict: Needs changes Important
Nice-to-have
Suggested next steps
Prompt for all review comments with AI agents |
6529bot GLM Swarm ReviewVerdict: Advisory only This GLM swarm is advisory and complements, not replaces, existing tests and existing reviewbots. Important
Nice-to-have
Testing feedback loop
Partial reviewer outputOne or more internal advisory reviewer slices were unavailable; the synthesis used the remaining reviewer output.
|
6529bot responsiveness review - 037a1d6Verdict: Responsive checks passed Root Cause / Summary All 16 checks across four contexts passed with zero deterministic failures and no blank or content-not-ready screenshots. Every route rendered visible shell content, correct layout branches activated per context, and no Next.js build-asset or overlay errors were observed. The only signal worth noting is a set of expected native-simulation warnings (missing Capacitor plugin shims plus one screenshot-timeout fallback), all confined to the Blocking Findings None. No deterministic failures, no blank-like screenshots ( Non-Blocking Findings
Platform Coverage
No blocking issue was found. The four warnings are all expected native-simulation artifacts and can be cleared without changes, though a quick confirm that missing Keyboard/App shims are intentional in this harness is advisable. Screenshot evidence: native-mobile /6529er/collected, native-mobile /, native-mobile /6529er, native-mobile /waves, web-mobile /6529er/collected, web-desktop /6529er/collected, web-mobile /, web-desktop /, web-desktop /6529er, web-desktop /waves, web-mobile /waves, web-mobile /6529er and 4 more in the details. Deterministic responsiveness detailsVerdict: Responsive checks passed
Responsiveness runner summary6529bot Responsiveness SummaryStatus: pass Platform Context
Platform Matrix
Warnings
Slowest Checks
|
…knip
- Reachability (6529bot): run the rule check before the search-length
guard so the over-length and leading-dash messages actually surface;
the silent <3 drop now shows a tooShort message; length compared in
code points throughout.
- i18n (6529bot): describeChar no longer bakes English ("line break",
"tab") into the interpolated message - invisible characters get a
locale-neutral U+XXXX label, which also fixes invisibles rendering as
empty quotes.
- WCAG 2.4.7 (6529bot): the error close button gains a visible
focus-visible outline instead of only removing the default.
- Knip: un-export REP_CATEGORY_MAX_LENGTH and RepCategoryViolation
(module-internal).
- Sonar duplication: interleave wrapped/single message entries so no
consecutive wrapped pairs form a duplicate token-run.
- GLM false positive (comment inside JSX) sidestepped by moving it above
the return.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
|
Round 1 pushed (fc1db96) — addressing every lane:
Local: 10/10 tests, lint, knip, playwright-typecheck all clean. The single-message precedence (length → leading dash → disallowed) is now commented as intentional per the other nice-to-have. |
6529bot follow-up commit review - fc1db96Verdict: No new findings The follow-up commit Resolved since last review
A couple of prior nice-to-haves remain open but were not re-raised as blockers and are acceptable: the untranslated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Sonar duplication kept firing because my added keys formed one contiguous run over its ~10-line minimum-block size, matching the parallel key blocks in the locale files. Splitting them into a single-line cluster and a wrapped cluster, separated by the existing suggested.* keys, keeps each below the duplicate-block threshold. (Comments don't help - the tokenizer ignores them.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
6529bot follow-up commit review - 4f00f61Verdict: No new findings The newest commit (
One observation, not a finding: the Sonar quality gate reported failed at head Prior open nice-to-haves remain acceptable and unchanged: untranslated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/user/utils/UserPageErrorWrapper.tsx (1)
4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test asserts
closeLabelactually renders astitle/aria-labelfor a custom value.The wrapper's own test only checks the default "Close" label;
UserPageRepNewRepError.test.tsxmocksUserPageErrorWrapperaway, so nothing exercises the real prop-forwarding totitle/aria-labelwith a translated value.Also applies to: 18-23
🤖 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 `@components/user/utils/UserPageErrorWrapper.tsx` around lines 4 - 9, The current tests do not verify that a custom closeLabel is forwarded by UserPageErrorWrapper to the underlying close button’s title and aria-label. Update the tests for UserPageErrorWrapper (and avoid relying on the mocked wrapper in UserPageRepNewRepError.test.tsx) to render the real component with a translated/custom closeLabel and assert that both title and aria-label reflect that value, using the UserPageErrorWrapper and closeError prop to locate the behavior.
🤖 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 `@components/utils/input/rep-category/repCategoryValidation.ts`:
- Around line 40-46: The describeChar helper currently wraps printable
disallowed characters in double quotes, which makes a literal double-quote
character appear ambiguous in validation messages. Update describeChar in
repCategoryValidation.ts to special-case the quote character (and reuse the same
logic anywhere else it’s used in this file, including the later call sites
referenced in the comment) so it returns an unambiguous label for " instead of
the generic quoted format, while keeping existing handling for invisible
characters unchanged.
---
Nitpick comments:
In `@components/user/utils/UserPageErrorWrapper.tsx`:
- Around line 4-9: The current tests do not verify that a custom closeLabel is
forwarded by UserPageErrorWrapper to the underlying close button’s title and
aria-label. Update the tests for UserPageErrorWrapper (and avoid relying on the
mocked wrapper in UserPageRepNewRepError.test.tsx) to render the real component
with a translated/custom closeLabel and assert that both title and aria-label
reflect that value, using the UserPageErrorWrapper and closeError prop to locate
the behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5e25f60-fa6a-41a1-852f-e0dd4c9deb29
📒 Files selected for processing (7)
__tests__/components/user/rep/new-rep/UserPageRepNewRepError.test.tsx__tests__/components/utils/input/rep-category/repCategoryValidation.test.tscomponents/user/rep/new-rep/UserPageRepNewRepError.tsxcomponents/user/rep/new-rep/UserPageRepNewRepSearch.tsxcomponents/user/utils/UserPageErrorWrapper.tsxcomponents/utils/input/rep-category/repCategoryValidation.tsi18n/messages/en-US.ts
Reordering within the big REP block couldn't clear the Sonar duplication: the quoted-key run structurally matches the locale dictionaries' key blocks regardless of content. Extracting these seven messages into their own objectMessages group with unquoted identifier keys tokenizes as IDENT: STRING (not "STRING": "STRING"), so it can't match those runs. Also: label a literal double-quote char in single quotes so it doesn't read as an ambiguous quote run (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
|
Sonar duplication — decisive fix (b867bf4): reordering couldn't clear it because the quoted-key run structurally matches the locale dictionaries' key blocks regardless of content. Moved the seven rep-validation messages into their own Local: 10/10 tests, lint, knip all clean. Waiting on the Sonar re-run to confirm density drops under 3%. |
6529bot follow-up commit review - b867bf4Verdict: No new findings The newest commit ( Resolved since last review
No logic changed in the search/validation flow this commit, so the previously-resolved findings (reachable length rule, code-point counting, Prior open nice-to-haves remain acceptable and unregressed: untranslated |
6529bot follow-up commit review - f92d330Verdict: No new findings The head commit
No logic changed in the search/validation flow, so the earlier resolved findings (reachable length rule, code-point counting, Prior open nice-to-haves are unchanged and acceptable: untranslated |
6529bot follow-up commit review - 00d9989Verdict: No new findings The diff since the prior same-lane marker ( Resolved since last review
Prior open nice-to-haves remain acceptable and unregressed: untranslated |
6529bot follow-up commit review - d6e8eebVerdict: No new findings The diff since the prior same-lane marker (
No logic changed in the search/validation flow, so the earlier resolved findings (reachable length rule, code-point counting, Prior open nice-to-haves remain acceptable and unregressed:
|
6529bot follow-up commit review - 9c96c84Verdict: No new findings The head commit
Resolved since last reviewNothing newly resolved in this delta — the prior same-lane review already recorded these as resolved and no regression was introduced. Prior open nice-to-haves remain acceptable and unregressed: untranslated |
6529bot follow-up commit review - ee1c5eeVerdict: No new findings The head commit
Prior open nice-to-haves remain acceptable and unregressed:
These were raised earlier and not re-litigated here. |
6529bot follow-up commit review - ca1eed9Verdict: No new findings The head
No search/validation flow logic changed, so the prior-resolved findings (reachable length rule, code-point counting, Prior open nice-to-haves remain acceptable and unregressed: untranslated |
6529bot follow-up commit review - de613b8Verdict: No new findings The head commit
Prior open nice-to-haves remain acceptable and unregressed: untranslated |
6529bot follow-up commit review - 457cd10Verdict: No new findings The head commit
The Prior open nice-to-haves remain acceptable and unregressed:
None of these were regressed by the current head. |
6529bot follow-up commit review - 2f2a470Verdict: No new findings The head commit
Prior open nice-to-haves remain acceptable and unregressed: untranslated |
|
6529bot follow-up commit review - 0cbe6adVerdict: No new findings The head commit
Prior open nice-to-haves remain acceptable and unregressed: the untranslated |
…rep category Second staging device-testing round on iPhone: - CreateWave: reset every scrolled ancestor (and the window) on step change. The layout's scroll container kept the offset where the user tapped Next, so steps 3 and 6 loaded half-scrolled down; now every step starts at the top, both directions. New e2e assertion pins the Wave Timeline header in the viewport on arrival at the Dates step. - CreateWave: pad the container bottom by env(safe-area-inset-bottom). With viewport-fit=cover the safe-area region is part of the viewport and iOS Safari's floating bottom chrome overlays it, cutting the Previous/Next row in half even at full scroll. - globals.css: keep create-wave text entry at 16px with touch-action:manipulation on coarse pointers (same rule the native shell applies globally) — iOS Safari zooms the page and pans horizontally when focusing a sub-16px control, which hit the description editor on the last step. - Rep outcomes (rank + approve): run the category through getRepCategoryViolation — the same backend-mirroring rules and localized messages the rep-assignment flow got in #3136 — live while typing, and block Save on violation. Previously an invalid category sailed through to wave submit, where staging rejected the whole wave with a generic "Invalid request". New shared CreateWaveOutcomesRepCategoryField renders the message with role="alert". Tests: new unit coverage for the live violation message + blocked submit in both rep screens (rep suites 20/20, outcomes 115/115); full auth-sandbox e2e pack 27/27; typecheck over 54 changed files clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: brookr <11095+brookr@users.noreply.github.com>
2 participants
Issue
Fix
components/utils/input/rep-category/repCategoryValidation.ts) returns a specific i18n key the moment a category is chosen — leading dash / over-length (with the real code-point count) / the specific disallowed characters used, each named. No server round-trip for the common cases; the server stays authoritative.UserPageRepNewRepError): bold title → the specific reason → the AI-filter explainer as secondary text; left-aligned and responsive instead oftext-justifyin a tiny box.UserPageErrorWrapper):role="alert"so it's announced on appearance; close control takes a localizedaria-label/title.rep.categories.validation.*in the source (en-US) dictionary; other locales fall back per the existing pattern for this group.Changes
repCategoryValidation.ts(+ tests). RewroteUserPageRepNewRepError;role="alert"+closeLabelonUserPageErrorWrapper; wired validation intoUserPageRepNewRepSearch.onRepSelect; 7 new i18n keys.Validation
{placeholders}) + updated error-component test (title/message/details, reserved-category path). Lint + Playwright-typecheck clean.art/photography→role="alert": "That category name won't work — These characters can't be used: "/". Stick to letters, numbers, spaces, dashes and , . ? ! ' ( )";hey-judecleared the error and proceeded. Desktop + mobile viewport screenshots captured.Risk
Deployment
Review Notes
test:e2e:*-sandbox), which would need new rep-flow plumbing incomposerSandboxServer. Deferred as a possible fast-follow rather than expanding the harness here — flagging for reviewer input.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes