-
Notifications
You must be signed in to change notification settings - Fork 8
[LC-1513] Add content negotiation logic and return html display for skills-viewer route #962
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
Conversation
|
✅ Deploy Preview for learncarddocs canceled.
|
|
👋 Hey there! It looks like you modified code, but didn't update the documentation in If this PR introduces new features, changes APIs, or modifies behavior that users or developers need to know about, please consider updating the docs. 🏄 Windsurf TipYou can ask Windsurf to help:
Windsurf will review your changes and suggest appropriate documentation updates based on what was modified. 📚 Documentation Guide
This is an automated reminder. If no docs are needed, feel free to ignore this message. |
❌ Deploy Preview for staging-learncardapp failed. Why did it fail? →
|
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.
✨ PR Review
The PR adds content negotiation and HTML rendering for skills-viewer routes, along with a logout modal UI. The implementation is generally sound, but there's a critical XSS vulnerability in the HTML rendering function that must be addressed before merging.
2 issues detected:
🔒 Security - Unescaped user input in HTML context enables XSS attacks
Details: User-controlled data from skill and framework objects is directly interpolated into HTML without escaping. An attacker could inject malicious scripts through fields like skill.statement, skill.description, skill.icon, or framework.name that would execute in users' browsers.
File:services/learn-card-network/brain-service/src/helpers/skill-renderer.ts🐞 Bug - Naive string matching for Accept header could incorrectly match partial MIME types 🛠️
Details: The Fastify implementation uses string inclusion check for content negotiation which could match unwanted MIME types (e.g., 'text/html-backup'), while the Express version uses the proper req.accepts() method. This inconsistency could lead to different behavior between the two implementations.
File:services/learn-card-network/brain-service/src/skills-viewer.ts (130-130)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
🥷 Code experts: TaylorBeeston, Custard7 TaylorBeeston, Custard7 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Co-authored-by: gitstream-cm[bot] <111687743+gitstream-cm[bot]@users.noreply.github.com>
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.
✨ PR Review
The PR successfully implements content negotiation for skill viewer endpoints with HTML rendering. The logout flow enhancement with visual feedback is well-structured. However, there is a brand configuration issue in the HTML renderer that affects social media preview URLs.
1 issues detected:
🐞 Bug - Social media preview URLs don't respect the brand configuration, always showing scoutnetwork.org domain even for LCA deployments. 🛠️
Details: The social media preview URL is hardcoded to scoutnetwork.org for all brands. When APP_BRAND is set to 'lca', OpenGraph and Twitter meta tags still reference scoutnetwork.org, causing social media shares to display incorrect URLs and potentially confuse users about which platform they're viewing.
File:services/learn-card-network/brain-service/src/helpers/skill-renderer.ts (21-24)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
services/learn-card-network/brain-service/src/helpers/skill-renderer.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: gitstream-cm[bot] <111687743+gitstream-cm[bot]@users.noreply.github.com>
Overview
🎟 Relevant Jira Issues
📚 What is the context and goal of this PR?
🥴 TL; RL:
💡 Feature Breakdown (screenshots & videos encouraged!)
This adds some very simple conditional rendering logic:
I suppose one could make more complex rendering or use some kind of minimal templating language or static site type framework but for now i am keeping it way simple by just using straight html no external dependencies or anything else needed, which for something simple like this i think is fine.
So in a browser environment it looks like this (if request header is "Accept: text/html")

If request header is "Accept: application/json " then it will return json
This also adds a learncard version by adding an new env variable.
Unrelated but when logging out scouts doesnt show a logging out modal so i added that here too.
🛠 Important tradeoffs made:
🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
Testing
🔬 How Can Someone QA This?
Have to run dockerized scouts and make a skills framework and make a boost with the skill and click the link (remove https in the link to see locally)
📱 🖥 Which devices would you like help testing on?
🧪 Code Coverage
Documentation
📝 Documentation Checklist
User-Facing Docs (
docs/→ docs.learncard.com)docs/tutorials/)docs/how-to-guides/)docs/sdks/)docs/core-concepts/)docs/apps/)Internal/AI Docs
Visual Documentation
💭 Documentation Notes
✅ PR Checklist
🚀 Ready to squash-and-merge?:
✨ PR Description
Purpose: Add HTML content negotiation to skills-viewer API route to render branded skill pages for web crawlers and social media sharing.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how