chore: history cleanup, to use conventional commits

[0xbrayo]

Not mergeable since I rewrote most of the history. Force pushing might be the only option

[github-actions]

Visit the preview URL for this PR (updated for commit 2999b6e):

https://aw-leaderboard--pr26-dev-history-cleanup-etydp5l6.web.app

_{(expires Tue, 04 Mar 2025 12:07:38 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c41748b80c8db8604c9c431c363ccab703d858c1}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to af312a5 in 3 minutes and 12 seconds

More details

• Looked at 1859 lines of code in 43 files
• Skipped 3 files when reviewing.
• Skipped posting 15 drafted comments based on config settings.

1. functions/src/data.ts:54

• Draft comment:
  Parameter type inconsistency: userId is typed as number here but elsewhere is string. Consider unifying the type.
• 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.

2. src/stores/screentime.ts:15

• Draft comment:
  Deriving userId from useAuthStore().user may be unsafe if user is null. Add null check to avoid runtime errors.
• 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 use of ! operator shows the author is aware of the null possibility and has explicitly chosen to assert it won't be null. Without seeing the auth store implementation or understanding the authentication flow, I can't definitively say whether this assertion is incorrect. The watch handler suggests this store handles user state changes appropriately.
  I don't have visibility into the auth store implementation or the broader authentication flow - the comment could be raising a valid concern about edge cases in auth state management.
  The explicit use of the non-null assertion and presence of cleanup logic suggests the author has considered the null case. Without strong evidence that this is actually unsafe, the comment is speculative.
  Delete the comment as it appears speculative without strong evidence that the current approach is problematic.

3. functions/src/index.ts:73

• Draft comment:
  In rotateApiKey, if the document doesn't exist the function sets a new API key but then returns doc.data()?.apiKey, which will be undefined. Consider capturing the generated key in a variable and returning that.
• Reason this comment was not posted:
  Marked as duplicate.

4. functions/src/index.ts:216

• Draft comment:
  Inside onUploadData, the code uses 'events.push(...events)' which pushes the array into itself. Rename one of the variables to correctly merge new events with existing ones.
• Reason this comment was not posted:
  Marked as duplicate.

5. src/components/PieChart.vue:2

• Draft comment:
  The canvas element uses width="width" and height="height" which are literal strings. Use numeric values or bound properties to set actual dimensions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
  The comment is technically correct that "width" and "height" are invalid values. However, since the chart is configured as responsive, the actual dimensions don't matter as Chart.js will resize the canvas automatically based on its container. The current values won't cause any functional issues. Suggesting hardcoded values of 500x500 might actually be worse as it could interfere with the responsive behavior.
  I might be wrong about how Chart.js handles canvas initialization - perhaps it needs valid initial dimensions even with responsive=true.
  Even if initial dimensions are needed, suggesting hardcoded values isn't the right solution - it should be handled via CSS or responsive props if necessary.
  While technically incorrect, the current width/height values don't cause functional issues due to Chart.js's responsive handling. The suggested fix of hardcoded dimensions could be counterproductive.

6. src/stores/auth.ts:28

• Draft comment:
  Calling this.$reset() followed immediately by setting this.user = null is redundant since $reset() should already clear the state.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

7. src/stores/screentime.ts:15

• Draft comment:
  The userId is captured from useAuthStore() as a constant and may not update if the authenticated user changes. Consider making it reactive.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment seems to misunderstand the code. While userId is declared with const, it's already being watched for changes (L33-38). The watch handler clears screenTimeData when userId changes. Additionally, the onSnapshot listener would be recreated when the store function runs again due to user changes. The current implementation appears correct.
  I could be wrong about how Pinia stores handle reactivity with useAuthStore(). Maybe there's an edge case where the watch doesn't catch a user change.
  The watch statement explicitly handles userId changes, and Pinia stores are designed to work with Vue's reactivity system. The current implementation follows Vue/Pinia patterns correctly.
  The comment should be deleted because the code already properly handles user changes through the watch statement and Pinia's reactivity system.

8. src/firebase/data.ts:129

• Draft comment:
  Using startAfter(offset) with a numeric offset is not the recommended way to paginate Firestore queries. Consider using the offset() method or using document snapshots for pagination.
• Reason this comment was not posted:
  Marked as duplicate.

9. src/components/SnackBar.vue:31

• Draft comment:
  Remove or disable debug logging ('console.log("message changed")') unless it's explicitly needed in production.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

10. functions/src/index.ts:216

• Draft comment:
  On line 216-217, the code declares a variable 'events' by extracting it from the document data, and then immediately does 'events.push(...events)'. This appears to be a copy-paste or naming error, causing the variable to reference itself. Please review and correct the variable names to ensure that the intended array is being updated.
• Reason this comment was not posted:
  Marked as duplicate.

11. src/components/Apikey.vue:30

• Draft comment:
  Consider updating the snackbar message to use consistent capitalization. For example, instead of 'ApiKey copied to clipboard', use 'API key copied to clipboard' to match the title in the header.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This is a new file being added. The comment points out a real inconsistency in capitalization between the header and snackbar messages. However, this is a very minor UI text issue that doesn't affect functionality. According to the rules, we should "NOT comment on any pure frontend UI or styling changes" and should "assume that if the author changed the UI, they did it correctly."
  The inconsistency is real and could affect user experience through inconsistent branding. Maybe this isn't purely a UI issue since it's about maintaining consistent terminology throughout the application?
  While consistent terminology is important, this is still fundamentally a UI text change that doesn't affect functionality. The rules clearly state to trust the author on UI decisions.
  The comment should be deleted as it violates the rule about not commenting on UI and styling changes.

12. src/firebase/firestore.rules:1

• Draft comment:
  The file is missing a trailing newline. Adding a newline at the end of the file can help avoid potential issues with some text processors and maintain consistency with common code style guidelines.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is purely informative, suggesting a style change that is not critical to the functionality of the code. It doesn't ask for a specific code change or test, nor does it point out a potential bug or issue that needs confirmation. Therefore, it violates the rule against making purely informative comments.

13. src/firebase/storage.rules:1

• Draft comment:
  The file currently does not end with a newline. While this is a minor cosmetic issue, it's best practice to include a final newline to ensure compatibility with various tools and editors.
• Reason this comment was not posted:
  Comment was not on a valid diff hunk.

14. src/stores/leaderboard.ts:16

• Draft comment:
  Minor typographical spacing issue: Consider adding a space between 'offset:' and 'number' (i.e., 'offset: number') for consistency with common TypeScript style.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the suggestion is technically correct according to TypeScript style guides, this is an extremely minor formatting issue. Most teams would have automatic code formatting tools like Prettier that would handle this. The change doesn't affect functionality at all. The comment feels too trivial to warrant attention in a code review.
  The spacing inconsistency could indicate that automatic formatting isn't set up properly in the project, which might be worth addressing. Some teams do care deeply about consistent formatting.
  Even if formatting tools aren't in place, this is too minor to warrant a manual review comment. If formatting is important, it should be handled systematically through tooling, not individual comments.
  Delete this comment as it's too trivial for a code review comment and would be better handled by automated formatting tools.

15. src/views/AboutView.vue:26

• Draft comment:
  Consider revising the phrase 'unavailable on latest Android,' in the ActivityWatch section. It might be clearer to say something like 'unavailable on the latest Android devices' or 'not available on the latest Android version' to complete the sentence.
• 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.

Workflow ID: wflow_xIIbv5VO6D22CDml

---

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.

[ellipsis-dev]

This function is identical to the existing implementation. Consider importing and reusing the existing function instead of duplicating the code.

• function dataToSummary (data.ts)

[ellipsis-dev]

In rotateApiKey, after setting a new key, returning doc.data()? may return stale data. Consider returning the new generated key directly.

Suggested change

	return {apiKey: doc.data()?.apiKey}
	return {apiKey: genKey.generateApiKey()}

[0xbrayo]

No, this would return an entire different key. will refactor later.

[ellipsis-dev]

Using startAfter(offset) with a number may not work as intended. Firestore queries expect document snapshots or use offset() in the web SDK v9.

Suggested change

	startAfter(offset)
	offset(offset)

[ellipsis-dev]

Canvas attributes width="width" height="height" likely are unintended; remove or replace with numeric values.

Suggested change

	<canvas ref="canvasRef" width="width" height="height"></canvas>
	<canvas ref="canvasRef" width="500" height="400"></canvas>

[ellipsis-dev]

Sorting the computed leaderboard array in place may cause unintended side effects. Consider sorting a copy of the array instead.

Suggested change

	leaderboardDataRef.value.sort((a: { total: number }, b: { total: number }) => b.total - a.total)
	leaderboardDataRef.value = [...leaderboardDataRef.value].sort((a: { total: number }, b: { total: number }) => b.total - a.total)

[ellipsis-dev]

Using JSON.stringify for deep equality check in the onSnapshot callback may be inefficient. Consider using a more optimized deep comparison or checking array lengths before detailed comparison.

[ellipsis-dev]

Minor naming issue: Consider renaming the getter from 'getleaderboardData' to 'getLeaderboardData' to maintain consistent camelCase style.

[0xbrayo]

So many suggestions, will review tomorrow.

[0xbrayo]

It's either a merge commit or a force push to master.

[ErikBjare]

Don't rewrite history on master.

[ErikBjare]

I protected the master branch to disallow force pushes and enforce linear history (no merge commits).