From b3801b4841af46a27a961480fd5f2fc553cd8038 Mon Sep 17 00:00:00 2001 From: Test User Date: Fri, 16 Jan 2026 11:07:30 +0100 Subject: [PATCH] story5.3 - implement checkpoint feedback input for semi-auto mode Features: - Add FeedbackInput component with textarea and link attachment support - Add FeedbackHistory component with expandable entries and pagination - Add feedback persistence to CheckpointService (save, load, clear) - Integrate FeedbackHistory into CheckpointDialog - Add i18n translations for EN and FR Code Review Fixes: - Add URL validation (only http/https allowed) to prevent XSS - Disable incomplete file attachment feature with TODO - Extract formatFileSize to shared utils.ts - Fix i18n pluralization format (_one/_other for i18next v21+) - Add attachments to recover_from_state result - Add edge case tests for empty attachments Tests: 55 backend tests, 1781 frontend tests pass Co-Authored-By: Claude Opus 4.5 --- apps/backend/core/checkpoint/service.py | 233 ++++++++- .../__tests__/FeedbackHistory.test.tsx | 243 ++++++++++ .../__tests__/FeedbackInput.test.tsx | 415 ++++++++++++++++ .../checkpoints/CheckpointDialog.tsx | 20 +- .../checkpoints/FeedbackHistory.tsx | 226 +++++++++ .../components/checkpoints/FeedbackInput.tsx | 351 ++++++++++++++ .../renderer/components/checkpoints/index.ts | 8 + .../renderer/components/checkpoints/types.ts | 63 +++ .../renderer/components/checkpoints/utils.ts | 37 ++ .../shared/i18n/locales/en/checkpoints.json | 21 + .../shared/i18n/locales/fr/checkpoints.json | 21 + tests/test_checkpoint_service.py | 453 ++++++++++++++++++ 12 files changed, 2087 insertions(+), 4 deletions(-) create mode 100644 apps/frontend/src/renderer/components/__tests__/FeedbackHistory.test.tsx create mode 100644 apps/frontend/src/renderer/components/__tests__/FeedbackInput.test.tsx create mode 100644 apps/frontend/src/renderer/components/checkpoints/FeedbackHistory.tsx create mode 100644 apps/frontend/src/renderer/components/checkpoints/FeedbackInput.tsx create mode 100644 apps/frontend/src/renderer/components/checkpoints/utils.ts diff --git a/apps/backend/core/checkpoint/service.py b/apps/backend/core/checkpoint/service.py index 1839ffe8..d29f7dca 100644 --- a/apps/backend/core/checkpoint/service.py +++ b/apps/backend/core/checkpoint/service.py @@ -11,6 +11,7 @@ Architecture Source: architecture.md#Checkpoint-Service import asyncio import json import logging +import uuid from collections.abc import Callable from dataclasses import dataclass, field from datetime import datetime @@ -84,6 +85,93 @@ FIXED_CHECKPOINTS: list[Checkpoint] = [ # ============================================================================= +@dataclass +class FeedbackAttachment: + """Attachment associated with checkpoint feedback (Story 5.3). + + Attributes: + id: Unique identifier for the attachment + type: Type of attachment ('file' or 'link') + name: Display name for the attachment + path: File path (for files) or URL (for links) + size: File size in bytes (for files) + mime_type: MIME type (for files) + """ + + id: str + type: str # 'file' or 'link' + name: str + path: str + size: int | None = None + mime_type: str | None = None + + def to_dict(self) -> dict[str, Any]: + """Serialize attachment to dictionary.""" + result = { + "id": self.id, + "type": self.type, + "name": self.name, + "path": self.path, + } + if self.size is not None: + result["size"] = self.size + if self.mime_type is not None: + result["mime_type"] = self.mime_type + return result + + @classmethod + def from_dict(cls, data: dict[str, Any]) -> "FeedbackAttachment": + """Deserialize attachment from dictionary.""" + return cls( + id=data["id"], + type=data["type"], + name=data["name"], + path=data["path"], + size=data.get("size"), + mime_type=data.get("mime_type"), + ) + + +@dataclass +class CheckpointFeedback: + """Feedback entry for a checkpoint (Story 5.3). + + Attributes: + id: Unique identifier for the feedback entry + checkpoint_id: ID of the checkpoint this feedback belongs to + feedback: The feedback text + attachments: List of attached files or links + created_at: When the feedback was submitted + """ + + id: str + checkpoint_id: str + feedback: str + attachments: list[FeedbackAttachment] = field(default_factory=list) + created_at: datetime = field(default_factory=datetime.now) + + def to_dict(self) -> dict[str, Any]: + """Serialize feedback to dictionary.""" + return { + "id": self.id, + "checkpoint_id": self.checkpoint_id, + "feedback": self.feedback, + "attachments": [a.to_dict() for a in self.attachments], + "created_at": self.created_at.isoformat(), + } + + @classmethod + def from_dict(cls, data: dict[str, Any]) -> "CheckpointFeedback": + """Deserialize feedback from dictionary.""" + return cls( + id=data["id"], + checkpoint_id=data["checkpoint_id"], + feedback=data["feedback"], + attachments=[FeedbackAttachment.from_dict(a) for a in data.get("attachments", [])], + created_at=datetime.fromisoformat(data["created_at"]), + ) + + @dataclass class CheckpointResult: """Result from a checkpoint pause/resume cycle. @@ -94,6 +182,7 @@ class CheckpointResult: (approve, reject, revise). Use CheckpointDecision.is_valid() to validate. feedback: Optional user feedback or comments + attachments: Optional list of attached files/links (Story 5.3) resumed_at: Timestamp when execution resumed metadata: Additional context from the checkpoint """ @@ -101,6 +190,7 @@ class CheckpointResult: checkpoint_id: str decision: str # One of CheckpointDecision values: "approve", "reject", "revise" feedback: str | None = None + attachments: list[FeedbackAttachment] = field(default_factory=list) resumed_at: datetime | None = None metadata: dict[str, Any] = field(default_factory=dict) @@ -122,6 +212,7 @@ class CheckpointState: artifacts: List of artifact paths produced so far context: Additional execution context is_paused: Whether execution is currently paused + feedback_history: History of feedback provided at checkpoints (Story 5.3) """ task_id: str @@ -131,6 +222,7 @@ class CheckpointState: artifacts: list[str] = field(default_factory=list) context: dict[str, Any] = field(default_factory=dict) is_paused: bool = True + feedback_history: list[CheckpointFeedback] = field(default_factory=list) def to_dict(self) -> dict[str, Any]: """Serialize state to dictionary for JSON persistence.""" @@ -142,6 +234,7 @@ class CheckpointState: "artifacts": self.artifacts, "context": self.context, "is_paused": self.is_paused, + "feedback_history": [f.to_dict() for f in self.feedback_history], } @classmethod @@ -155,6 +248,7 @@ class CheckpointState: artifacts=data.get("artifacts", []), context=data.get("context", {}), is_paused=data.get("is_paused", True), + feedback_history=[CheckpointFeedback.from_dict(f) for f in data.get("feedback_history", [])], ) @@ -229,10 +323,12 @@ class CheckpointService: self._resume_event = asyncio.Event() self._decision: str | None = None self._feedback: str | None = None + self._attachments: list[FeedbackAttachment] = [] self._current_checkpoint_id: str | None = None # State persistence self._state_file = self.spec_dir / "checkpoint_state.json" + self._feedback_history_file = self.spec_dir / "feedback_history.json" # Event callback (to be set by framework) self._event_callback: CheckpointEventCallback = None @@ -339,11 +435,12 @@ class CheckpointService: await self._resume_event.wait() self._resume_event.clear() - # Build result + # Build result (Story 5.3: include attachments) result = CheckpointResult( checkpoint_id=checkpoint.id, decision=self._decision or "approve", feedback=self._feedback, + attachments=self._attachments, resumed_at=datetime.now(), metadata={"phase_id": phase_id, "artifacts": artifacts or []}, ) @@ -352,9 +449,10 @@ class CheckpointService: state.is_paused = False self._save_state(state) - # Reset decision/feedback for next checkpoint + # Reset decision/feedback/attachments for next checkpoint self._decision = None self._feedback = None + self._attachments = [] self._current_checkpoint_id = None logger.info( @@ -362,7 +460,12 @@ class CheckpointService: ) return result - def resume(self, decision: str, feedback: str | None = None) -> None: + def resume( + self, + decision: str, + feedback: str | None = None, + attachments: list[dict[str, Any]] | None = None, + ) -> None: """Resume execution from a checkpoint. Called by the frontend/framework when the user has reviewed the @@ -371,6 +474,7 @@ class CheckpointService: Args: decision: User's decision (approve, reject, revise) feedback: Optional feedback or comments from user + attachments: Optional list of attachment dicts (Story 5.3) """ if not self._current_checkpoint_id: logger.warning("resume() called but no checkpoint is active") @@ -383,6 +487,21 @@ class CheckpointService: self._decision = decision self._feedback = feedback + + # Convert attachment dicts to FeedbackAttachment objects (Story 5.3) + self._attachments = [] + if attachments: + for a in attachments: + self._attachments.append(FeedbackAttachment.from_dict(a)) + + # Save feedback with attachments to history if provided (Story 5.3) + if feedback: + self._save_feedback_to_history( + self._current_checkpoint_id, + feedback, + self._attachments, + ) + self._resume_event.set() def get_current_checkpoint(self) -> Checkpoint | None: @@ -469,6 +588,108 @@ class CheckpointService: except OSError as e: logger.warning(f"Failed to remove checkpoint state: {e}") + # ========================================================================= + # Feedback History (Story 5.3) + # ========================================================================= + + def _save_feedback_to_history( + self, + checkpoint_id: str, + feedback: str, + attachments: list[FeedbackAttachment] | None = None, + ) -> CheckpointFeedback: + """Save feedback with attachments to the history file. + + Args: + checkpoint_id: ID of the checkpoint this feedback is for + feedback: The feedback text + attachments: Optional list of attachments + + Returns: + The created CheckpointFeedback object + """ + # Create feedback entry + feedback_entry = CheckpointFeedback( + id=str(uuid.uuid4()), + checkpoint_id=checkpoint_id, + feedback=feedback, + attachments=attachments or [], + created_at=datetime.now(), + ) + + # Load existing history + history = self.load_feedback_history() + + # Append new entry + history.append(feedback_entry) + + # Save updated history + self._save_feedback_history(history) + + logger.debug(f"Saved feedback for checkpoint {checkpoint_id}") + return feedback_entry + + def _save_feedback_history(self, history: list[CheckpointFeedback]) -> None: + """Save feedback history to JSON file. + + Args: + history: List of feedback entries to save + """ + try: + self.spec_dir.mkdir(parents=True, exist_ok=True) + with open(self._feedback_history_file, "w") as f: + json.dump([fb.to_dict() for fb in history], f, indent=2) + logger.debug(f"Feedback history saved to {self._feedback_history_file}") + except OSError as e: + logger.error(f"Failed to save feedback history: {e}") + raise + + def load_feedback_history(self) -> list[CheckpointFeedback]: + """Load feedback history from JSON file. + + Returns: + List of feedback entries, empty if file doesn't exist + """ + if not self._feedback_history_file.exists(): + logger.debug(f"No feedback history file at {self._feedback_history_file}") + return [] + + try: + with open(self._feedback_history_file) as f: + data = json.load(f) + history = [CheckpointFeedback.from_dict(fb) for fb in data] + logger.debug(f"Loaded {len(history)} feedback entries from history") + return history + except (OSError, json.JSONDecodeError, KeyError) as e: + logger.error(f"Failed to load feedback history: {e}") + return [] + + def get_feedback_for_checkpoint( + self, checkpoint_id: str + ) -> list[CheckpointFeedback]: + """Get all feedback entries for a specific checkpoint. + + Args: + checkpoint_id: ID of the checkpoint to get feedback for + + Returns: + List of feedback entries for the checkpoint + """ + history = self.load_feedback_history() + return [fb for fb in history if fb.checkpoint_id == checkpoint_id] + + def clear_feedback_history(self) -> None: + """Remove feedback history file. + + Called after successful task completion to clean up. + """ + if self._feedback_history_file.exists(): + try: + self._feedback_history_file.unlink() + logger.debug(f"Removed feedback history file {self._feedback_history_file}") + except OSError as e: + logger.warning(f"Failed to remove feedback history: {e}") + # ========================================================================= # Checkpoint Detection # ========================================================================= @@ -527,6 +748,9 @@ class CheckpointService: checkpoint: The checkpoint that was reached state: Current checkpoint state """ + # Get feedback history for this task (Story 5.3) + feedback_history = self.load_feedback_history() + event_data = { "event": "checkpoint_reached", "task_id": self.task_id, @@ -537,6 +761,7 @@ class CheckpointService: "paused_at": state.paused_at.isoformat(), "artifacts": state.artifacts, "requires_approval": checkpoint.requires_approval, + "feedback_history": [fb.to_dict() for fb in feedback_history], # Story 5.3 } logger.debug(f"Emitting checkpoint_reached event: {event_data}") @@ -592,6 +817,7 @@ class CheckpointService: checkpoint_id=checkpoint.id, decision=self._decision or "approve", feedback=self._feedback, + attachments=self._attachments, resumed_at=datetime.now(), metadata={ "phase_id": state.phase_id, @@ -606,6 +832,7 @@ class CheckpointService: self._decision = None self._feedback = None + self._attachments = [] self._current_checkpoint_id = None logger.info(f"Recovered checkpoint {checkpoint.id} with decision: {result.decision}") diff --git a/apps/frontend/src/renderer/components/__tests__/FeedbackHistory.test.tsx b/apps/frontend/src/renderer/components/__tests__/FeedbackHistory.test.tsx new file mode 100644 index 00000000..865a1cf0 --- /dev/null +++ b/apps/frontend/src/renderer/components/__tests__/FeedbackHistory.test.tsx @@ -0,0 +1,243 @@ +/** + * @vitest-environment jsdom + */ +/** + * Tests for FeedbackHistory component + * + * Story Reference: Story 5.3 - Implement Checkpoint Feedback Input + * Acceptance Criteria 3: View feedback history for the task + */ +import { render, screen, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { FeedbackHistory } from '../checkpoints/FeedbackHistory'; +import type { CheckpointFeedback, FeedbackAttachment } from '../checkpoints/types'; + +// Mock i18next +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ + t: (key: string, params?: Record) => { + const translations: Record = { + 'checkpoints:feedback.historyTitle': 'Previous Feedback', + 'checkpoints:feedback.showAll': `Show all (${params?.count || 0})`, + 'checkpoints:feedback.showLess': 'Show less', + 'checkpoints:feedback.attachments': `${params?.count || 0} attachment`, + 'checkpoints:feedback.attachmentCount': `${params?.count || 0} file`, + }; + return translations[key] || key; + }, + }), +})); + +// Helper to create mock feedback entries +const createMockFeedback = (overrides?: Partial): CheckpointFeedback => ({ + id: 'feedback-1', + checkpointId: 'after_planning', + feedback: 'Please add more error handling', + attachments: [], + createdAt: '2026-01-16T10:00:00Z', + ...overrides, +}); + +const createMockAttachment = (overrides?: Partial): FeedbackAttachment => ({ + id: 'attachment-1', + type: 'file', + name: 'example.md', + path: '/path/to/example.md', + size: 1024, + ...overrides, +}); + +describe('FeedbackHistory', () => { + const defaultProps = { + feedbackHistory: [createMockFeedback()], + onViewAttachment: vi.fn(), + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('rendering', () => { + it('renders history title', () => { + render(); + + expect(screen.getByText('Previous Feedback')).toBeInTheDocument(); + }); + + it('renders feedback count', () => { + render(); + + expect(screen.getByText('(1)')).toBeInTheDocument(); + }); + + it('does not render when history is empty', () => { + render(); + + expect(screen.queryByText('Previous Feedback')).not.toBeInTheDocument(); + }); + + it('does not render when history is undefined', () => { + render(); + + expect(screen.queryByText('Previous Feedback')).not.toBeInTheDocument(); + }); + + it('expands first entry by default', () => { + render(); + + expect(screen.getByText('Please add more error handling')).toBeInTheDocument(); + }); + }); + + describe('feedback entries', () => { + it('renders feedback text when entry is expanded', () => { + render(); + + expect(screen.getByText('Please add more error handling')).toBeInTheDocument(); + }); + + it('can collapse and expand entries', () => { + render(); + + // Feedback should be visible (first entry expanded by default) + expect(screen.getByText('Please add more error handling')).toBeInTheDocument(); + + // Find and click the header button to collapse + const headerButtons = screen.getAllByRole('button'); + fireEvent.click(headerButtons[0]); // First button is the entry header + + // Feedback text should be hidden + expect(screen.queryByText('Please add more error handling')).not.toBeInTheDocument(); + + // Click again to expand + fireEvent.click(headerButtons[0]); + + // Feedback should be visible again + expect(screen.getByText('Please add more error handling')).toBeInTheDocument(); + }); + + it('shows attachment count badge when entry has attachments', () => { + const feedbackWithAttachments = createMockFeedback({ + attachments: [createMockAttachment()], + }); + render(); + + expect(screen.getByText('1 file')).toBeInTheDocument(); + }); + }); + + describe('attachments display', () => { + it('renders file attachments', () => { + const feedbackWithFile = createMockFeedback({ + attachments: [ + createMockAttachment({ + type: 'file', + name: 'document.pdf', + path: '/path/document.pdf', + size: 2048, + }), + ], + }); + render(); + + expect(screen.getByText('document.pdf')).toBeInTheDocument(); + expect(screen.getByText('2 KB')).toBeInTheDocument(); + }); + + it('renders link attachments', () => { + const feedbackWithLink = createMockFeedback({ + attachments: [ + createMockAttachment({ + type: 'link', + name: 'Documentation', + path: 'https://docs.example.com', + }), + ], + }); + render(); + + expect(screen.getByText('Documentation')).toBeInTheDocument(); + }); + + it('calls onViewAttachment when attachment is clicked', () => { + const attachment = createMockAttachment({ id: 'test-attachment' }); + const feedbackWithAttachment = createMockFeedback({ + attachments: [attachment], + }); + render(); + + fireEvent.click(screen.getByText('example.md')); + + expect(defaultProps.onViewAttachment).toHaveBeenCalledWith(attachment); + }); + }); + + describe('pagination', () => { + it('shows only first 3 entries by default', () => { + const feedbackHistory = [ + createMockFeedback({ id: '1', feedback: 'Feedback 1', createdAt: '2026-01-16T10:00:00Z' }), + createMockFeedback({ id: '2', feedback: 'Feedback 2', createdAt: '2026-01-16T11:00:00Z' }), + createMockFeedback({ id: '3', feedback: 'Feedback 3', createdAt: '2026-01-16T12:00:00Z' }), + createMockFeedback({ id: '4', feedback: 'Feedback 4', createdAt: '2026-01-16T13:00:00Z' }), + createMockFeedback({ id: '5', feedback: 'Feedback 5', createdAt: '2026-01-16T14:00:00Z' }), + ]; + render(); + + // Should show "Show all" button when there are more than 3 + expect(screen.getByRole('button', { name: /show all/i })).toBeInTheDocument(); + }); + + it('shows all entries when "Show all" is clicked', () => { + const feedbackHistory = [ + createMockFeedback({ id: '1', feedback: 'Feedback 1', createdAt: '2026-01-16T10:00:00Z' }), + createMockFeedback({ id: '2', feedback: 'Feedback 2', createdAt: '2026-01-16T11:00:00Z' }), + createMockFeedback({ id: '3', feedback: 'Feedback 3', createdAt: '2026-01-16T12:00:00Z' }), + createMockFeedback({ id: '4', feedback: 'Feedback 4', createdAt: '2026-01-16T13:00:00Z' }), + ]; + render(); + + // Click "Show all" + fireEvent.click(screen.getByRole('button', { name: /show all/i })); + + // Should now show "Show less" + expect(screen.getByRole('button', { name: /show less/i })).toBeInTheDocument(); + }); + + it('hides pagination when 3 or fewer entries', () => { + const feedbackHistory = [ + createMockFeedback({ id: '1', feedback: 'Feedback 1' }), + createMockFeedback({ id: '2', feedback: 'Feedback 2' }), + ]; + render(); + + expect(screen.queryByRole('button', { name: /show all/i })).not.toBeInTheDocument(); + }); + }); + + describe('sorting', () => { + it('shows most recent feedback first', () => { + const feedbackHistory = [ + createMockFeedback({ id: '1', feedback: 'Old feedback', createdAt: '2026-01-15T10:00:00Z' }), + createMockFeedback({ id: '2', feedback: 'New feedback', createdAt: '2026-01-16T10:00:00Z' }), + ]; + render(); + + // The most recent feedback should be expanded (first visible) + expect(screen.getByText('New feedback')).toBeInTheDocument(); + }); + }); + + describe('date formatting', () => { + it('formats dates in readable format', () => { + const feedback = createMockFeedback({ + createdAt: '2026-01-16T14:30:00Z', + }); + render(); + + // The formatted date should be present (exact format depends on locale) + // Just verify the component renders without error + expect(screen.getByText('Previous Feedback')).toBeInTheDocument(); + }); + }); +}); diff --git a/apps/frontend/src/renderer/components/__tests__/FeedbackInput.test.tsx b/apps/frontend/src/renderer/components/__tests__/FeedbackInput.test.tsx new file mode 100644 index 00000000..8af2e5f3 --- /dev/null +++ b/apps/frontend/src/renderer/components/__tests__/FeedbackInput.test.tsx @@ -0,0 +1,415 @@ +/** + * @vitest-environment jsdom + */ +/** + * Tests for FeedbackInput component + * + * Story Reference: Story 5.3 - Implement Checkpoint Feedback Input + */ +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { FeedbackInput } from '../checkpoints/FeedbackInput'; + +// Mock uuid +vi.mock('uuid', () => ({ + v4: () => 'mock-uuid-1234', +})); + +// Mock i18next +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ + t: (key: string, params?: Record) => { + const translations: Record = { + 'checkpoints:feedback.placeholder': 'Enter your feedback...', + 'checkpoints:feedback.submit': 'Submit Feedback', + 'checkpoints:feedback.submitting': 'Submitting...', + 'checkpoints:feedback.attachFile': 'Attach File', + 'checkpoints:feedback.addLinkButton': 'Add Link', + 'checkpoints:feedback.removeAttachment': 'Remove attachment', + 'checkpoints:feedback.linkNamePlaceholder': 'Link name (optional)', + 'checkpoints:feedback.linkUrlPlaceholder': 'https://...', + 'checkpoints:feedback.cancelLink': 'Cancel', + 'checkpoints:feedback.addLink': 'Add', + 'checkpoints:feedback.attachments': `${params?.count || 0} attachment`, + 'checkpoints:feedback.invalidUrl': 'Please enter a valid URL', + 'checkpoints:feedback.fileAttachmentComingSoon': 'File attachment coming soon', + 'common:buttons.cancel': 'Cancel', + }; + return translations[key] || key; + }, + }), +})); + +describe('FeedbackInput', () => { + const defaultProps = { + onSubmit: vi.fn(), + disabled: false, + isProcessing: false, + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('rendering', () => { + it('renders textarea with placeholder', () => { + render(); + + expect(screen.getByPlaceholderText('Enter your feedback...')).toBeInTheDocument(); + }); + + it('renders custom placeholder when provided', () => { + render(); + + expect(screen.getByPlaceholderText('Custom placeholder')).toBeInTheDocument(); + }); + + it('renders submit button', () => { + render(); + + expect(screen.getByRole('button', { name: /submit feedback/i })).toBeInTheDocument(); + }); + + it('renders attach file button', () => { + render(); + + expect(screen.getByRole('button', { name: /attach file/i })).toBeInTheDocument(); + }); + + it('renders add link button', () => { + render(); + + expect(screen.getByRole('button', { name: /add link/i })).toBeInTheDocument(); + }); + }); + + describe('feedback submission', () => { + it('calls onSubmit with feedback when submitted', () => { + render(); + + const textarea = screen.getByPlaceholderText('Enter your feedback...'); + fireEvent.change(textarea, { target: { value: 'My feedback' } }); + fireEvent.click(screen.getByRole('button', { name: /submit feedback/i })); + + expect(defaultProps.onSubmit).toHaveBeenCalledWith('My feedback', undefined); + }); + + it('trims whitespace from feedback', () => { + render(); + + const textarea = screen.getByPlaceholderText('Enter your feedback...'); + fireEvent.change(textarea, { target: { value: ' Trimmed feedback ' } }); + fireEvent.click(screen.getByRole('button', { name: /submit feedback/i })); + + expect(defaultProps.onSubmit).toHaveBeenCalledWith('Trimmed feedback', undefined); + }); + + it('disables submit button when feedback is empty', () => { + render(); + + expect(screen.getByRole('button', { name: /submit feedback/i })).toBeDisabled(); + }); + + it('disables submit button when feedback is only whitespace', () => { + render(); + + const textarea = screen.getByPlaceholderText('Enter your feedback...'); + fireEvent.change(textarea, { target: { value: ' ' } }); + + expect(screen.getByRole('button', { name: /submit feedback/i })).toBeDisabled(); + }); + + it('clears textarea after submission', () => { + render(); + + const textarea = screen.getByPlaceholderText('Enter your feedback...'); + fireEvent.change(textarea, { target: { value: 'My feedback' } }); + fireEvent.click(screen.getByRole('button', { name: /submit feedback/i })); + + expect(textarea).toHaveValue(''); + }); + }); + + describe('disabled state', () => { + it('disables textarea when disabled', () => { + render(); + + expect(screen.getByPlaceholderText('Enter your feedback...')).toBeDisabled(); + }); + + it('disables all buttons when disabled', () => { + render(); + + expect(screen.getByRole('button', { name: /attach file/i })).toBeDisabled(); + expect(screen.getByRole('button', { name: /add link/i })).toBeDisabled(); + }); + + it('disables all inputs when processing', () => { + render(); + + expect(screen.getByPlaceholderText('Enter your feedback...')).toBeDisabled(); + expect(screen.getByRole('button', { name: /attach file/i })).toBeDisabled(); + }); + + it('shows submitting state when processing', () => { + render(); + + expect(screen.getByText('Submitting...')).toBeInTheDocument(); + }); + }); + + describe('link attachments', () => { + it('shows link input dialog when add link is clicked', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + + expect(screen.getByPlaceholderText('Link name (optional)')).toBeInTheDocument(); + expect(screen.getByPlaceholderText('https://...')).toBeInTheDocument(); + }); + + it('adds link attachment when submitted', async () => { + render(); + + // Open link dialog + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + + // Fill in link details + fireEvent.change(screen.getByPlaceholderText('Link name (optional)'), { + target: { value: 'Documentation' }, + }); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://docs.example.com' }, + }); + + // Add link + fireEvent.click(screen.getByRole('button', { name: /^add$/i })); + + // Check attachment is shown + await waitFor(() => { + expect(screen.getByText('Documentation')).toBeInTheDocument(); + expect(screen.getByText('https://docs.example.com')).toBeInTheDocument(); + }); + }); + + it('uses URL as name when name is not provided', async () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://example.com' }, + }); + fireEvent.click(screen.getByRole('button', { name: /^add$/i })); + + await waitFor(() => { + // URL appears twice: once as name, once as path + const elements = screen.getAllByText('https://example.com'); + expect(elements.length).toBeGreaterThanOrEqual(1); + }); + }); + + it('can cancel link dialog', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + expect(screen.getByPlaceholderText('https://...')).toBeInTheDocument(); + + fireEvent.click(screen.getByRole('button', { name: /^cancel$/i })); + + expect(screen.queryByPlaceholderText('https://...')).not.toBeInTheDocument(); + }); + + it('disables add button when URL is empty', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + + expect(screen.getByRole('button', { name: /^add$/i })).toBeDisabled(); + }); + }); + + describe('attachment management', () => { + it('can remove an attachment', async () => { + render(); + + // Add a link + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://example.com' }, + }); + fireEvent.click(screen.getByRole('button', { name: /^add$/i })); + + // Verify it's added (URL appears twice: as name and as path) + await waitFor(() => { + const elements = screen.getAllByText('https://example.com'); + expect(elements.length).toBeGreaterThanOrEqual(1); + }); + + // Remove it + fireEvent.click(screen.getByRole('button', { name: /remove attachment/i })); + + // Verify it's removed + expect(screen.queryAllByText('https://example.com')).toHaveLength(0); + }); + + it('includes attachments in submission', async () => { + render(); + + // Add feedback + fireEvent.change(screen.getByPlaceholderText('Enter your feedback...'), { + target: { value: 'Check this link' }, + }); + + // Add a link + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('Link name (optional)'), { + target: { value: 'Reference' }, + }); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://example.com' }, + }); + fireEvent.click(screen.getByRole('button', { name: /^add$/i })); + + // Wait for attachment to be added + await waitFor(() => { + expect(screen.getByText('Reference')).toBeInTheDocument(); + }); + + // Submit + fireEvent.click(screen.getByRole('button', { name: /submit feedback/i })); + + expect(defaultProps.onSubmit).toHaveBeenCalledWith( + 'Check this link', + expect.arrayContaining([ + expect.objectContaining({ + type: 'link', + name: 'Reference', + path: 'https://example.com', + }), + ]) + ); + }); + + it('clears attachments after submission', async () => { + render(); + + // Add feedback and link + fireEvent.change(screen.getByPlaceholderText('Enter your feedback...'), { + target: { value: 'Check this' }, + }); + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://example.com' }, + }); + fireEvent.click(screen.getByRole('button', { name: /^add$/i })); + + // URL appears twice: as name and as path + await waitFor(() => { + const elements = screen.getAllByText('https://example.com'); + expect(elements.length).toBeGreaterThanOrEqual(1); + }); + + // Submit + fireEvent.click(screen.getByRole('button', { name: /submit feedback/i })); + + // Attachments should be cleared + expect(screen.queryAllByText('https://example.com')).toHaveLength(0); + }); + }); + + describe('accessibility', () => { + it('submit button has minimum 44px touch target', () => { + render(); + + const submitButton = screen.getByRole('button', { name: /submit feedback/i }); + expect(submitButton).toHaveClass('min-h-[44px]'); + }); + }); + + describe('URL validation', () => { + it('rejects javascript: URLs', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'javascript:alert(1)' }, + }); + + // Add button should be disabled for invalid URL + expect(screen.getByRole('button', { name: /^add$/i })).toBeDisabled(); + }); + + it('rejects data: URLs', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'data:text/html,' }, + }); + + expect(screen.getByRole('button', { name: /^add$/i })).toBeDisabled(); + }); + + it('accepts valid https URLs', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://example.com' }, + }); + + expect(screen.getByRole('button', { name: /^add$/i })).not.toBeDisabled(); + }); + + it('accepts valid http URLs', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'http://example.com' }, + }); + + expect(screen.getByRole('button', { name: /^add$/i })).not.toBeDisabled(); + }); + }); + + describe('edge cases', () => { + it('passes undefined when all attachments are removed before submission', async () => { + render(); + + // Add feedback + fireEvent.change(screen.getByPlaceholderText('Enter your feedback...'), { + target: { value: 'Test feedback' }, + }); + + // Add a link + fireEvent.click(screen.getByRole('button', { name: /add link/i })); + fireEvent.change(screen.getByPlaceholderText('https://...'), { + target: { value: 'https://example.com' }, + }); + fireEvent.click(screen.getByRole('button', { name: /^add$/i })); + + // Wait for attachment to be added + await waitFor(() => { + const elements = screen.getAllByText('https://example.com'); + expect(elements.length).toBeGreaterThanOrEqual(1); + }); + + // Remove the attachment + fireEvent.click(screen.getByRole('button', { name: /remove attachment/i })); + + // Submit - should pass undefined, not empty array + fireEvent.click(screen.getByRole('button', { name: /submit feedback/i })); + + expect(defaultProps.onSubmit).toHaveBeenCalledWith('Test feedback', undefined); + }); + + it('file attachment button is disabled (feature not implemented)', () => { + render(); + + // File attachment feature should be disabled + expect(screen.getByRole('button', { name: /attach file/i })).toBeDisabled(); + }); + }); +}); diff --git a/apps/frontend/src/renderer/components/checkpoints/CheckpointDialog.tsx b/apps/frontend/src/renderer/components/checkpoints/CheckpointDialog.tsx index 3834655f..fcf559bd 100644 --- a/apps/frontend/src/renderer/components/checkpoints/CheckpointDialog.tsx +++ b/apps/frontend/src/renderer/components/checkpoints/CheckpointDialog.tsx @@ -38,7 +38,8 @@ import { Button } from '../ui/button'; import { Textarea } from '../ui/textarea'; import { cn } from '../../lib/utils'; -import type { CheckpointDialogProps, CheckpointArtifact, CheckpointDecisionItem } from './types'; +import type { CheckpointDialogProps, CheckpointArtifact, CheckpointDecisionItem, FeedbackAttachment } from './types'; +import { FeedbackHistory } from './FeedbackHistory'; /** * Get the appropriate icon for an artifact type. @@ -278,6 +279,7 @@ export function CheckpointDialog({ onOpenChange, onViewArtifact, isProcessing = false, + feedbackHistory, }: CheckpointDialogProps) { const { t } = useTranslation(['checkpoints', 'common']); const [expanded, setExpanded] = useState(false); @@ -337,6 +339,22 @@ export function CheckpointDialog({ )} + {/* Feedback History Section (Story 5.3) */} + {feedbackHistory && feedbackHistory.length > 0 && ( +
+ { + // For link attachments, open in browser + if (attachment.type === 'link') { + window.open(attachment.path, '_blank', 'noopener,noreferrer'); + } + // For file attachments, could emit an event to view in app + }} + /> +
+ )} + {/* Feedback Input (shown when requesting revision) */} {showFeedback && (
diff --git a/apps/frontend/src/renderer/components/checkpoints/FeedbackHistory.tsx b/apps/frontend/src/renderer/components/checkpoints/FeedbackHistory.tsx new file mode 100644 index 00000000..17fd8a13 --- /dev/null +++ b/apps/frontend/src/renderer/components/checkpoints/FeedbackHistory.tsx @@ -0,0 +1,226 @@ +/** + * FeedbackHistory component for displaying checkpoint feedback history. + * + * Shows all feedback entries provided at checkpoints for the current task, + * including timestamps, feedback text, and any attachments. + * + * Story Reference: Story 5.3 - Implement Checkpoint Feedback Input + * Acceptance Criteria 3: View feedback history for the task + */ + +import { useTranslation } from 'react-i18next'; +import { + ChevronDown, + ChevronUp, + Clock, + ExternalLink, + File, + History, + Link2, + MessageSquare, +} from 'lucide-react'; +import { useState } from 'react'; + +import { Button } from '../ui/button'; +import { cn } from '../../lib/utils'; + +import type { FeedbackHistoryProps, FeedbackAttachment, CheckpointFeedback } from './types'; +import { formatFileSize } from './utils'; + +/** + * Format a date string to a human-readable format. + */ +function formatDate(dateString: string): string { + const date = new Date(dateString); + return new Intl.DateTimeFormat('default', { + dateStyle: 'medium', + timeStyle: 'short', + }).format(date); +} + +/** + * Single attachment display. + */ +function AttachmentDisplay({ + attachment, + onView, +}: { + attachment: FeedbackAttachment; + onView?: (attachment: FeedbackAttachment) => void; +}) { + const { t } = useTranslation(['checkpoints']); + const Icon = attachment.type === 'file' ? File : Link2; + const isLink = attachment.type === 'link'; + + return ( + + ); +} + +/** + * Single feedback entry display. + */ +function FeedbackEntry({ + entry, + onViewAttachment, + defaultExpanded = false, +}: { + entry: CheckpointFeedback; + onViewAttachment?: (attachment: FeedbackAttachment) => void; + defaultExpanded?: boolean; +}) { + const { t } = useTranslation(['checkpoints']); + const [expanded, setExpanded] = useState(defaultExpanded); + const hasAttachments = entry.attachments && entry.attachments.length > 0; + + return ( +
+ {/* Header - always visible */} + + + {/* Content - expanded */} + {expanded && ( +
+ {/* Feedback text */} +

{entry.feedback}

+ + {/* Attachments */} + {hasAttachments && ( +
+

+ {t('checkpoints:feedback.attachments', { + count: entry.attachments.length, + })} +

+
+ {entry.attachments.map((attachment) => ( + + ))} +
+
+ )} +
+ )} +
+ ); +} + +/** + * FeedbackHistory component. + * + * Displays the history of feedback provided at checkpoints for the current task. + * Entries can be expanded to see full feedback text and attachments. + */ +export function FeedbackHistory({ + feedbackHistory, + onViewAttachment, +}: FeedbackHistoryProps) { + const { t } = useTranslation(['checkpoints']); + const [showAll, setShowAll] = useState(false); + + if (!feedbackHistory || feedbackHistory.length === 0) { + return null; + } + + // Sort by most recent first + const sortedHistory = [...feedbackHistory].sort( + (a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() + ); + + // Show only the last 3 by default + const visibleHistory = showAll ? sortedHistory : sortedHistory.slice(0, 3); + const hasMore = sortedHistory.length > 3; + + return ( +
+
+
+ +

+ {t('checkpoints:feedback.historyTitle')} +

+ + ({feedbackHistory.length}) + +
+ {hasMore && ( + + )} +
+ +
+ {visibleHistory.map((entry, index) => ( + + ))} +
+
+ ); +} + +export default FeedbackHistory; diff --git a/apps/frontend/src/renderer/components/checkpoints/FeedbackInput.tsx b/apps/frontend/src/renderer/components/checkpoints/FeedbackInput.tsx new file mode 100644 index 00000000..943ef713 --- /dev/null +++ b/apps/frontend/src/renderer/components/checkpoints/FeedbackInput.tsx @@ -0,0 +1,351 @@ +/** + * FeedbackInput component for checkpoint feedback with attachment support. + * + * Allows users to provide feedback and optionally attach files or links + * that provide additional context for the AI agent. + * + * Story Reference: Story 5.3 - Implement Checkpoint Feedback Input + * Architecture Source: architecture.md#Checkpoint-Feedback + */ + +import { useState, useRef, useCallback } from 'react'; +import { useTranslation } from 'react-i18next'; +import { + File, + Link2, + Loader2, + Paperclip, + Send, + X, +} from 'lucide-react'; +import { v4 as uuidv4 } from 'uuid'; + +import { Button } from '../ui/button'; +import { Textarea } from '../ui/textarea'; +import { cn } from '../../lib/utils'; + +import type { FeedbackInputProps, FeedbackAttachment } from './types'; +import { formatFileSize, isValidUrl } from './utils'; + +/** + * AttachmentItem component for displaying a single attachment. + */ +function AttachmentItem({ + attachment, + onRemove, +}: { + attachment: FeedbackAttachment; + onRemove: (id: string) => void; +}) { + const { t } = useTranslation(['checkpoints']); + const Icon = attachment.type === 'file' ? File : Link2; + + return ( +
+ +
+

{attachment.name}

+ {attachment.type === 'file' && attachment.size && ( +

+ {formatFileSize(attachment.size)} +

+ )} + {attachment.type === 'link' && ( +

+ {attachment.path} +

+ )} +
+ +
+ ); +} + +/** + * LinkInputDialog component for adding a link attachment. + */ +function LinkInputDialog({ + open, + onClose, + onAdd, +}: { + open: boolean; + onClose: () => void; + onAdd: (name: string, url: string) => void; +}) { + const { t } = useTranslation(['checkpoints']); + const [name, setName] = useState(''); + const [url, setUrl] = useState(''); + const [urlError, setUrlError] = useState(false); + + const handleUrlChange = (e: React.ChangeEvent) => { + const newUrl = e.target.value; + setUrl(newUrl); + // Clear error when user starts typing, validate on blur/submit + if (urlError) { + setUrlError(false); + } + }; + + const handleUrlBlur = () => { + // Validate URL on blur if there's content + if (url.trim() && !isValidUrl(url.trim())) { + setUrlError(true); + } + }; + + const handleSubmit = () => { + const trimmedUrl = url.trim(); + if (trimmedUrl && isValidUrl(trimmedUrl)) { + const linkName = name.trim() || trimmedUrl; + onAdd(linkName, trimmedUrl); + setName(''); + setUrl(''); + setUrlError(false); + onClose(); + } else if (trimmedUrl) { + setUrlError(true); + } + }; + + const handleClose = () => { + setName(''); + setUrl(''); + setUrlError(false); + onClose(); + }; + + if (!open) return null; + + const isUrlValid = url.trim() && isValidUrl(url.trim()); + + return ( +
+
+ setName(e.target.value)} + className={cn( + 'w-full px-3 py-2 text-sm rounded-md', + 'bg-card border border-border', + 'focus:outline-none focus:ring-2 focus:ring-ring' + )} + /> + + {urlError && ( +

+ {t('checkpoints:feedback.invalidUrl')} +

+ )} +
+
+ + +
+
+ ); +} + +/** + * FeedbackInput component. + * + * Provides a textarea for entering feedback and buttons to attach + * files or links. Attachments are displayed in a list below the textarea. + */ +export function FeedbackInput({ + onSubmit, + placeholder, + disabled = false, + isProcessing = false, +}: FeedbackInputProps) { + const { t } = useTranslation(['checkpoints', 'common']); + const [feedback, setFeedback] = useState(''); + const [attachments, setAttachments] = useState([]); + const [showLinkInput, setShowLinkInput] = useState(false); + const fileInputRef = useRef(null); + + /** + * Handle file selection from the file input. + * + * TODO: Story 5.3 - File upload implementation is incomplete. + * The file content is not transmitted to the backend. This feature + * is disabled until proper file upload mechanism is implemented. + * See: architecture.md#Checkpoint-Feedback for planned implementation. + */ + const handleFileSelect = useCallback((_event: React.ChangeEvent) => { + // File upload is disabled - feature incomplete + // When enabled, this should: + // 1. Read file content as base64 or use FormData + // 2. Include file content in the attachment object + // 3. Backend needs endpoint to receive file data + }, []); + + // File attachment feature is disabled until backend support is implemented + const isFileAttachmentEnabled = false; + + /** + * Handle adding a link attachment. + */ + const handleAddLink = useCallback((name: string, url: string) => { + const linkAttachment: FeedbackAttachment = { + id: uuidv4(), + type: 'link', + name, + path: url, + }; + setAttachments((prev) => [...prev, linkAttachment]); + }, []); + + /** + * Handle removing an attachment. + */ + const handleRemoveAttachment = useCallback((id: string) => { + setAttachments((prev) => prev.filter((a) => a.id !== id)); + }, []); + + /** + * Handle form submission. + */ + const handleSubmit = useCallback(() => { + if (feedback.trim()) { + onSubmit(feedback.trim(), attachments.length > 0 ? attachments : undefined); + setFeedback(''); + setAttachments([]); + } + }, [feedback, attachments, onSubmit]); + + const isDisabled = disabled || isProcessing; + const canSubmit = feedback.trim().length > 0 && !isDisabled; + + return ( +
+ {/* Textarea */} +