Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

20 creating popup js code #374

Merged
merged 14 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified .DS_Store
Binary file not shown.
6 changes: 4 additions & 2 deletions backend/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
FROM node:22-alpine3.20
FROM node:22-alpine3.21

RUN apk update && apk add bash && rm -rf /var/cache/apk/*

RUN apk update && apk add bash && rm -rf /var/cache/apk/*

WORKDIR /app

COPY package*.json ./

RUN npm install
RUN rm -rf package-lock.json && npm install

COPY . .

Expand Down
56 changes: 30 additions & 26 deletions backend/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"helmet": "^7.1.0",
"jsonwebtoken": "^9.0.2",
"nodemailer": "^6.9.15",
"pg": "^8.11.5",
"pg": "^8.13.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Yo dawg, that pg version ain't right! 🍝

The specified version ^8.13.1 doesn't exist in the npm registry. The latest version in the 8.x series is 8.11.3.

Apply this fix:

-    "pg": "^8.13.1",
+    "pg": "8.11.3",

Also consider removing the caret (^) to prevent unexpected minor version updates that could break your spaghetti code.

📝 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.

Suggested change
"pg": "^8.13.1",
"pg": "8.11.3",

"sequelize": "^6.37.3"
},
"devDependencies": {
Expand Down
Empty file.
52 changes: 52 additions & 0 deletions backend/public/scripts/popupRender.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const showPopup = (popupData) => {
if (!popupData || popupData.length === 0) {
console.warn('No popup data available');
return;
}

popupData.forEach(popup => {
// Create popup container
const popupContainer = document.createElement('div');
Object.assign(popupContainer.style, {
position: 'fixed',
bottom: '20px',
right: '20px',
backgroundColor: popup.headerBg || '#FFFFFF',
padding: '20px',
border: '1px solid #000',
zIndex: 1000,
});

// Create header
const header = document.createElement('h2');
header.textContent = popup.headerText;
header.style.color = popup.headerTextColor || '#000';
popupContainer.appendChild(header);

// Create content
const content = document.createElement('div');
content.innerHTML = popup.contentHtml;
Object.assign(content.style, {
color: popup.fontColor || '#000',
fontSize: popup.font || '14px',
});
popupContainer.appendChild(content);

// Create action button
const actionButton = document.createElement('button');
actionButton.textContent = popup.actionButtonText || 'Close';
Object.assign(actionButton.style, {
backgroundColor: popup.actionButtonColor || '#CCC',
});
actionButton.addEventListener('click', () => {
document.body.removeChild(popupContainer); // Remove popup when button is clicked
});
popupContainer.appendChild(actionButton);

// Append the popup to the document body
document.body.appendChild(popupContainer);
});
};

export default showPopup;
Comment on lines +1 to +51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mom's spaghetti alert! 🍝 Security and accessibility concerns!

  1. XSS vulnerability through raw innerHTML usage
  2. Missing accessibility features
  3. Code duplication with jsAgent/popupRender.js

Here's a safer implementation with accessibility support:

+ import DOMPurify from 'dompurify';

 const showPopup = (popupData) => {
   if (!popupData || popupData.length === 0) {
     console.warn('No popup data available');
     return;
   }

   popupData.forEach(popup => {
     const popupContainer = document.createElement('div');
+    popupContainer.setAttribute('role', 'dialog');
+    popupContainer.setAttribute('aria-modal', 'true');
+    popupContainer.setAttribute('aria-labelledby', 'popup-header');
     Object.assign(popupContainer.style, {
       position: 'fixed',
       bottom: '20px',
       right: '20px',
       backgroundColor: popup.headerBg || '#FFFFFF',
       padding: '20px',
       border: '1px solid #000',
       zIndex: 1000,
     });

     const header = document.createElement('h2');
+    header.id = 'popup-header';
     header.textContent = popup.headerText;
     header.style.color = popup.headerTextColor || '#000';
     popupContainer.appendChild(header);

     const content = document.createElement('div');
-    content.innerHTML = popup.contentHtml;
+    content.innerHTML = DOMPurify.sanitize(popup.contentHtml);
     Object.assign(content.style, {
       color: popup.fontColor || '#000',
       fontSize: popup.font || '14px',
     });
     popupContainer.appendChild(content);

     const actionButton = document.createElement('button');
     actionButton.textContent = popup.actionButtonText || 'Close';
+    actionButton.setAttribute('aria-label', 'Close popup');
     Object.assign(actionButton.style, {
       backgroundColor: popup.actionButtonColor || '#CCC',
     });
-    actionButton.addEventListener('click', () => {
+    const closePopup = () => {
       document.body.removeChild(popupContainer);
-    });
+    };
+    actionButton.addEventListener('click', closePopup);
+    // Add keyboard support
+    popupContainer.addEventListener('keydown', (e) => {
+      if (e.key === 'Escape') closePopup();
+    });
     popupContainer.appendChild(actionButton);

     document.body.appendChild(popupContainer);
   });
 };

Consider creating a shared component library to avoid code duplication between backend/public/scripts and jsAgent. This will help maintain consistency and reduce maintenance overhead.

📝 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.

Suggested change
const showPopup = (popupData) => {
if (!popupData || popupData.length === 0) {
console.warn('No popup data available');
return;
}
popupData.forEach(popup => {
// Create popup container
const popupContainer = document.createElement('div');
Object.assign(popupContainer.style, {
position: 'fixed',
bottom: '20px',
right: '20px',
backgroundColor: popup.headerBg || '#FFFFFF',
padding: '20px',
border: '1px solid #000',
zIndex: 1000,
});
// Create header
const header = document.createElement('h2');
header.textContent = popup.headerText;
header.style.color = popup.headerTextColor || '#000';
popupContainer.appendChild(header);
// Create content
const content = document.createElement('div');
content.innerHTML = popup.contentHtml;
Object.assign(content.style, {
color: popup.fontColor || '#000',
fontSize: popup.font || '14px',
});
popupContainer.appendChild(content);
// Create action button
const actionButton = document.createElement('button');
actionButton.textContent = popup.actionButtonText || 'Close';
Object.assign(actionButton.style, {
backgroundColor: popup.actionButtonColor || '#CCC',
});
actionButton.addEventListener('click', () => {
document.body.removeChild(popupContainer); // Remove popup when button is clicked
});
popupContainer.appendChild(actionButton);
// Append the popup to the document body
document.body.appendChild(popupContainer);
});
};
export default showPopup;
import DOMPurify from 'dompurify';
const showPopup = (popupData) => {
if (!popupData || popupData.length === 0) {
console.warn('No popup data available');
return;
}
popupData.forEach(popup => {
const popupContainer = document.createElement('div');
popupContainer.setAttribute('role', 'dialog');
popupContainer.setAttribute('aria-modal', 'true');
popupContainer.setAttribute('aria-labelledby', 'popup-header');
Object.assign(popupContainer.style, {
position: 'fixed',
bottom: '20px',
right: '20px',
backgroundColor: popup.headerBg || '#FFFFFF',
padding: '20px',
border: '1px solid #000',
zIndex: 1000,
});
const header = document.createElement('h2');
header.id = 'popup-header';
header.textContent = popup.headerText;
header.style.color = popup.headerTextColor || '#000';
popupContainer.appendChild(header);
const content = document.createElement('div');
content.innerHTML = DOMPurify.sanitize(popup.contentHtml);
Object.assign(content.style, {
color: popup.fontColor || '#000',
fontSize: popup.font || '14px',
});
popupContainer.appendChild(content);
const actionButton = document.createElement('button');
actionButton.textContent = popup.actionButtonText || 'Close';
actionButton.setAttribute('aria-label', 'Close popup');
Object.assign(actionButton.style, {
backgroundColor: popup.actionButtonColor || '#CCC',
});
const closePopup = () => {
document.body.removeChild(popupContainer);
};
actionButton.addEventListener('click', closePopup);
// Add keyboard support
popupContainer.addEventListener('keydown', (e) => {
if (e.key === 'Escape') closePopup();
});
popupContainer.appendChild(actionButton);
document.body.appendChild(popupContainer);
});
};
export default showPopup;


30 changes: 30 additions & 0 deletions backend/public/snippets/popupSnippet.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<!-- Client-side HTML/JS Snippet to be integrated into their website -->
<script>
(function() {
const apiId = 'YOUR_CLIENT_API_ID';
const apiUrl = `https://onboarding-demo.bluewavelabs.ca/api/onboard`;
Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Palms are sweaty: Don't hardcode those URLs! 🍝

The API URL and client ID should be configurable parameters.

-      const apiId = 'YOUR_CLIENT_API_ID';
-      const apiUrl = `https://onboarding-demo.bluewavelabs.ca/api/onboard`;
+      const apiId = window.ONBOARDING_API_ID;
+      const apiUrl = window.ONBOARDING_API_URL || 'https://onboarding-demo.bluewavelabs.ca/api/onboard';
📝 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.

Suggested change
const apiId = 'YOUR_CLIENT_API_ID';
const apiUrl = `https://onboarding-demo.bluewavelabs.ca/api/onboard`;
const apiId = window.ONBOARDING_API_ID;
const apiUrl = window.ONBOARDING_API_URL || 'https://onboarding-demo.bluewavelabs.ca/api/onboard';


fetch(apiUrl, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({ userId: apiId })
})
.then(response => response.json())
.then(data => {
const script = document.createElement('script');
script.src = `https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`;
script.type = 'module';
script.onload = () => {
import(`https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`)
.then(module => {
module.default(data.popupData);
});
};
document.head.appendChild(script);
Comment on lines +16 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

He's nervous: You're loading the script twice! 🍝

The script is being loaded twice - once via createElement and once via import. This is inefficient and could cause race conditions.

-        const script = document.createElement('script');
-        script.src = `https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`;
-        script.type = 'module';
-        script.onload = () => {
-          import(`https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`)
-            .then(module => {
-              module.default(data.popupData);
-            });
-        };
-        document.head.appendChild(script);
+        import(`https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`)
+          .then(module => {
+            module.default(data.popupData);
+          })
+          .catch(error => {
+            console.warn('Failed to load popup renderer');
+          });
📝 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.

Suggested change
const script = document.createElement('script');
script.src = `https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`;
script.type = 'module';
script.onload = () => {
import(`https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`)
.then(module => {
module.default(data.popupData);
});
};
document.head.appendChild(script);
import(`https://onboarding-demo.bluewavelabs.ca/api/scripts/popupRenderer.js?apiId=${apiId}`)
.then(module => {
module.default(data.popupData);
})
.catch(error => {
console.warn('Failed to load popup renderer');
});

})
.catch(error => console.error('Error fetching onboarding data:', error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

But on the surface he looks calm and ready: Secure that error handling! 🍝

Don't expose sensitive error information to the console.

-      .catch(error => console.error('Error fetching onboarding data:', error));
+      .catch(() => console.warn('Failed to initialize onboarding'));
📝 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.

Suggested change
.catch(error => console.error('Error fetching onboarding data:', error));
.catch(() => console.warn('Failed to initialize onboarding'));

})();
</script>

12 changes: 12 additions & 0 deletions backend/src/controllers/onboard/bannerData.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const bannerService = require("../service/banner.service.js");
const getBannerData = async (req, res) => {
try {
const { userId } = req.body;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Yo dawg, we need to validate that userId before we wreck ourselves! 🔍

Add input validation to ensure userId is present and in the correct format before proceeding.

- const { userId } = req.body;
+ const { userId } = req.body;
+ if (!userId || typeof userId !== 'string') {
+   return res.status(400).json({ error: 'Invalid or missing userId' });
+ }
📝 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.

Suggested change
const { userId } = req.body;
const { userId } = req.body;
if (!userId || typeof userId !== 'string') {
return res.status(400).json({ error: 'Invalid or missing userId' });
}

const bannerData = await bannerService.getBannerData(userId);
res.status(200).json(bannerData);
} catch (error) {
res.status(500).json({ error: error.message });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Careful with them error messages, they're spicier than mom's spaghetti! 🍝

The current error handling might expose sensitive information. Consider using a standardized error response format.

- res.status(500).json({ error: error.message });
+ res.status(500).json({ error: 'An error occurred while fetching banner data' });
+ console.error('Banner data error:', error);
📝 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.

Suggested change
res.status(500).json({ error: error.message });
res.status(500).json({ error: 'An error occurred while fetching banner data' });
console.error('Banner data error:', error);

}
};

module.exports = { getBannerData };
7 changes: 7 additions & 0 deletions backend/src/controllers/onboard/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module.exports = {
// bannerData: require('./bannerData.controller'),
popupData: require('./popupData.controller'),
// tourData: require('./tourData.controller'),
// hintData: require('./hintData.controller')
};

13 changes: 13 additions & 0 deletions backend/src/controllers/onboard/popupData.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const popupService = require('../../service/popup.service');

const getPopupData = async (req, res) => {
try {
const { userId } = req.body;
const popupData = await popupService.getPopups(userId);
res.status(200).json(popupData);
} catch (error) {
res.status(500).json({ error: error.message });
}
Comment on lines +9 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Let's keep our errors under wraps

Returning detailed error messages to the client can reveal sensitive information. Send a generic error message instead and log the error internally.

Apply this diff:

- res.status(500).json({ error: error.message });
+ console.error(error);
+ res.status(500).json({ error: 'Internal server error' });
📝 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.

Suggested change
res.status(500).json({ error: error.message });
}
console.error(error);
res.status(500).json({ error: 'Internal server error' });
}

};

module.exports = { getPopupData };
13 changes: 13 additions & 0 deletions backend/src/controllers/onboard/tourData.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const tourService = require('../service/tour.service');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Yo! The import path needs fixing, dawg! 🍝

The service import path doesn't match the project structure. From the onboard directory, you need to go up an extra level.

Here's the fix:

-const tourService = require('../service/tour.service');
+const tourService = require('../../service/tour.service');
📝 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.

Suggested change
const tourService = require('../service/tour.service');
const tourService = require('../../service/tour.service');


const getTourData = async (req, res) => {
try {
const { userId } = req.body;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Knees weak, arms heavy: Missing input validation! 🍝

There's no validation for the userId parameter. This could lead to server errors or security issues.

Add validation before processing:

+    if (!userId) {
+      return res.status(400).json({
+        status: 'error',
+        message: 'userId is required'
+      });
+    }
+    if (typeof userId !== 'string' && typeof userId !== 'number') {
+      return res.status(400).json({
+        status: 'error',
+        message: 'Invalid userId format'
+      });
+    }
📝 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.

Suggested change
const { userId } = req.body;
const { userId } = req.body;
if (!userId) {
return res.status(400).json({
status: 'error',
message: 'userId is required'
});
}
if (typeof userId !== 'string' && typeof userId !== 'number') {
return res.status(400).json({
status: 'error',
message: 'Invalid userId format'
});
}

const tourData = await tourService.getTourData(userId);
res.status(200).json(tourData);
} catch (error) {
res.status(500).json({ error: error.message });
}
};
Comment on lines +3 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mom's spaghetti moment: GET requests shouldn't use request body! 🍝

Using req.body for GET requests is problematic as some clients and middlewares might not parse the body for GET requests. Consider using query parameters or URL parameters instead.

Here's a better implementation:

-const getTourData = async (req, res) => {
+const getTourData = async (req, res) => {
   try {
-    const { userId } = req.body;
+    const { userId } = req.query;
     const tourData = await tourService.getTourData(userId);
     res.status(200).json(tourData);
   } catch (error) {
-    res.status(500).json({ error: error.message });
+    res.status(500).json({ 
+      status: 'error',
+      message: 'Failed to fetch tour data',
+      error: error.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.

Suggested change
const getTourData = async (req, res) => {
try {
const { userId } = req.body;
const tourData = await tourService.getTourData(userId);
res.status(200).json(tourData);
} catch (error) {
res.status(500).json({ error: error.message });
}
};
const getTourData = async (req, res) => {
try {
const { userId } = req.query;
const tourData = await tourService.getTourData(userId);
res.status(200).json(tourData);
} catch (error) {
res.status(500).json({
status: 'error',
message: 'Failed to fetch tour data',
error: error.message
});
}
};


module.exports = { getTourData };
35 changes: 35 additions & 0 deletions backend/src/middleware/onboard.middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
const db = require('../models');
const { v4: uuidv4 } = require('uuid');

// Middleware to validate API ID
const validateApiId = async (req, res, next) => {
const { apiId } = req.query; // Assume API ID is sent in query params
if (!apiId) {
return res.status(400).json({ error: "API ID is required." });
}
Comment on lines +6 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Mom's spaghetti: Sanitize that apiId input! 🍝

The apiId from query params needs sanitization to prevent SQL injection and other attacks.

-  const { apiId } = req.query;
+  const apiId = String(req.query.apiId).trim().slice(0, 100);
   if (!apiId) {
-    return res.status(400).json({ error: "API ID is required." });
+    return res.status(400).json({ error: "Invalid request" });
   }
📝 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.

Suggested change
const { apiId } = req.query; // Assume API ID is sent in query params
if (!apiId) {
return res.status(400).json({ error: "API ID is required." });
}
const apiId = String(req.query.apiId).trim().slice(0, 100);
if (!apiId) {
return res.status(400).json({ error: "Invalid request" });
}


try {
const user = await db.User.findOne({ where: { apiId } }); // API ID must be in User model
if (!user) {
return res.status(403).json({ error: "Invalid API ID." });
}

req.user = user; // Attach the user to the request for future use
next();
} catch (error) {
return res.status(500).json({ error: "API ID validation failed." });
}
Comment on lines +11 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Knees weak, arms heavy: Add rate limiting! 🍝

The API validation endpoint needs rate limiting to prevent brute force attacks.

Add rate limiting middleware before this function:

const rateLimit = require('express-rate-limit');

const apiLimiter = rateLimit({
  windowMs: 15 * 60 * 1000, // 15 minutes
  max: 100 // limit each IP to 100 requests per windowMs
});

};

// Middleware to generate client ID for each session
const generateClientId = (req, res, next) => {
if (!req.session.clientId) {
req.session.clientId = uuidv4(); // Generate new client ID and store in session
}
next();
};
Comment on lines +25 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

There's vomit on his sweater already: Check session middleware! 🍝

The generateClientId middleware assumes session middleware is configured but doesn't verify its existence.

Add session middleware check:

 const generateClientId = (req, res, next) => {
+  if (!req.session) {
+    return res.status(500).json({ error: "Server configuration error" });
+  }
   if (!req.session.clientId) {
     req.session.clientId = uuidv4();
   }
   next();
 };
📝 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.

Suggested change
const generateClientId = (req, res, next) => {
if (!req.session.clientId) {
req.session.clientId = uuidv4(); // Generate new client ID and store in session
}
next();
};
const generateClientId = (req, res, next) => {
if (!req.session) {
return res.status(500).json({ error: "Server configuration error" });
}
if (!req.session.clientId) {
req.session.clientId = uuidv4(); // Generate new client ID and store in session
}
next();
};


module.exports = {
validateApiId,
generateClientId
};
2 changes: 1 addition & 1 deletion backend/src/routes/guide.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const guideController = require("../controllers/guide.controller.js");

const router = express.Router();

router.get("/get_guides_by_url", guideController.getGuidesByUrl);
router.post("/get_guides_by_url", guideController.getGuidesByUrl);
// router.get("/get_incomplete_guides_by_url", guideController.getIncompleteGuidesByUrl);

module.exports = router;
15 changes: 15 additions & 0 deletions backend/src/routes/onboard.routes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const express = require("express");
const { validateApiId, generateClientId } = require("../middleware/onboard.middleware");
const onboardControllers = require("../controllers/onboard");

const router = express.Router();

router.use(validateApiId);
router.use(generateClientId);

// router.get("/banner", onboardControllers.bannerData.getBannerData);
router.get("/popup", onboardControllers.popupData.getPopupData);
// router.get("/tour", onboardControllers.tourData.getTourData);
// router.get("/hint", onboardControllers.hintData.getHintData);

module.exports = router;
Empty file.
Empty file.
18 changes: 18 additions & 0 deletions backend/src/service/popup.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,24 @@ class PopupService {
}
}

async getPopupByApiAndClientId(apiId, clientId) {
try {
return await Popup.findAll({
include: [
{
model: db.User,
as: "creator",
where: { apiId }
},
],
where: { clientId },
order: [['createdAt', 'DESC']],
});
} catch (error) {
throw new Error("Error retrieving popups for the given API and Client ID");
}
}
Comment on lines +58 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Yo dawg, we need to optimize this query performance!

The where condition on the included model might make your palms sweaty with poor performance on large datasets. Consider these improvements:

  1. Move the apiId condition to the main where clause
  2. Add index on (clientId, createdAt) for better query performance
  3. Add more specific error details in catch block
 async getPopupByApiAndClientId(apiId, clientId) {
   try {
     return await Popup.findAll({
       include: [
         {
           model: db.User,
-          as: "creator",
-          where: { apiId }
+          as: "creator"
         },
       ],
-      where: { clientId },
+      where: { 
+        clientId,
+        '$creator.apiId$': apiId 
+      },
       order: [['createdAt', 'DESC']],
     });
   } catch (error) {
-    throw new Error("Error retrieving popups for the given API and Client ID");
+    throw new Error(`Failed to retrieve popups: ${error.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.

Suggested change
async getPopupByApiAndClientId(apiId, clientId) {
try {
return await Popup.findAll({
include: [
{
model: db.User,
as: "creator",
where: { apiId }
},
],
where: { clientId },
order: [['createdAt', 'DESC']],
});
} catch (error) {
throw new Error("Error retrieving popups for the given API and Client ID");
}
}
async getPopupByApiAndClientId(apiId, clientId) {
try {
return await Popup.findAll({
include: [
{
model: db.User,
as: "creator"
},
],
where: {
clientId,
'$creator.apiId$': apiId
},
order: [['createdAt', 'DESC']],
});
} catch (error) {
throw new Error(`Failed to retrieve popups: ${error.message}`);
}
}


async getPopupByUrl(url) {
try {
return await Popup.findAll({ where: { url } });
Expand Down
5 changes: 3 additions & 2 deletions frontend/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
FROM node:22-alpine3.20
FROM node:22-alpine3.21

RUN apk update && apk add bash && rm -rf /var/cache/apk/*

RUN apk update && apk add bash && rm -rf /var/cache/apk/*

Expand All @@ -7,7 +9,6 @@ WORKDIR /app
COPY package*.json ./

RUN rm -rf package-lock.json && npm install
RUN npm install formik

COPY . .

Expand Down
Loading
Loading