Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@chhoumann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughNormalize and guard display items across suggesters: add a normalize utility, store normalized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/gui/GenericSuggester/genericSuggester.ts (1)
7-11: Duplicate utility function—see earlier comment onInputSuggester.This
normalizeDisplayItemfunction is identical to the one ininputSuggester.ts. Same recommendation applies: extract to a shared module.
🧹 Nitpick comments (3)
src/gui/GenericSuggester/genericSuggester.test.ts (1)
12-19: Good regression test for issue #586.The test correctly simulates non-string display items and verifies
getSuggestionsdoesn't throw. Consider strengthening the assertion to verify actual normalization behavior.🔎 Optional: verify normalized output
it("normalizes non-string display items for fuzzy matching", () => { const items = [{ id: 1 }, { id: 2 }]; const displayItems = items as unknown as string[]; const suggester = new GenericSuggester(app, displayItems, items); - expect(() => suggester.getSuggestions("1")).not.toThrow(); + const suggestions = suggester.getSuggestions("1"); + expect(suggestions).toBeDefined(); + // Verify normalization produces "[object Object]" for plain objects + expect(suggester.getItemText(items[0])).toBe("[object Object]"); });src/gui/InputSuggester/inputSuggester.ts (1)
18-22:normalizeDisplayItemis duplicated across suggesters.This utility function is identical to the one in
genericSuggester.ts. Consider extracting it to a shared module (e.g.,src/utils/normalize.tsor a shared suggester utilities file) to reduce duplication.🔎 Example shared utility
Create
src/gui/suggestUtils.ts:export const normalizeDisplayItem = (value: unknown): string => { if (typeof value === "string") return value; if (value == null) return ""; return String(value); };Then import in both suggesters:
-const normalizeDisplayItem = (value: unknown): string => { - if (typeof value === "string") return value; - if (value == null) return ""; - return String(value); -}; +import { normalizeDisplayItem } from "../suggestUtils";src/gui/GenericSuggester/genericSuggester.ts (1)
109-116: Defensive fallback for custom render is good, but consider logging the error.The try-catch prevents crashes from custom
renderItemimplementations. However, silently catching errors may make debugging harder for users with custom renderers.🔎 Optional: log the render error
try { el.empty(); this.renderItem(value.item, el); - } catch { + } catch (e) { // Fallback to default rendering if custom render throws el.empty(); super.renderSuggestion(value, el); + log.logWarning("Custom renderItem threw an error, falling back to default rendering."); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/gui/GenericSuggester/genericSuggester.test.ts(1 hunks)src/gui/GenericSuggester/genericSuggester.ts(4 hunks)src/gui/InputSuggester/inputSuggester.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/gui/GenericSuggester/genericSuggester.test.tssrc/gui/InputSuggester/inputSuggester.tssrc/gui/GenericSuggester/genericSuggester.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/gui/GenericSuggester/genericSuggester.test.tssrc/gui/InputSuggester/inputSuggester.tssrc/gui/GenericSuggester/genericSuggester.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/gui/GenericSuggester/genericSuggester.test.tssrc/gui/InputSuggester/inputSuggester.tssrc/gui/GenericSuggester/genericSuggester.ts
🧠 Learnings (4)
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to tests/**/*.{ts,tsx} : Add regression coverage for bug fixes
Applied to files:
src/gui/GenericSuggester/genericSuggester.test.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to src/**/*.{ts,tsx} : Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or `tests/obsidian-stub.ts`
Applied to files:
src/gui/GenericSuggester/genericSuggester.test.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to tests/**/*.{ts,tsx} : Co-locate specs with their source or group them under `tests/feature-name`
Applied to files:
src/gui/GenericSuggester/genericSuggester.test.ts
📚 Learning: 2025-12-09T21:20:52.425Z
Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.425Z
Learning: Applies to tests/**/*.test.{ts,tsx} : Use Testing Library helpers for Svelte components
Applied to files:
src/gui/GenericSuggester/genericSuggester.test.ts
🧬 Code graph analysis (3)
src/gui/GenericSuggester/genericSuggester.test.ts (2)
src/quickAddApi.ts (1)
suggester(585-629)src/gui/GenericSuggester/genericSuggester.ts (1)
GenericSuggester(13-144)
src/gui/InputSuggester/inputSuggester.ts (1)
docs/static/scripts/EzImport.js (1)
item(157-160)
src/gui/GenericSuggester/genericSuggester.ts (3)
tests/obsidian-stub.ts (1)
FuzzySuggestModal(106-181)docs/static/scripts/EzImport.js (1)
item(157-160)src/logger/logManager.ts (1)
log(59-59)
🔇 Additional comments (6)
src/gui/InputSuggester/inputSuggester.ts (3)
97-105: Alignment and warning logic looks correct.The length-mismatch handling properly normalizes missing display items using the corresponding item value as fallback. The one-time warning mechanism is appropriate for alerting developers without flooding logs.
108-114: Index-based lookup ingetItemTextis sound.The change correctly derives the display text from
displayItemsvia index mapping, with proper fallback normalization. This ensures fuzzy matching always receives a string.
183-196: One-time warning implementation is appropriate.Correctly uses the logger utility per coding guidelines and prevents log spam with the
warnedOnEmptyDisplayflag. Note: this method is also duplicated inGenericSuggester—could be extracted along withnormalizeDisplayItem.src/gui/GenericSuggester/genericSuggester.ts (3)
46-81: Constructor initialization and alignment logic is correct.The normalization happens immediately on construction, and the length-mismatch alignment ensures
displayItemsanditemsremain synchronized. The warning check beforeopen()is appropriately placed.
84-88: Core fix for issue #586.By normalizing the display item before returning,
getItemTextnow always returns a string, preventing then.toLowerCase is not a functionerror during fuzzy matching.
130-143: Warning implementation is correct.Mirrors the
InputSuggesterimplementation. The one-time warning behavior correctly alerts developers to potential mapping issues without overwhelming logs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/gui/GenericSuggester/genericSuggester.ts (1)
68-73: Consider adding a clarifying comment.The alignment logic correctly ensures
displayItemsanditemsarrays have matching lengths, falling back to normalizing the item itself when a display value is missing. However, this re-mapping might not be immediately obvious to future maintainers.🔎 Suggested comment
+ // Ensure displayItems and items arrays are aligned in length. + // If mismatched, re-map to normalize any missing display values. if (this.displayItems.length !== this.items.length) { this.displayItems = this.items.map((item, index) => { const displayItem = this.displayItems[index]; return normalizeDisplayItem(displayItem ?? item); }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/gui/GenericSuggester/genericSuggester.ts(6 hunks)src/gui/InputSuggester/inputSuggester.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in
src/: core logic underengine/,services/, andutils/; Svelte UI insrc/gui; shared types insrc/types; settings entry insrc/quickAddSettingsTab.ts
Files:
src/gui/GenericSuggester/genericSuggester.tssrc/gui/InputSuggester/inputSuggester.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through theloggerutilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters ortests/obsidian-stub.ts
Files:
src/gui/GenericSuggester/genericSuggester.tssrc/gui/InputSuggester/inputSuggester.ts
src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for classes and Svelte components
Files:
src/gui/GenericSuggester/genericSuggester.tssrc/gui/InputSuggester/inputSuggester.ts
🧬 Code graph analysis (1)
src/gui/InputSuggester/inputSuggester.ts (2)
src/gui/suggesters/utils.ts (1)
normalizeDisplayItem(5-9)docs/static/scripts/EzImport.js (1)
item(157-160)
🔇 Additional comments (15)
src/gui/GenericSuggester/genericSuggester.ts (6)
3-4: LGTM!The imports are correct and necessary for the normalization and logging functionality.
15-17: LGTM!The new private fields are well-typed and follow naming conventions. The explicit initialization of
warnedOnEmptyDisplayis clear and correct.
34-42: LGTM!The constructor correctly normalizes display items during initialization, which addresses the root cause of the crash described in issue #586. The explicit field assignment is clearer than using parameter properties.
79-83: LGTM!The updated
getItemTextcorrectly retrieves the normalized display value for each item. The additional normalization on line 82 is defensive and handles the fallback case when a display item is missing or when the item is not found in the array.
107-114: LGTM!Excellent defensive programming. The try-catch ensures that errors in custom
renderItemfunctions won't break the suggester, and the fallback to default rendering maintains functionality. The logging follows the project's logger utilities convention.
128-141: LGTM!The one-time warning pattern is implemented correctly and efficiently. The use of
some()stops at the first empty string, and the guard prevents warning spam. The message clearly directs developers to check theirdisplayItemsmapping.src/gui/InputSuggester/inputSuggester.ts (9)
3-4: LGTM!The new imports support normalization and error handling. Logging through logger utilities aligns with the coding guidelines.
29-31: LGTM!Separating
displayItemsanditemsis the right design to support non-string display values while maintaining type safety for filtering.
50-51: LGTM!The normalization at line 57 directly addresses the "n.toLowerCase is not a function" error from issue #586 by ensuring all display items are strings before fuzzy matching occurs.
Also applies to: 56-57
86-88: LGTM!The type guard prevents invalid limit assignments. Good defensive practice.
99-99: LGTM!Calling
warnIfEmptyDisplay()after alignment ensures the final state is validated.
103-109: LGTM!The index-based lookup correctly maps items to their display values with proper fallback handling. The
indexOfcall is acceptable for typical suggester sizes; if performance becomes a concern with very large lists (1000+), consider caching the index mapping.
111-113: LGTM!Returning
this.itemscorrectly exposes the underlying selectable values rather than display strings.
161-164: LGTM!The error handling prevents custom
renderItemimplementations from crashing the suggester. The fallback to default rendering ensures graceful degradation.
181-194: LGTM!The one-time warning pattern is well-implemented. The message clearly directs developers to check their
displayItemsmapping.
|
🎉 This PR is included in version 2.10.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Testing
Fixes #586
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.