Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CS-7889 task page lhs filter #2068

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

richardhjtan
Copy link
Contributor

@richardhjtan richardhjtan commented Jan 21, 2025

Refer to https://linear.app/cardstack/issue/CS-7889/task-page-lhs-filter-narrow-down-the-task-by-condition

What is changing

  • new set of CRM task filter
  • query changes when CRM active filter updating

Notes

  • lint errors to be addressed - how to pass additional props to isolated task planner
  • find better way to tackle reactive data, while not re-render/infinite looping on filter data from parent

Screenshot

Screen.Recording.2025-01-23.at.10.43.56.PM.mov

@richardhjtan richardhjtan requested a review from a team January 21, 2025 18:06
Copy link

github-actions bot commented Jan 21, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   23m 31s ⏱️ + 1m 48s
742 tests +9  740 ✔️ +9  2 💤 ±0  0 ±0 
747 runs  +9  745 ✔️ +9  2 💤 ±0  0 ±0 

Results for commit 3ce4c9c. ± Comparison against base commit 673efbc.

This pull request removes 733 and adds 742 tests. Note that renamed tests count towards both.
Chrome 131.0 ‑ Acceptance | AI Assistant tests: attaches a card in a conversation multiple times
Chrome 131.0 ‑ Acceptance | AI Assistant tests: displays active LLM in chat input
Chrome 131.0 ‑ Acceptance | Commands tests: OpenAiAssistantRoomCommand opens the AI assistant room
Chrome 131.0 ‑ Acceptance | Commands tests: a command executed via the AI Assistant shows the result as an embedded card
Chrome 131.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand with autoExecute flag is automatically executed by the bot, panel closed
Chrome 131.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand with autoExecute flag is automatically executed by the bot, panel open
Chrome 131.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand without autoExecute flag is not automatically executed by the bot
Chrome 131.0 ‑ Acceptance | Commands tests: a scripted command can create a card, update it and show it
Chrome 131.0 ‑ Acceptance | Commands tests: command returns serialized result in room message
Chrome 131.0 ‑ Acceptance | Freestyle: smoke check
…
Chrome 132.0 ‑ Acceptance | AI Assistant tests: attaches a card in a conversation multiple times
Chrome 132.0 ‑ Acceptance | AI Assistant tests: displays active LLM in chat input
Chrome 132.0 ‑ Acceptance | Commands tests: OpenAiAssistantRoomCommand opens the AI assistant room
Chrome 132.0 ‑ Acceptance | Commands tests: a command executed via the AI Assistant shows the result as an embedded card
Chrome 132.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand with autoExecute flag is automatically executed by the bot, panel closed
Chrome 132.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand with autoExecute flag is automatically executed by the bot, panel open
Chrome 132.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand without autoExecute flag is not automatically executed by the bot
Chrome 132.0 ‑ Acceptance | Commands tests: a scripted command can create a card, update it and show it
Chrome 132.0 ‑ Acceptance | Commands tests: command returns serialized result in room message
Chrome 132.0 ‑ Acceptance | Freestyle: smoke check
…
This pull request removes 2 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.
Chrome 131.0 ‑ Integration | card-editor: can create a specialized a new card to populate a linksTo field
Chrome 131.0 ‑ Integration | create app module via ai-assistant: it can create a module using a tool call
Chrome 132.0 ‑ Integration | card-editor: can create a specialized a new card to populate a linksTo field
Chrome 132.0 ‑ Integration | create app module via ai-assistant: it can create a module using a tool call

♻️ This comment has been updated with latest results.

Comment on lines +130 to +132
this.cardsQuery = getCards(this.getTaskQuery, this.realmHrefs, {
isLive: true,
});
Copy link
Contributor

@tintinthong tintinthong Jan 21, 2025

Choose a reason for hiding this comment

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

Why isn't getTaskQuery the only one which is changing? I think that is the expectation for the resource pattern. Generally, we shudn't reassign the resource particularly inside of a getter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tintinthong yes it should be only query changing. I have same thought that getCards API should be reactive when query changes, but currently it is not.

One example I did before was in deal summary page, we do a restartable tasks when the filter changes: #2002. This PR currently use this approach as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is surprising to me

@richardhjtan richardhjtan marked this pull request as ready for review January 24, 2025 09:03
@tintinthong
Copy link
Contributor

This should fix everything #2088. Depends on core team review and follow-up

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