Skip to content

fix: address Cube chart review feedback#7956

Merged
mattinannt merged 3 commits into
epic/v5from
fix/v5-address-cube-review
May 7, 2026
Merged

fix: address Cube chart review feedback#7956
mattinannt merged 3 commits into
epic/v5from
fix/v5-address-cube-review

Conversation

@BhagyaAmarasinghe
Copy link
Copy Markdown
Contributor

Addresses CodeRabbit comments from #7955.

Changes:

  • Enforces tenant filters for both FeedbackRecords and TopicsUnnested Cube members.
  • Adds audit logging for rejected security contexts before claim validation completes.
  • Aligns the NPS detractor calculation and exposes tenantId on TopicsUnnested.
  • Adds Cube-specific imagePullSecrets, container security context, readiness/liveness probes, and a Helm guard for unsafe multi-replica memory queue usage.
  • Keeps the Docker Cube config/schema in sync with the chart-managed Cube config.

Validation:

  • helm lint charts/formbricks --set cube.enabled=true --set secret.enabled=false --set externalSecret.enabled=true --set externalSecret.files.app-secrets.dataFrom.key=stage/formbricks
  • helm template formbricks charts/formbricks --set cube.enabled=true --set secret.enabled=false --set externalSecret.enabled=true --set externalSecret.files.app-secrets.dataFrom.key=stage/formbricks
  • helm template formbricks charts/formbricks --set cube.enabled=true --set cube.replicas=2 --set cube.env.CUBEJS_CACHE_AND_QUEUE_DRIVER=cubestore --set secret.enabled=false --set externalSecret.enabled=true --set externalSecret.files.app-secrets.dataFrom.key=stage/formbricks
  • helm template formbricks charts/formbricks --set cube.enabled=true --set cube.replicas=2 --set secret.enabled=false --set externalSecret.enabled=true --set externalSecret.files.app-secrets.dataFrom.key=stage/formbricks failed as expected with the memory-driver guard
  • node --check charts/formbricks/cube/cube.js && node --check docker/cube/cube.js && node --check charts/formbricks/cube/schema/FeedbackRecords.js && node --check docker/cube/schema/FeedbackRecords.js
  • git diff --check

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack

Walkthrough

This PR synchronizes Cube.js configuration and query security across Helm chart and Docker environments. Changes expand tenant-level query filtering to cover multiple Cube dimensions (FeedbackRecords.tenantId and TopicsUnnested.tenantId), refine NPS score calculation to enforce value bounds, and enhance audit logging when security context validation fails. The Helm deployment template now validates that in-memory cache driver is only used with single replica, and pod configuration gains conditional support for security contexts and health probes. These updates consolidate tenant enforcement across the data schema and query layer.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: address Cube chart review feedback' directly summarizes the main change—addressing review feedback on Cube configuration—and follows Conventional Commits.
Description check ✅ Passed The description provides clear context (references #7955), specific changes, and comprehensive validation steps. All required information is covered.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@charts/formbricks/templates/cube-deployment.yaml`:
- Around line 1-3: The check uses $cubeCacheDriver to compare against "memory"
but doesn't normalize casing/whitespace; update the assignment of
$cubeCacheDriver (the variable derived from get (.Values.cube.env)
"CUBEJS_CACHE_AND_QUEUE_DRIVER") to trim and lowercase the string before
comparison (e.g., pipe through trim and lower after toString) so the conditional
that enforces the memory/replica guard (the if using .Values.cube.enabled,
.Values.cube.replicas and eq $cubeCacheDriver "memory") correctly detects values
with accidental spaces or different casing, and leave the fail message
unchanged.

In `@docker/cube/cube.js`:
- Around line 145-157: The audit schema is inconsistent:
logCubeQuerySecurityContextFailure emits both errorName and errorMessage but
logCubeQueryAuditEvent only spreads errorName on error; update
logCubeQueryAuditEvent (the function that builds the audit event for cube.query)
to also include errorMessage when an Error (or other) is present so both fields
are emitted consistently — check where error is spread (errorName currently) and
add errorMessage similar to logCubeQuerySecurityContextFailure, ensuring you
stringify non-Error values the same way.
- Around line 181-185: The current unconditional injection of TENANT_MEMBERS
filters causes TopicsUnnested filters to be applied even on pure FeedbackRecords
queries; change the filter injection so each TENANT_MEMBERS entry is only added
when the target cube is actually present in the incoming query. Constrain the
mapping that produces the { member, operator, values } objects (the
TENANT_MEMBERS usage) to check the query's cubes (e.g., the list of requested
cubes—identify via the request/query context available where TENANT_MEMBERS is
used) and only return the TopicsUnnested filter when TopicsUnnested is included,
similarly only return FeedbackRecords filters when FeedbackRecords is included;
keep using context.tenantId for values. Ensure you reference TENANT_MEMBERS,
context.tenantId, TopicsUnnested and FeedbackRecords so the condition is
implemented at the source of the injected filters.

In `@docker/cube/schema/FeedbackRecords.js`:
- Around line 160-163: TopicsUnnested.tenantId is missing a description which
breaks docs consistency; add a descriptive description property to the tenantId
dimension in the TopicsUnnested cube (the tenantId definition) so it matches
other dimensions—e.g., set a clear one-line description like "Tenant identifier"
or similar that explains the field's meaning and usage in the
TopicsUnnested.tenantId dimension.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2f6ce3ce-5088-4564-9c86-6d66deaaa110

📥 Commits

Reviewing files that changed from the base of the PR and between d64fb54 and f703c7f.

📒 Files selected for processing (7)
  • charts/formbricks/README.md
  • charts/formbricks/cube/cube.js
  • charts/formbricks/cube/schema/FeedbackRecords.js
  • charts/formbricks/templates/cube-deployment.yaml
  • charts/formbricks/values.yaml
  • docker/cube/cube.js
  • docker/cube/schema/FeedbackRecords.js

Comment thread charts/formbricks/templates/cube-deployment.yaml Outdated
Comment thread docker/cube/cube.js
Comment thread docker/cube/cube.js Outdated
Comment thread docker/cube/schema/FeedbackRecords.js
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 7, 2026

@mattinannt mattinannt merged commit 2c22b00 into epic/v5 May 7, 2026
13 checks passed
@mattinannt mattinannt deleted the fix/v5-address-cube-review branch May 7, 2026 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants