fix(connections): apply active search filters on incoming messages #1939
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that incoming MQTT messages are immediately filtered by active topic/payload search parameters before being rendered.
- Made
batchUpdateMsgsandrenderMessageasynchronous to await search filtering - Inserted
searchMessagechecks in message batching based onsearchParams - Updated RxJS subscriptions to await the new async rendering flow
Comments suppressed due to low confidence (1)
src/views/connections/ConnectionsDetail.vue:1461
- Add unit tests for
batchUpdateMsgscovering scenarios with and withoutsearchParamsto ensure filtering behaves as expected under various message batches.
if (this.searchParams.topic !== '' || this.searchParams.payload !== '') {
a170aad to
a5f9538
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR ensures incoming messages are immediately filtered by active topic/payload search parameters without needing to re-press Enter.
- Converted
batchUpdateMsgsto async and integratedsearchMessagefiltering. - Updated
renderMessageto awaitbatchUpdateMsgsand added error handling. - Changed RxJS subscriptions to use async callbacks for rendering and logging failures.
Comments suppressed due to low confidence (3)
src/views/connections/ConnectionsDetail.vue:1455
- Cloning the entire message list on each update can become a bottleneck under high message rates. Consider using an incremental append/remove approach or a fixed-size dequeue to avoid deep cloning the array.
let _messages = _.cloneDeep(this.recordMsgs.list)
src/views/connections/ConnectionsDetail.vue:1457
- The new branch that applies
searchMessagefiltering whensearchParamsis set isn't covered by existing tests. Add unit tests for both empty and non-empty searchParams to verify correct inclusion/exclusion of messages.
const filteredMsgs = incomingMsgs.filter((msg) => {
src/views/connections/ConnectionsDetail.vue:1588
- [nitpick] The error log is generic and may lack context. Include identifiers like connection ID or message details to improve troubleshooting.
this.$log.error(`Error rendering message: ${error}`)
| messageSubject$.subscribe(async (message) => { | ||
| if (!message) return | ||
| this.printMessageLog(id, message) | ||
| if (isBufferEnabled) { | ||
| messageBuffer$.next(message) | ||
| } else { | ||
| this.renderMessage(id, message) | ||
| try { | ||
| await this.renderMessage(id, message) | ||
| } catch (error) { | ||
| this.$log.error(`Error rendering message: ${error}`) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Using an async callback in subscribe may lead to overlapping render calls if messages arrive faster than processing completes. Consider using RxJS operators like concatMap or exhaustMap to serialize async rendering and maintain order.
Copilot uses AI. Check for mistakes.
a5f9538 to
90d25b7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@muzaffarmhd Thank you for your contribution! I noticed an optimization opportunity: since After reviewing the code, I found that the I suggest refactoring this function to a synchronous implementation, which would simplify the entire call chain and improve code readability and maintainability. Here's a reference implementation: type MatchMultipleSearch = (data: any[], params: SearchParams) => any[] | null
const matchMultipleSearch: MatchMultipleSearch = (data, params) => {
const paramsKeys = Object.keys(params)
try {
const filterData = data.filter(($) => {
return paramsKeys.every((oneKey) => {
if ($[oneKey]) {
const key: string = $[oneKey].toLowerCase().replace(/\s+/g, '')
const value: string = params[oneKey]
.toLocaleLowerCase()
.replace(/\s+/g, '')
.replace(/[~#^$@%&!+*]/gi, (val) => `\\${val}`)
return key.match(value)
} else {
return null
}
})
})
return filterData
} catch (error) {
throw error
}
} |
|
Thanks for your review, well as you mention, it's just filtering and string matching, and the promise overhead isn't needed, for this. I'll make the necessary changes and squash the next commit into this one. |
90d25b7 to
7f2c1e3
Compare
|
@Red-Asuka I have made the necessary changes. The |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures incoming messages are filtered by active search parameters immediately upon receipt.
- Converts
searchMessageto synchronous for direct filtering. - Updates
batchUpdateMsgsto applysearchMessagewhen any search parameters are set. - Refactors
matchMultipleSearchto remove its Promise wrapper and return results directly.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/views/connections/ConnectionsDetail.vue | Synchronous searchMessage and updated message buffering logic |
| src/utils/matchMultipleSearch.ts | Removed Promise wrapper from matchMultipleSearch |
Comments suppressed due to low confidence (1)
src/views/connections/ConnectionsDetail.vue:1289
- [nitpick] You can simplify this to
return Boolean(res?.length);for clarity and brevity.
return res && res.length ? true : false
| private async searchMessage(oneMessage: MessageModel): Promise<boolean> { | ||
| const res = await matchMultipleSearch([oneMessage], this.searchParams) | ||
| private searchMessage(oneMessage: MessageModel): boolean { | ||
| const res = matchMultipleSearch([oneMessage], this.searchParams) | ||
| return res && res.length ? true : false |
There was a problem hiding this comment.
Since matchMultipleSearch can return null, res.length may throw a runtime error. Consider guarding with res?.length or ensuring the function never returns null.
| return res && res.length ? true : false | |
| return res?.length ? true : false |
Copilot uses AI. Check for mistakes.
| @@ -2,30 +2,29 @@ interface SearchParams { | |||
| [propName: string]: string | |||
| } | |||
|
|
|||
| type MatchMultipleSearch = (data: any[], params: SearchParams) => Promise<any[] | null> | |||
| type MatchMultipleSearch = (data: any[], params: SearchParams) => any[] | null | |||
There was a problem hiding this comment.
The return type includes null, but the implementation always returns an array or throws. Update the signature to any[] for consistency.
| type MatchMultipleSearch = (data: any[], params: SearchParams) => any[] | null | |
| type MatchMultipleSearch = (data: any[], params: SearchParams) => any[] |
Copilot uses AI. Check for mistakes.
| return null | ||
| } | ||
| }) | ||
| const matchMultipleSearch: MatchMultipleSearch = (data, params) => { |
There was a problem hiding this comment.
[nitpick] The try/catch only rethrows the error. Consider removing it or handling errors at the call site to simplify the function.
Copilot uses AI. Check for mistakes.
|
@muzaffarmhd Thank you for your contribution. |
PR Checklist
If you have any questions, you can refer to the Contributing Guide
What is the current behavior?
Incoming messages bypass active search filters unless Enter is pressed again.
Issue Number
Fixes #1900
What is the new behavior?
batchUpdateMsgs()updated to applysearchMessage()before renderingDoes this PR introduce a breaking change?
Specific Instructions
Other information