Support ignoring test/spec files inside trigger dirs (fixes #1593) #1596
Conversation
🦋 Changeset detectedLatest commit: b145274 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@ericallam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 27 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 ignored due to path filters (1)
📒 Files selected for processing (16)
WalkthroughThis pull request introduces enhancements to the configuration and build process for Trigger.dev, focusing on improving directory and file management. The changes include new configuration options for specifying task directories, adding ignore patterns, and implementing more robust entry point management during the build process. These modifications provide developers with greater flexibility in defining and processing task directories while introducing more sophisticated file watching and build capabilities. Changes
Sequence DiagramsequenceDiagram
participant Config as Trigger Config
participant EntryPointManager as Entry Point Manager
participant FileWatcher as Chokidar Watcher
participant BuildProcess as Build Process
Config->>EntryPointManager: Initialize with directories
EntryPointManager->>FileWatcher: Set up file watching
FileWatcher-->>EntryPointManager: Detect file changes
EntryPointManager->>BuildProcess: Update entry points
BuildProcess->>BuildProcess: Rebuild with new entry points
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
references/nextjs-realtime/src/trigger/openaiBatch.ts (1)
24-24: Delete temporary files after use to prevent accumulationThe file
batchinput.jsonlis written to disk but not deleted after upload. This could lead to accumulation of temporary files. Consider deleting the file once it's no longer needed.Apply this diff to delete the file after uploading:
const file = await openai.files.create({ file: createReadStream(batchFilename), purpose: "batch", }); +unlinkSync(batchFilename); logger.log("Created file", { file }); // ...references/nextjs-realtime/src/trigger/csv.ts (1)
82-82: Consider using a more meaningful timeout duration.The timeout duration of
1012seems arbitrary. Consider using a round number like1000for better readability, or document why this specific duration was chosen.- await setTimeout(200 + Math.random() * 1012); // 200ms - 1.2s + await setTimeout(200 + Math.random() * 1000); // 200ms - 1.2sreferences/nextjs-realtime/src/components/BatchSubmissionForm.tsx (1)
40-53: Enhance accessibility with ARIA attributes.The textarea should have additional ARIA attributes for better screen reader support.
<Textarea id="jsonl-input" value={jsonlContent} onChange={(e) => setJsonlContent(e.target.value)} placeholder="Paste your JSONL content here..." className="w-full h-48 p-2 text-sm bg-gray-50 border border-gray-300 rounded-md focus:ring-blue-500 focus:border-blue-500" required + aria-label="JSONL Content" + aria-describedby="jsonl-help" /> +<div id="jsonl-help" className="text-sm text-gray-500 mt-1"> + Enter valid JSONL content with one JSON object per line +</div>packages/core/src/v3/config.ts (1)
33-50: Consider adding examples for ignorePatterns.While the default patterns are well documented, it would be helpful to provide examples of custom ignore patterns.
/** * Specify glob patterns to ignore when detecting task files. By default we ignore: * * - *.test.ts * - *.spec.ts * - *.test.mts * - *.spec.mts * - *.test.cts * - *.spec.cts * - *.test.js * - *.spec.js * - *.test.mjs * - *.spec.mjs * - *.test.cjs * - *.spec.cjs + * + * @example + * ```ts + * ignorePatterns: [ + * "**/__tests__/**", + * "**/*.mock.*" + * ] + * ``` */ ignorePatterns?: string[];packages/cli-v3/src/dev/devSession.ts (1)
155-157: TODO comments need implementation.The comments indicate planned functionality for entry point management using glob and chokidar, but the implementation is missing.
Would you like me to help create a GitHub issue to track the implementation of these features? I can provide a detailed implementation plan for:
- Using glob to find initial entry points
- Setting up chokidar for watching entry point changes
- Implementing the build update logic
references/nextjs-realtime/batchinput.jsonl (1)
1-20: Consider rate limiting implications.While the JSONL structure is correct, submitting 20 concurrent requests might hit API rate limits. Consider adding a
retryconfiguration or implementing request batching.You could modify the requests to include rate limiting parameters:
{ "custom_id":"request-1", "method":"POST", "url":"/v1/chat/completions", "body":{ "model":"gpt-3.5-turbo-0125", "messages":[...], - "max_tokens":150 + "max_tokens":150, + "retry_settings": { + "max_retries": 3, + "initial_delay": 1000, + "max_delay": 10000 + } } }references/nextjs-realtime/src/components/BatchProgressIndicator.tsx (3)
26-37: Consider making the initial state configurable via props.The component currently uses hardcoded values for the initial state. Consider accepting these values as props to make the component more reusable and testable.
-export default function BatchProgressIndicator() { +interface BatchProgressIndicatorProps { + initialBatch?: Partial<BatchInfo>; +} + +export default function BatchProgressIndicator({ + initialBatch = { + id: "batch_abc123", + status: "in_progress", + totalRequests: 1000, + completedRequests: 750, + failedRequests: 10, + inputFileName: "input.jsonl", + outputFileName: null, + errorFileName: null, + createdAt: new Date().toISOString(), + completedAt: null, + } +}: BatchProgressIndicatorProps) { const [batchInfo, setBatchInfo] = useState<BatchInfo>({ - id: "batch_abc123", - status: "in_progress", - totalRequests: 1000, - completedRequests: 750, - failedRequests: 10, - inputFileName: "input.jsonl", - outputFileName: null, - errorFileName: null, - createdAt: "2023-03-15T10:30:00Z", - completedAt: null, + ...initialBatch as BatchInfo });
85-182: Enhance accessibility and user experience.The component needs accessibility improvements and better user feedback:
- Add ARIA labels and roles
- Include loading states
- Add error handling UI
- Implement keyboard navigation for interactive elements
Example improvements:
<Card className="w-full max-w-2xl bg-white text-gray-800 border border-gray-200 shadow-sm font-mono"> + <div role="region" aria-label="Batch Progress Status"> <CardHeader className="border-b border-gray-200"> <CardTitle className="flex items-center justify-between text-lg"> <span className="font-bold">Batch Progress: {batchInfo.id}</span> <Badge variant="outline" + role="status" + aria-live="polite" className={`${getStatusColor( batchInfo.status )} px-2 py-1 text-xs font-semibold rounded border`} >
158-179: Add confirmation dialog for batch cancellation.The Cancel Batch button should include a confirmation dialog to prevent accidental cancellations.
<Button variant="destructive" size="sm" className="bg-red-100 text-red-600 hover:bg-red-200 border border-red-300" disabled={ batchInfo.status === "completed" || batchInfo.status === "failed" || batchInfo.status === "expired" } + onClick={() => { + if (window.confirm('Are you sure you want to cancel this batch?')) { + // Handle batch cancellation + } + }} > Cancel Batch </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/famous-grapes-film.md(1 hunks)docs/config/config-file.mdx(1 hunks)docs/mint.json(9 hunks)packages/cli-v3/package.json(2 hunks)packages/cli-v3/src/build/bundle.ts(4 hunks)packages/cli-v3/src/build/entryPoints.ts(1 hunks)packages/cli-v3/src/dev/devSession.ts(1 hunks)packages/core/src/v3/config.ts(4 hunks)references/nextjs-realtime/batchinput.jsonl(1 hunks)references/nextjs-realtime/src/app/openai/page.tsx(1 hunks)references/nextjs-realtime/src/components/BatchProgressIndicator.tsx(1 hunks)references/nextjs-realtime/src/components/BatchSubmissionForm.tsx(1 hunks)references/nextjs-realtime/src/components/ui/alert.tsx(1 hunks)references/nextjs-realtime/src/components/ui/textarea.tsx(1 hunks)references/nextjs-realtime/src/trigger/csv.ts(2 hunks)references/nextjs-realtime/src/trigger/openaiBatch.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/mint.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/cli-v3/src/build/entryPoints.ts
[error] 61-61: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
packages/cli-v3/src/build/bundle.ts (1)
Line range hint
49-114: Efficient implementation of dynamic entry point managementThe introduction of
entryPointManagerand the use of ESBuild's context API for dynamic rebuilding enhance the efficiency of the build process. The code correctly handles the rebuilding of the context when entry points change, ensuring that the latest entry points are always used.references/nextjs-realtime/src/app/openai/page.tsx (1)
7-11: LGTM! Clean component structure.The component follows Next.js best practices with proper prop passing and semantic layout using Tailwind CSS.
references/nextjs-realtime/src/components/ui/textarea.tsx (2)
5-19: LGTM! Well-structured reusable component.The component follows React best practices with:
- Proper TypeScript typing
- Ref forwarding
- Comprehensive styling for various states
- Accessibility considerations
20-20: LGTM! Proper display name.Setting the display name helps with debugging in React DevTools.
references/nextjs-realtime/src/components/ui/alert.tsx (3)
6-20: LGTM! Well-structured variant system.The alert variants are well-defined using class-variance-authority with proper default variants.
22-33: LGTM! Properly implemented Alert component.The component follows best practices with:
- Proper role attribute for accessibility
- TypeScript integration with VariantProps
- Clean prop spreading
47-57: LGTM! Well-implemented AlertDescription component.The component follows best practices with proper typing and styling.
references/nextjs-realtime/src/trigger/csv.ts (1)
11-11: LGTM! Improved log message clarity.The log message now correctly reflects that multiple files may be handled.
references/nextjs-realtime/src/components/BatchSubmissionForm.tsx (1)
12-22: Consider adding error handling for hook initialization.The
useTaskTriggeranduseRealtimeRunhooks might fail if the access token is invalid or if there are network issues. Consider adding error boundaries or try-catch blocks.packages/core/src/v3/config.ts (1)
19-30: LGTM! Well-documented configuration options.The new configuration options are clearly documented with proper references to external documentation.
packages/cli-v3/package.json (1)
94-94: LGTM! Dependencies added for file watching and glob pattern matching.The additions of
chokidarandtinyglobbyare appropriate for implementing file watching and glob pattern matching capabilities. The caret version constraints allow for minor and patch updates while maintaining compatibility.Also applies to: 123-123
.changeset/famous-grapes-film.md (1)
1-5: LGTM! Changeset correctly describes the feature addition.The patch version bump is appropriate for this backward-compatible feature addition that enables ignoring test/spec files in trigger directories.
docs/config/config-file.mdx (1)
50-75: LGTM! Documentation is clear, comprehensive, and well-structured.The new "Dirs" section effectively documents both the
dirsandignorePatternsconfiguration options, providing clear examples and explaining the default behavior. The documentation maintains consistency with the existing style and provides all necessary information for users to implement these features.
@trigger.dev/build
trigger.dev
@trigger.dev/react-hooks
@trigger.dev/core
@trigger.dev/rsc
@trigger.dev/sdk
commit: |
ericallam
force-pushed
the
ea-branch-4
branch
from
d01a9d2 to
b145274
Compare
Summary by CodeRabbit
Release Notes
New Features
Documentation
Dependencies
chokidarandtinyglobbyto support advanced file watching and globbing functionalitiesImprovements