Clean Refactor Scribe as MFE | Support for Questionnaires #9636
Conversation
…into form-field-scribe
WalkthroughThis pull request encompasses several modifications across different files in a React application. The changes primarily involve removing the Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying care-fe with
|
| Latest commit: |
38b5a2a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fa06db9a.care-fe.pages.dev |
| Branch Preview URL: | https://scribe-exit.care-fe.pages.dev |
|
👋 Hi, @shivankacker, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
Deploy-Failed
|
👋 Hi, @shivankacker, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
CARE
|
||||||||||||||||||||||||||||
| Project |
CARE
|
| Branch Review |
scribe-exit
|
| Run status |
|
| Run duration | 01m 15s |
| Commit |
|
| Committer | Shivank Kacker |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
| View all changes introduced in this branch ↗︎ | |
gigincg
changed the title
There was a problem hiding this comment.
Requesting minor clarifications, Looks good for the most part.
@shivankacker Please get the Deployment fixed so that @nihal467 can test it out. @tellmeY18 Can you set up Github Pages for the Scribe MFE
src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx
Outdated
Show resolved
Hide resolved
src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx
Outdated
Show resolved
Hide resolved
|
Reminder: To add the "needs testing" label, comment "ready for testing" on this PR. |
![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
🔭 Outside diff range comments (1)
vite.config.mts (1)
Line range hint
137-140: Enforce HTTPS in productionThe remote URL is always constructed with HTTP protocol, which is insecure for production environments.
Add environment-based protocol selection:
- const remoteUrl = `"http://${host}/assets/remoteEntry.js"`; + const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http'; + const remoteUrl = `"${protocol}://${host}/assets/remoteEntry.js"`;
🧹 Nitpick comments (6)
src/components/Form/SelectMenuV2.tsx (1)
93-96: LGTM! Consider improving type safety in the onChange handler.The component correctly handles value updates, but the type assertion in
onChangecould be more specific.- onChange={(selection: any) => props.onChange(selection.value)} + onChange={(selection: typeof value) => props.onChange(selection.value)}src/components/Form/FormFields/Autocomplete.tsx (2)
8-8: Remove unused import.The
useEffectimport is already included in the destructured import from 'react'.-import { ReactNode, useEffect, useState } from "react"; +import { ReactNode, useState } from "react";
Line range hint
171-173: Improve type safety in the Combobox value prop.The type assertion to
Tcould lead to runtime errors. Consider using a more specific type that matches the option structure.- value={(value ?? props.placeholder ?? "Select") as T} + value={value ?? { label: props.placeholder ?? "Select", value: undefined }}src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx (2)
92-99: Improve type safety in the questionnaire response handling.The type assertion to
anycould be replaced with a more specific type.- const formStateValue = (questionnaireResponse.values[0]?.value as any)?.[0]; + const formStateValue = (questionnaireResponse.values[0]?.value as EncounterEditRequest[])?.[0];
88-88: Improve type safety in the encounter data handling.The type assertion to
unknownfollowed byEncounterEditRequestcould be simplified.- handleUpdateEncounter(encounterData as unknown as EncounterEditRequest); + handleUpdateEncounter(encounterData as EncounterEditRequest);src/pluginTypes.ts (1)
11-11: LGTM! Well-typed plugin interface.The
ScribeComponentTypeis properly typed with the necessary props for form state management. This ensures type safety when implementing the Scribe plugin.However, consider adding JSDoc comments to document the purpose and usage of these props:
export type ScribeComponentType = React.FC<{ + /** Array of questionnaire form states to be managed by the plugin */ formState: QuestionnaireFormState[]; + /** Callback to update the questionnaire form states */ setFormState: React.Dispatch<React.SetStateAction<QuestionnaireFormState[]>>; }>;Also applies to: 22-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.env(1 hunks)package.json(1 hunks)public/locale/en.json(0 hunks)src/Utils/useValueInjectionObserver.tsx(0 hunks)src/components/Common/DateInputV2.tsx(0 hunks)src/components/Form/FormFields/Autocomplete.tsx(1 hunks)src/components/Form/SelectMenuV2.tsx(2 hunks)src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx(1 hunks)src/components/Questionnaire/QuestionTypes/QuestionInput.tsx(1 hunks)src/components/Questionnaire/QuestionnaireForm.tsx(3 hunks)src/pluginTypes.ts(2 hunks)vite.config.mts(2 hunks)
💤 Files with no reviewable changes (3)
- src/components/Common/DateInputV2.tsx
- public/locale/en.json
- src/Utils/useValueInjectionObserver.tsx
✅ Files skipped from review due to trivial changes (2)
- src/components/Questionnaire/QuestionTypes/QuestionInput.tsx
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (3)
src/components/Questionnaire/QuestionTypes/EncounterQuestion.tsx (1)
65-82: LGTM! Initial state is well-structured.The encounter state is properly initialized with all required fields and appropriate default values.
src/components/Questionnaire/QuestionnaireForm.tsx (2)
14-14: LGTM! Good practice exporting the interface for reuse.The export of
QuestionnaireFormStateinterface makes it properly reusable across the application, which is essential for type safety when working with questionnaire forms.Also applies to: 33-37
298-302: Verify plugin component integration.The integration with the plugin system looks correct, but we should ensure that the Scribe plugin properly handles the form state updates.
Let's verify the plugin integration:
✅ Verification successful
Plugin integration is correctly implemented ✅
The Scribe plugin integration is properly typed and follows the plugin system's design patterns. The component receives correctly typed form state props that match the
ScribeComponentTypeinterface definition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Scribe plugin implementation and usage # Check if the Scribe plugin component exists and implements the correct interface # Search for Scribe plugin component definition echo "Searching for Scribe plugin component implementation..." ast-grep --pattern 'export const Scribe: ScribeComponentType = $_' # Check for other usages of PLUGIN_Component with Scribe echo "Checking for other usages of PLUGIN_Component with Scribe..." rg -l "PLUGIN_Component.*__name=\"Scribe\""Length of output: 364
Script:
#!/bin/bash # Search for plugin-related files and Scribe implementations echo "Searching for plugin-related files..." fd -e ts -e tsx . | grep -i "plugin" echo "Searching for Scribe-related files..." fd . -t f | grep -i "scribe" echo "Searching for plugin type definitions..." rg "type.*Plugin" -A 3 echo "Looking for PLUGIN_Component definition..." rg "PLUGIN_Component" -A 2Length of output: 3352
Script:
#!/bin/bash # Search for ScribeComponentType definition and usage echo "Searching for ScribeComponentType definition..." rg "ScribeComponentType" -B 2 -A 5 echo "Checking for any Scribe-related interfaces..." rg "interface.*Scribe" -A 5Length of output: 1204
shivankacker
added
needs testing
needs review
and removed
Deploy-Failed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/setup-care-apps.ts (2)
47-47: Enhance generated code formatting.While the added newline improves readability, consider standardizing the spacing between different sections of the generated code.
Consider this improved template structure:
-const pluginMapContent = `// Use type assertion for the static import\n${plugins +const pluginMapContent = `// Use type assertion for the static import + +${plugins .map( (plugin) => `// @ts-expect-error Remote module will be available at runtime\nimport ${plugin.camelCaseName}Manifest from "${plugin.repo}/manifest";`, ) - .join("\n")} + .join("\n")}\n import type { PluginManifest } from "./pluginTypes"; +// Plugin manifest array const pluginMap: PluginManifest[] = [${plugins.map((plugin) => `${plugin.camelCaseName}Manifest as PluginManifest`).join(",\n ")}]; export { pluginMap }; `;
Line range hint
1-58: Document the new plugin configuration format and provide migration guidance.The script now supports a new URL-based plugin format, which is a significant architectural change that affects how plugins are configured.
Consider:
- Updating documentation to explain the new format:
url|package[@version]- Providing migration guidance for existing plugin configurations
- Adding examples in
.env.exampleor similar documentation- Creating a validation schema for the plugin configuration format
Would you like me to help create this documentation or open an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env(1 hunks)scripts/setup-care-apps.ts(2 hunks)vite.config.mts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .env
- vite.config.mts
| const package_ = app.includes("|") | ||
| ? app.split("|")[1].split("@")[0] | ||
| : app.split("@")[0]; | ||
| console.log(package_); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling for the new URL format.
The new package extraction logic supports URLs with pipe-separated format but lacks validation and error handling. This could lead to runtime issues with malformed input.
Consider applying these improvements:
- const package_ = app.includes("|")
- ? app.split("|")[1].split("@")[0]
- : app.split("@")[0];
- console.log(package_);
+ let package_: string;
+ if (app.includes("|")) {
+ const [url, pkgIdentifier] = app.split("|");
+ if (!url || !pkgIdentifier) {
+ throw new Error(`Invalid app format: ${app}. Expected format: "url|package[@version]"`);
+ }
+ package_ = pkgIdentifier.split("@")[0];
+ } else {
+ package_ = app.split("@")[0];
+ }Also, consider removing or conditionally enabling the debug log based on NODE_ENV.
📝 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.
| const package_ = app.includes("|") | |
| ? app.split("|")[1].split("@")[0] | |
| : app.split("@")[0]; | |
| console.log(package_); | |
| let package_: string; | |
| if (app.includes("|")) { | |
| const [url, pkgIdentifier] = app.split("|"); | |
| if (!url || !pkgIdentifier) { | |
| throw new Error(`Invalid app format: ${app}. Expected format: "url|package[@version]"`); | |
| } | |
| package_ = pkgIdentifier.split("@")[0]; | |
| } else { | |
| package_ = app.split("@")[0]; | |
| } |
Requires ohcnetwork/care_scribe_fe#9
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Configuration
Localization
Component Updates
Type Updates