Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Container images for this PR have been built successfully!
Built from commit 7f4d8ae |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $effect(() => { | ||
| const parsed = parsePruneDurationInternal(untilValue); | ||
| if (parsed.amount !== durationAmountInternal || parsed.unit !== durationUnitInternal) { | ||
| durationAmountInternal = parsed.amount; | ||
| durationUnitInternal = parsed.unit; | ||
| } | ||
| }); | ||
|
|
||
| $effect(() => { | ||
| if (!hasOlderThanInternal() || value !== olderThanMode) { | ||
| return; | ||
| } | ||
|
|
||
| const normalized = serializePruneDurationInternal(durationAmountInternal, durationUnitInternal); | ||
| if (untilValue !== normalized) { | ||
| untilValue = normalized; | ||
| } | ||
| }); |
There was a problem hiding this comment.
$state updates inside $effect blocks
Two effects mutually update each other's state: Effect 1 (line 67) reads untilValue and writes durationAmountInternal/durationUnitInternal; Effect 2 (line 75) reads those locals and writes untilValue back. While the guard conditions (if (parsed.amount !== durationAmountInternal ...)) prevent infinite loops in practice, this bidirectional sync violates the project rule of not updating $state inside $effect. The pattern is fragile — a rounding difference in serializePruneDurationInternal / parsePruneDurationInternal could create an oscillating loop.
A cleaner approach is to keep durationAmountInternal and durationUnitInternal as the primary reactive state (bound to the inputs), derive untilValue from them, and initialize them once from the prop using a non-reactive assignment or by parsing inside $derived.by.
Rule Used: What: Avoid updating $state inside $effect blo... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/prune/prune-mode-card.svelte
Line: 67-84
Comment:
**Bidirectional `$state` updates inside `$effect` blocks**
Two effects mutually update each other's state: Effect 1 (line 67) reads `untilValue` and writes `durationAmountInternal`/`durationUnitInternal`; Effect 2 (line 75) reads those locals and writes `untilValue` back. While the guard conditions (`if (parsed.amount !== durationAmountInternal ...)`) prevent infinite loops in practice, this bidirectional sync violates the project rule of not updating `$state` inside `$effect`. The pattern is fragile — a rounding difference in `serializePruneDurationInternal` / `parsePruneDurationInternal` could create an oscillating loop.
A cleaner approach is to keep `durationAmountInternal` and `durationUnitInternal` as the primary reactive state (bound to the inputs), derive `untilValue` from them, and initialize them once from the prop using a non-reactive assignment or by parsing inside `$derived.by`.
**Rule Used:** What: Avoid updating `$state` inside `$effect` blo... ([source](https://app.greptile.com/review/custom-context?memory=8e0bee41-b073-4a49-a01c-2c2c8782b420))
How can I resolve this? If you propose a fix, please make it concise.
| $effect(() => { | ||
| if (open && !wasOpenInternal) { | ||
| containerMode = defaults?.pruneContainerMode ?? 'stopped'; | ||
| containerUntil = defaults?.pruneContainerUntil ?? ''; | ||
| imageMode = defaults?.pruneImageMode ?? 'dangling'; | ||
| imageUntil = defaults?.pruneImageUntil ?? ''; | ||
| networkMode = defaults?.pruneNetworkMode ?? 'unused'; | ||
| networkUntil = defaults?.pruneNetworkUntil ?? ''; | ||
| volumeMode = defaults?.pruneVolumeMode ?? 'none'; | ||
| buildCacheMode = defaults?.pruneBuildCacheMode ?? 'none'; | ||
| buildCacheUntil = defaults?.pruneBuildCacheUntil ?? ''; | ||
| } | ||
| wasOpenInternal = open; | ||
| }); |
There was a problem hiding this comment.
$state variables updated inside $effect
The effect on line 94 updates eight $state variables (containerMode, containerUntil, imageMode, etc.) when the dialog opens. Per the project rule, $state must not be updated inside $effect; computed/derived values should use $derived instead.
For a one-shot dialog initialization, a common Svelte 5 alternative is to derive a settings snapshot object and pass it down as a regular argument, or reset the state inside the onConfirm/onCancel handlers. If reset-on-open is strictly required, this is an accepted escape hatch, but the concern should at minimum be documented with a comment explaining why $derived cannot be used here.
Rule Used: What: Avoid updating $state inside $effect blo... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/dialogs/prune-confirmation-dialog.svelte
Line: 94-107
Comment:
**`$state` variables updated inside `$effect`**
The effect on line 94 updates eight `$state` variables (`containerMode`, `containerUntil`, `imageMode`, etc.) when the dialog opens. Per the project rule, `$state` must not be updated inside `$effect`; computed/derived values should use `$derived` instead.
For a one-shot dialog initialization, a common Svelte 5 alternative is to derive a settings snapshot object and pass it down as a regular argument, or reset the state inside the `onConfirm`/`onCancel` handlers. If reset-on-open is strictly required, this is an accepted escape hatch, but the concern should at minimum be documented with a comment explaining why `$derived` cannot be used here.
**Rule Used:** What: Avoid updating `$state` inside `$effect` blo... ([source](https://app.greptile.com/review/custom-context?memory=8e0bee41-b073-4a49-a01c-2c2c8782b420))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| disabled={getSelectedCountInternal() === 0 || isPruning} | ||
| loading={isPruning} | ||
| customLabel={m.prune_button({ count: getSelectedCountInternal() })} |
There was a problem hiding this comment.
getSelectedCountInternal() called twice per render
getSelectedCountInternal() is called on both lines 186 and 188, each of which invokes buildPruneRequestInternal() — rebuilding the entire request object and calling Object.keys twice on every render cycle. This should be a $derived to memoize the count:
| disabled={getSelectedCountInternal() === 0 || isPruning} | |
| loading={isPruning} | |
| customLabel={m.prune_button({ count: getSelectedCountInternal() })} | |
| let selectedCountInternal = $derived(Object.keys(buildPruneRequestInternal()).length); |
Then reference selectedCountInternal in both the disabled attribute and the customLabel.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/lib/components/dialogs/prune-confirmation-dialog.svelte
Line: 186-188
Comment:
**`getSelectedCountInternal()` called twice per render**
`getSelectedCountInternal()` is called on both lines 186 and 188, each of which invokes `buildPruneRequestInternal()` — rebuilding the entire request object and calling `Object.keys` twice on every render cycle. This should be a `$derived` to memoize the count:
```suggestion
let selectedCountInternal = $derived(Object.keys(buildPruneRequestInternal()).length);
```
Then reference `selectedCountInternal` in both the `disabled` attribute and the `customLabel`.
How can I resolve this? If you propose a fix, please make it concise.2 participants
Checklist
mainbranchWhat This PR Implements
Fixes: #1880
Changes Made
Testing Done
./scripts/development/dev.sh startjust lint all)just test backendAI Tool Used (if applicable)
AI Tool:
Assistance Level:
What AI helped with:
I reviewed and edited all AI-generated output:
I ran all required tests and manually verified changes:
Additional Context
Disclaimer Greptiles Reviews use AI, make sure to check over its work.
To better help train Greptile on our codebase, if the comment is useful and valid Like the comment, if its not helpful or invalid Dislike
To have Greptile Re-Review the changes, mention
greptileai.Greptile Summary
This PR replaces the legacy boolean prune flags and single
dockerPruneModesetting with per-resource mode enums (none | stopped | olderThanfor containers,none | dangling | all | olderThanfor images, etc.), adds time-filter support via an "olderThan" +untilduration, and introduces an in-process settings migration that lifts existing boolean flags into the new granular model. The change is fully backward-compatible: both the JSON API (via a customUnmarshalJSON) and the scheduled prune job handle the legacy format gracefully.Confidence Score: 5/5
$state-in-$effectpatterns and a double function call that should be$derived), none of which affect runtime correctness. The Go side — migration logic, typed options structs, legacy JSON decoding, and scheduler integration — is well-tested and logically correct.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: full control over prune options" | Re-trigger Greptile