🐛 Fixed bookmark filtering in Welcome Emails editor

[EvanHahn]

closes https://linear.app/ghost/issue/NY-1143

useFilterableApi filtered the response if data was already loaded, but not if it wasn't. This fixes that bug, which fixes the end-user bug.

Before.mp4

After.mp4

[coderabbitai]

Walkthrough

The changes refactor filtering logic in a filterable API hook by extracting a reusable filterData helper function that performs case-insensitive filtering on arrays of objects. The helper is applied to both cached data and fetched API responses. Test coverage is expanded to verify filter behavior on search input, validate correct API parameters, and ensure edge cases like empty results and concurrent filter changes are properly handled.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title mentions 'bookmark filtering in Welcome Emails editor' but the actual changes are in the useFilterableApi hook with test updates, making the title partially related but not focused on the main code change.	Consider a more precise title that reflects the core fix: 'Fix filtering in useFilterableApi hook' or similar, as the current title references a UI feature rather than the underlying hook implementation.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly explains the bug fix in useFilterableApi where responses weren't filtered when data wasn't already loaded, directly relating to the code changes in the pull request.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ny1143-fix-welcome-email-link

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

apps/admin-x-framework/test/unit/hooks/use-filterable-api.test.ts (1)

554-576: Test for whitespace-only filters could use non-empty mock data.

The mockData is an empty array, so this test doesn't actually validate the filtering behavior for whitespace inputs—it just confirms empty data returns empty results. Consider using non-empty mock data to meaningfully verify how whitespace-only filters (' ', '\t\n') affect filtering.

💡 Suggested improvement for meaningful whitespace test coverage

 it('handles empty and whitespace-only filters', async () => {
-    const mockData: TestData[] = [];
+    const mockData: TestData[] = [
+        {id: '1', name: 'John Doe', email: 'john@example.com'},
+        {id: '2', name: 'Jane Smith', email: 'jane@example.com'}
+    ];

     mockFetchApi.mockResolvedValue({
         users: mockData,
         meta: {pagination: {next: null}}
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/admin-x-framework/test/unit/hooks/use-filterable-api.test.ts` around
lines 554 - 576, The test "handles empty and whitespace-only filters" uses an
empty mockData which cannot verify filtering behavior; update the test to use a
non-empty mockData (e.g., an array with at least one TestData item), ensure
mockFetchApi.mockResolvedValue returns that data, then call
result.current.loadData with '', '   ', and '\t\n' and assert each call returns
the non-empty mockData so useFilterableApi and its loadData behavior for
whitespace-only filters is actually validated.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/admin-x-framework/test/unit/hooks/use-filterable-api.test.ts`:
- Around line 554-576: The test "handles empty and whitespace-only filters" uses
an empty mockData which cannot verify filtering behavior; update the test to use
a non-empty mockData (e.g., an array with at least one TestData item), ensure
mockFetchApi.mockResolvedValue returns that data, then call
result.current.loadData with '', '   ', and '\t\n' and assert each call returns
the non-empty mockData so useFilterableApi and its loadData behavior for
whitespace-only filters is actually validated.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 96088369-9c4b-4a63-b64d-375413b0d452

📥 Commits

Reviewing files that changed from the base of the PR and between fa877b1 and d05d7b9.

📒 Files selected for processing (2)

• apps/admin-x-framework/src/hooks/use-filterable-api.ts
• apps/admin-x-framework/test/unit/hooks/use-filterable-api.test.ts

[9larsons]

Looks great! Fixes a few other parts of Admin, too, as an added bonus.