CC-1717 #2816
CC-1717 #2816
Conversation
…d downvote functionality
… unused analytics tracking
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update refactors the voting functionality for community solution cards within a course stage step. The upvote and downvote button components are removed, and their logic is consolidated into the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FeedbackSection
participant SolutionModel
User->>FeedbackSection: Click Upvote or Downvote
alt Upvote
FeedbackSection->>FeedbackSection: handleUpvoteClickTask
alt Already upvoted
FeedbackSection->>SolutionModel: unvote()
else Not yet upvoted
FeedbackSection->>SolutionModel: upvote(metadataForUpvote)
FeedbackSection->>FeedbackSection: flashSuccessMessageTask
end
else Downvote
FeedbackSection->>FeedbackSection: handleDownvoteClickTask
alt Already downvoted
FeedbackSection->>SolutionModel: unvote()
else Not yet downvoted
FeedbackSection->>SolutionModel: downvote(metadataForDownvote)
FeedbackSection->>FeedbackSection: flashSuccessMessageTask
end
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results 1 files ±0 1 suites ±0 7m 33s ⏱️ -46s For more details on these failures, see this check. Results for commit 709cd3f. ± Comparison against base commit c4f2f92. ♻️ This comment has been updated with latest results. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Bundle ReportChanges will increase total bundle size by 253 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
Files in
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/components/course-page/course-stage-step/community-solution-card/feedback-section.hbs (2)
4-12: Decouple flash-message visibility from the task lifecycle to prevent UI flicker
animated-ifkeys offflashSuccessMessageTask.isRunning.
If a user clicks “Helpful/Not helpful” several times in quick succession,keepLatest: truewill cancel the running task and start a new one, momentarily settingisRunningtofalseand causing the “Thanks for your feedback!” line to flicker.A small tracked boolean dedicated to the banner’s visibility avoids this side-effect and makes the intent clearer.
-{{#animated-if this.flashSuccessMessageTask.isRunning …}} +{{#animated-if this.showFlashMessage …}}Inside the task:
this.showFlashMessage = true; await timeout(1500); this.showFlashMessage = false;This separates UI state from the task state while preserving the 1.5 s display duration.
18-22: Add explicittype="button"and ARIA labels for accessibilityThe
TertiaryButtonlikely renders a native<button>element.
Addingtype="button"prevents unintended form submission if the component is ever nested inside a<form>.
Anaria-label(oraria-pressedto convey toggle state) will also improve screen-reader support.-<TertiaryButton +<TertiaryButton + type="button" + aria-label={{if this.currentUserHasUpvoted "Remove helpful vote" "Mark as helpful"}}Apply the same pattern to the Not helpful button.
app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts (1)
42-49: DRY: Extract a shared helper to reduce duplication
handleUpvoteClickTaskandhandleDownvoteClickTaskare nearly identical. A private method (e.g.,_toggleVote) receiving the “direction” and metadata would reduce maintenance cost and make future changes (e.g., adding analytics) one-liner updates.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-45: app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts#L45
Added line #L45 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/course-page/course-stage-step/community-solution-card/content.hbs(1 hunks)app/components/course-page/course-stage-step/community-solution-card/content.ts(1 hunks)app/components/course-page/course-stage-step/community-solution-card/downvote-button.hbs(0 hunks)app/components/course-page/course-stage-step/community-solution-card/downvote-button.ts(0 hunks)app/components/course-page/course-stage-step/community-solution-card/feedback-section.hbs(2 hunks)app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts(1 hunks)app/components/course-page/course-stage-step/community-solution-card/header.hbs(0 hunks)app/components/course-page/course-stage-step/community-solution-card/index.hbs(1 hunks)app/components/course-page/course-stage-step/community-solution-card/upvote-button.hbs(0 hunks)app/components/course-page/course-stage-step/community-solution-card/upvote-button.ts(0 hunks)tests/acceptance/course-page/code-examples/vote-test.js(2 hunks)
💤 Files with no reviewable changes (5)
- app/components/course-page/course-stage-step/community-solution-card/header.hbs
- app/components/course-page/course-stage-step/community-solution-card/upvote-button.hbs
- app/components/course-page/course-stage-step/community-solution-card/downvote-button.hbs
- app/components/course-page/course-stage-step/community-solution-card/upvote-button.ts
- app/components/course-page/course-stage-step/community-solution-card/downvote-button.ts
🧰 Additional context used
🪛 ESLint
tests/acceptance/course-page/code-examples/vote-test.js
[error] 52-52: Replace "Your·feedback·helps·us·surface·better·examples." with 'Your·feedback·helps·us·surface·better·examples.'
(prettier/prettier)
[error] 97-97: Replace "Your·feedback·helps·us·identify·examples·that·need·review." with 'Your·feedback·helps·us·identify·examples·that·need·review.'
(prettier/prettier)
🪛 GitHub Actions: Test
tests/acceptance/course-page/code-examples/vote-test.js
[error] 52-52: Prettier formatting error: Replace double quotes with single quotes. (prettier/prettier)
[error] 97-97: Prettier formatting error: Replace double quotes with single quotes. (prettier/prettier)
[error] Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 GitHub Check: codecov/patch
app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts
[warning] 36-36: app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts#L36
Added line #L36 was not covered by tests
[warning] 45-45: app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts#L45
Added line #L45 was not covered by tests
🔇 Additional comments (6)
app/components/course-page/course-stage-step/community-solution-card/index.hbs (1)
18-19: Consistent metadata passing looks good.The addition of metadata arguments to the Content component ensures consistent data flow for voting functionality across the component hierarchy, matching what's already passed to the Header component.
app/components/course-page/course-stage-step/community-solution-card/content.ts (1)
18-19: Interface extension looks good.The addition of metadata arguments to the interface properly types the data being passed through the component hierarchy, supporting the consolidated voting functionality.
app/components/course-page/course-stage-step/community-solution-card/content.hbs (1)
89-94: Proper metadata forwarding to FeedbackSection.The component now correctly passes the voting metadata and solution data to the FeedbackSection component, supporting the refactored voting functionality.
app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts (3)
19-25: Guard against undefined vote arrays
currentUserUpvotes/currentUserDownvotesare assumed to be arrays, but if the server omits them (e.g., during an optimistic render)lengthwill throw. A small defensive tweak keeps the component safe:-return this.args.solution.currentUserDownvotes.length > 0; +return (this.args.solution.currentUserDownvotes ?? []).length > 0;Do the same for
currentUserUpvotes.
27-32: Good use of an isolated task for the success bannerThe refactor cleanly scopes banner timing logic and avoids leaking state—nice!
33-49: Add unit/acceptance tests for the new vote-toggle pathsCodecov flagged lines 36 and 45 as uncovered.
At minimum, test that:
- Clicking Helpful when not previously up-voted triggers
solution.upvote, shows the banner for ~1.5 s and disables the button while the task runs.- Clicking again calls
solution.unvote.- Error paths surface a message (once implemented).
This will protect against regressions in the concurrency logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts#L36
Added line #L36 was not covered by tests
[warning] 45-45: app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts#L45
Added line #L45 was not covered by tests
| handleUpvoteClickTask = task({ keepLatest: true }, async () => { | ||
| if (this.currentUserHasUpvoted) { | ||
| await this.args.solution.unvote({}); | ||
| } else { | ||
| await this.args.solution.upvote(this.args.metadataForUpvote || {}); | ||
| this.flashSuccessMessageTask.perform(); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Surface API failures to the user & keep button state in sync
If solution.upvote/unvote rejects (network error, 4xx, etc.) the task will fail silently; the button re-enables but the vote state won’t change, leaving users confused.
Consider wrapping the body in try/catch, showing a toast or banner, and optionally rolling back local state:
-handleUpvoteClickTask = task({ keepLatest: true }, async () => {
+handleUpvoteClickTask = task({ keepLatest: true }, async () => {
+ try {
if (this.currentUserHasUpvoted) {
await this.args.solution.unvote({});
} else {
await this.args.solution.upvote(this.args.metadataForUpvote || {});
this.flashSuccessMessageTask.perform();
}
+ } catch (e) {
+ // TODO: surface a user-friendly error message
+ console.error('Failed to toggle upvote', e);
+ }
});Replicate the pattern for the downvote task.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleUpvoteClickTask = task({ keepLatest: true }, async () => { | |
| if (this.currentUserHasUpvoted) { | |
| await this.args.solution.unvote({}); | |
| } else { | |
| await this.args.solution.upvote(this.args.metadataForUpvote || {}); | |
| this.flashSuccessMessageTask.perform(); | |
| } | |
| }); | |
| handleUpvoteClickTask = task({ keepLatest: true }, async () => { | |
| try { | |
| if (this.currentUserHasUpvoted) { | |
| await this.args.solution.unvote({}); | |
| } else { | |
| await this.args.solution.upvote(this.args.metadataForUpvote || {}); | |
| this.flashSuccessMessageTask.perform(); | |
| } | |
| } catch (e) { | |
| // TODO: surface a user-friendly error message | |
| console.error('Failed to toggle upvote', e); | |
| } | |
| }); |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: app/components/course-page/course-stage-step/community-solution-card/feedback-section.ts#L36
Added line #L36 was not covered by tests
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
Refactor
Style
Tests