Open
Conversation
Contributor
Reviewer's GuideImplements route-level lazy loading for app and signup layouts, optimizes bundle splitting via Vite/Rollup config, converts signup pages to default exports to support dynamic imports, tightens lodash usage in Apollo cache configuration, and updates Storybook stories and architecture tests to align with new routing, file naming, and mocking patterns. Sequence diagram for lazy loaded signup routes with React SuspensesequenceDiagram
actor User
participant BrowserRouter
participant SignupRoutes
participant ReactSuspense
participant LazySelectAccountTypePage
User->>BrowserRouter: Navigate to /signup/select-account-type
BrowserRouter->>SignupRoutes: Render SignupRoutes
SignupRoutes->>ReactSuspense: Render Suspense fallback
ReactSuspense->>LazySelectAccountTypePage: Trigger dynamic import
LazySelectAccountTypePage-->>ReactSuspense: Resolve module (default export)
ReactSuspense->>BrowserRouter: Render SelectAccountTypePage inside SectionLayout
BrowserRouter-->>User: Display select account type UI
User->>BrowserRouter: Navigate to /signup/profile-setup
BrowserRouter->>SignupRoutes: Match profile-setup route
SignupRoutes->>ReactSuspense: Render Suspense fallback
ReactSuspense->>LazySelectAccountTypePage: Keep previous lazy modules cached
ReactSuspense-->>BrowserRouter: Render ProfileSetupPage after import
BrowserRouter-->>User: Display profile setup UI
Updated class diagram for signup routes and Apollo cache mergingclassDiagram
class SignupRoutes {
+React_FC
+render(): JSX_Element
}
class SectionLayout {
+React_FC
+render(): JSX_Element
}
class SelectAccountTypePage {
+default_export: React_FC
+render(): JSX_Element
}
class AccountSetupPage {
+default_export: React_FC
+render(): JSX_Element
}
class ProfileSetupPage {
+default_export: React_FC
+render(): JSX_Element
}
class PaymentPage {
+default_export: React_FC
+render(): JSX_Element
}
class TermsPage {
+default_export: React_FC
+render(): JSX_Element
}
class ReactLazy {
+lazy(loader): React_ComponentType
}
class ReactSuspense {
+fallback: JSX_Element
}
SignupRoutes --> SectionLayout : uses
SignupRoutes --> SelectAccountTypePage : lazy_imports
SignupRoutes --> AccountSetupPage : lazy_imports
SignupRoutes --> ProfileSetupPage : lazy_imports
SignupRoutes --> PaymentPage : lazy_imports
SignupRoutes --> TermsPage : lazy_imports
SignupRoutes --> ReactLazy : calls
SignupRoutes --> ReactSuspense : wraps_with
class ApolloManualMergeCacheFix {
+config: InMemoryCacheConfig
}
class InMemoryCacheConfig {
+typePolicies: TypePolicies
}
class TypePolicies {
+PersonalUser: PersonalUserPolicy
}
class PersonalUserPolicy {
+fields: PersonalUserFields
}
class PersonalUserFields {
+account: AccountFieldPolicy
}
class AccountFieldPolicy {
+merge(existing, incoming): any
}
class LodashMerge {
+merge(target, source1, source2): any
}
ApolloManualMergeCacheFix --> InMemoryCacheConfig : holds
InMemoryCacheConfig --> TypePolicies : defines
TypePolicies --> PersonalUserPolicy : contains
PersonalUserPolicy --> PersonalUserFields : contains
PersonalUserFields --> AccountFieldPolicy : contains
AccountFieldPolicy --> LodashMerge : uses_merge_for_deep_merge
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…ility Git was tracking App.tsx (uppercase) while the filesystem had app.tsx (lowercase). This works on macOS but fails on Linux CI runners. Proper git mv ensures both Git index and filesystem use lowercase naming consistently.
The import statement was missing the file extension, causing Vitest browser tests to fail with "Failed to fetch dynamically imported module" error in CI. Adding the extension fixes module resolution in browser context.
Add configuration to handle flaky browser test failures: - Increase Playwright action timeout from 30s to 60s in CI - Add maxConcurrency limit of 5 tests in CI to prevent overwhelming dev server - These settings help prevent "Failed to fetch dynamically imported module" errors The existing retry (3x) and fileParallelism (disabled) settings remain in place to handle race conditions in Storybook + Vitest browser mode.
This reverts commit 361b9a9.
- Remove unused 'within' imports from story files
- Fix settings-page.stories.tsx import path
- Update play functions to check canvasElement.toBeTruthy()
- Follows removal of vi.mock('react') from vitest.setup.ts
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- For the new Suspense wrappers in
AppRoutesandSignupRoutes, consider using a shared loading component (and/or placing the Suspense boundary around the child routes rather than the entire layout) so that the main layout/Chrome remains visible while only the page content is lazily loaded. - In the Vite
manualChunksconfig you grouputils: ['lodash', 'dayjs', 'crypto-hash'], but the PR switches at least one usage tolodash/merge; double‑check that your chunking strategy still captures these subpath imports as intended or adjust the utilities chunk to reflect how lodash is actually being imported. - Several new Storybook stories for lazy routes and Suspense (
AppRoutes,SignupRoutes, etc.) only assert thatcanvasElementis truthy; you could make these more robust by asserting on the actual fallback/content (e.g. the 'Loading...' text or a known element from the loaded page) to verify that lazy loading and Suspense behavior work as expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the new Suspense wrappers in `AppRoutes` and `SignupRoutes`, consider using a shared loading component (and/or placing the Suspense boundary around the child routes rather than the entire layout) so that the main layout/Chrome remains visible while only the page content is lazily loaded.
- In the Vite `manualChunks` config you group `utils: ['lodash', 'dayjs', 'crypto-hash']`, but the PR switches at least one usage to `lodash/merge`; double‑check that your chunking strategy still captures these subpath imports as intended or adjust the utilities chunk to reflect how lodash is actually being imported.
- Several new Storybook stories for lazy routes and Suspense (`AppRoutes`, `SignupRoutes`, etc.) only assert that `canvasElement` is truthy; you could make these more robust by asserting on the actual fallback/content (e.g. the 'Loading...' text or a known element from the loaded page) to verify that lazy loading and Suspense behavior work as expected.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/components/layouts/signup/signup-routes.stories.tsx:19` </location>
<code_context>
+
+const Template: StoryFn<typeof SignupRoutes> = () => <SignupRoutes />;
+
+export const DefaultView: StoryFn<typeof SignupRoutes> = Template.bind({});
+
+DefaultView.play = async ({ canvasElement }) => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing these three nearly identical stories into a single (or at most two) stories that assert distinct observable behavior of the lazy routes and Suspense fallback.
You can reduce complexity and improve signal by collapsing these three near‑identical stories into one (or two) stories that assert actual behavior.
Right now all three `play` functions only check `expect(canvasElement).toBeTruthy()`, so they don’t differentiate:
```ts
export const DefaultView = Template.bind({});
DefaultView.play = async ({ canvasElement }) => {
expect(canvasElement).toBeTruthy();
};
export const SuspenseFallback = Template.bind({});
SuspenseFallback.play = async ({ canvasElement }) => {
expect(canvasElement).toBeTruthy();
};
export const LazyLoadedSignupRoutes = Template.bind({});
LazyLoadedSignupRoutes.play = async ({ canvasElement }) => {
expect(canvasElement).toBeTruthy();
};
```
Instead, you can:
1. Keep a single “happy path” story for the lazy‑loaded routes and assert on specific UI that proves the routes and `Suspense` are wired correctly.
2. Optionally keep a dedicated fallback story **only if** you assert on the fallback content itself.
Example refactor (pseudo‑selectors — adjust to your actual UI):
```ts
import { within, waitFor } from '@storybook/test';
const Template: StoryFn<typeof SignupRoutes> = () => <SignupRoutes />;
export const SignupRoutesLazyLoaded = Template.bind({});
SignupRoutesLazyLoaded.play = async ({ canvasElement }) => {
const canvas = within(canvasElement);
// Assert that the Suspense fallback shows first
await waitFor(() => {
expect(canvas.getByText(/loading/i)).toBeInTheDocument();
});
// Then assert that a lazy‑loaded signup route renders
await waitFor(() => {
expect(canvas.getByText(/select account type/i)).toBeInTheDocument();
});
};
```
If you truly need a separate fallback story (e.g., to document the visual state), make its assertion distinct:
```ts
export const SignupRoutesFallbackOnly = Template.bind({});
SignupRoutesFallbackOnly.play = async ({ canvasElement }) => {
const canvas = within(canvasElement);
expect(canvas.getByText(/loading/i)).toBeInTheDocument();
};
```
This keeps full functionality (still validating lazy routes and `Suspense`) while:
- Removing duplicate story variants that don’t add coverage.
- Making the story names and comments match observable, asserted behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
apps/ui-sharethrift/src/components/layouts/signup/signup-routes.stories.tsx
Outdated
Show resolved
Hide resolved
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
apps/ui-sharethrift/src/components/layouts/signup/signup-routes.stories.tsx
Fixed
Show fixed
Hide fixed
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.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
1 participant
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.
Summary by Sourcery
Introduce route-level code splitting and bundle optimisation while aligning Storybook stories, tests, and architecture rules with the updated frontend structure.
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: