Skip to content

fix: Allow repo dropdown to open on input when empty - #4422

Merged
regexowl merged 1 commit into
osbuild:mainfrom
jlsherrill:better_repos
May 27, 2026
Merged

fix: Allow repo dropdown to open on input when empty#4422
regexowl merged 1 commit into
osbuild:mainfrom
jlsherrill:better_repos

Conversation

@jlsherrill

@jlsherrill jlsherrill commented May 6, 2026

Copy link
Copy Markdown
Contributor

Removes inputValue condition from onInputClick so the dropdown opens when clicking the search box, even when empty, allowing users to see unfiltered repositories immediately.

Fixes HMS-10642

@jlsherrill
jlsherrill requested a review from a team as a code owner May 6, 2026 19:17

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • Setting skip: false on the repositories query means it will fire even when the dropdown is closed and no search is in progress; consider conditioning skip on isOpen and/or debouncedFilterValue to avoid unnecessary network requests.
  • The new empty-state branches for filtered vs unfiltered results add more conditional complexity in the render; you might consider extracting this into a small helper function (e.g., renderEmptyState(debouncedFilterValue)) to keep the JSX easier to scan.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Setting `skip: false` on the repositories query means it will fire even when the dropdown is closed and no search is in progress; consider conditioning `skip` on `isOpen` and/or `debouncedFilterValue` to avoid unnecessary network requests.
- The new empty-state branches for filtered vs unfiltered results add more conditional complexity in the render; you might consider extracting this into a small helper function (e.g., `renderEmptyState(debouncedFilterValue)`) to keep the JSX easier to scan.

## Individual Comments

### Comment 1
<location path="src/Components/CreateImageWizard/steps/Repositories/tests/Repositories.test.tsx" line_range="93-91" />
<code_context>
       resolveSearch(JSON.stringify({ data: [], meta: { count: 0 } }));
     });

+    test('shows unfiltered repositories when dropdown opens', async () => {
+      fetchMock.mockResponse(
+        createFetchHandler({ repositories: mockRepositoryResults }),
+      );
+      renderRepositoriesStep();
+      const user = createUser();
+
+      // Open dropdown by clicking toggle
+      const toggle = await screen.findByRole('button', { name: /menu toggle/i });
+      await user.click(toggle);
+
+      // Should show repositories without typing
+      expect(
+        await screen.findByRole('option', { name: /01-test-valid-repo/i }),
+      ).toBeInTheDocument();
+      expect(
+        await screen.findByRole('option', { name: /04-test-another-valid-repo/i }),
+      ).toBeInTheDocument();
+    });
+
+    test('shows appropriate empty state message based on filter state', async () => {
</code_context>
<issue_to_address>
**issue (testing):** Add a test that covers opening the dropdown via the search input click when the input is empty.

This change fixes behavior for clicking the empty search input, but the new test exercises opening via the toggle button instead. To directly cover the regression, please add a test that:

- Renders the component with no initial filter value
- Locates the search textbox (role="textbox", name /filter repositories/i)
- Clicks the textbox while the dropdown is closed and empty
- Asserts that the dropdown opens (options or the correct empty/loading state are visible)

That will specifically validate the `onInputClick` path this PR is addressing.
</issue_to_address>

### Comment 2
<location path="src/Components/CreateImageWizard/steps/Repositories/tests/Repositories.test.tsx" line_range="113-84" />
<code_context>
+      ).toBeInTheDocument();
+    });
+
+    test('shows appropriate empty state message based on filter state', async () => {
+      fetchMock.mockResponse(
+        createFetchHandler({ repositories: [] }),
+      );
+      renderRepositoriesStep();
+      const user = createUser();
+
+      // Open dropdown - should show "No repositories available"
+      const toggle = await screen.findByRole('button', { name: /menu toggle/i });
+      await user.click(toggle);
+
+      expect(
+        await screen.findByRole('option', { name: /no repositories available/i }),
+      ).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the empty‑state test by asserting that the previous message is no longer rendered after the filter changes.

In `shows appropriate empty state message based on filter state`, you already check that the message changes after typing. Please also assert that the initial "No repositories available" option is no longer present after typing (e.g. via `queryByRole` + `expect(...).not.toBeInTheDocument()`) to verify the component doesn’t render both empty states at once.

Suggested implementation:

```typescript
      expect(
        await screen.findByRole('option', { name: /no repositories match your filters/i }),
      ).toBeInTheDocument();

      // Initial empty-state message should no longer be rendered
      expect(
        screen.queryByRole('option', { name: /no repositories available/i }),
      ).not.toBeInTheDocument();

```

If the text of the second empty-state message is slightly different (e.g. different casing or wording), adjust the `/no repositories match your filters/i` regex in the SEARCH block to match the actual string used in your test.

If you used a different role or label for the initial empty-state (instead of `role: 'option'` or label `/no repositories available/i`), update the `queryByRole` arguments accordingly so the assertion targets the same element as your first empty-state check.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

).toBeInTheDocument();

resolveSearch(JSON.stringify({ data: [], meta: { count: 0 } }));
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (testing): Add a test that covers opening the dropdown via the search input click when the input is empty.

This change fixes behavior for clicking the empty search input, but the new test exercises opening via the toggle button instead. To directly cover the regression, please add a test that:

  • Renders the component with no initial filter value
  • Locates the search textbox (role="textbox", name /filter repositories/i)
  • Clicks the textbox while the dropdown is closed and empty
  • Asserts that the dropdown opens (options or the correct empty/loading state are visible)

That will specifically validate the onInputClick path this PR is addressing.

@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.57%. Comparing base (fb34a84) to head (6b1f5e1).
⚠️ Report is 2 commits behind head on main.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4422      +/-   ##
==========================================
+ Coverage   74.55%   74.57%   +0.02%     
==========================================
  Files         229      229              
  Lines        7478     7478              
  Branches     2777     2777              
==========================================
+ Hits         5575     5577       +2     
+ Misses       1636     1634       -2     
  Partials      267      267              
Files with missing lines Coverage Δ
...steps/Repositories/components/RepositorySearch.tsx 89.85% <100.00%> (+2.89%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84d79e4...6b1f5e1. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Removes inputValue condition from onInputClick so the dropdown
opens when clicking the search box, even when empty, allowing
users to see unfiltered repositories immediately.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@jlsherrill

Copy link
Copy Markdown
Contributor Author
image

@regexowl regexowl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Works nicely, thanks!

@regexowl
regexowl enabled auto-merge May 27, 2026 13:06
@regexowl
regexowl added this pull request to the merge queue May 27, 2026
Merged via the queue into osbuild:main with commit a7f3494 May 27, 2026
36 of 37 checks passed
@schutzbot schutzbot added the prod This PR has landed in production. label Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

prod This PR has landed in production.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants