refactor: improve type safety and DRY up codebase#6
Conversation
Remove index signatures from 16 interfaces, extract shared utils (datetime, validation), add mock client factory, delete deprecated createOAuthClientFromEnv
There was a problem hiding this comment.
Pull request overview
This PR improves code quality and type safety by removing index signatures from 16 interfaces, extracting shared utilities (datetime and validation), adding a mock client factory for testing, and deleting the deprecated createOAuthClientFromEnv function. The changes reduce code duplication and make the codebase more maintainable with explicit type definitions.
Key changes:
- Replaced
[key: string]: unknownindex signatures with explicitextra?: Record<string, unknown>fields in 16 interfaces - Extracted
localTimeToUtcutility function tosrc/utils/datetime.ts(used in 2 locations) - Created
validateTourIdutility function insrc/utils/validation.ts(used in 4 tool functions) - Added
createMockClientfactory and mock response helpers insrc/testing/mockClient.ts - Removed deprecated
createOAuthClientFromEnvfunction and its tests - Updated
createScheduleItemreturn type to includesyncIdfield
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/validation.ts | New utility for tour ID validation with comprehensive test coverage |
| src/utils/datetime.ts | New utility extracting timezone conversion logic from multiple files |
| src/testing/mockClient.ts | New mock client factory for test utilities with response builders |
| tests/unit/validation.test.ts | Complete test coverage for validateTourId utility (4 test cases) |
| tests/unit/datetime.test.ts | Complete test coverage for localTimeToUtc utility (7 test cases) |
| tests/unit/mockClient.test.ts | Complete test coverage for mock utilities (7 test cases) |
| src/api/client.ts | Removed index signatures from 16 interfaces, added explicit fields and extra property for type safety |
| src/tools/addScheduleItem.ts | Updated to use shared localTimeToUtc utility |
| src/tools/updateScheduleItem.ts | Updated to use shared localTimeToUtc utility |
| src/tools/getTourHotels.ts | Updated to use shared validateTourId utility |
| src/tools/getTourEvents.ts | Updated to use shared validateTourId utility |
| src/tools/getTourCrew.ts | Updated to use shared validateTourId utility |
| src/tools/getTodaySchedule.ts | Updated to use shared validateTourId utility |
| src/auth/oauth.ts | Removed deprecated createOAuthClientFromEnv function |
| tests/unit/auth.test.ts | Removed tests for deprecated function |
| tests/unit/addScheduleItem.test.ts | Updated mock return value to include syncId field |
| src/types/mastertour.ts | Deleted unused legacy type definitions file |
| src/types/index.ts | Updated export to point to outputs.js instead of mastertour.js |
| docs/TECHNICAL_DOCUMENTATION.md | Updated project structure documentation |
| docs/PROJECT_STATUS_EXPANDED.md | Updated status with completion of code quality refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function createMockClient(overrides?: Partial<Record<keyof MasterTourClient, MockFn>>): MockClient { | ||
| return { | ||
| listTours: vi.fn(), | ||
| getDay: vi.fn(), | ||
| getTourSummary: vi.fn(), | ||
| getTourHotels: vi.fn(), | ||
| getTourCrew: vi.fn(), | ||
| getTourEvents: vi.fn(), | ||
| createScheduleItem: vi.fn(), | ||
| updateScheduleItem: vi.fn(), | ||
| deleteScheduleItem: vi.fn(), | ||
| updateDayNotes: vi.fn(), | ||
| ...overrides, | ||
| } as MockClient; |
There was a problem hiding this comment.
The createMockClient function is missing mock implementations for getTourAll and getDayEvents methods that are defined in the MasterTourClient interface. While these may not be used in current tests, for completeness and to avoid potential runtime errors, all interface methods should be mocked.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| export type MockClient = MasterTourClient & { | ||
| listTours: MockFn; | ||
| getDay: MockFn; | ||
| getTourSummary: MockFn; | ||
| getTourHotels: MockFn; | ||
| getTourCrew: MockFn; | ||
| getTourEvents: MockFn; | ||
| createScheduleItem: MockFn; | ||
| updateScheduleItem: MockFn; | ||
| deleteScheduleItem: MockFn; | ||
| updateDayNotes: MockFn; | ||
| }; |
There was a problem hiding this comment.
The MockClient type definition is missing getTourAll and getDayEvents method signatures. These methods are part of the MasterTourClient interface and should be included in the MockClient type to maintain type consistency.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
[WIP] Address feedback on type safety and DRY refactor improvements
Remove index signatures from 16 interfaces, extract shared utils (datetime, validation), add mock client factory, delete deprecated createOAuthClientFromEnv