Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds sequence-tracking columns and indexes, ClickHouse tables/views, backfill and CRUD hooks to sync Team and TeamMember, updates external-db-sync normalization/mapping, and expands e2e tests and helpers for Teams, TeamMembers, and ContactChannels. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant CRUD as Backend CRUD
participant Sequencer as Sequencer
participant SyncLib as ExternalDbSyncLib
participant PG as Postgres
participant CH as ClickHouse
Client->>CRUD: create/update/delete Team or TeamMember
CRUD->>SyncLib: withExternalDbSyncUpdate / recordExternalDbSyncDeletion
SyncLib->>PG: insert DeletedRow / upsert _stack_sync_metadata
Note right of PG: internal Postgres state persisted
Sequencer->>PG: select shouldUpdateSequenceId=true
Sequencer->>PG: update sequenceId via nextval & clear flag
Sequencer->>SyncLib: enqueue sync batch (tenancy)
SyncLib->>CH: pushRowsToClickhouse (apply per-table normalizers)
CH-->>SyncLib: ack
SyncLib-->>CRUD: async ack
CRUD-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Greptile SummaryExtends the external DB sync system to sync
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph CRUD["CRUD Operations"]
TC["Team Create/Update"] -->|withExternalDbSyncUpdate| TDB["Team table\n(shouldUpdateSequenceId=true)"]
TMC["TeamMember Create"] -->|withExternalDbSyncUpdate| TMDB["TeamMember table\n(shouldUpdateSequenceId=true)"]
TD2["Team Delete"] -->|recordExternalDbSyncTeamMemberDeletionsForTeam| DR1["DeletedRow (TeamMember)"]
TD2 -->|recordExternalDbSyncDeletion| DR2["DeletedRow (Team)"]
TMD["TeamMember Delete"] -->|recordExternalDbSyncDeletion| DR3["DeletedRow (TeamMember)"]
UD["User Delete"] -->|recordExternalDbSyncTeamMemberDeletionsForUser| DR4["DeletedRow (TeamMember)"]
end
subgraph Sequencer["Sequencer (Cron)"]
SEQ["backfillSequenceIds"] -->|assigns global_seq_id| TDB
SEQ -->|assigns global_seq_id| TMDB
SEQ -->|assigns global_seq_id| DR1
SEQ -->|assigns global_seq_id| DR2
SEQ --> ENQ["enqueueExternalDbSyncBatch"]
end
subgraph Sync["Sync Worker"]
ENQ --> PG["Postgres External DB\n(teams, team_members,\ncontact_channels)"]
ENQ --> CH["ClickHouse\n(analytics_internal.teams,\nteam_members, contact_channels)"]
end
Last reviewed commit: 3c88950 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts`:
- Around line 731-752: Add an explicit create/delete lifecycle for a secondary
ContactChannel in this test: after User.create(...) use the API route that
creates contact channels (the same endpoint the app exposes for POST
/api/v1/contact-channels) to create a secondary channel (e.g., value
'cc-secondary@example.com'), wait for it to appear in the external DB with
waitForSyncedContactChannel(client, 'cc-secondary@example.com'), then call the
API DELETE for that channel, wait for removal with
waitForSyncedContactChannelDeletion(client, 'cc-secondary@example.com'), and
finally assert the row is gone (no rows returned from SELECT on contact_channels
for that value); keep the existing primary-channel assertions and reuse
createProjectWithExternalDb and dbManager client as in the test.
- Around line 644-675: Add ClickHouse E2E assertions for the new mappings by
extending the existing "Team sync (ClickHouse)" test: after creating the team
and awaiting its presence via runQueryForCurrentProject, add at least one
happy-path SELECT for contact_channels (e.g., query "SELECT * FROM
contact_channels WHERE ...") and one for team_members (e.g., query "SELECT *
FROM team_members WHERE ...") using runQueryForCurrentProject and expect 200 and
non-empty results; also add a negative/delete visibility assertion for one of
those tables by deleting or marking a record invisible via the existing test
helpers (or creating then deleting a contact_channel/team_member) and then
polling with the same runQueryForCurrentProject loop until the row is absent,
asserting length === 0. Ensure you reuse the same timeout/interval pattern and
reference response variable checks like response.body.result to validate the
results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea49df6a-2308-4d43-bd5c-6c965a84ced7
📒 Files selected for processing (11)
apps/backend/prisma/migrations/20260316000000_add_team_team_member_sequence_columns/migration.sqlapps/backend/prisma/schema.prismaapps/backend/scripts/clickhouse-migrations.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/src/app/api/latest/team-memberships/crud.tsxapps/backend/src/app/api/latest/teams/crud.tsxapps/backend/src/app/api/latest/users/crud.tsxapps/backend/src/lib/external-db-sync.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.tspackages/stack-shared/src/config/db-sync-mappings.ts
BilalG1
changed the title
…ck-auth/stack-auth into clickhouse-sync-extra-tables
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/backend/scripts/clickhouse-migrations.ts (1)
298-333: Clarify the intended use of the embeddeduserJSON column.The
userJSON column denormalizes user data into team_member_profiles. This creates a snapshot of user data at sync time, which:
- May become stale if the user record is updated separately
- Could contain PII (e.g.,
primary_email) that duplicates data already in theuserstableIf the intent is to provide a point-in-time snapshot for analytics, this is fine. If consumers expect this to reflect current user state, consider whether joining to the
usersview would be more appropriate for queries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/clickhouse-migrations.ts` around lines 298 - 333, The embedded user JSON column in TEAM_MEMBER_PROFILES_TABLE_BASE_SQL (the `user` field) denormalizes a point-in-time snapshot which may become stale or duplicate PII from the users table; clarify intended behavior and either (A) document in the schema comment and view (TEAM_MEMBER_PROFILES_VIEW_SQL) that this is a snapshot for analytics and may contain PII, or (B) change the design to not store full user JSON—store only non-PII fields or a reference (user_id) and update TEAM_MEMBER_PROFILES_VIEW_SQL to join the canonical users view at query time so consumers see current user state; update names/comments and any downstream documentation to reflect the chosen approach.
🤖 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/backend/scripts/clickhouse-migrations.ts`:
- Around line 218-257: The contact_channels table definitions
(CONTACT_CHANNELS_TABLE_BASE_SQL) store raw PII in the value column and the view
(CONTACT_CHANNELS_VIEW_SQL) currently exposes it; to address privacy/compliance,
change the schema to stop exposing raw values—add a hashed_value (or
masked_value) column to analytics_internal.contact_channels and populate it at
ingest or via a migration instead of storing/returning the plain value, then
update default.contact_channels view to select hashed_value (or a masked
representation) instead of value; additionally add a retention policy/TTL or
clear-on-delete behavior in the table definition and document the retention
rules for the new hashed/masked field.
In `@apps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.ts`:
- Around line 673-674: Replace the non-null assertion on the `response` variable
in the assertions (the two lines referencing `response!.body.result`) with a
null-coalescing check that calls `throwErr(...)` to provide an explicit error if
`response` is undefined; e.g., use `const resp = response ?? throwErr('Expected
response to be set before assertions')` and then assert on
`resp.body.result.length` and `resp.body.result[0].display_name`. Ensure you
import `throwErr` from `@stackframe/stack-shared/dist/utils/errors` if it’s not
already imported.
In `@packages/stack-shared/src/config/db-sync-mappings.ts`:
- Line 825: The mapping for last_active_at_millis mistakenly extracts epoch
milliseconds from "ProjectUser"."createdAt"; update the expression that defines
last_active_at_millis in db-sync-mappings (the CASE/EXTRACT expression
associated with 'last_active_at_millis') to use "ProjectUser"."lastActiveAt"
instead of "ProjectUser"."createdAt", ensuring the NULL check still references
"ProjectUser"."lastActiveAt" so the produced value matches the ClickHouse
variant.
---
Nitpick comments:
In `@apps/backend/scripts/clickhouse-migrations.ts`:
- Around line 298-333: The embedded user JSON column in
TEAM_MEMBER_PROFILES_TABLE_BASE_SQL (the `user` field) denormalizes a
point-in-time snapshot which may become stale or duplicate PII from the users
table; clarify intended behavior and either (A) document in the schema comment
and view (TEAM_MEMBER_PROFILES_VIEW_SQL) that this is a snapshot for analytics
and may contain PII, or (B) change the design to not store full user JSON—store
only non-PII fields or a reference (user_id) and update
TEAM_MEMBER_PROFILES_VIEW_SQL to join the canonical users view at query time so
consumers see current user state; update names/comments and any downstream
documentation to reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f8c340a-094c-4c4e-820e-66bf969d8f1a
📒 Files selected for processing (7)
apps/backend/scripts/clickhouse-migrations.tsapps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.tsapps/backend/src/lib/external-db-sync.tsapps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-basics.test.tsapps/e2e/tests/backend/endpoints/api/v1/external-db-sync-utils.tspackages/stack-shared/src/config/db-sync-mappings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts
Was using createdAt instead of lastActiveAt in the Postgres variant of the team_member_profiles sync query.
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.
Summary by CodeRabbit
New Features
Chores
Tests