feat(example): expose project source priority + cross-project compare in demo - #414
Conversation
… in demo Uploading a spec through the Project Spec Map could silently produce a project copy of DIFFERENT content than the uploaded file: the demo hardcoded the company master as the onboarding target, joined by section number only, and several flows rewrote the project's source chain clients-first — so a client master holding the same section shadowed the upload with no visible signal (#413). The backend was never at fault: POST /projects/:id/specs resolves by project_sources priority and reports source + shadowed; the demo discarded both. - Project Settings: ordered source-library editor (reorder / remove / add, priority = list order) saved via PUT /projects/:id/sources. - Spec Map upload: pick the destination source library (priority-1 default) instead of a hardcoded company master. - Join provenance: surface which library won resolution and warn when the targeted library was shadowed (new pure module source-order.mjs, tested). - syncProjectSourcesToTocScope now merges order-preservingly — scope changes append or drop sources, never reprioritize. - Compare pickers now include other projects' TOC copies labeled by project name, so two projects' versions of one section compare directly. Closes #413 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR reworks the web UI demo's project source-library management from unordered checkboxes to an explicit ordered priority chain, wires it into settings, upload, and TOC-scope flows, and replaces hard-delete spec removal with withdraw/restore semantics plus guarded conflict-retry logic, alongside cross-project compare catalog refresh, supporting CSS/HTML updates, and new unit tests. ChangesWeb UI demo: ordered source priority and safe spec removal
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as Settings UI (app.js)
participant SourceOrder as source-order.mjs
participant API as api.js
User->>SettingsUI: reorder/remove source rows
SettingsUI->>SettingsUI: setSourceDraftOrder(sourceDraft)
User->>SettingsUI: click Save Settings
SettingsUI->>SourceOrder: mergeSourcesWithScope(currentSources, scope, companyId)
SourceOrder-->>SettingsUI: merged ordered list
SettingsUI->>API: setProjectSources(projectId, order)
API-->>SettingsUI: updated project.sources
sequenceDiagram
participant Admin
participant AppJS as app.js
participant API as api.js
participant SpecRemoval as spec-removal.mjs
Admin->>AppJS: click Withdraw on library spec
AppJS->>API: withdrawSpec(specId)
API-->>AppJS: tombstoned
AppJS-->>Admin: toastWithAction "Undo"
Admin->>AppJS: click Undo
AppJS->>API: restoreSpec(specId)
API-->>AppJS: restored
Admin->>AppJS: remove project copy
AppJS->>API: removeSpecFromProject(projectId, specId)
API-->>AppJS: 409 conflict
AppJS->>SpecRemoval: classifyRemovalConflict(err)
SpecRemoval-->>AppJS: 'force-retry'
AppJS->>API: removeSpecFromProject(projectId, specId, {force:true})
API-->>AppJS: removed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… re-parse Give the demo the full deletion story for fixtures, using only landed API surface (#413): - Library masters (client + company): a Withdraw button soft-deletes via DELETE /specs/:id (ADR-030 tombstone) with an Undo toast wired to POST /specs/:id/restore. The stale deleteSpec helper ("hard-deletes") is renamed to withdrawSpec to match what the server actually does, and the never-enabled specDelete flag becomes specWithdraw: true. - Admin full reset: a Re-parse button on each master explains, then reuses the library-targeted picker — POST /libraries/:id/import upserts onto (section, source, library), hard-deleting every parsed paragraph and reference server-side and rebuilding them with a fresh inference pass (content_version bumps, id and clone lineage survive). - Project copies: removal now walks the admin force path — a 409 is classified (new pure module spec-removal.mjs, tested) so an EDITED copy offers "Force delete (admin)" (?force=true) while a package-pinned copy explains itself and never offers force. - removeTargetSpecs drops its dead specDelete purge block: the REST contract has no hard spec delete by design. Verified live: withdraw→undo round-trip, re-parse content_version 2→3 with 330 paragraphs rebuilt, force-remove of an edited copy logged force:true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/web_ui_demo/js/app.js (2)
1501-1552: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame partial-failure pattern in
withdrawSpecFromLibrary.
withdrawSpecand the subsequent UI refreshes (refreshSelectedLibrarySpecs,refreshTocLibrarySpecs,refreshCoordination, etc.) share one try/catch. IfwithdrawSpecsucceeds but a later refresh call throws, the catch reports`withdraw failed: ${err.message}`even though the withdraw itself succeeded server-side — an admin could then retry the withdraw and hit a confusing conflict on an already-withdrawn spec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/web_ui_demo/js/app.js` around lines 1501 - 1552, `withdrawSpecFromLibrary` is treating post-withdraw UI refresh failures as withdraw failures, which can misreport a successful server-side withdraw as an error. Split the `withdrawSpec(spec.specId)` call from the subsequent refresh/render steps in `app.js`, and keep the undo/toast flow tied only to the actual withdraw result. Use the `withdrawSpecFromLibrary` function and its `refreshSelectedLibrarySpecs`, `refreshTocLibrarySpecs`, `refreshCoordination`, and `refreshOpenComments` calls to update the UI, while ensuring only the real withdraw call is wrapped by the withdraw-specific error handling.
471-498: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPartial-failure messaging in
saveProjectSettings.
patchProjectandsaveSourceDraftare wrapped in one try/catch. IfpatchProjectsucceeds butsaveSourceDraft(or any later refresh) throws, the user only sees`settings save failed: ${err.message}`even though the name/format change was already persisted — masking a partial success as a total failure.💡 Proposed fix: isolate the source-draft save so failures are reported distinctly
try { await patchProject(activeProjectId, { name, sectionNumberFormat }); - await saveSourceDraft(); + try { + await saveSourceDraft(); + } catch (err) { + toast(`settings saved, but source order failed: ${err.message}`, 'warn'); + } await refreshProjectList(activeProjectId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/web_ui_demo/js/app.js` around lines 471 - 498, The `saveProjectSettings` flow currently treats `patchProject`, `saveSourceDraft`, and all refresh calls as one failure path, so a later exception can make a successful `patchProject` look like a total save failure. Split the source-draft save and the subsequent refresh/render work into separate handling inside `saveProjectSettings`, so the user gets a distinct message when `saveSourceDraft` (or refreshes like `refreshProjectList`/`refreshTocClientScope`) fail after the project update has already succeeded. Use the existing `patchProject`, `saveSourceDraft`, and `toast` logic to preserve the persisted name/section format change while reporting post-save failures accurately.
🧹 Nitpick comments (1)
examples/web_ui_demo/js/spec-removal.mjs (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMessage-string classification is fragile but intentionally fail-closed.
classifyRemovalConflictmatches on substrings of the server's error message ('force=true', 'package'). If backend wording changes, this silently degrades to'other'rather than breaking loudly — which per the file's own comment is the intended fail-safe behavior (never force-retry blindly). Consider whether the backend could instead return a structured error code/reason field, which would be less brittle than string matching, though this would require a backend contract change outside this demo-only PR's scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/web_ui_demo/js/spec-removal.mjs` around lines 10 - 16, `classifyRemovalConflict` in `spec-removal.mjs` uses fragile substring matching on `err.message`, but the review indicates this is intentionally fail-closed, so keep the current fallback-to-'other' behavior unchanged. If you do need to improve it, prefer switching the `409` handling to a structured backend-provided reason/code field in `classifyRemovalConflict` rather than expanding string checks, but that contract change is outside this demo-only PR’s scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/web_ui_demo/js/app.js`:
- Around line 2155-2178: The refreshCrossProjectSpecs workflow currently lets
one failed getProject(project.id) call wipe out the entire cross-project catalog
because Promise.all rejects and the catch resets crossProjectSpecs to an empty
array. Update refreshCrossProjectSpecs to tolerate per-project failures by
handling each project independently, using allSettled (or equivalent per-item
error handling) so successful results still populate crossProjectSpecs. Keep the
existing getProject, crossProjectSpecs, and refreshCrossProjectSpecs symbols,
and make sure the catch no longer discards already loaded entries.
---
Outside diff comments:
In `@examples/web_ui_demo/js/app.js`:
- Around line 1501-1552: `withdrawSpecFromLibrary` is treating post-withdraw UI
refresh failures as withdraw failures, which can misreport a successful
server-side withdraw as an error. Split the `withdrawSpec(spec.specId)` call
from the subsequent refresh/render steps in `app.js`, and keep the undo/toast
flow tied only to the actual withdraw result. Use the `withdrawSpecFromLibrary`
function and its `refreshSelectedLibrarySpecs`, `refreshTocLibrarySpecs`,
`refreshCoordination`, and `refreshOpenComments` calls to update the UI, while
ensuring only the real withdraw call is wrapped by the withdraw-specific error
handling.
- Around line 471-498: The `saveProjectSettings` flow currently treats
`patchProject`, `saveSourceDraft`, and all refresh calls as one failure path, so
a later exception can make a successful `patchProject` look like a total save
failure. Split the source-draft save and the subsequent refresh/render work into
separate handling inside `saveProjectSettings`, so the user gets a distinct
message when `saveSourceDraft` (or refreshes like
`refreshProjectList`/`refreshTocClientScope`) fail after the project update has
already succeeded. Use the existing `patchProject`, `saveSourceDraft`, and
`toast` logic to preserve the persisted name/section format change while
reporting post-save failures accurately.
---
Nitpick comments:
In `@examples/web_ui_demo/js/spec-removal.mjs`:
- Around line 10-16: `classifyRemovalConflict` in `spec-removal.mjs` uses
fragile substring matching on `err.message`, but the review indicates this is
intentionally fail-closed, so keep the current fallback-to-'other' behavior
unchanged. If you do need to improve it, prefer switching the `409` handling to
a structured backend-provided reason/code field in `classifyRemovalConflict`
rather than expanding string checks, but that contract change is outside this
demo-only PR’s scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf25c762-9299-4afe-b5e3-09431986a341
📒 Files selected for processing (9)
examples/web_ui_demo/css/app.cssexamples/web_ui_demo/index.htmlexamples/web_ui_demo/js/api.jsexamples/web_ui_demo/js/app.jsexamples/web_ui_demo/js/features.jsexamples/web_ui_demo/js/source-order.mjsexamples/web_ui_demo/js/spec-removal.mjsexamples/web_ui_demo/source-order.test.mjsexamples/web_ui_demo/spec-removal.test.mjs
…lure reporting Review findings on the #413 demo source-editor / deletion-tier work (CodeRabbit + Codex second-reviewer pass): - refreshCrossProjectSpecs: Promise.allSettled so one project's fetch failure no longer discards the whole cross-project compare catalog. - loadActiveProjectWorkspace: clear the Settings source-draft cached before project details load, so a stale empty draft can't hide the real chain or overwrite it on save. - refreshTocLibrarySpecs: only surface the company master when it is still in the saved source chain, so the TOC never offers specs the project cannot resolve after a source is removed. - withdrawSpecFromLibrary / saveProjectSettings: isolate the persisting call from later view refreshes so a refresh failure is not misreported as a failed operation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CodeRabbit body / outside-diff findings — dispositionsFixed in 936638b:
Declined:
|
Codex (GPT-5.5, xhigh) second-reviewer pass — additional eyesCodeRabbit reviewed this PR normally; Codex ran as a second adversarial reviewer. It surfaced two
Codex reported no other issues. Full transcript verdict retained locally. |
1 participant
Why
Uploading a DOCX through the demo's Project Spec Map could silently yield a project copy whose content is not the uploaded file. Live repro: two different versions of section 08 11 13 uploaded for projects A100 and B100 produced identical project copies, so the Compare view showed no differences. Diagnosis in #413: the backend is working as designed (priority-ordered
project_sourcesresolution, ADR-015, withsource/shadowedreported back); the demo hid the chain, hardcoded the company master as upload target, rewrote priorities clients-first from unrelated flows, and discarded the resolution provenance. The demo also had no working way to delete fixtures once loaded.What
Demo-only (
examples/web_ui_demo/) — nosrc/, no API, noopenapi.yamlchanges:Source priority made explicit
PUT /projects/:id/sources.js/source-order.mjs, pure + unit-tested).Fixture deletion tiers
DELETE /specs/:id(ADR-030 soft tombstone) with an Undo toast →POST /specs/:id/restore. The staledeleteSpechelper is renamedwithdrawSpecto match real server semantics; the never-enabledspecDeleteflag becomesspecWithdraw: true.content_versionbumps, id + clone lineage survive).js/spec-removal.mjs, pure + unit-tested) so an edited copy offers "Force delete (admin)" (?force=true) while a package-pinned copy explains itself and never offers force.Testing
node --testinexamples/web_ui_demo/: 90 pass, 0 fail (15 new acrosssource-order.test.mjs+spec-removal.test.mjs)src/change)content_version2→3 rebuilding 330 paragraphs; force-remove of an edited copy loggedforce: true🤖 Co-authored by Claude Fable 5. Closes #413.
Summary by CodeRabbit
New Features
Bug Fixes