added hints to get complete guides by url #431
Conversation
erenfn
commented
Dec 25, 2024
erenfn
commented
- Added Url field to Hints
- Added Hints to "/get_guides_by_url" and "/get_incomplete_guides_by_url"
WalkthroughThis pull request introduces enhancements to the hint management system across the backend and frontend. The changes primarily focus on integrating URL-based hint functionality, expanding the Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant HintController
participant HintService
participant Database
User->>Frontend: Request Hint Data
Frontend->>HintController: Get Hints by URL
HintController->>HintService: Retrieve Hints
HintService->>Database: Query Hints
Database-->>HintService: Return Hint Data
HintService-->>HintController: Process Hints
HintController-->>Frontend: Return Hint Response
Frontend->>User: Display Hints
Possibly Related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (1)backend/src/test/e2e/hint.test.mjs (1)
Your newly introduced "url" parameter is validated properly in the response, ensuring no mess—mom’s spaghetti is still intact. Keep shining bright, my friend. 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: 2
🧹 Nitpick comments (6)
backend/src/server.js (1)
47-47: Stay calm and deliberate,alter: truecan be tricky on a production database.Using
alter: truedynamically adjusts table schemas, but it can lead to unintended consequences if the schema changes in unexpected ways. Ensure that this approach aligns with your migration strategy or consider using more explicit migrations to keep your database schema stable and well-documented.[Nitpick]
frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx (1)
30-37: You’re spinning bars with your new URL input—looks good in the final mix.The new label and
CustomTextFieldfor theurlproperty align well with the rest of the UI. Consider adding optional validation or user feedback if you need to ensure correct URL formats. Otherwise, everything flows smoothly.backend/src/service/guidelog.service.js (1)
54-66: Your lines are punchy like the best in the game—this new method is close to flame.
getCompleteHintLogsis well-structured and consistent with your other retrieval methods. For potential performance boosts, you might add indexing onuserIdif you expect a large volume of logs, ensuring queries remain efficient in scale.backend/src/models/Hint.js (1)
19-23: Add Some “Wow Factor” to the NewurlField
Yo, yo, we’re adding a new property to store the URL. Overall, this is solid because it opens up more flexible retrieval options. Just ensure no indexing or max-length constraints are needed to handle extremely long URLs.frontend/src/scenes/hints/CreateHintPage.jsx (2)
32-33: Seamless URL Handling
So, you set the default URL to"https://". That’s lovely, mom’s spaghetti. Just confirm if your app expects that as a placeholder or if you’d prefer an empty string to indicate no URL set.
91-91: Focus on the Data
Includingurlin the payload is dope, but be mindful: if the API grows, you may need partial updates. Right now, sending the entire object is okay, but a patch approach might be more scalable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/src/controllers/guide.controller.js(3 hunks)backend/src/models/Hint.js(1 hunks)backend/src/routes/hint.routes.js(1 hunks)backend/src/server.js(1 hunks)backend/src/service/guidelog.service.js(1 hunks)backend/src/service/hint.service.js(1 hunks)frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx(2 hunks)frontend/src/scenes/hints/CreateHintPage.jsx(3 hunks)
🔇 Additional comments (7)
backend/src/routes/hint.routes.js (1)
16-16: You might want to keep your knees steady, check user access like mom’s spaghetti.
The new GET route is missing authentication in a secure environment. If users should retrieve hints without logging in, that’s fine, but you might want to confirm that it fits the intended use case and doesn’t inadvertently open the door to unauthorized data access.
frontend/src/components/HintPageComponents/HintLeftContent/HintLeftContent.jsx (2)
6-6: Props are piling up but we’re still on top—clean structure, no flop.
The inclusion of setUrl and url is consistent with the existing pattern for other props. Everything reads nicely and remains straightforward to maintain.
13-15: Handle the craft by controlling the draft—your state management looks tight.
Excellent approach to updating the URL state with handleUrl. This method helps keep the component’s internal logic consistent and ensures the new url field is properly tracked.
backend/src/controllers/guide.controller.js (2)
5-6: Import Check
We can see a new import for hintService. Looks good. Ensure it’s recognized properly in the service registry to prevent missing injection or undefined references.
16-22: Harmonize the Response
The promise-all usage is neat, dawg. Just double-check if you need to handle possible null returns for banners or popups. For instance, if bannerService.getBannerByUrl(url) returns null, the final response might not match the front-end’s expectations.
frontend/src/scenes/hints/CreateHintPage.jsx (1)
77-77: Ensure We’ve Got a Real URL
When editing, you set the URL from the fetched hint data if available, or fallback to "https://". Maybe consider a quick format check or a prompt to the user if the passed URL is empty or remains the default placeholder.
backend/src/service/hint.service.js (1)
80-81: Service Expansion
The new method getIncompleteHintsByUrl aligns with your logic to skip hints that have IDs in the complete logs. Good call. The method name is coherent with the rest of the code.
| const [completePopupLogs, completeBannerLogs, completeHintLogs, completeHelperLogs] = await Promise.all([ | ||
| guidelogService.getCompletePopupLogs(userId), | ||
| guidelogService.getCompleteBannerLogs(userId), | ||
| guidelogService.getCompleteHintLogs(userId), | ||
| guidelogService.getCompleteHelperLogs(userId), | ||
| ]); | ||
|
|
||
| const popupIds = completePopupLogs.map(log => log.guideId); | ||
| const bannerIds = completeBannerLogs.map(log => log.guideId); | ||
| const hintIds = completeHintLogs.map(log => log.guideId); | ||
| const helperLinkIds = completeHelperLogs.map(log => log.guideId); | ||
|
|
||
| const [banner, popup, helperLink] = await Promise.all([ | ||
| bannerService.getIncompleteBannersByUrl(url, bannerIds), | ||
| const [popup, banner, hint, helperLink] = await Promise.all([ | ||
| popupService.getIncompletePopupsByUrl(url, popupIds), | ||
| bannerService.getIncompleteBannersByUrl(url, bannerIds), | ||
| hintService.getIncompletePopupsByUrl(url, hintIds), | ||
| helperLinkService.getIncompleteHelpers(helperLinkIds), | ||
| ]); | ||
| res.status(200).json({ banner, popup, helperLink }); | ||
| res.status(200).json({popup, banner, hint, helperLink }); |
There was a problem hiding this comment.
Mistake in Incomplete Hints
The function call is referencing guidelogService.getCompleteHintLogs(userId), but at line 57 we call hintService.getIncompletePopupsByUrl(url, hintIds). Doesn’t appear consistent with the naming, since we specifically want “incomplete hints” but the function is “getIncompletePopupsByUrl.” Double-check that or rename it for clarity to avoid a spaghetti meltdown.
- hintService.getIncompletePopupsByUrl(url, hintIds),
+ hintService.getIncompleteHintsByUrl(url, hintIds),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [completePopupLogs, completeBannerLogs, completeHintLogs, completeHelperLogs] = await Promise.all([ | |
| guidelogService.getCompletePopupLogs(userId), | |
| guidelogService.getCompleteBannerLogs(userId), | |
| guidelogService.getCompleteHintLogs(userId), | |
| guidelogService.getCompleteHelperLogs(userId), | |
| ]); | |
| const popupIds = completePopupLogs.map(log => log.guideId); | |
| const bannerIds = completeBannerLogs.map(log => log.guideId); | |
| const hintIds = completeHintLogs.map(log => log.guideId); | |
| const helperLinkIds = completeHelperLogs.map(log => log.guideId); | |
| const [banner, popup, helperLink] = await Promise.all([ | |
| bannerService.getIncompleteBannersByUrl(url, bannerIds), | |
| const [popup, banner, hint, helperLink] = await Promise.all([ | |
| popupService.getIncompletePopupsByUrl(url, popupIds), | |
| bannerService.getIncompleteBannersByUrl(url, bannerIds), | |
| hintService.getIncompletePopupsByUrl(url, hintIds), | |
| helperLinkService.getIncompleteHelpers(helperLinkIds), | |
| ]); | |
| res.status(200).json({ banner, popup, helperLink }); | |
| res.status(200).json({popup, banner, hint, helperLink }); | |
| const [completePopupLogs, completeBannerLogs, completeHintLogs, completeHelperLogs] = await Promise.all([ | |
| guidelogService.getCompletePopupLogs(userId), | |
| guidelogService.getCompleteBannerLogs(userId), | |
| guidelogService.getCompleteHintLogs(userId), | |
| guidelogService.getCompleteHelperLogs(userId), | |
| ]); | |
| const popupIds = completePopupLogs.map(log => log.guideId); | |
| const bannerIds = completeBannerLogs.map(log => log.guideId); | |
| const hintIds = completeHintLogs.map(log => log.guideId); | |
| const helperLinkIds = completeHelperLogs.map(log => log.guideId); | |
| const [popup, banner, hint, helperLink] = await Promise.all([ | |
| popupService.getIncompletePopupsByUrl(url, popupIds), | |
| bannerService.getIncompleteBannersByUrl(url, bannerIds), | |
| hintService.getIncompleteHintsByUrl(url, hintIds), | |
| helperLinkService.getIncompleteHelpers(helperLinkIds), | |
| ]); | |
| res.status(200).json({popup, banner, hint, helperLink }); |
| async getIncompleteHintsByUrl(url, ids) { | ||
| try { | ||
| return await Hint.findAll({ | ||
| where: { | ||
| url, | ||
| id: { [Op.notIn]: ids } | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| throw new Error("Error retrieving hint by URL"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen Error Handling
We’re just throwing a generic “Error retrieving hint by URL.” Might be nice to pass the underlying error.message to the thrown Error, like you do in other methods, for better debugging.
catch (error) {
- throw new Error("Error retrieving hint by URL");
+ throw new Error("Error retrieving hint by URL: " + 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.
| async getIncompleteHintsByUrl(url, ids) { | |
| try { | |
| return await Hint.findAll({ | |
| where: { | |
| url, | |
| id: { [Op.notIn]: ids } | |
| } | |
| }); | |
| } catch (error) { | |
| throw new Error("Error retrieving hint by URL"); | |
| } | |
| }; | |
| async getIncompleteHintsByUrl(url, ids) { | |
| try { | |
| return await Hint.findAll({ | |
| where: { | |
| url, | |
| id: { [Op.notIn]: ids } | |
| } | |
| }); | |
| } catch (error) { | |
| throw new Error("Error retrieving hint by URL: " + error.message); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/test/mocks/hint.mock.js (1)
12-12: Great addition of theurlproperty!
Including an empty string as a default is an acceptable approach; however, consider verifying whether or not the URL must always remain optional or if it should hold a valid link. Incorporating a quick validation or test case for the URL property will help ensure data integrity and catch potential user errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/test/mocks/hint.mock.js(1 hunks)frontend/src/scenes/hints/CreateHintPage.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/scenes/hints/CreateHintPage.jsx
