Cyclic dependency #1000
Cyclic dependency #1000
Conversation
shivamashtikar
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu and
zereraz
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds madge-based circular-dependency checks and unsafe script variants to frontend and server package scripts; extracts PgBoss instantiation to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Runner as Script Runner
participant Madge as madge (check-deps)
participant App as App (Vite / Node)
Dev->>Runner: run "dev" or "build"
Runner->>Madge: run check-deps
alt no cycles
Madge-->>Runner: success (0)
Runner->>App: start vite/node build/server
App-->>Dev: running
else cycles found
Madge-->>Runner: fail (non-zero)
Runner-->>Dev: exit with error
end
note right of Runner: "dev:unsafe" / "build:unsafe" skip Madge and start App directly
sequenceDiagram
autonumber
participant App as Application code
participant VespaService as sharedVespaService
participant Vespa as Vespa backend
App->>VespaService: call search/insert/update helpers
VespaService->>Vespa: forward requests to Vespa endpoint
Vespa-->>VespaService: responses
VespaService-->>App: return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code graph analysis (1)server/search/utils.ts (1)
⏰ 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). (1)
🔇 Additional comments (9)
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. Comment |
Summary of ChangesHello @shivamashtikar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on improving the architectural health of the application by addressing and preventing cyclic dependencies. It involves significant refactoring to decouple modules, centralize shared instances, and introduce a robust static analysis tool to enforce a cleaner dependency graph. The changes are intended to enhance code modularity, reduce duplication, and improve testability and reusability without altering existing application behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses cyclic dependencies by refactoring the codebase into more modular pieces, such as creating boss.ts for the PgBoss instance and extractors.ts for utility functions. The introduction of madge for automated circular dependency checks is a commendable improvement for maintaining code health. However, the review identified a few significant issues that need attention. There's a critical security risk due to hardcoded database credentials. Additionally, a critical logic bug was found in the refactored extractDriveIds function that could lead to incorrect behavior when processing Google Drive items. I've also included a couple of medium-severity suggestions to improve type safety and code clarity.
shivamashtikar
force-pushed
the
cyclic-dependency
branch
from
1b8f052 to
3f506bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/package.json (1)
36-40: Strengthen madge config: resolve TS paths and ignore build outputs.Add tsconfig resolution and exclude dist/build to avoid false positives and slow scans.
- "check-deps": "madge --circular --extensions ts,tsx ./", + "check-deps": "madge --circular --extensions ts,tsx --ts-config tsconfig.json --exclude '(^|/)(dist|build|coverage)/' .",frontend/package.json (1)
7-11: Have madge respect TS path aliases to reduce false positives.Pass tsconfig to madge; you’re already scoping to src, so no exclude needed here.
- "check-deps": "madge --circular --extensions ts,tsx ./src", + "check-deps": "madge --circular --extensions ts,tsx --ts-config tsconfig.json ./src",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/bun.lockbis excluded by!**/bun.lockbserver/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (2)
frontend/package.json(2 hunks)server/package.json(2 hunks)
🔇 Additional comments (1)
frontend/package.json (1)
8-11: LGTM on dev/build gating with unsafe escape hatches.The pre-check on dev/build and unsafe variants are balanced and practical.
If dev becomes noticeably slower, consider documenting when to use the unsafe variants in CONTRIBUTING.md.
shivamashtikar
force-pushed
the
cyclic-dependency
branch
from
171a9ff to
6e17514
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/search/utils.ts (1)
74-117: Harden extractDriveIds: guard undefined selectedItem, tighten null checks, improve logging.
- Possible TypeError: options.selectedItem may be undefined; current access isn’t safe.
- Add defensive checks around folderItem shape and docId presence.
- Log the error object + context to aid debugging.
Apply this diff:
@@ let driveItem: string[] = [] - if ((options.selectedItem as any)[Apps.GoogleDrive]) { - driveItem = [...(options.selectedItem as any)[Apps.GoogleDrive]] - } + const selected = (options as any).selectedItem + if (selected?.[Apps.GoogleDrive]) { + driveItem = [...(selected[Apps.GoogleDrive] as string[])] + } @@ - const folderItem = await sharedVespaService.getFolderItems( + const folderItem = await sharedVespaService.getFolderItems( [curr], fileSchema, DriveEntity.Folder, email, ) - if ( - folderItem.root && - folderItem.root.children && - folderItem.root.children.length > 0 - ) { - for (const item of folderItem.root.children) { + const children = folderItem?.root?.children ?? [] + if (children.length > 0) { + for (const item of children) { if ( - item.fields && - (item.fields as any).entity === DriveEntity.Folder + item?.fields && + (item.fields as any).entity === DriveEntity.Folder ) { - driveItem.push((item.fields as any).docId) + const id = (item.fields as any).docId + if (id) driveItem.push(id) } else { - driveIds.push((item.fields as any).docId) + const id = (item.fields as any).docId + if (id) driveIds.push(id) } } } } catch (error) { - Logger.error("failed to fetch drive items") + Logger.error({ err: error, curr, email }, "failed to fetch drive items") }
🧹 Nitpick comments (1)
server/search/vespaService.ts (1)
50-51: Add a small factory for testability (non-breaking).A helper to create isolated instances with overrides makes unit/integration tests and future multi‑tenant scenarios easier, while keeping the shared singleton for app code.
// Create a single shared vespa service instance export const sharedVespaService = createVespaService(dependencies) + +// Factory for tests/overrides without touching the app-wide singleton. +export const createTestVespaService = ( + overrides: Partial<VespaDependencies> = {}, +) => createVespaService({ ...dependencies, ...overrides })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/bun.lockbis excluded by!**/bun.lockbserver/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (8)
frontend/package.json(2 hunks)server/package.json(2 hunks)server/queue/boss.ts(1 hunks)server/queue/index.ts(1 hunks)server/queue/toolSync.ts(0 hunks)server/search/utils.ts(2 hunks)server/search/vespa.ts(1 hunks)server/search/vespaService.ts(1 hunks)
💤 Files with no reviewable changes (1)
- server/queue/toolSync.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/queue/boss.ts
- server/queue/index.ts
- server/search/vespa.ts
- frontend/package.json
- server/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
server/search/vespaService.ts (2)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)server/config.ts (2)
NAMESPACE(15-15)CLUSTER(16-16)
server/search/utils.ts (1)
server/search/vespaService.ts (1)
sharedVespaService(51-51)
🔇 Additional comments (2)
server/search/utils.ts (1)
19-19: Consolidation to shared service looks good.Nice step toward removing cycles and improving reuse. Please confirm madge still reports no cycles with this new import path.
server/search/vespaService.ts (1)
22-30: createDefaultConfig and dependencies usage verified
createDefaultConfig from @xyne/vespa-ts v1.0.5 accepts a Partial and your overrides match its expected shape. VespaDependencies require logger, config, vespaEndpoint, and optional sourceSchemas, which align with your implementation.
- Introduce `check-deps` script using madge to detect circular imports. - Wire `check-deps` into the default `dev` and `build` npm scripts. - Add `check-deps:unsafe` variants that skip the circular dependency check for optional bypass.
- Extract PgBoss queue client into `boss.ts` and replace duplicated configuration in `queue/index.ts` and other files with a single exported instance. - Create `vespaService.ts` that provides a shared Vespa service, imported by `search/vespa.ts` and `search/utils.ts`. - Remove repeated configuration and schema imports across modules. - Simplify dependency management and reduce code duplication throughout the server.
- Replace placeholder DB URL in config.ts with real Postgres connection string - Expose getDatabaseUrl() to provide the connection string - Update boss.ts to use getDatabaseUrl() instead of hard‑coded URL - Keeps queue configuration DRY and provides a single source of truth for the DB URL
shivamashtikar
force-pushed
the
cyclic-dependency
branch
from
6e17514 to
07ee974
Compare
4 participants
Description
index.tstoboss.tsboss.tsmodulesearch/utils.tsintoextractors.tsutils.tsextractDriveIdsto accept agetFolderItemscallback to avoid circular importscheck-depsscript using madge to detect circular imports.check-depsinto the defaultdevandbuildnpm scripts.check-deps:unsafevariants that skip the circular dependency check for optional bypass.Testing
Additional Notes
Summary by CodeRabbit
Chores
Refactor