refactor: consolidate type checking into adamantite linting#100
refactor: consolidate type checking into adamantite linting#100adelrodriguez wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
adelrodriguez
force-pushed
the
03-20-refactor_migrate_from_.oxlintrc.json_to_oxlint.config.ts
branch
from
73b3803 to
5ebe8df
Compare
Greptile SummaryThis PR consolidates type checking from standalone Key observations:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/ui/src/components/carousel.tsx
Line: 95-98
Comment:
**`reInit` listener now properly cleaned up**
The old cleanup only removed the `select` listener while the `reInit` listener (registered on line 92 in the same effect) was never removed, causing a memory/event leak on unmount or `api` changes. The new cleanup correctly removes both listeners. This is a good fix.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix lint and type is..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
adelrodriguez
force-pushed
the
03-20-refactor_migrate_from_.oxlintrc.json_to_oxlint.config.ts
branch
from
5ebe8df to
2e4e55e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73b38038d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const tooltipLabel = React.useMemo(() => { | ||
| if (hideLabel || !payload?.length) { | ||
| if (hideLabel || payload.length === 0) { |
There was a problem hiding this comment.
CustomTooltipProps inherits Recharts' tooltip contract, where payload is optional, but this change dereferences payload.length before any nullish guard. When Recharts invokes the tooltip with payload === undefined (for example before the first hover or while clearing the tooltip), ChartTooltipContent will throw instead of rendering nothing, crashing any chart that uses this shared tooltip.
Useful? React with 👍 / 👎.
|
|
||
| export default defineConfig({ | ||
| extends: [core, react, node], | ||
| ignorePatterns: ["**/*.hbs", "**/src/**/_generated", "**/*.d.ts", "**/*.gen.ts", "cli/**"], |
There was a problem hiding this comment.
This refactor removes the CLI package's standalone typecheck script, but the new root adamantite check path still skips cli/** entirely via oxlint's ignore list. That leaves cli/src/** with no automated type-check coverage at all, so regressions in the published init-now CLI will now pass both local bun run check and the Adamantite workflow unnoticed.
Useful? React with 👍 / 👎.
|
|
||
| const tooltipLabel = React.useMemo(() => { | ||
| if (hideLabel || !payload?.length) { | ||
| if (hideLabel || payload.length === 0) { |
There was a problem hiding this comment.
payload can be undefined, causing a TypeError
payload comes from recharts' TooltipContentProps<ValueType, NameType> where it is typed as Payload<ValueType, NameType>[] | undefined. The original payload?.length safely short-circuits when payload is undefined (returning undefined, which is falsy), but payload.length will throw TypeError: Cannot read properties of undefined (reading 'length') when recharts passes undefined — which it routinely does before the tooltip is active or on initial mount.
The same crash occurs on line 177 for the same reason.
| if (hideLabel || payload.length === 0) { | |
| if (hideLabel || !payload?.length) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/chart.tsx
Line: 150
Comment:
**`payload` can be `undefined`, causing a TypeError**
`payload` comes from recharts' `TooltipContentProps<ValueType, NameType>` where it is typed as `Payload<ValueType, NameType>[] | undefined`. The original `payload?.length` safely short-circuits when `payload` is `undefined` (returning `undefined`, which is falsy), but `payload.length` will throw `TypeError: Cannot read properties of undefined (reading 'length')` when recharts passes `undefined` — which it routinely does before the tooltip is active or on initial mount.
The same crash occurs on line 177 for the same reason.
```suggestion
if (hideLabel || !payload?.length) {
```
How can I resolve this? If you propose a fix, please make it concise.| }, [label, labelFormatter, payload, hideLabel, labelClassName, config, labelKey]) | ||
|
|
||
| if (!active || !payload?.length) { | ||
| if (!active || payload.length === 0) { |
There was a problem hiding this comment.
payload undefined crash as line 150
payload is Payload[] | undefined from recharts' TooltipContentProps. When payload is undefined this line throws a TypeError rather than returning null as the original !payload?.length guard did.
| if (!active || payload.length === 0) { | |
| if (!active || !payload?.length) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/chart.tsx
Line: 177
Comment:
**Same `payload` undefined crash as line 150**
`payload` is `Payload[] | undefined` from recharts' `TooltipContentProps`. When `payload` is `undefined` this line throws a TypeError rather than returning `null` as the original `!payload?.length` guard did.
```suggestion
if (!active || !payload?.length) {
```
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| const root = document.documentElement | ||
| const mediaQuery = globalThis.matchMedia?.("(prefers-color-scheme: dark)") | ||
| const mediaQuery = globalThis.matchMedia("(prefers-color-scheme: dark)") |
There was a problem hiding this comment.
matchMedia may be undefined in test environments, causing a TypeError
While useEffect only runs in the browser and SSR is safe, globalThis.matchMedia is undefined in JSDOM (the default test environment for Vitest/Jest) unless explicitly mocked. The removed ?. optional chaining would previously return undefined gracefully, making the effect a no-op. Now it throws TypeError: globalThis.matchMedia is not a function, which will crash any component test that renders ThemeProvider without setting up a matchMedia mock.
| const mediaQuery = globalThis.matchMedia("(prefers-color-scheme: dark)") | |
| const mediaQuery = globalThis.matchMedia?.("(prefers-color-scheme: dark)") | |
| if (!mediaQuery) return |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/theme.tsx
Line: 59
Comment:
**`matchMedia` may be undefined in test environments, causing a TypeError**
While `useEffect` only runs in the browser and SSR is safe, `globalThis.matchMedia` is `undefined` in JSDOM (the default test environment for Vitest/Jest) unless explicitly mocked. The removed `?.` optional chaining would previously return `undefined` gracefully, making the effect a no-op. Now it throws `TypeError: globalThis.matchMedia is not a function`, which will crash any component test that renders `ThemeProvider` without setting up a `matchMedia` mock.
```suggestion
const mediaQuery = globalThis.matchMedia?.("(prefers-color-scheme: dark)")
if (!mediaQuery) return
```
How can I resolve this? If you propose a fix, please make it concise.
adelrodriguez
force-pushed
the
03-20-refactor_migrate_from_.oxlintrc.json_to_oxlint.config.ts
branch
from
2e4e55e to
6142aca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6142acac5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const authUser = await authComponent.getAuthUser(ctx) | ||
|
|
||
| if (!authUser) { | ||
| throw new UnauthenticatedError() | ||
| } | ||
|
|
||
| if (authUser.role !== "admin") { |
There was a problem hiding this comment.
getAuthUser in auth middleware
ctx.auth.getUserIdentity() only proves the request has an auth token; it does not guarantee authComponent.getAuthUser(ctx) returns a row. If the Better Auth user has been deleted or is otherwise out of sync, this branch now dereferences authUser.role and throws a 500 instead of returning an auth error, and the sibling withAuthentication middleware will pass authUser: null into protected handlers. Keeping the old UnauthenticatedError check avoids that stale-session crash path.
Useful? React with 👍 / 👎.
| export function withRateLimiting(interval: TimeExpression, limit: number) { | ||
| return rateLimiter<AppContext>({ | ||
| keyGenerator: (c) => c.var.session?.user.id ?? findIp(c.req.raw) ?? "unknown", | ||
| keyGenerator: (c) => c.var.session?.user.id ?? findIp(c.req.raw), |
There was a problem hiding this comment.
The public /health route is the current caller of withRateLimiting, so c.var.session is always null there. findIp(c.req.raw) can also fail to resolve an address (for example in local/dev setups or behind proxies that do not forward client IPs), and hono-rate-limiter uses whatever keyGenerator returns as the store key. After removing the 'unknown' fallback, all of those anonymous requests collapse onto the same undefined bucket, so a few probes can start 429ing every other health check.
Useful? React with 👍 / 👎.
| }) { | ||
| const inputOTPContext = React.useContext(OTPInputContext) | ||
| const { char, hasFakeCaret, isActive } = inputOTPContext?.slots[index] ?? {} | ||
| const { char, hasFakeCaret, isActive } = inputOTPContext.slots[index] ?? {} |
There was a problem hiding this comment.
inputOTPContext may be null when used outside its provider
React.useContext(OTPInputContext) returns null if InputOTPSlot is rendered outside an OTPInput provider. The original inputOTPContext?.slots[index] ?? {} handled this gracefully — the ?. would short-circuit and ?? {} would provide a safe fallback.
The new inputOTPContext.slots[index] ?? {} removes the guard: if inputOTPContext is null, accessing .slots throws TypeError: Cannot read properties of null (reading 'slots'), crashing any component that renders InputOTPSlot outside the correct provider context.
| const { char, hasFakeCaret, isActive } = inputOTPContext.slots[index] ?? {} | |
| const { char, hasFakeCaret, isActive } = inputOTPContext?.slots[index] ?? {} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/input-otp.tsx
Line: 49
Comment:
**`inputOTPContext` may be `null` when used outside its provider**
`React.useContext(OTPInputContext)` returns `null` if `InputOTPSlot` is rendered outside an `OTPInput` provider. The original `inputOTPContext?.slots[index] ?? {}` handled this gracefully — the `?.` would short-circuit and `?? {}` would provide a safe fallback.
The new `inputOTPContext.slots[index] ?? {}` removes the guard: if `inputOTPContext` is `null`, accessing `.slots` throws `TypeError: Cannot read properties of null (reading 'slots')`, crashing any component that renders `InputOTPSlot` outside the correct provider context.
```suggestion
const { char, hasFakeCaret, isActive } = inputOTPContext?.slots[index] ?? {}
```
How can I resolve this? If you propose a fix, please make it concise.
adelrodriguez
force-pushed
the
03-20-refactor_migrate_from_.oxlintrc.json_to_oxlint.config.ts
branch
from
6142aca to
2844ec6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2844ec6a83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
adelrodriguez
force-pushed
the
03-20-refactor_migrate_from_.oxlintrc.json_to_oxlint.config.ts
branch
from
2844ec6 to
f25f4c6
Compare
There was a problem hiding this comment.
Pull request overview
This PR streamlines the monorepo’s developer workflow by removing standalone TypeScript typecheck commands and making bun run check (Adamantite + Oxlint) run both linting and type checking, while also normalizing TS/typings versions via Bun’s catalog.
Changes:
- Removed
typecheckscripts/tasks/CI step and consolidated type checking into Oxlint (typeAware/typeCheck) viaoxlint.config.ts. - Updated tsconfigs across apps/packages to explicitly set runtime
typesand added repo-wide dependency version pinning viacatalog:. - Various small code tweaks to satisfy stricter type/lint rules and updated toolchain versions/lockfile.
Reviewed changes
Copilot reviewed 73 out of 75 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Removes typecheck pipeline task. |
| tsconfig.json | Adds Bun types and includes oxlint.config.ts. |
| tooling/tsconfig/base.json | Sets types: [] baseline for per-package opt-in. |
| tooling/env/src/vite.ts | Refactors ensureEnv to resolve workspace dir from path or file URL. |
| packages/workflows/package.json | Drops typecheck script; switches TS version to catalog:. |
| packages/utils/tsconfig.json | Adds Bun/Node runtime typings. |
| packages/utils/src/url.ts | Simplifies URL builder signature typing. |
| packages/utils/src/singleton.ts | Makes global cache field optional for strict typing. |
| packages/utils/package.json | Removes typecheck; switches to catalog: for typings/TS. |
| packages/ui/src/components/theme.tsx | Adjusts matchMedia usage and theme logic typing. |
| packages/ui/src/components/sonner.tsx | Removes default theme fallback from useTheme() destructure. |
| packages/ui/src/components/input-otp.tsx | Tightens OTP context access to satisfy strictness. |
| packages/ui/src/components/field.tsx | Simplifies length check for stricter typing. |
| packages/ui/src/components/chart.tsx | Refactors tooltip guards (introduces a runtime risk). |
| packages/ui/src/components/carousel.tsx | Cleans up Embla event unsubscription and props. |
| packages/ui/package.json | Removes typecheck; switches TS to catalog:. |
| packages/storage/tsconfig.json | Adds Bun runtime typings. |
| packages/storage/package.json | Removes typecheck; switches to catalog:. |
| packages/payments/package.json | Removes typecheck; switches TS to catalog:. |
| packages/observability/package.json | Removes typecheck; switches TS to catalog:. |
| packages/native-ui/src/components/text.tsx | Simplifies role/aria-level assignment. |
| packages/native-ui/package.json | Removes typecheck; switches TS to catalog:. |
| packages/kv/tsconfig.json | Adds Bun runtime typings. |
| packages/kv/package.json | Removes typecheck; switches typings/TS to catalog:. |
| packages/error/package.json | Removes typecheck; switches TS to catalog:. |
| packages/env/tsconfig.json | Adds Node runtime typings. |
| packages/env/src/runtime.ts | Improves runtime env typing across environments. |
| packages/env/package.json | Removes typecheck; adds @types/node via catalog:. |
| packages/email/tsconfig.json | Adds Node runtime typings. |
| packages/email/src/client.ts | Changes batch email return value (introduces return-shape mismatch). |
| packages/email/package.json | Removes typecheck; switches typings/TS to catalog:. |
| packages/db/package.json | Removes typecheck; switches TS to catalog:. |
| packages/core/package.json | Removes typecheck; switches TS to catalog:. |
| packages/backend/tsconfig.json | Adds Node runtime typings. |
| packages/backend/src/functions/system/health.ts | Refactors handler to explicit Promise return. |
| packages/backend/src/functions/shared/convex.ts | Removes redundant authUser null checks in middleware. |
| packages/backend/src/functions/public/auth.ts | Simplifies current user return value. |
| packages/backend/package.json | Removes typecheck; adds @types/node via catalog:. |
| packages/auth/src/client/index.ts | Tightens locale/error code lookup typing. |
| packages/auth/package.json | Removes typecheck; switches TS to catalog:. |
| packages/analytics/package.json | Removes typecheck; switches TS to catalog:. |
| packages/ai/package.json | Removes typecheck; switches TS to catalog:. |
| package.json | Removes root typecheck script; adds Bun catalog; updates tool versions and bun version. |
| oxlint.config.ts | Adds consolidated Oxlint config with type-aware/type-check enabled. |
| docs/development.md | Removes mention of bun run typecheck. |
| cli/package.json | Updates typecheck script to use project tsconfig; TS via catalog:. |
| bun.lock | Updates lockfile for toolchain upgrades and catalog resolution. |
| apps/web/tsconfig.json | Removes local esModuleInterop override. |
| apps/web/package.json | Removes typecheck; TS via catalog:. |
| apps/web/astro.config.ts | Updates ensureEnv call signature. |
| apps/mobile/tsconfig.json | Adds Node runtime typings. |
| apps/mobile/src/shared/utils.ts | Replaces ts-expect-error with explicit global typing. |
| apps/mobile/package.json | Removes typecheck; adds @types/node via catalog:. |
| apps/mobile/metro.config.js | Adds oxlint disable for CommonJS module. |
| apps/mobile/babel.config.js | Adds oxlint disable for CommonJS module. |
| apps/extension/wxt.config.ts | Updates ensureEnv call signature. |
| apps/extension/tsconfig.json | Adds Chrome runtime typings. |
| apps/extension/package.json | Removes compile/typecheck; TS via catalog:. |
| apps/docs/tsconfig.json | Removes local esModuleInterop override. |
| apps/docs/package.json | Removes typecheck; TS via catalog:. |
| apps/desktop/vite.config.ts | Updates ensureEnv call signature. |
| apps/desktop/package.json | Removes typecheck; TS via catalog:. |
| apps/app/vite.config.ts | Updates ensureEnv call signature. |
| apps/app/tsconfig.json | Adds Node runtime typings. |
| apps/app/src/shared/logger.ts | Uses "window" in globalThis for environment detection. |
| apps/app/src/routes/_authenticated/index.tsx | Removes UI fallback for missing user name. |
| apps/app/package.json | Removes typecheck; typings/TS via catalog:. |
| apps/api/turbo.json | Drops ^typecheck dependency from deploy task. |
| apps/api/tsconfig.json | Adds Bun runtime typings. |
| apps/api/src/shared/middleware.ts | Simplifies session middleware and rate limit key generation. |
| apps/api/src/routes/v1/index.ts | Removes optional chaining on validated query object. |
| apps/api/package.json | Removes typecheck; typings/TS via catalog:. |
| AGENTS.md | Updates contributor guidance for new check workflow. |
| .oxlintrc.json | Removes old JSON-based Oxlint config (replaced by TS config). |
| .github/workflows/adamantite.yml | Removes separate typecheck job; ensures codegen runs before check; adds Node setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const tooltipLabel = React.useMemo(() => { | ||
| if (hideLabel || !payload?.length) { | ||
| if (hideLabel || payload.length === 0) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
payload comes from TooltipContentProps and can be undefined; accessing payload.length will throw at runtime. Consider defaulting payload = [] in the destructuring or switching back to payload?.length checks.
| if (!active || payload.length === 0) { | ||
| return null |
There was a problem hiding this comment.
Same issue here: payload may be undefined per TooltipContentProps, so payload.length can crash. Guard with payload?.length or default payload to an empty array.
| } | ||
|
|
||
| return data.data ?? [] | ||
| return data |
There was a problem hiding this comment.
batchEmails returns an array in the MOCK_RESEND branch, but this now returns data from email.batch.send, which appears to change the return shape (previously returned data.data ?? []). Please keep the return type consistent (e.g., return the array of results) to avoid breaking callers.
There was a problem hiding this comment.
💡 Codex Review
Line 18 in f25f4c6
Add codegen/typegen prerequisites back to the root check script
bun run check is now the documented local verification step (AGENTS.md:9-12, docs/development.md:21-24), but this script no longer materializes generated inputs before invoking Adamantite. On a clean checkout, that means apps/web/src/middleware.ts:2 imports #shared/internationalization/runtime.js before apps/web/package.json:11-16 has run codegen, and apps/extension/tsconfig.json:7 still references .wxt/wxt.d.ts that only apps/extension/package.json:17-18 creates via wxt prepare. As written, fresh clones and cleaned worktrees will see bun run check fail before it can lint or type-check user code.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| api.on("select", onSelect) | ||
|
|
||
| return () => { | ||
| api?.off("select", onSelect) | ||
| api.off("reInit", onSelect) |
There was a problem hiding this comment.
reInit listener now properly cleaned up
The old cleanup only removed the select listener while the reInit listener (registered on line 92 in the same effect) was never removed, causing a memory/event leak on unmount or api changes. The new cleanup correctly removes both listeners. This is a good fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/carousel.tsx
Line: 95-98
Comment:
**`reInit` listener now properly cleaned up**
The old cleanup only removed the `select` listener while the `reInit` listener (registered on line 92 in the same effect) was never removed, causing a memory/event leak on unmount or `api` changes. The new cleanup correctly removes both listeners. This is a good fix.
How can I resolve this? If you propose a fix, please make it concise.
This pull request consolidates type checking functionality into the linting process and removes standalone type checking commands across the codebase.
Key Changes
Removed standalone type checking:
typecheckscript from all package.json filestypechecktask from turbo.json pipelinetypesstep from GitHub Actions workflow.oxlintrc.jsonconfiguration fileIntegrated type checking into linting:
oxlint.config.tswithtypeAware: trueandtypeCheck: trueoptionsadamantite checkto now handle both linting and type checkingadamantite checkruns both linting and type checksDependency management:
"typescript": "catalog:")Code quality improvements:
Configuration updates:
typesfield to tsconfig files for runtime-specific typesThis change streamlines the development workflow by having a single command (
bun run check) handle both linting and type checking, while maintaining the same level of code quality assurance.