Skip to content

Conversation

@joshribakoff
Copy link
Owner

Summary

  • Integrates @sailkit/atlas for Wikipedia-style magic links
  • Adds build-link-targets.ts to extract link targets from content collections
  • Configures remarkMagicLinks in astro.config.mjs
  • Updates postinstall to properly build sailkit workspaces

Usage

In any markdown content:

Check out [:context] for more info.
See [:context|:ctx] for fallback resolution.
[[context|Learn about context]] with custom display text.

Dependencies

  • Depends on: sailkit#7 (atlas implementation)

Test plan

  • Build passes with magic links plugin configured
  • Manual testing of magic link syntax in content

🤖 Generated with Claude Code

@vercel
Copy link

vercel bot commented Dec 15, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
fightingwithai Ready Ready Preview, Comment Jan 3, 2026 8:06pm

@claude
Copy link

claude bot commented Dec 20, 2025

Pull Request Review: Integrate @sailkit/atlas magic links

Overview

This PR integrates the @sailkit/atlas (now @bearing-dev/atlas) magic links plugin to replace a custom implementation with a simpler Wikipedia-style link syntax. The changes are mostly clean, but there are several important issues to address.


🔴 Critical Issues

1. Binary Files Committed (.DS_Store)

Files: .DS_Store, public/.DS_Store, public/audio/.DS_Store

Issue: macOS system files should not be committed to the repository.

Fix:

  • Remove these files from the commit
  • Add .DS_Store to .gitignore

2. Package Name Mismatch

File: astro.config.mjs:3

Issue: Import statement uses @bearing-dev/atlas but this package name does not appear to be published on npm. The dependency is a GitHub reference to bearing-dev/bearing-dev#atlas-magic-links.

Risk: This could break on fresh installs if the postinstall script fails or if the symlink structure changes.

Recommendation:

  • Verify the package is properly exported from the monorepo
  • Consider using the full path if symlinks are not reliable
  • Or ensure the package is published to npm under @bearing-dev/atlas

⚠️ Major Concerns

3. Complex Postinstall Script

File: package.json:13

Issue: The postinstall script is extremely fragile - it is a long one-liner that is hard to read and maintain, will break on Windows (symlinks require admin privileges), has no error handling, runs on every npm install (slowing CI/CD), and relies on specific directory structure.

Recommendation:

  • Split into a separate script file (e.g., scripts/setup-monorepo.js)
  • Add proper error handling
  • Use cross-platform symlink solutions or consider alternatives
  • Add documentation about platform requirements

4. Missing URL Builder Flexibility

File: astro.config.mjs:10

⚠️ Issue: The urlBuilder is hardcoded to /concepts/${id}/, but the documentation mentions the system should work across multiple collections (concepts, failure-modes, patterns, etc.).

Problem: Links to failure-modes or patterns will not work correctly.

Recommendation:

  • Implement a more sophisticated urlBuilder that determines the correct collection
  • Or document that this only works for concepts and plan to extend it later

📋 Code Quality & Best Practices

5. Import Path Changes Look Good

Files: src/layouts/Layout.astro:5-7, src/utils/collections.ts:18

The refactor from bearing-dev/packages/ to @bearing-dev/ improves readability and follows npm conventions.

6. Documentation Updates

File: .claude/skills/magic-links/SKILL.md

Good simplification of the documentation. The new syntax is clearer and more concise.

7. Configuration is Clean

File: astro.config.mjs

The Remark plugin configuration is straightforward and follows Astro conventions.


🧪 Test Coverage

Missing Tests

Test plan shows:

  • Build passes with magic links plugin configured
  • Manual testing of magic link syntax in content

Concerns:

  • No automated tests for the magic link transformation
  • No tests for edge cases (invalid IDs, special characters, nested brackets)
  • No validation that links resolve correctly
  • Previous system had scripts/validate-concept-ids.js - was this intentionally removed?

Recommendation:

  • Add unit tests for the magic link syntax transformation
  • Add integration tests verifying links work in actual content
  • Consider keeping or adapting the validation script to ensure links resolve

🔒 Security Concerns

No major security issues identified

The package is from a GitHub repository under the same organization, which reduces supply chain risk. However:

Minor concern: The postinstall script runs arbitrary code from the monorepo. Ensure the atlas-magic-links branch is trusted and review the build scripts.


🎯 Performance Considerations

Binary Audio Files

Files: 37 .wav files in public/audio/

⚠️ Issue: While not directly related to this PR, committing 37 audio files significantly increased the diff size (914 additions, 3038 deletions).

Observation: These appear to be new additions. Consider:

  • Using Git LFS for binary assets
  • Hosting audio files externally (CDN)
  • Compressing audio files if possible

Note: The package-lock.json changes are mostly removing resolved and integrity fields, which is expected when switching from npm registry to GitHub dependencies.


📝 Recommendations Summary

Must Fix (Before Merge):

  1. Remove .DS_Store files and update .gitignore
  2. Verify/fix package import path discrepancy
  3. Document or fix urlBuilder limitation for multiple collections

Should Fix (Before Merge):
4. Refactor postinstall script for maintainability and cross-platform support
5. Add automated tests or validation for magic links

Nice to Have (Follow-up PR):
6. Consider Git LFS for audio files
7. Add more comprehensive error handling in the urlBuilder
8. Document platform requirements (Linux/Mac due to symlinks)


✅ Approval Status

Conditional Approval - This PR is a good improvement but needs the critical issues addressed before merging.

The magic links feature simplifies the system nicely, but the implementation has some rough edges that could cause problems in production or for new contributors.

Great work on simplifying the documentation and reducing complexity! 🎉

@claude
Copy link

claude bot commented Dec 20, 2025

PR Review: Integrate @sailkit/atlas magic links

Summary

This PR migrates from a custom magic links implementation to @bearing-dev/atlas. Overall, the integration looks clean and simplifies the codebase significantly.

Code Quality & Best Practices

Strengths

  1. Simplification: Removing 54 lines of complex custom logic
  2. Clear documentation in updated SKILL.md
  3. Clean package structure

Issues Found

1. Package Name Mismatch (High Priority)

astro.config.mjs imports from '@bearing-dev/atlas' but SKILL.md documents '@sailkit/atlas'. Please update SKILL.md to match the actual implementation.

2. URL Builder Hardcoded Path

The urlBuilder is hardcoded to /concepts/ but the repo has multiple collection types. How will magic links work for non-concept pages? Consider documenting cross-collection link syntax.

Performance

  • Build time improvement from removing custom buildIdMap function
  • No runtime impact

Security

  • Low risk: Build-time only processing
  • Minor: Using branch reference instead of stable version

Test Coverage (Critical Issue)

The PR shows incomplete manual testing. Recommendations:

  1. Add automated tests for magic link transformation
  2. Test both syntaxes
  3. Test custom display text
  4. Verify links in code blocks are NOT transformed
  5. Complete manual testing

Must Fix Before Merge

  1. Fix package name in SKILL.md
  2. Complete manual testing
  3. Add automated tests
  4. Verify build works

Overall Assessment

Code Quality: 7/10
Risk Level: Medium
Recommendation: Request Changes

The concept is solid but needs test coverage and documentation fixes before merge.

joshribakoff and others added 5 commits January 3, 2026 11:41
Adds Wikipedia-style magic links for cross-referencing content.

- Update postinstall to properly build sailkit workspaces
- Add build-link-targets.ts to extract link targets from content
- Configure astro.config.mjs with remarkMagicLinks plugin

Usage in markdown:
- [:slug] - link to content by slug
- [:slug1|:slug2] - fallback IDs
- [[slug]] - wiki-style links
- [[slug|Custom Text]] - with display text

Depends on: sailkit#7

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create slug-resolver.ts for global slug lookup
- Throws on ambiguous slugs, requires collection prefix
- Convert all manual markdown links to [[wiki]] syntax
- Fix Layout.astro imports to use @bearing-dev/*

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Import createSlugResolver from atlas/node instead of local utility.
Simplifies site code by moving slug resolution into the library.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restored explicit display text for hyphenated slugs where the
original markdown had human-readable titles (e.g., "Agent loops"
instead of "agent-loops", "context collapse" instead of
"context-collapse").

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.

1 participant