501 add repetition to hints #516
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly Related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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
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: 4
🔭 Outside diff range comments (1)
backend/migrations/0006-1.0-hint.js (1)
Line range hint
3-3: Yo, we got some dead weight here! 🏋️♂️The
urlimport from 'inspector' is not used in this file.- const { url } = require("inspector");
🧹 Nitpick comments (4)
backend/src/utils/hint.helper.js (1)
6-6: Yo, we need to keep it DRY! 🍝The validation looks good, but the repetition options are duplicated across multiple files (model, helper, migration). Let's centralize these values in a shared constants file to maintain consistency.
Consider creating a shared constants file:
+ // backend/src/constants/hint.js + const HINT_CONSTANTS = { + REPETITION_TYPES: { + SHOW_ONCE: 'show only once', + SHOW_EVERY_VISIT: 'show every visit' + } + }; + + module.exports = HINT_CONSTANTS;Then import and use it across all files:
- const validRepetitionOptions = ['show only once', 'show every visit']; + const { HINT_CONSTANTS } = require('../constants/hint'); + const validRepetitionOptions = Object.values(HINT_CONSTANTS.REPETITION_TYPES);Also applies to: 25-32
frontend/src/scenes/hints/CreateHintPage.jsx (1)
94-94: Consider aligning state variable name with API propertyThe state variable
buttonRepetitionis saved asrepetitionTypein the API. Consider renaming the state variable to match the API property name for better consistency.-const [buttonRepetition, setButtonRepetition] = useState('Show only once'); +const [repetitionType, setRepetitionType] = useState('Show only once');frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx (2)
27-29: Add validation to the repetition handlerConsider adding validation to ensure only valid repetition types are set.
const handleRepetitionChange = (newRepetitionType) => { + const validTypes = ['Show only once', 'Show every visit']; + if (!validTypes.includes(newRepetitionType)) { + console.warn(`Invalid repetition type: ${newRepetitionType}`); + return; + } setButtonRepetition(newRepetitionType); };
90-94: Extract repetition options to constantsConsider moving the repetition options to a constants file for better maintainability and reuse.
+const REPETITION_OPTIONS = ['Show only once', 'Show every visit']; + <DropdownList - actions={['Show only once', 'Show every visit']} + actions={REPETITION_OPTIONS} onActionChange={handleRepetitionChange} selectedActionString={buttonRepetition} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/migrations/0006-1.0-hint.js(1 hunks)backend/src/models/Hint.js(1 hunks)backend/src/test/e2e/hint.test.mjs(1 hunks)backend/src/test/mocks/hint.mock.js(2 hunks)backend/src/test/unit/controllers/auth.test.js(1 hunks)backend/src/utils/hint.helper.js(2 hunks)frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx(4 hunks)frontend/src/scenes/hints/CreateHintPage.jsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/test/unit/controllers/auth.test.js
🔇 Additional comments (4)
backend/src/test/mocks/hint.mock.js (1)
6-6: Yo dawg, these changes are straight fire! 🔥The mock implementation is solid with proper test coverage for both missing and invalid repetition types. The default value matches the database migration.
Also applies to: 37-45
backend/migrations/0006-1.0-hint.js (1)
18-22: Clean migration, fam! 💯The repetitionType column is properly defined with appropriate constraints and default value.
frontend/src/scenes/hints/CreateHintPage.jsx (2)
47-48: Yo, the repetition state management is on point!The initialization and data fetching logic for
buttonRepetitionis well-implemented with proper default values and fallback handling.Also applies to: 75-75
169-170: Props passing looks clean!The repetition state and its setter are properly passed to the HintLeftContent component.
frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx
Outdated
Show resolved
Hide resolved
frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx
Outdated
Show resolved
Hide resolved
MandeepPaul
force-pushed
the
501-add-repetition-to-hints
branch
from
eb1db69 to
641870b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/scenes/hints/CreateHintPage.jsx (1)
Line range hint
94-109: There's vomit on his sweater already - we're missing validation!The hintData construction needs validation before sending to the API, eh? Let's add some error checking.
+ import { hint as hintSettings } from '../../../backend/config/settings'; const onSave = async () => { + const repetitionType = buttonRepetition.toLowerCase(); + if (!hintSettings.repetition.includes(repetitionType)) { + toastEmitter.emit( + TOAST_EMITTER_KEY, + `Error: Invalid repetition type. Allowed values are: ${hintSettings.repetition.join(', ')}` + ); + return; + } const hintData = { - repetitionType: buttonRepetition.toLowerCase(), + repetitionType, tooltipPlacement: tooltipPlacement.toLowerCase(), // ... rest of the properties };
🧹 Nitpick comments (2)
backend/config/settings.js (1)
46-46: Yo, the repetition values look solid, but let's keep our consistency game strong!The new repetition values align well with the existing popup and banner configurations, eh? However, we should consider extracting these common repetition values into a shared constant to maintain consistency across different components.
+ const commonRepetition = ['show only once', 'show every visit']; module.exports = { // ... other config hint: { action: ['no action', 'open url', 'open url in a new tab'], - repetition: ['show only once', 'show every visit'], + repetition: [...commonRepetition], }, popup: { action: ['no action', 'open url', 'open url in a new tab'], - repetition: ['show only once', 'show every visit'], + repetition: [...commonRepetition], }, banner: { - repetition: ['show only once', 'show every visit'], + repetition: [...commonRepetition], }, };frontend/src/scenes/hints/CreateHintPage.jsx (1)
169-170: Lose yourself in the documentation - we need prop types!The new props need proper TypeScript/PropTypes definitions to prevent future bugs.
HintLeftContent.propTypes = { buttonRepetition: PropTypes.string.isRequired, setButtonRepetition: PropTypes.func.isRequired, // ... other prop types };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/config/settings.js(1 hunks)backend/migrations/0006-1.0-hint.js(1 hunks)backend/src/models/Hint.js(1 hunks)backend/src/test/e2e/hint.test.mjs(2 hunks)backend/src/test/mocks/hint.mock.js(2 hunks)backend/src/utils/hint.helper.js(1 hunks)frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx(4 hunks)frontend/src/scenes/hints/CreateHintPage.jsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/src/utils/hint.helper.js
- backend/src/models/Hint.js
- backend/src/test/e2e/hint.test.mjs
- backend/src/test/mocks/hint.mock.js
- backend/migrations/0006-1.0-hint.js
- frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
frontend/src/scenes/hints/CreateHintPage.jsx (1)
67-75: Mom's spaghetti moment - we need better error handling here!The data fetching logic needs to validate the repetitionType against allowed values from settings.js before setting the state.
Describe your changes
Adde repetition field to hints. Updated tests, hint modal and migration.
Issue number
#501
Please ensure all items are checked off before requesting a review: