refactor: centralize slugify in utils (#30)#32
refactor: centralize slugify in utils (#30)#32clean6378-max-it wants to merge 3 commits intomasterfrom
Conversation
Add utils.slugify.slugify (ASCII-safe, matches former export_api). Remove duplicate implementations from api/export_api.py and scripts/export.py; add regression tests. Closes #30
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConsolidates two local slugify implementations into a single ChangesSlug Consolidation and API Response Update
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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: 2
🤖 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 `@api/export_api.py`:
- Around line 96-99: When slugify(project["name"]) can return an empty string
causing rel_path to start with a slash, ensure proj_slug is replaced with a safe
fallback before building rel_path: set proj_slug = slugify(project["name"]) or a
default (e.g., "project" or short_id) and use that sanitized value when
composing rel_path (alongside ts_file, title_slug, short_id) so the zip entry is
never absolute-like.
In `@scripts/export.py`:
- Around line 370-373: slugify(session["title"]) and slugify(project["name"])
can return an empty string; update the assignments for title_slug and
project_slug to fall back to a stable value when empty (e.g., use short_id,
"untitled", or a combination like f"untitled-{short_id}"). Specifically, after
computing title_slug = slugify(session["title"]) and project_slug =
slugify(project["name"]), replace them with logic that checks for a falsy string
and assigns a fallback (reference the variables title_slug, project_slug,
short_id and sid) so exported filenames/paths never contain empty slugs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c67d3b87-cc50-4ce8-a577-dc7c99c29490
📒 Files selected for processing (4)
api/export_api.pyscripts/export.pytests/test_slugify.pyutils/slugify.py
| def slugify(text: str) -> str: | ||
| """Lowercase *text* and replace each run of non-[a-z0-9] with a single hyphen.""" | ||
| text = text.lower() | ||
| text = re.sub(r"[^a-z0-9]+", "-", text) |
There was a problem hiding this comment.
Behavior-change comment ("AT&T" → "at-t", "issue#42" → "issue-42") belongs on line 17 — that's the regex.
| import re | ||
|
|
||
|
|
||
| def slugify(text: str) -> str: |
There was a problem hiding this comment.
Add a default arg so the fallback lives in the helper:
def slugify(text: str, *, default: str = "") -> str:
...
return text.strip("-") or default
Then the six callers below collapse to slugify(title, default="session").
| title_slug = _slugify(session["title"]) | ||
| short_id = sid[:8] | ||
| project_slug = _slugify(project["name"]) | ||
| title_slug = slugify(session["title"]) or "session" |
There was a problem hiding this comment.
Same — use the proposed default= arg instead of or "session" / or "project".
| short_id = sid[:8] | ||
| project_slug = _slugify(project["name"]) | ||
| title_slug = slugify(session["title"]) or "session" | ||
| project_slug = slugify(project["name"]) or "project" |
There was a problem hiding this comment.
The short_id / title_slug reorder isn't part of the rename. Please revert so the diff is purely _slugify → slugify.
| def _export_single(session: dict, stats: dict, fmt: str, out_dir: str): | ||
| """Write one session to disk as md, json, or both.""" | ||
| title_slug = _slugify(session["title"]) | ||
| short_id = session["session_id"][:8] | ||
| title_slug = slugify(session["title"]) or "session" |
There was a problem hiding this comment.
Same - revert the short_id / title_slug reorder
|
|
||
|
|
||
| def test_digits_preserved(): | ||
| assert slugify("Issue 42 Fix") == "issue-42-fix" |
There was a problem hiding this comment.
Please add one parity test that proves the API and CLI export paths produce identical filenames for the same session - that's the contract this PR is meant to lock.
Either trivial or fixture-driven through both modules' filename construction
2 participants
Summary
Consolidates
_slugifyinto a single implementation underutils.slugify.slugify, completing the residual CCC8 work after PR #24 (session-text exclusion plumbing).api/export_api.pyandscripts/export.pynow both import the shared helper; duplicate local definitions are removed.Changes
utils/slugify.pywithslugify(text: str) -> str: lowercase, replace runs of non-[a-z0-9]with-, strip hyphens (matches formerexport_apibehavior).api/export_api.py: drop local_slugify, usefrom utils.slugify import slugify.scripts/export.py: drop character-loop_slugify(isalnum/ Unicode-preserving), use sharedslugify.tests/test_slugify.pyfor ASCII, punctuation collapse, Unicode titles, empty-ish input, and digits.Behavior note
Canonical slugging is ASCII-only (same as the previous HTTP export path). CLI export filenames for titles with non-ASCII letters may change vs the old script (e.g. accented characters are folded into hyphens like the web bulk export). Zip and download filenames stay predictable across platforms.
Testing
pytest— 186 passed locally.References
claude-code-chat-browser— Exclusion rule consolidation: residual_slugifydivergence (**CCC8**) (3 pt) #30_slugifydivergence);claude-cursor.mdCCC8Summary by CodeRabbit