Skip to content

refactor: decompose ReviewView into focused components (FE-16)#923

Merged
Chris0Jeky merged 2 commits intomainfrom
refactor/fe-16-decompose-review-view
Apr 22, 2026
Merged

refactor: decompose ReviewView into focused components (FE-16)#923
Chris0Jeky merged 2 commits intomainfrom
refactor/fe-16-decompose-review-view

Conversation

@Chris0Jeky
Copy link
Copy Markdown
Owner

Summary

  • Decomposed ReviewView.vue from 1,659 lines into 9 focused files, each under 400 lines
  • Extracted 6 Vue components into src/components/review/:
    • ReviewHeader.vue (199 lines) -- hero section, board selector, action buttons
    • ReviewSummaryCards.vue (74 lines) -- statistics cards grid
    • ReviewEmptyState.vue (47 lines) -- empty state with guided actions
    • ReviewProposalCard.vue (342 lines) -- individual proposal card rendering
    • ReviewProposalActions.vue (229 lines) -- approve/reject/apply/dismiss action footer
    • ReviewProposalDetails.vue (346 lines) -- collapsible details (affected cards, planned changes, provenance)
  • Extracted 2 composables into src/composables/:
    • useReviewProposals.ts (346 lines) -- state management, filtering, data loading, navigation
    • useReviewActions.ts (159 lines) -- proposal actions (approve, reject, execute, dismiss, diff toggle)
  • ReviewView.vue is now 148 lines (orchestration + layout only)

Verification

  • All 45 existing ReviewView tests pass without any test changes
  • All 2,552 frontend tests pass (210 test files)
  • TypeScript typecheck passes
  • Vite production build passes
  • ESLint passes (0 new warnings)

Test plan

  • npm run typecheck passes
  • npm run build passes
  • npm run lint passes (no new warnings)
  • npx vitest --run -- all 2,552 tests pass
  • No single file exceeds 400 lines
  • All existing ReviewView behavior preserved (no test changes needed)

Closes #856

Extract the monolithic ReviewView.vue (1,659 lines) into focused
components and composables, each under 400 lines:

Components (in frontend/taskdeck-web/src/components/review/):
- ReviewHeader.vue (199 lines) - hero, board selector, action buttons
- ReviewSummaryCards.vue (74 lines) - statistics cards grid
- ReviewEmptyState.vue (47 lines) - empty state with guided actions
- ReviewProposalCard.vue (342 lines) - individual proposal card
- ReviewProposalActions.vue (229 lines) - approve/reject/apply/dismiss
- ReviewProposalDetails.vue (346 lines) - collapsible details sections

Composables (in frontend/taskdeck-web/src/composables/):
- useReviewProposals.ts (346 lines) - state, filtering, data loading
- useReviewActions.ts (159 lines) - proposal actions and diff toggle

ReviewView.vue is now 148 lines (orchestration + layout only).

All 45 existing ReviewView tests pass without modification.
Typecheck, build, and lint all pass with no new warnings.

Closes #856
@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Adversarial self-review

Verified correct

  1. Element IDs preserved: proposal-${proposal.id} is still set as id on each card article, so all existing test selectors (wrapper.get('#proposal-proposal-1')) work without modification.

  2. CSS class names preserved: All CSS class names used by tests (.td-review-status--expired, .td-review-card__flow-steps, .td-review-card__collapse-toggle, .td-review-card__provenance-meta, .td-review-summary-card, .td-review__toggle-input, .td-review__board-selector, .td-risk-badge, .td-risk--low, .td-risk--medium, .td-review-card__diff, .td-review-card__diff-label) are preserved identically in the child components.

  3. Event bubbling: All child component events properly bubble up through the component hierarchy (ReviewProposalActions -> ReviewProposalCard -> ReviewView).

  4. v-model contract: InputAssistField in ReviewHeader uses :model-value + @update:model-value which is functionally equivalent to the original v-model="boardFilterInput".

  5. Scoped styles: Each component owns its relevant CSS. Since Vue scoped styles add data attributes per component, the styles remain correctly isolated.

  6. Test compatibility: All 45 ReviewView tests pass without any modifications -- the tests mount ReviewView directly and the child components render transparently.

Behavior change (acceptable)

  1. Link dropdown isolation: The original used a single openLinkDropdown ref so only one link dropdown could be open at a time across all proposal cards. The decomposition gives each ReviewProposalDetails its own dropdown state. In practice, the blur handler closes any open dropdown when focus moves, so this is not a functional regression.

  2. Collapsible section state: The original used a single expandedSections Record keyed by proposalId. The decomposition uses component-local state in each ReviewProposalDetails. Since each instance is keyed by proposal.id in the v-for, Vue maintains identity correctly and the behavior is equivalent.

No issues found

  • No broken props or events
  • No lost functionality
  • No accessibility regressions (all role, aria-label, aria-expanded, aria-current attributes preserved)
  • No test coverage gaps (all 45 existing tests pass)

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the ReviewView by decomposing its logic into modular components and composables, such as ReviewHeader, ReviewProposalCard, and useReviewProposals. This restructuring enhances maintainability and separates concerns. The review feedback identifies several issues related to reactivity and null safety in the newly created components, specifically recommending the use of computed properties for prop-derived data and defensive checks for potentially null identifiers to prevent runtime errors.

