From e695e4d552e413dd5732859c73af7777fd106d71 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:07:13 +0200 Subject: [PATCH 1/8] chore(agents): Make the review skill clean up after itself, on request Changes: - Add Stage 4, which detects the run's artifacts, branches and worktrees and proposes removing them - Require AskUserQuestion before any deletion, with scratch output and triage docs asked separately - Refuse to delete a branch with unpushed commits or an open PR, even when selected - Note that a deleted reviews/ is not a broken state, since the runners recreate it A cycle left scratch folders, triage docs and a fix branch behind with nothing in the skill to clear them, so they accumulated across runs and muddied the resume detection the skill reads on every invocation. Notes: Cleanup is opt-in per run rather than automatic. A triage doc is the only record of the findings the user chose to skip, and a fix branch can hold the only copy of unpushed commits, so nothing here is safe to sweep on the agent's initiative. --- .../skills/multi-tool-code-review/SKILL.md | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/.claude/skills/multi-tool-code-review/SKILL.md b/.claude/skills/multi-tool-code-review/SKILL.md index 63322b8f..a532f107 100644 --- a/.claude/skills/multi-tool-code-review/SKILL.md +++ b/.claude/skills/multi-tool-code-review/SKILL.md @@ -85,6 +85,32 @@ 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. + +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 | 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. +- **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 behind an open PR**, even if the user selects it. Check `git log ..` and `gh pr list --head ` first, and report back instead of deleting. +- **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. @@ -96,4 +122,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. From 47821afad3e3a6fcf0fa15ea6753a0b745bf1825 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 18:31:38 +0200 Subject: [PATCH 2/8] fix(watchlist): Gate the save button on a parsed schema, not isValid Changes: - Derive isFormValid in use-watchlist-item-form by parsing the schema over the current values, and gate the submit button on it formState.isValid only refreshes when RHF runs the resolver, and seeding the prefill through reset does not run it, so the only pass was the mount one over the empty defaults. A first-time watch opened with a valid prefilled threshold but a dead Prati button, until switching watch mode triggered an unrelated validation pass and woke it up. Notes: this mirrors isEdited, which already compares values to baselines rather than trusting RHF's dirty flags. The resolver stays in place and still owns the messages under the field, so nothing is marked red before it is touched. --- .../components/forms/watchlist-item-modal.tsx | 6 ++---- .../app/products/hooks/use-watchlist-item-form.ts | 13 +++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/frontend/src/app/products/components/forms/watchlist-item-modal.tsx b/frontend/src/app/products/components/forms/watchlist-item-modal.tsx index 745a85fc..00daaf8f 100644 --- a/frontend/src/app/products/components/forms/watchlist-item-modal.tsx +++ b/frontend/src/app/products/components/forms/watchlist-item-modal.tsx @@ -70,6 +70,7 @@ export default function WatchlistItemModal({ existingItems, existingItemForType, isCheckingWatchlist, + isFormValid, isEdited, isEditedInAnyMode, hasSavableChange, @@ -121,10 +122,7 @@ export default function WatchlistItemModal({ submitIcon={existingItemForType ? EyePen : Eye} submitLoading={isSaving} submitDisabled={ - isCheckingWatchlist || - !product || - !form.formState.isValid || - !hasSavableChange + isCheckingWatchlist || !product || !isFormValid || !hasSavableChange } cancelLabel="Odustani" resetLabel="Resetiraj" diff --git a/frontend/src/app/products/hooks/use-watchlist-item-form.ts b/frontend/src/app/products/hooks/use-watchlist-item-form.ts index 08936852..71efd9d0 100644 --- a/frontend/src/app/products/hooks/use-watchlist-item-form.ts +++ b/frontend/src/app/products/hooks/use-watchlist-item-form.ts @@ -160,12 +160,25 @@ export function useWatchlistItemForm( const activeValue = watchType === WatchType.absolute ? absoluteValue : percentageValue; + // Parsed here rather than read off formState.isValid, for the same reason isEdited + // does not read dirtyFields: the flag only refreshes when RHF runs the resolver, + // and seeding the prefill through reset does not. The one validation that had run + // was the mount pass over the empty defaults, so a prefilled first-time watch was + // held invalid until an unrelated change (switching mode) triggered a fresh pass. + // The resolver stays in place and still owns the messages under the field. + const isFormValid = watchlistFormSchema.safeParse({ + watchType, + percentageValue, + absoluteValue, + }).success; + return { form, draftKey, existingItems, existingItemForType, isCheckingWatchlist, + isFormValid, // Compared against the baselines rather than read off RHF's dirtyFields: the // seed keeps dirty flags so an in-progress edit survives a refetch, which means // a flag can outlive the edit itself (a saved value equals its new baseline but From 9e2e7478129a6c7b15e4a433a4bb110240c8f8b3 Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 18:31:49 +0200 Subject: [PATCH 3/8] docs(state-persistence): Record that isValid is stale after seeding Changes: - Extend the prefill gotcha with the validity half: formState.isValid only refreshes when the resolver runs, a seeding reset does not run it, and a form gating an untouched prefill should parse the schema itself The dirty half of this trap was already written down; the validity half is what left a prefilled first-time watch unsavable until the mode was switched. --- docs/STATE-PERSISTENCE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/STATE-PERSISTENCE.md b/docs/STATE-PERSISTENCE.md index bbcf3b51..7fe80261 100644 --- a/docs/STATE-PERSISTENCE.md +++ b/docs/STATE-PERSISTENCE.md @@ -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. From 0cdf6fbdcc50cf8fb091fd7cbc5050cc3126181e Mon Sep 17 00:00:00 2001 From: CrazyFreak <44674613+OffCrazyFreak@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:44:32 +0200 Subject: [PATCH 4/8] feat(shopping-lists): Name the copy before creating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes: - Extract the list name field into shopping-list-title-field.tsx, shared by the create, edit and copy modals - Give the copy modal a react-hook-form title input, prefilled with " (Kopija)" and submitted through the modal's form id - Shorten the prefill by code point so the suffix fits the schema's ceiling, read off the schema rather than restated - Trim the title inside shoppingListRequestSchema, so whitespace cannot pass min(3) and reach the backend blank - Key CopyListModal by list id in the entity outlet, so the lingering instance cannot carry a typed name to the next list - Grow the option row icon to size-6 and the checkbox to size-8 with a size-6 tick - Relabel the option rows to "Kopiraj sve proizvode" and "Označeno i spremljene trgovine" Copying always produced " (Kopija)" and left renaming as a second trip through the edit modal. The name is now an input the user can change before the copy exists, and the field is the same one the create modal uses rather than a second copy of it. Notes: - The progress row's description still mentions prices while its new label does not. Deliberate, and the copy does carry the prices. --- .../components/forms/copy-list-modal.tsx | 91 +++++++++++-------- .../components/forms/copy-option-row.tsx | 8 +- .../components/forms/shopping-list-modal.tsx | 29 +----- .../forms/shopping-list-title-field.tsx | 48 ++++++++++ .../hooks/use-copy-list-modal.ts | 48 +++++++++- .../modal-router/entity-modal-outlet.tsx | 5 +- frontend/src/lib/api/schemas/shopping-list.ts | 8 ++ 7 files changed, 167 insertions(+), 70 deletions(-) create mode 100644 frontend/src/app/(user)/shopping-lists/components/forms/shopping-list-title-field.tsx diff --git a/frontend/src/app/(user)/shopping-lists/components/forms/copy-list-modal.tsx b/frontend/src/app/(user)/shopping-lists/components/forms/copy-list-modal.tsx index cc49cbe3..fcbe7c1b 100644 --- a/frontend/src/app/(user)/shopping-lists/components/forms/copy-list-modal.tsx +++ b/frontend/src/app/(user)/shopping-lists/components/forms/copy-list-modal.tsx @@ -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; @@ -20,6 +22,8 @@ export default function CopyListModal({ open, id }: ICopyListModalProps) { shoppingList, isLoading, isError, + form, + isValid, options, setOption, canCopySharing, @@ -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 ? ( @@ -53,45 +57,56 @@ export default function CopyListModal({ open, id }: ICopyListModalProps) { Popis nije pronađen. Možda je obrisan ili nemaš pristup.

) : ( -
- setOption("items", next)} - /> +
+ 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. */} + - setOption("progress", next)} - /> + setOption("items", next)} + /> - setOption("sharing", next)} - /> + setOption("progress", next)} + /> - {!options.items && ( -

- Kopirat će se samo naziv popisa. -

- )} -
+ setOption("sharing", next)} + /> + + {!options.items && ( +

+ Kopirat će se samo naziv popisa. +

+ )} + + )} ); diff --git a/frontend/src/app/(user)/shopping-lists/components/forms/copy-option-row.tsx b/frontend/src/app/(user)/shopping-lists/components/forms/copy-option-row.tsx index 4a6662a7..85a391bb 100644 --- a/frontend/src/app/(user)/shopping-lists/components/forms/copy-option-row.tsx +++ b/frontend/src/app/(user)/shopping-lists/components/forms/copy-option-row.tsx @@ -41,7 +41,9 @@ export default function CopyOptionRow({ : "bg-muted text-muted-foreground", )} > -