fix(coherence): align Signals UI with proposal design patterns#2219
Open
webguru-hypha wants to merge 60 commits into
Open
fix(coherence): align Signals UI with proposal design patterns#2219webguru-hypha wants to merge 60 commits into
webguru-hypha wants to merge 60 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThreads optional Changes
Sequence Diagram(s)sequenceDiagram
participant UI as HumanRightPanel (client)
participant State as Local state / pending queue
participant Timer as Debounce Timer
participant API as updateCoherenceBySlug (server)
UI->>State: compute (coherenceSlug, messages.length)
State-->>Timer: schedule debounced flush (500ms)
Timer->>State: trigger flush
State->>API: send payload (single in-flight)
API-->>State: success -> update last-persisted (slug,count)
API-->>State: failure -> keep pending, log warning
alt failure and no in-flight
State->>Timer: schedule retry (1000ms)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔗 Custom preview URL: https://pr-2219.preview-app.hypha.earth |
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 250-377: The nested interactive controls in SignalCard (e.g., the
trash Button that opens the AlertDialog, the delete confirm Button,
AlertDialogCancel, and the "show details" button handled by setDetailsOpen) only
stop click propagation but not keyboard events, which lets Enter/Space bubble to
the card-level onSignalClick in signal-grid.tsx; add onKeyDown handlers to these
controls (the Trash Button, the AlertDialogCancel wrapper, the delete confirm
Button, and the description button) that detect Enter (key === 'Enter') and
Space (key === ' ' or key === 'Spacebar') and call e.stopPropagation() (and
e.preventDefault() for Space if appropriate) so keyboard activation does not
reach the parent card handler. Ensure each added handler is attached to the same
element that has onClick (e.g., the Button elements and the explicit <button>
that opens details) so both mouse and keyboard interactions are isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8139e6f-5d8c-414f-b3b2-0482763dfdf6
⛔ Files ignored due to path filters (3)
apps/web/public/placeholder/signal-priority-high.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-priority-low.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-priority-medium.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
packages/epics/src/coherence/components/coherence-block.tsxpackages/epics/src/coherence/components/create-signal-form.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.json
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/epics/src/coherence/components/signal-card.tsx (1)
252-315:⚠️ Potential issue | 🟠 MajorStop keyboard-triggered event bubbling from nested controls to card activation.
Line 252 and Line 370 only stop click propagation. Enter/Space key events can still bubble to parent card keyboard handlers and trigger unintended card activation.
🔧 Proposed fix
+ const stopCardActivationKey = React.useCallback( + (e: React.KeyboardEvent<HTMLElement>) => { + if (e.key === 'Enter' || e.key === ' ' || e.key === 'Spacebar') { + if (e.key === ' ' || e.key === 'Spacebar') e.preventDefault(); + e.stopPropagation(); + } + }, + [], + ); + <Button type="button" @@ onClick={(e) => { e.stopPropagation(); setDeleteOpen(true); }} + onKeyDown={stopCardActivationKey} > @@ <AlertDialogCancel asChild> - <Button variant="outline" colorVariant="neutral"> + <Button + variant="outline" + colorVariant="neutral" + onKeyDown={stopCardActivationKey} + > {t('noLeave')} </Button> </AlertDialogCancel> <Button type="button" colorVariant="accent" @@ if (deleted) setDeleteOpen(false); }} + onKeyDown={stopCardActivationKey} > {tSignalCard('deleteConfirmAction')} </Button> @@ <button type="button" @@ onClick={(e) => { e.stopPropagation(); setDetailsOpen(true); }} + onKeyDown={stopCardActivationKey} >Also applies to: 370-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/coherence/components/signal-card.tsx` around lines 252 - 315, The nested controls only stop click propagation so keyboard activation still bubbles; update the Trash Button (the Button with aria-label={tSignalCard('deleteMenu')}) and the AlertDialogContent (the element with onClick={(e) => e.stopPropagation()}) to also handle keyboard events by adding an onKeyDown handler that detects Enter and Space (e.key === 'Enter' or e.key === ' ' / 'Spacebar') and calls e.preventDefault() and e.stopPropagation(); apply the same onKeyDown logic to the other nested control referenced in the comment (lines ~370-379) so Enter/Space no longer bubble to parent card keyboard handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 252-315: The nested controls only stop click propagation so
keyboard activation still bubbles; update the Trash Button (the Button with
aria-label={tSignalCard('deleteMenu')}) and the AlertDialogContent (the element
with onClick={(e) => e.stopPropagation()}) to also handle keyboard events by
adding an onKeyDown handler that detects Enter and Space (e.key === 'Enter' or
e.key === ' ' / 'Spacebar') and calls e.preventDefault() and
e.stopPropagation(); apply the same onKeyDown logic to the other nested control
referenced in the comment (lines ~370-379) so Enter/Space no longer bubble to
parent card keyboard handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b610dcfa-86d0-4f76-854d-c3090e8a9c5c
📒 Files selected for processing (1)
packages/epics/src/coherence/components/signal-card.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/epics/src/coherence/components/signal-card.tsx (1)
522-529:⚠️ Potential issue | 🟠 MajorAdd keyboard propagation guards to the bottom action buttons.
Line 522 (
unarchive) and Line 532 (open conversation) are nested interactive controls inside a keyboard-activatable parent card. WithoutonKeyDown={stopCardActivationKey}, Enter/Space can still bubble and trigger parent activation.🔧 Proposed fix
<ConfirmDialog @@ <Button variant="outline" colorVariant="accent" className="w-full" + onKeyDown={stopCardActivationKey} > {t('unarchive')} </Button> @@ <Button variant="outline" colorVariant="accent" className="w-full" disabled={isLoading || !roomId} + onKeyDown={stopCardActivationKey} onClick={(e) => { if (onOpenConversation) { e.stopPropagation();Also applies to: 532-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/coherence/components/signal-card.tsx` around lines 522 - 529, The bottom action Buttons ("unarchive" and "open conversation") are interactive controls nested inside a keyboard-activatable parent and need to stop Enter/Space from bubbling; locate the Button rendering the t('unarchive') (inside ConfirmDialog) and the Button rendering the "open conversation" action and add the onKeyDown={stopCardActivationKey} prop to each so keyboard events are consumed by the button and do not trigger parent activation; use the existing stopCardActivationKey function symbol to implement the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/coherence-type-button.tsx`:
- Around line 52-53: The class list for the icon wrapper includes a redundant
hardcoded color utility 'text-foreground' alongside the variable textColor;
update the JSX in coherence-type-button (the wrapper that currently uses the
string 'flex h-8 w-8 shrink-0 items-center justify-center text-foreground' and
then textColor) by removing 'text-foreground' so only textColor controls the
text color, preventing conflicts and redundancy.
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Line 423: In signal-card.tsx replace the missing translation key by changing
the call to the i18n translator: update t('mentions', { count: messages }) to
use the defined key t('messageCount', { count: messages }) inside the component
(look for the t(...) usage in the SignalCard render) so pluralization uses the
locale files; ensure the variable passed (messages) remains the same.
In `@packages/epics/src/coherence/components/signal-grid.container.tsx`:
- Around line 40-42: The current mapping creates shallow clones of each signal
before passing to SignalGrid (signals={paginatedSignals.map(signal => ({
...signal }))}), which is redundant and causes render churn; change the prop to
pass the original array directly (signals={paginatedSignals}) and ensure no
downstream code mutates the signal objects so memoization in SignalGrid and any
useMemo/useCallback comparisons remain valid.
In `@packages/i18n/src/messages/en.json`:
- Around line 2023-2025: The translation key was renamed from "mentions" to
"messageCount" but the SignalCard component still calls t('mentions', { count:
messages }), causing missing translations; update the call in the SignalCard
(where t(...) is invoked, around the t('mentions', { count: messages })
expression inside the SignalCard component) to use t('messageCount', { count:
messages }) (or add a fallback lookup) so the pluralized messageCount key is
used at runtime.
---
Outside diff comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 522-529: The bottom action Buttons ("unarchive" and "open
conversation") are interactive controls nested inside a keyboard-activatable
parent and need to stop Enter/Space from bubbling; locate the Button rendering
the t('unarchive') (inside ConfirmDialog) and the Button rendering the "open
conversation" action and add the onKeyDown={stopCardActivationKey} prop to each
so keyboard events are consumed by the button and do not trigger parent
activation; use the existing stopCardActivationKey function symbol to implement
the guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46b5d765-9979-4845-8dcb-c3b3b0396e53
⛔ Files ignored due to path filters (6)
apps/web/public/placeholder/signal-type-insight.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-opportunity.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-proposal.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-risk.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-tension.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-trend.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
packages/epics/src/coherence/components/coherence-block.tsxpackages/epics/src/coherence/components/coherence-priority-button.tsxpackages/epics/src/coherence/components/coherence-type-button.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/epics/src/coherence/components/signal-grid.container.tsxpackages/epics/src/coherence/components/signal-grid.tsxpackages/epics/src/coherence/components/signal-section.tsxpackages/epics/src/common/card-button.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1836-1883: The effect can start a new updateCoherenceBySlug while
a previous mutation is still in flight, letting an older response overwrite a
newer count; fix this by serialising writes with a monotonic sequence or
in-flight tracker: add refs like inFlightUpdateRef and coherenceUpdateSeqRef,
increment coherenceUpdateSeqRef before scheduling/issuing updateCoherenceBySlug,
capture the current seq in the promise .then/.catch and only write
lastPersistedCoherenceMessageCountRef/current state when the resolved seq
matches the latest seq; alternatively cancel/ignore earlier in-flight results by
checking the seq in the promise handlers so only the latest
updateCoherenceBySlug result can update persisted state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4332272-a604-4cef-a714-53adf8989271
📒 Files selected for processing (4)
packages/epics/src/coherence/components/coherence-block.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/epics/src/coherence/components/signal-section.tsxpackages/epics/src/common/human-right-panel.tsx
Contributor
Author
|
@CodeRabbit review please |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/epics/src/coherence/components/signal-card.tsx (1)
187-205:⚠️ Potential issue | 🟠 MajorSIG-02 is still rendering a shared hero instead of a type-specific treatment.
The type badge metadata no longer carries the Lucide icon, and the hero image source ignores
typein favor ofleadImage || DEFAULT_SPACE_LEAD_IMAGE. SinceSignalGridforwards the sameleadImageto every card, all signals can end up with the same banner instead of the per-type SVG/icon treatment from the spec.Also applies to: 328-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/coherence/components/signal-card.tsx` around lines 187 - 205, The hero and badge metadata ignore signal type causing shared banners and loss of the Lucide icon: update metaBadges (the typeBadge created in the React.useMemo) to include the type-specific icon property (e.g., typeMeta.icon or lucide mapping) so the badge renders the per-type icon, and change the hero image source logic (where leadImage || DEFAULT_SPACE_LEAD_IMAGE is used) to prefer a type-specific SVG/asset for the given type first (use typeMeta.leadImage or a type-to-asset map) and only fall back to leadImage and then DEFAULT_SPACE_LEAD_IMAGE; ensure SignalGrid continues forwarding leadImage but cards resolve a type-specific banner when typeMeta exists.
♻️ Duplicate comments (1)
packages/epics/src/coherence/components/signal-card.tsx (1)
529-535:⚠️ Potential issue | 🟠 MajorFooter actions still leak Enter/Space to the parent card.
This is the same nested-control keyboard issue as before, but the footer buttons still miss
onKeyDown={stopCardActivationKey}. In the clickable/link card variants, keyboard activation on “Unarchive” or “Open conversation” can still trigger the parent card action.🔧 Suggested fix
<Button variant="outline" colorVariant="accent" className="w-full" + onKeyDown={stopCardActivationKey} > {t('unarchive')} </Button> ... <Button variant="outline" colorVariant="accent" className="w-full" disabled={isLoading || !roomId} + onKeyDown={stopCardActivationKey} onClick={(e) => { if (onOpenConversation) { e.stopPropagation();Also applies to: 539-555
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/coherence/components/signal-card.tsx` around lines 529 - 535, Footer action Buttons (e.g., the "Unarchive" and "Open conversation" Button components) are missing the keyboard handler that prevents Enter/Space from bubbling to the parent card; add onKeyDown={stopCardActivationKey} to each footer Button and any other interactive footer controls in signal-card.tsx (referencing the Button instances around the "unarchive" and the block covering lines ~539-555) so keyboard activation is stopped from triggering the parent card's click handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 328-333: The Image component is using alt={title || ''}, which
either duplicates the card heading or hides the banner from screen readers;
update the Image JSX (the <Image ... /> that uses leadImage,
DEFAULT_SPACE_LEAD_IMAGE and title) to use a proper localized banner alt string
when the image conveys signal semantics (e.g., a localized prop or i18n key like
bannerAltText) and fall back to a decorative empty alt="" when it is purely
visual; ensure you reference the leadImage/DEFAULT_SPACE_LEAD_IMAGE logic so the
alt selection matches whether the image is semantic or decorative.
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1867-1889: The flush routine can drop queued updates if
updateCoherenceBySlug throws; modify flushMessageCountSync/its caller so
failures re-queue the currently processed payload and schedule a retry instead
of silently stopping: inside flushMessageCountSync keep the while-loop but catch
per-iteration errors (or catch the outer promise), on error set
pendingCoherenceMessageCountRef.current back to the failed "next" value (and
preserve lastPersistedCoherenceMessageCountRef.current unchanged), ensure
coherenceMessageCountSyncInFlightRef.current is cleared, and schedule another
attempt (e.g., setTimeout(() => void flushMessageCountSync(), retryDelay)) so
pending updates are retried; reference functions/refs: flushMessageCountSync,
updateCoherenceBySlug, pendingCoherenceMessageCountRef,
coherenceMessageCountSyncInFlightRef, lastPersistedCoherenceMessageCountRef.
In `@packages/epics/src/common/use-scroll-parallax.ts`:
- Around line 18-28: Initialize the reduceMotion state from matchMedia so users
with prefers-reduced-motion don't see a flash: change the React.useState call
that creates reduceMotion to use a lazy initializer that returns
window.matchMedia?.('(prefers-reduced-motion: reduce)')?.matches ?? false
(guarding for SSR with typeof window !== 'undefined' if needed), keep the
existing React.useEffect sync logic for changes, and remove relying on the
effect to set the initial value; this ensures the initial value of reduceMotion
is correct on first render in use-scroll-parallax.ts.
In `@packages/epics/src/governance/components/document-card.tsx`:
- Around line 121-124: DocumentCard currently calls useScrollParallax(), causing
each card to subscribe to scroll updates and re-render on each frame; instead
lift that hook to the parent list/container component so only one subscription
exists and pass the resulting parallaxY and reduceMotion into each DocumentCard
as props. Update the parent component to call useScrollParallax({rate: 0.12,
maxShiftPx: 18}), expose parallaxY and reduceMotion, and change DocumentCard to
accept these props (remove its internal useScrollParallax call) and use the
passed parallaxY / reduceMotion values.
In `@packages/i18n/src/messages/en.json`:
- Line 44: The copy is inconsistent: Common.Coherence is now "Signals" but other
keys still reference "Coherence" in their user-facing values; update the
remaining Coherence-related message values (e.g., CoherenceTab.backToCoherence,
CoherenceTab.errorCoherenceTab) in packages/i18n/src/messages/en.json to use
"Signals" (or the agreed tab label) so all displayed strings are consistent with
Common.Coherence, keeping the existing key names intact.
In `@packages/ui/src/upload/lead-image.tsx`:
- Around line 133-193: Multiple UploadLeadImage instances each add their own
scroll listener/RAF and call setParallaxY, causing N updates per frame; extract
the scroll source into a shared subscription/hook (e.g. create useSharedScrollY
or useParallaxSubscription) that installs a single window/ancestor scroll
listener + one RAF loop and publishes a single scrollY/parallax value to
subscribers, then have UploadLeadImage read that shared value (or receive it via
context/prop) instead of calling
findScrollableAncestor/readScrollTop/onScroll/setParallaxY itself; update/remove
rootRef-based per-instance listeners (readScrollTop, onScroll,
findScrollableAncestor, rafId) and use the shared hook's value to compute
transforms to eliminate per-card listeners and reduce re-renders.
- Around line 281-285: The AspectRatio element currently spreads
{...getRootProps()} and also sets ref={rootRef}, which overrides the
react-dropzone ref and breaks DnD wiring; fix by composing the refs instead:
update the useDropzone call to supply a refKey (e.g., refKey: 'innerRef') if
AspectRatio supports a custom ref prop, or replace ref={rootRef} with a callback
ref that assigns both rootRef.current and forwards the element to the ref
returned in getRootProps(), or derive the parallax element from the element
returned by getRootProps() rather than keeping a separate rootRef; ensure
AspectRatio, getRootProps, rootRef and useDropzone wiring preserve
click/focus/drag behavior and ARIA/accessibility attributes.
In `@packages/ui/src/upload/preview-img.tsx`:
- Around line 4-16: The PreviewImg component currently replaces the default
sizing/object-fit classes when a caller provides className; update PreviewImg so
it always applies the base classes ('pointer-events-none h-full w-full
object-cover') and then appends the caller's className (e.g., join or use a
small utility like clsx) so consumers can add utilities without losing layout
behavior; modify the JSX in PreviewImg to merge the base class string with the
incoming className instead of using the nullish fallback.
---
Outside diff comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 187-205: The hero and badge metadata ignore signal type causing
shared banners and loss of the Lucide icon: update metaBadges (the typeBadge
created in the React.useMemo) to include the type-specific icon property (e.g.,
typeMeta.icon or lucide mapping) so the badge renders the per-type icon, and
change the hero image source logic (where leadImage || DEFAULT_SPACE_LEAD_IMAGE
is used) to prefer a type-specific SVG/asset for the given type first (use
typeMeta.leadImage or a type-to-asset map) and only fall back to leadImage and
then DEFAULT_SPACE_LEAD_IMAGE; ensure SignalGrid continues forwarding leadImage
but cards resolve a type-specific banner when typeMeta exists.
---
Duplicate comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 529-535: Footer action Buttons (e.g., the "Unarchive" and "Open
conversation" Button components) are missing the keyboard handler that prevents
Enter/Space from bubbling to the parent card; add
onKeyDown={stopCardActivationKey} to each footer Button and any other
interactive footer controls in signal-card.tsx (referencing the Button instances
around the "unarchive" and the block covering lines ~539-555) so keyboard
activation is stopped from triggering the parent card's click handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fee4a9c3-0f2a-4a3d-8b9b-3d496fe8b928
📒 Files selected for processing (14)
packages/epics/src/coherence/components/coherence-type-button.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/epics/src/coherence/components/signal-grid.container.tsxpackages/epics/src/common/human-right-panel.tsxpackages/epics/src/common/use-scroll-parallax.tspackages/epics/src/governance/components/document-card.tsxpackages/epics/src/proposals/components/proposal-detail.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.jsonpackages/ui/src/upload/lead-image.tsxpackages/ui/src/upload/preview-img.tsx
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/epics/src/common/human-right-panel.tsx (1)
1891-1905:⚠️ Potential issue | 🟠 MajorRe-arm the retry loop when the scheduled retry also fails.
If
flushMessageCountSync()rejects inside the 1-second retry path,pendingCoherenceMessageCountRef.currentstays populated but nothing schedules another attempt. After that, the persisted count is stale untilmessages.lengthchanges again.🛠️ Suggested fix
+ const scheduleMessageCountRetry = () => { + coherenceMessageCountSyncTimeoutRef.current = setTimeout(() => { + if (coherenceMessageCountSyncInFlightRef.current) return; + void flushMessageCountSync().catch((retryError) => { + console.warn( + '[HumanRightPanel] Retry failed to persist coherence message count:', + retryError, + ); + if (pendingCoherenceMessageCountRef.current) { + scheduleMessageCountRetry(); + } + }); + }, 1000); + }; + void flushMessageCountSync().catch((error) => { console.warn( '[HumanRightPanel] Failed to persist coherence message count:', error, ); if (pendingCoherenceMessageCountRef.current) { - coherenceMessageCountSyncTimeoutRef.current = setTimeout(() => { - if (coherenceMessageCountSyncInFlightRef.current) return; - void flushMessageCountSync().catch((retryError) => { - console.warn( - '[HumanRightPanel] Retry failed to persist coherence message count:', - retryError, - ); - }); - }, 1000); + scheduleMessageCountRetry(); } });As per coding guidelines, “Error and loading states are handled in all user flows.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/common/human-right-panel.tsx` around lines 1891 - 1905, The retry path for flushMessageCountSync() does not re-arm itself if the scheduled retry fails, leaving pendingCoherenceMessageCountRef.current set but no further attempts; update the catch in the inner retry (inside the setTimeout) to, after logging the retryError, check pendingCoherenceMessageCountRef.current and if still set schedule another timeout via coherenceMessageCountSyncTimeoutRef.current = setTimeout(...) that re-runs the same guarded flushMessageCountSync() logic (respect coherenceMessageCountSyncInFlightRef.current to avoid concurrent runs) so failures keep being retried until the pending count is cleared; ensure the timeout ref is updated each time.packages/epics/src/coherence/components/signal-card.tsx (1)
514-555:⚠️ Potential issue | 🟠 MajorWire the footer CTA like the other nested controls.
onOpenConversationis optional, but this button is only disabled whenroomIdis missing. With aroomIdand no callback, it still renders enabled, and propagation is only stopped inside the callback guard, so the interaction can fall through to the parent card. This footer block also skipsstopCardActivationKey, unlike the other nested controls in this file.Suggested fix
<Button variant="outline" colorVariant="accent" className="w-full" + onKeyDown={stopCardActivationKey} > {t('unarchive')} </Button> ... <Button variant="outline" colorVariant="accent" className="w-full" - disabled={isLoading || !roomId} + disabled={isLoading || !roomId || !onOpenConversation} onClick={(e) => { - if (onOpenConversation) { - e.stopPropagation(); - e.preventDefault(); - onOpenConversation(); - } + e.stopPropagation(); + e.preventDefault(); + onOpenConversation?.(); }} + onKeyDown={stopCardActivationKey} title={!roomId ? tSignalCard('noConversationRoom') : undefined} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/coherence/components/signal-card.tsx` around lines 514 - 555, The footer CTA must match other nested controls: always stop propagation/preventDefault and use stopCardActivationKey, and only be enabled when both roomId and onOpenConversation exist; update the Button inside signal-card.tsx so its onClick handler first e.stopPropagation()/e.preventDefault(), call onOpenConversation() only if present, add onKeyDown={stopCardActivationKey} to the Button (or its wrapper) and change the disabled condition to disabled={isLoading || !roomId || !onOpenConversation}; reference the existing onOpenConversation prop, stopCardActivationKey utility, and the archived/handleUnarchive ConfirmDialog block for the same pattern.packages/i18n/src/messages/es.json (1)
44-44:⚠️ Potential issue | 🟡 MinorFinish the Spanish Signals rename.
Common.Coherencenow says"Señales", but this locale file still has user-facing copy likeCoherenceTab.backToCoherenceandCoherenceTab.errorCoherenceTabreferring to"Coherencia". That will surface both names in the same flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/messages/es.json` at line 44, Update the Spanish translations so the user-facing name is consistent with Common.Coherence ("Señales"): find the CoherenceTab entries (e.g., CoherenceTab.backToCoherence and CoherenceTab.errorCoherenceTab) and replace any occurrences of "Coherencia" in their values with "Señales"; scan the same locale file for other strings referencing "Coherencia" and update them to "Señales" so the UI shows a single name throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/`[lang]/dho/[id]/_components/navigation-tabs.tsx:
- Around line 76-78: Update the tab config for the signals tab so the legacy
/coherence route still matches and the visible label shows the new copy: change
the tab object's name from 'signals' to 'coherence' (so route triggers/active
state include the legacy path) and change title from t('Coherence') to
t('Signals'); keep the existing href using getDhoPathSignals(lang, id) so
routing stays the same.
In `@packages/epics/src/governance/components/document-grid.tsx`:
- Around line 29-32: DocumentGrid uses the React hook useScrollParallax, so mark
the file as a client component by adding 'use client'; as the very first line of
document-grid.tsx (before any imports) so DocumentGrid and any usages (via
document-grid.container.tsx -> document-section) run on the client and hooks are
allowed.
---
Duplicate comments:
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 514-555: The footer CTA must match other nested controls: always
stop propagation/preventDefault and use stopCardActivationKey, and only be
enabled when both roomId and onOpenConversation exist; update the Button inside
signal-card.tsx so its onClick handler first
e.stopPropagation()/e.preventDefault(), call onOpenConversation() only if
present, add onKeyDown={stopCardActivationKey} to the Button (or its wrapper)
and change the disabled condition to disabled={isLoading || !roomId ||
!onOpenConversation}; reference the existing onOpenConversation prop,
stopCardActivationKey utility, and the archived/handleUnarchive ConfirmDialog
block for the same pattern.
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1891-1905: The retry path for flushMessageCountSync() does not
re-arm itself if the scheduled retry fails, leaving
pendingCoherenceMessageCountRef.current set but no further attempts; update the
catch in the inner retry (inside the setTimeout) to, after logging the
retryError, check pendingCoherenceMessageCountRef.current and if still set
schedule another timeout via coherenceMessageCountSyncTimeoutRef.current =
setTimeout(...) that re-runs the same guarded flushMessageCountSync() logic
(respect coherenceMessageCountSyncInFlightRef.current to avoid concurrent runs)
so failures keep being retried until the pending count is cleared; ensure the
timeout ref is updated each time.
In `@packages/i18n/src/messages/es.json`:
- Line 44: Update the Spanish translations so the user-facing name is consistent
with Common.Coherence ("Señales"): find the CoherenceTab entries (e.g.,
CoherenceTab.backToCoherence and CoherenceTab.errorCoherenceTab) and replace any
occurrences of "Coherencia" in their values with "Señales"; scan the same locale
file for other strings referencing "Coherencia" and update them to "Señales" so
the UI shows a single name throughout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71134f99-c9ad-45bf-8a6a-037bec72529c
📒 Files selected for processing (24)
apps/web/src/app/[lang]/dho/[id]/@aside/[tab]/chat/[chatId]/page.tsxapps/web/src/app/[lang]/dho/[id]/@aside/[tab]/new-signal/page.tsxapps/web/src/app/[lang]/dho/[id]/@tab/signals/constants.tsapps/web/src/app/[lang]/dho/[id]/@tab/signals/default.tsxapps/web/src/app/[lang]/dho/[id]/@tab/signals/error.tsxapps/web/src/app/[lang]/dho/[id]/@tab/signals/page.tsxapps/web/src/app/[lang]/dho/[id]/_components/navigation-tabs.tsxpackages/epics/src/coherence/components/coherence-block.tsxpackages/epics/src/coherence/components/coherence-type-button.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/epics/src/coherence/components/signal-grid.container.tsxpackages/epics/src/coherence/components/signal-section.tsxpackages/epics/src/common/human-right-panel.tsxpackages/epics/src/common/use-scroll-parallax.tspackages/epics/src/governance/components/document-card.tsxpackages/epics/src/governance/components/document-grid.tsxpackages/epics/src/proposals/components/proposal-detail.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.jsonpackages/ui/src/upload/lead-image.tsxpackages/ui/src/upload/preview-img.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/create-signal-form.tsx`:
- Around line 369-371: The list rendering uses array index as key in the
typeOptions.map for CoherenceTypeButton which can cause reconciliation bugs;
replace the index key (`key={`type-option-${index}`}`) with a stable domain key
derived from the option object (e.g., `option.type` or a composite like
`${option.type}-${option.priority}`) in both occurrences (the map at
typeOptions.map and the similar map around lines 408-410) so each
CoherenceTypeButton has a stable, unique key.
- Around line 218-233: The Matrix provisioning path currently only logs
warnings; modify the async block invoked when isMatrixAvailable && coherenceSlug
to surface a non-blocking user-visible failure state: after catching matrixError
in the createRoom/updateCoherenceBySlug sequence (functions createRoom and
updateCoherenceBySlug, local var coherenceSlug), dispatch a toast/banner event
or call the existing UI notification helper (e.g., showToast/showBanner) with a
concise message like "Signal created but chat room provisioning failed" and
include matrixError.message for debugging; ensure this notification is
non-blocking so navigation still proceeds. Also add an explicit else branch for
the case where coherenceSlug is missing (when isMatrixAvailable is true but
coherenceSlug falsy) to emit a distinct non-blocking notification indicating
room couldn’t be linked and suggest retrying, so the degraded state is
observable and actionable.
In `@packages/i18n/src/messages/fr.json`:
- Around line 44-45: The French translations are inconsistent: you changed the
value for "Common.Coherence" to "Signaux" but other keys still use "Cohérence";
update the FR JSON values for the keys referenced (e.g.,
CoherenceTab.backToCoherence and CoherenceTab.errorCoherenceTab and any other
Coherence* keys) to use "Signaux" (or the chosen term) so all user-facing labels
match "Common.Coherence" consistently across the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51430bab-dfed-4438-bac1-a59f3ec2842c
📒 Files selected for processing (8)
apps/web/src/app/[lang]/dho/[id]/_components/navigation-tabs.tsxpackages/epics/src/coherence/components/create-signal-form.tsxpackages/epics/src/governance/components/document-grid.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.json
Contributor
Author
|
@CodeRabbit, review |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
n0umen0n
approved these changes
Apr 30, 2026
Contributor
Author
|
@code rabbit review |
Contributor
Author
|
@code rabbit review |
Contributor
Author
|
@code rabbit please review |
Remove the extra panel frame on the coherence tab, add priority illustrations and message counts on signal cards, restyle the create signal form and description dialog to match contribution proposal sections, and use a solid accent CTA for open conversation. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Use outline + accent like VoteProposalButton (Vote Now) instead of filled default variant. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Replace left-column priority icons with full-width SVG banners per signal type plus a translucent overlay for priority. Remove unused priority-only placeholder assets. Add i18n for banner alt text. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Use rounded-xl on the card shell and rounded-t-xl on the banner container, consistent with `Card` + `DocumentCard` lead image corners. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
CardButton now uses border-2 and p-5 like VotingMethodSelector; subtle selected state drops extra ring. Remove bordered icon boxes from coherence type/priority buttons. Signal card badges use soft fills for type/tags with safe color mapping for tension/insight. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Use the space lead image with priority/type overlays and reorganize top metadata into clearer tag-based rows so signal cards match the app's existing proposal card language. Made-with: Cursor
added 2 commits
May 1, 2026 00:47
…n flow" This reverts commit 06beecb.
…ation Show a non-blocking notice when Matrix room provisioning cannot complete, use stable option keys in the create signal form, and align French Signals tab copy.
webguru-hypha
force-pushed
the
cursor/fix-coherence-signals-design-044d
branch
from
333a131 to
d6ea10d
Compare
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)
packages/epics/src/coherence/components/signal-grid.tsx (1)
35-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let the card wrapper bypass the disabled conversation action.
When
onSignalClickis present, this branch makes every non-archived card interactive, so signals without aroomIdstill callonSignalClick(signal)from the wrapper even though the primary "Open conversation" CTA is supposed to be disabled in that state. Gate the clickable wrapper andonOpenConversationonsignal.roomId, or fall back to a non-clickable card for roomless signals.Suggested fix
- ) : onSignalClick ? ( + ) : onSignalClick && signal.roomId ? ( <div key={signal.id} role="button" tabIndex={0} @@ <SignalCard {...signal} leadImage={leadImage} className="h-full w-full min-h-0" isLoading={isLoading} refresh={refresh} onOpenConversation={() => onSignalClick(signal)} /> </div> + ) : onSignalClick ? ( + <SignalCard + key={signal.id} + {...signal} + leadImage={leadImage} + className="h-full w-full min-h-0" + isLoading={isLoading} + refresh={refresh} + /> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/epics/src/coherence/components/signal-grid.tsx` around lines 35 - 59, The wrapper currently calls onSignalClick for every non-archived signal, bypassing the SignalCard's disabled "Open conversation" CTA; update the conditional so the clickable wrapper div and the SignalCard prop onOpenConversation are only provided when signal.roomId exists (e.g., check signal.roomId before rendering the role="button" div and before passing onOpenConversation={() => onSignalClick(signal)}), otherwise render a non-clickable container (or no onOpenConversation) so roomless signals cannot trigger onSignalClick from the wrapper or card.
♻️ Duplicate comments (1)
packages/i18n/src/messages/pt.json (1)
44-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish the Signals rename in the Portuguese locale.
Common.Coherencenow renders asSinais, butCoherenceTab.backToCoherenceon Line 1982 andCoherenceTab.errorCoherenceTabon Line 2018 still exposeCoerência. That leaves the PT flow mixing both labels in the same feature.As per coding guidelines, “Message files are consistent across locales (no missing keys)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/messages/pt.json` around lines 44 - 45, The Portuguese locale mixes "Sinais" and "Coerência": update the remaining keys to match the renamed label by locating CoherenceTab.backToCoherence and CoherenceTab.errorCoherenceTab in packages/i18n/src/messages/pt.json and replace their current "Coerência" values with "Sinais" (so they match the existing "Coherence" and "Signals" entries), ensuring the PT messages are consistent with other locales and no keys are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/create-signal-form.tsx`:
- Around line 233-235: When Matrix is unavailable the branch only logs a warning
but doesn't set UI state; update the conditional in create-signal-form (the
block checking isMatrixAvailable and coherenceSlug) to call
setSignalProvisioningNotice with a proper notice (matching style used in other
failure paths) when !isMatrixAvailable so the user sees that chat provisioning
was skipped; reference the isMatrixAvailable check and the
setSignalProvisioningNotice function to locate where to add the call.
In `@packages/epics/src/coherence/components/signal-card.tsx`:
- Around line 206-213: The memo computed as metaBadges unnecessarily lists
coherenceType in its dependency array even though the memo body uses typeLabel
and typeColorVariant (which are derived from coherenceType elsewhere); remove
coherenceType from the dependency array and keep the existing dependencies
(priorityMeta, t, typeLabel, typeColorVariant, priorityColorVariant) so
metaBadges only recomputes when the actual used values change, ensuring
consistency with the memo's internal references.
In `@packages/epics/src/coherence/components/signal-section.tsx`:
- Around line 156-158: The provisioning notice currently rendered as <ErrorAlert
lines={provisioningNoticeLines} bgColor="bg-yellow-600" /> in signal-section.tsx
never disappears; introduce a dismiss mechanism by adding local state (e.g.,
const [showProvisioningNotice, setShowProvisioningNotice] =
useState(Boolean(provisioningNoticeLines.length))) and useEffect to auto-clear
it after a short timeout (or expose a manual "Dismiss" button) so the alert
hides when dismissed or timed out; update the conditional to render ErrorAlert
only when showProvisioningNotice is true, and either pass an onClose prop to
ErrorAlert (or render a button next to it) that calls
setShowProvisioningNotice(false).
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1897-1905: The retry logic in the timeout callback for
coherenceMessageCountSyncTimeoutRef stops after a single failed retry leaving
pendingCoherenceMessageCountRef.current set and counters stale; update the
handler that calls flushMessageCountSync() (the timeout callback using
coherenceMessageCountSyncInFlightRef and coherenceMessageCountSyncTimeoutRef) to
re-arm a retry when the promise rejects (e.g., schedule another setTimeout or
exponential backoff) and ensure coherenceMessageCountSyncTimeoutRef.current is
updated and coherenceMessageCountSyncInFlightRef/current state is cleared
appropriately so pendingCoherenceMessageCountRef.current will be retried until
success; reference the existing symbols coherenceMessageCountSyncTimeoutRef,
coherenceMessageCountSyncInFlightRef, flushMessageCountSync, and
pendingCoherenceMessageCountRef when making the changes.
In `@packages/epics/src/governance/components/document-card.tsx`:
- Around line 169-174: The parallax overscan is too small: the element using
className "absolute inset-x-0 top-[-10%] h-[120%] will-change-transform" can
shift by up to maxShiftPx (18px) via the parallaxY transform, exposing a gap;
update the overscan to account for the configured maxShiftPx by increasing the
top inset and/or height (e.g., compute top and height using maxShiftPx or use a
larger percentage) so the transformed element always covers the container when
reduceMotion is false; adjust the code that sets the className or style
alongside the reduceMotion/parallaxY logic so overscan is tied to the same
maxShiftPx value used in the grid.
In `@packages/epics/src/proposals/components/proposal-detail.tsx`:
- Around line 1143-1148: The parallax transform can exceed the container
overscan and reveal a top gap; clamp parallaxY or increase the overscan so the
transform never moves beyond the extra area. In practice, inside the element
using className "absolute inset-x-0 top-[-10%] h-[120%] will-change-transform",
either increase the overscan (e.g., bump h from 120% or top from -10% to match
the max shift) or wrap the style transform computation to limit parallaxY (e.g.,
compute clampedParallaxY = Math.max(Math.min(parallaxY, maxOverscanPx),
-maxOverscanPx) before using it) and keep the reduceMotion branch unchanged.
In `@packages/i18n/src/messages/de.json`:
- Line 1994: Update the static alt text for the key priorityVisualAlt so it
includes the dynamic priority value; change the string to include a {priority}
placeholder (e.g. "Prioritätsstufe: {priority}" or "{priority} Prioritätsstufe")
so the accessible label announces the current priority per card state and
consumers can interpolate the {priority} token at render time.
---
Outside diff comments:
In `@packages/epics/src/coherence/components/signal-grid.tsx`:
- Around line 35-59: The wrapper currently calls onSignalClick for every
non-archived signal, bypassing the SignalCard's disabled "Open conversation"
CTA; update the conditional so the clickable wrapper div and the SignalCard prop
onOpenConversation are only provided when signal.roomId exists (e.g., check
signal.roomId before rendering the role="button" div and before passing
onOpenConversation={() => onSignalClick(signal)}), otherwise render a
non-clickable container (or no onOpenConversation) so roomless signals cannot
trigger onSignalClick from the wrapper or card.
---
Duplicate comments:
In `@packages/i18n/src/messages/pt.json`:
- Around line 44-45: The Portuguese locale mixes "Sinais" and "Coerência":
update the remaining keys to match the renamed label by locating
CoherenceTab.backToCoherence and CoherenceTab.errorCoherenceTab in
packages/i18n/src/messages/pt.json and replace their current "Coerência" values
with "Sinais" (so they match the existing "Coherence" and "Signals" entries),
ensuring the PT messages are consistent with other locales and no keys are
missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd32a6f0-00df-43e7-9116-9aef634eb765
⛔ Files ignored due to path filters (6)
apps/web/public/placeholder/signal-type-insight.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-opportunity.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-proposal.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-risk.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-tension.svgis excluded by!**/*.svgapps/web/public/placeholder/signal-type-trend.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
apps/web/src/app/[lang]/dho/[id]/_components/navigation-tabs.tsxpackages/epics/src/coherence/components/coherence-block.tsxpackages/epics/src/coherence/components/coherence-priority-button.tsxpackages/epics/src/coherence/components/coherence-type-button.tsxpackages/epics/src/coherence/components/create-signal-form.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/epics/src/coherence/components/signal-grid.container.tsxpackages/epics/src/coherence/components/signal-grid.tsxpackages/epics/src/coherence/components/signal-section.tsxpackages/epics/src/common/card-button.tsxpackages/epics/src/common/human-right-panel.tsxpackages/epics/src/common/use-scroll-parallax.tspackages/epics/src/governance/components/document-card.tsxpackages/epics/src/governance/components/document-grid.tsxpackages/epics/src/proposals/components/proposal-detail.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.jsonpackages/ui/src/upload/lead-image.tsxpackages/ui/src/upload/preview-img.tsx
added 10 commits
May 1, 2026 01:20
Increase the signal card hero image overscan so parallax movement keeps the image fully covering the frame without exposing empty edges.
Surface a non-blocking notice when signal creation skips chat provisioning because Matrix is unavailable so users can see the degraded state.
Drop coherenceType from the metaBadges dependency array so the memo only tracks values referenced inside the callback.
Automatically clear the non-blocking provisioning notice after a short timeout so informational alerts do not linger indefinitely.
Re-arm retry scheduling after failed persistence attempts so pending coherence message counts continue retrying until they succeed.
Increase lead-image overscan so document card parallax shifts cannot expose top-edge gaps at maximum scroll offset.
Expand proposal detail image overscan so parallax translation cannot reveal top-edge gaps at peak movement.
Add the {priority} placeholder to the German priorityVisualAlt string so screen readers can announce the current priority level.
Clamp proposal detail hero parallax translation to stay within available overscan and avoid exposing top-edge gaps during scroll.
Update proposal parallax memo usage to use the existing React hook import and format signal card/proposal detail files to satisfy format:check. Made-with: Cursor
Contributor
Author
|
@code rabbit check |
Contributor
Author
|
@CodeRabbit please review |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[review] 🧠 Learnings used |
Use the same login prompt layout and CTA buttons as proposal access-denied states so the Signals unauthenticated experience stays visually consistent. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/create-signal-form.tsx`:
- Around line 233-262: Replace the three hardcoded English provisioning messages
passed to setSignalProvisioningNotice with i18n translation keys instead of
literal strings; locate the block using isMatrixAvailable, coherenceSlug,
createRoom and updateCoherenceBySlug and change the messages at the three sites
to use the app's translation helper (e.g.,
t('signals.provisioning.unavailable'), t('signals.provisioning.room_failed', {
error: matrixErrorMessage }), t('signals.provisioning.unlinked')) so the UI
shows translated text and pass any dynamic error text as a variable to the
translation call.
In `@packages/epics/src/coherence/components/signal-section.tsx`:
- Around line 70-88: The current SignalSection component only reads
sessionStorage once on mount (React.useEffect in signal-section.tsx) so a
provisioning notice written after a redirect can be missed; update SignalSection
to also listen for a custom event (e.g. "coherence:signalProvisioningNotice") on
window, and in the event handler perform the same read/parse/remove logic and
call setProvisioningNoticeLines, and remember to add/remove the event listener
in the effect cleanup and guard against server-side rendering (typeof window).
Also add the corresponding dispatch call (window.dispatchEvent(new
Event('coherence:signalProvisioningNotice'))) right after sessionStorage.setItem
in create-signal-form.tsx where provisioning failures are written so the notice
is picked up immediately.
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1845-1923: The effect can schedule retries after unmount because
flushMessageCountSync().catch(...) calls scheduleRetry() which re-arms timers;
add an effect-scoped cancellation flag (e.g., let mounted = true) and check it
inside scheduleRetry, before calling flushMessageCountSync, and at start of
flushMessageCountSync to skip work when mounted is false; set mounted = false in
the effect cleanup (alongside clearing coherenceMessageCountSyncTimeoutRef) and
also avoid re-creating coherenceMessageCountSyncTimeoutRef timers or updating
pendingCoherenceMessageCountRef/lastPersistedCoherenceMessageCountRef when
mounted is false so no orphan background writes occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f68d008-3d05-42ee-b175-38a75760341b
📒 Files selected for processing (8)
packages/epics/src/coherence/components/coherence-block.tsxpackages/epics/src/coherence/components/create-signal-form.tsxpackages/epics/src/coherence/components/signal-card.tsxpackages/epics/src/coherence/components/signal-section.tsxpackages/epics/src/common/human-right-panel.tsxpackages/epics/src/governance/components/document-card.tsxpackages/epics/src/proposals/components/proposal-detail.tsxpackages/i18n/src/messages/de.json
Localize signal provisioning notices, listen for late provisioning events after redirect, and stop coherence message-count retries after cleanup to avoid orphan timers. Made-with: Cursor
Contributor
Author
|
@CodeRabbit please review |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[full_review] 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/i18n/src/messages/es.json (1)
1978-2018:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish the Spanish Signals rename inside
CoherenceTab.This block now uses “Señales”, but
backToCoherenceanderrorCoherenceTabstill mention “Coherencia”, so the back label and load error remain inconsistent with the renamed tab.🌐 Suggested copy update
- "backToCoherence": "Volver a Coherencia", + "backToCoherence": "Volver a Señales", @@ - "errorCoherenceTab": "Vaya, algo salió mal. No se pudo cargar la pestaña de coherencia.", + "errorCoherenceTab": "Vaya, algo salió mal. No se pudo cargar la pestaña de señales.",As per coding guidelines, the Spanish locale should “update related back/error copy in the Signals tab.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/messages/es.json` around lines 1978 - 2018, Update the Spanish copy for the Signals tab to be consistent: change the "backToCoherence" and "errorCoherenceTab" entries in es.json to reference "Señales" instead of "Coherencia" so the back label and load-error message match the renamed tab (these keys are "backToCoherence" and "errorCoherenceTab" used by CoherenceTab).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/epics/src/coherence/components/create-signal-form.tsx`:
- Around line 217-226: The provisioning notice currently stores raw backend
error text (matrixErrorMessage) in sessionStorage via
setSignalProvisioningNotice, exposing untranslated/technical text to users;
modify setSignalProvisioningNotice to only persist the translated summary
(message) to sessionStorage (use SIGNAL_PROVISIONING_NOTICE_STORAGE_KEY and
dispatch SIGNAL_PROVISIONING_NOTICE_EVENT as before) and stop saving the
optional details there—instead log or send the details (matrixErrorMessage) to
console/telemetry for debugging; apply the same change to the other invocation
that currently passes matrixErrorMessage so only the translated message is
persisted and the raw error is logged.
In `@packages/epics/src/common/human-right-panel.tsx`:
- Around line 1845-1929: The effect uses an effect-local `cancelled` flag which
can abort the shared flusher and leave a newer pending update stranded; replace
the effect-local `cancelled` with an unmount-only ref (e.g., `isUnmountedRef`)
and only check that ref to prevent scheduling or retrying after unmount, but do
NOT let the unmount guard cause the shared `flushMessageCountSync` or
`scheduleRetry` to bail out early—remove early returns inside
`flushMessageCountSync` that check the effect-local cancel and instead only stop
retry scheduling/timeout when `isUnmountedRef.current` is true; update all
places referencing `cancelled` to use the new unmount ref and keep shared refs
`pendingCoherenceMessageCountRef` and `coherenceMessageCountSyncInFlightRef`
responsible for draining the queue across re-runs (affecting functions:
flushMessageCountSync, scheduleRetry, coherenceMessageCountSyncTimeoutRef,
lastPersistedCoherenceMessageCountRef).
In `@packages/i18n/src/messages/de.json`:
- Around line 44-45: Update CoherenceTab's copy to finish the terminology
migration: change the back navigation string in the CoherenceTab.backToCoherence
method to use "Signals" (matching Common.Signals/Common.Coherence -> "Signale")
instead of the German "Kohärenz", and update the error message in
CoherenceTab.errorCoherenceTab to reference "signals tab" (or "Signals" as
appropriate) rather than "Kohärenz"; ensure the strings align with
Common.Signals/Common.Coherence usage so the UI uses consistent
"Signale"/"Signals" terminology.
In `@packages/i18n/src/messages/pt.json`:
- Around line 44-45: The Coherence tab copy was not fully updated to "Sinais":
update the remaining hardcoded Portuguese strings in CoherenceTab by replacing
references to "Coerência" with the new "Sinais" text (or use the Common
namespace keys Common.Coherence/Common.Signals) inside
CoherenceTab.backToCoherence and CoherenceTab.errorCoherenceTab so
navigation/back/error messages are consistent with the i18n entries; ensure you
use the same localization accessor (e.g., t('Common.Coherence') or
t('Common.Signals')) used elsewhere in the tab to avoid duplicating literals.
---
Outside diff comments:
In `@packages/i18n/src/messages/es.json`:
- Around line 1978-2018: Update the Spanish copy for the Signals tab to be
consistent: change the "backToCoherence" and "errorCoherenceTab" entries in
es.json to reference "Señales" instead of "Coherencia" so the back label and
load-error message match the renamed tab (these keys are "backToCoherence" and
"errorCoherenceTab" used by CoherenceTab).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6152e6d-2b48-4d2b-b86f-546d24421511
📒 Files selected for processing (8)
packages/epics/src/coherence/components/create-signal-form.tsxpackages/epics/src/coherence/components/signal-section.tsxpackages/epics/src/common/human-right-panel.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/pt.json
Comment on lines
+1845
to
+1929
| useEffect(() => { | ||
| if (mode !== 'coherence') return; | ||
| if (!isMatrixAvailable || !coherenceSlug || !roomId) return; | ||
| let cancelled = false; | ||
|
|
||
| const messageCount = messages.length; | ||
| const previous = lastPersistedCoherenceMessageCountRef.current; | ||
| if (previous?.slug === coherenceSlug && previous.count === messageCount) { | ||
| return; | ||
| } | ||
|
|
||
| if (coherenceMessageCountSyncTimeoutRef.current) { | ||
| clearTimeout(coherenceMessageCountSyncTimeoutRef.current); | ||
| } | ||
|
|
||
| coherenceMessageCountSyncTimeoutRef.current = setTimeout(() => { | ||
| if (cancelled) return; | ||
| pendingCoherenceMessageCountRef.current = { | ||
| slug: coherenceSlug, | ||
| count: messageCount, | ||
| }; | ||
|
|
||
| if (coherenceMessageCountSyncInFlightRef.current) return; | ||
|
|
||
| const flushMessageCountSync = async () => { | ||
| if (cancelled) return; | ||
| coherenceMessageCountSyncInFlightRef.current = true; | ||
| try { | ||
| while (pendingCoherenceMessageCountRef.current) { | ||
| if (cancelled) return; | ||
| const next = pendingCoherenceMessageCountRef.current; | ||
| pendingCoherenceMessageCountRef.current = null; | ||
| try { | ||
| await updateCoherenceBySlug({ | ||
| slug: next.slug, | ||
| messages: next.count, | ||
| }); | ||
| lastPersistedCoherenceMessageCountRef.current = next; | ||
| } catch (error) { | ||
| if (!pendingCoherenceMessageCountRef.current) { | ||
| pendingCoherenceMessageCountRef.current = next; | ||
| } | ||
| throw error; | ||
| } | ||
| } | ||
| } finally { | ||
| coherenceMessageCountSyncInFlightRef.current = false; | ||
| } | ||
| }; | ||
|
|
||
| const scheduleRetry = () => { | ||
| if (cancelled) return; | ||
| if (!pendingCoherenceMessageCountRef.current) return; | ||
| if (coherenceMessageCountSyncTimeoutRef.current) { | ||
| clearTimeout(coherenceMessageCountSyncTimeoutRef.current); | ||
| } | ||
| coherenceMessageCountSyncTimeoutRef.current = setTimeout(() => { | ||
| if (cancelled) return; | ||
| if (coherenceMessageCountSyncInFlightRef.current) return; | ||
| void flushMessageCountSync().catch((retryError) => { | ||
| if (cancelled) return; | ||
| console.warn( | ||
| '[HumanRightPanel] Retry failed to persist coherence message count:', | ||
| retryError, | ||
| ); | ||
| scheduleRetry(); | ||
| }); | ||
| }, 1000); | ||
| }; | ||
|
|
||
| void flushMessageCountSync().catch((error) => { | ||
| if (cancelled) return; | ||
| console.warn( | ||
| '[HumanRightPanel] Failed to persist coherence message count:', | ||
| error, | ||
| ); | ||
| scheduleRetry(); | ||
| }); | ||
| }, 500); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| if (coherenceMessageCountSyncTimeoutRef.current) { | ||
| clearTimeout(coherenceMessageCountSyncTimeoutRef.current); | ||
| coherenceMessageCountSyncTimeoutRef.current = null; |
Contributor
There was a problem hiding this comment.
Don't let an old effect strand the next queued message-count update.
pendingCoherenceMessageCountRef and coherenceMessageCountSyncInFlightRef are shared across renders, but cancelled is effect-local. If signal A is flushing, signal B queues a newer count, and A's effect cleans up before its request resolves, the old flusher returns at Line 1874 with B still pending. Because B already exited at Line 1867 when inFlight was true, nothing restarts the queue until the count changes again, so the card counter can stay stale indefinitely.
Use an unmount-only guard for retry scheduling, but let the shared flusher drain queued payloads across effect re-runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/epics/src/common/human-right-panel.tsx` around lines 1845 - 1929,
The effect uses an effect-local `cancelled` flag which can abort the shared
flusher and leave a newer pending update stranded; replace the effect-local
`cancelled` with an unmount-only ref (e.g., `isUnmountedRef`) and only check
that ref to prevent scheduling or retrying after unmount, but do NOT let the
unmount guard cause the shared `flushMessageCountSync` or `scheduleRetry` to
bail out early—remove early returns inside `flushMessageCountSync` that check
the effect-local cancel and instead only stop retry scheduling/timeout when
`isUnmountedRef.current` is true; update all places referencing `cancelled` to
use the new unmount ref and keep shared refs `pendingCoherenceMessageCountRef`
and `coherenceMessageCountSyncInFlightRef` responsible for draining the queue
across re-runs (affecting functions: flushMessageCountSync, scheduleRetry,
coherenceMessageCountSyncTimeoutRef, lastPersistedCoherenceMessageCountRef).
Comment on lines
+44
to
+45
| "Coherence": "Signale", | ||
| "Signals": "Signale", |
Contributor
There was a problem hiding this comment.
Complete the terminology migration in CoherenceTab copy.
Common.Coherence/Common.Signals now use “Signale”, but CoherenceTab.backToCoherence (Line 1983) and CoherenceTab.errorCoherenceTab (Line 2019) still reference “Kohärenz”, which leaves mixed UI terminology.
As per coding guidelines, update related back/error copy in the Signals tab (“navigation/back copy uses ‘Signals’, and error copy references ‘signals tab’”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/i18n/src/messages/de.json` around lines 44 - 45, Update
CoherenceTab's copy to finish the terminology migration: change the back
navigation string in the CoherenceTab.backToCoherence method to use "Signals"
(matching Common.Signals/Common.Coherence -> "Signale") instead of the German
"Kohärenz", and update the error message in CoherenceTab.errorCoherenceTab to
reference "signals tab" (or "Signals" as appropriate) rather than "Kohärenz";
ensure the strings align with Common.Signals/Common.Coherence usage so the UI
uses consistent "Signale"/"Signals" terminology.
Comment on lines
+44
to
+45
| "Coherence": "Sinais", | ||
| "Signals": "Sinais", |
Contributor
There was a problem hiding this comment.
Finish the Coherence→Signals copy update in CoherenceTab.
Even though Common.Coherence/Common.Signals now use “Sinais”, CoherenceTab.backToCoherence (Line 1982) and CoherenceTab.errorCoherenceTab (Line 2018) still reference “Coerência”, so the tab language is inconsistent.
As per coding guidelines, apply the terminology change to related navigation/back/error copy in the Signals tab.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/i18n/src/messages/pt.json` around lines 44 - 45, The Coherence tab
copy was not fully updated to "Sinais": update the remaining hardcoded
Portuguese strings in CoherenceTab by replacing references to "Coerência" with
the new "Sinais" text (or use the Common namespace keys
Common.Coherence/Common.Signals) inside CoherenceTab.backToCoherence and
CoherenceTab.errorCoherenceTab so navigation/back/error messages are consistent
with the i18n entries; ensure you use the same localization accessor (e.g.,
t('Common.Coherence') or t('Common.Signals')) used elsewhere in the tab to avoid
duplicating literals.
Introduce a centered, low-contrast signal-type icon halo on card hero images so type is visible at a glance without overpowering existing priority overlays. Made-with: Cursor
Guard dynamic type icons and render them via React.createElement so check-types no longer treats icon names as intrinsic JSX elements. Made-with: Cursor
Increase center icon visibility with a refined halo, improved contrast, and corrected overlay layering so the type mark is noticeable without overpowering the hero image. Made-with: Cursor
Match signal hero and action-row vertical sizing to proposal card proportions so card footprints stay visually consistent in mixed grids. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the incorrect function-type guard for Lucide icons so the blended center type icon reliably renders on signal hero images. Co-authored-by: Cursor <cursoragent@cursor.com>
Render signal hero type icons from the Lucide icon map using the string icon keys provided by coherence type options, and simplify the center overlay so icons are clearly visible. Co-authored-by: Cursor <cursoragent@cursor.com>
Switch signal card hero icon rendering to the shared DynamicIcon component so Lucide string names resolve safely and check-types passes. Co-authored-by: Cursor <cursoragent@cursor.com>
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
3 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.
Ready-to-implement specifications
Scope is Signals UI only on the Coherence tab (list cards, create-signal aside, description dialog). No changes to Space Memory or non-signal coherence flows unless explicitly listed.
Functional requirements
apps/web/public/placeholder/signal-type-*.svg. Priority is shown as a tint overlay on the banner (high: destructive, medium: warning, low: success) plus the existing text label. The type badge row still shows the Lucide icon for quick scan.messagesfield (existing backend counter for the signal’s conversation).CreateSignalForminsideProposalOverlayShell) must align with contribution proposal patterns: sticky proposal-style header row (CreateAgreementBaseFields), section blocks for type/priority/tags, bordered inner shell for rich text description.Buttonwithvariant="outline"andcolorVariant="accent"(seeVoteProposalButton).background-2header, consistent borders, neutral outline close.Non-goals (this PR)
messagesuntil a separate sync story exists.Design reference
packages/epics/src/governance/components/vote-proposal-button.tsx—variant="outline"colorVariant="accent".packages/epics/src/agreements/components/create-agreement-base-fields.tsx— sticky header,sectioncards (rounded-xl border border-border/70 bg-muted/20 …), description editor wrapper.packages/epics/src/common/proposal-overlay-shell.tsx— unchanged; form content aligned to proposal density inside existing padding.i18n
CoherenceTab.messageCount— ICU plural for message count (en, de, es, fr, pt).CoherenceTab.priorityVisualAlt— legacy alt key (banner usestypeSignalBannerAlt).CoherenceTab.typeSignalBannerAlt—{type}+{priority}for banneralttext.Assets
apps/web/public/placeholder/signal-type-{opportunity,risk,tension,insight,trend,proposal}.svg— wide (400×96) banner art per type.Implementation traceability
coherence-block.tsx— removed bordered wrapper; flex column spacing only.signal-card.tsx—SIGNAL_TYPE_BANNER+Imagebanner;PRIORITY_BANNER_OVERLAYtint; type badge in body; delete control over banner.signal-card.tsx—t('messageCount', { count: messages ?? 0 });packages/i18nkeys.create-signal-form.tsx— header matches proposal toolbar; sections + rich text border wrapper.signal-card.tsx— Open conversation same asVoteProposalButton:variant="outline"colorVariant="accent".signal-card.tsx—DialogContent/ header / footer classes aligned with proposal modal.QA checklist (manual)
roomId.Summary
Updates Signals on the Coherence tab: proposal-aligned chrome, type banner illustrations with priority tint, message counts, and Vote Now–style open conversation CTA.
Summary by CodeRabbit
New Features
Style
Accessibility
Usability