Repository navigation
fix: core library fixes for Trigger.dev compatibility - #87
Merged
Merged
Conversation
- Remove dotenv auto-load and process.exit from env.ts (#78) - Add useGit option to walkFiles for non-git directories (#79) - Extend store.ts for multi-tenant Qdrant: repo_id, content, deterministic IDs, filters, payload index (#80) - Remove dotenv dependency from @codeindexer/core - 133 tests passing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved dotenv auto-loading and process.exit from env.ts (now throws on validation); added a configurable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/core/src/lib/walker.ts (1)
66-72: Consider whetherdot: falseintentionally excludes dotfiles.With
dot: false, fast-glob will skip dotfiles like.eslintrc,.prettierrc, and.github/workflows. This may be intentional to match typicalgit ls-filesbehavior with standard.gitignorepatterns, but it differs from whatgit ls-fileswould return for tracked dotfiles.If dotfiles should be discoverable in non-git directories, consider changing to
dot: true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/walker.ts` around lines 66 - 72, The fast-glob call that builds files (const files = await fg('**/*', { cwd: rootDir, ignore: FALLBACK_IGNORE, dot: false, absolute: false })) currently sets dot: false which excludes dotfiles and dot-directories; decide whether dotfiles should be discoverable and if so change the fg option to dot: true (or make it configurable) so functions using fg/Files discovery will include .eslintrc/.github etc.; update any tests or callers relying on files to account for dotfile inclusion and keep FALLBACK_IGNORE as-is unless you need to add explicit ignores for dot directories.packages/core/src/lib/store.ts (2)
198-218: Clarify the behavior whenrepoIdis provided with emptyidsarray.When
repoIdis provided andidsis empty, this will delete all points for that repository. While this is the intended design for bulk repo deletion, it's worth documenting this behavior to prevent accidental data loss.Consider adding a JSDoc comment to clarify:
/** * Delete points from Qdrant. * `@param` ids - Point IDs to delete (ignored if repoId is provided) * `@param` repoId - If provided, deletes ALL points for this repo (ignores ids) */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/store.ts` around lines 198 - 218, Add a JSDoc comment above the deletePoints function clarifying the semantics when repoId is provided (ids are ignored and an empty ids array plus repoId will delete all points for that repo), documenting parameters and the intended behavior; reference the deletePoints function name and mention that the implementation uses COLLECTION_NAME, qdrantRequest and log so maintainers understand this performs a bulk delete by repo when repoId is supplied.
154-159: Consider adding error handling for payload index creation.The payload index creation doesn't check for failure. If the PUT request fails (e.g., network error, invalid request),
ensureCollectionwill silently succeed without the index, potentially causing slow filtered queries.🛡️ Proposed fix to handle index creation errors
log.info(`Created collection "${COLLECTION_NAME}" (${dimension} dimensions, cosine distance)`); // Create payload index on repo_id for multi-tenant filtering - await qdrantRequest(`/collections/${COLLECTION_NAME}/index`, 'PUT', { + const { status: indexStatus, data: indexData } = await qdrantRequest(`/collections/${COLLECTION_NAME}/index`, 'PUT', { field_name: 'repo_id', field_schema: 'keyword', }); + + if (indexStatus !== 200) { + log.warn(`Failed to create repo_id payload index: ${JSON.stringify(indexData)}`); + } else { + log.info(`Created payload index on "repo_id" field`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/store.ts` around lines 154 - 159, ensureCollection's creation of the payload index currently ignores failures; wrap the qdrantRequest call that PUTs the repo_id index in a try/catch, and validate the response (or qdrantRequest's resolved value/status) to detect non-2xx outcomes—on failure log a clear error (include COLLECTION_NAME and the error/response), and either throw the error or return a failed Promise so callers know index creation failed; modify the call site where qdrantRequest is used (the qdrantRequest(...) that creates the index for COLLECTION_NAME in ensureCollection) to implement this handling.packages/core/src/lib/store.test.ts (1)
169-195: Consider adding tests for the newrepoIdfilter parameter.The existing tests cover the backward-compatible behavior (no
repoId), but there's no test coverage for:
deletePoints([], 'repo-123')— deleting by repo filtersearchPoints(vector, 10, 'repo-123')— searching with repo filterWould you like me to generate test cases for the
repoIdfilter functionality?Also applies to: 197-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/lib/store.test.ts` around lines 169 - 195, Add test cases that exercise the new repoId filter by calling deletePoints([], 'repo-123') and deletePoints(['id'], 'repo-123') (or analogous) and by calling searchPoints(<sampleVector>, 10, 'repo-123'); for each, mock globalThis.fetch as in existing tests, invoke the function (deletePoints or searchPoints), then inspect the outgoing request via getRequestBody(0) and getRequestUrl(0) to assert the request body contains the filter with repoId: 'repo-123' (e.g., body.filter.repoId or body.filter.repo_id depending on naming) and that the URL targets the correct endpoint (/points/delete or /points/search); follow the existing test patterns for mocking responses and error assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/lib/store.test.ts`:
- Around line 169-195: Add test cases that exercise the new repoId filter by
calling deletePoints([], 'repo-123') and deletePoints(['id'], 'repo-123') (or
analogous) and by calling searchPoints(<sampleVector>, 10, 'repo-123'); for
each, mock globalThis.fetch as in existing tests, invoke the function
(deletePoints or searchPoints), then inspect the outgoing request via
getRequestBody(0) and getRequestUrl(0) to assert the request body contains the
filter with repoId: 'repo-123' (e.g., body.filter.repoId or body.filter.repo_id
depending on naming) and that the URL targets the correct endpoint
(/points/delete or /points/search); follow the existing test patterns for
mocking responses and error assertions.
In `@packages/core/src/lib/store.ts`:
- Around line 198-218: Add a JSDoc comment above the deletePoints function
clarifying the semantics when repoId is provided (ids are ignored and an empty
ids array plus repoId will delete all points for that repo), documenting
parameters and the intended behavior; reference the deletePoints function name
and mention that the implementation uses COLLECTION_NAME, qdrantRequest and log
so maintainers understand this performs a bulk delete by repo when repoId is
supplied.
- Around line 154-159: ensureCollection's creation of the payload index
currently ignores failures; wrap the qdrantRequest call that PUTs the repo_id
index in a try/catch, and validate the response (or qdrantRequest's resolved
value/status) to detect non-2xx outcomes—on failure log a clear error (include
COLLECTION_NAME and the error/response), and either throw the error or return a
failed Promise so callers know index creation failed; modify the call site where
qdrantRequest is used (the qdrantRequest(...) that creates the index for
COLLECTION_NAME in ensureCollection) to implement this handling.
In `@packages/core/src/lib/walker.ts`:
- Around line 66-72: The fast-glob call that builds files (const files = await
fg('**/*', { cwd: rootDir, ignore: FALLBACK_IGNORE, dot: false, absolute: false
})) currently sets dot: false which excludes dotfiles and dot-directories;
decide whether dotfiles should be discoverable and if so change the fg option to
dot: true (or make it configurable) so functions using fg/Files discovery will
include .eslintrc/.github etc.; update any tests or callers relying on files to
account for dotfile inclusion and keep FALLBACK_IGNORE as-is unless you need to
add explicit ignores for dot directories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6556e886-8bcb-4d5e-847b-a5478e514e9d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
docs/plans/2026-03-24-phase2-step3-core-fixes.mddocs/plans/2026-03-24-phase2-step3-index-repo.mddocs/specs/2026-03-24-phase2-step3-index-repo-design.mdpackages/core/package.jsonpackages/core/src/config/env.tspackages/core/src/index.tspackages/core/src/lib/store.test.tspackages/core/src/lib/store.tspackages/core/src/lib/walker.test.tspackages/core/src/lib/walker.ts
💤 Files with no reviewable changes (1)
- packages/core/package.json
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dotenv.config()auto-load and replacedprocess.exit(1)withthrow new Error(). Library modules should not auto-load .env or kill the host process. Removeddotenvfrom dependencies. Closes fix: remove dotenv auto-load and process.exit from env.ts #78useGit?: booleanoption towalkFiles(). Whenfalse, skipsgit ls-filesand goes directly to fast-glob. Needed for tarball extractions which are not git repos. Closes feat: add useGit option to walkFiles for non-git directories #79repo_idandcontenttoPointPayload, optional deterministicidonUpsertPoint,repoIdfilter onsearchPointsanddeletePoints,repo_idpayload index inensureCollection. All additive/optional, backward compatible. Closes feat: extend store.ts for multi-tenant Qdrant #80Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Tests