From 66692e9517381acba9c26e849809da62cdc9b044 Mon Sep 17 00:00:00 2001 From: Lifei Zhou Date: Thu, 30 Apr 2026 14:20:56 +1000 Subject: [PATCH] chore: goose2 skill refactor (#8897) --- .agents/skills/ui-refactor-review/SKILL.md | 250 ------------ ui/goose2/src/features/skills/api/skills.ts | 49 +-- .../skills/hooks/useSkillImportExport.ts | 33 ++ .../features/skills/lib/skillCategories.ts | 14 +- .../src/features/skills/lib/skillsHelpers.ts | 141 ++++++- .../src/features/skills/lib/skillsPath.ts | 49 +++ .../skills/ui/SkillCategoryFilter.tsx | 93 +++++ .../features/skills/ui/SkillDetailPage.tsx | 30 +- ...{CreateSkillDialog.tsx => SkillEditor.tsx} | 57 +-- .../src/features/skills/ui/SkillsDialogs.tsx | 22 +- .../features/skills/ui/SkillsListSections.tsx | 7 +- .../src/features/skills/ui/SkillsToolbar.tsx | 107 +++++ .../src/features/skills/ui/SkillsView.tsx | 380 +++--------------- ...llDialog.test.tsx => SkillEditor.test.tsx} | 58 ++- .../skills/ui/__tests__/SkillsView.test.tsx | 10 - .../src/shared/i18n/locales/en/skills.json | 6 + .../src/shared/i18n/locales/es/skills.json | 6 + 17 files changed, 553 insertions(+), 759 deletions(-) delete mode 100644 .agents/skills/ui-refactor-review/SKILL.md create mode 100644 ui/goose2/src/features/skills/hooks/useSkillImportExport.ts create mode 100644 ui/goose2/src/features/skills/lib/skillsPath.ts create mode 100644 ui/goose2/src/features/skills/ui/SkillCategoryFilter.tsx rename ui/goose2/src/features/skills/ui/{CreateSkillDialog.tsx => SkillEditor.tsx} (82%) create mode 100644 ui/goose2/src/features/skills/ui/SkillsToolbar.tsx rename ui/goose2/src/features/skills/ui/__tests__/{CreateSkillDialog.test.tsx => SkillEditor.test.tsx} (88%) diff --git a/.agents/skills/ui-refactor-review/SKILL.md b/.agents/skills/ui-refactor-review/SKILL.md deleted file mode 100644 index 623e68c804..0000000000 --- a/.agents/skills/ui-refactor-review/SKILL.md +++ /dev/null @@ -1,250 +0,0 @@ ---- -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 `
`, `