Skip to content

Fix fetch more notes#16442

Merged
charlesBochet merged 11 commits intomainfrom
ej/16320
Dec 12, 2025
Merged

Fix fetch more notes#16442
charlesBochet merged 11 commits intomainfrom
ej/16320

Conversation

@etiennejouan
Copy link
Copy Markdown
Contributor

fixes : #16320

@etiennejouan etiennejouan self-assigned this Dec 9, 2025
Copy link
Copy Markdown
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
Copy Markdown
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.

2 issues found across 10 files

Prompt for AI agents (all 2 issues)

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


<file name="packages/twenty-front/src/modules/activities/tasks/components/TaskList.tsx">

<violation number="1" location="packages/twenty-front/src/modules/activities/tasks/components/TaskList.tsx:12">
P1: The `totalCount` prop receives the total count of ALL tasks instead of the count for the specific status group. Each `TaskList` grouped by status will incorrectly display the total count (e.g., showing &#39;TODO 10&#39; when there are only 5 TODO tasks out of 10 total). Consider using `tasksByStatus.length` or a per-status count from the API.</violation>
</file>

<file name="packages/twenty-front/src/modules/activities/notes/components/NotesCard.tsx">

<violation number="1" location="packages/twenty-front/src/modules/activities/notes/components/NotesCard.tsx:58">
P2: Changing the loading condition from `loading &amp;&amp; isNotesEmpty` to just `loading` may cause existing notes to disappear and be replaced with a skeleton loader during refetch scenarios. Other similar components (`FilesCard`, `EmailsCard`, `CalendarEventsCard`) specifically check for initial load only to avoid this UX issue. Consider keeping the original condition or using a separate `firstQueryLoading` state.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

title: string;
tasks: Task[];
button?: ReactElement | false;
totalCount: number;
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Dec 9, 2025

Choose a reason for hiding this comment

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

P1: The totalCount prop receives the total count of ALL tasks instead of the count for the specific status group. Each TaskList grouped by status will incorrectly display the total count (e.g., showing 'TODO 10' when there are only 5 TODO tasks out of 10 total). Consider using tasksByStatus.length or a per-status count from the API.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/activities/tasks/components/TaskList.tsx, line 12:

<comment>The `totalCount` prop receives the total count of ALL tasks instead of the count for the specific status group. Each `TaskList` grouped by status will incorrectly display the total count (e.g., showing &#39;TODO 10&#39; when there are only 5 TODO tasks out of 10 total). Consider using `tasksByStatus.length` or a per-status count from the API.</comment>

<file context>
@@ -9,6 +9,7 @@ type TaskListProps = {
   title: string;
   tasks: Task[];
   button?: ReactElement | false;
+  totalCount: number;
 };
 
</file context>

✅ Addressed in ba7d4a3

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 9, 2025

🚀 Preview Environment Ready!

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

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

@lucasbordeau lucasbordeau self-requested a review December 10, 2025 10:36
@lucasbordeau
Copy link
Copy Markdown
Contributor

Total count is displayed on each task group, we should display each group count :

image

@etiennejouan
Copy link
Copy Markdown
Contributor Author

Total count is displayed on each task group, we should display each group count :

image

@lucasbordeau thank you for you review with pertinent human-emitted (so old fashion, so hilarious!!) comments 🙏

I reverted changes on tasks. We can't add pagination on task as we need to fetch task with a given 'status' + taskTarget related to a given personId. Waiting for nested filters

targetableObjects,
activityTargetsOrderByVariables: FIND_MANY_TIMELINE_ACTIVITIES_ORDER_BY,
limit: 30,
limit: 200,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this hardcoded value validated with Charles ? Seems like someone could create more than 200 tasks, we should maybe create a follow-up issue ?

Copy link
Copy Markdown
Contributor Author

@etiennejouan etiennejouan Dec 11, 2025

Choose a reason for hiding this comment

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

@lucasbordeau I add the max. I'll check with Charles. You're right I open a follow up issue

Before this PR, tasks and notes were limited at 60 records.
After this PR, tasks is limited at 200 (to be validated) and notes are paginated.

@etiennejouan etiennejouan changed the title Fix fetch more activities Fix fetch more notes Dec 11, 2025
@etiennejouan etiennejouan enabled auto-merge (squash) December 12, 2025 13:25
@charlesBochet charlesBochet merged commit 6acdde7 into main Dec 12, 2025
61 checks passed
@charlesBochet charlesBochet deleted the ej/16320 branch December 12, 2025 13:43
@twenty-eng-sync
Copy link
Copy Markdown

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

NotYen pushed a commit to NotYen/twenty-ym that referenced this pull request Dec 23, 2025
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.

Can't see more than 60 notes

3 participants