fix(cli): cleanup pitfalls and improve overall fetch UX

[dbeal-eth]

fixes https://linear.app/usecannon/issue/CAN-735/fix-wierd-behaviors-with-cannon-fetch

[claude]

Code Review

This PR improves the cannon fetch command UX by enabling auto-detection of package names from IPFS data. Overall, this is a solid improvement that makes the command more user-friendly while maintaining backwards compatibility.

✅ Strengths

Excellent UX Improvements:

• Auto-detection of package name from IPFS data reduces user friction
• Backwards compatibility maintained with optional package reference parameter
• Clear validation warnings when provided package info doesn't match IPFS data
• Comprehensive documentation updates in CLAUDE.md with usage examples

Good Error Handling:

• Improved error message for missing IPFS data with helpful troubleshooting steps (lines 70-76)
• Proper validation between user-provided and IPFS data with clear warning messages

Code Quality:

• Clean parameter restructuring with proper null handling
• Consistent with project conventions and TypeScript best practices

🔧 Minor Issues & Suggestions

1. Unused Variables (fetch.ts:82)

let name: string, version: string, preset: string;

These variables are declared but never used. Consider removing them.

2. Logic Flaw in Validation (fetch.ts:93-96)
The current validation uses else if which means only one mismatch will be reported:

if (ref.name !== deployInfo.def.name) {
  warn(yellow('...'));
} else if (ref.version !== deployInfo.def.version) { // This won't run if name differs
  warn(yellow('...'));
} else if (deployInfo.chainId && chainId !== deployInfo.chainId) { // This won't run if name/version differs
  warn(yellow('...'));
}

Suggestion: Use separate if statements to report all mismatches:

if (ref.name !== deployInfo.def.name) {
  warn(yellow('The IPFS package you downloaded is being saved to a different name than is recorded in the package data. Please double check to make sure this is correct.'));
  warn(yellow(bold(`Package Name (IPFS Data): ${deployInfo.def.name}`)));
  warn(yellow(bold(`Provided Name:            ${ref.name}`)));
}
if (ref.version !== deployInfo.def.version) {
  warn(yellow('The IPFS package you downloaded is being saved to a different version than is recorded in the package data. Please double check to make sure that this is correct.'));
  warn(yellow(bold(`Package Version (IPFS Data): ${deployInfo.def.version}`)));
  warn(yellow(bold(`Provided Version:            ${ref.version}`)));
}
if (deployInfo.chainId && chainId !== deployInfo.chainId) {
  warn(yellow('The IPFS package you downloaded is being saved to a different chain ID than is recorded in the package data. Please double check to make sure that this is correct.'));
  warn(yellow(bold(`Chain ID (IPFS Data):    ${deployInfo.chainId}`)));
  warn(yellow(bold(`Chain ID (User Input):   ${chainId}`)));
}

3. Chain ID Handling (fetch.ts:80,113)
The fallback chain ID logic could be more explicit. Consider extracting it to a helper function for clarity:

const getResolvedChainId = (userChainId: number | null, deploymentChainId?: number) => {
  return userChainId || deploymentChainId || 13370;
};

🧪 Testing

The e2e test updates look appropriate:

• Tests both new auto-detection and legacy explicit package name usage
• Maintains backwards compatibility verification

🚀 Overall Assessment

This is a well-implemented improvement that significantly enhances user experience. The auto-detection feature addresses a real UX pain point while maintaining full backwards compatibility. The code follows project conventions and includes proper error handling.

Recommendation: ✅ Approve with minor fixes

The suggested changes are minor and don't block the PR, but addressing them would improve code quality and user experience.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review: CLI Fetch Command Improvements

I've thoroughly reviewed this PR that improves the cannon fetch command UX by adding auto-detection capabilities. Here's my comprehensive feedback:

✅ Positive Changes

Excellent UX Improvement

• Auto-detection of package name from IPFS data is a great enhancement that reduces friction
• Maintains backwards compatibility by making package reference optional
• Clear error messages with actionable guidance help users troubleshoot issues

Good Code Structure

• Well-structured validation logic with helpful warning messages
• Proper fallback to sensible defaults (chain ID 13370)
• Clear separation between auto-detection and explicit package reference paths

Documentation Updates

• Comprehensive CLAUDE.md updates with usage examples and best practices
• Good addition to the development workflow section

⚠️ Issues & Concerns

1. Breaking API Change Without Version Bump

The argument order change from <packageRef> <ipfsHash> to <ipfsHash> [packageRef] is a breaking change that could silently fail for existing scripts.

Recommendation: Consider a deprecation warning approach or maintain dual support temporarily.

2. Inconsistent Error Handling

// packages/cli/src/commands/fetch.ts:41
export async function fetch(fullPackageRef: string | null, chainId: number | null, ...)

The function signature allows null values but doesn't handle all edge cases consistently:

• Missing validation for when both chainId and deployInfo.chainId are null
• No explicit handling of invalid IPFS hash formats

3. Validation Logic Issues

Version Comparison Bug:

// This comparison may fail - should use semantic versioning comparison
} else if (ref.version !== deployInfo.def.version) {

Chain ID Type Mismatch:

} else if (deployInfo.chainId && chainId !== deployInfo.chainId) {

This comparison could have type issues since chainId might be null.

4. Import Issues

import { log, warn } from '../util/console';
// but the file uses:
import { logSpinner } from '../util/console';

The diff shows imports of log and warn but the original file uses different console utilities.

🔒 Security Considerations

Minor Concerns:

• IPFS hash validation could be more robust to prevent injection attacks
• Consider validating package name format to prevent path traversal issues
• The auto-detection feature relies heavily on trusting IPFS data integrity

🚀 Performance Notes

• No significant performance impacts identified
• Efficient fallback logic without unnecessary IPFS calls
• Good use of early returns to avoid redundant processing

🧪 Test Coverage Assessment

Gaps Identified:

• Only basic e2e shell tests updated - no comprehensive unit tests for new logic
• Missing edge case testing for validation warnings
• No tests for error scenarios (invalid IPFS data, network failures)
• Auto-detection logic needs dedicated test coverage

Recommended Tests:

# Test cases needed:
- Auto-detection with valid IPFS data
- Version/name mismatch warnings  
- Chain ID mismatch scenarios
- Invalid IPFS hash handling
- Backwards compatibility with explicit package refs

📝 Specific Recommendations

High Priority:

1. Add comprehensive unit tests for the new auto-detection logic
2. Fix version comparison to use semantic versioning
3. Validate IPFS hash format before attempting to fetch
4. Handle edge cases where all chain ID sources are null

Medium Priority:

1. Consider adding a --validate-strict flag for stricter validation
2. Add progress indicators for IPFS fetch operations
3. Consider adding retry logic for network failures

Code Style:

• Follows Google TypeScript Style Guide well
• Good use of destructuring and modern JS features
• Consistent error message formatting

🎯 Overall Assessment

This is a solid improvement that significantly enhances the user experience. The auto-detection feature is well-implemented and the backwards compatibility approach is thoughtful. However, there are some edge cases and testing gaps that should be addressed before merging.

Recommendation: Address the validation logic issues and add comprehensive tests, then this will be ready to merge.

Great work on improving the CLI UX! 🎉

[claude]

PR Review: Fix CLI Fetch UX and Cleanup Pitfalls

Overview

This PR improves the cannon fetch command by making the package reference optional and auto-detecting package information from IPFS data. This is a meaningful UX improvement that removes friction from the fetch workflow.

✅ Strengths

Code Quality & Best Practices

• Good API Design: Making package reference optional with auto-detection is intuitive and reduces user friction
• Backwards Compatibility: Still supports explicit package reference for validation scenarios
• Clear Error Messages: Improved error messaging with helpful troubleshooting steps (packages/cli/src/commands/fetch.ts:69-76)
• Proper Documentation: Updated CLAUDE.md with clear usage examples and when to use cannon fetch
• Following Conventions: Uses existing patterns for argument handling and validation warnings

Security Considerations

• Input Validation: Proper validation of IPFS URLs and package references
• No Security Regressions: Changes don't introduce security vulnerabilities
• Safe Defaults: Falls back to sensible defaults (chain ID 13370) when data is missing

⚠️ Issues & Recommendations

Potential Bugs

1. Import Inconsistency: The new code imports warn from console utils but the original file shows different imports:

   // In diff: import { log, warn } from '../util/console';  
   // In original: import { logSpinner } from '../util/console';

   Fix: Update imports to use logSpinner, warnSpinner consistently with the existing spinner pattern.

2. Console Function Usage: The PR switches from logSpinner() to log() which breaks the spinner UX that was recently implemented:

   // Should be: logSpinner() instead of log()
   log(`\nDetected package: ${packageRef}`);  // Line 106 in diff

Type Safety Issues

3. Loose Type Handling: Function signature changes introduce nullable types without proper null checks:

   // Old: export async function fetch(fullPackageRef: string, chainId: number, ...)
   // New: export async function fetch(fullPackageRef: string | null, chainId: number | null, ...)

   Consider using stricter types or better validation.

Testing Concerns

4. Limited Test Coverage: The e2e test only covers the happy path. Missing tests for:

   • Auto-detection with invalid IPFS data
   • Package name/version mismatches and warnings
   • Error scenarios (invalid IPFS hashes, network failures)
   • Metadata fetching edge cases

5. No Unit Tests: The fetch command has no unit test coverage. Consider adding tests in packages/cli/src/commands/__tests__/fetch.test.ts

Code Quality Improvements

6. Variable Extraction: The auto-detection logic could be extracted into a separate function for better testability:

   function detectPackageInfo(deployInfo: DeploymentInfo, def: ChainDefinition, preCtx: any) {
     return new PackageReference(`${deployInfo.def.name}:${def.getVersion(preCtx) || 'latest'}@${def.getPreset(preCtx)}`).fullPackageRef;
   }

7. Warning Message Formatting: The warning messages use inconsistent formatting and could benefit from a helper function for consistent styling.

🔧 Specific Fixes Needed

High Priority

1. Fix imports and console usage - Use warnSpinner instead of warn, maintain logSpinner consistency
2. Add error handling tests to the e2e suite
3. Validate the auto-detection flow with malformed IPFS data

Medium Priority

4. Add unit tests for the fetch command
5. Extract auto-detection logic into testable helper function
6. Improve type safety with better null handling

Low Priority

7. Standardize warning message formatting
8. Consider adding progress indicators for IPFS fetching steps

📝 Test Coverage Recommendations

# Add to packages/cli/test/e2e/scripts/non-interactive/fetch.sh
# Test error scenarios
$CANNON fetch invalid-ipfs-hash --chain-id 1 2>&1 | grep -q "Could not find package data"
$CANNON fetch QmInvalidHash synthetix:999.0.0 --chain-id 1 2>&1 | grep -q "different name"

# Test edge cases  
$CANNON fetch QmValidHash --chain-id 999  # Test with non-matching chain ID

🎯 Summary

This is a solid UX improvement that addresses a real user pain point. The auto-detection feature is well-implemented, but needs some fixes around console usage consistency and better test coverage. The backwards compatibility approach is excellent.

Recommendation: Approve after addressing the import/console usage issues and adding basic error scenario tests.

[claude]

PR Review: Fix CLI fetch command UX improvements

✅ Positive Changes

1. Improved UX with Auto-detection: The change from requiring explicit package names to auto-detecting them from IPFS data is a great UX improvement. This makes the command more intuitive for users.

2. Better Error Messages: The enhanced error handling with multi-line error messages provides clearer guidance when IPFS fetching fails.

3. Comprehensive Documentation: Excellent addition of fetch command documentation to CLAUDE.md with clear examples and usage patterns.

4. Backwards Compatibility: Maintaining support for explicit package references ensures existing scripts won't break.

5. Clear Warnings: The validation warnings when package data doesn't match user input help prevent confusion.

🔍 Code Quality Issues

1. Type Safety Concerns:

   • chainId: number | null parameter can cause issues downstream where number is expected
   • Consider using a default value instead of null to maintain type safety
   • The fallback chain deployInfo.chainId || chainId || 13370 could be clearer

2. Import Inconsistency:

   • Missing import cleanup: log, warn are imported but logSpinner calls are still used
   • yellow, bold from chalk are imported but the existing blueBright import pattern suggests using consistent styling

3. Error Handling:

   • The deployment info validation could be more robust - checking Object.keys(deployInfo).length === 0 may not catch all invalid states
   • Consider validating the structure of deployInfo.def before using it

🚨 Potential Bugs

1. Parameter Order Change: The CLI parameter order changed from packageRef ipfsHash to ipfsHash [packageRef]. This is a breaking change that could break existing scripts and needs clear migration notes.

2. Chain ID Validation: The chain ID comparison warnings may trigger false positives if deployInfo.chainId is undefined but a valid chainId is provided.

🧪 Test Coverage

The e2e test was updated appropriately, but consider adding:

• Test case for mismatched package names/versions
• Test case for invalid IPFS hashes
• Test case for missing chain ID scenarios

💡 Recommendations

1. Breaking Change Documentation: Add a clear migration guide for the parameter order change
2. Type Safety: Consider using chainId?: number with proper defaults instead of nullable types
3. Validation Logic: Strengthen the deployInfo validation to check for required fields
4. Consistent Styling: Align the chalk usage pattern with the existing codebase

🎯 Overall Assessment

This is a solid improvement to the fetch command UX. The auto-detection feature is valuable, and the code handles edge cases well. The main concerns are around the breaking change in parameter order and some type safety improvements.

Recommendation: ✅ Approve with minor fixes - Address the type safety concerns and ensure the breaking change is well documented.

[claude]

Code Review: Fix fetch UX and cleanup pitfalls

Overall Assessment ✅

This PR significantly improves the cannon fetch command UX by making package names optional and auto-detectable from IPFS data. The changes are well-implemented and follow Cannon's patterns.

Strengths 🎯

Excellent UX Improvement

• Auto-detection: Making package names optional and auto-detectable from IPFS data is a great UX enhancement
• Backwards compatibility: Retains support for explicit package specification while making it optional
• Clear documentation: Good updates to CLAUDE.md explaining when and how to use cannon fetch

Robust Error Handling

• Enhanced error messages: Much more helpful error messages with troubleshooting guidance (lines 69-76 in fetch.ts)
• Validation warnings: Thoughtful warnings when user-provided names/versions don't match IPFS data
• Graceful fallbacks: Proper fallback to default chain ID (13370) when none provided

Code Quality

• Type safety: Proper nullable types for optional parameters
• Clean imports: Good addition of warn and chalk imports for better user feedback
• Documentation updates: Comprehensive CLAUDE.md updates with examples

Areas for Improvement 🔧

1. Inconsistent Console Usage

The code imports warn from console utils but uses regular warn instead of warnSpinner for consistency with CLI spinner management.

Recommendation: Use warnSpinner from console utils for consistency with the CLI's spinner management.

2. Chain ID Logic Could Be Clearer

The chainId fallback logic works but could be more explicit about precedence. Consider adding a comment explaining the precedence order.

3. Test Coverage Considerations

The test update only shows happy path scenarios. Consider adding tests for:

• Auto-detection with mismatched user-provided names
• Invalid IPFS hashes
• Network connectivity issues
• Various chain ID scenarios

Security & Performance ✅

• No security concerns: Changes don't introduce vulnerabilities
• Performance neutral: Auto-detection adds minimal overhead
• Safe defaults: Appropriate fallback values used throughout

Minor Suggestions 📝

Error Message Enhancement

Consider making the IPFS error message even more actionable by showing the specific gateway and more specific hash format guidance.

Logging Consistency

The success message formatting could match other CLI commands with consistent emoji/formatting patterns.

Conclusion 🎉

This is a solid PR that meaningfully improves the fetch command's usability. The auto-detection feature addresses real user pain points while maintaining backwards compatibility. The implementation is thoughtful with good error handling and user feedback.

Recommendation: Approve with minor suggestions - the current implementation is ready for production, though the console utility consistency fix would be a nice polish.

Great work on improving the developer experience! 🚀

[claude]

Code Review: fix(cli): cleanup pitfalls and improve overall fetch UX

Thank you for this PR! This is a significant UX improvement for the cannon fetch command. Here's my comprehensive review:

---

✅ Strengths

1. Excellent UX Improvement

• Auto-detection of package names from IPFS data is a major usability win - users no longer need to know the package name beforehand
• Argument order swap (ipfsHash first, packageRef optional) makes the most common use case simpler
• Clear documentation added to CLAUDE.md with practical examples

2. Good Error Handling

The improved error message in fetch.ts:72-77 is much more helpful:

throw new Error(
  `Could not find package data on IPFS using the hash: ${ipfsUrl}\n` +
  `Please verify that:\n` +
  `  - The IPFS hash is correct\n` +
  `  - The IPFS gateway is accessible\n` +
  `  - The hash contains valid Cannon package data`
);

3. Backwards Compatibility

• Optional packageRef parameter maintains compatibility with existing workflows
• Validation warnings when user-provided data differs from IPFS data

4. Code Quality

• Follows ConventionalCommits format (fix(cli):)
• Imports are clean and properly organized
• Good use of existing utility functions

---

⚠️ Issues & Concerns

1. Critical: Missing Import in fetch.ts ⚠️

Line 17: The PR diff shows imports added:

import { log, warn } from '../util/console';
import { yellow, bold } from 'chalk';

However, the code continues to use logSpinner (line 61, 95, 96 in original). After the changes, I see:

• log is imported but the file originally used logSpinner
• Need to verify the actual changes use log instead of logSpinner consistently

Recommendation: Ensure all console output uses the appropriate function (log for non-spinner output, logSpinner for spinner-compatible output).

2. Type Safety Issue ⚠️

Line 41 in fetch.ts: Function signature changed to:

export async function fetch(fullPackageRef: string | null, chainId: number | null, ...)

Issue: The function tries to parse fullPackageRef as nullable but then uses it in multiple places:

• Line 85: new PackageReference(fullPackageRef) - This will fail if fullPackageRef is null
• Line 88: Direct string comparison

Recommendation: Add explicit null checks before using fullPackageRef:

if (fullPackageRef) {
  const ref = new PackageReference(fullPackageRef);
  // ... validation logic
}

3. Potential Bug: Default Chain ID Fallback

Line 71 & 112:

const preCtx = await createInitialContext(def, deployInfo.meta, deployInfo.chainId || chainId || 13370, deployInfo.options);
const resolvedChainId = chainId || deployInfo.chainId || 13370;

Issue: Order of precedence differs:

• Line 71: deployInfo.chainId || chainId || 13370 (prefers deployInfo)
• Line 112: chainId || deployInfo.chainId || 13370 (prefers user input)

Recommendation: Use consistent precedence. I suggest preferring user input (chainId) when explicitly provided, otherwise fall back to deployInfo.chainId.

4. Validation Warning Logic

Lines 91-98: The validation only checks if ref.name !== deployInfo.def.name but doesn't check version/preset mismatches separately. Consider:

if (ref.name !== deployInfo.def.name) {
  warn(yellow('Name mismatch...'));
} else if (ref.version !== deployInfo.def.version) {
  warn(yellow('Version mismatch...'));
} else if (deployInfo.chainId && chainId !== deployInfo.chainId) {
  warn(yellow('Chain ID mismatch...'));
}

Currently using else if means only one warning shows. Should these be independent if statements to show all mismatches?

---

📝 Test Coverage Concerns

1. Updated Test May Be Insufficient

File: packages/cli/test/e2e/scripts/non-interactive/fetch.sh

The test file now has:

# Test with auto-detection (new preferred usage)
$CANNON fetch QmUtELWtepNn1ByaFUF8YAgLMtdvy6GGe2P9ex48EVit4H --chain-id 1

# Test with explicit package name (backwards compatibility)
$CANNON fetch QmUtELWtepNn1ByaFUF8YAgLMtdvy6GGe2P9ex48EVit4H synthetix:3.3.4@main --chain-id 1

Issues:

1. The bats test in parallel.bats expects output containing synthetix:3.3.4@main, but with auto-detection, the detected package name might differ
2. No test for validation warnings when package names mismatch
3. No test for null/missing chainId handling
4. No test for error cases (invalid IPFS hash, network errors)

Recommendations:

• Update parallel.bats assertions to match the new auto-detected output format
• Add tests for edge cases: mismatched package names, missing chainId, invalid IPFS data
• Test both execution paths (with and without explicit packageRef)

2. Missing Unit Tests

Consider adding unit tests for:

• Auto-detection logic
• Validation warning conditions
• Chain ID resolution priority
• Error message formatting

---

🎯 Performance Considerations

No significant performance concerns. The changes are primarily UX improvements with minimal overhead:

• Single additional conditional check for fullPackageRef existence
• Auto-detection reuses existing PackageReference parsing

---

🔒 Security Considerations

✅ Good Security Practices:

• No new external dependencies introduced
• Maintains existing IPFS validation
• User input is properly validated before use

⚠️ Minor Concerns:

• Potential for confusion attacks: If auto-detection is used, users might not notice if the IPFS data contains a different package name than expected. The validation warnings help, but could be made more prominent (consider using error level or requiring confirmation for mismatches).

---

📋 Suggestions

High Priority:

1. ✅ Fix the type safety issue with nullable fullPackageRef
2. ✅ Ensure consistent chainId precedence order
3. ✅ Verify import consistency (log vs logSpinner)
4. ✅ Update test assertions to match new output format

Medium Priority:

5. Consider making validation warnings more prominent for security
6. Add more comprehensive error handling tests
7. Clarify in documentation when to use explicit packageRef vs auto-detection

Low Priority (Nice to have):

8. Add --force flag to skip validation warnings for automation
9. Consider adding a --verify flag to check IPFS hash integrity
10. Add progress indicator for large IPFS downloads

---

📚 Documentation

The CLAUDE.md additions are excellent! Clear examples and good explanation of when to use cannon fetch. Small suggestion:

Add a note about the security implications of auto-detection:

**Security Note:** When using auto-detection, always verify the detected package name matches your expectations, especially when working with IPFS hashes from untrusted sources.

---

Overall Assessment

Score: 8/10

This is a solid improvement to the CLI UX. The core idea (auto-detection) is excellent and the implementation is mostly sound. The main concerns are:

1. Type safety with nullable parameters
2. Test coverage gaps
3. Minor inconsistency in chainId precedence

With these addressed, this would be an excellent addition to Cannon. Great work! 🎉

[claude]

Pull Request Review: fix(cli): cleanup pitfalls and improve overall fetch UX

Thank you for working on improving the cannon fetch command! This is a valuable UX enhancement. I've reviewed the changes and have some feedback below.

---

Critical Issues

1. Undefined Variable Bug

Location: packages/cli/src/commands/fetch.ts:125-127

The variable pkgName is used but never defined. This will cause a runtime error.

Fix: Replace pkgName with packageRef in both log statements at lines 125 and 127.

---

Code Quality Issues

2. Unused Variable Declarations

Location: packages/cli/src/commands/fetch.ts:82

These variables are declared but never used. They should be removed.

3. Inconsistent Validation Logic

Location: packages/cli/src/commands/fetch.ts:93-97

The validation uses else if chains, which means only one warning will be shown even if multiple mismatches exist.

Suggestion: Use independent if statements instead of else if to show all applicable warnings.

4. Missing Import

Location: packages/cli/src/commands/fetch.ts:108

The log function is used but not imported. This will cause a runtime error.

Fix: Import log from the console utilities or replace with logSpinner.

5. Potential Null Reference

Location: packages/cli/src/commands/fetch.ts:97-100

The chain ID validation should add explicit null check for clarity.

---

Design and UX Considerations

6. Breaking Change Documentation

The argument order has changed. Consider adding migration notes in the PR description and documenting this as a breaking change.

7. Preset Detection Edge Case

Location: packages/cli/src/commands/fetch.ts:106

Add a fallback to main if preset is not detected.

8. Chain ID Fallback Consistency

Location: packages/cli/src/commands/fetch.ts:113

Consider logging when falling back to default chain ID 13370.

---

Test Coverage

9. Limited E2E Test Coverage

The e2e tests cover the happy path but should also test error cases, warning scenarios, and edge cases.

---

Security Considerations

10. IPFS Data Validation

Add validation that required fields exist in deployInfo.def (name and version).

---

Documentation

11. CLAUDE.md Updates

The documentation additions are excellent and helpful.

---

Positive Highlights

• Excellent UX improvement - Auto-detection removes friction
• Backwards compatible - Old usage pattern still works
• Better error messages - Clear, actionable error text
• Good validation - Warnings help users catch mistakes
• Documentation first - CLAUDE.md updates included

---

Summary

Overall: This is a valuable UX improvement. However, there are 2 critical bugs that will cause runtime errors:

1. Undefined pkgName variable (lines 125, 127)
2. Missing log import (line 108)

Recommendation: Fix the critical bugs before merging. The PR should not be merged until the runtime errors are resolved.

Let me know if you would like me to help with any of these fixes!