feat(issues/pulls): add j/k vim keybindings for navigation#34
feat(issues/pulls): add j/k vim keybindings for navigation#34benjamincanac merged 2 commits intovolta-net:mainfrom
Conversation
📝 WalkthroughWalkthroughAdd a generic selectNextIssue(delta) and keyboard shortcuts (j/k) to navigate issues; arrow up/down now delegate to that function. Navigation picks the first issue if none selected and prevents moving past list boundaries (no wrapping). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/issues/Issues.vue (1)
58-75: Deduplicate alias handlers to avoid future drift.
j/arrowdownandk/arrowupcurrently duplicate logic. Consider extracting shared next/previous handlers and binding aliases to those functions.Refactor sketch
+const getSelectedIndex = () => + props.issues.findIndex(issue => issue.id === selectedIssue.value?.id) + +const selectNextIssue = () => { + const index = getSelectedIndex() + if (index === -1) selectedIssue.value = props.issues[0] ?? null + else if (index < props.issues.length - 1) selectedIssue.value = props.issues[index + 1] +} + +const selectPreviousIssue = () => { + const index = getSelectedIndex() + if (index === -1) selectedIssue.value = props.issues[props.issues.length - 1] ?? null + else if (index > 0) selectedIssue.value = props.issues[index - 1] +} + defineShortcuts({ - arrowdown: () => { ... }, - arrowup: () => { ... }, - j: () => { ... }, - k: () => { ... } + arrowdown: selectNextIssue, + j: selectNextIssue, + arrowup: selectPreviousIssue, + k: selectPreviousIssue })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/issues/Issues.vue` around lines 58 - 75, Extract the duplicated navigation logic in the j and k handlers into shared functions (e.g., nextIssue() and prevIssue()) that operate on selectedIssue and props.issues: compute index via props.issues.findIndex(issue => issue.id === selectedIssue.value?.id) and then set selectedIssue.value to the next/previous item (or wrap to first/last when index === -1); replace the inline bodies of j and k with calls to those shared functions and bind the arrowdown/arrowup aliases to the same functions to avoid drift.
🤖 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/components/issues/Issues.vue`:
- Around line 52-53: The selection branches assign possibly undefined when
props.issues is empty; update all assignments to selectedIssue.value (the ones
around the index === -1 branch and the similar branches at lines 61-63 and
70-71) to default to null by using the nullish coalescing operator, e.g. set
selectedIssue.value = props.issues[props.issues.length - 1] ?? null (and the
analogous expressions) so the component consistently represents “no selection”
as null instead of undefined.
---
Nitpick comments:
In `@app/components/issues/Issues.vue`:
- Around line 58-75: Extract the duplicated navigation logic in the j and k
handlers into shared functions (e.g., nextIssue() and prevIssue()) that operate
on selectedIssue and props.issues: compute index via
props.issues.findIndex(issue => issue.id === selectedIssue.value?.id) and then
set selectedIssue.value to the next/previous item (or wrap to first/last when
index === -1); replace the inline bodies of j and k with calls to those shared
functions and bind the arrowdown/arrowup aliases to the same functions to avoid
drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41bdc507-cf21-457b-9cde-326b2eb80e69
📒 Files selected for processing (1)
app/components/issues/Issues.vue
app/components/issues/Issues.vue
Outdated
| if (index === -1) { | ||
| selectedIssue.value = props.issues[props.issues.length - 1] |
There was a problem hiding this comment.
Handle empty-list selection explicitly with null.
These branches can assign undefined when issues is empty. Use ?? null so the model consistently represents “no selection” as null.
Suggested fix
- selectedIssue.value = props.issues[props.issues.length - 1]
+ selectedIssue.value = props.issues[props.issues.length - 1] ?? null
@@
- selectedIssue.value = props.issues[0]
+ selectedIssue.value = props.issues[0] ?? null
@@
- selectedIssue.value = props.issues[props.issues.length - 1]
+ selectedIssue.value = props.issues[props.issues.length - 1] ?? nullAlso applies to: 61-63, 70-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/issues/Issues.vue` around lines 52 - 53, The selection
branches assign possibly undefined when props.issues is empty; update all
assignments to selectedIssue.value (the ones around the index === -1 branch and
the similar branches at lines 61-63 and 70-71) to default to null by using the
nullish coalescing operator, e.g. set selectedIssue.value =
props.issues[props.issues.length - 1] ?? null (and the analogous expressions) so
the component consistently represents “no selection” as null instead of
undefined.
Summary
jandkas vim-style aliases forArrowDown/ArrowUpin the issues listdefineShortcutswhich already ignores shortcuts when focus is inside an input/textarea, so typing in the search box is unaffectedTest plan
joutside any input — selected issue moves downkoutside any input — selected issue moves upj/k— no navigation triggeredSummary by CodeRabbit