Repository navigation
Conversation
Register Vercel React Best Practices in the canonical .ai skill system. Vendor 70 upstream rules and generate the Codex and Claude adapters.
| ttl: 5 * 60 * 1000 // 5 minutes | ||
| }) | ||
|
|
||
| export async function getUser(id: string) { |
There was a problem hiding this comment.
This cache is shared across requests but keyed only by user.id; tenant and authorization scope are missing. Equal IDs across tenants or changed permissions can return data from the wrong context. That conflicts with the skill rule against request-specific data in shared caches.
There was a problem hiding this comment.
Fixed in c3e4c23. The example keys the cache by tenant and ID. The rule now requires including every scope that changes the result in the key, checking authorization on every request before returning cached data, never caching authorization decisions or caller-specific views, and keeping the TTL short.
There was a problem hiding this comment.
This is not fully fixed yet. The prose requires authorization on every request, but getUser accepts no caller or permission context and line 26 returns a cached value before any authorization check. The copyable implementation still cannot enforce the security rule it claims.
There was a problem hiding this comment.
Fixed in 85abf45. The cache now sits behind an unexported loadUser. The exported getUser(caller, id) calls canReadUser(caller, id) on every request and throws before any cached value can be returned, and it takes the tenant from the authenticated caller rather than from input. The prose states that only the unexported loader touches the shared cache.
There was a problem hiding this comment.
The authorization path is fixed, but the cache key is still ambiguous: a:b + c and a + b:c both produce a:b:c. If either identifier may contain :, this can still return one tenant record for another key.
AmitAvital1
left a comment
AmitAvital1
left a comment
There was a problem hiding this comment.
Requesting changes for the line-level correctness and security issues noted in this review.
Seven vendored Vercel rules taught incorrect or unsafe patterns: - rendering-usetransition-loading: transitions do not cancel requests, so ignore stale responses instead of letting them overwrite newer results. - js-cache-function-results: cache only pure functions, bounded by size; context-dependent caches need explicit invalidation. - bundle-conditional: a typeof window guard in an effect does not affect bundling; effects never run during server rendering. - server-cache-lru: key shared caches by tenant and scope, and check authorization on every request. - rendering-hydration-no-flicker: the pre-hydration class change is still a mismatch; mark it with suppressHydrationWarning. - async-api-routes: mark early-started promises handled and return before dependent work when authentication fails. - advanced-event-handler-refs: Effect Events are not stable references. The skill now records these local corrections against the upstream revision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AmitAvital1
left a comment
AmitAvital1
left a comment
There was a problem hiding this comment.
Five previous threads are resolved. Two earlier issues remain incomplete, and three new line-level correctness issues were found in the full re-review.
- rendering-hydration-no-flicker: the title and label no longer claim the pattern avoids a hydration mismatch; the mismatch is marked as intentional. - server-cache-lru: only an unexported loader touches the shared cache, and the exported function authorizes the caller before any cached value. - js-cache-storage: invalidation resets the cookie cache, cookie values may contain "=", and the rule explains that cookie changes raise no event. - bundle-preload: removes the typeof window guard and its false bundling claim, matching bundle-conditional. - server-after-nonblocking: awaits the logging in after(), stops logging the session cookie, and notes that after() work is lost if the process stops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| tags: advanced, hooks, useEffectEvent, refs, optimization | ||
| --- | ||
|
|
||
| ## useEffectEvent for Stable Callback Refs |
There was a problem hiding this comment.
useEffectEvent is still described here as producing a stable callback, but React says its function identity intentionally changes every render. This contradicts the correction in advanced-effect-event-deps.md and can teach the same incorrect contract again.
| ```typescript | ||
| async function handleRequest(userId: string, skipProcessing: boolean) { | ||
| const userData = await fetchUserData(userId) | ||
|
|
There was a problem hiding this comment.
This PR currently fails git diff --check: this trailing whitespace appears here and across many of the new rule files. The generated guidance should land without whitespace errors, especially because the normal Python-only checks do not catch them.
AmitAvital1
left a comment
AmitAvital1
left a comment
There was a problem hiding this comment.
Four reported issues are fixed and resolved. The re-review found two remaining correctness inconsistencies plus a repository-wide whitespace failure in the added rule files; details are in the line threads.
This branch has not been deployed
2 participants
Register Vercel React Best Practices in the canonical .ai skill system. Vendor 70 upstream rules and generate the Codex and Claude adapters.