Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 22, 2025

Implements Issue #194: Phase 3 - File Attachments UI Integration (Display)

📋 Overview

This PR implements the UI components and integration for displaying and interacting with file attachments in the SecPal frontend. It follows a TDD approach with comprehensive test coverage.

✨ Changes

New Components

AttachmentUpload.tsx (262 lines, 12 tests ✅)

  • Drag-and-drop file upload zone with file picker fallback
  • File validation: max 10MB, allowed types (images, PDFs, documents)
  • Upload progress indicator (0-100%)
  • Keyboard accessible with i18n support
  • Blocks executable files for security

AttachmentPreview.tsx (224 lines, 9 tests ✅)

  • Modal dialog for previewing images and PDFs
  • Image preview with zoom controls (50%-200%)
  • PDF preview with iframe rendering
  • Download button and close handlers (ESC key, close button, backdrop click)
  • File size display in human-readable format
  • Handles unsupported file types gracefully

Integrations

SecretDetail.tsx

  • Replaced simple attachment list (emoji icons) with full AttachmentList component
  • Added master key management for attachment decryption
  • Implemented download handler with browser download trigger
  • Implemented delete handler with confirmation dialog
  • Implemented preview handler with AttachmentPreview modal
  • Loading states and error handling

SecretDetail.test.tsx

  • Added I18nProvider for Lingui Trans components
  • Mocked getSecretMasterKey for attachment display tests
  • All 11 tests passing

🧪 Testing

  • 21 new unit tests (all passing ✅)
  • 547 total tests passing (100% pass rate)
  • TDD approach: tests written first, implementation follows
  • Coverage includes: drag-drop, validation, zoom, preview, download, delete

🔍 Code Quality

  • ✅ TypeScript strict mode clean
  • ✅ ESLint clean (no warnings)
  • ✅ Prettier formatted
  • ✅ REUSE 3.3 compliant (SPDX headers)
  • ✅ Domain policy compliant
  • ✅ Follows existing patterns (Catalyst UI, Heroicons, Lingui)

📦 Scope Decision

File upload functionality (SecretCreate/Edit integration) moved to future Phase 4 PR to maintain "1 Topic = 1 PR" principle.

Phase 3 focuses solely on:

  • ✅ Display existing attachments
  • ✅ Download attachments
  • ✅ Delete attachments
  • ✅ Preview attachments

Phase 4 will handle:

  • ⏭️ Upload new attachments in SecretCreate
  • ⏭️ Upload new attachments in SecretEdit
  • ⏭️ Upload flow error handling

🔗 Related

🚀 Next Steps

  1. Wait for CI checks (quality.yml, codeql.yml, pr-size.yml)
  2. Request GitHub Copilot code review
  3. Address review feedback
  4. Merge when approved and CI is green
  5. Create Phase 4 issue for upload functionality

Note: This PR contains 1,077 line changes (within acceptable range for a feature PR with comprehensive tests).

@kevalyq kevalyq added the large-pr-approved Approved large PR (boilerplate/templates that cannot be split) label Nov 22, 2025
Implements Issue #194: Phase 3 - File Attachments UI Integration (Display)

## Changes

### New Components
- **AttachmentUpload.tsx** (262 lines, 12 tests)
  - Drag-and-drop file upload zone with file picker fallback
  - File validation: max 10MB, allowed types (images, PDFs, documents)
  - Upload progress indicator
  - Keyboard accessible with i18n support

- **AttachmentPreview.tsx** (224 lines, 9 tests)
  - Modal for previewing images and PDFs
  - Image preview with zoom controls (50%-200%)
  - PDF preview with iframe
  - Download button and close handlers (ESC key, close button)
  - File size display

### Integrations
- **SecretDetail.tsx**
  - Replaced simple attachment list with full AttachmentList component
  - Added master key management for decryption
  - Implemented download/delete/preview handlers
  - Added AttachmentPreview modal integration

### Tests
- 21 new unit tests (all passing)
- Updated SecretDetail tests with I18nProvider for Lingui
- Added getSecretMasterKey mock for attachment display
- All 547 frontend tests passing

## Implementation Details
- TDD approach: tests written first, then implementation
- Follows existing patterns: Catalyst UI, Heroicons, Lingui i18n
- REUSE 3.3 compliant (SPDX headers on all files)
- TypeScript strict mode clean
- ESLint clean

## Scope Note
File upload functionality (SecretCreate/Edit integration) moved to
future Phase 4 PR to maintain "1 Topic = 1 PR" principle. Phase 3
focuses solely on display/view/download of existing attachments.

Related: #192 (Phase 1), #193 (Phase 2), Epic #191
@kevalyq kevalyq force-pushed the feat/attachments-ui-phase3 branch from b04013c to a6251ef Compare November 22, 2025 13:04
@codecov
Copy link

codecov bot commented Nov 22, 2025

Codecov Report

❌ Patch coverage is 94.44444% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/components/AttachmentUpload.tsx 92.72% 4 Missing ⚠️
src/components/AttachmentPreview.tsx 91.66% 2 Missing ⚠️
src/pages/Secrets/SecretDetail.tsx 96.92% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

- Add tests for handleDownload with DOM manipulation
- Add tests for handleDelete with confirmation and refresh
- Add tests for handlePreview modal opening
- Add tests for handleClosePreview cleanup
- Add tests for master key loading error handling
- Add tests for all error scenarios with alerts
- Total: 20 tests (was 11 tests)

