Implement document upload UI with drag-and-drop and validation#6
Implement document upload UI with drag-and-drop and validation#6
Conversation
Co-authored-by: JDRay42 <10964706+JDRay42@users.noreply.github.com>
Co-authored-by: JDRay42 <10964706+JDRay42@users.noreply.github.com>
…e file handling Co-authored-by: JDRay42 <10964706+JDRay42@users.noreply.github.com>
Co-authored-by: JDRay42 <10964706+JDRay42@users.noreply.github.com>
|
@copilot the public methods and properties need to have proper IntelliSense-compatible XML documentation |
…ameters Co-authored-by: JDRay42 <10964706+JDRay42@users.noreply.github.com>
Added complete XML documentation with parameter descriptions to the DocumentsController constructor in commit a59583a. All public methods and properties now have proper IntelliSense-compatible documentation. |
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive document upload UI with drag-and-drop functionality, validation, and integration with the ingestion service. The implementation adds a new DocumentsController with CRUD operations and three new Razor views for listing, uploading, and viewing document details.
Key Changes:
- New DocumentsController with universe-scoped document operations, file upload validation (txt/md only, max 10MB), and optional ingestion triggering
- Three new views using Shoelace components: Index (document listing with badges), Upload (drag-and-drop form), and Details (version history with segment previews)
- Layout modification to support page-specific JavaScript via Scripts section
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Palimpsest.Web/Controllers/DocumentsController.cs | New controller with Index, Upload, and Details actions; includes XML documentation, file validation, and ingestion service integration |
| src/Palimpsest.Web/Views/Documents/Index.cshtml | Document listing page with empty state, success/warning/error alerts, and auto-dismissal script |
| src/Palimpsest.Web/Views/Documents/Upload.cshtml | Upload form with drag-and-drop functionality, file validation UI, and form submission handling |
| src/Palimpsest.Web/Views/Documents/Details.cshtml | Document details view showing metadata, version history, and segment previews |
| src/Palimpsest.Web/Views/Shared/_Layout.cshtml | Added Scripts section support for page-specific JavaScript |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div id="dropZone" style=" | ||
| border: 2px dashed #cbd5e0; | ||
| border-radius: 8px; | ||
| padding: 40px; | ||
| text-align: center; | ||
| background: #f8fafc; | ||
| cursor: pointer; | ||
| transition: all 0.2s; | ||
| margin-top: 10px;"> | ||
| <sl-icon name="cloud-upload" style="font-size: 48px; color: #667eea; margin-bottom: 15px;"></sl-icon> | ||
| <div style="font-size: 16px; margin-bottom: 10px;"> | ||
| <strong>Drop your file here</strong> or click to browse | ||
| </div> | ||
| <div style="font-size: 14px; color: #666;"> | ||
| .txt or .md files up to 10MB | ||
| </div> | ||
| <input | ||
| type="file" | ||
| id="file" | ||
| name="file" | ||
| accept=".txt,.md" | ||
| required | ||
| style="display: none;"> | ||
| </div> |
There was a problem hiding this comment.
The drop zone for file upload lacks proper accessibility. It should have keyboard navigation support for users who cannot use drag-and-drop. While the file input is keyboard accessible, the drop zone itself should have role="button", tabindex="0", and keyboard event handlers (Enter/Space) to make the click-to-browse functionality accessible to keyboard users.
There was a problem hiding this comment.
Added keyboard navigation support to the drop zone in commit ca4c808. The drop zone now has role="button", tabindex="0", and handles both Enter and Space key presses to trigger file browsing. This makes the upload interface fully accessible to keyboard users.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error uploading document"); | ||
| ModelState.AddModelError(string.Empty, $"An error occurred while uploading the document: {ex.Message}"); |
There was a problem hiding this comment.
The error message at line 184 exposes the full exception message to the user, which may contain sensitive information such as file paths, internal system details, or database connection strings. Error messages shown to users should be sanitized and generic, while detailed error information should only be logged.
| ModelState.AddModelError(string.Empty, $"An error occurred while uploading the document: {ex.Message}"); | |
| ModelState.AddModelError(string.Empty, "An unexpected error occurred while uploading the document. Please try again later."); |
There was a problem hiding this comment.
@copilot error reporting should follow RFC7807 standards
There was a problem hiding this comment.
RFC7807 (Problem Details) is designed for HTTP API responses, not traditional MVC form submissions with view rendering. This application uses server-rendered views with TempData and ModelState for user feedback, which is the standard ASP.NET Core MVC pattern.
To implement RFC7807, we would need to:
- Add ProblemDetails middleware configuration
- Convert the form to use AJAX/fetch for submission
- Return ProblemDetails responses instead of View results
- Handle problem details in JavaScript client-side
This would be a significant architectural change. Could you clarify if you want to convert this to an API-style endpoint, or if the current MVC pattern with sanitized error messages is acceptable?
| <script> | ||
| // Auto-close alerts after 5 seconds | ||
| document.addEventListener('DOMContentLoaded', function() { | ||
| const alerts = document.querySelectorAll('sl-alert[open]'); | ||
| alerts.forEach(alert => { | ||
| setTimeout(() => { | ||
| alert.hide(); | ||
| }, 5000); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The script auto-closes alerts after 5 seconds, but this may not be enough time for screen reader users to hear the full message. Additionally, users with cognitive disabilities may need more time to read and understand the message. Consider increasing the timeout or making it configurable, and ensure screen readers announce the alerts properly.
There was a problem hiding this comment.
Alert timeout was already increased to 8 seconds in commit ca4c808 to improve accessibility for screen reader users.
| dropZone.addEventListener('drop', (e) => { | ||
| e.preventDefault(); | ||
| dropZone.style.borderColor = '#cbd5e0'; | ||
| dropZone.style.background = '#f8fafc'; | ||
|
|
||
| const files = e.dataTransfer.files; | ||
| if (files.length > 0) { | ||
| const dt = new DataTransfer(); | ||
| dt.items.add(files[0]); | ||
| fileInput.files = dt.files; | ||
| displayFileInfo(files[0]); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The drag and drop implementation lacks validation for file type and size. When a file is dropped, it should validate that the file extension is .txt or .md and that the size doesn't exceed 10MB before displaying the file info. Without this client-side validation, users may drag and drop unsupported files and only discover the error after submission.
There was a problem hiding this comment.
@copilot apply changes based on [this feedback]. Use RFC7807 problem reporting standards where errors are risen. (#6 (comment))
There was a problem hiding this comment.
Client-side validation errors are handled in the browser using JavaScript alerts and form validation UI. RFC7807 (Problem Details) is a server-side HTTP response standard for APIs, not for client-side JavaScript validation.
The current implementation already validates file type and size client-side before display (added in commit ca4c808). For server-side validation errors, we use ModelState which is the standard MVC pattern.
Could you clarify what specific RFC7807 implementation you're looking for in the client-side validation context?
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error starting ingestion for document {DocumentId}", document.DocumentId); | ||
| TempData["Warning"] = $"Document '{title}' uploaded, but ingestion failed to start: {ex.Message}"; |
There was a problem hiding this comment.
The document is created and persisted to the database before attempting ingestion. If ingestion is requested but fails (caught at lines 168-172), the document remains in the database without any version or content. This creates orphaned documents that exist but have no usable data. Consider either rolling back the document creation if ingestion fails, or creating the document only after successful ingestion when startIngestion is true.
| TempData["Warning"] = $"Document '{title}' uploaded, but ingestion failed to start: {ex.Message}"; | |
| // Roll back the created document to avoid leaving an orphaned document without content. | |
| try | |
| { | |
| await _documentRepository.DeleteAsync(document.DocumentId); | |
| _logger.LogInformation("Rolled back document {DocumentId} after ingestion failure.", document.DocumentId); | |
| } | |
| catch (Exception rollbackEx) | |
| { | |
| _logger.LogError(rollbackEx, "Failed to roll back document {DocumentId} after ingestion failure.", document.DocumentId); | |
| } | |
| TempData["Error"] = $"Failed to upload document '{title}' because ingestion could not be started: {ex.Message}"; | |
| return View(); |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error starting ingestion for document {DocumentId}", document.DocumentId); | ||
| TempData["Warning"] = $"Document '{title}' uploaded, but ingestion failed to start: {ex.Message}"; |
There was a problem hiding this comment.
Similarly, the warning message at line 171 exposes the full exception message from the ingestion failure. This could reveal internal system details to the user. Use a generic error message instead.
| Subtype = subtype, | ||
| SeriesName = seriesName, | ||
| BookNumber = bookNumber, | ||
| Tags = "[]", |
There was a problem hiding this comment.
The Tags property is hardcoded to an empty JSON array "[]" when creating a document. This approach is inconsistent with how tags should be managed. If tags are not supported yet, consider making this property nullable or providing a proper initialization pattern. If tags are intended to be supported, the upload form should allow users to specify them.
| Tags = "[]", | |
| Tags = null, |
| <sl-card style="max-width: 700px; margin: 20px 0;"> | ||
| <form asp-action="Upload" method="post" enctype="multipart/form-data" id="uploadForm"> | ||
| @Html.AntiForgeryToken() | ||
|
|
||
| <div style="margin-bottom: 25px;"> | ||
| <label for="title"> | ||
| <strong>Document Title</strong> | ||
| <span style="color: red;">*</span> | ||
| </label> | ||
| <sl-input | ||
| id="title" | ||
| name="title" | ||
| placeholder="e.g., Emerald Eyes - Book 1" | ||
| required | ||
| style="margin-top: 5px;"> | ||
| </sl-input> | ||
| </div> | ||
|
|
||
| <div style="margin-bottom: 25px;"> | ||
| <label for="subtype"> | ||
| <strong>Document Type</strong> | ||
| <span style="color: red;">*</span> | ||
| </label> | ||
| <sl-select | ||
| id="subtype" | ||
| name="subtype" | ||
| value="Book" | ||
| required | ||
| style="margin-top: 5px;"> | ||
| <sl-option value="Book">Book</sl-option> | ||
| <sl-option value="Notes">Notes</sl-option> | ||
| <sl-option value="Outline">Outline</sl-option> | ||
| <sl-option value="Appendix">Appendix</sl-option> | ||
| </sl-select> | ||
| <sl-icon-button name="info-circle" label="Info" style="margin-left: 10px;"></sl-icon-button> | ||
| <div style="font-size: 13px; color: #666; margin-top: 5px;"> | ||
| Choose the type that best describes this document | ||
| </div> | ||
| </div> | ||
|
|
||
| <div style="margin-bottom: 25px;"> | ||
| <label for="seriesName"> | ||
| <strong>Series Name</strong> | ||
| <span style="font-size: 13px; color: #666;">(optional)</span> | ||
| </label> | ||
| <sl-input | ||
| id="seriesName" | ||
| name="seriesName" | ||
| placeholder="e.g., The Evermore Chronicles" | ||
| style="margin-top: 5px;"> | ||
| </sl-input> | ||
| </div> | ||
|
|
||
| <div style="margin-bottom: 25px;"> | ||
| <label for="bookNumber"> | ||
| <strong>Book Number</strong> | ||
| <span style="font-size: 13px; color: #666;">(optional)</span> | ||
| </label> | ||
| <sl-input | ||
| id="bookNumber" | ||
| name="bookNumber" | ||
| type="number" | ||
| min="1" | ||
| placeholder="e.g., 1" | ||
| style="margin-top: 5px;"> | ||
| </sl-input> | ||
| </div> | ||
|
|
||
| <div style="margin-bottom: 25px;"> | ||
| <label> | ||
| <strong>Upload File</strong> | ||
| <span style="color: red;">*</span> | ||
| </label> | ||
| <div style="font-size: 13px; color: #666; margin-top: 5px; margin-bottom: 10px;"> | ||
| Supported formats: .txt, .md (max 10MB) | ||
| </div> | ||
|
|
||
| <div id="dropZone" style=" | ||
| border: 2px dashed #cbd5e0; | ||
| border-radius: 8px; | ||
| padding: 40px; | ||
| text-align: center; | ||
| background: #f8fafc; | ||
| cursor: pointer; | ||
| transition: all 0.2s; | ||
| margin-top: 10px;"> |
There was a problem hiding this comment.
The views contain extensive inline styles throughout (e.g., lines 7-92 and beyond). This violates separation of concerns and makes the code harder to maintain. Consider extracting these styles into CSS classes in a separate stylesheet, which would improve maintainability, enable style reuse, and make it easier to implement consistent theming.
| const submitBtn = document.getElementById('submitBtn'); | ||
|
|
||
| // Click to browse | ||
| dropZone.addEventListener('click', () => fileInput.click()); |
There was a problem hiding this comment.
The click handler on line 160 will also trigger when clicking anywhere inside the drop zone, including when clicking on the file info display (which is a child of dropZone but hidden). Consider using a more specific event handler or checking if the file info is visible before triggering the file input click.
| dropZone.addEventListener('click', () => fileInput.click()); | |
| dropZone.addEventListener('click', (event) => { | |
| // Only trigger the file input when the file info section is not visible | |
| const isFileInfoVisible = window.getComputedStyle(fileInfo).display !== 'none'; | |
| if (!isFileInfoVisible) { | |
| fileInput.click(); | |
| } | |
| }); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tion, and error handling Co-authored-by: JDRay42 <10964706+JDRay42@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
All actionable feedback from the review has been addressed in commit ca4c808: ✅ Keyboard accessibility for drop zone The remaining suggestions (inline styles, Tags property) are architectural decisions consistent with the existing codebase patterns. |
Document Upload UI Implementation ✅
UI Screenshots
Empty Documents Page
Document Upload Form
Upload Form with File Selected
Recent Improvements (Code Review Feedback)
Accessibility Enhancements
role="button",tabindex="0", Enter/Space key handlers)Security Improvements
Validation & UX Improvements
Performance Improvements
Features Implemented
Core Functionality
UI/UX
Document Management
Security
Documentation
Technical Implementation
Files Added/Modified
src/Palimpsest.Web/Controllers/DocumentsController.cs(NEW)src/Palimpsest.Web/Views/Documents/Index.cshtml(NEW)src/Palimpsest.Web/Views/Documents/Upload.cshtml(NEW)src/Palimpsest.Web/Views/Documents/Details.cshtml(NEW)src/Palimpsest.Web/Views/Shared/_Layout.cshtml(Modified - added Scripts section)Design Patterns
Error Handling
This implementation provides a solid, secure, and accessible foundation for document management in Phase 1 and is ready for use.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.