Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a managed reverse-proxy feature: DB migration and TypeORM entity, backend service with DNS/TLS verification and edge gating, controllers/DTOs and guard, scheduled verification tasks, frontend settings UI and routes, docs, localization, and client models/types. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as ProxyDomainService
participant DB as Database
participant DNS as DNS Resolver
participant TLS as TLS Probe (443)
participant Redis
Client->>Service: request verify (domain id)
activate Service
Service->>DB: load domain record
DB-->>Service: domain (status,timestamps)
Service->>DNS: resolve CNAME (bounded timeout)
activate DNS
DNS-->>Service: resolved / not resolved
deactivate DNS
alt CNAME resolved
Service->>TLS: probe TLS on hostname:443 (timeout)
activate TLS
TLS-->>Service: cert ok / fail / timeout
deactivate TLS
alt TLS ok
Service->>DB: set status → LIVE, set liveSince/statusChangedAt
Service->>Redis: purge related cert cache (best-effort)
else TLS fail & grace < 30m
Service->>DB: update lastCheckedAt, record errorMessage
else TLS fail & elapsed ≥ 30m
Service->>DB: set status → ERROR, set errorMessage/statusChangedAt
end
else CNAME missing & grace < 7d
Service->>DB: update lastCheckedAt
else CNAME missing & elapsed ≥ 7d
Service->>DB: set status → ERROR, set errorMessage/statusChangedAt
end
Service-->>Client: return updated domain record
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/project/proxy-domain.service.ts`:
- Around line 388-412: Both queries are missing a stable ordering so batches can
starve rows; update findPendingForVerification and findLiveForRecheck to sort by
oldest check time (lastCheckedAt) and then by a stable tie-breaker (id). For
findPendingForVerification (method name) pass an order option to the repository
find call ordering lastCheckedAt ASC (NULLs first) and id ASC; for
findLiveForRecheck (method name) add an orderBy('pd.lastCheckedAt', 'ASC') and
addOrderBy('pd.id','ASC') on the query builder (ensure NULLs come first if your
DB supports it) so the scheduler processes the oldest-unchecked rows first and
ordering is deterministic.
- Around line 153-186: The create method can race because multiple callers can
pass proxyDomainRepository.count() then save; wrap the existence/count check and
save in a database transaction or row-level lock to serialize per-project domain
creation (e.g., beginTransaction, SELECT FOR UPDATE on the project row or a
dedicated lock table key for projectId), re-check the count and existing
hostname inside the transaction, then create/save the ProxyDomain entity (use
generateProxyTargetId, proxyDomainRepository.create/save) and commit; also keep
a unique DB constraint on hostname and translate duplicate-key/unique-constraint
DB errors thrown by proxyDomainRepository.save into ConflictException so
concurrent duplicate races remain surfaced as the same ConflictException.
- Around line 59-72: The validateHostname function currently uses a regex
replace on user-controlled input (hostname =
raw.trim().toLowerCase().replace(/\.+$/, '')) which CodeQL flagged; change
validateHostname to remove trailing dots without using regex by iteratively
trimming up to a small bounded number of characters (e.g., while the last
character is '.' and loop count < 10 remove the last char) after
trim().toLowerCase(), and then continue the existing checks (empty, length >
253) — reference validateHostname to locate and replace the .replace(/\.+$/, '')
step with a simple bounded loop that strips trailing '.' characters.
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 2158-2186: The verifyPendingProxyDomains cron can overlap and
re-run while prior work is still in flight causing duplicate probes; add an
in-memory re-entrancy guard (e.g., a boolean property this.verifyingPending) at
the start of verifyPendingProxyDomains so the method returns immediately if
already running, set it true before processing and false in a finally block;
additionally change/follow up on findPendingForVerification to support atomic
claiming (mark rows as VERIFYING or implement short locks so overlapping runs
skip claimed domains) and wrap any dns.resolveCname calls used during
verifyDomain with a hard timeout (e.g., Promise.race with a 10s timer) to avoid
hanging DNS resolution.
In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx`:
- Line 585: The submitText is using t('common.add') which is not defined in the
translations; either add an "add": "Add" entry under the common namespace in
your English locale JSON or switch the call in ProxyDomainsTab.tsx to an
existing proxy-specific key (e.g., t('proxy.addDomain')) and add that key to the
locale file; update the translation JSON accordingly so the button renders a
proper label instead of the raw key.
- Around line 95-103: The catch block inside the onCopy async function uses a
hard-coded English string for the failure toast; replace the literal 'Failed to
copy' with the localized key by calling the translation function (t) — e.g., use
toast.error(t('common.failedToCopy')) — so the error toast uses i18n like the
success path (reference: onCopy, navigator.clipboard.writeText, toast.error, t).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43d9fca9-41ab-4dfe-8d93-c0453bb06356
📒 Files selected for processing (17)
backend/apps/cloud/src/project/dto/index.tsbackend/apps/cloud/src/project/dto/proxy-domain-create.dto.tsbackend/apps/cloud/src/project/entity/index.tsbackend/apps/cloud/src/project/entity/proxy-domain.entity.tsbackend/apps/cloud/src/project/project.module.tsbackend/apps/cloud/src/project/proxy-domain.controller.tsbackend/apps/cloud/src/project/proxy-domain.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/migrations/mysql/2026_04_17_managed_proxy_domains.sqldocs/content/docs/adblockers/how-to-deal-with-adblockers.mdxdocs/content/docs/adblockers/managed-proxy.mdxdocs/content/docs/adblockers/meta.jsonweb/app/lib/models/Project.tsweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/components/ProxyDomainsTab.tsxweb/app/routes/projects.settings.$id.tsxweb/public/locales/en.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
2156-2240: Past feedback addressed; consider hoisting the new guard fields to the top of the class.The in-memory re-entrancy guards (
verifyingPendingProxyDomains,recheckingLiveProxyDomains) correctly wrap the work intry/finally, so the flag is always reset even on earlyreturn/ throw. Combined withisPrimaryNode()gating and theresolveCnameWithTimeout/ TLS timeout on the service side, the previous overlapping-cron concern is resolved.One small style nit: declaring private fields in the middle of the class body (between
deleteOldRefreshTokensandverifyPendingProxyDomains) is unconventional and easy to miss when scanning. Consider moving them next to the other class-level state near the top ofTaskManagerService.♻️ Suggested field placement
`@Injectable`() export class TaskManagerService { + // In-memory re-entrancy guards for the proxy-domain verifier crons. + // A single slow DNS/TLS probe can stretch a tick past the cron interval; + // without these flags the next tick would re-fire duplicate probes for + // everything still in flight. + private verifyingPendingProxyDomains = false + private recheckingLiveProxyDomains = false + constructor( private readonly mailerService: MailerService, ...And remove the declarations at lines 2156-2161.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/task-manager/task-manager.service.ts` around lines 2156 - 2240, Move the two in-memory re-entrancy guard fields (verifyingPendingProxyDomains and recheckingLiveProxyDomains) out of the middle of the class and hoist them to the top of TaskManagerService next to other class-level state; remove the original declarations between deleteOldRefreshTokens and verifyPendingProxyDomains, and keep the rest of the logic (the try/finally guards in verifyPendingProxyDomains and recheckLiveProxyDomains) unchanged so behavior is preserved.web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx (1)
182-200: Polling interval is reset on every render, and theelse ifbranch is unreachable.
fetchDomainsis recreated on every render because it closes overlistFetcher, whichuseFetcherreturns as a new object reference each render. As a result, this effect re-runs on every render: the cleanup tears down the interval and a newsetInterval(fetchDomains, 8000)is set up — so under frequent re-renders the 8s timer can keep being reset and never fire. Additionally, theelse if (pollRef.current)branch is dead code: the previous effect's cleanup has already cleared and nulledpollRef.currentbefore this effect body executes.Depend only on the polling condition and call the latest
fetchDomainsvia a ref.♻️ Suggested refactor
+ const fetchDomainsRef = useRef(fetchDomains) + useEffect(() => { + fetchDomainsRef.current = fetchDomains + }, [fetchDomains]) + + const hasPending = domains.some( + (d) => d.status === 'waiting' || d.status === 'issuing', + ) + useEffect(() => { - const hasPending = domains.some( - (d) => d.status === 'waiting' || d.status === 'issuing', - ) - - if (hasPending) { - pollRef.current = setInterval(fetchDomains, 8000) - } else if (pollRef.current) { - clearInterval(pollRef.current) - pollRef.current = null - } - - return () => { - if (pollRef.current) { - clearInterval(pollRef.current) - pollRef.current = null - } - } - }, [domains, fetchDomains]) + if (!hasPending) return + const id = setInterval(() => fetchDomainsRef.current(), 8000) + return () => clearInterval(id) + }, [hasPending])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx` around lines 182 - 200, The useEffect that manages polling re-runs every render because fetchDomains changes each render (it closes over listFetcher), causing the interval to be torn down and re-created so the 8s timer can be reset; also the else-if branch is effectively unreachable because cleanup already nulls pollRef.current. Fix by making the effect depend only on the polling condition (domains) and not fetchDomains: keep pollRef and store the latest fetchDomains in a ref (e.g., fetchDomainsRef.current) so the interval callback calls the up-to-date fetchDomains, and remove the else-if branch; update useEffect to set/clear interval based on hasPending only and use fetchDomainsRef inside the interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx`:
- Around line 378-383: The hardcoded English string "Live since" in the JSX that
renders domain.liveSince should be replaced with an i18n key (e.g.
project.settings.proxy.liveSince) and the date passed as an interpolation;
update the Text rendering inside ProxyDomainsTab (the block referencing
domain.liveSince and dayjs(domain.liveSince)) to call the translation function
(t) with the key and a value like { date:
dayjs(domain.liveSince).locale(i18n.language).format('lll') } or equivalent
locale-aware formatting so the label is localized and the formatted date is
injected via interpolation.
- Around line 251-256: The validation error currently sets hostnameError to
t('project.settings.proxy.domainPlaceholder') inside submitAddDomain when
validateHostnameClient fails; replace that placeholder string with a dedicated
error key (e.g. t('project.settings.proxy.invalidDomain')) and update the
translations accordingly so setHostnameError uses the new error copy instead of
the placeholder; ensure you modify submitAddDomain (where validateHostnameClient
is checked) and any UI that displays hostnameError to show the new message.
---
Nitpick comments:
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 2156-2240: Move the two in-memory re-entrancy guard fields
(verifyingPendingProxyDomains and recheckingLiveProxyDomains) out of the middle
of the class and hoist them to the top of TaskManagerService next to other
class-level state; remove the original declarations between
deleteOldRefreshTokens and verifyPendingProxyDomains, and keep the rest of the
logic (the try/finally guards in verifyPendingProxyDomains and
recheckLiveProxyDomains) unchanged so behavior is preserved.
In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx`:
- Around line 182-200: The useEffect that manages polling re-runs every render
because fetchDomains changes each render (it closes over listFetcher), causing
the interval to be torn down and re-created so the 8s timer can be reset; also
the else-if branch is effectively unreachable because cleanup already nulls
pollRef.current. Fix by making the effect depend only on the polling condition
(domains) and not fetchDomains: keep pollRef and store the latest fetchDomains
in a ref (e.g., fetchDomainsRef.current) so the interval callback calls the
up-to-date fetchDomains, and remove the else-if branch; update useEffect to
set/clear interval based on hasPending only and use fetchDomainsRef inside the
interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fe723ca-c5a2-44ed-9a63-44e4285d1127
📒 Files selected for processing (5)
backend/apps/cloud/src/project/proxy-domain.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsdocs/content/docs/adblockers/how-to-deal-with-adblockers.mdxweb/app/pages/Project/Settings/components/ProxyDomainsTab.tsxweb/public/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/cloud/src/project/proxy-domain.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/cloud/src/project/proxy-domain.controller.ts (1)
39-128: Optional: collapse the repeated project-load + authz boilerplate.The 4-line prelude (
isValidPID→getFullProject→NotFoundException→allowedToManage) is copy-pasted acrosslist,create,remove, andverify. A single private helper (or a param decorator / NestJS guard keyed on:pid) would centralize it, reduce drift risk (e.g., someone later forgetting theallowedToManagecall in a new handler), and keep each handler focused on its own logic.♻️ Example helper extraction
+ private async loadManageableProject(pid: string, uid: string) { + if (!isValidPID(pid)) { + throw new BadRequestException('The provided project ID is incorrect') + } + const project = await this.projectService.getFullProject(pid) + if (!project) { + throw new NotFoundException('Project not found') + } + this.projectService.allowedToManage(project, uid) + return project + }Each handler then becomes e.g.:
async list(`@Param`('pid') pid: string, `@CurrentUserId`() uid: string) { - if (!isValidPID(pid)) { - throw new BadRequestException('The provided project ID is incorrect') - } - - const project = await this.projectService.getFullProject(pid) - if (!project) { - throw new NotFoundException('Project not found') - } - this.projectService.allowedToManage(project, uid) - + await this.loadManageableProject(pid, uid) const domains = await this.proxyDomainService.listForProject(pid) return { domains, proxyBaseDomain: PROXY_BASE_DOMAIN } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/project/proxy-domain.controller.ts` around lines 39 - 128, Extract the repeated PID validation, project load and authorization into a single private helper (e.g. ensureProjectManageable or getProjectForManage) that encapsulates isValidPID(pid) -> throw BadRequestException, await this.projectService.getFullProject(pid) -> throw NotFoundException, and this.projectService.allowedToManage(project, uid); have it return the loaded project (or throw) and replace the duplicated four-line prelude in list, create, remove, and verify with a single await this.ensureProjectManageable(pid, uid) call; alternatively note you can implement the same via a custom param decorator or NestJS guard keyed on :pid, but keep handlers using the helper name above so their bodies only handle proxyDomainService calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/project/proxy-domain.controller.ts`:
- Around line 131-185: The public edge endpoints in ProxyDomainEdgeController
(methods allow and active) leak customer state and are unauthenticated and
unthrottled; update these handlers to require an edge-auth header or other guard
(e.g., validate a shared-secret header against config or attach a custom Nest
guard) before calling validateHostname and
proxyDomainService.isHostnameAllowedForIssuance / isHostnameLive, collapse
success/failure semantics so callers cannot distinguish states (return a generic
404 or always return {allowed:false}/{active:false} on failure without distinct
messages), and add a rate-limiting guard (e.g., ThrottlerGuard) or document the
reliance on the edge cache to prevent direct DB hits via
findActiveDomainByHostname.
---
Nitpick comments:
In `@backend/apps/cloud/src/project/proxy-domain.controller.ts`:
- Around line 39-128: Extract the repeated PID validation, project load and
authorization into a single private helper (e.g. ensureProjectManageable or
getProjectForManage) that encapsulates isValidPID(pid) -> throw
BadRequestException, await this.projectService.getFullProject(pid) -> throw
NotFoundException, and this.projectService.allowedToManage(project, uid); have
it return the loaded project (or throw) and replace the duplicated four-line
prelude in list, create, remove, and verify with a single await
this.ensureProjectManageable(pid, uid) call; alternatively note you can
implement the same via a custom param decorator or NestJS guard keyed on :pid,
but keep handlers using the helper name above so their bodies only handle
proxyDomainService calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cec4251-a4fb-42a4-97de-b96d0de6301e
📒 Files selected for processing (1)
backend/apps/cloud/src/project/proxy-domain.controller.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx (1)
157-202: AssignfetchDomainsRef.currentinside an effect, not during render.Line 164 mutates the ref as a render side-effect, which is not safe under StrictMode double-invocation or concurrent rendering. The polling interval captures
fetchDomainsRef.currentat interval-tick time, so writing the ref inuseLayoutEffect(so the update is visible before the poll effect's cleanup/resubscribe) is both sufficient and idiomatic.♻️ Proposed fix
- const fetchDomains = useCallback(() => { + const fetchDomains = useCallback(() => { listFetcher.submit( { intent: 'list-proxy-domains' }, { method: 'POST', action: settingsAction }, ) }, [listFetcher, settingsAction]) - fetchDomainsRef.current = fetchDomains + useLayoutEffect(() => { + fetchDomainsRef.current = fetchDomains + }, [fetchDomains])(and add
useLayoutEffectto thereactimport)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx` around lines 157 - 202, Move the assignment fetchDomainsRef.current = fetchDomains out of render and into a useLayoutEffect so the ref is updated as a layout side-effect before any polling effects run; import useLayoutEffect from react, create a useLayoutEffect that sets fetchDomainsRef.current = fetchDomains and cleans up if needed, leaving fetchDomains as the same useCallback and keeping the poll effect that reads fetchDomainsRef.current (pollRef and the setInterval logic remain unchanged).backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
2164-2241: Re-entrancy guards andisPrimaryNodegate look good.The
try/finallypattern correctly releases the flag on every exit path (including the_isEmptyshort‑circuits), which addresses the overlap concern from the earlier review. One operational note for later:
recheckLiveProxyDomainscaps at 500 every 6h, so once live domains exceed ~500 the effective recheck interval stretches to 12h+ and a broken CNAME/cert could go unnoticed for that long. Worth monitoring and/or making the batch size / cadence configurable as adoption grows.- This file still depends on
proxyDomainService.verifyDomainwrappingdns.resolveCname/ TLS probes with hard timeouts; without that, a single hanging DNS server can still hold the guard for minutes and starve subsequent ticks. That fix lives inproxy-domain.service.ts(out of scope for this hunk).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/task-manager/task-manager.service.ts` around lines 2164 - 2241, The cron handlers work but need operational configurability: make the batch sizes and cadence configurable instead of hardcoding 200/500 and CronExpression.EVERY_MINUTE / EVERY_6_HOURS so you can tune them as adoption grows; update verifyPendingProxyDomains and recheckLiveProxyDomains to read e.g. config.get('proxyDomain.batchSize') and config.get('proxyDomain.recheckIntervalCron') (or similar keys) and pass the configurable batch size into proxyDomainService.findPendingForVerification and findLiveForRecheck, and replace the fixed CronExpression constants with a value loaded from configuration; also ensure proxyDomainService.verifyDomain implements strict DNS/TLS timeouts (the verifyDomain method is the relevant symbol) so a single slow resolver cannot hang the cron guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx`:
- Around line 281-519: The snippet UI only uses liveDomain = useMemo(() =>
domains.find(...)) which hides additional live domains; change this to collect
all live domains (e.g. liveDomains = useMemo(() => domains.filter(d => d.status
=== 'live'), [domains])) and then either (A) map over liveDomains to render a
snippet card per domain (reusing CopyableCode and projectId) or (B) add a small
selector state (e.g. selectedLiveDomain via useState) that lets the user pick
one hostname from liveDomains before rendering the CopyableCode blocks; update
usages of liveDomain to use the selected item or each item in the map and ensure
the script/init strings still use domain.hostname.
---
Nitpick comments:
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 2164-2241: The cron handlers work but need operational
configurability: make the batch sizes and cadence configurable instead of
hardcoding 200/500 and CronExpression.EVERY_MINUTE / EVERY_6_HOURS so you can
tune them as adoption grows; update verifyPendingProxyDomains and
recheckLiveProxyDomains to read e.g. config.get('proxyDomain.batchSize') and
config.get('proxyDomain.recheckIntervalCron') (or similar keys) and pass the
configurable batch size into proxyDomainService.findPendingForVerification and
findLiveForRecheck, and replace the fixed CronExpression constants with a value
loaded from configuration; also ensure proxyDomainService.verifyDomain
implements strict DNS/TLS timeouts (the verifyDomain method is the relevant
symbol) so a single slow resolver cannot hang the cron guard.
In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx`:
- Around line 157-202: Move the assignment fetchDomainsRef.current =
fetchDomains out of render and into a useLayoutEffect so the ref is updated as a
layout side-effect before any polling effects run; import useLayoutEffect from
react, create a useLayoutEffect that sets fetchDomainsRef.current = fetchDomains
and cleans up if needed, leaving fetchDomains as the same useCallback and
keeping the poll effect that reads fetchDomainsRef.current (pollRef and the
setInterval logic remain unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2c85795-4936-4cea-9432-3212c4fff2fb
📒 Files selected for processing (3)
backend/apps/cloud/src/task-manager/task-manager.service.tsweb/app/pages/Project/Settings/components/ProxyDomainsTab.tsxweb/public/locales/en.json
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/apps/cloud/src/project/proxy-domain.controller.ts (1)
37-126: Repeated pid-validation / project-load / authorization preamble — consider extracting.Each of
list,create,remove, andverifyopens with the same 8-line block:isValidPID→getFullProject→NotFoundException→allowedToManage. Pulling this into a small private helper (e.g.loadManageableProject(pid, uid)) would remove four near-identical copies and make it harder for future endpoints to forget one of the three checks.♻️ Sketch
private async loadManageableProject(pid: string, uid: string) { if (!isValidPID(pid)) { throw new BadRequestException('The provided project ID is incorrect') } const project = await this.projectService.getFullProject(pid) if (!project) { throw new NotFoundException('Project not found') } this.projectService.allowedToManage(project, uid) return project }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/project/proxy-domain.controller.ts` around lines 37 - 126, Extract the repeated pid validation, project load and authorization logic used in list, create, remove and verify into a private helper (e.g. loadManageableProject) that takes (pid: string, uid: string), runs isValidPID, calls this.projectService.getFullProject(pid), throws BadRequestException/NotFoundException as currently done, calls this.projectService.allowedToManage(project, uid) and returns the project; then replace the initial duplicate blocks in list, create, remove and verify with a single await this.loadManageableProject(pid, uid) call to keep behavior identical but remove duplication.backend/apps/cloud/src/project/proxy-domain-edge.guard.ts (1)
22-30: Optional: avoid per-request warn spam whenMANAGED_PROXY_EDGE_API_KEYis unset.If the env var is missing in a managed deployment, every edge hit (including ACME probes and regular runtime
/activechecks) will emit awarnlog. That can flood logs quickly. Consider either logging once on first miss (module-level latch) or resolving the expected secret in the constructor and failing fast at boot for managed deployments.♻️ Illustrative change
export class ProxyDomainEdgeGuard implements CanActivate { private readonly logger = new Logger(ProxyDomainEdgeGuard.name) + private warnedMissingSecret = false canActivate(context: ExecutionContext): boolean { const expected = process.env.MANAGED_PROXY_EDGE_API_KEY if (!expected) { - this.logger.warn( - 'MANAGED_PROXY_EDGE_API_KEY is not configured; rejecting edge request', - ) + if (!this.warnedMissingSecret) { + this.logger.warn( + 'MANAGED_PROXY_EDGE_API_KEY is not configured; rejecting edge requests', + ) + this.warnedMissingSecret = true + } throw new NotFoundException() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/project/proxy-domain-edge.guard.ts` around lines 22 - 30, The guard's canActivate currently warns on every request when MANAGED_PROXY_EDGE_API_KEY is unset (in canActivate); change this to avoid per-request warn spam by resolving the expected secret once and failing or warning once: read process.env.MANAGED_PROXY_EDGE_API_KEY in the guard's constructor (or at module top) and store it on the class (e.g., this.expected); if running in a managed deployment and the secret is missing, either throw early in the constructor to fail fast at boot or emit a single warning using a module-level latch/boolean so subsequent calls to canActivate do not log; update canActivate to reference the cached this.expected and remove the per-request logger.warn call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/apps/cloud/src/project/proxy-domain-edge.guard.ts`:
- Around line 22-30: The guard's canActivate currently warns on every request
when MANAGED_PROXY_EDGE_API_KEY is unset (in canActivate); change this to avoid
per-request warn spam by resolving the expected secret once and failing or
warning once: read process.env.MANAGED_PROXY_EDGE_API_KEY in the guard's
constructor (or at module top) and store it on the class (e.g., this.expected);
if running in a managed deployment and the secret is missing, either throw early
in the constructor to fail fast at boot or emit a single warning using a
module-level latch/boolean so subsequent calls to canActivate do not log; update
canActivate to reference the cached this.expected and remove the per-request
logger.warn call.
In `@backend/apps/cloud/src/project/proxy-domain.controller.ts`:
- Around line 37-126: Extract the repeated pid validation, project load and
authorization logic used in list, create, remove and verify into a private
helper (e.g. loadManageableProject) that takes (pid: string, uid: string), runs
isValidPID, calls this.projectService.getFullProject(pid), throws
BadRequestException/NotFoundException as currently done, calls
this.projectService.allowedToManage(project, uid) and returns the project; then
replace the initial duplicate blocks in list, create, remove and verify with a
single await this.loadManageableProject(pid, uid) call to keep behavior
identical but remove duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9d24ce0-de8c-42dd-b947-675b87aab71e
📒 Files selected for processing (3)
backend/.env.examplebackend/apps/cloud/src/project/proxy-domain-edge.guard.tsbackend/apps/cloud/src/project/proxy-domain.controller.ts
✅ Files skipped from review due to trivial changes (1)
- backend/.env.example
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/project/proxy-domain.service.ts`:
- Around line 172-215: The create() path currently reserves a hostname as a
WAITING ProxyDomain and never frees it on verification failure, enabling
squatting; update create() (in proxy-domain.service.ts) to ignore or remove
expired unverified reservations when checking per-project limits and duplicate
hostnames by treating ProxyDomain rows with status WAITING (or ERROR) whose
statusChangedAt is older than your expiration threshold (e.g., 7 days) as
expired—either delete them inside the same transaction before counting/finding
duplicates or exclude them from projectDomains and duplicate checks so a new
owner can register the hostname; use the existing statusChangedAt,
ProxyDomainStatus enum, proxyTargetId and repo methods to implement this
cleanup/ignore logic atomically.
In `@web/app/pages/Project/Settings/components/ProxyDomainsTab.tsx`:
- Around line 330-333: The effect in ProxyDomainsTab currently only runs on
mount so when switching projects without remounting the UI shows stale domains
and uses stale IDs in settingsAction; update the useEffect that calls
fetchDomains to include projectId in its dependency array so it refetches
whenever projectId changes, and inside or before fetchDomains clear/reset the
local domains state (and any selectedDomainId) so previous project data isn't
visible during the load; ensure fetchDomains/settingsAction references the
current projectId variable (not a closed-over value) and consider aborting any
in-flight request if applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8676ed7e-f238-47ac-95fc-c537cb751051
📒 Files selected for processing (8)
backend/.env.examplebackend/apps/cloud/src/project/proxy-domain-edge.guard.tsbackend/apps/cloud/src/project/proxy-domain.controller.tsbackend/apps/cloud/src/project/proxy-domain.service.tsdocs/content/docs/adblockers/managed-proxy.mdxweb/app/pages/Project/Settings/components/ProxyDomainsTab.tsxweb/app/ui/icons/Cloudflare.tsxweb/public/locales/en.json
✅ Files skipped from review due to trivial changes (3)
- backend/.env.example
- web/app/ui/icons/Cloudflare.tsx
- docs/content/docs/adblockers/managed-proxy.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/cloud/src/project/proxy-domain-edge.guard.ts
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Documentation
Translations
Chores