PR for Issue#332 [BE: API key & code snippet] #347
Conversation
|
Warning Rate limit exceeded@d02ev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis pull request introduces server URL and API key configuration functionality for teams. The changes span multiple files across the backend and frontend, adding new database columns, permissions, routes, services, and UI components. The implementation allows administrators to set and retrieve server configuration details securely, with robust validation and encryption mechanisms. The changes enable teams to manage their server URLs and API keys through a centralized configuration interface. Changes
Sequence DiagramsequenceDiagram
participant Admin
participant Frontend
participant Backend
participant Database
Admin->>Frontend: Set server URL and API key
Frontend->>Backend: PUT /set-config
Backend->>Backend: Validate input
Backend->>Backend: Encrypt API key
Backend->>Database: Update team configuration
Database-->>Backend: Confirmation
Backend-->>Frontend: Success response
Frontend-->>Admin: Configuration saved
Possibly Related PRs
Suggested Reviewers
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: 15
🧹 Outside diff range and nitpick comments (3)
frontend/src/utils/constants.js (1)
3-6: Yo! Consider using environment variables instead of hardcoded URLsMom's spaghetti tells me that hardcoding URLs in the constants file might make our knees weak later. Instead, we should consider using environment variables (like
.envfiles) to manage these configurations, making it easier to switch between environments without code changes.Example implementation:
export const API_BASE_URL = process.env.REACT_APP_API_BASE_URL || 'http://localhost:3000/api/';Then in your
.envfiles:# .env.development REACT_APP_API_BASE_URL=http://localhost:3000/api/ # .env.staging REACT_APP_API_BASE_URL=https://onboarding-demo.bluewavelabs.ca/api/backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js (1)
1-2: Yo dawg, let's drop that redundant 'use strict'!The 'use strict' directive isn't needed in ES modules as they're automatically strict. Let's keep it clean!
-'use strict'; module.exports = {🧰 Tools
🪛 Biome
[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)
frontend/src/scenes/settings/CodeTab/CodeTab.jsx (1)
Line range hint
86-116: Lose yourself in this code snippet improvement! 🎵The current code snippet has some security and performance concerns:
- It's loading the script twice (fetch + import)
- The API key is exposed in global scope
- Missing error handling for script loading failures
Here's a more secure and efficient implementation:
<script> (function() { - const apiKey = '${apiKey}'; - const apiUrl = '${serverUrl}'; + const config = { + apiKey: '${apiKey}', + apiUrl: '${serverUrl}' + }; + const SCRIPT_URL = 'https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js'; - fetch(apiUrl, { + fetch(config.apiUrl, { method: 'POST', headers: { 'Content-Type': 'application/json', }, - body: JSON.stringify({ userId: apiKey }) + body: JSON.stringify({ userId: config.apiKey }) }) .then(response => response.json()) .then(data => { - const script = document.createElement('script'); - script.src = 'https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiKey=' + apiKey; - script.type = 'module'; - script.onload = () => { - import('https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiKey=' + apiKey) - .then(module => { - module.default(data.popupData); - }); - }; - document.head.appendChild(script); + return import(`${SCRIPT_URL}?apiKey=${config.apiKey}`) + .then(module => module.default(data.popupData)) + .catch(err => console.error('Failed to load popup renderer:', err)); }) - .catch(error => console.error('Error fetching onboarding data:', error)); + .catch(error => { + console.error('Onboarding initialization failed:', error); + // Optionally notify the parent application + window.dispatchEvent(new CustomEvent('onboardingError', { detail: error })); + }); })(); </script>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
backend/config/settings.js(1 hunks)backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js(1 hunks)backend/src/controllers/team.controller.js(3 hunks)backend/src/models/Team.js(1 hunks)backend/src/routes/team.routes.js(2 hunks)backend/src/service/team.service.js(2 hunks)frontend/src/scenes/settings/CodeTab/CodeTab.jsx(3 hunks)frontend/src/services/teamServices.js(1 hunks)frontend/src/utils/constants.js(1 hunks)
🧰 Additional context used
🪛 Biome
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js
[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)
🔇 Additional comments (7)
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js (1)
15-18: Looks solid! But let's verify the rollback order, yo!
The down migration correctly removes both columns. Just make sure to drop any indexes first if you add them.
✅ Verification successful
Yo, the migration's clean as mom's spaghetti! 🍝
Looking at the up migration, there's no indexes being created, just straight-up column additions. The down migration's order is perfect since there are no dependencies or constraints to worry about. You can drop these columns in any order without breaking a sweat!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any foreign key constraints or indexes that depend on these columns
rg -A 5 "FOREIGN KEY.*apiKey|FOREIGN KEY.*serverUrl|INDEX.*apiKey|INDEX.*serverUrl" --type sql
Length of output: 245
Script:
#!/bin/bash
# Let's check the up migration for any index creation
cat backend/migrations/*api_key_server_url_cols_teams_rel.js
# Let's also check for any other migrations that might be related
fd -e js . backend/migrations/ -x grep -l "apiKey\|serverUrl" {}
Length of output: 709
backend/config/settings.js (2)
26-26: Yo, this permission addition looks solid! 🔒
The restriction to ADMIN role is appropriate for managing sensitive API keys and server configurations.
26-26: Let's make sure this permission is properly wired up, fam! 🔍
We should verify that this permission is being correctly enforced in the route middleware.
✅ Verification successful
Yo, the permission is wired up tight like mom's spaghetti! 🍝
The serverUrlAndApiKey permission is properly implemented and secured:
- It's correctly used in
team.routes.jswith theaccessGuardmiddleware - The
accessGuardmiddleware inaccessGuard.middleware.jsproperly checks user roles - The route
/set-configis protected with bothauthenticateJWTand the correct permission check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the permission is properly used in route definitions
# Look for route definitions using this permission
rg -A 5 "serverUrlAndApiKey" ./backend/src/routes/
# Check for middleware usage
rg -A 5 "checkPermission.*serverUrlAndApiKey" ./backend/src/
Length of output: 636
Script:
#!/bin/bash
# Let's verify the middleware implementation and permission usage
# Check the accessGuard middleware implementation
rg -A 10 "accessGuard" ./backend/src/middleware/
# Look for the teamPermissions definition
rg -A 5 "teamPermissions.*=|const.*teamPermissions" ./backend/src/
# Check for any other usage of serverUrlAndApiKey
rg -A 5 "serverUrlAndApiKey" ./backend/src/
Length of output: 4056
backend/src/routes/team.routes.js (2)
4-4: Yo, this import's clean as mom's spaghetti! 🍝
The setConfig import follows the established pattern and integrates smoothly with existing controller imports.
29-29: 🛠️ Refactor suggestion
Knees weak, arms heavy - we need some extra security! 🔒
While the basic security setup looks good with JWT auth and access guard, when handling sensitive data like API keys, consider these additional safeguards:
- Add rate limiting to prevent brute force attempts
- Implement audit logging for config changes
- Consider adding request validation middleware
Here's a suggested enhancement:
+ const rateLimit = require('express-rate-limit');
+ const configLimiter = rateLimit({
+ windowMs: 15 * 60 * 1000, // 15 minutes
+ max: 5 // limit each IP to 5 requests per windowMs
+ });
- router.put('/set-config', authenticateJWT, accessGuard(teamPermissions.serverUrlAndApiKey), setConfig);
+ router.put('/set-config',
+ configLimiter,
+ authenticateJWT,
+ accessGuard(teamPermissions.serverUrlAndApiKey),
+ // Add validation middleware
+ (req, res, next) => {
+ const { serverUrl, apiKey } = req.body;
+ if (!serverUrl || !apiKey) {
+ return res.status(400).json({ error: 'Missing required fields' });
+ }
+ next();
+ },
+ setConfig
+ );Let's check if rate limiting is implemented elsewhere:
frontend/src/scenes/settings/CodeTab/CodeTab.jsx (1)
8-10: Yo, these imports are straight fire! 🔥
The new imports provide essential functionality for error handling and API communication. They're properly scoped and relevant to the new features.
backend/src/controllers/team.controller.js (1)
161-161: Yo! Export statement looking clean!
The module exports are properly organized and include all necessary functions.
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js (1)
1-2: Yo dawg, that 'use strict' is like wearing a life jacket in the shower! 🔥The 'use strict' directive is redundant here since JavaScript modules are automatically in strict mode.
-'use strict'; module.exports = {🧰 Tools
🪛 Biome
[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)
backend/src/controllers/team.controller.js (1)
116-126: Yo! There's a typo in your error message and we could make this URL validation tighter than your rap game!try { const url = new URL(serverUrl); if (!['http:', 'https:'].includes(url.protocol)) { throw new Error('Invalid protocol'); } if (url.username || url.password) { - throw new Error('URL cannt contain credentials'); + throw new Error('URL cannot contain credentials'); } + // Block private/internal IP addresses + const hostname = url.hostname; + if ( + /^(127\.|10\.|172\.(1[6-9]|2[0-9]|3[0-1])\.|192\.168\.)/.test(hostname) || + hostname === 'localhost' || + hostname.endsWith('.local') + ) { + throw new Error('Private/internal URLs are not allowed'); + } } catch (err) { return res.status(400).json({ message: 'Invalid server URL format. Only HTTP/HTTPS protocols are allowed.' }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js(1 hunks)backend/src/controllers/team.controller.js(2 hunks)backend/src/models/Team.js(1 hunks)frontend/src/scenes/settings/CodeTab/CodeTab.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/models/Team.js
- frontend/src/scenes/settings/CodeTab/CodeTab.jsx
🧰 Additional context used
🪛 Biome
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js
[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)
🔇 Additional comments (3)
backend/migrations/20241115084356-api_key_server_url_cols_teams_rel.js (2)
17-20: Clean rollback, straight fire! 🔥
The down migration is clean and properly removes both columns. No changes needed here!
4-15:
Mom's spaghetti moment! 🍝 We need to beef up this security!
While the TEXT type is solid, we've got some missing ingredients in our security recipe:
- The
apiKeyneeds encryption at rest to prevent unauthorized access - Add an index on
serverUrlfor query performance - Consider adding length validation constraints
- Add documentation comments explaining the purpose of these columns
Here's a more secure version with all the fixings:
async up (queryInterface, Sequelize) {
+ // Add encrypted API key column for secure team authentication
await queryInterface.addColumn('teams', 'apiKey', {
type: Sequelize.TEXT,
allowNull: true,
defaultValue: null,
+ // Add comment explaining encryption requirement
+ comment: 'Stores encrypted API key - must be encrypted before storage',
})
+ // Add server URL column for team configuration
await queryInterface.addColumn('teams', 'serverUrl', {
type: Sequelize.TEXT,
allowNull: true,
defaultValue: null,
+ validate: {
+ len: [1, 2048], // Reasonable URL length limit
+ isUrl: true,
+ },
+ comment: 'Stores team server URL configuration',
})
+ // Add index for performance
+ await queryInterface.addIndex('teams', ['serverUrl'])
}Let's check if there's any existing encryption middleware:
backend/src/controllers/team.controller.js (1)
169-169: LGTM! Exports are clean like fresh spaghetti!
The module exports are properly updated to include the new setConfig function.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
backend/src/utils/team.helper.js (1)
17-17: Let's drop some documentation beats! 📝Consider adding JSDoc documentation for better code maintainability.
+/** + * Encrypts an API key using JWT + * @param {string} apiKey - The API key to encrypt + * @returns {string} The encrypted API key + * @throws {Error} If the API key is invalid or encryption key is not configured + */ const encryptApiKey = (apiKey) => { // ... implementation }; +/** + * Decrypts an encrypted API key + * @param {string} apiKey - The encrypted API key to decrypt + * @returns {string|null} The decrypted API key or null if decryption fails + * @throws {Error} If the encryption key is not configured + */ const decryptApiKey = (apiKey) => { // ... implementation }; module.exports = { encryptApiKey, decryptApiKey };backend/src/controllers/team.controller.js (2)
124-124: Yo! There's a typo in your error message that's making my palms sweaty!Fix the typo in the error message: "cannt" should be "cannot"
- throw new Error('URL cannt contain credentials'); + throw new Error('URL cannot contain credentials');
105-140: Yo! This function needs some request throttling to prevent rapid-fire config changes!Consider implementing rate limiting for this endpoint to prevent abuse. This is especially important since we're dealing with API keys and server configurations.
Would you like me to provide an example implementation using Express rate limiter?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
backend/.env(1 hunks)backend/.env.production(1 hunks)backend/.env.test(1 hunks)backend/src/controllers/team.controller.js(3 hunks)backend/src/utils/team.helper.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/.env
- backend/.env.production
- backend/.env.test
🔇 Additional comments (1)
backend/src/utils/team.helper.js (1)
1-4: Yo! Let's check that JWT library version, fam!
We should verify the version of jsonwebtoken to ensure we're not using one with known vulnerabilities.
There was a problem hiding this comment.
The error message for the url is wrong. Here the url starts with https:// and the actual error message should be something like 'server url is not valid' since it is not in the form of X.Y
Also when you comeback you should see api key and server url in their fields.
…nd surname fields and used Formik and Yup to simplify SetNewPassword
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
backend/src/test/mocks/banner.mock.js (1)
Line range hint
1-150: There's spaghetti on this sweater: validList needs updating! 🍝The
validListarray doesn't include the newactionUrlfield, but theBannerBuilderdoes. This inconsistency could cause test failures.Add
actionUrlto all entries invalidListto maintain consistency with the builder pattern.frontend/src/tests/scenes/popup/CreatePopupPage.test.jsx (1)
Line range hint
116-171: Handle commented test code properlyInstead of keeping a large block of commented code, let's create a proper pending test!
- // TODO: Write a edit test - // it('initializes with existing popup data in edit mode', async () => { - // ... large block of commented code ... - // }); + it.todo('initializes with existing popup data in edit mode');Would you like me to create a GitHub issue to track the implementation of this test?
🧹 Nitpick comments (15)
backend/src/utils/popup.helper.js (1)
24-34: Knees weak, arms heavy - let's sanitize those URLs!The URL validation is good but could benefit from additional sanitization:
- Add URL encoding validation
- Check for potentially dangerous protocols
const validateUrl = (value, fieldName) => { if (!value) return; try { const url = new URL(value); + // Check for potentially dangerous protocols if (!['http:', 'https:'].includes(url.protocol)) { throw new Error('URL must use HTTP or HTTPS protocol'); } + // Validate URL encoding + if (value !== decodeURIComponent(encodeURIComponent(value))) { + throw new Error(`Invalid URL encoding for ${fieldName}`); + } } catch (error) { throw new Error(`Invalid URL for ${fieldName}: ${error.message}`); } };backend/src/utils/banner.helper.js (2)
8-8: Yo dawg, the error message looks good but could be more descriptive!Consider adding the invalid position value to the error message for better debugging:
- throw new Error("Invalid position"); + throw new Error(`Invalid position: ${value}. Expected one of: top, bottom`);
12-22: Enhance relative URL validation with path-only URL supportThe current implementation handles absolute URLs and paths starting with '/', but could be improved to handle path-only URLs (e.g., 'api/v1/endpoint').
const validateRelativeUrl = (value, fieldName) => { if (!value) return; try { new URL(value); } catch (error) { - if (value.startsWith('/')) { + if (value.startsWith('/') || /^[a-zA-Z0-9-_/.]+$/.test(value)) { return } throw new Error(`Invalid URL for ${fieldName}: ${error.message}`); } }frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (3)
26-28: Add URL validation feedback for better UXThe URL change handler could provide immediate feedback to users about URL validity.
const handleUrlChange = (event) => { + const value = event.target.value; + try { + if (value) new URL(value); + setUrl(value); + } catch (error) { + setUrl(value); + // Assuming you have a setUrlError state setter + setUrlError("Please enter a valid URL including protocol (http:// or https://)"); + } - setUrl(event.target.value); };
37-42: Enhance URL input field with validation feedbackConsider adding helper text to guide users on URL format requirements.
<h2 style={{ marginBottom: 0 }}>URL</h2> <CustomTextField TextFieldWidth='241px' value={url} onChange={handleUrlChange} + helperText="Enter a valid URL (e.g., https://example.com)" + error={Boolean(urlError)} + errorText={urlError} />
60-69: Props validation looking clean, homie! Just add isRequired where neededConsider marking required props with isRequired for better prop-type validation.
PopupContent.propTypes = { - actionButtonText: PropTypes.string, - setActionButtonText: PropTypes.func, - setActionButtonUrl: PropTypes.func, - buttonAction: PropTypes.string, - actionButtonUrl: PropTypes.string, - setButtonAction: PropTypes.func, - url: PropTypes.string, - setUrl: PropTypes.func, + actionButtonText: PropTypes.string.isRequired, + setActionButtonText: PropTypes.func.isRequired, + setActionButtonUrl: PropTypes.func.isRequired, + buttonAction: PropTypes.string.isRequired, + actionButtonUrl: PropTypes.string.isRequired, + setButtonAction: PropTypes.func.isRequired, + url: PropTypes.string.isRequired, + setUrl: PropTypes.func.isRequired, }frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx (1)
58-70: Arms are heavy: URL fields need better UX! 🎯Having two URL fields might confuse users. Consider:
- Adding helper text explaining the difference
- Adding placeholders with example URLs
- Visual indication of URL validity
<CustomTextField TextFieldWidth='241px' value={url} onChange={handleSetUrl} + placeholder="https://example.com" + helperText="Enter the main banner URL" />backend/src/models/Popup.js (2)
57-57: Maintain consistent string literal styleYo! The hex color validation calls are using different quote styles across different fields. Let's keep it consistent!
- validateHexColor(value, "headerBackgroundColor"); - validateHexColor(value, "headerColor"); - validateHexColor(value, "textColor"); - validateHexColor(value, "buttonBackgroundColor"); - validateHexColor(value, "buttonTextColor"); + validateHexColor(value, 'headerBackgroundColor'); + validateHexColor(value, 'headerColor'); + validateHexColor(value, 'textColor'); + validateHexColor(value, 'buttonBackgroundColor'); + validateHexColor(value, 'buttonTextColor');Also applies to: 67-67, 77-77, 87-87, 97-97
111-119: Consider grouping related fields togetherThe
actionUrlfield should be grouped with theurlfield since they're both URL-type fields. This improves code organization and readability.- actionUrl: { - type: DataTypes.STRING, - allowNull: true, - validate: { - isUrl(value) { - validateUrl(value, "actionUrl"); - }, - }, - }, + // Move this field next to the url field around line 38frontend/src/scenes/banner/CreateBannerPage.jsx (2)
47-47: Remove debug console.log statementMom's spaghetti! We've got a console.log that needs to go!
- console.log("Get banner successful:", bannerData);
82-90: Consider using object property shorthandWhen property names match variable names, we can use object property shorthand for cleaner code.
const bannerData = { - backgroundColor, - fontColor, - url, - actionUrl, - position: isTopPosition ? "top" : "bottom", - closeButtonAction: buttonAction.toLowerCase(), - bannerText, + backgroundColor, + fontColor, + url, + actionUrl, + position: isTopPosition ? 'top' : 'bottom', + closeButtonAction: buttonAction.toLowerCase(), + bannerText, };backend/src/controllers/banner.controller.js (1)
38-44: Consider adding URL validation before the color validation.The URL validation block should be moved before the color validation to fail fast on invalid URLs, which are more likely to be problematic than color values.
- const colorFields = { fontColor, backgroundColor }; - const colorCheck = checkColorFieldsFail(colorFields, res); - if (colorCheck) { - return colorCheck; - } - if (actionUrl) { try { validateUrl(actionUrl, "actionUrl"); } catch (err) { return res.status(400).json({ errors: [{ msg: err.message }] }); } } if (url) { try { validateRelativeUrl(url, "url"); } catch (err) { return res.status(400).json({ errors: [{ msg: err.message }] }); } } + + const colorFields = { fontColor, backgroundColor }; + const colorCheck = checkColorFieldsFail(colorFields, res); + if (colorCheck) { + return colorCheck; + }backend/src/controllers/popup.controller.js (1)
41-55: Consider extracting validation logic to middleware.The URL validation logic is duplicated between banner and popup controllers. This could be extracted to middleware.
Example middleware implementation:
const validateUrlFields = (req, res, next) => { const { actionUrl, url } = req.body; if (actionUrl) { try { validateUrl(actionUrl, "actionUrl"); } catch (err) { return res.status(400).json({ errors: [{ msg: err.message }] }); } } if (url) { try { validateRelativeUrl(url, "url"); } catch (err) { return res.status(400).json({ errors: [{ msg: err.message }] }); } } next(); };frontend/src/scenes/popup/CreatePopupPage.jsx (2)
102-115: DRY up URL validation logic.The URL validation is duplicated for both URLs. Consider extracting to a helper function.
+ const validateUrlField = (url, fieldName) => { + if (url && url !== "https://") { + const urlError = validateUrl(url); + if (urlError) { + emitToastError(`${fieldName}: ${urlError}`); + return false; + } + } + return true; + }; + const onSave = async () => { - if (actionButtonUrl && actionButtonUrl !== "https://") { - const urlError = validateUrl(actionButtonUrl); - if (urlError) { - emitToastError(urlError); - return; - } - } - if (url && url !== "https://") { - const urlError = validateUrl(url); - if (urlError) { - emitToastError(urlError); - return; - } + if (!validateUrlField(actionButtonUrl, 'Action Button URL') || + !validateUrlField(url, 'URL')) { + return; }
30-31: Consider using URL constructor for initial state.Instead of hardcoding "https://", consider using the URL constructor for a more robust initial state.
- const [actionButtonUrl, setActionButtonUrl] = useState("https://"); - const [url, setUrl] = useState("https://"); + const [actionButtonUrl, setActionButtonUrl] = useState(new URL("https://example.com").toString()); + const [url, setUrl] = useState(new URL("https://example.com").toString());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/migrations/20241210225728-add-action-url-banner.js(1 hunks)backend/migrations/20241210225734-add-action-url-popup.js(1 hunks)backend/src/controllers/banner.controller.js(10 hunks)backend/src/controllers/popup.controller.js(10 hunks)backend/src/models/Banner.js(1 hunks)backend/src/models/Popup.js(10 hunks)backend/src/test/mocks/banner.mock.js(1 hunks)backend/src/test/mocks/popup.mock.js(1 hunks)backend/src/test/unit/controllers/popup.test.js(1 hunks)backend/src/utils/banner.helper.js(1 hunks)backend/src/utils/popup.helper.js(1 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx(1 hunks)frontend/src/scenes/banner/CreateBannerPage.jsx(1 hunks)frontend/src/scenes/popup/CreatePopupPage.jsx(1 hunks)frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx(1 hunks)frontend/src/tests/scenes/popup/CreatePopupPage.test.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/test/unit/controllers/popup.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
backend/migrations/20241210225728-add-action-url-banner.js
[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)
backend/migrations/20241210225734-add-action-url-popup.js
[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)
🔇 Additional comments (4)
backend/migrations/20241210225728-add-action-url-banner.js (1)
9-11: Same validation issue as in popup migration
This migration has the same URL validation concern as discussed in the popup migration.
backend/src/utils/popup.helper.js (1)
37-41: LGTM! The exports are clean like mom's spaghetti!
The module exports are well-organized and include all necessary functions.
backend/src/models/Banner.js (1)
33-41: Yo! URL validation needs some love! 🍝
The URL validation for the url field uses validateRelativeUrl, but we should verify if this correctly handles all edge cases like protocol-less URLs or empty strings.
Consider adding URL normalization to ensure consistent storage format.
✅ Verification successful
Let me search for the validation function with a different approach since the first attempt didn't yield results. I'll look for both the function definition and its usage patterns to understand how URL validation is implemented.
I see the function is used across multiple files. Let's check its actual implementation in the helper files and how it's being used in the controllers to understand the validation logic better. Mom's spaghetti won't help us until we see what's cooking in those files! 🍝
Yo dawg, the URL validation is actually pretty solid! 🍝
After diving deep into the validation sauce, I can confirm that validateRelativeUrl handles all the key cases:
- Empty values (returns early)
- Full URLs (validates via URL constructor)
- Relative URLs (accepts paths starting with '/')
The implementation is consistent across both banner and popup helpers, and it's properly integrated in the controllers. No spaghetti code here - it's clean and handles edge cases well!
• The validation is used consistently in:
- backend/src/models/Banner.js
- backend/src/models/Popup.js
- backend/src/controllers/banner.controller.js
- backend/src/controllers/popup.controller.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validateRelativeUrl implementation and tests
ast-grep --pattern 'function validateRelativeUrl($_) {
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Let's try a broader search for the validation function and its imports
rg -l "validateRelativeUrl"
# Also search for any URL validation related functions
rg "function.*Url.*validate" -g "*.js" -g "*.ts"
# Check for URL validation tests
rg "test.*url.*validation" -g "*.test.js" -g "*.spec.js"
Length of output: 352
Script:
#!/bin/bash
# Let's check the implementation in the helper files
cat backend/src/utils/banner.helper.js
cat backend/src/utils/popup.helper.js
# Also check how it's used in controllers
rg -A 5 "validateRelativeUrl" backend/src/controllers/banner.controller.js
rg -A 5 "validateRelativeUrl" backend/src/controllers/popup.controller.js
Length of output: 3157
backend/src/controllers/banner.controller.js (1)
4-8: LGTM! Clean import organization.
The imports are well-organized and the validation utilities are properly segregated.
|
Hey, we are near to 1.0 release, and we have decided to remove API_KEY; Server URL will stay. Sorry for that change. Can you remove API_KEY both on backend and frontend, and keep Server url? |
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (1)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx (1)
18-24:⚠️ Potential issueMom's spaghetti: These URL handlers need validation! 🍜
Both
handleSetUrlandhandleSetActionUrlare setting values without any validation.Add URL validation:
const handleSetUrl = (event) => { + const value = event.target.value.trim(); + if (!value) { + setUrl(''); + return; + } + try { + const url = new URL(value); + if (!['http:', 'https:'].includes(url.protocol)) { + throw new Error('Invalid protocol'); + } + setUrl(value); + } catch (e) { + // Consider showing an error message to the user + console.error('Invalid URL format'); + } };
🧹 Nitpick comments (6)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx (2)
104-116: Knees weak: Add some helpful hints to these text fields! 💪The URL input fields could use some guidance for users.
Add placeholders and error handling:
<CustomTextField TextFieldWidth='241px' value={url} onChange={handleSetUrl} + placeholder="https://example.com" + helperText="Enter a valid https:// or http:// URL" + error={url && !isValidUrl(url)} />
137-146: Arms are heavy: Let's make these PropTypes more strict! 💪The PropTypes could be more specific about required props and function signatures.
Enhance PropTypes:
BannerLeftContent.propTypes = { - setIsTopPosition: PropTypes.func, + setIsTopPosition: PropTypes.func.isRequired, - url: PropTypes.string, + url: PropTypes.string.isRequired, - setUrl: PropTypes.func, + setUrl: PropTypes.func.isRequired, // ... rest of the props };🧰 Tools
🪛 Biome (1.9.4)
[error] 146-146: expected
}but instead the file endsthe file ends here
(parse)
backend/src/controllers/helperLink.controller.js (1)
194-194: Fix typo in error code constantYo! The error code "GET_ALL_HELPERS_WITH_LINKSERROR" should follow the consistent naming pattern used throughout the codebase.
- "GET_ALL_HELPERS_WITH_LINKSERROR", + "GET_ALL_HELPERS_WITH_LINKS_ERROR",backend/src/service/banner.service.js (1)
66-77: Don't let important error details slip awayWhen rethrowing errors, omitting the original message might leave us feeling shaky. Including the original error's message keeps everything on solid ground and helps with debugging.
Apply this diff to include the original error message:
} catch (error) { - throw new Error("Error retrieving banner by URL"); + throw new Error(`Error retrieving banner by URL: ${error.message}`); }backend/src/service/popup.service.js (1)
85-94: Keep the error handling in tuneLosing the original error message might throw us off balance. Let's include the original error's message to stay in sync and standardize the casing for consistency.
Apply this diff to enhance error handling:
} catch (error) { - throw new Error("Error retrieving popup by URL"); + throw new Error(`Error retrieving Popup by URL: ${error.message}`); }backend/src/controllers/popup.controller.js (1)
47-61: URL validation looking clean, but let's make it DRY! 🧹The URL validation logic is duplicated between
addPopupandeditPopup. Consider extracting it into a helper function.const validateUrls = (actionUrl, url, res) => { if (actionUrl) { try { validateUrl(actionUrl, "actionUrl"); } catch (err) { return res.status(400).json({ errors: [{ msg: err.message }] }); } } if (url) { try { validateRelativeUrl(url, "url"); } catch (err) { return res.status(400).json({ errors: [{ msg: err.message }] }); } } return null; };Also applies to: 164-178
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/Dockerfile(0 hunks)backend/config/settings.js(1 hunks)backend/src/controllers/banner.controller.js(10 hunks)backend/src/controllers/guide.controller.js(2 hunks)backend/src/controllers/helperLink.controller.js(1 hunks)backend/src/controllers/popup.controller.js(10 hunks)backend/src/models/Popup.js(10 hunks)backend/src/routes/guide.routes.js(1 hunks)backend/src/routes/helperLink.routes.js(1 hunks)backend/src/service/banner.service.js(2 hunks)backend/src/service/guidelog.service.js(2 hunks)backend/src/service/helperLink.service.js(2 hunks)backend/src/service/popup.service.js(3 hunks)frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx(1 hunks)frontend/src/scenes/popup/CreatePopupPage.jsx(1 hunks)
💤 Files with no reviewable changes (1)
- backend/Dockerfile
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/models/Popup.js
[error] 139-139: expected , but instead found }
Remove }
(parse)
frontend/src/scenes/popup/CreatePopupPage.jsx
[error] 141-141: Expected a statement but instead found 'catch (error)'.
Expected a statement here.
(parse)
[error] 144-144: expected , but instead found ;
Remove ;
(parse)
[error] 148-148: Expected a catch clause but instead found '}'.
Expected a catch clause here.
(parse)
[error] 148-148: expected , but instead found .
Remove .
(parse)
[error] 148-148: Object and Array patterns require initializers.
This pattern is declared, but it is not given an initialized value.
(parse)
[error] 148-148: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 206-206: Expected a statement but instead found 'catch (error)'.
Expected a statement here.
(parse)
[error] 270-317: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 14-14: Shouldn't redeclare 'RichTextEditor'. Consider to delete it or rename it.
'RichTextEditor' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'React'. Consider to delete it or rename it.
'React' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'useEffect'. Consider to delete it or rename it.
'useEffect' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'useState'. Consider to delete it or rename it.
'useState' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'useLocation'. Consider to delete it or rename it.
'useLocation' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'useNavigate'. Consider to delete it or rename it.
'useNavigate' is defined here:
(lint/suspicious/noRedeclare)
[error] 18-18: Shouldn't redeclare 'addPopup'. Consider to delete it or rename it.
'addPopup' is defined here:
(lint/suspicious/noRedeclare)
[error] 19-19: Shouldn't redeclare 'editPopup'. Consider to delete it or rename it.
'editPopup' is defined here:
(lint/suspicious/noRedeclare)
[error] 20-20: Shouldn't redeclare 'getPopupById'. Consider to delete it or rename it.
'getPopupById' is defined here:
(lint/suspicious/noRedeclare)
[error] 22-22: Shouldn't redeclare 'GuideTemplate'. Consider to delete it or rename it.
'GuideTemplate' is defined here:
(lint/suspicious/noRedeclare)
[error] 23-23: Shouldn't redeclare 'emitToastError'. Consider to delete it or rename it.
'emitToastError' is defined here:
(lint/suspicious/noRedeclare)
[error] 24-24: Shouldn't redeclare 'toastEmitter'. Consider to delete it or rename it.
'toastEmitter' is defined here:
(lint/suspicious/noRedeclare)
[error] 24-24: Shouldn't redeclare 'TOAST_EMITTER_KEY'. Consider to delete it or rename it.
'TOAST_EMITTER_KEY' is defined here:
(lint/suspicious/noRedeclare)
[error] 25-25: Shouldn't redeclare 'PopupAppearance'. Consider to delete it or rename it.
'PopupAppearance' is defined here:
(lint/suspicious/noRedeclare)
[error] 26-26: Shouldn't redeclare 'PopupContent'. Consider to delete it or rename it.
'PopupContent' is defined here:
(lint/suspicious/noRedeclare)
[error] 31-31: Shouldn't redeclare 'navigate'. Consider to delete it or rename it.
'navigate' is defined here:
(lint/suspicious/noRedeclare)
[error] 32-32: Shouldn't redeclare 'location'. Consider to delete it or rename it.
'location' is defined here:
(lint/suspicious/noRedeclare)
[error] 35-35: Shouldn't redeclare 'activeButton'. Consider to delete it or rename it.
'activeButton' is defined here:
(lint/suspicious/noRedeclare)
[error] 35-35: Shouldn't redeclare 'setActiveButton'. Consider to delete it or rename it.
'setActiveButton' is defined here:
(lint/suspicious/noRedeclare)
[error] 42-42: Shouldn't redeclare 'headerBackgroundColor'. Consider to delete it or rename it.
'headerBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 42-42: Shouldn't redeclare 'setHeaderBackgroundColor'. Consider to delete it or rename it.
'setHeaderBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 43-43: Shouldn't redeclare 'headerColor'. Consider to delete it or rename it.
'headerColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 43-43: Shouldn't redeclare 'setHeaderColor'. Consider to delete it or rename it.
'setHeaderColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 44-44: Shouldn't redeclare 'textColor'. Consider to delete it or rename it.
'textColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 44-44: Shouldn't redeclare 'setTextColor'. Consider to delete it or rename it.
'setTextColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 45-45: Shouldn't redeclare 'buttonBackgroundColor'. Consider to delete it or rename it.
'buttonBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 45-45: Shouldn't redeclare 'setButtonBackgroundColor'. Consider to delete it or rename it.
'setButtonBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 46-46: Shouldn't redeclare 'buttonTextColor'. Consider to delete it or rename it.
'buttonTextColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 46-46: Shouldn't redeclare 'setButtonTextColor'. Consider to delete it or rename it.
'setButtonTextColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 50-50: Shouldn't redeclare 'header'. Consider to delete it or rename it.
'header' is defined here:
(lint/suspicious/noRedeclare)
[error] 50-50: Shouldn't redeclare 'setHeader'. Consider to delete it or rename it.
'setHeader' is defined here:
(lint/suspicious/noRedeclare)
[error] 51-51: Shouldn't redeclare 'content'. Consider to delete it or rename it.
'content' is defined here:
(lint/suspicious/noRedeclare)
[error] 51-51: Shouldn't redeclare 'setContent'. Consider to delete it or rename it.
'setContent' is defined here:
(lint/suspicious/noRedeclare)
[error] 60-60: Shouldn't redeclare 'actionButtonUrl'. Consider to delete it or rename it.
'actionButtonUrl' is defined here:
(lint/suspicious/noRedeclare)
[error] 60-60: Shouldn't redeclare 'setActionButtonUrl'. Consider to delete it or rename it.
'setActionButtonUrl' is defined here:
(lint/suspicious/noRedeclare)
[error] 61-61: Shouldn't redeclare 'url'. Consider to delete it or rename it.
'url' is defined here:
(lint/suspicious/noRedeclare)
[error] 61-61: Shouldn't redeclare 'setUrl'. Consider to delete it or rename it.
'setUrl' is defined here:
(lint/suspicious/noRedeclare)
[error] 62-62: Shouldn't redeclare 'actionButtonText'. Consider to delete it or rename it.
'actionButtonText' is defined here:
(lint/suspicious/noRedeclare)
[error] 62-62: Shouldn't redeclare 'setActionButtonText'. Consider to delete it or rename it.
'setActionButtonText' is defined here:
(lint/suspicious/noRedeclare)
[error] 65-65: Shouldn't redeclare 'buttonAction'. Consider to delete it or rename it.
'buttonAction' is defined here:
(lint/suspicious/noRedeclare)
[error] 65-65: Shouldn't redeclare 'setButtonAction'. Consider to delete it or rename it.
'setButtonAction' is defined here:
(lint/suspicious/noRedeclare)
[error] 66-66: Shouldn't redeclare 'popupSize'. Consider to delete it or rename it.
'popupSize' is defined here:
(lint/suspicious/noRedeclare)
[error] 66-66: Shouldn't redeclare 'setPopupSize'. Consider to delete it or rename it.
'setPopupSize' is defined here:
(lint/suspicious/noRedeclare)
[error] 218-218: Shouldn't redeclare 'handleButtonClick'. Consider to delete it or rename it.
'handleButtonClick' is defined here:
(lint/suspicious/noRedeclare)
[error] 141-143: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
backend/src/controllers/popup.controller.js
[error] 30-32: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 42-44: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 72-72: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 75-75: Shouldn't redeclare 'colorCheck'. Consider to delete it or rename it.
'colorCheck' is defined here:
(lint/suspicious/noRedeclare)
[error] 89-89: expected , but instead found err
Remove err
(parse)
[error] 110-112: This code is unreachable
(lint/correctness/noUnreachable)
[error] 122-122: expected , but instead found err
Remove err
(parse)
[error] 147-149: This code is unreachable
(lint/correctness/noUnreachable)
[error] 202-202: expected , but instead found err
Remove err
(parse)
[error] 208-208: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 208-208: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 216-216: expected , but instead found err
Remove err
(parse)
[error] 188-188: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 191-191: Shouldn't redeclare 'colorCheck'. Consider to delete it or rename it.
'colorCheck' is defined here:
(lint/suspicious/noRedeclare)
[error] 231-231: expected , but instead found err
Remove err
(parse)
[error] 254-254: Expected a statement but instead found 'catch (err)'.
Expected a statement here.
(parse)
[error] 258-258: expected , but instead found err
Remove err
(parse)
[error] 250-250: This code is unreachable
(lint/correctness/noUnreachable)
[error] 280-280: Expected a statement but instead found 'catch (error)'.
Expected a statement here.
(parse)
[error] 288-288: Expected a catch clause but instead found '}'.
Expected a catch clause here.
(parse)
[error] 292-292: expected } but instead the file ends
the file ends here
(parse)
[error] 271-279: This code is unreachable
(lint/correctness/noUnreachable)
backend/src/controllers/banner.controller.js
[error] 27-29: This code is unreachable
(lint/correctness/noUnreachable)
[error] 66-66: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 69-69: Shouldn't redeclare 'colorCheck'. Consider to delete it or rename it.
'colorCheck' is defined here:
(lint/suspicious/noRedeclare)
[error] 83-83: expected , but instead found err
Remove err
(parse)
[error] 103-105: This code is unreachable
(lint/correctness/noUnreachable)
[error] 115-115: expected , but instead found err
Remove err
(parse)
[error] 137-139: This code is unreachable
(lint/correctness/noUnreachable)
[error] 186-186: expected , but instead found err
Remove err
(parse)
[error] 172-172: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 175-175: Shouldn't redeclare 'colorCheck'. Consider to delete it or rename it.
'colorCheck' is defined here:
(lint/suspicious/noRedeclare)
[error] 200-200: expected , but instead found err
Remove err
(parse)
[error] 215-215: expected , but instead found "GET_BANNERS_ERROR"
Remove "GET_BANNERS_ERROR"
(parse)
[error] 241-241: expected , but instead found err
Remove err
(parse)
[error] 263-263: Expected a statement but instead found 'catch (error)'.
Expected a statement here.
(parse)
[error] 271-271: Expected a catch clause but instead found '}'.
Expected a catch clause here.
(parse)
[error] 254-262: This code is unreachable
(lint/correctness/noUnreachable)
[error] 246-271: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx
[error] 135-136: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 146-146: expected } but instead the file ends
the file ends here
(parse)
[error] 46-46: Shouldn't redeclare 'handleActionChange'. Consider to delete it or rename it.
'handleActionChange' is defined here:
(lint/suspicious/noRedeclare)
[error] 53-53: Shouldn't redeclare 'handlePositionChange'. Consider to delete it or rename it.
'handlePositionChange' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (7)
frontend/src/scenes/popup/CreatePopupPage.jsx (1)
150-157: Enhance URL validation with HTTPS requirement
The current validation is too permissive. We should enforce HTTPS for security.
backend/src/controllers/banner.controller.js (3)
48-61: Yo, these URL validations are straight fire! 🔥
The new validation logic for actionUrl and url is clean and follows good error handling practices.
154-168: Clean URL validation implementation! 💯
The validation for both actionUrl and url is well-structured and handles errors appropriately.
264-268: 🛠️ Refactor suggestion
Let's standardize this error handling! 🎯
- const { payload, statusCode } = internalServerError(
+ const { statusCode, payload } = internalServerError(
"GET_BANNER_BY_URL_ERROR",
error.message
);Likely invalid or redundant comment.
backend/config/settings.js (1)
26-29: Yo! These permission changes look solid! 🔒
The new permissions serverUrlAndApiKey and helpers are properly restricted to admin role, maintaining security. Just need to verify these are used consistently.
✅ Verification successful
Yo dawg, these permission checks are straight fire! 🔥
The new permissions are consistently implemented across the codebase:
serverUrlAndApiKeypermission is properly guarded in team routes:- GET
/get-configendpoint - PUT
/set-configendpoint
- GET
helperspermission is correctly secured in helper routes:- POST
/add_helperendpoint - PUT
/edit_helper/:idendpoint - DELETE
/delete_helper/:idendpoint
- POST
The implementation is clean, no spaghetti code here - just proper permission checks using accessGuard middleware. Mom would be proud! 🍝
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of new permissions across the codebase
# Check for serverUrlAndApiKey permission usage
rg -l "serverUrlAndApiKey" --type js
# Check for helpers permission usage
rg -l "teamPermissions\.helpers" --type js
Length of output: 182
Script:
#!/bin/bash
# Let's check how these permissions are actually used in the routes
# Check serverUrlAndApiKey usage in team routes
rg "serverUrlAndApiKey" backend/src/routes/team.routes.js -B 2 -A 2
# Check helpers permission usage in helper routes
rg "teamPermissions\.helpers" backend/src/routes/helperLink.routes.js -B 2 -A 2
# Let's also verify if there are any other permission checks we might have missed
rg "checkPermission.*(?:serverUrlAndApiKey|helpers)" --type js
Length of output: 1544
backend/src/controllers/helperLink.controller.js (2)
188-199: Clean implementation, follows established patterns! 🍝
The new method maintains consistency with existing patterns in the codebase:
- Proper error handling ✓
- Async/await usage ✓
- Response structure ✓
- Service layer integration ✓
190-191: Verify handling of edge cases
Mom's spaghetti moment: Let's make sure we handle these edge cases gracefully:
- Empty result set
- Large number of helpers/links
- Deeply nested link structures
| }) => { | ||
| const handleSetUrl = (event) => { | ||
| setUrl(event.target.value); | ||
| }; | ||
|
|
||
| const handleSetActionUrl = (event) => { | ||
| setActionUrl(event.target.value); | ||
| }; | ||
|
|
||
| const handleActionChange = (newAction) => { | ||
| setButtonAction(newAction); | ||
| }; | ||
| const handleActionChange = (newAction) => { | ||
| setButtonAction(newAction); | ||
| }; | ||
|
|
||
| const handlePositionChange = (newPosition) => { | ||
| setIsTopPosition(newPosition); | ||
| }; | ||
| const handlePositionChange = (newPosition) => { | ||
| setIsTopPosition(newPosition); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className={styles.container}> | ||
| <h2>Action</h2> | ||
| <DropdownList | ||
| actions={["No action", "Open URL", "Open URL in a new tab"]} | ||
| onActionChange={handleActionChange} | ||
| selectedActionString={buttonAction} | ||
| /> | ||
| <h2>Position</h2> | ||
| <div className={styles.radioContent}> | ||
| <RadioButton | ||
| label='Top (centered)' | ||
| checked={isTopPosition} | ||
| onChange={() => handlePositionChange(true)} | ||
| /> | ||
| </div> | ||
| <div className={styles.radioContent}> | ||
| <RadioButton | ||
| label='Bottom (centered)' | ||
| checked={!isTopPosition} | ||
| onChange={() => handlePositionChange(false)} | ||
| /> | ||
| </div> | ||
| return ( | ||
| <div className={styles.container}> | ||
| <h2>Action</h2> | ||
| <DropdownList | ||
| actions={["No action", "Open URL", "Open URL in a new tab"]} | ||
| onActionChange={handleActionChange} | ||
| selectedActionString={buttonAction} | ||
| /> | ||
| <h2>Position</h2> | ||
| <div className={styles.radioContent}> | ||
| <RadioButton | ||
| label='Top (centered)' | ||
| checked={isTopPosition} | ||
| onChange={() => handlePositionChange(true)} | ||
| /> | ||
| </div> | ||
| <div className={styles.radioContent}> | ||
| <RadioButton | ||
| label='Bottom (centered)' | ||
| checked={!isTopPosition} | ||
| onChange={() => handlePositionChange(false)} | ||
| /> | ||
| </div> | ||
|
|
||
| <h2 style={{ marginBottom: 0, marginTop: "1.2rem" }}>URL</h2> | ||
| <CustomTextField | ||
| TextFieldWidth='241px' | ||
| value={url} | ||
| onChange={handleSetUrl} | ||
| /> | ||
|
|
||
| <h2 style={{ marginBottom: 0, marginTop: "1.2rem" }}>Action URL</h2> | ||
| <CustomTextField | ||
| TextFieldWidth='241px' | ||
| value={actionUrl} | ||
| onChange={handleSetActionUrl} | ||
| /> | ||
| </div> | ||
| ); | ||
| <h2 style={{ marginBottom: 0, marginTop: "1.2rem" }}>URL</h2> | ||
| <CustomTextField | ||
| TextFieldWidth='241px' | ||
| value={url} | ||
| onChange={handleSetUrl} | ||
| /> | ||
|
|
||
| <h2 style={{ marginBottom: 0, marginTop: "1.2rem" }}>Action URL</h2> | ||
| <CustomTextField | ||
| TextFieldWidth='241px' | ||
| value={actionUrl} | ||
| onChange={handleSetActionUrl} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Yo! There's spaghetti in this code! 🍝
The component definition and its contents are duplicated multiple times in the file. This needs to be cleaned up ASAP!
Remove the duplicate declarations:
- const BannerLeftContent = ({ // Remove duplicate starting at line 25
- setIsTopPosition,
- url,
- ...
- }) => {
- // Remove duplicate handlers and JSX
- };Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Shouldn't redeclare 'handleActionChange'. Consider to delete it or rename it.
'handleActionChange' is defined here:
(lint/suspicious/noRedeclare)
[error] 53-53: Shouldn't redeclare 'handlePositionChange'. Consider to delete it or rename it.
'handlePositionChange' is defined here:
(lint/suspicious/noRedeclare)
| const [header, setHeader] = useState(''); | ||
| const [content, setContent] = useState(''); | ||
|
|
||
| const [actionButtonUrl, setActionButtonUrl] = useState("https://"); | ||
| const [actionButtonText, setActionButtonText] = useState("Take me to subscription page"); | ||
| const [buttonAction, setButtonAction] = useState('No action'); | ||
| const [popupSize, setPopupSize] = useState('Small'); | ||
|
|
||
| const stateList = [ | ||
| { stateName: 'Header Background Color', state: headerBackgroundColor, setState: setHeaderBackgroundColor }, | ||
| { stateName: 'Header Color', state: headerColor, setState: setHeaderColor }, | ||
| { stateName: 'Text Color', state: textColor, setState: setTextColor }, | ||
| { stateName: 'Button Background Color', state: buttonBackgroundColor, setState: setButtonBackgroundColor }, | ||
| { stateName: 'Button Text Color', state: buttonTextColor, setState: setButtonTextColor }, | ||
| ]; | ||
|
|
||
| useEffect(() => { | ||
| if (location.state?.isEdit) { | ||
| const fetchPopupData = async () => { | ||
| try { | ||
| const popupData = await getPopupById(location.state.id); | ||
|
|
||
| // Update the state with the fetched data | ||
| setHeaderBackgroundColor(popupData.headerBackgroundColor || '#F8F9F8'); | ||
| setHeaderColor(popupData.headerColor || '#101828'); | ||
| setTextColor(popupData.textColor || '#344054'); | ||
| setButtonBackgroundColor(popupData.buttonBackgroundColor || '#7F56D9'); | ||
| setButtonTextColor(popupData.buttonTextColor || '#FFFFFF'); | ||
| setHeader(popupData.header || ''); | ||
| setContent(popupData.content || ''); | ||
| setActionButtonUrl(popupData.url || 'https://'); | ||
| setActionButtonText(popupData.actionButtonText || 'Take me to subscription page'); | ||
| setButtonAction(popupData.closeButtonAction || 'No action'); | ||
| setPopupSize(popupData.popupSize || 'Small'); | ||
| } catch (error) { | ||
| emitToastError(error); | ||
| } | ||
| }; | ||
|
|
||
| fetchPopupData(); | ||
| } | ||
| }, [location.state]); | ||
|
|
||
| const onSave = async () => { | ||
| const popupData = { | ||
| popupSize: popupSize.toLowerCase(), | ||
| url: actionButtonUrl, | ||
| actionButtonText: actionButtonText, | ||
| headerBackgroundColor: headerBackgroundColor, | ||
| headerColor: headerColor, | ||
| textColor: textColor, | ||
| buttonBackgroundColor: buttonBackgroundColor, | ||
| buttonTextColor: buttonTextColor, | ||
| closeButtonAction: buttonAction.toLowerCase(), | ||
| header: header, | ||
| content: content | ||
| }; | ||
| try { | ||
| const response = location.state?.isEdit | ||
| ? await editPopup(location.state?.id, popupData) | ||
| : await addPopup(popupData); | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
|
|
||
| const [activeButton, setActiveButton] = useState(0); | ||
| const [activeButton, setActiveButton] = useState(0); | ||
|
|
||
| const [headerBackgroundColor, setHeaderBackgroundColor] = useState("#F8F9F8"); | ||
| const [headerColor, setHeaderColor] = useState("#101828"); | ||
| const [textColor, setTextColor] = useState("#344054"); | ||
| const [buttonBackgroundColor, setButtonBackgroundColor] = useState("#7F56D9"); | ||
| const [buttonTextColor, setButtonTextColor] = useState("#FFFFFF"); | ||
| const [headerBackgroundColor, setHeaderBackgroundColor] = useState("#F8F9F8"); | ||
| const [headerColor, setHeaderColor] = useState("#101828"); | ||
| const [textColor, setTextColor] = useState("#344054"); | ||
| const [buttonBackgroundColor, setButtonBackgroundColor] = useState("#7F56D9"); | ||
| const [buttonTextColor, setButtonTextColor] = useState("#FFFFFF"); | ||
|
|
||
| const [header, setHeader] = useState(""); | ||
| const [content, setContent] = useState(""); | ||
| const [header, setHeader] = useState(""); | ||
| const [content, setContent] = useState(""); | ||
|
|
||
| const toastMessage = location.state?.isEdit ? 'You edited this popup' : 'New popup Saved' | ||
| const [actionButtonUrl, setActionButtonUrl] = useState("https://"); | ||
| const [url, setUrl] = useState("https://"); | ||
| const [actionButtonText, setActionButtonText] = useState( | ||
| "Take me to subscription page" | ||
| ); | ||
| const [buttonAction, setButtonAction] = useState("No action"); | ||
| const [popupSize, setPopupSize] = useState("Small"); | ||
| const [actionButtonUrl, setActionButtonUrl] = useState("https://"); | ||
| const [url, setUrl] = useState("https://"); | ||
| const [actionButtonText, setActionButtonText] = useState( | ||
| "Take me to subscription page" | ||
| ); | ||
| const [buttonAction, setButtonAction] = useState("No action"); | ||
| const [popupSize, setPopupSize] = useState("Small"); |
There was a problem hiding this comment.
Critical: Remove duplicate imports and state declarations!
Yo! There's a merge conflict that wasn't resolved properly, leading to duplicate code. This needs immediate attention as it will cause runtime issues.
Clean up the duplicates by removing lines 14-26, 31-32, 35, 42-46, 50-51, and 60-66.
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Shouldn't redeclare 'RichTextEditor'. Consider to delete it or rename it.
'RichTextEditor' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'React'. Consider to delete it or rename it.
'React' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'useEffect'. Consider to delete it or rename it.
'useEffect' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'useState'. Consider to delete it or rename it.
'useState' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'useLocation'. Consider to delete it or rename it.
'useLocation' is defined here:
(lint/suspicious/noRedeclare)
[error] 16-16: Shouldn't redeclare 'useNavigate'. Consider to delete it or rename it.
'useNavigate' is defined here:
(lint/suspicious/noRedeclare)
[error] 18-18: Shouldn't redeclare 'addPopup'. Consider to delete it or rename it.
'addPopup' is defined here:
(lint/suspicious/noRedeclare)
[error] 19-19: Shouldn't redeclare 'editPopup'. Consider to delete it or rename it.
'editPopup' is defined here:
(lint/suspicious/noRedeclare)
[error] 20-20: Shouldn't redeclare 'getPopupById'. Consider to delete it or rename it.
'getPopupById' is defined here:
(lint/suspicious/noRedeclare)
[error] 22-22: Shouldn't redeclare 'GuideTemplate'. Consider to delete it or rename it.
'GuideTemplate' is defined here:
(lint/suspicious/noRedeclare)
[error] 23-23: Shouldn't redeclare 'emitToastError'. Consider to delete it or rename it.
'emitToastError' is defined here:
(lint/suspicious/noRedeclare)
[error] 24-24: Shouldn't redeclare 'toastEmitter'. Consider to delete it or rename it.
'toastEmitter' is defined here:
(lint/suspicious/noRedeclare)
[error] 24-24: Shouldn't redeclare 'TOAST_EMITTER_KEY'. Consider to delete it or rename it.
'TOAST_EMITTER_KEY' is defined here:
(lint/suspicious/noRedeclare)
[error] 25-25: Shouldn't redeclare 'PopupAppearance'. Consider to delete it or rename it.
'PopupAppearance' is defined here:
(lint/suspicious/noRedeclare)
[error] 26-26: Shouldn't redeclare 'PopupContent'. Consider to delete it or rename it.
'PopupContent' is defined here:
(lint/suspicious/noRedeclare)
[error] 31-31: Shouldn't redeclare 'navigate'. Consider to delete it or rename it.
'navigate' is defined here:
(lint/suspicious/noRedeclare)
[error] 32-32: Shouldn't redeclare 'location'. Consider to delete it or rename it.
'location' is defined here:
(lint/suspicious/noRedeclare)
[error] 35-35: Shouldn't redeclare 'activeButton'. Consider to delete it or rename it.
'activeButton' is defined here:
(lint/suspicious/noRedeclare)
[error] 35-35: Shouldn't redeclare 'setActiveButton'. Consider to delete it or rename it.
'setActiveButton' is defined here:
(lint/suspicious/noRedeclare)
[error] 42-42: Shouldn't redeclare 'headerBackgroundColor'. Consider to delete it or rename it.
'headerBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 42-42: Shouldn't redeclare 'setHeaderBackgroundColor'. Consider to delete it or rename it.
'setHeaderBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 43-43: Shouldn't redeclare 'headerColor'. Consider to delete it or rename it.
'headerColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 43-43: Shouldn't redeclare 'setHeaderColor'. Consider to delete it or rename it.
'setHeaderColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 44-44: Shouldn't redeclare 'textColor'. Consider to delete it or rename it.
'textColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 44-44: Shouldn't redeclare 'setTextColor'. Consider to delete it or rename it.
'setTextColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 45-45: Shouldn't redeclare 'buttonBackgroundColor'. Consider to delete it or rename it.
'buttonBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 45-45: Shouldn't redeclare 'setButtonBackgroundColor'. Consider to delete it or rename it.
'setButtonBackgroundColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 46-46: Shouldn't redeclare 'buttonTextColor'. Consider to delete it or rename it.
'buttonTextColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 46-46: Shouldn't redeclare 'setButtonTextColor'. Consider to delete it or rename it.
'setButtonTextColor' is defined here:
(lint/suspicious/noRedeclare)
[error] 50-50: Shouldn't redeclare 'header'. Consider to delete it or rename it.
'header' is defined here:
(lint/suspicious/noRedeclare)
[error] 50-50: Shouldn't redeclare 'setHeader'. Consider to delete it or rename it.
'setHeader' is defined here:
(lint/suspicious/noRedeclare)
[error] 51-51: Shouldn't redeclare 'content'. Consider to delete it or rename it.
'content' is defined here:
(lint/suspicious/noRedeclare)
[error] 51-51: Shouldn't redeclare 'setContent'. Consider to delete it or rename it.
'setContent' is defined here:
(lint/suspicious/noRedeclare)
[error] 60-60: Shouldn't redeclare 'actionButtonUrl'. Consider to delete it or rename it.
'actionButtonUrl' is defined here:
(lint/suspicious/noRedeclare)
[error] 60-60: Shouldn't redeclare 'setActionButtonUrl'. Consider to delete it or rename it.
'setActionButtonUrl' is defined here:
(lint/suspicious/noRedeclare)
[error] 61-61: Shouldn't redeclare 'url'. Consider to delete it or rename it.
'url' is defined here:
(lint/suspicious/noRedeclare)
[error] 61-61: Shouldn't redeclare 'setUrl'. Consider to delete it or rename it.
'setUrl' is defined here:
(lint/suspicious/noRedeclare)
[error] 62-62: Shouldn't redeclare 'actionButtonText'. Consider to delete it or rename it.
'actionButtonText' is defined here:
(lint/suspicious/noRedeclare)
[error] 62-62: Shouldn't redeclare 'setActionButtonText'. Consider to delete it or rename it.
'setActionButtonText' is defined here:
(lint/suspicious/noRedeclare)
[error] 65-65: Shouldn't redeclare 'buttonAction'. Consider to delete it or rename it.
'buttonAction' is defined here:
(lint/suspicious/noRedeclare)
[error] 65-65: Shouldn't redeclare 'setButtonAction'. Consider to delete it or rename it.
'setButtonAction' is defined here:
(lint/suspicious/noRedeclare)
[error] 66-66: Shouldn't redeclare 'popupSize'. Consider to delete it or rename it.
'popupSize' is defined here:
(lint/suspicious/noRedeclare)
[error] 66-66: Shouldn't redeclare 'setPopupSize'. Consider to delete it or rename it.
'setPopupSize' is defined here:
(lint/suspicious/noRedeclare)
| return ( | ||
| <GuideTemplate | ||
| title={location.state?.isEdit ? "Edit Popup" : "New Popup"} | ||
| activeButton={activeButton} | ||
| handleButtonClick={handleButtonClick} | ||
| onSave={onSave} | ||
| rightContent={() => ( | ||
| <RichTextEditor | ||
| header={header} | ||
| content={content} | ||
| setHeader={setHeader} | ||
| setContent={setContent} | ||
| previewBtnText={actionButtonText} | ||
| headerBackgroundColor={headerBackgroundColor} | ||
| headerColor={headerColor} | ||
| textColor={textColor} | ||
| buttonBackgroundColor={buttonBackgroundColor} | ||
| buttonTextColor={buttonTextColor} | ||
| popupSize={popupSize} | ||
| sx={{ | ||
| width: "100%", | ||
| maxWidth: "700px", | ||
| marginLeft: "2.5rem", | ||
| marginTop: "1rem", | ||
| }} | ||
| /> | ||
| )} | ||
| leftContent={() => ( | ||
| <PopupContent | ||
| actionButtonUrl={actionButtonUrl} | ||
| setActionButtonText={setActionButtonText} | ||
| setActionButtonUrl={setActionButtonUrl} | ||
| actionButtonText={actionButtonText} | ||
| setButtonAction={setButtonAction} | ||
| buttonAction={buttonAction} | ||
| url={url} | ||
| setUrl={setUrl} | ||
| /> | ||
| )} | ||
| leftAppearance={() => ( | ||
| <PopupAppearance | ||
| data={stateList} | ||
| setPopupSize={setPopupSize} | ||
| popupSize={popupSize} | ||
| /> | ||
| )} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Remove unreachable duplicate return statement
Knees weak! There's an unreachable duplicate return statement that needs to be removed.
Remove the duplicate return statement from lines 270-317 as it will never be executed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 270-317: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
| const onSave = async () => { | ||
| if (actionButtonUrl && actionButtonUrl !== "https://") { | ||
| const urlError = validateUrl(actionButtonUrl); | ||
| if (urlError) { | ||
| emitToastError(urlError); | ||
| return; | ||
| } | ||
| } | ||
| if (url && url !== "https://") { | ||
| const urlError = validateUrl(url); | ||
| if (urlError) { | ||
| emitToastError(urlError); | ||
| return; | ||
| } | ||
| } | ||
| const popupData = { | ||
| popupSize: popupSize.toLowerCase(), | ||
| url, | ||
| actionUrl: actionButtonUrl, | ||
| actionButtonText, | ||
| headerBackgroundColor, | ||
| headerColor, | ||
| textColor, | ||
| buttonBackgroundColor, | ||
| buttonTextColor, | ||
| closeButtonAction: buttonAction.toLowerCase(), | ||
| header, | ||
| content, | ||
| }; | ||
| try { | ||
| const response = location.state?.isEdit | ||
| ? await editPopup(location.state?.id, popupData) | ||
| : await addPopup(popupData); | ||
|
|
||
| const toastMessage = location.state?.isEdit | ||
| ? "You edited this popup" | ||
| : "New popup Saved"; | ||
|
|
||
| toastEmitter.emit(TOAST_EMITTER_KEY, toastMessage); | ||
| navigate("/popup"); | ||
| } catch (error) { | ||
| const errorMessage = error.response?.data?.message | ||
| ? `Error: ${error.response.data.message}` | ||
| : "An unexpected error occurred. Please try again."; | ||
| toastEmitter.emit(TOAST_EMITTER_KEY, errorMessage); | ||
| toastEmitter.emit(TOAST_EMITTER_KEY, toastMessage); | ||
| navigate("/popup"); | ||
| } catch (error) { | ||
| const errorMessage = error.response?.data?.message | ||
| ? `Error: ${error.response.data.message}` | ||
| : "An unexpected error occurred. Please try again."; | ||
| toastEmitter.emit(TOAST_EMITTER_KEY, errorMessage); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor error handling to avoid duplication
Mom's spaghetti moment! The error handling code is duplicated. Let's clean this up!
Extract the error handling into a reusable function:
+ const handleError = (error) => {
+ const errorMessage = error.response?.data?.message
+ ? `Error: ${error.response.data.message}`
+ : "An unexpected error occurred. Please try again.";
+ toastEmitter.emit(TOAST_EMITTER_KEY, errorMessage);
+ };
const onSave = async () => {
// ... URL validation code ...
try {
const response = location.state?.isEdit
? await editPopup(location.state?.id, popupData)
: await addPopup(popupData);
const toastMessage = location.state?.isEdit
? "You edited this popup"
: "New popup Saved";
toastEmitter.emit(TOAST_EMITTER_KEY, toastMessage);
navigate("/popup");
} catch (error) {
- const errorMessage = error.response?.data?.message
- ? `Error: ${error.response.data.message}`
- : "An unexpected error occurred. Please try again.";
- toastEmitter.emit(TOAST_EMITTER_KEY, errorMessage);
+ handleError(error);
}
};Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 206-206: Expected a statement but instead found 'catch (error)'.
Expected a statement here.
(parse)
| return res.status(400).json({ | ||
| errors: [{ msg: "position and closeButtonAction are required" }], | ||
| }); | ||
| return res.status(400).json({ | ||
| errors: [{ msg: "position and closeButtonAction are required" }], | ||
| }); |
There was a problem hiding this comment.
These duplicate validations are heavy! Mom's spaghetti! 🍝
return res.status(400).json({
errors: [{ msg: "position and closeButtonAction are required" }],
});
- return res.status(400).json({
- errors: [{ msg: "position and closeButtonAction are required" }],
- });📝 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 res.status(400).json({ | |
| errors: [{ msg: "position and closeButtonAction are required" }], | |
| }); | |
| return res.status(400).json({ | |
| errors: [{ msg: "position and closeButtonAction are required" }], | |
| }); | |
| return res.status(400).json({ | |
| errors: [{ msg: "position and closeButtonAction are required" }], | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 137-139: This code is unreachable
(lint/correctness/noUnreachable)
backend/src/routes/guide.routes.js
Outdated
| @@ -4,6 +4,6 @@ const guideController = require("../controllers/guide.controller.js"); | |||
| const router = express.Router(); | |||
|
|
|||
| router.post("/get_guides_by_url", guideController.getGuidesByUrl); | |||
| // router.get("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl); | |||
| router.get("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl); | |||
There was a problem hiding this comment.
Switch the method to POST to keep the rhythm smooth
Since the controller relies on req.body, and to match the existing routes, it's best to change this route to a POST request. That way, we won't miss a beat.
Apply this diff to change the route method:
-router.get("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl);
+router.post("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl);📝 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.
| router.get("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl); | |
| router.post("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl); |
| async getCompleteBannerLogs(userId) { | ||
| try { | ||
| return await GuideLog.findAll({ | ||
| where: { | ||
| userId: userId, | ||
| completed: true, | ||
| guideType: GuideType.BANNER | ||
| }, | ||
| }); | ||
| } catch (err) { | ||
| throw new Error(`Failed to retrieve banner log for user ${userId}: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| async getCompletePopupLogs(userId) { | ||
| try { | ||
| return await GuideLog.findAll({ | ||
| where: { | ||
| userId: userId, | ||
| completed: true, | ||
| guideType: GuideType.POPUP | ||
| }, | ||
| }); | ||
| } catch (err) { | ||
| throw new Error(`Failed to retrieve popup log for user ${userId}: ${err.message}`); | ||
| } | ||
| } | ||
|
|
||
| async getCompleteHelperLogs(userId) { | ||
| try { | ||
| return await GuideLog.findAll({ | ||
| where: { | ||
| userId: userId, | ||
| completed: true, | ||
| guideType: GuideType.LINK | ||
| }, | ||
| }); | ||
| } catch (err) { | ||
| throw new Error(`Failed to retrieve helper link log for user ${userId}: ${err.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo! Let's DRY up these guide log methods, dawg!
These three methods share almost identical code structure, differing only in the guide type. We can refactor this into a single method to reduce code duplication.
Here's a fire 🔥 refactor suggestion:
- async getCompleteBannerLogs(userId) {
- try {
- return await GuideLog.findAll({
- where: {
- userId: userId,
- completed: true,
- guideType: GuideType.BANNER
- },
- });
- } catch (err) {
- throw new Error(`Failed to retrieve banner log for user ${userId}: ${err.message}`);
- }
- }
-
- async getCompletePopupLogs(userId) {
- try {
- return await GuideLog.findAll({
- where: {
- userId: userId,
- completed: true,
- guideType: GuideType.POPUP
- },
- });
- } catch (err) {
- throw new Error(`Failed to retrieve popup log for user ${userId}: ${err.message}`);
- }
- }
-
- async getCompleteHelperLogs(userId) {
- try {
- return await GuideLog.findAll({
- where: {
- userId: userId,
- completed: true,
- guideType: GuideType.LINK
- },
- });
- } catch (err) {
- throw new Error(`Failed to retrieve helper link log for user ${userId}: ${err.message}`);
- }
- }
+ async getCompleteLogsByType(userId, guideType) {
+ const typeNames = {
+ [GuideType.BANNER]: 'banner',
+ [GuideType.POPUP]: 'popup',
+ [GuideType.LINK]: 'helper link'
+ };
+ try {
+ return await GuideLog.findAll({
+ where: {
+ userId,
+ completed: true,
+ guideType
+ },
+ });
+ } catch (err) {
+ throw new Error(`Failed to retrieve ${typeNames[guideType]} log for user ${userId}: ${err.message}`);
+ }
+ }Then you can use it like:
async getCompleteBannerLogs(userId) {
return this.getCompleteLogsByType(userId, GuideType.BANNER);
}
async getCompletePopupLogs(userId) {
return this.getCompleteLogsByType(userId, GuideType.POPUP);
}
async getCompleteHelperLogs(userId) {
return this.getCompleteLogsByType(userId, GuideType.LINK);
}
backend/src/models/Popup.js
Outdated
| validateActionButton(value); | ||
| validateActionButton(value); |
There was a problem hiding this comment.
Yo dawg, I heard you like validation so you put validation in your validation! 🤔
Multiple validation functions are being called twice unnecessarily. Remove the duplicate calls to:
validateActionButtonvalidatePopupSizeWrappervalidateHexColor
isValidAction(value) {
- validateActionButton(value);
validateActionButton(value);
}Apply similar fixes to other duplicate validation calls.
Also applies to: 35-36, 59-60, 70-71, 81-82, 102-103
backend/src/models/Popup.js
Outdated
| } | ||
| } |
There was a problem hiding this comment.
Syntax error alert! Extra closing brace dropping like mom's spaghetti! 🍝
Remove the extra closing brace that's causing a syntax error.
{
tableName: "popup",
timestamps: false,
- }
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 139-139: expected , but instead found }
Remove }
(parse)
| return res.status(400).json({ | ||
| errors: [{ msg: "popupSize and closeButtonAction are required" }], | ||
| }); | ||
| return res.status(400).json({ | ||
| errors: [{ msg: "popupSize and closeButtonAction are required" }], | ||
| }); |
There was a problem hiding this comment.
Yo! These duplicate blocks are making me lose myself in the code! 🎤
Multiple code blocks are duplicated and creating unreachable code. The following patterns need to be fixed:
- Duplicate return statements
- Duplicate error checks
- Duplicate color checks
Remove all duplicate blocks. For example:
if (!popupSize || !closeButtonAction) {
return res.status(400).json({
errors: [{ msg: "popupSize and closeButtonAction are required" }],
});
- return res.status(400).json({
- errors: [{ msg: "popupSize and closeButtonAction are required" }],
- });
}Also applies to: 39-44, 71-78, 107-112, 144-149, 187-194, 249-250, 267-274
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-32: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
frontend/src/scenes/popup/CreatePopupPage.jsx (2)
Line range hint
61-61: Consider empty string defaults for URL fields!Yo! Having "https://" as default value could lead to validation confusion. If the user doesn't modify it, we'd be saving an invalid URL.
- const [url, setUrl] = useState("https://"); + const [url, setUrl] = useState("");
Line range hint
65-84: Standardize error handling and URL fallbacks!Mom's spaghetti moment! The error handling and URL fallbacks need cleanup:
- Error handling differs from
onSave- URL fallback should be empty string
- setUrl(popupData.url || "https://"); + setUrl(popupData.url || ""); - emitToastError(error); + const errorMessage = error.response?.data?.message + ? `Error: ${error.response.data.message}` + : "An unexpected error occurred. Please try again."; + toastEmitter.emit(TOAST_EMITTER_KEY, errorMessage);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx(1 hunks)frontend/src/scenes/popup/CreatePopupPage.jsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/scenes/banner/BannerPageComponents/BannerLeftContent/BannerLeftContent.jsx
🔇 Additional comments (1)
frontend/src/scenes/popup/CreatePopupPage.jsx (1)
Line range hint 154-203: LGTM! Component rendering changes are solid.
The UI updates correctly handle the new URL field.
| @@ -143,13 +145,13 @@ const CreatePopupPage = () => { | |||
| ? `Error: ${error.response.data.message}` | |||
| : "An unexpected error occurred. Please try again."; | |||
| toastEmitter.emit(TOAST_EMITTER_KEY, errorMessage); | |||
| } | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor URL validation logic!
Knees weak! The URL validation has duplicate code and might miss invalid URLs:
- Extract validation to a reusable function
- Remove "https://" special case
+ const validateUrls = (urls) => {
+ for (const [url, name] of urls) {
+ if (!url) continue;
+ try {
+ new URL(url);
+ } catch (err) {
+ emitToastError(`Invalid ${name} URL format`);
+ return false;
+ }
+ }
+ return true;
+ };
const onSave = async () => {
- if (actionButtonUrl && actionButtonUrl !== "https://") {
- const urlError = validateUrl(actionButtonUrl);
- if (urlError) {
- emitToastError(urlError);
- return;
- }
- }
- if (url && url !== "https://") {
- const urlError = validateUrl(url);
- if (urlError) {
- emitToastError(urlError);
- return;
- }
- }
+ const urlsToValidate = [
+ [actionButtonUrl, "action button"],
+ [url, "server"]
+ ];
+ if (!validateUrls(urlsToValidate)) return;Committable suggestion skipped: line range outside the PR's diff.
|
Also this brnach has 60 commits. It is possible to create a new branch from develop and move the changes you made about server url there? |
Sure, will do. |
I was thinking of same. I will close this PR, and raise a new one through a new branch |
Note: This PR is similar to #345, which I had to close due to some rebasing issues.
serverUrlandapiKeycolumns to theteamstable./set-configBE route to storeserverUrlandapiKey.