fix: update idempotencyKey API to make clear you only provide a portion #285
Conversation
therockstorm
requested review from
Davidster,
FelipePhilippCbh,
stefanchrobot and
thedeveloperr
📝 WalkthroughWalkthroughThis PR refactors the idempotency key structure in the notifications system, replacing a flat Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Key areas requiring attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 550d684
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.ts (1)
33-47: Test confirms design:resource.typeexcluded from hash.This test verifies that
resourceId: "same-id"andresource: { type: "account", id: "same-id" }produce identical hashes. Consider adding a test case that explicitly documents the behavior when different types share the same ID:it("produces same hash for different resource types with same id (type is not hashed)", () => { const accountInput = { ...baseParams, resource: { type: "account", id: "shared-id" }, }; const shiftInput = { ...baseParams, resource: { type: "shift", id: "shared-id" }, }; const actualAccount = triggerIdempotencyKeyParamsToHash(accountInput); const actualShift = triggerIdempotencyKeyParamsToHash(shiftInput); // Type is intentionally excluded from hash - IDs must be globally unique expect(actualAccount).toBe(actualShift); });packages/notifications/src/lib/notificationJobEnqueuer.ts (1)
235-241: Clarify behavior when bothidempotencyKeyandidempotencyKeyPartsare provided.The spread order means
idempotencyKeyPartsproperties overrideidempotencyKeyproperties (e.g.,eventOccurredAt). This is reasonable for migration, but consider either:
- Adding runtime validation to warn/error if both are provided, or
- Documenting the precedence behavior in the JSDoc
const idempotencyKeyParams: TriggerIdempotencyKeyParams = { // idempotencyKeyParts takes precedence over deprecated idempotencyKey ...data.idempotencyKey, ...data.idempotencyKeyParts, chunk: number, recipients, workflowKey: data.workflowKey, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
packages/ai-rules/.ruler/backend/notifications.md(1 hunks)packages/notifications/README.md(1 hunks)packages/notifications/examples/enqueueNotificationJob.ts(1 hunks)packages/notifications/examples/usage.md(1 hunks)packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.ts(1 hunks)packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.ts(2 hunks)packages/notifications/src/lib/notificationJobEnqueuer.ts(3 hunks)packages/notifications/src/lib/triggerIdempotencyKey.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Write concise, technical TypeScript code with accurate examples
Use functional and declarative programming patterns
Prefer iteration and modularization over code duplication
Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError)
Structure files: constants, types, exported functions, non-exported functions
Avoid magic strings and numbers; define constants
When declaring functions, use thefunctionkeyword, notconst
Files should read from top to bottom:exported items live on top and the internal functions and methods they call go below them
Prefer data immutability
Sanitize user input
Handle errors and edge cases at the beginning of functions
Use early returns for error conditions to avoid deeply nested if statements
Place the happy path last in the function for improved readability
Avoid unnecessary else statements; use the if-return pattern instead
Use guard clauses to handle preconditions and invalid states early
Implement proper error logging and user-friendly error messages
Favor@clipboard-health/util-ts'sEithertype for expected errors instead oftry/catch
Use strict-mode TypeScript for all code; prefer interfaces over types
Avoid enums; use const maps instead
Strive for precise types. Look for type definitions in the codebase and create your own if none exist
Avoid using type assertions likeasor!unless absolutely necessary
Use theunknowntype instead ofanywhen the type is truly unknown
Use an object to pass multiple function params and to return results
Leverage union types, intersection types, and conditional types for complex type definitions
Use mapped types and utility types (e.g.,Partial<T>,Pick<T>,Omit<T>) to transform existing types
Implement generic types to create reusable, flexible type definitions
Utilize thekeyofoperator and index access types for dynamic property access
Implement discriminated unions for type-safe handling of different object shapes where approp...
Files:
packages/notifications/src/lib/triggerIdempotencyKey.tspackages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.tspackages/notifications/examples/enqueueNotificationJob.tspackages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.tspackages/notifications/src/lib/notificationJobEnqueuer.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use camelCase for files and directories (e.g., modules/shiftOffers.ts)
Files:
packages/notifications/src/lib/triggerIdempotencyKey.tspackages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.tspackages/notifications/examples/enqueueNotificationJob.tspackages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.tspackages/notifications/src/lib/notificationJobEnqueuer.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx}: Follow the Arrange-Act-Assert convention for tests with newlines between each section
Name test variables using themockX,input,expected,actualconvention
Aim for high test coverage, writing both positive and negative test cases
Preferit.eachfor multiple test cases
Avoid conditional logic in tests
Files:
packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.ts
🧬 Code graph analysis (3)
packages/notifications/src/lib/triggerIdempotencyKey.ts (1)
packages/notifications/src/lib/notificationJobEnqueuer.ts (2)
IdempotencyKey(28-31)IdempotencyKeyParts(33-61)
packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.ts (1)
packages/notifications/src/lib/triggerIdempotencyKey.ts (1)
TriggerIdempotencyKeyParams(22-37)
packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.ts (1)
packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.ts (1)
triggerIdempotencyKeyParamsToHash(8-10)
⏰ 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). (2)
- GitHub Check: pull-request
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (9)
packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.ts (1)
17-22: Consider includingresource.typein the hash for collision prevention.The
resource.typefield is discarded during hash computation. If two different resource types share the sameid, they will produce identical hashes. While this may be intentional (the type exists only for API clarity), it could lead to unintended idempotency collisions.If this is by design (e.g.,
idvalues are globally unique across types), consider adding a comment clarifying this assumption. Otherwise, includetypein the hash:resourceId: "resourceId" in params ? params.resourceId : "resource" in params && params.resource && "id" in params.resource - ? params.resource.id + ? `${params.resource.type}:${params.resource.id}` : undefined,packages/notifications/examples/usage.md (1)
95-101: Documentation update looks good.The example correctly demonstrates the new
idempotencyKeyPartsstructure with the nestedresourceobject, aligning with the API changes.packages/notifications/README.md (1)
89-95: README example correctly updated.The embedded example properly reflects the new
idempotencyKeyPartsAPI with the nestedresourcestructure.packages/notifications/src/lib/triggerIdempotencyKey.ts (1)
22-37: Type definition correctly updated to support both formats.The union type
(IdempotencyKey | IdempotencyKeyParts)properly supports both the deprecated legacy format and the new structured format, maintaining backward compatibility.packages/notifications/examples/enqueueNotificationJob.ts (1)
19-25: Example correctly demonstrates the new API.The update properly shows the
idempotencyKeyPartsstructure with the nestedresourceobject, which should help prevent the misuse described in the PR objectives.packages/ai-rules/.ruler/backend/notifications.md (1)
87-93: AI rules documentation properly updated.The embedded example correctly reflects the new
idempotencyKeyPartsAPI structure.packages/notifications/src/lib/internal/triggerIdempotencyKeyParamsToHash.spec.ts (1)
1-127: Comprehensive test coverage for the new hashing logic.The tests properly cover resourceId extraction from both formats, deterministic hashing, recipient order invariance, and workplaceId inclusion. Good adherence to Arrange-Act-Assert pattern and naming conventions.
packages/notifications/src/lib/notificationJobEnqueuer.ts (2)
33-61: Well-designed discriminated union type.The
IdempotencyKeyPartsunion with mutually exclusive branches (eventOccurredAtvsresource) is properly structured. The JSDoc documentation clearly explains the intended usage patterns.
25-31: Deprecation path is well-documented.The
@deprecatedannotations onIdempotencyKeyandidempotencyKeyfield, along with the comprehensive JSDoc onidempotencyKeyParts, provide clear migration guidance.Also applies to: 63-87
Summary
After the API changes in #215, people are using the API as expected, with one exception: they're including data that is already included by default (e.g. workflow key, recipient ID) in
idempotencyKey.resourceId. For example:We explain this in the comments, but people don't reliably read comments. This PR attempts to address it in two ways:
idempotencyKeytoidempotencyKeyParts.typeof resource.Question to reviewers: Do you think this is sufficient to change behavior?