From 81179ded4beb8dc3f7d2dbb27c497b034a40e840 Mon Sep 17 00:00:00 2001 From: Anas Shahid Date: Sun, 14 Sep 2025 15:01:44 -0400 Subject: [PATCH 1/2] Optimize todo widget refetch behavior for homepage performance Add differentiated event handling for todo actions based on context: - Homepage: Skip refetch on snooze to maintain UI responsiveness - Homepage: Immediate refetch on mark-done for instant feedback - Regular page: Refetch on all actions for consistency --- .../homepage/components/todos_widget.vue | 18 ++- .../todos/components/todo_item.vue | 7 +- .../todos/components/todo_item_actions.vue | 8 +- .../homepage/components/todos_widget_spec.js | 77 +++++++++- .../components/todo_item_actions_spec.js | 132 +++++++++++++----- .../todos/components/todo_item_spec.js | 34 ++++- 6 files changed, 239 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/homepage/components/todos_widget.vue b/app/assets/javascripts/homepage/components/todos_widget.vue index e2ccde6364caba..0f589770015855 100644 --- a/app/assets/javascripts/homepage/components/todos_widget.vue +++ b/app/assets/javascripts/homepage/components/todos_widget.vue @@ -78,6 +78,7 @@ export default { currentTab: TABS_INDICES.pending, currentTime: new Date(), currentUserId: computed(() => this.currentUserId), + isHomepage: true, // Flag to indicate this is homepage context }; }, data() { @@ -133,6 +134,19 @@ export default { property: TRACKING_PROPERTY_ALL_TODOS, }); }, + handleTodoChange() { + // Default handler for other change events (like un-snooze) + // Use immediate refetch for simplicity + this.$apollo.queries.todos.refetch(); + }, + handleTodoSnoozed() { + // For snooze: Don't refetch at all + // Let optimistic response persist showing snoozed state + }, + handleTodoMarkDone() { + // For mark as done: Refetch immediately + this.$apollo.queries.todos.refetch(); + }, }, emptyTodosAllDoneSvg, @@ -212,7 +226,9 @@ export default { class="-gl-mx-3 gl-rounded-lg gl-border-b-0 !gl-px-3 gl-py-4" :todo="todo" :tracking-additional="todoTrackingContext" - @change="$apollo.queries.todos.refetch()" + @change="handleTodoChange" + @snoozed="handleTodoSnoozed" + @mark-done="handleTodoMarkDone" /> diff --git a/app/assets/javascripts/todos/components/todo_item.vue b/app/assets/javascripts/todos/components/todo_item.vue index 877b575b0c4dd2..278f7de1108c29 100644 --- a/app/assets/javascripts/todos/components/todo_item.vue +++ b/app/assets/javascripts/todos/components/todo_item.vue @@ -15,7 +15,10 @@ export default { TodoItemTimestamp, TodoItemActions, }, - inject: ['currentTab'], + inject: { + currentTab: 'currentTab', + isHomepage: { from: 'isHomepage', default: false }, + }, props: { todo: { type: Object, @@ -94,6 +97,8 @@ export default { :todo="todo" :is-snoozed="isSnoozed" @change="$emit('change')" + @snoozed="$emit('snoozed')" + @mark-done="$emit('mark-done')" /> diff --git a/app/assets/javascripts/todos/components/todo_item_actions.vue b/app/assets/javascripts/todos/components/todo_item_actions.vue index ed1363463544c5..697e0b43160cf3 100644 --- a/app/assets/javascripts/todos/components/todo_item_actions.vue +++ b/app/assets/javascripts/todos/components/todo_item_actions.vue @@ -18,6 +18,9 @@ export default { GlTooltip: GlTooltipDirective, }, mixins: [InternalEvents.mixin()], + inject: { + isHomepage: { default: false }, + }, props: { todo: { type: Object, @@ -95,6 +98,9 @@ export default { if (data.errors?.length > 0) { reportToSentry(this.$options.name, new Error(data.errors.join(', '))); showError(); + } else if (this.isHomepage && !this.isDone) { + // Only emit mark-done when actually marking as done (not undoing) + this.$emit('mark-done'); } else { this.$emit('change'); } @@ -117,7 +123,7 @@ export default { :todo="todo" :is-snoozed="isSnoozed" :is-pending="isPending" - @snoozed="$emit('change')" + @snoozed="isHomepage ? $emit('snoozed') : $emit('change')" @un-snoozed="$emit('change')" /> { }); }); + describe('todo action handling', () => { + let refetchSpy; + + beforeEach(async () => { + createComponent(); + await waitForPromises(); + + refetchSpy = jest.spyOn(wrapper.vm.$apollo.queries.todos, 'refetch'); + }); + + afterEach(() => { + if (refetchSpy) { + refetchSpy.mockRestore(); + } + }); + + describe('when a todo is snoozed', () => { + it('does not refetch the todos query', async () => { + const todoItem = findTodoItems().at(0); + + await todoItem.vm.$emit('snoozed'); + + expect(refetchSpy).not.toHaveBeenCalled(); + }); + + it('maintains the current todo list without updates', async () => { + const initialTodoCount = findTodoItems().length; + const initialFirstTodoId = findTodoItems().at(0).props('todo').id; + const todoItem = findTodoItems().at(0); + + await todoItem.vm.$emit('snoozed'); + await nextTick(); + + expect(findTodoItems()).toHaveLength(initialTodoCount); + expect(findTodoItems().at(0).props('todo').id).toBe(initialFirstTodoId); + }); + }); + + describe('when a todo is marked as done', () => { + it('immediately refetches the todos query', async () => { + const todoItem = findTodoItems().at(0); + + await todoItem.vm.$emit('mark-done'); + + expect(refetchSpy).toHaveBeenCalledTimes(1); + }); + + it('calls refetch to update the todo list with fresh data', async () => { + const todoItem = findTodoItems().at(0); + await todoItem.vm.$emit('mark-done'); + + // The key behavior: refetch is called to get updated data from server + expect(refetchSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe('when handling other todo actions', () => { + it('refetches the todos query for un-snooze and mark-as-pending actions', async () => { + const todoItem = findTodoItems().at(0); + + // Simulate un-snooze or mark-as-pending which emit 'change' event + await todoItem.vm.$emit('change'); + + expect(refetchSpy).toHaveBeenCalledTimes(1); + }); + + it('calls refetch to update the todo list with fresh data after un-snooze', async () => { + const todoItem = findTodoItems().at(0); + await todoItem.vm.$emit('change'); + + expect(refetchSpy).toHaveBeenCalledTimes(1); + }); + }); + }); + describe('tracking', () => { const { bindInternalEventDocument } = useMockInternalEventsTracking(); diff --git a/spec/frontend/todos/components/todo_item_actions_spec.js b/spec/frontend/todos/components/todo_item_actions_spec.js index 52dbbc10f0ee30..4ad94d7bde6c56 100644 --- a/spec/frontend/todos/components/todo_item_actions_spec.js +++ b/spec/frontend/todos/components/todo_item_actions_spec.js @@ -45,7 +45,7 @@ describe('TodoItemActions', () => { }, }); - const createComponent = ({ props = {} } = {}) => { + const createComponent = ({ props = {}, provide = {} } = {}) => { const mockApollo = createMockApollo([ [markAsDoneMutation, markAsDoneMutationSuccessHandler], [markAsPendingMutation, markAsPendingMutationSuccessHandler], @@ -58,6 +58,10 @@ describe('TodoItemActions', () => { isSnoozed: false, ...props, }, + provide: { + isHomepage: false, + ...provide, + }, mocks: { $toast: { show: mockToastShow, @@ -102,34 +106,74 @@ describe('TodoItemActions', () => { }); describe('toggling the status', () => { - it('marks pending todos as done, emits the `change` event, and optimistic update of the count', async () => { - createComponent(); - findToggleStatusButton().vm.$emit('click'); - await waitForPromises(); + describe('on regular todo page', () => { + it('marks pending todos as done, emits the `change` event, and optimistic update of the count', async () => { + createComponent(); + findToggleStatusButton().vm.$emit('click'); + await waitForPromises(); - expect(markAsDoneMutationSuccessHandler).toHaveBeenCalled(); - expect(wrapper.emitted('change')).toHaveLength(1); - expect(updateGlobalTodoCount).toHaveBeenCalledWith(-1); - }); + expect(markAsDoneMutationSuccessHandler).toHaveBeenCalled(); + expect(wrapper.emitted('change')).toHaveLength(1); + expect(updateGlobalTodoCount).toHaveBeenCalledWith(-1); + }); - it('marks snoozed todos as done and emits the `change` event, and NO optimistic update of the count', async () => { - createComponent({ props: { isSnoozed: true } }); - findToggleStatusButton().vm.$emit('click'); - await waitForPromises(); + it('marks snoozed todos as done and emits the `change` event, and NO optimistic update of the count', async () => { + createComponent({ props: { isSnoozed: true } }); + findToggleStatusButton().vm.$emit('click'); + await waitForPromises(); + + expect(markAsDoneMutationSuccessHandler).toHaveBeenCalled(); + expect(wrapper.emitted('change')).toHaveLength(1); + expect(updateGlobalTodoCount).not.toHaveBeenCalledWith(); + }); - expect(markAsDoneMutationSuccessHandler).toHaveBeenCalled(); - expect(wrapper.emitted('change')).toHaveLength(1); - expect(updateGlobalTodoCount).not.toHaveBeenCalledWith(); + it('marks done todos as pending and emits the `change` event, and optimistic update of the count', async () => { + createComponent({ props: { todo: { ...mockTodo, state: TODO_STATE_DONE } } }); + findToggleStatusButton().vm.$emit('click'); + await waitForPromises(); + + expect(markAsPendingMutationSuccessHandler).toHaveBeenCalled(); + expect(wrapper.emitted('change')).toHaveLength(1); + expect(updateGlobalTodoCount).toHaveBeenCalledWith(+1); + }); }); - it('marks done todos as pending and emits the `change` event, and optimistic update of the count', async () => { - createComponent({ props: { todo: { ...mockTodo, state: TODO_STATE_DONE } } }); - findToggleStatusButton().vm.$emit('click'); - await waitForPromises(); + describe('on homepage', () => { + it('marks pending todos as done and emits the `mark-done` event, and optimistic update of the count', async () => { + createComponent({ provide: { isHomepage: true } }); + findToggleStatusButton().vm.$emit('click'); + await waitForPromises(); - expect(markAsPendingMutationSuccessHandler).toHaveBeenCalled(); - expect(wrapper.emitted('change')).toHaveLength(1); - expect(updateGlobalTodoCount).toHaveBeenCalledWith(+1); + expect(markAsDoneMutationSuccessHandler).toHaveBeenCalled(); + expect(wrapper.emitted('mark-done')).toHaveLength(1); + expect(wrapper.emitted('change')).toBeUndefined(); + expect(updateGlobalTodoCount).toHaveBeenCalledWith(-1); + }); + + it('marks snoozed todos as done and emits the `mark-done` event, and NO optimistic update of the count', async () => { + createComponent({ props: { isSnoozed: true }, provide: { isHomepage: true } }); + findToggleStatusButton().vm.$emit('click'); + await waitForPromises(); + + expect(markAsDoneMutationSuccessHandler).toHaveBeenCalled(); + expect(wrapper.emitted('mark-done')).toHaveLength(1); + expect(wrapper.emitted('change')).toBeUndefined(); + expect(updateGlobalTodoCount).not.toHaveBeenCalledWith(); + }); + + it('marks done todos as pending and emits the `change` event, and optimistic update of the count', async () => { + createComponent({ + props: { todo: { ...mockTodo, state: TODO_STATE_DONE } }, + provide: { isHomepage: true }, + }); + findToggleStatusButton().vm.$emit('click'); + await waitForPromises(); + + expect(markAsPendingMutationSuccessHandler).toHaveBeenCalled(); + expect(wrapper.emitted('change')).toHaveLength(1); + expect(wrapper.emitted('mark-done')).toBeUndefined(); + expect(updateGlobalTodoCount).toHaveBeenCalledWith(+1); + }); }); it('should track an event', () => { @@ -156,19 +200,43 @@ describe('TodoItemActions', () => { expect(findToggleSnoozedStatus().exists()).toBe(true); }); - it.each(['snoozed', 'un-snoozed'])( - 'emits the `change` event when it receives the `%s` event', - (event) => { - createComponent(); + describe('on regular todo page', () => { + it.each(['snoozed', 'un-snoozed'])( + 'emits the `change` event when it receives the `%s` event', + (event) => { + createComponent(); + + expect(wrapper.emitted('change')).toBeUndefined(); + + findToggleSnoozedStatus().vm.$emit(event); + + expect(wrapper.emitted('change')).toHaveLength(1); + }, + ); + }); + + describe('on homepage', () => { + it('emits the `snoozed` event when it receives the `snoozed` event', () => { + createComponent({ provide: { isHomepage: true } }); + expect(wrapper.emitted('snoozed')).toBeUndefined(); + + findToggleSnoozedStatus().vm.$emit('snoozed'); + + expect(wrapper.emitted('snoozed')).toHaveLength(1); expect(wrapper.emitted('change')).toBeUndefined(); + }); - findToggleSnoozedStatus().vm.$emit(event); + it('emits the `change` event when it receives the `un-snoozed` event', () => { + createComponent({ provide: { isHomepage: true } }); - expect(wrapper.emitted('change')).toHaveLength(1); + expect(wrapper.emitted('change')).toBeUndefined(); - expect(findToggleSnoozedStatus().exists()).toBe(true); - }, - ); + findToggleSnoozedStatus().vm.$emit('un-snoozed'); + + expect(wrapper.emitted('change')).toHaveLength(1); + expect(wrapper.emitted('snoozed')).toBeUndefined(); + }); + }); }); }); diff --git a/spec/frontend/todos/components/todo_item_spec.js b/spec/frontend/todos/components/todo_item_spec.js index cfe00f524ec4eb..641fae15cffc0c 100644 --- a/spec/frontend/todos/components/todo_item_spec.js +++ b/spec/frontend/todos/components/todo_item_spec.js @@ -19,7 +19,7 @@ describe('TodoItem', () => { const findTodoItemTimestamp = () => wrapper.findComponent(TodoItemTimestamp); - const createComponent = (props = {}) => { + const createComponent = (props = {}, options = {}) => { wrapper = shallowMount(TodoItem, { propsData: { currentUserId: '1', @@ -30,6 +30,7 @@ describe('TodoItem', () => { }, provide: { currentTab: 0, + isHomepage: options.isHomepage !== undefined ? options.isHomepage : false, }, }); }; @@ -76,6 +77,20 @@ describe('TodoItem', () => { expect(wrapper.emitted('change')).toHaveLength(1); }); + it('emits snoozed event when TodoItemActions emits snoozed', async () => { + createComponent(); + const todoItemActions = wrapper.findComponent(TodoItemActions); + await todoItemActions.vm.$emit('snoozed'); + expect(wrapper.emitted('snoozed')).toHaveLength(1); + }); + + it('emits mark-done event when TodoItemActions emits mark-done', async () => { + createComponent(); + const todoItemActions = wrapper.findComponent(TodoItemActions); + await todoItemActions.vm.$emit('mark-done'); + expect(wrapper.emitted('mark-done')).toHaveLength(1); + }); + describe('multi-select checkbox', () => { describe('with `selectable` prop `false` (default)', () => { it('renders a checkbox', () => { @@ -119,4 +134,21 @@ describe('TodoItem', () => { expect(wrapper.findComponent(TodoItemActions).props('isSnoozed')).toBe(false); }); }); + + describe('isHomepage injection', () => { + it('defaults to false when not explicitly provided', () => { + createComponent(); + expect(wrapper.vm.isHomepage).toBe(false); + }); + + it('can be set to true when provided', () => { + createComponent({}, { isHomepage: true }); + expect(wrapper.vm.isHomepage).toBe(true); + }); + + it('can be set to false explicitly', () => { + createComponent({}, { isHomepage: false }); + expect(wrapper.vm.isHomepage).toBe(false); + }); + }); }); -- GitLab From 282bd20488fd05d11517aff474bf3e7b58a1d6ef Mon Sep 17 00:00:00 2001 From: Anas Shahid Date: Wed, 17 Sep 2025 20:23:43 -0400 Subject: [PATCH 2/2] Refactor: Update homepage context handling and clean up todo item tests --- .../homepage/components/todos_widget.vue | 2 +- .../todos/components/todo_item.vue | 5 +---- .../homepage/components/todos_widget_spec.js | 8 -------- .../todos/components/todo_item_spec.js | 20 +------------------ 4 files changed, 3 insertions(+), 32 deletions(-) diff --git a/app/assets/javascripts/homepage/components/todos_widget.vue b/app/assets/javascripts/homepage/components/todos_widget.vue index 0f589770015855..205b32e3c675ff 100644 --- a/app/assets/javascripts/homepage/components/todos_widget.vue +++ b/app/assets/javascripts/homepage/components/todos_widget.vue @@ -78,7 +78,7 @@ export default { currentTab: TABS_INDICES.pending, currentTime: new Date(), currentUserId: computed(() => this.currentUserId), - isHomepage: true, // Flag to indicate this is homepage context + isHomepage: true, // Homepage context enables optimized refetch behavior for better UX for the new homepage which is on /dashboard/home }; }, data() { diff --git a/app/assets/javascripts/todos/components/todo_item.vue b/app/assets/javascripts/todos/components/todo_item.vue index 278f7de1108c29..d2d2906ae3a9f4 100644 --- a/app/assets/javascripts/todos/components/todo_item.vue +++ b/app/assets/javascripts/todos/components/todo_item.vue @@ -15,10 +15,7 @@ export default { TodoItemTimestamp, TodoItemActions, }, - inject: { - currentTab: 'currentTab', - isHomepage: { from: 'isHomepage', default: false }, - }, + inject: ['currentTab'], props: { todo: { type: Object, diff --git a/spec/frontend/homepage/components/todos_widget_spec.js b/spec/frontend/homepage/components/todos_widget_spec.js index 4b7f8277928da9..88b12ea02b48cc 100644 --- a/spec/frontend/homepage/components/todos_widget_spec.js +++ b/spec/frontend/homepage/components/todos_widget_spec.js @@ -353,14 +353,6 @@ describe('TodosWidget', () => { }); describe('when a todo is marked as done', () => { - it('immediately refetches the todos query', async () => { - const todoItem = findTodoItems().at(0); - - await todoItem.vm.$emit('mark-done'); - - expect(refetchSpy).toHaveBeenCalledTimes(1); - }); - it('calls refetch to update the todo list with fresh data', async () => { const todoItem = findTodoItems().at(0); await todoItem.vm.$emit('mark-done'); diff --git a/spec/frontend/todos/components/todo_item_spec.js b/spec/frontend/todos/components/todo_item_spec.js index 641fae15cffc0c..55159f0bee0f25 100644 --- a/spec/frontend/todos/components/todo_item_spec.js +++ b/spec/frontend/todos/components/todo_item_spec.js @@ -19,7 +19,7 @@ describe('TodoItem', () => { const findTodoItemTimestamp = () => wrapper.findComponent(TodoItemTimestamp); - const createComponent = (props = {}, options = {}) => { + const createComponent = (props = {}) => { wrapper = shallowMount(TodoItem, { propsData: { currentUserId: '1', @@ -30,7 +30,6 @@ describe('TodoItem', () => { }, provide: { currentTab: 0, - isHomepage: options.isHomepage !== undefined ? options.isHomepage : false, }, }); }; @@ -134,21 +133,4 @@ describe('TodoItem', () => { expect(wrapper.findComponent(TodoItemActions).props('isSnoozed')).toBe(false); }); }); - - describe('isHomepage injection', () => { - it('defaults to false when not explicitly provided', () => { - createComponent(); - expect(wrapper.vm.isHomepage).toBe(false); - }); - - it('can be set to true when provided', () => { - createComponent({}, { isHomepage: true }); - expect(wrapper.vm.isHomepage).toBe(true); - }); - - it('can be set to false explicitly', () => { - createComponent({}, { isHomepage: false }); - expect(wrapper.vm.isHomepage).toBe(false); - }); - }); }); -- GitLab