Repository navigation
chore(hooks): source comments explain only non-obvious reasons (2 of 2) - #9520
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This comment cleanup is ready to merge: with comments removed, the 22 hook files are unchanged, and pnpm specs:for lists the same records and claims for every file. A few rewritten comments could still be tightened, and the inline notes give a rewrite for each; none of them blocks the merge:
useStreamingText.ts:131-132says natural speed uses--duration-fast-minand only fast speed has the 4 ms floor, but the code uses a tenth of the token and floors both speeds.useBusyAnnouncement.tsanduseRenamedProp.tsdrop theirSYNC: not exported from /packages/core/src/hooks/index.tsline, whichModalAnnounceProvider.tsxandmodalAnnounce.tskeep.useInteractiveRole.ts:89still starts with// 3., but steps 1, 2 and 4 are gone.
[Automated review]
Comment on lines
+131
to
+132
| // Tick timing comes from the motion tokens: natural uses | ||
| // --duration-fast-min, and fast uses half of it, at least 4ms. |
There was a problem hiding this comment.
This misstates the timing. Natural speed uses a tenth of --duration-fast-min (line 144), and the 4 ms floor on line 145 applies to both speeds. Lines 142-143 restate line 144 too. One comment could replace both:
// The tick follows the theme's --duration-fast-min, so the theme's motion
// speed sets the reveal speed.| ); | ||
|
|
||
| // Animation loop | ||
| useEffect(() => { |
There was a problem hiding this comment.
This Effect lost its only note (// Animation loop). The source-comment rule keeps the note that names what an Effect synchronizes:
// Runs the reveal on animation frames. Cleanup cancels the pending frame.
Comment on lines
+304
to
+309
| // A scroll moves only the offset, so it derives the logical edges from | ||
| // box geometry when the event fires. Style and layout are usually still | ||
| // clean then, so no computed style is read and no frame is requested. | ||
| // When the scroll cannot explain a change (overflow on a requested axis | ||
| // appears or goes away, or the offset sign does not match the writing | ||
| // direction), it schedules the full measurement. |
There was a problem hiding this comment.
The handler never reads computed style; clean layout only keeps the geometry read cheap, so the so on line 306 gives the wrong reason. The last sentence also has 31 words. Suggested:
// A scroll moves only the offset, so this handler reads only box geometry.
// Layout is usually clean when the event fires, so the read is cheap. A
// change the scroll cannot explain schedules the full measurement: overflow
// that appears or goes away, or an offset sign the writing direction rules out.
Comment on lines
+11
to
+12
| * @position Core a11y hook for controls that show a decorative busy Spinner. | ||
| * It is a shared detail of the busy controls, so it is not in hooks/index.ts. |
There was a problem hiding this comment.
The SYNC: reminder became @position prose. modalAnnounce.ts and ModalAnnounceProvider.tsx keep the same reminder as a SYNC: line, and the rule keeps SYNC: reminders in the header:
* @position Core a11y hook for controls that show a decorative busy Spinner.
* It is a shared detail of the busy controls.
*
* SYNC: not exported from /packages/core/src/hooks/index.ts.
Comment on lines
+9
to
+11
| * @position Core deprecation utility for the overlap of a prop rename. It is | ||
| * not in hooks/index.ts, because that barrel is public and a new public hook | ||
| * needs admission (AST-002). Import it by path. |
There was a problem hiding this comment.
As in useBusyAnnouncement.ts, the SYNC: reminder moved into @position. Keep the reason here and restore the tag line: * SYNC: not exported from /packages/core/src/hooks/index.ts.
Comment on lines
+15
to
+16
| // first and keeps the old name working for an overlap (AST-017). The new name | ||
| // wins when both are set, and development says so. Production behavior does |
There was a problem hiding this comment.
"The new name wins when both are set, and development says so" repeats the JSDoc at lines 35-36. Consider keeping only the reason ("A released prop is a public promise…") here, or moving the block onto that JSDoc.
|
|
||
| // 1. href → link (unless disabled — a disabled href is skipped here and | ||
| // resolved by the checks below, landing on 'inert' if nothing else applies) | ||
| // A disabled href is skipped, so the checks below decide the role. |
There was a problem hiding this comment.
This restates !isDisabled on line 81. Line 89 below still says // 3. Context-provided role override…, but steps 1, 2 and 4 and the header's list are gone. Consider deleting this comment and dropping the 3..
Comment on lines
+142
to
+145
| * Moves focus with the modality of the last input: after a pointer press a | ||
| * focused item shows no keyboard ring, and after a key press it does. Use it | ||
| * for a list whose items draw the ring and that opens from a pointer, such as | ||
| * a context menu. Unset (the default): a plain `focus()`. |
There was a problem hiding this comment.
The first sentence has 27 words, and the instruction after it has 21. Suggested:
* Moves focus with the modality of the last input. After a pointer press,
* a focused item shows no keyboard ring. After a key press, it shows one.
* Use it for a pointer-opened list whose items draw the ring, such as a
* context menu. Unset (the default): a plain `focus()`.
Contributor
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| charts/Chart | 60.7 kB (0) · 20 B CSS (0) | — |
| charts/ChartLegend | 50.2 kB (0) · 20 B CSS (0) | — |
| charts/ChartTooltip | 50.1 kB (0) · 20 B CSS (0) | — |
134 more components
| Component | Size | LoC |
|---|---|---|
| AlertDialog | 53.5 kB (0) · 12.7 kB CSS (0) | — |
| AppShell | 65.6 kB (0) · 12.1 kB CSS (0) | — |
| Avatar | 32.2 kB (0) · 6.5 kB CSS (0) | — |
| AvatarGroup | 35.0 kB (0) · 6.6 kB CSS (0) | — |
| AvatarGroupOverflow | 36.1 kB (0) · 6.5 kB CSS (0) | — |
| Banner | 44.4 kB (0) · 10.3 kB CSS (0) | — |
| BaseTypeahead | 52.6 kB (0) · 10.2 kB CSS (0) | — |
| BottomSheet | 34.8 kB (0) · 7.5 kB CSS (0) | — |
| BottomSheetSwitcher | 15.0 kB (0) · 3.4 kB CSS (0) | — |
| BreadcrumbItem | 64.7 kB (0) · 11.4 kB CSS (0) | — |
| Button | 39.4 kB (0) · 9.5 kB CSS (0) | — |
| ButtonGroup | 9.0 kB (0) · 3.4 kB CSS (0) | — |
| Calendar | 52.0 kB (0) · 10.3 kB CSS (0) | — |
| Carousel | 47.1 kB (0) · 10.5 kB CSS (0) | — |
| ChatComposer | 58.7 kB (0) · 11.2 kB CSS (0) | — |
| ChatComposerInput | 53.3 kB (0) · 10.4 kB CSS (0) | — |
| ChatComposerTokenElement | 53.3 kB (0) · 10.4 kB CSS (0) | — |
| ChatDictationButton | 43.0 kB (0) · 9.8 kB CSS (0) | — |
| ChatLayout | 45.5 kB (0) · 10.3 kB CSS (0) | — |
| ChatLayoutScrollButton | 42.9 kB (0) · 9.9 kB CSS (0) | — |
| ChatMessageList | 22.7 kB (0) · 7.9 kB CSS (0) | — |
| ChatSendButton | 42.2 kB (0) · 9.5 kB CSS (0) | — |
| ChatToolCalls | 25.8 kB (0) · 9.3 kB CSS (0) | — |
| CheckboxInput | 46.7 kB (0) · 9.7 kB CSS (0) | — |
| CheckboxList | 41.2 kB (0) · 8.6 kB CSS (0) | — |
| CheckboxListItem | 51.4 kB (0) · 11.0 kB CSS (0) | — |
| ClickableCard | 14.5 kB (0) · 7.1 kB CSS (0) | — |
| CodeBlock | 56.9 kB (0) · 11.1 kB CSS (0) | — |
| CommandPalette | 48.0 kB (0) · 12.2 kB CSS (0) | — |
| CommandPaletteInput | 26.6 kB (0) · 8.5 kB CSS (0) | — |
| ComplexSelector | 53.3 kB (0) · 11.1 kB CSS (0) | — |
| ContextMenu | 72.7 kB (0) · 15.3 kB CSS (0) | — |
| ContextMenuItem | 50.2 kB (0) · 9.9 kB CSS (0) | — |
| DateInput | 98.5 kB (0) · 15.3 kB CSS (0) | — |
| DateRangeInput | 67.4 kB (0) · 11.7 kB CSS (0) | — |
| DateTimeInput | 110.8 kB (0) · 15.8 kB CSS (0) | — |
| Dialog | 16.4 kB (0) · 6.6 kB CSS (0) | — |
| DialogHeader | 45.4 kB (0) · 10.2 kB CSS (0) | — |
| Drawer | 34.1 kB (0) · 8.2 kB CSS (0) | — |
| DrawerHeader | 45.5 kB (0) · 11.6 kB CSS (0) | — |
| DropdownMenu | 70.5 kB (0) · 15.4 kB CSS (0) | — |
| DropdownMenuCheckboxItem | 18.2 kB (0) · 5.9 kB CSS (0) | — |
| DropdownMenuItem | 17.4 kB (0) · 5.5 kB CSS (0) | — |
| DropdownMenuRadioItem | 18.2 kB (0) · 5.9 kB CSS (0) | — |
| DropdownMenuSubMenu | 55.7 kB (0) · 9.9 kB CSS (0) | — |
| Field | 39.4 kB (0) · 8.3 kB CSS (0) | — |
| FieldLabel | 33.1 kB (0) · 5.9 kB CSS (0) | — |
| FieldStatus | 9.4 kB (0) · 3.6 kB CSS (0) | — |
| FileInput | 51.6 kB (0) · 10.7 kB CSS (0) | — |
| Heading | 7.9 kB (0) · 6.0 kB CSS (0) | — |
| HoverCard | 18.3 kB (0) · 5.0 kB CSS (0) | — |
| IconButton | 39.5 kB (0) · 9.5 kB CSS (0) | — |
| InputGroup | 40.0 kB (0) · 8.4 kB CSS (0) | — |
| Item | 13.2 kB (0) · 4.9 kB CSS (0) | — |
| LayerProvider | 60.6 kB (0) · 10.5 kB CSS (0) | — |
| Lightbox | 48.1 kB (0) · 10.1 kB CSS (0) | — |
| Link | 40.0 kB (0) · 7.9 kB CSS (0) | — |
| List | 5.4 kB (0) · 435 B CSS (0) | — |
| ListItem | 50.8 kB (0) · 11.0 kB CSS (0) | — |
| Markdown | 143.4 kB (0) · 18.3 kB CSS (0) | — |
| MobileNav | 47.8 kB (0) · 10.4 kB CSS (0) | — |
| MobileNavToggle | 42.8 kB (0) · 9.7 kB CSS (0) | — |
| MoreMenu | 70.8 kB (0) · 15.4 kB CSS (0) | — |
| MultiSelector | 70.7 kB (0) · 15.2 kB CSS (0) | — |
| NavHeadingMenu | 14.3 kB (0) · 3.4 kB CSS (0) | — |
| NumberInput | 52.9 kB (0) · 11.2 kB CSS (0) | — |
| Outline | 24.2 kB (0) · 4.2 kB CSS (0) | — |
| OverflowList | 8.6 kB (0) · 3.6 kB CSS (0) | — |
| Overlay | 19.8 kB (0) · 4.0 kB CSS (0) | — |
| Pagination | 83.9 kB (0) · 15.9 kB CSS (0) | — |
| Popover | 46.3 kB (0) · 12.4 kB CSS (0) | — |
| PowerSearch | 179.7 kB (0) · 20.5 kB CSS (0) | — |
| ProgressBar | 7.9 kB (0) · 6.9 kB CSS (0) | — |
| RadioList | 40.8 kB (0) · 8.3 kB CSS (0) | — |
| RadioListItem | 49.3 kB (0) · 10.1 kB CSS (0) | — |
| ScrollableArea | 10.7 kB (0) · 4.4 kB CSS (0) | — |
| SegmentedControl | 34.0 kB (0) · 6.5 kB CSS (0) | — |
| SelectableCard | 14.3 kB (0) · 7.3 kB CSS (0) | — |
| Selector | 74.8 kB (0) · 15.6 kB CSS (0) | — |
| SelectorOption | 16.5 kB (0) · 5.4 kB CSS (0) | — |
| SideNav | 63.2 kB (0) · 10.9 kB CSS (0) | — |
| SideNavCollapseButton | 43.2 kB (0) · 9.8 kB CSS (0) | — |
| SideNavHeading | 55.7 kB (0) · 10.5 kB CSS (0) | — |
| SideNavItem | 51.7 kB (0) · 10.4 kB CSS (0) | — |
| Slider | 44.2 kB (0) · 8.9 kB CSS (0) | — |
| Spinner | 18.5 kB (0) · 6.7 kB CSS (0) | — |
| StatusDot | 20.2 kB (0) · 6.3 kB CSS (0) | — |
| Stepper | 44.0 kB (0) · 9.9 kB CSS (0) | — |
| Switch | 45.1 kB (0) · 9.7 kB CSS (0) | — |
| Tab | 11.2 kB (0) · 4.2 kB CSS (0) | — |
| Table | 82.7 kB (0) · 16.0 kB CSS (0) | — |
| TableCell | 73.9 kB (0) · 15.8 kB CSS (0) | — |
| TableHeaderCell | 73.6 kB (0) · 15.5 kB CSS (0) | — |
| TableSelectionToolbar | 49.2 kB (0) · 11.5 kB CSS (0) | — |
| TabList | 35.2 kB (0) · 7.5 kB CSS (0) | — |
| TabMenu | 52.2 kB (0) · 10.4 kB CSS (0) | — |
| Text | 7.4 kB (0) · 6.0 kB CSS (0) | — |
| TextArea | 45.1 kB (0) · 9.8 kB CSS (0) | — |
| TextInput | 50.6 kB (0) · 10.9 kB CSS (0) | — |
| Thumbnail | 47.3 kB (0) · 11.3 kB CSS (0) | — |
| TimeInput | 86.3 kB (0) · 14.3 kB CSS (0) | — |
| Timer | 9.5 kB (0) · 7.0 kB CSS (0) | — |
| Timestamp | 21.3 kB (0) · 9.9 kB CSS (0) | — |
| Toast | 56.5 kB (0) · 10.1 kB CSS (0) | — |
| ToggleButton | 40.3 kB (0) · 9.6 kB CSS (0) | — |
| Token | 25.1 kB (0) · 4.8 kB CSS (0) | — |
| Tokenizer | 67.5 kB (0) · 17.1 kB CSS (0) | — |
| Toolbar | 34.5 kB (0) · 7.9 kB CSS (0) | — |
| Tooltip | 18.0 kB (0) · 5.0 kB CSS (0) | — |
| TopNav | 49.3 kB (0) · 10.7 kB CSS (0) | — |
| TopNavHeading | 55.1 kB (0) · 10.4 kB CSS (0) | — |
| TopNavMegaMenu | 51.2 kB (0) · 11.2 kB CSS (0) | — |
| TopNavMenu | 55.0 kB (0) · 10.5 kB CSS (0) | — |
| TreeList | 37.8 kB (0) · 5.0 kB CSS (0) | — |
| Typeahead | 60.4 kB (0) · 11.8 kB CSS (0) | — |
| lab/ChatEmojiPicker | 62.5 kB (0) · 773 B CSS (0) | — |
| lab/ChatReactionBar | 63.1 kB (0) · 1.1 kB CSS (0) | — |
| lab/CommentComposer | 99.8 kB (0) · 1.9 kB CSS (0) | — |
| lab/CommentPin | 44.3 kB (0) · 1.2 kB CSS (0) | — |
| lab/Comments | 229.5 kB (0) · 2.5 kB CSS (0) | — |
| lab/CommentThread | 224.2 kB (0) · 2.4 kB CSS (0) | — |
| lab/DiffViewer | 48.0 kB (0) · 2.1 kB CSS (0) | — |
| lab/EmojiPicker | 67.9 kB (0) · 722 B CSS (0) | — |
| lab/InfoTip | 23.1 kB (0) · 458 B CSS (0) | — |
| lab/ListInput | 79.8 kB (0) · 1.6 kB CSS (0) | — |
| lab/MobilePowerSearch | 164.2 kB (0) · 1.2 kB CSS (0) | — |
| lab/MobileTokenizer | 98.8 kB (0) · 880 B CSS (0) | — |
| lab/Schedule | 57.6 kB (0) · 3.9 kB CSS (0) | — |
| lab/TourStep | 51.6 kB (0) · 590 B CSS (0) | — |
| lab/TransferList | 72.1 kB (0) · 1.7 kB CSS (0) | — |
| lab/TransferListSelector | 78.3 kB (0) · 1.9 kB CSS (0) | — |
| richtext/RichTextEditor | 274.1 kB (0) · 2.6 kB CSS (0) | — |
| richtext/RichTextEditorToolbar | 219.3 kB (0) · 454 B CSS (0) | — |
| richtext/RichTextView | 254.7 kB (0) · 2.3 kB CSS (0) | — |
Package size: @astryxdesign/core 5.83 MB packed (−8.1 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 5 rows; run pnpm api:report.
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
This branch was successfully deployed
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.
Maintainer impact
Long, narrating comments in
packages/core/src/hooksmake the code slower to read and review. Many restate the code, cite records, or run past the file-header limit. The repository comment rule asks that a comment explain only a non-obvious reason, constraint, invariant, or quirk.Intended invariant
The comment rule in
AGENTS.md(Source comments) and the knowledge-contracts source-comment invariant. Runtime behavior, types, and the public API stay the same.Change
Second of two hooks pull requests (the first is #9514). It changes comments only, in 22 files:
@input,@output,@position, andSYNC:(about 12 lines).pnpm specs:forstill lists the same records and claims without it. Seven citations stay, because each is a file's only link to a record or claim.Comment lines in the touched files: 1,897 → 1,665. Across
hooks/: 4,381 → 4,149 from this pull request alone, and 4,381 → 3,928 with #9514.Evidence
@astryxdesign/corebuild output is byte-identical with comments removed (see Testing).Scope
A
[docs]Changeset is included because changeset coverage requires one for anypackages/core/srcchange. A few public doc comments are reworded.Testing
tsc -p tsconfig.build.json --removeComments(JS and.d.ts, 1,936 files) and the package Babel build with--no-comments(969 files), at the merge base and at this head.diff -rfinds no difference.pnpm specs:foron every touched file: the same records and claims before and after.tsc --noEmitfor core: pass.eslint src/hooks: pass.vitest run packages/core/src/hooks: 43 files, 640 tests pass.