This repository was archived by the owner on Jun 12, 2026. It is now read-only.
Fixing some small Honeybager alerts#54
Merged
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ericvanjohnson
had a problem deploying
to
Testing
GitHub Actions
Failure
WalkthroughAdds optional website support for sponsors: makes the sponsors.website column nullable, provides a factory state to create sponsors without websites, updates the sponsor modal to hide or show the website link based on presence, and adds/updates tests for sponsor rendering and model/pivot behaviors. Also introduces an unrelated Alpine countdown and hero wiring. ChangesSponsor website optionality, tests, and docs
UI: Countdown and hero wiring
Sequence Diagram(s)(Skipped — changes are small/straightforward control updates and an unrelated UI component; no diagram generated.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
johncongdon
approved these changes
Mar 31, 2026
svpernova09
approved these changes
Mar 31, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Feature/Http/Controllers/SponsorControllerTest.php (1)
29-33: Simplifyattach()call by removing redundant foreign keys.Laravel's
BelongsToMany::attach()automatically handles the foreign keys (sponsor_uuid,conference_uuid) based on the relationship definition. Only pivot-specific data likesponsorship_levelneeds to be passed.♻️ Proposed simplification
$this->conference->sponsors()->attach($sponsor->uuid, [ - 'sponsor_uuid' => $sponsor->uuid, - 'conference_uuid' => $this->conference->uuid, 'sponsorship_level' => 'gold', ]);Apply the same simplification to the other
attach()calls on lines 45-49, 64-68, and 70-74.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/Http/Controllers/SponsorControllerTest.php` around lines 29 - 33, The attach() calls in SponsorControllerTest.php pass redundant pivot foreign keys; update each occurrence of $this->conference->sponsors()->attach(...) (including the other three similar calls) to only pass the related model id/uuid and the pivot-specific data (remove 'sponsor_uuid' and 'conference_uuid'), e.g. call attach($sponsor->uuid, ['sponsorship_level' => 'gold']) so Laravel's BelongsToMany relationship supplies the foreign keys automatically.database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php (1)
7-17: Consider adding adown()method for rollback capability.The migration lacks a
down()method to reverse the schema change. While Laravel allows this, it prevents clean rollbacks during development or if issues arise in production.🔧 Proposed fix to add rollback support
public function up(): void { Schema::table('sponsors', function (Blueprint $table) { $table->string('website')->nullable()->change(); }); } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('sponsors', function (Blueprint $table) { + $table->string('website')->nullable(false)->change(); + }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php` around lines 7 - 17, Add a down() method to the anonymous Migration class to reverse the up() change: in down() call Schema::table('sponsors', function (Blueprint $table) { ... }) and change the 'website' column back to non-nullable (undo the ->nullable()->change() applied in up()). Reference the same migration class and methods (up(), down(), Schema::table, Blueprint) and ensure the change() call mirrors the up() but removes nullability so rollbacks restore the original state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Feature/Http/Controllers/SponsorControllerTest.php`:
- Around line 35-39: The test in SponsorControllerTest is hitting the model's
logoUrl accessor which calls Storage::disk('s3')->url() because the factory
produced a relative logo path; to fix, update the test to either use the
phpArchitect() factory state (so the factory returns a full HTTP logo URL), add
a new factory state that sets a non-S3 or absolute logo path, or mock the
storage by calling Storage::fake() before creating the Sponsor so logoUrl won’t
call the real S3 disk; adjust the test setup accordingly to use phpArchitect()
or Storage::fake() and then assertSee as before.
---
Nitpick comments:
In `@database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php`:
- Around line 7-17: Add a down() method to the anonymous Migration class to
reverse the up() change: in down() call Schema::table('sponsors', function
(Blueprint $table) { ... }) and change the 'website' column back to non-nullable
(undo the ->nullable()->change() applied in up()). Reference the same migration
class and methods (up(), down(), Schema::table, Blueprint) and ensure the
change() call mirrors the up() but removes nullability so rollbacks restore the
original state.
In `@tests/Feature/Http/Controllers/SponsorControllerTest.php`:
- Around line 29-33: The attach() calls in SponsorControllerTest.php pass
redundant pivot foreign keys; update each occurrence of
$this->conference->sponsors()->attach(...) (including the other three similar
calls) to only pass the related model id/uuid and the pivot-specific data
(remove 'sponsor_uuid' and 'conference_uuid'), e.g. call attach($sponsor->uuid,
['sponsorship_level' => 'gold']) so Laravel's BelongsToMany relationship
supplies the foreign keys automatically.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 549df2be-1602-4547-be40-c97bd61fba2f
📒 Files selected for processing (4)
database/factories/SponsorFactory.phpdatabase/migrations/2026_03_31_004613_make_sponsor_website_nullable.phpresources/views/components/partners-section.blade.phptests/Feature/Http/Controllers/SponsorControllerTest.php
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to address Honeybadger alerts by making sponsor websites optional end-to-end (DB/schema + factory/test data + UI rendering) so the partners modal doesn’t error when a sponsor has no website.
Changes:
- Updates the partners modal JS to hide the “Website” block when
sponsor.websiteis missing. - Makes
sponsors.websitenullable and adds aSponsorFactory::withoutWebsite()state. - Replaces the placeholder SponsorController test with feature tests covering sponsors with/without websites on the home page.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/Feature/Http/Controllers/SponsorControllerTest.php | Adds feature tests asserting the home page renders sponsors correctly when website is present/absent. |
| resources/views/components/partners-section.blade.php | Prevents JS errors by conditionally rendering/hiding the sponsor website link in the modal. |
| database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php | Alters sponsors.website to be nullable. |
| database/factories/SponsorFactory.php | Adds a factory state for sponsors with a null website. |
Comment on lines
+14
to
+16
| Schema::table('sponsors', function (Blueprint $table) { | ||
| $table->string('website')->nullable()->change(); | ||
| }); |
There was a problem hiding this comment.
This migration uses ->change() to alter an existing column, but doctrine/dbal is not installed (not present in composer.json). On common setups (including SQLite), this can cause migrations to fail at runtime. Consider adding the required dependency or rewriting the migration to avoid change() (e.g., a DB-specific alter statement or table rebuild strategy).
Suggested change
| Schema::table('sponsors', function (Blueprint $table) { | |
| $table->string('website')->nullable()->change(); | |
| }); | |
| $connection = Schema::getConnection(); | |
| $driver = $connection->getDriverName(); | |
| if ($driver === 'mysql') { | |
| $connection->statement('ALTER TABLE sponsors MODIFY website VARCHAR(255) NULL'); | |
| } elseif ($driver === 'pgsql') { | |
| $connection->statement('ALTER TABLE sponsors ALTER COLUMN website DROP NOT NULL'); | |
| } elseif ($driver === 'sqlsrv') { | |
| $connection->statement('ALTER TABLE sponsors ALTER COLUMN website NVARCHAR(255) NULL'); | |
| } |
fwallen
approved these changes
Mar 31, 2026
fwallen
left a comment
fwallen
left a comment
Member
There was a problem hiding this comment.
Only one question. Not pulling the handbrake.
…ivot keys in sponsor tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Feature/SponsorModelTest.php (1)
27-27: ⚡ Quick winStrengthen assertions to exact expected URLs
Using
toContain(...)here can mask malformedlogo_urloutputs. Prefer asserting full expected values (or stable URL patterns including disk/base URL) so regressions fail loudly.As per coding guidelines, "Tests should test all of the happy paths, failure paths, and weird paths."Proposed tightening of assertions
- expect($sponsor->logo_url)->toContain('favicon.ico'); + expect($sponsor->logo_url)->toBe(asset('favicon.ico')); - expect($url)->toContain('vendor_logos/some-logo.png'); + expect($url)->toBe(Storage::disk('s3')->url('vendor_logos/some-logo.png')); - expect($url)->toContain('vendor_logos/some-logo.png'); + expect($url)->toBe(Storage::disk('s3')->url('vendor_logos/some-logo.png'));Also applies to: 39-39, 51-51
🤖 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 `@tests/Feature/SponsorModelTest.php` at line 27, The test currently uses loose substring assertions like expect($sponsor->logo_url)->toContain('favicon.ico');; update each such assertion (the ones asserting $sponsor->logo_url in SponsorModelTest) to assert the full, exact expected URL (e.g. compare against the result of Storage::url($sponsor->logo_path) or config('app.url').'/path/to/favicon.ico' or asset(...) as appropriate) using an equality assertion (toEqual()/toBe()/assertEquals) so malformed or missing base/disk URLs fail loudly; ensure all occurrences (the three assertions referencing $sponsor->logo_url) are replaced consistently.database/factories/SponsorFactory.php (1)
39-54: 💤 Low valueConsider dropping the unused
$attributesparameter from state closures.Both
withoutWebsiteandwithTestLogopass$attributesto their closures but never reference it, triggering the PHPMDUnusedFormalParameterwarning. Since neither state depends on existing attributes, the parameter can be omitted entirely.♻️ Proposed fix
- return $this->state(fn (array $attributes) => [ + return $this->state(fn () => [ 'website' => null, ]);- return $this->state(fn (array $attributes) => [ + return $this->state(fn () => [ 'logo' => 'https://example.com/logos/test-logo.png', ]);🤖 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 `@database/factories/SponsorFactory.php` around lines 39 - 54, Remove the unused $attributes parameter from the state closures in SponsorFactory: update the closures used by withoutWebsite() and withTestLogo() so they don't declare the unused parameter (i.e., use parameterless closures for the state callbacks), keeping the same returned arrays for 'website' => null and 'logo' => 'https://example.com/logos/test-logo.png' respectively to silence PHPMD UnusedFormalParameter warnings.
🤖 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 `@resources/views/components/hero-section.blade.php`:
- Around line 26-28: The element using role="timer" currently includes
aria-live="polite", which overrides the timer's implicit aria-live="off" and
causes per-second announcements; remove the aria-live attribute (or set it
explicitly to aria-live="off") on the element with role="timer" and keep
aria-label="Countdown to conference start" so the timer semantics are preserved
without noisy screen reader updates.
---
Nitpick comments:
In `@database/factories/SponsorFactory.php`:
- Around line 39-54: Remove the unused $attributes parameter from the state
closures in SponsorFactory: update the closures used by withoutWebsite() and
withTestLogo() so they don't declare the unused parameter (i.e., use
parameterless closures for the state callbacks), keeping the same returned
arrays for 'website' => null and 'logo' =>
'https://example.com/logos/test-logo.png' respectively to silence PHPMD
UnusedFormalParameter warnings.
In `@tests/Feature/SponsorModelTest.php`:
- Line 27: The test currently uses loose substring assertions like
expect($sponsor->logo_url)->toContain('favicon.ico');; update each such
assertion (the ones asserting $sponsor->logo_url in SponsorModelTest) to assert
the full, exact expected URL (e.g. compare against the result of
Storage::url($sponsor->logo_path) or config('app.url').'/path/to/favicon.ico' or
asset(...) as appropriate) using an equality assertion
(toEqual()/toBe()/assertEquals) so malformed or missing base/disk URLs fail
loudly; ensure all occurrences (the three assertions referencing
$sponsor->logo_url) are replaced consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5cf3ce9-7410-4207-86e2-8de8e2ef93fb
📒 Files selected for processing (8)
database/factories/SponsorFactory.phpdocs/superpowers/plans/2026-04-01-pr54-review-fixes.mdresources/js/app.jsresources/views/components/hero-section.blade.phpresources/views/welcome.blade.phptests/Feature/Http/Controllers/SponsorControllerTest.phptests/Feature/RelationshipTest.phptests/Feature/SponsorModelTest.php
✅ Files skipped from review due to trivial changes (3)
- resources/js/app.js
- tests/Feature/RelationshipTest.php
- docs/superpowers/plans/2026-04-01-pr54-review-fixes.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Feature/Http/Controllers/SponsorControllerTest.php
johncongdon
approved these changes
May 7, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 by CodeRabbit
New Features
Tests
Documentation