Fix status page logic to distinguish onboarding from active users#288
Fix status page logic to distinguish onboarding from active users#288dreikanter wants to merge 25 commits intomainfrom
Conversation
WalkthroughFeed creation now activates an onboarding user upon successful save; status rendering switched to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{rb,js,jsx,ts,tsx,css,scss,html,erb,slim}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{rb,js,jsx,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
app/views/**/*.{erb,slim,html}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.rb📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
=======================================
Coverage 97.47% 97.48%
=======================================
Files 157 157
Lines 3448 3459 +11
=======================================
+ Hits 3361 3372 +11
Misses 87 87
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
app/controllers/statuses_controller.rb (1)
4-5: Onboarding flags are correct; considerexists?for consistency/perf
@has_active_tokens = @user.access_tokens.active.any?and@has_feeds = @user.total_feeds_count > 0match the view’s state logic and are easy to read. For consistency withactive_access_tokens?inFeedsControllerand slightly clearer intent, you might prefer:@has_active_tokens = @user.access_tokens.active.exists?Functionally it’s the same here, so this is purely optional.
app/views/statuses/show.html.erb (1)
18-50: State-driven rendering matches onboarding/active requirements; small readability tweak optionalThe conditional chain correctly encodes:
- State 1: onboarding with no active tokens → welcome + “Add FreeFeed token”.
- State 2: onboarding with active token(s) and no feeds → welcome + “Add feed”.
- States 3/4: non-onboarding → dashboard, with an extra warning for active users without active tokens.
Copy is clear and user-friendly and lines up with tests/manual. If you want to reduce repetition, you could nest on
@user.onboarding?once:<% if @user.onboarding? %> <% if !@has_active_tokens %> ... <% elsif @has_active_tokens && !@has_feeds %> ... <% end %> <% else %> ... <% end %>Current structure is perfectly fine if you prefer it.
app/controllers/feeds_controller.rb (1)
57-61: Good placement of onboarding→active transition inside transaction; consider tying tocurrent_userexplicitlyPutting
Current.user.active! if Current.user.onboarding?inside theActiveRecord::Base.transactionthat wraps@feed.save!andreset_schedule!is the right place: validation failures won’t leave the user state half-updated, and a raised error will roll back both feed and state.Since
feeds_scopeis alreadycurrent_user.feeds, you might tighten the association and avoid relying onCurrenthere:user = current_user user.active! if user.onboarding?or even
@feed.userto make the coupling explicit. Behavior stays the same; this is just for clarity and to avoid surprises ifCurrent.useris ever changed.test/controllers/feeds_controller_test.rb (1)
170-236: New#createtests cover onboarding transitions and rollback wellThe three added tests do a good job of pinning down the intended behavior:
- Onboarding user with a valid first feed moves to
active.- Already-active user remains
active.- Onboarding user stays
onboardingwhen feed validation fails.They line up with the controller transaction behavior and give nice regression coverage. If you want to tighten them further, you could:
- In the “transition user from onboarding to active” test, also assert
assert_difference("Feed.count", 1)around the POST to document clearly that it’s the first successful feed creation that completes onboarding.- In the “rollback state transition” test, keep as-is (it already uses
assert_no_difference("Feed.count")).These are optional; the current tests are already solid.
test/controllers/statuses_controller_test.rb (2)
13-44: State 1–4 status-page tests accurately exercise onboarding and active flowsThe new tests for:
- onboarding state 1 (no active tokens),
- onboarding state 2 (has token, no feeds),
- active state 3 (normal dashboard),
- active state 4 (active user with missing active tokens),
all correctly set up
state, tokens, and feeds to match the view’s branching, and assert against the right headings and CTAs. This closely mirrors the documented states and should catch regressions in the onboarding logic.If you want to make these a bit more future-proof, you could explicitly assert the token count in the state 1/4 setups (e.g., ensure there are 0 active tokens) so the tests don’t start failing unexpectedly if user factories are later changed to pre-create tokens. Not required, just a defensive tweak.
59-187: Stats visibility tests align with data-keys and state gatingThe stats-related tests (
total_feeds, imported/published posts, most recent post, averages, and “hide stats when no posts”) correctly:
- Use
state: :active+ active tokens + feeds when stats should be shown.- Use an onboarding user when stats should not appear, matching the view’s branching.
- Assert against the
data-key="stats.*"attributes, which is in line with the preferred selector strategy from the guidelines.One small improvement you might consider is adding a variant where the user is
activewith zero posts to explicitly prove that hiding of “most recent post” / “average posts per day” is independent of onboarding vs active state, but the current tests are already quite comprehensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/feeds_controller.rb(1 hunks)app/controllers/statuses_controller.rb(1 hunks)app/views/statuses/show.html.erb(3 hunks)docs/onboarding_test_manual.md(1 hunks)test/controllers/feeds_controller_test.rb(1 hunks)test/controllers/statuses_controller_test.rb(12 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{rb,js,jsx,ts,tsx,css,scss,html,erb,slim}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add trailing line breaks to source files (Ruby, JS, CSS, HTML, etc.) unless there is a specific reason not to
Files:
app/controllers/statuses_controller.rbtest/controllers/feeds_controller_test.rbtest/controllers/statuses_controller_test.rbapp/controllers/feeds_controller.rbapp/views/statuses/show.html.erb
app/controllers/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Eliminate blank action methods in controllers
Files:
app/controllers/statuses_controller.rbapp/controllers/feeds_controller.rb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use two-space indentation in code files
Files:
app/controllers/statuses_controller.rbtest/controllers/feeds_controller_test.rbtest/controllers/statuses_controller_test.rbapp/controllers/feeds_controller.rb
test/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.rb: Prefer lazy test data initialization over eager initialization in setup block using methods with memoization
Use test naming conventiontest "#method should ..."for unit tests
Preferdata-keyattributes for DOM selectors in tests instead of styling classes
Use FactoryBot to create test data
Files:
test/controllers/feeds_controller_test.rbtest/controllers/statuses_controller_test.rb
app/views/**/*.{erb,slim,html}
📄 CodeRabbit inference engine (CLAUDE.md)
app/views/**/*.{erb,slim,html}: When writing user-facing text in views, use a friendly but not overly familiar tone, be brief and clear, avoid stiff formal language and unnecessary terminology
Don't expose UI implementation details in user-facing text; avoid terms like 'wizard', 'form', 'dialog', 'modal', 'interface', 'screen'
Keepdata-keyattributes short and namespaced using the formatcomponent.element
Files:
app/views/statuses/show.html.erb
🧠 Learnings (1)
📚 Learning: 2025-10-26T17:01:41.639Z
Learnt from: dreikanter
Repo: dreikanter/f2 PR: 163
File: app/controllers/resend_webhooks_controller.rb:76-99
Timestamp: 2025-10-26T17:01:41.639Z
Learning: In the Event model, both the `user` and `subject` attributes are optional. Events can be created and saved with `nil` values for these fields, which is intentional behavior to allow tracking events even when a user cannot be found.
Applied to files:
test/controllers/statuses_controller_test.rb
🧬 Code graph analysis (3)
app/controllers/statuses_controller.rb (1)
app/models/user.rb (1)
total_feeds_count(94-96)
test/controllers/feeds_controller_test.rb (2)
app/controllers/feeds_controller.rb (2)
create(51-67)feed_params(135-146)test/support/integration_test_helpers.rb (1)
sign_in_as(2-4)
app/controllers/feeds_controller.rb (1)
test/controllers/feeds_controller_test.rb (2)
user(3-436)user(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
test/controllers/statuses_controller_test.rb (1)
188-270: Recent-events tests thoroughly cover user scoping, filtering, and limitsThe recent-events tests collectively verify:
- Only the signed-in user’s events are shown.
- Debug-level and expired events are excluded.
- The list is capped at 10 items.
- The section hides entirely when there are no events.
Using
data-key="recent_events.<id>"keeps the selectors stable and decoupled from presentation. Given prior learnings thatEvent#useris optional, asserting behavior both when it’s present (these tests) and when other users’ events exist is particularly useful to guard theEvent.where(user: @user).user_relevant.recent.limit(10)query. Based on learnings, this is a good alignment with the intended event model semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/views/feeds/_identification_loading_poll.html.erb (1)
1-12: LGTM: Clean polling status partial.The structure is appropriate, with good accessibility via
role="status". The user-facing messages are clear and friendly.Add trailing newline per coding guidelines.
The coding guidelines specify that source files should have trailing line breaks.
As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/feed_details_controller.rb(3 hunks)app/views/feeds/_form_collapsed.html.erb(1 hunks)app/views/feeds/_form_expanded.html.erb(1 hunks)app/views/feeds/_identification_error.html.erb(1 hunks)app/views/feeds/_identification_loading.html.erb(1 hunks)app/views/feeds/_identification_loading_poll.html.erb(1 hunks)test/controllers/feed_details_controller_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/views/feeds/_form_expanded.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/feed_details_controller.rb
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rb,js,jsx,ts,tsx,css,scss,html,erb,slim}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add trailing line breaks to source files (Ruby, JS, CSS, HTML, etc.) unless there is a specific reason not to
Files:
app/views/feeds/_identification_error.html.erbapp/views/feeds/_identification_loading.html.erbapp/views/feeds/_form_collapsed.html.erbtest/controllers/feed_details_controller_test.rbapp/views/feeds/_identification_loading_poll.html.erb
app/views/**/*.{erb,slim,html}
📄 CodeRabbit inference engine (CLAUDE.md)
app/views/**/*.{erb,slim,html}: When writing user-facing text in views, use a friendly but not overly familiar tone, be brief and clear, avoid stiff formal language and unnecessary terminology
Don't expose UI implementation details in user-facing text; avoid terms like 'wizard', 'form', 'dialog', 'modal', 'interface', 'screen'
Keepdata-keyattributes short and namespaced using the formatcomponent.element
Files:
app/views/feeds/_identification_error.html.erbapp/views/feeds/_identification_loading.html.erbapp/views/feeds/_form_collapsed.html.erbapp/views/feeds/_identification_loading_poll.html.erb
**/*.{rb,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use two-space indentation in code files
Files:
test/controllers/feed_details_controller_test.rb
test/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.rb: Prefer lazy test data initialization over eager initialization in setup block using methods with memoization
Use test naming conventiontest "#method should ..."for unit tests
Preferdata-keyattributes for DOM selectors in tests instead of styling classes
Use FactoryBot to create test data
Files:
test/controllers/feed_details_controller_test.rb
🧠 Learnings (2)
📚 Learning: 2025-11-29T19:38:34.155Z
Learnt from: CR
Repo: dreikanter/f2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T19:38:34.155Z
Learning: Applies to app/views/**/*.{erb,slim,html} : Keep `data-key` attributes short and namespaced using the format `component.element`
Applied to files:
app/views/feeds/_identification_loading.html.erb
📚 Learning: 2025-11-29T19:38:34.155Z
Learnt from: CR
Repo: dreikanter/f2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T19:38:34.155Z
Learning: Applies to app/views/**/*.{erb,slim,html} : Don't expose UI implementation details in user-facing text; avoid terms like 'wizard', 'form', 'dialog', 'modal', 'interface', 'screen'
Applied to files:
app/views/feeds/_identification_loading.html.erb
🔇 Additional comments (4)
app/views/feeds/_identification_error.html.erb (1)
1-1: LGTM: Consistent with data-driven state handling.The removal of the
idattribute aligns with the shift toward data-attribute-based state management across the feed identification flow.app/views/feeds/_form_collapsed.html.erb (1)
2-25: LGTM: Wrapper structure enables Turbo Stream updates.The new wrapper structure with
feed-form-wrapperandfeed-formIDs is consistent with the broader refactoring and properly nests the form and footer elements.app/views/feeds/_identification_loading.html.erb (1)
1-21: LGTM: Polling configuration properly externalized to wrapper.The restructure moves polling configuration to the outer wrapper while simplifying the CardComponent to focus on presentation. The user-facing messages are clear and friendly.
test/controllers/feed_details_controller_test.rb (1)
216-216: LGTM: Test expectation aligns with Turbo Stream update semantics.The change from
action="replace"toaction="update"correctly reflects the controller's use ofturbo_stream.updatefor thefeed-formtarget.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/views/feeds/_identification_error.html.erb(1 hunks)app/views/feeds/_identification_loading.html.erb(1 hunks)app/views/feeds/_identification_loading_poll.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/feeds/_identification_loading_poll.html.erb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,js,jsx,ts,tsx,css,scss,html,erb,slim}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add trailing line breaks to source files (Ruby, JS, CSS, HTML, etc.) unless there is a specific reason not to
Files:
app/views/feeds/_identification_loading.html.erbapp/views/feeds/_identification_error.html.erb
app/views/**/*.{erb,slim,html}
📄 CodeRabbit inference engine (CLAUDE.md)
app/views/**/*.{erb,slim,html}: When writing user-facing text in views, use a friendly but not overly familiar tone, be brief and clear, avoid stiff formal language and unnecessary terminology
Don't expose UI implementation details in user-facing text; avoid terms like 'wizard', 'form', 'dialog', 'modal', 'interface', 'screen'
Keepdata-keyattributes short and namespaced using the formatcomponent.element
Files:
app/views/feeds/_identification_loading.html.erbapp/views/feeds/_identification_error.html.erb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
app/views/feeds/_identification_loading.html.erb (1)
1-18: Polling wrapper structure and timeout math are correctThe restructuring makes sense: the outer
#feed-form-wrapperowns thepollingcontroller and its values, while the inner#feed-form+CardComponentis purely presentational. The stop condition selector for[data-identification-state='complete']and[data-identification-state='error']aligns correctly with the state-driven partials.The timeout math checks out:
Feed::IDENTIFICATION_POLLING_INTERVAL_MS = 2000(2 seconds)FeedDetailsController::IDENTIFICATION_TIMEOUT_SECONDS = 30(30 seconds)- Max polls:
30 / 2 = 15polls, which yields15 × 2000ms = 30 secondsoverall timeout ✓State consistency is solid:
_identification_error.html.erbsetsdata-identification-state="error"and_form_expanded.html.erbsetsdata-identification-state="complete", both matching the stop condition reliably.
| <div id="feed-form" | ||
| data-identification-state="error" | ||
| class="rounded-lg border border-slate-200 bg-white px-6 py-8 shadow-sm space-y-6"> | ||
| <div data-identification-state="error" class="rounded-lg border border-slate-200 bg-white px-6 py-8 shadow-sm space-y-6"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for any remaining references to feed-form in the codebase
rg -n "feed-form" --hiddenRepository: dreikanter/f2
Length of output: 1399
🏁 Script executed:
# Check the actual file content to see context
cat -n app/views/feeds/_identification_error.html.erb | head -20Repository: dreikanter/f2
Length of output: 1065
🏁 Script executed:
# Search for all data-identification-state usages to understand the pattern
rg -n "data-identification-state" --hiddenRepository: dreikanter/f2
Length of output: 1624
Critical: Missing id for turbo_stream targets and data-key naming violates guidelines
The removal of the id="feed-form" from this partial breaks turbo_stream updates. The controller at feed_details_controller.rb (lines 8, 100, 110, 120, 129) targets "feed-form" and "feed-form-wrapper" in multiple turbo_stream responses, but _identification_error.html.erb now has no id to match them.
Additionally, the data-identification-state attribute violates the coding guideline for data-keys: it should use the component.element format (e.g., data-feed="error").
Required fixes:
- Add
id="feed-form"back to the wrapper div or update all turbo_stream targets in the controller to use the data attribute selector - Rename
data-identification-stateto follow thecomponent.elementnaming convention
1 participant
Changes:
user.stateto distinguish onboarding flow from active users with deactivated tokens.