Skip to content

Fix resume link path to resolve 404 error#19

Merged
calvindotsg merged 1 commit intomainfrom
claude/issue-18-20250824-1307
Aug 24, 2025
Merged

Fix resume link path to resolve 404 error#19
calvindotsg merged 1 commit intomainfrom
claude/issue-18-20250824-1307

Conversation

@calvindotsg
Copy link
Owner

Fixes #18

This PR resolves the broken resume link that was returning a 404 error.

Changes

  • Updated resume link from relative path ../assets/Calvin_Loh_Solutions_Engineer_Resume.pdf to absolute path /src/assets/Calvin_Loh_Solutions_Engineer_Resume.pdf
  • This ensures proper asset handling in Astro

Testing

  • Verified the resume file exists at the specified location
  • Updated configuration in src/lib/constants.ts

Generated with Claude Code

- Changed resume link from '../assets/Calvin_Loh_Solutions_Engineer_Resume.pdf' to '/src/assets/Calvin_Loh_Solutions_Engineer_Resume.pdf'
- This uses absolute path for proper Astro asset handling

Co-authored-by: Calvin <calvindotsg@users.noreply.github.com>
@claude
Copy link

claude bot commented Aug 24, 2025

Claude finished @calvindotsg's task —— View job


PR Review: Fix resume link path to resolve 404 error

Todo List

  • Read the changed file (src/lib/constants.ts) to understand the context
  • Verify the resume file exists at the specified location
  • Analyze if the path change is correct for Astro's asset handling
  • Review the broader constants file for any related issues
  • Provide comprehensive review feedback

Review Feedback

❌ Critical Issue - Incorrect Asset Path

Problem: The proposed path /src/assets/Calvin_Loh_Solutions_Engineer_Resume.pdf is incorrect for Astro's asset handling and will likely still result in a 404 error.

Analysis:

  1. File exists: ✅ The resume file exists at src/assets/Calvin_Loh_Solutions_Engineer_Resume.pdf
  2. Asset handling pattern: ❌ The current approach doesn't follow Astro's asset handling best practices
  3. Comparison with working assets: Looking at IntroCard.astro:8, images are imported using import portfolioImage from '../assets/me.webp' and then referenced via the imported variable

🔧 Recommended Solution

Option 1: Import-based approach (Recommended)

// At the top of constants.ts
import resumePdf from '../assets/Calvin_Loh_Solutions_Engineer_Resume.pdf';

// In the LINKS array
{
    link: resumePdf, 
    logo: "ri:file-pdf-2-line", 
    name: "Resume"
}

Option 2: Public folder approach

  • Move the PDF to public/resume.pdf
  • Reference it as /resume.pdf

📋 Additional Review Points

Code Quality: ✅ Good

  • Constants are well-structured and typed
  • Consistent naming conventions
  • Clear organization of data

Security: ✅ No issues

  • No sensitive data exposed
  • Safe file references

Performance: ⚠️ Minor consideration

  • PDFs in assets folder will be processed by Vite
  • Consider if the PDF should be in public/ instead for direct serving

Best Practices: ❌ Needs improvement

  • Current path doesn't align with Astro's asset handling conventions
  • Mixed approaches between images (imported) and PDF (hardcoded path)

🧪 Test Coverage

The PR description mentions verification, but the current path approach is fundamentally flawed for Astro's build process.

💡 Next Steps

  1. Choose between import-based or public folder approach
  2. Update the path accordingly
  3. Test the fix in development and production builds
  4. Consider adding a comment explaining the asset handling choice

@netlify
Copy link

netlify bot commented Aug 24, 2025

Deploy Preview for calvindotsg ready!

Name Link
🔨 Latest commit 219cd5e
🔍 Latest deploy log https://app.netlify.com/projects/calvindotsg/deploys/68ab0f823fed920008b25540
😎 Deploy Preview https://deploy-preview-19--calvindotsg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 87
Accessibility: 100
Best Practices: 75
SEO: 100
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@calvindotsg calvindotsg merged commit d4c356a into main Aug 24, 2025
4 of 5 checks passed
@calvindotsg calvindotsg deleted the claude/issue-18-20250824-1307 branch August 25, 2025 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix resume link error 404

1 participant