Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors issue pagination to use a new composable, moves filtering from client to server (adds assignedToMe and exclusive-filter logic), updates API response shape and default page size, replaces loading text with skeleton UI, and updates tests and i18n keys to match the new contract. (46 words) Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Issue Page UI
participant Store as Issues Store
participant Composable as usePaginatedSection
participant Cache as Local Cache
participant API as Issues API
UI->>Store: request nextPage()
Store->>Composable: nextPage()
Composable->>Cache: check cached page (by cursor)
alt cached
Cache-->>Composable: return cached items
Composable->>Composable: apply cached page
else not cached
Composable->>API: fetch(after: endCursor, params)
API-->>Composable: { items, totalCount, pageInfo }
Composable->>Cache: store result by cursor
end
Composable->>Store: update reactive state (data,totalCount,hasMore)
Store-->>UI: reactive update (sortedIssues)
sequenceDiagram
participant UI as IssueToolbar UI
participant Store as Issues Store
participant API as Issues API
UI->>Store: toggleFilter("assignedToMe")
Store->>Store: enforce EXCLUSIVE_FILTERS (remove conflicts)
Store->>Store: update activeFilters
Store->>Store: buildParams() with filters
Store->>Composable: section.refresh()
Composable->>API: fetch(first:20, params)
API-->>Composable: { items, totalCount, pageInfo }
Composable->>Store: updated section.data
Store-->>UI: reactive update (sortedIssues)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
i18n/locales/en.json (1)
282-288:⚠️ Potential issue | 🟡 MinorPipeline failure: remove unused keys to fix CI.
Same as
de.json— removeissues.serverSearch,issues.filter.hasLinkedPr, andissues.filter.noLinkedPrto resolve the pipeline errors.🧹 Proposed fix
"serverSearch": "Searching...", "filter": { "assignedToMe": "Assigned to me", "unassigned": "Unassigned", - "hasLinkedPr": "Has PR", - "noLinkedPr": "No PR", "hasMilestone": "Milestone" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@i18n/locales/en.json` around lines 282 - 288, Remove the unused localization keys that are causing CI failures: delete issues.serverSearch and the two filter keys issues.filter.hasLinkedPr and issues.filter.noLinkedPr from en.json so it matches the de.json changes; update the JSON object under "filter" to only include the remaining keys (e.g., "assignedToMe", "unassigned", "hasMilestone") and ensure the file remains valid JSON after removing those entries.i18n/locales/de.json (1)
282-288:⚠️ Potential issue | 🟡 MinorPipeline failure: unused translation keys need cleanup.
The CI pipeline reports these keys as unused:
issues.serverSearch(line 282)issues.filter.hasLinkedPr(line 286)issues.filter.noLinkedPr(line 287)Since the PR removed the
hasLinkedPrandnoLinkedPrfilter chips fromIssueToolbar.vue, these keys should be removed from both locale files and the schema to fix the pipeline.🧹 Proposed fix to remove unused keys
"serverSearch": "Suche läuft...", "filter": { "assignedToMe": "Mir zugewiesen", "unassigned": "Nicht zugewiesen", - "hasLinkedPr": "Hat PR", - "noLinkedPr": "Kein PR", "hasMilestone": "Meilenstein" },Also remove
issues.serverSearchif it's no longer used in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@i18n/locales/de.json` around lines 282 - 288, Remove the unused i18n keys reported by CI: delete "issues.serverSearch", "issues.filter.hasLinkedPr", and "issues.filter.noLinkedPr" from the German locale (i18n/locales/de.json) and the corresponding English locale entry, and also remove these keys from the i18n schema/translation types so they are no longer expected by the build; ensure you also remove any remaining references in components such as IssueToolbar.vue (the PR already removed the filter chips) and run the i18n validation to confirm the pipeline error is resolved.
🧹 Nitpick comments (2)
test/nuxt/issueStore.test.ts (1)
167-175: Consider adding a test for theassignedToMefilter.The PR introduces an
assignedToMefilter, but there's no corresponding test. Consider adding one for completeness:📝 Example test
const assignedToMeIssue: Issue = { ...mockIssue, id: 'I_assigned_me', assignees: [{ login: 'currentUser', avatarUrl: '' }], } // In registerEndpoint handler: if (url.searchParams.has('assignedToMe')) { issues = [assignedToMeIssue] } // Test: it('assignedToMe filter fetches issues assigned to current user from server', async () => { await withStore(async (store) => { await store.selectRepo('org/repo') store.activeFilters = ['assignedToMe'] await store.fetchIssues() expect(store.issues).toHaveLength(1) expect(store.issues[0]?.id).toBe('I_assigned_me') }) })Would you like me to open an issue to track adding this test coverage?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt/issueStore.test.ts` around lines 167 - 175, Add a unit test that verifies the new assignedToMe filter returns only issues assigned to the current user: in test/nuxt/issueStore.test.ts add a mock Issue (e.g., id 'I_assigned_me' with assignees containing 'currentUser'), update the test server handler used in registerEndpoint to return that issue when url.searchParams.has('assignedToMe'), and add a new test case (similar to the existing unassigned test) that uses withStore, calls store.selectRepo('org/repo'), sets store.activeFilters = ['assignedToMe'], awaits store.fetchIssues(), and asserts store.issues has length 1 and store.issues[0]?.id === 'I_assigned_me'; reference functions: fetchIssues, selectRepo, withStore, and the mockIssue used for other tests to place the new mock consistently.app/composables/usePaginatedSection.ts (1)
132-139: Consider preservingtotalCounton refresh failure.When
refresh()fails,cursorHistoryis restored buttotalCountanddatamay have been cleared or left in an inconsistent state from the failed fetch. The current implementation clears the cache before fetching, so a failure leaves the user with potentially stale or empty data.This is acceptable for a refresh operation (user explicitly requested fresh data), but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/usePaginatedSection.ts` around lines 132 - 139, The refresh() implementation clears pageCache and resets cursorHistory but does not preserve totalCount or data on a failed fetch; modify refresh() to snapshot current totalCount and data (and cursorHistory as already done) before clearing, then perform the clear/fetch, and if fetchData() returns false restore cursorHistory, totalCount, and data (and optionally pageCache) to their previous values so the UI stays consistent; reference functions/vars: refresh(), fetchData(), pageCache, cursorHistory, totalCount, and data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/stores/issues.ts`:
- Around line 55-60: handleError currently assumes section.refresh() will throw
on failure, but section.refresh() instead sets section.error, so fetchIssues
(and the code that sets loaded = true around lines 87–92) ends up marking
success on failed fetches; update the fetchIssues flow to check section.error
after calling section.refresh() (or inspect its result) and, if an error exists,
call handleError(section.error) and do not set loaded to true — reference the
section.refresh() call, the section.error property, the loaded flag, and the
handleError function when making this change.
In `@server/api/issues/index.get.ts`:
- Around line 52-53: Parse the incoming query params for assignedToMe and
unassigned explicitly into booleans (e.g., compare string values to 'true' or
'1') instead of relying on truthiness, and then validate their combination
before building the GitHub search query: if both assignedToMe and unassigned are
true, return a 400 error (or otherwise reject) because building both
`assignee:${login}` and `no:assignee` is contradictory; otherwise only append
`assignee:${login}` to query when the parsed assignedToMe boolean is true and
append `no:assignee` only when the parsed unassigned boolean is true. Ensure you
reference the same variables used in this file (assignedToMe, unassigned, query,
login) when adding the parsing and validation logic.
- Around line 54-58: The milestone/label values are inserted into the GitHub
search query unescaped and the special milestone value "*" is handled
incorrectly; update the code that builds query (using the milestone variable and
the label loop that splits String(label).split(',')) to (1) treat milestone ===
"*" by skipping adding a milestone filter (or use the explicit -no:milestone
handling if that was intended) and (2) escape any backslashes and double quotes
in milestone and each label value before interpolation (e.g., replace \ with \\
and " with \"), so that query += ` milestone:"${escapedMilestone}"` and query +=
` label:"${escapedLabel}"` use the safe, escaped strings.
---
Outside diff comments:
In `@i18n/locales/de.json`:
- Around line 282-288: Remove the unused i18n keys reported by CI: delete
"issues.serverSearch", "issues.filter.hasLinkedPr", and
"issues.filter.noLinkedPr" from the German locale (i18n/locales/de.json) and the
corresponding English locale entry, and also remove these keys from the i18n
schema/translation types so they are no longer expected by the build; ensure you
also remove any remaining references in components such as IssueToolbar.vue (the
PR already removed the filter chips) and run the i18n validation to confirm the
pipeline error is resolved.
In `@i18n/locales/en.json`:
- Around line 282-288: Remove the unused localization keys that are causing CI
failures: delete issues.serverSearch and the two filter keys
issues.filter.hasLinkedPr and issues.filter.noLinkedPr from en.json so it
matches the de.json changes; update the JSON object under "filter" to only
include the remaining keys (e.g., "assignedToMe", "unassigned", "hasMilestone")
and ensure the file remains valid JSON after removing those entries.
---
Nitpick comments:
In `@app/composables/usePaginatedSection.ts`:
- Around line 132-139: The refresh() implementation clears pageCache and resets
cursorHistory but does not preserve totalCount or data on a failed fetch; modify
refresh() to snapshot current totalCount and data (and cursorHistory as already
done) before clearing, then perform the clear/fetch, and if fetchData() returns
false restore cursorHistory, totalCount, and data (and optionally pageCache) to
their previous values so the UI stays consistent; reference functions/vars:
refresh(), fetchData(), pageCache, cursorHistory, totalCount, and data.
In `@test/nuxt/issueStore.test.ts`:
- Around line 167-175: Add a unit test that verifies the new assignedToMe filter
returns only issues assigned to the current user: in
test/nuxt/issueStore.test.ts add a mock Issue (e.g., id 'I_assigned_me' with
assignees containing 'currentUser'), update the test server handler used in
registerEndpoint to return that issue when url.searchParams.has('assignedToMe'),
and add a new test case (similar to the existing unassigned test) that uses
withStore, calls store.selectRepo('org/repo'), sets store.activeFilters =
['assignedToMe'], awaits store.fetchIssues(), and asserts store.issues has
length 1 and store.issues[0]?.id === 'I_assigned_me'; reference functions:
fetchIssues, selectRepo, withStore, and the mockIssue used for other tests to
place the new mock consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/issue/IssueRowSkeleton.vueapp/components/issue/IssueToolbar.vueapp/composables/usePaginatedSection.tsapp/pages/issues/index.vueapp/stores/focus.tsapp/stores/issues.tsi18n/locales/de.jsoni18n/locales/en.jsoni18n/schema.jsonserver/api/issues/index.get.tstest/nuxt/issueStore.test.ts
💤 Files with no reviewable changes (1)
- app/stores/focus.ts
server/api/issues/index.get.ts
Outdated
Comment on lines
+54
to
+58
| if (milestone) query += ` milestone:"${milestone}"` | ||
| if (label) { | ||
| for (const l of String(label).split(',')) { | ||
| if (l.trim()) query += ` label:"${l.trim()}"` | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to understand the search system context
cat -n server/api/issues/index.get.ts | head -80Repository: flumen-dev/flumen.dev
Length of output: 2938
🌐 Web query:
GitHub search syntax: milestone qualifier, how to query "any milestone" or "has milestone"
💡 Result:
In GitHub issues/PR search:
- Specific milestone:
milestone:"My Milestone"(quote if it contains spaces) [1] - No milestone set:
no:milestone[2] - “Has any milestone” / “any milestone”: negate the “no milestone” qualifier:
-no:milestone(GitHub search supports excluding a term/qualifier by prefixing it with-) [3]
Examples:
- All open issues with any milestone:
is:issue is:open -no:milestone - All open issues with no milestone:
is:issue is:open no:milestone
Note: There isn’t a documented wildcard like milestone:* for classic issues/PR search; -no:milestone is the usual way to express “has milestone.” [2][3]
Sources: [1] [2] [3]
Escape milestone/label qualifier values and fix milestone wildcard handling.
Unescaped quote/backslash characters can break search qualifiers (e.g., if milestone contains ", the query becomes malformed). Additionally, milestone:"*" does not represent "any milestone" in GitHub search syntax; the correct syntax is to omit the filter or use the -no:milestone pattern.
🔧 Suggested fix
+const escapeSearchValue = (value: string) => value.replace(/["\\]/g, '\\$&').trim()
+
- if (milestone) query += ` milestone:"${milestone}"`
+ if (milestone && milestone !== '*') {
+ const m = escapeSearchValue(String(milestone))
+ if (m) query += ` milestone:"${m}"`
+ }
if (label) {
for (const l of String(label).split(',')) {
- if (l.trim()) query += ` label:"${l.trim()}"`
+ const safe = escapeSearchValue(l)
+ if (safe) query += ` label:"${safe}"`
}
}📝 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.
Suggested change
| if (milestone) query += ` milestone:"${milestone}"` | |
| if (label) { | |
| for (const l of String(label).split(',')) { | |
| if (l.trim()) query += ` label:"${l.trim()}"` | |
| } | |
| const escapeSearchValue = (value: string) => value.replace(/["\\]/g, '\\$&').trim() | |
| if (milestone && milestone !== '*') { | |
| const m = escapeSearchValue(String(milestone)) | |
| if (m) query += ` milestone:"${m}"` | |
| } | |
| if (label) { | |
| for (const l of String(label).split(',')) { | |
| const safe = escapeSearchValue(l) | |
| if (safe) query += ` label:"${safe}"` | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/issues/index.get.ts` around lines 54 - 58, The milestone/label
values are inserted into the GitHub search query unescaped and the special
milestone value "*" is handled incorrectly; update the code that builds query
(using the milestone variable and the label loop that splits
String(label).split(',')) to (1) treat milestone === "*" by skipping adding a
milestone filter (or use the explicit -no:milestone handling if that was
intended) and (2) escape any backslashes and double quotes in milestone and each
label value before interpolation (e.g., replace \ with \\ and " with \"), so
that query += ` milestone:"${escapedMilestone}"` and query += `
label:"${escapedLabel}"` use the safe, escaped strings.
This was referenced Mar 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related issue(s)
Closes #30
Type of change
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Localization