-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: insert below and replace in smart-edit highlights text #2107
Conversation
frontend/appflowy_flutter/integration_test/util/mock/mock_openai_repository.dart
Outdated
Show resolved
Hide resolved
frontend/appflowy_flutter/integration_test/util/mock/mock_openai_repository.dart
Show resolved
Hide resolved
frontend/appflowy_flutter/integration_test/util/mock/mock_openai_repository.dart
Outdated
Show resolved
Hide resolved
...flutter/lib/plugins/document/presentation/plugins/openai/widgets/smart_edit_node_widget.dart
Outdated
Show resolved
Hide resolved
...flutter/lib/plugins/document/presentation/plugins/openai/widgets/smart_edit_node_widget.dart
Outdated
Show resolved
Hide resolved
...flutter/lib/plugins/document/presentation/plugins/openai/widgets/smart_edit_node_widget.dart
Outdated
Show resolved
Hide resolved
...flutter/lib/plugins/document/presentation/plugins/openai/widgets/smart_edit_node_widget.dart
Outdated
Show resolved
Hide resolved
This was my first time working with integration tests got to learn a lot from it, did a hard reset to remove garbage commits which were failing integration tests. Mocks and tests written should be helpful in future for increasing test coverage if more work on ai-tools is done. realized refactoring integration tests into multiple files is not a thing by default in flutter, so did a work-around for running multiple tests by creating a common test function and calling other test components in group of this main function, @a-wallen ran into similar issues in #2106, commits there seem to modify some scripts to run integration tests, so I'll be waiting on his pr to be merged, since commits here will be causing merge conflicts. Also do suggest improvements in other files or tests. |
Hello @a-wallen, I have run into new set of problems here, recent commits have changed the dependency of appflowy_editor so now it is being fetched as a hosted package. So changes made in #2100 which are necessary for this feature to work, would be redundant now as appflowy_editor form path is not being used. What should we do about this, should I open a similar issue as #2100 in appflowy_editor repo? Also isn't it redundant to keep appflowy_editor under packages in root if it is being fetched as a hosted package. Similarly commit eeacd87 made in this pr (that is a small issue in editor) would also be redundant. |
Will do. Edit: feature and tests are in place, once PR on appyflowy_editor is merged hopefully integration tests should pass and this issue would be closed. |
@squidrye, any plans for this? |
Hi @a-wallen, all the tests and features are in place for this issue, some github tasks were failing due to other changes in appflowy_editor, I was busy with work recently so couldn't check up on if those were resolved. If all the tests pass here, I'll do a quick local test as well and mark this ready for review. |
I have tested this out locally, please review for merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on getting this completed! It looks solid.
I think you're the first user to load a preset workspace for an integration test. Could you let me know how your experience was and if there's anything we can do to improve the experience?
Using open-ai to replace or insert a summary or spelling-fix from selected text was not highlighted due to which user had a hard time locating it in between large paragraphs. This PR adds this feature to highlight replacement text.
AwesomeScreenshot-3_25_2023.3.14.06PM.mp4
fixes #2059
requires #2100 to avoid inconsistencies