Open
Conversation
- Bump version from 1.1.0 to 2.0.0 in package.json - Upgrade ember-cli-babel to ^8.2.0 and ember-cli-htmlbars to ^6.3.0 - Update devDependencies including @ember/test-helpers, @embroider/test-setup, and others - Change node engine requirement to >= 20 - Remove EXTEND_PROTOTYPES configuration from environment.js - Clean up optional-features.json by removing unused features
…ncing type safety and structure
WalkthroughThe PR modernizes the addon by migrating to TypeScript, updating the ESLint configuration to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
tsconfig.json (2)
12-16: Missing"*": ["types/*"]path alias — ambient declarations intypes/may not resolve for module imports.The standard ember-cli-typescript blueprint for v1 addons includes
"*": ["types/*"]in thepathsconfiguration, enabling TypeScript to find module augmentations and ambient declarations placed intypes/.While the
types/**/*glob inincludeensures the files are compiled, the missing"*": ["types/*"]path entry means that module-specifier lookups (e.g.,import 'some-ambient-module'declared intypes/) won't resolve through this fallback. Iftypes/tawk-api.d.tsortypes/global.d.tscontaindeclare moduleblocks, they may not be found during module resolution.🔧 Suggested fix
"paths": { "@tawk.to/tawk-messenger-ember/*": ["addon/*"], "dummy/*": ["tests/dummy/app/*"], - "dummy/config/*": ["tests/dummy/config/*"] + "dummy/config/*": ["tests/dummy/config/*"], + "*": ["types/*"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` around lines 12 - 16, Add the missing fallback path mapping to the tsconfig "paths" object so ambient module declarations in types/ resolve for imports: add the entry "*" -> ["types/*"] alongside the existing mappings in the "paths" section (this ensures declare module blocks in types/tawk-api.d.ts or types/global.d.ts are discovered during module resolution).
4-7: Overridingstrict,verbatimModuleSyntax, andnoPropertyAccessFromIndexSignaturetofalseweakens the type safety baseline.The
@tsconfig/emberbase explicitly comments onstrict: trueandnoUncheckedIndexedAccess: true: "Strictness settings — you should not change these: Ember code is not guaranteed to type check with these set to looser values."Disabling all of these together during a migration is understandable as a pragmatic step, but carries risk: type errors that the base config was designed to surface will be silently ignored. Consider:
- Enabling them incrementally per file via
//@ts-check`` or targeted suppression rather than a blanket project-wide disable.- Tracking re-enabling as explicit follow-up tickets to avoid permanent technical debt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` around lines 4 - 7, The project tsconfig overrides disable important strict flags ("strict", "noUncheckedIndexedAccess", "noPropertyAccessFromIndexSignature", "verbatimModuleSyntax"); revert these keys to the base behavior (remove the explicit false overrides or set them to true) so the `@tsconfig/ember` strict baseline is enforced, and if you need a pragmatic migration path enable strictness incrementally by using per-file opt-outs (e.g., add // `@ts-nocheck` or targeted // `@ts-ignore` / // `@ts-expect-error` in problem files) or create a short-term task to re-enable full strictness—update tsconfig.json to stop weakening those flags and document any per-file suppressions.package.json (2)
84-84: TypeScript ^5.6.0 is fine but consider pinning to a narrower range.TypeScript minor versions can introduce stricter checks. A range like
~5.6.0(instead of^5.6.0) would prevent unexpected breakage from a future 5.7+ upgrade that might flag new errors. This is especially relevant sincetsc --noEmitis now part of CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 84, Update the TypeScript dependency entry in package.json to pin to a narrower minor range by replacing the caret specifier on the "typescript" dependency ("typescript": "^5.6.0") with a tilde specifier (e.g., "typescript": "~5.6.0") so CI's tsc --noEmit won't unexpectedly break on future 5.7+ minor releases.
56-57: Upgrade@typescript-eslintto v8 for the latest TypeScript linting rules.
@typescript-eslintv7 is in maintenance mode and v7.18.0 is the last v7 release. v8 is stable, provides new rules and improved TypeScript support, and requires ESLint >= 8.57.0—which matches your current version. This upgrade is straightforward and recommended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 56 - 57, The package.json currently pins "@typescript-eslint/eslint-plugin": "^7.18.0" and "@typescript-eslint/parser": "^7.18.0"; update both entries to v8 (e.g., "^8.0.0" or the latest 8.x) so the project uses `@typescript-eslint` v8 rules, verify ESLint is >= the required version, and run install and lint to catch any breaking rule changes; ensure you change both "@typescript-eslint/eslint-plugin" and "@typescript-eslint/parser" consistently.addon/utils/widget.ts (2)
1-7: Consider makingpropertyIdandwidgetIdrequired in the interface.Both fields default to
''in the destructuring, butloadScriptconstructs a URL from them (https://embed.${basePath}/${propertyId}/${widgetId}). Empty strings produce a malformed URL. If they're truly required for the widget to work, the types should reflect that — making it easier to catch misuse at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@addon/utils/widget.ts` around lines 1 - 7, The LoadScriptOptions interface currently marks propertyId and widgetId as optional but loadScript builds a URL using them (via the loadScript destructuring that defaults them to ''), which can create malformed URLs; make propertyId and widgetId required in the LoadScriptOptions interface (remove the optional modifiers) and remove the empty-string defaults in the loadScript function so callers must supply valid strings, then update any call sites to pass those values or adjust call-site logic to handle the new required parameters; reference LoadScriptOptions, propertyId, widgetId, and the loadScript function when making these changes.
37-37:script.charsetis deprecated.The
charsetproperty onHTMLScriptElementis deprecated per the HTML spec. Since UTF-8 is the default encoding for scripts, this line can be removed. If you want to keep it for legacy reasons, usescript.setAttribute('charset', 'UTF-8')instead of the deprecated DOM property.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@addon/utils/widget.ts` at line 37, The code sets the deprecated DOM property script.charset; update the widget code to stop using the deprecated property: remove the line that assigns script.charset = 'UTF-8' (since UTF-8 is the default) or, if preserving an explicit attribute for legacy support, replace it by calling script.setAttribute('charset', 'UTF-8') on the same script element variable (named script) in addon/utils/widget.ts.tests/unit/utils/widget-test.js (1)
1-3: Empty test module — no coverage for the refactoredloadScript.The
loadScriptfunction had a signature change (now accepts an options object). Tests verifying the new API contract — especially edge cases like missingpropertyId/widgetIdand theembedIdDOM element creation — would be valuable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/utils/widget-test.js` around lines 1 - 3, Add unit tests for the refactored loadScript to cover its new options-object signature and edge cases: call loadScript({ propertyId, widgetId, ... }) and assert successful script injection; assert it throws or rejects when propertyId or widgetId are missing; verify that when embedId is provided the DOM element with that id is created and has expected attributes; add tests in the existing widget-test module for both success and failure scenarios and use the loadScript function name to locate the implementation under test.tests/unit/utils/helper-test.js (1)
1-3: Empty test module provides no coverage forisValidString.The
addon/utils/helper.tsfile exportsisValidString, but this test module has no assertions. Consider adding basic test cases covering truthy strings, empty strings,null,undefined, and non-string types to validate the type guard behavior.Would you like me to generate test cases for
isValidString, or open an issue to track this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/utils/helper-test.js` around lines 1 - 3, The test module is empty and must include assertions for the exported type guard isValidString; update the helper-test.js module to import isValidString and add QUnit tests that assert true for a normal non-empty string (e.g., "hello") and assert false for empty string (""), null, undefined, and several non-string types (number like 123, object like {}, array like []), using clear test names (e.g., "isValidString returns true for non-empty strings" and "isValidString returns false for empty/null/other types") so the utility's behavior is covered.addon/components/tawk-messenger.ts (1)
9-40: Consider using specific types instead ofunknownfor callback parameters that have known shapes.Several callback Args use
unknownwhere the underlying Tawk API types are more specific — e.g.,tawkOnStatusChangecould be(status: TawkStatus) => void(Line 18), andtawkOnUnreadCountChangedcould be(count: number) => void(Line 36). Usingunknownis safe but pushes unnecessary casting burden onto consumers. Since you already have the precise types intawk-api.d.ts, leveraging them here would improve the developer experience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@addon/components/tawk-messenger.ts` around lines 9 - 40, Replace the generic unknown callback parameter types in the TawkMessengerSignature Args with the precise types from your tawk-api.d.ts to improve DX: update tawkOnStatusChange to (status: TawkStatus) => void, tawkOnPrechatSubmit and tawkOnOfflineSubmit to their specific PrechatData/OfflineData types, tawkOnAgentJoinChat/tawkOnAgentLeaveChat to AgentData, tawkOnTagsUpdated to TagsUpdate, and tawkOnUnreadCountChanged to (count: number) => void (or UnreadCount type) and import or reference those types at the top of the file so consumers of TawkMessengerSignature get proper typings instead of unknown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.npmignore:
- Around line 34-37: Remove /declarations/ from .npmignore so generated .d.ts
files are published, add a tsconfig.declarations.json that extends tsconfig.json
with noEmit: false (overriding noEmit: true) to emit declaration files into the
declarations directory, and add a prepack script in package.json to run tsc -p
tsconfig.declarations.json before publishing; keep /declarations/ in .gitignore
only so declarations aren't checked into git but are included in npm packages.
In `@addon/components/tawk-messenger.ts`:
- Around line 63-66: The guard in load() uses direct access to window and
document which can throw in SSR/Fastboot; change the checks to use typeof window
=== 'undefined' and typeof document === 'undefined' (or similar typeof checks)
before any direct access, e.g., replace the existing if (!window || !document)
return; with guards that use typeof to safely detect missing globals so the
load() method exits early in SSR environments.
- Around line 46-58: Fix the grammar in the error messages inside the Tawk
messenger component: update the console.error messages in the isValidString
checks for this.args.propertyId and this.args.widgetId so they read "You didn't
specify 'propertyId'" and "You didn't specify 'widgetId'" respectively (locate
the checks using isValidString(this.args.propertyId) and
isValidString(this.args.widgetId) in the tawk-messenger component).
- Around line 105-237: mapListeners() attaches many window event listeners but
never unregisters them; modify mapListeners() to create and store each listener
callback (e.g., this._tawkListeners = [{event: 'tawkLoad', handler}, ...] or a
Map) instead of inline anonymous functions, use those stored handlers when
calling window.addEventListener, and implement willDestroy() to iterate over
this._tawkListeners and call window.removeEventListener(event, handler) for each
entry and then clear the collection; reference mapListeners(), willDestroy(),
and the various args callbacks like
tawkOnLoad/tawkOnStatusChange/tawkOnChatMessageAgent etc. to find where to
attach and later remove the handlers.
In `@addon/services/tawk-messenger.ts`:
- Around line 128-130: The public method name is inconsistent: rename the
existing switchWidget method to tawkSwitchWidget (update the declaration and all
internal references) so it matches other public methods like tawkMaximize and
tawkAddTags; also update any unit tests to call tawkSwitchWidget and ensure the
window.Tawk_API.switchWidget invocation and its parameter types
(TawkSwitchWidgetData, TawkCallback) remain unchanged.
- Around line 11-131: TawkMessengerService methods currently access
window.Tawk_API directly and will throw if it's undefined; add a private helper
(e.g., private getTawkApi(): typeof window.Tawk_API | undefined or private
ensureTawkApi(methodName: string): TawkApi) that checks for window.Tawk_API and
either returns it or early-returns/no-ops (or throws a clear error) and update
all public methods (tawkStart, tawkShutdown, tawkMaximize, tawkMinimize,
tawkToggle, tawkPopup, tawkShowWidget, tawkHideWidget, tawkToggleVisibility,
tawkEndChat, tawkGetWindowType, tawkGetStatus, tawkIsChatMaximized,
tawkIsChatMinimized, tawkIsChatHidden, tawkIsChatOngoing, tawkIsVisitorEngaged,
tawkOnLoaded, tawkOnBeforeLoaded, tawkWidgetPosition, tawkVisitor,
tawkSetAttributes, tawkAddEvent, tawkAddTags, tawkRemoveTags, switchWidget) to
call that helper instead of accessing window.Tawk_API directly so calls are safe
in SSR/test/early-initialization scenarios.
In `@addon/utils/helper.ts`:
- Around line 7-13: The isValidString function contains redundant/ unreachable
checks; simplify it by making the function directly check that value is a string
and non-empty: replace the current falsy/null checks with a single return of
typeof value === 'string' && value.length > 0 so the function only validates
type and non-empty content (update the function isValidString accordingly).
In `@addon/utils/widget.ts`:
- Around line 40-41: Guard against a potential NPE when accessing
document.getElementsByTagName('script')[0] by checking whether the retrieved
firstScript is defined and has a parentNode before calling insertBefore; if it
is undefined or parentNode is null, fall back to a safe container such as
document.head || document.body and use appendChild(script) or insertBefore on
that container. Update the code around the firstScript variable and the
insertBefore call to: retrieve firstScript via
document.getElementsByTagName('script')[0], if (!firstScript ||
!firstScript.parentNode) use const container = document.head || document.body
and call container.appendChild(script), otherwise call
firstScript.parentNode.insertBefore(script, firstScript).
In `@package.json`:
- Around line 44-46: package.json's dependencies are missing `@babel/core` which
ember-cli-babel (symbol: "ember-cli-babel") v8 expects as a direct dependency in
v1 addons; add "@babel/core" to the "dependencies" object in package.json (next
to "ember-cli-babel") with an appropriate semver (e.g., a compatible ^7.x
version) and run install to update lockfile so the addon has `@babel/core`
available at runtime.
In `@tests/unit/services/tawk-messenger-test.js`:
- Around line 7-49: The test is missing assertions for three methods defined on
the tawk-messenger service; add strictEqual assertions similar to the others for
service.tawkStart, service.tawkShutdown, and service.switchWidget to verify each
is a 'function'. Locate the existing test block "Should set the Action services"
(uses const service = this.owner.lookup('service:tawk-messenger')) and append
the three assertions with the same message pattern used for the others (e.g.,
'Start is not a function', 'Shutdown is not a function', 'switchWidget is not a
function').
In `@tsconfig.json`:
- Around line 8-9: The tsconfig currently sets "declaration": true but inherits
"noEmit": true from the `@tsconfig/ember` base so no .d.ts files are emitted; fix
by either (A) explicitly setting "noEmit": false in tsconfig.json alongside
"declaration": true and "declarationDir": "declarations" so tsc will emit
declarations, or (B, preferred for v1 addons) add a new
tsconfig.declarations.json that extends the existing tsconfig and sets "noEmit":
false, "declaration": true and "declarationDir": "declarations", add a prepack
script to run tsc --project tsconfig.declarations.json to emit .d.ts files
before publishing, and ensure /declarations/ is removed from .npmignore so the
emitted declarations are included in the package.
In `@types/global.d.ts`:
- Around line 3-7: Update the global Window declaration so Tawk_API and
Tawk_LoadStart are optional rather than required: change the Window interface
members Tawk_API and Tawk_LoadStart to optional (e.g., Tawk_API? and
Tawk_LoadStart?) and then update call sites (notably the service using
window.Tawk_API and the component method load() in
addon/components/tawk-messenger.ts) to perform proper runtime null/undefined
checks or use safe narrowing before accessing them; this keeps the type contract
accurate and forces consumers to guard access.
---
Nitpick comments:
In `@addon/components/tawk-messenger.ts`:
- Around line 9-40: Replace the generic unknown callback parameter types in the
TawkMessengerSignature Args with the precise types from your tawk-api.d.ts to
improve DX: update tawkOnStatusChange to (status: TawkStatus) => void,
tawkOnPrechatSubmit and tawkOnOfflineSubmit to their specific
PrechatData/OfflineData types, tawkOnAgentJoinChat/tawkOnAgentLeaveChat to
AgentData, tawkOnTagsUpdated to TagsUpdate, and tawkOnUnreadCountChanged to
(count: number) => void (or UnreadCount type) and import or reference those
types at the top of the file so consumers of TawkMessengerSignature get proper
typings instead of unknown.
In `@addon/utils/widget.ts`:
- Around line 1-7: The LoadScriptOptions interface currently marks propertyId
and widgetId as optional but loadScript builds a URL using them (via the
loadScript destructuring that defaults them to ''), which can create malformed
URLs; make propertyId and widgetId required in the LoadScriptOptions interface
(remove the optional modifiers) and remove the empty-string defaults in the
loadScript function so callers must supply valid strings, then update any call
sites to pass those values or adjust call-site logic to handle the new required
parameters; reference LoadScriptOptions, propertyId, widgetId, and the
loadScript function when making these changes.
- Line 37: The code sets the deprecated DOM property script.charset; update the
widget code to stop using the deprecated property: remove the line that assigns
script.charset = 'UTF-8' (since UTF-8 is the default) or, if preserving an
explicit attribute for legacy support, replace it by calling
script.setAttribute('charset', 'UTF-8') on the same script element variable
(named script) in addon/utils/widget.ts.
In `@package.json`:
- Line 84: Update the TypeScript dependency entry in package.json to pin to a
narrower minor range by replacing the caret specifier on the "typescript"
dependency ("typescript": "^5.6.0") with a tilde specifier (e.g., "typescript":
"~5.6.0") so CI's tsc --noEmit won't unexpectedly break on future 5.7+ minor
releases.
- Around line 56-57: The package.json currently pins
"@typescript-eslint/eslint-plugin": "^7.18.0" and "@typescript-eslint/parser":
"^7.18.0"; update both entries to v8 (e.g., "^8.0.0" or the latest 8.x) so the
project uses `@typescript-eslint` v8 rules, verify ESLint is >= the required
version, and run install and lint to catch any breaking rule changes; ensure you
change both "@typescript-eslint/eslint-plugin" and "@typescript-eslint/parser"
consistently.
In `@tests/unit/utils/helper-test.js`:
- Around line 1-3: The test module is empty and must include assertions for the
exported type guard isValidString; update the helper-test.js module to import
isValidString and add QUnit tests that assert true for a normal non-empty string
(e.g., "hello") and assert false for empty string (""), null, undefined, and
several non-string types (number like 123, object like {}, array like []), using
clear test names (e.g., "isValidString returns true for non-empty strings" and
"isValidString returns false for empty/null/other types") so the utility's
behavior is covered.
In `@tests/unit/utils/widget-test.js`:
- Around line 1-3: Add unit tests for the refactored loadScript to cover its new
options-object signature and edge cases: call loadScript({ propertyId, widgetId,
... }) and assert successful script injection; assert it throws or rejects when
propertyId or widgetId are missing; verify that when embedId is provided the DOM
element with that id is created and has expected attributes; add tests in the
existing widget-test module for both success and failure scenarios and use the
loadScript function name to locate the implementation under test.
In `@tsconfig.json`:
- Around line 12-16: Add the missing fallback path mapping to the tsconfig
"paths" object so ambient module declarations in types/ resolve for imports: add
the entry "*" -> ["types/*"] alongside the existing mappings in the "paths"
section (this ensures declare module blocks in types/tawk-api.d.ts or
types/global.d.ts are discovered during module resolution).
- Around line 4-7: The project tsconfig overrides disable important strict flags
("strict", "noUncheckedIndexedAccess", "noPropertyAccessFromIndexSignature",
"verbatimModuleSyntax"); revert these keys to the base behavior (remove the
explicit false overrides or set them to true) so the `@tsconfig/ember` strict
baseline is enforced, and if you need a pragmatic migration path enable
strictness incrementally by using per-file opt-outs (e.g., add // `@ts-nocheck` or
targeted // `@ts-ignore` / // `@ts-expect-error` in problem files) or create a
short-term task to re-enable full strictness—update tsconfig.json to stop
weakening those flags and document any per-file suppressions.
Comment on lines
+34
to
+37
| # typescript | ||
| /tsconfig.json | ||
| *.tsbuildinfo | ||
| /declarations/ |
There was a problem hiding this comment.
/declarations/ is excluded from the npm package — type definitions won't be distributed to consumers.
tsconfig.json sets declarationDir: "declarations" to generate .d.ts files for package consumers, but this directory is also excluded from the npm publish via .npmignore. This means TypeScript users installing the package won't receive any type declarations.
For a v1 Ember addon, the standard approach is:
- Keep
noEmit: truein the maintsconfig.json(for IDE/type-checking). - Add a separate
tsconfig.declarations.jsonthat extends the main config, overridesnoEmit: false, and runs as aprepackscript. - Only ignore
/declarations/from.gitignore, not.npmignore, so.d.tsfiles are included in the published package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.npmignore around lines 34 - 37, Remove /declarations/ from .npmignore so
generated .d.ts files are published, add a tsconfig.declarations.json that
extends tsconfig.json with noEmit: false (overriding noEmit: true) to emit
declaration files into the declarations directory, and add a prepack script in
package.json to run tsc -p tsconfig.declarations.json before publishing; keep
/declarations/ in .gitignore only so declarations aren't checked into git but
are included in npm packages.
Comment on lines
+46
to
+58
| if (!isValidString(this.args.propertyId)) { | ||
| console.error( | ||
| '[Tawk-messenger-ember warn]: You didn\'t specified \'propertyId\'', | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (!isValidString(this.args.widgetId)) { | ||
| console.error( | ||
| '[Tawk-messenger-ember warn]: You didn\'t specified \'widgetId\'', | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Grammar: "You didn't specified" → "You didn't specify".
Proposed fix
if (!isValidString(this.args.propertyId)) {
console.error(
- '[Tawk-messenger-ember warn]: You didn\'t specified \'propertyId\'',
+ '[Tawk-messenger-ember warn]: You didn\'t specify \'propertyId\'',
);
return;
}
if (!isValidString(this.args.widgetId)) {
console.error(
- '[Tawk-messenger-ember warn]: You didn\'t specified \'widgetId\'',
+ '[Tawk-messenger-ember warn]: You didn\'t specify \'widgetId\'',
);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!isValidString(this.args.propertyId)) { | |
| console.error( | |
| '[Tawk-messenger-ember warn]: You didn\'t specified \'propertyId\'', | |
| ); | |
| return; | |
| } | |
| if (!isValidString(this.args.widgetId)) { | |
| console.error( | |
| '[Tawk-messenger-ember warn]: You didn\'t specified \'widgetId\'', | |
| ); | |
| return; | |
| } | |
| if (!isValidString(this.args.propertyId)) { | |
| console.error( | |
| '[Tawk-messenger-ember warn]: You didn\'t specify \'propertyId\'', | |
| ); | |
| return; | |
| } | |
| if (!isValidString(this.args.widgetId)) { | |
| console.error( | |
| '[Tawk-messenger-ember warn]: You didn\'t specify \'widgetId\'', | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@addon/components/tawk-messenger.ts` around lines 46 - 58, Fix the grammar in
the error messages inside the Tawk messenger component: update the console.error
messages in the isValidString checks for this.args.propertyId and
this.args.widgetId so they read "You didn't specify 'propertyId'" and "You
didn't specify 'widgetId'" respectively (locate the checks using
isValidString(this.args.propertyId) and isValidString(this.args.widgetId) in the
tawk-messenger component).
Comment on lines
+63
to
+66
| load(): void { | ||
| if (!window || !document) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
!window guard doesn't protect against SSR/Fastboot.
In SSR environments (e.g., Ember Fastboot), window is not undefined — it's simply not in scope, so accessing it throws a ReferenceError before the check runs. Use typeof window === 'undefined' instead.
Proposed fix
load(): void {
- if (!window || !document) {
+ if (typeof window === 'undefined' || typeof document === 'undefined') {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| load(): void { | |
| if (!window || !document) { | |
| return; | |
| } | |
| load(): void { | |
| if (typeof window === 'undefined' || typeof document === 'undefined') { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@addon/components/tawk-messenger.ts` around lines 63 - 66, The guard in load()
uses direct access to window and document which can throw in SSR/Fastboot;
change the checks to use typeof window === 'undefined' and typeof document ===
'undefined' (or similar typeof checks) before any direct access, e.g., replace
the existing if (!window || !document) return; with guards that use typeof to
safely detect missing globals so the load() method exits early in SSR
environments.
Comment on lines
+105
to
+237
| mapListeners(): void { | ||
| window.addEventListener('tawkLoad', () => { | ||
| if (this.args.tawkOnLoad) { | ||
| this.args.tawkOnLoad(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkStatusChange', ((status: CustomEvent) => { | ||
| if (this.args.tawkOnStatusChange) { | ||
| this.args.tawkOnStatusChange(status.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkBeforeLoad', () => { | ||
| if (this.args.tawkOnBeforeLoad) { | ||
| this.args.tawkOnBeforeLoad(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkChatMaximized', () => { | ||
| if (this.args.tawkOnChatMaximized) { | ||
| this.args.tawkOnChatMaximized(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkChatMinimized', () => { | ||
| if (this.args.tawkOnChatMinimized) { | ||
| this.args.tawkOnChatMinimized(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkChatHidden', () => { | ||
| if (this.args.tawkOnChatHidden) { | ||
| this.args.tawkOnChatHidden(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkChatStarted', () => { | ||
| if (this.args.tawkOnChatStarted) { | ||
| this.args.tawkOnChatStarted(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkChatEnded', () => { | ||
| if (this.args.tawkOnChatEnded) { | ||
| this.args.tawkOnChatEnded(); | ||
| } | ||
| }); | ||
|
|
||
| window.addEventListener('tawkPrechatSubmit', ((data: CustomEvent) => { | ||
| if (this.args.tawkOnPrechatSubmit) { | ||
| this.args.tawkOnPrechatSubmit(data.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkOfflineSubmit', ((data: CustomEvent) => { | ||
| if (this.args.tawkOnOfflineSubmit) { | ||
| this.args.tawkOnOfflineSubmit(data.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkChatMessageVisitor', (( | ||
| message: CustomEvent, | ||
| ) => { | ||
| if (this.args.tawkOnChatMessageVisitor) { | ||
| this.args.tawkOnChatMessageVisitor(message.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkChatMessageAgent', (( | ||
| message: CustomEvent, | ||
| ) => { | ||
| if (this.args.tawkOnChatMessageAgent) { | ||
| this.args.tawkOnChatMessageAgent(message.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkChatMessageSystem', (( | ||
| message: CustomEvent, | ||
| ) => { | ||
| if (this.args.tawkOnChatMessageSystem) { | ||
| this.args.tawkOnChatMessageSystem(message.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkAgentJoinChat', ((data: CustomEvent) => { | ||
| if (this.args.tawkOnAgentJoinChat) { | ||
| this.args.tawkOnAgentJoinChat(data.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkAgentLeaveChat', ((data: CustomEvent) => { | ||
| if (this.args.tawkOnAgentLeaveChat) { | ||
| this.args.tawkOnAgentLeaveChat(data.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkChatSatisfaction', (( | ||
| satisfaction: CustomEvent, | ||
| ) => { | ||
| if (this.args.tawkOnChatSatisfaction) { | ||
| this.args.tawkOnChatSatisfaction(satisfaction.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkVisitorNameChanged', (( | ||
| visitorName: CustomEvent, | ||
| ) => { | ||
| if (this.args.tawkOnVisitorNameChanged) { | ||
| this.args.tawkOnVisitorNameChanged(visitorName.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkFileUpload', ((link: CustomEvent) => { | ||
| if (this.args.tawkOnFileUpload) { | ||
| this.args.tawkOnFileUpload(link.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkTagsUpdated', ((data: CustomEvent) => { | ||
| if (this.args.tawkOnTagsUpdated) { | ||
| this.args.tawkOnTagsUpdated(data.detail); | ||
| } | ||
| }) as EventListener); | ||
|
|
||
| window.addEventListener('tawkUnreadCountChanged', (( | ||
| data: CustomEvent, | ||
| ) => { | ||
| if (this.args.tawkOnUnreadCountChanged) { | ||
| this.args.tawkOnUnreadCountChanged(data.detail); | ||
| } | ||
| }) as EventListener); | ||
| } |
There was a problem hiding this comment.
Event listener leak: listeners are never removed on component teardown.
mapListeners() adds 20+ listeners to window but they are never cleaned up when the component is destroyed. If the component is re-rendered or the route changes, stale listeners accumulate, causing memory leaks and duplicate callback invocations.
Store references to the listeners and remove them in willDestroy():
Proposed approach
export default class TawkMessengerComponent extends Component<TawkMessengerSignature> {
+ private listeners: Array<[string, EventListener]> = [];
+
+ private addListener(event: string, handler: EventListener): void {
+ window.addEventListener(event, handler);
+ this.listeners.push([event, handler]);
+ }
+
+ willDestroy(): void {
+ super.willDestroy();
+ for (const [event, handler] of this.listeners) {
+ window.removeEventListener(event, handler);
+ }
+ this.listeners = [];
+ }
+
mapListeners(): void {
- window.addEventListener('tawkLoad', () => {
+ this.addListener('tawkLoad', (() => {
if (this.args.tawkOnLoad) {
this.args.tawkOnLoad();
}
- });
+ }) as EventListener);
// ... same pattern for all other listeners🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@addon/components/tawk-messenger.ts` around lines 105 - 237, mapListeners()
attaches many window event listeners but never unregisters them; modify
mapListeners() to create and store each listener callback (e.g.,
this._tawkListeners = [{event: 'tawkLoad', handler}, ...] or a Map) instead of
inline anonymous functions, use those stored handlers when calling
window.addEventListener, and implement willDestroy() to iterate over
this._tawkListeners and call window.removeEventListener(event, handler) for each
entry and then clear the collection; reference mapListeners(), willDestroy(),
and the various args callbacks like
tawkOnLoad/tawkOnStatusChange/tawkOnChatMessageAgent etc. to find where to
attach and later remove the handlers.
Comment on lines
+11
to
131
| export default class TawkMessengerService extends Service { | ||
| /** | ||
| * API for calling an action on the widget | ||
| */ | ||
| tawkStart() { | ||
| tawkStart(): void { | ||
| return window.Tawk_API.start(); | ||
| } | ||
|
|
||
| tawkShutdown() { | ||
| tawkShutdown(): void { | ||
| return window.Tawk_API.shutdown(); | ||
| } | ||
|
|
||
| tawkMaximize() { | ||
| tawkMaximize(): void { | ||
| return window.Tawk_API.maximize(); | ||
| } | ||
|
|
||
| tawkMinimize() { | ||
| tawkMinimize(): void { | ||
| return window.Tawk_API.minimize(); | ||
| } | ||
|
|
||
| tawkToggle() { | ||
| tawkToggle(): void { | ||
| return window.Tawk_API.toggle(); | ||
| } | ||
|
|
||
| tawkPopup() { | ||
| tawkPopup(): void { | ||
| return window.Tawk_API.popup(); | ||
| } | ||
|
|
||
| tawkShowWidget() { | ||
| tawkShowWidget(): void { | ||
| return window.Tawk_API.showWidget(); | ||
| } | ||
|
|
||
| tawkHideWidget() { | ||
| tawkHideWidget(): void { | ||
| return window.Tawk_API.hideWidget(); | ||
| } | ||
|
|
||
| tawkToggleVisibility() { | ||
| tawkToggleVisibility(): void { | ||
| return window.Tawk_API.toggleVisibility(); | ||
| } | ||
|
|
||
| tawkEndChat() { | ||
| tawkEndChat(): void { | ||
| return window.Tawk_API.endChat(); | ||
| } | ||
|
|
||
| /** | ||
| * API for returning a data | ||
| */ | ||
| tawkGetWindowType() { | ||
| tawkGetWindowType(): TawkWindowType { | ||
| return window.Tawk_API.getWindowType(); | ||
| } | ||
|
|
||
| tawkGetStatus() { | ||
| tawkGetStatus(): TawkStatus { | ||
| return window.Tawk_API.getStatus(); | ||
| } | ||
|
|
||
| tawkIsChatMaximized() { | ||
| tawkIsChatMaximized(): boolean { | ||
| return window.Tawk_API.isChatMaximized(); | ||
| } | ||
|
|
||
| tawkIsChatMinimized() { | ||
| tawkIsChatMinimized(): boolean { | ||
| return window.Tawk_API.isChatMinimized(); | ||
| } | ||
|
|
||
| tawkIsChatHidden() { | ||
| tawkIsChatHidden(): boolean { | ||
| return window.Tawk_API.isChatHidden(); | ||
| } | ||
|
|
||
| tawkIsChatOngoing() { | ||
| tawkIsChatOngoing(): boolean { | ||
| return window.Tawk_API.isChatOngoing(); | ||
| } | ||
|
|
||
| tawkIsVisitorEngaged() { | ||
| tawkIsVisitorEngaged(): boolean { | ||
| return window.Tawk_API.isVisitorEngaged(); | ||
| } | ||
|
|
||
| tawkOnLoaded() { | ||
| tawkOnLoaded(): boolean | undefined { | ||
| return window.Tawk_API.onLoaded; | ||
| } | ||
|
|
||
| tawkOnBeforeLoaded() { | ||
| tawkOnBeforeLoaded(): boolean | undefined { | ||
| return window.Tawk_API.onBeforeLoaded; | ||
| } | ||
|
|
||
| tawkWidgetPosition() { | ||
| tawkWidgetPosition(): string { | ||
| return window.Tawk_API.widgetPosition(); | ||
| } | ||
|
|
||
| /** | ||
| * API for setting a data on the widget | ||
| */ | ||
| tawkVisitor(data) { | ||
| tawkVisitor(data: TawkVisitor): void { | ||
| window.Tawk_API.visitor = data; | ||
| } | ||
|
|
||
| tawkSetAttributes(attribute, callback) { | ||
| tawkSetAttributes( | ||
| attribute: Record<string, string>, | ||
| callback?: TawkCallback, | ||
| ): void { | ||
| window.Tawk_API.setAttributes(attribute, callback); | ||
| } | ||
|
|
||
| tawkAddEvent(event, metadata, callback) { | ||
| tawkAddEvent( | ||
| event: string, | ||
| metadata?: Record<string, string>, | ||
| callback?: TawkCallback, | ||
| ): void { | ||
| window.Tawk_API.addEvent(event, metadata, callback); | ||
| } | ||
|
|
||
| tawkAddTags(tags, callback) { | ||
| tawkAddTags(tags: string[], callback?: TawkCallback): void { | ||
| window.Tawk_API.addTags(tags, callback); | ||
| } | ||
|
|
||
| tawkRemoveTags(tags, callback) { | ||
| tawkRemoveTags(tags: string[], callback?: TawkCallback): void { | ||
| window.Tawk_API.removeTags(tags, callback); | ||
| } | ||
|
|
||
| switchWidget(data, callback) { | ||
| switchWidget(data: TawkSwitchWidgetData, callback?: TawkCallback): void { | ||
| window.Tawk_API.switchWidget(data, callback); | ||
| } | ||
| } |
There was a problem hiding this comment.
No defensive guard for window.Tawk_API — every method will throw if the widget hasn't loaded.
The previous implementation had a constructor-level validation for window.Tawk_API. With that removed, every method blindly accesses window.Tawk_API.*. If the service is injected and called before the <TawkMessenger> component initializes the global (or in SSR/test contexts), all calls will throw TypeError: Cannot read properties of undefined.
Consider adding a private helper that asserts or early-returns when Tawk_API is not yet available:
Proposed approach
export default class TawkMessengerService extends Service {
+ private get api() {
+ return window.Tawk_API;
+ }
+
+ private assertApi(): asserts this is { api: NonNullable<typeof window.Tawk_API> } {
+ if (!this.api) {
+ throw new Error('[tawk-messenger] Tawk_API is not initialized. Ensure <TawkMessenger> component is rendered first.');
+ }
+ }
+
tawkStart(): void {
- return window.Tawk_API.start();
+ this.assertApi();
+ this.api.start();
}
// ... apply to all methods🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@addon/services/tawk-messenger.ts` around lines 11 - 131, TawkMessengerService
methods currently access window.Tawk_API directly and will throw if it's
undefined; add a private helper (e.g., private getTawkApi(): typeof
window.Tawk_API | undefined or private ensureTawkApi(methodName: string):
TawkApi) that checks for window.Tawk_API and either returns it or
early-returns/no-ops (or throws a clear error) and update all public methods
(tawkStart, tawkShutdown, tawkMaximize, tawkMinimize, tawkToggle, tawkPopup,
tawkShowWidget, tawkHideWidget, tawkToggleVisibility, tawkEndChat,
tawkGetWindowType, tawkGetStatus, tawkIsChatMaximized, tawkIsChatMinimized,
tawkIsChatHidden, tawkIsChatOngoing, tawkIsVisitorEngaged, tawkOnLoaded,
tawkOnBeforeLoaded, tawkWidgetPosition, tawkVisitor, tawkSetAttributes,
tawkAddEvent, tawkAddTags, tawkRemoveTags, switchWidget) to call that helper
instead of accessing window.Tawk_API directly so calls are safe in
SSR/test/early-initialization scenarios.
Comment on lines
40
to
+41
| const firstScript = document.getElementsByTagName('script')[0]; | ||
| firstScript.parentNode.insertBefore(script, firstScript); | ||
| firstScript.parentNode!.insertBefore(script, firstScript); |
There was a problem hiding this comment.
Potential NPE if no <script> tags exist in the document.
getElementsByTagName('script')[0] returns undefined when there are no script elements, and the non-null assertion on parentNode! would then throw a TypeError at runtime. While unlikely in a browser with a fully loaded page, this is fragile — especially in SSR/test environments.
A safer pattern:
Proposed fix
- const firstScript = document.getElementsByTagName('script')[0];
- firstScript.parentNode!.insertBefore(script, firstScript);
+ const firstScript = document.getElementsByTagName('script')[0];
+ if (firstScript?.parentNode) {
+ firstScript.parentNode.insertBefore(script, firstScript);
+ } else {
+ document.head.appendChild(script);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const firstScript = document.getElementsByTagName('script')[0]; | |
| firstScript.parentNode.insertBefore(script, firstScript); | |
| firstScript.parentNode!.insertBefore(script, firstScript); | |
| const firstScript = document.getElementsByTagName('script')[0]; | |
| if (firstScript?.parentNode) { | |
| firstScript.parentNode.insertBefore(script, firstScript); | |
| } else { | |
| document.head.appendChild(script); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@addon/utils/widget.ts` around lines 40 - 41, Guard against a potential NPE
when accessing document.getElementsByTagName('script')[0] by checking whether
the retrieved firstScript is defined and has a parentNode before calling
insertBefore; if it is undefined or parentNode is null, fall back to a safe
container such as document.head || document.body and use appendChild(script) or
insertBefore on that container. Update the code around the firstScript variable
and the insertBefore call to: retrieve firstScript via
document.getElementsByTagName('script')[0], if (!firstScript ||
!firstScript.parentNode) use const container = document.head || document.body
and call container.appendChild(script), otherwise call
firstScript.parentNode.insertBefore(script, firstScript).
Comment on lines
44
to
+46
| "dependencies": { | ||
| "ember-cli-babel": "^7.26.11", | ||
| "ember-cli-htmlbars": "^6.0.1" | ||
| "ember-cli-babel": "^8.2.0", | ||
| "ember-cli-htmlbars": "^6.3.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an index.js at root (v1 addon entry point)
if [ -f "index.js" ]; then
echo "=== index.js found ==="
head -30 index.js
else
echo "No index.js at root"
fiRepository: tawk/tawk-messenger-ember
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check package.json for ember-addon configuration
cat -n package.json | head -60Repository: tawk/tawk-messenger-ember
Length of output: 2418
🏁 Script executed:
#!/bin/bash
# Search for ember-addon.configPath references
rg "ember-addon" --type jsonRepository: tawk/tawk-messenger-ember
Length of output: 352
🌐 Web query:
ember-cli-babel v8 compatibility with v1 addon format
💡 Result:
ember-cli-babel@8 is compatible with the classic “v1 addon” format (i.e. addons using index.js, Broccoli hooks, options.babel, etc.). The project docs explicitly describe “Addon usage (for V1 Addons)” and note that v2 addons should not use ember-cli-babel (they should remove it entirely). [1]
What changed in v8 that matters for v1 addons:
@babel/corebecame a required peer dependency, so v1 addons need to add@babel/coreto their owndependencies(because v1 addons ship their ownember-cli-babel). This was designed so addons can upgrade toember-cli-babel@8without forcing a breaking release just to make@babel/coreexplicit. [2]- The
includePolyfilloption was removed; apps should importcore-js/stabledirectly instead. [2]
Bottom line: v1 addon format works with ember-cli-babel@8; just ensure your v1 addon declares @babel/core (and don’t use includePolyfill). For v2 addon format, don’t depend on ember-cli-babel. [1][2]
References:
[1] ember-cli-babel README (V1 vs V2 addon note) https://github.com/emberjs/ember-cli-babel
[2] ember-cli-babel v8 upgrading guide (peer dep + addon guidance) https://raw.githubusercontent.com/emberjs/ember-cli-babel/master/UPGRADING.md
🏁 Script executed:
#!/bin/bash
# Get lines 44-80 to see full dependencies and start of devDependencies
sed -n '44,80p' package.jsonRepository: tawk/tawk-messenger-ember
Length of output: 1332
Add @babel/core to dependencies for ember-cli-babel@8 compatibility.
ember-cli-babel@8 is compatible with v1 addon format (the current structure), but v8 requires @babel/core as a direct dependency in v1 addons since it became a peer dependency. Add @babel/core to the dependencies section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 44 - 46, package.json's dependencies are missing
`@babel/core` which ember-cli-babel (symbol: "ember-cli-babel") v8 expects as a
direct dependency in v1 addons; add "@babel/core" to the "dependencies" object
in package.json (next to "ember-cli-babel") with an appropriate semver (e.g., a
compatible ^7.x version) and run install to update lockfile so the addon has
`@babel/core` available at runtime.
Comment on lines
7
to
+49
| test('Should set the Action services', async function (assert) { | ||
| const service = this.owner.lookup('service:tawk-messenger'); | ||
|
|
||
| assert.strictEqual(typeof service.maximize, 'function', 'Maximize is not a function'); | ||
| assert.strictEqual(typeof service.minimize, 'function', 'Minimize is not a function'); | ||
| assert.strictEqual(typeof service.toggle, 'function', 'Toggle is not a function'); | ||
| assert.strictEqual(typeof service.popup, 'function', 'Popup is not a function'); | ||
| assert.strictEqual(typeof service.showWidget, 'function', 'ShowWidget is not a function'); | ||
| assert.strictEqual(typeof service.hideWidget, 'function', 'HideWidget is not a function'); | ||
| assert.strictEqual(typeof service.toggleVisibility, 'function', 'ToggleVisibility is not a function'); | ||
| assert.strictEqual(typeof service.endChat, 'function', 'EndChat is not a function'); | ||
| assert.strictEqual( | ||
| typeof service.tawkMaximize, | ||
| 'function', | ||
| 'Maximize is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkMinimize, | ||
| 'function', | ||
| 'Minimize is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkToggle, | ||
| 'function', | ||
| 'Toggle is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkPopup, | ||
| 'function', | ||
| 'Popup is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkShowWidget, | ||
| 'function', | ||
| 'ShowWidget is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkHideWidget, | ||
| 'function', | ||
| 'HideWidget is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkToggleVisibility, | ||
| 'function', | ||
| 'ToggleVisibility is not a function', | ||
| ); | ||
| assert.strictEqual( | ||
| typeof service.tawkEndChat, | ||
| 'function', | ||
| 'EndChat is not a function', | ||
| ); |
There was a problem hiding this comment.
Missing assertions for tawkStart, tawkShutdown, and switchWidget.
The "Action services" test covers 8 methods but omits tawkStart, tawkShutdown (both defined in the service at Lines 15 and 19), and switchWidget (Line 128). Add them to maintain consistent coverage:
Proposed additions
test('Should set the Action services', async function (assert) {
const service = this.owner.lookup('service:tawk-messenger');
+ assert.strictEqual(
+ typeof service.tawkStart,
+ 'function',
+ 'Start is not a function',
+ );
+ assert.strictEqual(
+ typeof service.tawkShutdown,
+ 'function',
+ 'Shutdown is not a function',
+ );
assert.strictEqual(
typeof service.tawkMaximize,
...
+ assert.strictEqual(
+ typeof service.tawkSwitchWidget,
+ 'function',
+ 'SwitchWidget is not a function',
+ );
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/services/tawk-messenger-test.js` around lines 7 - 49, The test is
missing assertions for three methods defined on the tawk-messenger service; add
strictEqual assertions similar to the others for service.tawkStart,
service.tawkShutdown, and service.switchWidget to verify each is a 'function'.
Locate the existing test block "Should set the Action services" (uses const
service = this.owner.lookup('service:tawk-messenger')) and append the three
assertions with the same message pattern used for the others (e.g., 'Start is
not a function', 'Shutdown is not a function', 'switchWidget is not a
function').
Comment on lines
+8
to
+9
| "declaration": true, | ||
| "declarationDir": "declarations", |
There was a problem hiding this comment.
declaration: true is a no-op — noEmit: true is still inherited from the base config.
The @tsconfig/ember base sets "noEmit": true and "declaration": false. This project overrides declaration: true and sets declarationDir: "declarations", but never overrides noEmit: false. Since noEmit: true takes precedence, TypeScript will not emit any output files — including the .d.ts declarations — regardless of what declaration is set to.
To actually generate declarations, you need noEmit: false alongside declaration: true. The Ember Guides recommend a separate tsconfig.declarations.json (for v1 addons) specifically for emitting declaration files, keeping the main tsconfig.json with noEmit: true for IDE/type-checking use.
🔧 Minimal fix: add noEmit: false to the project tsconfig (or create a separate tsconfig.declarations.json)
Option A — Override noEmit inline (simplest, but emits on every tsc run):
"compilerOptions": {
"strict": false,
+ "noEmit": false,
"declaration": true,
"declarationDir": "declarations",Option B — Preferred for v1 addons: create tsconfig.declarations.json and run it as a prepack script:
// tsconfig.declarations.json
{
"extends": "./tsconfig.json",
"compilerOptions": {
"noEmit": false,
"declaration": true,
"declarationDir": "declarations"
}
}Then in package.json:
"scripts": {
"prepack": "tsc --project tsconfig.declarations.json"
}Also remove /declarations/ from .npmignore so the emitted .d.ts files are published.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tsconfig.json` around lines 8 - 9, The tsconfig currently sets "declaration":
true but inherits "noEmit": true from the `@tsconfig/ember` base so no .d.ts files
are emitted; fix by either (A) explicitly setting "noEmit": false in
tsconfig.json alongside "declaration": true and "declarationDir": "declarations"
so tsc will emit declarations, or (B, preferred for v1 addons) add a new
tsconfig.declarations.json that extends the existing tsconfig and sets "noEmit":
false, "declaration": true and "declarationDir": "declarations", add a prepack
script to run tsc --project tsconfig.declarations.json to emit .d.ts files
before publishing, and ensure /declarations/ is removed from .npmignore so the
emitted declarations are included in the package.
Comment on lines
+3
to
+7
| declare global { | ||
| interface Window { | ||
| Tawk_API: TawkAPI; | ||
| Tawk_LoadStart: Date; | ||
| } |
There was a problem hiding this comment.
Tawk_API and Tawk_LoadStart should be optional on Window.
These globals are only set at runtime after the component's load() method runs (see addon/components/tawk-messenger.ts Line 71). Declaring them as required means TypeScript won't warn about accessing them before initialization — which is exactly the kind of bug strong typing should catch.
Proposed fix
declare global {
interface Window {
- Tawk_API: TawkAPI;
- Tawk_LoadStart: Date;
+ Tawk_API?: TawkAPI;
+ Tawk_LoadStart?: Date;
}
}This would require a few narrowing checks (or non-null assertions) in the service and component, but it would make the type contract honest.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| declare global { | |
| interface Window { | |
| Tawk_API: TawkAPI; | |
| Tawk_LoadStart: Date; | |
| } | |
| declare global { | |
| interface Window { | |
| Tawk_API?: TawkAPI; | |
| Tawk_LoadStart?: Date; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/global.d.ts` around lines 3 - 7, Update the global Window declaration
so Tawk_API and Tawk_LoadStart are optional rather than required: change the
Window interface members Tawk_API and Tawk_LoadStart to optional (e.g.,
Tawk_API? and Tawk_LoadStart?) and then update call sites (notably the service
using window.Tawk_API and the component method load() in
addon/components/tawk-messenger.ts) to perform proper runtime null/undefined
checks or use safe narrowing before accessing them; this keeps the type contract
accurate and forces consumers to guard access.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores