mirror of
https://github.com/onyx-dot-app/onyx.git
synced 2026-10-23 03:42:59 -05:00
roshan
949bb44a1e
## Description This is the bottom of a stack of six PRs. Together they make coding agents (and humans) reuse Opal and refresh-components, not design UI from scratch. - **#16234 (this PR):** enforces the design-system and layout rules with oxlint, and adds an agent hook that makes agents migrate a file to Opal when they edit it. The hook works in Claude Code, Codex and Cursor. - **#16235:** adds a component catalog and a skill, so agents know which components exist before they build. - **#16240, #16241, #16242, #16243:** fix the existing violations (icons, raw elements, replaced refresh-components and refresh `Text`). ### Problem When agents do frontend work in `web/`, they often skip the existing components. They hand-write buttons, cards, chips, rows and empty states that Opal already has. They import `lucide-react` or `react-icons`. They also keep using refresh-components that Opal has replaced, such as the deprecated boolean-flag `Text`. The rules are already written down in `web/AGENTS.md` ("Buttons: `Button` from `@opal/components`. No raw `<button>`", "Icons: only `@opal/icons`", "Never import from `web/src/components/`"). But nothing enforced them: - `web/.oxlintrc.json` had no `no-restricted-imports` and no `react/forbid-elements` config. - A raw `<button>` or a `lucide-react` import passed lint, the pre-commit hooks and CI. - Rules written as prose in an agents file are advisory. Agents follow them some of the time, but not reliably. Current violation counts on `main`, before this PR: | Violation | Files | | --- | --- | | Raw `<button>`, `<input>`, `<select>`, `<textarea>` (outside Opal) | 17 app files, plus 16 in `lib/opal/src` (expected there) | | `lucide-react` / `react-icons` imports | 20 | | `@/components/*` (legacy) imports | 119 | | Imports of refresh-components that Opal replaces | 155 import sites | ### What this PR changes #### 1. New oxlint rules (`web/.oxlintrc.json`) | Rule | Level | What it catches | Message points to | | --- | --- | --- | --- | | `react/forbid-elements` | error | `<button>`, `<input>`, `<select>`, `<textarea>` | `Button`, `InputTypeIn` / `InputCheckbox`, `InputSingleSelect`, `InputTextArea` from `@opal/components` | | `no-restricted-imports` | error | `lucide-react`, `react-icons`, `react-icons/*` | `@opal/icons` | | `no-restricted-imports` | error | `@/components/*`, except `@/components/icons/icons` (`createLogoIcon`, the exception that `web/AGENTS.md` names) | `@opal/*`, then `@/refresh-components/*`, then `@/sections/*` | | `opal/no-replaced-refresh-import` (new) | warn | Imports of refresh-components that Opal replaces | The Opal replacement | Scope: - `lib/opal/src/**`: `react/forbid-elements` is off, because Opal implements these primitives and must render the raw elements. The import rules stay on, so Opal itself cannot import `lucide-react` or `@/components/*`. - `src/components/**`: both rules are off. The folder is legacy and is being deleted, so a lint pass there would be churn. - Tests, specs, `__tests__`, `__mocks__`, `tests/**` and stories: both rules are off. They often need raw elements or fixtures. #### 2. A warn-only baseline for existing violations 94 files outside the exempt paths already broke the error rules. I did not migrate them in this PR. Instead, a "design-system baseline" override in `.oxlintrc.json` lists them, and there the two rules only warn. This is the same per-path ratchet idea that the file already uses for `i18n/no-raw-jsx-text`, the other way round: - New code gets errors at once. - Existing code keeps building. - A file comes off the list when it is clean. No file gets added (the override comment says so). Two oxlint details a reviewer should know: - **The override repeats the full rule options, not only `"warn"`.** An override that only sets a severity (`"no-restricted-imports": "warn"`) resets the rule's options to the default, which is empty. The rule then restricts nothing, so the baseline files were silently not checked at all. I found this in testing. Now the baseline files show their real violations as warnings. - **Next.js dynamic-route segments are escaped** (`[id]` becomes `[[]id]`), because oxlint override globs treat `[...]` as a character class. Without the escape, the bracketed paths did not match, and 14 errors came back. #### 3. New oxlint plugin: `web/tools/oxlint/opal/` It has one rule, `no-replaced-refresh-import`, built the same way as the existing `anti-slop` and `i18n` plugins (`eslintCompatPlugin` + `defineRule`, `package.json` with `exports: ./index.ts`). It holds a map from each refresh-components module to its Opal replacement: | refresh-components module | Opal replacement | Import sites today | | --- | --- | --- | | `@/refresh-components/texts/Text` | `Text` (string-enum `font` and `color` props) | 130 | | `@/refresh-components/texts/Truncated` | `OverflowText` | 11 | | `@/refresh-components/cards/Card` | `Card` | 7 | | `@/refresh-components/buttons/LineItem` | `LineItemButton` | 4 | | `@/refresh-components/Divider` | `Divider` | 1 | | `@/refresh-components/buttons/SelectButton` | `SelectButton` | 1 | How I picked the mapping: I compared the refresh-components file list with the `@opal/components` exports, and then read both sides (JSDoc, README and props) to confirm that the Opal component does the same job. `Text` and `Card` are already marked `@deprecated` in favour of Opal. **Left out on purpose:** `@/refresh-components/buttons/Tag`. It is a display or editable chip with a different API, while Opal `Tag` is a colored status label, so a mechanical migration would change the UI. To add a row later, edit the map in `rules/no-replaced-refresh-import.ts`. #16235 reads the same map, so the catalog follows. This rule only warns, because 155 import sites cannot all move in one PR. The hook below is what drives the migration. #### 4. Agent hook: `.agents/hooks/opal_migrate_on_edit.py` The hook is for every agent that works in this repo: | Agent | Config | Event and matcher | How it gets the edited files | Feedback field the model reads | | --- | --- | --- | --- | --- | | Claude Code | `.claude/settings.json` | `PostToolUse`, `Edit\|MultiEdit\|Write` | `tool_input.file_path` | `hookSpecificOutput.additionalContext` | | Codex | `.codex/hooks.json` | `PostToolUse`, `apply_patch` | The `*** Add/Update File:` and `*** Move to:` lines in the patch text (`tool_input.command`) | `hookSpecificOutput.additionalContext` | | Cursor | `.claude/settings.json` (Cursor imports it by default; the repo's existing Stop hook already relies on this) | `postToolUse`, `Write` | `file_path` (top level or in `tool_input`) | `additional_context` | The per-agent parts are in the new shared module `.agents/hooks/agent_hook_io.py`: - `edited_files()` finds the edited paths in any of the three payload shapes. - `give_feedback()` writes the output shape that each agent reads. - `repo_root()` takes `CLAUDE_PROJECT_DIR`, then `CURSOR_PROJECT_DIR`, then Cursor's `workspace_roots`, then the payload `cwd`. The hook gives feedback as context with exit code 0, so it never blocks or undoes an edit. I did not add a `.cursor/hooks.json`. With Cursor's default import on, it would run each hook twice. What the hook does: 1. It reads the edited `file_path` from the hook payload. It returns 0 at once for anything that is not a `.ts`/`.tsx` file under `web/`, or when `web/node_modules/.bin/oxlint` does not exist (for example, before `bun install`). 2. It runs `oxlint -f json <file>` from `web/`. This uses the real repo config, so the hook and pre-commit cannot disagree. It takes about 0.2s per file. 3. It keeps the diagnostics from the three design-system rules: - errors, always; - warnings from `opal/no-replaced-refresh-import`, `react/forbid-elements` and the icon import restrictions; - but **not** the `@/components/*` warnings in baseline files. Many legacy components have no replacement yet, and forcing an agent to port one in the middle of an unrelated edit would derail the task. 4. If it found any, it gives them to the agent with the line number and the lint help text, plus a pointer to the Opal READMEs and "Do not change behavior". The agent acts on them at once. 5. If the file is in a baseline (design-system or layout) and has no diagnostics left for that baseline's rules, it tells the agent to remove the file from that baseline. So the ratchet tightens by itself. 6. It also reports errors from the two layout rules (section 5). It does **not** report layout-baseline warnings when an agent touches a baseline file. Turning a flex div into a `Section` changes its defaults (centered, full width and height, `gap={4}`), so it is not a mechanical swap. Forcing it on every edit to 327 files would derail unrelated work. To turn it on, delete one `if` in `_is_reported`. This puts the "migrate on touch" convention into practice: an agent cannot edit a file and leave the replaced refresh imports behind. Why a hook in addition to the lint: - The existing `Stop` hook (`pre_commit_on_stop.py`) already runs oxlint at the end of a turn, but only for errors. Warnings do not fail pre-commit, so the migration warnings would never reach the agent. - At the end of a turn, the agent has already written a whole feature the wrong way. Feedback after each edit is cheaper to act on. `web/AGENTS.md` gets one paragraph on the rules, the baseline and the hook. #### 5. Layout rules in the `opal` plugin Two more rules come from a local hook that has run since the summer. They are the lint version of the "everything inline, vertically and horizontally" layout principle. Raw `<button>` and `<input>` are the obvious violations, but most layout drift comes from hand-rolled flex divs, and those break the inline alignment. ##### Layout rule 1: `opal/no-layout-div` (error) The rule flags a `<div>` or `<span>` whose `className` makes it a layout container: `flex`, `inline-flex`, `grid`, `inline-grid`, `items-*`, `justify-*`, `gap-*` or `gap-x/y-*`. The message points to `Section` from `@opal/layouts`, then to `Content`, `ContentAction`, `Card`, `InputHorizontal`/`InputVertical` and `Divider`. - `Section` takes `className`, a ref and event handlers, so there is no reason to drop to a raw element for a container. - Class names are read from string literals, template literal text, `cn(...)`/`clsx(...)` arguments, conditionals, logical expressions, arrays and object keys (`web/tools/oxlint/opal/shared/class-names.ts`). - Variant prefixes are stripped before matching, so `md:flex` counts. - A flex **item** class (`flex-1`, `flex-col` alone, `shrink-0`) does not count. Only container classes do. - **Exempt:** `lib/opal/src/**` (Opal builds the primitives from raw elements), `src/components/**` (legacy) and tests and stories. **Baseline:** 1,035 sites in 327 files break the rule on this branch. A second "layout baseline" override in `.oxlintrc.json` lists those files, and there the rule only warns. As with the design-system baseline (section 2), a file comes off the list when it is clean, and no file gets added. New files get the error. ##### Layout rule 2: `opal/no-section-utility-classes` (error) It flags classes on `Section` that its props own. It detects `Section` from all three import paths: `@opal/layouts`, `@opal/layouts/general/components` and `@/layouts/general-layouts`, which re-exports the Opal Section. | Classes | Why it flags them | Message points to | | --- | --- | --- | | `p-*`, `px-*`, `py-*`, `pt-*`, `pr-*`, `pb-*`, `pl-*`, `ps-*`, `pe-*`, `gap-*`, `gap-x-*`, `gap-y-*` (with or without a variant prefix) | **Dead.** `Section` always sets `gap` and `padding` as inline styles (`spacingToRem` returns `"0rem"` for 0), and an inline style beats any class | `gap={n}`, `padding={n}` (N / 4 rem), or margin classes for asymmetric spacing | | `w-*`, `h-*`, `size-*` with no variant prefix | **Not dead, but they bypass the props.** `cn` uses `tailwind-merge` and puts `className` last, so a `w-40` does override the default `w-full` | `width` / `height` (`"auto" \| "fit" \| "full"` or a number in rem) | What it allows on purpose: - `min-*` and `max-*` classes. Section has no props for them, and `min-w-0`/`min-h-0` (30 sites today) are the standard fix for flex shrinking. - Responsive size classes such as `sm:w-auto`. The props cannot set a width per breakpoint, and Opal's own `Modal` footer uses them. - Margin classes, for asymmetric spacing. A correction to the rule's original idea: size classes on `Section` do not "fight" its defaults. `tailwind-merge` resolves them, and the class wins. That is why rule 2 has a separate message for size classes. ##### Silent bugs the layout rules found Rule 2 found 12 dead padding classes in 7 files. **None of these classes ever applied, so these Sections render with zero padding in production today.** I deleted the classes, so the rendered output stays exactly the same: | File | Dead class(es) | | --- | --- | | `src/ee/views/admin/HooksPage/HookFormModal.tsx` | `px-0.5` | | `src/ee/views/admin/HooksPage/HookLogsModal.tsx` | `py-1`, `py-2`, `py-6` | | `src/views/admin/connectors/AddConnectorPage/components/CredentialChecksCard.tsx` | `pt-1` | | `src/views/admin/GroupsPage/SharedGroupResources/index.tsx` | `p-0.5` (`shrink-0` kept) | | `src/views/admin/connectors/IndexingStatusPage/ConnectorRow.tsx` | `expanded ? "px-2 pt-2 pb-1" : "p-2"` | | `src/views/admin/connectors/ConnectorDetailPage/RunsCard.tsx` | `px-2 pb-2` | The authors meant to add this padding. **Reviewers, especially for the indexing pages:** if you want the spacing that was intended, set it with the `padding` prop, or with margins on the children for asymmetric values. That is a visible change, so I did not make it here. ##### Known gaps - **Grid:** the rule flags `grid` as the spec asked, but Opal has no grid layout. 38 baseline sites use grid. A new grid site has no Opal replacement until Opal adds a grid primitive. - **Inline spans:** `Section` renders a `div`, which cannot sit inside a `<p>`. An `inline-flex` span inside text has no Opal replacement yet. ### Options I rejected - **Prose rules only:** this is the state today. It does not work reliably. - **A `// oxlint-disable` comment in each violating file:** this edits about 100 files, and it hides the violations from the hook. The baseline list keeps the debt visible in one place. - **Making `no-replaced-refresh-import` an error with its own baseline:** 155 sites in about 130 files would mean a second huge list for little gain. A warning plus the hook gives the same result on touched files. - **Hand-rolled grep checks in the hook:** they would drift from the lint config. Running oxlint keeps one source of truth. ### Risks and trade-offs - **Bigger diffs for small edits.** 130 files import refresh `Text`. Any agent edit to one of them now also migrates every `Text` use in that file. This is intentional, but reviewers will see migration noise in otherwise small PRs while the debt goes down. - **Humans do not get the hook.** For humans, oxlint warnings show in the editor. Errors block commits through the existing `oxlint` pre-commit hook and in CI. - **Cursor was not tested live.** The Cursor CLI is not installed on my machine. I tested the Cursor payload and output shapes with sample stdin only. Cursor's docs do not list the field names of `tool_input` for `Write`, so `edited_files()` tries `file_path`, `path`, `target_file` and `filePath`. - **Codex hook trust.** Codex runs project hooks only after the user trusts them, and the trust is tied to each hook's hash. After this PR merges, Codex users accept the new hook once. - **A loop the agent cannot fix.** If an agent cannot migrate a file (for example, a needed Opal prop is missing), it will see the same message after each edit. The message is advisory and the agent can explain and move on. If this happens often, we can add a `stop_hook_active`-style escape like the one in `pre_commit_on_stop.py`. ### How to review 1. `web/tools/oxlint/opal/rules/no-replaced-refresh-import.ts`: is each mapping a correct replacement? 2. `web/.oxlintrc.json`, top-level `rules`: the rule configs and messages. 3. `web/.oxlintrc.json`, `overrides`: the exemptions (tests, Opal, `src/components`), then the baseline. The baseline is a generated list, so you can skim it. 4. `.agents/hooks/opal_migrate_on_edit.py`: the filtering in `_is_reported` and the baseline-cleanup branch. 5. `.agents/hooks/agent_hook_io.py`: the payload parsing for each agent. 6. `.claude/settings.json` and `.codex/hooks.json`: one new `PostToolUse` entry in each. 6. `web/tools/oxlint/opal/rules/no-layout-div.ts`, `no-section-utility-classes.ts` and `shared/class-names.ts`: the layout rules (section 5). Then the layout baseline at the end of `.oxlintrc.json` (generated; skim it) and the 7 files where dead Section classes were removed. ### Follow-ups (not in this PR) - Add a grid layout primitive to Opal, so `opal/no-layout-div` has a replacement for `grid` (38 sites). - Decide on the intended padding for the 7 files with dead Section classes (section 5). - Burn down the baseline and the 155 refresh imports. The hook does this file by file, but a dedicated sweep of `Text` would go faster. - Add `@deprecated` JSDoc to `Truncated`, `Divider`, `SelectButton` and `LineItem`, so editors also strike them through. ## How Has This Been Tested? - **Full lint run:** `oxlint` on all of `web/` gives 0 errors. Design-system warnings: 155 `opal(no-replaced-refresh-import)`, 78 `no-restricted-imports` and 58 `react(forbid-elements)`, all in baseline files. A script confirmed that every file in the baseline still has at least one hit, so there are no dead entries. - **Probe file:** I created `src/sections/__LintProbe.tsx` with `lucide-react`, `react-icons/fi`, `@/components/ErrorCallout`, `createLogoIcon` from `@/components/icons/icons`, refresh `Text` and a raw `<button>`. I got 4 errors with the right help messages and the refresh-`Text` warning. `createLogoIcon` was allowed. Then I deleted the file. - **Baseline options bug:** before the fix, `oxlint src/app/admin/agents/CollapsibleSection.tsx` reported nothing for its `react-icons/fi` import. After the fix, it reports a warning with "Use icons from `@opal/icons`." - **Hook, with sample stdin:** - `src/app/craft/components/BuildWelcome.tsx` (imports refresh `Text`): exit 2, and the message names `Text` from `@opal/components`. - `src/app/admin/agents/CollapsibleSection.tsx` (baseline file, `react-icons`): exit 2 with the icon message. - `lib/opal/src/utils.ts` (clean): exit 0, no output. - `backend/onyx/main.py`: exit 0 (out of scope). - **Live runs in the real CLIs**, on a probe file that imports refresh `Text`: - `claude -p` (Claude Code 2.1.303): after an `Edit`, the model quoted the hook's `additionalContext` with the Opal `Text` replacement. - `codex exec` (Codex 0.163.0): after an `apply_patch`, the model quoted the same message. - **Sample stdin for all three payload shapes:** Claude Code `tool_input.file_path`, the Codex `apply_patch` patch text and the Cursor `conversation_id` + `workspace_roots` payload. Each one gives the right output field. Invalid JSON exits 0 with no output. - **Layout rules probe file** with: - a flex `div`; - a `Section` with `px-2 w-40 sm:w-auto min-w-0 mt-2`; - a span with `cn("md:inline-flex", "text-03")`; - a `div` with `flex-1 p-2`. Result: errors on the flex div, the `md:inline-flex` span, `px-2` and `w-40`. No errors on `sm:w-auto`, `min-w-0`, `mt-2` or the `flex-1 p-2` flex item. The hook reported the same 4 problems. - **Hook on a layout-baseline file** with only layout warnings: no message, as designed. - **Full `oxlint` run on this branch, rebased on the latest main:** 0 errors. 1,035 `opal(no-layout-div)` warnings, all in layout-baseline files. 0 `opal(no-section-utility-classes)` diagnostics. - **Every branch above this one** (#16235, #16240 to #16243) also has 0 lint errors after the rebase. `bun run types:check` at the top of the stack reports no type errors (99.04% coverage). - **pre-commit** passes on all changed files (oxlint, oxfmt, ruff, feature-map integrity, typescript-check). ## Additional Options - [ ] [Optional] Please cherry-pick this PR to the latest release version. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Developer Tooling** * Added automated checks for layout patterns, component spacing, legacy imports, and restricted elements. * Editing web files can surface selected code-quality findings and identify files that are clean of configured baseline rules. * Added these checks to supported editing workflows, with status messages describing the checks being run. * Documented layout and component-spacing standards for web development. * **Style** * Updated spacing in several admin pages to use default section spacing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->