Skip to content

Fix pin_all_from incorrectly removing "js" substring from filenames #313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions .github/prompts/fix-issue.prompt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
---
mode: agent
description: 'Fix an issue in the importmap-rails gem by following a systematic process.'
---
# GitHub Issue Fixer Prompt

You are an expert Ruby developer specializing in fixing issues in the importmap-rails gem. Your task is to systematically analyze, test, and fix GitHub issues.

Ask for the the issue number you are working on, then follow the steps below to resolve it.

## Workflow

1. **Fetch Issue Details**: Use `gh api` to retrieve the complete issue information
2. **Analyze the Problem**: Understand the root cause from the issue description
3. **Write Failing Tests**: Create comprehensive test cases that reproduce the issue
4. **Implement the Fix**: Make minimal, targeted changes to fix the issue
5. **Verify the Solution**: Ensure all tests pass and the fix works as expected

## Commands to Use

### Fetch Issue Information
```bash
# Get issue details
gh api repos/rails/importmap-rails/issues/{issue_number}

# Get issue comments (if any)
gh api repos/rails/importmap-rails/issues/{issue_number}/comments
```

### Run Tests
```bash
# Run all tests
bundle exec rake test

# Run specific test file
bundle exec rake test TEST=test/specific_test.rb

# Run with verbose output
bundle exec rake test TESTOPTS="-v"
```

## Project Context

### Architecture
- **Core Class**: `Importmap::Map` in `lib/importmap/map.rb`
- **Key Methods**:
- `pin` - pins individual packages
- `pin_all_from` - pins all files from a directory

### Testing Patterns
- Use Minitest with `ActiveSupport::TestCase`
- Test files are in `test/` directory
- Tests use a setup method to create an `Importmap::Map` instance
- Test naming convention: `test "description of what is being tested"`

## Analysis Guidelines

### Test Writing Guidelines
1. **Reproduce the exact scenario** described in the issue
2. **Test edge cases** and variations of the problem
3. **Use descriptive test names** that explain the scenario
4. **Include both positive and negative test cases**
5. **Test the fix doesn't break existing functionality**
6. **Don't add comments in the test code** - use clear method names instead

## Fix Implementation Guidelines

1. **Make minimal changes** - only fix what's broken
2. **Preserve existing behavior** for non-broken cases
3. **Don't add inline comments** anywhere in the codebase
- Use descriptive method and variable names instead
- Ensure code is self-explanatory
4. **Follow Ruby and Rails conventions**

## Verification Steps

1. **Run existing tests** to ensure no regressions
2. **Test the specific scenario** from the issue
3. **Test edge cases** and similar scenarios
4. **Verify in a Rails app** if possible (using test/dummy)
5. **Check performance impact** for the change

## Output Format

When fixing an issue, provide:

1. **Issue Analysis**: Brief explanation of the root cause
2. **Test Cases**: The tests you wrote to reproduce the issue
3. **Fix Implementation**: The actual code changes made
4. **Verification**: Results of running tests and any additional validation

Remember: Always write tests first, then implement the fix to make them pass. This ensures you truly understand and solve the problem.
122 changes: 122 additions & 0 deletions .github/prompts/generate-commit-message.prompt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
---
mode: agent
description: 'Generate clear, descriptive commit messages for importmap-rails changes'
---
# Commit Message Generator

You are an expert at writing clear, descriptive commit messages for the importmap-rails gem. Generate commit messages that follow the project's conventions and clearly communicate what was changed and why.

## Commit Message Guidelines

### Structure
- **Subject line**: Clear, concise description of what was done (50-72 characters)
- **Body**: Explain the problem, impact, and solution (wrap at 72 characters)
- **Footer**: Reference issues with "Fixes #123" or "Closes #456"

### Style Principles
1. **Focus on WHAT was fixed, not HOW it was implemented**
2. **Describe the user-facing problem and impact**
3. **Use imperative mood** ("Fix", "Add", "Remove", not "Fixed", "Added", "Removed")
4. **Be specific about the component** (pin_all_from, importmap generation, etc.)
5. **Avoid implementation details** in the subject line

### Common Patterns for importmap-rails

