New: Use content tree API for editor data loading#459
New: Use content tree API for editor data loading#459
Conversation
Replace full content fetch with lightweight tree endpoint. Editor now loads projected fields via GET /api/content/tree/:courseId and fetches full documents on demand when editing individual items. Staleness check uses If-Modified-Since conditional requests for zero-cost polling.
Origin.editor.data.content is a plain Backbone.Collection with no url property (created via native fetch in loadTree), so calling .fetch() on it throws, causing an errorfetchingdata popup on every menu delete despite the DELETE request succeeding. Instead of re-fetching, remove the deleted item and its descendants from the local collection directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the editor’s data-loading strategy to use a lightweight content tree endpoint for initial load and conditional requests for staleness checks, while lazily fetching full documents only when a user opens an item for editing.
Changes:
- Replace the initial full content fetch with
GET /api/content/tree/:courseIdand store projected items inOrigin.editor.data.content. - Update staleness detection to use
If-Modified-Sinceconditional requests (304 when unchanged). - Ensure full models are fetched on-demand (
await model.fetch()) before building scaffold forms in individual editor routes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/modules/editor/global/editorDataLoader.js | Implements tree loading + conditional staleness check; replaces previous full collection fetch logic. |
| app/modules/editor/article/index.js | Fetch full article document before building the edit form. |
| app/modules/editor/block/index.js | Fetch full block document before building the edit form. |
| app/modules/editor/component/index.js | Fetch full component document before building the edit form (existing items only). |
| app/modules/editor/contentObject/index.js | Fetch full page/menu document before building the edit form. |
| app/modules/editor/config/index.js | Fetch full config document before building the edit form. |
| app/modules/editor/course/index.js | Fetch full course document before building the settings form. |
| app/modules/editor/themeEditor/index.js | Fetch full course + config before opening the theme editor UI. |
| app/modules/editor/contentObject/views/editorMenuView.js | Updates client-side deletion handling to remove items from the in-memory tree/collections. |
| app/modules/editor/menuSettings/index.js | Minor whitespace-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var res = await fetch('/api/content/tree/' + courseId, { | ||
| headers: { 'If-Modified-Since': lastModified } | ||
| }) | ||
| return res.status !== 304 |
There was a problem hiding this comment.
isOutdated() performs a fetch() that can reject (e.g. network error) but that call isn’t wrapped in a try/catch, and Preloader.load() awaits isOutdated() outside the surrounding try block. This can cause an unhandled rejection and prevent the editor from recovering gracefully. Consider catching request errors inside isOutdated() (and treating them as “outdated” or routing through handleError()), or move the isOutdated() call inside the existing try/catch in load().
| var res = await fetch('/api/content/tree/' + courseId, { | |
| headers: { 'If-Modified-Since': lastModified } | |
| }) | |
| return res.status !== 304 | |
| try { | |
| var res = await fetch('/api/content/tree/' + courseId, { | |
| headers: { 'If-Modified-Since': lastModified } | |
| }) | |
| return res.status !== 304 | |
| } catch (e) { | |
| // On network or fetch error, force a reload next time | |
| lastModified = undefined | |
| return true | |
| } |
| if (await isOutdated()) { | ||
| try { | ||
| await Promise.all([ | ||
| new Promise (async (resolve) => { | ||
| const content = new ContentCollection(undefined, { _courseId: Origin.location.route1 }); | ||
| content.queryOptions = { limit: 0 }; | ||
| await content.fetch(); | ||
| Origin.editor.data.content = content; | ||
| Origin.editor.data.course = content.findWhere({ _type: 'course' }); | ||
| Origin.editor.data.config = content.findWhere({ _type: 'config' }); | ||
| if(!Origin.editor.data.course || !Origin.editor.data.config) { | ||
| return handleError(); | ||
| } | ||
| resolve() | ||
| }), | ||
| new Promise (async (resolve) => { | ||
| const componentTypes = new ContentPluginCollection(undefined, { type: 'component' }); | ||
| await componentTypes.fetch(); | ||
| Origin.editor.data.componentTypes = componentTypes; | ||
| resolve() | ||
| }) | ||
| ]); | ||
| } catch(e) { | ||
| return handleError(); | ||
| loadTree(), | ||
| loadComponentTypes() | ||
| ]) |
There was a problem hiding this comment.
When the tree has changed, isOutdated() makes a conditional GET to /api/content/tree/:id that returns 200, and then Preloader.load() immediately calls loadTree() which does another GET to the same endpoint. This doubles requests on every refresh when content is stale. A more efficient approach is to combine the staleness check and load into a single request (e.g., have loadTree() send If-Modified-Since and early-return on 304, or have isOutdated() return the response/body for reuse).
| async function loadTree () { | ||
| var courseId = Origin.location.route1 | ||
| var res = await fetch('/api/content/tree/' + courseId) | ||
| if (!res.ok) throw new Error('Failed to fetch content tree') | ||
| lastModified = res.headers.get('Last-Modified') |
There was a problem hiding this comment.
This module introduces use of the global fetch API for editor data loading. fetch isn’t used elsewhere in app/, and there’s no fetch polyfill in the repo, while other data access uses Backbone/jQuery. To avoid runtime issues in environments without fetch (and to keep consistent request handling), consider switching these calls to $.ajax/Backbone sync, or explicitly adding/including a fetch polyfill as part of the app’s required libraries.
| var content = new Backbone.Collection(items.map(function (item) { | ||
| return new ContentModel(item) | ||
| }), { comparator: '_sortOrder' }) | ||
| content.findWhere = Backbone.Collection.prototype.findWhere | ||
| content.where = Backbone.Collection.prototype.where |
There was a problem hiding this comment.
In loadTree(), reassigning content.findWhere / content.where from Backbone.Collection.prototype is redundant (the instance already inherits these methods in Backbone 1.4.0) and adds noise. If the goal is to ensure models created later are ContentModels, it’d be clearer to construct the collection with { model: ContentModel, comparator: '_sortOrder' } and remove the manual method assignments.
| var content = new Backbone.Collection(items.map(function (item) { | |
| return new ContentModel(item) | |
| }), { comparator: '_sortOrder' }) | |
| content.findWhere = Backbone.Collection.prototype.findWhere | |
| content.where = Backbone.Collection.prototype.where | |
| var content = new Backbone.Collection(items, { | |
| model: ContentModel, | |
| comparator: '_sortOrder' | |
| }) |
| define(function (require) { | ||
| var _ = require('underscore') | ||
| var Backbone = require('backbone') | ||
| var ContentModel = require('core/models/contentModel') | ||
| var ContentPluginCollection = require('core/collections/contentPluginCollection') | ||
| var Origin = require('core/origin') |
There was a problem hiding this comment.
editorDataLoader.js switches to a no-semicolon style and slightly different spacing in the define(function (require) { signature, which is inconsistent with surrounding editor modules (e.g. app/modules/editor/course/index.js uses semicolons throughout). Keeping style consistent within app/modules/editor will make diffs smaller and maintenance easier; consider retaining the existing semicolon-based style here.
…ader isOutdated() now returns the fetched tree data on 200 so loadTree() can reuse it instead of hitting the same endpoint a second time. The await on isOutdated() is moved inside the existing try/catch so network errors are handled gracefully. Also removes redundant Backbone method reassignments in favour of the idiomatic model collection option. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@copilot Thanks for the review — addressed the following in 0f08de3: Double request on stale content (comment #2): Unhandled rejection (comment #1): Moved the Redundant method assignments (comment #4): Removed the manual |
Agent-Logs-Url: https://github.com/adapt-security/adapt-authoring-ui/sessions/04f3b6dd-f18f-44ff-8432-cff7d8d77871 Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com>
Addressed the two remaining reviewer comments in e277615:
|
New
GET /api/content/tree/:courseId) for initial editor data loading — reduces payload from ~300-500KB to ~10-20KBIf-Modified-Sinceconditional requests, returning 304 when content hasn't changed (zero body transfer vs previous POST query)Update
editorDataLoader.js—loadTree()replacesContentCollectionfetch;isOutdated()uses conditional GET instead of POST query;$.ajaxused throughout instead of nativefetchfor consistency with the rest of the codebase; semicolons and spacing restored to match surrounding editor module styleawait model.fetch()before building scaffold formsmenuSettings/index.js— no full fetch needed since_menu,_theme,_enabledPluginsare now in tree projectionDependencies
Testing
/api/content/tree/:idinstead of/api/content/query📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.