fix brief page not found while accessing plug page#16452
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe PR updates the plugin loading context to carry both apps and a combined loading state. PluginEngine computes the loading state from enabled-plugins and plugin manifest queries, stores it in the updated context shape, and AppRouter consumes this state to show a Loading screen during plugin initialization instead of an ErrorPage. ChangesPlugin Loading State and UI
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Greptile SummaryThis PR fixes a brief "page not found" flash that appeared during hard-refreshes on plugin pages by exposing a plugin-loading state through context and using it in the router to show a loading spinner instead of the error page while plugins are still registering their routes.
Confidence Score: 5/5Safe to merge — the change is a targeted, low-risk fix that adds a loading guard with no impact on existing routes or authenticated flows. The context shape change is backward-compatible (all consumers of No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "track plugs loading state in the same co..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR addresses an incorrect “page not found” experience when directly accessing plugin-provided pages before plugin routes have finished loading. It introduces a shared “plugins loading” state from the plugin engine and uses it in the main router to show a loader instead of a 404 during initial plugin resolution.
Changes:
- Expose plugin loading state via a new
CareAppsLoadingContextinuseCareApps. - Provide that loading state from
PluginEnginebased on enabled-plugin and manifest query load status. - Update
AppRouterfallback behavior to render<Loading />(instead of<ErrorPage />) while plugins are still loading.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Routers/AppRouter.tsx |
Uses the new loading signal to avoid premature 404s while plugin routes are still being registered. |
src/PluginEngine.tsx |
Computes and provides a consolidated “plugins loading” boolean via context. |
src/hooks/useCareApps.tsx |
Adds the loading context + hook to allow consumers (like the router) to read plugin load state. |
| export const CareAppsLoadingContext = createContext<boolean>(false); | ||
|
|
||
| export const useCareApps = () => { |
Deploying care-preview with
|
| Latest commit: |
c5802a1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://40fb6d29.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-555-handle-brief-page-no.care-preview-a7w.pages.dev |
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9533 |
rithviknishad
left a comment
rithviknishad
left a comment
There was a problem hiding this comment.
just curious, why have a new context when we can use the same context?
|
also: link jira ticket |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useCareApps.tsx`:
- Around line 16-19: The CareAppsContextValue type definition currently uses the
type keyword for defining an object shape, but the coding guidelines prefer
using interface for object type definitions. Convert the type
CareAppsContextValue declaration from a type alias to an interface, changing
from type keyword to interface keyword and adjusting the syntax accordingly
(replacing the equals sign with the interface body syntax).
In `@src/PluginEngine.tsx`:
- Around line 169-171: The CareAppsContext.Provider value prop contains an
inline object that creates a new reference on every render, causing unnecessary
re-renders in context consumers. Wrap the context value object with the useMemo
hook (already imported on line 13) and specify pluginsQuery and
arePluginsLoading as dependencies, so the object reference only changes when
these dependencies actually change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e473de16-d38f-4e26-940d-72bf45e03a38
📒 Files selected for processing (2)
src/PluginEngine.tsxsrc/hooks/useCareApps.tsx
| export type CareAppsContextValue = { | ||
| apps: CareAppsContextType; | ||
| isLoading: boolean; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use interface instead of type for object type definitions.
Per coding guidelines, prefer interface for defining object shapes in TypeScript.
Suggested fix
-export type CareAppsContextValue = {
- apps: CareAppsContextType;
- isLoading: boolean;
-};
+export interface CareAppsContextValue {
+ apps: CareAppsContextType;
+ isLoading: boolean;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type CareAppsContextValue = { | |
| apps: CareAppsContextType; | |
| isLoading: boolean; | |
| }; | |
| export interface CareAppsContextValue { | |
| apps: CareAppsContextType; | |
| isLoading: boolean; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useCareApps.tsx` around lines 16 - 19, The CareAppsContextValue
type definition currently uses the type keyword for defining an object shape,
but the coding guidelines prefer using interface for object type definitions.
Convert the type CareAppsContextValue declaration from a type alias to an
interface, changing from type keyword to interface keyword and adjusting the
syntax accordingly (replacing the equals sign with the interface body syntax).
Source: Coding guidelines
| <CareAppsContext.Provider | ||
| value={{ apps: pluginsQuery, isLoading: arePluginsLoading }} | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Memoize the context value to prevent unnecessary re-renders.
The inline object { apps: pluginsQuery, isLoading: arePluginsLoading } creates a new reference on every render, which can trigger re-renders in all context consumers even when the actual data hasn't changed.
Suggested fix
+ const contextValue = useMemo(
+ () => ({ apps: pluginsQuery, isLoading: arePluginsLoading }),
+ [pluginsQuery, arePluginsLoading],
+ );
+
return (
<Suspense fallback={<Loading />}>
<ErrorBoundary
fallback={
<div className="flex h-screen w-screen items-center justify-center">
Care has encountered an unexpected error.
</div>
}
>
- <CareAppsContext.Provider
- value={{ apps: pluginsQuery, isLoading: arePluginsLoading }}
- >
+ <CareAppsContext.Provider value={contextValue}>
<Suspense fallback={<Loading />}>{children}</Suspense>
</CareAppsContext.Provider>
</ErrorBoundary>
</Suspense>
);Note: useMemo is already imported on line 13.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/PluginEngine.tsx` around lines 169 - 171, The CareAppsContext.Provider
value prop contains an inline object that creates a new reference on every
render, causing unnecessary re-renders in context consumers. Wrap the context
value object with the useMemo hook (already imported on line 13) and specify
pluginsQuery and arePluginsLoading as dependencies, so the object reference only
changes when these dependencies actually change.
|
@khavinshankar Do add the relevant jira ticket (also for future: add as prefix for branch name). |
nihal467
left a comment
nihal467
left a comment
There was a problem hiding this comment.
ADD THE JIRA TICKET TO THE PR
Proposed Changes
Currently when accessing plugin page, there is a brief page not found before the plugin page loads completely. Go to a plugin tab in encounter, do a hard refresh, the error can be noticed.
Fixes #issue_number
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes