Skip to content

ENG-1470: Fix dual-read gaps found during flag-ON validation #896

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
sid597 merged 7 commits into from
Apr 15, 2026
Merged

ENG-1470: Fix dual-read gaps found during flag-ON validation #896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f561e23
ENG-1470: Fix dual-read gaps found during flag-ON validation
sid597 Mar 16, 2026
94cf1e2
Fix dual-read mismatches and ZodError on discourse node parse
sid597 Mar 16, 2026
e2d140e
Fix dual-read mismatches: alias timing, key-image re-render, deepEqua…
sid597 Mar 17, 2026
8830ade
Fix prettier formatting
sid597 Mar 17, 2026
86cf3f0
ENG-1574: Add dual-read console logs to setting setters (#914)
sid597 Apr 1, 2026
d8ff16f
Eng 1616 add getconfigtree equivalent for block pros on init (#944)
sid597 Apr 15, 2026
25cc406
ENG-1617: se existing 'getConfigTree equivalent functions' for specif…
sid597 Apr 15, 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
18 changes: 8 additions & 10 deletions apps/roam/src/components/DiscourseFloatingMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import { FeedbackWidget } from "./BirdEatsBugs";
import { render as renderSettings } from "~/components/settings/Settings";
import posthog from "posthog-js";
import { getPersonalSetting } from "./settings/utils/accessors";
import { type SettingsSnapshot } from "./settings/utils/accessors";
import { PERSONAL_KEYS } from "./settings/utils/settingKeys";

type DiscourseFloatingMenuProps = {
Expand Down Expand Up @@ -118,26 +118,23 @@ export const showDiscourseFloatingMenu = () => {

export const installDiscourseFloatingMenu = (
onLoadArgs: OnloadArgs,
props: DiscourseFloatingMenuProps = {
position: "bottom-right",
theme: "bp3-light",
buttonTheme: "bp3-light",
},
snapshot: SettingsSnapshot,
) => {
let floatingMenuAnchor = document.getElementById(ANCHOR_ID);
if (!floatingMenuAnchor) {
floatingMenuAnchor = document.createElement("div");
floatingMenuAnchor.id = ANCHOR_ID;
document.getElementById("app")?.appendChild(floatingMenuAnchor);
}
if (getPersonalSetting<boolean>([PERSONAL_KEYS.hideFeedbackButton])) {
if (snapshot.personalSettings[PERSONAL_KEYS.hideFeedbackButton]) {
floatingMenuAnchor.classList.add("hidden");
}
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<DiscourseFloatingMenu
position={props.position}
theme={props.theme}
buttonTheme={props.buttonTheme}
position="bottom-right"
theme="bp3-light"
buttonTheme="bp3-light"
onloadArgs={onLoadArgs}
/>,
floatingMenuAnchor,
Expand All @@ -148,6 +145,7 @@ export const removeDiscourseFloatingMenu = () => {
const anchor = document.getElementById(ANCHOR_ID);
if (anchor) {
try {
// eslint-disable-next-line react/no-deprecated
ReactDOM.unmountComponentAtNode(anchor);
} catch (e) {
// no-op: unmount best-effort
Expand Down
18 changes: 6 additions & 12 deletions apps/roam/src/components/DiscourseNodeMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,9 @@ import { getNewDiscourseNodeText } from "~/utils/formatUtils";
import { OnloadArgs } from "roamjs-components/types";
import { formatHexColor } from "./settings/DiscourseNodeCanvasSettings";
import posthog from "posthog-js";
import {
getPersonalSetting,
setPersonalSetting,
} from "~/components/settings/utils/accessors";
import { setPersonalSetting } from "~/components/settings/utils/accessors";
import { PERSONAL_KEYS } from "~/components/settings/utils/settingKeys";
import type { PersonalSettings } from "~/components/settings/utils/zodSchema";

type Props = {
textarea?: HTMLTextAreaElement;
Expand Down Expand Up @@ -420,19 +418,15 @@ export const comboToString = (combo: IKeyCombo): string => {

export const NodeMenuTriggerComponent = ({
extensionAPI,
initialValue,
}: {
extensionAPI: OnloadArgs["extensionAPI"];
initialValue: PersonalSettings["Personal node menu trigger"];
}) => {
const inputRef = useRef<HTMLInputElement>(null);
const [isActive, setIsActive] = useState(false);
const [comboKey, setComboKey] = useState<IKeyCombo>(
() =>
getPersonalSetting<IKeyCombo>([
PERSONAL_KEYS.personalNodeMenuTrigger,
]) || {
modifiers: 0,
key: "",
},
const [comboKey, setComboKey] = useState<IKeyCombo>(() =>
typeof initialValue === "object" ? initialValue : { modifiers: 0, key: "" },
);

const handleKeyDown = useCallback(
Expand Down
9 changes: 4 additions & 5 deletions apps/roam/src/components/DiscourseNodeSearchMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ import getDiscourseNodes, { DiscourseNode } from "~/utils/getDiscourseNodes";
import getDiscourseNodeFormatExpression from "~/utils/getDiscourseNodeFormatExpression";
import { Result } from "~/utils/types";
import MiniSearch from "minisearch";
import {
getPersonalSetting,
setPersonalSetting,
} from "~/components/settings/utils/accessors";
import { setPersonalSetting } from "~/components/settings/utils/accessors";
import { PERSONAL_KEYS } from "~/components/settings/utils/settingKeys";

type Props = {
Expand Down Expand Up @@ -709,12 +706,14 @@ export const renderDiscourseNodeSearchMenu = (props: Props) => {

export const NodeSearchMenuTriggerSetting = ({
onloadArgs,
initialValue,
}: {
onloadArgs: OnloadArgs;
initialValue: string;
}) => {
const extensionAPI = onloadArgs.extensionAPI;
const [nodeSearchTrigger, setNodeSearchTrigger] = useState<string>(
getPersonalSetting<string>([PERSONAL_KEYS.nodeSearchMenuTrigger]) ?? "@",
() => initialValue,
);

const handleNodeSearchTriggerChange = (
Expand Down
97 changes: 63 additions & 34 deletions apps/roam/src/components/LeftSidebarView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,18 @@ import { getLeftSidebarSettings } from "~/utils/getLeftSidebarSettings";
import {
getGlobalSetting,
getPersonalSetting,
getPersonalSettings,
setGlobalSetting,
setPersonalSetting,
type SettingsSnapshot,
} from "~/components/settings/utils/accessors";
import {
PERSONAL_KEYS,
GLOBAL_KEYS,
LEFT_SIDEBAR_KEYS,
LEFT_SIDEBAR_SETTINGS_KEYS,
} from "~/components/settings/utils/settingKeys";
import type {
LeftSidebarGlobalSettings,
PersonalSection,
} from "~/components/settings/utils/zodSchema";
import type { LeftSidebarGlobalSettings } from "~/components/settings/utils/zodSchema";
import { createBlock } from "roamjs-components/writes";
import deleteBlock from "roamjs-components/writes/deleteBlock";
import getTextByBlockUid from "roamjs-components/queries/getTextByBlockUid";
Expand Down Expand Up @@ -112,7 +111,7 @@ const openTarget = async (e: React.MouseEvent, targetUid: string) => {
}
};

const toggleFoldedState = ({
const toggleFoldedState = async ({
isOpen,
setIsOpen,
folded,
Expand All @@ -130,23 +129,26 @@ const toggleFoldedState = ({
const newFolded = !isOpen;

if (isOpen) {
setIsOpen(false);
if (folded.uid) {
void deleteBlock(folded.uid);
folded.uid = undefined;
folded.value = false;
}
const children = getBasicTreeByParentUid(parentUid);
await Promise.all(
children
.filter((c) => c.text === "Folded")
.map((c) => deleteBlock(c.uid)),
);
folded.uid = undefined;
folded.value = false;
} else {
setIsOpen(true);
const newUid = window.roamAlphaAPI.util.generateUID();
void createBlock({
await createBlock({
parentUid,
node: { text: "Folded", uid: newUid },
});
folded.uid = newUid;
folded.value = true;
}

refreshConfigTree();

if (isGlobal) {
setGlobalSetting(
[
Expand All @@ -157,13 +159,20 @@ const toggleFoldedState = ({
newFolded,
);
} else if (sectionIndex !== undefined) {
const sections =
getPersonalSetting<PersonalSection[]>([PERSONAL_KEYS.leftSidebar]) || [];
const sections = [...getPersonalSettings()[PERSONAL_KEYS.leftSidebar]];
if (sections[sectionIndex]) {
sections[sectionIndex].Settings.Folded = newFolded;
sections[sectionIndex] = {
...sections[sectionIndex],
Settings: {
...sections[sectionIndex].Settings,
Folded: newFolded,
},
};
setPersonalSetting([PERSONAL_KEYS.leftSidebar], sections);
}
}

setIsOpen(newFolded);
Comment on lines 132 to +175
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app bot Apr 15, 2026

Choose a reason for hiding this comment

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

Race condition: setIsOpen(newFolded) is called after async database operations complete, but the callers at lines 237 and 309 use void toggleFoldedState(...) which discards the promise. If the user clicks multiple times rapidly, the UI state (isOpen) can become out of sync with the database state.

The function should either:

// Option 1: Callers should await
await toggleFoldedState({...});

// Option 2: Move setIsOpen before async operations
const newFolded = !isOpen;
setIsOpen(newFolded);
// then do async operations

This prevents the component from being in an inconsistent state where isOpen doesn't match the actual folded state in the database during the async operation.

Spotted by Graphite

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.

};

const SectionChildren = ({
Expand Down Expand Up @@ -225,7 +234,7 @@ const PersonalSectionItem = ({
const handleChevronClick = () => {
if (!section.settings) return;

toggleFoldedState({
void toggleFoldedState({
isOpen,
setIsOpen,
folded: section.settings.folded,
Expand Down Expand Up @@ -297,7 +306,7 @@ const GlobalSection = ({ config }: { config: LeftSidebarConfig["global"] }) => {
className="sidebar-title-button flex w-full items-center border-none bg-transparent py-1 pl-6 pr-2.5 font-semibold outline-none"
onClick={() => {
if (!isCollapsable || !config.settings) return;
toggleFoldedState({
void toggleFoldedState({
isOpen,
setIsOpen,
folded: config.settings.folded,
Expand Down Expand Up @@ -328,14 +337,16 @@ const GlobalSection = ({ config }: { config: LeftSidebarConfig["global"] }) => {

// TODO(ENG-1471): Remove old-system merge when migration complete — just use accessor values directly.
// See mergeGlobalSectionWithAccessor/mergePersonalSectionsWithAccessor for why the merge exists.
const buildConfig = (): LeftSidebarConfig => {
const buildConfig = (snapshot?: SettingsSnapshot): LeftSidebarConfig => {
// Read VALUES from accessor (handles flag routing + mismatch detection)
const globalValues = getGlobalSetting<LeftSidebarGlobalSettings>([
GLOBAL_KEYS.leftSidebar,
]);
const personalValues = getPersonalSetting<PersonalSection[]>([
PERSONAL_KEYS.leftSidebar,
]);
const globalValues = snapshot
? snapshot.globalSettings[GLOBAL_KEYS.leftSidebar]
: getGlobalSetting<LeftSidebarGlobalSettings>([GLOBAL_KEYS.leftSidebar]);
const personalValues = snapshot
? snapshot.personalSettings[PERSONAL_KEYS.leftSidebar]
: getPersonalSetting<
ReturnType<typeof getPersonalSettings>[typeof PERSONAL_KEYS.leftSidebar]
>([PERSONAL_KEYS.leftSidebar]);

// Read UIDs from old system (needed for fold CRUD during dual-write)
const oldConfig = getCurrentLeftSidebarConfig();
Expand All @@ -356,8 +367,8 @@ const buildConfig = (): LeftSidebarConfig => {
};
};

export const useConfig = () => {
const [config, setConfig] = useState(() => buildConfig());
export const useConfig = (initialSnapshot?: SettingsSnapshot) => {
const [config, setConfig] = useState(() => buildConfig(initialSnapshot));
useEffect(() => {
const handleUpdate = () => {
setConfig(buildConfig());
Expand Down Expand Up @@ -496,8 +507,14 @@ const FavoritesPopover = ({ onloadArgs }: { onloadArgs: OnloadArgs }) => {
);
};

const LeftSidebarView = ({ onloadArgs }: { onloadArgs: OnloadArgs }) => {
const { config } = useConfig();
const LeftSidebarView = ({
onloadArgs,
initialSnapshot,
}: {
onloadArgs: OnloadArgs;
initialSnapshot?: SettingsSnapshot;
}) => {
const { config } = useConfig(initialSnapshot);

return (
<>
Expand Down Expand Up @@ -602,10 +619,15 @@ const migrateFavorites = async () => {
refreshConfigTree();
};

export const mountLeftSidebar = async (
wrapper: HTMLElement,
onloadArgs: OnloadArgs,
): Promise<void> => {
export const mountLeftSidebar = async ({
wrapper,
onloadArgs,
initialSnapshot,
}: {
wrapper: HTMLElement;
onloadArgs: OnloadArgs;
initialSnapshot?: SettingsSnapshot;
}): Promise<void> => {
if (!wrapper) return;

const id = "dg-left-sidebar-root";
Expand All @@ -622,7 +644,14 @@ export const mountLeftSidebar = async (
} else {
root.className = "starred-pages";
}
ReactDOM.render(<LeftSidebarView onloadArgs={onloadArgs} />, root);
// eslint-disable-next-line react/no-deprecated
ReactDOM.render(
<LeftSidebarView
onloadArgs={onloadArgs}
initialSnapshot={initialSnapshot}
/>,
root,
);
};

export default LeftSidebarView;
Loading
Loading