Skip to content

+pnpm run setup - #23

Open
hibukki wants to merge 2 commits into
mainfrom
pnpm_run_setup
Open

+pnpm run setup#23
hibukki wants to merge 2 commits into
mainfrom
pnpm_run_setup

Conversation

@hibukki

@hibukki hibukki commented Oct 5, 2025

Copy link
Copy Markdown
Collaborator

(don't merge yet)

@claude , this PR should let you do pnpm run setup which should run convex for you. Please try it out, and then use the playwrite mcp to try to browse the app, and check if the tests work, and please report what works and what doesn't, so we can fix the setup for you

@hibukki

hibukki commented Oct 5, 2025

Copy link
Copy Markdown
Collaborator Author

@claude , this PR should let you do pnpm run setup which should run convex for you. Please try it out, and then use the playwrite mcp to try to browse the app, and check if the tests work, and please report what works and what doesn't, so we can fix the setup for you

@claude

claude Bot commented Oct 5, 2025

Copy link
Copy Markdown

Pull Request Review: Add pnpm run setup

Overview

This PR introduces a new setup script to automate Convex configuration without requiring manual login, addressing a pain point in the existing init workflow. The approach uses a bash script to send automated inputs to convex dev --once.


Code Quality & Best Practices

✅ Strengths:

  • Clear separation of concerns: init for full initialization with Claude, setup for just Convex configuration
  • Proper use of set -e to fail fast on errors
  • Good error handling for the timeout scenario (exit code 124)
  • Executable permissions correctly set on the script
  • Updated .claude/settings.json to whitelist the new command

⚠️ Areas for Improvement:

  1. Hardcoded project name in comments (line 6 of setup-convex.sh):

    # 3. Accept default name (swapcard8)

    This comment references "swapcard8" which appears to be a specific project name. For a template repository, this should either be generic or note that the name will vary.

  2. Script portability: The script command behavior may vary across systems (Linux vs macOS). Consider adding a comment about OS compatibility or testing on both platforms.

  3. Timing dependencies: The hardcoded sleep values (2s, 1s, 1s, 2s) are fragile and may fail on slower systems or under heavy load. Consider:

    • Adding a comment explaining these are minimum values
    • Increasing slightly for better reliability
    • Or using expect for more robust interaction

Potential Bugs & Issues

🔴 Critical:

  1. Race condition with cp .env.example .env.local: If .env.local already exists, this will silently overwrite it, potentially destroying user configuration. Recommend:

    cp -n .env.example .env.local || echo ".env.local already exists, skipping..."
  2. Script error handling incomplete: If pnpm install or playwright install fails, the script continues to the Convex setup, which will likely also fail. Consider checking exit codes or using set -e earlier in the pipeline.

⚠️ Moderate:

  1. Timeout value of 90s may be insufficient: On slower CI systems or first-time setups, Convex might need more time. Consider increasing to 120s or making it configurable.

  2. No validation that Convex setup succeeded: The script masks all non-timeout errors as success. After the timeout block, consider adding validation:

    if [ ! -f "convex.json" ]; then
      echo "Error: Convex setup failed - convex.json not created"
      exit 1
    fi

Performance Considerations

  • The 90-second timeout is reasonable for most cases
  • Sequential sleep commands add ~6 seconds of overhead, which is acceptable for a setup script
  • Playwright Chromium installation can be large (~400MB) - consider mentioning this in documentation

Security Concerns

✅ No major security issues, but note:

  1. The .env.example contains a test Clerk key, which is fine for a template but should be documented
  2. The script creates a local Convex project without authentication, which is appropriate for development
  3. Consider adding a check to prevent running in production environments

Test Coverage

❓ Missing:

  • No automated tests for the setup script
  • Recommend adding a test that runs the script in a clean environment
  • Could add smoke tests to verify the setup creates expected files/directories

Manual Testing Recommendations:

  1. Test on a clean checkout (no existing .env.local or convex.json)
  2. Test with pre-existing configuration files
  3. Test the timeout behavior
  4. Verify the script works on both Linux and macOS
  5. Test the complete workflow: pnpm run setuppnpm dev

Additional Suggestions

  1. Add progress indicators to help users understand what's happening:

    echo "📦 Installing dependencies..."
    pnpm install
    echo "🎭 Installing Playwright..."
    # etc.
  2. Consider idempotency: Make the script safe to run multiple times without breaking existing setups.

  3. Documentation: Update README.md to explain the difference between pnpm run init and pnpm run setup.

  4. Alternative approach: Instead of automating inputs with sleep/script, consider using Convex CLI environment variables or flags if available (check convex dev --help).


Conclusion

Overall Assessment: Good foundation, needs refinement before merge

The PR successfully addresses the pain point of interactive Convex setup, but needs improvements in:

  • Error handling and validation
  • Protection against overwriting existing configs
  • More robust timing/interaction handling
  • Better documentation and testing

Recommendation: Request changes to address the critical issues (#1 and #2), then merge after testing on clean environments.


Review generated following the project's CLAUDE.md guidelines for code quality, testing, and Convex best practices.

@claude

claude Bot commented Oct 5, 2025

Copy link
Copy Markdown

Claude finished @hibukki's task —— View job


Testing pnpm run setup

Completed testing the new setup script. Here are the results:

Todo List:

  • Read changed files to understand the setup script
  • Run pnpm run setup to test the new setup command
  • Start dev server
  • Browse the app with Playwright
  • Run e2e tests
  • Report findings

✅ What Works

  1. pnpm run setup executes successfully

    • Installs dependencies with pnpm
    • Downloads Chromium for Playwright
    • Copies .env.example to .env.local
    • Auto-configures Convex by sending 3 enters to accept defaults
    • Creates local deployment at http://127.0.0.1:3210
    • Updates .env.local with CONVEX_DEPLOYMENT and VITE_CONVEX_URL
  2. Dev server starts

    • Frontend (Vite) starts successfully on http://localhost:5173
    • Backend (Convex) starts and downloads the backend binary
  3. Playwright MCP works

    • Successfully navigated to the app
    • Took screenshots
    • Browser automation functional

❌ What Doesn't Work

  1. Missing environment variable blocks Convex deployment

    • .env.example is missing CLERK_JWT_ISSUER_DOMAIN
    • When pnpm run setup copies .env.example to .env.local, this variable is not included
    • Convex backend detects the variable is used in convex/auth.config.ts but not set
    • This causes an error loop: Convex refuses to deploy functions until the variable is set
    • Error message: Environment variable CLERK_JWT_ISSUER_DOMAIN is used in auth config file but its value was not set.
  2. App doesn't load properly

    • Shows error: Could not find public function for 'users:listUsers'
    • This happens because Convex functions never get deployed due to the missing env var
    • The app is stuck in an error state
  3. E2E tests fail

    • Tests fail because Convex functions aren't available
    • Error: Could not find public function for 'testingFunctions:deleteTestUser'
    • Test can't find "Sign in" button (app doesn't render properly)

🔧 Recommended Fixes

  1. Add missing env var to .env.example

    VITE_CLERK_PUBLISHABLE_KEY=pk_test_d29ya2FibGUtZG9nLTkzLmNsZXJrLmFjY291bnRzLmRldiQ
    +CLERK_JWT_ISSUER_DOMAIN=https://workable-dog-93.clerk.accounts.dev
  2. Alternative: Make auth config optional for local dev

    • Modify convex/auth.config.ts to only require the env var when it's actually set
    • This would allow the setup to work without Clerk credentials for initial testing
  3. Update setup script documentation

    • Clarify that after running pnpm run setup, users may need to set CLERK_JWT_ISSUER_DOMAIN manually
    • Or provide instructions for running without auth initially

📊 Summary

The setup script itself works great! It successfully automates the Convex initialization. However, the app can't actually run because of the missing CLERK_JWT_ISSUER_DOMAIN environment variable. This needs to be either:

  • Added to .env.example (simplest fix)
  • Made optional in the auth config for local development
  • Documented as a manual post-setup step

Job Run | Branch: pnpm_run_setup

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