feat: allow preset repo(s) and add repo management tasks for local development #23
Conversation
|
Warning Rate limit exceeded@danielroe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces enhancements to the local development workflow for the application. It adds support for preset repositories during local development, allowing users to specify repositories to be indexed without registering a GitHub app. A new environment variable Changes
Sequence DiagramsequenceDiagram
participant User
participant LocalDevModule
participant PresetRepoPlugin
participant AddRepoTask
participant IndexRepoTask
User->>LocalDevModule: Start local development
LocalDevModule->>PresetRepoPlugin: Trigger preset repositories
PresetRepoPlugin->>AddRepoTask: Run add-repo task
AddRepoTask->>AddRepoTask: Fetch repository metadata
AddRepoTask-->>PresetRepoPlugin: Return added repositories
PresetRepoPlugin->>IndexRepoTask: Trigger indexing
IndexRepoTask->>IndexRepoTask: Index repositories
IndexRepoTask-->>PresetRepoPlugin: Return indexed repositories
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Hi @danielroe, I'm not sure the current implementation is aligning well. Do you have any suggestions or feedback? |
|
I love this - great idea! |
- Add `add-repo` task - Add `delete-repo` task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
server/tasks/index-repo.ts (1)
Line range hint
24-39: Improve error handling and null safetyThe current implementation has several areas for improvement:
- The error handling is too broad and silently continues
- The non-null assertions on owner/name splitting could be safer
Consider this safer implementation:
- const [owner, name] = repo.repo.split('/') - const meta = await getMetadataForRepo(owner!, name!) + const [owner, name] = repo.repo.split('/') + if (!owner || !name) { + console.error(`Invalid repository format: ${repo.repo}`) + continue + } + try { + const meta = await getMetadataForRepo(owner, name) + if (!meta) { + console.error(`No metadata found for repository: ${repo.repo}`) + continue + } + await indexRepo(octokit, meta) + indexed.push(repo.repo) + await setMetadataForRepo(owner, name, { ...meta, indexed: true }) + } catch (e) { + console.error(`Failed to process repository ${repo.repo}:`, e) + }
🧹 Nitpick comments (5)
server/tasks/delete-repo.ts (1)
20-23: Add validation for repository string formatWhen splitting the
repostring, the code assumes it follows the 'owner/name' format. Consider adding validation to handle cases whererepomight not be correctly formatted, to prevent potential runtime errors.Apply this diff to add validation:
for (const repo of repos) { const [owner, name] = repo.split('/') + if (!owner || !name) { + consola.error(`Invalid repository format: ${repo}`) + continue + } await removeMetadataForRepo(owner!, name!) await removeStoredEmbeddingsForRepo(owner!, name!) }server/utils/embeddings.ts (2)
30-30: Remove non-null assertionsThe non-null assertions are unnecessary as the parameters are required.
Apply this diff:
- const keys = await kv.getKeys(storagePrefixForRepo(owner!, repo!)) + const keys = await kv.getKeys(storagePrefixForRepo(owner, repo))
28-38: Consider adding batch processing for large repositoriesFor repositories with many issues, removing all embeddings at once could cause performance issues.
Consider implementing batch processing:
export async function removeStoredEmbeddingsForRepo(owner: string, repo: string, batchSize = 100) { const kv = hubKV() const keys = await kv.getKeys(storagePrefixForRepo(owner, repo)) let removed = 0 for (let i = 0; i < keys.length; i += batchSize) { const batch = keys.slice(i, i + batchSize) await Promise.allSettled(batch.map(async key => kv.removeItem(key))).then((r) => { const failedCount = r.filter(p => p.status === 'rejected').length if (failedCount > 0) { console.error(`Failed to remove ${failedCount} issues from batch ${i/batchSize + 1}`) } removed += batch.length - failedCount }) } console.log('removed', removed, 'issues from', `${owner}/${repo}`) }server/tasks/index-repo.ts (2)
5-10: Enhance filter property documentationThe JSDoc comments could be more specific about the expected format of the filter strings.
/** - * Filter repositories to index + * Filter repositories to index + * @example ['owner/repo', 'another-owner/another-repo'] */ filter?: string[]
Line range hint
41-43: Consider making the repository limit configurableThe hard-coded limit of 30 repositories isn't documented or configurable. Consider making this a configuration option.
+ const MAX_REPOS = useRuntimeConfig().github.maxReposToIndex ?? 30 - if (count++ > 30) { + if (count++ > MAX_REPOS) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!package.json,!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (9)
.env.example(1 hunks)README.md(1 hunks)modules/local.ts(1 hunks)server/plugins/preset-repo.ts(1 hunks)server/routes/github/webhook.post.ts(2 hunks)server/tasks/add-repo.ts(1 hunks)server/tasks/delete-repo.ts(1 hunks)server/tasks/index-repo.ts(1 hunks)server/utils/embeddings.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🔇 Additional comments (2)
server/plugins/preset-repo.ts (1)
13-13: Ensure TypeScript version supports the 'satisfies' operator
The code uses the satisfies operator, which was introduced in TypeScript 4.9. Please confirm that the project is using TypeScript version 4.9 or later to support this syntax.
server/tasks/index-repo.ts (1)
Line range hint 17-46: Verify the impact on preset repositories
Since this task is used for preset repositories (as mentioned in PR objectives), we should verify that the 30-repository limit doesn't affect the preset functionality.
✅ Verification successful
The 30-repository limit does not affect preset repositories
The preset repositories are processed differently from the general indexing flow. When preset repositories are configured, they are first added via the add-repo task, which has no limit. Then, they are indexed using the index-repo task with a specific filter that only includes the preset repositories. Since the filter ensures only preset repositories are processed, the 30-repository limit won't affect the preset functionality as it's unlikely to have more than 30 preset repositories configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any preset repository configurations that might hit the limit
# Search for NUXT_PRESET_REPO environment variable usage
rg -l "NUXT_PRESET_REPO"
# Search for preset repository configurations
ast-grep --pattern 'const $_ = process.env.NUXT_PRESET_REPO'
Length of output: 100
Script:
#!/bin/bash
# Let's check the content of .env.example and search for any preset repository related code
cat .env.example
# Search for any code that handles preset repositories
rg -A 5 "preset.*repo" -i
# Search for repository configuration patterns
ast-grep --pattern 'const repos = $_'
Length of output: 2272
Script:
#!/bin/bash
# Let's check how preset repositories are added and processed
cat server/plugins/preset-repo.ts
# Check the add-repo task implementation
cat server/tasks/add-repo.ts
# Look for the TaskPayload interface definition
rg -A 5 "interface TaskPayload"
Length of output: 2530
server/tasks/delete-repo.ts
Outdated
|
|
||
| for (const repo of repos) { | ||
| const [owner, name] = repo.split('/') | ||
| await removeMetadataForRepo(owner!, name!) |
There was a problem hiding this comment.
Missing import for 'removeMetadataForRepo' function
The function removeMetadataForRepo is called but not imported in this file. This will result in a reference error. Please import the function to resolve this issue.
Apply this diff to fix the issue:
+ import { removeMetadataForRepo } from '../utils/metadata'Ensure the import path is correct based on your project structure.
Committable suggestion skipped: line range outside the PR's diff.
server/tasks/add-repo.ts
Outdated
| const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! }) | ||
|
|
||
| if (!fetchedRepo.data || fetchedRepo.data.private) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Add error handling for GitHub API calls
The GitHub API call could fail, but there's no error handling in place.
Apply this diff to add proper error handling:
- const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! })
-
- if (!fetchedRepo.data || fetchedRepo.data.private) {
- continue
- }
+ try {
+ const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! })
+
+ if (!fetchedRepo.data || fetchedRepo.data.private) {
+ continue
+ }
+ } catch (error) {
+ console.error(`Failed to fetch repository ${owner}/${name}:`, error)
+ continue
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! }) | |
| if (!fetchedRepo.data || fetchedRepo.data.private) { | |
| continue | |
| } | |
| try { | |
| const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! }) | |
| if (!fetchedRepo.data || fetchedRepo.data.private) { | |
| continue | |
| } | |
| } catch (error) { | |
| console.error(`Failed to fetch repository ${owner}/${name}:`, error) | |
| continue | |
| } |
server/tasks/add-repo.ts
Outdated
| if (payload.index) { | ||
| runTask('index-repo', { | ||
| payload: { | ||
| filter: payload.repos, | ||
| } satisfies IndexRepoTaskPayload, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using await for task execution
The task execution is fire-and-forget, which could lead to race conditions or missed errors.
Apply this diff to properly handle the task execution:
- if (payload.index) {
- runTask('index-repo', {
- payload: {
- filter: payload.repos,
- } satisfies IndexRepoTaskPayload,
- })
- }
+ if (payload.index) {
+ try {
+ await runTask('index-repo', {
+ payload: {
+ filter: payload.repos,
+ } satisfies IndexRepoTaskPayload,
+ })
+ } catch (error) {
+ console.error('Failed to index repositories:', error)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (payload.index) { | |
| runTask('index-repo', { | |
| payload: { | |
| filter: payload.repos, | |
| } satisfies IndexRepoTaskPayload, | |
| }) | |
| } | |
| if (payload.index) { | |
| try { | |
| await runTask('index-repo', { | |
| payload: { | |
| filter: payload.repos, | |
| } satisfies IndexRepoTaskPayload, | |
| }) | |
| } catch (error) { | |
| console.error('Failed to index repositories:', error) | |
| } | |
| } |
server/tasks/add-repo.ts
Outdated
| const [owner, name] = repo.split('/') | ||
| if (await getMetadataForRepo(owner!, name!)) continue |
There was a problem hiding this comment.
Add null checks for repository name parts
The split operation could result in undefined values if the repository string format is invalid.
Apply this diff to add proper validation:
- const [owner, name] = repo.split('/')
- if (await getMetadataForRepo(owner!, name!)) continue
+ const parts = repo.split('/')
+ if (parts.length !== 2) {
+ console.error(`Invalid repository format: ${repo}`)
+ continue
+ }
+ const [owner, name] = parts
+ if (await getMetadataForRepo(owner, name)) continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [owner, name] = repo.split('/') | |
| if (await getMetadataForRepo(owner!, name!)) continue | |
| const parts = repo.split('/') | |
| if (parts.length !== 2) { | |
| console.error(`Invalid repository format: ${repo}`) | |
| continue | |
| } | |
| const [owner, name] = parts | |
| if (await getMetadataForRepo(owner, name)) continue |
server/routes/github/webhook.post.ts
Outdated
| }) | ||
|
|
||
| console.log('removed', keys.length, 'issues from', `${owner}/${name}`, 'to the index') | ||
| await removeStoredEmbeddingsForRepo(owner!, name!) |
There was a problem hiding this comment.
Add error handling for embeddings removal
The embeddings removal operation could fail silently.
Apply this diff to add proper error handling:
- await removeStoredEmbeddingsForRepo(owner!, name!)
+ try {
+ await removeStoredEmbeddingsForRepo(owner!, name!)
+ } catch (error) {
+ console.error(`Failed to remove embeddings for ${owner}/${name}:`, error)
+ throw error // Re-throw to ensure the webhook handler knows about the failure
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await removeStoredEmbeddingsForRepo(owner!, name!) | |
| try { | |
| await removeStoredEmbeddingsForRepo(owner!, name!) | |
| } catch (error) { | |
| console.error(`Failed to remove embeddings for ${owner}/${name}:`, error) | |
| throw error // Re-throw to ensure the webhook handler knows about the failure | |
| } |
server/utils/embeddings.ts
Outdated
| } | ||
| }) | ||
|
|
||
| console.log('removed', keys.length, 'issues from', `${owner}/${repo}`, 'to the index') |
There was a problem hiding this comment.
Fix incorrect log message
The log message suggests items are being added to the index when they're being removed.
Apply this diff:
- console.log('removed', keys.length, 'issues from', `${owner}/${repo}`, 'to the index')
+ console.log('removed', keys.length, 'issues from', `${owner}/${repo}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('removed', keys.length, 'issues from', `${owner}/${repo}`, 'to the index') | |
| console.log('removed', keys.length, 'issues from', `${owner}/${repo}`) |
server/tasks/index-repo.ts
Outdated
| export default defineTask({ | ||
| meta: { | ||
| name: 'index-repo', | ||
| description: 'Index repositories', | ||
| }, | ||
| async run() { | ||
| async run({ payload }: { payload: TaskPayload }) { |
There was a problem hiding this comment.
Add null check for payload parameter
The filter check could throw if payload is undefined. Consider adding a null check or providing a default value.
- async run({ payload }: { payload: TaskPayload }) {
+ async run({ payload = {} }: { payload: TaskPayload }) {And update the condition:
- if (repo.indexed || (payload.filter && !payload.filter.includes(repo.repo))) continue
+ if (repo.indexed || (payload?.filter?.length && !payload.filter.includes(repo.repo))) continueAlso applies to: 24-24
tankosinn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/web/server/tasks/index-repo.ts (1)
Line range hint
33-39: Improve error handling and reportingThe current error handling simply logs to console, which might not be sufficient for debugging or monitoring in production.
Consider collecting and returning error information:
+ const errors: Array<{ repo: string, error: string }> = [] try { // ... existing code ... } catch (e) { - console.error(e) + errors.push({ + repo: repo.repo, + error: e instanceof Error ? e.message : String(e) + }) + console.error(`Failed to index ${repo.repo}:`, e) }Then include the errors in the return value:
return { indexed, errors, success: errors.length === 0 }
♻️ Duplicate comments (1)
README.md (1)
110-111:⚠️ Potential issueResolve environment variable naming inconsistency
The documentation uses
DEV_REPOS_TO_INDEX, but previous reviews indicate the implementation usesNUXT_PRESET_REPO. This inconsistency could confuse users.Also applies to: 115-120
🧰 Tools
🪛 LanguageTool
[style] ~110-~110: Consider a shorter alternative to avoid wordiness.
Context: ...gister the GitHub app on any repository in order to see and play around with the cluster al...(IN_ORDER_TO_PREMIUM)
🧹 Nitpick comments (10)
packages/web/server/tasks/index-repo.ts (3)
5-10: Enhance the filter property documentationThe interface is well-structured, but the documentation could be more specific about the expected format of the repository identifiers in the filter array.
Consider updating the JSDoc to clarify the expected format:
/** - * Filter repositories to index + * Filter repositories to index. Each entry should be in the format "owner/repo" */ filter?: string[]
Line range hint
41-44: Consider making the repository limit configurableThe hard-coded limit of 30 repositories might need to be adjustable based on different environments or use cases.
Consider moving this to configuration:
+const MAX_REPOS = useRuntimeConfig().maxReposToIndex ?? 30 + - if (count++ > 30) { + if (count++ > MAX_REPOS) { break }
47-49: Enhance the return value structureThe current return format could be more informative for programmatic consumption.
Consider returning a more structured response:
- return { - result: `Indexed repositories (${indexed.join(', ')}).`, - } + return { + indexed: indexed, + count: indexed.length, + message: `Indexed repositories (${indexed.join(', ')}).`, + success: true + }packages/web/modules/local.ts (1)
32-34: Simplify environment variable check forpresetReposConsider simplifying the condition for setting
presetReposby directly accessing the environment variable, which enhances readability.Apply this diff to streamline the code:
- presetRepos: ('DEV_REPOS_TO_INDEX' in process.env && process.env.DEV_REPOS_TO_INDEX) - ? process.env.DEV_REPOS_TO_INDEX.split(',').filter(Boolean) - : ['nitrojs/nitro', 'nuxt/nuxt'], + presetRepos: process.env.DEV_REPOS_TO_INDEX + ? process.env.DEV_REPOS_TO_INDEX.split(',').filter(Boolean) + : ['nitrojs/nitro', 'nuxt/nuxt'],packages/web/server/tasks/populate-vectorize.ts (1)
39-41: Include error details in the failure messageIncluding the error information in the failure message can aid in debugging and provide more context.
Update the catch block:
- } catch { + } catch (error) { return { - result: `Failed to populate Vectorize with issue embeddings`, + result: `Failed to populate Vectorize with issue embeddings: ${error.message}`, } }packages/web/server/tasks/add-repo.ts (1)
47-53: Consider implementing rate limitingThe task might hit GitHub API rate limits when processing multiple repositories. Consider implementing rate limiting or chunking.
Example implementation:
+import pLimit from 'p-limit' + +const limit = pLimit(5) // Limit concurrent requests + if (payload.index) { - return runTask('index-repo', { + return limit(() => runTask('index-repo', { payload: { filter: payload.repos, } satisfies IndexRepoTaskPayload, - }) + })) }packages/web/server/routes/github/webhook.post.ts (1)
141-142: Consider atomic operations for repository deletionThe metadata and embedding removal operations are not atomic, which could lead to inconsistent state if one operation fails.
Consider implementing a transaction-like pattern:
async function deleteRepo(event: H3Event, repo: InstallationRepo) { const [owner, name] = repo.full_name.split('/') + const operations = [ + removeMetadataForRepo(owner!, name!), + removeStoredEmbeddingsForRepo(owner!, name!) + ] + + try { + await Promise.all(operations) + } catch (error) { + console.error(`Failed to delete repository ${owner}/${name}:`, error) + // Consider implementing rollback mechanism + } - event.waitUntil(removeMetadataForRepo(owner!, name!)) - await removeStoredEmbeddingsForRepo(owner!, name!) }README.md (3)
110-110: Simplify the wording for better readabilityApply this diff to improve clarity:
-in order to see and play around with the cluster algorithm. +to see and experiment with the cluster algorithm.🧰 Tools
🪛 LanguageTool
[style] ~110-~110: Consider a shorter alternative to avoid wordiness.
Context: ...gister the GitHub app on any repository in order to see and play around with the cluster al...(IN_ORDER_TO_PREMIUM)
112-112: Use Oxford spelling for consistencyApply this diff to maintain consistent spelling:
-If you want to customise this +If you want to customize this🧰 Tools
🪛 LanguageTool
[style] ~112-~112: Would you like to use the Oxford spelling “customize”? The spelling ‘customise’ is also correct.
Context: ... the cluster algorithm. If you want to customise this, you can configure the `DEV_REPOS_...(OXFORD_SPELLING_Z_NOT_S)
110-111: Clarify the default repository configurationThe documentation should explicitly list the default repositories in a code block for clarity.
Add this after line 111:
+```ini +# Default configuration +DEV_REPOS_TO_INDEX=nuxt/nuxt,nitrojs/nitro +``` +🧰 Tools
🪛 LanguageTool
[style] ~110-~110: Consider a shorter alternative to avoid wordiness.
Context: ...gister the GitHub app on any repository in order to see and play around with the cluster al...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(1 hunks)packages/web/.env.example(1 hunks)packages/web/modules/local.ts(1 hunks)packages/web/nuxt.config.ts(1 hunks)packages/web/server/plugins/preset-repo.ts(1 hunks)packages/web/server/routes/github/webhook.post.ts(2 hunks)packages/web/server/tasks/add-repo.ts(1 hunks)packages/web/server/tasks/delete-repo.ts(1 hunks)packages/web/server/tasks/index-repo.ts(2 hunks)packages/web/server/tasks/populate-vectorize.ts(1 hunks)packages/web/server/utils/embeddings.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/web/.env.example
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~110-~110: Consider a shorter alternative to avoid wordiness.
Context: ...gister the GitHub app on any repository in order to see and play around with the cluster al...
(IN_ORDER_TO_PREMIUM)
[style] ~112-~112: Would you like to use the Oxford spelling “customize”? The spelling ‘customise’ is also correct.
Context: ... the cluster algorithm. If you want to customise this, you can configure the `DEV_REPOS_...
(OXFORD_SPELLING_Z_NOT_S)
🔇 Additional comments (14)
packages/web/modules/local.ts (7)
5-23: Well-defined ModuleOptions interface
The ModuleOptions interface is clearly defined with comprehensive JSDoc comments, enhancing code readability and maintainability.
25-29: Type-safe module definition
Specifying <ModuleOptions> in defineNuxtModule<ModuleOptions> ensures type safety and improves the developer experience.
30-35: Logical default values for module options
The default values for uiOnly and presetRepos are appropriately set based on command-line arguments and environment variables.
37-39: Verify the use of internal Nuxt property _prepare
The condition checks nuxt.options._prepare, which is an internal property and may not be reliable for future versions. Ensure that relying on this property is appropriate and consider using documented alternatives if available.
41-44: Runtime configuration updated correctly
The runtime configuration is updated to include localDev.presetRepos, ensuring availability in the application context.
46-55: Conditional setup for uiOnly mode
The code correctly handles the uiOnly mode by updating the public runtime configuration and exiting early, maintaining the intended application behaviour.
59-60: Server plugin added when presetRepos are specified
Appropriately adds the server plugin preset-repo when there are preset repositories to process.
packages/web/server/plugins/preset-repo.ts (2)
1-1: Correct import of TaskPayload for type safety
Importing TaskPayload ensures the payload conforms to the expected structure, enhancing type safety.
6-13: Properly triggers repository addition when the hub is ready
The plugin correctly uses onHubReady to run the add-repo task with the preset repositories, integrating seamlessly with the application's lifecycle.
packages/web/server/tasks/delete-repo.ts (2)
4-9: Well-defined TaskPayload interface
The TaskPayload interface is properly defined, ensuring type safety for the task payload.
17-17: Type casting of payload
Casting ctx.payload to TaskPayload ensures correct access to the repos array.
packages/web/server/tasks/populate-vectorize.ts (1)
44-46: Consistent success message format
Returning a clear success message enhances task feedback and user experience.
packages/web/nuxt.config.ts (1)
68-70: LGTM: TypeScript hoisting configuration appears correct
The addition of TypeScript hoisting for 'nitropack/runtime' aligns with the task system implementation. This ensures proper type resolution for the Nitro tasks functionality.
Let's verify if this hoisting is necessary by checking task usage:
✅ Verification successful
TypeScript hoisting configuration is necessary for task system
The hoisting of 'nitropack/runtime' is indeed necessary as evidenced by multiple task definitions in the codebase:
- Four task files under
packages/web/server/tasks/(add-repo, delete-repo, index-repo, populate-vectorize) all import and usedefineTaskfrom 'nitropack/runtime' - Each task file follows the same pattern of importing and using the runtime package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for nitropack/runtime imports and task definitions
rg --type typescript "nitropack/runtime|defineTask"
Length of output: 88
Script:
#!/bin/bash
# Search for nitropack/runtime imports and task definitions using ripgrep without file type filter
rg "nitropack/runtime|defineTask"
# Also check for any .ts files that might contain task-related code
fd -e ts -x cat {} | grep -E "nitropack/runtime|defineTask|nitro.*tasks"
Length of output: 1142
packages/web/server/utils/embeddings.ts (1)
119-120: LGTM: Interface improvements enhance type safety
The changes to IssueMetadata are well-thought-out:
- Making
labelsrequired ensures consistent handling - The index signature allows for future extensibility
| async run(ctx) { | ||
| const payload = ctx.payload as unknown as TaskPayload |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for payload extraction
The current type casting through 'unknown' could be unsafe. Consider adding runtime validation for the payload structure.
Consider using a validation library or implementing a type guard:
function isTaskPayload(payload: unknown): payload is TaskPayload {
if (!payload || typeof payload !== 'object') return false
if ('filter' in payload) {
return Array.isArray((payload as TaskPayload).filter) &&
(payload as TaskPayload).filter!.every(item => typeof item === 'string')
}
return true
}| if (repo.indexed || (payload.filter && !payload.filter.includes(repo.repo))) | ||
| continue |
There was a problem hiding this comment.
Validate repository identifier format
The filter check doesn't validate that the repository identifiers follow the expected "owner/repo" format.
Add validation before using the filter:
- if (repo.indexed || (payload.filter && !payload.filter.includes(repo.repo)))
+ if (repo.indexed || (payload.filter &&
+ (!payload.filter.includes(repo.repo) ||
+ !repo.repo.match(/^[^/]+\/[^/]+$/))))
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (repo.indexed || (payload.filter && !payload.filter.includes(repo.repo))) | |
| continue | |
| if (repo.indexed || (payload.filter && | |
| (!payload.filter.includes(repo.repo) || | |
| !repo.repo.match(/^[^/]+\/[^/]+$/)))) | |
| continue |
| for (const repo of repos) { | ||
| const [owner, name] = repo.split('/') | ||
| await removeMetadataForRepo(owner!, name!) | ||
| await removeStoredEmbeddingsForRepo(owner!, name!) | ||
| } |
There was a problem hiding this comment.
Missing import for removeMetadataForRepo
The function removeMetadataForRepo is used but not imported, which will result in a ReferenceError.
Add the import statement at the top of the file:
+ import { removeMetadataForRepo } from '../utils/metadata'Committable suggestion skipped: line range outside the PR's diff.
| const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! }) | ||
|
|
||
| if (!fetchedRepo.data || fetchedRepo.data.private) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Add error handling for GitHub API calls
The GitHub API call lacks proper error handling, which could cause the task to fail silently.
Suggested implementation:
- const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! })
+ let fetchedRepo
+ try {
+ fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! })
+ } catch (error) {
+ if (error.status === 404) {
+ console.warn(`Repository not found: ${owner}/${name}`)
+ } else {
+ console.error(`Failed to fetch repository ${owner}/${name}:`, error)
+ }
+ continue
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! }) | |
| if (!fetchedRepo.data || fetchedRepo.data.private) { | |
| continue | |
| } | |
| let fetchedRepo | |
| try { | |
| fetchedRepo = await octokit.repos.get({ owner: owner!, repo: name! }) | |
| } catch (error) { | |
| if (error.status === 404) { | |
| console.warn(`Repository not found: ${owner}/${name}`) | |
| } else { | |
| console.error(`Failed to fetch repository ${owner}/${name}:`, error) | |
| } | |
| continue | |
| } | |
| if (!fetchedRepo.data || fetchedRepo.data.private) { | |
| continue | |
| } |
| for (const repo of payload.repos) { | ||
| const [owner, name] = repo.split('/') | ||
| if (await getMetadataForRepo(owner!, name!)) | ||
| continue | ||
|
|
There was a problem hiding this comment.
Add validation for repository format
The repository string splitting lacks validation, which could lead to runtime errors if the format is incorrect.
Suggested implementation:
for (const repo of payload.repos) {
+ if (!repo.includes('/')) {
+ console.warn(`Invalid repository format: ${repo}. Expected format: owner/name`)
+ continue
+ }
const [owner, name] = repo.split('/')
+ if (!owner || !name) {
+ console.warn(`Invalid repository format: ${repo}. Both owner and name are required`)
+ continue
+ }
- if (await getMetadataForRepo(owner!, name!))
+ if (await getMetadataForRepo(owner, name))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const repo of payload.repos) { | |
| const [owner, name] = repo.split('/') | |
| if (await getMetadataForRepo(owner!, name!)) | |
| continue | |
| for (const repo of payload.repos) { | |
| if (!repo.includes('/')) { | |
| console.warn(`Invalid repository format: ${repo}. Expected format: owner/name`) | |
| continue | |
| } | |
| const [owner, name] = repo.split('/') | |
| if (!owner || !name) { | |
| console.warn(`Invalid repository format: ${repo}. Both owner and name are required`) | |
| continue | |
| } | |
| if (await getMetadataForRepo(owner, name)) | |
| continue | |
| description: 'Add new repositories', | ||
| }, | ||
| async run(ctx) { | ||
| const payload = ctx.payload as unknown as TaskPayload |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using type-safe payload validation
The current type casting using as unknown as TaskPayload bypasses TypeScript's type safety. Consider implementing runtime validation using a schema validation library.
Example implementation using Zod:
+import { z } from 'zod'
+
+const taskPayloadSchema = z.object({
+ repos: z.array(z.string()),
+ index: z.boolean().optional()
+})
-const payload = ctx.payload as unknown as TaskPayload
+const payload = taskPayloadSchema.parse(ctx.payload)Committable suggestion skipped: line range outside the PR's diff.
| }) | ||
|
|
||
| console.log('removed', keys.length, 'issues from', `${owner}/${name}`, 'to the index') | ||
| await removeStoredEmbeddingsForRepo(owner!, name!) |
There was a problem hiding this comment.
Add error handling for embedding removal
The embedding removal operation lacks error handling, which could lead to incomplete cleanup if the operation fails.
Suggested implementation:
- await removeStoredEmbeddingsForRepo(owner!, name!)
+ try {
+ await removeStoredEmbeddingsForRepo(owner!, name!)
+ } catch (error) {
+ console.error(`Failed to remove embeddings for ${owner}/${name}:`, error)
+ // Consider retrying or adding to a cleanup queue
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await removeStoredEmbeddingsForRepo(owner!, name!) | |
| try { | |
| await removeStoredEmbeddingsForRepo(owner!, name!) | |
| } catch (error) { | |
| console.error(`Failed to remove embeddings for ${owner}/${name}:`, error) | |
| // Consider retrying or adding to a cleanup queue | |
| } |
Description
These changes simplify local development and play with core functionalities by allowing the preset repo(s) and with additional tasks.
Preset Repo(s)
Repositories specified with
PRESET_REPOSare indexed at startup.Nitro Tasks
add-repo: Add new repo(s) with theindexoption to index the added repositories.remove-repo: Remove repo(s) and their embeddings.