fix(search-contexts): Fix issue where a repository would not appear in a search context if it was created after the search context was created #354
Conversation
WalkthroughA new shared package ( Changes
Sequence Diagram(s)sequenceDiagram
participant Backend
participant Shared
participant DB
participant ConfigSource
Backend->>Shared: loadConfig(configPath)
alt Remote config
Shared->>ConfigSource: fetch(configPath)
ConfigSource-->>Shared: config JSON
else Local config
Shared->>ConfigSource: readFile(configPath)
ConfigSource-->>Shared: config JSON
end
Shared->>Shared: strip comments, validate schema
Shared-->>Backend: parsed config
Backend->>Shared: syncSearchContexts({contexts, orgId, db})
Shared->>DB: fetch repos, upsert/delete contexts
DB-->>Shared: result
Shared-->>Backend: true/false
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
packages/backend/src/main.ts (1)
14-25: Runtime crash whensettingsfield is absent
loadConfig()may return a config without asettingskey. SpreadingundefinedcausesTypeError: Cannot convert undefined or null to object.- const config = await loadConfig(configPath); - - return { - ...DEFAULT_SETTINGS, - ...config.settings, - } + const config = await loadConfig(configPath); + + return { + ...DEFAULT_SETTINGS, + ...(config.settings ?? {}), + };Add the null-coalescing guard (or default
settingsinsideloadConfig) to prevent crashes on startup.packages/shared/src/ee/syncSearchContexts.ts (1)
27-38: RepeatedfindManyis O(C×R) – fetch repos once per call
allReposis queried inside thefor … of Object.entries(contexts)loop, so the same repo set is fetched once for every context.
For orgs with many contexts this leads to needless DB round-trips.- for (const [key, newContextConfig] of Object.entries(contexts)) { - const allRepos = await db.repo.findMany({ where: { orgId }, … }); + const allRepos = await db.repo.findMany({ where: { orgId }, … }); + + for (const [key, newContextConfig] of Object.entries(contexts)) {Caching the result outside the loop drops the complexity to one query per org, greatly reducing latency.
packages/web/src/initialize.ts (1)
198-203: Un-awaited promise insidefs.watchcallback – errors lost
syncDeclarativeConfigreturns a promise, but the callback neitherawaits nor.catches it.
An exception will be unhandled and silently dropped by Node.-watch(configPath, () => { - logger.info(`Config file ${configPath} changed. Re-syncing...`); - syncDeclarativeConfig(configPath); -}); +watch(configPath, async () => { + logger.info(`Config file ${configPath} changed. Re-syncing...`); + try { + await syncDeclarativeConfig(configPath); + } catch (err) { + logger.error('Re-sync failed', err); + } +});
♻️ Duplicate comments (1)
packages/shared/src/entitlements.ts (1)
53-67: Signature verification depends onbase64Decodefix
decodeLicenseKeyPayloadusesbase64Decode, which will fail under Node due to theatobcall (see utils.ts).
Once the utility is patched, license verification will work consistently in all runtimes.
🧹 Nitpick comments (8)
packages/shared/src/constants.ts (1)
11-11: Confirm special seats flag.
SOURCEBOT_UNLIMITED_SEATS = -1is clear, but consider documenting its semantics via a JSDoc comment for future maintainers.packages/shared/src/index.client.ts (1)
2-2: Drop the explicit “.js” extension for better compatibility with TS/ESM setupsIf
moduleResolutionisnode/bundlerand the compiler later rewrites this to.js, the runtime import will still resolve.
Leaving the extension off ("./constants") avoids edge-cases when tools rewrite extensions (.ts → .js,.js → .cjs, etc.) and makes the statement work in both build and test environments.-export * from "./constants.js"; +export * from "./constants";packages/web/src/app/[domain]/settings/license/page.tsx (1)
20-23: Removal ofawaitis correct but drop the?operator forentitlements
getEntitlements()always returns an array – neverundefined.
You can simplify the rendering expression:-<div className="text-sm font-mono">{entitlements?.join(", ") || "None"}</div> +<div className="text-sm font-mono">{entitlements.length ? entitlements.join(", ") : "None"}</div>packages/web/src/actions.ts (1)
34-35: Prefer the server-specific entry point of the shared package
packages/web/src/actions.tsexecutes exclusively on the server, yet the import points at the root of@sourcebot/shared.
If the shared package exposes both browser-safe and Node-only entry points, pulling from the generic root risks bundling client helpers (or vice-versa) and can enlarge the server bundle.-import { getPlan, getSeats, hasEntitlement, SOURCEBOT_UNLIMITED_SEATS } from "@sourcebot/shared"; +import { getPlan, getSeats, hasEntitlement, SOURCEBOT_UNLIMITED_SEATS } from "@sourcebot/shared/server";(Replace
/serverwith the correct sub-path if different.)This keeps the import tree minimal and prevents accidental inclusion of browser-only code.
packages/web/src/lib/constants.ts (1)
35-35: Minor: guard against unnecessary re-exports on the clientRe-exporting
SOURCEBOT_SUPPORT_EMAILdirectly is fine, but importing from the/cliententry point here means every consumer oflib/constants.tsdrags that module into the browser bundle—even when the constant is already statically known.If the value is a plain string, consider just copying it instead of re-exporting, or switch to:
-export { SOURCEBOT_SUPPORT_EMAIL } from "@sourcebot/shared/client"; +import { SOURCEBOT_SUPPORT_EMAIL as _SUPPORT_EMAIL } from "@sourcebot/shared/client"; +export const SOURCEBOT_SUPPORT_EMAIL = _SUPPORT_EMAIL;This allows bundlers to tree-shake unused parts of the shared package.
packages/backend/src/connectionManager.ts (1)
12-12: Untyped side-effects: import cost awareness
loadConfig+syncSearchContextsare added to the hot path. Ensure they do not pull in heavy JSON-schema / AJV deps for each worker thread. If unavoidable, lazy-require them only whenenv.CONFIG_PATHis set.packages/shared/src/env.ts (1)
7-7: Consider making the enterprise license key validation more explicit.The optional string validation for
SOURCEBOT_EE_LICENSE_KEYcould be enhanced to validate the format or length if there are specific requirements for license keys.If there are specific format requirements for the license key, consider adding validation:
- SOURCEBOT_EE_LICENSE_KEY: z.string().optional(), + SOURCEBOT_EE_LICENSE_KEY: z.string().min(1).optional(),packages/shared/src/utils.ts (1)
21-34: Optional: guard against missingfetchin older Node versions
loadConfigrelies on the globalfetchAPI. Node ≤18 LTS does not enable it by default.
If the project supports those versions, consider a tiny ponyfill:import nodeFetch from "node-fetch"; const fetchFn = globalThis.fetch ?? nodeFetch;Otherwise document the minimum required Node version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
Dockerfile(4 hunks)LICENSE(1 hunks)Makefile(1 hunks)packages/backend/package.json(1 hunks)packages/backend/src/connectionManager.ts(3 hunks)packages/backend/src/constants.ts(1 hunks)packages/backend/src/index.ts(2 hunks)packages/backend/src/main.ts(1 hunks)packages/backend/src/utils.ts(0 hunks)packages/shared/.gitignore(1 hunks)packages/shared/package.json(1 hunks)packages/shared/src/constants.ts(1 hunks)packages/shared/src/ee/syncSearchContexts.ts(4 hunks)packages/shared/src/entitlements.ts(2 hunks)packages/shared/src/env.ts(1 hunks)packages/shared/src/index.client.ts(1 hunks)packages/shared/src/index.server.ts(1 hunks)packages/shared/src/utils.ts(1 hunks)packages/shared/tsconfig.json(1 hunks)packages/web/package.json(1 hunks)packages/web/src/actions.ts(1 hunks)packages/web/src/app/[domain]/layout.tsx(1 hunks)packages/web/src/app/[domain]/settings/license/page.tsx(2 hunks)packages/web/src/app/[domain]/settings/members/page.tsx(1 hunks)packages/web/src/app/layout.tsx(1 hunks)packages/web/src/auth.ts(1 hunks)packages/web/src/ee/features/billing/stripe.ts(1 hunks)packages/web/src/ee/features/publicAccess/publicAccess.tsx(1 hunks)packages/web/src/ee/sso/sso.tsx(2 hunks)packages/web/src/env.mjs(2 hunks)packages/web/src/features/entitlements/constants.ts(0 hunks)packages/web/src/features/entitlements/planProvider.tsx(1 hunks)packages/web/src/features/entitlements/useHasEntitlement.ts(1 hunks)packages/web/src/initialize.ts(3 hunks)packages/web/src/lib/authUtils.ts(1 hunks)packages/web/src/lib/constants.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/backend/src/utils.ts
- packages/web/src/features/entitlements/constants.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/backend/src/index.ts (1)
packages/logger/src/index.ts (1)
createLogger(86-86)
packages/web/src/ee/sso/sso.tsx (1)
packages/shared/src/entitlements.ts (1)
getSeats(110-113)
packages/shared/src/env.ts (1)
packages/shared/src/constants.ts (1)
SOURCEBOT_CLOUD_ENVIRONMENT(4-9)
packages/web/src/app/[domain]/settings/license/page.tsx (1)
packages/shared/src/entitlements.ts (3)
getLicenseKey(78-85)getEntitlements(120-123)getPlan(87-108)
packages/shared/src/ee/syncSearchContexts.ts (3)
packages/logger/src/index.ts (1)
createLogger(86-86)packages/shared/src/entitlements.ts (2)
hasEntitlement(115-118)getPlan(87-108)packages/shared/src/constants.ts (1)
SOURCEBOT_SUPPORT_EMAIL(2-2)
packages/shared/src/entitlements.ts (3)
packages/shared/src/utils.ts (1)
base64Decode(12-15)packages/crypto/src/index.ts (1)
verifySignature(70-93)packages/shared/src/env.ts (1)
env(5-15)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (31)
packages/backend/src/constants.ts (1)
19-19: Skip trivial newline addition.
A single newline was added at the end of the file; there are no substantive changes to the constants object.packages/backend/src/index.ts (1)
13-13: Refine logger context label.
The logger label was updated from'index'to'backend-entrypoint'for clearer identification in logs. Ensure any dashboards or alerting rules referencing the old label are updated accordingly.packages/shared/.gitignore (1)
1-2: Ignore shared package build artifacts.
The new.gitignorecorrectly excludesdist/and.tsbuildinfofiles, preventing compiled outputs from being committed.Makefile (1)
37-38: Extend clean target for shared package.
Addedpackages/shared/node_modulesandpackages/shared/distto thecleantarget, aligning cleanup with the new shared workspace.LICENSE (1)
5-5: Update enterprise license scope.
The license bullet now includespackages/shared/src/ee/, which covers the enterprise code in the shared package. Verify that this directory structure exists and the intended EE files are appropriately licensed.packages/shared/src/constants.ts (2)
2-2: Approve centralization of support email.
TheSOURCEBOT_SUPPORT_EMAILconstant is well-defined for reuse across packages.
4-9: Validate allowed environments tuple.
The readonly tuple forSOURCEBOT_CLOUD_ENVIRONMENTcorrectly enforces the set of valid environments. Ensure this aligns with any environment validation schemas.packages/web/src/features/entitlements/planProvider.tsx (1)
4-4: CentralizeEntitlementimport from shared package.
The import path is updated correctly to use the new@sourcebot/sharedpackage.packages/web/src/features/entitlements/useHasEntitlement.ts (1)
3-3: UpdateEntitlementimport to shared package.
Switching to the shared package import unifies entitlement types across the repo.packages/web/src/lib/authUtils.ts (1)
6-6: Verify server-sidehasEntitlementusage.
Confirm that the sharedhasEntitlementfunction is synchronous and needs no additional context (user/org) here. Adjust the import or call signature if the function is async or requires parameters.packages/web/src/app/layout.tsx (1)
11-11: EnsuregetEntitlementsruns synchronously.
Validate thatgetEntitlements()returns immediately and does not requireawait. If it is async, convertRootLayouttoasyncand update the call accordingly.packages/web/src/auth.ts (1)
17-17: Import path updated to shared package
ImportedhasEntitlementfrom@sourcebot/sharedaligns with the new shared-package refactor. Ensure that@sourcebot/sharedis declared inpackages/web/package.jsonand that your TypeScript path mappings have been updated to resolve this import correctly.packages/web/src/app/[domain]/layout.tsx (1)
17-17: Centralize entitlement utility import
SwitchedhasEntitlementto come from@sourcebot/shared, matching the new shared-package strategy. Verify that this function’s signature (sync vs. async) is consistent with its usages here—remove any unnecessaryawaitor adapt calls as needed.packages/web/package.json (1)
79-79: Add shared package dependency
Adding"@sourcebot/shared": "workspace:*"correctly introduces the new shared package. Confirm that the lockfile and workspace installation have been updated accordingly so builds won’t break.packages/web/src/app/[domain]/settings/members/page.tsx (1)
12-12: Switch to shared entitlements exports
ImportedgetSeatsandSOURCEBOT_UNLIMITED_SEATSfrom@sourcebot/sharedinstead of the local module. Please verify that these exports exist in the shared package and that no legacy entitlement imports remain in this file.packages/web/src/ee/features/publicAccess/publicAccess.tsx (1)
10-10: Refactored entitlement imports
Replaced local imports ofgetPlanandhasEntitlementwith ones from@sourcebot/shared. Make sure the shared implementations match the previous behavior, especially around error messaging and license-key fallback logic.packages/web/src/ee/sso/sso.tsx (2)
12-13: Import update looks goodSwitching to the shared package keeps the SSO module in sync with the new entitlement utilities.
No further action required.
219-223: Confirmed: synchronousgetSeats()call is safe
getSeatsin@sourcebot/sharedis synchronous, so removingawaitavoids an unnecessary micro-task without changing behaviour.
LGTM.packages/backend/package.json (1)
36-36: Dependency added – ensure lockfile is updated
@sourcebot/sharedhas been added. After merging, run the workspace install (pnpm install/yarn install) so the lockfile picks up the new workspace link; otherwise CI may fail.packages/web/src/app/[domain]/settings/license/page.tsx (1)
1-1: Import consolidation LGTMUsing the shared package eliminates duplicate logic and aligns with the new architecture.
packages/web/src/ee/features/billing/stripe.ts (1)
4-4: Good switch to shared entitlement utilThe import path change is correct and keeps the feature in step with the new shared package.
packages/web/src/env.mjs (1)
3-4: ValidateSOURCEBOT_CLOUD_ENVIRONMENTtyping
z.enum()requires a tuple of literal strings (readonly ["a","b",…]). EnsureSOURCEBOT_CLOUD_ENVIRONMENTis declared as a const-asserted tuple in the shared package:export const SOURCEBOT_CLOUD_ENVIRONMENT = ['dev','demo','staging','prod'] as const;If it is instead a regular
string[], TypeScript will accept the call, but at runtimez.enum()will throw.Also applies to: 111-112
packages/shared/src/env.ts (1)
1-3: Well-structured environment configuration.The use of
@t3-oss/env-corewith Zod validation and the centralized constants provides type-safe environment variable handling across the codebase. The server-side schema with appropriate optional/required fields aligns well with the application's needs.Also applies to: 5-12
Dockerfile (2)
46-46: Proper integration of shared package in build process.The shared package is correctly added to the build context and dependencies are installed appropriately in the shared-libs-builder stage.
Also applies to: 53-53
97-97: Consistent shared package distribution across build stages.The built shared package is correctly copied from the shared-libs-builder stage to all dependent stages (web-builder, backend-builder, and runner), ensuring availability across the application components.
Also applies to: 138-138, 222-222
packages/shared/package.json (3)
28-31: Clear separation of server and client exports.The dual export strategy with separate server and client entry points provides good separation of concerns for the shared package.
6-10: Appropriate build configuration with automatic postinstall.The build scripts and postinstall hook ensure the package is built after installation, which is necessary for proper functioning in the monorepo setup.
11-21: Well-chosen dependencies for shared functionality.The dependency selection aligns well with the package's purpose of providing shared utilities, environment configuration, and validation across the codebase.
packages/shared/tsconfig.json (1)
2-20: Modern and strict TypeScript configuration.The configuration uses modern TypeScript features (ES2022, Node16 modules) with strict type checking enabled, which will help catch potential issues early and ensure code quality.
packages/shared/src/index.server.ts (1)
1-20: Clean centralized export structure.The server entry point effectively consolidates shared functionality from multiple modules while maintaining clear separation between functions, types, and constants. This provides a clean API for consuming packages.
packages/web/src/initialize.ts (1)
125-128: Propagation ofcontexts: undefinedmay delete dataThis call passes
config.contextsverbatim; when the key is absent the downstream function currently deletes all contexts (see previous comment insyncSearchContexts.ts).
Until that behaviour is fixed, add a safeguard here or ensure the config always containscontexts.
brendan-kellam
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CHANGELOG.md (1)
12-12: Rephrase changelog entry for consistency and clarityConsider switching to past-tense and refining the description to align with other entries:
- - Fix issue where a repository would not be included in a search context if the context was created before the repository. [#354](https://github.com/sourcebot-dev/sourcebot/pull/354) + - Fixed issue where repositories created after a search context’s creation were not automatically included without a manual resync. [#354](https://github.com/sourcebot-dev/sourcebot/pull/354)packages/shared/README.md (1)
5-9: Typos & casing (“Node.js”, “package”) and minor wordingSmall polish keeps docs professional.
-... work in a Node.JS runtime (e.g., because it depends on the `fs` pacakge. Entitlements are a good example of this), and other code that is runtime agnostic (e.g., `constants.ts`). To deal with this, we these two index files export server code and client code, respectively. +... work in a Node.js runtime (e.g. because it depends on the `fs` package; entitlements are a good example), and other code that is runtime-agnostic (e.g., `constants.ts`). To handle this, we provide two index files that export server-only and client-safe code, respectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)package.json(1 hunks)packages/backend/src/connectionManager.ts(3 hunks)packages/shared/README.md(1 hunks)packages/shared/src/env.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/src/connectionManager.ts
🧰 Additional context used
🪛 LanguageTool
packages/shared/README.md
[uncategorized] ~5-~5: The official spelling of this programming framework is “Node.js”.
Context: ...n this package that will only work in a Node.JS runtime (e.g., because it depends on th...
(NODE_JS)
[formatting] ~5-~5: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ill only work in a Node.JS runtime (e.g., because it depends on the fs pacakge. Entitle...
(COMMA_BEFORE_BECAUSE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
packages/shared/src/env.ts (1)
25-25:SKIP_ENV_VALIDATIONescape-hatch can mask prod mis-configurationsThe toggle is handy during local dev, but leaving it wired into CI/CD or Docker images can silently bypass the very safety net this file provides.
Recommend:
- Add an assertion that
process.env.NODE_ENV !== "production"when the flag is detected.- Search infra configs to ensure the flag is never set outside development.
#!/bin/bash # Detect accidental use of SKIP_ENV_VALIDATION in deployment configs rg --line-number --context 2 'SKIP_ENV_VALIDATION' || true
Problem
If a repository is added after a search context (e.g., a new repository is synced from the code host), then it will never be added to the context even if it should be included. The workaround is to restart the instance.
Solution
This PR adds a call to re-sync all search contexts whenever a connection is successfully synced. This PR adds the
@sourcebot/sharedpackage that containssyncSearchContexts.ts(previously in web) and it's dependencies (namely the entitlements system).Why another package?
Because the
syncSearchContextscall is now called from:initialize.tsin web - handles syncing search contexts on startup and whenever the config is modified in watch mode. This is the same as before.connectionManager.tsin backend - syncs the search contexts whenever a connection is successfully synced.Follow-up devex work
Two things:
crypto,error, andlogger) that we can probably fold into this "general" shared package.schemasanddbfeels like they should remain separate (mostly because they are "code-gen" packages).yarn dev, any changes made to the shared package will only get picked if youctrl+cand restart the instance. Would be nice if we have watch mode work across package dependencies in the monorepo.Summary by CodeRabbit
New Features
@sourcebot/shared) providing centralized constants, environment configuration, entitlement utilities, and configuration loading for both backend and frontend.Refactor
Chores
Bug Fixes
Style
Revert