Skip to content

Commit ca87798

Browse files
seefeldbseefeldb
authored
Claude's suggestion for code improvements (#1204)
Claude's suggestion for code improvements
1 parent 04f2555 commit ca87798

File tree

2 files changed

+385
-0
lines changed

2 files changed

+385
-0
lines changed

‎docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md‎

Lines changed: 241 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,241 @@
1+
# Invalid State Representations Report
2+
3+
Based on the analysis of the codebase against AGENTS.md guidelines, the
4+
following interfaces allow invalid state representations that should be
5+
refactored:
6+
7+
## 1. LLMRequest Interface
8+
9+
**File**: `packages/llm/src/types.ts` **Lines**: 28-38
10+
11+
```typescript
12+
export interface LLMRequest {
13+
cache?: boolean;
14+
messages: LLMMessage[];
15+
model: ModelName;
16+
system?: string;
17+
maxTokens?: number;
18+
stream?: boolean;
19+
stop?: string;
20+
mode?: "json";
21+
metadata?: LLMRequestMetadata;
22+
}
23+
```
24+
25+
**Issue**: The `cache` property being optional creates ambiguity. Code
26+
throughout the codebase defaults it to `true` if undefined, but this implicit
27+
behavior isn't clear from the interface.
28+
29+
**Recommendation**: Rename `cache` to `noCache` with `false` as default:
30+
31+
```typescript
32+
export interface LLMRequest {
33+
noCache?: boolean;
34+
// ... other properties
35+
}
36+
```
37+
38+
## 2. OAuth2Tokens Interface
39+
40+
**File**:
41+
`packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts`
42+
**Lines**: 9-16
43+
44+
```typescript
45+
export interface OAuth2Tokens {
46+
accessToken: string;
47+
refreshToken?: string;
48+
expiresIn?: number;
49+
tokenType: string;
50+
scope?: string[];
51+
expiresAt?: number;
52+
}
53+
```
54+
55+
**Issue**: `refreshToken` is optional but critical for token renewal. Code has
56+
to handle cases where it might be missing, leading to potential authentication
57+
failures.
58+
59+
**Recommendation**: Create separate types for initial and renewable tokens:
60+
61+
```typescript
62+
export interface InitialOAuth2Tokens {
63+
accessToken: string;
64+
tokenType: string;
65+
expiresIn: number;
66+
scope: string[];
67+
}
68+
69+
export interface RenewableOAuth2Tokens extends InitialOAuth2Tokens {
70+
refreshToken: string;
71+
expiresAt: number;
72+
}
73+
```
74+
75+
## 3. UserInfo Interface
76+
77+
**File**:
78+
`packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts`
79+
**Lines**: 18-28
80+
81+
```typescript
82+
export interface UserInfo {
83+
id?: string;
84+
email?: string;
85+
verified_email?: boolean;
86+
name?: string;
87+
given_name?: string;
88+
family_name?: string;
89+
picture?: string;
90+
locale?: string;
91+
error?: string;
92+
}
93+
```
94+
95+
**Issue**: Mixes success and error states in one interface. Either you have user
96+
data OR an error, never both.
97+
98+
**Recommendation**: Use discriminated union:
99+
100+
```typescript
101+
type UserInfoResult =
102+
| {
103+
success: true;
104+
data: {
105+
id: string;
106+
email: string;
107+
verified_email: boolean;
108+
name: string;
109+
// ... other fields
110+
};
111+
}
112+
| { success: false; error: string };
113+
```
114+
115+
## 4. CallbackResult Interface
116+
117+
**File**:
118+
`packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts`
119+
**Lines**: 30-35
120+
121+
```typescript
122+
export interface CallbackResult extends Record<string, unknown> {
123+
success: boolean;
124+
error?: string;
125+
message?: string;
126+
details?: Record<string, unknown>;
127+
}
128+
```
129+
130+
**Issue**: When `success` is false, `error` should be required. When `success`
131+
is true, `error` shouldn't exist.
132+
133+
**Recommendation**: Use discriminated union:
134+
135+
```typescript
136+
type CallbackResult =
137+
| { success: true; message?: string; details?: Record<string, unknown> }
138+
| { success: false; error: string; details?: Record<string, unknown> };
139+
```
140+
141+
## 5. BackgroundCharmServiceOptions Interface
142+
143+
**File**: `packages/background-charm-service/src/service.ts` **Lines**: 12-19
144+
145+
```typescript
146+
export interface BackgroundCharmServiceOptions {
147+
identity: Identity;
148+
toolshedUrl: string;
149+
runtime: Runtime;
150+
bgSpace?: string;
151+
bgCause?: string;
152+
workerTimeoutMs?: number;
153+
}
154+
```
155+
156+
**Issue**: Optional properties have defaults (`BG_SYSTEM_SPACE_ID` and
157+
`BG_CELL_CAUSE`) applied in constructor, but this isn't clear from the
158+
interface.
159+
160+
**Recommendation**: Make defaults explicit in the type:
161+
162+
```typescript
163+
export interface BackgroundCharmServiceOptions {
164+
identity: Identity;
165+
toolshedUrl: string;
166+
runtime: Runtime;
167+
bgSpace: string;
168+
bgCause: string;
169+
workerTimeoutMs: number;
170+
}
171+
172+
export const DEFAULT_BG_OPTIONS = {
173+
bgSpace: BG_SYSTEM_SPACE_ID,
174+
bgCause: BG_CELL_CAUSE,
175+
workerTimeoutMs: 30000,
176+
} as const;
177+
```
178+
179+
## 6. Module Interface
180+
181+
**File**: `packages/builder/src/types.ts` **Lines**: 182-188
182+
183+
```typescript
184+
export interface Module {
185+
type: "ref" | "javascript" | "recipe" | "raw" | "isolated" | "passthrough";
186+
implementation?: ((...args: any[]) => any) | Recipe | string;
187+
wrapper?: "handler";
188+
argumentSchema?: JSONSchema;
189+
resultSchema?: JSONSchema;
190+
}
191+
```
192+
193+
**Issue**: `implementation` is optional but certain module types require it. The
194+
relationship between `type` and required properties isn't enforced.
195+
196+
**Recommendation**: Use discriminated unions:
197+
198+
```typescript
199+
type Module =
200+
| {
201+
type: "ref";
202+
implementation: string;
203+
wrapper?: "handler";
204+
argumentSchema?: JSONSchema;
205+
resultSchema?: JSONSchema;
206+
}
207+
| {
208+
type: "javascript";
209+
implementation: (...args: any[]) => any;
210+
wrapper?: "handler";
211+
argumentSchema?: JSONSchema;
212+
resultSchema?: JSONSchema;
213+
}
214+
| {
215+
type: "recipe";
216+
implementation: Recipe;
217+
wrapper?: "handler";
218+
argumentSchema?: JSONSchema;
219+
resultSchema?: JSONSchema;
220+
}
221+
| { type: "raw"; implementation: string }
222+
| { type: "isolated"; implementation: string }
223+
| { type: "passthrough" };
224+
```
225+
226+
## Impact of These Issues
227+
228+
1. **Runtime Errors**: Optional properties that are actually required lead to
229+
runtime failures
230+
2. **Defensive Programming**: Developers must add null checks everywhere,
231+
cluttering the code
232+
3. **Hidden Dependencies**: Implicit defaults make it hard to understand the
233+
true requirements
234+
4. **Type Safety Loss**: TypeScript can't catch invalid combinations of
235+
properties
236+
237+
## General Principles from AGENTS.md
238+
239+
"Making invalid states unrepresentable is good" - These interfaces violate this
240+
principle by allowing combinations of properties that shouldn't exist together
241+
or by making required properties optional.

