Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .claude/commands/pr.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
description: Create or update a pull request (delegates to the `pr` skill)
allowed-tools: Skill
---

Invoke the `pr` skill to handle this request. The skill covers both creating a new PR and updating an existing one's description, including frontend screenshot capture when the diff warrants it.
3 changes: 3 additions & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"enabledMcpjsonServers": ["playwright"]
}
86 changes: 86 additions & 0 deletions .claude/skills/frontend-conventions/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
---
name: frontend-conventions
description: >-
Frontend conventions for this project — the `Form::Group`/`Form::Input`
components for form fields, the `twlink` class for basic links, the
`number_display` helper for numbers, the `UI::Time::Component` for times,
the UI component library rule (buttons are `UI::Button`/`UI::ButtonLink`
— check `app/components/ui/` and `app/components/form/` before
hand-rolling markup), the shared collapse helpers for showing/hiding
elements, and ViewComponent rules (keyword arguments, instance
variables, `helpers.` prefix in templates). Trigger when adding or modifying views
(`.html.erb`), view components, Stimulus controllers, Tailwind classes,
or any frontend code that touches styling or interactivity. **Also
trigger before any `mcp__playwright__browser_take_screenshot` call** —
this skill defines the required `tmp/` filename rule so screenshots
don't land in the project root. Stimulus.js is the JavaScript framework.
---

# Frontend conventions

This project uses **Stimulus.js** for JavaScript interactivity and **Tailwind CSS** for styling.

The `bin/dev` command runs `bin/rails tailwindcss:watch` (see `Procfile.dev`) to build and update Tailwind CSS. JS is served directly via importmap (`config/importmap.rb`, `app/javascript/`) — there's no separate JS build step.

**Format ERB before committing.** After editing any `.html.erb`, run `bin/lint <file>` — it runs `herb-format`, which reflows long `class` attributes and normalizes ERB. CI does *not* run the herb steps (`.github/workflows/ci.yml` only calls `standardrb`, `rubocop`, and `bin/brakeman`), so unformatted ERB won't fail the build — run `bin/lint` anyway to keep formatting consistent. It needs `yarn install` to have run, since the herb tools come from `devDependencies`.

## Standard classes and helpers

- Form fields should be rendered through `Form::Group::Component` (label + input, `app/components/form/group`) or `Form::Input::Component` directly (`app/components/form/input`) — not hand-rolled `<input>`/`<label>` tags with ad-hoc classes. `Form::Group::Component` applies the shared label classes and delegates the field to `Form::Input::Component`, which centralizes the input's Tailwind classes.
- Basic links should use the `twlink` class.
- **Every number** should be rendered with `number_display(number)` (defined in `app/helpers/application_component_helper.rb`). This applies even when a number is composed into a string with non-numeric values — wrap the number itself, not the surrounding string.
- Good: `[number_display(user.year_joined), user.name].join(" ")`
- Bad: `[user.year_joined, user.name].join(" ")`
- "Number" includes years, counts, prices, distances, IDs — anything numeric, even when it reads like a label.
- **Times** should be rendered with `UI::Time::Component` rather than `time_ago_in_words` or raw `strftime`.
- **Horizontally-scrolling containers (`overflow-x-auto`/`overflow-x-scroll`) must bleed to their parent's edges**, with the parent's own horizontal padding reapplied on the scrollable element itself: cancel the parent's `px-N` with `-mx-N` on the scroller, then add that same `px-N` back on the scroller. This keeps the at-rest look identical (content still starts inset) but lets the scroll track — and touch/scroll gestures — reach the container's true edges instead of stopping short inside a padded dead zone. See `UI::Table::Component`'s wrapper (`-mx-4 … px-4`) for the pattern in practice.
- Good: parent has `p-4`; scroller has `class="-mx-4 flex gap-4 overflow-x-auto px-4"`.
- Bad: scroller sits inside the parent's padding with no margin adjustment — it never reaches the edge, so gestures starting at the edge miss it and there's no partial-next-item peek.

## Use the UI component library

**Check `app/components/ui/` (and `app/components/form/`) before hand-rolling any UI primitive.** If a `UI::*`/`Form::*` component exists for the pattern — buttons, dropdowns, badges, modals, pagination, tables, alerts — use it; if it almost fits, extend it rather than forking its markup inline. A hand-styled one-off silently drifts from the shared colors/sizes/dark-mode states the next time the design changes.

