UX-17: Hide applied proposals by default, add clear/dismiss action

[Chris0Jeky]

Summary

Closes #611

• Backend: Add Dismissed status to ProposalStatus enum, Dismiss() domain method on AutomationProposal, DismissProposalsAsync service method, and POST /api/automation/proposals/dismiss endpoint accepting { ids: [...] }. Only terminal-state proposals (Applied, Rejected, Failed, Expired) can be dismissed. Authorization checks ensure callers can only dismiss their own proposals. Batch size capped at 500.
• Frontend: Add "Show completed" toggle (default: off) to ReviewView that hides Applied/Rejected/Failed/Expired/Dismissed proposals. Add "Clear applied" button that batch-dismisses all terminal proposals via the new endpoint. Summary cards update reactively based on filtered list.
• Types: Dismissed added to frontend ProposalStatus type and normalizeProposalStatus index array.

Acceptance criteria

• "Show completed" toggle defaults to off, filtering out Applied and Rejected from visible proposals
• "Clear applied" button calls POST /automation/proposals/dismiss with array of IDs
• Review list shows only actionable items (PendingReview, Approved) on first load
• Proposal count badges update when filter changes

Test plan

• Backend tests pass (1572/1572)
• Frontend typecheck passes
• Manual: verify toggle shows/hides completed proposals
• Manual: verify Clear Applied button dismisses and removes them from list
• Manual: verify dismissed proposals stay hidden even after refresh

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Self-Review

Correctness

• Domain: Dismiss() correctly guards against non-terminal states (PendingReview, Approved). Only Applied, Rejected, Failed, Expired can be dismissed.
• Service: DismissProposalsAsync silently skips proposals that can't be dismissed (e.g. if status changed between the controller check and the service call). This is intentional -- partial success is better than failing the whole batch.
• Controller: Authorization checks ownership via GetProposalsAsync with the caller's userId. The unauthorizedIds check prevents dismissing other users' proposals.
• Frontend: visibleProposals always hides Dismissed, and hides other terminal states when toggle is off. Summary cards react to filtered list.

Edge cases considered

1. Ownership query bounded at 500: If a user has >500 proposals, old ones won't appear in the ownership check, potentially blocking dismiss of their own proposals. Low risk -- 500 is a generous limit for active proposals.
2. Race condition in frontend dismiss: IDs captured before API call may diverge from local state after. Mitigated by Set-based removal and the filter already hiding terminal proposals.
3. Applied summary card shows 0 when toggle off: Expected behavior per spec -- badges update with filter.

Security

• Endpoint requires [Authorize] via controller class attribute.
• Ownership check prevents cross-user dismissal.
• Batch size capped at MaxProposalListLimit (500) to prevent abuse.
• No board-level write access check for dismiss, but dismiss is a view-layer operation (low risk).

Style

• Follows existing C# conventions (PascalCase, 4-space indent).
• Frontend uses composition API, <script setup>, Tailwind design tokens.
• Commits are focused and small.

No issues requiring fixes found.

[gemini-code-assist]

Code Review

This pull request implements a feature to dismiss completed automation proposals, preventing them from cluttering the default review list. Key changes include a new batch dismissal endpoint in the API, service-level logic to update proposal statuses, and frontend enhancements providing a "Show completed" toggle and a "Clear applied" action. Review feedback highlights a critical authorization bug in the controller where a hardcoded limit on fetched proposals could lead to false "Forbidden" errors for users with many records. Additionally, improvements are suggested regarding the use of exceptions for control flow in the application service and simplifying redundant filtering logic in the Vue component.

[gemini-code-assist]

The authorization logic here is flawed for users with a large number of proposals. By fetching a list of proposals filtered by callerUserId with a limit of MaxProposalListLimit (500), the check will fail for any proposal ID that exists beyond that limit in the database. This results in a Forbidden error for valid proposals owned by the user simply because they aren't among the most recent 500.

Additionally, this check only validates ownership via RequestedByUserId. If the system allows dismissing proposals based on board permissions (consistent with the logic in AuthorizeProposalAsync), this check is too restrictive.

Consider updating the service to handle authorization by verifying the callerUserId against the specific proposals being dismissed, or fetch the specific proposals by the provided IDs in the controller to verify permissions for each.

[gemini-code-assist]

Swallowing all DomainException instances here is a bit broad. While the intent is to skip proposals that aren't in a terminal state, this could inadvertently hide other domain-level validation errors. It is cleaner to check the proposal status explicitly before calling Dismiss() to avoid using exceptions for expected control flow.

                if (proposal.Status is ProposalStatus.Applied or ProposalStatus.Rejected or ProposalStatus.Failed or ProposalStatus.Expired)
                {
                    proposal.Dismiss();
                    dismissed++;
                }

[gemini-code-assist]

There is a redundancy in the filtering logic. Since Dismissed is already included in the completedStatuses set (line 48), the explicit check on line 82 is only necessary if the intention is to hide dismissed proposals even when showCompleted is toggled on. If the intention is that "Show completed" should show everything except dismissed items, the code is correct but could be simplified by clarifying the set usage.

[Chris0Jeky]

Adversarial Code Review: PR #634 -- UX-17 Dismiss Proposals

Critical

1. GetByIdsAsync uses AsNoTracking -- dismissed proposals will never be persisted

DismissProposalsAsync in AutomationProposalService calls _unitOfWork.AutomationProposals.GetByIdsAsync(ids, cancellationToken) and then mutates the returned entities with proposal.Dismiss(). However, GetByIdsAsync in AutomationProposalRepository (line 70) uses .AsNoTracking():

return await _dbSet
    .AsNoTracking()
    .Where(proposal => uniqueIds.Contains(proposal.Id))
    .ToListAsync(cancellationToken);

EF Core will not track these entities, so when SaveChangesAsync is called, no UPDATE statements are emitted. The endpoint will report success but the status change is silently lost. The next refresh will show the proposals again, undismissed.

Fix: Either (a) add a tracked overload (e.g., GetByIdsForUpdateAsync) that omits AsNoTracking, or (b) fetch via GetByIdAsync per entity (which already tracks), or (c) call _dbSet.AttachRange(...) / DbContext.Update(...) before saving.

---

High

2. Authorization check has a hard ceiling at 500 -- silent false-Forbidden for prolific users

In the controller's DismissProposals action:

var proposals = await _proposalService.GetProposalsAsync(
    new ProposalFilterDto(null, null, callerUserId, null, MaxProposalListLimit),
    cancellationToken);

var ownedIds = proposals.Value.Select(p => p.Id).ToHashSet();
var unauthorizedIds = request.Ids.Where(id => !ownedIds.Contains(id)).ToList();

MaxProposalListLimit is 500. If a user has >500 proposals, older proposals will not appear in ownedIds. The user will receive a 403 Forbidden when attempting to dismiss their own valid proposal, simply because it fell outside the top-500 window.

Fix: Do not use GetProposalsAsync for authorization. Instead, fetch the specific proposals by ID (via GetByIdsAsync or GetByIdAsync), and verify RequestedByUserId == callerUserId on each entity. This is both correct and more efficient than loading 500 proposals to check a subset.

---

3. New enum value Dismissed is not in ReviewedStatuses -- future queries may silently exclude dismissed proposals

AutomationProposalRepository.ReviewedStatuses (line 11-17) lists Approved, Rejected, Applied, Failed -- but not Dismissed. This means HasReviewedByUserIdAsync treats dismissed proposals correctly (they were previously in a reviewed state, so the original status was already counted). However, any future code that uses ReviewedStatuses as "all terminal states" will silently exclude Dismissed.

Fix: Either add Dismissed to ReviewedStatuses, or rename the array to make its semantic clear (e.g., DecisionOutcomeStatuses) so future developers do not assume it means "all non-pending states."

---

Medium

4. Broad DomainException catch in DismissProposalsAsync hides bugs

foreach (var proposal in proposals)
{
    try
    {
        proposal.Dismiss();
        dismissed++;
    }
    catch (DomainException)
    {
        // Skip proposals that cannot be dismissed (e.g. still Pending/Approved)
    }
}

The inner catch swallows all DomainException -- including unexpected ones from Touch() or future domain logic. Since Dismiss() already has an explicit status guard, the caller should pre-filter to dismissable statuses instead of relying on exception flow for normal control.

Fix:

foreach (var proposal in proposals)
{
    if (proposal.Status is not (ProposalStatus.Applied or ProposalStatus.Rejected
        or ProposalStatus.Failed or ProposalStatus.Expired))
        continue;

    proposal.Dismiss();
    dismissed++;
}

This also avoids using exceptions for expected control flow, which is a performance concern in hot paths.

---

5. Dismissed is in completedStatuses AND has an explicit hide-always check -- redundant logic

In ReviewView.vue:

const completedStatuses = new Set(['Applied', 'Rejected', 'Failed', 'Expired', 'Dismissed'])

// ...
if (normalizeProposalStatus(proposal.status) === 'Dismissed') return false
if (!showCompleted.value && completedStatuses.has(normalizeProposalStatus(proposal.status))) return false

The first check is redundant because the second check already hides Dismissed when showCompleted is off. The only case the first check adds is hiding Dismissed when showCompleted is ON. If that is the intent, it should be documented with a comment. If not, remove the redundant check and take Dismissed out of completedStatuses.

Fix: If the intent is "dismissed are always hidden even when Show Completed is on," then remove Dismissed from completedStatuses (it is dead code there) and keep only the explicit check with a comment:

// Dismissed proposals are always hidden -- they are permanently cleared
if (normalizeProposalStatus(proposal.status) === 'Dismissed') return false

const completedStatuses = new Set(['Applied', 'Rejected', 'Failed', 'Expired'])
if (!showCompleted.value && completedStatuses.has(normalizeProposalStatus(proposal.status))) return false

---

6. No EF migration for the new enum value

ProposalStatus.Dismissed (value 6) is added to the C# enum. If EF Core stores this as an integer column (the default for SQLite), existing rows are fine -- but there is no migration to validate the schema snapshot is up to date. If the column uses string conversion, existing queries filtering by status will need to handle the new value.

Fix: Generate an EF migration (dotnet ef migrations add AddDismissedProposalStatus) even if it is empty, to keep the model snapshot in sync. Verify the storage format (int vs. string) and confirm the migration path is clean.

---

Low

7. Button label says "Clear applied" but action dismisses all terminal states

The button text reads "Clear applied (N)" but it dismisses Applied, Rejected, Failed, AND Expired proposals. This is misleading UX copy.

Fix: Rename to "Clear completed" to match the actual behavior.

---

8. Frontend removes dismissed proposals from local state optimistically

const dismissedSet = new Set(ids)
proposals.value = proposals.value.filter((p) => !dismissedSet.has(p.id))

This removes ALL requested IDs from the local list regardless of whether the backend actually dismissed them (some may have been skipped due to status). The result.dismissed count could be less than ids.length, but the UI removes all of them.

Fix: Either (a) have the backend return the list of actually-dismissed IDs so the frontend can filter accurately, or (b) after dismiss, re-fetch the proposal list from the server.

---

9. No backend tests for the new Dismiss() domain method or DismissProposalsAsync service method

The PR mentions "Backend tests pass (1572/1572)" but there are no new test cases for:

• Dismiss() succeeding from each valid source state (Applied, Rejected, Failed, Expired)
• Dismiss() throwing from invalid states (PendingReview, Approved, already Dismissed)
• DismissProposalsAsync partial-success scenarios
• The controller endpoint authorization and edge cases

Given the project's testing guidelines ("Behavior changes ship with tests"), this is a gap.

---

Verdict

Finding #1 (Critical) is blocking. The dismiss operation silently does nothing due to AsNoTracking. This must be fixed before merge.

Finding #2 (High) is blocking. The authorization logic has a concrete failure mode for users with >500 proposals. The fix is straightforward -- authorize against the fetched proposals, not a separate capped query.

The remaining findings are non-blocking but should be addressed -- especially #4 (exception swallowing) and #9 (missing tests).

[Chris0Jeky]

Adversarial review fixes (3d2376a)

Addressed all blocking issues from the adversarial review:

1. CRITICAL: GetByIdsAsync returned untracked entities — Dismiss() silently did nothing

Root cause: AutomationProposalRepository.GetByIdsAsync used .AsNoTracking(), so Dismiss() mutated in-memory status but EF Core emitted no UPDATE on SaveChangesAsync.
Fix: Removed AsNoTracking() from GetByIdsAsync so returned entities are change-tracked and mutations persist.

2. HIGH: Authorization capped at 500 proposals

Root cause: Controller pre-fetched all user proposals (capped at MaxProposalListLimit = 500) to build an ownership set, causing false 403s for users with >500 proposals.
Fix: Replaced with per-ID ownership check — fetches only the specific proposals being dismissed and verifies RequestedByUserId on each.

3. NON-BLOCKING: Broad DomainException catch

Root cause: DismissProposalsAsync used a try/catch on DomainException as flow control to skip non-dismissible proposals.
Fix: Replaced with explicit status guard (if proposal.Status is Applied or Rejected or Failed or Expired) before calling Dismiss().

All 1929 backend tests pass.