-
Notifications
You must be signed in to change notification settings - Fork 53.3k
fix(editor): Hide 'Create Folder' button if not applicable #23981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(editor): Hide 'Create Folder' button if not applicable #23981
Conversation
Hide the 'Create Folder' button when viewing the 'Shared With You' or 'Overview' page, as folder creation is not supported on these pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 3 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/frontend/editor-ui/src/app/views/WorkflowsView.test.ts">
<violation number="1" location="packages/frontend/editor-ui/src/app/views/WorkflowsView.test.ts:498">
P2: Test name says "overview or shared subpage" but only tests `isSharedSubPage: true`. Consider either adding a separate test for `isOverviewSubPage: true`, or rename this test to "should NOT show 'Create folder' button when in shared subpage".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| expect(getByTestId('add-folder-button')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should NOT show "Create folder" button when in overview or shared subpage', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Test name says "overview or shared subpage" but only tests isSharedSubPage: true. Consider either adding a separate test for isOverviewSubPage: true, or rename this test to "should NOT show 'Create folder' button when in shared subpage".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/editor-ui/src/app/views/WorkflowsView.test.ts, line 498:
<comment>Test name says "overview or shared subpage" but only tests `isSharedSubPage: true`. Consider either adding a separate test for `isOverviewSubPage: true`, or rename this test to "should NOT show 'Create folder' button when in shared subpage".</comment>
<file context>
@@ -477,6 +481,32 @@ describe('Folders', () => {
+ expect(getByTestId('add-folder-button')).toBeInTheDocument();
+ });
+
+ it('should NOT show "Create folder" button when in overview or shared subpage', async () => {
+ vi.spyOn(projectPages, 'isOverviewSubPage', 'get').mockReturnValue(false);
+ vi.spyOn(projectPages, 'isSharedSubPage', 'get').mockReturnValue(true);
</file context>
| it('should NOT show "Create folder" button when in overview or shared subpage', async () => { | |
| it('should NOT show "Create folder" button when in shared subpage', async () => { |
✅ Addressed in 53bfad1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I agree with this. Let's write 2 tests here to test both cases for both overview and shared pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I've updated an existing learning with this new information.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
E2E Tests: n8n tests passed after 9m 33.7s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
MarcL
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments on the tests but good otherwise. 👍🏻
| expect(getByTestId('folder-card-name')).toHaveTextContent(TEST_FOLDER_RESOURCE.name); | ||
| }); | ||
|
|
||
| it('should show "Create folder" button', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a clearer name here?
| it('should show "Create folder" button', async () => { | |
| it('should show "Create folder" button when not in the overview or sharing pages', async () => { |
| expect(getByTestId('add-folder-button')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should NOT show "Create folder" button when in overview or shared subpage', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I agree with this. Let's write 2 tests here to test both cases for both overview and shared pages.
This comment has been minimized.
This comment has been minimized.
…ared-with-you-section-is
Create one test for overview page as well as Shared subpage
|
Thanks for reviewing, I added another test case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 33 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/nodes-base/nodes/Box/BoxTrigger.node.ts">
<violation number="1">
P2: Rule violated: **Prefer Typeguards over Type casting**
Use a type guard instead of `as string` for type narrowing. The `webhook.id` from the API response should be validated with a type check before assignment.</violation>
</file>
<file name="packages/frontend/@n8n/design-system/src/components/AskAssistantChat/messages/ThinkingMessage.test.ts">
<violation number="1">
P2: This assertion could produce a false positive. If `statusText` is null (element not found), `statusText?.className` returns `undefined`, and `expect(undefined).not.toContain('shimmer')` passes silently. Add an existence check before the class assertion to ensure the test fails if the element is missing.</violation>
</file>
<file name="packages/nodes-base/nodes/Mailjet/test/Mailjet.node.test.ts">
<violation number="1">
P2: Same issue as above - `expect.not.objectContaining` with multiple properties passes if *either* is missing. Add explicit checks like:
```typescript
const lastCall = mailjetApiRequestSpy.mock.calls[mailjetApiRequestSpy.mock.calls.length - 1];
expect(lastCall[2].Messages[0]).not.toHaveProperty('CustomCampaign');
expect(lastCall[2].Messages[0]).not.toHaveProperty('DeduplicateCampaign');
```</violation>
<violation number="2">
P2: The `expect.not.objectContaining` assertion with multiple properties doesn't work as intended. It will pass if *either* property is missing, not if *both* are missing. Use separate `.not.toHaveProperty()` checks like you do at lines 369-380 for accurate verification.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
konstantintieber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice UX improvement 👍
…ared-with-you-section-is
Summary
Hide the 'Create Folder' button when viewing the 'Shared With You' or 'Overview' page, as folder creation is not supported on these pages.
Related Linear tickets, Github issues, and Community forum posts
fixes PAY-4362
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)