Skip to content

feat(visual-builder): show fields disabled when another session changes or deletes their content type (DRFT-927) #665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SahilCs15 merged 2 commits into from
Oct 7, 2026
Merged

feat(visual-builder): show fields disabled when another session changes or deletes their content type (DRFT-927) #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a52201e
feat(visual-builder): show fields disabled when another session chang…
SahilCs15 Oct 7, 2026
d7a2295
refactor(visual-builder): derive restriction allowlist from one list …
SahilCs15 Oct 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions src/visualBuilder/utils/__test__/entryEditRestriction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
waitForEntryLockInfo,
} from "../fieldLockIndicator";
import {
ENTRY_EDIT_RESTRICTIONS,
clearAllEntryFieldLockInfo,
getEntryEditRestriction,
setEntryEditRestriction,
Expand Down Expand Up @@ -130,6 +131,16 @@ describe("restriction updates", () => {
expect(getEntryEditRestriction(scope)).toBeNull();
});

it("accepts content type changes from the parent", () => {
useEntryEditRestrictionUpdateEvent();
const handler = mockPostMessage.on.mock.calls[0][1] as any;

handler({ data: { ...scope, restriction: "contentTypeDeleted" } });
expect(getEntryEditRestriction(scope)).toBe("contentTypeDeleted");
handler({ data: { ...scope, restriction: "contentTypeUpdated" } });
expect(getEntryEditRestriction(scope)).toBe("contentTypeUpdated");
});

it("ignores a restriction value it does not know", () => {
useEntryEditRestrictionUpdateEvent();
const handler = mockPostMessage.on.mock.calls[0][1] as any;
Expand Down Expand Up @@ -246,3 +257,37 @@ describe("snapshot seeding", () => {
).resolves.toBeUndefined();
});
});

describe("content type restrictions from the editor", () => {
it.each(ENTRY_EDIT_RESTRICTIONS)(

@hitesh-shetty-cstk hitesh-shetty-cstk Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: the cases now come from the list this test is checking, so the test no longer pins that list's membership on its own. Drop a value from ENTRY_EDIT_RESTRICTIONS and this block runs one case fewer and stays green, where the hardcoded array it replaced would have failed.

Most of that gap is covered elsewhere. ENTRY_RESTRICTION_MESSAGES is typed Record<EntryEditRestriction, string>, so a removal that leaves the message behind is an excess-property error at compile time. Only removing the value and its message together slips past both, and that is a deliberate edit rather than a slip.

I would take this as it stands. The duplication it removes is worth more than the oracle it gives up. Flagging it so the tradeoff is on the record rather than implied.

Generated by Claude Code

@SahilCs15 SahilCs15 Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, keeping it as is.

"accepts %s and has a field message for it",
(restriction) => {
useEntryEditRestrictionUpdateEvent();
const handler = mockPostMessage.on.mock.calls[0][1] as any;

handler({ data: { ...scope, restriction } });

expect(getEntryEditRestriction(scope)).toBe(restriction);
expect(getEntryEditRestrictionForField(meta())).toBe(
ENTRY_RESTRICTION_MESSAGES[restriction]
);
expect(ENTRY_RESTRICTION_MESSAGES[restriction]).toEqual(
expect.any(String)
);
}
);

it("seeds a content type reason from the lock snapshot and shows it on variant fields", async () => {
mockPostMessage.send.mockResolvedValueOnce({
fieldLockInfo: {},
editRestrictions: { "entry1:en-us": "contentTypeDeleted" },
});

await getEntryLockInfo({ ...scope, contentTypeUid: "shoe" });

expect(getEntryEditRestrictionForField(meta({ variant: "v1" }))).toBe(
ENTRY_RESTRICTION_MESSAGES.contentTypeDeleted
);
expect(isFieldBlockedByAutoDraft(meta({ variant: "v1" }))).toBe(true);
});
});
4 changes: 4 additions & 0 deletions src/visualBuilder/utils/fieldLockIndicator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ export const ENTRY_RESTRICTION_MESSAGES: Record<EntryEditRestriction, string> =
"You're viewing an older version of this entry. Switch to the latest version to edit.",
entryLocked:
"This entry was updated by someone else. Reload the entry to keep editing.",
contentTypeUpdated:
"This entry's content type was changed. Reload the entry to keep editing.",
contentTypeDeleted:
"This entry's content type was deleted. It can no longer be edited here.",
unlocalized:
"This entry isn't localized in this language yet. Save it from the form to localize it, then edit here.",
unsavedVariant:
Expand Down
23 changes: 12 additions & 11 deletions src/visualBuilder/utils/fieldLockStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,18 @@ export interface EntryFieldLock {

export type EntryFieldLockInfo = Record<string, EntryFieldLock>;

/** Every reason the parent can turn editing off for a whole entry scope. */
export const ENTRY_EDIT_RESTRICTIONS = [
"olderVersion",
"entryLocked",
"contentTypeUpdated",
"contentTypeDeleted",
"unlocalized",
"unsavedVariant",
] as const;

/** Why the parent has turned editing off for a whole entry scope. */

@hitesh-shetty-cstk hitesh-shetty-cstk Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this line and the comment on ENTRY_EDIT_RESTRICTIONS nine lines up say the same thing in two tenses. Now that the type is (typeof ENTRY_EDIT_RESTRICTIONS)[number], the const's comment already describes it. Deleting this one leaves the description in one place.

Generated by Claude Code

@SahilCs15 SahilCs15 Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair. Leaving it for now to avoid another CI round on a comment-only change.

export type EntryEditRestriction =
| "olderVersion"
| "entryLocked"
| "unlocalized"
| "unsavedVariant";
export type EntryEditRestriction = (typeof ENTRY_EDIT_RESTRICTIONS)[number];

/** The parts that identify a lock scope: entry + locale + variant. */
export interface EntryLockScopeParts {
Expand Down Expand Up @@ -121,12 +127,7 @@ export function getEntryEditRestrictionWriteSeq(): number {
return writeSeq;
}

const RESTRICTIONS: ReadonlySet<string> = new Set([
"olderVersion",
"entryLocked",
"unlocalized",
"unsavedVariant",
]);
const RESTRICTIONS: ReadonlySet<string> = new Set(ENTRY_EDIT_RESTRICTIONS);

/** Narrows a value from the parent; anything unknown is treated as "no restriction". */
export function toEntryEditRestriction(
Expand Down
Loading