[sync] fix(app): filter system bot users from collaborator management lists T6451 - #3616
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c806776f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| handleBestEffortDataDbDropError({ | ||
| error, | ||
| isMetaFallback, | ||
| logger: this.logger, | ||
| target: `table ${table.id}`, | ||
| }); |
There was a problem hiding this comment.
When this new handler swallows a dead BYODB drop, dropTables still falls through to invalidateDroppedTable. That invalidator resolves a V2 container via getContainerForBase, and on a cold cache container creation calls createTypeValidationStrategy, which immediately executes a query against the same data DB; for the deleted Supabase tenant/auth-failure case being handled here, that reconnect fails again and the trash cleanup remains stuck. Consider skipping or making the cache invalidation best-effort when the physical drop was already swallowed.
Useful? React with 👍 / 👎.
| const { isMetaFallback } = await this.dataDbClientManager.getDataDatabaseForBase(baseId, { | ||
| useTransaction: true, | ||
| }); |
There was a problem hiding this comment.
When the BYODB connection is already in error or needs migration, dataPrismaForBase can fail while DataDbClientManager.resolveSpaceDataDb runs ensureConnectionMigrated; this catch then calls getDataDatabaseForBase, which takes the same migration/connect path again before returning isMetaFallback. In that state the dead-tenant/auth failure never reaches handleBestEffortDataDbDropError, so permanent base deletion remains stuck instead of being swallowed.
Useful? React with 👍 / 👎.
| if (!classification || classification.retryable) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Because this only rethrows retryable or unclassified errors, a live BYODB that rejects the DDL with a classified non-retryable error such as Postgres 42501 (data_db.permission_denied) is logged and swallowed. In that scenario the metadata cleanup commits even though the physical table/schema was not dropped, leaving customer data orphaned in a reachable database; the helper should allowlist the missing/deleted/auth-failure cases it intends to tolerate rather than every non-retryable classification.
Useful? React with 👍 / 👎.
tea-artist
changed the title
6c80677 to
b39c6f8
Compare
tea-artist
changed the title
b39c6f8 to
6eefd41
Compare
tea-artist
changed the title
6eefd41 to
da63387
Compare
tea-artist
changed the title
da63387 to
c88d1ad
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c88d1add52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return rows.map((row) => ({ | ||
| ...row, | ||
| content: parseSettingContent(row.content), | ||
| content: stripCanarySpaceIdsForForceV2Cache(row.name, parseSettingContent(row.content)), |
There was a problem hiding this comment.
When FORCE_V2_ALL=true, this stripped value is what SettingService.getSetting() returns to clients, not just what Redis stores. The admin canary settings UI reads that response and, when toggling the canary switch, posts spaceIds: canaryConfig?.spaceIds ?? [], so with an existing canary list a save under FORCE_V2_ALL overwrites the DB with an empty list. Keep the lean cache separate from the API/model value or omit spaceIds on unrelated updates.
Useful? React with 👍 / 👎.
tea-artist
changed the title
c88d1ad to
3a17e14
Compare
tea-artist
changed the title
3a17e14 to
13870f4
Compare
tea-artist
changed the title
13870f4 to
ce9b62f
Compare
tea-artist
changed the title
ce9b62f to
b81a1d2
Compare
tea-artist
changed the title
b81a1d2 to
6a80708
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a807086cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Central policy switch: with the escape hatch on, nothing counts as a | ||
| // cross-space target, which disables the create-forbid and the | ||
| // field-duplicate downgrade in one place. | ||
| if (isCrossSpaceReferenceAllowed()) return false; |
There was a problem hiding this comment.
When ALLOW_CROSS_SPACE_REFERENCE=true, this returns false before checking where the two tables live. The only data-DB guard added in this change blocks starting a new space migration, so an instance with spaces already bound to different data databases can now create a link/lookup across those spaces; later reads and computed updates route by one table’s data DB and try to join the foreign table on that same connection, where it does not exist. Please keep the escape hatch limited to pairs that share the same physical data DB, or reject it for already-migrated spaces.
Useful? React with 👍 / 👎.
| if (!share) return ''; | ||
| return getShareUrl(share.shareId); | ||
| }, [share]); | ||
| return shortUrl ?? (isFallback ? getShareUrl(share.shareId, origin) : ''); |
There was a problem hiding this comment.
When a base share already exists, useShortLink initially returns no shortUrl and no error, so this passes '' into BaseShareContent; that component always renders EmbedConfigPopover, which calls new URL(shareUrl) during render and throws on the empty string. Opening an existing base share dialog, or the duplicated node-share path, can therefore crash before the short-link request finishes; keep using the long URL or gate the embed controls while loading.
Useful? React with 👍 / 👎.
tea-artist
changed the title
6a80708 to
9e4a2f7
Compare
tea-artist
changed the title
9e4a2f7 to
3daac16
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3daac1639e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ICsvParser, | ||
| } from '@teable/v2-core'; | ||
| import { domainError } from '@teable/v2-core'; | ||
| import { safeFetch } from '@teable/v2-utils'; |
There was a problem hiding this comment.
When @teable/v2-container-browser is used, it imports PapaparseCsvParser, so this new root import pulls in @teable/v2-utils' ./ssrf barrel; that barrel also re-exports agents.ts/fetch.ts, which import Node-only request-filtering-agent and undici. Browser builds using the browser container now have those Node transport modules in the dependency graph and can fail to bundle or require unusable polyfills; import a browser-safe registry subpath or split the Node SSRF exports from the root barrel.
Useful? React with 👍 / 👎.
| ? { type: ShortLinkType.Template, resourceId: templateDetail.id } | ||
| : undefined | ||
| ); | ||
| const displayShareUrl = shortUrl ?? (isFallback ? shareUrl : ''); |
There was a problem hiding this comment.
After publishing a previously unpublished base, onSuccess sets shareUrl and immediately opens the success dialog, but templateDetail has not refetched as published yet, so useShortLink is disabled and isFallback is false. This expression therefore returns '', making the success dialog, copy button, external open, and social share actions use an empty URL until a later refetch/short-link request completes; keep the permalink from shareUrl in this loading/disabled state.
Useful? React with 👍 / 👎.
tea-artist
changed the title
3daac16 to
6bc7755
Compare
tea-artist
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 087f28c1f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map((fieldDbName) => `coalesce(${quoteIdentifier(fieldDbName)}::text, '')`) | ||
| .join(` || ' ' || `); | ||
| return `to_tsvector(${quoteLiteral(languageConfig)}::regconfig, ${document || quoteLiteral('')})`; | ||
| .join(` || E'\\n' || `); | ||
| return `lower(${document || quoteLiteral('')})`; |
There was a problem hiding this comment.
When V2_TABLE_QUERY_OPS_SEARCH_ACCESS_PATH_RUNTIME=auto uses a generated-text access path that covers a long-text field, this builds the indexed document from the raw column text, but the default search predicate in buildFieldSearchCondition replaces CR/LF/tabs with spaces before applying ILIKE. A cell containing foo\nbar therefore matches a search for foo bar on the default path, while the generated document fails the LIKE lower('%foo bar%') prefilter and the row is excluded before the exact predicate can recheck it. Use the same long-text projection as the default search builder, or avoid the prefilter for fields whose document text is not a superset of the exact predicate.
Useful? React with 👍 / 👎.
| this.configService.get('V2_TABLE_QUERY_OPS_SEARCH_ACCESS_PATH_RUNTIME') ?? | ||
| this.configService.get('V2_TABLE_QUERY_OPS_SEARCH_VECTOR_RUNTIME') |
There was a problem hiding this comment.
When V2_TABLE_QUERY_OPS_SEARCH_ACCESS_PATH_RUNTIME=auto enables the task worker without manual index execution, this new env path makes searchVectorRuntimeEnabled true, but the default allowed kinds below still only include rebuild_search_vector. The schema-change scheduler now queues rebuild_search_access_path, and claimNextAccepted filters by allowedKinds, so those rebuilds are never claimed and the search config stays rebuild_pending instead of returning to ready. Include the new access-path rebuild kind in the default and parsed allowlists.
Useful? React with 👍 / 👎.
| (recordSearchAccessPath?.kind === 'generated_tsvector' || | ||
| recordSearchAccessPath?.kind === 'generated_text') && | ||
| query.search && | ||
| query.includeQueryExtra !== true |
There was a problem hiding this comment.
When generated_text is selected for a searched V2 records request that also has groupBy or collapsedGroupIds, this early return skips getQueryExtra() entirely unless the caller explicitly sets includeQueryExtra=true. That drops extra.groupPoints/allGroupHeaderRefs even though the returned record ids are still grouped, so grouped REST clients cannot render the matching group headers during search; keep loading the group extras and only skip the expensive search-hit extra, or make grouped requests opt in automatically.
Useful? React with 👍 / 👎.
tea-artist
changed the title
087f28c to
3a8b2c4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a8b2c43b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ssrDataFresh = | ||
| initialAnchorRef.current.tableId === tableId && initialAnchorRef.current.viewId === viewId; |
There was a problem hiding this comment.
When the user shallow-navigates away from the page-load table/view and later returns to that same initial pair, this expression becomes true again, so useTableSeed is disabled and the providers are seeded from the original page-load props instead of a fresh bounded seed. In a long-lived session this can briefly show obsolete fields/records or allow interacting with stale doc-less records until the ShareDB query catches up; track that the user has left the initial anchor once, or invalidate the SSR snapshot on the first anchor change.
Useful? React with 👍 / 👎.
tea-artist
changed the title
3a8b2c4 to
014e76c
Compare
tea-artist
changed the title
014e76c to
e251ac4
Compare
tea-artist
changed the title
e251ac4 to
381b80d
Compare
tea-artist
changed the title
381b80d to
fab7775
Compare
tea-artist
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfa173325b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const strippedFieldOptions = stripLookupFormulaExecutableOptions(field.options) ?? {}; | ||
| const { | ||
| formatting = inheritableOptions.formatting, | ||
| showAs, | ||
| ...inheritOptions | ||
| } = field.options as Record<string, unknown>; | ||
| } = strippedFieldOptions; |
There was a problem hiding this comment.
When a regular lookup field has explicitly cleared an inherited display mode (options.showAs === null), this newly stripped field.options path extracts that null into showAs, but the later options rebuild only spreads showAs when it is truthy. If the linked/source field changes and updateLookupField runs, the null tombstone is dropped from the persisted options; after reload the lookup can inherit the source field's showAs again, undoing the user's clear. Preserve the property based on ownership (including null) rather than truthiness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc1823befc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| await this.checkInvitationLimits(); | ||
|
|
||
| const departmentIds = this.cls.get('organization.departments')?.map((d) => d.id); |
There was a problem hiding this comment.
When MAX_INVITATIONS_PER_HOUR is configured, email invitations now proceed without any quota check because the only call to the removed checkInvitationLimits() was deleted and a repo-wide search shows no remaining reference to that env var. In deployments that relied on this setting for anti-abuse, a user can send unlimited invitations instead of being blocked/deactivated after the hourly threshold; keep the check here or replace it with an equivalent risk-control path.
Useful? React with 👍 / 👎.
| SELECT EXISTS ( | ||
| SELECT 1 | ||
| FROM computed_task_field_ref AS refs | ||
| WHERE refs.table_id = ${tableId} |
There was a problem hiding this comment.
This new reconciliation path runs on every compute-activity read and filters computed_task_field_ref by table_id, but the migration only creates computed_task_field_ref_field_id_idx, not a table_id index. On instances with many queued/completed compute refs, each healthy poll for any table has to scan the whole ref table (twice, including the grouped check below), which can make the diagnostics endpoint and toolbar polling expensive; add an index such as (table_id) or (table_id, field_id) with this query.
Useful? React with 👍 / 👎.
| value.trim() | ||
| ); | ||
| const exactOrigins = normalizeOrigins([ | ||
| publicOrigin, |
There was a problem hiding this comment.
When PUBLIC_ORIGIN is an internal or loopback app URL, an authenticated URL import can submit an absolute URL on that origin such as /admin; attachmentUrlSchema accepts absolute http(s) URLs, and adding publicOrigin to exactOrigins makes createTrustedUrlPredicate() bypass the filtering agent for every path on that origin. This reopens the app-path SSRF case the schema is trying to block, so keep PUBLIC_ORIGIN limited to the attachment-read path unless an origin was explicitly configured as fully trusted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c7d342c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.isLookup) { | ||
| return []; |
There was a problem hiding this comment.
When a regular lookup targets a formula (type === Formula and isLookup), this branch makes getReferenceFields() empty; because FormulaFieldCore.getLinkFields() still derives link dependencies only from those reference fields, it no longer returns the lookupOptions.linkFieldId. The record CTE builder relies on field.getLinkFields(this.table) to emit hidden link CTEs before resolving projected lookups, so lookup-formula values whose link field is hidden/not projected can be resolved without the required join and come back null. Keep expression references suppressed, but have getLinkFields() fall back to the base lookupOptions path for lookup fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e82e385dc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.eventEmitterService.emitAsync( | ||
| Events.COLLABORATOR_CREATE, | ||
| new CollaboratorCreateEvent(base.spaceId, { | ||
| resourceId: baseId, | ||
| resourceType: CollaboratorType.Base, |
There was a problem hiding this comment.
When this helper is invoked inside existing transactions such as DashboardService.installPlugin (checked its $tx wrapper) or the plugin context-menu install flow, this new contextual event is emitted immediately after createMany, before the surrounding transaction performs later writes. If a later write rolls back, CollaboratorInviteNotificationListener can already have inserted/sent an invite notification for a collaborator that was never committed; either keep skipEvent for transactional callers and emit post-commit, or register the notification after the outer transaction commits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Move share cleanup after node deletion succeeds
If deleteResource() throws after this call (for example a table/drop or dashboard delete failure), the method exits before deleting the base_node, but deleteNodeShares() has already hard-deleted the node's base-share rows and marked their short links dead. That leaves the still-existing table/dashboard with its share links permanently revoked after a failed delete attempt; perform this cleanup only after the resource/node delete succeeds, or make the whole sequence transactional where possible.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const startIndex = queryRef.current.skip ?? 0; | ||
| const take = queryRef.current.take ?? LOAD_PAGE_SIZE; | ||
| lastMergedSkipRef.current = startIndex; | ||
| const isFirstFreshDelivery = pendingFreshRef.current && (extra != null || records.length > 0); |
There was a problem hiding this comment.
When switching back to a cached grid whose current result is empty and ungrouped, the first ShareDB ready event has records.length === 0 and usually no extra, so this condition never marks it as the authoritative fresh delivery. The merge below then copies the seeded preLoadedRecords back into state instead of clearing them, and settledRef never evicts that snapshot, leaving rows from the previous result visible until another non-empty/extra update arrives. Treat the first ready/changed delivery as fresh even when it is empty, or use an explicit readiness signal, before retaining cached rows.
Useful? React with 👍 / 👎.
| // previous view's structure — writing it here would poison the new | ||
| // view's slot, so skip until state and key agree again | ||
| if (keyChanged) return; | ||
| if (view?.id && groupPoints != null) { |
There was a problem hiding this comment.
When a view that has cached groupPoints is later changed to an ungrouped view, the fresh subscription result typically has no groupPoints (and may have no extra), so this guard skips updating the cache. The old grouped structure remains in useGridViewCacheStore and is seeded the next time the user returns to the view, causing stale group headers/layout to appear for an ungrouped result until another grouped delivery overwrites it; clear the cached group facet when the authoritative result has no group points.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 388019c370
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { provider, publicUrl, publicBucket, privateBucket, privateBucketEndpoint, s3, minio } = | ||
| storageConfig(); |
There was a problem hiding this comment.
When local storage is used and BACKEND_STORAGE_ENCRYPTION_KEY/IV is rotated while Redis/SQLite preview cache survives a deploy, this fingerprint does not change because it omits storageConfig().encryption. LocalStorage.getPreviewUrl() embeds an encrypted read token in the cached URL, so after rotation verifyReadToken() cannot decrypt the old token and attachment previews keep returning broken cached URLs until the preview cache TTL expires; include the encryption settings (digesting secrets like the S3/MinIO keys) in the config signature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55b8263fd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Promise.resolve() | ||
| .then(() => view.updateColumnMeta(columnMetaRo)) | ||
| .catch(() => { |
There was a problem hiding this comment.
When a user performs a second drag before the first updateColumnMeta request settles, this launches another independent write whose order was calculated from the first drag's optimistic state, while persisting only the field moved by the second drag. If the first request then fails or the requests are applied out of order, the second write is committed against server ordering that never contained its prerequisite move, and the first rejection also clears the newer optimistic state, leaving a different column order than the user selected. Serialize these writes or persist/rollback each optimistic ordering as one versioned operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5641a1e88d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { userId } = resetPasswordEmail; | ||
| // Consume the one-time code atomically before updating the password so | ||
| // concurrent requests carrying the same code cannot both reset it. | ||
| const consumed = await this.cacheService.del(`reset-password-email:${code}`); |
There was a problem hiding this comment.
If password hashing or the database update fails after this deletion (for example, during a transient database outage), the user's one-time code is permanently consumed even though their password remains unchanged. A retry then reports that the token is invalid and forces the user to request another email; preserve or restore the token until the password update succeeds while still using an atomic claim to reject concurrent requests.
Useful? React with 👍 / 👎.
| for (const parameter of prismaOnlySearchParams) { | ||
| url.searchParams.delete(parameter); | ||
| } |
There was a problem hiding this comment.
When the database URL selects a non-public schema with ?schema=..., removing that parameter from the actual pg connection leaves the session search path at its default. The schema passed to PrismaPg qualifies generated Prisma operations, but the many unqualified $queryRawUnsafe statements and registry-backed Knex queries (for example, PinService.fetchWorkflows() querying workflow) still execute against public, causing missing-relation errors or reads from the wrong schema. Configure the pooled connections' search path while continuing to exclude the Prisma-only URL parameter.
Useful? React with 👍 / 👎.
| const { rows, normalizedRows } = await this.dataDbClientManager.withDataKnexConnectionForBase( | ||
| sourceBaseId, | ||
| async (dataKnex, connection) => { | ||
| const rows = await dataKnex<Record<string, unknown>>(sourceDbTableName) | ||
| .connection(connection) |
There was a problem hiding this comment.
When source and destination bindings use different connection records or schemas on the same PostgreSQL endpoint and BYODB_DATA_DB_POOL_MAX=1, the V2 link-restoration phase consumes this source generator from inside DuplicateBaseHandler.restoreLinkFieldsStream()'s destination transaction. Because the new registry merges those clients onto one pool, this call waits for a second connection while the sole connection is held by that transaction, so base duplication stalls until timeout. Materialize each source batch before opening the destination transaction or reuse its connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e6ddf8411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| () => createScopedDataPrismaClient(resolved.url, resolved.internalSchema), | ||
| async () => { | ||
| const poolLease = this.pgPoolRegistry.acquire(resolved.url, { | ||
| max: Number(process.env.BYODB_DATA_DB_POOL_MAX ?? 5), |
There was a problem hiding this comment.
When a BYODB connection URL specifies a lower connection_limit and BYODB_DATA_DB_POOL_MAX is unset, this always passes max: 5; PgPoolRegistry.normalizePoolConfig() prioritizes options.max over the URL parameter, so the URL limit is never considered. Under concurrent traffic, installations relying on a tenant/database connection cap such as connection_limit=1 can now open up to five connections and hit provider limits. Preserve the URL value before applying the default of five; the Knex and V2-container acquisition paths use the same unconditional fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcb6524809
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .selectFrom('computed_field_activity as activity') | ||
| .leftJoin(refs, 'refs.field_id', 'activity.field_id') |
There was a problem hiding this comment.
When an active computed_task_field_ref survives but its computed_field_activity row is missing, this activity-first left join never returns that ref, while the earlier dangling check also considers it valid because its outbox task exists. shouldReconcile() therefore reports the snapshot as healthy, leaving the active calculation absent from diagnostics and realtime state indefinitely; drive this comparison from the authoritative refs (or explicitly check for refs without activity rows) and ensure reconciliation can recreate the missing projection.
Useful? React with 👍 / 👎.
| for (const parameter of prismaOnlySearchParams) { | ||
| url.searchParams.delete(parameter); | ||
| } |
There was a problem hiding this comment.
When a PostgreSQL URL sets Prisma's pool_timeout, normalization deletes it before constructing the shared pg pool and no equivalent checkout timeout is configured. Once all max connections are occupied, queries that previously failed after the configured deadline can remain queued indefinitely, potentially hanging request handlers and workers during pool saturation; translate this value into the shared pool's acquisition-timeout behavior instead of silently discarding it.
Useful? React with 👍 / 👎.
| minio.endPoint, | ||
| minio.port, | ||
| minio.useSSL, | ||
| minio.accessKey, | ||
| digest(minio.secretKey), |
There was a problem hiding this comment.
When BACKEND_STORAGE_MINIO_REGION changes while the preview cache survives a deployment, this fingerprint remains unchanged even though MinioStorage passes that region into the client that signs preview URLs. Cached URLs can therefore retain the old region in their signature and fail authentication until their existing TTL expires; include minio.region in the fingerprint so region changes invalidate them immediately.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,3 @@ | |||
| -- AlterTable (idempotent: safe to replay on databases where the columns already exist) | |||
| ALTER TABLE "task_run" ADD COLUMN IF NOT EXISTS "base_id" TEXT; | |||
| ALTER TABLE "task_run" ADD COLUMN IF NOT EXISTS "depends_on_run_ids" TEXT[] DEFAULT ARRAY[]::TEXT[]; | |||
There was a problem hiding this comment.
On databases created or upgraded through this migration, depends_on_run_ids remains nullable even though both Prisma schemas declare dependsOnRunIds as the required String[] type and the Kysely schema also models it as non-null. This leaves the deployed schema in drift and allows raw or legacy writes to store NULL, which Prisma callers may receive where they expect an array; add NOT NULL alongside the empty-array default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e55d231a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // on the next remount. | ||
| const clearAlerts = useCallback(() => { | ||
| setImportantNotifications([]); | ||
| toast.dismiss(); |
There was a problem hiding this comment.
When the user selects “mark all as read” while any unrelated application toast is visible, this unscoped toast.dismiss() removes every Sonner toast, including error or success messages from other workflows. Track and dismiss the notification toast IDs instead so clearing notification alerts does not erase unrelated feedback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bc302591c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!share) return ''; | ||
| return getShareUrl(share.shareId); | ||
| }, [share]); | ||
| return shortUrl ?? (isFallback ? getShareUrl(share.shareId, origin) : ''); |
There was a problem hiding this comment.
When an existing whole-base share dialog opens, useShortLink initially has neither shortUrl nor an error, so this expression returns '' despite the valid long URL already being derivable from share.shareId. The URL input, copy button, and QR code are therefore empty until the request finishes; NodeShareContent repeats the same behavior for node shares. Use the long URL while loading, or disable those controls until the short URL is available.
Useful? React with 👍 / 👎.
| export const getPreviewUrlConfigSig = () => { | ||
| const { provider, publicUrl, publicBucket, privateBucket, privateBucketEndpoint, s3, minio } = | ||
| storageConfig(); |
There was a problem hiding this comment.
When local storage is used and STORAGE_PREFIX or its PUBLIC_ORIGIN fallback changes while the preview cache survives a deployment, this signature remains unchanged because it only reads storageConfig(). LocalStorage.getPreviewUrl() prepends baseConfig.storagePrefix for API-originated requests, so cached preview URLs continue pointing at the old origin until their existing TTL expires; include that prefix in the fingerprint.
Useful? React with 👍 / 👎.
| // Central policy switch: with the escape hatch on, nothing counts as a | ||
| // cross-space target, which disables the create-forbid and the | ||
| // field-duplicate downgrade in one place. | ||
| if (isCrossSpaceReferenceAllowed()) return false; |
There was a problem hiding this comment.
When ALLOW_CROSS_SPACE_REFERENCE=true is enabled after one or more spaces have already been migrated to per-space data databases, this unconditional return permits links between tables that can reside in different PostgreSQL databases. The new migration preflight guard only prevents migrations started after the flag is enabled; it does not protect existing bindings, so link junction/FK creation and subsequent computed updates can target relations that are absent from the routed database. Check that both tables resolve to the same data database before bypassing the cross-space restriction, or reject the flag whenever per-space bindings exist.
Useful? React with 👍 / 👎.
| declaredEventName === Events.BASE_CREATE && typeof reqBody?.baseId === 'string' | ||
| ? Events.BASE_UPDATE |
There was a problem hiding this comment.
When create-from-template receives the schema-valid payload baseId: '', BaseService.createBaseFromTemplate() treats that value as absent and creates a new base, but this typeof check reclassifies the successful operation as BASE_UPDATE. That gives event consumers and the audit/action-trigger trail an update for a resource that was actually created; use the same nonempty check as the service or reject empty base IDs in the request schema.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63485bb388
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| db.oid::text AS "databaseOid", | ||
| current_database()::text AS "databaseName" | ||
| FROM pg_catalog.pg_database AS db | ||
| CROSS JOIN pg_catalog.pg_control_system() AS control |
There was a problem hiding this comment.
When an existing BYODB connection uses a normal non-superuser role, this query fails because PostgreSQL restricts pg_control_system() by default. assertExistingBindingDatabaseIdentityUnchanged() catches that failure and rejects the update, so users of managed or least-privilege databases cannot switch the connection to PgBouncer even when both URLs reach the same database. Verify identity using data accessible to the supported BYODB role, or persist a stable installation identifier in Teable's internal schema.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02095e66b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function createStrictSsrfAgent(): Agent { | ||
| const baseConnector = buildConnector({}); | ||
| const connector: buildConnector.connector = (options, callback) => { | ||
| baseConnector(options, (err, socket) => { |
There was a problem hiding this comment.
When a user-supplied import URL resolves to a loopback or RFC1918 address, baseConnector has already completed the TCP/TLS connection before this callback checks socket.remoteAddress. The guarded fetch therefore still reaches internal ports, enabling port probing and potentially triggering connect-time behavior even though it destroys the socket before sending HTTP data; resolve and reject non-public addresses in the lookup/connect path before opening the socket.
Useful? React with 👍 / 👎.
| const classification = classifyDataDbRuntimeError(error); | ||
| if (!classification || classification.retryable) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
When a permanent base or table deletion reaches a live BYODB database but the configured role lacks DROP or DELETE privileges, the classifier returns data_db.permission_denied with retryable: false, so this helper swallows the error and the callers continue deleting Teable's metadata. The physical schema, table, history, or trash rows then remain orphaned with no metadata left to retry cleanup; only confirmed absence cases should be treated as successful best-effort cleanup, while permission failures must abort and remain retryable after grants are fixed.
Useful? React with 👍 / 👎.
… lists T6451 Synced from teableio/teable-ee@3a3cdbc Co-authored-by: Aries X <caoxing9@gmail.com> Co-authored-by: Bieber <artist@teable.io> Co-authored-by: Boris <boris2code@outlook.com> Co-authored-by: Jocky Zhou <jocky@teable.ai> Co-authored-by: Jun Lu <hammond@teable.io> Co-authored-by: Pengap <penganpingprivte@gmail.com> Co-authored-by: SkyHuang <sky.huang.fe@gmail.com> Co-authored-by: Uno <uno@teable.ai> Co-authored-by: nichenqin <nichenqin@hotmail.com> Co-authored-by: teable-mini[bot] <310066019+teable-mini[bot]@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9d1874f4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const eventName = | ||
| declaredEventName === Events.BASE_CREATE && typeof reqBody?.baseId === 'string' | ||
| ? Events.BASE_UPDATE |
There was a problem hiding this comment.
When POST /base/duplicate supplies its schema-valid optional baseId, that value is the requested ID of the newly created copy (duplicateBaseV2 passes it to createBaseV2), not an existing base being updated. Because every base-creation route passes through this condition, such duplications now emit BASE_UPDATE instead of BASE_CREATE, giving event and audit consumers the wrong operation type. Restrict this reclassification to the create-from-template route rather than inferring it solely from the request body.
Useful? React with 👍 / 👎.
| const expectedTableStatus = | ||
| queuedFieldCount + calculatingFieldCount > 0 ? 'calculating' : 'idle'; | ||
| return Boolean( | ||
| snapshot.table && |
There was a problem hiding this comment.
When the computed_table_activity row is absent but the table still has computed_field_activity rows whose counters agree with their refs, this guard makes the final drift check false solely because snapshot.table is null. The reader therefore never calls reconcileTable(), even though that method's field-target path recreates the table aggregate via syncFromTaskRefs; HTTP and realtime consumers remain without table metadata until another compute transition happens. Treat a missing table row as reconciliation-worthy whenever field activity exists.
Useful? React with 👍 / 👎.
🧹 Preview Environment Cleanup
|
3 participants
🔄 Automated sync from EE repository.
72 commit(s) synced since last sync.
Authors
Included commits
Latest source commit: teableio/teable-ee@3a3cdbc
This PR was automatically created by the sync workflow.