Conversation
Pull Request Review - PR #254SummaryThis PR performs a significant refactoring of the skills/configuration structure:
Code Quality Assessment✅ Strengths
|
There was a problem hiding this comment.
Pull request overview
This PR appears to migrate agent/project guidance away from Claude/OpenCode-specific config (CLAUDE.md, .claude/, .opencode/) toward the repo’s .agents/-based skills and documentation, while adding a new “vercel-composition-patterns” skill set.
Changes:
- Remove Claude/OpenCode configuration and bundled skill documents (
CLAUDE.md,.claude/,.opencode/skill/...). - Add a new
.agents/skills/vercel-composition-patternsskill (rules + compiledAGENTS.md). - Slightly edit
.agents/skills/vercel-react-best-practices/AGENTS.mdwording.
Reviewed changes
Copilot reviewed 70 out of 70 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| CLAUDE.md | Removes Claude-specific project instructions (now duplicated in root AGENTS.md in the repo). |
| .opencode/skills/web-design-guidelines | Removes OpenCode skill pointer. |
| .opencode/skills/vercel-react-best-practices | Removes OpenCode skill pointer. |
| .opencode/skill/web-design-guidelines/SKILL.md | Removes OpenCode skill definition/content. |
| .opencode/skill/vercel-react-best-practices/rules/server-serialization.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/server-parallel-fetching.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/server-cache-react.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/server-cache-lru.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/server-after-nonblocking.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-transitions.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-memo.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-lazy-state-init.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-functional-setstate.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-derived-state.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-dependencies.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rerender-defer-reads.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-svg-precision.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-hydration-no-flicker.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-hoist-jsx.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-content-visibility.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-conditional-render.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-animate-svg-wrapper.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/rendering-activity.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-tosorted-immutable.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-set-map-lookups.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-min-max-loop.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-length-check-first.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-index-maps.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-hoist-regexp.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-early-exit.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-combine-iterations.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-cache-storage.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-cache-property-access.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-cache-function-results.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/js-batch-dom-css.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/client-swr-dedup.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/client-event-listeners.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/bundle-preload.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/bundle-dynamic-imports.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/bundle-defer-third-party.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/bundle-conditional.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/bundle-barrel-imports.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/async-suspense-boundaries.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/async-parallel.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/async-dependencies.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/async-defer-await.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/async-api-routes.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/advanced-use-latest.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/advanced-event-handler-refs.md | Removes OpenCode rule content. |
| .opencode/skill/vercel-react-best-practices/rules/_template.md | Removes OpenCode rule template. |
| .opencode/skill/vercel-react-best-practices/rules/_sections.md | Removes OpenCode sections metadata. |
| .opencode/skill/vercel-react-best-practices/metadata.json | Removes OpenCode skill metadata. |
| .opencode/skill/vercel-react-best-practices/SKILL.md | Removes OpenCode skill overview. |
| .opencode/skill/vercel-react-best-practices/README.md | Removes OpenCode skill README. |
| .opencode/skill/vercel-react-best-practices/AGENTS.md | Removes OpenCode compiled guide. |
| .claude/skills/web-design-guidelines | Removes Claude skill pointer. |
| .claude/skills/vercel-react-best-practices | Removes Claude skill pointer. |
| .claude/settings.json | Removes Claude settings. |
| .claude/commands/work.md | Removes Claude command helper. |
| .agents/skills/vercel-react-best-practices/AGENTS.md | Small wording change in existing .agents skill content. |
| .agents/skills/vercel-composition-patterns/rules/state-lift-state.md | Adds composition/state-lifting guidance (new). |
| .agents/skills/vercel-composition-patterns/rules/state-decouple-implementation.md | Adds provider-vs-UI separation guidance (new). |
| .agents/skills/vercel-composition-patterns/rules/state-context-interface.md | Adds context interface/DI guidance (new). |
| .agents/skills/vercel-composition-patterns/rules/react19-no-forwardref.md | Adds React 19 notes (new). |
| .agents/skills/vercel-composition-patterns/rules/patterns-explicit-variants.md | Adds explicit variant pattern (new). |
| .agents/skills/vercel-composition-patterns/rules/patterns-children-over-render-props.md | Adds children-over-render-props guidance (new). |
| .agents/skills/vercel-composition-patterns/rules/architecture-compound-components.md | Adds compound components guidance (new). |
| .agents/skills/vercel-composition-patterns/rules/architecture-avoid-boolean-props.md | Adds avoid-boolean-props guidance (new). |
| .agents/skills/vercel-composition-patterns/SKILL.md | Adds skill metadata/entrypoint (new). |
| .agents/skills/vercel-composition-patterns/AGENTS.md | Adds compiled composition-patterns guide (new). |
| onChange={(text) => sync.updateInput(text)} | ||
| /> | ||
| <Composer.Submit onPress={() => sync.submit()} /> |
There was a problem hiding this comment.
In this “Incorrect” example, sync.updateInput / sync.submit are used but sync is never defined (the snippet destructures submit/updateInput instead). This will not compile as written; update the snippet and then regenerate this compiled AGENTS.md so it stays consistent.
| onChange={(text) => sync.updateInput(text)} | |
| /> | |
| <Composer.Submit onPress={() => sync.submit()} /> | |
| onChange={(text) => updateInput(text)} | |
| /> | |
| <Composer.Submit onPress={() => submit()} /> |
| function ChannelComposer({ channelId }: { channelId: string }) { | ||
| // UI component knows about global state implementation | ||
| const state = useGlobalChannelState(channelId) | ||
| const { submit, updateInput } = useChannelSync(channelId) |
There was a problem hiding this comment.
In the “Incorrect” example, useChannelSync() is destructured into submit and updateInput, but those bindings are never used. This makes the example inconsistent and also hides the actual coupling being demonstrated; use the destructured functions directly or rename the returned object and use that consistently.
| const { submit, updateInput } = useChannelSync(channelId) | |
| const sync = useChannelSync(channelId) |
| <ComposerContext value={{ state, actions, meta }}> | ||
| {children} | ||
| </ComposerContext> |
There was a problem hiding this comment.
The context provider usage is incorrect: createContext() returns a Context object; you need to render <ComposerContext.Provider value={...}>, not <ComposerContext value={...}>. As written, the example won’t compile.
| <ComposerContext value={{ state, actions, meta }}> | |
| {children} | |
| </ComposerContext> | |
| <ComposerContext.Provider value={{ state, actions, meta }}> | |
| {children} | |
| </ComposerContext.Provider> |
|
|
||
| return ( | ||
| <ComposerContext | ||
| value={{ | ||
| state, | ||
| actions: { update, submit }, | ||
| meta: { inputRef }, | ||
| }} | ||
| > | ||
| {children} |
There was a problem hiding this comment.
Same issue here: <ComposerContext value={...}> should be <ComposerContext.Provider value={...}> so the context is correctly provided.
| <ComposerContext value={{ state, actions, meta }}> | ||
| {children} | ||
| </ComposerContext> |
There was a problem hiding this comment.
This snippet uses <ComposerContext value={...}>, but to provide context you need <ComposerContext.Provider value={...}>. Since this file is the compiled/consumed guide, it should be regenerated after fixing the source rule content so readers don’t copy broken code.
| <ComposerContext value={{ state, actions, meta }}> | |
| {children} | |
| </ComposerContext> | |
| <ComposerContext.Provider value={{ state, actions, meta }}> | |
| {children} | |
| </ComposerContext.Provider> |
| onChange={(text) => sync.updateInput(text)} | ||
| /> | ||
| <Composer.Submit onPress={() => sync.submit()} /> |
There was a problem hiding this comment.
sync is referenced (sync.updateInput, sync.submit) but no sync variable is defined in this snippet; it will not compile as written. Use the destructured updateInput/submit functions (or assign const sync = useChannelSync(channelId) and use sync.*).
| onChange={(text) => sync.updateInput(text)} | |
| /> | |
| <Composer.Submit onPress={() => sync.submit()} /> | |
| onChange={(text) => updateInput(text)} | |
| /> | |
| <Composer.Submit onPress={() => submit()} /> |
| const submit = useForwardMessage() | ||
|
|
||
| return ( | ||
| <ComposerContext | ||
| value={{ | ||
| state, | ||
| actions: { update: setState, submit }, | ||
| meta: { inputRef }, | ||
| }} |
There was a problem hiding this comment.
This provider example uses <ComposerContext value={...}>, but it should be <ComposerContext.Provider value={...}> to actually provide context. Otherwise the snippet won’t compile / won’t provide the value to consumers.
| } | ||
|
|
||
| function ForwardButton() { | ||
| const { actions } = use(Composer.Context) |
There was a problem hiding this comment.
use(Composer.Context) is referenced, but the docs elsewhere define the context as ComposerContext (and the example provider is <Composer.Provider ...>). Unless Composer.Context is an actual exported context API, this looks like a typo/inconsistency that will confuse readers; use the same context identifier consistently (e.g., use(ComposerContext)).
| const { actions } = use(Composer.Context) | |
| const { actions } = use(ComposerContext) |
| In React 19, `ref` is now a regular prop (no `forwardRef` wrapper needed), and `use()` replaces `useContext()`. | ||
|
|
||
| **Incorrect (forwardRef in React 19):** | ||
|
|
||
| ```tsx |
There was a problem hiding this comment.
The text labels forwardRef and useContext() as “Incorrect” in React 19, but both APIs remain valid/supported; React 19 adds alternatives (ref as a prop, and use(Context)), it doesn’t make the old APIs wrong. Consider rewording to “Prefer …” / “Alternative …” to avoid spreading misinformation in the skill docs.
No description provided.