PR Review: AI search updates time picker instead of filtering by timestamp

Good overall approach — centralising time picker updates into updateTimePicker and keeping timestamp logic out of KQL queries is the right design. A few issues worth addressing:

Bug: systemPromptOverride silently drops timezone

buildSystemPrompt in AI.hs:606:

let basePrompt = fromMaybe (systemPrompt now config.timezone) config.systemPromptOverride

When systemPromptOverride is Just, the user's timezone is never injected. This is likely to cause wrong time interpretation for callers that set a custom system prompt (e.g. issue investigation). Either append the timezone line to the override, or document this limitation explicitly.

Redundant double-parse of requestBody in Log.hs

let inputTextM = AET.parseMaybe (AE.withObject "request" (AE..: "input")) requestBody
    timezoneM  = AET.parseMaybe (AE.withObject "request" (AE..: "timezone")) requestBody

The requestBody is traversed twice. With aeson you can parse both fields in one pass:

let parsed = AET.parseMaybe (AE.withObject "request" $ \o -> (,) <$> o AE..: "input" <*> o AE..:? "timezone") requestBody

Then pattern-match on parsed to get both inputText and timezoneM together.

submitAction for custom date range lost setParams on the Just submitForm path

Before this PR the Nothing branch of submitAction called window.setParams({from, to, since: ''}). After the refactor updateTimePicker handles that. But the Just fm branch (TimePicker.hs:193) no longer calls setParams or updateTimePicker — it only triggers the HTMX form submit and closes the popover. The URL params (from/to) will not be updated when a submitForm target is provided. Was this intentional, or did updateTimePicker need to be called here too?

updateTimePicker defaults targetPr to 'n' silently

In main.ts:

const tp = opts?.targetPr || 'n';

The default 'n' matches the Haskell fromMaybe "n" targetIdM convention, which is fine, but a short comment explaining the 'n' sentinel would help future readers understand it isn't an arbitrary default.

Minor: onClickHandler string concatenation instead of quasi-quote

TimePicker.hs:170:

"on click call window.updateTimePicker({since: @data-value}, {targetPr: '" <> targetPr <> "', label: @data-title}) then " <> action <> " then call #" <> targetPr <> "-timepicker-popover.hidePopover()"

The surrounding code uses [text|...|] / [fmt|...|] interpolation consistently. This manual concatenation is harder to read and breaks the pattern — [fmt|...|] with escaped braces for the JS object literals would be more consistent.

Nit: kqlGuide instruction placement

The new IMPORTANT block is appended after the closing example block. Consider placing it alongside the existing IMPORTANT block about field names so all constraints are grouped together.

Overall the feature logic is sound and the refactor to updateTimePicker is a clear improvement.

PR Review: fix AI search updates time picker instead of filtering by timestamp

Overall this is a clean fix — moving time filtering out of KQL and into the time picker UI is the right architectural call. A few issues worth addressing:

src/Pkg/AI.hs — buildSystemPrompt timezone duplication

The timezoneSection / whenJust' logic is both redundant and confusing:

buildSystemPrompt config now =
  let basePrompt = fromMaybe (systemPrompt now config.timezone) config.systemPromptOverride
      timezoneSection = maybe "" (\tz -> "\nUSER TIMEZONE: " <> tz <> "\nCURRENT TIME (UTC): " <> show now <> "\n") config.timezone
      ...
   in basePrompt <> whenJust' config.systemPromptOverride timezoneSection <> facetSection <> customSection
  where
    whenJust' (Just _) x = x
    whenJust' Nothing _ = ""

• When systemPromptOverride is Nothing, systemPrompt now config.timezone already embeds the timezone — whenJust' returns "", so it's fine.
• When systemPromptOverride is Just _, timezoneSection is appended — injecting timezone into custom override prompts that may not expect it.
• whenJust' just reimplements maybe "" (const x). Drop the local helper and inline it:

in basePrompt <> maybe "" (const tzSection) config.systemPromptOverride <> facetSection <> customSection

src/Pkg/Components/TimePicker.hs — submitAction / setParams({}, true)

-- updateTimePicker already set params; reload page for non-form case
"window.setParams({}, true); document.getElementById(`$targetPr-timepicker-popover`).hidePopover();"

The previous code explicitly passed {from, to, since} to setParams. Passing {} with reload=true may clear other URL params depending on how setParams merges vs. replaces its input. Since updateTimePicker already called setParams with the correct values, passing {} here should be safe — but if setParams with reload=true does a second merge/replace cycle, the outcome depends on internal state. Worth a quick sanity check or at minimum replacing with just window.location.reload() to make intent explicit.

web-components/src/main.ts — label formatting nit

rangeEl.innerText = m
  ? `Last ${m[1]} ${units[m[2].toUpperCase()] || m[2]}${parseInt(m[1]) !== 1 ? 's' : ''}`
  : 'Last ' + timeRange.since;

• parseInt(m[1]) !== 1 — since m[1] is already \d+ from the regex, m[1] !== '1' is simpler and avoids the unnecessary parse.
• The fallback 'Last ' + timeRange.since will fire if the AI returns a non-standard format (e.g. "30m" instead of "30M"). The regex is case-insensitive but only matches [SMHD] — lowercase is covered. The main risk is the AI returning something outside this pattern entirely (e.g. "2 hours"). Consider constraining the AI output format more tightly in the system prompt, or expanding the fallback handling.

src/Pages/Charts/Charts.hs — cache fallback silent overhead

The empty-after-trim → fresh query fallback is logically correct for the stated bug. The one concern: if a time range genuinely has no data, trimToRange will return empty and you'll always do a fresh DB query, even though the cache correctly said "no data here". This is a tradeoff worth acknowledging — either document it or add a secondary check (e.g. confirm the cached range actually covers the requested range before concluding the trim result is anomalous).

Minor

• src/Pages/Replay.hs: Removing pure $ ...\npure legacyEvents → direct return is a nice simplification. ✅
• src/Pages/Settings.hs: label_ + for_ for modal trigger is the right fix. ✅
• src/Pages/LogExplorer/Log.hs: Combining both field parses into one parseMaybe with applicative style is cleaner than two separate calls. ✅

Summary

test comment - please ignore

hello world - test 2

PR Review test

PR Review

Overall this is a clean, well-scoped change. A few issues worth addressing.

src/Pkg/AI.hs - buildSystemPrompt / whenJust'

Bug: whenJust' guard is inverted - timezone injected in wrong branch

whenJust' appends timezoneSection only when systemPromptOverride is set (Just _). But systemPrompt now config.timezone already embeds the timezone in the default (no-override) case. When an override is present, the timezone should be appended separately - but the guard fires on exactly the wrong branch.

The helper is also unnecessary - maybe x (const timezoneSection) config.systemPromptOverride or foldMap (const timezoneSection) config.systemPromptOverride expresses this inline.

Cleaner fix: drop the timezoneM param from systemPrompt, always append timezone in buildSystemPrompt:

systemPrompt :: UTCTime -> Text

buildSystemPrompt config now =
  let basePrompt      = fromMaybe (systemPrompt now) config.systemPromptOverride
      timezoneSection = newlinePrefixedTimezone config.timezone now
      facetSection    = formatFacetContext config.facetContext
      customSection   = fromMaybe mempty config.customContext
   in basePrompt <> timezoneSection <> facetSection <> customSection

This removes the helper, the duplicate embedding, and the inverted guard in one stroke.

src/Pages/LogExplorer/Log.hs - parsing style

Minor: liftA2 reads more naturally than the explicit (,) <$> ... <*> ...:

AET.parseMaybe (AE.withObject request backslasho -> liftA2 (,) (o .: input) (o .:? timezone)) requestBody

