-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: bundled the test initialization to a utility class #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughSwap several registry initializations from runtime casts to generic Changes
Sequence Diagram(s)(omitted — changes are type/signature updates and test scaffolding; no new runtime control-flow warranting a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/root/util/wireless.ts (1)
43-45: Type update to IRegistry<Partial) is sound.Minor: consider reserving the injector key to avoid collisions.
Apply this small tweak to make the intent explicit and reduce collision risk:
+export const INJECTOR_KEY = "$injector" as const ... - registry.set("$injector" as KeyOf<TCatalog>, ["const", injector as TCatalog[KeyOf<TCatalog>]]) + registry.set(INJECTOR_KEY as KeyOf<TCatalog>, ["const", injector as TCatalog[KeyOf<TCatalog>]])Also applies to: 48-49
src/root/util/wire.ts (1)
231-233: Removeunknown asby passing a typed get-only view to Wire.Current cast hides type mismatches. Wrap
getinstead; keeps types honest with Partial.Apply this diff at the return:
- return new Wire(nameForDuplicatesOf, registry as unknown as IRegistry<TCatalog>, new Map()) + const getOnly: Pick<IRegistry<TCatalog>, "get"> = { + get: (k) => registry.get(k) as ResolveQuery<TCatalog> | undefined, + } + return new Wire(nameForDuplicatesOf, getOnly, new Map())Also applies to: 236-237, 247-248
src/test/ghostbird.test.ts (4)
76-85: Avoid leaking extra keys into the registry initialization.Keep the registry to BackgroundCatalog keys to reduce ambiguity in DI resolution during tests.
Example:
- let registry = makeRegistry<Partial<BackgroundCatalog>>({ - messenger: Symbol("messenger") as unknown as typeof globalThis.messenger, - optionsSyncCtor: Symbol("optionsSyncCtor") as unknown as typeof OptionsSync, - menuItems: [], - ...mockedThunderbird, - } satisfies MockedThunderbirdCatalog & BackgroundConstants) + const values: Partial<BackgroundCatalog> = { + messenger: Symbol("messenger") as unknown as typeof globalThis.messenger, + optionsSyncCtor: Symbol("optionsSyncCtor") as unknown as typeof OptionsSync, + menuItems: [], + heart: mockedThunderbird.heart, + composeTabDetector: mockedThunderbird.composeTabDetector, + } + let registry = makeRegistry<Partial<BackgroundCatalog>>(values)
90-96: Guard againstgetSelection()returning null in test envs.JSDOM can return null; add a safe fallback.
- let registry = makeRegistry<Partial<ComposeCatalog>>({ + const selection = globalThis.getSelection?.() ?? ({} as unknown as Selection) + let registry = makeRegistry<Partial<ComposeCatalog>>({ messenger: Symbol("messenger") as unknown as typeof globalThis.messenger, body: document.body, - selection: getSelection() as Selection, + selection, domParser: new DOMParser(), } satisfies ComposeConstants)
166-169: Replace theObject-hack with a tiny typed resolver for readability.Cleaner intent and fewer casts.
- this.composeBody = composeInjector(Object, ["body"]) as unknown as HTMLBodyElement + const resolve = <T, K extends keyof T & string>(inj: WirelessInjector<T>, key: K): T[K] => + inj(Object, [key]) as unknown as T[K] + this.composeBody = resolve(composeInjector, "body") as HTMLBodyElement
199-202: closeSession() should handle “not started” gracefully.Small guard avoids brittle expectations if called early.
- closeSession(): Promise<void> { - this.serverSocket.close() - return expect(this.session).resolves.toBeUndefined() - } + closeSession(): Promise<void> { + this.serverSocket.close() + const session = this.session ?? Promise.resolve() + return expect(session).resolves.toBeUndefined() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/root/startup/startup_background.ts(2 hunks)src/root/startup/startup_compose.ts(1 hunks)src/root/startup/startup_options.ts(1 hunks)src/root/util/wire.ts(2 hunks)src/root/util/wired.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/test/ghostbird.test.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/root/util/wireless.ts (1)
src/root/util/wire.ts (1)
IRegistry(121-121)
src/root/startup/startup_options.ts (2)
src/root/util/wireless.ts (1)
wireless(46-58)src/root/util/registry.ts (1)
makeRegistry(5-7)
src/root/startup/startup_background.ts (3)
src/ghosttext-runner/api.ts (1)
IHeart(11-17)src/root/util/wireless.ts (1)
wireless(46-58)src/root/util/registry.ts (1)
makeRegistry(5-7)
src/root/startup/startup_compose.ts (2)
src/root/util/wireless.ts (1)
wireless(46-58)src/root/util/registry.ts (1)
makeRegistry(5-7)
src/test/ghostbird.test.ts (11)
src/root/util/wireless.ts (2)
WirelessInjector(33-36)wireless(46-58)src/root/startup/startup_background.ts (2)
BackgroundCatalog(23-27)BackgroundConstants(16-21)src/root/util/registry.ts (1)
makeRegistry(5-7)src/root/startup/startup_compose.ts (2)
ComposeCatalog(15-19)ComposeConstants(8-13)src/ghosttext-adaptor/api.ts (2)
ComposeDetails(27-35)IComposeWindow(41-72)src/app-background/background_event_router.ts (1)
BackgroundEventRouter(6-56)src/app-compose/compose_event_router.ts (1)
ComposeEventRouter(6-37)src/ghosttext-runner/message.ts (1)
IMessagePort(25-60)src/thunderbird/util/promisified_message_port.ts (1)
promisifyMessageChannel(9-14)src/app-background/api.ts (1)
ITab(32-37)src/ghosttext-session/types.ts (2)
EditorChangeResponse(50-55)UpdateRequest(33-44)
src/root/util/wired.ts (1)
src/root/util/wire.ts (2)
IClassInfo(25-58)IRegistry(121-121)
🔇 Additional comments (8)
src/root/startup/startup_compose.ts (1)
23-23: Registry typing to Partial is correct and consistent.Matches wireless/wired signatures; removes brittle casting. No behavior change observed.
src/root/startup/startup_options.ts (1)
19-19: LGTM: registry now uses Partial.Aligned with DI utilities; safer than casting.
src/root/util/wired.ts (1)
14-20: Widened registry typing matches the DI flow.Interface remains stable; no functional change. Good cleanup.
src/root/startup/startup_background.ts (2)
31-34: LGTM: Partial registry.Consistent with other startup modules and DI utility typing.
5-20: IHeart interface properly enforced throughout dependency chain.Verification confirms the pattern is correct: AlarmHeart is instantiated in the bootstrap phase (background.ts:30) but immediately passed as
IHearttype toprepareBackgroundRouter. BackgroundConstants correctly typesheart: IHeart, and no problematic AlarmHeart-specific assumptions remain in the codebase.src/test/ghostbird.test.ts (3)
28-43: Good end-to-end test for toolbar start.Covers initial handshake and outbound update. Nice.
45-61: Second scenario reads well.Validates server->compose flow and final DOM state.
1-26: Verification complete — no issues found.The search confirms only two matches for
$injector, both insrc/root/util/wireless.tswhere the injector is intentionally registered. No accidental collisions or problematic references elsewhere in the codebase.
95dc12d to
e7a8731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/ghostbird.test.ts (1)
172-172: Consider simplifying the composeBody initialization.The cast
composeInjector(Object, ["body"]) as unknown as HTMLBodyElementworks but is convoluted. Since the registry setsbody: document.body(line 98), you could simplify this by directly storingdocument.bodyin the constructor or using a more straightforward retrieval.Example:
- this.composeBody = composeInjector(Object, ["body"]) as unknown as HTMLBodyElement + this.composeBody = document.body
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/root/startup/startup_background.ts(2 hunks)src/root/startup/startup_compose.ts(1 hunks)src/root/startup/startup_options.ts(1 hunks)src/root/util/wire.ts(2 hunks)src/root/util/wired.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/test/ghostbird.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/root/util/wired.ts
- src/root/startup/startup_compose.ts
- src/root/util/wireless.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/root/startup/startup_options.ts (2)
src/root/util/wireless.ts (1)
wireless(46-58)src/root/util/registry.ts (1)
makeRegistry(5-7)
src/test/ghostbird.test.ts (9)
src/root/util/wireless.ts (2)
WirelessInjector(33-36)wireless(46-58)src/root/startup/startup_background.ts (2)
BackgroundCatalog(23-27)BackgroundConstants(16-21)src/root/util/registry.ts (1)
makeRegistry(5-7)src/root/startup/startup_compose.ts (2)
ComposeCatalog(15-19)ComposeConstants(8-13)src/ghosttext-adaptor/api.ts (2)
ComposeDetails(27-35)IComposeWindow(41-72)src/ghosttext-runner/message.ts (1)
IMessagePort(25-60)src/thunderbird/util/promisified_message_port.ts (1)
promisifyMessageChannel(9-14)src/app-background/api.ts (1)
ITab(32-37)src/ghosttext-session/types.ts (2)
EditorChangeResponse(50-55)UpdateRequest(33-44)
src/root/startup/startup_background.ts (3)
src/ghosttext-runner/api.ts (1)
IHeart(11-17)src/root/util/wireless.ts (1)
wireless(46-58)src/root/util/registry.ts (1)
makeRegistry(5-7)
🔇 Additional comments (13)
src/root/util/wire.ts (2)
231-231: Documentation added for generic parameter.The JSDoc template clearly documents the TCatalog parameter.
236-247: Verify that partial registries are fully populated before use.The signature now accepts
IRegistry<Partial<TCatalog>>but returnsIWire<TCatalog>, with a runtime cast at line 247. This allows gradual registration but bypasses type safety—missing dependencies will only fail at runtime with "No registration for [name]" errors.Ensure that all startup code fully populates required dependencies before the wire is used to resolve instances.
src/root/startup/startup_options.ts (1)
19-19: LGTM! Cleaner generic usage.Replacing the explicit cast with a generic parameter is more idiomatic and aligns with the Partial pattern introduced in wire.ts.
src/root/startup/startup_background.ts (2)
5-5: Public API change: heart type widened from AlarmHeart to IHeart.The
BackgroundConstants.heartfield now expects theIHeartinterface instead of the concreteAlarmHeartclass. This increases flexibility for callers who can now provide any implementation ofIHeart. SinceAlarmHeartis still exported (line 14), existing code can continue to use it.Also applies to: 18-18
31-34: LGTM! Registry pattern aligned with other startup modules.The use of
makeRegistry<Partial<BackgroundCatalog>>(consts)follows the same pattern as startup_options.ts and startup_compose.ts, ensuring consistency across the codebase.src/test/ghostbird.test.ts (8)
1-27: LGTM! Imports aligned with expanded public API.The updated imports reflect the broader public type surface needed for the new Ghostbird test scaffolding, including
ITab,IComposeWindow,IGhostClientPort,UpdateRequest, and the newBackgroundConstantsandComposeConstantstypes.
28-62: LGTM! Well-structured tests using the new Ghostbird helper.The new test cases clearly demonstrate the Ghostbird helper's benefits:
- Clean setup with
new Ghostbird()- Expressive API:
startFromToolbar(),sendsToServer(),receives(),closeSession()- Good coverage of toolbar-initiated sessions and bidirectional communication
79-90: LGTM! Mock registry updated with Partial pattern.The use of
makeRegistry<Partial<BackgroundCatalog>>aligns with the updated registry typing pattern. Thesatisfiesclause ensures type correctness while allowing TypeScript to infer the precise type.
107-151: LGTM! Comprehensive mock implementation.The mocked Thunderbird catalog provides all necessary dependencies with appropriate stubs. The use of
satisfieson lines 123, 127, and 150 ensures the mocks conform to their respective interfaces while maintaining type inference.
153-175: LGTM! Well-designed test scaffold.The
Ghostbirdconstructor effectively encapsulates test setup:
- Message channels for background↔compose and client↔server communication
- Proper dependency injection using the mocked registries
- Clean initialization of routers and components
This significantly improves test readability and maintainability.
177-186: LGTM! Clear session lifecycle management.The
startFromToolbar()method correctly initiates both background and compose workflows, with proper session tracking to prevent duplicate starts.
188-195: LGTM! Clean bidirectional communication helpers.The
receives()andsendsToServer()methods provide clear APIs for testing message exchange between the editor server and the compose window.Also applies to: 197-202
204-208: LGTM! Proper test cleanup.The
closeSession()method ensures the server socket is closed and all promises are settled, preventing test leaks.
Summary by CodeRabbit
Tests
Refactor