Repository navigation
Stabilize account secret detail refresh - #75
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Playwright E2E spec that verifies switching between user-scoped secret detail views updates the UI without a full page reload, and refactors the account-secrets route to use monotonic request IDs, loading/failure keys, and retry logic to ignore stale fetches and avoid redundant loads. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(55,125,255,0.5)
participant Browser as Browser (UI)
participant Route as AccountSecrets Route
participant Queue as Render Queue / Scheduler
participant Server as API Server
end
Browser->>Route: navigate/select secret (dataKey A)
Route->>Queue: queueTask(loadAccountSecrets, dataKeyA)
Queue->>Route: run loadAccountSecrets (requestId 1)
Route->>Server: fetch /account/secrets.json?dataKey=A
Server-->>Route: respond with secrets A
alt requestId == latest && dataKey matches
Route->>Browser: update UI with secret A
Route->>Route: set lastLoadedDataKey = dataKeyA
else stale response
Route-->>Route: ignore response
end
Browser->>Route: select secret B
Route->>Queue: queueTask(loadAccountSecrets, dataKeyB)
Queue->>Route: run loadAccountSecrets (requestId 2)
Route->>Server: fetch /account/secrets.json?dataKey=B
Server-->>Route: respond with secrets B
alt requestId == latest && dataKey matches
Route->>Browser: update UI with secret B (no full reload)
Route->>Route: set lastLoadedDataKey = dataKeyB
else failure
Route->>Route: record lastFailedDataKey and schedule retry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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 |
cursor
Bot
changed the title
kentcdodds
marked this pull request as ready for review
March 28, 2026 05:03
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
force-pushed
the
cursor/secret-detail-view-update-03be
branch
from
March 28, 2026 05:05
8e6f687 to
8271f76
Compare
Contributor
|
🔎 Preview deployed: https://kody-pr-75.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/account-secrets.spec.ts (2)
63-65: Static analysis false positive - but could use exact string matching.The static analysis flagged potential ReDoS risk. In this case,
secondSecret.namecontains only alphanumeric characters and hyphens (from theDate.now().toString(36)nonce), so it's safe. However, you could avoid the regex entirely for cleaner assertions.♻️ Alternative using exact string match
- await expect(page).toHaveURL( - new RegExp(`/account/secrets/user/${secondSecret.name}$`), - ) + await expect(page).toHaveURL(`/account/secrets/user/${secondSecret.name}`)Note:
toHaveURLwith a string performs substring matching by default. If you need exact path matching, keeping the regex with$anchor is fine given the controlled input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/account-secrets.spec.ts` around lines 63 - 65, Replace the regex-based URL assertion in the toHaveURL call to avoid the ReDoS false positive by using an exact string match built from the known-safe secret name; locate the test using page.toHaveURL(...) that references secondSecret.name and change it to assert the exact path string (e.g., `/account/secrets/user/${secondSecret.name}`) so the matcher performs a direct string comparison instead of a RegExp.
3-27: Consider using Playwright's idiomatic response assertion.The
saveSecrethelper works correctly. Minor improvement: Playwright provides a built-intoBeOK()matcher for response assertions.♻️ Suggested change
- expect(response.ok()).toBeTruthy() + await expect(response).toBeOK()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/account-secrets.spec.ts` around lines 3 - 27, The helper saveSecret uses a raw assertion expect(response.ok()).toBeTruthy(); replace this with Playwright's idiomatic response matcher by asserting the Response object directly (await expect(response).toBeOK()) after the page.request.post call; update the assertion in saveSecret to use expect(response).toBeOK() so Playwright provides better diagnostics and timing, keeping the function name saveSecret and the page.request.post call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/account-secrets.spec.ts`:
- Around line 63-65: Replace the regex-based URL assertion in the toHaveURL call
to avoid the ReDoS false positive by using an exact string match built from the
known-safe secret name; locate the test using page.toHaveURL(...) that
references secondSecret.name and change it to assert the exact path string
(e.g., `/account/secrets/user/${secondSecret.name}`) so the matcher performs a
direct string comparison instead of a RegExp.
- Around line 3-27: The helper saveSecret uses a raw assertion
expect(response.ok()).toBeTruthy(); replace this with Playwright's idiomatic
response matcher by asserting the Response object directly (await
expect(response).toBeOK()) after the page.request.post call; update the
assertion in saveSecret to use expect(response).toBeOK() so Playwright provides
better diagnostics and timing, keeping the function name saveSecret and the
page.request.post call unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7719959d-e08f-4b79-8182-edf1d5fc6ec3
📒 Files selected for processing (2)
e2e/account-secrets.spec.tspackages/worker/client/routes/account-secrets.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 438-445: The code is incorrectly marking failed fetches as loaded
by setting lastLoadedDataKey in the catch path; remove or move the assignment to
lastLoadedDataKey so it is only set on successful loads (e.g., after the
successful fetch/processing code path), keep setting status = 'error' inside the
catch, and ensure the requestId/loadRequestId and
getDataRefreshKey(getCurrentHref()) checks remain to avoid racing updates
(referencing lastLoadedDataKey, status, requestId, loadRequestId,
getDataRefreshKey, getCurrentHref, dataKey).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86646ee8-d442-4e8d-ade1-978ffdc18edd
📒 Files selected for processing (2)
e2e/account-secrets.spec.tspackages/worker/client/routes/account-secrets.tsx
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/client/routes/account-secrets.tsx (1)
430-436:⚠️ Potential issue | 🟠 MajorRe-check staleness after JSON parse before applying payload.
There’s still a race window after
await readJson(...): if navigation changes during parse, stale data can be applied because no guard runs between parse completion andapplyPayload(...).💡 Suggested fix
const payload = await readJson<AccountSecretsPayload>(response) + if ( + requestId !== loadRequestId || + getDataRefreshKey(getCurrentHref()) !== dataKey + ) + return if (!response.ok || !payload?.ok) { throw new Error('Unable to load your secrets.') }🤖 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 430 - 436, After awaiting readJson(response) there is a race where navigation could change and stale data might be applied; after parsing the payload re-check that response.ok and the current dataKey still match lastLoadedDataKey/current selection before calling applyPayload. Specifically, keep the existing variables (response, dataKey, lastLoadedDataKey, payload, selection) and, once payload is parsed, validate response.ok and that dataKey still equals the expected current key (or that lastLoadedDataKey hasn't changed) and payload?.ok; only then call applyPayload(payload, selection, null), otherwise abort/return to avoid applying stale data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 701-707: The refresh gate currently treats any change between
currentDataKey and lastLoadedDataKey as eligible for retry, causing tight loops
after failures; add a failure-tracking guard (e.g., lastFailedDataKey or
failedDataKeyWithTimestamp) and update it in the fetch failure path so the retry
condition for isRefreshingForLocationChange only returns true if the
currentDataKey differs from lastLoadedDataKey AND is not the recently failed key
(or sufficient backoff time has elapsed). Concretely: change the computed guard
around status/currentDataKey/lastLoadedDataKey/loadingDataKey (the lines that
define isRefreshingForLocationChange and the if that checks
status/isRefreshingForLocationChange/isLoadingCurrentLocation) to consult the
new failure marker, and in the fetch logic set that failure marker on error and
clear it on success (and optionally record a timestamp to implement backoff).
---
Outside diff comments:
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 430-436: After awaiting readJson(response) there is a race where
navigation could change and stale data might be applied; after parsing the
payload re-check that response.ok and the current dataKey still match
lastLoadedDataKey/current selection before calling applyPayload. Specifically,
keep the existing variables (response, dataKey, lastLoadedDataKey, payload,
selection) and, once payload is parsed, validate response.ok and that dataKey
still equals the expected current key (or that lastLoadedDataKey hasn't changed)
and payload?.ok; only then call applyPayload(payload, selection, null),
otherwise abort/return to avoid applying stale data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d728e1d2-e45b-4de3-9e9a-0991c4de3290
📒 Files selected for processing (1)
packages/worker/client/routes/account-secrets.tsx
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Retry timeout race loses auto-retry for second failure
- Cleared any existing retry timeout before scheduling a new one so the latest failure always triggers an auto-retry.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/routes/account-secrets.tsx`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c572b47-409e-4adc-8030-9e0c2d30d39e
📒 Files selected for processing (1)
packages/worker/client/routes/account-secrets.tsx
Comment on lines
+721
to
+730
| 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 | ||
| ) { |
There was a problem hiding this comment.
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 === currentDataKeyThen 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.
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
2 participants
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
Testing
Summary by CodeRabbit
Tests
Bug Fixes