BA-2259: improve dev mode #218
Conversation
anicioalexandre
requested review from
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis pull request introduces several new asynchronous utility functions and build scripts to improve process management, file synchronization, and readiness checks across multiple packages. The updates include revised documentation commands, modifications to build and development scripts in both components and design system packages, and adjustments to dependency management. Additionally, configuration changes ensure old build artifacts are cleaned prior to new builds, and workspace setup now replaces React Native libraries with new dev-tools dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant DevCommand
participant BuildScript
participant BuildTool
participant FS
Developer->>DevCommand: Start build/watch command
DevCommand->>BuildScript: Invoke runBuild/runWatchBuild
BuildScript->>BuildTool: Execute build steps (tsup, tsc, bundling)
BuildTool->>FS: Clean output and write readiness file
FS-->>BuildTool: File operations complete
BuildTool-->>BuildScript: Build process complete
BuildScript-->>DevCommand: Return build status
sequenceDiagram
participant Watcher
participant DevManager
participant ConsumerUpdater
Watcher->>DevManager: Detect file change
DevManager->>DevManager: Cancel current build if running
DevManager->>ConsumerUpdater: Trigger update via rsync and utility funcs
ConsumerUpdater-->>DevManager: Acknowledge update completion
Suggested reviewers
Poem
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
packages/components/.scripts/build-command.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/tsup.config.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/.scripts/build.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
packages/design-system/tsup.config.ts (1)
7-7: LGTM! Consistent configuration across packages.The change to enable cleaning before builds matches the components package, maintaining consistency in build configuration across packages.
This consistent approach to build configuration across packages is good practice for maintainability. Consider documenting these shared build configurations in a central location or extracting common options into a shared config file if more packages are added in the future.
packages/components/.scripts/build.mjs (1)
58-60: ReassigningcurrentBuildProcessesmay not clear the original array reference.
If you intend to clear the same array object passed from the caller, consider resetting its length instead of reassigning the variable. Reassigning will only affect the local reference, leaving any references in the caller intact.A possible adjustment:
} finally { - currentBuildProcesses = [] + currentBuildProcesses.length = 0 }packages/components/.scripts/build-command.mjs (1)
1-3: Optional array tracking for concurrency.
Currently,runBuildis invoked without passing acurrentBuildProcessesarray. If you plan to track ongoing processes from this script, consider passing an array argument torunBuildand monitoring it here.packages/design-system/.scripts/build-command.mjs (1)
1-3: Same recommendation for optional array tracking.
Similar to the components build-command script, you could track ongoing build processes here if desired, by passing a shared array argument torunBuild.packages/design-system/.scripts/build.mjs (2)
13-17: Consider explicit error handling for file deletion.The current implementation silently ignores all errors during file deletion. While this works for missing files, it might mask other important filesystem errors.
try { await fs.unlink(readyFilePath) } catch (error) { - // pass + if (error.code !== 'ENOENT') { + console.warn(`${chalk.yellow(`${commandTag} Warning: Failed to delete ready file: ${error.message}`)}`) + } }
21-42: Consider adding timeout protection for build processes.Long-running or hanging build processes could block the entire build. Consider adding timeout protection.
await Promise.all([ (async () => { console.log(chalk.yellow(`${commandTag} Running tsup bundling...`)) - const tsupProc = execa('pnpm', ['tsup:bundle', '--silent'], { preferLocal: true }) + const tsupProc = execa('pnpm', ['tsup:bundle', '--silent'], { + preferLocal: true, + timeout: 300000 // 5 minutes + }) currentBuildProcesses?.push(tsupProc) await tsupProc console.log(chalk.yellow(`${commandTag} Bundling completed.`)) })(), // Apply similar changes to other processes ])packages/design-system/.scripts/dev-command.mjs (1)
60-60: Consider extracting watch patterns to configuration.The watch regex is hardcoded, making it less maintainable and harder to update.
+ const WATCH_PATTERNS = { + DESIGN_SYSTEM: /^(components|hooks|layouts|providers|styles|utils)\/(common|web|native)(\/.*)?$/, + } - const watchRegex = /^(components|hooks|layouts|providers|styles|utils)\/(common|web|native)(\/.*)?$/ + const watchRegex = WATCH_PATTERNS.DESIGN_SYSTEMpackages/components/.scripts/dev-command.mjs (2)
67-91: Consider sharing watcher configuration with design-system.The watcher configuration is duplicated between components and design-system, with only the regex pattern being different.
Extract the watcher configuration to a shared utility:
// .scripts/watch-utils.mjs export const createWatcher = (rootDir, watchRegex, options = {}) => chokidar.watch(rootDir, { ignoreInitial: true, usePolling: true, interval: 100, awaitWriteFinish: { stabilityThreshold: 500, pollInterval: 100, }, ignored: (filePath, stats) => { if (filePath.includes('node_modules') || filePath.includes('dist') || filePath.includes('tmp')) { return true } if (stats?.isFile()) { const relative = path.relative(rootDir, filePath).replace(/\\/g, '/') if (!watchRegex.test(relative)) { return true } } return false }, ...options })🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
93-98: Consider more robust debouncing strategy.The current debouncing implementation might not handle rapid successive changes optimally.
+ import debounce from 'lodash/debounce' + + const debouncedBuild = debounce(() => { + runWatchBuild() + }, 2000, { maxWait: 5000 }) + watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) - if (buildTimeout) clearTimeout(buildTimeout) - buildTimeout = setTimeout(runWatchBuild, 2000) + debouncedBuild() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.scripts/command-utils.mjs(1 hunks)README.md(1 hunks)package.json(1 hunks)packages/components/.scripts/build-command.mjs(1 hunks)packages/components/.scripts/build.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/README.md(1 hunks)packages/components/package.json(3 hunks)packages/components/tsup.config.ts(1 hunks)packages/design-system/.scripts/build-command.mjs(1 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/README.md(1 hunks)packages/design-system/package.json(2 hunks)packages/design-system/tsup.config.ts(1 hunks)pnpm-workspace.yaml(2 hunks)turbo.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design-system/README.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/design-system/.scripts/dev-command.mjs
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (12)
packages/components/.scripts/build.mjs (2)
1-5: Great use ofchalkandexecaimports.
No issues noted with these initial imports and constants.
22-43: Commendable concurrency approach.
Running tsup bundling and type declaration generation in parallel is an efficient way to speed up the build process. This pattern leverages Promise.all correctly.turbo.json (1)
6-6:❓ Verification inconclusive
Verify environment variable documentation and usage.
The addition of
BASEAPP_FRONTEND_TEMPLATE_PATHto the dev task's environment variables suggests template path configuration. Please ensure this variable is properly documented and its usage is explained in the project documentation.
🏁 Script executed:
#!/bin/bash # Search for documentation or usage of BASEAPP_FRONTEND_TEMPLATE_PATH rg -i "BASEAPP_FRONTEND_TEMPLATE_PATH" --type mdLength of output: 48
Action Required: Verify Environment Variable Documentation
The environment variable
BASEAPP_FRONTEND_TEMPLATE_PATHwas recently added to the development task settings inturbo.json. A search within markdown files did not reveal any documentation references for this variable. Please review the entire codebase or relevant configuration files to ensure its usage is clearly explained. If the variable plays a key role in configuring template paths for development mode, update the project documentation (e.g., README or setup guides) accordingly.
- Confirm if
BASEAPP_FRONTEND_TEMPLATE_PATHis referenced or explained elsewhere in the repository.- Update or add documentation detailing its purpose and integration if it is currently missing.
🏁 Script executed:
#!/bin/bash # Search for any occurrence of BASEAPP_FRONTEND_TEMPLATE_PATH across the entire codebase (not just markdown) rg -i "BASEAPP_FRONTEND_TEMPLATE_PATH"Length of output: 98
Action Required: Document and Verify the New Environment Variable
The addition of
BASEAPP_FRONTEND_TEMPLATE_PATHto the dev task environment inturbo.jsonindicates a new template path configuration, but our search shows it's only referenced in that file with no accompanying documentation. Please ensure that:
- The purpose and usage of
BASEAPP_FRONTEND_TEMPLATE_PATHare clearly explained in the project documentation (e.g., README or setup guides).- Any necessary instructions for configuring or updating this variable in development mode are provided.
package.json (1)
26-29: LGTM! Dev tools will improve build process.The addition of chalk (terminal styling), chokidar (file watching), and execa (process execution) will enhance the development workflow.
packages/components/README.md (1)
21-26: LGTM! Improved command clarity.The storybook command now uses the full package name
@baseapp-frontend/components, which is more explicit and consistent with workspace package naming conventions.pnpm-workspace.yaml (1)
146-149: LGTM! New dev-tools catalog aligns with package.json.The addition of the dev-tools catalog with chalk, chokidar, and execa matches the dependencies added to package.json, providing consistent tooling across the workspace.
packages/design-system/package.json (2)
116-121: LGTM! Dev tools dependencies match workspace configuration.The addition of chalk, chokidar, and execa from the dev-tools catalog aligns with the workspace-level configuration.
11-12:❓ Verification inconclusive
Verify existence of new script files.
The build and dev scripts now use dedicated command files. Please ensure these files exist:
.scripts/build-command.mjs.scripts/dev-command.mjs
🏁 Script executed:
#!/bin/bash # Check if the new script files exist fd -t f "build-command.mjs|dev-command.mjs" -pLength of output: 48
Action Required: Verify the Existence of Script Files
The updated build and dev commands now reference dedicated command files. However, the initial automated check did not yield any output. Please manually verify that these files exist in the expected location (likely under
packages/design-system/.scripts/):
packages/design-system/.scripts/build-command.mjspackages/design-system/.scripts/dev-command.mjsIf these files are missing or misplaced, adjust the file paths in
package.jsonor add the missing files accordingly.packages/components/package.json (3)
7-13: LGTM! Script changes improve modularity and clarity.The changes improve the development workflow by:
- Renaming
babel:bundletobabel:transpileto better reflect its purpose.- Moving build and dev scripts to
.scriptsdirectory for better organization.
147-156: LGTM! New dev dependencies align with PR objectives.The addition of
chalk,chokidar, andexecaas dev dependencies aligns with the PR objective to improve dev mode:
chalk: Terminal styling for better developer experience.chokidar: File watching for improved development workflow.execa: Process execution for better build script control.
115-115:❓ Verification inconclusive
Verify the impact of the version constraint change.
The version constraint for
@baseapp-frontend/confighas been changed fromworkspace:*toworkspace:^. This change makes the version constraint more restrictive.Run the following script to check for potential version conflicts:
🏁 Script executed:
#!/bin/bash # Description: Check for potential version conflicts with @baseapp-frontend/config. # Find all package.json files that depend on @baseapp-frontend/config rg -l '"@baseapp-frontend/config":\s*"workspace:[*^]"' -g 'package.json'Length of output: 324
Review Note: Confirm Consistent Version Constraints for @baseapp-frontend/config
The dependency in
packages/components/package.jsonhas been updated to"workspace:^"from the previous"workspace:*", making the allowed version range more restrictive. Running the dependency search revealed several other package manifests (inpackages/authentication,packages/graphql,packages/provider,packages/test,packages/utils,packages/design-system, andpackages/wagtail) that reference this dependency.Please verify that this stricter semver constraint is intended project-wide. In particular, ensure that:
- All packages that depend on
@baseapp-frontend/configare updated consistently if a unified version range is desired.- The new
"workspace:^"constraint does not introduce unintended version mismatches or conflicts within the dependency graph.README.md (1)
86-86: LGTM! Command update improves clarity.Using the full package name
@baseapp-frontend/authenticationinstead of justauthenticationis more explicit and consistent with the monorepo's naming convention.
anicioalexandre
force-pushed
the
BA-2259-improve-dev-mode
branch
from
e4a8414 to
1ea18a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/design-system/package.json (1)
61-86:⚠️ Potential issueClean up React Native dependencies.
These React Native dependencies should be removed since the
react-native-corecatalog has been removed from the workspace:
@expo/vector-icons@gorhom/bottom-sheet@react-navigation/drawer@react-navigation/nativeexpo-routerreact-nativereact-native-paperreact-native-reanimatedreact-native-svg
🧹 Nitpick comments (3)
packages/design-system/.scripts/build.mjs (1)
14-17: Improve error handling for file deletion.The current implementation silently ignores all errors during file deletion. It should at least log non-ENOENT errors.
try { await fs.unlink(readyFilePath) } catch (error) { - // pass + if (error.code !== 'ENOENT') { + console.warn(`${chalk.yellow(`${commandTag} Warning:`)} Failed to delete ready file: ${error.message}`) + } }packages/design-system/.scripts/dev-command.mjs (1)
78-84: Use optional chaining for stats check.The static analysis tool correctly identifies an opportunity to use optional chaining.
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/components/.scripts/dev-command.mjs (1)
83-84: Use optional chaining for stats check.The static analysis tool correctly identifies an opportunity to use optional chaining.
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.scripts/command-utils.mjs(1 hunks)README.md(1 hunks)package.json(1 hunks)packages/components/.scripts/build-command.mjs(1 hunks)packages/components/.scripts/build.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/README.md(1 hunks)packages/components/package.json(3 hunks)packages/components/tsup.config.ts(1 hunks)packages/design-system/.scripts/build-command.mjs(1 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/README.md(1 hunks)packages/design-system/package.json(2 hunks)packages/design-system/tsup.config.ts(1 hunks)pnpm-workspace.yaml(2 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/design-system/tsup.config.ts
- packages/design-system/.scripts/build-command.mjs
- packages/components/.scripts/build-command.mjs
- packages/components/README.md
- packages/components/tsup.config.ts
- package.json
- packages/components/.scripts/build.mjs
- turbo.json
- packages/components/package.json
- packages/design-system/README.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/design-system/.scripts/dev-command.mjs
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
README.md
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...
(EN_COMPOUNDS_MULTI_STEP)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (11)
packages/design-system/.scripts/build.mjs (1)
59-59: Fix potential memory leak in process management.The current implementation assigns an empty array to the parameter, which doesn't affect the original array reference.
- currentBuildProcesses = [] + currentBuildProcesses.length = 0packages/design-system/.scripts/dev-command.mjs (1)
91-93: Add cleanup for buildTimeout on watcher error.The buildTimeout isn't cleared when the watcher encounters an error, potentially leading to memory leaks.
watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) if (buildTimeout) clearTimeout(buildTimeout) buildTimeout = setTimeout(runWatchBuild, 2000) }) + + watcher.on('error', (error) => { + console.error(chalk.red(`${commandTag} Watcher error: ${error}`)) + if (buildTimeout) clearTimeout(buildTimeout) + }).scripts/command-utils.mjs (2)
23-24: Fix potential memory leak in process management.Similar to the previous issue, assigning an empty array to the parameter doesn't affect the original array.
- currentBuildProcesses = [] + currentBuildProcesses.length = 0
62-68: Add error handling for rsync command availability.The code assumes rsync is available but doesn't handle cases where it's not installed.
try { + try { + await execa('which', ['rsync']) + } catch (error) { + throw new Error('rsync is not installed. Please install rsync to continue.') + } await execa( "rsync", ["-av", "--delete", "--delay-updates", sourceDist, targetDist], { shell: true, } )packages/components/.scripts/dev-command.mjs (1)
36-38: Add path existence verification for design system.The code assumes the design-system path exists but doesn't verify it.
const designSystemPath = path.join(rootDir, '..', 'design-system') + try { + await fs.access(designSystemPath) + } catch (error) { + throw new Error(`Design system path not found: ${designSystemPath}`) + } const readyFilePath = path.join(designSystemPath, 'dist', 'build.ready')pnpm-workspace.yaml (2)
33-45: Verify complete removal of React Native dependencies.The
react-native-corecatalog has been removed, but there are still React Native dependencies inpackages/design-system/package.json. This inconsistency needs to be addressed.
146-149: LGTM! Well-chosen development tools.The new
dev-toolscatalog introduces essential utilities that align well with the PR's objective of improving dev mode:
chalk: For enhanced terminal output stylingchokidar: For efficient file watchingexeca: For reliable process executionpackages/design-system/package.json (2)
11-12: LGTM! Improved build process organization.Moving build and dev commands to dedicated scripts improves maintainability and allows for better error handling and logging.
116-121: LGTM! Dev tools integration.The addition of
chalk,chokidar, andexecafrom the dev-tools catalog aligns with the workspace configuration and supports the improved dev mode.README.md (2)
86-86: LGTM! Improved command clarity.Using the full package name
@baseapp-frontend/authenticationis more explicit and consistent with the monorepo structure.
91-124: LGTM! Excellent dev mode documentation.The new development mode documentation is comprehensive and well-structured:
- Clear explanation of the watch mode functionality
- Detailed breakdown of the multi-step build process
- Helpful example output showing the build stages
🧰 Tools
🪛 LanguageTool
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...(EN_COMPOUNDS_MULTI_STEP)
anicioalexandre
force-pushed
the
BA-2259-improve-dev-mode
branch
from
1ea18a1 to
7afb5f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.scripts/command-utils.mjs (2)
23-24:⚠️ Potential issueFix potential memory leak in process management.
The attempt to clear the array by reassignment won't affect the original array reference.
Apply this diff to properly clear the array:
await execa("pnpm", ["clean:tmp"], { preferLocal: true }) - currentBuildProcesses = [] + currentBuildProcesses.length = 0
61-69:⚠️ Potential issueAdd error handling for rsync command availability.
The code assumes rsync is available but doesn't handle cases where it's not installed.
Apply this diff to add error handling:
try { + try { + await execa('which', ['rsync']) + } catch (error) { + throw new Error('rsync is not installed. Please install rsync to continue.') + } await execa( "rsync", ["-av", "--delete", "--delay-updates", sourceDist, targetDist], { shell: true, } )packages/design-system/.scripts/dev-command.mjs (1)
88-93:⚠️ Potential issueAdd cleanup for buildTimeout on watcher error.
The buildTimeout isn't cleared when the watcher encounters an error, potentially leading to memory leaks.
Apply this diff:
watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) if (buildTimeout) clearTimeout(buildTimeout) buildTimeout = setTimeout(runWatchBuild, 2000) }) + + watcher.on('error', (error) => { + console.error(chalk.red(`${commandTag} Watcher error: ${error}`)) + if (buildTimeout) clearTimeout(buildTimeout) + })
🧹 Nitpick comments (4)
packages/components/.scripts/dev-command.mjs (1)
83-88: Use optional chaining for safer property access.The
statsparameter might be undefined, making the current property access unsafe.Apply this diff to use optional chaining:
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/design-system/.scripts/dev-command.mjs (3)
23-58: Enhance error logging for better debugging.While the error handling is comprehensive, consider adding more detailed error logging to help with debugging build failures.
Apply this diff to enhance error logging:
} catch (error) { if (error.signal !== 'SIGTERM') { console.error(chalk.red(`${commandTag} Build failed:`)) + console.error(chalk.red(`${commandTag} Error type: ${error.name}`)) + console.error(chalk.red(`${commandTag} Stack trace:`)) console.error(chalk.red(error.stderr || error.message)) + console.error(chalk.red(error.stack)) } }
78-84: Use optional chaining for safer property access.The static analysis tool suggests using optional chaining for safer property access.
Apply this diff:
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
70-85: Consider modularizing the file filtering logic.The file filtering logic could be extracted into a separate function for better maintainability and reusability.
Apply this diff:
+const isIgnoredFile = (filePath, stats, rootDir) => { + if ( + filePath.includes('node_modules') || + filePath.includes('dist') || + filePath.includes('tmp') + ) { + return true + } + if (stats?.isFile()) { + const relative = path.relative(rootDir, filePath).replace(/\\/g, '/') + if (!watchRegex.test(relative)) { + return true + } + } + return false +} ignored: (filePath, stats) => { - if ( - filePath.includes('node_modules') || - filePath.includes('dist') || - filePath.includes('tmp') - ) { - return true - } - if (stats && stats.isFile()) { - const relative = path.relative(rootDir, filePath).replace(/\\/g, '/') - if (!watchRegex.test(relative)) { - return true - } - } - return false + return isIgnoredFile(filePath, stats, rootDir) },🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.scripts/command-utils.mjs(1 hunks)README.md(2 hunks)package.json(1 hunks)packages/components/.scripts/build-command.mjs(1 hunks)packages/components/.scripts/build.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/README.md(1 hunks)packages/components/package.json(3 hunks)packages/components/tsup.config.ts(1 hunks)packages/design-system/.scripts/build-command.mjs(1 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/README.md(1 hunks)packages/design-system/package.json(2 hunks)packages/design-system/tsup.config.ts(1 hunks)pnpm-workspace.yaml(2 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/design-system/tsup.config.ts
- turbo.json
- packages/design-system/.scripts/build-command.mjs
- packages/components/.scripts/build-command.mjs
- packages/components/.scripts/build.mjs
- packages/design-system/package.json
- packages/components/README.md
- packages/design-system/README.md
- packages/design-system/.scripts/build.mjs
- package.json
- pnpm-workspace.yaml
- packages/components/package.json
- packages/components/tsup.config.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...
(EN_COMPOUNDS_MULTI_STEP)
🪛 Biome (1.9.4)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/design-system/.scripts/dev-command.mjs
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (7)
.scripts/command-utils.mjs (1)
80-95: Well-structured error handling and resource cleanup!The implementation properly handles both success and error cases, ensuring the file watcher is always closed.
packages/components/.scripts/dev-command.mjs (1)
96-97: Well-implemented debouncing logic!The code properly clears existing timeouts before setting new ones, preventing build thrashing.
README.md (2)
91-94: Excellent documentation of the development mode!The new section clearly explains the immediate feedback loop and automatic synchronization features.
107-124: Well-structured build process documentation!The example output effectively illustrates each step of the build process, making it easy for developers to understand what's happening.
packages/design-system/.scripts/dev-command.mjs (3)
1-22: LGTM! Well-organized imports and state management.The imports are appropriate, and the state variables are clearly named and well-organized for managing the build process.
60-60: LGTM! Well-structured regex pattern.The regular expression effectively filters relevant directories for watching.
95-95: LGTM! Appropriate initial build invocation.The initial build is correctly triggered after setting up the watcher.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.scripts/command-utils.mjs (2)
23-24:⚠️ Potential issueFix potential memory leak in process management.
Assigning an empty array to the parameter doesn't affect the original array reference.
- currentBuildProcesses = [] + currentBuildProcesses.length = 0
61-69: 🛠️ Refactor suggestionAdd error handling for rsync command availability.
The code should verify that rsync is available before attempting to use it.
try { + try { + await execa('which', ['rsync']) + } catch (error) { + throw new Error('rsync is not installed. Please install rsync to continue.') + } await execa( "rsync", ["-av", "--delete", "--delay-updates", sourceDist, targetDist], { shell: true, } )
🧹 Nitpick comments (3)
packages/components/.scripts/dev-command.mjs (1)
75-91: Consider using optional chaining for stats check.The stats check can be simplified using optional chaining for better readability.
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/design-system/.scripts/dev-command.mjs (1)
90-96: Consider using optional chaining for stats check.Similar to the components file, the stats check can be simplified.
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
.scripts/command-utils.mjs (1)
108-110: Improve error handling in cleanupReadyFile.Silent error handling should at least log debug information for troubleshooting.
try { await fs.unlink(readyFilePath) } catch (err) { - // pass + console.debug(`Failed to cleanup ready file: ${err.message}`) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.scripts/command-utils.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/package.json(3 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/package.json
- packages/design-system/package.json
- packages/design-system/.scripts/build.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
packages/design-system/.scripts/dev-command.mjs
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
anicioalexandre
force-pushed
the
BA-2259-improve-dev-mode
branch
from
d935304 to
f25e030
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/components/.scripts/dev-command.mjs (1)
93-98:⚠️ Potential issueAdd error handling for the file watcher.
The watcher should handle potential errors to prevent silent failures.
watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) if (buildTimeout) clearTimeout(buildTimeout) buildTimeout = setTimeout(runWatchBuild, 2000) }) + +watcher.on('error', (error) => { + console.error(chalk.red(`${commandTag} Watcher error: ${error}`)) + if (buildTimeout) clearTimeout(buildTimeout) +})packages/design-system/.scripts/dev-command.mjs (2)
26-31:⚠️ Potential issueAdd cleanup for build timeout in SIGINT handler.
The SIGINT handler should clean up the build timeout to prevent memory leaks.
process.on('SIGINT', async () => { const readyFilePath = path.join(rootDir, 'dist', 'build.ready') + if (buildTimeout) clearTimeout(buildTimeout) await cleanupReadyFile({ readyFilePath }) process.exit() })
100-105:⚠️ Potential issueAdd cleanup for buildTimeout on watcher error.
The buildTimeout isn't cleared when the watcher encounters an error, potentially leading to memory leaks.
watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) if (buildTimeout) clearTimeout(buildTimeout) buildTimeout = setTimeout(runWatchBuild, 2000) }) + +watcher.on('error', (error) => { + console.error(chalk.red(`${commandTag} Watcher error: ${error}`)) + if (buildTimeout) clearTimeout(buildTimeout) +}).scripts/command-utils.mjs (2)
23-24:⚠️ Potential issueFix potential memory leak in process management.
Assigning an empty array to the parameter doesn't affect the original array.
- currentBuildProcesses = [] + currentBuildProcesses.length = 0
61-69:⚠️ Potential issueAdd error handling for rsync command availability.
The code assumes rsync is available but doesn't handle cases where it's not installed.
try { + try { + await execa('which', ['rsync']) + } catch (error) { + throw new Error('rsync is not installed. Please install rsync to continue.') + } await execa( "rsync", ["-av", "--delete", "--delay-updates", sourceDist, targetDist], { shell: true, } )
🧹 Nitpick comments (3)
packages/components/.scripts/dev-command.mjs (2)
19-22: Consider using a class to manage build state.The current implementation uses global variables for state management. Consider encapsulating the build state in a class for better maintainability and testability.
-let isBuilding = false -let needsRebuild = false -let buildTimeout = null -let currentBuildProcesses = [] +class BuildManager { + constructor() { + this.isBuilding = false + this.needsRebuild = false + this.buildTimeout = null + this.currentBuildProcesses = [] + } + // ... methods to manage state +} +const buildManager = new BuildManager()
75-91: Optimize file watcher configuration.The current polling interval might be too aggressive. Consider increasing the interval and stability threshold for better performance.
const watcher = chokidar.watch(rootDir, { ignoreInitial: true, usePolling: true, - interval: 100, + interval: 1000, awaitWriteFinish: { - stabilityThreshold: 500, + stabilityThreshold: 2000, pollInterval: 100, },🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
.scripts/command-utils.mjs (1)
105-111: Improve error handling in cleanupReadyFile.The current implementation silently ignores all errors. Consider logging specific error types.
export const cleanupReadyFile = async ({readyFilePath}) => { try { await fs.unlink(readyFilePath) } catch (err) { - // pass + if (err.code !== 'ENOENT') { + console.warn(`Failed to cleanup ready file: ${err.message}`) + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.scripts/command-utils.mjs(1 hunks)README.md(3 hunks)package.json(1 hunks)packages/components/.scripts/build-command.mjs(1 hunks)packages/components/.scripts/build.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/README.md(1 hunks)packages/components/package.json(3 hunks)packages/components/tsup.config.ts(1 hunks)packages/design-system/.scripts/build-command.mjs(1 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/README.md(1 hunks)packages/design-system/package.json(2 hunks)packages/design-system/tsup.config.ts(1 hunks)pnpm-workspace.yaml(2 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/components/.scripts/build-command.mjs
- packages/components/tsup.config.ts
- packages/components/README.md
- packages/design-system/tsup.config.ts
- packages/design-system/README.md
- turbo.json
- package.json
- packages/components/package.json
- packages/design-system/.scripts/build.mjs
- packages/components/.scripts/build.mjs
- packages/design-system/.scripts/build-command.mjs
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...
(EN_COMPOUNDS_MULTI_STEP)
[style] ~131-~131: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...sumption and CPU usage if you’re making a lot of simultaneous changes. It is recommende...
(A_LOT_OF)
[uncategorized] ~133-~133: Possible missing comma found.
Context: ... use this live mode only at appropriate times rather than throughout your entire deve...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
README.md
125-125: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🪛 Biome (1.9.4)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/design-system/.scripts/dev-command.mjs
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (8)
pnpm-workspace.yaml (2)
33-45:❓ Verification inconclusive
Verify impact of removing React Native dependencies.
The removal of the
react-native-corecatalog is a significant change. Please ensure all React Native specific components have been migrated or removed.
🏁 Script executed:
#!/bin/bash # Search for usage of removed React Native packages rg -l "@react-navigation|expo-|react-native" --type tsx --type tsLength of output: 95
🏁 Script executed:
#!/bin/bash # Updated script: Search for usage of removed React Native packages using glob patterns for TypeScript files. rg -l "@react-navigation|expo-|react-native" -g "*.ts" -g "*.tsx"Length of output: 6593
Action Required: Review React Native Dependency Usage
The updated state in
pnpm-workspace.yaml(lines 33-45) still lists several React Native-related packages. Our search reveals that these packages (e.g.,@react-navigation/*,expo-*,react-native) are referenced across many parts of the codebase—including production components (like those underpackages/components/modules/profiles/native/) as well as in mocks. Please manually verify that all React Native-specific components have been either successfully migrated or intentionally maintained, ensuring that removing or refactoring these dependencies won’t lead to breaking changes.
146-149:❓ Verification inconclusive
Verify compatibility of dev tool versions.
The new dev tools are using caret versions which might lead to compatibility issues.
🌐 Web query:
What are the latest stable versions of chalk, chokidar, and execa? Are there any known compatibility issues between these versions?💡 Result:
The latest stable versions and compatibility considerations for these packages are:
Latest Versions
Package Latest Stable Key Details Chalk 5.4.1 Pure ESM requiring Node.js ≥12.20
No dependencies after v5[1][6]Chokidar 4.0.3 Requires Node.js ≥14
Dropped glob support[2][7][5]Execa 9.5.2 Latest major version with IPC enhancements
Requires npm-run-path v7+[8]Compatibility Considerations
Chalk v5:
- Pure ESM package (cannot use with CommonJS without transpilation)[1]
- Removed
.keyword()/.hsl()methods - usecolor-convertinstead[1]- Tagged template literals moved to
chalk-templatepackage[1]Chokidar v4:
- Breaking changes vs v3:
- No glob pattern support (requires manual filtering)[5]
- Dropped
fseventsdependency (native Node.js watcher)[7]- Hybrid CJS/ESM format[2]
Execa v9:
- Requires Node.js ≥16 (check your environment)[8]
- Potential conflicts with older versions of
npm-run-path[8]Version Matrix Compatibility
Combination Status Notes Chalk 5 + Chokidar 4 Stable Modern ESM stack Chalk 5 + Execa 9 Stable Both require recent Node.js versions Chokidar 4 + Execa 9 Stable No dependency overlaps Key warnings:
- Chokidar ≤3.x breaks on Node.js ≥14[4][7]
- Execa ≤7.x has security advisories for subdependencies[3]
- Chalk ≤4.x remains supported but lacks modern features like overline styling[1][6]
For legacy systems:
- Use Chalk 4.x if needing CommonJS
- Keep Chokidar ≤3.x only if requiring glob patterns
- Execa ≤7.x should be avoided due to security concerns[3][8]
Citations:
- 1: https://github.com/chalk/chalk/releases
- 2: https://github.com/paulmillr/chokidar/releases
- 3: https://www.cdnpkg.com/execa
- 4: vuejs/vue-cli#5690
- 5: https://dev.to/43081j/migrating-from-chokidar-3x-to-4x-5ab5
- 6: https://www.npmjs.com/package/chalk?activeTab=versions
- 7: https://security.snyk.io/package/npm/chokidar
- 8: https://github.com/sindresorhus/execa/releases
- 9: https://discourse.gohugo.io/t/hugo-115-1-and-postcss-do-not-work-well-enough-to-be-usable/45342
- 10: https://crashoverride.com/docs/chalk/release-notes
Action: Verify Node.js Version Compatibility for Updated Dev Tools
The current versions in
pnpm-workspace.yamlare the latest stable releases:
- Chalk 5.4.1: A pure ESM package requiring Node.js ≥12.20. Ensure that the project supports ESM, or consider transpilation if using CommonJS.
- Chokidar 4.0.3: Requires Node.js ≥14 and no longer supports glob patterns. Confirm that glob pattern support isn’t needed in our workflow.
- Execa 9.5.2: Requires Node.js ≥16 due to its latest enhancements. Verify that our environment meets this requirement.
Please double-check that our target environment’s Node.js version aligns with these requirements to avoid any compatibility issues.
packages/design-system/package.json (3)
11-12: LGTM! Build and dev scripts are now more maintainable.The changes improve the development workflow by:
- Using dedicated scripts for better modularity
- Cleaning old artifacts before starting dev mode
116-117: LGTM! New dev dependencies support improved development workflow.The added dependencies (chalk, chokidar, execa) are appropriate for enhancing the build and watch processes.
Also applies to: 121-121
6-6:✅ Verification successful
Verify the security of requests package version.
Let's check if version 2.32.3 of requests is the latest and if there are any known security vulnerabilities.
🌐 Web query:
What is the latest version of the requests package in Python? Are there any known security vulnerabilities in version 2.32.3?💡 Result:
The latest version of the Python
requestspackage is v2.32.3, as documented in its official release notes[1]. This version includes security patches and improvements over previous releases.Security Vulnerabilities in v2.32.x
A critical vulnerability (CVE-2024-35195) was identified in earlier versions of Requests (v2.x up to v2.31.x), where disabling certificate verification (
verify=False) on a Session's first request caused subsequent requests to ignore certificate validation[4][5]. This was fixed in v2.32.x via:# Example fix implemented in v2.x: session = requests.Session() session.get("https://example.com", verify=False) # Subsequent requests now honor `verify=True` if set[4][5]However:
1️⃣ A critical malicious code vulnerability was reported inrequests-2-32-3(hyphenated naming variant) by ReversingLabs[2]. This appears unrelated to the officialrequests==2.32.*releases (using underscores), suggesting either:
- A compromised fork/typosquatting attack
- False positive reporting
The official Requests v2.x series shows no unresolved vulnerabilities as of February 2025[8][10].
Recommendations
To safely use Requests:
# Install/upgrade to latest secure version: pip install --upgrade requests==2.*Always verify package authenticity via PyPI (
https://pypi.org/project/requests/)[9].Citations:
- 1: https://requests.readthedocs.io
- 2: https://vulert.com/vuln-db/pypi-requests-2-32-3-177728
- 3: https://github.com/psf/requests/releases
- 4: https://data.safetycli.com/vulnerabilities/CVE-2024-35195/71064/
- 5: https://github.com/psf/requests/blob/main/HISTORY.md
- 6: https://security.snyk.io/package/pip/requests
- 7: https://www.activestate.com/resources/quick-reads/how-to-pip-install-requests-python-package/
- 8: https://requests.readthedocs.io/en/latest/community/vulnerabilities/
- 9: https://pypi.org/project/requests/
- 10: https://security.snyk.io/package/pip/requests/2.32.3
Requests package (2.32.3) security verified
Version 2.32.3 is confirmed as the latest official release of the Requests library. It contains the necessary fixes for earlier vulnerabilities (such as CVE-2024-35195) and there are no unresolved security issues in this release. Please ensure that installations are sourced directly from the official PyPI repository to avoid any typosquatting or compromised variants.
README.md (3)
86-86: LGTM! Package name is now more explicit.Using the full scoped package name (
@baseapp-frontend/authentication) improves clarity and follows monorepo best practices.
91-134: LGTM! Excellent documentation of the development mode.The new documentation:
- Clearly explains the dev mode functionality
- Provides helpful build step examples
- Includes important resource usage considerations
🧰 Tools
🪛 LanguageTool
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...(EN_COMPOUNDS_MULTI_STEP)
[style] ~131-~131: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...sumption and CPU usage if you’re making a lot of simultaneous changes. It is recommende...(A_LOT_OF)
[uncategorized] ~133-~133: Possible missing comma found.
Context: ... use this live mode only at appropriate times rather than throughout your entire deve...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
125-125: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
149-150: LGTM! Improved catalog management documentation.The documentation now:
- Uses more generic commands for catalog management
- Includes important installation verification steps
Also applies to: 158-159
anicioalexandre
force-pushed
the
BA-2259-improve-dev-mode
branch
from
f25e030 to
e183314
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/components/.scripts/dev-command.mjs (1)
93-98:⚠️ Potential issueAdd error handling for the file watcher.
The watcher should handle potential errors to prevent silent failures.
Apply this diff to add error handling:
watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) if (buildTimeout) clearTimeout(buildTimeout) buildTimeout = setTimeout(runWatchBuild, 2000) }) + +watcher.on('error', (error) => { + console.error(chalk.red(`${commandTag} Watcher error: ${error}`)) + if (buildTimeout) clearTimeout(buildTimeout) +})packages/design-system/.scripts/dev-command.mjs (1)
100-105:⚠️ Potential issueAdd cleanup for buildTimeout on watcher error.
The buildTimeout isn't cleared when the watcher encounters an error.
.scripts/command-utils.mjs (2)
23-24:⚠️ Potential issueFix potential memory leak in process management.
Assigning an empty array to the parameter doesn't affect the original array.
Apply this diff to fix the memory leak:
- currentBuildProcesses = [] + currentBuildProcesses.length = 0
61-69:⚠️ Potential issueAdd error handling for rsync command availability.
The code assumes rsync is available but doesn't handle cases where it's not installed.
Apply this diff to add error handling:
try { + try { + await execa('which', ['rsync']) + } catch (error) { + throw new Error('rsync is not installed. Please install rsync to continue.') + } await execa( "rsync", ["-av", "--delete", "--delay-updates", sourceDist, targetDist], { shell: true, } )
🧹 Nitpick comments (4)
packages/components/.scripts/dev-command.mjs (1)
65-91: Optimize file watching configuration.The current implementation uses polling which can be resource-intensive. Consider using native file system events when possible.
Apply this diff to optimize the watcher configuration:
const watcher = chokidar.watch(rootDir, { ignoreInitial: true, - usePolling: true, - interval: 100, + usePolling: process.platform === 'win32', // Only use polling on Windows + interval: process.platform === 'win32' ? 100 : 1000, awaitWriteFinish: { stabilityThreshold: 500, pollInterval: 100, }, ignored: (filePath, stats) => { if ( filePath.includes('node_modules') || filePath.includes('dist') || filePath.includes('tmp') ) { return true } - if (stats && stats.isFile()) { + if (stats?.isFile()) { const relative = path.relative(rootDir, filePath).replace(/\\/g, '/') if (!watchRegex.test(relative)) { return true } } return false }, })🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/design-system/.scripts/dev-command.mjs (1)
33-70: Add retry mechanism for build failures.The build process should implement a retry mechanism for transient failures.
Apply this diff to add retry logic:
const runWatchBuild = async () => { + const maxRetries = 3 + let retryCount = 0 + await cleanupReadyFile({ readyFilePath }) if (isBuilding) { needsRebuild = true await cancelCurrentBuild({ commandTag, currentBuildProcesses }) return } isBuilding = true try { const consumerAppPath = getConsumerAppBasePath() - await runBuild(currentBuildProcesses) + while (retryCount < maxRetries) { + try { + await runBuild(currentBuildProcesses) + break + } catch (error) { + retryCount++ + if (retryCount === maxRetries) throw error + console.log(chalk.yellow(`${commandTag} Build failed, retrying (${retryCount}/${maxRetries})...`)) + await new Promise(resolve => setTimeout(resolve, 1000)) + } + }README.md (2)
120-124: Refine Disclaimer Section
The disclaimer effectively warns users about potential increased resource consumption during continuous rebuilds. Consider rephrasing "a lot of simultaneous changes" to a more formal alternative for clarity. For example:- if you’re making a lot of simultaneous changes. + if you’re making numerous simultaneous changes.
120-120: Consider Using a Heading for "Disclaimer"
Markdownlint recommends using headings rather than emphasized text for section titles. Consider converting "Disclaimer" to a heading (e.g. "### Disclaimer") for improved semantic structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.scripts/command-utils.mjs(1 hunks)README.md(3 hunks)package.json(1 hunks)packages/components/.scripts/build-command.mjs(1 hunks)packages/components/.scripts/build.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/README.md(1 hunks)packages/components/package.json(3 hunks)packages/components/tsup.config.ts(1 hunks)packages/design-system/.scripts/build-command.mjs(1 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/README.md(1 hunks)packages/design-system/package.json(2 hunks)packages/design-system/tsup.config.ts(1 hunks)pnpm-workspace.yaml(2 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/design-system/.scripts/build-command.mjs
- packages/design-system/tsup.config.ts
- packages/components/tsup.config.ts
- packages/components/README.md
- packages/components/.scripts/build-command.mjs
- turbo.json
- packages/design-system/README.md
- package.json
- packages/components/.scripts/build.mjs
- packages/design-system/package.json
- packages/design-system/.scripts/build.mjs
- packages/components/package.json
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...
(EN_COMPOUNDS_MULTI_STEP)
[style] ~131-~131: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...sumption and CPU usage if you’re making a lot of simultaneous changes. It is recommende...
(A_LOT_OF)
🪛 markdownlint-cli2 (0.17.2)
README.md
125-125: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🪛 Biome (1.9.4)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/design-system/.scripts/dev-command.mjs
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (9)
packages/design-system/.scripts/dev-command.mjs (1)
74-98: Optimize watcher configuration for better performance.Similar to the components package, the watcher configuration can be optimized.
🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
pnpm-workspace.yaml (2)
146-149: LGTM! Development tools added.The addition of
chalk,chokidar, andexecaaligns with the PR's objective to improve dev mode.
33-45:❓ Verification inconclusive
Verify impact of removing React Native dependencies.
The removal of the
react-native-corecatalog is a significant change.Run the following script to verify the removal:
🏁 Script executed:
#!/bin/bash # Search for usage of removed React Native packages rg -l "@react-navigation|expo-|react-native" --type tsx --type tsLength of output: 95
Action: Verify Impact of Removing React Native Dependencies Manually
We've updated the verification command below to include explicit file type definitions (for TS and TSX files) to avoid the "unrecognized file type: tsx" error. Please run the updated command and confirm that there are no references to the removed dependencies (e.g., any usage of "@react-navigation", "expo‑", or "react‑native") in the codebase.
- Steps:
Execute the following shell script:
# Extended search for React Native package references in TypeScript files rg --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -l '@react-navigation|expo-|react-native'Verify that the command returns no output. If it does, review each occurrence to ensure that the removal of the
react-native-corecatalog does not cause unintended side effects.The removal of the entire
react-native-corecatalog is a significant change, so a careful manual verification is required.README.md (6)
86-86: Verify Build Command Update
The build command now specifically targets@baseapp-frontend/authentication. Please confirm that this change reflects the intended package naming convention across the repository.
91-93: Review Dev Mode Introduction
The new dev mode section clearly explains how packages automatically watch for changes, rebuild, and synchronize outputs. Verify that the described behavior accurately reflects the actual implementation.
97-97: Double-check Potential Misspelling
Static analysis flagged a potential misspelling at this line. The term "multi-step" appears to be correct; please verify that no unintended typo exists.🧰 Tools
🪛 LanguageTool
[misspelling] ~97-~97: This word is normally spelled as one.
Context: ...@baseapp-frontend/design-system`—have a multi-step build process. When you run: ```bash p...(EN_COMPOUNDS_MULTI_STEP)
99-119: Validate Dev Mode Output Example
The example output provides a comprehensive walkthrough of the dev mode process—including waiting, starting the build, transpilation, bundling, and syncing. Ensure the logs and their order match what is actually produced during a dev mode run.
146-151: Review Catalog Removal Instructions
The updated instructions now clarify the need to remove catalog entries before using unpublished GitHub versions. The commands (pnpm ifollowed bypnpm replace-catalogs) align with the updated dependency management strategy.
157-160: Review Catalog Restoration Commands
The restoration instructions usingpnpm iandpnpm restore-catalogsare clear and concise. Please verify that these commands correctly reinstate catalog entries across packages.
Summary by CodeRabbit
New Features
Documentation
Chores
react-native-corecatalog and introduction of a newdev-toolscatalog with development dependencies.