feat(app): support legacy env variables and adapt to new settings object #818
Conversation
WalkthroughConvert flat environment maps into a typed Settings object by deep-cloning a default skeleton and applying recognized env keys; add a comprehensive env test fixture; make logger.transport optional; update tests for camelCase outputs; adjust config loading to prefer env-derived settings when appropriate. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as flatEnv (Record<string,string|undefined>)
participant Conv as convertEnvToSettings()
participant Def as defaultSettings (skeleton)
participant Out as Settings (typed)
Env->>Conv: pass flat env map
Conv->>Def: cloneDeep(defaultSettings)
Conv->>Conv: iterate recognized KEYS -> convert & assign (camelCase, cast types, parse presets)
Conv-->>Out: return populated Settings object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🧹 Nitpick comments (2)
packages/tego/src/__tests__/env (2)
85-85: Typo:BULTIN→BUILTINSpelling error in
PRESETS_BULTIN_PLUGINSwill break env parsing or preset discovery.
Diff suggestion:-PRESETS_BULTIN_PLUGINS= +PRESETS_BUILTIN_PLUGINS=
80-88: Very long plugin lists may hit OS env-var length limitsLines 85-86 pack dozens of plugin names into single variables. In some shells/process managers (e.g., systemd with ~131 kB limit) this can truncate or fail. Consider moving to a config file or splitting across multiple smaller vars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tego/src/__tests__/env(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: test-coverage
| ################# TACHYBASE APPLICATION ################# | ||
| APP_ENV=development | ||
| APP_PORT=3000 | ||
| APP_KEY=test-key |
There was a problem hiding this comment.
Hard-coded secret committed
APP_KEY contains a real-looking key. Even in test fixtures, commit history persists and may leak into production workflows. Replace with an obvious placeholder (e.g. CHANGE_ME) or load from CI secrets.
🤖 Prompt for AI Agents
In packages/tego/src/__tests__/env at line 4, the APP_KEY is hard-coded with a
real-looking secret key, which risks leaking sensitive information through
commit history. Replace the current value with a clear placeholder like
CHANGE_ME or configure the test environment to load this key securely from CI
secrets instead of hard-coding it.
| INIT_APP_LANG=en-US | ||
| [email protected] | ||
| INIT_ROOT_PASSWORD=!Admin123. | ||
| INIT_ROOT_NICKNAME=Super Admin | ||
| INIT_ROOT_USERNAME=tachybase | ||
|
|
There was a problem hiding this comment.
Root account credentials exposed
INIT_ROOT_PASSWORD (and related user details) are in plain text. This is high-risk if the file ever ships outside test scope. Use dummy values or remove the variable entirely from VCS.
🤖 Prompt for AI Agents
In packages/tego/src/__tests__/env around lines 51 to 56, the root account
credentials including INIT_ROOT_PASSWORD are exposed in plain text, which is a
security risk. Replace the real password and related user details with dummy
placeholder values or remove these variables entirely from version control to
prevent accidental exposure outside the test environment.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/tego/src/__tests__/env (1)
51-56: Root credentials present in fixture — use unmistakably dummy valuesEven for fixtures, prefer obviously fake data to avoid reuse.
- Example: INIT_ROOT_EMAIL=[email protected]
- INIT_ROOT_PASSWORD=ChangeMe123!
- INIT_ROOT_USERNAME=admin
- INIT_ROOT_NICKNAME=Admin
This mirrors prior feedback.
🧹 Nitpick comments (11)
packages/tego/src/__tests__/utils.test.ts (1)
17-21: LGTM on camelCase expectations and core mappingsAssertions now align with the new structured settings output. Consider adding coverage for:
- LOGGER_TRANSPORT with spaces/duplicates ("console, dailyRotateFile, console") and case variations.
- Boolean normalization (DB_LOGGING='true'/'1', DB_UNDERSCORED='on').
- Unknown keys are ignored.
packages/tego/src/utils.ts (8)
231-238: LOGGER_MAX_FILES / LOGGER_MAX_SIZE: keep as strings but consider numeric fallbackYour type allows string | number (e.g., "14d", "10m"). Current assignment is fine. If a pure integer is provided, you could parse to number:
- If
^\d+$=> Number(value)- Else keep string
Optional; no change required.
236-238: LOGGER_FORMAT: support legacy alias 'splitter' -> 'delimiter'Env comments mention "splitter", but Settings uses 'delimiter'. Normalize to avoid confusion.
- case 'LOGGER_FORMAT': - settings.logger.format = value as any; + case 'LOGGER_FORMAT': { + const mapped = v === 'splitter' ? 'delimiter' : v; + settings.logger.format = mapped as any; + break; + }
251-252: DB_PORT: guard against NaNIf a non-numeric slips through, we’ll assign
NaN. Consider guarding:- case 'DB_PORT': - settings.database.port = +value; + case 'DB_PORT': { + const num = Number(v); + if (Number.isFinite(num)) settings.database.port = num; + break; + }
272-287: Avoid repeated object re-creation pattern for SSLMinor readability nit: you repeatedly assign
settings.database.ssl = settings.database.ssl || {}. Consider hoisting once, but current code is correct and safe.
321-327: Worker counts: maintain numeric guard (optional)Same NaN guard suggestion as DB_PORT applies to WORKER_COUNT and WORKER_COUNT_MAX. Optional.
330-335: Export settings numeric guard (optional)Guard against NaN for EXPORT_LENGTH_MAX and EXPORT_WORKER_PAGESIZE. Optional.
337-344: Unknown keys: good choice to ignore; consider logging in debug modeSilently ignoring is fine. Optionally log unknown keys when a DEBUG flag is set to help diagnose typos.
213-349: Minor: annotate function return type for clarityType inference yields
Settings, but explicit return type helps readers.-export function convertEnvToSettings(flatEnv: Record<string, string | undefined>) { +export function convertEnvToSettings( + flatEnv: Record<string, string | undefined>, +): Settings {packages/tego/src/__tests__/env (2)
21-22: LOGGER_FORMAT doc mentions "splitter" but type uses "delimiter"Docs/code mismatch. Either update the comment to "delimiter" or support aliasing (see utils.ts suggestion to map 'splitter' -> 'delimiter').
69-79: WORKER_COUNT_SUB / WORKER_COUNT_MAX_SUB are unusedThe converter ignores these keys. If sub-app worker settings are desired, add mappings; otherwise, remove from fixture to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/globals/src/settings.d.ts(1 hunks)packages/tego/src/__tests__/env(1 hunks)packages/tego/src/__tests__/utils.test.ts(1 hunks)packages/tego/src/utils.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Full Build
packages/tego/src/utils.ts
[error] 19-19: File '/home/runner/work/tego/tego/packages/tego/presets/settings.js' is not under 'rootDir' '/home/runner/work/tego/tego/packages/tego/src'. 'rootDir' is expected to contain all source files.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-coverage
- GitHub Check: tests
🔇 Additional comments (4)
packages/globals/src/settings.d.ts (1)
15-15: Makinglogger.transportoptional looks fine, but verify downstream assumptionsA few code paths likely assume
transportis always present (array). Ensure either:
defaultSettings.logger.transportprovides a default, or- Call sites handle
undefinedsafely.If both are true, we're good.
packages/tego/src/utils.ts (1)
306-319: External plugins: clarify!vs|semantics
I didn’t find any documentation or tests in the repo that define how!name(removal) and|name(add-but-disabled) should be handled for external plugins. Please confirm the intended behavior and update accordingly:
!name⇒ exclude the entry entirely|name⇒ include withenabledByDefault: false- (Optionally) add unit tests or documentation illustrating both cases
- Also confirm whether
PRESETS_BUILTIN_PLUGINSshould support the same prefixes (it currently just splits and enables all)Suggested implementation in
packages/tego/src/utils.ts:- case 'PRESETS_EXTERNAL_PLUGINS': - settings.presets.externalPlugins = value.split(',').map((name) => { - if (name.startsWith('!')) { - return { name: name.slice(1), enabledByDefault: false }; - } - if (name.startsWith('|')) { - return { name: name.slice(1), enabledByDefault: false }; - } - return { name, enabledByDefault: true }; - }); - break; + case 'PRESETS_EXTERNAL_PLUGINS': { + const items = value + .split(',') + .map((n) => n.trim()) + .filter(Boolean) + .map((name) => { + if (name.startsWith('!')) return null; // remove + const disabled = name.startsWith('|'); + const clean = disabled ? name.slice(1) : name; + return { name: clean, enabledByDefault: !disabled }; + }) + .filter((x): x is { name: string; enabledByDefault: boolean } => !!x); + settings.presets.externalPlugins = items; + break; + }packages/tego/src/__tests__/env (2)
4-4: APP_KEY uses a non-sensitive placeholderUsing
test-keyavoids leaking real secrets. Good.
62-66: Preset semantics in docs vs implementationDocs say:
- !name = remove
- |name = add but disabled
Current implementation previously treated both as disabled. Ensure the utils mapping aligns with the intended behavior (suggested code provided in utils.ts review).
| import npmRegistryFetch from 'npm-registry-fetch'; | ||
| import * as tar from 'tar'; | ||
|
|
||
| import defaultSettings from '../presets/settings'; |
There was a problem hiding this comment.
Fix TS build: avoid importing files outside rootDir (pipeline error)
The CI error shows TypeScript rejects importing ../presets/settings because it’s outside packages/tego/src. Two low-risk options:
- Re-export defaultSettings via an in-src shim and import from there (recommended).
- Adjust tsconfig rootDir/includes (higher blast radius).
Apply this diff here, plus add the small shim file below.
-import defaultSettings from '../presets/settings';
+import defaultSettings from './presets/settings';Add new file at packages/tego/src/presets/settings.ts:
// Re-export to satisfy tsc rootDir constraints
export { default } from '../../presets/settings';This resolves the CI error without changing build config.
🧰 Tools
🪛 GitHub Actions: Full Build
[error] 19-19: File '/home/runner/work/tego/tego/packages/tego/presets/settings.js' is not under 'rootDir' '/home/runner/work/tego/tego/packages/tego/src'. 'rootDir' is expected to contain all source files.
🤖 Prompt for AI Agents
In packages/tego/src/utils.ts around line 19, the file imports
../presets/settings which violates TypeScript rootDir constraints; change the
import to point to a new in-src shim (./presets/settings) and add a new file
packages/tego/src/presets/settings.ts that re-exports the original default
export from ../../presets/settings so tsc sees the module inside the package
root. Ensure the utils.ts import is updated to import defaultSettings from
'./presets/settings' and create the shim file that simply re-exports the default
export from the upstream presets path.
| for (const [key, value] of Object.entries(flatEnv)) { | ||
| if (!value) continue; | ||
|
|
||
| switch (key) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trim values before processing; skip only when empty after trim
Prevents accidental ignoring when values contain whitespace.
- for (const [key, value] of Object.entries(flatEnv)) {
- if (!value) continue;
+ for (const [key, value] of Object.entries(flatEnv)) {
+ if (value == null) continue;
+ const v = String(value).trim();
+ if (!v) continue;Follow-up: use v instead of value below.
📝 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.
| for (const [key, value] of Object.entries(flatEnv)) { | |
| if (!value) continue; | |
| switch (key) { | |
| for (const [key, value] of Object.entries(flatEnv)) { | |
| if (value == null) continue; | |
| const v = String(value).trim(); | |
| if (!v) continue; | |
| switch (key) { |
🤖 Prompt for AI Agents
In packages/tego/src/utils.ts around lines 216 to 219, the loop currently skips
entries based on truthiness of value which will miss non-empty strings
containing only whitespace; convert the entry to a trimmed string first (e.g.
const v = String(value).trim()) and change the guard to if (!v) continue; then
use v (not value) in the subsequent switch/processing so all whitespace-only
values are treated as empty.
| case 'LOGGER_TRANSPORT': | ||
| settings.logger.transport = value.split(',') as any; | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden LOGGER_TRANSPORT parsing (trim, dedupe, validate)
Trim tokens, dedupe, and ignore invalid entries to avoid type casting to any.
- case 'LOGGER_TRANSPORT':
- settings.logger.transport = value.split(',') as any;
+ case 'LOGGER_TRANSPORT': {
+ const allowed = new Set(['console', 'file', 'dailyRotateFile']);
+ const transports = Array.from(
+ new Set(
+ v
+ .split(',')
+ .map((t) => t.trim())
+ .filter((t) => allowed.has(t)),
+ ),
+ );
+ if (transports.length) {
+ settings.logger.transport = transports as any;
+ }
+ break;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tego/src/utils.ts around lines 221 to 223, the LOGGER_TRANSPORT
branch currently casts the split result to any; change it to robustly parse the
comma-separated string by splitting, trimming each token, filtering out empty
strings, deduplicating entries, and validating each token against the allowed
transport names (e.g., the enum or union type used for logger transports);
finally assign the resulting typed array (not any) to settings.logger.transport
so invalid entries are dropped and types are preserved.
| settings.database.logging = value === 'on'; | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DB_LOGGING: accept common truthy values ('on', 'true', '1', 'yes')
More forgiving parsing helps compatibility.
- case 'DB_LOGGING':
- settings.database.logging = value === 'on';
+ case 'DB_LOGGING': {
+ const truthy = new Set(['on', 'true', '1', 'yes']);
+ settings.database.logging = truthy.has(v.toLowerCase());
+ break;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tego/src/utils.ts around lines 263-264, the code currently sets
settings.database.logging only when value === 'on'; change it to accept common
truthy strings (case-insensitive) — e.g. 'on', 'true', '1', 'yes' — by
normalizing the value (toString(), trim(), toLowerCase()) and checking
membership in that set, then set settings.database.logging to true for matches
and false otherwise (handling undefined/null safely).
| settings.database.underscored = value === 'true'; | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DB_UNDERSCORED: normalize common boolean values
Follow same boolean normalization as DB_LOGGING.
- case 'DB_UNDERSCORED':
- settings.database.underscored = value === 'true';
+ case 'DB_UNDERSCORED': {
+ const truthy = new Set(['on', 'true', '1', 'yes']);
+ settings.database.underscored = truthy.has(v.toLowerCase());
+ break;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/tego/src/utils.ts around lines 269-270, the assignment
settings.database.underscored = value === 'true' only accepts the literal 'true'
string; replace it with the same boolean normalization used for DB_LOGGING
(i.e., normalize common boolean representations from the env value such as
'true'/'false', '1'/'0', 'yes'/'no' in a case-insensitive way) so underscored is
reliably set to true or false for those inputs; implement by reusing or
duplicating the DB_LOGGING normalization helper logic so value is converted to a
boolean before assignment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/tego/src/utils.ts (5)
207-209: Trim env values first; skip only when empty after trimPrevents silently accepting whitespace and improves robustness.
- for (const [key, value] of Object.entries(flatEnv)) { - if (!value) continue; + for (const [key, value] of Object.entries(flatEnv)) { + if (value == null) continue; + const v = String(value).trim(); + if (!v) continue;Follow-up: use v instead of value in the switch below.
212-214: LOGGER_TRANSPORT: trim, dedupe, and validate tokensAvoids invalid values and preserves types.
- case 'LOGGER_TRANSPORT': - settings.logger.transport = value.split(',') as any; - break; + case 'LOGGER_TRANSPORT': { + const allowed = new Set(['console', 'file', 'dailyRotateFile']); + const transports = Array.from( + new Set( + (typeof v === 'string' ? v : String(v)) + .split(',') + .map((t) => t.trim()) + .filter((t) => allowed.has(t)), + ), + ); + if (transports.length) { + settings.logger.transport = transports as any; + } + break; + }
254-255: DB_LOGGING: accept common truthy valuesMore compatible parsing.
- case 'DB_LOGGING': - settings.database.logging = value === 'on'; - break; + case 'DB_LOGGING': { + const truthy = new Set(['on', 'true', '1', 'yes']); + settings.database.logging = truthy.has(v.toLowerCase()); + break; + }
260-261: DB_UNDERSCORED: normalize booleans like DB_LOGGINGKeep boolean parsing consistent.
- case 'DB_UNDERSCORED': - settings.database.underscored = value === 'true'; - break; + case 'DB_UNDERSCORED': { + const truthy = new Set(['on', 'true', '1', 'yes']); + settings.database.underscored = truthy.has(v.toLowerCase()); + break; + }
19-19: Fix TS build: import inside rootDir via shim (matches CI failure)The pipeline fails because this import reaches outside rootDir. Use an in-src shim and import from there.
-import defaultSettings from '../presets/settings'; +import defaultSettings from './presets/settings';Add new file at packages/tego/src/presets/settings.ts:
// Re-export to satisfy tsc rootDir constraints export { default } from '../../presets/settings';This resolves the error:
"File 'packages/tego/presets/settings.js' is not under rootDir 'packages/tego/src'."
🧹 Nitpick comments (6)
packages/tego/src/utils.ts (6)
204-205: Add explicit return type for clarityTS can infer it, but an explicit return type helps maintainability and API stability.
-export function convertEnvToSettings(flatEnv: Record<string, string | undefined>) { +export function convertEnvToSettings(flatEnv: Record<string, string | undefined>): Settings {
276-278: DB_DIALECT_OPTIONS_SSL_REJECT_UNAUTHORIZED: normalize booleanAlign with other boolean keys.
- case 'DB_DIALECT_OPTIONS_SSL_REJECT_UNAUTHORIZED': - settings.database.ssl = settings.database.ssl || {}; - settings.database.ssl.rejectUnauthorized = value === 'true'; - break; + case 'DB_DIALECT_OPTIONS_SSL_REJECT_UNAUTHORIZED': { + settings.database.ssl = settings.database.ssl || {}; + const truthy = new Set(['on', 'true', '1', 'yes']); + settings.database.ssl.rejectUnauthorized = truthy.has(v.toLowerCase()); + break; + }
297-300: Accept both PRESETS_BULTIN_PLUGINS (legacy) and PRESETS_BUILTIN_PLUGINS; trim valuesKeeps backward compatibility and avoids whitespace issues.
- case 'PRESETS_BULTIN_PLUGINS': - settings.presets.builtinPlugins = value.split(','); - break; + case 'PRESETS_BULTIN_PLUGINS': + case 'PRESETS_BUILTIN_PLUGINS': { + settings.presets.builtinPlugins = v + .split(',') + .map((t) => t.trim()) + .filter(Boolean); + break; + }
301-309: PRESETS_EXTERNAL_PLUGINS: trim items and drop emptiesPrevents empty or whitespace-only entries.
- case 'PRESETS_EXTERNAL_PLUGINS': - settings.presets.externalPlugins = value.split(',').map((name) => { - if (name.startsWith('!')) { - return { name: name.slice(1), enabledByDefault: false }; - } - if (name.startsWith('|')) { - return { name: name.slice(1), enabledByDefault: false }; - } - return { name, enabledByDefault: true }; - }); - break; + case 'PRESETS_EXTERNAL_PLUGINS': { + settings.presets.externalPlugins = v + .split(',') + .map((raw) => { + const name = raw.trim(); + if (!name) return null; + if (name.startsWith('!') || name.startsWith('|')) { + return { name: name.slice(1), enabledByDefault: false }; + } + return { name, enabledByDefault: true }; + }) + .filter(Boolean) as any; + break; + }
329-331: FORBID_SUB_APP_PLUGINS: trim and drop emptiesMinor robustness improvement.
- case 'FORBID_SUB_APP_PLUGINS': - settings.misc.forbidSubAppPlugins = value.split(','); - break; + case 'FORBID_SUB_APP_PLUGINS': { + settings.misc.forbidSubAppPlugins = v + .split(',') + .map((t) => t.trim()) + .filter(Boolean); + break; + }
22-59: Consider loading TEGO_RUNTIME_HOME/.env in parseEnvironment for legacy supportCurrently, parseEnvironment only reads process.cwd()/.env, and TEGO_RUNTIME_HOME/.env is only loaded in constants.ts when settings.js is missing. To ensure that any legacy TEGO_RUNTIME_HOME/.env is always applied—regardless of the entry point—add this immediately after setting TEGO_RUNTIME_HOME:
// load legacy .env in TEGO_RUNTIME_HOME if present const runtimeEnvPath = resolve(process.env.TEGO_RUNTIME_HOME!, '.env'); if (_existsSync(runtimeEnvPath)) { config({ path: runtimeEnvPath }); }This optional refactor keeps environment‐loading behavior consistent across all initialization paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/devkit/src/util.ts(0 hunks)packages/tego/src/constants.ts(1 hunks)packages/tego/src/utils.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/devkit/src/util.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/tego/src/constants.ts (2)
packages/tego/src/utils.ts (2)
parseEnvironment(22-59)convertEnvToSettings(204-340)packages/devkit/src/constants.ts (1)
__dirname(5-5)
packages/tego/src/utils.ts (2)
packages/globals/src/index.ts (3)
settings(59-61)settings(62-64)Settings(69-69)packages/globals/src/settings.d.ts (1)
Settings(1-83)
🪛 GitHub Actions: Full Build
packages/tego/src/utils.ts
[error] 19-19: TypeScript error: File 'packages/tego/presets/settings.js' is not under rootDir 'packages/tego/src'. 'rootDir' is expected to contain all source files.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: test-coverage
🔇 Additional comments (2)
packages/tego/src/constants.ts (2)
5-5: Import of convertEnvToSettings looks correctBringing convertEnvToSettings into constants aligns with the new env-driven settings path.
22-22: Confirm and Cover .env vs settings.js Precedence in constants.ts
The current logic in packages/tego/src/constants.ts falls back to convertEnvToSettings whenever both a .env file and settings.js exist. Please ensure this is intentional and add tests to cover all three scenarios:• When only .env exists (no settings.js) → legacy settings.js is copied and loaded
• When only settings.js exists (no .env) → convertEnvToSettings() → settings.js ignored
• When both .env and settings.js exist → convertEnvToSettings() takes precedenceSuggested action:
- Create a new test suite in packages/tego/src/tests/constants.test.ts
- Mock process.env.TEGO_RUNTIME_HOME and fs.existsSync/require to simulate each scenario
- Assert that TachybaseGlobal.settings is loaded from the correct source in each case
| // 兼容旧的 .env 环境变量文件作为配置（仅在 settings.js 不存在时使用，且 .env 文件存在） | ||
| if ( | ||
| fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME, '.env')) && | ||
| !fs.existsSync(`${process.env.TEGO_RUNTIME_HOME}/settings.js`) | ||
| ) { | ||
| if (!fs.existsSync(`${process.env.TEGO_RUNTIME_HOME}/settings.js`)) { | ||
| fs.mkdirSync(`${process.env.TEGO_RUNTIME_HOME}`, { recursive: true }); | ||
| fs.copyFileSync(path.join(__dirname, '../presets/settings.js'), `${process.env.TEGO_RUNTIME_HOME}/settings.js`); | ||
| } | ||
| TachybaseGlobal.settings = require(`${process.env.TEGO_RUNTIME_HOME}/settings.js`); | ||
| } else { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Legacy .env path is not loaded; remove redundant existsSync and use path.join
The branch checks for TEGO_RUNTIME_HOME/.env but never loads it. Unless the generated settings.js itself loads TEGO_RUNTIME_HOME/.env, env values in that file will be ignored. Also, the inner existsSync check is redundant, and mixing template strings with filesystem paths is brittle.
Suggested inline refactor (loads runtime .env, removes the redundant check, and uses path.join consistently):
-if (
- fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME, '.env')) &&
- !fs.existsSync(`${process.env.TEGO_RUNTIME_HOME}/settings.js`)
-) {
- if (!fs.existsSync(`${process.env.TEGO_RUNTIME_HOME}/settings.js`)) {
- fs.mkdirSync(`${process.env.TEGO_RUNTIME_HOME}`, { recursive: true });
- fs.copyFileSync(path.join(__dirname, '../presets/settings.js'), `${process.env.TEGO_RUNTIME_HOME}/settings.js`);
- }
- TachybaseGlobal.settings = require(`${process.env.TEGO_RUNTIME_HOME}/settings.js`);
+if (
+ fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME!, '.env')) &&
+ !fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME!, 'settings.js'))
+) {
+ // Load legacy .env before generating settings.js so its values are available
+ dotenvConfig({ path: path.join(process.env.TEGO_RUNTIME_HOME!, '.env') });
+ fs.mkdirSync(process.env.TEGO_RUNTIME_HOME!, { recursive: true });
+ fs.copyFileSync(
+ path.join(__dirname, '../presets/settings.js'),
+ path.join(process.env.TEGO_RUNTIME_HOME!, 'settings.js'),
+ );
+ TachybaseGlobal.settings = require(path.join(process.env.TEGO_RUNTIME_HOME!, 'settings.js'));Add this import at the top of the file:
import { config as dotenvConfig } from 'dotenv';Would you confirm whether presets/settings.js actually loads TEGO_RUNTIME_HOME/.env by itself? If not, loading it here (as shown) ensures legacy env files take effect.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Locating presets/settings.js ==="
PRESET_FILE=$(fd -t f presets/settings.js || echo "")
if [[ -n "$PRESET_FILE" ]]; then
echo "Found presets/settings.js at: $PRESET_FILE"
echo "----- First 200 lines of presets/settings.js -----"
sed -n '1,200p' "$PRESET_FILE"
else
echo "presets/settings.js not found"
fi
echo
echo "=== Locating packages/tego/src/constants.ts ==="
CONST_FILE=$(fd -t f -p "constants.ts" | grep "packages/tego/src/constants.ts" || echo "")
if [[ -n "$CONST_FILE" ]]; then
echo "Found constants.ts at: $CONST_FILE"
echo "----- Lines 1-50 of constants.ts -----"
sed -n '1,50p' "$CONST_FILE"
else
echo "packages/tego/src/constants.ts not found"
fiLength of output: 2218
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Searching for any 'presets' directories ==="
fd -t d presets || echo "No 'presets' directory found"
echo
echo "=== Searching for settings.ts files ==="
fd -t f settings.ts || echo "No settings.ts files found"
echo
echo "=== Searching for settings.js files ==="
fd -t f settings.js || echo "No settings.js files found"
echo
echo "=== Showing contents of all settings.* files (first 200 lines) ==="
for f in $(fd -t f 'settings\.(ts|js)$'); do
echo "----- File: $f -----"
sed -n '1,200p' "$f"
echo
doneLength of output: 4891
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Locating packages/tego/src/utils.ts ==="
UTILS_FILE=$(fd -t f utils.ts | grep "packages/tego/src/utils.ts" || echo "")
if [[ -n "$UTILS_FILE" ]]; then
echo "Found utils.ts at: $UTILS_FILE"
echo "----- First 200 lines of utils.ts -----"
sed -n '1,200p' "$UTILS_FILE"
else
echo "packages/tego/src/utils.ts not found"
fiLength of output: 6534
Load legacy .env and streamline settings.js logic
The presets/settings.js file does not invoke dotenv, so legacy .env values under TEGO_RUNTIME_HOME are never applied. We should explicitly load that file before generating settings.js, remove the redundant inner existsSync check, and consistently use path.join.
• File: packages/tego/src/constants.ts (lines 11–21)
• Preserves the existing else branch that falls back to convertEnvToSettings.
Suggested changes:
- Add at the top of the file:
import { config as dotenvConfig } from 'dotenv';
- Replace the block starting at line 11 with:
-if ( - fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME, '.env')) && - !fs.existsSync(`${process.env.TEGO_RUNTIME_HOME}/settings.js`) -) { - if (!fs.existsSync(`${process.env.TEGO_RUNTIME_HOME}/settings.js`)) { - fs.mkdirSync(`${process.env.TEGO_RUNTIME_HOME}`, { recursive: true }); - fs.copyFileSync(path.join(__dirname, '../presets/settings.js'), `${process.env.TEGO_RUNTIME_HOME}/settings.js`); - } - TachybaseGlobal.settings = require(`${process.env.TEGO_RUNTIME_HOME}/settings.js`); +if ( + fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME!, '.env')) && + !fs.existsSync(path.join(process.env.TEGO_RUNTIME_HOME!, 'settings.js')) +) { + // Load legacy .env so its variables are available to the generated settings + dotenvConfig({ path: path.join(process.env.TEGO_RUNTIME_HOME!, '.env') }); + fs.mkdirSync(process.env.TEGO_RUNTIME_HOME!, { recursive: true }); + fs.copyFileSync( + path.join(__dirname, '../presets/settings.js'), + path.join(process.env.TEGO_RUNTIME_HOME!, 'settings.js'), + ); + TachybaseGlobal.settings = require(path.join(process.env.TEGO_RUNTIME_HOME!, 'settings.js')); }
This ensures legacy environment variables under TEGO_RUNTIME_HOME/.env are loaded before the fallback settings file is generated.
🤖 Prompt for AI Agents
In packages/tego/src/constants.ts around lines 11 to 21, the logic that
generates a settings.js from presets when TEGO_RUNTIME_HOME/.env exists fails to
load legacy .env values and contains a redundant inner existsSync; import dotenv
and explicitly load the .env file at TEGO_RUNTIME_HOME before creating/copying
settings.js, remove the duplicate fs.existsSync check, and consistently use
path.join for paths (create the runtime dir if needed, copy presets/settings.js
into path.join(process.env.TEGO_RUNTIME_HOME, 'settings.js'), then require that
file into TachybaseGlobal.settings).
Summary by CodeRabbit
Chores
Refactor
Tests