feat: overall progress, completion celebration, first-launch welcome (#14, #15, #16)#31
Conversation
Closes #15, closes #16. Re-implemented on v0.4.0 after the pylings->pythonlings rename (the original PRs #20/#21 were built on the old layout). - #15: the progress bar now shows overall curriculum progress alongside the current topic. New pure format_progress() in widgets/progress.py; new core.state.completed_count() counts only curriculum members so stale state (renamed/removed exercises) can't push the display past 100% — addresses a code-review finding on the original PR. - #16: finishing the final pending exercise overall shows a celebration (count, shareable line, star nudge) instead of the topic-complete message; gated on next_pending(manifest.exercises, completed) is None, so it fires exactly once at true 100% and stays non-blocking. - Unit tests for format_progress, completed_count (incl. stale-entry case), and celebration_message.
Closes #14. Re-implemented on v0.4.0. On a genuine first launch (no prior progress) and only when launching into an exercise, pythonlings shows a dismissible welcome overlay explaining the loop (edit -> save -> check reruns -> remove '# I AM NOT DONE' to advance) plus the key shortcuts. Reuses the persisted state.seen_intro flag so it shows once; dismiss with Enter/Esc; non-blocking. Not shown for 'pythonlings topics', which already has the picker's first-run banner. The shared pilot helper now seeds seen_intro so returning-user tests are unaffected; the first-run banner test keeps genuine first-run state. Centered modal styling mirrors the docs window.
📝 WalkthroughWalkthroughAdds three first-run and retention features: a ChangesWelcome Screen, Overall Progress Bar, and Celebration Message
Sequence Diagram(s)sequenceDiagram
actor User
participant App
participant WelcomeScreen
participant TrackScreen
participant ProgressBar
participant StateModule
User->>App: Launch (first run)
App->>TrackScreen: push_screen(topic, exercise)
App->>WelcomeScreen: push_screen (first_launch=True)
WelcomeScreen-->>User: Show onboarding modal
User->>WelcomeScreen: Press Enter
WelcomeScreen->>StateModule: seen_intro=True then save_state
WelcomeScreen->>App: dismiss()
User->>TrackScreen: Complete an exercise
TrackScreen->>StateModule: completed_count(all_exercises, state.completed)
StateModule-->>TrackScreen: overall_completed int
TrackScreen->>ProgressBar: update_progress(topic_done, topic_total, overall_done, overall_total)
ProgressBar-->>User: Two-line topic and overall progress bar
alt All exercises complete
TrackScreen-->>User: celebration_message(total)
else Topic complete only
TrackScreen-->>User: Topic complete with F4 hint
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pythonlings/screens/track.py`:
- Around line 195-203: The else branch in track.py calls show_final() which
displays a global completion header, creating misleading messaging when only the
current topic is complete. Create a new method show_topic_complete() in the
OutputPanel class in pythonlings/widgets/output_panel.py that matches the
behavior of show_final() but sets the header to "Topic complete" instead of the
global completion header. Then replace the show_final() call in the else branch
with a call to this new show_topic_complete() method, passing the topic
completion message.
In `@pythonlings/screens/welcome.py`:
- Around line 18-19: The onboarding message on line 18 contains contradictory
instructions that say to save but then state checks rerun "as you type", which
conflicts with the save-driven loop workflow. Update the welcome screen message
text to clarify that checks rerun when you save (remove the "as you type"
phrase) so the instructions are consistent with the actual save-driven behavior
and align with the PR objectives. The message is part of a string literal in the
welcome.py file that instructs users on the workflow.
- Around line 29-32: The BINDINGS class attribute is defined as a list, which is
a mutable shared class attribute that triggers the RUF012 linting rule and can
cause unintended mutations. Convert the BINDINGS list to a tuple by replacing
the square brackets with parentheses around the Binding declarations, keeping
all the Binding elements unchanged.
In `@tests/tui/test_welcome_pilot.py`:
- Around line 1-3: The test_welcome_pilot.py module is missing the required
future-annotations import at the top. Add `from __future__ import annotations`
as the very first line in the file, before the existing imports of shutil and
pathlib.Path, to comply with the coding guidelines that require this import in
all Python modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75663634-681f-4b8f-834b-713b6d23e2d9
📒 Files selected for processing (11)
pythonlings/app.pypythonlings/core/state.pypythonlings/pythonlings.tcsspythonlings/screens/track.pypythonlings/screens/welcome.pypythonlings/widgets/progress.pytests/tui/test_app_pilot.pytests/tui/test_welcome_pilot.pytests/unit/test_celebration.pytests/unit/test_progress.pytests/unit/test_welcome.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
tests/unit/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep unit tests in
tests/unit/directory
Files:
tests/unit/test_celebration.pytests/unit/test_welcome.pytests/unit/test_progress.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python 3.11+ idioms in all Python code
Use 4-space indentation in all Python code
Prefer small, typed functions where practical in Python code
**/*.py: Guard newer-stdlib usage withrequires-python = ">=3.9"and use fallbacks (e.g.tomllibfalls back totomli) in modules likecore/manifest.py
Includefrom __future__ import annotationsat the top of Python modules
Files:
tests/unit/test_celebration.pytests/unit/test_welcome.pytests/unit/test_progress.pypythonlings/screens/welcome.pytests/tui/test_app_pilot.pypythonlings/core/state.pypythonlings/app.pytests/tui/test_welcome_pilot.pypythonlings/widgets/progress.pypythonlings/screens/track.py
tests/**/*test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Name test files as
test_<behavior>.py
Files:
tests/unit/test_celebration.pytests/unit/test_welcome.pytests/unit/test_progress.pytests/tui/test_app_pilot.pytests/tui/test_welcome_pilot.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Name test functions astest_<expected_behavior>
Use pytest for all tests with pytest-asyncio in auto mode for async tests
Files:
tests/unit/test_celebration.pytests/unit/test_welcome.pytests/unit/test_progress.pytests/tui/test_app_pilot.pytests/tui/test_welcome_pilot.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name tests as
test_<behavior>.pyortest_<expected_behavior>(e.g.,test_runner.py,test_state.py)
Files:
tests/unit/test_celebration.pytests/unit/test_welcome.pytests/unit/test_progress.pytests/tui/test_app_pilot.pytests/tui/test_welcome_pilot.py
pythonlings/pythonlings.tcss
📄 CodeRabbit inference engine (AGENTS.md)
TUI styles must be defined in
pythonlings/pythonlings.tcss
Files:
pythonlings/pythonlings.tcss
pythonlings/{screens,widgets}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Textual screens and widgets must live in
pythonlings/screens/andpythonlings/widgets/directories
Files:
pythonlings/screens/welcome.pypythonlings/widgets/progress.pypythonlings/screens/track.py
pythonlings/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep UI behavior in
screens/orwidgets/directories; keep filesystem, manifest, reset, and runner behavior incore/
Files:
pythonlings/screens/welcome.pypythonlings/core/state.pypythonlings/app.pypythonlings/widgets/progress.pypythonlings/screens/track.py
tests/tui/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep TUI tests in
tests/tui/directory
Files:
tests/tui/test_app_pilot.pytests/tui/test_welcome_pilot.py
pythonlings/core/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Core exercise loading, workspace setup, state, reset, solutions, and runner logic must live in
pythonlings/core/directoryKeep UI behavior in
screens/andwidgets/modules; keep behavior logic incore/modules—do not import UI in core
Files:
pythonlings/core/state.py
🪛 Ruff (0.15.17)
pythonlings/screens/welcome.py
[warning] 29-32: Mutable default value for class attribute
(RUF012)
🔇 Additional comments (5)
pythonlings/screens/track.py (1)
13-13: LGTM!Also applies to: 22-30, 96-102
tests/unit/test_celebration.py (1)
1-10: LGTM!pythonlings/app.py (1)
31-40: LGTM!tests/tui/test_app_pilot.py (1)
23-25: LGTM!Also applies to: 75-78
tests/tui/test_welcome_pilot.py (1)
4-63: LGTM!
| all_exercises = self.app.manifest.exercises | ||
| if next_pending(all_exercises, self.app.state.completed) is None: | ||
| self.query_one(OutputPanel).show_final( | ||
| celebration_message(len(all_exercises)) | ||
| ) | ||
| else: | ||
| self.query_one(OutputPanel).show_final( | ||
| f"Topic '{self.topic}' complete — press F4 for topics." | ||
| ) |
There was a problem hiding this comment.
Topic-only completion still renders a global “All exercises complete” header.
In the else branch (not fully complete curriculum), show_final(...) is still used. That method hardcodes the global completion header in pythonlings/widgets/output_panel.py, so users can see a misleading completion state when only the current topic is done.
Suggested direction
- self.query_one(OutputPanel).show_final(
+ self.query_one(OutputPanel).show_topic_complete(
f"Topic '{self.topic}' complete — press F4 for topics."
)# pythonlings/widgets/output_panel.py (new helper)
def show_topic_complete(self, message: str) -> None:
self.remove_class("failed", "pending")
self.add_class("passed")
self.query_one("`#hint`", Static).remove_class("visible")
self.query_one("`#output-header`", Static).update("[bold green]Topic complete[/bold green]")
self.query_one("`#goal`", Static).update("")
self.query_one("`#docs`", Static).update("")
self.query_one("`#status`", Static).update(message)
self.query_one("`#next-step`", Static).update("")
self.query_one("`#details`", Static).update("")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pythonlings/screens/track.py` around lines 195 - 203, The else branch in
track.py calls show_final() which displays a global completion header, creating
misleading messaging when only the current topic is complete. Create a new
method show_topic_complete() in the OutputPanel class in
pythonlings/widgets/output_panel.py that matches the behavior of show_final()
but sets the header to "Topic complete" instead of the global completion header.
Then replace the show_final() call in the else branch with a call to this new
show_topic_complete() method, passing the topic completion message.
| " 2. Save -- the check reruns automatically as you type.\n" | ||
| " 3. Remove the `# I AM NOT DONE` marker to advance to the next one.\n\n" |
There was a problem hiding this comment.
Clarify the onboarding loop sentence to remove contradictory behavior.
Line 18 says users should save, then says checks rerun “as you type,” which conflicts with the save-driven loop described in the PR objectives.
Proposed copy tweak
- " 2. Save -- the check reruns automatically as you type.\n"
+ " 2. Save -- checks rerun automatically.\n"📝 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.
| " 2. Save -- the check reruns automatically as you type.\n" | |
| " 3. Remove the `# I AM NOT DONE` marker to advance to the next one.\n\n" | |
| " 2. Save -- checks rerun automatically.\n" | |
| " 3. Remove the `# I AM NOT DONE` marker to advance to the next one.\n\n" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pythonlings/screens/welcome.py` around lines 18 - 19, The onboarding message
on line 18 contains contradictory instructions that say to save but then state
checks rerun "as you type", which conflicts with the save-driven loop workflow.
Update the welcome screen message text to clarify that checks rerun when you
save (remove the "as you type" phrase) so the instructions are consistent with
the actual save-driven behavior and align with the PR objectives. The message is
part of a string literal in the welcome.py file that instructs users on the
workflow.
| BINDINGS = [ | ||
| Binding("enter", "start", "Start", priority=True), | ||
| Binding("escape", "start", "Start"), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pythonlings/screens/welcome.py | sed -n '25,35p'Repository: abhiksark/pythonlings
Length of output: 444
🌐 Web query:
Ruff RUF012 rule mutable class attribute list comprehension
💡 Result:
The Ruff rule RUF012, titled "mutable-class-default," identifies mutable default values assigned to class attributes [1]. Such attributes are shared across all instances of the class, which can lead to unintended side effects where changes in one instance unexpectedly impact others [1]. While the rule is commonly triggered by simple assignments like my_list: list[int] = [], it also applies to list comprehensions or any other expressions that result in a mutable data structure [1][2]. To resolve RUF012 violations, consider the following approaches: 1. Initialize in init: Move the assignment into the class constructor (init) so that each instance gets its own unique, mutable object [1][3]. 2. Use typing.ClassVar: If the attribute is intentionally meant to be shared across all instances, annotate it with typing.ClassVar to explicitly signal this intent [1]. 3. Use immutable types: Replace the mutable structure with an immutable alternative, such as a tuple instead of a list [1]. 4. Use Optional: Assign None as the default value and handle initialization within the class logic [1][3]. Note that RUF012 generally excludes certain frameworks—such as Pydantic or msgspec—that have built-in copy semantics for their model fields, as these frameworks automatically handle the instantiation of unique values [2][4]. Diagnostic messages for this rule typically suggest these fixes to help you choose the most appropriate pattern for your code [5][6].
Citations:
- 1: https://docs.astral.sh/ruff/rules/mutable-class-default/
- 2: https://github.com/astral-sh/ruff/blob/main/crates%2Fruff_linter%2Fsrc%2Frules%2Fruff%2Frules%2Fmutable_class_default.rs
- 3: Improve docs for RUF012 astral-sh/ruff#15804
- 4: https://fossies.org/linux/ruff/crates/ruff_linter/resources/test/fixtures/ruff/RUF012.py
- 5: [
ruff] Improve the diagnostic forRUF012astral-sh/ruff#23202 - 6: astral-sh/ruff@3795db1
🏁 Script executed:
rg "BINDINGS\s*=" pythonlings/ -A 2Repository: abhiksark/pythonlings
Length of output: 912
🌐 Web query:
Textual framework BINDINGS class attribute mutable RUF012
💡 Result:
In the Ruff linter, RUF012 is a rule that identifies mutable default values in class attributes [1][2]. What it does: It flags class attributes that are assigned mutable data structures (such as lists, sets, or dictionaries) directly in the class body [1][2]. Because these objects are created when the class is defined, they are shared across all instances of that class, which often leads to unintended side effects when one instance modifies the shared state [1][2]. Why it is flagged: Mutable default values in class bodies are generally considered an anti-pattern because they create implicit shared state between instances [1]. Common fixes: - Initialize the attribute in the class's init method, which ensures each instance gets its own unique copy of the mutable structure [1][3]. - Annotate the attribute with typing.ClassVar if you explicitly intend for the variable to be shared across all instances [1]. - Use an immutable type, such as a tuple instead of a list, or types.MappingProxyType for dictionaries [1][3]. - Use None as the default value and initialize the object within init [1][3]. Note: The diagnostic message and documentation for RUF012 were improved to clarify these options, as previous versions primarily suggested ClassVar even when initialization in init was the preferred architectural fix [4][5][6]. The rule generally excludes specific frameworks or patterns where default values are intentionally copied on instance creation (e.g., certain uses of Pydantic or msgspec) [7].
Citations:
- 1: https://docs.astral.sh/ruff/rules/mutable-class-default/
- 2: https://github.com/astral-sh/ruff/blob/main/crates%2Fruff_linter%2Fsrc%2Frules%2Fruff%2Frules%2Fmutable_class_default.rs
- 3: Improve docs for RUF012 astral-sh/ruff#15804
- 4: [
ruff] Improve the diagnostic forRUF012astral-sh/ruff#23202 - 5: astral-sh/ruff@3795db1
- 6: add instance variable examples to RUF012 astral-sh/ruff#15982
- 7: https://fossies.org/linux/ruff/crates/ruff_linter/src/rules/ruff/rules/mutable_class_default.rs
Use a tuple instead of a list for the BINDINGS class attribute to avoid Ruff RUF012 and prevent accidental mutation.
The list-based BINDINGS is a mutable class attribute shared across all instances, which triggers the RUF012 rule and can lead to unintended side effects.
Suggested change
- BINDINGS = [
+ BINDINGS = (
Binding("enter", "start", "Start", priority=True),
Binding("escape", "start", "Start"),
- ]
+ )🧰 Tools
🪛 Ruff (0.15.17)
[warning] 29-32: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pythonlings/screens/welcome.py` around lines 29 - 32, The BINDINGS class
attribute is defined as a list, which is a mutable shared class attribute that
triggers the RUF012 linting rule and can cause unintended mutations. Convert the
BINDINGS list to a tuple by replacing the square brackets with parentheses
around the Binding declarations, keeping all the Binding elements unchanged.
Source: Linters/SAST tools
| import shutil | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
Add from __future__ import annotations at the module top.
This new Python module is missing the required future-annotations import.
Proposed fix
+from __future__ import annotations
+
import shutil
from pathlib import PathAs per coding guidelines, **/*.py: Include from __future__ import annotations at the top of Python modules.
📝 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.
| import shutil | |
| from pathlib import Path | |
| from __future__ import annotations | |
| import shutil | |
| from pathlib import Path |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tui/test_welcome_pilot.py` around lines 1 - 3, The
test_welcome_pilot.py module is missing the required future-annotations import
at the top. Add `from __future__ import annotations` as the very first line in
the file, before the existing imports of shutil and pathlib.Path, to comply with
the coding guidelines that require this import in all Python modules.
Source: Coding guidelines
…elcome-copy fix: topic-complete header + welcome copy (CodeRabbit follow-up to #31)
1 participant
Closes #14
Closes #15
Closes #16
Re-implements the three still-novel 0.3.0 TUI features on top of v0.4.0, after the
pylings→pythonlingsrename made the original PRs (#20/#21) unmergeable. #13 was dropped — main's zero-config first-run already covers it. Built test-first (TDD); the code-review findings from the original PRs are incorporated.#15 — overall progress
The progress bar now shows overall curriculum progress alongside the current topic. New pure
format_progress(); newcore.state.completed_count()counts only curriculum members, so stale state (renamed/removed exercises) can't push the display past 100% — fixes the >100% finding from the original review.#16 — completion celebration
Finishing the final pending exercise overall shows a celebration (count, shareable line, ⭐ nudge) instead of the topic-complete message. Gated on
next_pending(manifest.exercises, completed) is None→ fires exactly once at true 100%; non-blocking.#14 — first-launch welcome
On a genuine first launch (and only when launching into an exercise) a dismissible overlay explains the loop + key shortcuts. Reuses the persisted
seen_introflag (shows once); Enter/Esc to dismiss. Not shown forpythonlings topics(the picker already has its first-run banner).Tests
9 new tests (format_progress, completed_count incl. stale-entry case, celebration_message, welcome content, 3 welcome pilot tests). Full suite 136 → 145 passed, verified on Python 3.9 and 3.13.
Summary by CodeRabbit
Release Notes
New Features
Tests