feat(mailing-lists): implement rich text editor and html handling #218
Conversation
- Add Quill rich text editor for mailing list descriptions - Create strip-html pipe for table display sanitization - Implement HTML-aware validators (htmlRequired, htmlMinLength, htmlMaxLength) - Enhance shared service handling with correct prefix logic - Improve empty state when no services configured - Update committee filter labels to use dynamic terminology LFXV2-950 Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new rich-text editor component and strip-html utility/pipe, HTML-aware validators, shared-service naming and form changes in mailing-list flows, updates templates to use the editor and sanitized descriptions, adjusts interfaces to relax required fields, and adds a quill dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as EditorComponent
participant Form as ReactiveForm
participant Validators as HTMLValidators
participant Manage as MailingListManage
participant API as GroupsIO/MailingListService
User->>Editor: type/paste HTML description
Editor->>Form: emit text change
Form->>Validators: validate description (stripHtml -> length/required)
Validators-->>Form: validation result
alt form valid & user submits
Form->>Manage: submit payload (includes service selection)
Manage->>Manage: determine isSharedService & compute prefix
alt needs shared service
Manage->>API: create shared service (with prefix)
API-->>Manage: service created (uid)
end
Manage->>API: prepareMailingListData(service) & create mailing list
API-->>User: creation success
else validation error
Form-->>User: show validation errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In
@apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts:
- Around line 68-80: The computed property groupNameLengthError incorrectly
double-counts the hyphen and treats prefixValue as truthy even when prefix() is
empty; fix by basing the conditional on prefix() (not prefixValue) and compute
the prefix-with-hyphen length only when prefix() is non-empty (prefix().length +
1), otherwise treat it as 0; ensure prefixValue used in the message includes the
hyphen only when prefix() is non-empty so the displayed "uses X of 34 allowed"
count and text are accurate.
In
@apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts:
- Around line 267-282: prepareMailingListData currently assumes the service
parameter is defined and directly accesses service.type and service.uid; make it
defensive by treating service as optional: if the passed service is falsy, fall
back to this.selectedService() (or an explicit default object) before using it,
and change direct accesses to safe accesses (e.g., use the resolvedService
variable and service?.uid ?? '' pattern) so groupName and service_uid
computation never dereference undefined.
In @apps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts:
- Around line 10-18: The transform method in StripHtmlPipe uses
document.createElement which breaks SSR; update transform(value: string | null |
undefined) to first check if typeof document !== 'undefined' and only then use
document.createElement('div') and extract textContent, otherwise fall back to a
safe server-side HTML-stripping approach (e.g., a simple regex strip like
value.replace(/<[^>]+>/g, '') or other non-DOM parsing) so the pipe returns
plain text both on server and client; keep the XSS rationale comment noting this
is a false positive because the element is never attached to the DOM.
🧹 Nitpick comments (2)
apps/lfx-one/src/app/shared/components/editor/editor.component.scss (1)
4-11: LGTM - Editor content styling with Tailwind.The styling correctly applies
text-smto editor content using Tailwind's@applydirective.Note:
::ng-deepis deprecated but still necessary for penetrating third-party component encapsulation. Consider migrating to global styles or CSS custom properties when PrimeNG provides better theming hooks.packages/shared/src/validators/mailing-list.validators.ts (1)
13-18: Consider SSR compatibility forDOMParserusage.
DOMParseris a browser API that may not be available during server-side rendering. If this code runs in an SSR context, it will throw an error.Consider adding a fallback for SSR environments:
🔎 Proposed fix with SSR fallback
function stripHtml(html: string): string { if (!html) return ''; - // Remove HTML tags and decode HTML entities - const doc = new DOMParser().parseFromString(html, 'text/html'); - return doc.body.textContent?.trim() || ''; + // Remove HTML tags and decode HTML entities + if (typeof DOMParser !== 'undefined') { + const doc = new DOMParser().parseFromString(html, 'text/html'); + return doc.body.textContent?.trim() || ''; + } + // Fallback for SSR: simple regex-based stripping + return html.replace(/<[^>]*>/g, '').trim(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
apps/lfx-one/package.jsonapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.htmlapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.htmlapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.htmlapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.htmlapps/lfx-one/src/app/shared/components/editor/editor.component.scssapps/lfx-one/src/app/shared/components/editor/editor.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tspackages/shared/src/interfaces/mailing-list.interface.tspackages/shared/src/validators/mailing-list.validators.ts
🧰 Additional context used
📓 Path-based instructions (9)
apps/lfx-one/src/**/*.component.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.component.html: Use data-testid naming convention: [section]-[component]-[element] for hierarchical structure
Use flex + flex-col + gap-* instead of space-y-* for spacing
Files:
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.htmlapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.htmlapps/lfx-one/src/app/shared/components/editor/editor.component.htmlapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.html
{apps,packages}/**/*.{ts,tsx,js,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx,js,html,scss,css}: License headers are required on all source files
Always prepend 'Generated with Claude Code (https://claude.ai/code)' if Claude Code assisted with the code
Files:
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.htmlapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.htmlapps/lfx-one/src/app/shared/components/editor/editor.component.scsspackages/shared/src/interfaces/mailing-list.interface.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.htmlpackages/shared/src/validators/mailing-list.validators.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.html
apps/lfx-one/**/*.{ts,tsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier configuration with Tailwind integration in lfx-one app
Files:
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.htmlapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.htmlapps/lfx-one/src/app/shared/components/editor/editor.component.scssapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.htmlapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.html
apps/lfx-one/src/**/*.{html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Tailwind CSS with PrimeUI plugin and LFX colors for styling
Files:
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.htmlapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.htmlapps/lfx-one/src/app/shared/components/editor/editor.component.scssapps/lfx-one/src/app/shared/components/editor/editor.component.htmlapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.html
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript interfaces instead of union types for better maintainability
Files:
packages/shared/src/interfaces/mailing-list.interface.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tspackages/shared/src/validators/mailing-list.validators.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.ts
packages/shared/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
All shared types, interfaces, and constants must be centralized in @lfx-one/shared package
Files:
packages/shared/src/interfaces/mailing-list.interface.tspackages/shared/src/validators/mailing-list.validators.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Do not nest ternary expressions
Always use async/await for promises in TypeScript/JavaScript
Use camelCase for variable and function names
Files:
packages/shared/src/interfaces/mailing-list.interface.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tspackages/shared/src/validators/mailing-list.validators.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.ts
apps/lfx-one/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.{ts,tsx}: Use zoneless change detection with signals in Angular 19
Always use direct imports for standalone components - no barrel exports
Use ESLint and Prettier for code quality, following Angular-specific ESLint rules
Files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.ts
apps/lfx-one/src/**/*.component.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.component.ts: All PrimeNG components must be wrapped in LFX components for UI library independence
Add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T23:20:29.281Z
Learnt from: asithade
Repo: linuxfoundation/lfx-v2-ui PR: 206
File: apps/lfx-one/src/app/modules/meetings/components/agenda-template-selector/agenda-template-selector.component.ts:11-15
Timestamp: 2025-12-10T23:20:29.281Z
Learning: For Angular 19+ projects, standalone defaults to true for components, directives, and pipes when not explicitly declared. Therefore, components with an imports array in the Component decorator do not require an explicit standalone: true declaration to be standalone. In reviews, verify that a component without an explicit standalone flag and with an imports array will behave as a standalone component according to Angular 19+ defaults. If you need to enforce explicit standalone behavior for clarity or consistency, you can still add standalone: true, but it is not required by default in Angular 19+. Ensure this guidance is considered when reviewing any component.ts files across the codebase.
Applied to files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/components/editor/editor.component.ts
🧬 Code graph analysis (4)
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.ts (1)
packages/shared/src/constants/committees.constants.ts (1)
COMMITTEE_LABEL(17-20)
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts (2)
packages/shared/src/validators/mailing-list.validators.ts (3)
htmlRequiredValidator(87-98)htmlMinLengthValidator(42-59)htmlMaxLengthValidator(65-82)packages/shared/src/interfaces/mailing-list.interface.ts (2)
GroupsIOService(37-68)CreateMailingListRequest(123-142)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (2)
apps/lfx-one/src/app/shared/components/editor/editor.component.ts (1)
Component(10-49)packages/shared/src/interfaces/mailing-list.interface.ts (1)
GroupsIOService(37-68)
apps/lfx-one/src/app/shared/components/editor/editor.component.ts (1)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (1)
Component(11-81)
🪛 ast-grep (0.40.3)
apps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts
[warning] 14-14: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: temp.innerHTML = value
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 14-14: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: temp.innerHTML = value
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (16)
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.ts (2)
12-12: LGTM - Using shared constant for dynamic terminology.Good use of the
COMMITTEE_LABELconstant for configurable UI labels.
189-189: LGTM - Dynamic label for committee filter.Using
COMMITTEE_LABEL.pluralensures consistent terminology across the application.apps/lfx-one/src/app/shared/components/editor/editor.component.ts (1)
10-49: LGTM - Well-structured editor wrapper component.The component properly wraps PrimeNG's Editor with Angular 19 signal-based inputs/outputs. The toolbar configuration is sensible, and the event handlers correctly propagate changes.
Note: The
dataTestinput is correctly exposed for test targeting. As per coding guidelines, ensure the template applies this asdata-testidattribute for reliable test targeting.apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (1)
33-46: LGTM - Clear shared service detection logic.The
isSharedServicecomputed property correctly identifies shared services by checking both explicit'shared'type and the case where a primary service is used cross-project.apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.ts (1)
16-16: LGTM - StripHtmlPipe integration.Clean addition of the pipe import and registration for sanitizing HTML descriptions in the table display.
Also applies to: 32-32
apps/lfx-one/package.json (1)
56-56: LGTM - Quill dependency added for rich text editor support.The addition of
quill@^2.0.3is appropriate for the new EditorComponent wrapping PrimeNG's Editor. The caret constraint ensures compatibility with future patch and minor releases.apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.html (1)
64-64: LGTM!The
stripHtmlpipe is correctly applied to sanitize rich text content for table display, with appropriate fallback to'-'for empty descriptions. The pattern is consistently used in both Maintainer and PMO views.Also applies to: 114-114
packages/shared/src/interfaces/mailing-list.interface.ts (1)
148-161: LGTM!Making
domain,group_name, andpublicoptional aligns with the shared service creation flow where onlytype,prefix,project_uid, anddomainare provided. This supports the new service creation logic inMailingListManageComponent.apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.html (2)
53-59: LGTM!The
lfx-editorcomponent correctly replaces the textarea for rich text editing. The fixed height and form binding look appropriate.
61-67: Validation error display correctly uses@else ifpattern.The else-if chaining ensures only one error message is shown at a time, which provides a cleaner UX compared to stacking multiple error messages.
apps/lfx-one/src/app/shared/components/editor/editor.component.html (1)
1-54: LGTM! Well-structured editor wrapper component.The component properly wraps PrimeNG's
p-editorfollowing the coding guideline to wrap PrimeNG components for UI library independence. Good use of aria-labels for accessibility on toolbar buttons.apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.html (1)
17-45: LGTM! Good empty state implementation.The empty state provides clear guidance when no mailing list services are configured, with appropriate styling using
flex flex-col gap-*pattern per coding guidelines. The external link properly includestarget="_blank"andrel="noopener noreferrer"for security.packages/shared/src/validators/mailing-list.validators.ts (1)
42-59: LGTM! Well-designed HTML-aware validators.The validators correctly follow Angular's validator conventions by returning error objects matching the built-in
minlength/maxlength/requiredformats. This ensures seamless integration with the template error handling that checkserrors?.['minlength'], etc.Also applies to: 65-82, 87-98
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts (3)
169-169: LGTM! HTML validators correctly applied.The description control now uses
htmlRequiredValidator(),htmlMinLengthValidator(11), andhtmlMaxLengthValidator(500)which properly validate the plain text content length after stripping HTML tags.
60-73: Prefix handling logic is consistent.The prefix now excludes the trailing hyphen in
servicePrefix()(line 63), but it's correctly added:
- When calculating
maxGroupNameLength(line 71):const prefix = this.servicePrefix() + '-';- When constructing
groupNameinprepareMailingListData(line 270):`${prefix}-${formValue.group_name}`This keeps the prefix clean for storage while ensuring correct length calculations and name construction.
57-57: Submit flow is unreachable when bothparentService()is null andavailableServices()is empty due to form visibility control.The
hasNoServices()computed signal (line 53) returns true when no services are available, and the template wraps the entire form/stepper in@if (hasNoServices()) { ... } @else { ... }. This means whenavailableServicesis empty, the form is completely hidden and theonSubmit()button is not accessible, regardless of theparentServicevalue. Therefore, thecreateSharedService()call at line 126 cannot be reached in the scenario where bothparentService()is null andavailableServices()is empty.However, the logic at line 57 remains potentially confusing. Consider clarifying the intent of
needsSharedServiceCreationwith a comment, or adjust the condition to be more explicit about the shared service creation requirement (e.g.,this.parentService() !== nullalone, if the empty services case should never reach submission).
...odules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR implements rich text editing capabilities for mailing list descriptions, replacing plain textarea with a Quill-based WYSIWYG editor. It also enhances shared service handling with improved prefix logic, updates the user experience for service configuration states, and adds HTML-aware validators to properly handle rich content.
Key changes:
- Integrated Quill editor (v2.0.3) with a reusable editor component wrapper for WYSIWYG description editing
- Added HTML sanitization via strip-html pipe and HTML-aware validators (htmlRequired, htmlMinLength, htmlMaxLength)
- Improved shared service creation logic and prefix handling for mailing list email addresses
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Added Quill editor and related dependencies (quill, quill-delta, parchment, lodash-es, fast-diff) |
| apps/lfx-one/package.json | Added quill@^2.0.3 as a dependency |
| packages/shared/src/validators/mailing-list.validators.ts | Added HTML-aware validators and internal stripHtml utility function |
| packages/shared/src/interfaces/mailing-list.interface.ts | Made domain, group_name, and public fields optional in CreateGroupsIOServiceRequest |
| apps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts | New pipe for stripping HTML tags from content for table display |
| apps/lfx-one/src/app/shared/components/editor/ | New reusable Quill editor component with toolbar configuration |
| apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/ | Updated service prefix logic, shared service creation, and form validation with HTML validators |
| apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/ | Updated committee filter label to use dynamic terminology |
| apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/ | Applied strip-html pipe to description display in tables |
| apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/ | Enhanced project context display, improved email preview for shared services, and integrated rich text editor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const prefix = this.servicePrefix() + '-'; | ||
| return 34 - prefix.length; |
There was a problem hiding this comment.
The maxGroupNameLength calculation always adds a hyphen to the prefix, even for non-shared services. This could lead to incorrect length calculations for primary services that don't have a prefix. The hyphen should only be added when calculating for shared services.
| const prefix = this.servicePrefix() + '-'; | |
| return 34 - prefix.length; | |
| const prefix = this.servicePrefix(); | |
| const effectivePrefix = prefix ? `${prefix}-` : ''; | |
| return 34 - effectivePrefix.length; |
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts
Show resolved
Hide resolved
...ules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.html
Show resolved
Hide resolved
...odules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts
Outdated
Show resolved
Hide resolved
Address critical PR review comments from #218: - Create shared stripHtml utility for SSR compatibility - Implements regex-based HTML stripping that works in both browser and Node.js - Replaces DOM-based approaches (document.createElement, DOMParser) - Prevents SSR compilation errors during build - Update StripHtmlPipe to use shared utility - Removes browser-only document.createElement() usage - Ensures mailing list table descriptions render correctly in SSR - Update validators to use shared utility - Removes DOMParser-based implementation - Centralizes HTML stripping logic in one location - Add null safety to prepareMailingListData method - Update signature to accept GroupsIOService | null - Use optional chaining for service.type and service.uid - Prevents runtime errors when service is null Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts:
- Around line 33-46: The shared-service detection in the computed
isSharedService incorrectly treats a primary service as shared when
currentProjectUid is undefined; update the logic in
mailing-list-basic-info.component.ts (computed isSharedService) to handle
missing project context by first deriving currentProjectUid safely from
projectContextService.selectedProject() or selectedFoundation() and if still
undefined return false (or only perform the currentProjectUid !==
service.project_uid check when currentProjectUid is defined), keeping the
existing explicit check for service.type === 'shared' and the
primary-service-as-shared check guarded by a defined currentProjectUid.
In @apps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts:
- Around line 7-14: The Pipe decorator for StripHtmlPipe is missing standalone:
true which prevents importing it into standalone components; update the @Pipe
metadata for the StripHtmlPipe class (the decorator above the StripHtmlPipe
declaration that contains name: 'stripHtml') to include standalone: true so the
pipe can be used in component imports; no changes to the transform method or
stripHtml usage are needed, just add the standalone flag to the @Pipe decorator.
♻️ Duplicate comments (3)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (1)
68-80: Partial fix applied; truthy check issue remains.The double-counting issue from the previous review has been partially addressed—line 74 no longer adds
+1to the length. However, the condition on line 73 still checksprefixValue(which isthis.prefix() + '-') for truthiness. Whenprefix()returns an empty string,prefixValuebecomes"-", which is truthy, causing a confusing error message:"prefix "-" uses 1 of 34 allowed".🔎 Recommended fix
public readonly groupNameLengthError = computed(() => { const maxLength = this.maxGroupNameLength(); - const prefixValue = this.prefix() + '-'; + const prefixValue = this.prefix(); // For shared services, include hyphen in the count - if (this.isSharedService() && prefixValue) { - const prefixWithHyphenLength = prefixValue.length; - return `Name cannot exceed ${maxLength} characters (prefix "${prefixValue}" uses ${prefixWithHyphenLength} of 34 allowed)`; + if (this.isSharedService() && prefixValue.length > 0) { + const prefixWithHyphenLength = prefixValue.length + 1; // +1 for the hyphen separator + return `Name cannot exceed ${maxLength} characters (prefix "${prefixValue}-" uses ${prefixWithHyphenLength} of 34 allowed)`; } // For non-shared services, no prefix consideration return `Name cannot exceed ${maxLength} characters`; });apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts (2)
72-75: Fix maxGroupNameLength calculation for primary services.The calculation always adds a hyphen to the prefix (
this.servicePrefix() + '-'), but primary services don't use a prefix in the group name (line 272). This incorrectly reduces the maximum allowed length by 1 character for primary services.🔎 Proposed fix
public readonly maxGroupNameLength = computed(() => { - const prefix = this.servicePrefix() + '-'; - return 34 - prefix.length; + const service = this.selectedService(); + if (service?.type === 'primary') { + return 34; // No prefix for primary services + } + const prefix = this.servicePrefix(); + return 34 - prefix.length - 1; // -1 for the hyphen separator });
282-282: Fix title field to use the prefixed group name.The
titlefield is set to the rawformValue.group_name, but line 275 shows thatgroupNameincludes the prefix for shared services. This creates an inconsistency where the title won't match the actual group name for shared services.🔎 Proposed fix
return { group_name: groupName, public: formValue.public, type: formValue.type, audience_access: formValue.audience_access, description: formValue.description || '', service_uid: service?.uid ?? '', committees: formValue.committees?.length > 0 ? formValue.committees : undefined, - title: formValue.group_name, + title: groupName, };
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (1)
25-27: LGTM: Fallback chain handles missing project names.The computed property correctly falls back through available project/foundation names with proper optional chaining.
Optional: consider line breaking for readability
The line is quite long (117+ characters). If you prefer, you could break it for readability:
public readonly projectName = computed(() => { - return this.service()?.project_name || this.projectContextService.selectedProject()?.name || this.projectContextService.selectedFoundation()?.name || ''; + return ( + this.service()?.project_name || + this.projectContextService.selectedProject()?.name || + this.projectContextService.selectedFoundation()?.name || + '' + ); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tspackages/shared/src/utils/html-utils.tspackages/shared/src/utils/index.tspackages/shared/src/validators/mailing-list.validators.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript interfaces instead of union types for better maintainability
Files:
packages/shared/src/utils/index.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tspackages/shared/src/utils/html-utils.tspackages/shared/src/validators/mailing-list.validators.ts
packages/shared/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
All shared types, interfaces, and constants must be centralized in @lfx-one/shared package
Files:
packages/shared/src/utils/index.tspackages/shared/src/utils/html-utils.tspackages/shared/src/validators/mailing-list.validators.ts
{apps,packages}/**/*.{ts,tsx,js,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx,js,html,scss,css}: License headers are required on all source files
Always prepend 'Generated with Claude Code (https://claude.ai/code)' if Claude Code assisted with the code
Files:
packages/shared/src/utils/index.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tspackages/shared/src/utils/html-utils.tspackages/shared/src/validators/mailing-list.validators.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: Do not nest ternary expressions
Always use async/await for promises in TypeScript/JavaScript
Use camelCase for variable and function names
Files:
packages/shared/src/utils/index.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.tspackages/shared/src/utils/html-utils.tspackages/shared/src/validators/mailing-list.validators.ts
apps/lfx-one/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.{ts,tsx}: Use zoneless change detection with signals in Angular 19
Always use direct imports for standalone components - no barrel exports
Use ESLint and Prettier for code quality, following Angular-specific ESLint rules
Files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts
apps/lfx-one/src/**/*.component.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.component.ts: All PrimeNG components must be wrapped in LFX components for UI library independence
Add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts
apps/lfx-one/**/*.{ts,tsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier configuration with Tailwind integration in lfx-one app
Files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.tsapps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T23:20:29.281Z
Learnt from: asithade
Repo: linuxfoundation/lfx-v2-ui PR: 206
File: apps/lfx-one/src/app/modules/meetings/components/agenda-template-selector/agenda-template-selector.component.ts:11-15
Timestamp: 2025-12-10T23:20:29.281Z
Learning: For Angular 19+ projects, standalone defaults to true for components, directives, and pipes when not explicitly declared. Therefore, components with an imports array in the Component decorator do not require an explicit standalone: true declaration to be standalone. In reviews, verify that a component without an explicit standalone flag and with an imports array will behave as a standalone component according to Angular 19+ defaults. If you need to enforce explicit standalone behavior for clarity or consistency, you can still add standalone: true, but it is not required by default in Angular 19+. Ensure this guidance is considered when reviewing any component.ts files across the codebase.
Applied to files:
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.tsapps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts
🧬 Code graph analysis (3)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (3)
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts (1)
Component(25-367)apps/lfx-one/src/app/shared/components/editor/editor.component.ts (1)
Component(10-49)packages/shared/src/interfaces/mailing-list.interface.ts (1)
GroupsIOService(37-68)
apps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts (1)
packages/shared/src/utils/html-utils.ts (1)
stripHtml(20-39)
packages/shared/src/validators/mailing-list.validators.ts (1)
packages/shared/src/utils/html-utils.ts (1)
stripHtml(20-39)
🔇 Additional comments (10)
apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts (3)
4-4: LGTM: Import changes align with editor integration.The addition of
inject,ProjectContextService, andEditorComponentimports correctly supports the new rich text editor and shared service logic.Also applies to: 6-7, 13-13
17-18: LGTM: Correct dependency injection pattern.The use of
inject()to initialize theprojectContextServicefollows Angular 19+ best practices.
48-60: LGTM: Email preview correctly handles shared service prefixes.The conditional logic properly formats email addresses for shared vs. non-shared services, aligning with the PR objectives.
packages/shared/src/utils/html-utils.ts (1)
20-39: LGTM! SSR-compatible HTML stripping implementation.The regex-based approach correctly works in both browser and SSR environments, resolving the previous SSR compatibility concerns. The function appropriately handles null/undefined inputs and decodes common HTML entities.
Note: The regex tag stripping is basic and won't handle all edge cases (e.g., malformed HTML,
<in attributes), and entity decoding is limited to common entities. However, this is sufficient for the stated use case of stripping HTML for table displays and validation.packages/shared/src/utils/index.ts (1)
8-8: LGTM!The export follows the established pattern and correctly exposes the new HTML utilities to consumers of the shared package.
apps/lfx-one/src/app/shared/pipes/strip-html.pipe.ts (1)
11-13: LGTM! SSR compatibility issue resolved.The implementation now delegates to the shared
stripHtmlutility, which uses regex-based stripping that works correctly in both browser and SSR environments. This resolves the previous SSR compatibility concerns withdocument.createElement.packages/shared/src/validators/mailing-list.validators.ts (1)
31-87: LGTM! Well-implemented HTML-aware validators.The three new validators correctly:
- Strip HTML tags before validating plain text content
- Return standard Angular validator error formats
- Handle null/undefined inputs appropriately
- Delegate empty value handling to the required validator (for min/max length validators)
The implementation properly separates concerns by delegating HTML stripping to the shared utility.
apps/lfx-one/src/app/modules/mailing-lists/mailing-list-manage/mailing-list-manage.component.ts (3)
57-59: LGTM! Correct shared service creation logic.The logic now correctly uses AND (&&) to check both conditions:
- Parent service exists, AND
- No shared services are available
This filters specifically for shared services, ensuring shared service creation only happens when needed. The previous concern about OR vs AND logic has been properly addressed.
171-171: LGTM! Correct HTML-aware validation.The description field now properly uses the HTML-aware validators (
htmlRequiredValidator,htmlMinLengthValidator,htmlMaxLengthValidator) which strip HTML tags before validating plain text content length. This ensures validation works correctly with the rich text editor.
269-284: LGTM! Null-safe service parameter handling.The method signature now correctly accepts
GroupsIOService | null, and uses optional chaining (service?.type,service?.uid) throughout to safely handle null values. This addresses the previous concern about potential undefined access errors.
...odules/mailing-lists/components/mailing-list-basic-info/mailing-list-basic-info.component.ts
Show resolved
Hide resolved
Summary
Enhance mailing list description handling with rich text editing capabilities and proper HTML content management.
Changes
Rich Text Editor Integration
HTML Content Handling
strip-htmlpipe to sanitize HTML content for display in tablesMailing List UX Improvements
Technical Details
apps/lfx-one/src/app/shared/pipes/strip-html.pipe.tsapps/lfx-one/src/app/shared/components/editor/packages/shared/src/validators/mailing-list.validators.tsquill@^2.0.3Testing
JIRA
LFXV2-950
Generated with Claude Code