web-next: Stop the <Show> stale-read race with a Deno lint plugin#284
Conversation
Solid's non-keyed `<Show when={x}>{(value) => ...}` passes a guarded
accessor that throws "Stale read from <Show>" if invoked while the
condition is falsy. solid-relay publishes fragment snapshots inside
`batch()`, so descendant reactive computations sharing dependencies
with the gated field can race with the Show's own re-evaluation and
trip that throw — observed in production as Sentry WEB-NEXT-4 on the
profile articles page.
Address it structurally with a custom Deno lint plugin
(`web-next/lint-plugins/keyed-show.ts`) that flags non-keyed `<Show>`
and `<Match>` whose children is a function with arity ≥ 1. Autofix
inserts `keyed` and rewrites bare `param()` calls in the body to
`param` so the body type-checks under the value form. Reconcile keeps
record identity stable (`key: "__id"`), so `keyed` only re-mounts on
actual record changes.
The autofix is scope-aware: it walks function ancestors to identify
which arrow owns each `param()` call, and conservatively skips the
body rewrite whenever any same-name binding (`const`/`let`/`var`,
`catch`, nested function param, function/class id, class static
block) exists inside the children's body. Only `keyed` (shorthand)
and `keyed={true}` count as already keyed; `keyed={false}` and
`keyed={someVar}` are still flagged but not autofixed.
Plugin registered under `lint.plugins` in `web-next/deno.jsonc` and
validated by 16 unit tests using `Deno.lint.runPlugin`. Running
`deno lint --fix` cleared 144 violations across the tree;
`SearchResults`'s `$posts` prop type was adjusted from
`Accessor<...$key>` to the value form to match the keyed callsites.
Fixes WEB-NEXT-4
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5
The previous version of the rule flagged any non-keyed `<Show>`/`<Match>`
with a function child, regardless of where the gated value came from.
That was overly aggressive: the stale-read race that motivates the rule
is specific to solid-relay's `batch()`-published fragment snapshots, and
forcing `keyed` on plain Solid signals just adds remount churn whenever
the value's identity changes without a real benefit.
Tighten the rule with a small scope analysis:
* Track variables initialised via solid-relay primitives
(`createPreloadedQuery`, `createFragment`,
`createPaginationFragment`, `createRefetchableFragment`,
`createLazyLoadQuery`, `createSubscription`, `createQueryLoader`),
including namespace imports (`relay.createFragment(...)`).
* Push/pop scopes on Program / Function* / ArrowFunction so a binding
only counts inside its declaring function.
* Propagate Relay-backed-ness into the first param of a keyed
Show/Match callback whose own `when` was Relay-backed, so nested
Show patterns over the same record graph are still caught.
* The `when` walker only descends into expression positions (skips
function bodies, non-computed member properties, etc.) so that
unrelated identifiers in the same JSX subtree don't leak in.
Tests grow from 16 to 20 cases, covering: Relay-backed Show flagged,
plain Solid signal not flagged, plain prop accessor not flagged,
namespace import recognised, lexical scoping (binding in one function
not visible in a sibling), and Relay propagation through outer keyed
Show callback param.
Also revert three keyed conversions that the previous over-aggressive
rule applied to non-Relay sites:
* `components/ui/combobox.tsx`: `local.children` is a Solid prop, not
a Relay value.
* `lib/i18n/index.tsx`: `i18n()` is from a context.
* `lib/mentionHoverCards.tsx`: `props.state.lookup()` is a derived
signal.
`deno lint` (with the tightened rule) is clean across `web-next/`,
the plugin's 20 tests pass, and `deno task check` is clean.
Assisted-by: Claude Code:claude-opus-4-7
Assisted-by: Codex:gpt-5
|
@codex review |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Deno lint plugin that detects Relay-backed Solid ChangesLint plugin + registration
Application of keyed renders and value-based access
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a66ef4d3c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Deno lint plugin to enforce the use of and for values derived from Relay primitives in Solid.js components, aiming to prevent "Stale read from " errors. The changes include the plugin implementation, tests, and a codebase-wide application of the new rule. Feedback indicates that the plugin's propagation logic should be extended to support FunctionExpression children in addition to ArrowFunctionExpression to ensure consistent enforcement across different function syntaxes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-next/src/components/notification/NotificationMessage.tsx (1)
20-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winJSDoc examples are stale — they still call
notification()as an accessor.The component now consumes
$notificationas a fragment ref but the docstring examples still show$notification={notification()}. After this PR, callers likeFollowNotificationCard.tsx(line 30) pass$notification={notification}from inside a keyed<Show>callback. The examples should reflect the new pattern so future contributors don't re-introduce the stale-read race.📝 Proposed doc fix
/** * Generic notification message renderer. * * `@example` Basic usage (e.g., Mention) - * <NotificationMessage - * singleActorMessage={t`${"ACTOR"} mentioned you`} - * multipleActorMessage={t`${"ACTOR"} and ${"COUNT"} others mentioned you`} - * $notification={notification()} - * /> + * <Show keyed when={notification()}> + * {(notification) => ( + * <NotificationMessage + * singleActorMessage={t`${"ACTOR"} mentioned you`} + * multipleActorMessage={t`${"ACTOR"} and ${"COUNT"} others mentioned you`} + * $notification={notification} + * /> + * )} + * </Show> * * `@example` Extra placeholder (e.g., React) - * <NotificationMessage - * singleActorMessage={t`${"ACTOR"} reacted to your post with ${"EMOJI"}`} - * multipleActorMessage={t`${"ACTOR"} and ${"COUNT"} others reacted to your post with ${"EMOJI"}`} - * $notification={notification()} - * additionalValues={{ EMOJI: () => emojiElement() }} - * /> + * <Show keyed when={notification()}> + * {(notification) => ( + * <NotificationMessage + * singleActorMessage={t`${"ACTOR"} reacted to your post with ${"EMOJI"}`} + * multipleActorMessage={t`${"ACTOR"} and ${"COUNT"} others reacted to your post with ${"EMOJI"}`} + * $notification={notification} + * additionalValues={{ EMOJI: () => emojiElement() }} + * /> + * )} + * </Show> */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/components/notification/NotificationMessage.tsx` around lines 20 - 37, The JSDoc examples for NotificationMessage are stale because they show passing notification() as an accessor; update the examples to pass the fragment ref/variable itself (use $notification={notification} instead of $notification={notification()}) and adjust any example caller text accordingly (e.g., keep singleActorMessage, multipleActorMessage and additionalValues the same but show $notification={notification} as used in FollowNotificationCard and keyed <Show> callbacks) so docs reflect the new $notification prop usage in the NotificationMessage component.
🧹 Nitpick comments (5)
web-next/src/components/RemoteFollowButton.tsx (2)
248-255: 💤 Low valueMinor: redundant non-null assertions on
info.software.Line 249's
<Show when={info.software && info.software !== "unknown"}>already narrowsinfo.softwareto a non-null string, but lines 252-253 still useinfo.software!. This is because non-keyed<Show>with a non-function child does not narrow the type into the body. Either:
- Convert to a function child to get proper narrowing, e.g.
<Show when={...}>{(software) => <p>{software.charAt(0).toUpperCase() + software.slice(1)}</p>}</Show>— note this would then needkeyedonly if narrowing via accessor matters; hereinfo.softwareis a captured value so non-keyed function child is fine.- Or leave the
!assertions — they are valid since the narrowing argument is the same expression.Not blocking; flagging only because cleaning this up removes the assertions and makes the body easier to read.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/components/RemoteFollowButton.tsx` around lines 248 - 255, The Show wrapper currently narrows info.software but the JSX body still uses non-null assertions (info.software!) — either convert the Show to a function child to get proper type narrowing (e.g., <Show when={info.software && info.software !== "unknown"}>{(software) => <p>{software.charAt(0).toUpperCase() + software.slice(1)}</p>}</Show>) so you can use the provided software param without (!) or keep the current non-keyed Show and remove the redundant ! only if you re-evaluate that the same expression is used; update the Show usage around info.software in RemoteFollowButton so the body no longer uses info.software! (or switch to the function child and use the local software param).
186-188: 💤 Low valueVerify intentional
keyedon a non-Relay local signal.
actorInfo(line 71) is a plaincreateSignalpopulated bysetActorInfo(result.lookupRemoteFollower)after a one-shotfetchQuery. It is not asolid-relayprimitive subscribed to the store, so the documented "Relay publishes insidebatch()and flips fields to null" race shouldn't apply here. Per the PR objectives, the tightened lint rule limits keyed to Relay-backed values, and three non-Relay files were reverted.The conversion is functionally benign (signal identity changes on every
setActorInfo, so keyed and non-keyed behave equivalently here), but it does deviate from the rule's stated scope. Confirm whether this file was intentionally kept keyed (e.g., defense-in-depth, or because the lint plugin still treats it as Relay-backed via theWebFingerResulttyping) or whether it should also have been reverted alongside the other three.Also applies to: 231-231, 256-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/components/RemoteFollowButton.tsx` around lines 186 - 188, The Show component is marked with keyed while its condition actorInfo() is a plain createSignal set via setActorInfo(result.lookupRemoteFollower), so remove the keyed prop from the Show usage (and the other occurrences at the same file) to match the lint rule and other reverts; if keyed was intentionally kept, instead add an explicit inline comment documenting the reason (defense-in-depth or Relay-typing false-positive) or adjust the WebFingerResult typing so the lint plugin no longer treats actorInfo as Relay-backed, or add a narrowly scoped eslint-disable with a comment referencing actorInfo and setActorInfo.web-next/src/components/NotificationActor.tsx (1)
28-37: 💤 Low valueOptional: simplify the
Notificationtype.
Exclude<ReturnType<typeof notification>, undefined>works, butNonNullable<ReturnType<typeof notification>>is more idiomatic and additionally excludesnull. Not blocking.♻️ Proposed refactor
- type Notification = Exclude< - ReturnType<typeof notification>, - undefined - >; + type Notification = NonNullable<ReturnType<typeof notification>>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/components/NotificationActor.tsx` around lines 28 - 37, Replace the current Notification typedef that uses Exclude<ReturnType<typeof notification>, undefined> with the more idiomatic NonNullable<ReturnType<typeof notification>>; update the type alias referenced by the Notification symbol so it excludes both undefined and null (this will also cover any uses like the firstActor(notification: Notification) helper).web-next/lint-plugins/keyed-show.test.ts (2)
13-18: ⚡ Quick winMissing test coverage for several of the Relay primitives the rule tracks.
The PR description lists
createPaginationFragment,createRefetchableFragment,createLazyLoadQuery,createSubscription, andcreateQueryLoaderas tracked primitives, but none of them appear in any test fixture.createFragmentis imported inRELAY_PRELUDEbut is only exercised via namespace access (relay.createFragmentat line 340), not as a named binding. A regression in recognition of those primitives would go undetected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/lint-plugins/keyed-show.test.ts` around lines 13 - 18, Add missing test cases that import and exercise the Relay primitives listed in the PR so the rule’s recognition is covered: add named imports and usage fixtures for createPaginationFragment, createRefetchableFragment, createLazyLoadQuery, createSubscription, and createQueryLoader in keyed-show.test.ts (similar to how RELAY_PRELUDE defines loadQuery/Q), and ensure createFragment is also exercised as a named binding (not only via relay.createFragment) so regressions in detection are caught; update or add test inputs that call each function (or reference them) so the lint rule sees them as direct named imports.
122-176: 💤 Low valueInconsistent non-null assertion on
fixacross autofix tests.
diagnostics[0].fix!.lengthuses!as iffixmight beundefined, but the Deno lint API guaranteesfixis always aFixData[](possibly empty). The tests at lines 252 and 270 correctly useassertEquals(diagnostics[0].fix, [])without!. The!on the other assertions is harmless but misleading.✨ Suggested cleanup
- assertEquals(diagnostics[0].fix!.length, 3); + assertEquals(diagnostics[0].fix.length, 3);Apply the same removal across lines 159, 175, 192, 213, and 234.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/lint-plugins/keyed-show.test.ts` around lines 122 - 176, Remove the unnecessary non-null assertion (!) on diagnostics[0].fix in the autofix tests: change usages like diagnostics[0].fix!.length to diagnostics[0].fix.length (or compare diagnostics[0].fix to [] where appropriate) in the Deno.test blocks such as "autofix adds keyed and rewrites bare param() calls", "autofix skips param() calls inside nested fn that rebinds the name", "autofix preserves param() calls passing arguments" and the other autofix test assertions so all tests consistently treat fix as a guaranteed array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-next/lint-plugins/keyed-show.ts`:
- Around line 66-79: The rule currently treats Relay-backed names as living for
an entire function/program by only managing scopes at function boundaries;
adjust scope management so lexical block scopes are tracked instead: use
pushScope/popScope around BlockStatement, Program, SwitchCase, CatchClause (and
any other lexical boundaries you traverse) so addToCurrentScope(name) adds to
the innermost block, and ensure isRelayBacked(name) walks those nested sets to
respect shadowing; also make sure
variable/parameter/FunctionDeclaration/VariableDeclarator visitors add names to
the current scope (not an outer scope) so redeclarations shadow outer
Relay-backed names and popping occurs on block exit.
- Around line 101-130: The propagation logic currently runs only for
ArrowFunctionExpression; extend the same handling to FunctionExpression so
regular function children (e.g., function (value) { ... }) get their first
parameter marked Relay-backed when used as children of a TARGET_TAGS JSX (use
the same checks: ensure parent is JSXExpressionContainer, parent.parent is
JSXElement, inspect openingElement.name against TARGET_TAGS,
getWhenExpression(opening) and compute outerSays via expressionIsRelayBacked
against the outer scope), and call addToCurrentScope(firstParam.name) when
firstParam.type === "Identifier"; also ensure you push/pop scopes consistently
(matching pushScope and popScope like the ArrowFunctionExpression handler) and
mirror the "ArrowFunctionExpression:exit": popScope behavior for
FunctionExpression.
- Around line 303-313: isRelayPrimitiveCall currently flags any identifier named
in RELAY_PRIMITIVES (or member prop) as Relay-backed; update it to verify that
the referenced identifier actually comes from a "solid-relay" import before
returning true. Modify isRelayPrimitiveCall to, given the call node and the
current scope/root, resolve the callee identifier (or member object identifier)
back to its ImportDeclaration and only return true if that import source ===
"solid-relay" (or resolves to an imported binding from that module); keep the
existing checks for Identifier vs MemberExpression and RELAY_PRIMITIVES but add
the import-origin resolution step so local helpers or other namespaces are not
misclassified. Ensure the logic handles both direct imports (import {
createFragment } from "solid-relay") and namespaced/member usages
(relay.createFragment) by checking the binding/source of the identifier used as
callee.object or callee itself.
In `@web-next/src/components/PostControls.tsx`:
- Around line 274-276: The BookmarkButton is receiving the resolved fragment
data (`note`) but expects a fragment key of type BookmarkButton_post$key; change
the prop to pass the original fragment key (`props.$post`) instead of `note` so
the fragment masking contract is preserved (update the BookmarkButton invocation
that currently sets $post={note} to use $post={props.$post} while leaving
bookmarkListConnections={props.bookmarkListConnections} unchanged).
---
Outside diff comments:
In `@web-next/src/components/notification/NotificationMessage.tsx`:
- Around line 20-37: The JSDoc examples for NotificationMessage are stale
because they show passing notification() as an accessor; update the examples to
pass the fragment ref/variable itself (use $notification={notification} instead
of $notification={notification()}) and adjust any example caller text
accordingly (e.g., keep singleActorMessage, multipleActorMessage and
additionalValues the same but show $notification={notification} as used in
FollowNotificationCard and keyed <Show> callbacks) so docs reflect the new
$notification prop usage in the NotificationMessage component.
---
Nitpick comments:
In `@web-next/lint-plugins/keyed-show.test.ts`:
- Around line 13-18: Add missing test cases that import and exercise the Relay
primitives listed in the PR so the rule’s recognition is covered: add named
imports and usage fixtures for createPaginationFragment,
createRefetchableFragment, createLazyLoadQuery, createSubscription, and
createQueryLoader in keyed-show.test.ts (similar to how RELAY_PRELUDE defines
loadQuery/Q), and ensure createFragment is also exercised as a named binding
(not only via relay.createFragment) so regressions in detection are caught;
update or add test inputs that call each function (or reference them) so the
lint rule sees them as direct named imports.
- Around line 122-176: Remove the unnecessary non-null assertion (!) on
diagnostics[0].fix in the autofix tests: change usages like
diagnostics[0].fix!.length to diagnostics[0].fix.length (or compare
diagnostics[0].fix to [] where appropriate) in the Deno.test blocks such as
"autofix adds keyed and rewrites bare param() calls", "autofix skips param()
calls inside nested fn that rebinds the name", "autofix preserves param() calls
passing arguments" and the other autofix test assertions so all tests
consistently treat fix as a guaranteed array.
In `@web-next/src/components/NotificationActor.tsx`:
- Around line 28-37: Replace the current Notification typedef that uses
Exclude<ReturnType<typeof notification>, undefined> with the more idiomatic
NonNullable<ReturnType<typeof notification>>; update the type alias referenced
by the Notification symbol so it excludes both undefined and null (this will
also cover any uses like the firstActor(notification: Notification) helper).
In `@web-next/src/components/RemoteFollowButton.tsx`:
- Around line 248-255: The Show wrapper currently narrows info.software but the
JSX body still uses non-null assertions (info.software!) — either convert the
Show to a function child to get proper type narrowing (e.g., <Show
when={info.software && info.software !== "unknown"}>{(software) =>
<p>{software.charAt(0).toUpperCase() + software.slice(1)}</p>}</Show>) so you
can use the provided software param without (!) or keep the current non-keyed
Show and remove the redundant ! only if you re-evaluate that the same expression
is used; update the Show usage around info.software in RemoteFollowButton so the
body no longer uses info.software! (or switch to the function child and use the
local software param).
- Around line 186-188: The Show component is marked with keyed while its
condition actorInfo() is a plain createSignal set via
setActorInfo(result.lookupRemoteFollower), so remove the keyed prop from the
Show usage (and the other occurrences at the same file) to match the lint rule
and other reverts; if keyed was intentionally kept, instead add an explicit
inline comment documenting the reason (defense-in-depth or Relay-typing
false-positive) or adjust the WebFingerResult typing so the lint plugin no
longer treats actorInfo as Relay-backed, or add a narrowly scoped eslint-disable
with a comment referencing actorInfo and setActorInfo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d475f72-1f1f-465f-ba42-9281d20bc8b3
📒 Files selected for processing (91)
web-next/deno.jsoncweb-next/lint-plugins/keyed-show.test.tsweb-next/lint-plugins/keyed-show.tsweb-next/src/app.tsxweb-next/src/components/ActorArticleList.tsxweb-next/src/components/ActorFollowerList.tsxweb-next/src/components/ActorFollowingList.tsxweb-next/src/components/ActorHoverCardLoader.tsxweb-next/src/components/ActorNoteList.tsxweb-next/src/components/ActorPostList.tsxweb-next/src/components/ActorPreviewCard.tsxweb-next/src/components/ActorSharedPostList.tsxweb-next/src/components/AppSidebar.tsxweb-next/src/components/ArticleCard.tsxweb-next/src/components/ArticleControls.tsxweb-next/src/components/BookmarkButton.tsxweb-next/src/components/Bookmarks.tsxweb-next/src/components/DocumentView.tsxweb-next/src/components/EmojiReactionPopover.tsxweb-next/src/components/FollowButton.tsxweb-next/src/components/LinkPreview.tsxweb-next/src/components/NavigateIfHandleIsNotCanonical.tsxweb-next/src/components/NoteCard.tsxweb-next/src/components/NoteCardInternal.tsxweb-next/src/components/NoteComposer.tsxweb-next/src/components/NoteHeader.tsxweb-next/src/components/NoteMedia.tsxweb-next/src/components/NotificationActor.tsxweb-next/src/components/NotificationCard.tsxweb-next/src/components/NotificationList.tsxweb-next/src/components/PersonalTimeline.tsxweb-next/src/components/PostAvatar.tsxweb-next/src/components/PostCard.tsxweb-next/src/components/PostControls.tsxweb-next/src/components/PostSharer.tsxweb-next/src/components/ProfileCard.tsxweb-next/src/components/ProfileTabs.tsxweb-next/src/components/PublicTimeline.tsxweb-next/src/components/QuestionCard.tsxweb-next/src/components/QuotedNoteCard.tsxweb-next/src/components/QuotedPostCard.tsxweb-next/src/components/RemoteFollowButton.tsxweb-next/src/components/SearchGuide.tsxweb-next/src/components/SearchResults.tsxweb-next/src/components/SettingsTabs.tsxweb-next/src/components/SmallProfileCard.tsxweb-next/src/components/admin/AdminAccountsTable.tsxweb-next/src/components/notification/FollowNotificationCard.tsxweb-next/src/components/notification/MentionNotificationCard.tsxweb-next/src/components/notification/NotificationMessage.tsxweb-next/src/components/notification/QuoteNotificationCard.tsxweb-next/src/components/notification/ReactNotificationCard.tsxweb-next/src/components/notification/ReplyNotificationCard.tsxweb-next/src/components/notification/ShareNotificationCard.tsxweb-next/src/routes/(root)/[handle]/(profile)/articles.tsxweb-next/src/routes/(root)/[handle]/(profile)/followers.tsxweb-next/src/routes/(root)/[handle]/(profile)/following.tsxweb-next/src/routes/(root)/[handle]/(profile)/index.tsxweb-next/src/routes/(root)/[handle]/(profile)/notes.tsxweb-next/src/routes/(root)/[handle]/(profile)/shares.tsxweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/[lang].tsxweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/edit.tsxweb-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsxweb-next/src/routes/(root)/[handle]/[noteId].tsxweb-next/src/routes/(root)/[handle]/bookmarks.tsxweb-next/src/routes/(root)/[handle]/drafts/[id].tsxweb-next/src/routes/(root)/[handle]/drafts/index.tsxweb-next/src/routes/(root)/[handle]/drafts/new.tsxweb-next/src/routes/(root)/[handle]/invite/[id].tsxweb-next/src/routes/(root)/[handle]/settings/index.tsxweb-next/src/routes/(root)/[handle]/settings/invite.tsxweb-next/src/routes/(root)/[handle]/settings/language.tsxweb-next/src/routes/(root)/[handle]/settings/passkeys.tsxweb-next/src/routes/(root)/[handle]/settings/preferences.tsxweb-next/src/routes/(root)/admin/index.tsxweb-next/src/routes/(root)/admin/invitations.tsxweb-next/src/routes/(root)/authorize_interaction.tsxweb-next/src/routes/(root)/coc.tsxweb-next/src/routes/(root)/fediverse.tsxweb-next/src/routes/(root)/feed/articles.tsxweb-next/src/routes/(root)/feed/index.tsxweb-next/src/routes/(root)/feed/without-shares.tsxweb-next/src/routes/(root)/index.tsxweb-next/src/routes/(root)/local.tsxweb-next/src/routes/(root)/markdown.tsxweb-next/src/routes/(root)/notifications.tsxweb-next/src/routes/(root)/privacy.tsxweb-next/src/routes/(root)/search.tsxweb-next/src/routes/(root)/sign/up/[token].tsxweb-next/src/routes/(root)/tags/[tag].tsxweb-next/src/routes/(root)/tree.tsx
The keyed-show autofix in 0b5b513 changed two `t\`...\`` template expressions in *web-next/src/routes/(root)/[handle]/[idOrYear]/[slug]/index.tsx* from `${name()}` to `${name}` (both inside `<Show keyed when=...>` callbacks). Lingui extracts a tagged-template placeholder as `{name}` when the embedded expression is a bare identifier and `{0}` when it is any other expression, so the autofix shifted the catalog msgids: "Translating to {0}…" -> "Translating to {name}…" "Translation request failed for {0}" -> "Translation request failed for {name}" Re-running `pnpm extract` updates the catalogs accordingly. This commit copies each existing translation across to the new key, leaving the text identical apart from the placeholder name. Assisted-by: Claude Code:claude-opus-4-7
The previous propagation logic was inlined in the ArrowFunctionExpression
visitor, so `{function (value) { ... }}` children of a Relay-backed
`<Show>`/`<Match>` never had `value` marked Relay-backed in their inner
scope. Nested Show patterns under those callbacks therefore slipped
past the rule.
Extract the propagation step into a `propagateRelayBindingFromShowParent`
helper and call it from both the ArrowFunctionExpression and a new
FunctionExpression visitor, so traditional-function children behave the
same as arrow children. A "flags FunctionExpression children of
Relay-backed Show" test covers it (21 plugin tests, all passing).
hackers-pub#284 (comment)
hackers-pub#284 (comment)
Assisted-by: Claude Code:claude-opus-4-7
isRelayPrimitiveCall used to match any callee named in RELAY_PRIMITIVES regardless of where it came from. Local helpers or other namespaces that happened to expose `createFragment`, `createPreloadedQuery`, etc. were misclassified as Relay-backed, causing false-positive flags and unsafe autofixes outside the intended surface. Track ImportDeclaration nodes whose source is "solid-relay" and remember each local binding (named or namespace). isRelayPrimitiveCallResolved now requires the callee identifier (or member-object identifier) to resolve through one of those bindings before returning true. Removes the old standalone isRelayPrimitiveCall helper that did the bare-name match. Two new tests cover the recognition surface: one imports every tracked primitive and asserts each is flagged on a Relay-backed Show, one declares a same-named `createFragment` from a local module and asserts it is not flagged. 23 plugin tests pass. hackers-pub#284 (comment) hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
Two leftovers from the keyed-show migration (caught in the PR review's
outside-diff / nitpick section):
- NotificationActor.tsx: replace `Exclude<ReturnType<typeof
notification>, undefined>` with the more idiomatic
`NonNullable<ReturnType<typeof notification>>`. Equivalent in this
context (createFragment returns `T | undefined`), reads more cleanly.
- NotificationMessage.tsx: the two JSDoc `@example` blocks still showed
`$notification={notification()}` (accessor form), but every real
caller in components/notification/ now passes the keyed value
(`$notification={notification}`). Update the examples to match.
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88516d9d67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request implements a custom Deno lint plugin, show-keyed-on-fn-child, to enforce the keyed attribute on Solid's <Show> and <Match> components when using Relay-backed values. This prevents 'Stale read from ' errors by ensuring function children receive values directly rather than through guarded accessors. The PR includes tests for the new rule and applies the autofix across the web-next codebase, updating numerous components and routes. Translation files were also updated accordingly. I have no feedback to provide.
isRelayPrimitiveCallResolved gated the identifier-callee branch on
RELAY_PRIMITIVES.has(callee.name) before consulting the import map, so
`import { createFragment as frag } from "solid-relay"; const data =
frag(...)` was treated as non-Relay (the local name `frag` is not in
RELAY_PRIMITIVES), and unsafe non-keyed Show usage on `data` slipped
past the rule.
Look the local binding up in the import map first and gate on
RELAY_PRIMITIVES.has(imp.imported) instead. The check now matches the
import's *original* name regardless of the local alias, while still
requiring it to be one of the tracked primitives. Same shape as the
existing namespace-import check (which already keyed off the property
name).
A "recognises aliased solid-relay imports" test covers it (24 plugin
tests, all passing).
hackers-pub#284 (comment)
Assisted-by: Claude Code:claude-opus-4-7
The CallExpression visitor short-circuited on `node.optional === true`, so `value?.()` calls inside a Show callback were left in place while `keyed` was still inserted on the opening element. After the keyed conversion, `value` is a concrete value rather than an accessor function, and calling it (`value?.()`) would attempt to invoke a non-callable at runtime. Drop the optional-call guard. Optional and non-optional bare calls of the param both get replaced with the bare param identifier; the CallExpression node's range covers the whole `value?.()` text, so the existing replaceTextRange handles both forms uniformly. A new "autofix rewrites optional bare param?.() calls too" test exercises the case (25 plugin tests, all passing). hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web-next/lint-plugins/keyed-show.test.ts (1)
122-139: ⚡ Quick winAssert transformed output, not just fix count
Autofix tests currently validate only
fix!.length, which can miss incorrect edit text/ranges. Please add assertions on the post-fix source (or at least applied edit snippets) for representative cases likeactor()→actorandvalue?.()→value.Also applies to: 141-183, 185-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/lint-plugins/keyed-show.test.ts` around lines 122 - 139, Update the test "autofix adds keyed and rewrites bare param() calls" to assert the actual transformed source after applying the suggested fixes (not just diagnostics[0].fix!.length): take the original input string used in Deno.test, apply the diagnostics' fixes to produce the post-fix source, and add assertions that occurrences like "actor()" were rewritten to "actor" and optional-call forms like "value?.()" were rewritten to "value"; do the same pattern for the other affected tests referenced (the ones around the other ranges) so each test verifies the edited text/snippets produced by applying the fixes from the diagnostics rather than only counting edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-next/src/locales/zh-CN/messages.po`:
- Around line 264-270: The two adjacent translation entries for the error
messages differ in how “contact support” is translated—update the msgstr for the
msgid "An error occurred while saving your preferred languages. Please try
again, or contact support if the problem persists." (in
src/routes/(root)/[handle]/settings/language.tsx) to use the same
glossary-approved term as the other entry (change "联系支持" to "联系客服") so both
messages use identical wording; then run your i18n/l10n checks (or build) to
ensure no formatting/PO errors.
---
Nitpick comments:
In `@web-next/lint-plugins/keyed-show.test.ts`:
- Around line 122-139: Update the test "autofix adds keyed and rewrites bare
param() calls" to assert the actual transformed source after applying the
suggested fixes (not just diagnostics[0].fix!.length): take the original input
string used in Deno.test, apply the diagnostics' fixes to produce the post-fix
source, and add assertions that occurrences like "actor()" were rewritten to
"actor" and optional-call forms like "value?.()" were rewritten to "value"; do
the same pattern for the other affected tests referenced (the ones around the
other ranges) so each test verifies the edited text/snippets produced by
applying the fixes from the diagnostics rather than only counting edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d265d0a-795a-4487-9efc-2e55a229e8d4
📒 Files selected for processing (9)
web-next/lint-plugins/keyed-show.test.tsweb-next/lint-plugins/keyed-show.tsweb-next/src/components/NotificationActor.tsxweb-next/src/components/notification/NotificationMessage.tsxweb-next/src/locales/en-US/messages.poweb-next/src/locales/ja-JP/messages.poweb-next/src/locales/ko-KR/messages.poweb-next/src/locales/zh-CN/messages.poweb-next/src/locales/zh-TW/messages.po
✅ Files skipped from review due to trivial changes (3)
- web-next/src/locales/ja-JP/messages.po
- web-next/src/locales/zh-TW/messages.po
- web-next/src/locales/ko-KR/messages.po
🚧 Files skipped from review as they are similar to previous changes (2)
- web-next/src/components/NotificationActor.tsx
- web-next/lint-plugins/keyed-show.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92dad288d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Deno lint plugin, show-keyed-on-fn-child, to enforce the keyed attribute on Solid's <Show> and <Match> components when using solid-relay primitives. This architectural improvement prevents potential "Stale read" runtime errors by ensuring children receive direct values rather than guarded accessors. The PR includes a robust test suite for the plugin and migrates a large number of components to comply with the new rule, replacing accessor calls with direct property access. I have no feedback to provide as no review comments were submitted.
Three settings pages each had a "contact support if the problem persists" error toast, but the zh-CN and zh-TW translations had drifted to three different renderings: preferences.tsx -> 联系支持团队 / 聯繫支援團隊 language.tsx -> 联系支持 / 聯繫支援 index.tsx -> 联系客服 / 聯繫客服 Standardise all three on the shorter form (联系支持 / 聯繫支援) so the adjacent toasts read consistently. ja-JP (サポート) and ko-KR (지원팀) were already internally consistent and are left unchanged. hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
The fixer used to insert `keyed` even when `entry.bodyHasRebinding` was
true (the body contains a const/let/var/catch/nested function/class id
binder for the same identifier as the keyed param). It would just skip
the param() call rewrites in that case. That left a partial autofix on
disk: any non-shadowed `value()` call elsewhere in the body would, after
the keyed conversion, try to invoke the now-keyed concrete value as a
function and crash at runtime.
Treat bodyHasRebinding the same way as a non-truthy `keyed={...}` attr:
report the diagnostic but offer no fix. The reviewer keeps the warning,
makes the call themselves, and intervenes manually instead of trusting
a partial mechanical rewrite. The two existing shadow tests now assert
`fix === []`; the nested-rebind test was renamed and re-described to
match the new behaviour. 25 plugin tests pass.
hackers-pub#284 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Deno lint plugin, 'show-keyed-on-fn-child', to enforce the 'keyed' attribute on Solid's and components when using Relay-backed values, preventing 'Stale read' errors. It includes a large-scale refactor of components in the 'web-next' directory and updates to translation files. Feedback on the plugin implementation recommends extending scope tracking to include block-level scoping for better accuracy and replacing the manual AST traversal map with a more maintainable visitor pattern to ensure comprehensive node coverage.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5750ca6c95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Deno lint plugin, show-keyed-on-fn-child, to enforce the use of the keyed attribute in Solid's <Show> and <Match> components when handling values from solid-relay primitives. This change prevents "Stale read from " runtime errors by ensuring children receive direct values instead of guarded accessors. The PR includes the plugin implementation, comprehensive tests, and a codebase-wide migration. Feedback on the plugin implementation suggests enhancing the autofix safety by detecting non-standard parameter usages, including OptionalCallExpression in the visitor, and accounting for parameter reassignment in the shadowing detection logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37f19a642a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The CallExpression visitor only collected zero-argument calls of the
keyed-Show callback param into the rewrite list and silently dropped
calls with arguments. After the keyed flip, `value` is a concrete
record rather than an accessor, so leaving `value(arg)` untouched would
attempt to invoke a non-callable at runtime. TypeScript would normally
catch `value(arg)` because Solid's accessor type is `() => T`, but the
rule should not depend on the user's type-checking discipline to stay
safe.
Track non-zero-argument calls of the param under a new
`hasUnsafeParamUse` flag on the ShowEntry, and treat the flag the same
way as `bodyHasRebinding` and a non-truthy `keyed={...}` attribute:
report the diagnostic but offer no fix. The previous "preserves
param() calls passing arguments" test (which expected `keyed` alone to
still be inserted) is renamed to "suppresses autofix when body calls
the param with arguments" and now asserts `fix === []`. 30 plugin
tests pass.
hackers-pub#284 (comment)
Assisted-by: Claude Code:claude-opus-4-7
detectRebinding looked for declaration-style binders (VariableDeclarator, CatchClause, function/class id, function param) but ignored an AssignmentExpression like `value = compute()` that overwrites the callback param itself. After such an assignment the identifier no longer refers to the original keyed value, so any later `value()` call would, after the keyed flip, invoke whatever the assignment stored rather than the keyed value the rule promised. Reassigning a param is unusual, but the rule should not silently produce a wrong rewrite if someone does it. Add an `AssignmentExpression` check (left side is an Identifier with the param name) to detectRebinding's binder list. The flag flows into the existing "suppress autofix on body rebinding" path, so the diagnostic is reported but no fix is offered. A new "flags but suppresses autofix on assignment to the param" test covers it (31 plugin tests, all passing). hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Deno lint plugin, show-keyed-on-fn-child, which enforces the use of the keyed attribute on Solid's <Show> and <Match> components when they are used with values from solid-relay primitives. This change is designed to prevent "Stale read from " runtime errors by ensuring that child functions receive values directly rather than through guarded accessors. The PR includes extensive tests for the plugin and updates numerous components and translation files across the web-next stack to adhere to this new pattern. Feedback on the plugin implementation suggests that the CallExpression visitor should be expanded to explicitly handle OptionalCallExpression nodes to ensure that optional calls like param?.() are correctly identified and handled by the autofix logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6c497df20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-next/lint-plugins/keyed-show.ts`:
- Around line 325-358: The CallExpression handler incorrectly computes
replacement ranges for optional calls (param?.()) because it uses the
CallExpression node range which covers only the parentheses; update the logic in
the CallExpression visitor (the block that inspects calleeName, argCount and
pushes to entry.calls / sets entry.hasUnsafeParamUse) to detect optional
chaining and select the correct range: if the CallExpression is wrapped in a
ChainExpression (node.parent.type === "ChainExpression") use the parent's range
for replacement, or alternatively check node.optional === true and expand the
replacement to include the optional chaining token; ensure the shows map
entry.calls stores the node/range that includes the ChainExpression when the
calleeName matches and argCount === 0 and !entry.bodyHasRebinding so
replacements produce valid syntax.
- Around line 191-205: The enclosing-function walk in the call-site shadow check
misses named function self-bindings, so add a check for the function's own id to
avoid false positives: inside the loop that inspects enclosing functions (the
block using cursor, checking isFn), before calling
paramRebindsName(cursor.params, bindingName) and detectRebinding(cursor.body,
bindingName), test if cursor.id?.name === bindingName and return false if true;
this uses the existing symbols bindingName, cursor, paramRebindsName and
detectRebinding and prevents treating a function's own name (from a
FunctionExpression or FunctionDeclaration) as the imported binding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eb3406d-acce-46c9-adda-d215e01868cc
📒 Files selected for processing (4)
web-next/lint-plugins/keyed-show.test.tsweb-next/lint-plugins/keyed-show.tsweb-next/src/locales/zh-CN/messages.poweb-next/src/locales/zh-TW/messages.po
✅ Files skipped from review due to trivial changes (3)
- web-next/src/locales/zh-TW/messages.po
- web-next/src/locales/zh-CN/messages.po
- web-next/lint-plugins/keyed-show.test.ts
The shadow tracking from the previous round only created scopes at function and program boundaries, so block-scoped `let`/`const` declarations and catch parameters were recorded into the enclosing function's scope. Combined with the "don't downgrade relay to shadow" guard inside `recordBinding`, this meant that an inner block-level re-declaration of a Relay-backed name was silently dropped, leaving the outer "relay" binding visible inside the block. The Show inside that inner block was then incorrectly flagged as Relay-backed. Push a fresh scope on BlockStatement entry (skipping the case where the parent is a function, since the function visitor already pushed), and pop it on exit. CatchClause params are hoisted into the just- pushed catch-body scope so `e` shadows outer same-name bindings inside the catch. Two new tests cover the case: an inner if-block shadow that must not be flagged, and a sibling-block shadow that does not bleed into a later Show outside the block. 33 plugin tests pass. hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
isRelayPrimitiveCallResolved walked the full body of every enclosing
function via detectRebinding, which (a) descended into nested function
bodies and (b) ignored the function's own id binding. Both produced
false negatives for the Relay-shadow check and let unsafe non-keyed
<Show>/<Match> usages slip through:
- A `function Helper() { const createPreloadedQuery = ... }` sibling
inside the same App was wrongly counted as shadowing an outer call to
the import, because detectRebinding entered Helper's body looking for
the name.
- A named `function createFragment() { ... createFragment(...) }`
binds its own name in its body (per ECMAScript NamedEvaluation
semantics for FunctionExpression / FunctionDeclaration), so an inner
reference resolves to the function itself rather than the import; the
walk did not check `cursor.id?.name`.
Add an `enterNestedFunctions` option to detectRebinding (default true,
preserves the conservative body-rebinding behaviour), and use
`{ enterNestedFunctions: false }` in isRelayPrimitiveCallResolved so
the body walk stops at nested function boundaries. Also test
`cursor.id?.name === bindingName` for FunctionExpression /
FunctionDeclaration before checking params and body.
Two new tests cover the cases: a sibling Helper that should not
shadow, and a self-binding named FunctionExpression that should
shadow. 35 plugin tests pass.
hackers-pub#284 (comment)
hackers-pub#284 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Deno lint rule, show-keyed-on-fn-child, to enforce the keyed attribute on Solid's <Show> and <Match> components when using solid-relay primitives, preventing potential "Stale read" runtime errors. The PR includes extensive tests and applies this pattern across the web-next codebase, updating components and translation files. The review feedback identifies critical improvements for the lint rule, specifically the need to handle OptionalCallExpression in the visitor and to enhance shadowing detection for destructuring assignments and update expressions to ensure the safety of the autofix.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
deno_ast represents `value?.()` as a `CallExpression` with `optional: true` (wrapped in a `ChainExpression`), so the existing `CallExpression` visitor already catches both forms. Successive re-reviews keep flagging the lack of a top-level `OptionalCallExpression` visitor as a possible source of broken autofixes, however, because the rule already has defensive `OptionalCallExpression` cases in `expressionIsRelayBacked` and the detectRebinding `FIELDS` map. Make the wiring symmetric: extract the call-handling body into a `handleParamCall` closure and call it from both `CallExpression` and a new `OptionalCallExpression` visitor. The new visitor is a no-op on the current AST (deno_ast never produces that node type), but it silences the recurring concern and keeps the rule defensively correct if a future AST revision splits the two node types. 35 plugin tests pass. hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
…ndings
The AssignmentExpression branch in detectRebinding only matched when
the left side was a plain Identifier, so `[value] = compute()` and
`({ value } = compute())` slipped through and the rule still offered
its autofix despite the param being reassigned. Switch to the existing
`bindsName` helper so destructuring patterns count too.
Also handle UpdateExpression: `value++` and `value--` mutate the
binding the same way an assignment does, so any later `value()` call
in the body would invoke the post-mutation value (already not the
keyed value) after the autofix flips the Show to keyed.
Two new tests cover the destructuring-assignment and
update-expression cases. 37 plugin tests pass.
hackers-pub#284 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Deno lint plugin, keyed-show, to enforce the use of the keyed prop on <Show> and <Match> components when handling Relay-backed values. This prevents potential race conditions and 'Stale read' errors. A large-scale refactoring was performed across the web-next codebase to adopt this pattern. Feedback includes a performance optimization for the lint plugin by hoisting a constant mapping to the module scope and a warning regarding the lack of native support for keyed props in standard Solid.js <Match> components.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33593e19a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e walk The previous implementation walked enclosing function bodies via detectRebinding to detect a local shadow of a tracked solid-relay import. That walk descended into sibling blocks within the same function, so a popped block-scoped `const createFragment = ...` was still treated as shadowing call sites *outside* that block. Real solid-relay calls were therefore misclassified as non-Relay, and unsafe non-keyed Show callbacks for them slipped past the rule. Replace the function-wide walk with a `lookupBinding` lookup against the live `scopes` stack. The stack already reflects the call's true lexical scope (with the BlockStatement push/pop work added in an earlier round, sibling blocks have already been popped by the time we visit a call outside them), so a `null` lookup means "no closer binding than the import." Keep a separate small walk for the named FunctionExpression / FunctionDeclaration self-binding case, since the scopes stack does not record those ids. A new "still flags when a sibling block (not the call's scope) shadows the import" test pins the regression. 38 plugin tests pass. hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
The per-node-type child-field map used by detectRebinding to walk the AST manually was declared inside the function body, so the literal was reconstructed on every call. detectRebinding runs once per flagged Show callback and once per import-shadow check, so the allocation was not free. Move the map to a module-level constant DETECT_REBINDING_FIELDS and reference it from inside the walker. No behaviour change. 38 plugin tests pass. hackers-pub#284 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a custom Deno lint plugin, "show-keyed-on-fn-child", to enforce the use of the "keyed" attribute on "Show" and "Match" components when the condition is backed by a "solid-relay" primitive. This change addresses potential "Stale read from " errors caused by reactive races during Relay snapshot updates. The PR includes the plugin implementation, comprehensive tests, and a widespread application of the rule across the "web-next" codebase, including necessary updates to component logic and translation files. I have no feedback to provide as no review comments were submitted.
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Sentry has been reporting
Error: Stale read from <Show>on the profile articles route (issue WEB-NEXT-4), thrown from inside Solid's reactive runtime whilesolid-relaywas publishing a fragment snapshot. One user impacted, one occurrence so far, but the stack trace pointed at a class of latent races worth fixing structurally.Why it happens
Solid's non-keyed
<Show when={x}>{(value) => …}passes a guarded accessor to the children. The accessor checks the condition each time it's called and throwsnarrowedError("Show")if the condition has flipped to falsy.solid-relaypublishes fragment snapshots insidebatch(() => setResult("data", reconcile(...))), so a single store update can dirty both the outer<Show>'s memo and any descendant reactive computation that reads the same fragment field. If the descendant runs before the outer<Show>has a chance to re-render its fallback, the descendant's call tovalue()returns through the throw, andsolid-jscatches it at the nearestErrorBoundary. With keyed<Show>, the children receives the value directly, so the throw never fires. Reconcile preserves record identity (key: "__id"), sokeyedonly re-mounts when the actual record changes (typically: navigating to a different record).The fix
A round of case-by-case
keyedconversions across the profile, settings, notification, and admin trees took the obvious sites out of the race. Continuing that sweep manually was going to be unbounded, so the rest of this PR adds a guardrail: a custom Deno lint plugin in web-next/lint-plugins/keyed-show.ts that flags the unsafe pattern and autofixes it.The rule looks for
<Show>and<Match>whose children is a function with arity ≥ 1. It only flags when the gated value can be traced back to one of the solid-relay primitives (createPreloadedQuery,createFragment,createPaginationFragment,createRefetchableFragment,createLazyLoadQuery,createSubscription,createQueryLoader) within the current lexical scope. Plain Solid signals, props, context values, and other non-Relay reactive sources are left alone, since the race is rare there andkeyedwould just add remount churn whenever the value's identity changes.Most of the plugin is scope tracking. It pushes a new scope on
Program,FunctionDeclaration,FunctionExpression, andArrowFunctionExpression; tracksVariableDeclaratorwhose init is a Relay primitive call (named or via namespace import); and propagates Relay-backed-ness into the first param of a keyed<Show>/<Match>callback whose ownwhenwas already Relay-backed, so chained patterns over the same record graph still get flagged. Lookup walks the scope stack bottom-up, so a binding declared in one function isn't visible from a sibling.The autofix inserts
keyedon the opening element and rewrites bareparam()calls in the children body toparam. The rewrite is conservative: if any same-name lexical binding exists anywhere in the body subtree (aconst/let/var, acatch (param), a nested function param, a function or class id, or a class static block), the body rewrite is skipped and onlykeyedis added, since the bare-call walker doesn't model lexical scope precisely enough to safely rewrite under a shadow. If the element already has a non-truthykeyedattribute (keyed={false}orkeyed={someVar}), no fix is offered at all, since we can't tell which value the user intended.What was applied
Running
deno lint --fixcleared 144 violations across 91 source files, that accounts for most of the diff in this PR. Three of those files turned out to be Solid prop or context plumbing, not Relay (components/ui/combobox.tsx, lib/i18n/index.tsx, lib/mentionHoverCards.tsx); a follow-up commit reverted those after the rule was tightened to be Relay-aware. One real type breakage surfaced from the bulk conversion: components/SearchResults.tsx's$postsprop was typedAccessor<…$key>, which assumed the call site passed a Solid accessor. The two call sites now pass the value directly out of a keyed<Show>, so I changed$poststo take the value form to match.The plugin is registered under
lint.pluginsin web-next/deno.jsonc and validated by 20 unit tests usingDeno.lint.runPlugin(web-next/lint-plugins/keyed-show.test.ts).deno task check(fmt + lint +tsc) is clean.Test plan
deno task checkpasses locally.deno test web-next/lint-plugins/keyed-show.test.tspasses (20 tests)./@nebuleto/articlesand any other profile sub-tab; no Sentry events forStale read from <Show>arrive after this branch ships.deno lint --fixfrom a clean tree; no further changes are produced.Fixes
Closes WEB-NEXT-4.
AI usage
Drafted, debugged, and committed with Claude Code (Opus 4.7); code review on every commit and on this PR description from Codex (GPT-5).