325 add unit tests for event viewer#326
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR enhances event viewer components with unit tests, adds missing Vue component imports (Checkbox, ToggleSwitch), refactors event filtering and log-level counting logic in EventViewer.vue, sets a default query limit of 100 events, and updates mock API handlers to support event endpoint testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/events/EventViewer.vue (1)
178-182:⚠️ Potential issue | 🟡 Minor
filteredEventCountis not reset when new events are loaded via the date filter.After
handleShowRequestEventsreplaceseventswith a fresh dataset,filteredEventCountretains whatever value was set by the last DataTable@filterevent. Downstream consumers —:max="filteredEventCount || eventCount"onMeterGroupand the:eventCountprop onDateFilterGraph— will reflect a stale filtered count rather than the new total.🐛 Proposed fix
function handleShowRequestEvents(requestedEvents: EventLogResponse) { events.value = requestedEvents.data; eventCount.value = requestedEvents.meta.count; + filteredEventCount.value = 0; updateLogLevelCounts(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/events/EventViewer.vue` around lines 178 - 182, When replacing the events dataset in handleShowRequestEvents, reset the DataTable filter state so downstream consumers use the new total; inside handleShowRequestEvents (which currently sets events.value, eventCount.value and calls updateLogLevelCounts), also clear filteredEventCount (e.g. set filteredEventCount.value = 0 or null/undefined) so :max="filteredEventCount || eventCount" on MeterGroup and :eventCount on DateFilterGraph reflect the fresh eventCount rather than a stale filter value.
🧹 Nitpick comments (4)
test/components/header/MenuHeader.spec.ts (1)
10-18: Incorrect type annotation and unnecessaryasynconbeforeAll.Two issues on adjacent lines:
Line 10 —
DefineComponent<typeof defineComponent>passes the function-type ofdefineComponentitself as the props-type generic argument, which is incorrect. The equivalent inAvatarButton.spec.tsusestypeof AvatarButton. Use the same pattern here for correctness and consistency.Line 13 —
beforeAllis declaredasyncbut the body contains only a synchronousdefineComponent({...})assignment (noawait). This is the exact pattern that was corrected forbeforeAllinAvatarButton.spec.tsin this same PR.♻️ Proposed fix
- let MenuHeaderTestComponent: DefineComponent<typeof defineComponent>; + let MenuHeaderTestComponent: typeof MenuHeader; // Render the component with the fake params - beforeAll(async () => { + beforeAll(() => { MenuHeaderTestComponent = defineComponent({ components: { MenuHeader }, template: "<Suspense><MenuHeader/></Suspense>", }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/header/MenuHeader.spec.ts` around lines 10 - 18, The variable MenuHeaderTestComponent is incorrectly typed and beforeAll is unnecessarily async: change the generic on MenuHeaderTestComponent from DefineComponent<typeof defineComponent> to DefineComponent<typeof MenuHeader> (matching AvatarButton.spec.ts) and remove the async modifier from the beforeAll declaration since the body only performs a synchronous defineComponent({...}) assignment for the MenuHeader component.composables/useAPIFetch.ts (1)
29-37:opts.queryis silently overridden — merge query params instead.Because
...optsis spread beforequery: { limit: 100 }, anyqueryproperty insideoptsis completely replaced. While the current only call site (EventViewer.vue) doesn't pass custom query params, the function signature acceptsoptswithout restriction, making it a latent issue for future callers. The same pattern affects multiple functions across this file (getProjectNodes, getProjects, getAnalyses, etc.).♻️ Proposed fix — merge caller query with the default
export function getEvents(opts?) { return useAPIFetch<EventLogResponse>("/events", { ...opts, method: "GET", query: { limit: 100, + ...opts?.query, }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composables/useAPIFetch.ts` around lines 29 - 37, The spread of ...opts before setting query in getEvents causes any caller-provided opts.query to be overwritten; change the call to merge default and caller query without mutating opts, e.g. keep method:"GET" and set query: { limit: 100, ...(opts?.query || {}) } so callers can override or add params; apply the same merge pattern to the other affected functions (getProjectNodes, getProjects, getAnalyses, etc.) to ensure caller query params are preserved.components/events/EventViewer.vue (1)
57-79: Redundantif (eventList)guard can be removed.
eventListhas typeEventLog[]with a default ofevents.value(initialized asref<EventLog[]>([])), so it is always an array and never falsy. The guard on line 66 is dead code and slightly misleads readers into thinkingundefinedis possible. Note that if the guard were removed, the outerupdatedCountsmap (with zero counts) would be unconditionally assigned tologLevelDistributions.value, which is the correct reset behavior.♻️ Proposed simplification
function updateLogLevelCounts(eventList: EventLog[] = events.value) { const updatedCounts = new Map( logLevelDistributions.value.map((dist) => [ dist.label, { ...dist, value: 0 }, ]), ); - if (eventList) { - // Count occurrences - eventList.forEach((event) => { - const tags = event.attributes?.tags || []; - updatedCounts.forEach((dist) => { - if (tags.includes(dist.label)) { - dist.value++; - } - }); - }); - logLevelDistributions.value = Array.from(updatedCounts.values()); - } + // Count occurrences + eventList.forEach((event) => { + const tags = event.attributes?.tags || []; + updatedCounts.forEach((dist) => { + if (tags.includes(dist.label)) { + dist.value++; + } + }); + }); + logLevelDistributions.value = Array.from(updatedCounts.values()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/events/EventViewer.vue` around lines 57 - 79, The guard `if (eventList)` in updateLogLevelCounts is redundant because eventList has type EventLog[] with a default of events.value (events is initialized as ref<EventLog[]>([])), so remove the `if` block and its matching braces, run the counting loop unconditionally, and always assign the reset updatedCounts to logLevelDistributions.value so the meter group is reliably reset; refer to updateLogLevelCounts, eventList, events.value, and logLevelDistributions in your changes.test/components/events/TagFilterSidePanel.spec.ts (1)
13-14:wrapper.find("button")targets the Panel toggle button instead of the clear button.The
Panelcomponent withtoggleableattribute (line 40) renders a toggle button before the explicit clear button (lines 53-61). This causeswrapper.find("button")to match the wrong element and silently test the Panel toggle instead of the clear filter functionality. Use a data-testid attribute to explicitly target the clear button.In
TagFilterSidePanel.vue, add a test identifier to the clear button:<Button icon="pi pi-filter-slash" + data-testid="clear-tag-filter" `@click`="clearFilters()" size="small" variant="text" severity="contrast" v-tooltip.top="'Clear the tag filters'" rounded />Then in the test:
- const button = wrapper.find("button"); + const button = wrapper.find("[data-testid='clear-tag-filter']");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/events/TagFilterSidePanel.spec.ts` around lines 13 - 14, The test is clicking the wrong button because wrapper.find("button") matches the Panel toggle; add a data-testid to the clear button in the TagFilterSidePanel component (e.g., data-testid="clear-filters-btn" on the Clear button element), then update the test in TagFilterSidePanel.spec.ts to target that specific element (use wrapper.find('[data-testid="clear-filters-btn"]') and trigger("click")) so the clearFilter handler/path is exercised instead of the Panel toggle; reference the Clear button markup in TagFilterSidePanel.vue and the test that currently uses wrapper.find("button").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/components/events/DateFilterGraph.spec.ts`:
- Around line 30-38: The test for DateFilterGraph may assert the
"showRequestedEvents" emit before the mocked $hubApi promise resolves; after
triggering the click (submit.trigger("click")) add an await flushPromises() to
drain pending microtasks before checking wrapper.emitted("showRequestedEvents"),
and ensure flushPromises is imported from '@vue/test-utils' at the top of the
spec so the emit assertion runs after the mocked $hubApi resolves.
In `@test/components/events/EventViewer.spec.ts`:
- Line 56: The test's exact datetime assertion is timezone- and locale-dependent
because formatTimestamp uses Intl.DateTimeFormat; replace the brittle equality
check in EventViewer.spec.ts (the expect against rowCells[0].text()) with a
stable assertion—either assert that the rendered string contains a
locale-agnostic fragment such as the time portion or date digits using
toContain, or explicitly set the test TZ by assigning process.env.TZ = 'UTC' at
the top of the spec and update the expected string; target the expectation
around the output of formatTimestamp to make the assertion deterministic.
In `@test/mockapi/handlers.ts`:
- Around line 320-326: The MSW handler in test/mockapi/handlers.ts returns an
object wrapper { status: 200, data: fakeEventResponse } which breaks getEvents()
(which uses useAPIFetch<EventLogResponse>() and expects the raw
EventLogResponse); update the `/events` handler so it returns the
EventLogResponse payload directly by calling HttpResponse.json with
fakeEventResponse (i.e., remove the outer { status, data } wrapper) so
response.value.data and response.value.meta are present as expected.
---
Outside diff comments:
In `@components/events/EventViewer.vue`:
- Around line 178-182: When replacing the events dataset in
handleShowRequestEvents, reset the DataTable filter state so downstream
consumers use the new total; inside handleShowRequestEvents (which currently
sets events.value, eventCount.value and calls updateLogLevelCounts), also clear
filteredEventCount (e.g. set filteredEventCount.value = 0 or null/undefined) so
:max="filteredEventCount || eventCount" on MeterGroup and :eventCount on
DateFilterGraph reflect the fresh eventCount rather than a stale filter value.
---
Nitpick comments:
In `@components/events/EventViewer.vue`:
- Around line 57-79: The guard `if (eventList)` in updateLogLevelCounts is
redundant because eventList has type EventLog[] with a default of events.value
(events is initialized as ref<EventLog[]>([])), so remove the `if` block and its
matching braces, run the counting loop unconditionally, and always assign the
reset updatedCounts to logLevelDistributions.value so the meter group is
reliably reset; refer to updateLogLevelCounts, eventList, events.value, and
logLevelDistributions in your changes.
In `@composables/useAPIFetch.ts`:
- Around line 29-37: The spread of ...opts before setting query in getEvents
causes any caller-provided opts.query to be overwritten; change the call to
merge default and caller query without mutating opts, e.g. keep method:"GET" and
set query: { limit: 100, ...(opts?.query || {}) } so callers can override or add
params; apply the same merge pattern to the other affected functions
(getProjectNodes, getProjects, getAnalyses, etc.) to ensure caller query params
are preserved.
In `@test/components/events/TagFilterSidePanel.spec.ts`:
- Around line 13-14: The test is clicking the wrong button because
wrapper.find("button") matches the Panel toggle; add a data-testid to the clear
button in the TagFilterSidePanel component (e.g.,
data-testid="clear-filters-btn" on the Clear button element), then update the
test in TagFilterSidePanel.spec.ts to target that specific element (use
wrapper.find('[data-testid="clear-filters-btn"]') and trigger("click")) so the
clearFilter handler/path is exercised instead of the Panel toggle; reference the
Clear button markup in TagFilterSidePanel.vue and the test that currently uses
wrapper.find("button").
In `@test/components/header/MenuHeader.spec.ts`:
- Around line 10-18: The variable MenuHeaderTestComponent is incorrectly typed
and beforeAll is unnecessarily async: change the generic on
MenuHeaderTestComponent from DefineComponent<typeof defineComponent> to
DefineComponent<typeof MenuHeader> (matching AvatarButton.spec.ts) and remove
the async modifier from the beforeAll declaration since the body only performs a
synchronous defineComponent({...}) assignment for the MenuHeader component.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
test/components/header/AvatarButton.spec.ts (2)
2-2: Remove stale development comment.
// add flushPromisesreads as a TODO/note-to-self and should be dropped from the committed test file.🧹 Proposed cleanup
-import { flushPromises, mount } from "@vue/test-utils"; // add flushPromises +import { flushPromises, mount } from "@vue/test-utils";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/header/AvatarButton.spec.ts` at line 2, Remove the stale inline development comment from the import statement in AvatarButton.spec.ts: locate the import line that reads "import { flushPromises, mount } from \"@vue/test-utils\"; // add flushPromises" and delete the trailing "// add flushPromises" comment so the import is clean and contains no TODO/note-to-self text.
16-16: Remove dead code:vi.mocked(useRuntimeConfig)with no effect.
useRuntimeConfigis already globally mocked intest/mockapi/setup.ts(registered viavitest.config.tssetupFiles). The standalonevi.mocked(useRuntimeConfig)call on line 16 does nothing—vi.mocked()is just a TypeScript type helper, and when called without chaining a method like.mockReturnValue(), it produces no side effects and the return value is discarded. This line is dead code and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/header/AvatarButton.spec.ts` at line 16, Remove the dead call vi.mocked(useRuntimeConfig) from AvatarButton.spec.ts: useRuntimeConfig is already globally mocked in test/mockapi/setup.ts via vitest setupFiles, and vi.mocked(...) here is only a TypeScript helper with no effect when not chained, so delete that standalone line and run tests to confirm nothing else relies on a local mock.test/components/header/MenuHeader.spec.ts (1)
13-18:beforeAllcallback isasyncwithout anyawait
defineComponent(...)is synchronous, so theasynckeyword is unnecessary here. Dropping it removes a false signal that async work is performed.🧹 Proposed fix
- beforeAll(async () => { + beforeAll(() => { MenuHeaderTestComponent = defineComponent({ components: { MenuHeader }, template: "<Suspense><MenuHeader/></Suspense>", }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/header/MenuHeader.spec.ts` around lines 13 - 18, The beforeAll callback is marked async but contains no awaits; remove the unnecessary async modifier from the beforeAll used to initialize MenuHeaderTestComponent (the defineComponent call that registers MenuHeader and sets template "<Suspense><MenuHeader/></Suspense>), so change the beforeAll to a synchronous function to avoid implying asynchronous work.test/components/events/EventViewer.spec.ts (1)
16-18: Prefervi.resetAllMocks()overvi.restoreAllMocks()for module mocks
vi.restoreAllMocks()is semantically intended for spies created viavi.spyOn()(restoring the original implementation). Forvi.fn()module mocks created viavi.mock(),vi.resetAllMocks()is the idiomatic choice — it clears themockResolvedValueset in the previous test, which is the actual goal here.♻️ Proposed fix
beforeEach(() => { - vi.restoreAllMocks(); + vi.resetAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/events/EventViewer.spec.ts` around lines 16 - 18, The beforeEach currently calls vi.restoreAllMocks(); change this to vi.resetAllMocks() to correctly clear module mocks created with vi.mock()/vi.fn() between tests; update the beforeEach block in EventViewer.spec.ts so that vi.resetAllMocks() is invoked instead of vi.restoreAllMocks() to ensure mockResolvedValue and similar mock state is cleared between tests.test/components/events/TagFilterSidePanel.spec.ts (1)
13-14: Use a specific selector for the clear-filter button
wrapper.find("button")relies on DOM order to find the clearFilters button rather than PrimeVue's internal Panel toggle button. If PrimeVue reorders its rendered header elements across versions, this will silently click the wrong button.♻️ Proposed fix
- const button = wrapper.find("button"); + const button = wrapper.find(".event-viewer-filter-panel-header-clear-btn button");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/events/TagFilterSidePanel.spec.ts` around lines 13 - 14, The test currently uses wrapper.find("button") which can target the wrong element; change the click target to a stable, specific selector for the clear-filters control (e.g., add and use a data-testid or an aria-label on the clearFilters button in the TagFilterSidePanel component and replace wrapper.find("button") with wrapper.find('[data-testid="clear-filters"]') or wrapper.find('[aria-label="Clear filters"]'), or use wrapper.findComponent to locate the specific PrimeVue Button instance); update the test to trigger("click") on that specific selector instead of wrapper.find("button") so it no longer relies on DOM ordering.composables/useAPIFetch.ts (1)
29-36:opts.queryis silently overwritten — merge it insteadBecause
query: { limit: 100 }is placed after...opts, any caller that passesopts = { query: { start_date: '...' } }will have that key completely replaced.DateFilterGraph.vueworks around this by calling$hubApidirectly, butgetEventsitself presents a broken passthrough contract.♻️ Proposed fix
export function getEvents(opts?) { return useAPIFetch<EventLogResponse>("/events", { ...opts, method: "GET", query: { + ...(opts?.query ?? {}), limit: 100, }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composables/useAPIFetch.ts` around lines 29 - 36, getEvents currently clobbers any caller-provided opts.query because query: { limit: 100 } is placed after ...opts; change it to merge the caller's query with the default instead. In getEvents, build the query as something like query: { limit: 100, ...(opts?.query || {}) } (or if you want to keep 100 enforced, reverse the spread to { ...(opts?.query || {}), limit: 100 }) and ensure opts is handled when undefined; update the call in getEvents so other query keys from opts aren't lost.components/events/EventViewer.vue (2)
127-147:FilterService.registeris a global side effect — move it out of the component.
FilterService.registerruns on every mount ofEventViewer, registering a PrimeVue-global filter each time. While idempotent in production, this pattern complicates unit testing (the filter must be registered before mounting the component) and is semantically wrong — a global registry entry should be set up once at app initialisation.Consider moving the registration to a Nuxt plugin or the composable layer:
// plugins/primevue-filters.ts import { FilterService } from "@primevue/core/api"; import { EventServiceTag, EventLogLevelTag } from "~/types/eventTag"; import type { EventTag } from "~/types/eventTag"; export default defineNuxtPlugin(() => { FilterService.register("tagsContainsAny", (value, filter) => { if (!filter || filter.length === 0) return true; if (!value || value.length === 0) return false; const selectedServices = filter.filter((f: EventTag) => Object.values(EventServiceTag).includes(f), ); const selectedLogLevels = filter.filter((f: EventTag) => Object.values(EventLogLevelTag).includes(f), ); const matchesService = selectedServices.length === 0 || selectedServices.some((f: EventTag) => value.includes(f)); const matchesLogLevel = selectedLogLevels.length === 0 || selectedLogLevels.some((f: EventTag) => value.includes(f)); return matchesService && matchesLogLevel; }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/events/EventViewer.vue` around lines 127 - 147, Move the PrimeVue global filter registration out of the EventViewer component: remove the FilterService.register("tagsContainsAny", ...) call from EventViewer and instead register it once at app initialization (e.g., a Nuxt plugin or a composable setup). Keep the same predicate logic and imports for EventServiceTag, EventLogLevelTag and EventTag, and ensure the global registration happens before components mount so tests and mounts don’t re-register the filter; reference the existing FilterService.register, the "tagsContainsAny" name, and the EventServiceTag/EventLogLevelTag enums when relocating the code.
57-79: Redundantif (eventList)guard — always truthy.
eventListis typed asEventLog[]and defaults toevents.value(initialized to[]), so it can never benull/undefinedunder TypeScript. The guard implies a nullish path that doesn't exist, and the placement leavesupdatedCounts(already reset to zeros) silently unused if the branch is somehow skipped.♻️ Proposed cleanup
function updateLogLevelCounts(eventList: EventLog[] = events.value) { const updatedCounts = new Map( logLevelDistributions.value.map((dist) => [ dist.label, { ...dist, value: 0 }, ]), ); - if (eventList) { - // Count occurrences - eventList.forEach((event) => { - const tags = event.attributes?.tags || []; - - updatedCounts.forEach((dist) => { - if (tags.includes(dist.label)) { - dist.value++; - } - }); - }); - logLevelDistributions.value = Array.from(updatedCounts.values()); - } + // Count occurrences + eventList.forEach((event) => { + const tags = event.attributes?.tags || []; + updatedCounts.forEach((dist) => { + if (tags.includes(dist.label)) { + dist.value++; + } + }); + }); + logLevelDistributions.value = Array.from(updatedCounts.values()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/events/EventViewer.vue` around lines 57 - 79, The guard inside updateLogLevelCounts is redundant because eventList is typed EventLog[] and defaults to events.value (initialized to []); remove the if (eventList) branch, unindent its body so updatedCounts is always used to count tags and then assign logLevelDistributions.value = Array.from(updatedCounts.values()); keep the initial reset of updatedCounts and the forEach over eventList (using event.attributes?.tags || []) and preserve the existing updatedCounts.forEach increment logic so counts are always updated even when eventList is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/components/header/MenuHeader.spec.ts`:
- Line 10: Replace the incorrect generic usage by changing the type of
MenuHeaderTestComponent from DefineComponent<typeof defineComponent> to
ReturnType<typeof defineComponent> (or remove the explicit annotation) so the
variable reflects the component instance type correctly; also remove the
unnecessary async modifier from the beforeAll block since it contains only
synchronous code. Ensure you update references to MenuHeaderTestComponent,
DefineComponent, defineComponent, and beforeAll accordingly.
In `@test/mockapi/handlers.ts`:
- Around line 35-40: The mock handler for the GET route defined with
http.get("/node/settings", ...) is returning a JSON body that includes a status
field; remove the status: 200 property so the handler returns only {
data_required: false } to match the NodeSettings interface and the real backend
shape, i.e. update the HttpResponse.json(...) call inside the /node/settings
handler to return only the data_required property (leave status codes to HTTP
headers/status, not the JSON body).
---
Duplicate comments:
In `@test/components/events/DateFilterGraph.spec.ts`:
- Around line 30-38: The test for DateFilterGraph triggers a click that calls
the async requestEvents which awaits $hubApi; to ensure the mocked promise
settles before asserting the emit, add an await flushPromises() (imported from
"flush-promises") immediately after await submit.trigger("click") and before
expect(wrapper.emitted("showRequestedEvents")).toHaveLength(1); this guarantees
the microtask from mockResolvedValue is drained and the component’s emit
(showRequestedEvents) is present; reference DateFilterGraph, requestEvents,
submit.trigger("click") and showRequestedEvents when locating where to insert
the flushPromises call.
In `@test/mockapi/handlers.ts`:
- Around line 320-326: The MSW handler http.get(`/events`) is returning an
object wrapper ({ status: 200, data: fakeEventResponse }) instead of the
expected EventLogResponse shape, which breaks consumers that expect
response.value.data to be EventLog[] and response.value.meta.count to exist;
update the handler in handlers.ts (the http.get('/events') callback) to return
the fakeEventResponse directly via HttpResponse.json so the response shape
matches the real EventLogResponse (use HttpResponse.json with the
EventLogResponse payload rather than wrapping it).
---
Nitpick comments:
In `@components/events/EventViewer.vue`:
- Around line 127-147: Move the PrimeVue global filter registration out of the
EventViewer component: remove the FilterService.register("tagsContainsAny", ...)
call from EventViewer and instead register it once at app initialization (e.g.,
a Nuxt plugin or a composable setup). Keep the same predicate logic and imports
for EventServiceTag, EventLogLevelTag and EventTag, and ensure the global
registration happens before components mount so tests and mounts don’t
re-register the filter; reference the existing FilterService.register, the
"tagsContainsAny" name, and the EventServiceTag/EventLogLevelTag enums when
relocating the code.
- Around line 57-79: The guard inside updateLogLevelCounts is redundant because
eventList is typed EventLog[] and defaults to events.value (initialized to []);
remove the if (eventList) branch, unindent its body so updatedCounts is always
used to count tags and then assign logLevelDistributions.value =
Array.from(updatedCounts.values()); keep the initial reset of updatedCounts and
the forEach over eventList (using event.attributes?.tags || []) and preserve the
existing updatedCounts.forEach increment logic so counts are always updated even
when eventList is empty.
In `@composables/useAPIFetch.ts`:
- Around line 29-36: getEvents currently clobbers any caller-provided opts.query
because query: { limit: 100 } is placed after ...opts; change it to merge the
caller's query with the default instead. In getEvents, build the query as
something like query: { limit: 100, ...(opts?.query || {}) } (or if you want to
keep 100 enforced, reverse the spread to { ...(opts?.query || {}), limit: 100 })
and ensure opts is handled when undefined; update the call in getEvents so other
query keys from opts aren't lost.
In `@test/components/events/EventViewer.spec.ts`:
- Around line 16-18: The beforeEach currently calls vi.restoreAllMocks(); change
this to vi.resetAllMocks() to correctly clear module mocks created with
vi.mock()/vi.fn() between tests; update the beforeEach block in
EventViewer.spec.ts so that vi.resetAllMocks() is invoked instead of
vi.restoreAllMocks() to ensure mockResolvedValue and similar mock state is
cleared between tests.
In `@test/components/events/TagFilterSidePanel.spec.ts`:
- Around line 13-14: The test currently uses wrapper.find("button") which can
target the wrong element; change the click target to a stable, specific selector
for the clear-filters control (e.g., add and use a data-testid or an aria-label
on the clearFilters button in the TagFilterSidePanel component and replace
wrapper.find("button") with wrapper.find('[data-testid="clear-filters"]') or
wrapper.find('[aria-label="Clear filters"]'), or use wrapper.findComponent to
locate the specific PrimeVue Button instance); update the test to
trigger("click") on that specific selector instead of wrapper.find("button") so
it no longer relies on DOM ordering.
In `@test/components/header/AvatarButton.spec.ts`:
- Line 2: Remove the stale inline development comment from the import statement
in AvatarButton.spec.ts: locate the import line that reads "import {
flushPromises, mount } from \"@vue/test-utils\"; // add flushPromises" and
delete the trailing "// add flushPromises" comment so the import is clean and
contains no TODO/note-to-self text.
- Line 16: Remove the dead call vi.mocked(useRuntimeConfig) from
AvatarButton.spec.ts: useRuntimeConfig is already globally mocked in
test/mockapi/setup.ts via vitest setupFiles, and vi.mocked(...) here is only a
TypeScript helper with no effect when not chained, so delete that standalone
line and run tests to confirm nothing else relies on a local mock.
In `@test/components/header/MenuHeader.spec.ts`:
- Around line 13-18: The beforeAll callback is marked async but contains no
awaits; remove the unnecessary async modifier from the beforeAll used to
initialize MenuHeaderTestComponent (the defineComponent call that registers
MenuHeader and sets template "<Suspense><MenuHeader/></Suspense>), so change the
beforeAll to a synchronous function to avoid implying asynchronous work.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Updates
Tests