-
Notifications
You must be signed in to change notification settings - Fork 44
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
Added helper_link and link seeder. #496
Added helper_link and link seeder. #496
Conversation
WalkthroughThe changes in the seeders file enhance the database initialization process by introducing new bulk insert operations. The modifications focus on populating the "hints", "helper_link", and "link" tables with predefined data. The seeder now adds more comprehensive initial data, including details like action URLs, colors, and related link information, which will help set up a more structured initial database state. Changes
Sequence DiagramsequenceDiagram
participant DB as Database
participant Seeder as Seeder Script
Seeder->>DB: Insert Hints
Seeder->>DB: Insert Helper Links
Seeder->>DB: Retrieve Helper Link ID
Seeder->>DB: Insert Links with Helper Link References
The sequence diagram illustrates the order of database insertions, showing how the seeder populates different tables sequentially, with each step building upon the previous one by using retrieved IDs for referential integrity. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backend/seeders/seeders.js
Outdated
const links = [ | ||
{ title: 'GuideFox Website', url: 'http://localhost:4173https://guidefox-demo.bluewavelabs.ca', target: true, helperId: helperLink.id }, | ||
{ title: 'GuideFox Repo', url: 'https://github.com/bluewave-labs/guidefox', target: false, helperId: helperLink.id }, | ||
]; | ||
|
||
await queryInterface.bulkInsert('link', links, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Knees weak, arms heavy! That URL is looking sketchy! 🍝
There are several issues in the links array:
- The first URL is malformed:
'http://localhost:4173https://guidefox-demo.bluewavelabs.ca'
- Mixing different environment URLs (localhost, demo, and production)
- The
target
boolean's purpose isn't documented (is it fortarget="_blank"
?)
Here's the fix:
+ const { environments } = require('../config/environments');
const links = [
{
title: 'GuideFox Website',
- url: 'http://localhost:4173https://guidefox-demo.bluewavelabs.ca',
+ url: environments.DEMO_URL,
target: true, // Opens in new tab
helperId: helperLink.id
},
{
title: 'GuideFox Repo',
url: 'https://github.com/bluewave-labs/guidefox',
target: false, // Opens in same tab
helperId: helperLink.id
},
];
Consider adding JSDoc comments to document the schema:
/**
* @typedef {Object} Link
* @property {string} title - The display text for the link
* @property {string} url - The destination URL
* @property {boolean} target - Whether to open in new tab
* @property {number} helperId - Reference to helper_link.id
*/
backend/seeders/seeders.js
Outdated
await queryInterface.bulkInsert( | ||
'hints', | ||
[ | ||
{ | ||
action: 'open url', | ||
url: 'https://guidefox-demo.bluewavelabs.ca', | ||
actionButtonUrl: 'https://guidefox.io/', | ||
actionButtonText: 'Take me to GuideFox page', | ||
targetElement: '.element', | ||
tooltipPlacement: 'right', | ||
hintContent: | ||
'Guidefox helps app owners build knowledge and user-experience oriented apps. It includes the following features: welcome tours, popups, banners, helper links, hints', | ||
header: 'GuideFox Intro', | ||
headerBackgroundColor: '#FFFFFF', | ||
headerColor: '#101828', | ||
textColor: '#101828', | ||
buttonBackgroundColor: '#7F56D9', | ||
buttonTextColor: '#FFFFFF', | ||
createdBy: 1, | ||
}, | ||
], | ||
{} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Yo dawg, these hardcoded values are making me nervous! 🍝
The hint seeder's got some issues that need fixin':
- The URLs are all over the place - mixing demo and production environments
- Those color codes ain't coming from the theme (PR objectives mentioned this)
- No validation on those hex color codes
Here's how we can clean this up:
+ const { themeColors } = require('../config/theme');
+ const { environments } = require('../config/environments');
await queryInterface.bulkInsert(
'hints',
[{
action: 'open url',
- url: 'https://guidefox-demo.bluewavelabs.ca',
+ url: environments.DEMO_URL,
actionButtonUrl: 'https://guidefox.io/',
actionButtonText: 'Take me to GuideFox page',
targetElement: '.element',
tooltipPlacement: 'right',
hintContent:
'Guidefox helps app owners build knowledge and user-experience oriented apps. It includes the following features: welcome tours, popups, banners, helper links, hints',
header: 'GuideFox Intro',
- headerBackgroundColor: '#FFFFFF',
- headerColor: '#101828',
- textColor: '#101828',
- buttonBackgroundColor: '#7F56D9',
- buttonTextColor: '#FFFFFF',
+ headerBackgroundColor: themeColors.background.primary,
+ headerColor: themeColors.text.primary,
+ textColor: themeColors.text.primary,
+ buttonBackgroundColor: themeColors.button.primary,
+ buttonTextColor: themeColors.button.text,
createdBy: 1,
}],
{}
);
Committable suggestion skipped: line range outside the PR's diff.
backend/seeders/seeders.js
Outdated
url: 'https://guidefox-demo.bluewavelabs.ca', | ||
actionButtonUrl: 'https://guidefox.io/', | ||
actionButtonText: 'Take me to GuideFox page', | ||
targetElement: '.element', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this targetElement really exist in the actual website?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! I was a bit confused earlier. I will update it to use an element from the actual website.
So, to confirm, we are showcasing hints, banners, etc., on this website: https://guidefox-demo.bluewavelabs.ca/, for demonstration purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try duplicating the helper link from frontend, it gives an error, suggesting that it may not have been set up successfully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/seeders/seeders.js (2)
35-39
: 🛠️ Refactor suggestionMom's spaghetti alert! These hardcoded colors are making me nervous! 🍝
These color values should be coming from the theme as mentioned in the PR objectives:
#FFFFFF
for header background#101828
for header and text#7F56D9
for button background#FFFFFF
for button textImport and use theme colors:
+ const { themeColors } = require('../config/theme'); { - headerBackgroundColor: '#FFFFFF', - headerColor: '#101828', - textColor: '#101828', - buttonBackgroundColor: '#7F56D9', - buttonTextColor: '#FFFFFF', + headerBackgroundColor: themeColors.background.primary, + headerColor: themeColors.text.primary, + textColor: themeColors.text.primary, + buttonBackgroundColor: themeColors.button.primary, + buttonTextColor: themeColors.button.text, }
91-93
: 🛠️ Refactor suggestionMore hardcoded colors! My sweater's getting heavy! 🍝
These color values should also come from the theme:
#adb2f5
for header background#344054
for link font#7F56D9
for iconImport and use theme colors:
+ const { themeColors } = require('../config/theme'); { - headerBackgroundColor: '#adb2f5', - linkFontColor: '#344054', - iconColor: '#7F56D9', + headerBackgroundColor: themeColors.background.secondary, + linkFontColor: themeColors.text.link, + iconColor: themeColors.icon.primary, }
🧹 Nitpick comments (1)
backend/seeders/seeders.js (1)
100-103
: Knees weak, documentation's heavy! Let's make it better! 🍝The
target
boolean's purpose isn't immediately clear. Let's add JSDoc comments to document the schema:+ /** + * @typedef {Object} Link + * @property {string} title - The display text for the link + * @property {string} url - The destination URL + * @property {boolean} target - Whether to open in new tab (true) or same tab (false) + * @property {number} helperId - Reference to helper_link.id + */ const links = [ { title: 'GuideFox Website', url: 'https://guidefox-demo.bluewavelabs.ca', target: true, helperId: helperLink.id }, { title: 'GuideFox Repo', url: 'https://github.com/bluewave-labs/guidefox', target: false, helperId: helperLink.id }, ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/seeders/seeders.js
(2 hunks)
🔇 Additional comments (2)
backend/seeders/seeders.js (2)
30-30
: Yo dawg, that target element is looking sus! 🍝Based on the previous discussion,
.element
doesn't exist on the actual website. We need to update this to target a real element from https://guidefox-demo.bluewavelabs.ca/.Let's verify what elements are available on the demo site:
#!/bin/bash # Description: Check for commonly used elements in the codebase that might be present on the demo site rg -g '*.{jsx,tsx,vue,html}' 'class=|className=' --no-filename | sort -u
27-28
: 🛠️ Refactor suggestionThese URLs are all over the place! Let's clean up this spaghetti! 🍝
We're mixing demo and production URLs. Let's use environment configuration for consistency:
+ const { environments } = require('../config/environments'); { - url: 'https://guidefox-demo.bluewavelabs.ca', - actionButtonUrl: 'https://guidefox.io/', + url: environments.DEMO_URL, + actionButtonUrl: environments.PRODUCTION_URL, }Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/seeders/seeders.js (1)
100-103
: Knees weak, arms heavy! This links array needs some love! 🍝
- The
target
boolean's purpose isn't documented- Links are tightly coupled with the helper link
Let's add some JSDoc and improve the structure:
+ /** @type {Array<{ + * title: string, + * url: string, + * target: boolean, // When true, opens in new tab + * helperId: number + * }>} */ const links = [ { title: 'GuideFox Website', url: 'https://guidefox-demo.bluewavelabs.ca', target: true, // Opens in new tab helperId: helperLink.id }, // ... other links ];Consider moving the links to a separate configuration file for better maintainability:
// config/seed-data/links.js module.exports = { guidefoxLinks: [ { title: 'GuideFox Website', url: 'https://guidefox-demo.bluewavelabs.ca', target: true, }, // ... other links ] };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/seeders/seeders.js
(2 hunks)
🔇 Additional comments (2)
backend/seeders/seeders.js (2)
35-39
: These hardcoded colors are making me nervous! 🍝Previous review already flagged this - we still need to use theme colors here.
91-93
: Mom's spaghetti alert! More hardcoded colors! 🍝Previous review already flagged this - we still need to use theme colors here.
Describe your changes
Added helper_link and link seeder. The target element for the hints is set to '.element' for now.
Issue number
#495
Please ensure all items are checked off before requesting a review: