diff --git a/.agents/skills/ui-refactor-review/SKILL.md b/.agents/skills/ui-refactor-review/SKILL.md new file mode 100644 index 00000000..623e68c8 --- /dev/null +++ b/.agents/skills/ui-refactor-review/SKILL.md @@ -0,0 +1,250 @@ +--- +name: ui-refactor-review +description: >- + Review ui/goose2 frontend changes specifically for refactoring opportunities + and long-term maintainability. Use when the user wants cleanup feedback or + wants to improve structure, decomposition, layering, type hygiene, + duplication, dead code, readability, and extensibility in goose2 React + + TypeScript + Tauri UI code. +--- + +# UI Refactor Quality + +Use this skill for `ui/goose2` pull requests. + +Keep the focus on behavior-preserving frontend improvement. Favor the repo's +existing architecture and patterns over generic frontend advice. + +## Goals + +- Review changed code for refactor quality, not just correctness. +- Bias toward detecting maintainability smells even when the code is still functionally correct. +- Review the final shape of the changed code, not whether it is better than what came before. +- Judge changes by whether they leave the code easier to maintain and extend in future work, not just whether they are correct today. +- Produce an actionable checklist instead of vague feedback. +- Ask for approval before making code changes unless the user explicitly asks for fixes. +- Preserve `ui/goose2` boundaries: `ui/`, `hooks/`, `api/`, `lib/`, `stores/`, and `shared/`. + +## Workflow + +1. Determine the review scope. + - Review only the changed lines in the branch or working tree. + - If both committed and uncommitted changes exist, clarify which scope to review when needed. + - If the change mixes feature work with refactoring, call that out explicitly and review the feature changes separately from the cleanup quality. +2. Inspect only the changed lines in that scope, but follow each changed code path into surrounding modules when needed to judge whether the shape is clean. +3. Run the `Smell Checklist` below before looking for bugs. + - Do not require a user-visible bug before calling out a maintainability problem. + - Prefer concrete findings like "this responsibility is misplaced" or "this logic should be extracted" over generic style commentary. + - If a Smell Checklist item is true, turn it into an `Issue`. + - Do not leave a confirmed smell unaccounted for in the final review. + - Do not suppress an `Issue` because the PR already improved the previous version. + - Do not treat partial cleanup as resolution. A smell that remains in the post-PR code is still an `Issue`. +4. Evaluate the changed code against the `Rules` below and identify what the PR already improved and what should still be refactored or cleaned up. +5. Review the changed code separately for these buckets: + - decomposition + - layering + - hooks/effects + - pure helpers + - type shapes + - duplication + - tests + - feature wiring + - For each bucket, explicitly determine whether zero, one, or multiple `Issues` remain. + - If any remain, report all distinct `Issues` you can verify in that bucket. +6. Verify each non-trivial issue against the actual code before turning it into a task. + - Trace the relevant code path end to end. + - Check whether the issue is already handled elsewhere. + - Confirm the suggested cleanup would actually simplify the code. + - Keep the finding if the maintainability problem is real, local, and behavior-preserving to fix, even when the code still works. + - Drop speculative or preference-only findings. +7. Before finalizing the review, run a second pass focused only on finding issues not already listed. + - Discover issues before prioritizing them. + - Do not stop after the first few findings. + - Do not omit a verified issue because it is lower priority than other findings. +8. Produce review output in this order: + - `Applied Well` + - `Issues` + - one ordered `Checklist` for the whole reviewed scope + - Do not stop after the findings until the ordered checklist is complete. +9. Stop after the checklist and ask for approval before making code changes, unless the user explicitly asked to implement fixes. +10. Fix approved checklist items in order, using the `Rules` below as the quality bar for the implementation. + - State the main maintainability problem in one sentence. + - Fix the highest-value items first. + - Make the smallest behavior-preserving change that clearly improves the code. +11. Summarize what changed, what remains, and what verification ran. + +## Strict Mode + +- Any confirmed smell in the changed code must be reported as an `Issue`. +- Review the post-PR shape. Do not grade on a curve. +- Partial extraction, partial deduplication, or partial cleanup does not clear a remaining smell. +- If multiple distinct smells remain in one file, report each distinct responsibility problem separately. + +## Smell Checklist + +Before finalizing the review, explicitly ask: + +- Is any view or page component still doing too many jobs? +- Is any pure derivation logic still trapped in a component instead of `lib/`? +- Is any repeated async UI workflow ready for a focused hook? +- Are helpers duplicated or living in the wrong layer? +- Are any inline object shapes large enough to deserve a named type? +- Did logic move without moving or adding the right tests? +- Did the refactor preserve feature wiring while improving structure? + +## Rules + +### Size And Decomposition + +- Treat these as smell thresholds, not hard limits: + - components around 200 lines + - functions around 40 lines + - files around 300 lines + - JSX nesting around 4 levels +- Treat "many unrelated state variables + many handlers + many effects in one view" as a smell even when the line count is still tolerated. +- Treat a file that owns multiple unrelated responsibilities across data loading, derivation, mutation, and rendering orchestration as a smell unless there is a strong reason to keep it together. +- If a component does more than its name claims, rename it or split it. +- Split by responsibility, not by arbitrary line count. +- When a view contains substantial pure derivation logic, prefer extracting it into `lib/` helpers with direct tests. +- When a view contains substantial effectful workflow logic, prefer extracting it into a focused hook. +- Do not suppress a decomposition `Issue` just because the PR already extracted some responsibilities. +- If the remaining file still does too many jobs, report that as an `Issue`. +- File size alone is not the finding. The finding is the number of unrelated responsibilities still owned by the final file. + +### Naming Reveals Intent + +- Use names that describe intent, not implementation trivia. +- Prefer domain terms over generic placeholders like `data`, `value`, or `handler`. +- A helper name should describe what it returns or decides, not how it computes it. +- Rename misleading functions before adding comments to explain them. + +### Layer Discipline + +- `ui/`: rendering and light view logic only. +- `hooks/`: glue between React state/effects and lower layers. +- `api/`: backend transport wrappers and DTO adaptation only. +- `lib/`: pure functions and domain helpers only. +- `stores/`: shared feature state only. +- Keep business logic out of render-heavy components when a hook or utility would make it clearer. +- If a component mixes pure transforms and UI event orchestration, split the pure transforms out first. +- Do not move simple local state into a store unless multiple consumers truly need it. +- Keep `api/` free of UI imports, path logic, and unrelated domain policy. +- Keep `lib/` free of React, DOM, `window`, and I/O. +- Prefer shared domain helpers in `lib/` when the same normalization, formatting, or parsing logic appears in multiple modules. +- If logic lives in the wrong layer after the PR, report that as an `Issue` even if the PR reduced the amount of misplaced logic. + +### Module Encapsulation + +- Export the minimum surface a module needs to share. +- Keep helpers, constants, and intermediate transforms private unless another module genuinely needs them. +- Treat removing stale exports as a quality improvement. +- If a helper is used in only one module, default to keeping it local. +- If similar helpers appear across two modules, default to extracting them. + +### DRY And Hooks + +- Extract shared behavior once the duplication is clear and the shared abstraction is stable. +- Two call sites can be enough when the shared shape is obvious and both call sites become simpler. +- Prefer a hook when the shared logic is stateful or effectful. +- Keep each hook focused on one job. +- Keep hook return shapes stable so callers are not forced to handle shifting contracts. +- Do not use a hook as the default extraction target for oversized components. +- If the logic is pure and React-independent, report extraction to `lib/`. +- If the logic coordinates React state, effects, async actions, or UI event orchestration, report extraction to `hooks/`. +- Treat repeated pure UI derivation logic as helper extraction candidates. +- If repeated effectful orchestration remains in the changed code, report that as an `Issue`. +- If repeated pure transforms remain in the changed code, report that as an `Issue`. + +### Type Hygiene + +- Keep canonical cross-feature types in `src/shared/types/`. +- Do not duplicate types across features when one shared type should exist. +- Give inline object types with 3 or more fields a name when they start obscuring the code. +- Prefer `Pick`, `Omit`, and `Partial` over restating shapes by hand. +- Avoid `any`, unchecked `as`, non-null assertions, and string-encoded pseudo-unions when a discriminated union would be clearer. +- Treat repeated or verbose inline object shapes as extraction candidates for named types. +- If verbose or repeated inline shapes remain after the PR, report that as an `Issue`. + +### React And UI + +- Prefer straight-line render logic, guard clauses, and early returns over deep nesting. +- Prefer controlled components where practical. +- Use semantic HTML like `
`, `