feat: setup deployments alerts#80
Merged
waltergalvao merged 6 commits intomainfrom Feb 20, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds workspace feature-adoption tracking across backend and frontend: DB migration and Prisma schema, API update to record last deployment timestamps, GraphQL types, frontend hook/provider and UI components for feature gating, and replaces Slack-specific alerts with a generic feature-enable alert. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 35 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/app/deployment/services/deployment.service.ts">
<violation number="1" location="apps/api/src/app/deployment/services/deployment.service.ts:146">
P2: updateWorkspaceFeatureAdoption runs before the deployment upsert, so a failed upsert will still mark lastDeploymentCreatedAt as updated. Move this update to after a successful upsert (or in a try block) so the feature adoption reflects real deployments.</violation>
</file>
<file name="apps/web/src/components/alert-banner/alert-banner.tsx">
<violation number="1" location="apps/web/src/components/alert-banner/alert-banner.tsx:49">
P2: Add rel="noopener noreferrer" when using target="_blank" to prevent the opened page from accessing window.opener.</violation>
</file>
<file name="apps/api/src/app/workspaces/services/workspace.types.ts">
<violation number="1" location="apps/api/src/app/workspaces/services/workspace.types.ts:4">
P2: `z.string().date()` only accepts YYYY-MM-DD, but this field is populated with ISO datetimes (e.g., from `new Date().toISOString()`) and exposed as `DateTime`. This will reject valid values. Use a datetime schema instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🧹 Nitpick comments (8)
apps/web/src/app/humans/teams/[id]/digests/settings/team-wip/page.tsx (1)
22-22: Prefer the barrel import over the direct file path.This import (and the same pattern in
slow-review/page.tsxandteam-metrics/page.tsx) reaches into the implementation file rather than using the barrel created in this very PR.♻️ Proposed fix
-import { AlertEnableFeature } from "../../../../../../../components/alert-enable-feature/alert-enable-feature"; +import { AlertEnableFeature } from "../../../../../../../components/alert-enable-feature";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/humans/teams/`[id]/digests/settings/team-wip/page.tsx at line 22, The import reaches into the component implementation file instead of using the new barrel export; update the import in team-wip/page.tsx (and the same pattern in slow-review/page.tsx and team-metrics/page.tsx) to import AlertEnableFeature from the component barrel export rather than the deep path. Replace the direct file import (the current import of AlertEnableFeature from "../../../../../../../components/alert-enable-feature/alert-enable-feature") with the barrel import (import { AlertEnableFeature } from "components/alert-enable-feature" or from the components/alert-enable-feature barrel used in this PR) so all three files consume the barrel export.apps/api/prisma/migrations/20260220015223_alter_table_workspaces_add_feature_adoption_column/migration.sql (1)
1-2: Migration SQL is correct, but it's bundled with feature code against the project convention.The
ADD COLUMN ... JSONB NOT NULL DEFAULT '{}'is a safe, non-destructive migration. However, per the established project practice, schema migrations should land in a separate PR from the feature implementation code.Based on learnings: "In this project, updates to the Prisma schema that require a migration are handled in separate PRs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/prisma/migrations/20260220015223_alter_table_workspaces_add_feature_adoption_column/migration.sql` around lines 1 - 2, The migration SQL adding the new column (ALTER TABLE "Workspace" ADD COLUMN "featureAdoption" JSONB NOT NULL DEFAULT '{}') must not be bundled with feature code; remove this migration from the current feature PR, revert or delete the migration file here, and create a separate PR that contains only the migration (generated via prisma migrate so the SQL matches project conventions) with verification steps (run prisma migrate dev / apply and tests). Ensure the separate PR references the same ALTER TABLE ... "featureAdoption" change and that CI runs the migration to validate it before landing.apps/web/src/app/humans/teams/[id]/alerts/page.tsx (1)
10-10: Prefer the barrel import over the direct file path.Four files in this PR import
AlertEnableFeaturedirectly from the implementation file (/alert-enable-feature/alert-enable-feature) instead of the barrel (/alert-enable-feature). The same PR'sdeployments/page.tsxandincidents/page.tsxcorrectly use the barrel. Using the barrel consistently decouples consumers from internal file structure. Affected files beyond this one:merged-without-approval/page.tsx(Line 14),slow-merge/page.tsx(Line 22), anddigests/page.tsx(Line 9).♻️ Suggested fix (applies to all 4 affected files)
-import { AlertEnableFeature } from "../../../../../components/alert-enable-feature/alert-enable-feature"; +import { AlertEnableFeature } from "../../../../../components/alert-enable-feature";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/humans/teams/`[id]/alerts/page.tsx at line 10, The import uses the implementation path for the AlertEnableFeature component instead of the barrel; update the import in page.tsx (and the other affected files) to import AlertEnableFeature from the component's barrel module (i.e., change imports from .../alert-enable-feature/alert-enable-feature to .../alert-enable-feature) so consumers use the barrel export and remain decoupled from internal file structure.apps/web/src/components/alert-enable-feature/alert-enable-feature.tsx (1)
4-8: Consider exportingFeatureandAlertEnableFeaturePropsfor type-safe consumer usage.Neither
FeaturenorAlertEnableFeaturePropsis exported. Consumers who build wrappers or pass thefeatureprop dynamically will need to reconstruct the type or cast. Exporting them costs nothing and prevents futureas Featurecasts.♻️ Suggested export additions
-type Feature = "deployments" | "slack"; +export type Feature = "deployments" | "slack"; -type AlertEnableFeatureProps = AlertProps & { +export type AlertEnableFeatureProps = AlertProps & { feature: Feature; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/alert-enable-feature/alert-enable-feature.tsx` around lines 4 - 8, Export the Feature union type and the AlertEnableFeatureProps interface so downstream consumers can import them for type-safe usage; update the declarations for Feature and AlertEnableFeatureProps (the types currently named Feature and AlertEnableFeatureProps) to be exported (e.g., add export before each type declaration) so wrappers or dynamic prop consumers can reuse the exact types instead of casting.apps/web/src/app/metrics-and-insights/page.tsx (1)
131-199: DORA overview cards are not gated behindtriedDeployments— verify this is intentional.The
AlertEnableFeaturebanner is conditional (line 70–72), but all fourCardDoraMetriccards (Deployments, Lead Time, Failure Rate, MTTR) render unconditionally. For a workspace that has never set up deployments, every card will show0/"0 hours". If showing zero-state cards alongside the setup alert is deliberate (soft nudge, not hard gate), a brief comment would clarify the intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/metrics-and-insights/page.tsx` around lines 131 - 199, The DORA metric cards (CardDoraMetric instances) render even when the workspace hasn't tried deployments, causing zero-state cards to appear alongside the AlertEnableFeature banner; either gate the entire Group of CardDoraMetric components behind the triedDeployments flag (e.g., only render the Group when triedDeployments is true) or, if the current soft-nudge behavior is intentional, add a clear inline comment near the AlertEnableFeature and the CardDoraMetric block explaining that cards intentionally show zero-state while prompting setup; update references: AlertEnableFeature, CardDoraMetric, triedDeployments, and the metrics usages (deploymentFrequency, leadTime, changeFailureRate, meanTimeToRecover) accordingly.apps/web/src/providers/feature-adoption.provider.ts (2)
15-22: 30-day window causestriedDeployments = falsefor users who set up deployments but haven't deployed in 31+ days.A workspace that set up integrations and deployed continuously for months will start showing "Setup Deployments" alerts once no deployment has occurred within the last 30 days. If the intent is simply "has the user ever tried this feature?", using
!!featureAdoption.lastDeploymentCreatedAt(without theisAfterwindow) is a cleaner semantic. If re-engagement nudging is the explicit goal, a comment explaining this 30-day reset behaviour would prevent confusion during future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/providers/feature-adoption.provider.ts` around lines 15 - 22, The current triedDeployments logic uses isAfter(parseISO(featureAdoption.lastDeploymentCreatedAt), thirtyDaysAgo()) which zeroes the flag after 30 days; change it to a semantic "ever used" check by replacing that expression with a truthy check like !!featureAdoption.lastDeploymentCreatedAt on the triedDeployments return, or if you intend the 30-day re-engagement reset, keep the isAfter logic but add a clear comment above triedDeployments explaining the 30-day expiry behavior (reference symbols: triedDeployments, featureAdoption.lastDeploymentCreatedAt, isAfter, parseISO, thirtyDaysAgo).
15-22:triedDeploymentsis not typed asboolean— add!!to normalise.The
&&short-circuit means the return isnull | undefined | boolean, notboolean. WhenlastDeploymentCreatedAtisnull/undefinedthe value isnull/undefinedrather thanfalse, which may cause unexpected type-narrowing in callers (e.g.triedDeployments === falsewon't match the no-deployment case).🔧 Proposed fix
return { triedDeployments: - featureAdoption.lastDeploymentCreatedAt && - isAfter( - parseISO(featureAdoption.lastDeploymentCreatedAt), - thirtyDaysAgo(), - ), + !!( + featureAdoption.lastDeploymentCreatedAt && + isAfter( + parseISO(featureAdoption.lastDeploymentCreatedAt), + thirtyDaysAgo(), + ) + ), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/providers/feature-adoption.provider.ts` around lines 15 - 22, The returned triedDeployments property can be null/undefined due to the && short-circuit; coerce it to a strict boolean by wrapping the whole expression with a boolean cast (e.g., use !!(...) or Boolean(...)) so callers always get true/false. Update the triedDeployments line that uses featureAdoption.lastDeploymentCreatedAt && isAfter(parseISO(featureAdoption.lastDeploymentCreatedAt), thirtyDaysAgo()) to return a boolean, referencing the triedDeployments property, featureAdoption.lastDeploymentCreatedAt, isAfter, parseISO and thirtyDaysAgo in your change.apps/api/src/app/workspaces/services/workspace.service.ts (1)
171-174: Align with codebase pattern: add null coalescing beforeassign().
workspace.featureAdoptionis a PrismaJsoncolumn that isnullfor workspaces created before migration. Whileradash.assign(null, features)safely returnsfeatures, every other service in this codebase (digest, automation, application, alerts) uses a null check before callingassign(). This inconsistency and the unsafeas WorkspaceFeatureAdoptioncast make the code less explicit and maintainable.Suggested alignment
- const updatedFeatures: WorkspaceFeatureAdoption = assign( - workspace.featureAdoption as WorkspaceFeatureAdoption, - features - ); + const updatedFeatures: WorkspaceFeatureAdoption = workspace.featureAdoption + ? assign(workspace.featureAdoption as WorkspaceFeatureAdoption, features) + : features;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/workspaces/services/workspace.service.ts` around lines 171 - 174, Replace the direct cast and call to assign with a null-coalescing check so we don't pass null into radash.assign: instead of casting workspace.featureAdoption to WorkspaceFeatureAdoption, coalesce it to an empty object (or the default shape) before calling assign for updatedFeatures; update the updatedFeatures assignment (function assign, variable updatedFeatures, and workspace.featureAdoption) to follow the same null-check pattern used in digest/automation/application/alerts services and remove the unsafe "as WorkspaceFeatureAdoption" cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/deployment/services/deployment.service.ts`:
- Around line 146-151: Move the updateWorkspaceFeatureAdoption call so it runs
only after the deployment upsert succeeds, and record the deployment's actual
timestamp instead of wall-clock time: replace the current pre-upsert call to
updateWorkspaceFeatureAdoption({ workspaceId: input.workspaceId, features: {
lastDeploymentCreatedAt: new Date().toISOString() } }) with a post-upsert call
that uses the persisted deployment's timestamp (e.g., new
Date(input.deployedAt).toISOString() or input.deployedAt.toISOString() as
appropriate); ensure this is executed after the upsert/insert promise resolves
(and not before) so failures won't produce false adoption records.
In `@apps/api/src/app/workspaces/services/workspace.types.ts`:
- Around line 3-5: The zod schema for WorkspaceFeatureAdoption is using
z.string().date() for lastDeploymentCreatedAt which will reject ISO datetime
strings; update the validator on
WorkspaceFeatureAdoption.lastDeploymentCreatedAt to use z.string().datetime()
(keeping .optional()) so it accepts full ISO timestamps (the field mapped to the
DateTime GraphQL scalar and populated with new Date().toISOString()).
In `@apps/web/src/components/alert-banner/alert-banner.tsx`:
- Around line 13-29: The component type AlertBannerProps should not extend
AlertProps because that incorrectly promises top-level AlertProps are forwarded;
remove the "extends AlertProps" from the AlertBannerProps declaration so only
the intended props (text, ctaHref, ctaText, alertProps, buttonProps, color) are
accepted, and keep forwarding any actual Alert props via the alertProps prop
when rendering Alert inside the AlertBanner component (update AlertBanner
signature to match AlertBannerProps and ensure you pass alertProps into the
underlying <Alert>).
In `@apps/web/src/components/alert-enable-feature/alert-enable-feature.tsx`:
- Around line 10-18: AlertBanner currently always renders ctaHref via React
Router's Link which breaks absolute external URLs (e.g., the deployments href
"https://...") — update AlertBanner (or pre-process in alert-enable-feature.tsx)
to detect external URLs and render a normal anchor tag for them: inspect the
ctaHref/href string (from featureMap entries like deployments and slack) and if
it startsWith("http") (or isAbsolute) render <a href=... target="_blank"
rel="noopener noreferrer"> instead of Link/ to={...}, otherwise keep using Link
for internal SPA routes.
In `@apps/web/src/components/card-pull-request/timeline-pull-request.tsx`:
- Around line 265-271: The "Setup Deployments" Button is non-functional and
wrapped in an unnecessary fragment; update the conditional block that references
triedDeployments to remove the fragment and wire the Button to navigate to the
deployments management page by adding a navigation prop (e.g., component/link
with to="/deployments" or an onClick that calls router.push('/deployments')) so
clicking it routes to the deployments page; keep the Button props
(color="violet" size="xs" variant="outline" mt={5}) and only change the JSX to
attach the navigation handler.
In `@apps/web/src/providers/app.provider.ts`:
- Around line 10-19: WorkspaceData type was expanded to include featureAdoption
which is persisted to localStorage and will break hydration for users with an
older cache; fix by adding a persist version and migrate handler to the zustand
persist config used where WorkspaceData is stored (reference the WorkspaceData
type and the persist config for userWorkspaces) to either backfill
featureAdoption or reset stale workspace entries, and/or modify the
useFeatureAdoption hook to return a safe default (e.g., triedDeployments: false)
instead of throwing on missing featureAdoption so pages like
MetricsAndInsightsPage, TimelinePullRequest, DeploymentsPage and IncidentsPage
do not crash during hydration.
---
Duplicate comments:
In
`@apps/web/src/app/humans/teams/`[id]/alerts/settings/merged-without-approval/page.tsx:
- Line 14: The import in page.tsx uses a direct file path for the
AlertEnableFeature component; update the import for AlertEnableFeature to use
the barrel export instead (replace the current
"../../../../../../../components/alert-enable-feature/alert-enable-feature"
import with "../../../../../../../components/alert-enable-feature") so the
component is imported from the barrel index export.
In `@apps/web/src/app/humans/teams/`[id]/alerts/settings/slow-merge/page.tsx:
- Line 22: The import in page.tsx uses a direct file import
"components/alert-enable-feature/alert-enable-feature"; update the import for
the AlertEnableFeature symbol to use the barrel export instead (import from
"../../../../../../../components/alert-enable-feature") so it matches the other
file (alerts/page.tsx) and avoids direct file path usage; modify the single
import statement that references AlertEnableFeature accordingly.
In `@apps/web/src/app/humans/teams/`[id]/alerts/settings/slow-review/page.tsx:
- Line 22: The import in page.tsx brings in AlertEnableFeature via a deep direct
path; replace that with the components barrel export to match the rest of the
codebase (as noted in team-wip/page.tsx). Update the import of
AlertEnableFeature to come from the shared components barrel (the central export
file) instead of
"../../../../../../../components/alert-enable-feature/alert-enable-feature" so
other files import AlertEnableFeature from the barrel (e.g., the top-level
components export) for consistency.
In `@apps/web/src/app/humans/teams/`[id]/digests/page.tsx:
- Line 9: The import in page.tsx currently pulls the component via a direct file
path; update the import for AlertEnableFeature to use the barrel export instead
so it matches other files (use the module import
"components/alert-enable-feature" rather than the specific file path). Locate
the import of AlertEnableFeature in
apps/web/src/app/humans/teams/[id]/digests/page.tsx and replace the direct file
import with the barrel module import that exports AlertEnableFeature.
In `@apps/web/src/app/humans/teams/`[id]/digests/settings/team-metrics/page.tsx:
- Line 20: The import in team-metrics/page.tsx directly references the component
file (AlertEnableFeature) instead of the module barrel; update the import to use
the package/barrel export the project uses (the same pattern used in
team-wip/page.tsx) so AlertEnableFeature is imported from the module's
index/barrel rather than
"../../../../../../../components/alert-enable-feature/alert-enable-feature";
locate the AlertEnableFeature import statement in team-metrics/page.tsx and
replace it with the barrel import path used elsewhere in the app.
In `@apps/web/src/app/metrics-and-insights/page.tsx`:
- Line 44: useFeatureAdoption() can throw ResourceNotFound when featureAdoption
is missing, so protect the metrics page from crashing by making the call site
resilient: wrap the call in a try/catch (catching ResourceNotFound) or guard its
result and supply a safe default for triedDeployments (e.g., false) when the
hook throws or returns undefined; update the usage in
metrics-and-insights/page.tsx around the useFeatureAdoption() invocation to
return a fallback instead of letting the exception propagate until the root fix
in app.provider.ts is applied.
---
Nitpick comments:
In
`@apps/api/prisma/migrations/20260220015223_alter_table_workspaces_add_feature_adoption_column/migration.sql`:
- Around line 1-2: The migration SQL adding the new column (ALTER TABLE
"Workspace" ADD COLUMN "featureAdoption" JSONB NOT NULL DEFAULT '{}') must not
be bundled with feature code; remove this migration from the current feature PR,
revert or delete the migration file here, and create a separate PR that contains
only the migration (generated via prisma migrate so the SQL matches project
conventions) with verification steps (run prisma migrate dev / apply and tests).
Ensure the separate PR references the same ALTER TABLE ... "featureAdoption"
change and that CI runs the migration to validate it before landing.
In `@apps/api/src/app/workspaces/services/workspace.service.ts`:
- Around line 171-174: Replace the direct cast and call to assign with a
null-coalescing check so we don't pass null into radash.assign: instead of
casting workspace.featureAdoption to WorkspaceFeatureAdoption, coalesce it to an
empty object (or the default shape) before calling assign for updatedFeatures;
update the updatedFeatures assignment (function assign, variable
updatedFeatures, and workspace.featureAdoption) to follow the same null-check
pattern used in digest/automation/application/alerts services and remove the
unsafe "as WorkspaceFeatureAdoption" cast.
In `@apps/web/src/app/humans/teams/`[id]/alerts/page.tsx:
- Line 10: The import uses the implementation path for the AlertEnableFeature
component instead of the barrel; update the import in page.tsx (and the other
affected files) to import AlertEnableFeature from the component's barrel module
(i.e., change imports from .../alert-enable-feature/alert-enable-feature to
.../alert-enable-feature) so consumers use the barrel export and remain
decoupled from internal file structure.
In `@apps/web/src/app/humans/teams/`[id]/digests/settings/team-wip/page.tsx:
- Line 22: The import reaches into the component implementation file instead of
using the new barrel export; update the import in team-wip/page.tsx (and the
same pattern in slow-review/page.tsx and team-metrics/page.tsx) to import
AlertEnableFeature from the component barrel export rather than the deep path.
Replace the direct file import (the current import of AlertEnableFeature from
"../../../../../../../components/alert-enable-feature/alert-enable-feature")
with the barrel import (import { AlertEnableFeature } from
"components/alert-enable-feature" or from the components/alert-enable-feature
barrel used in this PR) so all three files consume the barrel export.
In `@apps/web/src/app/metrics-and-insights/page.tsx`:
- Around line 131-199: The DORA metric cards (CardDoraMetric instances) render
even when the workspace hasn't tried deployments, causing zero-state cards to
appear alongside the AlertEnableFeature banner; either gate the entire Group of
CardDoraMetric components behind the triedDeployments flag (e.g., only render
the Group when triedDeployments is true) or, if the current soft-nudge behavior
is intentional, add a clear inline comment near the AlertEnableFeature and the
CardDoraMetric block explaining that cards intentionally show zero-state while
prompting setup; update references: AlertEnableFeature, CardDoraMetric,
triedDeployments, and the metrics usages (deploymentFrequency, leadTime,
changeFailureRate, meanTimeToRecover) accordingly.
In `@apps/web/src/components/alert-enable-feature/alert-enable-feature.tsx`:
- Around line 4-8: Export the Feature union type and the AlertEnableFeatureProps
interface so downstream consumers can import them for type-safe usage; update
the declarations for Feature and AlertEnableFeatureProps (the types currently
named Feature and AlertEnableFeatureProps) to be exported (e.g., add export
before each type declaration) so wrappers or dynamic prop consumers can reuse
the exact types instead of casting.
In `@apps/web/src/providers/feature-adoption.provider.ts`:
- Around line 15-22: The current triedDeployments logic uses
isAfter(parseISO(featureAdoption.lastDeploymentCreatedAt), thirtyDaysAgo())
which zeroes the flag after 30 days; change it to a semantic "ever used" check
by replacing that expression with a truthy check like
!!featureAdoption.lastDeploymentCreatedAt on the triedDeployments return, or if
you intend the 30-day re-engagement reset, keep the isAfter logic but add a
clear comment above triedDeployments explaining the 30-day expiry behavior
(reference symbols: triedDeployments, featureAdoption.lastDeploymentCreatedAt,
isAfter, parseISO, thirtyDaysAgo).
- Around line 15-22: The returned triedDeployments property can be
null/undefined due to the && short-circuit; coerce it to a strict boolean by
wrapping the whole expression with a boolean cast (e.g., use !!(...) or
Boolean(...)) so callers always get true/false. Update the triedDeployments line
that uses featureAdoption.lastDeploymentCreatedAt &&
isAfter(parseISO(featureAdoption.lastDeploymentCreatedAt), thirtyDaysAgo()) to
return a boolean, referencing the triedDeployments property,
featureAdoption.lastDeploymentCreatedAt, isAfter, parseISO and thirtyDaysAgo in
your change.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/web/src/components/alert-banner/alert-banner.tsx (2)
45-57: Consider using native<a>instead ofLinkfor external URLs.The
IconExternalLinkicon andctaHrefnaming signal that this component is intended for external destinations. React Router'sLinkhandles absolute URLs by falling through to an<a>, but using<a>directly is semantically clearer and avoids any potential router interception (e.g., history push, link prefetching) that a future react-router upgrade might add.♻️ Proposed refactor
-import { Link } from "react-router-dom"; - ... <Button size="xs" variant="filled" - component={Link} + component="a" + href={ctaHref} target="_blank" - to={ctaHref} color={color} rel="noopener noreferrer" rightSection={<IconExternalLink stroke={1.5} size={16} />} {...buttonProps} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/alert-banner/alert-banner.tsx` around lines 45 - 57, The Button in alert-banner.tsx currently uses component={Link} with to={ctaHref} for external links; change it to use a native anchor by setting component="a" and replace to={ctaHref} with href={ctaHref} (keep target="_blank", rel="noopener noreferrer", rightSection={<IconExternalLink .../>}, size/variant/color and {...buttonProps} intact) so the CTA uses a semantic <a> and avoids React Router interception.
39-39:bdCSS variable interpolation silently breaks for non-paletteMantineColorvalues.
var(--mantine-color-${color}-4)only resolves correctly for named Mantine palette colors (e.g.,"violet","blue"). If a caller passes a hex string or custom color token, thevar()will be undefined and the border will silently disappear. Consider guarding against this or documenting the constraint.🛡️ Proposed defensive fix
- bd={`1px solid var(--mantine-color-${color}-4)`} + bd={color.startsWith("#") || color.startsWith("rgb") ? `1px solid ${color}` : `1px solid var(--mantine-color-${color}-4)`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/alert-banner/alert-banner.tsx` at line 39, The border-defining expression using bd={`1px solid var(--mantine-color-${color}-4)`} fails for non-palette values (hex/custom tokens); update the AlertBanner component to detect whether the passed color is a named Mantine palette key (e.g., exists in theme.colors or matches a palette-name pattern) and only use var(--mantine-color-${color}-4) in that case, otherwise fall back to using the color value directly (e.g., `1px solid ${color}`) so hex strings and custom tokens render a border; locate the bd usage in alert-banner.tsx and update the logic around the color prop/variable to choose the var(...) form for palette names and the direct color string for all other inputs.apps/web/src/components/card-pull-request/timeline-pull-request.tsx (1)
266-281: Usecomponent="a"instead ofcomponent={Link}for external links.React Router maintainers explicitly state that "links outside of the application have nothing to do with the internal router at all and should reach for the appropriate tag." If you need to simply link to an external URL, you can use an
<a>tag. Usingcomponent={Link}withto="https://..."is non-idiomatic and its behavior with fully-qualified external URLs is not officially supported by react-router-dom v6.Mantine's
Buttonnatively supportscomponent="a"for this pattern, and the unnecessary<>…</>fragment wrapping a single child can be dropped at the same time:♻️ Proposed fix
{!triedDeployments && ( - <> - <Button - color="violet" - size="xs" - variant="outline" - mt={5} - component={Link} - target="_blank" - rel="noopener noreferrer" - to="https://docs.sweetr.dev/features/deployments" - > - Setup Deployments - </Button> - </> + <Button + color="violet" + size="xs" + variant="outline" + mt={5} + component="a" + href="https://docs.sweetr.dev/features/deployments" + target="_blank" + rel="noopener noreferrer" + > + Setup Deployments + </Button> )}The
Linkimport added on line 28 can be removed along with this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/card-pull-request/timeline-pull-request.tsx` around lines 266 - 281, The Button render for the external docs link should use an anchor element instead of React Router Link: in the block gated by triedDeployments change Button's prop from component={Link} and to="https://docs.sweetr.dev/features/deployments" to component="a" and href="https://docs.sweetr.dev/features/deployments", keep target and rel, and remove the unnecessary fragment wrapper; also remove the now-unused Link import near the top of the file to avoid dead imports.apps/web/src/providers/feature-adoption.provider.ts (1)
16-22:triedDeploymentsis not typed asboolean— use!!coercion.When
lastDeploymentCreatedAtisnullorundefined, the&&short-circuit makestriedDeploymentsnullorundefinedrather thanfalse. TypeScript infers the field asstring | boolean | null | undefined, notboolean. Current callers only use truthy/falsy checks so there's no runtime bug, but strict equality (=== false) or typed props downstream would behave unexpectedly.♻️ Proposed fix
return { triedDeployments: - featureAdoption.lastDeploymentCreatedAt && - isAfter( + !!featureAdoption.lastDeploymentCreatedAt && + isAfter( parseISO(featureAdoption.lastDeploymentCreatedAt), thirtyDaysAgo(), ), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/providers/feature-adoption.provider.ts` around lines 16 - 22, The triedDeployments field can be null/undefined due to the && short-circuit; coerce it to a strict boolean by applying double-negation (e.g., !!) to the expression that checks featureAdoption.lastDeploymentCreatedAt and isAfter(parseISO(...), thirtyDaysAgo()); update the return value for triedDeployments so it always yields a boolean while keeping the same logic that uses featureAdoption.lastDeploymentCreatedAt, parseISO, isAfter and thirtyDaysAgo().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/components/alert-banner/alert-banner.tsx`:
- Around line 13-20: The AlertBannerProps interface change is correct: it no
longer extends AlertProps and instead exposes alertProps?: AlertProps as an
explicit escape hatch; no code changes required—leave the interface definition
(AlertBannerProps) as shown and ensure components that previously relied on
inherited AlertProps now pass extra alert attributes via the alertProps prop
when rendering the Alert component.
In `@apps/web/src/providers/app.provider.ts`:
- Around line 10-19: The hydration-crash concern is resolved so no code change
is necessary; keep the WorkspaceData type (export type WorkspaceData) as-is with
billing?: Partial<Billing> | null and ensure the hook/useFeatureAdoption
implementation returns a safe default like { triedDeployments: false } when
featureAdoption is missing so stale caches (version: 1) won’t cause runtime
throws during hydration.
- Line 39: The persist config currently sets { name: "app-store", version: 1 }
which will cause Zustand to drop persisted state on version mismatch; add a
migrate function to the persist options (or keep version 0) so existing keys
like authenticatedUser are preserved. Implement a migrate(oldState, fromVersion)
handler referenced in the persist options that maps or returns the previous
state shape (e.g., copy authenticatedUser and other non-adoption fields into the
new shape) and returns the migrated state, ensuring the persist config
containing "app-store" uses that migrate function to avoid clearing users'
persisted auth on upgrade.
---
Nitpick comments:
In `@apps/web/src/components/alert-banner/alert-banner.tsx`:
- Around line 45-57: The Button in alert-banner.tsx currently uses
component={Link} with to={ctaHref} for external links; change it to use a native
anchor by setting component="a" and replace to={ctaHref} with href={ctaHref}
(keep target="_blank", rel="noopener noreferrer",
rightSection={<IconExternalLink .../>}, size/variant/color and {...buttonProps}
intact) so the CTA uses a semantic <a> and avoids React Router interception.
- Line 39: The border-defining expression using bd={`1px solid
var(--mantine-color-${color}-4)`} fails for non-palette values (hex/custom
tokens); update the AlertBanner component to detect whether the passed color is
a named Mantine palette key (e.g., exists in theme.colors or matches a
palette-name pattern) and only use var(--mantine-color-${color}-4) in that case,
otherwise fall back to using the color value directly (e.g., `1px solid
${color}`) so hex strings and custom tokens render a border; locate the bd usage
in alert-banner.tsx and update the logic around the color prop/variable to
choose the var(...) form for palette names and the direct color string for all
other inputs.
In `@apps/web/src/components/card-pull-request/timeline-pull-request.tsx`:
- Around line 266-281: The Button render for the external docs link should use
an anchor element instead of React Router Link: in the block gated by
triedDeployments change Button's prop from component={Link} and
to="https://docs.sweetr.dev/features/deployments" to component="a" and
href="https://docs.sweetr.dev/features/deployments", keep target and rel, and
remove the unnecessary fragment wrapper; also remove the now-unused Link import
near the top of the file to avoid dead imports.
In `@apps/web/src/providers/feature-adoption.provider.ts`:
- Around line 16-22: The triedDeployments field can be null/undefined due to the
&& short-circuit; coerce it to a strict boolean by applying double-negation
(e.g., !!) to the expression that checks featureAdoption.lastDeploymentCreatedAt
and isAfter(parseISO(...), thirtyDaysAgo()); update the return value for
triedDeployments so it always yields a boolean while keeping the same logic that
uses featureAdoption.lastDeploymentCreatedAt, parseISO, isAfter and
thirtyDaysAgo().
Contributor
There was a problem hiding this comment.
2 issues found across 11 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/components/alert-banner/alert-banner.tsx">
<violation number="1" location="apps/web/src/components/alert-banner/alert-banner.tsx:55">
P2: External CTA buttons ignore buttonProps, so callers cannot customize or disable the button for external links. Spread buttonProps in the external branch to keep behavior consistent.</violation>
</file>
<file name="apps/web/src/components/card-pull-request/timeline-pull-request.tsx">
<violation number="1" location="apps/web/src/components/card-pull-request/timeline-pull-request.tsx:268">
P2: Avoid nesting a button inside an anchor. Use a single interactive element (e.g., Button rendered as an anchor) to keep valid HTML and prevent conflicting click/keyboard behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/alert-banner/alert-banner.tsx`:
- Around line 48-74: The external-link branch is not applying the passed
buttonProps, so props like disabled/loading/size/variant are dropped when
isExternalLink is true; update the JSX so the external Anchor's inner <Button>
(inside the isExternalLink branch) also spreads {...buttonProps} (same as the
internal Link branch) and ensure the prop spread is applied to the <Button>
element next to existing props like size, variant, color, and rightSection to
preserve overrides.
---
Duplicate comments:
In `@apps/web/src/components/alert-banner/alert-banner.tsx`:
- Around line 14-21: The AlertBannerProps interface should be a standalone
contract (remove any extends AlertProps) and expose an optional alertProps
escape hatch; update the AlertBanner component to accept AlertBannerProps and
pass ...alertProps into the Mantine Alert (or whatever Alert component) so
external callers can override Alert props while keeping the public API
explicit—verify symbols AlertBannerProps, alertProps and the Alert component
usage are updated accordingly.
In `@apps/web/src/components/card-pull-request/timeline-pull-request.tsx`:
- Around line 266-279: Nesting an interactive <Anchor> around a <Button> (when
rendering the Setup Deployments CTA) is invalid; replace the Anchor+Button pair
by rendering the Mantine Button as a link using its polymorphic component prop
(e.g., set component="a" on Button and pass href, target, rel directly) and
remove the unnecessary fragment; update the conditional block around
triedDeployments to render only the Button (styled outline, color "violet", size
"xs", mt={5}) with component="a" so it produces a single <a> element instead of
nested interactive elements.
Comment on lines
+160
to
+184
| export const updateWorkspaceFeatureAdoption = ({ | ||
| workspaceId, | ||
| features, | ||
| }: UpdateWorkspaceFeaturesInput): void => { | ||
| // Force it to run in the background - no await | ||
| (async () => { | ||
| const workspace = await findWorkspaceById(workspaceId); | ||
|
|
||
| if (!workspace) { | ||
| throw new ResourceNotFoundException("Workspace not found"); | ||
| } | ||
|
|
||
| const updatedFeatures: WorkspaceFeatureAdoption = assign( | ||
| workspace.featureAdoption as WorkspaceFeatureAdoption, | ||
| features | ||
| ); | ||
|
|
||
| await getPrisma(workspaceId).workspace.update({ | ||
| where: { id: workspaceId }, | ||
| data: { featureAdoption: updatedFeatures as Prisma.InputJsonValue }, | ||
| }); | ||
| })().catch((error) => | ||
| logger.warn("Failed to update feature adoption", { error, workspaceId }) | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
Race condition: concurrent updates to featureAdoption will overwrite each other. assign() merges existing DB value with new features, but if two upsertDeployment calls run simultaneously, both read old value, merge it, and write - last write wins, losing the first update.
Use atomic database update instead:
Suggested change
| export const updateWorkspaceFeatureAdoption = ({ | |
| workspaceId, | |
| features, | |
| }: UpdateWorkspaceFeaturesInput): void => { | |
| // Force it to run in the background - no await | |
| (async () => { | |
| const workspace = await findWorkspaceById(workspaceId); | |
| if (!workspace) { | |
| throw new ResourceNotFoundException("Workspace not found"); | |
| } | |
| const updatedFeatures: WorkspaceFeatureAdoption = assign( | |
| workspace.featureAdoption as WorkspaceFeatureAdoption, | |
| features | |
| ); | |
| await getPrisma(workspaceId).workspace.update({ | |
| where: { id: workspaceId }, | |
| data: { featureAdoption: updatedFeatures as Prisma.InputJsonValue }, | |
| }); | |
| })().catch((error) => | |
| logger.warn("Failed to update feature adoption", { error, workspaceId }) | |
| ); | |
| }; | |
| export const updateWorkspaceFeatureAdoption = ({ | |
| workspaceId, | |
| features, | |
| }: UpdateWorkspaceFeaturesInput): void => { | |
| // Force it to run in the background - no await | |
| (async () => { | |
| const currentFeatures = await getPrisma(workspaceId).workspace.findUnique({ | |
| where: { id: workspaceId }, | |
| select: { featureAdoption: true } | |
| }); | |
| if (!currentFeatures) { | |
| throw new ResourceNotFoundException("Workspace not found"); | |
| } | |
| const updatedFeatures: WorkspaceFeatureAdoption = assign( | |
| currentFeatures.featureAdoption as WorkspaceFeatureAdoption, | |
| features | |
| ); | |
| await getPrisma(workspaceId).workspace.update({ | |
| where: { id: workspaceId }, | |
| data: { featureAdoption: updatedFeatures as Prisma.InputJsonValue }, | |
| }); | |
| })().catch((error) => | |
| logger.warn("Failed to update feature adoption", { error, workspaceId }) | |
| ); | |
| }; |
sweetrdev
approved these changes
Feb 20, 2026
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.
Greptile Summary
This PR implements a feature adoption tracking system to show contextual alerts prompting users to setup deployments and Slack integrations when they haven't been tried recently.
Key Changes:
featureAdoptionJSONB column toWorkspacetable to track feature usage timestampsupdateWorkspaceFeatureAdoptionfunction that runs asynchronously on deployment creationAlertBannercomponent and refactored existingAlertTrialto use itAlertEnableFeaturecomponent to show setup prompts for deployments and SlackIssues Found:
updateWorkspaceFeatureAdoptionwhere concurrent deployment creations can overwrite each other's updates due to read-modify-write patternConfidence Score: 3/5
apps/api/src/app/workspaces/services/workspace.service.tswhich has the race condition bug, andapps/web/src/components/alert-banner/alert-banner.tsxwhich has the previously reported issuesImportant Files Changed
Sequence Diagram
sequenceDiagram participant Client as Web Client participant API as GraphQL API participant DB as Database participant DeploymentSvc as DeploymentService participant WorkspaceSvc as WorkspaceService Note over Client,DB: User visits Deployments/Incidents page Client->>API: Query workspace with featureAdoption API->>DB: SELECT workspace, featureAdoption JSONB DB-->>API: Return workspace data API->>WorkspaceSvc: safeParseFeatureAdoption(jsonb) WorkspaceSvc-->>API: Validated WorkspaceFeatureAdoption API-->>Client: Workspace with featureAdoption Client->>Client: Check if triedDeployments<br/>(lastDeploymentCreatedAt within 30 days) alt Not tried deployments Client->>Client: Show AlertEnableFeature banner end Note over Client,WorkspaceSvc: External system creates deployment Client->>API: Mutation: upsertDeployment API->>DeploymentSvc: upsertDeployment(input) DeploymentSvc->>WorkspaceSvc: updateWorkspaceFeatureAdoption<br/>(fire-and-forget) par Async background update WorkspaceSvc->>DB: findWorkspaceById DB-->>WorkspaceSvc: Current workspace WorkspaceSvc->>WorkspaceSvc: Merge featureAdoption with<br/>lastDeploymentCreatedAt WorkspaceSvc->>DB: UPDATE featureAdoption JSONB and Main flow continues DeploymentSvc->>DB: UPSERT deployment DB-->>DeploymentSvc: Deployment created DeploymentSvc-->>API: Success API-->>Client: Deployment created endLast reviewed commit: 8823227