chore:history rewrite to use conventional commits and misc fixups #24
Conversation
|
Visit the preview URL for this PR (updated for commit 9a90a16): https://aw-leaderboard--pr24-dev-history-cleanup-7jlen7i1.web.app (expires Fri, 14 Feb 2025 08:44:40 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c41748b80c8db8604c9c431c363ccab703d858c1 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9a90a16 in 2 minutes and 45 seconds
More details
- Looked at
2272lines of code in55files - Skipped
4files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. src/firebase/data.ts:36
-
Draft comment:
This function is identical to the existing implementation. Consider importing and reusing the existing function instead of duplicating the code. -
function
dataToSummary(index.ts) -
Reason this comment was not posted:
Marked as duplicate.
2. src/stores/screentime.ts:15
- Draft comment:
Potential runtime risk: Directly using 'useAuthStore().user!.uid' assumes the user is not null. In a scenario where the user is not yet authenticated, this could lead to an error. Consider adding null checks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The ! operator is an explicit developer choice indicating they're certain user won't be null. The store is likely only used in authenticated contexts. The watch handler suggests this is handled intentionally. Without seeing the auth store implementation or usage patterns, we can't be certain this is actually a problem.
I might be missing authentication initialization patterns that could lead to this store being accessed before auth is ready. The ! operator doesn't guarantee runtime safety.
However, this appears to be a deliberate design choice with logout handling in place. Without strong evidence of an actual issue, this comment is too speculative.
Delete the comment as it's speculative without strong evidence of an actual issue, and the code shows intentional handling of the auth state.
3. functions/src/index.ts:73
- Draft comment:
In the rotateApiKey function, the else branch sets a new API key but then returns doc.data()?.apiKey, which is still the old (or undefined) value. This likely does not return the newly generated key. - Reason this comment was not posted:
Marked as duplicate.
4. functions/src/index.ts:216
- Draft comment:
Variable shadowing in the onUploadData function: the 'events' variable from the outer scope is re-declared inside the then-callback, causing an in-place push on itself. This appears unintended and should use a different name for the fetched events (e.g. 'existingEvents'). - Reason this comment was not posted:
Marked as duplicate.
5. functions/firebase/data.ts:54
- Draft comment:
The addScreenTimeData function has its userId parameter typed as a number, whereas elsewhere (and in the ScreenTimeData interface) the userId is a string. This inconsistency could lead to subtle bugs. Consider changing the type to string. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. functions/firebase/data.ts:129
- Draft comment:
The getLeaderboard function uses 'startAfter(offset)' with an offset calculated as a number. Firestore queries typically require a document snapshot or field value matching the orderBy clause. Verify that this pagination approach works as intended or consider using the proper offset method. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/stores/screentime.ts:15
- Draft comment:
The store immediately accesses 'useAuthStore().user!.uid' to obtain the userId. This assumes a logged-in user is always present, which may not hold true (e.g. during logout). Consider adding a check or making userId reactive. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_E7GPVLSfKxCOkjsV
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
functions/src/index.ts
Outdated
| info("Data processed successfully") | ||
| }) | ||
|
|
||
| function dataToSummary(data: ScreenTimeData): ScreenTimeSummary { |
There was a problem hiding this comment.
This function is an exact duplicate of the existing implementation.
- function
dataToSummary(data.ts)
functions/src/index.ts
Outdated
| } else { | ||
| await docref.set({apiKey: genKey.generateApiKey()}) | ||
| info("ApiKey set and sent to client") | ||
| return {apiKey: doc.data()?.apiKey} |
There was a problem hiding this comment.
Potential issue in the else branch of rotateApiKey: after calling set() to create a new API key, the function returns doc.data()?.apiKey which may be undefined. It should fetch the newly set data (or directly return the generated key).
functions/src/index.ts
Outdated
| .get() | ||
| .then((doc) => { | ||
| if (doc.exists) { | ||
| const events = doc.data()?.events as Event[] |
There was a problem hiding this comment.
Variable shadowing issue: The local variable 'events' is re-declared inside the promise chain when updating documents. This may lead to unintended behavior when merging event arrays.
src/firebase/data.ts
Outdated
| collection(db, "leaderboard"), | ||
| orderBy('rank'), | ||
| limit(50), | ||
| startAfter(offset) |
There was a problem hiding this comment.
Firestore pagination concern: Using 'startAfter(offset)' with a numeric offset is not standard, as startAfter expects a document snapshot or field values. Consider using cursor-based pagination.
src/components/Leaderboard.vue
Outdated
| const { fetchLeaderboardData, leaderboardData } = useLeaderboardStore() | ||
| fetchLeaderboardData() | ||
| const leaderboardDataRef: ComputedRef = computed(() => leaderboardData) | ||
| leaderboardDataRef.value.sort((a: { total: number }, b: { total: number }) => b.total - a.total) |
There was a problem hiding this comment.
Sorting the reactive computed value in place may mutate state unexpectedly. It is safer to sort a copy of the array (e.g. using slice()) before assigning it to entries.
0xbrayo
force-pushed
the
dev/history-cleanup
branch
from
9a90a16 to
666de93
Compare
Important
This pull request implements a leaderboard feature for ActivityWatch using Firebase, including backend functions, Firestore rules, and Vue components for user interface and state management.
onUserCreatedandonUserDeletedfunctions inindex.tsto manage user lifecycle events.getApiKeyandrotateApiKeycallable functions for API key management.updateLeaderboardDatascheduled function to update leaderboard daily.uploadDataandonUploadDatafunctions for data storage and processing.firestore.rulesto manage access toscreentime,leaderboard, anduserscollections.firestore.indexes.jsonfor optimized queries.Apikey.vue,Dashboard.vue,Leaderboard.vue, andPieChart.vuefor UI enhancements.Header.vueandFooter.vuefor improved navigation and layout.apikey.ts,auth.ts,leaderboard.ts, andscreentime.tsfor state management..eslintrc.cjs,.prettierrc.json, andpackage.jsonfor linting and formatting.test.ymlfor CI/CD.firebase.jsonfor Firebase hosting and emulators.This description was created by
for 9a90a16. It will automatically update as commits are pushed.