Skip to content

fix(typecheck): narrow hook event counts - #1496

Merged
kevincodex1 merged 1 commit into
Twigpine:mainfrom
chioarub:fix/typecheck-hook-event-counts
Jun 4, 2026
Merged

kevincodex1 merged 1 commit into
Twigpine:mainfrom
chioarub:fix/typecheck-hook-event-counts

Conversation

@chioarub

@chioarub chioarub commented Jun 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Refs #1486.

Fixes a focused hook menu typecheck error by preserving the concrete hook grouping shape through the memoized count calculation.

What changed

  • Added a local HooksByEventAndMatcher type for the grouped hook config map.
  • Typed the per-event count accumulator and reducer callback so Object.values(matchers) stays IndividualHookConfig[] instead of degrading to unknown.

Why

The component already builds a HookEvent -> matcher -> hooks[] map, but the React compiler cache slot erased that shape before the reduce call. Restoring the explicit local type keeps the runtime behavior unchanged while removing the loose object-access error.

Validation

  • bun run typecheck 2>&1 | tee /tmp/openclaude-typecheck-after-hook-counts.txt — still reports unrelated backlog errors, but grep -n "HooksConfigMenu" /tmp/openclaude-typecheck-after-hook-counts.txt has no matches
  • git diff --check — passed
  • bun run build — passed

Summary by CodeRabbit

Release Notes

  • Refactor
    • Improved code type safety and reliability.

This release contains no user-facing changes.

@chioarub

chioarub commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 699e4d21-00e7-42e7-b7d8-9e9722ac58cf

📥 Commits

Reviewing files that changed from the base of the PR and between cfcc5d0 and 852d691.

📒 Files selected for processing (1)
  • src/components/hooks/HooksConfigMenu.tsx

📝 Walkthrough

Walkthrough

This PR adds explicit TypeScript type annotations to HooksConfigMenu.tsx to improve static typing of hook configuration data structures. A type alias is defined for the nested hook record structure and applied to computed values, accumulators, and helper function parameters.

Changes

Hook Configuration Type Annotations

Layer / File(s) Summary
Hook data structure and helper function typing
src/components/hooks/HooksConfigMenu.tsx
A HooksByEventAndMatcher type alias is defined for the nested record structure keyed by HookEvent and matcher name, applied to the derived hooksByEventAndMatcher value, and used to type the byEvent accumulator and _temp5 helper function parameters.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 Types bring clarity to the hook configuration flow,
Each annotation sharpens what TypeScript should know,
From nested records to helper function calls,
Static typing now stands proud in these halls!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing a TypeScript type checking error related to hook event counts in the HooksConfigMenu component.
Description check ✅ Passed The description covers the what, why, and validation, but is missing explicit testing sections from the template (bun run build, smoke, check checkboxes are not checked).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I do not see any actionable issues from my review.

@kevincodex1

@kevincodex1
kevincodex1 merged commit 11e46af into Twigpine:main Jun 4, 2026
3 checks passed
@chioarub
chioarub deleted the fix/typecheck-hook-event-counts branch June 4, 2026 22:40
hotmanxp pushed a commit to hotmanxp/openclaude that referenced this pull request Jun 5, 2026
Upstream 343cd1a..1204fe2, applied 6 of 8 KEEP candidates in tier 1:

  11e46af fix(typecheck): narrow hook event counts (Twigpine#1496)
  2c755d3 fix(typecheck): restore typed add-dir source (Twigpine#1504)
  2bed184 perf(attachments): skip skill listings for utility forks (Twigpine#1545)
  47eea3f fix(typecheck): type search UI state (Twigpine#1529)
  96ddec7 fix(test): stop use-input test from leaking a global stdin mock (Twigpine#1501)
  1fc5116 fix(api): tighten reasoning_content heuristic to prevent false-positives (Twigpine#1201)

Notes:
- 2c755d3: react-compiler compiled output (add-dir.tsx) replaced with typed
  source; build re-compiles on next run
- 47eea3f: kept fork-specific // @ts-nocheck at top of GlobalSearchDialog.tsx
  (added by local b40814b, not in upstream); applied type Props + true arg
- 96ddec7: bun.lock regenerated via 'bun install' to reconcile with new
  package.json (devDependencies pruned: react-hooks, react-test-renderer);
  new use-input.test.ts (208 lines) injected via StdinContext.Provider
- 1fc5116: new runtimeMetadata.test.ts (90 lines, regression coverage for
  the segment-boundary heuristic per jatmn review)

Verification:
  baseline:  2223 pass / 23 skip / 4 fail
  after:     2232 pass / 23 skip / 2 fail   (+9 pass, -2 fail)
  typecheck: 90 errors (pre-existing, no new)
  build:     Built v0.16.1

Rebase onto OpenCC v0.14.0 (2292a59).
hotmanxp added a commit to hotmanxp/openclaude that referenced this pull request Jun 7, 2026
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
deagwon97 pushed a commit to deagwon97/openclaude that referenced this pull request Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants