Message save location#2731
Open
burnettk wants to merge 50 commits into
Open
Conversation
# Conflicts: # spiffworkflow-backend/src/spiffworkflow_backend/routes/messages_controller.py
Contributor
|
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:
📝 WalkthroughWalkthroughBackend: message model responses include primary Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as MessageEditor
participant API as Backend API
participant DB as Database
User->>Frontend: Open message editor
activate Frontend
Frontend->>API: GET /process-groups/{id}
Frontend->>API: GET /message-models/{id}
activate API
API->>DB: Query process group
API->>DB: Query message models (include id, location)
DB-->>API: Process group + message models
API-->>Frontend: Return process group + message models
deactivate API
Frontend->>Frontend: Build shared options\nfindNearestAncestorLocation\nprefill form
User->>Frontend: Select shared message / edit
Frontend->>Frontend: Merge correlation properties\nset schema/id/location as needed
User->>Frontend: Submit update
Frontend->>API: PATCH /process-groups/{id} (sanitized messages)
activate API
API->>API: _strip_message_model_metadata(payload)
API->>DB: Lookup MessageModel by (identifier, location)
DB-->>API: existing message (optional)
API->>DB: db.session.merge(message_model) -> flush -> obtain id
API->>DB: delete & recreate correlation properties for message_id
DB-->>API: commit
API-->>Frontend: Updated process group
deactivate API
deactivate Frontend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
spiffworkflow-frontend/src/interfaces.ts (1)
245-247: Sync the fetched message-model type too.These new fields update the saved message definition shape, but
MessageModellater in this file still omitsid. That leaves the/message-modelsresponse under-typed even though this PR now depends on that field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spiffworkflow-frontend/src/interfaces.ts` around lines 245 - 247, The MessageModel type is missing the new fields added to MessageDefinition, causing the /message-models response to be under-typed; update the MessageModel interface to include the same optional properties (e.g., add id?: number and location?: string to the MessageModel declaration) so its shape matches MessageDefinition and the fetched response is correctly typed.spiffworkflow-frontend/src/components/messages/MessageEditor.tsx (1)
310-344: Consider adding cleanup to prevent state updates on unmounted components.The coordination pattern using closure variables works, but if the component unmounts before both API calls complete, the callbacks will attempt to update state on an unmounted component. While React handles this gracefully, adding an abort signal or mounted flag improves robustness.
♻️ Optional improvement with mounted flag
useEffect(() => { + let isMounted = true; const currentGroupLocation = unModifyProcessIdentifierForPathParam( modifiedProcessGroupIdentifier, ); // ... existing code ... const maybeFinalize = () => { - if (processGroupLoaded && messageModelsLoaded && processGroupResult) { + if (isMounted && processGroupLoaded && messageModelsLoaded && processGroupResult) { processResult(processGroupResult, messageModelsResult); } }; // ... API calls ... + + return () => { + isMounted = false; + }; }, [modifiedProcessGroupIdentifier, correlationProperties, messageId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spiffworkflow-frontend/src/components/messages/MessageEditor.tsx` around lines 310 - 344, The component makes two HttpService.makeCallToBackend calls and uses closure vars (processGroupResult, messageModelsResult, processGroupLoaded, messageModelsLoaded, maybeFinalize) to update state when both return, but there’s no cleanup to prevent updates after unmount; modify the surrounding useEffect to create a mounted flag (or an AbortController) and in each successCallback/failureCallback check the flag (or abort signal) before assigning to processGroupResult/messageModelsResult, setting the loaded flags, and calling maybeFinalize, and return a cleanup function that sets mounted=false (or calls abort()) so callbacks won’t update state on an unmounted component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@spiffworkflow-backend/src/spiffworkflow_backend/services/message_definition_service.py`:
- Around line 18-29: The code currently trusts a caller-supplied message_id and
assigns it to message_model.id which can overwrite an unrelated row; before
setting message_model.id (or before merge), query MessageModel by the numeric
message_id (e.g., MessageModel.query.get(message_id) or
filter_by(id=message_id)) and verify that if a record exists its identifier and
location match the current identifier and message_location; if they match, you
may reuse that id, if a record exists but identifier/location differ,
raise/abort (reject the payload) to avoid reusing someone else's PK, and if no
record exists allow using the supplied id; if caller did not supply an id fall
back to existing_model.id as before.
In `@spiffworkflow-frontend/src/components/messages/MessageHelper.tsx`:
- Around line 9-15: The filter for candidateLocations in MessageHelper.tsx fails
to treat the root (normalizedCandidate === '') as an ancestor; update the
predicate in the candidateLocations.filter (where normalizedCandidate is
derived) to explicitly accept an empty normalizedCandidate as matching any
descendant (e.g., add a check like normalizedCandidate === '' || ...), so that
normalizedCurrentLocation is considered a match when the candidate is root,
while keeping the existing equality and startsWith(`${normalizedCandidate}/`)
checks.
---
Nitpick comments:
In `@spiffworkflow-frontend/src/components/messages/MessageEditor.tsx`:
- Around line 310-344: The component makes two HttpService.makeCallToBackend
calls and uses closure vars (processGroupResult, messageModelsResult,
processGroupLoaded, messageModelsLoaded, maybeFinalize) to update state when
both return, but there’s no cleanup to prevent updates after unmount; modify the
surrounding useEffect to create a mounted flag (or an AbortController) and in
each successCallback/failureCallback check the flag (or abort signal) before
assigning to processGroupResult/messageModelsResult, setting the loaded flags,
and calling maybeFinalize, and return a cleanup function that sets mounted=false
(or calls abort()) so callbacks won’t update state on an unmounted component.
In `@spiffworkflow-frontend/src/interfaces.ts`:
- Around line 245-247: The MessageModel type is missing the new fields added to
MessageDefinition, causing the /message-models response to be under-typed;
update the MessageModel interface to include the same optional properties (e.g.,
add id?: number and location?: string to the MessageModel declaration) so its
shape matches MessageDefinition and the fetched response is correctly typed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4619d60-8b95-431e-b9e2-2a2f46e2819d
📒 Files selected for processing (10)
spiffworkflow-backend/src/spiffworkflow_backend/routes/messages_controller.pyspiffworkflow-backend/src/spiffworkflow_backend/routes/process_groups_controller.pyspiffworkflow-backend/src/spiffworkflow_backend/services/message_definition_service.pyspiffworkflow-backend/tests/spiffworkflow_backend/integration/test_messages.pyspiffworkflow-frontend/src/components/messages/MessageEditor.test.tsxspiffworkflow-frontend/src/components/messages/MessageEditor.tsxspiffworkflow-frontend/src/components/messages/MessageHelper.test.tsxspiffworkflow-frontend/src/components/messages/MessageHelper.tsxspiffworkflow-frontend/src/interfaces.tsspiffworkflow-frontend/src/locales/en_us/translation.json
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spiffworkflow-frontend/Dockerfile (1)
45-45: Pin the NGINX base image by digest for reproducibility and supply-chain traceability.Line [45] uses a mutable tag (
nginx:1.29.7-alpine). Pin this image to its immutable digest for better supply-chain safety and build reproducibility:Suggested Dockerfile adjustment
-FROM nginx:1.29.7-alpine +FROM nginx:1.29.7-alpine@sha256:<resolved_digest>To find the exact digest, run:
docker manifest inspect nginx:1.29.7-alpine | jq '.[0].digest'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spiffworkflow-frontend/Dockerfile` at line 45, Replace the mutable base image reference "FROM nginx:1.29.7-alpine" with its immutable digest form to ensure reproducible builds and better supply-chain traceability; obtain the exact digest by running `docker manifest inspect nginx:1.29.7-alpine | jq '.[0].digest'` (or the registry equivalent) and update the Dockerfile's FROM line to use "nginx@sha256:<digest>" instead of the tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spiffworkflow-frontend/Dockerfile`:
- Line 45: Replace the mutable base image reference "FROM nginx:1.29.7-alpine"
with its immutable digest form to ensure reproducible builds and better
supply-chain traceability; obtain the exact digest by running `docker manifest
inspect nginx:1.29.7-alpine | jq '.[0].digest'` (or the registry equivalent) and
update the Dockerfile's FROM line to use "nginx@sha256:<digest>" instead of the
tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0598cbc7-bc6d-487e-a4be-b9bf0cfeae1b
⛔ Files ignored due to path filters (1)
spiffworkflow-backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
spiffworkflow-backend/pyproject.tomlspiffworkflow-frontend/Dockerfile
✅ Files skipped from review due to trivial changes (1)
- spiffworkflow-backend/pyproject.toml
Contributor
Author
|
@copilot Verify each finding against the current code and only fix it if needed. In |
Agent-Logs-Url: https://github.com/sartography/spiff-arena/sessions/8af82614-cd72-4fe4-b910-7dff33cda721 Co-authored-by: burnettk <18027+burnettk@users.noreply.github.com>
Contributor
Fixed in 6f8eeb3. When |
Agent-Logs-Url: https://github.com/sartography/spiff-arena/sessions/c961d6bd-c5cb-48e5-96e0-a9508db4dac0 Co-authored-by: burnettk <18027+burnettk@users.noreply.github.com>
Copilot stopped work on behalf of
burnettk due to an error
March 27, 2026 03:25
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@spiffworkflow-backend/src/spiffworkflow_backend/services/message_definition_service.py`:
- Around line 18-33: The code currently treats any non-None
message_definition.get("id") that isn't a digit-only string as a candidate
primary key, allowing malformed IDs through; update the logic in
message_definition_service so that after extracting message_id from
message_definition you explicitly reject (raise ValueError) any non-None id that
is not an int and not a digit-only string before any DB lookup—i.e., if
isinstance(message_id, str) and message_id.isdigit(): convert to int; elif
message_id is None: continue; else: raise ValueError indicating the payload id
is invalid; keep the subsequent checks against MessageModel.query.filter_by(...)
and resolved_message_id assignment unchanged.
- Around line 25-33: The check that a supplied message_id belongs to the same
message must be made atomic with the save: when message_id is not None, SELECT
the MessageModel row for that id using a FOR UPDATE lock (e.g.,
MessageModel.query.filter_by(id=message_id).with_for_update().first()) inside
the same transaction so concurrent requests block; if the locked row exists and
identifier/location mismatch, raise ValueError; if it does not exist, proceed to
create/insert the new MessageModel (avoid db.session.merge() which can overwrite
a concurrent insert) — use db.session.add()/flush() and handle IntegrityError to
detect races and raise a conflict instead. Ensure the lock-and-save logic
replaces the separate validation (MessageModel lookup) and the later
db.session.merge() paths.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab58e95f-55f8-4c9e-9a90-710f2323d21d
⛔ Files ignored due to path filters (1)
spiffworkflow-backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
spiffworkflow-backend/src/spiffworkflow_backend/services/message_definition_service.pyspiffworkflow-frontend/src/components/messages/MessageEditor.test.tsxspiffworkflow-frontend/src/components/messages/MessageEditor.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- spiffworkflow-frontend/src/components/messages/MessageEditor.test.tsx
- spiffworkflow-frontend/src/components/messages/MessageEditor.tsx
Comment on lines
+25
to
+33
| if message_id is not None: | ||
| model_by_id = MessageModel.query.filter_by(id=message_id).first() | ||
| if model_by_id is not None and (model_by_id.identifier != identifier or model_by_id.location != message_location): | ||
| raise ValueError( | ||
| f"Supplied message_id {message_id} belongs to a different message" | ||
| f" ('{model_by_id.identifier}' at '{model_by_id.location}')." | ||
| f" Cannot reuse this id for '{identifier}' at '{message_location}'." | ||
| ) | ||
| resolved_message_id = message_id |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n spiffworkflow-backend/src/spiffworkflow_backend/services/message_definition_service.py | head -120Repository: sartography/spiff-arena
Length of output: 5792
🏁 Script executed:
# Check for transaction context and how this is called
rg -B 5 -A 5 "save_all_message_models" spiffworkflow-backend/src --type pyRepository: sartography/spiff-arena
Length of output: 4501
🏁 Script executed:
# Check the MessageModel definition for primary key setup
cat -n spiffworkflow-backend/src/spiffworkflow_backend/models/message_model.py | head -50Repository: sartography/spiff-arena
Length of output: 2233
🏁 Script executed:
# Check if there's any explicit transaction locking or isolation elsewhere
rg -B 3 -A 3 "with_for_update|FOR UPDATE" spiffworkflow-backend/src --type pyRepository: sartography/spiff-arena
Length of output: 49
🏁 Script executed:
# Verify the exact flow of how these methods interact
rg -B 8 "save_all_message_models\|delete_all_message_models" spiffworkflow-backend/src/spiffworkflow_backend/services/data_setup_service.py -A 2Repository: sartography/spiff-arena
Length of output: 49
🏁 Script executed:
# Check data_setup_service for the call sequence
cat -n spiffworkflow-backend/src/spiffworkflow_backend/services/data_setup_service.py | grep -A 10 -B 5 "MessageDefinitionService"Repository: sartography/spiff-arena
Length of output: 2725
🏁 Script executed:
# Search for any locking patterns in the codebase
rg "for_update" spiffworkflow-backend/src --type pyRepository: sartography/spiff-arena
Length of output: 49
Use pessimistic locking to make the message_id ownership check atomic with the save.
The validation at lines 25-33 and the db.session.merge() at line 102 are separate operations. Between these steps, a concurrent request could insert a new message using the same message_id but with a different identifier and location. The merge() operation would then overwrite the newly inserted row instead of rejecting the conflict.
To fix this, lock the row during validation using .with_for_update() so that any concurrent attempt to insert with the same primary key fails atomically, rather than silently corrupting data:
Safer save-path sketch
- model_by_id = MessageModel.query.filter_by(id=message_id).first()
+ model_by_id = MessageModel.query.filter_by(id=message_id).with_for_update().first()Then replace the separate insert/merge logic with:
- message_model_to_merge = MessageModel(
+ if message_model.id is not None:
+ existing_by_id = MessageModel.query.filter_by(id=message_model.id).with_for_update().first()
+ if existing_by_id is not None and (
+ existing_by_id.identifier != message_model.identifier
+ or existing_by_id.location != message_model.location
+ ):
+ raise ValueError("Supplied message_id belongs to a different message.")
+ if existing_by_id is not None:
+ existing_by_id.schema = message_model.schema
+ merged_message_model = existing_by_id
+ else:
+ message_model_to_merge = MessageModel(
- identifier=message_model.identifier,
- location=message_model.location,
- schema=message_model.schema,
- )
- if message_model.id is not None:
- message_model_to_merge.id = message_model.id
-
- merged_message_model: MessageModel = db.session.merge(message_model_to_merge)
+ identifier=message_model.identifier,
+ location=message_model.location,
+ schema=message_model.schema,
+ id=message_model.id,
+ )
+ db.session.add(message_model_to_merge)
+ merged_message_model = message_model_to_mergeAlso applies to: 94-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@spiffworkflow-backend/src/spiffworkflow_backend/services/message_definition_service.py`
around lines 25 - 33, The check that a supplied message_id belongs to the same
message must be made atomic with the save: when message_id is not None, SELECT
the MessageModel row for that id using a FOR UPDATE lock (e.g.,
MessageModel.query.filter_by(id=message_id).with_for_update().first()) inside
the same transaction so concurrent requests block; if the locked row exists and
identifier/location mismatch, raise ValueError; if it does not exist, proceed to
create/insert the new MessageModel (avoid db.session.merge() which can overwrite
a concurrent insert) — use db.session.add()/flush() and handle IntegrityError to
detect races and raise a conflict instead. Ensure the lock-and-save logic
replaces the separate validation (MessageModel lookup) and the later
db.session.merge() paths.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spiffworkflow-frontend/src/components/messages/MessageEditor.tsx (1)
312-346: Consider using Promise-based coordination for cleaner async handling.The manual coordination with flags (
processGroupLoaded,messageModelsLoaded) works but is verbose. The current approach correctly waits for both calls before initializing, and thefailureCallbackproperly handles message-models endpoint failures.However, the pattern of using mutable variables (
processGroupResult,messageModelsResult) that are captured in closures could be fragile if the effect re-runs before both callbacks complete. This is mitigated by the effect's dependency array, but worth noting.♻️ Optional: Consider Promise-based coordination
// Alternative using Promise wrapper (optional improvement) const fetchProcessGroup = () => new Promise<ProcessGroup>((resolve) => { HttpService.makeCallToBackend({ path: `/process-groups/${modifiedProcessGroupIdentifier}`, successCallback: resolve, }); }); const fetchMessageModels = () => new Promise<{ messages: MessageModelResponse[] }>((resolve) => { HttpService.makeCallToBackend({ path: `/message-models/${modifiedProcessGroupIdentifier}`, successCallback: resolve, failureCallback: () => resolve({ messages: [] }), }); }); Promise.all([fetchProcessGroup(), fetchMessageModels()]) .then(([processGroupResult, messageModelsResult]) => { processResult(processGroupResult, messageModelsResult); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spiffworkflow-frontend/src/components/messages/MessageEditor.tsx` around lines 312 - 346, Replace the manual flag-and-closure coordination with Promise-based coordination: wrap the two HttpService.makeCallToBackend calls in small promises (e.g., fetchProcessGroup and fetchMessageModels that use modifiedProcessGroupIdentifier), have fetchMessageModels resolve to { messages: [] } on failure, then call Promise.all([fetchProcessGroup(), fetchMessageModels()]).then(([processGroupResult, messageModelsResult]) => processResult(processGroupResult, messageModelsResult)); this removes the mutable flags/processGroupResult/messageModelsResult capture and keeps the same failure behavior for message models.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spiffworkflow-frontend/src/components/messages/MessageEditor.tsx`:
- Around line 312-346: Replace the manual flag-and-closure coordination with
Promise-based coordination: wrap the two HttpService.makeCallToBackend calls in
small promises (e.g., fetchProcessGroup and fetchMessageModels that use
modifiedProcessGroupIdentifier), have fetchMessageModels resolve to { messages:
[] } on failure, then call Promise.all([fetchProcessGroup(),
fetchMessageModels()]).then(([processGroupResult, messageModelsResult]) =>
processResult(processGroupResult, messageModelsResult)); this removes the
mutable flags/processGroupResult/messageModelsResult capture and keeps the same
failure behavior for message models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 457a9e08-a04d-4590-8871-f05a700b6f7c
📒 Files selected for processing (2)
spiffworkflow-frontend/src/components/messages/MessageEditor.test.tsxspiffworkflow-frontend/src/components/messages/MessageEditor.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
spiffworkflow-frontend/src/components/messages/MessageHelper.tsx (1)
9-15:⚠️ Potential issue | 🟡 MinorRoot candidate still won’t match descendant paths.
Line 13 does not handle
normalizedCandidate === ''(root), so root-scoped shared messages are excluded for non-rootcurrentLocation.Possible fix
const matchingLocations = candidateLocations.filter((candidateLocation) => { const normalizedCandidate = candidateLocation.replace(/(^\/+|\/+$)/g, ''); return ( + normalizedCandidate === '' || normalizedCurrentLocation === normalizedCandidate || normalizedCurrentLocation.startsWith(`${normalizedCandidate}/`) ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spiffworkflow-frontend/src/components/messages/MessageHelper.tsx` around lines 9 - 15, The filter in matchingLocations excludes root-scoped candidates because normalizedCandidate === '' isn't handled; update the predicate in the candidateLocations.filter used in matchingLocations so it returns true when normalizedCandidate === '' (root), or when normalizedCurrentLocation equals normalizedCandidate, or when normalizedCurrentLocation.startsWith(`${normalizedCandidate}/`); reference normalizedCandidate, normalizedCurrentLocation and the matchingLocations variable when making this change.
🧹 Nitpick comments (1)
spiffworkflow-frontend/src/components/messages/MessageHelper.tsx (1)
21-23: Prefer normalized-length ranking for nearest ancestor selection.Sorting by raw string length can be skewed by leading/trailing slashes. Rank by normalized path length instead.
Possible refactor
- return matchingLocations.sort( - (a: string, b: string) => b.length - a.length, - )[0]; + const normalize = (value: string) => value.replace(/(^\/+|\/+$)/g, ''); + return matchingLocations.sort( + (a: string, b: string) => normalize(b).length - normalize(a).length, + )[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spiffworkflow-frontend/src/components/messages/MessageHelper.tsx` around lines 21 - 23, The selection currently sorts matchingLocations by raw string length (matchingLocations.sort(...)[0]) which is biased by leading/trailing slashes; change the comparator to rank by normalized path length instead: normalize each candidate (e.g., trim leading/trailing slashes and collapse duplicate slashes) and compare the lengths of those normalized strings in the sort callback so the longest normalized path is chosen; update the comparator used on matchingLocations.sort and return the first element after sorting by normalized length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spiffworkflow-frontend/src/components/messages/MessageHelper.tsx`:
- Around line 100-104: The current conditional uses a truthy check for
currentLocation which treats an empty string (root) as falsy and skips ancestor
lookup; change the condition in MessageHelper (the block using message,
currentLocation, findNearestAncestorLocation, and matchingMessageModels) to
explicitly allow empty-string locations by checking for null/undefined instead
(e.g., if (!message && currentLocation !== null && currentLocation !==
undefined) or if (!message && currentLocation != null) ) so the ancestor lookup
runs for root ('') values.
---
Duplicate comments:
In `@spiffworkflow-frontend/src/components/messages/MessageHelper.tsx`:
- Around line 9-15: The filter in matchingLocations excludes root-scoped
candidates because normalizedCandidate === '' isn't handled; update the
predicate in the candidateLocations.filter used in matchingLocations so it
returns true when normalizedCandidate === '' (root), or when
normalizedCurrentLocation equals normalizedCandidate, or when
normalizedCurrentLocation.startsWith(`${normalizedCandidate}/`); reference
normalizedCandidate, normalizedCurrentLocation and the matchingLocations
variable when making this change.
---
Nitpick comments:
In `@spiffworkflow-frontend/src/components/messages/MessageHelper.tsx`:
- Around line 21-23: The selection currently sorts matchingLocations by raw
string length (matchingLocations.sort(...)[0]) which is biased by
leading/trailing slashes; change the comparator to rank by normalized path
length instead: normalize each candidate (e.g., trim leading/trailing slashes
and collapse duplicate slashes) and compare the lengths of those normalized
strings in the sort callback so the longest normalized path is chosen; update
the comparator used on matchingLocations.sort and return the first element after
sorting by normalized length.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d89540b-5a2d-4185-9d0d-3f9ef04303ae
📒 Files selected for processing (3)
spiffworkflow-frontend/src/components/messages/MessageEditor.test.tsxspiffworkflow-frontend/src/components/messages/MessageEditor.tsxspiffworkflow-frontend/src/components/messages/MessageHelper.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- spiffworkflow-frontend/src/components/messages/MessageEditor.test.tsx
- spiffworkflow-frontend/src/components/messages/MessageEditor.tsx
# Conflicts: # spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_data_setup_service.py # spiffworkflow-frontend/Dockerfile
# Conflicts: # spiffworkflow-backend/pyproject.toml # spiffworkflow-backend/src/spiffworkflow_backend/routes/process_groups_controller.py # spiffworkflow-backend/uv.lock # spiffworkflow-frontend/package-lock.json
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
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.
No description provided.