fix: missing qbd direct environment variable breaking compilation #1047
Conversation
WalkthroughThe changes in this pull request involve modifications to two files. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 0
🧹 Outside diff range and nitpick comments (2)
src/app/core/services/common/helper.service.ts (2)
55-55: The fix addresses the compilation issue but could be improved.While the change prevents the compilation error by checking for the existence of
qbd_direct_api_url, there are a few potential improvements:
- An empty string as fallback might cause silent failures in API calls
- The type assertion to string is redundant since the environment variable would already be typed
Consider this alternative implementation:
- [AppUrl.QBD_DIRECT]: ('qbd_direct_api_url' in environment) ? environment.qbd_direct_api_url as string : "", + [AppUrl.QBD_DIRECT]: environment.qbd_direct_api_url || environment.cluster_domain_api_url + '/qbd-direct-api/api',This approach:
- Uses a more meaningful fallback URL pattern consistent with other integrations
- Removes unnecessary type assertion
- Maintains compatibility with both production and development environments
Line range hint
52-62: Consider standardizing environment variable handling across all integrations.The current implementation has inconsistent error handling across different integration URLs. Some use direct environment variables, others use template literals with conditionals, and now QBD_DIRECT has its own existence check.
Consider:
- Implementing a consistent pattern for all URLs
- Adding environment variable validation at startup
- Creating a centralized configuration service that handles environment variables with proper typing and validation
Example configuration service:
interface ApiUrlConfig { production: boolean; cluster_domain_api_url: string; qbd_direct_api_url?: string; // ... other URLs } @Injectable({ providedIn: 'root' }) export class ConfigService { private config: ApiUrlConfig; constructor() { this.validateConfig(environment); this.config = environment; } private validateConfig(config: any): void { const requiredKeys = ['production', 'cluster_domain_api_url']; for (const key of requiredKeys) { if (!(key in config)) { throw new Error(`Missing required configuration key: ${key}`); } } } getApiUrl(app: AppUrl): string { // Implement consistent URL resolution logic } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app/core/services/common/helper.service.ts (1 hunks)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts
Clickup
app.clickup.com
Summary by CodeRabbit
Bug Fixes
QBDExportSettingFormOptionin the QBD Direct Export Settings component for improved consistency.Chores