feat: added prototype code to make ui grey and present a banner to user#39
feat: added prototype code to make ui grey and present a banner to user#39dlabaj wants to merge 3 commits intopatternfly:mainfrom
Conversation
dlabaj
requested review from
a team and
cdcabrera
and removed request for
a team
WalkthroughThis pull request introduces a prototype mode feature for design applications. It adds a CLI command to automate PatternFly prototype setup, including a reusable React banner component, CSS styling, and logic to inject these into existing React projects by modifying entry and component files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
src/prototype.ts (1)
63-69: Multi-line imports may cause incorrect insertion.The import detection only checks if a line starts with
import. Multi-line imports (e.g.,import {\n foo\n} from 'bar';) could result in the CSS import being inserted mid-statement.For most real-world cases this works fine. If you want to handle multi-line imports more robustly, consider tracking brace/quote balance or using a simple AST parser. This is a nice-to-have improvement for edge cases.
🔧 Basic multi-line import tracking
// Find the last import statement + let inMultilineImport = false; for (let i = 0; i < lines.length; i++) { const line = lines[i]; - if (line && line.trim().startsWith('import ')) { + const trimmedLine = line?.trim() ?? ''; + + if (trimmedLine.startsWith('import ')) { + inMultilineImport = !trimmedLine.includes(' from ') || + (trimmedLine.includes('{') && !trimmedLine.includes('}')); lastImportIndex = i; + } else if (inMultilineImport) { + lastImportIndex = i; + if (trimmedLine.includes(' from ') || trimmedLine.endsWith(';')) { + inMultilineImport = false; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prototype.ts` around lines 63 - 69, The loop that finds the last import (the for loop using lines, line, and lastImportIndex) only checks a single line start and can stop inside multi-line imports; update it to detect multi-line import blocks by, when you see a line starting with 'import', advancing a secondary index until the import terminator is reached (e.g., the line containing the closing semicolon or balanced braces/quotes) and then set lastImportIndex to that end index; implement simple token/balance checks (track braces and quote state or stop on a trailing ';' outside quotes/braces) inside that detection so insertion happens after the full import statement.package.json (1)
29-29: Build script uses Unix-specificcpcommand.The
cpcommand won't work on Windows. Consider using a cross-platform alternative.🔧 Cross-platform alternative using Node's copyfile
- "build": "tsc && cp src/prototype.css dist/prototype.css", + "build": "tsc && node -e \"require('fs').copyFileSync('src/prototype.css', 'dist/prototype.css')\"",Alternatively, add a dev dependency like
cpx2orcopyfilesfor a cleaner solution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 29, The build script uses the Unix-only cp command; update the "build" npm script to perform a cross-platform copy of src/prototype.css to dist/prototype.css (replace the cp invocation in the "build" script). You can either invoke a small Node copy (using fs.copyFileSync) from the script or switch to a cross-platform package such as cpx2 or copyfiles and call it in the "build" script; ensure the script still runs tsc first and then copies src/prototype.css to dist/prototype.css.tests/prototype.test.ts (1)
131-144: Consider asserting written content for completeness.This test verifies the correct file is read (
srcIndexPath) but doesn't assert on the written content. Adding a content assertion would strengthen the test.✨ Add content assertion
await runPrototype(testCwd); expect(mockReadFile).toHaveBeenCalledWith(srcIndexPath, 'utf-8'); + expect(mockWriteFile).toHaveBeenCalledWith( + srcIndexPath, + `import React from 'react';\n${CSS_IMPORT}`, + 'utf-8' + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/prototype.test.ts` around lines 131 - 144, Update the test "should prefer src directory when multiple index files are found" to also assert that mockWriteFile was invoked with the correct path and content: after calling runPrototype(testCwd) add an expectation that mockWriteFile was called with srcIndexPath and the expected content (use the existing originalContent or compute the expected transformed content) and the correct encoding if applicable; reference mockWriteFile, srcIndexPath, originalContent and runPrototype when making the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/protoBanner.tsx`:
- Line 6: The Banner instantiation is using an invalid prop; remove the
`default` prop from the <Banner> component in protoBanner.tsx and replace it
with a supported prop such as `status="info"` or `color="blue"` while keeping
the existing `isSticky` prop; locate the Banner JSX (the line with Banner and
isSticky) and update its props accordingly so it uses `status` or `color`
instead of `default`.
---
Nitpick comments:
In `@package.json`:
- Line 29: The build script uses the Unix-only cp command; update the "build"
npm script to perform a cross-platform copy of src/prototype.css to
dist/prototype.css (replace the cp invocation in the "build" script). You can
either invoke a small Node copy (using fs.copyFileSync) from the script or
switch to a cross-platform package such as cpx2 or copyfiles and call it in the
"build" script; ensure the script still runs tsc first and then copies
src/prototype.css to dist/prototype.css.
In `@src/prototype.ts`:
- Around line 63-69: The loop that finds the last import (the for loop using
lines, line, and lastImportIndex) only checks a single line start and can stop
inside multi-line imports; update it to detect multi-line import blocks by, when
you see a line starting with 'import', advancing a secondary index until the
import terminator is reached (e.g., the line containing the closing semicolon or
balanced braces/quotes) and then set lastImportIndex to that end index;
implement simple token/balance checks (track braces and quote state or stop on a
trailing ';' outside quotes/braces) inside that detection so insertion happens
after the full import statement.
In `@tests/prototype.test.ts`:
- Around line 131-144: Update the test "should prefer src directory when
multiple index files are found" to also assert that mockWriteFile was invoked
with the correct path and content: after calling runPrototype(testCwd) add an
expectation that mockWriteFile was called with srcIndexPath and the expected
content (use the existing originalContent or compute the expected transformed
content) and the correct encoding if applicable; reference mockWriteFile,
srcIndexPath, originalContent and runPrototype when making the assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0082bbf5-266b-4730-a452-c62ca658d1fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.jsonsrc/cli.tssrc/components/protoBanner.tsxsrc/index.tssrc/prototype.csssrc/prototype.tstests/prototype.test.ts
|
|
||
| const ProtoBanner: React.FC = () => { | ||
| return ( | ||
| <Banner default isSticky> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PatternFly react-core Banner component variant prop
💡 Result:
In the current version of PatternFly (v6 as of 2026), the @patternfly/react-core Banner component does not have a "variant" prop. Instead, it uses "color" and "status" props for styling. color: 'red' | 'orangered' | 'orange' | 'yellow' | 'green' | 'teal' | 'blue' | 'purple' - Color options, overwritten by status. status: 'success' | 'warning' | 'danger' | 'info' | 'custom' - Status styles that overwrite color. Other props: children (React.ReactNode), className (string), isSticky (boolean, default false), screenReaderText (string). In older versions like PatternFly 4, there was a "variant" prop with values 'default' | 'info' | 'danger' | 'success' | 'warning'. The official documentation at patternfly.org/components/banner lists the React props table confirming no "variant" prop in the latest version. GitHub repo patternfly/patternfly-react main branch demos also align with this.
Citations:
- 1: https://www.patternfly.org/components/banner
- 2: https://patternfly.org/components/banner
- 3: https://github.com/patternfly/patternfly-react
- 4: https://patternfly-react.surge.sh/components/banner
🏁 Script executed:
find . -name "package.json" -type f | head -5 | xargs grep -l "@patternfly/react-core" 2>/dev/nullRepository: patternfly/patternfly-cli
Length of output: 84
🏁 Script executed:
cat package.json | grep -A 2 -B 2 "@patternfly/react-core" 2>/dev/null || echo "Not found in root package.json"Repository: patternfly/patternfly-cli
Length of output: 364
Remove invalid default prop from Banner component.
In PatternFly v6, the Banner component does not use a default prop or variant prop. Use status or color props instead (e.g., status="info" or color="blue"). The isSticky prop is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/protoBanner.tsx` at line 6, The Banner instantiation is using
an invalid prop; remove the `default` prop from the <Banner> component in
protoBanner.tsx and replace it with a supported prop such as `status="info"` or
`color="blue"` while keeping the existing `isSticky` prop; locate the Banner JSX
(the line with Banner and isSticky) and update its props accordingly so it uses
`status` or `color` instead of `default`.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/__tests__/prototype.test.ts (1)
45-145: Add a regression test for quoted custom banner messages.Given the new “custom message” behavior, add a case with quotes/newlines and assert the written App content remains valid JSX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/prototype.test.ts` around lines 45 - 145, Add a regression test that exercises the new "custom message" path by supplying a banner string containing quotes and newlines (use the same test scaffolding that calls runPrototype and the existing mocks: mockPathExists/mockReadFile/mockWriteFile), then assert that runPrototype writes the App (or index) file with the banner inserted in a way that preserves valid JSX (i.e., quotes/newlines are properly escaped or wrapped so the resulting mockWriteFile call's content is still valid JSX). Reference runPrototype, mockReadFile, mockWriteFile and assert on the written content to contain the banner in a JSX-safe form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/prototype.test.ts`:
- Around line 1-24: The tests hang because runPrototype calls inquirer.prompt
but the test file doesn't mock it; add a jest.mock for 'inquirer' in the test
(prototype.test.ts) that supplies a mock prompt implementation returning a
resolved promise with the answers your tests expect (e.g.,
jest.fn().mockResolvedValue({ ... })), so inquirer.prompt is replaced and
runPrototype can proceed without interactive input.
In `@src/prototype.ts`:
- Around line 252-265: The current flow only writes when insertProtoBanner
changes appContent, so an added import from addProtoBannerImport can be lost;
capture the original appContent (e.g., originalAppContent = appContent) before
any modifications, then after calling hasProtoBannerImport/addProtoBannerImport
and insertProtoBanner compare the final content (updatedAppContent) against
originalAppContent and call fs.writeFile(appFile, updatedAppContent, 'utf-8')
whenever they differ; reference hasProtoBannerImport, addProtoBannerImport,
insertProtoBanner, appContent, updatedAppContent, originalAppContent, appFile
and fs.writeFile to locate and implement this change.
- Around line 170-174: The assignment to indentation from the regex capture
match (inside the block handling nextLine) can be undefined under strict TS with
noUncheckedIndexedAccess; change the assignment in the block that uses nextLine
and match so indentation is set to a definite string (e.g., use the capture or a
fallback) — reference the variables/functions: nextLine, match, indentation, and
the /^\s+/ regex — so replace the direct match[1] assignment with a null-safe
expression (e.g., use match[1] ?? '' or match?.[1] ?? '') to ensure indentation
is always a string.
- Around line 39-40: The code uses a hardcoded '/src/' substring to prefer paths
under a src directory (see the matches.find that sets srcMatch and the similar
logic at lines 107-108), which fails on Windows; update both places to normalize
the path and check path segments instead of a literal '/src/'. Use Node's path
utilities (e.g., path.normalize and path.sep) or split the normalized path by
path.sep and test if segments include 'src' so the matches.find reliably detects
a src directory on all OSes.
- Around line 153-154: User-provided customMessage is embedded directly into the
JSX string via messageAttribute and bannerElement which can break the generated
JSX; create and use a small helper (e.g., escapeJsxAttribute) to escape
characters that would break JSX attributes (quotes, ampersand, angle brackets,
newlines) and call that helper when building messageAttribute for ProtoBanner so
the generated JSX is syntactically safe; update the code that computes
messageAttribute to use escapeJsxAttribute(customMessage) before interpolation.
---
Nitpick comments:
In `@src/__tests__/prototype.test.ts`:
- Around line 45-145: Add a regression test that exercises the new "custom
message" path by supplying a banner string containing quotes and newlines (use
the same test scaffolding that calls runPrototype and the existing mocks:
mockPathExists/mockReadFile/mockWriteFile), then assert that runPrototype writes
the App (or index) file with the banner inserted in a way that preserves valid
JSX (i.e., quotes/newlines are properly escaped or wrapped so the resulting
mockWriteFile call's content is still valid JSX). Reference runPrototype,
mockReadFile, mockWriteFile and assert on the written content to contain the
banner in a JSX-safe form.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f68386ae-73d2-4f69-a107-e3691a1c4262
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (3)
src/__tests__/prototype.test.tssrc/components/protoBanner.tsxsrc/prototype.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/protoBanner.tsx
| jest.mock('fs-extra', () => { | ||
| const real = jest.requireActual<typeof import('fs-extra')>('fs-extra'); | ||
| return { | ||
| __esModule: true, | ||
| default: { | ||
| pathExists: jest.fn(), | ||
| readFile: jest.fn(), | ||
| writeFile: jest.fn(), | ||
| existsSync: real.existsSync, | ||
| readFileSync: real.readFileSync, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('glob', () => ({ | ||
| __esModule: true, | ||
| glob: jest.fn(), | ||
| })); | ||
|
|
||
| import path from 'path'; | ||
| import fs from 'fs-extra'; | ||
| import { glob } from 'glob'; | ||
| import { runPrototype } from '../prototype.js'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether inquirer is mocked in this test file
rg -n "jest\\.mock\\('inquirer'|inquirer\\.prompt|runPrototype\\(" src/__tests__/prototype.test.tsRepository: patternfly/patternfly-cli
Length of output: 378
🏁 Script executed:
# Find the prototype implementation file
fd -e ts -e js prototype | grep -v test | head -5Repository: patternfly/patternfly-cli
Length of output: 86
🏁 Script executed:
# Read the full test file to see the current setup
cat -n src/__tests__/prototype.test.ts | head -50Repository: patternfly/patternfly-cli
Length of output: 2204
🏁 Script executed:
# Check the runPrototype implementation
find . -name "prototype.ts" -o -name "prototype.js" | grep -v test | xargs cat 2>/dev/null | head -80Repository: patternfly/patternfly-cli
Length of output: 2206
🏁 Script executed:
# Get more of the prototype.ts file to see if runPrototype calls inquirer.prompt
cat -n src/prototype.ts | grep -A 5 -B 5 "inquirer\|async function runPrototype"Repository: patternfly/patternfly-cli
Length of output: 1200
🏁 Script executed:
# Check if tests actually run or fail without the inquirer mock
tail -100 src/__tests__/prototype.test.ts | head -50Repository: patternfly/patternfly-cli
Length of output: 2532
Mock inquirer.prompt so tests do not hang waiting for user input.
runPrototype calls inquirer.prompt() at line 206 of src/prototype.ts, but this test file lacks a mock for inquirer. All test cases will hang or fail in CI environments without it.
Proposed fix
+jest.mock('inquirer', () => ({
+ __esModule: true,
+ default: {
+ prompt: jest.fn(),
+ },
+}));
...
import path from 'path';
import fs from 'fs-extra';
import { glob } from 'glob';
+import inquirer from 'inquirer';
import { runPrototype } from '../prototype.js';
...
const mockGlob = glob as jest.MockedFunction<typeof glob> & jest.Mock;
+const mockPrompt = inquirer.prompt as jest.MockedFunction<typeof inquirer.prompt> & jest.Mock;
...
beforeEach(() => {
jest.clearAllMocks();
+ mockPrompt.mockResolvedValue({ bannerMessage: '' });
// Suppress console.log and console.error during tests
jest.spyOn(console, 'log').mockImplementation();
jest.spyOn(console, 'error').mockImplementation();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/prototype.test.ts` around lines 1 - 24, The tests hang because
runPrototype calls inquirer.prompt but the test file doesn't mock it; add a
jest.mock for 'inquirer' in the test (prototype.test.ts) that supplies a mock
prompt implementation returning a resolved promise with the answers your tests
expect (e.g., jest.fn().mockResolvedValue({ ... })), so inquirer.prompt is
replaced and runPrototype can proceed without interactive input.
| const srcMatch = matches.find(m => m.includes('/src/')); | ||
| return srcMatch ?? matches[0] ?? null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify hardcoded POSIX separator checks that are platform-fragile
rg -n "includes\\('/src/'\\)" src/prototype.tsRepository: patternfly/patternfly-cli
Length of output: 198
🏁 Script executed:
sed -n '20,50p' src/prototype.tsRepository: patternfly/patternfly-cli
Length of output: 876
🏁 Script executed:
sed -n '95,120p' src/prototype.tsRepository: patternfly/patternfly-cli
Length of output: 690
🏁 Script executed:
sed -n '1,30p' src/prototype.ts | head -20Repository: patternfly/patternfly-cli
Length of output: 654
🏁 Script executed:
cat > /tmp/verify_glob.js << 'EOF'
// Quick verification of how the 'glob' package returns paths
console.log("path.sep on this system:", require('path').sep);
console.log("Example absolute path on this system would use:", require('path').sep);
console.log("\nOn Windows: C:\\Users\\project\\src\\index.tsx would NOT match /src/");
console.log("On Unix: /home/user/project/src/index.tsx WOULD match /src/");
EOF
node /tmp/verify_glob.jsRepository: patternfly/patternfly-cli
Length of output: 275
🏁 Script executed:
# Verify that matches come from glob with absolute: true
rg -A 8 "glob\(globPattern" src/prototype.ts | head -20Repository: patternfly/patternfly-cli
Length of output: 620
Use path-segment checks instead of hardcoded '/src/'.
The glob function with absolute: true returns OS-specific paths: backslash separators on Windows (C:\...\src\...) and forward slashes on Unix (/...../src/...). The hardcoded /src/ check only matches forward slashes, so it fails on Windows and incorrectly falls back to matches[0] instead of preferring the src directory.
Proposed fix
+function isUnderSrc(cwd: string, filePath: string): boolean {
+ const rel = path.relative(cwd, filePath);
+ return rel.split(path.sep).includes('src');
+}
...
- const srcMatch = matches.find(m => m.includes('/src/'));
+ const srcMatch = matches.find(m => isUnderSrc(cwd, m));
return srcMatch ?? matches[0] ?? null;
...
- const srcMatch = matches.find(m => m.includes('/src/'));
+ const srcMatch = matches.find(m => isUnderSrc(cwd, m));
return srcMatch ?? matches[0] ?? null;Also applies to: 107-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/prototype.ts` around lines 39 - 40, The code uses a hardcoded '/src/'
substring to prefer paths under a src directory (see the matches.find that sets
srcMatch and the similar logic at lines 107-108), which fails on Windows; update
both places to normalize the path and check path segments instead of a literal
'/src/'. Use Node's path utilities (e.g., path.normalize and path.sep) or split
the normalized path by path.sep and test if segments include 'src' so the
matches.find reliably detects a src directory on all OSes.
| const messageAttribute = customMessage ? ` message="${customMessage}"` : ''; | ||
| const bannerElement = `<ProtoBanner${messageAttribute} />`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/prototype.ts | sed -n '145,165p'Repository: patternfly/patternfly-cli
Length of output: 908
🏁 Script executed:
cat -n src/prototype.ts | sed -n '148,200p'Repository: patternfly/patternfly-cli
Length of output: 2070
🏁 Script executed:
rg "insertProtoBanner" src/prototype.ts -B 5 -A 5Repository: patternfly/patternfly-cli
Length of output: 766
🏁 Script executed:
cat -n src/prototype.ts | sed -n '192,250p'Repository: patternfly/patternfly-cli
Length of output: 2583
🏁 Script executed:
# Let's verify there's no other escaping or validation happening
rg "customMessage" src/prototype.tsRepository: patternfly/patternfly-cli
Length of output: 449
Escape customMessage before embedding into JSX.
The customMessage parameter originates directly from user input (via inquirer.prompt() at line 206) and is embedded into the JSX string without escaping at line 153. Quotes, newlines, or other special characters in user input would break or corrupt the generated JSX syntax.
Proposed fix
- const messageAttribute = customMessage ? ` message="${customMessage}"` : '';
+ const messageAttribute = customMessage
+ ? ` message={${JSON.stringify(customMessage)}}`
+ : '';
const bannerElement = `<ProtoBanner${messageAttribute} />`;📝 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 messageAttribute = customMessage ? ` message="${customMessage}"` : ''; | |
| const bannerElement = `<ProtoBanner${messageAttribute} />`; | |
| const messageAttribute = customMessage | |
| ? ` message={${JSON.stringify(customMessage)}}` | |
| : ''; | |
| const bannerElement = `<ProtoBanner${messageAttribute} />`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/prototype.ts` around lines 153 - 154, User-provided customMessage is
embedded directly into the JSX string via messageAttribute and bannerElement
which can break the generated JSX; create and use a small helper (e.g.,
escapeJsxAttribute) to escape characters that would break JSX attributes
(quotes, ampersand, angle brackets, newlines) and call that helper when building
messageAttribute for ProtoBanner so the generated JSX is syntactically safe;
update the code that computes messageAttribute to use
escapeJsxAttribute(customMessage) before interpolation.
| const nextLine = lines[i + 1]; | ||
| if (nextLine) { | ||
| const match = nextLine.match(/^(\s+)/); | ||
| if (match) { | ||
| indentation = match[1]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check strict TS option and the offending assignment site
fd -a '^tsconfig.*\.json$' | xargs -I{} sh -c 'echo "== {} =="; cat "{}" | rg -n "noUncheckedIndexedAccess|strict"'
rg -n "indentation = match\\[1\\]" src/prototype.tsRepository: patternfly/patternfly-cli
Length of output: 210
🏁 Script executed:
sed -n '160,180p' src/prototype.ts | cat -nRepository: patternfly/patternfly-cli
Length of output: 774
🏁 Script executed:
rg -n "indentation\s*:" src/prototype.ts | head -20Repository: patternfly/patternfly-cli
Length of output: 51
🏁 Script executed:
rg -B5 "indentation = match" src/prototype.ts | head -30Repository: patternfly/patternfly-cli
Length of output: 290
Fix strict TypeScript assignment for regex capture group.
Line 174 assigns match[1] (type string | undefined with noUncheckedIndexedAccess: true) to indentation: string, causing a build failure.
Proposed fix
- const nextLine = lines[i + 1];
- if (nextLine) {
- const match = nextLine.match(/^(\s+)/);
- if (match) {
- indentation = match[1];
- }
- }
+ const nextLine = lines[i + 1];
+ const match = nextLine?.match(/^(\s+)/);
+ if (match?.[1]) {
+ indentation = match[1];
+ }📝 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 nextLine = lines[i + 1]; | |
| if (nextLine) { | |
| const match = nextLine.match(/^(\s+)/); | |
| if (match) { | |
| indentation = match[1]; | |
| const nextLine = lines[i + 1]; | |
| const match = nextLine?.match(/^(\s+)/); | |
| if (match?.[1]) { | |
| indentation = match[1]; | |
| } |
🧰 Tools
🪛 GitHub Check: build
[failure] 174-174:
Type 'string | undefined' is not assignable to type 'string'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/prototype.ts` around lines 170 - 174, The assignment to indentation from
the regex capture match (inside the block handling nextLine) can be undefined
under strict TS with noUncheckedIndexedAccess; change the assignment in the
block that uses nextLine and match so indentation is set to a definite string
(e.g., use the capture or a fallback) — reference the variables/functions:
nextLine, match, indentation, and the /^\s+/ regex — so replace the direct
match[1] assignment with a null-safe expression (e.g., use match[1] ?? '' or
match?.[1] ?? '') to ensure indentation is always a string.
| // Add ProtoBanner import if not present | ||
| if (!hasProtoBannerImport(appContent)) { | ||
| appContent = addProtoBannerImport(appContent); | ||
| console.log('✅ Added ProtoBanner import to App file.'); | ||
| } else { | ||
| console.log('ℹ️ ProtoBanner import already exists in App file.'); | ||
| } | ||
|
|
||
| // Insert ProtoBanner component | ||
| const updatedAppContent = insertProtoBanner(appContent, customMessage); | ||
|
|
||
| if (updatedAppContent !== appContent) { | ||
| await fs.writeFile(appFile, updatedAppContent, 'utf-8'); | ||
| console.log('✅ Added ProtoBanner component to App.\n'); |
There was a problem hiding this comment.
Persist App import changes even when banner insertion is not possible.
If import is added at Line 254 but insertProtoBanner makes no further change, no write occurs and the new import is lost.
Proposed fix
- let appContent = await fs.readFile(appFile, 'utf-8');
+ let appContent = await fs.readFile(appFile, 'utf-8');
+ let appChanged = false;
// Add ProtoBanner import if not present
if (!hasProtoBannerImport(appContent)) {
appContent = addProtoBannerImport(appContent);
+ appChanged = true;
console.log('✅ Added ProtoBanner import to App file.');
} else {
console.log('ℹ️ ProtoBanner import already exists in App file.');
}
// Insert ProtoBanner component
const updatedAppContent = insertProtoBanner(appContent, customMessage);
- if (updatedAppContent !== appContent) {
- await fs.writeFile(appFile, updatedAppContent, 'utf-8');
- console.log('✅ Added ProtoBanner component to App.\n');
- } else {
- console.log('ℹ️ ProtoBanner component already exists in App.\n');
- }
+ if (updatedAppContent !== appContent) {
+ appContent = updatedAppContent;
+ appChanged = true;
+ console.log('✅ Added ProtoBanner component to App.\n');
+ } else {
+ console.log('ℹ️ ProtoBanner component already exists in App (or insertion point not found).\n');
+ }
+
+ if (appChanged) {
+ await fs.writeFile(appFile, appContent, 'utf-8');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/prototype.ts` around lines 252 - 265, The current flow only writes when
insertProtoBanner changes appContent, so an added import from
addProtoBannerImport can be lost; capture the original appContent (e.g.,
originalAppContent = appContent) before any modifications, then after calling
hasProtoBannerImport/addProtoBannerImport and insertProtoBanner compare the
final content (updatedAppContent) against originalAppContent and call
fs.writeFile(appFile, updatedAppContent, 'utf-8') whenever they differ;
reference hasProtoBannerImport, addProtoBannerImport, insertProtoBanner,
appContent, updatedAppContent, originalAppContent, appFile and fs.writeFile to
locate and implement this change.
There was a problem hiding this comment.
Question: This feature comes across as an outlier to the other CLI options/tools. It's definitely more opinionated and deviates from the "CLI is a gateway to other tools" concept.
Are we opposed to reviewing where this tool might be housed and keeping the CLI a gateway? There's a separation of concerns in the original CLI architecture vs this tool that are throwing me for a loop.
This can be pulled out of the cli if needed. I am expecting to be used more with ai IDEs for prototyping then with the cli. |
2 participants
Added cli command to add grey scale and prototype banner to a project"
Summary by CodeRabbit
prototypeCLI command to automatically set up prototype mode in React projects, applying grayscale styling and a notification bannerProtoBannercomponent to display a customizable sticky notification banner alerting users the app is in prototype mode