Polish: compact roadmap phase cards + collapsible task groups - #27
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds live PR gate status visibility: a new ChangesPR Gate Status and Phase-Aware Dashboard
Sequence DiagramsequenceDiagram
participant User
participant TodoPage as Todo Page
participant ForgeClient as Forge Client
participant GateAPI as /api/gates
User->>TodoPage: Load page or trigger refresh
TodoPage->>TodoPage: Call loadData()
TodoPage->>ForgeClient: fetchTasks()
ForgeClient-->>TodoPage: Return tasks[]
TodoPage->>TodoPage: Group tasks by phase
TodoPage->>TodoPage: Derive phaseNames
loop For each task with PR
TodoPage->>ForgeClient: fetchPRGates(pr_number)
ForgeClient->>GateAPI: GET /api/gates?pr_number=N
GateAPI-->>ForgeClient: PRGateStatus
ForgeClient-->>TodoPage: HttpResult<PRGateStatus>
TodoPage->>TodoPage: Cache gateStatuses[pr_number]
end
TodoPage->>User: Render grouped phases with gate badges
Note over TodoPage: setInterval re-runs loadData() periodically
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/routes/roadmap/+page.svelteOops! Something went wrong! :( ESLint: 10.4.1 The requested operation requires ESLint to serialize configuration data, Please double-check your configuration for errors. If you still have problems, please stop by https://eslint.org/chat/help to chat src/routes/todo/+page.svelteOops! Something went wrong! :( ESLint: 10.4.1 The requested operation requires ESLint to serialize configuration data, Please double-check your configuration for errors. If you still have problems, please stop by https://eslint.org/chat/help to chat 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/todo/+page.svelte (1)
231-330:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrettier check is failing for this file.
CI already reports
Prettier --checkissues here; please run formatter to unblock lint gate.🤖 Prompt for 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. In `@src/routes/todo/`+page.svelte around lines 231 - 330, Prettier is failing on the Svelte template formatting in +page.svelte; run the project's formatter (e.g., npm run format or npx prettier --write) against src/routes/todo/+page.svelte to fix whitespace and JSX/Svelte attribute formatting so CI passes. Ensure the resulting file preserves existing logic around grouped, collapsed, togglePhase, statusClass, priorityClass, and gateStatuses (do not change identifiers), then re-run Prettier --check and commit the formatted file.Source: Pipeline failures
🧹 Nitpick comments (1)
src/routes/roadmap/+page.svelte (1)
158-165: 💤 Low valueConsider handling Space key for complete
role="button"semantics.Elements with
role="button"should respond to both Enter and Space for full keyboard accessibility. Currently only Enter triggers navigation.♿ Proposed enhancement
onkeydown={(e) => e.key === 'Enter' && goToPhase(phase.title)} + onkeydown={(e) => (e.key === 'Enter' || e.key === ' ') && (e.preventDefault(), goToPhase(phase.title))}Note:
preventDefault()on Space prevents page scrolling.🤖 Prompt for 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. In `@src/routes/roadmap/`+page.svelte around lines 158 - 165, The button-like div with role="button" only handles Enter, so add Space key handling to provide full keyboard semantics: update the onkeydown handler on that element (the handler calling goToPhase) to check for e.key === 'Enter' || e.key === ' ' || e.key === 'Spacebar' and call goToPhase(phase.title) for either, and for the Space branch call e.preventDefault() to stop page scrolling; keep using the same goToPhase(phase.title) call and ensure tabindex, role, and onclick remain unchanged.
🤖 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 `@src/routes/roadmap/`+page.svelte:
- Line 1: Prettier formatting errors in src/routes/roadmap/+page.svelte are
causing CI to fail; run the formatter (e.g., npx prettier --write
src/routes/roadmap/+page.svelte) to auto-fix style, then commit the updated file
— ensure the <script lang="ts"> block and surrounding template are formatted
according to project Prettier rules before pushing.
- Around line 230-246: Remove the onkeydown handler on the collapse button to
avoid double-invoking toggleCollapse (the browser already fires click on
Enter/Space), keep the existing onclick handler that calls e.stopPropagation()
and toggleCollapse(phase.id); ensure the button still has aria-label and the SVG
uses collapsed[phase.id] for rotation so collapse state remains driven by
toggleCollapse.
In `@src/routes/todo/`+page.svelte:
- Line 14: The cache key for gateStatuses (variable gateStatuses) is currently
only pr_number which causes collisions across repositories; update all places
where gateStatuses is set/read (the state var gateStatuses and any helpers that
build/consume its keys) to include the repository identifier (e.g., repo name or
repo_id combined with pr_number such as "<repo>:<pr_number>" or a {repo,pr}
tuple) and propagate that change to the upstream API/helper that supplies gate
data so keys are disambiguated end-to-end; ensure lookups, inserts, and
invalidations use the new composite key everywhere it’s referenced.
- Around line 53-57: The loadData function can leave updating true and keep the
poller running when supabase is falsy; update loadData to use a try/finally so
updating is always reset (set updating = true at start, then in finally set
updating = false), and ensure the early-return branch for missing supabase also
clears any polling and sets loading = false and updating = false; also add a
guard before starting the poll (or clearInterval of the poller) so polling never
runs when supabase is unavailable. Reference: loadData, updating, loading and
your polling/interval handler.
- Around line 59-61: The gate polling issues duplicate requests because the
current tasks array can contain multiple entries for the same PR and the code
calls loadGateStatus for every task; before calling loadGateStatus (the block
guarded by config.useLiveBridge), deduplicate tasks by the PR identifier (e.g.,
a prId or task.prNumber field) and only call loadGateStatus once per unique PR
(use a Set or map keyed by that PR id and then Promise.all over the unique
list), keeping the existing config.useLiveBridge check and using the same
loadGateStatus function.
---
Outside diff comments:
In `@src/routes/todo/`+page.svelte:
- Around line 231-330: Prettier is failing on the Svelte template formatting in
+page.svelte; run the project's formatter (e.g., npm run format or npx prettier
--write) against src/routes/todo/+page.svelte to fix whitespace and JSX/Svelte
attribute formatting so CI passes. Ensure the resulting file preserves existing
logic around grouped, collapsed, togglePhase, statusClass, priorityClass, and
gateStatuses (do not change identifiers), then re-run Prettier --check and
commit the formatted file.
---
Nitpick comments:
In `@src/routes/roadmap/`+page.svelte:
- Around line 158-165: The button-like div with role="button" only handles
Enter, so add Space key handling to provide full keyboard semantics: update the
onkeydown handler on that element (the handler calling goToPhase) to check for
e.key === 'Enter' || e.key === ' ' || e.key === 'Spacebar' and call
goToPhase(phase.title) for either, and for the Space branch call
e.preventDefault() to stop page scrolling; keep using the same
goToPhase(phase.title) call and ensure tabindex, role, and onclick remain
unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e83b310d-e125-4121-a752-8398f99f027a
📒 Files selected for processing (3)
src/lib/api/forgeClient.tssrc/routes/roadmap/+page.sveltesrc/routes/todo/+page.svelte
Comment on lines
+230
to
+246
| <button | ||
| onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }} | ||
| onkeydown={(e) => { e.stopPropagation(); if (e.key === 'Enter') toggleCollapse(phase.id); }} | ||
| class="flex shrink-0 items-center justify-center rounded p-0.5 transition hover:bg-white/[0.05]" | ||
| aria-label="Toggle phase details" | ||
| > | ||
| <svg | ||
| class="h-3 w-3 text-text-muted transition {collapsed[phase.id] ? '-rotate-90' : ''}" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| stroke-width="2" | ||
| stroke="currentColor" | ||
| > | ||
| <path stroke-linecap="round" stroke-linejoin="round" d="m19.5 8.25-7.5 7.5-7.5-7.5" /> | ||
| </svg> | ||
| </button> |
There was a problem hiding this comment.
Keyboard toggle fires twice, nullifying the collapse action.
When a user presses Enter on the collapse <button>, the browser fires both keydown and then synthesizes a click event. Both handlers call toggleCollapse(phase.id), so the state toggles and immediately toggles back—resulting in no visible change.
Remove the keydown handler since buttons natively trigger click on Enter/Space:
🐛 Proposed fix
<button
onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }}
- onkeydown={(e) => { e.stopPropagation(); if (e.key === 'Enter') toggleCollapse(phase.id); }}
class="flex shrink-0 items-center justify-center rounded p-0.5 transition hover:bg-white/[0.05]"
aria-label="Toggle phase details"
>📝 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
| <button | |
| onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }} | |
| onkeydown={(e) => { e.stopPropagation(); if (e.key === 'Enter') toggleCollapse(phase.id); }} | |
| class="flex shrink-0 items-center justify-center rounded p-0.5 transition hover:bg-white/[0.05]" | |
| aria-label="Toggle phase details" | |
| > | |
| <svg | |
| class="h-3 w-3 text-text-muted transition {collapsed[phase.id] ? '-rotate-90' : ''}" | |
| xmlns="http://www.w3.org/2000/svg" | |
| fill="none" | |
| viewBox="0 0 24 24" | |
| stroke-width="2" | |
| stroke="currentColor" | |
| > | |
| <path stroke-linecap="round" stroke-linejoin="round" d="m19.5 8.25-7.5 7.5-7.5-7.5" /> | |
| </svg> | |
| </button> | |
| <button | |
| onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }} | |
| class="flex shrink-0 items-center justify-center rounded p-0.5 transition hover:bg-white/[0.05]" | |
| aria-label="Toggle phase details" | |
| > | |
| <svg | |
| class="h-3 w-3 text-text-muted transition {collapsed[phase.id] ? '-rotate-90' : ''}" | |
| xmlns="http://www.w3.org/2000/svg" | |
| fill="none" | |
| viewBox="0 0 24 24" | |
| stroke-width="2" | |
| stroke="currentColor" | |
| > | |
| <path stroke-linecap="round" stroke-linejoin="round" d="m19.5 8.25-7.5 7.5-7.5-7.5" /> | |
| </svg> | |
| </button> |
🤖 Prompt for 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.
In `@src/routes/roadmap/`+page.svelte around lines 230 - 246, Remove the onkeydown
handler on the collapse button to avoid double-invoking toggleCollapse (the
browser already fires click on Enter/Space), keep the existing onclick handler
that calls e.stopPropagation() and toggleCollapse(phase.id); ensure the button
still has aria-label and the SVG uses collapsed[phase.id] for rotation so
collapse state remains driven by toggleCollapse.
| let loading = $state(true); | ||
| let updating = $state(false); | ||
| let collapsed = $state<Record<string, boolean>>({}); | ||
| let gateStatuses = $state<Record<number, PRGateCheck[] | null>>({}); |
There was a problem hiding this comment.
Gate status cache key is ambiguous across repositories.
Line 14/45/298 key gate data by pr_number only. If two repos both have PR #123, one status can overwrite the other and render incorrect gate badges.
Suggested direction
- let gateStatuses = $state<Record<number, PRGateCheck[] | null>>({});
+ let gateStatuses = $state<Record<string, PRGateCheck[] | null>>({});
+ function gateKey(task: Pick<Task, 'repo' | 'pr_number'>): string | null {
+ if (!task.pr_number) return null;
+ return `${task.repo}#${task.pr_number}`;
+ }
async function loadGateStatus(task: Task) {
- if (!task.pr_number || !config.useLiveBridge) return;
- const result = await fetchPRGates(task.pr_number);
+ const key = gateKey(task);
+ if (!key || !config.useLiveBridge) return;
+ const result = await fetchPRGates(task.pr_number /* include repo upstream */);
if (result.ok) {
- gateStatuses[task.pr_number] = result.data.gates;
+ gateStatuses[key] = result.data.gates;
}
}(Upstream API/helper also needs repo disambiguation, not just UI cache changes.)
Also applies to: 45-50, 298-300
🤖 Prompt for 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.
In `@src/routes/todo/`+page.svelte at line 14, The cache key for gateStatuses
(variable gateStatuses) is currently only pr_number which causes collisions
across repositories; update all places where gateStatuses is set/read (the state
var gateStatuses and any helpers that build/consume its keys) to include the
repository identifier (e.g., repo name or repo_id combined with pr_number such
as "<repo>:<pr_number>" or a {repo,pr} tuple) and propagate that change to the
upstream API/helper that supplies gate data so keys are disambiguated
end-to-end; ensure lookups, inserts, and invalidations use the new composite key
everywhere it’s referenced.
Comment on lines
+53
to
57
| async function loadData() { | ||
| if (!supabase) { | ||
| loading = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
updating can get stuck true when Supabase is unavailable.
Line 73 sets updating = true, but Line 54 returns early before Line 63 resets it. Also, polling still runs even when supabase is falsy.
Suggested guard/finally pattern
async function loadData() {
- if (!supabase) {
- loading = false;
- return;
- }
- tasks = await fetchTasks();
- if (config.useLiveBridge && tasks.length > 0) {
- await Promise.all(tasks.map(loadGateStatus));
- }
- loading = false;
- updating = false;
+ try {
+ if (!supabase) {
+ loading = false;
+ return;
+ }
+ tasks = await fetchTasks();
+ if (config.useLiveBridge && tasks.length > 0) {
+ await Promise.all(tasks.map(loadGateStatus));
+ }
+ loading = false;
+ } finally {
+ updating = false;
+ }
}
onMount(() => {
+ if (!supabase) return;
const phaseParam = $page.url.searchParams.get('phase');
if (phaseParam) phaseFilter = phaseParam;Also applies to: 63-64, 72-75
🤖 Prompt for 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.
In `@src/routes/todo/`+page.svelte around lines 53 - 57, The loadData function can
leave updating true and keep the poller running when supabase is falsy; update
loadData to use a try/finally so updating is always reset (set updating = true
at start, then in finally set updating = false), and ensure the early-return
branch for missing supabase also clears any polling and sets loading = false and
updating = false; also add a guard before starting the poll (or clearInterval of
the poller) so polling never runs when supabase is unavailable. Reference:
loadData, updating, loading and your polling/interval handler.
Comment on lines
+59
to
+61
| if (config.useLiveBridge && tasks.length > 0) { | ||
| await Promise.all(tasks.map(loadGateStatus)); | ||
| } |
There was a problem hiding this comment.
Gate polling currently issues duplicate requests for repeated PRs.
Line 60 fetches gate status once per task; multiple tasks tied to the same PR trigger redundant calls every refresh cycle.
Low-impact dedupe fix
- if (config.useLiveBridge && tasks.length > 0) {
- await Promise.all(tasks.map(loadGateStatus));
- }
+ if (config.useLiveBridge && tasks.length > 0) {
+ const seen = new Set<string>();
+ const uniqueTasks = tasks.filter((t) => {
+ if (!t.pr_number) return false;
+ const key = `${t.repo}#${t.pr_number}`;
+ if (seen.has(key)) return false;
+ seen.add(key);
+ return true;
+ });
+ await Promise.all(uniqueTasks.map(loadGateStatus));
+ }📝 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 (config.useLiveBridge && tasks.length > 0) { | |
| await Promise.all(tasks.map(loadGateStatus)); | |
| } | |
| if (config.useLiveBridge && tasks.length > 0) { | |
| const seen = new Set<string>(); | |
| const uniqueTasks = tasks.filter((t) => { | |
| if (!t.pr_number) return false; | |
| const key = `${t.repo}#${t.pr_number}`; | |
| if (seen.has(key)) return false; | |
| seen.add(key); | |
| return true; | |
| }); | |
| await Promise.all(uniqueTasks.map(loadGateStatus)); | |
| } |
🤖 Prompt for 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.
In `@src/routes/todo/`+page.svelte around lines 59 - 61, The gate polling issues
duplicate requests because the current tasks array can contain multiple entries
for the same PR and the code calls loadGateStatus for every task; before calling
loadGateStatus (the block guarded by config.useLiveBridge), deduplicate tasks by
the PR identifier (e.g., a prId or task.prNumber field) and only call
loadGateStatus once per unique PR (use a Set or map keyed by that PR id and then
Promise.all over the unique list), keeping the existing config.useLiveBridge
check and using the same loadGateStatus function.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/todo/+page.svelte (2)
20-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize phase names once for both filtering and grouping.
Line 20 builds filter options from raw
t.phase, but Lines 34-35 remap missing phases to'(no phase)'. That makes the rendered group label and the filter values diverge, so no-phase tasks can end up with a blank option and cannot be filtered consistently.Suggested fix
+ function phaseName(task: Task): string { + return task.phase?.trim() || '(no phase)'; + } + - let phaseNames = $derived([...new Set(tasks.map((t) => t.phase))].sort()); + let phaseNames = $derived([...new Set(tasks.map((t) => phaseName(t)))].sort()); let filtered = $derived( tasks.filter((t) => { - if (phaseFilter !== 'all' && t.phase !== phaseFilter) return false; + if (phaseFilter !== 'all' && phaseName(t) !== phaseFilter) return false; if (statusFilter !== 'all' && t.status !== statusFilter) return false; if (priorityFilter !== 'all' && t.priority !== priorityFilter) return false; return true; }) ); let grouped = $derived.by(() => { const map: Record<string, Task[]> = {}; for (const t of filtered) { - const p = t.phase || '(no phase)'; + const p = phaseName(t); if (!map[p]) map[p] = []; map[p].push(t); }Also applies to: 33-35
🤖 Prompt for 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. In `@src/routes/todo/`+page.svelte around lines 20 - 25, The phase option generation and filtering use different phase values causing the "(no phase)" mismatch; normalize phases once and reuse that normalized value for both the derived phase list and filters. Add a small helper (e.g., normalizePhase) that maps empty/undefined/null phases to the canonical string "(no phase)", then use that helper when computing phaseNames (the derived set from tasks) and when filtering tasks in the filtered derived store (replace uses of t.phase and phaseFilter comparisons with normalizePhase(t.phase) and normalizePhase(phaseFilter) or compare against the canonical string); also ensure grouping/rendering uses the same normalizePhase output so labels and filter values match.
72-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent overlapping refresh cycles.
Line 72 starts a new poll on a fixed cadence even if the previous
loadData()is still running. That can double-firefetchTasks/gate requests and let slower, older responses win the last state write.Suggested fix
- const intervalId = setInterval(async () => { - updating = true; - await loadData(); - }, REFRESH_MS); + let refreshInFlight = false; + const intervalId = setInterval(async () => { + if (refreshInFlight) return; + refreshInFlight = true; + updating = true; + try { + await loadData(); + } finally { + refreshInFlight = false; + } + }, REFRESH_MS);🤖 Prompt for 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. In `@src/routes/todo/`+page.svelte around lines 72 - 75, The poll currently starts a new interval task regardless of whether the prior loadData() is still running, which can double-fire fetchTasks; modify the logic so the interval handler first checks a guard (e.g., the existing updating flag) and returns immediately if updating is true, OR replace setInterval with a self-rescheduling setTimeout loop that awaits loadData() before scheduling the next call; also ensure loadData() (and any inner fetchTasks calls) always sets updating = true at start and clearing updating = false in a finally block so the guard is reliable. Reference: intervalId, REFRESH_MS, loadData, updating, fetchTasks.
♻️ Duplicate comments (1)
src/routes/roadmap/+page.svelte (1)
232-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
keydownfor propagation only; letclickown the toggle.On a native
<button>, Enter already dispatchesclick. The currentonkeydowntoggles once and the follow-upclicktoggles again, so pressing Enter leaves the collapse state unchanged. Because the parent card also handles Enter onkeydown, you still need this handler here to stop propagation—just not to mutate state.🐛 Proposed fix
<button onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }} onkeydown={(e) => { - e.stopPropagation(); - if (e.key === 'Enter') toggleCollapse(phase.id); + if (e.key === 'Enter' || e.key === ' ') e.stopPropagation(); }} class="flex shrink-0 items-center justify-center rounded p-0.5 transition hover:bg-white/[0.05]" aria-label="Toggle phase details" >🤖 Prompt for 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. In `@src/routes/roadmap/`+page.svelte around lines 232 - 239, The onkeydown handler currently both stops propagation and calls toggleCollapse(phase.id) which causes Enter to trigger toggle twice (keydown + resulting click); change the onkeydown for the clickable element to only call e.stopPropagation() and remove the toggleCollapse call so the native Enter->click triggers the single toggle via the onclick handler (leave onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }} intact and keep onkeydown only for propagation control).
🤖 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.
Outside diff comments:
In `@src/routes/todo/`+page.svelte:
- Around line 20-25: The phase option generation and filtering use different
phase values causing the "(no phase)" mismatch; normalize phases once and reuse
that normalized value for both the derived phase list and filters. Add a small
helper (e.g., normalizePhase) that maps empty/undefined/null phases to the
canonical string "(no phase)", then use that helper when computing phaseNames
(the derived set from tasks) and when filtering tasks in the filtered derived
store (replace uses of t.phase and phaseFilter comparisons with
normalizePhase(t.phase) and normalizePhase(phaseFilter) or compare against the
canonical string); also ensure grouping/rendering uses the same normalizePhase
output so labels and filter values match.
- Around line 72-75: The poll currently starts a new interval task regardless of
whether the prior loadData() is still running, which can double-fire fetchTasks;
modify the logic so the interval handler first checks a guard (e.g., the
existing updating flag) and returns immediately if updating is true, OR replace
setInterval with a self-rescheduling setTimeout loop that awaits loadData()
before scheduling the next call; also ensure loadData() (and any inner
fetchTasks calls) always sets updating = true at start and clearing updating =
false in a finally block so the guard is reliable. Reference: intervalId,
REFRESH_MS, loadData, updating, fetchTasks.
---
Duplicate comments:
In `@src/routes/roadmap/`+page.svelte:
- Around line 232-239: The onkeydown handler currently both stops propagation
and calls toggleCollapse(phase.id) which causes Enter to trigger toggle twice
(keydown + resulting click); change the onkeydown for the clickable element to
only call e.stopPropagation() and remove the toggleCollapse call so the native
Enter->click triggers the single toggle via the onclick handler (leave
onclick={(e) => { e.stopPropagation(); toggleCollapse(phase.id); }} intact and
keep onkeydown only for propagation control).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec531a15-257d-4166-a91b-57dffdce41b1
📒 Files selected for processing (2)
src/routes/roadmap/+page.sveltesrc/routes/todo/+page.svelte
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.
Roadmap: reduced phase card density (
p-5→p-3,text-base→text-sm), timeline + progress on one line, collapsible per-card detailsTodo: grouped tasks by phase in collapsible sections, sticky-top filters
No logic changes, all existing functionality preserved
vitest: 220 passed,svelte-check: 0 errors, 0 warningsSummary by CodeRabbit
New Features
UI Improvements