Skip to content

refactor: consolidate createMockTab test factories (Phase 03D)#814

Merged
pedramamini merged 3 commits intoRunMaestro:rcfrom
jSydorowicz21:dedup/phase-03d-mock-tab-factory
Apr 16, 2026
Merged

refactor: consolidate createMockTab test factories (Phase 03D)#814
pedramamini merged 3 commits intoRunMaestro:rcfrom
jSydorowicz21:dedup/phase-03d-mock-tab-factory

Conversation

@jSydorowicz21
Copy link
Copy Markdown
Contributor

@jSydorowicz21 jSydorowicz21 commented Apr 12, 2026

Summary

Test-only change. Zero production risk.

Introduces createMockAITab and createMockFileTab factories at src/__tests__/helpers/mockTab.ts. Replaces per-file tab factory definitions across 19 test files.

Net: -85 lines across 21 files

Files migrated: 14 hook tests, 4 util/store tests, 2 component tests. One file skipped (broadcastService - uses different AITabData shape).

Test plan

  • npm run lint passes clean
  • Hook / util / tab tests pass

Risk

Zero - no production code changes.

Consolidate 19 local createMockTab/createMockAITab/createMockFileTab
factory definitions across test files into shared helpers at
src/__tests__/helpers/mockTab.ts.

- New shared createMockAITab(overrides) and createMockFileTab(overrides)
  helpers accept Partial<AITab>/Partial<FilePreviewTab> with sensible
  defaults for all required fields
- Files with unique defaults (saveToHistory, non-null agentSessionId,
  pre-seeded logs) retain thin local wrappers that delegate to the shared
  factory with the file-specific overrides layered in
- Positional-signature wrappers in useMergeSession, useSendToAgent, and
  MergeSessionModal tests are preserved as thin wrappers to keep call
  sites unchanged
- broadcastService.test.ts is excluded because it uses the AITabData
  type (a different shape) rather than AITab
- tabHelpers.test.ts migrates to the shared factory directly (no local
  wrapper needed)

Test-only change. Zero production impact.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 12, 2026

Warning

Rate limit exceeded

@jSydorowicz21 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 29 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 29 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 82dcacf6-a56e-44ac-b102-8abdb7c175a2

📥 Commits

Reviewing files that changed from the base of the PR and between 36b677a and f04bc32.

📒 Files selected for processing (21)
  • src/__tests__/helpers/index.ts
  • src/__tests__/helpers/mockTab.ts
  • src/__tests__/renderer/components/MergeSessionModal.test.tsx
  • src/__tests__/renderer/components/ThinkingStatusPill.test.tsx
  • src/__tests__/renderer/hooks/useAgentExecution.test.ts
  • src/__tests__/renderer/hooks/useAgentListeners.test.ts
  • src/__tests__/renderer/hooks/useAgentSessionManagement.test.ts
  • src/__tests__/renderer/hooks/useInputProcessing.test.ts
  • src/__tests__/renderer/hooks/useMergeSession.test.ts
  • src/__tests__/renderer/hooks/useModalHandlers.test.ts
  • src/__tests__/renderer/hooks/useRemoteIntegration.test.ts
  • src/__tests__/renderer/hooks/useSendToAgent.test.ts
  • src/__tests__/renderer/hooks/useSessionLifecycle.test.ts
  • src/__tests__/renderer/hooks/useSummarizeHandler.test.ts
  • src/__tests__/renderer/hooks/useTabExportHandlers.test.ts
  • src/__tests__/renderer/hooks/useTabHandlers.test.ts
  • src/__tests__/renderer/hooks/useWizardHandlers.test.ts
  • src/__tests__/renderer/stores/tabStore.test.ts
  • src/__tests__/renderer/utils/contextExtractor.test.ts
  • src/__tests__/renderer/utils/tabExport.test.ts
  • src/__tests__/renderer/utils/tabHelpers.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

…t test

Imports should precede type alias declarations per ES module conventions.
@jSydorowicz21 jSydorowicz21 self-assigned this Apr 14, 2026
@jSydorowicz21 jSydorowicz21 marked this pull request as ready for review April 14, 2026 04:35
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 14, 2026

Greptile Summary

This PR is a test-only refactoring (Phase 03D) that introduces shared createMockAITab and createMockFileTab factories in src/__tests__/helpers/mockTab.ts and migrates 19 test files away from per-file factory duplicates, netting -85 lines across 21 files. No production code is touched.

The factories are well-designed: all required interface fields are covered, optional fields are correctly omitted, and files needing unique IDs or extra defaults use local wrapper functions rather than mutating the shared defaults.

Confidence Score: 5/5

  • Safe to merge — test-only refactor with no production code changes and no failing logic.
  • All findings are P2 style observations (one implicit factory-default coupling, one consolidation opportunity for future phases). No correctness issues, no production risk.
  • src/tests/renderer/hooks/useSessionLifecycle.test.ts — relies on the implicit id: 'tab-1' default from createMockAITab() without passing it explicitly.

Important Files Changed

Filename Overview
src/tests/helpers/mockTab.ts New shared factory file with createMockAITab and createMockFileTab; all required interface fields are covered, optional fields are correctly omitted, type assertions are appropriate.
src/tests/helpers/index.ts Clean barrel export for the new shared test factories.
src/tests/renderer/hooks/useSessionLifecycle.test.ts Migrated to shared factory; uses createMockAITab() with no args alongside hardcoded activeTabId: 'tab-1', creating an implicit dependency on the factory's default id value — contrary to the factory's own JSDoc guidance.
src/tests/renderer/utils/tabHelpers.test.ts Imports createMockAITab as createMockTab directly (no local wrapper); all multi-tab tests correctly provide explicit id overrides.
src/tests/renderer/stores/tabStore.test.ts Wraps base factories with random-id generators; clean pattern for multi-tab tests.
src/tests/renderer/hooks/useTabHandlers.test.ts Wraps base factories with random-id generators; correctly adds isAtBottom and hasUnread defaults needed by this hook's tests.
src/tests/renderer/hooks/useAgentExecution.test.ts Migrated to shared factory with a local wrapper that adds createdAt and saveToHistory; straightforward and correct.
src/tests/renderer/components/MergeSessionModal.test.tsx Clean migration to shared factory; no issues found.
src/tests/renderer/utils/contextExtractor.test.ts Clean direct import of shared factory; used appropriately.
src/tests/renderer/utils/tabExport.test.ts Clean migration; factory used straightforwardly for single-tab export tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["src/__tests__/helpers/mockTab.ts"] -->|"exports createMockAITab"| B["14 hook tests"]
    A -->|"exports createMockFileTab"| C["4 util/store tests"]
    A -->|"exports both"| D["2 component tests"]
    E["src/__tests__/helpers/index.ts"] -->|"barrel re-export"| A

    B --> B1["useAgentExecution\n(local wrapper: +createdAt, +saveToHistory)"]
    B --> B2["useAgentSessionManagement\n(local wrapper: +createdAt, +saveToHistory)"]
    B --> B3["useAgentListeners\n(local wrapper: +createdAt, +saveToHistory)"]
    B --> B4["useTabHandlers\n(local wrapper: +random id, +hasUnread, +isAtBottom)"]
    B --> B5["useMergeSession / useSendToAgent\n(direct import)"]

    C --> C1["tabStore\n(local wrapper: +random id)"]
    C --> C2["tabHelpers\n(aliased direct import)"]
    C --> C3["tabExport / contextExtractor\n(direct import)"]

    D --> D1["MergeSessionModal"]
    D --> D2["ThinkingStatusPill"]
Loading

Comments Outside Diff (1)

  1. src/__tests__/renderer/hooks/useSessionLifecycle.test.ts, line 44-45 (link)

    P2 Implicit coupling to factory's default id

    createMockAITab() is called with no arguments, so id defaults to 'tab-1'. The hardcoded activeTabId: 'tab-1' on the next line silently depends on that default. The factory's own JSDoc warns: "If your test depends on a specific default… pass it explicitly via overrides." If the factory default ever changes, this session will have an activeTabId that matches no tab without any compile-time or immediate runtime signal.

Reviews (1): Last reviewed commit: "fix: move import before type declaration..." | Re-trigger Greptile

Comment on lines +23 to +36
export function createMockAITab(overrides: Partial<AITab> = {}): AITab {
return {
id: 'tab-1',
agentSessionId: null,
name: null,
starred: false,
logs: [],
inputValue: '',
stagedImages: [],
createdAt: Date.now(),
state: 'idle',
...overrides,
} as AITab;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Repeated local createMockTab wrappers could be extracted

Four hook test files (useAgentExecution, useAgentSessionManagement, useAgentListeners, useRemoteIntegration) each define a nearly-identical local createMockTab that adds createdAt: 1700000000000 and saveToHistory: true on top of createMockAITab. If a future phase wants to consolidate these, a second shared factory (e.g., createMockSessionAITab) or an optional parameter on this one could eliminate the remaining boilerplate. Not a blocker for this phase — flagging for awareness.

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!

@jSydorowicz21 jSydorowicz21 added refactor Clean-up needs ready to merge This PR is ready to merge labels Apr 15, 2026
@pedramamini pedramamini merged commit 4b0f89d into RunMaestro:rc Apr 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge This PR is ready to merge refactor Clean-up needs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants