feat: add shouldNotMerge for task data for skip task merge #807
Conversation
WalkthroughThis update introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TaskService
participant Task
Client->>TaskService: createTask(data)
TaskService->>Task: Instantiate Task with data
TaskService->>Task: call needMergeWhenWaiting()
alt data.shouldNotMerge is true
Task-->>TaskService: return false
else type is SyncBinary or SyncPackage
Task-->>TaskService: return true
else
Task-->>TaskService: return false
end
TaskService-->>Client: Proceed based on merge decision
Possibly related PRs
Suggested labels
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 (9)
✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/common/adapter/binary/PuppeteerBinary.ts(1 hunks)app/core/entity/Task.ts(2 hunks)app/core/service/TaskService.ts(2 hunks)test/core/entity/Task.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/core/entity/Task.test.ts (1)
app/core/entity/Task.ts (1)
Task(102-321)
🪛 GitHub Actions: Node.js CI
app/core/service/TaskService.ts
[error] 7-8: ESLint (no-duplicate-imports): '../entity/Task.js' import is duplicated. Merge the duplicated import into a single import statement.
[error] 7-8: eslint-plugin-import (no-duplicates): Modules should not be imported multiple times in the same file. Merge these imports into a single import statement.
[error] 8-8: typescript-eslint (no-import-type-side-effects): TypeScript will only remove the inline type specifiers which leaves behind a side effect import at runtime. Convert this to a top-level type qualifier to properly remove the entire import.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-postgresql-fs-nfs (24, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
🔇 Additional comments (5)
app/common/adapter/binary/PuppeteerBinary.ts (1)
59-59: LGTM! Good performance optimization.Adding the
max-keys=100parameter to limit S3 results per request is a sensible approach to prevent memory issues and improve performance, especially given the existing iteration limit mentioned in the comments.test/core/entity/Task.test.ts (1)
1-18: LGTM! Well-structured tests for the new functionality.The test cases properly validate the new
needMergeWhenWaiting()instance method behavior:
- Correctly tests that
shouldNotMerge: trueprevents merging- Verifies default behavior for sync binary tasks
- Good use of the existing
createSyncBinaryfactory methodapp/core/service/TaskService.ts (1)
33-33: LGTM! Correct transition to instance method.The change from the static method
Task.needMergeWhenWaiting(task.type)to the instance methodtask.needMergeWhenWaiting()correctly implements the new task merging logic that considers theshouldNotMergeflag.app/core/entity/Task.ts (2)
19-19: LGTM! Well-designed flag for task merging control.The optional
shouldNotMergeflag provides a clean way to control task merging behavior, particularly useful for historical task compensation scenarios as mentioned in the comments.
272-280: LGTM! Correct implementation of instance-based merging logic.The transition from a static method to an instance method is appropriate here since the decision now depends on task-specific data (
shouldNotMergeflag). The implementation correctly:
- Prioritizes the
shouldNotMergeflag to prevent merging when needed- Falls back to the existing type-based logic for
SyncBinaryandSyncPackagetasks- Includes clear comments explaining the use case for historical task compensation
app/core/service/TaskService.ts
Outdated
| import type { Task} from '../entity/Task.js'; | ||
| import { type CreateSyncPackageTaskData } from '../entity/Task.js'; |
There was a problem hiding this comment.
Fix the duplicate imports and import structure.
The ESLint errors indicate issues with the import statements:
Taskis being imported twice (duplicate imports)- The type-only import syntax is causing side effect issues
Apply this diff to fix the import issues:
-import type { Task} from '../entity/Task.js';
-import { type CreateSyncPackageTaskData } from '../entity/Task.js';
+import { Task, type CreateSyncPackageTaskData } from '../entity/Task.js';📝 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.
| import type { Task} from '../entity/Task.js'; | |
| import { type CreateSyncPackageTaskData } from '../entity/Task.js'; | |
| import { Task, type CreateSyncPackageTaskData } from '../entity/Task.js'; |
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 7-8: ESLint (no-duplicate-imports): '../entity/Task.js' import is duplicated. Merge the duplicated import into a single import statement.
[error] 7-8: eslint-plugin-import (no-duplicates): Modules should not be imported multiple times in the same file. Merge these imports into a single import statement.
[error] 8-8: typescript-eslint (no-import-type-side-effects): TypeScript will only remove the inline type specifiers which leaves behind a side effect import at runtime. Convert this to a top-level type qualifier to properly remove the entire import.
🤖 Prompt for AI Agents
In app/core/service/TaskService.ts around lines 7 to 8, there are duplicate
imports of Task and inconsistent type-only import syntax causing ESLint errors.
Consolidate the imports by combining Task and CreateSyncPackageTaskData into a
single type-only import statement from '../entity/Task.js' to avoid duplication
and side effect issues.
Pull request summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #807 +/- ##
========================================
Coverage 95.33% 95.34%
========================================
Files 196 196
Lines 23666 23673 +7
Branches 2165 1937 -228
========================================
+ Hits 22563 22570 +7
Misses 1103 1103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
[skip ci] ## [4.7.0](v4.6.3...v4.7.0) (2025-05-31) ### Features * add shouldNotMerge for task data for skip task merge ([#807](#807)) ([490dce3](490dce3))
[skip ci] ## [4.7.0](v4.6.3...v4.7.0) (2025-06-09) ### Features * add shouldNotMerge for task data for skip task merge ([#807](#807)) ([490dce3](490dce3)) ### Bug Fixes * content type check ([#809](#809)) ([b8c7c06](b8c7c06)), closes [#693](#693) * order binary by date ([#808](#808)) ([12aa425](12aa425))
[skip ci] ## [4.7.0](v4.6.3...v4.7.0) (2025-06-10) ### Features * add shouldNotMerge for task data for skip task merge ([#807](#807)) ([490dce3](490dce3)) ### Bug Fixes * content type check ([#809](#809)) ([b8c7c06](b8c7c06)), closes [#693](#693) * order binary by date ([#808](#808)) ([12aa425](12aa425))
Summary by CodeRabbit