Skip to content

fix: break 3-file import cycle in cli/project-files - #1170

Open
AbhijitK20 wants to merge 1 commit into
CodebuffAI:mainfrom
AbhijitK20:fix-import-cycle-v2
Open

fix: break 3-file import cycle in cli/project-files#1170
AbhijitK20 wants to merge 1 commit into
CodebuffAI:mainfrom
AbhijitK20:fix-import-cycle-v2

Conversation

@AbhijitK20

Copy link
Copy Markdown

Summary

Breaks the circular import dependency between project-files.ts, auth.ts, and logger.ts.

The cycle:

project-files.ts → auth.ts → logger.ts → project-files.ts

project-files.ts imported getConfigDir from ./utils/auth, which imports logger, which imports getCurrentChatDir and getProjectRoot back from ../project-files — creating a 3-file circular dependency.

The fix: Import getConfigDir directly from ./utils/config-dir, which has no dependency on auth.ts, logger.ts, or project-files.ts.

Maintainer Feedback Addressed

  1. Same signature? Yes — config-dir.ts:14 exports the identical getConfigDir function. auth.ts:37 is a thin wrapper: export const getConfigDir = (): string => getConfigDirBase(). No behavior change.

  2. Does the cycle cause a real problem? Yes — circular dependencies can cause undefined values at import time depending on module load order, especially in bundlers or when tree-shaking.

  3. Test coverage — Added two tests for getProjectDataDir in project-files-chat-id.test.ts to exercise the fixed import path.

Notes

  • anonymous-id.ts already imports getConfigDir directly from config-dir (this PR aligns project-files.ts with that pattern)
  • No functional change — getConfigDir returns the same value either way

@codebuff-team

Copy link
Copy Markdown
Contributor

Good, focused change. Importing getConfigDir directly from ./utils/config-dir instead of via the auth.ts re-export is a sensible way to break the project-files.ts -> auth.ts -> logger.ts -> project-files.ts cycle, and it matches the existing pattern in anonymous-id.ts. Since auth.ts:37 is just a thin re-export of the same function, this is a pure refactor with no behavior change.

One nitpick: the test file references .config/manicode in a regex assuming a specific config dir name/format — verify that matches config-dir.ts's actual implementation for all platforms (e.g., macOS may use a different base path via env vars or XDG overrides). If config-dir.ts supports overriding the directory via env var, this test could be flaky in CI depending on environment. Otherwise, the tests are a nice touch for a one-line fix like this and specifically target the import path that was fixed.

Worth porting as long as the test assertions are validated against the actual config dir resolution logic across platforms/CI.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Sep 1, 2026
project-files.ts -> auth.ts -> logger.ts -> project-files.ts

Import getConfigDir directly from config-dir instead of auth to
break the circular dependency. config-dir.ts exports the identical
function (auth.ts just wraps it). anonymous-id.ts already imports
from config-dir directly.

Add getProjectDataDir tests to exercise the fixed import path.
@AbhijitK20

Copy link
Copy Markdown
Author

Thanks for the review and the "worth porting" label!

the test file references .config/manicode in a regex assuming a specific config dir name/format — verify that matches config-dir.ts's actual implementation for all platforms

Fixed. The test now imports getConfigDir from ./utils/config-dir directly and asserts expect(dataDir).toContain(configDir) instead of hardcoding the path regex. This is immune to different home directories, non-prod env suffixes (manicode-dev), or any future changes to config-dir.ts.

Verified config-dir.ts:14-24 — it's hardcoded as os.homedir() + '/.config/manicode' with no XDG or env var overrides, so the old regex would have worked too. But the dynamic assertion is more robust regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants