-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore:history rewrite to use conventional commits and misc fixups #23
Conversation
Visit the preview URL for this PR (updated for commit 5f0c7d8): https://aw-leaderboard--pr23-dev-history-cleanup-tukktwxw.web.app (expires Fri, 14 Feb 2025 08:25:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c41748b80c8db8604c9c431c363ccab703d858c1 |
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.
❌ Changes requested. Reviewed everything up to c9bdba9 in 1 minute and 41 seconds
More details
- Looked at
2268
lines of code in54
files - Skipped
4
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. .eslintrc.cjs:6
- Draft comment:
Theextends
key should be a string. Use quotes aroundextends
to avoid syntax errors.
'extends': [
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
- Object keys in JavaScript don't require quotes unless they contain special characters or spaces. 2. 'extends' is a valid identifier. 3. The change actually removes quotes, making the code more idiomatic. 4. The code will work perfectly fine either way - this is purely stylistic. 5. ESLint itself would enforce any required quoting rules if they existed.
The comment could be correct if there's an ESLint rule requiring quoted keys that I'm not aware of.
Even if such a rule existed, it would be automatically caught by ESLint itself during builds, so we shouldn't comment on it per the rules.
Delete this comment as it suggests an unnecessary change, and any actual quoting requirements would be caught by ESLint itself.
2. functions/src/index.ts:76
- Draft comment:
Ensure thatdoc.data()?.apiKey
is accessed only when the document exists. Otherwise, it may result in an error.
return {apiKey: apiKey}
- Reason this comment was not posted:
Comment did not seem useful.
3. functions/src/index.ts:170
- Draft comment:
Specify the content type when saving the file to ensure it is correctly interpreted when retrieved. - Reason this comment was not posted:
Comment did not seem useful.
4. functions/src/index.ts:91
- Draft comment:
ThetotalsMap
is initialized but never used. Consider removing it to clean up the code. - Reason this comment was not posted:
Confidence changes required:50%
Inindex.ts
, theupdateLeaderboardData
function initializestotalsMap
but never uses it, which is unnecessary and could be removed.
Workflow ID: wflow_MVU2Zp3IvSq7yFx6
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.
src/firebase/data.ts
Outdated
collection(db, "leaderboard"), | ||
orderBy('rank'), | ||
limit(50), | ||
startAfter(offset) |
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.
startAfter
should be used with a document snapshot or field value, not an offset. Consider using offset
with limit
in a different way to paginate results.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid reusing the events
variable name to prevent confusion and potential logical errors. Consider renaming one of the variables.
functions/src/index.ts
Outdated
} | ||
await Promise.all(promises) | ||
// TODO: error handling | ||
await batch.commit() |
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.
Add error handling for the batch commit to prevent unhandled promise rejections.
c9bdba9
to
1266ed2
Compare
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.
❌ Changes requested. Incremental review on 1266ed2 in 52 seconds
More details
- Looked at
311
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. src/components/StackedBarChart.vue:29
- Draft comment:
Duplicate import ofautocolors
. Remove the redundant import statement. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_5pC6Nl8KqEW3rdKm
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.
src/firebase/data.ts
Outdated
@@ -36,18 +36,21 @@ const functions = getFunctions() | |||
export function dataToSummary(data: ScreenTimeData): ScreenTimeSummary { | |||
const total = data.events.reduce((acc, event) => acc + event.duration, 0) | |||
const categoryTotals: { [key: string]: number } = {} | |||
const categoryTotals: { [key: string]: number } = {} |
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.
Duplicate declaration of categoryTotals
. Remove the redundant line.
src/firebase/data.ts
Outdated
if (!categoryTotals[category]) { | ||
categoryTotals[category] = 0 | ||
} | ||
categoryTotals[category] += event.duration | ||
categoryTotals[category] += event.duration |
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.
Duplicate operation: categoryTotals[category] += event.duration
. Remove the redundant line.
src/firebase/data.ts
Outdated
}) | ||
return { | ||
userId: data.userId, | ||
total, | ||
date: data.date, | ||
categoryTotals | ||
categoryTotals |
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.
Duplicate property categoryTotals
in the returned object. Remove the redundant line.
src/components/StackedBarChart.vue
Outdated
import autocolors from 'chartjs-plugin-autocolors' | ||
import { Bar } from 'vue-chartjs' | ||
import type { ChartDataset } from '@/types' | ||
import type { ScreenTimeSummary } from '@/types' |
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.
Duplicate import of ScreenTimeSummary
. Remove the redundant import statement.
1266ed2
to
5f0c7d8
Compare
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.
❌ Changes requested. Incremental review on 5f0c7d8 in 1 minute and 12 seconds
More details
- Looked at
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. src/stores/leaderboard.ts:16
- Draft comment:
Consider adding error handling to the promise chain. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_mxzMnxzV1qIDa7Zi
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.
src/stores/leaderboard.ts
Outdated
persist: true, | ||
actions: { | ||
fetchLeaderboardData(offset:number = 0) { | ||
getLeaderboard(offset).then((data) => { |
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.
Consider adding error handling (e.g., a .catch) on the getLeaderboard promise to handle API call failures.
5f0c7d8
to
666de93
Compare
Important
This pull request adds Firebase functions, new Vue components, and state management for a leaderboard feature in the ActivityWatch project, along with updated configurations and documentation.
onUserCreated
andonUserDeleted
functions infunctions/src/index.ts
to handle user creation and deletion events.getApiKey
androtateApiKey
callable functions for managing user API keys.updateLeaderboardData
scheduled function to update leaderboard data daily.uploadData
andonUploadData
functions to handle data uploads and processing.Apikey.vue
,Dashboard.vue
,Leaderboard.vue
,LoginForm.vue
,PieChart.vue
,SnackBar.vue
, andStackedBarChart.vue
components.App.vue
,Header.vue
, andFooter.vue
to integrate new components and features.apikey.ts
,auth.ts
,leaderboard.ts
, andscreentime.ts
stores using Pinia for state management.router/index.ts
to include new routes forDashboard
,Leaderboard
, andApikey
components..eslintrc.cjs
,.prettierrc.json
, andfunctions/.eslintrc.js
for linting and formatting.firebase.json
to update Firestore and storage rules paths.README.md
with new project setup instructions and details.vue-material-design-icons.d.ts
for type declarations of icon components.This description was created by
for 5f0c7d8. It will automatically update as commits are pushed.