Skip to content

Distill and normalize UI to match design system tokens#334

Open
tonyalaribe wants to merge 8 commits intomasterfrom
ui/distill-and-normalize
Open

Distill and normalize UI to match design system tokens#334
tonyalaribe wants to merge 8 commits intomasterfrom
ui/distill-and-normalize

Conversation

@tonyalaribe
Copy link
Contributor

  • Flatten surface-raised from gradient+shadows to bg-fillWeaker border
  • Remove surface-table box-shadow, compact empty states
  • Replace bg-base-100 with bg-bgOverlay across all pages
  • Normalize shadows: shadow-2xl → shadow-lg, bare shadow → shadow-sm
  • Replace raw colors (bg-red-500, text-red-500, etc.) with semantic tokens
  • Standardize font weights: font-bold → font-semibold on headings
  • Replace hand-rolled buttons with DaisyUI btn variants
  • Replace LogQueryBox gradient button with btn-primary
  • Lighten search bar: transparent bg, strokeWeak border, h-9

- Flatten surface-raised from gradient+shadows to bg-fillWeaker border
- Remove surface-table box-shadow, compact empty states
- Replace bg-base-100 with bg-bgOverlay across all pages
- Normalize shadows: shadow-2xl → shadow-lg, bare shadow → shadow-sm
- Replace raw colors (bg-red-500, text-red-500, etc.) with semantic tokens
- Standardize font weights: font-bold → font-semibold on headings
- Replace hand-rolled buttons with DaisyUI btn variants
- Replace LogQueryBox gradient button with btn-primary
- Lighten search bar: transparent bg, strokeWeak border, h-9
@blacksmith-sh
Copy link
Contributor

blacksmith-sh bot commented Mar 8, 2026

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
Pages.Anomalies, Check Anomaly List, should acknowledge endpoint anomaly View Logs
Pages.Projects.Integrations, Notification Testing, Project-Level Tests, sends test noti
fication to Slack and records history
View Logs

Fix in Cursor

@claude
Copy link

claude bot commented Mar 8, 2026

PR Review: Distill and normalize UI to match design system tokens

Overall: clean, well-scoped refactoring. The changes are consistent and the direction is right. A few things worth flagging:


Concerns

1. Widget.hs - semantic color loss on stat values

Removes text-textSuccess-strong in favour of generic text-textStrong on stat widget value displays. Stat values showing normal/good state presumably benefited from the success colour. If intentional (stat values should always be neutral), fine - but worth noting since it is a visual semantic change not listed in the PR description.

The tabular-nums addition is a nice improvement for number alignment.

2. Settings.hs - undocumented spacing changes mixed into button refactor

ml-3 to ml-2 and ml-6 to ml-4 are unrelated spacing tweaks embedded in the button replacement. Not a problem, but invisible in the PR description and could cause surprise in a bisect.

3. Components.hs - silent removal of font-medium on empty state subtext

The p_ subtext in emptyState_ drops font-medium. Intentional weight reduction? Fine if so, just not mentioned.

4. Utils.hs - pre-existing duplicate tabindex_ (not introduced here)

The context-menu ul_ has both tabindex_ -1 and tabindex_ 0 on the same element. Not this PR's fault, but since the line is touched it is a good opportunity to clean it up.


Positives

  • Typo fix (Acknowleged to Acknowledged)
  • Consistent replacement of bg-base-100 with bg-bgOverlay and raw Tailwind colors (bg-red-500, text-yellow-600) with semantic tokens across all files
  • btn btn-sm btn-* replacements cut a lot of hand-rolled focus ring / padding boilerplate in Settings.hs
  • Shadow CSS custom properties (--shadow-xs/sm/md/lg) unify elevation; using var(--shadow-toast) and var(--shadow-lg) in CSS is cleaner
  • oklch migration is perceptually consistent and future-proof; the warm-neutral palette shift is subtle but coherent
  • surface-raised simplification (gradient + multi-shadow to flat bg-fillWeaker + border) is a significant reduction in CSS complexity

Add sidebar-specific design tokens (bgSidebar, fillSidebar-hover/active,
strokeSidebar, textSidebar/strong) with light mode using a subtle cool
tint and dark mode using a deeper surface. Update all sidebar elements
in BodyWrapper to use the new tokens.
@claude
Copy link

claude bot commented Mar 8, 2026

PR Review

Overall this is clean, well-scoped normalization work. The token migration is consistent and the shadow scale system is a clear improvement. A few things worth flagging:

Potential Bug — ECharts & oklch in log-list.ts

color: 'oklch(48% 0.205 265 / 0.2)',
borderColor: 'oklch(48% 0.205 265)',

ECharts typically parses colors via canvas 2D context or its own color parser. oklch is not yet supported in ECharts' color parser and canvas fillStyle oklch support is browser-dependent (Chrome 111+, Firefox 113+, but no Safari canvas support until 16.4+). This could silently fall back to black or show nothing at all in the mark area. The previous rgba(0, 104, 255, .2) / rgb(0 104 255) was safer here. Consider either keeping rgb for ECharts or converting at use-site with a CSS variable resolved at runtime.

Similarly pulseIndicator keyframe uses oklch — but this is pure CSS inside a LitElement css tagged template, so it goes through the browser's CSS engine and is fine.

Breaking URL Change — Acknowledged typo fix in Anomalies.hs

- Just "Acknowleged" -> (Just True, Nothing, "Acknowleged")
+ Just "Acknowledged" -> (Just True, Nothing, "Acknowledged")

The two sides (match + TabFilterOpt) are updated consistently so the UI round-trip works. But the old spelling was part of the URL query string (?filterT=Acknowleged). Any bookmarked links, shared URLs, or external integrations using the old misspelling will silently fall through to the Inbox default. Low risk but worth a heads-up.

Design Semantic — Logout loses destructive styling

- bg-fillError-weak text-iconError
+ bg-fillSidebar-active text-textSidebar

Previously logout had a red error affordance signalling it as a destructive action. Now it matches all other sidebar items. Intentional flatten? Fine if so, just worth confirming this is deliberate per design spec.

Minor — stat value color regression

- text-textSuccess-strong
+ text-textStrong

Stat placeholder and stat value previously rendered in textSuccess-strong (green). Now they're neutral textStrong. Was textSuccess-strong an existing semantic mistake (all stats green regardless of value) being fixed here, or should success/warning/error colouring be conditional on the value's meaning? If it was always green by mistake, this is a good fix — just making sure it's intentional.

Positive callouts

  • Shadow scale tokenization (--shadow-xs/sm/md/lg) is a clean system — eliminates scattered magic values throughout CSS.
  • DaisyUI btn btn-sm btn-success in Settings.hs replacing 200+ chars of hand-rolled classes is exactly right.
  • tabular-nums on stat values is a solid UX improvement.
  • border-black-30 → border-strokeWeak in Table.hs removes the only remaining raw Tailwind arbitrary-ish class in that file.
  • text-base-content/60 → text-textWeak in TimePicker.hs removes the last DaisyUI opacity-shorthand leaking through the token layer.

@claude
Copy link

claude bot commented Mar 9, 2026

PR Review: Distill and normalize UI to match design system tokens

Overall this is a clean, well-scoped refactor — net -31 lines, consistent direction, and good use of semantic tokens over raw values. A few things to flag:


Bug: oklch in ECharts context (web-components/src/log-list.ts)

// Changed to:
color: 'oklch(48% 0.205 265 / 0.2)',
borderColor: 'oklch(48% 0.205 265)',

ECharts parses colors through its own internal engine (zrender), which does not support the oklch() CSS color syntax. This will silently fall back to no color / transparent for the mark area highlight and pulse indicator animation. The previous rgba(0, 104, 255, …) and rgb(0, 104, 255) values worked because ECharts supports legacy RGB. You'll want to keep hex or rgba() here, or derive the resolved value via getComputedStyle at runtime if you want it to track the CSS variable.

Similarly, the @keyframes pulseIndicator block inside the component's shadow DOM:

background-color: oklch(48% 0.205 265);

This is injected as a string into a <style> tag — it'll work in modern browsers but is worth noting for the same reason the ECharts case isn't: the animation already lives in the light theme CSS, so having two sources of truth for the colour could diverge.


Pre-existing but now more visible: double tabindex_ (src/Utils.hs)

The context-menu ul_ element still has both tabindex_ "-1" and tabindex_ "0" on the same node (unchanged by this PR, just surfaced). One of them should be removed — the second wins but it's confusing.


Minor: surface-raised hover transition silently removed

The transition: transform 150ms ease-out, box-shadow 150ms ease-out and the @media (hover: hover) lift were deleted. This is fine for the "flatten" goal, but worth a quick visual sweep of any card grids that relied on that lift for affordance feedback.


Nits (no action needed)

  • --backgroundPrimary shifting from rgb(255 255 255) to oklch(98.5% 0.002 260) (off-white) is intentional but subtle — worth checking against any surfaces that assumed a pure-white backdrop.
  • text-textSuccess-strongtext-textStrong in Widget.hs looks like a bug fix (no such token existed); good catch.
  • Typo fix (AcknowlegedAcknowledged) is a welcome cleanup.

Summary: The main actionable item is the ECharts oklch color issue in log-list.ts — the rest is clean. Would suggest converting those two colour strings back to rgba()/hex.

Convert all light mode RGB/RGBA color tokens to perceptually equivalent
OKLCH values while preserving the original production appearance.
Dark mode changes are kept as-is.
@claude
Copy link

claude bot commented Mar 9, 2026

Additional note: shadow variable mismatch in .widget-drawer-stuck

Before: box-shadow: 0 2px 8px -2px rgba(0, 0, 0, 0.08)
After: box-shadow: var(--shadow-xs) -- which is 0 1px 2px -1px rgba(0 0 0 / 0.06)

The original value (0 2px 8px -2px rgba(0,0,0,0.08)) is exactly the first layer of --shadow-sm, not --shadow-xs. This should be var(--shadow-sm) -- shadow-xs is noticeably lighter and produces a weaker sticky header indicator than the current behaviour.

tonyalaribe and others added 2 commits March 9, 2026 14:47
…lish

- Fix empty investigation panel by removing stale time hint from getTraceDetails
  (updatedAt drifted far from actual trace timestamp, causing ±5min window miss)
- Fix activity 500 error: use explicit column list for users query to match
  Haskell record field order (is_sudo/phone_number column order mismatch)
- Fix page layout: left column scrolls independently with space-y-4 block flow
  instead of flex-col (prevents flex shrinking), AI chat stays fixed via h-full
- Add clock-rotate-left icon to FA regular sprite sheet
- Apply highlight.js syntax highlighting to stack traces
- Hide Crisp chat widget on anomaly detail page
- Normalize section headers to consistent xs/semibold/uppercase style
- Add tabular-nums to timestamp and count columns in monitors/dashboards
@claude
Copy link

claude bot commented Mar 9, 2026

PR Review

Overall a clean, well-scoped design-system normalization. Good work removing raw colors in favour of semantic tokens and trimming the over-engineered surface-raised gradient/shadow machinery.


Potential Bug: OKLCH in ECharts canvas context (web-components/src/log-list.ts)

ECharts renders to canvas and passes color strings directly to the Canvas 2D API (ctx.fillStyle, etc.). Canvas color parsing tracks CSS Color Level 4 in modern browsers, but ECharts itself may pre-process color strings internally and fall back to black/transparent for formats it does not recognise. The new OKLCH values (oklch(48% 0.205 265 / 0.2)) need a real browser + ECharts version test. rgb()/hex is the safe choice for chart colors going through a JS charting library's internal pipeline.


Missing padding on stack trace (src/Pages/Anomalies.hs)

p-4 was dropped from the wrapper div around the stack trace pre, so the block is now flush against the card edge. Intentional, or a slip?

Also, code_ [] has no class. Highlight.js auto-detection fires on every unlabelled block, which can be slow or incorrect. A language-* class (or nohighlight) makes intent explicit.


SQL explicit column list is fragile (src/Pages/Anomalies.hs)

The explicit SELECT id, created_at, ..., phone_number, is_sudo fixes the current column-order mismatch with the Haskell record, but will silently break again if a future migration adds a field in a different position. A brief comment explaining why the list is explicit would help future maintainers. Worth considering a typed RowParser approach for User to make order-independence explicit.


Inconsistent OKLCH conversion in CSS

Light mode is thoroughly converted to OKLCH, but dark mode retains rgba() for several semantic groups (error/success/warning/information stroke and icon colors). The PR description says dark mode changes are kept as-is, yet some dark-mode brand tokens are converted. Fine to defer — just worth noting so the follow-up pass knows the scope.


Nits

  • detailItem tuple grew from (Text, Text, Text) to (Text, Text, Text, Text). A small record or type alias would self-document (icon, iconColor, lbl, value) at call sites — minor since it is a local helper, but positional 4-tuples are easy to misorder.
  • _ <- div_ in logPatternCards looks like an unintentional discard (may be pre-existing). Plain div_ ... is cleaner.
  • Double space in "space-x-2 border-b" in BodyWrapper.hs — pre-existing but this line was touched.

What is good

  • Removing the surface-raised gradient and multi-layer shadow in favour of flat bg-fillWeaker border is the right call — simpler CSS, less visual noise.
  • Shadow scale via CSS custom properties (--shadow-xs/sm/md/lg) with dark-mode overrides is clean and DRY.
  • bg-base-100 swept to bg-bgOverlay; raw bg-red-500/bg-yellow-500/bg-green-500 replaced with semantic tokens in monitor status — exactly right.
  • getTraceDetails pid tId Nothing now — dropping the drifted updatedAt time hint is a solid root-cause fix for the empty investigation panel.
  • Explicit column list for issueActivityGetH correctly addresses the record-field order mismatch causing the 500 on the activity endpoint.

tonyalaribe and others added 2 commits March 9, 2026 21:55
- Route path "anomalies" → "issues" across all URL references
- Page title "Anomalies" → "Issues" in navbar and sidebar
- Make breadcrumb pageTitle a link when on detail/sub pages
- Fix prePageTitle links using menu URL instead of hardcoded /dashboards
- Extract colorChip_/metadataChip_ to Pages.Components (shared)
- Replace local getDataset with Widget.toWidgetDataset in Reports
- Centralize hashPrefix and defaultRecommendedAction in Issues model
@claude
Copy link

claude bot commented Mar 9, 2026

test

@claude
Copy link

claude bot commented Mar 9, 2026

PR Review: Distill and normalize UI to match design system tokens

Overall a clean, well-scoped PR. The URL rename, token normalization, and component extractions are all solid. A few things worth flagging:


Potential Bug: Title not rendered when projectM is Nothing

In the updated navbar logic (BodyWrapper.hs):

if targetPage /= "" && isJust pageTitleSuffix
  then whenJust projectM \p -> a_ [...] $ toHtml pageTitle
  else label_ [...] $ toHtml pageTitle

When targetPage is non-empty and pageTitleSuffix is set, but projectM is Nothing, the whenJust silently renders nothing - the page title disappears entirely. The old code had label_ in the else branch which always rendered. Worth checking whether Issues detail pages can ever have projectM = Nothing (seems unlikely but worth guarding against).


Style nit: underscore bind on Html () actions

In logPatternCards (new code in Anomalies.hs):

_ <- div_ [class_ "surface-raised rounded-2xl overflow-hidden"] do

div_ returns HtmlT m (), so _ <- is equivalent to just writing div_ directly. This pattern slipped into new code. Prefer dropping the bind.


Naming: colorChip_ first parameter

colorChip_ :: Monad m => Text -> Text -> Text -> HtmlT m ()
colorChip_ color icon label = ...

The color parameter is actually a full CSS class string (e.g. "text-fillInformation-strong bg-fillInformation-weak"), not just a color token. Consider renaming to extraClasses or cls to avoid confusion at call sites.


Good catches / improvements

  • Widget.toWidgetDataset reuse in Reports.hs: removing the local getDataset copy is the right call.
  • colorChip_ / metadataChip_ extraction to Pages.Components and removing the local redefinition in Testing.hs: good DRY.
  • Explicit columns instead of SELECT * in issueActivityGetH: safer against schema drift.
  • Typo fix "Acknowleged" to "Acknowledged" in both filter keys and UI: this was a latent bug where the filter tab silently fell through to the catch-all case.
  • prePageTitle breadcrumb link fix: old code hardcoded /dashboards regardless of the matched menu item; new code uses the actual URL from the menu tuple. Solid fix.
  • hashPrefix / live widget query replacing the pre-loaded fetchPatternHourlyStats: cleaner, avoids stale data on page load.
  • void replacing _ <- in anomalyBulkActionsPostH: idiomatic.
  • Doctest examples on stripSummaryTokens: good documentation habit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant