Repository navigation
test(core): a source check fails on module-scope cross-module identity outside a shared key (AST-110 FR3) - #10807
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
8883915 to
d787148
Compare
|
/approve-spec d787148 |
PR Analysis Report🔎 Preview ·
|
There was a problem hiding this comment.
Requesting changes. The source check is a useful ratchet and its four tests pass, but it promises more than it checks.
- The title, the User impact ("makes every such site visible") and the new FR7 sentence in
docs/specs/AST-110-swizzle-output/spec.md(lines 148-149) say the check fails on any module-local Symbol, WeakMap or registry in an FR3 module, but the FR3 stores the record itself names pass with no baseline entry, namely the layer stack'sentriesarray (packages/core/src/Layer/layerStack.ts:158), the modal outlet stackoutlets(packages/core/src/Layer/modalOutlet.ts:44) and the liveregions(packages/core/src/hooks/useAnnounce.ts:181), because arrays and object literals are never candidates and aletor collection counts only behind aregister…/get…-style exported function that references it directly: widen the check (arrays and object literals, every exported user, private helpers,export {…}lists, annotatedDocumentkeys) and baseline the new hits, or narrow those three claims to what it detects.
Also worth fixing: line 236 of the record still says the other verification rows have no check, though the new FR7 row names this test. Because the record changes either way, the new head needs a fresh /approve-spec.
[Automated review]
|
/approve-spec 8cb1050 |
|
/approve-spec 7d52273 |
|
/approve-spec d721ce9 |
There was a problem hiding this comment.
Requesting changes. The check now catches every store the earlier review named, and its five tests pass, but one store in the announcement channel still slips through.
deferredByScope(packages/core/src/hooks/modalAnnounce.ts:349), a module-localWeakMapin the announcement channel that FR3 names, passes with no baseline entry although the new FR7 sentence (docs/specs/AST-110-swizzle-output/spec.mdlines 148-149), the title and the User impact say the check fails on it, becausescripts/shared-identity.mjsstarts its walk only from exported names and this module handsannounceFromScopetouseAnnounceat load throughinstallModalRoute({…})(lines 474-477): also start from the functions a top-level statement names, and add the one new hit to the F5 group.
Two smaller points: collections still decide "changes" with a text regex (scripts/shared-identity.mjs lines 151-156) while literals use isMutated, and the record's gap line (line 236) could name the unverified rows instead of saying they have no check on main.
[Automated review]
| | FR7 | `scripts/shared-identity.test.mjs` over the source of the packages swizzle copies from, against its baseline | A module-local `Symbol()` brand in an FR3 module; the same brand under `sharedKey`; a module-scope registry an exported register function writes | The module-local brand or registry passes, the `sharedKey` brand fails, or a baseline entry the source no longer has stays listed | | ||
|
|
||
| Known verification gap: only FR3's shared-context checks (`sharedContexts.test.tsx`) exist; the other rows have no check on `main`. | ||
| Known verification gap: only FR3's shared-context checks (`sharedContexts.test.tsx`) and FR7's source check (`scripts/shared-identity.test.mjs`) exist; the other rows have no check on `main`. |
There was a problem hiding this comment.
Non-blocking: "the other rows have no check on main" is temporary status. Name the unverified rows instead, for example: "Known verification gap: FR1, FR2, FR3's store row, FR4, FR5, FR6 and FR7's swizzle-proof row have no check."
|
/approve-spec a13fa57 |
There was a problem hiding this comment.
Approved pending CI: CI has not run this pull request's tests on this head yet, and the deferred test lane runs before merge.
The source check now also follows functions a module hands to another module at load, so deferredByScope in packages/core/src/hooks/modalAnnounce.ts is caught and listed in the baseline, and seeded collections use the same mutation test as literals. The six tests pass, and the baseline matches the source on this head and on main.
Two optional cleanups:
docs/specs/AST-110-swizzle-output/spec.mdline 236 still ends "the other rows have no check onmain"; naming the unverified rows instead keeps temporary status out of the record.- The description still says 253 hits and 121 grouped entries; the baseline now holds 254 entries, 122 of them in fix groups.
[Automated review]
…y outside a shared key scripts/shared-identity.mjs reads the shipped source of the packages swizzle copies from with the TypeScript AST and lists module-local symbols that key an exported type or that an exported function reads or writes, and module-scope registries an exported register, subscribe, read or identity function uses, or that are keyed by a document or window. scripts/shared-identity.test.mjs fails on a hit the baseline does not list and on a listed entry the source no longer has. The baseline holds today's 79 hits: open entries name their fix group, allowed entries their reason. AST-110 FR7 names the check.
a13fa57 to
20e1229
Compare
|
/approve-spec 20e1229 |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #10831.
Stack: #10807 → #10817 → #10822 → #10831
User impact
None at runtime. A swizzled component carries its own copy of every Core module it reaches, so a module that keeps cross-module identity in module scope — a
Symbol()brand, aletcounter, aMap/WeakMapregistry — splits into two instances: the package's components and the copy's stop agreeing. Today that ships unnoticed: a swizzled Table ignored the package'suseTableRowsVirtualization(#10798), and a swizzled LayerProvider showed no toasts (#10800). This check lists such module-scope sites and fails CI on a new one, so a product team's swizzled component keeps working with the package components beside it.Problem and solution fit
spec:AST-110FR3 requires a copy and the package to share coordinating state, brands and contexts.scripts/shared-identity.mjsreads the shipped source of Core, Lab, Charts, RichText and Vega with the TypeScript AST and lists:Symbol(...)orunique symbolthat keys an exported type, or that an exported function reaches;let, or aMap/Set/WeakMap/WeakSetor an object or array literal that changes — that an exported function reaches, directly or through the module's own functions (exports from modifiers,export {…}lists and default exports all count);DocumentorWindow, whether the key is a type argument or a declaration annotation.It skips state created through the shared-realm helpers (
sharedKey,realmValue,createSharedContext), collections with fixed contents that nothing changes, and read-only-typed constants.scripts/shared-identity.test.mjsfails on a hitscripts/shared-identity.baseline.jsondoes not list, and on a listed entry the source no longer has. The baseline holds today's 253 hits: 121openentries each name the fix group that removes them, 73allowedentries carry a reason (memo caches, per-copy observers, idempotent style insertion, development-warning deduplication, synchronous per-call run state, per-copy motion role tables, Markdown's deliberately unshared trust and claim symbols), and 59 areopenundertriage. Each fix pull request deletes its group's entries.Expected behavior and authority
spec:AST-110FR3 (approved,authority: draft): coordinating state lives in one store per document under a registered key, and "a symbol that identifies objects across modules, such as a brand or a context key, is registered under the same kind of key, so a copy and the package agree on it." This change adds one FR7 sentence and its verification row, and updates the record's known verification gap to name the check: "A source check fails when a module that FR3 covers creates a module-local Symbol, WeakMap or registry instead of a registered shared key."Evidence
Symbol()brand on an exported type is flagged; the same brand undersharedKeywith a phantom type member passes; a module-scope registry an exportedregister*writes is flagged; state an export reaches only through a private helper and anexport {…}list is flagged; aWeakMap<Document, …>written as a declaration annotation is flagged; an object literal the module changes is flagged.mainthe check finds exactly the 253 baseline entries; it runs in about 1 s.Scope
Writing rubric results
R1 narration: none; the FR7 sentence states present behavior.
R2 research: none.
R3 evidence: one verification row added.
R4 rejected alternatives: none added.
R5 change references: none.
R6 open questions: none.
Testing
pnpm exec vitest run --project node scripts/shared-identity.test.mjspnpm check:knowledge