Conversation
Reviewer's GuideAdds ShowSearch parameter to SelectCity enabling optional search/filtering with pinyin support via a new PinYinService and JS interop, and updates related CSS and templates for consistent dropdown styling. Sequence diagram for city search interaction in SelectCitysequenceDiagram
actor User
participant SelectCityComponent
participant JSInterop
participant PinYinService
User->>SelectCityComponent: Type in search input
SelectCityComponent->>JSInterop: TriggerSearch(text)
JSInterop->>SelectCityComponent: JSInvokable TriggerSearch(text)
SelectCityComponent->>PinYinService: GetFirstLetters(text)
PinYinService-->>SelectCityComponent: pinyin result
SelectCityComponent->>SelectCityComponent: Filter provinces/cities
SelectCityComponent->>User: Display filtered results
Class diagram for new and updated types in SelectCityclassDiagram
class SelectCity {
+bool ShowSearch
+string? SearchIcon
+bool AutoClose
-string? _searchText
+void TriggerSearch(string v)
+HashSet<ProvinceItem> GenerateProvincePinYin()
+HashSet<CityItem> GenerateCityPinYin(string provinceName)
+HashSet<string> GetProvinces()
+HashSet<string> GetCities(string provinceName)
+bool IsChinese(string text)
}
class PinYinService {
+string GetFirstLetters(string text)
+Dictionary<string, string> Cache
}
class ProvinceItem {
+string Name
+string PinYin
+HashSet<CityItem> Cities
}
class CityItem {
+string Name
+string PinYin
}
SelectCity --> PinYinService : uses
SelectCity --> ProvinceItem : uses
ProvinceItem --> CityItem : contains
SelectCity --> CityItem : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:66` </location>
<code_context>
+ /// <summary>
+ /// <inheritdoc/>
+ /// </summary>
+ protected override void OnParametersSet()
+ {
+ base.OnParametersSet();
</code_context>
<issue_to_address>
**suggestion:** OnParametersSet sets SearchIcon only if null.
Using string.IsNullOrWhiteSpace(SearchIcon) will ensure the fallback icon is used when SearchIcon is empty or whitespace.
```suggestion
if (string.IsNullOrWhiteSpace(SearchIcon))
{
SearchIcon = IconTheme.GetIconByKey(ComponentIcons.SelectSearchIcon);
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.js:34` </location>
<code_context>
+ });
+ }
+
+ Data.set(id, { el, popover, searchInput, search });
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Dispose function does not check for region existence.
Add a null check for region in dispose() to prevent errors when called with an invalid id.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.Region/Services/PinYinService.cs:11` </location>
<code_context>
+
+static class PinYinService
+{
+ public static string GetFirstLetters(string text)
+ {
+ var ret = new StringBuilder();
</code_context>
<issue_to_address>
**issue (complexity):** Consider pre-computing a reverse lookup dictionary and loading the mapping data from a JSON file to reduce per-character lookup complexity and keep the source file small.
Consider pre-computing a reverse lookup so you only scan your huge map once, rather than on every character, and move the raw data into a JSON/resource file.
Example – invert the map at startup and load the data from a JSON file:
```csharp
// PinYinService.cs
using System.IO;
using System.Text.Json;
static class PinYinService
{
// holds char → first-letter
private static readonly Dictionary<char, char> _reverseMap;
static PinYinService()
{
// load your large map from an external pinyin.json
var json = File.ReadAllText("pinyin.json");
var cache = JsonSerializer.Deserialize<Dictionary<string, string>>(json);
_reverseMap = cache
.SelectMany(kv => kv.Value.Select(ch => (ch, letter: kv.Key[0])))
.ToDictionary(x => x.ch, x => x.letter);
}
public static string GetFirstLetters(string text)
{
var sb = new StringBuilder(text.Length);
foreach (var c in text)
{
if (_reverseMap.TryGetValue(c, out var letter))
sb.Append(letter);
}
return sb.ToString().ToUpperInvariant();
}
}
```
pinyin.json (partial):
```json
{
"a": "阿啊吖嗄腌锕",
"ai": "爱埃碍矮挨唉哎哀皑癌蔼艾隘捱嗳嗌嫒瑷暧砹锿霭",
…
}
```
Steps:
1. Move the inline `Cache = new Dictionary<string,string>` block into `pinyin.json`.
2. In the static constructor, read and deserialize that file.
3. Build `_reverseMap` once by flattening the JSON map.
4. Update `GetFirstLetters` to do a single `TryGetValue` per character.
5. Remove the old `Cache.Where(…)` loop.
This preserves behavior but cuts the per-character lookup from O(N) to O(1), and keeps your source file tiny.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
Add search capability to SelectCity, including Pinyin-first-letter filtering, and scope dropdown styles to avoid global collisions.
- Add ShowSearch and AutoClose parameters to SelectCity with JS interop to filter cities by text or Pinyin initials.
- Introduce PinYinService for first-letter generation and CityItem/ProvinceItem models to support filtering.
- Scope dropdown CSS classes for Region/Province/City components.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/BootstrapBlazor.Region/Components/SelectCity.razor | Adds search UI and rendering changes for city lists. |
| src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs | Adds search logic, Pinyin filtering, AutoClose behavior, and JS init. |
| src/components/BootstrapBlazor.Region/Components/SelectCity.razor.js | Adds client-side search wiring, popover management, and cleanup helpers. |
| src/components/BootstrapBlazor.Region/Components/SelectCity.razor.css | Adds styles for search UI and scoping via .dropdown-menu-city. |
| src/components/BootstrapBlazor.Region/Components/SelectProvince.razor | Scopes dropdown menu class name. |
| src/components/BootstrapBlazor.Region/Components/SelectProvince.razor.css | Scopes dropdown styles via .dropdown-menu-province and updates selectors. |
| src/components/BootstrapBlazor.Region/Components/SelectRegion.razor | Scopes dropdown menu class name. |
| src/components/BootstrapBlazor.Region/Components/SelectRegion.razor.css | Scopes dropdown styles via .dropdown-menu-region and updates selectors. |
| src/components/BootstrapBlazor.Region/Services/PinYinService.cs | New service for computing first-letter Pinyin from Chinese characters. |
| src/components/BootstrapBlazor.Region/Data/CityItem.cs | New record struct for city name and Pinyin. |
| src/components/BootstrapBlazor.Region/Data/ProvinceItem.cs | New record struct for province name, Pinyin, and city set. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var ret = new StringBuilder(); | ||
| foreach (var c in text) | ||
| { | ||
| foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture))) |
There was a problem hiding this comment.
string.Contains(char, StringComparison) is not a valid overload and will not compile. Replace with item.Value.Contains(c) (or item.Value.AsSpan().IndexOf(c) >= 0) since comparison culture is irrelevant for a single char.
Suggested change
| foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture))) | |
| foreach (var item in Cache.Where(item => item.Value.Contains(c))) |
Comment on lines
+21
to
+23
| Input.composition(searchInput, async v => { | ||
| await invoke.invokeMethodAsync(options.triggerSearch, v); | ||
| }); |
There was a problem hiding this comment.
Property name casing mismatch: options.triggerSearch is undefined because the .NET side passes TriggerSearch. Use options.TriggerSearch to successfully invoke the .NET method.
Comment on lines
+29
to
+31
| searchInput.value = ''; | ||
| await invoke.invokeMethodAsync(options.triggerSearch, ''); | ||
| }); |
There was a problem hiding this comment.
Same casing issue as above; change options.triggerSearch to options.TriggerSearch so the clear action calls the correct .NET method.
Comment on lines
+38
to
+41
| const region = Data.get(id) | ||
| const { popover } = region; | ||
| if (popover) { | ||
| popover.hide(); |
There was a problem hiding this comment.
Potential runtime error if Data.get(id) returns undefined (e.g., init didn't run). Guard before destructuring: const region = Data.get(id); if (!region) return; const { popover } = region;.
Comment on lines
+45
to
+58
| export function dispose(id) { | ||
| const region = Data.get(id) | ||
| Data.remove(id) | ||
|
|
||
| const { popover, searchInput, search } = region; | ||
| if (popover) { | ||
| Popover.dispose(popover); | ||
| } | ||
| if (searchInput) { | ||
| Input.dispose(searchInput); | ||
| } | ||
| if (search) { | ||
| EventHandler.off(search, 'click'); | ||
| } |
There was a problem hiding this comment.
If region is undefined, destructuring throws. Add a null check before Data.remove and destructuring: const region = Data.get(id); if (!region) return;.
| return ret.ToString().ToUpperInvariant(); | ||
| } | ||
|
|
||
| public static Dictionary<string, string> Cache = new() |
There was a problem hiding this comment.
Expose the mapping as private static readonly to avoid external mutation and reduce API surface. For example: private static readonly Dictionary<string, string> Cache = new();
Suggested change
| public static Dictionary<string, string> Cache = new() | |
| private static readonly Dictionary<string, string> Cache = new() |
Comment on lines
+14
to
+21
| foreach (var c in text) | ||
| { | ||
| foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture))) | ||
| { | ||
| ret.Append(item.Key.First()); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
GetFirstLetters does an O(n × m) scan across the entire Cache for each character. Consider building a one-time Dictionary<char, char> lookup (char -> first-letter) at startup and then do O(1) lookups inside the loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #609
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable searchable city selection by adding ShowSearch, SearchIcon, and AutoClose parameters, integrating pinyin filtering via a new PinYinService, and refactoring markup, CSS, and JS interop accordingly
New Features:
Enhancements:
Chores: