fix(publish): sync restricted access list from server so "Change" can't silently wipe it - #491
Open
StpMax wants to merge 3 commits into
Open
fix(publish): sync restricted access list from server so "Change" can't silently wipe it #491StpMax wants to merge 3 commits into
StpMax wants to merge 3 commits into
Conversation
# Conflicts: # src/renderer/cowork/components/artifact/publish/PublishMenu.jsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Editing an already-shared artifact's restricted access list via "Change" read the email list from local component state that was only ever populated by the immediate response of a same-session
publish()call. For an already-published artifact opened any other way — a page reload, a new session/device, or a chat-bubble preview whose artifact object is synthesized from stream steps — the list silently reset to empty, even though the server still had the real list stored.If the user then clicked "Change" → made any tweak → "Update", the empty/stale local list was submitted and overwrote the real server-side access list, revoking everyone who actually had access. This is a data-loss bug, not a display bug.
Fixes ENG-931. Unblocks ENG-930 (restricted-access summary needs this data to exist client-side first).
Root cause
usePublishseededaccessEmails/orgAllowedpurely from theartifactprop and never fetched them from the server for an existing artifact.refresh()called/artifacts/statusbut consumed onlymodified/publishedUrl/artifactKey— it dropped theaccessMode/accessEmails/orgAllowedfields the endpoint already returns.publish()response — good for that one session, gone the moment the viewer closed and reopened.Note: the server side already returns the access list (and deliberately strips the plaintext password) via
/artifacts/status— it has since June, well before this ticket. So the fix is client-only: consume what the server already sends.How
All changes are in
usePublish.jsandPublishMenu.jsx; no server or API-signature changes.refresh()now consumes the access fields (accessMode/accessEmails/orgAllowed) from/artifacts/status, and is called on viewer open for every artifact type — so a synthesized (chat-bubble) or stale prop is corrected from the server before it can be edited./artifacts/statusis used rather than thepreview-mountpayload because it is type-agnostic (covers.md, which bypasses mount) and omits the plaintext password.onChangefromrefresh()now carries the access fields back into the artifact prop. This is load-bearing: the broad re-sync effect re-seedsaccessEmailsfrom the prop wheneverpublishedUrl/accessModechange, so dropping the fields there would immediately self-clobber the list we just loaded.accessLoadedflag (seeded from whether the prop carries authoritative access —accessMode/accessProtectedfrom a listing — via a sharedhasServerAccess()helper). While the real prior list has not loaded, "Update" in the Change-access view is disabled ("Loading current access…"), with a matching guard indoApplyAccess, so an unconfirmed list can never be submitted. The flag is reset per-artifact-identity (keyed ontargetPath), not in the broad re-sync effect, soonChange-driven prop churn can't leave it stuck.draftDirty).Behavior by entry point
Tests
usePublish.test.jsx(new):refresh()consumes access even whenmodified/publishedUrlare unchanged; open-refresh fills the list for a prop lacking it;accessLoadedseed (incl. legacyaccessProtected-only) and reset; self-clobber roundtrip (onChange result fed back into the prop must not wipe the loaded list); same-sessionpublish(restricted)still shows the new list immediately.PublishMenu.test.jsx(extended): enabled "Update" for a loaded restricted artifact (grid/rail); "Update" blocked while unloaded (chat-bubble guard); draft re-seeds when the real list arrives after "Change" was opened.Verification
npm test→ 510/510 passingnpm run typecheck→ cleannpm run check:cowork-purity→ cleanNotes / out of scope
/artifacts/statusreturnsnull(transient network error, or an old server without the endpoint),accessLoadedstaysfalsefor a synthesized artifact and "Update" remains disabled rather than risking an overwrite. On a supported server this recovers on the next window focus; a dedicated failed/retry state was intentionally not added.staging(resolved one conflict:FooterButtonadopts staging's sharedButtoncomponent while keeping thetitleprop used by the loading guard).Type of change
Please delete options that are not relevant.
Checklist: