Skip to content

fix(button): share functionality fixing for collaborative editing - I297#298

Merged
DianaLease merged 13 commits intoaccordproject:mainfrom
teja-pola:teja/i297/Share-Functionality-for-Collaborative-editing
Mar 25, 2025
Merged

fix(button): share functionality fixing for collaborative editing - I297#298
DianaLease merged 13 commits intoaccordproject:mainfrom
teja-pola:teja/i297/Share-Functionality-for-Collaborative-editing

Conversation

@teja-pola
Copy link
Contributor

@teja-pola teja-pola commented Mar 18, 2025

Closes #297

Partially closes #134

This PR resolves the client-side issues with the share functionality in Template Playground, ensuring shared links render the full editor state locally for collaborative editing. The changes align the URL generation with the root route and optimize initialization to prevent partial renders.

Changes

Content.tsx

  1. Removed : Promise<void> return type and void operator from loadContent, as TypeScript infers it, improving code clarity.
  2. Changed content to contentData inside try block to avoid shadowing the state variable.

UseShare.tsx

  1. Removed unnecessary void from message.success and message.error calls, as they’re fire-and-forget operations.
  2. Retained async with proper await for navigator.clipboard.writeText, ensuring clipboard completion before success message.

store.ts

  1. Share Link Path Issue: Original generateShareableLink used /v1?data=..., mismatching the root route (/), causing partial renders (navbar/footer only).
  2. Initialization Tracking: isInitialized existed but wasn’t fully utilized to ensure state readiness before App.tsx rendering.
  3. Corrected Share Link Path: Updated generateShareableLink to ${window.location.origin}?data=${compressedData}, aligning with Route path="/".
  4. State Update Fix: Moved data update inside setData’s try block, ensuring it only updates on successful rebuild.

App.tsx

  1. Routing Mismatch: Shared links with ?data= didn’t redirect to / if on a different path (e.g., /learn), showing only navbar/footer until manual navigation.
  2. Tour Timing: Tour ran before isInitialized was set, risking premature start before shared state loaded.
  3. Rendering Control (Forced Root Navigation): Added navigate("/", { replace: true }) when a shared link is detected, ensuring immediate full render.

Flags

  • Tested locally; works as expected. The 403 error on the deployed site persists, indicating a server-side issue for mentors to tackle.
  • No tests added yet—could use guidance on where to integrate them.

Screenshots or Video

Untitled.video.-.Made.with.Clipchamp.7.mp4

Related Issues

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

@teja-pola teja-pola requested a review from a team as a code owner March 18, 2025 20:01
@netlify
Copy link

netlify bot commented Mar 18, 2025

Deploy Preview for ap-template-playground ready!

Name Link
🔨 Latest commit c184f66
🔍 Latest deploy log https://app.netlify.com/sites/ap-template-playground/deploys/67e191697f023600094c27ef
😎 Deploy Preview https://deploy-preview-298--ap-template-playground.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
@teja-pola teja-pola force-pushed the teja/i297/Share-Functionality-for-Collaborative-editing branch from 1fd0dcc to c1394bf Compare March 18, 2025 20:08
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
@teja-pola
Copy link
Contributor Author

@DianaLease @sanketshevkar

I have worked on the Share Functionality for Collaborative Editing, and it's working smoothly now. Given its impact on real-time collaboration, I believe this is a highly valuable feature that should be prioritized. Could you please review the implementation and share your feedback?

@teja-pola
Copy link
Contributor Author

@DianaLease @sanketshevkar

I have worked on the Share Functionality for Collaborative Editing, and it's working smoothly now. Given its impact on real-time collaboration, I believe this is a highly valuable feature that should be prioritized. Could you please review the implementation and share your feedback?

I’ve identified some issue in the client side and cleaned up the client-side code, and the share functionality now works perfectly on locally and deployment too. Shared links generate correctly (e.g., http://localhost:5173/?data=...), and opening them renders the full editor state—including Navbar, Footer, and all editor panels—without requiring manual navigation. This confirms the client-side logic is solid.

However, if the 403 Forbidden error still persists on the deployed site template-playground.accordproject.org, indicating a server-side issue. This is likely beyond contributor scope and needs mentor attention.
Likely Causes:
CloudFront origin misconfiguration
CORS headers missing for lz-string payloads
WAF rules blocking encoded data
API Gateway/Lambda timeout or size limits

teja-pola and others added 6 commits March 24, 2025 22:35
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Signed-off-by: Dharma Teja <dteja2468@gmail.com>
Comment on lines -15 to -31
editorValue: string;
modelCto: string;
editorModelCto: string;
data: string;
editorAgreementData: string;
agreementHtml: string;
error: string | undefined;
samples: Array<Sample>;
sampleName: string;
backgroundColor: string;
textColor: string;
setTemplateMarkdown: (template: string) => Promise<void>;
setEditorValue: (value: string) => void;
setModelCto: (model: string) => Promise<void>;
setEditorModelCto: (value: string) => void;
setData: (data: string) => Promise<void>;
setEditorAgreementData: (value: string) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were the editor states removed from the app state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DianaLease Thanks for reviewing! I removed editorValue, editorModelCto, and editorAgreementData from store.ts cuz this reduces complexity in syncing editor content and streamline state management. Previously, these duplicate states tracked editor content separately from templateMarkdown, modelCto, and data, which led to sync issues—shared links sometimes reflected stale or partial states because rebuild operated on the processed fields, not the editor ones.

Now Unified them, the editors now directly update templateMarkdown, modelCto, and data, which generateShareableLink compresses into the URL. This ensures shared links always capture the latest editor content, aligning with the root route (/) and preventing partial renders (navbar/footer only), as reported in #297.

One trade-off is that we lose a buffer for unsaved changes if rebuild fails, but the debounced setters mitigate this by delaying updates until valid. I think this simplification is worth it for reliable sharing, What do you think—should I restore them for that use case, or is this approach sufficient for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DianaLease If this sounds good. I would like you to merge this and see whether this fixes the share functionality in the main site https://playground.accordproject.org/ and could possibly revert the merge if it doesn't.

};
const compressedData = compress(dataToShare);
return `${window.location.origin}?data=${compressedData}`;
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vinyl-Davyl Do you remember why we had v1 in this url in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure why /v1 was there initially—maybe it was intended for versioning or a server-side route? Since the share functionality is client-side (compressing and loading state locally), the root path works perfectly now, as shown in the demo video. The 403 error on the deployed site (template-playground.accordproject.org) suggests a server-side expectation of /v1, but that’s flagged as a separate issue for mentors to handle. @Vinyl-Davyl, if /v1 had a specific purpose (e.g., future API integration), I can adjust this back and tweak App.tsx to handle it differently—let me know!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vinyl-Davyl Do you remember why we had v1 in this url in the first place?

Yeah, I remember this was the reason. #298 (comment)

@Vinyl-Davyl
Copy link
Collaborator

#298 (comment)

Yeah @DianaLease we had the versioning added while I was building this based off reasons not to use the template archive JSON at the time. So links not to support generation with the format in addition to template archives due to time constraint. As you would probably notice in the loadFromLink: async (compressedData: string) in the /store file

Based off mutual agreement with @sanketshevkar and @mttrbrts at the time we suggested looking into versioning the URLs. We started with v1 having a simple object compressed and added to URL(what we have currently). And probably introduce v2 in future to support template archive JSON object format.

Of course, We can always go back to no version if there is agreement not to go into considerations for template archive JSON objects for shareables in future at this point.

@teja-pola
Copy link
Contributor Author

teja-pola commented Mar 24, 2025

Given Vinyl-Davyl’s note, we could keep the root path if template archive support isn’t imminent, or revert to /v1 and adjust App.tsx to handle it if versioning is still a priority. I lean toward merging as-is to test the fix live, then revisiting versioning later—what’s your take @DianaLease ?

Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, tested in the netlify preview for the PR and it is working as expected there 👍

@DianaLease DianaLease merged commit 3e9e825 into accordproject:main Mar 25, 2025
7 checks passed
@nitro56565
Copy link
Contributor

@DianaLease It would work on netlify deploy because the URL is not restricted on netlify deploy on the size of url, can you check once on the playground as well because I still see the same issue.

@teja-pola
Copy link
Contributor Author

Yes you were right @nitro56565. This issue just solved the generated link mismatch. And the issue still persists because of the size of the url. Try testing in the playground changing the sample to Formula Now and it works cause the url generated there is much smaller than the Customer Order. It works only now cause of this url mismatch being solved. So I would like @DianaLease to look into another pull request that solved the too long url by tinyurl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants