Skip to content

BL-15855 upgrade config-r #7675

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
andrew-polk merged 3 commits into from
Feb 23, 2026
Merged

BL-15855 upgrade config-r #7675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27aad28
BL-15855 upgrade config-r
hatton Feb 10, 2026
02574b7
Guard collection settings save
hatton Feb 17, 2026
8cca304
Normalize configr settings posting
hatton Feb 17, 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
418 changes: 223 additions & 195 deletions src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 8 additions & 3 deletions src/BloomBrowserUI/bookEdit/bookSettings/FieldVisibilityGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const FieldVisibilityGroup: React.FunctionComponent<{
labelFrame: string;
labelFrameL10nKey: string;
settings: object | undefined;
settingsToReturnLater: string;
settingsToReturnLater: string | object | undefined;
disabled: boolean;
L1MustBeTurnedOn?: boolean;

Expand Down Expand Up @@ -83,8 +83,13 @@ export const FieldVisibilityGroup: React.FunctionComponent<{
const [showL1, showL2, showL3, numberShowing] = useMemo(() => {
let appearance = props.settings?.["appearance"];
if (props.settingsToReturnLater) {
// although we declared it a string, it appears the Config-R callback always gives us an object
appearance = props.settingsToReturnLater["appearance"];
// although we originally declared it a string, Config-R may return a JSON string or an object
if (typeof props.settingsToReturnLater === "string") {
const parsedSettings = JSON.parse(props.settingsToReturnLater);
appearance = parsedSettings["appearance"];
} else {
appearance = props.settingsToReturnLater["appearance"];
}
}
if (!appearance) {
// This is a bit arbitrary. It should only apply during early renders.
Expand Down
111 changes: 87 additions & 24 deletions src/BloomBrowserUI/collection/CollectionSettingsDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { css } from "@emotion/react";
import * as React from "react";
import { ConfigrGroup, ConfigrPane } from "@sillsdev/config-r";
import {
ConfigrGroup,
ConfigrPage,
ConfigrPane,
ConfigrStatic,
} from "@sillsdev/config-r";
import {
BloomDialog,
DialogBottomButtons,
Expand All @@ -27,15 +32,30 @@ export const CollectionSettingsDialog: React.FunctionComponent = () => {
);

const [settingsString, setSettingsString] = React.useState<string>("{}");
// Fetch collection settings when the dialog opens so we sync with host state.
React.useEffect(() => {
if (propsForBloomDialog.open)
get("collection/settings", (result) => {
setSettingsString(result.data);
});
}, [propsForBloomDialog.open]);

const [settingsToReturnLater, setSettingsToReturnLater] =
React.useState("");
const [settingsToReturnLater, setSettingsToReturnLater] = React.useState<
string | object | undefined
>(undefined);
Comment on lines +43 to +45
Copy link

@devin-ai-integration devin-ai-integration bot Feb 23, 2026

Choose a reason for hiding this comment

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

📝 Info: Stale settingsToReturnLater in CollectionSettingsDialog after Cancel

The settingsToReturnLater state at CollectionSettingsDialog.tsx:43-45 is never reset when the dialog reopens. Since useEventLaunchedBloomDialog keeps the component mounted and just toggles the open prop, the following scenario is possible:

  1. Open dialog → settingsToReturnLater = undefined
  2. Make changes → settingsToReturnLater = {some object}
  3. Click Cancel → dialog closes, state persists
  4. Open dialog again → settings is re-fetched (CollectionSettingsDialog.tsx:36-41), but settingsToReturnLater is NOT reset
  5. Click OK without changes → stale settings from step 2 get posted

This is pre-existing (the old code had the same issue with settingsToReturnLater = "" never being reset). It currently has no real impact because this dialog only has placeholder content ("Settings for this section are not available yet") and onChange never actually fires. But this will need to be addressed when real settings are added — e.g., by resetting setSettingsToReturnLater(undefined) inside the propsForBloomDialog.open effect.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Contributor

@andrew-polk andrew-polk Feb 23, 2026

Choose a reason for hiding this comment

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

I'm not fully understanding this. But it seems to be saying we have an existing bug here.
Seems worth dealing with it while we know about it?

Copy link
Contributor

@andrew-polk andrew-polk Feb 23, 2026

Choose a reason for hiding this comment

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

Oh, I see. This is for a new collection settings dialog we aren't using.
Ok, not worth holding up this PR.


const normalizeConfigrSettings = (
settingsValue: string | object | undefined,
): object | undefined => {
if (!settingsValue) {
return undefined;
}
if (typeof settingsValue === "string") {
return JSON.parse(settingsValue) as object;
}
return settingsValue;
};
// Parse the settings JSON for Configr's initial values once it arrives.
React.useEffect(() => {
if (settingsString === "{}") {
return; // leave settings as undefined
Expand Down Expand Up @@ -66,33 +86,76 @@ export const CollectionSettingsDialog: React.FunctionComponent = () => {
height: 100%;
`}
>
<ConfigrPane
label={"Collection Settings"}
showSearch={true}
// showJson={true} // useful for debugging
initialValues={settings || {}}
showAllGroups={true}
//themeOverrides={lightTheme}
themeOverrides={{
// enhance: we'd like to just be passing `lightTheme` but at the moment that seems to clobber everything
palette: {
primary: { main: kBloomBlue },
},
}}
onChange={(s) => {
setSettingsToReturnLater(s);
}}
>
<ConfigrGroup label={"Languages"}></ConfigrGroup>
<ConfigrGroup label={"Appearance"}></ConfigrGroup>
</ConfigrPane>
{settings && (
<ConfigrPane
label={"Collection Settings"}
showAppBar={false}
showSearch={true}
// showJson={true} // useful for debugging
initialValues={settings}
//themeOverrides={lightTheme}
themeOverrides={{
// enhance: we'd like to just be passing `lightTheme` but at the moment that seems to clobber everything
palette: {
primary: { main: kBloomBlue },
},
}}
onChange={(s) => {
setSettingsToReturnLater(s);
}}
>
<ConfigrPage
label={"Languages"}
pageKey="languages"
topLevel={true}
>
<ConfigrGroup label={"Languages"}>
<ConfigrStatic>
<div
css={css`
font-size: 0.9em;
color: #555;
`}
>
Settings for this section are not
available yet.
</div>
</ConfigrStatic>
</ConfigrGroup>
</ConfigrPage>
<ConfigrPage
label={"Appearance"}
pageKey="appearance"
topLevel={true}
>
<ConfigrGroup label={"Appearance"}>
<ConfigrStatic>
<div
css={css`
font-size: 0.9em;
color: #555;
`}
>
Settings for this section are not
available yet.
</div>
</ConfigrStatic>
</ConfigrGroup>
</ConfigrPage>
</ConfigrPane>
)}
</div>
</DialogMiddle>
<DialogBottomButtons>
<DialogOkButton
default={true}
onClick={() => {
postJson("collection/settings", settingsToReturnLater);
const settingsToPost = normalizeConfigrSettings(
settingsToReturnLater,
);
if (settingsToPost) {
postJson("collection/settings", settingsToPost);
}
closeDialog();
}}
/>
Expand Down
2 changes: 1 addition & 1 deletion src/BloomBrowserUI/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@
"@nivo/core": "^0.80.0",
"@nivo/scatterplot": "^0.80.0",
"@nivo/tooltip": "^0.80.0",
"@sillsdev/config-r": "1.0.0-alpha.15",
"@sillsdev/config-r": "1.0.0-alpha.18",
"@types/filesize": "^5.0.0",
"@types/react-transition-group": "^4.4.1",
"@use-it/event-listener": "^0.1.7",
Expand Down
15 changes: 0 additions & 15 deletions src/BloomBrowserUI/patches/@sillsdev+config-r+1.0.0-alpha.15.patch
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions src/BloomBrowserUI/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2930,10 +2930,10 @@
resolved "https://registry.yarnpkg.com/@rollup/rollup-win32-x64-msvc/-/rollup-win32-x64-msvc-4.52.5.tgz#1657f56326bbe0ac80eedc9f9c18fc1ddd24e107"
integrity sha512-TAcgQh2sSkykPRWLrdyy2AiceMckNf5loITqXxFI5VuQjS5tSuw3WlwdN8qv8vzjLAUTvYaH/mVjSFpbkFbpTg==

"@sillsdev/config-r@1.0.0-alpha.15":
version "1.0.0-alpha.15"
resolved "https://registry.yarnpkg.com/@sillsdev/config-r/-/config-r-1.0.0-alpha.15.tgz#0979c316e50e6d7edbe699e30235c3160cbf18b0"
integrity sha512-blKYURgkxTr7kdjR13+7JDwnyqolMtfKaTKAVRuEiyLxbNNUiHDIXazc7A9k/tNBPNpItAT21tu4E+3RZB9gXQ==
"@sillsdev/config-r@1.0.0-alpha.18":
version "1.0.0-alpha.18"
resolved "https://registry.npmjs.org/@sillsdev/config-r/-/config-r-1.0.0-alpha.18.tgz#177178ec2bba9e2843a3edab949c6b6489f0286d"
integrity sha512-EFiyAwUTMJ4jlvXRMBsO4+Zm8Gkaur+idUB3czXADqE0zG8ZnrMug951dWv67uFLH6hZT9jhGasEsHU1G/2/qA==
dependencies:
"@textea/json-viewer" "^2.13.1"
formik "^2.2.9"
Expand Down