- **Every button** goes through `UI::Button::Component` — never a bare `<button>` or submit input with ad-hoc Tailwind classes. It centralizes the colors (`:primary`/`:secondary`/`:error`/`:link`), sizes (`:sm`/`:md`/`:lg`), and the focus/active/dark-mode states.
- A link styled as a button is `UI::ButtonLink::Component.new(href:, text:, color:, size:)` — same palette, renders an `<a>` via `link_to`.

## Showing and hiding elements: use the collapse helpers

Any time you show, hide, or toggle an element in response to interaction, go through the shared collapse helpers. **Never** hand-roll it with the `hidden` attribute, `element.style.display`, `element.hidden = true`, or ad-hoc `classList.add('hidden')` — those skip the shared show/hide animation and the `hidden!`/`hidden` class contract the rest of the app depends on.

- **Markup-only toggle** (a trigger reveals/collapses a panel, no other logic): add `data-controller="collapse"`, mark the collapsible element `data-collapse-target="content"`, and wire the trigger's `data-action` to `collapse#toggle` / `collapse#show` / `collapse#hide` (`app/javascript/controllers/collapse_controller.js`; optional `data-collapse-duration-value`).
- **Inside your own Stimulus controller** (you have extra logic — a redirect branch, a query-param check, etc.): import the collapse util and call it directly:

```js
import { collapse } from 'utils/collapse_utils'
// ...
collapse('show', this.formTarget) // 'show' | 'hide' | 'toggle'; optional duration (default 200)
```

The collapsible element starts hidden with the **`hidden` class** (not the `hidden` attribute) — `collapse` toggles `hidden`/`hidden!` and runs the height transition for you. Because the initial hidden state is a class, component specs assert it by class (`have_css("[…].hidden")`), not Capybara visibility — the rack_test driver doesn't evaluate CSS, so it can't tell a class-hidden element is hidden.

## No dead hooks in markup

Only add an `id` or non-utility `class` when something concrete consumes it — a CSS rule, a JS/Stimulus selector, a test fixture, an accessibility attribute. Don't keep or invent "structural identifier" hooks "in case something needs them later," and don't replace a removed hook with a renamed one out of inertia.

When deleting an `id`/`class`, grep the repo for the name before deciding what to do with it:

- Zero consumers: delete it, don't rename it.
- Consumers exist: either update them, or leave the hook in place — the consumers are the *reason* it earns its spot in the markup.

## ViewComponent rules

This project uses the ViewComponent gem to render components.

- **Prefer view components to partials.**
- Generate a new view component with `rails generate component ComponentName argument1 argument2`.
- View components must initialize with **keyword arguments**. Everything the component needs must be passed in explicitly by the caller — never reach into controller state from inside a component (e.g. `controller.instance_variable_get(:@user)`). If the component needs `@user`, the caller renders `Component.new(user: @user)`.
- In view components, **use instance variables directly** — don't add `attr_reader`/`attr_accessor`. Reference `@foo` everywhere, including in the template.
- In ViewComponent templates, use the `helpers.` prefix for view helpers (e.g. `helpers.current_page_active?`, from `ApplicationHelper`).
- Rule of thumb: try the bare call first. Only add `helpers.` if it fails with `NoMethodError` — route helpers (`user_path`, `new_user_session_path`) and ActionView tag/url builders (`tag.span`, `content_tag`, `link_to`, `button_to`, `url_for`) are mixed into `ViewComponent::Base` directly, so they don't need it.
- **Never nest a component inside a folder that already holds a `component.rb`.** Each component lives in `app/components/<path>/component.rb` (and `spec/components/<path>/component_spec.rb`); siblings go in sibling folders, not subfolders. If you have `ui/dropdown/component.rb` and need a related component, place it at `ui/dropdown_item/component.rb` (module `UI::DropdownItem`), not `ui/dropdown/item/component.rb`.
- **Converting a partial to a component is a faithful move, not a cleanup.** Carry the markup over verbatim — including comments and commented-out code, which are often a deliberate stash someone expects to restore. The only changes a conversion should introduce are the mechanical ones the move *requires* (e.g. adding `helpers.` where a helper now needs it). If you spot genuine dead code worth removing, that's a separate judgment call — raise it or do it in its own commit, don't fold it into the move.

## Manual browser verification

