Skip to content

fix: standardize API error handling across services and composables #137

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.

Sign up for GitHub

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

Merged
NickGhignatti merged 4 commits into from
Mar 28, 2026
Merged

fix: standardize API error handling across services and composables #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11be756
feat: errors openapi spec
NickGhignatti Mar 26, 2026
971c1f8
feat: error handler
NickGhignatti Mar 27, 2026
52467df
fix: remove fetch for using the useApi composable
NickGhignatti Mar 28, 2026
e2776d7
fix: test with the new way of managing errors
NickGhignatti Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/audit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ jobs:
with:
node-version: 24.x
- name: Audit Root Level
run: npm audit --audit-level=high
run: |
npm ci
npm audit --audit-level=high --omit=dev
- name: Audit Client
run: cd client && npm audit --audit-level=high
- name: Audit Auth Service
Expand Down
4 changes: 2 additions & 2 deletions client/src/components/PushNotificationToast.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<script setup lang="ts">
import { usePush } from '@/composables/usePush.ts'
import { useWebPushNotifications } from '@/composables/useWebPushNotifications.ts'

import { useI18n } from 'vue-i18n'

const { t } = useI18n()
const { permission, subscribe, isSupported } = usePush()
const { permission, subscribe, isSupported } = useWebPushNotifications()

const enableAlerts = async () => {
await subscribe()
Expand Down
4 changes: 2 additions & 2 deletions client/src/components/__tests__/PushNotificationToast.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ const { subscribeMock } = vi.hoisted(() => ({ subscribeMock: vi.fn() }))
const permission = ref('default')
const isSupported = ref(true)

vi.mock('@/composables/usePush', () => ({
usePush: () => ({ permission, isSupported, subscribe: subscribeMock }),
vi.mock('@/composables/useWebPushNotifications', () => ({
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PushNotificationToast.vue imports the composable using the full path @/composables/useWebPushNotifications.ts, but this test mocks @/composables/useWebPushNotifications (no extension). Vitest module IDs are string-based, so the mock may not match and the real composable will be used. Make the mock path exactly match the component import (or remove the .ts extension from the component import for consistency).

Suggested change
vi.mock('@/composables/useWebPushNotifications', () => ({
vi.mock('@/composables/useWebPushNotifications.ts', () => ({

Copilot uses AI. Check for mistakes.
useWebPushNotifications: () => ({ permission, isSupported, subscribe: subscribeMock }),
}))

beforeEach(() => {
Expand Down
4 changes: 2 additions & 2 deletions client/src/components/cards/__tests__/DomainCard.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ vi.mock('vue-i18n', () => ({
useI18n: () => ({ t: (key: string) => key }),
}))

vi.mock('@/helpers/roles.ts', () => ({
vi.mock('@/helpers/role.ts', () => ({
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DomainCard.vue imports getRoleMeta from @/helpers/roles.ts, but this test mocks @/helpers/role.ts. The mock won't apply, and the test will use the real helper (or fail to resolve the mocked module). Update the mocked path to match the component import.

Suggested change
vi.mock('@/helpers/role.ts', () => ({
vi.mock('@/helpers/roles.ts', () => ({

Copilot uses AI. Check for mistakes.
getRoleMeta: vi.fn((role: string) => ({ i18nKey: `domains.roles.${role}` })),
}))

Expand Down Expand Up @@ -51,7 +51,7 @@ describe('DomainCard.vue', () => {
})

expect(wrapper.text()).toContain('globex')
expect(wrapper.text()).toContain('domains.roles.business_staff')
expect(wrapper.text()).toContain('domains.roles.businessStaff')
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rendered role text for business_staff is built from domains.roles.${role} (see the mock and the component’s usage), so the expectation should remain domains.roles.business_staff unless the role strings were intentionally changed everywhere. As-is, expecting domains.roles.businessStaff will fail with the current role values used throughout the app.

Suggested change
expect(wrapper.text()).toContain('domains.roles.businessStaff')
expect(wrapper.text()).toContain('domains.roles.business_staff')

Copilot uses AI. Check for mistakes.
})

it('renders the upload button only when canUpload is true', () => {
Expand Down
17 changes: 9 additions & 8 deletions ...src/composables/__tests__/usePush.spec.ts → ...__tests__/useWebPushNotifications.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, it, expect, vi, beforeEach, afterEach, type Mock } from 'vitest'
import { usePush } from '../usePush'
import { useWebPushNotifications } from '../useWebPushNotifications.ts'

describe('usePush', () => {
describe('useWebPushNotifications', () => {
const MOCK_VAPID_KEY = 'AQID'

type MockRegistration = {
Expand Down Expand Up @@ -65,7 +65,7 @@ describe('usePush', () => {
})

it('correctly detects support', () => {
const { isSupported } = usePush()
const { isSupported } = useWebPushNotifications()
expect(isSupported.value).toBe(true)
})

Expand All @@ -74,15 +74,15 @@ describe('usePush', () => {
// @ts-expect-error
delete global.window.PushManager

const { isSupported, subscribe } = usePush()
const { isSupported, subscribe } = useWebPushNotifications()
expect(isSupported.value).toBe(false)

await subscribe()
expect(global.navigator.serviceWorker.register).not.toHaveBeenCalled()
})

it('subscribes successfully (New Subscription)', async () => {
const { subscribe, isSubscribed, permission } = usePush()
const { subscribe, isSubscribed, permission } = useWebPushNotifications()

// The composable fetches VAPID key first, then posts the subscription.
const fetchMock = vi
Expand All @@ -108,6 +108,7 @@ describe('usePush', () => {
expect(fetchMock).toHaveBeenNthCalledWith(
1,
expect.stringContaining('/notification/public-key'),
expect.objectContaining({ method: 'GET' }),
)

// Check second call (Subscribe POST)
Expand All @@ -129,7 +130,7 @@ describe('usePush', () => {
})

it('handles Key Mismatch (Resubscribes)', async () => {
const { subscribe } = usePush()
const { subscribe } = useWebPushNotifications()

global.fetch = vi
.fn()
Expand All @@ -149,7 +150,7 @@ describe('usePush', () => {
})

it('handles API errors gracefully', async () => {
const { subscribe, permission } = usePush()
const { subscribe, permission } = useWebPushNotifications()

global.fetch = vi.fn().mockRejectedValue(new Error('Network Error'))

Expand All @@ -159,7 +160,7 @@ describe('usePush', () => {
})

it('handles Server Subscription failure', async () => {
const { subscribe, permission } = usePush()
const { subscribe, permission } = useWebPushNotifications()

global.fetch = vi
.fn()
Expand Down
1 change: 0 additions & 1 deletion client/src/composables/useBuildingModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ export function useBuildingModel() {
await domainsStore.fetchMemberships()
if (!domainsStore.memberships) return

// Parallel fetch — no more sequential for loop
await buildingsStore.fetch(domainsStore.memberships)

allBuildings.value = buildingsStore.all
Expand Down
29 changes: 17 additions & 12 deletions client/src/composables/usePush.ts → ...rc/composables/useWebPushNotifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { ref } from 'vue'
import { makeRequest } from '@/composables/useApi.ts'

const SERVER_URL = import.meta.env.VITE_SERVER_URL || ''

export function usePush() {
export function useWebPushNotifications() {
// Detects if the browser supports both Service Workers and the Push API
const isSupported = ref('serviceWorker' in navigator && 'PushManager' in window)
const permission = ref(Notification.permission)
const isSubscribed = ref(false)
Expand Down Expand Up @@ -31,16 +31,21 @@ export function usePush() {
if (!isSupported.value) return

try {
const keyResponse = await fetch(`${SERVER_URL}/notification/public-key`)
if (!keyResponse.ok) throw new Error('Could not fetch VAPID key')
const { publicVapidKey } = await keyResponse.json()
const keyResponse = await makeRequest(`/notification/public-key`)
const data = await keyResponse.json()

if (!keyResponse.ok){
console.log(`Failed to fetch VAPID key: ${data.type} - ${data.message}`)
return
}

const register = await navigator.serviceWorker.register('/service-worker.js')
let subscription = await register.pushManager.getSubscription()

// If is present an old subscription with different keys I have to update the server key
if (subscription) {
const currentKey = subscription.options.applicationServerKey
if (!areKeysEqual(publicVapidKey, currentKey)) {
if (!areKeysEqual(data.publicVapidKey, currentKey)) {
await subscription.unsubscribe()
subscription = null
}
Expand All @@ -49,18 +54,18 @@ export function usePush() {
if (!subscription) {
subscription = await register.pushManager.subscribe({
userVisibleOnly: true,
applicationServerKey: urlBase64ToUint8Array(publicVapidKey),
applicationServerKey: urlBase64ToUint8Array(data.publicVapidKey),
})
}

const response = await fetch(`${SERVER_URL}/notification/subscribe`, {
method: 'POST',
const response = await makeRequest(`/notification/subscribe`, 'POST', {
body: JSON.stringify(subscription),
headers: { 'Content-Type': 'application/json' },
})

if (!response.ok) {
throw new Error(`Server error: ${response.status}`)
const errorData = await response.json()
console.log(`Failed to subscribe: ${errorData.type} - ${errorData.message}`)
return
}

isSubscribed.value = true
Expand Down
23 changes: 17 additions & 6 deletions client/src/stores/__tests__/authentication.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,16 @@ describe('useAuthStore', () => {
})

it('throws when the response is not ok', async () => {
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name says it "throws when the response is not ok", but the updated assertions expect the action to resolve without throwing. Renaming this test will keep the suite accurate and self-documenting.

Suggested change
it('throws when the response is not ok', async () => {
it('resolves without throwing when the response is not ok', async () => {

Copilot uses AI. Check for mistakes.
vi.mocked(makeRequest).mockResolvedValue(makeResponse(false) as unknown as Response)
vi.mocked(makeRequest).mockResolvedValue(
makeResponse(false, {
type: 'AuthError',
message: 'User already exists',
}) as unknown as Response,
)
await expect(
useAuthStore().login('bob', 'pass'),
).resolves.toBeUndefined()
Comment on lines 87 to +96
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case name still says "throws when the response is not ok", but the updated expectation now asserts the action resolves without throwing. Renaming the test (e.g., "returns early when response is not ok") will keep the suite self-documenting and avoid confusion.

Copilot uses AI. Check for mistakes.

await expect(useAuthStore().login('alice', 'wrong')).rejects.toThrow('Login failed')
})

it('does not mutate state when the response is not ok', async () => {
Expand Down Expand Up @@ -178,11 +185,15 @@ describe('useAuthStore', () => {
})

it('throws when the response is not ok', async () => {
vi.mocked(makeRequest).mockResolvedValue(makeResponse(false) as unknown as Response)

await expect(useAuthStore().register('bob', 'bob@example.com', 'pass')).rejects.toThrow(
'Registration failed',
vi.mocked(makeRequest).mockResolvedValue(
makeResponse(false, {
type: 'AuthError',
message: 'User already exists',
}) as unknown as Response,
)
await expect(
useAuthStore().register('bob', 'bob@example.com', 'pass'),
).resolves.toBeUndefined()
Comment on lines 187 to +196
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case name still says "throws when the response is not ok", but the updated expectations now assert the action resolves without throwing. Rename the test to reflect the new contract (early return / no state mutation) to avoid misleading future readers.

Copilot uses AI. Check for mistakes.
})

it('does not mutate state when the response is not ok', async () => {
Expand Down
11 changes: 9 additions & 2 deletions client/src/stores/authentication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@ export const useAuthStore = defineStore('authentication', {
const res = await makeRequest('/auth/login', 'POST', {
body: JSON.stringify({ accountName, password }),
})
if (!res.ok) throw new Error('Login failed')
const data = await res.json()
if (!res.ok) {
console.log(`Failed to login: ${data.type} - ${data.message}`)
return;
}
this.accountName = data.account.accountName
this.isAuthenticated = true
},
Expand All @@ -28,8 +31,12 @@ export const useAuthStore = defineStore('authentication', {
const res = await makeRequest('/auth/register', 'POST', {
body: JSON.stringify(payload),
})
if (!res.ok) throw new Error('Registration failed')
const data = await res.json()

if (!res.ok) {
console.log(`Failed to register: ${data.type} - ${data.message}`)
return
}
this.accountName = data.account.accountName
this.isAuthenticated = true
},
Expand Down
4 changes: 2 additions & 2 deletions client/src/stores/domain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export const useDomainsStore = defineStore('domains', {
const { accountName } = useAuthStore()
const res = await makeRequest(`/auth/domains/${accountName}`)
const data = await res.json()
this.memberships = data.domains ?? []
this.memberships = res.ok ? (data.domains ?? []) : []
} finally {
this.loading = false
}
Expand All @@ -31,7 +31,7 @@ export const useDomainsStore = defineStore('domains', {
try {
const res = await makeRequest('/auth/domains')
const data = await res.json()
this.allDomains = data.domains ?? []
this.allDomains = res.ok ? (data.domains ?? []) : []
} finally {
this.loadingAll = false
}
Expand Down
14 changes: 8 additions & 6 deletions landing-page/api/auth.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ paths:
"201":
description: Account created successfully
"400":
description: Bad request (e.g., account already exists)
$ref: "error.yaml#/components/responses/BadRequest"
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/register in auth-service now throws ConflictError (HTTP 409) for existing accounts, but this OpenAPI doc only documents a 400 response. Add a 409/Conflict response (and define a Conflict response in error.yaml if needed) so clients and docs match runtime behavior.

Suggested change
$ref: "error.yaml#/components/responses/BadRequest"
$ref: "error.yaml#/components/responses/BadRequest"
"409":
$ref: "error.yaml#/components/responses/Conflict"

Copilot uses AI. Check for mistakes.

/login:
post:
Expand Down Expand Up @@ -158,7 +158,7 @@ paths:
account:
$ref: "#/components/schemas/Account"
"401":
description: Invalid credentials
$ref: "error.yaml#/components/responses/Unauthorized"
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAML indentation is off for the 401 response here ($ref is indented more than other response fields). As written, this will likely make the OpenAPI document invalid or move $ref under the wrong node. Align $ref indentation with the other response entries (same level as description/content under the status code).

Suggested change
$ref: "error.yaml#/components/responses/Unauthorized"
$ref: "error.yaml#/components/responses/Unauthorized"

Copilot uses AI. Check for mistakes.

/business/register:
post:
Expand Down Expand Up @@ -196,7 +196,9 @@ paths:
"201":
description: Enterprise account created
"401":
description: Unauthorized / Invalid HMAC signature
$ref: "error.yaml#/components/responses/Unauthorized"
"403":
$ref: "error.yaml#/components/responses/Forbidden"

/me:
get:
Expand All @@ -211,7 +213,7 @@ paths:
schema:
$ref: "#/components/schemas/Account"
"401":
description: Unauthorized
$ref: "error.yaml#/components/responses/Unauthorized"
Comment on lines 213 to +216
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same indentation issue for the 401 response under /me: $ref is indented too far, which can invalidate the OpenAPI YAML structure. Ensure $ref is at the correct level directly under the "401" response object.

Copilot uses AI. Check for mistakes.

Comment on lines 215 to 217
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same indentation issue for the "401" response under /me: $ref is over-indented, making the OpenAPI YAML invalid.

Copilot uses AI. Check for mistakes.
/logout:
post:
Expand Down Expand Up @@ -252,7 +254,7 @@ paths:
"201":
description: Domain created successfully
"403":
description: Forbidden (insufficient role)
$ref: "error.yaml#/components/responses/Forbidden"

/domains/{accountName}:
get:
Expand Down Expand Up @@ -317,7 +319,7 @@ paths:
"201":
description: Subdomain created successfully
"403":
description: Forbidden
$ref: "error.yaml#/components/responses/Forbidden"
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 403 response under /subdomains/{domainName} has $ref indented too far (line 322), which will break the OpenAPI YAML structure. $ref should be directly under the "403" response object with the same indentation as other status-code entries.

Suggested change
$ref: "error.yaml#/components/responses/Forbidden"
$ref: "error.yaml#/components/responses/Forbidden"

Copilot uses AI. Check for mistakes.

/domains/{accountName}/subscribe:
post:
Expand Down
Loading
Loading