docs: use mermaid on typedoc output#32
Conversation
📝 WalkthroughWalkthroughAdds GhostTextStarter and EmailEditorFactory, refactors GhostTextRunner to an instance/session API (GhostTextSession), introduces withRouter for lazy background router initialization and caching, makes ComposeTab tabId public, exports Alarm types, and updates tooling, configs, docs, and package manifest. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TB as Thunderbird UI
participant BG as Background (entry)
participant BH as background_helper
participant SB as startup_background
participant R as BackgroundEventRouter
User->>TB: Trigger compose action / command
TB->>BG: emit event (onClicked/onCommand/onMessage)
BG->>BH: withRouter(handler)
alt router cached
BH->>R: run handler with cached router
else initialize router
BH->>SB: prepareBackgroundRouter()
SB-->>BH: router
BH->>R: run handler with new router
end
R-->>BG: Handler completes
sequenceDiagram
autonumber
actor User
participant BG as ComposeActionNotifier
participant EF as EmailEditorFactory
participant ST as GhostTextStarter
participant HB as Heart
participant GC as GhostTextClient
participant GR as GhostTextRunner
participant SRV as GhostText Server
User->>BG: Compose action invoked
BG->>EF: create(tab, port)
EF->>EF: load client options
EF-->>BG: EmailEditor
BG->>ST: run(statusIndicator, editor)
ST->>HB: start heartbeat
ST->>GC: run() → GhostTextSession
ST->>GR: new GhostTextRunner(connector, options, statusIndicator, editor)
GR->>SRV: handshake/session commands
loop session active
GR->>SRV: send command
SRV-->>GR: command result
GR->>GR: update status/editor
end
GR-->>ST: close()
ST->>HB: stop heartbeat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
26-27: Restore the markdown TypeDoc plugin or adjust the script.
doc:mdstill invokes--plugin typedoc-plugin-markdown, but the dependency was dropped fromdevDependencies, so the script now fails. Either keep the plugin installed or update the script to match the new tooling.Apply this diff to keep the script working:
- "doc:md": "run doc --plugin typedoc-plugin-markdown", + "doc:md": "run doc",…or re-add
typedoc-plugin-markdowntodevDependenciesif you still need that plugin.src/ghosttext-runner/ghost_text_runner.ts (1)
15-82: Cache the latest client status instead of defaulting to “error”
lastStatuscurrently returns"error"whenever the session generator hasn’t finished yet. When callers invokeclose()mid-session (while we’re still handling commands like"notifyStatus"with"running"), the status indicator is incorrectly updated to"error", producing a false failure state for the user.Track the most recent translated status explicitly—update it whenever we process a
notifyStatuscommand and when the generator completes—solastStatusalways reflects reality.export class GhostTextRunner { private command: IteratorResult<Command, SessionStatus> | undefined private session: ISession | undefined + private status: ClientStatus = "inactive" @@ get lastStatus(): ClientStatus { - return this.command?.done ? translateStatus(this.command.value) : "error" + return this.status } @@ close(): Promise<void> { this.session?.close() - return this.statusIndicator.update(this.lastStatus).catch(() => {}) + return this.statusIndicator.update(this.lastStatus).catch(() => {}) } async run(g: GhostTextSession): Promise<void> { this.command = g.next() + this.updateFinalStatus() while (!this.command.done && this.command.value.type !== "queryEditor") { let [r, s] = await this.runHandshakeCommand(this.command.value) @@ this.session ??= s this.command = g.next(r) + this.updateFinalStatus() } if (!this.session) { return } while (!this.command.done) { let result = await this.runSessionCommand(this.session, this.command.value) this.command = g.next(result) + this.updateFinalStatus() } } async runHandshakeCommand(command: Command): Promise<[CommandResult, ISession?]> { switch (command.type) { @@ case "notifyStatus": - await this.statusIndicator.update(translateStatus(command.status)) + this.status = translateStatus(command.status) + await this.statusIndicator.update(this.status) return [{ type: "statusUpdated" }] @@ async runSessionCommand(session: ISession, command: Command): Promise<CommandResult> { @@ case "notifyStatus": - await this.statusIndicator.update(translateStatus(command.status)) + this.status = translateStatus(command.status) + await this.statusIndicator.update(this.status) return { type: "statusUpdated" } @@ } + + private updateFinalStatus(): void { + if (this.command?.done) { + this.status = translateStatus(this.command.value) + } + } }
🧹 Nitpick comments (3)
tools/tsdown_config.ts (1)
55-55: Clarify use of empty regex in noExternalThe pattern
/(?:)/matches the empty string at every position, so it will treat all dependencies as non-external (bundling everything)—equivalent totrue. If you intended to bundle every dependency, usetrue; otherwise supply a more specific regex.src/test/startup.test.ts (1)
148-154: Consider edge cases in the regex pattern.The regex pattern
/\bconstructor\s*[(]\s*[{]?\s*[^{]+[{][^}]+[}]/dmay not match all constructor formats, particularly:
- Constructors with complex parameter destructuring
- Multi-line constructors with irregular spacing
- Constructors with nested braces
While this appears to work for the current codebase, consider adding a comment explaining the expected constructor format or documenting known limitations.
src/ghosttext-runner/ghost_text_starter.ts (1)
15-28: Excellent orchestration with proper cleanup.The
runmethod effectively coordinates the GhostText workflow:
- Heartbeat lifecycle is properly managed (start/stop)
- Options are loaded before runner creation
- Cleanup in the
finallyblock ensures resources are released regardless of success or failure- The flow integrates well with the new
GhostTextSessiontypeConsider the production console.log.
The console.log on Line 24 (
"generator finished") may be verbose for production environments. Consider whether this should be at a different log level or removed for production builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.github/workflows/doc.yml(1 hunks).vscode/tasks.example.json(1 hunks).web-ext-config.example.mjs(1 hunks)BUILDING.md(1 hunks)CONTRIBUTING.md(1 hunks)README.md(7 hunks)biome.jsonc(1 hunks)doc/design.md(5 hunks)doc/faq.md(3 hunks)doc/res/typedoc.cjs(1 hunks)ext/NOTICE.md(1 hunks)package.json(2 hunks)src/app-background/background_event_router.ts(1 hunks)src/app-background/compose_action_notifier.ts(2 hunks)src/app-options/api.ts(1 hunks)src/ghosttext-adaptor/email_editor_factory.ts(1 hunks)src/ghosttext-runner/ghost_text_runner.ts(2 hunks)src/ghosttext-runner/ghost_text_starter.ts(1 hunks)src/ghosttext-session/ghost_text_client.ts(1 hunks)src/root/background.ts(1 hunks)src/root/background_helper.ts(1 hunks)src/root/startup/startup_background.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/test/startup.test.ts(1 hunks)src/thunderbird/background_util/compose_tab.ts(4 hunks)src/thunderbird/messenger/alarms.ts(2 hunks)src/thunderbird/options_util/options_store.ts(0 hunks)tools/tsdown_config.ts(1 hunks)tools/vitest_config.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/thunderbird/options_util/options_store.ts
🧰 Additional context used
🧬 Code graph analysis (7)
src/ghosttext-runner/ghost_text_starter.ts (3)
src/ghosttext-runner/api.ts (5)
IGhostTextConnector(20-27)IClientOptionsLoader(39-44)IHeart(11-17)IStatusIndicator(109-116)IClientEditor(74-96)src/ghosttext-session/ghost_text_client.ts (1)
GhostTextClient(30-70)src/ghosttext-runner/ghost_text_runner.ts (1)
GhostTextRunner(4-86)
src/root/util/wireless.ts (2)
src/root/util/wire.ts (4)
wire(157-160)wire(233-247)IClassInfo(25-58)KeyOf(20-20)src/root/util/wired.ts (1)
wired(16-21)
src/ghosttext-adaptor/email_editor_factory.ts (3)
src/ghosttext-runner/api.ts (1)
IClientOptionsLoader(39-44)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(34-65)src/ghosttext-adaptor/email_editor.ts (1)
EmailEditor(12-54)
src/root/background_helper.ts (2)
src/app-background/background_event_router.ts (1)
BackgroundEventRouter(5-52)src/root/startup/startup_background.ts (1)
prepareBackgroundRouter(27-35)
src/app-background/compose_action_notifier.ts (2)
src/ghosttext-runner/ghost_text_starter.ts (1)
GhostTextStarter(5-29)src/ghosttext-adaptor/email_editor_factory.ts (1)
EmailEditorFactory(5-14)
src/ghosttext-runner/ghost_text_runner.ts (2)
src/ghosttext-session/ghost_text_client.ts (4)
Command(12-17)SessionStatus(10-10)GhostTextSession(28-28)CommandResult(19-26)src/ghosttext-runner/api.ts (6)
ISession(47-69)IGhostTextConnector(20-27)ClientOptions(30-34)IStatusIndicator(109-116)IClientEditor(74-96)ClientStatus(106-106)
src/thunderbird/background_util/compose_tab.ts (3)
src/ghosttext-runner/message.ts (1)
MessagesFromBackground(3-6)src/ghosttext-adaptor/api.ts (1)
IGhostServerPort(91-91)src/thunderbird/util/promisified_port.ts (1)
PromisifiedPort(12-67)
🔇 Additional comments (21)
src/app-options/api.ts (1)
7-7: LGTM! Documentation now matches the parameter name.The JSDoc
@paramtag now correctly references "form" instead of the previous parameter name, improving documentation accuracy and consistency with the function signature.src/thunderbird/messenger/alarms.ts (1)
30-30: Approve exported types
ExportingAlarmInfoandAlarmis an additive, non-breaking change—no internal imports are required, as these types are exposed for external consumers.src/thunderbird/background_util/compose_tab.ts (2)
8-9: LGTM! Clean refactoring to public property.The change from
private readonly idwith a getter toreadonly tabIdis more idiomatic and provides direct access to the property. All internal usages have been consistently updated throughout the class.
60-64: LGTM! Improved debugging with port naming.Adding a
nameparameter to the port connection usingimport.meta.urlis a good practice. This will help identify which module created the connection during debugging, making it easier to trace port lifecycle issues.src/root/util/wireless.ts (3)
49-50: LGTM!The registry key change from
"$wire$"to"$injector"better reflects the purpose of the registered function. The comment clearly explains the intent.
52-52: LGTM!Returning the
injectorwrapper instead of an inline function improves code clarity and maintainability.
45-47: Optional deps behavior and type‐safety are safe.parseConstructorForDependencyNamesis fully tested and all current injector calls pass explicit deps, so no runtime risks detected.CONTRIBUTING.md (1)
4-4: LGTM!The addition of the MDN open source etiquette link provides helpful guidance for contributors.
.web-ext-config.example.mjs (1)
8-8: LGTM!Using
import.meta.dirnamemakes the path relative to the module location, improving portability. This is compatible with the Node.js v22 LTS requirement mentioned in CONTRIBUTING.md.tools/vitest_config.ts (1)
4-5: LGTM!Extracting the output directory path into a constant eliminates duplication and improves maintainability.
BUILDING.md (1)
1-7: Referenced documentation files exist. Both CONTRIBUTING.md and doc/building.md are present.biome.jsonc (1)
2-2: No action required Schema version 2.2.5 is valid and contains only patch fixes with no breaking changes..github/workflows/doc.yml (1)
54-54: Validate upload-pages-artifact@v4 upgrade
In.github/workflows/doc.ymlline 54, confirm v4’s removal of dotfiles and its pinned upload-artifact SHA won’t break your workflow—review the v4 release notes and adjust include/exclude patterns if needed.src/app-background/background_event_router.ts (1)
23-23: LGTM!The JSDoc comment clearly describes the method's purpose and aligns with the centralized routing changes introduced in this PR.
src/ghosttext-session/ghost_text_client.ts (1)
28-33: LGTM!The introduction of the
GhostTextSessiontype alias improves code readability and maintainability. The type signature correctly represents the generator's behavior.src/test/startup.test.ts (1)
156-171: Good use of soft assertions for better error reporting.The refactoring correctly uses
expect.softto collect multiple dependency usage failures in a single test run, which improves the developer experience. The destructuring pattern[name, [method, arg]]and the updated error message are clear improvements.src/root/background.ts (1)
6-28: LGTM! Centralized routing simplifies the background script.The refactoring successfully delegates routing logic to the centralized
withRouterhelper frombackground_helper. This:
- Removes duplicated router management code
- Provides consistent error handling across all event listeners
- Simplifies the background script's responsibility to event registration only
The immediate startup log on Line 8 complements the existing "started" log on Line 34, providing visibility into the initialization lifecycle.
src/ghosttext-adaptor/email_editor_factory.ts (1)
5-14: LGTM! Clean factory pattern implementation.The
EmailEditorFactoryproperly encapsulates the creation logic forEmailEditorinstances:
- Singleton design is appropriate for this factory
- Dependency injection of
IClientOptionsLoaderenables testability- The
createmethod correctly loads options before constructing the editor- Clear separation of concerns between option loading and editor instantiation
This aligns well with the refactoring objectives of the PR.
src/app-background/compose_action_notifier.ts (2)
8-11: LGTM! Dependency injection aligned with factory pattern.The constructor correctly uses the new
GhostTextStarterandEmailEditorFactorydependencies, replacing the previous direct usage ofGhostTextRunnerandIClientOptionsLoader. This change:
- Aligns with the factory and starter patterns introduced in this PR
- Improves testability through dependency injection
- Delegates orchestration concerns to appropriate components
23-26: LGTM! Factory-based editor creation is clean.The refactored
startmethod correctly:
- Uses
emailEditorFactory.createto obtain anEmailEditorinstance- Passes the editor as both
statusIndicatorandeditorparameters toghostTextStarter.run, which is valid sinceEmailEditorimplements bothIStatusIndicatorandIClientEditorinterfaces (as confirmed by the relevant code snippets)This simplifies the method by removing direct option loading and editor instantiation.
src/root/startup/startup_background.ts (1)
1-1: Manually verify removal ofcommandConfigreferences
Automated searches yielded no files due to glob filters; please confirm no code still referencescommandConfig.
snipsnipsnip
force-pushed
the
fix-typedoc
branch
2 times, most recently
from
40a4163 to
a08c133
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
69-83: Restore the markdown plugin dependency.
yarn doc:mdstill runstypedoc --plugin typedoc-plugin-markdown, but the package is no longer listed anywhere in dependencies. TypeDoc will now fail to load that plugin, breaking the docs pipeline. Please addtypedoc-plugin-markdownback todevDependencies(or drop the script if it’s truly unused)."devDependencies": { "@biomejs/biome": "^2.2.5", "@types/node": "^24.3.0", @@ - "typedoc": "^0.28.12", - "typedoc-plugin-mdn-links": "^5.0.9", + "typedoc": "^0.28.12", + "typedoc-plugin-markdown": "^4.2.4", + "typedoc-plugin-mdn-links": "^5.0.9",
♻️ Duplicate comments (1)
src/root/background_helper.ts (1)
15-50: Address the concurrent initialization race condition flagged in previous review.As noted in the previous review, concurrent calls to
withRouterbefore the first initialization completes can result in duplicate router initialization. The suggested fix withrouterInitmemoization is correct and should be implemented.Additionally, consider whether
withRouterSyncshould returnundefinedon error. Currently, errors are logged but not propagated, which may hide issues from callers who expect a Promise rejection.Apply the diff from the previous review comment to fix the race condition. Additionally, consider this adjustment to error handling:
function withRouterSync(handler: RouterHandler, router: BackgroundEventRouter): Promise<void> | undefined { try { return handler(router) } catch (error) { console.error({ error }) - return + return Promise.reject(error) } }This ensures errors are visible to callers who may need to handle failures appropriately.
🧹 Nitpick comments (2)
.web-ext-config.example.mjs (1)
8-8: Consider using path.join() for cross-platform compatibility.While the change to use
import.meta.dirnameimproves portability by removing hard-coded paths, string concatenation may cause issues on Windows or whenimport.meta.dirnamehas trailing separators.Apply this diff for better cross-platform path handling:
+import { join } from "node:path"; + // You have to adjust it to debug // See: https://extensionworkshop.com/documentation/develop/web-ext-command-reference/#web-ext-run // See: https://extensionworkshop.com/documentation/develop/getting-started-with-web-ext/#use-a-custom-profile export default { run: { // You can use flatpak by specifying "flatpak:org.mozilla.Thunderbird" firefox: "/usr/bin/thunderbird", - firefoxProfile: import.meta.dirname + "/.cache/thunderbird-testing-profile/", + firefoxProfile: join(import.meta.dirname, ".cache", "thunderbird-testing-profile"), profileCreateIfMissing: true, // keepProfileChanges: true, }, };Note:
import.meta.dirnamerequires Node.js 20.11.0+. Ensure this minimum version is documented if not already specified.src/ghosttext-runner/ghost_text_runner.ts (1)
19-22: Consider propagating status update errors in close().The
close()method safely handles session cleanup with optional chaining. However, errors fromstatusIndicator.update()are silently caught and ignored. Consider whether callers should be aware of status update failures.If status update failures are non-critical, the current implementation is acceptable. Otherwise, consider:
close(): Promise<void> { this.session?.close() - return this.statusIndicator.update(this.lastStatus).catch(() => {}) + return this.statusIndicator.update(this.lastStatus) }This allows callers to decide how to handle status update failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.github/workflows/doc.yml(1 hunks).vscode/tasks.example.json(1 hunks).web-ext-config.example.mjs(1 hunks)BUILDING.md(1 hunks)CONTRIBUTING.md(1 hunks)README.md(7 hunks)doc/design.md(5 hunks)doc/faq.md(3 hunks)doc/res/typedoc.cjs(1 hunks)ext/NOTICE.md(1 hunks)package.json(2 hunks)src/app-background/background_event_router.ts(1 hunks)src/app-background/compose_action_notifier.ts(2 hunks)src/app-options/api.ts(1 hunks)src/ghosttext-adaptor/email_editor_factory.ts(1 hunks)src/ghosttext-runner/ghost_text_runner.ts(2 hunks)src/ghosttext-runner/ghost_text_starter.ts(1 hunks)src/ghosttext-session/ghost_text_client.ts(1 hunks)src/root/background.ts(1 hunks)src/root/background_helper.ts(1 hunks)src/root/startup/startup_background.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/thunderbird/background_util/compose_tab.ts(4 hunks)src/thunderbird/messenger/alarms.ts(2 hunks)src/thunderbird/options_util/options_store.ts(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/thunderbird/options_util/options_store.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/doc.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- src/app-options/api.ts
- doc/faq.md
- src/root/startup/startup_background.ts
- BUILDING.md
- src/ghosttext-runner/ghost_text_starter.ts
- .vscode/tasks.example.json
- src/thunderbird/messenger/alarms.ts
- doc/res/typedoc.cjs
- doc/design.md
- src/thunderbird/background_util/compose_tab.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/root/background_helper.ts (2)
src/app-background/background_event_router.ts (1)
BackgroundEventRouter(5-52)src/root/startup/startup_background.ts (1)
prepareBackgroundRouter(27-35)
src/app-background/compose_action_notifier.ts (2)
src/ghosttext-runner/ghost_text_starter.ts (1)
GhostTextStarter(5-29)src/ghosttext-adaptor/email_editor_factory.ts (1)
EmailEditorFactory(5-14)
src/ghosttext-runner/ghost_text_runner.ts (2)
src/ghosttext-session/ghost_text_client.ts (4)
Command(12-17)SessionStatus(10-10)GhostTextSession(28-28)CommandResult(19-26)src/ghosttext-runner/api.ts (6)
ISession(47-69)IGhostTextConnector(20-27)ClientOptions(30-34)IStatusIndicator(109-116)IClientEditor(74-96)ClientStatus(106-106)
src/root/util/wireless.ts (2)
src/root/util/wire.ts (4)
wire(157-160)wire(233-247)IClassInfo(25-58)KeyOf(20-20)src/root/util/wired.ts (1)
wired(16-21)
src/ghosttext-adaptor/email_editor_factory.ts (3)
src/ghosttext-runner/api.ts (1)
IClientOptionsLoader(39-44)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(34-65)src/ghosttext-adaptor/email_editor.ts (1)
EmailEditor(12-54)
🔇 Additional comments (20)
src/root/util/wireless.ts (3)
45-45: LGTM!The refactoring to use constants improves code readability and makes the flow clearer.
Also applies to: 52-52
49-50: Registry key change verified—no remaining$wire$references.
45-47: Verify auto-resolved dependency names and optimize parsingThe unchecked cast from
string[]toKeyOf<TCatalog>[]inwireless.ts(lines 45–47) assumes constructor parameter names exactly match catalog keys, risking runtime failures if they don’t. Manually confirm all injected constructors align with yourTCatalogkeys or require explicitdeps. To reduce overhead, cacheparseConstructorForDependencyNamesresults in aWeakMapso each constructor is parsed only once.src/app-background/background_event_router.ts (1)
23-23: LGTM! Clear documentation added.The JSDoc comment accurately describes the method's purpose and improves code readability.
src/ghosttext-session/ghost_text_client.ts (2)
28-28: LGTM! Type alias improves code clarity.Introducing the
GhostTextSessiontype alias improves readability and enables consistent typing across the codebase.
33-33: LGTM! Return type correctly uses the new type alias.The method signature correctly references the newly introduced
GhostTextSessiontype, maintaining type safety.src/root/background.ts (3)
6-6: LGTM! Centralized router management.Importing
withRouterfrom the dedicated helper module improves separation of concerns and code organization.
10-20: LGTM! Event handlers correctly use withRouter.The event listeners are properly wrapped with
withRouter, which handles initialization and error handling consistently. The error handling for unknown commands is appropriate.
22-28: LGTM! Message handler correctly integrated.The
onMessagelistener properly useswithRouterand thesendResponsecallback pattern.src/ghosttext-adaptor/email_editor_factory.ts (2)
5-8: LGTM! Clean factory pattern with singleton indication.The factory class is well-structured with proper dependency injection. The
isSingletonflag correctly indicates this should be a singleton instance.
10-13: Consider error handling for options loading. Thecreatemethod can reject ifclientOptionsLoader.load()fails; ensure callers handle this.Run:
#!/bin/bash # Locate callers of EmailEditorFactory.create rg -nP 'EmailEditorFactory\\.create\\(' -C5 -g '!**/test/**'src/app-background/compose_action_notifier.ts (2)
8-11: LGTM! Constructor updated to use factory pattern.The constructor correctly accepts
GhostTextStarterandEmailEditorFactorydependencies, enabling better separation of concerns and testability.
23-30: LGTM! Start method correctly uses factory pattern.The refactored
startmethod:
- Uses
emailEditorFactory.create()for editor creation- Properly delegates to
ghostTextStarter.run()- Maintains the try-finally block for cleanup
The error handling and cleanup logic are preserved correctly.
src/ghosttext-runner/ghost_text_runner.ts (6)
8-13: LGTM! Constructor refactored for instance-based state.The constructor now properly injects
options,statusIndicator, andeditoras dependencies, enabling instance-based state management. This is a cleaner design compared to passing these in therun()method.
15-17: LGTM! Status getter provides clean access to session state.The
lastStatusgetter correctly derivesClientStatusfrom the generator state using thetranslateStatushelper, with an appropriate fallback to"error"for incomplete states.
24-44: LGTM! Clean two-phase session flow.The refactored
run()method implements a clear two-phase approach:
- Handshake phase (lines 27-33): Establishes connection and session
- Session phase (lines 39-43): Processes commands during active session
The guard clause at lines 35-37 correctly handles the case where handshake fails to establish a session.
46-64: LGTM! Handshake command handling is robust.The
runHandshakeCommandmethod:
- Correctly returns a tuple
[CommandResult, ISession?]- Only returns a session from the
"connect"command- Properly handles connection failures with error capture
- Uses TypeScript exhaustiveness checking
- Appropriately rejects unexpected commands during handshake
66-85: LGTM! Session command handling is well-structured.The
runSessionCommandmethod:
- Correctly uses instance fields for editor and status indicator
- Delegates editor-session coordination to the
receiveChangehelper- Handles all session command types appropriately
- Maintains TypeScript exhaustiveness checking
- Properly rejects unexpected commands during session
88-96: Clarify retry behavior when no edit or change is returned.
receiveChangeOncereturnsundefinedonly ifeditor.popLastEdit()orsession.popServerChange()yields a falsy value, causingreceiveChangeto loop forever. Confirm this infinite‐retry on missing data is intentional and won’t lead to unbounded loops in edge cases.CONTRIBUTING.md (1)
4-4: Verified MDN Open source etiquette link is accessible (HTTP 200).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
26-83: Restoretypedoc-plugin-markdownfordoc:md.
doc:mdstill invokestypedoc --plugin typedoc-plugin-markdown, but the dependency was dropped from devDependencies. Running that script will now fail with “Cannot load plugin typedoc-plugin-markdown”. Please add the package back (or adjust the script)."devDependencies": { "@biomejs/biome": "^2.2.5", @@ - "typedoc-plugin-mdn-links": "^5.0.9", - "typescript": "^5.9.3", + "typedoc-plugin-markdown": "^4.1.0", + "typedoc-plugin-mdn-links": "^5.0.9", + "typescript": "^5.9.3",
♻️ Duplicate comments (1)
src/root/background_helper.ts (1)
45-50: Address the race condition flagged in the previous review.The concurrent initialization issue identified in the previous review remains unresolved. Two background events arriving before the first initialization completes will both call
prepareBackgroundRouter()and register handlers twice.
🧹 Nitpick comments (4)
.web-ext-config.example.mjs (1)
8-8: Usepath.join()and declare Node.js version requirement.
- Replace string concatenation with
path.join(import.meta.dirname, ".cache", "thunderbird-testing-profile")for cross-platform compatibility.- Add
"node": ">=20.11.0"topackage.jsonengines or document the minimum Node.js version in your project docs.src/ghosttext-runner/ghost_text_starter.ts (1)
24-24: Consider removing or elevating the debug log.The
console.log("generator finished")statement appears to be a development/debug artifact. Consider either removing it or replacing it with a proper logging mechanism if this information is valuable for troubleshooting.Apply this diff to remove the debug statement:
} finally { - console.log("generator finished") await runner.close() stopHeartbeat()src/ghosttext-runner/ghost_text_runner.ts (1)
19-22: Consider logging or handling errors from status update.The
close()method silently swallows errors fromstatusIndicator.update()via.catch(() => {}). While this prevents cleanup failures from propagating, it may hide legitimate issues with status updates during shutdown.Consider logging the error for troubleshooting:
close(): Promise<void> { this.session?.close() - return this.statusIndicator.update(this.lastStatus).catch(() => {}) + return this.statusIndicator.update(this.lastStatus).catch((error) => { + console.warn("Failed to update status during close:", error) + }) }src/root/util/wireless.ts (1)
45-47: Enhanced default dependency resolution improves API ergonomics.The introduction of the
injectorwrapper with automatic dependency parsing viaparseConstructorForDependencyNameswhendepsis undefined is a good enhancement. The explicitconstdeclarations make the code clearer.Note the type safety trade-offs:
- Line 45: The double cast
as unknown as Iterable<IClassInfo<TCatalog>>suppresses variance issues inherent to dynamic class discovery. This is acceptable given the convention-based nature of this DI system.- Line 47: The cast to
KeyOf<TCatalog>[]assumes parsed dependency names will match catalog keys. If names don't match, resolution will fail loudly at runtime, which is appropriate behavior for a DI system.Performance consideration:
parseConstructorForDependencyNamesperforms regex parsing on every injection whendepsis undefined. This is likely fine for most use cases, but could become a bottleneck if certain classes are instantiated very frequently. Consider memoizing the parsing results if performance issues arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.github/workflows/doc.yml(1 hunks).web-ext-config.example.mjs(1 hunks)doc/res/typedoc.cjs(1 hunks)package.json(2 hunks)src/app-background/background_event_router.ts(1 hunks)src/app-background/compose_action_notifier.ts(2 hunks)src/app-options/api.ts(1 hunks)src/ghosttext-adaptor/email_editor_factory.ts(1 hunks)src/ghosttext-runner/ghost_text_runner.ts(2 hunks)src/ghosttext-runner/ghost_text_starter.ts(1 hunks)src/ghosttext-session/ghost_text_client.ts(1 hunks)src/root/background.ts(1 hunks)src/root/background_helper.ts(1 hunks)src/root/startup/startup_background.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/thunderbird/background_util/compose_tab.ts(4 hunks)src/thunderbird/messenger/alarms.ts(2 hunks)src/thunderbird/options_util/options_store.ts(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/thunderbird/options_util/options_store.ts
✅ Files skipped from review due to trivial changes (1)
- doc/res/typedoc.cjs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/ghosttext-session/ghost_text_client.ts
- .github/workflows/doc.yml
- src/root/background.ts
- src/thunderbird/background_util/compose_tab.ts
- src/app-options/api.ts
- src/thunderbird/messenger/alarms.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/ghosttext-adaptor/email_editor_factory.ts (3)
src/ghosttext-runner/api.ts (1)
IClientOptionsLoader(39-44)src/ghosttext-adaptor/api.ts (1)
IComposeWindow(34-65)src/ghosttext-adaptor/email_editor.ts (1)
EmailEditor(12-54)
src/app-background/compose_action_notifier.ts (3)
src/ghosttext-adaptor/api.ts (1)
IGhostServerPort(91-91)src/ghosttext-runner/ghost_text_starter.ts (1)
GhostTextStarter(5-29)src/ghosttext-adaptor/email_editor_factory.ts (1)
EmailEditorFactory(5-14)
src/ghosttext-runner/ghost_text_starter.ts (3)
src/ghosttext-runner/api.ts (5)
IGhostTextConnector(20-27)IClientOptionsLoader(39-44)IHeart(11-17)IStatusIndicator(109-116)IClientEditor(74-96)src/ghosttext-session/ghost_text_client.ts (1)
GhostTextClient(30-70)src/ghosttext-runner/ghost_text_runner.ts (1)
GhostTextRunner(4-86)
src/ghosttext-runner/ghost_text_runner.ts (2)
src/ghosttext-session/ghost_text_client.ts (4)
Command(12-17)SessionStatus(10-10)GhostTextSession(28-28)CommandResult(19-26)src/ghosttext-runner/api.ts (6)
ISession(47-69)IGhostTextConnector(20-27)ClientOptions(30-34)IStatusIndicator(109-116)IClientEditor(74-96)ClientStatus(106-106)
src/root/background_helper.ts (2)
src/app-background/background_event_router.ts (1)
BackgroundEventRouter(5-52)src/root/startup/startup_background.ts (1)
prepareBackgroundRouter(27-35)
src/root/util/wireless.ts (2)
src/root/util/wire.ts (4)
wire(157-160)wire(233-247)IClassInfo(25-58)KeyOf(20-20)src/root/util/wired.ts (1)
wired(16-21)
🔇 Additional comments (17)
src/app-background/background_event_router.ts (1)
23-23: LGTM!The JSDoc comment accurately describes the method's purpose and improves code documentation.
src/root/startup/startup_background.ts (1)
1-1: LGTM!The removal of
ICommandConfigfrom the import aligns with the refactoring to externalize routing logic and reduce the public surface area of this module.src/root/background_helper.ts (2)
15-21: LGTM!The branching logic correctly delegates to synchronous or asynchronous paths based on router availability.
30-37: LGTM!The error handling is appropriate, logging errors while returning
undefinedto prevent exceptions from propagating.src/ghosttext-runner/ghost_text_starter.ts (1)
5-13: LGTM! Clean dependency injection pattern.The singleton marker and constructor with injected dependencies follow a clear pattern for orchestrating the GhostText session lifecycle.
src/ghosttext-adaptor/email_editor_factory.ts (1)
5-13: LGTM! Simple and effective factory pattern.The factory correctly encapsulates the creation logic for
EmailEditorinstances, loading options and constructing the editor. The pattern is appropriate for the use case where editors are created per session.src/app-background/compose_action_notifier.ts (3)
1-2: LGTM! Imports updated to reflect the new architecture.The import statements correctly reference the new factory and starter classes, replacing the previous direct imports.
8-11: LGTM! Constructor refactored to use factory and starter patterns.The constructor signature cleanly reflects the shift from direct component instantiation to dependency injection of the factory and starter, improving testability and separation of concerns.
25-26: LGTM! Clean integration of factory and starter.The refactored flow correctly uses the factory to create the editor and the starter to run the session, maintaining the same overall behavior while improving modularity.
src/ghosttext-runner/ghost_text_runner.ts (6)
1-2: LGTM! Imports updated for refactored architecture.The import statements correctly reflect the shift to using
GhostTextSessionand related types from the session module, aligning with the new generator-based pattern.
8-13: LGTM! Constructor refactored for improved dependency injection.The constructor now directly accepts the required dependencies (
options,statusIndicator,editor) rather than managing their lifecycle, which improves testability and separation of concerns.
15-17: LGTM! Clean status accessor.The
lastStatusgetter provides a convenient way to access the final session status, with appropriate fallback to "error" when the command is not done.
24-44: LGTM! Well-structured generator-based session flow.The refactored
run()method cleanly separates the handshake phase from the session phase, iterating through the generator commands and handling early termination if the session fails to establish.
46-64: LGTM! Proper handshake command handling.The method correctly handles connection establishment and status notifications during the handshake phase, with appropriate error handling and type exhaustiveness checking.
66-86: LGTM! Robust session command handling.The method properly routes session commands to the appropriate handlers, with good separation of concerns and exhaustiveness checking to ensure all command types are handled.
tsconfig.json (1)
35-36: customJs path confirmed:doc/res/typedoc.cjsexists as configured. No changes needed.src/root/util/wireless.ts (1)
49-50: No$wire$references found—key rename is safe
Search across TS/JS files returned no occurrences of$wire$, so switching the registry key to$injectordoes not introduce a breaking change.
snipsnipsnip
force-pushed
the
fix-typedoc
branch
from
a08c133 to
e572c89
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/thunderbird/messenger/alarms.ts (1)
30-36: Fix JSDoc spacing on line 31
Insert a space before the closing*/so it reads:/** Number of minutes from the current time after which the alarm should first fire. */ delayInMinutes: number[src/thunderbird/messenger/alarms.ts:31]
♻️ Duplicate comments (1)
src/root/background_helper.ts (1)
23-30: Reset routerPromise when initialization failsIf
import("./startup/startup_background")orprepareBackgroundRouter()rejects, the rejected promise stays cached inrouterPromise, so every later call immediately fails and the router is never retried. Reset the promise (and cached router) on failure so we can recover from transient startup issues.Apply this diff:
- routerPromise ??= import("./startup/startup_background").then((s) => s.prepareBackgroundRouter()) + routerPromise ??= import("./startup/startup_background") + .then((s) => s.prepareBackgroundRouter()) + .catch((error) => { + console.error({ error }) + routerPromise = undefined + routerCache = undefined + throw error + })
🧹 Nitpick comments (5)
.web-ext-config.example.mjs (1)
8-8: Good portability improvement.Switching to
import.meta.dirnamemakes this example configuration portable across different development environments instead of requiring manual path edits.Note:
import.meta.dirnamerequires Node.js ≥20.11 or ≥21.2. If targeting older versions, consider usingpath.dirname(new URL(import.meta.url).pathname)instead.For slightly better cross-platform explicitness, you could use:
- firefoxProfile: import.meta.dirname + "/.cache/thunderbird-testing-profile/", + firefoxProfile: import.meta.dirname + "/.cache/thunderbird-testing-profile/",But string concatenation with
/works fine in Node.js across platforms, so this is optional.src/test/startup.test.ts (1)
140-141: Use explicit boolean conversion for clarity.The variable
hasDependencysuggests a boolean value, butdeps.lengthreturns a number. While this works due to JavaScript's truthiness rules, it's clearer and more idiomatic to explicitly convert to boolean.Apply this diff:
- const hasDependency = deps.length + const hasDependency = deps.length > 0 if (hasDependency) {doc/res/typedoc.cjs (1)
3-4: Consider usingconstfor immutable bindings.Since
nodesandmermaidPromiseare not reassigned, declare them withconstinstead ofletto signal immutability.Apply this diff:
-let nodes = document.querySelectorAll('code.mermaid'); -let mermaidPromise = nodes.length ? import('https://cdn.jsdelivr.net/npm/mermaid@11.12.0/dist/mermaid.esm.min.mjs') : null; +const nodes = document.querySelectorAll('code.mermaid'); +const mermaidPromise = nodes.length ? import('https://cdn.jsdelivr.net/npm/mermaid@11.12.0/dist/mermaid.esm.min.mjs') : null;src/ghosttext-runner/ghost_text_starter.ts (1)
24-24: Consider removing or improving the debug log.The
console.logstatement appears to be debug code. Consider either removing it or replacing it with a proper logging mechanism if this information is valuable for troubleshooting.src/ghosttext-runner/ghost_text_runner.ts (1)
19-22: Consider logging suppressed errors in close().The
catch(() => {})on line 21 silently suppresses errors from the status indicator update. While this is reasonable for a cleanup method, logging these errors might help with debugging.close(): Promise<void> { this.session?.close() - return this.statusIndicator.update(this.lastStatus).catch(() => {}) + return this.statusIndicator.update(this.lastStatus).catch((err) => { + console.error("Failed to update status indicator during close:", err) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.github/workflows/doc.yml(1 hunks).vscode/tasks.example.json(1 hunks).web-ext-config.example.mjs(1 hunks)BUILDING.md(1 hunks)CONTRIBUTING.md(1 hunks)README.md(7 hunks)biome.jsonc(1 hunks)doc/design.md(5 hunks)doc/faq.md(3 hunks)doc/res/typedoc.cjs(1 hunks)ext/NOTICE.md(1 hunks)package.json(2 hunks)src/app-background/background_event_router.ts(1 hunks)src/app-background/compose_action_notifier.ts(2 hunks)src/app-options/api.ts(1 hunks)src/ghosttext-adaptor/email_editor_factory.ts(1 hunks)src/ghosttext-runner/ghost_text_runner.ts(2 hunks)src/ghosttext-runner/ghost_text_starter.ts(1 hunks)src/ghosttext-session/ghost_text_client.ts(1 hunks)src/root/background.ts(1 hunks)src/root/background_helper.ts(1 hunks)src/root/startup/startup_background.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/test/startup.test.ts(2 hunks)src/thunderbird/background_util/compose_tab.ts(4 hunks)src/thunderbird/messenger/alarms.ts(2 hunks)src/thunderbird/options_util/options_store.ts(0 hunks)tools/tsdown_config.ts(1 hunks)tools/vitest_config.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/thunderbird/options_util/options_store.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/app-background/compose_action_notifier.ts
- .vscode/tasks.example.json
- .github/workflows/doc.yml
- src/ghosttext-adaptor/email_editor_factory.ts
- tools/vitest_config.ts
- doc/design.md
- BUILDING.md
- src/root/util/wireless.ts
- CONTRIBUTING.md
- doc/faq.md
- src/ghosttext-session/ghost_text_client.ts
- package.json
- tools/tsdown_config.ts
- biome.jsonc
🧰 Additional context used
🧬 Code graph analysis (4)
src/ghosttext-runner/ghost_text_starter.ts (3)
src/ghosttext-runner/api.ts (5)
IGhostTextConnector(20-27)IClientOptionsLoader(39-44)IHeart(11-17)IStatusIndicator(109-116)IClientEditor(74-96)src/ghosttext-session/ghost_text_client.ts (1)
GhostTextClient(30-70)src/ghosttext-runner/ghost_text_runner.ts (1)
GhostTextRunner(4-86)
src/root/background_helper.ts (1)
src/app-background/background_event_router.ts (1)
BackgroundEventRouter(5-52)
src/ghosttext-runner/ghost_text_runner.ts (2)
src/ghosttext-session/ghost_text_client.ts (4)
Command(12-17)SessionStatus(10-10)GhostTextSession(28-28)CommandResult(19-26)src/ghosttext-runner/api.ts (6)
ISession(47-69)IGhostTextConnector(20-27)ClientOptions(30-34)IStatusIndicator(109-116)IClientEditor(74-96)ClientStatus(106-106)
src/thunderbird/background_util/compose_tab.ts (3)
src/ghosttext-runner/message.ts (1)
MessagesFromBackground(3-6)src/ghosttext-adaptor/api.ts (1)
IGhostServerPort(91-91)src/thunderbird/util/promisified_port.ts (1)
PromisifiedPort(12-67)
🔇 Additional comments (13)
src/app-options/api.ts (1)
7-7: LGTM! Documentation fix aligns JSDoc with actual parameter name.The JSDoc now correctly documents the
formparameter instead of the incorrectselectorreference, improving documentation accuracy.src/thunderbird/background_util/compose_tab.ts (3)
27-27: LGTM!All internal references have been consistently updated from
this.idtothis.tabIdthroughout the class methods.Also applies to: 37-37, 50-50, 56-56, 69-69
60-64: Verify the use ofimport.meta.urlas the connection name.The openPort method now passes
{ name: import.meta.url }when establishing the connection. This will use the current module's URL as the connection identifier. Confirm this is the intended behavior for identifying/debugging port connections.
9-9: No remaining.tabId()calls found. All usages have been updated to use the publictabIdfield.src/test/startup.test.ts (1)
150-156: LGTM!The helper function correctly extracts the class body after the constructor using optional chaining for safe match handling. While stringifying constructors is fragile and depends on JS engine implementation, this is acceptable for test code.
tsconfig.json (1)
31-50: LGTM! Well-coordinated TypeDoc configuration changes.The configuration changes effectively replace the
typedoc-plugin-mermaidplugin with a custom JavaScript solution (doc/res/typedoc.cjs) while addingignoredHighlightLanguages: ["mermaid"]to prevent syntax highlighting conflicts. The addition oftypedoc-plugin-mdn-linksprovides MDN reference links, and the newBUILDING.mddocument enhances project documentation.ext/NOTICE.md (1)
11-40: Good practice: Using release tag URLs for attribution.The update to use specific release tag URLs (e.g.,
/releases/tag/vX.Y.Z) instead of generic repository links provides better version traceability and makes it easier to verify the exact code being attributed.README.md (1)
1-271: LGTM! Comprehensive documentation improvements.The README updates enhance clarity and completeness:
- Consistent ESR 140 version references
- Reorganized badges for better visibility
- Clearer build instructions
- Added vimghost example with visual demonstration
- Expanded help wanted section with more specific tasks
- Updated roadmap with current version status
All changes improve the project's documentation quality and user experience.
src/ghosttext-runner/ghost_text_runner.ts (4)
15-17: LGTM!The
lastStatusgetter correctly handles both the initial state (command undefined) and completed state, returning appropriateClientStatusvalues.
24-44: LGTM!The refactored
run()method clearly separates the handshake and session phases, with appropriate early exit when session establishment fails. The use of??=on line 30 is clean and modern.
46-64: LGTM!The handshake command handler correctly uses injected dependencies and returns appropriate results. The exhaustiveness comment ensures TypeScript catches any missing command types.
66-86: LGTM!The session command handler correctly uses injected dependencies and delegates appropriately to helper functions. The exhaustiveness comment ensures TypeScript catches any missing command types.
src/thunderbird/messenger/alarms.ts (1)
39-46: LGTM!The export of the
Alarmtype and its definition are correct. TheperiodInMinutesfield is appropriately marked as optional, matching the JSDoc on line 44.
* Add an example tasks.json for vscode * Add BUILDING.md as a shorter version of CONTRIBUTING.md * Fix wording
* Render mermaid diagrams in docs
snipsnipsnip
force-pushed
the
fix-typedoc
branch
from
e572c89 to
384fc7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
doc/design.md (1)
175-175: Fix broken BUILDING.md link casing.The link points to
./building.md(lowercase) but the repository file is namedBUILDING.md(uppercase). This will fail on case-sensitive filesystems.Apply this diff to fix the link:
- * See [building.md](./building.md) for details on build script internals. + * See [BUILDING.md](./BUILDING.md) for details on build script internals.src/root/background_helper.ts (1)
23-30: Add error handling to reset initialization state on failure.If
prepareBackgroundRouter()rejects,routerPromiseremains set to the rejected promise. Subsequent calls will reuse this rejected promise rather than retrying initialization, causing all future router requests to fail immediately.Apply this diff to add error recovery:
- routerPromise ??= import("./startup/startup_background").then((s) => s.prepareBackgroundRouter()) + routerPromise ??= import("./startup/startup_background") + .then((s) => s.prepareBackgroundRouter()) + .catch((error) => { + console.error({ error }) + routerPromise = undefined + routerCache = undefined + throw error + }) return routerPromise.then((router) => { routerCache = router routerPromise = undefined return runHandler(handler, router) })
🧹 Nitpick comments (2)
tsconfig.json (1)
35-36: Use array form forcustomJs.TypeDoc’s option expects an array; providing it as a string works inconsistently across versions. Wrapping it in an array avoids config parsing warnings.
- "customJs": "doc/res/typedoc.cjs", + "customJs": [ + "doc/res/typedoc.cjs" + ],src/root/util/wireless.ts (1)
33-36: Optional: Add JSDoc for better documentation.The type definition is clear and well-structured. Consider adding JSDoc comments to explain the injector's purpose and parameters for improved developer experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.github/workflows/doc.yml(1 hunks).vscode/tasks.example.json(1 hunks).web-ext-config.example.mjs(1 hunks)BUILDING.md(1 hunks)CONTRIBUTING.md(1 hunks)README.md(7 hunks)doc/design.md(5 hunks)doc/faq.md(3 hunks)doc/res/typedoc.cjs(1 hunks)ext/NOTICE.md(1 hunks)package.json(2 hunks)src/app-background/background_event_router.ts(1 hunks)src/app-background/compose_action_notifier.ts(2 hunks)src/app-options/api.ts(1 hunks)src/ghosttext-adaptor/email_editor_factory.ts(1 hunks)src/ghosttext-runner/ghost_text_runner.ts(2 hunks)src/ghosttext-runner/ghost_text_starter.ts(1 hunks)src/ghosttext-session/ghost_text_client.ts(1 hunks)src/root/background.ts(1 hunks)src/root/background_helper.ts(1 hunks)src/root/startup/startup_background.ts(1 hunks)src/root/util/wireless.ts(1 hunks)src/thunderbird/background_util/compose_tab.ts(4 hunks)src/thunderbird/messenger/alarms.ts(2 hunks)src/thunderbird/options_util/options_store.ts(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/thunderbird/options_util/options_store.ts
✅ Files skipped from review due to trivial changes (1)
- src/app-options/api.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- package.json
- .web-ext-config.example.mjs
- BUILDING.md
- README.md
- doc/res/typedoc.cjs
- src/ghosttext-adaptor/email_editor_factory.ts
- .vscode/tasks.example.json
- src/root/background.ts
- src/thunderbird/messenger/alarms.ts
- ext/NOTICE.md
- src/root/startup/startup_background.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/root/util/wireless.ts (2)
src/root/util/wire.ts (4)
wire(157-160)wire(233-247)IClassInfo(25-58)KeyOf(20-20)src/root/util/wired.ts (1)
wired(16-21)
src/app-background/compose_action_notifier.ts (3)
src/ghosttext-adaptor/api.ts (1)
IGhostServerPort(91-91)src/ghosttext-runner/ghost_text_starter.ts (1)
GhostTextStarter(5-32)src/ghosttext-adaptor/email_editor_factory.ts (1)
EmailEditorFactory(5-14)
src/ghosttext-runner/ghost_text_starter.ts (3)
src/ghosttext-runner/api.ts (5)
IGhostTextConnector(20-27)IClientOptionsLoader(39-44)IHeart(11-17)IStatusIndicator(109-116)IClientEditor(74-96)src/ghosttext-session/ghost_text_client.ts (1)
GhostTextClient(30-70)src/ghosttext-runner/ghost_text_runner.ts (1)
GhostTextRunner(4-86)
src/thunderbird/background_util/compose_tab.ts (3)
src/ghosttext-runner/message.ts (1)
MessagesFromBackground(3-6)src/ghosttext-adaptor/api.ts (1)
IGhostServerPort(91-91)src/thunderbird/util/promisified_port.ts (1)
PromisifiedPort(12-67)
src/root/background_helper.ts (1)
src/app-background/background_event_router.ts (1)
BackgroundEventRouter(5-52)
src/ghosttext-runner/ghost_text_runner.ts (2)
src/ghosttext-session/ghost_text_client.ts (4)
Command(12-17)SessionStatus(10-10)GhostTextSession(28-28)CommandResult(19-26)src/ghosttext-runner/api.ts (6)
ISession(47-69)IGhostTextConnector(20-27)ClientOptions(30-34)IStatusIndicator(109-116)IClientEditor(74-96)ClientStatus(106-106)
🔇 Additional comments (19)
src/thunderbird/background_util/compose_tab.ts (2)
9-9: LGTM! Consistent API clarification.The refactoring consistently updates the constructor parameter from
private readonly idtoreadonly tabId, making the field public and accessible. All internal references have been correctly updated throughout the class methods.Also applies to: 27-27, 37-37, 50-50, 56-56, 69-69
59-65: Port connection name is acceptable.Using
import.meta.urlas the port name provides a unique identifier for the connection, which can be helpful for debugging and distinguishing between different port connections..github/workflows/doc.yml (1)
54-54: Upgrade to v4 looks good.Version bump keeps the workflow on the supported release line with no other adjustments needed. ✅
src/root/util/wireless.ts (1)
45-52: LGTM! Well-designed injector pattern.The refactoring is excellent:
- Using
constinstead ofletfor immutability- The injector wrapper provides a clean API with auto-resolution of dependencies
- Returning the injector makes the API cleaner and more flexible
The type cast on line 50 is necessary for the registry pattern. The comment acknowledges this is intentional.
src/app-background/background_event_router.ts (1)
23-23: LGTM! Documentation clarity improved.The JSDoc comment accurately describes the method's purpose and improves code readability.
doc/faq.md (3)
17-17: LGTM! Clarified ESR support.The addition of "the latest" makes the support commitment clearer for users.
129-129: LGTM! Fixed anchor reference.The anchor now correctly points to
#callgraphs(plural), matching the heading in design.md.
152-152: LGTM! Updated file path.The reference now correctly points to the reorganized startup file location under the
startup/subdirectory.doc/design.md (3)
42-42: LGTM! Diagram syntax updates.The mermaid diagram arrow syntax changes (from
<-->to<-.->and<-....->) appropriately reflect the architectural changes in the codebase, particularly the lazy initialization patterns introduced in this PR.Also applies to: 54-54, 57-57
161-165: LGTM! Improved formatting.The nested bullet points improve readability of the MV3 suspension and connection closure notes.
319-319: LGTM! Clarified auto-instantiation scope.Narrowing the scope from
root/*.tstoroot/(directory-only) makes the guideline more precise and aligns with the architectural intent.src/ghosttext-session/ghost_text_client.ts (1)
28-28: LGTM! Type alias improves clarity.Extracting the
GhostTextSessiontype alias makes the generator signature more readable and the type more easily reusable across the codebase.Also applies to: 33-33
src/ghosttext-runner/ghost_text_starter.ts (1)
15-31: LGTM! Proper resource cleanup.The nested try-finally blocks correctly ensure cleanup in all scenarios:
- The outer finally (line 27) guarantees
stopHeartbeat()is called even if option loading fails- The inner finally (line 24) ensures
runner.close()is called only when a runner was successfully createdThe heartbeat lifecycle is properly managed regardless of where failures occur.
src/app-background/compose_action_notifier.ts (2)
1-2: LGTM! Constructor refactored for new dependencies.The updated imports and constructor parameters properly introduce the factory and starter patterns, improving separation of concerns.
Also applies to: 9-10
25-26: LGTM! Factory and starter pattern applied.The code correctly uses
emailEditorFactory.create()to construct the editor andghostTextStarter.run()to orchestrate the session. Passingeditoras both status indicator and editor parameters is valid whenEmailEditorimplements both interfaces.src/ghosttext-runner/ghost_text_runner.ts (4)
5-6: LGTM! Enhanced lifecycle management.The new
lastStatusgetter andclose()method provide explicit lifecycle control, allowing callers to inspect final status and ensure proper cleanup.Also applies to: 15-22
8-13: LGTM! Simplified constructor.The refactored constructor accepts dependencies directly rather than orchestrating initialization, following the single responsibility principle.
24-44: LGTM! Phased execution pattern.The
run()method cleanly separates handshake (lines 27-33) and session (lines 39-43) phases, with proper guard against missing session (lines 35-37). This makes the state machine explicit and easier to test.
46-64: LGTM! Command handlers properly scoped.
runHandshakeCommandandrunSessionCommandcorrectly handle their respective command types with exhaustiveness checks, ensuring compile-time verification that all command types are handled.Also applies to: 66-86
Docstrings generation was requested by @snipsnipsnip. * #32 (comment) The following files were modified: * `src/ghosttext-runner/ghost_text_runner.ts` * `src/root/background_helper.ts` * `src/root/util/wireless.ts`
|
Note Generated docstrings for this pull request at #33 |
github-project-automation
bot
moved this from 📥️Inbox
to 🪺Experimental
in Ghostbird Issues
Docstrings generation was requested by @snipsnipsnip. * #32 (comment) The following files were modified: * `src/ghosttext-runner/ghost_text_runner.ts` * `src/root/background_helper.ts` * `src/root/util/wireless.ts`
Docstrings generation was requested by @snipsnipsnip. * #32 (comment) The following files were modified: * `src/ghosttext-runner/ghost_text_runner.ts` * `src/root/background_helper.ts` * `src/root/util/wireless.ts`
Docstrings generation was requested by @snipsnipsnip. * #32 (comment) The following files were modified: * `src/ghosttext-runner/ghost_text_runner.ts` * `src/root/background_helper.ts` * `src/root/util/wireless.ts`
Docstrings generation was requested by @snipsnipsnip. * #32 (comment) The following files were modified: * `src/ghosttext-runner/ghost_text_runner.ts` * `src/root/background_helper.ts` * `src/root/util/wireless.ts`
Docstrings generation was requested by @snipsnipsnip. * #32 (comment) The following files were modified: * `src/ghosttext-runner/ghost_text_runner.ts` * `src/root/background_helper.ts` * `src/root/util/wireless.ts` <details> <summary>These files were ignored</summary> * `src/test/startup.test.ts` </details> <details> <summary>These file types are not supported</summary> * `.github/workflows/doc.yml` * `.vscode/tasks.example.json` * `BUILDING.md` * `CONTRIBUTING.md` * `README.md` * `biome.jsonc` * `doc/design.md` * `doc/faq.md` * `ext/NOTICE.md` * `package.json` * `tsconfig.json` </details> <details> <summary>ℹ️ Note</summary><blockquote> CodeRabbit cannot perform edits on its own pull requests yet. </blockquote></details>
1 participant
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
Developer Experience
Tests