Coverage improvement:
- SecretDetail.tsx handlers now fully tested
- All error paths covered
- All user interactions tested
@kevalyq kevalyq marked this pull request as ready for review November 22, 2025 13:13
Copilot AI review requested due to automatic review settings November 22, 2025 13:13
Copilot finished reviewing on behalf of kevalyq November 22, 2025 13:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements Phase 3 of the file attachments UI integration, adding comprehensive display and interaction capabilities for encrypted file attachments in SecPal. The implementation follows a test-driven development approach with 21 new unit tests and maintains 100% test pass rate across 547 total tests.

Key Changes:

  • Integrated AttachmentList and AttachmentPreview components into SecretDetail page for full attachment management
  • Added master key retrieval and management for attachment decryption operations
  • Implemented download, delete, and preview handlers with proper error handling and loading states

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
src/pages/Secrets/SecretDetail.tsx Integrated attachment display with master key management, added download/delete/preview handlers, and AttachmentPreview modal
src/pages/Secrets/SecretDetail.test.tsx Added I18nProvider wrapper and comprehensive tests for attachment operations including master key failure scenarios
src/components/AttachmentUpload.tsx New drag-and-drop file upload component with validation (10MB max, allowed file types), progress indicator, and keyboard accessibility
src/components/AttachmentUpload.test.tsx Comprehensive test suite covering file validation, drag-and-drop, progress display, and accessibility
src/components/AttachmentPreview.tsx New modal component for previewing images and PDFs with zoom controls, download button, and keyboard support
src/components/AttachmentPreview.test.tsx Test suite covering preview rendering for multiple file types, zoom controls, and modal interactions

kevalyq added a commit that referenced this pull request Nov 22, 2025
Critical Fixes:
- Add cleanup useEffect to revoke blob URLs on unmount (memory leak)
- Wrap download URL.createObjectURL in try-finally for guaranteed cleanup
- Extract triggerDownload() helper to DRY download logic

UX Improvements:
- Show attachments section with error message when masterKey fails
- Previously: Section hidden completely, no user feedback
- Now: Display warning to refresh or contact support

Security Enhancements:
- Add sandbox="allow-scripts" to PDF iframe (XSS protection)
- Add SVG XSS warning comment in ALLOWED_MIME_TYPES
- Remove non-standard image/jpg MIME type (image/jpeg is correct)

Accessibility:
- Add aria-live="polite" and aria-atomic="true" to AttachmentUpload
- Screen readers now announce upload state changes

Internationalization:
- Use i18n._(msg`...`) for all alert() and confirm() dialogs
- Error messages: download, delete, preview failures
- Consistent with app's i18n architecture using @lingui/macro

Tests:
- Add test for cleanup useEffect on unmount
- Add test for i18n error messages
- Verify revokeObjectURL called on unmount

Addresses Copilot review comments #2553137276-2553137308 on PR #200
Critical Fixes:
- Add cleanup useEffect to revoke blob URLs on unmount (memory leak)
- Wrap download URL.createObjectURL in try-finally for guaranteed cleanup
- Extract triggerDownload() helper to DRY download logic

UX Improvements:
- Show attachments section with error message when masterKey fails
- Previously: Section hidden completely, no user feedback
- Now: Display warning to refresh or contact support

Security Enhancements:
- Add sandbox="allow-scripts" to PDF iframe (XSS protection)
- Add SVG XSS warning comment in ALLOWED_MIME_TYPES
- Remove non-standard image/jpg MIME type (image/jpeg is correct)

Accessibility:
- Add aria-live="polite" and aria-atomic="true" to AttachmentUpload
- Screen readers now announce upload state changes

Internationalization:
- Use i18n._(msg`...`) for all alert() and confirm() dialogs
- Error messages: download, delete, preview failures
- Consistent with app's i18n architecture using @lingui/macro

Tests:
- Add test for cleanup useEffect on unmount
- Add test for i18n error messages
- Verify revokeObjectURL called on unmount

Addresses Copilot review comments #2553137276-2553137308 on PR #200
@kevalyq kevalyq force-pushed the feat/attachments-ui-phase3 branch from e6c7d86 to 703a872 Compare November 22, 2025 13:33
@kevalyq kevalyq merged commit 8519267 into main Nov 22, 2025
16 checks passed
@kevalyq kevalyq deleted the feat/attachments-ui-phase3 branch November 22, 2025 13:35
kevalyq added a commit that referenced this pull request Nov 22, 2025
- Add Phase 1 (Secret List & Detail Views, PR #197)
- Add Phase 2 (Secret Create/Edit Forms, PR #198)
- Add Phase 3 (File Attachments UI, PR #200)
- Update README.md with Secret Management feature section
- Update Epic #191 with completed phases (60% progress)

All phases merged on 22.11.2025 with 87.95% test coverage.

Part of: #191
kevalyq added a commit that referenced this pull request Nov 22, 2025
Phase 1: #192 (issue) → #197 (PR)
Phase 2: #193 (issue) → #198 (PR)
Phase 3: #194 (issue) → #200 (PR)

Addresses Copilot review comments.
kevalyq added a commit that referenced this pull request Nov 22, 2025
* chore: format SecretForm.enhanced.test.tsx

* docs: Add Secret Management Phase 1-3 documentation

- Add Phase 1 (Secret List & Detail Views, PR #197)
- Add Phase 2 (Secret Create/Edit Forms, PR #198)
- Add Phase 3 (File Attachments UI, PR #200)
- Update README.md with Secret Management feature section
- Update Epic #191 with completed phases (60% progress)

All phases merged on 22.11.2025 with 87.95% test coverage.

Part of: #191

* fix: Correct PR numbers in CHANGELOG.md

Phase 1: #192 (issue) → #197 (PR)
Phase 2: #193 (issue) → #198 (PR)
Phase 3: #194 (issue) → #200 (PR)

Addresses Copilot review comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

large-pr-approved Approved large PR (boilerplate/templates that cannot be split)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants