feat(nocodb): toml configuration, validation, cli flags #11444
Conversation
📝 Walkthrough""" WalkthroughThis change centralizes all configuration management for the NocoDB server by introducing a structured configuration module. All usages of environment variables throughout the codebase are replaced with accesses to a single Changes
Sequence Diagram(s)sequenceDiagram
participant CLI/Env/TOML as Config Sources
participant ConfigModule as config/index.ts
participant App as Application Code
CLI/Env/TOML->>ConfigModule: Provide config values (defaults, env, TOML, CLI)
ConfigModule->>ConfigModule: Merge config with precedence (TOML < env < CLI < defaults)
ConfigModule->>ConfigModule: Validate merged config (class-validator)
ConfigModule->>App: Export validated serverConfig
App->>ConfigModule: Import serverConfig
App->>App: Use serverConfig for all configuration needs
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ 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 (
|
4e7a301 to
28c6ea9
Compare
…LED->NC_CONNECT_TO_EXTERNAL_DB
28c6ea9 to
3c5d564
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
packages/nocodb/src/helpers/initBaseBehaviour.ts (1)
69-84: 🛠️ Refactor suggestionConsider avoiding direct mutation of shared configuration object.
The code directly mutates
serverConfig.nocoDbConfig.dataReflectionwhich could have unintended side effects across the application. Consider these alternatives:
- Use a separate runtime state manager for dynamic configuration changes
- Emit events to notify other parts of the system about configuration changes
- Return the determined value instead of mutating global state
- serverConfig.nocoDbConfig.dataReflection = false + // Consider using a runtime state manager or event system instead + // runtimeConfig.setDataReflection(false)Direct mutation of shared configuration objects can make the application state unpredictable and harder to debug.
♻️ Duplicate comments (2)
packages/nocodb/src/app.module.ts (1)
1-1: Same import path verification needed.Same as previous file - verify the
'config'import resolves correctly.packages/nocodb/src/decorators/use-worker.decorator.ts (1)
1-1: Same import path verification needed.Same as previous files - verify the
'config'import resolves correctly.
🧹 Nitpick comments (2)
packages/nocodb/src/helpers/initBaseBehaviour.ts (1)
53-55: Consider using strict equality for type safety.The condition uses loose equality (
==) which allows type coercion. IfserverConfig.nocoDbConfig.dataReflectionis always a boolean, consider using strict equality for better type safety:- if (serverConfig.nocoDbConfig.dataReflection == false) { + if (serverConfig.nocoDbConfig.dataReflection === false) {packages/nocodb/config/util.ts (1)
17-29: Use strict equality for consistency.The loose equality check
!= undefinedshould be strict equality!== undefinedfor consistency with other undefined checks in this file.- } else if (obj[key] != undefined) { + } else if (obj[key] !== undefined) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/nocodb/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (49)
nix/package.nix(1 hunks)packages/nocodb/config/index.ts(1 hunks)packages/nocodb/config/model.ts(1 hunks)packages/nocodb/config/util.ts(1 hunks)packages/nocodb/src/Noco.ts(7 hunks)packages/nocodb/src/app.config.ts(2 hunks)packages/nocodb/src/app.module.ts(3 hunks)packages/nocodb/src/cache/RedisCacheMgr.ts(2 hunks)packages/nocodb/src/controllers/api-docs/api-docs.controller.ts(5 hunks)packages/nocodb/src/controllers/plugins.controller.ts(2 hunks)packages/nocodb/src/controllers/utils.controller.ts(2 hunks)packages/nocodb/src/db/sql-data-mapper/__tests__/models/index.js(1 hunks)packages/nocodb/src/decorators/use-worker.decorator.ts(2 hunks)packages/nocodb/src/gateways/socket.gateway.ts(1 hunks)packages/nocodb/src/helpers/NcPluginMgrv2.ts(3 hunks)packages/nocodb/src/helpers/dataReflectionHelpers.ts(1 hunks)packages/nocodb/src/helpers/initBaseBehaviour.ts(4 hunks)packages/nocodb/src/main.ts(2 hunks)packages/nocodb/src/middlewares/global/global.middleware.ts(1 hunks)packages/nocodb/src/modules/auth/auth.controller.ts(2 hunks)packages/nocodb/src/modules/auth/auth.module.ts(2 hunks)packages/nocodb/src/modules/jobs/jobs.module.ts(2 hunks)packages/nocodb/src/modules/jobs/jobs/at-import/at-import.controller.ts(2 hunks)packages/nocodb/src/modules/jobs/redis/jobs-redis.ts(2 hunks)packages/nocodb/src/modules/jobs/redis/jobs.service.ts(3 hunks)packages/nocodb/src/modules/noco.module.ts(2 hunks)packages/nocodb/src/nocobuild.ts(2 hunks)packages/nocodb/src/providers/init-meta-service.provider.ts(4 hunks)packages/nocodb/src/run/cloud.ts(3 hunks)packages/nocodb/src/run/docker.ts(2 hunks)packages/nocodb/src/run/dockerEntry.ts(2 hunks)packages/nocodb/src/run/dockerRunMysql.ts(2 hunks)packages/nocodb/src/run/dockerRunPG.ts(2 hunks)packages/nocodb/src/run/dockerRunPG_CyQuick.ts(2 hunks)packages/nocodb/src/run/local.ts(2 hunks)packages/nocodb/src/run/testDocker.ts(6 hunks)packages/nocodb/src/run/timely.ts(1 hunks)packages/nocodb/src/services/bases.service.ts(5 hunks)packages/nocodb/src/services/hooks.service.ts(2 hunks)packages/nocodb/src/services/shared-bases.service.ts(2 hunks)packages/nocodb/src/services/users/users.service.ts(3 hunks)packages/nocodb/src/services/utils.service.ts(3 hunks)packages/nocodb/src/strategies/google.strategy/google.strategy.ts(3 hunks)packages/nocodb/src/utils/appUrl.ts(1 hunks)packages/nocodb/src/utils/cloud/populateCloudPlugins.ts(3 hunks)packages/nocodb/src/utils/nc-config/NcConfig.ts(2 hunks)packages/nocodb/src/utils/tele.ts(5 hunks)packages/nocodb/src/version-upgrader/NcUpgrader.ts(2 hunks)packages/nocodb/src/version-upgrader/upgraders/0225002_ncDatasourceDecrypt.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (38)
packages/nocodb/src/services/hooks.service.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/helpers/dataReflectionHelpers.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/main.ts (2)
packages/nocodb/src/Noco.ts (2)
httpServer(165-167)server(169-171)packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/run/dockerRunPG.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/run/dockerEntry.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/app.config.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/db/sql-data-mapper/__tests__/models/index.js (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/auth/auth.controller.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/run/timely.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/services/shared-bases.service.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/app.module.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/nocobuild.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/decorators/use-worker.decorator.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/controllers/api-docs/api-docs.controller.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/utils/appUrl.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/gateways/socket.gateway.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/version-upgrader/NcUpgrader.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/utils/cloud/populateCloudPlugins.ts (2)
packages/nocodb/src/Noco.ts (1)
Noco(32-234)packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/jobs/jobs/at-import/at-import.controller.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/controllers/utils.controller.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/version-upgrader/upgraders/0225002_ncDatasourceDecrypt.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/services/utils.service.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/auth/auth.module.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/providers/init-meta-service.provider.ts (3)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)packages/nocodb/src/utils/cloud/populateCloudPlugins.ts (1)
populatePluginsForCloud(7-97)packages/nocodb/src/Noco.ts (1)
Noco(32-234)
packages/nocodb/src/helpers/initBaseBehaviour.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/jobs/jobs.module.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/services/users/users.service.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/noco.module.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/run/dockerRunPG_CyQuick.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/utils/tele.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/services/bases.service.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/jobs/redis/jobs-redis.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/modules/jobs/redis/jobs.service.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/strategies/google.strategy/google.strategy.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/Noco.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/utils/nc-config/NcConfig.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/cache/RedisCacheMgr.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
packages/nocodb/src/helpers/NcPluginMgrv2.ts (1)
packages/nocodb/config/index.ts (1)
serverConfig(184-184)
🪛 Biome (1.9.4)
packages/nocodb/src/run/local.ts
[error] 8-8: Shouldn't redeclare 'serverConfig'. Consider to delete it or rename it.
'serverConfig' is defined here:
(lint/suspicious/noRedeclare)
packages/nocodb/src/run/timely.ts
[error] 8-8: Shouldn't redeclare 'serverConfig'. Consider to delete it or rename it.
'serverConfig' is defined here:
(lint/suspicious/noRedeclare)
packages/nocodb/src/strategies/google.strategy/google.strategy.ts
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/nocodb/config/index.ts
[error] 158-158: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/nocodb/src/Noco.ts
[error] 91-91: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 203-203: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: jest-unit-test
🔇 Additional comments (92)
packages/nocodb/src/main.ts (2)
1-1: LGTM: Centralized configuration import.Good practice to centralize configuration management by importing from the unified config module.
19-19:✅ Verification successful
Verify that serverConfig provides proper port defaults.
The change from
process.env.PORT || 8080toserverConfig.portcentralizes configuration, which is excellent. However, ensure that the centralized config provides a proper default port value to maintain backwards compatibility.
🏁 Script executed:
#!/bin/bash # Description: Verify that serverConfig.port has proper defaults configured # Expected: Configuration should handle defaults for port # Check the config module for port defaults ast-grep --pattern 'port: $_' # Check for any references to default port values rg -A 3 -B 3 " 8000 8080|port.*default"Length of output: 74809
🏁 Script executed:
#!/bin/bash # Show serverConfig usage in main.ts echo "=== packages/nocodb/src/main.ts around serverConfig ===" rg -n "serverConfig" -A 5 -B 5 packages/nocodb/src/main.ts # Show complete config/index.ts to inspect default port echo -e "\n=== packages/nocodb/config/index.ts ===" sed -n '1,200p' packages/nocodb/config/index.tsLength of output: 5881
🏁 Script executed:
#!/bin/bash # Locate and inspect ServerConfig model to check type of port echo "=== packages/nocodb/config/model.ts ===" rg -n "class ServerConfig" -A 30 -B 5 packages/nocodb/config/model.ts || echo "ServerConfig not found" echo -e "\n=== Full content of model.ts ===" sed -n '1,200p' packages/nocodb/config/model.tsLength of output: 3790
serverConfig.port defaults to ‘8080’ as expected
Verified that in packages/nocodb/config/index.ts the fromDefault function setsport: '8080'and that the ServerConfig model (@isport) accepts this string. No further changes needed.packages/nocodb/src/helpers/dataReflectionHelpers.ts (2)
1-1: LGTM: Consistent configuration centralization.Good practice to use the centralized configuration module across the codebase.
6-8: LGTM: Preserved URL processing logic.The change correctly centralizes the public URL configuration while maintaining the existing URL processing logic (removing protocol prefix and trailing slash). The fallback to
NC_DATA_REFLECTION_HOSTenvironment variable is appropriately preserved.packages/nocodb/src/run/dockerRunPG_CyQuick.ts (2)
1-1: LGTM: Consistent configuration import.Properly imports the centralized configuration module.
30-30: LGTM: Consistent port configuration centralization.The change aligns with the centralized configuration approach while maintaining the same server initialization logic.
packages/nocodb/src/run/docker.ts (2)
1-1: LGTM: Centralized configuration import.Consistent with the configuration centralization approach across all server run scripts.
28-28: LGTM: Thorough port configuration updates.Excellent attention to detail in updating both active code and commented code to use the centralized
serverConfig.port. This ensures consistency throughout the codebase and prevents confusion when uncommenting code in the future.Also applies to: 34-34
packages/nocodb/src/gateways/socket.gateway.ts (1)
13-13: Good addition of centralized configuration import.The import of
serverConfigaligns with the PR's objective to centralize configuration management.packages/nocodb/src/run/dockerRunMysql.ts (2)
1-1: Good addition of centralized configuration import.The import of
serverConfigis consistent with the configuration unification approach across the codebase.
37-37: Port configuration centralization looks good.Replacing the hardcoded fallback logic with
serverConfig.portcentralizes the port configuration management effectively. This change is consistent with similar updates across other server startup files mentioned in the PR.packages/nocodb/src/controllers/plugins.controller.ts (2)
1-1: Good addition of centralized configuration import.The import of
serverConfigmaintains consistency with the configuration unification approach.
22-22: Good update to maintain consistency in commented code.Even though this is commented code, updating it to use
serverConfig.nocoDbConfig.isCloudinstead of the environment variable maintains consistency with the new centralized configuration approach and improves future maintainability.packages/nocodb/src/run/dockerEntry.ts (2)
1-1: Good addition of centralized configuration import.The import of
serverConfigis consistent with the configuration unification pattern applied across all server startup files.
19-19: Port configuration centralization is consistent and correct.Replacing the hardcoded port fallback with
serverConfig.portmaintains consistency with the configuration refactor across other server entry points. This centralizes port management effectively.packages/nocodb/src/run/dockerRunPG.ts (2)
1-1: LGTM: Clean import addition for centralized configuration.The import of
serverConfigproperly supports the configuration centralization objective.
36-36: Good refactor: Environment variable replaced with centralized config.This change correctly migrates from direct environment variable access to the structured
serverConfig.portproperty, improving configuration management consistency across the codebase.packages/nocodb/src/db/sql-data-mapper/__tests__/models/index.js (2)
1-1: LGTM: Proper import for configuration centralization.The import correctly supports the migration to centralized configuration management.
8-8:❓ Verification inconclusive
Verify environment value mapping for test configuration.
The change from
process.env.NODE_ENV === 'test'toserverConfig.environment === 'testing'introduces a potential breaking change. Ensure that the configuration module correctly maps the NODE_ENV value 'test' to the environment property 'testing'.
🏁 Script executed:
#!/bin/bash # Description: Verify that test environment is properly mapped in the configuration # Expected: Configuration should handle NODE_ENV=test correctly # Search for environment mapping in config files rg -A 5 -B 5 "environment.*test" --type ts --type js # Search for NODE_ENV usage in config module rg -A 5 -B 5 "NODE_ENV" packages/nocodb/config/ # Look for environment enum or mapping ast-grep --pattern 'environment: $_'Length of output: 68151
Ensure NODE_ENV ‘test’ is mapped to serverConfig.environment = 'testing'
It looks like there’s no explicit mapping of
process.env.NODE_ENV === 'test'→serverConfig.environment = 'testing'in the config module. Please verify (or add) this mapping so that existing test workflows aren’t broken:• Review your config loader in packages/nocodb/config/**
• Confirm that whenNODE_ENV==='test',serverConfig.environmentis set to'testing'
• If it’s missing, update the logic to translate'test'→'testing'before other code (e.g. before usage ininit-meta-service.provider.tsandutils/tele.ts)packages/nocodb/src/services/hooks.service.ts (2)
1-1: LGTM: Clean import for configuration centralization.The import properly supports the migration to structured configuration management.
37-37: Excellent improvement: Better type safety for cloud detection.The change from
process.env.NC_CLOUD === 'true'toserverConfig.nocoDbConfig.isCloudnot only centralizes configuration but also improves type safety by using a boolean property instead of string comparison. This reduces the risk of configuration errors.packages/nocodb/src/run/local.ts (2)
18-18: Good migration: Dashboard URL centralized.The change from
process.env.NC_DASHBOARD_URL ?? '/dashboard'toserverConfig.dashboardUrlproperly centralizes the dashboard URL configuration. The fallback logic should now be handled in the configuration module.
24-24: Proper port configuration centralization.The migration from
process.env.PORT || 8080toserverConfig.portcorrectly implements the centralized configuration pattern, moving fallback logic to the configuration module where it belongs.packages/nocodb/src/run/timely.ts (1)
15-15:✅ Verification successful
Configuration centralization looks good.
The replacement of environment variables with
serverConfigproperties aligns well with the PR objective to centralize configuration management.Verify that the
serverConfigobject has the expected properties:Also applies to: 21-21
🏁 Script executed:
#!/bin/bash # Description: Verify serverConfig properties exist in the configuration module echo "Checking serverConfig properties in config module..." # Search for dashboardUrl and port in the config module ast-grep --pattern $'dashboardUrl: $_' ast-grep --pattern $'port: $_' # Also check the config module structure rg -A 10 "export.*serverConfig" packages/nocodb/config/Length of output: 314
🏁 Script executed:
#!/bin/bash set -e echo "Searching for getCfg definition in packages/nocodb/config..." rg --context 3 "function getCfg" -n packages/nocodb/config echo "Searching for ServerConfig type or interface in config directory..." rg --context 3 "type ServerConfig" -n packages/nocodb/config rg --context 3 "interface ServerConfig" -n packages/nocodb/configLength of output: 200
🏁 Script executed:
#!/bin/bash set -e echo "Locate all references to getCfg in the repository..." rg -n "getCfg" . echo echo "Inspect imports of getCfg in config/index.ts:" sed -n '1,50p' packages/nocodb/config/index.ts | sed -n '1,20p' echo echo "Search for files exporting getCfg:" rg -n "export.*getCfg" -n .Length of output: 1225
🏁 Script executed:
#!/bin/bash set -e echo "Extracting getCfg function body to verify properties:" sed -n '140,260p' packages/nocodb/config/index.tsLength of output: 1539
LGTM — serverConfig properties verified
The centralized
serverConfigobject defined inpackages/nocodb/config/index.tsincludes all expected fields:
hostandport(defaulted infromDefault)publicUrlanddashboardUrl(derived ingetCfg)- Nested
nocoDbConfig, plus any CLI, TOML, or ENV overridesConfiguration centralization aligns with the PR’s objectives. Approving these changes.
packages/nocodb/src/app.config.ts (1)
1-1: Configuration centralization improves code simplicity.The replacement of the conditional environment variable logic with
serverConfig.dashboardUrlsimplifies the configuration and aligns with the centralized configuration approach.Also applies to: 19-19
packages/nocodb/src/modules/auth/auth.controller.ts (1)
1-1:✅ Verification successful
Configuration centralization is correctly implemented.
The replacement of
process.env.NC_PUBLIC_URLwithserverConfig.publicUrlfollows the centralized configuration pattern consistently.Verify that the
publicUrlproperty exists inserverConfig:Also applies to: 264-264
🏁 Script executed:
#!/bin/bash # Description: Verify publicUrl property exists in serverConfig echo "Checking for publicUrl property in config module..." # Search for publicUrl in the config module ast-grep --pattern $'publicUrl: $_' rg "publicUrl" packages/nocodb/config/Length of output: 346
Approved: Configuration centralization in auth.controller.ts
The
serverConfig.publicUrlreplacement forprocess.env.NC_PUBLIC_URLis valid—publicUrlis defined in:
- packages/nocodb/config/model.ts
- packages/nocodb/config/index.ts
No further changes needed.
packages/nocodb/src/nocobuild.ts (1)
2-2: Configuration centralization is consistently applied.The replacement of environment variable usage with
serverConfig.dashboardUrlmaintains consistency with the centralized configuration approach across the codebase.Also applies to: 11-11
packages/nocodb/src/modules/jobs/redis/jobs-redis.ts (1)
1-1: LGTM: Centralized configuration importThe import statement correctly brings in the serverConfig object from the new centralized configuration module.
packages/nocodb/src/controllers/api-docs/api-docs.controller.ts (2)
1-1: LGTM: Centralized configuration importThe import statement correctly brings in the serverConfig object for consistent configuration management.
62-62:✅ Verification successful
Excellent consistency in replacing environment variable usage
The changes consistently replace
process.env.NC_PUBLIC_URL || ''withserverConfig.publicUrlacross all four HTML generation methods. This centralizes the public URL configuration and improves maintainability.Please verify that
serverConfig.publicUrlproperly handles cases where the URL is not configured (equivalent to the previous|| ''fallback behavior).Also applies to: 73-73, 84-84, 95-95
🏁 Script executed:
#!/bin/bash # Description: Verify serverConfig.publicUrl handles empty/undefined values # Expected: Find serverConfig definition and publicUrl property handling # Search for publicUrl property definition and default value handling rg -A 3 -B 3 "publicUrl" # Search for any remaining NC_PUBLIC_URL usage rg "NC_PUBLIC_URL"Length of output: 76456
Verified publicUrl fallback – no changes needed
The replacement of
process.env.NC_PUBLIC_URL || ''withserverConfig.publicUrlpreserves and even strengthens the previous behavior. In packages/nocodb/config/index.ts you can see:
mergedCfg.publicUrl ??= \http://${mergedCfg.host}:${mergedCfg.port}/`;`This guarantees
serverConfig.publicUrlis always set (either from NC_PUBLIC_URL or the host/port default), so there’s no risk of an empty string. All HTML generators will receive a valid URL.packages/nocodb/src/services/shared-bases.service.ts (2)
1-1: LGTM: Centralized configuration importThe import statement correctly integrates the centralized configuration module.
105-105:✅ Verification successful
Improved cloud environment detection
The change from
process.env.NC_CLOUD === 'true'toserverConfig.nocoDbConfig.isCloudis a semantic improvement that uses structured configuration instead of string-based environment variable parsing. The nested property structure (nocoDbConfig.isCloud) suggests good organization of cloud-specific settings.Please verify that
serverConfig.nocoDbConfig.isCloudis properly typed as a boolean and correctly maps the NC_CLOUD environment variable.
🏁 Script executed:
#!/bin/bash # Description: Verify isCloud property definition and usage # Expected: Find serverConfig definition and other isCloud usages # Search for isCloud property definition and usage rg -A 3 -B 3 "isCloud" # Search for any remaining NC_CLOUD usage rg "NC_CLOUD"Length of output: 13156
Confirmed:
isCloudis a boolean and correctly mapsNC_CLOUD
- In packages/nocodb/config/model.ts,
NocoDbConfigdeclares
@IsBoolean() isCloud: boolean;- In packages/nocodb/config/index.ts,
isCloudis initialized via
stringToBoolTry(process.env.NC_CLOUD)No further changes required.
packages/nocodb/src/version-upgrader/upgraders/0225002_ncDatasourceDecrypt.ts (2)
4-4: LGTM: Centralized configuration importThe import statement correctly brings in the serverConfig object for consistent configuration access.
64-64:✅ Verification successful
Critical: Verify JWT secret mapping in encryption context
This change replaces
process.env.NC_AUTH_JWT_SECRETwithserverConfig.auth.jwt.secretin a database version upgrader that handles encryption/decryption of sensitive data. The correctness of this mapping is critical for data integrity.Please ensure that:
serverConfig.auth.jwt.secretcorrectly maps the NC_AUTH_JWT_SECRET environment variable- The value type and format remain consistent (string)
- The fallback mechanism on lines 72 8000 -77 still functions correctly with the new configuration source
🏁 Script executed:
#!/bin/bash # Description: Verify JWT secret configuration mapping and usage # Expected: Find serverConfig definition and ensure JWT secret is properly configured # Search for JWT secret configuration and auth structure rg -A 5 -B 5 "jwt.*secret|auth.*jwt" # Search for any remaining NC_AUTH_JWT_SECRET usage rg "NC_AUTH_JWT_SECRET" # Check if the auth configuration structure is defined ast-grep --pattern 'auth: { $$$, jwt: { $$$, secret: $_ }, $$$ }'Length of output: 77138
Verify JWT Secret Source and Fallback Logic
We’ve confirmed:
- There is no direct
process.env.NC_AUTH_JWT_SECRETin the code—everything reads fromserverConfig.auth.jwt.secret.Noco.initJwt()will fetchnc_auth_jwt_secretfrom the metadata store and assign it toserverConfig.auth.jwt.secretif it was not already set.- All consumers (the upgrader, JWT strategy,
NcConfig, etc.) rely exclusively onserverConfig.auth.jwt.secret.Next steps for you:
- Ensure your deployment or bootstrap code populates
serverConfig.auth.jwt.secretfrom theNC_AUTH_JWT_SECRETenv-var (or other secure source) before running migrations.- Verify that
Noco.initJwt()is called early (as it is ininit-meta-service.provider.ts) so the fallback (lines 72–77 in the upgrader) will pick up the stored secret if the env var was absent.- Confirm in your configuration loader (e.g. wherever you construct
serverConfig) that it readsNC_AUTH_JWT_SECRETintoauth.jwt.secret.No code changes required here, but please double-check that your runtime config flow wires
NC_AUTH_JWT_SECRET → serverConfig.auth.jwt.secret.packages/nocodb/src/modules/auth/auth.module.ts (2)
18-18:✅ Verification successful
Verify the logic change for worker type detection.
The logic has changed significantly:
- Old:
process.env.NC_WORKER_CONTAINER === 'true'(only includes AuthController when explicitly 'true')- New:
serverConfig.workerType != "worker"(includes AuthController unless workerType is 'worker')This semantic change could affect when AuthController is included. Please verify this matches the intended behavior.
🏁 Script executed:
#!/bin/bash # Check if workerType property exists in config model and verify possible values fd -t f "model.ts" packages/nocodb/config/ --exec cat {} \; | grep -A 10 -B 5 "workerType" # Also search for any documentation or comments about worker types rg -A 5 -B 5 "workerType|worker.*type|WORKER.*TYPE" --type tsLength of output: 11957
No changes needed: AuthController inclusion logic is correct
The new check
serverConfig.workerType !== 'worker'is consistent with how other modules gate their controllers and matches the intended “only exclude in pure worker” behavior.
workerTypecan be'disabled','main', or'worker'(seepackages/nocodb/config/model.ts)- Default (
'disabled') and'main'both include controllers; only'worker'excludes them- This aligns with the code in
Noco.ts,jobs.module.ts, and elsewhere
1-1:❓ Verification inconclusive
Verify the import path for serverConfig.
The import uses
'config'as the module path. Ensure this resolves correctly topackages/nocodb/config/index.tsand that theserverConfigobject is properly exported.
🏁 Script executed:
#!/bin/bash # Verify the config module exists and exports serverConfig fd -t f "index.ts" packages/nocodb/config/ --exec cat {} \; | grep -A 5 -B 5 "export.*serverConfig"Length of output: 172
🏁 Script executed:
#!/bin/bash # Show the import and the workerType conditional in auth.module.ts echo "==== Top of auth.module.ts ====" sed -n '1,20p' packages/nocodb/src/modules/auth/auth.module.ts echo "==== workerType conditional snippet ====" rg -n "serverConfig.workerType" -n packages/nocodb/src/modules/auth/auth.module.ts -C 3Length of output: 1428
Verify AuthController inclusion logic with new workerType config
- The import
import { serverConfig } from 'config'correctly resolves topackages/nocodb/config/index.ts, andserverConfigis properly exported.- In
packages/nocodb/src/modules/auth/auth.module.ts(line 18), the conditional was changed from
process.env.NC_WORKER_CONTAINER === 'true'
to
serverConfig.workerType != "worker".
This inverts the inclusion semantics:
- Old: only register
AuthControllerwhenNC_WORKER_CONTAINER === 'true'.- New: register
AuthControllerfor allworkerTypevalues except"worker".Please confirm that this semantic change aligns with the intended behavior across your deployment environments.
packages/nocodb/src/app.module.ts (2)
75-75:✅ Verification successful
Verify serverConfig.dashboardUrl property and default behavior.
The previous code used
process.env.NC_DASHBOARD_URL ?? '/dashboard'as the default. EnsureserverConfig.dashboardUrlprovides the same default behavior.
🏁 Script executed:
#!/bin/bash # Check if dashboardUrl property exists and has proper default fd -t f "model.ts" packages/nocodb/config/ --exec cat {} \; | grep -A 5 -B 5 "dashboardUrl\|dashboard.*[Uu]rl" # Also check the config loading logic for defaults fd -t f "index.ts" packages/nocodb/config/ --exec cat {} \; | grep -A 10 -B 5 "dashboard"Length of output: 1011
Default behavior of
serverConfig.dashboardUrlverifiedThe
dashboardUrldefault is explicitly set inpackages/nocodb/config/index.ts:
- Uses the provided
NC_DASHBOARD_URLif set- Otherwise defaults to:
'/dashboard'when not in cloud mode'/'whennocoDbConfig.isCloudistrueThis matches the previous fallback of
process.env.NC_DASHBOARD_URL ?? '/dashboard'for non-cloud setups (with an added root path override for cloud). No further changes required.
41-41:✅ Verification successful
Verify serverConfig.environment property exists.
Ensure the
environmentproperty is defined in the serverConfig schema and properly maps fromNODE_ENV.
🏁 Script executed:
#!/bin/bash # Check if environment property exists in config model fd -t f "model.ts" packages/nocodb/config/ --exec cat {} \; | grep -A 5 -B 5 "environment"Length of output: 551
🏁 Script executed:
#!/bin/bash # Locate serverConfig definition and its assignment for 'environment' rg -C 3 "serverConfig" -g "*.ts" packages/nocodbLength of output: 66576
🏁 Script executed:
#!/bin/bash # Verify how `environment` is populated from NODE_ENV in config/index.ts rg -n "NODE_ENV" -C 5 packages/nocodb/config/index.ts rg -n "environment" -C 5 packages/nocodb/config/index.tsLength of output: 1206
serverConfig.environment Verified
TheServerConfigclass includes anenvironmentproperty (validated against ['testing','production','development','staging']), andpackages/nocodb/config/index.tscorrectly assigns it fromprocess.env.NODE_ENV(with a production default). No further changes are required.nix/package.nix (1)
113-113: LGTM: Routine dependency hash update.This appears to be a standard update to the pnpm dependencies hash, likely reflecting the new configuration-related dependencies. The hash ensures integrity verification of fetched dependencies.
packages/nocodb/src/run/cloud.ts (3)
1-1: LGTM: Clean import of centralized configuration.Good addition of the centralized configuration import to replace scattered environment variable access.
36-36: LGTM: Consistent port configuration usage.The replacement of
process.env.PORTwithserverConfig.portaligns well with the centralized configuration approach.
12-12:✅ Verification successful
Verify dashboard URL default handling in centralized config.
The transition from
process.env.NC_DASHBOARD_URLtoserverConfig.dashboardUrllooks correct. However, the previous code had implicit fallback behavior whereprocess.env.NC_DASHBOARD_URLwould beundefinedand potentially set to'/'elsewhere. Ensure that the centralized configuration properly handles the default dashboard URL value.Also applies to: 18-18, 24-24
🏁 Script executed:
#!/bin/bash # Description: Verify that serverConfig.dashboardUrl has proper default handling # Expected: Should find default dashboard URL configuration in the config module # Search for dashboard URL configuration and defaults rg -A 5 -B 5 "dashboardUrl|dashboard.*url" --type tsLength of output: 30412
Default dashboardUrl fallback confirmed
The centralized config in
packages/nocodb/config/index.tsalready provides the expected default:
mergedCfg.dashboardUrl ??= mergedCfg.nocoDbConfig.isCloud ? '/' : '/dashboard';No further changes are needed here.
packages/nocodb/src/modules/jobs/jobs/at-import/at-import.controller.ts (2)
1-1: LGTM: Consistent import of centralized configuration.Good addition of the centralized configuration import.
52-52:✅ Verification successful
Verify port default handling in centralized config.
The change from
process.env.PORT || 8080toserverConfig.portremoves the explicit fallback to port 8080. Ensure that the centralized configuration system properly handles the default port value, especially for Docker environments.
🏁 Script executed:
#!/bin/bash # Description: Verify that serverConfig.port has proper default handling including 8080 fallback # Expected: Should find port configuration with appropriate defaults # Search for port configuration and defaults rg -A 5 -B 5 "port.*8080|8080.*port|serverConfig.*port" --type ts # Also check for any Docker-specific port configuration rg -A 3 -B 3 "NC_DOCKER.*port|port.*NC_DOCKER" --type tsLength of output: 14692
Port default fallback confirmed in centralized config
Verified that
serverConfig.portfalls back to8080in the centralized configuration:
- In packages/nocodb/src/utils/nc-config/NcConfig.ts:
ncConfig.port = +(port ?? 8080);- In packages/nocodb/config/index.ts:
port: '8080',No further changes are needed.
packages/nocodb/src/controllers/utils.controller.ts (2)
1-1: LGTM: Consistent import of centralized configuration.Good addition of the centralized configuration import.
46-46:✅ Verification successful
Excellent improvement: String comparison replaced with boolean check.
The change from
process.env.NC_CLOUD !== 'true'to!serverConfig.nocoDbConfig.isCloudis a significant improvement. Using a boolean property instead of string comparison enhances type safety and reduces potential for errors.
🏁 Script executed:
#!/bin/bash # Description: Verify that NC_CLOUD environment variable is properly converted to boolean in config # Expected: Should find proper boolean conversion logic for isCloud property # Search for NC_CLOUD environment variable handling and boolean conversion rg -A 5 -B 5 "NC_CLOUD|isCloud" --type ts # Look for boolean conversion patterns rg -A 3 -B 3 "true.*false|boolean.*NC_CLOUD" --type tsLength of output: 51938
Verified NC_CLOUD Boolean Conversion
I’ve confirmed that
process.env.NC_CLOUDis parsed into a boolean viastringToBoolTry(process.env.NC_CLOUD)inpackages/nocodb/config/index.ts. TheisCloudflag is now correctly typed and safe to use throughout the codebase.packages/nocodb/src/utils/appUrl.ts (2)
1-1: LGTM: Consistent import of centralized configuration.Good addition of the centralized configuration import.
3-5:✅ Verification successful
Excellent simplification, but verify URL construction logic preservation.
The refactored
getAppUrlfunction is much cleaner and aligns perfectly with the centralized configuration approach. However, ensure that all the previous URL construction logic (including environment variable fallbacks and validation) is now properly handled within the centralizedserverConfigsystem.
🏁 Script executed:
#!/bin/bash # Description: Verify that URL construction logic is preserved in centralized config # Expected: Should find comprehensive URL handling in config module # Search for public URL and dashboard URL configuration rg -A 10 -B 5 "publicUrl|public.*url|NC_PUBLIC_URL" --type ts # Look for URL construction and validation logic rg -A 5 -B 5 "url.*construction|dashboard.*url.*config" --type ts # Check for any URL-related environment variable handling rg -A 3 -B 3 "process\.env.*URL|URL.*process\.env" --type tsLength of output: 37316
🏁 Script executed:
#!/bin/bash # Inspect how mergedCfg is populated and which environment variables are read sed -n '1,200p' packages/nocodb/config/index.tsLength of output: 5195
getAppUrl URL logic validated
All of the URL-construction and fallback logic has been centralized in
config/index.ts:•
hostandportnow come from (in order) TOML, environment (HOST/PORT), CLI, or fall back tolocalhost:8080.
•publicUrlis derived (if unset) ashttp://<host>:<port>/.
•nocoDbConfig.isCloud(fromNC_CLOUDor TOML) drives the defaultdashboardUrl(/vs./dashboard).
• BothpublicUrlanddashboardUrlare validated byclass-validator(@IsUrl).The simplified
export const getAppUrl = () => `${serverConfig.publicUrl}${serverConfig.dashboardUrl} `;now correctly reflects the same behavior as before. No further changes needed.
packages/nocodb/src/services/users/users.service.ts (3)
1-1: LGTM: Clean import for centralized configuration.The import of
serverConfigsupports the migration from scattered environment variable access to centralized configuration management.
148-148: LGTM: Correct migration from environment variable to centralized config.The logic is semantically equivalent:
- Old:
process.env.NC_CLOUD !== 'true'(not in cloud)- New:
!serverConfig.nocoDbConfig.isCloud(not in cloud)This change improves configuration management by centralizing all environment-based decisions through the
serverConfigobject.
185-185: LGTM: Consistent application of centralized configuration.This mirrors the same logical transformation as line 148, maintaining consistency in the cloud detection logic for default project creation.
packages/nocodb/src/version-upgrader/NcUpgrader.ts (2)
1-1: LGTM: Consistent import for configuration centralization.The import aligns with the broader refactoring effort to centralize configuration management.
97-97: LGTM: Correct cloud detection logic migration.The change maintains logical equivalence while improving configuration consistency:
- Old:
process.env.NC_CLOUD !== 'true'- New:
!serverConfig.nocoDbConfig.isCloudBoth expressions evaluate to
truewhen not running in cloud mode, preserving the upgrade detection logic.packages/nocodb/src/middlewares/global/global.middleware.ts (3)
6-6: LGTM: Import supports configuration centralization.Adding the
serverConfigimport enables the middleware to access centralized configuration properties.
12-12: LGTM: Good practice to indicate unused parameter.Renaming the
resparameter to_clearly communicates that this parameter is not used in the method implementation.
20-20: LGTM: Simplified dashboard URL configuration.The change from potentially complex config service logic to direct property access (
serverConfig.dashboardUrl) simplifies the code while maintaining functionality through the centralized configuration system.packages/nocodb/src/modules/noco.module.ts (2)
1-1: LGTM: Consistent configuration import.The import enables access to centralized configuration, following the established pattern across the codebase.
175-175: LGTM: Correct worker type detection migration.The change correctly migrates from environment variable to centralized configuration:
- Old:
process.env.NC_WORKER_CONTAINER !== 'true'- New:
serverConfig.workerType !== 'worker'This aligns with the PR's mention of renaming
NC_WORKER_CONTAINERtoNC_WORKER_TYPEand maintains the logical intent of excluding controllers when running as a worker.packages/nocodb/src/modules/jobs/jobs.module.ts (2)
1-1: LGTM: Clean import of centralized configuration.The import of
serverConfigfrom 'config' aligns with the centralized configuration architecture.
81-92: LGTM: Worker type logic correctly refactored.The conditional logic is preserved while moving from environment variables to centralized configuration. The change from checking
NC_WORKER_CONTAINER !== 'true'toserverConfig.workerType !== 'worker'maintains the intent of conditionally including controllers based on worker type.packages/nocodb/src/services/utils.service.ts (5)
1-1: LGTM: Consistent configuration import.The import follows the same pattern as other files in this refactoring effort.
444-444: LGTM: Environment configuration centralized.Replacing
process.env.NODE_ENVwithserverConfig.environmentmaintains the same functionality while using centralized configuration.
445-450: LGTM: OAuth configuration centralized.Moving OAuth client credentials to centralized configuration improves security and maintainability by allowing validation and centralized management of sensitive data.
454-454: LGTM: External database configuration centralized.The
connectToExternalDBproperty now uses centralized configuration, which is consistent with the refactoring goals.
474-474: LGTM: Cloud configuration centralized.The
isCloudproperty now uses centralized configuration, maintaining consistency with the overall refactoring effort.packages/nocodb/src/run/testDocker.ts (3)
1-1: LGTM: Consistent configuration import.Import follows the established pattern for centralized configuration.
32-32: LGTM: Worker type check consistent with other files.The worker type check uses the same
serverConfig.workerTypeproperty as other files, maintaining logical consistency across the codebase.
33-103: LGTM: Port configuration centralized throughout the test setup.All server and HTTP client operations now use
serverConfig.port, ensuring consistency between server listen port and client requests. This eliminates potential port mismatches that could occur with hardcoded values or separate environment variables.packages/nocodb/src/helpers/initBaseBehaviour.ts (1)
7-7: LGTM: Consistent configuration import.Import follows the same pattern used across the refactoring.
packages/nocodb/src/cache/RedisCacheMgr.ts (2)
1-1: LGTM: Centralized configuration import added.The import of
serverConfigaligns with the configuration unification effort.
16-18:✅ Verification successful
Verify the logic mapping between old environment variables and new config properties.
The refactoring looks clean, but please ensure the logic equivalence:
NC_WORKER_CONTAINER→serverConfig.workerType !== 'worker'NC_CLOUD→serverConfig.nocoDbConfig.isCloudConfirm that the worker type mapping and cloud flag behavior remain consistent with the previous implementation.
🏁 Script executed:
#!/bin/bash # Description: Verify serverConfig workerType and isCloud property usage across codebase # Expected: Find consistent usage patterns for these properties # Search for workerType usage echo "=== serverConfig.workerType usage ===" rg -A 3 "serverConfig\.workerType" echo -e "\n=== serverConfig.nocoDbConfig.isCloud usage ===" rg -A 3 "serverConfig\.nocoDbConfig\.isCloud"Length of output: 8477
✅ Mapping of legacy flags validated
I’ve confirmed that replacing
NC_WORKER_CONTAINERwithserverConfig.workerType !== 'worker'andNC_CLOUDwithserverConfig.nocoDbConfig.isCloudpreserves the original semantics across the codebase (see consistent patterns in Noco.ts, modules/auth, modules/jobs, RedisCacheMgr.ts, etc.). No further changes needed.packages/nocodb/src/modules/jobs/redis/jobs.service.ts (2)
1-1: LGTM: Centralized configuration import added.The import of
serverConfigaligns with the configuration unification effort.
28-28: Verify the logic change from environment variable to worker type.The condition changed from
process.env.NC_WORKER_CONTAINER === 'false'toserverConfig.workerType === 'main'. Please confirm this logic mapping is intentional and correct.packages/nocodb/src/utils/cloud/populateCloudPlugins.ts (4)
1-1: LGTM: Centralized configuration import added.The import of
serverConfigaligns with the configuration unification effort.
8-8: Excellent simplification of S3 configuration check.Replacing individual environment variable checks with a single
serverConfig.s3CloudConfigcheck improves readability and maintainability.
30-35: LGTM: Clean refactoring to structured S3 configuration.The replacement of individual
process.envaccesses withserverConfig.s3CloudConfigproperties provides better type safety and centralized configuration management.
74-78: LGTM: Clean refactoring to structured SES configuration.The replacement of individual
process.envaccesses withserverConfig.sesConfigproperties maintains consistency with the S3 refactoring and improves configuration management.packages/nocodb/src/utils/nc-config/NcConfig.ts (3)
1-1: LGTM: Centralized configuration import added.The import of
serverConfigaligns with the configuration unification effort across the codebase.
146-146: LGTM: JWT secret configuration centralized.Replacing
process.env.NC_AUTH_JWT_SECRETwithserverConfig.auth.jwt.secretprovides better type safety and follows the centralized configuration pattern.
150-151: LGTM: Dashboard and public URL configuration centralized.The replacement of
process.env.NC_DASHBOARD_URLandprocess.env.NC_PUBLIC_URLwithserverConfig.dashboardUrlandserverConfig.publicUrlmaintains consistency with the overall configuration refactoring effort.packages/nocodb/config/util.ts (1)
31-45: LGTM for boolean conversion logic.The string-to-boolean conversion function correctly handles case-insensitive "true"/"false" strings and returns the original value for other inputs.
packages/nocodb/src/services/bases.service.ts (2)
1-1: LGTM for configuration centralization.The import of
serverConfigaligns with the broader refactor to centralize configuration management.
224-224: LGTM for remaining configuration replacements.The replacements for minimal DB, external DB, and cloud settings correctly map environment variables to the centralized configuration properties.
Also applies to: 272-272, 330-330
packages/nocodb/src/providers/init-meta-service.provider.ts (2)
20-20: LGTM for configuration import.The import of
serverConfigsupports the configuration centralization effort.
103-103: LGTM for configuration property replacements.All environment variable replacements with
serverConfigproperties are consistent and maintain equivalent logic:
- Migration jobs version, license key, cloud flags, and environment checks are correctly mapped to the centralized configuration.
Also applies to: 117-117, 122-122, 136-136, 140-140
packages/nocodb/src/utils/tele.ts (3)
1-1: LGTM for configuration import.The import of
serverConfigsupports the configuration centralization effort.
81-81: LGTM for environment variable replacements.The replacements of
process.env.NODE_ENVwithserverConfig.environmentare consistent and maintain the same logic for telemetry data collection.Also applies to: 257-258, 267-267
300-307:✅ Verification successful
Verify that publicUrl is always defined to prevent HTTP request failures.
The keep-alive HTTP request was changed from being conditional (likely based on
process.env.NC_PUBLIC_URL) to always running withserverConfig.publicUrl. This could cause failures ifpublicUrlis undefined or null.Ensure the configuration schema provides a default value for
publicUrlor add a conditional check:-setInterval(() => { - axios({ - method: 'get', - url: serverConfig.publicUrl, - }) - .then(() => {}) - .catch(() => {}); -}, 2 * 60 * 60 * 1000).unref(); +if (serverConfig.publicUrl) { + setInterval(() => { + axios({ + method: 'get', + url: serverConfig.publicUrl, + }) + .then(() => {}) + .catch(() => {}); + }, 2 * 60 * 60 * 1000).unref(); +}
🏁 Script executed:
#!/bin/bash # Description: Verify that serverConfig.publicUrl has a default value or proper validation # Expected: Find configuration schema that ensures publicUrl is always defined rg -A 10 -B 5 "publicUrl|NC_PUBLIC_URL" --type tsLength of output: 22003
publicUrl is always defined and validated at startup
The configuration loader inpackages/nocodb/config/index.tsassigns a default:mergedCfg.publicUrl ??= `http://${mergedCfg.host}:${mergedCfg.port}/`;and the
ServerConfigmodel enforces@IsUrl. As a result,serverConfig.publicUrlcan never be undefined at runtime, so the keep-alive HTTP call is safe as written.packages/nocodb/config/model.ts (4)
1-13: LGTM! Well-structured imports for configuration validation.The imports are properly organized and include all necessary decorators for comprehensive validation and type transformation.
15-41: Authentication configuration classes look good.The OIDC and JWT configuration classes properly implement validation with appropriate length constraints. The AuthConfig class correctly aggregates optional OIDC configurations and required JWT configuration.
108-114: Well-defined constants with proper TypeScript typing.The environment and worker type constants are properly defined using
as constfor type safety and include comprehensive options.
116-154: Comprehensive server configuration with excellent validation patterns.The ServerConfig class effectively aggregates all configuration components with:
- Proper validation decorators for each property type
- Appropriate use of enum validation with
@IsIn()- Well-structured nested validation with
@ValidateNested()- Logical separation between required and optional configurations
The structure promotes type safety and centralized configuration management.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/nocodb/config/index.ts (3)
77-82: Fix incorrect environment variable mappings for S3 cloud configuration.The S3 cloud configuration has incorrect environment variable mappings that will cause configuration failures.
Apply this diff to fix the mappings:
s3CloudConfig: { accessKey: process.env.NC_CLOUD_S3_ACCESS_KEY, - secretKey: process.env.NC_CLOUD_S3_ACCESS_SECRET, + secretKey: process.env.NC_CLOUD_S3_SECRET_KEY, acl: process.env.NC_CLOUD_S3_ACL, - bucketName: process.env.NC_CLOUD_BUCKET_NAME, + bucketName: process.env.NC_CLOUD_S3_BUCKET_NAME, region: process.env.NC_CLOUD_S3_REGION, },
84-90: Fix incorrect environment variable mappings for S3 configuration.The S3 configuration has incorrect environment variable mappings and uses the wrong variable name for the secret key.
Apply this diff to fix the mappings:
s3Config: { accessKey: process.env.NC_S3_ACCESS_KEY, - secretKey: process.env.NC_S3_ACCESS_SECRET, + secretKey: process.env.NC_S3_SECRET_KEY, acl: process.env.NC_S3_ACL, bucketName: process.env.NC_S3_BUCKET_NAME, region: process.env.NC_S3_REGION, forcePathStyle: stringToBoolTry(process.env.NC_S3_FORCE_PATH_STYLE), endPoint: process.env.NC_S3_ENDPOINT, },
95-95: Fix the typo in the SMTP password field name.The field name has a typo that will cause the SMTP password configuration to fail.
Apply this diff to fix the typo:
- passowrd: process.env.NC_SMTP_PASSWORD, + password: process.env.NC_SMTP_PASSWORD,
🧹 Nitpick comments (1)
packages/nocodb/config/index.ts (1)
158-158: Replace delete operator with undefined assignment for better performance.The static analysis tool correctly identified that the
deleteoperator can impact performance.Apply this change for better performance:
- delete cliCfg['config']; + cliCfg['config'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nocodb/config/index.ts(1 hunks)packages/nocodb/config/model.ts(1 hunks)packages/nocodb/src/helpers/NcPluginMgrv2.ts(3 hunks)packages/nocodb/src/run/local.ts(2 hunks)packages/nocodb/src/run/timely.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nocodb/src/run/timely.ts
- packages/nocodb/src/run/local.ts
- packages/nocodb/src/helpers/NcPluginMgrv2.ts
- packages/nocodb/config/model.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/nocodb/config/index.ts
[error] 158-158: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-tests-pg
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: release-docker / buildx
- GitHub Check: jest-unit-test
🔇 Additional comments (3)
packages/nocodb/config/index.ts (3)
1-8: Import statements look good.The imports are appropriate for the configuration module's functionality including file system operations, validation, CLI parsing, TOML parsing, and utility functions.
10-45: Default configuration function is well-structured.The
fromDefaultfunction provides sensible defaults and conditionally includes S3 and SMTP configurations based on usage, which is a good optimization.
176-183: Configuration validation and error handling look solid.The use of
class-transformerandclass-validatorfor configuration validation is a best practice that ensures type safety and validation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/nocodb/config/index.ts (3)
95-95: Fix the typo in the SMTP password field name.The field name has a typo that will cause the SMTP password configuration to fail.
Apply this diff to fix the typo:
- passowrd: process.env.NC_SMTP_PASSWORD, + password: process.env.NC_SMTP_PASSWORD,
116-138: Add error handling for TOML file reading.The function will throw an unhandled error if the specified config path exists but cannot be read (permissions, invalid TOML syntax, etc.).
Add proper error handling:
const fromToml = (cfgPath: string = process.env.NC_CFG_PATH): object => { const defaultCfgPaths = ['./nocodb.toml', '/etc/nocodb.toml']; let tomlString: string; if (cfgPath) { - tomlString = fs.readFileSync(cfgPath, 'utf8'); + try { + tomlString = fs.readFileSync(cfgPath, 'utf8'); + } catch (error) { + throw new Error(`Failed to read config file ${cfgPath}: ${error.message}`); + } } else { for (const cfgPath of defaultCfgPaths) { if (!fs.existsSync(cfgPath)) { continue; } - tomlString = fs.readFileSync(cfgPath, 'utf8'); + try { + tomlString = fs.readFileSync(cfgPath, 'utf8'); + } catch (error) { + continue; // Try next path if this one fails + } break; } if (typeof tomlString === 'undefined') { return {}; } } - return toml.parse(tomlString); + try { + return toml.parse(tomlString); + } catch (error) { + throw new Error(`Failed to parse TOML configuration: ${error.message}`); + } };
174-175: Fix the conditional logic for externalDb configuration.The current logic unconditionally overwrites
externalDbwhen it's truthy, which seems incorrect. The condition should only modifyexternalDbwhen needed.The logic appears flawed. If
externalDbis true andminimalDbis also true, this would setexternalDbto false, which may not be intended. Consider clarifying the intended behavior:- if (mergedCfg.nocoDbConfig.externalDb) - mergedCfg.nocoDbConfig.externalDb = !mergedCfg.nocoDbConfig.minimalDb; + // Only disable externalDb if minimalDb is enabled + if (mergedCfg.nocoDbConfig.minimalDb) { + mergedCfg.nocoDbConfig.externalDb = false; + }
🧹 Nitpick comments (2)
packages/nocodb/config/index.ts (2)
155-155: Fix typo in comment.There's a typo in the comment that should be corrected for clarity.
Apply this diff to fix the typo:
- // disable cliFlags on teting, flags conflict + // disable cliFlags on testing, flags conflict
159-159: Replace delete operator with undefined assignment for better performance.The delete operator can impact performance. Use undefined assignment instead as suggested by static analysis.
Apply this diff to improve performance:
- delete cliCfg['config']; + cliCfg['config'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 159-159: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nocodb/config/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/nocodb/config/index.ts (2)
packages/nocodb/config/util.ts (2)
stringToBoolTry(31-45)rmUndefined(17-29)packages/nocodb/config/model.ts (1)
ServerConfig(116-154)
🪛 Biome (1.9.4)
packages/nocodb/config/index.ts
[error] 159-159: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
- GitHub Check: bumper
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/nocodb/config/index.ts (2)
116-138: Add error handling for TOML file reading.The function will throw an unhandled error if the specified config path exists but cannot be read (permissions, invalid TOML syntax, etc.).
Add proper error handling:
const fromToml = (cfgPath: string = process.env.NC_CFG_PATH): object => { const defaultCfgPaths = ['./nocodb.toml', '/etc/nocodb.toml']; let tomlString: string; if (cfgPath) { - tomlString = fs.readFileSync(cfgPath, 'utf8'); + try { + tomlString = fs.readFileSync(cfgPath, 'utf8'); + } catch (error) { + throw new Error(`Failed to read config file ${cfgPath}: ${error.message}`); + } } else { for (const cfgPath of defaultCfgPaths) { if (!fs.existsSync(cfgPath)) { continue; } - tomlString = fs.readFileSync(cfgPath, 'utf8'); + try { + tomlString = fs.readFileSync(cfgPath, 'utf8'); + } catch (error) { + continue; // Try next path if this one fails + } break; } if (typeof tomlString === 'undefined') { return {}; } } - return toml.parse(tomlString); + try { + return toml.parse(tomlString); + } catch (error) { + throw new Error(`Failed to parse TOML configuration: ${error.message}`); + } };
174-175: Fix the conditional logic for externalDb configuration.The current logic unconditionally overwrites
externalDbwhen it's truthy, which seems incorrect. The condition should only modifyexternalDbwhen needed.The logic appears flawed. If
externalDbis true andminimalDbis also true, this would setexternalDbto false, which may not be intended. Consider clarifying the intended behavior:- if (mergedCfg.nocoDbConfig.externalDb) - mergedCfg.nocoDbConfig.externalDb = !mergedCfg.nocoDbConfig.minimalDb; + // Only disable externalDb if minimalDb is enabled + if (mergedCfg.nocoDbConfig.minimalDb) { + mergedCfg.nocoDbConfig.externalDb = false; + }
🧹 Nitpick comments (2)
packages/nocodb/config/index.ts (2)
155-155: Fix typo in comment.There's a spelling error in the comment that affects code readability.
- // disable cliFlags on teting, flags conflict + // disable cliFlags on testing, flags conflict
159-159: Replace delete operator with undefined assignment for better performance.The static analysis tool correctly identifies that the
deleteoperator can impact performance. Using undefined assignment is more efficient.- delete cliCfg['config']; + cliCfg['config'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 159-159: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nocodb/config/index.ts(1 hunks)packages/nocodb/config/util.ts(1 hunks)packages/nocodb/src/modules/jobs/redis/jobs.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nocodb/config/util.ts
- packages/nocodb/src/modules/jobs/redis/jobs.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/nocodb/config/index.ts (2)
packages/nocodb/config/util.ts (2)
stringToBoolTry(31-45)rmUndefined(17-29)packages/nocodb/config/model.ts (1)
ServerConfig(116-154)
🪛 Biome (1.9.4)
packages/nocodb/config/index.ts
[error] 159-159: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
- GitHub Check: bumper
🔇 Additional comments (4)
packages/nocodb/config/index.ts (4)
51-51: Environment variable naming aligns with PR objectives.The usage of
NC_WORKER_TYPEcorrectly implements the renaming fromNC_WORKER_CONTAINERas mentioned in the PR objectives. This maintains consistency with theNC_*prefix pattern.
77-91: S3 configuration environment variable mappings are now correct.The S3 and S3 cloud configuration mappings appear to have been fixed from previous review cycles. The environment variables now correctly map to their respective configuration fields.
95-95: SMTP password field name is correctly spelled.The field name has been corrected from the previous typo and now properly maps to
NC_SMTP_PASSWORD.
161-167: Configuration merging logic is now complete and correct.The
lodash.mergeoperation now properly merges the default configuration with the user-provided configuration, resolving the previous incomplete merge issue.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/nocodb/config/util.ts (1)
3-17: 🛠️ Refactor suggestionFix logic inconsistency in empty object check.
The initial check
Object.values(obj).length === 0doesn't align with the function's purpose. An object like{a: undefined, b: undefined}would haveObject.values(obj).length === 2but should returnfalsefor "occupied" since all values are undefined.-const objectOccupied = (obj: object) => { - if (typeof obj === 'undefined' || Object.values(obj).length === 0) { +const objectOccupied = (obj: object) => { + if (typeof obj === 'undefined' || Object.keys(obj).length === 0) { return false; }Using
Object.keys(obj).length === 0properly checks for truly empty objects{}, which aligns with the recursive logic below that checks for defined values.packages/nocodb/config/index.ts (3)
51-51:⚠️ Potential issueEnvironment variable naming inconsistency with PR objectives.
The PR objectives mention renaming
NC_WORKER_CONTAINERtoNC_WORKER_TYPE, but the code still uses the old name.- workerType: stringToBoolWorker(process.env.NC_WORKER_CONTAINER), + workerType: stringToBoolWorker(process.env.NC_WORKER_TYPE),This change is essential to align with the PR's stated objectives of updating environment variable naming.
119-141: 🛠️ Refactor suggestionAdd error handling for TOML file operations.
The function lacks error handling for file reading and TOML parsing, which can cause unhandled exceptions for invalid files or syntax errors.
const fromToml = (cfgPath: string = process.env.NC_CFG_PATH): object => { const defaultCfgPaths = ['./nocodb.toml', '/etc/nocodb.toml']; let tomlString: string; if (cfgPath) { - tomlString = fs.readFileSync(cfgPath, 'utf8'); + try { + tomlString = fs.readFileSync(cfgPath, 'utf8'); + } catch (error) { + throw new Error(`Failed to read config file ${cfgPath}: ${error.message}`); + } } else { for (const cfgPath of defaultCfgPaths) { if (!fs.existsSync(cfgPath)) { continue; } - tomlString = fs.readFileSync(cfgPath, 'utf8'); + try { + tomlString = fs.readFileSync(cfgPath, 'utf8'); + } catch (error) { + continue; // Try next path if this one fails + } break; } if (typeof tomlString === 'undefined') { return {}; } } - return toml.parse(tomlString); + try { + return toml.parse(tomlString); + } catch (error) { + throw new Error(`Failed to parse TOML configuration: ${error.message}`); + } };
177-178: 🛠️ Refactor suggestionClarify the conditional logic for externalDb configuration.
The current logic unconditionally overwrites
externalDbwhen it's truthy, which may not be the intended behavior.- if (mergedCfg.nocoDbConfig.externalDb) - mergedCfg.nocoDbConfig.externalDb = !mergedCfg.nocoDbConfig.minimalDb; + // Only disable externalDb if minimalDb is enabled + if (mergedCfg.nocoDbConfig.minimalDb) { + mergedCfg.nocoDbConfig.externalDb = false; + }This makes the intention clearer: when minimal database mode is enabled, external database connections should be disabled.
🧹 Nitpick comments (2)
packages/nocodb/config/util.ts (1)
33-48: Simplify unnecessary ternary operators.The static analysis correctly identifies that the ternary operators can be simplified for better readability.
if (s.toLowerCase() === 'true') { - return inverse ? false : true; + return !inverse; } else if (s.toLowerCase() === 'false') { - return inverse ? true : false; + return inverse; } else {This makes the logic more concise and eliminates unnecessary boolean literals in conditional expressions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 42-42: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 44-44: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/nocodb/config/index.ts (1)
162-162: Replace delete operator for better performance.Static analysis correctly identifies that the
deleteoperator can impact performance. Using undefined assignment is more efficient.- delete cliCfg['config']; + cliCfg['config'] = undefined;This change eliminates the performance impact of the delete operator while achieving the same result.
🧰 Tools
🪛 Biome (1.9.4)
[error] 162-162: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nocodb/config/index.ts(1 hunks)packages/nocodb/config/model.ts(1 hunks)packages/nocodb/config/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nocodb/config/model.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/nocodb/config/index.ts (2)
packages/nocodb/config/util.ts (3)
stringToBoolWorker(51-65)stringToBoolTry(33-48)rmUndefined(19-31)packages/nocodb/config/model.ts (1)
ServerConfig(117-155)
packages/nocodb/config/util.ts (1)
packages/nocodb/config/model.ts (1)
WorkerType(115-115)
🪛 Biome (1.9.4)
packages/nocodb/config/index.ts
[error] 162-162: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/nocodb/config/util.ts
[error] 42-42: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 44-44: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
- GitHub Check: bumper
🔇 Additional comments (2)
packages/nocodb/config/util.ts (1)
50-65: LGTM! Well-implemented backward compatibility function.The
stringToBoolWorkerfunction correctly handles the special case conversion for worker types, providing sensible defaults and maintaining backward compatibility.packages/nocodb/config/index.ts (1)
180-187: LGTM! Proper validation implementation.The validation logic correctly uses class-transformer and class-validator to ensure the configuration meets the defined schema requirements.
Change Summary
Currently we use different methods server configuration, set and and read env vars, config service, etc...
unify everything, provide toml configuration and cli flags
Change type