add slug conflict handling logic #596
Open
Conversation
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
WalkthroughThe slug manager plugin adds conflict detection and handling for slug generation. New types Sequence DiagramsequenceDiagram
participant Caller
participant Manager
participant isSlugConfict
participant API
participant Formatter
Caller->>Manager: webhook / getUpdates (profiles)
activate Manager
Manager->>Manager: compute desiredSlug (slugFormat)
Manager->>isSlugConfict: isSlugConfict(desiredSlug, api)
activate isSlugConfict
isSlugConfict->>API: listEntities(query by slug)
API-->>isSlugConfict: matching entities
isSlugConfict-->>Manager: conflict? (boolean)
deactivate isSlugConfict
alt conflict detected
Manager->>Formatter: apply slugConflictFormat (string or func)
Formatter-->>Manager: conflict-slug
Manager->>Manager: use conflict-slug
else no conflict
Manager->>Manager: use desiredSlug
end
Manager-->>Caller: return updated profile(s) with final slug
deactivate Manager
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
J=CR-3805 TEST=unit
dsurducan
force-pushed
the
dev/add-slug-conflict-field
branch
from
03473cf to
5ee1cc7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/slugManager/test.ts (1)
1-338: Add test coverage for conflict handling.The new conflict handling feature has no test coverage. Please add tests to verify:
- Conflict detection: When
listEntitiesreturns entities with matching slugs, verify thatisSlugConflictreturnstrue.- String-based conflict format: When
slugConflictFormatis a string, verify it's applied when conflicts exist.- Function-based conflict format: When
slugConflictFormatis a function, verify it's called and its result is used.- Default conflict format: When
slugConflictFormatisnull/undefined, verify the entity ID is appended to the slug.- No conflict case: When no conflicts exist, verify the original slug is used.
- Webhook flow: Verify conflict handling in the webhook function.
- Batch flow: Verify conflict handling in
getUpdates.Would you like me to generate test cases for the conflict handling logic?
🧹 Nitpick comments (1)
packages/plugins/slugManager/manager.ts (1)
166-197: Consider performance and race condition implications.The current implementation checks for conflicts individually for each profile (line 179), making a separate API call each time. If processing many profiles, this could be slow and potentially hit rate limits.
Additionally, there's an inherent race condition: between checking for a conflict and applying the update, another process could create an entity with the same slug. This is difficult to avoid without database-level uniqueness constraints or transactional guarantees.
Consider:
- Batching: Fetch all existing slugs in one API call, then check against that set in memory.
- Database constraints: Add a unique index on the slug field at the database level if possible.
- Retry logic: Implement retry with exponential backoff if a conflict is detected after update.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/slugManager/manager.ts(4 hunks)packages/plugins/slugManager/test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/plugins/slugManager/manager.ts (1)
packages/plugins/slugManager/api.ts (2)
BaseEntity(15-24)IAPI(56-69)
packages/plugins/slugManager/test.ts (1)
packages/plugins/slugManager/manager.ts (1)
getUpdates(166-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: call_playwright / acceptance (macos-latest, 22.x)
- GitHub Check: call_playwright / acceptance (macos-latest, 20.x)
- GitHub Check: call_playwright / acceptance (macos-latest, 18.x)
- GitHub Check: call_playwright / acceptance (ubuntu-latest, 22.x)
- GitHub Check: call_playwright / acceptance (windows-latest, 18.x)
- GitHub Check: call_playwright / acceptance (ubuntu-latest, 20.x)
- GitHub Check: call_playwright / acceptance (windows-latest, 20.x)
- GitHub Check: call_playwright / acceptance (windows-latest, 22.x)
- GitHub Check: call_playwright / acceptance (ubuntu-latest, 18.x)
- GitHub Check: call_jstest / jstest
- GitHub Check: semgrep/ci
🔇 Additional comments (4)
packages/plugins/slugManager/manager.ts (2)
14-27: Type definitions look good.The conflict format types mirror the existing slug format pattern appropriately.
87-94: LGTM!The connector properly passes the new parameters to
getUpdatesand correctly awaits the result.packages/plugins/slugManager/test.ts (2)
55-109: Test updates look correct.All tests have been properly updated to handle the async
getUpdatessignature and pass the requiredapiparameter.
111-133: Mock implementation is basic but functional.The
baseMockAPIprovides the minimal implementation needed. Note thatlistEntitiesalways returns an empty array, so conflict detection will never trigger in the existing tests.
Comment on lines
+135
to
+141
| let desiredSlug = | ||
| typeof slugFormat == "string" ? slugFormat : slugFormat(lang, profile); | ||
| return api.updateField(event.entityId, lang, slugField, slug); | ||
| const isDuplicate = await isSlugConfict(desiredSlug, api); | ||
| if (isDuplicate) { | ||
| desiredSlug = slugConflictFormat == null ? desiredSlug + "-[[entityId]]" : typeof slugConflictFormat == "string" ? slugConflictFormat : slugConflictFormat(lang, profile); | ||
| } | ||
| return api.updateField(event.entityId, lang, slugField, desiredSlug); |
There was a problem hiding this comment.
Update conflict detection call and fix default format.
Issues with the webhook conflict handling:
- The function call needs to be updated to match the corrected function signature (pass
slugFieldandexcludeEntityId). - The default conflict format
"-[[entityId]]"uses template syntax but won't be expanded. It should use the actualevent.entityIdvalue. - Line 139 has a complex nested ternary that's hard to read.
Apply this diff:
let desiredSlug =
typeof slugFormat == "string" ? slugFormat : slugFormat(lang, profile);
- const isDuplicate = await isSlugConfict(desiredSlug, api);
+ const isDuplicate = await isSlugConflict(desiredSlug, api, slugField, event.entityId);
if (isDuplicate) {
- desiredSlug = slugConflictFormat == null ? desiredSlug + "-[[entityId]]" : typeof slugConflictFormat == "string" ? slugConflictFormat : slugConflictFormat(lang, profile);
+ if (slugConflictFormat == null) {
+ desiredSlug = `${desiredSlug}-${event.entityId}`;
+ } else if (typeof slugConflictFormat === "string") {
+ desiredSlug = slugConflictFormat;
+ } else {
+ desiredSlug = slugConflictFormat(lang, profile);
+ }
}
return api.updateField(event.entityId, lang, slugField, desiredSlug);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/plugins/slugManager/manager.ts around lines 135 to 141, update the
conflict detection call and default conflict format: call
isSlugConfict(desiredSlug, api, slugField, event.entityId) (matching the new
signature that accepts slugField and excludeEntityId), replace the literal
"-[[entityId]]" with a string that concatenates the actual event.entityId (e.g.
`"-" + event.entityId`) so the ID is inserted, and simplify the nested ternary
by using a short if/else to set desiredSlug when isDuplicate (check if
slugConflictFormat is null, else if it's a string use it, else call
slugConflictFormat(lang, profile)).
Comment on lines
+147
to
+164
| async function isSlugConfict(desiredSlug : string, api : Pick<IAPI, "updateField" | "listLanguageProfiles" | "listEntities">) { | ||
| const params = new URLSearchParams({ | ||
| fields: ["meta", ...["slug"]].join(","), | ||
| /** | ||
| * Match entities that have the same slug. | ||
| */ | ||
| filter: JSON.stringify({ | ||
| "slug": { | ||
| $contains: desiredSlug, | ||
| } | ||
| }), | ||
| }); | ||
| const entitiesResponse = await api.listEntities(params); | ||
| if (entitiesResponse?.entities?.length > 1) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix typo, hardcoded field, filter logic, and conflict detection.
This function has several critical issues:
- Typo: Function name should be
isSlugConflictnotisSlugConfict. - Hardcoded field: Line 149 hardcodes
"slug"but the field name is configurable viaslugFieldparameter. This function should acceptslugFieldas a parameter. - Incorrect filter:
$contains: desiredSlug(line 155) matches any slug containing the desired slug as a substring. For example, ifdesiredSlugis"product", it will match"my-product","product-123", etc. Use exact match instead:$eq: desiredSlug. - Wrong conflict logic: Checking
> 1(line 160) means a conflict is only detected when 2+ entities exist. However, if even 1 entity has the slug, that's a conflict for a new entity. The logic should be>= 1. Additionally, for updates to existing entities, you need to exclude the entity being updated from the conflict check (otherwise updating an entity would always detect itself as a conflict).
Apply this diff:
-async function isSlugConfict(desiredSlug : string, api : Pick<IAPI, "updateField" | "listLanguageProfiles" | "listEntities">) {
+async function isSlugConflict(
+ desiredSlug: string,
+ api: Pick<IAPI, "updateField" | "listLanguageProfiles" | "listEntities">,
+ slugField: string,
+ excludeEntityId?: string
+) {
const params = new URLSearchParams({
- fields: ["meta", ...["slug"]].join(","),
+ fields: ["meta", slugField].join(","),
/**
* Match entities that have the same slug.
*/
filter: JSON.stringify({
- "slug": {
- $contains: desiredSlug,
+ [slugField]: {
+ $eq: desiredSlug,
}
}),
});
- const entitiesResponse = await api.listEntities(params);
- if (entitiesResponse?.entities?.length > 1) {
+ const entitiesResponse = await api.listEntities(params);
+ const conflictingEntities = excludeEntityId
+ ? entitiesResponse?.entities?.filter(e => e.meta.id !== excludeEntityId) || []
+ : entitiesResponse?.entities || [];
+ if (conflictingEntities.length > 0) {
return true;
}
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function isSlugConfict(desiredSlug : string, api : Pick<IAPI, "updateField" | "listLanguageProfiles" | "listEntities">) { | |
| const params = new URLSearchParams({ | |
| fields: ["meta", ...["slug"]].join(","), | |
| /** | |
| * Match entities that have the same slug. | |
| */ | |
| filter: JSON.stringify({ | |
| "slug": { | |
| $contains: desiredSlug, | |
| } | |
| }), | |
| }); | |
| const entitiesResponse = await api.listEntities(params); | |
| if (entitiesResponse?.entities?.length > 1) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| async function isSlugConflict( | |
| desiredSlug: string, | |
| api: Pick<IAPI, "updateField" | "listLanguageProfiles" | "listEntities">, | |
| slugField: string, | |
| excludeEntityId?: string | |
| ) { | |
| const params = new URLSearchParams({ | |
| fields: ["meta", slugField].join(","), | |
| /** | |
| * Match entities that have the same slug. | |
| */ | |
| filter: JSON.stringify({ | |
| [slugField]: { | |
| $eq: desiredSlug, | |
| } | |
| }), | |
| }); | |
| const entitiesResponse = await api.listEntities(params); | |
| const conflictingEntities = excludeEntityId | |
| ? entitiesResponse?.entities?.filter(e => e.meta.id !== excludeEntityId) || [] | |
| : entitiesResponse?.entities || []; | |
| if (conflictingEntities.length > 0) { | |
| return true; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In packages/plugins/slugManager/manager.ts around lines 147 to 164, rename the
function to isSlugConflict and change its signature to accept slugField (string)
and an optional excludeEntityId (string | undefined) alongside the api; stop
hardcoding "slug" and use slugField in the fields param and filter key; use
exact match ($eq: desiredSlug) instead of $contains; and change conflict logic
to detect >= 1 match, excluding the current entity when excludeEntityId is
provided by adding an id $ne condition to the filter so updates don't count the
entity itself as a conflict.
Comment on lines
+177
to
+182
| let desiredSlug = | ||
| typeof slugFormat == "string" ? slugFormat : slugFormat(lang, profile); | ||
| const isDuplicate = await isSlugConfict(desiredSlug, api); | ||
| if (isDuplicate) { | ||
| desiredSlug = slugConflictFormat == null ? desiredSlug + "-[[entityId]]" : typeof slugConflictFormat == "string" ? slugConflictFormat : slugConflictFormat(lang, profile); | ||
| } |
There was a problem hiding this comment.
Update conflict detection call and fix default format.
Same issues as in the webhook flow:
- The conflict check needs to pass
slugFieldand exclude the current entity ID. - The default format uses template syntax that won't be expanded.
- The nested ternary is hard to read.
Apply this diff:
let desiredSlug =
typeof slugFormat == "string" ? slugFormat : slugFormat(lang, profile);
- const isDuplicate = await isSlugConfict(desiredSlug, api);
+ const isDuplicate = await isSlugConflict(desiredSlug, api, slugField, profile.meta.id);
if (isDuplicate) {
- desiredSlug = slugConflictFormat == null ? desiredSlug + "-[[entityId]]" : typeof slugConflictFormat == "string" ? slugConflictFormat : slugConflictFormat(lang, profile);
+ if (slugConflictFormat == null) {
+ desiredSlug = `${desiredSlug}-${profile.meta.id}`;
+ } else if (typeof slugConflictFormat === "string") {
+ desiredSlug = slugConflictFormat;
+ } else {
+ desiredSlug = slugConflictFormat(lang, profile);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/plugins/slugManager/manager.ts lines ~177-182: the current block builds
desiredSlug with a nested ternary, calls isSlugConfict without passing slugField
or excluding the current entity, and uses a default suffix "-[[entityId]]" that
won't be expanded; replace with clearer imperative logic: determine desiredSlug
by calling slugFormat(lang, profile) when slugFormat is a function otherwise use
the string, call isSlugConflict(desiredSlug, api, slugField, { excludeEntityId:
profile.id }) (or equivalent args per function signature) to include slugField
and exclude the current entity, and if conflict detected set desiredSlug to
either the string slugConflictFormat or slugConflictFormat(lang, profile) when
it's a function, falling back to a default suffix that uses the project’s
templating token (e.g. "-{entityId}") instead of "-[[entityId]]"; rewrite the
nested ternary into straightforward if/else for readability.
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.
J=CR-3805
TEST=unit