Conversation
DEFAULT_BEZIER_POINTS from @/types/keyframe was used at module scope in BEZIER_PRESETS, causing a temporal dead zone error when production chunk ordering evaluates keyframe-graph-panel before keyframe.ts.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughWidespread introduction of a structured logging/events system: a new wide-event logging API was added, many modules replaced direct console calls with the centralized logger (and event usage), media import/resolution logic was refactored for deduplication and evented operations, plus one small bezier preset tweak. Changes
Sequence Diagram(s)(Skipped — changes are primarily logging, refactors, and internal batching without a new multi-component sequential runtime flow that requires visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Greptile SummaryThis single-line fix inlines the
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Vite production build"] --> B{"Chunk ordering"}
B -->|"keyframe-graph-panel.tsx evaluated first"| C["BEZIER_PRESETS module-scope init"]
C --> D{"References DEFAULT_BEZIER_POINTS?"}
D -->|"Before fix: yes (import)"| E["TDZ Error: Cannot access '_t'"]
D -->|"After fix: no (inlined)"| F["Initializes successfully"]
F --> G["keyframe.ts evaluated later"]
G --> H["DEFAULT_BEZIER_POINTS available for runtime usages"]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/features/timeline/components/keyframe-graph-panel.tsx
Line: 105
Comment:
**Inlined values may drift from source of truth**
The values `{ x1: 0.42, y1: 0, x2: 0.58, y2: 1 }` are correct today (matching `DEFAULT_BEZIER_POINTS` in `keyframe.ts:163-168`), but since the "Soft" preset is semantically the default bezier curve, a future change to `DEFAULT_BEZIER_POINTS` won't automatically propagate here. Consider adding a comment referencing the source constant so a future maintainer knows to keep them in sync, e.g.:
```suggestion
{ value: 'soft', label: 'Soft', points: { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } }, // must match DEFAULT_BEZIER_POINTS
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 5dc06d4 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ]; | ||
| const BEZIER_PRESETS = [ | ||
| { value: 'soft', label: 'Soft', points: DEFAULT_BEZIER_POINTS }, | ||
| { value: 'soft', label: 'Soft', points: { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } }, |
There was a problem hiding this comment.
Inlined values may drift from source of truth
The values { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } are correct today (matching DEFAULT_BEZIER_POINTS in keyframe.ts:163-168), but since the "Soft" preset is semantically the default bezier curve, a future change to DEFAULT_BEZIER_POINTS won't automatically propagate here. Consider adding a comment referencing the source constant so a future maintainer knows to keep them in sync, e.g.:
| { value: 'soft', label: 'Soft', points: { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } }, | |
| { value: 'soft', label: 'Soft', points: { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } }, // must match DEFAULT_BEZIER_POINTS |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/timeline/components/keyframe-graph-panel.tsx
Line: 105
Comment:
**Inlined values may drift from source of truth**
The values `{ x1: 0.42, y1: 0, x2: 0.58, y2: 1 }` are correct today (matching `DEFAULT_BEZIER_POINTS` in `keyframe.ts:163-168`), but since the "Soft" preset is semantically the default bezier curve, a future change to `DEFAULT_BEZIER_POINTS` won't automatically propagate here. Consider adding a comment referencing the source constant so a future maintainer knows to keep them in sync, e.g.:
```suggestion
{ value: 'soft', label: 'Soft', points: { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } }, // must match DEFAULT_BEZIER_POINTS
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/features/timeline/components/keyframe-graph-panel.tsx (1)
104-105: Consider documenting why this literal is intentionally inlined.A brief comment here would help prevent future “cleanup” refactors from reintroducing the module-evaluation TDZ path.
Optional patch
const BEZIER_PRESETS = [ + // Intentionally inlined to avoid module-scope TDZ from imported constants in production chunk ordering. { value: 'soft', label: 'Soft', points: { x1: 0.42, y1: 0, x2: 0.58, y2: 1 } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/keyframe-graph-panel.tsx` around lines 104 - 105, The BEZIER_PRESETS literal is intentionally inlined to avoid a module-evaluation TDZ path; add a concise comment above the BEZIER_PRESETS declaration explaining that it must remain an inline literal (not extracted to another module or computed at import time) to prevent dead temporal zone initialization issues and to reference the specific usage in KeyframeGraphPanel or functions that rely on its immediate availability; keep the comment short, mention "TDZ/module-evaluation" and the rationale so future maintainers don't refactor it away.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/features/timeline/components/keyframe-graph-panel.tsx`:
- Around line 104-105: The BEZIER_PRESETS literal is intentionally inlined to
avoid a module-evaluation TDZ path; add a concise comment above the
BEZIER_PRESETS declaration explaining that it must remain an inline literal (not
extracted to another module or computed at import time) to prevent dead temporal
zone initialization issues and to reference the specific usage in
KeyframeGraphPanel or functions that rely on its immediate availability; keep
the comment short, mention "TDZ/module-evaluation" and the rationale so future
maintainers don't refactor it away.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dfac978-3dc1-4ba1-b978-262883d16c60
📒 Files selected for processing (1)
src/features/timeline/components/keyframe-graph-panel.tsx
…o structured logger Enhance logger with startEvent/event methods for accumulating context across multi-step operations (export, import, auto-save) and emitting one structured event at completion. Add createOperationId() for correlating log entries. Migrate all raw console.warn/error calls (~55 across 30 files) to use createLogger, leaving only logger internals and the debug panel using console directly.
walterlow
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| event.set('outcome', 'cancelled'); | ||
| event.set('duration_ms', Date.now()); | ||
| log.event('render', { opId, outcome: 'cancelled' }); |
There was a problem hiding this comment.
Incorrect handling of cancelled export event. Line 432 sets duration_ms to Date.now() (absolute timestamp) instead of elapsed time. Line 433 calls log.event() with a minimal new object, discarding all accumulated context (codec, resolution, items, tracks, etc.) collected throughout the export.
Fix: Remove lines 431-433 and emit the accumulated event:
event.set('outcome', 'cancelled');
event.success({ outcome: 'cancelled' });Or add a cancel() method to WideEvent interface that properly calculates duration and emits all accumulated data.
| event.set('outcome', 'cancelled'); | |
| event.set('duration_ms', Date.now()); | |
| log.event('render', { opId, outcome: 'cancelled' }); | |
| event.set('outcome', 'cancelled'); | |
| event.success({ outcome: 'cancelled' }); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| event.set('outcome', 'cancelled'); | ||
| logger.event('import', { opId, outcome: 'cancelled' }); |
There was a problem hiding this comment.
Incorrect handling of cancelled import event. Line 221 calls logger.event() with a minimal new object containing only opId and outcome, discarding all accumulated context (source, projectId, fileCount) collected during the import setup.
Fix: Emit the accumulated event data:
event.set('outcome', 'cancelled');
event.success({ outcome: 'cancelled' });Or add a cancel() method to WideEvent interface for consistent cancellation handling.
| event.set('outcome', 'cancelled'); | |
| logger.event('import', { opId, outcome: 'cancelled' }); | |
| event.set('outcome', 'cancelled'); | |
| event.success({ outcome: 'cancelled' }); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
1 participant
Summary
src/shared/logging/logger.tswith wide event support:startEvent(),event(), andcreateOperationId()for accumulating context across multi-step operations and emitting one structured event at completionuse-client-render.ts) and media import (media-import-actions.ts) flows into single wide events with business context (project ID, codec, resolution, item counts, duration)console.warn/console.errorcalls across 30 files to usecreateLogger, leaving only logger internals and the debug panel using console directlyTest plan
npm run dev)[Export] renderevent appears in console with full context[MediaImport] importevent appears with counts[AutoSave] saveevent with durationnpm run build) — no TDZ issues from logger changesSummary by CodeRabbit
Refactor
Chores