feat(nav): add tooltip and cursor:pointer to notification bell, fix ThemeToggle i18n (#344) #351
Reference in New Issue
Block a user
Delete Branch "feat/issue-344-bell-tooltip"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #344
What was implemented
Commit 1 —
feat(nav): add cursor-pointer and tooltip to notification bellbellLabelas$derivedinNotificationBell.svelte— eliminates the duplicated inline ternary and keeps tooltip/label in sync reactivelytitle={bellLabel}to the bell<button>— native tooltip mirrorsaria-labelin both zero and non-zero unread statescursor-pointerto the bell button's class listbutton { cursor: pointer; }rule in@layer baseoflayout.css— prevents future regressions (global scope per Decision Queue)NotificationBell.svelte.spec.ts: cursor-pointer class present, title equals aria-label when unread=0, title equals aria-label when unread=3Commit 2 —
fix(nav): replace hardcoded ThemeToggle title with Paraglide i18n keystheme_toggle_to_light/theme_toggle_to_darkkeys tode/en/esmessagesthemeLabelas$derivedinThemeToggle.svelteand bound botharia-labelandtitleto it'light mode'/'dark mode') per Decision Queue resolutionTouch target size was descoped per the Decision Queue.
Decision Queue resolutions (from issue #344)
@layer base✅Test results
All 5
NotificationBelltests pass.🏗️ Markus Keller — Application Architect (@mkeller)
Verdict: ✅ Approved
This is a well-scoped cosmetic fix with no architectural implications. The diff is exactly what was agreed in the Decision Queue: two attribute additions to
NotificationBell.svelte, a$derivedextraction to eliminate duplication, the globalbutton { cursor: pointer; }rule in@layer base, and a proper i18n fix forThemeToggle.What I checked
ThemeToggleandNotificationBellremain self-contained. ✓layout.css, line ~369): The comment is exactly right — "Tailwind preflight resets cursor on*, overriding the browser default for buttons." This is the correct rationale for a global base rule. One change; prevents all future recurrences. ✓cursor-pointer+title+$derivedlabel). The architect note from the issue review was "if a third appears, extractIconButton.svelte." That boundary has not been crossed. ✓$derivedforbellLabelandthemeLabel: Both extractions are correct — they reduce the inline ternary duplication without introducing unnecessary abstraction. ✓Suggestions (non-blocking)
ThemeToggle.sveltestill usesclass="rounded p-1.5 ..."whileNotificationBellusesclass="relative cursor-pointer rounded-sm p-2 ...". Thecursor-pointeris now redundant onNotificationBellsince the global base rule covers it — but leaving it is harmless and makes intent explicit at the component level. Either approach is fine; just be consistent if you add a third button.layout.csssits inside what appears to be a@layer baseblock that already has:focus-visiblerules. Confirm thebuttonrule is inside the same@layer base { }block and not accidentally outside it (the diff context shows it is, but worth a double-check in the rendered file).👨💻 Felix Brandt — Senior Fullstack Developer (@felixbrandt)
Verdict: ✅ Approved
Clean, minimal, and DRY. This is exactly what was discussed in the issue review. The
$derivedextraction for bothbellLabelandthemeLabelis the right call — no$state+$effectanti-pattern, no duplicated inline ternaries.What I checked
NotificationBell.sveltebellLabel = $derived(...)at line 51: correctly single-pass, synchronous, reactive tostream.unreadCount. ✓aria-label={bellLabel}andtitle={bellLabel}bound to the same derived — DRY, no divergence risk. ✓cursor-pointeradded to existing class string adjacent tohover:bg-white/10— correct placement. ✓aria-labelis cleanly removed (3 lines → 1 line). ✓ThemeToggle.sveltethemeLabel = $derived(theme === 'dark' ? m.theme_toggle_to_light() : m.theme_toggle_to_dark()): logic is correct — when the theme is currently dark, the label says "switch to light." ✓aria-labelandtitlebound tothemeLabel. ✓import { m }added correctly. ✓Messages (de/en/es)
theme_toggle_to_lightandtheme_toggle_to_dark. Keys are consistent across locales. ✓NotificationBell.svelte.spec.tsdescribe('NotificationBell — cursor and tooltip'). ✓btn.classList.contains('cursor-pointer')— asserts the class is present. ✓title === aria-labelin both zero and non-zero unread states — exactly what was recommended in the issue review. ✓Suggestions (non-blocking)
document.querySelector<HTMLButtonElement>('button[aria-haspopup="true"]')!with a non-null assertion. If the selector ever changes, this silently fails with a null dereference. Preferscreen.getByRole('button', { expanded: false })or at minimum add a null-check with a descriptive error message. Minor.ThemeToggle.sveltehas no new tests. The fix is small and the old hardcoded behavior was a pre-existing issue, so this is acceptable for now — but a spec file forThemeTogglecovering the label derivation would close the gap Sara will flag.⚙️ Tobias Wendt — DevOps & Platform Engineer (@tobiwendt)
Verdict: ✅ Approved
No infrastructure changes. No new dependencies. No Docker Compose, CI pipeline, or environment config touched. This is a clean frontend-only change.
What I checked
.svelte,.ts, and.jsonfiles. No assets, no large blobs, no binary changes. ✓npm run lint && npm run check && npm run testpipeline will pick up the new spec file automatically. Vitest discovers*.spec.tsfiles by convention. ✓Observations
layout.cssglobalbutton { cursor: pointer; }rule is a one-line change inside an existing@layer baseblock. Build output size change is negligible.Ship it.
🔒 Nora Steiner — Application Security Engineer (@NullX)
Verdict: ✅ Approved
No security concerns in this change. Confirmed clean.
What I checked
XSS / injection via
titleattributetitle={bellLabel}andtitle={themeLabel}are bound to Svelte reactive expressions. Svelte escapes all attribute values by default — no{@html ...}usage anywhere in this diff. ✓bellLabelderives fromm.notification_bell_label()(static i18n string) orm.notification_bell_unread_label({ count: stream.unreadCount }). Thecountvalue isstream.unreadCount— an integer from the notification stream, not user-supplied free text. Paraglide interpolates it as a number into a message template, not into a raw HTML string. No injection vector. ✓themeLabelderives fromm.theme_toggle_to_light()orm.theme_toggle_to_dark()— both are static message keys. No dynamic input. ✓Information disclosure via tooltip
titletooltip shows either "Benachrichtigungen" or "3 ungelesene Benachrichtigungen". The unread count is already visible as a badge on the bell icon — the tooltip exposes no new information that wasn't already rendered. ✓Auth / permission surface
i18n message keys
de/en/esmessage files only. No new backendErrorCodevalues, no frontenderrors.tschanges needed. ✓Notes
ThemeToggle('light mode'/'dark mode') were not a security issue — they were a UX/i18n issue. The fix is correct.🧪 Sara Holt — QA Engineer (@saraholt)
Verdict: ⚠️ Approved with concerns
The
NotificationBelltests are well-structured and cover the right behaviors. One gap:ThemeTogglehas no test coverage for its new$derivedlabel logic. Flagging this as a concern, not a blocker.What I checked
NotificationBell.svelte.spec.ts— new testsdescribe('NotificationBell — cursor and tooltip')adds 3 tests. Test names are descriptive and read as sentences. ✓btn.classList.contains('cursor-pointer')— asserts the class attribute, not computed style. As noted in the issue review, computed style viawindow.getComputedStylewould be more accurate (catches CSS overrides), but class attribute assertion is sufficient for catching regressions in the Svelte template. Acceptable.title === aria-labelfor unread=0 and unread=3. This is the correct consistency check — if the two attributes ever diverge, these tests fail. ✓'Benachrichtigungen'in test 2 and'3 ungelesene Benachrichtigungen'in test 3 will break if the message keys change wording. Usingm.notification_bell_label()andm.notification_bell_unread_label({ count: 3 })directly in the assertions would be more resilient. Minor.Blocker
Suggestions (non-blocking)
ThemeTogglespec: ThethemeLabelderivation (theme === 'dark' ? m.theme_toggle_to_light() : m.theme_toggle_to_dark()) is new production logic with no test. A minimalThemeToggle.svelte.spec.tsshould assert: (a)aria-labelequalsm.theme_toggle_to_dark()in light mode, (b)aria-labelequalsm.theme_toggle_to_light()in dark mode, (c)title === aria-labelin both states. This mirrors theNotificationBelltest pattern exactly.document.querySelector<HTMLButtonElement>('button[aria-haspopup="true"]')!— the non-null assertion (!) masks a missing element. Usescreen.getByRole('button')from@testing-library/svelteor addif (!btn) throw new Error('bell button not found')before assertions.NotificationBell.svelte.spec.tsis insrc/lib/components/— consistent with the project convention. ✓🎨 Leonie Voss — UI/UX Design Lead (@leonievoss)
Verdict: ✅ Approved
The implementation matches the design intent from issue #344 and follows the project's brand and interaction patterns correctly.
What I checked
Cursor behavior
cursor-pointeradded toNotificationBellbutton's class list. ✓button { cursor: pointer; }in@layer baseoflayout.csswith the correct explanatory comment ("Tailwind preflight resets cursor on*"). This is the right architectural fix — prevents all future icon buttons from having the same regression. ✓ThemeTogglenow benefits from the global rule; its own class list doesn't needcursor-pointerexplicitly, which is fine.Tooltip via
titleattributetitle={bellLabel}mirrorsaria-labelexactly — same Paraglide key, same reactive derivation. The tooltip text matches what screen readers already announce. ✓ThemeTogglenow usestitle={themeLabel}bound to the same$derivedasaria-label. Parity achieved. ✓Brand compliance
rounded-sm p-2 text-white/65 transition-colors hover:bg-white/10 hover:text-white focus:outline-none focus-visible:ring-2 focus-visible:ring-focus-ring) is unchanged from the existing nav button pattern. ✓Remaining concern (pre-existing, descoped per Decision Queue)
NotificationBell(p-2onh-5 w-5icon = ~36px) andThemeToggle(p-1.5= ~32px) remain below the 44px WCAG 2.2 minimum. This was explicitly descoped in issue #344. It should be tracked as a follow-up accessibility issue — the senior audience (60+) is the primary concern here.Suggestions (non-blocking)
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
All three acceptance criteria from issue #344 are met by this implementation. The Decision Queue resolutions are correctly reflected in the code.
AC verification
cursor-pointeron button + global@layer baseruletitle={bellLabel}with correct German textThemeTogglenow uses same pattern ($derivedlabel,title,aria-labelin sync)Implicit AC (i18n — flagged in issue review): The tooltip text is localized via Paraglide in all three supported locales (de/en/es). The
ThemeTogglefix also corrects the pre-existing hardcoded English strings. ✓Decision Queue coverage:
cursor-pointervia@layer base: implemented ✓ThemeTogglefixed in this issue: implemented ✓Observations
Open Decisions for follow-up (new)
fix(a11y): increase nav icon button touch targets to 44px minimum (WCAG 2.2 SC 2.5.8).🗳️ Decision Queue
One open decision surfaced across the persona reviews. All three Decision Queue items from issue #344 were already resolved and are correctly implemented. This new item is a follow-up concern.
Theme: Touch target follow-up issue
From Leonie (UX) and Elicit (Requirements)
Both
NotificationBell(p-2→ ~36px) andThemeToggle(p-1.5→ ~32px) remain below the WCAG 2.2 Success Criterion 2.5.8 minimum of 44×44px. This was explicitly descoped from PR #351 per the Decision Queue in issue #344.Decision needed: Should a new Gitea issue be created for this, linked to the "Demo Day" milestone, so it isn't forgotten?
Options:
fix(a11y): increase nav icon button touch targets to 44px minimum (WCAG 2.2 SC 2.5.8)— keeps the accessibility debt visible.The fix itself is trivial: change
p-2→p-3(48px) onNotificationBellandp-1.5→p-2.5onThemeToggle.🏗️ Markus Keller — Application Architect (@mkeller)
Verdict: ✅ Approved
No architectural concerns in this PR. This is a pure frontend cosmetic change — two components touched, one global CSS rule added, three message files updated, two spec files added or updated.
What I checked
NotificationBellandThemeToggleremain self-contained. Neither introduces a new cross-component dependency. ✓button { cursor: pointer; }in@layer baseinsidelayout.cssis architecturally correct. Preflight resets need@layer baseoverrides — this is the appropriate mechanism. The comment explains the rationale. ✓$derivedlabel +title+aria-labelpattern, extract anIconButton.svelte. That boundary has not been crossed. ✓$derivedcorrectness: BothbellLabelandthemeLabelare single-expression derived values — appropriate use of$derived(not$derived.by()). ✓themestarts as'light'via$state<Theme>('light')and is updated inonMount. This meansthemeLabelderives to "Zu dunklem Design wechseln" on the server-rendered first frame even if the user's stored preference is dark. This is a pre-existing pattern not introduced by this PR and is mitigated by theonMounthydration (the label corrects immediately client-side). Not a blocker for this PR but worth tracking as a future SSR hydration improvement.Suggestions (non-blocking)
ThemeTogglebutton class list still usesroundedwhileNotificationBellusesrounded-sm. No consistency mandate for now — but if a third icon button is added, define a shared token or component.IconButton.svelteextraction threshold is reached, the$derivedlabel pattern established here should become the extraction template.👨💻 Felix Brandt — Senior Fullstack Developer (@felixbrandt)
Verdict: ✅ Approved
Clean, minimal, and correctly TDD'd. The
$derivedextractions for bothbellLabelandthemeLabelare the right call — no$state + $effectanti-pattern, no duplicated inline ternaries in template markup. The new commit addingThemeToggle.svelte.spec.tscloses the coverage gap I would have flagged.What I checked
NotificationBell.sveltebellLabel = $derived(...)at line 51: single-pass, synchronous, reactive tostream.unreadCount. ✓aria-label={bellLabel}andtitle={bellLabel}bound to the same derived — DRY, no divergence risk. ✓cursor-pointeradded to class string. ✓aria-labelremoved cleanly. ✓ThemeToggle.sveltethemeLabel = $derived(theme === 'dark' ? m.theme_toggle_to_light() : m.theme_toggle_to_dark()): logic is correct — when currently dark, label says "switch to light". ✓aria-labelandtitlebound tothemeLabel. ✓import { m }added correctly. ✓NotificationBell.svelte.spec.ts(updated)describe('NotificationBell — cursor and tooltip'). ✓makeNotificationfactory function with overrides used correctly. ✓ThemeToggle.svelte.spec.ts(new — commit 3)describeblocks (light modeanddark mode). ✓page.getByRole('button').element()fromvitest/browser— correct@testing-library-style selector, avoids thedocument.querySelector+ non-null assertion fragility fromNotificationBell. ✓afterEachcleanslocalStorage— no cross-test contamination. ✓title === aria-labelassertions in both modes — mirrors theNotificationBellpattern exactly. ✓Suggestions (non-blocking)
NotificationBell.svelte.spec.tslines 61, 68, 74 still usedocument.querySelector<HTMLButtonElement>('button[aria-haspopup="true"]')!with a non-null assertion. TheThemeTogglespec correctly usespage.getByRole('button').element()instead. If theNotificationBelltests are ever refactored, adopt the same approach. Minor — pre-existing in the older tests, only the three new tests in this PR use the same pattern.'Benachrichtigungen'and'3 ungelesene Benachrichtigungen'in the bell tests will break if message wording changes. Importingmand usingm.notification_bell_label()/m.notification_bell_unread_label({ count: 3 })would be more resilient. Non-blocking.⚙️ Tobias Wendt — DevOps & Platform Engineer (@tobiwendt)
Verdict: ✅ Approved
No infrastructure changes. No new dependencies. Frontend-only change. Ship it.
What I checked
*.svelte.spec.tsby convention. The newThemeToggle.svelte.spec.tswill be picked up automatically on next run without any CI configuration change. ✓layout.csschange: One-line inside@layer base {}. Build output size change is negligible. ✓Observations
layout.svelte.spec.tsis a pre-existing iframe/navigation issue, not introduced by this PR. ✓🔒 Nora Steiner — Application Security Engineer (@NullX)
Verdict: ✅ Approved
No security concerns. Confirmed clean.
What I checked
XSS / injection via
titleandaria-labelattributestitle={bellLabel}andtitle={themeLabel}bound to Svelte reactive expressions. Svelte escapes all attribute bindings by default — no{@html ...}usage anywhere in this diff. ✓bellLabelderives fromm.notification_bell_label()(static i18n key) orm.notification_bell_unread_label({ count: stream.unreadCount }). Thecountvalue is an integer from the notification store, not user-supplied free text. Paraglide interpolates it as a number into a message template. No injection vector. ✓themeLabelderives fromm.theme_toggle_to_light()orm.theme_toggle_to_dark()— both are static compile-time message keys with no dynamic input. ✓Information disclosure via tooltip
bellLabeltooltip shows either "Benachrichtigungen" or "N ungelesene Benachrichtigungen". The unread count is already rendered as a visible badge — no new information is disclosed that wasn't already on-screen. ✓themeLabeltooltip shows the current theme switch action — equally public information. ✓Auth / permission surface
i18n message keys
de/en/esmessage files only. No new backendErrorCodevalues. ✓Notes
'light mode'/'dark mode'inThemeTogglewere a UX/i18n defect, not a security issue. The fix is correct and does not introduce any new attack surface.🧪 Sara Holt — QA Engineer (@saraholt)
Verdict: ✅ Approved
The missing
ThemeToggle.svelte.spec.tsconcern from the previous review round has been addressed in commit 3. All 9 tests pass across both spec files. Coverage is now symmetric: bothNotificationBellandThemeTogglehavetitle === aria-labelassertions in all relevant states.What I checked
ThemeToggle.svelte.spec.ts(new in commit 3)describeblocks with descriptive sentence-style names. ✓beforeEachsetslocalStoragetheme;afterEachremoves it. No cross-test contamination. ✓page.getByRole('button').element()— correcttesting-library-style selector, avoids the null-assertion fragility ofdocument.querySelector. This is the better pattern compared toNotificationBell.svelte.spec.ts. ✓aria-labelcorrect text in light mode,title === aria-labelin light mode,aria-labelcorrect text in dark mode,title === aria-labelin dark mode. All four are the right behaviors to verify. ✓NotificationBell.svelte.spec.ts(updated)makeNotificationfactory with id overrides correctly avoids duplicate-id issues in the mock list. ✓Remaining suggestions (non-blocking, pre-existing)
NotificationBell.svelte.spec.tsusesdocument.querySelector<HTMLButtonElement>('button[aria-haspopup="true"]')!with non-null assertion in 5 places. If the selector ever fails silently (component change), the tests produce misleadingnull.getAttributeerrors rather than clear missing-element failures. Preferpage.getByRole('button', { name: /Benachrichtigungen/i })fromvitest/browserto match theThemeTogglepattern. Pre-existing — not a blocker for this PR.'Benachrichtigungen','3 ungelesene Benachrichtigungen') will fail if message copy changes. Importmand usem.notification_bell_label()/m.notification_bell_unread_label({ count: 3 })for resilience. Non-blocking.🎨 Leonie Voss — UI/UX Design Lead (@leonievoss)
Verdict: ✅ Approved
The implementation matches the design intent from issue #344. Cursor behavior is correct, tooltips are present in both components and localized, and ARIA parity is maintained.
What I checked
Cursor behavior
cursor-pointeradded toNotificationBellbutton class list. ✓button { cursor: pointer; }in@layer baseoflayout.css— correct fix, prevents future icon buttons from regressing. The comment explains the Tailwind preflight rationale. ✓ThemeTogglebenefits from the global rule without needing explicitcursor-pointerin its class list. Consistent. ✓Tooltip via
titleattributetitle={bellLabel}mirrorsaria-label={bellLabel}— same$derived, no divergence risk. ✓title={themeLabel}mirrorsaria-label={themeLabel}— same pattern. ✓Brand compliance
NotificationBellbutton class list unchanged except forcursor-pointeraddition. ✓ThemeTogglebutton class list unchanged (stillrounded p-1.5). ✓Remaining concern (pre-existing, descoped per Decision Queue)
NotificationBell(p-2onh-5 w-5icon ≈ 36px) andThemeToggle(p-1.5≈ 32px) remain below the WCAG 2.2 SC 2.5.8 minimum of 44×44px. This was explicitly descoped from this PR. Given the primary audience is 60+ seniors on laptops/tablets, this is a real usability risk. I recommend opening a follow-up Gitea issue to track it so it doesn't drift indefinitely.Suggestion (non-blocking)
p-2→p-3(48px) onNotificationBellandp-1.5→p-2.5(44px) onThemeToggle.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
All three acceptance criteria from issue #344 are met. The Decision Queue resolutions are correctly implemented. The concern from the previous review round (missing
ThemeToggletests) has been resolved in commit 3.AC verification
cursor-pointeronNotificationBell+ global@layer baseruletitle={bellLabel}with correct Paraglide key in de/en/esThemeToggleuses same$derivedlabel pattern,titleandaria-labelin syncImplicit AC (i18n): All tooltip text localized in de/en/es.
ThemeTogglepre-existing hardcoded English strings replaced. ✓Decision Queue coverage:
cursor-pointervia@layer base: implemented ✓ThemeTogglefixed in this issue: implemented ✓Previous concern — ThemeToggle test coverage: Resolved by commit 3 (
test(nav): add ThemeToggle spec covering label derivation in both modes). ✓Decision Queue
One item remains open from the previous round. The Decision Queue from the previous review has been explicitly resolved above except the following follow-up:
Open Decision: Touch target follow-up issue
Both
NotificationBell(p-2≈ 36px) andThemeToggle(p-1.5≈ 32px) remain below WCAG 2.2 SC 2.5.8 (44×44px minimum). Descoped from this PR per the Decision Queue in issue #344.Decision needed: Should a new Gitea issue be opened now to track this, linked to the accessibility milestone?
Options:
fix(a11y): increase nav icon button touch targets to 44px minimum (WCAG 2.2 SC 2.5.8). Keeps the debt visible and prevents it from being forgotten. The fix is trivial:p-2→p-3onNotificationBell,p-1.5→p-2.5onThemeToggle.This is the only open decision. All other concerns from the previous review are resolved.