Bring the v13 line to parity with v14 (Quests, image upload, bug fixes)#48
Bring the v13 line to parity with v14 (Quests, image upload, bug fixes)#48gharezlak wants to merge 6 commits into
Conversation
Investigation found v13 and v14 already share the same ApplicationV2 sheet framework (contrary to the assumption that v13 used a legacy FormApplication) — the two lines diverged in features/fixes, not platform. Diffed main against the v14 line's pre-Quest-work state to scope this precisely rather than guessing. Ported wholesale (confirmed no v13-only content lost in each case): - Full Quest feature (this session's finished version, not the half-built one) and Journal support: new quest.hbs/journal.hbs sheets, archivist-api.js CRUD + normalization, config/utils map entries, reconcile-service/journal-manager quest+journal handling. - Both API augments from the v14 branch: local image upload (Character/Item/Faction/Location) and link PATCH. - Sync dialog UX fixes: shift-click multi-select, live progress indicator during sync (ported templates/sync-dialog.hbs + styles/sync-dialog.css to match), per-row error handling with a fail/success count instead of a blanket message, confirm-before- delete. - Bug fixes: markdownToStoredHtml's double-escaping/library-lookup fix, a sidebar tab-ordering fix (insertBefore instead of append, applied to both places it was duplicated), session-summary now markdown-converted before sync like every other entity type. - Dead code: removed the unreferenced scripts/modules/utils.js.bak (confirmed zero references on either line). - New "Quests" tab on Character/Item/Location/Faction sheets, and the Cursor API docs' Quest/Journal/Links section. Deliberately NOT ported: - The Sidebar.TABS removal — v14 dropped it because that dynamic-tab API doesn't work there, but it's the mechanism v13 actually uses for the chat sidebar tab today. Kept v13's registration in both the `init` and `setup` hooks, layered the rest of the v14 file's Quest/ Journal registrations and hooks on top. - module.json stays untouched (1.x / compat 13.x). Included as a side effect of a wholesale file copy (flagged, not hand- picked): world-setup-dialog.js's large diff is mostly an unrelated dead-code removal (a legacy "Step 4 mapping" wizard step already guarded by `if (false && ...)`) that happened on the v14 line before the Quest work — same dead code existed on main, so removing it here is inert cleanup, not a functional change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the v14 line (projection/*.js is shared, unversioned logic — confirmed byte-identical pre-fix). Project Descriptions is documented as "Append an Archivist section to the most likely description field," but projectDescription() only appended for HTML-flagged slots. 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, silently replacing whatever was there. Matches user reports of losing existing descriptions. Added mergeArchivistPlainSection (merge.js): wraps Archivist content in a recognizable marker, appends it if none exists yet, and replaces only that marked block on future syncs — never touching the rest of the field's content, mirroring what mergeArchivistSection already did for HTML fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the v14 line. Quests were already being auto-imported exactly like Factions/Recaps (_importArchivistMissing() pulls them in unconditionally, no reconcile/selection step), and the count was already computed (count.imp.quests / plan.importFromArchivist.quests) — it just never made it into the summaryRows array the Step 6 table renders from. Added a Quests row identical in shape to the Recaps row. Not porting the matching Step 4 helper-note text update from v14: that note (and the wizard-cleanup rewrite it lives in) doesn't exist in this branch's older world-setup-dialog.hbs, and this branch's Step 6 table is otherwise the same generic summaryRows-driven markup, so the new row renders correctly without it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the v14 line (these files were wholesale-copied from there during the parity backport, confirmed identical pre-fix). Two real bugs, both regressions from the Quests/image-upload work: 1. The upload-local-image button reused .archivist-edit-toggle's exact absolute-position slot with no offset, rendering directly on top of the edit-mode toggle and making it unclickable. Added .archivist-upload-toggle (right: 48px) so they sit side by side. 2. _onRender fired the grid/quest/actor-item population functions without awaiting them. Verified against Foundry's own core source that _render() awaits _onRender via _doEvent — but since ours wasn't async, that await resolved before population finished. Opening a sheet via a card click in another custom sheet could leave cards populated but tab counts never applied. Made _onRender (base class, plus the Location and Quest overrides) async and properly awaited. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…l work Same fixes as the v14 line (feat/api-augments-v14) — these files were already wholesale-copied from there during the parity backport, confirmed identical pre-fix. Not porting his module.json/compatibility changes or the ask-chat-sidebar-tab.js deletion from the same source branch — that reintroduces the exact incident this project started from (v14-only code with a compat.minimum permissive enough for v13 clients, missing the Sidebar.TABS registration this branch still needs). - sync-dialog.js: journal content was being dropped on import/sync (never checked row.content, where Journal's actual content lives); shift+click range-select got flipped back by the row click handler; unescaped journal names in the delete-confirmation dialog; no folder fallback for journals/quests on pre-migration worlds. Wholesale- replaced with the fixed version, re-added this branch's own relatedEntityRefs hydration needed by the richer quest-linking UI. - world-setup-dialog.js (+ its template): added _updateSyncStatusUI() direct-DOM progress updates to the six always-imported-in-full loops, replacing a full render() per entity. - archivist-sync.js: the quest content-edit realtime hook no longer PATCHes the unchanged quest name to the API on every keystroke commit (renames are already handled by a separate title-sync hook). - archivist-api.js: _normalizeQuestPayload rewritten to a strict whitelist matching QuestCreate/QuestUpdate's extra="forbid" schema, and the keepalive guard is now conditional on body size (Chromium rejects keepalive fetches over ~64KB). - merge.js: stripHtml no longer calls the nonexistent foundry.utils.TextEditor.stripHTML. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: camrun01 <cameron.b.llewellyn@gmail.com>
Same as the v14 line — wholesale-copied (confirmed identical pre-change). Adopts camrun91's _localize(key) helper: tries game.i18n.localize() and returns null (not the raw key) on a miss, so statusLabel/categoryLabel can chain `|| fallback` to the existing hardcoded English strings. 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
Here are some automated review suggestions for this pull request.
Reviewed commit: 17779f13e0
ℹ️ 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".
| const data = await this._request(apiKey, `/journals`, { | ||
| method: 'PUT', | ||
| body: JSON.stringify(norm), |
There was a problem hiding this comment.
The API contract added in .cursor/rules/40-archivist-api.mdc says journal updates are PATCH /v1/journals with id in the body, but this wrapper sends PUT /journals. Any Journal sheet save, title sync, or page-content sync that reaches updateJournal() will hit the wrong method and fail to persist remotely; switch this to PATCH.
Useful? React with 👍 / 👎.
| const data = await this._request( | ||
| apiKey, | ||
| `/journals?id=${encodeURIComponent(journalId)}`, | ||
| { method: 'DELETE' } |
There was a problem hiding this comment.
The same API contract specifies DELETE /v1/journals/{journal_id}, but this implementation sends DELETE /journals?id=.... When a synced Journal entry is deleted in Foundry, the local delete will complete while the Archivist journal remains undeleted because the request is routed to the wrong endpoint.
Useful? React with 👍 / 👎.
| res = await archivistApi.createJournal(apiKey, { | ||
| world_id: worldId, | ||
| title: entry.name || 'Journal', |
There was a problem hiding this comment.
Journal creation requires campaign_id per .cursor/rules/40-archivist-api.mdc, but the new realtime Journal path passes world_id and createJournal() only sanitizes fields without translating it. Creating a new Archivist Journal sheet with realtime sync enabled will therefore POST a payload missing the required campaign id and fail validation.
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.
When runFull() creates the new journal rows added here, journalManager.create() delegates to Utils.createArchivistJournal(), whose sheet-class map still only covers pc/npc/item/location/faction/recap. Reconciled Journal entries (and the Quest call just below) are created with an empty core sheet class, so they open in Foundry's default journal sheet instead of the new Archivist Journal/Quest V2 UI.
Useful? React with 👍 / 👎.
| 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 a Quest's giver/status/definitions and clicks Save, this branch sends the remote update but never writes the same payload back into flags.archivist.questData, which _prepareContext() uses for the next render. The sheet immediately re-renders from the old flags, so the saved quest metadata appears to revert locally until some later full import/reconcile refreshes it.
Useful? React with 👍 / 👎.
| const list = root.querySelector('.quest-objectives-list'); | ||
| if (!list) return; | ||
| const objectives = context.quest?.objectives || []; | ||
| list.innerHTML = ''; |
There was a problem hiding this comment.
In edit mode the Handlebars template renders objective inputs, status selects, and remove buttons, but _renderQuestObjectives() clears that list and replaces it with read-only icon/text rows before _wireQuestEditActions() runs. As a result, existing quest objectives cannot be edited or removed from the Quest sheet; skip this DOM replacement when setup.editingInfo is true or render equivalent editable controls.
Useful? React with 👍 / 👎.
| failureConditions: q.failureConditions || '', | ||
| nextAction: q.nextAction || '', | ||
| resolution: q.resolution || '', | ||
| objectives: Array.isArray(q.objectives) ? q.objectives : [], |
There was a problem hiding this comment.
When listQuests() returns summary rows without detail arrays, this reconcile path overwrites flags.archivist.questData.objectives, progress, and related refs with empty arrays; the world-setup and sync import paths already guard this by calling getQuest() when details are missing. Running full reconcile on an existing Quest sheet can therefore erase the locally displayed objectives/links even though the full quest still exists remotely.
Useful? React with 👍 / 👎.
| : type === 'Quest' | ||
| ? arch.questName || arch.quest_name || arch.name || '' |
There was a problem hiding this comment.
Adding Quest to the generic diff path only compares the title/text/image/link buckets, but quests store their real data in structured fields like status, objectives, progress, and relatedEntityRefs. For an already-imported Quest whose remote objectives or status change, the sync dialog will show no diff and _applyDiff() never refreshes flags.archivist.questData, leaving the local Quest sheet stale.
Useful? React with 👍 / 👎.
| ui.notifications.error(message, { permanent: true }); | ||
| } else { | ||
| ui.notifications.info('Sync completed'); | ||
| ui.notifications.info('Sync completed successfully. Click "Complete Setup" to finish.'); |
There was a problem hiding this comment.
This success path now leaves the wizard open and tells the user to click Complete Setup, but _canProceedFromCurrentStep() still disables completion when syncStatus.total === 0. In an empty campaign, or a campaign where only recaps were synced because recaps are not counted in syncStatus.total, Begin Sync can finish successfully while Complete Setup remains disabled, trapping the user in the setup wizard.
Useful? React with 👍 / 👎.
1 participant
Context
Companion to #47. This backports the finished Quests feature, the image-upload/link-PATCH API work, and the bug fixes from that PR onto
main(the legacy v13 line), so both lines stay at feature parity instead ofmainfalling further behindstaging.Turned out
mainand the v14 line already share the same ApplicationV2 sheet framework — v13 doesn't need a FormApplication rewrite here, just the same feature/fix set applied on top. Diffedmainagainst v14's pre-Quest-work state first to scope this precisely rather than guessing what was safe to port; most of it was a clean wholesale copy of already-reviewed v14 files, verified file-by-file that nothing v13-only was getting lost in the process.What's here
Same feature set as #47 (Quests completion, image upload, link PATCH, the #46-derived bug fixes — see that PR's description for the full list), plus a few things specific to backporting cleanly:
compatibilitystays untouched:minimum: "13.341",verified: "13.351". This branch is v13-only, same asmainis today.Sidebar.TABSregistration (ask-chat-sidebar-tab.js+ theinit/setuphook wiring) — v14 had to drop this because that API doesn't support dynamic tab addition there, but it's the mechanism this line actually needs for the chat sidebar tab. Layered the rest of the v14 file's Quest/Journal registrations and hooks on top of it rather than deleting it.module.json'scompatibility.verifiedwas"13.351"whileminimumwas"14.359"on the pre-work v14 line (verified lower than minimum, which is nonsensical) — looks like the kind of v13/v14 field cross-contamination that likely caused the original v14-breaks-v13 incident this whole effort traces back to. Not present onmainitself, just flagging since it's the same failure mode as the compat-field issue called out in Finish Quests feature, add image upload + link PATCH, fold in review fixes from #46 #47.markdownToStoredHtml's no-library fallback, a sidebar tab-ordering fix (insertBeforeinstead ofappendChild, fixed in both places it was duplicated), session-summary now markdown-converted before sync like every other entity type.scripts/modules/utils.js.bak— an unreferenced stray backup file, confirmed zero references anywhere.Testing
npm run lint: 0 errors. All touched files passnode --check. Same caveat as #47 — no live Foundry pass yet, worth testing both PRs side by side if possible since they're meant to behave identically modulo the v13/v14-specific bits called out above.🤖 Generated with Claude Code