Improve error message when manifest is empty #872

justin808 · 2026-02-04T00:32:21Z

Summary

Detect when the manifest is empty (bundler still building) and show a clearer, more actionable error message
Previously users saw a confusing list of 7 possible causes when the real issue was simply that the bundler hadn't finished compiling
Now users see a focused message explaining the bundler is still compiling with actionable next steps

Test plan

Run bundle exec rspec spec/shakapacker/manifest_spec.rb - all 26 tests pass
Run bundle exec rubocop on changed files - no offenses

Closes #871

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved error messaging when the asset manifest is missing or empty—users now get distinct, clearer guidance (including next steps and the manifest path) indicating whether the bundler is still compiling or the manifest hasn’t been built yet.
Tests
- Added tests covering both empty-manifest and missing-manifest scenarios to ensure consistent error behavior.

When the dev server is starting and hasn't finished initial compilation, the manifest file is empty. Previously, users would see a confusing error listing 7 possible causes. Now, when the manifest is empty, users see a clear message that the bundler is still compiling with actionable next steps. Closes #871 Co-Authored-By: Claude Opus 4.5 <[email protected]>

coderabbitai · 2026-02-04T00:32:58Z

Walkthrough

Detects when the manifest is empty and raises a specific MissingEntryError describing that the bundler is still building (or the manifest is not yet built), instead of the previous generic missing-entry message.

Changes

Cohort / File(s)	Summary
Manifest Error Handling `lib/shakapacker/manifest.rb`	Added check for empty manifest in `handle_missing_entry`, introduced private `manifest_empty_error(manifest_exists)` to return distinct messages for an empty existing manifest vs. a missing manifest, and raise `MissingEntryError` accordingly.
Manifest Tests `spec/shakapacker/manifest_spec.rb`	Added tests covering both cases: manifest file exists but is empty, and manifest file does not exist; assert `lookup!` and `lookup_pack_with_chunks!` raise `MissingEntryError` with the new, context-aware messages.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 The manifest sits empty, the bundler must hum,
Wait a beat, dear dev, don't worry or run.
A kinder error whispers: "I'm building, hold tight,"
Then dawns the compiled bundles — the page loads just right. 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: improving error messages when the manifest is empty, which directly aligns with the primary objective of the PR.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue `#871`: detecting empty manifests, providing clear error messages with bundler names, and displaying the manifest path with actionable guidance.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the requirements: adding manifest-empty detection logic, new error helper methods, and comprehensive test coverage for empty and missing manifest scenarios.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch jg/fix-871

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 and usage tips.}

claude · 2026-02-04T00:33:53Z

Pull Request Review

Summary

This PR improves the user experience by detecting when the manifest file is empty and providing a clearer, more actionable error message. The implementation is clean, focused, and directly addresses issue #871.

✅ Strengths

Code Quality

Well-structured solution: The early return pattern in handle_missing_entry is clean and maintainable
Follows project conventions: Adheres to existing error message patterns using heredoc syntax
Good separation of concerns: New manifest_empty_error method mirrors the existing missing_file_from_manifest_error pattern
Minimal changes: Only touches the necessary code without unnecessary refactoring

Error Message Quality

Clear and actionable: The new error message immediately tells users what's happening and what to do
Context-aware: Uses config.assets_bundler to reference the correct bundler (webpack/rspack)
Helpful guidance: Provides specific troubleshooting steps in order of likelihood
Includes manifest path: Shows the exact file location for debugging

Test Coverage

Tests both code paths: Covers both lookup! and lookup_pack_with_chunks! methods
Appropriate assertions: Uses regex matching to verify key message content
Proper mocking: Correctly stubs dependencies to isolate the behavior under test

💡 Suggestions

Minor Enhancements

Consider testing the exact error message content (optional):
The tests use /manifest is empty.*still compiling/i which is good for flexibility, but you might want to verify more of the message content to catch regressions:

}.to raise_error(Shakapacker::Manifest::MissingEntryError) do |error|
  expect(error.message).to include("manifest is empty")
  expect(error.message).to include("still compiling")
  expect(error.message).to include("What to do:")
end

Edge case: non-existent vs empty manifest (question):
The load method (line 165-170) returns {} when the manifest file doesn't exist. Should we differentiate between "file doesn't exist" and "file exists but is empty"? Currently both trigger the "still compiling" message. This might be correct, but worth confirming it matches the intended behavior.
Consistency in error message formatting (very minor):
The new manifest_empty_error uses <<~MSG (squiggly heredoc, which strips leading whitespace), while missing_file_from_manifest_error uses <<-MSG (dash heredoc). This is fine and might be intentional for better formatting, but worth noting for consistency.

🔍 Technical Review

Potential Issues: None Found

No race conditions
No performance concerns
No security issues
No backwards compatibility issues
Proper error handling

Code Path Analysis

✅ Empty manifest ({}) → new clear error message
✅ Non-empty manifest with missing entry → existing detailed error message
✅ Entry exists → normal lookup returns path

📋 Checklist

Code follows project conventions (per CLAUDE.md)
Tests are comprehensive and passing
Error messages are clear and actionable
No breaking changes
Minimal, focused changes
Documentation (via error message) is helpful

🎯 Recommendation

APPROVE - This is a high-quality PR that solves a real usability problem with a clean, focused implementation. The code is well-tested, follows project conventions, and significantly improves the user experience when encountering empty manifests during development.

The suggestions above are minor and optional - the PR is solid as-is.

Reviewed by Claude Code

greptile-apps · 2026-02-04T00:35:05Z

Greptile Overview

Greptile Summary

Detected when the manifest is empty (bundler still building) and replaced the confusing 7-point error list with a focused message explaining the bundler is still compiling.

Added manifest_empty_error method providing clearer guidance for users when the bundler hasn't finished compiling
Modified handle_missing_entry to check if data.empty? before showing generic error
Added comprehensive test coverage for both lookup! and lookup_pack_with_chunks! methods
Tests properly stub the manifest data and verify error message content
All files follow project conventions with trailing newlines

Confidence Score: 5/5

This PR is safe to merge with no risk
The change is well-tested, follows project conventions, improves user experience with clearer error messaging, and doesn't modify any core logic - only adds an early check for empty manifest to provide better error messages
No files require special attention

Important Files Changed

Filename	Overview
lib/shakapacker/manifest.rb	Added empty manifest detection to show clearer error message when bundler is still compiling
spec/shakapacker/manifest_spec.rb	Added comprehensive tests for empty manifest error handling in both lookup methods

Sequence Diagram

sequenceDiagram
    participant User
    participant ViewHelper
    participant Manifest
    participant HandleMissingEntry
    
    User->>ViewHelper: Request asset (e.g., javascript_pack_tag)
    ViewHelper->>Manifest: lookup!("application.js")
    Manifest->>Manifest: compile() if needed
    Manifest->>Manifest: lookup("application.js")
    Manifest->>Manifest: find("application.js")
    Manifest-->>Manifest: returns nil (not found)
    Manifest->>HandleMissingEntry: handle_missing_entry(name, pack_type)
    
    alt Manifest data is empty
        HandleMissingEntry->>HandleMissingEntry: Check data.empty?
        HandleMissingEntry->>HandleMissingEntry: manifest_empty_error()
        HandleMissingEntry-->>User: Raise MissingEntryError<br/>"Manifest is empty. Bundler still compiling..."
    else Manifest data exists
        HandleMissingEntry->>HandleMissingEntry: missing_file_from_manifest_error()
        HandleMissingEntry-->>User: Raise MissingEntryError<br/>"Can't find file in manifest. 7 possible causes..."
    end

Update the error handling to provide different messages for: 1. Empty manifest file (bundler is still compiling) 2. Missing manifest file (bundler hasn't started yet) This gives users more actionable guidance based on the specific scenario. Co-Authored-By: Claude Opus 4.5 <[email protected]>

claude · 2026-02-04T01:14:13Z

Pull Request Review

Summary

This PR improves the error messaging when the manifest file is empty or missing, addressing issue #871. The implementation provides clearer, more actionable feedback to users instead of showing a confusing list of 7 possible causes.

✅ Strengths

1. Excellent User Experience Improvement

The new error messages are significantly more helpful:

Distinguishes between empty manifest (bundler still compiling) vs missing manifest (bundler not started)
Provides clear, actionable next steps for each scenario
Reduces cognitive load by focusing on the most likely cause

2. Well-Structured Code

Clean separation of concerns with manifest_empty_error method
Good use of conditional logic based on manifest_path.exist?
Maintains backward compatibility for non-empty manifests

3. Comprehensive Test Coverage

The tests cover all critical scenarios:

Empty manifest with both lookup! and lookup_pack_with_chunks!
Missing manifest file cases
Uses proper RSpec stubbing patterns as per CLAUDE.md guidelines

4. Follows Project Conventions

Uses heredoc (<<~MSG) for multi-line strings ✅
Proper line endings (files end with newline) ✅
Clear error message formatting ✅

🔍 Code Quality Observations

Minor Considerations

1. Early Return Check Location (lib/shakapacker/manifest.rb:158-160)

The empty manifest check happens in handle_missing_entry, which is only called when an entry is not found. This is correct, but consider:

The check runs for every missing asset lookup (multiple times per page load)
Alternatively, could be checked once in the load method, but current approach is fine for clarity

Recommendation: Current implementation is acceptable. No change needed.

2. Test Isolation (spec/shakapacker/manifest_spec.rb:9-10, 19-20, etc.)

Tests stub Shakapacker.manifest.data to return {}, which is good. However, consider whether these tests would benefit from also testing the manifest_path.exist? logic more explicitly.

Current approach:

allow(Shakapacker.manifest).to receive(:data).and_return({})
allow(Shakapacker.config.manifest_path).to receive(:exist?).and_return(true)

This works well and tests the right behavior. ✅

3. Error Message Consistency

The error messages use different formatting styles:

New messages: Clean, modern format with bullet points
Old message (line 188-203): Uses numbered list with different structure

Recommendation: Consider standardizing error message format across the codebase in a future PR (not blocking for this PR).

🐛 Potential Issues

None Found

I did not identify any bugs, security concerns, or performance issues.

🧪 Test Coverage Analysis

Coverage: Excellent ✅

The tests cover:

✅ Empty manifest with file existing
✅ Missing manifest file
✅ Both lookup! and lookup_pack_with_chunks! methods
✅ Proper error message format validation using regex

Suggestions for additional testing (optional, not blocking):

Test that the bundler name (config.assets_bundler) appears correctly in error messages
Test that the manifest path appears in the error message
Integration test with actual empty manifest file (not just stubbed data)

🔒 Security Considerations

No security concerns identified. ✅

The changes only affect error message generation and don't:

Execute user input
Modify file system operations
Change authentication/authorization logic
Introduce XSS or injection vulnerabilities

⚡ Performance Considerations

No performance concerns. ✅

The empty check (data.empty?) is O(1) for Ruby hashes
Only executes in error paths (when asset lookup fails)
No additional file I/O or expensive operations introduced

📋 Checklist Review

Based on CLAUDE.md requirements:

✅ Files end with trailing newline
✅ RSpec tests added and should pass (per PR description)
✅ Code follows existing conventions
✅ Changes are focused and minimal
✅ No unnecessary comments
⚠️ bundle exec rubocop should be run (marked as done in PR description)
⚠️ Tests should be run (marked as done in PR description)

📝 Recommendations

Before Merge:

✅ Verify tests pass: Run bundle exec rspec spec/shakapacker/manifest_spec.rb to confirm all 30 tests pass (4 new + 26 existing)
✅ Verify linting: Run bundle exec rubocop lib/shakapacker/manifest.rb spec/shakapacker/manifest_spec.rb to ensure no style violations
Consider manual testing: Start dev server, load page before compilation finishes, verify error message appears as expected

Future Improvements (not blocking):

Consider standardizing error message format across the codebase
Consider adding integration tests with actual empty manifest files
Could add a link to documentation in error messages

🎯 Overall Assessment

LGTM! Excellent PR. ✅✅✅

This is a high-quality change that significantly improves the developer experience. The code is clean, well-tested, and solves a real user pain point. The implementation is thoughtful with proper differentiation between empty and missing manifest scenarios.

Recommendation: Approve and merge after verifying:

Tests pass (bundle exec rspec spec/shakapacker/manifest_spec.rb)
Linting passes (bundle exec rubocop)

Review completed by Claude Sonnet 4.5 🤖

claude · 2026-02-04T02:21:51Z

Code Review

Summary

This PR improves error messaging when the manifest is empty or missing, helping users quickly identify whether the bundler is still compiling or hasn't run yet. The implementation is solid overall with good test coverage.

✅ Strengths

Clear User Experience Improvement: The new error messages are much more actionable than the generic 7-point checklist, especially for the common case of the bundler still compiling.
Good Test Coverage: The new tests cover both scenarios (empty manifest and missing manifest) for both lookup! and lookup_pack_with_chunks! methods.
Follows Project Conventions: Code style matches existing patterns, proper use of heredocs, and appropriate error handling.
Dynamic Configuration: Uses config.assets_bundler to show the correct bundler name (webpack/rspack) in error messages.

🔍 Issues Found

1. Missing Trailing Newline ⚠️ CRITICAL

Location: lib/shakapacker/manifest.rb:247

The file is missing the required trailing newline. According to CLAUDE.md:

ALWAYS end all files with a trailing newline character. This is required by the project's linting rules.

This will cause bundle exec rubocop to fail.

2. Missing RBS Type Signature

Location: sig/shakapacker/manifest.rbs

The new private method manifest_empty_error needs to be added to the type signature file:

def manifest_empty_error: (bool manifest_exists) -> String

3. Inconsistent Error Flow Logic

Location: lib/shakapacker/manifest.rb:157-159

The current implementation in handle_missing_entry:

def handle_missing_entry(name, pack_type)
  if data.empty?
    raise Shakapacker::Manifest::MissingEntryError, manifest_empty_error(config.manifest_path.exist?)
  end

  raise Shakapacker::Manifest::MissingEntryError, missing_file_from_manifest_error(full_pack_name(name, pack_type[:type]))
end

Issue: This method is only called when an entry is not found in the manifest. However, if data.empty? is true, we already know the entry won't be found. The check happens after we've already determined the lookup failed.

Potential Edge Case: If the manifest has entries but just not the one we're looking for, we still get the old detailed error message (which is correct). The new error only shows when the manifest is completely empty AND we're looking for something.

Consideration: While this works correctly, the logic could be clearer. The check happens at the right time (when lookup fails), but the code flow might benefit from a comment explaining this is intentional.

4. Test Implementation Uses Mocking of Internal State

Location: spec/shakapacker/manifest_spec.rb:8-22

The tests mock data to return an empty hash:

allow(Shakapacker.manifest).to receive(:data).and_return({})

Concern: This mocks a private method, which couples tests to implementation details. If the internal implementation changes, tests could break even though the public API works correctly.

Better Approach: Consider testing with actual fixture files (empty manifest.json) rather than mocking private methods. This would make tests more robust and realistic.

Example:

# Create a temporary empty manifest file
let(:empty_manifest_path) { Tempfile.new(['manifest', '.json']) }
before do
  empty_manifest_path.write('{}')
  empty_manifest_path.close
  allow(Shakapacker.config).to receive(:manifest_path).and_return(Pathname.new(empty_manifest_path.path))
end

📝 Suggestions

1. Changelog Entry Missing

According to CLAUDE.md, this change should have a changelog entry:

Update CHANGELOG.md for user-visible changes only (features, bug fixes, breaking changes, deprecations, performance improvements)

This is a user-visible bug fix (improved error messages). Should be added to the ### Fixed section under ## [Unreleased].

Suggested entry:

- **Improved error message when manifest is empty**. [PR #872](https://github.com/shakacode/shakapacker/pull/872) by [justin808](https://github.com/justin808). When the bundler is still compiling (manifest empty) or hasn't run yet (manifest missing), users now see clear, actionable error messages instead of a confusing 7-point checklist.

2. Error Message Wording

Location: lib/shakapacker/manifest.rb:208-210

The phrase "#{bundler_name} is likely still compiling" could be more confident:

# Current
Shakapacker manifest is empty. #{bundler_name} is likely still compiling.

# Suggestion
Shakapacker manifest is empty. #{bundler_name} is still compiling or hasn't started yet.

This is more precise about the two states that lead to an empty manifest.

🎯 Action Items

Before merging:

✅ Add trailing newline to lib/shakapacker/manifest.rb
✅ Add type signature to sig/shakapacker/manifest.rbs
✅ Add changelog entry to CHANGELOG.md
⚠️ Run bundle exec rubocop to verify compliance
⚠️ Run bundle exec rspec to verify all tests pass

Nice to have:

Consider using fixture files instead of mocking private methods in tests
Consider adding a comment in handle_missing_entry explaining the empty check happens after lookup failure

🏁 Conclusion

This is a valuable improvement to user experience. The implementation is sound, but needs a few small fixes before merging:

Missing trailing newline (required by linting)
Missing type signature
Missing changelog entry

Once these are addressed, this will be ready to merge! Great work on improving the developer experience. 🎉

Overall Rating: ⭐⭐⭐⭐ (4/5) - Solid improvement with minor fixes needed

coderabbitai bot approved these changes Feb 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve error message when manifest is empty #872

Improve error message when manifest is empty #872

justin808 commented Feb 4, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Feb 4, 2026 •

edited

Loading

Uh oh!

claude bot commented Feb 4, 2026

Uh oh!

greptile-apps bot commented Feb 4, 2026

Uh oh!

claude bot commented Feb 4, 2026

Uh oh!

claude bot commented Feb 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Improve error message when manifest is empty #872

Are you sure you want to change the base?

Improve error message when manifest is empty #872

Conversation

justin808 commented Feb 4, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

claude bot commented Feb 4, 2026

Pull Request Review

Summary

✅ Strengths

Code Quality

Error Message Quality

Test Coverage

💡 Suggestions

Minor Enhancements

🔍 Technical Review

Potential Issues: None Found

Code Path Analysis

📋 Checklist

🎯 Recommendation

Uh oh!

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

claude bot commented Feb 4, 2026

Pull Request Review

Summary

✅ Strengths

1. Excellent User Experience Improvement

2. Well-Structured Code

3. Comprehensive Test Coverage

4. Follows Project Conventions

🔍 Code Quality Observations

Minor Considerations

1. Early Return Check Location (lib/shakapacker/manifest.rb:158-160)

2. Test Isolation (spec/shakapacker/manifest_spec.rb:9-10, 19-20, etc.)

3. Error Message Consistency

🐛 Potential Issues

None Found

🧪 Test Coverage Analysis

🔒 Security Considerations

⚡ Performance Considerations

📋 Checklist Review

📝 Recommendations

Before Merge:

Future Improvements (not blocking):

🎯 Overall Assessment

Uh oh!

claude bot commented Feb 4, 2026

Code Review

Summary

✅ Strengths

🔍 Issues Found

1. Missing Trailing Newline ⚠️ CRITICAL

2. Missing RBS Type Signature

3. Inconsistent Error Flow Logic

4. Test Implementation Uses Mocking of Internal State

📝 Suggestions

1. Changelog Entry Missing

2. Error Message Wording

🎯 Action Items

🏁 Conclusion

Uh oh!

Reviewers

justin808 commented Feb 4, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 4, 2026 •

edited

Loading