Added support for migrating notes from Simple Calendar#20
Conversation
📝 WalkthroughWalkthroughAdds a legacy module name constant and updates calendar import logic to detect and convert notes stored under the legacy module flag to the new module flag, adjust sheet class and calendarId, and handle create/update decisions for converted entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/classes/applications/configuration-app.ts:
- Around line 1858-1869: When converting legacy entries, avoid a null-pointer
and remove the redundant reassignment: ensure journalEntryData.flags["core"]
exists before setting its sheetClass (create journalEntryData.flags["core"] =
journalEntryData.flags["core"] || {} ), and remove the unnecessary else branch
that does journalEntryData.flags[ModuleName].noteData = noteData since noteData
was just read from that path; retain the logic that copies flags from
LegacyModuleName into ModuleName and then clears LegacyModuleName, and still
update journalEntryData.flags[ModuleName].noteData.calendarId = calendarId.
- Around line 1870-1876: The current create-vs-update check incorrectly treats
isConverted as a reason to create a new JournalEntry, which can attempt to
create a document with an existing ID; remove isConverted from the create
condition so that you only call JournalEntry.create when
noteImportedIntoDifferentCalendar is true or
game.journal.has(journalEntryData._id) is false, and otherwise call
game.journal.get(journalEntryData._id)?.update(journalEntryData); keep the
existing keepId logic (keepId: !noteImportedIntoDifferentCalendar) as-is since
conversion of flags is already applied to journalEntryData before this decision.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/classes/applications/configuration-app.tssrc/constants.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/classes/applications/configuration-app.ts (3)
src/classes/notes/note-stub.ts (1)
noteData(79-85)src/constants.ts (2)
ModuleName(5-5)LegacyModuleName(10-10)src/classes/index.ts (1)
NManager(13-13)
🔇 Additional comments (2)
src/constants.ts (1)
6-10: LGTM! Clean backward compatibility addition.The
LegacyModuleNameconstant is well-documented and appropriately placed alongsideModuleName. This will enable the import logic to recognize notes from the original Simple Calendar module.src/classes/applications/configuration-app.ts (1)
12-12: LGTM! Import added for backward compatibility.The
LegacyModuleNameimport is correctly added to support the legacy module flag handling in the import logic below.
5bbbe9a to
515af60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/classes/applications/configuration-app.ts:
- Around line 1864-1868: The code sets journalEntryData.flags["core"].sheetClass
without ensuring journalEntryData.flags["core"] exists, which can throw; update
the block inside the isConverted branch (where ModuleName and LegacyModuleName
are used) to ensure journalEntryData.flags and journalEntryData.flags["core"]
are initialized (e.g., create an empty object if missing) before assigning
flags[ModuleName], setting core.sheetClass to ModuleName + ".NoteSheet", and
clearing flags[LegacyModuleName].
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/classes/applications/configuration-app.tssrc/constants.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Arctis-Fireblight
Repo: Fireblight-Studios/foundryvtt-simple-calendar PR: 20
File: src/classes/applications/configuration-app.ts:1870-1876
Timestamp: 2026-01-08T08:30:38.842Z
Learning: In the SimpleCalendar Reborn fork, when importing journal entries from the legacy "foundryvtt-simple-calendar" module that exist in the current world, the maintainer prefers to create new journal entries with new IDs rather than updating in-place, to avoid potential issues with "dirty" legacy configuration data.
📚 Learning: 2026-01-08T08:30:38.842Z
Learnt from: Arctis-Fireblight
Repo: Fireblight-Studios/foundryvtt-simple-calendar PR: 20
File: src/classes/applications/configuration-app.ts:1870-1876
Timestamp: 2026-01-08T08:30:38.842Z
Learning: In the Fireblight-Studios/foundryvtt-simple-calendar fork, when importing journal entries from the legacy 'foundryvtt-simple-calendar' module that already exist in the world, prefer creating new journal entries with new IDs instead of updating existing ones. This avoids potential issues with lingering or 'dirty' legacy configuration data. Apply this approach specifically in the configuration/import logic (e.g., in src/classes/applications/configuration-app.ts) and ensure that existing entries are left untouched unless a mechanism explicitly clones and re-links references.
Applied to files:
src/constants.tssrc/classes/applications/configuration-app.ts
🧬 Code graph analysis (1)
src/classes/applications/configuration-app.ts (3)
src/classes/notes/note-stub.ts (1)
noteData(79-85)src/constants.ts (2)
ModuleName(5-5)LegacyModuleName(10-10)src/classes/index.ts (1)
NManager(13-13)
🔇 Additional comments (5)
src/constants.ts (1)
6-10: LGTM! Clean backwards compatibility constant.The addition of
LegacyModuleNameis well-documented and follows the existing code patterns. This constant appropriately supports migration from the original "foundryvtt-simple-calendar" module.src/classes/applications/configuration-app.ts (4)
12-12: LGTM! Import added for legacy module support.The import of
LegacyModuleNameis necessary for the note migration logic implemented below.
1858-1863: LGTM! Solid legacy format detection logic.The code correctly:
- Retrieves noteData from either new or legacy module flags using optional chaining
- Guards against missing noteData
- Detects legacy entries before any modifications occur
- Identifies when legacy entries exist in the current world for proper handling
Based on learnings, the approach of detecting and handling legacy entries separately is appropriate.
1872-1882: LGTM! Create/update logic correctly implements migration strategy.The branching logic properly handles three scenarios:
- Legacy entries that exist: Creates new entries with new IDs to avoid dirty configuration data (aligns with learnings)
- Notes imported to different calendars or missing entries: Creates entries, using
keepIdappropriately based on whether the calendar changed- Existing non-legacy entries: Updates in place
The check for
noteImportedIntoDifferentCalendarat line 1876 correctly uses the value computed at line 1860 before the calendarId was modified, ensuring accurate detection.Based on learnings, this approach of creating new entries for legacy imports is the preferred strategy.
1870-1870: The code is already properly defensive about thenoteDatastructure. At line 1858, the code explicitly attempts to retrievenoteDatafrom the expected location (flags[LegacyModuleName]?.noteData), and line 1859 guards the entire block to ensurenoteDataexists before proceeding. When the conversion happens at line 1865, the entire legacy flags object is copied, which includes the verifiednoteData. No risk exists at line 1870.Likely an incorrect or invalid review comment.
Fixes #14
Fixes #17
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.