refactor(api): apply function style convention across api and web#67
Open
refactor(api): apply function style convention across api and web#67
Conversation
📝 WalkthroughWalkthroughConverts many non-component functions to arrow-const form across API and web, adds a Cursor rule documenting the convention, tightens several repository/service typings, adds type guards and Cloudinary URL optimization, and introduces minor HTTP status/response adjustments for some handlers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/api/errorHandler.ts (1)
30-39:⚠️ Potential issue | 🟡 MinorReplace the object cast with guard-based narrowing.
data as Record<string, unknown>can be avoided; use'message' in dataafter the object/null checks to keep the type chain safe.As per coding guidelines, "`**/*.{ts,tsx}`: Generics Over Casting: Avoid type assertions; use proper type guards to maintain the type chain."Suggested cast-free rewrite
const getBackendMessage = (data: unknown): string | null => { if (data === null || typeof data !== 'object') { return null; } - const obj = data as Record<string, unknown>; - if (typeof obj.message === 'string' && obj.message.trim() !== '') { - return obj.message; + if (!('message' in data)) { + return null; + } + const { message } = data; + if (typeof message === 'string' && message.trim() !== '') { + return message; } return null; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/errorHandler.ts` around lines 30 - 39, The getBackendMessage function currently uses a type assertion (data as Record<string, unknown>) which breaks the type chain; replace the cast with guard-based narrowing by using the existing null/object checks then using "'message' in data" and typeof checks (e.g., if ('message' in data && typeof data.message === 'string' && data.message.trim() !== '') return data.message) so you avoid any explicit casts and preserve type safety in getBackendMessage.apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts (1)
27-32:⚠️ Potential issue | 🟠 MajorAvoid suppressing explicit return-type checks on exported repository methods.
These disables weaken contract stability for a core data-access boundary. Prefer explicit method return aliases instead of turning off the rule.
As per coding guidelines, "
**/*.{ts,tsx}: Explicit Return Types: All functions, especially exported ones, must have explicit return type definitions."Also applies to: 50-51, 58-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts` around lines 27 - 32, There's an eslint rule being disabled for explicit return types on exported repository methods; remove the "eslint-disable-next-line `@typescript-eslint/explicit-module-boundary-types`" and add explicit TypeScript return type annotations for the exported methods (start with findManyAndCount) so the repository boundary has a stable contract; update findManyAndCount's signature to return a concrete type (e.g., Promise<{ items: InventoryAdjustment[]; count: number } or the inferred Prisma type aliased) and similarly add explicit return types for the other exported methods in this file (the other methods where the same disable was applied) and introduce any small type aliases/interfaces as needed to keep signatures clear.apps/web/src/api/client.ts (1)
5-23:⚠️ Potential issue | 🟠 MajorNormalize
VITE_API_URLbefore appending/api.
getApiBaseUrl()checksv.trim()but returns the raw string, so common values likehttps://api.example.com/orhttps://api.example.comcan turn into malformed request URLs such as//api/...or whitespace-prefixed URLs.💡 Proposed fix
- if (typeof v === 'string' && v.trim() !== '') return v; + if (typeof v === 'string' && v.trim() !== '') { + return v.trim().replace(/\/+$/, ''); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/client.ts` around lines 5 - 23, getApiBaseUrl currently returns the raw VITE_API_URL which can include surrounding whitespace or a trailing slash and cause malformed URLs when getApiUrl appends "/api"; update getApiBaseUrl to trim whitespace from the environment value and strip any trailing slash before returning (leave fallback to 'http://localhost:3000' unchanged) so apiBaseUrl and getApiUrl produce correct concatenated URLs; reference the functions getApiBaseUrl, apiBaseUrl, and getApiUrl when making the change.
🧹 Nitpick comments (12)
apps/api/src/modules/ecommerce/mappers/enum.mapper.test.ts (1)
25-25: Optional: tightensortedreturn type to readonly for immutability.The helper already avoids mutating inputs; returning
readonly string[]keeps the contract fully immutable.Suggested diff
-const sorted = (values: readonly string[]): string[] => [...values].sort(); +const sorted = (values: readonly string[]): readonly string[] => [...values].sort();Based on learnings: Applies to **/*.{ts,tsx} : Functional Immutability: Prefer
readonlyarrays and properties where possible. Useconstoverlet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/mappers/enum.mapper.test.ts` at line 25, The helper `sorted` currently returns a mutable `string[]`; change its signature to return a readonly array to enforce immutability (update `const sorted = (values: readonly string[]): string[] => ...` to `const sorted = (values: readonly string[]): readonly string[] => ...`) and ensure the implementation still returns a readonly (e.g., `[...]` is fine because the declared return type enforces readonly) and update any consumers if they rely on a mutable array.apps/web/src/views/PriceHistoryPage.tsx (1)
24-33: ExtractformatDateTimeto a shared util to avoid drift.This formatter is duplicated across views; centralizing it will keep date/time rendering consistent and reduce maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/views/PriceHistoryPage.tsx` around lines 24 - 33, The local formatDateTime function is duplicated across views; extract it into a shared util module and import it where needed to avoid drift. Create a new utility (e.g., export function formatDateTime(date: Date | string): string) in a common utils file, move the implementation from PriceHistoryPage.tsx into that function, update PriceHistoryPage to import and use the shared formatDateTime instead of the local definition, and remove any other duplicated copies across views so all components reference the single exported symbol.apps/web/src/views/Products/index.tsx (1)
59-62: Consider readonly inputs for helper immutability.This helper can be made safer against accidental mutation by accepting readonly parameters.
♻️ Suggested refinement
const getAttributesFromForm = ( - raw: Record<string, string>, - definitions: AttributeDefinition[] + raw: Readonly<Record<string, string>>, + definitions: readonly AttributeDefinition[] ): Record<string, string> => {Based on learnings: Applies to **/*.{ts,tsx} : Functional Immutability: Prefer
readonlyarrays and properties where possible. Useconstoverlet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/views/Products/index.tsx` around lines 59 - 62, Change getAttributesFromForm to accept readonly inputs to prevent accidental mutation: update its signature to take raw: Readonly<Record<string, string>> and definitions: readonly AttributeDefinition[] (or ReadonlyArray<AttributeDefinition>) and ensure the implementation builds and returns a new object (e.g., via reduce/Object.fromEntries) rather than mutating the inputs; update any call sites if they rely on the previous mutable types. Reference: getAttributesFromForm, raw, definitions, AttributeDefinition.apps/api/src/modules/ecommerce/product.service.ts (1)
14-15: Consider centralizingisStringRecordin a shared utility.This guard appears duplicated in other ecommerce services; extracting one shared helper will prevent divergence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/product.service.ts` around lines 14 - 15, The type guard isStringRecord is duplicated across services; extract it into a shared utility (e.g., create or add to a module like utils/typeGuards or shared/utils) and export it, then replace the local declaration in product.service.ts with an import of isStringRecord; update other ecommerce services to import the same shared helper so all callers (isStringRecord) use the centralized implementation and avoid divergence.apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts (1)
12-14: UseOrCondition[]directly to avoid type duplication.You already defined
OrCondition; reusing it here keeps the where-builder type surface single-sourced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts` around lines 12 - 14, The explicit inline type for the variable `or` duplicates the existing `OrCondition` type; replace the current declaration (the `or` const initialized to [{ sku: { product: { name: { contains: qTrim } } } }]) so it uses `OrCondition[]` as its type instead of the long inline union type, keeping the where-builder types single-sourced (update the declaration of `or` in inventory-adjustment.repository.ts to `const or: OrCondition[] = ...`).apps/api/src/modules/ecommerce/inventory-adjustment.controller.ts (1)
24-56: Add explicit return types to the exported handlers.These controller functions now rely on inferred Hono response types. Please pin the return types explicitly so the route contract stays stable.
As per coding guidelines, "Explicit Return Types: All functions, especially exported ones, must have explicit return type definitions to prevent unintended type inference."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/inventory-adjustment.controller.ts` around lines 24 - 56, Add explicit return types to the three exported handlers: listInventoryAdjustmentsHandler, getInventoryAdjustmentByIdHandler, and createInventoryAdjustmentHandler. Change their signatures to explicitly return Promise<Response> (or the appropriate Response type from your framework) instead of relying on inference, and add the necessary import for Response if missing so the route contract is pinned and type-checking stays stable.apps/web/src/api/client.ts (1)
29-55: Don't cast parsed responses toTat the fetch boundary.This helper still manufactures
Tfrom arbitrary JSON and 204 responses viaas, so callers get static certainty without any runtime validation. Returnunknownhere and narrow or validate per endpoint instead.
As per coding guidelines, "Generics Over Casting: Avoid type assertions (e.g.,as SelectedType). Use generics or proper type guards to maintain the type chain."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/client.ts` around lines 29 - 55, The request function currently asserts parsed responses to the generic type T which fabricates compile-time certainty without runtime validation; change the function signature of request to return Promise<unknown> (remove the generic <T>), stop using any `as T` casts throughout (including the 204 branch and the final JSON parse), and instead return parsed data as unknown (or undefined for 204) after JSON.parse or undefined for empty text; keep the existing error handling (mapHttpError) but ensure callers are updated to narrow/validate the unknown result per endpoint rather than relying on casting at the fetch boundary (refer to the request function and its use of JSON.parse and the 204 branch).apps/api/src/modules/ecommerce/product.repository.ts (1)
35-37: Avoid disabling explicit return types on the exported repository methods.These methods sit at a public module boundary. Letting their return shapes be inferred from the current query definition makes the contract change whenever the select/include changes.
As per coding guidelines, "Explicit Return Types: All functions, especially exported ones, must have explicit return type definitions to prevent unintended type inference."Also applies to: 63-64, 71-81, 99-109, 128-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/product.repository.ts` around lines 35 - 37, The exported productRepository methods (e.g., async create, and the other methods flagged at ranges 63-64, 71-81, 99-109, 128-129) must declare explicit return types instead of relying on inferred Prisma payloads; update each method signature to include a concrete return type (for example a Promise of the domain DTO or a Prisma.GetPayload type that matches the include/select used) and import any needed types (Product, CreateProductInput, Prisma types, or a repository-specific DTO) so the public API contract is stable and not tied to query shape changes. Ensure all exported functions in productRepository have explicit return type annotations that reflect the intended public shape.apps/api/src/modules/ecommerce/sku-price-history.repository.ts (1)
27-33: Avoid suppressing explicit return types on exported repository methods.These methods are the repository's public contract, so leaving them inferred means their shapes drift whenever the query/include changes.
As per coding guidelines, "Explicit Return Types: All functions, especially exported ones, must have explicit return type definitions to prevent unintended type inference."Also applies to: 51-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/sku-price-history.repository.ts` around lines 27 - 33, The exported skuPriceHistoryRepository methods (notably findManyAndCount) currently rely on inferred return types; add explicit return type annotations to each exported repository function to lock the public contract—e.g., declare findManyAndCount’s return type (a Promise of the specific result shape) instead of relying on inference, and do the same for the other exported method referenced (the one around lines 51-60); use the appropriate Prisma-generated types or a defined DTO/interface to express the exact return shape so future query/include changes don’t silently change the exported API.apps/api/src/modules/health/health.controller.ts (1)
4-6: Add explicit return types to the exported handlers.These route handlers now rely entirely on
c.json(...)inference. Please pin the response type so the public API surface does not drift with implementation details.
As per coding guidelines, "Explicit Return Types: All functions, especially exported ones, must have explicit return type definitions to prevent unintended type inference."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/health/health.controller.ts` around lines 4 - 6, The exported handlers healthHandler and healthLiveHandler lack explicit return types; add an explicit return type to each exported function signature (e.g., annotate both healthHandler and healthLiveHandler with the appropriate Response/Promise<Response> type used by your framework) instead of relying on c.json inference so the public API surface is pinned and cannot drift with implementation details; update the function signatures for healthHandler and healthLiveHandler accordingly.apps/api/src/modules/upload/upload.controller.ts (1)
24-41: Consider applying Cloudinary transformations for optimized delivery.The upload returns the raw
secure_urlwithout optimization transformations. As per coding guidelines, Cloudinary images should includef_auto(format),q_auto(quality), and dimension constraints (w_,h_withc_fillorc_limit).You could either:
- Apply eager transformations during upload, or
- Transform the URL before returning it to include these optimizations
♻️ Option: Transform URL before returning
+const applyTransformations = (url: string): string => { + // Insert transformations after /upload/ + return url.replace('/upload/', '/upload/f_auto,q_auto/'); +}; + const uploadBufferToCloudinary = (buffer: Buffer): Promise<{ secure_url: string }> => new Promise((resolve, reject) => { const uploadStream = cloudinary.uploader.upload_stream( { folder: 'products' }, (err, result) => { if (err) { reject(err); return; } if (!result || !result.secure_url) { reject(new Error('Cloudinary returned no URL')); return; } - resolve({ secure_url: result.secure_url }); + resolve({ secure_url: applyTransformations(result.secure_url) }); } ); Readable.from(buffer).pipe(uploadStream); });Based on learnings: "Include
f_autotransformation to automatically deliver the best format (WebP/AVIF) based on the browser for every Cloudinary image URL" and "Includeq_autotransformation to automatically adjust compression level for Cloudinary images."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/upload/upload.controller.ts` around lines 24 - 41, The uploadBufferToCloudinary function currently returns the raw secure_url; modify it to include Cloudinary optimizations by either adding eager transformations in the upload_stream options (e.g., include f_auto, q_auto, w_<px>/h_<px> with c_limit or c_fill) or programmatically transform the returned URL before resolving (inject the transformation segment such as /f_auto,q_auto,w_... ,c_limit into the result.secure_url). Update the Promise handler inside uploadBufferToCloudinary (the cloudinary.uploader.upload_stream callback and resolve path) to return the optimized URL and ensure the transformation approach handles missing result.secure_url the same way as before.apps/api/src/modules/ecommerce/sku-price-history.service.ts (1)
30-35: Good type guard implementation, consider extracting to shared utility.The
isStringRecordguard is well-implemented and correctly narrowsunknowntoRecord<string, string>. However, this identical guard is duplicated ininventory-adjustment.service.ts(lines 31-32) andproduct.service.ts(lines 14-15).♻️ Optional: Extract to shared utility
Create a shared type guard utility:
// apps/api/src/common/utils/type-guards.ts export const isStringRecord = (v: unknown): v is Record<string, string> => typeof v === 'object' && v !== null && !Array.isArray(v) && Object.values(v).every((x) => typeof x === 'string');Then import it in all three service files instead of duplicating the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/sku-price-history.service.ts` around lines 30 - 35, The isStringRecord type guard is duplicated across services; extract it into a single shared utility (e.g., export const isStringRecord = (...) => ...) and replace local definitions with imports; update files that reference the guard (where it's used alongside toListItem and where SKU attributes are validated) to import the shared isStringRecord, ensuring the exported name matches existing usages to avoid changing call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cursor/rules/function-style.mdc:
- Line 3: The "globs" entry currently uses the invalid comma-separated pattern
"*.ts,*.tsx" so the rule isn't applied recursively; replace that value with a
valid recursive pattern such as "**/*.{ts,tsx}" or change it to an array like
["**/*.ts","**/*.tsx"] in the same "globs" key to ensure the function-style rule
matches all TypeScript files across the repository.
In `@apps/api/src/modules/ecommerce/product.service.ts`:
- Line 28: The returned imageUrl is currently passed through raw (row.imageUrl
|| undefined); modify the mapper in product.service.ts that sets imageUrl to
detect Cloudinary URLs and inject the required transforms (f_auto,q_auto and
explicit dimensions with crop mode) into the Cloudinary upload path (i.e.,
insert "f_auto,q_auto,w_{WIDTH},h_{HEIGHT},c_fill" or c_limit between "/upload/"
and the rest of the URL) before returning, preserving undefined when
row.imageUrl is absent and using a shared size constant or config for
WIDTH/HEIGHT to keep values consistent across the API.
In `@eslint.config.js`:
- Around line 113-126: The file-scope ESLint disables for
'@typescript-eslint/no-unsafe-*' hide unsafe typing caused by Prisma's include
behavior; instead, update the repository/service methods in
apps/api/src/modules/ecommerce (e.g., InventoryAdjustmentService,
ProductService, SkuPriceHistoryService) to return explicitly-typed results using
Prisma payload types (e.g., Prisma.InventoryAdjustmentGetPayload<...> or
Prisma.ProductGetPayload<...>) or create a typed wrapper/helper that maps raw
Prisma results to strongly-typed DTOs, and remove the file-level rule disables;
if you must defer, keep the disables but add an inline comment and documentation
referencing the Prisma include limitation and which methods
(inventory-adjustment.service.ts, product.service.ts,
sku-price-history.service.ts) are affected.
- Around line 98-112: Remove the ESLint override block that disables explicit
return types for API controllers and instead add explicit return type
annotations to each exported handler in the listed files
(apps/api/src/modules/ecommerce/inventory-adjustment.controller.ts,
product.controller.ts, sku-price-history.controller.ts,
apps/api/src/modules/health/health.controller.ts,
apps/api/src/modules/upload/upload.controller.ts,
apps/api/src/modules/user/user.controller.ts) by declaring handlers like "export
const <handlerName> = async (c: Context): Promise<Response> => { ... }" so they
no longer rely on disabled '@typescript-eslint/explicit-function-return-type' or
'@typescript-eslint/explicit-module-boundary-types'; after updating those
exported handlers, remove the double casts in app.ts (the "as unknown as
RouteHandler<...>" workarounds) because the handlers now have correct explicit
Promise<Response> types.
---
Outside diff comments:
In `@apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts`:
- Around line 27-32: There's an eslint rule being disabled for explicit return
types on exported repository methods; remove the "eslint-disable-next-line
`@typescript-eslint/explicit-module-boundary-types`" and add explicit TypeScript
return type annotations for the exported methods (start with findManyAndCount)
so the repository boundary has a stable contract; update findManyAndCount's
signature to return a concrete type (e.g., Promise<{ items:
InventoryAdjustment[]; count: number } or the inferred Prisma type aliased) and
similarly add explicit return types for the other exported methods in this file
(the other methods where the same disable was applied) and introduce any small
type aliases/interfaces as needed to keep signatures clear.
In `@apps/web/src/api/client.ts`:
- Around line 5-23: getApiBaseUrl currently returns the raw VITE_API_URL which
can include surrounding whitespace or a trailing slash and cause malformed URLs
when getApiUrl appends "/api"; update getApiBaseUrl to trim whitespace from the
environment value and strip any trailing slash before returning (leave fallback
to 'http://localhost:3000' unchanged) so apiBaseUrl and getApiUrl produce
correct concatenated URLs; reference the functions getApiBaseUrl, apiBaseUrl,
and getApiUrl when making the change.
In `@apps/web/src/api/errorHandler.ts`:
- Around line 30-39: The getBackendMessage function currently uses a type
assertion (data as Record<string, unknown>) which breaks the type chain; replace
the cast with guard-based narrowing by using the existing null/object checks
then using "'message' in data" and typeof checks (e.g., if ('message' in data &&
typeof data.message === 'string' && data.message.trim() !== '') return
data.message) so you avoid any explicit casts and preserve type safety in
getBackendMessage.
---
Nitpick comments:
In `@apps/api/src/modules/ecommerce/inventory-adjustment.controller.ts`:
- Around line 24-56: Add explicit return types to the three exported handlers:
listInventoryAdjustmentsHandler, getInventoryAdjustmentByIdHandler, and
createInventoryAdjustmentHandler. Change their signatures to explicitly return
Promise<Response> (or the appropriate Response type from your framework) instead
of relying on inference, and add the necessary import for Response if missing so
the route contract is pinned and type-checking stays stable.
In `@apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts`:
- Around line 12-14: The explicit inline type for the variable `or` duplicates
the existing `OrCondition` type; replace the current declaration (the `or` const
initialized to [{ sku: { product: { name: { contains: qTrim } } } }]) so it uses
`OrCondition[]` as its type instead of the long inline union type, keeping the
where-builder types single-sourced (update the declaration of `or` in
inventory-adjustment.repository.ts to `const or: OrCondition[] = ...`).
In `@apps/api/src/modules/ecommerce/mappers/enum.mapper.test.ts`:
- Line 25: The helper `sorted` currently returns a mutable `string[]`; change
its signature to return a readonly array to enforce immutability (update `const
sorted = (values: readonly string[]): string[] => ...` to `const sorted =
(values: readonly string[]): readonly string[] => ...`) and ensure the
implementation still returns a readonly (e.g., `[...]` is fine because the
declared return type enforces readonly) and update any consumers if they rely on
a mutable array.
In `@apps/api/src/modules/ecommerce/product.repository.ts`:
- Around line 35-37: The exported productRepository methods (e.g., async create,
and the other methods flagged at ranges 63-64, 71-81, 99-109, 128-129) must
declare explicit return types instead of relying on inferred Prisma payloads;
update each method signature to include a concrete return type (for example a
Promise of the domain DTO or a Prisma.GetPayload type that matches the
include/select used) and import any needed types (Product, CreateProductInput,
Prisma types, or a repository-specific DTO) so the public API contract is stable
and not tied to query shape changes. Ensure all exported functions in
productRepository have explicit return type annotations that reflect the
intended public shape.
In `@apps/api/src/modules/ecommerce/product.service.ts`:
- Around line 14-15: The type guard isStringRecord is duplicated across
services; extract it into a shared utility (e.g., create or add to a module like
utils/typeGuards or shared/utils) and export it, then replace the local
declaration in product.service.ts with an import of isStringRecord; update other
ecommerce services to import the same shared helper so all callers
(isStringRecord) use the centralized implementation and avoid divergence.
In `@apps/api/src/modules/ecommerce/sku-price-history.repository.ts`:
- Around line 27-33: The exported skuPriceHistoryRepository methods (notably
findManyAndCount) currently rely on inferred return types; add explicit return
type annotations to each exported repository function to lock the public
contract—e.g., declare findManyAndCount’s return type (a Promise of the specific
result shape) instead of relying on inference, and do the same for the other
exported method referenced (the one around lines 51-60); use the appropriate
Prisma-generated types or a defined DTO/interface to express the exact return
shape so future query/include changes don’t silently change the exported API.
In `@apps/api/src/modules/ecommerce/sku-price-history.service.ts`:
- Around line 30-35: The isStringRecord type guard is duplicated across
services; extract it into a single shared utility (e.g., export const
isStringRecord = (...) => ...) and replace local definitions with imports;
update files that reference the guard (where it's used alongside toListItem and
where SKU attributes are validated) to import the shared isStringRecord,
ensuring the exported name matches existing usages to avoid changing call sites.
In `@apps/api/src/modules/health/health.controller.ts`:
- Around line 4-6: The exported handlers healthHandler and healthLiveHandler
lack explicit return types; add an explicit return type to each exported
function signature (e.g., annotate both healthHandler and healthLiveHandler with
the appropriate Response/Promise<Response> type used by your framework) instead
of relying on c.json inference so the public API surface is pinned and cannot
drift with implementation details; update the function signatures for
healthHandler and healthLiveHandler accordingly.
In `@apps/api/src/modules/upload/upload.controller.ts`:
- Around line 24-41: The uploadBufferToCloudinary function currently returns the
raw secure_url; modify it to include Cloudinary optimizations by either adding
eager transformations in the upload_stream options (e.g., include f_auto,
q_auto, w_<px>/h_<px> with c_limit or c_fill) or programmatically transform the
returned URL before resolving (inject the transformation segment such as
/f_auto,q_auto,w_... ,c_limit into the result.secure_url). Update the Promise
handler inside uploadBufferToCloudinary (the cloudinary.uploader.upload_stream
callback and resolve path) to return the optimized URL and ensure the
transformation approach handles missing result.secure_url the same way as
before.
In `@apps/web/src/api/client.ts`:
- Around line 29-55: The request function currently asserts parsed responses to
the generic type T which fabricates compile-time certainty without runtime
validation; change the function signature of request to return Promise<unknown>
(remove the generic <T>), stop using any `as T` casts throughout (including the
204 branch and the final JSON parse), and instead return parsed data as unknown
(or undefined for 204) after JSON.parse or undefined for empty text; keep the
existing error handling (mapHttpError) but ensure callers are updated to
narrow/validate the unknown result per endpoint rather than relying on casting
at the fetch boundary (refer to the request function and its use of JSON.parse
and the 204 branch).
In `@apps/web/src/views/PriceHistoryPage.tsx`:
- Around line 24-33: The local formatDateTime function is duplicated across
views; extract it into a shared util module and import it where needed to avoid
drift. Create a new utility (e.g., export function formatDateTime(date: Date |
string): string) in a common utils file, move the implementation from
PriceHistoryPage.tsx into that function, update PriceHistoryPage to import and
use the shared formatDateTime instead of the local definition, and remove any
other duplicated copies across views so all components reference the single
exported symbol.
In `@apps/web/src/views/Products/index.tsx`:
- Around line 59-62: Change getAttributesFromForm to accept readonly inputs to
prevent accidental mutation: update its signature to take raw:
Readonly<Record<string, string>> and definitions: readonly AttributeDefinition[]
(or ReadonlyArray<AttributeDefinition>) and ensure the implementation builds and
returns a new object (e.g., via reduce/Object.fromEntries) rather than mutating
the inputs; update any call sites if they rely on the previous mutable types.
Reference: getAttributesFromForm, raw, definitions, AttributeDefinition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d85e399-27a9-40db-8e99-fc0e0910e639
📒 Files selected for processing (35)
.cursor/rules/function-style.mdcapps/api/src/common/filters/error-handler.tsapps/api/src/common/guards/auth.guard.tsapps/api/src/common/interceptors/logging.tsapps/api/src/config/index.tsapps/api/src/modules/ecommerce/inventory-adjustment.controller.tsapps/api/src/modules/ecommerce/inventory-adjustment.repository.tsapps/api/src/modules/ecommerce/inventory-adjustment.service.tsapps/api/src/modules/ecommerce/mappers/enum.mapper.test.tsapps/api/src/modules/ecommerce/mappers/enum.mapper.tsapps/api/src/modules/ecommerce/product.controller.tsapps/api/src/modules/ecommerce/product.repository.tsapps/api/src/modules/ecommerce/product.service.tsapps/api/src/modules/ecommerce/sku-price-history.controller.tsapps/api/src/modules/ecommerce/sku-price-history.repository.tsapps/api/src/modules/ecommerce/sku-price-history.service.tsapps/api/src/modules/health/health.controller.tsapps/api/src/modules/upload/upload.controller.tsapps/api/src/modules/user/dto/user-response.dto.tsapps/api/src/modules/user/user.controller.tsapps/web/src/api/client.tsapps/web/src/api/errorHandler.tsapps/web/src/api/inventoryAdjustments.tsapps/web/src/api/priceHistory.tsapps/web/src/api/products.tsapps/web/src/api/upload.tsapps/web/src/components/DataBoundary.tsxapps/web/src/context/NotificationContext.tsxapps/web/src/hooks/useLocalStorage.tsapps/web/src/mocks/ecommerce/products.tsapps/web/src/ui/notification.tsapps/web/src/views/InventoryAdjustmentPage.tsxapps/web/src/views/PriceHistoryPage.tsxapps/web/src/views/Products/index.tsxeslint.config.js
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)
apps/api/src/modules/ecommerce/sku-price-history.repository.ts (1)
75-89:⚠️ Potential issue | 🟠 MajorRefetch after the SKU update so the returned relation isn't stale.
Line 83 includes
skuinrowbefore Line 85 updates that SKU'sprice, socreate()can return a payload whererow.sku.priceis still the old value even though the mutation succeeded. If the service maps that included relation into the response, clients will see inconsistent data immediately after a successful price change.🐛 Proposed fix
- const row = await tx.skuPriceHistory.create({ + const row = await tx.skuPriceHistory.create({ data: { skuId: data.skuId, oldPrice, newPrice: data.newPrice, effectiveDate, changedBy: data.changedBy, }, - include: { sku: { include: { product: true } } }, }); await tx.sku.update({ where: { id: data.skuId }, data: { price: data.newPrice }, }); - return row; + return tx.skuPriceHistory.findUniqueOrThrow({ + where: { id: row.id }, + include: { sku: { include: { product: true } } }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/sku-price-history.repository.ts` around lines 75 - 89, The create() call on tx.skuPriceHistory returns row with include: { sku: { include: { product: true } } } before you update the SKU, which can leave row.sku.price stale; after calling tx.sku.update({ where: { id: data.skuId }, data: { price: data.newPrice } }), re-fetch the SKU (e.g., via tx.sku.findUnique or tx.sku.findFirst using data.skuId with the same include for product) and replace row.sku with the fresh SKU before returning row so the returned relation reflects the updated price.
🧹 Nitpick comments (1)
apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts (1)
10-24: Good typing improvements; minor suggestion.The
OrConditionandWhereClausetypes improve type safety. However, line 18-19 duplicates theOrConditionunion inline rather than using the type alias:const or: Array< { sku: { product: { name: { contains: string } } } } | { skuId: number } > = ...Consider using the type alias for consistency:
const or: OrCondition[] = [{ sku: { product: { name: { contains: qTrim } } } }];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts` around lines 10 - 24, The local variable declaration in buildWhere duplicates the OrCondition union inline instead of reusing the OrCondition type alias; update the declaration of or inside buildWhere to use OrCondition[] (e.g., const or: OrCondition[] = [...]) so it consistently leverages the OrCondition type, leaving the rest of the logic (qTrim, id parsing, pushing skuId) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app.ts`:
- Around line 208-228: All route handlers are annotated as Promise<Response> or
Response which strips TypedResponse inference and forces unsafe casts in app.ts;
update each handler in the listed controller files (health.controller.ts,
sku-price-history.controller.ts, inventory-adjustment.controller.ts,
product.controller.ts, user.controller.ts, upload.controller.ts, and
error-handler.ts) to use RouteHandler<typeof <route>> (or append "satisfies
RouteHandler<typeof <route>>") instead of Promise<Response>/Response so the
OpenAPIHono .openapi(...) calls no longer require "as unknown as
RouteHandler<...>" casts—adjust the function signatures (e.g., healthHandler,
listPriceHistoryHandler, createProductHandler, getUserByIdHandler,
uploadHandler, errorHandler, etc.) to the RouteHandler form to restore typed
c.body()/c.json() response inference and remove the unsafe casts in app.ts.
---
Outside diff comments:
In `@apps/api/src/modules/ecommerce/sku-price-history.repository.ts`:
- Around line 75-89: The create() call on tx.skuPriceHistory returns row with
include: { sku: { include: { product: true } } } before you update the SKU,
which can leave row.sku.price stale; after calling tx.sku.update({ where: { id:
data.skuId }, data: { price: data.newPrice } }), re-fetch the SKU (e.g., via
tx.sku.findUnique or tx.sku.findFirst using data.skuId with the same include for
product) and replace row.sku with the fresh SKU before returning row so the
returned relation reflects the updated price.
---
Nitpick comments:
In `@apps/api/src/modules/ecommerce/inventory-adjustment.repository.ts`:
- Around line 10-24: The local variable declaration in buildWhere duplicates the
OrCondition union inline instead of reusing the OrCondition type alias; update
the declaration of or inside buildWhere to use OrCondition[] (e.g., const or:
OrCondition[] = [...]) so it consistently leverages the OrCondition type,
leaving the rest of the logic (qTrim, id parsing, pushing skuId) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a87c0f38-8805-405c-8ac4-8c45dba656dc
📒 Files selected for processing (14)
.cursor/rules/function-style.mdcapps/api/src/app.tsapps/api/src/lib/cloudinary.tsapps/api/src/modules/ecommerce/inventory-adjustment.controller.tsapps/api/src/modules/ecommerce/inventory-adjustment.repository.tsapps/api/src/modules/ecommerce/product.controller.tsapps/api/src/modules/ecommerce/product.repository.tsapps/api/src/modules/ecommerce/product.service.tsapps/api/src/modules/ecommerce/sku-price-history.controller.tsapps/api/src/modules/ecommerce/sku-price-history.repository.tsapps/api/src/modules/health/health.controller.tsapps/api/src/modules/upload/upload.controller.tsapps/api/src/modules/user/user.controller.tseslint.config.js
✅ Files skipped from review due to trivial changes (1)
- eslint.config.js
Comment on lines
+208
to
+228
| // Handlers are typed as Promise<Response> / Response; OpenAPIHono expects TypedResponse, so cast is required. | ||
| const api = new OpenAPIHono() | ||
| .openapi(healthRoute, healthHandler as unknown as RouteHandler<typeof healthRoute>) | ||
| .openapi(healthLiveRoute, healthLiveHandler as unknown as RouteHandler<typeof healthLiveRoute>) | ||
| .openapi(listPriceHistoryRoute, listPriceHistoryHandler as RouteHandler<typeof listPriceHistoryRoute>) | ||
| .openapi(getPriceHistoryByIdRoute, getPriceHistoryByIdHandler as RouteHandler<typeof getPriceHistoryByIdRoute>) | ||
| .openapi(createPriceHistoryRoute, createPriceHistoryHandler as RouteHandler<typeof createPriceHistoryRoute>) | ||
| .openapi(listInventoryAdjustmentsRoute, listInventoryAdjustmentsHandler as RouteHandler<typeof listInventoryAdjustmentsRoute>) | ||
| .openapi(getInventoryAdjustmentByIdRoute, getInventoryAdjustmentByIdHandler as RouteHandler<typeof getInventoryAdjustmentByIdRoute>) | ||
| .openapi(createInventoryAdjustmentRoute, createInventoryAdjustmentHandler as RouteHandler<typeof createInventoryAdjustmentRoute>) | ||
| .openapi(listProductsRoute, listProductsHandler as RouteHandler<typeof listProductsRoute>) | ||
| .openapi(getProductByIdRoute, getProductByIdHandler as RouteHandler<typeof getProductByIdRoute>) | ||
| .openapi(createProductRoute, createProductHandler as RouteHandler<typeof createProductRoute>) | ||
| .openapi(updateProductRoute, updateProductHandler as RouteHandler<typeof updateProductRoute>) | ||
| .openapi(deleteProductRoute, deleteProductHandler as RouteHandler<typeof deleteProductRoute>) | ||
| .openapi(uploadRoute, uploadHandler as RouteHandler<typeof uploadRoute>) | ||
| .openapi(listUsersRoute, listUsersHandler as RouteHandler<typeof listUsersRoute>) | ||
| .openapi(getUserByIdRoute, getUserByIdHandler as RouteHandler<typeof getUserByIdRoute>) | ||
| .openapi(createUserRoute, createUserHandler as RouteHandler<typeof createUserRoute>) | ||
| .openapi(updateUserRoute, updateUserHandler as RouteHandler<typeof updateUserRoute>) | ||
| .openapi(deleteUserRoute, deleteUserHandler as RouteHandler<typeof deleteUserRoute>) | ||
| .openapi(listPriceHistoryRoute, listPriceHistoryHandler as unknown as RouteHandler<typeof listPriceHistoryRoute>) | ||
| .openapi(getPriceHistoryByIdRoute, getPriceHistoryByIdHandler as unknown as RouteHandler<typeof getPriceHistoryByIdRoute>) | ||
| .openapi(createPriceHistoryRoute, createPriceHistoryHandler as unknown as RouteHandler<typeof createPriceHistoryRoute>) | ||
| .openapi(listInventoryAdjustmentsRoute, listInventoryAdjustmentsHandler as unknown as RouteHandler<typeof listInventoryAdjustmentsRoute>) | ||
| .openapi(getInventoryAdjustmentByIdRoute, getInventoryAdjustmentByIdHandler as unknown as RouteHandler<typeof getInventoryAdjustmentByIdRoute>) | ||
| .openapi(createInventoryAdjustmentRoute, createInventoryAdjustmentHandler as unknown as RouteHandler<typeof createInventoryAdjustmentRoute>) | ||
| .openapi(listProductsRoute, listProductsHandler as unknown as RouteHandler<typeof listProductsRoute>) | ||
| .openapi(getProductByIdRoute, getProductByIdHandler as unknown as RouteHandler<typeof getProductByIdRoute>) | ||
| .openapi(createProductRoute, createProductHandler as unknown as RouteHandler<typeof createProductRoute>) | ||
| .openapi(updateProductRoute, updateProductHandler as unknown as RouteHandler<typeof updateProductRoute>) | ||
| .openapi(deleteProductRoute, deleteProductHandler as unknown as RouteHandler<typeof deleteProductRoute>) | ||
| .openapi(uploadRoute, uploadHandler as unknown as RouteHandler<typeof uploadRoute>) | ||
| .openapi(listUsersRoute, listUsersHandler as unknown as RouteHandler<typeof listUsersRoute>) | ||
| .openapi(getUserByIdRoute, getUserByIdHandler as unknown as RouteHandler<typeof getUserByIdRoute>) | ||
| .openapi(createUserRoute, createUserHandler as unknown as RouteHandler<typeof createUserRoute>) | ||
| .openapi(updateUserRoute, updateUserHandler as unknown as RouteHandler<typeof updateUserRoute>) | ||
| .openapi(deleteUserRoute, deleteUserHandler as unknown as RouteHandler<typeof deleteUserRoute>) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For hono v4.6.0 and @hono/zod-openapiv0.19.10, what is the recommended way to type handlers soapp.openapi(route, handler)works withoutas unknown as RouteHandler? Does annotating handlers as PromiseeraseTypedResponseinference fromc.json()/c.body()?
💡 Result:
1) Typing handlers so app.openapi(route, handler) accepts them (no cast)
For @hono/zod-openapi, the intended handler type is RouteHandler<R>, which is defined to return MaybePromise<RouteConfigToTypedResponse<R>> (and only allows plain Response in some cases). So the “recommended” way is to type-check your handler against RouteHandler<typeof route>. (github.com)
Best pattern (keeps inference, avoids widening):
import { createRoute, type RouteHandler } from '@hono/zod-openapi'
const route = createRoute({ /* ... */ })
export const handler =
((c) => c.json({ ok: true }, 200)) satisfies RouteHandler<typeof route>
// later
app.openapi(route, handler)You can also annotate directly:
export const handler: RouteHandler<typeof route> = (c) => c.json({ ok: true }, 200)But satisfies is usually nicer because it checks compatibility without forcing the function’s inferred return type to widen.
2) Does Promise<Response> annotation erase TypedResponse inference?
Yes—at the handler boundary.
In Hono’s types, TypedResponse<T, U, F> is the marker that carries response-body/status/format type info. (jsr.io) If you annotate your handler as Promise<Response> (or Response), TypeScript will accept returning c.json() (because it’s still a Response), but it will widen the function’s return type to Response, so app.openapi(...) can no longer “see” the TypedResponse information for that handler’s output typing.
So: don’t annotate OpenAPI handlers as Promise<Response>; use RouteHandler<typeof route> (or satisfies it) to preserve the TypedResponse-based inference. (github.com)
Citations:
- 1: https://github.com/honojs/middleware/blob/aee369e7/packages/zod-openapi/src/index.ts
- 2: https://jsr.io/%40hono/hono/doc
- 3: https://github.com/honojs/middleware/blob/aee369e7/packages/zod-openapi/src/index.ts
🏁 Script executed:
# Find all handlers referenced in app.ts
rg -A 2 "healthHandler|healthLiveHandler|listPriceHistoryHandler|getPriceHistoryByIdHandler|createPriceHistoryHandler" apps/api/src --type ts | head -50Repository: sam821203/SalesOps
Length of output: 3043
🏁 Script executed:
# Look for handler file definitions and their return types
fd "handler" apps/api/src --type ts | head -20Repository: sam821203/SalesOps
Length of output: 231
🏁 Script executed:
# Check where handlers are imported from in app.ts
head -50 apps/api/src/app.ts | rg "import.*Handler"Repository: sam821203/SalesOps
Length of output: 303
🏁 Script executed:
# Count all handlers annotated with Promise<Response> or Response
rg "export const \w+Handler.*:\s*(async\s+)?\(.*\)\s*:\s*(Promise<)?Response" apps/api/src --type-list | wc -lRepository: sam821203/SalesOps
Length of output: 470
🏁 Script executed:
# Get a comprehensive list of all handlers and their annotations
rg "export const \w+Handler" apps/api/src -A 1 | grep -E "export const|Promise|Response" | head -40Repository: sam821203/SalesOps
Length of output: 3390
Refactor all handlers to use RouteHandler<typeof route> instead of Promise<Response> annotations.
Every handler across the codebase is explicitly annotated as Promise<Response> or Response, which erases the TypedResponse inference that @hono/zod-openapi needs. This is why the casts as unknown as RouteHandler<...> are required in app.ts—they work around a type mismatch created by the controller annotations. The casts disable compile-time verification: mismatched input schemas or undocumented response shapes will still register cleanly.
Fix by removing Promise<Response> annotations from all handlers and using RouteHandler<typeof route> instead (or satisfies RouteHandler<typeof route>). This preserves TypedResponse inference from c.json() / c.body() and allows the route chain to stay type-checked end to end.
Affected files:
apps/api/src/modules/health/health.controller.ts(2 handlers)apps/api/src/modules/ecommerce/sku-price-history.controller.ts(3 handlers)apps/api/src/modules/ecommerce/inventory-adjustment.controller.ts(3 handlers)apps/api/src/modules/ecommerce/product.controller.ts(5 handlers)apps/api/src/modules/user/user.controller.ts(6 handlers)apps/api/src/modules/upload/upload.controller.ts(1 handler)apps/api/src/common/filters/error-handler.ts(1 handler)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app.ts` around lines 208 - 228, All route handlers are annotated
as Promise<Response> or Response which strips TypedResponse inference and forces
unsafe casts in app.ts; update each handler in the listed controller files
(health.controller.ts, sku-price-history.controller.ts,
inventory-adjustment.controller.ts, product.controller.ts, user.controller.ts,
upload.controller.ts, and error-handler.ts) to use RouteHandler<typeof <route>>
(or append "satisfies RouteHandler<typeof <route>>") instead of
Promise<Response>/Response so the OpenAPIHono .openapi(...) calls no longer
require "as unknown as RouteHandler<...>" casts—adjust the function signatures
(e.g., healthHandler, listPriceHistoryHandler, createProductHandler,
getUserByIdHandler, uploadHandler, errorHandler, etc.) to the RouteHandler form
to restore typed c.body()/c.json() response inference and remove the unsafe
casts in app.ts.
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
1 participant
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
functiondeclarations) across the API and web app made the codebase harder to read and review.functiondeclarations for React components), documented it in.cursor/rules/function-style.mdc, refactored API and web code to follow it, and added ESLint rules to enforce the convention.Changes
.cursor/rules/function-style.mdcand extend ESLint config to enforce the function-style conventionScope
Select one (aligns with
commitlint.config.cjsscopes):Related
Branch: refactor/function-style
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Chores