feat(web): support markdown in notification messages #963
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the Changes
Possibly related PRs
Suggested reviewers
🪧 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
CodeRabbit Configuration File (
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![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
🧹 Outside diff range and nitpick comments (3)
web/helpers/markdown.ts (1)
4-9: Enhance documentation with specific detailsThe documentation is good but could be more comprehensive to help other developers.
/** * Parses arbitrary markdown content as sanitized html. May throw if parsing fails. * * @param markdownContent string of markdown content * @returns safe, sanitized html content + * @throws {Error} When markdown parsing fails or content is invalid + * @example + * ```ts + * // Basic usage + * const html = await safeParseMarkdown('# Hello **world**'); + * + * // Error handling + * try { + * const html = await safeParseMarkdown(content); + * } catch (error) { + * console.error('Failed to parse markdown:', error); + * } + * ``` + * @remarks + * The returned HTML is sanitized using DOMPurify's default configuration, + * which removes potentially dangerous tags and attributes. */web/components/Notifications/Item.vue (1)
21-27: Consider improving error handling and loading statesWhile the implementation is functional, consider these improvements:
- Add error logging to help with debugging
- Consider showing a loading state during markdown parsing
- Use the original description as initial value to prevent UI flicker
-const descriptionMarkup = computedAsync(async () => { +const descriptionMarkup = computedAsync(async () => { try { return await safeParseMarkdown(props.description); } catch (e) { + console.error('Failed to parse markdown:', e); return props.description; } -}, ''); +}, props.description);web/store/updateOsChangelog.ts (1)
Line range hint
67-96: Consider separating markdown configuration concernsThe current implementation mixes markdown parsing configuration with data fetching logic. Consider:
- Moving all markdown-related configuration (renderer setup, link handling) to a dedicated configuration file or the markdown helper
- Separating the presentation logic (opening links in new tabs) from the data layer
This would improve maintainability and make the code more modular.
Example structure:
// web/config/markdown.ts export const getMarkdownConfig = () => ({ baseUrl: DOCS_RELEASE_NOTES.toString(), renderer: createCustomRenderer(), // ... other config }); // This file import { getMarkdownConfig } from '~/config/markdown'; // ... marked.setOptions(getMarkdownConfig());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
web/components/Notifications/Item.vue(2 hunks)web/helpers/markdown.ts(1 hunks)web/package.json(1 hunks)web/store/updateOsChangelog.ts(2 hunks)
🔇 Additional comments (8)
web/helpers/markdown.ts (1)
1-2: Verify DOMPurify configuration for security
While using DOMPurify is a good security practice, we should verify its configuration across the codebase to ensure consistent sanitization rules.
#!/bin/bash
# Search for other DOMPurify configurations that might affect sanitization
rg -l "DOMPurify\.setConfig|DOMPurify\.sanitize.*config"
# Check for any custom sanitization rules or overrides
rg -l "DOMPurify" --type ts --type vueweb/package.json (2)
72-72: Verify compatibility between markdown-related packages.
Ensure DOMPurify (^3.2.0) works seamlessly with marked (^12.0.2) and marked-base-url (^1.1.3) for safe HTML rendering.
#!/bin/bash
# Check for any reported compatibility issues
echo "Checking package.json and lock files for compatibility notes..."
rg -A 5 "dompurify.*marked|marked.*dompurify"
# Look for existing usage patterns
echo "Checking for existing usage patterns..."
ast-grep --pattern 'import $_ from "dompurify"'
ast-grep --pattern 'import $_ from "marked"'Also applies to: 78-79
72-72: LGTM! Verify DOMPurify version for security.
The addition of DOMPurify is essential for XSS prevention when rendering markdown as HTML in notifications.
web/components/Notifications/Item.vue (3)
7-13: LGTM! Import changes are well-organized
The imports are logically grouped and the new safeParseMarkdown import supports the markdown feature requirement.
31-44: LGTM! Style changes only
97-97: Verify markdown sanitization and consider styling
While safeParseMarkdown should handle sanitization, let's verify its implementation. Also, consider adding specific CSS classes for markdown content styling (e.g., for links, lists, etc.).
Consider adding markdown-specific styles:
- <div class="text-secondary-foreground" v-html="descriptionMarkup" />
+ <div class="text-secondary-foreground prose prose-sm dark:prose-invert" v-html="descriptionMarkup" />✅ Verification successful
Add prose classes for proper markdown styling
The markdown content is properly sanitized using DOMPurify in safeParseMarkdown, and Tailwind prose styles are already configured in the project with custom theming. Add the prose classes to ensure consistent markdown styling:
- <div class="text-secondary-foreground" v-html="descriptionMarkup" />
+ <div class="text-secondary-foreground prose prose-sm dark:prose-invert" v-html="descriptionMarkup" />🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the implementation of safeParseMarkdown
ast-grep --pattern 'function safeParseMarkdown($_) {
$$$
}'
# Check for any existing markdown-specific styles
rg -g '*.css' -g '*.scss' 'markdown|prose'
Length of output: 376
Script:
#!/bin/bash
# Check imports and usage of DOMPurify
rg -A 3 "import.*DOMPurify"
# Check if DOMPurify is properly configured
rg "DOMPurify\.setConfig|DOMPurify\.addHook"
# Look for any markdown-related styles in Tailwind config
rg -g "tailwind.config.*" "prose"
# Check for any existing prose/markdown classes in Vue components
rg -g "*.vue" "class=.*prose"
Length of output: 6940
web/store/updateOsChangelog.ts (2)
12-12: LGTM: Import statement is correctly placed
The import of safeParseMarkdown is appropriately placed and follows the project's conventions.
96-96: Verify interaction with marked.js configuration
The custom marked.js configuration (renderer, baseUrl, link handling) might be redundant or conflict with safeParseMarkdown. Please verify:
- If
safeParseMarkdownhandles relative URLs and link modifications - If the security settings in the custom renderer are still needed
Let's check the implementation of safeParseMarkdown:
✅ Verification successful
Let me gather more context about the marked configuration in updateOsChangelog.ts:
The marked.js configuration is necessary and complementary to safeParseMarkdown
The current implementation is correct and there's no conflict:
safeParseMarkdownprovides basic parsing and sanitization using DOMPurify- The marked.js configuration in updateOsChangelog.ts adds essential features:
- Sets base URL for relative links
- Opens links in new tabs
- Removes .md extensions from links
- Configures sanitization at the renderer level
These configurations work together - marked.js handles the markdown-specific transformations before safeParseMarkdown applies final HTML sanitization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if safeParseMarkdown has its own marked configuration
# and how it handles URLs and link modifications
# Find the implementation
rg -A 10 "export.*safeParseMarkdown" web/helpers/markdown.ts
# Check for other uses of marked configuration
rg "marked\.use|marked\.setOptions" --type ts
Length of output: 416
Script:
#!/bin/bash
# Check the marked configuration and renderer setup in updateOsChangelog
rg -B 5 -A 10 "marked\.use|marked\.setOptions" web/store/updateOsChangelog.ts
# Check if there are any other files using safeParseMarkdown
rg "safeParseMarkdown" --type ts
Length of output: 1423
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/components/Notifications/Item.vue(2 hunks)web/helpers/markdown.ts(1 hunks)web/store/updateOsChangelog.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/Notifications/Item.vue
🧰 Additional context used
🪛 Biome
web/helpers/markdown.ts
[error] 16-41: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 GitHub Check: Build Web App
web/helpers/markdown.ts
[failure] 16-16:
Unexpected class with only static properties
web/store/updateOsChangelog.ts
[failure] 7-7:
'Marked' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (4)
web/helpers/markdown.ts (2)
1-11: Well-structured sanitization setup!
The implementation effectively uses marked's hooks system for HTML sanitization, with proper typing and binding preservation. This is a clean approach that ensures consistent sanitization across all markdown parsing.
16-41: Consider enhancing flexibility while maintaining simplicity
While the current static-only class implementation effectively maintains a consistent parser configuration, consider these optional enhancements:
- Allow runtime configuration updates:
static updateConfig(...extensions: Parameters<Marked['use']>) {
Markdown.instance = Markdown.create(...extensions);
}- Support per-parse options:
static async parse(
markdownContent: string,
options?: marked.MarkedOptions
): Promise<string> {
return options
? Markdown.create().setOptions(options).parse(markdownContent)
: Markdown.instance.parse(markdownContent);
}Let's verify the current usage pattern:
🧰 Tools
🪛 Biome
[error] 16-41: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 GitHub Check: Build Web App
[failure] 16-16:
Unexpected class with only static properties
web/store/updateOsChangelog.ts (2)
32-35: LGTM! Improved readability
The computed properties have been reformatted for better readability while maintaining the same logic.
Also applies to: 38-40, 56-59
116-123: LGTM! Improved parameter formatting
The callback parameters have been reformatted for better readability while maintaining the same structure.
pujitm
force-pushed
the
feat/md-notifs
branch
from
67c243e to
7396ec2
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
web/helpers/markdown.test.ts (3)
5-6: Update misleading comment.The comment suggests adding a "random" extension, but you're specifically adding the baseUrl extension.
-// add a random extension to the instance +// create instance with baseUrl extension for testing URL transformations
7-10: Add return type to parse helper function.The parse helper function would benefit from explicit return type definition for better type safety.
-const parse = async (content: string) => ({ +const parse = async (content: string): Promise<{ + fromDefault: string; + fromInstance: string; +}> => ({
20-32: Add more URL transformation test cases.Consider adding test cases for various URL formats and edge cases:
- Absolute URLs
- URLs with query parameters
- Fragment identifiers
- URLs with special characters
- Empty URLs
Example additional test cases:
test('handles various URL formats', async () => { const cases = [ { input: '[Link](https://external.com)', // Should not modify absolute URLs expected: '<p><a href="https://external.com">Link</a></p>' }, { input: '[Query](/search?q=test)', expected: '<p><a href="https://unraid.net/search?q=test">Query</a></p>' }, { input: '[Fragment](/page#section)', expected: '<p><a href="https://unraid.net/page#section">Fragment</a></p>' }, { input: '[Special](/path with spaces)', expected: '<p><a href="https://unraid.net/path%20with%20spaces">Special</a></p>' } ]; for (const { input, expected } of cases) { const parsed = await parse(input); expect(parsed.fromInstance.trim()).toBe(expected); } });web/helpers/markdown.ts (1)
16-41: Consider a more flexible architecture.While the current implementation works, there are a few architectural considerations:
- The static-only class could be refactored to standalone functions for simplicity:
const defaultInstance = createMarkedInstance(); export function createMarkedInstance(...args: Parameters<Marked['use']>) { return new Marked(defaultMarkedExtension, ...args); } export async function parseMarkdown(markdownContent: string): Promise<string> { return defaultInstance.parse(markdownContent); }
- The singleton pattern might make it difficult to use different configurations in different parts of the application. Consider allowing instance-specific configurations:
export class MarkdownParser { private marked: Marked; constructor(...extensions: Parameters<Marked['use']>) { this.marked = new Marked(defaultMarkedExtension, ...extensions); } async parse(markdownContent: string): Promise<string> { return this.marked.parse(markdownContent); } } // Usage: const defaultParser = new MarkdownParser(); export const parseMarkdown = (content: string) => defaultParser.parse(content);Would you like me to help implement either of these alternatives?
🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 16-16:
Unexpected class with only static properties🪛 Biome
[error] 16-41: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
web/helpers/__snapshots__/markdown.test.ts.snapis excluded by!**/*.snapweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
web/helpers/markdown.test.ts(1 hunks)web/helpers/markdown.ts(1 hunks)web/package.json(1 hunks)web/store/updateOsChangelog.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/package.json
- web/store/updateOsChangelog.ts
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/helpers/markdown.ts
[failure] 16-16:
Unexpected class with only static properties
🪛 Biome
web/helpers/markdown.ts
[error] 16-41: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
web/helpers/markdown.ts (1)
1-11: LGTM! Well-structured sanitization setup.
The implementation elegantly integrates DOMPurify sanitization through marked's hook system, and the use of isomorphic-dompurify ensures compatibility across different environments.
|
@zackspear how can i test the changelog for regressions? I'm slightly worried about how i'm importing the helper code in the store--the old import was |
fix(web): replaces dompurify with isomorphic-dompurify to enable server-side usage with same syntax
pujitm
force-pushed
the
feat/md-notifs
branch
from
7396ec2 to
a6bac2d
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/helpers/markdown.ts (1)
19-41: Consider enhancing error handling in parse method.While the implementation is solid, consider adding specific error handling to provide more context when parsing fails:
static async parse(markdownContent: string): Promise<string> { - return Markdown.instance.parse(markdownContent); + try { + return Markdown.instance.parse(markdownContent); + } catch (error) { + throw new Error(`Failed to parse markdown: ${error instanceof Error ? error.message : 'unknown error'}`); + } }🧰 Tools
🪛 Biome
[error] 16-41: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
web/helpers/__snapshots__/markdown.test.ts.snapis excluded by!**/*.snapweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
web/helpers/markdown.test.ts(1 hunks)web/helpers/markdown.ts(1 hunks)web/package.json(1 hunks)web/store/updateOsChangelog.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/helpers/markdown.test.ts
- web/package.json
- web/store/updateOsChangelog.ts
🧰 Additional context used
🪛 Biome
web/helpers/markdown.ts
[error] 16-41: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 GitHub Check: Build Web App
web/helpers/markdown.ts
[failure] 16-16:
Unexpected class with only static properties
🔇 Additional comments (2)
web/helpers/markdown.ts (2)
1-11: Well-structured markdown sanitization setup!
Good practices observed:
- Using
isomorphic-dompurifyfor cross-platform compatibility - Implementing sanitization through marked's postprocess hook
- Clear documentation about the function syntax choice
13-18: Class structure is appropriate despite static analysis warning.
While static analysis suggests avoiding classes with only static members, the class structure here is justified as it:
- Provides a clear namespace for markdown-related functionality
- Enables singleton pattern for consistent parser configuration
- Allows for future instance-specific extensions if needed
Let's verify if there are any similar patterns in the codebase:
✅ Verification successful
Class structure is well-utilized across the codebase
The verification confirms that the Markdown class structure is appropriate and effectively used throughout the codebase:
- The singleton pattern is actively used in multiple components (
Item.vue,updateOsChangelog.ts) - The class supports both global instance (
Markdown.parse) and custom instances (Markdown.create(baseUrl)) - Tests demonstrate the dual usage pattern, validating both default and custom instances
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar utility classes with static-only members
ast-grep --pattern 'class $_ {
private static $_ = $_
static $_ ($_) {
$$$
}
}'
Length of output: 91
Script:
#!/bin/bash
# Let's try a broader search for classes with static members and instance patterns
ast-grep --pattern 'class $_ {
private static $_
}'
# Also search for static create methods which might indicate similar patterns
ast-grep --pattern 'class $_ {
static create'
# Let's also check for actual usage of this Markdown class
rg "Markdown\." -A 2
Length of output: 1179
🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 16-16:
Unexpected class with only static properties
I've tested this by deploying to a dev server and verifying that the changelogs & links were correct in the upgrade/downgrade os tools. they seem to be correct, but i'm not 100% sure that this is sufficient. moving on anyway! |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/main.yml (1)
139-141: Enhance test step configurationWhile adding the test step is good, consider these improvements for better CI/CD practices:
- Add continue-on-error strategy to match the lint step's behavior
- Capture and upload test results as artifacts for better debugging
Apply this diff to enhance the test step:
- name: Test - run: npm run test:ci + continue-on-error: true + run: | + npm run test:ci || exit_code=$? + mkdir -p test-results + cp coverage/* test-results/ || true + exit ${exit_code:-0} + +- name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-results + path: web/test-results
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/main.yml(1 hunks)web/helpers/markdown.ts(1 hunks)web/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/package.json
🧰 Additional context used
📓 Learnings (1)
web/helpers/markdown.ts (1)
Learnt from: pujitm
PR: unraid/api#963
File: web/helpers/markdown.ts:1-41
Timestamp: 2024-11-19T16:34:44.804Z
Learning: In `web/store/updateOsChangelog.ts`, when custom logic is required, `Markdown.create` is used to obtain a custom `marked` instance. This is the correct and intended usage of the `Markdown` helper in such cases.
🪛 Biome
web/helpers/markdown.ts
[error] 17-42: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (3)
web/helpers/markdown.ts (3)
1-2: LGTM! Good choice of dependencies.
The imports are well-chosen:
isomorphic-dompurifyensures HTML sanitization works in all environments- Explicit type imports from
markedshow good TypeScript practices
4-11: LGTM! Secure implementation with good documentation.
The default extension is well-implemented:
- Uses postprocess hook to sanitize all HTML output
- Good comment explaining the function syntax requirement
16-42: LGTM! Well-designed API with good documentation.
While static analysis suggests avoiding static-only classes, this design is justified:
- Provides a clean API for both simple (
parse) and custom (create) use cases - Maintains a singleton instance for consistent behavior
- Allows extensibility through the
createmethod
The implementation is clean and well-documented.
Let's verify the usage patterns:
✅ Verification successful
Static-only class design is well-justified by actual usage patterns
The codebase analysis confirms the design is appropriate:
- Simple use case:
Markdown.parse()is used in notifications and tests for straightforward markdown rendering - Custom use case:
Markdown.create()is utilized in changelog handling with custom base URL configuration - Singleton pattern proves valuable for consistent behavior across different parts of the application
The implementation successfully serves both simple and advanced use cases while maintaining clean API design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage patterns of the Markdown helper
echo "Checking for Markdown.parse usage (simple cases):"
rg "Markdown\.parse" --type ts --type vue
echo -e "\nChecking for Markdown.create usage (custom cases):"
rg "Markdown\.create" --type ts --type vue
Length of output: 370
Script:
#!/bin/bash
# Description: Verify usage patterns of the Markdown helper with correct file type flags
echo "Checking for Markdown.parse usage (simple cases):"
rg "Markdown\.parse" -t ts -t typescript -t tsx
echo -e "\nChecking for Markdown.create usage (custom cases):"
rg "Markdown\.create" -t ts -t typescript -t tsx
# Also check for any imports of this class
echo -e "\nChecking for Markdown imports:"
rg "import.*Markdown.*from.*helpers/markdown" -t ts -t typescript -t tsx
Length of output: 583
Script:
#!/bin/bash
# Description: Verify usage patterns of the Markdown helper without file type flags
echo "Checking for Markdown.parse usage (simple cases):"
rg "Markdown\.parse"
echo -e "\nChecking for Markdown.create usage (custom cases):"
rg "Markdown\.create"
# Also check for any imports of this class
echo -e "\nChecking for Markdown imports:"
rg "import.*Markdown.*from.*['\"].*markdown"
Length of output: 1068
🧰 Tools
🪛 Biome
[error] 17-42: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
pujitm
force-pushed
the
feat/md-notifs
branch
from
d1b5c54 to
c153800
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
TBH this is what I would have done. As long as the changelog content comes back and you clicked various link types then it should be good to go. |
3 participants
Summary by CodeRabbit
New Features
Markdownhelper for improved markdown parsing and rendering.Bug Fixes
Chores
isomorphic-dompurifydependency for improved security in HTML rendering.Markdownmodule to ensure functionality and security.Markdownhelper for better performance.