User stats page #552
User stats page #552
Conversation
erenfn
requested review from
swoopertr,
thomastepi,
bolatein,
DeboraSerra and
gorkem-bwl
WalkthroughThis pull request updates the tour configuration and related functionalities across both backend and frontend. In the backend, unused properties in the tour object are removed from the settings, and a new property Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Tour API Endpoint
participant Controller as TourController
participant Service as TourService
participant DB as Database
Client->>API: POST /api/tour/add_tour
API->>Controller: createTour(req)
Controller->>Service: createTour(data)
Service->>DB: Begin Transaction
Service->>DB: Insert Tour record
Service->>DB: Bulk create TourPopup entries
DB-->>Service: Transaction Commit
Service-->>Controller: Tour Created
Controller-->>Client: Success Response
sequenceDiagram
participant Browser
participant Frontend as Frontend Router
participant Statistics as UserStatisticsPage
participant Service as guidelogService
participant API as Backend API
Browser->>Frontend: Navigate to /statistics
Frontend->>Statistics: Render UserStatisticsPage
Statistics->>Service: getAllGuideLogs()
Service->>API: GET /guide_log/all_guide_logs
API-->>Service: Guide Logs Data
Service-->>Statistics: Logs Data
Statistics-->>Browser: Render CustomTable with Logs
Possibly related PRs
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 17
🔭 Outside diff range comments (8)
frontend/src/utils/guideHelper.js (1)
4-11: 🛠️ Refactor suggestionMom's spaghetti moment: Let's make this error handling more robust!
The current implementation could throw if
errorsarray is undefined.Here's a more defensive implementation:
export const emitToastError = (error) => { - if (error.response?.data) { + if (error.response?.data?.errors?.length) { for (let i = 0; i < error.response.data.errors.length; i++){ toastEmitter.emit(TOAST_EMITTER_KEY, 'An error occurred: ' + error.response.data.errors[i].msg) } } else { toastEmitter.emit(TOAST_EMITTER_KEY, 'An error occurred. Please check your network connection and try again.') } }frontend/src/scenes/links/NewLinksPopup.jsx (1)
171-176:⚠️ Potential issueAdd missing prop type validations.
The
setIsEditprop is used but not included in the PropTypes validation.NewLinksPopup.propTypes = { autoOpen: PropTypes.bool, isEdit: PropTypes.bool.isRequired, itemId: PropTypes.number, setItemsUpdated: PropTypes.func.isRequired, + setIsEdit: PropTypes.func.isRequired, };frontend/src/scenes/links/LinkPageComponents/LinkAppearance.jsx (1)
178-180:⚠️ Potential issueRemove unused PropTypes declaration.
The
handleSaveHelperprop is declared in PropTypes but not used in the component.-LinkAppearance.propTypes = { - handleSaveHelper: PropTypes.func.isRequired, -};frontend/src/scenes/popup/CreatePopupPage.jsx (1)
81-84:⚠️ Potential issueImprove error handling in fetchPopupData.
The error handling should be more specific and avoid console.log in production code.
} catch (error) { - console.log({ error }); - emitToastError(error); + const errorMessage = error.response?.data?.message || 'Failed to fetch popup data'; + emitToastError(new Error(errorMessage)); }frontend/src/components/Links/Settings/Settings.jsx (2)
21-22: 🛠️ Refactor suggestionYo dawg, we need to clean up this state management!
The component is maintaining duplicate state with both Formik (
values) and local state (state). This can lead to synchronization issues and bugs.Consider using only Formik's state management:
-const [state, setState] = useState(linkToEdit ?? defaultState); +const formikRef = useRef();
35-52:⚠️ Potential issueMom's spaghetti alert: Memory leak in useEffect!
The cleanup function doesn't handle all subscribed events and state updates.
Add cleanup for state updates:
useEffect(() => { document.querySelector('#title').focus(); if (linkToEdit) { const newState = { ...linkToEdit, helperId: helper.id, }; setState(newState); } else { setState({ ...defaultState, id: `${Date.now()}-${Math.random().toString(36).substring(2, 11)}`, }); } window.addEventListener('mousedown', handleMouseDown); - return () => window.removeEventListener('mousedown', handleMouseDown); + return () => { + window.removeEventListener('mousedown', handleMouseDown); + setState(defaultState); // Reset state on unmount + }; }, []);backend/src/test/unit/services/helperLink.test.js (1)
94-107: 🛠️ Refactor suggestionTransaction rollback test's got me nervous!
The error test case doesn't verify the complete rollback scenario.
Add more comprehensive transaction tests:
it('should rollback all changes if link creation fails', async () => { HelperLinkMock.create = sinon.stub(HelperLink, 'create').resolves(); LinkMock.bulkCreate = sinon.stub(Link, 'bulkCreate').rejects(); try { await helperLinkService.createHelper( mocks.HelperLinkBuilder.helperLink(1).build(), mocks.LinksList ); } catch (e) { expect(e.message).to.equal('Error creating helper'); } expect(commit.called).to.be.false; expect(rollback.called).to.be.true; expect(HelperLinkMock.create.called).to.be.true; });frontend/src/tests/scenes/links/NewLinksPopup.test.jsx (1)
131-166: 🛠️ Refactor suggestionThis skipped test's got me losing myself!
The drag-and-drop test is skipped. Either fix and enable it or remove it if it's no longer relevant.
Consider using a more reliable approach for testing drag-and-drop:
import { DndProvider } from 'react-dnd'; import { HTML5Backend } from 'react-dnd-html5-backend'; // Wrap your component with DndProvider in the test const renderWithDnd = () => { return render( <DndProvider backend={HTML5Backend}> <NewLinksPopup /> </DndProvider> ); };
🧹 Nitpick comments (42)
frontend/src/utils/guideHelper.js (1)
3-11: One shot, one opportunity: Let's modernize this code!The function could benefit from modern JavaScript features.
Here's a more concise implementation using array methods:
export const emitToastError = (error) => { - if (error.response?.data) { - for (let i = 0; i < error.response.data.errors.length; i++){ - toastEmitter.emit(TOAST_EMITTER_KEY, 'An error occurred: ' + error.response.data.errors[i].msg) - } - } else { - toastEmitter.emit(TOAST_EMITTER_KEY, 'An error occurred. Please check your network connection and try again.') - } + const errors = error.response?.data?.errors || []; + if (errors.length) { + errors.forEach(({ msg }) => + toastEmitter.emit(TOAST_EMITTER_KEY, `An error occurred: ${msg}`)); + } else { + toastEmitter.emit(TOAST_EMITTER_KEY, + 'An error occurred. Please check your network connection and try again.'); + } }backend/src/controllers/tour.controller.js (2)
45-54: Validate the URL parameter.
The method is searching tours byurl. If theurlmight be invalid or not well-formed, consider adding validation or normalizing it.
67-85: User ownership caution.
We removeuserIdfrom the request body but do not verify the requesting user has permission to modify this tour. Pulled a sly “knees weak” move on the data. If needed, add an ownership check or role-based validation.backend/src/service/tour.service.js (4)
16-22: Optimal indexing.
If the table grows large, consider indexingcreatedByfor quick queries. That’s a detail for a separate DB-level optimization, though.
24-41: Graceful error handling.
We log the error and throw a new one. This is fine, though structured logging might help debug further. Let’s keep the code as stable as a balanced rap.
83-108: In-place update approach is good.
We remove old popups then create the new ones. This ensures consistency, though partial updates might be lost if you only intended to update certain fields. If the plan is always to replace them, all good—just a housekeeping note so you don’t get spaghetti sauce all over.
110-124: Check relationships with foreign-key constraints.
We’re destroying associatedTourPopupentries before the mainTour. This approach is fine. Just ensure no references remain, or that no other tables reference these popups.frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.jsx (2)
7-7: Validation schema is neat.
EnsuringapperanceSchemais accurate will avoid unexpected data. Double-check spelling: “appearance” might be spelled differently, but that’s minor.
21-37: Consider passing form errors to onSave.
Right now, if there are form errors, we rely on the validation schema to block submission. That’s fine, but you could also show specific error messages if needed as part ofonSave. Not mandatory, though.backend/src/test/mocks/tour.mock.js (2)
67-80: New properties, no sweat.
Your introduction ofheaderColor,textColor, and buddies is reminiscent of unstoppable bars—just ensure types align so we don’t drop our spaghetti on the floor. Consider additional constraints if necessary.
91-109: Color validations ain't messing around.
The methods to mark header colours, text colours, and button backgrounds as invalid are tight. If you want next-level dryness, you could unify these into a single color validator method and polish that flow.frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (2)
50-56:handleDropdownChangeis slick.
Nice synergy withsetFieldValueandsetPopupContent. This approach helps keep the left and right channels in sync, so you don’t lose your mind in the rap flow.
83-144: Text fields: watch for extra sauce.
Your usage of Formik’shandleChangeplus a parent-levelsetPopupContentensures real-time state sync. If your arms get heavy from more fields, consider a shared helper to handle repeated logic.backend/src/test/e2e/tour.test.mjs (2)
47-47: Database Readiness Logging
Catching the DB readiness error and logging it is good. Keeping everything visible is wise—no hidden nerves to blow your performance.Also applies to: 50-50
82-91: Misspelled Test Name
Notice the test name says"buttonBackgroundColorundColor". It should probably say"buttonBackgroundColor"to keep your code from stumbling over its own lines.-it('should return 400 if "buttonBackgroundColorundColor" is not a valid hex color', ... +it('should return 400 if "buttonBackgroundColor" is not a valid hex color', ...backend/src/models/TourPopup.js (1)
32-39: Index missing on tourId - performance gonna drop like mom's spaghetti! 🍝Add an index to speed up those foreign key lookups.
tourId: { type: DataTypes.INTEGER, allowNull: false, references: { model: 'tours', key: 'id', }, + index: true },backend/migrations/0011-1.1-helperlink.js (2)
1-1: Lose yourself in the code, but drop that 'use strict'! 🎤The 'use strict' directive is redundant in modules. Let's drop it like it's hot!
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
13-51: This transaction handling is the real slim shady! 🎵Clean transaction management with proper rollback on errors. But yo, we might want to log those errors before they vanish into thin air!
- } catch { + } catch (error) { + console.error('Migration failed:', error); await transaction.rollback(); + throw error; }backend/migrations/0013-1.1-tour-popup.js (2)
1-1: Drop that 'use strict' like a hot beat! 🎵Same deal as before, the 'use strict' directive isn't needed in modules.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
13-45: This table structure is spitting straight fire! 🔥The schema is tight with proper constraints and that sweet CASCADE action on the foreign key. But yo, consider adding some timestamps for tracking!
tourId: { // ... existing config ... }, + createdAt: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.literal('CURRENT_TIMESTAMP'), + }, + updatedAt: { + type: Sequelize.DATE, + allowNull: false, + defaultValue: Sequelize.literal('CURRENT_TIMESTAMP'), + },frontend/src/utils/popupHelper.js (1)
3-4: Extract regex patterns into named constantsConsider extracting these regex patterns into well-named constants at the top of the file for better maintainability.
+const VALID_PATH_CHARS = '[a-zA-Z0-9_-]'; +const RELATIVE_URL_REGEX = new RegExp(`^\\/(${VALID_PATH_CHARS}+\\/?)+$`); +const HEX_COLOR_REGEX = /^#[0-9A-Fa-f]{6}$/; -const RELATIVE_URL_REGEX = /^\/([a-zA-Z0-9_-]+\/?)+$/; -const COLOR_REGEX = /^#[0-9A-Fa-f]{6}$/;backend/src/utils/tour.helper.js (1)
19-36: Add missing validations for steps arrayThe steps array validation could be more robust:
body('steps').isArray().withMessage('steps must be an array'), +body('steps').custom((value) => { + if (!Array.isArray(value) || value.length === 0) { + throw new Error('At least one step is required'); + } + return true; +}), body('steps.*.title').isString().withMessage('Invalid value for title'), +body('steps.*.title').notEmpty().withMessage('Title cannot be empty'), body('steps.*.description').isString().withMessage('Invalid value for description'), body('steps.*.targetElement').isString().withMessage('Invalid value for targetElement'), -body('steps.*.order').isInt().withMessage('Invalid value for order'), +body('steps.*.order').isInt({ min: 0 }).withMessage('Order must be a non-negative integer'),frontend/src/components/Table/Table.jsx (1)
26-30: Enhance table accessibilityAdd ARIA labels and roles to improve accessibility:
-<TableRow className={styles.tableHeader}> +<TableRow className={styles.tableHeader} role="row"> {tableHeaders.map((header, index) => ( - <TableCell key={index} className={styles.heading}>{header}</TableCell> + <TableCell + key={index} + className={styles.heading} + role="columnheader" + aria-label={header} + > + {header} + </TableCell> ))} </TableRow>backend/src/models/HelperLink.js (1)
64-73: Yo! The URL validation needs some error handling love!The URL validation is missing proper error messaging. Let's make it more informative.
- validate: { - customValidation(value) { - return validateUrl(value); - }, - }, + validate: { + customValidation(value) { + if (!validateUrl(value)) { + throw new Error('Invalid URL format. Please provide a valid URL.'); + } + }, + },frontend/src/services/linksProvider.jsx (1)
5-13: Yo! These default values are looking clean!The default values are well-structured and match the backend model. However, consider making this more maintainable.
+const COLORS = { + HEADER_BG: '#F8F9F8', + LINK_FONT: '#344054', + ICON: '#7F56D9', +}; export const DEFAULT_VALUES = { title: '', - headerBackgroundColor: '#F8F9F8', - linkFontColor: '#344054', - iconColor: '#7F56D9', + headerBackgroundColor: COLORS.HEADER_BG, + linkFontColor: COLORS.LINK_FONT, + iconColor: COLORS.ICON, url: 'https://', active: true, absolutePath: false, };backend/src/models/Tour.js (1)
69-73: The URL validation needs more swagger!The error handling could be more descriptive for better debugging.
customValidation(value) { if (!validateUrl(value)) { - throw new Error('Invalid value for URL'); + throw new Error(`Invalid URL format: ${value}. Please provide a valid URL starting with http:// or https://`); } },frontend/src/services/helperLinkService.js (1)
58-59: Clean up those console logs, yo!There are duplicate console log statements. Let's keep it clean and consistent.
Apply this diff to remove the duplicate:
- console.log(error.message); console.error('Create helper link error:', error.message);backend/src/controllers/guide.controller.js (1)
54-54: Clean up that commented code, keep it fresh!Remove or implement the commented code. Keeping commented code makes the codebase messy and confusing.
Either remove the commented lines or implement the intended functionality:
- //const helperLinkIds = completeHelperLogs.map(log => log.guideId); - //helperLinkService.getIncompleteHelpers(helperLinkIds),Also applies to: 61-61
frontend/src/products/Popup/PopupComponent.jsx (1)
43-49: Yo, these string literals are making me nervous! 🍝Consider extracting the button action types into constants to prevent typos and improve maintainability.
+const BUTTON_ACTIONS = { + CLOSE: 'close the popup', + OPEN_URL: 'open url', + OPEN_URL_NEW: 'open url in new page' +}; const handleButtonClick = () => { - if (buttonAction === 'close the popup') { + if (buttonAction === BUTTON_ACTIONS.CLOSE) { handleClose(); - } else if (buttonAction === 'open url') { + } else if (buttonAction === BUTTON_ACTIONS.OPEN_URL) { window.open(actionButtonUrl, '_self'); - } else if (buttonAction === 'open url in new page') { + } else if (buttonAction === BUTTON_ACTIONS.OPEN_URL_NEW) { window.open(actionButtonUrl, '_blank'); } };backend/migrations/0012-1.1-tour.js (2)
1-3: Drop that 'use strict' like it's hot! 🔥The 'use strict' directive is redundant in ES modules as they are automatically in strict mode.
-'use strict'; const { validateHexColor } = require('../src/utils/guide.helper');🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
109-110: Yo, that comment's not quite right! 🤔The comment mentions dropping 'guide_logs' table but the code drops 'tours' table.
- // Drop the guide_logs table + // Drop the tours table await queryInterface.dropTable(TABLE_NAME, { transaction });backend/migrations/0008-1.0-popup.js (2)
87-88: Consider adding validation before removing columns.The migration removes columns without first checking if they exist. This could cause issues if the migration is run on a fresh database or if the columns were already removed.
- await queryInterface.removeColumn(TABLE_NAME, 'closeButtonAction', { transaction }); + if (await queryInterface.tableExists(TABLE_NAME)) { + const columns = await queryInterface.describeTable(TABLE_NAME); + if (columns.closeButtonAction) { + await queryInterface.removeColumn(TABLE_NAME, 'closeButtonAction', { transaction }); + } + }Also applies to: 101-102, 114-115
127-136: Add error handling for bulk update operation.The bulk update operation could fail silently if any of the updates fail. Consider adding specific error handling for this operation.
if (allPopups.length > 0) { + try { const updates = allPopups.map((val) => ({ id: val.id, closeButtonAction: val.closeButtonAction, popupSize: val.popupSize, repetitionType: val.repetitionType, })); await queryInterface.bulkUpdate(TABLE_NAME, updates, null, { transaction }); + } catch (error) { + console.error('Failed to update existing popups:', error); + throw error; + } }frontend/src/scenes/links/NewLinksPopup.jsx (1)
56-59: Improve error handling in fetchHelperData.The error handling could be more specific by checking for different types of errors (network, validation, etc.).
} catch (error) { + if (error instanceof AxiosError) { + const message = error.response?.data?.errors?.[0] || 'Failed to fetch helper data'; + emitToastError(buildToastError(message)); + } else { emitToastError(buildToastError(error)); + } resetHelper(); }frontend/src/scenes/links/LinkPageComponents/LinkAppearance.jsx (1)
22-32: Simplify handleHelperChange logic.The current implementation has multiple conditions that could be simplified.
const handleHelperChange = (e) => { - const { name, checked } = e.target; - let { value } = e.target; - if (name === 'active') { - value = checked; - } - if (name === 'absolutePath') { - value = !checked; - } + const { name, checked, value: inputValue } = e.target; + const value = name === 'active' ? checked : name === 'absolutePath' ? !checked : inputValue; setHelper((prev) => ({ ...prev, [name]: value })); };frontend/src/scenes/popup/CreatePopupPage.jsx (1)
31-38: Extract default values to constants.The default values for popup appearance and content should be extracted to constants for better maintainability.
+const DEFAULT_POPUP_APPEARANCE = { + headerBackgroundColor: '#F8F9F8', + headerColor: '#101828', + textColor: '#344054', + buttonBackgroundColor: '#7F56D9', + buttonTextColor: '#FFFFFF', + popupSize: 'Small', +}; + +const DEFAULT_POPUP_CONTENT = { + buttonRepetition: 'Show only once', + buttonAction: 'No action', + url: 'https://', + actionButtonUrl: 'https://', + actionButtonText: 'Take me to subscription page', +}; - const [popupAppearance, setPopupAppearance] = useState({ - headerBackgroundColor: '#F8F9F8', - headerColor: '#101828', - textColor: '#344054', - buttonBackgroundColor: '#7F56D9', - buttonTextColor: '#FFFFFF', - popupSize: 'Small', - }); + const [popupAppearance, setPopupAppearance] = useState(DEFAULT_POPUP_APPEARANCE); - const [popupContent, setPopupContent] = useState({ - buttonRepetition: 'Show only once', - buttonAction: 'No action', - url: 'https://', - actionButtonUrl: 'https://', - actionButtonText: 'Take me to subscription page', - }); + const [popupContent, setPopupContent] = useState(DEFAULT_POPUP_CONTENT);Also applies to: 40-46
frontend/src/components/Links/Settings/Settings.jsx (1)
227-233: Hidden submit button's making me nervous!The hidden submit button with display: none is not accessible and might cause issues with keyboard navigation.
Consider using a visually hidden class instead:
-<button type="submit" style={{ display: 'none' }}> +<button type="submit" className={style.visuallyHidden}> {isSubmitting ? ( <CircularProgress size={12} color="inherit" /> ) : ( 'Submit' )} </button>Add this to your CSS:
.visuallyHidden { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0, 0, 0, 0); border: 0; }backend/src/test/unit/controllers/tour.test.js (2)
21-29: Yo, these test descriptions ain't telling the whole story!The test descriptions should be more descriptive and follow the given-when-then pattern.
Refactor the test description:
-it('should return 201 if all required fields are provided', async () => { +it('should return 201 and created tour when creating a tour with valid fields', async () => {
50-58: These error messages got me sweating!The error message structure is inconsistent across different endpoints.
Standardize error message format:
-expect(body).to.be.deep.equal({ errors: [{"msg": "Tour with the specified id does not exist"}] }); +expect(body).to.be.deep.equal({ + error: 'Not Found', + errorCode: 'TOUR_NOT_FOUND', + message: 'Tour with the specified id does not exist' +});backend/src/test/unit/services/helperLink.test.js (1)
31-44: Yo, why these test assertions taking a vacation?There are commented-out test assertions that should either be removed or uncommented.
Either remove the commented code or uncomment and fix the assertions if they're still relevant.
frontend/src/tests/scenes/links/NewLinksPopup.test.jsx (2)
199-201: These hardcoded colors making my palms sweaty!The test uses hardcoded RGB values for color assertions, which makes the tests brittle.
Use color constants or helper functions:
const COLORS = { DEFAULT_HEADER_BG: 'rgb(248, 249, 248)', MODIFIED_HEADER_BG: 'rgb(242, 242, 242)' }; expect(getComputedStyle(previewHeader).backgroundColor).toBe(COLORS.DEFAULT_HEADER_BG);Also applies to: 212-214
58-77: Missing accessibility tests got me nervous!The test suite lacks accessibility testing for form interactions.
Add accessibility tests:
it('should be keyboard navigable', async () => { await renderPopup(); const addNewLinkBtn = screen.getByText('+ Add new link'); // Test keyboard navigation await userEvent.tab(); expect(addNewLinkBtn).toHaveFocus(); // Test form submission via keyboard await userEvent.keyboard('{Enter}'); expect(screen.getByTestId('settings-form')).toBeInTheDocument(); });
🛑 Comments failed to post (17)
frontend/src/scenes/statistics/UserStatisticsPage.jsx (2)
21-25: 🛠️ Refactor suggestion
Mom's spaghetti time - let's beef up that render logic!
The component's looking weak without proper loading and error states in the UI.
return ( <div className={styles.container}> + {isLoading && <div>Loading...</div>} + {error && <div className={styles.error}>{error}</div>} + {!isLoading && !error && ( <CustomTable fullData={guideLogs} /> + )} </div> );📝 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.return ( <div className={styles.container}> {isLoading && <div>Loading...</div>} {error && <div className={styles.error}>{error}</div>} {!isLoading && !error && ( <CustomTable fullData={guideLogs} /> )} </div> );
6-19: 🛠️ Refactor suggestion
Yo dawg, we need to handle that loading state!
The component's got mad anxiety right now - no loading state, just raw nerves. Let's add some state management to keep our users in the loop.
const UserStatisticsPage = () => { const [guideLogs, setGuideLogs] = useState([]); + const [isLoading, setIsLoading] = useState(true); + const [error, setError] = useState(null); useEffect(() => { const fetchGuideLogs = async () => { try { + setIsLoading(true); const guideLogsData = await getAllGuideLogs(); setGuideLogs(guideLogsData); } catch (err) { console.error("Error fetching guide logs:", err); + setError("Failed to load guide logs"); + } finally { + setIsLoading(false); } }; fetchGuideLogs(); }, []);📝 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 UserStatisticsPage = () => { const [guideLogs, setGuideLogs] = useState([]); const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState(null); useEffect(() => { const fetchGuideLogs = async () => { try { setIsLoading(true); const guideLogsData = await getAllGuideLogs(); setGuideLogs(guideLogsData); } catch (err) { console.error("Error fetching guide logs:", err); setError("Failed to load guide logs"); } finally { setIsLoading(false); } }; fetchGuideLogs(); }, []);backend/src/models/TourPopup.js (1)
28-31: 🛠️ Refactor suggestion
Yo, that order field needs some boundaries, fam!
The order field's running wild like loose spaghetti - let's add some validation to keep it in check.
order: { type: DataTypes.INTEGER, allowNull: false, + validate: { + min: 0, + isInt: true + } },📝 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.order: { type: DataTypes.INTEGER, allowNull: false, validate: { min: 0, isInt: true } },frontend/src/utils/popupHelper.js (1)
6-13:
⚠️ Potential issueStrengthen URL validation to prevent security vulnerabilities
The current URL validation is too permissive and could allow malicious URLs. Consider adding additional checks:
const validateUrl = (url) => { try { - new URL(url); + const urlObj = new URL(url); + // Whitelist allowed protocols + if (!['http:', 'https:'].includes(urlObj.protocol)) { + return false; + } + // Optional: Whitelist allowed domains + const allowedDomains = ['yourdomain.com', 'api.yourdomain.com']; + if (!allowedDomains.includes(urlObj.hostname)) { + return false; + } return true; } catch { return RELATIVE_URL_REGEX.test(url); } };📝 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 validateUrl = (url) => { try { const urlObj = new URL(url); // Whitelist allowed protocols if (!['http:', 'https:'].includes(urlObj.protocol)) { return false; } // Optional: Whitelist allowed domains const allowedDomains = ['yourdomain.com', 'api.yourdomain.com']; if (!allowedDomains.includes(urlObj.hostname)) { return false; } return true; } catch { return RELATIVE_URL_REGEX.test(url); } };backend/src/utils/tour.helper.js (1)
5-8: 🛠️ Refactor suggestion
Consolidate URL validation patterns
The URL regex patterns should be shared between frontend and backend to maintain consistency.
Consider creating a shared validation package or config that both frontend and backend can import.
frontend/src/components/Table/Table.jsx (1)
13-21: 🛠️ Refactor suggestion
Add PropTypes and improve type safety
The component would benefit from proper prop type definitions:
+import PropTypes from 'prop-types'; export default function CustomTable({ fullData = [], headers = [], data = [] }) { // ... component code ... } +CustomTable.propTypes = { + fullData: PropTypes.arrayOf(PropTypes.object), + headers: PropTypes.arrayOf(PropTypes.string), + data: PropTypes.arrayOf(PropTypes.array), +};📝 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 PropTypes from 'prop-types'; export default function CustomTable({ fullData = [], headers = [], data = [] }) { const tableHeaders = fullData.length > 0 ? Object.keys(fullData[0]) : headers; const tableData = fullData.length > 0 ? fullData.map(item => Object.values(item)) : data; // ... component logic and rendering return ( // ... table rendering logic ); } CustomTable.propTypes = { fullData: PropTypes.arrayOf(PropTypes.object), headers: PropTypes.arrayOf(PropTypes.string), data: PropTypes.arrayOf(PropTypes.array), };backend/src/models/Tour.js (1)
98-102:
⚠️ Potential issueYo! This association is looking sus!
The foreign key in the Tour-TourPopup association doesn't follow the conventional naming pattern.
Tour.hasMany(models.TourPopup, { - foreignKey: 'tourPopupId', + foreignKey: 'tourId', as: 'steps', onDelete: 'CASCADE', });📝 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.Tour.hasMany(models.TourPopup, { foreignKey: 'tourId', as: 'steps', onDelete: 'CASCADE', });frontend/src/App.jsx (2)
49-49:
⚠️ Potential issueSecure that statistics route, homie!
The
/statisticsroute needs authentication protection to prevent unauthorized access to user data.Apply this diff to add authentication:
- <Route path="/statistics" element={<UserStatisticsPage />} /> + <Route path="/statistics" element={<Private Component={UserStatisticsPage} />} />📝 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.<Route path="/statistics" element={<Private Component={UserStatisticsPage} />} />
45-46:
⚠️ Potential issueYo dawg, we need to secure these routes!
Removing the
Privatecomponent wrapper from/bannerand/popuproutes could expose sensitive admin functionality to unauthorized users. This is making me nervous, like mom's spaghetti.Apply this diff to restore authentication:
- <Route path="/banner" element={<BannerDefaultPage /> }/> - <Route path="/popup" element={<PopupDefaultPage /> }/> + <Route path="/banner" element={<Private Component={BannerDefaultPage} />} /> + <Route path="/popup" element={<Private Component={PopupDefaultPage} />} />📝 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.<Route path="/banner" element={<Private Component={BannerDefaultPage} />} /> <Route path="/popup" element={<Private Component={PopupDefaultPage} />} />backend/src/service/helperLink.service.js (1)
58-61: 🛠️ Refactor suggestion
Enhance that error handling, fam!
The error message is too generic. We need more specific error details to help with debugging.
Apply this diff to improve error handling:
- console.log(e); - await t.rollback(); - throw new Error('Error creating helper'); + console.error('Failed to create helper:', e.message); + await t.rollback(); + throw new Error(`Failed to create helper: ${e.message}`);📝 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.console.error('Failed to create helper:', e.message); await t.rollback(); throw new Error(`Failed to create helper: ${e.message}`);frontend/src/services/helperLinkService.js (1)
23-39: 🛠️ Refactor suggestion
Validate those new props like your life depends on it!
The validation function isn't checking the new properties (url, active, absolutePath).
Add validation for the new properties:
const validateHelper = (helper, links) => { + if (!helper?.url) { + throw new Error('URL is required'); + } + if (typeof helper?.active !== 'boolean') { + throw new Error('Active status must be a boolean'); + } + if (!helper?.absolutePath) { + throw new Error('Absolute path is required'); + } // ... existing validations ... };📝 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 validateHelper = (helper, links) => { if (!helper?.url) { throw new Error('URL is required'); } if (typeof helper?.active !== 'boolean') { throw new Error('Active status must be a boolean'); } if (!helper?.absolutePath) { throw new Error('Absolute path is required'); } if (!helper?.title) { throw new Error('Header is required'); } if (!helper?.headerBackgroundColor) { throw new Error('Header background color is required'); } if (!helper?.linkFontColor) { throw new Error('Link font color is required'); } if (!helper?.iconColor) { throw new Error('Icon color is required'); } if (links.some((it) => !it?.title || !it?.url)) { throw new Error('Title and URL are required'); } };backend/src/controllers/guide.controller.js (1)
67-68:
⚠️ Potential issueFix that error handling, it's slipping like mom's spaghetti!
The error handling in
getIncompleteGuidesByUrlisn't sending a response, which will cause the request to hang.Apply this diff to fix the error handling:
- internalServerError('GET_INCOMPLETE_GUIDES_BY_URL_ERROR', error.message); + const { payload, statusCode } = internalServerError('GET_INCOMPLETE_GUIDES_BY_URL_ERROR', error.message); + res.status(statusCode).json(payload);📝 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 { payload, statusCode } = internalServerError('GET_INCOMPLETE_GUIDES_BY_URL_ERROR', error.message); res.status(statusCode).json(payload); }frontend/src/products/Popup/PopupComponent.jsx (1)
46-48:
⚠️ Potential issueMom's spaghetti moment: Add security for external URLs! 🔒
When opening URLs, add
rel="noopener noreferrer"to prevent potential security issues withwindow.open.-window.open(actionButtonUrl, '_self'); +const url = new URL(actionButtonUrl, window.location.origin); +if (url.origin === window.location.origin) { + window.open(url, '_self'); +} else { + window.open(url, '_blank', 'noopener,noreferrer'); +}📝 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 url = new URL(actionButtonUrl, window.location.origin); if (url.origin === window.location.origin) { window.open(url, '_self'); } else { window.open(url, '_blank', 'noopener,noreferrer'); } } else if (buttonAction === 'open url in new page') { window.open(actionButtonUrl, '_blank');backend/migrations/0012-1.1-tour.js (1)
31-34: 🛠️ Refactor suggestion
Color validation needs some extra sauce! 🎨
Consider adding length validation for hex colors to prevent potential SQL injection or overflow.
validate: { isHexColor(value) { + if (value.length > 7) { + throw new Error('Color value exceeds maximum length'); + } validateHexColor(value, 'headerColor'); }, },Also applies to: 41-44, 51-54, 61-64
frontend/src/components/Links/Settings/Settings.jsx (1)
144-147: 🛠️ Refactor suggestion
Knees weak, arms are heavy: Redundant onChange handlers!
The onChange handlers are updating both Formik and local state, which is redundant and could cause race conditions.
Use Formik's
setFieldValueinstead:-onChange={(e) => { - handleChange(e); - setState((prev) => ({ ...prev, id: e.target.value })); -}} +onChange={handleChange}Also applies to: 161-164, 188-191
backend/src/test/unit/controllers/tour.test.js (1)
30-42: 🛠️ Refactor suggestion
Error handling's making me lose myself in the moment!
The error test case only checks for 500 status code but doesn't verify different types of errors.
Add more error test cases:
it('should return 400 when creating a tour with invalid data', async () => { req.body = { ...tour().build(), name: '' }; await tourController.createTour(req, res); expect(res.status.getCall(0).args[0]).to.equal(400); }); it('should return 403 when user lacks permission', async () => { req.body = tour().build(); req.user.role = 'guest'; await tourController.createTour(req, res); expect(res.status.getCall(0).args[0]).to.equal(403); });backend/postman/Tours.postman_collection.json (1)
142-142:
⚠️ Potential issueYo dawg, we got a syntax error! 🚨
There's an unexpected
buttonBackgroundColortext that breaks the JSON structure.Apply this fix:
- {buttonBackgroundColor + {📝 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.{🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: Property key must be double quoted
(parse)
|
Looks good. Are you going to use our components on the UI, instead of the generic MUI components? |
|
Do we have a custom component for tables? I used the same one we used for the settings page |
|
Looks like we don't. Possible that we can copy one of the components from other project (VerifyWise, Checkmate or Headcount) and see how it looks? I can then suggest some improvements if needed. |
There was a problem hiding this comment.
It would be nice to see PropTypes here.
Solves #508
