Skip to content

feat(frontend): add runtime-configurable loading tips with branded dots#817

Open
jeremyeder wants to merge 2 commits intomainfrom
feat/loading-tips-ui
Open

feat(frontend): add runtime-configurable loading tips with branded dots#817
jeremyeder wants to merge 2 commits intomainfrom
feat/loading-tips-ui

Conversation

@jeremyeder
Copy link
Contributor

Summary

  • Replace joke loading messages with educational tips about ACP features
  • Add 4 branded loading dots (#0066B1, #522DAE, #F40000, white) at 2x size
  • Make tips runtime-configurable via LOADING_TIPS environment variable
  • Add markdown link support in tips using [text](url) syntax

Changes

  • New API endpoint: /api/config/loading-tips serves tips from env var with fallback
  • New hook: useLoadingTips() with React Query caching (staleTime: Infinity)
  • Shared constants: lib/loading-tips.ts for default tips
  • Updated LoadingDots: 4 colored dots, markdown link parsing, improved contrast

Configuration

Set LOADING_TIPS env var as JSON array:

LOADING_TIPS='["Tip: First tip", "Tip: Second tip", "[Link text](https://example.com)"]'

Or use a ConfigMap (example in artifacts folder).

Test plan

  • Verify loading dots appear during AI response generation
  • Verify tips rotate every 8 seconds
  • Test with LOADING_TIPS env var set to custom JSON
  • Test markdown links render as clickable
  • Verify fallback to defaults when env var not set

🤖 Generated with Claude Code

Replace joke loading messages with educational tips about ACP features.
Tips are now runtime-configurable via LOADING_TIPS environment variable,
allowing updates without code deployment.

Changes:
- Add 4 branded loading dots (#0066B1, #522DAE, #F40000, white)
- Double dot size for better visibility
- Add markdown link support in tips [text](url)
- Add /api/config/loading-tips endpoint
- Add React Query hook with session-long caching
- Extract defaults to shared lib/loading-tips.ts

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions

This comment has been minimized.

- Add URL scheme validation in parseMarkdownLinks to prevent javascript: injection
- Reset messageIndex when tips.length changes to prevent undefined access
- Increase white dot stroke contrast (#E0E0E0 -> #9CA3AF) for light backgrounds
- Add gcTime: Infinity to prevent cache garbage collection
- Namespace query key as ['config', 'loading-tips']

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

test

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

Claude Code Review

Summary

PR 817 adds runtime-configurable loading tips to replace static joke messages in the LoadingDots component. The changes introduce a Next.js API route, a React Query hook, an API client function, and default tip constants — all following established architectural patterns well. One runtime crash bug exists, and the branded SVG colors introduce theming concerns.


Issues by Severity

Blocker Issues

None.


Critical Issues

1. Runtime TypeError when custom tips array is shorter than defaults

File: components/frontend/src/components/ui/message.tsx (~lines 162–176, 234)

When useLoadingTips() resolves with a custom array that has fewer items than DEFAULT_LOADING_TIPS (10 items), there is a window between the render that picks up the new tips array and the effect that resets messageIndex where tips[messageIndex] evaluates to undefined. This is then passed as the text: string argument to parseMarkdownLinks. Inside that function, text.length is accessed unconditionally after the regex loop — this throws a TypeError since text is undefined at runtime.

Trace:

  1. Mount: messageIndex = 7 (against 10-item defaults)
  2. Query resolves with 3-item custom array. Re-render: tips has 3 items, messageIndex still 7
  3. tips[7]undefined. parseMarkdownLinks(undefined) is called. undefined.length throws TypeError
  4. Effect fires next tick and sets messageIndex = 7 % 3 = 1 — too late

Suggested fix:

// 1. Defensive guard in parseMarkdownLinks
function parseMarkdownLinks(text: string): React.ReactNode {
  if (!text) return null;
  // rest unchanged
}

// 2. Initialize from DEFAULT_LOADING_TIPS length (always safe), re-sync on change
const [messageIndex, setMessageIndex] = React.useState(
  () => Math.floor(Math.random() * DEFAULT_LOADING_TIPS.length)
);

React.useEffect(() => {
  if (tips.length > 0) {
    setMessageIndex((prev) => prev % tips.length);
  }
}, [tips.length]);

Major Issues

1. Hardcoded hex colors in SVG break theming

File: components/frontend/src/components/ui/message.tsx (~lines 205–232)

The four dots use raw hex values (#0066B1, #522DAE, #F40000, #FFFFFF) not connected to the design token system. The project uses Tailwind CSS for all styling; these values will not adapt to future theme changes.

Suggested fix: Define them as named brand colors in tailwind.config and reference via CSS custom properties or Tailwind utilities:

// tailwind.config: theme.extend.colors.brand = { blue: '#0066B1', purple: '#522DAE', red: '#F40000' }
<circle fill="var(--brand-blue)" ... />

2. White dot is invisible on light backgrounds

File: components/frontend/src/components/ui/message.tsx (~lines 228–232)

The fourth dot has fill="#FFFFFF" with only a 1px stroke="#9CA3AF" for contrast. On a white or near-white background (default light mode) this dot is invisible — users see only 3 animated dots instead of 4.

Suggested fix:

<circle
  className="loading-dot loading-dot-4"
  cx="50" cy="8" r="6"
  fill="var(--muted-foreground)"
/>

Minor Issues

1. Excessive JSDoc on straightforward functions

Files: components/frontend/src/services/api/config.ts (lines 5–7, 12–15), components/frontend/src/services/queries/use-loading-tips.ts (lines 4–6, 9–11)

Per project convention, only add comments where logic is not self-evident. The names getLoadingTips and useLoadingTips are self-descriptive. The inline comment on staleTime: Infinity is genuinely useful and should be kept.

2. URL validation in parseMarkdownLinks could use new URL()

File: components/frontend/src/components/ui/message.tsx (~lines 125–130)

The current scheme-prefix check correctly blocks javascript: and data: URIs. Using new URL() would also reject structurally malformed URLs that happen to start with https://:

const isSafeUrl = (() => {
  try {
    const u = new URL(href);
    return u.protocol === 'https:' || u.protocol === 'http:';
  } catch {
    return false;
  }
})();

Positive Highlights

  • Correct React Query architecture. The new hook cleanly separates API client (services/api/config.ts) from query hook (services/queries/use-loading-tips.ts), exactly matching the established pattern. staleTime: Infinity is the right choice for near-immutable configuration data.

  • Solid XSS defense in parseMarkdownLinks. Validating the URL scheme before rendering it as an href prevents javascript: injection from admin-configured tips. rel="noopener noreferrer" on external links is correct.

  • Graceful server-side validation in route.ts. The env var is validated as Array.isArray, non-empty, and every element is a string before acceptance, with a safe fallback on any parse failure.

  • Zero any types. All new TypeScript (LoadingTipsResponse, array types, React.ReactNode return) is properly typed — fully compliant with the zero-any rule.

  • Client-side fallback pattern. data?.tips ?? DEFAULT_LOADING_TIPS ensures tips are always shown even while the API call is in-flight or fails.


Recommendations (priority order)

  1. (Critical — fix before merge) Add null guard in parseMarkdownLinks and clamp messageIndex initialization to prevent transient out-of-bounds TypeError when custom tips array is shorter than defaults.
  2. (Major — fix before merge) Fix the white fourth dot light-mode visibility — replace fill="#FFFFFF" with a theme-adaptive color.
  3. (Major — consider before merge) Move hardcoded hex values into Tailwind named colors or CSS custom properties.
  4. (Minor — follow-up OK) Remove redundant JSDoc blocks from config.ts and use-loading-tips.ts.
  5. (Minor — follow-up OK) Upgrade URL validation in parseMarkdownLinks to use new URL() for structural correctness.

Reviewed by Claude Sonnet 4.6 against ambient-code/platform standards.


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

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