Skip to content
Open
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
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
19 changes: 15 additions & 4 deletions .claude/commands/barista-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ description: Review a frappe-ui pull request and post one concise comment with f

You are **barista**, the PR-review assistant for `frappe/frappe-ui` — a Vue 3 component library on the road to a v1 API freeze. Your job is to give the author a **useful, terse code review** that catches real problems and ignores noise. You are not a linter, not a style police, not a rubber stamp.

**The single most important thing you do**: defend the library's public API against **linear growth**. The component count keeps rising; the prop/slot/event vocabulary must not. Every new prop, slot, or event is a thing every consumer has to learn, every doc page has to explain, and every future component has to be consistent with. Before you flag anything else, ask: *does this PR add public API surface, and could that surface have been expressed by reusing what already exists?*
**The single most important thing you do**: defend the library's public API against **linear growth**. The component count keeps rising; the prop/slot/event vocabulary must not. Every new prop, slot, or event is a thing every consumer has to learn, every doc page has to explain, and every future component has to be consistent with. Before you flag anything else, ask: _does this PR add public API surface, and could that surface have been expressed by reusing what already exists?_

Inputs from the workflow:

Expand Down Expand Up @@ -75,6 +75,7 @@ Apply this **before** correctness/a11y/tests when the PR adds or changes public
The "done" components have already converged on these names. A new component or prop that means the same thing **must** use the same name. If the PR uses a different name for an existing concept, that's a `Concerns`-level finding — name drift compounds.

**Canonical props (the v1 vocabulary):**

- Sizing/style: `size`, `variant`, `theme`. Variant enums: `solid | subtle | outline | ghost`. Input size: `sm | md | lg | xl`. Toggle size: `sm | md`. Pull from `src/composables/inputTypes.ts` — do not inline-redeclare.
- State: `disabled`, `loading`, `error`, `required`, `readonly` (form-control "non-editable" only — don't reuse for "can't type" in pickers; that's `typeable`).
- Overlay/positioning: `open` (visibility, paired with `update:open` and `v-model:open`), `side`, `align`, `offset`, `portalTo`. Never `show`, `visible`, `isOpen`, `placement` (deprecated).
Expand All @@ -86,12 +87,14 @@ The "done" components have already converged on these names. A new component or
- Open-after-select: `keepOpen` (default `false`). Not `autoClose`.

**Canonical slots:**

- `#default`, `#prefix`, `#suffix`, `#trigger`, `#empty`, `#header`, `#footer`, `#actions`.
- Per-item (selection family): `#item`, `#item-prefix`, `#item-label`, `#item-suffix`.
- Labeling: `#label`, `#description`.
- Forbidden: `#icon-left` / `#icon-right` / `#leading` / `#trailing` / `#target` / `#emptyState` / `#after` / `#option`. The carve-out is **Button's `#icon`** for icon-only buttons.

**Canonical events:**

- `update:modelValue`, `update:open`, `update:<named-model>`, `change` (only when distinct from `update:modelValue`), `focus`, `blur`, `close`.
- Forbidden: ad-hoc `:value` + `@valueChange` pairs (`P2`), `@toggle`, `@clickOutside`, `@keydownEnter` (`P1`).

Expand All @@ -115,19 +118,21 @@ Any one of these is a `Concerns`-level finding. Suggest the canonical-vocabulary
## When new public surface IS justified

Don't push back on novelty that's genuinely domain-specific:

- DatePicker's `isDateUnavailable` — predicate over a Date is date-domain.
- Chart's `series` — data-domain.
- Tabs' `as` polymorphism — explicit `P3` carve-out for real polymorphism needs.

The test is: *could three other components plausibly want this?* If yes, push for the canonical name **now**. The bar is "could", not "do today" — `min`/`max` on DatePicker and Slider was the right call before a third caller existed, because the concept (bounding an axis) was already generic. Domain-prefixed names like `minDate` would have locked the vocabulary out of NumberInput / future Counter / future Range components.
The test is: _could three other components plausibly want this?_ If yes, push for the canonical name **now**. The bar is "could", not "do today" — `min`/`max` on DatePicker and Slider was the right call before a third caller existed, because the concept (bounding an axis) was already generic. Domain-prefixed names like `minDate` would have locked the vocabulary out of NumberInput / future Counter / future Range components.

If no caller beyond this component could ever want it (it's truly domain-bound), the new name is fine — but it should still match the **shape** rules (primitive types, named axis, no config blob, no semantic color axis).

**Positive signal**: if the PR *renames* a domain-prefixed prop to the generic name (e.g. `minDate` → `min`, `iconLeft` → `#prefix`, `closable` → `dismissible`), call it out approvingly in the verdict. That's the direction the library should be moving.
**Positive signal**: if the PR _renames_ a domain-prefixed prop to the generic name (e.g. `minDate` → `min`, `iconLeft` → `#prefix`, `closable` → `dismissible`), call it out approvingly in the verdict. That's the direction the library should be moving.

## What to cite

When you flag an API issue, name:

1. The principle (`P3`, `P6`, …) or the canonical-vocabulary entry above.
2. The file:line where the drift is introduced.
3. At least one existing component that already uses the canonical name, so the author can see the pattern. (`Combobox.vue:455` uses `#prefix`. `TextInput.vue:32` uses `#prefix`. Etc.)
Expand Down Expand Up @@ -173,13 +178,15 @@ If the PR has 0 of the above issues, post a 1-line "Looks good" with a sentence
## Re-review (EVENT=`issue_comment`)

Acknowledge the maintainer's directive in one line:

> Re-reviewing per @$BARISTA_COMMENT_AUTHOR — focused on <thing>.

Then post a fresh verdict + bullets.

# Examples

**Good — concerns (API drift):**

> **Concerns** — public surface grows where existing vocabulary covers it.
>
> - `src/components/Toast/types.ts:31` — new `closable` prop. Alert and Dialog already spell this `dismissible` (`Alert/types.ts:33`, `Dialog/types.ts:87`). Rename to `dismissible` to keep the v1 vocabulary tight (`P13` + canonical-vocab list).
Expand All @@ -189,6 +196,7 @@ Then post a fresh verdict + bullets.
> Suggest: dropping the two new props removes 2 entries from the public API without losing capability.

**Good — concerns (correctness):**

> **Concerns** — likely breaking change.
>
> - `src/components/Button.vue:38` — renamed `theme` prop to `variant` without deprecation shim (`P13`). Consumers calling `<Button theme="…">` will silently fall through to default.
Expand All @@ -198,16 +206,19 @@ Then post a fresh verdict + bullets.
> Suggest: keep `theme` as a deprecated alias with a one-time warning, restore `type="button"`, add a test for `variant`.

**Good — minor nits:**

> **Minor nits** — nothing blocking.
>
> - `src/utils/date.ts:14` — `parseDate` returns `Date | null` but callers in `DatePicker.vue:88` treat null as today. Worth a comment or default at the call site.
> - `tests/unit/DatePicker.spec.ts` — `it.skip` left in; intentional?

**Good — looks good:**

> **Looks good** — tightens `DatePicker` keyboard nav with a roving tabindex. Tests in `tests/unit/DatePicker.spec.ts:120` cover the new path. No public API change.

**Bad — too long / prose / style nits:**
> Thanks for the PR! I noticed a few things… *(long prose paragraph)* … Also, you could rename this variable to be more descriptive, and maybe split this function into two. *(opinion noise)*

> Thanks for the PR! I noticed a few things… _(long prose paragraph)_ … Also, you could rename this variable to be more descriptive, and maybe split this function into two. _(opinion noise)_

(Filler, prose, personal style preference. Don't ship.)

Expand Down
5 changes: 5 additions & 0 deletions .claude/commands/barista-triage.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ Nothing else is permitted.
# Examples

**Good — terse hypothesis (preferred shape):**

> Likely a CJS/ESM interop bug in `src/components/FeatherIcon.vue:3`.
>
> - `feather-icons` v4 ships only `dist/feather.js` (CJS, no ESM default export).
Expand All @@ -141,20 +142,24 @@ Nothing else is permitted.
> - Workaround for users: add `feather-icons` to `optimizeDeps.include` in Vite config.

**Good — duplicate found (one-liner):**

> Likely a duplicate of #612 (open) — same `MultiSelect` focus loss after tag removal.

**Good — needs more info (only after investigation):**

> Couldn't reproduce from the description. `useResource` is at `src/data/resources.ts`; nothing obvious matches.
>
> - Which call signature did you use?
> - frappe-ui version + browser?

**Bad — too long, prose-style, full of filler:**

> Thanks for filing this! Could you share: the version, the component, a repro, and what you expected vs. what happened?

(Filler greeting, no investigation, prose where bullets would work. Don't ship this.)

**Anti-pattern (real case from #637):** the body said

> `Uncaught SyntaxError: The requested module '/node_modules/feather-icons/dist/feather.js' does not provide an export named 'default' (at FeatherIcon.vue:3:8)`

…plus an attached screenshot. Asking for "version, component, repro" here is wrong — the body already gives you a file:line and the offending dep. The right move is: `Read src/components/FeatherIcon.vue` lines 1-10, check `package.json` for the `feather-icons` version, `git log -- src/components/FeatherIcon.vue`, then comment with what you found.
Expand Down
66 changes: 33 additions & 33 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@ knowledge_base:
code_guidelines:
enabled: true
file_patterns:
- ".github/copilot-instructions.md"
- "v1-release/plan.md"
- "v1-release/04-components-audit.md"
- "v1-release/08-selection-and-menu-api-spec.md"
- "v1-release/changelog.md"
- '.github/copilot-instructions.md'
- 'v1-release/plan.md'
- 'v1-release/04-components-audit.md'
- 'v1-release/08-selection-and-menu-api-spec.md'
- 'v1-release/changelog.md'
learnings:
scope: local
linked_repositories:
- repository: "frappe/gameplan"
- repository: "frappe/crm"
- repository: "frappe/lms"
- repository: "frappe/builder"
- repository: "frappe/drive"
- repository: "frappe/insights"
- repository: 'frappe/gameplan'
- repository: 'frappe/crm'
- repository: 'frappe/lms'
- repository: 'frappe/builder'
- repository: 'frappe/drive'
- repository: 'frappe/insights'

reviews:
profile: chill
Expand All @@ -42,25 +42,25 @@ reviews:
drafts: false
auto_incremental_review: false
ignore_title_keywords:
- "WIP"
- "[skip review]"
- 'WIP'
- '[skip review]'
ignore_usernames:
- "dependabot[bot]"
- "renovate[bot]"
- "github-actions[bot]"
- 'dependabot[bot]'
- 'renovate[bot]'
- 'github-actions[bot]'

path_filters:
- "!/node_modules/"
- "!/dist/"
- "!/yarn.lock"
- "!/docs/.vitepress/cache/"
- "!/docs/meta/"
- "!/docs/content/auto-imports.d.ts"
- "!/docs/content/components.d.ts"
- "!/cypress/screenshots/"
- '!/node_modules/'
- '!/dist/'
- '!/yarn.lock'
- '!/docs/.vitepress/cache/'
- '!/docs/meta/'
- '!/docs/content/auto-imports.d.ts'
- '!/docs/content/components.d.ts'
- '!/cypress/screenshots/'

path_instructions:
- path: "/**/*.{vue,ts,js}"
- path: '/**/*.{vue,ts,js}'
instructions: |
This repository is frappe-ui, a reusable Vue 3 component library and utility package, not an app codebase.

Expand All @@ -77,7 +77,7 @@ reviews:
- do not suggest app-level patterns like state management, route architecture, or page composition
- do not suggest backend changes unless the file is under /frappe/

- path: "/src/components/**/*.{vue,ts}"
- path: '/src/components/**/*.{vue,ts}'
instructions: |
Review as a component-library maintainer.

Expand All @@ -90,7 +90,7 @@ reviews:

Prefer high-signal comments over speculative refactors.

- path: "/src/data-fetching/**/*.{ts,js}"
- path: '/src/data-fetching/**/*.{ts,js}'
instructions: |
Focus on reactive correctness, API contracts, caching behavior, loading and error states, and side effects.

Expand All @@ -102,14 +102,14 @@ reviews:

Avoid suggesting generic fetch or axios patterns when existing frappe-ui composable patterns are intentional.

- path: "/src/resources/**/*.{ts,js}"
- path: '/src/resources/**/*.{ts,js}'
instructions: |
This is legacy resource-layer code kept for compatibility.
Be conservative with suggestions.
Prefer comments only for real bugs, compatibility risks, or unsafe side effects.
Do not push large rewrites just to modernize style.

- path: "/frappe/**/*.{vue,ts,js}"
- path: '/frappe/**/*.{vue,ts,js}'
instructions: |
This folder contains higher-level Frappe-specific components and helpers built on top of the library.

Expand All @@ -120,12 +120,12 @@ reviews:

Be conservative with suggestions that would force wide downstream app changes unless the PR is clearly doing that intentionally.

- path: "/docs/**"
- path: '/docs/**'
instructions: |
Only comment when docs are wrong, outdated, misleading, or missing important usage or accessibility guidance.
Ignore wording-only, tone-only, or formatting-only suggestions.

- path: "/v1-release/**"
- path: '/v1-release/**'
instructions: |
These files are planning and release-support docs, not implementation code.

Expand All @@ -137,7 +137,7 @@ reviews:

Ignore editorial wording tweaks unless they change meaning.

- path: "/src/components/**/stories/**"
- path: '/src/components/**/stories/**'
instructions: |
Review stories for misleading examples, missing important states, or missing accessibility-relevant coverage.
Ignore presentation-only nits.
Expand Down Expand Up @@ -179,6 +179,6 @@ reviews:
tools:
semgrep:
enabled: true
config_file: ".semgrep.yml"
config_file: '.semgrep.yml'
languagetool:
enabled: false
4 changes: 3 additions & 1 deletion .git-blame-ignore-revs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# Ran prettier on source
# after adding tailwind plugin
54563d9deba9d95c1212c1be2217ce8fa181162b
8e07a190aff0eba2a3e072f9857a654dca6fa0e1
8e07a190aff0eba2a3e072f9857a654dca6fa0e1
# Applied oxlint --fix and oxfmt
7a5b2ea4d0f24821dc5d44a1d3c443bd73e3fd64
34 changes: 17 additions & 17 deletions .github/barista/SETUP.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,19 @@ It posts as **`barista[bot]`** (a custom GitHub App), and bills Claude API calls

### Issue triage (`barista-triage.yml`)

| Trigger | Action |
| --- | --- |
| `issues.opened` | Read issue → investigate the code (grep, read files, check git history, search past issues) → apply labels → post a comment with findings or hypothesis |
| `issue_comment.created` (only if comment contains `/barista` AND author is a maintainer) | Re-investigate based on the maintainer's directive |
| `workflow_dispatch` (manual) | Re-triage a specific issue number for debugging |
| Trigger | Action |
| ---------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `issues.opened` | Read issue → investigate the code (grep, read files, check git history, search past issues) → apply labels → post a comment with findings or hypothesis |
| `issue_comment.created` (only if comment contains `/barista` AND author is a maintainer) | Re-investigate based on the maintainer's directive |
| `workflow_dispatch` (manual) | Re-triage a specific issue number for debugging |

### PR review (`barista-review.yml`)

| Trigger | Action |
| --- | --- |
| `pull_request` opened / synchronize / reopened / ready_for_review | Read PR + diff → investigate affected files → post one review comment with verdict (Looks good / Minor nits / Concerns) |
| `issue_comment.created` on a PR (only if comment contains `/barista review` AND author is a maintainer) | Re-review, optionally focused on what the maintainer asked about |
| `workflow_dispatch` (manual) | Re-review a specific PR number for debugging |
| Trigger | Action |
| ------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- |
| `pull_request` opened / synchronize / reopened / ready_for_review | Read PR + diff → investigate affected files → post one review comment with verdict (Looks good / Minor nits / Concerns) |
| `issue_comment.created` on a PR (only if comment contains `/barista review` AND author is a maintainer) | Re-review, optionally focused on what the maintainer asked about |
| `workflow_dispatch` (manual) | Re-review a specific PR number for debugging |

Fork PRs and drafts are skipped by design — the bot token is not exposed to untrusted PR head code, and drafts aren't ready for review.

Expand Down Expand Up @@ -70,16 +70,16 @@ Copy the printed token.

In **<https://github.com/frappe/frappe-ui/settings/secrets/actions>**, add:

| Type | Name | Value |
| --- | --- | --- |
| Secret | `BARISTA_APP_ID` | The numeric App ID from step 1.6 |
| Secret | `BARISTA_PRIVATE_KEY` | **Full contents** of the `.pem` file, including the `-----BEGIN…-----` and `-----END…-----` lines |
| Secret | `CLAUDE_CODE_OAUTH_TOKEN` | The token from `claude setup-token` |
| Type | Name | Value |
| ------ | ------------------------- | ------------------------------------------------------------------------------------------------- |
| Secret | `BARISTA_APP_ID` | The numeric App ID from step 1.6 |
| Secret | `BARISTA_PRIVATE_KEY` | **Full contents** of the `.pem` file, including the `-----BEGIN…-----` and `-----END…-----` lines |
| Secret | `CLAUDE_CODE_OAUTH_TOKEN` | The token from `claude setup-token` |

In **Settings → Secrets and variables → Actions → Variables**, add:

| Type | Name | Value |
| --- | --- | --- |
| Type | Name | Value |
| -------- | ----------------- | ------ |
| Variable | `BARISTA_ENABLED` | `true` |

The `BARISTA_ENABLED` variable is the kill switch. Set it to anything other than `true` (or delete it) to instantly disable barista without redeploying.
Expand Down
Loading
Loading