Skip to content

Story 2436: Mailing List Subscription Post Auth#2475

Open
herzog0 wants to merge 8 commits into
developfrom
teo/2436-ml-post-auth
Open

Story 2436: Mailing List Subscription Post Auth#2475
herzog0 wants to merge 8 commits into
developfrom
teo/2436-ml-post-auth

Conversation

@herzog0

@herzog0 herzog0 commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Issue: #2436

⚠️ Base branch is teo/2283-mailing-list-modal

Summary & Context

Adds a post-login mailing list subscription modal that appears automatically on the homepage the first (and only) time a user lands there after logging in. The modal pre-fills the user's account email and lets them choose which lists to subscribe to.

Changes

  • users/signals.py - set request.session["show_ml_post_auth_modal"] in user_logged_in when user hasn't seen the modal yet (User.data["ml_post_auth_seen"] falsy)
  • ak/views.py (HomepageView) - pop session flag in get_v3_context_data; mark user as seen immediately; inject modal list context
  • mailing_list/views.py - add PostAuthSubscribeView (login required); reuses existing subscription + confirmation-email logic; returns empty HTMX fragment to remove modal from DOM
  • mailing_list/urls.py - register /mailing-list/post-auth-subscribe/
  • templates/v3/includes/_mailing_list_post_auth_modal.html - new standalone modal partial; auto-opens via Alpine x-init + CSS :target; reuses .mailing-list-modal__* classes for checkboxes/list items
  • static/css/v3/mailing-list-card.css - add .ml-post-auth-modal__* classes for email row, body, footer row, and count
  • templates/v3/homepage.html - conditionally include the modal when show_ml_post_auth_modal is in context

How the "seen once" logic works

  1. User logs in - user_logged_in signal sets request.session["show_ml_post_auth_modal"] = True (only if User.data["ml_post_auth_seen"] is not set)
  2. User lands on homepage - HomepageView.get_v3_context_data pops the session key and immediately sets User.data["ml_post_auth_seen"] = True on the User record
  3. Template renders the modal; Alpine x-init opens it by setting the URL hash
  4. Close/cancel: no backend call needed - the seen flag is already persisted from step 2
  5. Subscribe: HTMX POSTs to PostAuthSubscribeView, which creates PENDING subscription records and sends the confirmation email, then returns an empty response that HTMX uses to remove the modal from DOM

Risks & Considerations

  • The User.data JSONField is used for the ml_post_auth_seen flag - no migration needed since the field already exists and defaults to {}
  • The modal only renders on V3 homepage (waffle v3 flag gate via HomepageView.get_v3_context_data)
  • No-JS: modal won't auto-open (Alpine required), but subscription is still accessible via the mailing list card on other pages

Screenshots

image image image

Peer-review testing steps

  1. Mkae sure the v3 flag is enabled
  2. Log out completely
  3. Log in - you should be redirected to homepage and the modal should open automatically
  4. Subscribe or cancel - modal closes, no reappearance on refresh
  5. Log out and log in again - modal should NOT appear (flag set in the user model)
  6. For a fresh test (you'll have to log out and log in again):
    • Open a djano shell in a new terminal with docker compose exec web python manage.py shell
    • Run (replace your email):
from users.models import User
user = User.objects.get(email="your@email.com")
user.data.pop("ml_post_auth_seen", None)
user.save()

Self-review Checklist

  • Tag at least one team member from each team to review this PR
  • Link this PR to the related GitHub Project ticket

Frontend

  • UI implementation matches Figma design
  • Tested in light and dark mode
  • Responsive / mobile verified
  • Accessibility checked (keyboard navigation, etc.)
  • Ensure design tokens are used for colors, spacing, typography, etc. - No hardcoded values
  • Test without JavaScript (if applicable)
  • No console errors or warnings

Summary by CodeRabbit

  • New Features

    • Added a post-login mailing list subscription prompt on the homepage for eligible users.
    • Users can select one or more mailing lists, enter/confirm their email, and submit without leaving the page.
  • Bug Fixes

    • Improved multi-list subscription flow to handle partial failures more reliably and only send confirmations when appropriate.
    • Failed confirmation attempts now properly roll back newly created pending subscriptions.
  • UX / Accessibility

    • Improved dialog/backdrop behavior (including checkbox-driven dialogs) and refined modal accessibility and non-JavaScript fallbacks.

@herzog0 herzog0 linked an issue May 29, 2026 that may be closed by this pull request
@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch from a2851ad to a7c3795 Compare May 29, 2026 17:03
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from 75d904c to fcfd84e Compare May 29, 2026 17:03
@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch from a7c3795 to d4c3e88 Compare May 29, 2026 17:05
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from fcfd84e to 6bc71c1 Compare May 29, 2026 17:05
@julioest julioest self-requested a review June 2, 2026 15:25

@julioest julioest left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Works as expected on my end ✅

@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch from d4c3e88 to 6a64a43 Compare June 3, 2026 15:22
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch 2 times, most recently from c722aa4 to bd2f4c3 Compare June 3, 2026 17:11
@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch from 99178e5 to 879e91d Compare June 8, 2026 14:48
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from bd2f4c3 to abb2720 Compare June 8, 2026 14:48
@ycanales ycanales self-requested a review June 11, 2026 16:59
@ycanales

Copy link
Copy Markdown
Collaborator

Works great, approving :)

Should the modal be disimissable with Esc key? I noticed that pattern in other modals with aria-label="Close", it does not work here so maybe that attribute that dialog.js uses to bind the Esc key is missing?

@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch from 879e91d to 2c676fc Compare June 17, 2026 15:56
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from abb2720 to d62840d Compare June 17, 2026 16:37
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a post-login mailing list subscription modal. Login now sets a session flag, the homepage consumes it to render the modal, and a new authenticated subscribe endpoint handles submissions. Dialog behavior, template markup, and CSS were updated to support the new modal flow.

Changes

Post-auth mailing list modal

Layer / File(s) Summary
Login signal sets show_ml_post_auth_modal flag
users/signals.py
user_logged_in_handler sets session["show_ml_post_auth_modal"] when user.data["ml_post_auth_seen"] is falsy.
Homepage context and modal include
ak/views.py, templates/v3/homepage.html
HomepageView.get_v3_context_data consumes the session flag, marks ml_post_auth_seen, and populates subscribe URL, mailing list labels, and user email; the homepage template conditionally includes the modal partial.
Subscription helper, PRG handling, and post-auth endpoint
mailing_list/views.py, mailing_list/urls.py
Adds _subscribe_pending, refactors authenticated subscription handling to use it, changes non-HTMX card behavior to PRG redirects, and registers PostAuthSubscribeView at post-auth-subscribe/.
Checkbox-driven dialog behavior
static/css/v3/dialog.css, static/js/dialog.js, templates/v3/includes/_content_modal.html, templates/v3/includes/_dialog.html
Adds checkbox-open dialog support in CSS and JS, including focus/close handling, and removes aria-hidden from dialog backdrops.
Mailing list card trigger and form updates
templates/v3/includes/_mailing_list_card.html
Replaces JS-driven manage triggers with direct hash links and updates the existing subscription modal form/backdrop markup.
Post-auth modal template and styling
templates/v3/includes/_mailing_list_post_auth_modal.html, static/css/v3/mailing-list-card.css
Adds the new modal template with Alpine.js, HTMX, list selection, and submit controls, plus CSS for its layout and form fields.

Sequence Diagram

sequenceDiagram
  participant Browser
  participant HomepageView
  participant PostAuthSubscribeView
  participant _subscribe_pending
  participant UserMailingListSubscription

  Browser->>HomepageView: GET homepage after login
  HomepageView->>HomepageView: consume show_ml_post_auth_modal
  HomepageView->>HomepageView: save ml_post_auth_seen and build modal context
  HomepageView-->>Browser: render page with post-auth modal
  Browser->>PostAuthSubscribeView: POST subscription form
  PostAuthSubscribeView->>_subscribe_pending: request, user, email, list_ids
  _subscribe_pending->>UserMailingListSubscription: upsert PENDING rows
  _subscribe_pending-->>PostAuthSubscribeView: succeeded IDs and error state
  PostAuthSubscribeView-->>Browser: HTMX no-op or redirect
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • boostorg/website-v2#2444: Shares the same mailing-list subscription path in mailing_list/views.py and extends the pending-subscription flow that this PR refactors and reuses.

Suggested reviewers

  • julhoang
  • ycanales
  • jlchilders11

A rabbit hopped in with a login-day grin,
Saw a modal appear and the lists within.
“Click, check, and subscribe,” the soft whiskers say,
With HTMX and Alpine to guide the way.
Then off through the burrow, the form sent its cheer,
One tiny hop later — the choices were clear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately highlights the main change: a post-auth mailing list subscription flow.
Description check ✅ Passed The description matches the template well, with issue, summary, changes, risks, screenshots, and checklist sections included.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch teo/2436-ml-post-auth

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 OpenGrep (1.23.0)
mailing_list/views.py

┌──────────────┐
│ Opengrep CLI │
└──────────────┘

�[32m✔�[39m �[1mOpengrep OSS�[0m
�[32m✔�[39m Basic security coverage for first-party code vulnerabilities.

[00.13][ERROR]: unable to find a config; path .coderabbit-opengrep-fallback.yml does not exist

static/js/dialog.js

┌──────────────┐
│ Opengrep CLI │
└──────────────┘

�[32m✔�[39m �[1mOpengrep OSS�[0m
�[32m✔�[39m Basic security coverage for first-party code vulnerabilities.

[00.13][ERROR]: unable to find a config; path .coderabbit-opengrep-fallback.yml does not exist

🔧 Stylelint (17.14.0)
static/css/v3/dialog.css

Error: ENOENT: no such file or directory, open '/.stylelintrc.json'
at async open (node:internal/fs/promises:640:25)
at async Object.readFile (node:internal/fs/promises:1287:14)
at async #readConfiguration (/usr/local/lib/node_modules/stylelint/node_modules/cosmiconfig/dist/Explorer.js:83:26)
at async load (/usr/local/lib/node_modules/stylelint/node_modules/cosmiconfig/dist/Explorer.js:20:48)
at async Explorer.load (/usr/local/lib/node_modules/stylelint/node_modules/cosmiconfig/dist/Explorer.js:23:20)
at async getConfigForFile (file:///usr/local/lib/node_modules/stylelint/lib/getConfigForFile.mjs:72:5)
at async resolveOptionValue (file:///usr/local/lib/node_modules/stylelint/lib/utils/resolveOptionValue.mjs:27:24)
at async standalone (file:///usr/local/lib/node_modules/stylelint/lib/standalone.mjs:127:22)


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch 2 times, most recently from a9e7cdf to c8e16c5 Compare June 17, 2026 19:45
@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch 2 times, most recently from 83e72a6 to 1bb6fe2 Compare June 17, 2026 19:46
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from c8e16c5 to 3fb8455 Compare June 17, 2026 19:47
@herzog0 herzog0 force-pushed the teo/2283-mailing-list-modal branch 2 times, most recently from 61a5bfe to f5b0031 Compare June 25, 2026 17:03
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from 3fb8455 to b7f1334 Compare June 25, 2026 17:04
Base automatically changed from teo/2283-mailing-list-modal to develop June 25, 2026 17:07
feat: inject post-auth modal context in HomepageView

feat: add PostAuthSubscribeView for post-login modal

feat: add post-auth mailing list modal template

feat: add post-auth modal CSS

feat: wire post-auth modal into v3 homepage

fix: correct post-auth modal width, email field, and button padding

fix: use $refs for checkbox container to fix unselect all

fix: drop only from modal include so CSRF token is available

refactor: extract _subscribe_pending helper to remove duplicate sub logic

feat: toggle select all / unselect all button based on checked state

fix: add missing bottom padding below select/unselect all row

fix: title padding

fix: correct email field text color and font size to match text field spec

fix: revert email field color and font-size to match figma spec

fix: colors and radii

fix: remove aria-hidden from backdrop links to prevent focus conflict warning

fix: rebase fixes
@herzog0 herzog0 force-pushed the teo/2436-ml-post-auth branch from b7f1334 to 3061f4d Compare June 25, 2026 17:07

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@mailing_list/views.py`:
- Around line 695-707: The HTMX branch in the subscription flow is returning an
empty response even when `_is_rate_limited(request)` blocks the action or
`_subscribe_pending()` fails, which causes the modal to close on a silent
failure. Update the subscription handler around the `list_ids`,
`_is_rate_limited`, and `_subscribe_pending` logic so it detects these failure
cases, keeps the modal open, and returns an error state/message instead of
`HttpResponse("")` when the subscription did not succeed. Ensure the
`_is_htmx(request)` response path only dismisses the modal after a successful
subscribe.
- Around line 95-117: The rollback in the confirmation-email failure path is
deleting all subscriptions in succeeded, but some of those rows may have already
existed before this request. In the UserMailingListSubscription update_or_create
flow, capture the created flag or primary key for each list_id inside the
transaction, store only rows actually created by this request, and use that
tracked set in the exception handler instead of filtering by user and
list_id__in=succeeded. Keep the fix localized to the subscription creation loop
and the _send_confirmation_email rollback block in mailing_list/views.py.

In `@static/css/v3/mailing-list-card.css`:
- Around line 254-263: The email field’s invalid state is overriding the
keyboard focus cue, so fix the interaction between
`.ml-post-auth-modal__email-field:focus-visible` and
`.ml-post-auth-modal__email-field--error`. Keep a visible focus indicator when
the field is both focused and in error by adding a more specific focused-error
state or adjusting the error styles so they don’t suppress the focus ring.
Update the focus styling in this CSS module to ensure the focus-visible cue
remains clear even when validation errors are present.

In `@templates/v3/includes/_content_modal.html`:
- Line 21: The dialog backdrop anchor is now exposed to assistive tech without
an accessible name, so update the backdrop link in the modal template to either
remain hidden from screen readers or provide an appropriate label. Adjust the
dialog-modal__backdrop element so it has a clear accessible name consistent with
the close control, or mark it non-interactive for assistive technologies if it
is only decorative.

In `@templates/v3/includes/_dialog.html`:
- Line 28: The dialog backdrop anchor is now exposed to assistive tech without
any accessible name, so it should either be hidden from screen readers or given
a label. Update the dialog template around dialog-modal__backdrop to make the
backdrop non-interactive for accessibility, or add an appropriate accessible
name so it no longer appears as an empty link before the labeled close action.

In `@templates/v3/includes/_mailing_list_card.html`:
- Line 102: The modal backdrop link in the mailing list card template is now
exposed to assistive tech without an accessible name, so it should be hidden
from the accessibility tree or otherwise given a proper label. Update the dialog
backdrop anchor in the modal markup to match the accessible behavior used by the
other modal templates, and ensure the visible close control remains the first
labeled focus target in this component.

In `@templates/v3/includes/_mailing_list_post_auth_modal.html`:
- Around line 38-39: The modal hash handling in the mailing list post-auth
template leaves stale history when closing via the backdrop or ESC path. Update
the close behavior for the modal setup in the x-init / dialog-modal backdrop
flow so every dismissal path clears the `#ml-post-auth-modal` hash, not just the
Cancel action. Make the cleanup consistent in the modal’s close link/backdrop
handling so Back does not reopen the modal after it has been dismissed.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3a177ca3-b364-47e1-8255-eb8ad955e9fa

📥 Commits

Reviewing files that changed from the base of the PR and between dccd443 and 90b84b1.

📒 Files selected for processing (10)
  • ak/views.py
  • mailing_list/urls.py
  • mailing_list/views.py
  • static/css/v3/mailing-list-card.css
  • templates/v3/homepage.html
  • templates/v3/includes/_content_modal.html
  • templates/v3/includes/_dialog.html
  • templates/v3/includes/_mailing_list_card.html
  • templates/v3/includes/_mailing_list_post_auth_modal.html
  • users/signals.py

Comment thread mailing_list/views.py
Comment on lines +95 to +117
succeeded = []
for lid in list_ids:
try:
with transaction.atomic():
UserMailingListSubscription.objects.update_or_create(
user=user,
list_id=lid,
defaults={"email": email, "status": SubscriptionStatus.PENDING},
)
succeeded.append(lid)
except IntegrityError:
pass

if not succeeded:
return [], None

try:
_send_confirmation_email(request, email, user.pk, succeeded)
except Exception as exc:
logger.error("Failed to send confirmation email to %s: %s", email, exc)
UserMailingListSubscription.objects.filter(
user=user, list_id__in=succeeded
).delete()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Roll back only rows created by this request.

The rollback path deletes every UserMailingListSubscription matching user/list_id__in=succeeded, but update_or_create() does not guarantee those rows were created here. A duplicate submit race can hit created=False, and an email failure in this request would then delete the other request’s pending row. Track the created PKs (or the created flag) and only delete those on rollback.

Suggested fix
 def _subscribe_pending(
     request, user, email: str, list_ids: list[str]
 ) -> tuple[list[str], str | None]:
@@
-    succeeded = []
+    succeeded = []
+    created_pks = []
     for lid in list_ids:
         try:
             with transaction.atomic():
-                UserMailingListSubscription.objects.update_or_create(
+                sub, created = UserMailingListSubscription.objects.update_or_create(
                     user=user,
                     list_id=lid,
                     defaults={"email": email, "status": SubscriptionStatus.PENDING},
                 )
+            if created:
+                created_pks.append(sub.pk)
             succeeded.append(lid)
         except IntegrityError:
             pass
@@
     try:
         _send_confirmation_email(request, email, user.pk, succeeded)
     except Exception as exc:
         logger.error("Failed to send confirmation email to %s: %s", email, exc)
-        UserMailingListSubscription.objects.filter(
-            user=user, list_id__in=succeeded
-        ).delete()
+        UserMailingListSubscription.objects.filter(pk__in=created_pks).delete()
         return [], "Could not send confirmation email. Please try again."
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
succeeded = []
for lid in list_ids:
try:
with transaction.atomic():
UserMailingListSubscription.objects.update_or_create(
user=user,
list_id=lid,
defaults={"email": email, "status": SubscriptionStatus.PENDING},
)
succeeded.append(lid)
except IntegrityError:
pass
if not succeeded:
return [], None
try:
_send_confirmation_email(request, email, user.pk, succeeded)
except Exception as exc:
logger.error("Failed to send confirmation email to %s: %s", email, exc)
UserMailingListSubscription.objects.filter(
user=user, list_id__in=succeeded
).delete()
succeeded = []
created_pks = []
for lid in list_ids:
try:
with transaction.atomic():
sub, created = UserMailingListSubscription.objects.update_or_create(
user=user,
list_id=lid,
defaults={"email": email, "status": SubscriptionStatus.PENDING},
)
if created:
created_pks.append(sub.pk)
succeeded.append(lid)
except IntegrityError:
pass
if not succeeded:
return [], None
try:
_send_confirmation_email(request, email, user.pk, succeeded)
except Exception as exc:
logger.error("Failed to send confirmation email to %s: %s", email, exc)
UserMailingListSubscription.objects.filter(pk__in=created_pks).delete()
return [], "Could not send confirmation email. Please try again."
🧰 Tools
🪛 Ruff (0.15.18)

[warning] 113-113: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mailing_list/views.py` around lines 95 - 117, The rollback in the
confirmation-email failure path is deleting all subscriptions in succeeded, but
some of those rows may have already existed before this request. In the
UserMailingListSubscription update_or_create flow, capture the created flag or
primary key for each list_id inside the transaction, store only rows actually
created by this request, and use that tracked set in the exception handler
instead of filtering by user and list_id__in=succeeded. Keep the fix localized
to the subscription creation loop and the _send_confirmation_email rollback
block in mailing_list/views.py.

Comment thread mailing_list/views.py
Comment on lines +695 to +707
if list_ids and not _is_rate_limited(request):
current = {
sub.list_id
for sub in UserMailingListSubscription.objects.filter(
user=request.user, list_id__in=managed_lists
)
}
to_subscribe = [lid for lid in list_ids if lid not in current]

_subscribe_pending(request, request.user, email, to_subscribe)

if _is_htmx(request):
return HttpResponse("")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Don’t close the modal on failed subscriptions.

This branch swallows both rate-limit failures and _subscribe_pending() errors, then still returns an empty HTMX response. Since HomepageView.get_v3_context_data() already persists user.data["ml_post_auth_seen"] = True, any transient mail/backend failure becomes a silent one-shot no-op for the user. Keep the modal open and surface an error state before dismissing it.

🧰 Tools
🪛 ast-grep (0.44.0)

[error] 706-706: Lack of sanitization of user data
Context: HttpResponse("")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').

(http-response-from-request)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mailing_list/views.py` around lines 695 - 707, The HTMX branch in the
subscription flow is returning an empty response even when
`_is_rate_limited(request)` blocks the action or `_subscribe_pending()` fails,
which causes the modal to close on a silent failure. Update the subscription
handler around the `list_ids`, `_is_rate_limited`, and `_subscribe_pending`
logic so it detects these failure cases, keeps the modal open, and returns an
error state/message instead of `HttpResponse("")` when the subscription did not
succeed. Ensure the `_is_htmx(request)` response path only dismisses the modal
after a successful subscribe.

Comment on lines +254 to +263
.ml-post-auth-modal__email-field:focus-visible {
background-color: var(--color-surface-mid);
border-color: var(--color-stroke-strong);
}

.ml-post-auth-modal__email-field--error {
background-color: var(--color-surface-error-weak) !important;
border-color: var(--color-stroke-error) !important;
color: var(--color-text-error);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Invalid email state hides the keyboard focus indicator.

outline: none removes the native ring, and the --error rule’s !important border/background override the :focus-visible styling. Once validation fails, the focused field no longer has a clear focus cue.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@static/css/v3/mailing-list-card.css` around lines 254 - 263, The email
field’s invalid state is overriding the keyboard focus cue, so fix the
interaction between `.ml-post-auth-modal__email-field:focus-visible` and
`.ml-post-auth-modal__email-field--error`. Keep a visible focus indicator when
the field is both focused and in error by adding a more specific focused-error
state or adjusting the error styles so they don’t suppress the focus ring.
Update the focus styling in this CSS module to ensure the focus-visible cue
remains clear even when validation errors are present.

{% endcomment %}
<div class="dialog-modal content-modal" id="{{ modal_id }}">
<a class="dialog-modal__backdrop" href="{{ close_url|default:'#_' }}" aria-hidden="true" tabindex="-1"></a>
<a class="dialog-modal__backdrop" href="{{ close_url|default:'#_' }}" tabindex="-1"></a>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Backdrop link is now exposed without an accessible name.

Removing aria-hidden makes this anchor visible to assistive tech, but unlike the explicit close control on Line 30 it still has no label. Screen readers can encounter it as an empty link.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/v3/includes/_content_modal.html` at line 21, The dialog backdrop
anchor is now exposed to assistive tech without an accessible name, so update
the backdrop link in the modal template to either remain hidden from screen
readers or provide an appropriate label. Adjust the dialog-modal__backdrop
element so it has a clear accessible name consistent with the close control, or
mark it non-interactive for assistive technologies if it is only decorative.


<div class="dialog-modal" id="{{ dialog_id }}">
<a class="dialog-modal__backdrop" href="#_" aria-hidden="true" tabindex="-1"></a>
<a class="dialog-modal__backdrop" href="#_" tabindex="-1"></a>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Backdrop link is now exposed without an accessible name.

After removing aria-hidden, this backdrop anchor is no longer purely decorative for assistive tech, but it still has no label. That leaves screen readers with an empty link ahead of the actual labeled close action on Line 39.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/v3/includes/_dialog.html` at line 28, The dialog backdrop anchor is
now exposed to assistive tech without any accessible name, so it should either
be hidden from screen readers or given a label. Update the dialog template
around dialog-modal__backdrop to make the backdrop non-interactive for
accessibility, or add an appropriate accessible name so it no longer appears as
an empty link before the labeled close action.

{% if modal_subscribe_url and mailing_lists %}
<div class="dialog-modal" id="mailing-list-modal">
<a class="dialog-modal__backdrop" href="#_" tabindex="-1" aria-hidden="true"></a>
<a class="dialog-modal__backdrop" href="#_" tabindex="-1"></a>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Backdrop link is now exposed without an accessible name.

Removing aria-hidden here has the same side effect as the other modal templates: assistive tech can now hit an unlabeled link before reaching the labeled close button on Line 113.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/v3/includes/_mailing_list_card.html` at line 102, The modal
backdrop link in the mailing list card template is now exposed to assistive tech
without an accessible name, so it should be hidden from the accessibility tree
or otherwise given a proper label. Update the dialog backdrop anchor in the
modal markup to match the accessible behavior used by the other modal templates,
and ensure the visible close control remains the first labeled focus target in
this component.

Comment thread templates/v3/includes/_mailing_list_post_auth_modal.html Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@templates/v3/includes/_mailing_list_post_auth_modal.html`:
- Line 20: The no-JS fallback for the mailing list post-auth modal is not
keyboard operable because the hidden toggle and label-based close controls
cannot be reliably activated without JavaScript. Update the fallback in the
mailing list modal markup so the close path uses a native keyboard-accessible
control, or make the existing toggle operable without JS; focus on the controls
around ml-post-auth-toggle, the backdrop close affordance, and the visible
Cancel action in the modal template.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c6df8e8-4254-4f21-bf6b-7862a72183c9

📥 Commits

Reviewing files that changed from the base of the PR and between 90b84b1 and bf23387.

📒 Files selected for processing (5)
  • mailing_list/views.py
  • static/css/v3/dialog.css
  • static/js/dialog.js
  • templates/v3/includes/_mailing_list_card.html
  • templates/v3/includes/_mailing_list_post_auth_modal.html

JS enhances this with HTMX submit, inline email validation, and ESC/focus trap
via dialog.js (which recognises the checkbox toggle).
{% endcomment %}
<input type="checkbox" id="ml-post-auth-toggle" class="dialog-modal__toggle" checked hidden>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

The documented no-JS close path is not keyboard-operable.

The controlling checkbox is hidden, the backdrop close affordance is unfocusable, and the visible “Cancel” control is a <label> rather than a natively activatable button. That means a keyboard user cannot reliably dismiss this auto-opened modal unless dialog.js is running, which breaks the fallback described on Lines 13-18. Please switch the fallback to a natively keyboard-operable close mechanism, or make the toggle itself operable in the no-JS path.

Also applies to: 43-43, 124-127

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@templates/v3/includes/_mailing_list_post_auth_modal.html` at line 20, The
no-JS fallback for the mailing list post-auth modal is not keyboard operable
because the hidden toggle and label-based close controls cannot be reliably
activated without JavaScript. Update the fallback in the mailing list modal
markup so the close path uses a native keyboard-accessible control, or make the
existing toggle operable without JS; focus on the controls around
ml-post-auth-toggle, the backdrop close affordance, and the visible Cancel
action in the modal template.

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.

Mailing List Subscription Post Auth

3 participants