Complete Notifications correctness and Slack parity - #472
Merged
Conversation
Expose attachFromStorage and attachFromStorageDisk directly on MailMessage while preserving Laravel method order, named arguments, display names, MIME options, and default-disk behavior. Cover default and named disks, basename fallback, custom names, MIME metadata, and real attachment contents through an isolated framework filesystem fixture.
Move automatic action ID generation into a shared trait, preserve ordinary IDs, replace unusable empty slugs, and cap the final generated value at Slack’s 255-byte protocol limit. Add counterfactual coverage for expanding slugs, non-transliterable labels, explicit overlong IDs, and unchanged valid button identifiers.
Port static and users select accessories with placeholders, focus state, initial choices, explicit IDs, and concrete fluent return types. Wire both select types into ActionsBlock using deterministic text-derived identifiers. Normalize supported scalar option values once, reject values that collapse to an unusable empty Slack identifier, and cover serialization, chaining, defaults, and unknown-option failures.
Use mb_strcut only after Slack text exceeds its existing byte limit so truncation cannot split a multibyte character while unchanged ASCII and in-limit text keep their current behavior. Declare the required extension at the split-package boundary, remove unused Filesystem and Object Pool dependencies, and enforce the resulting dependency and provider metadata against the monorepo manifest.
Expose the generated Slack Block Kit Builder URL without terminating execution and make dd delegate to that public representation while retaining raw-payload dumping. Move SlackMessage coverage beside its source namespace, extract only the genuinely shared channel fixtures and test setup, and restore direct ImageElement coverage so future upstream changes retain a clear source-to-test mapping.
Allow incoming webhooks to serialize both modern Block Kit messages and retained legacy attachment messages without converting either representation or duplicating the delivery channel. Cover string and PSR URI webhooks, Web API routes, route short-circuiting, and Horizon’s intentional legacy-webhook versus modern-Web-API payload selection.
Save and restore the exact coroutine-local failure marker around each channel attempt so nested sends cannot suppress or duplicate an outer NotificationFailed event. The process-global dispatcher listener now marks only an active sender-owned attempt. Exercise the real provider listener across nested success and failure, sequential attempts, cleanup, external events, and sibling coroutines. Remove the redundant ChannelManager singleton registration while proving both public aliases resolve the same auto-singleton.
Interleave sibling coroutines after assigning different delivery channels and locales, then verify each observes only its own state and a fresh context returns to the worker defaults. This pins the existing coroutine-local manager contract without adding production state, locks, cleanup hooks, or request-wide work.
Use strict queue-interface membership when annotating notification mail, type queued-notification cloning explicitly, and retain the established object ownership during clone. Port current queue-precedence regressions proving runtime onQueue selection and constructor values override queue attributes while attributes remain the fallback when no explicit queue is chosen.
Give notification relationships precise model generics and make read and unread scopes return the supplied Eloquent builder explicitly across the query-builder forwarding boundary. Cover real read/unread selection through builder dispatch, frozen initial read state, all relationship types, and existing UUID-backed notification behavior without changing the public predicate or scope APIs.
Return null explicitly from the mixed getKey contract so anonymous notification fakes, broadcast naming, and other identity consumers cannot hit an implicit-null TypeError. Cover the public identity result and keep anonymous route accumulation class-owned across mail, multiple-route, and fake integration paths.
Move the remaining notification unit coverage onto Hypervel’s coroutine-aware base test case and complete native test and fixture method typing. Preserve the existing behavioral assertions across mail integration, broadcast messages, actions, simple messages, and Slack blocks while removing raw PHPUnit lifecycle gaps.
Own both missing-translation probe globals in class setup and teardown so reverse ordering and prior failures cannot leak process-global state into later integration tests. This is a test-lifecycle correction only; Translation source behavior and its later package audit remain unchanged.
Document notification mail attachments from default and named filesystem disks, Slack static and users selects, and non-terminating Block Kit Builder URL retrieval in the primary guide. Keep the package README minimal while linking the user documentation and both upstream projects that Notifications intentionally tracks.
Record the need for first-class PHPStan extensions that preserve Eloquent builder model types through forwarded query methods and expose named scopes on builders and relations. Use Larastan as prior art and avoid runtime wrappers, duplicate method inventories, or wider source types whose only purpose would be satisfying static analysis.
Record the verified upstream baselines, accepted findings, rejected speculative concerns, architecture boundaries, complete implementation shape, and regression strategy for the Notifications audit. Carry the core anti-overengineering and performance-tradeoff rules verbatim so the work remains correctly constrained after context restoration without duplicating decision history.
Mark Notifications complete, normalize the shared finding records, and record the final correctness, parity, coroutine-lifecycle, performance, regression, and API assessment. Close the Contracts, Queue, Eloquent, Support, Macroable, and Horizon revalidation edges; attribute the Translation fixture correction; restore the routing index to None; and keep the package checklist synchronized with the audited package set.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThe Notifications package adds storage attachments, Slack Block Kit selectors, UTF-8 validation, webhook compatibility, Block Kit Builder URLs, coroutine-local failure handling, stronger typing, expanded tests, and audit documentation. ChangesNotifications correctness and audit completion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NotificationSender
participant CoroutineContext
participant NotificationFailed
participant SlackWebhookChannel
participant SlackMessage
NotificationSender->>CoroutineContext: initialize attempt failure state
NotificationSender->>SlackWebhookChannel: send notification
SlackWebhookChannel->>SlackMessage: serialize modern or legacy payload
SlackWebhookChannel->>NotificationFailed: dispatch failure event on error
NotificationFailed->>CoroutineContext: mark active attempt
NotificationSender->>CoroutineContext: restore prior state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Greptile SummaryThis PR completes the Notifications audit with current mail-storage attachment APIs, Slack Block Kit and webhook support, coroutine-local failure ownership, and corrected queue and database notification behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported static-select identity mismatch is fixed because registration, serialization, and initial-option lookup now use the same exact value.
|
| Filename | Overview |
|---|---|
| src/notifications/src/Slack/BlockKit/Elements/Selects/StaticSelectElement.php | Uses exact option values consistently for registration and initial selection while enforcing static-select cardinality. |
| src/notifications/src/Slack/BlockKit/Elements/Selects/SelectOption.php | Preserves option values verbatim and validates nonempty, character-bounded interaction identities. |
| src/notifications/src/NotificationSender.php | Adds save-and-restore semantics for coroutine-local nested channel failure ownership. |
| src/notifications/src/Channels/SlackWebhookChannel.php | Extends webhook delivery to modern Block Kit payloads while retaining legacy attachments. |
| src/notifications/src/Messages/MailMessage.php | Adds storage-backed attachment APIs with disk, display-name, and MIME support. |
| src/notifications/src/DatabaseNotification.php | Corrects database notification read-state, relationship, and query-scope contracts. |
Reviews (2): Last reviewed commit: "Harden Slack Block Kit protocol boundari..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/notifications/src/Slack/BlockKit/Elements/Selects/SelectElement.php`:
- Around line 58-60: Update SelectElement::placeholder() to construct
PlainTextOnlyTextObject with the Slack-specific 150-character limit instead of
the generic default, and add a boundary test confirming 150 characters are
accepted while longer placeholders are rejected.
In `@src/notifications/src/Slack/BlockKit/Elements/Selects/SelectOption.php`:
- Around line 45-54: The value method must reject normalized option values
exceeding Slack’s 150-character limit. Add a length check for $normalizedValue
before assigning it to $this->value, and add a regression test covering a
151-character normalized value.
In
`@src/notifications/src/Slack/BlockKit/Elements/Selects/StaticSelectElement.php`:
- Around line 37-73: Validate the option count in StaticSelectElement: update
addOption() to throw when adding a 101st unique value while allowing replacement
of an existing value, and update toArray() to reject serialization when no
options exist instead of emitting an empty options array. Adjust the
static-select tests covering empty-option serialization to expect the new
validation behavior.
In `@src/notifications/src/Slack/SlackMessage.php`:
- Around line 330-332: Update toBlockKitBuilderUrl() to encode the filtered
message data with the JSON option that throws JsonException on failures,
ensuring rawurlencode() receives a valid string. Add coverage for invalid UTF-8
in a Block Kit text value and assert that JsonException is raised.
In `@tests/Notifications/Slack/Blocks/ActionsBlockTest.php`:
- Around line 121-138: Update StaticSelectElement::toArray() to reject empty
option lists and accept only one through 100 options, enforcing Slack’s required
bounds. Modify the ActionsBlock test to add at least one option to the static
select before asserting the serialized payload, including that option in the
expected array.
🪄 Autofix
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: f0f451ce-ab2c-4ded-aa3e-ebba66c85a01
📒 Files selected for processing (67)
docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-08-04-2030-notifications-correctness-slack-parity-and-reentrant-failure-ownership.mddocs/todo.mdsrc/boost/docs/grpc.mdsrc/boost/docs/notifications.mdsrc/grpc/src/Metadata.phpsrc/notifications/README.mdsrc/notifications/composer.jsonsrc/notifications/src/AnonymousNotifiable.phpsrc/notifications/src/Channels/MailChannel.phpsrc/notifications/src/Channels/SlackWebhookChannel.phpsrc/notifications/src/DatabaseNotification.phpsrc/notifications/src/HasDatabaseNotifications.phpsrc/notifications/src/Messages/MailMessage.phpsrc/notifications/src/NotificationSender.phpsrc/notifications/src/NotificationServiceProvider.phpsrc/notifications/src/SendQueuedNotifications.phpsrc/notifications/src/Slack/BlockKit/Blocks/ActionsBlock.phpsrc/notifications/src/Slack/BlockKit/Composites/PlainTextOnlyTextObject.phpsrc/notifications/src/Slack/BlockKit/Elements/ButtonElement.phpsrc/notifications/src/Slack/BlockKit/Elements/Selects/SelectElement.phpsrc/notifications/src/Slack/BlockKit/Elements/Selects/SelectOption.phpsrc/notifications/src/Slack/BlockKit/Elements/Selects/StaticSelectElement.phpsrc/notifications/src/Slack/BlockKit/Elements/Selects/UsersSelectElement.phpsrc/notifications/src/Slack/BlockKit/Elements/Traits/GeneratesDefaultIds.phpsrc/notifications/src/Slack/Contracts/AccessoryContract.phpsrc/notifications/src/Slack/SlackMessage.phptests/Horizon/Notifications/LongWaitDetectedTest.phptests/Integration/Notifications/DatabaseNotificationTest.phptests/Integration/Notifications/NotificationFailedEventTest.phptests/Integration/Notifications/SendingMailNotificationsTest.phptests/Integration/Notifications/SendingMailableNotificationsTest.phptests/Integration/Notifications/SendingNotificationsViaAnonymousNotifiableTest.phptests/Integration/Translation/TranslatorTest.phptests/Notifications/CoroutineIsolationTest.phptests/Notifications/NotificationActionTest.phptests/Notifications/NotificationBroadcastChannelTest.phptests/Notifications/NotificationChannelManagerTest.phptests/Notifications/NotificationDatabaseChannelTest.phptests/Notifications/NotificationMailMessageTest.phptests/Notifications/NotificationMessageTest.phptests/Notifications/NotificationRoutesNotificationsTest.phptests/Notifications/NotificationSenderTest.phptests/Notifications/NotificationSlackChannelTest.phptests/Notifications/PackageMetadataTest.phptests/Notifications/Slack/Blocks/ActionsBlockTest.phptests/Notifications/Slack/Blocks/ContextBlockTest.phptests/Notifications/Slack/Blocks/DividerBlockTest.phptests/Notifications/Slack/Blocks/HeaderBlockTest.phptests/Notifications/Slack/Blocks/ImageBlockTest.phptests/Notifications/Slack/Blocks/SectionBlockTest.phptests/Notifications/Slack/Composites/ConfirmObjectTest.phptests/Notifications/Slack/Composites/PlainTextOnlyTextObjectTest.phptests/Notifications/Slack/Composites/TextObjectTest.phptests/Notifications/Slack/Elements/ButtonElementTest.phptests/Notifications/Slack/Elements/ImageElementTest.phptests/Notifications/Slack/Elements/Selects/SelectOptionTest.phptests/Notifications/Slack/Elements/Selects/StaticSelectElementTest.phptests/Notifications/Slack/Elements/Selects/UsersSelectElementTest.phptests/Notifications/Slack/Fixtures/SlackChannelTestNotifiable.phptests/Notifications/Slack/Fixtures/SlackChannelTestNotification.phptests/Notifications/Slack/SlackMessageTest.phptests/Notifications/Slack/SlackWebApiChannelTest.phptests/Notifications/Slack/TestCase.phptests/Notifications/SlackNotificationRouterChannelTest.phptypes/Database/Eloquent/Model.php
Preserve static-select option values as exact interaction identities and enforce Slack's documented option, placeholder, cardinality, image, action, and block limits using character counts. Make text truncation UTF-8 safe, surface Block Kit Builder JSON failures as JsonException, and route image-block constructor alt text through the existing invariant owner. Add counterfactual coverage for exact identities, select bounds, multibyte limits, malformed text, image construction, and JSON encoding. Update the public Laravel differences, guide example, implementation plan, and audit ledger to match the final behavior.
Collaborator
Author
|
@coderabbitai @greptile-apps Thanks for the review. I addressed the concrete findings in
I did not add blanket docstrings for the coverage warning. This repository uses targeted Laravel-style docblocks where they add contract or behavioral information, not percentage-driven comments. The complete |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-ext-diff --format=fuller --stat cdb61df9f
printf '\n--- Changed files ---\n'
git diff-tree --no-commit-id --name-only -r cdb61df9f
printf '\n--- Relevant implementation and test diff ---\n'
git show --no-ext-diff --format= --find-renames --find-copies cdb61df9f -- \
src/notifications/src/Slack \
tests/Notifications/Slack \
docs/plans \
| sed -n '1,1600p'Length of output: 64007
The commit addresses the concrete Slack Block Kit findings:
The targeted-docblock approach is appropriate. No blanket coverage-driven documentation is needed. 🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
The fixes in
The implementation is clean. No further changes needed from my side. |
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This completes the Notifications audit and brings the package up to date with the supported Laravel framework and Slack notification-channel APIs. It adds notification mail attachments from storage, current Slack Block Kit selects and Builder URL support, modern webhook payloads, and the missing queue and database notification coverage.
It also fixes several correctness issues: nested channel failures now preserve exact event ownership, generated Slack action IDs remain valid, truncated Slack text remains valid UTF-8, anonymous notifiables have an explicit null identity, and database notification relationships and scopes carry accurate types.
For more details, see:
docs/plans/2026-08-04-2030-notifications-correctness-slack-parity-and-reentrant-failure-ownership.md.What changed
MailMessage::attachFromStorage()andattachFromStorageDisk()with default disk, named disk, display name, and MIME support.SlackMessage::toBlockKitBuilderUrl()and madedd()delegate to it.NotificationFailedownership safe for nested and concurrent channel attempts using coroutine-local save and restore semantics.ChannelManagersingleton binding while retaining identical Dispatcher and Factory alias resolution.Correctness and compatibility
No supported Laravel Notifications API was removed. The public changes are additive except where existing generated Slack values or nested failure handling were demonstrably incorrect.
The failure marker exists only during a notification channel attempt and restores the exact prior coroutine state. External failure events do not create sender state, nested sends cannot suppress their parent, and sibling coroutines cannot observe one another's delivery configuration.
Slack delivery remains direct and unpooled. Legacy attachment messages remain supported alongside modern Block Kit messages.
Performance
Successful channel delivery adds only constant-time coroutine-context reads and writes around the existing transport call. It adds no lock, I/O, retry, reflection, registry, retained allocation, or request-wide work. UTF-8 truncation runs only after the existing over-limit check.
Validation
composer fixgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation