Durga js fetch api debugging#545
Conversation
Summary of ChangesHello @Saidurgasatturi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces several new front-end assignments, primarily focusing on practical application of JavaScript's Fetch API and modern CSS techniques. It delivers a functional, albeit simple, e-commerce experience and separate examples for responsive design and animations, serving as foundational exercises for web development concepts. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new files for CSS and JavaScript assignments. My review focuses on improving security, code organization, and adherence to web development best practices. The most critical issues are related to Cross-Site Scripting (XSS) vulnerabilities due to the use of innerHTML with data fetched from an external API. I've also provided feedback on separating CSS and JavaScript from HTML files, improving user experience by avoiding alert(), and correctly encoding URL parameters. Addressing these points will make the code more secure, maintainable, and robust.
| container.innerHTML = ` | ||
| <img src="${product.thumbnail}" alt="${product.title}"> | ||
| <h2>${product.title}</h2> | ||
| <p>${product.description}</p> | ||
| <p><strong>Price:</strong> $${product.price}</p> | ||
| <p><strong>Brand:</strong> ${product.brand}</p> | ||
| <p><strong>Category:</strong> ${product.category}</p> | ||
| <button onclick="window.history.back()">⬅️ Go Back</button> | ||
| `; |
There was a problem hiding this comment.
Using innerHTML to inject content from an API response can expose your application to Cross-Site Scripting (XSS) attacks if the fetched data contains malicious scripts. It's much safer to create DOM elements programmatically and set their content using properties like textContent, which prevents string content from being parsed as HTML.
| productContainer.innerHTML = products | ||
| .map( | ||
| (p) => ` | ||
| <div class="product-card"> | ||
| <img src="${p.thumbnail}" alt="${p.title}"> | ||
| <h3>${p.title}</h3> | ||
| <p>$${p.price}</p> | ||
| <button onclick="viewProduct(${p.id})">View Details</button> | ||
| </div> | ||
| ` | ||
| ) | ||
| .join(""); |
There was a problem hiding this comment.
Using innerHTML to render a list of items from an API is a significant security risk, making the application vulnerable to Cross-Site Scripting (XSS). Additionally, using inline onclick handlers is not a modern best practice. The recommended approach is to create DOM elements for each product card programmatically, use textContent to safely insert data, and attach event listeners with addEventListener.
| @media (max-width: 768px) { | ||
| main { | ||
| flex-direction: column; | ||
| flex:1; |
There was a problem hiding this comment.
The flex: 1 property here has no effect. main is a flex container, but it is not a flex item of another flex container (its parent, body, does not have display: flex by default). You can safely remove this line.
| <style> | ||
| body { | ||
| font-family: Arial, sans-serif; | ||
| margin: 0; | ||
| padding: 0; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 100vh; | ||
| background: #f0f0f0; | ||
| } | ||
| .box { | ||
| width: 150px; | ||
| height: 150px; | ||
| background: #3498db; | ||
| border-radius: 10px; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| color: white; | ||
| font-weight: bold; | ||
| animation: bounce 2s infinite; | ||
| } | ||
| @keyframes bounce { | ||
| 0%, 100% { | ||
| transform: translateY(0); | ||
| } | ||
| 50% { | ||
| transform: translateY(-50px); | ||
| } | ||
| } | ||
| @media (max-width: 768px) { | ||
| .box { | ||
| width: 120px; | ||
| height: 120px; | ||
| font-size: 14px; | ||
| background: #e67e22; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 480px) { | ||
| .box { | ||
| width: 90px; | ||
| height: 90px; | ||
| font-size: 12px; | ||
| background: #2ecc71; | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
It's a best practice to keep your CSS in a separate file rather than embedding it within a <style> tag in your HTML. This improves code organization, reusability, and browser caching. Consider creating a separate .css file and linking it in the <head> using a <link> tag.
| <script> | ||
| async function loadProductDetails() { | ||
| const params = new URLSearchParams(window.location.search); | ||
| const id = params.get("id"); | ||
|
|
||
| try { | ||
| const res = await fetch(`https://dummyjson.com/products/${id}`); | ||
| if (!res.ok) throw new Error("Failed to fetch product details"); | ||
| const product = await res.json(); | ||
|
|
||
| const container = document.getElementById("productDetails"); | ||
| container.innerHTML = ` | ||
| <img src="${product.thumbnail}" alt="${product.title}"> | ||
| <h2>${product.title}</h2> | ||
| <p>${product.description}</p> | ||
| <p><strong>Price:</strong> $${product.price}</p> | ||
| <p><strong>Brand:</strong> ${product.brand}</p> | ||
| <p><strong>Category:</strong> ${product.category}</p> | ||
| <button onclick="window.history.back()">⬅️ Go Back</button> | ||
| `; | ||
| } catch (error) { | ||
| document.getElementById("productDetails").innerHTML = `<p>Error: ${error.message}</p>`; | ||
| } | ||
| } | ||
|
|
||
| loadProductDetails(); | ||
| </script> |
There was a problem hiding this comment.
For better code organization and maintainability, it's a best practice to move JavaScript code out of HTML files. Consider creating a separate product-details.js file and including it with <script src="product-details.js" defer></script> just before the closing </body> tag.
| <button onclick="window.history.back()">⬅️ Go Back</button> | ||
| `; | ||
| } catch (error) { | ||
| document.getElementById("productDetails").innerHTML = `<p>Error: ${error.message}</p>`; |
There was a problem hiding this comment.
Using innerHTML to display an error message can be a security risk if the error.message can be manipulated. It's safer to use textContent to prevent any potential HTML injection.
| document.getElementById("productDetails").innerHTML = `<p>Error: ${error.message}</p>`; | |
| document.getElementById("productDetails").textContent = `Error: ${error.message}`; |
| const data = await res.json(); | ||
| displayProducts(data.products); | ||
| } catch (error) { | ||
| productContainer.innerHTML = `<p>Error: ${error.message}</p>`; |
There was a problem hiding this comment.
Using innerHTML to display an error message can be a security risk if the error.message can be manipulated. It's safer to use textContent to prevent any potential HTML injection.
| productContainer.innerHTML = `<p>Error: ${error.message}</p>`; | |
| productContainer.textContent = `Error: ${error.message}`; |
|
|
||
| async function searchProducts(query) { | ||
| try { | ||
| const res = await fetch(`https://dummyjson.com/products/search?q=${query}`); |
There was a problem hiding this comment.
The search query should be properly encoded before being included in a URL to handle special characters (e.g., &, ?, ) correctly. Using encodeURIComponent() ensures the query is treated as a single, safe string parameter.
| const res = await fetch(`https://dummyjson.com/products/search?q=${query}`); | |
| const res = await fetch(`https://dummyjson.com/products/search?q=${encodeURIComponent(query)}`); |
| const data = await res.json(); | ||
| displayProducts(data.products); | ||
| } catch (error) { | ||
| productContainer.innerHTML = `<p>Error: ${error.message}</p>`; |
There was a problem hiding this comment.
Using innerHTML to display an error message can be a security risk if the error.message can be manipulated. It's safer to use textContent to prevent any potential HTML injection.
| productContainer.innerHTML = `<p>Error: ${error.message}</p>`; | |
| productContainer.textContent = `Error: ${error.message}`; |
| searchBtn.addEventListener("click", () => { | ||
| const query = searchInput.value.trim(); | ||
| if (query) searchProducts(query); | ||
| else alert("Please enter something to search!"); |
There was a problem hiding this comment.
Using alert() provides a poor user experience because it's a blocking modal dialog. A better approach is to display a non-blocking message within the UI, for instance, by adding a text element near the search bar or changing the input's placeholder text temporarily.
1 participant
Terms and Conditions
HTML Best Practices
File Naming Convention:
Follow consistent and descriptive naming (e.g., dashboard.html, user-profile.html).
Use lowercase letters and hyphens instead of spaces.
Page Title:
Ensure the <title> tag is descriptive and aligns with the page content.
Include meaningful keywords for SEO if applicable.
Semantic Markup:
Use appropriate tags like <header>, <footer>, <section>, <article> for better readability and accessibility.
Accessibility Standards:
Ensure the use of alt attributes for images and proper labels for form elements.
Use ARIA roles where necessary.
Validation:
Ensure the code passes HTML validation tools without errors or warnings.
Structure and Indentation:
Maintain consistent indentation and proper nesting of tags.
Attributes:
Ensure all required attributes (e.g., src, href, type, etc.) are correctly used and not left empty.
CSS Best Practices
File Organization:
Use modular CSS files if applicable (e.g., base.css, layout.css, theme.css).
Avoid inline styles unless absolutely necessary.
Naming Conventions:
Use meaningful class names following BEM or other conventions (e.g., block__element--modifier).
Code Reusability:
Avoid duplicate code; use classes or mixins for shared styles.
Responsive Design:
Ensure proper usage of media queries for mobile, tablet, and desktop views.
Performance Optimization:
Minimize the use of unnecessary CSS selectors.
Avoid overly specific selectors and ensure selectors are not overly deep (e.g., avoid #id .class1 .class2 p).
Consistency:
Follow consistent spacing, indentation, and use of units (rem/em vs. px).
Maintain a single coding style (e.g., always use double or single quotes consistently).
Javascript Best Practices
File Organization:
Ensure scripts are modular and logically separated into files if needed.
Avoid mixing inline JavaScript with HTML.
Logic Optimization:
Check for redundancy and ensure the code is optimized for performance.
Avoid unnecessary API calls or DOM manipulations.
Solution Approach:
Confirm that the code solves the given problem efficiently.
Consider scalability for future enhancements.
Readability:
Use clear variable and function names.
Add comments for complex logic or algorithms.
Error Handling:
Ensure proper error handling for API calls or user input validation.
Code Quality:
Check for potential bugs (e.g., missing await, mishandling of null/undefined values).
Avoid unnecessary console.log statements in production code.
Security:
Avoid hardcoding sensitive data.
Sanitize user input to prevent XSS and other vulnerabilities.
Best Practices:
Use const and let instead of var.
Follow ES6+ standards where applicable.