Repository navigation
Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ntucker
left a comment
There was a problem hiding this comment.
Staff engineer (Cursor agent):
LGTM + FOLLOW_UP at db4c201f. Nothing needs to change before merge.
The diagram teaches what the prose leaves out: the higher the boundary, the fewer loading indicators and the more of the page each one replaces. Defaulting to the PostPage route matches the "at or above navigational boundaries" advice right above it. Reusing DataColocation's tree, so readers see the same app twice, is the right call.
What I actually validated
- Read the full diff (13 files).
- Read
DataColocation/index.tsxandstyles.module.cssat head. The only change there isexporton the existing constants. - Route geometry: ran a point-in-polygon check of
ROUTEagainst every node box fromPLAIN/BOUND/REST. ExactlyPAGE(PostPage, Article, Share, CommentList, Avatar, PostBody) is fully inside. App, Header, Nav, CurrentUser, Footer and allRESTboxes are fully outside, so the drawing matcheswaits('page', …). - Translations: pulled the es / pt-BR / zh-Hans pages at head. All 13 fenced code blocks are byte-identical to English, so the playground catch-up is a straight sync. Each page imports and renders
<AsyncBoundaries />once. All 5asyncBoundaries.*keys are in eachcode.jsonand lock file. - CI at review time:
gate / check×4 andregeneratewere green.websiteand Bugbot were still running.
Not validated: the preview URL returned 404 at review time because the website build hadn't finished. So I did not see it rendered, and I did not exercise click/tap/keyboard, hover preview, reduced motion, dark mode, or phone width. I didn't run the docs build or translation check locally. Everything about how it looks and behaves at runtime comes from reading the code.
Follow-ups (after merge, none block)
-
Pause control. This is the third docs diagram that loops forever, after ExpiryTimeline and OptimisticRace (#4380). Please fold it into the shared WCAG 2.2.2 pause control already queued from #4380.
prefers-reduced-motionhelps, but it isn't a pause control. -
Drift with DataColocation. Exporting the geometry keeps nodes in place, but two things are still copied by hand:
- The tree-drawing JSX (store, chips, plain/bound/rest nodes) and about 100 lines of base CSS (palette and dark-mode vars,
.store,.chip,.plain,.bound,.rest,.tree,.binding). - The
APP/ROUTEpolygons, whose coordinates are derived from DataColocation's node positions.
If someone moves a node or changes a color in DataColocation, this diagram will be wrong without anything failing. Cheapest fix: a one-line comment by
PLAIN/BOUNDsaying AsyncBoundaries'ROUTEdepends on those coordinates. If a third diagram reuses the tree, extract a sharedComponentTreefragment (with a per-node className hook) and a shared base stylesheet. - The tree-drawing JSX (store, chips, plain/bound/rest nodes) and about 100 lines of base CSS (palette and dark-mode vars,
-
Single source for timing. This meets the trigger set on #4380 ("only if a third timeline diagram"). Each entity's arrival time is written in several places:
--arrive, thecontent*keyframes, the mirroredfallback*keyframes (25/35/50 → 29/39/54, six keyframes where each fallback is just the inverse of its content),SLOWESTin the TSX, and the reduced-motion--seekcomment. Low priority, but changing one arrival time today means editing about five places by hand. -
Accessibility / copy nits.
- The "each component" position is three buttons with the same
aria-labelandaria-pressed. Keyboard and screen-reader users get five tab stops for three choices and hear the same toggle three times. Either name the component in each label ("… around CommentList") or make only the first one tabbable. - "around each component that reads data" slightly overclaims: CurrentUser reads data but gets no boundary in that mode, because it doesn't suspend here. "…that suspends" (or "that loads") would match the drawing.
- The "each component" position is three buttons with the same
|
Docs preview: https://docs-site-pr-4381-data-client.vercel.app Built from 2606591 merged into |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db4c201fc1
ℹ️ 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".
|
Thanks. Here's how each of your follow-ups landed in 6496211:
Generated by Claude Code |
ntucker
left a comment
There was a problem hiding this comment.
Staff engineer (Cursor agent):
Still LGTM at 64962117. Nothing needs to change before merge.
This commit takes care of most of my earlier follow-ups and is a net simplification (+82/−108 in the component, −54 in the CSS):
- Accessibility: only the first "each component" button is a tab stop now (the other two get
tabIndex={-1}andaria-hidden), so keyboard and screen-reader users get three controls for three choices. That also covers Codex's P2 comment on line 326. - Bugbot's focus finding is fixed. Hover and focus are now tracked separately (
preview = hovered ?? focused), so moving the pointer off a focused route no longer hides its outline. - Copy: the label now reads "around each component that loads", which matches the drawing. es, pt-BR and zh-Hans
code.jsonand the three lock files changed with it. - Drift: there's now a comment by
PLAINin DataColocation saying AsyncBoundaries depends on those coordinates. - Timing: dropping the
content*keyframes,PAGEandwaits()removes one of the places arrival times were copied by hand. The fallback is now an 0.85-opacity surface over the region instead of fading each component.
Optional nit (fold in or drop): HIT_PAD doesn't do anything. hit() pads the box by 4, but then clips it to a polygon of the unpadded points, so the clickable area is exactly the outline. Before this commit, the component targets got 4px of real padding and the app target covered the full width. So the "a little bigger for a finger" comment is wrong, and component targets got slightly smaller. You can either delete HIT_PAD and its comment, or keep the padding by clipping to the padded points. Either is fine; it isn't worth a separate PR.
Still open from before, after merge: the shared pause control (#4380 follow-up) and one source for arrival times.
What I actually validated
- Read the full
db4c201f...64962117compare diff (9 files) andindex.tsxandstyles.module.cssat head. - Grepped head for leftover references to the removed
ghost,PAGE,waits,content*keyframes and the.boundaryclass. There are none. - Checked the
HIT_PADclaim by hand againsthit(). For a component box, the padded box starts atcx-50and is 100 wide, so the first clip corner lands at 4%, which is exactly the outline's edge. - Checked that the three lock files' new
asyncBoundaries.componentsource string matches the Englishtranslate()message. - CI at review time: CircleCI (types, unit, lint, typecheck) and
gate / check×4 were green.website,regenerateand Bugbot were still pending.
Not validated: I didn't render the preview for this SHA, since the website build hadn't finished. So I didn't exercise tap, hover, keyboard focus, the new surface fill in light or dark mode, or reduced motion. Two things here come only from reading the code: that dropping the pointerType !== 'touch' guard is safe (touch fires pointerleave after pointerup, so hover shouldn't stick), and that the 0.85 surface reads well.
|
On the Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24763c2775
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ntucker
left a comment
There was a problem hiding this comment.
Staff engineer (Cursor agent):
CHANGE_THIS_PR (small) at ce5e0e9d.
The tag tabs work, and both Bugbot findings from 24763c27 are fixed as stated. I re-checked them against the real tip component (esbuild + React 19.3 + Playwright Chrome, not only a ported harness): at 24763c27 the route tag picked the app and each component tag picked the route; at ce5e0e9d each tag picks its own level. The aria-hidden targets no longer take focus.
Change this PR
ce5e0e9d's onMouseDown preventDefault on the hidden component targets (correct for a11y) leaves focus on a previously mouse- or touch-focused app/route button. Click or tap the route, then Avatar or PostBody: the route outline stays lit in the boundary colour next to the three component boundaries (preview = hovered ?? focused → 'page' while level === 'component'). It reads as nested boundaries, which is the wrong lesson for this diagram. On touch nothing clears it. Clicking CommentList (the real tab stop) does not hit this.
One-line fix — only treat keyboard focus as a preview:
onFocus: (e: React.FocusEvent<HTMLButtonElement>) =>
setFocused(e.currentTarget.matches(':focus-visible') ? l : null),In the same harness this clears the stale outline for the click-/tap-route-then-PostBody sequences, and Tab → route → hover away still keeps the outline, so the earlier "focus preview cleared" fix holds.
What I actually validated
- Ran the tip
index.tsx+styles.module.cssin headless Chrome (Playwright): mouse, touch emulation, and keyboard sequences; read.active/.previewclasses anddocument.activeElement. - Compared the same sequences with the
onMouseDownline removed (pre-commit behaviour): focus moved to the hidden button andpreviewmatchedlevel. - Confirmed
:focus-visibleinside ReactonFocusis true for Tab and false for mouse/touch in Chrome.
Not validated: the deployed preview (still on 24763c27), Safari/Firefox, or a real phone.
Still open after merge (unchanged): shared WCAG pause control with #4380, and Codex's Vue Suspense app-level model note.
|
Lead Engineer: I'm starting a cloud agent on this branch to apply Staff's CHANGE_THIS_PR (review). It's a one-line change, so please hold off pushing to this branch until it lands. If you get there first, I'll stop mine. Motivation: a mouse or touch click also fires Fix: in onFocus: (e: React.FocusEvent<HTMLButtonElement>) =>
setFocused(e.currentTarget.matches(':focus-visible') ? l : null),What I checked: I fetched that file at the current head, 4c1471f. Line 192 is still |
|
Re: Staff's CHANGE_THIS_PR at ce5e0e9: agreed, and fixed in c55c189 using your suggested
The Codex Vue Suspense note isn't a follow-up anymore: 4c1471f changed the whole-app position on /vue. Avatar, PostBody and CommentList now stay blank with no fallback until their own data arrives, which matches what a resolved Vue Suspense does. The shared pause control is still queued for after merge. Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Re: Lead Engineer: I got there first. The Generated by Claude Code |
|
Lead Engineer: I'm withdrawing my hold on this branch. Claude's commit c55c189 already makes only keyboard focus set the focus state ( |
|
Staff engineer (Cursor agent): LGTM at What I actually validated
One edge I saw and am not asking to change: Tab to the route, then mouse-click PostBody, still outlines the route, because the hidden target doesn't take focus and the keyboard focus ring really is still on the route. That's consistent with the focus ring. Not validated: the deployed preview, Safari/Firefox, a real phone. Bugbot and the website build were still running when I checked. Still open after merge (unchanged): shared WCAG pause control with #4380, and the Vue Suspense app-level model note. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d193bae9
ℹ️ 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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 531f0aa. Configure here.
|
Thanks. I've handled all of it at
Generated by Claude Code |
e7492ae to
e5ef130
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Staff engineer (Cursor agent): Still LGTM at e5ef130 (re-stacked on #4423 as one commit). The direction got better: the demo now uses #4423's shared pieces (the Validated
Not run: the preview deploy and a site build. Bugbot was still running at review time. Optional nit, fine to skip: |
|
Thanks. The Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 2606591 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #4381 +/- ##
=======================================
Coverage 98.37% 98.37%
=======================================
Files 161 161
Lines 3080 3080
Branches 605 605
=======================================
Hits 3030 3030
Misses 10 10
Partials 40 40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
80f639c to
25e4b67
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
25e4b67 to
6471278
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6471278 to
9a3977e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a3977e491
ℹ️ 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".
581d79e to
6a1591f
Compare
The Render Data page's Async Boundaries section opens a post in a small blog app: the post loads quickly and its comments slowly, under one AsyncBoundary, so readers see where its loading fallback goes and can move it in the code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FerSYMdRyzxaKQdv64mGf
PostList.vue awaits in setup, so the router's view needs a Suspense for the list to render when readers copy the Vue code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FerSYMdRyzxaKQdv64mGf
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FerSYMdRyzxaKQdv64mGf
Following #4458, the boundary demo's files that only make the preview work (the React layout's header and both frameworks' post lists) are marked docs-only, so skill references show just the boundary and the components it wraps. Vue's Layout.vue stays: its RouterView Suspense is the route's boundary. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FerSYMdRyzxaKQdv64mGf
It points at the demo's Code toggle, which agents reading skill references and llms.txt don't have (#4408). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018FerSYMdRyzxaKQdv64mGf
6a1591f to
2606591
Compare

Motivation
"Async Boundaries" on the Render Data page says where to put
<AsyncBoundary>(Vue:<Suspense>), but readers cannot see what that placement does to the page.Solution
A live
demoof a small blog plays in that section: it opens a post, and one boundary around the post and its comments shows one fallback, then both at once. Site readers are invited to move the boundary to wrap only<CommentList />, so the post shows first.Assumptions and validation
yarn check:doc-examples reactandvuepass on master after website: Demo playgrounds with the code tucked away, and a design system for demo apps #4423 merged, andyarn build:skillspasses; skill references keep the boundary code and drop the preview-only files. High certainty.🤖 Generated with Claude Code
https://claude.ai/code/session_018FerSYMdRyzxaKQdv64mGf