Avoid duplicate renders; set project-id on load

[cubap]

Track the current manifest ID in ProjectDetails and skip render() when the manifest hasn't changed to prevent duplicate/expensive re-renders (components/project-details/index.js). Also add a render() helper and set the tpen-project-id attribute on tpen-project-details when the tpen-project-loaded event fires so the component receives the project ID and performs a controlled render (interfaces/project/index.html).

[github-actions]

🚀 Deployed on https://6995fd768b1fd13557f77dc3--tpen-interfaces-preview.netlify.app

[Copilot]

Pull request overview

This PR optimizes the ProjectDetails component to prevent duplicate renders by tracking the current manifest ID. When the tpen-project-loaded event fires, a new render() helper function sets the tpen-project-id attribute on the component, which triggers a controlled render through the attributeChangedCallback.

Changes:

• Added duplicate render prevention in ProjectDetails by tracking _currentManifestId and skipping render when manifest hasn't changed
• Added render() helper function in project/index.html to set tpen-project-id attribute on tpen-project-loaded event

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
components/project-details/index.js	Added _currentManifestId property and early return logic in render() to skip duplicate renders when manifest hasn't changed
interfaces/project/index.html	Added render() helper function and call in tpen-project-loaded event handler to set tpen-project-id attribute

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thehabes]

Static Review Comments

Branch: 433-project-interface-loads-defaultjpg-way-too-many-times
PR: #468 — Avoid duplicate renders; set project-id on load
Review Date: 2026-02-13
Files Changed: 2

Summary

This PR addresses issue #433 where the /project interface was loading default.jpg excessively, causing a black box instead of an image. The fix adds a guard in ProjectDetails.render() to skip rendering when the manifest hasn't changed, and introduces a render() helper in the page script that sets the tpen-project-id attribute when tpen-project-loaded fires.

The root cause is a double-render: onProjectReady() (in connectedCallback) and the page-level tpen-project-loaded listener both trigger render() on the same event. The manifest guard prevents the second pass from doing real DOM work.

The approach is sound and minimal. There are a couple of correctness concerns around the comparison semantics.

Category	Issues Found
🔴 Critical	0
🟠 Major	1
🟡 Minor	2
🔵 Suggestions	2

Major Issues 🟠

Issue 1: Reference-equality guard on array manifest is fragile

File: components/project-details/index.js:129-132
Category: Logic Error