web-components/src/main.ts - updateTimePicker: opts.label ignored for absolute ranges

When timeRange.from / timeRange.to are set, opts?.label is silently dropped and the label is always derived from the ISO strings. If the AI ever returns a human-readable label alongside an absolute range it will be lost. Suggest:

if (rangeEl) rangeEl.innerText = opts?.label ?? (formatLocal(timeRange.from) + separator + formatLocal(timeRange.to));

Where separator is ' - '.

src/Pages/Charts/Charts.hs - fallback pattern duplicated

The same guard appears in both CacheHit and PartialHit. Consider a local helper or bool:

bool (pure result) (executeQueryWith sqlQueryCfg queryAST)
     (V.null result.dataset && not (V.null cached.dataset))

src/Pages/Replay.hs - good cleanup

Removing the redundant pure $ ... legacyEvents is a correct simplification.

Security: prompt injection via timezone

The timezone string from the client is interpolated directly into the LLM system prompt without sanitisation. An adversarial client could send a timezone value containing newlines followed by injected instructions. At minimum strip newlines before interpolating, or validate against a known IANA zone list (the tz package provides one).

Summary

PR Review

Overall this is a clean, well-scoped change. A few issues worth addressing.

src/Pkg/AI.hs — buildSystemPrompt / whenJust'

Bug: whenJust' guard is inverted — timezone injected in wrong branch

whenJust' appends timezoneSection only when systemPromptOverride is set (Just _). But systemPrompt now config.timezone already embeds the timezone in the default (no-override) case. When an override is present, the timezone should be appended separately — but the guard fires on exactly the wrong branch.

The helper is also unnecessary — maybe x (const timezoneSection) config.systemPromptOverride or foldMap (const timezoneSection) config.systemPromptOverride expresses this inline.

Cleaner fix: drop the timezoneM param from systemPrompt, always append timezone in buildSystemPrompt:

systemPrompt :: UTCTime -> Text   -- restore original signature

buildSystemPrompt config now =
  let basePrompt      = fromMaybe (systemPrompt now) config.systemPromptOverride
      timezoneSection = "\nUSER TIMEZONE: " <> fromMaybe "UTC" config.timezone
                      <> "\nCURRENT TIME (UTC): " <> show now <> "\n"
      facetSection    = formatFacetContext config.facetContext
      customSection   = fromMaybe "" config.customContext
   in basePrompt <> timezoneSection <> facetSection <> customSection

This removes the helper, the duplicate embedding, and the inverted guard in one stroke.

src/Pages/LogExplorer/Log.hs — parsing style

Minor: liftA2 reads more naturally than the explicit (,) <$> ... <*> ...:

AET.parseMaybe (AE.withObject "request" \o -> liftA2 (,) (o .: "input") (o .:? "timezone")) requestBody

web-components/src/main.ts — updateTimePicker: opts.label ignored for absolute ranges

When timeRange.from / timeRange.to are set, opts?.label is silently dropped and the label is always derived from the ISO strings. If the AI ever returns a human-readable label alongside an absolute range it will be lost. Suggest:

if (rangeEl) rangeEl.innerText = opts?.label ?? (formatLocal(timeRange.from) + ' \u2013 ' + formatLocal(timeRange.to));

src/Pages/Charts/Charts.hs — fallback pattern duplicated

The same guard appears in both CacheHit and PartialHit. Consider a local helper or bool:

bool (pure result) (executeQueryWith sqlQueryCfg queryAST)
     (V.null result.dataset && not (V.null cached.dataset))

src/Pages/Replay.hs — good cleanup

Removing the redundant pure $ ... legacyEvents is a correct simplification.

Security: prompt injection via timezone

The timezone string from the client is interpolated directly into the LLM system prompt without sanitisation. An adversarial client could send "timezone": "UTC\n\nIGNORE ALL PREVIOUS INSTRUCTIONS...". At minimum strip newlines before interpolating, or validate against a known IANA zone list (the tz package provides one).

Summary

PR Review: AI search time picker integration

Overall this is a clean, well-scoped fix. A few observations:

src/Pages/LogExplorer/Log.hs

Parsing tuple inline — minor style nit:

let parsed = AET.parseMaybe (AE.withObject "request" \o -> (,) <$> o AE..: "input" <*> o AE..:? "timezone") requestBody

With RecordWildCards / OverloadedRecordDot already in scope and BlockArguments enabled, a small named record or a local do block reads better here. Also (,) applied pointfree is fine, but the explicit lambda and <*> chain is equivalent to:

AET.parseMaybe (AE.withObject "request" \o -> liftA2 (,) (o AE..: "input") (o AE..:? "timezone")) requestBody

Not a blocker, just a readability note.

src/Pkg/AI.hs

systemPrompt signature change:

systemPrompt :: UTCTime -> Maybe Text -> Text
systemPrompt now timezoneM = ...
    , "USER TIMEZONE: " <> fromMaybe "UTC" timezoneM

fromMaybe "UTC" timezoneM is fine, but note there is no IANA timezone validation on the Haskell side — a client can send any arbitrary string. Since this goes directly into the LLM system prompt, a malformed or adversarial value (prompt injection via the timezone field) is a low-severity but real concern. Consider at minimum stripping newlines: T.replace "\n" " " (fromMaybe "UTC" timezoneM).

AgenticConfig record default:
The new field timezone :: Maybe Text is correctly defaulted to Nothing in defaultAgenticConfig. Good.

src/Pages/Charts/Charts.hs

The cache fallback logic is correct in spirit, but the condition is repeated twice with slight differences. The CacheHit guard is:

V.null trimmed.dataset && not (V.null entry.cachedData.dataset)
  && not (entry.cachedFrom <= reqFrom && entry.cachedTo >= reqTo)

and PartialHit uses:

V.null result.dataset && not (V.null trimmed.dataset)
  && not (slidingWindowStart <= reqFrom)

The third condition in PartialHit (not (slidingWindowStart <= reqFrom)) seems to be checking a different thing than the CacheHit version — is slidingWindowStart <= reqFrom really the right proxy for "cache range covers request"? If not it's a subtle bug. Worth a comment or extracting to a named predicate (cacheRangeCoversRequest) to make the intent explicit and keep the two call-sites aligned.

src/Pkg/Components/TimePicker.hs

The updateTimePicker call in hyperscript now passes {since: @data-value}:

[text|on click call window.updateTimePicker({since: @data-value}, {targetPr: '${targetPr}', label: @data-title}) then $action then call $popoverId.hidePopover()|]

Previously, clicking a preset also set #$targetPr-custom_range_input's value directly in the hyperscript. Now that's delegated to updateTimePicker. This is the right direction — less duplication. Just verify that the inputEl path in updateTimePicker always finds the element for every targetPr variant in use (e.g. the replay page's picker uses a different prefix).

The old custom date-range JS now does:

window.setParams({}, true);

Previously it called setParams({from:..., to:..., since:''}, true). If setParams with an empty object doesn't clear from/to/since, the URL could get stale params. Confirm setParams({}, true) behaves identically to passing explicit empty values when true (submit) is the second arg.

web-components/src/main.ts

Label auto-generation for since values:

const units: Record<string, string> = { S: 'Second', M: 'Minute', H: 'Hour', D: 'Day' };
const m = timeRange.since.match(/^(\d+)\s*([SMHD])$/i);
rangeEl.innerText = m
  ? `Last ${m[1]} ${units[m[2].toUpperCase()] || m[2]}${m[1] !== '1' ? 's' : ''}`
  : 'Last ' + timeRange.since;

This is reasonable. One edge case: m[1] is a string ("1"), so m[1] !== '1' works for the plural check, but m[2].toUpperCase() may not be in units for values like W (week) if the backend ever emits those. The fallback || m[2] handles it gracefully, so this is low-risk.

No from/to branch sets inputEl.value for the custom range display — it sets it to timeRange.from + '/' + timeRange.to. If the existing custom range picker parses inputEl.value as a /-separated pair, this is fine; just confirm that format is what the date picker widget expects.

src/Pkg/Components/LogQueryBox.hs

The hyperscript on htmx:afterRequest change is correct. One subtlety:

if :result.query then call #filterElement.handleAddQuery(:result.query, true)
else if :result.time_range then trigger submit on #log_explorer_form end

If both query and time_range are present, updateTimePicker is called but the form is not re-submitted (the else if is only reached when query is falsy). Is that intentional? If the AI returns both a KQL query and a time range, the query gets applied but the time range change does not trigger a fresh fetch. The handleAddQuery with true presumably triggers a submit itself — confirm that path also picks up the updated time params.

src/Pages/Settings.hs

Moving the modal trigger label inside a label_ with for_ is the correct fix for the duplicate-trigger issue.

src/Pages/Replay.hs

-- Before
legacyEvents <- getMinioFile ...
pure legacyEvents
-- After
getMinioFile ...

Good cleanup — pure η-reduction.

Summary

The core approach — moving time range concerns out of KQL and into the time picker UI — is the right design. The TypeScript updateTimePicker centralisation is a net improvement.

Code Review

Overall this is a clean, well-motivated PR. A few targeted observations:

src/Pages/Charts/Charts.hs — fallbackIfEmpty

Positive: The bool trick is idiomatic. Minor issues:

1. Naming vs. semantics. fallbackIfEmpty implies it falls back when the result is empty, but the guard coversRange || not (V.null result.dataset) || V.null cached.dataset also avoids a re-fetch when the cache is empty. Something like refetchUnlessAdequate would be less surprising.

2. Possible logic gap — third clause. V.null cached.dataset short-circuits to "do not re-fetch" when the cached data is empty. On a CacheHit/PartialHit path an empty cache usually means re-fetching is exactly what we want. Verify this is intentional — it could silently return an empty result when the data source has data.

3. Style: guards over bool. With the MultiWayIf/BlockArguments extensions already enabled, guards are more readable than bool (f) (pure result) condition (which requires mentally flipping the boolean):

   fallbackIfEmpty coversRange cached result
     | coversRange || not (V.null result.dataset) || V.null cached.dataset = pure result
     | otherwise = executeQueryWith sqlQueryCfg queryAST

src/Pkg/AI.hs — orphaned comment

"CURRENT TIME (UTC): " <> show now was removed from systemPrompt but the line immediately below still reads:

"Use this to interpret relative time requests (e.g., 'last 2 hours' → {\"since\": \"2H\"})"

"this" no longer refers to anything. Remove or update the comment.

src/Pages/Settings.hs — modal trigger

The pass removal and switch to a label_ outside modalWith_ is cleaner. Verify the for_/id wiring matches the id generated internally by modalWith_ for "pricing-modal" — if it uses a DaisyUI checkbox toggle the label_ approach works, but if it uses the Popover API the for_ attribute has no effect without an <input type="checkbox"> target.

web-components/src/main.ts — updateTimePicker

1. Silent no-op on malformed input. If timeRange has neither since nor from+to, nothing happens and callers get no feedback. A console.warn would help debugging.

2. Incomplete units map. Only S/M/H/D are covered. If the AI ever returns a unit like W (weeks) the label will read "Last 2 W" with no pluralisation. Extend the map or document the constraint.

3. Double param-write risk. For the time_range-only path (no query), the Hyperscript in LogQueryBox.hs triggers submit on #log_explorer_form. Confirm that the form submit handler does not also call setParams, which would duplicate the write already made by updateTimePicker.

src/Pkg/Components/TimePicker.hs

