Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions app/eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,36 @@ export default [
},
},

// Frontend config is centralized in src/utils/config.ts (AGENTS.md). A direct
// import.meta.env read elsewhere bypasses the derived values that file owns --
// IS_DEV_LIKE exists precisely because import.meta.env.DEV is false under the
// E2E harness (vite build --mode development) -- and cannot be stubbed once the
// module graph has loaded, which is why the loopback OAuth test reads config
// instead. The rule was documented but unenforced, and had already drifted.
{
files: ['src/**/*.ts', 'src/**/*.tsx'],
ignores: [
'src/utils/config.ts',
'src/test/**',
'src/**/__tests__/**',
'src/**/*.test.ts',
'src/**/*.test.tsx',
],
rules: {
'no-restricted-syntax': [
'error',
{
// Pin the meta-property to `import.meta` -- `new.target` is a MetaProperty
// too -- and match both `import.meta.env` and `import.meta['env']`.
selector:
'MemberExpression[object.type="MetaProperty"][object.meta.name="import"][object.property.name="meta"]:matches([computed=false][property.name="env"], [computed=true][property.value="env"])',
message:
'Read frontend config from src/utils/config.ts (IS_DEV, IS_DEV_LIKE, ...) instead of import.meta.env directly; add the value there if it is missing.',
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
],
},
},

// React files configuration
{
files: ['src/**/*.jsx', 'src/**/*.tsx'],
Expand Down
3 changes: 2 additions & 1 deletion app/src/components/layout/TwoPanelLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,14 @@ import {
setSidebarWidth,
toggleSidebar,
} from '../../store/layoutSlice';
import { IS_DEV } from '../../utils/config';
import { Button } from '../ui';
import { clampWidth, useResizableDivider } from './useResizableDivider';

const namespace = 'two-panel-layout';

function debug(message: string, payload?: Record<string, unknown>) {
if (import.meta.env.DEV) {
if (IS_DEV) {
console.debug(`[${namespace}] ${message}`, payload ?? {});
}
}
Expand Down
6 changes: 3 additions & 3 deletions app/src/services/socketService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { setBackend } from '../store/connectivitySlice';
import { resetForUser, setSocketIdForUser, setStatusForUser } from '../store/socketSlice';
import type { ChannelAuthMode, ChannelConnectionStatus, ChannelType } from '../types/channels';
import type { UserErrorScope } from '../types/userError';
import { IS_DEV } from '../utils/config';
import { IS_DEV, IS_TEST } from '../utils/config';
import { createSafeLogData, sanitizeError } from '../utils/sanitize';
import { getCoreRpcToken, getCoreRpcUrl } from './coreRpcClient';
import { createCoreSocket } from './coreSocket';
Expand All @@ -26,11 +26,11 @@ const socketWarn = debug('socket:warn');
const socketError = debug('socket:error');

// Enable socket logging in development by default — but never under test.
// `IS_DEV` is truthy in vitest, so without the MODE guard this force-enable
// `IS_DEV` is truthy in vitest, so without the `IS_TEST` guard this force-enable
// floods every test file that imports this service (measured: 412 lines /
// 46KB of `flow:approval_request` listener churn in a single run), inflating
// runtime enough to push suites past the runner's foreground timeout.
if (IS_DEV && import.meta.env.MODE !== 'test') {
if (IS_DEV && !IS_TEST) {
debug.enable('socket*');
}

Expand Down
70 changes: 70 additions & 0 deletions app/src/test/eslintCentralizedConfigRule.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import tsparser from '@typescript-eslint/parser';
import { Linter } from 'eslint';
import { describe, expect, it } from 'vitest';

// eslint.config.js is plain JS and ships no declarations; importing the real
// file is the point of this test, so the shape is asserted below at runtime.
// @ts-expect-error -- untyped JS module
import eslintConfig from '../../eslint.config.js';

/**
* Guards the `no-restricted-syntax` selector that keeps frontend config reads
* funnelled through `src/utils/config.ts`. The selector is easy to get subtly
* wrong in both directions -- `new.target` is a MetaProperty just like
* `import.meta`, and computed access stores the key on `property.value` rather
* than `property.name` -- so the exact boundary is pinned here.
*
* The selector and the ignore list are read out of the real `eslint.config.js`
* rather than restated, so these assertions cannot drift away from what ships.
* Linting runs through a bare `Linter` because the repo config is type-aware
* and `parserOptions.project` rejects the synthetic file used for the probes.
*/

const configBlock = (eslintConfig as Linter.Config[]).find(
block => block.rules?.['no-restricted-syntax']
);

const [, restriction] = (configBlock?.rules?.['no-restricted-syntax'] ?? []) as [
string,
{ selector: string; message: string },
];

const linter = new Linter();

function lint(code: string) {
const messages = linter.verify(code, {
languageOptions: { parser: tsparser, ecmaVersion: 'latest', sourceType: 'module' },
rules: { 'no-restricted-syntax': ['error', restriction] },
});
// A parse failure would otherwise read as "the rule did not fire".
expect(messages.filter(message => message.fatal)).toEqual([]);
return messages;
}

describe('centralized-frontend-config lint rule', () => {
it('is wired into the shipped config', () => {
expect(restriction?.selector).toBeTypeOf('string');
expect(configBlock?.ignores).toContain('src/utils/config.ts');
expect(configBlock?.ignores).toContain('src/test/**');
});

it('rejects dotted import.meta.env access', () => {
expect(lint('export const a = import.meta.env.DEV;')).toHaveLength(1);
});

it('rejects computed import.meta["env"] access', () => {
expect(lint("export const a = import.meta['env'].DEV;")).toHaveLength(1);
});

it('allows other import.meta properties', () => {
expect(lint('export const a = import.meta.url;')).toHaveLength(0);
});

it('allows new.target.env, which is a different meta-property', () => {
expect(lint('export function G() { return new.target.env; }')).toHaveLength(0);
});

it('allows a plain object property named env', () => {
expect(lint('const o = { env: 1 }; export const a = o.env;')).toHaveLength(0);
});
});
1 change: 1 addition & 0 deletions app/src/test/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ vi.mock('../utils/config', () => ({
CORE_RPC_TIMEOUT_MS: 30_000,
IS_DEV: true,
IS_DEV_LIKE: true,
IS_TEST: true,
IS_PROD: false,
E2E_DEFAULT_CORE_MODE: '',
E2E_RESTART_APP_AS_RELOAD: false,
Expand Down
7 changes: 7 additions & 0 deletions app/src/utils/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ export const E2E_DEFAULT_CORE_MODE =
*/
export const IS_DEV_LIKE = IS_DEV || import.meta.env.MODE === 'development';

/**
* True under vitest. Distinct from `IS_DEV`, which is *also* true there — so a
* dev-only default that must stay off in tests needs both, and reading `MODE`
* at the call site is exactly the direct access this module exists to absorb.
*/
export const IS_TEST = import.meta.env.MODE === 'test';

/** Dev only: skip `.skip_onboarding` workspace check and ignore onboarded state so `/onboarding` always shows. Set `VITE_DEV_FORCE_ONBOARDING=true` in `.env.local`. */
export const DEV_FORCE_ONBOARDING =
import.meta.env.DEV && import.meta.env.VITE_DEV_FORCE_ONBOARDING === 'true';
Expand Down
Loading