Skip to content

BL-15855 upgrade config-r#7675

Open
hatton wants to merge 1 commit intomasterfrom
BL-15855-configr-upgrade
Open

BL-15855 upgrade config-r#7675
hatton wants to merge 1 commit intomasterfrom
BL-15855-configr-upgrade

Conversation

@hatton
Copy link
Member

@hatton hatton commented Feb 10, 2026


Open with Devin

This change is Reviewable

Copilot AI review requested due to automatic review settings February 10, 2026 16:02
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

View 3 additional findings in Devin Review.

Open in Devin Review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Upgrades the BloomBrowserUI dependency on @sillsdev/config-r and updates the Book Settings dialog to match the newer Config-R page/group API.

Changes:

  • Bump @sillsdev/config-r from 1.0.0-alpha.15 to 1.0.0-alpha.18 (and update yarn.lock accordingly).
  • Remove the no-longer-needed patch-package patch that adjusted @sillsdev/config-r’s package.json exports.
  • Refactor BookSettingsDialog to use ConfigrPage/ConfigrGroup/ConfigrStatic and update the initial selection prop name.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
src/BloomBrowserUI/yarn.lock Locks @sillsdev/config-r to 1.0.0-alpha.18 with updated resolved/integrity metadata.
src/BloomBrowserUI/patches/@sillsdev+config-r+1.0.0-alpha.15.patch Removes the patch previously required to fix Config-R exports.
src/BloomBrowserUI/package.json Updates the declared @sillsdev/config-r dependency version.
src/BloomBrowserUI/bookEdit/bookSettings/BookSettingsDialog.tsx Updates the dialog structure to Config-R’s newer page-based API and adjusts theming for the custom slider.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hatton hatton force-pushed the BL-15855-configr-upgrade branch from 023f82b to 6c6f578 Compare February 10, 2026 17:53
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

@hatton hatton force-pushed the BL-15855-configr-upgrade branch from 6c6f578 to 426e9b1 Compare February 10, 2026 18:04
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

View 4 additional findings in Devin Review.

Open in Devin Review

@hatton hatton force-pushed the BL-15855-configr-upgrade branch from 426e9b1 to 27aad28 Compare February 10, 2026 18:46
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

🐛 1 issue in files not directly in the diff

🐛 FieldVisibilityGroup still accesses settingsToReturnLater as an object after config-r upgrade changed onChange to return a JSON string (src/BloomBrowserUI/bookEdit/bookSettings/FieldVisibilityGroup.tsx:85-88)

The config-r upgrade changed the onChange callback to return a JSON string instead of an object. BookSettingsDialog.tsx:361-362 was updated to call JSON.parse(settingsToReturnLater) to handle this change, but FieldVisibilityGroup.tsx was not updated.

Root Cause and Impact

At FieldVisibilityGroup.tsx:85-88, settingsToReturnLater is still accessed as if it were an object:

if (props.settingsToReturnLater) {
    // although we declared it a string, it appears the Config-R callback always gives us an object
    appearance = props.settingsToReturnLater["appearance"];
}

The old comment on line 86 even acknowledges the old config-r was giving an object despite the string type. Now that the new config-r correctly returns a JSON string, props.settingsToReturnLater["appearance"] on a string will return undefined. This causes the code to fall through to the early return at FieldVisibilityGroup.tsx:89-91, always returning [true, false, false, 1].

Impact: The checkbox locking logic (which prevents the user from turning off all language visibility checkboxes) will not reflect the current live state of the settings. It will always behave as if only L1 is showing (numberShowing === 1), meaning L1 will always be locked and L2/L3 will never be locked, even when they are the only one turned on. This breaks the mutual-exclusion guard that ensures at least one language is always visible.

View 6 additional findings in Devin Review.

Open in Devin Review

Choose a reason for hiding this comment

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

🚩 CollectionSettingsDialog posts empty string on OK if user makes no changes

When the user opens the CollectionSettingsDialog and clicks OK without changing anything, settingsToReturnLater is still the initial empty string "". Unlike BookSettingsDialog.tsx:384-388 which guards with if (settingsToReturnLater), the CollectionSettingsDialog at line 140 unconditionally calls postJson("collection/settings", settingsToReturnLater). This sends an empty string to the server. Whether this is a bug depends on how the C# backend handles an empty string body — it may be harmless (e.g., the backend ignores empty bodies), or it may cause an error or unintended reset. The BookSettingsDialog has this guard; CollectionSettingsDialog should probably have it too for consistency.

(Refers to lines 137-143)

Open in Devin Review

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant