feat: add Sprints keyboard shortcut (gt) - #99
Conversation
|
@SinghAstra is attempting to deploy a commit to the MagicAPI Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe Sprints navigation link now supports the ChangesSprints keyboard navigation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/tests/components/command-palette.test.tsx`:
- Around line 121-132: Add test coverage for buildSidebarNav’s Sprints entry,
verifying it exposes the /sprints route with binding “g t”. Update the
navigation test setup or add a focused sidebar test so it exercises
buildSidebarNav rather than only buildNavigation, and ensure the assertion would
fail if that binding or route regresses.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ab32166-638c-40af-982b-54dd71c183e1
📒 Files selected for processing (2)
apps/web/src/lib/navigation.tsapps/web/tests/components/command-palette.test.tsx
imshashank
left a comment
There was a problem hiding this comment.
Thanks for this. g t is free, the binding registers correctly, and bun run verify is green with your branch merged into current main. A few changes before it lands.
1. Resolve the CodeRabbit thread. It is a fair catch, not noise: command-palette.test.tsx builds its sections with buildNavigation, so nothing in the suite touches the copy of the link you added to buildSidebarNav. Delete the binding from buildSidebarNav and every test still passes, which means the sidebar half of your change is untested. House rule is that a feature is not done until it has a test that would fail if the feature broke. Please add one that asserts /sprints carries binding: 'g t' in the sidebar nav, and reply on or resolve the thread yourself. We do not resolve contributor threads on their behalf.
2. Please revert the unrelated reformatting. Roughly half the diff is single-line link objects expanded across five lines each. Those lines are inside Biome's line width so the formatter does not require it, bun run lint passes either way, and it buries the one line that matters. Keep the diff to the binding: 'g t' addition and the test.
3. Merge main into your branch. It has moved a long way since you opened this and the fork never ran the lint, typecheck, test or build jobs, so nothing here has been checked against the tree it would land on. The Vercel entry is a fork authorisation prompt, not a build failure, so ignore that one.
Not something to fix here, but worth knowing since it is why you had to make the same edit twice: buildNavigation and buildSidebarNav maintain two copies of the same link table, so every navigation change has to be made in both and only one of them has test coverage. That duplication is on us and I am tracking it separately. If you would rather wait for that to be collapsed into one source before adding the binding, say so and I will ping you when it lands.
…pstream/main' into feat/add-cycles-shortcut
|
Heads up: the duplication I mentioned is now gone. #101 collapsed That means your change gets simpler: after you merge |
|
Sure i will fix it right away thanks for simplifying with single object |
|
Heads up:
#107 (merged, Two things to do:
The unrelated reformatting of The CodeRabbit thread on this PR is still open and needs your reply, as does the one on #95. |
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/tests/components/command-palette.test.tsx`:
- Around line 127-145: Extend the test around Palette and StandupTimer to verify
the competing binding is active: after the Sprints command closes, send
standalone t, assert toggleTimer was called once, then clear the mock before
sending gt and retain the existing navigation and non-toggle assertions. If
standalone t does not trigger, configure the active standup scope in the test
setup.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f67e556-994d-4e26-83c4-69c1c02c3f78
📒 Files selected for processing (2)
apps/web/src/lib/navigation.tsapps/web/tests/components/command-palette.test.tsx
✅ Action performedComments resolved. Approval is disabled; enable |
Fixes #98
What Changed
Added the missing workspace keyboard navigation shortcut for Sprints to maintain consistency with the rest of the sidebar navigation.
As noted in the issue, I used
g tinstead of the initially intuitiveg cto prevent a direct collision with the globalc(Create issue) single-key shortcut, which was intercepting the chord.Verification
binding: 'g t'to bothbuildNavigationandbuildSidebarNavinapps/web/src/lib/navigation.ts.apps/web/tests/components/command-palette.test.tsxverifying thegtroute.g ton hover in the UI.Greptile Summary
Adds the missing
g tworkspace shortcut for navigating to Sprints.g troutes to Sprints while a standalonetstill invokes its competing surface action.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
tbinding.Reviews (6): Last reviewed commit: "test: prove standalone t binding is acti..." | Re-trigger Greptile