-
Notifications
You must be signed in to change notification settings - Fork 100
Add a fix language function #452
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
base: main
Are you sure you want to change the base?
Conversation
Added a function to rename the language name to consolidate the same language EX js and javascript to Javascript. Bug report: hackclub#402
I literally removed 2 lines of nothing bc the linter was mad Hopefully the linter is happy now
I removed 2 tabs from line 135 The linter was still mad, I hope to all powerful that its happy now
|
Note: This does not fix any old heartbeats. I don't know how/where to write a script to do so. |
|
I like the initial approach, but this is going to take a little bit more thinking. We need to set this up in a way where we don't clobber any data. The raw fields being sent to us shouldn't be rewritten without backing them up! |
|
Il change it so it does the renaming on the UI side of it, would that be better? |
|
@JeffreyWangDev if you can get away with that, sure! I imagine it's going to be hard to get everything though because there are some more complex things like the graphs that generate on the frontend based on SQL queries. Try it out and see how possible it is. The other option is something like a "raw_language" and "language" field- the "raw_language" is what comes in originally, the "language" is the one we remap |
Added a function to strings to rename the language to a universal name in the main home page
|
Idk if it works in other places cause I didnt put it in other places. I don't really have access to other places like the admin pages. |
I removed 3 spaces bc linter was mad
Fixed the filters working with the new name thing
|
I got the graphs and the dropdown to reduce languages by renaming it in the heartbeats |
|
will review shortly |
"shortly". Please check again. |
|
kinda forgot this existed, please let me know what to change |
|
@3kh0 bump? |
3kh0
left a comment
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.
Conflicts present, resolve and I will review
|
I fixed it with the commit |
| .map { |r| [ r.language&.categorize_language, r.language_count ] } # fix the bug where langs can have both upper and lower case like JAVA and java found here (https://github.com/hackclub/hackatime/issues/402) | ||
| .reject { |lang, _| lang.nil? || lang.empty? } | ||
| .group_by { |lang, _| lang } | ||
| .transform_values { |pairs| pairs.sum { |_, count| count } } |
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.
Bug: Calling Hash#uniq on language_counts will raise a NoMethodError, crashing the application.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The language_counts calculation attempts to call .uniq on a Hash object after group_by and transform_values have been applied. The Hash#uniq method does not exist in Ruby, leading to a NoMethodError. This error will occur every time a logged-in user with heartbeats visits the public-facing homepage, causing the application to crash.
💡 Suggested Fix
Remove the .uniq call from line 52. The group_by and transform_values operations already produce a deduplicated Hash with unique language keys and aggregated counts, making .uniq redundant and incorrect.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/controllers/static_pages_controller.rb#L51
Potential issue: The `language_counts` calculation attempts to call `.uniq` on a Hash
object after `group_by` and `transform_values` have been applied. The `Hash#uniq` method
does not exist in Ruby, leading to a `NoMethodError`. This error will occur every time a
logged-in user with heartbeats visits the public-facing homepage, causing the
application to crash.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6043025
| k.categorize_language | ||
| elsif %i[operating_system editor].include?(filter) | ||
| k.capitalize | ||
| else | ||
| k |
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.
This change replaces the display_editor_name helper with a simple .capitalize call, which will produce incorrect results. For example, display_editor_name('vscode') correctly returns 'VS Code', but 'vscode'.capitalize returns 'Vscode'. This breaks the visual display of editor names. Use the helper method instead to maintain correct formatting.
Severity: HIGH
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/controllers/static_pages_controller.rb#L277-L281
Potential issue: This change replaces the `display_editor_name` helper with a simple
`.capitalize` call, which will produce incorrect results. For example,
`display_editor_name('vscode')` correctly returns 'VS Code', but `'vscode'.capitalize`
returns 'Vscode'. This breaks the visual display of editor names. Use the helper method
instead to maintain correct formatting.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6043025
| filtered_heartbeats = filtered_heartbeats.where(filter => normalized_arr) | ||
| elsif filter == :language | ||
| # find the real name, not the pretty one cause some edditors are stupid and return stuff like JAVASCRIPT and javascript and i need to add stuff to make them both fit the lookup | ||
| raw_language_values = [] | ||
| current_user.heartbeats.distinct.pluck(filter).compact_blank.each do |raw_lang| | ||
| categorized = raw_lang.categorize_language | ||
| if filter_arr.include?(categorized) | ||
| raw_language_values << raw_lang | ||
| end | ||
| end | ||
| Rails.logger.debug "lang filter: selected=#{filter_arr}, raw_language_values=#{raw_language_values}" # Debug line | ||
|
|
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.
The language filtering logic (lines 291-302) is inefficient because it retrieves all raw language values from the database in-memory and then filters them in Ruby. This approach doesn't scale well and performs unnecessary database queries. Consider building a database query that filters on the categorized language values instead, or using a scope/helper method. The current implementation also lacks error handling if categorize_language behaves unexpectedly.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/controllers/static_pages_controller.rb#L291-L302
Potential issue: The language filtering logic (lines 291-302) is inefficient because it
retrieves all raw language values from the database in-memory and then filters them in
Ruby. This approach doesn't scale well and performs unnecessary database queries.
Consider building a database query that filters on the categorized language values
instead, or using a scope/helper method. The current implementation also lacks error
handling if `categorize_language` behaves unexpectedly.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6043025
| if filter == :language | ||
| key = key.categorize_language unless key == "Unknown" | ||
| elsif %i[editor operating_system].include?(filter) | ||
| key = key.downcase | ||
| end | ||
| agg[key] = (agg[key] || 0) + duration | ||
| end | ||
|
|
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.
In the pie chart data section, when normalizing language keys, the code uses key.categorize_language but then later passes it to display_language_name for labeling. This creates a potential disconnect: categorize_language is designed for standardization (lowercase -> standardized form), but display_language_name expects the standardized form and provides UI-specific formatting. Ensure these two methods work well together and document the expectation. For consistency, consider whether languages should be stored as their raw form or categorized form.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/controllers/static_pages_controller.rb#L351-L358
Potential issue: In the pie chart data section, when normalizing language keys, the code
uses `key.categorize_language` but then later passes it to `display_language_name` for
labeling. This creates a potential disconnect: `categorize_language` is designed for
standardization (lowercase -> standardized form), but `display_language_name` expects
the standardized form and provides UI-specific formatting. Ensure these two methods work
well together and document the expectation. For consistency, consider whether languages
should be stored as their raw form or categorized form.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6043025
| key = key.downcase | ||
| end | ||
| agg[key] = (agg[key] || 0) + 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.
Line 354 uses .downcase for editor and operating_system, but this is inconsistent with the display_editor_name and display_os_name helpers which provide proper capitalization (e.g., 'vscode' -> 'VS Code', 'darwin' -> 'macOS'). To be consistent with the dashboard section above and maintain correct aggregation, you should apply the same normalization logic. Consider using a helper method or applying the same categorization approach used for languages.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/controllers/static_pages_controller.rb#L354-L356
Potential issue: Line 354 uses `.downcase` for editor and operating_system, but this is
inconsistent with the display_editor_name and display_os_name helpers which provide
proper capitalization (e.g., 'vscode' -> 'VS Code', 'darwin' -> 'macOS'). To be
consistent with the dashboard section above and maintain correct aggregation, you should
apply the same normalization logic. Consider using a helper method or applying the same
categorization approach used for languages.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 6043025

Added a function to rename the language name to consolidate the same language, e.g., js and javascript to JavaScript.
Bug report: #402