feat(patternfly): add PatternFly migration plugin#428
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandkuma77 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new PatternFly plugin is registered and documented: adds plugin manifests, PLUGINS index, README, a ChangesPatternFly plugin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/patternfly/commands/migrate.md`:
- Line 3: Update the /patternfly:migrate command docs to consistently document
the --dry-run flag: add [--dry-run] to the command hint/synopsis and examples
shown (the usages around /patternfly:migrate [file-path], /patternfly:migrate
src/components/Dashboard.tsx, and /patternfly:migrate src/components --dry-run),
add a bullet for `--dry-run` in the Arguments section explaining it analyzes and
reports the migration plan without applying edits, and add bash fence languages
to the code blocks to satisfy MD040; then regenerate/update the related
registries/docs (docs/data.json, PLUGINS.md, plugins/patternfly/README.md) so
the synopsis and argument hints for /patternfly:migrate remain aligned.
In `@plugins/patternfly/README.md`:
- Around line 74-79: Update the PatternFly resource links under the "##
Resources" section to use the stable domain and correct upgrade path: replace
any occurrences of "staging-v6.patternfly.org" with "www.patternfly.org" and
change the migration guide path from "/get-started/migrate-to-v6/" to
"/get-started/upgrade/"; ensure the three token/component/doc links (Token
Reference, Component Examples, PatternFly v6 Documentation) now point to the
corresponding "www.patternfly.org" URLs with the updated paths.
In `@plugins/patternfly/skills/patternfly-migration/SKILL.md`:
- Around line 484-492: Replace the destructive commands in the troubleshooting
entries: under the "Module not found" section remove the `rm -rf node_modules
package-lock.json && npm install` recommendation and instead advise using `npm
ci` to install from the lockfile; under the "Property does not exist" section
remove `npm install typescript@latest` and instead instruct contributors to bump
TypeScript through the repository's standard dependency-update process (e.g.,
open a dependency-update PR and run CI) so upgrades are reviewed and lockfile
changes are explicit.
- Around line 207-212: The markdown references use the staging domain
(staging-v6.patternfly.org); update every occurrence of that domain to the
canonical public site (www.patternfly.org) so links like
"https://staging-v6.patternfly.org/tokens/all-patternfly-tokens" become
"https://www.patternfly.org/tokens/all-patternfly-tokens"; search the SKILL.md
for any "staging-v6.patternfly.org" strings (including in the "Token Migration
Strategy" section and the other referenced locations) and replace them with
"www.patternfly.org", keeping the rest of the URL path and HTTPS scheme intact.
- Around line 53-60: Update the Token Updates section to clarify that the t_
prefix is applied automatically by the PatternFly codemod (not manually) and
emphasize running the codemod command `npx `@patternfly/pf-codemods`@latest --v6
./src --fix`; explicitly mention that React import forms are transformed (e.g.,
`import global_BorderWidth_lg` becomes `import
t_global_border_width_extra_strong`) and that the codemod also converts CSS vars
(e.g., `--pf-v5-global--BorderWidth--lg` →
`--pf-t--global--border--width--extra-strong`), so readers should rely on the
automated tool rather than manually adding the `t_` prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6856ece-f71d-4684-a5ec-5a9656014ec8
📒 Files selected for processing (7)
.claude-plugin/marketplace.jsonPLUGINS.mddocs/data.jsonplugins/patternfly/.claude-plugin/plugin.jsonplugins/patternfly/README.mdplugins/patternfly/commands/migrate.mdplugins/patternfly/skills/patternfly-migration/SKILL.md
| @@ -0,0 +1,94 @@ | |||
| --- | |||
| description: Migrate React components from PatternFly 5 to PatternFly 6 | |||
| argument-hint: "[file-path]" | |||
There was a problem hiding this comment.
Keep --dry-run discoverable or remove the example.
Line 75 advertises --dry-run, but the command hint, synopsis, generated registries, and Arguments section only document [file-path]. If dry-run is supported, include it consistently and define that no edits are applied; otherwise remove the example. While touching this block, add fence languages to satisfy MD040.
📝 Proposed docs update if dry-run is supported
-description: Migrate React components from PatternFly 5 to PatternFly 6
-argument-hint: "[file-path]"
+description: Migrate React components from PatternFly 5 to PatternFly 6
+argument-hint: "[file-path] [--dry-run]"
skill: patternfly-migration
---
@@
-```
-/patternfly:migrate [file-path]
+```bash
+/patternfly:migrate [file-path] [--dry-run]@@
-
/patternfly:migrate src/components/Dashboard.tsx
@@
-
/patternfly:migrate
@@
-
Shows migration plan without applying changes (if user wants to review first)
/patternfly:migrate src/components --dry-run
@@
--$1: (Optional) File path or directory to migrate. If not provided, analyzes the current context or prompts for clarification.
+-$1: (Optional) File path or directory to migrate. If not provided, analyzes the current context or prompts for clarification.
+---dry-run: (Optional) Analyze and report the migration plan without applying edits.
Also regenerate/update `docs/data.json`, `PLUGINS.md`, and `plugins/patternfly/README.md` so their synopsis and argument hints stay aligned.
</details>
Also applies to: 10-13, 59-80
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @plugins/patternfly/commands/migrate.md at line 3, Update the
/patternfly:migrate command docs to consistently document the --dry-run flag:
add [--dry-run] to the command hint/synopsis and examples shown (the usages
around /patternfly:migrate [file-path], /patternfly:migrate
src/components/Dashboard.tsx, and /patternfly:migrate src/components --dry-run),
add a bullet for --dry-run in the Arguments section explaining it analyzes and
reports the migration plan without applying edits, and add bash fence languages
to the code blocks to satisfy MD040; then regenerate/update the related
registries/docs (docs/data.json, PLUGINS.md, plugins/patternfly/README.md) so
the synopsis and argument hints for /patternfly:migrate remain aligned.
</details>
<!-- fingerprinting:phantom:poseidon:ibis -->
<!-- This is an auto-generated comment by CodeRabbit -->
| #### 5. **Token Updates** | ||
| - Global tokens prefixed with `t_` when they reference `--pf-t` variables | ||
| - Many v5 tokens replaced with v6 equivalents | ||
| - Color tokens replaced with `t_temp_dev_tbd` (requires manual update) | ||
| - Examples: | ||
| - `global_BorderWidth_lg` → `global_border_width_extra_strong` | ||
| - `global_FontWeight_normal` → `global_font_weight_body_default` | ||
| - CSS vars: `--pf-v5-global--BorderWidth--lg` → `--pf-t--global--border--width--extra-strong` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -fsSL https://raw.githubusercontent.com/patternfly/pf-codemods/main/README.md \
| grep -E 'global_border_width_extra_strong|global_font_weight_body_default|t_temp_dev_tbd'Repository: openshift-eng/ai-helpers
Length of output: 50
🏁 Script executed:
cat plugins/patternfly/skills/patternfly-migration/SKILL.md | head -70 | tail -20Repository: openshift-eng/ai-helpers
Length of output: 981
🏁 Script executed:
#!/bin/bash
# Check if SKILL.md file exists and get its size
if [ -f "plugins/patternfly/skills/patternfly-migration/SKILL.md" ]; then
wc -l "plugins/patternfly/skills/patternfly-migration/SKILL.md"
echo "---"
sed -n '53,60p' "plugins/patternfly/skills/patternfly-migration/SKILL.md"
else
echo "File not found"
fiRepository: openshift-eng/ai-helpers
Length of output: 588
🌐 Web query:
PatternFly v6 token migration codemod global token examples
💡 Result:
PatternFly v6 provides codemods via the @patternfly/pf-codemods package to automate token migration from v5 to v6, particularly for global tokens. Run with npx @patternfly/pf-codemods@latest --v6 ./path-to-src --fix after updating dependencies to PatternFly 6. Key rules affecting tokens: - Prefix rule: Adds 't_' prefix to React tokens referencing --pf-t variables. Example: In: import color_green_10 from "@patternfly/react-tokens/dist/esm/color_green_10"; Out: import t_color_green_10 from "@patternfly/react-tokens/dist/esm/t_color_green_10"; - tokens-update rule: Updates CSS tokens. Autofixes global non-color tokens (e.g., spacers, font sizes). For global color tokens, replaces with temporary hot pink placeholder t_temp_dev_tbd (or --pf-t--temp--dev--tbd) requiring manual replacement using v6 token docs. Non-global tokens need manual update. Examples (inferred from snippets): In: global_Color_100; global_Color_200; color300; global_BorderColor_100; Out (color): t_temp_dev_tbd as global_Color_100 /* CODEMODS: you should update this color token /; Out (non-color, e.g.): t_global_border_width_regular or similar autofix. Style usage example: In: style={{ color: "var(--pf-v5-global--Color--100)" }} Out: style={{ color: "var(--pf-t--temp--dev--tbd)/ CODEMODS: original v5 color was --pf-v5-global--Color--100 */" }} Manual steps post-codemod: - Replace t_temp_dev_tbd with semantic v6 global tokens from https://www.patternfly.org/tokens/all-patternfly-tokens (prefer semantic like --pf-t--global--background--color--primary--default, avoid palette/base ending in numbers). - No 1:1 mapping; choose semantically (e.g., v5 global_link_Color_hover -> t_global_color_nonstatus_green_clicked). - Non-color example: --pf-v5-global--FontSize--lg -> --pf-t--global--font--size--lg (t_global_font_size_lg). - Import React tokens: import t_global_spacer_sm from '@patternfly/react-tokens/dist/esm/t_global_spacer_sm'; Run codemods iteratively, review flagged issues, remove overrides, check token docs for replacements. Sources confirm up-to-date as of 2026.
Citations:
- 1: https://github.com/patternfly/pf-codemods?tab=readme-ov-file
- 2: https://staging.patternfly.org/releases/upgrade-guide
- 3: https://github.com/patternfly/pf-codemods
- 4: https://www.patternfly.org/get-started/upgrade
- 5: https://www.patternfly.org/tokens/develop-with-tokens
- 6: https://patternfly.org/get-started/upgrade
- 7: https://www.patternfly.org/tokens/all-patternfly-tokens
🌐 Web query:
PatternFly pf-codemods global_BorderWidth_lg global_border_width_extra_strong
💡 Result:
In PatternFly's pf-codemods tool, used for upgrading PatternFly React code (e.g., from v5 to v6), the "tokens-update" rule automatically replaces deprecated token imports like global_BorderWidth_lg from "@patternfly/react-tokens" with the updated global_border_width_extra_strong. This is part of broader token updates where about half of the tokens were replaced with newer ones. The codemod provides autofix for global non-color tokens via --fix flag. Example transformation shown in docs: In: import global_BorderWidth_lg from "@patternfly/react-tokens/dist/esm/global_BorderWidth_lg"; Out: import global_border_width_extra_strong from "@patternfly/react-tokens/dist/esm/global_border_width_extra_strong"; In PatternFly 6, border widths use semantic tokens like --pf-t--global--border--width--extra-strong, which likely corresponds to this React token (following the pattern where React tokens mirror CSS vars with t_ prefix and underscores, e.g., t_global_border_width_extra_strong). To apply: Run npx @patternfly/pf-codemods@latest --v6 ./src --fix (targets v5 to v6 changes).
Citations:
- 1: https://github.com/patternfly/pf-codemods
- 2: https://github.com/patternfly/pf-codemods?tab=readme-ov-file
- 3: https://github.com/patternfly/pf-codemods/
- 4: https://staging-v6.patternfly.org/get-started/upgrade/
🏁 Script executed:
#!/bin/bash
# Check if there are other similar misleading statements in SKILL.md about token prefixing
rg -n "Global tokens|prefixed with.*t_|React.*token" plugins/patternfly/skills/patternfly-migration/SKILL.md -A 2 -B 2Repository: openshift-eng/ai-helpers
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Verify the exact import forms that would result from applying these rules
# by checking if there are code examples elsewhere in the file
sed -n '1,100p' plugins/patternfly/skills/patternfly-migration/SKILL.md | grep -E "import|from.*react-tokens" -A 1 -B 1Repository: openshift-eng/ai-helpers
Length of output: 508
Clarify that the t_ prefix is applied by the automated codemod, not manually.
The statement "Global tokens prefixed with t_ when they reference --pf-t variables" is accurate per PatternFly's codemod behavior, but the examples (global_BorderWidth_lg → global_border_width_extra_strong) only show token name updates and don't illustrate the React import form. The codemod's prefix rule automatically adds t_ to React token imports: import global_BorderWidth_lg → import t_global_border_width_extra_strong. The guidance should emphasize running the automated codemod (npx @patternfly/pf-codemods@latest --v6 ./src --fix) rather than manually applying these transformation rules, to avoid confusion about when and how the t_ prefix applies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/patternfly/skills/patternfly-migration/SKILL.md` around lines 53 -
60, Update the Token Updates section to clarify that the t_ prefix is applied
automatically by the PatternFly codemod (not manually) and emphasize running the
codemod command `npx `@patternfly/pf-codemods`@latest --v6 ./src --fix`;
explicitly mention that React import forms are transformed (e.g., `import
global_BorderWidth_lg` becomes `import t_global_border_width_extra_strong`) and
that the codemod also converts CSS vars (e.g., `--pf-v5-global--BorderWidth--lg`
→ `--pf-t--global--border--width--extra-strong`), so readers should rely on the
automated tool rather than manually adding the `t_` prefix.
Adds new plugin for migrating React components from PatternFly v5 to v6. Includes: - /patternfly:migrate command for automated migration analysis - MigrationPf5ToPf6 skill with comprehensive v6 migration knowledge - Component replacements, prop updates, and import path migrations - Manual verification guidance for structural changes
- Replace staging-v6.patternfly.org with www.patternfly.org - Update migration guide path from /migrate-to-v6/ to /upgrade/ - Replace destructive npm commands with safer alternatives: - Use npm ci instead of rm -rf node_modules - Recommend dependency-update PR process instead of npm install @latest
anandkuma77
force-pushed
the
add-patternfly-plugin
branch
from
8ba5a43 to
cf5f44f
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugins/patternfly/commands/migrate.md (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
--dry-runconsistently and add fence languages.Line 75 advertises
--dry-run, but Line 3/Line 12/Line 80 don’t document it, and code fences at Line 11, Line 62, Line 68, and Line 74 are missing language tags.📝 Suggested doc alignment
-argument-hint: "[file-path]" +argument-hint: "[file-path] [--dry-run]" @@ -``` -/patternfly:migrate [file-path] +```bash +/patternfly:migrate [file-path] [--dry-run]@@
@@
@@
@@
$1: (Optional) File path or directory to migrate. If not provided, analyzes the current context or prompts for clarification.
+---dry-run: (Optional) Analyze and report the migration plan without applying edits.</details> Also applies to: 11-13, 62-76, 79-80 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@plugins/patternfly/commands/migrate.mdat line 3, Update the migrate command
docs in plugins/patternfly/commands/migrate.md to consistently document the
--dry-run flag and add language tags to the code fences: include --dry-run in
the usage line for /patternfly:migrate, add a bullet describing--dry-run
(“Analyze and report the migration plan without applying edits”), and change all
triple-backtick fences shown (the examples around /patternfly:migrate
[file-path], /patternfly:migrate src/components/Dashboard.tsx,
/patternfly:migrate, and /patternfly:migrate src/components --dry-run) to use
bash fences (```bash) so examples and the flag description are aligned and
consistent.</details> </blockquote></details> <details> <summary>plugins/patternfly/skills/patternfly-migration/SKILL.md (1)</summary><blockquote> `54-60`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Align token examples with the `t_` prefix rule.** Line 54 says the migration uses `t_`-prefixed token forms, but Line 58-59 examples omit that prefix. This contradiction can cause incorrect manual edits during migration. <details> <summary>📝 Suggested clarification</summary> ```diff -- Global tokens prefixed with `t_` when they reference `--pf-t` variables +- React token imports are automatically prefixed with `t_` by codemods when targeting `--pf-t` variables @@ - - `global_BorderWidth_lg` → `global_border_width_extra_strong` - - `global_FontWeight_normal` → `global_font_weight_body_default` + - `global_BorderWidth_lg` → `t_global_border_width_extra_strong` + - `global_FontWeight_normal` → `t_global_font_weight_body_default` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/patternfly/skills/patternfly-migration/SKILL.md` around lines 54 - 60, The examples in the migration note contradict the stated `t_` prefix rule: update the examples so they include the `t_` prefix (e.g., change the example mappings such as `global_BorderWidth_lg` → `global_border_width_extra_strong` and `global_FontWeight_normal` → `global_font_weight_body_default`, and the CSS var `--pf-v5-global--BorderWidth--lg` → `--pf-t--global--border--width--extra-strong`) to consistently show `t_`-prefixed token names and `--pf-t` CSS vars; ensure all example tokens in the SKILL.md section reference `t_`-prefixed forms to match the rule described. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@plugins/patternfly/commands/migrate.md:
- Line 3: Update the migrate command docs in
plugins/patternfly/commands/migrate.md to consistently document the --dry-run
flag and add language tags to the code fences: include --dry-run in the usage
line for /patternfly:migrate, add a bullet describing--dry-run(“Analyze and
report the migration plan without applying edits”), and change all
triple-backtick fences shown (the examples around /patternfly:migrate
[file-path], /patternfly:migrate src/components/Dashboard.tsx,
/patternfly:migrate, and /patternfly:migrate src/components --dry-run) to use
bash fences (```bash) so examples and the flag description are aligned and
consistent.In
@plugins/patternfly/skills/patternfly-migration/SKILL.md:
- Around line 54-60: The examples in the migration note contradict the stated
t_prefix rule: update the examples so they include thet_prefix (e.g.,
change the example mappings such asglobal_BorderWidth_lg→
global_border_width_extra_strongandglobal_FontWeight_normal→
global_font_weight_body_default, and the CSS var
--pf-v5-global--BorderWidth--lg→
--pf-t--global--border--width--extra-strong) to consistently show
t_-prefixed token names and--pf-tCSS vars; ensure all example tokens in
the SKILL.md section referencet_-prefixed forms to match the rule described.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `6130a91d-c76a-4278-a6b8-484e92c2f3a5` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c0d829088092404f8b1d7649dab30c741b9d1629 and cf5f44fabd62a54f20c5ef720c55de0510c89b16. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `.claude-plugin/marketplace.json` * `PLUGINS.md` * `docs/data.json` * `plugins/patternfly/.claude-plugin/plugin.json` * `plugins/patternfly/README.md` * `plugins/patternfly/commands/migrate.md` * `plugins/patternfly/skills/patternfly-migration/SKILL.md` </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * .claude-plugin/marketplace.json * PLUGINS.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * plugins/patternfly/.claude-plugin/plugin.json * docs/data.json </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/patternfly/commands/migrate.md (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the
--dry-runflag or remove the example.Line 75 demonstrates
--dry-run, but the frontmatterargument-hint(line 3), synopsis (line 12), and Arguments section (lines 79-80) do not document it. Either add--dry-runto the argument-hint, synopsis, and Arguments section with a description, or remove the dry-run example.📝 Proposed fix if --dry-run is supported
--- description: Migrate PatternFly v5 to v6 (React components, CSS classes, tokens) -argument-hint: "[file-path]" +argument-hint: "[file-path] [--dry-run]" skill: pf-react-migration ---## Synopsis -``` -/patternfly:migrate [file-path] +```bash +/patternfly:migrate [file-path] [--dry-run]```diff ## Arguments - `$1`: (Optional) File path or directory to migrate. If not provided, analyzes the current context or prompts for clarification. +- `--dry-run`: (Optional) Analyze and report the migration plan without applying edits.Also applies to: 75-77, 79-80
🤖 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 `@plugins/patternfly/commands/migrate.md` at line 3, The example uses a --dry-run flag but the frontmatter argument-hint, synopsis, and Arguments section lack it; either remove the dry-run example or add support documentation: update the frontmatter entry `argument-hint` to include "[--dry-run]" (in plugins/patternfly/commands/migrate.md), update the synopsis line to show `/patternfly:migrate [file-path] [--dry-run]`, and add an Arguments entry describing `--dry-run` (boolean flag that performs a non-destructive preview) alongside the existing file-path description so the example, synopsis, and frontmatter are consistent.
🤖 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.
Inline comments:
In `@plugins/patternfly/commands/migrate.md`:
- Around line 11-13: Add the bash language specifier to all fenced code blocks
in the Synopsis and Examples for the /patternfly:migrate command so they become
```bash fences; specifically update the block containing "/patternfly:migrate
[file-path]" and the example blocks (e.g., "/patternfly:migrate
src/components/Dashboard.tsx", "/patternfly:migrate", "/patternfly:migrate
src/components --dry-run") to use ```bash at the opening fence so markdownlint
MD040 is satisfied.
---
Duplicate comments:
In `@plugins/patternfly/commands/migrate.md`:
- Line 3: The example uses a --dry-run flag but the frontmatter argument-hint,
synopsis, and Arguments section lack it; either remove the dry-run example or
add support documentation: update the frontmatter entry `argument-hint` to
include "[--dry-run]" (in plugins/patternfly/commands/migrate.md), update the
synopsis line to show `/patternfly:migrate [file-path] [--dry-run]`, and add an
Arguments entry describing `--dry-run` (boolean flag that performs a
non-destructive preview) alongside the existing file-path description so the
example, synopsis, and frontmatter are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 055e5c3a-fc06-4d3e-823e-a23ca14e6b61
📒 Files selected for processing (4)
plugins/patternfly/.claude-plugin/plugin.jsonplugins/patternfly/README.mdplugins/patternfly/commands/migrate.mdplugins/patternfly/skills/patternfly-migration/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- plugins/patternfly/skills/patternfly-migration/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/patternfly/.claude-plugin/plugin.json
| ``` | ||
| /patternfly:migrate [file-path] | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to code blocks.
The code blocks in Synopsis and Examples sections should specify bash as the language for proper syntax highlighting and to satisfy the markdownlint MD040 rule.
📝 Proposed fix
## Synopsis
-```
+```bash
/patternfly:migrate [file-path]
```diff
1. **Migrate a single component file**:
- ```
+ ```bash
/patternfly:migrate src/components/Dashboard.tsx
```
2. **Migrate current working directory**:
- ```
+ ```bash
/patternfly:migrate
``` 3. **Analyze before migrating**:
- ```
+ ```bash
/patternfly:migrate src/components --dry-run
```Also applies to: 62-64, 68-70, 74-77
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@plugins/patternfly/commands/migrate.md` around lines 11 - 13, Add the bash
language specifier to all fenced code blocks in the Synopsis and Examples for
the /patternfly:migrate command so they become ```bash fences; specifically
update the block containing "/patternfly:migrate [file-path]" and the example
blocks (e.g., "/patternfly:migrate src/components/Dashboard.tsx",
"/patternfly:migrate", "/patternfly:migrate src/components --dry-run") to use
```bash at the opening fence so markdownlint MD040 is satisfied.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@plugins/patternfly/skills/pf-glass-theme/scripts/glass-theme-enhanced.css`:
- Around line 64-84: The badge pseudo-element on the selector .pf-v6-theme-glass
.pf-v6-c-page__main-section:first-child::before (and the related pseudo-element
at lines 86-91) is always visible; make it opt-in by default and only show when
an explicit opt-in modifier is present (e.g., a class or data-attribute on the
theme root). Update the CSS so the existing rules set the badge to be hidden by
default (display:none or opacity:0 and pointer-events:none) and add a second
rule that targets the opt-in modifier (for example
.pf-v6-theme-glass.pf-v6-theme-glass--show-badge
.pf-v6-c-page__main-section:first-child::before) to restore the current visible
styles; apply the same pattern to the other pseudo-element at 86-91 so both
badges only appear when the opt-in modifier is present.
In `@plugins/patternfly/skills/pf-glass-theme/scripts/GlassThemeToggle.tsx`:
- Around line 15-17: The state initializer for isGlassEnabled currently reads
document.documentElement during render which breaks SSR; change the
initialization to a safe default (e.g., false) and move the DOM check into a
React.useEffect that runs on mount to call
setIsGlassEnabled(document.documentElement.classList.contains('pf-v6-theme-glass'));
update any logic that relied on the synchronous initial value to handle the
deferred update (use effect to run once). Refer to the isGlassEnabled state and
setIsGlassEnabled in GlassThemeToggle and add a React.useEffect(...) that
performs the document.classList.contains check.
In `@plugins/patternfly/skills/pf-glass-theme/scripts/useGlassTheme.ts`:
- Around line 27-33: The cleanup currently blindly removes global classes
(pf-v6-theme-glass and pf-v6-theme-redhat) which can break other consumers;
change useGlassTheme to track ownership with a reference count so classes are
only removed when the last consumer unmounts: on mount increment a shared
counter (stored on the html element via a dataset key or a module-scoped WeakMap
keyed by document.documentElement), add classes if count becomes 1, and on
unmount decrement the counter and only call
htmlElement.classList.remove('pf-v6-theme-glass') and
htmlElement.classList.remove('pf-v6-theme-redhat') (for variant === 'redhat')
when the counter reaches 0. Ensure the counter references the same key for both
adding and removing in the useGlassTheme hook and handle absence/initialization
of the counter safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8cfecb22-3ecb-49d9-a3e7-7d6623d10767
📒 Files selected for processing (10)
.claude-plugin/marketplace.jsonPLUGINS.mddocs/data.jsonplugins/patternfly/OWNERSplugins/patternfly/skills/pf-glass-theme/README.mdplugins/patternfly/skills/pf-glass-theme/SKILL.mdplugins/patternfly/skills/pf-glass-theme/scripts/GlassThemeToggle.tsxplugins/patternfly/skills/pf-glass-theme/scripts/glass-example.tsxplugins/patternfly/skills/pf-glass-theme/scripts/glass-theme-enhanced.cssplugins/patternfly/skills/pf-glass-theme/scripts/useGlassTheme.ts
✅ Files skipped from review due to trivial changes (2)
- plugins/patternfly/OWNERS
- PLUGINS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude-plugin/marketplace.json
| .pf-v6-theme-glass .pf-v6-c-page__main-section:first-child::before { | ||
| content: 'Glass Theme Active'; | ||
| position: fixed; | ||
| bottom: 16px; | ||
| right: 16px; | ||
| padding: 8px 16px; | ||
| background: rgba(255, 255, 255, 0.6); | ||
| backdrop-filter: blur(12.5px); | ||
| -webkit-backdrop-filter: blur(12.5px); | ||
| color: rgba(0, 0, 0, 0.85); | ||
| border: 1px solid rgba(0, 0, 0, 0.1); | ||
| border-radius: 20px; | ||
| font-size: 11px; | ||
| font-weight: 600; | ||
| letter-spacing: 0.3px; | ||
| z-index: 9999; | ||
| box-shadow: 0 2px 8px rgba(0, 0, 0, 0.15); | ||
| pointer-events: none; | ||
| user-select: none; | ||
| opacity: 0.5; | ||
| } |
There was a problem hiding this comment.
Make the indicator badge truly opt-in.
The badge is currently always shown for glass theme users, even though this section is documented as optional.
Suggested fix
-.pf-v6-theme-glass .pf-v6-c-page__main-section:first-child::before {
+.pf-v6-theme-glass.pf-v6-theme-glass-debug .pf-v6-c-page__main-section:first-child::before {
@@
-.pf-v6-theme-glass.pf-v6-theme-dark .pf-v6-c-page__main-section:first-child::before {
+.pf-v6-theme-glass.pf-v6-theme-dark.pf-v6-theme-glass-debug .pf-v6-c-page__main-section:first-child::before {Also applies to: 86-91
🤖 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 `@plugins/patternfly/skills/pf-glass-theme/scripts/glass-theme-enhanced.css`
around lines 64 - 84, The badge pseudo-element on the selector
.pf-v6-theme-glass .pf-v6-c-page__main-section:first-child::before (and the
related pseudo-element at lines 86-91) is always visible; make it opt-in by
default and only show when an explicit opt-in modifier is present (e.g., a class
or data-attribute on the theme root). Update the CSS so the existing rules set
the badge to be hidden by default (display:none or opacity:0 and
pointer-events:none) and add a second rule that targets the opt-in modifier (for
example .pf-v6-theme-glass.pf-v6-theme-glass--show-badge
.pf-v6-c-page__main-section:first-child::before) to restore the current visible
styles; apply the same pattern to the other pseudo-element at 86-91 so both
badges only appear when the opt-in modifier is present.
| const [isGlassEnabled, setIsGlassEnabled] = React.useState(() => | ||
| document.documentElement.classList.contains('pf-v6-theme-glass') | ||
| ); |
There was a problem hiding this comment.
Avoid DOM access during initial render.
This state initializer reads document during render, which can crash in SSR/non-browser rendering contexts.
Suggested fix
- const [isGlassEnabled, setIsGlassEnabled] = React.useState(() =>
- document.documentElement.classList.contains('pf-v6-theme-glass')
- );
+ const [isGlassEnabled, setIsGlassEnabled] = React.useState(false);
+
+ React.useEffect(() => {
+ setIsGlassEnabled(
+ document.documentElement.classList.contains('pf-v6-theme-glass')
+ );
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [isGlassEnabled, setIsGlassEnabled] = React.useState(() => | |
| document.documentElement.classList.contains('pf-v6-theme-glass') | |
| ); | |
| const [isGlassEnabled, setIsGlassEnabled] = React.useState(false); | |
| React.useEffect(() => { | |
| setIsGlassEnabled( | |
| document.documentElement.classList.contains('pf-v6-theme-glass') | |
| ); | |
| }, []); |
🤖 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 `@plugins/patternfly/skills/pf-glass-theme/scripts/GlassThemeToggle.tsx` around
lines 15 - 17, The state initializer for isGlassEnabled currently reads
document.documentElement during render which breaks SSR; change the
initialization to a safe default (e.g., false) and move the DOM check into a
React.useEffect that runs on mount to call
setIsGlassEnabled(document.documentElement.classList.contains('pf-v6-theme-glass'));
update any logic that relied on the synchronous initial value to handle the
deferred update (use effect to run once). Refer to the isGlassEnabled state and
setIsGlassEnabled in GlassThemeToggle and add a React.useEffect(...) that
performs the document.classList.contains check.
…ive consumers Implement reference counting to track multiple consumers of glass theme. Classes are only added when first consumer mounts and only removed when last consumer unmounts, preventing breaking other components.
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/patternfly/skills/pf-glass-theme/scripts/useGlassTheme.ts (1)
20-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack class ownership, not just consumer count.
On Line 20 and Line 39 paths, counters assume this hook owns the class lifecycle. If
pf-v6-theme-glass/pf-v6-theme-redhatwere pre-existing, the last unmount still removes them.Proposed minimal fix
useEffect(() => { const htmlElement = document.documentElement; + const hadGlassClass = htmlElement.classList.contains('pf-v6-theme-glass'); // Increment reference counts const glassCount = parseInt(htmlElement.dataset.pfGlassCount || '0', 10) + 1; htmlElement.dataset.pfGlassCount = glassCount.toString(); let redhatCount = 0; + const hadRedhatClass = htmlElement.classList.contains('pf-v6-theme-redhat'); if (variant === 'redhat') { redhatCount = parseInt(htmlElement.dataset.pfGlassRedhatCount || '0', 10) + 1; htmlElement.dataset.pfGlassRedhatCount = redhatCount.toString(); } // Apply classes only if this is the first consumer if (glassCount === 1) { - htmlElement.classList.add('pf-v6-theme-glass'); + if (!hadGlassClass) { + htmlElement.classList.add('pf-v6-theme-glass'); + htmlElement.dataset.pfGlassOwned = '1'; + } else { + htmlElement.dataset.pfGlassOwned = '0'; + } } if (variant === 'redhat' && redhatCount === 1) { - htmlElement.classList.add('pf-v6-theme-redhat'); + if (!hadRedhatClass) { + htmlElement.classList.add('pf-v6-theme-redhat'); + htmlElement.dataset.pfGlassRedhatOwned = '1'; + } else { + htmlElement.dataset.pfGlassRedhatOwned = '0'; + } } // Cleanup: decrement counts and remove classes only when last consumer unmounts return () => { const newGlassCount = parseInt(htmlElement.dataset.pfGlassCount || '1', 10) - 1; - htmlElement.dataset.pfGlassCount = Math.max(0, newGlassCount).toString(); + htmlElement.dataset.pfGlassCount = Math.max(0, newGlassCount).toString(); - if (newGlassCount === 0) { + if (newGlassCount === 0 && htmlElement.dataset.pfGlassOwned === '1') { htmlElement.classList.remove('pf-v6-theme-glass'); } if (variant === 'redhat') { const newRedhatCount = parseInt(htmlElement.dataset.pfGlassRedhatCount || '1', 10) - 1; htmlElement.dataset.pfGlassRedhatCount = Math.max(0, newRedhatCount).toString(); - if (newRedhatCount === 0) { + if (newRedhatCount === 0 && htmlElement.dataset.pfGlassRedhatOwned === '1') { htmlElement.classList.remove('pf-v6-theme-redhat'); } } }; }, [variant]);Also applies to: 30-35, 39-52
🤖 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 `@plugins/patternfly/skills/pf-glass-theme/scripts/useGlassTheme.ts` around lines 20 - 21, The hook currently uses a simple numeric counter (htmlElement.dataset.pfGlassCount) which assumes it "owns" the class lifecycle and can remove theme classes even when they were pre-existing; change this to track ownership per hook instance instead: in useGlassTheme generate a unique instance id, store it in the element dataset (e.g., append to dataset.pfGlassOwners as a comma-separated list or JSON), on mount add your id to that owners list and only add the classes if they were not present, and on cleanup remove your id and only remove the pf-v6-theme-glass / pf-v6-theme-redhat classes when the owners list becomes empty; update all places that read/write dataset.pfGlassCount (and any add/remove class logic) to use this owners list check so pre-existing classes are preserved until no tracked owners remain.
🤖 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.
Duplicate comments:
In `@plugins/patternfly/skills/pf-glass-theme/scripts/useGlassTheme.ts`:
- Around line 20-21: The hook currently uses a simple numeric counter
(htmlElement.dataset.pfGlassCount) which assumes it "owns" the class lifecycle
and can remove theme classes even when they were pre-existing; change this to
track ownership per hook instance instead: in useGlassTheme generate a unique
instance id, store it in the element dataset (e.g., append to
dataset.pfGlassOwners as a comma-separated list or JSON), on mount add your id
to that owners list and only add the classes if they were not present, and on
cleanup remove your id and only remove the pf-v6-theme-glass /
pf-v6-theme-redhat classes when the owners list becomes empty; update all places
that read/write dataset.pfGlassCount (and any add/remove class logic) to use
this owners list check so pre-existing classes are preserved until no tracked
owners remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 604255a9-3f97-4704-b890-a7a0ba4e4c5f
📒 Files selected for processing (1)
plugins/patternfly/skills/pf-glass-theme/scripts/useGlassTheme.ts
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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 kubernetes-sigs/prow repository. |
1 participant
Adds a new patternfly plugin with migration tooling for React components.
New command:
New skill:
Plugin structure:
Why
PatternFly v6 introduces significant breaking changes that require manual code updates. While official codemods handle batch migrations, developers need:
This plugin complements the official @patternfly/pf-codemods by providing intelligent analysis, clear explanations, and manual verification checklists for structural changes that can't be automated.
Summary by CodeRabbit
New Features
Improvements
Documentation