Open
Conversation
Preview deployments |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4d4043e92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes “module” rows from the search index flow while remaining tolerant of existing type='module' rows in boxel_index. Module dependency/error information is now sourced from the modules cache (via DefinitionLookup) and instance deps are expanded transitively so module rows are no longer needed for invalidation.
Changes:
- Stop writing/querying
type='module'index entries; filter out existing module rows inboxel_indexqueries and counts. - Expand instance deps (and instance error deps) with transitive module dependencies using the module cache; switch dependency-error collection to read from cached module errors.
- Plumb
definitionLookupthrough worker/task args and addDefinitionLookup.getModuleCacheEntries()to support batched cache reads.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/worker.ts | Creates and passes a CachingDefinitionLookup into task args. |
| packages/runtime-common/tasks/index.ts | Extends TaskArgs to include definitionLookup. |
| packages/runtime-common/tasks/indexer.ts | Passes definitionLookup and realmOwnerUserId into IndexRunner. |
| packages/runtime-common/index-runner.ts | Removes module indexing; expands instance deps transitively via module cache; reads module errors from cache instead of index. |
| packages/runtime-common/definition-lookup.ts | Exports cache types and adds getModuleCacheEntries() for batched module cache retrieval. |
| packages/runtime-common/index-writer.ts | Removes module entry support; filters out module rows in reads/copy/counts; rejects module-error writes. |
| packages/runtime-common/index-structure.ts | Removes 'module' from BoxelIndexTable.type. |
| packages/runtime-common/index-query-engine.ts | Removes getModule() and associated module result types. |
| packages/runtime-common/realm-index-query-engine.ts | Removes module() query surface. |
| packages/runtime-common/realm-index-updater.ts | Drops module stats fields from initializer. |
| packages/realm-server/worker-manager.ts | Stops emitting module-error index entries for failed paths. |
| packages/realm-server/tests/server-endpoints/maintenance-endpoints-test.ts | Updates expected stats shape (no module counters). |
| packages/realm-server/tests/indexing-test.ts | Updates tests away from module index queries; validates behavior via instance deps and module cache rows. |
| packages/realm-server/tests/full-reindex-test.ts | Updates task args construction for definitionLookup. |
| packages/realm-server/tests/billing-test.ts | Updates task args construction for definitionLookup. |
| packages/host/tests/unit/index-writer-test.ts | Updates invalidation expectations to rely on instance deps instead of module rows; adjusts copy assertions. |
| packages/host/tests/unit/index-query-engine-test.ts | Adds getModuleCacheEntries to the mock DefinitionLookup. |
| packages/host/tests/integration/components/ai-module-creation-test.gts | Switches from module index assertions to file index assertions for module files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Host Test Results 1 files ± 0 1 suites ±0 1m 10s ⏱️ - 1h 38m 18s For more details on these failures and errors, see this check. Results for commit c9ed4be. ± Comparison against base commit e58dcc0. This pull request removes 1764 and adds 23 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
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.
This PR removes module indexing. This PR is meant to be backwards compatible so that if there are still module index entries we can ignore them safely.
After this PR gets deployed to staging and production, I'll create a followup PR to remove all the module index entries and streamline the queries since we can safely assume those wont exist.