Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughAdds type guards and a normalization pipeline for people entries, tightens cachified validation to require string ids, ensures cached or fresh data is normalized, and adds tests covering stale entries without ids and revalidation via downloadFile. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant getPeople as getPeople()
participant Cachified as cachified
participant GitHub as downloadFile / getFreshValue
participant Parser as YAML parser / mapPerson
Client->>getPeople: request people
getPeople->>Cachified: fetch cached value (with checkValue)
alt cached valid
Cachified-->>getPeople: cached array
getPeople->>Parser: normalizePeople(mapPerson + hasStringId)
getPeople-->>Client: normalized people
else cached missing ids / invalid
Cachified->>GitHub: call getFreshValue -> downloadFile
GitHub-->>Cachified: fresh YAML content
Cachified-->>getPeople: fresh value
getPeople->>Parser: normalizePeople(parse YAML)
getPeople-->>Client: normalized fresh people
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
🧹 Nitpick comments (3)
app/utils/__tests__/credits.server.test.ts (2)
31-38: Suppress expectedconsole.warnto keep test output clean.
mapPersoncallsconsole.warn('Had to use fallback', ...)when it falls back to a slugifiedidfor the stale entry. This is expected in this test but will appear as noise in CI logs. Adding avi.spyOn(console, 'warn').mockImplementation(() => {})suppresses it.🧹 Suggested fix
test('getPeople normalizes stale cached people with missing id values', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) const people = await getPeople({}) expect(people).toHaveLength(1) expect(people[0]).toMatchObject({ id: 'jane-doe', name: 'Jane Doe', }) + expect(warnSpy).toHaveBeenCalledOnce() + warnSpy.mockRestore() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/credits.server.test.ts` around lines 31 - 38, The test for getPeople triggers an expected console.warn from mapPerson; suppress it by spying on console.warn before the call (e.g., use vi.spyOn(console, 'warn').mockImplementation(() => {})) and restore the spy after the test (either call spy.mockRestore() or vi.restoreAllMocks()) so CI logs stay clean while the assertions on getPeople and normalized id/name remain unchanged.
10-27:checkValuepath is not tested.The mocked
cachifiedreturnsstaleCachedPeopledirectly, bypassingcheckValueandgetFreshValue. This validates the post-cachifiednormalization (line 164) but leaves thecheckValuerejection + fresh-fetch path untested. Consider adding a second test wherecachifiedis not fully mocked (orcheckValueis invoked directly) to confirm that entries missing a stringidare caught and trigger revalidation.app/utils/credits.server.ts (1)
157-164: Consider adding a clarifying comment to explain the dual-normalization pattern.
normalizePeopleis called twice: once ingetFreshValue(line 157) to ensure newly cached entries always have stringids, and again on line 164 aftercachifiedreturns. The second call guards against stale cache entries that may be returned when bothcheckValuerejects the entry andgetFreshValuethrows an error (e.g., GitHub API unavailable), providedforceFreshis true and the cache age is withinfallbackToCachebounds. Without an explanatory comment, future maintainers may incorrectly assume line 164 is redundant and remove it.📝 Suggested clarifying comment
- return normalizePeople(allPeople) + // normalizePeople is also applied here (in addition to getFreshValue) to + // handle stale cache entries returned when checkValue fails and getFreshValue + // throws, allowing cachified to fall back to stale data if within fallbackToCache. + return normalizePeople(allPeople)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/credits.server.ts` around lines 157 - 164, The code calls normalizePeople twice (once inside getFreshValue and again after cachified returns); add a concise clarifying comment immediately above the second normalizePeople(allPeople) call explaining the dual-normalization pattern: that normalizePeople is used in getFreshValue to ensure freshly cached entries have string ids, and the repeated call guards against stale cache entries returned by cachified when checkValue rejects and getFreshValue throws (e.g., GitHub API unavailable) while forceFresh/fallbackToCache semantics allow a cached fallback. Reference normalizePeople, getFreshValue, checkValue, cachified, forceFresh, and fallbackToCache in the comment so future maintainers know the rationale and not to remove the second call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/utils/__tests__/credits.server.test.ts`:
- Around line 31-38: The test for getPeople triggers an expected console.warn
from mapPerson; suppress it by spying on console.warn before the call (e.g., use
vi.spyOn(console, 'warn').mockImplementation(() => {})) and restore the spy
after the test (either call spy.mockRestore() or vi.restoreAllMocks()) so CI
logs stay clean while the assertions on getPeople and normalized id/name remain
unchanged.
In `@app/utils/credits.server.ts`:
- Around line 157-164: The code calls normalizePeople twice (once inside
getFreshValue and again after cachified returns); add a concise clarifying
comment immediately above the second normalizePeople(allPeople) call explaining
the dual-normalization pattern: that normalizePeople is used in getFreshValue to
ensure freshly cached entries have string ids, and the repeated call guards
against stale cache entries returned by cachified when checkValue rejects and
getFreshValue throws (e.g., GitHub API unavailable) while
forceFresh/fallbackToCache semantics allow a cached fallback. Reference
normalizePeople, getFreshValue, checkValue, cachified, forceFresh, and
fallbackToCache in the comment so future maintainers know the rationale and not
to remove the second call.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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 `@app/utils/credits.server.ts`:
- Around line 49-50: The current hasStringId type guard (and the id check in
mapPerson) accepts empty/whitespace-only strings, allowing invalid anchor ids
like "" that render href="#" — change the validation to require a non-empty,
trimmed string with no whitespace (and ideally match a safe anchor pattern).
Specifically, update hasStringId to check isUnknownObj(v) && isString(v.id) &&
v.id.trim().length > 0 && !/\s/.test(v.id) (or use a stricter regex such as
/^[A-Za-z][A-Za-z0-9\-_:.]*$/) and apply the same stricter check where mapPerson
currently accepts v.id so only anchor-safe ids pass.
| const hasStringId = (v: unknown): v is UnknownObj & { id: string } => | ||
| isUnknownObj(v) && isString(v.id) |
There was a problem hiding this comment.
id validation is still too permissive for anchor safety.
At Line 49 and Line 160, isString accepts empty/whitespace ids, so cached entries like { id: '' } still pass validation and can produce href="#". This misses the “valid ID” objective.
Suggested fix
-const isString = (v: unknown): v is string => typeof v === 'string'
+const isString = (v: unknown): v is string => typeof v === 'string'
+const isNonEmptyString = (v: unknown): v is string =>
+ typeof v === 'string' && v.trim().length > 0
-const hasStringId = (v: unknown): v is UnknownObj & { id: string } =>
- isUnknownObj(v) && isString(v.id)
+const hasStringId = (v: unknown): v is UnknownObj & { id: string } =>
+ isUnknownObj(v) && isNonEmptyString(v.id)And in mapPerson:
const id = getValueWithFallback(rawPerson, 'id', {
fallback: slugify(name),
- validateType: isString,
+ validateType: isNonEmptyString,
})Also applies to: 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/utils/credits.server.ts` around lines 49 - 50, The current hasStringId
type guard (and the id check in mapPerson) accepts empty/whitespace-only
strings, allowing invalid anchor ids like "" that render href="#" — change the
validation to require a non-empty, trimmed string with no whitespace (and
ideally match a safe anchor pattern). Specifically, update hasStringId to check
isUnknownObj(v) && isString(v.id) && v.id.trim().length > 0 && !/\s/.test(v.id)
(or use a stricter regex such as /^[A-Za-z][A-Za-z0-9\-_:.]*$/) and apply the
same stricter check where mapPerson currently accepts v.id so only anchor-safe
ids pass.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/utils/__tests__/credits.server.test.ts (3)
36-36: RedundantmockImplementationOnce— the base mock already returnsstaleCachedPeople.The module-level
vi.fn(async () => staleCachedPeople)at line 12 is the default implementation, so line 36 overrides with the exact same behavior. This adds noise and could mislead readers into thinking a distinct override is necessary.♻️ Proposed fix
test('getPeople normalizes stale cached people with missing id values', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) try { - vi.mocked(cachified).mockImplementationOnce(async () => staleCachedPeople) - const people = await getPeople({})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/credits.server.test.ts` at line 36, Redundant mock override: remove the unnecessary vi.mocked(cachified).mockImplementationOnce(async () => staleCachedPeople) call in app/utils/__tests__/credits.server.test.ts because the module-level mock (vi.fn(async () => staleCachedPeople)) already provides that behavior; keep the base mock and delete the mockImplementationOnce line referencing cachified to avoid noise and potential confusion.
34-46:console.warnis suppressed but never asserted — consider asserting the expected warning.Both tests call
warnSpy.mockImplementation(() => {})to suppress output, but neither assertswarnSpywas actually called. The implementation emits a warning during normalization/validation; silently swallowing it removes signal about whether that code path was exercised. A simpleexpect(warnSpy).toHaveBeenCalled()(ortoHaveBeenCalledWith(expect.stringContaining(...))) would turn the suppression into a meaningful assertion.♻️ Proposed addition for test 1
expect(people[0]).toMatchObject({ id: 'jane-doe', name: 'Jane Doe', }) + expect(warnSpy).toHaveBeenCalled() } finally {Also applies to: 50-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/credits.server.test.ts` around lines 34 - 46, The test suppresses console.warn via warnSpy but never asserts it was invoked; update the tests that mock console.warn around getPeople (the block using vi.spyOn(console, 'warn') and vi.mocked(cachified).mockImplementationOnce(..., including the other similar block at lines ~50-80) to add an assertion such as expect(warnSpy).toHaveBeenCalled() or expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('...')) after calling await getPeople({}) so the warning emission is verified while still restoring the spy in the finally block.
4-14: Usevi.hoisted()for the shared fixture to make mock hoisting explicit and safe.
vi.mock()factories are statically hoisted before module initialization.staleCachedPeopleescapes a TDZ error here only because it's referenced inside a lazy inner arrow function (async () => staleCachedPeople) rather than at factory-evaluation time — but this is an implicit, fragile dependency on that laziness. The idiomatic Vitest pattern isvi.hoisted()for any value shared between a mock factory and test bodies.♻️ Proposed refactor using
vi.hoisted()+const { staleCachedPeople } = vi.hoisted(() => ({ + staleCachedPeople: [{ name: 'Jane Doe', id: undefined }], +})) + -const staleCachedPeople = [ - { - name: 'Jane Doe', - id: undefined, - }, -] - vi.mock('@epic-web/cachified', () => ({ cachified: vi.fn(async () => staleCachedPeople), verboseReporter: vi.fn(() => undefined), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/__tests__/credits.server.test.ts` around lines 4 - 14, Replace the top-level shared fixture `staleCachedPeople` with a hoisted declaration using `vi.hoisted()` and update the `vi.mock` factory to reference that hoisted value instead of the module-scope variable; specifically, declare the fixture via `vi.hoisted(() => [ { name: 'Jane Doe', id: undefined } ])` and keep the mock of `cachified` (the `cachified: vi.fn(async () => staleCachedPeople)` factory) pointing to the hoisted identifier so the mock factory no longer depends on a variable that can trigger a TDZ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/utils/__tests__/credits.server.test.ts`:
- Line 36: Redundant mock override: remove the unnecessary
vi.mocked(cachified).mockImplementationOnce(async () => staleCachedPeople) call
in app/utils/__tests__/credits.server.test.ts because the module-level mock
(vi.fn(async () => staleCachedPeople)) already provides that behavior; keep the
base mock and delete the mockImplementationOnce line referencing cachified to
avoid noise and potential confusion.
- Around line 34-46: The test suppresses console.warn via warnSpy but never
asserts it was invoked; update the tests that mock console.warn around getPeople
(the block using vi.spyOn(console, 'warn') and
vi.mocked(cachified).mockImplementationOnce(..., including the other similar
block at lines ~50-80) to add an assertion such as
expect(warnSpy).toHaveBeenCalled() or
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('...')) after
calling await getPeople({}) so the warning emission is verified while still
restoring the spy in the finally block.
- Around line 4-14: Replace the top-level shared fixture `staleCachedPeople`
with a hoisted declaration using `vi.hoisted()` and update the `vi.mock` factory
to reference that hoisted value instead of the module-scope variable;
specifically, declare the fixture via `vi.hoisted(() => [ { name: 'Jane Doe',
id: undefined } ])` and keep the mock of `cachified` (the `cachified:
vi.fn(async () => staleCachedPeople)` factory) pointing to the hoisted
identifier so the mock factory no longer depends on a variable that can trigger
a TDZ.
2 participants
Fix broken anchor links on the credits page by ensuring all credit records have a valid ID.
The
hreffor anchor links was constructed usingperson.id, but cached credit records could sometimes be missing thisid, leading to#undefinedin the URL. This PR normalizes these records to prevent such cases.Note
Low Risk
Small, localized change to credits data normalization and cache validation; primary risk is unintended cache misses or extra revalidation if
checkValueis overly strict.Overview
Fixes broken credits-page anchor links by guaranteeing every returned person has a non-empty string
id, even whencachifiedserves stale/fallback cached data.getPeoplenow validates cache entries with a strictercheckValue(rejects arrays containing missing/invalidids) and normalizes both freshly fetched YAML and cached results via a sharednormalizePeoplepipeline that slugifiesnamewhenidis absent. Adds Vitest coverage for normalizing stale cached records and for revalidation behavior whencheckValuefails (including asserting the GitHub fetch path).Written by Cursor Bugbot for commit e5834df. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests