refactor: simplify posts route tag handling#105
Conversation
WalkthroughThe PR removes URL encoding and decoding transformations for tag names across routing and tag processing logic. Tag names are now passed as raw values without Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/assets/router-helper.test.ts (1)
24-44: 🛠️ Refactor suggestion | 🟠 MajorAdd an explicit regression test for
|inside a tag nameCurrent coverage misses the separator-collision edge case. Add a test that documents/enforces expected behavior when a tag includes
|(e.g., throw, escape, or alternate encoding), so this does not regress silently.Example test shape
+ it('rejects tag names containing the "|" separator', () => { + expect(() => + generatePostsRoute( + '/posts', + 'safebooru.org', + undefined, + [new Tag({ name: 'foo|bar' })], + undefined + ) + ).toThrow() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/assets/router-helper.test.ts` around lines 24 - 44, Add a regression test to ensure generatePostsRoute does not treat a literal '|' inside a Tag name as the separator: create a Tag with a name containing '|' (e.g., new Tag({ name: 'foo|bar' })), call generatePostsRoute with multiple tags including that tag, then assert that the resulting route.query.tags preserves that tag as a single value (i.e., the '|' is escaped/encoded or otherwise not split) by checking String(route.query?.tags) and toMatchObject so future changes to the router serialization won’t silently split or lose the pipe character.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/assets/router-helper.test.ts`:
- Around line 24-44: Add a regression test to ensure generatePostsRoute does not
treat a literal '|' inside a Tag name as the separator: create a Tag with a name
containing '|' (e.g., new Tag({ name: 'foo|bar' })), call generatePostsRoute
with multiple tags including that tag, then assert that the resulting
route.query.tags preserves that tag as a single value (i.e., the '|' is
escaped/encoded or otherwise not split) by checking String(route.query?.tags)
and toMatchObject so future changes to the router serialization won’t silently
split or lose the pipe character.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03039255-5fb2-43f3-b573-67d02b9dfc8a
📒 Files selected for processing (4)
assets/js/RouterHelper.tspages/posts/[domain].vuepages/premium/saved-posts/[domain].vuetest/assets/router-helper.test.ts
💤 Files with no reviewable changes (2)
- pages/premium/saved-posts/[domain].vue
- pages/posts/[domain].vue
|
|
||
| if (tags != null && Array.isArray(tags) && tags.length) { | ||
| route.query.tags = tags.map((tag) => encodeURIComponent(tag.name)).join('|') | ||
| route.query.tags = tags.map((tag) => tag.name).join('|') |
There was a problem hiding this comment.
Prevent separator-collision in tag serialization
Line 25 joins raw tag names with |, so any tag containing | becomes lossy/ambiguous when parsed back. This silently corrupts filtering for that input.
Proposed defensive fix
if (tags != null && Array.isArray(tags) && tags.length) {
+ if (tags.some((tag) => tag.name.includes('|'))) {
+ throw new Error('Tag names cannot contain "|" because it is used as the route separator')
+ }
route.query.tags = tags.map((tag) => tag.name).join('|')
}📝 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.
| route.query.tags = tags.map((tag) => tag.name).join('|') | |
| if (tags != null && Array.isArray(tags) && tags.length) { | |
| if (tags.some((tag) => tag.name.includes('|'))) { | |
| throw new Error('Tag names cannot contain "|" because it is used as the route separator') | |
| } | |
| route.query.tags = tags.map((tag) => tag.name).join('|') | |
| } |
|
Following up on the CodeRabbit suggestion about handling literal I intentionally did not apply that change in this PR. Reason:
That delimiter behavior is currently baked into:
So for this PR, I left the delimiter contract unchanged on purpose. If we want to support literal |
1 participant
Summary
|-joined tag format while simplifying both posts pages and RouterHelper behaviorValidation
.nuxt/tsconfig.jsonpnpm exec vitest run test/assets/router-helper.test.tsSummary by CodeRabbit
Bug Fixes
Tests