diff --git a/.claude/skills/comment-rules/SKILL.md b/.claude/skills/comment-rules/SKILL.md index 25d5ac7e..a82c562b 100644 --- a/.claude/skills/comment-rules/SKILL.md +++ b/.claude/skills/comment-rules/SKILL.md @@ -24,7 +24,26 @@ These rules do not apply to unnamed inline callbacks passed directly to framewor ## Params, returns, and throws -Omit `@param` and `@returns` unless they add information the signature and type do not already convey - units, nullability meaning, ownership, side effects, or constraints. Never write `@param name - the name`. An exception: if any single method parameter deserves a comment, document all the parameters of that method. +Omit `@param` and `@returns` when they only restate the signature and type. Keep them when they carry something the declaration cannot - units, nullability meaning, ownership, side effects, constraints, and anything else in that spirit. That list is illustrative, not a checklist to match against. Never write `@param name - the name`. + +**All-or-nothing, and this overrides the omit rule above:** never document only some of a method's parameters. A block with one `@param` on a three-parameter method reads as an oversight - the reader cannot tell whether the other two were judged self-evident or simply forgotten. So when one parameter deserves a note, pick one of: + +- **Document every parameter**, including ones you would otherwise omit. Use this when each has something real to say. Write what each contributes to the call - never pad with `@param name - the name`. +- **Fold the note into the summary prose** and drop the `@param` entirely. Use this when the other parameters would only restate the signature, so documenting them all would mean writing exactly the padding the rule above forbids. + +Prefer whichever leaves the block honest. Do not resolve the tension by leaving the lone `@param` in place. + +A `@returns` earns its place whenever the returned value is more than its type. Keep it when it names: + +- a **fixed or constrained value** - always `false`, an empty array rather than `undefined` +- a **transformation** the type doesn't show - normalized, sorted, deduped, cloned +- something **synthesized** by the call - a generated id, a defaulted field +- a **test-visible contract** a fixture guarantees - a `data-testid`, a specific element shape +- **which** of several same-typed results comes back, or what an ambiguous boolean means + +Delete a `@returns` that only re-says the type: `@returns The render result.` on a function returning `RenderResult`, `@returns Nothing.` on `void`, `@returns JSX element.` on a component. + +When a `@returns` is genuinely borderline, keep it. A redundant line costs a reader a second; a deleted one can cost them a trip into the body. Include `@throws` for every error condition the caller must handle; omit it when the function never throws. diff --git a/__mocks__/lucide-react.tsx b/__mocks__/lucide-react.tsx index 5cf52279..284c86e7 100644 --- a/__mocks__/lucide-react.tsx +++ b/__mocks__/lucide-react.tsx @@ -7,9 +7,6 @@ import type { ReactElement } from 'react'; /** * Stub for the LocateFixed icon; renders a bare SVG element so tests can locate the icon by test * ID. - * - * @param props - SVG props forwarded from the component, including optional className. - * @returns A ReactElement SVG element used as a locate-fixed icon stub in tests. */ export function LocateFixed(props: Readonly<{ className?: string }>): ReactElement { return ; @@ -17,9 +14,6 @@ export function LocateFixed(props: Readonly<{ className?: string }>): ReactEleme /** * Stub for the Info icon. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as an info icon stub in tests. */ export function Info(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -27,9 +21,6 @@ export function Info(props: Readonly<{ size?: number; className?: string }>): Re /** * Stub for the Trash2 icon. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as a trash icon stub in tests. */ export function Trash2(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -37,9 +28,6 @@ export function Trash2(props: Readonly<{ size?: number; className?: string }>): /** * Stub for the X icon. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as an X icon stub in tests. */ export function X(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -47,9 +35,6 @@ export function X(props: Readonly<{ size?: number; className?: string }>): React /** * Stub for the Link2 (link) icon used by the between-token link button. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as a link icon stub in tests. */ export function Link2(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -57,9 +42,6 @@ export function Link2(props: Readonly<{ size?: number; className?: string }>): R /** * Stub for the Link2Off (unlink) icon used by the arc-split button. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as an unlink icon stub in tests. */ export function Link2Off(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -68,9 +50,6 @@ export function Link2Off(props: Readonly<{ size?: number; className?: string }>) /** * Stub for the Unlink2 icon used by the between-token unlink button; a broken-chain glyph whose * chain sits at the same vertical position as Link2 so their button centers line up. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as an unlink icon stub in tests. */ export function Unlink2(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -78,9 +57,6 @@ export function Unlink2(props: Readonly<{ size?: number; className?: string }>): /** * Stub for the Settings gear icon used by the view-options dropdown button. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as a settings icon stub in tests. */ export function Settings(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -88,9 +64,6 @@ export function Settings(props: Readonly<{ size?: number; className?: string }>) /** * Stub for the Plus icon used by the token chip's suggestion-dropdown toggle. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as a plus icon stub in tests. */ export function Plus(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; @@ -99,9 +72,6 @@ export function Plus(props: Readonly<{ size?: number; className?: string }>): Re * Stub for the Merge icon used by both merge controls (the row-gap merge in the segment list and the * cross-segment merge in the continuous strip). A single Y-join glyph, deliberately a different * shape from the split marker's arrows-apart glyph. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as a merge icon stub in tests. */ export function Merge(props: Readonly<{ className?: string }>): ReactElement { return ; @@ -110,9 +80,6 @@ export function Merge(props: Readonly<{ className?: string }>): ReactElement { /** * Stub for the Split icon used by the Alt-gated split markers (token-chip and baseline-text). One * stroke diverging into two — the mirror of the `Merge` glyph's join. - * - * @param props - SVG props forwarded from the component. - * @returns A ReactElement SVG element used as a split-marker icon stub in tests. */ export function Split(props: Readonly<{ size?: number; className?: string }>): ReactElement { return ; diff --git a/__mocks__/papi-frontend-react.ts b/__mocks__/papi-frontend-react.ts index 042c4e09..181102cc 100644 --- a/__mocks__/papi-frontend-react.ts +++ b/__mocks__/papi-frontend-react.ts @@ -35,9 +35,6 @@ const useProjectData = jest.fn(() => * Mock for `useProjectSetting`. Returns `[defaultState, setSetting, resetSetting, isLoading]`, * passing `defaultState` through unchanged so callers receive a predictable initial value. * - * @param _projectDataProviderSource - Ignored project data provider source. - * @param _key - Ignored setting key. - * @param defaultState - Value surfaced as the current setting state. * @returns Tuple of `[defaultState, jest.fn(), jest.fn(), false]`. */ const useProjectSetting = jest @@ -53,7 +50,6 @@ const useProjectSetting = jest * Mock for `useLocalizedStrings`. Maps each requested key to itself so tests receive a * predictable `Record` without a real localization service. * - * @param keys - BCP47-style string keys to resolve. * @returns Tuple of `[record, isLoading]` where every key maps to itself and `isLoading` is * `false`. */ @@ -66,8 +62,6 @@ const useLocalizedStrings = jest.fn().mockImplementation((keys: string[]) => [ * Mock for `useSetting`. Returns `[defaultState, setSetting, resetSetting, false]`, passing * `defaultState` through unchanged so callers receive a predictable initial value. * - * @param _key - Ignored setting key. - * @param defaultState - Value surfaced as the current setting state. * @returns Tuple of `[defaultState, jest.fn(), jest.fn(), false]`. */ const useSetting = jest @@ -82,8 +76,6 @@ const useSetting = jest /** * Mock for `useRecentScriptureRefs`. Returns an empty history and a no-op `addRecentScriptureRef` * so components that display recent references render without errors. - * - * @returns Object with `recentScriptureRefs` (empty array) and `addRecentScriptureRef` (jest spy). */ const useRecentScriptureRefs = jest .fn() diff --git a/__mocks__/platform-bible-react.tsx b/__mocks__/platform-bible-react.tsx index 612fd4d1..5f12410f 100644 --- a/__mocks__/platform-bible-react.tsx +++ b/__mocks__/platform-bible-react.tsx @@ -116,14 +116,8 @@ export const MOCK_WIPE_MENU_ITEM: MenuItemContainingCommand = { * Stub toolbar that renders project-menu and view-info buttons using sentinel menu items so tests * can trigger menu commands without a real toolbar implementation. * - * @param props - Component props. - * @param props.startAreaChildren - Content rendered in the start slot. - * @param props.endAreaChildren - Content rendered in the end slot. - * @param props.onSelectProjectMenuItem - Called with a sentinel item when a project-menu button is - * clicked (select-project, new-project, or view-project-info buttons). - * @param props.onSelectViewInfoMenuItem - Called with a generic sentinel item when the view-info - * button is clicked. - * @returns A div with `data-testid="tab-toolbar"` containing the rendered buttons. + * @returns A `data-testid="tab-toolbar"` container; its buttons carry `tab-toolbar-`-prefixed ids + * naming the command each one sends. */ export function TabToolbar({ startAreaChildren, @@ -225,11 +219,7 @@ export function TabToolbar({ * Stub scroll-group selector rendered as a native `` element. + * @returns A `data-testid="scroll-group-selector"` `` element with the forwarded attributes. */ export const Input = forwardRef< HTMLInputElement, @@ -476,18 +427,6 @@ export const Input = forwardRef< /** * Stub textarea rendered as a native `