Conversation
Introduce a new Manifest Import feature: adds a web component (components/manifest-import/index.js) that imports multiple IIIF manifests into TPEN, handles authentication, creates projects sequentially, and renders progress/results. Includes docs and support pages (IMPLEMENTATION.md, README.md, QUICKSTART.md), an examples page and interface (interfaces/manifest-import/index.html, examples.html, INDEX.md), and interface metadata (manifest.yml). Uses TPEN Services POST /project/import?createFrom=URL and integrates with TPEN auth/token handling.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new "IIIF import direct" feature that enables users to create TPEN projects from multiple IIIF manifests via query string parameters. The feature includes a web component, interface page, comprehensive documentation, and examples.
Changes:
- Added
tpen-manifest-importweb component that handles manifest parsing, authentication, batch project creation, and results display - Created
/import-manifestinterface page with responsive styling - Provided extensive documentation including quickstart guide, examples, and implementation details
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| components/manifest-import/index.js | Main web component implementing the import workflow with authentication, API calls, and UI rendering |
| components/manifest-import/README.md | Technical documentation for the component |
| components/manifest-import/IMPLEMENTATION.md | Detailed implementation summary and architecture notes |
| interfaces/manifest-import/index.html | Interface page hosting the component with permalink configuration |
| interfaces/manifest-import/examples.html | Interactive examples and integration guide for developers |
| interfaces/manifest-import/QUICKSTART.md | Quick start guide for end users |
| interfaces/manifest-import/INDEX.md | Complete reference documentation with links to all resources |
| interfaces/manifest-import/manifest.yml | Interface metadata and configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Visit this example with real manifests: | ||
|
|
||
| ``` | ||
| /import-manifest?manifest=http%3A%2F%2Fwww.e-codices.unifr.ch%2Fmetadata%2Fiiif%2Fsl-0002%2Fmanifest.json |
There was a problem hiding this comment.
The URL in the example shows "/?manifest=" but should be "/import-manifest?manifest=" to match the permalink configured in index.html.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ``` | ||
| /?manifest=http%3A%2F%2Fwww.e-codices.unifr.ch%2Fmetadata%2Fiiif%2Fsl-0002%2Fmanifest.json | ||
| ``` | ||
|
|
||
| ### Multiple Manifests | ||
| ``` | ||
| /?manifest=URL1&manifest=URL2&manifest=URL3 | ||
| ``` | ||
|
|
||
| **Key Points**: | ||
| - Manifests must be URL-encoded | ||
| - Multiple manifests use repeated `manifest` parameters | ||
| - Duplicates are automatically filtered out | ||
| - One manifest per parameter | ||
|
|
||
| ## Component Architecture | ||
|
|
||
| ### Private Properties | ||
| - `#manifests` - Array of manifest URLs to import | ||
| - `#createdProjects` - Array of created project objects | ||
| - `#isCreating` - Loading state flag | ||
| - `#authToken` - Current auth token | ||
|
|
||
| ### Public Methods | ||
| - `load()` - Main entry point, orchestrates the workflow | ||
|
|
||
| ### Private Methods | ||
| - `#extractManifests()` - Parses query parameters | ||
| - `#createProjects()` - Creates projects sequentially | ||
| - `#importManifest(url)` - API call for single manifest | ||
| - `render*()` - Various render methods for different states | ||
|
|
||
| ### Render States | ||
| 1. **renderNeedAuth()** - Login prompt | ||
| 2. **renderNoManifests()** - No manifests found in URL | ||
| 3. **renderCreating()** - Loading indicator | ||
| 4. **renderResults()** - Results page with projects and errors | ||
|
|
||
| ## API Integration | ||
|
|
||
| The component uses this TPEN Services API endpoint: | ||
|
|
||
| ``` | ||
| POST /project/import?createFrom=URL | ||
| Headers: | ||
| Authorization: Bearer {token} | ||
| Content-Type: application/json | ||
| Body: | ||
| { "url": "https://example.com/manifest.json" } | ||
| ``` | ||
|
|
||
| **Response** (success): | ||
| ```json | ||
| { | ||
| "_id": "project-id", | ||
| "label": "Project Name", | ||
| "layers": [...], | ||
| "metadata": [...], | ||
| "creator": "user-id", | ||
| ... | ||
| } | ||
| ``` | ||
|
|
||
| ## Error Handling | ||
|
|
||
| The component gracefully handles: | ||
|
|
||
| | Scenario | Behavior | | ||
| |----------|----------| | ||
| | Not authenticated | Shows login button | | ||
| | No manifests in URL | Shows helpful error with example | | ||
| | Invalid manifest URL | Listed in failed section with error | | ||
| | Network error | Caught and displayed | | ||
| | Invalid JSON response | Error message shown | | ||
| | Partial failures | Successful projects still display | | ||
|
|
||
| ## Styling Features | ||
|
|
||
| - **Responsive Grid**: Project cards use CSS Grid that adapts to screen size | ||
| - **Mobile Optimized**: Single column on mobile, multi-column on desktop | ||
| - **Accessibility**: Proper contrast, readable fonts, clear hierarchy | ||
| - **Theme Support**: CSS custom properties for easy customization | ||
| - **Loading State**: Animated spinner with status text | ||
| - **Error States**: Clear visual distinction with colors and icons | ||
|
|
||
| ## Results Display | ||
|
|
||
| ### Successful Projects Show: | ||
| - Project title | ||
| - Project ID | ||
| - Layer and page counts | ||
| - "View" button (goes to project details) | ||
| - "Transcribe" button (starts transcription interface) | ||
|
|
||
| ### Failed Imports Show: | ||
| - Error message | ||
| - Manifest URL (for reference) | ||
| - Grouped in separate "Failed" section | ||
|
|
||
| ### Navigation: | ||
| - "Back to TPEN" button (links to home) | ||
| - "View Projects" button (if any succeeded) | ||
|
|
||
| ## Key Design Decisions | ||
|
|
||
| 1. **Web Component**: Encapsulated, reusable, no dependencies on external frameworks | ||
| 2. **Sequential Creation**: Projects created one at a time to prevent overload | ||
| 3. **No State Persistence**: Each session is independent | ||
| 4. **Auth Delegation**: Leverages existing TPEN authentication system | ||
| 5. **Graceful Degradation**: Works without JavaScript (prompts to enable) | ||
| 6. **Duplicate Filtering**: Uses `Set` to remove duplicate manifest URLs | ||
|
|
||
| ## Browser Compatibility | ||
|
|
||
| - ✓ Chrome 90+ | ||
| - ✓ Firefox 88+ | ||
| - ✓ Safari 14+ | ||
| - ✓ Edge 90+ | ||
| - ✓ Mobile browsers (iOS Safari, Chrome Android) | ||
|
|
||
| ## Testing Scenarios | ||
|
|
||
| To test the feature, try these URLs: | ||
|
|
||
| ``` | ||
| # Single manifest (e-codices) | ||
| /?manifest=http%3A%2F%2Fwww.e-codices.unifr.ch%2Fmetadata%2Fiiif%2Fsl-0002%2Fmanifest.json | ||
|
|
||
| # Multiple manifests | ||
| /?manifest=URL1&manifest=URL2 | ||
|
|
||
| # With duplicates (one will be filtered) | ||
| /?manifest=URL1&manifest=URL1&manifest=URL2 | ||
|
|
||
| # No manifests (shows error) | ||
| /?manifest= | ||
| ``` | ||
|
|
||
| ## Files Created | ||
|
|
||
| ``` | ||
| components/manifest-import/ | ||
| ├── index.js (576 lines - main component) | ||
| └── README.md (comprehensive documentation) | ||
|
|
||
| interfaces/manifest-import/ |
There was a problem hiding this comment.
The URL examples use "/?manifest=" but should use "/import-manifest?manifest=" to match the permalink configured in index.html. This appears in multiple places (lines 86, 90, 212, 221, 227, 231).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
components/manifest-import/index.js
Outdated
| #escapeHtml(text) { | ||
| const div = document.createElement('div') | ||
| div.textContent = text | ||
| return div.innerHTML | ||
| } |
There was a problem hiding this comment.
The component defines its own #escapeHtml method, but there's a shared escapeHtml utility function at '/js/utils.js' that's used by other components (navigation-manager, project-options, quicktype-tool, etc.). For consistency with the codebase, import and use the shared utility instead of defining a private method. This also ensures the same escaping logic is applied across all components.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| </style> | ||
| </head> | ||
| <body> | ||
| <tpen-manifest-import></tpen-manifest-import> |
There was a problem hiding this comment.
The custom element 'tpen-manifest-import' doesn't have any ARIA attributes or semantic markup. For better accessibility, consider adding role="main" or wrapping it in a main element so screen readers can properly identify the main content area.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| .loader-wrapper { | ||
| text-align: center; | ||
| } | ||
|
|
||
| .spinner { | ||
| border: 4px solid var(--light-gray, #f5f5f5); | ||
| border-top: 4px solid var(--interface-primary, #005a8c); | ||
| border-radius: 50%; | ||
| width: 50px; | ||
| height: 50px; | ||
| animation: spin 1s linear infinite; | ||
| margin: 0 auto 20px; | ||
| } |
There was a problem hiding this comment.
The spinner animation in the loading state has no accessible label or ARIA attributes. Screen reader users won't know that content is loading. Add role="status" and aria-live="polite" to the loader-wrapper div, and consider adding aria-label="Loading" to the spinner div.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * Fix URL format in INDEX.md to match permalink Co-authored-by: cubap <1119165+cubap@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
* Initial plan * Fix URL examples to use /import-manifest path Co-authored-by: cubap <1119165+cubap@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
* Initial plan * Replace custom #escapeHtml with shared utility from /js/utils.js Co-authored-by: cubap <1119165+cubap@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
* Initial plan * Fix /?manifest= to /import-manifest?manifest= across all documentation Co-authored-by: cubap <1119165+cubap@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
* Initial plan * Add main element wrapper for improved accessibility Co-authored-by: cubap <1119165+cubap@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
| </style> | ||
| </head> | ||
| <body> | ||
| <main> |
There was a problem hiding this comment.
@copilot put this inside of a tpen-page component instead of building out a bespoke interface
Static Review Comments@cubap - Here's a code review of this PR. Branch: SummaryThis PR adds a new manifest import feature that allows users to import multiple IIIF manifests as TPEN projects via a single URL. The implementation includes a web component ( Overall, the feature is well-structured and includes good error handling for most scenarios. However, there are security vulnerabilities that need to be addressed before deployment, along with some code quality improvements.
Critical Issues 🔴Issue 1: XSS Vulnerability - Unescaped User Input in Error DisplayFile: Problem: Current Code: ${failed.map(error => `
<div class="error-item">
<strong>${error.message}</strong>
<div class="error-url">${error.manifestUrl}</div>
</div>
`).join('')}Suggested Fix: ${failed.map(error => `
<div class="error-item">
<strong>${escapeHtml(error.message)}</strong>
<div class="error-url">${escapeHtml(error.manifestUrl)}</div>
</div>
`).join('')}How to Verify:
Major Issues 🟠Issue 1: Unused ImportFile: Problem: Current Code: import { getUserFromToken } from '../iiif-tools/index.js'Suggested Fix: // Remove this line entirely - the import is not usedHow to Verify:
Issue 2: Error Handling - JSON Parse May FailFile: Problem: Current Code: if (!response.ok) {
const errorData = await response.json()
throw new Error(errorData.message || `HTTP ${response.status}`)
}Suggested Fix: if (!response.ok) {
let errorMessage = `HTTP ${response.status}`
try {
const errorData = await response.json()
errorMessage = errorData.message || errorMessage
} catch {
// Response was not JSON - use HTTP status
}
throw new Error(errorMessage)
}How to Verify:
Issue 3: Missing Component CleanupFile: Problem: Note: Since event listeners are attached to elements inside the shadow DOM and the Minor Issues 🟡Issue 1: Inconsistent Import PathsFile: Problem: Current Code: import TPEN from '../../api/TPEN.js'
import { getUserFromToken } from '../iiif-tools/index.js'
import { escapeHtml } from '/js/utils.js'Suggested Fix: import TPEN from '../../api/TPEN.js'
import { escapeHtml } from '/js/utils.js'Issue 2: Accessibility - Missing ARIA Attributes on Loading StateFile: Problem: Suggested Fix: <div class="container" role="status" aria-live="polite" aria-busy="true">
<div class="loader-wrapper">
<div class="spinner" aria-hidden="true"></div>
<h2>Creating Projects</h2>
<p>Importing ${this.#manifests.length} manifest${this.#manifests.length !== 1 ? 's' : ''}...</p>
</div>
</div>Issue 3: Accessibility - Missing Focus StylesFile: Problem: Suggested Fix: button:focus {
outline: 2px solid var(--focus-ring, #005a8c);
outline-offset: 2px;
}
button:focus:not(:focus-visible) {
outline: none;
}Issue 4: Documentation DateFile: The documentation states specific creation dates. Consider if these will become stale or if they should be removed. Suggestions 🔵Suggestion 1: Consider Using Custom Dialog Instead of window.confirm()File: The component uses Suggestion 2: Add Rate Limiting Warning for Large BatchesFile: For very large batch imports (e.g., >10 manifests), consider warning users about potential rate limiting or long wait times. Suggestion 3: Add Visual Progress for Batch ImportsFile: For batch imports, the loading screen shows a static message. Consider showing which manifest is currently being processed (e.g., "Creating project 3 of 5..."). Files Reviewed
Checklist for Developer
|
thehabes
left a comment
thehabes
left a comment
There was a problem hiding this comment.
Tested manually on a localhost deploy. Manifests are imported and projects are generated. I can start logged in or logged out.
See comments from the Claude Code /static-review they are all easy fixes and updates for the ones you don't want to ignore.
Follow up with copilot as well after making changes since it had so much to say earlier and you have done resolutions since.
|
@copilot Please follow up on Claude's recommendations:
|
…omponent (#414) * Initial plan * Fix security and code quality issues in manifest-import component Co-authored-by: cubap <1119165+cubap@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cubap <1119165+cubap@users.noreply.github.com>
Add component lifecycle and concurrency improvements for manifest import: introduce a CleanupRegistry to manage event handlers and call cleanup.run() on disconnect; attach TPEN authentication and wait for tpen-authenticated events, render a need-auth state when not authorized, and guard initialization with a #hasInitialized flag to avoid double-init. Rework #createProjects to perform imports with a worker pool and a concurrency limit (5) while preserving result order and collecting per-manifest errors. Also enhance loader markup with ARIA attributes for better accessibility.
interface for directly creating projects through querystring.
manifest=into multiple projects with safe failingIt doesn't currently...
This contains