‎docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
# Module Graph and Import Issues Report
2+
3+
Based on the analysis of the codebase against AGENTS.md guidelines, the
4+
following module graph and import issues were found:
5+
6+
## 1. Default Exports Instead of Named Exports
7+
8+
The following files violate the "prefer named exports over default exports"
9+
guideline:
10+
11+
### Component Files
12+
13+
- `packages/charm/src/iframe/recipe.ts`
14+
- `packages/charm/src/commands.ts`
15+
- `packages/html/src/render.ts`
16+
- `packages/html/src/path.ts`
17+
- `packages/memory/clock.ts` - `export default new Clock();`
18+
19+
### Plugin Files
20+
21+
- `packages/deno-vite-plugin/src/index.ts`
22+
- `packages/deno-vite-plugin/src/resolvePlugin.ts`
23+
- `packages/deno-vite-plugin/src/prefixPlugin.ts`
24+
25+
### View Components
26+
27+
- `packages/jumble/src/assets/ShapeLogo.tsx`
28+
- `packages/jumble/src/views/CharmView.tsx`
29+
- `packages/jumble/src/views/ErrorBoundaryView.tsx`
30+
- `packages/jumble/src/views/DebugView.tsx`
31+
- Multiple other view files in `packages/jumble/src/views/`
32+
33+
**Example Fix**:
34+
35+
```typescript
36+
// Bad
37+
export default class Clock { ... }
38+
39+
// Good
40+
export class Clock { ... }
41+
42+
// Bad
43+
export default new Clock();
44+
45+
// Good
46+
export const clock = new Clock();
47+
// Or better: export a factory function
48+
export function createClock(): Clock { ... }
49+
```
50+
51+
## 2. Missing or Incorrect Import Grouping
52+
53+
The following files don't follow the import grouping convention (standard
54+
library → external → internal):
55+
56+
### `packages/jumble/src/components/CharmRunner.tsx`
57+
58+
```typescript
59+
// Current (mixed imports)
60+
import { html } from "lit";
61+
import { Recipe } from "@commontools/common-runner";
62+
import { SpellRunner } from "./SpellRunner.js";
63+
64+
// Should be:
65+
// Standard library
66+
import { html } from "lit";
67+
68+
// External
69+
// (none in this case)
70+
71+
// Internal
72+
import { Recipe } from "@commontools/common-runner";
73+
import { SpellRunner } from "./SpellRunner.js";
74+
```
75+
76+
### `packages/builder/src/utils.ts`
77+
78+
```typescript
79+
// Current (internal packages mixed without grouping)
80+
import { JSONSchema7 } from "json-schema";
81+
import { isOpaqueRef, OpaqueRef } from "./spell.js";
82+
import { diffAndUpdate, maybeGetCellLink } from "@commontools/runner";
83+
84+
// Should be:
85+
// Standard library
86+
// (none)
87+
88+
// External
89+
import { JSONSchema7 } from "json-schema";
90+
91+
// Internal
92+
import { diffAndUpdate, maybeGetCellLink } from "@commontools/runner";
93+
import { isOpaqueRef, OpaqueRef } from "./spell.js";
94+
```
95+
96+
### `packages/identity/src/ed25519/index.ts`
97+
98+
- External imports (`@scure/bip39`) mixed with internal imports without proper
99+
grouping
100+
101+
## 3. Module-Specific Dependencies (Good Practices Found)
102+
103+
The codebase correctly follows the guideline for module-specific dependencies:
104+
105+
### Good Examples:
106+
107+
- `packages/llm/deno.json` - Correctly declares `json5` dependency
108+
- `packages/ui/deno.json` - Correctly declares `@shoelace-style/shoelace`
109+
dependency
110+
- Most packages don't have unnecessary dependencies in their `deno.json`
111+
112+
**This is a positive finding** - the codebase correctly avoids adding
113+
package-specific dependencies to the workspace `deno.json`.
114+
115+
## Recommendations
116+
117+
### 1. Convert Default to Named Exports
118+
119+
```typescript
120+
// For all identified files:
121+
// Change: export default Clock
122+
// To: export { Clock }
123+
124+
// Change: export default new Clock()
125+
// To: export const clock = new Clock()
126+
// or export function createClock(): Clock
127+
```
128+
129+
### 2. Enforce Import Grouping
130+
131+
Add ESLint rule or formatter configuration to enforce:
132+
133+
1. Standard library imports
134+
2. External package imports
135+
3. Internal package imports
136+
4. Relative imports
137+
138+
## Impact Summary
139+
140+
- **Circular dependencies**: Critical issue affecting maintainability and
141+
testability
142+
- **Default exports**: Medium impact - affects consistency and tree-shaking
143+
- **Import grouping**: Low impact - affects readability
144+
- **Heavy utilities**: Medium impact - affects bundle size and modularity

0 commit comments

Comments
 (0)