-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(root): MRK-876 Removed logs to reduces load in the terminal #8501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
WalkthroughThe changes rename interfaces to use an "I" prefix, update type annotations for improved type safety, remove unused imports and self-removal logic, and standardize logging output. String escaping for template literals is enhanced. Minor formatting and type casting adjustments are applied, and the package version is incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant PackageManager
participant Logger
User->>CLI: Run CLI command
CLI->>PackageManager: installDependencies()
PackageManager-->>CLI: Install result
CLI->>CLI: Verify installation (package.json, node_modules)
alt Installation fails
CLI->>PackageManager: Restore package.json backup
CLI->>Logger: Log error
else Installation succeeds
CLI->>Logger: Log next steps info
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/add-inbox/src/generators/frameworks/react.ts (1)
17-21: Safer NaN check is 👍, but consider a full semver parseSwitching from the global
isNaNtoNumber.isNaNprevents the surprising' 'coercion edge-case – nice.
If you ever need to compare pre-releases (18.0.0-rc.0etc.) or handle caret/tilde ranges, a dedicated parser such assemverwould be more robust than a manual split.packages/add-inbox/src/config/framework.ts (3)
12-16: Nit:IPackageJsoncould leverage the official type
type PackageJson = import('type-fest').PackageJson;gives you a vetted, exhaustive declaration instead of maintaining a local subset.
34-45:getPackageJsonsynchronously blocks the event-loopCLI tools are usually fine, but if this ever runs inside long-lived processes a non-blocking variant (
fs.promises) avoids IO pauses.-const packageJsonPath = path.join(process.cwd(), 'package.json'); -if (!fs.existsSync(packageJsonPath)) { +const packageJsonPath = path.join(process.cwd(), 'package.json'); +try { + await fs.promises.access(packageJsonPath); +} catch { return null; } - -return JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')); +const raw = await fs.promises.readFile(packageJsonPath, 'utf8'); +return JSON.parse(raw);
100-104: Return type doc updated but not the comment aboveThe JSDoc above
detectFrameworkstill mentionsFrameworkin a few places. Update toIFrameworkfor consistency.packages/add-inbox/src/utils/logger.ts (1)
18-24: Potential naming collision instepsignatureThe parameter name
numbershadows the primitivenumbertype, which can confuse code-readers and some IDE quick-info popups.-step: (number: number, title: string) => void; +step: (stepNumber: number, title: string) => void;packages/add-inbox/src/cli/index.ts (1)
410-410: Good grammatical fix.The apostrophe correction improves the professionalism of the user-facing text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/add-inbox/src/cli/index.ts(6 hunks)packages/add-inbox/src/config/framework.ts(5 hunks)packages/add-inbox/src/generators/component.ts(1 hunks)packages/add-inbox/src/generators/env.ts(1 hunks)packages/add-inbox/src/generators/frameworks/nextjs.ts(1 hunks)packages/add-inbox/src/generators/frameworks/react.ts(2 hunks)packages/add-inbox/src/utils/logger.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/add-inbox/src/config/framework.ts (2)
packages/add-inbox/src/constants/index.ts (1)
FrameworkType(6-6)scripts/setup-env-files.js (2)
path(2-2)fs(1-1)
packages/add-inbox/src/generators/component.ts (1)
packages/add-inbox/src/config/framework.ts (1)
IFramework(6-10)
packages/add-inbox/src/cli/index.ts (1)
packages/add-inbox/src/config/framework.ts (1)
IFramework(6-10)
🔇 Additional comments (13)
packages/add-inbox/src/generators/env.ts (1)
1-1: Good catch: removed unused constant importThe
FRAMEWORKSconstant wasn’t referenced anywhere in this module, so dropping it keeps the bundle lean and avoids eslint / TS warnings.packages/add-inbox/src/config/framework.ts (2)
6-10: Interface rename looks fine but update re-exports
IFrameworkis exported, yet downstream code might still importFramework. Ensure all imports are updated (IDE-wide rename is easy to miss in templates/tests).
88-90: Explicit radix &Number.isNaN: good!The added
, 10and stricter NaN check remove two subtle bugs (octal parsing & false positives). ✔︎packages/add-inbox/src/generators/component.ts (1)
7-11: Type alignment completedImporting
IFrameworkand updating the parameter keeps the generics in sync with the refactor. No functional impact.packages/add-inbox/src/utils/logger.ts (2)
3-9: Interface rename & unknown[] : solid upgradeChanging
any[]tounknown[]tightens type-safety without runtime cost – good move.
29-31: Template-literal refactor ✔︎Concat → template literal reduces cognitive load and avoids accidental spacing issues. 👍
packages/add-inbox/src/generators/frameworks/nextjs.ts (3)
1-27: Good refactoring to follow TypeScript naming conventions.The interface renaming with the "I" prefix and the type improvement from
anytounknownenhance type safety and maintain consistency with the codebase conventions.
37-38: Excellent security enhancement for template literal escaping.The addition of backtick and
${escaping prevents potential code injection issues when generating dynamic code within template literals.
40-40: Correct fix for the Next.js 'use client' directive.Using a plain single quote instead of a backslash-escaped quote is the proper syntax for Next.js directives.
packages/add-inbox/src/cli/index.ts (4)
8-9: Consistent type updates align with interface renaming.The changes from
FrameworktoIFrameworkmaintain consistency with the interface renaming across the codebase.Also applies to: 21-21, 178-178
192-288: Excellent improvements to dependency installation robustness.The enhanced error handling with backup/restore functionality, package verification, and timeout configuration significantly improves the reliability of the installation process.
293-315: Good logging standardization, but PR title seems inconsistent.The wrapping of colorized logger calls with
logger.info()provides better consistency. However, the PR title mentions "Removed logs to reduces load in the terminal" while these changes standardize rather than remove logs.Likely an incorrect or invalid review comment.
394-399:Details
✅ Verification successful
Verify the createComponentStructure function signature change.
The parameter change from what was likely
framework.frameworkto justframeworksuggests the function now accepts the fullIFrameworkobject.
🏁 Script executed:
#!/bin/bash # Description: Verify the createComponentStructure function signature accepts IFramework # Search for the createComponentStructure function definition ast-grep --pattern 'export $_ function createComponentStructure($_) { $$$ }' # Also search with async ast-grep --pattern 'export $_ async function createComponentStructure($_) { $$$ }'Length of output: 167
I’ll locate where
createComponentStructureis defined to inspect its first parameter’s type.
🏁 Script executed:
#!/bin/bash # Find all usages and definitions of createComponentStructure in .ts files rg "createComponentStructure" -n --glob "*.ts" echo -e "\nSearching specifically for its export/definition" rg "export.*createComponentStructure" -n --glob "*.ts"Length of output: 593
Inspecting the signature directly:
🏁 Script executed:
#!/bin/bash # Show the first 30 lines of the component generator to view the createComponentStructure signature sed -n '1,30p' packages/add-inbox/src/generators/component.tsLength of output: 1155
createComponentStructure signature confirmed
The
createComponentStructurefunction is exported as:export async function createComponentStructure( framework: IFramework, overwriteComponents: boolean, subscriberId: string | null | undefined, region: 'us' | 'eu' = 'us' ): Promise<void> { … }It explicitly accepts an
IFrameworkfor its first parameter, so passing the fullframeworkobject is correct. No further changes are required.
commit: |
Summary by CodeRabbit