Finish Quests feature, add image upload + link PATCH, fold in review fixes from #46#47
Finish Quests feature, add image upload + link PATCH, fold in review fixes from #46#47gharezlak wants to merge 9 commits into
Conversation
compatibility.verified was "13.351" while minimum was "14.359" - verified must never be lower than minimum. Looks like a v13/v14 field cross-contamination similar to the incident that caused the original v14 rollback. Reset verified to match minimum until tested against a newer 14.x build.
Previously only the quest title round-tripped to the API; every other field (giver, category, status, success/failure/next-action/resolution) was display-only after first import, objectives were local-only, and Quests sat outside the shared linking system entirely. - Info tab is now a structured edit form instead of a stubbed ProseMirror body (Quest has no generic `description` field in the API); drop the hack that stuffed successDefinition/nextAction into page content on import. - _commitEdits sends the full updateQuest payload, not just questName. - Objectives (add/remove/edit text+status) sync to the API instead of being local-flag-only. - Quest relationships persist via relatedEntityRefs on the quest itself (updateQuest), since the API's link system has no ENTITY_CONFIG entry for Quest in either direction. Links tab now renders real, clickable, unlinkable cards backed by entity IDs instead of static name chips. - Character/Item/Location/Faction sheets get a read-only "Quests" tab (client-side filtered from listQuests(), since there's no reverse lookup endpoint) so relationships are visible from both sides. - reconcile-service now includes quests/journals in the ongoing sync pass, not just first import. - Documented Quest/Journal/Links shapes in the Cursor API rules doc, which previously covered every other entity but not these. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Historically users couldn't import existing Foundry Actor/Item images
because they're stored locally and the module had no way to send image
bytes to the API. Confirmed the API's actual upload shape: a 3-step
presigned-URL flow (init -> PUT bytes directly to R2 -> complete), not
a raw multipart POST. 4.5MB cap and content-sniffing are server-enforced.
- archivist-api.js: initImageUpload/uploadImageBytes/completeImageUpload
plus a uploadEntityImage convenience wrapper running all three steps.
- Character/Item/Location/Faction sheets get an "Upload local image"
header button (GM-only) that reads the linked Actor/Item/Scene's (or
the journal's own) local img via fetch() and pushes it to Archivist.
Not available for Quest — the API has no image support for that type.
- archivist-api.js: added updateLink (PATCH) alongside the existing
createLink/deleteLink so a link's alias can be corrected in place
instead of delete+recreate, once a UI flow needs it.
Other endpoints surveyed but intentionally left for a future pass to
keep this change bounded: unified /campaigns/{id}/search, the
lightweight /entities picker endpoint, session transcript/handout ->
Journal auto-generation, the recordings/transcription pipeline, and
journal-folder CRUD (create/rename/delete).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed zero references anywhere in the repo (grep across .js/.json). Stray backup file, not part of the build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Project Descriptions is documented as "Append an Archivist section to the most likely description field," but projectDescription() only appended for HTML-flagged slots (via mergeArchivistSection). For the one plain-text slot in the registry (dnd5e/pf2e Actor system.details.notes, adapter-registry.js html:false) — which becomes the top candidate for actor subtypes lacking a biography field, e.g. dnd5e's vehicle/group types — the code did stripHtml(archivistHtml) with no reference to the field's existing content at all, silently replacing whatever was there. This matches user reports of losing existing descriptions. Added mergeArchivistPlainSection (merge.js), which does for plain-text fields what mergeArchivistSection already did for HTML ones: wrap the Archivist content in a recognizable marker, append it if none exists yet, and replace only that marked block on future syncs — never touching the rest of the field's content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Quests were already being auto-imported exactly like Factions/Recaps — _importArchivistMissing() pulls them in unconditionally, no reconcile/ selection step, and the count was already being computed (count.imp.quests / plan.importFromArchivist.quests). They just never made it into the summaryRows array the Step 6 table renders from, so the count was invisible before clicking "Start Sync." Added a Quests row identical in shape to the Recaps row, and updated Step 4's helper note (which named Factions/Recaps as auto-imported) to mention Quests too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two real bugs, both regressions from the Quests/image-upload work:
1. The upload-local-image button reused .archivist-edit-toggle's exact
`position: absolute; top: 8px; right: 8px` slot with no offset, so
it rendered directly on top of the edit-mode toggle — completely
covering it. That's why editing a Faction (or any Character/Item/
Location) sheet looked impossible: the button was there, just
unclickable underneath the newer one. Added a distinguishing
.archivist-upload-toggle class with `right: 48px` so they sit side
by side.
2. _onRender fired _renderLinkedGrids()/_renderActorItemCards()/
_renderRelatedQuests() without awaiting them, all async (dynamic
import + DOM work). Verified against Foundry's own core source
(client/applications/api/application.mjs): _render() does
`await this._doEvent(this._onRender, ...)`, but since our _onRender
wasn't declared async, that await resolved as soon as the
synchronous body returned — before grid population finished. Opening
a sheet by clicking a card in another custom sheet chains
.render({force:true}).then(bringToFront) right after that early
resolution, so the window could appear with cards populated but tab
counts (the "(N)" suffix, set later by the same population pass)
never applied — a real, reproducible gap, not just a cosmetic flash.
Made _onRender (base class, plus the Location and Quest overrides
that call super._onRender) async and properly awaited so Foundry's
pipeline doesn't consider the render complete until population
actually finishes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…l work A collaborator independently built the same quest/journal feature set on his fork (upstream/quest-journal-review-fixes, from the same 40d439f base). His module.json/compatibility changes and the ask-chat-sidebar-tab.js deletion are NOT ported — that branch reintroduces the exact incident this whole effort started from (a v14-only build with compat.minimum low enough for v13 clients to install, missing the Sidebar.TABS registration v13 actually needs). But several of his bug fixes are real, verified independently against this branch's actual call sites, and are ported here: - sync-dialog.js: journal content was being dropped entirely on import/sync (read row.description/row.summary but never row.content, which is where Journal's actual content lives) — the most serious bug found. Also: shift+click range-select was getting flipped back by the row-level click handler (missing stopPropagation); journal names were interpolated unescaped into the delete-confirmation dialog's HTML; no folder fallback for journals/quests on worlds that predate this feature, landing them in the world root. Wholesale-replaced with the fixed version, then re-added this branch's own relatedEntityRefs hydration (needed by the richer quest-linking Links tab) which his simpler version doesn't populate. - world-setup-dialog.js: added _updateSyncStatusUI() (direct progress-bar DOM update) to the six "always imported in full" loops (characters, items, locations, factions, journals, quests), replacing a full await this.render() per entity — a real perf cost on large imports. - archivist-sync.js: the realtime page-content-edit hook was PATCHing the quest's unchanged name back to the API on every content keystroke commit; quest content isn't a synced field and renames are already handled by a separate title-sync hook, so this was pure waste. Now a no-op return. - archivist-api.js: _normalizeQuestPayload rewritten from a blocklist (rename+delete known keys, pass everything else through) to a strict whitelist, matching QuestCreate/QuestUpdate's extra="forbid" schema. Verified no current call site was actually vulnerable (all send narrow literal payloads), but the whitelist is a strictly safer contract for any future caller — verified functionally: read-only fields (questGiverId, progressLogEntries, first/lastSession) are dropped, empty-text objectives are dropped (API requires min_length=1), relatedEntityRefs' nested keys stay camelCase as the backend's alias_generator expects. Also fixed the keepalive guard: Chromium rejects keepalive fetches over ~64KB, so it's now conditional on body size instead of unconditional for all writes. - merge.js: stripHtml no longer calls foundry.utils.TextEditor.stripHTML, which doesn't exist in either v13 or v14 core (verified against the actual installed Foundry app) — always fell through to the manual DOM-based strip anyway, just via a dead branch first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: camrun01 <cameron.b.llewellyn@gmail.com>
Adopts camrun91's approach: QuestPageSheetV2._localize(key) tries game.i18n.localize() and returns null (not the raw key) on a miss or when i18n isn't ready yet, so callers can chain `|| fallback` cleanly. statusLabel/categoryLabel now try ARCHIVIST_SYNC.quest.status.*/ category.* (already present in lang/en.json from the earlier merge of his branch's new keys) before falling back to the hardcoded English strings that were the only source before. Not in scope here: the edit-mode <select> option text in quest.hbs (questCategory/status dropdowns) — those don't exist on his branch at all (it kept the free-text Info tab instead of the structured form), so there's nothing of his to adopt for them; they're still hardcoded English, same as before this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: camrun01 <cameron.b.llewellyn@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
archivist-sync/scripts/dialogs/sync-dialog.js
Lines 557 to 558 in bdf8d73
Diff structured quest fields in manual sync
For linked Quest sheets, the manual Sync dialog now only compares the quest name here and then falls through to generic description/link checks, but Quest has no generic description and its relationships/objectives/status live in questData/relatedEntityRefs. If Archivist changes a quest's status, objectives, progress, or related entities, no diff is shown and _applyDiff() cannot pull those updates; add Quest-specific comparisons and application for the structured fields.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nextAction: readVal('.quest-next-action-input'), | ||
| resolution: readVal('.quest-resolution-input'), | ||
| }; | ||
| result = await archivistApi.updateQuest(apiKey, archivistId, payload); |
There was a problem hiding this comment.
When a GM edits quest metadata, this branch only PATCHes Archivist and never updates flags.questData, but _prepareContext() reads the displayed quest fields from those flags and _toggleEditMode() immediately re-renders after this call. The edit therefore appears to disappear locally, and entering edit mode again can send the stale flag values back to the API. Update local questData from the payload or successful response before re-rendering.
Useful? React with 👍 / 👎.
| <input type="text" class="quest-objective-text-input" value="{{this.text}}" /> | ||
| <select class="quest-objective-status-select"> |
There was a problem hiding this comment.
When a GM enters quest edit mode, the template renders these objective text/status controls, but QuestPageSheetV2._onRender() immediately calls _renderQuestObjectives(), which clears .quest-objectives-list and rebuilds display-only spans. That removes the new inputs before _wireQuestEditActions() can attach the change handlers, so objective text/status cannot actually be edited.
Useful? React with 👍 / 👎.
| // (title/image handled generically by ensureSheet; the rest is quest-specific). | ||
| for (const q of questsData) { | ||
| const entity = { ...q, name: q.questName || 'Quest' }; | ||
| const j = await ensureSheet(entity, 'quest'); |
There was a problem hiding this comment.
When full reconcile creates a missing quest here, it goes through journalManager.create() → Utils.createArchivistJournal(), whose sheetClassMap does not include quest (unlike createCustomJournalForImport). The journal gets quest flags and questData, but its core sheet class is empty, so it opens as a default JournalEntry instead of QuestPageSheetV2. Use the import helper or add the quest sheet mapping on this creation path.
Useful? React with 👍 / 👎.
| const card = document.createElement('div'); | ||
| card.className = 'archivist-card'; | ||
| card.dataset.archivistId = entityId; | ||
| card.innerHTML = `<img src="${img}" alt=""/><span class="name">${foundry.utils.escapeHTML(name)}</span><div class="actions"><button type="button" data-action="unlink-quest-entity" data-entity-type="${type}" data-entity-id="${foundry.utils.escapeHTML(entityId)}" title="Unlink"><i class="fas fa-unlink"></i></button></div>`; |
There was a problem hiding this comment.
For any non-GM who can open a quest sheet with related entities, this renders an unlink button and _wireQuestEditActions() binds it without checking game.user.isGM; if the user has update permission and an API key is configured, clicking it mutates questData and PATCHes the quest links. Other unlink paths in this sheet guard GM access, and the template only describes unlinking under setup.isGM, so the button/handler should be GM-only as well.
Useful? React with 👍 / 👎.
| const j = await ensureSheet(entity, 'quest'); | ||
| try { | ||
| const flags = j.getFlag(CONFIG.MODULE_ID, 'archivist') || {}; | ||
| flags.questData = { |
There was a problem hiding this comment.
When listQuests() returns summary rows without full structured fields (the setup import path already handles this by calling getQuest() when objectives are absent), this assignment replaces the whole local questData and defaults missing objectives, progress, and related refs to empty arrays. Running full reconcile on an existing quest can therefore wipe its locally stored objectives/links until the quest is re-imported; hydrate with getQuest() or preserve existing fields when the list row omits them.
Useful? React with 👍 / 👎.
| for (const it of itemsData) await ensureSheet(it, 'item'); | ||
| for (const l of locations) await ensureSheet(l, 'location'); | ||
| for (const f of factions) await ensureSheet(f, 'faction'); | ||
| for (const j of journalsData) await ensureSheet(j, 'journal'); |
There was a problem hiding this comment.
For Archivist Journals that do not yet exist in Foundry, this new loop sends each row through the generic ensureSheet() path, which creates the journal with text: '' and never copies content or summary. Because the new empty entry is then flagged with the Archivist id, the Sync dialog treats it as already linked and no longer offers a real import, leaving the journal body missing locally.
Useful? React with 👍 / 👎.
| const card = document.createElement('div'); | ||
| card.className = 'archivist-card'; | ||
| card.dataset.archivistId = String(q.id); | ||
| card.innerHTML = `<img src="icons/svg/scroll.svg" alt=""/><span class="name">${foundry.utils.escapeHTML(name)}</span>`; |
There was a problem hiding this comment.
When Hide by Ownership is enabled, the existing linked grids filter cards through testUserPermission, but this new quest tab renders every API match by name without checking whether the current user can observe the quest journal. A non-GM who can open a linked character/item/location can therefore see private quest names/relationships that should remain hidden; filter matches through the same ownership check before rendering cards.
Useful? React with 👍 / 👎.
| for (const it of itemsData) await ensureSheet(it, 'item'); | ||
| for (const l of locations) await ensureSheet(l, 'location'); | ||
| for (const f of factions) await ensureSheet(f, 'faction'); | ||
| for (const j of journalsData) await ensureSheet(j, 'journal'); |
There was a problem hiding this comment.
This new reconcile path creates missing Archivist Journals through journalManager.create(), which calls Utils.createArchivistJournal(); that helper's sheetClassMap still has no journal entry, while the other import paths use createCustomJournalForImport() which does. The created journal is flagged as an Archivist journal but opens with the default Foundry sheet instead of JournalPageSheetV2, so users lose the intended edit/sync UI.
Useful? React with 👍 / 👎.
1 participant
Context
This branch was built independently, in parallel with #46, both starting from the same point in history and both finishing the same "half-built Quests feature" work — before I saw #46 existed. Once I found it, I diffed the two implementations file-by-file and folded in every genuine bug fix from #46 (with credit — several commits below are co-authored to reflect that). This PR is meant to supersede #46 rather than sit alongside it; happy to talk through anything below that looks wrong.
What's here
Quests, finished properly (previously only the title round-tripped to the API):
_commitEditsnow sends the fullupdateQuestpayload instead of just the name.relatedEntityRefson the quest itself (the API has noENTITY_CONFIGentry for Quest in the generic/linkssystem in either direction, so it can't use the same link path as every other entity type). Links tab renders real clickable/unlinkable cards backed by entity IDs, not static name chips.listQuests(), since there's no reverse-lookup endpoint) so the relationship is visible from both sides.reconcile-service.js), not just first import.New API surface used:
init→ directPUTto R2 →complete). Not available for Quest — the API has no image support for that type.PATCH(updateLink) alongside the existing create/delete, for re-pointing a link's alias in place.Folded in from #46 (verified independently, not just taken on faith):
sync-dialog.jsreadrow.description/row.summarybut neverrow.content, which is where Journal's actual content lives. The most serious bug in either branch._normalizeQuestPayloadrewritten from a blocklist to a strict whitelist, matchingQuestCreate/QuestUpdate'sextra="forbid"schema. No call site in my branch was actually vulnerable to the 422 this describes, but the whitelist is a strictly safer contract for any future caller — verified functionally (read-only fields dropped, empty-text objectives dropped,relatedEntityRefs' nested keys correctly stay camelCase).keepaliveon writes is now conditional on body size — Chromium rejects keepalive fetches over ~64KB.stopPropagation).stripHtmlno longer callsfoundry.utils.TextEditor.stripHTML, which doesn't exist in either v13 or v14 core (verified against the actual installed app) — it always fell through to the manual DOM strip anyway, just via a dead branch first.game.i18nwith English fallback.Deliberately NOT folded in from #46
module.jsonthere setscompatibility.minimum: "13.351"while the code has already droppedask-chat-sidebar-tab.js/theSidebar.TABSregistration (v13's actual mechanism for the chat sidebar tab — v14 can't use it the same way). That combination would let a v13 client install a build that's missing the chat tab. Given the whole reason this got picked back up was v14 code breaking for v13 users once before, I didn't want to reintroduce that shape of bug even scoped to the beta channel. This branch keepscompatibility.minimum: "14.359"(v14-only) and handles v13 separately — see the parity PR againstmain.Also not touched: the wizard cleanup in
world-setup-dialog.js/.hbs(removing the old "Step 4 mapping" UI) that's bundled into #46 — that's real, unrelated dead-code cleanup that predates both of our branches; worth doing but as its own PR so it doesn't get lost in this diff.Heads up, unrelated to this PR
Noticed #45 (
staging→main, draft) is still open. Merging that as-is would collapse v13 support into a single v14-onlymain— probably worth a conversation before it goes anywhere, independent of this PR.Testing
npm run lint: 0 errors. All touched files passnode --check. No test suite exists in this repo. I ran functional checks in isolation for the payload whitelist (confirmed read-only fields get dropped, empty objectives get dropped) and the plain-text description merge fix, but this hasn't been exercised in a live Foundry world yet — worth a manual pass on quest editing/linking and the image upload flow before merging.🤖 Generated with Claude Code