Skip to content

Add a 'symbolList' entry type to handle the UI for lenders of last re… - #46

Draft
kurtnordstrom wants to merge 1 commit into
mainfrom
adjust-lenders-of-last-resort
Draft

kurtnordstrom wants to merge 1 commit into
mainfrom
adjust-lenders-of-last-resort

Conversation

@kurtnordstrom

Copy link
Copy Markdown
Contributor

…sort

Copilot AI left a comment

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.

🟡 Changes recommended

It introduces a potentially backwards-incompatible config shape change for lendersOfLastResort and the current symbol loading strategy can create avoidable load/UX blocking for large Institution datasets.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces a new symbolList field type to the SettingsConfigEditor, enabling config fields (notably ILL “lenders of last resort”) to be edited as a list of Institution symbols loaded from directory/entries.

Changes:

  • Added symbolList support to SettingsConfigEditor, including Institution symbol loading/paging and new UI strings.
  • Switched lendersOfLastResort in EditIllConfigRoute from stringArray to symbolList.
  • Added a Jest/RTL test suite covering symbol loading, selection, de-duplication, and patch payload shape.
File summaries
File Description
ui-rsdir/translations/ui-rsdir/en.json Adds i18n strings for symbol selection/removal and load errors.
ui-rsdir/src/routes/EditIllConfigRoute.js Uses the new symbolList type for lendersOfLastResort.
ui-rsdir/src/components/SettingsConfigEditor.js Implements symbolList behavior, including fetching/paging Institution entries and rendering/selecting/removing symbols.
ui-rsdir/src/components/SettingsConfigEditor.test.js Adds tests validating symbolList UI behavior and patch serialization.
Review details

Suppressed comments (2)

ui-rsdir/src/components/SettingsConfigEditor.js:335

  • symbolsAreLoading treats hasNextInstitutionPage as "loading", which keeps the symbol <Select> disabled until the entire Institution dataset finishes paging. If paging is slow, users cannot select from already-loaded symbols; consider basing the disabled state on isLoading || isFetchingNextPage instead.
  const symbolsAreLoading = institutionEntriesAreLoading ||
    isFetchingNextInstitutionPage || hasNextInstitutionPage;

ui-rsdir/src/components/SettingsConfigEditor.js:325

  • This effect will automatically page through all Institution results (calling fetchNextInstitutionPage() repeatedly) as long as hasNextInstitutionPage remains true. For large directories this can cause sustained background load; consider deferring auto-paging until the symbol selector is actually needed (e.g., edit mode / selector opened) or adding a hard cap.
  useEffect(() => {
    if (hasSymbolList && hasNextInstitutionPage &&
      !isFetchingNextInstitutionPage && !institutionEntriesHaveError) {
      fetchNextInstitutionPage();
    }
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 22 to 25
{
fieldName: 'lendersOfLastResort',
valueType: 'stringArray',
valueType: 'symbolList',
},
Comment on lines +299 to +303
},
enabled: hasSymbolList,
getNextPageParam: nextInstitutionPageOffset,
staleTime: 2 * 60 * 1000,
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants