Conversation
WalkthroughThis update introduces a new "init" CLI command for initializing Keycloak theme projects, including support for login, account, admin, and email themes. It adds several new utility modules for setting up Vite plugins and ESLint configurations, revises project initialization logic, simplifies theme source directory detection, and improves deterministic sorting in extension metadata and managed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI ("init" command)
participant VitePluginSetup
participant ESLintSetup
participant BuildContext
participant ThemeTypePrompt
participant ThemeInitializer
User->>CLI: Run "init" command
CLI->>VitePluginSetup: setupVitePluginIfNeeded(projectDirPath)
CLI->>BuildContext: getBuildContext(projectDirPath)
CLI->>ESLintSetup: setupEslint(projectDirPath)
CLI->>ThemeTypePrompt: Prompt for theme type (login/account/admin/email)
ThemeTypePrompt->>ThemeInitializer: Initialize selected theme type
ThemeInitializer->>CLI: Complete initialization
CLI->>User: Initialization complete
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/bin/init/init.ts (1)
253-255: Remove redundant type assertion after Zod validation.Similar to the other file, the assertion is redundant after Zod parsing:
- zParsedPackageJson.parse(parsedPackageJson); - - assert(is<ParsedPackageJson>(parsedPackageJson)); + parsedPackageJson = zParsedPackageJson.parse(parsedPackageJson);
🧹 Nitpick comments (7)
src/bin/init/setupVitePluginIfNeeded.ts (3)
24-26: Consider more precise detection of existing plugin configuration.The simple string search for "keycloakify" could produce false positives if the term appears in comments or unrelated contexts. Consider parsing the AST first and checking for the actual import or plugin configuration.
109-113: Consider making the plugin configuration customizable.The
accountThemeImplementation: "none"is hardcoded. Consider accepting configuration options as a parameter to make this function more flexible.-export async function setupVitePluginIfNeeded(params: { projectDirPath: string }) { +export async function setupVitePluginIfNeeded(params: { + projectDirPath: string; + pluginConfig?: Record<string, any>; +}) {
126-126: Use recast.print() instead of babelGenerate for better formatting preservation.Using
babelGeneratedirectly may not preserve the original formatting as well asrecast.print().-let viteConfigTsContent_modified = babelGenerate(root).code; +let viteConfigTsContent_modified = recast.print(root).code;src/bin/init/setupEslint.ts (1)
65-74: Consider refactoring the labeled block pattern.The labeled block pattern, while functional, is flagged by static analysis and is an uncommon pattern in TypeScript. Consider using a more conventional approach:
- format: { - if (!(await getIsPrettierAvailable())) { - break format; - } - - eslintConfigJsContent_modified = await runPrettier({ - sourceCode: eslintConfigJsContent_modified, - filePath: eslintConfigJsFilePath - }); - } + if (await getIsPrettierAvailable()) { + eslintConfigJsContent_modified = await runPrettier({ + sourceCode: eslintConfigJsContent_modified, + filePath: eslintConfigJsFilePath + }); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 65-65: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
src/bin/initialize-login-theme.ts (3)
61-63: Remove redundant type assertion after Zod validation.The
assert(is<ParsedPackageJson>(parsedPackageJson))is redundant since Zod'sparsemethod already ensures type safety. If parsing succeeds, the type is guaranteed.- zParsedPackageJson.parse(parsedPackageJson); - - assert(is<ParsedPackageJson>(parsedPackageJson)); + parsedPackageJson = zParsedPackageJson.parse(parsedPackageJson);
90-166: Consider refactoring multiple labeled blocks.The nested labeled blocks pattern makes the code harder to follow. Consider extracting this logic into a separate function:
async function installStorybookIfNeeded(params: { doInstallStories: boolean; buildContext: BuildContext; parsedPackageJson: ParsedPackageJson; }) { const { doInstallStories, buildContext, parsedPackageJson } = params; if (!doInstallStories || buildContext.bundler !== "vite") { return; } if (Object.keys({ ...parsedPackageJson.dependencies, ...parsedPackageJson.devDependencies }).includes("storybook")) { return; } // ... rest of the storybook installation logic }Then call it from the main function instead of using labeled blocks.
🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 153-153: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
255-297: Consider externalizing the story order configuration.The hardcoded list of ordered pages could become outdated as new pages are added. Consider moving this to a configuration file or generating it dynamically:
Would you like me to help create a configuration file approach or a dynamic solution that reads available story files?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
package.json(1 hunks)src/bin/init/index.ts(1 hunks)src/bin/init/init.ts(1 hunks)src/bin/init/setupEslint.ts(1 hunks)src/bin/init/setupVitePluginIfNeeded.ts(1 hunks)src/bin/initialize-account-theme/initialize-account-theme.ts(1 hunks)src/bin/initialize-login-theme.ts(1 hunks)src/bin/main.ts(1 hunks)src/bin/shared/buildContext.ts(2 hunks)src/bin/shared/customHandler.ts(1 hunks)src/bin/sync-extensions/extensionModuleMeta.ts(1 hunks)src/bin/sync-extensions/managedGitignoreFiles.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/bin/main.ts (5)
src/bin/initialize-account-theme/initialize-account-theme.ts (1)
command(11-92)src/bin/init/init.ts (1)
command(17-354)src/bin/initialize-login-theme.ts (1)
command(16-213)src/bin/initialize-admin-theme.ts (1)
command(6-26)src/bin/initialize-email-theme.ts (1)
command(17-156)
src/bin/init/setupVitePluginIfNeeded.ts (1)
src/bin/tools/runPrettier.ts (2)
getIsPrettierAvailable(13-31)runPrettier(119-161)
🪛 Biome (1.9.4)
src/bin/sync-extensions/extensionModuleMeta.ts
[error] 218-218: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
src/bin/init/setupEslint.ts
[error] 65-65: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
src/bin/init/init.ts
[error] 37-37: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 216-216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 231-231: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 274-274: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
src/bin/init/setupVitePluginIfNeeded.ts
[error] 128-128: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
src/bin/initialize-login-theme.ts
[error] 90-90: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 153-153: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 180-180: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test with Node v18 on ubuntu-latest
🔇 Additional comments (10)
src/bin/sync-extensions/extensionModuleMeta.ts (2)
209-311: LGTM! Deterministic sorting implementation looks good.The sorting changes effectively ensure deterministic output by:
- Sorting installed extension modules by
moduleName- Sorting files by composite key (
isPublic+fileRelativePath)- Sorting peer dependencies alphabetically
These changes align well with the PR objective of improving consistency in extension metadata handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 218-218: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
218-218: The labeled statement is appropriate and can be ignored.The static analysis tool flagged this as suspicious, but the
use_cache:label is a legitimate JavaScript/TypeScript feature being used correctly here to break out of the code block when cached metadata is found. This is a common pattern for early returns in nested blocks.🧰 Tools
🪛 Biome (1.9.4)
[error] 218-218: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
src/bin/sync-extensions/managedGitignoreFiles.ts (2)
76-76: Excellent deterministic sorting implementation.The consistent application of
posixPathCompareFnacross different arrays ensures predictable, ordered output in the generated.gitignorefiles. The sorting of extension modules by name length and lexicographically is a thoughtful approach.Also applies to: 80-85, 99-100
198-219: Well-designed POSIX path comparison function.The implementation correctly handles path sorting by:
- Prioritizing by segment count (depth) for logical ordering
- Using locale-aware comparison with numeric and case-insensitive options
- Handling edge cases appropriately
This function will ensure consistent file path ordering in generated
.gitignorefiles.package.json (1)
3-3: Version bump looks good.The release candidate version (11.8.47-rc.1) is appropriate for testing the new init functionality before a stable release.
src/bin/init/index.ts (1)
1-1: Standard barrel export pattern.Clean module organization using the barrel export pattern.
src/bin/initialize-account-theme/initialize-account-theme.ts (1)
29-29: LGTM: Enhanced user experience with clear prompts.The added console output provides better visual feedback during the CLI interaction, making it clearer what the user is being asked to select and confirming their choice.
Also applies to: 37-37
src/bin/shared/customHandler.ts (1)
13-13: LGTM: Properly extends command name types.The additions of
"initialize-login-theme"and"init"command names align with the new CLI commands being introduced, and the trailing semicolon removal improves syntax consistency.Also applies to: 18-19
src/bin/main.ts (1)
149-161: LGTM: Well-structured new CLI command.The new
initcommand follows the established patterns in the codebase, uses appropriate dynamic imports for code splitting, and correctly defaults the project directory path. The BETA marking appropriately sets user expectations.src/bin/shared/buildContext.ts (1)
168-168:Details
❓ Verification inconclusive
Verify backward compatibility of the simplified theme directory detection.
The removal of the fallback detection logic significantly simplifies the code but could potentially break existing projects that relied on the automatic theme directory discovery. While this aligns with the new
initcommand approach, ensure that all existing valid project structures still work correctly.
🏁 Script executed:
#!/bin/bash # Description: Check for projects that might rely on the removed fallback detection logic # Expected: Find examples of theme directory structures that might be affected # Search for potential theme source directories that don't follow the standard pattern echo "Searching for potential theme directories..." fd -t d "keycloak" --exec echo "Found potential keycloak directory: {}" # Look for existing documentation or tests that reference the old detection behavior echo "Searching for references to theme detection..." rg -i "theme.*src.*dir" -A 3 -B 3 # Check for any configuration files that might specify custom theme paths echo "Searching for theme path configurations..." rg "themeSrcDirPath|theme.*path" -A 2 -B 2Length of output: 50288
Validate backward compatibility of theme directory resolution
We removed the heuristic search in
src/bin/shared/buildContext.ts(now always returningsrcDirPathasthemeSrcDirPath), which simplifies the code but may break existing projects that relied on nested or custom-named theme folders. Our grep/fdsweep in this repo didn’t uncover any nonstandard theme directories, but absence of evidence here isn’t evidence of absence in user codebases.• Check
src/bin/shared/buildContext.ts(lines 166–168): the old crawl-based lookup was replaced by an unconditionalreturn { themeSrcDirPath: srcDirPath }.
• Review any projects that previously usedsrc/<KEYCLOAK_THEME>directories in non-top-level or renamed folders—ensure they still load correctly.
• Update documentation or add a migration note specifying that themes must now live undersrc/with the exact<KEYCLOAK_THEME>basename.Please manually verify on real projects and document this breaking change.
| // @ts-expect-error | ||
| body.unshift(importDeclaration); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address type safety issues instead of suppressing errors.
Multiple @ts-expect-error comments indicate type mismatches. Consider:
- Using proper type assertions with type guards
- Installing/updating type definitions for babel/recast
- Creating helper functions with proper typing
For example, replace error suppression with type assertions:
-// @ts-expect-error
-body.unshift(importDeclaration);
+body.unshift(importDeclaration as any); // or use proper babel typesAlso applies to: 89-90, 92-93, 98-99
🤖 Prompt for AI Agents
In src/bin/init/setupVitePluginIfNeeded.ts around lines 80-81 and also 89-90,
92-93, 98-99, the code uses @ts-expect-error to suppress type errors when
manipulating the AST nodes. Instead of suppressing these errors, identify the
exact type mismatches causing the errors and fix them by adding appropriate type
assertions or type guards to ensure type safety. Additionally, verify that the
correct and up-to-date type definitions for babel or recast are installed and
used. Refactor the code to use helper functions with explicit typings to handle
AST node manipulations safely without disabling TypeScript checks.
| format: { | ||
| if (!(await getIsPrettierAvailable())) { | ||
| break format; | ||
| } | ||
|
|
||
| viteConfigTsContent_modified = await runPrettier({ | ||
| sourceCode: viteConfigTsContent_modified, | ||
| filePath: viteConfigTsFilePath | ||
| }); | ||
| } |
There was a problem hiding this comment.
Replace labeled statement with standard conditional logic.
The labeled statement pattern is confusing and flagged by static analysis. This is not idiomatic TypeScript/JavaScript.
Apply this diff to use standard conditional:
-format: {
- if (!(await getIsPrettierAvailable())) {
- break format;
- }
-
- viteConfigTsContent_modified = await runPrettier({
- sourceCode: viteConfigTsContent_modified,
- filePath: viteConfigTsFilePath
- });
-}
+if (await getIsPrettierAvailable()) {
+ viteConfigTsContent_modified = await runPrettier({
+ sourceCode: viteConfigTsContent_modified,
+ filePath: viteConfigTsFilePath
+ });
+}📝 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.
| format: { | |
| if (!(await getIsPrettierAvailable())) { | |
| break format; | |
| } | |
| viteConfigTsContent_modified = await runPrettier({ | |
| sourceCode: viteConfigTsContent_modified, | |
| filePath: viteConfigTsFilePath | |
| }); | |
| } | |
| if (await getIsPrettierAvailable()) { | |
| viteConfigTsContent_modified = await runPrettier({ | |
| sourceCode: viteConfigTsContent_modified, | |
| filePath: viteConfigTsFilePath | |
| }); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
🤖 Prompt for AI Agents
In src/bin/init/setupVitePluginIfNeeded.ts around lines 128 to 137, replace the
labeled statement used for formatting with a standard if statement. Remove the
label and the break statement, and instead use a simple if condition to check if
Prettier is available before running the formatting logic. This will make the
code more idiomatic and clear.
| visitExportDefaultDeclaration(path) { | ||
| // @ts-expect-error | ||
| const args = path.node.declaration.arguments; | ||
|
|
||
| if (!Array.isArray(args)) return false; | ||
|
|
There was a problem hiding this comment.
Add proper type guards for AST node access.
The code accesses path.node.declaration.arguments with a @ts-expect-error comment and minimal validation. This could cause runtime errors if the ESLint config has a different structure than expected (e.g., if the default export is not a function call).
Consider adding proper type guards:
visitExportDefaultDeclaration(path) {
- // @ts-expect-error
- const args = path.node.declaration.arguments;
+ const declaration = path.node.declaration;
+
+ // Check if declaration is a CallExpression
+ if (!babelTypes.isCallExpression(declaration)) {
+ return false;
+ }
+
+ const args = declaration.arguments;
if (!Array.isArray(args)) return false;📝 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.
| visitExportDefaultDeclaration(path) { | |
| // @ts-expect-error | |
| const args = path.node.declaration.arguments; | |
| if (!Array.isArray(args)) return false; | |
| visitExportDefaultDeclaration(path) { | |
| const declaration = path.node.declaration; | |
| // Check if declaration is a CallExpression | |
| if (!babelTypes.isCallExpression(declaration)) { | |
| return false; | |
| } | |
| const args = declaration.arguments; | |
| if (!Array.isArray(args)) return false; |
🤖 Prompt for AI Agents
In src/bin/init/setupEslint.ts around lines 39 to 44, the code accesses
path.node.declaration.arguments with a @ts-expect-error and only checks if it's
an array, which risks runtime errors if the default export is not a function
call. To fix this, add proper type guards to verify that path.node.declaration
is a CallExpression before accessing arguments, ensuring safe access and
preventing runtime errors from unexpected AST node structures.
| install_stories: { | ||
| if (!doInstallStories) { | ||
| break install_stories; | ||
| } | ||
|
|
||
| const moduleName = "@keycloakify/login-ui-storybook"; | ||
|
|
||
| const latestVersion = await getModuleLatestVersion({ moduleName }); | ||
|
|
||
| (parsedPackageJson.devDependencies ??= {})[moduleName] = `~${latestVersion}`; | ||
|
|
||
| delete parsedPackageJson.dependencies[moduleName]; | ||
| } |
There was a problem hiding this comment.
Add safety check before deleting from dependencies.
Line 191 attempts to delete from parsedPackageJson.dependencies without checking if it exists, which could throw an error.
- delete parsedPackageJson.dependencies[moduleName];
+ if (parsedPackageJson.dependencies) {
+ delete parsedPackageJson.dependencies[moduleName];
+ }📝 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.
| install_stories: { | |
| if (!doInstallStories) { | |
| break install_stories; | |
| } | |
| const moduleName = "@keycloakify/login-ui-storybook"; | |
| const latestVersion = await getModuleLatestVersion({ moduleName }); | |
| (parsedPackageJson.devDependencies ??= {})[moduleName] = `~${latestVersion}`; | |
| delete parsedPackageJson.dependencies[moduleName]; | |
| } | |
| install_stories: { | |
| if (!doInstallStories) { | |
| break install_stories; | |
| } | |
| const moduleName = "@keycloakify/login-ui-storybook"; | |
| const latestVersion = await getModuleLatestVersion({ moduleName }); | |
| (parsedPackageJson.devDependencies ??= {})[moduleName] = `~${latestVersion}`; | |
| if (parsedPackageJson.dependencies) { | |
| delete parsedPackageJson.dependencies[moduleName]; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 180-180: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
🤖 Prompt for AI Agents
In src/bin/initialize-login-theme.ts around lines 180 to 192, before deleting
the moduleName from parsedPackageJson.dependencies, add a check to ensure
parsedPackageJson.dependencies exists to prevent runtime errors. Only perform
the delete operation if parsedPackageJson.dependencies is defined.
| async function getModuleLatestVersion(params: { moduleName: string }) { | ||
| const { moduleName } = params; | ||
|
|
||
| const versions = ((): string[] => { | ||
| const cmdOutput = child_process | ||
| .execSync(`npm show ${moduleName} versions --json`) | ||
| .toString("utf8") | ||
| .trim(); | ||
|
|
||
| const versions = JSON.parse(cmdOutput) as string | string[]; | ||
|
|
||
| // NOTE: Bug in some older npm versions | ||
| if (typeof versions === "string") { | ||
| return [versions]; | ||
| } | ||
|
|
||
| return versions; | ||
| })(); | ||
|
|
||
| const version = versions.reverse().filter(version => !version.includes("-"))[0]; | ||
|
|
||
| assert(version !== undefined); | ||
|
|
||
| return version; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make getModuleLatestVersion async and add error handling.
The function uses execSync which blocks the event loop and lacks error handling. Since it's called with await, it should be async:
-async function getModuleLatestVersion(params: { moduleName: string }) {
+async function getModuleLatestVersion(params: { moduleName: string }): Promise<string> {
const { moduleName } = params;
- const versions = ((): string[] => {
- const cmdOutput = child_process
- .execSync(`npm show ${moduleName} versions --json`)
- .toString("utf8")
- .trim();
+ try {
+ const { promisify } = await import('util');
+ const exec = promisify(child_process.exec);
+
+ const { stdout } = await exec(`npm show ${moduleName} versions --json`);
+ const cmdOutput = stdout.trim();
const versions = JSON.parse(cmdOutput) as string | string[];
// NOTE: Bug in some older npm versions
if (typeof versions === "string") {
return [versions];
}
return versions;
- })();
+ } catch (error) {
+ throw new Error(`Failed to fetch versions for ${moduleName}: ${error.message}`);
+ }
const version = versions.reverse().filter(version => !version.includes("-"))[0];
assert(version !== undefined);
return version;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/bin/initialize-login-theme.ts around lines 215 to 239, the
getModuleLatestVersion function uses execSync which blocks the event loop and
lacks error handling. Refactor the function to use an asynchronous method like
exec or execFile from child_process with promises, handle any errors that may
occur during execution, and ensure the function remains async to be compatible
with await calls. This will prevent blocking and improve robustness.
| setup_src: { | ||
| if (buildContext.bundler !== "vite") { | ||
| break setup_src; | ||
| } | ||
|
|
||
| const srcDirPath = pathJoin(buildContext.projectDirPath, "src"); | ||
|
|
||
| const mainTsxFilePath = pathJoin(srcDirPath, "main.tsx"); | ||
|
|
||
| if (!(await existsAsync(mainTsxFilePath))) { | ||
| break setup_src; | ||
| } | ||
|
|
||
| const isAlreadySetup = await (async () => { | ||
| if (buildContext.themeSrcDirPath !== srcDirPath) { | ||
| return true; | ||
| } | ||
|
|
||
| { | ||
| const basenames = await fs.readdir(srcDirPath); | ||
|
|
||
| for (const basename of basenames) { | ||
| const path = pathJoin(srcDirPath, basename); | ||
|
|
||
| if (!(await fs.stat(path)).isFile()) { | ||
| continue; | ||
| } | ||
|
|
||
| if ((await fs.readFile(path)).toString("utf8").includes("./kc.gen")) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const themeType of [...THEME_TYPES, "email"]) { | ||
| if (!(await existsAsync(pathJoin(srcDirPath, themeType)))) { | ||
| continue; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| })(); | ||
|
|
||
| if (isAlreadySetup) { | ||
| break setup_src; | ||
| } | ||
|
|
||
| const doSetupAsStandalone = await (async () => { | ||
| const relativeProjectDirPath = pathRelative( | ||
| process.cwd(), | ||
| buildContext.projectDirPath | ||
| ); | ||
|
|
||
| console.log( | ||
| chalk.cyan( | ||
| [ | ||
| relativeProjectDirPath === "" | ||
| ? "This Vite project" | ||
| : `The Vite project at ${relativeProjectDirPath}`, | ||
| "is it dedicated *only* to building a Keycloak theme?" | ||
| ].join(" ") | ||
| ) | ||
| ); | ||
|
|
||
| const YES = "Yes — this project is only for the Keycloak theme (recommended)"; | ||
| const NO = | ||
| "No — I'm building an app and a Keycloak theme in the same project (advanced)"; | ||
|
|
||
| const { value } = await cliSelect({ values: [YES, NO] }).catch(() => { | ||
| process.exit(-1); | ||
| }); | ||
|
|
||
| console.log(`${value}\n`); | ||
|
|
||
| return value === YES; | ||
| })(); | ||
|
|
||
| let files: { relativeFilePath: string; fileContent: string }[]; | ||
|
|
||
| if (doSetupAsStandalone) { | ||
| const viteEnvDTsFilePath = pathJoin( | ||
| buildContext.themeSrcDirPath, | ||
| "vite-env.d.ts" | ||
| ); | ||
|
|
||
| const viteEnvDTsContent = await fs.readFile(viteEnvDTsFilePath); | ||
|
|
||
| await fs.rm(srcDirPath, { recursive: true }); | ||
|
|
||
| await fs.mkdir(srcDirPath); | ||
|
|
||
| await fs.writeFile(viteEnvDTsFilePath, viteEnvDTsContent); | ||
|
|
||
| files = [ | ||
| { | ||
| relativeFilePath: "main.tsx", | ||
| fileContent: [ | ||
| `if (window.kcContext !== undefined) {`, | ||
| ` import("./main-kc");`, | ||
| `} else {`, | ||
| ` import("./main-kc.dev");`, | ||
| `}` | ||
| ].join("\n") | ||
| }, | ||
| { | ||
| relativeFilePath: "main-kc.dev.tsx", | ||
| fileContent: `export {};\n` | ||
| }, | ||
| { | ||
| relativeFilePath: "main-kc.tsx", | ||
| fileContent: [ | ||
| `import { createRoot } from "react-dom/client";`, | ||
| `import { StrictMode } from "react";`, | ||
| `import { KcPage } from "./kc.gen";`, | ||
| ``, | ||
| `if (!window.kcContext) {`, | ||
| ` throw new Error("No Keycloak context");`, | ||
| `}`, | ||
| ``, | ||
| `createRoot(document.getElementById("root")!).render(`, | ||
| ` <StrictMode>`, | ||
| ` <KcPage kcContext={window.kcContext} />`, | ||
| ` </StrictMode>`, | ||
| `);` | ||
| ].join("\n") | ||
| } | ||
| ]; | ||
| } else { | ||
| doAddRunDevScript = true; | ||
|
|
||
| await fs.copyFile( | ||
| mainTsxFilePath, | ||
| pathJoin(buildContext.themeSrcDirPath, "main-app.tsx") | ||
| ); | ||
|
|
||
| files = [ | ||
| { | ||
| relativeFilePath: "main.tsx", | ||
| fileContent: [ | ||
| `if (window.kcContext !== undefined) {`, | ||
| ` import("./keycloak-theme/main");`, | ||
| `} else if (import.meta.env.VITE_KC_DEV === "true") {`, | ||
| ` import("./keycloak-theme/main.dev");`, | ||
| `} else {`, | ||
| ` import("./main-app");`, | ||
| `}`, | ||
| `` | ||
| ].join("\n") | ||
| }, | ||
| { | ||
| relativeFilePath: pathJoin(KEYCLOAK_THEME, "main.dev.tsx"), | ||
| fileContent: `export {};\n` | ||
| }, | ||
| { | ||
| relativeFilePath: pathJoin(KEYCLOAK_THEME, "main.tsx"), | ||
| fileContent: [ | ||
| `import { createRoot } from "react-dom/client";`, | ||
| `import { StrictMode } from "react";`, | ||
| `import { KcPage } from "./kc.gen";`, | ||
| ``, | ||
| `if (!window.kcContext) {`, | ||
| ` throw new Error("No Keycloak context");`, | ||
| `}`, | ||
| ``, | ||
| `createRoot(document.getElementById("root")!).render(`, | ||
| ` <StrictMode>`, | ||
| ` <KcPage kcContext={window.kcContext} />`, | ||
| ` </StrictMode>`, | ||
| `);`, | ||
| `` | ||
| ].join("\n") | ||
| } | ||
| ]; | ||
| } | ||
|
|
||
| for (let { relativeFilePath, fileContent } of files) { | ||
| const filePath = pathJoin(srcDirPath, relativeFilePath); | ||
|
|
||
| run_prettier: { | ||
| if (!(await getIsPrettierAvailable())) { | ||
| break run_prettier; | ||
| } | ||
|
|
||
| fileContent = await runPrettier({ | ||
| filePath: filePath, | ||
| sourceCode: fileContent | ||
| }); | ||
| } | ||
|
|
||
| await fs.writeFile(filePath, Buffer.from(fileContent, "utf8")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract large setup_src block into a separate function.
The setup_src labeled block spans nearly 200 lines, making the main function difficult to follow. Consider extracting this into a dedicated function:
async function setupSrcDirectory(params: {
buildContext: BuildContext;
projectDirPath: string;
}): Promise<boolean> {
const { buildContext, projectDirPath } = params;
if (buildContext.bundler !== "vite") {
return false;
}
// ... rest of the setup_src logic
return doAddRunDevScript;
}
// Then in the main function:
const doAddRunDevScript = await setupSrcDirectory({ buildContext, projectDirPath });🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 216-216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
🤖 Prompt for AI Agents
In src/bin/init/init.ts between lines 37 and 229, the large setup_src labeled
block should be extracted into a separate async function named setupSrcDirectory
that accepts buildContext and projectDirPath as parameters. Move all the
existing logic inside this new function, replacing the labeled block, and have
it return the boolean doAddRunDevScript. Then, in the main function, replace the
setup_src block with a call to await setupSrcDirectory passing the necessary
parameters and assign its result to doAddRunDevScript. This refactoring improves
readability by isolating the setup logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/bin/init/init.ts (1)
37-229: 🛠️ Refactor suggestionConsider the previous refactoring suggestion.
The large
setup_srclabeled block continues to span nearly 200 lines, making the function difficult to follow. As suggested in the previous review, extracting this into a separatesetupSrcDirectoryfunction would improve readability and maintainability.🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 216-216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
🧹 Nitpick comments (1)
src/bin/init/init.ts (1)
37-37: Consider replacing labeled blocks with functions.Static analysis correctly flags the use of labeled blocks for non-loop statements as suspicious. While functionally correct, these could be replaced with well-named functions or reorganized to improve code clarity.
For example, the
run_prettierblocks could be extracted into a utility function:async function formatWithPrettierIfAvailable(filePath: string, content: string): Promise<string> { if (!(await getIsPrettierAvailable())) { return content; } return await runPrettier({ filePath, sourceCode: content }); }Also applies to: 216-216, 231-231, 274-274
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(1 hunks)src/bin/init/init.ts(1 hunks)src/bin/shared/buildContext.ts(4 hunks)src/bin/shared/initializeSpa.ts(4 hunks)src/bin/tools/fetchProxyOptions.ts(1 hunks)src/bin/tools/npmInstall.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bin/shared/buildContext.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/bin/init/init.ts
[error] 37-37: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 216-216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 231-231: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 274-274: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test with Node v18 on ubuntu-latest
🔇 Additional comments (6)
src/bin/shared/initializeSpa.ts (3)
4-4: LGTM: Clean imports for async file operations.The addition of
fsPr(fs/promises) andexistsAsyncimports supports the new asynchronous file manipulation functionality.Also applies to: 16-16
87-89: LGTM: Well-placed function call in initialization flow.The
disableVerbatimModuleSyntaxcall is appropriately positioned early in the SPA initialization process to ensure TypeScript configuration is set up correctly before other operations.
164-184: LGTM: Robust implementation with proper error handling.The function correctly:
- Checks file existence before attempting to read
- Uses a precise regex pattern to match the specific JSON property
- Preserves optional trailing commas in the replacement
- Handles async file operations properly
The regex
/\"verbatimModuleSyntax\"\s*:\s*true\s*(,?)/mis well-crafted to handle various JSON formatting styles.src/bin/tools/npmInstall.ts (2)
102-110: LGTM: Improved npm install reliability.Adding the
--forceflag specifically for npm (while excluding it for other package managers) is a sensible approach to handle dependency conflicts that might arise during theme initialization.
114-122: LGTM: Enhanced error handling with cleaner output.The change to accumulate stderr output and only display it on failure provides a better user experience by reducing noise during successful installs while still showing relevant errors when they occur.
Also applies to: 126-126
src/bin/init/init.ts (1)
17-354: LGTM: Comprehensive theme initialization logic.The core functionality is well-implemented with proper:
- Async/await patterns
- Error handling and validation
- User interaction via CLI prompts
- Dynamic module imports for theme types
- TypeScript type safety with zod validation
The initialization flow correctly handles both standalone and mixed project setups.
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 216-216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 231-231: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 274-274: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
| return undefined; | ||
| } | ||
|
|
||
| console.log(chalk.cyan(`Which theme theme type would you like to initialize?`)); |
There was a problem hiding this comment.
Fix typo in user prompt.
There's a duplicate word in the user-facing message.
- console.log(chalk.cyan(`Which theme theme type would you like to initialize?`));
+ console.log(chalk.cyan(`Which theme type would you like to initialize?`));📝 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.
| console.log(chalk.cyan(`Which theme theme type would you like to initialize?`)); | |
| console.log(chalk.cyan(`Which theme type would you like to initialize?`)); |
🤖 Prompt for AI Agents
In src/bin/init/init.ts at line 302, the user prompt contains a duplicated word
"theme theme". Remove the extra "theme" so the message reads "Which theme type
would you like to initialize?" to correct the typo.
| cwd: npmConfigGetCwd, | ||
| stdio: ["pipe", "pipe", "ignore"] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Consider the impact on error visibility.
The stdio configuration change to suppress stderr output is appropriate for reducing console noise during normal npm config operations. However, ensure that critical npm errors are still properly handled, as ignored stderr might mask important warnings or configuration issues.
Verify that error handling works as expected when npm config operations fail:
🏁 Script executed:
#!/bin/bash
# Description: Test error handling behavior for npm config get failures
# Expected: execSync should still throw exceptions for actual failures despite stderr being ignored
# Check if there are any try-catch blocks around this execSync call in the codebase
ast-grep --pattern $'try {
$$$
child_process.execSync("npm config get", $_)
$$$
} catch ($_) {
$$$
}'
# Look for error handling patterns around execSync calls in this file or related files
rg -A 3 -B 3 "execSync.*npm config get"Length of output: 1930
Approve proxy fetching change, but add error handling
The stdio config (["pipe","pipe","ignore"]) is consistent with suppressing npm’s stderr noise and matches patterns in buildContext.ts. However, unlike src/bin/shared/buildContext.ts, this call isn’t wrapped in a try/catch, so any npm failures will surface without contextual handling.
Points to address:
- File:
src/bin/tools/fetchProxyOptions.ts - Location: around the
child_process.execSync("npm config get", { cwd: npmConfigGetCwd, stdio: ["pipe","pipe","ignore"] })call - Suggestion: Wrap this in a
try/catch(similar to lines 751–762 insrc/bin/shared/buildContext.ts) to:- Detect and handle specific npm errors
- Provide clearer failure messages or fallback behavior
Example snippet:
try {
const output = child_process.execSync("npm config get", {
cwd: npmConfigGetCwd,
stdio: ["pipe", "pipe", "ignore"],
});
// parse output…
} catch (error) {
// handle or rethrow with context
throw new Error(`Failed to read npm proxy config: ${error}`);
}🤖 Prompt for AI Agents
In src/bin/tools/fetchProxyOptions.ts around lines 21 to 22, the call to
child_process.execSync("npm config get", { cwd: npmConfigGetCwd, stdio:
["pipe","pipe","ignore"] }) is not wrapped in a try/catch block, which means npm
errors will surface without contextual handling. To fix this, wrap the execSync
call in a try/catch block that catches errors, provides a clearer failure
message including the original error, and optionally implements fallback
behavior or rethrows with added context. This will improve error visibility and
handling consistency with similar code in src/bin/shared/buildContext.ts.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/bin/initialize-login-theme.ts (2)
191-191:⚠️ Potential issueAdd safety check before deleting from dependencies.
Attempting to delete from
parsedPackageJson.dependencieswithout checking if it exists could throw an error.- delete parsedPackageJson.dependencies[moduleName]; + if (parsedPackageJson.dependencies) { + delete parsedPackageJson.dependencies[moduleName]; + }
261-285: 🛠️ Refactor suggestionMake getModuleLatestVersion async and add error handling.
The function uses
execSyncwhich blocks the event loop and lacks error handling. Since it's called withawait, it should be async.-function getModuleLatestVersion(params: { moduleName: string }) { +async function getModuleLatestVersion(params: { moduleName: string }): Promise<string> { const { moduleName } = params; - const versions = ((): string[] => { - const cmdOutput = child_process - .execSync(`npm show ${moduleName} versions --json`) - .toString("utf8") - .trim(); + try { + const { promisify } = await import('util'); + const exec = promisify(child_process.exec); + + const { stdout } = await exec(`npm show ${moduleName} versions --json`); + const cmdOutput = stdout.trim(); const versions = JSON.parse(cmdOutput) as string | string[]; // NOTE: Bug in some older npm versions if (typeof versions === "string") { return [versions]; } return versions; - })(); + } catch (error) { + throw new Error(`Failed to fetch versions for ${moduleName}: ${error.message}`); + } const version = versions.reverse().filter(version => !version.includes("-"))[0]; assert(version !== undefined); return version; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/bin/initialize-login-theme.ts(1 hunks)src/bin/shared/buildContext.ts(4 hunks)src/bin/tools/fetchProxyOptions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/bin/tools/fetchProxyOptions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bin/shared/buildContext.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/bin/initialize-login-theme.ts
[error] 90-90: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 153-153: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 180-180: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 194-194: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test with Node v18 on ubuntu-latest
| { | ||
| const moduleName = "@keycloakify/login-ui"; | ||
|
|
||
| const latestVersion = getModuleLatestVersion({ moduleName }); |
There was a problem hiding this comment.
Make version fetching asynchronous.
The getModuleLatestVersion function uses execSync internally but should be called with await for consistency with the async command pattern.
- const latestVersion = getModuleLatestVersion({ moduleName });
+ const latestVersion = await getModuleLatestVersion({ moduleName });Also applies to: 187-187
🤖 Prompt for AI Agents
In src/bin/initialize-login-theme.ts at lines 171 and 187, the call to
getModuleLatestVersion is currently synchronous but should be awaited to follow
the asynchronous command pattern. Change the calls to use await before
getModuleLatestVersion and ensure the function is called within an async
context. This requires updating the function call to be asynchronous and
handling the returned promise properly.
| install_storybook: { | ||
| if (!doInstallStories) { | ||
| break install_storybook; | ||
| } | ||
|
|
||
| if (buildContext.bundler !== "vite") { | ||
| break install_storybook; | ||
| } | ||
|
|
||
| if ( | ||
| Object.keys({ | ||
| ...parsedPackageJson.dependencies, | ||
| ...parsedPackageJson.devDependencies | ||
| }).includes("storybook") | ||
| ) { | ||
| break install_storybook; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace labeled blocks with proper control flow.
The use of labels on non-loop blocks is non-standard and can be confusing. Consider using early returns or proper conditional nesting instead.
- install_storybook: {
- if (!doInstallStories) {
- break install_storybook;
- }
-
- if (buildContext.bundler !== "vite") {
- break install_storybook;
- }
-
- if (
- Object.keys({
- ...parsedPackageJson.dependencies,
- ...parsedPackageJson.devDependencies
- }).includes("storybook")
- ) {
- break install_storybook;
- }
+ if (doInstallStories && buildContext.bundler === "vite" &&
+ !Object.keys({
+ ...parsedPackageJson.dependencies,
+ ...parsedPackageJson.devDependencies
+ }).includes("storybook")
+ ) {📝 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.
| install_storybook: { | |
| if (!doInstallStories) { | |
| break install_storybook; | |
| } | |
| if (buildContext.bundler !== "vite") { | |
| break install_storybook; | |
| } | |
| if ( | |
| Object.keys({ | |
| ...parsedPackageJson.dependencies, | |
| ...parsedPackageJson.devDependencies | |
| }).includes("storybook") | |
| ) { | |
| break install_storybook; | |
| } | |
| // Only install Storybook if requested, using Vite, and not already present | |
| if (doInstallStories && buildContext.bundler === "vite" && | |
| !Object.keys({ | |
| ...parsedPackageJson.dependencies, | |
| ...parsedPackageJson.devDependencies | |
| }).includes("storybook") | |
| ) { | |
| // ... existing Storybook installation logic ... | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
🤖 Prompt for AI Agents
In src/bin/initialize-login-theme.ts between lines 90 and 106, the code uses a
labeled block with breaks for control flow, which is non-standard and confusing.
Replace the labeled block and breaks with early return statements or properly
nested if conditions to handle the control flow more clearly and idiomatically.
Minimal implementation of the next big iteration of Keycloakify to be introduced at Keycloak Japan
No breaking change.
Summary by CodeRabbit
New Features
Improvements
.gitignorefiles and extension metadata for more predictable outputs.Bug Fixes