refactor(savings-goals): apply technical review

- Centralize the feature-flag guard in a HasMiddleware closure (matches
  McpTokenController) instead of repeating abort_unless in every action.
- Move the index aggregation into SavingsGoal::withStatsForUser so budgets
  stay decoupled from goals.
- Route single-goal reads through savedAmountInCents() so the sign convention
  lives in one place.
- Extract useControllableOpen hook shared by both create dialogs.

app/Http/Controllers/BudgetController.php

@@ -12,8 +12,6 @@ use App\Models\Budget;
 use App\Models\Category;
 use App\Models\Label;
 use App\Models\SavingsGoal;
-use App\Models\Transaction;
-use App\Models\User;
 use App\Services\BudgetPeriodService;
 use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
 use Illuminate\Http\RedirectResponse;
@@ -41,51 +39,16 @@ class BudgetController extends Controller
             }])
             ->get();
 
+        $savingsGoalsEnabled = Feature::active(SavingsGoals::class);
+
         return Inertia::render('budgets/index', [
             'budgets' => $budgets,
-            'savingsGoals' => $this->savingsGoalsForIndex($user),
-            'savingsGoalsEnabled' => Feature::active(SavingsGoals::class),
+            'savingsGoals' => $savingsGoalsEnabled ? SavingsGoal::withStatsForUser($user) : [],
+            'savingsGoalsEnabled' => $savingsGoalsEnabled,
             'currencyCode' => $user->currency_code ?? 'USD',
         ]);
     }
 
-    /**
-     * Savings goals shown alongside budgets on the index, each enriched with its
-     * computed progress. Returns an empty list when the feature is off.
-     *
-     * @return list<array<string, mixed>>
-     */
-    private function savingsGoalsForIndex(User $user): array
-    {
-        if (! Feature::active(SavingsGoals::class)) {
-            return [];
-        }
-
-        $goals = $user->savingsGoals()->with('label')->get();
-
-        // ponytail: one grouped sum for all goals' labels avoids N+1 across the list.
-        $savedByLabel = Transaction::query()
-            ->join('label_transaction', 'label_transaction.transaction_id', '=', 'transactions.id')
-            ->whereIn('label_transaction.label_id', $goals->pluck('label_id')->filter())
-            ->groupBy('label_transaction.label_id')
-            ->selectRaw('label_transaction.label_id as label_id, SUM(transactions.amount) as total')
-            ->pluck('total', 'label_id');
-
-        return $goals->map(function (SavingsGoal $goal) use ($savedByLabel): array {
-            $saved = -1 * (int) ($savedByLabel[$goal->label_id] ?? 0);
-
-            return array_merge($goal->toArray(), [
-                'stats' => SavingsGoal::project(
-                    $saved,
-                    $goal->target_amount,
-                    $goal->created_at,
-                    $goal->target_date,
-                    now(),
-                ),
-            ]);
-        })->all();
-    }
-
     public function show(Request $request, Budget $budget): Response
     {
         $this->authorize('view', $budget);

app/Http/Controllers/SavingsGoalController.php

@@ -12,22 +12,38 @@ use App\Models\Bank;
 use App\Models\Category;
 use App\Models\Label;
 use App\Models\SavingsGoal;
+use Closure;
 use Illuminate\Foundation\Auth\Access\AuthorizesRequests;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
+use Illuminate\Routing\Controllers\HasMiddleware;
 use Illuminate\Support\Facades\DB;
 use Inertia\Inertia;
 use Inertia\Response;
 use Laravel\Pennant\Feature;
 
-class SavingsGoalController extends Controller
+class SavingsGoalController extends Controller implements HasMiddleware
 {
     use AuthorizesRequests;
 
+    /**
+     * Hide the whole savings-goals surface behind the rollout feature flag.
+     *
+     * @return array<int, Closure>
+     */
+    public static function middleware(): array
+    {
+        return [
+            function (Request $request, Closure $next): mixed {
+                abort_unless(Feature::active(SavingsGoals::class), 404);
+
+                return $next($request);
+            },
+        ];
+    }
+
     public function store(StoreSavingsGoalRequest $request): RedirectResponse
     {
-        abort_unless(Feature::active(SavingsGoals::class), 404);
-
         $goal = DB::transaction(function () use ($request) {
             $label = $request->user()->labels()->create([
                 'name' => $request->name,
@@ -48,7 +64,6 @@ class SavingsGoalController extends Controller
 
     public function show(Request $request, SavingsGoal $savingsGoal): Response
     {
-        abort_unless(Feature::active(SavingsGoals::class), 404);
         $this->authorize('view', $savingsGoal);
 
         $user = $request->user();
@@ -61,10 +76,8 @@ class SavingsGoalController extends Controller
                 ->get()
             : collect();
 
-        $saved = -1 * (int) $transactions->sum('amount');
-
         $stats = SavingsGoal::project(
-            $saved,
+            $savingsGoal->savedAmountInCents(),
             $savingsGoal->target_amount,
             $savingsGoal->created_at,
             $savingsGoal->target_date,
@@ -98,7 +111,6 @@ class SavingsGoalController extends Controller
 
     public function update(UpdateSavingsGoalRequest $request, SavingsGoal $savingsGoal): RedirectResponse
     {
-        abort_unless(Feature::active(SavingsGoals::class), 404);
         $this->authorize('update', $savingsGoal);
 
         DB::transaction(function () use ($request, $savingsGoal) {
@@ -114,7 +126,6 @@ class SavingsGoalController extends Controller
 
     public function destroy(Request $request, SavingsGoal $savingsGoal): RedirectResponse
     {
-        abort_unless(Feature::active(SavingsGoals::class), 404);
         $this->authorize('delete', $savingsGoal);
 
         DB::transaction(function () use ($savingsGoal) {

app/Models/SavingsGoal.php

@@ -64,6 +64,41 @@ class SavingsGoal extends Model
         return -1 * (int) $this->label->transactions()->sum('transactions.amount');
     }
 
+    /**
+     * All of a user's goals with their computed progress, for the combined
+     * budgets/goals index. Kept here (not in the budget controller) so budgets
+     * stay decoupled from goals.
+     *
+     * @return list<array<string, mixed>>
+     */
+    public static function withStatsForUser(User $user): array
+    {
+        $goals = $user->savingsGoals()->with('label')->get();
+
+        // ponytail: one grouped sum for all goals' labels avoids N+1 across the list.
+        $savedByLabel = Transaction::query()
+            ->join('label_transaction', 'label_transaction.transaction_id', '=', 'transactions.id')
+            ->whereIn('label_transaction.label_id', $goals->pluck('label_id')->filter())
+            ->groupBy('label_transaction.label_id')
+            ->selectRaw('label_transaction.label_id as label_id, SUM(transactions.amount) as total')
+            ->pluck('total', 'label_id');
+
+        return $goals->map(function (SavingsGoal $goal) use ($savedByLabel): array {
+            // Negated net flow, mirroring savedAmountInCents(); batched to avoid N+1.
+            $saved = -1 * (int) ($savedByLabel[$goal->label_id] ?? 0);
+
+            return array_merge($goal->toArray(), [
+                'stats' => self::project(
+                    $saved,
+                    $goal->target_amount,
+                    $goal->created_at,
+                    $goal->target_date,
+                    now(),
+                ),
+            ]);
+        })->all();
+    }
+
     /**
      * Linear progress + projection, computed from primitives so it stays a pure,
      * testable function. Dates are day-granular. `rate_per_day` is cents/day and

resources/js/components/budgets/create-budget-dialog.tsx

@@ -21,6 +21,7 @@ import {
     SelectTrigger,
     SelectValue,
 } from '@/components/ui/select';
+import { useControllableOpen } from '@/hooks/use-controllable-open';
 import { buildCategoryTree, flattenCategoryTree } from '@/lib/category-tree';
 import { cn } from '@/lib/utils';
 import { SharedData } from '@/types';
@@ -57,16 +58,11 @@ export function CreateBudgetDialog({
     onOpenChange,
 }: Props) {
     const page = usePage<SharedData>();
-    const [internalOpen, setInternalOpen] = useState(false);
-    const isControlled = open !== undefined;
-    const dialogOpen = isControlled ? open : internalOpen;
-    const setOpen = (next: boolean) => {
-        if (isControlled) {
-            onOpenChange?.(next);
-        } else {
-            setInternalOpen(next);
-        }
-    };
+    const {
+        open: dialogOpen,
+        setOpen,
+        isControlled,
+    } = useControllableOpen({ open, onOpenChange });
     const [name, setName] = useState('');
     const [periodType, setPeriodType] = useState<BudgetPeriodType>('monthly');
     const [periodStartDay, setPeriodStartDay] = useState<number>(1);

resources/js/components/savings-goals/create-savings-goal-dialog.tsx

@@ -13,6 +13,7 @@ import {
 } from '@/components/ui/dialog';
 import { Input } from '@/components/ui/input';
 import { Label as UILabel } from '@/components/ui/label';
+import { useControllableOpen } from '@/hooks/use-controllable-open';
 import { cn } from '@/lib/utils';
 import { __ } from '@/utils/i18n';
 import { router } from '@inertiajs/react';
@@ -34,16 +35,11 @@ export function CreateSavingsGoalDialog({
     open,
     onOpenChange,
 }: Props) {
-    const [internalOpen, setInternalOpen] = useState(false);
-    const isControlled = open !== undefined;
-    const dialogOpen = isControlled ? open : internalOpen;
-    const setDialogOpen = (next: boolean) => {
-        if (isControlled) {
-            onOpenChange?.(next);
-        } else {
-            setInternalOpen(next);
-        }
-    };
+    const {
+        open: dialogOpen,
+        setOpen: setDialogOpen,
+        isControlled,
+    } = useControllableOpen({ open, onOpenChange });
 
     const [name, setName] = useState('');
     const [targetAmount, setTargetAmount] = useState<number>(0);

resources/js/hooks/use-controllable-open.ts

@@ -0,0 +1,30 @@
+import { useState } from 'react';
+
+interface Options {
+    open?: boolean;
+    onOpenChange?: (open: boolean) => void;
+}
+
+/**
+ * Lets a dialog work both controlled (parent passes open/onOpenChange) and
+ * uncontrolled (own internal state) without each component reimplementing the
+ * branching.
+ */
+export function useControllableOpen({ open, onOpenChange }: Options) {
+    const [internalOpen, setInternalOpen] = useState(false);
+    const isControlled = open !== undefined;
+
+    const setOpen = (next: boolean) => {
+        if (isControlled) {
+            onOpenChange?.(next);
+        } else {
+            setInternalOpen(next);
+        }
+    };
+
+    return {
+        open: isControlled ? open : internalOpen,
+        setOpen,
+        isControlled,
+    };
+}