Conversation
Reviewer's GuideThis PR adds a new Pinyin service component by integrating a BootstrapBlazor.Pinyin project into the solution, defining an IPinyinService interface, implementing its logic in DefaultPinyinService with a static character‐to‐pinyin cache, and providing an AddPinyinService extension for DI registration. Class diagram for the new Pinyin serviceclassDiagram
class IPinyinService {
+bool IsChinese(string text)
+string GetFirstLetters(string text)
}
class DefaultPinyinService {
+bool IsChinese(string text)
+string GetFirstLetters(string text)
+static Dictionary<string, string> Cache
}
IPinyinService <|.. DefaultPinyinService
Class diagram for the service collection extensionclassDiagram
class ServiceCollectionExtension {
+IServiceCollection AddPinyinService(IServiceCollection services)
}
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 - here's some feedback:
- GetFirstLetters iterates the entire Cache for each character – consider building a reverse lookup (char→initial) once to improve performance.
- IsChinese only checks the basic CJK block (0x4E00–0x9FFF); consider including extended CJK ranges or documenting its intended scope.
- GetFirstLetters silently skips non-Chinese characters; you may want to explicitly handle or preserve them to avoid unexpected empty output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GetFirstLetters iterates the entire Cache for each character – consider building a reverse lookup (char→initial) once to improve performance.
- IsChinese only checks the basic CJK block (0x4E00–0x9FFF); consider including extended CJK ranges or documenting its intended scope.
- GetFirstLetters silently skips non-Chinese characters; you may want to explicitly handle or preserve them to avoid unexpected empty output.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Pinyin/Services/DefaultPinyinService.cs:13-25` </location>
<code_context>
+ public string GetFirstLetters(string text)
+ {
+ var ret = new StringBuilder();
+ foreach (var c in text)
+ {
+ foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture)))
+ {
+ ret.Append(item.Key.First());
</code_context>
<issue_to_address>
**suggestion (performance):** Nested loops over Cache for each character may impact performance.
Consider replacing the nested loop with a reverse lookup dictionary to improve performance, especially for long input strings.
```suggestion
// Reverse lookup dictionary for fast character-to-first-letter mapping
private static readonly Dictionary<char, char> CharToFirstLetter = BuildCharToFirstLetter();
public string GetFirstLetters(string text)
{
var ret = new StringBuilder();
foreach (var c in text)
{
if (CharToFirstLetter.TryGetValue(c, out var firstLetter))
{
ret.Append(firstLetter);
}
}
return ret.ToString().ToUpperInvariant();
}
private static Dictionary<char, char> BuildCharToFirstLetter()
{
var dict = new Dictionary<char, char>();
foreach (var kvp in Cache)
{
var firstLetter = kvp.Key.First();
foreach (var ch in kvp.Value)
{
if (!dict.ContainsKey(ch))
{
dict[ch] = firstLetter;
}
}
}
return dict;
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.Pinyin/Services/DefaultPinyinService.cs:27-28` </location>
<code_context>
+ return ret.ToString().ToUpperInvariant();
+ }
+
+ public static Dictionary<string, string> Cache = new()
+ {
+ { "a", "阿啊吖嗄腌锕" },
</code_context>
<issue_to_address>
**suggestion:** Consider making Cache readonly to prevent modification.
Marking Cache as readonly will ensure its contents cannot be reassigned, reinforcing its role as a static lookup table.
```suggestion
public static readonly Dictionary<string, string> Cache = new()
{
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+13
to
+25
| public string GetFirstLetters(string text) | ||
| { | ||
| var ret = new StringBuilder(); | ||
| foreach (var c in text) | ||
| { | ||
| foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture))) | ||
| { | ||
| ret.Append(item.Key.First()); | ||
| break; | ||
| } | ||
| } | ||
| return ret.ToString().ToUpperInvariant(); | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Nested loops over Cache for each character may impact performance.
Consider replacing the nested loop with a reverse lookup dictionary to improve performance, especially for long input strings.
Suggested change
| public string GetFirstLetters(string text) | |
| { | |
| var ret = new StringBuilder(); | |
| foreach (var c in text) | |
| { | |
| foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture))) | |
| { | |
| ret.Append(item.Key.First()); | |
| break; | |
| } | |
| } | |
| return ret.ToString().ToUpperInvariant(); | |
| } | |
| // Reverse lookup dictionary for fast character-to-first-letter mapping | |
| private static readonly Dictionary<char, char> CharToFirstLetter = BuildCharToFirstLetter(); | |
| public string GetFirstLetters(string text) | |
| { | |
| var ret = new StringBuilder(); | |
| foreach (var c in text) | |
| { | |
| if (CharToFirstLetter.TryGetValue(c, out var firstLetter)) | |
| { | |
| ret.Append(firstLetter); | |
| } | |
| } | |
| return ret.ToString().ToUpperInvariant(); | |
| } | |
| private static Dictionary<char, char> BuildCharToFirstLetter() | |
| { | |
| var dict = new Dictionary<char, char>(); | |
| foreach (var kvp in Cache) | |
| { | |
| var firstLetter = kvp.Key.First(); | |
| foreach (var ch in kvp.Value) | |
| { | |
| if (!dict.ContainsKey(ch)) | |
| { | |
| dict[ch] = firstLetter; | |
| } | |
| } | |
| } | |
| return dict; | |
| } |
Comment on lines
+27
to
+28
| public static Dictionary<string, string> Cache = new() | ||
| { |
There was a problem hiding this comment.
suggestion: Consider making Cache readonly to prevent modification.
Marking Cache as readonly will ensure its contents cannot be reassigned, reinforcing its role as a static lookup table.
Suggested change
| public static Dictionary<string, string> Cache = new() | |
| { | |
| public static readonly Dictionary<string, string> Cache = new() | |
| { |
There was a problem hiding this comment.
Pull Request Overview
Adds a new Pinyin service to provide Chinese character detection and initial-letter extraction, and wires it into DI. Also updates build/targeting to include .NET 10 and registers the new project in the solution.
- Introduces IPinyinService and DefaultPinyinService with a large pinyin mapping.
- Adds DI extension AddPinyinService.
- Updates targeting to include net10.0 and adjusts a build target condition.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/BootstrapBlazor.Pinyin/Services/IPinyinService.cs | Defines the Pinyin service interface. |
| src/components/BootstrapBlazor.Pinyin/Services/DefaultPinyinService.cs | Implements the service with a large static mapping and initial-letter logic. |
| src/components/BootstrapBlazor.Pinyin/Extensions/ServiceCollectionExtension.cs | Adds DI registration for IPinyinService. |
| src/components/BootstrapBlazor.Pinyin/BootstrapBlazor.Pinyin.csproj | New project file for the Pinyin component. |
| src/Frameworks.props | Adds net10.0 to TargetFrameworks. |
| src/Directory.Build.targets | Changes CopyLogo target condition to net10.0. |
| BootstrapBlazor.Extensions.slnx | Adds the Pinyin project to the solution. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+18
to
+22
| 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.
For polyphonic characters (e.g., 重/行/乐), this loop picks the first matching entry by dictionary insertion order, leading to inaccurate initials (e.g., 重 -> 'c' from 'chong' instead of 'z' from 'zhong'). Build a deterministic char->initial mapping with curated defaults or a frequency-based tie-breaker, or return multiple candidates; avoid relying on enumeration order.
Comment on lines
+15
to
+24
| var ret = new StringBuilder(); | ||
| foreach (var c in text) | ||
| { | ||
| foreach (var item in Cache.Where(item => item.Value.Contains(c, StringComparison.InvariantCulture))) | ||
| { | ||
| ret.Append(item.Key.First()); | ||
| break; | ||
| } | ||
| } | ||
| return ret.ToString().ToUpperInvariant(); |
There was a problem hiding this comment.
The current algorithm is O(n*m): for each input char it scans all 400+ cache entries with string.Contains checks. Precompute a Dictionary<char, char> (or char->string) once from Cache and use O(1) lookups in GetFirstLetters.
| 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.
Use a char-based check without culture overhead. Prefer item.Value.Contains(c) or item.Value.IndexOf(c) >= 0 (Ordinal), as culture-aware comparisons are unnecessary for Chinese characters and add overhead.
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))) |
| return ret.ToString().ToUpperInvariant(); | ||
| } | ||
|
|
||
| public static Dictionary<string, string> Cache = new() |
There was a problem hiding this comment.
Exposing a public mutable static dictionary invites external mutation and thread-safety issues. Make it private and readonly (e.g., private static readonly IReadOnlyDictionary<string, string> Cache) to protect invariants.
Suggested change
| public static Dictionary<string, string> Cache = new() | |
| private static readonly Dictionary<string, string> Cache = new() |
|
|
||
| class DefaultPinyinService : IPinyinService | ||
| { | ||
| public bool IsChinese(string text) => text.Any(i => i >= 0x4E00 && i <= 0x9FFF); |
There was a problem hiding this comment.
This only matches the basic CJK block and will miss many Chinese characters (e.g., Extension A and CJK Compatibility Ideographs). Consider extending the ranges: (0x3400–0x4DBF), (0x4E00–0x9FFF), and (0xF900–0xFAFF). Note: Extension B+ are surrogate pairs and can't be handled by a single char.
Suggested change
| public bool IsChinese(string text) => text.Any(i => i >= 0x4E00 && i <= 0x9FFF); | |
| public bool IsChinese(string text) => | |
| text.Any(i => | |
| (i >= 0x3400 && i <= 0x4DBF) || // CJK Unified Ideographs Extension A | |
| (i >= 0x4E00 && i <= 0x9FFF) || // CJK Unified Ideographs | |
| (i >= 0xF900 && i <= 0xFAFF) // CJK Compatibility Ideographs | |
| ); |
| <Project> | ||
|
|
||
| <Target Name="CopyLogo" AfterTargets="Build" Condition="'$(TargetFramework)' == 'net9.0'"> | ||
| <Target Name="CopyLogo" AfterTargets="Build" Condition="'$(TargetFramework)' == 'net10.0'"> |
There was a problem hiding this comment.
Changing the condition to only net10.0 stops copying the logo for net9.0 builds. If both are intended, update the condition to include both frameworks, e.g.: Condition="'$(TargetFramework)' == 'net9.0' Or '$(TargetFramework)' == 'net10.0'".
Suggested change
| <Target Name="CopyLogo" AfterTargets="Build" Condition="'$(TargetFramework)' == 'net10.0'"> | |
| <Target Name="CopyLogo" AfterTargets="Build" Condition="'$(TargetFramework)' == 'net9.0' Or '$(TargetFramework)' == 'net10.0'"> |
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 #615
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a new Pinyin service component for detecting Chinese text and extracting Pinyin initials, along with DI registration and integrate it into the solution build.
New Features:
Build: