Feature 3623 project group field issues#50
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds sessionStorage-backed persistence for dashboard workspace and project group selections, refactors the ProjectGroupPicker component to support infinite-scroll pagination with cached display names, updates the project groups API to return pagination metadata and support sorting, and ensures session cleanup on logout. ChangesDashboard Persistence & ProjectGroupPicker Pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. 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: 14
🤖 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 `@components/dashboard/WorkspaceItem.vue`:
- Line 2: The template call still passes an unused workspace argument to
getClasses; update the button template to call getClasses() with no arguments to
match the revised function signature (change ":class=\"getClasses(workspace)\""
to ":class=\"getClasses()\""); ensure any other template occurrences of
getClasses(...) (e.g., the one referencing workspace) are similarly updated so
the component's template and the getClasses method signature stay consistent.
In `@components/DatasetPicker.vue`:
- Around line 47-52: The watch callbacks and async refreshDatasets can race and
let older responses overwrite newer ones; modify refreshDatasets (and its
callers in the projectGroupId and searchText watchers) to use a request token or
incrementing requestId (captured by each async call) or an AbortController so
that only the latest invocation's result updates datasets.value; implement the
token in the module scope (e.g., currentRequestId) incremented before awaiting
tdeiClient.getDatasetsByProjectGroupAndName and compare after the await to skip
setting datasets if the token no longer matches.
- Around line 14-17: The placeholder <option> in DatasetPicker.vue is using an
undefined bound :selected and a string "null" value while the component sets
model.value = null; fix by binding the option value to the actual null primitive
(use :value="null") and make the selected binding reflect the v-model state
(e.g., :selected="model.value === null") or simply remove the manual selected
and rely on v-model by using :value="null"; update the placeholder option near
the top of the template and reference model.value to ensure it becomes selected
when model.value is null.
In `@components/ProjectGroupPicker.vue`:
- Around line 283-292: The branch logic in the component leaves searchText and
selectedGroupName unset when model.value exists but nameModel.value is empty;
add a branch handling "model.value && !nameModel.value" (adjacent to the
existing checks around applyCachedName) that finds the group in projectGroups by
id (use projectGroups.value.find(g => g.id === model.value)), and if found set
searchText.value, selectedGroupName.value and nameModel.value to that group's
name (or if not found fall back to the first group similarly) so display state
stays consistent; reference symbols: model, nameModel, applyCachedName,
projectGroups, searchText, selectedGroupName.
In `@components/RapidEditor.vue`:
- Around line 2-3: The template binds :class="classes" but RapidEditor.vue does
not define a classes property; add a classes definition (either as a data
property, prop, or computed) or replace the binding with a static class to avoid
undefined state. Locate the RapidEditor component and either add a prop named
classes (with default/fallback), return classes from data() or a computed() that
synthesizes the expected class string/array, or change the template to use a
literal class name; ensure the chosen symbol is named exactly "classes" so the
template binding resolves.
In `@components/ServicePicker.vue`:
- Around line 44-45: When services are refreshed (e.g. after projectGroupId or
serviceType changes), the current model may reference an id no longer in
services; update the existing logic in ServicePicker.vue to not only default
when model is empty but also validate the current model against services: check
if services.value contains an entry with id === model.value, and if it does not,
set model.value to services.value[0]?.id (or null/undefined when services is
empty). Apply this check wherever the current snippet runs (the code using
model.value and services.value) so the selection resets whenever the chosen id
is not present in the refreshed services list.
- Around line 35-42: Concurrent calls to refreshServices() can race and let an
older slow response overwrite newer results; introduce a monotonic request token
(e.g., let latestRequest = 0) and in refreshServices() capture a local req =
++latestRequest before awaiting tdeiUserClient.getMyServices(...), then only
assign services.value if req === latestRequest; reference the refreshServices
function, the watch(projectGroupId) and watch(serviceType) triggers,
services.value, and tdeiUserClient.getMyServices when implementing this guard
(also consider catching errors so failed requests don't block the token logic).
In `@pages/workspace/`[id]/export/tdei.vue:
- Line 150: There's a typo in the toast success message: replace the misspelled
"sucessfully" in the template string toast.info(`TDEI import job ${jobId}
created sucessfully.`) with "successfully" so the message reads "TDEI import job
${jobId} created successfully."; update the single occurrence of that template
string (the toast.info call referencing jobId) to fix the spelling.
- Line 142: The schema_version assignment currently sets 'v0.2' for OSW
workspaces; change the ternary in the schema_version expression (schema_version:
workspace.type === 'osw' ? 'v0.2' : 'v1.0') to use 'v0.3' for the OSW branch so
it reads schema_version: workspace.type === 'osw' ? 'v0.3' : 'v1.0', ensuring
OSW exports use the current TDEI v0.3 schema.
In `@services/pathways.ts`:
- Around line 45-53: The code is passing the promise returned by
e.getData(textWriter) into parseCsv, so map entries store unresolved promises;
change the Promise executor in filePromises (where e.getData is called) to await
the result of e.getData(textWriter) (or use .then) before calling parseCsv or
resolving: call const csv = await e.getData(textWriter) (or
e.getData(textWriter).then(...)) and then if (parseObjects) resolve([e.filename,
parseCsv(csv)]) else resolve([e.filename, csv]); apply the same fix to the
similar block around the parseObjects handling at lines 58-60.
- Around line 289-292: The branch that handles Array.isArray(options.headers) is
pushing the Authorization header incorrectly: instead of calling
options.headers.push('Authorization', tokenHeader) you must push a tuple like
options.headers.push(['Authorization', tokenHeader]) so the header array remains
a list of [key, value] pairs; update the block that currently pushes
['X-Workspace', this.editorContext.workspaceId] and the Authorization header
(referencing options.headers, this.editorContext.workspaceId, and tokenHeader)
to push both as tuple arrays.
- Around line 64-67: The call to xml.parse when creating oscDoc incorrectly
passes a second argument; remove the second parameter so xml.parse is invoked
with only the XML string (matching the parse signature in util/xml.ts), e.g.,
replace the xml.parse call that assigns oscDoc to pass only the '<osmChange...>'
string; ensure xml.parse and oscDoc are the only identifiers changed.
In `@services/rapid.ts`:
- Around line 92-95: The branch handling Array.isArray(options.headers) is
pushing the Authorization header with incorrect syntax; in the else-if that
checks Array.isArray(options.headers) (inside the Rapid client request builder),
change the second push so it adds a two-element array like the first push (i.e.,
push ['Authorization', tokenHeader]) instead of calling push with two arguments,
so both headers are appended as [name, value] tuples.
In `@util/gtfs.ts`:
- Around line 120-123: The ZIP finalization is not awaited: after awaiting
Promise.all(filePromises) you must await the zipWriter.close() Promise before
calling blobWriter.getData() to ensure the archive is fully written; update the
function to use await zipWriter.close() (keeping Promise.all(filePromises) and
then call await blobWriter.getData()) so getData() returns the complete ZIP.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8c61a0b-d39c-4843-ab7b-4d37bdf2c51c
📒 Files selected for processing (34)
components/AppIcon.vuecomponents/AppLogo.vuecomponents/AppSpinner.vuecomponents/DatasetPicker.vuecomponents/DatasetTypeRadio.vuecomponents/LoadingSpinner.vuecomponents/OswEditor.vuecomponents/PathwaysEditor.vuecomponents/ProjectGroupPicker.vuecomponents/RapidEditor.vuecomponents/ServicePicker.vuecomponents/SigninForm.vuecomponents/dashboard/DetailsTable.vuecomponents/dashboard/Map.vuecomponents/dashboard/WorkspaceItem.vuepages/dashboard.vuepages/index.vuepages/signin.vuepages/workspace/[id]/edit.vuepages/workspace/[id]/export/index.vuepages/workspace/[id]/export/tdei.vuepages/workspace/create/blank.vuepages/workspace/create/file.vuepages/workspace/create/index.vuepages/workspace/create/tdei.vueservices/export/tdei.tsservices/pathways.tsservices/rapid.tsservices/tdei.tsutil/csv.tsutil/geojson.tsutil/gtfs.tsutil/url.tsutil/xml.ts
Contributor
|
@cyrossignol I believe you reported/asked for these issues, can you review please? I also see this has the rebase changes in it. I still think we should have just left those in, but I guess for this PR, do the best review you can with them in there too? |
Refactor ProjectGroupPicker UI and behavior: rebuild dropdown markup/CSS, show result counts and a scroll-to-load hint, add debounced input and click-to-open behavior, change infinite-scroll trigger to 80% of list height, and make hasMore/totalCount reactive. Improve selection logic to prefer current model or first item and avoid unnecessary reloads when unfiltered results are present.
Persist selection across sessions: add localStorage getters/setters for selected project group and workspace, restore them on mount, and save updates via watchers; add a min-width for the picker in dashboard styles.
API/client changes: clear stored selection keys on logout and update TdeiUserClient.getMyProjectGroups to return { items, total }, accept a sort_by param, and parse X-Total-Count for total results (with fallback on parse errors). These changes support accurate pagination and the new picker UI.
- Init currentProjectGroup synchronously from sessionStorage (was ref(null), causing a race where the picker defaulted to page-1 first group before onMounted ran) - Store and restore the project group name alongside its ID so the picker input field shows the correct name for page-2+ groups (which aren't in the first API response) - Add nameModel (v-model:name) to ProjectGroupPicker and emit it on selection - Switch all dashboard storage from localStorage to sessionStorage so each browser tab maintains its own independent selection that survives reloads
Replace duplicated conditional logic in onKeydown and handleClickOutside with optional chaining and nullish coalescing. Compute name = pg?.name ?? selectedGroupName.value, assign it to searchText, and only update selectedGroupName when a matching project group exists. This makes the code more concise and preserves the previously selected group name instead of clearing searchText when no matching group is found.
Clear the pending timeout on ProjectGroupPicker unmount to avoid leaked timers. Switch saved project/group keys removal from localStorage to sessionStorage and also remove the project-group-name key to fully clear auth selection. Harden parsing of the X-Total-Count response header: treat invalid/NaN parses as null to avoid returning an incorrect numeric value.
Add applyCachedName() to ProjectGroupPicker and use it on mount/selection to display a cached project group name immediately while the groups API call completes, removing duplicated assignments. In pages/dashboard, wrap sessionStorage access with typeof window checks and try/catch blocks so reads/writes are no-ops in SSR or when storage access throws (silently fail). This prevents runtime errors in server-side rendering or environments with disabled/blocked storage.
Collaborator
|
No prob, I can just rebase the branch. |
cyrossignol
force-pushed
the
feature-3623-projectGroup-field-issues
branch
from
cd8a31a to
1d09094
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
components/DatasetPicker.vue (2)
47-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAsync dataset refresh can apply stale responses out of order.
Rapid changes in
projectGroupIdorsearchTextcan trigger overlapping requests; an older response can resolve later and overwrite newer results.Proposed fix
+let refreshSeq = 0 + async function refreshDatasets(projectGroupId: string, name: string) { - datasets.value = (await tdeiClient.getDatasetsByProjectGroupAndName(projectGroupId, name)) + const seq = ++refreshSeq + const result = await tdeiClient.getDatasetsByProjectGroupAndName(projectGroupId, name) + if (seq !== refreshSeq) return + + datasets.value = result .sort((a, b) => a.name.localeCompare(b.name))What is the recommended Vue 3 pattern to prevent stale async watcher results from overwriting newer state (e.g., request token / sequence guard / AbortController)?🤖 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 `@components/DatasetPicker.vue` around lines 47 - 52, The async refreshDatasets can apply stale responses when multiple overlapping calls resolve out of order; modify watchers (watch(projectGroupId, ...), watch(searchText, ...)) and refreshDatasets to use a request guard: add a locally scoped incrementing requestId (or an AbortController) that you increment/create before calling tdeiClient.getDatasetsByProjectGroupAndName, pass the signal or capture the id, and only set datasets.value when the returned result matches the latest requestId (or when the signal hasn't been aborted); update refreshDatasets to accept/observe the token/signal and drop results from older requests to prevent overwriting newer state.
14-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlaceholder option value does not match the model’s null state.
Line 16 uses
"null"(string) while Line 55 assignsmodel.value = null(primitive), so the placeholder won’t correctly reflect/reset selection state withv-model.Proposed fix
<option - :selected - value="null" + :value="null" disabled > Select a (matching) dataset... </option>In Vue 3, when using <select v-model="x"> with <option>, how are string "null" and null treated, and what is the recommended way to represent a null placeholder option value?Also applies to: 55-55
🤖 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 `@components/DatasetPicker.vue` around lines 14 - 17, The placeholder option in DatasetPicker.vue is using the string "null" which won't match the actual null assigned to model.value at the reset site; update the placeholder <option> to use a real null value binding (e.g., :value="null") so the <select v-model="model.value"> properly reflects and resets to the placeholder state, and keep the option disabled to remain a non-selectable placeholder.components/ServicePicker.vue (2)
44-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset selection when current model is not in the refreshed list.
Line 44 only defaults when
modelis falsy. AfterprojectGroupIdorserviceTypechanges,model.valuecan remain set to an ID that's no longer present in the newserviceslist, resulting in an invalid selection state.✅ Suggested fix to validate current selection
- if (!model.value && services.value.length > 0) { + const hasCurrent = services.value.some(s => s.id === model.value) + if (!hasCurrent && services.value.length > 0) { model.value = services.value[0].id }🤖 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 `@components/ServicePicker.vue` around lines 44 - 45, The current initialization only sets model.value when it is falsy, which leaves stale IDs when projectGroupId or serviceType updates; update the logic in ServicePicker.vue (where model and services refs are handled) to validate the existing selection by checking if services.value contains an item with id === model.value, and if not, reset model.value to services.value[0]?.id (or null/undefined if services is empty); ensure this check runs whenever services is refreshed (same place as the existing if (!model.value && services.value.length > 0) block).
35-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale async responses from overwriting newer service results.
Lines 35 and 36 can trigger overlapping
refreshServices()calls when bothprojectGroupIdandserviceTypechange. A slower earlier request can resolve last and replace newer state with stale services.🔒 Suggested fix with request token
+let refreshToken = 0 + async function refreshServices() { + const token = ++refreshToken - services.value = (await tdeiUserClient.getMyServices(props.projectGroupId, props.serviceType)) + const next = (await tdeiUserClient.getMyServices(props.projectGroupId, props.serviceType)) .sort((a, b) => a.name.localeCompare(b.name)) + if (token !== refreshToken) return + services.value = next if (!model.value && services.value.length > 0) { model.value = services.value[0].id } }🤖 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 `@components/ServicePicker.vue` around lines 35 - 36, When multiple watchers (projectGroupId and serviceType) can trigger overlapping async calls to refreshServices(), add a request-disambiguation token so stale responses cannot overwrite newer state: create a module-scoped incremental requestId (or use an AbortController) and have refreshServices() capture the current id at start, attach it to the outgoing async call, and only commit the returned services to component state if the captured id still matches the latestRequestId (or if the AbortSignal is not aborted). Update references to projectGroupId, serviceType, and refreshServices to use this token-checking/cancellation mechanism so only the most recent request updates the service list.services/pathways.ts (3)
289-292:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse a tuple when appending
Authorizationto header arrays.Line 291 currently pushes two separate array items, which breaks the key/value pair shape used on Line 290 and can drop/mangle the auth header.
🔧 Proposed fix
else if (Array.isArray(options.headers)) { options.headers.push(['X-Workspace', this.editorContext.workspaceId]) - options.headers.push('Authorization', tokenHeader) + options.headers.push(['Authorization', tokenHeader]) }🤖 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 `@services/pathways.ts` around lines 289 - 292, The code in services/pathways.ts incorrectly pushes the Authorization header as two separate items into options.headers (inside the branch that checks Array.isArray(options.headers)), breaking the expected [key, value] tuple shape; change the second push to add a single tuple like ['Authorization', tokenHeader] (matching the existing ['X-Workspace', this.editorContext.workspaceId] tuple) so headers remain key/value pairs when calling whatever consumes options.headers.
64-67:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the second argument from
xml.parsecall—the function accepts only one parameter.The
parsefunction inutil/xml.tsaccepts a singleinput: stringargument and internally handles XML parsing with theapplication/xmlMIME type. The call at line 66 incorrectly passes a second argument, which causes a TypeScript error.🔧 Proposed fix
const oscDoc = xml.parse( - '<osmChange version="0.6"><create /><modify /><delete /></osmChange>', - 'application/xml', + '<osmChange version="0.6"><create /><modify /><delete /></osmChange>', )Run the following script to verify the xml.parse signature:
#!/bin/bash # Description: Verify xml.parse declaration and call arity # Check the parse function signature rg -n 'export function parse' util/xml.ts -A 3 # Check all xml.parse calls rg -n 'xml\.parse\(' services/pathways.ts -C 2🤖 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 `@services/pathways.ts` around lines 64 - 67, The call to xml.parse in services/pathways.ts passes an extra second argument but the exported parse function in util/xml.ts accepts only a single input string; remove the second parameter so the call becomes xml.parse('<osmChange ...>') to match the parse signature and avoid the TypeScript arity error, updating the usage where oscDoc is assigned.
45-54:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAwait archive entry extraction before parsing CSV.
Line 46 stores a promise (
e.getData(textWriter)) incsv, and Line 49 passes that promise intoparseCsv. This makes parsed results incorrect and can leave the map values as unresolved promises instead of parsed rows/strings.🔧 Proposed fix
- filePromises.push(new Promise((resolve) => { - const csv = e.getData(textWriter) - - if (parseObjects) { - resolve([e.filename, parseCsv(csv)]) - } - else { - resolve([e.filename, csv]) - } - })) + filePromises.push( + e.getData(textWriter).then(async (csv) => { + const data = parseObjects ? await parseCsv(csv) : csv + return [e.filename, data] + }), + )Run the following script to verify getData() returns a Promise:
#!/bin/bash # Description: Verify all getData() calls in pathways.ts are properly awaited # Find all getData usage in the file rg -n 'getData\(' services/pathways.ts -C 3🤖 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 `@services/pathways.ts` around lines 45 - 54, The code is passing the unresolved Promise from e.getData(textWriter) into parseCsv, so csv must be awaited before parsing; change the Promise executor to an async function (or explicitly await e.getData) and await const csv = await e.getData(textWriter) inside the filePromises push, then call parseCsv(csv) (or resolve([e.filename, csv]) when parseObjects is false) so the map stores the actual string/parsed rows rather than a pending Promise; update the block around filePromises, e.getData, parseCsv, textWriter, parseObjects and e.filename accordingly.services/rapid.ts (1)
92-95:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix incorrect array push syntax for Authorization header.
Line 94 pushes the Authorization header incorrectly. It should push a two-element array
['Authorization', tokenHeader]to match the pattern on Line 93, not two separate arguments.🐛 Proposed fix
else if (Array.isArray(options.headers)) { options.headers.push(['X-Workspace', this.rapidContext.workspaceId]) - options.headers.push('Authorization', tokenHeader) + options.headers.push(['Authorization', tokenHeader]) }🤖 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 `@services/rapid.ts` around lines 92 - 95, The Array.push call in the else-if branch handling Array.isArray(options.headers) uses incorrect syntax for the Authorization header; update the push so it adds a two-element array like the X-Workspace header (i.e., push ['Authorization', tokenHeader] instead of passing two arguments), modifying the block that references options.headers, this.rapidContext.workspaceId, and tokenHeader to ensure both headers are pushed as ['key', 'value'] entries.util/gtfs.ts (1)
120-123:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAwait ZIP finalization before reading blob data.
At line 121,
zipWriter.close()is not awaited. The close operation returns a Promise that finalizes the ZIP archive, and callingblobWriter.getData()before it completes risks returning an incomplete archive.🐛 Suggested fix
await Promise.all(filePromises) - zipWriter.close() + await zipWriter.close() return await blobWriter.getData()Run the following script to confirm the ZipWriter.close() API contract:
#!/bin/bash # Description: Verify ZipWriter.close() returns a Promise in `@zip.js/zip.js` # Check package.json for the exact version jq -r '.dependencies["@zip.js/zip.js"] // .devDependencies["@zip.js/zip.js"]' package.json # Search for ZipWriter usage patterns in the codebase rg -n 'zipWriter\.close\(\)' --type ts -C 2🤖 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 `@util/gtfs.ts` around lines 120 - 123, The ZIP finalization is not awaited: ensure you await the ZipWriter.close() Promise before calling blobWriter.getData() so the archive is fully written; specifically, after awaiting Promise.all(filePromises) call await zipWriter.close() (referencing zipWriter.close()) and only then call blobWriter.getData() (referencing blobWriter.getData()) to return the completed blob.pages/workspace/[id]/export/tdei.vue (2)
142-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate schema_version from v0.2 to v0.3 for OSW workspaces.
The code currently uses
v0.2, but the TDEI API supportsv0.3as the current standard OSW schema version. A previous commit (61413bb) updated this to v0.3, but it appears to have been reverted. Update line 142 to usev0.3to align with current TDEI API requirements.📝 Proposed fix
- schema_version: workspace.type === 'osw' ? 'v0.2' : 'v1.0', + schema_version: workspace.type === 'osw' ? 'v0.3' : 'v1.0',Please search the web to confirm the current TDEI OSW schema version:
What is the current supported OSW schema version in the TDEI API documentation as of 2026?🤖 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 `@pages/workspace/`[id]/export/tdei.vue at line 142, The schema_version for OSW workspaces is still set to 'v0.2'; update the conditional in the export component so that when workspace.type === 'osw' it returns 'v0.3' instead of 'v0.2' (modify the expression setting schema_version in the export logic in pages/workspace/[id]/export/tdei.vue).
150-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in success message.
"sucessfully" should be "successfully".
📝 Proposed fix
- toast.info(`TDEI import job ${jobId} created sucessfully.`) + toast.info(`TDEI import job ${jobId} created successfully.`)🤖 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 `@pages/workspace/`[id]/export/tdei.vue at line 150, The success message in the toast call contains a typo; update the string in the toast.info invocation that references jobId to read "successfully" instead of "sucessfully" so the notification reads: TDEI import job ${jobId} created successfully.
🧹 Nitpick comments (1)
components/ServicePicker.vue (1)
33-33: ⚡ Quick winConsider adding a TypeScript type annotation for
services.The
servicesref is currently inferred asRef<never[]>from the empty array initializer. Adding an explicit type annotation would improve type safety and code clarity.📝 Suggested type annotation
-const services = ref([]) +const services = ref<Array<{ id: string; name: string }>>([])🤖 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 `@components/ServicePicker.vue` at line 33, The services ref is inferred as Ref<never[]> because it's initialized with an empty array; update the declaration of services to include an explicit type parameter for ref (e.g., ref<ServiceType[]>([])) so the reactive type is correct. Locate the services variable and the ref import, create or import the appropriate interface/type (e.g., ServiceType, ServiceItem, or the existing service model) and use it as the generic parameter to ref to ensure proper typing throughout the component.
🤖 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 `@components/ServicePicker.vue`:
- Around line 40-47: refreshServices currently calls
tdeiUserClient.getMyServices without error handling; wrap the await call in a
try/catch inside refreshServices, on success sort and assign to services.value
and set model.value to the first id if unset, and on failure set services.value
= [] and model.value = null (or leave unchanged if preferred) and surface the
error (e.g., emit an "error" event or set a local error reactive like
fetchError) so the UI can show feedback; ensure you reference refreshServices,
tdeiUserClient.getMyServices, services.value and model.value when making the
changes.
In `@pages/workspace/create/tdei.vue`:
- Around line 200-202: The assignments to projectGroupId.value and
tdeiRecordId.value assume nested properties exist and can throw if
record.project_group or record.tdei_dataset_id are undefined; update these
assignments (the lines that set projectGroupId.value and tdeiRecordId.value) to
use optional chaining and a safe fallback (e.g.,
record.project_group?.tdei_project_group_id ?? '' and record?.tdei_dataset_id ??
'') so missing API fields don't cause a runtime crash.
---
Duplicate comments:
In `@components/DatasetPicker.vue`:
- Around line 47-52: The async refreshDatasets can apply stale responses when
multiple overlapping calls resolve out of order; modify watchers
(watch(projectGroupId, ...), watch(searchText, ...)) and refreshDatasets to use
a request guard: add a locally scoped incrementing requestId (or an
AbortController) that you increment/create before calling
tdeiClient.getDatasetsByProjectGroupAndName, pass the signal or capture the id,
and only set datasets.value when the returned result matches the latest
requestId (or when the signal hasn't been aborted); update refreshDatasets to
accept/observe the token/signal and drop results from older requests to prevent
overwriting newer state.
- Around line 14-17: The placeholder option in DatasetPicker.vue is using the
string "null" which won't match the actual null assigned to model.value at the
reset site; update the placeholder <option> to use a real null value binding
(e.g., :value="null") so the <select v-model="model.value"> properly reflects
and resets to the placeholder state, and keep the option disabled to remain a
non-selectable placeholder.
In `@components/ServicePicker.vue`:
- Around line 44-45: The current initialization only sets model.value when it is
falsy, which leaves stale IDs when projectGroupId or serviceType updates; update
the logic in ServicePicker.vue (where model and services refs are handled) to
validate the existing selection by checking if services.value contains an item
with id === model.value, and if not, reset model.value to services.value[0]?.id
(or null/undefined if services is empty); ensure this check runs whenever
services is refreshed (same place as the existing if (!model.value &&
services.value.length > 0) block).
- Around line 35-36: When multiple watchers (projectGroupId and serviceType) can
trigger overlapping async calls to refreshServices(), add a
request-disambiguation token so stale responses cannot overwrite newer state:
create a module-scoped incremental requestId (or use an AbortController) and
have refreshServices() capture the current id at start, attach it to the
outgoing async call, and only commit the returned services to component state if
the captured id still matches the latestRequestId (or if the AbortSignal is not
aborted). Update references to projectGroupId, serviceType, and refreshServices
to use this token-checking/cancellation mechanism so only the most recent
request updates the service list.
In `@pages/workspace/`[id]/export/tdei.vue:
- Line 142: The schema_version for OSW workspaces is still set to 'v0.2'; update
the conditional in the export component so that when workspace.type === 'osw' it
returns 'v0.3' instead of 'v0.2' (modify the expression setting schema_version
in the export logic in pages/workspace/[id]/export/tdei.vue).
- Line 150: The success message in the toast call contains a typo; update the
string in the toast.info invocation that references jobId to read "successfully"
instead of "sucessfully" so the notification reads: TDEI import job ${jobId}
created successfully.
In `@services/pathways.ts`:
- Around line 289-292: The code in services/pathways.ts incorrectly pushes the
Authorization header as two separate items into options.headers (inside the
branch that checks Array.isArray(options.headers)), breaking the expected [key,
value] tuple shape; change the second push to add a single tuple like
['Authorization', tokenHeader] (matching the existing ['X-Workspace',
this.editorContext.workspaceId] tuple) so headers remain key/value pairs when
calling whatever consumes options.headers.
- Around line 64-67: The call to xml.parse in services/pathways.ts passes an
extra second argument but the exported parse function in util/xml.ts accepts
only a single input string; remove the second parameter so the call becomes
xml.parse('<osmChange ...>') to match the parse signature and avoid the
TypeScript arity error, updating the usage where oscDoc is assigned.
- Around line 45-54: The code is passing the unresolved Promise from
e.getData(textWriter) into parseCsv, so csv must be awaited before parsing;
change the Promise executor to an async function (or explicitly await e.getData)
and await const csv = await e.getData(textWriter) inside the filePromises push,
then call parseCsv(csv) (or resolve([e.filename, csv]) when parseObjects is
false) so the map stores the actual string/parsed rows rather than a pending
Promise; update the block around filePromises, e.getData, parseCsv, textWriter,
parseObjects and e.filename accordingly.
In `@services/rapid.ts`:
- Around line 92-95: The Array.push call in the else-if branch handling
Array.isArray(options.headers) uses incorrect syntax for the Authorization
header; update the push so it adds a two-element array like the X-Workspace
header (i.e., push ['Authorization', tokenHeader] instead of passing two
arguments), modifying the block that references options.headers,
this.rapidContext.workspaceId, and tokenHeader to ensure both headers are pushed
as ['key', 'value'] entries.
In `@util/gtfs.ts`:
- Around line 120-123: The ZIP finalization is not awaited: ensure you await the
ZipWriter.close() Promise before calling blobWriter.getData() so the archive is
fully written; specifically, after awaiting Promise.all(filePromises) call await
zipWriter.close() (referencing zipWriter.close()) and only then call
blobWriter.getData() (referencing blobWriter.getData()) to return the completed
blob.
---
Nitpick comments:
In `@components/ServicePicker.vue`:
- Line 33: The services ref is inferred as Ref<never[]> because it's initialized
with an empty array; update the declaration of services to include an explicit
type parameter for ref (e.g., ref<ServiceType[]>([])) so the reactive type is
correct. Locate the services variable and the ref import, create or import the
appropriate interface/type (e.g., ServiceType, ServiceItem, or the existing
service model) and use it as the generic parameter to ref to ensure proper
typing throughout the component.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 392be518-6155-4bde-a3bb-66d5b0257f41
📒 Files selected for processing (32)
components/AppIcon.vuecomponents/AppLogo.vuecomponents/AppSpinner.vuecomponents/DatasetPicker.vuecomponents/DatasetTypeRadio.vuecomponents/LoadingSpinner.vuecomponents/OswEditor.vuecomponents/PathwaysEditor.vuecomponents/ProjectGroupPicker.vuecomponents/RapidEditor.vuecomponents/ServicePicker.vuecomponents/SigninForm.vuecomponents/dashboard/DetailsTable.vuecomponents/dashboard/Map.vuecomponents/dashboard/WorkspaceItem.vuepages/index.vuepages/signin.vuepages/workspace/[id]/edit.vuepages/workspace/[id]/export/index.vuepages/workspace/[id]/export/tdei.vuepages/workspace/create/blank.vuepages/workspace/create/file.vuepages/workspace/create/index.vuepages/workspace/create/tdei.vueservices/export/tdei.tsservices/pathways.tsservices/rapid.tsutil/csv.tsutil/geojson.tsutil/gtfs.tsutil/url.tsutil/xml.ts
✅ Files skipped from review due to trivial changes (18)
- pages/signin.vue
- util/url.ts
- pages/index.vue
- util/xml.ts
- pages/workspace/create/index.vue
- components/AppLogo.vue
- components/LoadingSpinner.vue
- util/csv.ts
- components/dashboard/DetailsTable.vue
- components/AppSpinner.vue
- pages/workspace/create/blank.vue
- components/AppIcon.vue
- components/DatasetTypeRadio.vue
- components/PathwaysEditor.vue
- components/OswEditor.vue
- pages/workspace/[id]/edit.vue
- components/SigninForm.vue
- components/dashboard/Map.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- components/ProjectGroupPicker.vue
cyrossignol
force-pushed
the
feature-3623-projectGroup-field-issues
branch
from
f26c0d7 to
1d09094
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/ProjectGroupPicker.vue (1)
158-165: 💤 Low value
nameModelcan go stale on external model changes.When
modelis mutated by a parent to an id that is not currently inprojectGroups.value(e.g., programmatic switch, parent restoring from storage after picker mounted), the watcher'sfindreturnsundefinedandnameModel/searchTextare left pointing at the previously selected group. SincenameModelis what's persisted tosessionStoragebypages/dashboard.vue, this can also cache an out-of-sync name. Consider invalidatingnameModel(or kicking off a single-group lookup) when no match is found, even while the dropdown is open.🤖 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 `@components/ProjectGroupPicker.vue` around lines 158 - 165, The watcher on model currently only updates nameModel/searchText when a matching project group is found, causing nameModel to remain stale if the new id isn't in projectGroups; update the watch (the function that watches model) to handle the no-match case by clearing or invalidating nameModel.value (and also searchText.value and selectedGroupName.value) when pg is undefined, or alternatively trigger a one-off lookup to resolve the id; modify the existing watcher logic around projectGroups.value.find(...) so both branches (pg found and pg not found) maintain consistent state.
🤖 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 `@components/ProjectGroupPicker.vue`:
- Around line 115-116: The current assignment uses a strict null check and can
store undefined into totalCount; change the guard around the response from
tdeiUserClient.getMyProjectGroups so you only assign when total is a real number
(e.g., use a loose null check total != null or typeof total === 'number').
Update the block referencing total (the local variable total and the ref
totalCount) so totalCount.value is only set when total is not null/undefined.
- Around line 286-297: The unbounded while(hasMore.value) loop in the mount
logic (checking model.value and calling loadGroups()) can spin through every
page or race with a reset; change it to first try fetching the single group by
id (use an API or a getGroupById helper) and only if that is unavailable do a
bounded paginated search with a maxPages lookahead (e.g., 3–5 pages) and check
cancellation flags between iterations — specifically bail out if pendingReset
becomes true or if pageNo/hasMore have been mutated; keep references to
model.value, loadGroups(), projectGroups, hasMore, pendingReset, pageNo,
nameModel and selectedGroupName when applying the fix.
---
Nitpick comments:
In `@components/ProjectGroupPicker.vue`:
- Around line 158-165: The watcher on model currently only updates
nameModel/searchText when a matching project group is found, causing nameModel
to remain stale if the new id isn't in projectGroups; update the watch (the
function that watches model) to handle the no-match case by clearing or
invalidating nameModel.value (and also searchText.value and
selectedGroupName.value) when pg is undefined, or alternatively trigger a
one-off lookup to resolve the id; modify the existing watcher logic around
projectGroups.value.find(...) so both branches (pg found and pg not found)
maintain consistent state.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61770f77-4a44-46c4-aebc-25969982df0e
📒 Files selected for processing (2)
components/ProjectGroupPicker.vuepages/workspace/create/tdei.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- pages/workspace/create/tdei.vue
ProjectGroupPicker: when a model ID is present but the group name is not cached, paginate through groups until the matching group is found and populate searchText, selectedGroupName, and nameModel so the UI shows the correct name. tdei.vue: make assignment of projectGroupId and tdeiRecordId null-safe by using optional chaining and null coalescing to avoid errors when nested fields are missing.
cyrossignol
force-pushed
the
feature-3623-projectGroup-field-issues
branch
from
6c29fef to
e6efe84
Compare
cyrossignol
requested changes
May 13, 2026
Collaborator
cyrossignol
left a comment
cyrossignol
left a comment
There was a problem hiding this comment.
Thanks for the changes. The dropdown behaves quite smoothly now. I think the header messaging will resolve the concerns. Gradient and rounded border are a nice touch!
Contributor
Author
|
Addressed the comments. Please check the updated code @cyrossignol |
Add sessionStorage caching for selected project group (stores {id,name}) and wire ProjectGroupPicker to read/write the cached name so the UI can show a name immediately even when the group isn't on page 1. Remove the separate nameModel/v-model:name usage in dashboard and consolidate storage to the single JSON key. Change getMyProjectGroups to use an optional total (undefined instead of null) and update callers accordingly. Other updates: persist cached name on selection, simplify input open logic, switch ProjectGroupPicker styles to SCSS and theme variables, clear the removed storage key from auth sign-out, and add dataset validation/error display in workspace/create/tdei.vue when dataset metadata is incomplete.
Refactor dropdown closing logic by extracting repeated code into a closeDropdown() helper. Use this helper from the Escape key handler and the outside-click handler to reduce duplication. Add an @focusout handler (onFocusOut) on the root element to close the dropdown when focus moves outside the picker (checks relatedTarget), ensuring consistent behavior for focus-based dismissals.
cyrossignol
force-pushed
the
feature-3623-projectGroup-field-issues
branch
from
8c7b4d2 to
408cb8f
Compare
The focusout event handler covers this interaction.
cyrossignol
approved these changes
May 18, 2026
Collaborator
cyrossignol
left a comment
cyrossignol
left a comment
There was a problem hiding this comment.
Minor issue. I'll fix before merging.
Comment on lines
+288
to
+290
| const onFocusOut = (e: FocusEvent) => { | ||
| if (!pickerRef.value?.contains(e.relatedTarget as Node)) { | ||
| if (isOpen.value) closeDropdown() |
Collaborator
There was a problem hiding this comment.
The handleClickOutside focus detection mechanism is now redundant. The onFocusOut handler covers the click interaction as well.
cyrossignol
added a commit
that referenced
this pull request
May 19, 2026
This fixes a missed call of `getMyProjectGroups()` in #44 following rebase of #50 which changed the behavior of the method. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary This PR fixes a missed update in the TDEI workspace export page's data loading logic that was introduced during the project group refactor from PR `#50` and issue `#44`. ### Changes **pages/workspace/[id]/export/tdei.vue** Updated the `Promise.all` call that fetches initial data to properly handle the API response structure: - Modified the `getMyProjectGroups()` call to include explicit pagination parameters: `getMyProjectGroups(1, '', 10000)` - Updated the destructuring pattern to extract the `items` property from the response object: `{ items: myProjectGroups }` instead of receiving the list directly This aligns with the API contract where `getMyProjectGroups()` returns an object with structure `{ items: TdeiProjectGroup[], total?: number }` rather than directly returning an array. The fix ensures that `eligibleProjectGroups` is computed correctly from the proper array of project groups, maintaining the role-based export permission checks and default project-group selection logic. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/TaskarCenterAtUW/workspaces-frontend/pull/52?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/TaskarCenterAtUW/workspaces-frontend/pull/52?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
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
4 participants
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.
PR: Project Group Field Issues
Task/Bugs Covered:
https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3635
https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3624/
https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3623/
Changes Implemented
components/ProjectGroupPicker.vuepages/dashboard.vuesessionStorage-backed helpers using named constants for all storage keys (tdei-selected-project-group,tdei-selected-project-group-name,tdei-selected-workspace)sessionStorageoperations are wrapped intry/catchwith SSR guards (typeof window === 'undefined') to handle private browsing and restricted browser policieslastProjectGroupIdin-memory fallback is removedservices/tdei.tsTdeiAuthStore.clear()now removes all threesessionStoragepicker keys on logoutTesting Checklist
↑/↓arrow keysEnteron highlighted itemEscapekeysessionStoragepicker keys cleared; fresh session on next loginScreenshots
Changes Overview
This PR addresses issues with the Project Group picker and session persistence across four files:
components/ProjectGroupPicker.vue
Refactored dropdown UI and state management:
defineModel('name', { default: '' })watch(searchText)@scrolllistener on dedicated list wrapper with revised thresholdpages/dashboard.vue
Added sessionStorage persistence for selections:
v-model:namebinding in addition to id bindingtdei-selected-project-group,tdei-selected-project-group-name,tdei-selected-workspacetypeof window === 'undefined')currentProjectGroupandcurrentProjectGroupNameinitialized synchronously at setup time from sessionStoragelastWorkspaceIdfrom sessionStorage instead of in-memory variables.project-group-pickerstyling with minimum widthservices/tdei.ts
Updated API client for project group functionality:
TdeiAuthStore.clear()now removes all three sessionStorage picker keys on logout (in addition to existing localStorage auth key removal)TdeiUserClient.getMyProjectGroups()signature updated to acceptsortByparameter with default value'name'Promise<TdeiProjectGroup[]>toPromise<{ items: TdeiProjectGroup[]; total: number | null }>X-Total-Countresponse header and returns total count; returns{ items: [], total: null }on parsing errorspages/workspace/create/tdei.vue
Defensive property assignments:
nextTick()assignments now use optional chaining and nullish coalescingworkspaceTitlefalls back to''projectGroupIdfalls back tonullwhenrecord.project_groupis missingtdeiRecordIdfalls back tonullwhenrecord.tdei_dataset_idis missing