Problem:
Across the codebase, project.manifest is used as an array of URIs (e.g. TPEN.activeProject?.manifest?.[0], and it's passed as the manifestUrls array argument to vault.getWithFallback()). The === operator on arrays compares by reference, not by value.

This works for the specific double-render bug because both render paths reference the same TPEN.activeProject object, so project.manifest is the same array instance both times. However, if the project is re-fetched (producing a new object), or if attributeChangedCallback creates a new Project and calls fetch() (line 83), the manifest array will be a different reference with identical content — the guard will fail to de-duplicate, and the original bug could resurface on those paths.

Additionally, _currentManifestId (named as a string ID) actually stores an array reference, which is misleading.

Current Code:

/** @type {string|null} Track current manifest ID to prevent duplicate renders */
_currentManifestId = null

// ...

// Only render if manifest has changed or first render
if (this._currentManifestId === project.manifest) {
    return
}
this._currentManifestId = project.manifest

Suggested Fix:
Use a stable string representation for comparison so the guard is value-based:

/** @type {string|null} Track current manifest value to prevent duplicate renders */
_currentManifest = null

// ...

// Only render if manifest has changed or first render
const manifestKey = Array.isArray(project.manifest) 
    ? project.manifest.join(',') 
    : project.manifest
if (this._currentManifest === manifestKey) {
    return
}
this._currentManifest = manifestKey

This way the guard works correctly regardless of whether the manifest comes from the same object reference or a re-fetched copy.

How to Verify:

1. Open /project?projectID=<id> — component renders once, image loads correctly
2. Force a second tpen-project-loaded dispatch (e.g. via devtools: TPEN.eventDispatcher.dispatch('tpen-project-loaded', TPEN.activeProject)) — component should not re-render
3. Navigate to a different project — component should re-render with new data

---

Minor Issues 🟡

Issue 1: render() function name in page script is generic and may confuse maintainers

File: interfaces/project/index.html:90-92
Category: Code Hygiene

Problem:
The top-level render() function in the page script only sets the tpen-project-id attribute — it doesn't render anything itself. The name overlaps conceptually with the component's own render() method, which could confuse future maintainers reading through event handlers.

Current Code:

function render() {
    document.querySelector('tpen-project-details').setAttribute('tpen-project-id', TPEN.screen.projectInQuery)
}

Suggested Fix:

function setProjectId() {
    document.querySelector('tpen-project-details').setAttribute('tpen-project-id', TPEN.screen.projectInQuery)
}

---

Issue 2: Guard state is never reset on disconnect

File: components/project-details/index.js:119-123
Category: Potential Bug

Problem:
_currentManifestId is never cleared in disconnectedCallback. If the element is removed from the DOM and later re-inserted (e.g. in a single-page app transition), the stale value would prevent the first render from executing if the same project is still active.

Suggested Fix:

disconnectedCallback() {
    try { this._unsubProject?.() } catch {}
    this.renderCleanup.run()
    this.cleanup.run()
    this._currentManifestId = null
}

How to Verify:
Remove and re-add <tpen-project-details> to the DOM via devtools — the component should render correctly on re-insertion.

---

Suggestions 🔵

Suggestion 1: Guard against null project before accessing .manifest

File: components/project-details/index.js:126-132

The guard accesses project.manifest immediately after const project = this.Project ?? TPEN.activeProject. If both this.Project and TPEN.activeProject are nullish (e.g. during early lifecycle before auth completes), this would throw a TypeError. Adding an early return would make the component more resilient:

render() {
    const project = this.Project ?? TPEN.activeProject
    if (!project) return

    // Only render if manifest has changed or first render
    // ...
}

This is a pre-existing gap (not introduced by this PR), but worth addressing since you're already modifying render().

---

Suggestion 2: Consider whether the page-level render() call is needed at all

File: interfaces/project/index.html:94-96

The component already has onProjectReady(this, this.authgate) in connectedCallback, which fires on tpen-project-loaded and calls authgate() → render(). The page script's additional render() call (which sets tpen-project-id → triggers attributeChangedCallback → calls render() again) is the source of the double-render that the manifest guard must now prevent.

An alternative approach would be to set the tpen-project-id attribute in the HTML (from the query param) or set it once in a load handler that runs before onProjectReady registers, rather than adding a guard to suppress an intentional second trigger. This would eliminate the double-render at the source rather than mitigating it inside the component. However, this is a design-level question and the current approach is a valid minimal fix.

---

Files Reviewed

• components/project-details/index.js — Manifest guard added to render(); functionally correct for the target bug, but comparison semantics are fragile
• interfaces/project/index.html — Page-level render() helper and attribute-setting on project load; works as intended

Checklist for Developer

• Address Major issue: switch from reference equality to value-based comparison for manifest guard
• Review Minor issue: consider resetting _currentManifestId in disconnectedCallback
• Review Minor issue: consider renaming page-level render() to something more descriptive
• Consider Suggestion: guard against null project in render()
• Consider Suggestion: whether the double-trigger architecture is the right long-term approach
• Manual test: open /project?projectID=<id>, verify image loads once without black box
• Manual test: verify project switching still works correctly

[thehabes]

I think am still experiencing the behavior for the following, unless it legit calls default.jpg this many times naturally. In the end, I still just see the black box, it's been over 5 minutes now I don't think it will be resolving.

• http://localhost:4000/project/?projectID=698378c3c19363cc0b368ef4

• http://localhost:4000/project/?projectID=697b9905048e901a6e0e6b45

[cubap]

Worthwhile changes, but ultimately failed to satisfy the issue.

[cubap]

This does not fix #433, as there is a problem with digirati components we are using at the moment.

digirati-co-uk/iiif-canvas-panel#282