SITES-41734: fix loading flag staying false while host is loading#123
Open
SITES-41734: fix loading flag staying false while host is loading#123
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes useExtensions().loading so it accurately reflects Host loading state during load cycles (including when filters return zero matching extensions), and updates Jest config to ensure React/TSX hook tests are discovered and mapped correctly.
Changes:
- Refactors
useExtensionsto use a dedicatedisLoadingstate driven by Host events (guestload,loadallguests) and fixes the “empty filtered list forces loading=false” regression. - Adds/updates hook tests to cover loading lifecycle behavior and documents the difference between
useExtensions().loadinganduseExtensionListFetched(). - Updates root Jest configuration to include
*.test.tsxand adjusts per-project mapping/ignore patterns.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/uix-host-react/src/hooks/useExtensions.ts | Fixes loading computation and event subscriptions; adds isLoading state and hook-order compliant early return. |
| packages/uix-host-react/src/hooks/useExtensions.test.tsx | Expands coverage for loading transitions, reload cycles, update modes, unload behavior, and provide() behavior. |
| packages/uix-host-react/src/hooks/useLoadingFlags.comparison.test.tsx | New comparison/regression tests documenting loading vs extensionListFetched semantics. |
| packages/uix-host-react/src/hooks/useExtensionListFetched.test.ts | New unit test for useExtensionListFetched. |
| jest.config.ts | Enables TSX test discovery and adds per-project .js-extension stripping mapper + ignores. |
You can also share your feedback on Copilot code review. Take the survey.
fe-lix-
changed the title
fe-lix-
changed the title
…ing (SITES-41734) Two bugs caused loading to always return false: 1. Wrong ternary forced loading = false when no extensions matched the filter, even when host.loading was true. 2. In updateOn: each mode, no re-render fired after the last guest loaded because the hook only subscribed to guestload, not loadallguests. Fix introduces a dedicated isLoading state initialised from host.loading and a new useEffect that subscribes to loadallguests (regardless of updateOn) to set isLoading = false when all guests have finished loading. Also fixes jest.config.ts to pick up .test.tsx files and moves moduleNameMapper into each project config so .js extension stripping works correctly for TSX files. Co-authored-by: Claude <noreply@anthropic.com>
- Add afterEach cleanup for mockHost.loading and mockListeners to prevent test pollution when assertions fail - Add test documenting that isLoading does not reset to true on a subsequent load cycle (potential bug for future fix) - Add test verifying guest.provide() is called for each loaded extension - Add test documenting that guest.provide() is re-called on every load event even when the provided API has not changed Co-authored-by: Claude <noreply@anthropic.com>
- Rules of Hooks: move early error return to after all hook calls so hook count is stable across renders regardless of error state - isLoading on re-load: subscribe to guestload and check host.loading inside the handler to detect when a subsequent load cycle begins; loadallguests still sets isLoading=false at the end of each cycle - Stale closure: inline the guestunload callback directly into its useEffect instead of capturing a function defined outside the effect - baseDeps spread: replace spread deps with explicit [host, requires] for getExtensions and [host] for the unload/loading/error effects; eslint-disable is now only needed in the two places where user-provided deps must be spread (useMemo config and getExtensions deps that flow through requires) - provides asymmetry: add comment documenting that Port has no unprovide() API so the provides effect cannot clean up; this is a Host API limitation - Update test to verify loading correctly resets to true on a second load cycle via the guestload-based detection Co-authored-by: Claude <noreply@anthropic.com>
- loading is false on initial mount when host is not loading - error path: returns loading:false, empty extensions, and the error when useHost returns an error (validates Rules of Hooks fix) - updateOn:each (default): extensions refresh on each guestload - updateOn:all: guestload does not trigger refresh; loadallguests does - guestunload: removes the correct guest from the extensions list Co-authored-by: Claude <noreply@anthropic.com>
…Copilot review - Memoize boundryExtensionPointsAsString with useMemo so getExtensions useCallback can safely depend on it without causing stale closures when the ExtensibleComponentBoundary extensionPoints prop changes - Add .tsx to extensionsToTreatAsEsm in jest.config.ts so ts-jest treats TSX test files as ESM, preventing import/ESM parsing errors Co-Authored-By: Claude <noreply@anthropic.com>
fe-lix-
force-pushed
the
fix/sites-41734-loading-flag
branch
from
0cca5f6 to
9f2e2a3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes useExtensions().loading in @adobe/uix-host-react so it correctly reflects Host loading state (including when the filtered extension list is empty) and reliably flips back to false when all guests finish loading. Also updates the root Jest config so .test.tsx files are included and per-project ESM .js-extension mapping works.
Changes:
- Refactors
useExtensionsto track loading via a dedicatedisLoadingstate and Host event subscriptions. - Adds/expands hook tests (including comparison tests documenting
loadingvsextensionListFetchedbehavior). - Updates root
jest.config.tsto match.test.tsxfiles and apply.jsextension stripping per project.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/uix-host-react/src/hooks/useExtensions.ts | Reworks loading state handling + event subscriptions; fixes the “empty filtered list forces loading=false” regression. |
| packages/uix-host-react/src/hooks/useExtensions.test.tsx | Adds coverage for loading transitions and event-driven updates. |
| packages/uix-host-react/src/hooks/useLoadingFlags.comparison.test.tsx | Adds behavioral documentation tests comparing loading vs extensionListFetched, including the regression case. |
| packages/uix-host-react/src/hooks/useExtensionListFetched.test.ts | Adds basic unit tests for useExtensionListFetched. |
| jest.config.ts | Ensures .test.tsx is picked up and moduleNameMapper is applied per Jest project. |
You can also share your feedback on Copilot code review. Take the survey.
Runs all three checks on every pull_request to main (and push to main): - Lint: prettier + fixpack - Unit tests: Jest (all packages) - E2E local dist: builds SDK, copies dist into e2e apps, starts host/guest apps, and runs TestCafe tests; uploads screenshots on failure Co-Authored-By: Claude <noreply@anthropic.com>
- Rename boundryExtensionPointsAsString → boundaryExtensionPointsAsString (typo fix: "boundry" → "boundary") - Subscribe to guestbeforeload instead of guestload to detect the start of a new load cycle; guestbeforeload fires even when all guests fail to load (no guestload events), so isLoading now correctly flips to true in all reload scenarios - Update tests to fire guestbeforeload instead of guestload when simulating a new load cycle Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes useExtensions().loading so it accurately reflects host loading cycles (including cases where no extensions match filters and when updateOn: "each" would otherwise miss the final “all done” event). Also updates Jest config to discover TSX tests and adds CI coverage.
Changes:
- Reworks
useExtensionsto track loading via dedicated state +guestbeforeload/loadallguestsevents and to comply with Rules of Hooks. - Adds/expands hook tests (including comparison tests documenting
loadingvsextensionListFetchedsemantics). - Updates Jest config to include
*.test.tsxand adds a new GitHub Actions CI workflow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/uix-host-react/src/hooks/useExtensions.ts | Fixes loading behavior by tracking load cycles via host events and avoids early-return-before-hooks. |
| packages/uix-host-react/src/hooks/useExtensions.test.tsx | Adds tests for loading lifecycle, update modes, unload handling, and provides behavior. |
| packages/uix-host-react/src/hooks/useLoadingFlags.comparison.test.tsx | Documents expected differences between loading and extensionListFetched, including the regression case. |
| packages/uix-host-react/src/hooks/useExtensionListFetched.test.ts | Adds unit tests for useExtensionListFetched. |
| jest.config.ts | Enables discovery/execution of TSX tests and updates ESM handling/mapping. |
| .github/workflows/ci.yml | Introduces CI jobs for lint/unit tests and local-dist E2E. |
You can also share your feedback on Copilot code review. Take the survey.
- Remove lint step (will be introduced in a separate PR) - Pin Node.js to 20.19.5 to match existing e2e workflow and avoid floating minor/patch upgrades causing CI-only breakages - Add paths filters (packages/**, e2e/local-dist/**) so the workflow only triggers on relevant changes, avoiding duplicate runs with the existing e2e-local-dist.yml and skipping docs-only PRs Co-Authored-By: Claude <noreply@anthropic.com>
Inter-package TypeScript references (@adobe/uix-core, @adobe/uix-host) resolve through dist/ type declarations. Without a prior build step CI gets TS2307 "cannot find module" errors on every uix-host-react test suite. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes useExtensions().loading in @adobe/uix-host-react so it correctly reflects host load cycles (including cases where filters match zero extensions), and ensures React hooks re-render when all guests finish loading. Also updates Jest configuration to properly discover .test.tsx files and handle ESM-style .js import specifiers in TS/TSX tests.
Changes:
- Refactors
useExtensionsto track loading via dedicatedisLoadingstate driven byguestbeforeload/loadallguestsevents. - Adds/expands tests around loading behavior, unload behavior, and the
extensionListFetchedflag semantics. - Updates root Jest config for TSX test discovery and adds a new CI workflow that runs unit + local-dist E2E tests on PRs/pushes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/uix-host-react/src/hooks/useExtensions.ts | Fixes loading calculation and subscriptions; adds isLoading lifecycle tracking. |
| packages/uix-host-react/src/hooks/useExtensions.test.tsx | Adds coverage for loading transitions, update modes, unload behavior, and provide() behavior. |
| packages/uix-host-react/src/hooks/useLoadingFlags.comparison.test.tsx | Adds comparison/regression tests documenting loading vs extensionListFetched behavior. |
| packages/uix-host-react/src/hooks/useExtensionListFetched.test.ts | Adds basic tests for useExtensionListFetched. |
| jest.config.ts | Updates testMatch to include TSX; adds per-project moduleNameMapper; treats .tsx as ESM. |
| .github/workflows/ci.yml | Adds a CI workflow running build + unit tests and local-dist E2E tests. |
You can also share your feedback on Copilot code review. Take the survey.
e2e-local-dist.yml already runs the E2E local-dist suite on PRs/pushes affecting packages/** or e2e/local-dist/**. Having a second job with the same triggers caused E2E to run twice on every relevant PR. ci.yml now owns only unit tests (scoped to packages/**) and e2e-local-dist.yml remains the single owner of E2E runs. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Two bugs caused loading to always return false:
Fix introduces a dedicated isLoading state initialised from host.loading and a new useEffect that subscribes to loadallguests (regardless of updateOn) to set isLoading = false when all guests have finished loading.
Also fixes jest.config.ts to pick up .test.tsx files and moves moduleNameMapper into each project config so .js extension stripping works correctly for TSX files.
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: