Conversation
There was a problem hiding this comment.
Pull request overview
Adds a GitHub Copilot instruction set and a set of “skills” documents intended to guide consistent development practices in this Vue utilities monorepo.
Changes:
- Add
.github/copilot-instructions.mdas the entrypoint for Copilot guidance. - Add multiple
.github/skills/*.mddocuments covering testing, state management, types, workflows, debugging, and validation. - Update
oxfmtconfig to ignore the new Copilot/skills documentation paths.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| .oxfmtrc.json | Expands formatter ignore patterns to exclude Copilot/skills docs. |
| .github/copilot-instructions.md | New top-level Copilot guidance and links into the skills docs. |
| .github/skills/common-workflows.md | New workflow playbook for feature work, plugins, testing, and releases. |
| .github/skills/common-pitfalls.md | New “do/don’t” guidance for recurring repo pitfalls. |
| .github/skills/debugging-strategies.md | New debugging guide for Vitest, browser tests, and lint/format issues. |
| .github/skills/instruction-validation.md | New process doc for keeping instructions/examples aligned with the codebase. |
| .github/skills/package-structure.md | New conventions doc for exports, file layout, and dependency protocols. |
| .github/skills/state-management.md | New state-management patterns doc (module singletons, cleanup, debouncing). |
| .github/skills/testing-decisions.md | New decision tree for choosing unit vs browser tests and test structure. |
| .github/skills/type-patterns.md | New TypeScript patterns doc (type guards, generics, ambient decls, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/skills/common-pitfalls.md
Outdated
| | No cleanup in composables | Memory leaks on unmount | Use `onScopeDispose()` for auto-cleanup | | ||
| | Import cycles | Build failures | Check `.oxlintrc.json` enforces `import/no-cycle` | | ||
| | Mixing test environments | Tests fail in wrong environment | Use `.unit.test.ts` or `.browser.test.ts` suffix | | ||
| | Using vitest hooks | Implicit dependencies, hidden failures | Use explicit setup/teardown functions | |
There was a problem hiding this comment.
The quick-reference table marks “Using vitest hooks” as a pitfall, but the repo currently uses beforeEach in unit tests (e.g., packages/vuebugger/src/registry.unit.test.ts, devtools.unit.test.ts) to clear module-level state. Consider rewording this to a narrower, accurate rule (e.g., avoid hooks where they hide setup, but allow them for singleton reset) or update the tests to match the guidance.
| | Using vitest hooks | Implicit dependencies, hidden failures | Use explicit setup/teardown functions | | |
| | Overusing vitest hooks for setup | Implicit dependencies, hidden failures | Use hooks only for global/singleton reset; keep test-specific setup inline in each test | |
.github/skills/state-management.md
Outdated
| export const byUid = new Map<uid, VuebuggerEntry>() | ||
| export const byGroupId = new Map<groupId, Set<uid>>() | ||
|
|
There was a problem hiding this comment.
This example uses uid/groupId type names (Map<uid, ...>) that don’t exist in the current codebase (vuebugger uses VuebuggerEntry['uid'] / ['groupId']). To keep the docs copy/pasteable and accurate, align the types with packages/vuebugger/src/registry.ts.
.github/skills/common-workflows.md
Outdated
|
|
||
| ```typescript | ||
| export const vueltipPlugin = { | ||
| install: (app: App, options?: Partial<Options & { component: Component }>) => { |
There was a problem hiding this comment.
In this snippet options is optional, but the code immediately destructures it (const { component, ...rest } = options), which would throw if install is called without options. Either make options required (matching the current packages/vueltip/src/plugin.ts signature) or default it to {} before destructuring.
| install: (app: App, options?: Partial<Options & { component: Component }>) => { | |
| install: (app: App, options: Partial<Options & { component: Component }>) => { |
| // "Example: [listeners.ts](../../packages/vueltip/src/listeners.ts)" | ||
|
|
||
| // Verify file exists and has example: | ||
| grep -A 5 "export const onMouseenter" packages/vueltip/src/listeners.ts |
There was a problem hiding this comment.
This validation command searches for export const onMouseenter, but packages/vueltip/src/listeners.ts exports onMouseover/onMouseout (so the grep will fail even when the example is present). Update the command to match the actual exported handler name(s).
| grep -A 5 "export const onMouseenter" packages/vueltip/src/listeners.ts | |
| grep -A 5 "export const onMouseover" packages/vueltip/src/listeners.ts |
.github/skills/type-patterns.md
Outdated
| | Branded type | Create distinct types | `uid` brand prevents mixing IDs | | ||
| | Ambient declaration | Extend Vue globally | `declare module 'vue'` | | ||
| | Generic constraint | Bind getter/setter types | `getOption<T extends keyof Options>` | | ||
| | Conditional type | Compute types | `T | null \| undefined` | |
There was a problem hiding this comment.
The “Conditional type” quick-reference example (T | null | undefined) is a union type, not a conditional type. Consider using an example that actually uses T extends ... ? ... : ... (you already have ExtractEvent<T> later) to avoid teaching the wrong concept.
| | Conditional type | Compute types | `T | null \| undefined` | | |
| | Conditional type | Compute types | `T extends string ? string[] : T[]` | |
.github/skills/state-management.md
Outdated
| ```typescript | ||
| export const byUid = new Map<uid, VuebuggerEntry>() | ||
| export const byGroupId = new Map<groupId, Set<uid>>() | ||
|
|
||
| export const upsert = (entry: VuebuggerEntry) => { | ||
| byUid.set(entry.uid, entry) | ||
| const group = byGroupId.get(entry.groupId) | ||
| if (!group) byGroupId.set(entry.groupId, new Set([entry.uid])) | ||
| else group.add(entry.uid) | ||
| } | ||
|
|
||
| export const remove = (uid: uid) => { | ||
| const entry = byUid.get(uid) | ||
| if (!entry) return | ||
| byUid.delete(uid) | ||
| byGroupId.get(entry.groupId)?.delete(uid) | ||
| } | ||
|
|
||
| // Optional: Callback pattern for listeners | ||
| const callbacks: ((entry: VuebuggerEntry) => void)[] = [] | ||
| const runCallbacks = (entry: VuebuggerEntry) => | ||
| callbacks.forEach((cb) => cb(entry)) | ||
| const withCallbacks = | ||
| (fn: (entry: VuebuggerEntry) => void) => | ||
| (entry: VuebuggerEntry) => { | ||
| fn(entry) | ||
| runCallbacks(entry) | ||
| } | ||
|
|
||
| export const upsert = withCallbacks((entry) => { | ||
| // ... upsert logic | ||
| }) |
There was a problem hiding this comment.
This code block defines export const upsert twice (first as a plain function, then reassigned via withCallbacks). As an example, it’s confusing and not copy/pasteable—either show only the final callback-wrapped version, or rename the intermediate function (e.g., upsertInternal) and demonstrate wrapping without redeclaring the export.
| ```typescript | ||
| export const hoveredElement = ref<Maybe<HTMLElement>>() | ||
| export const contentMap = ref(new Map<string, Content>()) | ||
|
|
||
| // Getter function | ||
| export const getContent = (key: string) => | ||
| contentMap.value.get(key) | ||
|
|
||
| // Setter function | ||
| export const setContent = (key: string, value: Content) => | ||
| contentMap.value.set(key, value) | ||
|
|
||
| export const deleteContent = (key: string) => | ||
| contentMap.value.delete(key) | ||
| ``` |
There was a problem hiding this comment.
Pattern 2’s example exports contentMap, but the current implementation in packages/vueltip/src/state.ts keeps contentMap internal and only exports getContent/setContent/deleteContent. Also, the “Critical” bullets say to avoid direct Map access. Update the snippet to match the real pattern (non-exported contentMap + exported accessors).
.github/skills/package-structure.md
Outdated
| ```typescript | ||
| // ✅ index.ts - Public API only | ||
| export { debug } from './debug' | ||
| export { plugin as DebugPlugin } from './index' |
There was a problem hiding this comment.
The vuebugger index.ts example shows export { plugin as DebugPlugin } from './index', which would self-import and is not how packages/vuebugger/src/index.ts is implemented (it exports a local plugin as DebugPlugin). Please update the example to match the actual file and avoid suggesting a circular/self import.
| export { plugin as DebugPlugin } from './index' | |
| export const DebugPlugin = plugin |
.oxfmtrc.json
Outdated
| "ignorePatterns": [".changeset/**"], | ||
| "ignorePatterns": [ | ||
| ".changeset/**", | ||
| ".github/skills/*", |
There was a problem hiding this comment.
ignorePatterns uses .github/skills/*, which only matches files directly under that folder. For consistency with the existing .changeset/** pattern (and to ensure future nested paths are ignored), use .github/skills/** instead.
| ".github/skills/*", | |
| ".github/skills/**", |
| **Step 3: Verify setup/teardown** | ||
| ```typescript | ||
| beforeEach(() => { | ||
| byUid.clear() // Are we clearing? | ||
| byGroupId.clear() | ||
| }) | ||
| ``` |
There was a problem hiding this comment.
This section shows beforeEach as the suggested setup/teardown pattern, which contradicts other new skills that label Vitest hooks as an anti-pattern. The repo should have one consistent recommendation (either allow hooks for unit tests like the current vuebugger tests do, or remove/replace this example).
13e84f3 to
264974d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Overusing vitest hooks for setup | Implicit dependencies, hidden failures | Use hooks only for global/singleton reset; keep test-specific setup inline in each test | | ||
| | Multiple timers stacking | Race conditions, doubled effects | Always `clearTimeout()` before setting new one | | ||
| | Inline functions in event listeners | Handlers created on every render | Store as module-level const, reference it | | ||
|
|
||
| --- | ||
|
|
||
| ## Detailed Pitfalls & Fixes | ||
|
|
||
| ### Pitfall 1: Inline Event Listeners | ||
|
|
||
| **Problem:** Event listeners created inline can't be removed because there's no reference. | ||
|
|
||
| ```typescript | ||
| // ❌ WRONG - Can't remove this listener | ||
| el.addEventListener('mouseenter', () => { | ||
| console.log('entered') | ||
| }) | ||
|
|
||
| // On unmount - which function do we remove? | ||
| el.removeEventListener('mouseenter', () => { | ||
| console.log('entered') | ||
| }) // Doesn't match! New function instance | ||
| ``` | ||
|
|
||
| **Solution:** Store handler as module-level const | ||
|
|
||
| ```typescript | ||
| // ✅ CORRECT - Store reference | ||
| const onMouseenter = () => { | ||
| console.log('entered') | ||
| } | ||
|
|
||
| // Add listener | ||
| el.addEventListener('mouseenter', onMouseenter) | ||
|
|
||
| // Remove listener - same reference | ||
| el.removeEventListener('mouseenter', onMouseenter) | ||
| ``` | ||
|
|
||
| **In directives:** | ||
|
|
||
| ```typescript | ||
| export const vueltipDirective = { | ||
| created: (el, binding) => { | ||
| // Store reference for removal | ||
| el.addEventListener('mouseenter', onMouseover) // onMouseover is stored const | ||
| el.addEventListener('mouseleave', onMouseout) | ||
| }, | ||
| beforeUnmount: (el) => { | ||
| // Remove same reference | ||
| el.removeEventListener('mouseenter', onMouseover) | ||
| el.removeEventListener('mouseleave', onMouseout) | ||
| }, | ||
| } | ||
| ``` | ||
|
|
||
| **Where to define handlers:** [listeners.ts](../../packages/vueltip/src/listeners.ts) - module-level, not inside hooks | ||
|
|
||
| --- | ||
|
|
||
| ### Pitfall 2: Forgetting to Remove Listeners | ||
|
|
||
| **Problem:** Listeners accumulate, tests fail, memory leaks in production. | ||
|
|
||
| ```typescript | ||
| // ❌ WRONG - No cleanup | ||
| it('attaches listener', () => { | ||
| const el = document.createElement('div') | ||
| el.addEventListener('mouseenter', onMouseover) | ||
| // End of test - listener still attached | ||
| // If you run 100 tests, 100 listeners still attached | ||
| }) | ||
| ``` | ||
|
|
||
| **Solution:** Always pair add/remove | ||
|
|
||
| ```typescript | ||
| // ✅ CORRECT - Explicit cleanup | ||
| const setupDirective = () => { | ||
| const el = document.createElement('div') | ||
| document.body.appendChild(el) | ||
| return el | ||
| } | ||
|
|
||
| const teardownDirective = (el: HTMLElement) => { | ||
| el.removeEventListener('mouseenter', onMouseover) | ||
| el.removeEventListener('mouseleave', onMouseout) |
There was a problem hiding this comment.
The guidance on using beforeEach is inconsistent and contradicts actual codebase practice. The documentation states "Use hooks only for global/singleton reset; keep test-specific setup inline in each test" (line 14), and lists "Using vitest hooks (beforeEach, afterEach) - prefer explicit setup/cleanup functions" as an anti-pattern (line 99).
However, the actual test files show that beforeEach IS used appropriately for clearing global/singleton state (packages/vuebugger/src/registry.unit.test.ts lines 19-23), which matches the exception mentioned on line 14. The anti-pattern guidance should be clarified to distinguish between:
- ✅ GOOD: Using
beforeEachfor clearing module-level singletons/Maps/global mocks - ❌ BAD: Using
beforeEachfor test-specific element setup/teardown
The current wording makes it sound like beforeEach should never be used, which contradicts both the codebase and the exception on line 14.
| const upsertInternal = (entry: VuebuggerEntry) => { | ||
| byUid.set(entry.uid, entry) | ||
| const group = byGroupId.get(entry.groupId) | ||
| if (!group) byGroupId.set(entry.groupId, new Set([entry.uid])) | ||
| else group.add(entry.uid) | ||
| } | ||
|
|
||
| export const remove = (entry: VuebuggerEntry) => { | ||
| const { uid, groupId } = entry | ||
| byUid.delete(uid) | ||
| const group = byGroupId.get(groupId) | ||
| group?.delete(uid) | ||
| if (group?.size === 0) byGroupId.delete(groupId) | ||
| } | ||
|
|
||
| // Callback pattern for listeners | ||
| const callbacks: ((entry: VuebuggerEntry) => void)[] = [] | ||
| const runCallbacks = (entry: VuebuggerEntry) => | ||
| callbacks.forEach((cb) => cb(entry)) | ||
| const withCallbacks = | ||
| (fn: (entry: VuebuggerEntry) => void) => | ||
| (entry: VuebuggerEntry) => { | ||
| fn(entry) | ||
| runCallbacks(entry) | ||
| } | ||
|
|
||
| export const upsert = withCallbacks(upsertInternal) |
There was a problem hiding this comment.
The code example shows upsertInternal as a named internal function that is then wrapped with withCallbacks, but the actual implementation in packages/vuebugger/src/registry.ts (lines 23-31) uses an inline anonymous function instead: export const upsert = withCallbacks((entry: VuebuggerEntry) => { ... }).
While the pattern is conceptually the same, showing a named upsertInternal function that doesn't exist in the actual code may confuse developers looking at the real implementation. Consider either updating the example to match the actual inline implementation, or noting that this is a simplified/refactored version of the pattern.
| **Example:** [Vuebugger types.ts](../../packages/vuebugger/src/types.ts) | ||
|
|
||
| ```typescript | ||
| // Prevent mixing different ID types | ||
| export type uid = string & { readonly __brand: 'uid' } | ||
| export type groupId = string & { readonly __brand: 'groupId' } | ||
|
|
||
| // Helper to create branded values | ||
| const uid = (value: string): uid => value as uid | ||
| const groupId = (value: string): groupId => value as groupId | ||
|
|
||
| // Now these are distinct types - can't mix them | ||
| const entry: VuebuggerEntry = { | ||
| uid: uid('component/state-1'), | ||
| groupId: groupId('module-state'), | ||
| } | ||
|
|
||
| // TypeScript prevents mixing: | ||
| // ❌ entry.uid = groupId('foo') // Error: Type 'groupId' is not assignable to type 'uid' | ||
| // ✅ entry.uid = uid('component/state-2') | ||
| ``` |
There was a problem hiding this comment.
The branded types example references uid and groupId as branded types with __brand properties, but the actual codebase (packages/vuebugger/src/types.ts) uses plain string types for these fields. The example code showing export type uid = string & { readonly __brand: 'uid' } and the helper functions do not exist in the referenced file.
Either update the example to match the actual codebase implementation (where uid and groupId are plain strings in the VuebuggerEntry type), or implement branded types in the codebase if this is a desired pattern. Currently, this is misleading documentation that references non-existent code.
264974d to
638709c
Compare
No description provided.