**Every `mcp__playwright__browser_take_screenshot` call must pass a `filename:` that starts with `tmp/`** (e.g. `tmp/tooltip-hover.png`). The MCP tool's default root is the project root — a bare filename like `tooltip.png` lands in the working tree, shows up in `git status`, and has to be cleaned up by hand. `tmp/` is gitignored, so screenshots there stay out of commits and don't pollute the diff. This rule applies to ad-hoc visual verification, not just PR-screenshot capture.
91 changes: 91 additions & 0 deletions .claude/skills/frontend-screenshots/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
---
name: frontend-screenshots
description: >-
Capture desktop+mobile viewport screenshots of local pages served by
`bin/dev` via Playwright MCP, with a PII safety check that keeps real
data out of uploaded images. Use whenever a task needs screenshots of local
pages — PR documentation, bug repros, before/after comparisons across
branches, design review, demos — including mid-interaction states like an
open dropdown, a modal showing, a form mid-fill, or a hover. Use it even when
the user just says "grab a screenshot" or "show me what this looks like"
without naming Playwright. For a component that only renders under an env var
/ feature flag / hard-to-reach state, screenshot its Lookbook preview URL
instead of a full page. Inputs: `(url-path, page-slug)` pairs, optionally with
per-URL interaction steps. Output: local PNG paths.
allowed-tools: Bash, Read, ToolSearch, mcp__playwright__*
---

# Frontend screenshots

Drive Playwright MCP to capture viewport screenshots of pages served by `bin/dev`.

## Output filenames (load-bearing — callers parse these)

`tmp/pr_screenshots/<branch>-<page>-<timestamp>-{desktop,mobile}.png`, where `<branch>=$(git rev-parse --abbrev-ref HEAD | tr '/' '-')` and `<timestamp>=$(date +%Y%m%d-%H%M%S)`. Cross-branch shots get an extra `-main-` segment.

## Preflight

- `eval "$(ruby bin/env --export)"` so `$WORKSPACE_ID`, `$DEV_PORT`, `$BASE_URL`, and `$REDIS_URL` are set. **Each Bash tool call is a fresh shell, so re-export in any shell that reads `$BASE_URL`** — `eval "$(ruby bin/env --export)" && curl …` chained in one call is the safe pattern.
- `curl -fs "$BASE_URL/" >/dev/null` — if it isn't up, **stop and ask the user to start it** (`bin/dev`). `bin/env` resolves `$DEV_PORT` from the workspace ID in `.workspace_id` (falling back to `3009` in the root checkout), so the `bin/dev` the user starts binds to the same port and databases this skill expects.
- If `bin/dev` exits immediately with `Could not find 'bundler' (X.X.X)` or similar, the shell resolved system Ruby 2.6 instead of mise-installed 4.0.6 — see the [`sandbox-test-setup`](../sandbox-test-setup/SKILL.md) skill's local-macOS section for the one-line PATH fix; don't reinstall.
- If `mcp__playwright__*` tools aren't registered, the project's `.mcp.json` defines the `playwright` server — approve it on project entry (Claude Code prompts) and restart the session, or `/mcp` → **playwright** → reconnect. A server added mid-session doesn't load until restart.

## Sign in (when needed)

The MCP browser session persists across calls, so signing in is a one-time-per-session step. For public pages (the root page, Lookbook previews at `/lookbook/...`, etc.), skip this entirely. **Only ever authenticate against the local dev server** (`$BASE_URL` / localhost) — never sign in to any other host, and never create, promote, or impersonate users to bypass auth.

`db/seeds.rb` is the stock empty Rails template — convus_webapp doesn't seed any users, so there's no fixed credential to drive Playwright with. When a navigation lands on `/users/sign_in`, ask the user for an email/password to sign in with (and which role, if it matters: `User#role` is `basic_user`, `developer`, or `admin` — `admin_access?` is true for `developer` or `admin`, and gates `/admin/...` routes). Fill `Email` + `Password` and click `Log in`; the post-login redirect dumps you on `/` (or the originally-requested path if Devise stored it).

**Don't upload real PII.** Screenshots are permanent once uploaded. Even when signed in, if a page shows records that don't look like test data (unfamiliar names/emails, real-looking user content), stop and ask — the dev DB may have been loaded with production data.

## Capture

Clear stale shots: `rm -f tmp/pr_screenshots/<branch>-<page>-*.png 2>/dev/null || true`.

Two viewports — resize once each, then walk every URL:
1. `browser_resize` 1440×900 → for each URL: navigate → settle → `browser_take_screenshot` (`fullPage: true`) to `...-desktop.png`.
2. `browser_resize` 390×844 → same loop → `...-mobile.png`.

