fix(shell): add request id fallback when randomUUID is unavailable#28
fix(shell): add request id fallback when randomUUID is unavailable#28skulidropek wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesRandom ID Fallback Implementation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client as createClientEffect
participant RandomID as randomID()
participant Crypto as globalThis.crypto
Client->>RandomID: request new ID
RandomID->>Crypto: check randomUUID()
alt randomUUID available
Crypto-->>RandomID: UUID string
else getRandomValues available
RandomID->>Crypto: getRandomValues(buffer)
Crypto-->>RandomID: random bytes
else no crypto available
RandomID->>RandomID: use timestamp + counter
end
RandomID-->>Client: generated ID
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/app/src/shell/api-client/create-client-runtime-helpers.ts (2)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
"crypto" in globalThischeck.Since
globalThis.cryptosimply evaluates toundefinedwhen the property is absent, theincheck adds no behavior beyondglobalThis.cryptoitself (optional chaining is already used by callers). Can be simplified.♻️ Simplification
-const webCrypto = (): Crypto | undefined => ( - "crypto" in globalThis ? globalThis.crypto : undefined -) +const webCrypto = (): Crypto | undefined => globalThis.crypto🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/shell/api-client/create-client-runtime-helpers.ts` around lines 30 - 32, The webCrypto helper in create-client-runtime-helpers.ts is doing a redundant `"crypto" in globalThis` existence check, which can be simplified. Update the webCrypto function to rely directly on globalThis.crypto (or equivalent direct access) and keep the return type unchanged so callers of webCrypto continue to work with the same optional behavior.
24-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTruncated IDs + mod-256 counter increase collision risk; also creates hidden cross-test coupling.
fallbackRandomIdLength = 9truncates the crypto-derived randomness down to ~36 bits, andrandomIdFromClockdisambiguates same-millisecond calls with only a 256-value counter — beyond 256 fallback calls within the same millisecond, IDs collide.Separately,
fallbackRequestCounterislet-mutable module state (violates the "без мутаций" purity rule) shared across everyrandomID()call in the process. The new test atcreate-client-effect.test.ts(lines 184-206) asserts an exact"00"counter value, implicitly depending on no prior test in the file having hit the clock-fallback path first. Any reordering, parallelization, or a new test inserted earlier that stubscryptoaway will silently break this expectation.Consider deriving the counter via an injected
Ref/Effectservice (per the Layer/DI guideline) rather than module-level mutable state, and/or widening the ID or documenting the accepted collision risk.Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/shell/api-client/create-client-runtime-helpers.ts` around lines 24 - 25, The fallback ID generator in randomID/randomIdFromClock is too collision-prone and relies on shared module-level mutation, which also creates hidden test coupling. Update the helpers around fallbackRandomIdLength, fallbackRandomHexBytes, and fallbackRequestCounter so the counter/state is provided through an injected Effect/Ref or similar per-call service instead of a mutable let, and either widen the generated ID space or otherwise remove the 256-call-per-millisecond collision limit. Adjust any tests that assert a fixed counter value so they no longer depend on prior calls or global state.Source: Coding guidelines
packages/app/tests/api-client/create-client-effect.test.ts (1)
156-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNew tests don't follow the
.test.tsguideline conventions.Per repo guidelines for
**/*.test.{ts,tsx}, tests should useit.effect()/property-based (fast-check) approaches instead of manualEffect.runPromisewrapping, and should carry explicit documentation of invariants/preconditions/postconditions. These two new tests useit(...).pipe(Effect.runPromise)(consistent with the rest of the file's pre-existing style) and have no invariant/precondition/postcondition documentation. Since this is an example-based verification of exact fallback output rather than a general invariant, full property-based conversion may not fit well here, but adding the required doc fields and consideringit.effect()(from@effect/vitest) would align with the stated conventions.As per coding guidelines,
**/*.test.{ts,tsx}: "Использовать Property-Based Testing (fast-check) для проверки инвариантов. Unit-тесты должны использовать мок-зависимости и .effect() вместо async/await." and "Тесты должны содержать явную документацию инвариантов, предусловий, постусловий и вариантных функций".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/tests/api-client/create-client-effect.test.ts` around lines 156 - 206, The two new request-id fallback tests in create-client-effect.test.ts do not follow the test conventions for *.test.ts files. Update the test cases around createClientEffect and the GET("/api/auth/me") assertions to use it.effect() from `@effect/vitest` instead of wrapping with Effect.runPromise, and add the required invariant/precondition/postcondition documentation for each test. Keep the existing example-based checks for the randomUUID/getRandomValues/Date.now fallback behavior, but make the test structure conform to the repo’s .test.ts guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/shell/api-client/create-client-runtime-helpers.ts`:
- Around line 24-63: The helper functions in create-client-runtime-helpers.ts
are plain side-effecting utilities, but the review requires CORE/SHELL style
Effect-TS purity and documentation. Update the random ID helpers around
truncateRandomId, webCrypto, randomIdFromUuid, randomIdFromGetRandomValues,
randomIdFromClock, and randomID to follow the mandated functional contract: add
the required TSDoc and functional comment tags, and make the side-effecting
parts explicit in the API. If converting to Effect is too large, at minimum
isolate the impure operations behind a single shell boundary and keep the rest
pure, and note that randomID’s synchronous use in create-client-runtime.ts will
need a matching call-site change if the signature changes.
---
Nitpick comments:
In `@packages/app/src/shell/api-client/create-client-runtime-helpers.ts`:
- Around line 30-32: The webCrypto helper in create-client-runtime-helpers.ts is
doing a redundant `"crypto" in globalThis` existence check, which can be
simplified. Update the webCrypto function to rely directly on globalThis.crypto
(or equivalent direct access) and keep the return type unchanged so callers of
webCrypto continue to work with the same optional behavior.
- Around line 24-25: The fallback ID generator in randomID/randomIdFromClock is
too collision-prone and relies on shared module-level mutation, which also
creates hidden test coupling. Update the helpers around fallbackRandomIdLength,
fallbackRandomHexBytes, and fallbackRequestCounter so the counter/state is
provided through an injected Effect/Ref or similar per-call service instead of a
mutable let, and either widen the generated ID space or otherwise remove the
256-call-per-millisecond collision limit. Adjust any tests that assert a fixed
counter value so they no longer depend on prior calls or global state.
In `@packages/app/tests/api-client/create-client-effect.test.ts`:
- Around line 156-206: The two new request-id fallback tests in
create-client-effect.test.ts do not follow the test conventions for *.test.ts
files. Update the test cases around createClientEffect and the
GET("/api/auth/me") assertions to use it.effect() from `@effect/vitest` instead of
wrapping with Effect.runPromise, and add the required
invariant/precondition/postcondition documentation for each test. Keep the
existing example-based checks for the randomUUID/getRandomValues/Date.now
fallback behavior, but make the test structure conform to the repo’s .test.ts
guidelines.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f9d016c-5aa3-4ca6-89f3-78ae8e1d3cfe
📒 Files selected for processing (2)
packages/app/src/shell/api-client/create-client-runtime-helpers.tspackages/app/tests/api-client/create-client-effect.test.ts
| const fallbackRandomIdLength = 9 | ||
| const fallbackRandomHexBytes = 5 | ||
| let fallbackRequestCounter = 0 | ||
|
|
||
| const truncateRandomId = (value: string): string => value.slice(0, fallbackRandomIdLength) | ||
|
|
||
| const webCrypto = (): Crypto | undefined => ( | ||
| "crypto" in globalThis ? globalThis.crypto : undefined | ||
| ) | ||
|
|
||
| const randomIdFromUuid = (): string | undefined => { | ||
| const cryptoApi = webCrypto() | ||
| return typeof cryptoApi?.randomUUID === "function" | ||
| ? truncateRandomId(cryptoApi.randomUUID().replaceAll("-", "")) | ||
| : undefined | ||
| } | ||
|
|
||
| const randomIdFromGetRandomValues = (): string | undefined => { | ||
| const cryptoApi = webCrypto() | ||
| if (typeof cryptoApi?.getRandomValues !== "function") { | ||
| return undefined | ||
| } | ||
|
|
||
| const bytes = new Uint8Array(fallbackRandomHexBytes) | ||
| cryptoApi.getRandomValues(bytes) | ||
| return truncateRandomId(Array.from(bytes, (value) => value.toString(16).padStart(2, "0")).join("")) | ||
| } | ||
|
|
||
| const randomIdFromClock = (): string => { | ||
| const timestamp = Date.now().toString(16) | ||
| const counter = fallbackRequestCounter.toString(16).padStart(2, "0") | ||
| fallbackRequestCounter = (fallbackRequestCounter + 1) % 256 | ||
| return `${timestamp}${counter}`.slice(-fallbackRandomIdLength).padStart(fallbackRandomIdLength, "0") | ||
| } | ||
|
|
||
| export const randomID = (): string => ( | ||
| globalThis.crypto.randomUUID().replaceAll("-", "").slice(0, 9) | ||
| randomIdFromUuid() | ||
| ?? randomIdFromGetRandomValues() | ||
| ?? randomIdFromClock() | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
New helpers don't follow the mandated Effect-TS/CORE-purity house style.
All four functions (truncateRandomId, webCrypto, randomIdFromUuid, randomIdFromGetRandomValues, randomIdFromClock, randomID) perform side effects (reading globalThis.crypto, calling Date.now(), mutating fallbackRequestCounter) directly as plain functions, with no TSDoc and no functional comment tags.
As per coding guidelines, "Функции с побочными эффектами (IO, сеть, БД) должны возвращать Effect<Success, Error, Requirements>... Запрещено выбрасывать исключения; все ошибки должны быть в типе" and "Все функции должны иметь строгую TSDoc документацию с @param, @returns, @pure, @effect, @invariant, @precondition, @postcondition, @complexity, @throws (Never для effect функций)" and "Использовать функциональные комментарии с обязательными полями: CHANGE, WHY, QUOTE(ТЗ), REF, SOURCE, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT, INVARIANT, COMPLEXITY."
None of this is present here. Given the surrounding contract (randomID() is called synchronously as an object-literal value in create-client-runtime.ts), fully converting to Effect would require touching the call site too — flagging for awareness even though it's a larger, cross-cutting change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/src/shell/api-client/create-client-runtime-helpers.ts` around
lines 24 - 63, The helper functions in create-client-runtime-helpers.ts are
plain side-effecting utilities, but the review requires CORE/SHELL style
Effect-TS purity and documentation. Update the random ID helpers around
truncateRandomId, webCrypto, randomIdFromUuid, randomIdFromGetRandomValues,
randomIdFromClock, and randomID to follow the mandated functional contract: add
the required TSDoc and functional comment tags, and make the side-effecting
parts explicit in the API. If converting to Effect is too large, at minimum
isolate the impure operations behind a single shell boundary and keep the rest
pure, and note that randomID’s synchronous use in create-client-runtime.ts will
need a matching call-site change if the signature changes.
Source: Coding guidelines
1 participant
Summary
id: stringcontract unchangedgetRandomValuesand no-Web-Crypto runtimes with regressionsProof of fix
globalThis.crypto.randomUUID()existed in every browser-like environment, so older WebView/browser runtimes crashed before the first request completed.randomID()now triescrypto.randomUUID(), thencrypto.getRandomValues(), then a deterministic clock/counter fallback.typecheckandbuildalso pass.Verification
corepack pnpm --filter @prover-coder-ai/openapi-effect test -- --runInBand packages/app/tests/api-client/create-client-effect.test.tscorepack pnpm --filter @prover-coder-ai/openapi-effect typecheckcorepack pnpm --filter @prover-coder-ai/openapi-effect build