fix: observation timestamp normalization to float8 - #373
Merged
Conversation
Cast EXTRACT(EPOCH FROM ...) * 1000 to float8 for both WiGLE observation queries to ensure numeric timestamps are returned to client for proper temporal rendering in Geospatial Explorer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cast EXTRACT(EPOCH FROM o.time) to float8 before multiplication to ensure numeric epoch millisecond timestamps in API responses. Update OpenAPI and API reference documentation to reflect numeric time field. Add focused test assertions for type correctness in service and integration layers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThis PR normalizes observation timestamps to numeric epoch-millisecond values across server queries, API docs, and OpenAPI schema, renames a WiGLE route parameter to ChangesObservation Timestamp and Route Fixes
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by QodoNormalize observation timestamps to float8 epoch milliseconds
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/e2e/wigleTimestampDiag.spec.ts (2)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the file header to reflect removed instrumentation.
The header comment says "Captures console logs at every instrumentation boundary to identify exactly where wigle_v3_last_seen stops propagating to the SEEN cell," but the
[popup:*]instrumentation logs were removed frommapHandlers.tsin this same PR. The test's log capture is now vestigial for its stated purpose. Consider updating the description to reflect that the test primarily verifies the rendered SEEN timestamp and mocks enrichment, rather than tracing instrumentation boundaries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/wigleTimestampDiag.spec.ts` around lines 1 - 9, Update the header comment in wigleTimestampDiag.spec.ts to match the current behavior, since the popup instrumentation referenced by the diagnostic note was removed from mapHandlers.ts. Revise the description around the test’s purpose so it reflects that wigleTimestampDiag.spec.ts now verifies the rendered SEEN timestamp and mocked enrichment flow rather than capturing console logs at instrumentation boundaries.
141-142: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace
waitForTimeoutwith a deterministic wait.
await page.waitForTimeout(600)is used to let asyncconsole.logpromise handlers resolve before checkinglogs. This is non-deterministic and can flake under load. Consider waiting for a specific condition instead, such as waiting for thelogsarray to reach an expected length or usingpage.waitForFunction.♻️ Suggested fix
- // Let async console.log promises resolve - await page.waitForTimeout(600); + // Let async console.log promise handlers resolve + await page.waitForFunction( + () => (window as any).__e2eLogsReady !== false, + { timeout: 5000 } + ).catch(() => {});Alternatively, restructure the log capture to resolve a promise when the enrichment warning is received, then
awaitthat promise directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/wigleTimestampDiag.spec.ts` around lines 141 - 142, Replace the non-deterministic `page.waitForTimeout(600)` in `wigleTimestampDiag.spec.ts` with a deterministic synchronization point. Update the test around the log-capture flow so it waits on a concrete condition tied to the `logs` array or a `page.waitForFunction`, or resolve a promise when the expected enrichment warning is captured and await that promise before asserting.tests/unit/observationService.test.ts (1)
135-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding float8 SQL assertions for WiGLE observation functions.
The
getObservationsByBSSIDtest now verifies the SQL contains(EXTRACT(EPOCH FROM o.time) * 1000)::float8 as time, but thegetWigleObservationsByBSSIDandgetWigleObservationsBatchtest blocks don't have equivalent assertions for their::float8cast. Adding them would ensure consistency if the cast is accidentally reverted in one query.♻️ Suggested additions
describe('getWigleObservationsByBSSID', () => { it('should return enriched WiGLE observations', async () => { // ... existing mock setup ... const result = await getWigleObservationsByBSSID('AA:BB'); expect(result).toEqual(mockRows); + const sql = (query as jest.Mock).mock.calls[0][0]; + expect(sql).toContain('(EXTRACT(EPOCH FROM we.time) * 1000)::float8 as time'); expect(query).toHaveBeenCalledWith(expect.stringContaining('app.wigle_v3_observations'), [ 'AA:BB', ]); }); });describe('getWigleObservationsBatch', () => { it('should return batch of enriched WiGLE observations', async () => { // ... existing mock setup ... const result = await getWigleObservationsBatch(['AA:BB', 'CC:DD']); expect(result).toEqual(mockRows); + const sql = (query as jest.Mock).mock.calls[0][0]; + expect(sql).toContain('(EXTRACT(EPOCH FROM we.time) * 1000)::float8 as time'); expect(query).toHaveBeenCalledWith(expect.stringContaining('ANY($1)'), [['AA:BB', 'CC:DD']]); }); });Also applies to: 175-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/observationService.test.ts` around lines 135 - 153, Add SQL string assertions in the getWigleObservationsByBSSID and getWigleObservationsBatch tests to verify the query includes the same ::float8 cast used in getObservationsByBSSID, so all WiGLE observation paths are covered consistently. Update the existing expect(query).toHaveBeenCalledWith checks in observationService.test.ts to match the relevant SQL built by getWigleObservationsByBSSID and getWigleObservationsBatch, specifically asserting the time expression contains (EXTRACT(EPOCH FROM o.time) * 1000)::float8 as time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/api/routes/v1/wigle/database.ts`:
- Around line 73-81: The route handler now uses the updated `:bssid` parameter
in `wigle/database.ts`, but the client-facing and documentation artifacts still
reference `:netid`. Propagate the rename in
`client/src/config/apiTestEndpoints.ts`, `docs/API_REFERENCE.md`,
`docs/api/route-inventory.md`, and the `tests/unit/wigleDatabase.test.ts`
describe block so all endpoint metadata stays consistent with the `page/network`
route and its `macParamMiddleware`/handler contract. Ensure the client config
entry, docs, and test names all match the current route param name used by
`getWiglePageNetworkFromMv` and `getWiglePageNetwork`.
In `@tests/e2e/wigleTimestampDiag.spec.ts`:
- Around line 74-87: The enrichment failure check in wigleTimestampDiag.spec.ts
is filtering for the wrong console prefix, so the warning emitted by
mapHandlers.ts is never recorded in logs. Update the page.on('console') handler
to match the actual warning tag from the enrichment path (the [wigle-tooltip]
enrichment fetch failed message), and keep the structured-args parsing so the
warning payload is captured. Then ensure enrichmentFailed is derived from the
captured warning entry so the assertion at the end reflects real enrichment
failures.
---
Nitpick comments:
In `@tests/e2e/wigleTimestampDiag.spec.ts`:
- Around line 1-9: Update the header comment in wigleTimestampDiag.spec.ts to
match the current behavior, since the popup instrumentation referenced by the
diagnostic note was removed from mapHandlers.ts. Revise the description around
the test’s purpose so it reflects that wigleTimestampDiag.spec.ts now verifies
the rendered SEEN timestamp and mocked enrichment flow rather than capturing
console logs at instrumentation boundaries.
- Around line 141-142: Replace the non-deterministic `page.waitForTimeout(600)`
in `wigleTimestampDiag.spec.ts` with a deterministic synchronization point.
Update the test around the log-capture flow so it waits on a concrete condition
tied to the `logs` array or a `page.waitForFunction`, or resolve a promise when
the expected enrichment warning is captured and await that promise before
asserting.
In `@tests/unit/observationService.test.ts`:
- Around line 135-153: Add SQL string assertions in the
getWigleObservationsByBSSID and getWigleObservationsBatch tests to verify the
query includes the same ::float8 cast used in getObservationsByBSSID, so all
WiGLE observation paths are covered consistently. Update the existing
expect(query).toHaveBeenCalledWith checks in observationService.test.ts to match
the relevant SQL built by getWigleObservationsByBSSID and
getWigleObservationsBatch, specifically asserting the time expression contains
(EXTRACT(EPOCH FROM o.time) * 1000)::float8 as time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83718142-9837-4a84-b788-d5aee5f99870
📒 Files selected for processing (11)
CHANGELOG.mdclient/src/components/wigle/mapHandlers.tsdocs/API_REFERENCE.mddocs/metrics/lines-of-code.mddocs/openapi.yamlserver/src/api/routes/v1/wigle/database.tsserver/src/services/observationService.tstests/e2e/README.mdtests/e2e/wigleTimestampDiag.spec.tstests/integration/api/v1/observations.test.tstests/unit/observationService.test.ts
Comment on lines
+73
to
+81
| '/page/network/:bssid', | ||
| macParamMiddleware, | ||
| asyncHandler(async (req: Request, res: Response) => { | ||
| const { netid } = req.params; | ||
| const { bssid } = req.params; | ||
| // Try MV first (single-row read); fall back to live 4-query fan-out if MV | ||
| // is unavailable (pre-migration deployment) or returns no row. | ||
| let network = await wigleService.getWiglePageNetworkFromMv(netid); | ||
| let network = await wigleService.getWiglePageNetworkFromMv(bssid); | ||
| if (!network) { | ||
| network = await wigleService.getWiglePageNetwork(netid); | ||
| network = await wigleService.getWiglePageNetwork(bssid); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cross-file contract drift: param rename not propagated to client config, docs, or route inventory.
The route param rename from :netid to :bssid is server-internal and doesn't change the URL path, so existing clients continue to work. However, multiple downstream artifacts still reference :netid:
client/src/config/apiTestEndpoints.ts:1081-1086— path and params still usenetiddocs/API_REFERENCE.md:943-951— documents:netiddocs/api/route-inventory.md:306-310— lists:netidtests/unit/wigleDatabase.test.ts:52-63— describe block still uses:netid
Per coding guidelines, every endpoint requires an entry in client/src/config/apiTestEndpoints.ts and a JSDoc comment on the route handler. While the JSDoc was updated, the client config and documentation were not, creating inconsistency that will confuse future maintainers and API consumers.
As per coding guidelines: "Every new endpoint requires: entry in client/src/config/apiTestEndpoints.ts, JSDoc comment on route handler, and note in relevant docs/schema/ file if touching DB schema."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/routes/v1/wigle/database.ts` around lines 73 - 81, The route
handler now uses the updated `:bssid` parameter in `wigle/database.ts`, but the
client-facing and documentation artifacts still reference `:netid`. Propagate
the rename in `client/src/config/apiTestEndpoints.ts`, `docs/API_REFERENCE.md`,
`docs/api/route-inventory.md`, and the `tests/unit/wigleDatabase.test.ts`
describe block so all endpoint metadata stays consistent with the `page/network`
route and its `macParamMiddleware`/handler contract. Ensure the client config
entry, docs, and test names all match the current route param name used by
`getWiglePageNetworkFromMv` and `getWiglePageNetwork`.
Source: Coding guidelines
Comment on lines
+74
to
+87
| page.on('console', (msg) => { | ||
| const text = msg.text(); | ||
| if (!text.includes('[popup:')) return; | ||
| // Playwright serialises structured console.log args — grab them | ||
| const args = msg.args(); | ||
| // tag is first arg, traceId is second, data object is third | ||
| Promise.all(args.map((a) => a.jsonValue().catch(() => String(a)))).then((vals) => { | ||
| logs.push({ | ||
| tag: String(vals[0] ?? ''), | ||
| traceId: String(vals[1] ?? ''), | ||
| data: vals[2] ?? {}, | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enrichment failure detection is broken — the console filter doesn't match the actual warning tag.
The console listener at line 76 filters with if (!text.includes('[popup:')) return;, but the actual enrichment failure warning in mapHandlers.ts (line 125) is console.warn('[wigle-tooltip] enrichment fetch failed', err) — which does not contain [popup:. As a result, the warning is never captured in the logs array, enrichmentFailed at line 167 is always undefined, and the assertion at line 181 (expect(enrichmentFailed, 'Enrichment must not fail').toBeUndefined()) will always pass regardless of whether enrichment actually failed.
This gives false confidence that the test verifies enrichment success.
🐛 Proposed fix — broaden the console filter
page.on('console', (msg) => {
const text = msg.text();
- if (!text.includes('[popup:')) return;
+ if (!text.includes('[popup:') && !text.includes('[wigle-tooltip]')) return;
// Playwright serialises structured console.log args — grab them
const args = msg.args();
// tag is first arg, traceId is second, data object is third
Promise.all(args.map((a) => a.jsonValue().catch(() => String(a)))).then((vals) => {
logs.push({
tag: String(vals[0] ?? ''),
traceId: String(vals[1] ?? ''),
data: vals[2] ?? {},
});
});
});Also applies to: 167-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/wigleTimestampDiag.spec.ts` around lines 74 - 87, The enrichment
failure check in wigleTimestampDiag.spec.ts is filtering for the wrong console
prefix, so the warning emitted by mapHandlers.ts is never recorded in logs.
Update the page.on('console') handler to match the actual warning tag from the
enrichment path (the [wigle-tooltip] enrichment fetch failed message), and keep
the structured-args parsing so the warning payload is captured. Then ensure
enrichmentFailed is derived from the captured warning entry so the assertion at
the end reflects real enrichment failures.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 11 files
Confidence score: 3/5
- In
tests/e2e/wigleTimestampDiag.spec.ts, theenrichmentFailedcheck depends on log data filled by a fire-and-forget async.then()in the console handler, so the assertion can run before logs are populated and intermittently fail or miss real regressions — make the log capture/processing awaitable (or assert via a deterministic signal) before merging. - In
tests/e2e/wigleTimestampDiag.spec.ts, the console filter only watches[popup:while the warning now emits[wigle-tooltip], so the test may never observe the expected failure path and can pass/fail for the wrong reason — align the filter with current prefixes and replacepage.waitForTimeout(600)with a deterministic wait condition to reduce CI flakiness. - In
server/src/api/routes/v1/wigle/database.ts, renaming the route param from:netidto:bssidwithout updatingclient/src/config/apiTestEndpoints.ts,docs/API_REFERENCE.md,docs/api/route-inventory.md, andtests/unit/wigleDat...risks endpoint mismatches (broken tests/callers and stale docs) after merge — update all referenced clients/tests/docs to the same param name before merging.
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="tests/e2e/wigleTimestampDiag.spec.ts">
<violation number="1" location="tests/e2e/wigleTimestampDiag.spec.ts:74">
P2: The enrichmentFailed assertion reads a logs array populated by a fire-and-forget async promise chain inside the console event handler. Since the .then() callback isn't awaited before the assertion on line 130, a slow promise resolution (common in CI) would cause logs.find() to miss entries even if a failure event was emitted — resulting in a false-negative pass. Await the promise chain or use page.waitForEvent('console') instead.</violation>
<violation number="2" location="tests/e2e/wigleTimestampDiag.spec.ts:76">
P2: The console filter here only captures messages containing `[popup:`, but the enrichment failure warning was changed to use `[wigle-tooltip]` prefix (in `mapHandlers.ts`). Since the `[wigle-tooltip] enrichment fetch failed` message doesn't pass this filter, it never enters the `logs` array. Consequently, the `enrichmentFailed` check at line 167 will always be `undefined`, and the assertion at line 181 will always pass — even if enrichment actually fails. The filter should also match `[wigle-tooltip]` to detect real failures.</violation>
<violation number="3" location="tests/e2e/wigleTimestampDiag.spec.ts:142">
P2: Using page.waitForTimeout(600) instead of a deterministic wait condition introduces flakiness (too short under CI load, unnecessarily slow otherwise) and deviates from the repo convention of waiting for visible elements or text. Replace with a targeted condition — for example, await a specific log count or the presence of a known log entry.</violation>
</file>
<file name="server/src/api/routes/v1/wigle/database.ts">
<violation number="1" location="server/src/api/routes/v1/wigle/database.ts:73">
P2: The route param was renamed from `:netid` to `:bssid` here, but the corresponding references in `client/src/config/apiTestEndpoints.ts`, `docs/API_REFERENCE.md`, `docs/api/route-inventory.md`, and `tests/unit/wigleDatabase.test.ts` still use `:netid`. While the URL path itself is unchanged (so clients still work), the inconsistency across the codebase will confuse future maintainers and may cause test/config mismatches if someone searches by param name.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client (Browser)
participant Router as API Router v1
participant ObsSvc as Observation Service
participant DB as PostgreSQL
participant Frontend as Frontend Map Handlers
Note over Client,Frontend: CHANGED: Timestamp normalized to float8 numeric ms
alt Core network observations
Client->>Router: GET /api/networks/:bssid/observations
Router->>ObsSvc: getObservationsByBSSID(bssid, lat, lon)
ObsSvc->>DB: SELECT ... (EXTRACT(EPOCH FROM o.time)*1000)::float8 as time ...
DB-->>ObsSvc: Rows with time as float8 (e.g. 1.72e12)
ObsSvc-->>Router: { observations: [{ time: 1720000000000 }, ...] }
Router-->>Client: JSON response (time is numeric)
else WiGLE observations (popup enrichment)
Client->>Router: GET /api/wigle/page/network/:bssid
Note over Router: Route param renamed to :bssid (no URL change)
Router->>ObsSvc: getWiglePageNetwork(bssid)
opt If materialized view returns no row
ObsSvc->>ObsSvc: Fallback to getWiglePageNetwork(bssid)
end
ObsSvc->>DB: SELECT ... (EXTRACT(EPOCH FROM we.time)*1000)::float8 as time ...
DB-->>ObsSvc: Rows with time as float8
ObsSvc->>ObsSvc: getWigleObservationsBatch/ByBSSID (same cast)
ObsSvc-->>Router: Enrichment data (wigle_v3_last_seen, wigle_v2_lasttime as float8)
Router-->>Client: { wigle: { wigle_v3_last_seen: 1720000000000, ... } }
Client->>Frontend: normalizeTooltipData(mergedData)
Frontend-->>Client: Render popup with numeric timestamps
end
Note over Client,Router: CHANGED: Removed [popup:*] debug console.log calls
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| await expect(page.locator('.sc-popup')).toContainText('42', { timeout: 8000 }); | ||
|
|
||
| // Let async console.log promises resolve | ||
| await page.waitForTimeout(600); |
Contributor
There was a problem hiding this comment.
P2: Using page.waitForTimeout(600) instead of a deterministic wait condition introduces flakiness (too short under CI load, unnecessarily slow otherwise) and deviates from the repo convention of waiting for visible elements or text. Replace with a targeted condition — for example, await a specific log count or the presence of a known log entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/wigleTimestampDiag.spec.ts, line 142:
<comment>Using page.waitForTimeout(600) instead of a deterministic wait condition introduces flakiness (too short under CI load, unnecessarily slow otherwise) and deviates from the repo convention of waiting for visible elements or text. Replace with a targeted condition — for example, await a specific log count or the presence of a known log entry.</comment>
<file context>
@@ -0,0 +1,182 @@
+ await expect(page.locator('.sc-popup')).toContainText('42', { timeout: 8000 });
+
+ // Let async console.log promises resolve
+ await page.waitForTimeout(600);
+
+ // Expand the Timestamps <details> section if present
</file context>
| // Collect every [popup:*] console message in order | ||
| const logs: { tag: string; traceId: string; data: any }[] = []; | ||
|
|
||
| page.on('console', (msg) => { |
Contributor
There was a problem hiding this comment.
P2: The enrichmentFailed assertion reads a logs array populated by a fire-and-forget async promise chain inside the console event handler. Since the .then() callback isn't awaited before the assertion on line 130, a slow promise resolution (common in CI) would cause logs.find() to miss entries even if a failure event was emitted — resulting in a false-negative pass. Await the promise chain or use page.waitForEvent('console') instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/wigleTimestampDiag.spec.ts, line 74:
<comment>The enrichmentFailed assertion reads a logs array populated by a fire-and-forget async promise chain inside the console event handler. Since the .then() callback isn't awaited before the assertion on line 130, a slow promise resolution (common in CI) would cause logs.find() to miss entries even if a failure event was emitted — resulting in a false-negative pass. Await the promise chain or use page.waitForEvent('console') instead.</comment>
<file context>
@@ -0,0 +1,182 @@
+ // Collect every [popup:*] console message in order
+ const logs: { tag: string; traceId: string; data: any }[] = [];
+
+ page.on('console', (msg) => {
+ const text = msg.text();
+ if (!text.includes('[popup:')) return;
</file context>
|
|
||
| page.on('console', (msg) => { | ||
| const text = msg.text(); | ||
| if (!text.includes('[popup:')) return; |
Contributor
There was a problem hiding this comment.
P2: The console filter here only captures messages containing [popup:, but the enrichment failure warning was changed to use [wigle-tooltip] prefix (in mapHandlers.ts). Since the [wigle-tooltip] enrichment fetch failed message doesn't pass this filter, it never enters the logs array. Consequently, the enrichmentFailed check at line 167 will always be undefined, and the assertion at line 181 will always pass — even if enrichment actually fails. The filter should also match [wigle-tooltip] to detect real failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/wigleTimestampDiag.spec.ts, line 76:
<comment>The console filter here only captures messages containing `[popup:`, but the enrichment failure warning was changed to use `[wigle-tooltip]` prefix (in `mapHandlers.ts`). Since the `[wigle-tooltip] enrichment fetch failed` message doesn't pass this filter, it never enters the `logs` array. Consequently, the `enrichmentFailed` check at line 167 will always be `undefined`, and the assertion at line 181 will always pass — even if enrichment actually fails. The filter should also match `[wigle-tooltip]` to detect real failures.</comment>
<file context>
@@ -0,0 +1,182 @@
+
+ page.on('console', (msg) => {
+ const text = msg.text();
+ if (!text.includes('[popup:')) return;
+ // Playwright serialises structured console.log args — grab them
+ const args = msg.args();
</file context>
Suggested change
| if (!text.includes('[popup:')) return; | |
| if (!text.includes('[popup:') && !text.includes('[wigle-tooltip]')) return; |
| */ | ||
| router.get( | ||
| '/page/network/:netid', | ||
| '/page/network/:bssid', |
Contributor
There was a problem hiding this comment.
P2: The route param was renamed from :netid to :bssid here, but the corresponding references in client/src/config/apiTestEndpoints.ts, docs/API_REFERENCE.md, docs/api/route-inventory.md, and tests/unit/wigleDatabase.test.ts still use :netid. While the URL path itself is unchanged (so clients still work), the inconsistency across the codebase will confuse future maintainers and may cause test/config mismatches if someone searches by param name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/api/routes/v1/wigle/database.ts, line 73:
<comment>The route param was renamed from `:netid` to `:bssid` here, but the corresponding references in `client/src/config/apiTestEndpoints.ts`, `docs/API_REFERENCE.md`, `docs/api/route-inventory.md`, and `tests/unit/wigleDatabase.test.ts` still use `:netid`. While the URL path itself is unchanged (so clients still work), the inconsistency across the codebase will confuse future maintainers and may cause test/config mismatches if someone searches by param name.</comment>
<file context>
@@ -70,15 +70,15 @@ const validateWigleNetworksQuery = validateQuery({
*/
router.get(
- '/page/network/:netid',
+ '/page/network/:bssid',
macParamMiddleware,
asyncHandler(async (req: Request, res: Response) => {
</file context>
Code Review by Qodo
Context used✅ Compliance rules (platform):
232 rules 1. API_ENDPOINTS still uses :netid
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+73
to
+81
| '/page/network/:bssid', | ||
| macParamMiddleware, | ||
| asyncHandler(async (req: Request, res: Response) => { | ||
| const { netid } = req.params; | ||
| const { bssid } = req.params; | ||
| // Try MV first (single-row read); fall back to live 4-query fan-out if MV | ||
| // is unavailable (pre-migration deployment) or returns no row. | ||
| let network = await wigleService.getWiglePageNetworkFromMv(netid); | ||
| let network = await wigleService.getWiglePageNetworkFromMv(bssid); | ||
| if (!network) { | ||
| network = await wigleService.getWiglePageNetwork(netid); | ||
| network = await wigleService.getWiglePageNetwork(bssid); |
There was a problem hiding this comment.
1. api_endpoints still uses :netid 📘 Rule violation ≡ Correctness
The WiGLE DB page route parameter was renamed from :netid to :bssid, but the Admin API Testing endpoint registry/preset still references the old :netid path. This leaves the testing UI out of sync with the backend and causes Bulk Endpoint Verification to generate non-matching/invalid URLs that consistently fail MAC validation, violating the compliance requirement to register modified API routes.
Agent Prompt
## Issue description
The WiGLE DB Page endpoint preset in the Admin API Testing registry is out of sync with the backend: the backend route now uses `:bssid` and the MAC validation middleware reads `req.params.bssid`, but the registry/preset still uses `:netid`. This mismatch breaks the compliance requirement to register modified API routes and makes Bulk Endpoint Verification generate invalid URLs (often substituting `'1'`) that fail MAC validation and report false failures.
## Issue Context
- Backend route changed to `/api/wigle/page/network/:bssid` (previously `:netid`).
- `macParamMiddleware` validates `req.params.bssid`, so requests using `:netid` placeholders won’t map correctly.
- Admin bulk tester fills missing path params via `FALLBACK_PARAMS[paramName] || '1'`; because the preset still uses `netid`, it can fall back to `'1'`, producing `/api/wigle/page/network/1` and consistent 400 failures from MAC validation.
## Fix Focus Areas
- server/src/api/routes/v1/wigle/database.ts[72-82]
- client/src/config/apiTestEndpoints.ts[1074-1086]
- client/src/components/admin/hooks/useApiTesting.ts[220-267]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
Overview
Normalizes observation endpoint timestamps to float8 numeric type for JavaScript-safe epoch millisecond values.
Changes
EXTRACT(EPOCH FROM o.time) * 1000tofloat8to ensure numeric epoch milliseconds in API responsesValidation
Note
This branch previously had a buried Vite build fix (NODE_ENV=production) in the same commit. That fix has been extracted and already landed on master (commit 7917b37). This PR contains only the observation timestamp normalization.
Summary by cubic
Return observation timestamps as numeric epoch milliseconds (
float8) across API and WiGLE endpoints to fix JavaScript date handling. Updates schema/docs and tests; removes WiGLE popup debug logs and aligns the WiGLE network route to:bssid.(EXTRACT(EPOCH FROM time) * 1000)::float8in all observation queries (core API and WiGLE) to return JSON numbers in ms.docs/openapi.yamland API reference totype: number/format: double; add unit/integration type checks and a deterministic E2E diagnostic; clarify E2E Compose steps.:bssid(no URL or behavior change).Written for commit 7de1a0d. Summary will update on new commits.
Summary by CodeRabbit
observations[].timeis a JSON number in milliseconds.