fix(public-status): prevent duplicate group slugs#1102
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
📝 WalkthroughWalkthrough为公开状态页面引入重复 public-group slug 检测与处理：新增多语言提示、表单提交时校验并标记冲突、加载器预先追踪已用 slug、以及在配置工具中新增确定性唯一 slug 生成与冲突解决策略，同时增加相应单元测试覆盖。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 PR introduces robust slug management for public status pages, including support for non-ASCII characters via stable hashing and a validation mechanism to prevent duplicate slugs. Key changes include the implementation of a collision-resistant slugification utility, UI updates to highlight and navigate to conflicting inputs in the settings form, and logic to ensure unique default slugs are generated during data loading. Feedback focuses on improving the comprehensive detection of all slug collisions in the UI, adopting a two-pass approach in the loader to prevent potential collisions between generated and custom slugs, and refactoring duplicated logic in the slug generation utilities.
| function findDuplicateSlugError( | ||
| groups: PublicStatusSettingsFormGroup[] | ||
| ): DuplicateSlugErrorState | null { | ||
| const seenGroupNameBySlug = new Map<string, string>(); | ||
|
|
||
| for (const group of groups) { | ||
| if (!group.enabled || normalizePublicStatusModels(group.publicModels).length === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| const normalizedSlug = slugifyPublicGroup(group.publicGroupSlug.trim() || group.groupName); | ||
| if (!normalizedSlug) { | ||
| continue; | ||
| } | ||
|
|
||
| const existingGroupName = seenGroupNameBySlug.get(normalizedSlug); | ||
| if (existingGroupName) { | ||
| return { | ||
| slug: normalizedSlug, | ||
| groupNames: [existingGroupName, group.groupName], | ||
| }; | ||
| } | ||
|
|
||
| seenGroupNameBySlug.set(normalizedSlug, group.groupName); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The findDuplicateSlugError function returns as soon as it identifies the first collision between two groups. If three or more groups share the same slug, only the first two will be highlighted in the UI.
Consider collecting all group names that share a conflicting slug to provide a more comprehensive error state and better UX.
| const usedDefaultSlugs = new Set<string>(); | ||
|
|
||
| return { | ||
| initialWindowHours: settings.publicStatusWindowHours, | ||
| initialAggregationIntervalMinutes: settings.publicStatusAggregationIntervalMinutes, | ||
| initialGroups: groups.map((group) => { | ||
| const parsed = parsePublicStatusDescription(group.description); | ||
| const publicGroupSlug = | ||
| parsed.publicStatus?.publicGroupSlug ?? | ||
| createUniquePublicGroupSlug(group.name, usedDefaultSlugs); | ||
| if (parsed.publicStatus?.publicGroupSlug) { | ||
| const normalizedConfiguredSlug = slugifyPublicGroup(parsed.publicStatus.publicGroupSlug); | ||
| if (normalizedConfiguredSlug) { | ||
| usedDefaultSlugs.add(normalizedConfiguredSlug); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current single-pass approach for generating default slugs can lead to collisions if a group appearing later in the list has an explicit custom slug that matches a generated slug for an earlier group.
Collecting all explicit custom slugs in a first pass ensures that generated defaults for other groups are truly unique and avoids unnecessary validation errors in the initial form state.
const usedDefaultSlugs = new Set<string>();
for (const group of groups) {
const parsed = parsePublicStatusDescription(group.description);
const slug = parsed.publicStatus?.publicGroupSlug;
if (slug) {
const normalized = slugifyPublicGroup(slug);
if (normalized) usedDefaultSlugs.add(normalized);
}
}
return {
initialWindowHours: settings.publicStatusWindowHours,
initialAggregationIntervalMinutes: settings.publicStatusAggregationIntervalMinutes,
initialGroups: groups.map((group) => {
const parsed = parsePublicStatusDescription(group.description);
const publicGroupSlug =
parsed.publicStatus?.publicGroupSlug ??
createUniquePublicGroupSlug(group.name, usedDefaultSlugs);| export function createUniquePublicGroupSlug(groupName: string, usedSlugs: Set<string>): string { | ||
| const baseSlug = normalizePublicGroupSlug(groupName); | ||
| if (!usedSlugs.has(baseSlug)) { | ||
| usedSlugs.add(baseSlug); | ||
| return baseSlug; | ||
| } | ||
|
|
||
| let counter = 1; | ||
| let candidate = baseSlug; | ||
| while (usedSlugs.has(candidate)) { | ||
| const suffixSource = counter === 1 ? groupName : `${groupName}-${counter}`; | ||
| candidate = appendStablePublicGroupSlugSuffix( | ||
| baseSlug || PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, | ||
| createStablePublicGroupSlugSuffix(suffixSource) | ||
| ); | ||
| counter += 1; | ||
| } | ||
|
|
||
| usedSlugs.add(candidate); | ||
| return candidate; | ||
| } |
There was a problem hiding this comment.
The logic for resolving slug collisions is duplicated between createUniquePublicGroupSlug and createCollisionPublicGroupSlug. Refactoring createUniquePublicGroupSlug to use the helper function improves maintainability and reduces code duplication.
export function createUniquePublicGroupSlug(groupName: string, usedSlugs: Set<string>): string {
const baseSlug = normalizePublicGroupSlug(groupName);
const uniqueSlug = createCollisionPublicGroupSlug(baseSlug, groupName, usedSlugs);
usedSlugs.add(uniqueSlug);
return uniqueSlug;
}
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| initialGroups: groups.map((group) => { | ||
| const parsed = parsePublicStatusDescription(group.description); | ||
| const publicGroupSlug = | ||
| parsed.publicStatus?.publicGroupSlug ?? | ||
| createUniquePublicGroupSlug(group.name, usedDefaultSlugs); | ||
| if (parsed.publicStatus?.publicGroupSlug) { | ||
| const normalizedConfiguredSlug = slugifyPublicGroup(parsed.publicStatus.publicGroupSlug); | ||
| if (normalizedConfiguredSlug) { | ||
| usedDefaultSlugs.add(normalizedConfiguredSlug); | ||
| } | ||
| } |
There was a problem hiding this comment.
Because explicit slugs are added to usedDefaultSlugs only after their group is processed, a group without a stored slug that is iterated first can grab the same base slug as an explicit slug on a later group. For example, if Group A has no slug and its name normalizes to "cc", it receives "cc" and usedDefaultSlugs becomes {"cc"}. Group B with stored slug "cc" then also returns "cc" as publicGroupSlug, leaving the form in an already-conflicting initial state that blocks saving.
A two-pass approach would prevent this: collect all normalized explicit slugs into usedDefaultSlugs first, then run the auto-generation pass.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/status-page/loader.ts
Line: 22-32
Comment:
**Single-pass ordering can produce initial slug collisions**
Because explicit slugs are added to `usedDefaultSlugs` only after their group is processed, a group without a stored slug that is iterated first can grab the same base slug as an explicit slug on a later group. For example, if Group A has no slug and its name normalizes to `"cc"`, it receives `"cc"` and `usedDefaultSlugs` becomes `{"cc"}`. Group B with stored slug `"cc"` then also returns `"cc"` as `publicGroupSlug`, leaving the form in an already-conflicting initial state that blocks saving.
A two-pass approach would prevent this: collect all normalized explicit slugs into `usedDefaultSlugs` first, then run the auto-generation pass.
How can I resolve this? If you propose a fix, please make it concise.| function createCollisionPublicGroupSlug( | ||
| baseSlug: string, | ||
| groupName: string, | ||
| usedSlugs: Set<string> | ||
| ): string { | ||
| let counter = 1; | ||
| let candidate = baseSlug; | ||
| while (usedSlugs.has(candidate)) { | ||
| const suffixSource = counter === 1 ? groupName : `${groupName}-${counter}`; | ||
| candidate = appendStablePublicGroupSlugSuffix( | ||
| baseSlug || PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, | ||
| createStablePublicGroupSlugSuffix(suffixSource) | ||
| ); | ||
| counter += 1; | ||
| } | ||
|
|
||
| return candidate; | ||
| } |
There was a problem hiding this comment.
createCollisionPublicGroupSlug duplicates createUniquePublicGroupSlug's loop body
The while-loop in createCollisionPublicGroupSlug is byte-for-byte identical to the one inside createUniquePublicGroupSlug. The only difference is that createUniquePublicGroupSlug also handles the no-collision fast path and calls usedSlugs.add. createCollisionPublicGroupSlug could delegate to a shared private helper, eliminating the duplicate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/public-status/config.ts
Line: 227-244
Comment:
**`createCollisionPublicGroupSlug` duplicates `createUniquePublicGroupSlug`'s loop body**
The while-loop in `createCollisionPublicGroupSlug` is byte-for-byte identical to the one inside `createUniquePublicGroupSlug`. The only difference is that `createUniquePublicGroupSlug` also handles the no-collision fast path and calls `usedSlugs.add`. `createCollisionPublicGroupSlug` could delegate to a shared private helper, eliminating the duplicate.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 275c2fb19d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const publicGroupSlug = | ||
| parsed.publicStatus?.publicGroupSlug ?? | ||
| createUniquePublicGroupSlug(group.name, usedDefaultSlugs); | ||
| if (parsed.publicStatus?.publicGroupSlug) { | ||
| const normalizedConfiguredSlug = slugifyPublicGroup(parsed.publicStatus.publicGroupSlug); |
There was a problem hiding this comment.
This loop generates a default slug before adding the current group's configured slug into usedDefaultSlugs, so ordering can still produce duplicates: an earlier no-slug group may get alpha, and a later group with stored slug alpha is only registered afterward. That leaves duplicate normalized slugs in the loaded form and defeats the “unique default slug” hydration goal for mixed legacy data. Seed usedDefaultSlugs with configured slugs first (or reserve the current configured slug before calling createUniquePublicGroupSlug).
Useful? React with 👍 / 👎.
| const normalizedSlug = slugifyPublicGroup(group.publicGroupSlug.trim() || group.groupName); | ||
| if (!normalizedSlug) { | ||
| continue; |
There was a problem hiding this comment.
The client-side duplicate preflight drops groups when slugifyPublicGroup(...) returns an empty string, but server-side normalization falls back to groupName when a provided slug normalizes empty. In cases like punctuation-only slugs ("!!!") across groups whose names normalize to the same slug, the UI will allow submit while collectEnabledPublicStatusGroups still throws on save. Use the same fallback normalization as the backend instead of continue on empty normalizedSlug.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/unit/public-status/config-publisher.test.ts (1)
280-328: 冲突 slug 容错测试 LGTM；可选：补充内部快照断言。此处仅断言 public snapshot 中两个分组的 slug 映射，建议同时对
mockPublishInternalPublicStatusConfigSnapshot做断言，确保被加后缀的cc逆向在内部快照里仍然以sourceGroupName: "cc逆向"与生成的新 slug 正确关联，避免后续指标聚合错位。♻️ 可选追加断言示例
expect(result.written).toBe(true); + expect(mockPublishInternalPublicStatusConfigSnapshot).toHaveBeenCalledWith( + expect.objectContaining({ + snapshot: expect.objectContaining({ + groups: expect.arrayContaining([ + expect.objectContaining({ sourceGroupName: "cc特价", slug: "cc" }), + expect.objectContaining({ + sourceGroupName: "cc逆向", + slug: expect.stringMatching(/^cc-[a-z0-9]{6}$/), + }), + ]), + }), + }) + ); expect(mockPublishPublicStatusConfigSnapshot).toHaveBeenCalledWith(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/public-status/config-publisher.test.ts` around lines 280 - 328, Add an assertion that verifies mockPublishInternalPublicStatusConfigSnapshot is called with an internal snapshot mapping the original sourceGroupName to the generated slug: after calling publishCurrentPublicStatusConfigProjection, assert mockPublishInternalPublicStatusConfigSnapshot was invoked with an objectContaining({ snapshot: objectContaining({ groups: arrayContaining([ objectContaining({ sourceGroupName: "cc逆向", slug: expect.stringMatching(/^cc-[a-z0-9]{6}$/) }) ]) }) }) so the internally-published snapshot preserves the original group name and the same generated slug used in mockPublishPublicStatusConfigSnapshot.src/app/[locale]/settings/status-page/loader.ts (1)
23-32: 可选：抽取本地变量减少对parsed.publicStatus?.publicGroupSlug的重复读取逻辑本身正确（已配置的 slug 走原值并把归一化结果计入
usedDefaultSlugs，未配置则用createUniquePublicGroupSlug生成），但parsed.publicStatus?.publicGroupSlug被读取了三次，且??（nullish）与if (...)（truthy）两个分支的判定语义存在细微不一致（实践中sanitizeString已保证非空字符串，所以无 bug）。可抽个本地常量让分支结构更清晰：♻️ 建议改写
- const publicGroupSlug = - parsed.publicStatus?.publicGroupSlug ?? - createUniquePublicGroupSlug(group.name, usedDefaultSlugs); - if (parsed.publicStatus?.publicGroupSlug) { - const normalizedConfiguredSlug = slugifyPublicGroup(parsed.publicStatus.publicGroupSlug); - if (normalizedConfiguredSlug) { - usedDefaultSlugs.add(normalizedConfiguredSlug); - } - } + const configuredSlug = parsed.publicStatus?.publicGroupSlug; + let publicGroupSlug: string; + if (configuredSlug) { + publicGroupSlug = configuredSlug; + const normalizedConfiguredSlug = slugifyPublicGroup(configuredSlug); + if (normalizedConfiguredSlug) { + usedDefaultSlugs.add(normalizedConfiguredSlug); + } + } else { + publicGroupSlug = createUniquePublicGroupSlug(group.name, usedDefaultSlugs); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/status-page/loader.ts around lines 23 - 32, Extract parsed.publicStatus?.publicGroupSlug into a local constant (e.g., const configuredSlug = parsed.publicStatus?.publicGroupSlug) and use that constant for the nullish-coalescing when computing publicGroupSlug, and for the conditional normalization block that calls slugifyPublicGroup; keep createUniquePublicGroupSlug(group.name, usedDefaultSlugs) and usedDefaultSlugs.add(...) semantics the same and reference those functions/variables (parsed, configuredSlug, publicGroupSlug, createUniquePublicGroupSlug, usedDefaultSlugs, slugifyPublicGroup) so the conditional is consistent and the repeated property access is eliminated.tests/unit/settings/status-page/public-status-settings-form.test.tsx (1)
379-440: 测试清理建议：将 prototype 修改与 spy 还原放入 try/finally 或 afterEach
Object.defineProperty(HTMLElement.prototype, "scrollIntoView", …)直接修改了原型，但没有对应的还原逻辑；requestAnimationFrame.mockRestore()也只在断言全部通过时才会执行。一旦中间任意一个expect抛出，原型上的 mock 与 spy 都会泄漏到后续用例（哪怕文件内的下一个用例覆盖了scrollIntoView，也会掩盖失败现场）。建议用afterEach或try/finally保证清理。♻️ 建议改写
it("blocks submit and highlights slug inputs when enabled groups share the same slug", async () => { const scrollIntoView = vi.fn(); + const originalScrollIntoView = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + "scrollIntoView" + ); Object.defineProperty(HTMLElement.prototype, "scrollIntoView", { configurable: true, value: scrollIntoView, }); const requestAnimationFrame = vi .spyOn(window, "requestAnimationFrame") .mockImplementation((callback) => { callback(0); return 0; }); + try { // ... existing test body ... + } finally { + requestAnimationFrame.mockRestore(); + if (originalScrollIntoView) { + Object.defineProperty(HTMLElement.prototype, "scrollIntoView", originalScrollIntoView); + } else { + delete (HTMLElement.prototype as { scrollIntoView?: unknown }).scrollIntoView; + } + } });同样适用于 442-532 行的第二个用例。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/status-page/public-status-settings-form.test.tsx` around lines 379 - 440, The test mutates HTMLElement.prototype.scrollIntoView and installs a spy on window.requestAnimationFrame without guaranteed cleanup; wrap the prototypal override and the requestAnimationFrame spy in a try/finally (or move the setup to a beforeEach and the cleanup to an afterEach) so that HTMLElement.prototype.scrollIntoView is restored and requestAnimationFrame.mockRestore() is always called even if assertions fail; refer to the symbols HTMLElement.prototype.scrollIntoView, requestAnimationFrame (vi.spyOn(window, "requestAnimationFrame")), and requestAnimationFrame.mockRestore() to locate and implement the safe teardown.src/app/[locale]/settings/status-page/_components/public-status-settings-form.tsx (1)
387-407:onChange全局清空duplicateSlugError会同时取消另一个冲突输入框的高亮在某个分组的 slug 输入框里输入字符时，
setDuplicateSlugError(null)会一次性把所有冲突分组上的aria-invalid、destructive 边框和内联错误提示一起清掉，而此时另一个真正还冲突的分组其实并没有被修改。用户可能因此误以为问题已经解决，再次点击保存又会被提示同样的错误。可以考虑只在“当前分组”的 slug 真正被修改后清掉错误，或者在
onChange里重新跑一次findDuplicateSlugError决定是否清除。当前实现作为简化版可接受，所以仅作为可选优化提出。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/status-page/_components/public-status-settings-form.tsx around lines 387 - 407, 当前 onChange 处理器在任何字符输入时直接调用 setDuplicateSlugError(null)，会清掉所有分组的冲突高亮与提示；请改为只在“当前分组”真实不再冲突时清除，方法是在 onChange 里先用 updateGroup 构造包含新 publicGroupSlug 的临时分组列表，然后调用 findDuplicateSlugError(…) 或检查 duplicateSlugError?.groupId 与当前 group.id 来决定是否调用 setDuplicateSlugError(null)；定位到 onChange 中对 setDuplicateSlugError 和 updateGroup 的调用并用上述条件逻辑替换直接清空的行为。src/lib/public-status/config.ts (1)
205-244:createUniquePublicGroupSlug与createCollisionPublicGroupSlug存在明显的循环体重复两者从
let counter = 1到counter += 1这段后缀分配循环逻辑完全一致，仅在“是否提前命中 baseSlug 直接返回”和“是否把结果写回 usedSlugs”这两点上不同。建议抽出一个共享的内部 helper（例如resolveSlugWithStableSuffix），让两个导出/内部函数共用，减少未来调整后缀策略时的维护成本与不一致风险。♻️ 建议改写
+function resolveSlugWithStableSuffix( + baseSlug: string, + groupName: string, + usedSlugs: Set<string> +): string { + let counter = 1; + let candidate = baseSlug; + while (usedSlugs.has(candidate)) { + const suffixSource = counter === 1 ? groupName : `${groupName}-${counter}`; + candidate = appendStablePublicGroupSlugSuffix( + baseSlug || PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, + createStablePublicGroupSlugSuffix(suffixSource) + ); + counter += 1; + } + return candidate; +} + export function createUniquePublicGroupSlug(groupName: string, usedSlugs: Set<string>): string { const baseSlug = normalizePublicGroupSlug(groupName); - if (!usedSlugs.has(baseSlug)) { - usedSlugs.add(baseSlug); - return baseSlug; - } - - let counter = 1; - let candidate = baseSlug; - while (usedSlugs.has(candidate)) { - const suffixSource = counter === 1 ? groupName : `${groupName}-${counter}`; - candidate = appendStablePublicGroupSlugSuffix( - baseSlug || PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, - createStablePublicGroupSlugSuffix(suffixSource) - ); - counter += 1; - } - + const candidate = resolveSlugWithStableSuffix(baseSlug, groupName, usedSlugs); usedSlugs.add(candidate); return candidate; } -function createCollisionPublicGroupSlug( - baseSlug: string, - groupName: string, - usedSlugs: Set<string> -): string { - let counter = 1; - let candidate = baseSlug; - while (usedSlugs.has(candidate)) { - const suffixSource = counter === 1 ? groupName : `${groupName}-${counter}`; - candidate = appendStablePublicGroupSlugSuffix( - baseSlug || PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, - createStablePublicGroupSlugSuffix(suffixSource) - ); - counter += 1; - } - - return candidate; -} +const createCollisionPublicGroupSlug = resolveSlugWithStableSuffix;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/public-status/config.ts` around lines 205 - 244, The loop that builds a non-colliding slug is duplicated in createUniquePublicGroupSlug and createCollisionPublicGroupSlug; extract a shared helper (e.g., resolveSlugWithStableSuffix(baseSlug: string, groupName: string, usedSlugs: Set<string>) => string) that encapsulates the while loop using appendStablePublicGroupSlugSuffix, createStablePublicGroupSlugSuffix and PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, then have createCollisionPublicGroupSlug call that helper and have createUniquePublicGroupSlug use it for the collision case (retaining the early-return when baseSlug is unused and ensuring createUniquePublicGroupSlug still adds the final candidate to usedSlugs before returning).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/app/`[locale]/settings/status-page/_components/public-status-settings-form.tsx:
- Around line 387-407: 当前 onChange 处理器在任何字符输入时直接调用
setDuplicateSlugError(null)，会清掉所有分组的冲突高亮与提示；请改为只在“当前分组”真实不再冲突时清除，方法是在 onChange
里先用 updateGroup 构造包含新 publicGroupSlug 的临时分组列表，然后调用 findDuplicateSlugError(…) 或检查
duplicateSlugError?.groupId 与当前 group.id 来决定是否调用 setDuplicateSlugError(null)；定位到
onChange 中对 setDuplicateSlugError 和 updateGroup 的调用并用上述条件逻辑替换直接清空的行为。
In `@src/app/`[locale]/settings/status-page/loader.ts:
- Around line 23-32: Extract parsed.publicStatus?.publicGroupSlug into a local
constant (e.g., const configuredSlug = parsed.publicStatus?.publicGroupSlug) and
use that constant for the nullish-coalescing when computing publicGroupSlug, and
for the conditional normalization block that calls slugifyPublicGroup; keep
createUniquePublicGroupSlug(group.name, usedDefaultSlugs) and
usedDefaultSlugs.add(...) semantics the same and reference those
functions/variables (parsed, configuredSlug, publicGroupSlug,
createUniquePublicGroupSlug, usedDefaultSlugs, slugifyPublicGroup) so the
conditional is consistent and the repeated property access is eliminated.
In `@src/lib/public-status/config.ts`:
- Around line 205-244: The loop that builds a non-colliding slug is duplicated
in createUniquePublicGroupSlug and createCollisionPublicGroupSlug; extract a
shared helper (e.g., resolveSlugWithStableSuffix(baseSlug: string, groupName:
string, usedSlugs: Set<string>) => string) that encapsulates the while loop
using appendStablePublicGroupSlugSuffix, createStablePublicGroupSlugSuffix and
PUBLIC_STATUS_SLUG_FALLBACK_PREFIX, then have createCollisionPublicGroupSlug
call that helper and have createUniquePublicGroupSlug use it for the collision
case (retaining the early-return when baseSlug is unused and ensuring
createUniquePublicGroupSlug still adds the final candidate to usedSlugs before
returning).
In `@tests/unit/public-status/config-publisher.test.ts`:
- Around line 280-328: Add an assertion that verifies
mockPublishInternalPublicStatusConfigSnapshot is called with an internal
snapshot mapping the original sourceGroupName to the generated slug: after
calling publishCurrentPublicStatusConfigProjection, assert
mockPublishInternalPublicStatusConfigSnapshot was invoked with an
objectContaining({ snapshot: objectContaining({ groups: arrayContaining([
objectContaining({ sourceGroupName: "cc逆向", slug:
expect.stringMatching(/^cc-[a-z0-9]{6}$/) }) ]) }) }) so the
internally-published snapshot preserves the original group name and the same
generated slug used in mockPublishPublicStatusConfigSnapshot.
In `@tests/unit/settings/status-page/public-status-settings-form.test.tsx`:
- Around line 379-440: The test mutates HTMLElement.prototype.scrollIntoView and
installs a spy on window.requestAnimationFrame without guaranteed cleanup; wrap
the prototypal override and the requestAnimationFrame spy in a try/finally (or
move the setup to a beforeEach and the cleanup to an afterEach) so that
HTMLElement.prototype.scrollIntoView is restored and
requestAnimationFrame.mockRestore() is always called even if assertions fail;
refer to the symbols HTMLElement.prototype.scrollIntoView, requestAnimationFrame
(vi.spyOn(window, "requestAnimationFrame")), and
requestAnimationFrame.mockRestore() to locate and implement the safe teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16947309-6919-47af-bab9-5106d7819877
📒 Files selected for processing (13)
messages/en/settings/statusPage.jsonmessages/ja/settings/statusPage.jsonmessages/ru/settings/statusPage.jsonmessages/zh-CN/settings/statusPage.jsonmessages/zh-TW/settings/statusPage.jsonsrc/app/[locale]/settings/status-page/_components/public-status-settings-form.tsxsrc/app/[locale]/settings/status-page/loader.tssrc/lib/public-status/config-publisher.tssrc/lib/public-status/config.tstests/unit/public-status/config-publisher.test.tstests/unit/public-status/public-status-config.test.tstests/unit/settings/status-page/public-status-settings-form.test.tsxtests/unit/settings/status-page/status-page-loader.test.tsx
![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
This PR addresses a real issue with slug collisions for non-English group names and provides a three-layer fix. The implementation is generally sound but has two issues that should be addressed: a code duplication that could create maintenance burden, and an edge case in the loader that could produce initial state collisions.
PR Size: M
- Lines changed: 575 (539 additions, 36 deletions)
- Files changed: 13
- Note: Size label is
size/Lbut by the table this should besize/M(575 < 1000 lines, 13 < 30 files)
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 | 1 | 0 |
High Priority Issues (Should Fix)
[HIGH] loader.ts: Single-pass ordering can produce initial slug collisions
In src/app/[locale]/settings/status-page/loader.ts, the single-pass approach adds explicit slugs to usedDefaultSlugs only after processing each group. This means an earlier group without a stored slug can grab a base slug that collides with a later group's explicit slug.
Example: If Group A (no slug, name="cc") is processed first, it gets slug "cc". Then Group B (stored slug="cc") is processed and also returns "cc", leaving the form in an already-conflicting state that blocks saving.
Suggested fix: Use a two-pass approach—first collect all normalized explicit slugs into usedDefaultSlugs, then run auto-generation.
// First pass: collect explicit slugs
const explicitSlugs = new Set<string>();
for (const group of groups) {
const parsed = parsePublicStatusDescription(group.description);
if (parsed.publicStatus?.publicGroupSlug) {
const normalized = slugifyPublicGroup(parsed.publicStatus.publicGroupSlug);
if (normalized) explicitSlugs.add(normalized);
}
}
// Second pass: generate unique defaults with explicit slugs pre-seeded
const usedDefaultSlugs = new Set<string>(explicitSlugs);
// ... rest of mapping logicMedium Priority Issues (Consider Fixing)
[MEDIUM] config.ts: createCollisionPublicGroupSlug duplicates createUniquePublicGroupSlug's loop body
The while-loop logic in createCollisionPublicGroupSlug (lines 392-409) is nearly identical to createUniquePublicGroupSlug (lines 370-390). The only differences are: (1) createUniquePublicGroupSlug handles the no-collision fast path and adds to usedSlugs, and (2) createCollisionPublicGroupSlug takes a baseSlug parameter instead of deriving it from groupName.
Suggested fix: Extract a shared private helper to eliminate the duplication:
function generateUniqueSlugWithSuffix(
baseSlug: string,
groupName: string,
usedSlugs: Set<string>
): string {
let counter = 1;
let candidate = baseSlug;
while (usedSlugs.has(candidate)) {
const suffixSource = counter === 1 ? groupName : `${groupName}-${counter}`;
candidate = appendStablePublicGroupSlugSuffix(
baseSlug || PUBLIC_STATUS_SLUG_FALLBACK_PREFIX,
createStablePublicGroupSlugSuffix(suffixSource)
);
counter += 1;
}
return candidate;
}Then both createUniquePublicGroupSlug and createCollisionPublicGroupSlug can delegate to this helper.
Review Coverage
- Logic and correctness - 1 issue found (loader ordering edge case)
- Security (OWASP Top 10) - Clean
- Error handling - Clean (no silent failures)
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (13 new tests added)
- Code clarity - 1 issue found (duplicated loop logic)
Additional Notes
- i18n Compliance: All 5 languages properly updated with
duplicateSlugmessage (per CLAUDE.md i18n rule) - Test Coverage: New tests cover non-English slug generation, duplicate detection, Redis projection with legacy slugs, and UI validation
- Pre-commit Checklist: PR description indicates all checks passed
Automated review by Claude AI
| initialAggregationIntervalMinutes: settings.publicStatusAggregationIntervalMinutes, | ||
| initialGroups: groups.map((group) => { | ||
| const parsed = parsePublicStatusDescription(group.description); | ||
| const publicGroupSlug = |
There was a problem hiding this comment.
[HIGH] Single-pass ordering can produce initial slug collisions
The current logic adds explicit stored slugs to usedDefaultSlugs only after their group is processed. This means an earlier group without a stored slug can grab a base slug that collides with a later group's explicit slug.
Example scenario:
- Group A: no stored slug, name normalizes to "cc" → gets "cc"
- usedDefaultSlugs = {"cc"}
- Group B: stored slug="cc" → also returns "cc"
- Form loads with both groups having slug "cc", blocking save
Suggested fix: Use a two-pass approach—first collect all normalized explicit slugs, then run auto-generation with those slugs pre-seeded in usedDefaultSlugs.
|
|
||
| return appendStablePublicGroupSlugSuffix(asciiSlug, suffix); | ||
| } | ||
|
|
There was a problem hiding this comment.
[MEDIUM] Duplicate while-loop logic
This while-loop is nearly identical to the one in createUniquePublicGroupSlug (lines 370-390). Both:
- Initialize
counter = 1andcandidate = baseSlug - Use the same suffix generation logic
- Have identical loop conditions
The only meaningful difference is that createUniquePublicGroupSlug adds to usedSlugs while this function returns the candidate for the caller to add.
Suggested fix: Extract a shared private helper generateUniqueSlugWithSuffix() to eliminate the ~15 lines of duplicated logic.
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in the changed lines. Reviewed slug normalization, duplicate handling, UI validation behavior, and accompanying unit tests for regression coverage.
PR Size: L
- Lines changed: 575
- Files changed: 13
- Split suggestion: Consider separating into (1) slug generation + projection fallback changes in
src/lib/public-status/, (2) settings UI validation + i18n strings, (3) unit tests.
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 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
None.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Identified open PR
#1102(fix(public-status): prevent duplicate group slugs) from the GitHub Actions event payload. - Applied PR size label:
size/L(575 lines changed across 13 files). - Completed 6-perspective diff-focused review and submitted the required summary via
gh pr review --comment. - No inline review comments were posted (no diff-line issues met the reporting threshold after validation).
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/settings/status-page/public-status-settings-form.test.tsx (2)
488-493:getGroupToggleButton使用textContent.includes(groupName)存在误匹配风险。
"openai-primary"是"openai-fallback"之外的另一个组名，但若其他按钮（例如保存/折叠图标按钮的 aria-label 或父级容器）也包含该字符串，Array.find会取到第一个匹配项，可能并非真正的折叠切换按钮。建议改为基于data-testid或更精确的选择器（例如要求按钮文本完全等于groupName）来定位，避免后续 UI 文案变化导致测试静默失效。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/status-page/public-status-settings-form.test.tsx` around lines 488 - 493, getGroupToggleButton currently uses textContent?.includes(groupName) which can false-match other buttons; update getGroupToggleButton to target the actual group toggle precisely (e.g., query container.querySelectorAll("button") but match textContent.trim() === groupName or, better, select by a dedicated data-testid or aria-label on the group toggle). Locate the helper function getGroupToggleButton and replace the loose includes() check with an exact text match or a data-testid/aria selector tied to the group toggle button (ensuring tests use that attribute where the component renders).
384-387: 建议在测试结束后还原HTMLElement.prototype.scrollIntoView。三个新增用例均通过
Object.defineProperty直接修改HTMLElement.prototype上的scrollIntoView，但只对requestAnimationFrame间谍执行了mockRestore，原型上的猴补丁残留可能影响同 worker 中后续运行的其他测试文件。建议在afterEach中删除该属性，或仅在容器元素实例上设置scrollIntoView。♻️ 建议改动
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ describe("public-status settings form", () => { beforeEach(() => { vi.clearAllMocks(); ... }); + + afterEach(() => { + delete (HTMLElement.prototype as { scrollIntoView?: unknown }).scrollIntoView; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/status-page/public-status-settings-form.test.tsx` around lines 384 - 387, The tests mutate HTMLElement.prototype.scrollIntoView via Object.defineProperty but never restore it, which can leak into other tests; update the test file to clean up by restoring or deleting that property in an afterEach hook (or, alternatively, avoid patching the prototype by defining scrollIntoView only on the test container element), and ensure you still call mockRestore on the requestAnimationFrame spy; reference the prototype mutation (HTMLElement.prototype.scrollIntoView), the existing requestAnimationFrame spy/mockRestore, and add an afterEach to delete or restore the prototype property or scope the mock to the container element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/settings/status-page/public-status-settings-form.test.tsx`:
- Around line 488-493: getGroupToggleButton currently uses
textContent?.includes(groupName) which can false-match other buttons; update
getGroupToggleButton to target the actual group toggle precisely (e.g., query
container.querySelectorAll("button") but match textContent.trim() === groupName
or, better, select by a dedicated data-testid or aria-label on the group
toggle). Locate the helper function getGroupToggleButton and replace the loose
includes() check with an exact text match or a data-testid/aria selector tied to
the group toggle button (ensuring tests use that attribute where the component
renders).
- Around line 384-387: The tests mutate HTMLElement.prototype.scrollIntoView via
Object.defineProperty but never restore it, which can leak into other tests;
update the test file to clean up by restoring or deleting that property in an
afterEach hook (or, alternatively, avoid patching the prototype by defining
scrollIntoView only on the test container element), and ensure you still call
mockRestore on the requestAnimationFrame spy; reference the prototype mutation
(HTMLElement.prototype.scrollIntoView), the existing requestAnimationFrame
spy/mockRestore, and add an afterEach to delete or restore the prototype
property or scope the mock to the container element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1320e7e3-5589-45f0-be25-27640a27a65c
📒 Files selected for processing (5)
src/app/[locale]/settings/status-page/_components/public-status-settings-form.tsxsrc/app/[locale]/settings/status-page/loader.tssrc/lib/public-status/config.tstests/unit/settings/status-page/public-status-settings-form.test.tsxtests/unit/settings/status-page/status-page-loader.test.tsx
✅ Files skipped from review due to trivial changes (2)
- src/app/[locale]/settings/status-page/_components/public-status-settings-form.tsx
- src/lib/public-status/config.ts
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Problem
Provider groups such as
cc特价andcc逆向can normalize to the same ASCII slug (cc). That made public status initialization fragile and could triggerDuplicatePublicStatusGroupSlugErrorduring Redis projection publishing.Solution
Tests
bunx vitest run tests/unit/public-status/public-status-config.test.ts tests/unit/settings/status-page/public-status-settings-form.test.tsx tests/unit/settings/status-page/status-page-loader.test.tsx— 3 files / 18 tests passedbun run build— passed; existing Turbopack Edge-runtime warnings fornode:async_hooks/node:netremain warningsbun run lint— passed; existing Biome schema-version info and two unsafe suggestions in dashboard testsbun run lint:fix— passed; no fixes appliedbun run typecheck— passedbun run test— 545 files passed, 5073 tests passed, 2 files / 13 tests skippedUI Screenshot
Greptile Summary
This PR implements a three-layer fix for duplicate public status group slugs: FNV-1a-inspired hash suffixes for non-ASCII group names in
slugifyPublicGroup, aduplicateSlugStrategy: "suffix"option incollectEnabledPublicStatusGroupsto de-duplicate legacy Redis projections without throwing, and a save-timefindDuplicateSlugErrorpreflight in the settings form with expand/highlight/scroll/focus UX. Theloader.tscorrectly uses a two-pass approach (collect explicit slugs first, then auto-generate) to avoid initial-state conflicts.Confidence Score: 5/5
Safe to merge; all findings are P2 edge cases that do not affect correctness under normal usage.
No P0 or P1 issues found. The two P2 findings are: (1) only the first conflicting slug pair is highlighted per save attempt when multiple independent conflicts exist, and (2) the single-pass suffix resolution in collectEnabledPublicStatusGroups could theoretically displace a later group's explicit custom slug if it happens to match an auto-generated hash — an extremely unlikely scenario confined to legacy Redis projection. The implementation is well-structured with comprehensive test coverage across all three layers.
src/lib/public-status/config.ts (suffix collision edge case) and src/app/[locale]/settings/status-page/_components/public-status-settings-form.tsx (single-conflict-per-save UX).
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant Form as PublicStatusSettingsForm participant Config as config.ts participant Publisher as config-publisher.ts participant Redis User->>Form: Click Save Form->>Config: findDuplicateSlugError(groups) alt Duplicate found Config-->>Form: { slug, groupNames } Form->>Form: expand conflicting groups Form->>Form: highlight slug inputs (aria-invalid) Form->>User: toast.error + scroll/focus first input else No duplicate Form->>Form: build SavePublicStatusSettingsInput payload Form->>Publisher: savePublicStatusSettings(payload) Publisher->>Config: collectEnabledPublicStatusGroups(..., { duplicateSlugStrategy: suffix }) Config->>Config: normalizePublicGroupSlug per group alt Legacy slug collision detected Config->>Config: createCollisionPublicGroupSlug (FNV-1a suffix loop) end Config-->>Publisher: EnabledPublicStatusGroup[] Publisher->>Redis: publishCurrentPublicStatusConfigProjection Redis-->>Publisher: written Publisher-->>Form: { ok: true } Form->>User: toast.success endPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(status-page): align duplicate slug v..." | Re-trigger Greptile