Coding Practices from Team Discussions

This document captures coding practices, standards, and decisions that emerged from team discussions. These represent our current understanding and should be integrated with existing coding standards documentation.

Pull Request Review Process

PR Review Requirements

Link issues to PRs: All PRs should have linked issues (visible via the “bullseye” icon in GitHub)
Include ticket numbers in commits: Every commit should include the ticket number
PR descriptions: Should be clear and concise, with accompanying documentation (PR comments, walkthrough, snapshots, demos) commensurate with the complexity of the change
Shortcuts listed: Any shortcuts or workarounds should be listed and justified in the PR

Code Review Tools

We use multiple AI code review tools:

Claude Code (via GitHub app): Can be set up with /install-github-app in a repo. Provides full control over prompts to enforce coding/architecture conventions specific to the project.
CodeRabbit: Automated code review
Korbit: Automated code review (may be phased out in favor of Claude)

Note: AI reviewers can add many comments, and non-deterministic behavior means running them again may produce different suggestions. These can be ignored with comments if not relevant.

Large Code Reviews

For large code reviews, the team has discussed:

AI “summaries” received skepticism
Team appreciates clear PR context and documentation
Reviewers should ask “How is this supposed to work?” when something is unclear

Review Channels

Dedicated channel: #tech_pull-requests for PR review requests
Not in release channel: PR reviews are NOT release-specific and shouldn’t clutter the release coordination channel

Testing Practices

Docker and Laravel Sail

Standard Approach: Use Laravel Sail for local development and testing.

Using Sail: Must use Sail commands: sail test, sail npm run dev, sail artisan migrate
Using local stack: Must use regular commands: php artisan test, npm run dev, php artisan migrate
Don’t mix modes: Mixing Sail and local commands produces unexpected behavior (databases being wiped, .env.testing being ignored)

Key Principle: Never add layers on top of Sail or attempt to override its behavior. Sail already handles:

CI browser tests
TDD and browser tests
HMR (hot module reloading)
Separate docker containers for tests

Testing Environment Configuration

.env.testing: Should be in .gitignore (not committed)
.env.testing.example: Should exist as a template
Database setup: Use scripts/setup_db.sh --testing to create test databases and users
Alternative setups: Some developers use bare metal testing (mapping DB port from Sail) for faster performance on M1 Macs

Browser Tests

Browser tests are configured to work in multiple environments:

Running the app locally in docker-compose
Running individual browser tests locally in docker-compose
Running all tests in GitHub CI Pipeline
Running the app in Fly.io environment

The main challenge was configuring the Vite server correctly for all these scenarios.

GitHub Enterprise

Migration Notes

We migrated to GitHub Enterprise for Startups. Key changes:

Email notifications: If you rely on email notifications from GitHub, add your c15.io email to your GitHub account
CI workflow permissions: After upgrading, CI workflows need explicit permissions

CI Workflow Permissions

For GitHub Enterprise, update all workflow files in .github/workflows/ that use:

actions/github-script to post comments on PRs/issues
mikepenz/action-junit-report or similar to publish test reports
Docker services pulling from ghcr.io/cadenceonefive/

Add these permissions at the workflow level (after on:, before jobs:):

permissions:
  contents: read
  packages: read
  issues: write
  pull-requests: write
  checks: write

For steps that post PR comments using actions/github-script, add continue-on-error: true so the job doesn’t fail if commenting fails:

- name: Post coverage comment
  continue-on-error: true
  uses: actions/github-script@v7

API Key Management

Google Cloud API Keys

Previous Issues: We were using unrestricted Google Cloud API keys, shared across prod/staging/dev, with unclear names (bad practice).

Current Approach:

Created separate, restricted API keys for different environments
Local dev key: Restricted to a small subset of APIs we actually use
Key location: See Doppler dev config (key ends in ...6FY*g instead of ...GhzY)
For calc service: Use the same key in GOOGLE_API_KEY

Security Considerations:

API restrictions: Restricted APIs but not domains (leaves a gap since Street View image URLs include the API key)
Plan: Leave dev key unrestricted on URLs, but tighten staging and prod
Note: Google’s terms of service prohibit caching of Street View images (with rare exceptions), so we can’t proxy them to protect user data

Development Environment

Branch Strategy

Current Structure:

main → release (always locked) - deployed at momentum-staging
dev → main (never locked, default branch) - deployed at momentum-development

Branch Naming:

Include ticket number in branch name
Branch from main for new features
Use release branch for release candidates

Commit Standards

Include ticket numbers: Every commit should reference the ticket number
Clear messages: Commit messages should be descriptive

Development Workflow

Before starting feature work:

For “feature” size tickets: Call an ad hoc meeting or make a Slack post before starting
Small tickets and bugs: Fine to just start

PR Process:

Link issues to PRs
Include clear descriptions with context
Add documentation, screenshots, or demos as needed
List any shortcuts or workarounds

Architecture and Refactoring

DDD (Domain-Driven Design) Refactoring

When doing DDD refactoring:

We need to set directions and standards moving forward
All team members must agree on how to do our version of DDD
DDD is not universal - it’s a set of principles applied differently per project/techstack
Review DDD refactoring PRs carefully to establish patterns for future work

Scope Builder Refactoring

Key considerations for refactoring the Scope Builder:

Map data dependencies between components using Wirespy
Refactor Livewire components into Blade components using a modelable approach
Consider building scopes and warming up tagged caches
Dispatch data updates via events only to avoid heavy DOM morphs
Refactor into a structure that’s easy to mock for tests

PHP and Technology Choices

PHP 8 Evolution

The team has noted that PHP 7 → PHP 8 was a significant modernization jump, adding:

Full type system maturation
Union types, attributes, enums
Nullsafe operator, JIT
Readonly properties, constructor promotion
Fibers (async building block)
Massive engine speed upgrades

Philosophy: Technologies are used because they fit the solution (PHP for web, Python for heavy calculations), not for novelty.

Additional Practices

Documentation

README files: Consider README files in every folder explaining the purpose (similar to Slack channel descriptions)
PR documentation: Include documentation updates in PRs when relevant
Wiki updates: Update wiki/documentation when making significant changes to setup or processes

Communication

Testing setup changes: Communicate testing setup changes to the team before merging
Breaking changes: Add clear notices (e.g., “review, but do not merge”) if PRs aren’t ready
Database changes: Coordinate database changes, especially large SQL statements - consider pairing and backups

Code Quality

Remove unused code: Clean up unnecessary code when possible
Test coverage: Consider test coverage, especially for “hotspots” where bugs might hide
Code clarity: If a reviewer doesn’t understand code and can’t maintain it, don’t approve

This document is a synthesis of team discussions. For official standards, see the main Coding Standards documentation.