521 hint values arent being updated #522
Conversation
WalkthroughThis pull request introduces significant refactoring across backend migration scripts and frontend components, focusing on state management and database schema improvements. The changes primarily involve consolidating state in React components, updating migration scripts with enhanced error handling and type annotations, and modifying how hint-related properties are managed. The modifications aim to improve code organization and data integrity across the application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src/scenes/hints/CreateHintPage.jsx (3)
47-57: Clean up those commented state variables, fam! 🧹Remove the commented-out state variables since they've been replaced by the
leftContentobject.
59-79: Let's make those default values reusable! 🎮Extract the default values into a constant object for reusability and maintainability.
+const DEFAULT_LEFT_CONTENT = { + buttonRepetition: 'show only once', + url: 'https://', + actionButtonUrl: 'https://', + actionButtonText: 'Take me to subscription page', + action: 'No action', + targetElement: '.element', + tooltipPlacement: 'Top', + isHintIconVisible: true, +}; -const [leftContent, setLeftContent] = useState({ - buttonRepetition: 'show only once', - url: 'https://', - actionButtonUrl: 'https://', - actionButtonText: 'Take me to subscription page', - action: 'No action', - targetElement: '.element', - tooltipPlacement: 'Top', - isHintIconVisible: true, -}); +const [leftContent, setLeftContent] = useState(DEFAULT_LEFT_CONTENT);
Line range hint
91-117: Error handling could use more specifics! 🎯Consider providing more specific error messages for different failure scenarios.
} catch (error) { - emitToastError(error); + const errorMessage = error.response?.data?.message + ? `Failed to fetch hint data: ${error.response.data.message}` + : 'Failed to fetch hint data. Please try again.'; + emitToastError(errorMessage); }frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx (2)
210-223: Prop types could be more strict! 🔒Consider adding:
- Required flags
- Specific string literals for enums
- Default props
HintLeftContent.propTypes = { data: PropTypes.shape({ - buttonRepetition: PropTypes.string, + buttonRepetition: PropTypes.oneOf(['show only once', 'show every visit']).isRequired, - action: PropTypes.string, + action: PropTypes.oneOf(['No action', 'Open URL', 'Open URL in a new tab']).isRequired, - tooltipPlacement: PropTypes.string, + tooltipPlacement: PropTypes.oneOf(['Top', 'Right', 'Bottom', 'Left']).isRequired, // ... other props - }), + }).isRequired, - setState: PropTypes.func, + setState: PropTypes.func.isRequired, onSave: PropTypes.func, };
41-45: Form validation could be more robust! 💪Consider enabling validation on blur for better user experience.
<Formik initialValues={data} validationSchema={newHintSchema} enableReinitialize={true} validateOnMount={false} - validateOnBlur={false} + validateOnBlur={true} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/migrations/0006-1.0-hint.js(4 hunks)backend/migrations/0009-1.0-guidelog.js(2 hunks)frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx(9 hunks)frontend/src/scenes/hints/CreateHintPage.jsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (4)
backend/migrations/0009-1.0-guidelog.js (2)
6-47: Yo, this table creation is straight fire! 🔥Clean implementation with proper:
- Type annotations
- Transaction handling
- Column constraints
- Nullable flags
Line range hint
66-88: Transaction handling is on point! 💯Solid implementation with:
- Proper commit on success
- Rollback on failure
- Error propagation
backend/migrations/0006-1.0-hint.js (2)
Line range hint
6-96: Table creation is looking clean! 🧼Solid implementation with:
- Type annotations
- Proper foreign key references
- Appropriate column constraints
Line range hint
97-130: Heads up about case sensitivity in ENUM types! 🎯The ENUM types are case-sensitive, which could cause issues when comparing values. Consider normalizing case in the application layer.
Run this script to check for case mismatches in existing data:
Describe your changes
Changed migrations to prevent breaking the flow.
Changed behaviour of Formik to update the fields when data is updated
Issue number
#521
Please ensure all items are checked off before requesting a review: