Update Storybook & Start using jest #334
Conversation
…count activity, and showing jsdoc component descriptions in the default story
WalkthroughThe pull request introduces several updates primarily focused on enhancing the Storybook configuration and TypeScript support across various components and files. Key changes include modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Storybook
participant Component
participant Jest
User->>Storybook: Access stories
Storybook->>Component: Render component
Component-->>Storybook: Return rendered output
Storybook-->>User: Display stories
User->>Jest: Run tests
Jest->>Component: Execute test cases
Component-->>Jest: Return test results
Jest-->>User: Display test results
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (13)
.vscode/launch.json (1)
1-20: LGTM! Consider these suggestions for potential improvements.The VS Code launch configuration for unit tests is well-structured and appropriate for debugging Jest tests. Here are a few suggestions to consider:
The Jest config path (
${workspaceRoot}/build/src/test/jest.config.js) assumes a specific build structure. Ensure this path is correct and consider using a more flexible path if needed.The
--runInBandoption (line 13) runs tests sequentially, which might slow down execution for larger test suites. Consider removing this option if parallel execution is preferred and doesn't cause issues.The
outFilespattern (line 17) might be overly inclusive. Consider narrowing it down to specific directories where your compiled test files are located to improve debugging performance.Here's a suggested refinement for the
outFilespattern:- "outFiles": ["${workspaceFolder}/build/**/*.js", "!**/node_modules/**"] + "outFiles": ["${workspaceFolder}/build/src/**/*.js", "${workspaceFolder}/build/test/**/*.js", "!**/node_modules/**"]This change assumes your compiled source and test files are in
build/srcandbuild/testdirectories, respectively. Adjust the paths as needed to match your project structure.src/helpers/BasicPointsAndBadges/CustomActivity.ts (1)
Line range hint
1-15: Summary of changes and potential impactThe changes in this file involve:
- Removing the unused
MyDataHelpsimport.- Modifying the
awardCustomActivityPointsfunction signature by removing theactivityStateparameter.These changes appear to be part of a larger refactoring effort mentioned in the PR objectives. To ensure the integrity of the codebase:
- Verify that the removal of the
activityStateparameter doesn't break any existing functionality.- Ensure that the new unit tests for Custom activities (mentioned in the PR objectives) thoroughly cover the updated function behavior.
- Update any documentation or comments related to this function to reflect the new signature.
- Consider reviewing other files that might be affected by these changes, especially those that call the
awardCustomActivityPointsfunction.Given the significance of these changes, it might be beneficial to include a brief comment explaining the rationale behind removing the
activityStateparameter, either in the code or in the PR description..storybook/preview.js (1)
49-49: Excellent addition of autodocs!The addition of
export const tags = ["autodocs"];is a great improvement. This enables Storybook's automatic documentation generation feature, which aligns well with the PR objective of enhancing documentation.To further improve this change, consider the following suggestions:
- Ensure that all components, especially BasicPointsForBadges, have proper JSDoc comments to make the most of the autodocs feature.
- You might want to add more tags if needed, such as
["autodocs", "docs"]to enable both autodocs and manual docs pages.Here's an example of how you could expand this:
export const tags = ["autodocs", "docs"];This would allow for both automatic and manual documentation, giving you more flexibility in how you present component information.
src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts (2)
8-8: LGTM: Interface update improves type safetyThe change from
string[]toConnectExternalActivityType[]forproviderCategoriesis consistent with the new type definition and enhances type safety. This ensures that only valid provider categories can be used in the activity.Consider adding a JSDoc comment to the
ConnectExternalAccountActivityinterface to describe its purpose and the significance of theproviderCategoriesproperty. This would improve code documentation and make it easier for other developers to understand and use this interface correctly.
1-23: Overall assessment: Good improvements with room for minor enhancementsThe changes in this file significantly improve type safety by introducing the
ConnectExternalActivityTypeand updating theConnectExternalAccountActivityinterface. The modifications to theawardConnectExternalAccountActivityPointsfunction maintain backwards compatibility while incorporating the new type system.Key points:
- The new type definition enhances code clarity and restricts possible values for provider categories.
- The interface update is consistent with the new type and improves type safety.
- The function changes maintain compatibility while accommodating the new type system.
Suggestions for further improvement:
- Add JSDoc comments to improve documentation, especially for the
ConnectExternalAccountActivityinterface.- Consider using a type guard instead of casting in the
awardConnectExternalAccountActivityPointsfunction.- Clarify the necessity of casting to
string[]and consider adding error handling for unexpected types.As the codebase continues to evolve, consider gradually updating other parts of the system to fully leverage the new
ConnectExternalActivityType. This will help maintain consistency and maximize the benefits of the improved type safety throughout the project.src/helpers/BasicPointsAndBadges/CustomActivity.test.ts (2)
5-35: Consider refining the ParticipantInfo objectThe ParticipantInfo object is comprehensive, but there are a few points to consider:
- Many fields are set to empty strings. Consider omitting these or setting them to null if they're required.
- The custom field "DailyGoals" is set to "8" as a string. If this represents a numeric value, consider using a number instead.
Here's a suggested refinement:
const pptInfo: ParticipantInfo = { participantID: "123", participantIdentifier: "123", secondaryIdentifier: "123", linkIdentifier: "123", demographics: { city: 'Naples', utcOffset: '-05:00:00', email: "abc@ce.com", firstName: "John", lastName: "Doe", mobilePhone: "1234567890", state: "FL" }, enrollmentDate: new Date().toString(), projectID: "123", customFields: { DailyGoals: 8 } };This removes empty fields and changes the DailyGoals value to a number.
49-59: Improve clarity of the negative test caseThe test case effectively checks that no points are awarded when the custom field is not present. However, the naming of the custom field "DailyGoalsMissing" might be slightly confusing.
Consider renaming the custom field to something that clearly indicates it doesn't exist in the participant info, for example:
const activity: CustomActivity = { key: "CustomGoals", type: "custom", points: 10, customField: "NonExistentField" };This makes it clearer that we're testing for a field that doesn't exist, rather than one that's explicitly marked as "missing".
src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (1)
1-98: Consider these minor improvements for enhanced test quality.While the test file is well-structured and comprehensive, consider the following suggestions:
Replace type assertions with explicit typing:
Instead ofas ConnectExternalAccountActivity, consider defining the activity object with explicit typing:const activity: ConnectExternalAccountActivity = { type: "connectExternalAccount", points: 10, providerCategories: ["Health Plan"] };Make test descriptions more specific:
For example, "should award points for connecting a single external account" could be "should award 10 points for connecting a single Health Plan account".Add a test case for multiple provider categories:
Consider adding a test where the activity specifies multiple provider categories to ensure the function handles this scenario correctly.These suggestions aim to improve the clarity and coverage of your tests. Let me know if you'd like assistance implementing any of these changes.
src/stories/Introduction.mdx (3)
15-113: Approve CSS changes with a minor suggestionThe CSS changes significantly improve the layout, responsiveness, and visual appeal of the documentation. The use of media queries and modern CSS properties is commendable. However, there's a small opportunity for improvement:
Consider using CSS custom properties (variables) for repeated values like colors and dimensions. This would enhance maintainability. For example:
<style> {` + :root { + --primary-color: #1EA7FD; + --text-color: #333333; + --border-color: #00000010; + } .subheading { - --mediumdark: '#999999'; + --mediumdark: #999999; font-weight: 900; font-size: 13px; - color: #999; + color: var(--mediumdark); letter-spacing: 6px; line-height: 24px; text-transform: uppercase; margin-bottom: 12px; margin-top: 40px; } /* ... other styles ... */ .link-item { display: block; padding: 20px 30px 20px 15px; - border: 1px solid #00000010; + border: 1px solid var(--border-color); border-radius: 5px; transition: background 150ms ease-out, border 150ms ease-out, transform 150ms ease-out; - color: #333333; + color: var(--text-color); display: flex; align-items: flex-start; } .link-item:hover { - border-color: #1EA7FD50; + border-color: var(--primary-color); transform: translate3d(0, -3px, 0); box-shadow: rgba(0, 0, 0, 0.08) 0 3px 10px 0; } .link-item:active { - border-color: #1EA7FD; + border-color: var(--primary-color); transform: translate3d(0, 0, 0); } /* ... other styles ... */ `} </style>This change would make it easier to maintain consistent colors and update the theme in the future.
Line range hint
128-171: Approve usage instructions with a minor suggestionThe updated usage instructions are clear, concise, and well-formatted. The addition of line breaks and bullet points improves readability. However, there's a small opportunity for improvement:
For consistency, consider using numbered lists for all step-by-step instructions. Currently, the steps for running Storybook previews are numbered, but the installation steps are not. Here's a suggested change:
# Using MyDataHelpsUI in your participant view -Install via: +1. Install via npm: `npm install @careevolution/mydatahelps-ui` -### Example of how to use MyDataHelpsUI in your Participant View +2. Use MyDataHelpsUI components in your Participant View: ```jsx import { Action, Card, Layout, SurveyTaskList } from "@careevolution/mydatahelps-ui" export default function () { // ... (rest of the example code) }This change would make the documentation structure more consistent and easier to follow.
🧰 Tools
🪛 Gitleaks
166-166: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
171-173: Approve final notes with a minor suggestionThe addition of the note about potential Node version issues and the copyright notice are both valuable inclusions.
To further improve the troubleshooting note, consider adding a link to the Node.js download page or version requirements. For example:
-If you have issues getting storybook to run, particuarly around the NODE\_OPTIONS being used, try upgrading Node to the latest version. +If you have issues getting storybook to run, particularly around the NODE_OPTIONS being used, try upgrading Node to the latest LTS version. You can download it from [nodejs.org](https://nodejs.org/).This change would provide more specific guidance to users encountering issues.
src/components/container/BasicPointsForBadges/BasicPointsForBadges.tsx (1)
26-33: Enhance the JSDoc comment for better documentationThe addition of the JSDoc comment is a great improvement to the component's documentation. However, consider the following enhancements to make it more comprehensive:
- Include descriptions of key props like
pointsPerBadgeandawardBadgesViewUrl.- Explain the
previewStateprop and its impact on the component's behavior.- Improve formatting for better readability.
Here's a suggested improvement:
/** * BasicPointsForBadges: Earn points for completing activities and receive badges. * * @description * This component displays a progress bar that fills as points are earned. When the * points reach `pointsPerBadge`, a badge is awarded, triggering a celebration step, * and the progress bar resets. * * Supported activity types: * - Survey Completion * - Daily device data (e.g., sleep and steps) * - Connecting external accounts (EHR, Health Plan, or wearable device) * - Custom field value * * @param {Object} props * @param {number} props.pointsPerBadge - Number of points required to earn a badge * @param {BasicPointsForBadgesActivity[]} props.activities - List of activities that can earn points * @param {string} props.awardBadgesViewUrl - URL to open when a new badge is awarded * @param {string} [props.previewState] - If set to "default", enables preview mode with mock data * @param {ColorDefinition} [props.progressBarFillColor] - Color of the progress bar fill * @param {ColorDefinition} [props.pointsLabelColor] - Color of the points label * @param {boolean} [props.showTotalPoints] - If true, shows total points instead of points towards next badge * @param {string} props.customField - Custom field to store points and badges state */src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx (1)
14-21: LGTM: Improved Storybook configuration with better typing and documentation.The changes to the meta object improve type safety and allow for better documentation within Storybook. This aligns well with the PR objectives.
Consider adding a brief description for the
BasicPointsForBadgescomponent using theDescriptioncomponent. This would further enhance the documentation. For example:docs: { description: { component: 'BasicPointsForBadges is a component that displays points and badges for various activities.' } }Also applies to: 25-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
- .storybook/main.js (1 hunks)
- .storybook/preview.js (1 hunks)
- .vscode/launch.json (1 hunks)
- package.json (4 hunks)
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx (4 hunks)
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.tsx (1 hunks)
- src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (1 hunks)
- src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts (1 hunks)
- src/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1 hunks)
- src/helpers/BasicPointsAndBadges/CustomActivity.ts (1 hunks)
- src/helpers/BasicPointsAndBadges/PointsAndBadges.ts (1 hunks)
- src/stories/Introduction.mdx (4 hunks)
🧰 Additional context used
🪛 Gitleaks
src/stories/Introduction.mdx
166-166: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (23)
.storybook/main.js (4)
4-10: Approve new Storybook addonsThe addition of "@storybook/addon-webpack5-compiler-babel" and "@chromatic-com/storybook" addons aligns with the PR objectives of updating Storybook and enhancing testing capabilities.
Please ensure that these new addons are compatible with your project's requirements and other dependencies. Run the following script to check for any potential conflicts:
#!/bin/bash # Description: Verify compatibility of new Storybook addons # Test: Check if the new addons are properly installed echo "Checking if new addons are installed:" npm list @storybook/addon-webpack5-compiler-babel @chromatic-com/storybook # Test: Look for any warnings or errors related to these addons in the package.json echo "Checking for warnings or errors in package.json:" npm ls @storybook/addon-webpack5-compiler-babel @chromatic-com/storybook
17-17: Clarify the removal of "autodocs" settingThe "autodocs" setting has been removed from the "docs" property, resulting in an empty object. This change might affect the automatic generation of documentation for stories.
Could you please clarify the reason for this change and its potential impact on the project's documentation?
To assess the impact, run the following script:
#!/bin/bash # Description: Check for existing autodocs usage # Test: Search for files that might be using autodocs echo "Searching for files potentially using autodocs:" rg -i 'autodoc' --type js --type ts --type jsx --type tsx # Test: Check if there are any manual doc blocks that might replace autodocs echo "Checking for manual doc blocks:" rg -i '@storybook/addon-docs' --type js --type ts --type jsx --type tsx
19-21: Approve TypeScript configuration for documentationThe addition of the "typescript" property with "reactDocgen: "react-docgen-typescript"" is a positive change that aligns with the PR objectives of improving documentation.
To ensure that this configuration is properly set up and working as expected, please run the following script:
#!/bin/bash # Description: Verify TypeScript setup and react-docgen-typescript usage # Test: Check if TypeScript is properly installed echo "Checking TypeScript installation:" npm list typescript # Test: Verify react-docgen-typescript is installed echo "Verifying react-docgen-typescript installation:" npm list react-docgen-typescript # Test: Look for TypeScript configuration file echo "Checking for tsconfig.json:" cat tsconfig.json # Test: Search for TypeScript React components echo "Searching for TypeScript React components:" fd -e tsx -e ts
2-2: Verify the impact of removing ".stories.mdx" patternThe removal of "../src/**/*.stories.mdx" from the "stories" array might affect how Storybook discovers and loads stories. Please ensure that this change doesn't break any existing stories.
Consider keeping the ".stories.mdx" pattern for backward compatibility:
- "stories": ["../src/**/*.mdx", "../src/**/*.stories.@(js|jsx|ts|tsx)"], + "stories": ["../src/**/*.mdx", "../src/**/*.stories.@(js|jsx|ts|tsx|mdx)"],To verify the impact, run the following script:
✅ Verification successful
Verification Successful: Removal of
.stories.mdxPatternNo
.stories.mdxfiles were found in the codebase. Removing the"../src/**/*.stories.mdx"pattern will not affect Storybook's ability to discover and load existing stories.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any .stories.mdx files that might be affected # Test: Search for .stories.mdx files echo "Searching for .stories.mdx files:" fd -e stories.mdx # Test: Check if these files are still being loaded by Storybook echo "Checking if these files are referenced in Storybook:" rg --type js -g '*.stories.*' -i 'stories\.mdx'Length of output: 47826
src/helpers/BasicPointsAndBadges/CustomActivity.ts (2)
1-1: LGTM: Import statement cleanupThe removal of the unused
MyDataHelpsimport and retaining only the necessaryParticipantInfoimport is a good practice. This change helps to keep the code clean and reduces potential confusion about dependencies.
9-9: Verify the impact of removing theactivityStateparameterThe removal of the
activityStateparameter from the function signature is a significant change. Please ensure that:
- This change is intentional and aligns with the overall design of the points awarding system.
- All calls to this function throughout the codebase have been updated accordingly.
- The function's internal logic still works correctly without the
activityStateparameter.- The newly created unit tests (mentioned in the PR objectives) cover this updated function signature and behavior.
Also, consider if the
newActivityStateobject creation within the function is still necessary or if it should be refactored.Would you like me to generate a script to check for any remaining calls to this function with the old signature?
.storybook/preview.js (1)
Line range hint
29-30: Security: Handle sensitive information with cautionWhile using environment variables for the access token is a good practice, having it directly in the code could still pose a security risk if the file is accidentally shared or if the environment variable is mishandled.
Consider the following improvements:
- Move the token handling logic to a separate, gitignored file.
- Use a more secure method for token management, such as a secret management service.
- Add a clear comment warning about the sensitivity of this information.
Here's an example of how you could refactor this:
// In a separate file, e.g., tokenHandler.js (add to .gitignore) import { getToken } from './secureTokenService'; // Implement this service export const setParticipantToken = async () => { const token = await getToken(); myDataHelps.setParticipantAccessToken(token, process.env.PARTICIPANT_ENVIRONMENT_API || "https://mydatahelps.org/"); }; // In preview.js import { setParticipantToken } from './tokenHandler'; export const decorators = [(story, context) => { setParticipantToken(); // ... rest of the decorator logic }];To verify that no sensitive information is exposed, run the following script:
#!/bin/bash # Description: Check for potential exposure of sensitive information echo "Checking for potential sensitive information in .storybook/preview.js:" rg --type js 'access_token|secret|password' .storybook/preview.js echo "Checking if tokenHandler.js is properly gitignored:" grep 'tokenHandler.js' .gitignore || echo "Warning: tokenHandler.js is not in .gitignore"This script will help ensure that sensitive information is properly handled and that the new file is correctly gitignored.
src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts (2)
4-4: LGTM: New type improves type safetyThe introduction of
ConnectExternalActivityTypeas a union type enhances type safety by explicitly defining the allowed values for provider categories. This change aligns with TypeScript best practices and improves code clarity.
16-22: Approved with suggestions: Function update maintains compatibilityThe changes to
awardConnectExternalAccountActivityPointsfunction maintain compatibility with existing code while accommodating the new type system. The casting tostring[]and the use ofactivityProviderCategoriesin the filtering logic achieve this goal.Consider the following suggestions to improve the code:
- Instead of casting to
string[], you could use a type guard to ensure type safety:const activityProviderCategories = activity.providerCategories.filter((category): category is string => typeof category === 'string');
- To future-proof the code, you might want to add a warning or error logging when encountering unexpected types in
activity.providerCategories.Could you clarify why the casting to
string[]is necessary? Are there specific scenarios whereactivity.providerCategoriesmight contain values that are not part ofConnectExternalActivityType?✅ Verification successful
Verified: Function update maintains compatibility
The changes to the
awardConnectExternalAccountActivityPointsfunction have been verified across the codebase. Castingactivity.providerCategoriestostring[]is consistent with existing usages ofConnectExternalAccountActivity, ensuring compatibility and preventing potential type mismatches.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of ConnectExternalAccountActivity to understand potential incompatibilities rg --type typescript -A 5 "ConnectExternalAccountActivity"Length of output: 93
Script:
#!/bin/bash # Retry searching for usages of ConnectExternalAccountActivity in TypeScript files using glob pattern rg -g "*.ts" -A 5 "ConnectExternalAccountActivity"Length of output: 10021
src/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1)
36-46: Clarify point calculation logicThe test expects 80 points to be awarded, but the activity object defines only 10 points. The calculation logic for reaching 80 points is not clear from the test case.
Could you please clarify:
- How are the 80 points calculated?
- Is there a multiplication factor based on the "DailyGoals" value?
- Should we add a comment explaining this calculation for better test readability?
Consider adding a comment explaining the point calculation logic, for example:
// Points are calculated as: activity.points * parseInt(pptInfo.customFields[activity.customField]) // In this case: 10 * 8 = 80 const newActivityState = await awardCustomActivityPoints(activity, pptInfo); expect(newActivityState.pointsAwarded).toBe(80);This would greatly improve the test's clarity and maintainability.
src/helpers/BasicPointsAndBadges/PointsAndBadges.ts (1)
39-39: Verify the impact of removingactivityStatefromawardCustomActivityPoints.The
activityStateparameter has been removed from theawardCustomActivityPointsfunction call for custom activities. This change might affect the behavior of custom activities and how they're tracked over time.Please confirm that this change is intentional and doesn't negatively impact the functionality of custom activities. Consider the following:
- Does this change align with the intended behavior for custom activities?
- Are there any potential side effects of not considering the previous activity state?
- How does this change affect the consistency of handling different activity types?
✅ Verification successful
Verified removal of
activityStateparameter fromawardCustomActivityPoints.The
activityStateparameter has been successfully removed from theawardCustomActivityPointsfunction call inPointsAndBadges.ts. All usages and tests reflect this change, ensuring consistent handling of custom activities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of awardCustomActivityPoints across the codebase # Test: Search for all occurrences of awardCustomActivityPoints echo "Occurrences of awardCustomActivityPoints:" rg --type typescript "awardCustomActivityPoints" -A 3 # Test: Check the implementation of awardCustomActivityPoints echo "\nImplementation of awardCustomActivityPoints:" ast-grep --lang typescript --pattern 'function awardCustomActivityPoints($_) { $$$ }'Length of output: 372
Script:
#!/bin/bash # Description: Verify the usage of awardCustomActivityPoints across the codebase # Test: Search for all occurrences of awardCustomActivityPoints in .ts and .tsx files echo "Occurrences of awardCustomActivityPoints in .ts and .tsx files:" rg "awardCustomActivityPoints" --glob "*.ts" --glob "*.tsx" -A 3 # Test: Check the implementation of awardCustomActivityPoints with corrected language echo "\nImplementation of awardCustomActivityPoints:" ast-grep --lang typescript --pattern 'function awardCustomActivityPoints($_, $_?) { $$$ }'Length of output: 2976
package.json (3)
47-47: LGTM: Added @chromatic-com/storybook dependencyThe addition of @chromatic-com/storybook is a good enhancement for visual testing and reviewing Storybook components. This aligns with the overall goal of improving testing in the project.
However, could you please clarify the specific purpose for adding this dependency, as it wasn't explicitly mentioned in the PR objectives?
54-61: LGTM: Updated Storybook dependenciesThe Storybook dependencies have been successfully updated to version 8.3.5, which is a significant upgrade from the previous version 7.0.12. This aligns with the PR objective of upgrading Storybook.
A new addon (@storybook/addon-webpack5-compiler-babel) has also been added, which might be related to the upgrade process.
However, I have two questions:
- The PR objectives mentioned upgrading to Storybook 8.5, but the actual version is 8.3.5. Is this intentional or should we update to 8.5?
- Could you please explain the purpose of adding the new addon-webpack5-compiler-babel?
23-24: LGTM: Updated build-storybook script and added test scriptThe changes to the scripts section look good:
- The minor formatting change to the build-storybook script (adding a trailing comma) improves consistency.
- The addition of the test script ("test": "jest") aligns with the PR objective of introducing Jest for testing.
To ensure the test script is working correctly, please run:
#!/bin/bash # Run the test script and check for successful execution npm test || echo "Test script failed"src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (2)
1-36: LGTM: Imports and mock setup are well-structured.The imports are correctly defined, and the mock for
@careevolution/mydatahelps-jsis comprehensive. It provides the necessary data for testing, including both default and named exports. This setup will allow for effective testing of theConnectExternalAccountActivityfunctionality.
38-98: LGTM: Well-structured and comprehensive test suite.The test suite is well-organized and covers various scenarios:
- Connecting a single external account
- Connecting multiple devices
- Appending points and resetting providers
- Handling cases where the provider is not connected
Each test case is clearly defined with appropriate expectations. The use of
awaitwithawardConnectExternalAccountActivityPointscorrectly handles the asynchronous nature of the function.src/stories/Introduction.mdx (1)
1-1: Verify Storybook version compatibilityThe import statement for
Metahas been updated from '@storybook/addon-docs' to '@storybook/blocks'. This change likely reflects an update in the Storybook library structure. Please ensure that this change is compatible with the Storybook version specified in yourpackage.json.src/components/container/BasicPointsForBadges/BasicPointsForBadges.tsx (2)
34-34: Approve function signature changeThe update from an unnamed default export to a named export
BasicPointsForBadgesis a good improvement. This change enhances code clarity and follows React best practices for component naming.
Line range hint
1-114: Additional considerations based on PR objectives
Unit Tests: The PR objectives mention adding unit tests for Custom and ExternalAccount activities. Ensure these tests have been added in the appropriate test file.
Security Check: As per the PR objectives, please double-check that no sensitive information or temporary testing changes have been accidentally committed in this file or any other files in the PR.
Activity Handling: The component seems to handle various activity types, including Custom and ExternalAccount. Confirm that any changes required for these specific activity types have been implemented correctly, even if not directly visible in this file.
To help verify the absence of sensitive information, you can run the following script:
Would you like assistance in reviewing the unit tests for Custom and ExternalAccount activities?
src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx (4)
11-12: LGTM: Updated imports for Storybook.The new imports from '@storybook/blocks' and '@storybook/react' are appropriate for the updated Storybook configuration. These changes align with modern Storybook practices and improve type safety.
26-26: LGTM: Added type definition for stories.The addition of the
Storytype usingStoryObj<typeof BasicPointsForBadges>improves type safety for the stories. This is a best practice in Storybook and enhances code maintainability.
34-34: LGTM: Updated story exports with proper typing.The updates to the story exports (
Default,ShowTotalPoints,WithGoalsAndBadges, andLive) to use theStorytype improve type safety while maintaining the existing functionality. These changes are consistent across all stories and align with best practices for Storybook.Also applies to: 52-52, 72-72, 151-151
Line range hint
1-191: Overall improvements align with PR objectives, but one item is missing.The changes in this file successfully update the Storybook configuration, improve type safety, and enhance documentation, which aligns well with the PR objectives. However, one item mentioned in the PR objectives is missing:
The PR objectives mentioned adding a JSDoc comment at the component level for BasicPointsForBadges, which should also be rendered as the description of the Default story. This comment is not present in the current changes.
To fully meet the PR objectives, consider adding a JSDoc comment to the
BasicPointsForBadgescomponent in its implementation file. Then, import and use this comment in the Storybook configuration. Here's a script to verify if the JSDoc comment exists:If the JSDoc comment is not present, please add it to the component file and then update this Storybook file to use that comment as the description.
| } |
There was a problem hiding this comment.
💡 Codebase verification
Action Logging May Be Affected by Removal of actions Parameter
No manual action declarations (actions:) were found in the story files. Removing the automatic actions parameter might lead to missing action logs in Storybook.
🔗 Analysis chain
Verify the removal of automatic action logging.
The actions property has been removed from the parameters object. This property was previously used to automatically add actions for props matching the regex pattern "^on[A-Z].*".
Please confirm if this removal was intentional. If so, consider the following:
- How will action logging be handled for components now?
- Is there a new strategy for managing actions in Storybook?
- Does this change align with the PR objective of enhancing testing?
If this removal was unintentional, consider restoring the actions property:
export const parameters = {
controls: {
matchers: {
color: /(background|color)$/i,
date: /Date$/,
},
},
+ actions: { argTypesRegex: "^on[A-Z].*" },
}To verify the impact of this change, we can run the following script:
This script will help us understand if there are manual action declarations in place that might compensate for the removal of automatic action logging.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for manual action declarations in story files
# Search for manual action declarations in story files
echo "Searching for manual action declarations in story files:"
rg --type js --type ts 'actions:' src
Length of output: 721
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tsconfig.json (1)
16-17: LGTM! Consider adding"declaration": truefor better package support.The changes to the TypeScript configuration are well-aligned with the PR objectives and enhance the project's setup. The new options improve TypeScript and React support, which is great for the Storybook upgrade and Jest integration.
As an additional suggestion, consider adding the
"declaration": trueoption. This will generate.d.tsfiles, which can be beneficial if you're planning to publish this as an npm package (as mentioned in the PR objectives about exporting descriptions).Here's how you can add it:
"compilerOptions": { // ... other options ... "allowSyntheticDefaultImports": true, "outDir": "./dist", + "declaration": true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- package.json (3 hunks)
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx (5 hunks)
- src/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1 hunks)
- src/stories/Introduction.mdx (4 hunks)
- tsconfig.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx
- src/helpers/BasicPointsAndBadges/CustomActivity.test.ts
- src/stories/Introduction.mdx
🧰 Additional context used
🔇 Additional comments (5)
package.json (5)
23-24: LGTM: Test script added and build-storybook updatedThe addition of the Jest test script aligns with the PR objective of introducing Jest as a testing framework. The trailing comma in the "build-storybook" script is a minor syntax change that improves consistency.
47-63: LGTM: Dependencies updated and new testing dependencies addedThe updates to Storybook dependencies (to version 8.3.5) align with the PR objective of upgrading Storybook. The addition of Jest-related dependencies supports the introduction of Jest as a testing framework.
It's good to see that the previous suggestion to move @storybook/addon-jest to devDependencies has been implemented.
69-69: LGTM: Jest added as a devDependencyThe addition of Jest (version 29.7.0) as a devDependency aligns perfectly with the PR objective of introducing Jest as a testing framework.
78-78: LGTM: Storybook updated to version 8.3.5The update of Storybook to version 8.3.5 is consistent with the other Storybook-related dependencies and aligns with the PR objective of upgrading Storybook.
Line range hint
1-112: Overall: LGTM - Package.json updates align with PR objectivesThe changes in this file successfully implement the PR objectives:
- Storybook has been upgraded to version 8.3.5.
- Jest has been introduced as a testing framework.
- The package structure has been modernized with the addition of a browser export.
All past review comments have been addressed, and the changes are consistent and well-implemented.
| "browser": "./dist/browser/index.js", | ||
| "require": "./dist/cjs/index.js", | ||
| "import": "./dist/esm/index.js", | ||
| "browser": "./dist/browser/index.js" | ||
| "import": "./dist/esm/index.js" |
There was a problem hiding this comment.
💡 Codebase verification
Issue: Browser bundle is missing
The ./dist/browser/index.js file does not exist, making the browser export invalid. Please ensure that the browser bundle is correctly generated during the build process.
🔗 Analysis chain
LGTM: Browser export added
The addition of the browser export is a good practice for improved compatibility. This change aligns with modern package structure standards.
Please ensure that the ./dist/browser/index.js file is correctly generated during the build process. Run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the browser bundle
if [ -f dist/browser/index.js ]; then
echo "Browser bundle exists."
else
echo "Browser bundle not found."
fi
Length of output: 95
Script:
#!/bin/bash
# List the contents of the dist directory
echo "Contents of dist/:"
ls -l dist/
# Check if the dist/browser directory exists and list its contents
if [ -d dist/browser ]; then
echo "Contents of dist/browser/:"
ls -l dist/browser/
else
echo "dist/browser directory does not exist."
fi
Length of output: 226
I appreciate that feedback. I do like the tests within the same folder as the component bc it encourages creating it as part of component/helper completion and makes the tests easy to find/reference. I am interested in hearing others thoughts too. |
I tend to prefer a separate We'll also want to make sure that the tests and other test related files are not included in the published builds. That'll probably be easiest if we can just exclude (or not include) the |
|
Just putting this here so we don't forget, but the existing PR about upgrading the Storybook can probably go away once this one gets merged. |
…nfigs in npmignore. Run tests as part of amplify deploy
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
amplify.yml (1)
Line range hint
13-15: Consider adding test results to build artifacts.While we're storing the Storybook static files, it would be valuable to also preserve test results for debugging failed builds.
Add test results to artifacts:
artifacts: baseDirectory: /storybook-static/ files: - '**/*' + - '../test-results/**/*'__tests__/helpers/BasicPointsAndBadges/CustomActivity.test.ts (3)
35-45: Enhance test coverage with additional assertions.While the basic point calculation is tested, consider:
- Adding assertions for other properties of newActivityState
- Documenting the point calculation formula (8 * 10 = 80)
it("should award points based on custom field value", async () => { + // Points calculation: customFieldValue (8) * points (10) = 80 const activity : CustomActivity = { key: "CustomGoals", type: "custom", points: 10, customField: "DailyGoals" }; const newActivityState = await awardCustomActivityPoints(activity, pptInfo); expect(newActivityState.pointsAwarded).toBe(80); + expect(newActivityState.key).toBe("CustomGoals"); + expect(newActivityState.type).toBe("custom"); });
48-58: Enhance error handling test coverage.The test verifies points calculation but should also ensure proper error handling:
- Verify no errors are thrown for missing fields
- Make the test description more explicit
- it("should NOT award points where missing custom field value", async () => { + it("should safely handle missing custom fields by awarding 0 points", async () => { const activity : CustomActivity = { key: "CustomGoals", type: "custom", points: 10, customField: "DailyGoalsMissing" }; - const newActivityState = await awardCustomActivityPoints(activity, pptInfo); - expect(newActivityState.pointsAwarded).toBe(0); + await expect(awardCustomActivityPoints(activity, pptInfo)).resolves.toEqual( + expect.objectContaining({ + pointsAwarded: 0, + key: "CustomGoals" + }) + ); });
60-74: Expand decimal handling test coverage.The current test only covers one decimal case. Consider:
- Adding test cases for edge cases (e.g., very small decimals, large decimals)
- Explicitly documenting and verifying the rounding behavior
+ it.each([ + { value: "1.2", points: 8, expected: 9.6 }, + { value: "0.1", points: 10, expected: 1.0 }, + { value: "99.9", points: 1, expected: 99.9 } + ])('should handle decimal value $value with $points points', async ({ value, points, expected }) => { + const pptInfoWithDecimal = { + ...pptInfo, + customFields: { DailyGoals: value } + }; + const activity: CustomActivity = { + key: "CustomGoals", + type: "custom", + points, + customField: "DailyGoals" + }; + + const newActivityState = await awardCustomActivityPoints(activity, pptInfoWithDecimal); + expect(newActivityState.pointsAwarded).toBe(expected); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
- .npmignore (1 hunks)
- tests/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (1 hunks)
- tests/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1 hunks)
- amplify.yml (1 hunks)
- package.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🔇 Additional comments (3)
.npmignore (1)
4-5: LGTM! The test file exclusion patterns support flexible organization.The patterns support both discussed test organization strategies:
__tests__/for dedicated test directories*.test.*for co-located test filesamplify.yml (1)
10-10: Verify Jest compatibility with Node.js 16.The addition of the test command is a good practice for ensuring quality before deployment. However, since the build uses Node.js 16, we should verify Jest compatibility.
✅ Verification successful
Based on the results, I can now provide a final response since we have confirmed the Jest version being used.
Jest 29.7.0 is fully compatible with Node.js 16
Jest 29.7.0 supports Node.js 14, 16, and 18, so there are no compatibility concerns with the Node.js 16 environment specified in the build configuration. The test command can be safely executed in the build pipeline.
Note: I'm marking this as verified because Jest 29.7.0 (as shown in package.json) officially supports Node.js 16. This version of Jest was released with explicit support for Node.js 14, 16, and 18, making it fully compatible with the build environment. The original review comment's concern about compatibility has been addressed through the version information found in package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check package.json for Jest version and its Node.js compatibility # Test: Look for Jest version in package.json and any engine requirements rg -A 5 '"jest":' package.json rg '"engines":' package.jsonLength of output: 227
__tests__/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1)
1-2: LGTM! Clean imports with proper type definitions.
| jest.mock('@careevolution/mydatahelps-js', () => { | ||
|
|
||
| const externalAccounts = [{ | ||
| "id" : 1, | ||
| "provider": { "id": 1, "name": "Garmin", "category": "Device Manufacturer"} | ||
| }, | ||
| { | ||
| "id" : 2, | ||
| "provider": { "id": 2, "name": "FitBit", "category": "Device Manufacturer"} | ||
| }, | ||
| { | ||
| "id" : 3, | ||
| "provider": { "id": 3, "name": "Health Plan 1", "category": "Health Plan"} | ||
| }]; | ||
|
|
||
| return { | ||
| __esModule: true, | ||
| default: { | ||
| token: { access_token: '1' }, | ||
| getCurrentLanguage: () => jest.fn(), | ||
| getExternalAccounts: jest.fn(() => { | ||
| return Promise.resolve(externalAccounts); | ||
| }), | ||
| on: jest.fn(), | ||
| off: jest.fn(), | ||
| }, | ||
| MyDataHelps: { | ||
| getCurrentLanguage: () => jest.fn(), | ||
| getExternalAccounts: jest.fn(), | ||
| on: jest.fn(), | ||
| off: jest.fn(), | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve mock implementation efficiency and maintainability.
Consider the following improvements to the mock setup:
- Remove unused mock methods (getCurrentLanguage, on, off) to keep the mock focused.
- Consolidate the duplicate mock structure between default export and MyDataHelps export.
- Implement the empty getExternalAccounts mock in the MyDataHelps object.
- Move test data to a separate fixture file for reusability across tests.
Here's the suggested implementation:
// __tests__/fixtures/externalAccounts.ts
export const mockExternalAccounts = [{
"id": 1,
"provider": { "id": 1, "name": "Garmin", "category": "Device Manufacturer" }
}, {
"id": 2,
"provider": { "id": 2, "name": "FitBit", "category": "Device Manufacturer" }
}, {
"id": 3,
"provider": { "id": 3, "name": "Health Plan 1", "category": "Health Plan" }
}];
// ConnectExternalAccountActivity.test.ts
import { mockExternalAccounts } from '../fixtures/externalAccounts';
jest.mock('@careevolution/mydatahelps-js', () => {
const implementation = {
getExternalAccounts: jest.fn(() => Promise.resolve(mockExternalAccounts))
};
return {
__esModule: true,
default: {
token: { access_token: '1' },
...implementation
},
MyDataHelps: implementation
}
});| describe("ConnectExternalAccountActivity Awards", () => { | ||
| it("should award points for connecting a single external account", async () => { | ||
| const activity = { | ||
| type: "connectExternalAccount", | ||
| points: 10, | ||
| providerCategories: ["Health Plan"] | ||
| } as ConnectExternalAccountActivity; | ||
| const activityState = { | ||
| pointsAwarded: 0, | ||
| providersConnected: [] | ||
| }; | ||
| const newActivityState = await awardConnectExternalAccountActivityPoints(activity, activityState); | ||
| expect(newActivityState.pointsAwarded).toBe(10); | ||
| expect(newActivityState.providersConnected).toEqual([3]); | ||
| }); | ||
|
|
||
| it("should award points for connecting multiple devices", async () => { | ||
| const activity = { | ||
| type: "connectExternalAccount", | ||
| points: 10, | ||
| providerCategories: ["Device Manufacturer"] | ||
| } as ConnectExternalAccountActivity; | ||
| const activityState = { | ||
| pointsAwarded: 0, | ||
| providersConnected: [] | ||
| }; | ||
| const newActivityState = await awardConnectExternalAccountActivityPoints(activity, activityState); | ||
| expect(newActivityState.pointsAwarded).toBe(20); | ||
| expect(newActivityState.providersConnected).toEqual([1, 2]); | ||
| }); | ||
|
|
||
| it("should append points and reset providers connected if not on file", async () => { | ||
| const activity = { | ||
| type: "connectExternalAccount", | ||
| points: 10, | ||
| providerCategories: ["Health Plan"] | ||
| } as ConnectExternalAccountActivity; | ||
| const activityState = { | ||
| pointsAwarded: 10, | ||
| providersConnected: [1,2] | ||
| }; | ||
| const newActivityState = await awardConnectExternalAccountActivityPoints(activity, activityState); | ||
| expect(newActivityState.pointsAwarded).toBe(20); | ||
| expect(newActivityState.providersConnected).toEqual([3]); | ||
| }); | ||
|
|
||
| it("should NOT award points when the ppt does not have this provider connected", async () => { | ||
| const activity = { | ||
| type: "connectExternalAccount", | ||
| points: 10, | ||
| providerCategories: ["Provider"] | ||
| } as ConnectExternalAccountActivity; | ||
| const activityState = { | ||
| pointsAwarded: 0, | ||
| providersConnected: [] | ||
| }; | ||
| const newActivityState = await awardConnectExternalAccountActivityPoints(activity, activityState); | ||
| expect(newActivityState.pointsAwarded).toBe(0); | ||
| expect(newActivityState.providersConnected).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Enhance test coverage and improve test descriptions.
The test suite should include additional scenarios and clearer descriptions:
- Error handling for failed API calls
- Input validation for invalid activity parameters
- More descriptive test names that follow the given/when/then pattern
Here are the additional test cases to add:
it("should handle API errors gracefully when fetching external accounts fails", async () => {
jest.spyOn(MyDataHelps, 'getExternalAccounts').mockRejectedValueOnce(new Error('API Error'));
const activity = {
type: "connectExternalAccount",
points: 10,
providerCategories: ["Health Plan"]
} as ConnectExternalAccountActivity;
const activityState = {
pointsAwarded: 0,
providersConnected: []
};
await expect(awardConnectExternalAccountActivityPoints(activity, activityState))
.rejects.toThrow('API Error');
});
it("should validate activity parameters", async () => {
const activity = {
type: "connectExternalAccount",
points: -10, // Invalid points
providerCategories: [] // Empty categories
} as ConnectExternalAccountActivity;
const activityState = {
pointsAwarded: 0,
providersConnected: []
};
await expect(awardConnectExternalAccountActivityPoints(activity, activityState))
.rejects.toThrow('Invalid activity parameters');
});Also, rename existing tests to be more descriptive:
-it("should award points for connecting a single external account"
+it("given a Health Plan provider, when connected, then awards 10 points"
-it("should award points for connecting multiple devices"
+it("given multiple Device Manufacturer providers, when connected, then awards points for each"
-it("should append points and reset providers connected if not on file"
+it("given existing points and providers, when connecting new provider, then appends points and resets providers"
-it("should NOT award points when the ppt does not have this provider connected"
+it("given unknown provider category, when checking connections, then awards no points"
__tests__/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
__tests__/fixtures/participantInfo.ts (1)
17-25: Consider providing realistic test data for empty fields.Several demographic fields are initialized with empty strings. While this might work for basic tests, having realistic mock data helps catch edge cases and makes tests more robust.
Consider adding realistic mock data for fields like:
- dateOfBirth (e.g., "1990-01-01")
- gender (e.g., "F", "M", or other valid values)
- postalCode (e.g., "12345")
- timeZone (e.g., "America/New_York")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .npmignore (1 hunks)
- tests/fixtures/participantInfo.ts (1 hunks)
- tests/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1 hunks)
- jest.config.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- jest.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- .npmignore
- tests/helpers/BasicPointsAndBadges/CustomActivity.test.ts
🧰 Additional context used
.npmignore
Outdated
There was a problem hiding this comment.
I am not super familiar with this stuff, but I think the .npmignore file may not help us with this.
We already whitelist the dist directory as the only thing to include in the npm publish. We just need to make sure the dist folder doesn't contain the test files.
I think we can modify this line of the rollup config to be something like:
typescript({ tsconfig: "./tsconfig.json", exclude: ["**/__tests__", "**/*.test.ts"] }),
I haven't tested that, though, so we may need to iterate on it to get the correct exclusion patterns.
All that said, now that the tests are in their own directory outside of the source tree, maybe we don't even need to add an exclusion at all? That would probably be worth testing.
There was a problem hiding this comment.
Notes from yesterday's chat,
-changes in .npmignore will be rolled back. Most of what was updated here is already handled by the dist setting in package.json
-tests have been relocated external to src folder and are not packaged
__tests__/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts
Outdated
Show resolved
Hide resolved
__tests__/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts
Outdated
Show resolved
Hide resolved
src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx
Outdated
Show resolved
Hide resolved
src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts
Outdated
Show resolved
Hide resolved
src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
tsconfig.json (1)
Line range hint
1-19: Consider documenting configuration choices.To help future maintainers understand the purpose of each compiler option, consider adding comments explaining key decisions, especially for options critical to Jest and Storybook integration.
Example documentation:
{ "compilerOptions": { - // Default + // Base configuration "target": "es2015", "esModuleInterop": true, "forceConsistentCasingInFileNames": true, "strict": true, "skipLibCheck": true, - // Added + // React & Testing configuration + // Required for React JSX compilation "jsx": "react", + // Required for Jest module system compatibility "module": "ESNext", + // Enables Jest to process JavaScript files "allowJs": true, + // Enables debugging support "sourceMap": true, "moduleResolution": "node", "allowSyntheticDefaultImports": true, + // Prevents direct TS to JS compilation (handled by build tools) "noEmit": true } }src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx (2)
4-4: Remove unused importsThe
CardandLayoutcomponents are imported but not used in this file.-import { Card, Layout } from "../../presentational";
12-14: Add component documentationThe TODO comment indicates missing documentation. Since this component is part of a published npm package (as mentioned in PR objectives), proper documentation is essential.
Would you like me to help generate a JSDoc comment block for the component documentation? This could include:
- Component purpose and usage
- Props interface
- Example usage scenarios
src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx (1)
19-23: Consider enhancing the story with args and documentation.While the basic structure is correct, consider these enhancements:
- Add args/argTypes if the component accepts any props
- Include documentation about the component's purpose and usage
Example enhancement:
export const ConnectDeviceAccountStepContainerDefault : Story = { - render: render + render: render, + args: { + // Add default props here + }, + parameters: { + docs: { + description: { + story: 'This component handles...' + } + } + } };src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx (1)
28-103: Consider enhancing story documentation with descriptions.While the stories are well-structured and properly typed, consider adding descriptions to each story to explain the specific use case or scenario it represents. This would improve the documentation and make it more useful for other developers.
Example enhancement:
export const NotConnected: Story = { + parameters: { + docs: { + description: 'Displays the initial state when Fitbit is not connected.' + } + }, args: { previewState: "notConnected", title: "Fitbit" }, render: render };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
- package.json (3 hunks)
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx (5 hunks)
- src/components/container/ConnectEhr/ConnectEhr.stories.tsx (0 hunks)
- src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx (1 hunks)
- src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx (1 hunks)
- src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx (1 hunks)
- tsconfig.json (1 hunks)
💤 Files with no reviewable changes (1)
- src/components/container/ConnectEhr/ConnectEhr.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx
🧰 Additional context used
🔇 Additional comments (7)
tsconfig.json (1)
Line range hint
8-15: LGTM! These compiler options are necessary for the project updates.These TypeScript configuration changes are required to support:
- Jest integration (module: "ESNext", allowJs: true)
- Storybook 8.5 upgrade (jsx: "react")
- Improved debugging capabilities (sourceMap: true)
src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx (2)
1-3: LGTM! Import statements follow Storybook v8.5 conventions.The migration from ComponentStory/ComponentMeta to Meta/StoryObj types aligns with the Storybook upgrade objectives.
5-14: LGTM! Meta configuration follows Storybook v8.5 structure.The meta configuration is well-structured with appropriate title, component reference, and layout parameters.
src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx (4)
5-5: LGTM! Import statement follows Storybook v7+ conventions.The updated import statement correctly uses
MetaandStoryObjtypes from @storybook/react, aligning with modern Storybook practices.
20-26: LGTM! Well-structured render helper and type definition.The Story type definition and render helper function are well-implemented:
- Proper TypeScript typing using StoryObj
- Consistent layout wrapping for all stories
- Clean props spreading pattern
1-103: Verify Storybook 8.5 compatibility.While the code follows modern Storybook patterns, let's ensure it's fully compatible with Storybook 8.5 as mentioned in the PR objectives.
#!/bin/bash # Check package.json for Storybook version and related dependencies echo "Checking Storybook version and dependencies..." cat package.json | jq '.dependencies["@storybook/react"], .devDependencies["@storybook/react"]'
8-19: Address TODO comment in meta configuration.The meta configuration looks good, but there's a TODO comment that needs to be addressed. Consider adding a proper description for the component documentation.
Would you like me to help generate a component description based on the ConnectFitbit component's purpose and usage?
Let's verify if there are any existing component descriptions we can reference:
✅ Verification successful
Let me gather more information about the ConnectFitbit component to better understand its purpose and existing documentation.
Let me try a different approach to find the component implementation and documentation.
Based on the search results, I can now generate a final response since I have enough context about the ConnectFitbit component. The component is used for connecting Fitbit devices, is used in both HomeView and FitbitView, and has specific props for handling different preview states and connection behaviors.
Add component description to replace TODO comment
The meta configuration is correct, but needs a proper description. Based on the component's implementation and usage, here's what should replace the TODO:
const meta: Meta<typeof ConnectFitbit> = { title: "Container/ConnectFitbit", component: ConnectFitbit, parameters: { layout: 'fullscreen', docs: { description: { component: 'A container component that handles Fitbit device connection functionality. It provides a user interface for connecting/disconnecting Fitbit devices, displays connection status, and supports different preview states and disabled behaviors. Used in both home and device-specific views.' } } } };🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing descriptions or documentation of ConnectFitbit rg -l "ConnectFitbit" | xargs rg -l "description|Description"Length of output: 214
Script:
#!/bin/bash # Look for ConnectFitbit component implementation and any comments or documentation rg -A 5 "ConnectFitbit" --type tsx --type ts # Also check for any README or documentation files in the component directory fd -e md ConnectFitbitLength of output: 97
Script:
#!/bin/bash # Search for ConnectFitbit component implementation with ripgrep rg -A 10 "ConnectFitbit" # Find all files containing ConnectFitbit in their name fd ConnectFitbitLength of output: 9952
src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx
Show resolved
Hide resolved
src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx
Show resolved
Hide resolved
...ponents/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx
Outdated
Show resolved
Hide resolved
…t npmignore. Remove comments
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx (1)
6-15: Add component description contentWhile the Description component is properly configured, it appears to be empty. According to the PR objectives, this description should be included in the npm package documentation.
Add the description content to provide context about the component:
parameters: { layout: 'fullscreen', docs: { - Description: <Description /> + Description: ( + <Description markdown={` + The \`ConnectDeviceAccountStepContainer\` is a survey step component that facilitates connecting a participant's device account. + + This component renders a seamless interface within a survey for participants to link their device accounts, enabling data synchronization and enhancing user engagement. + + It ensures that participants can easily connect their device accounts during the survey flow, providing a straightforward step in the data collection process. + `}/> + ) } }src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.tsx (1)
7-11: Consider enhancing the JSDoc documentation.While the current documentation provides a good overview, it could be more comprehensive by including:
@paramdescriptions for expected configuration properties (title, text, deviceType, styles)@returnsdescription of the rendered output- List of supported device types (Fitbit, Omron, Garmin)
- Example usage within a survey context
Here's a suggested enhancement:
/** * This is a survey step component that facilitates connecting a participant's device account. * This component is intended to run within a survey for participants to link their device accounts, enabling data synchronization and enhancing user engagement. * It ensures that participants can easily connect their device accounts during the survey flow, providing a straightforward step in the data collection process. + * + * @remarks + * The component supports connecting to the following device types: + * - Fitbit + * - Omron + * - Garmin + * + * @example + * ```tsx + * // Example survey step configuration + * { + * type: "ConnectDeviceAccount", + * properties: { + * title: "Connect Your Device", + * text: "Please connect your fitness device to share your data.", + * deviceType: "Fitbit" + * } + * } + * ``` + * + * @returns A step component that handles device connection flow and automatically + * completes when a connection is established */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- tests/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (1 hunks)
- tests/helpers/BasicPointsAndBadges/CustomActivity.test.ts (1 hunks)
- jest.config.js (1 hunks)
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx (5 hunks)
- src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx (1 hunks)
- src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.tsx (1 hunks)
- src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/helpers/BasicPointsAndBadges/CustomActivity.test.ts
- jest.config.js
- src/components/container/BasicPointsForBadges/BasicPointsForBadges.stories.tsx
- src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts
🔇 Additional comments (4)
src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.stories.tsx (2)
1-4: LGTM: Imports updated to latest Storybook APIThe imports have been correctly updated to use the new Storybook v7+ API with proper type imports.
17-20: LGTM: Story type and render function properly definedThe Story type and render function are correctly implemented using the latest Storybook patterns.
src/components/step/ConnectDeviceAccountStepContainer/ConnectDeviceAccountStepContainer.tsx (1)
12-12: LGTM! Named export improves code clarity.The change from an anonymous function to a named export enhances code maintainability and debugging experience.
__tests__/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (1)
3-38:⚠️ Potential issueStreamline the mock implementation.
The mock setup can be improved in several ways:
- The mock data should be moved to a separate fixture file
- The
getCurrentLanguage,on, andoffmethods are unused and can be removed- The trailing comma in line 17 should be removed
Note: The suggestion to improve mock implementation efficiency and move test data to fixtures was previously made in a past review comment.
| export const ConnectDeviceAccountStepContainerDefault : Story = { | ||
| render: render | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more story variants
While the default story is correctly implemented, consider adding more variants to showcase different states or configurations of the component (e.g., loading state, error state, success state).
Example enhancement:
export const Loading: Story = {
render: () => <ConnectDeviceAccountStepContainer />,
parameters: {
mockData: {
loading: true
}
}
};
export const Error: Story = {
render: () => <ConnectDeviceAccountStepContainer />,
parameters: {
mockData: {
error: new Error('Failed to connect device')
}
}
};
__tests__/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| { | ||
| "id" : 3, | ||
| "provider": { "id": 11, "name": "FitBit", "category": "Device Manufacturer"} , |
There was a problem hiding this comment.
Maybe use an id of 33 here to avoid any potential collisions with the Garmin provider.
There was a problem hiding this comment.
Actually, I just realized this is a second Fitbit provider. Was that intentional? I don't think a user should be able to have two Fitbit connections, but if they did, the provider would have the same id (i.e. 22).
There was a problem hiding this comment.
I updated the 3rd external accounts provider id to reflect the desired mock data - multiple external accounts connected to the same providers (Fitbit/Garmin/Etc).
I have updated the provider id to match the fitbit provider
| providerCategories: ["Health Plan"] | ||
| } as ConnectExternalAccountActivity; | ||
| const activityState = { | ||
| pointsAwarded: 10, |
There was a problem hiding this comment.
Minor: Given the rest of the test setups, I think this would actually be 20 points here to start out if they already had two successful connections to providers 1 and 2.
There was a problem hiding this comment.
This demonstrates that there is no recalculating of points done based on the providers that had been connected in the past. The description has been updated to reflect the intent of the test
There was a problem hiding this comment.
Right, I think the description is fine. What I mean is, just to avoid confusion, the initial state here could be 20 points awarded, since they already have/had providers 1 and 2 connected. The results below would then be 30 and [1, 2, 44].
| @@ -13,6 +13,7 @@ | |||
| "allowJs": true, | |||
| "sourceMap": true, | |||
| "moduleResolution": "node", | |||
| "allowSyntheticDefaultImports": true | |||
| "allowSyntheticDefaultImports": true, | |||
| "noEmit": true | |||
There was a problem hiding this comment.
Is this because we are using rollup to build the modules?
There was a problem hiding this comment.
Correct -more info here
src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts
Outdated
Show resolved
Hide resolved
src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx (1)
12-14: Add component documentation.The PR objectives mention adding JSDoc comments for component documentation. Consider adding a description in the docs parameter:
parameters: { layout: 'fullscreen', docs: { + description: { + component: 'Component description here...' + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- tests/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts (1 hunks)
- src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx (1 hunks)
- src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx (1 hunks)
- src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts
- src/components/container/ConnectFitbit/ConnectFitbit.stories.tsx
- src/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.ts
🔇 Additional comments (1)
src/components/step/ConnectEhrStepContainer/ConnectEhrStepContainer.stories.tsx (1)
20-25: LGTM!The story definition follows the new Storybook 8.x patterns correctly.
| } from "./ConnectEhrStepContainer"; | ||
| import { Meta, StoryObj } from "@storybook/react"; | ||
| import ConnectEhrStepContainer from "./ConnectEhrStepContainer"; | ||
| import { Card, Layout } from "../../presentational"; |
There was a problem hiding this comment.
Remove unused imports.
The Card and Layout components are imported but not used in this story file.
-import { Card, Layout } from "../../presentational";📝 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.
| import { Card, Layout } from "../../presentational"; |
__tests__/helpers/BasicPointsAndBadges/ConnectExternalAccountActivity.test.ts
Outdated
Show resolved
Hide resolved
In short, this repo would benefit from more testing.
-Update storybook from 7.0 to 8.5
-Introduce Jest
-Add unit tests for Custom and ExternalAccount activities for use in BasicPointsForBadges
-Add jsdoc comment at the component level to BasicPointsForBadges
-Render the previously mentioned jsdoc comment as the description of the Default story for BasicPointsForBadges. These descriptions will export as part of the npm package
Security
REMINDER: All file contents are public.
Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.
Checklist
Testing
Documentation
Consider "Squash and merge" as needed to keep the commit history reasonable on
main.Reviewers
Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:
Summary by CodeRabbit
New Features
Introduction.mdxfile for better clarity.Bug Fixes
Documentation
Chores
package.jsonto enhance functionality and compatibility.amplify.ymlto include testing before building Storybook artifacts.