Address Issues 13-17 from static review: Fix vault consistency, error handling, and performance issues

[Copilot]

This PR addresses Issues 13-17 from the static review of PR #436, fixing code consistency, removing dead code, improving performance, and enhancing code clarity in the vault implementation.

Changes Made

Issue 13 - Code Consistency (components/annotorious-annotator/line-parser.js)

• Removed inconsistent this.vault = vault assignment from constructor
• Updated all usages to use module-level vault directly, matching pattern used in all other components

Issue 14 - Dead Code Removal (js/vault.js)

• Removed unreachable empty catch block in prefetchDocuments() method
• The catch block was dead code because each promise already has its own .catch() handler

Issue 15 - Performance Fix (js/vault.js)

• Removed duplicate localStorage write in hydrateFromObject() method
• Eliminated redundant explicit localStorage.setItem() call since this.set() already handles caching
• Fixes expensive double-serialization issue for large objects

Issue 16 - Bug Fix (js/vault.js)

• Fixed all() method to correctly work with Map data structure
• Changed from Object.values(this.store) (always returns [] for Maps) to [...this.store.values()]

Issue 17 - Code Clarity (js/utils.js and js/vault.js)

• Changed urlFromIdAndType() to return null instead of empty string for IIIF resource types
• Using null semantically indicates "no URL exists" rather than using empty string as control-flow signal
• Updated vault guard comment for clarity

Testing

• ✅ All files passed JavaScript syntax checks
• ✅ Code review completed with no critical issues
• ✅ CodeQL security scan passed with 0 alerts

These changes improve code consistency, remove dead code, fix performance issues, and enhance code clarity without breaking existing functionality.

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[cubap]

Here is the context from the comment pasted directly in for @copilot to consider:

Minor Issues 🟡
Issue 13: this.vault = vault instance assignment is inconsistent with other components
File: components/annotorious-annotator/line-parser.js:55
Category: Code Consistency

Problem:
AnnotoriousAnnotator assigns this.vault = vault in its constructor and uses this.vault.get(...) throughout. Every other component in this PR uses the module-level vault directly (e.g., vault.get(...)). This creates an inconsistent pattern — readers must check whether this.vault is the same singleton or something different.

Suggested Fix: Use vault directly like all other components, and remove the this.vault assignment.

Issue 14: Empty catch block silently swallows errors in prefetchDocuments
File: js/vault.js:258-260
Category: Error Handling — Dead Code

Problem:
The outer try/catch around Promise.all(promises) is unreachable because every individual promise already has its own .catch() handler that returns null. Promise.all will never reject. The empty catch block is dead code.

Current Code:

try {
await Promise.all(promises)
} catch (err) {
}
Suggested Fix:

await Promise.all(promises)
Issue 15: Double localStorage write in hydrateFromObject
File: js/vault.js:176-179
Category: Performance — Redundant Work

Problem:
this.set(data, storageType) on line 176 internally calls localStorage.setItem(cacheKey, JSON.stringify(item)). Then line 179 does it again explicitly:

this.set(data, storageType) // writes to localStorage internally
const cacheKeyToUse = hasKnownType ? this._cacheKey(dataType, id) : cacheKey
try {
localStorage.setItem(cacheKeyToUse, JSON.stringify(data)) // writes AGAIN
} catch {}
Every hydrateFromObject call serializes and writes the parent object to localStorage twice. For large objects, JSON.stringify is expensive.

Suggested Fix: Remove lines 177-180 (the explicit localStorage.setItem). The set() call already handles it.

Issue 16: all() method uses Object.values() on a Map — always returns []
File: js/vault.js:243-245
Category: Bug in Dead Code

Problem:
this.store is a Map (line 6). Object.values() on a Map returns an empty array because Map entries are not enumerable own string-keyed properties. Currently dead code (no callers found), but will be a surprise to anyone who tries to use it.

all() {
return Object.values(this.store) // Always returns []
}
Suggested Fix:

all() {
return [...this.store.values()]
}
Issue 17: urlFromIdAndType returns empty string for IIIF types — fragile control flow coupling
File: js/utils.js:34-40
Category: Code Clarity

Problem:
The added cases return '' with a comment saying "Return empty string to trigger fetch failure and fallback to seed/embedded." This is using an empty return value as a control-flow signal, which is fragile. The vault's _fetchAndHydrate checks if (!uri) to guard against this, but the coupling between these two files is implicit and undocumented.

Suggested: Consider returning null (semantically "no URL exists") instead of ''. Update the vault guard to match.

[Copilot]

Here is the context from the comment pasted directly in for @copilot to consider:

Minor Issues 🟡
...

All Issues 13-17 have been addressed in commit ba241ab:

Issue 13: Removed this.vault = vault from line-parser.js and updated all usages to use vault directly for consistency.

Issue 14: Removed the unreachable empty catch block from prefetchDocuments().

Issue 15: Removed duplicate localStorage write in hydrateFromObject() - this.set() already handles it.

Issue 16: Fixed all() method to use [...this.store.values()] instead of Object.values(this.store).

Issue 17: Changed urlFromIdAndType to return null instead of '' for IIIF types and updated the vault guard comment.

All changes have been syntax-checked and passed CodeQL security scan.