Positive: Centralising label/input/param updates in updateTimePicker is a clear improvement over the previous scattered inline Hyperscript mutations.

Nit: The non-form submitAction calls window.setParams({}, true) (empty object + reload). This works because updateTimePicker already set the params, but it is non-obvious. A short comment would help future readers.

src/Pages/Replay.hs

Redundant bind→pure elimination is correct and clean.

Security — prompt injection via timezone

AgenticConfig.timezone :: Maybe Text is sourced from client JS (Intl.DateTimeFormat().resolvedOptions().timeZone), which a user can spoof, and is interpolated verbatim into the LLM system prompt in buildSystemPrompt. At minimum, strip newlines/control characters before interpolation, or validate against a known IANA timezone allowlist.

Code Review

Overall this is a solid, well-scoped fix. The separation of time-range handling out of KQL and into a dedicated UI mechanism is the right architectural move. A few observations:

src/Pages/Charts/Charts.hs — refetchUnlessAdequate

Style nit — redundant let binding in PartialHit:

let result = QC.trimToRange trimmed reqFrom reqTo
refetchUnlessAdequate (slidingWindowStart <= reqFrom) trimmed result

result is only used once immediately after; it can be inlined to reduce visual noise:

refetchUnlessAdequate (slidingWindowStart <= reqFrom) trimmed (QC.trimToRange trimmed reqFrom reqTo)

Guard logic readability: The guard

| coversRange || not (V.null result.dataset) || V.null cached.dataset = pure result

encodes three distinct short-circuit cases. Reordering to the most obvious case first (V.null cached.dataset || coversRange || not (V.null result.dataset)) or adding a brief comment would aid future readers.

src/Pages/LogExplorer/Log.hs

Good use of liftA2 for the dual parse — clean and idiomatic.

null vs absent time_range: When resp.timeRange is Nothing, "time_range" AE..= resp.timeRange serializes to "time_range": null. The current hyperscript if :result.time_range is falsy for both null and undefined, so it works — but it may be cleaner to omit the key entirely when absent (via AE.omitNothingFields on a record, or building the object conditionally).

src/Pkg/AI.hs

systemPrompt as a constant — good simplification, removes the UTCTime threading.

systemPromptOverride + timezoneSection: timezoneSection is now always appended after basePrompt, including when systemPromptOverride is set. Worth verifying that callers using systemPromptOverride (e.g., issue investigation) either want this timezone/time context, or are unaffected by it.

Timezone injection into LLM prompt: The value comes from the browser's Intl.DateTimeFormat().resolvedOptions().timeZone unvalidated. Low risk for an LLM prompt, but worth noting.

src/Pkg/Components/TimePicker.hs

Good reduction of duplicated inline JS. The new onClickHandler is meaningfully shorter.

submitAction reload logic for non-form case:

"window.setParams({}, true); document.getElementById(`$targetPr-timepicker-popover`).hidePopover();"

window.setParams({}, true) with an empty object reloads with no param changes — the comment explains that updateTimePicker already set the params internally. This is fragile. window.location.reload() would make the intent explicit, or at minimum the comment should be inline with the code rather than above it.

web-components/src/main.ts — window.updateTimePicker

Default targetPr = 'n': The fallback to 'n' when no opts.targetPr is supplied is implicit. A comment explaining this (it's the DOM id prefix for the default time picker instance) would help.

Unit map coverage: The regex ^(\d+)\s*([SMHD])$ covers seconds/minutes/hours/days. If the AI ever returns a week-level since value (e.g. 1W), it falls through to 'Last 1W' raw. Consider adding W: 'Week' or documenting the supported set.

src/Pages/Settings.hs

Clean fix — label_ with for_ pointing to the modal checkbox is the correct DaisyUI pattern. Removing the pass placeholder is tidy.

src/Pages/Replay.hs

Removing the redundant pure legacyEvents wrapper is correct and idiomatic.

Summary