Repository navigation
Library + cross-cutting typing debt - #507
Conversation
Dead code — only consumed by its own test file. The AI system uses read_page tool to access page content directly from the database. Fixes #439 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce JsonValue, LogMetadata (strict), LogInput (caller-facing), and HttpMethod types. Replace all `any` in logger.ts, logger-browser.ts, logger-config.ts, logger-database.ts, and activity-tracker.ts. Type request params with LoggableRequest union (Next.js + Express). sanitizeData now uses unknown throughout. Fixes #443 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Page.content: any → string | null (all page content stored as text). Page.fileMetadata: Record<string, any> → Record<string, JsonValue>. Validator data params: any → Record<string, unknown>. PageTypeConfig.defaultContent: () => any → () => string | Record<string, unknown>. storage-limits existingTx: any → Drizzle transaction type. Fixes #444 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ntent-store Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Viewer imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR comprehensively hardens TypeScript typing across the codebase by replacing permissive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/src/content/page-type-validators.ts (1)
117-123:⚠️ Potential issue | 🔴 CriticalFix type error:
unknowncannot be passed tostringparameter without type narrowing.Line 119 passes
data.content(typeunknown) toisValidSheetContent(content: string)without checking the type first. Withstrict: trueenabled in tsconfig, this fails TypeScript type checking. ThevalidatePageUpdatefunction (lines 193–196) shows the correct pattern: use atypeofcheck to narrow the type tostringbefore callingisValidSheetContent.🔧 Proposed fix
case PageType.SHEET: - if (data.content) { - if (!isValidSheetContent(data.content)) { + if (typeof data.content === 'string' && data.content) { + if (!isValidSheetContent(data.content)) { errors.push('Invalid sheet content'); } }
🤖 Fix all issues with AI agents
In `@apps/web/package.json`:
- Line 132: The package.json entry for "copy-webpack-plugin" is causing CI
failures because the lockfile is out of sync; run pnpm install to regenerate
pnpm-lock.yaml and commit the updated lockfile, and ensure the dependency is
present in the lockfile. Also reorder the "copy-webpack-plugin" key in
apps/web/package.json so it is alphabetically before the eslint* entries to
match the repo's sort convention (i.e., move the "copy-webpack-plugin" line to
the correct spot in the dependencies/devDependencies list).
In `@packages/lib/src/content/page-types.config.ts`:
- Line 25: getDefaultContent's declared return is string but it currently
returns a value typed string | Record<string, unknown> (the local variable
content); add a runtime type check in getDefaultContent to ensure you only
return a string: if typeof content === 'string' return it, otherwise convert the
object branch to a string (e.g. JSON.stringify(content) or otherwise serialize)
or throw an explicit error—modify the return so the function always returns a
string and update any early returns to use the same check for the content
variable.
🧹 Nitpick comments (17)
apps/processor/src/utils/security.ts (1)
101-107: Consider importingDANGEROUS_MIME_TYPESfrom the shared lib instead of redeclaring.An identical
DANGEROUS_MIME_TYPESconstant exists inpackages/lib/src/utils/file-security.ts(lines 38-44). Re-exporting from the lib package would eliminate the duplication and ensure both stay in sync if the list ever changes.packages/lib/src/utils/file-security.ts (1)
39-45: Nit:DANGEROUS_MIME_TYPESis duplicated inapps/processor/src/utils/security.ts.The same constant (identical values and
as const) exists inapps/processor/src/utils/security.ts(lines 100-106). Since this file already lives in the shared@pagespace/libpackage, the processor could import from here to avoid drift.packages/lib/src/services/storage-limits.ts (1)
186-188: Good: eliminatesanyfor the transaction parameter. Consider extracting a type alias to reduce repetition and improve readability.♻️ Optional: extract a type alias
+type DbTransaction = Parameters<Parameters<typeof db.transaction>[0]>[0]; + export async function updateStorageUsage( userId: string, deltaBytes: number, context?: { pageId?: string; driveId?: string; eventType?: 'upload' | 'delete' | 'update' | 'reconcile'; }, - existingTx?: Parameters<Parameters<typeof db.transaction>[0]>[0] + existingTx?: DbTransaction ): Promise<void> { - const executeUpdate = async (tx: Parameters<Parameters<typeof db.transaction>[0]>[0]) => { + const executeUpdate = async (tx: DbTransaction) => {If this transaction type is used elsewhere in the codebase (e.g., other files accepting an optional
tx), consider exporting it from@pagespace/dbfor reuse.packages/lib/src/file-processing/file-processor.ts (3)
8-23:Record<string, unknown>in the union neutralises the discriminated union.Because every object is assignable to
Record<string, unknown>, including it inExtractionContentMetadatameans the compiler can never narrow toPdfExtractionMetadataorVisionExtractionMetadatavia themethoddiscriminant. This limits the value of the new interfaces for any downstream consumer.Consider replacing the catch-all with a small
TextExtractionMetadata(or similar) that covers the remainingmethodliterals ('mammoth' | 'direct' | 'text-by-extension'), plus anUnsupportedTypeMetadatafor the default branch. That way the union is fully discriminated onmethodand callers get exhaustive narrowing.♻️ Sketch
+interface TextExtractionMetadata { + method: 'mammoth' | 'direct' | 'text-by-extension'; +} + +interface UnsupportedTypeMetadata { + unsupportedType: string; +} + -type ExtractionContentMetadata = PdfExtractionMetadata | VisionExtractionMetadata | Record<string, unknown>; +type ExtractionContentMetadata = + | PdfExtractionMetadata + | VisionExtractionMetadata + | TextExtractionMetadata + | UnsupportedTypeMetadata;
25-27: OpenAI response interface doesn't cover error shapes.
OpenAIVisionResponseassumeschoicesalways exists, but a 200-with-error body (rate-limit soft errors, content-filter refusals) can return{ error: { message, type, code } }instead. The optional chaining on line 418 prevents a crash, but theasassertion silently hides the mismatch.A lightweight guard (e.g. checking
'choices' in data) before accessing the field would make this more robust.
170-175: Localmetadatavariable widens specific metadata types back toRecord<string, unknown>.On line 184 (
metadata = pdfResult.metadata) and line 243 (metadata = ocrResult.metadata), the precisely-typedPdfExtractionMetadata/VisionExtractionMetadatavalues are immediately widened toRecord<string, unknown>, losing the type information the new interfaces provide.If the union is tightened per the earlier suggestion, this variable's type can become
ExtractionContentMetadatadirectly, preserving specificity through the function body.apps/processor/src/cache/content-store.ts (2)
349-358: Typing improvement looks correct, but raw entries are not validated as objects.Line 350's change to
Record<string, Record<string, unknown>>is more honest about the parsed JSON shape. However,rawParsed[key](line 353) is assigned without verifying it's actually an object — if a corrupt metadata file has a primitive value for a preset key,metadata[preset].lastAccessed = …on line 357 would silently fail or produce unexpected serialization.This is a pre-existing concern, not introduced by this PR, so flagging as optional.
Suggested guard (optional)
for (const key of Object.keys(rawParsed)) { if (isSafePropertyKey(key) && isValidPreset(key)) { - metadata[key] = rawParsed[key]; + const val: unknown = rawParsed[key]; + if (typeof val === 'object' && val !== null) { + metadata[key] = val as Record<string, unknown>; + } } }
504-505: Date fields use a truthy check +as stringinstead of atypeofguard.Lines 504–505 check truthiness of
entry.createdAt/entry.lastAccessedand then castas string, but unlike every other field in this method (and innormalizeOriginalMetadata), they skip thetypeofcheck. If a corrupted file has a non-string truthy value (e.g., a number or boolean),new Date(...)may produce an unexpected result.For consistency with the rest of the file, prefer
typeofguards:Suggested fix
- createdAt: entry.createdAt ? new Date(entry.createdAt as string) : new Date(0), - lastAccessed: entry.lastAccessed ? new Date(entry.lastAccessed as string) : new Date(0) + createdAt: typeof entry.createdAt === 'string' ? new Date(entry.createdAt) : new Date(0), + lastAccessed: typeof entry.lastAccessed === 'string' ? new Date(entry.lastAccessed) : new Date(0)apps/web/src/components/ai/ui/confirmation.tsx (1)
15-41: Duplicate union variant inToolUIPartApproval.Lines 26-30 and 31-35 define the same variant
{ id: string; approved: true; reason?: string }twice. One can be removed without any behavioral change.♻️ Suggested diff
type ToolUIPartApproval = | { id: string; approved?: never; reason?: never; } | { id: string; approved: boolean; reason?: string; } | { id: string; approved: true; reason?: string; } - | { - id: string; - approved: true; - reason?: string; - } | { id: string; approved: false; reason?: string; } | undefined;apps/web/src/components/ai/ui/tool.tsx (1)
38-49: UseRecord<ExtendedToolState, ReactNode>for exhaustive key checking.
Record<string, ReactNode>accepts any string and won't flag if a newExtendedToolStatevariant is added without a corresponding icon. All seven current variants (input-streaming,input-available,approval-requested,approval-responded,output-available,output-error,output-denied) are covered in the icon map. Switching toRecord<ExtendedToolState, ReactNode>ensures TypeScript enforces exhaustive coverage when the union grows, preventing future omissions at compile-time.Note: With the stricter type, the fallback on line 49 becomes unreachable code and can be simplified to just
return icons[status];♻️ Suggested diff
- const icons: Record<string, ReactNode> = { + const icons: Record<ExtendedToolState, ReactNode> = { "input-streaming": <CircleIcon className="size-4 text-muted-foreground" />, "input-available": <ClockIcon className="size-4 text-primary animate-pulse" />, "approval-requested": <ClockIcon className="size-4 text-yellow-600" />, "approval-responded": <CheckCircleIcon className="size-4 text-blue-600" />, "output-available": <CheckCircleIcon className="size-4 text-green-600" />, "output-error": <XCircleIcon className="size-4 text-red-600" />, "output-denied": <XCircleIcon className="size-4 text-orange-600" />, }; - return icons[status] || <CircleIcon className="size-4 text-muted-foreground" />; + return icons[status];apps/desktop/src/main/mcp-manager.ts (1)
172-183: LGTM on the error typing change. Thecatch (error: unknown)withhasErrorCodeis correct.However, note that
getErrorMessage(error)is used here (line 180) but other catch blocks in this same file (e.g.,saveConfigat line 206,startServerat line 395,executeToolat line 1056) still inline the sameerror instanceof Error ? error.message : String(error)pattern. Consider usinggetErrorMessageconsistently throughout the file.apps/desktop/src/main/index.ts (1)
28-28: Pre-existingas anyonelectron-store.Line 28 has
as anywhich conflicts with the coding guideline "Never useanytypes." This isn't introduced by this PR, but since the PR is specifically about removinganytypes, it might be worth addressing here. The comment says it works around electron-store v10 type definitions.apps/desktop/src/main/error-utils.ts (1)
9-11: Redundant cast inhasErrorCode.After
isNodeError(error)narrows the type toError & { code?: string }, the explicit(error as Error & { code?: string })cast is unnecessary —error.codeis already valid.♻️ Suggested simplification
export function hasErrorCode(error: unknown, code: string): boolean { - return isNodeError(error) && (error as Error & { code?: string }).code === code; + return isNodeError(error) && error.code === code; }packages/lib/src/logging/logger-database.ts (2)
120-142: Unsafe cast: narrow the parameter type instead of casting.
metrics.methodis typed asstring(line 122) but cast toHttpMethodon line 142. This defeats the purpose of introducing theHttpMethodtype — callers can still pass arbitrary strings. Consider typing the parameter asHttpMethoddirectly.♻️ Suggested fix
export async function writeApiMetrics(metrics: { endpoint: string; - method: string; + method: HttpMethod; statusCode: number; ...
264-291: Same unsafe cast pattern inwriteError.
error.methodisstring?(line 272) but cast toHttpMethodon line 291. Same recommendation: type the parameter asHttpMethodto get compile-time safety at call sites.packages/lib/src/logging/logger-browser.ts (2)
190-194: Type assertions aftersanitizeDataare a pragmatic trade-off — consider a brief inline comment.
sanitizeDatareturnsunknown, so theas LogContext(line 190) andas LogInput(line 194) casts are required to satisfy the typed fields. These are safe in practice becausesanitizeDatapreserves the top-level object structure (it only truncates strings and caps nesting depth), but technically the assertion could mask a shape mismatch if sanitization ever replaces a top-level object with a string (e.g.,'[Object: max depth exceeded]'). Since context and metadata are always passed atdepth=0, this cannot happen today.A one-line
// safe: top-level objects are always preserved by sanitizeDatawould document the invariant for future readers.
304-320:errorandfatalmethods: identical discrimination logic — consider extracting a helper.The
errorandfatalmethods share the exact sameinstanceof Errordiscrimination block (lines 307–316 vs 325–334). This is a minor DRY opportunity — a small private helper could deduplicate it. Low priority since there are only two occurrences.♻️ Optional: extract shared discrimination
+ private splitErrorAndMeta( + errorOrMetadata?: Error | LogInput, + metadata?: LogInput + ): { error: Error | undefined; meta: LogInput | undefined } { + if (errorOrMetadata instanceof Error) { + return { error: errorOrMetadata, meta: metadata }; + } + return { error: undefined, meta: errorOrMetadata }; + } + public error(message: string, errorOrMetadata?: Error | LogInput, metadata?: LogInput): void { if (!this.shouldLog(LogLevel.ERROR)) return; - let error: Error | undefined; - let meta: LogInput | undefined; - if (errorOrMetadata instanceof Error) { - error = errorOrMetadata; - meta = metadata; - } else { - error = undefined; - meta = errorOrMetadata; - } - const entry = this.createLogEntry(LogLevel.ERROR, message, meta, error); + const { error, meta } = this.splitErrorAndMeta(errorOrMetadata, metadata); + const entry = this.createLogEntry(LogLevel.ERROR, message, meta, error); this.output(entry); }Apply the same pattern to
fatal.Also applies to: 322-338
- Reorder copy-webpack-plugin alphabetically in devDependencies and regenerate pnpm-lock.yaml (CodeRabbit #1) - Add runtime typeof guard in getDefaultContent return (CodeRabbit #2) - Fix WebSocketMessage cast via unknown for ToolExecutionRequest - Add index signatures to PdfExtractionMetadata/VisionExtractionMetadata - Guard sheet content validation with typeof string check - Cast logger-database category field to string | undefined - Wrap ZodError/Error/MetricsSummary at logger call sites instead of passing incompatible types to LogInput (Record<string, unknown>) - Remove unused ToolUIPart import from confirmation.tsx - Include root types/ directory in lib, processor, and web tsconfigs so pdf-parse-debugging-disabled ambient module resolves in all builds - Spread PDFInfo to satisfy Record<string, unknown> constraint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addressing Review Feedback (b6f50c5)CodeRabbit Comment 1 — Lockfile sync + alphabetical ordering (
|
Resolve modify/delete conflict on page-content-parser.ts (keep deleted per #439). Auto-merged CODE page type additions with our typing improvements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
getPageContentForAIparser and its tests (Tech debt: Wire up or deprecate unused getPageContentForAI parser #439)anywithLogInput/LogMetadata/HttpMethodacross logger, browser-logger, config, database logger, and activity tracker ([Typing Debt] Logging and monitoring metadata typing overhaul #443)Page.content: any→string | null,fileMetadata→Record<string, JsonValue>, validators takeRecord<string, unknown>, Drizzle tx type extracted ([Typing Debt] Shared page/content model typing cleanup #444)as any→readonly string[]widening, content-storeunknowninputs ([Typing Debt] File processing and MIME/pdf typing cleanup #445)pdfjs-distworker topublic/via CopyWebpackPlugin, eliminating unpkg CDN dependency for air-gapped deployments (External CDN scripts break air-gapped deployments, no SRI hashes #466)error-utils.tshelpers (getErrorMessage,hasErrorCode),catch (error: unknown)across index/mcp-manager/auth-storage,WebSocketMessageindexunknown, preloadIpcRendererEvent([Typing Debt] Desktop IPC/MCP typing cleanup #446)ExtendedToolStateunion for forward-compatible AI SDK states, removing 6@ts-expect-errorcomments; typedreact-pdfdynamic imports replacingany([Typing Debt] Reduce runtime TS suppressions at integration boundaries #447)Closes #439, #443, #444, #445, #446, #447, #466
Test plan
pnpm typecheckpasses across monorepopnpm test— all existing tests passpnpm --filter web build— web app builds,public/pdf.worker.min.mjsexistspnpm build:desktop🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements