Closed
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:
📝 WalkthroughWalkthroughMigrates multiple modules from SurrealQL to SPARQL, replaces DB-side literal parsing with a local Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
❌ Deploy Preview for fluxsocial-dev failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/api/src/conversation/index.ts (1)
146-186:⚠️ Potential issue | 🔴 CriticalUnreachable code: sorting logic will never execute.
The
return await Promise.all(...)at line 146 exits the function, making line 186 (return subgroups.sort(...)) unreachable. Additionally,subgroupsis not defined in this scope.The intended sorting by
starttimestamp needs to be applied to the Promise.all result before returning.🐛 Proposed fix
- return await Promise.all( + const subgroups = await Promise.all( Array.from(subgroupMap.values()).map(async (subgroup: any) => { // ... inner mapping code ... }), ); // Sort by actual content start time, not link creation time return subgroups.sort((a, b) => a.start - b.start);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation/index.ts` around lines 146 - 186, The Promise.all(...) is returned immediately, making the later sorting code unreachable and `subgroups` undefined; instead assign the Promise.all result to a variable (e.g., const subgroups = await Promise.all(...)), then sort that array by the start property (subgroups.sort((a,b) => a.start - b.start)) and return the sorted array; update references around subgroupMap, the mapping async function, and the final return so the timestampQuery / this.perspective.querySurrealDB logic stays the same but the sorted subgroups are returned.packages/api/src/conversation-subgroup/index.ts (1)
57-92:⚠️ Potential issue | 🔴 CriticalSELECT clause mismatch:
?topicNameRawwon't be returned.Line 60 selects
?topicNamebut line 65 binds?topicNameRaw, and line 78 accessesbinding.topicNameRaw?.value. SPARQL only returns variables explicitly listed in the SELECT clause, so topic names will always be empty.Compare with
topicsWithRelevance()at line 173 which correctly selects?topicNameRaw.🐛 Proposed fix
const sparqlQuery = ` PREFIX ad4m: <ad4m://ontology/> - SELECT ?topicBase ?topicName WHERE { + SELECT ?topicBase ?topicNameRaw WHERE { ?tagLink a ad4m:Link ; ad4m:predicate "flux://has_tag" ; ad4m:source ?semRel ; ad4m:target ?topicBase .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation-subgroup/index.ts` around lines 57 - 92, The SPARQL SELECT in the sparqlQuery inside the method that builds uniqueTopics currently lists ?topicName but the code later reads binding.topicNameRaw; update the SELECT to include ?topicNameRaw (matching topicsWithRelevance’s query) so binding.topicNameRaw?.value is populated, or alternatively change the binding reads to topicName if you intentionally want ?topicName—ensure sparqlQuery, uniqueTopics population code, and parseLit usage all reference the same variable name (?topicNameRaw) consistently.packages/api/src/channel/index.ts (1)
73-123:⚠️ Potential issue | 🟠 MajorIncomplete migration:
allItems()still uses SurrealQL.The
unprocessedItems()andtotalItemCount()methods have been migrated to SPARQL, butallItems()still uses SurrealQL withfn::parse_literal(...). This creates an inconsistency where half the methods won't work after the SurrealDB→Oxigraph migration is complete on the AD4M side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/channel/index.ts` around lines 73 - 123, allItems() is still issuing a SurrealQL query and using fn::parse_literal, so migrate it to SPARQL like the other methods: replace the SurrealQL string and the call to this.perspective.querySurrealDB(...) with a SPARQL query executed via the same helper used by unprocessedItems()/totalItemCount() (e.g., this.perspective.querySparql or the project's SPARQL helper), select bindings for id, author, timestamp, entry type and the body/title/name literals, remove fn::parse_literal usage, and adapt the mapping logic to read values from SPARQL result bindings (converting xsd:dateTime / literal values to strings and mapping 'flux://has_message'|'flux://has_post'|'flux://has_task' to 'Message'|'Post'|'Task') so the returned SynergyItem objects and icon lookup (icons[type]) remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 55: Run the project's formatter to restore the missing trailing newline
in package.json by executing the existing format script (yarn format or npm run
format) which will apply Prettier rules; after formatting, verify package.json
ends with a newline and commit the change (ensure the "format" script in
package.json is invoked).
---
Outside diff comments:
In `@packages/api/src/channel/index.ts`:
- Around line 73-123: allItems() is still issuing a SurrealQL query and using
fn::parse_literal, so migrate it to SPARQL like the other methods: replace the
SurrealQL string and the call to this.perspective.querySurrealDB(...) with a
SPARQL query executed via the same helper used by
unprocessedItems()/totalItemCount() (e.g., this.perspective.querySparql or the
project's SPARQL helper), select bindings for id, author, timestamp, entry type
and the body/title/name literals, remove fn::parse_literal usage, and adapt the
mapping logic to read values from SPARQL result bindings (converting
xsd:dateTime / literal values to strings and mapping
'flux://has_message'|'flux://has_post'|'flux://has_task' to
'Message'|'Post'|'Task') so the returned SynergyItem objects and icon lookup
(icons[type]) remain correct.
In `@packages/api/src/conversation-subgroup/index.ts`:
- Around line 57-92: The SPARQL SELECT in the sparqlQuery inside the method that
builds uniqueTopics currently lists ?topicName but the code later reads
binding.topicNameRaw; update the SELECT to include ?topicNameRaw (matching
topicsWithRelevance’s query) so binding.topicNameRaw?.value is populated, or
alternatively change the binding reads to topicName if you intentionally want
?topicName—ensure sparqlQuery, uniqueTopics population code, and parseLit usage
all reference the same variable name (?topicNameRaw) consistently.
In `@packages/api/src/conversation/index.ts`:
- Around line 146-186: The Promise.all(...) is returned immediately, making the
later sorting code unreachable and `subgroups` undefined; instead assign the
Promise.all result to a variable (e.g., const subgroups = await
Promise.all(...)), then sort that array by the start property
(subgroups.sort((a,b) => a.start - b.start)) and return the sorted array; update
references around subgroupMap, the mapping async function, and the final return
so the timestampQuery / this.perspective.querySurrealDB logic stays the same but
the sorted subgroups are returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2989c29-5d13-47ba-920d-5e2feedb89cf
📒 Files selected for processing (4)
package.jsonpackages/api/src/channel/index.tspackages/api/src/conversation-subgroup/index.tspackages/api/src/conversation/index.ts
HexaField
force-pushed
the
feat/sparql-migration
branch
from
607c0d3 to
584dea4
Compare
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 (3)
packages/api/src/conversation-subgroup/index.ts (1)
33-43:⚠️ Potential issue | 🟠 MajorCount distinct subgroup items in
stats().
totalItems = itemsResult.lengthwill overcount when duplicateSUBGROUP_ITEMlinks exist.views/synergy-demo-view/src/components/TimelineBlock/TimelineBlock.tsxalready treats duplicate item IDs as real data to clean up, so this should count unique item IDs instead of raw rows.🐛 Proposed fix
const itemsQuery = ` PREFIX ad4m: <ad4m://ontology/> - SELECT ?item WHERE { + SELECT (COUNT(DISTINCT ?item) AS ?count) WHERE { ?link1 a ad4m:Link ; ad4m:source "${this.id}" ; ad4m:predicate "${SUBGROUP_ITEM}" ; ad4m:target ?item . ?link2 a ad4m:Link ; ad4m:source ?item ; ad4m:predicate "flux://entry_type" ; ad4m:target ?type . FILTER(?type IN ("flux://has_message", "flux://has_post", "flux://has_task")) } `; const itemsResult = await this.perspective.querySurrealDB(itemsQuery); - const totalItems = itemsResult?.length || 0; + const totalItems = parseInt(itemsResult?.[0]?.count?.value ?? '0', 10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation-subgroup/index.ts` around lines 33 - 43, In stats(), itemsQuery can return duplicate ?item rows so totalItems = itemsResult?.length overcounts; change the logic after calling this.perspective.querySurrealDB(itemsQuery) to build a set of unique item IDs from itemsResult (extract the ?item value from each row) and assign totalItems to the size of that set; update the calculation around the itemsResult and totalItems variables in the stats() method of the ConversationSubgroup class so duplicates are deduplicated before counting.packages/api/src/conversation/index.ts (1)
146-186:⚠️ Potential issue | 🔴 CriticalThis returns before
subgroupsexists.
return await Promise.all(...)makes the laterreturn subgroups.sort(...)unreachable, andsubgroupsis never declared. That drops the intended start-time sort and should fail TypeScript checks.🐛 Proposed fix
- return await Promise.all( + const subgroups = await Promise.all( Array.from(subgroupMap.values()).map(async (subgroup: any) => { // SPARQL migration - get creation timestamps from channel→item links, not grouping timestamps from subgroup→item links const timestampQuery = ` PREFIX ad4m: <ad4m://ontology/> SELECT ?transcriptStart ?channelTs WHERE { @@ return { id: subgroup.id, name: subgroup.name || '', summary: subgroup.summary || '', start, end, }; }), ); // Sort by actual content start time, not link creation time return subgroups.sort((a, b) => a.start - b.start);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation/index.ts` around lines 146 - 186, The code currently does "return await Promise.all(Array.from(subgroupMap.values()).map(...))" which makes the later "return subgroups.sort(...)" unreachable and leaves "subgroups" undefined; change this by assigning the Promise.all result to a variable (e.g., const subgroups = await Promise.all(...)) and then perform "return subgroups.sort((a,b) => a.start - b.start)"; update any type annotations if needed and keep the mapping logic inside the Promise.all call as-is, referencing subgroupMap and subgroup.id/name/summary as before.packages/api/src/channel/index.ts (1)
128-202:⚠️ Potential issue | 🟠 MajorFinish migrating
Channel.allItems()in this file.
unprocessedItems()andtotalItemCount()are now SPARQL, butallItems()above still sends a SurrealQLSELECT ... FROM linkquery throughquerySurrealDB(). Once that endpoint is backed by the SPARQL executor only,Channel.allItems()will still fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/channel/index.ts` around lines 128 - 202, Channel.allItems() still sends a SurrealQL "SELECT ... FROM link" query and must be migrated to SPARQL like unprocessedItems() and totalItemCount(); replace the SurrealQL query in Channel.allItems() with a SPARQL query (use this.id for ad4m:source and the same filters used in unprocessedItems()/totalItemCount(): ad4m://has_child, flux://entry_type IN ("flux://has_message","flux://has_post","flux://has_task"), exclude subgroup items via SUBGROUP_ITEM and flux://conversation_subgroup, OPTIONAL flux://body, flux://title, flux://name), call this.perspective.querySurrealDB(sparqlQuery) to get bindings, deduplicate by binding.id?.value, map bindings to the existing return shape using parseLit for body/title/name, set type based on flux://... values and use icons[type] fallback, preserve timestamp conversion and the try/catch error handling as in other methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/conversation-subgroup/index.ts`:
- Around line 117-136: The Map-based deduplication (itemMap) in
conversation-subgroup index.ts hides duplicate IDs from callers (e.g.,
itemsData() -> TimelineBlock.tsx) that expect duplicates to detect and call
removeDuplicateItems(); instead, stop swallowing duplicates: preserve the
original binding occurrences and expose them for the cleanup path by either (a)
removing the early dedupe here and returning the full list (sparqlResult
processed into an array) so itemsData()/TimelineBlock can compute duplicates, or
(b) collect both deduplicated items and a duplicates array (track seen ids while
iterating and push subsequent bindings into a duplicates list) and return both
structures so callers can run removeDuplicateItems(); refer to symbols itemMap,
sparqlResult, transcriptStart/channelTs/fallbackTs extraction, itemsData(), and
removeDuplicateItems() when making the change.
- Around line 99-112: The SPARQL orders by the link timestamp (?timestamp) but
itemsData() maps each row to item.timestamp = transcriptStart || channelTs ||
timestamp, which can break chronology when transcriptStart or channelTs are
present; update itemsData() (the mapping/return logic) to sort the final items
array by the resolved item.timestamp (coalescing transcriptStart, channelTs,
timestamp into a numeric/time value) before returning so the UI receives items
in true chronological order; ensure you parse the values to comparable
numbers/Date and reference transcriptStart, channelTs, timestamp and the
itemsData() return array when implementing the sort.
- Around line 58-79: The SPARQL selects ?topicName but the mapper reads
binding.topicNameRaw, causing empty names; in the code that builds uniqueTopics
(in conversation-subgroup/index.ts, around the loop using uniqueTopics Map and
this.perspective.querySurrealDB) replace the reference to binding.topicNameRaw
with binding.topicName (i.e., call parseLit(binding.topicName?.value)) so the
selected ?topicName value is actually used; also verify the SELECT variable name
and the parseLit call both match.
---
Outside diff comments:
In `@packages/api/src/channel/index.ts`:
- Around line 128-202: Channel.allItems() still sends a SurrealQL "SELECT ...
FROM link" query and must be migrated to SPARQL like unprocessedItems() and
totalItemCount(); replace the SurrealQL query in Channel.allItems() with a
SPARQL query (use this.id for ad4m:source and the same filters used in
unprocessedItems()/totalItemCount(): ad4m://has_child, flux://entry_type IN
("flux://has_message","flux://has_post","flux://has_task"), exclude subgroup
items via SUBGROUP_ITEM and flux://conversation_subgroup, OPTIONAL flux://body,
flux://title, flux://name), call this.perspective.querySurrealDB(sparqlQuery) to
get bindings, deduplicate by binding.id?.value, map bindings to the existing
return shape using parseLit for body/title/name, set type based on flux://...
values and use icons[type] fallback, preserve timestamp conversion and the
try/catch error handling as in other methods.
In `@packages/api/src/conversation-subgroup/index.ts`:
- Around line 33-43: In stats(), itemsQuery can return duplicate ?item rows so
totalItems = itemsResult?.length overcounts; change the logic after calling
this.perspective.querySurrealDB(itemsQuery) to build a set of unique item IDs
from itemsResult (extract the ?item value from each row) and assign totalItems
to the size of that set; update the calculation around the itemsResult and
totalItems variables in the stats() method of the ConversationSubgroup class so
duplicates are deduplicated before counting.
In `@packages/api/src/conversation/index.ts`:
- Around line 146-186: The code currently does "return await
Promise.all(Array.from(subgroupMap.values()).map(...))" which makes the later
"return subgroups.sort(...)" unreachable and leaves "subgroups" undefined;
change this by assigning the Promise.all result to a variable (e.g., const
subgroups = await Promise.all(...)) and then perform "return
subgroups.sort((a,b) => a.start - b.start)"; update any type annotations if
needed and keep the mapping logic inside the Promise.all call as-is, referencing
subgroupMap and subgroup.id/name/summary as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a924d66-d911-46c8-b1ac-2039f8bf7007
📒 Files selected for processing (4)
package.jsonpackages/api/src/channel/index.tspackages/api/src/conversation-subgroup/index.tspackages/api/src/conversation/index.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
- Fix SPARQL SELECT to project ?topicNameRaw (was ?topicName, causing empty topic names) - Sort itemsData() by effective timestamp in JS (SPARQL ORDER BY uses link timestamp, not coalesced) - Preserve duplicate IDs in itemsData() for view-layer cleanup detection - Restore trailing newline in package.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/api/src/conversation-subgroup/index.ts (1)
3-7: Consider extractingparseLitto a shared utility.This helper is duplicated verbatim in
conversation/index.ts,channel/index.ts, and here. Extracting it to@coasys/flux-utilsor a local shared module would reduce duplication and centralize any future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation-subgroup/index.ts` around lines 3 - 7, The parseLit helper is duplicated; extract it into a shared module (e.g., export function parseLit(val: string | undefined): string) in a utilities package or local shared file, keep the exact behavior (return '' for falsy, try Literal.fromUrl(val).get() and fall back to val on error) and re-export; then replace the local parseLit implementations in conversation-subgroup/index.ts, conversation/index.ts, and channel/index.ts with an import from that shared module and remove the duplicate functions, ensuring any required import of Literal (used inside parseLit) is moved into the new utility so callers only import parseLit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/api/src/conversation-subgroup/index.ts`:
- Around line 3-7: The parseLit helper is duplicated; extract it into a shared
module (e.g., export function parseLit(val: string | undefined): string) in a
utilities package or local shared file, keep the exact behavior (return '' for
falsy, try Literal.fromUrl(val).get() and fall back to val on error) and
re-export; then replace the local parseLit implementations in
conversation-subgroup/index.ts, conversation/index.ts, and channel/index.ts with
an import from that shared module and remove the duplicate functions, ensuring
any required import of Literal (used inside parseLit) is moved into the new
utility so callers only import parseLit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a437e55-677d-4e47-83d3-8bfa3a24536c
📒 Files selected for processing (1)
packages/api/src/conversation-subgroup/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
47-54:⚠️ Potential issue | 🔴 CriticalLocal filesystem paths will break builds for other developers and CI.
The resolutions for
@coasys/ad4mand@coasys/ad4m-connectare pointing to absolute paths on a local machine (/tmp/sparql-demo/ad4m/...). These paths:
- Won't exist on other developers' machines or CI runners
- Reference
/tmp, which is ephemeral and cleared on reboot- Appear to be leftover from local testing against the companion AD4M PR
Before merging, revert these to a published npm version (or a pre-release version from the companion PR once published).
🔧 Proposed fix — revert to npm versions
"resolutions": { - "@coasys/ad4m": "link:/tmp/sparql-demo/ad4m/core", - "@coasys/ad4m-connect": "link:/tmp/sparql-demo/ad4m/connect", + "@coasys/ad4m": "0.12.0", + "@coasys/ad4m-connect": "0.12.0", "@coasys/ad4m-vue-hooks": "0.12.0",Or, if the companion PR (ad4m#760) publishes a pre-release:
"resolutions": { - "@coasys/ad4m": "link:/tmp/sparql-demo/ad4m/core", - "@coasys/ad4m-connect": "link:/tmp/sparql-demo/ad4m/connect", + "@coasys/ad4m": "0.13.0-prerelease.1", + "@coasys/ad4m-connect": "0.13.0-prerelease.1", "@coasys/ad4m-vue-hooks": "0.12.0",Based on learnings: "dev and pre-release versions of coasys/ad4m and coasys/ad4m-connect packages may be published to npm before being flagged as stable releases" — consider publishing a pre-release from the companion PR and referencing that version here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 47 - 54, The package.json resolutions currently lock `@coasys/ad4m` and `@coasys/ad4m-connect` to local filesystem links (link:/tmp/sparql-demo/ad4m/core and link:/tmp/sparql-demo/ad4m/connect) which will break other developers and CI; update the "resolutions" entries for "@coasys/ad4m" and "@coasys/ad4m-connect" to point to published npm versions (or a pre-release version from the companion PR) instead of absolute local paths, and ensure the other resolution entries remain valid (e.g., "@coasys/ad4m-vue-hooks", "@coasys/ad4m-react-hooks", "@coasys/hooks-helpers", "@preact/preset-vite").
🤖 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/src/utils/routeUtils.ts`:
- Around line 19-20: Update the prefix handling so both legacy
"literal://string:" and current "literal:string:" are recognized and normalized:
when stripping (the branch using channelId.startsWith and slice) check for both
prefixes and remove whichever is present, and when restoring/re-adding the
literal prefix (the code that prepends "literal:string:") ensure you only add
the canonical "literal:string:" and not re-add if the ID already had either
prefix; reference the channelId variable and the two literal prefixes
("literal:string:" and "literal://string:") so both occurrences (strip and
re-add) are updated.
In `@packages/utils/src/linkHelpers.ts`:
- Around line 37-42: The mapLiteralLinks function only checks new
"literal:string:", "literal:number:", and "literal:json:" prefixes so legacy
targets like "literal://string:" are left undecoded; update the logic in
mapLiteralLinks to recognize legacy forms by normalizing link.data.target (e.g.,
replace "literal://" with "literal:") or by checking both forms
("literal:string:" OR "literal://string:", etc.) before calling Literal.fromUrl
and extracting .get() or .get().data for json, ensuring legacy values are
decoded the same way as the new format.
---
Outside diff comments:
In `@package.json`:
- Around line 47-54: The package.json resolutions currently lock `@coasys/ad4m`
and `@coasys/ad4m-connect` to local filesystem links
(link:/tmp/sparql-demo/ad4m/core and link:/tmp/sparql-demo/ad4m/connect) which
will break other developers and CI; update the "resolutions" entries for
"@coasys/ad4m" and "@coasys/ad4m-connect" to point to published npm versions (or
a pre-release version from the companion PR) instead of absolute local paths,
and ensure the other resolution entries remain valid (e.g.,
"@coasys/ad4m-vue-hooks", "@coasys/ad4m-react-hooks", "@coasys/hooks-helpers",
"@preact/preset-vite").
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbf3b095-fbe1-4c26-867a-c7c1205c14a6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
app/src/composables/useSignallingService.tsapp/src/utils/routeUtils.tsapp/src/views/main/community/channel/view/ViewView.vueapp/src/views/main/profile/ProfileView.vuepackage.jsonpackages/utils/src/getNeighbourhoodMeta.tspackages/utils/src/linkHelpers.tspackages/utils/src/prologHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- app/src/composables/useSignallingService.ts
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 (1)
packages/api/src/conversation/index.ts (1)
146-186:⚠️ Potential issue | 🔴 CriticalUnreachable code and missing sort logic.
Line 146 returns the
Promise.allresult directly, making lines 185-186 unreachable. Additionally,subgroupsis undefined at line 186, which would cause aReferenceErrorif this code were somehow reached.The intended sort by
starttimestamp is never applied to the returned array.🐛 Proposed fix to apply the sort correctly
- return await Promise.all( + const subgroups = await Promise.all( Array.from(subgroupMap.values()).map(async (subgroup: any) => { // ... nested timestamp query logic ... }), ); - // Sort by actual content start time, not link creation time - return subgroups.sort((a, b) => a.start - b.start); + // Sort by actual content start time, not link creation time + return subgroups.sort((a, b) => a.start - b.start);Full fix:
- return await Promise.all( + const subgroups = await Promise.all( Array.from(subgroupMap.values()).map(async (subgroup: any) => { // SPARQL migration - get creation timestamps from channel→item links, not grouping timestamps from subgroup→item links const timestampQuery = ` PREFIX ad4m: <ad4m://ontology/> SELECT ?transcriptStart ?channelTs WHERE { ?itemLink a ad4m:Link ; ad4m:source "${subgroup.id}" ; ad4m:predicate "flux://has_item" ; ad4m:target ?item . ?chLink a ad4m:Link ; ad4m:predicate "ad4m://has_child" ; ad4m:target ?item ; ad4m:source ?chSrc ; ad4m:timestamp ?channelTs . ?chTypeLink a ad4m:Link ; ad4m:source ?chSrc ; ad4m:predicate "flux://entry_type" ; ad4m:target "flux://has_channel" . OPTIONAL { ?tsLink a ad4m:Link ; ad4m:source ?item ; ad4m:predicate "flux://transcript_started_at" ; ad4m:target ?transcriptStart . } } ORDER BY ?channelTs `; const timestampResults = await this.perspective.querySparql(timestampQuery); // Filter out null/undefined timestamps and convert to numeric timestamps const timestamps = (timestampResults || []) .map((r: any) => { const ts = parseLit(r.transcriptStart?.value) || r.channelTs?.value; return ts; }) .filter((ts) => ts != null && ts !== '') .map((ts) => new Date(ts).getTime()) .filter((time) => !isNaN(time)); const start = timestamps.length > 0 ? timestamps[0] : 0; const end = timestamps.length > 0 ? timestamps[timestamps.length - 1] : 0; return { id: subgroup.id, name: subgroup.name || '', summary: subgroup.summary || '', start, end, }; }), ); // Sort by actual content start time, not link creation time return subgroups.sort((a, b) => a.start - b.start);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation/index.ts` around lines 146 - 186, The Promise.all result is returned early so the subsequent sort is never applied and the identifier subgroups is undefined; change the early return to assign the resolved array to a variable (e.g., const subgroups = await Promise.all(...map(async (subgroup) => { ... }))); then sort that array by start (subgroups.sort((a, b) => a.start - b.start)) and finally return the sorted subgroups. Update references to subgroupMap, querySparql, and parseLit remain the same inside the mapper; ensure start/end are numeric before sorting.
🧹 Nitpick comments (1)
packages/api/src/conversation/index.ts (1)
2-7: Consider extractingparseLitto a shared utility.This helper is duplicated identically in
channel/index.ts,conversation/index.ts, andconversation-subgroup/index.ts. For maintainability, consider extracting it to a shared module (e.g.,@coasys/flux-utilsor a localsparql-utils.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/conversation/index.ts` around lines 2 - 7, Extract the duplicated parseLit helper into a shared utility module (e.g., create sparql-utils.ts or add to `@coasys/flux-utils`) and replace the local implementations in conversation/index.ts, channel/index.ts, and conversation-subgroup/index.ts with an import from that module; specifically, move the function parseLit(val: string | undefined): string { ... } into the new module, export it (e.g., export function parseLit...), update each file to import { parseLit } from the shared module, and remove the duplicate function declarations so all three modules use the single shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/api/src/conversation/index.ts`:
- Around line 146-186: The Promise.all result is returned early so the
subsequent sort is never applied and the identifier subgroups is undefined;
change the early return to assign the resolved array to a variable (e.g., const
subgroups = await Promise.all(...map(async (subgroup) => { ... }))); then sort
that array by start (subgroups.sort((a, b) => a.start - b.start)) and finally
return the sorted subgroups. Update references to subgroupMap, querySparql, and
parseLit remain the same inside the mapper; ensure start/end are numeric before
sorting.
---
Nitpick comments:
In `@packages/api/src/conversation/index.ts`:
- Around line 2-7: Extract the duplicated parseLit helper into a shared utility
module (e.g., create sparql-utils.ts or add to `@coasys/flux-utils`) and replace
the local implementations in conversation/index.ts, channel/index.ts, and
conversation-subgroup/index.ts with an import from that module; specifically,
move the function parseLit(val: string | undefined): string { ... } into the new
module, export it (e.g., export function parseLit...), update each file to
import { parseLit } from the shared module, and remove the duplicate function
declarations so all three modules use the single shared implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc41705f-56cc-4290-b2b8-048dcabd21c0
📒 Files selected for processing (3)
packages/api/src/channel/index.tspackages/api/src/conversation-subgroup/index.tspackages/api/src/conversation/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/api/src/channel/index.ts
- packages/api/src/conversation-subgroup/index.ts
Contributor
Author
|
Closing in favour of a new PR with branch name matching the AD4M SPARQL branch ( |
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
Migrate all SurrealQL queries in Flux API packages to SPARQL, matching the AD4M executor's switch from SurrealDB to Oxigraph.
Companion PR: coasys/ad4m#760
Changes
packages/api/src/conversation/index.ts— Replace SurrealQL with SPARQL for conversation queriespackages/api/src/conversation-subgroup/index.ts— Replace SurrealQL with SPARQL for subgroup queries4 files changed, +222 / -152 lines.
Context
The AD4M executor now uses Oxigraph/SPARQL instead of SurrealDB. The GraphQL field
perspectiveQuerySurrealDbis preserved for backward compatibility but routes to SPARQL. These Flux changes update the query strings from SurrealQL syntax to SPARQL syntax.Summary by CodeRabbit
depends on d9011e34-8219-4ba1-bdd1-7d104a25cd1f/91215135-9143-46eb-8f5d-b38504a40392#760