Refactor transcription interfaces and improve event handling #385
Conversation
Swapped the default components for /transcribe and /transcribe-simple interfaces, making tpen-simple-transcription the default for /transcribe and tpen-transcription-interface for /transcribe-simple. Standardized interface selection using a data attribute for tool components. Improved event handling for layer changes and error feedback in simple transcription, including toast notifications for loading errors and empty pages. Updated layer-selector to dispatch a new event for layer changes and maintained backward compatibility.
Introduces a 'tpen-column-selected' event dispatched from the column selector and handled in both simple and standard transcription interfaces. This allows the active line to update based on column selection, improving integration between the column selector and transcription components while maintaining legacy support.
Eliminated code related to the old transcription interface, including the getTpenNodes and applyLineSelection methods, and removed their usage from selectColumn. This streamlines the component and reduces maintenance of deprecated functionality.
Update the 'tpen-column-selected' event listener to correctly destructure the event detail, ensuring columnData is properly accessed.
Updated links in both SimpleTranscriptionInterface and TranscriptionInterface to use 'projectID' instead of 'projectId' as the query parameter when directing users to manage their project. This ensures consistency and correct routing.
In both SimpleTranscriptionInterface and TranscriptionInterface, if no active tool is set, the first available tool from the project's tools list is now selected by default. This ensures the right pane always displays a tool if any are available.
This is because we're using a cheap version of Vault and will be fixed later
Claude Code
|
| Category | Issues Found |
|---|---|
| Critical | 2 |
| Major | 4 |
| Minor | 5 |
| Suggestions | 4 |
Critical Issues
1. Layer Selector Value Mismatch Bug
File: components/layer-selector/index.js:89
Category: Logic Error
The layer selector uses layer.URI to find the selected layer, but the option values are set to layer["@id"]. This will cause layer selection to silently fail because selectedLayer will always be undefined.
Current Code:
const selectedURI = e.target.value
const selectedLayer = this.layers.find((layer) => layer.URI === selectedURI)Suggested Fix:
const selectedURI = e.target.value
const selectedLayer = this.layers.find((layer) => layer["@id"] === selectedURI)2. Event Detail Destructuring Inconsistency
File: components/simple-transcription/index.js:376
Category: Logic Error
The tpen-column-selected event handler incorrectly destructures the event data. The dispatcher wraps the payload in a detail property via CustomEvent, but this handler destructures incorrectly, causing columnData.lineIndex to be undefined.
Current Code:
TPEN.eventDispatcher.on('tpen-column-selected', ({detail: columnData}) => {
if (typeof columnData.lineIndex === 'number') {
TPEN.activeLineIndex = columnData.lineIndex
this.updateLines()
}
})Suggested Fix:
TPEN.eventDispatcher.on('tpen-column-selected', (event) => {
const columnData = event.detail
if (typeof columnData?.lineIndex === 'number') {
TPEN.activeLineIndex = columnData.lineIndex
this.updateLines()
}
})Major Issues
3. Event Listener Memory Leak in TranscriptionInterface
File: interfaces/transcription/index.js:247-258
Category: Memory Leak / Missing Cleanup
The TranscriptionInterface class adds event listeners for tpen-layer-changed and tpen-column-selected but never removes them. This class lacks a disconnectedCallback() entirely, which could cause memory leaks if the component is removed and re-added to the DOM.
Suggested Fix:
Store handler references and add cleanup:
// In addEventListeners():
this._layerChangeHandler = (layerData) => {
this.updateLines()
}
this._columnSelectedHandler = (event) => {
const columnData = event.detail
if (typeof columnData?.lineIndex === 'number') {
TPEN.activeLineIndex = columnData.lineIndex
this.updateLines()
}
}
TPEN.eventDispatcher.on('tpen-layer-changed', this._layerChangeHandler)
TPEN.eventDispatcher.on('tpen-column-selected', this._columnSelectedHandler)
// Add new method:
disconnectedCallback() {
if (this._layerChangeHandler) {
TPEN.eventDispatcher.off('tpen-layer-changed', this._layerChangeHandler)
}
if (this._columnSelectedHandler) {
TPEN.eventDispatcher.off('tpen-column-selected', this._columnSelectedHandler)
}
}4. PostMessage Security - Using Wildcard Origin
File: components/simple-transcription/index.js:826-835
Category: Security Concern
Multiple postMessage calls use "*" as the target origin, which could leak sensitive data to malicious iframes if a tool is compromised.
Suggested Fix:
const toolOrigin = new URL(tool.url).origin
iframe.contentWindow?.postMessage(
{
type: "MANIFEST_CANVAS_ANNOTATIONPAGE_ANNOTATION",
// ...
},
toolOrigin
)5. Missing Origin Validation in Message Handler
File: components/simple-transcription/index.js:881-910
Category: Security Concern
The #handleToolMessages method processes incoming messages from any origin without validation.
Suggested Fix:
#handleToolMessages(event) {
// Validate message origin against loaded tools
const toolOrigins = TPEN.activeProject?.tools
?.filter(t => t.url)
?.map(t => new URL(t.url).origin) ?? []
if (!toolOrigins.includes(event.origin)) {
return // Ignore messages from unknown origins
}
// ... rest of handler
}6. Duplicate Script Loading Prevention Missing
File: interfaces/transcription/index.js:324-341
Category: Bug / Race Condition
The loadRightPaneContent() method loads tool scripts without checking if they're already being loaded. This differs from SimpleTranscriptionInterface which properly checks for existing scripts.
Suggested Fix:
const scriptId = `tool-script-${tool.toolName}`
const existingScript = document.getElementById(scriptId)
if (existingScript) {
return
}
const script = document.createElement('script')
script.type = 'module'
script.src = tool.url
script.id = scriptId
script.onload = () => {
rightPane.innerHTML = `<${tagName}></${tagName}>`
}
document.head.appendChild(script)Minor Issues
7. Debug Statement Left in Production Code
File: components/simple-transcription/index.js:790
console.debug('Loading tool:', { tool, tagName: tool.custom?.tagName, url: tool.url, location: tool.location })Remove this debug statement or wrap in a development mode check.
8. Inconsistent Event Handler Data Access
Files: Multiple
The codebase has inconsistent patterns for accessing event data:
- SimpleTranscriptionInterface uses
({detail: columnData})(incorrect) - TranscriptionInterface uses
(columnData)(also incorrect) - Both should use
(event) => { const data = event.detail; ... }
9. Toast Notification May Fire Repeatedly
File: components/simple-transcription/index.js:544-548
The info toast "This page has no line annotations" fires every time updateLines() is called on an empty page. Consider tracking whether the toast was already shown for the current page.
10. Missing Null Check in Layer Change Handler
File: components/simple-transcription/index.js:369-373
The handler resets activeLineIndex to 0 but doesn't verify if the new layer has any pages or items.
Suggestions
- Add JSDoc Documentation for the new events tpen-layer-changed and tpen-column-selected
- Extract Shared Logic between SimpleTranscriptionInterface and TranscriptionInterface into a base class
- Add Unit Tests for the new event-based communication
- Consider TypeScript or JSDoc type definitions for event payloads
Positive Observations
- Good architectural decision: Moving to event-based communication improves component decoupling and testability
- IIIF compatibility: Adding support for Presentation API v2 alongside v3 is thoughtful
- Error feedback: New toast notifications provide clear user feedback
- Standardized interface discovery: The data-interface-type="transcription" attribute is cleaner than tag-name checking
- Default tool selection: Auto-selecting the first available tool improves UX
Checklist
- Fix Critical Issue 1: Layer selector value mismatch
- Fix Critical Issue 2: Event detail destructuring
- Add disconnectedCallback to TranscriptionInterface
- Review postMessage security improvements
- Fix duplicate script loading prevention
- Remove console.debug statement
- Run full test suite after changes
- Manual test layer and column selection in both interfaces
Updated event handler signatures for 'tpen-column-selected' to consistently use event.detail. In TranscriptionInterface, event handlers are now stored as instance properties and properly removed in disconnectedCallback to prevent memory leaks.
Introduces state tracking to avoid showing duplicate toast/alert messages when a page has no line annotations in both SimpleTranscriptionInterface and TranscriptionInterface. Also ensures notification state is cleared when lines are present, and improves event detail handling and tool script loading logic.
Consolidates and enhances handling of tool messages for line navigation in both SimpleTranscriptionInterface and TranscriptionInterface. Now supports multiple message types and flexible line ID matching (full URI or ID part). Also ensures proper event listener cleanup and updates tool filtering logic in SplitscreenTool to default to enabled when custom.enabled is not set.
Static Review CommentsBranch: SummaryThis PR refactors transcription interfaces by swapping the default components between
Major Issues 🟠Issue 1: Memory Leak - Missing Event Cleanup in SimpleTranscriptionInterfaceFile: Problem: Current Code: // In addEventListeners() - handlers registered as anonymous functions
TPEN.eventDispatcher.on('tpen-layer-changed', (event) => {
const layerData = event?.detail
if (this.#page?.items?.length > 0) {
TPEN.activeLineIndex = 0
}
this.updateLines()
})
TPEN.eventDispatcher.on('tpen-column-selected', (ev) => {
const columnData = ev.detail
if (typeof columnData.lineIndex === 'number') {
TPEN.activeLineIndex = columnData.lineIndex
this.updateLines()
}
})Suggested Fix: // In addEventListeners():
this._layerChangeHandler = (event) => {
const layerData = event?.detail
if (this.#page?.items?.length > 0) {
TPEN.activeLineIndex = 0
}
this.updateLines()
}
TPEN.eventDispatcher.on('tpen-layer-changed', this._layerChangeHandler)
this._columnSelectedHandler = (ev) => {
const columnData = ev.detail
if (typeof columnData.lineIndex === 'number') {
TPEN.activeLineIndex = columnData.lineIndex
this.updateLines()
}
}
TPEN.eventDispatcher.on('tpen-column-selected', this._columnSelectedHandler)
// In disconnectedCallback():
if (this._layerChangeHandler) {
TPEN.eventDispatcher.off('tpen-layer-changed', this._layerChangeHandler)
}
if (this._columnSelectedHandler) {
TPEN.eventDispatcher.off('tpen-column-selected', this._columnSelectedHandler)
}How to Verify:
Issue 2: Memory Leak - Uncleaned Event Listeners in TranscriptionInterfaceFile: Problem: Current Code: TPEN.eventDispatcher.on('tpen-transcription-previous-line', () => {
iframe.contentWindow?.postMessage(
{ type: "SELECT_ANNOTATION", lineId: this.#page?.items?.[TPEN.activeLineIndex]?.id },
"*"
)
})
TPEN.eventDispatcher.on('tpen-transcription-next-line', () => {
iframe.contentWindow?.postMessage(
{ type: "SELECT_ANNOTATION", lineId: this.#page?.items?.[TPEN.activeLineIndex]?.id },
"*"
)
})Suggested Fix: // Clean up previous listeners before adding new ones
this.#cleanupToolLineListeners()
const sendLineSelection = () => {
const activeLineId = this.#page?.items?.[TPEN.activeLineIndex]?.id ?? null
iframe.contentWindow?.postMessage(
{ type: "SELECT_ANNOTATION", lineId: activeLineId },
"*"
)
}
this.#toolLineListeners = sendLineSelection
TPEN.eventDispatcher.on('tpen-transcription-previous-line', sendLineSelection)
TPEN.eventDispatcher.on('tpen-transcription-next-line', sendLineSelection)How to Verify:
Issue 3: Memory Leak - Window Event Listeners Not Cleaned UpFile: Problem: Current Code: window.addEventListener('keydown', e => {
if (e.key === 'Escape') closeSplitscreen()
})Suggested Fix: // In addEventListeners():
this._escapeHandler = (e) => {
if (e.key === 'Escape') closeSplitscreen()
}
window.addEventListener('keydown', this._escapeHandler)
// In disconnectedCallback():
if (this._escapeHandler) {
window.removeEventListener('keydown', this._escapeHandler)
}How to Verify:
Issue 4: Security - postMessage with Wildcard OriginFile: Multiple locations Problem: Locations:
Current Code: iframe.contentWindow?.postMessage(
{
type: "MANIFEST_CANVAS_ANNOTATIONPAGE_ANNOTATION",
manifest: TPEN.activeProject?.manifest?.[0] ?? '',
canvas: this.#canvas?.id ?? this.#canvas?.['@id'] ?? this.#canvas ?? '',
annotationPage: this.fetchCurrentPageId() ?? this.#page?.id ?? '',
annotation: TPEN.activeLineIndex >= 0 ? this.#page?.items?.[TPEN.activeLineIndex]?.id ?? null : null,
columns: projectPage?.columns || []
},
'*'
)Suggested Fix: // Store the tool origin when creating the iframe
const toolOrigin = new URL(tool.url).origin
iframe.contentWindow?.postMessage(
{ /* ... */ },
toolOrigin
)How to Verify:
Minor Issues 🟡Issue 1: Code Duplication - Identical handleToolMessages MethodFile: Problem: How to Verify: Issue 2: Empty Event HandlersFile: Problem: Current Code: this.addEventListener('drawer-opening', () => {
})
this.addEventListener('drawer-closing', () => {
})Suggested Fix: How to Verify: Issue 3: Unused VariableFile: Problem: Current Code: TPEN.eventDispatcher.on('tpen-layer-changed', (event) => {
// Prefer event.detail for consistency; guard against empty pages
const layerData = event?.detail // Never used
if (this.#page?.items?.length > 0) {
TPEN.activeLineIndex = 0
}
this.updateLines()
})Suggested Fix: Issue 4: Potential Null ReturnFile: Problem: Current Code: fetchCanvasesFromCurrentLayer() {
const currentLayer = TPEN.activeProject?.layers.find(layer => layer.pages.some(page => page.id.split('/').pop() === TPEN.screen.pageInQuery))
const canvases = currentLayer?.pages.flatMap(page => {
return {
id: page.target,
label: page.label
}
})
return canvases // Could be undefined
}Suggested Fix: return canvases ?? []How to Verify: Suggestions 🔵Suggestion 1: Extract Shared Tool Message HandlingConsider extracting the // utilities/toolMessageHandler.js
export function createToolMessageHandler(getPage, setActiveIndex, updateLines) {
return function(event) {
const lineId = event.data?.lineId ?? event.data?.lineid ?? event.data?.annotation
if (!lineId) return
if (["CURRENT_LINE_INDEX", "RETURN_LINE_ID", "SELECT_ANNOTATION", "NAVIGATE_TO_LINE"].includes(event.data?.type)) {
const lineIndex = getPage()?.items?.findIndex(item => {
const itemId = item.id ?? item['@id']
return itemId === lineId || itemId?.endsWith?.(`/${lineId}`) || itemId?.split?.('/').pop() === lineId
})
if (lineIndex !== undefined && lineIndex !== -1) {
setActiveIndex(lineIndex)
updateLines()
}
}
}
}Suggestion 2: Consider Using a Cleanup Registry PatternBoth interfaces have multiple event listeners that need cleanup. Consider a pattern to track and clean up all listeners: #cleanupRegistry = []
registerListener(target, event, handler, options) {
target.addEventListener(event, handler, options)
this.#cleanupRegistry.push(() => target.removeEventListener(event, handler, options))
}
registerDispatcherListener(event, handler) {
TPEN.eventDispatcher.on(event, handler)
this.#cleanupRegistry.push(() => TPEN.eventDispatcher.off(event, handler))
}
disconnectedCallback() {
this.#cleanupRegistry.forEach(cleanup => cleanup())
this.#cleanupRegistry = []
}Files Reviewed
Checklist for Developer
|
Swapped the default components for /transcribe and /transcribe-simple interfaces, making tpen-simple-transcription the default for /transcribe and tpen-transcription-interface for /transcribe-simple. Standardized interface selection using a data attribute for tool components. Improved event handling for layer changes and error feedback in simple transcription, including toast notifications for loading errors and empty pages. Updated layer-selector to dispatch a new event for layer changes and maintained backward compatibility.