Conversation
quangtuanitmo18
commented
Mar 2, 2026
- Integrate event deletion logic, pr api: feat: add api remove event hawk.api.nodejs#633
- In the event overview, add three dots in the upper right corner that will open a context menu (via codex-ui). There will also be a Delete Event button.
2a016b0 to
acb81f2
Compare
There was a problem hiding this comment.
Pull request overview
Adds “delete event” functionality to the event modal, wiring a new GraphQL mutation through the API/store layers and exposing it via a 3-dots context menu in the event header (Codex UI popover), with notifications and list refresh on return to the project overview.
Changes:
- Added GraphQL mutation + API/store action to remove an event (and related data).
- Implemented event header “more” menu with delete confirmation, success/error notifications, and navigation back to overview.
- Integrated Codex UI Popover globally and adjusted popover z-index to render above modals.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/styles/base.css | Overrides Codex UI popover z-index to render above app modals. |
| src/store/modules/events/index.ts | Adds Vuex action to remove an event via API. |
| src/store/modules/events/actionTypes.js | Introduces REMOVE_EVENT action type constant. |
| src/main.ts | Ensures Codex UI styles are imported (order adjustment). |
| src/i18n/messages/ru.json | Adds RU strings for remove-event UI flow. |
| src/i18n/messages/en.json | Adds EN strings for remove-event UI flow. |
| src/components/project/Overview.vue | Listens for event-deleted from nested route component to refresh daily events. |
| src/components/event/Layout.vue | Re-emits event-deleted and navigates back to overview to close modal. |
| src/components/event/EventHeader.vue | Adds 3-dots menu, confirmation, store dispatch, notifier messages, and emits event-deleted. |
| src/components/event/EventActionsMenu.vue | New popover menu component rendering action items. |
| src/api/events/queries.ts | Adds MUTATION_REMOVE_EVENT GraphQL query string. |
| src/api/events/index.ts | Exposes removeEvent(projectId, eventId) API wrapper. |
| src/App.vue | Mounts Codex UI <Popover /> needed for usePopover(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/components/event/EventHeader.vue
Outdated
| <div class="event-header__button event-header__button--more"> | ||
| <Icon | ||
| class="event-header__button--more-icon" | ||
| symbol="dots" | ||
| @click="onMoreClick($event)" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
The “more” control only has the click handler on the inner , so clicks on the surrounding 24×24 area won’t open the menu. Consider moving the @click to the wrapper (and preferably make it a ) so the whole hit area is interactive and keyboard-accessible.
| <div class="event-header__button event-header__button--more"> | |
| <Icon | |
| class="event-header__button--more-icon" | |
| symbol="dots" | |
| @click="onMoreClick($event)" | |
| /> | |
| </div> | |
| <button | |
| type="button" | |
| class="event-header__button event-header__button--more" | |
| @click="onMoreClick($event)" | |
| > | |
| <Icon | |
| class="event-header__button--more-icon" | |
| symbol="dots" | |
| /> | |
| </button> |
| onMoreClick(event: MouseEvent) { | ||
| this.showPopover({ | ||
| targetEl: event.currentTarget as HTMLElement, | ||
| with: { |
There was a problem hiding this comment.
onMoreClick anchors the popover to event.currentTarget from an SVG click, then force-casts it to HTMLElement. This is brittle (and currently anchors to the SVG, not the full button). If you move the click handler to the wrapper element, you can pass a real HTML element to the popover without unsafe casting.
src/components/event/EventHeader.vue
Outdated
| onConfirm: async () => { | ||
| try { | ||
| await this.$store.dispatch(REMOVE_EVENT, { | ||
| projectId, | ||
| eventId, | ||
| }); | ||
| notifier.show({ | ||
| message: this.$t('event.removeSuccess').toString(), | ||
| style: 'success', | ||
| time: 5000, | ||
| }); | ||
| this.$emit('event-deleted'); | ||
| } catch { |
There was a problem hiding this comment.
The delete flow treats any resolved dispatch as success. Since REMOVE_EVENT ultimately returns a boolean, a false result (no exception) would still show a success notification and close the modal. Please check the returned value and handle false as a failure (e.g., throw / show error).
src/store/modules/events/index.ts
Outdated
| async [REMOVE_EVENT]({ commit }, { projectId, eventId }: { | ||
| projectId: string; | ||
| eventId: string; | ||
| }): Promise<boolean> { | ||
| return eventsApi.removeEvent(projectId, eventId); | ||
| }, |
There was a problem hiding this comment.
This action is marked async but doesn’t await, which triggers the repo’s @typescript-eslint/require-await warning. It also destructures commit but never uses it, causing an unused-vars warning and making the JSDoc misleading; remove commit from the signature (or rename to _commit) and either drop async or add an await with proper handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onConfirm: async () => { | ||
| try { | ||
| await this.$store.dispatch(REMOVE_EVENT, { | ||
| projectId, | ||
| eventId, | ||
| }); | ||
| notifier.show({ | ||
| message: this.$t('event.removeSuccess').toString(), | ||
| style: 'success', | ||
| time: 5000, | ||
| }); | ||
| this.$emit('event-deleted'); | ||
| } catch { | ||
| notifier.show({ | ||
| message: this.$t('event.removeError').toString(), | ||
| style: 'error', | ||
| time: 5000, | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The deletion flow treats any resolved dispatch as success, but eventsApi.removeEvent() uses api.callOld(), which does not throw on GraphQL errors and may return false/null while still resolving. Please check the boolean result from dispatch(REMOVE_EVENT, ...) and show the error notifier (and avoid emitting event-deleted) when the result is falsy (or switch this API call to api.call() and handle response.errors).
| <Icon | ||
| class="event-header__button--more" | ||
| symbol="dots" | ||
| @click="onMoreClick($event)" | ||
| /> |
There was a problem hiding this comment.
The 3-dots control is an SVG Icon with a click handler, which is not keyboard-focusable and has no accessible name. Please render it as a real <button type="button"> (or add appropriate role="button", tabindex="0", key handlers) and provide an aria-label like "More actions" so keyboard/screen-reader users can access the menu.
| <Icon | |
| class="event-header__button--more" | |
| symbol="dots" | |
| @click="onMoreClick($event)" | |
| /> | |
| <button | |
| type="button" | |
| class="event-header__button--more" | |
| aria-label="More actions" | |
| @click="onMoreClick($event)" | |
| > | |
| <Icon | |
| symbol="dots" | |
| aria-hidden="true" | |
| /> | |
| </button> |
| :key="index" | ||
| class="event-actions-menu__item" | ||
| :class="{ 'event-actions-menu__item--danger': item.danger }" | ||
| @click="item.onClick" |
There was a problem hiding this comment.
@click="item.onClick" passes the click event argument to the handler; this conflicts with the declared type onClick: () => void and makes the invocation ambiguous. Prefer @click="item.onClick()" (or update the type to accept MouseEvent) and set type="button" on these <button> elements to avoid accidental form submission defaults.
| @click="item.onClick" | |
| type="button" | |
| @click="item.onClick()" |
src/store/modules/events/index.ts
Outdated
| * @param context - vuex action context (not used) | ||
| * @param context.commit - standard Vuex commit function | ||
| * @param payload - vuex action payload | ||
| * @param payload.projectId - project event is related to | ||
| * @param payload.eventId - original event id to remove | ||
| */ | ||
| async [REMOVE_EVENT]({ commit }, { projectId, eventId }: { | ||
| projectId: string; | ||
| eventId: string; | ||
| }): Promise<boolean> { | ||
| return await eventsApi.removeEvent(projectId, eventId); |
There was a problem hiding this comment.
REMOVE_EVENT action destructures commit but never uses it, which is likely to trip no-unused-vars/linting and the JSDoc still documents context.commit as used. Consider removing commit from the signature (or renaming to _commit) and updating the JSDoc accordingly; also return await ... can be simplified to return ....
| * @param context - vuex action context (not used) | |
| * @param context.commit - standard Vuex commit function | |
| * @param payload - vuex action payload | |
| * @param payload.projectId - project event is related to | |
| * @param payload.eventId - original event id to remove | |
| */ | |
| async [REMOVE_EVENT]({ commit }, { projectId, eventId }: { | |
| projectId: string; | |
| eventId: string; | |
| }): Promise<boolean> { | |
| return await eventsApi.removeEvent(projectId, eventId); | |
| * @param _context - vuex action context (unused) | |
| * @param payload - vuex action payload | |
| * @param payload.projectId - project event is related to | |
| * @param payload.eventId - original event id to remove | |
| */ | |
| async [REMOVE_EVENT](_context, { projectId, eventId }: { | |
| projectId: string; | |
| eventId: string; | |
| }): Promise<boolean> { | |
| return eventsApi.removeEvent(projectId, eventId); |
src/api/events/index.ts
Outdated
| * @param eventId — original event id to remove | ||
| */ | ||
| export async function removeEvent(projectId: string, eventId: string): Promise<boolean> { | ||
| return (await api.callOld(MUTATION_REMOVE_EVENT, { |
There was a problem hiding this comment.
please, use api.call() instead of deprecated callOld()
and process possible errors on store level
| @@ -0,0 +1,93 @@ | |||
| <template> | |||
| <div class="event-actions-menu"> | |||
| <button | |||
There was a problem hiding this comment.
With the current Codexui contextmenu code, I can't add custom styles.
| }); | ||
| </script> | ||
|
|
||
| <style scoped> |
| </div> | ||
| </template> | ||
|
|
||
| <script lang="ts"> |
There was a problem hiding this comment.
please, use script setup syntax in new components
| <EventHeader | ||
| v-if="event || loading" | ||
| :event="event" | ||
| @event-deleted="onEventDeleted" |
There was a problem hiding this comment.
I think this event is redundant, you can handle it inside
There was a problem hiding this comment.
@neSpecc if i remove this handling, the events list won’t refresh after deleting an event.
Closing the event modal only changes the route; it does not automatically reload EventsList, so the deleted item can still appear until a manual reload.