fix(usage-logs): filter blank model options#1287
Conversation
Blank or whitespace-only model values appeared in the model filter dropdown. Add SQL-level btrim exclusion in getUsedModels, filter blank entries in the lazy-loading hook, and guard the UI select rendering. Includes unit tests for repository and UI layers. Fixes #1285
📝 WalkthroughWalkthrough修复了模型下拉框因数据库中存在空/空白字符串模型名而触发 Changes空白模型字符串过滤
Biome 工具链版本更新
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request filters out empty or blank model names from the model filter options. This filtering is implemented at the database query level, within the useLazyModels hook, and in the RequestFilters UI component, with accompanying tests added to verify the behavior. The reviewer feedback correctly identifies that filtering inside the UI component is redundant since it is already handled at the hook and database levels. The reviewer suggests removing this redundant filtering, mapping directly over models, and updating the test mocks to simulate the hook's filtering behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| () => new Map(providers.map((provider) => [provider.id, provider.name])), | ||
| [providers] | ||
| ); | ||
| const modelOptions = useMemo(() => models.filter((model) => model.trim().length > 0), [models]); |
There was a problem hiding this comment.
The models array returned by the useLazyModels() hook is already filtered to exclude empty and blank strings (both at the hook level and the repository/API level). Therefore, defining modelOptions with another filter inside useMemo is redundant. We can remove this useMemo entirely and use models directly.
| <SelectContent> | ||
| <SelectItem value="all">{t("logs.filters.allModels")}</SelectItem> | ||
| {models.map((model) => ( | ||
| {modelOptions.map((model) => ( |
There was a problem hiding this comment.
Since modelOptions is redundant and can be removed, we should map over models directly here.
| {modelOptions.map((model) => ( | |
| {models.map((model) => ( |
| vi.mock("@/app/[locale]/dashboard/logs/_hooks/use-lazy-filter-options", () => ({ | ||
| useLazyModels: () => ({ | ||
| data: [], | ||
| data: lazyFilterOptionMocks.models, |
There was a problem hiding this comment.
Since the filtering responsibility has been moved to the useLazyModels hook, the mock for useLazyModels in this test should simulate this behavior by filtering the mocked models. This allows us to remove the redundant filtering from the RequestFilters component while keeping the integration test green and accurate.
| data: lazyFilterOptionMocks.models, | |
| data: lazyFilterOptionMocks.models.filter((m) => m.trim().length > 0), |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| and( | ||
| isNull(messageRequest.deletedAt), | ||
| sql`${messageRequest.model} IS NOT NULL`, | ||
| sql`btrim(${messageRequest.model}) <> ''` | ||
| ) |
There was a problem hiding this comment.
IS NOT NULL condition is now redundant. In PostgreSQL, btrim(NULL) returns NULL, and NULL <> '' evaluates to NULL (not TRUE), so null rows are already excluded by the btrim condition. Keeping it is harmless, but removing it simplifies the predicate.
| and( | |
| isNull(messageRequest.deletedAt), | |
| sql`${messageRequest.model} IS NOT NULL`, | |
| sql`btrim(${messageRequest.model}) <> ''` | |
| ) | |
| and( | |
| isNull(messageRequest.deletedAt), | |
| sql`btrim(${messageRequest.model}) <> ''` | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/usage-logs.ts
Line: 1473-1477
Comment:
The `IS NOT NULL` condition is now redundant. In PostgreSQL, `btrim(NULL)` returns `NULL`, and `NULL <> ''` evaluates to `NULL` (not `TRUE`), so null rows are already excluded by the `btrim` condition. Keeping it is harmless, but removing it simplifies the predicate.
```suggestion
and(
isNull(messageRequest.deletedAt),
sql`btrim(${messageRequest.model}) <> ''`
)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| const selectDistinctMock = vi.fn(() => | ||
| createThenableQuery([ | ||
| { model: "" }, | ||
| { model: " " }, | ||
| { model: "claude-sonnet-4-5" }, | ||
| { model: "gpt-4o" }, | ||
| { model: null }, | ||
| ]) | ||
| ); | ||
|
|
||
| vi.doMock("@/drizzle/db", () => ({ |
There was a problem hiding this comment.
The mock's where function ignores its arguments and always returns the same pre-populated result set (including blank/null rows). This means the test only exercises the JavaScript post-processing filter in getUsedModels, not the btrim / IS NOT NULL SQL conditions. If those SQL conditions were accidentally removed, this test would still pass because isNonBlankString on line 1481 catches the blanks at the JS level. Consider adding an assertion that where received the expected SQL conditions, or document that SQL coverage is intentionally deferred to integration tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/repository/usage-logs-model-filter-options.test.ts
Line: 14-25
Comment:
**SQL WHERE conditions are not asserted in the test**
The mock's `where` function ignores its arguments and always returns the same pre-populated result set (including blank/null rows). This means the test only exercises the JavaScript post-processing filter in `getUsedModels`, not the `btrim` / `IS NOT NULL` SQL conditions. If those SQL conditions were accidentally removed, this test would still pass because `isNonBlankString` on line 1481 catches the blanks at the JS level. Consider adding an assertion that `where` received the expected SQL conditions, or document that SQL coverage is intentionally deferred to integration tests.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`[locale]/dashboard/logs/_hooks/use-lazy-filter-options.ts:
- Around line 103-106: The code at lines 123-125 directly calls value.trim()
without proper runtime type checking, which will throw an exception if the
upstream data contains null, undefined, or non-string values. Add a proper type
guard that checks whether the value is actually a string before calling trim()
on it. This ensures that malformed or unexpected data from the upstream response
won't cause the model dropdown loading to fail, and makes the defensive
filtering truly robust against all data type violations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c663e03-bcd0-4077-8124-f3e8590f0c3f
📒 Files selected for processing (6)
biome.jsonsrc/app/[locale]/dashboard/logs/_components/filters/request-filters.tsxsrc/app/[locale]/dashboard/logs/_hooks/use-lazy-filter-options.tssrc/repository/usage-logs.tstests/unit/dashboard-logs-sessionid-suggestions-ui.test.tsxtests/unit/repository/usage-logs-model-filter-options.test.ts
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: S
- Lines changed: 127 (116 additions, 11 deletions)
- Files changed: 6
Changes Analysis
This PR implements a well-designed three-layer defensive approach to filter blank model names from the usage-logs filter UI:
- SQL Layer (
usage-logs.ts): PostgreSQLbtrim()condition filters at query time - Hook Layer (
use-lazy-filter-options.ts): JS filter on API response - Component Layer (
request-filters.tsx): Memoized filter before render
The defense-in-depth strategy is sound. If any layer fails or has stale data, the subsequent layers prevent blank Select.Item values from reaching the UI.
Coverage Assessment
- ✅ Unit test validates JS
isNonBlankStringfilter behavior - ✅ UI test verifies Select.Item rendering excludes blank values
- ✅ Biome schema aligned with CLI version
Note: The SQL btrim WHERE condition is not asserted in the unit test (the mock ignores where() arguments). This is an acceptable gap given:
- Integration tests cover end-to-end behavior
- The JS filter provides a reliable safety net
- SQL filter is primarily a performance optimization
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Claude AI
isNonBlankString assumed its argument was always a string, so malformed API responses containing null, numbers, or other non-string values could slip through the blank check and pollute filter dropdowns or cause runtime errors. Widen the parameter to unknown and convert the helper into a type guard that rejects non-string values before trimming. Add a test verifying useLazyModels drops malformed entries and retains only valid non-blank strings. Fixes #1285
🧪 测试结果
总体结果: ✅ 所有测试通过 |
1 participant
Summary
Fixes #1285
Tests
Greptile Summary
This PR adds layered, defensive filtering of blank/whitespace-only model names that could otherwise create empty
<SelectItem>values in the usage-log model filter dropdown. Filtering is applied at three independent layers: the SQL query (btrim), the API-response hook, and the UI component'suseMemo.usage-logs.ts): adds abtrim(model) <> ''SQL predicate and a JS-levelisNonBlankStringfallback so the DB query never returns blank strings.use-lazy-filter-options.ts): wrapsgetModelList()with anisNonBlankStringguard so malformed API responses are stripped before reaching state.request-filters.tsx): computesmodelOptionsviauseMemoto prevent any remaining blanks from being rendered as<SelectItem>nodes, andbiome.jsonis bumped to match the locked CLI version.Confidence Score: 5/5
Safe to merge — the change only adds filtering guards at multiple layers, with no mutations to data writes or auth paths.
The change is purely additive: blank-string filtering is applied at the SQL predicate, the API hook, and the UI rendering stage. No existing data is modified, no auth or write paths are touched, and each new layer has a corresponding test.
No files require special attention.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(logs): drop non-string entries from ..." | Re-trigger Greptile