@@ -0,0 +1,346 @@
<script setup lang="ts">
import { ref } from 'vue'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The computed function needs to be imported from vue to support the reactive property suggested for fullCorrelationId below.

import { computed, ref } from 'vue'

linkDropdownOpen.value = false
}

const fullCorrelationId = props.proposal.correlationId.trim()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This assignment is not reactive. Since it is a const initialized during the component's setup phase, it will not update if the proposal prop changes (which can happen if the component instance is reused in a keyed v-for list). Additionally, calling .trim() directly on correlationId lacks null safety. Using a computed property with optional chaining resolves both issues.

const fullCorrelationId = computed(() => props.proposal.correlationId?.trim() ?? '')

Comment on lines +72 to +75
function shortCorrelationId(correlationId: string): string {
const trimmed = correlationId.trim()
return trimmed.length > 8 ? trimmed.slice(0, 8) + '...' : trimmed
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function will throw a runtime error if correlationId is null or undefined. It is safer to handle potential missing values defensively.

function shortCorrelationId(correlationId: string | null | undefined): string {
  const trimmed = (correlationId || '').trim()
  return trimmed.length > 8 ? trimmed.slice(0, 8) + '...' : trimmed
}

- Make fullCorrelationId a computed() in ReviewProposalDetails for
  reactivity when the proposal prop changes (gemini-code-assist finding)
- Add null safety to shortCorrelationId in ReviewProposalCard for
  defensive API data handling (gemini-code-assist finding)
- Narrow onBoardFilterInput type in ReviewHeader from
  string|number|boolean to string to match InputAssistField emit type
@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Adversarial Review: PR #923 -- Decompose ReviewView

Verification results

All frontend checks pass after the fixes below:

  • typecheck: clean
  • build: clean
  • lint: clean (0 errors, 6 pre-existing warnings in unrelated files)
  • vitest: 2552/2552 tests pass (210 test files)

Issues found and fixed (pushed to branch)

1. Reactivity bug in ReviewProposalDetails.vue (line 45) -- FIXED

const fullCorrelationId = props.proposal.correlationId.trim() was a plain const evaluated once during setup. If the proposal prop object changes (e.g. after approve/reject replaces the proposal in the array), fullCorrelationId would be stale. Changed to computed(() => props.proposal.correlationId?.trim() ?? ''). The gemini-code-assist bot correctly identified this.

2. Null safety in ReviewProposalCard.vue shortCorrelationId -- FIXED

Although correlationId is typed as string (non-nullable) in types/automation.ts, defensive coding is appropriate since the data arrives from an API. Widened parameter type to string | null | undefined with a fallback. The gemini-code-assist bot correctly identified this.

3. Imprecise type in ReviewHeader.vue onBoardFilterInput -- FIXED

The handler accepted string | number | boolean but InputAssistField only emits string via update:modelValue. Narrowed to string to match the actual emit type and removed the unnecessary String() coercion.

Analysis: No issues found

Collapsible section state isolation -- The old code stored expandedSections per-proposal-id in a shared Record<string, Record<string, boolean>>. The decomposition gives each ReviewProposalDetails instance its own local expandedSections ref. Since Vue creates separate component instances per v-for item (keyed by proposal.id), this is correct and actually cleaner.

Link dropdown state isolation -- Same pattern: old single openLinkDropdown ref replaced by per-component linkDropdownOpen. Correct behavior since only one dropdown per card was ever possible.

Event wiring correctness -- All events are properly forwarded through the component hierarchy: ReviewView -> ReviewProposalCard -> ReviewProposalActions/ReviewProposalDetails. The @reject event correctly forwards both proposalId and riskLevel arguments via (id, risk) => $emit('reject', id, risk).

Composable extraction -- useReviewProposals and useReviewActions correctly share the proposals ref by passing it as a parameter. The watchers for route.hash and activeBoardFilter are properly registered inside the composable (called during setup), so they are automatically cleaned up when the component unmounts.

Keyboard shortcuts -- The original ReviewView had no keyboard shortcuts to lose. The decomposition preserves all interactive behavior.

Accessibility -- ARIA attributes (role, aria-label, aria-expanded, aria-current) are properly preserved in the child components.

Test coverage -- The existing 210 test files (including the dedicated ReviewView.spec.ts and ReviewView.coverage.spec.ts) all pass without modification, confirming the refactoring is behavior-preserving at the integration test level.

Verdict

The decomposition is well-structured and the three issues above have been fixed and pushed. The PR is ready for merge.

@Chris0Jeky Chris0Jeky merged commit 97d4856 into main Apr 22, 2026
14 checks passed
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Apr 22, 2026
@Chris0Jeky Chris0Jeky deleted the refactor/fe-16-decompose-review-view branch April 23, 2026 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

FE-16: Decompose ReviewView (1,659 lines → <400 each)

1 participant