feat(voice poc): feature branch voice experiments #40764
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 2677752 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40764 +/- ##
===========================================
+ Coverage 69.95% 70.05% +0.09%
===========================================
Files 3336 3369 +33
Lines 123479 124629 +1150
Branches 21980 22190 +210
===========================================
+ Hits 86384 87310 +926
- Misses 33750 33960 +210
- Partials 3345 3359 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/call-history/service.ts">
<violation number="1" location="apps/meteor/server/services/call-history/service.ts:29">
P1: `importHistoryForUser` is awaited synchronously inside `search`, blocking the paginated query result until the external HTTP call completes. This turns every search request into a potentially multi-second operation, which is problematic for a listing endpoint that should be fast.</violation>
<violation number="2" location="apps/meteor/server/services/call-history/service.ts:33">
P1: The `type` filter on line 27 uses a weaker condition than the import gate on line 23. If the external history setting is enabled but `host` or `username` are empty, no import runs, yet `type` is still set to `'mitel'`. This causes the search to return zero mitel-type results even when local media-call entries exist, effectively hiding all call history.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 100 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
|
|
||
| const externalHistoryConfig = this.getExternalCallHistorySettings(); | ||
| if (externalHistoryConfig?.host && externalHistoryConfig.username) { | ||
| await importHistoryForUser(uid, externalHistoryConfig); |
There was a problem hiding this comment.
P1: importHistoryForUser is awaited synchronously inside search, blocking the paginated query result until the external HTTP call completes. This turns every search request into a potentially multi-second operation, which is problematic for a listing endpoint that should be fast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/services/call-history/service.ts, line 29:
<comment>`importHistoryForUser` is awaited synchronously inside `search`, blocking the paginated query result until the external HTTP call completes. This turns every search request into a potentially multi-second operation, which is problematic for a listing endpoint that should be fast.</comment>
<file context>
@@ -0,0 +1,64 @@
+
+ const externalHistoryConfig = this.getExternalCallHistorySettings();
+ if (externalHistoryConfig?.host && externalHistoryConfig.username) {
+ await importHistoryForUser(uid, externalHistoryConfig);
+ }
+
</file context>
| } | ||
|
|
||
| // If external history is toggled on, only external entries may be listed | ||
| const type = externalHistoryConfig ? 'mitel' : 'media-call'; |
There was a problem hiding this comment.
P1: The type filter on line 27 uses a weaker condition than the import gate on line 23. If the external history setting is enabled but host or username are empty, no import runs, yet type is still set to 'mitel'. This causes the search to return zero mitel-type results even when local media-call entries exist, effectively hiding all call history.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/services/call-history/service.ts, line 33:
<comment>The `type` filter on line 27 uses a weaker condition than the import gate on line 23. If the external history setting is enabled but `host` or `username` are empty, no import runs, yet `type` is still set to `'mitel'`. This causes the search to return zero mitel-type results even when local media-call entries exist, effectively hiding all call history.</comment>
<file context>
@@ -0,0 +1,64 @@
+ }
+
+ // If external history is toggled on, only external entries may be listed
+ const type = externalHistoryConfig ? 'mitel' : 'media-call';
+
+ const { cursor, totalCount } = CallHistory.findAllByUserIdAndSearchFilters(
</file context>
ggazzo
force-pushed
the
feat/voice
branch
5 times, most recently
from
7ff94a1 to
19af245
Compare
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments