Skip to content

Conversation

myftija
Copy link
Member

@myftija myftija commented Sep 22, 2025

Fixes small issue in the settings page which would cause outdated connected
project repos to be shown.

Copy link

changeset-bot bot commented Sep 22, 2025

⚠️ No Changeset found

Latest commit: e764726

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

Walkthrough

Adds constraints to the Prisma query in apps/webapp/app/services/projectSettingsPresenter.server.ts to only consider GitHub installations where deletedAt and suspendedAt are null when fetching the connected repository. No changes to the selection, mapping, or handling of the case when no repository is connected.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description is a single-sentence summary and does not follow the repository's required template: it omits the "Closes #" line, the ✅ Checklist, the Testing section with steps taken, a Changelog entry, and any Screenshots, so maintainers cannot link the change to an issue or verify testing performed. As written the description is incomplete for reviewers. Please update the PR description to include "Closes #" if applicable, complete the checklist items, add a Testing section describing the steps you ran to verify the fix, provide a short Changelog entry summarizing the change, and attach screenshots for the UI change if available.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix(webapp): hide outdated connected repos from deleted installations" is concise and accurately summarizes the primary change: excluding connected repositories tied to deleted or suspended GitHub installations, which matches the added filters on installation.deletedAt and installation.suspendedAt in apps/webapp/app/services/projectSettingsPresenter.server.ts. It clearly communicates the intent to teammates scanning history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-outdated-connected-repo-issue

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49728b5 and e764726.

📒 Files selected for processing (1)
  • apps/webapp/app/services/projectSettingsPresenter.server.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/services/projectSettingsPresenter.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/services/projectSettingsPresenter.server.ts
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/services/projectSettingsPresenter.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly

Files:

  • apps/webapp/app/services/projectSettingsPresenter.server.ts
apps/webapp/app/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead

Files:

  • apps/webapp/app/services/projectSettingsPresenter.server.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.

Applied to files:

  • apps/webapp/app/services/projectSettingsPresenter.server.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/services/projectSettingsPresenter.server.ts (1)

57-62: Correctly gate connected repo to active (non-deleted, non-suspended) installations — re-run verification

Good fix; aligns the connected-repo lookup with the installations list and prevents stale repos from deleted/suspended installations from appearing.

File: apps/webapp/app/services/projectSettingsPresenter.server.ts (lines 57–62)

            repository: {
              installation: {
                deletedAt: null,
                suspendedAt: null,
              },
            },

The earlier ripgrep run returned "No files were searched." Re-run these from the repository root (or run locally) to find other usages and confirm Prisma uniqueness constraints:

#!/bin/bash
set -euo pipefail

echo "1) Find other ConnectedGithubRepository queries that may need the same filter:"
rg --hidden --glob '!node_modules/**' -n -C2 -S 'connectedGithubRepository\.(findFirst|findMany)\(' || true

echo
echo "2) Verify Prisma models/constraints for connected repo uniqueness:"
rg --hidden --glob '!node_modules/**' -n -C3 -P 'model\s+ConnectedGithubRepository\b|@@unique\(|@unique\b|model\s+GithubAppInstallation\b|model\s+GithubRepository\b' || true

echo
echo "3) Spot GitHub installation queries to ensure consistent filtering across the app:"
rg --hidden --glob '!node_modules/**' -n -C2 -S 'githubAppInstallation\.find(Many|First)\(' || true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@matt-aitken matt-aitken merged commit 412e80f into main Sep 23, 2025
31 checks passed
@matt-aitken matt-aitken deleted the fix-outdated-connected-repo-issue branch September 23, 2025 12:25
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.

2 participants