carverauto/serviceradar

Fork 0

1769 bugui device search is broken #2322

Merged

mfreeman451 merged 3 commits from refs/pull/2322/head into main

2025-10-15 05:35:58 +00:00

mfreeman451 commented

2025-10-15 05:00:21 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1771
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1771
Original created: 2025-10-15T05:00:21Z
Original updated: 2025-10-15T05:36:01Z
Original head: carverauto/serviceradar:1769-bugui-device-search-is-broken
Original base: main
Original merged: 2025-10-15T05:35:58Z by @mfreeman451

PR Type

Bug fix, Tests

Description

Fixed web UI device search to use unified search filter instead of hostname-only matching
Enhanced SRQL query planner with smart IP/MAC address search across multiple device fields
Optimized test timeouts and added proper cleanup handlers to prevent flakiness
Added case-insensitive LIKE operations (ILIKE) for improved search matching

Diagram Walkthrough

flowchart LR
  A["Web UI Search"] -- "uses search filter" --> B["SRQL Query Planner"]
  B -- "expands to multiple fields" --> C["hostname/ip/mac/device_id"]
  C -- "ILIKE operator" --> D["Case-insensitive matching"]
  B -- "detects IP literals" --> E["Exact + wildcard IP search"]

File Walkthrough

Relevant files

Bug fix

1 files

Dashboard.tsx
Replace hostname filter with unified search parameter +5/-3

Enhancement

3 files

query_planner.ml `Add smart device search with IP detection and multi-field matching`	+61/-4
translator.ml `Switch to case-insensitive LIKE and improve array field detection`	+21/-15
field_mapping.ml `Add ipaddress and macaddress field aliases for devices`	+2/-2

Tests

8 files

test_query_engine.ml `Add comprehensive tests for device search and IP filtering`	+78/-6
sweeper_test.go `Reduce test timeouts and add proper cleanup handlers`	+83/-18
sweeper.go `Make KV watch backoff parameters overridable for testing`	+8/-6
base_processor_test.go `Reduce memory test sleep durations for faster execution`	+2/-2
sticky_availability_test.go `Fix formatting and add cleanup handler for store`	+72/-70
circuit_breaker_test.go `Replace retry loops with Eventually assertion for reliability`	+11/-16
service_error_handling_test.go `Extract timeout constants and reduce test durations`	+12/-6
service_test.go `Reduce operation delays and wait timeouts in tests`	+8/-3

Imported from GitHub pull request. Original GitHub pull request: #1771 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1771 Original created: 2025-10-15T05:00:21Z Original updated: 2025-10-15T05:36:01Z Original head: carverauto/serviceradar:1769-bugui-device-search-is-broken Original base: main Original merged: 2025-10-15T05:35:58Z by @mfreeman451 --- ### **PR Type** Bug fix, Tests ___ ### **Description** - Fixed web UI device search to use unified `search` filter instead of hostname-only matching - Enhanced SRQL query planner with smart IP/MAC address search across multiple device fields - Optimized test timeouts and added proper cleanup handlers to prevent flakiness - Added case-insensitive LIKE operations (ILIKE) for improved search matching ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Web UI Search"] -- "uses search filter" --> B["SRQL Query Planner"] B -- "expands to multiple fields" --> C["hostname/ip/mac/device_id"] C -- "ILIKE operator" --> D["Case-insensitive matching"] B -- "detects IP literals" --> E["Exact + wildcard IP search"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>Dashboard.tsx</strong><dd><code>Replace hostname filter with unified search parameter</code>        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-da1f03ea181f8f19b672164eb86b914cef8bcef6ea7df61d174e3c19421a3461">+5/-3</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>query_planner.ml</strong><dd><code>Add smart device search with IP detection and multi-field matching</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-b215cfcd9aca2dc66252f05b2e2c2a39cde9d86744ee5f87b935077ed3863b06">+61/-4</a>    </td> </tr> <tr> <td><strong>translator.ml</strong><dd><code>Switch to case-insensitive LIKE and improve array field detection</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-c33a65c30e4433358fbe3a1806b6c35291fd999f3abfdaad0ca38419e94764b9">+21/-15</a>  </td> </tr> <tr> <td><strong>field_mapping.ml</strong><dd><code>Add ipaddress and macaddress field aliases for devices</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-202ecb348ad59ad7d01c3319a6719ffcb0d04acc7d7b4a8ec5b69c825513fca9">+2/-2</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>8 files</summary><table> <tr> <td><strong>test_query_engine.ml</strong><dd><code>Add comprehensive tests for device search and IP filtering</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-0c1dcb695d94269d0e3f899c01455e21cb50242b4665e3a0ef5f2b198c5cbe2f">+78/-6</a>    </td> </tr> <tr> <td><strong>sweeper_test.go</strong><dd><code>Reduce test timeouts and add proper cleanup handlers</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-7714a2d9e6f06cd0d5944247437dda7796228b95a507478e92f0fc6e09179424">+83/-18</a>  </td> </tr> <tr> <td><strong>sweeper.go</strong><dd><code>Make KV watch backoff parameters overridable for testing</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47a">+8/-6</a>      </td> </tr> <tr> <td><strong>base_processor_test.go</strong><dd><code>Reduce memory test sleep durations for faster execution</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-4ed190a88d8cbc2a1111b5e1ea955789305c1198baf80e33e2a4d0fb74fca627">+2/-2</a>      </td> </tr> <tr> <td><strong>sticky_availability_test.go</strong><dd><code>Fix formatting and add cleanup handler for store</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-2f0503b2d4b07c57bf1e7f979f2d700fae0e680ae3eea8e7a9ce5f9a3be08eb4">+72/-70</a>  </td> </tr> <tr> <td><strong>circuit_breaker_test.go</strong><dd><code>Replace retry loops with Eventually assertion for reliability</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-ad013257ec45e4158dbc1a5cca84ab158db4271198f40d4ec4c3aef269033d46">+11/-16</a>  </td> </tr> <tr> <td><strong>service_error_handling_test.go</strong><dd><code>Extract timeout constants and reduce test durations</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-d0105ea2cc75138b515ca8bf402050cfc718bd0688de8e62e866538da1adc609">+12/-6</a>    </td> </tr> <tr> <td><strong>service_test.go</strong><dd><code>Reduce operation delays and wait timeouts in tests</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1771/files#diff-1775a061aca7003b65ee5bac4510511d264530cadb4939909327db56f6163092">+8/-3</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-15 05:01:00 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1771#issuecomment-3404529093
Original created: 2025-10-15T05:01:00Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟡

🎫 #1769

🟢	Fix device view search/filter so users can search by device IP address.
🟢	Ensure UI search behavior matches expected user experience for device filtering.
⚪	Provide robust handling that works across browsers/environments mentioned.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

⚪

No custom compliance provided

Follow the guide to enable custom compliance check.

Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1771#issuecomment-3404529093 Original created: 2025-10-15T05:01:00Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/1769>#1769</a></summary> <table width='100%'><tbody> <tr><td rowspan=2>🟢</td> <td>Fix device view search/filter so users can search by device IP address.</td></tr> <tr><td>Ensure UI search behavior matches expected user experience for device filtering.</td></tr> <tr><td rowspan=1>⚪</td> <td>Provide robust handling that works across browsers/environments mentioned.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-10-15 05:02:06 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1771#issuecomment-3404530823
Original created: 2025-10-15T05:02:06Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix data race in test In the `TestKVWatchAutoReconnect` test, reorder the shutdown signals by calling `cancel()` before `close(sweeper.done)` to ensure a more deterministic and predictable shutdown sequence for the goroutine. pkg/sweeper/sweeper_test.go [997-1001] `// Clean shutdown + cancel() close(sweeper.done) <-sweeper.watchDone - cancel() wg.Wait()` Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that using two shutdown signals (`cancel()` and `close(sweeper.done)`) can lead to non-deterministic behavior in the test, and proposes reordering them to make the shutdown sequence more predictable.	Low
General	Improve test assertion logic Refactor the `TestCircuitBreaker_BasicFunctionality` test by moving the assertions on `finalErr` and `finalState` inside the `require.Eventually` block to make the test logic clearer and more robust. pkg/sync/circuit_breaker_test.go [70-83] - // Wait for timeout to transition to half-open - retry approach for robustness - var finalErr error - var ( - finalErr error - finalState CircuitBreakerState - ) + // Wait for timeout to transition to half-open and then close + require.Eventually(t, func() bool { + // This call should be allowed when the breaker is in Half-Open state + err := cb.Execute(context.Background(), func() error { return nil }) + if err != nil { + return false // Keep trying if execute fails + } + // After a successful call in Half-Open, it should transition to Closed + return assert.Equal(t, StateClosed, cb.GetState()) + }, 50time.Millisecond, 2time.Millisecond, "Circuit breaker should transition to half-open and then close") - require.Eventually(t, func() bool { - finalErr = cb.Execute(context.Background(), func() error { return nil }) - finalState = cb.GetState() - return finalErr == nil && finalState == StateClosed - }, 50time.Millisecond, 2time.Millisecond, "Should close after successful call") - - require.NoError(t, finalErr) - assert.Equal(t, StateClosed, finalState) - Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly points out that the assertions should be inside the `require.Eventually` block for better readability and robustness, which is a valid code improvement, although the current code is functionally correct.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1771#issuecomment-3404530823 Original created: 2025-10-15T05:02:06Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Fix data race in test</summary> ___ **In the <code>TestKVWatchAutoReconnect</code> test, reorder the shutdown signals by calling <br><code>cancel()</code> before <code>close(sweeper.done)</code> to ensure a more deterministic and <br>predictable shutdown sequence for the goroutine.** [pkg/sweeper/sweeper_test.go [997-1001]](https://github.com/carverauto/serviceradar/pull/1771/files#diff-7714a2d9e6f06cd0d5944247437dda7796228b95a507478e92f0fc6e09179424R997-R1001) ```diff // Clean shutdown + cancel() close(sweeper.done) <-sweeper.watchDone - cancel() wg.Wait() ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies that using two shutdown signals (`cancel()` and `close(sweeper.done)`) can lead to non-deterministic behavior in the test, and proposes reordering them to make the shutdown sequence more predictable. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Improve test assertion logic</summary> ___ **Refactor the <code>TestCircuitBreaker_BasicFunctionality</code> test by moving the assertions <br>on <code>finalErr</code> and <code>finalState</code> inside the <code>require.Eventually</code> block to make the test <br>logic clearer and more robust.** [pkg/sync/circuit_breaker_test.go [70-83]](https://github.com/carverauto/serviceradar/pull/1771/files#diff-ad013257ec45e4158dbc1a5cca84ab158db4271198f40d4ec4c3aef269033d46R70-R83) ```diff - // Wait for timeout to transition to half-open - retry approach for robustness - var finalErr error - var ( - finalErr error - finalState CircuitBreakerState - ) + // Wait for timeout to transition to half-open and then close + require.Eventually(t, func() bool { + // This call should be allowed when the breaker is in Half-Open state + err := cb.Execute(context.Background(), func() error { return nil }) + if err != nil { + return false // Keep trying if execute fails + } + // After a successful call in Half-Open, it should transition to Closed + return assert.Equal(t, StateClosed, cb.GetState()) + }, 50*time.Millisecond, 2*time.Millisecond, "Circuit breaker should transition to half-open and then close") - require.Eventually(t, func() bool { - finalErr = cb.Execute(context.Background(), func() error { return nil }) - finalState = cb.GetState() - return finalErr == nil && finalState == StateClosed - }, 50*time.Millisecond, 2*time.Millisecond, "Should close after successful call") - - require.NoError(t, finalErr) - assert.Equal(t, StateClosed, finalState) - ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly points out that the assertions should be inside the `require.Eventually` block for better readability and robustness, which is a valid code improvement, although the current code is functionally correct. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>