fix(web): kb recall top_k max#1297
Merged
Merged
Conversation
Contributor
审查者指南调整网络图中图节点标签的渲染方式以提升可读性，并将知识库召回测试的 top_k 输入上限限制为 100，同时更新校验逻辑和 UI 提示。 文件级变更
使用技巧和命令与 Sourcery 交互
自定义你的使用体验访问你的 dashboard 以：
获取帮助Original review guide in EnglishReviewer's GuideAdjusts graph node label rendering for better readability in the network chart and caps the knowledge base recall test top_k input to 100, updating both validation and UI hints. File-Level Changes
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}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体性的反馈：
GraphNetworkChart中的自动换行逻辑使用了若干“魔法数字”（例如0.25、1.2、0.55、20）——建议把这些提取成具名常量，以便更容易理解和调整尺寸相关行为。- 目前基于字符的宽度估算（
charWidth = fontSize * 0.55并按每个字符拆分）在非拉丁字符集或 emoji 上可能表现不佳；建议使用真实的文本测量方式（例如getComputedTextLength），或者至少在可能的情况下按单词/字素（grapheme）拆分。 - 针对每个节点的
.each文本布局处理会在每次渲染时运行，在图很大时可能会变得昂贵；可以考虑对布局做缓存，或者只对大小/名称发生变化的节点重新计算。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- The text-wrapping logic in `GraphNetworkChart` uses several magic numbers (e.g., `0.25`, `1.2`, `0.55`, `20`)—consider extracting these into named constants to make the sizing behavior easier to understand and tune.
- The character-based width approximation (`charWidth = fontSize * 0.55` and splitting on every character) may behave poorly for non-Latin scripts or emoji; consider using actual text measurement (e.g., `getComputedTextLength`) or at least splitting on words/graphemes where possible.
- The per-node `.each` text layout work runs for every render and may become expensive for large graphs; consider memoizing layout or limiting recomputation to nodes whose size/name changed.
## Individual Comments
### Comment 1
<location path="web/src/components/Charts/GraphNetworkChart.tsx" line_range="330-331" />
<code_context>
.attr('text-anchor', 'middle')
- .attr('font-size', '10px')
+ .attr('dominant-baseline', 'middle')
+ .attr('font-size', d => {
+ const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25))
+ return `${fontSize}px`
+ })
</code_context>
<issue_to_address>
**suggestion:** Avoid recomputing fontSize in both the attribute and the wrapping logic to keep them in sync and slightly reduce work.
The `fontSize` logic here is duplicated in the `.each` handler below, which risks the two paths diverging and making wrapping vs. rendered size hard to reason about. Please refactor to a single source of truth (e.g., compute once in the attr callback and store on the datum, or read the computed value from the element in `.each`).
Suggested implementation:
```typescript
nodeSel.append('text')
.attr('x', 0)
.attr('y', 0)
.attr('text-anchor', 'middle')
.attr('dominant-baseline', 'middle')
.attr('font-size', d => {
const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25))
// store on datum so wrapping logic can reuse without recomputing
;(d as any).labelFontSize = fontSize
return `${fontSize}px`
})
```
Somewhere below this snippet, you likely have a `.each` handler that performs text wrapping and currently recomputes the font size using the same `Math.max(6, Math.min(12, d.symbolSize * 0.25))` logic.
Refactor that handler to read the value we just stored instead of recomputing it, e.g.:
```ts
selection
.each(function (d) {
const fontSize =
(d as any).labelFontSize ??
Math.max(6, Math.min(12, d.symbolSize * 0.25)) // optional fallback if needed
// use `fontSize` for wrapping logic instead of recomputing
})
```
You should:
1. Remove the duplicated `Math.max(6, Math.min(12, d.symbolSize * 0.25))` expression in the `.each` block.
2. Replace it with reading `(d as any).labelFontSize` (or use a stronger type if your node datum type can be extended).
3. Optionally keep the fallback to preserve behavior for any text elements that might not have gone through the `nodeSel.append('text')` path.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的 Review。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The text-wrapping logic in
GraphNetworkChartuses several magic numbers (e.g.,0.25,1.2,0.55,20)—consider extracting these into named constants to make the sizing behavior easier to understand and tune. - The character-based width approximation (
charWidth = fontSize * 0.55and splitting on every character) may behave poorly for non-Latin scripts or emoji; consider using actual text measurement (e.g.,getComputedTextLength) or at least splitting on words/graphemes where possible. - The per-node
.eachtext layout work runs for every render and may become expensive for large graphs; consider memoizing layout or limiting recomputation to nodes whose size/name changed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The text-wrapping logic in `GraphNetworkChart` uses several magic numbers (e.g., `0.25`, `1.2`, `0.55`, `20`)—consider extracting these into named constants to make the sizing behavior easier to understand and tune.
- The character-based width approximation (`charWidth = fontSize * 0.55` and splitting on every character) may behave poorly for non-Latin scripts or emoji; consider using actual text measurement (e.g., `getComputedTextLength`) or at least splitting on words/graphemes where possible.
- The per-node `.each` text layout work runs for every render and may become expensive for large graphs; consider memoizing layout or limiting recomputation to nodes whose size/name changed.
## Individual Comments
### Comment 1
<location path="web/src/components/Charts/GraphNetworkChart.tsx" line_range="330-331" />
<code_context>
.attr('text-anchor', 'middle')
- .attr('font-size', '10px')
+ .attr('dominant-baseline', 'middle')
+ .attr('font-size', d => {
+ const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25))
+ return `${fontSize}px`
+ })
</code_context>
<issue_to_address>
**suggestion:** Avoid recomputing fontSize in both the attribute and the wrapping logic to keep them in sync and slightly reduce work.
The `fontSize` logic here is duplicated in the `.each` handler below, which risks the two paths diverging and making wrapping vs. rendered size hard to reason about. Please refactor to a single source of truth (e.g., compute once in the attr callback and store on the datum, or read the computed value from the element in `.each`).
Suggested implementation:
```typescript
nodeSel.append('text')
.attr('x', 0)
.attr('y', 0)
.attr('text-anchor', 'middle')
.attr('dominant-baseline', 'middle')
.attr('font-size', d => {
const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25))
// store on datum so wrapping logic can reuse without recomputing
;(d as any).labelFontSize = fontSize
return `${fontSize}px`
})
```
Somewhere below this snippet, you likely have a `.each` handler that performs text wrapping and currently recomputes the font size using the same `Math.max(6, Math.min(12, d.symbolSize * 0.25))` logic.
Refactor that handler to read the value we just stored instead of recomputing it, e.g.:
```ts
selection
.each(function (d) {
const fontSize =
(d as any).labelFontSize ??
Math.max(6, Math.min(12, d.symbolSize * 0.25)) // optional fallback if needed
// use `fontSize` for wrapping logic instead of recomputing
})
```
You should:
1. Remove the duplicated `Math.max(6, Math.min(12, d.symbolSize * 0.25))` expression in the `.each` block.
2. Replace it with reading `(d as any).labelFontSize` (or use a stronger type if your node datum type can be extended).
3. Optionally keep the fallback to preserve behavior for any text elements that might not have gone through the `nodeSel.append('text')` path.
</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
+330
to
+331
| .attr('font-size', d => { | ||
| const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25)) |
Contributor
There was a problem hiding this comment.
suggestion: 避免在属性设置和换行逻辑中重复计算 fontSize，这样既能保持两者一致，又能稍微减少一些计算工作。
这里的 fontSize 逻辑在下面的 .each 处理函数中又写了一遍，这会带来两个路径行为不一致的风险，从而让“换行效果”和“实际渲染的字号”变得难以推理。请重构为单一数据源（例如：在 attr 回调里计算一次并存到 datum 上，或者在 .each 中从元素上读取已计算好的值）。
建议实现方式：
nodeSel.append('text')
.attr('x', 0)
.attr('y', 0)
.attr('text-anchor', 'middle')
.attr('dominant-baseline', 'middle')
.attr('font-size', d => {
const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25))
// store on datum so wrapping logic can reuse without recomputing
;(d as any).labelFontSize = fontSize
return `${fontSize}px`
})在这段代码下面，你很可能有一个负责文本换行的 .each 处理函数，目前会使用相同的 Math.max(6, Math.min(12, d.symbolSize * 0.25)) 逻辑再次计算字号。
请重构该处理函数，让它读取我们刚才存储的值，而不是重新计算，例如：
selection
.each(function (d) {
const fontSize =
(d as any).labelFontSize ??
Math.max(6, Math.min(12, d.symbolSize * 0.25)) // optional fallback if needed
// use `fontSize` for wrapping logic instead of recomputing
})你应该：
- 删除
.each代码块中重复的Math.max(6, Math.min(12, d.symbolSize * 0.25))表达式。 - 改为读取
(d as any).labelFontSize（如果你的节点数据类型可以扩展，也可以使用更强类型的方式）。 - 可以保留这个回退逻辑，以保证那些没有走过
nodeSel.append('text')路径的文本元素行为不变。
Original comment in English
suggestion: Avoid recomputing fontSize in both the attribute and the wrapping logic to keep them in sync and slightly reduce work.
The fontSize logic here is duplicated in the .each handler below, which risks the two paths diverging and making wrapping vs. rendered size hard to reason about. Please refactor to a single source of truth (e.g., compute once in the attr callback and store on the datum, or read the computed value from the element in .each).
Suggested implementation:
nodeSel.append('text')
.attr('x', 0)
.attr('y', 0)
.attr('text-anchor', 'middle')
.attr('dominant-baseline', 'middle')
.attr('font-size', d => {
const fontSize = Math.max(6, Math.min(12, d.symbolSize * 0.25))
// store on datum so wrapping logic can reuse without recomputing
;(d as any).labelFontSize = fontSize
return `${fontSize}px`
})Somewhere below this snippet, you likely have a .each handler that performs text wrapping and currently recomputes the font size using the same Math.max(6, Math.min(12, d.symbolSize * 0.25)) logic.
Refactor that handler to read the value we just stored instead of recomputing it, e.g.:
selection
.each(function (d) {
const fontSize =
(d as any).labelFontSize ??
Math.max(6, Math.min(12, d.symbolSize * 0.25)) // optional fallback if needed
// use `fontSize` for wrapping logic instead of recomputing
})You should:
- Remove the duplicated
Math.max(6, Math.min(12, d.symbolSize * 0.25))expression in the.eachblock. - Replace it with reading
(d as any).labelFontSize(or use a stronger type if your node datum type can be extended). - Optionally keep the fallback to preserve behavior for any text elements that might not have gone through the
nodeSel.append('text')path.
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
1 participant
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.
Summary by Sourcery
改进图形节点标签渲染，并收紧知识库召回参数限制。
Bug 修复：
top_k输入最大值从 1024 限制为 100，以符合预期限制。改进：
Original summary in English
Summary by Sourcery
Improve graph node label rendering and tighten knowledge base recall parameter limits.
Bug Fixes:
Enhancements: