refactor(core): Clean up workflow history compaction service and related utils #24043
+255
−492
Conversation
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
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/@n8n/db/src/repositories/workflow-history.repository.ts">
<violation number="1" location="packages/@n8n/db/src/repositories/workflow-history.repository.ts:101">
P1: `SKIP_RULES.skipDifferentUsers` expects a `user` property but `WorkflowHistory` entity has `authors` instead. This will always compare `undefined !== undefined` (returns `false`), so consecutive versions by different users will NOT be protected from deletion as intended.</violation>
</file>
<file name="packages/workflow/src/workflow-diff.ts">
<violation number="1" location="packages/workflow/src/workflow-diff.ts:234">
P2: Incomplete error message - the text cuts off after "no ". This should describe what's missing (e.g., "no right diff" or "no right element") for better debugging.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (shouldMerge) { | ||
| const right = diffs.splice(i, 1)[0]; | ||
| if (!right) throw new Error('invariant broken'); | ||
| if (!right) throw new Error('invariant broken - no '); |
Contributor
There was a problem hiding this comment.
P2: Incomplete error message - the text cuts off after "no ". This should describe what's missing (e.g., "no right diff" or "no right element") for better debugging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workflow/src/workflow-diff.ts, line 234:
<comment>Incomplete error message - the text cuts off after "no ". This should describe what's missing (e.g., "no right diff" or "no right element") for better debugging.</comment>
<file context>
@@ -277,13 +225,13 @@ export function groupWorkflows<W extends WorkflowDiffBase = WorkflowDiffBase>(
if (shouldMerge) {
const right = diffs.splice(i, 1)[0];
- if (!right) throw new Error('invariant broken');
+ if (!right) throw new Error('invariant broken - no ');
// merge diffs
</file context>
Suggested change
| if (!right) throw new Error('invariant broken - no '); | |
| if (!right) throw new Error('invariant broken - no right diff'); |
✅ Addressed in 8ce7ce6
This comment has been minimized.
This comment has been minimized.
n8n-assistant
bot
added
core
|
E2E Tests: n8n tests passed after 10m 15s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
Contributor
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts">
<violation number="1" location="packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts:116">
P0: `it.only` should not be committed. This will cause all other tests in the file to be skipped, breaking CI test coverage.</violation>
<violation number="2" location="packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts:173">
P1: Commented-out assertions suggest incomplete test or debugging code. The test title states it should 'never prune previously active or named versions', but assertions for id2 (named version) and id4 (activated version) are commented out, which contradicts the test's stated purpose.</violation>
</file>
<file name="packages/workflow/src/workflow-diff.ts">
<violation number="1" location="packages/workflow/src/workflow-diff.ts:17">
P1: Rule violated: **Tests**
Tests for `skipDifferentUsers` use the old `user` property, but the implementation now uses `authors`. The test helper creates workflows with `{ user }` but the type expects `{ authors }`. These tests need to be updated to match the implementation change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }); | ||
|
|
||
| it('should never prune previously active or named versions', async () => { | ||
| it.only('should never prune previously active or named versions', async () => { |
Contributor
There was a problem hiding this comment.
P0: it.only should not be committed. This will cause all other tests in the file to be skipped, breaking CI test coverage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts, line 116:
<comment>`it.only` should not be committed. This will cause all other tests in the file to be skipped, breaking CI test coverage.</comment>
<file context>
@@ -113,7 +113,7 @@ describe('WorkflowHistoryRepository', () => {
});
- it('should never prune previously active or named versions', async () => {
+ it.only('should never prune previously active or named versions', async () => {
// ARRANGE
const id1 = uuid();
</file context>
Suggested change
| it.only('should never prune previously active or named versions', async () => { | |
| it('should never prune previously active or named versions', async () => { |
✅ Addressed in 8ce7ce6
| // ASSERT | ||
| expect(deleted).toBe(1); | ||
| expect(seen).toBe(5); | ||
| // expect(deleted).toBe(1); |
Contributor
There was a problem hiding this comment.
P1: Commented-out assertions suggest incomplete test or debugging code. The test title states it should 'never prune previously active or named versions', but assertions for id2 (named version) and id4 (activated version) are commented out, which contradicts the test's stated purpose.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts, line 173:
<comment>Commented-out assertions suggest incomplete test or debugging code. The test title states it should 'never prune previously active or named versions', but assertions for id2 (named version) and id4 (activated version) are commented out, which contradicts the test's stated purpose.</comment>
<file context>
@@ -169,15 +169,16 @@ describe('WorkflowHistoryRepository', () => {
// ASSERT
- expect(deleted).toBe(1);
expect(seen).toBe(5);
+ // expect(deleted).toBe(1);
const history = await repository.find();
</file context>
✅ Addressed in 8ce7ce6
| connections: IConnections; | ||
| createdAt: Date; | ||
| user?: unknown; | ||
| authors?: string; |
Contributor
There was a problem hiding this comment.
P1: Rule violated: Tests
Tests for skipDifferentUsers use the old user property, but the implementation now uses authors. The test helper creates workflows with { user } but the type expects { authors }. These tests need to be updated to match the implementation change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workflow/src/workflow-diff.ts, line 17:
<comment>Tests for `skipDifferentUsers` use the old `user` property, but the implementation now uses `authors`. The test helper creates workflows with `{ user }` but the type expects `{ authors }`. These tests need to be updated to match the implementation change.</comment>
<file context>
@@ -14,7 +14,7 @@ export type DiffableWorkflow<N extends DiffableNode = DiffableNode> = {
connections: IConnections;
createdAt: Date;
- user?: unknown;
+ authors?: string;
};
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts">
<violation number="1" location="packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts:173">
P1: Test assertion contradicts test purpose. The test is named "should never prune previously active or named versions" but expects `id4` (which has publish history with `event: 'activated'`) to be pruned. Either the test name should be updated, or `id4` should be included in the expected result with `deleted` being 1 instead of 2.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/cli/test/integration/database/repositories/workflow-history.repository.test.ts
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
A few changes here:
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-4601
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)