Implement Custom CSS Feature #2146
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive feature for custom CSS in the resume builder application. The changes span multiple files and components, enabling users to apply custom CSS styles dynamically. Key modifications include adding a new CSS section in the right sidebar, implementing a code editor for CSS input, updating metadata schemas, and ensuring custom CSS can be applied during PDF generation and in the artboard view. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🪧 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 (
|
|
View your CI Pipeline Execution ↗ for commit eab996f.
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
apps/client/src/pages/builder/sidebars/right/sections/css.tsx (1)
22-25: Consider bundling Prism themes.Loading Prism themes from external files could cause flash of unstyled content. Consider bundling the themes with the application.
- {isDarkMode && <link rel="stylesheet" href="/styles/prism-dark.css" />} - {!isDarkMode && <link rel="stylesheet" href="/styles/prism-light.css" />} + import "prismjs/themes/prism-dark.css"; + import "prismjs/themes/prism.css";apps/client/src/pages/builder/sidebars/right/index.tsx (1)
91-97: Consider adding ARIA label for better accessibility.The section icon for Custom CSS could benefit from an ARIA label to improve screen reader support.
<SectionIcon id="css" name={t`Custom CSS`} + aria-label={t`Navigate to Custom CSS section`} onClick={() => { scrollIntoView("#css"); }} />libs/schema/src/sample.ts (1)
311-311: Consider using a more specific CSS selector.Using the universal selector
*might affect elements that shouldn't have outlines (e.g., icons, images). Consider using a more targeted selector or adding exclusions.- value: "* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}", + value: ".resume-section {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}\n.resume-section * {\n\toutline: inherit;\n}",apps/client/public/templates/json/ditto.json (1)
318-318: Consider performance implications of universal selector.Using the universal selector
*to apply outlines to every element could impact rendering performance, especially for complex documents. Consider limiting the scope to specific container elements or using a more targeted selector.- "value": "* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}", + "value": ".resume-section {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}",apps/client/public/styles/prism-dark.css (1)
62-65: Consider adding CSS custom properties for token colors.Using CSS variables for token colors would make it easier to maintain and switch between themes.
+:root { + --prism-boolean: #00e0e0; + /* Add other token colors as variables */ +} .token.boolean, .token.number { - color: #00e0e0; + color: var(--prism-boolean); }apps/client/public/templates/json/rhyhorn.json (1)
291-291: Consider performance implications of universal selectorThe change from
.sectionto*selector will affect all elements in the document. While this might be useful for debugging, it could impact rendering performance when enabled.Consider using a more specific selector or adding a common class to elements that need the outline:
- "value": "* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}", + "value": "[data-outline] {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}",apps/client/public/templates/json/onyx.json (1)
290-290: Consider performance and visual implications of universal selectorWhile the universal selector
*provides comprehensive outline visualization for debugging, it might impact performance and create visual clutter. Consider:
- Using a more specific selector for better performance
- Adding a comment explaining the debugging purpose
- "value": "* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}", + "value": "/* Debug mode: Visualize element boundaries */\n.resume-section, .resume-section * {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}",apps/client/public/templates/json/azurill.json (1)
292-292: Add a comment explaining the debugging purpose of this CSS.Consider adding a comment in the metadata to clarify that this outline styling is intended for development/debugging purposes. This would help other developers understand why the universal selector is being used.
"css": { "value": "* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}", - "visible": false + "visible": false, + "description": "Debug outline to visualize element boundaries during development" },apps/client/public/templates/json/gengar.json (1)
292-292: Consider centralizing shared template configurations.Since this debugging CSS is identical across all templates, consider extracting it into a shared configuration file. This would make future updates easier and ensure consistency.
You could:
- Create a shared metadata configuration file
- Import shared settings into individual templates
- Allow templates to override specific values when needed
This would reduce duplication and make the codebase more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
apps/artboard/src/pages/artboard.tsx(1 hunks)apps/client/public/styles/prism-dark.css(1 hunks)apps/client/public/styles/prism-light.css(1 hunks)apps/client/public/templates/json/azurill.json(1 hunks)apps/client/public/templates/json/bronzor.json(1 hunks)apps/client/public/templates/json/chikorita.json(1 hunks)apps/client/public/templates/json/ditto.json(1 hunks)apps/client/public/templates/json/gengar.json(1 hunks)apps/client/public/templates/json/glalie.json(1 hunks)apps/client/public/templates/json/kakuna.json(1 hunks)apps/client/public/templates/json/leafish.json(1 hunks)apps/client/public/templates/json/nosepass.json(1 hunks)apps/client/public/templates/json/onyx.json(1 hunks)apps/client/public/templates/json/pikachu.json(1 hunks)apps/client/public/templates/json/rhyhorn.json(1 hunks)apps/client/src/pages/builder/sidebars/right/index.tsx(3 hunks)apps/client/src/pages/builder/sidebars/right/sections/css.tsx(1 hunks)apps/client/src/pages/builder/sidebars/right/shared/section-icon.tsx(3 hunks)apps/server/src/printer/printer.service.ts(1 hunks)libs/schema/src/metadata/index.ts(2 hunks)libs/schema/src/sample.ts(1 hunks)package.json(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/client/public/styles/prism-light.css
🧰 Additional context used
🪛 Biome (1.9.4)
apps/client/public/styles/prism-dark.css
[error] 113-113: Don't use unknown media feature names.
Unexpected unknown media feature name.
You should use media feature names defined in the CSS Specifications.
(lint/correctness/noUnknownMediaFeatureName)
🔇 Additional comments (14)
apps/client/src/pages/builder/sidebars/right/shared/section-icon.tsx (1)
2-2: LGTM! Icon and type changes follow existing patterns.The changes correctly integrate the CSS section icon by:
- Importing the Code icon
- Extending MetadataKey type
- Adding the CSS case to getSectionIcon
Also applies to: 23-23, 50-52
apps/client/src/pages/builder/sidebars/right/index.tsx (1)
8-8: LGTM! The CSS section is well-integrated into the sidebar.The placement between Theme and Page sections is logical and follows the existing component structure.
Also applies to: 41-41
apps/client/public/templates/json/kakuna.json (1)
290-290: Ensure consistent CSS approach across all templates.The change to use the universal selector
*should be consistently applied across all template files. Also, consider the same selector refinement suggested earlier.Run this script to check consistency across templates:
✅ Verification successful
CSS approach is already consistent across templates
The universal selector
*and the outline styles are uniformly applied across all template files. No changes needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check CSS consistency across all template files fd -e json . apps/client/public/templates/json/ -x jq -r '.metadata.css.value' {} \; | sort | uniq -cLength of output: 190
package.json (2)
4-4: Version bump looks appropriate.The version increment from 4.3.4 to 4.3.5 follows semantic versioning for a feature addition.
78-78: Verify the dependencies' compatibility and security.The new dependencies added support the custom CSS feature:
- prismjs for syntax highlighting
- react-simple-code-editor for the code editor interface
- @types/prismjs for TypeScript support
Run the following script to check for any known vulnerabilities or compatibility issues:
Also applies to: 219-219, 230-230
✅ Verification successful
Dependencies are secure and compatible ✅
- prismjs@^1.29.0 is well above all known vulnerable versions
- react-simple-code-editor has no known vulnerabilities
- React 18.x satisfies all peer dependency requirements
- Type definitions are up to date
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and compatibility issues in new dependencies # Check for security advisories echo "Checking security advisories for prismjs..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "prismjs") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' echo "Checking security advisories for react-simple-code-editor..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-simple-code-editor") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check peer dependencies echo "Checking peer dependencies..." npm info [email protected] peerDependencies npm info [email protected] peerDependenciesLength of output: 2189
Script:
#!/bin/bash # Check exact versions of dependencies echo "Checking package.json for exact versions..." cat package.json | jq '.dependencies | { "prismjs": .prismjs, "react": .react, "react-dom": .["react-dom"] }'Length of output: 289
apps/client/public/templates/json/pikachu.json (1)
318-318: Same performance concern as in ditto.jsonapps/client/public/styles/prism-dark.css (2)
1-21: LGTM! Well-structured base styles.The base styles are properly structured with vendor prefixes and essential text formatting properties.
23-41: LGTM! Proper code block styling.The code block styles provide good visual separation and readability with appropriate spacing and border-radius.
apps/client/public/templates/json/rhyhorn.json (1)
291-292: Consistent implementation across templatesThe CSS changes are consistently implemented across all template files, which is good for maintainability. However, the same performance considerations apply to all templates.
Let's verify the consistency of these changes across all template files:
✅ Verification successful
CSS outline implementation verified across all templates
The CSS outline property and its visibility state are consistently implemented across all 12 template files, with identical values and disabled state, confirming good maintainability practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CSS implementation consistency across all template files # Expected: All template files should have the same CSS value and visible state # Search for CSS value in all template files echo "Checking CSS value consistency:" rg -U --multiline '("css": \{[^}]+\})' 'apps/client/public/templates/json/*.json' # Search for visible state echo "Checking visible state consistency:" rg '"visible": false' 'apps/client/public/templates/json/*.json'Length of output: 466
Script:
#!/bin/bash # First, let's find all JSON files that might be templates echo "Looking for JSON template files:" fd -e json . apps/client/public # If we find files, let's examine a sample of their content echo -e "\nChecking file contents for CSS and visible state:" fd -e json . apps/client/public --exec head -n 20 {}Length of output: 6921
Script:
#!/bin/bash # Search for the specific CSS outline property and its visibility state echo "Checking CSS outline implementation:" rg -U --multiline '"value": "\* \{[^}]+outline[^}]+\}"' apps/client/public/templates/json/*.json -A 1 # Count total occurrences to verify consistency echo -e "\nCounting occurrences:" rg -c '"value": "\* \{[^}]+outline[^}]+\}"' apps/client/public/templates/json/*.jsonLength of output: 4302
apps/client/public/templates/json/nosepass.json (1)
309-309: Same CSS selector concern as in onyx.jsonThe universal selector usage has the same implications as discussed in the onyx.json template.
apps/client/public/templates/json/chikorita.json (1)
317-317: Same CSS selector concern as in onyx.jsonThe universal selector usage has the same implications as discussed in the onyx.json template.
#!/bin/bash # Description: Verify consistency of CSS changes across all template files # Test: Check if all template files have the same CSS outline implementation echo "Checking CSS implementation across all templates..." rg -U --multiline 'css":\s*{\s*"value":\s*"\*\s*{[^}]*}"' apps/client/public/templates/json/apps/client/public/templates/json/bronzor.json (1)
317-317: Same CSS selector concern as in onyx.jsonThe universal selector usage has the same implications as discussed in the onyx.json template.
Consider implementing a shared configuration for these debug styles to:
- Maintain consistency across templates
- Make future updates easier
- Allow for template-specific customizations when needed
apps/client/public/templates/json/glalie.json (1)
291-291: Consider the performance implications of using the universal selector.The change from
.sectionto*will apply the outline to every element in the document. While this provides better visibility for debugging, it could impact rendering performance when enabled, especially in documents with many nested elements.Let's verify the potential impact by checking for deeply nested elements in the templates:
apps/client/public/templates/json/leafish.json (1)
292-292: LGTM! Consistent with other templates.The CSS change maintains consistency with other template files, which is important for a unified debugging experience across different resume templates.
| @@ -12,7 +12,7 @@ export const metadataSchema = z.object({ | |||
| template: z.string().default("rhyhorn"), | |||
| layout: z.array(z.array(z.array(z.string()))).default(defaultLayout), // pages -> columns -> sections | |||
| css: z.object({ | |||
| value: z.string().default(".section {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}"), | |||
| value: z.string().default("* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}"), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific CSS selector.
The universal selector * could lead to:
- Performance issues by affecting all elements
- Unintended style overrides
- Lack of style isolation between sections
Consider using a more specific selector like [data-page] .section to scope styles appropriately.
- value: z.string().default("* {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}"),
+ value: z.string().default("[data-page] .section {\n\toutline: 1px solid #000;\n\toutline-offset: 4px;\n}"),Also applies to: 53-53
| <div className="rounded border p-4"> | ||
| <CodeEditor | ||
| tabSize={4} | ||
| value={css.value} | ||
| className="language-css font-mono" | ||
| highlight={(code) => Prism.highlight(code, Prism.languages.css, "css")} | ||
| onValueChange={(value) => { | ||
| setValue("metadata.css.value", value); | ||
| }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Add CSS validation and security measures.
The current implementation lacks:
- CSS validation before applying
- Input size limits
- CSS sanitization
- Error handling for invalid CSS
This could lead to security vulnerabilities or broken styles.
Consider adding:
- CSS validation library
- Size limits on the input
- CSS sanitization
- Error handling for invalid CSS
Example implementation:
+ import { validateCSS } from 'css-validator';
+ import DOMPurify from 'dompurify';
<CodeEditor
tabSize={4}
value={css.value}
className="language-css font-mono"
+ maxLength={5000}
highlight={(code) => Prism.highlight(code, Prism.languages.css, "css")}
onValueChange={(value) => {
+ try {
+ if (value.length > 5000) {
+ throw new Error('CSS exceeds maximum length');
+ }
+ const sanitizedValue = DOMPurify.sanitize(value);
+ if (!validateCSS(sanitizedValue)) {
+ throw new Error('Invalid CSS');
+ }
setValue("metadata.css.value", sanitizedValue);
+ } catch (error) {
+ // Handle error appropriately
+ }
}}
/>Committable suggestion skipped: line range outside the PR's diff.
| return <Outlet />; | ||
| return ( | ||
| <> | ||
| {metadata.css.visible && <style lang="css">{`[data-page] { ${metadata.css.value} }`}</style>} |
There was a problem hiding this comment.
Sanitize CSS before injection to prevent XSS.
Direct string interpolation of user input into a style tag is potentially dangerous. Consider sanitizing the CSS before injection.
+ import DOMPurify from 'dompurify';
- {metadata.css.visible && <style lang="css">{`[data-page] { ${metadata.css.value} }`}</style>}
+ {metadata.css.visible && (
+ <style lang="css">
+ {`[data-page] { ${DOMPurify.sanitize(metadata.css.value, { ALLOWED_TAGS: [], ALLOWED_ATTR: [] })} }`}
+ </style>
+ )}Committable suggestion skipped: line range outside the PR's diff.
| // Apply custom CSS if enabled | ||
| const css = resume.data.metadata.css; | ||
|
|
||
| if (css.visible) { | ||
| await page.evaluate((cssValue: string) => { | ||
| const styleTag = document.createElement("style"); | ||
| styleTag.textContent = cssValue; | ||
| document.head.append(styleTag); | ||
| }, css.value); | ||
| } |
There was a problem hiding this comment.
Add CSS sanitization to prevent security vulnerabilities.
The current implementation directly injects CSS without sanitization. This could potentially lead to CSS-based attacks (e.g., stealing data through CSS injection).
Consider adding CSS sanitization:
if (css.visible) {
await page.evaluate((cssValue: string) => {
const styleTag = document.createElement("style");
- styleTag.textContent = cssValue;
+ // Sanitize CSS before applying
+ const sanitizedCss = sanitizeCss(cssValue);
+ styleTag.textContent = sanitizedCss;
document.head.append(styleTag);
}, css.value);
}Also, consider cleaning up the injected style tag after PDF generation to prevent affecting subsequent pages:
await page.evaluate((temporaryHtml_: string) => {
+ // Clean up injected style tag
+ document.head.querySelector('style:last-child')?.remove();
document.body.innerHTML = temporaryHtml_;
}, temporaryHtml);Committable suggestion skipped: line range outside the PR's diff.
|
The GOAT of resumes has brought back a needed feature! Thanks! |
Custom CSS is back!
Summary by CodeRabbit
Release Notes v4.3.5
New Features
Improvements
Dependencies