OU-1465: feat: parse descriptions as markdown#119
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgbernalp: This pull request references OU-1465 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesMarkdown Rendering Integration
Sequence Diagram(s)sequenceDiagram
participant RunDetailComponents
participant markdown.ts
participant marked
participant DOMPurify
RunDetailComponents->>markdown.ts: Pass Markdown text
markdown.ts->>marked: Parse block or inline Markdown
marked-->>markdown.ts: Return HTML
markdown.ts->>DOMPurify: Sanitize HTML and harden anchors
DOMPurify-->>RunDetailComponents: Return sanitized HTML
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/markdown.ts (1)
4-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDOMPurify hook is a global side effect at import time.
DOMPurify.addHookmutates the shared DOMPurify singleton. Any other module that importsdompurifydirectly will also gettarget="_blank"andrel="noopener noreferrer"on all sanitized links—potentially unexpected. Use an isolated instance to contain the hook.♻️ Use an isolated DOMPurify instance
-import DOMPurify from 'dompurify'; +import createDOMPurify from 'dompurify'; + +const purify = createDOMPurify(window); -DOMPurify.addHook('afterSanitizeAttributes', (node) => { +purify.addHook('afterSanitizeAttributes', (node) => { if (node.tagName === 'A') { node.setAttribute('target', '_blank'); node.setAttribute('rel', 'noopener noreferrer'); } }); export function renderMarkdown(text: string): string { - return DOMPurify.sanitize(marked.parse(text ?? '', { async: false })); + return purify.sanitize(marked.parse(text ?? '', { async: false })); } export function renderMarkdownInline(text: string): string { - return DOMPurify.sanitize(marked.parseInline(text ?? '', { async: false })); + return purify.sanitize(marked.parseInline(text ?? '', { async: false })); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/markdown.ts` around lines 4 - 9, Replace the global DOMPurify hook setup with an isolated DOMPurify instance in the markdown sanitization module, and register the afterSanitizeAttributes hook on that instance only. Ensure the module’s existing sanitization flow uses the isolated instance so direct imports of the shared DOMPurify singleton remain unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/markdown.ts`:
- Around line 4-9: Replace the global DOMPurify hook setup with an isolated
DOMPurify instance in the markdown sanitization module, and register the
afterSanitizeAttributes hook on that instance only. Ensure the module’s existing
sanitization flow uses the isolated instance so direct imports of the shared
DOMPurify singleton remain unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c6c819ac-62c3-4882-9be4-7fc49ca6f2b4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (5)
.ai/spec/how/project-structure.mdpackage.jsonsrc/components/runs/detail/AnalysisSummary.tsxsrc/components/runs/detail/ExecutionSummary.tsxsrc/utils/markdown.ts
jgbernalp
force-pushed
the
parse-markdown-in-rca-description
branch
3 times, most recently
from
f2e49f6 to
4b2543d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/runs/detail/RemediationOptionCard.tsx (1)
139-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
<strong dangerouslySetInnerHTML={...}>directly instead of nesting<Content p>inside<strong>.Nesting a
<p>(viaContent) inside<strong>is semantically unusual and may introduce paragraph margins in the card header flex layout.AnalysisSummary.tsxalready uses the cleaner pattern of puttingdangerouslySetInnerHTMLdirectly on<strong>.♻️ Proposed refactor
- <strong> - <Content - component={ContentVariants.p} - dangerouslySetInnerHTML={{ - __html: renderMarkdownInline(option.title), - }} - /> - </strong> + <strong + dangerouslySetInnerHTML={{ + __html: renderMarkdownInline(option.title), + }} + />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/runs/detail/RemediationOptionCard.tsx` around lines 139 - 147, Update the option title rendering in RemediationOptionCard by removing the nested Content component and applying dangerouslySetInnerHTML directly to the surrounding strong element, matching the existing pattern in AnalysisSummary.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/runs/detail/RemediationOptionCard.tsx`:
- Around line 139-147: Update the option title rendering in
RemediationOptionCard by removing the nested Content component and applying
dangerouslySetInnerHTML directly to the surrounding strong element, matching the
existing pattern in AnalysisSummary.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c842d995-de85-409b-b140-95d47d9ea8f5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (11)
.ai/spec/how/project-structure.mdpackage.jsonsrc/components/runs/RunDetailPage.tsxsrc/components/runs/RunListPage.tsxsrc/components/runs/detail/AnalysisSummary.tsxsrc/components/runs/detail/ExecutionSummary.tsxsrc/components/runs/detail/RemediationOptionCard.tsxsrc/components/runs/detail/VerificationSummary.tsxsrc/models/agenticrun.tssrc/utils/approval.test.tssrc/utils/markdown.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .ai/spec/how/project-structure.md
- src/utils/markdown.ts
- package.json
jgbernalp
force-pushed
the
parse-markdown-in-rca-description
branch
2 times, most recently
from
ea2aba4 to
1476db1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/runs/detail/RemediationOptionCard.tsx (1)
176-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
action.descriptionnot rendered as Markdown — inconsistent with ExecutionSummary.Line 176 renders
action.descriptionas plain text, whileExecutionSummary.tsxline 138 renders the same field throughrenderMarkdownInline. Given the PR objective to parse descriptions as Markdown, this looks like an oversight.💚 Proposed fix
<Label variant="outline" isCompact> {action.type} </Label>{' '} - {action.description} + <span dangerouslySetInnerHTML={{ __html: renderMarkdownInline(action.description) }} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/runs/detail/RemediationOptionCard.tsx` at line 176, Update the description rendering in RemediationOptionCard to pass action.description through the existing renderMarkdownInline helper, matching ExecutionSummary’s handling of the same field while preserving the surrounding card layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/runs/detail/RemediationOptionCard.tsx`:
- Line 176: Update the description rendering in RemediationOptionCard to pass
action.description through the existing renderMarkdownInline helper, matching
ExecutionSummary’s handling of the same field while preserving the surrounding
card layout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2fc3f04b-f1b2-449f-982b-238a95b129f4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (12)
.ai/spec/how/project-structure.mdpackage.jsonsrc/components/runs/RunDetailPage.tsxsrc/components/runs/RunListPage.tsxsrc/components/runs/detail/AnalysisSummary.tsxsrc/components/runs/detail/ExecutionSummary.tsxsrc/components/runs/detail/RemediationOptionCard.tsxsrc/components/runs/detail/VerificationSummary.tsxsrc/global.d.tssrc/models/agenticrun.tssrc/utils/approval.test.tssrc/utils/markdown.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/runs/RunDetailPage.tsx
- .ai/spec/how/project-structure.md
- src/components/runs/RunListPage.tsx
- src/utils/approval.test.ts
- package.json
- src/utils/markdown.ts
- src/models/agenticrun.ts
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
parse-markdown-in-rca-description
branch
from
1476db1 to
488cdeb
Compare
2 participants
This PR parses the descriptions as markdown text, sanitizing them with dompurify.