Skip hosted-database check in self-hosted mode#1726
Conversation
Replace per-navigation `/saas/hosted-database/:companyId` fetch in ConnectionsService with a local hostname suffix check against `.db.rocketadmin.com`, and gate it behind `environment.saas` so self-hosted builds no longer hit a SaaS-only endpoint on every page. Also apply pnpm audit overrides for ajv, picomatch, path-to-regexp, lodash, and @nestjs/core. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaces async hosted-database hostname caching and service-based checks with a synchronous computed getter that uses environment.saas and hostname pattern matching; removes HostedDatabaseService and UserService from the service and its tests; expands pnpm.overrides in package.json. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR removes a per-navigation SaaS-only hosted-database lookup from the Angular ConnectionsService by switching to a local hostname-based check gated by environment.saas, preventing self-hosted builds from calling /saas/hosted-database/:companyId on every page. It also applies pnpm audit overrides to bump several transitive dependencies.
Changes:
- Replace hosted-database API fetch/caching in
ConnectionsServicewith a local.db.rocketadmin.comsuffix check gated byenvironment.saas. - Simplify
ConnectionsServiceunit test setup by removing now-unused SaaS service fakes/providers. - Add pnpm overrides (and lockfile updates) for ajv, picomatch, path-to-regexp, lodash, and
@nestjs/core.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pnpm-lock.yaml | Applies security-related override resolutions and updates resolved versions in the lockfile. |
| package.json | Adds pnpm audit overrides for several packages (and keeps existing overrides). |
| frontend/src/app/services/connections.service.ts | Removes SaaS hosted-database network lookup and replaces it with an env-gated hostname suffix check. |
| frontend/src/app/services/connections.service.spec.ts | Removes HostedDatabase/User service test fakes that are no longer needed by ConnectionsService. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Silently fail - non-hosted path will be used | ||
| } | ||
| private checkIfHostedConnection(connectionHost: string) { | ||
| this.isHostedConnection = !!environment.saas && !!connectionHost && connectionHost.endsWith('.db.rocketadmin.com'); |
There was a problem hiding this comment.
The hosted-DB detection misses the case where the hostname is exactly db.rocketadmin.com (no leading subdomain). The repo’s hosted DB test fixtures use db.rocketadmin.com as the hostname (e.g. hosted-database.service.spec.ts / own-connections.component.spec.ts), but endsWith('.db.rocketadmin.com') will return false for that value, leaving isHostedConnection unset and routing users to the wrong page. Consider accepting both db.rocketadmin.com and *.db.rocketadmin.com (or parsing the hostname and matching a normalized suffix/equality).
| this.isHostedConnection = !!environment.saas && !!connectionHost && connectionHost.endsWith('.db.rocketadmin.com'); | |
| const normalizedHost = connectionHost?.trim().toLowerCase(); | |
| this.isHostedConnection = | |
| !!environment.saas && | |
| !!normalizedHost && | |
| (normalizedHost === 'db.rocketadmin.com' || normalizedHost.endsWith('.db.rocketadmin.com')); |
| // Silently fail - non-hosted path will be used | ||
| } | ||
| private checkIfHostedConnection(connectionHost: string) { | ||
| this.isHostedConnection = !!environment.saas && !!connectionHost && connectionHost.endsWith('.db.rocketadmin.com'); |
There was a problem hiding this comment.
This change materially alters how isHostedConnection is determined (network lookup removed, now based on environment.saas + hostname suffix), but there are no unit tests exercising the new detection behavior. Since this service already has extensive test coverage, please add tests covering both hosted and non-hosted hosts (including the db.rocketadmin.com exact-host case and a *.db.rocketadmin.com subdomain case) so regressions don’t silently break navigation.
| this.isHostedConnection = !!environment.saas && !!connectionHost && connectionHost.endsWith('.db.rocketadmin.com'); | |
| this.isHostedConnection = | |
| !!environment.saas && | |
| !!connectionHost && | |
| (connectionHost === 'db.rocketadmin.com' || connectionHost.endsWith('.db.rocketadmin.com')); |
| "ajv@>=7.0.0-alpha.0 <8.18.0": ">=8.18.0", | ||
| "picomatch@>=4.0.0 <4.0.4": ">=4.0.4", | ||
| "path-to-regexp@>=8.0.0 <8.4.0": ">=8.4.0", | ||
| "lodash@>=4.0.0 <=4.17.23": ">=4.18.0", |
There was a problem hiding this comment.
The lodash override is duplicated: lodash@<=4.17.23 already covers the lodash@>=4.0.0 <=4.17.23 selector, so one of these entries is redundant and makes the override list harder to maintain. Remove the duplicate selector to keep overrides minimal and unambiguous.
| "lodash@>=4.0.0 <=4.17.23": ">=4.18.0", |
| "path-to-regexp@>=8.0.0 <8.4.0": ">=8.4.0", | ||
| "lodash@>=4.0.0 <=4.17.23": ">=4.18.0", | ||
| "lodash@<=4.17.23": ">=4.18.0", | ||
| "@nestjs/core@<=11.1.17": ">=11.1.18" |
There was a problem hiding this comment.
Several new audit overrides use open-ended ranges (e.g. ">=8.18.0", ">=4.18.0", ">=11.1.18"). Unlike the existing pinned overrides (cipher-base, tar-fs), these ranges can unexpectedly float to newer major versions on the next lockfile refresh (or in consumers that don’t use the lockfile), potentially introducing breaking changes. Prefer pinning to a specific patched version or at least constraining to the intended major line (e.g. ^8.18.0, ^11.1.18).
| "@nestjs/core@<=11.1.17": ">=11.1.18" | |
| "@nestjs/core@<=11.1.17": "11.1.18" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
4-4:⚠️ Potential issue | 🔴 CriticalRemove
version: 10from the pnpm/action-setup step in.github/workflows/code-quality.yml(line 20).The
ERR_PNPM_BAD_PM_VERSIONerror occurs because bothversion: 10in the workflow andpackageManager: pnpm@10.33.0inpackage.jsonare specified. The pnpm/action-setup@v4 action expects a single source of truth. Remove theversioninput to let the action use thepackageManagerfield instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 4, The workflow is supplying a conflicting pnpm version; remove the `version` input from the `pnpm/action-setup@v4` step so the action will use the `packageManager` field in package.json (`"packageManager": "pnpm@10.33.0"`) as the single source of truth; locate the `pnpm/action-setup@v4` step in your code-quality workflow and delete the `version: 10` line (leaving other step config intact).
🧹 Nitpick comments (1)
frontend/src/app/services/connections.service.ts (1)
178-239: Stale debugconsole.logleft in a hot path.Not introduced by this PR, but while you're simplifying
setConnectionInfotheconsole.log('this.defaultDisplayTable')/console.log(this.defaultDisplayTable)pair at lines 237–238 fires on everyNavigationEndand leaks the default table name to the browser console in production. Good opportunity to drop them alongside the other cleanup in this change.Proposed cleanup
- console.log('this.defaultDisplayTable'); - console.log(this.defaultDisplayTable); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/connections.service.ts` around lines 178 - 239, Remove the two debug console.log calls at the end of setConnectionInfo (the console.log('this.defaultDisplayTable') and console.log(this.defaultDisplayTable)) — they run on every NavigationEnd and should not be in production; simply delete these lines from the setConnectionInfo method (or replace with an appropriate logger guarded by an environment check if you need runtime debug logging).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/app/services/connections.service.ts`:
- Around line 610-612: Rename the private method checkIfHostedConnection to
_checkIfHostedConnection and update all call sites (e.g., change
this.checkIfHostedConnection(...) to this._checkIfHostedConnection(...)); inside
the method, normalize the hostname to lowercase before performing the suffix
check (use connectionHost?.toLowerCase() and then
endsWith('.db.rocketadmin.com')) so the DNS comparison is case-insensitive, and
keep the existing environment.saas guard and boolean assignment to
this.isHostedConnection.
In `@package.json`:
- Around line 29-30: Remove the redundant override entry for "lodash@>=4.0.0
<=4.17.23" and keep the broader "lodash@<=4.17.23": ">=4.18.0" rule in
package.json so only one override remains; after making that change, run
dependency resolution (e.g., npm/yarn install or audit) and verify that forcing
lodash >=4.18.0 is safe for all transitive consumers (inspect callers of
_.unset/_.omit and run tests) to avoid behavioral regressions.
---
Outside diff comments:
In `@package.json`:
- Line 4: The workflow is supplying a conflicting pnpm version; remove the
`version` input from the `pnpm/action-setup@v4` step so the action will use the
`packageManager` field in package.json (`"packageManager": "pnpm@10.33.0"`) as
the single source of truth; locate the `pnpm/action-setup@v4` step in your
code-quality workflow and delete the `version: 10` line (leaving other step
config intact).
---
Nitpick comments:
In `@frontend/src/app/services/connections.service.ts`:
- Around line 178-239: Remove the two debug console.log calls at the end of
setConnectionInfo (the console.log('this.defaultDisplayTable') and
console.log(this.defaultDisplayTable)) — they run on every NavigationEnd and
should not be in production; simply delete these lines from the
setConnectionInfo method (or replace with an appropriate logger guarded by an
environment check if you need runtime debug logging).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 175385e5-0dd3-4dd8-aaf0-dcfc29e10075
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
frontend/src/app/services/connections.service.spec.tsfrontend/src/app/services/connections.service.tspackage.json
💤 Files with no reviewable changes (1)
- frontend/src/app/services/connections.service.spec.ts
| private checkIfHostedConnection(connectionHost: string) { | ||
| this.isHostedConnection = !!environment.saas && !!connectionHost && connectionHost.endsWith('.db.rocketadmin.com'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -g 'environment*.ts' frontend/src/environments | xargs rg -nP -C2 '\bsaas\b'Repository: rocket-admin/rocketadmin
Length of output: 1463
🏁 Script executed:
# Get the method declaration and surrounding context at lines 610-612
sed -n '605,620p' frontend/src/app/services/connections.service.ts
# Find all occurrences of the method name to identify call sites
rg -n 'checkIfHostedConnection' frontend/src/app/services/connections.service.tsRepository: rocket-admin/rocketadmin
Length of output: 405
Normalize the hostname before the suffix check and rename method per convention.
endsWith is case-sensitive but DNS hostnames are not — a connection stored with host Example.DB.RocketAdmin.com (or any mixed-case variant returned by the backend / entered by a user) will evaluate to false here and the UI will incorrectly treat a hosted DB as non-hosted. Additionally, per coding guidelines, rename the private method to use the underscore prefix convention.
Proposed fix
- private checkIfHostedConnection(connectionHost: string) {
- this.isHostedConnection = !!environment.saas && !!connectionHost && connectionHost.endsWith('.db.rocketadmin.com');
- }
+ private _checkIfHostedConnection(connectionHost: string) {
+ this.isHostedConnection =
+ !!environment.saas &&
+ !!connectionHost &&
+ connectionHost.toLowerCase().endsWith('.db.rocketadmin.com');
+ }Update the call site at line 218 from this.checkIfHostedConnection(...) to this._checkIfHostedConnection(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/services/connections.service.ts` around lines 610 - 612,
Rename the private method checkIfHostedConnection to _checkIfHostedConnection
and update all call sites (e.g., change this.checkIfHostedConnection(...) to
this._checkIfHostedConnection(...)); inside the method, normalize the hostname
to lowercase before performing the suffix check (use
connectionHost?.toLowerCase() and then endsWith('.db.rocketadmin.com')) so the
DNS comparison is case-insensitive, and keep the existing environment.saas guard
and boolean assignment to this.isHostedConnection.
| "lodash@>=4.0.0 <=4.17.23": ">=4.18.0", | ||
| "lodash@<=4.17.23": ">=4.18.0", |
There was a problem hiding this comment.
Redundant lodash override — second range subsumes the first.
lodash@<=4.17.23 already covers every version matched by lodash@>=4.0.0 <=4.17.23, so one of the two entries is dead. Keep just the broader rule:
Proposed cleanup
- "lodash@>=4.0.0 <=4.17.23": ">=4.18.0",
"lodash@<=4.17.23": ">=4.18.0",Also worth double-checking that forcing lodash >=4.18.0 is safe for every transitive consumer — 4.18.0 tightened prototype-pollution guards in _.unset/_.omit so calls that previously returned true now return false, which can break code relying on the old behavior.
📝 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.
| "lodash@>=4.0.0 <=4.17.23": ">=4.18.0", | |
| "lodash@<=4.17.23": ">=4.18.0", | |
| "lodash@<=4.17.23": ">=4.18.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 29 - 30, Remove the redundant override entry for
"lodash@>=4.0.0 <=4.17.23" and keep the broader "lodash@<=4.17.23": ">=4.18.0"
rule in package.json so only one override remains; after making that change, run
dependency resolution (e.g., npm/yarn install or audit) and verify that forcing
lodash >=4.18.0 is safe for all transitive consumers (inspect callers of
_.unset/_.omit and run tests) to avoid behavioral regressions.
Replace the mutable `isHostedConnection` field + per-navigation setter with a getter derived from `this.connection.host`, so the state has a single source of truth and no longer needs manual reset/recompute in `setConnectionInfo`. Also drop leftover `console.log` debug lines that fired on every navigation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/app/services/connections.service.ts (1)
143-146:⚠️ Potential issue | 🟡 MinorNormalize the hostname before matching.
endsWithis case-sensitive, but DNS hostnames are not. A stored host likeExample.DB.RocketAdmin.comwould be treated as non-hosted.Proposed fix
get isHostedConnection(): boolean { - const host = this.connection?.host; + const host = this.connection?.host?.toLowerCase(); return !!environment.saas && !!host && host.endsWith('.db.rocketadmin.com'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/services/connections.service.ts` around lines 143 - 146, Normalize the hostname to lowercase before checking suffix in the isHostedConnection getter: when reading this.connection?.host, convert it (e.g., toLowerCase()) and then test endsWith('.db.rocketadmin.com') while still honoring environment.saas; update the isHostedConnection accessor to use the normalized host value to ensure case-insensitive matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/app/services/connections.service.ts`:
- Around line 143-146: Normalize the hostname to lowercase before checking
suffix in the isHostedConnection getter: when reading this.connection?.host,
convert it (e.g., toLowerCase()) and then test endsWith('.db.rocketadmin.com')
while still honoring environment.saas; update the isHostedConnection accessor to
use the normalized host value to ensure case-insensitive matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92ea8eee-ed77-4914-8a5d-90a6ae626ced
📒 Files selected for processing (1)
frontend/src/app/services/connections.service.ts
Replace per-navigation
/saas/hosted-database/:companyIdfetch in ConnectionsService with a local hostname suffix check against.db.rocketadmin.com, and gate it behindenvironment.saasso self-hosted builds no longer hit a SaaS-only endpoint on every page.Also apply pnpm audit overrides for ajv, picomatch, path-to-regexp, lodash, and @nestjs/core.
Summary by CodeRabbit
Refactor
Chores
Tests