**Pass the path as workspace-relative**, e.g. `tmp/pr_screenshots/<branch>-<page>-<ts>-desktop.png`. Playwright MCP rejects absolute paths that escape the workspace root with `File access denied: … is outside allowed roots`.

**Full page, no `target:` arg.** Capture the whole page (`fullPage: true`) so nothing below the fold is cut off. convus_webapp's layout (`app/views/layouts/application.html.erb`) has no site footer, so there's nothing to hide before the shot.

Element-only crops (`target:`) still slice context off — don't use them for page captures.

**Settle before the screenshot.** Stimulus + Chartkick render after document load; either `browser_wait_for` on a known element or pause ~500ms–1s. Otherwise charts capture mid-draw.

**Mid-interaction states are in scope.** When the caller asks for a dropdown open, a modal showing, a hover state, a partially-filled form, etc., drive Playwright between settle and the screenshot — `browser_click`, `browser_type`, `browser_press_key`, `browser_hover`, then wait for the UI to reach the target state (`browser_wait_for` on a marker element, or check via `browser_evaluate`) before `browser_take_screenshot`. Treat the interaction sequence as part of the page-slug — e.g. capture `dropdown-open` after clicking, distinct from a static page-load shot. For cross-branch comparisons, run the *same* interaction sequence on each branch so the screenshots actually compare like-for-like.

Sanity-check each PNG: under ~5 KB usually means the page errored. Pull `browser_console_messages` and look only for **uncaught exceptions from app code** (Stimulus registration failures, `TypeError`s in `app/javascript/**`) — asset 404s and third-party deprecation warnings are noise. To diagnose a failed capture: HTTP status via `curl -s -o /dev/null -w "%{http_code}\n" "$BASE_URL/<path>"`, response body via `curl -s "$BASE_URL/<path>" | head -200`, full backtrace via `tail -200 log/development.log`.

Only stop and surface to the user once you understand the cause and either (a) have a fix to propose, (b) need input they must provide (e.g. which URL to screenshot instead), or (c) concluded it's a real bug in the diff.

## Component previews (when no page shows the state)

Some components only render in a context you can't reproduce on a normal dev page — gated by an env var, a feature flag, or a hard-to-reach error/empty state. When a component has a Lookbook preview, screenshot the **preview URL** instead of hunting for a page that happens to render it:

```
$BASE_URL/lookbook/preview/<preview_path>/<scenario>
```

`<preview_path>` is the component's module nesting underscored (drop the `ComponentPreview` suffix), and `<scenario>` is the preview method. `UI::Dropdown::ComponentPreview#placements` → `/lookbook/preview/ui/dropdown/placements`. If a scenario doesn't exist yet, add a method to the component's `*_preview.rb` first — a preview that renders the exact state (pass the args that trigger it) is often the fastest path to a clean shot.

The preview page loads Tailwind and renders the component standalone (no site chrome), so capture the viewport as usual (`fullPage: false`); a small render-timing line at the bottom is harmless. Everything else still applies — same PII caution, same `(url-path, page-slug)` naming (use a slug like `dropdown-placements`).

convus_webapp's component previews render from static sample data (`OpenStruct` records, hardcoded chart series) rather than the dev DB, so there's no seed step before a preview renders. This is component-only: a preview can't show layout/stacking against the rest of the page (e.g. a navbar z-index fix), so use a real page for those.

## Cross-branch comparison (optional)

When the caller wants before/after, repeat the capture loop against `main`.

1. `git status` — abort if there are uncommitted changes.
2. Diff `db/migrate/` between the branch and `main`; abort if it changed — a branch-only migration leaves the DB schema ahead of `main`'s code, so `main` pages can error.
3. `BRANCH=$(git rev-parse --abbrev-ref HEAD)`, `git checkout origin/main` (detached — `git checkout main` fails if a sibling worktree holds the `main` branch; detached HEAD at `origin/main` is allowed concurrently and is the same code), navigate the browser to force Rails to reload the changed files, repeat capture into `...-main-...` filenames, then `git checkout $BRANCH`.

A `Gemfile.lock` diff is **not** a reason to abort.

The dev DB persists across checkouts, so any signed-in session usually still works.

## Clean up

Once every screenshot is captured, quit Chrome with `browser_close`. Leaving it running holds the shared browser profile lock, so the next `browser_navigate` (this skill or another) fails with "Browser is already in use". Always close it before returning, even if the capture failed partway.
Loading