Fix/remixer punchlist - #948
Conversation
jakeaturner
left a comment
There was a problem hiding this comment.
Critical
- Deferring deletes past the conflict loop turns a common reorder into a hard job failure that leaves the library half-mutated
server/api/services/remixer-service.ts:1780
The delete phase runs after the retry loop and after the Unable to resolve title/URL conflicts throw. Any rename or move that targets a path currently occupied by a page in deleteOrdered can no longer succeed, because the occupant is not removed until the throw has already fired.
Concrete failure: user deletes section 1.1 Alpha and renames 1.2 Beta to Alpha. Beta's target path is 01:_Alpha, still held by the undeleted Alpha.
- Phase 1: Beta gets a 409, throwForMindTouchResponse raises TitleConflictError (line 157), Beta lands on deferredStack.
- Retry loop (line 1701): Beta is the only stuck entry, so stillStuck.length === beforeCount on every pass and the deadlock branch fires each time.
- relocatable resolves to Beta (real numeric id), so temporarilyRelocatePage moves a live library page to remixer-swap-tmp-- with allow=deleteredirects.
- base62(8) gives a fresh name each pass, so the move keeps succeeding. With maxRetryPasses = pages.length + 10, a 200 page book issues 210 real MindTouch moves of the same page.
- The loop exhausts and throws. Alpha is still live, Beta is parked at a throwaway title, and the job is failed.
Before this change Alpha preceded Beta in BFS order and was deleted first, so the rename went through.
The cascade problem the comment describes is real, but the fix needs deletes to run before conflict resolution can give up. Simplest correct ordering is the straight-line pass, then deletes, then the deferred retry loop:
let deferredStack: OrderedEntry[] = [];
for (const entry of nonDeleteOrdered) {
const outcome = await processOrderedEntry(entry);
if (outcome === "conflict") deferredStack.push(entry);
}
// Survivors have been reparented out of any doomed subtree by now, so a
// recursive delete can no longer take them with it.
for (const entry of deleteOrdered) {
await processOrderedEntry(entry);
}
const maxRetryPasses = pages.length + 10;
// ...existing retry loop
That preserves the cascade guarantee (all moves complete before any delete) and frees the deleted paths before the retry loop decides it is deadlocked.
Two smaller things in the same block: processOrderedEntry's return value is discarded at line 1783, and handleDeletedPage failures are swallowed by the bare catch (error) {} at line 1628 (pre-existing). Combined, a delete that fails for a non-transient reason is reported to the user as processed and dropped from finalBook, so the saved state claims a page is gone while it is still live in the library. At minimum push a message on that path.
Major
- canAccessPage now bypasses the TOC cache on every authorization check
server/api/services/book-service.ts:231
getBookPageIDs(false) here forces a full recursive GET_Page_Tree fetch against MindTouch on every call. canAccessPage is a per-request authz gate on eight call sites well outside the remixer:
- server/api/co-author.ts:34, 77, 116, 198, 313
- server/api/books.ts:2315, 2355
- server/api/restacker.ts:628
The 120 second cache (TOC_CACHE_TTL_SECONDS, book-service.ts:69) existed to keep these off the wire. Every co-author AI summary request now pays a full book tree fetch. That is added latency on each request plus upstream rate limit exposure, and none of it is needed for the remixer correctness this PR is chasing.
The two remixer call sites that genuinely need freshness are validateRemixerBookOwnership (remixer.ts:109) and runRemixerJob (remixer-service.ts:1388), both user-initiated and infrequent. Leave canAccessPage on the cache and scope the bypass to those two. If publish staleness is the underlying worry, invalidate the cache entry after a successful job rather than disabling reads globally.
- preserveConfigs copies change-detection baselines, not just user settings
server/api/services/remixer-service.ts:1937
REMIXER_PAGE_CONFIG_KEYS includes originalPathNumber, originalFormattedPath, and originalFormattedPathOverride. Those are not configuration. They are the baseline the client diffs against to decide what changed.
The overlay defeats reseeding in normalizeBookState (RemixerDashboard.tsx:372):
const seedFormattedOriginals =
!page.addedItem && page.originalFormattedPathOverride === undefined;
Because the merge makes originalFormattedPathOverride defined, this is false, so the formatted-path baseline is never reseeded from the freshly loaded library tree. After "Fresh from Library" the user has current structure measured against a stale draft's baseline. hasFormattedPathChanged then reports overridden pages as changed when they are not, or misses a real change, and the next publish issues spurious renames or skips required ones.
originalPathNumber happens to survive because the fresh path passes initializeOriginalPathNumber: true (line 383), but it is the same category mistake and will bite the moment a caller omits that flag.
Drop all three:
const REMIXER_PAGE_CONFIG_KEYS = [
"pathNumber",
"numberedPath",
"formattedPath",
"formattedPathOverride",
"siblingTitleIndex",
"formattedPathPrefix",
"formattedPathIndex",
] as const;
- loadSelectedBook overlays project configs onto an arbitrary browsed book
client/src/components/remixer/RemixerDashboard.tsx:596
const treeRes = await api.getRemixerTreeFlattened(id, targetNodeId, lib);
No options, so the server defaults to preserveConfigs: true (validators/remixer.ts:30). targetNodeId is whatever book the user picked from the catalog, which is usually not the project's book. The handler still merges this project's saved remixerCurrentBook onto that tree by page id.
When the user browses their own project book from the catalog, ids do match and the library pane renders with the draft's path numbers instead of the library's. Pass { flatten: true, preserveConfigs: false } here. This call is a library browse, not a book load.
Minor:
- options is a breaking body shape on /remixer/:id/page/tree
server/api/validators/remixer.ts:30
Zod strips unknown keys by default, so an old caller sending top-level flatten: false now silently gets flatten: true with no error. All three in-repo callers were updated, so this is only a risk for anything outside this repo hitting the endpoint. Worth a note in the PR body.
Also, preserveConfigs is silently a no-op when flatten: false, since the merge is gated on Array.isArray(tree) (remixer.ts:657) and the unflattened shape is a nested object. Fine as a fail-safe, but it deserves a comment so a future caller does not assume it works.
- Query convention drift
server/api/remixer.ts:443
{ projectID: id, status: "success" } drops the { $eq: ... } wrapper the rest of the file uses consistently (see line 488, and the pending-job check at line ~190). req.params.id is always a string in Express so there is no injection here, but matching the file's defensive style keeps the pattern greppable.
There was a problem hiding this comment.
Many users, including Delmar, experienced the same error, which was caused by caching. We met yesterday, and he was understandably frustrated with the issue.
I noticed that caching was the likely cause because the error would resolve itself after some time. I’ve been looking into it to identify the root cause and prevent it from happening again.
There was a problem hiding this comment.
Totally understood 👍 I don't think it will be too much of a real performance concern until we can find a more cohesive solution
…d configuration preservation Enhances Remixer book loading to preserve configs and show publish date Users can now choose to preserve autonumbering and path format settings when loading a book "Fresh from Library" from the recovery modal. This functionality is implemented by passing a `preserveConfigs` option to the API, allowing the server to merge a live Table of Contents with previously saved configuration details from the project's Remix. Additionally, the UI now displays the last successful publication date for the book in the recovery modal, providing more context for available book sources. Includes a fix to correctly detect changes in formatted paths.
…mponent Updated the CatalogList component to improve modal body styling by adding a class for visible overflow. Adjusted the maximum height of the table to enhance layout consistency and prevent double scrollbars. Updated pagination class names for better responsiveness.
…reeNodeContainer components Refactored the link rendering logic in both the Dashboard and TreeNodeContainer components to improve readability and maintainability. The new implementation consolidates conditions for displaying titles and links, ensuring consistent styling and behavior based on the component's state. This change enhances the user experience by providing clearer interactions with item links.
Updated the PublishPanel component to improve the layout and styling of the modal body and accordion items. Added classes for visible overflow and adjusted text sizes for better readability. These changes aim to enhance the user experience by providing a more visually appealing and functional interface.
…te Dashboard component Introduced the isBookRootChild prop to the TreeNodeContainer component, allowing it to indicate if a node is a direct child of the book cover. Updated the Dashboard component to utilize this new prop for conditional rendering of folder icons, enhancing the visual representation of the book tree structure.
…nd accessibility Modified the LibraryActions component to enhance the layout by centering elements and adjusting class names for better styling. Added a label for the library select input to improve accessibility and updated button size and class for consistent appearance. These changes aim to provide a more user-friendly interface.
…caching for page ownership checkings Modified the getBookPageIDs method and its related functions in the BookService to accept a caching parameter, allowing for more flexible data retrieval. Updated calls to getBookPageIDs in the remixer API to disable caching where necessary. Additionally, adjusted the ControlPanel component's styling for improved layout and accessibility.
…hboard and TreeNodeContainer components Refactored the RemixerDashboard and TreeNodeContainer components to enhance code readability by standardizing formatting and indentation. Removed unnecessary elements and adjusted the layout for better clarity. These changes aim to improve maintainability and streamline the development process.
…er usability Replaced icons in the ControlPanel component to enhance visual clarity and updated button classes for improved layout consistency. Adjusted flex properties to ensure proper alignment and spacing of elements, contributing to a more user-friendly interface.
…n and icon updates Updated the RecoveryModal component to include logic for determining the most recent source among available options. Introduced new icons from the Tabler library for improved visual consistency and updated the layout to display recent source badges. These changes aim to enhance user experience by providing clearer information on available recovery options.
…ta handling Refactored the loadSelectedBook function in the RemixerDashboard component to enhance data loading and ancestry tree building. Introduced error handling and streamlined the logic for fetching selected book details and their descendants, ensuring a more efficient and reliable data retrieval process. These changes aim to improve the overall performance and user experience of the remixer dashboard.
… runRemixerJob Enhanced the runRemixerJob function to process non-delete entries first, followed by a second phase for delete entries. This change prevents conflicts during processing by ensuring that deletions occur only after all other operations are completed, improving the reliability of the remixer job execution.
…nly nodes Refactored the applyBookNodeDeletion function to differentiate between draft-only nodes and published nodes during deletion. Introduced logic to completely remove draft-only nodes instead of soft-deleting them, ensuring they do not appear in the publish delete set. This change enhances the accuracy of node management in the remixer process.
…proved layout Updated the selectClassName in the ControlPanel component to include right padding, enhancing the visual layout and ensuring better alignment of elements. This change aims to improve the overall user experience by refining the component's appearance.
…nd path segment overrides Introduced a CreatePageOptions type to allow for title and path segment overrides in the handleNewPage and handleImportedPage functions. Updated the logic to utilize these options, improving flexibility in page creation. Added a new function to find deleted path occupants, enhancing conflict resolution during page operations. This refactor aims to streamline page management and improve user experience in the remixer process.
…s for config preservation Modified the remixer tree fetching logic in the RemixerDashboard component to include options for flattening the tree and preserving existing remixer configurations. This change enhances the flexibility of data retrieval, ensuring that the tree structure is tailored to library browsing without overlaying saved configurations, thereby improving the user experience.
…d Remixer Added new methods in BookService for fetching, downloading, and uploading page files, enhancing file management capabilities. Updated Remixer service to support copying files between pages, including error handling for file migrations. Adjusted API endpoints for file operations and improved template handling for migrated files. These changes aim to streamline file interactions within the remixer process, improving overall functionality.
…ime checks Modified the validateRemixerBookOwnership function to include a new parameter for real-time book checks, enhancing the flexibility of ownership validation. Adjusted the logic for fetching owned page IDs based on this parameter, ensuring accurate permission verification. This change aims to improve the reliability of remixer project state management and ownership validation processes.
…ed fields Removed several unused fields from the REMIXER_PAGE_CONFIG_KEYS array in the remixer-service. This change streamlines the configuration, improving code clarity and maintainability by eliminating unnecessary entries.
…l check for formattedPathOverride Expanded the REMIXER_PAGE_CONFIG_KEYS array to include additional fields for improved configuration handling. Introduced a conditional check in the pickSavedPageConfigs function to return an empty object if formattedPathOverride is not true, enhancing the logic for managing saved page states. This change aims to improve clarity and maintainability in the remixer service.
…iguration Removed several unused fields from the REMIXER_PAGE_CONFIG_KEYS array and added the formattedPath field back for improved clarity. This change enhances the maintainability of the remixer service by ensuring only relevant configuration keys are retained.
Added functionality to highlight the currently opened catalog node in the library tree until another catalog book is selected. Introduced a new state for managing the highlighted node and updated the TreeNodeContainer to apply the highlight style conditionally. This enhancement improves user experience by providing visual feedback on the selected catalog book.
Modified the PublishPanel component to disable the save button when the publish status is "success". Enhanced the openPublishModal function in RemixerDashboard to reset the publish status and messages when opening the modal after a successful or error state. This change improves user experience by ensuring the button reflects the correct state and resets the modal appropriately.
Implemented a restore feature in the RemixerDashboard, allowing users to recover deleted book nodes. Updated the BookActions component to include a restore button when an item is marked as deleted. Enhanced the context menu to provide a restore option, improving user experience by enabling easy recovery of items. The applyBookNodeRestore function was also added to manage the restoration process in the services module.
… management Added support for managing URL-ending overrides in the EditPanel component, including a new prop for matter pages. Updated the RemixerDashboard to handle original URI endings and detect changes in overrides, improving the logic for page state management. This enhancement streamlines the user experience by allowing for more precise control over page URLs and their formatting.
…econciliation Updated the handleImportedPage function to exclude source page's article tags that do not reflect the target placement. Implemented a reconciliation pass to ensure that every page's article type aligns with its final position, improving the accuracy of article kind assignments. This change enhances the integrity of page metadata during the import process and ensures consistency across the remixer service.
…ode handling and notifications Updated the EditPanel to better manage URI ending overrides during editing and viewing modes. Enhanced the RemixerDashboard to prevent restoring nodes under deleted ancestors, adding user notifications for clarity. This improves the user experience by ensuring proper handling of node states and providing informative feedback during interactions.
…board draft management Updated the EditPanel to track user interaction with the URL-ending field, ensuring that only explicitly modified values are saved. Enhanced the RemixerDashboard to clear stale local drafts upon successful publishing, preventing unintended reversion to outdated states. These changes improve user experience by providing clearer control over URL management and draft persistence.
… and deletion logic Refined the EditPanel to ensure accurate management of URL-ending overrides, allowing for clearer user interactions. Updated the Remixer model to include a new property for tracking deletion via ancestor nodes, enhancing the restoration logic for deleted items. These changes improve the overall user experience by providing better control over URL management and node states.
… formatting Added a new utility function, joinPrefixAndIndex, to streamline the formatting of paths by joining a prefix and index with appropriate spacing. Updated the EditPanel and RemixerDashboard components to utilize this function, enhancing the clarity and consistency of formatted paths throughout the application. This change improves user experience by ensuring proper path representation in the UI.
applyArticleKindToPage received the page before its id was adopted from the create response, so parseInt reduced the local <sourceID>-<ts>-<rand> id to the source page. Every import rewrote tags, and sometimes content, on a page outside the book, past the ownership check that deliberately skips an imported node's own id. It now takes the created page id explicitly. hasFormattedPathChanged compared originalPathNumber against pathNumber, restating the position check arePathNumbersEqual already does for movedItem. Editing an override's text left renamedItem false, so the rename never reached the library. It now compares the override text on both sides. handleDeletedPage treated 404 as a failure. Deletes are recursive and descendants are queued as their own entries, so every multi-level delete reported pages as still live. 404 and 410 now return cleanly. Also carries originalFormattedPath through preserveConfigs so a preserved override keeps its baseline, instead of flagging every page as renamed on a fresh-from-library load.
241110f to
7b7aed9
Compare
Address six defects found reviewing #948. - Stop storing percent-encoded URL endings. EditPanel seeded the override field from the encoded uri.ui leaf, so ticking "Override URL Ending" and saving with no edit republished the page at 01%253A_Introduction. Decode on seed and swap % for : and () in both sanitizer alphabets, matching what buildRemixerPagePathSegment emits. The server decodes before cleaning, so values already saved in the encoded form heal on next save. Dots-only endings (. and ..) are now rejected on both sides. - Make the placeholder rename non-fatal, like the delete phase above it. A 409 there threw out of runRemixerJob and failed the job with the page still published under remixer-replace-tmp-*. - Add ParentNotReadyError so the deadlock breaker skips pages deferred for a missing parent. Their own title is not the contested slot, so relocating one freed nothing and moved a live library page once per retry pass. - Seed originalOverrideUriUiEnding to "" rather than undefined. The undefined value left the guard true forever, defeating the early return and reallocating every node on every normalizeBookState call. - Reset undo/redo and panel state after a successful publish. This path replaced a page reload, so Undo restored the pre-publish tree, autosaved it as a draft, and republished it on the next save. - Re-snapshot descendants after a placeholder rename. Their finalBook entries were captured at processing time and kept the throwaway path, which the client then rendered as broken page links.
…e copies The article-kind reconciliation pass wrote page tags and, via activateShowOrg, page bodies. It iterated every finalBook entry, but the job's ownership gate only inspects pages whose status is new/imported/modified/deleted. An entry with no change flags reads as `unchanged`, is never checked against ownedIDs, and still reaches finalBook. Since currentBook is client-supplied, a project member could name any page id in the library and have the pass overwrite it. Guard the loop on ownedIDs plus a new createdPageIDs set tracking adoptions, and refuse non-numeric ids so an unadopted import id cannot parseInt down to its source page in the target library. Persist deletedViaAncestor so node restore survives a save/reload. The client sets it, but it was missing from RemixerSubPageState and its Mongoose schema, so strict mode dropped it and every cascade-deleted child read as an independent delete on the next load. Restoring a chapter left its pages flagged, and publish deleted them again. Treat an absent flag as a cascade in applyBookNodeRestore, which is the only thing older drafts could have been. Pass silentFail on the three file helpers. CXOneFetch throws on any non-2xx unless told otherwise, so their `.ok` guards were unreachable and a single bad attachment aborted file copying for the whole page. Surface per-file degradations through job messages instead of leaving them in the server log. Cap bulk attachment copying at 50 MiB per file. getFileBytes buffered an unbounded ArrayBuffer and putPageFile copied it again, so one large media attachment could exhaust the 4 GiB task. Check the listing's declared size, then Content-Length before reading the body, then real byteLength for chunked responses. Use Buffer.from(arrayBuffer) rather than a Uint8Array copy to halve peak memory per file.
Address six defects found reviewing #948. - Stop storing percent-encoded URL endings. EditPanel seeded the override field from the encoded uri.ui leaf, so ticking "Override URL Ending" and saving with no edit republished the page at 01%253A_Introduction. Decode on seed and swap % for : and () in both sanitizer alphabets, matching what buildRemixerPagePathSegment emits. The server decodes before cleaning, so values already saved in the encoded form heal on next save. Dots-only endings (. and ..) are now rejected on both sides. - Make the placeholder rename non-fatal, like the delete phase above it. A 409 there threw out of runRemixerJob and failed the job with the page still published under remixer-replace-tmp-*. - Add ParentNotReadyError so the deadlock breaker skips pages deferred for a missing parent. Their own title is not the contested slot, so relocating one freed nothing and moved a live library page once per retry pass. - Seed originalOverrideUriUiEnding to "" rather than undefined. The undefined value left the guard true forever, defeating the early return and reallocating every node on every normalizeBookState call. - Reset undo/redo and panel state after a successful publish. This path replaced a page reload, so Undo restored the pre-publish tree, autosaved it as a draft, and republished it on the next save. - Re-snapshot descendants after a placeholder rename. Their finalBook entries were captured at processing time and kept the throwaway path, which the client then rendered as broken page links.
|
🎉 This PR is included in version 2.151.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.