Release 25.10.01 #23
Release 25.10.01 #23
Conversation
WalkthroughAdds a new NestJS Bedrock AI package (module, service, controller, DTOs, tests, configs, README), introduces language-specific SonarCloud property files and CI steps to position/fix them, removes the root sonar-project.properties content, and applies broad dependency version bumps plus small test/type/JSX tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as BedrockController
participant Service as BedrockService
participant LLM as LangChain Bedrock
Client->>Controller: POST /ai-chats { promptText, input }
Controller->>Service: generate(promptText, input)
Service->>Service: validate inputs
Service->>LLM: invoke(SystemMessage, HumanMessage)
alt success
LLM-->>Service: AI text
Service-->>Controller: { output: text }
Controller-->>Client: 200 { output }
else failure
LLM-->>Service: error
Service-->>Controller: throw HttpException(500, "AI service unavailable")
Controller-->>Client: 500 error
end
sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Move as Move sonar.*.properties
participant Fix as Fix coverage.xml paths
participant Scan as SonarCloud Scan
GH->>Move: Rename/move `sonar.ts.properties` / `sonar.python.properties` -> `sonar-project.properties`
opt Python workflow present
GH->>Fix: Rewrite coverage.xml source paths (runner -> github workspace)
end
GH->>Scan: Run SonarCloud Scan using resulting `sonar-project.properties`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
typescript/packages/mobile-components/package.json (1)
39-39: Keep the minimum preset version at 7.24.7 or higher.Line [39] relaxes the constraint from the known-good
7.24.7to^7.20.0, which allows installers (especially in fresh environments or mirrors) to resolve as far back as7.20.0. That downgrades the minimum feature set we rely on. Stick with^7.24.7(or a newer upper-minor) so we preserve the fixes present in 7.24.x while still allowing automatic patch bumps.typescript/packages/nestjs-modules/bedrock/jest.config.ts (1)
4-7: Optional: async function wrapper unnecessary.The function doesn't perform any async operations, so the
asynckeyword is unnecessary.-export default async (): Promise<Config> => ({ +export default (): Config => ({ ...nodeConfig, coveragePathIgnorePatterns: ['mocks'] })typescript/packages/nestjs-modules/bedrock/src/bedrock.service.spec.ts (1)
44-44: Note: Type assertion acceptable for test mocks.The
as anycast for the mock AIMessageChunk is acceptable in test context, though if AIMessageChunk has a simple constructor or factory, using that would be more type-safe.typescript/packages/nestjs-modules/bedrock/package.json (1)
22-30: Update @langchain/core to 0.3.78 and verify security
- Bump @langchain/core from 0.3.77 to 0.3.78 to align with the latest patch release.
- Confirm no known vulnerabilities exist for any listed dependencies.
typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts (2)
27-33: Consider lazy initialization to defer AWS credential validation.The
ChatBedrockConverseclient is initialized eagerly in the constructor. If AWS credentials are missing or invalid, the service will fail to instantiate, potentially preventing the entire NestJS application from starting. Consider lazy initialization (initializingllmon first use in thegeneratemethod) to defer credential validation and provide clearer error messages at invocation time rather than startup.Example lazy initialization pattern:
- constructor(private configService: ConfigService) { - this.llm = new ChatBedrockConverse({ - model: this.configService.get<string>('AI_MODEL_NAME') || 'us.meta.llama4-maverick-17b-instruct-v1:0', - temperature: this.configService.get<number>('AI_MODEL_TEMPERATURE') || 0.3, - maxRetries: this.configService.get<number>('AI_MODEL_MAX_RETRIES') || 4 - }) - } + constructor(private configService: ConfigService) {} + + private getLLM(): ChatBedrockConverse { + if (!this.llm) { + this.llm = new ChatBedrockConverse({ + model: this.configService.get<string>('AI_MODEL_NAME') || 'us.meta.llama4-maverick-17b-instruct-v1:0', + temperature: this.configService.get<number>('AI_MODEL_TEMPERATURE') || 0.3, + maxRetries: this.configService.get<number>('AI_MODEL_MAX_RETRIES') || 4 + }) + } + return this.llm + }Then update line 60 to use
this.getLLM().invoke(messages).
1-66: Consider adding observability for production readiness.For production use, consider adding structured logging and metrics:
- Log invocation parameters (without sensitive prompt content if applicable)
- Track latency, token usage, and error rates
- Add configuration validation on startup
Example logging additions:
import { Injectable, Logger } from '@nestjs/common' @Injectable() export class BedrockService { private readonly logger = new Logger(BedrockService.name) // ... rest of implementation async generate(promptText: string, input: string): Promise<string> { if (!promptText || !input) { throw new Error('promptText and input are required') } this.logger.debug('Invoking Bedrock model') const messages = [new SystemMessage({ content: promptText }), new HumanMessage({ content: input })] try { const response = await this.llm.invoke(messages) this.logger.debug('Bedrock invocation successful') return response.text } catch (error) { this.logger.error('Bedrock invocation failed', error instanceof Error ? error.stack : String(error)) throw new Error(`AI generation failed: ${error instanceof Error ? error.message : error?.toString()}`) } } }typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock.dto.spec.ts (1)
76-93: Add missing test cases for ChatResponseDto.The
ChatResponseDtotests only cover valid data and empty string validation. Consider adding tests for:
- Missing
outputfield validation- Non-string type validation (similar to
ChatRequestDtolines 55-73)Add the following tests after line 92:
it('should fail validation when output is missing', async () => { const dto = new ChatResponseDto() // output is missing const errors = await validate(dto) expect(errors.length).toBeGreaterThan(0) expect(errors[0].property).toBe('output') }) it('should fail validation when output is not a string', async () => { const dto = new ChatResponseDto() ;(dto as any).output = 123 // Type violation const errors = await validate(dto) expect(errors.length).toBeGreaterThan(0) expect(errors[0].property).toBe('output') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
python/packages/common/poetry.lockis excluded by!**/*.lockpython/packages/fast-api-modules/authz/poetry.lockis excluded by!**/*.locktypescript/apps/admin-panel-web-spa/src/ui/DataTable/__snapshots__/DataTable.spec.tsx.snapis excluded by!**/*.snaptypescript/apps/admin-panel-web-spa/src/ui/Layout/__snapshots__/Layout.spec.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (71)
.github/workflows/test_python.yml(2 hunks).github/workflows/test_typescript.yml(2 hunks)package.json(2 hunks)typescript/apps/admin-panel-web-spa/package.json(1 hunks)typescript/apps/admin-panel-web-spa/src/feature/ProductList/ProductList.spec.tsx(2 hunks)typescript/apps/admin-panel-web-spa/src/feature/Type/Type.spec.tsx(1 hunks)typescript/apps/admin-panel-web-spa/src/feature/Type/Type.tsx(1 hunks)typescript/apps/admin-panel-web-spa/src/testUtils/types.ts(1 hunks)typescript/apps/metabase-proxy-server/package.json(1 hunks)typescript/apps/next-auth-auth0-app/next-env.d.ts(1 hunks)typescript/apps/next-auth-auth0-app/package.json(1 hunks)typescript/apps/rds-backup/package.json(1 hunks)typescript/apps/remix-firebase/app/routes/r.$topic.tsx(1 hunks)typescript/apps/remix-firebase/package.json(0 hunks)typescript/apps/server-api/package.json(1 hunks)typescript/apps/serverless-api-cqrs/package.json(1 hunks)typescript/apps/serverless-api/package.json(1 hunks)typescript/apps/serverless-scraper/package.json(1 hunks)typescript/apps/serverless-settings-service/package.json(1 hunks)typescript/apps/serverless-url-manager/package.json(1 hunks)typescript/apps/strapi-payment-spa/src/pages/SubscriptionsPage/SubscriptionsPage.tsx(1 hunks)typescript/apps/strapi/package.json(1 hunks)typescript/apps/web-spa/package.json(1 hunks)typescript/apps/web-ssr/package.json(1 hunks)typescript/modules/nestjs/package.json(1 hunks)typescript/packages/apollo-client/package.json(1 hunks)typescript/packages/common/package.json(1 hunks)typescript/packages/cypress/package.json(1 hunks)typescript/packages/feature-config/package.json(1 hunks)typescript/packages/firebase/package.json(1 hunks)typescript/packages/jest-config/package.json(1 hunks)typescript/packages/mobile-components/package.json(1 hunks)typescript/packages/nestjs-modules/auth-jwt/package.json(1 hunks)typescript/packages/nestjs-modules/authz/package.json(1 hunks)typescript/packages/nestjs-modules/bedrock/README.md(1 hunks)typescript/packages/nestjs-modules/bedrock/jest.config.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/package.json(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.module.spec.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.service.spec.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-request.dto.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-response.dto.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock.dto.spec.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/index.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/tsconfig.json(1 hunks)typescript/packages/nestjs-modules/bedrock/tsconfig.spec.json(1 hunks)typescript/packages/nestjs-modules/cache-manager/package.json(1 hunks)typescript/packages/nestjs-modules/database/package.json(1 hunks)typescript/packages/nestjs-modules/health/package.json(1 hunks)typescript/packages/nestjs-modules/logger/package.json(1 hunks)typescript/packages/nestjs-modules/sendgrid/package.json(1 hunks)typescript/packages/nestjs-modules/sentry/package.json(1 hunks)typescript/packages/nestjs-modules/settings/package.json(1 hunks)typescript/packages/nestjs-modules/strapi-roles/package.json(1 hunks)typescript/packages/nestjs-modules/stripe-payment/package.json(1 hunks)typescript/packages/nestjs-modules/stripe/package.json(1 hunks)typescript/packages/next-intl-localization/package.json(1 hunks)typescript/packages/react-google-maps/package.json(1 hunks)typescript/packages/react-google-pay/package.json(1 hunks)typescript/packages/react-localization-provider/package.json(1 hunks)typescript/packages/react-microsoft-login/package.json(1 hunks)typescript/packages/react-stripe/package.json(1 hunks)typescript/packages/s3-log-transport/package.json(1 hunks)typescript/packages/sendgrid/package.json(1 hunks)typescript/packages/strapi-plugins/permissions-manager/package.json(1 hunks)typescript/packages/web-features/package.json(1 hunks)typescript/packages/web-storages/jotai-storage/package.json(1 hunks)typescript/packages/web-storages/rtk-storage/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- typescript/apps/remix-firebase/package.json
🧰 Additional context used
🧬 Code graph analysis (9)
typescript/apps/admin-panel-web-spa/src/feature/Type/Type.spec.tsx (1)
typescript/apps/admin-panel-web-spa/src/testUtils/types.ts (1)
RouterLinkProps(3-3)
typescript/apps/admin-panel-web-spa/src/feature/ProductList/ProductList.spec.tsx (1)
typescript/apps/admin-panel-web-spa/src/testUtils/types.ts (1)
RouterLinkProps(3-3)
typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.ts (2)
typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-request.dto.ts (1)
ChatRequestDto(3-21)typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-response.dto.ts (1)
ChatResponseDto(3-12)
typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts (1)
typescript/packages/nestjs-modules/bedrock/src/index.ts (1)
BedrockService(2-2)
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.spec.ts (1)
typescript/packages/nestjs-modules/bedrock/src/index.ts (2)
BedrockModule(1-1)BedrockService(2-2)
typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts (1)
typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-request.dto.ts (1)
ChatRequestDto(3-21)
typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock.dto.spec.ts (2)
typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-request.dto.ts (1)
ChatRequestDto(3-21)typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-response.dto.ts (1)
ChatResponseDto(3-12)
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts (1)
typescript/packages/nestjs-modules/bedrock/src/index.ts (2)
BedrockService(2-2)BedrockModule(1-1)
typescript/packages/nestjs-modules/bedrock/src/bedrock.service.spec.ts (1)
typescript/packages/nestjs-modules/bedrock/src/index.ts (1)
BedrockService(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-build
- GitHub Check: test-and-build
🔇 Additional comments (29)
typescript/apps/remix-firebase/app/routes/r.$topic.tsx (1)
76-78: Multi-line map formatting looks goodThe expanded JSX improves readability without altering behavior.
typescript/apps/strapi-payment-spa/src/pages/SubscriptionsPage/SubscriptionsPage.tsx (1)
30-32: Readable JSX formatting ✅Breaking the mapped
<ProductCard>onto multiple lines improves readability without changing behavior. Looks good.typescript/apps/admin-panel-web-spa/src/feature/Type/Type.tsx (1)
176-183: Verify the necessity of the expanded dependency array.The updated dependency array
[availableViews, dispatch, gridApiRef]replaces the original[], causing the effect to re-run wheneveravailableViews,dispatch, orgridApiRefchange. This appears problematic:
gridApiRef: Ref objects fromuseGridApiRef()are stable but including them in dependency arrays can still trigger re-runs in some scenarios and is generally discouraged.availableViews: If this becomes empty after being populated (e.g., all views deleted), the effect will attempt to save the default view again, which may not be intended.dispatch: Stable from Redux, but unnecessary in the array.The original
[]dependency array ensured this initialization effect ran only once on mount, which aligns with the logic of checking if no views exist and creating a default.Consider whether this effect should remain mount-only:
- }, [availableViews, dispatch, gridApiRef]) + }, [])If the expanded dependencies are intentional, please verify that re-running the effect when
availableViewschanges doesn't cause unintended side effects (e.g., duplicate default view creation when views are cleared).typescript/apps/admin-panel-web-spa/src/feature/ProductList/ProductList.spec.tsx (1)
13-13: LGTM! Type safety improvement for test mock.Importing
RouterLinkPropsand using it to type the mockedLinkcomponent replacesanywith proper type constraints, improving test type safety.Also applies to: 23-23
typescript/apps/admin-panel-web-spa/src/testUtils/types.ts (1)
1-3: LGTM! Well-structured test utility type.This centralized
RouterLinkPropstype properly combinesAnchorHTMLAttributeswith the requiredtoprop, providing type safety for router Link mocks across test files.typescript/apps/admin-panel-web-spa/src/feature/Type/Type.spec.tsx (1)
13-13: LGTM! Type safety improvement for test mock.Importing and using
RouterLinkPropsto type the mockedLinkcomponent replacesanywith proper type constraints, maintaining consistency with other test files and improving type safety.Also applies to: 21-21
typescript/packages/s3-log-transport/package.json (1)
23-24: Verify Node 18+ support before bumping@aws-sdk/client-s3.Version 3.895.0 of
@aws-sdk/client-s3(and the current 3.88x line) sets"engines": { "node": ">=18.0.0" }, reflecting AWS’s January 6 2025 policy that the JavaScript SDK v3 no longer supports Node 16.x.(app.unpkg.com) If any of our services, build agents, or Lambda runtimes are still pinned to Node 16, this upgrade will fail to install or emitENOTSUPerrors. Please confirm that every runtime consuming@dbbs/s3-log-transportis already on Node 18+ (or bump them alongside this change) before merging.(aws.amazon.com)typescript/packages/web-storages/rtk-storage/package.json (1)
23-25: LGTM! Coordinated Redux ecosystem upgrade.The dependency updates are part of a coordinated upgrade across multiple packages in the PR. The version bumps are incremental (minor and patch updates) and align with the broader ecosystem upgrade mentioned in the PR objectives.
typescript/packages/nestjs-modules/bedrock/src/index.ts (1)
1-2: LGTM! Clean barrel exports.The module follows NestJS best practices by exporting both the module and service. The
.jsextensions are correct for ESM packages.typescript/packages/nestjs-modules/bedrock/tsconfig.spec.json (1)
1-10: LGTM! Standard test configuration.The test configuration properly extends the base TypeScript config and includes appropriate directories for test execution.
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts (1)
27-32: LGTM! Well-structured module with clear documentation.The module follows NestJS best practices: properly registers controller and provider, exports the service for consumption by other modules, and includes comprehensive documentation.
typescript/packages/nestjs-modules/health/package.json (1)
23-28: Confirmed@nestjs/common@11.1.6is published. Please review the changelog between 11.0.10 and 11.1.6 for any configuration updates or breaking changes affecting the health module.typescript/packages/nestjs-modules/bedrock/src/bedrock.module.spec.ts (1)
17-38: LGTM! Module compilation and service provision tests are well-structured.The tests correctly verify module compilation and BedrockService availability with proper mock setup.
typescript/packages/nestjs-modules/bedrock/src/bedrock.service.spec.ts (1)
34-76: LGTM! Comprehensive test coverage with proper error handling.The test suite thoroughly covers success, validation errors, and failure scenarios with appropriate mocking.
typescript/packages/nestjs-modules/bedrock/package.json (1)
1-11: LGTM! Package structure follows best practices.The package configuration correctly uses ESM with proper exports, types fields, and files specification.
typescript/apps/admin-panel-web-spa/package.json (1)
17-33: Dependency versions verified; audit for vulnerabilities.
- All specified versions exist on npm.
- Run
npm auditlocally with your lockfile to confirm no high or critical vulnerabilities.typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-response.dto.ts (1)
1-12: LGTM! Clean DTO implementation.The DTO structure is correct with appropriate validators for a required string response field. The JSDoc documentation clearly describes the field's purpose with a helpful example.
typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-request.dto.ts (1)
1-21: LGTM! Well-documented request DTO.The DTO correctly defines two required string fields with appropriate validators. The JSDoc comments provide clear context and practical examples for both the prompt text and user input fields.
typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts (1)
1-61: LGTM! Comprehensive test coverage.The test suite is well-structured with good coverage:
- Controller initialization verification
- Successful service invocation with correct parameter passing
- Error handling with appropriate HttpException
The mock setup correctly simulates the BedrockService, and both happy path and error scenarios are tested.
typescript/packages/nestjs-modules/bedrock/README.md (2)
101-103: ```bash
#!/bin/bashLocate and check for @langchain/aws in the bedrock package.json
echo "=== Locating bedrock/package.json ==="
pkg=$(find . -type f -path "*/typescript/packages/nestjs-modules/bedrock/package.json")
if [ -z "$pkg" ]; then
echo "Error: bedrock/package.json not found"
exit 1
fi
echo "Found: $pkg"
echo "=== Checking for @langchain/aws in dependencies ==="
jq '.dependencies, .peerDependencies' "$pkg" | grep -i "@langchain/aws" || echo "Dependency @langchain/aws not declared"--- `9-13`: **Package name matches installation command.** No changes required. </blockquote></details> <details> <summary>typescript/modules/nestjs/package.json (1)</summary><blockquote> `24-27`: **Verified dependency updates; no unresolved security advisories.** All specified versions exist on npm and exceed the first-patched versions for their respective advisories, and no advisories affect @nestjs/config or @compodoc/compodoc. </blockquote></details> <details> <summary>typescript/packages/nestjs-modules/authz/package.json (2)</summary><blockquote> `23-28`: **LGTM! Dependency updates look good.** The dependency version bumps are minor and patch updates that align with the PR's objective to update TypeScript packages. The NestJS updates (11.0.10 → 11.1.6) and other library updates follow semantic versioning appropriately. --- `31-36`: **LGTM! DevDependency updates are appropriate.** The devDependency updates are minor version bumps that should not introduce breaking changes. </blockquote></details> <details> <summary>typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts (2)</summary><blockquote> `52-65`: **LGTM! Error handling and input validation are solid.** The `generate` method correctly validates required inputs (lines 54-56) and wraps LLM invocation errors with context (lines 59-64). The error messages are clear and actionable. --- `29-29`: **Verify AWS Bedrock default model name** The fallback value `'us.meta.llama4-maverick-17b-instruct-v1:0'` includes a region prefix and specific version. Confirm this model ID is valid and accessible in all target AWS regions by cross-checking AWS Bedrock documentation or the AWS Console. </blockquote></details> <details> <summary>typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock.dto.spec.ts (1)</summary><blockquote> `5-74`: **LGTM! Comprehensive test coverage for ChatRequestDto.** The test suite thoroughly validates all constraints on `ChatRequestDto`: - Valid data scenario (lines 6-13) - Missing field validation (lines 15-33) - Empty string validation (lines 35-53) - Type validation with runtime checks (lines 55-73) </blockquote></details> <details> <summary>typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.ts (2)</summary><blockquote> `10-12`: **LGTM! Controller setup follows NestJS conventions.** The controller route and dependency injection are properly configured. --- `22-28`: **LGTM! Success path is clean and follows NestJS patterns.** The method correctly delegates to the service and maps the response to the DTO format. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| it('should import ConfigModule', async () => { | ||
| const createTestingModuleSpy = jest.spyOn(Test, 'createTestingModule') | ||
|
|
||
| await Test.createTestingModule({ | ||
| imports: [BedrockModule] | ||
| }) | ||
| .overrideProvider(ConfigService) | ||
| .useValue(mockConfigService) | ||
| .compile() | ||
|
|
||
| const passedModules = createTestingModuleSpy.mock.calls[0][0] | ||
| expect(passedModules.imports).toContain(BedrockModule) | ||
|
|
||
| createTestingModuleSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Test logic doesn't match the test name.
The test is named "should import ConfigModule" but it verifies that BedrockModule exists in the imports passed to Test.createTestingModule, not that BedrockModule internally imports ConfigModule. This assertion checks the test setup itself rather than BedrockModule's internal composition.
To properly verify that BedrockModule imports ConfigModule, consider either:
- Removing this test if ConfigModule import is an internal implementation detail, or
- Refactoring to inspect the module's actual imports metadata using NestJS reflection APIs
- it('should import ConfigModule', async () => {
- const createTestingModuleSpy = jest.spyOn(Test, 'createTestingModule')
-
- await Test.createTestingModule({
- imports: [BedrockModule]
- })
- .overrideProvider(ConfigService)
- .useValue(mockConfigService)
- .compile()
-
- const passedModules = createTestingModuleSpy.mock.calls[0][0]
- expect(passedModules.imports).toContain(BedrockModule)
-
- createTestingModuleSpy.mockRestore()
- })📝 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.
| it('should import ConfigModule', async () => { | |
| const createTestingModuleSpy = jest.spyOn(Test, 'createTestingModule') | |
| await Test.createTestingModule({ | |
| imports: [BedrockModule] | |
| }) | |
| .overrideProvider(ConfigService) | |
| .useValue(mockConfigService) | |
| .compile() | |
| const passedModules = createTestingModuleSpy.mock.calls[0][0] | |
| expect(passedModules.imports).toContain(BedrockModule) | |
| createTestingModuleSpy.mockRestore() | |
| }) |
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test_python.yml(2 hunks)sonar-project.properties(0 hunks)sonar.python.properties(1 hunks)sonar.ts.properties(1 hunks)
💤 Files with no reviewable changes (1)
- sonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test_python.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-build
- GitHub Check: test-and-build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/test_python.yml (1)
102-111: Linux sed fix is correct; make replacement robust and drop stale comment.Good switch to GNU
sed -i. To avoid hard‑coded paths and handle wildcard fallbacks reliably, prefer finding reports and replacing${GITHUB_WORKSPACE}with the container mount path. Also remove the now‑stale macOS note.Apply this diff:
- - name: Fix coverage.xml source paths - if: ${{ env.AFFECTED_PATHS != '' }} - run: | - for dir in $AFFECTED_PATHS; do - if [ -f "$dir/coverage.xml" ]; then - sed -i 's#<source>/home/runner/work/dbbs-solutions/dbbs-solutions#<source>/github/workspace#g' "$dir/coverage.xml" - fi - done - # change sed -i '' to sed -i if you are using linux runner + - name: Fix coverage.xml source paths + if: ${{ env.AFFECTED_PATHS != '' }} + shell: bash + run: | + # Replace host workspace prefix with the scanner's container mount path. + # Example: /home/runner/work/<repo>/<repo> -> /github/workspace + mapfile -t reports < <(find $AFFECTED_PATHS -type f -name coverage.xml 2>/dev/null || true) + for report in "${reports[@]}"; do + sed -i "s#<source>${GITHUB_WORKSPACE}#<source>/github/workspace#g" "$report" + doneIf you still need macOS support in other runners, gate the
sed -iflavor by OS in a separate step.
🧹 Nitpick comments (1)
.github/workflows/test_python.yml (1)
112-114: Guard the Sonar properties rename to avoid brittle failures.Use a guarded copy; fail explicitly if the source is missing, rather than an unguarded mv.
- - name: Setup sonarcloud - run: mv sonar.python.properties sonar-project.properties + - name: Setup sonarcloud + shell: bash + run: | + if [ -f sonar.python.properties ]; then + cp -f sonar.python.properties sonar-project.properties + else + echo "sonar.python.properties not found" >&2 + exit 1 + fiAlso verify the scan step uses the correct env var (
SONAR_HOST_URL) for the action version you’re on.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test_python.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-build
- GitHub Check: test-and-build
🔇 Additional comments (1)
.github/workflows/test_python.yml (1)
22-22: LGTM on PR path filter for Python sonar config.Including sonar.python.properties ensures CI runs when Python Sonar settings change. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts (3)
29-29: ConfigModule documentation still needed.The past review comment regarding ConfigModule setup and AWS environment variables documentation remains valid and should be addressed.
29-29: Past review comment still applies: Document ConfigModule setup and required AWS environment variables.The previous reviewer's concern about documenting ConfigModule setup and AWS environment variables remains valid. The README should instruct consumers to import
ConfigModule.forRoot()in their root AppModule and document all required environment variables including AWS credentials and Bedrock-specific configuration.Based on learnings
29-29: ConfigModule documentation concern already raised.The past review comment correctly identified that the README should document ConfigModule.forRoot() setup requirements and list required environment variables (AWS_DEFAULT_REGION, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AI_MODEL_NAME, etc.).
🧹 Nitpick comments (9)
typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts (2)
12-29: Consider usingbeforeEachfor better test isolation.While the current
beforeAllapproach works, usingbeforeEachis a safer pattern that prevents potential test pollution and makes each test truly independent. This is especially important if the test suite grows.Apply this diff:
- beforeAll(async () => { + beforeEach(async () => { const mockAIService = { generate: jest.fn() }
12-29: Consider usingbeforeEachinstead ofbeforeAllfor better test isolation.While the current implementation works because each test uses
jest.spyOnwith fresh mock implementations, usingbeforeEachis a safer pattern that:
- Ensures complete isolation between tests
- Prevents potential call count accumulation
- Makes tests more resilient to future changes
Apply this diff:
- beforeAll(async () => { + beforeEach(async () => { const mockAIService = { generate: jest.fn() }typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts (5)
50-55: Consider validating non-empty AI responses.The service returns
response.textwithout checking if it's empty or undefined. While this may be rare with properly configured models, adding a validation ensures consistent behavior.Apply this diff:
try { const response = await this.llm.invoke(messages) - return response.text + if (!response.text) { + throw new Error('AI service returned an empty response') + } + return response.text } catch (error) { throw new Error(`AI generation failed: ${error instanceof Error ? error.message : error?.toString()}`) }
50-55: Preserve error stack traces in error handling.The current error wrapping creates a new Error instance, losing the original stack trace. This makes debugging LLM failures more difficult, especially when troubleshooting AWS Bedrock issues.
Consider preserving the original error as the cause:
try { const response = await this.llm.invoke(messages) return response.text } catch (error) { - throw new Error(`AI generation failed: ${error instanceof Error ? error.message : error?.toString()}`) + const message = `AI generation failed: ${error instanceof Error ? error.message : String(error)}` + if (error instanceof Error) { + throw new Error(message, { cause: error }) + } + throw new Error(message) }
51-51: Consider adding timeout handling for LLM invocation.The
llm.invokecall has no explicit timeout, relying solely on AWS SDK defaults and the configuredmaxRetries. Long-running requests could tie up application resources or lead to poor user experience in HTTP contexts.While the BedrockController wraps this in HTTP error handling, consider adding an explicit timeout for better control:
import { setTimeout } from 'timers/promises' // In generate method const timeoutMs = 30000 // 30 seconds const response = await Promise.race([ this.llm.invoke(messages), setTimeout(timeoutMs).then(() => { throw new Error(`LLM invocation timed out after ${timeoutMs}ms`) }) ])Alternatively, document the expected behavior and ensure consuming applications implement appropriate timeouts at their level.
43-47: Consider more explicit validation for empty strings.The current truthiness check catches most cases but could be more robust:
async generate(promptText: string, input: string): Promise<string> { // Additional validation to ensure the service is robust and can handle non-HTTP use cases - if (!promptText || !input) { + if (!promptText?.trim() || !input?.trim()) { throw new Error('promptText and input are required') }This ensures strings with only whitespace are also rejected.
53-55: Consider preserving original error for better debugging.The current error wrapping loses the original stack trace. Consider preserving error context:
} catch (error) { - throw new Error(`AI generation failed: ${error instanceof Error ? error.message : error?.toString()}`) + const message = error instanceof Error ? error.message : error?.toString(); + throw new Error(`AI generation failed: ${message}`, { cause: error }) } }The
causeoption (Node.js 16.9+) preserves the original error for debugging while still providing a clear message.typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts (2)
36-41: Consider adding error handling for ChatBedrockConverse instantiation.The factory creates a
ChatBedrockConverseinstance without error handling. If the AWS SDK cannot initialize (e.g., missing credentials, invalid region, network issues), this will fail silently at module initialization, making debugging difficult.Consider wrapping the instantiation in a try-catch to provide clearer error messages:
useFactory: (config: ConfigService) => { + try { new ChatBedrockConverse({ model: config.get<string>('AI_MODEL_NAME') || 'us.meta.llama4-maverick-17b-instruct-v1:0', temperature: config.get<number>('AI_MODEL_TEMPERATURE') || 0.3, maxRetries: config.get<number>('AI_MODEL_MAX_RETRIES') || 4 }) + } catch (error) { + throw new Error(`Failed to initialize Bedrock LLM: ${error instanceof Error ? error.message : String(error)}`) + } }
36-41: Consider validating configuration values for production.The factory provides sensible defaults but doesn't validate that custom values are within acceptable ranges. For production deployments, consider adding validation:
temperatureshould be between 0 and 1maxRetriesshould be a positive integermodelshould follow AWS Bedrock model ID formatThis could prevent runtime errors from misconfigured environment variables.
Example validation:
useFactory: (config: ConfigService) => { + const temperature = config.get<number>('AI_MODEL_TEMPERATURE') || 0.3; + const maxRetries = config.get<number>('AI_MODEL_MAX_RETRIES') || 4; + + if (temperature < 0 || temperature > 1) { + throw new Error('AI_MODEL_TEMPERATURE must be between 0 and 1'); + } + if (maxRetries < 0 || !Number.isInteger(maxRetries)) { + throw new Error('AI_MODEL_MAX_RETRIES must be a positive integer'); + } + return new ChatBedrockConverse({ model: config.get<string>('AI_MODEL_NAME') || 'us.meta.llama4-maverick-17b-instruct-v1:0', - temperature: config.get<number>('AI_MODEL_TEMPERATURE') || 0.3, - maxRetries: config.get<number>('AI_MODEL_MAX_RETRIES') || 4 + temperature, + maxRetries }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
sonar.ts.properties(1 hunks)typescript/apps/serverless-api-cqrs/package.json(1 hunks)typescript/apps/serverless-api/package.json(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.service.spec.ts(1 hunks)typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- typescript/apps/serverless-api-cqrs/package.json
- typescript/apps/serverless-api/package.json
- typescript/packages/nestjs-modules/bedrock/src/bedrock.service.spec.ts
🧰 Additional context used
🧬 Code graph analysis (3)
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts (1)
typescript/packages/nestjs-modules/bedrock/src/index.ts (2)
BedrockService(2-2)BedrockModule(1-1)
typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts (1)
typescript/packages/nestjs-modules/bedrock/src/index.ts (1)
BedrockService(2-2)
typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts (1)
typescript/packages/nestjs-modules/bedrock/src/dtos/bedrock-request.dto.ts (1)
ChatRequestDto(3-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-build
- GitHub Check: test-and-build
🔇 Additional comments (11)
typescript/packages/nestjs-modules/bedrock/src/bedrock.module.ts (2)
7-27: LGTM! Comprehensive module documentation.The JSDoc provides clear examples and references to related components, making it easy for developers to understand how to use the module.
44-44: LGTM!Correctly exports only
BedrockService, allowing consuming modules to inject the service while keeping controller and LLM provider as internal implementation details.typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts (4)
35-47: LGTM! Well-structured success case test.The test properly verifies both the service method call and the response structure.
49-59: LGTM! Error handling test is comprehensive.The test correctly validates that service failures are properly caught and transformed into appropriate HTTP exceptions.
35-47: LGTM!The success test properly verifies:
- Service method is called with correct parameters
- Response is correctly shaped as
{ output: string }
49-59: LGTM!The error test correctly verifies that service failures are caught and transformed into an appropriate HTTP exception with status 500 and a user-friendly message.
typescript/packages/nestjs-modules/bedrock/src/bedrock.service.ts (5)
5-16: LGTM! Clear and helpful service documentation.The JSDoc provides a practical example that demonstrates how to use the service.
17-24: LGTM! Proper dependency injection setup.The service correctly injects the BEDROCK_LLM token configured in the module.
44-47: Defensive validation is appropriate for service reusability.The comment correctly identifies this as defensive programming for non-HTTP use cases. This is a good practice as it makes the service more robust and reusable beyond just the controller context.
17-24: LGTM!The service correctly injects the
ChatBedrockConverseLLM using theBEDROCK_LLMtoken and follows standard NestJS DI patterns.
49-52: LGTM!Correct use of LangChain message types:
SystemMessagefor AI context/instructionsHumanMessagefor user input- Proper invocation and text extraction
typescript/packages/nestjs-modules/bedrock/src/bedrock.controller.spec.ts
Show resolved
Hide resolved
|
|
List of changes
Checklist:
Summary by CodeRabbit