Skip to content

Implement #1#2

Draft
habesoftware-claude-connector[bot] wants to merge 1 commit into
mainfrom
full-stack-issue1-ede9443
Draft

Implement #1#2
habesoftware-claude-connector[bot] wants to merge 1 commit into
mainfrom
full-stack-issue1-ede9443

Conversation

@habesoftware-claude-connector

Copy link
Copy Markdown

Review all changes carefully before merging.

Closes #1

Files Changed

.github/workflows/generate-manifests.yml  |   2 +-
 README.md                                 |  37 +++++++-
 package.json                              |   2 +-
 projects/example-project/README.md        |  11 ++-
 projects/example-project/manifest-v3.json | 119 +++++++++++++++++++++++
 projects/example-project/manifest-v4.json | 119 +++++++++++++++++++++++
 scripts/config.js                         |  11 ++-
 scripts/generate.js                       | 152 +++++++++++++++++++++---------
 scripts/validate.js                       |  49 +++++++---
 9 files changed, 435 insertions(+), 67 deletions(-)

Static Review Comments

Branch: full-stack-issue1-ede9443
Review Date: 2026-06-11
Reviewer: Static Reviewer - @thehabes

Bryan and Claude make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

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

Critical Issues 🔴

None found.

Major Issues 🟠

None found.

Minor Issues 🟡

🟡 Issue 1: Cross-repository upgrades from issue #1 are out of scope for this run (NEEDS_HUMAN)

File: N/A
Category: Scope / Task Completion

Problem:
GitHub issue #1 has two parts. The first part — generating IIIF Presentation 4 compliant manifests in this repository with easy version switching — is implemented here: every project now publishes manifest.json (primary, switchable via presentationVersion in info.yml, defaulting to 3 so the existing public URL contract is unchanged), plus always-available manifest-v3.json and manifest-v4.json with identical canvas/page/annotation ids across all serializations. The second part — auditing other TPEN-related repositories in the CenterForDigitalHumanities organization and submitting PRs so they accept Presentation 4 manifests as an Image Resource — cannot be performed from this non-interactive run, which is restricted to this repository and cannot create branches, push, or open PRs.

Current Code:

Not applicable — work item exists outside this repository.

Suggested Fix:

A human (or separate per-repository task) should open issues/PRs in the other
TPEN-related repositories (e.g. TPEN services and interfaces) to accept
manifests with @context http://iiif.io/api/presentation/4/context.json.
The version-pinned manifest URLs published by this change
(.../manifest-v3.json and .../manifest-v4.json) give those tools a stable
target for each version during the transition.

How to Verify:

  1. Review issue Prepare for v4 #1's second paragraph for the cross-repository requirement.
  2. Confirm this change only modifies files within Github-Manifests.

Suggestions 🔵

🔵 Issue 2: Project README template hardcodes versions 3 and 4 while generation is config-driven

File: scripts/generate.js:714
Category: Maintainability

Problem:
generateProject writes the version-pinned manifests by iterating SUPPORTED_PRESENTATION_VERSIONS, but generateProjectReadme takes explicit manifestV3Url/manifestV4Url parameters and hardcodes the "Generated Outputs" and "Links" entries for versions 3 and 4. If a future Presentation version is added to IIIF_CONTEXTS, the new manifest file would be generated and validated but silently missing from project READMEs until the template is also updated.

Current Code:

function generateProjectReadme(projectName, { manifestUrl, manifestV3Url, manifestV4Url, presentationVersion }) {

Suggested Fix:

// Pass the versionedUrls map and render entries by iteration instead:
function generateProjectReadme(projectName, { manifestUrl, versionedUrls, presentationVersion }) {
  const versionLinks = Object.entries(versionedUrls)
    .map(([version, url]) => `- [Presentation ${version} manifest](${url})`)
    .join("\n")
  // ...embed versionLinks in the template
}

How to Verify:

  1. Add a hypothetical version entry to IIIF_CONTEXTS in scripts/config.js.
  2. Run npm run generate:all and confirm the project README lists the new manifest without further template edits.

🔵 Issue 3: Validator does not enforce the cross-version canvas id guarantee

File: scripts/validate.js:54
Category: Validation Coverage

Problem:
The documentation promises that canvas, page, and annotation ids are identical across manifest.json, manifest-v3.json, and manifest-v4.json so annotations remain valid whichever version a tool loads. The generator implements this (all serializations share canvases derived from the primary manifest URL), but validate.js checks each file independently and would not catch a future regression that breaks id stability across serializations.

Current Code:

async function validateProject(projectName) {
  await validateManifestFile(projectName, "manifest.json", undefined)

  for (const version of SUPPORTED_PRESENTATION_VERSIONS) {
    await validateManifestFile(projectName, `manifest-v${version}.json`, version)
  }

  return `${projectName}: OK`
}

Suggested Fix:

// Collect canvas ids from each parsed manifest and compare:
const canvasIds = manifests.map((manifest) => manifest.items.map((canvas) => canvas.id).join("\n"))
if (new Set(canvasIds).size !== 1) {
  fail(`${projectName}: canvas ids differ between manifest serializations.`)
}

How to Verify:

  1. Manually edit a canvas id in projects/example-project/manifest-v4.json.
  2. Run npm run validate:all and confirm it fails with a cross-version id mismatch error.

If there are significant code changes in response to this review please test those changes. Run the application manually and test. Run internal programmatic tests when applicable.

Generated via Full Stack Developer by HabeSoftware
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prepare for v4

0 participants