add banner cmpt in new pages #8024
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BannerComponent
participant Backend
User->>BannerComponent: Loads page (Home/Schools)
BannerComponent->>Backend: GET /db/banner
Backend-->>BannerComponent: Returns banner content (Markdown)
BannerComponent->>BannerComponent: Localize and process content (Markdown→HTML, sanitize)
BannerComponent-->>User: Renders banner HTML at top of page
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
app/views/home/PageHome.vue (2)
84-88: Fix invalid nested h2 elementsNested
h2elements create invalid HTML structure and can cause accessibility and SEO issues.- <h2 class="text-h2"> - <h2 class="text-h2"> - {{ $t('home_v3.programming_languages') }} - </h2> - </h2> + <h2 class="text-h2"> + {{ $t('home_v3.programming_languages') }} + </h2>
193-201: Fix another instance of invalid nested h2 elementsAnother occurrence of nested
h2elements that needs to be corrected.- <h2 class="text-h2"> - <h2 class="text-h2"> - <mixed-color-label - :text="$t('home_v3.faq_visit_help_center')" - link="https://codecombat.zendesk.com/hc/en-us" - target="_blank" - /> - </h2> - </h2> + <h2 class="text-h2"> + <mixed-color-label + :text="$t('home_v3.faq_visit_help_center')" + link="https://codecombat.zendesk.com/hc/en-us" + target="_blank" + /> + </h2>
🧹 Nitpick comments (3)
app/components/common/elements/BannerComponent.vue (3)
31-31: Consider adding request timeout and better error contextThe API request lacks timeout configuration and error handling doesn't provide context about what failed.
- const data = await fetchJson('/db/banner', { data: { cacheEdge: true } }) + const data = await fetchJson('/db/banner', { + data: { cacheEdge: true }, + timeout: 5000 + })
42-44: Improve error handling with loggingSilent failure makes debugging difficult. Consider logging errors while still providing graceful degradation.
} catch (e) { + console.warn('Failed to fetch banner content:', e) this.bannerHtml = '' }
50-51: Document CSS scoping decision and potential conflictsThe unscoped styles could lead to conflicts with existing application styles, especially with generic class names like
.content.Consider using more specific class names or CSS modules to avoid conflicts:
.content { + &.banner-content { display: inline-block; max-width: 900px; // ... rest of styles + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/common/elements/BannerComponent.vue(1 hunks)app/views/home/PageHome.vue(3 hunks)app/views/schools/PageSchools.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (20.19.2)
🔇 Additional comments (2)
app/views/schools/PageSchools.vue (1)
3-3: Clean integration of BannerComponentThe banner component is properly imported, registered, and placed at the top of the page layout. The implementation follows Vue best practices.
Also applies to: 519-519, 542-542
app/views/home/PageHome.vue (1)
3-3: Proper integration of BannerComponentThe banner component is correctly imported, registered, and positioned at the top of the page layout, following consistent patterns with the schools page.
Also applies to: 241-241, 269-269
| .no-gap-banner { | ||
| margin-bottom: -80px !important; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Questionable CSS with magic number and layout impact
The negative margin of -80px with !important suggests a layout hack that could cause issues across different screen sizes and layouts.
Consider a more robust approach to spacing:
-.no-gap-banner {
- margin-bottom: -80px !important;
-}
+.no-gap-banner {
+ margin-bottom: 0;
+}
+
+.no-gap-banner + * {
+ margin-top: 0;
+}📝 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.
| .no-gap-banner { | |
| margin-bottom: -80px !important; | |
| } | |
| .no-gap-banner { | |
| margin-bottom: 0; | |
| } | |
| .no-gap-banner + * { | |
| margin-top: 0; | |
| } |
🤖 Prompt for AI Agents
In app/components/common/elements/BannerComponent.vue around lines 89 to 91, the
CSS uses a negative margin of -80px with !important, which is a fragile layout
hack that may break on different screen sizes. Replace this magic number with a
more flexible spacing method such as using relative units (em, rem, %) or CSS
grid/flexbox gap properties to control spacing without relying on negative
margins or !important overrides.
| if (window.marked && window.DOMPurify) { | ||
| this.bannerHtml = window.DOMPurify.sanitize(window.marked(content || '')) | ||
| } else if (window.marked) { | ||
| this.bannerHtml = window.marked(content || '') | ||
| } else { | ||
| this.bannerHtml = content || '' | ||
| } |
There was a problem hiding this comment.
Critical security vulnerability: Unsanitized HTML rendering
When only window.marked is available without window.DOMPurify, the markdown is converted to HTML but not sanitized before rendering via v-html. This creates an XSS vulnerability where malicious content from the /db/banner endpoint could execute arbitrary JavaScript.
Apply this fix to ensure content is always sanitized or falls back to plain text:
- if (window.marked && window.DOMPurify) {
- this.bannerHtml = window.DOMPurify.sanitize(window.marked(content || ''))
- } else if (window.marked) {
- this.bannerHtml = window.marked(content || '')
- } else {
- this.bannerHtml = content || ''
- }
+ if (window.marked && window.DOMPurify) {
+ this.bannerHtml = window.DOMPurify.sanitize(window.marked(content || ''))
+ } else {
+ // Only use plain text if sanitization is not available
+ this.bannerHtml = content || ''
+ }📝 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.
| if (window.marked && window.DOMPurify) { | |
| this.bannerHtml = window.DOMPurify.sanitize(window.marked(content || '')) | |
| } else if (window.marked) { | |
| this.bannerHtml = window.marked(content || '') | |
| } else { | |
| this.bannerHtml = content || '' | |
| } | |
| if (window.marked && window.DOMPurify) { | |
| this.bannerHtml = window.DOMPurify.sanitize(window.marked(content || '')) | |
| } else { | |
| // Only use plain text if sanitization is not available | |
| this.bannerHtml = content || '' | |
| } |
🤖 Prompt for AI Agents
In app/components/common/elements/BannerComponent.vue lines 35 to 41, the code
converts markdown to HTML using window.marked but only sanitizes it if
window.DOMPurify is available, leaving a security risk when DOMPurify is
missing. To fix this, ensure that the HTML output is always sanitized before
assigning to bannerHtml by either using DOMPurify if available or falling back
to rendering plain text without HTML. This prevents any unsanitized HTML from
being rendered and eliminates the XSS vulnerability.
fixes eng-1881
Summary by CodeRabbit