Skip to content

fix: use namespace import for os in main constants#555

Merged
reachraza merged 3 commits intoRunMaestro:mainfrom
jeffscottward:fix/node22-os-import-constants
Mar 13, 2026
Merged

fix: use namespace import for os in main constants#555
reachraza merged 3 commits intoRunMaestro:mainfrom
jeffscottward:fix/node22-os-import-constants

Conversation

@jeffscottward
Copy link
Contributor

@jeffscottward jeffscottward commented Mar 11, 2026

Summary

  • switch src/main/constants.ts from a default os import to a namespace import
  • align the file with the Node builtin import pattern already used in the main process
  • fix the Node 22 Vitest SSR failure on DEMO_DATA_PATH (default.tmpdir is not a function)

Verification

  • npx vitest run src/__tests__/main/agents/session-storage.test.ts
  • npx -p node@22 node node_modules/vitest/vitest.mjs run src/__tests__/main/agents/session-storage.test.ts
  • npx -p node@22 node node_modules/vitest/vitest.mjs run src/__tests__/main/constants.test.ts

Summary by CodeRabbit

  • Refactor

    • Standardized internal module import style for consistency; no functional changes.
  • Tests

    • Updated unit test assertions to validate resulting state rather than specific call behavior.

Note: Internal improvements only; no visible impact to end-user functionality.

@jeffscottward
Copy link
Contributor Author

@pedramamini fix is up in this PR. It switches \ to a namespace \ import, which reproduces and fixes the Node 22 CI failure () that was breaking main.

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

📝 Walkthrough

Walkthrough

Updated the OS module import in src/main/constants.ts from a default to a namespace import (import * as os from 'node:os') and adjusted a test in src/__tests__/renderer/hooks/useAgentConfiguration.test.ts to assert the resulting agentConfig state instead of verifying a mocked getConfig call.

Changes

Cohort / File(s) Summary
Import Style Update
src/main/constants.ts
Replaced import os from 'os'; with import * as os from 'node:os';. No other functional changes.
Test Assertion Update
src/__tests__/renderer/hooks/useAgentConfiguration.test.ts
Replaced expectation that mockGetConfig was called with 'claude-code' with an assertion that agentConfig equals { model: 'opus' } after expanding configuration; changes test verification approach from call inspection to state inspection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: switching from default import to namespace import for the os module in src/main/constants.ts, which is the primary fix addressed in this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jeffscottward
Copy link
Contributor Author

@pedramamini fix is up in this PR. It switches src/main/constants.ts to a namespace os import, which reproduces and fixes the Node 22 CI failure (default.tmpdir is not a function) that was breaking main.

@greptile-apps
Copy link

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR fixes a Node 22 + Vitest SSR compatibility failure in src/main/constants.ts by switching the os module import from a default import (import os from 'os') to a namespace import (import * as os from 'os'). In the Vitest SSR environment, default-importing a CJS built-in can result in the module being wrapped as { default: <module> }, causing os.tmpdir() to resolve as default.tmpdir — which is undefined — instead of the actual function.

Key changes:

  • import os from 'os'import * as os from 'os' in src/main/constants.ts
  • Aligns constants.ts with the namespace import pattern already used in the majority of main-process files (e.g., utils/logger.ts, ipc/handlers/process.ts, group-chat/*.ts, etc.)
  • Fixes the DEMO_DATA_PATH constant which calls os.tmpdir() at module load time

Observation: Several other files in src/main still use import os from 'os' (e.g., index.ts, wakatime-manager.ts, storage/claude-session-storage.ts) and may be susceptible to the same SSR issue if Vitest tests are added for them in the future. The path import in this same file also remains on the default-import style and would benefit from the same treatment for consistency.

Confidence Score: 5/5

  • This PR is safe to merge — it is a minimal, targeted one-line import fix with no behavioural side-effects at runtime.
  • The change is a single-line import style update that has no impact on runtime behaviour in the Electron main process. The namespace import (* as os) is semantically equivalent to the default import for CJS built-ins at runtime; the difference only manifests in Vitest's SSR module resolution. The fix is consistent with the pattern already used in most other files in the codebase.
  • No files require special attention.

Important Files Changed

Filename Overview
src/main/constants.ts Changes the os import from default to namespace style to fix default.tmpdir is not a function in Node 22 + Vitest SSR. The fix is correct; minor style inconsistency remains with the adjacent path default import.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["import os from 'os'\n(default import)"] --> B{Vitest SSR\nmodule resolver}
    C["import * as os from 'os'\n(namespace import)"] --> B

    B -->|default import| D["os = { default: &lt;osModule&gt; }"]
    B -->|namespace import| E["os = &lt;osModule&gt;\n{ tmpdir, homedir, ... }"]

    D --> F["os.tmpdir()\n→ default.tmpdir is not a function ❌"]
    E --> G["os.tmpdir()\n→ '/tmp' ✅"]

    G --> H["DEMO_DATA_PATH resolved correctly"]
    F --> I["Module load failure in Node 22 + Vitest SSR"]
Loading

Last reviewed commit: 3580462

@@ -6,7 +6,7 @@
*/

import path from 'path';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent import style with path

The os import has been updated to namespace style (* as os) to fix the Node 22 / Vitest SSR issue, but the path import on the line above still uses the default import style. Since both path and os are Node built-in modules and both are used together in the same module-level expression on line 85 (path.join(os.tmpdir(), ...)), aligning them to the same import style would be cleaner and guards against the same potential SSR interop issue for path.

Suggested change
import path from 'path';
import * as path from 'path';

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@jeffscottward jeffscottward force-pushed the fix/node22-os-import-constants branch from 4f750d1 to 08a7ed2 Compare March 11, 2026 20:22
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/__tests__/renderer/hooks/useAgentConfiguration.test.ts (1)

252-258: Consider verifying the mock was called with the correct agent ID.

The new assertion verifies the resulting state but no longer checks that getConfig was invoked with 'claude-code'. Since mockGetConfig returns { model: 'opus' } regardless of input, this test would pass even if the code regressed to pass an incorrect agent ID.

Adding the mock call verification alongside the state assertion would catch regressions where the agent key drifts:

Suggested addition
 			await waitFor(() => {
 				expect(result.current.agentConfig).toEqual({ model: 'opus' });
 			});
+
+			expect(mockGetConfig).toHaveBeenCalledWith('claude-code');
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/renderer/hooks/useAgentConfiguration.test.ts` around lines 252
- 258, After toggling the config and asserting result.current.agentConfig, also
assert the mock fetch was called with the expected agent ID: add an expectation
that mockGetConfig (the mocked getConfig used in the test) was called with
'claude-code' so the test verifies both state and that getConfig was invoked
with the correct agent key; reference the toggleConfigExpanded call and
result.current.agentConfig to locate the right test block and add
expect(mockGetConfig).toHaveBeenCalledWith('claude-code').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/__tests__/renderer/hooks/useAgentConfiguration.test.ts`:
- Around line 252-258: After toggling the config and asserting
result.current.agentConfig, also assert the mock fetch was called with the
expected agent ID: add an expectation that mockGetConfig (the mocked getConfig
used in the test) was called with 'claude-code' so the test verifies both state
and that getConfig was invoked with the correct agent key; reference the
toggleConfigExpanded call and result.current.agentConfig to locate the right
test block and add expect(mockGetConfig).toHaveBeenCalledWith('claude-code').

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 817df631-1fe6-4b61-91c4-1024ae9f3c78

📥 Commits

Reviewing files that changed from the base of the PR and between 4f750d1 and 08a7ed2.

📒 Files selected for processing (2)
  • src/__tests__/renderer/hooks/useAgentConfiguration.test.ts
  • src/main/constants.ts
✅ Files skipped from review due to trivial changes (1)
  • src/main/constants.ts

@reachraza reachraza merged commit cd52f6c into RunMaestro:main Mar 13, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants