refactor: DRY toAuthContext, parallel checkAll, clean up IIFEs, improve JSDoc#2
Merged
n-hallberg merged 1 commit intoamadeni:mainfrom Mar 16, 2026
Conversation
Contributor
|
@greptile |
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
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.
Non-breaking cleanup based on Echo's code review:
toAuthContext— extracted totypes.ts, imported inprimitives.tsandauthorized.tscheckAll—Promise.allinstead of sequential awaits incapabilities.tszid— clarifies it only checks string type, not Convex ID formatCI: 22/22 tests pass, prettier/eslint/tsc/cspell all green.
Greptile Summary
This PR is a clean, non-breaking refactor that consolidates shared logic, improves performance, and enhances documentation with no functional regressions.
toAuthContext: The helper was duplicated verbatim in bothprimitives.tsandauthorized.ts. It is now defined once intypes.ts(alongside theAuthContexttype it constructs) and imported where needed. Both import sites correctly use a value import (import { toAuthContext }) while continuing to useimport typefor the pure-type exports.checkAll: The sequentialfor…of / awaitloop is replaced withPromise.all. Eachhas(ctx, role, key)call is an independent DB read with no shared mutable state, so concurrent execution is correct in all Convex contexts (query, mutation, action). Theas consttuple assertion and finalObject.fromEntries(…) as Record<Key, boolean>cast are idiomatic TypeScript for this pattern.getPermissionCheckerFor*functions in bothauthorized.tsandprimitives.tswere unnecessary — the outer arrow function body is equivalent. The pre-existing IIFE inhasCapabilityChecker(a type-guard, not a checker-getter) is intentionally out of scope.zidJSDoc: Accurately warns consumers that onlytypeof value === 'string'is checked, not the actual Convex ID format — important context for anyone relying on this validator for ID validation.No bugs or logic errors were found.
Confidence Score: 5/5
Promise.allincheckAll) is a correct parallelisation of independent, side-effect-free DB reads. All 22 tests pass and the full CI suite is green.Important Files Changed
toAuthContextruntime helper, extracted from bothprimitives.tsandauthorized.ts; correctly co-located with theAuthContexttype it constructs.for…ofloop incheckAllreplaced withPromise.all; allhascalls are independent DB reads so parallel execution is safe in every Convex context (query, mutation, action).toAuthContextduplicate removed and imported from./types; IIFE wrappers removed from threegetPermissionCheckerFor*functions — semantics are identical to before.authorized.tsapplied to threegetCapabilityCheckerFor*functions; localtoAuthContextduplicate removed. One pre-existing IIFE inhasCapabilityCheckeris intentionally out of scope for this PR.zidvalidates string type only, not the actual Convex ID format — improves consumer awareness with no runtime change.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[checkAll called\nctx, role] --> B[keys = Object.keys registry] B --> C[Promise.all\nkeys.map] C --> D1[has ctx, role, key1] C --> D2[has ctx, role, key2] C --> D3[has ctx, role, keyN] D1 --> E{admin role?} D2 --> E D3 --> E E -- yes --> F[return true] E -- no --> G[getOverride ctx, key] G --> H{override found?} H -- yes --> I[override.roles.includes role] H -- no --> J[registry.defaultRoles.includes role] I --> K[boolean result] J --> K F --> K K --> L[entries array\nreadonly tuples] L --> M[Object.fromEntries entries\nas Record Key boolean]Last reviewed commit: 4c165b6