Phase 15 Batch A: Settings, Notifications, Profile#359
Merged
JustAGhosT merged 6 commits intodevfrom Mar 11, 2026
Merged
Conversation
FE-008: Enhanced settings page with language selector (en-US/fr-FR/de-DE), Data & Privacy consent toggles (analytics, telemetry, personalized content, third-party sharing), descriptions on all toggles, save confirmation. FE-009: New /settings/notifications page with channel toggles (email, push, SMS, in-app), 5 notification categories with per-category enable/disable, quiet hours with start/end time and timezone. FE-010: New /profile page with account info, role badges (Admin/Analyst/ Viewer), GDPR & EU AI Act consent management (4 consent types), privacy summary with status dots, data export request (GDPR Article 20), session info. Added Profile nav item with User icon to sidebar. Store: Extended usePreferencesStore with language, privacyConsent, and notificationPreferences state + actions (setLanguage, setPrivacyConsent, setNotificationChannel, setQuietHours). Build: 14 pages generated (was 12), 0 TypeScript errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughAdds multiple new frontend dashboards and pages (Profile, Notification Preferences, Balance/Impact/Value/Sandwich), expands preferences store with language/privacy/notification/GDPR state/actions, introduces toggle UI primitives, widget APIs/types, Kubernetes/Terraform/frontend deployment artifacts, and several focused UI/type/accessibility and backend service fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant UI as "ProfilePage / NotificationPage"
participant Store as "usePreferencesStore"
participant API as "Backend GDPR API"
User->>UI: Toggle consent / notification setting
UI->>Store: call setPrivacyConsent / setNotificationChannel
Store-->>UI: state updated
UI->>User: update visual state
alt user requests data export
User->>UI: Click Export Data
UI->>API: POST /gdpr/export
API-->>UI: 202 Accepted / export link or error
UI->>Store: (optional) setGdprConsent / export status
UI->>User: show export loading / success / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
SummarySummary
CoverageCognitiveMesh.Shared - 14.2%
|
![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 (4)
src/UILayer/web/src/app/(app)/settings/notifications/page.tsx (2)
121-130: Consider validating or constraining timezone input.The timezone is a free-text input that accepts any string. Invalid values (e.g., "foo") would be stored and could cause issues when used for time calculations.
Options to improve:
- Use a
<select>with valid IANA timezones (viaIntl.supportedValuesOf('timeZone')in modern browsers)- Add validation on change/blur
- Use a timezone picker library
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/app/`(app)/settings/notifications/page.tsx around lines 121 - 130, The timezone input currently accepts free text via value={quietHours.timezone} and onChange={e => setQuietHours({ timezone: e.target.value })}; replace or augment this with validation or a constrained selector: either render a <select> populated from Intl.supportedValuesOf('timeZone') (fallback to a curated list if unsupported) and setQuietHours on selection, or keep the input but validate onChange/onBlur using a helper that checks the value against Intl.supportedValuesOf('timeZone') (or a regex/fallback list) and prevent saving/emit an error state if invalid; update the component state (quietHours and any error flag) and UI to reflect invalid timezone entries.
206-232: ToggleRow component is duplicated across settings pages.This
ToggleRowcomponent is nearly identical to the one insettings/page.tsx(lines 219-260), with the only difference being thedisabledprop support here. Consider extracting to a shared component.♻️ Suggested shared component location
Create a shared component at
src/UILayer/web/src/components/ui/ToggleRow.tsxthat supports thedisabledprop, then import it in both settings pages.// src/UILayer/web/src/components/ui/ToggleRow.tsx export function ToggleRow({ label, description, checked, onChange, disabled, }: { label: string description?: string checked: boolean onChange: (v: boolean) => void disabled?: boolean }) { // ... implementation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/app/`(app)/settings/notifications/page.tsx around lines 206 - 232, Extract the duplicated ToggleRow component into a single shared component named ToggleRow that accepts label, description?, checked, onChange, and disabled? props, preserves the useId() usage and the child ToggleButton with its checked/onChange/disabled/label props, then replace the local ToggleRow definitions in both settings pages by importing and using this shared ToggleRow to avoid duplication.src/UILayer/web/src/app/(app)/settings/page.tsx (1)
32-40: Dual localStorage persistence may cause sync issues.Language is persisted in two places:
- The Zustand store (via persist middleware to
cm-preferences)- Directly to
cognitivemesh_languagekeyIf
i18nConfigreads fromcognitivemesh_languageseparately from the store, this works but creates a maintenance risk—the two could get out of sync if one is updated without the other (e.g., onresetDefaults()).Consider:
- Having
i18nConfigread from the store instead of a separate key- Or documenting why both are needed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/app/`(app)/settings/page.tsx around lines 32 - 40, The code persists language both via the Zustand store (persist middleware storing to "cm-preferences") and directly in handleLanguageChange using localStorage key "cognitivemesh_language", which risks divergence (e.g., on resetDefaults); pick one source of truth and keep the other in sync: either update i18nConfig to read the language from the Zustand store (so remove direct localStorage writes in handleLanguageChange and ensure setLanguage updates persist) or, if keeping the localStorage key, ensure all store operations (including resetDefaults and setLanguage) also write to "cognitivemesh_language" and add a sync helper used by resetDefaults; reference handleLanguageChange, setLanguage, "cognitivemesh_language", i18nConfig, resetDefaults and the Zustand persist config ("cm-preferences") when making the change.src/UILayer/web/src/stores/usePreferencesStore.ts (1)
82-91: Address potential SSR hydration timing with Zustand persist.
Intl.DateTimeFormat().resolvedOptions().timeZoneexecutes at module load time (line 89). In Next.js SSR, this captures the server's timezone. The persist middleware (line 107) rehydrates only on the client, so there's a brief mismatch between server-rendered defaults and client-persisted values. While Zustand rehydrates quickly, this can trigger hydration warnings if client and server timezones differ.To eliminate this safely, configure persist with
skipHydration: trueand manually rehydrate in a client-only effect, or track hydration state and gate rendering. See Zustand's SSR documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/stores/usePreferencesStore.ts` around lines 82 - 91, The defaultNotificationPreferences object currently calls Intl.DateTimeFormat().resolvedOptions().timeZone at module load (affecting server-side render) which can mismatch client rehydration; update the persist middleware configuration used in usePreferencesStore to include skipHydration: true and stop computing timezone at module level (e.g., set quietHours.timezone to undefined or a placeholder in defaultNotificationPreferences), then perform client-only rehydration in a useEffect (or call setState) inside the store consumer or a hydration helper to set quietHours.timezone using Intl.DateTimeFormat().resolvedOptions().timeZone so server and client values won’t conflict during SSR rehydration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/UILayer/web/src/app/`(app)/profile/page.tsx:
- Around line 162-166: The "Authenticated since" InfoRow currently uses new
Date().toLocaleString() which shows render time; replace it to read the real
authentication timestamp from your auth/session store (e.g., use
authContext.user?.authenticatedAt or session.startTime) inside the profile page
component instead of calling new Date(), and format that value for display; if
no reliable timestamp exists, either remove the InfoRow or change the label to
"Current session" to avoid implying a start time.
- Around line 44-51: The consents state is only local (const [consents,
setConsents] = useState<ConsentRecord[]>(GDPR_CONSENTS)) so toggleConsent only
updates UI and is not persisted; update the handler to persist changes to the
app store or backend: replace or augment toggleConsent(type: string) so it
updates the shared preferences store (usePreferencesStore) by calling the store
updater used elsewhere (setPrivacyConsent) or call an API save endpoint, and
ensure exported/initial state is read from usePreferencesStore instead of
GDPR_CONSENTS; alternatively, add an explicit "Save consent preferences" action
that sends the current consents to setPrivacyConsent or the API and update the
UI text about logging accordingly.
---
Nitpick comments:
In `@src/UILayer/web/src/app/`(app)/settings/notifications/page.tsx:
- Around line 121-130: The timezone input currently accepts free text via
value={quietHours.timezone} and onChange={e => setQuietHours({ timezone:
e.target.value })}; replace or augment this with validation or a constrained
selector: either render a <select> populated from
Intl.supportedValuesOf('timeZone') (fallback to a curated list if unsupported)
and setQuietHours on selection, or keep the input but validate onChange/onBlur
using a helper that checks the value against Intl.supportedValuesOf('timeZone')
(or a regex/fallback list) and prevent saving/emit an error state if invalid;
update the component state (quietHours and any error flag) and UI to reflect
invalid timezone entries.
- Around line 206-232: Extract the duplicated ToggleRow component into a single
shared component named ToggleRow that accepts label, description?, checked,
onChange, and disabled? props, preserves the useId() usage and the child
ToggleButton with its checked/onChange/disabled/label props, then replace the
local ToggleRow definitions in both settings pages by importing and using this
shared ToggleRow to avoid duplication.
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx:
- Around line 32-40: The code persists language both via the Zustand store
(persist middleware storing to "cm-preferences") and directly in
handleLanguageChange using localStorage key "cognitivemesh_language", which
risks divergence (e.g., on resetDefaults); pick one source of truth and keep the
other in sync: either update i18nConfig to read the language from the Zustand
store (so remove direct localStorage writes in handleLanguageChange and ensure
setLanguage updates persist) or, if keeping the localStorage key, ensure all
store operations (including resetDefaults and setLanguage) also write to
"cognitivemesh_language" and add a sync helper used by resetDefaults; reference
handleLanguageChange, setLanguage, "cognitivemesh_language", i18nConfig,
resetDefaults and the Zustand persist config ("cm-preferences") when making the
change.
In `@src/UILayer/web/src/stores/usePreferencesStore.ts`:
- Around line 82-91: The defaultNotificationPreferences object currently calls
Intl.DateTimeFormat().resolvedOptions().timeZone at module load (affecting
server-side render) which can mismatch client rehydration; update the persist
middleware configuration used in usePreferencesStore to include skipHydration:
true and stop computing timezone at module level (e.g., set quietHours.timezone
to undefined or a placeholder in defaultNotificationPreferences), then perform
client-only rehydration in a useEffect (or call setState) inside the store
consumer or a hydration helper to set quietHours.timezone using
Intl.DateTimeFormat().resolvedOptions().timeZone so server and client values
won’t conflict during SSR rehydration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5042a5ae-4ce8-40c9-968d-e5362caba8bf
📒 Files selected for processing (6)
src/UILayer/web/src/app/(app)/profile/page.tsxsrc/UILayer/web/src/app/(app)/settings/notifications/page.tsxsrc/UILayer/web/src/app/(app)/settings/page.tsxsrc/UILayer/web/src/components/Navigation/Sidebar.tsxsrc/UILayer/web/src/components/Navigation/navItems.tssrc/UILayer/web/src/stores/usePreferencesStore.ts
- Replace <a> with Next.js <Link> in settings, notifications, profile pages - Call i18n.changeLanguage() on language select for immediate effect - Move GDPR consent from local useState to Zustand store (persisted) - Add GdprConsentRecord type + setGdprConsent action to preferences store - Fix "Authenticated since" to use useMemo (stable across re-renders) - Extract shared ToggleRow/ToggleButton to components/ui/toggle-switch.tsx - Replace freeform timezone input with curated timezone <select> - Add accessible label (htmlFor) to timezone select - Fix store doc comment: local-only with TODO for backend sync - Use canonical Tailwind class bg-white/2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/UILayer/web/src/app/(app)/profile/page.tsx (1)
141-170: Consider disabling the export button after submission to prevent duplicate requests.The
DataExportSectionshows a confirmation message after clicking, but there's a brief window where a user could double-click the button beforeexportRequestedstate updates. Addingdisabled={exportRequested}would provide visual feedback and prevent accidental double submissions.🛡️ Proposed fix
<button type="button" onClick={handleDataExport} + disabled={exportRequested} className="rounded-md bg-cyan-600 px-4 py-2 text-sm font-medium text-white transition hover:bg-cyan-500" > Request data export </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/app/`(app)/profile/page.tsx around lines 141 - 170, The export button in DataExportSection can be double-clicked before exportRequested updates; update the button element in DataExportSection to set disabled={exportRequested} (and optionally adjust its className for a disabled visual state) so that when handleDataExport sets exportRequested(true) the button is immediately disabled to prevent duplicate POSTs to the GDPR export endpoint.src/UILayer/web/src/stores/usePreferencesStore.ts (1)
139-145: Shallow merge ofcategoriesarray may cause unintended overwrites.
setNotificationPreferencesperforms a shallow spread. If a caller passes{ categories: [...] }, it replaces the entire array rather than merging individual category entries. This differs fromsetGdprConsentwhich properly filters and appends.Consider whether this is intentional. If per-category updates are expected, you may want a dedicated
setCategoryPreferenceaction similar tosetNotificationChannel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/stores/usePreferencesStore.ts` around lines 139 - 145, The current setNotificationPreferences action does a shallow merge of notificationPreferences (including replacing the categories array) which can unintentionally overwrite individual category entries; update this by either: (A) implementing a per-category updater (e.g. add a new setCategoryPreference action similar to setNotificationChannel that looks up notificationPreferences.categories, merges or updates the specific category entry, and writes back the updated categories array), or (B) change setNotificationPreferences to deep-merge categories by mapping existing notificationPreferences.categories and merging incoming prefs.categories entries by key/id (preserving entries not present in the incoming array) while still spreading other top-level fields; reference the existing setGdprConsent and setNotificationChannel implementations as patterns for filtering/appending and targeted updates.src/UILayer/web/src/app/(app)/settings/page.tsx (1)
47-52: Save button provides visual feedback only — consider clarifying intent.The "Save changes" button shows a "Saved" indicator but doesn't actually trigger any save action since Zustand's persist middleware auto-saves. The TODO mentions backend sync.
This is fine if purely for user reassurance, but if backend sync is planned, consider:
- Disabling the button when no changes were made
- Only showing "Saved" after successful backend confirmation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/app/`(app)/settings/page.tsx around lines 47 - 52, The handleSave function currently only sets local UI state (setSaved) while persistence is handled by Zustand; update it to reflect intended behavior: if you want a UI-only reassurance, clarify the button label/tooltip and disable it when no changes are pending by tracking a dirty flag from the preferences store; if backend sync is intended, implement an async save call from handleSave (call your user preferences API), await the response, show the "Saved" indicator only on success and surface errors on failure (use the same setSaved/state hooks and a loading/disabled state while the request is in-flight). Ensure you reference and update handleSave, setSaved, and the preferences dirty flag or API call wrapper used for syncing.src/UILayer/web/src/components/ui/toggle-switch.tsx (1)
5-31: Unusedidgenerated in ToggleRow.The
useId()call generates an ID that's applied to the label<span>, butToggleButtonuses its ownaria-labelprop rather thanaria-labelledby={id}. The ID serves no functional purpose currently.Either remove the unused ID or wire it up with
aria-labelledbyon the button for a more semantic association:♻️ Option: Use aria-labelledby instead of aria-label
export function ToggleRow({ label, description, checked, onChange, disabled, }: { label: string description?: string checked: boolean onChange: (v: boolean) => void disabled?: boolean }) { const id = useId() return ( <div className={`flex items-center justify-between gap-4 ${disabled ? "opacity-50" : ""}`}> <div> <span id={id} className="text-sm text-gray-400"> {label} </span> {description && <p className="text-xs text-gray-600">{description}</p>} </div> - <ToggleButton checked={checked} onChange={onChange} disabled={disabled} label={label} /> + <ToggleButton checked={checked} onChange={onChange} disabled={disabled} labelledBy={id} /> </div> ) }Then update
ToggleButtonto acceptlabelledByinstead of/in addition tolabel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/ui/toggle-switch.tsx` around lines 5 - 31, The useId() call in ToggleRow creates an unused id variable; either remove useId() and its id usage or wire it to the ToggleButton for proper labeling. Update ToggleRow to pass the generated id to ToggleButton (e.g., as labelledBy or labelledById) instead of/in addition to label, and update ToggleButton to accept the new prop and use aria-labelledby={labelledBy} (or prefer aria-labelledby over aria-label) so the span id from useId() is referenced; alternatively, strip useId() and the id prop from ToggleRow and keep ToggleButton's existing aria-label behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/UILayer/web/src/app/`(app)/profile/page.tsx:
- Line 224: Replace the invalid Tailwind opacity class "bg-white/2" with
"bg-white/5" in the JSX className for the div that currently reads "flex
items-center justify-between gap-4 rounded-md border border-white/5 bg-white/2
px-4 py-3" in the profile page component, and make the same change for the
analogous div in the notifications settings page where "bg-white/2" is used.
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx:
- Around line 35-45: handleLanguageChange currently writes language to
localStorage ("cognitivemesh_language") while the app persists preferences via
the zustand key ("cm-preferences"), and it calls i18n.changeLanguage() without
awaiting it; to fix, stop writing the duplicate "cognitivemesh_language" key and
instead rely on the zustand persisted store as the single source of truth (or
update i18nConfig.getPersistedLanguage to read from the store's persisted key),
and change handleLanguageChange to await i18n.changeLanguage(lang) (matching
LanguageSelector.tsx) before calling setLanguage so the runtime language change
completes before state persists to avoid flicker and inconsistency; update
references in handleLanguageChange, i18nConfig.getPersistedLanguage, and ensure
cm-preferences is the authoritative persisted key.
---
Nitpick comments:
In `@src/UILayer/web/src/app/`(app)/profile/page.tsx:
- Around line 141-170: The export button in DataExportSection can be
double-clicked before exportRequested updates; update the button element in
DataExportSection to set disabled={exportRequested} (and optionally adjust its
className for a disabled visual state) so that when handleDataExport sets
exportRequested(true) the button is immediately disabled to prevent duplicate
POSTs to the GDPR export endpoint.
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx:
- Around line 47-52: The handleSave function currently only sets local UI state
(setSaved) while persistence is handled by Zustand; update it to reflect
intended behavior: if you want a UI-only reassurance, clarify the button
label/tooltip and disable it when no changes are pending by tracking a dirty
flag from the preferences store; if backend sync is intended, implement an async
save call from handleSave (call your user preferences API), await the response,
show the "Saved" indicator only on success and surface errors on failure (use
the same setSaved/state hooks and a loading/disabled state while the request is
in-flight). Ensure you reference and update handleSave, setSaved, and the
preferences dirty flag or API call wrapper used for syncing.
In `@src/UILayer/web/src/components/ui/toggle-switch.tsx`:
- Around line 5-31: The useId() call in ToggleRow creates an unused id variable;
either remove useId() and its id usage or wire it to the ToggleButton for proper
labeling. Update ToggleRow to pass the generated id to ToggleButton (e.g., as
labelledBy or labelledById) instead of/in addition to label, and update
ToggleButton to accept the new prop and use aria-labelledby={labelledBy} (or
prefer aria-labelledby over aria-label) so the span id from useId() is
referenced; alternatively, strip useId() and the id prop from ToggleRow and keep
ToggleButton's existing aria-label behavior.
In `@src/UILayer/web/src/stores/usePreferencesStore.ts`:
- Around line 139-145: The current setNotificationPreferences action does a
shallow merge of notificationPreferences (including replacing the categories
array) which can unintentionally overwrite individual category entries; update
this by either: (A) implementing a per-category updater (e.g. add a new
setCategoryPreference action similar to setNotificationChannel that looks up
notificationPreferences.categories, merges or updates the specific category
entry, and writes back the updated categories array), or (B) change
setNotificationPreferences to deep-merge categories by mapping existing
notificationPreferences.categories and merging incoming prefs.categories entries
by key/id (preserving entries not present in the incoming array) while still
spreading other top-level fields; reference the existing setGdprConsent and
setNotificationChannel implementations as patterns for filtering/appending and
targeted updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 163cb5b9-b66f-4410-8bd1-4163530cf6e3
📒 Files selected for processing (5)
src/UILayer/web/src/app/(app)/profile/page.tsxsrc/UILayer/web/src/app/(app)/settings/notifications/page.tsxsrc/UILayer/web/src/app/(app)/settings/page.tsxsrc/UILayer/web/src/components/ui/toggle-switch.tsxsrc/UILayer/web/src/stores/usePreferencesStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/UILayer/web/src/app/(app)/settings/notifications/page.tsx
| const id = useId() | ||
|
|
||
| return ( | ||
| <div className="flex items-center justify-between gap-4 rounded-md border border-white/5 bg-white/2 px-4 py-3"> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if bg-white/2 is used elsewhere or if similar patterns exist
rg -n "bg-white/[0-9]" --type tsx --type ts || rg -n "bg-white/[0-9]"Repository: phoenixvc/cognitive-mesh
Length of output: 6147
🌐 Web query:
Does Tailwind CSS support arbitrary opacity values like bg-white/2?
💡 Result:
Tailwind’s /opacity modifier doesn’t treat bg-white/2 as an automatic “2% opacity” by default—the number after / is looked up in your configured opacity scale (e.g. /50, /75, etc.). (tailwindcss.com)
If you want a truly arbitrary opacity, Tailwind supports square-bracket arbitrary opacity modifiers, for example:
bg-white/[0.02](2% as a 0–1 alpha)bg-white/[2%]
Citations:
- 1: https://tailwindcss.com/blog/tailwindcss-2-2?utm_source=openai
- 2: https://tailwindcss.com/blog/tailwindcss-2-2?utm_source=openai
🏁 Script executed:
find . -name "tailwind.config.*" -o -name "tailwind.js" -o -name ".tailwindrc*" | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 104
🏁 Script executed:
# Also check the package.json or postcss.config for Tailwind setup
ls -la | grep -E "(tailwind|postcss|next\.config)" || echo "No obvious config files in root"Repository: phoenixvc/cognitive-mesh
Length of output: 99
🏁 Script executed:
cat -n src/UILayer/web/tailwind.config.jsRepository: phoenixvc/cognitive-mesh
Length of output: 5227
Change bg-white/2 to bg-white/5.
bg-white/2 is not a valid Tailwind opacity modifier. Tailwind's default opacity scale includes 5, 10, 20, 25, etc., but not 2. Using bg-white/5 aligns with the rest of the codebase, which consistently uses this value for similar background elements.
Note: This same issue appears in src/UILayer/web/src/app/(app)/settings/notifications/page.tsx:206 and should also be corrected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/app/`(app)/profile/page.tsx at line 224, Replace the
invalid Tailwind opacity class "bg-white/2" with "bg-white/5" in the JSX
className for the div that currently reads "flex items-center justify-between
gap-4 rounded-md border border-white/5 bg-white/2 px-4 py-3" in the profile page
component, and make the same change for the analogous div in the notifications
settings page where "bg-white/2" is used.
Comment on lines
+35
to
+45
| function handleLanguageChange(lang: SupportedLanguage) { | ||
| setLanguage(lang) | ||
| // Update i18next runtime language immediately | ||
| i18n.changeLanguage(lang) | ||
| // Also persist to localStorage key used by i18nConfig | ||
| try { | ||
| localStorage.setItem("cognitivemesh_language", lang) | ||
| } catch { | ||
| // localStorage may be unavailable | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Dual storage for language creates potential inconsistency.
Language is stored in two places:
- Zustand persist middleware (
cm-preferenceskey) - Direct localStorage (
cognitivemesh_languagekey)
The i18nConfig.getPersistedLanguage() reads from cognitivemesh_language (per context snippet 3), while the store uses cm-preferences. If these get out of sync (e.g., one write fails), the app may show inconsistent language on reload.
Additionally, i18n.changeLanguage() is async but not awaited here, unlike in LanguageSelector.tsx which properly awaits it. This could cause a brief UI flicker.
Consider consolidating to a single source of truth or making the store's persist the authoritative source that i18nConfig reads from.
🔧 Proposed fix to await the async operation
- function handleLanguageChange(lang: SupportedLanguage) {
+ async function handleLanguageChange(lang: SupportedLanguage) {
setLanguage(lang)
// Update i18next runtime language immediately
- i18n.changeLanguage(lang)
+ await i18n.changeLanguage(lang)
// Also persist to localStorage key used by i18nConfig
try {
localStorage.setItem("cognitivemesh_language", lang)
} catch {
// localStorage may be unavailable
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx around lines 35 - 45,
handleLanguageChange currently writes language to localStorage
("cognitivemesh_language") while the app persists preferences via the zustand
key ("cm-preferences"), and it calls i18n.changeLanguage() without awaiting it;
to fix, stop writing the duplicate "cognitivemesh_language" key and instead rely
on the zustand persisted store as the single source of truth (or update
i18nConfig.getPersistedLanguage to read from the store's persisted key), and
change handleLanguageChange to await i18n.changeLanguage(lang) (matching
LanguageSelector.tsx) before calling setLanguage so the runtime language change
completes before state persists to avoid flicker and inconsistency; update
references in handleLanguageChange, i18nConfig.getPersistedLanguage, and ensure
cm-preferences is the authoritative persisted key.
Backend: CancellationToken propagation, atomic ConcurrentDictionary updates, Cypher injection prevention via regex validation, authority override revocation. UI components: forwardRef type corrections, aria-hidden/aria-label a11y fixes, event listener cleanup, CSS sanitization for dangerouslySetInnerHTML, unique keys with index fallback, variant priority fix, displayName casing. Pages/hooks/stores: open redirect prevention, SSR hydration fix, timer cleanup, SignalR mounted guard, auth token expiry check, Array.isArray guard, crypto randomUUID replacing module counter, Zustand persist with versioned migration, devDependencies cleanup, dark-themed select options. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolves package.json conflicts: adopt pinned versions from Renovate, keep @testing-library/dom and @types/uuid in devDependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/UILayer/web/src/components/ParticleField.tsx (1)
150-158:⚠️ Potential issue | 🟠 MajorMemory leak: resize event listener is never removed.
The resize event listener is added on mount but not cleaned up when the component unmounts. This causes a memory leak and can lead to errors when the handler references a stale canvas.
🐛 Proposed fix to clean up the resize listener
animate(); - window.addEventListener("resize", () => { + const handleResize = () => { width = window.innerWidth; height = window.innerHeight; canvas.width = width; canvas.height = height; - }); + }; + window.addEventListener("resize", handleResize); return () => { - cancelAnimationFrame(animationRef.current!); + if (animationRef.current !== undefined) { + cancelAnimationFrame(animationRef.current); + } + window.removeEventListener("resize", handleResize); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/ParticleField.tsx` around lines 150 - 158, The resize listener added in the ParticleField component is not removed on unmount; capture the handler (e.g. a named function like onResize or const handleResize) that updates width/height and canvas.width/height and register it with window.addEventListener, then remove it in the cleanup returned by the effect using window.removeEventListener; also keep the existing cancelAnimationFrame(animationRef.current!) call in the same cleanup so both the animation and the resize listener are properly cleaned up to avoid stale canvas references.
♻️ Duplicate comments (1)
src/UILayer/web/src/app/(app)/settings/page.tsx (1)
42-52:⚠️ Potential issue | 🟠 MajorCollapse language persistence into one source of truth.
setLanguage()already persists through the store, but this handler also writescognitivemesh_languagedirectly and firesi18n.changeLanguage()without awaiting/catching it. That lets the selected language, the runtime locale, and the persisted locale drift apart;resetDefaults()is already a path that bypasses this handler entirely.#!/bin/bash # Inspect the language persistence keys and i18n runtime updates. rg -n -C2 'cognitivemesh_language|cm-preferences|getPersistedLanguage|changeLanguage\s*\(' src/UILayer/web/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/app/`(app)/settings/page.tsx around lines 42 - 52, handleLanguageChange currently writes localStorage directly and calls i18n.changeLanguage without awaiting, which can desync persisted preference, runtime locale, and the store; remove the direct localStorage write and make setLanguage the single source of truth, then update the runtime i18next locale by awaiting i18n.changeLanguage(lang) inside a try/catch so failures are handled (e.g., try { await i18n.changeLanguage(lang) } catch (e) { /* handle/log */ }); keep resetDefaults unchanged so it interacts via the same store path; reference handleLanguageChange, setLanguage, i18n.changeLanguage, and the "cognitivemesh_language" key when making the change.
🧹 Nitpick comments (2)
src/UILayer/web/src/components/agency/AgentActionAuditTrail.tsx (1)
329-329:onKeyPressis deprecated; preferonKeyDown.The
onKeyPressevent is deprecated in React. Additionally, button-like elements should typically respond to both Enter and Space keys for accessibility.♻️ Suggested refactor
- onKeyPress={(e) => e.key === 'Enter' && toggleExpand(groupKey)} + onKeyDown={(e) => (e.key === 'Enter' || e.key === ' ') && toggleExpand(groupKey)}Apply the same pattern to line 348.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/agency/AgentActionAuditTrail.tsx` at line 329, Replace deprecated onKeyPress handlers with onKeyDown and handle both Enter and Space keys to improve accessibility: update the onKeyPress={(e) => e.key === 'Enter' && toggleExpand(groupKey)} occurrences (e.g., the one using toggleExpand and groupKey at line ~329 and the similar handler at ~348) to use onKeyDown, check for e.key === 'Enter' || e.key === ' ' (or 'Spacebar' for older browsers) and call toggleExpand(groupKey); ensure the same change is applied to both locations referenced in AgentActionAuditTrail.tsx.src/UILayer/web/src/stores/usePreferencesStore.ts (1)
180-186: Consider adding a migration fallback for robustness.The migration handles
version === 0correctly. For defensive coding, consider adding a fallback that merges defaults for any unrecognized version to prevent potential issues if the version numbering ever gets out of sync.migrate: (persisted, version) => { if (version === 0) { return { ...defaults, ...(persisted as Partial<PreferencesState>) } } - return persisted as PreferencesState & PreferencesActions + // Fallback: merge with defaults to ensure all fields exist + return { ...defaults, ...(persisted as Partial<PreferencesState>) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/stores/usePreferencesStore.ts` around lines 180 - 186, Update the migrate handler in the persist config (the migrate function) to include a defensive fallback: when the incoming version is not explicitly handled (i.e., not 0), return a merged object combining defaults with the persisted state cast to PreferencesState & PreferencesActions so unknown versions still get default fields; keep the existing branch for version === 0 and add this fallback branch that merges defaults with persisted to ensure robustness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/BusinessApplications/CustomerIntelligence/CustomerIntelligenceManager.cs`:
- Around line 71-79: Move the ASCII-only validation for customerId to run
immediately after the existing null/whitespace check and before any port calls
(e.g., before _customerDataPort.GetProfileAsync) so malformed IDs fail fast;
replace the Unicode \w class with an explicit ASCII character class by using
Regex.IsMatch(customerId, "^[A-Za-z0-9_-]+$") and keep throwing the same
ArgumentException(nameof(customerId)) when the check fails; ensure the existing
_knowledgeGraphManager.QueryAsync call remains after this new guard.
In `@src/BusinessApplications/NISTCompliance/Services/NISTComplianceService.cs`:
- Around line 223-233: The AddOrUpdate call on _evidence mutates the existing
EvidenceRecord instance (inside the lambda) which can cause partial/inconsistent
views; instead, in the update lambda of AddOrUpdate (the (_, existing) => {...}
passed in NISTComplianceService), create and return a new EvidenceRecord
instance copying all necessary fields from existing and setting ReviewStatus =
request.Decision, ReviewedBy = request.ReviewerId, ReviewedAt = now, and
ReviewNotes = request.Notes so the replacement is atomic; ensure you use the
same key (request.EvidenceId) and preserve any other properties from existing
when constructing the new instance.
In `@src/UILayer/web/MIGRATION.md`:
- Line 307: Update the migration summary and page table in MIGRATION.md to
include the two routes added by this PR: /profile and /settings/notifications;
add rows for these routes to the pages table (the section listing built pages)
and update the aggregate line currently reading "Migration progress: 96%
complete (145/151 items done, 0 TypeScript errors, Next.js 16 build passing)"
and the "11 pages" tally to reflect 14 pages and 147/151 items done (adjust the
percentage accordingly, e.g., ~97%).
In `@src/UILayer/web/src/app/`(app)/profile/page.tsx:
- Around line 159-173: The handleDataExport function is sending the POST without
auth; update it to include authentication by either using the shared API client
(import from "@/lib/api/client") to make the request so headers are handled
automatically, or add an Authorization header to the fetch call (e.g., obtain
the current token/session and set Authorization: `Bearer <token>` in the fetch
options) and keep existing error handling (setExporting, setExportError,
setExportRequested) unchanged; ensure the request options for handleDataExport
include the headers object with the auth header so GDPR export is authenticated.
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx:
- Around line 54-59: handleSave currently only sets the transient "Saved" badge
(setSaved) and clears it after 2s without persisting to the backend; update
handleSave to perform an async save to the backend user preferences API (call
the existing persist function or create one) and only setSaved(true) after the
backend confirms success, show a "Saving..." state while the request is in
flight, handle and surface errors (do not flip the saved badge on failure) and
keep using saveTimerRef to clear the success state; reference the handleSave
function, setSaved, saveTimerRef.current and the TODO about backend sync when
adding the API call, its error handling and optional retry/backoff.
In `@src/UILayer/web/src/components/agency/AgentActionAuditTrail.tsx`:
- Around line 244-248: The useEffect registers a named handler with
themeAdapter.onThemeChanged but never cleans it up, risking memory leaks and
setState-on-unmounted warnings; update the onThemeChanged signature to return an
unsubscribe function (e.g., onThemeChanged: (cb: (s: ThemeSettings) => void) =>
() => void), update any mocks to match that return type, and modify the
useEffect in AgentActionAuditTrail to capture the unsubscribe (const unsubscribe
= themeAdapter.onThemeChanged(handler)) and return a cleanup that calls
unsubscribe(), while keeping the existing getCurrentThemeAsync call and
setCurrentTheme handler.
In `@src/UILayer/web/src/components/ui/chart.tsx`:
- Around line 70-72: The current sanitizeCssToken function's blacklist is
incomplete and still allows CSS injection via characters like parentheses,
slash, asterisk, and newlines; update the sanitizeCssToken implementation to
either (1) switch to a strict whitelist for CSS variable names (e.g., only allow
[a-zA-Z0-9-_] and validate against that) or (2) expand the blacklist to remove
parentheses () , forward/back slashes / \ , asterisk * , newline/CR characters
and any other token-delimiting characters; additionally, if the token is
expected to be a color value, validate against a strict pattern such as a hex
color or named color instead of broad sanitization. Ensure the change is applied
to the sanitizeCssToken function so all callers use the tightened validation.
---
Outside diff comments:
In `@src/UILayer/web/src/components/ParticleField.tsx`:
- Around line 150-158: The resize listener added in the ParticleField component
is not removed on unmount; capture the handler (e.g. a named function like
onResize or const handleResize) that updates width/height and
canvas.width/height and register it with window.addEventListener, then remove it
in the cleanup returned by the effect using window.removeEventListener; also
keep the existing cancelAnimationFrame(animationRef.current!) call in the same
cleanup so both the animation and the resize listener are properly cleaned up to
avoid stale canvas references.
---
Duplicate comments:
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx:
- Around line 42-52: handleLanguageChange currently writes localStorage directly
and calls i18n.changeLanguage without awaiting, which can desync persisted
preference, runtime locale, and the store; remove the direct localStorage write
and make setLanguage the single source of truth, then update the runtime i18next
locale by awaiting i18n.changeLanguage(lang) inside a try/catch so failures are
handled (e.g., try { await i18n.changeLanguage(lang) } catch (e) { /* handle/log
*/ }); keep resetDefaults unchanged so it interacts via the same store path;
reference handleLanguageChange, setLanguage, i18n.changeLanguage, and the
"cognitivemesh_language" key when making the change.
---
Nitpick comments:
In `@src/UILayer/web/src/components/agency/AgentActionAuditTrail.tsx`:
- Line 329: Replace deprecated onKeyPress handlers with onKeyDown and handle
both Enter and Space keys to improve accessibility: update the onKeyPress={(e)
=> e.key === 'Enter' && toggleExpand(groupKey)} occurrences (e.g., the one using
toggleExpand and groupKey at line ~329 and the similar handler at ~348) to use
onKeyDown, check for e.key === 'Enter' || e.key === ' ' (or 'Spacebar' for older
browsers) and call toggleExpand(groupKey); ensure the same change is applied to
both locations referenced in AgentActionAuditTrail.tsx.
In `@src/UILayer/web/src/stores/usePreferencesStore.ts`:
- Around line 180-186: Update the migrate handler in the persist config (the
migrate function) to include a defensive fallback: when the incoming version is
not explicitly handled (i.e., not 0), return a merged object combining defaults
with the persisted state cast to PreferencesState & PreferencesActions so
unknown versions still get default fields; keep the existing branch for version
=== 0 and add this fallback branch that merges defaults with persisted to ensure
robustness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9cbea9d-198e-4de2-b01e-54476b32179d
📒 Files selected for processing (32)
src/BusinessApplications/AgentRegistry/Services/AgentRegistryService.cssrc/BusinessApplications/AgentRegistry/Services/AuthorityService.cssrc/BusinessApplications/CustomerIntelligence/CustomerIntelligenceManager.cssrc/BusinessApplications/NISTCompliance/Services/NISTComplianceService.cssrc/UILayer/web/MIGRATION.mdsrc/UILayer/web/package.jsonsrc/UILayer/web/src/app/(app)/profile/page.tsxsrc/UILayer/web/src/app/(app)/settings/notifications/page.tsxsrc/UILayer/web/src/app/(app)/settings/page.tsxsrc/UILayer/web/src/app/login/page.tsxsrc/UILayer/web/src/components/Navigation/TopBar.tsxsrc/UILayer/web/src/components/Nexus/index.tsxsrc/UILayer/web/src/components/ParticleField.tsxsrc/UILayer/web/src/components/agency/AgentActionAuditTrail.tsxsrc/UILayer/web/src/components/ui/alert.tsxsrc/UILayer/web/src/components/ui/breadcrumb.tsxsrc/UILayer/web/src/components/ui/carousel.tsxsrc/UILayer/web/src/components/ui/chart.tsxsrc/UILayer/web/src/components/ui/collapsible.tsxsrc/UILayer/web/src/components/ui/form.tsxsrc/UILayer/web/src/components/ui/menubar.tsxsrc/UILayer/web/src/components/ui/pagination.tsxsrc/UILayer/web/src/components/ui/progress.tsxsrc/UILayer/web/src/components/ui/resizable.tsxsrc/UILayer/web/src/components/ui/sidebar.tsxsrc/UILayer/web/src/components/ui/toggle-group.tsxsrc/UILayer/web/src/contexts/AuthContext.tsxsrc/UILayer/web/src/hooks/use-toast.tssrc/UILayer/web/src/hooks/useSignalR.tssrc/UILayer/web/src/stores/useAgentStore.tssrc/UILayer/web/src/stores/useNotificationStore.tssrc/UILayer/web/src/stores/usePreferencesStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/UILayer/web/src/app/(app)/settings/notifications/page.tsx
Comment on lines
+71
to
79
| // Validate customerId contains only safe characters (alphanumeric, hyphens, underscores) | ||
| if (!System.Text.RegularExpressions.Regex.IsMatch(customerId, @"^[\w\-]+$")) | ||
| { | ||
| throw new ArgumentException("Customer ID contains invalid characters", nameof(customerId)); | ||
| } | ||
|
|
||
| var relationships = await _knowledgeGraphManager.QueryAsync( | ||
| $"MATCH (c:Customer {{id: '{safeCustomerId}'}})-[r]->(s:Segment) RETURN s", | ||
| $"MATCH (c:Customer {{id: '{customerId}'}})-[r]->(s:Segment) RETURN s", | ||
| cancellationToken).ConfigureAwait(false); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET regular expressions, does \win the pattern^[\w-]+$match only ASCII[A-Za-z0-9_], or does it also match Unicode letters and combining marks?
💡 Result:
In .NET regex by default (Unicode mode), \w is not limited to ASCII. It matches any character in these Unicode categories: Ll, Lu, Lt, Lo, Lm (letters), Mn (nonspacing combining marks), Nd (decimal digits), and Pc (connector punctuation, e.g. _). So ^[\w\-]+$ will accept many non-ASCII letters and also nonspacing combining marks. [1]
Only when you use RegexOptions.ECMAScript does \w become ASCII-only, equivalent to [a-zA-Z_0-9]. [1]
Source
[1] Microsoft Learn — “Character Classes in .NET Regular Expressions” (Word character: \w) (learn.microsoft.com)
Citations:
🏁 Script executed:
cat -n src/BusinessApplications/CustomerIntelligence/CustomerIntelligenceManager.cs | sed -n '50,85p'Repository: phoenixvc/cognitive-mesh
Length of output: 1899
Move this guard before the port call, and use an ASCII-only character class instead of \w.
The regex pattern ^[\w\-]+$ accepts Unicode letters and combining marks by default in .NET, which is broader than intended. Use ^[A-Za-z0-9_-]+$ to enforce ASCII-only identifiers. Additionally, since this validation currently runs after the call to _customerDataPort.GetProfileAsync, malformed IDs still reach that port; move the guard earlier in the method to fail fast.
Suggested fix
- // Validate customerId contains only safe characters (alphanumeric, hyphens, underscores)
- if (!System.Text.RegularExpressions.Regex.IsMatch(customerId, @"^[\w\-]+$"))
- {
- throw new ArgumentException("Customer ID contains invalid characters", nameof(customerId));
- }// Add validation immediately after the null/whitespace check
if (!System.Text.RegularExpressions.Regex.IsMatch(customerId, "^[A-Za-z0-9_-]+$"))
{
throw new ArgumentException("Customer ID contains invalid characters", nameof(customerId));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BusinessApplications/CustomerIntelligence/CustomerIntelligenceManager.cs`
around lines 71 - 79, Move the ASCII-only validation for customerId to run
immediately after the existing null/whitespace check and before any port calls
(e.g., before _customerDataPort.GetProfileAsync) so malformed IDs fail fast;
replace the Unicode \w class with an explicit ASCII character class by using
Regex.IsMatch(customerId, "^[A-Za-z0-9_-]+$") and keep throwing the same
ArgumentException(nameof(customerId)) when the check fails; ensure the existing
_knowledgeGraphManager.QueryAsync call remains after this new guard.
Comment on lines
+223
to
+233
| _evidence.AddOrUpdate( | ||
| request.EvidenceId, | ||
| _ => throw new InvalidOperationException("Evidence record disappeared during review"), | ||
| (_, existing) => | ||
| { | ||
| existing.ReviewStatus = request.Decision; | ||
| existing.ReviewedBy = request.ReviewerId; | ||
| existing.ReviewedAt = now; | ||
| existing.ReviewNotes = request.Notes; | ||
| return existing; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In .NET's ConcurrentDictionary<TKey,TValue>, are AddOrUpdatedelegates invoked outside the dictionary's internal locks and can the update delegate be called multiple times? IfTValueis a mutable class, does mutating the existing instance insideupdateValueFactory make the whole object update atomic for concurrent readers?
💡 Result:
- Yes. In
ConcurrentDictionary<TKey,TValue>, theaddValueFactory/updateValueFactorydelegates used byGetOrAdd/AddOrUpdateare invoked outside the dictionary’s internal locks (so user code doesn’t run under a lock). (learn.microsoft.com) - Yes. Under contention, the update delegate can be invoked multiple times (the operation is implemented with an optimistic “retry if someone else changed it first” approach), so the delegates must be safe to run more than once and concurrently. (stackoverflow.com)
- No. If
TValueis a mutable reference type, mutating the existing instance insideupdateValueFactorydoes not make “the whole object update” atomic for concurrent readers.ConcurrentDictionaryonly makes the dictionary’s slot update (i.e., which reference is stored for the key) thread-safe; it does not synchronize or make atomic the internal state changes of the object you stored. This mutable-value pattern is specifically called out as a pitfall. (stackoverflow.com)
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?view=net-9.0&utm_source=openai
- 2: https://stackoverflow.com/questions/10486579/concurrentdictionary-pitfall-are-delegate-factories-from-getoradd-and-addorupd?utm_source=openai
- 3: https://stackoverflow.com/questions/12414546/lock-for-concurrentdictionary-when-addorupdate-ing?utm_source=openai
🏁 Script executed:
# First, check the file exists and get its structure
cd src/BusinessApplications/NISTCompliance/Services
ls -la NISTComplianceService.cs
# Get line count to verify context
wc -l NISTComplianceService.cs
# Extract lines around 223-233 to see the actual code
sed -n '220,240p' NISTComplianceService.csRepository: phoenixvc/cognitive-mesh
Length of output: 1899
🏁 Script executed:
# Search for _evidence field declaration to verify type
grep -n "_evidence" src/BusinessApplications/NISTCompliance/Services/NISTComplianceService.cs | head -5Repository: phoenixvc/cognitive-mesh
Length of output: 1899
🏁 Script executed:
# Search for EvidenceRecord class definition to verify it's mutable
fd "EvidenceRecord" --type f -x grep -l "class EvidenceRecord" {} \;
# Get the EvidenceRecord class definition
grep -n "class EvidenceRecord" src/BusinessApplications/NISTCompliance/Services/NISTComplianceService.csRepository: phoenixvc/cognitive-mesh
Length of output: 1899
Avoid in-place mutation inside ConcurrentDictionary.AddOrUpdate.
AddOrUpdate only synchronizes the dictionary slot, not the internal state of mutable reference types. Mutating the existing EvidenceRecord instance allows concurrent reviews to interleave field assignments, causing readers to see mixed ReviewStatus/ReviewedBy/ReviewedAt/ReviewNotes snapshots. Per .NET best practices, return a new EvidenceRecord instance so the full review state update is atomic.
🔧 Suggested fix
- _evidence.AddOrUpdate(
- request.EvidenceId,
- _ => throw new InvalidOperationException("Evidence record disappeared during review"),
- (_, existing) =>
- {
- existing.ReviewStatus = request.Decision;
- existing.ReviewedBy = request.ReviewerId;
- existing.ReviewedAt = now;
- existing.ReviewNotes = request.Notes;
- return existing;
- });
+ _evidence.AddOrUpdate(
+ request.EvidenceId,
+ _ => throw new InvalidOperationException("Evidence record disappeared during review"),
+ (_, existing) => new EvidenceRecord
+ {
+ EvidenceId = existing.EvidenceId,
+ StatementId = existing.StatementId,
+ ArtifactType = existing.ArtifactType,
+ Content = existing.Content,
+ SubmittedBy = existing.SubmittedBy,
+ Tags = existing.Tags is null ? null : new List<string>(existing.Tags),
+ FileSizeBytes = existing.FileSizeBytes,
+ SubmittedAt = existing.SubmittedAt,
+ ReviewStatus = request.Decision,
+ ReviewedBy = request.ReviewerId,
+ ReviewedAt = now,
+ ReviewNotes = request.Notes
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/BusinessApplications/NISTCompliance/Services/NISTComplianceService.cs`
around lines 223 - 233, The AddOrUpdate call on _evidence mutates the existing
EvidenceRecord instance (inside the lambda) which can cause partial/inconsistent
views; instead, in the update lambda of AddOrUpdate (the (_, existing) => {...}
passed in NISTComplianceService), create and return a new EvidenceRecord
instance copying all necessary fields from existing and setting ReviewStatus =
request.Decision, ReviewedBy = request.ReviewerId, ReviewedAt = now, and
ReviewNotes = request.Notes so the replacement is atomic; ensure you use the
same key (request.EvidenceId) and preserve any other properties from existing
when constructing the new instance.
| | **Totals** | **151** | **145** | **0** | **1** | **5** | **0** | | ||
|
|
||
| **Migration progress: 100% complete (0 TypeScript errors, Next.js 16 build passing)** | ||
| **Migration progress: 96% complete (145/151 items done, 0 TypeScript errors, Next.js 16 build passing)** |
Contributor
There was a problem hiding this comment.
Refresh the tracker totals for the routes added in this PR.
The test plan already calls out 14 built pages, but this summary still rolls up to 11 pages and 145/151 items. /profile and /settings/notifications need to be reflected in the page table and the aggregate counts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/MIGRATION.md` at line 307, Update the migration summary and
page table in MIGRATION.md to include the two routes added by this PR: /profile
and /settings/notifications; add rows for these routes to the pages table (the
section listing built pages) and update the aggregate line currently reading
"Migration progress: 96% complete (145/151 items done, 0 TypeScript errors,
Next.js 16 build passing)" and the "11 pages" tally to reflect 14 pages and
147/151 items done (adjust the percentage accordingly, e.g., ~97%).
Comment on lines
+159
to
+173
| async function handleDataExport() { | ||
| setExporting(true) | ||
| setExportError(null) | ||
| try { | ||
| const res = await fetch( | ||
| `${process.env.NEXT_PUBLIC_API_BASE_URL ?? "http://localhost:5000"}/api/v1/compliance/gdpr/data-export`, | ||
| { method: "POST" } | ||
| ) | ||
| if (!res.ok) throw new Error(`Export request failed (${res.status})`) | ||
| setExportRequested(true) | ||
| } catch (err) { | ||
| setExportError(err instanceof Error ? err.message : "Export request failed") | ||
| } finally { | ||
| setExporting(false) | ||
| } |
Contributor
There was a problem hiding this comment.
Data export request is missing authentication.
The POST to /api/v1/compliance/gdpr/data-export doesn't include an Authorization header. GDPR data export is a sensitive operation that should require authentication to prevent unauthorized data requests.
🔧 Proposed fix to add authentication
async function handleDataExport() {
setExporting(true)
setExportError(null)
try {
+ const token = localStorage.getItem("cm_access_token")
const res = await fetch(
`${process.env.NEXT_PUBLIC_API_BASE_URL ?? "http://localhost:5000"}/api/v1/compliance/gdpr/data-export`,
- { method: "POST" }
+ {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/json",
+ ...(token ? { Authorization: `Bearer ${token}` } : {}),
+ },
+ }
)Alternatively, consider using the shared API client (@/lib/api/client) which likely handles auth headers automatically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/app/`(app)/profile/page.tsx around lines 159 - 173, The
handleDataExport function is sending the POST without auth; update it to include
authentication by either using the shared API client (import from
"@/lib/api/client") to make the request so headers are handled automatically, or
add an Authorization header to the fetch call (e.g., obtain the current
token/session and set Authorization: `Bearer <token>` in the fetch options) and
keep existing error handling (setExporting, setExportError, setExportRequested)
unchanged; ensure the request options for handleDataExport include the headers
object with the auth header so GDPR export is authenticated.
Comment on lines
+54
to
+59
| function handleSave() { | ||
| // Preferences are already persisted to localStorage via Zustand persist. | ||
| // TODO: Sync to backend user preferences API. | ||
| setSaved(true) | ||
| if (saveTimerRef.current) clearTimeout(saveTimerRef.current) | ||
| saveTimerRef.current = setTimeout(() => setSaved(false), 2000) |
Contributor
There was a problem hiding this comment.
Don't show a successful save before the backend record exists.
handleSave() only flips a transient "Saved" badge, and the TODO confirms there is still no backend sync. For the GDPR/privacy toggles on this page, browser-local persistence alone is not a durable audit trail or cross-device consent record.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/app/`(app)/settings/page.tsx around lines 54 - 59,
handleSave currently only sets the transient "Saved" badge (setSaved) and clears
it after 2s without persisting to the backend; update handleSave to perform an
async save to the backend user preferences API (call the existing persist
function or create one) and only setSaved(true) after the backend confirms
success, show a "Saving..." state while the request is in flight, handle and
surface errors (do not flip the saved badge on failure) and keep using
saveTimerRef to clear the success state; reference the handleSave function,
setSaved, saveTimerRef.current and the TODO about backend sync when adding the
API call, its error handling and optional retry/backoff.
Comment on lines
244
to
248
| useEffect(() => { | ||
| themeAdapter.getCurrentThemeAsync().then((t) => setCurrentTheme(t)); | ||
| themeAdapter.onThemeChanged((t) => setCurrentTheme(t)); | ||
| const handler = (settings: ThemeSettings) => setCurrentTheme(settings); | ||
| themeAdapter.onThemeChanged(handler); | ||
| }, [themeAdapter]); |
Contributor
There was a problem hiding this comment.
Missing cleanup for theme subscription may cause memory leaks or state updates on unmounted components.
The refactor introduces a named handler but the useEffect still lacks a cleanup function. If the component unmounts or themeAdapter changes, the old callback remains registered, potentially causing "setState on unmounted component" warnings or memory leaks.
The interface for onThemeChanged (line 222) should return an unsubscribe function to enable proper cleanup:
🛠️ Proposed fix
Update the adapter interface to return an unsubscribe function:
onThemeChanged: (callback: (settings: ThemeSettings) => void) => () => void;Then update the mock and useEffect:
const mockThemeAdapter = {
getCurrentThemeAsync: async (): Promise<ThemeSettings> => ({ name: 'Light', highContrastEnabled: false, languageCode: 'en-US' }),
- onThemeChanged: (callback: (settings: ThemeSettings) => void) => {
+ onThemeChanged: (callback: (settings: ThemeSettings) => void): (() => void) => {
// Simulate theme change after 5 seconds for testing
- setTimeout(() => {
+ const timeoutId = setTimeout(() => {
callback({ name: 'Dark', highContrastEnabled: false, languageCode: 'en-US' });
}, 5000);
+ return () => clearTimeout(timeoutId);
},
}; useEffect(() => {
themeAdapter.getCurrentThemeAsync().then((t) => setCurrentTheme(t));
const handler = (settings: ThemeSettings) => setCurrentTheme(settings);
- themeAdapter.onThemeChanged(handler);
+ const unsubscribe = themeAdapter.onThemeChanged(handler);
+ return () => unsubscribe();
}, [themeAdapter]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/components/agency/AgentActionAuditTrail.tsx` around lines
244 - 248, The useEffect registers a named handler with
themeAdapter.onThemeChanged but never cleans it up, risking memory leaks and
setState-on-unmounted warnings; update the onThemeChanged signature to return an
unsubscribe function (e.g., onThemeChanged: (cb: (s: ThemeSettings) => void) =>
() => void), update any mocks to match that return type, and modify the
useEffect in AgentActionAuditTrail to capture the unsubscribe (const unsubscribe
= themeAdapter.onThemeChanged(handler)) and return a cleanup that calls
unsubscribe(), while keeping the existing getCurrentThemeAsync call and
setCurrentTheme handler.
Comment on lines
+70
to
+72
| function sanitizeCssToken(str: string): string { | ||
| return str.replace(/[<>"'&;{}\\]/g, "") | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete CSS sanitization blacklist.
The regex misses characters that could enable CSS injection:
- Parentheses
()allowurl()orexpression()injection (e.g.,url(https://attacker.com)) /and*allow CSS comment injection/* */- Newlines could break out of the declaration context
Consider using a whitelist approach for the key (CSS variable name) and validating color values against expected formats.
🔒 Proposed fix with whitelist approach
function sanitizeCssToken(str: string): string {
- return str.replace(/[<>"'&;{}\\]/g, "")
+ // Whitelist: allow only alphanumeric, hyphen, underscore, hash, and spaces
+ return str.replace(/[^a-zA-Z0-9_\-#(). ]/g, "")
+}
+
+function sanitizeCssVariableName(str: string): string {
+ // CSS custom property names: only allow alphanumeric, hyphen, underscore
+ return str.replace(/[^a-zA-Z0-9_\-]/g, "")
}Then on line 95:
- return color ? ` --color-${sanitizeCssToken(key)}: ${sanitizeCssToken(color)};` : null
+ return color ? ` --color-${sanitizeCssVariableName(key)}: ${sanitizeCssToken(color)};` : nullAlternatively, if colors are always hex or named CSS colors, validate against a strict pattern like /^#[0-9a-fA-F]{3,8}$|^[a-zA-Z]+$/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/components/ui/chart.tsx` around lines 70 - 72, The
current sanitizeCssToken function's blacklist is incomplete and still allows CSS
injection via characters like parentheses, slash, asterisk, and newlines; update
the sanitizeCssToken implementation to either (1) switch to a strict whitelist
for CSS variable names (e.g., only allow [a-zA-Z0-9-_] and validate against
that) or (2) expand the blacklist to remove parentheses () , forward/back
slashes / \ , asterisk * , newline/CR characters and any other token-delimiting
characters; additionally, if the token is expected to be a color value, validate
against a strict pattern such as a hex color or named color instead of broad
sanitization. Ensure the change is applied to the sanitizeCssToken function so
all callers use the tightened validation.
Widgets (FE-011 to FE-015): - NIST Compliance: maturity gauge, gap analysis table, compliance timeline - Adaptive Balance: spectrum sliders, balance history chart - Value Generation: radar chart, organizational blindness heatmap - Impact Metrics: safety gauge, impact radar, resistance timeline - Cognitive Sandwich: phase stepper, burndown chart CI/CD (FECICD-002 to FECICD-004): - Frontend Dockerfile (multi-stage, standalone, non-root) - Docker Compose with frontend + API services - Frontend deploy pipeline (ACR → AKS staging → prod) - Dependabot npm coverage for frontend deps - CodeQL TypeScript analysis Infrastructure (FECICD-005, FECICD-006): - K8s frontend manifests (deployment, service, configmap, ingress) - K8s overlays (dev: 1 replica, staging: 2, prod: 3 + TLS) - Terraform frontend-hosting module (Azure App Service, Node.js 22) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Frontend grade C→B. 5 widget PRDs built (NIST, Adaptive Balance, Value Gen, Impact Metrics, Cognitive Sandwich). Frontend Docker, K8s manifests, Terraform module, deploy pipeline, Dependabot npm, CodeQL TypeScript all added. 14 items remaining for Phases 16-17. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (13)
.github/dependabot.yml (1)
33-67: LGTM! Well-structured Dependabot configuration for npm.The configuration correctly mirrors the existing nuget block's structure and provides sensible groupings for the major frontend dependencies. The groups for react, next, radix, testing, and storybook will help reduce PR noise from related package updates.
Optional enhancement: Consider adding groups for TypeScript and ESLint packages to further reduce individual PRs:
♻️ Optional: Add typescript and eslint groups
storybook: patterns: - "@storybook/*" - "storybook" + typescript: + patterns: + - "typescript" + - "@types/*" + eslint: + patterns: + - "eslint*" + - "@eslint/*" + - "@typescript-eslint/*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/dependabot.yml around lines 33 - 67, Add explicit Dependabot groups for TypeScript and ESLint to the existing npm block (the one with package-ecosystem: "npm" and groups:) so related updates are bundled; create new group entries named typescript and eslint under groups: and include patterns like "typescript", "@types/*", "ts-*", "eslint*", "eslint-*" and common ESLint plugin/package names so Dependabot groups those deps together (ensure you add the new group keys alongside the existing react/next/radix/testing/storybook groups)..github/workflows/codeql.yml (1)
49-71: New JS/TS analysis job looks good; consider optional Node.js setup for improved TypeScript analysis.The extractorless approach is correct for JavaScript/TypeScript—CodeQL analyzes source files directly without requiring a build. The job structure mirrors the C# job appropriately.
For TypeScript-heavy projects, adding a Node.js setup and
npm cistep can sometimes improve analysis quality by allowing CodeQL to resolve type information and dependencies. This is optional but may reduce false negatives:♻️ Optional: Add Node.js setup for better TypeScript analysis
- name: Checkout repository uses: actions/checkout@v6 with: fetch-depth: 0 + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: '20' + cache: 'npm' + cache-dependency-path: src/UILayer/web/package-lock.json + + - name: Install dependencies + run: npm ci + working-directory: src/UILayer/web + - name: Initialize CodeQL uses: github/codeql-action/init@v4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codeql.yml around lines 49 - 71, Add an optional Node.js setup and dependency install to the analyze-javascript job to improve TypeScript analysis: modify the analyze-javascript job (look for the job name "analyze-javascript" and the steps using "github/codeql-action/init@v4" and "github/codeql-action/analyze@v4") to include a step that sets up Node.js (actions/setup-node) and a step that runs dependency install (e.g., npm ci or pnpm install) before the CodeQL init/analyze steps so CodeQL can resolve types and dependencies for better TypeScript results.infra/variables.tf (1)
140-144: Consider removing the hardcoded default API URL.The default value
https://cognitive-mesh-api.azurewebsites.netassumes a specific backend naming convention. If the backend App Service name changes or varies by environment, this could lead to misconfiguration. Consider either:
- Removing the default and requiring explicit configuration
- Using a computed value based on
project_nameandenvironment♻️ Suggested approach to derive the URL dynamically
variable "frontend_api_base_url" { description = "Base URL for the backend API, consumed by the Next.js frontend." type = string - default = "https://cognitive-mesh-api.azurewebsites.net" + default = null # Must be explicitly set per environment }Then in
main.tf, compute a fallback if needed, or require it via validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/variables.tf` around lines 140 - 144, The variable frontend_api_base_url currently hardcodes "https://cognitive-mesh-api.azurewebsites.net"; remove that hardcoded default from variable "frontend_api_base_url" so the value must be provided (or replace the default with a computed value built from existing variables like project_name and environment), and add validation or a fallback computation in main configuration (e.g., derive "<project_name>-api-<environment>.azurewebsites.net" when appropriate) to prevent misconfiguration; update any modules or references that depended on the default to require an explicit input or to use the new computed fallback.infra/modules/frontend-hosting/main.tf (2)
7-11: SKU override may cause unexpected behavior.The
sku_namelocal overridesvar.app_service_plan_skutoS1for bothprodandstagingenvironments, ignoring any explicit SKU input. This could confuse operators who expect their variable input to be respected. Consider documenting this behavior or using the variable as a minimum baseline.♻️ Alternative: Use variable as minimum, allow higher SKUs
locals { is_production = var.environment == "prod" || var.environment == "staging" - sku_name = local.is_production ? "S1" : var.app_service_plan_sku + # For prod/staging, enforce minimum S1; otherwise use configured SKU + sku_name = local.is_production && var.app_service_plan_sku == "B1" ? "S1" : var.app_service_plan_sku always_on = local.is_production ? true : var.always_on }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/modules/frontend-hosting/main.tf` around lines 7 - 11, The locals block currently forces sku_name to "S1" for prod and staging, overriding user input (see locals, sku_name, var.app_service_plan_sku, local.is_production); change the logic so var.app_service_plan_sku is respected as a minimum (e.g., compare tiers and select the higher/more capable SKU) or clearly document the override behavior; update the sku_name assignment to compute the effective SKU from var.app_service_plan_sku and local.is_production (or add a new variable like min_prod_sku) and adjust any references to locals.sku_name accordingly.
15-25: Consider zone redundancy for production workloads.The App Service Plan doesn't configure zone redundancy, which Checkov flagged (CKV_AZURE_225). For production environments, zone-redundant deployment improves availability. This requires Premium v3 (P1v3+) or Elastic Premium SKUs.
♻️ Add conditional zone redundancy for production
resource "azurerm_service_plan" "this" { name = "${var.project_name}-frontend-plan-${var.environment}" location = var.location resource_group_name = var.resource_group_name os_type = "Linux" sku_name = local.sku_name + zone_balancing_enabled = local.is_production && can(regex("^P[1-3]v[23]$", local.sku_name)) tags = merge(var.common_tags, { Module = "frontend-hosting" }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/modules/frontend-hosting/main.tf` around lines 15 - 25, The App Service Plan resource azurerm_service_plan.this lacks zone redundancy; update it to set zone_redundant = true for production environments (e.g., when var.environment == "prod" or "production") but only when the SKU supports it (ensure local.sku_name corresponds to Premium v3 (P1v3+) or an Elastic Premium SKU); implement a conditional expression around zone_redundant that checks var.environment and local.sku_name (or add a helper local/var like local.enable_zone_redundancy) so non-production SKUs/environments remain unchanged and CI Checkov CKV_AZURE_225 is satisfied.src/UILayer/web/src/components/widgets/NistCompliance/MaturityGauge.tsx (2)
108-114:svgRefin dependency array is unnecessary.Same as noted for
ImpactRadar—refs have stable identity and shouldn't be in the dependency array.🧹 Suggested fix
- }, [render, svgRef]); + }, [render]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/NistCompliance/MaturityGauge.tsx` around lines 108 - 114, The useEffect in MaturityGauge unnecessarily includes svgRef in its dependency array; refs are stable and should not trigger effects — update the effect to depend only on render, keeping the body as-is (const el = svgRef.current; if (el) { const { width, height } = el.getBoundingClientRect(); if (width > 0 && height > 0) render(width, height); }) so the hook reads useEffect(..., [render]) and continues to reference svgRef.current inside the effect.
35-106: SamesvgRefreference pattern asImpactRadar.The
rendercallback referencessvgRef.current(line 37) beforesvgRefis declared (line 106). This works due to closure semantics but is fragile. Consider the same refactor suggested forImpactRadar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/NistCompliance/MaturityGauge.tsx` around lines 35 - 106, The render callback captures svgRef.current before svgRef is declared which is fragile; define svgRef before render and pass it into useD3 (or adjust useD3 to accept an externally created ref) so the ref is created deterministically. Concretely, create const svgRef = useRef<SVGSVGElement | null>(null) (or the same ref type used in ImpactRadar) above the render function, update render to use that svgRef, and then call useD3(render, svgRef) or the equivalent API change so render no longer closes over a later-declared svgRef.src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactRadar.tsx (2)
103-109:svgRefin dependency array is unnecessary.React refs have a stable identity across renders. Including
svgRefin the dependency array doesn't cause harm but is misleading since ref changes don't trigger the effect.🧹 Suggested fix
useEffect(() => { const el = svgRef.current; if (el) { const { width, height } = el.getBoundingClientRect(); if (width > 0 && height > 0) render(width, height); } - }, [render, svgRef]); + }, [render]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactRadar.tsx` around lines 103 - 109, The effect's dependency array incorrectly includes the stable ref variable svgRef; remove svgRef from the dependencies so the useEffect depends only on render (i.e., change the dependency array in the useEffect that references svgRef.current and render to [render]) to avoid implying the ref will trigger re-runs—keep the body using svgRef.current and render as-is.
18-101:svgRefreferenced before declaration causes potential issues.The
rendercallback at line 20 referencessvgRef.current, butsvgRefis declared later at line 101 viauseD3(render). This creates a circular dependency whererenderneedssvgRef, butsvgRefis obtained by passingrendertouseD3.This works only because
useCallbackcreates a closure that capturessvgRefby reference after hoisting, andrenderisn't invoked until aftersvgRefis assigned. However, this pattern is fragile and confusing—consider restructuring so the ref is passed into the render function or obtained inside it.♻️ Suggested refactor: pass svgRef into render
const render = useCallback( - (width: number, height: number) => { - const svg = d3.select(svgRef.current); + (width: number, height: number, ref: SVGSVGElement | null) => { + const svg = d3.select(ref); if (!svg.node()) return;Then update the
useD3hook contract or the call sites to pass the ref accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactRadar.tsx` around lines 18 - 101, The render callback captures svgRef.current before svgRef is created, creating a fragile ordering dependency; fix by removing the closure dependency: either obtain svgRef first (call useD3 to get svgRef before defining render) or change render to accept the ref as an argument and update useD3 to call render(svgRef) (i.e., adjust the useD3 contract to pass the ref into the render function) so the code uses svgRef deterministically; update references to svgRef in render and the useD3 invocation accordingly (symbols: render, svgRef, useD3).src/UILayer/web/src/components/widgets/AdaptiveBalance/SpectrumSlider.tsx (1)
29-36: Minor: Reuse thepctvariable instead of recalculating.Line 36 recalculates
dim.value * 100whenpctis already available from line 29.const pct = dim.value * 100; ... -<span className="text-xs text-gray-400">{(dim.value * 100).toFixed(0)}%</span> +<span className="text-xs text-gray-400">{pct.toFixed(0)}%</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/AdaptiveBalance/SpectrumSlider.tsx` around lines 29 - 36, The JSX recomputes dim.value * 100 instead of reusing the previously computed pct; update the span that renders the percentage in the SpectrumSlider component to use the pct variable (declared above) rather than recalculating dim.value * 100 so the code uses the existing pct value consistently (look for pct, dim.value, and the percentage span in SpectrumSlider.tsx).src/UILayer/web/src/components/widgets/NistCompliance/NistComplianceDashboard.tsx (1)
126-128: Consider extracting the score scale as a constant.The percentage calculation assumes
averageScoreis on a 0-5 scale. If this scale is defined by the API contract, consider extracting it as a named constant for clarity and maintainability.+const MAX_PILLAR_SCORE = 5; + {scoreData.pillarScores.map((ps) => { - const pct = (ps.averageScore / 5) * 100; + const pct = (ps.averageScore / MAX_PILLAR_SCORE) * 100;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/NistCompliance/NistComplianceDashboard.tsx` around lines 126 - 128, Extract the hard-coded 5 used to compute percentages into a named constant (e.g., SCORE_SCALE_MAX or MAX_AVERAGE_SCORE) inside the NistComplianceDashboard module and replace the literal in the percentage calculation for scoreData.pillarScores (where you compute const pct = (ps.averageScore / 5) * 100) with the constant; ensure the constant is documented briefly and used wherever the 0–5 scale is assumed so changes to the API scale require updating only that constant.src/UILayer/web/src/components/widgets/ValueGeneration/BlindnessHeatmap.tsx (1)
49-56: Consider using spot content as key if blind spots are unique.Using array index as
key(line 51) is acceptable for static display lists, but if blind spots are guaranteed unique strings, using the content directly would be more robust against future list manipulations.-key={i} +key={spot}If duplicates are possible, the current approach is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/ValueGeneration/BlindnessHeatmap.tsx` around lines 49 - 56, The map is using the array index as the React key in the blindSpots.map within BlindnessHeatmap (key={i}); if blindSpots elements are guaranteed unique strings, replace the index with the spot value (key={spot}) to make keys stable across reorderings; if duplicates are possible, use a deterministic composite key (e.g., `${spot}-${i}`) instead to avoid collisions while keeping severityClass and rendering unchanged.src/UILayer/web/src/components/widgets/ImpactMetrics/SafetyGauge.tsx (1)
25-85: Consider reordering for clarity:svgRefis referenced before its declaration.The
rendercallback on line 27 referencessvgRef.current, butsvgRefis only obtained fromuseD3on line 85. While this works at runtime (closures capture references, not values, and the callback isn't invoked until after mounting), the ordering can be confusing for readers.♻️ Suggested reorder for clarity
export default function SafetyGauge({ score, label }: SafetyGaugeProps) { + const { svgRef } = useD3((width, height) => { + // render logic moved here or keep separate + }); + const render = useCallback( (width: number, height: number) => { const svg = d3.select(svgRef.current); // ... }, [score, label], ); - const { svgRef } = useD3(render);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UILayer/web/src/components/widgets/ImpactMetrics/SafetyGauge.tsx` around lines 25 - 85, The render callback references svgRef.current before svgRef is defined, which is confusing; move the useD3 call so that const { svgRef } = useD3(render) is declared before the render useCallback (or alternatively create svgRef via useRef and pass it into useD3) to ensure svgRef is defined prior to being referenced in render — update references to the render function passed into useD3 accordingly (symbols: render, svgRef, useD3).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/state/orchestrator.json:
- Around line 142-148: The "phase" field in the JSON state uses a string "15a"
which is inconsistent with prior integer phases and will break numeric
comparisons; update the entry so "phase" is an integer (e.g., 15) and add a
separate field to capture the sub-phase (e.g., "batch": "a") or, if you choose
consistent string typing across all entries, update all previous "phase" values
to strings—modify the object containing "phase": "15a" and add the new
distinguishing field (or convert existing phases) so type consistency is
enforced for the phase key.
- Around line 189-193: The backlog's numeric `done` counters are inconsistent
with the `[DONE]` markers in the item arrays for p2_medium_widgets and
p3_low_advanced; update either the `done` values or the item strings so they
match. Specifically, for p2_medium_widgets (items FE-016, FE-017, FE-018,
FE-019, FE-020) ensure four items are marked `[DONE]` or change `"done": 4` to
the actual number of `[DONE]` markers present, and for p3_low_advanced (items
FE-024, FE-025, FE-026, FE-027, FE-028) ensure three items are marked `[DONE]`
or adjust `"done": 3` to match the count of `[DONE]` in the items array; keep
the format consistent across all categories.
In @.github/workflows/deploy-frontend.yml:
- Around line 97-98: The workflow currently hard-codes
NEXT_PUBLIC_API_BASE_URL=https://api.cognitivemesh.io into the built frontend,
causing the same image to call production from staging; change the build
strategy so the frontend bundle does not bake a production NEXT_PUBLIC_* value:
either build separate images per environment with environment-specific build-arg
values (e.g., set NEXT_PUBLIC_API_BASE_URL dynamically per job), or stop using
NEXT_PUBLIC_API_BASE_URL in client code and instead use a server-read env var
(non-NEXT_PUBLIC_) that the server injects into pages/runtime config at request
time; update the deploy pipeline to pass per-environment build-args or modify
the app to read a server env var (remove reliance on NEXT_PUBLIC_API_BASE_URL
for runtime API host).
In `@docker-compose.yml`:
- Around line 35-41: Update the NEXT_PUBLIC_API_BASE_URL value so the
build-embedded public env var points to a browser-reachable URL (e.g., use
http://localhost:8080 instead of the internal Docker hostname http://api:8080)
and move the internal service hostname into a non-public variable (e.g.,
API_INTERNAL_HOST or similar) for container-to-container communication; change
the docker-compose service block that sets NEXT_PUBLIC_API_BASE_URL and add a
separate non-public env var for internal use so Next.js client-side code
(AuthContext, useSignalR, page components, API clients) receives a resolvable
URL at runtime.
In `@k8s/base/frontend-configmap.yaml`:
- Around line 13-15: SUMMARY: NEXT_PUBLIC_API_BASE_URL is currently set to an
internal cluster hostname that will be inlined at Next.js build time and not
reachable from browsers; change it to a relative path. Update the ConfigMap
entry NEXT_PUBLIC_API_BASE_URL in frontend-configmap.yaml to "/api" (remove
"http://cognitive-mesh-api"), then rebuild/redeploy the frontend so Next.js
picks up the relative API base; reference the NEXT_PUBLIC_API_BASE_URL key to
locate the change.
In `@k8s/base/frontend-deployment.yaml`:
- Around line 24-50: Add a container-level securityContext to the
cognitive-mesh-frontend container: set allowPrivilegeEscalation: false and
readOnlyRootFilesystem: true (and keep existing pod-level runAsNonRoot). Ensure
the existing /tmp emptyDir mount remains present and mounted at /tmp with
writable permission so the container can write to /tmp despite the read-only
root filesystem. Update the container spec for the container named
cognitive-mesh-frontend to include these fields.
In `@k8s/base/frontend-ingress.yaml`:
- Around line 12-15: Remove the global
nginx.ingress.kubernetes.io/rewrite-target: / annotation from the Ingress
manifest so requests keep their original paths (this annotation currently
rewrites everything to / and breaks path-based routing and health checks); if
you actually need to strip a prefix like /api, replace the global annotation
with nginx.ingress.kubernetes.io/rewrite-target: /$2 and ensure the
corresponding path uses a capture group (e.g., a regex path that captures the
prefix and the rest) so only the intended prefix is removed; update occurrences
of nginx.ingress.kubernetes.io/rewrite-target in this file (and the other noted
blocks) accordingly and verify /healthz and /readyz reach the backend unchanged.
In
`@src/UILayer/web/src/components/widgets/AdaptiveBalance/AdaptiveBalanceDashboard.tsx`:
- Around line 28-48: The fetchData useCallback currently includes selectedDim in
its dependency array causing refetches when the user changes the history
dropdown; remove selectedDim from the dependency list of fetchData so it does
not re-run on local selection changes, and ensure default-dimension selection
still happens after the initial load by setting selectedDim inside fetchData (or
in a separate one-time effect) only when balance.dimensions exists and
selectedDim is empty; update references to fetchData (the useCallback) and the
effect that calls it (the useEffect that triggers the initial load) so they rely
on stable dependencies and do not include selectedDim.
- Around line 51-60: When selectedDim changes, clear the old history immediately
so the UI doesn't show stale data: inside the useEffect that watches selectedDim
(the block using getSpectrumHistory, cancelled, setDimHistory), call
setDimHistory(null) (or a loading state) before invoking
getSpectrumHistory(selectedDim) so the chart is cleared while the new request is
in flight; keep the existing cancelled logic and the .then/.catch handlers to
setDimHistory(h) or setDimHistory(null) on resolution.
In `@src/UILayer/web/src/components/widgets/AdaptiveBalance/BalanceHistory.tsx`:
- Around line 39-43: The history array is consumed in input order which can be
unsorted; before deriving dates or building the area/line series in the
BalanceHistory component, create and use a sorted copy of history (e.g., sort by
numeric timestamp) so dates, the d3.scaleTime domain (dates const) and the
area/line path generation (the code around lines where area/line are created,
previously using history directly) all operate on time-ascending data; update
usages of history (dates mapping and the area/line generation) to use this
sortedHistory copy.
In `@src/UILayer/web/src/components/widgets/api.ts`:
- Around line 11-17: The helper getApiBaseUrl currently falls back to
'http://localhost:5000' which is unsafe for client-side code; change
getApiBaseUrl (and the BASE const) to not default to localhost—either throw an
error or return an empty string when process.env.NEXT_PUBLIC_API_BASE_URL is
missing so production bundles fail fast (or explicitly surface the missing
config) instead of pointing the browser at the user's localhost; update callers
to handle the missing value accordingly (refer to getApiBaseUrl and BASE).
In
`@src/UILayer/web/src/components/widgets/CognitiveSandwich/CognitiveSandwichDashboard.tsx`:
- Around line 20-37: The fetchData callback can be clobbered by slower earlier
requests; add a request-id guard using a useRef counter (e.g., requestIdRef)
incremented before each async call inside fetchData, capture the currentId in
the async scope, and only call setProcess, setAudit, setDebt, setError, and
setLoading if requestIdRef.current === currentId; keep the useCallback signature
(fetchData, processId) and ensure the counter ref is defined outside fetchData
so overlapping calls are ignored when stale.
- Line 121: The server-rendered timestamp uses new
Date(e.timestamp).toLocaleString(), which can differ between SSR and client and
cause hydration mismatch; update CognitiveSandwichDashboard to produce a
deterministic timestamp string by either formatting with Intl.DateTimeFormat
using explicit locale and options (e.g., Intl.DateTimeFormat('en-US', {
dateStyle: 'medium', timeStyle: 'short' }).format(new Date(e.timestamp))) or
compute formatting only on the client (use useEffect/useState to format
e.timestamp after mount) and render a placeholder server-side, and replace all
occurrences of new Date(e.timestamp).toLocaleString() accordingly (referencing
e.timestamp and the CognitiveSandwichDashboard component).
In
`@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactMetricsDashboard.tsx`:
- Line 3: The component ImpactMetricsDashboard has an out-of-order response bug
where older async fetches can overwrite newer tenant-scoped state; fix it by
adding a per-request token (e.g., incrementing requestId or using
AbortController) inside the useEffect/async fetch logic that produces the report
(the fetch function that sets report state, e.g., fetchImpactMetricsReport or
the effect that calls it) and only commit the result when the token matches the
latest token for that tenant; alternatively cancel previous requests via
AbortController and check signal.aborted before calling
setReport/commitTenantScopedState so stale responses are ignored.
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactRadar.tsx`:
- Around line 7-12: The ImpactRadar component should validate that
ImpactRadarProps.labels and ImpactRadarProps.values have the same length before
rendering; inside the ImpactRadar component (where props of type
ImpactRadarProps are received) add a guard that checks if labels.length !==
values.length and handle it (e.g., console.warn with both lengths and component
name, then either return null or proceed using Math.min(labels.length,
values.length) to align data consistently). Ensure the check references the
ImpactRadarProps labels and values so the chart rendering logic (axes/data
mapping) uses matched array lengths or aborts rendering with a clear warning.
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactTimeline.tsx`:
- Line 30: The date rendering in ImpactTimeline.tsx is non-deterministic because
new Date(ind.detectedAt).toLocaleDateString() relies on environment
locale/timezone and causes hydration mismatches; replace it with a deterministic
formatter such as Intl.DateTimeFormat with an explicit locale and timeZone
(e.g., use new Intl.DateTimeFormat('en-US', { year:'numeric', month:'short',
day:'numeric', timeZone:'UTC' }).format(new Date(ind.detectedAt'))) so both
server and client produce the same string; update the JSX where ind.detectedAt
is used in the <p> line in the ImpactTimeline component accordingly.
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/SafetyGauge.tsx`:
- Around line 95-101: The aria-label on the SVG in SafetyGauge currently uses
Math.round(score) which can differ from the visual because the chart uses
clampedScore; update the aria-label to use the same clamped value (clampedScore)
and rounded form so the accessible text matches the rendered gauge, or move the
clamping logic into a shared variable at the top of the component and reference
that variable in both the rendering and the aria-label (ensure references to
svgRef and existing label logic remain unchanged).
In `@src/UILayer/web/src/components/widgets/NistCompliance/GapAnalysisTable.tsx`:
- Around line 72-84: The table headers rendered by renderHeader are not
keyboard-accessible and don't expose sort state; change the <th> to include an
aria-sort attribute derived from sortField and sortDir (e.g., aria-sort={active
? (sortDir === 'asc' ? 'ascending' : 'descending') : 'none'}), move the click
target to a <button type="button"> wrapping the label (and call
handleSort(field) from the button onClick), and mark the visual arrow string as
aria-hidden so it isn't read twice; ensure focus styles remain and that
renderHeader continues to use sortField, sortDir, and handleSort to determine
active state.
In
`@src/UILayer/web/src/components/widgets/ValueGeneration/ValueGenerationDashboard.tsx`:
- Line 3: The component ValueGenerationDashboard is vulnerable to stale-response
overwrites; add a request-scoping guard (either an AbortController signal or an
incrementing requestId token) inside the async fetch/refresh handlers (e.g.,
refreshDashboard / loadDashboard used in the useEffect and the Refresh button
callback) and only call state setters (setDashboardState / setLoading /
setError) if the current requestId matches the latest or the AbortSignal is not
aborted; ensure the token/controller is updated before each new request and
cleaned up on unmount to prevent older responses from mutating state.
In `@src/UILayer/web/src/components/widgets/ValueGeneration/ValueRadarChart.tsx`:
- Around line 24-34: The component renders the radar grid using axes.length but
draws the polygon/dots using values, so mismatched lengths cause misaligned
points; in the ValueRadarChart component validate that values.length ===
axes.length (and that both are non-empty) before drawing, and if they differ
display the same "No data for radar chart." fallback (or a clear error message)
and return early; update places that assume axes/values parity (the early-return
block that currently checks only axes.length and the polygon/dot-rendering
logic) to use this parity check and avoid drawing when lengths mismatch.
---
Nitpick comments:
In @.github/dependabot.yml:
- Around line 33-67: Add explicit Dependabot groups for TypeScript and ESLint to
the existing npm block (the one with package-ecosystem: "npm" and groups:) so
related updates are bundled; create new group entries named typescript and
eslint under groups: and include patterns like "typescript", "@types/*", "ts-*",
"eslint*", "eslint-*" and common ESLint plugin/package names so Dependabot
groups those deps together (ensure you add the new group keys alongside the
existing react/next/radix/testing/storybook groups).
In @.github/workflows/codeql.yml:
- Around line 49-71: Add an optional Node.js setup and dependency install to the
analyze-javascript job to improve TypeScript analysis: modify the
analyze-javascript job (look for the job name "analyze-javascript" and the steps
using "github/codeql-action/init@v4" and "github/codeql-action/analyze@v4") to
include a step that sets up Node.js (actions/setup-node) and a step that runs
dependency install (e.g., npm ci or pnpm install) before the CodeQL init/analyze
steps so CodeQL can resolve types and dependencies for better TypeScript
results.
In `@infra/modules/frontend-hosting/main.tf`:
- Around line 7-11: The locals block currently forces sku_name to "S1" for prod
and staging, overriding user input (see locals, sku_name,
var.app_service_plan_sku, local.is_production); change the logic so
var.app_service_plan_sku is respected as a minimum (e.g., compare tiers and
select the higher/more capable SKU) or clearly document the override behavior;
update the sku_name assignment to compute the effective SKU from
var.app_service_plan_sku and local.is_production (or add a new variable like
min_prod_sku) and adjust any references to locals.sku_name accordingly.
- Around line 15-25: The App Service Plan resource azurerm_service_plan.this
lacks zone redundancy; update it to set zone_redundant = true for production
environments (e.g., when var.environment == "prod" or "production") but only
when the SKU supports it (ensure local.sku_name corresponds to Premium v3
(P1v3+) or an Elastic Premium SKU); implement a conditional expression around
zone_redundant that checks var.environment and local.sku_name (or add a helper
local/var like local.enable_zone_redundancy) so non-production SKUs/environments
remain unchanged and CI Checkov CKV_AZURE_225 is satisfied.
In `@infra/variables.tf`:
- Around line 140-144: The variable frontend_api_base_url currently hardcodes
"https://cognitive-mesh-api.azurewebsites.net"; remove that hardcoded default
from variable "frontend_api_base_url" so the value must be provided (or replace
the default with a computed value built from existing variables like
project_name and environment), and add validation or a fallback computation in
main configuration (e.g., derive
"<project_name>-api-<environment>.azurewebsites.net" when appropriate) to
prevent misconfiguration; update any modules or references that depended on the
default to require an explicit input or to use the new computed fallback.
In `@src/UILayer/web/src/components/widgets/AdaptiveBalance/SpectrumSlider.tsx`:
- Around line 29-36: The JSX recomputes dim.value * 100 instead of reusing the
previously computed pct; update the span that renders the percentage in the
SpectrumSlider component to use the pct variable (declared above) rather than
recalculating dim.value * 100 so the code uses the existing pct value
consistently (look for pct, dim.value, and the percentage span in
SpectrumSlider.tsx).
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactRadar.tsx`:
- Around line 103-109: The effect's dependency array incorrectly includes the
stable ref variable svgRef; remove svgRef from the dependencies so the useEffect
depends only on render (i.e., change the dependency array in the useEffect that
references svgRef.current and render to [render]) to avoid implying the ref will
trigger re-runs—keep the body using svgRef.current and render as-is.
- Around line 18-101: The render callback captures svgRef.current before svgRef
is created, creating a fragile ordering dependency; fix by removing the closure
dependency: either obtain svgRef first (call useD3 to get svgRef before defining
render) or change render to accept the ref as an argument and update useD3 to
call render(svgRef) (i.e., adjust the useD3 contract to pass the ref into the
render function) so the code uses svgRef deterministically; update references to
svgRef in render and the useD3 invocation accordingly (symbols: render, svgRef,
useD3).
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/SafetyGauge.tsx`:
- Around line 25-85: The render callback references svgRef.current before svgRef
is defined, which is confusing; move the useD3 call so that const { svgRef } =
useD3(render) is declared before the render useCallback (or alternatively create
svgRef via useRef and pass it into useD3) to ensure svgRef is defined prior to
being referenced in render — update references to the render function passed
into useD3 accordingly (symbols: render, svgRef, useD3).
In `@src/UILayer/web/src/components/widgets/NistCompliance/MaturityGauge.tsx`:
- Around line 108-114: The useEffect in MaturityGauge unnecessarily includes
svgRef in its dependency array; refs are stable and should not trigger effects —
update the effect to depend only on render, keeping the body as-is (const el =
svgRef.current; if (el) { const { width, height } = el.getBoundingClientRect();
if (width > 0 && height > 0) render(width, height); }) so the hook reads
useEffect(..., [render]) and continues to reference svgRef.current inside the
effect.
- Around line 35-106: The render callback captures svgRef.current before svgRef
is declared which is fragile; define svgRef before render and pass it into useD3
(or adjust useD3 to accept an externally created ref) so the ref is created
deterministically. Concretely, create const svgRef = useRef<SVGSVGElement |
null>(null) (or the same ref type used in ImpactRadar) above the render
function, update render to use that svgRef, and then call useD3(render, svgRef)
or the equivalent API change so render no longer closes over a later-declared
svgRef.
In
`@src/UILayer/web/src/components/widgets/NistCompliance/NistComplianceDashboard.tsx`:
- Around line 126-128: Extract the hard-coded 5 used to compute percentages into
a named constant (e.g., SCORE_SCALE_MAX or MAX_AVERAGE_SCORE) inside the
NistComplianceDashboard module and replace the literal in the percentage
calculation for scoreData.pillarScores (where you compute const pct =
(ps.averageScore / 5) * 100) with the constant; ensure the constant is
documented briefly and used wherever the 0–5 scale is assumed so changes to the
API scale require updating only that constant.
In `@src/UILayer/web/src/components/widgets/ValueGeneration/BlindnessHeatmap.tsx`:
- Around line 49-56: The map is using the array index as the React key in the
blindSpots.map within BlindnessHeatmap (key={i}); if blindSpots elements are
guaranteed unique strings, replace the index with the spot value (key={spot}) to
make keys stable across reorderings; if duplicates are possible, use a
deterministic composite key (e.g., `${spot}-${i}`) instead to avoid collisions
while keeping severityClass and rendering unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 838f4418-0e29-4f7f-9b3f-261b046af315
📒 Files selected for processing (53)
.claude/state/orchestrator.json.github/dependabot.yml.github/workflows/codeql.yml.github/workflows/deploy-frontend.ymldocker-compose.ymlinfra/main.tfinfra/modules/frontend-hosting/main.tfinfra/modules/frontend-hosting/outputs.tfinfra/modules/frontend-hosting/variables.tfinfra/outputs.tfinfra/variables.tfk8s/base/frontend-configmap.yamlk8s/base/frontend-deployment.yamlk8s/base/frontend-ingress.yamlk8s/base/frontend-service.yamlk8s/base/kustomization.yamlk8s/overlays/dev/kustomization.yamlk8s/overlays/prod/kustomization.yamlk8s/overlays/staging/kustomization.yamlsrc/UILayer/web/Dockerfilesrc/UILayer/web/next.config.jssrc/UILayer/web/src/app/(app)/balance/page.tsxsrc/UILayer/web/src/app/(app)/compliance/page.tsxsrc/UILayer/web/src/app/(app)/impact/page.tsxsrc/UILayer/web/src/app/(app)/sandwich/page.tsxsrc/UILayer/web/src/app/(app)/value/page.tsxsrc/UILayer/web/src/app/api/health/route.tssrc/UILayer/web/src/components/Navigation/navItems.tssrc/UILayer/web/src/components/widgets/AdaptiveBalance/AdaptiveBalanceDashboard.tsxsrc/UILayer/web/src/components/widgets/AdaptiveBalance/BalanceHistory.tsxsrc/UILayer/web/src/components/widgets/AdaptiveBalance/SpectrumSlider.tsxsrc/UILayer/web/src/components/widgets/AdaptiveBalance/index.tssrc/UILayer/web/src/components/widgets/CognitiveSandwich/BurndownChart.tsxsrc/UILayer/web/src/components/widgets/CognitiveSandwich/CognitiveSandwichDashboard.tsxsrc/UILayer/web/src/components/widgets/CognitiveSandwich/PhaseStepper.tsxsrc/UILayer/web/src/components/widgets/CognitiveSandwich/index.tssrc/UILayer/web/src/components/widgets/ImpactMetrics/ImpactMetricsDashboard.tsxsrc/UILayer/web/src/components/widgets/ImpactMetrics/ImpactRadar.tsxsrc/UILayer/web/src/components/widgets/ImpactMetrics/ImpactTimeline.tsxsrc/UILayer/web/src/components/widgets/ImpactMetrics/SafetyGauge.tsxsrc/UILayer/web/src/components/widgets/ImpactMetrics/index.tssrc/UILayer/web/src/components/widgets/NistCompliance/ComplianceTimeline.tsxsrc/UILayer/web/src/components/widgets/NistCompliance/GapAnalysisTable.tsxsrc/UILayer/web/src/components/widgets/NistCompliance/MaturityGauge.tsxsrc/UILayer/web/src/components/widgets/NistCompliance/NistComplianceDashboard.tsxsrc/UILayer/web/src/components/widgets/NistCompliance/index.tssrc/UILayer/web/src/components/widgets/ValueGeneration/BlindnessHeatmap.tsxsrc/UILayer/web/src/components/widgets/ValueGeneration/ValueGenerationDashboard.tsxsrc/UILayer/web/src/components/widgets/ValueGeneration/ValueRadarChart.tsxsrc/UILayer/web/src/components/widgets/ValueGeneration/index.tssrc/UILayer/web/src/components/widgets/api.tssrc/UILayer/web/src/components/widgets/types.tssrc/UILayer/web/src/lib/code-splitting/registry/lazyWidgets.ts
Comment on lines
+142
to
148
| { | ||
| "phase": "15a", | ||
| "timestamp": "2026-03-11T00:00:00Z", | ||
| "teams": ["frontend"], | ||
| "result": "success", | ||
| "notes": "Batch A: Settings page (theme/font/a11y/privacy/language), Notification preferences (channels/categories/quiet hours), User profile (GDPR consent, JWT auth timestamp, data export). 40 code quality fixes across 32 files (backend + frontend). PR #359 open. 4 Renovate PRs merged." | ||
| } |
Contributor
There was a problem hiding this comment.
Type inconsistency in phase identifier.
Phase 15a uses a string ("15a") while all previous phases (1–14) use integers. If any consuming code performs numeric comparisons, sorting, or expects consistent typing on the phase field, this mismatch could cause unexpected behavior.
Consider using either:
- Integer
15with a distinguishing field (e.g.,"batch": "a") - Consistent string type for all phases (would require updating prior entries)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/state/orchestrator.json around lines 142 - 148, The "phase" field in
the JSON state uses a string "15a" which is inconsistent with prior integer
phases and will break numeric comparisons; update the entry so "phase" is an
integer (e.g., 15) and add a separate field to capture the sub-phase (e.g.,
"batch": "a") or, if you choose consistent string typing across all entries,
update all previous "phase" values to strings—modify the object containing
"phase": "15a" and add the new distinguishing field (or convert existing phases)
so type consistency is enforced for the phase key.
Comment on lines
+189
to
+193
| "p2_medium_widgets": { "total": 5, "done": 4, "items": ["FE-016 Context Eng", "FE-017 Agentic System [DONE]", "FE-018 Convener", "FE-019 Marketplace", "FE-020 Org Mesh"] }, | ||
| "p2_medium_app": { "total": 3, "done": 1, "items": ["FE-021 Multi-page routing", "FE-022 Navigation [DONE]", "FE-023 Role-based UI"] }, | ||
| "p2_medium_cicd": { "total": 6, "done": 6, "items": ["FECICD-001 CI pipeline [DONE]", "FECICD-002 Docker [DONE]", "FECICD-003 Compose [DONE]", "FECICD-004 Deploy [DONE]", "FECICD-005 K8s [DONE]", "FECICD-006 Terraform [DONE]"] }, | ||
| "p2_medium_testing": { "total": 5, "done": 0, "items": ["FETEST-001 Unit tests 80%", "FETEST-002 API integration", "FETEST-003 E2E real API", "FETEST-004 Visual regression", "FETEST-005 Lighthouse CI"] }, | ||
| "p3_low_advanced": { "total": 5, "done": 0, "items": ["FE-024 Export", "FE-025 Cmd+K", "FE-026 Collaboration", "FE-027 Locales", "FE-028 PWA"] } | ||
| "p3_low_advanced": { "total": 5, "done": 3, "items": ["FE-024 Export", "FE-025 Cmd+K", "FE-026 Collaboration", "FE-027 Locales [DONE]", "FE-028 PWA [DONE]"] } |
Contributor
There was a problem hiding this comment.
Backlog item markers inconsistent with done counts.
The done counts don't match the [DONE] markers in the item lists:
| Category | done |
Items marked [DONE] |
|---|---|---|
p2_medium_widgets |
4 | 1 (FE-017 only) |
p3_low_advanced |
3 | 2 (FE-027, FE-028) |
Either update the item strings to include [DONE] markers on the completed items, or correct the done counts. Inconsistent state can cause confusion when reviewing progress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/state/orchestrator.json around lines 189 - 193, The backlog's
numeric `done` counters are inconsistent with the `[DONE]` markers in the item
arrays for p2_medium_widgets and p3_low_advanced; update either the `done`
values or the item strings so they match. Specifically, for p2_medium_widgets
(items FE-016, FE-017, FE-018, FE-019, FE-020) ensure four items are marked
`[DONE]` or change `"done": 4` to the actual number of `[DONE]` markers present,
and for p3_low_advanced (items FE-024, FE-025, FE-026, FE-027, FE-028) ensure
three items are marked `[DONE]` or adjust `"done": 3` to match the count of
`[DONE]` in the items array; keep the format consistent across all categories.
Comment on lines
+97
to
+98
| build-args: | | ||
| NEXT_PUBLIC_API_BASE_URL=https://api.cognitivemesh.io |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Next.js App Router, when browser code reads a NEXT_PUBLIC_*environment variable, is that value resolved atnext build time or can it be changed later at container runtime?
💡 Result:
In the App Router, any process.env.NEXT_PUBLIC_* used in browser/client code is resolved at next build time: Next.js inlines (“hard-codes”) the value into the JavaScript bundle that gets shipped to the browser. After the image/artifact is built, changing the container runtime environment variables will not change what the browser sees—you must rebuild to change those values. [1][2][3]
(Non-NEXT_PUBLIC_* env vars can still be read at runtime on the server side, but they are not directly available to browser code.) [1]
This build bakes the production API URL into the staging image.
Lines 97-98 always build the frontend with NEXT_PUBLIC_API_BASE_URL=https://api.cognitivemesh.io, but the same image is later deployed to staging and production. In Next.js, NEXT_PUBLIC_* values used by browser code are hard-coded into the JavaScript bundle at build time, so the staging ConfigMap cannot override this later—staging will call production. Either build separate images per environment, or use a non-NEXT_PUBLIC_* variable for the API base URL and read it from the server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy-frontend.yml around lines 97 - 98, The workflow
currently hard-codes NEXT_PUBLIC_API_BASE_URL=https://api.cognitivemesh.io into
the built frontend, causing the same image to call production from staging;
change the build strategy so the frontend bundle does not bake a production
NEXT_PUBLIC_* value: either build separate images per environment with
environment-specific build-arg values (e.g., set NEXT_PUBLIC_API_BASE_URL
dynamically per job), or stop using NEXT_PUBLIC_API_BASE_URL in client code and
instead use a server-read env var (non-NEXT_PUBLIC_) that the server injects
into pages/runtime config at request time; update the deploy pipeline to pass
per-environment build-args or modify the app to read a server env var (remove
reliance on NEXT_PUBLIC_API_BASE_URL for runtime API host).
Comment on lines
+35
to
+41
| args: | ||
| NEXT_PUBLIC_API_BASE_URL: http://api:8080 | ||
| container_name: cognitive-mesh-frontend | ||
| ports: | ||
| - "3000:3000" | ||
| environment: | ||
| - NEXT_PUBLIC_API_BASE_URL=http://api:8080 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== References to NEXT_PUBLIC_API_BASE_URL =="
rg -n -C2 'NEXT_PUBLIC_API_BASE_URL|process\.env\.NEXT_PUBLIC_API_BASE_URL' src/UILayer/web/src
echo
echo "== Client components that reference NEXT_PUBLIC_API_BASE_URL =="
rg -l 'NEXT_PUBLIC_API_BASE_URL|process\.env\.NEXT_PUBLIC_API_BASE_URL' src/UILayer/web/src | while read -r f; do
if rg -q '^[[:space:]]*["'\'']use client["'\'']' "$f"; then
echo "$f"
fi
doneRepository: phoenixvc/cognitive-mesh
Length of output: 3129
Using an internal Docker hostname in a public frontend env var breaks browser API calls.
Lines 35-41 set NEXT_PUBLIC_API_BASE_URL to http://api:8080. Since this variable is prefixed with NEXT_PUBLIC_, Next.js embeds it into the browser bundle at build time. When the browser tries to fetch from http://api:8080 (used in AuthContext, useSignalR hook, page components, and API clients), the hostname is not resolvable from outside the Docker network, causing all API calls to fail. Runtime environment variables cannot override values already compiled into the bundle.
Use a browser-reachable public URL (e.g., http://localhost:8080), and store any internal service URL in a separate non-public variable:
Suggested fix
frontend:
build:
context: src/UILayer/web
dockerfile: Dockerfile
args:
- NEXT_PUBLIC_API_BASE_URL: http://api:8080
+ NEXT_PUBLIC_API_BASE_URL: http://localhost:8080
environment:
- - NEXT_PUBLIC_API_BASE_URL=http://api:8080
+ - NEXT_PUBLIC_API_BASE_URL=http://localhost:8080
+ - INTERNAL_API_BASE_URL=http://api:8080📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| args: | |
| NEXT_PUBLIC_API_BASE_URL: http://api:8080 | |
| container_name: cognitive-mesh-frontend | |
| ports: | |
| - "3000:3000" | |
| environment: | |
| - NEXT_PUBLIC_API_BASE_URL=http://api:8080 | |
| args: | |
| NEXT_PUBLIC_API_BASE_URL: http://localhost:8080 | |
| container_name: cognitive-mesh-frontend | |
| ports: | |
| - "3000:3000" | |
| environment: | |
| - NEXT_PUBLIC_API_BASE_URL=http://localhost:8080 | |
| - INTERNAL_API_BASE_URL=http://api:8080 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 35 - 41, Update the NEXT_PUBLIC_API_BASE_URL
value so the build-embedded public env var points to a browser-reachable URL
(e.g., use http://localhost:8080 instead of the internal Docker hostname
http://api:8080) and move the internal service hostname into a non-public
variable (e.g., API_INTERNAL_HOST or similar) for container-to-container
communication; change the docker-compose service block that sets
NEXT_PUBLIC_API_BASE_URL and add a separate non-public env var for internal use
so Next.js client-side code (AuthContext, useSignalR, page components, API
clients) receives a resolvable URL at runtime.
Comment on lines
+13
to
+15
| data: | ||
| NEXT_PUBLIC_API_BASE_URL: "http://cognitive-mesh-api" | ||
| NODE_ENV: "production" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files k8s/ | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 408
🏁 Script executed:
git ls-files | grep -E "(Dockerfile|next\.config|frontend)" | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 462
🏁 Script executed:
cat k8s/base/frontend-configmap.yamlRepository: phoenixvc/cognitive-mesh
Length of output: 616
🏁 Script executed:
find . -name "*frontend*" -o -name "*kustomization*" | grep -v node_modules | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 462
🏁 Script executed:
cat src/UILayer/web/DockerfileRepository: phoenixvc/cognitive-mesh
Length of output: 2461
🏁 Script executed:
cat src/UILayer/web/next.config.jsRepository: phoenixvc/cognitive-mesh
Length of output: 535
🏁 Script executed:
cat k8s/base/frontend-deployment.yamlRepository: phoenixvc/cognitive-mesh
Length of output: 2356
🏁 Script executed:
cat k8s/overlays/dev/kustomization.yamlRepository: phoenixvc/cognitive-mesh
Length of output: 3524
🏁 Script executed:
cat k8s/overlays/prod/kustomization.yamlRepository: phoenixvc/cognitive-mesh
Length of output: 4138
🏁 Script executed:
rg "NEXT_PUBLIC_API_BASE_URL" --type js --type ts --type jsx --type tsx -B 2 -A 2Repository: phoenixvc/cognitive-mesh
Length of output: 95
🏁 Script executed:
rg "NEXT_PUBLIC_API_BASE_URL" -A 3 -B 3Repository: phoenixvc/cognitive-mesh
Length of output: 8899
🏁 Script executed:
find src/UILayer/web -name "*.ts" -o -name "*.js" | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 974
🏁 Script executed:
rg "api" src/UILayer/web --type js --type ts -i | grep -i "fetch\|axios\|baseurl" | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 2299
🏁 Script executed:
cat docker-compose.ymlRepository: phoenixvc/cognitive-mesh
Length of output: 2796
🏁 Script executed:
cat k8s/base/frontend-ingress.yamlRepository: phoenixvc/cognitive-mesh
Length of output: 1764
Use a relative /api base URL for browser-reachable API calls.
NEXT_PUBLIC_API_BASE_URL is inlined by Next.js at build time; cluster-internal service names like http://cognitive-mesh-api and .svc.cluster.local won't resolve from a browser, and ConfigMap overrides at deployment time won't affect the already-compiled code. The ingress already routes /api/* to the backend, so a relative URL works correctly and avoids DNS resolution and TLS mixed-content issues.
🔧 Proposed fix
data:
- NEXT_PUBLIC_API_BASE_URL: "http://cognitive-mesh-api"
+ NEXT_PUBLIC_API_BASE_URL: "/api"
NODE_ENV: "production"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@k8s/base/frontend-configmap.yaml` around lines 13 - 15, SUMMARY:
NEXT_PUBLIC_API_BASE_URL is currently set to an internal cluster hostname that
will be inlined at Next.js build time and not reachable from browsers; change it
to a relative path. Update the ConfigMap entry NEXT_PUBLIC_API_BASE_URL in
frontend-configmap.yaml to "/api" (remove "http://cognitive-mesh-api"), then
rebuild/redeploy the frontend so Next.js picks up the relative API base;
reference the NEXT_PUBLIC_API_BASE_URL key to locate the change.
| <span className="text-xs font-medium text-gray-300">{ind.pattern}</span> | ||
| <span className={`inline-block rounded-full px-2 py-0.5 text-[10px] font-semibold ${severityBadgeClass(ind.severity)}`}>{ind.severity}</span> | ||
| </div> | ||
| <p className="mt-0.5 text-xs text-gray-500">{ind.affectedUsers} affected users - {new Date(ind.detectedAt).toLocaleDateString()}</p> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactTimeline.tsx" | head -50Repository: phoenixvc/cognitive-mesh
Length of output: 1838
🏁 Script executed:
rg "toLocaleDateString|toLocaleString|toISOString|formatDate|dateFormatter" --type tsx --type ts -i src/UILayer/web/src/componentsRepository: phoenixvc/cognitive-mesh
Length of output: 95
🏁 Script executed:
fd -e ts -e tsx . src/UILayer/web/src/components/widgets/ImpactMetrics | head -20Repository: phoenixvc/cognitive-mesh
Length of output: 419
🏁 Script executed:
rg "toLocaleDateString|toLocaleString|toISOString|formatDate" src/UILayer/web/src --max-count=10Repository: phoenixvc/cognitive-mesh
Length of output: 3113
🏁 Script executed:
cat -n "src/UILayer/web/src/components/widgets/ImpactMetrics/types.ts" 2>/dev/null || rg "ResistanceIndicator" src/UILayer/web/src --max-count=5 -A 10Repository: phoenixvc/cognitive-mesh
Length of output: 7289
Use a deterministic formatter for detectedAt.
Line 30 uses toLocaleDateString() without explicit locale and timezone parameters. In Next.js, this renders with the server's environment defaults during initial render, then re-renders with the browser's locale during client hydration, causing a hydration mismatch and potential console warnings.
Suggested fix
- <p className="mt-0.5 text-xs text-gray-500">{ind.affectedUsers} affected users - {new Date(ind.detectedAt).toLocaleDateString()}</p>
+ <p className="mt-0.5 text-xs text-gray-500">
+ {ind.affectedUsers} affected users - {new Date(ind.detectedAt).toLocaleDateString('en-US', { timeZone: 'UTC' })}
+ </p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p className="mt-0.5 text-xs text-gray-500">{ind.affectedUsers} affected users - {new Date(ind.detectedAt).toLocaleDateString()}</p> | |
| <p className="mt-0.5 text-xs text-gray-500"> | |
| {ind.affectedUsers} affected users - {new Date(ind.detectedAt).toLocaleDateString('en-US', { timeZone: 'UTC' })} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/ImpactTimeline.tsx` at
line 30, The date rendering in ImpactTimeline.tsx is non-deterministic because
new Date(ind.detectedAt).toLocaleDateString() relies on environment
locale/timezone and causes hydration mismatches; replace it with a deterministic
formatter such as Intl.DateTimeFormat with an explicit locale and timeZone
(e.g., use new Intl.DateTimeFormat('en-US', { year:'numeric', month:'short',
day:'numeric', timeZone:'UTC' }).format(new Date(ind.detectedAt'))) so both
server and client produce the same string; update the JSX where ind.detectedAt
is used in the <p> line in the ImpactTimeline component accordingly.
Comment on lines
+95
to
+101
| return ( | ||
| <svg | ||
| ref={svgRef} | ||
| className="h-40 w-full" | ||
| role="img" | ||
| aria-label={`Safety gauge: ${Math.round(score)} out of 100${label ? ` — ${label}` : ''}`} | ||
| /> |
Contributor
There was a problem hiding this comment.
Minor: aria-label should use clamped score for consistency.
The gauge visual uses clampedScore (line 40), but the aria-label on line 100 uses Math.round(score). If score is outside 0-100, the label would be inconsistent with the visual.
-aria-label={`Safety gauge: ${Math.round(score)} out of 100${label ? ` — ${label}` : ''}`}
+aria-label={`Safety gauge: ${Math.round(Math.max(0, Math.min(100, score)))} out of 100${label ? ` — ${label}` : ''}`}Or extract clamping to a shared variable at the component level.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <svg | |
| ref={svgRef} | |
| className="h-40 w-full" | |
| role="img" | |
| aria-label={`Safety gauge: ${Math.round(score)} out of 100${label ? ` — ${label}` : ''}`} | |
| /> | |
| return ( | |
| <svg | |
| ref={svgRef} | |
| className="h-40 w-full" | |
| role="img" | |
| aria-label={`Safety gauge: ${Math.round(Math.max(0, Math.min(100, score)))} out of 100${label ? ` — ${label}` : ''}`} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/components/widgets/ImpactMetrics/SafetyGauge.tsx` around
lines 95 - 101, The aria-label on the SVG in SafetyGauge currently uses
Math.round(score) which can differ from the visual because the chart uses
clampedScore; update the aria-label to use the same clamped value (clampedScore)
and rounded form so the accessible text matches the rendered gauge, or move the
clamping logic into a shared variable at the top of the component and reference
that variable in both the rendering and the aria-label (ensure references to
svgRef and existing label logic remain unchanged).
Comment on lines
+72
to
+84
| function renderHeader(label: string, field: SortField) { | ||
| const active = sortField === field; | ||
| const arrow = active ? (sortDir === 'asc' ? ' \u25B2' : ' \u25BC') : ''; | ||
| return ( | ||
| <th | ||
| scope="col" | ||
| className="cursor-pointer select-none px-3 py-2 text-left text-xs font-semibold uppercase text-gray-400 hover:text-white" | ||
| onClick={() => handleSort(field)} | ||
| > | ||
| {label} | ||
| {arrow} | ||
| </th> | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "GapAnalysisTable.tsx" -type fRepository: phoenixvc/cognitive-mesh
Length of output: 144
🏁 Script executed:
cat -n ./src/UILayer/web/src/components/widgets/NistCompliance/GapAnalysisTable.tsx | head -150Repository: phoenixvc/cognitive-mesh
Length of output: 4805
Make the sortable headers keyboard accessible and expose sort state to assistive technology.
The <th> elements with onClick handlers in renderHeader (lines 72-84) are not keyboard accessible and don't expose sort state. Interactive controls must be actual buttons, and sortable headers must declare their sort state via aria-sort. Wrap the label in a <button type="button">, set aria-sort on the <th>, and mark the decorative arrow with aria-hidden="true".
Suggested fix
function renderHeader(label: string, field: SortField) {
const active = sortField === field;
const arrow = active ? (sortDir === 'asc' ? ' \u25B2' : ' \u25BC') : '';
+ const ariaSort = active ? (sortDir === 'asc' ? 'ascending' : 'descending') : 'none';
return (
<th
scope="col"
- className="cursor-pointer select-none px-3 py-2 text-left text-xs font-semibold uppercase text-gray-400 hover:text-white"
- onClick={() => handleSort(field)}
+ aria-sort={ariaSort}
+ className="px-3 py-2 text-left text-xs font-semibold uppercase text-gray-400"
>
- {label}
- {arrow}
+ <button
+ type="button"
+ onClick={() => handleSort(field)}
+ className="flex items-center gap-1 hover:text-white"
+ >
+ <span>{label}</span>
+ <span aria-hidden="true">{arrow}</span>
+ </button>
</th>
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/components/widgets/NistCompliance/GapAnalysisTable.tsx`
around lines 72 - 84, The table headers rendered by renderHeader are not
keyboard-accessible and don't expose sort state; change the <th> to include an
aria-sort attribute derived from sortField and sortDir (e.g., aria-sort={active
? (sortDir === 'asc' ? 'ascending' : 'descending') : 'none'}), move the click
target to a <button type="button"> wrapping the label (and call
handleSort(field) from the button onClick), and mark the visual arrow string as
aria-hidden so it isn't read twice; ensure focus styles remain and that
renderHeader continues to use sortField, sortDir, and handleSort to determine
active state.
| @@ -0,0 +1,183 @@ | |||
| 'use client'; | |||
|
|
|||
| import React, { useState, useEffect, useCallback } from 'react'; | |||
Contributor
There was a problem hiding this comment.
Guard state updates against stale requests.
This callback always commits the last response that resolves, not the latest request that was started. If the props change or the user clicks Refresh twice, an older org/tenant response can overwrite the newer dashboard state.
Suggested fix
-import React, { useState, useEffect, useCallback } from 'react';
+import React, { useState, useEffect, useCallback, useRef } from 'react';
...
+ const requestIdRef = useRef(0);
+
const fetchData = useCallback(async () => {
+ const requestId = ++requestIdRef.current;
setLoading(true);
setError(null);
try {
const [diag, blind] = await Promise.all([
runValueDiagnostic(targetId, targetType, tenantId),
detectOrgBlindness(organizationId, tenantId),
]);
+ if (requestId !== requestIdRef.current) return;
setDiagnostic(diag);
setBlindness(blind);
} catch (err) {
+ if (requestId !== requestIdRef.current) return;
const msg = err instanceof Error ? err.message : 'Failed to load value generation data.';
setError(msg);
} finally {
+ if (requestId !== requestIdRef.current) return;
setLoading(false);
}
}, [targetId, targetType, organizationId, tenantId]);Also applies to: 34-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/UILayer/web/src/components/widgets/ValueGeneration/ValueGenerationDashboard.tsx`
at line 3, The component ValueGenerationDashboard is vulnerable to
stale-response overwrites; add a request-scoping guard (either an
AbortController signal or an incrementing requestId token) inside the async
fetch/refresh handlers (e.g., refreshDashboard / loadDashboard used in the
useEffect and the Refresh button callback) and only call state setters
(setDashboardState / setLoading / setError) if the current requestId matches the
latest or the AbortSignal is not aborted; ensure the token/controller is updated
before each new request and cleaned up on unmount to prevent older responses
from mutating state.
Comment on lines
+24
to
+34
| if (axes.length === 0) { | ||
| svg | ||
| .append('text') | ||
| .attr('x', width / 2) | ||
| .attr('y', height / 2) | ||
| .attr('text-anchor', 'middle') | ||
| .attr('fill', 'rgb(107,114,128)') | ||
| .attr('font-size', 12) | ||
| .text('No data for radar chart.'); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Reject mismatched axes / values input before drawing.
This chart computes its angular grid from axes.length but renders the polygon and dots from values. If those arrays diverge, points wrap or land on unlabeled axes, so the visualization is wrong.
Suggested fix
- if (axes.length === 0) {
+ if (axes.length === 0 || axes.length !== values.length) {
svg
.append('text')
.attr('x', width / 2)
.attr('y', height / 2)
.attr('text-anchor', 'middle')
.attr('fill', 'rgb(107,114,128)')
.attr('font-size', 12)
- .text('No data for radar chart.');
+ .text('Radar chart data is unavailable.');
return;
}Also applies to: 80-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/UILayer/web/src/components/widgets/ValueGeneration/ValueRadarChart.tsx`
around lines 24 - 34, The component renders the radar grid using axes.length but
draws the polygon/dots using values, so mismatched lengths cause misaligned
points; in the ValueRadarChart component validate that values.length ===
axes.length (and that both are non-empty) before drawing, and if they differ
display the same "No data for radar chart." fallback (or a clear error message)
and return early; update places that assume axes/values parity (the early-return
block that currently checks only axes.length and the polygon/dot-rendering
logic) to use this parity check and avoid drawing when lengths mismatch.
5 tasks
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.
Summary
/settings/notificationspage — channel toggles (email, push, SMS, in-app), 5 notification categories, quiet hours with timezone/profilepage — account info, role badges, GDPR/EU AI Act consent (4 types), data export request, session infoTest plan
/settingsrenders language selector and privacy toggles/settings/notificationsrenders channel/category/quiet hours controls/profilerenders user info, role badges, consent togglesnpx tsc --noEmitreports 0 errorsnpx next buildgenerates all 14 pages🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Navigation