Skip to content

feat(ui-react): add user-facing announcement modal#6265

Open
luizhf42 wants to merge 2 commits intomasterfrom
feat/ui-react/announcement-modal
Open

feat(ui-react): add user-facing announcement modal#6265
luizhf42 wants to merge 2 commits intomasterfrom
feat/ui-react/announcement-modal

Conversation

@luizhf42
Copy link
Copy Markdown
Member

What

Implements the user-facing announcement modal in the React UI, matching the feature already present in the Vue UI. When a new announcement is posted via the admin panel, logged-in users see a dismissible modal on their next visit.

Why

Feature parity gap tracked in team issue #91. The admin-side CRUD for announcements was already in place; this adds the missing consumer of that data.

Changes

  • useLatestAnnouncement hook: fetches the latest announcement in two sequential React Query calls — list (page 1, perPage 1, desc order) to get the UUID, then a detail fetch for the full content. Both queries are gated on getConfig().announcements.
  • AnnouncementModal: BaseDialog-based dialog (size md) with a header (title + date + close button), a scrollable read-only Tiptap body capped at 60vh, and a "Got it" footer button. Uses the same Tiptap extension set as the admin AnnouncementContent viewer, with isAllowedUrl link validation and noopener noreferrer on external links.
  • AnnouncementModalTrigger: mirrors the WelcomeWizardTrigger pattern — derives show during render, no setState-in-effect. Tracks dismissed announcements in localStorage under key "announcement" using btoa(JSON.stringify(announcement)) as the hash, matching the Vue UI's storage contract so behaviour is consistent across both UIs.
  • AppLayout: mounts AnnouncementModalTrigger alongside WelcomeWizardTrigger.

Testing

Enable the flag in ui-react/apps/console/public/config.json ("announcements": true), create an announcement via the admin panel, then log in as a regular user — the modal should appear once and not reappear after dismissal. Updating the announcement content should cause it to reappear on the next load.

@luizhf42 luizhf42 requested a review from a team as a code owner April 30, 2026 20:08
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

Claude finished @luizhf42's task in 1m 12s —— View job


Code Review Complete

Reviewed 8 files across code quality, security, testing, TypeScript/React patterns, and architecture. 2 inline issues posted (1 high, 1 medium) — both in test files.

The implementation itself is well-structured: the hook pattern, component split, feature flag gating, storage contract compatibility with the Vue UI, and Tiptap security configuration (link validation, noopener noreferrer, read-only mode) all look correct.

Inline comments posted

Severity File Issue
high AnnouncementModal.test.tsx:107-113 Timezone-dependent date assertion — midnight UTC rolls to previous day in western timezones
medium useLatestAnnouncement.test.ts:158-173 Raw setTimeout(50) instead of waitFor — sole outlier in the file, potentially flaky

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

@otavio
Copy link
Copy Markdown
Member

otavio commented Apr 30, 2026

/review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

Claude finished @otavio's task in 14m 10s —— View job


Code Review Complete

Reviewed 8 files across code quality, security, testing, TypeScript/React patterns, and architecture. 2 inline issues posted (1 high, 1 medium).

The implementation is well-structured overall: the hook composition, component split with feature-flag gating, Tiptap security configuration (editable: false, link validation, noopener noreferrer, base64 image blocking), accessibility (aria-labelledby, focus trap via BaseDialog), and localStorage contract compatibility with the Vue UI all look correct. No security vulnerabilities, no architecture concerns, no cross-repo conflicts with the cloud/ enterprise code.

Inline comments posted

Severity File Issue
high AnnouncementModalTrigger.tsx:9-11 btoa(JSON.stringify(...)) throws InvalidCharacterError on non-ASCII announcement content (emoji, accented chars, CJK) — crashes the modal or prevents dismissal
medium useLatestAnnouncement.test.ts:232-235 waitFor guard resolves immediately (mock called on first render), so the test passes due to initial loading state rather than the intended partial-loading scenario

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment on lines +9 to +11
function computeHash(announcement: Announcement): string {
return btoa(JSON.stringify(announcement));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runtime crash on non-ASCII content (high)

btoa() only accepts Latin-1 strings (U+0000–U+00FF). When an admin saves an announcement with emoji, accented characters, or CJK text, btoa(JSON.stringify(announcement)) throws InvalidCharacterError. This affects both call sites — the show check (line 34) and markSeen (line 37) — so the modal either crashes on render or cannot be dismissed.

Compatibility note: The Vue UI uses the identical btoa(JSON.stringify(...)) pattern with the same "announcement" localStorage key (AnnouncementsModal.vue:57, UserWarning.vue:112). Any fix here must either be applied to both UIs simultaneously, or use an approach that stays compatible.

The simplest compatible fix is the standard UTF-8 → Latin-1 encoding trick:

Suggested change
function computeHash(announcement: Announcement): string {
return btoa(JSON.stringify(announcement));
}
function computeHash(announcement: Announcement): string {
const json = JSON.stringify(announcement);
return btoa(
Array.from(new TextEncoder().encode(json), (b) =>
String.fromCharCode(b),
).join(""),
);
}

This produces the same output as btoa(JSON.stringify(...)) for Latin-1 content (preserving backward compatibility with existing localStorage entries) while correctly handling non-ASCII characters. The Vue UI would need the equivalent change.

Fix this →

Comment on lines +232 to +235
// Let the list query settle so latestUuid is set
await waitFor(() =>
expect(mockGetAnnouncementOptions).toHaveBeenCalled(),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test passes for the wrong reason — does not verify the intended scenario (medium)

getAnnouncementOptions is called inline on every render (line 24 of the hook: ...getAnnouncementOptions({ path: { uuid: latestUuid ?? "" } })), so the mock is already called on the first render — before the list query resolves. This waitFor resolves immediately, and isLoading is true because listResult.isLoading is still true, not because of the intended condition !!latestUuid && detailResult.isLoading.

This effectively duplicates the "returns isLoading true while list query is pending" test (line 123) and would not catch a regression in the partial-loading branch.

Wait for the mock to be called with the actual UUID (which only happens after the list resolves):

Suggested change
// Let the list query settle so latestUuid is set
await waitFor(() =>
expect(mockGetAnnouncementOptions).toHaveBeenCalled(),
);
await waitFor(() =>
expect(mockGetAnnouncementOptions).toHaveBeenCalledWith(
expect.objectContaining({ path: { uuid: "ann-uuid-1" } }),
),
);

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants