Add activity filters to the activity view#2357
Open
hahn-kev wants to merge 9 commits into
Open
Conversation
- Updated ProjectActivity method to accept additional parameters for filtering by author and sorting. - Introduced ListActivityAuthors and ListActivityChangeTypes methods to retrieve distinct authors and change types. - Added ActivityQuery record to encapsulate query parameters. - Implemented frontend components for filtering activities by author and change type. - Updated TypeGen configuration to include new types for activity authors and change types. - Added tests for new functionality in HistoryService. This commit improves the activity tracking capabilities and enhances the user experience by allowing more granular filtering of activities.
github-actions
Bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughExtends the project activity feed with author filtering, change-type filtering, FieldWorks exclusion, and sort ordering. New ChangesActivity Filter, Sort, and Author/Change-Type Listing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Contributor
C# FwHeadless Unit Tests48 tests 48 ✅ 16s ⏱️ Results for commit 6216487. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
C# Unit Tests165 tests 165 ✅ 19s ⏱️ Results for commit 6216487. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs (1)
91-104: ⚡ Quick winAdd a synced-order regression test with two synced commits using different offsets.
Current synced coverage checks synced-vs-unsynced placement, but not relative ordering between synced commits whose
SyncDateoffsets differ. Add that case to prevent regressions in synced sort semantics.🤖 Prompt for 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. In `@backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs` around lines 91 - 104, The test method ProjectActivity_SyncedSort_PlacesUnsyncedLast currently only verifies relative ordering between a synced and an unsynced commit, but does not test the relative ordering between multiple synced commits with different SyncDate offsets. Add a second synced commit with a different DateTimeOffset value (either earlier or later than the first synced commit's date), then use Array.FindIndex to locate both synced commits in the results and add an assertion to verify they are ordered correctly relative to each other according to the ActivitySort.SyncedNewestFirst sort semantics.
🤖 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 `@frontend/viewer/src/lib/activity/ActivityView.svelte`:
- Around line 130-131: The style attribute in the div element contains an
invalid CSS declaration with orphaned minmax fragments on line 131. The fragment
"minmax(min-content, 1fr) minmax(min-content, 2fr);" appears between
grid-template-rows and grid-template-columns without a property name, causing
the browser to ignore it. Remove this orphaned fragment from the style attribute
to ensure the grid layout declaration is valid and the layout behavior is
consistent.
---
Nitpick comments:
In `@backend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.cs`:
- Around line 91-104: The test method
ProjectActivity_SyncedSort_PlacesUnsyncedLast currently only verifies relative
ordering between a synced and an unsynced commit, but does not test the relative
ordering between multiple synced commits with different SyncDate offsets. Add a
second synced commit with a different DateTimeOffset value (either earlier or
later than the first synced commit's date), then use Array.FindIndex to locate
both synced commits in the results and add an assertion to verify they are
ordered correctly relative to each other according to the
ActivitySort.SyncedNewestFirst sort semantics.
🪄 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: CHILL
Plan: Pro
Run ID: 5bd4e79f-5dfb-45f0-9803-e9062c28582b
📒 Files selected for processing (26)
backend/FwLite/FwLiteShared/Services/HistoryServiceJsInvokable.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/FwLiteWeb/Routes/ActivityRoutes.csbackend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.csbackend/FwLite/LcmCrdt/HistoryService.csfrontend/viewer/src/lib/activity/ActivityFilter.sveltefrontend/viewer/src/lib/activity/ActivityView.sveltefrontend/viewer/src/lib/activity/utils.tsfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/ActivitySort.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IActivityAuthor.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IActivityChangeType.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IActivityQuery.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectActivity.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/index.tsfrontend/viewer/src/lib/history/HistoryView.sveltefrontend/viewer/src/lib/services/history-service.tsfrontend/viewer/src/lib/services/service-provider-dotnet.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.po
Collaborator
|
This is very cool. I find several things about the UI a little awkward: 1) I think it would be great if we combined the author options (dropdown and FieldWorks switch)E.g. something like this:
I assume grouping them is more intuitive.
2) The sort options are cool, but challenging. They have non-trivial meanings.
3) The whole view width?I'm a bit surprised you opted for a design that uses the whole width of the view, rather than just the changes panel e.g. how the filters in the browse view are only above the entry list. |
Collaborator
Author
|
In the meeting we decided
|
- Introduced a "History" button in `ActivityItem` to display related history entries. - Enhanced `HistoryView` with support for selecting a specific commit. - Updated HistoryView.svelte to svelte 5
Collaborator
Author
|
I've implemented most of the changes we talked about (updated screenshots at the top):
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/viewer/src/lib/history/HistoryView.svelte (1)
34-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
load()against stale async completions.Concurrent
load()calls can resolve out of order and overwritehistoryafteridchanges or after the dialog is closed.💡 Suggested change
+ let loadToken = 0; + async function load() { + const token = ++loadToken; loading = true; try { history = []; - history = await historyService.load(id); + const nextHistory = await historyService.load(id); + if (token !== loadToken || !open) return; + history = nextHistory; if (!selectedCommitId) selectedCommitId = history[0]?.commitId; } finally { - loading = false; + if (token === loadToken) loading = false; } } function reset() { + loadToken++; selectedCommitId = undefined; history = []; }🤖 Prompt for 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. In `@frontend/viewer/src/lib/history/HistoryView.svelte` around lines 34 - 65, The load() function can have multiple concurrent calls that resolve out of order, causing stale results to overwrite the current history. When id changes or the dialog closes (open becomes false), a previous load() call might still be pending and could restore outdated data after reset() clears it. Implement a mechanism to track and ignore stale async completions in the load() function by either using an AbortController to cancel previous in-flight requests when a new load starts, or by tracking a request identifier that gets updated each time load() is called and checking that the identifier still matches before updating the history state at the end of the async operation.
🤖 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 `@frontend/viewer/src/lib/activity/ActivityItem.svelte`:
- Around line 119-125: Move the HistoryView component rendering outside of the
per-row conditional block that checks showHistoryButton. The HistoryView
component and its wrapping {`#if` openHistoryId} conditional should be placed
after the main content loop at the component level, so that only a single
HistoryView instance is rendered regardless of how many rows exist. Keep the
HistoryView binding and props the same, but ensure it renders once for the
entire component rather than once per row where showHistoryButton is true.
---
Outside diff comments:
In `@frontend/viewer/src/lib/history/HistoryView.svelte`:
- Around line 34-65: The load() function can have multiple concurrent calls that
resolve out of order, causing stale results to overwrite the current history.
When id changes or the dialog closes (open becomes false), a previous load()
call might still be pending and could restore outdated data after reset() clears
it. Implement a mechanism to track and ignore stale async completions in the
load() function by either using an AbortController to cancel previous in-flight
requests when a new load starts, or by tracking a request identifier that gets
updated each time load() is called and checking that the identifier still
matches before updating the history state at the end of the async operation.
🪄 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: CHILL
Plan: Pro
Run ID: e8f010f7-3908-4a1e-9752-b71a904798db
📒 Files selected for processing (22)
backend/FwLite/FwLiteShared/Services/HistoryServiceJsInvokable.csbackend/FwLite/FwLiteWeb/Routes/ActivityRoutes.csbackend/FwLite/LcmCrdt.Tests/HistoryServiceActivityTests.csbackend/FwLite/LcmCrdt/HistoryService.csfrontend/viewer/src/lib/activity/ActivityFilter.sveltefrontend/viewer/src/lib/activity/ActivityItem.sveltefrontend/viewer/src/lib/activity/ActivityView.sveltefrontend/viewer/src/lib/activity/utils.tsfrontend/viewer/src/lib/components/ui/select/select-item.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IActivityQuery.tsfrontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectActivity.tsfrontend/viewer/src/lib/history/HistoryView.sveltefrontend/viewer/src/lib/services/history-service.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.po
✅ Files skipped from review due to trivial changes (7)
- frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectActivity.ts
- frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IActivityQuery.ts
- frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.ts
- frontend/viewer/src/lib/activity/utils.ts
- frontend/viewer/src/locales/ms.po
- frontend/viewer/src/locales/en.po
- frontend/viewer/src/locales/sw.po
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/FwLite/LcmCrdt/HistoryService.cs
- frontend/viewer/src/locales/es.po
- frontend/viewer/src/lib/activity/ActivityView.svelte
- frontend/viewer/src/lib/services/history-service.ts
Comment on lines
+119
to
+125
| {#if showHistoryButton} | ||
| <Button icon="i-mdi-history" onclick={() => openHistoryId = context.snapshot?.id}> | ||
| {$t`History`} | ||
| </Button> | ||
| {#if openHistoryId} | ||
| <HistoryView bind:open={() => !!openHistoryId, (open) => (open ? undefined : openHistoryId = undefined)} id={openHistoryId} selectedCommitId={activity.commitId}/> | ||
| {/if} |
There was a problem hiding this comment.
Render HistoryView once, outside the per-change row.
openHistoryId is shared for the whole component, but the modal is rendered inside each row. When it is set, multiple HistoryView instances can be created at once.
💡 Suggested change
- {`#if` openHistoryId}
- <HistoryView bind:open={() => !!openHistoryId, (open) => (open ? undefined : openHistoryId = undefined)} id={openHistoryId} selectedCommitId={activity.commitId}/>
- {/if}
+ <!-- Keep the button here, but move HistoryView outside the row loop -->{`#if` openHistoryId}
<HistoryView
bind:open={() => !!openHistoryId, (open) => (open ? undefined : openHistoryId = undefined)}
id={openHistoryId}
selectedCommitId={activity.commitId}
/>
{/if}🤖 Prompt for 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.
In `@frontend/viewer/src/lib/activity/ActivityItem.svelte` around lines 119 - 125,
Move the HistoryView component rendering outside of the per-row conditional
block that checks showHistoryButton. The HistoryView component and its wrapping
{`#if` openHistoryId} conditional should be placed after the main content loop at
the component level, so that only a single HistoryView instance is rendered
regardless of how many rows exist. Keep the HistoryView binding and props the
same, but ensure it renders once for the entire component rather than once per
row where showHistoryButton is true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Users of the activity page can now filter by Author, Change Type, and order by sync date.
Author filter:
History view:
opens to the selected commit on the activity page