Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
82 changes: 82 additions & 0 deletions e2e/account-secrets.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { expect, test } from './playwright-utils.ts'

async function saveSecret(
page: Parameters<
Parameters<typeof test>[1]
>[0]['page'],
input: {
name: string
description: string
value: string
}
) {
const response = await page.request.post('/account/secrets.json', {
data: {
action: 'save',
name: input.name,
scope: 'user',
appId: null,
description: input.description,
value: input.value,
allowedHosts: [],
allowedCapabilities: [],
},
headers: { 'Content-Type': 'application/json' },
})
expect(response.ok()).toBeTruthy()
}

test('switching secrets updates detail view without a full reload', async ({
page,
login,
}) => {
await login()

const nonce = Date.now().toString(36)
const firstSecret = {
name: `secret-switch-a-${nonce}`,
description: `First router test secret ${nonce}`,
value: `value-a-${nonce}`,
}
const secondSecret = {
name: `secret-switch-b-${nonce}`,
description: `Second router test secret ${nonce}`,
value: `value-b-${nonce}`,
}

await saveSecret(page, firstSecret)
await saveSecret(page, secondSecret)

await page.goto(`/account/secrets/user/${firstSecret.name}`)
await expect(
page.getByRole('heading', { level: 2, name: firstSecret.name }),
).toBeVisible()
await expect(page.getByLabel('Description')).toHaveValue(firstSecret.description)

await page.evaluate(() => {
;(window as typeof window & { __secretRouteMarker?: string }).__secretRouteMarker =
'still-here'
})

await page.getByRole('button', { name: secondSecret.name }).click()

await expect(page).toHaveURL(
new RegExp(`/account/secrets/user/${secondSecret.name}$`),
)
await expect(
page.getByRole('heading', { level: 2, name: secondSecret.name }),
).toBeVisible()
await expect(page.getByLabel('Description')).toHaveValue(
secondSecret.description,
)
await expect(
page.getByPlaceholder('Enter the secret value').first(),
).toHaveValue(secondSecret.value)
await expect(
page.evaluate(
() =>
(window as typeof window & { __secretRouteMarker?: string })
.__secretRouteMarker,
),
).resolves.toBe('still-here')
})
66 changes: 56 additions & 10 deletions packages/worker/client/routes/account-secrets.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import {
parseAccountSecretId,
parseAccountSecretPath,
} from '@kody-internal/shared/account-secret-route.ts'
import { navigate, routerEvents } from '#client/client-router.tsx'
import {
navigate,
routerEvents,
} from '#client/client-router.tsx'
import { createDoubleCheck } from '#client/double-check.ts'
import {
type AccountStatus,
Expand Down Expand Up @@ -329,6 +332,10 @@ export function AccountSecretsRoute(handle: Handle) {
let submittingApprovalAction: ApprovalAction | null = null
let saveState: 'idle' | 'saving' | 'deleting' = 'idle'
let lastLoadedDataKey = ''
let lastFailedDataKey: string | null = null
let loadingDataKey: string | null = null
let loadRequestId = 0
let retryTimeout: ReturnType<typeof setTimeout> | null = null
let showSecretValue = false
const deleteSecretCheck = createDoubleCheck(handle)
const filterAppCombobox = TypeaheadCombobox(handle)
Expand Down Expand Up @@ -393,11 +400,13 @@ export function AccountSecretsRoute(handle: Handle) {
saveState = 'idle'
}

async function loadAccountSecrets(signal: AbortSignal) {
async function loadAccountSecrets() {
const href = getCurrentHref()
const selection = getSelectionState(href)
const dataKey = getDataRefreshKey(href)
const requestId = ++loadRequestId
loadingDataKey = dataKey
try {
const href = getCurrentHref()
const selection = getSelectionState(href)
lastLoadedDataKey = getDataRefreshKey(href)
const requestUrl = new URL(accountSecretsApiPath, href)
requestUrl.search = new URL(href).search
if (selection.selectedSecretId) {
Expand All @@ -409,9 +418,12 @@ export function AccountSecretsRoute(handle: Handle) {
const response = await fetch(requestUrl.toString(), {
headers: { Accept: 'application/json' },
credentials: 'include',
signal,
})
if (signal.aborted) return
if (
requestId !== loadRequestId ||
getDataRefreshKey(getCurrentHref()) !== dataKey
)
return
if (response.status === 401) {
window.location.assign('/login')
return
Expand All @@ -422,14 +434,42 @@ export function AccountSecretsRoute(handle: Handle) {
throw new Error('Unable to load your secrets.')
}

lastLoadedDataKey = dataKey
lastFailedDataKey = null
if (retryTimeout) {
clearTimeout(retryTimeout)
retryTimeout = null
}
applyPayload(payload, selection, null)
handle.update()
} catch (error) {
if (signal.aborted) return
if (
requestId !== loadRequestId ||
getDataRefreshKey(getCurrentHref()) !== dataKey
)
return
lastFailedDataKey = dataKey
status = 'error'
Comment thread
coderabbitai[bot] marked this conversation as resolved.
message =
error instanceof Error ? error.message : 'Unable to load your secrets.'
handle.update()
if (typeof window !== 'undefined') {
if (retryTimeout) {
clearTimeout(retryTimeout)
retryTimeout = null
}
retryTimeout = window.setTimeout(() => {
retryTimeout = null
if (lastFailedDataKey !== dataKey) return
if (getDataRefreshKey(getCurrentHref()) !== dataKey) return
lastFailedDataKey = null
handle.update()
}, 3000)
}
Comment thread
cursor[bot] marked this conversation as resolved.
} finally {
if (requestId === loadRequestId && loadingDataKey === dataKey) {
loadingDataKey = null
}
Comment thread
cursor[bot] marked this conversation as resolved.
}
}

Expand Down Expand Up @@ -678,10 +718,16 @@ export function AccountSecretsRoute(handle: Handle) {
},
...appOptions,
]
const currentDataKey = getDataRefreshKey(currentHref)
const isRefreshingForLocationChange =
status !== 'loading' &&
getDataRefreshKey(currentHref) !== lastLoadedDataKey
if (status === 'loading' || isRefreshingForLocationChange) {
currentDataKey !== lastLoadedDataKey &&
currentDataKey !== lastFailedDataKey
const isLoadingCurrentLocation = loadingDataKey === currentDataKey
if (
(status === 'loading' || isRefreshingForLocationChange) &&
!isLoadingCurrentLocation
) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +721 to +730

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Split “stale view” from “retry suppression” to avoid stale-detail actions after failed switch.

At Line 724-Line 725, failed-key suppression is folded into isRefreshingForLocationChange. That makes the route appear “not refreshing” immediately after a failed switch, even though currentDataKey !== lastLoadedDataKey. This can leave stale detail content actionable until retry re-runs.

💡 Suggested fix
-		const isRefreshingForLocationChange =
-			status !== 'loading' &&
-			currentDataKey !== lastLoadedDataKey &&
-			currentDataKey !== lastFailedDataKey
+		const isStaleForCurrentLocation = currentDataKey !== lastLoadedDataKey
+		const isRetrySuppressedForFailedKey = currentDataKey === lastFailedDataKey
+		const isRefreshingForLocationChange =
+			status !== 'loading' &&
+			isStaleForCurrentLocation &&
+			!isRetrySuppressedForFailedKey
 		const isLoadingCurrentLocation = loadingDataKey === currentDataKey

Then use isStaleForCurrentLocation (not isRefreshingForLocationChange) for stale-UI guards where needed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/worker/client/routes/account-secrets.tsx` around lines 721 - 730,
The current boolean folds failed-key suppression into
isRefreshingForLocationChange causing the route to stop showing as refreshing
after a failed switch; split the logic: introduce isStaleForCurrentLocation =
status !== 'loading' && currentDataKey !== lastLoadedDataKey (do NOT check
lastFailedDataKey) and keep isRefreshingForLocationChange as the variant that
additionally checks currentDataKey !== lastFailedDataKey (and loadingDataKey
check stays the same). Replace uses that guard stale-UI or stale-detail actions
to use isStaleForCurrentLocation (while leaving retry-suppression/refresh logic
using isRefreshingForLocationChange) so stale detail content is disabled until a
successful refresh re-runs; reference getDataRefreshKey, currentHref, status,
lastLoadedDataKey, lastFailedDataKey, loadingDataKey, currentDataKey,
isRefreshingForLocationChange and new isStaleForCurrentLocation when making the
change.

handle.queueTask(loadAccountSecrets)
}

Expand Down
Loading