Skip to content

Conversation

@ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Jan 5, 2026

CleanShot.2026-01-05.at.22.31.46.mp4
CleanShot.2026-01-05.at.22.32.44.mp4
CleanShot.2026-01-05.at.22.33.38.mp4

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 11 files

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2026

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:56592

This environment will automatically shut down when the PR is closed or after 5 hours.

@ehconitin ehconitin requested a review from bosiraphael January 5, 2026 18:45
@lucasbordeau
Copy link
Contributor

@Bonapara Should we fix one of the axis to avoid having the tooltip bump like that ?

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

4 issues found across 55 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts">

<violation number="1">
P1: This change appears unrelated to the PR's stated purpose (bar chart tooltips). Changing `IS_IMAP_SMTP_CALDAV_ENABLED` default from `false` to `true` enables IMAP/SMTP/CalDAV integration by default for all deployments, which is a significant behavioral change that doesn't match the PR description. If this is intentional, it should be in a separate PR with appropriate documentation. If accidental, please revert.</violation>
</file>

<file name="packages/twenty-front/src/modules/settings/data-model/fields/forms/currency/hooks/useCurrencySettingsFormInitialValues.ts">

<violation number="1">
P2: Object-level nullish coalescing may lose individual property defaults. If `settings` exists but is partial (e.g., only has `format`), `decimals` won't get `DEFAULT_DECIMAL_VALUE` as it did before. Consider spreading the returned settings with defaults to preserve per-property fallbacks.</violation>
</file>

<file name="packages/twenty-front/src/modules/page-layout/widgets/graph/graphWidgetBarChart/utils/findSliceAtPosition.ts">

<violation number="1" location="packages/twenty-front/src/modules/page-layout/widgets/graph/graphWidgetBarChart/utils/findSliceAtPosition.ts:45">
P2: Potential runtime error if `nearestSlice.bars` is empty. The `findAnchorBarInSlice` function uses `reduce` without an initial value, which throws a `TypeError` on empty arrays. Consider adding a guard similar to the existing `slices.length === 0` check.</violation>
</file>

<file name="packages/twenty-server/src/engine/core-modules/message-queue/message-queue-core.module.ts">

<violation number="1">
P2: Inconsistency: `MetricsModule` is added to `registerAsync` imports but missing from `register` method. If `register` is used with BullMQ driver, `config.metricsService` will be `undefined`. Consider adding `imports: [...(dynamicModule.imports ?? []), MetricsModule]` to the `register` method's return value for consistency.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@ehconitin ehconitin requested a review from bosiraphael January 7, 2026 13:36
@Bonapara
Copy link
Member

Bonapara commented Jan 7, 2026

If the “bump” is caused by the tooltip’s height changing when you move the mouse, that seems normal/fine to me.

Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

LGTM

@ehconitin ehconitin added this pull request to the merge queue Jan 8, 2026
Merged via the queue into main with commit d653b0a Jan 8, 2026
67 checks passed
@ehconitin ehconitin deleted the bar-chart-tooltip-new-behavior branch January 8, 2026 09:17
@twenty-eng-sync
Copy link

Hey @ehconitin! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

1 similar comment
@twenty-eng-sync
Copy link

Hey @ehconitin! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Thanks @ehconitin for your contribution!
This marks your 282nd PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants