[sync] feat(admin): add system check to detect and repair users without avatars T6492 - #3620
[sync] feat(admin): add system check to detect and repair users without avatars T6492#3620tea-artist wants to merge 1 commit into
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: 5d24ae0c57
ℹ️ 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 userCount = members.filter((m) => m.principalType === PrincipalType.User).length; | ||
| if (await confirmSeat({ role: role as IRole, count: userCount, action: 'invite' })) { |
There was a problem hiding this comment.
When the organization selection contains only departments, userCount is zero, so useSeatConfirm returns immediately and skips both the cloud billing confirmation and the EE license limit. Department collaborators grant access to their users—the backend includes a user's organization departments when resolving collaborators—so assigning a billable role to a department can consume multiple seats. Resolve the department's newly billable members instead of filtering departments out; the same zero-seat assumption is present in the base invitation and department role-change handlers.
Useful? React with 👍 / 👎.
| onCreate={async (ro) => { | ||
| if (!(await confirmSeat({ role: ro.role, count: ro.emails.length, action: 'invite' }))) { | ||
| return false; |
There was a problem hiding this comment.
On an EE instance, ro.emails.length is not necessarily the number of new seats. For example, inviting a user who is already billable through another base or space creates a new collaborator without increasing the instance-wide billable-user count. If the instance is at its seat limit, this calculation rejects that valid invitation because seats + emails.length exceeds the limit. The confirmation needs a server-derived count of newly billable users; the base email flow has the same issue.
Useful? React with 👍 / 👎.
| onUpdate={async (invitationId, role) => { | ||
| if (await confirmSeat({ role, count: 1, action: 'link' })) { | ||
| updateInviteLink({ |
There was a problem hiding this comment.
Updating an existing invitation link from one billable role to another, such as Creator to Editor, does not add a collaborator or consume a seat—the backend only updates the invitation row. Nevertheless this always checks with count: 1, so an EE instance at its seat limit cannot make that role adjustment, and cloud users receive an inaccurate bill-increase confirmation. Compare the existing link role and apply a positive delta only when the transition actually introduces billable access; the base-link handler repeats this behavior.
Useful? React with 👍 / 👎.
| const { confirm, alert } = useConfirm(); | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| const isPaidSpace = isCloud && level != null && level !== BillingProductLevel.Free; |
There was a problem hiding this comment.
On Cloud, level is undefined while useBillingLevel is still fetching, and it remains undefined if that query fails. This classifies the space as unpaid; because isEE is also false, the callback falls through to true. A paid customer who submits an invitation or role change before the usage query resolves therefore bypasses the required bill-increase confirmation. Await or block on the billing query rather than treating an unknown level as Free.
Useful? React with 👍 / 👎.
| queryFn: () => getInstanceUsage().then((res) => res.data), | ||
| staleTime: 0, | ||
| }) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
When the instance-usage request fails, this catch converts the error to undefined, leaving seatLimit absent and allowing the callback to return true. During any transient usage-service or network failure, the self-hosted seat-limit dialog and the stated hard stop are therefore bypassed, and the billable mutation is attempted without knowing whether capacity remains. Surface the failure or fail closed instead of silently authorizing the action.
Useful? React with 👍 / 👎.
Coverage Report for CI Build 30802930988Coverage at 57.238% (no base build to compare)Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
tea-artist
changed the title
5d24ae0 to
f0f79a9
Compare
tea-artist
changed the title
f0f79a9 to
e600e6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e600e6e93d
ℹ️ 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; | ||
| } | ||
| if (knownCount > 0) { | ||
| ctx.nextGroupPoints.push({ type: GroupPointType.Row, count: knownCount }); |
There was a problem hiding this comment.
When a multi-level group's parent is collapsed, its descendant headers are removed from groupPoints; re-expanding that parent here restores only one aggregate row block and cannot reconstruct those headers. The locally patched state therefore flattens the parent, and useGridAsyncRecords subsequently caches that malformed structure, so on a slow or failed fresh delivery the child groups remain missing and cannot be expanded or collapsed. Preserve the cached subtree for parent expansion, or avoid applying the in-place expansion when only an aggregate count is available.
Useful? React with 👍 / 👎.
| // deliveries re-send a structurally identical list on every page (and | ||
| // after an exact local collapse patch): keep the previous reference so | ||
| // the grid does not rebuild its O(total rows) linear layout for nothing | ||
| setGroupPoints((prev) => (isEqual(prev, freshGroupPoints) ? prev : freshGroupPoints)); |
There was a problem hiding this comment.
After a same-view filter, sort, or row-hiding search change, the layout effect clears groupRowCounts; if the authoritative delivery has structurally equal group points—which is common when sorting within groups or when a filter preserves each group's count—this equality branch retains the old state reference, so the effect that collects counts never runs again. Collapsing a group then omits its now-missing count, and re-expanding it takes the unknown-size path that drops every loaded row behind it until another delivery arrives. Refresh the count cache from every authoritative delivery even when the React layout state can retain its reference.
Useful? React with 👍 / 👎.
tea-artist
changed the title
e600e6e to
49b46d0
Compare
tea-artist
changed the title
49b46d0 to
d6ab6e0
Compare
tea-artist
changed the title
d6ab6e0 to
bee7cb1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bee7cb196b
ℹ️ 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".
| }) | ||
| } | ||
| onCreate={async (role) => { | ||
| if (await confirmSeat({ role, count: 1, action: 'link' })) { |
There was a problem hiding this comment.
On an EE instance already at its seat limit, this count: 1 makes useSeatConfirm reject creating a billable-role link even though link creation consumes no seat: InvitationService.generateInvitationLink only creates the invitation, while a collaborator is added later by acceptInvitationLink. The eventual accepter may also already be billable, so capacity cannot be determined at link-creation time; retain the Cloud warning but do not apply the EE seat-delta gate here. The space-link creation handler has the same problem.
Useful? React with 👍 / 👎.
| const isInIframe = useIsInIframe(); | ||
| const [dismissed, setDismissed] = useState(false); | ||
|
|
||
| if (!isHydrated || !isMobile || isInIframe || dismissed) { |
There was a problem hiding this comment.
When a ?embed=... share URL is opened outside an actual iframe on a viewport at or below 640px, isInIframe is false and this condition renders the fixed edit/save bar. The share view components consistently treat the query parameter itself as embed mode and suppress their headers, so a direct embed preview or new-tab embed now gains an operation overlay despite requesting chrome-free embed output. Check router.query.embed here in addition to the window iframe state.
Useful? React with 👍 / 👎.
| const handleUpdateRole = useCallback( | ||
| (role: IRole, item: CollaboratorItem) => { | ||
| async (role: IRole, item: CollaboratorItem) => { | ||
| const addedSeats = item.type === PrincipalType.User && !item.billable ? 1 : 0; |
There was a problem hiding this comment.
For the base and space collaborator-list responses implemented in this tree, CollaboratorService.getListByBase and getListBySpace never populate the optional billable property, so every user reaches this expression with item.billable === undefined and is treated as a new seat. Consequently, changing an already billable user from Creator to Editor shows a false Cloud cost-increase confirmation and is blocked entirely on an EE instance at its seat limit. Populate the billing status or obtain the actual server-side seat delta before gating the role update; the invitation-dialog role handlers repeat this assumption.
Useful? React with 👍 / 👎.
tea-artist
changed the title
bee7cb1 to
f2f75f1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2f75f1457
ℹ️ 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 (connection?.saveData || connection?.effectiveType === '2g') { | ||
| return; |
There was a problem hiding this comment.
When the Network Information API reports effectiveType === 'slow-2g' and data saver is disabled, this condition still schedules the heavy Table chunk download. Since slow-2g is an explicit effective-connection type and is slower than 2g, the users most affected by this speculative download bypass the protection described by the hook; exclude both 2g and slow-2g before warming the bundles.
Useful? React with 👍 / 👎.
tea-artist
changed the title
f2f75f1 to
a937e10
Compare
tea-artist
changed the title
a937e10 to
0985fee
Compare
tea-artist
changed the title
0985fee to
7a06430
Compare
tea-artist
changed the title
7a06430 to
0fe137f
Compare
tea-artist
changed the title
0fe137f to
becce22
Compare
tea-artist
changed the title
becce22 to
d75e6a3
Compare
tea-artist
changed the title
tea-artist
changed the title
d9adb85 to
cec5ffb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec5ffb66f
ℹ️ 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".
| 'ViewColumnMetaUpdated', | ||
| 'ViewRenamed', | ||
| 'ViewDescriptionUpdated', | ||
| 'ViewLockedUpdated', | ||
| 'ViewOrderUpdated', |
There was a problem hiding this comment.
When a V2 refresh-share-id or disable-share request revokes a share that already has a cached short link, ViewShareIdRefreshed and ViewShareDisabled are absent from this awaitable set. Their new cleanup projection is therefore deferred until after the HTTP response, so an immediate short-link lookup can still return the cached path instead of the expected 404 (the cache otherwise lasts 60 seconds). Include these revocation events in the synchronous projection path so a successful response guarantees that the old short link has been invalidated.
Useful? React with 👍 / 👎.
| async updateShareMeta( | ||
| tableId: string, | ||
| viewId: string, | ||
| shareMeta: IViewShareMetaRo | ||
| ): Promise<void> { |
There was a problem hiding this comment.
When the canary selects V2, updating share metadata or refreshing, enabling, or disabling a share bypasses the corresponding V1 methods decorated with @Audit for SHARED_VIEW_UPDATE, SHARED_VIEW_REFRESH, SHARED_VIEW_CREATE, and SHARED_VIEW_DELETE. The new domain events have realtime and short-link projections but no bridge to AuditScope or the legacy event emitter, so these successful mutations disappear from audit history and action subscribers. Add equivalent audit emission around the V2 methods or a projection that records the same masked payloads.
Useful? React with 👍 / 👎.
| const records = result.value.records.map((record) => { | ||
| const dto = mapTableRecordToDto(record); | ||
| if (dto.isErr()) this.throwDomainError(dto.error); | ||
| return dto.value; | ||
| }); | ||
| return { countMap: { ...result.value.countMap }, records }; |
There was a problem hiding this comment.
When a shared calendar includes an attachment field, this direct DTO mapping returns the stored attachment token/path without the fresh presignedUrl and thumbnail URLs supplied by both the V1 getRecordsById path and the normal V2 record presentation pipeline. Consumers opening an event from the calendar payload therefore cannot preview or download its attachments until another record read replaces the seeded data. Run these records through the V2 attachment and user-cell presentation enrichment before returning them.
Useful? React with 👍 / 👎.
tea-artist
changed the title
cec5ffb to
b096508
Compare
tea-artist
changed the title
b096508 to
4816885
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4816885c84
ℹ️ 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 clearedAtSec = await this.cacheService.get(`auth:session-user-cleared:${userId}`); | ||
| if (clearedAtSec && this.sessionRenewedAtSec(session) <= clearedAtSec) { |
There was a problem hiding this comment.
When an existing session request begins before clearByUserId (for example during a password change) but completes a rolling touch after the clear marker is written, its cookie-derived renewal time becomes newer than clearedAtSec. Once the temporary expire key lapses, a concurrent map update that dropped this SID causes this branch to repair and accept the old session instead of revoking it, defeating the intended logout-all behavior. Session generations need to be immutable across touches or otherwise tied atomically to the clear operation.
Useful? React with 👍 / 👎.
| const { confirm, alert } = useConfirm(); | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| const isPaidSpace = isCloud && level != null && level !== BillingProductLevel.Free; |
There was a problem hiding this comment.
When a paid-cloud collaborator dialog is opened before useBillingLevel has returned—most notably the base dialog, whose base-usage query starts client-side—level is temporarily undefined, so isPaidSpace is false and the callback immediately permits a billable invite or role change without showing the new charge confirmation. Await the billing lookup at decision time or keep billable actions gated while the level is unresolved.
Useful? React with 👍 / 👎.
tea-artist
changed the title
4816885 to
55b3123
Compare
tea-artist
changed the title
55b3123 to
1b3ef49
Compare
There was a problem hiding this comment.
💡 Codex Review
Block record reorders during data-database migration
When a space migration is in freezing_writes/switching or a base database move is copying or validating data, this new V2 path executes the reorder without calling SpaceDataDbMigrationGuardService. The equivalent V1 method calls assertTableWritable, and normal V2 record mutations call assertTableRecordWritable; reordering also writes the hidden row-order column in the data database, so a request accepted after the copy snapshot can update the old source and disappear when the binding switches to the destination. Call assertTableRecordWritable(tableId) before resolving and executing the command.
ℹ️ 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".
| @@ -73,6 +78,20 @@ const quoteLiteral = (value: string): string => `'${value.replace(/'/g, "''")}'` | |||
| const ISO_DATE_OR_DATETIME_SQL_REGEX = | |||
| '^[0-9]{4}-[0-9]{2}-[0-9]{2}([T ][0-9]{2}:[0-9]{2}(:[0-9]{2}(\\.[0-9]+)?)?([Zz]|[+-][0-9]{2}(:?[0-9]{2})?)?)?$'; | |||
|
|
|||
| const buildRatingConversionExpression = (valueExpression: string, max: number): string => | |||
| `CASE WHEN (${valueExpression}) IS NULL OR ROUND((${valueExpression})::double precision) < 1 THEN NULL ELSE LEAST(ROUND((${valueExpression})::double precision), ${max}) END`; | |||
There was a problem hiding this comment.
When a number, text, lookup, or formula value lies exactly on a half step, this expression can disagree with the API typecast path: PostgreSQL's round(double precision) uses platform-dependent tie rounding—commonly ties-to-even, making 2.5 become 2 and 0.5 become 0—whereas FieldToSpecVisitor uses Math.round, producing 3 and 1. Consequently, converting an existing field can persist different ratings from writing the same values through typecast: true, including turning 0.5 into NULL; cast to numeric or otherwise implement the same deterministic half-up rule here.
Useful? React with 👍 / 👎.
| const symmetricFieldId = field.symmetricFieldId(); | ||
| return symmetricFieldId |
There was a problem hiding this comment.
When the deleted view is used as filterByViewId by a one-way link field in another table, that field has no symmetricFieldId, so this collector drops the dependency entirely. The legacy deletion path explicitly searches incoming foreign link fields and clears matching filters; without that equivalent, the stale field configuration survives and GetViewLinkRecordsHandler subsequently tries to load the deleted view and returns view.not_found, breaking the link candidate picker until the field is manually reconfigured. Discover and clear incoming one-way fields as part of the deletion transaction.
Useful? React with 👍 / 👎.
tea-artist
changed the title
1b3ef49 to
6f60eb1
Compare
There was a problem hiding this comment.
💡 Codex Review
Do not toast success for partial field restores
When one or more restore batches emit errors but the stream still supplies its final done event, this branch correctly sets the dialog status to partial and then unconditionally displays the fully successful restore toast. Users who dismiss or overlook the dialog are told the operation succeeded even when updatedCount is below totalCount; only show this success toast when errors is empty, and report the partial result otherwise.
teable/apps/nestjs-backend/src/features/share/share.controller.ts
Lines 310 to 311 in 6f60eb1
Preserve the V2 shared-button run ID
When the canary selects the new V2 button-click path for a shared view, ShareService.buttonClick returns the workflow run ID produced by ClickButtonHandler, but this response overwrites it with an empty string. useButtonClickStatus uses that ID to mark the button as running and correlate realtime completion events, so shared-view users receive no running or success/failure status even though the workflow was launched. Only supply the legacy empty fallback for V1, or return result unchanged when V2 is active.
ℹ️ 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 handleSignIn = () => { | ||
| const loginUrl = `/auth/login?redirect=${encodeURIComponent(router.asPath)}`; | ||
| router.push(loginUrl); | ||
| router.push(`/auth/login?redirect=${encodeURIComponent(window.location.href)}`); |
There was a problem hiding this comment.
When password login is disabled and exactly one social provider is configured, this absolute window.location.href is validated during login SSR by redirectSocialAuth; server-side isValidRedirectPath compares it with http://placeholder.local, rejects the production origin, and starts OAuth without a redirect_uri. The user therefore lands at the default destination instead of returning to the editable shared view. Preserve the previous relative router.asPath redirect or validate absolute URLs against the request origin.
Useful? React with 👍 / 👎.
| // Computed fields use the stored value unless the field is currently errored. | ||
| visitFormulaField(field: FormulaField): Result<AliasedRawBuilder<unknown, string>, DomainError> { | ||
| return this.selectColumn(field); | ||
| return this.selectComputedColumn(field); |
There was a problem hiding this comment.
When a computed field is marked errored while stale values remain in its physical column, this change projects the field as NULL, but record filters still go through TableRecordConditionWhereVisitor.resolveColumn and compare the physical column directly. A view or API filter such as isEmpty or equality against an old formula value therefore includes or excludes rows based on invisible stale data even though every returned cell is null. Build filter operands through the same effective stored-value expression used by projection, ordering, aggregation, and search.
Useful? React with 👍 / 👎.
| details: { | ||
| fieldId: this.fieldId().toString(), | ||
| count: currentCount, | ||
| maxCount, | ||
| i18nKey: 'httpErrors.field.button.clickCountReachedMaxCount', |
There was a problem hiding this comment.
When a V2 button has reached its maximum click count, this puts the translation key inside details, but throwV2Error only forwards error.localization, and the SDK now intentionally translates only that property. Non-English users therefore see the raw English domain message instead of the existing localized button error; the inactive-workflow and reset errors repeat the same construction. Set localization: { i18nKey: ... } on these domain errors rather than storing the key in diagnostic details.
Useful? React with 👍 / 👎.
tea-artist
changed the title
6f60eb1 to
edd2371
Compare
tea-artist
changed the title
edd2371 to
b38c4aa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b38c4aaaaf
ℹ️ 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".
| operator = 'is'; | ||
| value = value ? false : null; |
There was a problem hiding this comment.
When a V2 shared-view copy collapses an unchecked checkbox or false boolean-formula group, the grouped value is null/false and this assignment produces null, so the resulting is null condition selects the collapsed group instead of excluding it. The established V1 generateFilterItem behavior maps this bucket to true; use the actual complement so the copied range contains the visible checked group rather than hidden rows.
Useful? React with 👍 / 👎.
| let value = path[depth]; | ||
| let operator: RecordFilterCondition['operator'] = 'isNot'; |
There was a problem hiding this comment.
When a V2 shared-view copy collapses a date or date-time-formula group, aggregation supplies a value truncated to the field's display granularity, but this default isNot compares exact timestamps. For a daily or monthly group, records with non-boundary times therefore remain in the result and shift copied ranges onto rows hidden by the collapsed group. Build the same exactFormatDate exclusion used by the V1 collapsed-group path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50f04abd87
ℹ️ 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 baseSchema = z.string().transform((val) => (val === '' ? null : val)); | ||
| return ok(this.applyNullable(baseSchema, field.notNull().toBoolean())); |
There was a problem hiding this comment.
When a field has notNull enabled, inputs such as '' still pass this non-null z.string() schema and are then transformed to null; applyNullable only controls whether the input may be null and does not validate the transformed output. Strict V2 creates or updates can therefore reach persistence with null for a required text field and fail at the database constraint instead of returning the expected validation error. The same post-validation null conversion affects required checkbox, multiple-select, attachment, and multi-link fields, so required schemas need a refinement after normalization.
Useful? React with 👍 / 👎.
| const itemTimeIso = itemCreatedTime.toISOString(); | ||
| return [...found.values()].filter((row) => row.removedTime <= itemTimeIso); |
There was a problem hiding this comment.
When a restore stream partially succeeds and leaves its trash item behind, a restored record can be deleted again; after both snapshots are flushed, this point lookup retains only the newest cold row for that record ID and the subsequent timestamp filter discards it because it belongs to the later trash item. The older row that actually satisfies itemCreatedTime is never considered, so retrying the original restore silently omits that record and then deletes the trash item. Apply the item-time bound during the lookup rather than filtering the already reduced latest-row map.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 052f89e342
ℹ️ 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".
| DELETE FROM "task_run" WHERE "base_id" IS NULL; | ||
|
|
||
| ALTER TABLE "task_run" DROP CONSTRAINT IF EXISTS "task_run_base_id_not_null"; | ||
| ALTER TABLE "task_run" ADD CONSTRAINT "task_run_base_id_not_null" CHECK ("base_id" IS NOT NULL) NOT VALID; |
There was a problem hiding this comment.
When an old pod inserts a task_run without base_id after the DELETE completes but before this ADD CONSTRAINT obtains its lock, NOT VALID exempts that now-existing row from the initial check. The immediately following validation migration then finds the NULL and fails, blocking the rolling deployment in exactly the concurrent-old-pod scenario described above. Add the not-valid constraint before cleaning up existing rows so new NULL writes are rejected throughout the cleanup window.
Useful? React with 👍 / 👎.
| @UseV2Feature('buttonClick') | ||
| @UseGuards(V2FeatureGuard, ShareAuthGuard) | ||
| @UseInterceptors(V2IndicatorInterceptor) |
There was a problem hiding this comment.
When this canary selects V2 for a shared-view button click, ShareService.buttonClick now returns the workflow runId, but this controller still unconditionally replaces it with '' on line 311. useButtonClickStatus consequently sets loading to false and never tracks or reports the workflow run, unlike the newly corrected non-share button route. Return the V2 result unchanged and only synthesize an empty ID for the legacy branch.
Useful? React with 👍 / 👎.
| const workflowResult = yield* await handler.buttonClickWorkflowService.trigger( | ||
| context, | ||
| buttonClicked | ||
| ); |
There was a problem hiding this comment.
When the undo-store append, plugin afterCommit, or workflow adapter fails after updateOne commits, this yield* chain returns an error even though the click count was already incremented, and failures before this line prevent the workflow from being launched at all. Retrying consumes another click (or immediately hits maxCount) and can permanently skip the intended automation run. Persist a durable/idempotent workflow-dispatch record with the click mutation, and do not let ancillary undo bookkeeping gate dispatch after commit.
Useful? React with 👍 / 👎.
| const commandResult = RestoreRecordsCommand.create({ | ||
| tableId, | ||
| records: records.map((record) => this.toV2RestoreRecord(record)), | ||
| }); |
There was a problem hiding this comment.
When an archived record containing attachments is restored on a V2-enabled base, archive deletion deliberately retains its attachments_table rows, but this command omits cleanupAttachmentRefs. PostgresTableRecordRepository.insertMany therefore leaves the retained rows in place and writes fresh references for the restored cells, double-counting those files toward attachment usage after every archive/restore cycle. Pass the archive-specific cleanup flag through this shared restore service.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b930d4ab3
ℹ️ 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.stopped = false; | ||
| this.loopPromise = this.runForever().catch((error) => { |
There was a problem hiding this comment.
When the backend has multiple replicas and a migration spends more than the five-minute stale timeout in a phase that does not update lastModifiedTime—for example, assertSourceInventoryUnchangedForJob performs the potentially long buildInventory scan before its next job update—this starts a worker loop in every pod, so an idle peer repeatedly calls recoverStaleActiveMigrationJobs, marks the still-running job failed, and may delete its target artifacts while the owning pod continues copying. Previously the service was not lifecycle-started in every application instance, so recovery could not race the sole busy worker this way; recovery needs an owner-aware lease/heartbeat or must run only on a singleton worker.
Useful? React with 👍 / 👎.
|
|
||
| // remove the whole cold prefix of a table — BOTH reason subtrees at once | ||
| // (table permanent deletion) | ||
| async deleteTablePrefix(tableId: string): Promise<void> { |
There was a problem hiding this comment.
When a table with flushed trash or archive snapshots is permanently deleted, this cleanup method is never called: the V1 permanent-delete path only invokes cleanupColdHistoryPrefixes, and the V2 path only invokes cleanupRecordHistoryAfterPermanentDelete; a repo-wide search finds no caller of this record-removal deleteTablePrefix. Consequently, record-removal/v1/{tableId} continues retaining the deleted table's record snapshots indefinitely while monthly compaction keeps discovering the orphan prefix, so wire this service into both permanent-delete paths.
Useful? React with 👍 / 👎.
| // full prefix wipe needs no tombstones, and running after the PG deletes | ||
| // leaves a retryable state if the wipe fails. The archived/ subtree is | ||
| // untouched. | ||
| await this.recordRemovalColdStorageService.deleteReasonPrefix( |
There was a problem hiding this comment.
When a table-trash reset overlaps the daily flusher or monthly compactor, the maintenance job can read the old rows or parts before this call and upload its replacement part after deleteReasonPrefix finishes. Because the reset has already deleted the PG rows and deliberately creates no tombstone or reset-generation marker, that late upload is never removed and retains the snapshots the user just permanently purged. Coordinate prefix resets with the cold writers or record a generation marker that invalidates their in-flight output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a5fcb2a15
ℹ️ 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".
| queryFn: () => getInstanceUsage().then((res) => res.data), | ||
| staleTime: 0, | ||
| }) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
On self-hosted EE, if getInstanceUsage rejects because of a timeout, authorization error, or unavailable usage endpoint, this catch converts the failure to undefined; seatLimit is consequently unset and the callback returns true, allowing every invite, link, or billable role-change caller to proceed. This makes the documented hard seat-limit gate fail open precisely when its authoritative count is unavailable, so the action should remain blocked or surface a retryable error.
Useful? React with 👍 / 👎.
| await this.recordRemovalTombstoneService.markRestored( | ||
| await this.trashTombstoneClientForTable(tableId), | ||
| tableId, | ||
| [...matchedRecordTrashRows, ...coldTrashRows].map(({ recordId }) => recordId) | ||
| ); |
There was a problem hiding this comment.
When restoring a record whose snapshot came from cold storage, a process crash or transient database error after the preceding trash-row transaction commits but before this call succeeds leaves the live record restored and its trash item deleted without the only marker that suppresses the immutable cold copy. The user can no longer retry that trash item, and the supposedly restored snapshot remains retained in cold parts; write the tombstone atomically with deletion of the hot trash metadata. The non-stream restore path repeats the same ordering.
Useful? React with 👍 / 👎.
| const column = sql.ref(`a.${fieldColumns.get(group.fieldId.toString())!}`); | ||
| aggregateQuery = aggregateQuery | ||
| .select(column.as(groupAliases[index]!)) | ||
| .groupBy(column) | ||
| .orderBy(column, group.order); |
There was a problem hiding this comment.
When a V2 aggregation or shared group-points request groups a date-like field, this groups by the raw timestamp rather than the field's formatted date bucket, so records on the same visible day or month become separate, identically rendered groups with incorrect counts and can prematurely hit maxGroupPoints. User-like fields can likewise split when stored snapshots differ despite identifying the same user. Apply the existing buildGroupFieldValueExpression logic here, as the normal grouped-record query already does.
Useful? React with 👍 / 👎.
| */ | ||
| export const recordHistoryColdConfig = (): IRecordHistoryColdConfig => { | ||
| const disabled = readBoolEnv('BACKEND_RECORD_HISTORY_COLD_DISABLED'); | ||
| const disabled = readBoolEnv('BACKEND_STORAGE_COLD_ARCHIVE_DISABLED'); |
There was a problem hiding this comment.
During a rolling upgrade or rollback there is no environment setting that disables record-history migration on both versions: older pods read BACKEND_RECORD_HISTORY_COLD_DISABLED, while this version now reads only BACKEND_STORAGE_COLD_ARCHIVE_DISABLED. Existing deployments that already have the old safety switch enabled will therefore start flush, compaction, and source-row deletion as soon as a new pod comes up, while switching to the new variable leaves still-running old pods enabled. Treat either variable as disabling the feature for a compatibility window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35d79a39d
ℹ️ 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 nonTableUrl = await getDefaultNodeUrl(ctx, { | ||
| filterNode: (node) => node.resourceType !== BaseNodeResourceType.Table, | ||
| }); | ||
| return redirect(nonTableUrl ?? `/base/${baseId}`); |
There was a problem hiding this comment.
When this path is reached because a prefetched table URL outlived deletion of the base's last table and the base-node list is still cached, nonTableUrl is null and this redirects to /base/${baseId}. Checked BasePage.tsx: that route calls getDefaultNodeUrl without the non-table filter, selects the same stale table node, and redirects straight back here, creating a redirect loop until the node-list cache refreshes. Return the base page or a terminal not-found result when no non-table destination exists instead of re-entering the unfiltered resolver.
Useful? React with 👍 / 👎.
| if (recordIds.length <= MAX_ARCHIVE_RECORDS_PER_REQUEST) { | ||
| await archiveRecordsApi(tableId, { recordIds }); | ||
| } else { | ||
| await archiveRecordsStream( |
There was a problem hiding this comment.
When archiving more than 1,000 records and one or more batches emit an error event before the final done event, archiveRecordsStream resolves with both done and a non-empty errors array, but this call discards that result and line 1628 always displays the success toast. The caller then also clears the grid selection because this promise resolved, so users are told the whole selection was archived and lose the selection needed to retry failed records. Inspect the returned errors and report a partial result instead, as the delete, clear, duplicate, and paste stream paths already do.
Useful? React with 👍 / 👎.
| } | ||
| toast.success(t('table:table.actionTips.archiveSuccessful'), { id: toastId }); | ||
| } catch (error) { | ||
| if ((error as HttpError).status === 402) { |
There was a problem hiding this comment.
When a selection over 1,000 records is rejected before the SSE stream starts with HTTP 402—for example after a stale usage check or a billing gate changes—streamSSE throws a plain Error containing the status text rather than an HttpError with a status property. Consequently this condition is never true for the streamed branch, so the intended upgrade modal is replaced by a generic archive error toast; the smaller Axios-backed request does not have this problem. Preserve the response status in the SSE error or classify that rejection before checking it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920771630c
ℹ️ 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 ( | ||
| scale <= 1 || | ||
| !containerDimensions.width || | ||
| !containerDimensions.height || | ||
| !imageDimensions.width || | ||
| !imageDimensions.height | ||
| ) { | ||
| return { x: 0, y: 0 }; |
There was a problem hiding this comment.
On mobile, rotating a portrait image can lower minScale below 1; at an intermediate zoom such as 0.8, the rotated width can already exceed the container, but this guard resets the translation to zero (and the drag handlers similarly require scale > 1). The clipped sides therefore cannot be reached until the user zooms past 100%. Determine pannability from the rotated scaled dimensions rather than the nominal scale.
Useful? React with 👍 / 👎.
| const undoRedoResult = await this.viewUndoRedoService.appendDelete( | ||
| context, | ||
| transactionResult.value.table, | ||
| transactionResult.value.deletedSnapshot | ||
| ); |
There was a problem hiding this comment.
When the undo store is unavailable after the metadata transaction commits, appendDelete returns an error and the handler reports the request as failed even though the view and its dependency updates are already persisted. Retrying then returns not-found, and no undo entry exists to recover the deleted view. Persist the undo intent atomically/durably with the deletion, or treat an undo-store failure after commit as non-fatal to the mutation response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2bf60d9b5
ℹ️ 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 missing = findMissingSecrets(ALL_SECRET_SPECS, env); | ||
| if (missing.length > 0) { | ||
| throw new Error(`\n${buildMissingSecretsMessage(missing)}`); |
There was a problem hiding this comment.
When deploying or upgrading with the checked-in standalone Compose configuration, this now aborts startup because dockers/examples/standalone/.env supplies none of SECRET_KEY or the required encryption variables; the preview workflow similarly supplies only the JWT and session secrets. Since the policy runs during module composition, those documented deployment paths cannot boot until every newly required variable is added to their manifests or rollout compatibility is retained.
Useful? React with 👍 / 👎.
| pinInstruction: | ||
| "previous effective value: your BACKEND_ACCESS_TOKEN_ENCRYPTION_KEY if it was set, otherwise compute `node -e \"console.log(require('crypto').createHash('sha256').update(process.env.SECRET_KEY).digest('hex').slice(0,16))\"`. New deployments: `openssl rand -hex 8`.", |
There was a problem hiding this comment.
For an existing deployment that stored BYODB URLs without any dedicated encryption variables, access-token encryption variables, or SECRET_KEY, the previous key and IV were hashes of two distinct hardcoded seeds (teable-data-db-url-secret and teable-data-db-url-secret-iv). This instruction instead derives both from the newly configured SECRET_KEY, producing different material and making the existing URLs undecryptable after the operator follows the startup remediation; provide the two actual legacy values for this case.
Useful? React with 👍 / 👎.
| .join('\n'); | ||
| const newBlock = missing | ||
| .map( | ||
| (s) => ` ${s.envKey}=$(openssl rand ${isAes16Slot(s.envKey) ? '-hex 8' : '-base64 32'})` |
There was a problem hiding this comment.
When a new deployment follows this copy-pastable remediation, every AES encryption key is generated with openssl rand -hex 8: that produces 8 random bytes encoded as 16 ASCII hex characters, so the key passed to AES has only 64 bits of entropy despite using aes-128-cbc. An attacker with encrypted personal-access-token or BYODB data can therefore perform a substantially cheaper offline key search; accept and decode a full 16-byte representation (and validate it at startup) rather than satisfying the cipher's 16-character input length with only eight random bytes.
Useful? React with 👍 / 👎.
| const newBlock = missing | ||
| .map( | ||
| (s) => ` ${s.envKey}=$(openssl rand ${isAes16Slot(s.envKey) ? '-hex 8' : '-base64 32'})` | ||
| ) |
There was a problem hiding this comment.
When an operator copies this advertised block into the .env file used by the documented deployments, dotenv stores $(openssl rand ...) literally because it does not perform shell command substitution, yielding invalid AES lengths and predictable literal JWT/session secrets. Running the lines in a shell does execute OpenSSL, but the assignments are not exported and therefore still do not reach the backend process; emit concrete generated values through a helper or provide commands that write/export them in the deployment's actual environment format.
Useful? React with 👍 / 👎.
…ut avatars T6492 Synced from teableio/teable-ee@20ebfda Co-authored-by: Aries X <caoxing9@gmail.com> 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: 70545e3f6f
ℹ️ 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".
| repaired++; | ||
| continue; | ||
| } | ||
| const avatar = await this.generateDefaultAvatar(id); |
There was a problem hiding this comment.
When a user uploads an avatar after this findUnique check—or is already inside updateAvatar, which uploads before updating users.avatar—generateDefaultAvatar overwrites the same per-user storage path and attachment token. The subsequent conditional updateMany may correctly update zero rows, but the custom image bytes and attachment metadata have already been replaced, leaving the user's avatar URL pointing at the generated image. Reserve the null-avatar row atomically before touching shared storage, or generate to a temporary object and claim it conditionally.
Useful? React with 👍 / 👎.
| if (!Number.isFinite(expiresMs)) { | ||
| return Math.floor(Date.now() / 1000); | ||
| } | ||
| return Math.floor(expiresMs / 1000) - this.ttl; |
There was a problem hiding this comment.
When an SID was lost from the unlocked per-user map—the race this new repair path handles—and clearByUserId is then called, this derives its renewal time by subtracting the store TTL from the cookie expiry. The middleware sets the cookie lifetime to one year while the default store TTL is seven days, so an old session appears to have been renewed roughly 358 days in the future, fails the <= clearedAtSec check, and is inserted back into the map. Password changes, resets, or other all-session revocations can therefore leave such a session authenticated; persist an actual issue/renewal timestamp or derive it using the cookie lifetime.
Useful? React with 👍 / 👎.
| if (mode === 'undo') { | ||
| await this.markV2RestoredTombstones(tableId, executeResult.value.entry.undoCommand); | ||
| } |
There was a problem hiding this comment.
When a stale V2 undo contains RestoreRecords snapshots that the replay guard declines to restore, this still tombstones every record ID from the stored command. For example, if another window restores a deleted record and then archives it, replaying the old delete undo is a no-op because only an archived trash row exists, but the new tombstone is newer than that archive removal and suppresses its cold copy; after the hot row is flushed, the archive disappears and can eventually be dropped by compaction. Propagate the actual restored IDs from replay and mark only those.
Useful? React with 👍 / 👎.
2 participants
🔄 Automated sync from EE repository.
51 commit(s) synced since last sync.
Authors
Included commits
Latest source commit: teableio/teable-ee@20ebfda
This PR was automatically created by the sync workflow.