Part2 of design rebuild - DO NOT MERGE YET - #7087
Draft
giacoelho wants to merge 8 commits into
Draft
Conversation
…7051 Starting point for the part-2 tracking PR. Five items, each verified still open against main rather than copied forward from notes: - the 3 xit'd flaky case-contact system specs, which need a tracking issue before they can be re-enabled - two view specs still raising SystemStackError from the settings rail calling edit_casa_org_path(current_organization) unstubbed (pre-existing, still red) - sort still living on the filterrific form, which is why Clear filters has to carry sorted_by by hand - mailers naming the chapter inconsistently (@casa_organization vs @casa_org vs nothing on Devise) - contact types on the case-contact form staying an exposed checkbox fieldset, with the reason it is deliberate Dropped one item while checking: the read-notification icon is already slate-500, so its contrast was fixed in the merged work and is not outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
giacoelho
requested review from
FireLemons,
compwron and
elasticspoon
as code owners
July 29, 2026 02:20
giacoelho
marked this pull request as draft
July 29, 2026 02:20
Editing a case contact could silently lose work. Autosave was bound per field, to notes, topic answers and expense descriptions only, so an edit to duration or medium was dropped on navigation -- UNLESS the user also happened to touch one of those three, because an autosave posts the entire form and therefore committed everything. Whether your work persisted depended on which field you touched last, with no feedback either way. Measured before: edit the duration, wait past the debounce, click Back -> old value; edit the duration then type one character in notes -> both saved. Bound once on the <form> instead: `input` bubbles and fires for every control type, so text, number, date, select, checkbox and radio all save, and a field added later cannot be forgotten. The three per-field triggers are now redundant and removed. Verified each control type saves on its own and survives leaving: duration 60 -> 210 with nothing else touched and still 210 after Back; a radio choice; a contact- type checkbox. Also verified checking a topic -- which now triggers an autosave too -- still yields exactly one ContactTopicAnswer, i.e. it does not race the create that stores the answer's id. This is why the form has no Cancel and no unsaved-changes warning: with everything autosaved there is nothing unsaved to warn about. The two coherent models are full autosave (Google Docs / Notion: navigation is the exit) and explicit save (GitHub / Jira: Cancel plus a dirty guard). A partly-autosaving form was neither. One measurement trap, documented: the "Saved!" alert lingers ~3s, so a second interaction in one example matches the previous save's alert and reads the DB before its own debounce elapses -- which made checkboxes look broken when they were not. The new specs do one interaction each and wait on the alert. case_contacts 148 examples 0 failures; the 3 new examples pass 3/3 repeat runs. standardrb + erb_lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
🧪 Tests
…e org
`MileageRatesController` authorized the CasaAdmin *class* for new/create/edit/
update. Two problems:
- `GET /mileage_rates/:id/edit` raised NoMethodError: undefined method
'casa_org' for class CasaAdmin. Pundit resolved the class to
CasaAdminPolicy, whose #edit? is the one method there that reaches
record.casa_org -- fine for a CasaAdmin instance, fatal for the class. The
page 500'd for every admin in every org. #update survived only because
CasaAdminPolicy aliases update? to index? (is_admin?, never touches the
record).
- Authorizing a class skipped the org check, and `set_mileage_rate` found by
id unscoped, so an admin could edit another org's rate.
Authorize the record against a new MileageRatePolicy (is_admin_same_org?) and
scope the finder to current_organization. Behaviour is otherwise unchanged.
The edit action had no request spec -- only a system spec asserting the "Edit"
link's text, never following it -- which is why this shipped. Adds GET /edit
coverage plus cross-org rejection for edit and update, and puts the update
block's rate in the admin's org (those examples were passing *because* of the
missing org check).
Found while running a whole-app WCAG audit: the page's axe run reported
document-title/html-has-lang/landmark-one-main/region, which is the signature
of a layout-less Rails error page rather than an accessibility defect. Behind
the crash the page audits clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An axe-core (WCAG 2.1 A/AA) sweep of all 80 pages across the four roles found violations on 12 admin pages; the volunteer, all-casa and signed-out surfaces were already clean. Fixes, by rule: heading-order (7 pages) -- an <h6> was used as the subtitle directly under the page <h1>, skipping h2-h5, in court_dates/_form, placements/_form and bulk_court_dates/new; those are captions, so they are now <p>. In court_dates/show the fact labels were <dt><h6>Judge:</h6></dt>: a <dt> is already the term, so the nested heading is gone and "Court orders:" -- a real section heading -- is an <h2>. case_contacts/_case_contact now takes a heading_level local (default 3): #index nests cards under an <h2> case-number section, #drafts has no grouping level and passes 2. label (critical) -- the three import file inputs had no accessible name. Their ids are overridden for the import JS, so the <label for> points at the rendered id, per the pattern already documented in design.md. aria-input-field-name + scrollable-region-focusable -- <trix-editor role="textbox"> on the banner form. label/for only associates with form-associated elements, so the correct-looking visible <label> never named the custom element; it now carries aria-label. Trix also ships its toolbar row as `overflow-x: auto` with nothing focusable, so tailwind.css lets the row wrap instead of scroll (also fixes buttons hiding behind a scrollbar-less scroll on narrow screens). link-in-text-block -- the case-number link inside the placements <h1> was brand-600 on slate-900, 2.83:1 against the surrounding text where 3:1 is required, with no other cue. Underlined. color-contrast -- emerald-600 is 3.67:1 on white: fine for a decorative icon (3:1) but short of 4.5:1 for the 12px "+N vs last month" delta on analytics. Measured every neighbouring token from the built oklch values and recorded the table in design.md; amber-600 (3.19:1) is worse and slate-400 is 2.63:1. Moved the analytics delta pair to emerald-700/rose-700 and the org-settings contact topic chips to emerald-700/slate-500. Two of these were invisible to the first pass because axe only sees the rendered DOM and the fixtures seeded no rows: the emancipation checklist's category and option inputs had no accessible name at all, and the org-settings status chips never rendered. The checklist inputs get aria-label rather than a real label -- the JS sets checked state after an AJAX save, so an associated label would toggle natively on top of it. Extends spec/system/accessibility/axe_spec.rb to cover all 14 fixed pages plus the emancipation checklist, each seeding the rows its page needs, so these cannot regress. Ran the sweep again afterwards: 80/80 clean. Not fixed here, and pre-existing on a clean tree: the two SystemStackError view specs (mileage_rates/index, casa_admins/admins_table) and Selenium races in bulk_court_dates/new_spec and casa_cases/edit_spec, all of which fail intermittently with or without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…points
The first sweep ran at one desktop width, which cannot see this app's mobile
markup at all: axe skips hidden elements, and every list page renders a
`md:hidden` card list beside its `hidden md:block` table. Re-running the same 81
pages at 390px surfaced six violations that 1400px reported clean.
page-has-heading-one (3 auth pages) -- the only <h1> lived in
layouts/casa_auth's `<aside class="hidden … lg:flex">`, so below lg the sign-in,
password-reset and all-casa sign-in pages had no h1 whatsoever. The page's
subject is the form, not the brand line, so each form heading is now the <h1>
("Welcome back", "Reset your password", "All CASA log in", …) and the marketing
statement is a <p>. Applied across sessions, passwords, invitations and
confirmations so the auth surface is consistent.
color-contrast -- the lg:hidden org-settings group labels were slate-400
(2.63:1) at 12px.
scrollable-region-focusable -- metric_data_table and metric_heatmap wrap their
tables in `overflow-x-auto`, which becomes a real scroll container once the
table stops fitting. Both hold pure data with no links or controls, so nothing
inside could take focus and the region was unreachable by keyboard; the scroll
containers now carry tabindex="0".
Separately, /case_contacts/new_design is gated behind the
:new_case_contact_table Flipper flag, so walking the routes just followed the
redirect and never audited it. With the flag on it renders "Reached" /
"Not reached" in emerald-600 (3.65:1) and amber-600 (3.19:1) as 14px text --
both moved to -700, matching the fix already applied elsewhere.
Sweeps now clean at 390 / 768 / 1400 for all 81 pages. axe_spec.rb gains a
mobile-viewport context (it resizes, then restores) covering the auth pages,
org settings, analytics, the two index pages with mobile card lists, and the
flag-gated table, so none of this can regress from a desktop-only run.
design.md records the viewport rule, the auth heading structure, the
tabindex-on-scroll-container pattern and the flag-gated blind spot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… flaky
The example clicked "Create" and immediately visited another page. `click_on`
does not wait for the POST or its redirect, so the navigation could still be in
flight when `visit casa_case_path` fired, and whichever won decided which page
the assertions ran against. When the redirect won, the assertions ran against
the bulk form we had just left.
Its first assertion could not catch that, because `hearing_type.name` also
appears in that form's own "Hearing type" <select>: have_content matched an
<option> on the stale page, returned immediately, and only the court-order
assertion failed -- which is why the failure looked unrelated to navigation.
Measured in isolation on a clean tree, this failed 5 runs out of 10, every one
of them with the same signature: have_content(court_order_text) against a page
whose text still read "1 court date created!". So:
- wait for the create to land (the flash) before navigating away;
- anchor on the case page's <h1> before asserting its contents, so a stale
page cannot satisfy the assertions;
- scope both assertions to the elements that carry the values -- the
court-date row link and the court-orders table -- so no <option> or
unrelated copy can satisfy them.
Also stop caching a raw Selenium reference for the court-order textarea. The row
is cloned from a <template> by the court-order-form controller, so a `.native`
handle taken from `first` while that insertion settles can go stale; Capybara's
find/#set re-resolves the node and waits. The textarea already carries
aria-label="Court order text", so it has a stable locator (Capybara's aria-label
lookup is off in this suite, hence the attribute selector).
Verification, no retries: the reproducer batch
(bulk_court_dates + court_dates + placements + banners --seed 12650) failed on
every baseline run and is now clean across ~26 runs; the wider 11-directory
batch went from 1 of 3 clean to 3 of 3. In isolation, 5/10 -> 2/20, and the two
residual failures are a different, pre-existing problem: Chrome raises
"Node with given id does not belong to the document" when Capybara reads the
document mid-Turbo-swap. That is a driver-level error rather than a spec bug --
Selenium raises a non-retryable UnknownError instead of a stale-element error --
and it also hits court_dates/new_spec and placements/edit_spec. It is covered by
this repo's CI policy (spec/support/rspec_retry.rb retries :js examples 3x on
CI): under CI=true this file passed 20/20 without needing a single retry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent CDP errors
Follow-on from the bulk_court_dates fix. Every one of these was an assertion or
element read that could not wait, so a slow page swap failed the example rather
than being retried. Measured rates below are without retries (rspec-retry is
CI-only here).
court_dates/new_spec -- same cached `.native` handle as bulk_court_dates: the
court-order row is cloned from a <template> by the court-order-form controller,
so a raw Selenium reference taken from `first` while that insertion settles can
detach before send_keys runs. Uses Capybara's finder and #set instead.
placements/edit_spec -- the same defect the bulk spec had. `click_link("Edit")`
was followed by assertions on the case number, but the placement *show* page we
came from also renders the case number, so have_content("123") could be
satisfied before the navigation finished and the form interactions then raced the
page swap. The before block now anchors on the edit page's <h1>.
casa_cases/edit_spec (2 examples) -- `expect(assign_badge.text).to eq
"Unassigned"` read .text off an already-found element and compared it with a
plain Ruby ==, which never retries, while the badge updates asynchronously after
"Unassign volunteer". Now a waiting matcher with exact_text, preserving the
whole-string comparison. 2/12 runs -> 0/16.
other_duties/new_spec -- `find("#other_duty_notes").native.attribute(
"validationMessage")` sits outside Capybara's synchronize, so the
StaleElementReferenceError raised while the page settled went straight through,
even though stale-element is in Capybara's retry list. Capybara has a
:validation_message node filter for exactly this; as a matcher filter it
re-resolves the field on every attempt. Routing through Element#[] would not
work: Capybara::Selenium::Node#[] rescues WebDriverError and returns nil, so the
staleness would become a silent nil. 3/20 -> 0/20.
Adds spec/support/capybara_transient_node_retry.rb for the one failure mode that
is not a spec bug: Chrome raises
UnknownError: unhandled inspector error:
{"code":-32000,"message":"Node with given id does not belong to the document"}
when Capybara queries the page mid-Turbo-swap. It is transient and semantically
a stale element, but Selenium reports a generic UnknownError and Capybara's
retry loop matches by exception class, so it was re-raised immediately. The patch
matches that single message inside Capybara's existing synchronize loop, so it
reloads and retries until default_max_wait_time and a genuinely stuck page still
fails, just at the wait timeout. Adding UnknownError to invalid_element_errors
would have been far too broad; unit-checked that other UnknownErrors are still
re-raised.
Verification: the previously flaky files at 0/20 each; the four-directory
reproducer clean across 12 random-seed and 5 fixed-seed runs; and the full
spec/system suite (713 examples) clean 4 runs in a row, where it had been
averaging about one failure per run on a different spec each time. Untouched:
the .native read in all_casa_admins/patch_notes/index_spec, which is a static
textarea already scoped by `within` and has not been observed failing, and the
two pre-existing SystemStackError view specs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Banner (layouts/_casa_banner). The megaphone icon was text-amber-500, which is
2.07:1 against the amber-50 banner background where WCAG 1.4.11 (non-text
contrast) needs 3:1. It now inherits the container's amber-900 (8.77:1), matching
shared/_flashes where the severity icon takes the container colour rather than
setting its own. Measured in a real browser, not read off the classes.
This is the class of defect the whole-app axe sweep could not find: axe has no
rule for non-text contrast, so the dashboard audited clean with a 2.07:1 icon on
it. design.md now says icon contrast has to be checked by hand, and against the
tinted surface rather than white.
The Dismiss control was `<a href="#">`. It performs an action rather than
navigating, so a link mis-announces it to screen readers and does not activate on
Space; it is now `<button type="button">`, underlined for a non-colour-only
affordance, with a focus-visible ring. amber-700 on amber-50 is 4.87:1. Capybara's
click_on matches buttons and links alike, so banners/dismiss_spec is unaffected.
The banner content was already fine at 8.77:1.
Flash messages (shared/_flashes). Success messages now clear themselves after ~6s
via a new auto-dismiss controller; warnings and errors stay put, since they are
often the only record of what went wrong. The timer pauses on hover and focusin so
a message cannot vanish mid-read, which with the delay keeps this clear of WCAG
2.2.1; role="status" is unchanged, so it is still announced on arrival. The fade
is an inline style and the removal is on a timer rather than transitionend: a
class added from JS only works while Tailwind still emits it, and a missed
transitionend would leave the message up forever.
Two things found while doing it:
- "Signed in successfully." only appears when a return path was stored. Devise
prepends allow_params_authentication!, so when Accessible#check_user calls
current_user during POST /users/sign_in it authenticates from the params;
current_user is therefore truthy in a before_action on create, check_user runs
flash.clear and redirects, and Devise's create -- which sets the message --
never runs. With session[:user_return_to] set it is skipped and the message
survives, which is the flow where it is seen. Left as-is: making it appear on
every sign-in is a product call, not part of this fix.
- Authorization failures were being sent as flash[:notice]. The partial maps
notice to the green success variant, so a permission error already rendered as
a success, and auto-dismissing success would have quietly thrown it away.
They now use flash[:alert] (amber, role="alert", no auto-dismiss) in all three
places that produce that message: ApplicationController#not_authorized and the
cross-org RecordNotFound rescues in CasaCasesController and
CourtDatesController. Request specs asserting the key move with it; the system
specs already asserted `.alert`, which every flash div carries.
Covered by spec/system/layouts/flashes_spec.rb (auto-dismiss, hover pause, and an
authorization error staying on screen). Verified: full spec/system 716 examples
clean twice, spec/requests + spec/controllers 1000 examples clean three times,
jest 126/126, standardrb/erb_lint/standardjs clean. Adds the patch_notes `.native`
cleanup to design-todo.md Part 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
open part 2 with the follow-ups carried over from #7051
Starting point for the part-2 tracking PR. Five items, each verified still open against main rather than copied forward from notes:
Dropped one item while checking: the read-notification icon is already slate-500, so its contrast was fixed in the merged work and is not outstanding.