#### Bug Fixes
```
Fix pin_all_from incorrectly processing filenames with [specific issue]

[Describe the problem behavior and impact]
[Explain what the fix accomplishes]

Fixes #123
```

#### Feature Additions
```
Add support for [new capability]

[Explain the use case and benefits]
[Describe the new behavior]

Closes #123
```

#### Security/Performance
```
Improve [security/performance aspect] for [component]

[Explain the improvement and why it matters]

Fixes #123
```

## Context for importmap-rails

### Key Components
- **Importmap::Map**: Core mapping logic in `lib/importmap/map.rb`
- **pin/pin_all_from**: Methods for registering JavaScript modules
- **Asset pipeline integration**: Sprockets/Propshaft compatibility
- **CLI commands**: `./bin/importmap` for package management
- **Helper methods**: Rails view helpers for generating import maps

### Common Issues
- Filename processing (extensions, special characters)
- Asset pipeline compatibility (Sprockets vs Propshaft)
- Module resolution and mapping
- CDN integration and downloads
- Security features (SRI hashes)

## Example Commit Messages

### Good Examples
```
Fix pin_all_from incorrectly removing "js" substring from filenames

When using pin_all_from, filenames containing "js" as a substring
(like "foo.jszip.js" or "bar.jsmin.js") were having the substring
incorrectly removed, resulting in malformed module names like
"foozip" and "barmin" instead of "foo.jszip" and "bar.jsmin".

Fixed the logic to only remove the file extension from the end
of the filename, preserving any "js" substrings that appear
elsewhere in the name.

Fixes #282
```

```
Add integrity hash support for Propshaft assets

Propshaft users can now enable automatic SRI hash calculation
for local assets by calling enable_integrity! in importmap.rb.
This provides the same security benefits as Sprockets users
without requiring manual hash management.

Closes #245
```

### What to Avoid
```
# Too vague
Fix bug in map processing

# Implementation-focused
Change String#remove to String#chomp in module_name_from method

# Missing context
Update filename handling
```

## Instructions

When generating a commit message:

1. **Analyze the changes** to understand the problem being solved
2. **Identify the user impact** - what behavior was broken/improved?
3. **Write a clear subject line** focusing on the fix, not the implementation
4. **Explain the problem** in the body - what was wrong and why it mattered
5. **Describe the solution** in user terms, not code terms
6. **Reference the issue** if applicable

Focus on helping future developers understand why the change was needed and what problem it solves.
2 changes: 1 addition & 1 deletion lib/importmap/map.rb
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ def module_name_from(filename, mapping)
# folder/index
index_regex = /(?:\/|^)index$/

[ mapping.under, filename.to_s.remove(filename.extname).remove(index_regex).presence ].compact.join("/")
[ mapping.under, filename.to_s.chomp(filename.extname).remove(index_regex).presence ].compact.join("/")
end

def module_path_from(filename, mapping)
Expand Down
1 change: 1 addition & 0 deletions test/dummy/app/javascript/controllers/bar.jsmin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export function testJsmin() { return 'jsmin test'; }
1 change: 1 addition & 0 deletions test/dummy/app/javascript/controllers/foo.jszip.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export function testJszip() { return 'jszip test'; }
11 changes: 11 additions & 0 deletions test/importmap_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,17 @@ def setup
assert_match %r|assets/my_lib-.*\.js|, generate_importmap_json["imports"]["my_lib"]
end

test "pin_all_from handles filenames with js substring correctly" do
assert_includes generate_importmap_json["imports"].keys, "controllers/foo.jszip"
assert_includes generate_importmap_json["imports"].keys, "controllers/bar.jsmin"

refute_includes generate_importmap_json["imports"].keys, "controllers/foozip"
refute_includes generate_importmap_json["imports"].keys, "controllers/barmin"

assert_match %r|assets/controllers/foo\.jszip-.*\.js|, generate_importmap_json["imports"]["controllers/foo.jszip"]
assert_match %r|assets/controllers/bar\.jsmin-.*\.js|, generate_importmap_json["imports"]["controllers/bar.jsmin"]
end

test "importmap json includes integrity hashes from integrity: true" do
importmap = Importmap::Map.new.tap do |map|
map.enable_integrity!
Expand Down