Repository navigation
fix(typecheck): restore proactive module import surface - #1495
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNew proactive state module: subscription-based listener notifications, state query helpers, and control functions (activate/deactivate/pause/resume/setContextBlocked). Listeners are invoked on each transition with isolated error handling; Bun tests validate context-block clearing and listener error isolation. ChangesProactive State Management
Sequence DiagramssequenceDiagram
participant Caller
participant Proactive Module
participant Listeners
Caller->>Proactive Module: subscribeToProactiveChanges(listener)
Proactive Module->>Proactive Module: add listener to Set
Proactive Module->>Caller: return unsubscribe function
Caller->>Proactive Module: isProactiveActive()
Proactive Module->>Caller: return boolean state
Caller->>Proactive Module: getNextTickAt()
Proactive Module->>Caller: return nextTickAt or null
Caller->>Proactive Module: unsubscribe()
Proactive Module->>Proactive Module: remove listener from Set
stateDiagram-v2
[*] --> Inactive
Inactive --> Active: activateProactive()
Active --> Inactive: deactivateProactive()
Active --> Paused: pauseProactive()
Paused --> Active: resumeProactive()
Active --> ContextBlocked: setContextBlocked(true)
Paused --> ContextBlocked: setContextBlocked(true)
ContextBlocked --> Active: setContextBlocked(false)
note right of Active
notifyListeners() called
on every transition
end note
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/proactive/index.ts (1)
10-18: 💤 Low valueConsider logging swallowed listener exceptions for observability.
Silent exception swallowing is correct for isolation, but completely silent failures can make debugging listener issues difficult. A
console.errorwould preserve isolation while aiding diagnosis.🔇 Proposed improvement for error visibility
function notifyProactiveListeners(): void { for (const listener of [...listeners]) { try { listener() - } catch { + } catch (err) { + console.error('Proactive listener threw:', err) // Listener failures must not prevent state transitions or later listeners. } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/proactive/index.ts` around lines 10 - 18, The notifyProactiveListeners function currently swallows exceptions from each listener (variable listeners) without any visibility; modify the catch block inside notifyProactiveListeners to log the caught error (and optionally context such as which listener or its index) using console.error so listener failures remain isolated but are observable for debugging; ensure the log message is descriptive (e.g., "proactive listener error") and include the error object to preserve stack traces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/proactive/index.ts`:
- Around line 10-18: The notifyProactiveListeners function currently swallows
exceptions from each listener (variable listeners) without any visibility;
modify the catch block inside notifyProactiveListeners to log the caught error
(and optionally context such as which listener or its index) using console.error
so listener failures remain isolated but are observable for debugging; ensure
the log message is descriptive (e.g., "proactive listener error") and include
the error object to preserve stack traces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a275501-d311-449d-95d5-397f4863e7ab
📒 Files selected for processing (2)
src/proactive/index.test.tssrc/proactive/index.ts
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
Upstream 47eea3f..e7abb81, applied 2 of 5 KEEP candidates in tier 1 (3 already applied under prior sync 4f6bccc but were missed by subject-match dedup due to squash-merge subject loss): e357d59 fix(typecheck): restore proactive module import surface (Twigpine#1495) e7abb81 fix(typecheck): make session history cache variant-safe (Twigpine#1494) Already-applied (byte-equal content + fork-specific @ts-nocheck): 47eea3f fix(typecheck): type search UI state (Twigpine#1529) 11e46af fix(typecheck): narrow hook event counts (Twigpine#1496) 2c755d3 fix(typecheck): restore typed add-dir source (Twigpine#1504) Notes: - e357d59: 2 new files (src/proactive/{index.ts,index.test.ts}); pulls cleanly, no provider leakage - e7abb81: 1 new file (sessionHistorySerialization.ts) + 3 mods. sessionHistory.test.ts gets // @ts-nocheck (upstream tests use SDKUserMessage.message + new serialization import; local SDKUserMessage type doesn't include .message field, and sessionHistory.ts still uses inline impl). sessionHistory.ts gets the upstream rewrite (inline functions replaced with imports from sessionHistorySerialization.ts); @ts-nocheck preserved. - conversationCache.ts: Message[] → CacheMessage[]; CacheMessage becomes Record<string, unknown> (was: alias of Message interface) Verification: typecheck: 0 errors bun test src/assistant/sessionHistory.test.ts: 6 pass, 0 fail bun test src/proactive/index.test.ts: 2 pass, 0 fail build: Built v0.16.1 → dist/cli.mjs naming scan: no openclaude/gitlawb leakage
* fix(typecheck): restore proactive module import surface * test(proactive): harden state change notifications * fix(proactive): log listener notification errors
Summary
Refs #1486.
Restores the proactive module import surface expected by feature-gated consumers and hardens the new state helper against stale pause state and subscriber failures.
What changed
src/proactive/index.tswith the proactive active/paused state helpers used by CLI, REPL, prompt, and tool code.Why
Several consumers lazy-require
../proactive/index.jsbehind feature gates, butmainno longer has that module. This leaves focused typecheck errors for the proactive import path and would also fail at runtime if the feature-gated path is enabled.The review pass found two edge cases in the initial shim: context blocking could survive activation, and notification callbacks were allowed to throw through the state transition. Both are fixed in the state module rather than making callers defend against broken global state.
Validation
bun test src/proactive/index.test.ts- 2 pass, 0 failgit diff --check- passedbun run build- passedbun run typecheck 2>&1 | tee /tmp/openclaude-typecheck-after-proactive-review.txt- still reports unrelated repository backlog errors, but targeted grep forproactive/indexandsrc/proactivehas no matchesSummary by CodeRabbit