Skip to content
Merged
31 changes: 30 additions & 1 deletion .claude/skills/multi-tool-code-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,34 @@ Follow `04-fix-protocol.md`. If the harness supports plan mode, enter it first a
2. File GitHub issues (labeled) for every finding the user excluded, and for anything you deferred. Surface deferrals with a recommendation; never silently skip.
3. Offer a recap and to watch CI settle.

## Stage 4: Clean up after the cycle

A run leaves debris in three places: scratch output and triage docs under `reviews/`, the fix branch locally and on the remote, and a worktree if the review used one. Clear it once the PR is merged, so the next run starts from a clean detection.

**Never delete anything without asking first.** Everything here looks disposable and is not: a triage doc is the only record of the findings the user chose to skip, a fix branch may hold the only copy of unpushed commits, and disk state is what this skill's own resume detection reads. A user who has not finished reading the doc, or who wants to re-review next week, will not get any of it back. So propose, then wait.

Offer cleanup once, when the cycle is genuinely over: the PR merged, or the user says they are done. Do not offer it while a PR is open, and do not fold it into another question as a default-on extra.

Record what this cycle creates as it creates it: the exact review directory, report paths, branch names, their remote counterparts, and any worktree. That manifest, not the detection below, is what may be proposed for removal. The commands find everything matching a shape, including branches and worktrees from unrelated work that happen to be named the same way, so intersect their output with the manifest and keep anything whose owner you cannot establish, naming it in the question as retained.

Build the proposal by detecting what exists, then put it to the user with `AskUserQuestion`, one question per category, options built from what you actually found:

```bash
ls -d reviews/_review-run* reviews/_archive 2>/dev/null # scratch, safe to drop
ls reviews/REVIEW-*.md reviews/REVIEW-*.html 2>/dev/null # triage docs, the user's record
git worktree list # which are this run's
git branch --merged <base> | grep -E 'fix/.*review' # merged fix branches
git ls-remote --heads origin 'fix/*review*' # their remote counterparts
```

Rules that hold regardless of the answer:

- **Only ever propose what this cycle created.** Other branches and worktrees belong to unrelated in-flight work, and the host repo's `AGENTS.md` forbids touching it. List them in the question as explicitly excluded rather than leaving the user to wonder whether you swept them up.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
- **Scratch and reports are different questions.** `_review-run*` folders are pure working output and are the safe default to remove. `REVIEW-*.md` and `.html` are the deliverable; offer keeping them, archiving them, or deleting them, and default to keeping.
- **Never delete a branch with unpushed commits, or one associated with an open PR**, even if the user selects it, whether the branch is that PR's head or its base. Check `git log <remote>..<branch>`, then `gh pr list --head <branch>` and `gh pr list --base <branch>` as separate calls, since the two filters combine as AND rather than OR. Report back instead of deleting. If either check cannot be run, keep the branch and say why.
- **Say what a report is still referenced by.** A PR body that cites a triage doc by path leaves a dangling reference once it is gone. Mention it, then let the user decide.
- Report exactly what was removed and what was left standing.

## Conventions (apply throughout)

- Ask if you are unsure of anything rather than assuming. Follow the host repo's `AGENTS.md` / `CLAUDE.md` closely.
Expand All @@ -96,4 +124,5 @@ Follow `04-fix-protocol.md`. If the harness supports plan mode, enter it first a
- In a **worktree with symlinked `node_modules`**, do not use `pnpm exec` or `pnpm run`: both run a deps-status check, see the symlink as out of sync, and try to purge the main tree's real `node_modules` through it. Call the binary directly there instead. In a normal checkout `pnpm exec` is fine.
- If `pnpm` is not on PATH, prepend it: `export PATH="$HOME/.local/share/pnpm/bin:$HOME/.local/share/nvm/*/bin:$PATH"`.
- Never run the dev server or any deploy/Docker command. The production build is allowed, and Stage 3 expects it.
- Runner outputs and the triage doc live under `reviews/` (gitignored). Keep them out of commits; `git add` explicit files, never `-A`.
- Runner outputs and the triage doc live under `reviews/` (gitignored). Keep them out of commits; `git add` explicit files, never `-A`. The runners recreate the folder, so a deleted `reviews/` is not a broken state.
- Deleting artifacts, branches or worktrees is always a question for the user, never a tidy-up you perform on your own initiative. See Stage 4.
11 changes: 10 additions & 1 deletion docs/SHARING.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ Worth knowing before changing any of this:
| Level copy | `frontend/src/app/(user)/shopping-lists/utils/link-access-copy.ts` |
| Level icons | `frontend/src/app/(user)/shopping-lists/utils/link-access-icons.ts` |
| Copy modal | `frontend/src/app/(user)/shopping-lists/components/forms/copy-list-modal.tsx` |
| Copy state | `frontend/src/app/(user)/shopping-lists/hooks/use-copy-list-modal.ts` |
| Title field | `frontend/src/app/(user)/shopping-lists/components/forms/shopping-list-title-field.tsx` |
| Optimism | `frontend/src/lib/api/shopping-lists/optimistic-list.ts` |
| Client access | `frontend/src/app/(user)/shopping-lists/utils/shopping-list-access.ts` |

Expand All @@ -159,7 +161,14 @@ Worth knowing before changing any of this:
never directly.
- **Copying a list asks what to carry**, through `POST /api/shopping-lists/{id}/copy`.
Products default on; the ticks with their captured prices, and the sharing settings,
default off. One endpoint rather than a create followed by an add per item, because
default off. The name is an input rather than a fixed `<title> (Kopija)`, prefilled with
that suggestion so renaming and copying are one action. It shares
`shopping-list-title-field.tsx` with the create and edit modals, and the prefill shortens
the original so the suffix always fits inside the schema's 100-character ceiling, which
it reads off the schema rather than restating. It takes whole code points so the cut
cannot split an emoji, but budgets in UTF-16 units, because that is what zod's `max()`
counts. One endpoint rather than a
create followed by an add per item, because
those were separate transactions: a failure partway left a half-populated copy behind
that no retry could tidy up, and pressing the button again made another one.
- **The sharing option on a copy is owner-only, enforced server side.** A recipient could
Expand Down
2 changes: 1 addition & 1 deletion docs/STATE-PERSISTENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ The URL and localStorage layers use only browser-native APIs; there is no extra

- **Old drafts are type-guarded on restore.** If a field's type changed since a draft was written (for example a number where the field is now a string), the restore skips it so a stale draft cannot poison validation. Keys the form no longer has at all are skipped too, so renaming or splitting a field cannot strand a dead entry for the rest of the TTL.

- **A prefill has to become the form's `defaultValue`, or the draft engine saves it as a change.** Drafts diff against the defaults, so seeding a server-loaded value with `setValue` writes a draft for a number the user never typed, and makes the reset button offer to clear a field the user never touched. Seed with `reset(values, { keepDirtyValues: true })`, which updates values and defaults together and leaves in-progress edits alone. `resetField` does the same thing for one field but only works on a field that is registered right now, so it silently does nothing while the modal is still loading or for a field the current branch does not render (the watchlist modal renders only the selected watch mode). Note that `keepDirtyValues` keeps the dirty flags as they were rather than recomputing them, so a flag can outlive the edit that set it: gate buttons on a value-vs-baseline comparison, not on `dirtyFields`.
- **A prefill has to become the form's `defaultValue`, or the draft engine saves it as a change.** Drafts diff against the defaults, so seeding a server-loaded value with `setValue` writes a draft for a number the user never typed, and makes the reset button offer to clear a field the user never touched. Seed with `reset(values, { keepDirtyValues: true })`, which updates values and defaults together and leaves in-progress edits alone. `resetField` does the same thing for one field but only works on a field that is registered right now, so it silently does nothing while the modal is still loading or for a field the current branch does not render (the watchlist modal renders only the selected watch mode). Note that `keepDirtyValues` keeps the dirty flags as they were rather than recomputing them, so a flag can outlive the edit that set it: gate buttons on a value-vs-baseline comparison, not on `dirtyFields`. `formState.isValid` is stale after seeding for the same kind of reason, since it only refreshes when the resolver runs and a seeding `reset` does not run it, so a form that is prefilled with a valid value reads as invalid until an unrelated change triggers a pass. Where an untouched prefill has to be submittable straight away, parse the schema yourself (`schema.safeParse(values).success`) and gate on that; the resolver still owns the messages under the fields. Most modals hide this by gating on `!isDirty || !isValid`, where the dirty term disables the button anyway.

- **Closing mid-debounce still saves, but a submit never re-persists.** The watch effect's cleanup flushes the last keystrokes on unmount unless the form is submitting or submitted. The `isSubmitting` guard matters for the optimistic-close pattern: the modal unmounts before the mutation resolves and `clearDraft` runs, so without it a late flush could rewrite a draft that was just cleared and a reopen would show stale data.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
import { Copy, ListChecks, Share2, ShoppingBasket } from "lucide-react";

import { ModalShell } from "@/components/custom/modal/modal-shell";
import { Form } from "@/components/ui/form";
import { Skeleton } from "@/components/ui/skeleton";
import { LOADING_LABELS } from "@/constants/loading-labels";
import { closeModalUrl } from "@/lib/modal/modal-navigation";
import { useCopyListModal } from "@/app/(user)/shopping-lists/hooks/use-copy-list-modal";
import CopyOptionRow from "@/app/(user)/shopping-lists/components/forms/copy-option-row";
import ShoppingListTitleField from "@/app/(user)/shopping-lists/components/forms/shopping-list-title-field";

interface ICopyListModalProps {
open: boolean;
Expand All @@ -20,6 +22,8 @@ export default function CopyListModal({ open, id }: ICopyListModalProps) {
shoppingList,
isLoading,
isError,
form,
isValid,
options,
setOption,
canCopySharing,
Expand All @@ -35,11 +39,11 @@ export default function CopyListModal({ open, id }: ICopyListModalProps) {
description="Odaberi što se prenosi u kopiju."
size="sm"
preventClose={isCopying}
formId="copy-list-form"
submitLabel={isCopying ? LOADING_LABELS.copying : "Kopiraj"}
submitIcon={Copy}
submitLoading={isCopying}
submitDisabled={isLoading || isError || !shoppingList}
onSubmit={() => void copyList()}
submitDisabled={isLoading || isError || !shoppingList || !isValid}
cancelLabel="Odustani"
>
{isLoading ? (
Expand All @@ -53,45 +57,50 @@ export default function CopyListModal({ open, id }: ICopyListModalProps) {
Popis nije pronađen. Možda je obrisan ili nemaš pristup.
</p>
) : (
<div className="space-y-4">
<CopyOptionRow
icon={ShoppingBasket}
label="Proizvodi"
description="Prenesi sve proizvode s popisa."
checked={options.items}
onCheckedChange={(next) => setOption("items", next)}
/>
<Form {...form}>
<form
id="copy-list-form"
onSubmit={form.handleSubmit((data) => void copyList(data))}
className="space-y-4"
>
{/* No autofocus: the field mounts only once the list query settles, so
focusing it would land mid-announcement and, on mobile, open the
keyboard over the options this modal is actually about. */}
<ShoppingListTitleField control={form.control} autoFocus={false} />

<CopyOptionRow
icon={ListChecks}
label="Označeno i spremljene cijene"
description="Zadrži što je kupljeno, u kojoj trgovini i po kojoj cijeni."
// Coerced, not just disabled: a box that stays ticked while inert states a
// choice the copy will not honour.
checked={options.progress && options.items}
disabled={!options.items}
onCheckedChange={(next) => setOption("progress", next)}
/>
<CopyOptionRow
icon={ShoppingBasket}
label="Proizvodi"
description="Kopiraj sve proizvode"
checked={options.items}
onCheckedChange={(next) => setOption("items", next)}
/>

<CopyOptionRow
icon={Share2}
label="Postavke dijeljenja"
description={
canCopySharing
? "Kopija se dijeli jednako kao i original."
: "Samo vlasnik popisa može prenijeti dijeljenje."
}
checked={options.sharing && canCopySharing}
disabled={!canCopySharing}
onCheckedChange={(next) => setOption("sharing", next)}
/>
<CopyOptionRow
icon={ListChecks}
label="Označeno i spremljene trgovine"
description="Zadrži što je kupljeno, u kojoj trgovini i po kojoj cijeni."
// Coerced, not just disabled: a box that stays ticked while inert states a
// choice the copy will not honour.
checked={options.progress && options.items}
disabled={!options.items}
onCheckedChange={(next) => setOption("progress", next)}
/>

{!options.items && (
<p className="text-xs text-muted-foreground">
Kopirat će se samo naziv popisa.
</p>
)}
</div>
<CopyOptionRow
icon={Share2}
label="Postavke dijeljenja"
description={
canCopySharing
? "Kopija se dijeli jednako kao i original."
: "Samo vlasnik popisa može prenijeti dijeljenje."
}
checked={options.sharing && canCopySharing}
disabled={!canCopySharing}
onCheckedChange={(next) => setOption("sharing", next)}
/>
</form>
</Form>
)}
</ModalShell>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ export default function CopyOptionRow({
: "bg-muted text-muted-foreground",
)}
>
<Icon aria-hidden="true" className="size-5" />
{/* size-6 matches the share modal's access row, the bottom nav glyph and the
list card's visibility indicator, so the same icons read at one size. */}
<Icon aria-hidden="true" className="size-6" />
</span>

{/* The description sits outside the label on purpose. Accessible-name computation
Expand Down Expand Up @@ -74,7 +76,9 @@ export default function CopyOptionRow({
disabled ? undefined : (next) => onCheckedChange(next === true)
}
aria-describedby={descriptionId}
className="size-6 shrink-0 [&_svg]:size-4"
// A step below the default size-10 the list items use: these are settings on a
// form row, not the primary target of the screen.
className="size-8 shrink-0 [&_svg]:size-6"
/>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,9 @@ import { Save } from "lucide-react";

import { ModalShell } from "@/components/custom/modal/modal-shell";
import { resolveShoppingListAccess } from "@/app/(user)/shopping-lists/utils/shopping-list-access";
import { Input } from "@/components/ui/input";
import { Skeleton } from "@/components/ui/skeleton";
import {
Form,
FormControl,
FormField,
FormItem,
FormLabel,
FormMessage,
} from "@/components/ui/form";
import { Form } from "@/components/ui/form";
import ShoppingListTitleField from "@/app/(user)/shopping-lists/components/forms/shopping-list-title-field";
import type { ShoppingListDto, ShoppingListRequest } from "@/lib/api/types";
import { shoppingListRequestSchema } from "@/lib/api/types";
import { shoppingListService } from "@/lib/api";
Expand Down Expand Up @@ -177,23 +170,7 @@ export default function ShoppingListModal({
</div>
)}

<FormField
control={form.control}
name="title"
render={({ field }) => (
<FormItem>
<FormLabel>Naziv popisa</FormLabel>
<FormControl>
<Input
{...field}
placeholder="Roštilj 01.05.2026."
autoFocus
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
<ShoppingListTitleField control={form.control} />
</form>
</Form>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
"use client";

import type { Control } from "react-hook-form";

import { Input } from "@/components/ui/input";
import {
FormControl,
FormField,
FormItem,
FormLabel,
FormMessage,
} from "@/components/ui/form";
import type { ShoppingListRequest } from "@/lib/api/types";

interface IShoppingListTitleFieldProps {
control: Control<ShoppingListRequest>;
/** Off where the field is not the point of the modal, or where it mounts late. */
autoFocus?: boolean;
}

/**
* The list name, shared by the create, edit and copy modals so the label, the
* placeholder and the validation message read the same in all three.
*/
export default function ShoppingListTitleField({
control,
autoFocus = true,
}: IShoppingListTitleFieldProps) {
return (
<FormField
control={control}
name="title"
render={({ field }) => (
<FormItem>
<FormLabel>Naziv popisa</FormLabel>
<FormControl>
<Input
{...field}
placeholder="Roštilj 01.05.2026."
autoFocus={autoFocus}
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
);
}
Loading