chore: sync conventions, tooling, and tests#402
Open
gvieira18 wants to merge 21 commits into
Open
Conversation
Apply the project Rector named-argument rule across models, commands, jobs and Livewire components.
Scaffold the module ServiceProvider at src/ root instead of a Providers/ subdir, and migrate squads and onboarding to match.
Add a shard-aware Pest plugin with a committed timings baseline, exclude it from Rector/PHPStan, and split CI tests into 2 shards.
Ship a tracked .env.testing.example, untrack the per-dev .env.testing, and bootstrap it on setup.
Defer each feature test's wrapping transaction to the first query instead of opening it eagerly in setUp. Full suite green: 881 tests.
|
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:
📝 WalkthroughWalkthroughGuidelines, testing infrastructure, and CI were expanded for module dependency rules, typed JSON casts, enum Filament contracts, issue tracking, and branch naming. Test execution now uses a Pest shard plugin, sharded GitHub Actions, updated Composer scripts, and refreshed shard timing data. Onboarding and squads service providers moved out of Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/Support/PestShardPlugin.php (1)
533-557: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked
file_put_contentsinwriteTimings()silently loses timing data.If the write fails (disk full, permissions), the canonical
shards.jsonis not updated and subsequent sharded runs use stale timings with no error signal. Same issue inwriteWorkerTimings()at line 394.♻️ Proposed fix
- file_put_contents($path, json_encode([ + $result = file_put_contents($path, json_encode([ 'timings' => $timings, 'checksum' => md5(implode("\n", $canonical)), 'updated_at' => date('c'), ], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)."\n"); + + if ($result === false) { + throw new InvalidOption('Failed to write [tests/.pest/shards.json]. Check directory permissions.'); + }🤖 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 `@app/Support/PestShardPlugin.php` around lines 533 - 557, `writeTimings()` is ignoring the result of `file_put_contents`, so timing updates can fail silently and leave stale shard data; update `writeTimings()` to check the write result and surface a clear failure when the file cannot be written. Apply the same safeguard in `writeWorkerTimings()` as noted in the comment, using the existing shard-writing flow around `shardsPath()` and the JSON payload generation.
🤖 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 @.ai/guidelines/domain/06-typed-json-casts.blade.php:
- Around line 36-38: The example has inconsistent cast names between the prose
and snippet, which can mislead readers copying the wrong class. Update the
guidance around AsIdentityMetadata so the descriptive text and the example use
the same class name throughout, and keep the reference aligned with the typed VO
cast shown in the snippet.
In @.ai/guidelines/domain/07-enum-filament-contracts.blade.php:
- Around line 64-71: The ordered example in getColor() does not match the
documented light-to-red ramp because Analyst is mapped to blue. Update the
getColor() match in the enum contract example so the ordered cases follow the
same ramp (or, if that cannot be expressed, change the example to an unordered
enum), keeping the mapping consistent with the comment and the enum’s intent.
In @.ai/guidelines/workflow/01-issue-tracker.blade.php:
- Around line 13-22: The example issue creation command is missing the required
difficulty label even though the workflow text says
triage/type/module/difficulty labels are mandatory. Update the sample under the
issue creation snippet to include a difficulty label alongside the existing type
and module labels, using the same gh issue create example so it matches the
guidance in workflow/triage-labels.
In @.env.testing.example:
- Line 3: The example environment file currently ships a real APP_KEY value,
which should not be committed. Replace the concrete key in the
.env.testing.example entry with a non-secret placeholder so contributors
generate their own value, and keep the change limited to the APP_KEY constant in
the example config.
In `@app/Support/PestShardPlugin.php`:
- Around line 207-209: The PestShardPlugin logic in the empty-tests path is
falling back to the original arguments, which causes a shard with no assigned
tests to run the full suite. Update the shard argument building flow in
PestShardPlugin so that when testsToRun is empty it returns a configuration that
executes zero tests for that shard instead of omitting --filter, and verify the
behavior in the code path around the test selection and arguments assembly.
In `@Makefile`:
- Line 68: The setup-test-db target currently runs only the migrate step, which
assumes test_heartdevs already exists and causes first-time setup to fail.
Update the Makefile’s setup-test-db recipe to create the testing database before
calling php artisan migrate --env=testing, keeping the existing migration
command in place and ensuring the full setup path works from a clean
environment.
---
Nitpick comments:
In `@app/Support/PestShardPlugin.php`:
- Around line 533-557: `writeTimings()` is ignoring the result of
`file_put_contents`, so timing updates can fail silently and leave stale shard
data; update `writeTimings()` to check the write result and surface a clear
failure when the file cannot be written. Apply the same safeguard in
`writeWorkerTimings()` as noted in the comment, using the existing shard-writing
flow around `shardsPath()` and the JSON payload generation.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5f06e8dc-e0c3-448d-9619-c71584757bfb
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (95)
.ai/guidelines/domain/02-module-architecture.blade.php.ai/guidelines/domain/06-typed-json-casts.blade.php.ai/guidelines/domain/07-enum-filament-contracts.blade.php.ai/guidelines/workflow/01-issue-tracker.blade.php.ai/guidelines/workflow/04-branch-naming.blade.php.env.testing.env.testing.example.github/workflows/_pest.yml.github/workflows/continuous-integration.yml.gitignoreMakefileapp-modules/activity/database/factories/.gitkeepapp-modules/activity/database/migrations/.gitkeepapp-modules/activity/src/Moderation/Models/ModerationEvent.phpapp-modules/activity/src/Voice/Models/Voice.phpapp-modules/community/database/factories/.gitkeepapp-modules/community/database/migrations/.gitkeepapp-modules/docs/src/Console/Commands/CacheDocsCommand.phpapp-modules/economy/database/factories/.gitkeepapp-modules/economy/database/migrations/.gitkeepapp-modules/gamification/database/factories/.gitkeepapp-modules/gamification/database/migrations/.gitkeepapp-modules/gamification/database/seeders/.gitkeepapp-modules/identity/database/factories/.gitkeepapp-modules/identity/database/migrations/.gitkeepapp-modules/identity/src/Tenant/Models/TenantUser.phpapp-modules/identity/src/User/Models/User.phpapp-modules/integration-devto/src/Polling/SyncDevToArticles.phpapp-modules/integration-discord/database/factories/.gitkeepapp-modules/integration-discord/database/migrations/.gitkeepapp-modules/integration-discord/src/ETL/Console/BackfillVoiceLogsCommand.phpapp-modules/integration-discord/src/ETL/Console/ImportDiscordMessagesCommand.phpapp-modules/integration-discord/src/ETL/Console/ImportDiscordProfilesCommand.phpapp-modules/integration-discord/src/ETL/Console/MergeDuplicateDiscordProfilesCommand.phpapp-modules/integration-discord/src/Sync/Console/PurgeUnusedInvitesCommand.phpapp-modules/integration-discord/src/Sync/Console/SyncDiscordGuildCommand.phpapp-modules/integration-github/database/factories/.gitkeepapp-modules/integration-github/database/migrations/.gitkeepapp-modules/integration-github/src/Backfill/Jobs/BackfillGithubRepository.phpapp-modules/integration-github/src/Console/BackfillGithubCommand.phpapp-modules/integration-github/tests/Feature/.gitkeepapp-modules/integration-twitch/database/migrations/.gitkeepapp-modules/integration-twitch/src/Console/LinkTwitchChannelCommand.phpapp-modules/integration-twitch/src/Console/SubscribeTwitchEventsCommand.phpapp-modules/integration-whatsapp/src/Models/WhatsAppEventLog.phpapp-modules/moderation/src/Appeals/ModerationAppeal.phpapp-modules/moderation/src/Audit/ModerationAuditLog.phpapp-modules/moderation/src/Cases/Models/ModerationCase.phpapp-modules/moderation/src/Cases/Models/ModerationReport.phpapp-modules/moderation/src/Classification/Jobs/ClassifyAndRoute.phpapp-modules/moderation/src/Classification/Jobs/ScreenContent.phpapp-modules/moderation/src/Enforcement/ModerationAction.phpapp-modules/moderation/src/Rules/ModerationRule.phpapp-modules/moderation/tests/Feature/.gitkeepapp-modules/moderation/tests/Unit/.gitkeepapp-modules/onboarding/composer.jsonapp-modules/onboarding/src/OnboardingServiceProvider.phpapp-modules/panel-admin/resources/views/.gitkeepapp-modules/panel-admin/tests/Feature/.gitkeepapp-modules/panel-app/src/Livewire/Timeline/Feed.phpapp-modules/panel-app/src/Livewire/Timeline/PostShow.phpapp-modules/panel-app/src/Livewire/Timeline/ThreadReplies.phpapp-modules/panel-app/src/Pages/ProfilePage.phpapp-modules/portal/src/Livewire/CommunityRetrospectivePage.phpapp-modules/portal/src/Livewire/Homepage.phpapp-modules/portal/src/Livewire/SocialLinksPage.phpapp-modules/portal/src/Retrospective/CommunityRetrospective.phpapp-modules/profile/src/Models/Profile.phpapp-modules/profile/src/Models/WorkExperience.phpapp-modules/squads/composer.jsonapp-modules/squads/src/SquadsServiceProvider.phpapp/Console/Commands/AnalyzeDiscordProfiles.phpapp/Console/Commands/CommunityReport.phpapp/Console/Commands/FetchDiscordMembers.phpapp/Console/Commands/FetchDiscordProfile.phpapp/Console/Commands/FetchDiscordProfiles.phpapp/Console/Commands/FixPostSwitchTimestampsCommand.phpapp/Console/Commands/GenerateDiscordTenant.phpapp/Console/Commands/ImportDiscordMembers.phpapp/Support/PestShardPlugin.phpcomposer.jsonconfig/app-modules.phpdatabase/factories/.gitkeepdocs/agents/domain.mddocs/agents/issue-tracker.mddocs/agents/triage-labels.mdpackage.jsonphpstan.neonphpunit.xmlrector.phpstubs/app-modules/.gitkeepstubs/app-modules/ServiceProvider.phpstubs/app-modules/composer-stub.jsontests/.pest/shards.jsontests/Feature/.gitkeep
💤 Files with no reviewable changes (4)
- docs/agents/issue-tracker.md
- docs/agents/domain.md
- phpunit.xml
- docs/agents/triage-labels.md
It's a per-developer, gitignored file; the tracked template lives in .env.testing.example. It was committed by mistake earlier on this branch.
- Empty APP_KEY in the tracked .env.testing.example (CodeRabbit); generate it per env via a shared composer key:generate script (app + testing), run before migrate in setup. - Make/Task/CI delegate to composer scripts (single source of truth). - Sync post-autoload-dump with the standard hooks: filament:upgrade, modules:sync, pre-package-uninstall.
Add the standard hooks both sibling projects run: filament:upgrade, modules:sync (--no-phpunit), and pre-package-uninstall.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Makefile (1)
68-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
setup-test-dbstill lacks database creation step.
php artisan migrate --env=testingrequirestest_heartdevsto already exist. First-timemake setup-test-dbfails on a clean environment. This was previously flagged and remains unresolved.🤖 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 `@Makefile` at line 68, The setup-test-db target still runs migrate before ensuring the testing database exists, so clean environments fail on first run. Update the Makefile’s setup-test-db flow to create the test_heartdevs database before the php artisan migrate --env=testing --no-interaction --force step, keeping the database creation logic in the same target or a helper it calls.
🤖 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.
Duplicate comments:
In `@Makefile`:
- Line 68: The setup-test-db target still runs migrate before ensuring the
testing database exists, so clean environments fail on first run. Update the
Makefile’s setup-test-db flow to create the test_heartdevs database before the
php artisan migrate --env=testing --no-interaction --force step, keeping the
database creation logic in the same target or a helper it calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 755f9534-4871-40eb-981b-7518bf167853
📒 Files selected for processing (5)
.env.testing.example.github/workflows/_pest.ymlMakefileTaskfile.ymlcomposer.json
Clintonrocha98
previously approved these changes
Jul 9, 2026
The gate used if: !cancelled() with two steps that only tested for 'failure'/'cancelled' in needs.*.result. When Setup PHP is cancelled, downstream checks become 'skipped' — neither string matched, so the gate reported all-green without a single check running (PR #402 run 28997719361, attempt 1). Switch to always() so the gate always decides, and whitelist success: any failure/cancelled/skipped result now fails the gate. Skip only on draft PRs, matching the checks' own draft guard.
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
2 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.
Note
Why
tests/.pest/shards.jsonis committed — it's intentional. Pest v4.6.0 balances the suite across shards using recorded per-test timings. Committing this baseline (instead of regenerating it every run) makes the split deterministic and evenly balanced from the first CI run — no warm-up pass and no cross-run artifact upload to wire up. It's refreshed automatically as the suite grows. See Time-based sharding in Pest v4.6.0.Summary
he4rt/heartdevs.com, added typed-JSON-cast + enum-contract guidance); drop 3 staledocs/agents/*mirrors.make:modulestubs +config/app-modules.phpso a module's ServiceProvider scaffolds atsrc/root (notProviders/); migratesquadsandonboardingto match..env.testing.example, untrack the per-dev.env.testing, bootstrap it viapost-root-package-install; useLazilyRefreshDatabasefor feature tests.feature/**,bugfix/**,chore/**,story/**,*.x).he4rt/*modules to^1.0.0; bump dependencies..gitkeepplaceholders.Test plan
composer check— Rector, Pint, PHPStan all cleanphp artisan test— 881 pass (539 feature + 342 unit, 2515 assertions)make setupcreates.env.testing;make setup-test-dbmigrates itsrc/root