Skip to content

Breaking: Improve performance#108

Open
taylortom wants to merge 35 commits intomasterfrom
feature/content-tree
Open

Breaking: Improve performance#108
taylortom wants to merge 35 commits intomasterfrom
feature/content-tree

Conversation

@taylortom
Copy link
Copy Markdown
Contributor

@taylortom taylortom commented Mar 10, 2026

Breaking

  • Removed getDescendants standalone utility function (replaced by ContentTree.getDescendants + config handling inlined in ContentModule.delete)

New

  • ContentTree utility class providing O(1) lookup indexes (byId, byParent, byType) and efficient tree operations (getDescendants, getAncestors, getSiblings, getComponentNames) over flat content arrays
  • Dedicated GET /api/content/tree/:_courseId endpoint returning lightweight projected content with _children IDs and updatedAt timestamps
  • Tree endpoint supports If-Modified-Since conditional requests — returns 304 with no body when the course hasn't changed, using a single projected findOne for the staleness check
  • generateFriendlyIds for bulk counter allocation in a single atomic increment
  • Utility functions extracted to lib/utils/: computeSortOrderOps, contentTypeToSchemaName, formatFriendlyId, parseMaxSeq
  • Automatic friendly ID generation added

Update

  • Rewrite clone as a flat bulk insert: pre-generates all _id values via createObjectId and friendly IDs upfront, builds payloads with mapped parent references, fires preInsertHook/postInsertHook manually, then does a single insertMany instead of recursive per-item inserts (2 DB round-trips instead of 2N)
  • Replace convertObjectIds in clone() with targeted _id/_courseId/_parentId assignment using new ObjectId() directly — ObjectId instances are pre-computed in idMap, avoiding a full-document field scan and the App.instance side effect it triggers in test environments
  • Pre-allocate friendly IDs in bulk per type during clone (1 counter query per type instead of per item)
  • Remove generateFriendlyId — single-ID callers now use generateFriendlyIds with count 1
  • Clone pre-fetches the source course once and uses ContentTree for child lookups, eliminating N+1 DB queries per tree level
  • Clone disables updateSortOrder and updateEnabledPlugins during recursion, running updateEnabledPlugins once at top level after all children exist
  • Clone error cleanup uses deleteMany instead of individual deletes
  • Guard updateSortOrder and updateEnabledPlugins to skip irrelevant content types
  • Added MongoDB indexes for _parentId and _type queries
  • Added projections to all find call sites to reduce document transfer
  • Delete uses super.find with projection for tree building, avoiding hooks and full-document fetch
  • Delete passes a ContentTree of remaining items to updateEnabledPlugins instead of a raw array
  • updateEnabledPlugins accepts a pre-built ContentTree via options.tree, uses tree.config and tree.getComponentNames() for O(1) lookups instead of array scans
  • updateEnabledPlugins uses Set-based equality checks and filters instead of O(n²) .includes() loops
  • updateEnabledPlugins filters out falsy values before constructing the enabled plugins Set, preventing undefined/null _menu/_theme entries
  • insertRecursive post-loop call to updateEnabledPlugins now passes { forceUpdate: true } to ensure config _enabledPlugins is not skipped by the _type guard
  • updateSortOrder batches writes into a single bulkWrite instead of individual super.update calls
  • insertRecursive defers updateSortOrder and updateEnabledPlugins to run once after the loop instead of per-item
  • Standardised this.* vs super.* convention — super.* for internal bookkeeping, this.* for user-facing operations
  • Schema caching at ContentModule level (keyed by schemaName + enabled plugin schemas, cleared on schema re-registration)
  • Replaced find()[0] patterns with findOne using throwOnMissing
  • Cached module references (contentplugin, jsonschema, authored, tags) in init()
  • Bump adapt-authoring-mongodb to ^3.1.0 for preserveId support

Fix

  • Removed silent error swallowing in getSchema — errors now propagate instead of silently falling back to less-specific schemas
  • Eliminated double-fetch in getSchema/getSchemaName_courseId is now fetched alongside _type/_component in a single query
  • Clone now rolls back on failure — tracks created items and cleans up if an error occurs mid-clone
  • Clone validates _id in handleClone before proceeding
  • Delete uses deleteMany for descendants (1 bulk query) instead of N individual deletes
  • adapt-authoring-mongodb moved from peer to standard dependency

Testing

  1. Run npm test — 122 unit tests pass (ContentModule + ContentTree + utils)
  2. Run npx standard — clean lint
  3. Clone a course and verify all content is correctly duplicated
  4. Delete a page/article and verify descendants are removed
  5. Delete a course and verify config is included in deletion
  6. Test tree endpoint: GET /api/content/tree/:courseId returns projected items with _children
  7. Test conditional request: repeat with If-Modified-Since header, expect 304
  8. Verify friendly ID uniqueness under concurrent clones
  9. Companion PR: adapt-authoring-authored#44 (course timestamp source of truth)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@taylortom taylortom changed the title New: Add ContentTree abstraction (refs #80) Code audit updates Mar 10, 2026
…refs #79)

- Fix INVALID_PARENT error to return 400 instead of 500
- Fix clone() error message providing type: undefined
- Fix delete() missing default options parameter
- Fix insertRecursive not checking for missing parent
- Fix updateSortOrder splice logic for undefined _sortOrder
- Cache contentplugin, jsonschema, authored, tags module refs in init()
- Replace quadratic reduce with spread with flatMap in getSchema
- Use destructuring instead of payload mutation in clone()
- Remove unused UNKNOWN_SCHEMA_NAME error definition
- Simplify unit tests to pure logic only, move orchestration tests
  to adapt-authoring-integration-tests
…efs #79)

Only call updateSortOrder when _sortOrder or _parentId changes, and
updateEnabledPlugins when _component, _menu, _theme, or _enabledPlugins
changes. Accept optional contentItems parameter in updateEnabledPlugins
to skip redundant full-course fetch; used in delete() which already has
a tree.
Reduces data transfer by projecting only needed fields:
- getSchemaName: _type, _component
- getSchema: _courseId and _enabledPlugins
- updateSortOrder: _id, _sortOrder
- updateEnabledPlugins: 6 fields instead of full docs
- contentplugin.find: name only
- clone parent lookup: _id, _type, _courseId
Use super.find in updateSortOrder and updateEnabledPlugins to bypass
hooks for internal bookkeeping queries. Add class-level JSDoc
documenting the convention: this.* for user-facing operations (triggers
hooks), super.* for internal bookkeeping (avoids infinite recursion).
Replace find()[0] and [doc] = find() patterns with findOne() where a
single result is expected. Use throwOnMissing: false where the caller
handles the missing case, and default throwOnMissing: true where
not-found should throw. Also replace deprecated strict option with
throwOnMissing.
Replace N individual super.delete() calls for descendants with a single
super.deleteMany() bulk operation. The target doc is still deleted via
super.delete() to trigger the deleteHook middleware (required by
multilang for peer cascade deletion). Pre/post delete hooks still fire
per item via deleteMany for courseassets cleanup and authored timestamps.

For a 200-item course delete, this reduces ~400 DB round-trips to ~4.
- Rollback: track created items during clone and clean up on failure,
  matching the pattern in insertRecursive
- Input validation: validate _id is present in handleClone before
  proceeding
- Performance: parallelise sibling clones with Promise.all instead of
  sequential loop

Remaining items from #111 (require cross-module changes):
- Batch updateCourseTimestamp during clone (authored module)
- Fragile course double-write pattern (mitigated by validate: false)
- Remove silent error swallowing in getSchema — errors from
  getSchemaName and config lookup now propagate, preventing silent
  fallback to base schema which could strip plugin-specific fields
- Eliminate double-fetch: getSchemaName now also fetches _courseId when
  looking up a document, attaching it to data so getSchema skips the
  redundant query
- Add schema cache keyed by schemaName + sorted enabledPluginSchemas,
  avoiding expensive schema build/compile on repeated calls with the
  same plugin set. Cache is cleared on schema re-registration.
@taylortom taylortom changed the title Code audit updates Breaking: Improve performance Mar 11, 2026
taylortom and others added 12 commits March 11, 2026 16:24
Replaces the non-atomic read-max-then-increment approach with an atomic
counter using findOneAndUpdate/$inc, fixing duplicate _friendlyId errors
during parallel clone inserts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Content now imports parseObjectId directly, so it's a real dependency.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace queue.shift() with index pointer in BFS, cache toString()
in getSiblings, and avoid intermediate array in getComponentNames.
Replace raw contentItems array with ContentTree for O(1) type lookups,
use Set-based equality checks, add defensive nullish coalescing on
config._enabledPlugins, simplify reduce chains with flatMap and Set
loop, add optional chaining on schema lookup, document type guard
relationship with callers, and batch updateSortOrder writes into a
single bulkWrite.
…e (refs #109)

Use super.find with projection in delete for tree building, defer
updateSortOrder and updateEnabledPlugins to end of insertRecursive,
remove redundant updateEnabledPlugins from top-level clone insert,
and use deleteMany for clone error cleanup.
Removed unnecessary comments and simplified tree initialization.
- Extract formatFriendlyId, parseMaxSeq, contentTypeToSchemaName, computeSortOrderOps to lib/utils/
- Skip updateEnabledPlugins in clone for same-course copies
- Add unit tests for all extracted utilities (30 tests)
Reserves a range of sequence numbers in a single atomic counter increment,
enabling batch ID allocation for clone and insertRecursive operations.
Walk the source tree upfront to count items by type, then reserve
all friendly IDs in one atomic counter increment per type. Eliminates
per-item counter round-trips during recursive clone operations.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new ContentTree abstraction and a lightweight /tree/:_courseId API to improve content traversal performance, while also refactoring content cloning/deletion to reduce DB round-trips and adding bulk-friendly ID generation plus _assetIds tracking.

Changes:

  • Added ContentTree with O(1) indexes and tree operations, and exposed it publicly.
  • Added GET /api/content/tree/:_courseId with projections + If-Modified-Since conditional 304 support.
  • Refactored ContentModule for performance (bulk clone, batched sort order updates, projections, counters, asset reference tracking) and extracted several utilities into lib/utils/.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
lib/ContentModule.js Major refactor: schema caching, bulk friendly IDs/counters, _assetIds tracking, optimized delete, bulk clone, new tree handler
lib/ContentTree.js New tree/index abstraction over flat content arrays
lib/utils.js Re-exports new utilities and ContentTree
lib/utils/computeSortOrderOps.js New helper to compute bulkWrite ops for _sortOrder recalculation
lib/utils/contentTypeToSchemaName.js Centralized _type → schema name mapping
lib/utils/formatFriendlyId.js New friendly ID formatter utility
lib/utils/parseMaxSeq.js New helper to parse max sequence from existing friendly IDs
lib/utils/extractAssetIds.js New helper to derive _assetIds via schema walking
routes.json Adds /tree/:_courseId route + OpenAPI-like metadata
schema/contentassets.schema.json Adds schema extension for _assetIds
migrations/3.0.0.js Backfills _friendlyId and _assetIds on existing content docs
errors/errors.json Adjusts INVALID_PARENT; adds RESOURCE_IN_USE
package.json Adds adapt-authoring-mongodb dependency; updates peers
index.js Exports ContentTree from package entry
tests/* Adds unit tests for new utilities and ContentTree; refactors ContentModule tests; removes getDescendants tests

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

const topItem = newItems[0]
await Promise.all([
this.updateSortOrder(topItem, topItem),
this.updateEnabledPlugins(topItem)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In insertRecursive(), the post-loop side effects run against topItem (often course for new courses, or article/page/menu for non-course roots). With the new guard in updateEnabledPlugins() (skips non-component/config unless forceUpdate), this call will typically no-op, leaving config _enabledPlugins stale even though a component/config was created. Consider invoking updateEnabledPlugins with { forceUpdate: true } and/or passing a doc whose _type is config/component (or removing the _type guard for this internal call).

Suggested change
this.updateEnabledPlugins(topItem)
this.updateEnabledPlugins(topItem, { forceUpdate: true })

Copilot uses AI. Check for mistakes.
Comment on lines +528 to +533
const nextPlugins = new Set([
...[...currentPlugins].filter(name => extensionNames.includes(name)), // only extensions, rest are calculated below
...componentNames,
config._menu,
config._theme
]))
])
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextPlugins is constructed with config._menu and config._theme unconditionally. If either is undefined/null, it will be included in the Set and written back into config._enabledPlugins, which can introduce invalid entries. Consider filtering out falsy values before adding menu/theme (and similarly ensure tree.getComponentNames() doesn’t contribute undefineds).

Copilot uses AI. Check for mistakes.
delete payload._id
delete payload._courseId
await this.update({ _id: newData._id }, payload, { validate: false })

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clone builds payloads directly and inserts via insertMany, bypassing insert() where _assetIds are computed/recomputed. If _assetIds is missing on the source docs (or if customData changes any asset-referencing fields), the cloned docs may end up with incorrect _assetIds, which can break the new asset deletion protection. Consider recomputing _assetIds for each payload (or at least the root payload when applying customData) before insert.

Suggested change
// Recompute _assetIds for each cloned payload to ensure asset-deletion protection remains accurate
for (const payload of payloads) {
payload._assetIds = extractAssetIds(payload)
}

Copilot uses AI. Check for mistakes.
Comment on lines +569 to +592
async handleTree (req, res, next) {
try {
const _courseId = req.apiData.query._courseId
const course = await this.findOne(
{ _type: 'course', _courseId },
{ validate: false },
{ projection: { updatedAt: 1 } }
)
const lastModified = new Date(course.updatedAt)
const ifModifiedSince = req.headers['if-modified-since'] && new Date(req.headers['if-modified-since'])
if (ifModifiedSince && lastModified <= ifModifiedSince) {
return res.status(304).end()
}
const items = await this.find(
{ _courseId },
{ validate: false },
{ projection: { _id: 1, _parentId: 1, _courseId: 1, _type: 1, _sortOrder: 1, title: 1, displayTitle: 1, _component: 1, _layout: 1, _menu: 1, _theme: 1, _enabledPlugins: 1, updatedAt: 1 } }
)
const tree = new ContentTree(items)
res.set('Last-Modified', lastModified.toUTCString())
res.json(items.map(item => ({
...item,
_children: tree.getChildren(item._id).map(c => c._id)
})))
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new /tree/:_courseId endpoint handler isn’t covered by unit tests (including If-Modified-Since → 304 behavior, projection correctness, and _children construction). Given this is a new public API surface, consider adding unit tests to lock down response shape and conditional request semantics.

Copilot uses AI. Check for mistakes.
Comment on lines +63 to +67
const courseIds = [...new Set(usedBy.map(d => d._courseId?.toString()).filter(Boolean))]
const courses = (await this.find(
{ _type: 'course', _id: { $in: courseIds } },
{ validate: false },
{ projection: { title: 1, displayTitle: 1 } }
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asset pre-delete check builds courseIds as strings (toString()), then queries courses via { _id: { $in: courseIds } }. If _id is stored as ObjectId (which this module often assumes via parseObjectId()), this query will return no courses and the error payload will be missing course titles. Consider converting courseIds to ObjectIds (e.g. via parseObjectId/convertObjectIds) before querying.

Copilot uses AI. Check for mistakes.
Comment on lines +436 to +440
let friendlyId
if (isCourse) friendlyId = item._friendlyId
else if (isConfig) friendlyId = formatFriendlyId('config')
else friendlyId = friendlyIds.get(item._type)?.shift()

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Array.prototype.shift() to consume preallocated friendly IDs makes payload generation O(n²) per type (shift is linear). For large clones this can become a noticeable CPU cost. Consider tracking an index per type (or popping from the end) instead of shifting from the front.

Copilot uses AI. Check for mistakes.
Comment on lines +64 to +71
it('inserts at beginning when _sortOrder is 1', () => {
const siblings = [
{ _id: 'a', _sortOrder: 1 },
{ _id: 'b', _sortOrder: 2 }
]
const item = { _id: 'new', _sortOrder: 0 }
const ops = computeSortOrderOps(siblings, item)
// _sortOrder - 1 = -1, which is not > -1, so appends to end
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test name doesn’t match the scenario/assertion: it says "inserts at beginning when _sortOrder is 1" but the item uses _sortOrder: 0 and the comment/assertion expects an append-to-end behavior. Consider renaming the test (or adjusting the input) so the intent matches the behavior being validated.

Suggested change
it('inserts at beginning when _sortOrder is 1', () => {
const siblings = [
{ _id: 'a', _sortOrder: 1 },
{ _id: 'b', _sortOrder: 2 }
]
const item = { _id: 'new', _sortOrder: 0 }
const ops = computeSortOrderOps(siblings, item)
// _sortOrder - 1 = -1, which is not > -1, so appends to end
it('appends item to end when _sortOrder is 0', () => {
const siblings = [
{ _id: 'a', _sortOrder: 1 },
{ _id: 'b', _sortOrder: 2 }
]
const item = { _id: 'new', _sortOrder: 0 }
const ops = computeSortOrderOps(siblings, item)
// _sortOrder 0 is treated as append-to-end: result [a, b, new] with new having _sortOrder 3

Copilot uses AI. Check for mistakes.
Comment on lines 368 to +468
async clone (userId, _id, _parentId, customData = {}, options = {}) {
const [originalDoc] = await this.find({ _id })
let { tree, parent } = options

const originalDoc = tree
? tree.getById(_id)
: await this.findOne({ _id })
if (!originalDoc) {
throw this.app.errors.NOT_FOUND
.setData({ type: originalDoc?._type, id: _id })
.setData({ type: 'content', id: _id })
}
if (options.invokePreHook !== false) await this.preCloneHook.invoke(originalDoc)

const [parent] = _parentId ? await this.find({ _id: _parentId }) : []

if (!parent && _parentId) {
parent = await this.findOne({ _id: _parentId }, { throwOnMissing: false }, { projection: { _id: 1, _type: 1, _courseId: 1 } })
}
if (!parent && originalDoc._type !== 'course' && originalDoc._type !== 'config') {
throw this.app.errors.INVALID_PARENT.setData({ parentId: _parentId })
}
const schemaName = originalDoc._type === 'menu' || originalDoc._type === 'page' ? 'contentobject' : originalDoc._type
const payload = stringifyValues({
...originalDoc,
_id: undefined,
_trackingId: undefined,
_friendlyId: originalDoc._type !== 'course' ? undefined : originalDoc._friendlyId,
_courseId: parent?._type === 'course' ? parent?._id : parent?._courseId,
_parentId,
createdBy: userId,
...customData
if (!tree) {
const sourceItems = await this.mongodb.find(this.collectionName, { _courseId: originalDoc._courseId })
tree = new ContentTree(sourceItems)
}

// Collect all items to clone: root, config (if course clone), then all descendants
const allItems = [originalDoc]
if (originalDoc._type === 'course' && tree.config) {
allItems.push(tree.config)
}
allItems.push(...tree.getDescendants(_id))

if (options.invokePreHook !== false) {
for (const item of allItems) await this.preCloneHook.invoke(item)
}

// Pre-generate ObjectIds for every item (old _id → new _id)
const idMap = new Map()
for (const item of allItems) {
idMap.set(item._id.toString(), createObjectId())
}

const newCourseId = originalDoc._type === 'course'
? idMap.get(originalDoc._id.toString()).toString()
: (parent?._type === 'course' ? parent._id.toString() : parent._courseId.toString())

// Pre-allocate friendly IDs in bulk per type
const typeCounts = new Map()
for (const item of allItems) {
if (item._type === 'course' || item._type === 'config') continue
typeCounts.set(item._type, (typeCounts.get(item._type) ?? 0) + 1)
}
const friendlyIds = new Map()
await Promise.all([...typeCounts].map(async ([_type, count]) => {
const ids = await this.generateFriendlyIds(_type, newCourseId, count)
friendlyIds.set(_type, ids)
}))

// Build all insert payloads with pre-mapped IDs and parent references
const rootId = _id.toString()
const payloads = allItems.map(item => {
const oldId = item._id.toString()
const newId = idMap.get(oldId)
const isCourse = item._type === 'course'
const isConfig = item._type === 'config'

let newParentId
if (oldId === rootId) newParentId = _parentId
else if (isConfig) newParentId = undefined
else newParentId = idMap.get(item._parentId?.toString())?.toString()

let friendlyId
if (isCourse) friendlyId = item._friendlyId
else if (isConfig) friendlyId = formatFriendlyId('config')
else friendlyId = friendlyIds.get(item._type)?.shift()

return stringifyValues({
...item,
_id: newId,
_trackingId: undefined,
_friendlyId: friendlyId,
_courseId: isCourse ? newId.toString() : newCourseId,
_parentId: newParentId,
createdBy: userId,
...(oldId === rootId ? customData : {})
})
})
const newData = await this.insert(payload, { schemaName, validate: false })

if (originalDoc._type === 'course') {
const [config] = await this.find({ _type: 'config', _courseId: originalDoc._courseId })
if (config) {
await this.clone(userId, config._id, undefined, { _courseId: newData._id.toString() })
delete payload._id
delete payload._courseId
await this.update({ _id: newData._id }, payload, { validate: false })

// Fire preInsertHook on each payload (allows observer modules to set timestamps etc.)
await Promise.all(payloads.map(payload =>
this.preInsertHook.invoke(payload, { schemaName: contentTypeToSchemaName(payload._type), collectionName: this.collectionName }, {})
))

// Convert string IDs to ObjectId instances and bulk insert in a single round-trip
const allNewIds = allItems.map(item => idMap.get(item._id.toString()))
for (const payload of payloads) convertObjectIds(payload)

const collection = this.mongodb.getCollection(this.collectionName)
try {
await collection.insertMany(payloads, { ordered: false })
} catch (e) {
await collection.deleteMany({ _id: { $in: allNewIds } }).catch(() => {})
throw e
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clone() has been substantially rewritten (bulk payload build + insertMany + rollback). There are currently no unit tests covering this new code path (including counter allocation, parent remapping, and rollback on insert failure), so regressions here would be hard to catch. Consider adding focused tests for clone’s core invariants and error cleanup behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +9 to +12
export default function formatFriendlyId (_type, count, idInterval, _language) {
if (_type === 'course') return `course-${count}${_language ? `-${_language}` : ''}`
if (_type === 'config') return 'config'
return `${_type[0]}-${count * idInterval}`
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatFriendlyId() derives the prefix from _type[0], which will format menu IDs as m-{n}. Elsewhere in this PR (e.g. migration logic) menu IDs are treated like pages (p-{n}), so this will generate a different friendlyId format for menus and could break expectations/consistency. Consider using an explicit type→prefix mapping (page+menu→'p', article→'a', block→'b', component→'c') and add a test case for menu.

Suggested change
export default function formatFriendlyId (_type, count, idInterval, _language) {
if (_type === 'course') return `course-${count}${_language ? `-${_language}` : ''}`
if (_type === 'config') return 'config'
return `${_type[0]}-${count * idInterval}`
const TYPE_PREFIXES = {
page: 'p',
menu: 'p',
article: 'a',
block: 'b',
component: 'c'
}
export default function formatFriendlyId (_type, count, idInterval, _language) {
if (_type === 'course') return `course-${count}${_language ? `-${_language}` : ''}`
if (_type === 'config') return 'config'
const prefix = TYPE_PREFIXES[_type] || _type[0]
return `${prefix}-${count * idInterval}`

Copilot uses AI. Check for mistakes.
taylortom and others added 2 commits March 27, 2026 13:19
Remove ID interval (was 5, now 1:1 mapping). Migration now imports
shared formatFriendlyId and parseMaxSeq utilities instead of duplicating
logic. Replace console.log with log function passed from migrations runner.
Fix incorrect menu prefix (was 'p', now correctly uses _type[0]).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Pass forceUpdate to updateEnabledPlugins in insertRecursive to avoid no-op
- Filter falsy values from nextPlugins Set to prevent undefined menu/theme entries
- Fix misleading test name in computeSortOrderOps
- Add unit tests for clone and handleTree

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@taylortom
Copy link
Copy Markdown
Contributor Author

@copilot Thanks for the review. Here's a summary of how each comment was addressed:

Actioned (40e2751):

  1. insertRecursive updateEnabledPlugins no-op — Fixed. Now passes { forceUpdate: true } so the call isn't silently skipped by the _type guard.
  2. Falsy _menu/_theme in nextPlugins — Fixed. Added .filter(Boolean) to strip undefined/null values before constructing the Set.
  3. Misleading test name — Fixed. Renamed to "appends to end when _sortOrder is 0".
  4. Missing tests for clone and handleTree — Added 6 tests for clone (NOT_FOUND, INVALID_PARENT, page clone with descendants, course clone with config, parent ID remapping, rollback on failure, hook invocation) and 3 tests for handleTree (304 conditional response, items with _children, error forwarding).

Dismissed:

  1. String courseIds in asset pre-delete — Not an issue. this.find calls convertObjectIds(query) internally which recursively converts valid ObjectId strings in $in arrays to ObjectId instances before querying.
  2. Array.shift() O(n²) — Skipped. The arrays are per-type within a single course (pages, articles, blocks, components). Even large courses have <1000 components, so the overhead is negligible.
  3. Clone bypasses _assetIds computation — Skipped. Clone copies source documents which already have correct _assetIds. The customData applied to the root is limited to metadata (title, parent) and doesn't affect asset-referencing fields.
  4. Menu prefix m- vs p- — Left as-is. Technically the prefix should be co (for contentobject) but neither m- nor p- is more correct here, and changing it now would add unnecessary churn.

…nment to avoid App.instance side effect in tests

Agent-Logs-Url: https://github.com/adapt-security/adapt-authoring-content/sessions/9952e7df-96fe-4af3-8034-bbbebd385aaa

Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI commented Mar 27, 2026

@copilot Thanks for the review. Here's a summary of how each comment was addressed:

Actioned (40e2751):...

Fixed in cbb24c9. The CI failures had two root causes:

  1. Test expectations out of sync with implementation (20 individual failures): updated ContentModule.spec.js to use generateFriendlyIds (renamed from generateFriendlyId), updated findMaxSeq assertions to expect raw max values (no idInterval division), updated utils-formatFriendlyId.spec.js to use the new 3-param signature, and updated utils-parseMaxSeq.spec.js to use the new 1-param signature.

  2. File-wrapper test failure (1 failure, all individual tests passed): the clone tests call ContentModule.prototype.clone() which internally called convertObjectIds(payload). This function calls isValidObjectId() on every string field — for non-ObjectId strings (like title, _type), it calls parseObjectId() which accesses App.instance, creating the singleton and triggering async App initialization. That initialization fails in the test environment and sets process.exitCode = 1, causing the subprocess to exit 1 even with fail: 0. Fixed by replacing convertObjectIds(payload) in clone() with targeted assignment of only the known ID fields (_id, _courseId, _parentId) using new ObjectId() directly — the ObjectId instances were already pre-computed in idMap.

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.

3 participants