Relationships: support signatures with table names #887
Conversation
WalkthroughThe PR adds overloads and dispatcher implementations for getOrThrow, getAll, and getAllOrThrow to accept either implicit or explicit table arguments, introduces an internal NonUnion type to constrain generics, and expands tests broadly to cover both calling conventions, error conditions, and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (2)packages/convex-helpers/server/relationships.test.ts (2)
packages/convex-helpers/server/relationships.ts (2)
🔇 Additional comments (7)
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 |
Nicolapps
requested review from
ianmacartney
and removed request for
ianmacartney
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/convex-helpers/server/relationships.ts (1)
24-60: Implementation is correct, but ESLint rule is undefined.The overload dispatch logic correctly handles both calling conventions. However, the pipeline is failing because the ESLint rule
@convex-dev/explicit-table-idsis not defined.The ESLint configuration needs to be updated to either:
- Define the
@convex-dev/explicit-table-idsrule- Remove the eslint-disable comments if the rule is no longer needed
- Use a different rule name that exists
This is blocking the pipeline. Please verify whether this is a custom ESLint plugin that needs to be configured or if the rule name has changed.
🧹 Nitpick comments (2)
packages/convex-helpers/server/relationships.test.ts (1)
669-744: System table tests are functional but typing could be improved.The FIXME comments at lines 702 and 741 correctly identify that
getManyViareturnsunknown[]for system tables, requiring type assertions. While the runtime behavior is properly tested, this typing limitation degrades developer experience.Consider addressing the system table typing in a follow-up. Would you like me to investigate whether the return type can be refined to properly infer system table document types, or is this limitation intentional due to the dynamic nature of system table access?
packages/convex-helpers/server/relationships.ts (1)
71-102: Implementation is correct; consider ESLint comment consistency.The overloads correctly use
NonUnion<TableName>to prevent unwanted union inference when explicitly passing a table name. The dispatch logic properly routes to the appropriatedb.getvariant.Note: Line 101 calls
db.get(id)without an eslint-disable comment, whilegetOrThrow(line 54-55) includes one for the same pattern. Consider adding the comment here for consistency once the ESLint rule issue is resolved.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/convex-helpers/server/relationships.test.ts(3 hunks)packages/convex-helpers/server/relationships.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When modifying complex TypeScript types, run
npm run typecheckin the repository root to ensure types are correct, rather than runningtscmanually
Files:
packages/convex-helpers/server/relationships.test.tspackages/convex-helpers/server/relationships.ts
🧬 Code graph analysis (2)
packages/convex-helpers/server/relationships.test.ts (1)
packages/convex-helpers/server/relationships.ts (3)
getOrThrow(45-60)getAll(96-102)getAllOrThrow(138-152)
packages/convex-helpers/server/relationships.ts (2)
packages/convex-helpers/server.ts (1)
Table(33-57)packages/convex-helpers/index.ts (2)
asyncMap(9-21)nullThrows(39-44)
🪛 GitHub Actions: Test and lint
packages/convex-helpers/server/relationships.ts
[error] 54-54: ESLint: Definition for rule '@convex-dev/explicit-table-ids' was not found
🪛 GitHub Check: Test and lint
packages/convex-helpers/server/relationships.ts
[failure] 54-54:
Definition for rule '@convex-dev/explicit-table-ids' was not found
[failure] 451-451:
Definition for rule '@convex-dev/explicit-table-ids' was not found
[failure] 448-448:
Definition for rule '@convex-dev/explicit-table-ids' was not found
[failure] 519-519:
Definition for rule '@convex-dev/explicit-table-ids' was not found
[failure] 511-511:
Definition for rule '@convex-dev/explicit-table-ids' was not found
🔇 Additional comments (4)
packages/convex-helpers/server/relationships.test.ts (3)
44-148: LGTM! Comprehensive test coverage for both calling conventions.The nested describe blocks clearly distinguish between implicit and explicit table name usage. The addition of the wrong table error test (lines 128-146) is particularly valuable for validating runtime type safety.
150-292: LGTM! Thorough coverage of both overloads and edge cases.The tests appropriately validate that
getAllreturnsnullfor missing documents rather than throwing, and the wrong table test ensures proper runtime validation.
294-434: LGTM! Excellent validation of throw behavior and type safety.The test at lines 402-432 correctly uses
@ts-expect-errorto validate runtime table mismatch detection while acknowledging that TypeScript would catch this at compile time in normal usage.packages/convex-helpers/server/relationships.ts (1)
113-152: LGTM! Clean implementation that properly delegates to getOrThrow.The overloads consistently use
NonUnion<TableName>for type safety, and the implementation correctly reusesgetOrThrowto avoid code duplication. This ensures consistent error handling across both functions.
| /** | ||
| * This prevents TypeScript from inferring that the generic `TableName` type is | ||
| * a union type when `table` and `id` disagree. | ||
| */ | ||
| type NonUnion<T> = T extends never // `never` is the bottom type for TypeScript unions | ||
| ? never | ||
| : T; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the NonUnion type definition and usage context
cat -n packages/convex-helpers/server/relationships.ts | sed -n '525,545p'Repository: get-convex/convex-helpers
Length of output: 415
🏁 Script executed:
# Look at the overload signatures where NonUnion is used (lines 78-79, 120-121)
cat -n packages/convex-helpers/server/relationships.ts | sed -n '70,90p'
echo "---"
cat -n packages/convex-helpers/server/relationships.ts | sed -n '110,130p'Repository: get-convex/convex-helpers
Length of output: 1978
🏁 Script executed:
# Also check if there are type tests or examples showing the intended behavior
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "NonUnion\|relationships" 2>/dev/null | head -5Repository: get-convex/convex-helpers
Length of output: 175
🏁 Script executed:
cat -n packages/convex-helpers/server/relationships.test.ts | head -100Repository: get-convex/convex-helpers
Length of output: 3366
🏁 Script executed:
# Search for any tests or comments related to NonUnion
rg -i "nonunion\|union.*distribute\|overload" packages/convex-helpers/server/relationships.test.ts -A 3 -B 3Repository: get-convex/convex-helpers
Length of output: 51
🏁 Script executed:
cd packages/convex-helpers && npm run typecheck 2>&1 | head -50Repository: get-convex/convex-helpers
Length of output: 142
🏁 Script executed:
# Also look at the broader context - check what overloads exist for getAll
rg -A 20 "export async function getAll" packages/convex-helpers/server/relationships.ts | head -80Repository: get-convex/convex-helpers
Length of output: 3121
🏁 Script executed:
# Let's create a small test to verify the NonUnion behavior
cat > /tmp/test_nonunion.ts << 'EOF'
// Simplified test of NonUnion behavior
type NonUnion<T> = T extends never ? never : T;
// Test what NonUnion does with unions
type Test1 = NonUnion<"users">; // Should be "users"
type Test2 = NonUnion<"users" | "posts">; // What does this become?
// The identity conditional distributes over unions
// NonUnion<"users" | "posts"> =
// ("users" extends never ? never : "users") | ("posts" extends never ? never : "posts") =
// "users" | "posts"
// So it doesn't actually prevent the union from being inferred
type Test3 = NonUnion<string | number>; // This is still string | number
// Check if it's actually being used for overload resolution correctly
declare function getAll<T extends "users" | "posts">(
table: T,
ids: Array<NonUnion<T>>
): void;
declare function getAll<T extends "users" | "posts">(
ids: Array<T>
): void;
// Try to call it - does overload resolution work?
const userIds: Array<"users"> = [];
getAll("users", userIds); // Should match first overload
const mixedIds: Array<"users" | "posts"> = [];
getAll(mixedIds); // Should match second overload, not first
EOF
cat /tmp/test_nonunion.tsRepository: get-convex/convex-helpers
Length of output: 1162
🏁 Script executed:
# Look for any comments or discussions about union types in the relationships file
rg -i "union\|distribute" packages/convex-helpers/server/relationships.ts -B 2 -A 2 | head -40Repository: get-convex/convex-helpers
Length of output: 51
🏁 Script executed:
# Let's check if there are any actual type errors or issues in the actual implementation
# by looking at the full getOrThrow function which has similar patterns
rg -A 25 "export async function getOrThrow" packages/convex-helpers/server/relationships.ts | head -60Repository: get-convex/convex-helpers
Length of output: 1613
🏁 Script executed:
# Let's check the git history or any discussions about NonUnion
git log -p --all -S "NonUnion" -- packages/convex-helpers/server/relationships.ts 2>/dev/null | head -100Repository: get-convex/convex-helpers
Length of output: 51
🏁 Script executed:
# Also verify the actual type behavior in context by checking GenericId definition
rg -B 5 -A 10 "type GenericId" packages/convex-helpers/server/relationships.tsRepository: get-convex/convex-helpers
Length of output: 51
🏁 Script executed:
# Let's verify the actual behavior by looking at how NonUnion is used in context
# and whether there are any actual type safety issues being prevented
cat -n packages/convex-helpers/server/relationships.ts | sed -n '1,70p' | tail -20Repository: get-convex/convex-helpers
Length of output: 881
🏁 Script executed:
# Check if npm run typecheck actually works with proper setup
cd packages/convex-helpers && npm list typescript 2>&1 | head -20Repository: get-convex/convex-helpers
Length of output: 147
🏁 Script executed:
# Check the entire relationships.ts file around the NonUnion definition for any related discussions
cat -n packages/convex-helpers/server/relationships.ts | sed -n '520,560p'Repository: get-convex/convex-helpers
Length of output: 678
🏁 Script executed:
# Look for any issues or PRs mentioning NonUnion
cd /home/jailuser/git && git log --all --oneline --grep="NonUnion" 2>/dev/null || git log --all --oneline | grep -i "union\|overload" | head -10Repository: get-convex/convex-helpers
Length of output: 51
🏁 Script executed:
# Let me verify the actual type behavior with a concrete test case
cat > /tmp/verify_nonunion.ts << 'EOF'
// Test the actual NonUnion behavior in overload context
type NonUnion<T> = T extends never ? never : T;
// Simulate the function overloads
declare function test<T extends "a" | "b">(
table: T,
id: NonUnion<T>
): "overload1";
declare function test<T extends "a" | "b">(
id: T
): "overload2";
// Test cases
type Case1 = typeof test extends (table: infer T, id: infer I) => any ? [T, I] : never;
type Case2 = typeof test extends (id: infer I) => any ? I : never;
// The real issue: does the identity conditional actually prevent the union?
type TestUnion = NonUnion<"a" | "b">;
// This evaluates to: ("a" | "b") extends never ? never : ("a" | "b") = "a" | "b"
// So it's just "a" | "b", not preventing anything
// The standard way to prevent union distribution
type NonUnionCorrect<T> = [T] extends [T] ? T : never;
type TestUnionCorrect = NonUnionCorrect<"a" | "b">;
// This evaluates to: ["a" | "b"] extends ["a" | "b"] ? "a" | "b" : never = "a" | "b"
// But the key is that T is not "naked" - it's wrapped, so it doesn't distribute
// For actual union prevention:
type PreventUnion<T> = T extends any ? T : never;
// No, this still distributes!
// The ONLY way to prevent distribution is the bracket pattern:
type ActuallyPreventUnion<T> = [T] extends [any] ? T : never;
EOF
cat /tmp/verify_nonunion.tsRepository: get-convex/convex-helpers
Length of output: 1353
🏁 Script executed:
# Count all the overloads for functions using NonUnion
rg "NonUnion<" packages/convex-helpers/server/relationships.ts -cRepository: get-convex/convex-helpers
Length of output: 70
🏁 Script executed:
# Get the context - are there validation tests checking type behavior?
rg -l "\.test\.ts\|\.spec\.ts" packages/convex-helpers/server | xargs grep -l "type.*Test\|expectType" 2>/dev/nullRepository: get-convex/convex-helpers
Length of output: 53
NonUnion type implementation uses non-standard union distribution prevention pattern.
The official TypeScript approach to prevent conditional type distribution is wrapping each side of the extends keyword with square brackets. The current implementation type NonUnion<T> = T extends never ? never : T is an identity conditional that doesn't actually prevent distribution—it simply returns T unchanged.
Replace with the standard documented pattern:
type NonUnion<T> = [T] extends [T] ? T : never;This explicitly prevents TypeScript from distributing over union types during type resolution, which is the documented and recommended approach.
🤖 Prompt for AI Agents
packages/convex-helpers/server/relationships.ts around lines 530 to 536: the
current NonUnion type uses `T extends never ? never : T`, which doesn't prevent
conditional type distribution; replace it with the documented non-distribution
pattern by changing the implementation to use a tuple-wrapped extends check so
the type prevents union distribution (i.e., update the type definition to the
standard pattern that wraps T in square brackets on both sides of extends).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.